[U-Boot] [PATCH v3 10/16] drivers: mtd: spi: sf_probe: add compatible for spansion spi flash

[Mugunthan V N <mugunthanvnm@ti.com>]

On Thursday 03 December 2015 09:32 PM, Jagan Teki wrote:
> Hi Mugunthan,
> 
> On 23 November 2015 at 18:42, Jagan Teki <jteki@openedev.com> wrote:
>> On 23 November 2015 at 16:59, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>>> On Friday 20 November 2015 05:57 PM, Jagan Teki wrote:
>>>> On 20 November 2015 at 11:31, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>>>>> Jagan
>>>>>
>>>>> On Thursday 19 November 2015 03:40 PM, Jagan Teki wrote:
>>>>>> On 19 November 2015 at 12:35, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>>>>>>> Add compatible for spansion 32MiB spi flash s25fl256s1.
>>>>>>>
>>>>>>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
>>>>>>> ---
>>>>>>>  drivers/mtd/spi/sf_probe.c | 1 +
>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
>>>>>>> index bc05d30..ffbce59 100644
>>>>>>> --- a/drivers/mtd/spi/sf_probe.c
>>>>>>> +++ b/drivers/mtd/spi/sf_probe.c
>>>>>>> @@ -515,6 +515,7 @@ static const struct dm_spi_flash_ops spi_flash_std_ops = {
>>>>>>>
>>>>>>>  static const struct udevice_id spi_flash_std_ids[] = {
>>>>>>>         { .compatible = "spi-flash" },
>>>>>>> +       { .compatible = "s25fl256s1" },
>>>>>>>         { }
>>>>>>
>>>>>> Never compare with spi-flash vs s25fl256s1 here former is driver
>>>>>> compatible string and later one is probed flash string name. we don't
>>>>>> need to add them in compatible list, better to add it on dts node
>>>>>> otherwise no issue the connected flash will detect while 'sf probe'
>>>>>>
>>>>>
>>>>> I agree that spi flash will be probed even without addition of compatibles.
>>>>>
>>>>> If there is no matching compatible between the dtb and driver,  the dt
>>>>> properties like spi-max-frequency, spi clock phase/polarity and spi
>>>>> bus-width are not parsed from DT as the of_offset of the spi device will
>>>>> be '-1'. This check and dt parse is done in spi_child_post_bind() in
>>>>> spi-uclass driver.
>>>>>
>>>>> Since spi bus-width is not read from DT, by default spi transfers are
>>>>> done in one wire mode and read throughput drops to 5.3Mbps. With Quad
>>>>> mode read throughput is 16.6Mbps in DRA74x EVM.
>>>>>
>>>>
>>>> Why can't we try something like this [1] [2] I do agree with
>>>> documentation missing, may be we add add that well.
>>>>
>>>> [1] arch/arm/dts/socfpga_cyclone5_sockit.dts
>>>
>>> This dts file is not in sync with kernel dts. There is no qspi support
>>> in kernel. It is going to be really tough time to sync this dts between
>>> kernel and u-boot ;)
>>
>> Since we are in starting point of adding dts files (unlike Linux)
>> these dts files may not sync with Linux, we started adding these to
>> make dm or related drivers need to work. Yes I agreed with your point
>> and we really need to work on that as well.
>>
>>>
>>>> [2] arch/arm/dts/zynqmp-ep108.dts
>>>
>>> I don't see compatible string "spi-flash" in zynqmp-ep108.dts
>>
>> Yes, cant you add "s25fl256s1" like n25q512a11 added in this dts.
>>
>>>
>>> Adding spi-flash compatible will be tough as spi-flash works with the
>>> existing compatibles and convincing spi maintainer (Mark Brown) for
>>> adding a generic compatible will be difficult.
>>
>> This is what you pointed out - id_table.
>>
>> static struct spi_driver m25p80_driver = {
>>         .driver = {
>>                 .name   = "m25p80",
>>                 .of_match_table = m25p_of_table,
>>         },
>>         .id_table       = m25p_ids,
>>
> 
> Comments?

Yes, I was pointing to this compatible only.

> 
>>
>>>
>>> If you still insist to add spi-flash compatible to the dts file and *Tom
>>> Rini* is also fine, I will re-spin the series with compatible changes to
>>> the dts files.
>>
>> I'm saying like s25fl256s1 few of compatible strings were added
>> already on existing dts files, probably we may proceed with the same
>> for now. will trimming up later.
> 
> Any comments? proceed with above suggestion.

I agree, we can add compatibles, if needed we can work on trimming it later.

Regards
Mugunthan V N