Change in coreboot[master]: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Gaggery Tsai (Code Review)

25 Mar 2020 25 Mar '20

9:23 p.m.

Gaggery Tsai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39850 )

Change subject: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

[WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation

BUG=b:151972149 TEST=pending on test result

Signed-off-by: Gaggery Tsai gaggery.tsai@intel.com Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 --- M src/mainboard/google/hatch/variants/puff/mainboard.c 1 file changed, 22 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/39850/1

diff --git a/src/mainboard/google/hatch/variants/puff/mainboard.c b/src/mainboard/google/hatch/variants/puff/mainboard.c index f00f47f..518da42 100644 --- a/src/mainboard/google/hatch/variants/puff/mainboard.c +++ b/src/mainboard/google/hatch/variants/puff/mainboard.c @@ -97,6 +97,23 @@ * | n (U22) | 29 | .9n | .9n | x(43) | * +-------------+-----+---------+---------+-------+ */ + +/* + * Psys_pmax consideratins + * Given the hardware design in puff, the serial shunt resistor is 0.01ohm + * The full scale of hardware PSYS signal 0.8v maps to system current 9.58A + * instead of real system power. The equestion is shown below: + * PSYS = 0.8v = (0.01ohm x Iinput) x 50 (INA213, gain 50V/V) x 15k/(15k + 75k) + * Hence, Iinput (Amps) = 9.58A + * Since there is no voltage information from PSYS, different voltage input + * would map to different Psys_pmax settings: + * For Type-C 15V, the Psys_pmax sholud be 15v x 9.58A = 143.7W + * For Type-C 20V, the Psys_pmax should be 20v x 9.58A = 191.6W + * For a barral jack, the Psys_pmax should be 19v x 9.58A = 182W + */ +#define PSYS_IMAX_MINIAMPS 9580 +#define BJ_MINIVOLTS 19000 + static void mainboard_set_power_limits(config_t *conf) { enum usb_chg_type type; @@ -118,8 +135,12 @@ /* set minimum duty cycle */ conf->tdp_psyspl3_dutycycle = PUFF_MIN_DUTYCYCLE; conf->tdp_pl4 = SET_PSYSPL2(psyspl2); + } else { + /* Input type is barrel jack */ + mini_volts = BJ_MINIVOLTS; } - + + conf->psys_pmax = (u16)(((u32)PSYS_IMAX_MINIAMPS * mini_volts)/1000000); conf->tdp_pl2_override = PUFF_PL2; /* set psyspl2 to 90% of max adapter power */ conf->tdp_psyspl2 = SET_PSYSPL2(psyspl2);

Attachments:

attachment.htm (text/html — 5.2 KB)

Show replies by date

build bot (Jenkins) (Code Review)

25 Mar 25 Mar

9:23 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39850 )

Change subject: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39850/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:

https://review.coreboot.org/c/coreboot/+/39850/1/src/mainboard/google/hatch/... PS1, Line 142: trailing whitespace

Gaggery Tsai (Code Review)

9:27 p.m.

Hello build bot (Jenkins),

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

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

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

Change subject: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

[WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation

BUG=b:151972149 TEST=pending on test result

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/39850/2

Harry Pan (Code Review)

26 Mar 26 Mar

1:44 a.m.

Harry Pan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39850 )

Change subject: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39850/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:

https://review.coreboot.org/c/coreboot/+/39850/2/src/mainboard/google/hatch/... PS2, Line 102: consideratins considerations?

Gaggery Tsai (Code Review)

11:37 p.m.

Hello build bot (Jenkins),

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

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

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

Change subject: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

[WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation

This patch adds psys_pmax calculation. There are two types of power sources. One is barrel jack and the other is USB TYPE-C. The voltage level is fixed for a barrel jack while TYPE-C maybe varies which dependnets on power ratings. We need to get voltage infomration from EC and calculate correct psys_pmax value. The psys_pmax needs to be set before FSP-S since FSP-S will handle the setting passing to pcode, so move the routine ahead to variant_ramstage_init.

BUG=b:151972149 TEST=pending on test result

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/39850/3

Gaggery Tsai (Code Review)

11:41 p.m.

Hello build bot (Jenkins),

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

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

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

Change subject: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

[WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation

BUG=b:151972149 TEST=pending on test result

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/39850/4

Gaggery Tsai (Code Review)

11:42 p.m.

Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39850 )

Change subject: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39850/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:

https://review.coreboot.org/c/coreboot/+/39850/2/src/mainboard/google/hatch/... PS2, Line 102: consideratins

...

considerations?

done.

Harry Pan (Code Review)

27 Mar 27 Mar

6:10 a.m.

Harry Pan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39850 )

