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 <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).
>
> 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
signature.asc
Description: PGP signature
