[U-Boot] [PATCH v3] dm: spi: Read default speed and mode values from DT

[Vignesh R]

Hi Qianyu,

[...]

>>>> @@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int
>>> speed, int
>>>
>>>> mode,
>>>
>>>>                            slave->dev = dev;
>>>
>>>>             }
>>>
>>>>
>>>
>>>> +          plat = dev_get_parent_platdata(dev);
>>>
>>>> +          if (!speed) {
>>>
>>>> +                         speed = plat->max_hz;
>>>
>>>> +                         mode = plat->mode;
>>>
>>>> +          }
>>>
>>>>             ret = spi_set_speed_mode(bus, speed, mode);
>>>
>>>>             if (ret)
>>>
>>>>                            goto err;
>>>
>>>> --
>>>
>>>
>>>
>>> I just doubt if spi_set_speed_mode() has really made a difference to
>>>
>>> the actual transfer.
>>>
>>
>> It does (see below)...
>>
>>> Seems that if the device is inactive, calling device_probe() would
>>> also call
>>>
>>> spi_set_speed_mode() and do the data transfer. Even if it's active,
>>> setting
>>>
>>> speed and mode for it again would not be necessary.
>>
>>
>> Yes, spi_set_speed_mode() is called from
>> spi_flash_probe_slave()->spi_claim_bus() as part of device_probe().
>> spi_claim_bus() in spi-uclass.c speed & mode are appropriately passed based on DT
>> data to spi_set_speed_mode(). But that's not the issue.
>>
>> But in spi_get_bus_and_cs() (called from sf probe) there is a call to
>> spi_set_speed_mode() after device_probe() for inactive devices. This call is to
> 
> Yes. Actually I'm thinking that the second spi_set_speed_mode()(called from
> spi_get_bus_and_cs()) could just be removed from the end of the function.
> 

If second call to spi_set_speed_mode() is removed then how would you
override default speed/mode specified via DT with that of cmd line args
passed to sf probe. (else we need to change the definition of sf probe
to not accept speed/mode in case of DT)

>> _override_ the speed set via DT with those passed as cmd line args of sf probe. But,
>> if no args are passed to sf probe, speed and mode default to
>> CONFIG_SF_DEFAULT_SPEED/MODE (see do_spi_flash_probe() in
>> cmd/sf.c) instead of using DT inputs.
>>
> 
> Yes. But notice that the speed and mode will only be replaced by 
> CONFIG_SF_DEFAULT_SPEED/MODE at the end of the calling. Every time 'sf probe' is
> called, the device will be removed(if active) and thus it'll always call device_probe()
> to set the speed&mode from DT. Then the driver will use them in the actual transfer.

True, I see the first call and speed/mode is set to DT values
accordingly. But, that's not the final spi_set_speed_mode() call to the
SPI driver.

> After the transfer is finished, the speed and mode are replaced by
> CONFIG_SF_DEFAULT_SPEED/MODE(or anything else) again but they wouldn't be used
> for the transfer at all.
> 

Sorry... I don't understand. What do you mean by transfer? sf probe does
not do any data transfer other than reading jedec id.
The values set by the SPI driver during device_probe() will be
overwritten by the last spi_set_speed_mode() call in
spi_get_bus_and_cs() which happens to pass CONFIG_SF_DEFAULT_SPEED/MODE.

Here is the call sequence:

sf probe()
---> device_probe()
  ---> spi_set_speed_mode(values from DT)
    ---> driver's pi_set_speed_mode() /* set to DT values */
---> spi_set_speed_mode(overridden values)
  ---> driver's spi_set_speed_mode() /* set to newer(not DT) values */

Now if sf read is called after sf probe. One would see
CONFIG_SF_DEFAULT_SPEED/MODE values in driver settings and data
transfers happen at DEFAULT_SPEED.


> And there may be another related issue in this case that I have reported to Simon.
> If you would like to test on your board, please remove the device_unbind() in
> do_spi_flash_probe(). The current SPI driver model will discard users' spi slave in DT from
> the second 'sf probe' and create a new one using CONFIG_SF_DEFAULT_SPEED/MODE. 
>  

Yes, I am aware of the problem with second sf probe discarding values
from DT. IIRC, Simon not just wanted to remove device_unbind() but also
do some more refactoring of messages in uclass drivers. Maybe Simon is
working on the fix.

-- 
Regards
Vignesh