Change subject: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 4: Code-Review+1

Verified on BJ90W, TC65W, and TC45W, they all works well (mostly likely).

That said, I noticed there is a corner case in cold boot (1st plug or ectool reboot_ec cold) that the API (google_chromeec_get_usb_pd_power_info) returns type in 9 (USB_CHG_TYPE_UNKNOWN) [1]; this is happening in all adapters I tested (BJ90W/TC65W/TC45W); when we hit such case, the psys from MSR would incorrectly increasing against the external gauge.

[1] firmware log: HP: rv=0 type=9 psys_pmax=182

Harry Pan (Code Review)

31 Mar 31 Mar

5:12 p.m.

Harry Pan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39850 )

Change subject: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 4:

Please ignore my previous comment of USB_CHG_TYPE_UNKNOWN. It could be my faulty environment, and I am unable to see this odd now.

Paul Menzel (Code Review)

1 Apr 1 Apr

9:39 a.m.

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

Change subject: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 4:

(12 comments)

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG@12 PS4, Line 12: dependnets depends

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG@11 PS4, Line 11: maybe varies which : dependnets on power ratings … can vary depending on the power ratings

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG@12 PS4, Line 12: infomration information

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG@14 PS4, Line 14: pcode What is that?

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 87: * Given the hardware design in puff, the serial shunt resistor is 0.01ohm Please add a dot/period at the end of sentences.

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 90: 15k What is the unit *k*?

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 91: * Hence, Iinput (Amps) = 9.58A Sorry my ignorance, I am no engineer, but I get 9.6 A.

...

Iinput = 0.8 * 100 / 50 / 15 * 90 A = 0.8 * 2 * 6 = 9.6 A

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 98: Please use exactly one space and no tab.

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 127: / Please add spaces around the operator.

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 139: gpio_configure_pads. */ Fits on one line in 96 characters.

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 144: && !gpio_get(GPIO_DP_HPD)) { One line? Only copied, so fix in a commit before or in a follow-up?

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 149: setup set up

Gaggery Tsai (Code Review)

7:59 p.m.

Gaggery Tsai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39850 )

Change subject: [WIP]/mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 4:

(12 comments)

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG@12 PS4, Line 12: dependnets

...

depends

Ack

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG@11 PS4, Line 11: maybe varies which : dependnets on power ratings

...

… can vary depending on the power ratings

Ack

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG@12 PS4, Line 12: infomration

...

information

Ack

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG@14 PS4, Line 14: pcode

...

What is that?

Pcode is like a microprocessor who manage the power like PL1, PL2, PsysPLx and etc.

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 87: * Given the hardware design in puff, the serial shunt resistor is 0.01ohm

...

Please add a dot/period at the end of sentences.

Ack

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 90: 15k

...

What is the unit *k*?

k is kilo, 1000. The unit here is ohms.

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 91: * Hence, Iinput (Amps) = 9.58A

...

Sorry my ignorance, I am no engineer, but I get 9.6 A. […]

Ack

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 98:

...

Please use exactly one space and no tab.

Ack

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 127: /

...

Please add spaces around the operator.

Ack

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 139: gpio_configure_pads. */

...

Fits on one line in 96 characters.

Ack

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 144: && !gpio_get(GPIO_DP_HPD)) {

...

One line? Only copied, so fix in a commit before or in a follow-up?

I like the original style, it's clear to me.

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 149: setup

...

set up

Ack

Gaggery Tsai (Code Review)

8:29 p.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

Hello build bot (Jenkins), Harry Pan,

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

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

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

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

mb/google/hatch/vr/puff: Add psys_pmax calculation

BUG=b:151972149 TEST=emerge-puff coreboot chromeos-bootimage check firmware log and ensure psys_pmax is passed to FSP check the data from dump_intel_rapl_consumption in the OS and ensure the power data is close to an external power meter.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/39850/5

Gaggery Tsai (Code Review)

8:35 p.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

Hello build bot (Jenkins), Harry Pan,

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

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

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

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

mb/google/hatch/vr/puff: Add psys_pmax calculation

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/39850/6

Jamie Chen (Code Review)

6 Apr 6 Apr

4:16 a.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

Jamie Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39850 )

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 6: Code-Review+1

