[PATCH v3 17/23] ns16550: match when to define bdf with uart code

[Troy Kisky]

Hi Tom,

You are looking at an old patch, here's the new.

commit c969bedb9cb6029360e6fe7e25a331680fabe3ee
Author: Troy Kisky <troykiskyboundary at 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 at gmail.com>
    Reviewed-by: Simon Glass <sjg at 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 at 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 at 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 at gmail.com>
> > > > Reviewed-by: Simon Glass <sjg at 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
>