* Broken CONFIG_SPL_SATA_SUPPORT=y
@ 2021-08-01 12:25 Pali Rohár
2021-08-02 19:21 ` Simon Glass
2021-08-02 20:55 ` Tom Rini
0 siblings, 2 replies; 24+ messages in thread
From: Pali Rohár @ 2021-08-01 12:25 UTC (permalink / raw)
To: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich,
Marek Behún, u-boot
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.
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-01 12:25 Broken CONFIG_SPL_SATA_SUPPORT=y Pali Rohár @ 2021-08-02 19:21 ` Simon Glass 2021-08-02 20:16 ` Pali Rohár 2021-08-02 20:55 ` Tom Rini 1 sibling, 1 reply; 24+ messages in thread From: Simon Glass @ 2021-08-02 19:21 UTC (permalink / raw) To: Pali Rohár Cc: Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 19:21 ` Simon Glass @ 2021-08-02 20:16 ` Pali Rohár 0 siblings, 0 replies; 24+ messages in thread From: Pali Rohár @ 2021-08-02 20:16 UTC (permalink / raw) To: Simon Glass Cc: Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-01 12:25 Broken CONFIG_SPL_SATA_SUPPORT=y Pali Rohár 2021-08-02 19:21 ` Simon Glass @ 2021-08-02 20:55 ` Tom Rini 2021-08-02 21:09 ` Pali Rohár 1 sibling, 1 reply; 24+ messages in thread From: Tom Rini @ 2021-08-02 20:55 UTC (permalink / raw) To: Pali Rohár Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, u-boot [-- 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 --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 20:55 ` Tom Rini @ 2021-08-02 21:09 ` Pali Rohár 2021-08-02 21:14 ` Simon Glass 0 siblings, 1 reply; 24+ messages in thread From: Pali Rohár @ 2021-08-02 21:09 UTC (permalink / raw) To: Tom Rini Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, u-boot 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 21:09 ` Pali Rohár @ 2021-08-02 21:14 ` Simon Glass 2021-08-02 21:18 ` Pali Rohár 0 siblings, 1 reply; 24+ messages in thread From: Simon Glass @ 2021-08-02 21:14 UTC (permalink / raw) To: Pali Rohár Cc: Tom Rini, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 21:14 ` Simon Glass @ 2021-08-02 21:18 ` Pali Rohár 2021-08-02 21:23 ` Simon Glass 0 siblings, 1 reply; 24+ messages in thread From: Pali Rohár @ 2021-08-02 21:18 UTC (permalink / raw) To: Simon Glass Cc: Tom Rini, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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... ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 21:18 ` Pali Rohár @ 2021-08-02 21:23 ` Simon Glass 2021-08-02 21:25 ` Pali Rohár 0 siblings, 1 reply; 24+ messages in thread From: Simon Glass @ 2021-08-02 21:23 UTC (permalink / raw) To: Pali Rohár Cc: Tom Rini, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List () 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 21:23 ` Simon Glass @ 2021-08-02 21:25 ` Pali Rohár 2021-08-02 21:30 ` Simon Glass 2021-08-02 21:37 ` Tom Rini 0 siblings, 2 replies; 24+ messages in thread From: Pali Rohár @ 2021-08-02 21:25 UTC (permalink / raw) To: Simon Glass Cc: Tom Rini, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 21:25 ` Pali Rohár @ 2021-08-02 21:30 ` Simon Glass 2021-08-02 21:41 ` Pali Rohár 2021-08-02 21:43 ` Tom Rini 2021-08-02 21:37 ` Tom Rini 1 sibling, 2 replies; 24+ messages in thread From: Simon Glass @ 2021-08-02 21:30 UTC (permalink / raw) To: Pali Rohár Cc: Tom Rini, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 21:30 ` Simon Glass @ 2021-08-02 21:41 ` Pali Rohár 2021-08-02 21:59 ` Tom Rini 2021-08-02 21:43 ` Tom Rini 1 sibling, 1 reply; 24+ messages in thread From: Pali Rohár @ 2021-08-02 21:41 UTC (permalink / raw) To: Simon Glass Cc: Tom Rini, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 21:41 ` Pali Rohár @ 2021-08-02 21:59 ` Tom Rini 0 siblings, 0 replies; 24+ messages in thread From: Tom Rini @ 2021-08-02 21:59 UTC (permalink / raw) To: Pali Rohár Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 21:30 ` Simon Glass 2021-08-02 21:41 ` Pali Rohár @ 2021-08-02 21:43 ` Tom Rini 2021-08-02 21:56 ` Pali Rohár 1 sibling, 1 reply; 24+ messages in thread From: Tom Rini @ 2021-08-02 21:43 UTC (permalink / raw) To: Simon Glass Cc: Pali Rohár, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 21:43 ` Tom Rini @ 2021-08-02 21:56 ` Pali Rohár 2021-08-02 22:11 ` Tom Rini 0 siblings, 1 reply; 24+ messages in thread From: Pali Rohár @ 2021-08-02 21:56 UTC (permalink / raw) To: Tom Rini Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 21:56 ` Pali Rohár @ 2021-08-02 22:11 ` Tom Rini 2021-08-02 22:38 ` Pali Rohár 0 siblings, 1 reply; 24+ messages in thread From: Tom Rini @ 2021-08-02 22:11 UTC (permalink / raw) To: Pali Rohár Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 22:11 ` Tom Rini @ 2021-08-02 22:38 ` Pali Rohár 2021-08-02 22:46 ` Tom Rini 0 siblings, 1 reply; 24+ messages in thread From: Pali Rohár @ 2021-08-02 22:38 UTC (permalink / raw) To: Tom Rini Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 22:38 ` Pali Rohár @ 2021-08-02 22:46 ` Tom Rini 2021-08-02 23:33 ` Pali Rohár 0 siblings, 1 reply; 24+ messages in thread From: Tom Rini @ 2021-08-02 22:46 UTC (permalink / raw) To: Pali Rohár Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 22:46 ` Tom Rini @ 2021-08-02 23:33 ` Pali Rohár 2021-08-03 1:40 ` Tom Rini 0 siblings, 1 reply; 24+ messages in thread From: Pali Rohár @ 2021-08-02 23:33 UTC (permalink / raw) To: Tom Rini Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 23:33 ` Pali Rohár @ 2021-08-03 1:40 ` Tom Rini 2021-08-15 11:48 ` Pali Rohár 0 siblings, 1 reply; 24+ messages in thread From: Tom Rini @ 2021-08-03 1:40 UTC (permalink / raw) To: Pali Rohár Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-03 1:40 ` Tom Rini @ 2021-08-15 11:48 ` Pali Rohár 2021-08-15 14:23 ` Tom Rini 0 siblings, 1 reply; 24+ messages in thread From: Pali Rohár @ 2021-08-15 11:48 UTC (permalink / raw) To: Tom Rini Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-15 11:48 ` Pali Rohár @ 2021-08-15 14:23 ` Tom Rini 2021-08-27 15:17 ` Pali Rohár 0 siblings, 1 reply; 24+ messages in thread From: Tom Rini @ 2021-08-15 14:23 UTC (permalink / raw) To: Pali Rohár Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-15 14:23 ` Tom Rini @ 2021-08-27 15:17 ` Pali Rohár 2021-08-27 15:22 ` Tom Rini 0 siblings, 1 reply; 24+ messages in thread From: Pali Rohár @ 2021-08-27 15:17 UTC (permalink / raw) To: Tom Rini Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-27 15:17 ` Pali Rohár @ 2021-08-27 15:22 ` Tom Rini 0 siblings, 0 replies; 24+ messages in thread From: Tom Rini @ 2021-08-27 15:22 UTC (permalink / raw) To: Pali Rohár Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Broken CONFIG_SPL_SATA_SUPPORT=y 2021-08-02 21:25 ` Pali Rohár 2021-08-02 21:30 ` Simon Glass @ 2021-08-02 21:37 ` Tom Rini 1 sibling, 0 replies; 24+ messages in thread From: Tom Rini @ 2021-08-02 21:37 UTC (permalink / raw) To: Pali Rohár Cc: Simon Glass, Stefan Roese, Masami Hiramatsu, Frank Wunderlich, Marek Behún, U-Boot Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-08-27 15:22 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-01 12:25 Broken CONFIG_SPL_SATA_SUPPORT=y Pali Rohár 2021-08-02 19:21 ` Simon Glass 2021-08-02 20:16 ` Pali Rohár 2021-08-02 20:55 ` Tom Rini 2021-08-02 21:09 ` Pali Rohár 2021-08-02 21:14 ` Simon Glass 2021-08-02 21:18 ` Pali Rohár 2021-08-02 21:23 ` Simon Glass 2021-08-02 21:25 ` Pali Rohár 2021-08-02 21:30 ` Simon Glass 2021-08-02 21:41 ` Pali Rohár 2021-08-02 21:59 ` Tom Rini 2021-08-02 21:43 ` Tom Rini 2021-08-02 21:56 ` Pali Rohár 2021-08-02 22:11 ` Tom Rini 2021-08-02 22:38 ` Pali Rohár 2021-08-02 22:46 ` Tom Rini 2021-08-02 23:33 ` Pali Rohár 2021-08-03 1:40 ` Tom Rini 2021-08-15 11:48 ` Pali Rohár 2021-08-15 14:23 ` Tom Rini 2021-08-27 15:17 ` Pali Rohár 2021-08-27 15:22 ` Tom Rini 2021-08-02 21:37 ` Tom Rini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.