Re: [PATCH v1 17/24] ns16550: match when to define bdf with uart code

Troy Kisky
2023-02-22T18:43:10Z

On Wed, Feb 22, 2023 at 10:18 AM Tom Rini <trini@konsulko.com> wrote:

> On Tue, Feb 21, 2023 at 05:38:14PM -0800, Troy Kisky wrote:
>
> > When switching defined(CONFIG_PCI) to CONFIG_IS_ENABLED(PCI)
> > bdf is no longer accessible. So change to preprocessor to avoid access.
> >
> > Signed-off-by: Troy Kisky <troykiskyboundary@gmail.com>
> > ---
> >
> > arch/x86/cpu/apollolake/uart.c | 6 +++---
> > include/ns16550.h | 2 +-
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/cpu/apollolake/uart.c
> b/arch/x86/cpu/apollolake/uart.c
> > index a9362436000..143217755ff 100644
> > --- a/arch/x86/cpu/apollolake/uart.c
> > +++ b/arch/x86/cpu/apollolake/uart.c
> > @@ -79,11 +79,11 @@ void apl_uart_init(pci_dev_t bdf, ulong base)
> >
> > static int apl_ns16550_probe(struct udevice *dev)
> > {
> > +#if !CONFIG_IS_ENABLED(PCI)
> > struct apl_ns16550_plat *plat = dev_get_plat(dev);
> >
> > - if (!CONFIG_IS_ENABLED(PCI))
> > - apl_uart_init(plat->ns16550.bdf, plat->ns16550.base);
> > -
> > + apl_uart_init(plat->ns16550.bdf, plat->ns16550.base);
> > +#endif
> > return ns16550_serial_probe(dev);
> > }
> >
> > diff --git a/include/ns16550.h b/include/ns16550.h
> > index e7e68663d03..8d7eb7d8f9c 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 !CONFIG_IS_ENABLED(PCI) || CONFIG_IS_ENABLED(OF_PLATDATA)
> > int bdf;
> > #endif
> > };
>
> This isn't equivalent. This means platforms such as am335x_evm which do
> not enable PCI nor SPL_PCI now get this field and grow their rodata.
>
> --
> Tom
>

tkisky@OS2:~/u-boot-tkisky$ make am335x_evm_defconfig
#
# configuration written to .config
#
tkisky@OS2:~/u-boot-tkisky$ grep -w CONFIG_INTEL_APOLLOLAKE .config
tkisky@OS2:~/u-boot-tkisky$

Would this be better ?
#if defined(CONFIG_INTEL_APOLLOLAKE) && ( !CONFIG_IS_ENABLED(PCI) ||
CONFIG_IS_ENABLED(OF_PLATDATA))

I don't understand what bdf is, but
#if defined(CONFIG_PCI) && defined(CONFIG_SPL)
int bdf;
#endif

in the include/ns16550.h, doesn't make sense with the usage in uart.c
if (!CONFIG_IS_ENABLED(PCI))
apl_uart_init(plat->ns16550.bdf, plat->ns16550.base);
_________
I think I might be trying to paper over a bug.

Thanks
Troy