Re: [PATCH v3 08/36] mtd: devices: Provide header for shared OPCODEs and SFDP commands

Brian Norris
2013-12-10T22:23:14Z

On Tue, Dec 10, 2013 at 01:48:49PM -0800, Brian Norris wrote:
> It doesn't look to me like the dual/quad bitfields are laid out very
> usefully. Can't they be divided so that their bit position is more
> meaningful? Like reserve bits [8:23] for dual/quad descriptions, then
> give the following:
>
> [8:11] number of pins for the opcode (1=single; 2=dual; 4=quad)
> [12:15] number of pins for the address (1=single; 2=dual; ...)
> [16:19] number of pins for the data
> [20] read or write opcode (0=read; 1=write)
>
> (you could easily pack these differently, but you get the idea)
>
> Then:
>
> #define FLASH_FLAG_DUAL_MASK 0x00022200
> #define FLASH_FLAG_QUAD_MASK 0x00044400
>
> And we could do things like:
>
> #define FLASH_FLAG_OPCODE_PINS(x) (((x) & 0x00000f00) >> 8)
> #define FLASH_FLAG_ADDR_PINS(x) (((x) & 0x0000f000) >> 12)
> #define FLASH_FLAG_DATA_PINS(x) (((x) & 0x000f0000) >> 16)
>
> (and maybe replace those mask/shifts with macro names)

I realized you are packing these tight because you're using them as
combinable (bitwise OR) flags (hence the name FLASH_FLAG_*, duh!). So
while my scheme is nicer for representing a single opcode w.r.t.
controller requirements, it is not able to represent the exact opcodes
supported by a particular flash. Hence, this is not the right place for
it.

But I don't want to imitate your flags in any generic framework; we need
a method (that doesn't involve too many extra tables like in your
driver; or that supports these tables generically, to share with other
drivers) to map the "supported opcodes" flags to useful properties that
the controller/driver can examine.

Is it possible, for instance, to describe a range of opcodes supported
using patterns? Like "this flash supports opcodes for 1-4 data pins and
1-2 address pins"? Or am I being too idealistic, and most flash really
support a totally arbitrary combination of opcodes?

Brian