Re: [PATCH v2 04/10] spi: Extend the core to ease integration of SPI memory controllers

Boris Brezillon
2018-04-12T15:10:31Z

On Thu, 12 Apr 2018 20:08:28 +0530
Vignesh R <vigneshr@ti.com> wrote:

> Hi,
>
> - Sourav Poddar <sourav.poddar@ti.com> mail ID no longer valid.
>
> On Wednesday 11 April 2018 04:14 AM, Boris Brezillon wrote:
> > Some controllers are exposing high-level interfaces to access various
> > kind of SPI memories. Unfortunately they do not fit in the current
> > spi_controller model and usually have drivers placed in
> > drivers/mtd/spi-nor which are only supporting SPI NORs and not SPI
> > memories in general.
> >
> > This is an attempt at defining a SPI memory interface which works for
> > all kinds of SPI memories (NORs, NANDs, SRAMs).
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
>
> [...]
>
> > +/**
> > + * struct spi_mem_op - describes a SPI memory operation
> > + * @cmd.buswidth: number of IO lines used to transmit the command
> > + * @cmd.opcode: operation opcode
> > + * @addr.nbytes: number of address bytes to send. Can be zero if the operation
> > + * does not need to send an address
> > + * @addr.buswidth: number of IO lines used to transmit the address cycles
> > + * @addr.val: address value. This value is always sent MSB first on the bus.
> > + * Note that only @addr.nbytes are taken into account in this
> > + * address value, so users should make sure the value fits in the
> > + * assigned number of bytes.
> > + * @dummy.nbytes: number of dummy bytes to send after an opcode or address. Can
> > + * be zero if the operation does not require dummy bytes
> > + * @dummy.buswidth: number of IO lanes used to transmit the dummy bytes
> > + * @data.buswidth: number of IO lanes used to send/receive the data
> > + * @data.dir: direction of the transfer
> > + * @data.buf.in: input buffer
> > + * @data.buf.out: output buffer
> > + */
> > +struct spi_mem_op {
> > + struct {
> > + u8 buswidth;
> > + u8 opcode;
> > + } cmd;
> > +
> > + struct {
> > + u8 nbytes;
> > + u8 buswidth;
> > + u64 val;
>
> You could consider using loff_t here and be consistent with
> spi_nor_read/write() API as well as mtd->_read().

Hm, I always have a hard time using types which does not clearly say
how large they are, but okay.

>
> > + } addr;
> > +
> > + struct {
> > + u8 nbytes;
> > + u8 buswidth;
> > + } dummy;
> > +
> > + struct {
> > + u8 buswidth;
> > + enum spi_mem_data_dir dir;
> > + unsigned int nbytes;
> > + /* buf.{in,out} must be DMA-able. */
> > + union {
> > + void *in;
> > + const void *out;
> > + } buf;
> > + } data;
> > +};
> > +
>
> Some flash devices support Dual/Quad DDR (Double Data Rate) mode and the
> SPI controller driver would need to know this information. We will need
> to add a field for that.

Well, let's wait until we actually need that.

>
> Currently, there are drivers under mtd/spi-nor/ that need to know
> page/sector/total size of flash memory(info available in
> -`struct spi_nor). We would need a way to provide this info to spi_mem
> drivers, if we ever plan to move drivers under mtd/spi-nor to spi/

Again, we'll see when we'll try to move them, but I hope still we won't
need that. Looks like the kind of information I'd like to keep away
from spi controller drivers.