[U-Boot] [PATCH v4 1/1] board: arm: Add support for Broadcom BCM7445

[Bin Meng <bmeng.cn@gmail.com>]

+Simon

On Thu, Aug 29, 2019 at 1:24 AM Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote:
>
> Hi Bin,
>
> Bin Meng <bmeng.cn@gmail.com> writes:
>
> > Hi Thomas,
> >
> > On Wed, Aug 28, 2019 at 6:31 AM Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote:
> >>
> >> Hi Bin,
> >>
> >> Bin Meng <bmeng.cn@gmail.com> writes:
> >>
> >> > Hi Thomas,
> >> >
> >> > On Sat, Jun 9, 2018 at 6:06 AM Thomas Fitzsimmons <fitzsim@fitzsim.org> wrote:
> >> >>
> >> >> Add support for loading U-Boot on the Broadcom 7445 SoC.  This port
> >> >> assumes Broadcom's BOLT bootloader is acting as the second stage
> >> >> bootloader, and U-Boot is acting as the third stage bootloader, loaded
> >> >> as an ELF program by BOLT.
> >> >>
> >> >> Signed-off-by: Thomas Fitzsimmons <fitzsim@fitzsim.org>
> >> >> Cc: Stefan Roese <sr@denx.de>
> >> >> Cc: Tom Rini <trini@konsulko.com>
> >> >> Cc: Florian Fainelli <f.fainelli@gmail.com>
> >> >> ---
> >> >> Changes for v4:
> >> >>    - Use high timer register for get_ticks
> >> >>    - Move hard-coded register addresses from Kconfig to header
> >> >>    - Document I-cache/D-cache expectation
> >> >>
> >> >>  MAINTAINERS                                     |  10 +
> >> >>  arch/arm/Kconfig                                |  12 +
> >> >>  arch/arm/Makefile                               |   1 +
> >> >>  arch/arm/mach-bcmstb/Kconfig                    |  36 ++
> >> >>  arch/arm/mach-bcmstb/Makefile                   |   8 +
> >> >>  arch/arm/mach-bcmstb/include/mach/gpio.h        |  11 +
> >> >>  arch/arm/mach-bcmstb/include/mach/hardware.h    |  11 +
> >> >>  arch/arm/mach-bcmstb/include/mach/prior_stage.h |  30 ++
> >> >>  arch/arm/mach-bcmstb/include/mach/sdhci.h       |  15 +
> >> >>  arch/arm/mach-bcmstb/include/mach/timer.h       |  13 +
> >> >>  arch/arm/mach-bcmstb/lowlevel_init.S            |  21 ++
> >> >>  board/broadcom/bcmstb/MAINTAINERS               |   7 +
> >> >>  board/broadcom/bcmstb/Makefile                  |   8 +
> >> >>  board/broadcom/bcmstb/bcmstb.c                  | 194 +++++++++++
> >> >>  configs/bcm7445_defconfig                       |  27 ++
> >> >>  doc/README.bcm7xxx                              | 150 ++++++++
> >> >>  drivers/mmc/Kconfig                             |  11 +
> >> >>  drivers/mmc/Makefile                            |   1 +
> >> >>  drivers/mmc/bcmstb_sdhci.c                      |  67 ++++
> >> >>  drivers/spi/Kconfig                             |   7 +
> >> >>  drivers/spi/Makefile                            |   1 +
> >> >>  drivers/spi/bcmstb_spi.c                        | 439 ++++++++++++++++++++++++
> >> >>  drivers/spi/spi-uclass.c                        |   2 +-
> >> >>  dts/Kconfig                                     |   7 +
> >> >>  include/configs/bcm7445.h                       |  26 ++
> >> >>  include/configs/bcmstb.h                        | 183 ++++++++++
> >> >>  include/fdtdec.h                                |   4 +
> >> >>  lib/fdtdec.c                                    |   4 +
> >> >>  28 files changed, 1305 insertions(+), 1 deletion(-)
> >> >>  create mode 100644 arch/arm/mach-bcmstb/Kconfig
> >> >>  create mode 100644 arch/arm/mach-bcmstb/Makefile
> >> >>  create mode 100644 arch/arm/mach-bcmstb/include/mach/gpio.h
> >> >>  create mode 100644 arch/arm/mach-bcmstb/include/mach/hardware.h
> >> >>  create mode 100644 arch/arm/mach-bcmstb/include/mach/prior_stage.h
> >> >>  create mode 100644 arch/arm/mach-bcmstb/include/mach/sdhci.h
> >> >>  create mode 100644 arch/arm/mach-bcmstb/include/mach/timer.h
> >> >>  create mode 100644 arch/arm/mach-bcmstb/lowlevel_init.S
> >> >>  create mode 100644 board/broadcom/bcmstb/MAINTAINERS
> >> >>  create mode 100644 board/broadcom/bcmstb/Makefile
> >> >>  create mode 100644 board/broadcom/bcmstb/bcmstb.c
> >> >>  create mode 100644 configs/bcm7445_defconfig
> >> >>  create mode 100644 doc/README.bcm7xxx
> >> >>  create mode 100644 drivers/mmc/bcmstb_sdhci.c
> >> >>  create mode 100644 drivers/spi/bcmstb_spi.c
> >> >>  create mode 100644 include/configs/bcm7445.h
> >> >>  create mode 100644 include/configs/bcmstb.h
> >> >>
> >> >
> >> > [snip]
> >> >
> >> >> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> >> >> index d2d091f..c517d06 100644
> >> >> --- a/drivers/spi/spi-uclass.c
> >> >> +++ b/drivers/spi/spi-uclass.c
> >> >> @@ -273,7 +273,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
> >> >>         bool created = false;
> >> >>         int ret;
> >> >>
> >> >> -#if CONFIG_IS_ENABLED(OF_PLATDATA)
> >> >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) || CONFIG_IS_ENABLED(OF_PRIOR_STAGE)
> >> >
> >> > Could you please explain this change a little bit? Why should we call
> >> > uclass_first_device_err() for OF_PRIOR_STAGE?
> >> >
> >> >>         ret = uclass_first_device_err(UCLASS_SPI, &bus);
> >> >>  #else
> >> >>         ret = uclass_get_device_by_seq(UCLASS_SPI, busnum, &bus);
> >>
> >> On the BCM7445 eval board, the prior-stage-provided device tree does not
> >> have an alias for spi0, though it has aliases for other device types; I
> >> don't know why this is the case, but I don't have control over what the
> >> prior stage bootloader (Broadcom's BOLT) provides.  Without that alias,
> >> uclass_get_device_by_seq fails to find the SPI bus (and so U-Boot can't
> >> find the SPI flash device that stores its environment).
> >>
> >
> > I checked uclass_get_device_by_seq() and did not find any codes that
> > are trying to read aliases. Am I missing anything?
>
> Alias handling is not in the direct uclass_get_device_by_seq call chain,
> and it took some debugging to find this.  The requested sequence number
> is handled earlier in the boot, in device_bind_common:
>
>    dev->seq = -1;
>    dev->req_seq = -1;
>    if (CONFIG_IS_ENABLED(DM_SEQ_ALIAS) &&
>        (uc->uc_drv->flags & DM_UC_FLAG_SEQ_ALIAS)) {
>            /*
>             * Some devices, such as a SPI bus, I2C bus and serial ports
>             * are numbered using aliases.
>             *
>             * This is just a 'requested' sequence, and will be
>             * resolved (and ->seq updated) when the device is probed.
>             */
>            if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
>                    if (uc->uc_drv->name && ofnode_valid(node))
>                            dev_read_alias_seq(dev, &dev->req_seq);
>            } else {
>                    dev->req_seq = uclass_find_next_free_req_seq(drv->id);
>            }
>    }
>
> If the prior stage bootloader provides no SPI bus alias, then
> dev_read_alias_seq fails, dev->req_seq stays unset (-1), and later when
> an attempt is made to access the SPI bus, the call to
> uclass_get_device_by_seq fails.

