Hi Tom,
You are looking at an old patch, here's the new.
commit c969bedb9cb6029360e6fe7e25a331680fabe3ee Author: Troy Kisky troykiskyboundary@gmail.com Date: Thu Feb 23 08:01:46 2023 -0800
ns16550: match when to define bdf with uart code
When switching defined(CONFIG_PCI) to CONFIG_IS_ENABLED(PCI) bdf is no longer accessible. So add preprocessor protection to avoid access.
Series-changes: 2 - changed condition of when to include field bdf - added protection to another instance of bdf in uart.c - Thanks to Simon for getting this corrected
Signed-off-by: Troy Kisky troykiskyboundary@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/arch/x86/cpu/apollolake/uart.c b/arch/x86/cpu/apollolake/uart.c index a9362436000..878aa48ed76 100644 --- a/arch/x86/cpu/apollolake/uart.c +++ b/arch/x86/cpu/apollolake/uart.c @@ -79,10 +79,12 @@ void apl_uart_init(pci_dev_t bdf, ulong base)
static int apl_ns16550_probe(struct udevice *dev) { +#if IS_ENABLED_NOCHECK(CONFIG_PCI) && defined(CONFIG_SPL_BUILD) struct apl_ns16550_plat *plat = dev_get_plat(dev);
if (!CONFIG_IS_ENABLED(PCI)) apl_uart_init(plat->ns16550.bdf, plat->ns16550.base); +#endif
return ns16550_serial_probe(dev); } @@ -110,7 +112,9 @@ static int apl_ns16550_of_to_plat(struct udevice *dev) ns.reg_offset = 0; ns.clock = dtplat->clock_frequency; ns.fcr = UART_FCR_DEFVAL; +#if IS_ENABLED_NOCHECK(CONFIG_PCI) && defined(CONFIG_SPL_BUILD) ns.bdf = pci_ofplat_get_devfn(dtplat->reg[0]); +#endif memcpy(plat, &ns, sizeof(ns)); #else int ret; diff --git a/include/ns16550.h b/include/ns16550.h index e7e68663d03..41b977b5b26 100644 --- a/include/ns16550.h +++ b/include/ns16550.h @@ -74,7 +74,7 @@ struct ns16550_plat { int clock; u32 fcr; int flags; -#if defined(CONFIG_PCI) && defined(CONFIG_SPL) +#if IS_ENABLED_NOCHECK(CONFIG_PCI) && defined(CONFIG_SPL_BUILD) int bdf; #endif }; __________________________
It reads as, If the bdf exists, then if spl var PCI is not enabled, then init uart.
The PCI code will handle it if PCI is enabled.
So, PCI needs to be enabled, and SPL_PCI needs to not be enabled, and currently building for SPL
On Wed, May 10, 2023 at 1:49 PM Tom Rini trini@konsulko.com wrote:
On Wed, May 10, 2023 at 02:46:14PM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 10 May 2023 at 14:41, Tom Rini trini@konsulko.com wrote:
On Mon, Mar 13, 2023 at 02:31:39PM -0700, Troy Kisky wrote:
When switching defined(CONFIG_PCI) to CONFIG_IS_ENABLED(PCI) bdf is no longer accessible. So add preprocessor protection to avoid access.
Signed-off-by: Troy Kisky troykiskyboundary@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- changed condition of when to include field bdf
- added protection to another instance of bdf in uart.c
- Thanks to Simon for getting this corrected
arch/x86/cpu/apollolake/uart.c | 4 ++++ include/ns16550.h | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/cpu/apollolake/uart.c
b/arch/x86/cpu/apollolake/uart.c
index a9362436000..878aa48ed76 100644 --- a/arch/x86/cpu/apollolake/uart.c +++ b/arch/x86/cpu/apollolake/uart.c @@ -79,10 +79,12 @@ void apl_uart_init(pci_dev_t bdf, ulong base)
static int apl_ns16550_probe(struct udevice *dev) { +#if IS_ENABLED(CONFIG_PCI) && defined(CONFIG_SPL_BUILD) struct apl_ns16550_plat *plat = dev_get_plat(dev);
if (!CONFIG_IS_ENABLED(PCI)) apl_uart_init(plat->ns16550.bdf, plat->ns16550.base);+#endif
return ns16550_serial_probe(dev);}
Looking at this again, this hunk here doesn't make sense. Reading outloud, "if we have CONFIG_PCI enabled and CONFIG_SPL_BUILD, declare plat to be .. and then if we do not have PCI enabled ...". I really don't know what the code is supposed to be doing here.
This is because the bdf member is only present in SPL when PCI is
enabled.
Perhaps it could be CONFIG_SPL instead of CONFIG_SPL_BUILD ?
You can apply whatever makes it build if you like. I can look at it either tonight or when I get back.
Digging at this when you get back is fine since it still makes no sense to say "If PCI is enabled, when PCI is not enabled actually do something". There's no side-effects on "dev" from the call to dev_get_plat which is the only thing that happens, in this case.
-- Tom