On Mon, Aug 02, 2021 at 11:41:29PM +0200, Pali Rohár wrote:
> On Monday 02 August 2021 15:30:20 Simon Glass wrote:
> > Hi Pali,
> >
> > On Mon, 2 Aug 2021 at 15:26, Pali Rohár <p...@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 <p...@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 <p...@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
>
> Ok, this patch also fixes above compile errors:
>
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 4811b2f82c4e..d48001b51848 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -5,7 +5,7 @@
>
> obj-$(CONFIG_DWC_AHCI) += dwc_ahci.o
> obj-$(CONFIG_AHCI) += ahci-uclass.o
> -obj-$(CONFIG_AHCI_PCI) += ahci-pci.o
> +obj-$(CONFIG_$(SPL_TPL_)AHCI_PCI) += ahci-pci.o
> obj-$(CONFIG_SCSI_AHCI) += ahci.o
> obj-$(CONFIG_DWC_AHSATA) += dwc_ahsata.o
> obj-$(CONFIG_FSL_SATA) += fsl_sata.o
> 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;
>
> But it is correct to use this
> 'obj-$(CONFIG_$(SPL_TPL_)AHCI_PCI) += ahci-pci.o'
> part in Makefile? And why other object files do not have that
> $(SPL_TPL_) guard?I think before we can figure out what the right solution is, we need to know the answer to what I asked in another part of the thread (and maybe our mails are just about to cross..) which is how SATA is hooked up on this board? -- Tom
signature.asc
Description: PGP signature
