Change in coreboot[master]: mb/google/kaisa: Override power limits configuration

[David Wu (Code Review)]

David Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44805 )

Change subject: mb/google/kaisa: Override power limits configuration
......................................................................

mb/google/kaisa: Override power limits configuration

Override SysPl2, SysPl3 and Pl4 values, based on our experiment results.

BUG=b:160676773
TEST=Built and check firmware log.

Signed-off-by: David Wu 
Change-Id: I4b621a8cc1749ee52a9f16a7ad2ae7a7aa0f7a5a
---
M src/mainboard/google/hatch/variants/kaisa/Makefile.inc
A src/mainboard/google/hatch/variants/kaisa/mainboard.c
2 files changed, 58 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/44805/1

diff --git a/src/mainboard/google/hatch/variants/kaisa/Makefile.inc b/src/mainboard/google/hatch/variants/kaisa/Makefile.inc
index 3b5b7d0..2afd494 100644
--- a/src/mainboard/google/hatch/variants/kaisa/Makefile.inc
+++ b/src/mainboard/google/hatch/variants/kaisa/Makefile.inc
@@ -1,4 +1,5 @@
 ## SPDX-License-Identifier: GPL-2.0-only

ramstage-y += gpio.c
+ramstage-y += mainboard.c
 bootblock-y += gpio.c
diff --git a/src/mainboard/google/hatch/variants/kaisa/mainboard.c b/src/mainboard/google/hatch/variants/kaisa/mainboard.c
new file mode 100644
index 0000000..9b0089b
--- /dev/null
+++ b/src/mainboard/google/hatch/variants/kaisa/mainboard.c
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <baseboard/variants.h>
+#include <chip.h>
+#include <device/device.h>
+#include <device/pci_ids.h>
+#include <device/pci_ops.h>
+#include <ec/google/chromeec/ec.h>
+#include <intelblocks/power_limit.h>
+#include <soc/pci_devs.h>
+
+/*
+ * For type-C chargers, set PSYSPL2 to 97% of max power.
+ */
+#define SET_PSYSPL2(w)               (97 * (w) / 100)
+
+/*
+ * mainboard_override_power_limits
+ *
+ * Override SysPl2, SysPl3 and Pl4 values.
+ * n = max value of power adapter.
+ * For USB C charger:
+ * +-------------+---------+---------+-------+
+ * | Max Power(W)| PsysPL2 | PsysPL3 |  PL4  |
+ * +-------------+---------+---------+-------+
+ * | n           |  0.97n  |  0.97n  | 0.97n |
+ * +-------------+---------+---------+-------+
+ */
+
+static void mainboard_override_power_limits(struct soc_power_limits_config *conf)
+{
+	enum usb_chg_type type;
+	u32 watts;
+	u16 volts_mv, current_ma;
+	u32 psyspl2;
+	int rv = google_chromeec_get_usb_pd_power_info(&type, &current_ma, &volts_mv);
+
+	if (rv == 0 && type == USB_CHG_TYPE_PD) {
+		/* Detected USB-PD.  Base on max value of adapter */
+		watts = ((u32)current_ma * volts_mv) / 1000000;
+		/* set psyspl2 to 97% of adapter rating */
+		psyspl2 = SET_PSYSPL2(watts);
+
+		conf->tdp_psyspl2 = psyspl2;
+		conf->tdp_psyspl3 = psyspl2;
+		conf->tdp_pl4 = psyspl2;
+	}
+}
+
+void variant_mainboard_enable(struct device *dev)
+{
+	struct soc_power_limits_config *soc_config;
+	config_t *conf = config_of_soc();
+
+	soc_config = &conf->power_limits_config;
+	mainboard_override_power_limits(soc_config);
+}

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44805 )

Change subject: mb/google/kaisa: Override power limits configuration
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44805/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44805/1//COMMIT_MSG@9 
PS1, Line 9: experiment results
What was the experiment?

[David Wu (Code Review)]

David Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44805 )

Change subject: mb/google/kaisa: Override power limits configuration
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44805/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44805/1//COMMIT_MSG@9 
PS1, Line 9: experiment results
> What was the experiment?

Please refer to the test result(Excel file) in comment#30
https://partnerissuetracker.corp.google.com/issues/160676773#comment30

Before:
* For USB C charger:
 * +-------------+-----------------+---------+---------+-------+
 * | Max Power(W)|       PL2       | PsysPL2 | PsysPL3 |  PL4  |
 * +-------------+-----------------+---------+---------+-------+
 * | n           |  min(0.9n, PL2) |   0.9n  |  0.9n   | 0.9n  |
 * +-------------+-----------------+---------+---------+-------+

After:
* For USB C charger:
 * +-------------+-----------------+---------+---------+-------+
 * | Max Power(W)|       PL2       | PsysPL2 | PsysPL3 |  PL4  |
 * +-------------+-----------------+---------+---------+-------+
 * | n           |  min(0.9n, PL2) |  0.97n  |  0.97n  | 0.97n |
 * +-------------+-----------------+---------+---------+-------+

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44805 )

Change subject: mb/google/kaisa: Override power limits configuration
......................................................................

Patch Set 1: Code-Review-2

Hi David,

So  this is a incorrect approch as we already have `src/mainboard/google/hatch/variants/baseboard/mainboard.c` shared between the boards.

What you probably need to do here to generalise it as parameteric on the variants is have each variant have a header with the `#define` differences. This way when each board is compiled the common mainboard.c will get the correct constants.

[David Wu (Code Review)]

David Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44805 )

Change subject: mb/google/kaisa: Override power limits configuration
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-2
>
> Hi David,
>
> So  this is a incorrect approch as we already have `src/mainboard/google/hatch/variants/baseboard/mainboard.c` shared between the boards.
>
> What you probably need to do here to generalise it as parameteric on the variants is have each variant have a header with the `#define` differences. This way when each board is compiled the common mainboard.c will get the correct constants.

Sorry, I don't understand what you mean. could you share sample code for my reference? or would you help to update this CL? Thanks.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44805 )

Change subject: mb/google/kaisa: Override power limits configuration
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1: Code-Review-2
> >
> > Hi David,
> >
> > So  this is a incorrect approch as we already have `src/mainboard/google/hatch/variants/baseboard/mainboard.c` shared between the boards.
> >
> > What you probably need to do here to generalise it as parameteric on the variants is have each variant have a header with the `#define` differences. This way when each board is compiled the common mainboard.c will get the correct constants.
>
> Sorry, I don't understand what you mean. could you share sample code for my reference? or would you help to update this CL? Thanks.

Look in `src/mainboard/google/hatch/variants/baseboard/mainboard.c` that is shared between Puff boards. All you are doing here is overriding the common code with duplicate implementations in each variant. Fix the common code in `src/mainboard/google/hatch/variants/baseboard/mainboard.c`

Change in coreboot[master]: mb/google/puff: Update psyspl2 to 97% of adapter rating

