[v8,2/4] dt-bindings: Add bindings for SPI NAND devices

Message ID 20180601131400.17634-3-boris.brezillon@bootlin.com
State Changes Requested
Headers show
Series
  • mtd: Add a SPI NAND driver
Related show

Commit Message

Boris Brezillon June 1, 2018, 1:13 p.m.
Add bindings for SPI NAND chips.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v8:
- Fixed a typo in the commit message
---
 Documentation/devicetree/bindings/mtd/spi-nand.txt | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt

Comments

Geert Uytterhoeven June 1, 2018, 2:42 p.m. | #1
Hi Boris,

I became interested after reading the cover letter...

On Fri, Jun 1, 2018 at 3:13 PM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> Add bindings for SPI NAND chips.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/spi-nand.txt
> @@ -0,0 +1,27 @@
> +SPI NAND flash
> +
> +Required properties:
> +- compatible: should be "spi-nand"
> +- reg: should encode the chip-select line used to access the NAND chip
> +
> +Optional properties
> +- spi-max-frequency: maximum frequency of the SPI bus the chip can operate at.
> +                    This should encode board limitations (i.e. max freq can't
> +                    be achieved due to crosstalk on IO lines).
> +                    When unspecified, the driver assumes the chip can run at
> +                    the max frequency defined in the spec (information
> +                    extracted chip detection time).

This is a standard property according to
Documentation/devicetree/bindings/spi/spi-bus.txt. Can't you just refer
to that file, or just omit it, as it applies to all SPI slaves anyway?

> +- spi-tx-bus-width: The bus width (number of data wires) that is used for MOSI.
> +                   Only encodes the board constraints (i.e. when not all IO
> +                   signals are routed on the board). Device constraints are
> +                   extracted when detecting the chip, and controller
> +                   constraints are exposed by the SPI mem controller. If this
> +                   property is missing that means no constraint at the board
> +                   level.
> +- spi-rx-bus-width: The bus width (number of data wires) that is used for MISO.
> +                   Only encodes the board constraints (i.e. when not all IO
> +                   signals are routed on the board). Device constraints are
> +                   extracted when detecting the chip, and controller
> +                   constraints are exposed by the SPI mem controller. If this
> +                   property is missing that means no constraint at the board
> +                   level.

This does not match Documentation/devicetree/bindings/spi/spi-bus.txt,
which says the default is 1.

As these properties are handled by the SPI core in of_spi_parse_dt, why
would you want to deviate?

Commenting to the question in the cover letter: what would be the
purpose of spi-max-bus-width?

Thanks!

Gr{oetje,eeting}s,

                        Geert
Boris Brezillon June 1, 2018, 5:09 p.m. | #2
Hi Geert,

On Fri, 1 Jun 2018 16:42:26 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Boris,
> 
> I became interested after reading the cover letter...
> 
> On Fri, Jun 1, 2018 at 3:13 PM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > Add bindings for SPI NAND chips.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> Thanks for your patch!

And thanks for reviewing it ;-).

> 
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/spi-nand.txt
> > @@ -0,0 +1,27 @@
> > +SPI NAND flash
> > +
> > +Required properties:
> > +- compatible: should be "spi-nand"
> > +- reg: should encode the chip-select line used to access the NAND chip
> > +
> > +Optional properties
> > +- spi-max-frequency: maximum frequency of the SPI bus the chip can operate at.
> > +                    This should encode board limitations (i.e. max freq can't
> > +                    be achieved due to crosstalk on IO lines).
> > +                    When unspecified, the driver assumes the chip can run at
> > +                    the max frequency defined in the spec (information
> > +                    extracted chip detection time).
> 
> This is a standard property according to
> Documentation/devicetree/bindings/spi/spi-bus.txt. Can't you just refer
> to that file, or just omit it, as it applies to all SPI slaves anyway?

The thing is, the maximum frequency supported by a SPI NAND is directly
encoded in the NAND device ID and can be auto-detected. Why should we
define spi-max-frequency in the DT when we can automatically detect
this information? The only reason one might want to override
spi-max-frequency is when the board design impose such restrictions,
hence the precision I give here. 

