From: Pratyush Yadav <p.yadav@ti.com>
To: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Tudor Ambarus <tudor.ambarus@microchip.com>,
Mark Brown <broonie@kernel.org>, Lee Jones <lee.jones@linaro.org>,
Michael Walle <michael@walle.cc>,
Miquel Raynal <miquel.raynal@bootlin.com>,
Richard Weinberger <richard@nod.at>,
Vignesh Raghavendra <vigneshr@ti.com>,
Jonathan Corbet <corbet@lwn.net>,
Mauro Lima <mauro.lima@eclypsium.com>,
Alexander Sverdlin <alexander.sverdlin@nokia.com>,
<linux-mtd@lists.infradead.org>, <linux-spi@vger.kernel.org>
Subject: Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM
Date: Thu, 7 Oct 2021 18:06:23 +0530 [thread overview]
Message-ID: <20211007123621.ld4aqasr3hlwq2c7@ti.com> (raw)
In-Reply-To: <YVrSbyEsSLMOu1bU@lahna>
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@linux.intel.com>
> > > ---
> > > 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@cadence.com/
>
> Hope this clarifies.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
next prev parent reply other threads:[~2021-10-07 12:37 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-30 10:07 [PATCH 0/3] mtd: spi-nor / spi / MFD: Convert intel-spi to SPI MEM Mika Westerberg
2021-09-30 10:07 ` [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked Mika Westerberg
2021-10-01 20:23 ` Mauro Lima
2021-10-04 5:18 ` Mika Westerberg
2021-10-12 18:49 ` Mauro Lima
2021-10-13 9:03 ` Mika Westerberg
2021-10-13 18:22 ` Mauro Lima
2021-09-30 10:07 ` [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM Mika Westerberg
2021-10-04 9:52 ` Pratyush Yadav
2021-10-04 10:07 ` Mika Westerberg
2021-10-07 12:36 ` Pratyush Yadav [this message]
2021-10-07 16:46 ` Mika Westerberg
2021-10-07 18:00 ` Pratyush Yadav
2021-10-08 9:02 ` Mika Westerberg
2021-10-08 10:56 ` Pratyush Yadav
2021-10-04 14:29 ` Andy Shevchenko
2021-10-05 9:41 ` Mika Westerberg
2021-09-30 10:07 ` [PATCH 3/3] Documentation / MTD: Rename the intel-spi driver Mika Westerberg
2021-09-30 15:03 ` Alexander Sverdlin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211007123621.ld4aqasr3hlwq2c7@ti.com \
--to=p.yadav@ti.com \
--cc=alexander.sverdlin@nokia.com \
--cc=broonie@kernel.org \
--cc=corbet@lwn.net \
--cc=lee.jones@linaro.org \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-spi@vger.kernel.org \
--cc=mauro.lima@eclypsium.com \
--cc=michael@walle.cc \
--cc=mika.westerberg@linux.intel.com \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
--cc=tudor.ambarus@microchip.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).