[David Wu (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Edward O'Callaghan, Andrew McRae,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/44805

to look at the new patch set (#2).

Change subject: mb/google/puff: Update psyspl2 to 97% of adapter rating
......................................................................

mb/google/puff: Update psyspl2 to 97% of adapter rating

Set psyspl2 to 97% of adapter rating, based on our experiment results.

BUG=b:160676773
TEST=Built and check firmware log.

Signed-off-by: David Wu 
Change-Id: I4b621a8cc1749ee52a9f16a7ad2ae7a7aa0f7a5a
---
M src/mainboard/google/hatch/variants/baseboard/mainboard.c
1 file changed, 3 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/44805/2

Change in coreboot[master]: mb/google/puff: Update psyspl2 to 97% of adapter rating

[David Wu (Code Review)]

David Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44805 )

Change subject: mb/google/puff: Update psyspl2 to 97% of adapter rating
......................................................................

Patch Set 2:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1: Code-Review-2
> > >
> > > Hi David,
> > >
> > > So  this is a incorrect approch as we already have `src/mainboard/google/hatch/variants/baseboard/mainboard.c` shared between the boards.
> > >
> > > What you probably need to do here to generalise it as parameteric on the variants is have each variant have a header with the `#define` differences. This way when each board is compiled the common mainboard.c will get the correct constants.
> >
> > Sorry, I don't understand what you mean. could you share sample code for my reference? or would you help to update this CL? Thanks.
>
> Look in `src/mainboard/google/hatch/variants/baseboard/mainboard.c` that is shared between Puff boards. All you are doing here is overriding the common code with duplicate implementations in each variant. Fix the common code in `src/mainboard/google/hatch/variants/baseboard/mainboard.c`

Done. Thank you so much.

Change in coreboot[master]: mb/google/puff: Update psyspl2 to 97% of adapter rating

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44805 )

Change subject: mb/google/puff: Update psyspl2 to 97% of adapter rating
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: mb/google/puff: Update psyspl2 to 97% of adapter rating

[Sam McNally (Code Review)]

Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44805 )

Change subject: mb/google/puff: Update psyspl2 to 97% of adapter rating
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: mb/google/puff: Update psyspl2 to 97% of adapter rating

[Daniel Kurtz (Code Review)]

Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44805 )

Change subject: mb/google/puff: Update psyspl2 to 97% of adapter rating
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: mb/google/puff: Update psyspl2 to 97% of adapter rating

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44805 )

Change subject: mb/google/puff: Update psyspl2 to 97% of adapter rating
......................................................................

mb/google/puff: Update psyspl2 to 97% of adapter rating

Set psyspl2 to 97% of adapter rating, based on our experiment results.

BUG=b:160676773
TEST=Built and check firmware log.

Signed-off-by: David Wu 
Change-Id: I4b621a8cc1749ee52a9f16a7ad2ae7a7aa0f7a5a
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44805
Tested-by: build bot (Jenkins) 
Reviewed-by: Edward O'Callaghan 
Reviewed-by: Sam McNally 
Reviewed-by: Daniel Kurtz 
---
M src/mainboard/google/hatch/variants/baseboard/mainboard.c
1 file changed, 3 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Daniel Kurtz: Looks good to me, approved
  Edward O'Callaghan: Looks good to me, approved
  Sam McNally: Looks good to me, approved

diff --git a/src/mainboard/google/hatch/variants/baseboard/mainboard.c b/src/mainboard/google/hatch/variants/baseboard/mainboard.c
index 537e3df..3b247e9 100644
--- a/src/mainboard/google/hatch/variants/baseboard/mainboard.c
+++ b/src/mainboard/google/hatch/variants/baseboard/mainboard.c
@@ -35,10 +35,10 @@
 }

/*
- * For type-C chargers, set PL2 to 90% of max power to account for
+ * For type-C chargers, set PL2 to 97% of max power to account for
  * cable loss and FET Rdson loss in the path from the source.
  */
-#define SET_PSYSPL2(w)               (9 * (w) / 10)
+#define SET_PSYSPL2(w)               (97 * (w) / 100)
 #define PUFF_U22_PL2                 (35)
 #define PUFF_U62_U42_PL2             (51)
 #define PUFF_CELERON_PENTIUM_PSYSPL2 (65)
@@ -65,7 +65,7 @@
  * +-------------+-----------------+---------+---------+-------+
  * | Max Power(W)|       PL2       | PsysPL2 | PsysPL3 |  PL4  |
  * +-------------+-----+-----------+---------+---------+-------+
- * | n           |  min(0.9n, PL2) |   0.9n  |  0.9n   | 0.9n  |
+ * | n           | min(0.97n, PL2) |  0.97n  |  0.97n  | 0.97n |
  * +-------------+-----+-----------+---------+---------+-------+
  */