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

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

> 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?
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.

>         }
> 
> -       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?

> 
> -               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

                                          \
> +       /* 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.

Neat work. Thanks for the patience.
ta