diff mbox series

[v3,05/17] mtd: spinand: Define ctrl_ops for non-page read/write op templates

Message ID 20220101074250.14443-6-a-nandan@ti.com (mailing list archive)
State New, archived
Headers show
Series mtd: spinand: Add Octal DTR SPI (8D-8D-8D) mode support | expand

Commit Message

Apurva Nandan Jan. 1, 2022, 7:42 a.m. UTC
'ctrl_ops' are op templates for non-page read/write operations,
which are: reset, get_feature, set_feature, write_enable, block_erase,
page_read and program_execute ops. The 'ctrl_ops' struct contains in it
op templates for each of this op, as well as enum spinand_protocol
denoting protocol of all these ops.

We require these new op templates because of deviation in standard
SPINAND ops by manufacturers and also due to changes when there is a
change in SPI protocol/mode. This prevents the core from live-patching
and vendor-specific adjustments in ops.

Define 'ctrl_ops', add macros to initialize it and add it in
spinand_device.

Signed-off-by: Apurva Nandan <a-nandan@ti.com>
---
 include/linux/mtd/spinand.h | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Boris Brezillon Jan. 3, 2022, 10:01 a.m. UTC | #1
On Sat, 1 Jan 2022 13:12:38 +0530
Apurva Nandan <a-nandan@ti.com> wrote:

