Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

Hi,

On Mon, Oct 04, 2021 at 03:22:41PM +0530, Pratyush Yadav wrote:
> On 30/09/21 01:07PM, Mika Westerberg wrote:
> > The preferred way to implement SPI-NOR controller drivers is through SPI
> > subsubsystem utilizing the SPI MEM core functions. This converts the
> > Intel SPI flash controller driver over the SPI MEM by moving the driver
> > from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> > SPI MEM functions. The driver name will be changed from intel-spi to
> > spi-intel to match the convention used in the SPI subsystem.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/mtd/spi-nor/controllers/Kconfig       |  36 ---
> >  drivers/mtd/spi-nor/controllers/Makefile      |   3 -
> >  drivers/mtd/spi-nor/controllers/intel-spi.h   |  21 --
> >  drivers/spi/Kconfig                           |  38 +++
> >  drivers/spi/Makefile                          |   3 +
> >  .../intel-spi-pci.c => spi/spi-intel-pci.c}   |  20 +-
> >  .../spi-intel-platform.c}                     |  21 +-
> >  .../intel-spi.c => spi/spi-intel.c}           | 300 +++++++++++-------
> >  drivers/spi/spi-intel.h                       |  19 ++
> >  include/linux/mfd/lpc_ich.h                   |   2 +-
> >  .../x86/{intel-spi.h => spi-intel.h}          |   6 +-
> >  11 files changed, 252 insertions(+), 217 deletions(-)
> >  delete mode 100644 drivers/mtd/spi-nor/controllers/intel-spi.h
> >  rename drivers/{mtd/spi-nor/controllers/intel-spi-pci.c => spi/spi-intel-pci.c} (86%)
> >  rename drivers/{mtd/spi-nor/controllers/intel-spi-platform.c => spi/spi-intel-platform.c} (65%)
> >  rename drivers/{mtd/spi-nor/controllers/intel-spi.c => spi/spi-intel.c} (80%)
> >  create mode 100644 drivers/spi/spi-intel.h
> >  rename include/linux/platform_data/x86/{intel-spi.h => spi-intel.h} (89%)
> > 
> [...]
> > +static bool intel_spi_supports_mem_op(struct spi_mem *mem,
> > +				      const struct spi_mem_op *op)
> > +{
> > +	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> >  
> > -			offs += erase_size;
> > -			len -= erase_size;
> > +	if (op->cmd.dtr)
> > +		return false;
> > +
> > +	if (ispi->swseq_reg && ispi->locked) {
> > +		int i;
> > +
> > +		for (i = 0; i < ARRAY_SIZE(ispi->opcodes); i++) {
> > +			if (ispi->opcodes[i] == op->cmd.opcode)
> > +				return true;
> >  		}
> >  
> > -		return 0;
> > +		return false;
> >  	}
> >  
> > -	/* Not needed with HW sequencer erase, make sure it is cleared */
> > -	ispi->atomic_preopcode = 0;
> > +	switch (op->cmd.opcode) {
> > +	case SPINOR_OP_RDID:
> > +	case SPINOR_OP_RDSR:
> > +	case SPINOR_OP_WRSR:
> > +		return true;
> >  
> > -	while (len > 0) {
> > -		writel(offs, ispi->base + FADDR);
> > +	case SPINOR_OP_READ:
> > +	case SPINOR_OP_READ_FAST:
> > +	case SPINOR_OP_READ_4B:
> > +	case SPINOR_OP_READ_FAST_4B:
> > +		return true;
> 
> I think the checks need to be stricter here. For example, I don't see 
> you pass in protocol width (dual, quad, octal, etc.) to intel_spi_read() 
> so I assume it can only do a certain protocol. You need to make sure 
> that other protocols are rejected here.
> 
> Similarly, you also don't pass in dummy cycles to intel_spi_read(), so I 
> assume it can only do a fix number of dummy cycles. Need to make sure 
> you reject unsupported ones. Same for other opcodes/cases.

Unfortunately there is no way to tell the controller any of these. It
simply does "read" or "write" (as we command it) and internally then
uses whatever it got from the SFDP tables of the flash chip. That's why
we just claim to support all these operations and let the controller do
its thing (whatever it is).

Hope this clarifies.