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

[Boris Brezillon <boris.brezillon@bootlin.com>]

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.