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