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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Tue, Feb 01, 2022 at 03:24:58PM +0000, Tudor.Ambarus@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! :)

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/