diff mbox series

[v2,35/35] docs: mtd: spi-nor: Add details about how to propose a new flash addition

Message ID 20210727045222.905056-36-tudor.ambarus@microchip.com (mailing list archive)
State New, archived
Headers show
Series mtd: spi-nor: Handle ID collisions and clean params init | expand

Commit Message

Tudor Ambarus July 27, 2021, 4:52 a.m. UTC
Add some guideliness on how to propose a new flash addition.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 Documentation/driver-api/mtd/spi-nor.rst | 65 ++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

Comments

Michael Walle July 27, 2021, 7:22 a.m. UTC | #1
Am 2021-07-27 06:52, schrieb Tudor Ambarus:
> Add some guideliness on how to propose a new flash addition.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  Documentation/driver-api/mtd/spi-nor.rst | 65 ++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/Documentation/driver-api/mtd/spi-nor.rst
> b/Documentation/driver-api/mtd/spi-nor.rst
> index 4a3adca417fd..ffb8d97a2766 100644
> --- a/Documentation/driver-api/mtd/spi-nor.rst
> +++ b/Documentation/driver-api/mtd/spi-nor.rst
[..]
> +Every new flash addition that define the SFDP tables, should hexdump 
> its SFDP
> +tables in the patch's comment section below the --- line, so that we 
> can
> +reference it in case of ID collisions.

Nice, but could you add some guidelines how to do it? That is the exact
commands, maybe with a notice one should use these whenever possible. I
want to prevent having all sorts of variations of the output and I want
to be able to reverse the operation and verify it.

# xxd -p /path/to/sfdp
# md5sum /path/to/sfdp
# cat /path/to/jedec_id
# cat /path/to/partname
# cat /path/to/manufacturer

-michael
Tudor Ambarus July 27, 2021, 8:09 a.m. UTC | #2
On 7/27/21 10:22 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-07-27 06:52, schrieb Tudor Ambarus:
>> Add some guideliness on how to propose a new flash addition.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  Documentation/driver-api/mtd/spi-nor.rst | 65 ++++++++++++++++++++++++
>>  1 file changed, 65 insertions(+)
>>
>> diff --git a/Documentation/driver-api/mtd/spi-nor.rst
>> b/Documentation/driver-api/mtd/spi-nor.rst
>> index 4a3adca417fd..ffb8d97a2766 100644
>> --- a/Documentation/driver-api/mtd/spi-nor.rst
>> +++ b/Documentation/driver-api/mtd/spi-nor.rst
> [..]
>> +Every new flash addition that define the SFDP tables, should hexdump
>> its SFDP
>> +tables in the patch's comment section below the --- line, so that we
>> can
>> +reference it in case of ID collisions.
> 
> Nice, but could you add some guidelines how to do it? That is the exact
> commands, maybe with a notice one should use these whenever possible. I
> want to prevent having all sorts of variations of the output and I want
> to be able to reverse the operation and verify it.

ok, will do

> 
> # xxd -p /path/to/sfdp
> # md5sum /path/to/sfdp

maybe sha1sum here?

> # cat /path/to/jedec_id
> # cat /path/to/partname
> # cat /path/to/manufacturer
> 

Will add some short examples of mtd_debug and the erase, verify, write, read back
and compare too.

Do you have some locking test examples? I'll have to check those too.
Michael Walle July 27, 2021, 8:49 a.m. UTC | #3
Am 2021-07-27 10:09, schrieb Tudor.Ambarus@microchip.com:
> On 7/27/21 10:22 AM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2021-07-27 06:52, schrieb Tudor Ambarus:
>>> Add some guideliness on how to propose a new flash addition.
>>> 
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>> ---
>>>  Documentation/driver-api/mtd/spi-nor.rst | 65 
>>> ++++++++++++++++++++++++
>>>  1 file changed, 65 insertions(+)
>>> 
>>> diff --git a/Documentation/driver-api/mtd/spi-nor.rst
>>> b/Documentation/driver-api/mtd/spi-nor.rst
>>> index 4a3adca417fd..ffb8d97a2766 100644
>>> --- a/Documentation/driver-api/mtd/spi-nor.rst
>>> +++ b/Documentation/driver-api/mtd/spi-nor.rst
>> [..]
>>> +Every new flash addition that define the SFDP tables, should hexdump
>>> its SFDP
>>> +tables in the patch's comment section below the --- line, so that we
>>> can
>>> +reference it in case of ID collisions.
>> 
>> Nice, but could you add some guidelines how to do it? That is the 
>> exact
>> commands, maybe with a notice one should use these whenever possible. 
>> I
>> want to prevent having all sorts of variations of the output and I 
>> want
>> to be able to reverse the operation and verify it.
> 
> ok, will do
> 
>> 
>> # xxd -p /path/to/sfdp
>> # md5sum /path/to/sfdp
> 
> maybe sha1sum here?

sure, that one doesn't really matter. any *sum will work.

>> # cat /path/to/jedec_id
>> # cat /path/to/partname
>> # cat /path/to/manufacturer
>> 
> 
> Will add some short examples of mtd_debug and the erase, verify,
> write, read back
> and compare too.
> 
> Do you have some locking test examples? I'll have to check those too.

Not really, I usually check the locking by looking at the BP bits,
but there is no easy method to look at them in linux.