Edward O'Callaghan (Code Review)

4:24 a.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

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

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 6: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/39850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 Gerrit-Change-Number: 39850 Gerrit-PatchSet: 6 Gerrit-Owner: Gaggery Tsai gaggery.tsai@intel.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Harry Pan harry.pan@intel.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Shelley Chen shchen@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 06 Apr 2020 02:24:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Paul Menzel (Code Review)

5:25 p.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

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

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 90: 15k

...

k is kilo, 1000. The unit here is ohms.

Could you add the unit despite it cancelling each other out?

-- To view, visit https://review.coreboot.org/c/coreboot/+/39850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 Gerrit-Change-Number: 39850 Gerrit-PatchSet: 6 Gerrit-Owner: Gaggery Tsai gaggery.tsai@intel.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Harry Pan harry.pan@intel.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Shelley Chen shchen@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 06 Apr 2020 15:25:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Gaggery Tsai gaggery.tsai@intel.com Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

Paul Menzel (Code Review)

5:28 p.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

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

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39850/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:

https://review.coreboot.org/c/coreboot/+/39850/6/src/mainboard/google/hatch/... PS6, Line 105: u16 volts_milli; In a follow-up the variable names should be cleaned up.

`vlimit_mv` or s,o and the same for `watt`.

-- To view, visit https://review.coreboot.org/c/coreboot/+/39850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 Gerrit-Change-Number: 39850 Gerrit-PatchSet: 6 Gerrit-Owner: Gaggery Tsai gaggery.tsai@intel.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Harry Pan harry.pan@intel.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Shelley Chen shchen@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 06 Apr 2020 15:28:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Gaggery Tsai (Code Review)

15 Apr 15 Apr

3:41 a.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

Hello Shelley Chen, build bot (Jenkins), Jamie Chen, Furquan Shaikh, Kangheui Won, Harry Pan, Edward O'Callaghan, Gaggery Tsai,

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

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

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

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

mb/google/hatch/vr/puff: Add psys_pmax calculation

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/39850/7

-- To view, visit https://review.coreboot.org/c/coreboot/+/39850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 Gerrit-Change-Number: 39850 Gerrit-PatchSet: 7 Gerrit-Owner: Gaggery Tsai gaggery.tsai@intel.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Gaggery Tsai gaggery.tsai@intel.corp-partner.google.com Gerrit-Reviewer: Harry Pan harry.pan@intel.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Shelley Chen shchen@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Edward O'Callaghan (Code Review)

4:28 a.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

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

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 7: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/39850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 Gerrit-Change-Number: 39850 Gerrit-PatchSet: 7 Gerrit-Owner: Gaggery Tsai gaggery.tsai@intel.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Gaggery Tsai gaggery.tsai@intel.corp-partner.google.com Gerrit-Reviewer: Harry Pan harry.pan@intel.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Shelley Chen shchen@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 15 Apr 2020 02:28:41 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Gaggery Tsai (Code Review)

5:42 a.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

Hello Shelley Chen, build bot (Jenkins), Jamie Chen, Furquan Shaikh, Kangheui Won, Harry Pan, Edward O'Callaghan, Gaggery Tsai,

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

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

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

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

mb/google/hatch/vr/puff: Add psys_pmax calculation

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/39850/8

-- To view, visit https://review.coreboot.org/c/coreboot/+/39850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 Gerrit-Change-Number: 39850 Gerrit-PatchSet: 8 Gerrit-Owner: Gaggery Tsai gaggery.tsai@intel.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Gaggery Tsai gaggery.tsai@intel.corp-partner.google.com Gerrit-Reviewer: Harry Pan harry.pan@intel.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Shelley Chen shchen@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Patrick Georgi (Code Review)

20 Apr 20 Apr

8:49 a.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