Ah, I see. Thanks!

>
> >> At the time, I checked other ARM device trees in the Linux kernel and
> >> not many set an alias for spi0, so I wrote the patch to choose the first
> >> SPI bus.  Doing so was in line with what CONFIG_OF_PLATDATA did on
> >> boards that wanted to avoid device tree accesses.
> >>
> >
> > Based on what you said, the adding of OF_PRIOR_STAGE here sounds like a hack.
>
> Yeah, it makes an assumption about which SPI bus to use, that may or may
> not be valid on other boards.
>
> >> I see that since I introduced CONFIG_OF_PRIOR_STAGE, several other ports
> >> have started using it, so there's probably a need to generalize this; if
> >> other prior stage bootloaders provide SPI aliases then there should be a
> >> way for this code to use them.
> >>
> >
> > I think the correct way is to call uclass_get_device_by_seq().
> > CONFIG_OF_PRIOR_STAGE should not imply such behavior.
>
> OK, but without other changes this would break boards that rely on the
> above assumption (for example BCM7445).
>
> I'm experimenting with patches that work on BCM7445 and eliminate the
> SPI bus assumption but I don't know what effect they might have on other
> ports that use OF_PRIOR_STAGE.
>
> In general it seems like a good idea to use the prior-stage-provided
> aliases, but the question is what to do when an alias is missing; maybe
> always fall back to calling uclass_find_next_free_req_seq?

I tend to agree. As I see lots of places in U-Boot sources, like below:

drivers/i2c/intel_i2c.c::intel_i2c_bind()

if (device_is_on_pci_bus(dev)) {
               /*
                * ToDo:
                * Setting req_seq in the driver is probably not recommended.
                * But without a DT alias the number is not configured. And
                * using this driver is impossible for PCIe I2C devices.
                * This can be removed, once a better (correct) way for this
                * is found and implemented.
                */
               dev->req_seq = num_cards;

And another example:
drivers/usb/host/ehci-vf.c::vf_usb_bind()

       /*
        * Without this hack, if we return ENODEV for USB Controller 0, on
        * probe for the next controller, USB Controller 1 will be given a
        * sequence number of 0. This conflicts with our requirement of
        * sequence numbers while initialising the peripherals.
        */
       dev->req_seq = num_controllers;

Simon, do you think we should change the behavior of dev->req_seq in
the device bind for uclass drivers with DM_UC_FLAG_SEQ_ALIAS flag?

>
> Have you found a board for which uclass_get_device_by_seq works for SPI
> and uclass_first_device_err does the wrong thing?  Is it a publicly
> available port that I can have a look at?

No, I was not indicating an error like uclass_get_device_by_seq works
for SPI and uclass_first_device_err does the wrong thing. The issue
with your implementation is that it forces probing the first SPI
controller and ignore the "busnum" completely. This makes a board with
multiple SPI controllers unable to probe other controllers than the
very first one.

Regards,
Bin