> 
> > +- spi-tx-bus-width: The bus width (number of data wires) that is used for MOSI.
> > +                   Only encodes the board constraints (i.e. when not all IO
> > +                   signals are routed on the board). Device constraints are
> > +                   extracted when detecting the chip, and controller
> > +                   constraints are exposed by the SPI mem controller. If this
> > +                   property is missing that means no constraint at the board
> > +                   level.
> > +- spi-rx-bus-width: The bus width (number of data wires) that is used for MISO.
> > +                   Only encodes the board constraints (i.e. when not all IO
> > +                   signals are routed on the board). Device constraints are
> > +                   extracted when detecting the chip, and controller
> > +                   constraints are exposed by the SPI mem controller. If this
> > +                   property is missing that means no constraint at the board
> > +                   level.
> 
> This does not match Documentation/devicetree/bindings/spi/spi-bus.txt,
> which says the default is 1.

Yes, I know.

> 
> As these properties are handled by the SPI core in of_spi_parse_dt, why
> would you want to deviate?

Because, again, this information can be extracted from the NAND ID, and
the only reason we might want to override the information extracted
from the NAND ID is when the board design adds extra restrictions
(like, only 2 SIO lines wired on the 4 available).

> 
> Commenting to the question in the cover letter: what would be the
> purpose of spi-max-bus-width?

Defining how many IO lines are wired on the board design. If this
property is missing we would assume all IO lines are wired and the
restrictions would be negotiated between the controller and the device
without requiring explicit description in the DT.

Regards,

Boris
Geert Uytterhoeven June 1, 2018, 5:40 p.m. | #3
Hi Boris,

On Fri, Jun 1, 2018 at 7:09 PM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> On Fri, 1 Jun 2018 16:42:26 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Fri, Jun 1, 2018 at 3:13 PM, Boris Brezillon
>> <boris.brezillon@bootlin.com> wrote:
>> > Add bindings for SPI NAND chips.

>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/mtd/spi-nand.txt
>> > @@ -0,0 +1,27 @@
>> > +SPI NAND flash
>> > +
>> > +Required properties:
>> > +- compatible: should be "spi-nand"
>> > +- reg: should encode the chip-select line used to access the NAND chip
>> > +
>> > +Optional properties
>> > +- spi-max-frequency: maximum frequency of the SPI bus the chip can operate at.
>> > +                    This should encode board limitations (i.e. max freq can't
>> > +                    be achieved due to crosstalk on IO lines).
>> > +                    When unspecified, the driver assumes the chip can run at
>> > +                    the max frequency defined in the spec (information
>> > +                    extracted chip detection time).
>>
>> This is a standard property according to
>> Documentation/devicetree/bindings/spi/spi-bus.txt. Can't you just refer
>> to that file, or just omit it, as it applies to all SPI slaves anyway?
>
> The thing is, the maximum frequency supported by a SPI NAND is directly
> encoded in the NAND device ID and can be auto-detected. Why should we
> define spi-max-frequency in the DT when we can automatically detect
> this information? The only reason one might want to override

Because that's how we dealt with this traditionally. Or at least I thought so.
But include/linux/spi/spi.h says:

 * @max_speed_hz: Maximum clock rate to be used with this chip
 *      (on this board); may be changed by the device's driver.
 *      The spi_transfer.speed_hz can override this for each transfer.

and many drivers seem to do so.

> spi-max-frequency is when the board design impose such restrictions,
> hence the precision I give here.

Which is exactly the meaning of the standard property, isn't it?

So I think it can just be omitted here anyway.  If it's present, the SPI
core will make sure it's taken into account.

