Re: [PATCH v3 3/3] spi: airoha: add SPI-NAND Flash controller driver

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6654 bytes --]

> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]