[PATCH v5 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM
Mika Westerberg
mika.westerberg at linux.intel.com
Wed Feb 2 02:54:11 PST 2022
Hi,
On Tue, Feb 01, 2022 at 03:24:58PM +0000, Tudor.Ambarus at microchip.com wrote:
>
> > diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/spi/spi-intel.c
> > similarity index 59%
> > rename from drivers/mtd/spi-nor/controllers/intel-spi.c
> > rename to drivers/spi/spi-intel.c
> > index f35597cbea0c..3b126927419c 100644
> > --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> > +++ b/drivers/spi/spi-intel.c
>
> cut
>
> > -static int intel_spi_erase(struct spi_nor *nor, loff_t offs)
> > +static int intel_spi_erase(struct intel_spi *ispi, const struct spi_mem_op *op)
> > {
> > - size_t erase_size, len = nor->mtd.erasesize;
> > - struct intel_spi *ispi = nor->priv;
> > + u8 opcode = op->cmd.opcode;
> > + u32 addr = op->addr.val;
> > u32 val, status, cmd;
> > int ret;
> >
> > - /* If the hardware can do 64k erase use that when possible */
> > - if (len >= SZ_64K && ispi->erase_64k) {
> > + switch (opcode) {
> > + case SPINOR_OP_SE:
>
> Would it worth to extend the intel_spi_mem_op struct and introduce an
> u32 replacement_op; member and use it directly here without doing the
> switch, so that we don't mix SPI NOR code in the driver?
Sure.
> Also the cmd assignement can be done after if (ispi->swseq_erase), right?
>
> > cmd = HSFSTS_CTL_FCYCLE_ERASE_64K;
> > - erase_size = SZ_64K;
> > - } else {
> > + break;
> > +
> > + case SPINOR_OP_BE_4K:
> > cmd = HSFSTS_CTL_FCYCLE_ERASE;
> > - erase_size = SZ_4K;
> > + break;
> > +
> > + default:
> > + return -EINVAL;
>
> you have a INTEL_SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_BE_4K_4B, 1), ...) defined.
> supported_op will return true, but you get an -EINVAL here.
Good catch. This will be dealt with the iop->replacement_op too (so the
switch will be dropped completely) in v6.
> > }
> >
> > - if (ispi->swseq_erase) {
> > - while (len > 0) {
> > - writel(offs, ispi->base + FADDR);
> > + writel(addr, ispi->base + FADDR);
> > +
> > + if (ispi->swseq_erase)
> > + return intel_spi_sw_cycle(ispi, opcode, 0,
> > + OPTYPE_WRITE_WITH_ADDR);
> > +
> > + /* Not needed with HW sequencer erase, make sure it is cleared */
> > + ispi->atomic_preopcode = 0;
> > +
> > + val = readl(ispi->base + HSFSTS_CTL);
> > + val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
> > + val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
> > + val |= HSFSTS_CTL_FGO;
> > + val |= cmd;
> > + writel(val, ispi->base + HSFSTS_CTL);
> > +
> > + ret = intel_spi_wait_hw_busy(ispi);
> > + if (ret)
> > + return ret;
> > +
> > + status = readl(ispi->base + HSFSTS_CTL);
> > + if (status & HSFSTS_CTL_FCERR)
> > + return -EIO;
> > + if (status & HSFSTS_CTL_AEL)
> > + return -EACCES;
> > +
> > + return 0;
> > +}
> > +
>
> cut
>
> > +static int intel_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> > +{
> > + struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> > + const struct intel_spi_mem_op *iop;
> >
> > - val = readl(ispi->base + HSFSTS_CTL);
> > - val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
> > - val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
> > - val |= cmd;
> > - val |= HSFSTS_CTL_FGO;
> > - writel(val, ispi->base + HSFSTS_CTL);
> > + iop = intel_spi_match_mem_op(ispi, op);
> > + if (!iop)
> > + return -EINVAL;
>
> return -EOPNOTSUPP?
OK
> >
> > - ret = intel_spi_wait_hw_busy(ispi);
> > - if (ret)
> > - return ret;
> > + return iop->exec_op(ispi, op);
> > +}
> >
> > - status = readl(ispi->base + HSFSTS_CTL);
> > - if (status & HSFSTS_CTL_FCERR)
> > - return -EIO;
> > - else if (status & HSFSTS_CTL_AEL)
> > - return -EACCES;
> > +static const char *intel_spi_get_name(struct spi_mem *mem)
> > +{
> > + const struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> > +
> > + /*
> > + * Return name of the flash controller device to be compatible
> > + * with the MTD version.
> > + */
> > + return dev_name(ispi->dev);
> > +}
> > +
> > +static const struct spi_controller_mem_ops intel_spi_mem_ops = {
> > + .supports_op = intel_spi_supports_mem_op,
> > + .exec_op = intel_spi_exec_mem_op,
> > + .get_name = intel_spi_get_name,
> > +};
> > +
> > +#define INTEL_SPI_OP_ADDR(__nbytes) \
> > + { \
> > + .nbytes = __nbytes, \
> > + }
> > +
> > +#define INTEL_SPI_OP_NO_DATA \
> > + { \
> > + .dir = SPI_MEM_NO_DATA, \
> > + }
> > +
> > +#define INTEL_SPI_OP_DATA_IN(__buswidth) \
> > + { \
> > + .dir = SPI_MEM_DATA_IN, \
> > + .buswidth = __buswidth, \
> > + }
> > +
> > +#define INTEL_SPI_OP_DATA_OUT(__buswidth) \
> > + { \
> > + .dir = SPI_MEM_DATA_OUT, \
> > + .buswidth = __buswidth, \
> > + }
> > +
> > +#define INTEL_SPI_MEM_OP(__cmd, __addr, __data, __exec_op) \
> > + { \
> > + .mem_op = { \
> > + .cmd = __cmd, \
> > + .addr = __addr, \
> > + .data = __data, \
> > + }, \
> > + .exec_op = __exec_op, \
> > + }
> > +
> > +/*
> > + * The controller handles pretty much everything internally based on the
> > + * SFDP data but we want to make sure we only support the operations
> > + * actually possible. Only check buswidth and transfer direction, the
> > + * core validates data.
> > + */
> > +#define INTEL_SPI_GENERIC_OPS
>
> checkpatch --strict complains:
> ERROR: Macros with complex values should be enclosed in parentheses
> #955: FILE: drivers/spi/spi-intel.c:826:
> +#define INTEL_SPI_GENERIC_OPS
I saw that too but to be honest, I have no idea how this could be
applied to the macros here. Therefore I chose to ignore this one.
> \
> > + /* Status register operations */ \
> > + INTEL_SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), \
> > + SPI_MEM_OP_NO_ADDR, \
> > + INTEL_SPI_OP_DATA_IN(1), \
> > + intel_spi_read_reg), \
>
> I like the idea with the array of ops. Maybe you'd like to order the ops and
> put the ops that are highly used as the first elements in the array, so that
> you speed up a bit the op selection at run-time. No hard requirement.
I think for better maintainability ordering them by type is better. I
will be adding dirmap support in subsequent patches which should then
use cached ->exec_op for read/write so there is no need to look up the
op more than once.
> Neat work. Thanks for the patience.
Thanks for the review! :)
More information about the linux-mtd
mailing list