>> > +- spi-tx-bus-width: The bus width (number of data wires) that is used for MOSI.
>> > +                   Only encodes the board constraints (i.e. when not all IO
>> > +                   signals are routed on the board). Device constraints are
>> > +                   extracted when detecting the chip, and controller
>> > +                   constraints are exposed by the SPI mem controller. If this
>> > +                   property is missing that means no constraint at the board
>> > +                   level.
>> > +- spi-rx-bus-width: The bus width (number of data wires) that is used for MISO.
>> > +                   Only encodes the board constraints (i.e. when not all IO
>> > +                   signals are routed on the board). Device constraints are
>> > +                   extracted when detecting the chip, and controller
>> > +                   constraints are exposed by the SPI mem controller. If this
>> > +                   property is missing that means no constraint at the board
>> > +                   level.
>>
>> This does not match Documentation/devicetree/bindings/spi/spi-bus.txt,
>> which says the default is 1.
>
> Yes, I know.
>
>> As these properties are handled by the SPI core in of_spi_parse_dt, why
>> would you want to deviate?
>
> Because, again, this information can be extracted from the NAND ID, and
> the only reason we might want to override the information extracted
> from the NAND ID is when the board design adds extra restrictions
> (like, only 2 SIO lines wired on the 4 available).

In struct spi_device, this is specified using the SPI_[RT]_X_{DUAL,QUAD}
bits in the mode field. Documentation says:

 * @mode: The spi mode defines how data is clocked out and in.
 *      This may be changed by the device's driver.
 *      The "active low" default for chipselect mode can be overridden
 *      (by specifying SPI_CS_HIGH) as can the "MSB first" default for
 *      each word in a transfer (by specifying SPI_LSB_FIRST).

So this may also be specified by the device driver, but it seems no driver
does so for the dual/quad bits, except for drivers/mtd/spi-nor/nxp-spifi.c
(for rx only).

But here we do have the generic SPI DT bindings saying the default is 1.
So personally, I wouldn't go for the option of the least surprise, and
don't deviate from the generic bindings.

>> Commenting to the question in the cover letter: what would be the
>> purpose of spi-max-bus-width?
>
> Defining how many IO lines are wired on the board design. If this
> property is missing we would assume all IO lines are wired and the
> restrictions would be negotiated between the controller and the device
> without requiring explicit description in the DT.

Which is exactly the meaning of the standard property, except for the
default of all vs. 1.

I'll have to defer to Mark (broonie) for his opinion about deviating from
the way this is handled traditionally, and assuming different defaults...

Gr{oetje,eeting}s,

                        Geert
Rob Herring June 1, 2018, 7:18 p.m. | #4
On Fri, Jun 1, 2018 at 12:09 PM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> Hi Geert,
>
> On Fri, 1 Jun 2018 16:42:26 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
>> Hi Boris,
>>
>> I became interested after reading the cover letter...
>>
>> On Fri, Jun 1, 2018 at 3:13 PM, Boris Brezillon
>> <boris.brezillon@bootlin.com> wrote:
>> > Add bindings for SPI NAND chips.
>> >
>> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
>>
>> Thanks for your patch!
>
> And thanks for reviewing it ;-).
>
>>
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/mtd/spi-nand.txt
>> > @@ -0,0 +1,27 @@
>> > +SPI NAND flash
>> > +
>> > +Required properties:
>> > +- compatible: should be "spi-nand"

Seems awfully generic if you expect this alone. No chips with quirks
yet? Is there a standard for detection like jedec?

>> > +- reg: should encode the chip-select line used to access the NAND chip

"see spi.txt" should be enough.

>> > +
>> > +Optional properties
>> > +- spi-max-frequency: maximum frequency of the SPI bus the chip can operate at.
>> > +                    This should encode board limitations (i.e. max freq can't
>> > +                    be achieved due to crosstalk on IO lines).
>> > +                    When unspecified, the driver assumes the chip can run at
>> > +                    the max frequency defined in the spec (information
>> > +                    extracted chip detection time).
>>
>> This is a standard property according to
>> Documentation/devicetree/bindings/spi/spi-bus.txt. Can't you just refer
>> to that file, or just omit it, as it applies to all SPI slaves anyway?
>
> The thing is, the maximum frequency supported by a SPI NAND is directly
> encoded in the NAND device ID and can be auto-detected. Why should we
> define spi-max-frequency in the DT when we can automatically detect
> this information? The only reason one might want to override
> spi-max-frequency is when the board design impose such restrictions,
> hence the precision I give here.

