Re: [PATCH] ARM: tegra: add "nor-jedec" flash compatible binding

Re: [PATCH] ARM: tegra: add "nor-jedec" flash compatible binding

[Brian Norris]

On Fri, May 08, 2015 at 11:43:17AM -0700, Brian Norris wrote:
> I'm open to change, as this binding is new in 4.1-rc1.

Though we'd need to make sure any change to "nor-jedec" gets applied to
the shmobile platforms that have started using this string in
linux-next.

Brian

Re: [PATCH] ARM: tegra: add "nor-jedec" flash compatible binding

[Geert Uytterhoeven]

On Fri, May 8, 2015 at 8:43 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Fri, May 08, 2015 at 10:00:12AM -0600, Stephen Warren wrote:
>> On 05/08/2015 12:21 AM, Rafał Miłecki wrote:
>> >Starting with commits
>> >8ff16cf ("Documentation: devicetree: m25p80: add "nor-jedec" binding")
>> >1103b85 ("mtd: m25p80: bind to "nor-jedec" ID, for auto-detection")
>> >we have "nor-jedec" binding indicating support for JEDEC identification.
>>
>> The documentation looks quite incomplete. "nor-jedec" sounds like
>> it's intended to be something generic. As such, it should be
>> documented in e.g.
>> Documentation/devicetree/bindings/mtd/nor-jedec.txt, not buried in
>> one particular flash device's binding. If it's not intended to be
>> generic, why isn't the existing "winbond,w25q32dw" enough?
>
> It is generic, though there are plenty of additional manufacturer/device
> pairs that could go on top of it. m25p80 was (one of?) the first
> supported, so the naming has been based on legacy, and we're in the
> process of unwinding a bit of that. If it helps, we could move the doc
> to .../mtd/spi-nor,nor-jedec.txt or something like that.
>
>> Equally, "nor-jedec" doesn't sound like the right name. It doesn't
>> differentiate between SPI and parallel NOR flash, which presumably
>> need different compatible values, since the programming model is
>> quite different, and the compatible value is supposed to
>> define/imply the SW-visible programming model.
>
> It's definitely for SPI only. There was much discussion about this a
> few months back. Somewhere along the way, it was mentioned that the
> context (SPI slave is a child of SPI master) would make this clear. I'm
> still not sure why we didn't end up with something more descriptive,
> though, like "spi-nor,nor-jedec".

What about "jedec,spi-nor"?

