[PATCH v6 11/15] nand: spi: add basic operations support
Peter Pan 潘栋 (peterpandong)
peterpandong at micron.com
Tue May 30 23:51:34 PDT 2017
Hi Boris,
> On 30 May 2017, at 06:12, Boris Brezillon <boris.brezillon at free-electrons.com> wrote:
>
> On Wed, 24 May 2017 15:07:07 +0800
> Peter Pan <peterpandong at micron.com> wrote:
>
>> This commit is to support read, readoob, write,
>> writeoob and erase operations in the new spi nand
>> framework. No ECC support right now.
>
> I still don't understand why patch 10 and 11 are separated. I'd prefer
> to see one single patch adding basic spi-nand support, that is,
> everything to support detection+initialization+read/write access.
Let' put patch 10 and 11 together in v7:)
>>
>> Signed-off-by: Peter Pan <peterpandong at micron.com>
>> ---
>> drivers/mtd/nand/spi/core.c | 638 ++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/mtd/spinand.h | 3 +
>> 2 files changed, 641 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index 93ce212..6251469 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -108,6 +108,222 @@ static int spinand_read_status(struct spinand_device *spinand, u8 *status)
>> }
>>
>> /**
>> + * spinand_get_cfg - get configuration register value
>> + * @spinand: SPI NAND device structure
>> + * @cfg: buffer to store value
>> + * Description:
>> + * Configuration register includes OTP config, Lock Tight enable/disable
>> + * and Internal ECC enable/disable.
>> + */
>> +static int spinand_get_cfg(struct spinand_device *spinand, u8 *cfg)
>> +{
>> + return spinand_read_reg(spinand, REG_CFG, cfg);
>> +}
>> +
>> +/**
>> + * spinand_set_cfg - set value to configuration register
>> + * @spinand: SPI NAND device structure
>> + * @cfg: value to set
>> + * Description:
>> + * Configuration register includes OTP config, Lock Tight enable/disable
>> + * and Internal ECC enable/disable.
>> + */
>> +static int spinand_set_cfg(struct spinand_device *spinand, u8 cfg)
>> +{
>> + return spinand_write_reg(spinand, REG_CFG, cfg);
>> +}
>> +
>> +/**
>> + * spinand_disable_ecc - disable internal ECC
>> + * @spinand: SPI NAND device structure
>> + */
>> +static void spinand_disable_ecc(struct spinand_device *spinand)
>> +{
>> + u8 cfg = 0;
>> +
>> + spinand_get_cfg(spinand, &cfg);
>> +
>> + if ((cfg & CFG_ECC_MASK) == CFG_ECC_ENABLE) {
>> + cfg &= ~CFG_ECC_ENABLE;
>> + spinand_set_cfg(spinand, cfg);
>> + }
>
> Is this really a generic (manufacturer-agnostic) feature??? I had the
> feeling that is was only working for Micron. If that's the case, this
> 'disable-ecc' step should be done in spi/micron.c in a
> micron_spinand_init() function.
As far as I can see, Micron is not the only vendor to disable on die ecc
by this way, actually all vendors I know use this . And this series
doesn't support on die ecc, so IMO it is necessary to disable it during
initialization
>
>> +}
>> +
>> +/**
>> + * spinand_write_enable - send command 06h to enable write or erase the
>> + * NAND cells
>> + * @spinand: SPI NAND device structure
>> + */
>> +static int spinand_write_enable(struct spinand_device *spinand)
>> +{
>> + struct spinand_op op;
>> +
>> + spinand_init_op(&op);
>> + op.cmd = SPINAND_CMD_WR_ENABLE;
>> +
>> + return spinand_exec_op(spinand, &op);
>> +}
>> +
>> +/**
>> + * spinand_read_page_to_cache - send command 13h to read data from NAND array
>> + * to cache
>> + * @spinand: SPI NAND device structure
>> + * @page_addr: page to read
>> + */
>> +static int spinand_read_page_to_cache(struct spinand_device *spinand,
>> + u32 page_addr)
>> +{
>> + struct spinand_op op;
>> +
>> + spinand_init_op(&op);
>> + op.cmd = SPINAND_CMD_PAGE_READ;
>> + op.n_addr = 3;
>> + op.addr[0] = (u8)(page_addr >> 16);
>> + op.addr[1] = (u8)(page_addr >> 8);
>> + op.addr[2] = (u8)page_addr;
>> +
>> + return spinand_exec_op(spinand, &op);
>> +}
>> +
>> +/**
>> + * spinand_get_address_bits - return address should be transferred
>> + * by how many bits
>> + * @opcode: command's operation code
>> + */
>> +static int spinand_get_address_bits(u8 opcode)
>> +{
>> + switch (opcode) {
>> + case SPINAND_CMD_READ_FROM_CACHE_QUAD_IO:
>> + return 4;
>> + case SPINAND_CMD_READ_FROM_CACHE_DUAL_IO:
>> + return 2;
>> + default:
>> + return 1;
>> + }
>> +}
>> +
>> +/**
>> + * spinand_get_data_bits - return data should be transferred by how many bits
>> + * @opcode: command's operation code
>> + */
>> +static int spinand_get_data_bits(u8 opcode)
>> +{
>> + switch (opcode) {
>> + case SPINAND_CMD_READ_FROM_CACHE_QUAD_IO:
>> + case SPINAND_CMD_READ_FROM_CACHE_X4:
>> + case SPINAND_CMD_PROG_LOAD_X4:
>> + case SPINAND_CMD_PROG_LOAD_RDM_DATA_X4:
>> + return 4;
>> + case SPINAND_CMD_READ_FROM_CACHE_DUAL_IO:
>> + case SPINAND_CMD_READ_FROM_CACHE_X2:
>> + return 2;
>> + default:
>> + return 1;
>> + }
>> +}
>> +
>> +/**
>> + * spinand_read_from_cache - read data out from cache register
>> + * @spinand: SPI NAND device structure
>> + * @page_addr: page to read
>> + * @column: the location to read from the cache
>> + * @len: number of bytes to read
>> + * @rbuf: buffer held @len bytes
>> + */
>> +static int spinand_read_from_cache(struct spinand_device *spinand,
>> + u32 page_addr, u32 column,
>> + size_t len, u8 *rbuf)
>> +{
>> + struct spinand_op op;
>> +
>> + spinand_init_op(&op);
>> + op.cmd = spinand->read_cache_op;
>> + op.n_addr = 2;
>> + op.addr[0] = (u8)(column >> 8);
>> + op.addr[1] = (u8)column;
>
> Casts are unneeded here.
Yes you are right
>
>> + op.addr_nbits = spinand_get_address_bits(spinand->read_cache_op);
>> + op.n_rx = len;
>> + op.rx_buf = rbuf;
>> + op.data_nbits = spinand_get_data_bits(spinand->read_cache_op);
>> +
>> + if (spinand->manufacturer.manu->ops->prepare_op)
>> + spinand->manufacturer.manu->ops->prepare_op(spinand, &op,
>> + page_addr, column);
>> +
>> + return spinand_exec_op(spinand, &op);
>> +}
>> +
>> +/**
>> + * spinand_write_to_cache - write data to cache register
>> + * @spinand: SPI NAND device structure
>> + * @page_addr: page to write
>> + * @column: the location to write to the cache
>> + * @len: number of bytes to write
>> + * @wrbuf: buffer held @len bytes
>> + */
>> +static int spinand_write_to_cache(struct spinand_device *spinand, u32 page_addr,
>> + u32 column, size_t len, const u8 *wbuf)
>> +{
>> + struct spinand_op op;
>> +
>> + spinand_init_op(&op);
>> + op.cmd = spinand->write_cache_op;
>> + op.n_addr = 2;
>> + op.addr[0] = (u8)(column >> 8);
>> + op.addr[1] = (u8)column;
>> + op.addr_nbits = spinand_get_address_bits(spinand->write_cache_op);
>> + op.n_tx = len;
>> + op.tx_buf = wbuf;
>> + op.data_nbits = spinand_get_data_bits(spinand->write_cache_op);
>> +
>> + if (spinand->manufacturer.manu->ops->prepare_op)
>> + spinand->manufacturer.manu->ops->prepare_op(spinand, &op,
>> + page_addr, column);
>> +
>> + return spinand_exec_op(spinand, &op);
>> +}
>> +
>> +/**
>> + * spinand_program_execute - send command 10h to write a page from
>> + * cache to the NAND array
>> + * @spinand: SPI NAND device structure
>> + * @page_addr: the physical page location to write the page.
>> + */
>> +static int spinand_program_execute(struct spinand_device *spinand,
>> + u32 page_addr)
>> +{
>> + struct spinand_op op;
>> +
>> + spinand_init_op(&op);
>> + op.cmd = SPINAND_CMD_PROG_EXC;
>> + op.n_addr = 3;
>> + op.addr[0] = (u8)(page_addr >> 16);
>> + op.addr[1] = (u8)(page_addr >> 8);
>> + op.addr[2] = (u8)page_addr;
>
> You don't need to adjust the number of dummy cycles here?
>
> The usage of ->prepare_op() is really weird, maybe you should document
> a bit more when it's supposed to be used.
>
>> +
>> + return spinand_exec_op(spinand, &op);
>> +}
>> +
>
> [...]
>
>>
>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>> index dd9da71..04ad1dd 100644
>> --- a/include/linux/mtd/spinand.h
>> +++ b/include/linux/mtd/spinand.h
>> @@ -103,11 +103,14 @@ struct spinand_controller_ops {
>> * return directly and let others to detect.
>> * @init: initialize SPI NAND device.
>> * @cleanup: clean SPI NAND device footprint.
>> + * @prepare_op: prepara read/write operation.
>
> ^ prepare
>
>
>
>> */
>> struct spinand_manufacturer_ops {
>> bool (*detect)(struct spinand_device *spinand);
>> int (*init)(struct spinand_device *spinand);
>> void (*cleanup)(struct spinand_device *spinand);
>> + void (*prepare_op)(struct spinand_device *spinand,
>> + struct spinand_op *op, u32 page, u32 column);
>
> It seems to be here to prepare read/write page operations, so I'd like
> to rename this method ->prepare_page_op() if you don't mind.
I'm ok with the new name
Thanks
Peter Pan
>
>> };
>>
>> /**
>
More information about the linux-mtd
mailing list