This should always be the case. The operating frequency should be
min(host max, device max) unless the board has restrictions and needs
to set spi-max-frequency (and we really should have used
'bus-frequency' here). No doubt though, that is not what has been
done.

>> > +- spi-tx-bus-width: The bus width (number of data wires) that is used for MOSI.
>> > +                   Only encodes the board constraints (i.e. when not all IO
>> > +                   signals are routed on the board). Device constraints are
>> > +                   extracted when detecting the chip, and controller
>> > +                   constraints are exposed by the SPI mem controller. If this
>> > +                   property is missing that means no constraint at the board
>> > +                   level.
>> > +- spi-rx-bus-width: The bus width (number of data wires) that is used for MISO.
>> > +                   Only encodes the board constraints (i.e. when not all IO
>> > +                   signals are routed on the board). Device constraints are
>> > +                   extracted when detecting the chip, and controller
>> > +                   constraints are exposed by the SPI mem controller. If this
>> > +                   property is missing that means no constraint at the board
>> > +                   level.
>>
>> This does not match Documentation/devicetree/bindings/spi/spi-bus.txt,
>> which says the default is 1.
>
> Yes, I know.

Like frequency, this should have been similar. I imagine for the
common case, the number of host and device lines are 1 so it doesn't
really apply as this was added later on. Perhaps we can reword the
common definition to work for both?

Rob
Boris Brezillon June 1, 2018, 8:32 p.m. | #5
Hi Rob,

On Fri, 1 Jun 2018 14:18:35 -0500
Rob Herring <robh+dt@kernel.org> wrote:

> On Fri, Jun 1, 2018 at 12:09 PM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > Hi Geert,
> >
> > On Fri, 1 Jun 2018 16:42:26 +0200
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> >> Hi Boris,
> >>
> >> I became interested after reading the cover letter...
> >>
> >> On Fri, Jun 1, 2018 at 3:13 PM, Boris Brezillon
> >> <boris.brezillon@bootlin.com> wrote:
> >> > Add bindings for SPI NAND chips.
> >> >
> >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >>
> >> Thanks for your patch!
> >
> > And thanks for reviewing it ;-).
> >
> >>
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/mtd/spi-nand.txt
> >> > @@ -0,0 +1,27 @@
> >> > +SPI NAND flash
> >> > +
> >> > +Required properties:
> >> > +- compatible: should be "spi-nand"
> 
> Seems awfully generic if you expect this alone. No chips with quirks
> yet?

Believe it or not, but it seems that NAND vendors agreed on a common
instruction set, made the READID instruction mandatory, and the 2 bytes
returned by this operation seem to be unique and allow us to reliably
extract information about he SPI NAND chip. I'm not saying this will
keep going like that (I'm pretty sure they'll screw up the READID
detection and start re-using IDs for different NANDs at some point),
but so far it seems to work.

> Is there a standard for detection like jedec?

Not that I know of. Some NANDs embed an ONFi table, which is usually
used on parallel/raw NANDs, but this is not mandatory.

> 
> >> > +- reg: should encode the chip-select line used to access the NAND chip
> 
> "see spi.txt" should be enough.

Okay.

> 
> >> > +
> >> > +Optional properties
> >> > +- spi-max-frequency: maximum frequency of the SPI bus the chip can operate at.
> >> > +                    This should encode board limitations (i.e. max freq can't
> >> > +                    be achieved due to crosstalk on IO lines).
> >> > +                    When unspecified, the driver assumes the chip can run at
> >> > +                    the max frequency defined in the spec (information
> >> > +                    extracted chip detection time).
> >>
> >> This is a standard property according to
> >> Documentation/devicetree/bindings/spi/spi-bus.txt. Can't you just refer
> >> to that file, or just omit it, as it applies to all SPI slaves anyway?
> >
> > The thing is, the maximum frequency supported by a SPI NAND is directly
> > encoded in the NAND device ID and can be auto-detected. Why should we
> > define spi-max-frequency in the DT when we can automatically detect
> > this information? The only reason one might want to override
> > spi-max-frequency is when the board design impose such restrictions,
> > hence the precision I give here.
> 
> This should always be the case. The operating frequency should be
> min(host max, device max) unless the board has restrictions and needs
> to set spi-max-frequency (and we really should have used
> 'bus-frequency' here).

