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.
WARNING: multiple messages have this Message-ID (diff)
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. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2021-10-07 12:37 UTC|newest] Thread overview: 38+ 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 ` Mika Westerberg 2021-09-30 10:07 ` [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked Mika Westerberg 2021-09-30 10:07 ` Mika Westerberg 2021-10-01 20:23 ` Mauro Lima 2021-10-01 20:23 ` Mauro Lima 2021-10-04 5:18 ` Mika Westerberg 2021-10-04 5:18 ` Mika Westerberg 2021-10-12 18:49 ` Mauro Lima 2021-10-12 18:49 ` Mauro Lima 2021-10-13 9:03 ` Mika Westerberg 2021-10-13 9:03 ` Mika Westerberg 2021-10-13 18:22 ` Mauro Lima 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-09-30 10:07 ` Mika Westerberg 2021-10-04 9:52 ` Pratyush Yadav 2021-10-04 9:52 ` Pratyush Yadav 2021-10-04 10:07 ` Mika Westerberg 2021-10-04 10:07 ` Mika Westerberg 2021-10-07 12:36 ` Pratyush Yadav [this message] 2021-10-07 12:36 ` Pratyush Yadav 2021-10-07 16:46 ` Mika Westerberg 2021-10-07 16:46 ` Mika Westerberg 2021-10-07 18:00 ` Pratyush Yadav 2021-10-07 18:00 ` Pratyush Yadav 2021-10-08 9:02 ` Mika Westerberg 2021-10-08 9:02 ` Mika Westerberg 2021-10-08 10:56 ` Pratyush Yadav 2021-10-08 10:56 ` Pratyush Yadav 2021-10-04 14:29 ` Andy Shevchenko 2021-10-04 14:29 ` Andy Shevchenko 2021-10-05 9:41 ` Mika Westerberg 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 10:07 ` Mika Westerberg 2021-09-30 15:03 ` Alexander Sverdlin 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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.