> Il 23/04/24 12:16, Lorenzo Bianconi ha scritto:
[...]
> > +
> > + err = airoha_snand_nfi_config(as_ctrl);
> > + if (err)
> > + return err;
> > +
> > + dma_sync_single_for_device(as_ctrl->dev, as_dev->dma_addr,
> > + as_dev->buf_len, DMA_BIDIRECTIONAL);
> > + mb();
>
> Are you sure you need this memory barrier here?
>
> P.S.: Just in case... any other write or read will anyway add a barrier ;-)
>
ack, I will fix it in v4.
> > +
> > + /* set dma addr */
> > + err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_STRADDR,
> > + as_dev->dma_addr);
> > + if (err)
> > + return err;
> > +
>
> ..snip..
>
> > +
> > +static ssize_t airoha_snand_dirmap_write(struct spi_mem_dirmap_desc *desc,
> > + u64 offs, size_t len, const void *buf)
> > +{
> > + struct spi_device *spi = desc->mem->spi;
> > + struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
> > + struct spi_mem_op *op = &desc->info.op_tmpl;
> > + struct airoha_snand_ctrl *as_ctrl;
> > + u32 wr_mode, val;
> > + int err;
> > +
> > + as_ctrl = spi_controller_get_devdata(spi->controller);
> > + err = airoha_snand_set_mode(as_ctrl, SPI_MODE_MANUAL);
> > + if (err < 0)
> > + return err;
> > +
> > + dma_sync_single_for_cpu(as_ctrl->dev, as_dev->dma_addr,
> > + as_dev->buf_len, DMA_BIDIRECTIONAL);
> > + memcpy(as_dev->txrx_buf + offs, buf, len);
> > + dma_sync_single_for_device(as_ctrl->dev, as_dev->dma_addr,
> > + as_dev->buf_len, DMA_BIDIRECTIONAL);
> > + mb();
> > +
> > + err = airoha_snand_set_mode(as_ctrl, SPI_MODE_DMA);
> > + if (err < 0)
> > + return err;
> > +
> > + err = airoha_snand_nfi_config(as_ctrl);
> > + if (err)
> > + return err;
> > +
> > + if (op->cmd.opcode == SPI_NAND_OP_PROGRAM_LOAD_QUAD ||
> > + op->cmd.opcode == SPI_NAND_OP_PROGRAM_LOAD_RAMDON_QUAD)
> > + wr_mode = 2;
>
> I'm mostly sure that this '2' is supposed to be BIT(1) instead
ack, I will fix it in v4.
>
> > + else
> > + wr_mode = 0;
> > +
> > + err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_STRADDR,
> > + as_dev->dma_addr);
> > + if (err)
> > + return err;
> > +
> > + val = as_ctrl->nfi_cfg.sec_size * as_ctrl->nfi_cfg.sec_num;
> > + val = FIELD_PREP(SPI_NFI_PROG_LOAD_BYTE_NUM, val);
>
> val = FIELD_PREP(SPI_NFI_PROG_LOAD_BYTE_NUM,
> as_ctrl->nfi_cfg.sec_size * as_ctrl->nfi_cfg.sec_num);
>
> Being this 100 cols eventually, you wouldn't even need two lines - but
> if you don't want to go 100, it's still more readable (imo) like this,
> even if in two lines.
I prefer to be below 79 columns, so I will go for the 2 lines :)
I will fix it in v4.
>
> > + err = regmap_update_bits(as_ctrl->regmap_nfi,
> > + REG_SPI_NFI_SNF_MISC_CTL2,
> > + SPI_NFI_PROG_LOAD_BYTE_NUM, val);
> > + if (err)
> > + return err;
> > +
> > + err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_PG_CTL1,
> > + FIELD_PREP(SPI_NFI_PG_LOAD_CMD,
> > + op->cmd.opcode));
> > + if (err)
> > + return err;
> > +
> > + err = regmap_write(as_ctrl->regmap_nfi, REG_SPI_NFI_SNF_MISC_CTL,
> > + FIELD_PREP(SPI_NFI_DATA_READ_WR_MODE, wr_mode));
> > + if (err)
> > + return err;
> > +
>
> ..snip..
>
> > +
> > +#define SPI_NAND_CACHE_SIZE (4096 + 256)
>
> #include <linux/sizes.h> <--- at the beginning
>
> #define SPI_NAND_CACHE_SIZE (SZ_4K + SZ_256)
>
> ...there are macros for that, might as well use them, right? :-)
ack, I will fix it in v4.
>
> > +
> > +static int airoha_snand_setup(struct spi_device *spi)
> > +{
> > + struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
> > + struct airoha_snand_ctrl *as_ctrl;
> > +
> > + as_dev = kzalloc(sizeof(*as_dev), GFP_KERNEL);
> > + if (!as_dev)
> > + return -ENOMEM;
> > +
> > + spi_set_ctldata(spi, as_dev);
> > + as_dev->data_need_update = true;
> > +
> > + /* prepare device buffer */
> > + as_dev->buf_len = SPI_NAND_CACHE_SIZE;
> > + as_dev->txrx_buf = kzalloc(as_dev->buf_len, GFP_KERNEL);
> > + if (!as_dev->txrx_buf)
> > + goto error_dev_free;
> > +
> > + as_ctrl = spi_controller_get_devdata(spi->controller);
> > + as_dev->dma_addr = dma_map_single(as_ctrl->dev, as_dev->txrx_buf,
> > + as_dev->buf_len, DMA_BIDIRECTIONAL);
> > + if (dma_mapping_error(as_ctrl->dev, as_dev->dma_addr))
> > + goto error_buf_free;
> > +
> > + return 0;
> > +
> > +error_buf_free:
> > + kfree(as_dev->txrx_buf);
> > +error_dev_free:
> > + kfree(as_dev);
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static void airoha_snand_cleanup(struct spi_device *spi)
> > +{
> > + struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
> > + struct airoha_snand_ctrl *as_ctrl;
> > +
> > + as_ctrl = spi_controller_get_devdata(spi->controller);
> > + dma_unmap_single(as_ctrl->dev, as_dev->dma_addr,
> > + as_dev->buf_len, DMA_BIDIRECTIONAL);
> > + kfree(as_dev->txrx_buf);
> > +
> > + spi_set_ctldata(spi, NULL);
> > +}
> > +
> > +static int airoha_snand_nfi_setup(struct airoha_snand_ctrl *as_ctrl)
> > +{
> > + u32 val, sec_size, sec_num;
> > + int err;
> > +
> > + err = regmap_read(as_ctrl->regmap_nfi, REG_SPI_NFI_CON, &val);
> > + if (err)
> > + return err;
> > +
> > + sec_num = FIELD_GET(SPI_NFI_SEC_NUM, val);
> > +
> > + err = regmap_read(as_ctrl->regmap_nfi,
> > + REG_SPI_NFI_SECCUS_SIZE, &val);
> > + if (err)
> > + return err;
> > +
> > + sec_size = FIELD_GET(SPI_NFI_CUS_SEC_SIZE, val);
> > +
> > + /* init default value */
> > + as_ctrl->nfi_cfg.sec_size = sec_size;
> > + as_ctrl->nfi_cfg.sec_num = sec_num;
> > + as_ctrl->nfi_cfg.page_size = rounddown(sec_size * sec_num, 1024);
> > + as_ctrl->nfi_cfg.spare_size = 16;
> > +
> > + err = airoha_snand_nfi_init(as_ctrl);
> > + if (err)
> > + return err;
> > +
> > + return airoha_snand_nfi_config(as_ctrl);
> > +}
> > +
> > +static const struct regmap_config spi_ctrl_regmap_config = {
> > + .name = "ctrl",
> > + .reg_bits = 32,
> > + .val_bits = 32,
> > + .reg_stride = 4,
> > + .max_register = REG_SPI_CTRL_NFI2SPI_EN,
> > +};
> > +
> > +static const struct regmap_config spi_nfi_regmap_config = {
> > + .name = "nfi",
> > + .reg_bits = 32,
> > + .val_bits = 32,
> > + .reg_stride = 4,
> > + .max_register = REG_SPI_NFI_SNF_NFI_CNFG,
> > +};
> > +
> > +static const struct of_device_id airoha_snand_ids[] = {
> > + { .compatible = "airoha,en7581-snand" },
> > + { }
>
> { /* sentinel */ }
>
> ...please!
ack, I will fix it in v4.
Regards,
Lorenzo
>
> > +};
> > +MODULE_DEVICE_TABLE(of, airoha_snand_ids);
> > +
>
> Cheers,
> Angelo
>
>
Attachment:
signature.asc
Description: PGP signature