Patrick Georgi has uploaded a new patch set (#9) to the change originally created by Gaggery Tsai. ( https://review.coreboot.org/c/coreboot/+/39850 )

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

mb/google/hatch/vr/puff: Add psys_pmax calculation

This patch adds psys_pmax calculation. There are two types of power sources. One is barrel jack and the other is USB TYPE-C. The voltage level is fixed for a barrel jack while TYPE-C may vary depending on power ratings. We need to get voltage information from EC and calculate correct psys_pmax value. The psys_pmax needs to be set before FSP-S since FSP-S will handle the setting passing to pcode, so move the routine ahead to variant_ramstage_init.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/39850/9

-- To view, visit https://review.coreboot.org/c/coreboot/+/39850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 Gerrit-Change-Number: 39850 Gerrit-PatchSet: 9 Gerrit-Owner: Gaggery Tsai gaggery.tsai@intel.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Gaggery Tsai gaggery.tsai@intel.corp-partner.google.com Gerrit-Reviewer: Harry Pan harry.pan@intel.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Shelley Chen shchen@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Patrick Georgi (Code Review)

8:49 a.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39850 )

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/39850/4//COMMIT_MSG@14 PS4, Line 14: pcode

...

Pcode is like a microprocessor who manage the power like PL1, PL2, PsysPLx and etc.

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/39850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 Gerrit-Change-Number: 39850 Gerrit-PatchSet: 9 Gerrit-Owner: Gaggery Tsai gaggery.tsai@intel.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Gaggery Tsai gaggery.tsai@intel.corp-partner.google.com Gerrit-Reviewer: Harry Pan harry.pan@intel.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Shelley Chen shchen@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 20 Apr 2020 06:49:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Gaggery Tsai gaggery.tsai@intel.com Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

8:57 a.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

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

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

Patch Set 9:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/mainboard.c:

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 90: 15k

...

Could you add the unit despite it cancelling each other out?

I think this is pretty obvious.

https://review.coreboot.org/c/coreboot/+/39850/4/src/mainboard/google/hatch/... PS4, Line 144: && !gpio_get(GPIO_DP_HPD)) {

...

I like the original style, it's clear to me.

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/39850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 Gerrit-Change-Number: 39850 Gerrit-PatchSet: 9 Gerrit-Owner: Gaggery Tsai gaggery.tsai@intel.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Gaggery Tsai gaggery.tsai@intel.corp-partner.google.com Gerrit-Reviewer: Harry Pan harry.pan@intel.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Shelley Chen shchen@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 20 Apr 2020 06:57:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Gaggery Tsai gaggery.tsai@intel.com Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

8:57 a.m.

New subject: Change in coreboot[master]: mb/google/hatch/vr/puff: Add psys_pmax calculation

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

Change subject: mb/google/hatch/vr/puff: Add psys_pmax calculation ......................................................................

mb/google/hatch/vr/puff: Add psys_pmax calculation

This patch adds psys_pmax calculation. There are two types of power sources. One is barrel jack and the other is USB TYPE-C. The voltage level is fixed for a barrel jack while TYPE-C may vary depending on power ratings. We need to get voltage information from EC and calculate correct psys_pmax value. The psys_pmax needs to be set before FSP-S since FSP-S will handle the setting passing to pcode, so move the routine ahead to variant_ramstage_init.

Signed-off-by: Gaggery Tsai gaggery.tsai@intel.com Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 Reviewed-on: https://review.coreboot.org/c/coreboot/+/39850 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Edward O'Callaghan quasisec@chromium.org --- M src/mainboard/google/hatch/variants/puff/mainboard.c 1 file changed, 37 insertions(+), 18 deletions(-)

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

diff --git a/src/mainboard/google/hatch/variants/puff/mainboard.c b/src/mainboard/google/hatch/variants/puff/mainboard.c index 3f74c96..ceeb0c5 100644 --- a/src/mainboard/google/hatch/variants/puff/mainboard.c +++ b/src/mainboard/google/hatch/variants/puff/mainboard.c @@ -31,23 +31,6 @@ stopwatch_duration_msecs(&sw)); }