-michael
Pratyush Yadav Aug. 24, 2021, 5:58 p.m. UTC | #4
On 27/07/21 09:22AM, Michael Walle wrote:
> Am 2021-07-27 06:52, schrieb Tudor Ambarus:
> > Add some guideliness on how to propose a new flash addition.
> > 
> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> > ---
> >  Documentation/driver-api/mtd/spi-nor.rst | 65 ++++++++++++++++++++++++
> >  1 file changed, 65 insertions(+)
> > 
> > diff --git a/Documentation/driver-api/mtd/spi-nor.rst
> > b/Documentation/driver-api/mtd/spi-nor.rst
> > index 4a3adca417fd..ffb8d97a2766 100644
> > --- a/Documentation/driver-api/mtd/spi-nor.rst
> > +++ b/Documentation/driver-api/mtd/spi-nor.rst
> [..]
> > +Every new flash addition that define the SFDP tables, should hexdump
> > its SFDP
> > +tables in the patch's comment section below the --- line, so that we
> > can
> > +reference it in case of ID collisions.
> 
> Nice, but could you add some guidelines how to do it? That is the exact
> commands, maybe with a notice one should use these whenever possible. I
> want to prevent having all sorts of variations of the output and I want
> to be able to reverse the operation and verify it.
> 
> # xxd -p /path/to/sfdp
> # md5sum /path/to/sfdp
> # cat /path/to/jedec_id
> # cat /path/to/partname
> # cat /path/to/manufacturer

It would be nice if we could have checkpatch.pl check for these. But I 
don't know how difficult that would be to add.
diff mbox series

Patch

diff --git a/Documentation/driver-api/mtd/spi-nor.rst b/Documentation/driver-api/mtd/spi-nor.rst
index 4a3adca417fd..ffb8d97a2766 100644
--- a/Documentation/driver-api/mtd/spi-nor.rst
+++ b/Documentation/driver-api/mtd/spi-nor.rst
@@ -66,3 +66,68 @@  when you want to write a new driver for a SPI NOR controller.
 Another API is spi_nor_restore(), this is used to restore the status of SPI
 flash chip such as addressing mode. Call it whenever detach the driver from
 device or reboot the system.
+
+Part IV - How to propose a new flash addition?
+----------------------------------------------
+
+First we have to clarify where the new flash_info entry will reside. Typically
+each manufacturer have their own driver and the new flash will be placed in that
+specific manufacturer driver. There are cases however, where special care has to
+be taken. In case of flash ID collisions between different manufacturers, the
+place to add the new flash is in the manuf-id-collisions.c driver. ID collisions
+between flashes of the same manufacturer should be handled in their own
+manufacturer driver, macronix being an example. There will be a single
+flash_info entry for all the ID collisions of the same ID.
+
+manuf-id-collisions.c is the place to add new flash additions where the
+manufacturer is ignorant enough to not implement the ID continuation scheme
+that is described in the JEP106 JEDEC Standard. One has to dump its flash ID and
+compare it with the flash's manufacturer identification code that is defined in
+the JEP106 JEDEC Standard. If the manufacturer ID is defined in bank two or
+higher and the manufacturer does not implement the ID continuation scheme, then
+it is likely that the flash ID will collide with a manufacturer from bank one or
+with other manufacturer from other bank that does not implement the ID
+continuation scheme as well.
+
+flash_info entries will be added in a first come, first served manner. If there
+are ID collisions, differentiation between flashes will be done at runtime if
+possible. Where runtime differentiation is not possible, new compatibles will be
+introduced, but this will be done as a last resort.
+
+New flash additions that support the SFDP standard should be declared using
+SPI_NOR_PARSE_SFDP. Support that can be discovered when parsing SFDP should not
+be duplicated by explicit flags at flash declaration. All the SFDP flash
+parameters and settings will be discovered when parsing SFDP. There are
+flash_info flags that indicate support that is not SFDP discoverable. These
+flags initialize non SFDP support in the spi_nor_nonsfdp_flags_init() method.
+SPI_NOR_PARSE_SFDP is usually followed by other flash_info flags from the
+aforementioned function. Sometimes manufacturers wrongly define some fields in
+the SFDP tables. If that's the case, SFDP data can be amended with the fixups()
+hooks. It is not common, but if the SFDP tables are entirely wrong, and it does
+not worth the hassle to tweak the SFDP parameters by using the fixups hooks, or
+if the flash does not define the SFDP tables at all, then one can statically
+init the flash with the SPI_NOR_SKIP_SFDP flag and specify the rest of the flash
+capabilities with the flash info flags.
+
+With time we want to convert all flashes to either use SPI_NOR_PARSE_SFDP or
+SPI_NOR_SKIP_SFDP and stop triggering the SFDP parsing with the
+SPI_NOR_{DUAL, QUAD, OCTAL*}_READ flags. There are flashes that support QUAD
+mode but do not support the RDSFDP command, we should avoid issuing unsupported
+commands to flashes where possible. It is unlikely that RDSFDP will cause any
+problems, but still, it's better to avoid it. There are cases however of flash
+ID collisions between flashes that define the SFDP tables and flashes that don't
+(again, macronix). We usually differentiate between the two by issuing the
+RDSFDP command. In such a case one has to declare the SPI_NOR_PARSE_SFDP
+together with all the relevant flags from spi_nor_nonsfdp_flags_init() for the
+SFDP compatible flash, but should also declare the relevant flags that are used
+in the spi_nor_info_init_params() method in order to init support that can't be
+discovered via SFDP for the non-SFDP compatible flash.
+
+Every new flash addition that define the SFDP tables, should hexdump its SFDP
+tables in the patch's comment section below the --- line, so that we can
+reference it in case of ID collisions.
+
+Every flash_info flag declared should be tested. Typically one uses the
+mtd-utils and does an erase, verify erase, write, read back and compare test.
+Locking and other flags that are declared in the flash_info entry and used in
+the spi_nor_nonsfdp_flags_init() should be tested as well.