Change in ...coreboot[master]: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC contr...
V Sowmya has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30614
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller
This patch adds an Kconfig option to exclude the EMMC interface ACPI objects for cannonlake based platforms.
BUG=b:120914069 BRANCH=none TEST=USE="-intel_mrc -bmpblk" emerge-hatch coreboot.
Change-Id: I90c0230e845c8d02386b50b1100faf7064ecf8f6 Signed-off-by: V Sowmya v.sowmya@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/acpi/scs.asl 2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/30614/1
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index 9e007b6..121909a 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -208,6 +208,13 @@ default 4 if SOC_INTEL_CANNONLAKE_PCH_H default 6
+config EXCLUDE_EMMC_INTERFACE + bool + default n + help + If you set this option to n, it will not use EMMC controller. + + # Clock divider parameters for 115200 baud rate config SOC_INTEL_COMMON_LPSS_UART_CLK_M_VAL hex diff --git a/src/soc/intel/cannonlake/acpi/scs.asl b/src/soc/intel/cannonlake/acpi/scs.asl index 896fd77..29939be 100644 --- a/src/soc/intel/cannonlake/acpi/scs.asl +++ b/src/soc/intel/cannonlake/acpi/scs.asl @@ -27,6 +27,7 @@ ^PCRA (Arg0, 0x4820, 0x0) }
+#if !IS_ENABLED(CONFIG_EXCLUDE_EMMC_INTERFACE) /* EMMC */ Device(PEMC) { Name(_ADR, 0x001A0000) @@ -77,6 +78,7 @@ } } } +#endif
/* SD CARD */ Device (SDXC)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/30614/2/src/soc/intel/cannonlake/acpi/scs.as... File src/soc/intel/cannonlake/acpi/scs.asl:
https://review.coreboot.org/#/c/30614/2/src/soc/intel/cannonlake/acpi/scs.as... PS2, Line 30: #
yes, those bits represents […]
Well it should get evaluated for _ADR devices in general, not sure about absent _ADR devices. Though, if there is no rule to ignore _STA, that sounds like a bug in the OS.
Without _STA, OSPM has to assume the device is present. I trust you that you tested this in the past, but I'd still want to see the code to avoid misunderstandings.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 3: -Code-Review
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 3:
Patch Set 3: -Code-Review
Moving it to a separate asl and including it from MB seems like a good idea, then all the MBs based on cannonlake have to be updated as well.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 3:
Patch Set 2:
Patch Set 2:
@Arthur/Patrick R: Do you care to take -1 back?
So you somehow need to manually sync this Kconfig option with the devicetree in which the eMMC controller gets disabled too? If so my -1 stays here. If you somehow really don't want this ACPI code to be included when the device is disabled, generate it on runtime like told before or put it in a different file and don't include DSDT but yet another Kconfig option does not seem like a good solution.
Having another ASL still does not absolve us of keeping in-sync 2 things. in this approach we would have keep the devicetree and ASL (DSDT) in sync.
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 3:
(1 comment)
Patch Set 2:
Patch Set 2:
@Arthur/Patrick R: Do you care to take -1 back?
So you somehow need to manually sync this Kconfig option with the devicetree in which the eMMC controller gets disabled too? If so my -1 stays here. If you somehow really don't want this ACPI code to be included when the device is disabled, generate it on runtime like told before or put it in a different file and don't include DSDT but yet another Kconfig option does not seem like a good solution.
@Arthur, Aligning with the approach that you suggested here i will be making the following changes,
SOC: Split the scs.asl into two different files (Sdcard and eMMC). Mainboard:
* Create a storage.asl for every variant and include the ASL files for storage they support. * Include the storage.asl in the mainboard dsdt.asl file. Any thoughts on the above mentioned changes or i will push this changes?
https://review.coreboot.org/#/c/30614/2/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/30614/2/src/soc/intel/cannonlake/Kconfig@216 PS2, Line 216:
a newline to much.
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 3:
Patch Set 2:
Patch Set 2:
@Arthur/Patrick R: Do you care to take -1 back?
So you somehow need to manually sync this Kconfig option with the
devicetree in which the eMMC controller gets disabled too? If so my -1 stays here. If you somehow really don't want this ACPI code to be included when the device is disabled, generate it on runtime like told before or put it in a different file and don't include DSDT but yet another Kconfig option does not seem like a good solution.
Having another ASL still does not absolve us of keeping in-sync 2 things. in this approach we would have keep the devicetree and ASL (DSDT) in sync.
Yes, that's correct. Only remaining option is to use coreboot's ssdt generator.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 3:
Only remaining option is to use coreboot's ssdt generator.
Or test _STA method proper.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 3:
Only remaining option is to use coreboot's ssdt generator.
Or test _STA method proper.
_STA is against the ACPI spec in case of PCI device. i don't understand whats the point of pushing in that line ?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 3:
Only remaining option is to use coreboot's ssdt generator.
Or test _STA method proper.
_STA is against the ACPI spec in case of PCI device. i don't understand whats the point of pushing in that line ?
Because I couldn't find anything in ACPI spec telling that. Can you point me to the specific paragraph, please?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 3:
Only remaining option is to use coreboot's ssdt generator.
Or test _STA method proper.
_STA is against the ACPI spec in case of PCI device. i don't understand whats the point of pushing in that line ?
Because I couldn't find anything in ACPI spec telling that. Can you point me to the specific paragraph, please?
I thought you read entire gerrit discussion's for this CL.
I have replied the same in Jan 10th that.
src/soc/intel/cannonlake/acpi/scs.asl Line 30: . For _HID devices, OSPM evaluates the _STA method. For _ADR devices, OSPM checks with the bus driver for that device
https://www.uefi.org/sites/default/files/resources/ACPI_6.0.pdf section 6.3.3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/30614/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30614/3//COMMIT_MSG@7 PS3, Line 7: Adding Please use imperative mood: Add
https://review.coreboot.org/#/c/30614/3//COMMIT_MSG@9 PS3, Line 9: an Kconfig a Kconfig
https://review.coreboot.org/#/c/30614/3//COMMIT_MSG@11 PS3, Line 11: … as there are platforms – like … – that do not have an eMMC slot(?).
https://review.coreboot.org/#/c/30614/3/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/30614/3/src/soc/intel/cannonlake/Kconfig@194 PS3, Line 194: config EXCLUDE_EMMC_INTERFACE This should be name-spaced by prepending SOC_INTEL_.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30614 )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Patch Set 3:
Only remaining option is to use coreboot's ssdt generator.
Or test _STA method proper.
_STA is against the ACPI spec in case of PCI device. i don't understand whats the point of pushing in that line ?
Because I couldn't find anything in ACPI spec telling that. Can you point me to the specific paragraph, please?
I thought you read entire gerrit discussion's for this CL.
I have replied the same in Jan 10th that.
src/soc/intel/cannonlake/acpi/scs.asl Line 30: . For _HID devices, OSPM evaluates the _STA method. For _ADR devices, OSPM checks with the bus driver for that device
That's your comment, we have been there.
https://www.uefi.org/sites/default/files/resources/ACPI_6.0.pdf section 6.3.3
Section 6.3.3 is about an eject method. Also, 6.3.7 which is about _STA doesn't talk about HID vs PCI devices. I've already looked into the spec and the acpica implementation and have failed to find any hint regarding this. If you really want to help me, I'd need a very specific pointer or a quote of the spec.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/30614?usp=email )
Change subject: soc/intel/cannonlake: Adding Kconfig option to disable the eMMC controller ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Martin L Roth (Code Review) -
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Rizwan Qureshi (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)