That was my feeling too.

> No doubt though, that is not what has been
> done.

Definitely not, actually, I'm not even sure that this sort if
negotiation is supported by the framework. Right now, the SPI NAND
driver does not try to set max device freq, but that's something I was
planning to work on at some point, and since bindings are supposed to
be set in stone I thought I would clarify the meaning of
spi-max-frequency for the SPI NAND use case.

> 
> >> > +- spi-tx-bus-width: The bus width (number of data wires) that is used for MOSI.
> >> > +                   Only encodes the board constraints (i.e. when not all IO
> >> > +                   signals are routed on the board). Device constraints are
> >> > +                   extracted when detecting the chip, and controller
> >> > +                   constraints are exposed by the SPI mem controller. If this
> >> > +                   property is missing that means no constraint at the board
> >> > +                   level.
> >> > +- spi-rx-bus-width: The bus width (number of data wires) that is used for MISO.
> >> > +                   Only encodes the board constraints (i.e. when not all IO
> >> > +                   signals are routed on the board). Device constraints are
> >> > +                   extracted when detecting the chip, and controller
> >> > +                   constraints are exposed by the SPI mem controller. If this
> >> > +                   property is missing that means no constraint at the board
> >> > +                   level.
> >>
> >> This does not match Documentation/devicetree/bindings/spi/spi-bus.txt,
> >> which says the default is 1.
> >
> > Yes, I know.
> 
> Like frequency, this should have been similar. I imagine for the
> common case, the number of host and device lines are 1 so it doesn't
> really apply as this was added later on. Perhaps we can reword the
> common definition to work for both?

Do you have a suggestion? Also, I fear that globally changing the
meaning of these props will make most implementations not compliant
with this new definition.

Regards,

Boris
Boris Brezillon June 1, 2018, 9:07 p.m. | #6
On Fri, 1 Jun 2018 19:40:00 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Boris,
> 
> On Fri, Jun 1, 2018 at 7:09 PM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > On Fri, 1 Jun 2018 16:42:26 +0200
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> >> On Fri, Jun 1, 2018 at 3:13 PM, Boris Brezillon
> >> <boris.brezillon@bootlin.com> wrote:  
> >> > Add bindings for SPI NAND chips.  
> 
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/mtd/spi-nand.txt
> >> > @@ -0,0 +1,27 @@
> >> > +SPI NAND flash
> >> > +
> >> > +Required properties:
> >> > +- compatible: should be "spi-nand"
> >> > +- reg: should encode the chip-select line used to access the NAND chip
> >> > +
> >> > +Optional properties
> >> > +- spi-max-frequency: maximum frequency of the SPI bus the chip can operate at.
> >> > +                    This should encode board limitations (i.e. max freq can't
> >> > +                    be achieved due to crosstalk on IO lines).
> >> > +                    When unspecified, the driver assumes the chip can run at
> >> > +                    the max frequency defined in the spec (information
> >> > +                    extracted chip detection time).  
> >>
> >> This is a standard property according to
> >> Documentation/devicetree/bindings/spi/spi-bus.txt. Can't you just refer
> >> to that file, or just omit it, as it applies to all SPI slaves anyway?  
> >
> > The thing is, the maximum frequency supported by a SPI NAND is directly
> > encoded in the NAND device ID and can be auto-detected. Why should we
> > define spi-max-frequency in the DT when we can automatically detect
> > this information? The only reason one might want to override  
> 
> Because that's how we dealt with this traditionally. Or at least I thought so.
> But include/linux/spi/spi.h says:
> 
>  * @max_speed_hz: Maximum clock rate to be used with this chip
>  *      (on this board); may be changed by the device's driver.
>  *      The spi_transfer.speed_hz can override this for each transfer.
> 
> and many drivers seem to do so.

