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

[Tom Rini]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230510/aabed74b/attachment.sig>