> 'ctrl_ops' are op templates for non-page read/write operations,
> which are: reset, get_feature, set_feature, write_enable, block_erase,
> page_read and program_execute ops. The 'ctrl_ops' struct contains in it
> op templates for each of this op, as well as enum spinand_protocol
> denoting protocol of all these ops.
> 
> We require these new op templates because of deviation in standard
> SPINAND ops by manufacturers and also due to changes when there is a
> change in SPI protocol/mode. This prevents the core from live-patching
> and vendor-specific adjustments in ops.
> 
> Define 'ctrl_ops', add macros to initialize it and add it in
> spinand_device.
> 
> Signed-off-by: Apurva Nandan <a-nandan@ti.com>
> ---
>  include/linux/mtd/spinand.h | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 439d8ce40e1d..e5df6220ec1e 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -356,6 +356,35 @@ struct spinand_op_variants {
>  			sizeof(struct spi_mem_op),			\
>  	}
>  
> +struct spinand_ctrl_ops {
> +	const struct {
> +		struct spi_mem_op reset;
> +		struct spi_mem_op get_feature;
> +		struct spi_mem_op set_feature;
> +		struct spi_mem_op write_enable;
> +		struct spi_mem_op block_erase;
> +		struct spi_mem_op page_read;
> +		struct spi_mem_op program_execute;
> +	} ops;
> +	const enum spinand_protocol protocol;

Do you really need that protocol field?

> +};
> +
> +#define SPINAND_CTRL_OPS(__protocol, __reset, __get_feature, __set_feature,	\
> +			 __write_enable, __block_erase, __page_read,		\
> +			 __program_execute)					\
> +	{									\
> +		.ops = {							\
> +			.reset = __reset,					\
> +			.get_feature = __get_feature,				\
> +			.set_feature = __set_feature,				\
> +			.write_enable = __write_enable,				\
> +			.block_erase = __block_erase,				\
> +			.page_read = __page_read,				\
> +			.program_execute = __program_execute,			\
> +		},								\
> +		.protocol = __protocol,						\
> +	}
> +
>  /**
>   * spinand_ecc_info - description of the on-die ECC implemented by a SPI NAND
>   *		      chip
> @@ -468,6 +497,8 @@ struct spinand_dirmap {
>   * @data_ops.read_cache: read cache op template
>   * @data_ops.write_cache: write cache op template
>   * @data_ops.update_cache: update cache op template
> + * @ctrl_ops: various SPI mem op templates for handling the flash device, i.e.
> + *	      non page-read/write ops.
>   * @select_target: select a specific target/die. Usually called before sending
>   *		   a command addressing a page or an eraseblock embedded in
>   *		   this die. Only required if your chip exposes several dies
> @@ -498,6 +529,8 @@ struct spinand_device {
>  		const struct spi_mem_op *update_cache;
>  	} data_ops;
>  
> +	const struct spinand_ctrl_ops *ctrl_ops;
> +

Okay, I had something slightly different in mind. First, I'd put all
templates in a struct:

struct spinand_op_templates {
	const struct spi_mem_op *read_cache;
	const struct spi_mem_op *write_cache;
	const struct spi_mem_op *update_cache;
	const struct spi_mem_op *reset;
	const struct spi_mem_op *get_feature;
	const struct spi_mem_op *set_feature;
	const struct spi_mem_op *write_enable;
	const struct spi_mem_op *block_erase;
	const struct spi_mem_op *page_load;
	const struct spi_mem_op *program_execute;
};

Then, at the spinand level, I'd define an array of templates:

enum spinand_protocol {
	SPINAND_1S_1S_1S,
	SPINAND_2S_2S_2S,
	SPINAND_4S_4S_4S,
	SPINAND_8S_8S_8S,
	SPINAND_8D_8D_8D,
	SPINAND_NUM_PROTOCOLS,
};

struct spinand_device {
	...
	enum spinand_protocol protocol;
	const struct spinand_op_templates *op_templates[SPINAND_NUM_PROTOCOLS];
	...
};

This way, you can easily pick the right set of operations based
on the protocol/mode you're in:

#define spinand_get_op_template(spinand, opname) \
	((spinand)->op_templates[(spinand)->protocol]->opname)

static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
{
	struct spi_mem_op op = *spinand_get_op_template(spinand, get_feature);
	int ret;

	...
}
Boris Brezillon Jan. 3, 2022, 10:36 a.m. UTC | #2
On Mon, 3 Jan 2022 11:01:07 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:


> struct spinand_device {
> 	...
> 	enum spinand_protocol protocol;
> 	const struct spinand_op_templates *op_templates[SPINAND_NUM_PROTOCOLS];

My bad, it should be:

	struct spinand_op_templates op_templates[SPINAND_NUM_PROTOCOLS];

since those templates get populated dynamically at probe time based on
what the flash and the controller support.

> 	...
> };
>
Apurva Nandan Feb. 15, 2022, 3:33 p.m. UTC | #3
Hi Boris,

On 03/01/22 15:31, Boris Brezillon wrote:
> On Sat, 1 Jan 2022 13:12:38 +0530
> Apurva Nandan <a-nandan@ti.com> wrote:
>
>> 'ctrl_ops' are op templates for non-page read/write operations,
>> which are: reset, get_feature, set_feature, write_enable, block_erase,
>> page_read and program_execute ops. The 'ctrl_ops' struct contains in it
>> op templates for each of this op, as well as enum spinand_protocol
>> denoting protocol of all these ops.
>>
>> We require these new op templates because of deviation in standard
>> SPINAND ops by manufacturers and also due to changes when there is a
>> change in SPI protocol/mode. This prevents the core from live-patching
>> and vendor-specific adjustments in ops.
>>
>> Define 'ctrl_ops', add macros to initialize it and add it in
>> spinand_device.
>>
>> Signed-off-by: Apurva Nandan <a-nandan@ti.com>
>> ---
>>   include/linux/mtd/spinand.h | 33 +++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>> index 439d8ce40e1d..e5df6220ec1e 100644
>> --- a/include/linux/mtd/spinand.h
>> +++ b/include/linux/mtd/spinand.h
>> @@ -356,6 +356,35 @@ struct spinand_op_variants {
>>   			sizeof(struct spi_mem_op),			\
>>   	}
>>   
>> +struct spinand_ctrl_ops {
>> +	const struct {
>> +		struct spi_mem_op reset;
>> +		struct spi_mem_op get_feature;
>> +		struct spi_mem_op set_feature;
>> +		struct spi_mem_op write_enable;
>> +		struct spi_mem_op block_erase;
>> +		struct spi_mem_op page_read;
>> +		struct spi_mem_op program_execute;
>> +	} ops;
>> +	const enum spinand_protocol protocol;
> Do you really need that protocol field?
>
>> +};
>> +
>> +#define SPINAND_CTRL_OPS(__protocol, __reset, __get_feature, __set_feature,	\
>> +			 __write_enable, __block_erase, __page_read,		\
>> +			 __program_execute)					\
>> +	{									\
>> +		.ops = {							\
>> +			.reset = __reset,					\
>> +			.get_feature = __get_feature,				\
>> +			.set_feature = __set_feature,				\
>> +			.write_enable = __write_enable,				\
>> +			.block_erase = __block_erase,				\
>> +			.page_read = __page_read,				\
>> +			.program_execute = __program_execute,			\
>> +		},								\
>> +		.protocol = __protocol,						\
>> +	}
>> +
>>   /**
>>    * spinand_ecc_info - description of the on-die ECC implemented by a SPI NAND
>>    *		      chip
>> @@ -468,6 +497,8 @@ struct spinand_dirmap {
>>    * @data_ops.read_cache: read cache op template
>>    * @data_ops.write_cache: write cache op template
>>    * @data_ops.update_cache: update cache op template
>> + * @ctrl_ops: various SPI mem op templates for handling the flash device, i.e.
>> + *	      non page-read/write ops.
>>    * @select_target: select a specific target/die. Usually called before sending
>>    *		   a command addressing a page or an eraseblock embedded in
>>    *		   this die. Only required if your chip exposes several dies
>> @@ -498,6 +529,8 @@ struct spinand_device {
>>   		const struct spi_mem_op *update_cache;
>>   	} data_ops;
>>   
>> +	const struct spinand_ctrl_ops *ctrl_ops;
>> +
> Okay, I had something slightly different in mind. First, I'd put all
> templates in a struct:
>
> struct spinand_op_templates {
> 	const struct spi_mem_op *read_cache;
> 	const struct spi_mem_op *write_cache;
> 	const struct spi_mem_op *update_cache;
> 	const struct spi_mem_op *reset;
> 	const struct spi_mem_op *get_feature;
> 	const struct spi_mem_op *set_feature;
> 	const struct spi_mem_op *write_enable;
> 	const struct spi_mem_op *block_erase;
> 	const struct spi_mem_op *page_load;
> 	const struct spi_mem_op *program_execute;
> };
>
> Then, at the spinand level, I'd define an array of templates:
>
> enum spinand_protocol {
> 	SPINAND_1S_1S_1S,
> 	SPINAND_2S_2S_2S,
> 	SPINAND_4S_4S_4S,
> 	SPINAND_8S_8S_8S,
> 	SPINAND_8D_8D_8D,
> 	SPINAND_NUM_PROTOCOLS,
> };
>
> struct spinand_device {
> 	...
> 	enum spinand_protocol protocol;
> 	const struct spinand_op_templates *op_templates[SPINAND_NUM_PROTOCOLS];
> 	...
> };
>
> This way, you can easily pick the right set of operations based
> on the protocol/mode you're in:
>
> #define spinand_get_op_template(spinand, opname) \
> 	((spinand)->op_templates[(spinand)->protocol]->opname)
>
> static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
> {
> 	struct spi_mem_op op = *spinand_get_op_template(spinand, get_feature);
> 	int ret;
>
> 	...
> }

I find a couple of issues with this  method,

1. read_cache, write_cache, update_cache op templates don't fit well 
with the other non-data ops, as
these data ops are used to create a dirmap, and that can be done only 
once at probe time. Hence, there
is a different mechanism of selecting of data ops and non-data ops. 
Hence, this division in the op templates
struct as data_ops and ctrl_ops is required. Currently, the core only 
supports using a single protocol for
data ops, chosen at the time of probing.

2. If we use this single op_templates struct, I can't think of any good 
way to initialize these in the
manufacturers driver (winbond.c), refer to 17th patch in this series. 
Could you please suggest a macro
implementation also for winbond.c with the suggested op_templates struct.

Thanks,
Apurva Nandan
Boris Brezillon Feb. 15, 2022, 5:37 p.m. UTC | #4
Hi Apurva,

On Tue, 15 Feb 2022 21:03:52 +0530
Apurva Nandan <a-nandan@ti.com> wrote:

> Hi Boris,
> 
> On 03/01/22 15:31, Boris Brezillon wrote:
> > On Sat, 1 Jan 2022 13:12:38 +0530
> > Apurva Nandan <a-nandan@ti.com> wrote:
> >  
> >> 'ctrl_ops' are op templates for non-page read/write operations,
> >> which are: reset, get_feature, set_feature, write_enable, block_erase,
> >> page_read and program_execute ops. The 'ctrl_ops' struct contains in it
> >> op templates for each of this op, as well as enum spinand_protocol
> >> denoting protocol of all these ops.
> >>
> >> We require these new op templates because of deviation in standard
> >> SPINAND ops by manufacturers and also due to changes when there is a
> >> change in SPI protocol/mode. This prevents the core from live-patching
> >> and vendor-specific adjustments in ops.
> >>
> >> Define 'ctrl_ops', add macros to initialize it and add it in
> >> spinand_device.
> >>
> >> Signed-off-by: Apurva Nandan <a-nandan@ti.com>
> >> ---
> >>   include/linux/mtd/spinand.h | 33 +++++++++++++++++++++++++++++++++
> >>   1 file changed, 33 insertions(+)
> >>
> >> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> >> index 439d8ce40e1d..e5df6220ec1e 100644
> >> --- a/include/linux/mtd/spinand.h
> >> +++ b/include/linux/mtd/spinand.h
> >> @@ -356,6 +356,35 @@ struct spinand_op_variants {
> >>   			sizeof(struct spi_mem_op),			\
> >>   	}
> >>   
> >> +struct spinand_ctrl_ops {
> >> +	const struct {
> >> +		struct spi_mem_op reset;
> >> +		struct spi_mem_op get_feature;
> >> +		struct spi_mem_op set_feature;
> >> +		struct spi_mem_op write_enable;
> >> +		struct spi_mem_op block_erase;
> >> +		struct spi_mem_op page_read;
> >> +		struct spi_mem_op program_execute;
> >> +	} ops;
> >> +	const enum spinand_protocol protocol;  
> > Do you really need that protocol field?
> >  
> >> +};
> >> +
> >> +#define SPINAND_CTRL_OPS(__protocol, __reset, __get_feature, __set_feature,	\
> >> +			 __write_enable, __block_erase, __page_read,		\
> >> +			 __program_execute)					\
> >> +	{									\
> >> +		.ops = {							\
> >> +			.reset = __reset,					\
> >> +			.get_feature = __get_feature,				\
> >> +			.set_feature = __set_feature,				\
> >> +			.write_enable = __write_enable,				\
> >> +			.block_erase = __block_erase,				\
> >> +			.page_read = __page_read,				\
> >> +			.program_execute = __program_execute,			\
> >> +		},								\
> >> +		.protocol = __protocol,						\
> >> +	}
> >> +
> >>   /**
> >>    * spinand_ecc_info - description of the on-die ECC implemented by a SPI NAND
> >>    *		      chip
> >> @@ -468,6 +497,8 @@ struct spinand_dirmap {
> >>    * @data_ops.read_cache: read cache op template
> >>    * @data_ops.write_cache: write cache op template
> >>    * @data_ops.update_cache: update cache op template
> >> + * @ctrl_ops: various SPI mem op templates for handling the flash device, i.e.
> >> + *	      non page-read/write ops.
> >>    * @select_target: select a specific target/die. Usually called before sending
> >>    *		   a command addressing a page or an eraseblock embedded in
> >>    *		   this die. Only required if your chip exposes several dies
> >> @@ -498,6 +529,8 @@ struct spinand_device {
> >>   		const struct spi_mem_op *update_cache;
> >>   	} data_ops;
> >>   
> >> +	const struct spinand_ctrl_ops *ctrl_ops;
> >> +  
> > Okay, I had something slightly different in mind. First, I'd put all
> > templates in a struct:
> >
> > struct spinand_op_templates {
> > 	const struct spi_mem_op *read_cache;
> > 	const struct spi_mem_op *write_cache;
> > 	const struct spi_mem_op *update_cache;
> > 	const struct spi_mem_op *reset;
> > 	const struct spi_mem_op *get_feature;
> > 	const struct spi_mem_op *set_feature;
> > 	const struct spi_mem_op *write_enable;
> > 	const struct spi_mem_op *block_erase;
> > 	const struct spi_mem_op *page_load;
> > 	const struct spi_mem_op *program_execute;
> > };
> >
> > Then, at the spinand level, I'd define an array of templates:
> >
> > enum spinand_protocol {
> > 	SPINAND_1S_1S_1S,
> > 	SPINAND_2S_2S_2S,
> > 	SPINAND_4S_4S_4S,
> > 	SPINAND_8S_8S_8S,
> > 	SPINAND_8D_8D_8D,
> > 	SPINAND_NUM_PROTOCOLS,
> > };
> >
> > struct spinand_device {
> > 	...
> > 	enum spinand_protocol protocol;
> > 	const struct spinand_op_templates *op_templates[SPINAND_NUM_PROTOCOLS];

It should probably be

	struct spinand_op_templates op_templates[SPINAND_NUM_PROTOCOLS];

with the spinand_op_templates struct defined as:

struct spinand_op_templates {
 	struct spi_mem_op read_cache;
 	struct spi_mem_op write_cache;
 	struct spi_mem_op update_cache;
 	struct spi_mem_op reset;
 	struct spi_mem_op get_feature;
 	struct spi_mem_op set_feature;
 	struct spi_mem_op write_enable;
 	struct spi_mem_op block_erase;
 	struct spi_mem_op page_load;
 	struct spi_mem_op program_execute;
};

so the NAND framework can populate these ops.

Or maybe even better, define an enum that contains all the ops:

enum spinand_op_id {
	SPI_NAND_OP_READ_CACHE,
	SPI_NAND_OP_WRITE_CACHE,
	SPI_NAND_OP_UPDATE_CACHE,
	SPI_NAND_OP_RESET,
...
	SPI_NAND_NUM_OPS,
};

struct spinand_device {
	...
 	enum spinand_protocol protocol;
 	struct spi_mem_op op_templates[SPINAND_NUM_PROTOCOLS][SPI_NAND_NUM_OPS];
	...
};

> >
> > This way, you can easily pick the right set of operations based
> > on the protocol/mode you're in:
> >
> > #define spinand_get_op_template(spinand, opname) \
> > 	((spinand)->op_templates[(spinand)->protocol]->opname)
> >
> > static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
> > {
> > 	struct spi_mem_op op = *spinand_get_op_template(spinand, get_feature);
> > 	int ret;
> >
> > 	...
> > }  
> 
> I find a couple of issues with this  method,
> 
> 1. read_cache, write_cache, update_cache op templates don't fit well 
> with the other non-data ops, as
> these data ops are used to create a dirmap, and that can be done only 
> once at probe time. Hence, there
> is a different mechanism of selecting of data ops and non-data ops. 

Not sure I see why this is a problem. You can populate data-ops for all
modes, and pick the one that provides the best perfs when you create
the dirmap (which should really be at the end of the probe, if it's not
already).

> Hence, this division in the op templates
> struct as data_ops and ctrl_ops is required. Currently, the core only 
> supports using a single protocol for
> data ops, chosen at the time of probing.

Again, I don't see why you need to differentiate the control and data
ops when populating this table. Those are just operations the NAND
supports, and the data operations is just a subset.

> 
> 2. If we use this single op_templates struct, I can't think of any good 
> way to initialize these in the
> manufacturers driver (winbond.c), refer to 17th patch in this series. 
> Could you please suggest a macro
> implementation also for winbond.c with the suggested op_templates struct.

First replace the op_variants field by something more generic:

struct spinand_info {
...
	const struct spinand_op_variants **ops_variants;
...
};

#define SPINAND_OP_VARIANTS(_id, ...) \
	[SPI_NAND_OP_ ## _id] = { __VA_ARGS__ }

#define SPINAND_OPS_VARIANTS(name, ...)
	const struct spinand_op_variants name[]{
		__VA_ARGS__,
	};

#define SPINAND_INFO_OPS_VARIANTS(defs)
	.ops_variants = defs

...

static SPINAND_OPS_VARIANTS(w35n01jw_ops_variants,
		SPINAND_OP_VARIANTS(READ_CACHE,
			SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(0, 24, NULL, 0),
			SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
			...)),
		SPINAND_OP_VARIANTS(WRITE_CACHE,
			SPINAND_PROG_LOAD_OCTALIO_DTR(true, 0, NULL, 0),
			SPINAND_PROG_LOAD(true, 0, NULL, 0)),
		...
		SPINAND_OP_VARIANTS(RESET,
			SPINAND_RESET_OP_OCTAL_DTR,
			SPINAND_RESET_OP,
		...
		);
...


	SPINAND_INFO("W35N01JW",
		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xdc),
		     NAND_MEMORG(1, 4096, 128, 64, 512, 20, 1, 1, 1),
		     NAND_ECCREQ(1, 512),
		     SPINAND_HAS_OCTAL_DTR_BIT | SPINAND_HAS_CR_FEAT_BIT,
		     SPINAND_ECCINFO(&w35n01jw_ooblayout, NULL),
		     SPINAND_INFO_OPS_VARIANTS(&w35n01jw_ops_variants)),

You also need to adjust spinand_match_and_init() to account for this
new layout and put each template op in the right subset based on
op.cmd.width and op.cmd.dtr.

Regards,

Boris
Apurva Nandan March 2, 2022, 3:30 p.m. UTC | #5
Hi Boris,

On 15/02/22 23:07, Boris Brezillon wrote:
> Hi Apurva,
>
> On Tue, 15 Feb 2022 21:03:52 +0530
> Apurva Nandan<a-nandan@ti.com>  wrote:
>
>> Hi Boris,
>>
>> On 03/01/22 15:31, Boris Brezillon wrote:
>>> On Sat, 1 Jan 2022 13:12:38 +0530
>>> Apurva Nandan<a-nandan@ti.com>  wrote:
>>>   
>>>> 'ctrl_ops' are op templates for non-page read/write operations,
>>>> which are: reset, get_feature, set_feature, write_enable, block_erase,
>>>> page_read and program_execute ops. The 'ctrl_ops' struct contains in it
>>>> op templates for each of this op, as well as enum spinand_protocol
>>>> denoting protocol of all these ops.
>>>>
>>>> We require these new op templates because of deviation in standard
>>>> SPINAND ops by manufacturers and also due to changes when there is a
>>>> change in SPI protocol/mode. This prevents the core from live-patching
>>>> and vendor-specific adjustments in ops.
>>>>
>>>> Define 'ctrl_ops', add macros to initialize it and add it in
>>>> spinand_device.
>>>>
>>>> Signed-off-by: Apurva Nandan<a-nandan@ti.com>
>>>> ---
>>>>    include/linux/mtd/spinand.h | 33 +++++++++++++++++++++++++++++++++
>>>>    1 file changed, 33 insertions(+)
>>>>
>>>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>>>> index 439d8ce40e1d..e5df6220ec1e 100644
>>>> --- a/include/linux/mtd/spinand.h
>>>> +++ b/include/linux/mtd/spinand.h
>>>> @@ -356,6 +356,35 @@ struct spinand_op_variants {
>>>>    			sizeof(struct spi_mem_op),			\
>>>>    	}
>>>>    
>>>> +struct spinand_ctrl_ops {
>>>> +	const struct {
>>>> +		struct spi_mem_op reset;
>>>> +		struct spi_mem_op get_feature;
>>>> +		struct spi_mem_op set_feature;
>>>> +		struct spi_mem_op write_enable;
>>>> +		struct spi_mem_op block_erase;
>>>> +		struct spi_mem_op page_read;
>>>> +		struct spi_mem_op program_execute;
>>>> +	} ops;
>>>> +	const enum spinand_protocol protocol;
>>> Do you really need that protocol field?
>>>   
>>>> +};
>>>> +
>>>> +#define SPINAND_CTRL_OPS(__protocol, __reset, __get_feature, __set_feature,	\
>>>> +			 __write_enable, __block_erase, __page_read,		\
>>>> +			 __program_execute)					\
>>>> +	{									\
>>>> +		.ops = {							\
>>>> +			.reset = __reset,					\
>>>> +			.get_feature = __get_feature,				\
>>>> +			.set_feature = __set_feature,				\
>>>> +			.write_enable = __write_enable,				\
>>>> +			.block_erase = __block_erase,				\
>>>> +			.page_read = __page_read,				\
>>>> +			.program_execute = __program_execute,			\
>>>> +		},								\
>>>> +		.protocol = __protocol,						\
>>>> +	}
>>>> +
>>>>    /**
>>>>     * spinand_ecc_info - description of the on-die ECC implemented by a SPI NAND
>>>>     *		      chip
>>>> @@ -468,6 +497,8 @@ struct spinand_dirmap {
>>>>     * @data_ops.read_cache: read cache op template
>>>>     * @data_ops.write_cache: write cache op template
>>>>     * @data_ops.update_cache: update cache op template
>>>> + * @ctrl_ops: various SPI mem op templates for handling the flash device, i.e.
>>>> + *	      non page-read/write ops.
>>>>     * @select_target: select a specific target/die. Usually called before sending
>>>>     *		   a command addressing a page or an eraseblock embedded in
>>>>     *		   this die. Only required if your chip exposes several dies
>>>> @@ -498,6 +529,8 @@ struct spinand_device {
>>>>    		const struct spi_mem_op *update_cache;
>>>>    	} data_ops;
>>>>    
>>>> +	const struct spinand_ctrl_ops *ctrl_ops;
>>>> +
>>> Okay, I had something slightly different in mind. First, I'd put all
>>> templates in a struct:
>>>
>>> struct spinand_op_templates {
>>> 	const struct spi_mem_op *read_cache;
>>> 	const struct spi_mem_op *write_cache;
>>> 	const struct spi_mem_op *update_cache;
>>> 	const struct spi_mem_op *reset;
>>> 	const struct spi_mem_op *get_feature;
>>> 	const struct spi_mem_op *set_feature;
>>> 	const struct spi_mem_op *write_enable;
>>> 	const struct spi_mem_op *block_erase;
>>> 	const struct spi_mem_op *page_load;
>>> 	const struct spi_mem_op *program_execute;
>>> };
>>>
>>> Then, at the spinand level, I'd define an array of templates:
>>>
>>> enum spinand_protocol {
>>> 	SPINAND_1S_1S_1S,
>>> 	SPINAND_2S_2S_2S,
>>> 	SPINAND_4S_4S_4S,
>>> 	SPINAND_8S_8S_8S,
>>> 	SPINAND_8D_8D_8D,
>>> 	SPINAND_NUM_PROTOCOLS,
>>> };
>>>
>>> struct spinand_device {
>>> 	...
>>> 	enum spinand_protocol protocol;
>>> 	const struct spinand_op_templates *op_templates[SPINAND_NUM_PROTOCOLS];
> It should probably be
>
> 	struct spinand_op_templates op_templates[SPINAND_NUM_PROTOCOLS];
>
> with the spinand_op_templates struct defined as:
>
> struct spinand_op_templates {
>   	struct spi_mem_op read_cache;
>   	struct spi_mem_op write_cache;
>   	struct spi_mem_op update_cache;
>   	struct spi_mem_op reset;
>   	struct spi_mem_op get_feature;
>   	struct spi_mem_op set_feature;
>   	struct spi_mem_op write_enable;
>   	struct spi_mem_op block_erase;
>   	struct spi_mem_op page_load;
>   	struct spi_mem_op program_execute;
> };
>
> so the NAND framework can populate these ops.
>
> Or maybe even better, define an enum that contains all the ops:
>
> enum spinand_op_id {
> 	SPI_NAND_OP_READ_CACHE,
> 	SPI_NAND_OP_WRITE_CACHE,
> 	SPI_NAND_OP_UPDATE_CACHE,
> 	SPI_NAND_OP_RESET,
> ...
> 	SPI_NAND_NUM_OPS,
> };
>
> struct spinand_device {
> 	...
>   	enum spinand_protocol protocol;
>   	struct spi_mem_op op_templates[SPINAND_NUM_PROTOCOLS][SPI_NAND_NUM_OPS];
> 	...
> };
>
>>> This way, you can easily pick the right set of operations based
>>> on the protocol/mode you're in:
>>>
>>> #define spinand_get_op_template(spinand, opname) \
>>> 	((spinand)->op_templates[(spinand)->protocol]->opname)
>>>
>>> static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
>>> {
>>> 	struct spi_mem_op op = *spinand_get_op_template(spinand, get_feature);
>>> 	int ret;
>>>
>>> 	...
>>> }
>> I find a couple of issues with this  method,
>>
>> 1. read_cache, write_cache, update_cache op templates don't fit well
>> with the other non-data ops, as
>> these data ops are used to create a dirmap, and that can be done only
>> once at probe time. Hence, there
>> is a different mechanism of selecting of data ops and non-data ops.
> Not sure I see why this is a problem. You can populate data-ops for all
> modes, and pick the one that provides the best perfs when you create
> the dirmap (which should really be at the end of the probe, if it's not
> already).
>
>> Hence, this division in the op templates
>> struct as data_ops and ctrl_ops is required. Currently, the core only
>> supports using a single protocol for
>> data ops, chosen at the time of probing.
> Again, I don't see why you need to differentiate the control and data
> ops when populating this table. Those are just operations the NAND
> supports, and the data operations is just a subset.
>
>> 2. If we use this single op_templates struct, I can't think of any good
>> way to initialize these in the
>> manufacturers driver (winbond.c), refer to 17th patch in this series.
>> Could you please suggest a macro
>> implementation also for winbond.c with the suggested op_templates struct.
> First replace the op_variants field by something more generic:
>
> struct spinand_info {
> ...
> 	const struct spinand_op_variants **ops_variants;
> ...
> };
>
> #define SPINAND_OP_VARIANTS(_id, ...) \
> 	[SPI_NAND_OP_ ## _id] = { __VA_ARGS__ }
>
> #define SPINAND_OPS_VARIANTS(name, ...)
> 	const struct spinand_op_variants name[]{
> 		__VA_ARGS__,
> 	};
>
> #define SPINAND_INFO_OPS_VARIANTS(defs)
> 	.ops_variants = defs
>
> ...
>
> static SPINAND_OPS_VARIANTS(w35n01jw_ops_variants,
> 		SPINAND_OP_VARIANTS(READ_CACHE,
> 			SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(0, 24, NULL, 0),
> 			SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
> 			...)),
> 		SPINAND_OP_VARIANTS(WRITE_CACHE,
> 			SPINAND_PROG_LOAD_OCTALIO_DTR(true, 0, NULL, 0),
> 			SPINAND_PROG_LOAD(true, 0, NULL, 0)),
> 		...
> 		SPINAND_OP_VARIANTS(RESET,
> 			SPINAND_RESET_OP_OCTAL_DTR,
> 			SPINAND_RESET_OP,
> 		...
> 		);
> ...

I find a issue with this implementation, please give corrective suggestions:

In type of op variant listing, there is no way to specify the protocol 
of the op in the variants struct itself.
     - This will lead to filtering/sorting/searching of ops for finding 
the protocols in the spinand core
     while in spinand_match_and_init(), which I don't feel is a good way 
for protocol based op categorization.
     - This would also lead to complexities in cases of mixed mode 
operations.
     - In addition, we can't simply choose the first supported protocol 
in each op id, as some ops have
     intendependency of protocol with other ops. This is because 
non-data ops (like reset, block erase..)
     cannot be in different protocols at same time, so it would make 
sense to have some form of protocol
     based arrangement while listing them.

> 	SPINAND_INFO("W35N01JW",
> 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xdc),
> 		     NAND_MEMORG(1, 4096, 128, 64, 512, 20, 1, 1, 1),
> 		     NAND_ECCREQ(1, 512),
> 		     SPINAND_HAS_OCTAL_DTR_BIT | SPINAND_HAS_CR_FEAT_BIT,
> 		     SPINAND_ECCINFO(&w35n01jw_ooblayout, NULL),
> 		     SPINAND_INFO_OPS_VARIANTS(&w35n01jw_ops_variants)),
>
> You also need to adjust spinand_match_and_init() to account for this
> new layout and put each template op in the right subset based on
> op.cmd.width and op.cmd.dtr.
>
> Regards,
>
> Boris
Boris Brezillon March 2, 2022, 8:05 p.m. UTC | #6
On Wed, 2 Mar 2022 21:00:55 +0530
Apurva Nandan <a-nandan@ti.com> wrote:

> >> 1. read_cache, write_cache, update_cache op templates don't fit well
> >> with the other non-data ops, as
> >> these data ops are used to create a dirmap, and that can be done only
> >> once at probe time. Hence, there
> >> is a different mechanism of selecting of data ops and non-data ops.  
> > Not sure I see why this is a problem. You can populate data-ops for all
> > modes, and pick the one that provides the best perfs when you create
> > the dirmap (which should really be at the end of the probe, if it's not
> > already).
> >  
> >> Hence, this division in the op templates
> >> struct as data_ops and ctrl_ops is required. Currently, the core only
> >> supports using a single protocol for
> >> data ops, chosen at the time of probing.  
> > Again, I don't see why you need to differentiate the control and data
> > ops when populating this table. Those are just operations the NAND
> > supports, and the data operations is just a subset.
> >  
> >> 2. If we use this single op_templates struct, I can't think of any good
> >> way to initialize these in the
> >> manufacturers driver (winbond.c), refer to 17th patch in this series.
> >> Could you please suggest a macro
> >> implementation also for winbond.c with the suggested op_templates struct.  
> > First replace the op_variants field by something more generic:
> >
> > struct spinand_info {
> > ...
> > 	const struct spinand_op_variants **ops_variants;
> > ...
> > };
> >
> > #define SPINAND_OP_VARIANTS(_id, ...) \
> > 	[SPI_NAND_OP_ ## _id] = { __VA_ARGS__ }
> >
> > #define SPINAND_OPS_VARIANTS(name, ...)
> > 	const struct spinand_op_variants name[]{
> > 		__VA_ARGS__,
> > 	};
> >
> > #define SPINAND_INFO_OPS_VARIANTS(defs)
> > 	.ops_variants = defs
> >
> > ...
> >
> > static SPINAND_OPS_VARIANTS(w35n01jw_ops_variants,
> > 		SPINAND_OP_VARIANTS(READ_CACHE,
> > 			SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(0, 24, NULL, 0),
> > 			SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
> > 			...)),
> > 		SPINAND_OP_VARIANTS(WRITE_CACHE,
> > 			SPINAND_PROG_LOAD_OCTALIO_DTR(true, 0, NULL, 0),
> > 			SPINAND_PROG_LOAD(true, 0, NULL, 0)),
> > 		...
> > 		SPINAND_OP_VARIANTS(RESET,
> > 			SPINAND_RESET_OP_OCTAL_DTR,
> > 			SPINAND_RESET_OP,
> > 		...
> > 		);
> > ...  
> 
> I find a issue with this implementation, please give corrective suggestions:
> 
> In type of op variant listing, there is no way to specify the protocol 
> of the op in the variants struct itself.
>      - This will lead to filtering/sorting/searching of ops for finding 
> the protocols in the spinand core
>      while in spinand_match_and_init(), which I don't feel is a good way 
> for protocol based op categorization.

You'll have to go over all those operations to check which ones are
supported by the controller anyway. And it's not like the
classification is complicated since the cmd bus-width+DTR seems to be
the discriminant, and it's stored directly in the operation template.

>      - This would also lead to complexities in cases of mixed mode 
> operations.

Not sure what you mean by mixed mode. Are you referring to something
like 1S-8D-8D? IIUC, all we care about is the mode used for the cmd
cycle. I don't think there are commands to switch between stateless
(1S-x[S,D]-x[S,D]) modes (assuming 1S-xD-xD is a thing).

>      - In addition, we can't simply choose the first supported protocol 
> in each op id, as some ops have
>      intendependency of protocol with other ops. This is because 
> non-data ops (like reset, block erase..)
>      cannot be in different protocols at same time, so it would make 
> sense to have some form of protocol
>      based arrangement while listing them.

I'm not suggesting to choose the first supported operation and use it
unconditionally, but instead choose one operation per stateful mode
(1S, 2S, 4S, ..., 8D) and use the appropriate template depending on
the mode the flash is currently in.
Apurva Nandan March 10, 2022, 7:57 a.m. UTC | #7
Hi Boris,

On 15/02/22 23:07, Boris Brezillon wrote:
> Hi Apurva,
>
> On Tue, 15 Feb 2022 21:03:52 +0530
> Apurva Nandan <a-nandan@ti.com> wrote:
>
>> Hi Boris,
>>
>> On 03/01/22 15:31, Boris Brezillon wrote:
>>> On Sat, 1 Jan 2022 13:12:38 +0530
>>> Apurva Nandan <a-nandan@ti.com> wrote:
>>>   
>>>> 'ctrl_ops' are op templates for non-page read/write operations,
>>>> which are: reset, get_feature, set_feature, write_enable, block_erase,
>>>> page_read and program_execute ops. The 'ctrl_ops' struct contains in it
>>>> op templates for each of this op, as well as enum spinand_protocol
>>>> denoting protocol of all these ops.
>>>>
>>>> We require these new op templates because of deviation in standard
>>>> SPINAND ops by manufacturers and also due to changes when there is a
>>>> change in SPI protocol/mode. This prevents the core from live-patching
>>>> and vendor-specific adjustments in ops.
>>>>
>>>> Define 'ctrl_ops', add macros to initialize it and add it in
>>>> spinand_device.
>>>>
>>>> Signed-off-by: Apurva Nandan <a-nandan@ti.com>
>>>> ---
>>>>    include/linux/mtd/spinand.h | 33 +++++++++++++++++++++++++++++++++
>>>>    1 file changed, 33 insertions(+)
>>>>
>>>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>>>> index 439d8ce40e1d..e5df6220ec1e 100644
>>>> --- a/include/linux/mtd/spinand.h
>>>> +++ b/include/linux/mtd/spinand.h
>>>> @@ -356,6 +356,35 @@ struct spinand_op_variants {
>>>>    			sizeof(struct spi_mem_op),			\
>>>>    	}
>>>>    
>>>> +struct spinand_ctrl_ops {
>>>> +	const struct {
>>>> +		struct spi_mem_op reset;
>>>> +		struct spi_mem_op get_feature;
>>>> +		struct spi_mem_op set_feature;
>>>> +		struct spi_mem_op write_enable;
>>>> +		struct spi_mem_op block_erase;
>>>> +		struct spi_mem_op page_read;
>>>> +		struct spi_mem_op program_execute;
>>>> +	} ops;
>>>> +	const enum spinand_protocol protocol;
>>> Do you really need that protocol field?
>>>   
>>>> +};
>>>> +
>>>> +#define SPINAND_CTRL_OPS(__protocol, __reset, __get_feature, __set_feature,	\
>>>> +			 __write_enable, __block_erase, __page_read,		\
>>>> +			 __program_execute)					\
>>>> +	{									\
>>>> +		.ops = {							\
>>>> +			.reset = __reset,					\
>>>> +			.get_feature = __get_feature,				\
>>>> +			.set_feature = __set_feature,				\
>>>> +			.write_enable = __write_enable,				\
>>>> +			.block_erase = __block_erase,				\
>>>> +			.page_read = __page_read,				\
>>>> +			.program_execute = __program_execute,			\
>>>> +		},								\
>>>> +		.protocol = __protocol,						\
>>>> +	}
>>>> +
>>>>    /**
>>>>     * spinand_ecc_info - description of the on-die ECC implemented by a SPI NAND
>>>>     *		      chip
>>>> @@ -468,6 +497,8 @@ struct spinand_dirmap {
>>>>     * @data_ops.read_cache: read cache op template
>>>>     * @data_ops.write_cache: write cache op template
>>>>     * @data_ops.update_cache: update cache op template
>>>> + * @ctrl_ops: various SPI mem op templates for handling the flash device, i.e.
>>>> + *	      non page-read/write ops.
>>>>     * @select_target: select a specific target/die. Usually called before sending
>>>>     *		   a command addressing a page or an eraseblock embedded in
>>>>     *		   this die. Only required if your chip exposes several dies
>>>> @@ -498,6 +529,8 @@ struct spinand_device {
>>>>    		const struct spi_mem_op *update_cache;
>>>>    	} data_ops;
>>>>    
>>>> +	const struct spinand_ctrl_ops *ctrl_ops;
>>>> +
>>> Okay, I had something slightly different in mind. First, I'd put all
>>> templates in a struct:
>>>
>>> struct spinand_op_templates {
>>> 	const struct spi_mem_op *read_cache;
>>> 	const struct spi_mem_op *write_cache;
>>> 	const struct spi_mem_op *update_cache;
>>> 	const struct spi_mem_op *reset;
>>> 	const struct spi_mem_op *get_feature;
>>> 	const struct spi_mem_op *set_feature;
>>> 	const struct spi_mem_op *write_enable;
>>> 	const struct spi_mem_op *block_erase;
>>> 	const struct spi_mem_op *page_load;
>>> 	const struct spi_mem_op *program_execute;
>>> };
>>>
>>> Then, at the spinand level, I'd define an array of templates:
>>>
>>> enum spinand_protocol {
>>> 	SPINAND_1S_1S_1S,
>>> 	SPINAND_2S_2S_2S,
>>> 	SPINAND_4S_4S_4S,
>>> 	SPINAND_8S_8S_8S,
>>> 	SPINAND_8D_8D_8D,
>>> 	SPINAND_NUM_PROTOCOLS,
>>> };
>>>
>>> struct spinand_device {
>>> 	...
>>> 	enum spinand_protocol protocol;
>>> 	const struct spinand_op_templates *op_templates[SPINAND_NUM_PROTOCOLS];
> It should probably be
>
> 	struct spinand_op_templates op_templates[SPINAND_NUM_PROTOCOLS];
>
> with the spinand_op_templates struct defined as:
>
> struct spinand_op_templates {
>   	struct spi_mem_op read_cache;
>   	struct spi_mem_op write_cache;
>   	struct spi_mem_op update_cache;
>   	struct spi_mem_op reset;
>   	struct spi_mem_op get_feature;
>   	struct spi_mem_op set_feature;
>   	struct spi_mem_op write_enable;
>   	struct spi_mem_op block_erase;
>   	struct spi_mem_op page_load;
>   	struct spi_mem_op program_execute;
> };
>
> so the NAND framework can populate these ops.
>
> Or maybe even better, define an enum that contains all the ops:
>
> enum spinand_op_id {
> 	SPI_NAND_OP_READ_CACHE,
> 	SPI_NAND_OP_WRITE_CACHE,
> 	SPI_NAND_OP_UPDATE_CACHE,
> 	SPI_NAND_OP_RESET,
> ...
> 	SPI_NAND_NUM_OPS,
> };
>
> struct spinand_device {
> 	...
>   	enum spinand_protocol protocol;
>   	struct spi_mem_op op_templates[SPINAND_NUM_PROTOCOLS][SPI_NAND_NUM_OPS];
> 	...
> };
>
>>> This way, you can easily pick the right set of operations based
>>> on the protocol/mode you're in:
>>>
>>> #define spinand_get_op_template(spinand, opname) \
>>> 	((spinand)->op_templates[(spinand)->protocol]->opname)
>>>
>>> static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
>>> {
>>> 	struct spi_mem_op op = *spinand_get_op_template(spinand, get_feature);
>>> 	int ret;
>>>
>>> 	...
>>> }
>> I find a couple of issues with this  method,
>>
>> 1. read_cache, write_cache, update_cache op templates don't fit well
>> with the other non-data ops, as
>> these data ops are used to create a dirmap, and that can be done only
>> once at probe time. Hence, there
>> is a different mechanism of selecting of data ops and non-data ops.
> Not sure I see why this is a problem. You can populate data-ops for all
> modes, and pick the one that provides the best perfs when you create
> the dirmap (which should really be at the end of the probe, if it's not
> already).
>
>> Hence, this division in the op templates
>> struct as data_ops and ctrl_ops is required. Currently, the core only
>> supports using a single protocol for
>> data ops, chosen at the time of probing.
> Again, I don't see why you need to differentiate the control and data
> ops when populating this table. Those are just operations the NAND
> supports, and the data operations is just a subset.
>
>> 2. If we use this single op_templates struct, I can't think of any good
>> way to initialize these in the
>> manufacturers driver (winbond.c), refer to 17th patch in this series.
>> Could you please suggest a macro
>> implementation also for winbond.c with the suggested op_templates struct.
> First replace the op_variants field by something more generic:
>
> struct spinand_info {
> ...
> 	const struct spinand_op_variants **ops_variants;
> ...
> };
>
> #define SPINAND_OP_VARIANTS(_id, ...) \
> 	[SPI_NAND_OP_ ## _id] = { __VA_ARGS__ }
>
> #define SPINAND_OPS_VARIANTS(name, ...)
> 	const struct spinand_op_variants name[]{
> 		__VA_ARGS__,
> 	};
>
> #define SPINAND_INFO_OPS_VARIANTS(defs)
> 	.ops_variants = defs

If we modify these macros, it would require other spinand vendor drivers 
to change (toshiba, micron, etc).
The older macros suit them well, should we go about changing them to 
this new macro (will require re-testing all of them),
or can we keep them unchanged and have new set of macros with different 
name (please give suggestion for it) for op variants.

>
> ...
>
> static SPINAND_OPS_VARIANTS(w35n01jw_ops_variants,
> 		SPINAND_OP_VARIANTS(READ_CACHE,
> 			SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(0, 24, NULL, 0),
> 			SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
> 			...)),
> 		SPINAND_OP_VARIANTS(WRITE_CACHE,
> 			SPINAND_PROG_LOAD_OCTALIO_DTR(true, 0, NULL, 0),
> 			SPINAND_PROG_LOAD(true, 0, NULL, 0)),
> 		...
> 		SPINAND_OP_VARIANTS(RESET,
> 			SPINAND_RESET_OP_OCTAL_DTR,
> 			SPINAND_RESET_OP,
> 		...
> 		);
> ...
>
>
> 	SPINAND_INFO("W35N01JW",
> 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xdc),
> 		     NAND_MEMORG(1, 4096, 128, 64, 512, 20, 1, 1, 1),
> 		     NAND_ECCREQ(1, 512),
> 		     SPINAND_HAS_OCTAL_DTR_BIT | SPINAND_HAS_CR_FEAT_BIT,
> 		     SPINAND_ECCINFO(&w35n01jw_ooblayout, NULL),
> 		     SPINAND_INFO_OPS_VARIANTS(&w35n01jw_ops_variants)),
>
> You also need to adjust spinand_match_and_init() to account for this
> new layout and put each template op in the right subset based on
> op.cmd.width and op.cmd.dtr.
>
> Regards,
>
> Boris
Boris Brezillon March 10, 2022, 8:40 a.m. UTC | #8
On Thu, 10 Mar 2022 13:27:06 +0530
Apurva Nandan <a-nandan@ti.com> wrote:

> >>> This way, you can easily pick the right set of operations based
> >>> on the protocol/mode you're in:
> >>>
> >>> #define spinand_get_op_template(spinand, opname) \
> >>> 	((spinand)->op_templates[(spinand)->protocol]->opname)
> >>>
> >>> static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
> >>> {
> >>> 	struct spi_mem_op op = *spinand_get_op_template(spinand, get_feature);
> >>> 	int ret;
> >>>
> >>> 	...
> >>> }  
> >> I find a couple of issues with this  method,
> >>
> >> 1. read_cache, write_cache, update_cache op templates don't fit well
> >> with the other non-data ops, as
> >> these data ops are used to create a dirmap, and that can be done only
> >> once at probe time. Hence, there
> >> is a different mechanism of selecting of data ops and non-data ops.  
> > Not sure I see why this is a problem. You can populate data-ops for all
> > modes, and pick the one that provides the best perfs when you create
> > the dirmap (which should really be at the end of the probe, if it's not
> > already).
> >  
> >> Hence, this division in the op templates
> >> struct as data_ops and ctrl_ops is required. Currently, the core only
> >> supports using a single protocol for
> >> data ops, chosen at the time of probing.  
> > Again, I don't see why you need to differentiate the control and data
> > ops when populating this table. Those are just operations the NAND
> > supports, and the data operations is just a subset.
> >  
> >> 2. If we use this single op_templates struct, I can't think of any good
> >> way to initialize these in the
> >> manufacturers driver (winbond.c), refer to 17th patch in this series.
> >> Could you please suggest a macro
> >> implementation also for winbond.c with the suggested op_templates struct.  
> > First replace the op_variants field by something more generic:
> >
> > struct spinand_info {
> > ...
> > 	const struct spinand_op_variants **ops_variants;
> > ...
> > };
> >
> > #define SPINAND_OP_VARIANTS(_id, ...) \
> > 	[SPI_NAND_OP_ ## _id] = { __VA_ARGS__ }
> >
> > #define SPINAND_OPS_VARIANTS(name, ...)
> > 	const struct spinand_op_variants name[]{
> > 		__VA_ARGS__,
> > 	};
> >
> > #define SPINAND_INFO_OPS_VARIANTS(defs)
> > 	.ops_variants = defs  
> 
> If we modify these macros, it would require other spinand vendor drivers 
> to change (toshiba, micron, etc).
> The older macros suit them well, should we go about changing them to 
> this new macro (will require re-testing all of them),
> or can we keep them unchanged and have new set of macros with different 
> name (please give suggestion for it) for op variants.

I'd rather have everything converted to the new approach (we don't want
2 ways of describing the same thing), and I'm not sure you can make the
old macros map to the new solution, so I fear you'll have to patch all
vendors. This being said, I'm fine providing simple wrappers if that
helps, but I don't see how they'd make the description simpler/more
compact to be honest.
Apurva Nandan March 14, 2022, 11:47 a.m. UTC | #9
On 10/03/22 14:10, Boris Brezillon wrote:
> On Thu, 10 Mar 2022 13:27:06 +0530
> Apurva Nandan <a-nandan@ti.com> wrote:
>
>>>>> This way, you can easily pick the right set of operations based
>>>>> on the protocol/mode you're in:
>>>>>
>>>>> #define spinand_get_op_template(spinand, opname) \
>>>>> 	((spinand)->op_templates[(spinand)->protocol]->opname)
>>>>>
>>>>> static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
>>>>> {
>>>>> 	struct spi_mem_op op = *spinand_get_op_template(spinand, get_feature);
>>>>> 	int ret;
>>>>>
>>>>> 	...
>>>>> }
>>>> I find a couple of issues with this  method,
>>>>
>>>> 1. read_cache, write_cache, update_cache op templates don't fit well
>>>> with the other non-data ops, as
>>>> these data ops are used to create a dirmap, and that can be done only
>>>> once at probe time. Hence, there
>>>> is a different mechanism of selecting of data ops and non-data ops.
>>> Not sure I see why this is a problem. You can populate data-ops for all
>>> modes, and pick the one that provides the best perfs when you create
>>> the dirmap (which should really be at the end of the probe, if it's not
>>> already).
>>>   
>>>> Hence, this division in the op templates
>>>> struct as data_ops and ctrl_ops is required. Currently, the core only
>>>> supports using a single protocol for
>>>> data ops, chosen at the time of probing.
>>> Again, I don't see why you need to differentiate the control and data
>>> ops when populating this table. Those are just operations the NAND
>>> supports, and the data operations is just a subset.
>>>   
>>>> 2. If we use this single op_templates struct, I can't think of any good
>>>> way to initialize these in the
>>>> manufacturers driver (winbond.c), refer to 17th patch in this series.
>>>> Could you please suggest a macro
>>>> implementation also for winbond.c with the suggested op_templates struct.
>>> First replace the op_variants field by something more generic:
>>>
>>> struct spinand_info {
>>> ...
>>> 	const struct spinand_op_variants **ops_variants;
>>> ...
>>> };
>>>
>>> #define SPINAND_OP_VARIANTS(_id, ...) \
>>> 	[SPI_NAND_OP_ ## _id] = { __VA_ARGS__ }
>>>
>>> #define SPINAND_OPS_VARIANTS(name, ...)
>>> 	const struct spinand_op_variants name[]{
>>> 		__VA_ARGS__,
>>> 	};
>>>
>>> #define SPINAND_INFO_OPS_VARIANTS(defs)
>>> 	.ops_variants = defs
>> If we modify these macros, it would require other spinand vendor drivers
>> to change (toshiba, micron, etc).
>> The older macros suit them well, should we go about changing them to
>> this new macro (will require re-testing all of them),
>> or can we keep them unchanged and have new set of macros with different
>> name (please give suggestion for it) for op variants.
> I'd rather have everything converted to the new approach (we don't want
> 2 ways of describing the same thing), and I'm not sure you can make the
> old macros map to the new solution, so I fear you'll have to patch all
> vendors. This being said, I'm fine providing simple wrappers if that
> helps, but I don't see how they'd make the description simpler/more
> compact to be honest.
Okay, I will convert all of the vendor drivers, but please note I don't 
have any way to test the changes on all the flashes.
diff mbox series

Patch

diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 439d8ce40e1d..e5df6220ec1e 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -356,6 +356,35 @@  struct spinand_op_variants {
 			sizeof(struct spi_mem_op),			\
 	}
 
+struct spinand_ctrl_ops {
+	const struct {
+		struct spi_mem_op reset;
+		struct spi_mem_op get_feature;
+		struct spi_mem_op set_feature;
+		struct spi_mem_op write_enable;
+		struct spi_mem_op block_erase;
+		struct spi_mem_op page_read;
+		struct spi_mem_op program_execute;
+	} ops;
+	const enum spinand_protocol protocol;
+};
+
+#define SPINAND_CTRL_OPS(__protocol, __reset, __get_feature, __set_feature,	\
+			 __write_enable, __block_erase, __page_read,		\
+			 __program_execute)					\
+	{									\
+		.ops = {							\
+			.reset = __reset,					\
+			.get_feature = __get_feature,				\
+			.set_feature = __set_feature,				\
+			.write_enable = __write_enable,				\
+			.block_erase = __block_erase,				\
+			.page_read = __page_read,				\
+			.program_execute = __program_execute,			\
+		},								\
+		.protocol = __protocol,						\
+	}
+
 /**
  * spinand_ecc_info - description of the on-die ECC implemented by a SPI NAND
  *		      chip
@@ -468,6 +497,8 @@  struct spinand_dirmap {
  * @data_ops.read_cache: read cache op template
  * @data_ops.write_cache: write cache op template
  * @data_ops.update_cache: update cache op template
+ * @ctrl_ops: various SPI mem op templates for handling the flash device, i.e.
+ *	      non page-read/write ops.
  * @select_target: select a specific target/die. Usually called before sending
  *		   a command addressing a page or an eraseblock embedded in
  *		   this die. Only required if your chip exposes several dies
@@ -498,6 +529,8 @@  struct spinand_device {
 		const struct spi_mem_op *update_cache;
 	} data_ops;
 
+	const struct spinand_ctrl_ops *ctrl_ops;
+
 	struct spinand_dirmap *dirmaps;
 
 	int (*select_target)(struct spinand_device *spinand,