Hello V Sowmya,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/33233
to review the following change.
Change subject: soc/intel/cannonlake: Add _DSM method for SD controller ......................................................................
soc/intel/cannonlake: Add _DSM method for SD controller
Change-Id: I15090ed9f9bc90b35dfcba47c913e3d37b799d0b Signed-off-by: V Sowmya v.sowmya@intel.com --- M src/soc/intel/cannonlake/acpi/scs.asl 1 file changed, 61 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/33233/1
diff --git a/src/soc/intel/cannonlake/acpi/scs.asl b/src/soc/intel/cannonlake/acpi/scs.asl index cdfff91..924228e 100644 --- a/src/soc/intel/cannonlake/acpi/scs.asl +++ b/src/soc/intel/cannonlake/acpi/scs.asl @@ -95,6 +95,67 @@ PGEN, 1, /* PG_ENABLE */ }
+ /* _DSM x86 Device Specific Method + * Arg0: UUID Unique function identifier + * Arg1: Integer Revision Level + * Arg2: Integer Function Index (0 = Return Supported Functions) + * Arg3: Package Parameters + */ + Method (_DSM, 4) + { + If(LEqual(Arg0, ToUUID("f6c13ea5-65cd-461f-ab7a-29f7e8d5bd61"))) { + /* Check the revision */ + If(LGreaterEqual(Arg1, Zero)) { + /* Switch statement based on the function index. */ + Switch(ToInteger(Arg2)) { + /* + * Function Index 0 the return value is a buffer containing + * one bit for each function index, starting with zero. + * Bit 0 - Indicates whether there is support for any functions other than function 0. + * Bit 1 - Indicates support to clear power control register + * Bit 2 - Indicates support to set power control register + * Bit 3 - Indicates support to set 1.8V signalling + * Bit 4 - Indicates support to set 3.3V signalling + * Bit 5 - Indicates support for HS200 mode + * Bit 6 - Indicates support for HS400 mode + * Bit 9 - Indicates eMMC I/O Driver Strength + */ + /* + * For SD we have to support functions to + * set 1.8V signalling and 3.3V signalling [BIT4, BIT3] + */ + Case(0) { + Return (Buffer() {0x19}) + } + + /* + * Function Index 3: Set 1.8v signalling. + * We put a sleep of 100ms in this method to + * work around a known issue with detecting + * UHS SD card on PCH. This is to compensate + * for the SD VR slowness. + */ + Case(3) { + Sleep (100) // Sleep 100ms + Return(Buffer(){0x00}) + } + /* + * Function Index 4: Set 3.3v signalling. + * We put a sleep of 100ms in this method to + * work around a known issue with detecting + * UHS SD card on PCH. This is to compensate + * for the SD VR slowness. + */ + Case(4) { + Sleep (100) // Sleep 100ms + Return(Buffer(){0x00}) + } + } // End - Switch(Arg2) + } + } // End - UUID check + Return(Buffer() {0x0}) + } // End _DSM + Method(_INI) { /* Clear register 0x1C20/0x4820 */
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33233 )
Change subject: soc/intel/cannonlake: Add _DSM method for SD controller ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33233/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33233/1//COMMIT_MSG@8 PS1, Line 8: Can you please add the BUG= field.
https://review.coreboot.org/#/c/33233/1/src/soc/intel/cannonlake/acpi/scs.as... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/33233/1/src/soc/intel/cannonlake/acpi/scs.as... PS1, Line 106: ( Please use space before ( here and in rest of the uses in this file.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33233 )
Change subject: soc/intel/cannonlake: Add _DSM method for SD controller ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33233/1/src/soc/intel/cannonlake/acpi/scs.as... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/33233/1/src/soc/intel/cannonlake/acpi/scs.as... PS1, Line 106: ToUUID("f6c13ea5-65cd-461f-ab7a-29f7e8d5bd61") Not a big deal, but it would be easier to read through if this was a Name(UUID, ToUUID(...)) up at the Device level, and then If (LEqual (Arg0, ^UUID)).
https://review.coreboot.org/#/c/33233/1/src/soc/intel/cannonlake/acpi/scs.as... PS1, Line 131: /* : * Function Index 3: Set 1.8v signalling. : * We put a sleep of 100ms in this method to : * work around a known issue with detecting : * UHS SD card on PCH. This is to compensate : * for the SD VR slowness. : */ : Case(3) { : Sleep (100) // Sleep 100ms : Return(Buffer(){0x00}) : } : /* : * Function Index 4: Set 3.3v signalling. : * We put a sleep of 100ms in this method to : * work around a known issue with detecting : * UHS SD card on PCH. This is to compensate : * for the SD VR slowness. : */ : Case(4) { : Sleep (100) // Sleep 100ms : Return(Buffer(){0x00}) : } Are these issues documented somewhere?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33233 )
Change subject: soc/intel/cannonlake: Add _DSM method for SD controller ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33233/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33233/1//COMMIT_MSG@7 PS1, Line 7: soc/intel/cannonlake: Add _DSM method for SD controller Can we get a better description here also? That the _DSM method is adding sleeps to voltage level switching to work around controller issues?
Hello Medha Garima, Paul Fagerburg, Tim Wawrzynczak, Philip Chen, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33233
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Add _DSM method for SD controller ......................................................................
soc/intel/cannonlake: Add _DSM method for SD controller
The SD controller seems to take some time after restarting the clock at 1.8V before it actually switches from 3.3V to 1.8V. Add a _DSM method that simply sleeps when switching between 3.3V and 1.8V. Otherwise, the kernel times out too quickly waiting for the card to acknowledge the 1.8V switch. The card itself is waiting until it sees the clk signal being driven at 1.8V.
BUG=b:125441242 TEST=Boot Hatch with SD card and CR2 removed, observe voltage switch succeeds.
Change-Id: I15090ed9f9bc90b35dfcba47c913e3d37b799d0b Signed-off-by: V Sowmya v.sowmya@intel.com Signef-off-by: Evan Green evgreen@chromium.org --- M src/soc/intel/cannonlake/acpi/scs.asl 1 file changed, 62 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/33233/2
Evan Green has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33233 )
Change subject: soc/intel/cannonlake: Add _DSM method for SD controller ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/#/c/33233/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33233/1//COMMIT_MSG@7 PS1, Line 7: soc/intel/cannonlake: Add _DSM method for SD controller
Can we get a better description here also? That the _DSM method is adding sleeps to voltage level s […]
Done
https://review.coreboot.org/#/c/33233/1//COMMIT_MSG@8 PS1, Line 8:
Can you please add the BUG= field.
Done
https://review.coreboot.org/#/c/33233/1/src/soc/intel/cannonlake/acpi/scs.as... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/33233/1/src/soc/intel/cannonlake/acpi/scs.as... PS1, Line 106: (
Please use space before ( here and in rest of the uses in this file.
Done
https://review.coreboot.org/#/c/33233/1/src/soc/intel/cannonlake/acpi/scs.as... PS1, Line 106: ToUUID("f6c13ea5-65cd-461f-ab7a-29f7e8d5bd61")
Not a big deal, but it would be easier to read through if this was a Name(UUID, ToUUID(... […]
Done
https://review.coreboot.org/#/c/33233/1/src/soc/intel/cannonlake/acpi/scs.as... PS1, Line 131: /* : * Function Index 3: Set 1.8v signalling. : * We put a sleep of 100ms in this method to : * work around a known issue with detecting : * UHS SD card on PCH. This is to compensate : * for the SD VR slowness. : */ : Case(3) { : Sleep (100) // Sleep 100ms : Return(Buffer(){0x00}) : } : /* : * Function Index 4: Set 3.3v signalling. : * We put a sleep of 100ms in this method to : * work around a known issue with detecting : * UHS SD card on PCH. This is to compensate : * for the SD VR slowness. : */ : Case(4) { : Sleep (100) // Sleep 100ms : Return(Buffer(){0x00}) : }
Are these issues documented somewhere?
Not that I know of.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33233 )
Change subject: soc/intel/cannonlake: Add _DSM method for SD controller ......................................................................
Patch Set 2: Code-Review+1
(11 comments)
mostly nits about asl style.
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 107: ( Space before (
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 109: ( same here
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 111: ( same here
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 128: ( same here
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 129: ( same here
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 139: ( same here
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 141: } Space before }
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 141: ( same here
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 141: { spaces around {
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 150: ( same here
https://review.coreboot.org/#/c/33233/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 152: ( same here
Hello Medha Garima, Paul Fagerburg, Tim Wawrzynczak, Philip Chen, Shelley Chen, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33233
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Add _DSM method for SD controller ......................................................................
soc/intel/cannonlake: Add _DSM method for SD controller
The SD controller seems to take some time after restarting the clock at 1.8V before it actually switches from 3.3V to 1.8V. Add a _DSM method that simply sleeps when switching between 3.3V and 1.8V. Otherwise, the kernel times out too quickly waiting for the card to acknowledge the 1.8V switch. The card itself is waiting until it sees the clk signal being driven at 1.8V.
BUG=b:125441242 TEST=Boot Hatch with SD card and CR2 removed, observe voltage switch succeeds.
Change-Id: I15090ed9f9bc90b35dfcba47c913e3d37b799d0b Signed-off-by: V Sowmya v.sowmya@intel.com Signef-off-by: Evan Green evgreen@chromium.org --- M src/soc/intel/cannonlake/acpi/scs.asl 1 file changed, 62 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/33233/3
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33233 )
Change subject: soc/intel/cannonlake: Add _DSM method for SD controller ......................................................................
Patch Set 3: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33233 )
Change subject: soc/intel/cannonlake: Add _DSM method for SD controller ......................................................................
Patch Set 3: Code-Review+2
Furquan Shaikh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33233 )
Change subject: soc/intel/cannonlake: Add _DSM method for SD controller ......................................................................
soc/intel/cannonlake: Add _DSM method for SD controller
The SD controller seems to take some time after restarting the clock at 1.8V before it actually switches from 3.3V to 1.8V. Add a _DSM method that simply sleeps when switching between 3.3V and 1.8V. Otherwise, the kernel times out too quickly waiting for the card to acknowledge the 1.8V switch. The card itself is waiting until it sees the clk signal being driven at 1.8V.
BUG=b:125441242 TEST=Boot Hatch with SD card and CR2 removed, observe voltage switch succeeds.
Change-Id: I15090ed9f9bc90b35dfcba47c913e3d37b799d0b Signed-off-by: V Sowmya v.sowmya@intel.com Signef-off-by: Evan Green evgreen@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/33233 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/cannonlake/acpi/scs.asl 1 file changed, 62 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/acpi/scs.asl b/src/soc/intel/cannonlake/acpi/scs.asl index cdfff91..0012a4d 100644 --- a/src/soc/intel/cannonlake/acpi/scs.asl +++ b/src/soc/intel/cannonlake/acpi/scs.asl @@ -84,6 +84,7 @@ Name (_ADR, 0x00140005) Name (_DDN, "SD Controller") Name (TEMP, 0) + Name (DSUU, ToUUID("f6c13ea5-65cd-461f-ab7a-29f7e8d5bd61"))
OperationRegion (SDPC, PCI_Config, 0x00, 0x100) Field (SDPC, WordAcc, NoLock, Preserve) @@ -95,6 +96,67 @@ PGEN, 1, /* PG_ENABLE */ }
+ /* _DSM x86 Device Specific Method + * Arg0: UUID Unique function identifier + * Arg1: Integer Revision Level + * Arg2: Integer Function Index (0 = Return Supported Functions) + * Arg3: Package Parameters + */ + Method (_DSM, 4) + { + If (LEqual (Arg0, ^DSUU)) { + /* Check the revision */ + If (LGreaterEqual (Arg1, Zero)) { + /* Switch statement based on the function index. */ + Switch (ToInteger (Arg2)) { + /* + * Function Index 0 the return value is a buffer containing + * one bit for each function index, starting with zero. + * Bit 0 - Indicates whether there is support for any functions other than function 0. + * Bit 1 - Indicates support to clear power control register + * Bit 2 - Indicates support to set power control register + * Bit 3 - Indicates support to set 1.8V signalling + * Bit 4 - Indicates support to set 3.3V signalling + * Bit 5 - Indicates support for HS200 mode + * Bit 6 - Indicates support for HS400 mode + * Bit 9 - Indicates eMMC I/O Driver Strength + */ + /* + * For SD we have to support functions to + * set 1.8V signalling and 3.3V signalling [BIT4, BIT3] + */ + Case (0) { + Return (Buffer () { 0x19 }) + } + + /* + * Function Index 3: Set 1.8v signalling. + * We put a sleep of 100ms in this method to + * work around a known issue with detecting + * UHS SD card on PCH. This is to compensate + * for the SD VR slowness. + */ + Case (3) { + Sleep (100) + Return(Buffer () { 0x00 }) + } + /* + * Function Index 4: Set 3.3v signalling. + * We put a sleep of 100ms in this method to + * work around a known issue with detecting + * UHS SD card on PCH. This is to compensate + * for the SD VR slowness. + */ + Case (4) { + Sleep (100) + Return(Buffer () { 0x00 }) + } + } + } + } + Return(Buffer() { 0x0 }) + } + Method(_INI) { /* Clear register 0x1C20/0x4820 */
-
Evan Green (Code Review) -
Furquan Shaikh (Code Review)
-
Tim Wawrzynczak (Code Review)