-void variant_ramstage_init(void) -{ - static const long display_timeout_ms = 3000; - - /* This is reconfigured back to whatever FSP-S expects by - gpio_configure_pads. */ - gpio_input(GPIO_HDMI_HPD); - gpio_input(GPIO_DP_HPD); - if (display_init_required() - && !gpio_get(GPIO_HDMI_HPD) - && !gpio_get(GPIO_DP_HPD)) { - /* This has to be done before FSP-S runs. */ - if (google_chromeec_wait_for_displayport(display_timeout_ms)) - wait_for_hpd(GPIO_DP_HPD, display_timeout_ms); - } -} - /* * For type-C chargers, set PL2 to 90% of max power to account for * cable loss and FET Rdson loss in the path from the source. @@ -86,6 +69,24 @@ * | n (U22) | 29 | .9n | .9n | x(43) | * +-------------+-----+---------+---------+-------+ */ + +/* + * Psys_pmax considerations + * + * Given the hardware design in puff, the serial shunt resistor is 0.01ohm. + * The full scale of hardware PSYS signal 0.8v maps to system current 9.6A + * instead of real system power. The equation is shown below: + * PSYS = 0.8v = (0.01ohm x Iinput) x 50 (INA213, gain 50V/V) x 15k/(15k + 75k) + * Hence, Iinput (Amps) = 9.6A + * Since there is no voltage information from PSYS, different voltage input + * would map to different Psys_pmax settings: + * For Type-C 15V, the Psys_pmax sholud be 15v x 9.6A = 144W + * For Type-C 20V, the Psys_pmax should be 20v x 9.6A = 192W + * For a barral jack, the Psys_pmax should be 19v x 9.6A = 182.4W + */ +#define PSYS_IMAX 9600 +#define BJ_VOLTS_MV 19000 + static void mainboard_set_power_limits(config_t *conf) { enum usb_chg_type type; @@ -108,15 +109,33 @@ /* set minimum duty cycle */ conf->tdp_psyspl3_dutycycle = PUFF_MIN_DUTYCYCLE; conf->tdp_pl4 = SET_PSYSPL2(psyspl2); + } else { + /* Input type is barrel jack */ + volts_mv = BJ_VOLTS_MV; } + /* voltage unit is milliVolts and current is in milliAmps */ + conf->psys_pmax = (u16)(((u32)PSYS_IMAX * volts_mv) / 1000000);

conf->tdp_pl2_override = PUFF_PL2; /* set psyspl2 to 90% of max adapter power */ conf->tdp_psyspl2 = SET_PSYSPL2(psyspl2); }

-void variant_mainboard_enable(struct device *dev) +void variant_ramstage_init(void) { + static const long display_timeout_ms = 3000; config_t *conf = config_of_soc(); + + /* This is reconfigured back to whatever FSP-S expects by gpio_configure_pads. */ + gpio_input(GPIO_HDMI_HPD); + gpio_input(GPIO_DP_HPD); + if (display_init_required() + && !gpio_get(GPIO_HDMI_HPD) + && !gpio_get(GPIO_DP_HPD)) { + /* This has to be done before FSP-S runs. */ + if (google_chromeec_wait_for_displayport(display_timeout_ms)) + wait_for_hpd(GPIO_DP_HPD, display_timeout_ms); + } + /* Psys_pmax needs to be setup before FSP-S */ mainboard_set_power_limits(conf); }

-- To view, visit https://review.coreboot.org/c/coreboot/+/39850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ea01f856411e05a533489280fc2b4a46a1440c5 Gerrit-Change-Number: 39850 Gerrit-PatchSet: 10 Gerrit-Owner: Gaggery Tsai gaggery.tsai@intel.com Gerrit-Reviewer: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Gaggery Tsai gaggery.tsai@intel.corp-partner.google.com Gerrit-Reviewer: Harry Pan harry.pan@intel.com Gerrit-Reviewer: Jamie Chen jamie.chen@intel.com Gerrit-Reviewer: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Shelley Chen shchen@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1837

days inactive

1863

days old

coreboot-gerrit@coreboot.org

23 comments

7 participants

tags (0)

participants (7)

build bot (Jenkins) (Code Review)
Edward O'Callaghan (Code Review)
Gaggery Tsai (Code Review)
Harry Pan (Code Review)
Jamie Chen (Code Review)
Patrick Georgi (Code Review)
Paul Menzel (Code Review)