Is this "SERIAL FLASH DISCOVERABLE PARAMETERS (SFDP)"?
https://www.jedec.org/standards-documents/docs/jesd216b
(Don't have time to register with jedec now...)

If yes, "jedec,sfdp"? "jedec,jesd216b"?

> I'm open to change, as this binding is new in 4.1-rc1.

Indeed, as long as all users are fixed before 4.1 is released.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: tegra: add "nor-jedec" flash compatible binding

[Stephen Warren]

On 05/08/2015 02:57 PM, Geert Uytterhoeven wrote:
> On Fri, May 8, 2015 at 8:43 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
>> On Fri, May 08, 2015 at 10:00:12AM -0600, Stephen Warren wrote:
>>> On 05/08/2015 12:21 AM, Rafał Miłecki wrote:
>>>> Starting with commits
>>>> 8ff16cf ("Documentation: devicetree: m25p80: add "nor-jedec" binding")
>>>> 1103b85 ("mtd: m25p80: bind to "nor-jedec" ID, for auto-detection")
>>>> we have "nor-jedec" binding indicating support for JEDEC identification.
>>>
>>> The documentation looks quite incomplete. "nor-jedec" sounds like
>>> it's intended to be something generic. As such, it should be
>>> documented in e.g.
>>> Documentation/devicetree/bindings/mtd/nor-jedec.txt, not buried in
>>> one particular flash device's binding. If it's not intended to be
>>> generic, why isn't the existing "winbond,w25q32dw" enough?
>>
>> It is generic, though there are plenty of additional manufacturer/device
>> pairs that could go on top of it. m25p80 was (one of?) the first
>> supported, so the naming has been based on legacy, and we're in the
>> process of unwinding a bit of that. If it helps, we could move the doc
>> to .../mtd/spi-nor,nor-jedec.txt or something like that.
>>
>>> Equally, "nor-jedec" doesn't sound like the right name. It doesn't
>>> differentiate between SPI and parallel NOR flash, which presumably
>>> need different compatible values, since the programming model is
>>> quite different, and the compatible value is supposed to
>>> define/imply the SW-visible programming model.
>>
>> It's definitely for SPI only. There was much discussion about this a
>> few months back. Somewhere along the way, it was mentioned that the
>> context (SPI slave is a child of SPI master) would make this clear. I'm
>> still not sure why we didn't end up with something more descriptive,
>> though, like "spi-nor,nor-jedec".
>
> What about "jedec,spi-nor"?

That seems unique enough to me, or the options below if they're actually 
applicable.

> Is this "SERIAL FLASH DISCOVERABLE PARAMETERS (SFDP)"?
> https://www.jedec.org/standards-documents/docs/jesd216b
> (Don't have time to register with jedec now...)
>
> If yes, "jedec,sfdp"? "jedec,jesd216b"?

Re: [PATCH] ARM: tegra: add "nor-jedec" flash compatible binding

[Brian Norris]

On Fri, May 08, 2015 at 03:04:26PM -0600, Stephen Warren wrote:
> On 05/08/2015 02:57 PM, Geert Uytterhoeven wrote:
> >On Fri, May 8, 2015 at 8:43 PM, Brian Norris
> ><computersforpeace@gmail.com> wrote:
> >>On Fri, May 08, 2015 at 10:00:12AM -0600, Stephen Warren wrote:
> >>>On 05/08/2015 12:21 AM, Rafał Miłecki wrote:
> >>>>Starting with commits
> >>>>8ff16cf ("Documentation: devicetree: m25p80: add "nor-jedec" binding")
> >>>>1103b85 ("mtd: m25p80: bind to "nor-jedec" ID, for auto-detection")
> >>>>we have "nor-jedec" binding indicating support for JEDEC identification.
> >>>
> >>>The documentation looks quite incomplete. "nor-jedec" sounds like
> >>>it's intended to be something generic. As such, it should be
> >>>documented in e.g.
> >>>Documentation/devicetree/bindings/mtd/nor-jedec.txt, not buried in
> >>>one particular flash device's binding. If it's not intended to be
> >>>generic, why isn't the existing "winbond,w25q32dw" enough?
> >>
> >>It is generic, though there are plenty of additional manufacturer/device
> >>pairs that could go on top of it. m25p80 was (one of?) the first
> >>supported, so the naming has been based on legacy, and we're in the
> >>process of unwinding a bit of that. If it helps, we could move the doc
> >>to .../mtd/spi-nor,nor-jedec.txt or something like that.
> >>
> >>>Equally, "nor-jedec" doesn't sound like the right name. It doesn't
> >>>differentiate between SPI and parallel NOR flash, which presumably
> >>>need different compatible values, since the programming model is
> >>>quite different, and the compatible value is supposed to
> >>>define/imply the SW-visible programming model.
> >>
> >>It's definitely for SPI only. There was much discussion about this a
> >>few months back. Somewhere along the way, it was mentioned that the
> >>context (SPI slave is a child of SPI master) would make this clear. I'm
> >>still not sure why we didn't end up with something more descriptive,
> >>though, like "spi-nor,nor-jedec".
> >
> >What about "jedec,spi-nor"?
> 
> That seems unique enough to me, or the options below if they're
> actually applicable.

That could be OK with me. If I can get the magic blessing from the DT
folks, then I'll send patches to update everything.

> >Is this "SERIAL FLASH DISCOVERABLE PARAMETERS (SFDP)"?
> >https://www.jedec.org/standards-documents/docs/jesd216b
> >(Don't have time to register with jedec now...)
> >
> >If yes, "jedec,sfdp"? "jedec,jesd216b"?

It's not SFDP. We're looking at SPI flash that are detected simply by
the READ ID (9Fh) opcode. SFDP is on newer SPI flash which support the
5Ah opcode, for reading a parameter table. (I'd like to support this
soon, actually, and one of those sorts of strings might work.)

Brian

Re: [PATCH] ARM: tegra: add "nor-jedec" flash compatible binding

[Brian Norris]

Hi DT maintainers,

On Fri, May 08, 2015 at 02:34:14PM -0700, Brian Norris wrote:
> On Fri, May 08, 2015 at 03:04:26PM -0600, Stephen Warren wrote:
> > On 05/08/2015 02:57 PM, Geert Uytterhoeven wrote:
> > >On Fri, May 8, 2015 at 8:43 PM, Brian Norris <computersforpeace@gmail.com> wrote:
> > >>On Fri, May 08, 2015 at 10:00:12AM -0600, Stephen Warren wrote:
> > >>>Equally, "nor-jedec" doesn't sound like the right name. It doesn't
> > >>>differentiate between SPI and parallel NOR flash, which presumably
> > >>>need different compatible values, since the programming model is
> > >>>quite different, and the compatible value is supposed to
> > >>>define/imply the SW-visible programming model.
> > >>
> > >>It's definitely for SPI only. There was much discussion about this a
> > >>few months back. Somewhere along the way, it was mentioned that the
> > >>context (SPI slave is a child of SPI master) would make this clear. I'm
> > >>still not sure why we didn't end up with something more descriptive,
> > >>though, like "spi-nor,nor-jedec".
> > >
> > >What about "jedec,spi-nor"?
> > 
> > That seems unique enough to me, or the options below if they're
> > actually applicable.
> 
> That could be OK with me. If I can get the magic blessing from the DT
> folks, then I'll send patches to update everything.

Can I get an 'ack' for this change? We merged a binding for "nor-jedec"
in 4.1-rc1, with review (but no explicit 'ack') from Mark, and a few
DTS's are starting to use it. But we're now seeing objections, with a
request to change this to "jedec,spi-nor". I don't care too much, but I
can understand Stephen's point.

Anyway, I don't want to go through too many more patch cycles without an
explicit ack here on the "jedec,spi-nor" binding (i.e.,
s/nor-jedec/jedec,spi-nor/g). With an ack, then I can make sure the
binding and current users get changed before 4.1 is minted, and prevent
other ARM subarchitectures from pulling in the "wrong" binding for 4.2.

Brian