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