And that's wrong, isn't it? I mean, if you have a constraint at the
board level, why should we allow the SPI device driver to override this
value? We should have:

	max(controller_max, device_max, board_max)

and not (this is my understanding of how SPI max-freq selection works)

	if (board_max)
		max = board_max;
	else
		max = controller_max;

	if (device_max)
		max = device_max;

> 
> > spi-max-frequency is when the board design impose such restrictions,
> > hence the precision I give here.  
> 
> Which is exactly the meaning of the standard property, isn't it?

Maybe it is how people expect the max-freq selection to work, but in
practice it doesn't seem to work like that (see spi_setup() and how the
value passed by the device driver overrides everything without even
checking if the controller and board constraints impose a lower freq).

> 
> So I think it can just be omitted here anyway.  If it's present, the SPI
> core will make sure it's taken into account.

Yes, I should probably just refer to spi-bus.txt here.

> 
> >> > +- spi-tx-bus-width: The bus width (number of data wires) that is used for MOSI.
> >> > +                   Only encodes the board constraints (i.e. when not all IO
> >> > +                   signals are routed on the board). Device constraints are
> >> > +                   extracted when detecting the chip, and controller
> >> > +                   constraints are exposed by the SPI mem controller. If this
> >> > +                   property is missing that means no constraint at the board
> >> > +                   level.
> >> > +- spi-rx-bus-width: The bus width (number of data wires) that is used for MISO.
> >> > +                   Only encodes the board constraints (i.e. when not all IO
> >> > +                   signals are routed on the board). Device constraints are
> >> > +                   extracted when detecting the chip, and controller
> >> > +                   constraints are exposed by the SPI mem controller. If this
> >> > +                   property is missing that means no constraint at the board
> >> > +                   level.  
> >>
> >> This does not match Documentation/devicetree/bindings/spi/spi-bus.txt,
> >> which says the default is 1.  
> >
> > Yes, I know.
> >  
> >> As these properties are handled by the SPI core in of_spi_parse_dt, why
> >> would you want to deviate?  
> >
> > Because, again, this information can be extracted from the NAND ID, and
> > the only reason we might want to override the information extracted
> > from the NAND ID is when the board design adds extra restrictions
> > (like, only 2 SIO lines wired on the 4 available).  
> 
> In struct spi_device, this is specified using the SPI_[RT]_X_{DUAL,QUAD}
> bits in the mode field. Documentation says:
> 
>  * @mode: The spi mode defines how data is clocked out and in.
>  *      This may be changed by the device's driver.
>  *      The "active low" default for chipselect mode can be overridden
>  *      (by specifying SPI_CS_HIGH) as can the "MSB first" default for
>  *      each word in a transfer (by specifying SPI_LSB_FIRST).
> 
> So this may also be specified by the device driver, but it seems no driver
> does so for the dual/quad bits, except for drivers/mtd/spi-nor/nxp-spifi.c
> (for rx only).

Same problem as for spi-max-frequency, the driver can override the value
that was specified in the DT, and the core does not seem to check
if the board or controller constraint were stronger.

> 
> But here we do have the generic SPI DT bindings saying the default is 1.
> So personally, I wouldn't go for the option of the least surprise, and
> don't deviate from the generic bindings.

I do have a problem with this approach: we are in the process of
converting existing spi-nor controller drivers to the SPI mem model in
order to allow the same controller to indifferently control a SPI NAND
or a SPI NOR. Such drivers had their own bindings which most of the
time was close to the bindings described in spi-bus.txt with a few
exceptions. One of these exception is that drivers tend to not
explicitly describe the bus width but still use the max buswidth
supported by the controller (QuadSPI) to interface with NOR chips.

When we convert those drivers we have 2 choices:
1/ make existing setup work in a degraded mode (1-1-1) until they update
   their DT to explicitly specify the spi-{rx,tx}-bus-width
