From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hannes Schmelzer Date: Wed, 24 Aug 2016 16:10:43 +0200 Subject: [U-Boot] [PATCH v1] cmd/sf: probe flash with speed of last known flash or speed from devicetree In-Reply-To: <57BD9132.7030408@schmelzer.or.at> References: <1472033154-31475-1-git-send-email-oe5hpm@oevsv.at> <86295f5e-70ad-e660-4ff4-2046e12a6780@ti.com> <57BD7C19.4030401@schmelzer.or.at> <196cb9a1-b5d9-f1bb-1d5a-ac2dfd2fe3a0@ti.com> <57BD9132.7030408@schmelzer.or.at> Message-ID: <57BDAAE3.8070607@schmelzer.or.at> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 08/24/2016 02:21 PM, Hannes Schmelzer wrote: > On 08/24/2016 01:12 PM, Vignesh R wrote: >> >> On Wednesday 24 August 2016 04:21 PM, Hannes Schmelzer wrote: >>> On 08/24/2016 12:35 PM, Vignesh R wrote: >>>> Hi, >>> Hi Vignesh, >>>> On Wednesday 24 August 2016 03:35 PM, Hannes Schmelzer wrote: >>>>> During probing flashes on the spi bus using the "sf probe" command, a >>>>> maybe existing flash (from fdt) is unbound and removed to force the >>>>> 'spi_flash_probe_bus_cs' really scanning the bus. >>>>> >>>>> Today the bus is probed with speed 0, this triggers several fall-back >>>>> mechanism (mostly in the low-level drivers) to catch the >>>>> impossible zero >>>>> speed. >>>>> Result of this is, that the spi-flash runs at very low speed >>>>> depending >>>>> on the minimum given by low-level driver/hardware. >>>>> >>>>> Values like 'spi-max-frequency' from devicetree are ignored totally >>>>> today. >>>>> >>>>> This commit changes as following: >>>>> - if there was already some flash binding in devicetree (having some >>>>> spi-max-frequency within) speed is taken from it >>>>> - if no flash binding was present for speed the >>>>> 'spi-max-frequency' from >>>>> the responsible spi node is taken. >>>>> >>>>> Signed-off-by: Hannes Schmelzer >>>> With commit 96907c0fe50a8 ("dm: spi: Read default speed and mode >>>> values >>>> from DT") sf probe picks spi-max-frequency from DT if not specified as >>>> argument. >>>> >>>> But when sf probe is called second time, the command fails to pick up >>>> speed from DT. This is because flash node is unbound from the SPI >>>> controller children nodes. Below patch should fix this issue: >>>> https://patchwork.ozlabs.org/patch/659979/ >>> sry ... i didn't took notice about this patch at the beginning of mine. >>> Just reviewed and tested it. >>> >>> The named patch makes things a bit better but not good. >>> Speed for flash still has no relationship to 'spi-max-frequency' from >>> the spi-bus nor with a maybe defined flash in dts. >>> >>> Rather the #define CONFIG_ENV_SPI_MAX_HZ is used. >>> >> sf probe calls do_spi_flash_probe() >> -spi_flash_probe_bus_cs() >> -spi_get_bus_and_cs() >> >> In spi_get_bus_and_cs(): >> if (!speed) { >> speed = plat->max_hz; >> mode = plat->mode; >> } >> This should set speed to spi-max-frequency as per flash DT node. > yes that should do. > But have look a few lines up, around 296, there the newly created > flash node has been modified with wrong values before. > No glue why a new flash node is being created. >> AFAIU, saveenv() uses CONFIG_ENV_SPI_MAX_HZ only when >> CONFIG_DM_SPI_FLASH is not defined. Could please explain how >> CONFIG_ENV_SPI_MAX_HZ takes precedence over spi-max-frequency during sf >> probe? > Thanks the discussion, I think we coming closer to the problem. > Your'e right saveenv() behaves as you described, but not so > env_relocate_spec(). > > There the flash is probed like this: > env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, > CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); > > and thats the point where some existing dt node becomes modified with > those values. Thats probably wrong doing so. > > I think there should be same logic applied as in saveen(). Tested again, and confirming this behaviour. > > Just for testing i fixed the probe there. > Now i'm a bit confused about that the function > spi_find_chip_select(bus, cs, &dev) in spi-uclass doesn't find child > devices on the spi node. > Instead there a new flash binding is created. > spi_get_bus_and_cs: Binding new device 'spi_flash at 0:0', busnum=0, > cs=0, driver=spi_flash_std Debugged into DM and found out that my devicetree didn't represent a valid flash-child. I've now modified it to: => fdt print spi0 spi at e000d000 { clock-names = "ref_clk", "pclk"; clocks = <0x00000001 0x0000000a 0x00000001 0x0000002b>; compatible = "xlnx,zynq-qspi-1.0"; status = "okay"; interrupt-parent = <0x00000003>; interrupts = <0x00000000 0x00000013 0x00000004>; reg = <0xe000d000 0x00001000>; #address-cells = <0x00000001>; #size-cells = <0x00000000>; u-boot,dm-pre-reloc; spi-max-frequency = <0x05f5e100>; spiflash at 0 { u-boot,dm-pre-reloc; compatible = "spidev", "spi-flash"; spi-max-frequency = <0x05f5e100>; reg = <0x00000000>; }; }; => So that works and u-boot knows the flash from beginning and uses as correct flash-node from fdt instead creating a new one. Finally i think, we can drop my patch and somebody (maybe me?) should fix the probing during environment relocation. cheers, Hannes