Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
soc/amd/common/block/lpc: Add config options for eSPI
eSPI on Picasso is configured using the LPC bridge configuration registers. This change enables config options to allow SoC to select if it supports eSPI (SOC_AMD_COMMON_BLOCK_HAS_ESPI) and mainboard to select if it wants to use eSPI instead of LPC for talking to legacy devices and embedded controllers (SOC_AMD_COMMON_BLOCK_USE_ESPI).
BUG=b:154445472
Change-Id: I15e9eb25706e09393c019eea4d61b66f17490be6 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/common/block/lpc/Kconfig 1 file changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/41069/1
diff --git a/src/soc/amd/common/block/lpc/Kconfig b/src/soc/amd/common/block/lpc/Kconfig index b0d59a5..9bc9010 100644 --- a/src/soc/amd/common/block/lpc/Kconfig +++ b/src/soc/amd/common/block/lpc/Kconfig @@ -3,3 +3,18 @@ default n help Select this option to use the traditional LPC-ISA bridge at D14F3. + +config SOC_AMD_COMMON_BLOCK_HAS_ESPI + bool + default n + help + Select this option if platform supports eSPI using D14F3 configuration + registers. + +config SOC_AMD_COMMON_BLOCK_USE_ESPI + bool + depends on SOC_AMD_COMMON_BLOCK_HAS_ESPI + default n + help + Select this option if mainboard uses eSPI instead of LPC (if supported + by platform).
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... PS1, Line 19: Select this option if mainboard uses eSPI instead of LPC (if supported This sounds a bit like LPC and eSPI were mutually exclusive which they aren't. On Mandolin the EC is on eSPI, but I get serial output via a SIO chip on LPC
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... PS1, Line 19: Select this option if mainboard uses eSPI instead of LPC (if supported
This sounds a bit like LPC and eSPI were mutually exclusive which they aren't. […]
Please see my comment5 on b/154445472.
The way this is handled in chromium coreboot tree is not really right. Picasso doesn't really require a special LPC driver. The only thing that is really required is the ability to identify if a resource belongs to LPC v/s eSPI. If we have to support both LPC and eSPI for Picasso using this driver, I think it should be possible. We should simply enable device types that can be used by mainboards to indicate LPC or eSPI rather than using generic device numbers differently.
Also, I don't really see mandolin adding anything under the LPC generic device in chromium coreboot tree. I think let's continue with this for now and when you are upstreaming support for Mandolin, I can work with you to update this.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... PS1, Line 19: Select this option if mainboard uses eSPI instead of LPC (if supported
Please see my comment5 on b/154445472. […]
I agree with Felix, we should cleanly support both espi and lpc. I also believe it will help with clarity when looking at the device tree. I like your idea about adding a new bus. I threw up a WIP CL that adds espi and lpc buses: https://review.coreboot.org/c/coreboot/+/41099 I can finish it up if you want.
Not sure what you are thinking about eSPI config, but having the bus, we could also have a chip specific config:
device pci 14.3 on # - D14F3 bridge chip soc/amd/common/espi register "bus_width" = "ESPI_IO_MODE_QUAD" register "espi_freq_mhz" = "ESPI_OP_FREQ_33_MHZ" device espi 0 on chip ec/google/chromeec device pnp 0c09.0 on end end end end end
Thoughts?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... PS1, Line 19: Select this option if mainboard uses eSPI instead of LPC (if supported
I agree with Felix, we should cleanly support both espi and lpc. […]
Thanks Raul. I see you updated your patchset. I can rebase on top of that and rework this series to make lpc and espi work together.
About eSPI config, for now I am thinking to keep it in SoC config just like all other IP configs. We can revisit all those configs at once to see if it makes sense to add chip drivers for each of the devices and move them under the device.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... PS1, Line 19: Select this option if mainboard uses eSPI instead of LPC (if supported
Thanks Raul. I see you updated your patchset. […]
I spent some time evaluating this option and I don't think it is really possible to simply add espi and lpc devices under the LPC bridge. There are numerous assumptions in common EC code that the EC lives under LPCB device. Adding anything in between would lead to all kinds of breakages. Also, most of the EC code is currently all asl based (and this is not just about Chrome EC. Even other ECs have the same assumptions).
If we have to add a device under LPC Bridge, then we need to change all EC drivers to generate ACPI tables using SSDT and then update all SoCs which make direct references to LPCB.EC0 device. I think this is way out of scope of the current effort.
I explored a few options on how we can trick acpi_device_name()/acpi_device_path() to emit only one device name even if the device tree exposes 2 devices. But, this is going to lead to uglier hacks.
Again, I go back to my original argument that from a practical standpoint having LPC and eSPI for real devices (non-reference platforms) is not really something we need to worry about for now. So, I think we should continue with the Kconfig options that this CL sets up.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... PS1, Line 19: Select this option if mainboard uses eSPI instead of LPC (if supported
I spent some time evaluating this option and I don't think it is really possible to simply add espi […]
Thanks for evaluating the possibility. I think it's fine to move forward for now then.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 1: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... PS1, Line 19: Select this option if mainboard uses eSPI instead of LPC (if supported
Thanks for evaluating the possibility. I think it's fine to move forward for now then.
Can't say that I really like this approach, but since this only affects Mandolin and not the boards that will be publicly available, I can live with this. The chrome devices have an eMMC that has the interface multiplexed with at least some of the LPC lines, so they'll only be using eSPI. Mandolin also has some other serial ports that might be used instead, so it likely won't completely break things
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/41069/1/src/soc/amd/common/block/lp... PS1, Line 19: Select this option if mainboard uses eSPI instead of LPC (if supported
Can't say that I really like this approach, but since this only affects Mandolin and not the boards […]
Thanks Felix. I will go ahead with this change. Let's revisit this again in the future if required.
Hello build bot (Jenkins), Raul Rangel, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41069
to look at the new patch set (#2).
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
soc/amd/common/block/lpc: Add config options for eSPI
eSPI on Picasso is configured using the LPC bridge configuration registers. This change enables config options to allow SoC to select if it supports eSPI (SOC_AMD_COMMON_BLOCK_HAS_ESPI) and mainboard to select if it wants to use eSPI instead of LPC for talking to legacy devices and embedded controllers (SOC_AMD_COMMON_BLOCK_USE_ESPI).
BUG=b:154445472
Change-Id: I15e9eb25706e09393c019eea4d61b66f17490be6 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/common/block/lpc/Kconfig 1 file changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/41069/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 2:
Rebased to resolve merge conflicts.
Hello build bot (Jenkins), Raul Rangel, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41069
to look at the new patch set (#3).
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
soc/amd/common/block/lpc: Add config options for eSPI
eSPI on Picasso is configured using the LPC bridge configuration registers. This change enables config options to allow SoC to select if it supports eSPI (SOC_AMD_COMMON_BLOCK_HAS_ESPI) and mainboard to select if it wants to use eSPI instead of LPC for talking to legacy devices and embedded controllers (SOC_AMD_COMMON_BLOCK_USE_ESPI).
BUG=b:154445472
Change-Id: I15e9eb25706e09393c019eea4d61b66f17490be6 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/common/block/lpc/Kconfig 1 file changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/41069/3
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 3: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
Patch Set 3: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41069 )
Change subject: soc/amd/common/block/lpc: Add config options for eSPI ......................................................................
soc/amd/common/block/lpc: Add config options for eSPI
eSPI on Picasso is configured using the LPC bridge configuration registers. This change enables config options to allow SoC to select if it supports eSPI (SOC_AMD_COMMON_BLOCK_HAS_ESPI) and mainboard to select if it wants to use eSPI instead of LPC for talking to legacy devices and embedded controllers (SOC_AMD_COMMON_BLOCK_USE_ESPI).
BUG=b:154445472
Change-Id: I15e9eb25706e09393c019eea4d61b66f17490be6 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/41069 Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/common/block/lpc/Kconfig 1 file changed, 15 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/common/block/lpc/Kconfig b/src/soc/amd/common/block/lpc/Kconfig index 3cfbfe5d..1ec8dd4 100644 --- a/src/soc/amd/common/block/lpc/Kconfig +++ b/src/soc/amd/common/block/lpc/Kconfig @@ -9,3 +9,18 @@ default n help Select this option if the LPC bridge supports ROM sharing. + +config SOC_AMD_COMMON_BLOCK_HAS_ESPI + bool + default n + help + Select this option if platform supports eSPI using D14F3 configuration + registers. + +config SOC_AMD_COMMON_BLOCK_USE_ESPI + bool + depends on SOC_AMD_COMMON_BLOCK_HAS_ESPI + default n + help + Select this option if mainboard uses eSPI instead of LPC (if supported + by platform).
-
Aaron Durbin (Code Review) -
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Raul Rangel (Code Review)