2/ consider that when spi-{rx,tx}-bus-width is missing the device should
   work in the fastest possible mode matching the device+controller
   constraints

I went for option #2, but maybe #1 is fine. Note that spi-nand is not a
problem here because this is a new binding, but I thought it would be
better to clarify the meaning of these props before people start
(ab)using them.

> 
> >> Commenting to the question in the cover letter: what would be the
> >> purpose of spi-max-bus-width?  
> >
> > Defining how many IO lines are wired on the board design. If this
> > property is missing we would assume all IO lines are wired and the
> > restrictions would be negotiated between the controller and the device
> > without requiring explicit description in the DT.  
> 
> Which is exactly the meaning of the standard property, except for the
> default of all vs. 1.

And that's a huge difference. Say you have a board design and several
equivalence for the SPI NAND chip, one is supporting mode 1-1-4 (AKA
QuadSPI) and the other one is supporting only mode 1-1-1 (Single SPI).
With your solution we'd have to have 2 DTs or force all models to
operate in 1-1-1 mode. 

> 
> I'll have to defer to Mark (broonie) for his opinion about deviating from
> the way this is handled traditionally, and assuming different defaults...

I'm not saying that we should blindly change the meaning of those
existing props, I just wanted to start a discussion to see how the
problem I'm mentioning here should be handled (addition of new props is
something I'm perfectly fine with).
Boris Brezillon June 4, 2018, 10:04 a.m. | #7
On Fri, 1 Jun 2018 19:40:00 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Boris,
> 
> On Fri, Jun 1, 2018 at 7:09 PM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > On Fri, 1 Jun 2018 16:42:26 +0200
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> >> On Fri, Jun 1, 2018 at 3:13 PM, Boris Brezillon
> >> <boris.brezillon@bootlin.com> wrote:  
> >> > Add bindings for SPI NAND chips.  
> 
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/mtd/spi-nand.txt
> >> > @@ -0,0 +1,27 @@
> >> > +SPI NAND flash
> >> > +
> >> > +Required properties:
> >> > +- compatible: should be "spi-nand"
> >> > +- reg: should encode the chip-select line used to access the NAND chip
> >> > +
> >> > +Optional properties
> >> > +- spi-max-frequency: maximum frequency of the SPI bus the chip can operate at.
> >> > +                    This should encode board limitations (i.e. max freq can't
> >> > +                    be achieved due to crosstalk on IO lines).
> >> > +                    When unspecified, the driver assumes the chip can run at
> >> > +                    the max frequency defined in the spec (information
> >> > +                    extracted chip detection time).  
> >>
> >> This is a standard property according to
> >> Documentation/devicetree/bindings/spi/spi-bus.txt. Can't you just refer
> >> to that file, or just omit it, as it applies to all SPI slaves anyway?  
> >
> > The thing is, the maximum frequency supported by a SPI NAND is directly
> > encoded in the NAND device ID and can be auto-detected. Why should we
> > define spi-max-frequency in the DT when we can automatically detect
> > this information? The only reason one might want to override  
> 
> Because that's how we dealt with this traditionally. Or at least I thought so.
> But include/linux/spi/spi.h says:
> 
>  * @max_speed_hz: Maximum clock rate to be used with this chip
>  *      (on this board); may be changed by the device's driver.
>  *      The spi_transfer.speed_hz can override this for each transfer.
> 
> and many drivers seem to do so.
> 
> > spi-max-frequency is when the board design impose such restrictions,
> > hence the precision I give here.  
> 
> Which is exactly the meaning of the standard property, isn't it?
> 
> So I think it can just be omitted here anyway.  If it's present, the SPI
> core will make sure it's taken into account.
> 
> >> > +- spi-tx-bus-width: The bus width (number of data wires) that is used for MOSI.
> >> > +                   Only encodes the board constraints (i.e. when not all IO
> >> > +                   signals are routed on the board). Device constraints are
> >> > +                   extracted when detecting the chip, and controller
> >> > +                   constraints are exposed by the SPI mem controller. If this
> >> > +                   property is missing that means no constraint at the board
> >> > +                   level.
> >> > +- spi-rx-bus-width: The bus width (number of data wires) that is used for MISO.
> >> > +                   Only encodes the board constraints (i.e. when not all IO
> >> > +                   signals are routed on the board). Device constraints are
> >> > +                   extracted when detecting the chip, and controller
> >> > +                   constraints are exposed by the SPI mem controller. If this
> >> > +                   property is missing that means no constraint at the board
> >> > +                   level.  
> >>
> >> This does not match Documentation/devicetree/bindings/spi/spi-bus.txt,
> >> which says the default is 1.  
> >
> > Yes, I know.
> >  
> >> As these properties are handled by the SPI core in of_spi_parse_dt, why
> >> would you want to deviate?  
> >
> > Because, again, this information can be extracted from the NAND ID, and
> > the only reason we might want to override the information extracted
> > from the NAND ID is when the board design adds extra restrictions
> > (like, only 2 SIO lines wired on the 4 available).  
> 
> In struct spi_device, this is specified using the SPI_[RT]_X_{DUAL,QUAD}
> bits in the mode field. Documentation says:
> 
>  * @mode: The spi mode defines how data is clocked out and in.
>  *      This may be changed by the device's driver.
>  *      The "active low" default for chipselect mode can be overridden
>  *      (by specifying SPI_CS_HIGH) as can the "MSB first" default for
>  *      each word in a transfer (by specifying SPI_LSB_FIRST).
> 
> So this may also be specified by the device driver, but it seems no driver
> does so for the dual/quad bits, except for drivers/mtd/spi-nor/nxp-spifi.c
> (for rx only).
> 
> But here we do have the generic SPI DT bindings saying the default is 1.
> So personally, I wouldn't go for the option of the least surprise, and
> don't deviate from the generic bindings.
> 
> >> Commenting to the question in the cover letter: what would be the
> >> purpose of spi-max-bus-width?  
> >
> > Defining how many IO lines are wired on the board design. If this
> > property is missing we would assume all IO lines are wired and the
> > restrictions would be negotiated between the controller and the device
> > without requiring explicit description in the DT.  
> 
> Which is exactly the meaning of the standard property, except for the
> default of all vs. 1.
> 
> I'll have to defer to Mark (broonie) for his opinion about deviating from
> the way this is handled traditionally, and assuming different defaults...

