Hi Simon,
On Thu, Aug 29, 2019 at 11:24 PM Bin Meng <bmeng...@gmail.com> wrote:
>
> +Simon
>
> On Thu, Aug 29, 2019 at 1:24 AM Thomas Fitzsimmons <fitz...@fitzsim.org>
> wrote:
> >
> > Hi Bin,
> >
> > Bin Meng <bmeng...@gmail.com> writes:
> >
> > > Hi Thomas,
> > >
> > > On Wed, Aug 28, 2019 at 6:31 AM Thomas Fitzsimmons <fitz...@fitzsim.org>
> > > wrote:
> > >>
> > >> Hi Bin,
> > >>
> > >> Bin Meng <bmeng...@gmail.com> writes:
> > >>
> > >> > Hi Thomas,
> > >> >
> > >> > On Sat, Jun 9, 2018 at 6:06 AM Thomas Fitzsimmons
> > >> > <fitz...@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 <fitz...@fitzsim.org>
> > >> >> Cc: Stefan Roese <s...@denx.de>
> > >> >> Cc: Tom Rini <tr...@konsulko.com>
> > >> >> Cc: Florian Fainelli <f.faine...@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?
> >
> > 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
