[U-Boot] [PATCH v4 1/1] board: arm: Add support for Broadcom BCM7445
Simon Glass
sjg at chromium.org
Tue Sep 17 05:48:03 UTC 2019
Hi Bin,
On Thu, 5 Sep 2019 at 06:10, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Thu, Aug 29, 2019 at 11:24 PM Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > +Simon
> >
> > On Thu, Aug 29, 2019 at 1:24 AM Thomas Fitzsimmons <fitzsim at fitzsim.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > Bin Meng <bmeng.cn at gmail.com> writes:
> > >
> > > > Hi Thomas,
> > > >
> > > > On Wed, Aug 28, 2019 at 6:31 AM Thomas Fitzsimmons <fitzsim at fitzsim.org> wrote:
> > > >>
> > > >> Hi Bin,
> > > >>
> > > >> Bin Meng <bmeng.cn at gmail.com> writes:
> > > >>
> > > >> > Hi Thomas,
> > > >> >
> > > >> > On Sat, Jun 9, 2018 at 6:06 AM Thomas Fitzsimmons <fitzsim at 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 at fitzsim.org>
> > > >> >> Cc: Stefan Roese <sr at denx.de>
> > > >> >> Cc: Tom Rini <trini at konsulko.com>
> > > >> >> Cc: Florian Fainelli <f.fainelli at 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?
> >
>
> Would you please give some comments regarding this?
Yes I think we should do something here.
I am not quite clear on the best action though.
At present ->seq is set when the device is proved and dropped when it
is removed. This is good in the sense that different devices can 'take
over' a sequence number. But in practice I don't think we have found
that to be very useful.
An alternative would be to do this processing when the device is
bound. We might then remove the two separate values and just have one.
However it would mean processing aliases which could potentially be
slow.
In general I had hoped that sequence numbers would become less
important in U-Boot, as they have in Linux. But for now we need to
deal with it.
>
> > >
> > > 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
Regards,
Simon
More information about the U-Boot
mailing list