I think I'll drop the extra description on spi-max-frequency and
spi-{rx,tx}-bus-width props for now. Those are anyway not taken into
account by the SPI NAND driver yet.

Patch

diff --git a/Documentation/devicetree/bindings/mtd/spi-nand.txt b/Documentation/devicetree/bindings/mtd/spi-nand.txt
new file mode 100644
index 000000000000..d55f80196c63
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/spi-nand.txt
@@ -0,0 +1,27 @@ 
+SPI NAND flash
+
+Required properties:
+- compatible: should be "spi-nand"
+- reg: should encode the chip-select line used to access the NAND chip
+
+Optional properties
+- spi-max-frequency: maximum frequency of the SPI bus the chip can operate at.
+		     This should encode board limitations (i.e. max freq can't
+		     be achieved due to crosstalk on IO lines).
+		     When unspecified, the driver assumes the chip can run at
+		     the max frequency defined in the spec (information
+		     extracted chip detection time).
+- spi-tx-bus-width: The bus width (number of data wires) that is used for MOSI.
+		    Only encodes the board constraints (i.e. when not all IO
+		    signals are routed on the board). Device constraints are
+		    extracted when detecting the chip, and controller
+		    constraints are exposed by the SPI mem controller. If this
+		    property is missing that means no constraint at the board
+		    level.
+- spi-rx-bus-width: The bus width (number of data wires) that is used for MISO.
+		    Only encodes the board constraints (i.e. when not all IO
+		    signals are routed on the board). Device constraints are
+		    extracted when detecting the chip, and controller
+		    constraints are exposed by the SPI mem controller. If this
+		    property is missing that means no constraint at the board
+		    level.