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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Wed, 2 Mar 2022 21:00:55 +0530
Apurva Nandan <a-nandan@xxxxxx> 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.