From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bin Meng Date: Thu, 29 Aug 2019 23:24:22 +0800 Subject: [U-Boot] [PATCH v4 1/1] board: arm: Add support for Broadcom BCM7445 In-Reply-To: References: <16fc0901f4521d3c399eac950c52a634b2f9473b.1528485916.git.fitzsim@fitzsim.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de +Simon On Thu, Aug 29, 2019 at 1:24 AM Thomas Fitzsimmons wrote: > > Hi Bin, > > Bin Meng writes: > > > Hi Thomas, > > > > On Wed, Aug 28, 2019 at 6:31 AM Thomas Fitzsimmons wrote: > >> > >> Hi Bin, > >> > >> Bin Meng writes: > >> > >> > Hi Thomas, > >> > > >> > On Sat, Jun 9, 2018 at 6:06 AM Thomas Fitzsimmons 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 > >> >> Cc: Stefan Roese > >> >> Cc: Tom Rini > >> >> Cc: Florian Fainelli > >> >> --- > >> >> 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