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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxx>]

Hi Vignesh,

On Tue, 17 Apr 2018 09:42:18 +0530
Vignesh R <vigneshr@xxxxxx> wrote:

> >>   
> >> >     
> >> > > + } 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?

It's not decided yet. I'm not closing the door to any of those advanced
controllers yet, just need to have a closer look before taking a
decision. To me, it sounds like information that might be needed to
handle direct mapping in write mode for SPI NORs and we're not there
yet.

> 
> 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

Ideally, all of them, but maybe not with all the advanced features
supported at the beginning. The only things I'd like to keep outside of
drivers/spi are controllers that are only able to send commands for a
specific type of memory (like only commands to address SPI NORs).

I'm fine discussing all of this, with real use cases to explain why we
need to expose the memory type, and the memory organization information
to SPI controller drivers, but I'd prefer to do that in a separate
thread if you don't mind.

Regards,

Boris
--
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