Broken CONFIG_SPL_SATA_SUPPORT=y

Mon Aug 2 23:56:58 CEST 2021

On Monday 02 August 2021 17:43:22 Tom Rini wrote:
> On Mon, Aug 02, 2021 at 03:30:20PM -0600, Simon Glass wrote:
> > Hi Pali,
> > 
> > On Mon, 2 Aug 2021 at 15:26, Pali Rohár <pali at kernel.org> wrote:>
> > > On Monday 02 August 2021 15:23:22 Simon Glass wrote:
> > > > () Hi Pali,
> > > >
> > > > On Mon, 2 Aug 2021 at 15:18, Pali Rohár <pali at kernel.org> wrote:
> > > > >
> > > > > On Monday 02 August 2021 15:14:30 Simon Glass wrote:
> > > > > > Hi Pali,
> > > > > >
> > > > > > On Mon, 2 Aug 2021 at 15:09, Pali Rohár <pali at kernel.org> wrote:
> > > > > > >
> > > > > > > On Monday 02 August 2021 16:55:34 Tom Rini wrote:
> > > > > > > > On Sun, Aug 01, 2021 at 02:25:16PM +0200, Pali Rohár wrote:
> > > > > > > >
> > > > > > > > > Hello!
> > > > > > > > >
> > > > > > > > > Option CONFIG_SPL_SATA_SUPPORT=y is currently broken in u-boot master
> > > > > > > > > branch. If I try to enable it for A38x platform I'm getting following
> > > > > > > > > compiler error:
> > > > > > > > >
> > > > > > > > >     LD      spl/u-boot-spl
> > > > > > > > >   arm-linux-gnueabihf-ld.bfd: drivers/ata/ahci.o: in function `ahci_probe_scsi_pci':
> > > > > > > > >   drivers/ata/ahci.c:1205: undefined reference to `dm_pci_map_bar'
> > > > > > > > >   arm-linux-gnueabihf-ld.bfd: drivers/ata/ahci.c:1215: undefined reference to `dm_pci_read_config16'
> > > > > > > > >   arm-linux-gnueabihf-ld.bfd: drivers/ata/ahci.c:1216: undefined reference to `dm_pci_read_config16'
> > > > > > > > >   arm-linux-gnueabihf-ld.bfd: drivers/ata/ahci.c:1220: undefined reference to `dm_pci_map_bar'
> > > > > > > > >   make[1]: *** [scripts/Makefile.spl:512: spl/u-boot-spl] Error 1
> > > > > > > > >   make: *** [Makefile:1977: spl/u-boot-spl] Error 2
> > > > > > > > >
> > > > > > > > > You can reproduce it by running following commands:
> > > > > > > > >
> > > > > > > > >   $ make turris_omnia_defconfig
> > > > > > > > >   $ echo CONFIG_SPL_SATA_SUPPORT=y >> .config
> > > > > > > > >   $ make CROSS_COMPILE=arm-linux-gnueabihf-
> > > > > > > > >
> > > > > > > > > I workaround it by following patch:
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> > > > > > > > > index d4047c04f5d0..6bad72e4cfa4 100644
> > > > > > > > > --- a/drivers/ata/ahci.c
> > > > > > > > > +++ b/drivers/ata/ahci.c
> > > > > > > > > @@ -1196,7 +1196,7 @@ int ahci_probe_scsi(struct udevice *ahci_dev, ulong base)
> > > > > > > > >     return 0;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > -#ifdef CONFIG_DM_PCI
> > > > > > > > > +#if CONFIG_IS_ENABLED(DM_PCI)
> > > > > > > > >  int ahci_probe_scsi_pci(struct udevice *ahci_dev)
> > > > > > > > >  {
> > > > > > > > >     ulong base;
> > > > > > > > >
> > > > > > > > > It fixed this particular problem. So it looks like that CONFIG_DM_PCI is
> > > > > > > > > defined also when building SPL even when it is not enabled for SPL.
> > > > > > > > > Whole PCI is disabled in SPL.
> > > > > > > > >
> > > > > > > > > But then I got another compile error:
> > > > > > > > >
> > > > > > > > >     LD      spl/u-boot-spl
> > > > > > > > >   arm-linux-gnueabihf-ld.bfd: drivers/ata/ahci-pci.o: in function `ahci_pci_probe':
> > > > > > > > >   drivers/ata/ahci-pci.c:21: undefined reference to `ahci_probe_scsi_pci'
> > > > > > > > >   make[1]: *** [scripts/Makefile.spl:512: spl/u-boot-spl] Error 1
> > > > > > > > >   make: *** [Makefile:1977: spl/u-boot-spl] Error 2
> > > > > > > > >
> > > > > > > > > Seems that u-boot is trying to compile and link ahci-pci.o into SPL
> > > > > > > > > binary even when it is not enabled nor used. PCI is completed disabled
> > > > > > > > > in SPL for this case.
> > > > > > > > >
> > > > > > > > > I workaround it by putting whole ahci-pci.c file into one big #idef:
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/ata/ahci-pci.c b/drivers/ata/ahci-pci.c
> > > > > > > > > index b1d231e0f9e1..34afebd2f87f 100644
> > > > > > > > > --- a/drivers/ata/ahci-pci.c
> > > > > > > > > +++ b/drivers/ata/ahci-pci.c
> > > > > > > > > @@ -9,6 +9,8 @@
> > > > > > > > >  #include <dm.h>
> > > > > > > > >  #include <pci.h>
> > > > > > > > >
> > > > > > > > > +#if CONFIG_IS_ENABLED(DM_PCI)
> > > > > > > > > +
> > > > > > > > >  static int ahci_pci_bind(struct udevice *dev)
> > > > > > > > >  {
> > > > > > > > >     struct udevice *scsi_dev;
> > > > > > > > > @@ -42,3 +44,5 @@ static struct pci_device_id ahci_pci_supported[] = {
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > >  U_BOOT_PCI_DEVICE(ahci_pci, ahci_pci_supported);
> > > > > > > > > +
> > > > > > > > > +#endif
> > > > > > > > >
> > > > > > > > > And then finally U-Boot produced final target image u-boot-spl.kwb:
> > > > > > > > >
> > > > > > > > >   LD      spl/u-boot-spl
> > > > > > > > >   OBJCOPY spl/u-boot-spl-nodtb.bin
> > > > > > > > >   SYM     spl/u-boot-spl.sym
> > > > > > > > >   CAT     spl/u-boot-spl-dtb.bin
> > > > > > > > >   COPY    spl/u-boot-spl.bin
> > > > > > > > >   MKIMAGE u-boot-spl.kwb
> > > > > > > > >
> > > > > > > > > So this looks like a bug in Kconfig or Makefile dependences that build
> > > > > > > > > system is trying to compile and link also files which should not be
> > > > > > > > > linked at all.
> > > > > > > >
> > > > > > > > Yes, it's a missing dependency in Kconfig.  You need to enable SPL_DM
> > > > > > > > and then quite possibly SPL_OF_CONTROL (I forget if that was a hard
> > > > > > > > dependency on the main conversion or not) on the platform.
> > > > > > >
> > > > > > > CONFIG_SPL_DM=y is already enabled
> > > > > >
> > > > > > CONFIG_SPL_PCI ?
> > > > > >
> > > > > > It seems that pci-uclass.c is not being built for SPL.
> > > > >
> > > > > CONFIG_SPL_PCI is not enabled.
> > > > >
> > > > > But why should be CONFIG_SPL_PCI needed for SATA? As above my patches
> > > > > show no PCI is required for SATA...
> > > >
> > > > I suggest investigating what is calling those functions you mention...
> > > >
> > > > If you look at ahci_probe_scsi_pci() it is bracketed by:
> > > >
> > > > #ifdef CONFIG_DM_PCI
> > > >
> > > > Perhaps it should be:
> > > >
> > > > #if CONFIG_IS_ENABLED(DM_PCI)
> > > >
> > > > so it takes account of SPL?
> > >
> > > And this is exactly what I did in first patch attempt.
> > 
> > Then how about changing the Makefile rule to:
> > 
> > obj-$(CONFIG_$(SPL_TPL_)AHCI_PCI) += ahci-pci.o
> 
> But there is no SPL_AHCI_PCI nor TPL_AHCI_PCI.  I really want to know in
> addition to how SATA is hooked up on this board what the expected truth
> table of U-Boot and SPL CONFIG options is expected to be.

What do you mean by hooked? SATA support on A38x is provided by standard
AHCI interface implemented by ahci_mvebu.c driver (CONFIG_AHCI_MVEBU).
No PCIe is involved.

But I think this is not board neither platform specific, but general SPL
U-Boot issue. As I mentioned in the first email, U-Boot build system is
trying to compile and link files into SPL which should not and which are
not enabled in config at all. More precisely, build system is trying to
link into SPL binary functions from AHCI_PCI even when PCI is not
enabled in SPL. PCI is enabled only in proper U-Boot.

So this looks like a broken dependency either in Makefile or Kconfig,
but I really do not know where nor why it happens.

AHCI_PCI depends on PCI and AHCI. AHCI_MVEBU depends only on AHCI. So
when PCI is not enabled in SPL then also AHCI_PCI must not be enabled in
SPL. But AHCI still can be enabled in SPL even when AHCI_PCI is
disabled. Therefore also AHCI_MVEBU can be enabled in SPL when PCI is
disabled in SPL.