Change in coreboot[master]: soc/intel/common: Add function to check if PCI device is a LPSS contr...
Aamir Bohra has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
soc/intel/common: Add function to check if PCI device is a LPSS controller
Change-Id: I3f5bb268fc581280bb1b87b6b175a0299a24a44a Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/common/block/include/intelblocks/lpss.h M src/soc/intel/common/block/lpss/lpss.c 2 files changed, 24 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34137/1
diff --git a/src/soc/intel/common/block/include/intelblocks/lpss.h b/src/soc/intel/common/block/include/intelblocks/lpss.h index eb38f13..059b06d 100644 --- a/src/soc/intel/common/block/include/intelblocks/lpss.h +++ b/src/soc/intel/common/block/include/intelblocks/lpss.h @@ -16,6 +16,7 @@ #ifndef SOC_INTEL_COMMON_BLOCK_LPSS_H #define SOC_INTEL_COMMON_BLOCK_LPSS_H
+#include <device/device.h> #include <stdint.h>
/* Gets controller out of reset */ @@ -30,4 +31,7 @@ /* Check if controller is in reset. */ bool lpss_is_controller_in_reset(uintptr_t base);
+/* Check if the device is a LPSS controller */ +bool is_lpss(struct device *dev); + #endif /* SOC_INTEL_COMMON_BLOCK_LPSS_H */ diff --git a/src/soc/intel/common/block/lpss/lpss.c b/src/soc/intel/common/block/lpss/lpss.c index 6b6d17b..46cd6e7 100644 --- a/src/soc/intel/common/block/lpss/lpss.c +++ b/src/soc/intel/common/block/lpss/lpss.c @@ -15,6 +15,7 @@
#include <device/mmio.h> #include <intelblocks/lpss.h> +#include <soc/soc_chip.h>
/* Clock register */ #define LPSS_CLOCK_CTL_REG 0x200 @@ -39,6 +40,12 @@ /* DMA Software Reset Control */ #define LPSS_DMA_RST_RELEASE (1 << 2)
+/* SoC override function */ +__weak const int *soc_lpss_controllers(int *size) +{ + return NULL; +} + bool lpss_is_controller_in_reset(uintptr_t base) { uint8_t *addr = (void *)base; @@ -69,3 +76,16 @@
write32(addr, clk_sel); } + +bool is_lpss(struct device *dev) +{ + int size = 0; + const int *lpss_devices = soc_lpss_controllers(&size); + if (!lpss_devices) + return false; + for (int i = 0; i < size; i++) { + if (lpss_devices[i] == dev->path.pci.devfn) + return true; + } + return false; +}
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
Patch Set 3:
This change is ready for review.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
Patch Set 3:
Why is this needed?
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
Patch Set 3:
Patch Set 3:
Why is this needed?
Due to recommendation(captured in EDS and BIOS Spec), no LPSS controllers can share same IRQs. However the rest of the PCI devices can share the IRQs from 16-23. That is why is controller is a LPSS controllers, will have to re-assign a different IRQ that the other controllers. So this function was needed to know if controller is a LPSS controller and assign a unique IRQ.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/3/src/soc/intel/common/block/... PS3, Line 43: /* SoC override function */ : __weak const int *soc_lpss_controllers(size_t *size) : { : return NULL; : } : If you set *size to 0, then a NULL check is not needed while looping over the returned array.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/3/src/soc/intel/common/block/... PS3, Line 44: __weak If a SoC is selecting LPSS common block, I would expect it to always provide the definition of soc_lpss_controllers. In that case having a weak function seems unnecessary.
Hello Patrick Rudolph, Karthik Ramasubramanian, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34137
to look at the new patch set (#4).
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
soc/intel/common: Add function to check if PCI device is a LPSS controller
Change-Id: I3f5bb268fc581280bb1b87b6b175a0299a24a44a Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/common/block/include/intelblocks/lpss.h M src/soc/intel/common/block/lpss/lpss.c 2 files changed, 27 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34137/4
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34137/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/3/src/soc/intel/common/block/... PS3, Line 44: __weak
If a SoC is selecting LPSS common block, I would expect it to always provide the definition of soc_l […]
LPSS controller check was currently needed for irq table implementation. Since plan was to support it to CNL onwards, the weak implementation is for older platforms.
https://review.coreboot.org/c/coreboot/+/34137/3/src/soc/intel/common/block/... PS3, Line 43: /* SoC override function */ : __weak const int *soc_lpss_controllers(size_t *size) : { : return NULL; : } :
If you set *size to 0, then a NULL check is not needed while looping over the returned array.
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... PS4, Line 80: is_dev_lpss r u planning to call this function multiple time ?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... PS4, Line 35: soc nit: All the soc specific functions have the format with soc_*. Prefer to maintain that format. Again my personal opinion.
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... PS4, Line 83: static const int *lpss_devices; Please leave a blank line between declaration block and function body.
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... PS4, Line 87: dev->path.pci.devfn It is also better to ensure that the device path is a PCI type in addition to checking the devfn.
Hello Patrick Rudolph, Karthik Ramasubramanian, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34137
to look at the new patch set (#5).
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
soc/intel/common: Add function to check if PCI device is a LPSS controller
Change-Id: I3f5bb268fc581280bb1b87b6b175a0299a24a44a Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/common/block/include/intelblocks/lpss.h M src/soc/intel/common/block/lpss/lpss.c 2 files changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34137/5
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... PS4, Line 35: soc
nit: All the soc specific functions have the format with soc_*. Prefer to maintain that format. […]
Done
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... PS4, Line 80: is_dev_lpss
r u planning to call this function multiple time ?
With the irq implementation yes, wanted to avoid SOC callbacks everytime.
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... PS4, Line 83: static const int *lpss_devices;
Please leave a blank line between declaration block and function body.
Done
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... PS4, Line 87: dev->path.pci.devfn
It is also better to ensure that the device path is a PCI type in addition to checking the devfn.
certainly. Included the check now. Thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... PS5, Line 35: int unsigned int?
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... PS5, Line 83: int Shouldn't this be unsigned int?
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common: Add function to check if PCI device is a LPSS controller ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... PS5, Line 35: int
unsigned int?
Ack
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... PS5, Line 83: int
Shouldn't this be unsigned int?
Ack
Hello Patrick Rudolph, Karthik Ramasubramanian, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34137
to look at the new patch set (#6).
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
soc/intel/common/lpss: Add function to check for a LPSS controller
Change-Id: I3f5bb268fc581280bb1b87b6b175a0299a24a44a Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/common/block/include/intelblocks/lpss.h M src/soc/intel/common/block/lpss/lpss.c 2 files changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34137/6
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... PS5, Line 35: int
Ack
Done
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/5/src/soc/intel/common/block/... PS5, Line 83: int
Ack
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34137/2/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/2/src/soc/intel/common/block/... PS2, Line 44: __weak const int *soc_lpss_controllers(int *size) : { : return NULL; : }
if you want enforce this for all soc then let's remove __weak function
Do not plan to implement it for apollolake and skylake for now. Currently added this for the irq implementation that would be supported CNL onwards, In future can remove it, if APL and SKL implements strong for it.
https://review.coreboot.org/c/coreboot/+/34137/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/3/src/soc/intel/common/block/... PS3, Line 44: __weak
LPSS controller check was currently needed for irq table implementation. […]
Done.
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... PS4, Line 80: is_dev_lpss
With the irq implementation yes, wanted to avoid SOC callbacks everytime.
Done
https://review.coreboot.org/c/coreboot/+/34137/4/src/soc/intel/common/block/... PS4, Line 87: dev->path.pci.devfn
certainly. Included the check now. Thanks.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... PS6, Line 44: unsigned int Probably need to update this as well?
Also would be good to add a comment indicating what the SoC code is expected to return.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... PS6, Line 44: unsigned int
Probably need to update this as well? […]
Ok. sure, will add the comment also.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 6: Code-Review-1
(2 comments)
https://review.coreboot.org/c/coreboot/+/34137/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34137/6//COMMIT_MSG@7 PS6, Line 7: soc/intel/common/lpss: Add function to check for a LPSS controller how was it tested? Why is it useful?
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... PS6, Line 103: if (dev->path.type != DEVICE_PATH_PCI) you need to verify the bus number, too.
Hello Patrick Rudolph, Karthik Ramasubramanian, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34137
to look at the new patch set (#7).
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
soc/intel/common/lpss: Add function to check for a LPSS controller
Add an API to check if device is a LPSS controller. This API can be used for IRQ assignments for LPSS PCI controllers. Since the LPSS controllers have a requirement of unique IRQ assigments and do not share same IRQ# with other LPSS controllers.
Change-Id: I3f5bb268fc581280bb1b87b6b175a0299a24a44a Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/common/block/include/intelblocks/lpss.h M src/soc/intel/common/block/lpss/lpss.c 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34137/7
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34137/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34137/6//COMMIT_MSG@7 PS6, Line 7: soc/intel/common/lpss: Add function to check for a LPSS controller
how was it tested? […]
Done. Updated the commit msg.
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... PS6, Line 103: if (dev->path.type != DEVICE_PATH_PCI)
you need to verify the bus number, too.
not sure why we would need that, I have list coming from SOC, and we are just scanning the tree to compare against that tree , if the device is a LPSS controller as supported by SOC.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... PS6, Line 44: unsigned int
Ok. sure, will add the comment also.
Done
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 7: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/7/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/7/src/soc/intel/common/block/... PS7, Line 45: controller nit: controllers
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 7: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... PS6, Line 103: if (dev->path.type != DEVICE_PATH_PCI)
not sure why we would need that, I have list coming from SOC, and we are just scanning the tree to c […]
1. That's not part of this commit. 2. It's not documented that only specific devices should be passed to this function. 3. There's no check if the device is a SoC device.
Hello Patrick Rudolph, Karthik Ramasubramanian, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34137
to look at the new patch set (#8).
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
soc/intel/common/lpss: Add function to check for a LPSS controller
Add an API to check if device is a LPSS controller. This API can be used for IRQ assignments for LPSS PCI controllers. Since the LPSS controllers have a requirement of unique IRQ assigments and do not share same IRQ# with other LPSS controllers.
SOC code is reponsible to provide list of the LPSS controllers supported and needs to implement strong of soc_lpss_controllers_list API, in case it needs to use this common implementation.
Change-Id: I3f5bb268fc581280bb1b87b6b175a0299a24a44a Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/common/block/include/intelblocks/lpss.h M src/soc/intel/common/block/lpss/lpss.c 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34137/8
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34137/7/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/7/src/soc/intel/common/block/... PS7, Line 45: controller
nit: controllers
Ok. Done
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... PS6, Line 103: if (dev->path.type != DEVICE_PATH_PCI)
- That's not part of this commit. […]
1. Ok updated the commit message to highlight the SOC expectations. 2/3. As updated in the commit msg, The strong API for the SOC list needs to come from SOC. Cannonlake already maintains that list. The SOC would ensure that all and valid LPSS controllers are provided through strong function.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 8: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34137/8//COMMIT_MSG@15 PS8, Line 15: implement strong of soc_lpss_controllers_list implement soc_lpss_controllers_list
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34137/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34137/8//COMMIT_MSG@10 PS8, Line 10: . S , since ?
https://review.coreboot.org/c/coreboot/+/34137/7/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/7/src/soc/intel/common/block/... PS7, Line 45: for nit: of
Hello Patrick Rudolph, Karthik Ramasubramanian, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34137
to look at the new patch set (#9).
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
soc/intel/common/lpss: Add function to check for a LPSS controller
Add an API to check if device is a LPSS controller. This API can be used for IRQ assignments for LPSS PCI controllers, since the LPSS controllers have a requirement of unique IRQ assigments and do not share same IRQ# with other LPSS controllers.
SOC code is reponsible to provide list of the LPSS controllers supported and needs to implement soc_lpss_controllers_list API, in case it needs to use this common implementation.
Change-Id: I3f5bb268fc581280bb1b87b6b175a0299a24a44a Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/common/block/include/intelblocks/lpss.h M src/soc/intel/common/block/lpss/lpss.c 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34137/9
Hello Patrick Rudolph, Karthik Ramasubramanian, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34137
to look at the new patch set (#10).
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
soc/intel/common/lpss: Add function to check for a LPSS controller
Add an API to check if device is a LPSS controller. This API can be used for IRQ assignments for LPSS PCI controllers, since the LPSS controllers have a requirement of unique IRQ assigments and do not share same IRQ# with other LPSS controllers.
SOC code is reponsible to provide list of the LPSS controllers supported and needs to implement soc_lpss_controllers_list API, in case it needs to use this common implementation.
Change-Id: I3f5bb268fc581280bb1b87b6b175a0299a24a44a Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/common/block/include/intelblocks/lpss.h M src/soc/intel/common/block/lpss/lpss.c 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34137/10
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34137/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34137/8//COMMIT_MSG@10 PS8, Line 10: . S
, since ?
Done
https://review.coreboot.org/c/coreboot/+/34137/8//COMMIT_MSG@15 PS8, Line 15: implement strong of soc_lpss_controllers_list
implement soc_lpss_controllers_list
Done
https://review.coreboot.org/c/coreboot/+/34137/7/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpss.h:
https://review.coreboot.org/c/coreboot/+/34137/7/src/soc/intel/common/block/... PS7, Line 45: for
nit: of
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34137/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34137/6//COMMIT_MSG@7 PS6, Line 7: soc/intel/common/lpss: Add function to check for a LPSS controller
Done. Updated the commit msg.
Done
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/6/src/soc/intel/common/block/... PS6, Line 103: if (dev->path.type != DEVICE_PATH_PCI)
- Ok updated the commit message to highlight the SOC expectations. […]
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34137/10/src/soc/intel/common/block... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/10/src/soc/intel/common/block... PS10, Line 100: static size_t size; : static const pci_devfn_t *lpss_devices; I don't see why this needs to be static. The state does not need to live in between calls to this function. The call to soc_lpss_controllers_list is just a simple pointer assignment and should have virtually no overhead.
https://review.coreboot.org/c/coreboot/+/34137/10/src/soc/intel/common/block... PS10, Line 106: if (!lpss_devices) : lpss_devices = soc_lpss_controllers_list(&size); See above.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/10/src/soc/intel/common/block... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/10/src/soc/intel/common/block... PS10, Line 100: static size_t size; : static const pci_devfn_t *lpss_devices;
I don't see why this needs to be static. […]
Ok. For the IRQ list , this is would be called for each IRQ PCI dev that SOC would support(> 30 list). For hatch it was getting called 16 times. I thought this could save callbacks. But I am Ok to change , if you don't see value in it. Let me know.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/11/src/soc/intel/common/block... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/11/src/soc/intel/common/block... PS11, Line 98: struct device *dev nit: 'const struct device *dev" would be a little more clear
Hello Patrick Rudolph, Karthik Ramasubramanian, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34137
to look at the new patch set (#12).
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
soc/intel/common/lpss: Add function to check for a LPSS controller
Add an API to check if device is a LPSS controller. This API can be used for IRQ assignments for LPSS PCI controllers, since the LPSS controllers have a requirement of unique IRQ assignments and do not share same IRQ# with other LPSS controllers.
SOC code is reponsible to provide list of the LPSS controllers supported and needs to implement soc_lpss_controllers_list API, in case it needs to use this common implementation.
Change-Id: I3f5bb268fc581280bb1b87b6b175a0299a24a44a Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/common/block/include/intelblocks/lpss.h M src/soc/intel/common/block/lpss/lpss.c 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34137/12
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/11/src/soc/intel/common/block... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/11/src/soc/intel/common/block... PS11, Line 98: struct device *dev
nit: 'const struct device *dev" would be a little more clear
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/12/src/soc/intel/common/block... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/12/src/soc/intel/common/block... PS12, Line 50: __weak const pci_devfn_t *soc_lpss_controllers_list(size_t *size) : { : *size = 0; : return NULL; : } Just curious: If you do not provide this, does the compilation fail? I am wondering because is_dev_lpss() should be unused and so get removed during linking. Is that not the case?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 12: Code-Review+1
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/12/src/soc/intel/common/block... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/12/src/soc/intel/common/block... PS12, Line 50: __weak const pci_devfn_t *soc_lpss_controllers_list(size_t *size) : { : *size = 0; : return NULL; : }
Just curious: If you do not provide this, does the compilation fail? I am wondering because is_dev_l […]
Compilation does not fail on removing this. This would serve more as indicator that the SOC needs to provide the list or else the is_dev_lpss would always return false.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/12/src/soc/intel/common/block... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/12/src/soc/intel/common/block... PS12, Line 50: __weak const pci_devfn_t *soc_lpss_controllers_list(size_t *size) : { : *size = 0; : return NULL; : }
Compilation does not fail on removing this. This would serve more as indicator that the SOC needs to provide the list or else the is_dev_lpss would always return false.
I don't understand what you mean by "indicator that the SOC needs to provide the list or else the is_dev_lpss would always return false."
If is_dev_lpss gets included in the image, it will fail linking if soc_lpss_controllers_list() is not provided by the SoC. The way it is done right now will return false always. Isn't a build time failure better indication that SoC implementation should be provided?
Hello Patrick Rudolph, Karthik Ramasubramanian, Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34137
to look at the new patch set (#13).
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
soc/intel/common/lpss: Add function to check for a LPSS controller
Add an API to check if device is a LPSS controller. This API can be used for IRQ assignments for LPSS PCI controllers, since the LPSS controllers have a requirement of unique IRQ assignments and do not share same IRQ# with other LPSS controllers.
SOC code is reponsible to provide list of the LPSS controllers supported and needs to implement soc_lpss_controllers_list API, in case it needs to use this common implementation.
Change-Id: I3f5bb268fc581280bb1b87b6b175a0299a24a44a Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/common/block/include/intelblocks/lpss.h M src/soc/intel/common/block/lpss/lpss.c 2 files changed, 27 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34137/13
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 13: Code-Review+2
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34137/12/src/soc/intel/common/block... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/12/src/soc/intel/common/block... PS12, Line 50: __weak const pci_devfn_t *soc_lpss_controllers_list(size_t *size) : { : *size = 0; : return NULL; : }
Compilation does not fail on removing this. […]
Yes. Agree. Updated the CL to remove soc_lpss_controllers_list definition.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34137/10/src/soc/intel/common/block... File src/soc/intel/common/block/lpss/lpss.c:
https://review.coreboot.org/c/coreboot/+/34137/10/src/soc/intel/common/block... PS10, Line 100: static size_t size; : static const pci_devfn_t *lpss_devices;
Ok. […]
Done
https://review.coreboot.org/c/coreboot/+/34137/10/src/soc/intel/common/block... PS10, Line 106: if (!lpss_devices) : lpss_devices = soc_lpss_controllers_list(&size);
See above.
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 13: Code-Review+2
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
Patch Set 14: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34137 )
Change subject: soc/intel/common/lpss: Add function to check for a LPSS controller ......................................................................
soc/intel/common/lpss: Add function to check for a LPSS controller
Add an API to check if device is a LPSS controller. This API can be used for IRQ assignments for LPSS PCI controllers, since the LPSS controllers have a requirement of unique IRQ assignments and do not share same IRQ# with other LPSS controllers.
SOC code is reponsible to provide list of the LPSS controllers supported and needs to implement soc_lpss_controllers_list API, in case it needs to use this common implementation.
Change-Id: I3f5bb268fc581280bb1b87b6b175a0299a24a44a Signed-off-by: Aamir Bohra aamir.bohra@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34137 Reviewed-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/include/intelblocks/lpss.h M src/soc/intel/common/block/lpss/lpss.c 2 files changed, 27 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved Paul Fagerburg: Looks good to me, approved
diff --git a/src/soc/intel/common/block/include/intelblocks/lpss.h b/src/soc/intel/common/block/include/intelblocks/lpss.h index dafe351..e80f3dd 100644 --- a/src/soc/intel/common/block/include/intelblocks/lpss.h +++ b/src/soc/intel/common/block/include/intelblocks/lpss.h @@ -40,4 +40,13 @@ /* Set controller power state to D0 or D3*/ void lpss_set_power_state(const struct device *dev, enum lpss_pwr_state state);
+/* + * Handler to get list of LPSS controllers. The SOC is expected to send out a + * list of pci devfn for all LPSS controllers supported by the SOC. + */ +const pci_devfn_t *soc_lpss_controllers_list(size_t *size); + +/* Check if the device is a LPSS controller */ +bool is_dev_lpss(const struct device *dev); + #endif /* SOC_INTEL_COMMON_BLOCK_LPSS_H */ diff --git a/src/soc/intel/common/block/lpss/lpss.c b/src/soc/intel/common/block/lpss/lpss.c index a519bf6..1722dcd 100644 --- a/src/soc/intel/common/block/lpss/lpss.c +++ b/src/soc/intel/common/block/lpss/lpss.c @@ -89,3 +89,21 @@
pci_update_config8(lpss_dev, PME_CTRL_STATUS, ~POWER_STATE_MASK, state); } + +bool is_dev_lpss(const struct device *dev) +{ + static size_t size; + static const pci_devfn_t *lpss_devices; + + if (dev->path.type != DEVICE_PATH_PCI) + return false; + + if (!lpss_devices) + lpss_devices = soc_lpss_controllers_list(&size); + + for (int i = 0; i < size; i++) { + if (lpss_devices[i] == dev->path.pci.devfn) + return true; + } + return false; +}
-
Aamir Bohra (Code Review) -
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Martin Roth (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Fagerburg (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)