[U-Boot] [PATCH v1] cmd/sf: probe flash with speed of last known flash or speed from devicetree

[Hannes Schmelzer]

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 <oe5hpm at oevsv.at>
>>>> 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