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

[Jagan Teki]

Hi Mugunthan,

On 23 November 2015 at 18:42, Jagan Teki <jteki at openedev.com> wrote:
> On 23 November 2015 at 16:59, Mugunthan V N <mugunthanvnm at 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 at 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 at ti.com> wrote:
>>>>>> Add compatible for spansion 32MiB spi flash s25fl256s1.
>>>>>>
>>>>>> Signed-off-by: Mugunthan V N <mugunthanvnm at 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?

>
>>
>> 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.

thanks!
-- 
Jagan | openedev.