| Message ID | 1447916745-1233-11-git-send-email-mugunthanvnm@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
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'
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. Adding spi-splash to DT is not possible now as it is not documented in Kernel binding documentation. Even if "spi-flash" compatible is added to u-boot DT then it will show diff when syncing the dts files from Kernel to U-boot and it has to be taken care for every sync. Tom Rini also has agreed to this [1]. [1]: http://patchwork.ozlabs.org/patch/539780/ Regards Mugunthan V N
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 [2] arch/arm/dts/zynqmp-ep108.dts > Adding spi-splash to DT is not possible now as it is not documented in > Kernel binding documentation. Even if "spi-flash" compatible is added to > u-boot DT then it will show diff when syncing the dts files from Kernel > to U-boot and it has to be taken care for every sync. Tom Rini also has > agreed to this [1]. > > [1]: http://patchwork.ozlabs.org/patch/539780/ >
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 ;) > [2] arch/arm/dts/zynqmp-ep108.dts I don't see compatible string "spi-flash" in zynqmp-ep108.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. 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. Regards Mugunthan V N
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, > > 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. thanks!
On Mon, Nov 23, 2015 at 04:59:12PM +0530, Mugunthan V N 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 ;) True, but you're pushing that too, right? I suppose the high level take-away here is that when you're posting the bindings for Linux you need to keep some of the U-Boot folks in CC as well. So first up, where / how far along is the kernel support today?
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? > >> >> 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!
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
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" }, { } };
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(+)