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

[Pratyush Yadav <p.yadav@xxxxxx>]

On 04/10/21 01:07PM, Mika Westerberg wrote:
> 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).

That is not ideal. SPI NOR uses this to negotiate the best available 
protocol with the controller. Say you have a flash that is capable of 
octal mode but the controller can only support quad mode. Your driver 
will happily tell SPI NOR that it can support octal mode. I think you 
should check the SPI mode bits to make sure the protocol bus width is 
supported at least (see spi_check_buswidth_req() in spi-mem.c).

As for opcodes, is there no way to find out what opcodes the controller 
discovered via SFDP? Maybe we can't change them, but can we at least 
take a peek at them?

I think this has problems similar to the Cadence xSPI controller [0].

Sorry I only replied to this after you posted a new version. It got lost 
in the heap of emails in my inbox :-(

[0] https://patchwork.kernel.org/project/spi-devel-general/patch/1630499858-20456-1-git-send-email-pthombar@xxxxxxxxxxx/

> 
> Hope this clarifies.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.