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

[Simon Glass]

Hi Troy,

On Wed, 22 Feb 2023 at 11:43, Troy Kisky <troykiskyboundary at gmail.com> wrote:
>
> On Wed, Feb 22, 2023 at 10:18 AM Tom Rini <trini at 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 at 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 at OS2:~/u-boot-tkisky$ make am335x_evm_defconfig
> #
> # configuration written to .config
> #
> tkisky at OS2:~/u-boot-tkisky$ grep -w CONFIG_INTEL_APOLLOLAKE .config
> tkisky at 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

bus, device, function. Basically it identifies everything on the PCI
bus that might need a driver.

>
> 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.

Yes I agree, and we have a number of these sorts of inconsistencies.

You can also add a static inline accessor, as is done with
gd_multi_dtb_fit(), for example. That would be simpler.

Specifically, for Intel Apollo Lake, the PCI Kconfig means that PCI
support is enabled in the U-Boot phase. It doesn't mean that the board
doesn't have PCI (it always does), It's just that in early stages we
don't want to bring in all that code. I believe that the bdf field is
only used in TPL, before PCI is enabled.

So an accessor seems best, or make use of Heinrich's new thing [1]

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20230219113629.39142-3-heinrich.schuchardt@canonical.com/