Broken CONFIG_SPL_SATA_SUPPORT=y

Broken CONFIG_SPL_SATA_SUPPORT=y

[Pali Rohár]

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.

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Simon Glass]

Hi Pali,

On Sun, 1 Aug 2021 at 06:25, Pali Rohár <pali@kernel.org> 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.

I sent a series to drop DM_PCI:

http://patchwork.ozlabs.org/project/uboot/list/?series=256181

Can you give that a try and see if it helps?

Regards,
Simon

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Pali Rohár]

On Monday 02 August 2021 13:21:51 Simon Glass wrote:
> Hi Pali,
> 
> On Sun, 1 Aug 2021 at 06:25, Pali Rohár <pali@kernel.org> 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.
> 
> I sent a series to drop DM_PCI:
> 
> http://patchwork.ozlabs.org/project/uboot/list/?series=256181
> 
> Can you give that a try and see if it helps?

  $ make turris_omnia_defconfig
  $ echo CONFIG_SPL_SATA_SUPPORT=y >> .config
  $ make CROSS_COMPILE=arm-linux-gnueabihf-
  ...
    LD      spl/u-boot-spl
  arm-linux-gnueabihf-ld.bfd: drivers/ata/ahci.o: in function `ahci_probe_scsi_pci':
  /home/pali/omnia/u-boot/drivers/ata/ahci.c:1151: undefined reference to `dm_pci_map_bar'
  arm-linux-gnueabihf-ld.bfd: /home/pali/omnia/u-boot/drivers/ata/ahci.c:1161: undefined reference to `dm_pci_read_config16'
  arm-linux-gnueabihf-ld.bfd: /home/pali/omnia/u-boot/drivers/ata/ahci.c:1162: undefined reference to `dm_pci_read_config16'
  arm-linux-gnueabihf-ld.bfd: /home/pali/omnia/u-boot/drivers/ata/ahci.c:1166: undefined reference to `dm_pci_map_bar'
  make[1]: *** [scripts/Makefile.spl:512: spl/u-boot-spl] Error 1
  make: *** [Makefile:1998: spl/u-boot-spl] Error 2

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3481 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Pali Rohár]

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

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Simon Glass]

Hi Pali,

On Mon, 2 Aug 2021 at 15:09, Pali Rohár <pali@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.

Regards,
SImon

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Pali Rohár]

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

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Simon Glass]

() Hi Pali,

On Mon, 2 Aug 2021 at 15:18, Pali Rohár <pali@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@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?

Regards,
SImon

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Pali Rohár]

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

> Regards,
> SImon

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Simon Glass]

Hi Pali,

On Mon, 2 Aug 2021 at 15:26, Pali Rohár <pali@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@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@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

Regards,
SImon

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5786 bytes --]

On Mon, Aug 02, 2021 at 11:25:54PM +0200, Pali Rohár 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@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@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.

I think we're missing how SATA is hooked up on this board.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Pali Rohár]

On Monday 02 August 2021 15:30:20 Simon Glass wrote:
> Hi Pali,
> 
> On Mon, 2 Aug 2021 at 15:26, Pali Rohár <pali@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@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@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?

> Regards,
> SImon

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6376 bytes --]

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

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Pali Rohár]

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

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7850 bytes --]

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 <pali@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@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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 8427 bytes --]

On Mon, Aug 02, 2021 at 11:56:58PM +0200, Pali Rohár wrote:
> 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@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@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@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.

OK, so why are you building drivers/ata/ahci-pci.c at all?  Should you
be enabling SCSI_AHCI_PLAT like the armada 37xx and 8k platforms are?

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

It's a combination of a lack combination testing really.

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

Why would you be enabling AHCI_PCI and not enabling PCI?  Or, what
functionality would you be using in that case?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Pali Rohár]

On Monday 02 August 2021 18:11:20 Tom Rini wrote:
> On Mon, Aug 02, 2021 at 11:56:58PM +0200, Pali Rohár wrote:
> > 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@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@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@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.
> 
> OK, so why are you building drivers/ata/ahci-pci.c at all?

ahci-pci is enabled for proper U-Boot. It is driver for PCIe add-in
cards which speak AHCI protocol. You can e.g. insert SATA-based SSD disk
into e.g. mPCIe form-factor AHCI card and then this mPCIe card you can
insert into mPCIe slot on the board.

Obviously you cannot load U-Boot from this SSD disk (as bootrom does not
support it) but you can boot Linux kernel and whole Linux system from
this disk via proper U-Boot (not SPL), if you enable this driver in
U-Boot.

So for this kind of usage, it makes sense to enable also driver for
AHCI_PCI in proper U-Boot (not SPL).

> Should you
> be enabling SCSI_AHCI_PLAT like the armada 37xx and 8k platforms are?

But this is something totally different, not?

> > 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.
> 
> It's a combination of a lack combination testing really.
> 
> > 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.
> 
> Why would you be enabling AHCI_PCI and not enabling PCI?  Or, what
> functionality would you be using in that case?

Seems that there is some kind of misunderstanding.

Enabling AHCI_PCI without PCI should not be possible and should be
disallowed. I'm not trying to enable such combination, I'm trying to
show you that build system somehow enabled this nonsense combination. Or
for some unknown reason it is trying to use AHCI_PCI in SPL when PCI is
not enabled in SPL. And I really do not know what is happening here.

My guess is that there is some mix of SPL and non-SPL symbol
dependences. Build system sees that PCI is enabled for proper U-Boot and
this information somehow propagates into SPL and build system "force"
linking of AHCI_PCI into SPL even no PCI is enabled for SPL. And also
even when there is no AHCI_PCI config for SPL.

So what I need to achieve:
* enable AHCI_MVEBU in both proper U-Boot and SPL
* enable PCI and AHCI_PCI in proper U-Boot
* disable PCI and AHCI_PCI in SPL
* make dependences between these options and SPL combination correctly
  defined, so it would not be possible to enable nonsense combination

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 11859 bytes --]

On Tue, Aug 03, 2021 at 12:38:19AM +0200, Pali Rohár wrote:
> On Monday 02 August 2021 18:11:20 Tom Rini wrote:
> > On Mon, Aug 02, 2021 at 11:56:58PM +0200, Pali Rohár wrote:
> > > 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@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@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@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.
> > 
> > OK, so why are you building drivers/ata/ahci-pci.c at all?
> 
> ahci-pci is enabled for proper U-Boot. It is driver for PCIe add-in
> cards which speak AHCI protocol. You can e.g. insert SATA-based SSD disk
> into e.g. mPCIe form-factor AHCI card and then this mPCIe card you can
> insert into mPCIe slot on the board.
> 
> Obviously you cannot load U-Boot from this SSD disk (as bootrom does not
> support it) but you can boot Linux kernel and whole Linux system from
> this disk via proper U-Boot (not SPL), if you enable this driver in
> U-Boot.
> 
> So for this kind of usage, it makes sense to enable also driver for
> AHCI_PCI in proper U-Boot (not SPL).

OK, thanks, that does help.

> > Should you
> > be enabling SCSI_AHCI_PLAT like the armada 37xx and 8k platforms are?
> 
> But this is something totally different, not?

Not exactly, no.  This shows one of the valid criticism of having both
SPL and U-Boot (or TPL and SPL and U-Boot) build from a single config
file.  If you had that option enabled, none of the SPL build part would
try and do PCI things.  But if you had that enabled, full U-Boot would
then fail to do PCI disks.

> > > 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.
> > 
> > It's a combination of a lack combination testing really.
> > 
> > > 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.
> > 
> > Why would you be enabling AHCI_PCI and not enabling PCI?  Or, what
> > functionality would you be using in that case?
> 
> Seems that there is some kind of misunderstanding.

Yes, but I think I see it now.

> Enabling AHCI_PCI without PCI should not be possible and should be
> disallowed. I'm not trying to enable such combination, I'm trying to
> show you that build system somehow enabled this nonsense combination. Or
> for some unknown reason it is trying to use AHCI_PCI in SPL when PCI is
> not enabled in SPL. And I really do not know what is happening here.
> 
> My guess is that there is some mix of SPL and non-SPL symbol
> dependences. Build system sees that PCI is enabled for proper U-Boot and
> this information somehow propagates into SPL and build system "force"
> linking of AHCI_PCI into SPL even no PCI is enabled for SPL. And also
> even when there is no AHCI_PCI config for SPL.

Right, so as you guess, there's a mix of SPL and non-SPL symbols,
because we don't have SPL symbols for everything, and haven't needed to
duplicate any of them for PCI or SATA until now.

> So what I need to achieve:
> * enable AHCI_MVEBU in both proper U-Boot and SPL
> * enable PCI and AHCI_PCI in proper U-Boot
> * disable PCI and AHCI_PCI in SPL
> * make dependences between these options and SPL combination correctly
>   defined, so it would not be possible to enable nonsense combination

Can you not just enable PCI and AHCI_PCI within SPL too?  Just because
ROM doesn't support it doesn't mean SPL cannot.  Or do you not have the
room in the binary?  If you don't then yes, a number of symbols need to
be duplicated to have SPL_FOO options, and code updated to use
CONFIG_IS_ENABLED(FOO).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Pali Rohár]

On Monday 02 August 2021 18:46:48 Tom Rini wrote:
> On Tue, Aug 03, 2021 at 12:38:19AM +0200, Pali Rohár wrote:
> > On Monday 02 August 2021 18:11:20 Tom Rini wrote:
> > > On Mon, Aug 02, 2021 at 11:56:58PM +0200, Pali Rohár wrote:
> > > > 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@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@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@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.
> > > 
> > > OK, so why are you building drivers/ata/ahci-pci.c at all?
> > 
> > ahci-pci is enabled for proper U-Boot. It is driver for PCIe add-in
> > cards which speak AHCI protocol. You can e.g. insert SATA-based SSD disk
> > into e.g. mPCIe form-factor AHCI card and then this mPCIe card you can
> > insert into mPCIe slot on the board.
> > 
> > Obviously you cannot load U-Boot from this SSD disk (as bootrom does not
> > support it) but you can boot Linux kernel and whole Linux system from
> > this disk via proper U-Boot (not SPL), if you enable this driver in
> > U-Boot.
> > 
> > So for this kind of usage, it makes sense to enable also driver for
> > AHCI_PCI in proper U-Boot (not SPL).
> 
> OK, thanks, that does help.
> 
> > > Should you
> > > be enabling SCSI_AHCI_PLAT like the armada 37xx and 8k platforms are?
> > 
> > But this is something totally different, not?
> 
> Not exactly, no.  This shows one of the valid criticism of having both
> SPL and U-Boot (or TPL and SPL and U-Boot) build from a single config
> file.  If you had that option enabled, none of the SPL build part would
> try and do PCI things.  But if you had that enabled, full U-Boot would
> then fail to do PCI disks.
> 
> > > > 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.
> > > 
> > > It's a combination of a lack combination testing really.
> > > 
> > > > 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.
> > > 
> > > Why would you be enabling AHCI_PCI and not enabling PCI?  Or, what
> > > functionality would you be using in that case?
> > 
> > Seems that there is some kind of misunderstanding.
> 
> Yes, but I think I see it now.
> 
> > Enabling AHCI_PCI without PCI should not be possible and should be
> > disallowed. I'm not trying to enable such combination, I'm trying to
> > show you that build system somehow enabled this nonsense combination. Or
> > for some unknown reason it is trying to use AHCI_PCI in SPL when PCI is
> > not enabled in SPL. And I really do not know what is happening here.
> > 
> > My guess is that there is some mix of SPL and non-SPL symbol
> > dependences. Build system sees that PCI is enabled for proper U-Boot and
> > this information somehow propagates into SPL and build system "force"
> > linking of AHCI_PCI into SPL even no PCI is enabled for SPL. And also
> > even when there is no AHCI_PCI config for SPL.
> 
> Right, so as you guess, there's a mix of SPL and non-SPL symbols,
> because we don't have SPL symbols for everything, and haven't needed to
> duplicate any of them for PCI or SATA until now.
> 
> > So what I need to achieve:
> > * enable AHCI_MVEBU in both proper U-Boot and SPL
> > * enable PCI and AHCI_PCI in proper U-Boot
> > * disable PCI and AHCI_PCI in SPL
> > * make dependences between these options and SPL combination correctly
> >   defined, so it would not be possible to enable nonsense combination
> 
> Can you not just enable PCI and AHCI_PCI within SPL too?  Just because
> ROM doesn't support it doesn't mean SPL cannot.  Or do you not have the
> room in the binary?  If you don't then yes, a number of symbols need to
> be duplicated to have SPL_FOO options, and code updated to use
> CONFIG_IS_ENABLED(FOO).

In the last mvebu patch series I tried to reduce size of SPL binary and
also to increase/remove limit for its maximal size. For more mvebu
boards SPL was near its maximal limit as U-Boot is increasing its size
in time.

So enabling unused features and unused code into SPL is not a good idea.

Moreover 32bit mvebu is kind of special in way how SPL and proper U-Boot
is used. Bootrom can boot only image in vendor format (called kwbimage)
which is divided into two main parts: DDR init code (loaded only into
CPU cache, responsible for initializing RAM and returning to bootrom)
and main bootloader. Bootloader in our case is proper U-Boot and DDR
init code is now U-Boot SPL binary. (In past for DDR vendor binary was
used, but now SPL contains required DDR driver and so is used). This
applies to 32bit mvebu which includes at least axp, a37x, a38x, msys...

"Proper vendor way" is that DDR init code (SPL in our case) returns back
to bootrom which loads then rest of the image/bootloader (proper U-Boot
in our case). But because bootrom is reading SPI-NOR slowly, there is
option to compile SPL to not return to bootrom, but rather load this
image (in vendor format) from SPI-NOR and de-facto use standard way for
loading/booting proper U-Boot from SPL. Using of returning to bootrom
has advantage in smaller binary and now everybody can decide if want
smaller binary or binary which is faster for SPI-NOR. If you are
targeting UART booting, you would choose "smaller binary" option as it
would be faster transferred over UART.

You wrote "Just because ROM doesn't support it doesn't mean SPL cannot."
That would mean that you completely want to ignore "bootloader" part of
vendor kwbimage. And supplied ddr init code (SPL) does not return to
bootrom and instead loads proper U-Boot from completely different
storage about which bootrom has absolutely no idea. Theoretically it
should be possible and then into kwbimage bootloader part you put some
1 byte nop image. But I think that nobody ever tried it with targeting
AHCI_PCI. Plus current U-Boot Makefile always put proper U-Boot into
"bootloader part of kwimage". So even U-Boot build system is not
prepared for this kind of "experiments" :-) It could be interesting
upgrade for above mentioned platforms but I think that there is no such
big value in it...

PS: mvebu bootrom supports booting also from PCIe, but this is works
differently as all other methods and does not match how PCIe works in
U-Boot SPL. mvebu bootrom initialize PCIe device, enable bus mastering,
enable memory access and then expects that PCIe device initializes DDR,
then transfers bootloader into RAM (from some its own specific storage)
and poke bootrom to start executing bootloader from RAM.

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 16137 bytes --]

On Tue, Aug 03, 2021 at 01:33:53AM +0200, Pali Rohár wrote:
> On Monday 02 August 2021 18:46:48 Tom Rini wrote:
> > On Tue, Aug 03, 2021 at 12:38:19AM +0200, Pali Rohár wrote:
> > > On Monday 02 August 2021 18:11:20 Tom Rini wrote:
> > > > On Mon, Aug 02, 2021 at 11:56:58PM +0200, Pali Rohár wrote:
> > > > > 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@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@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@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.
> > > > 
> > > > OK, so why are you building drivers/ata/ahci-pci.c at all?
> > > 
> > > ahci-pci is enabled for proper U-Boot. It is driver for PCIe add-in
> > > cards which speak AHCI protocol. You can e.g. insert SATA-based SSD disk
> > > into e.g. mPCIe form-factor AHCI card and then this mPCIe card you can
> > > insert into mPCIe slot on the board.
> > > 
> > > Obviously you cannot load U-Boot from this SSD disk (as bootrom does not
> > > support it) but you can boot Linux kernel and whole Linux system from
> > > this disk via proper U-Boot (not SPL), if you enable this driver in
> > > U-Boot.
> > > 
> > > So for this kind of usage, it makes sense to enable also driver for
> > > AHCI_PCI in proper U-Boot (not SPL).
> > 
> > OK, thanks, that does help.
> > 
> > > > Should you
> > > > be enabling SCSI_AHCI_PLAT like the armada 37xx and 8k platforms are?
> > > 
> > > But this is something totally different, not?
> > 
> > Not exactly, no.  This shows one of the valid criticism of having both
> > SPL and U-Boot (or TPL and SPL and U-Boot) build from a single config
> > file.  If you had that option enabled, none of the SPL build part would
> > try and do PCI things.  But if you had that enabled, full U-Boot would
> > then fail to do PCI disks.
> > 
> > > > > 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.
> > > > 
> > > > It's a combination of a lack combination testing really.
> > > > 
> > > > > 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.
> > > > 
> > > > Why would you be enabling AHCI_PCI and not enabling PCI?  Or, what
> > > > functionality would you be using in that case?
> > > 
> > > Seems that there is some kind of misunderstanding.
> > 
> > Yes, but I think I see it now.
> > 
> > > Enabling AHCI_PCI without PCI should not be possible and should be
> > > disallowed. I'm not trying to enable such combination, I'm trying to
> > > show you that build system somehow enabled this nonsense combination. Or
> > > for some unknown reason it is trying to use AHCI_PCI in SPL when PCI is
> > > not enabled in SPL. And I really do not know what is happening here.
> > > 
> > > My guess is that there is some mix of SPL and non-SPL symbol
> > > dependences. Build system sees that PCI is enabled for proper U-Boot and
> > > this information somehow propagates into SPL and build system "force"
> > > linking of AHCI_PCI into SPL even no PCI is enabled for SPL. And also
> > > even when there is no AHCI_PCI config for SPL.
> > 
> > Right, so as you guess, there's a mix of SPL and non-SPL symbols,
> > because we don't have SPL symbols for everything, and haven't needed to
> > duplicate any of them for PCI or SATA until now.
> > 
> > > So what I need to achieve:
> > > * enable AHCI_MVEBU in both proper U-Boot and SPL
> > > * enable PCI and AHCI_PCI in proper U-Boot
> > > * disable PCI and AHCI_PCI in SPL
> > > * make dependences between these options and SPL combination correctly
> > >   defined, so it would not be possible to enable nonsense combination
> > 
> > Can you not just enable PCI and AHCI_PCI within SPL too?  Just because
> > ROM doesn't support it doesn't mean SPL cannot.  Or do you not have the
> > room in the binary?  If you don't then yes, a number of symbols need to
> > be duplicated to have SPL_FOO options, and code updated to use
> > CONFIG_IS_ENABLED(FOO).
> 
> In the last mvebu patch series I tried to reduce size of SPL binary and
> also to increase/remove limit for its maximal size. For more mvebu
> boards SPL was near its maximal limit as U-Boot is increasing its size
> in time.
> 
> So enabling unused features and unused code into SPL is not a good idea.
> 
> Moreover 32bit mvebu is kind of special in way how SPL and proper U-Boot
> is used. Bootrom can boot only image in vendor format (called kwbimage)
> which is divided into two main parts: DDR init code (loaded only into
> CPU cache, responsible for initializing RAM and returning to bootrom)
> and main bootloader. Bootloader in our case is proper U-Boot and DDR
> init code is now U-Boot SPL binary. (In past for DDR vendor binary was
> used, but now SPL contains required DDR driver and so is used). This
> applies to 32bit mvebu which includes at least axp, a37x, a38x, msys...
> 
> "Proper vendor way" is that DDR init code (SPL in our case) returns back
> to bootrom which loads then rest of the image/bootloader (proper U-Boot
> in our case). But because bootrom is reading SPI-NOR slowly, there is
> option to compile SPL to not return to bootrom, but rather load this
> image (in vendor format) from SPI-NOR and de-facto use standard way for
> loading/booting proper U-Boot from SPL. Using of returning to bootrom
> has advantage in smaller binary and now everybody can decide if want
> smaller binary or binary which is faster for SPI-NOR. If you are
> targeting UART booting, you would choose "smaller binary" option as it
> would be faster transferred over UART.
> 
> You wrote "Just because ROM doesn't support it doesn't mean SPL cannot."
> That would mean that you completely want to ignore "bootloader" part of
> vendor kwbimage. And supplied ddr init code (SPL) does not return to
> bootrom and instead loads proper U-Boot from completely different
> storage about which bootrom has absolutely no idea. Theoretically it
> should be possible and then into kwbimage bootloader part you put some
> 1 byte nop image. But I think that nobody ever tried it with targeting
> AHCI_PCI. Plus current U-Boot Makefile always put proper U-Boot into
> "bootloader part of kwimage". So even U-Boot build system is not
> prepared for this kind of "experiments" :-) It could be interesting
> upgrade for above mentioned platforms but I think that there is no such
> big value in it...
> 
> PS: mvebu bootrom supports booting also from PCIe, but this is works
> differently as all other methods and does not match how PCIe works in
> U-Boot SPL. mvebu bootrom initialize PCIe device, enable bus mastering,
> enable memory access and then expects that PCIe device initializes DDR,
> then transfers bootloader into RAM (from some its own specific storage)
> and poke bootrom to start executing bootloader from RAM.

OK, thanks.  Then the right fix is to introduce whatever SPL_xxx symbols
you need so that you can turn off what you don't want in SPL but leave
it enabled in full U-Boot, and update the code to use
CONFIG_IS_ENABLED(xxx).  If you can do that part, I can iterate over
converting everyone else that assumes what we have today is what it
wants in SPL and non-SPL.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Pali Rohár]

On Monday 02 August 2021 21:40:07 Tom Rini wrote:
> OK, thanks.  Then the right fix is to introduce whatever SPL_xxx symbols
> you need so that you can turn off what you don't want in SPL but leave
> it enabled in full U-Boot, and update the code to use
> CONFIG_IS_ENABLED(xxx).  If you can do that part, I can iterate over
> converting everyone else that assumes what we have today is what it
> wants in SPL and non-SPL.

Hello Tom! Below is patch which adds SPL_AHCI_PCI symbol. I'm not sure
if this is correct approach but it looks like that it fixed that issue.
With this patch I can compile U-Boot with and also without enabling of
CONFIG_SPL_SATA_SUPPORT option.


diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 0c1490a9f900..96c7c30375bd 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -41,6 +41,12 @@ config AHCI_PCI
 	help
 	  Enables support for the PCI-based AHCI controller.
 
+config SPL_AHCI_PCI
+	bool "Support for PCI-based AHCI controller for SPL"
+	depends on SPL
+	depends on SPL_PCI
+	depends on SPL_SATA_SUPPORT && DM_SCSI
+
 config SATA_CEVA
 	bool "Ceva Sata controller"
 	depends on AHCI
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 4811b2f82c4e..cd88131dcd13 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_)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

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1854 bytes --]

On Sun, Aug 15, 2021 at 01:48:26PM +0200, Pali Rohár wrote:
> On Monday 02 August 2021 21:40:07 Tom Rini wrote:
> > OK, thanks.  Then the right fix is to introduce whatever SPL_xxx symbols
> > you need so that you can turn off what you don't want in SPL but leave
> > it enabled in full U-Boot, and update the code to use
> > CONFIG_IS_ENABLED(xxx).  If you can do that part, I can iterate over
> > converting everyone else that assumes what we have today is what it
> > wants in SPL and non-SPL.
> 
> Hello Tom! Below is patch which adds SPL_AHCI_PCI symbol. I'm not sure
> if this is correct approach but it looks like that it fixed that issue.
> With this patch I can compile U-Boot with and also without enabling of
> CONFIG_SPL_SATA_SUPPORT option.
> 
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 0c1490a9f900..96c7c30375bd 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -41,6 +41,12 @@ config AHCI_PCI
>  	help
>  	  Enables support for the PCI-based AHCI controller.
>  
> +config SPL_AHCI_PCI
> +	bool "Support for PCI-based AHCI controller for SPL"
> +	depends on SPL
> +	depends on SPL_PCI
> +	depends on SPL_SATA_SUPPORT && DM_SCSI
> +
>  config SATA_CEVA
>  	bool "Ceva Sata controller"
>  	depends on AHCI
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 4811b2f82c4e..cd88131dcd13 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_)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

Looks right to me, please submit it as a proper patch, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Pali Rohár]

On Sunday 15 August 2021 10:23:03 Tom Rini wrote:
> On Sun, Aug 15, 2021 at 01:48:26PM +0200, Pali Rohár wrote:
> > On Monday 02 August 2021 21:40:07 Tom Rini wrote:
> > > OK, thanks.  Then the right fix is to introduce whatever SPL_xxx symbols
> > > you need so that you can turn off what you don't want in SPL but leave
> > > it enabled in full U-Boot, and update the code to use
> > > CONFIG_IS_ENABLED(xxx).  If you can do that part, I can iterate over
> > > converting everyone else that assumes what we have today is what it
> > > wants in SPL and non-SPL.
> > 
> > Hello Tom! Below is patch which adds SPL_AHCI_PCI symbol. I'm not sure
> > if this is correct approach but it looks like that it fixed that issue.
> > With this patch I can compile U-Boot with and also without enabling of
> > CONFIG_SPL_SATA_SUPPORT option.
> > 
> > 
> > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> > index 0c1490a9f900..96c7c30375bd 100644
> > --- a/drivers/ata/Kconfig
> > +++ b/drivers/ata/Kconfig
> > @@ -41,6 +41,12 @@ config AHCI_PCI
> >  	help
> >  	  Enables support for the PCI-based AHCI controller.
> >  
> > +config SPL_AHCI_PCI
> > +	bool "Support for PCI-based AHCI controller for SPL"
> > +	depends on SPL
> > +	depends on SPL_PCI
> > +	depends on SPL_SATA_SUPPORT && DM_SCSI
> > +
> >  config SATA_CEVA
> >  	bool "Ceva Sata controller"
> >  	depends on AHCI
> > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> > index 4811b2f82c4e..cd88131dcd13 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_)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
> 
> Looks right to me, please submit it as a proper patch, thanks!

Patches are already waiting here:
https://patchwork.ozlabs.org/project/uboot/list/?series=258094

Re: Broken CONFIG_SPL_SATA_SUPPORT=y

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

On Fri, Aug 27, 2021 at 05:17:25PM +0200, Pali Rohár wrote:
> On Sunday 15 August 2021 10:23:03 Tom Rini wrote:
> > On Sun, Aug 15, 2021 at 01:48:26PM +0200, Pali Rohár wrote:
> > > On Monday 02 August 2021 21:40:07 Tom Rini wrote:
> > > > OK, thanks.  Then the right fix is to introduce whatever SPL_xxx symbols
> > > > you need so that you can turn off what you don't want in SPL but leave
> > > > it enabled in full U-Boot, and update the code to use
> > > > CONFIG_IS_ENABLED(xxx).  If you can do that part, I can iterate over
> > > > converting everyone else that assumes what we have today is what it
> > > > wants in SPL and non-SPL.
> > > 
> > > Hello Tom! Below is patch which adds SPL_AHCI_PCI symbol. I'm not sure
> > > if this is correct approach but it looks like that it fixed that issue.
> > > With this patch I can compile U-Boot with and also without enabling of
> > > CONFIG_SPL_SATA_SUPPORT option.
> > > 
> > > 
> > > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> > > index 0c1490a9f900..96c7c30375bd 100644
> > > --- a/drivers/ata/Kconfig
> > > +++ b/drivers/ata/Kconfig
> > > @@ -41,6 +41,12 @@ config AHCI_PCI
> > >  	help
> > >  	  Enables support for the PCI-based AHCI controller.
> > >  
> > > +config SPL_AHCI_PCI
> > > +	bool "Support for PCI-based AHCI controller for SPL"
> > > +	depends on SPL
> > > +	depends on SPL_PCI
> > > +	depends on SPL_SATA_SUPPORT && DM_SCSI
> > > +
> > >  config SATA_CEVA
> > >  	bool "Ceva Sata controller"
> > >  	depends on AHCI
> > > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> > > index 4811b2f82c4e..cd88131dcd13 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_)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
> > 
> > Looks right to me, please submit it as a proper patch, thanks!
> 
> Patches are already waiting here:
> https://patchwork.ozlabs.org/project/uboot/list/?series=258094

Yes, thanks, I'll pick them up for the merge window.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]