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

[Vignesh R <vigneshr@xxxxxx>]

On Friday 13 April 2018 01:29 AM, Boris Brezillon wrote:
[...]
>> > > +/**
>> > > + * 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.
> 
> BTW, loff_t is signed, which doesn't really make sense here, so I'd
> like to keep an u64 unless you have a strong reason not to.
> 

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.
> 
> Let me clarify this part. I already thought a bit about this problem,
> and that's the very reason we have an intermediate layer with a spi_mem
> struct pointing to the real spi_device object. The idea is to add new
> fields to spi_mem object if/when we really have to. We'd also have to
> add ->attach/detach() methods to spi_mem_ops so that SPI mem controller
> can know when a new device is about to be accessed by a spi-mem
> driver, can parse the information provided in spi_mem and configure the
> controller accordingly.
> 
> Now, even if that's something I considered when designing the spi-mem
> interface, I'd like to stay away from this sort of specialization as
> long as possible. Why? Simply because dealing with memory specificities
> like "is it a NOR, a NAND or an SRAM? Should I erase blocks before
> writing data? What's the page size, sector size, eraseblock size? ..."
> is not something that belongs in the SPI framework. IMHO, it should
> stay in SPI mem drivers (the SPI NOR or SPI NAND framework are such SPI
> mem drivers).
> 
> This being said, I see a real need for advanced features. One example I
> have in mind is a "direct-mapping API", where a spi_mem user could ask
> for a specific region of the memory to be directly mapped (if the
> feature is supported by the controller of course). And that's something
> I think we can make generic enough to consider adding it to the
> spi_mem_ops interface. All we'll need is a way to say "I want to map
> this portion of the memory in R, W or RW and when you need to
> read/write use this spi_mem_op and patch the address based on the
> actual memory address that is being accessed".
>


Many of the SPI NOR controllers, especially the ones that support direct
mapping are smart and need more flash specific data. For example,
cadence-quadspi needs to know pagesize as this controller automatically
sends write enable when writes cross page boundary. I guess, such
controllers pose a problem to spi_mem_ops in passing spi_nor internal
data to drivers. Or such controllers may need to be continued to be
supported directly under spi-nor framework?

I am okay with this series in general. But, was trying to understand
which drivers will fall under spi_mem and which will continue to remain
under mtd/spi-nor


> Maybe I'm wrong and some controllers actually need to know which type
> of device they are dealing with, but in that case, I'm not so sure they
> belong in the SPI framework at all, unless they provide a dummy mode in
> which they can act as a regular SPI/SPI mem controller (i.e. send SPI
> mem operations without trying to be smart and do things behind our
> back).
> 


-- 
Regards
Vignesh
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html