Broken CONFIG_SPL_SATA_SUPPORT=y
Pali Rohár
pali at kernel.org
Tue Aug 3 01:33:53 CEST 2021
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 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).
>
> 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.
More information about the U-Boot
mailing list