| Message ID | 20210702144110.250481-5-tudor.ambarus@microchip.com |
|---|---|
| State | Superseded |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: Handle flash ID collisions | expand |
Hi Tudor, Am Fr., 2. Juli 2021 um 16:41 Uhr schrieb Tudor Ambarus <tudor.ambarus@microchip.com>: > > MX25L12835F define SFDP, while MX25L12805D does not. I tested this on the kontron pitx-imx8m board with a MX25L12835F equipped. [ 1.213448] spi-nor spi0.0: mx25l12835f (16384 Kbytes) > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> Testd-by: Heiko Thiery <heiko.thiery@gmail.com> > --- > drivers/mtd/spi-nor/macronix.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c > index 83a097708949..38701347bf04 100644 > --- a/drivers/mtd/spi-nor/macronix.c > +++ b/drivers/mtd/spi-nor/macronix.c > @@ -8,6 +8,26 @@ > > #include "core.h" > > +static const char *mx25l12835f = "mx25l12835f"; > + > +static int mx25l12835f_post_bfpt_fixups(struct spi_nor *nor, > + const struct sfdp_parameter_header *bfpt_header, > + const struct sfdp_bfpt *bfpt) > +{ > + /* > + * Macronix has a bad habit of reusing flash IDs: MX25L12835F collides > + * with MX25L12805D. MX25L12835F defines SFDP tables, while the older > + * variant does not. > + */ > + nor->name = mx25l12835f; > + > + return 0; > +} > + > +static struct spi_nor_fixups mx25l12835f_fixups = { > + .post_bfpt = mx25l12835f_post_bfpt_fixups, > +}; > + > static const char *mx25l3233f = "mx25l3233f"; > > static int mx25l3233f_post_bfpt_fixups(struct spi_nor *nor, > @@ -71,8 +91,9 @@ static const struct flash_info macronix_parts[] = { > { "mx25u4035", INFO(0xc22533, 0, 64 * 1024, 8, SECT_4K) }, > { "mx25u8035", INFO(0xc22534, 0, 64 * 1024, 16, SECT_4K) }, > { "mx25u6435f", INFO(0xc22537, 0, 64 * 1024, 128, SECT_4K) }, > - { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SECT_4K | > - SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) }, > + { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP | > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) > + .fixups = &mx25l12835f_fixups }, > { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) }, > { "mx25r1635f", INFO(0xc22815, 0, 64 * 1024, 32, > SECT_4K | SPI_NOR_DUAL_READ | Thank you -- Heiko
On 7/2/21 5:41 PM, Tudor Ambarus wrote: > + { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP | > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) I should have removed SECT_4K, since 4k erase should be set when parsing SFDP. If SFDP is wrong, we can amend it in the post_bfpt hook. Heiko, can you test 4k erase with SECT_4K removed? Also, would you please dump the SFDP tables using Michael's sysfs patches? They were applied. Every new flash addition should have the SFDP tables dumped in the patch's comment section, under the --- line. I don't have the flash, so I'll need your help. Thanks! ta
Hi Tudor, Am Mo., 5. Juli 2021 um 09:51 Uhr schrieb <Tudor.Ambarus@microchip.com>: > > On 7/2/21 5:41 PM, Tudor Ambarus wrote: > > + { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP | > > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) > > I should have removed SECT_4K, since 4k erase should be set when parsing SFDP. > If SFDP is wrong, we can amend it in the post_bfpt hook. But what about the 4K support for the "old" mx25l12805d that does not support reading SFDP? As far as I understand, the post_bfpt hook is only called when the SFDP was successfully read and this would mean the "old" one would never get this setting. > Heiko, can you test 4k erase with SECT_4K removed? Also, would you please dump the > SFDP tables using Michael's sysfs patches? They were applied. Every new flash addition > should have the SFDP tables dumped in the patch's comment section, under the --- line. > I don't have the flash, so I'll need your help. Nevertheless I removed the SECT_4K entry from the info table. After that the correct value still was set due to the parsing of the SFDP data. # cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0 .0/mtd/mtd0/erasesize 4096 And here is the requested dump: # cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0 .0/spi-nor/sfdp | xxd -p 53464450000101ff00000109300000ffc2000104600000ffffffffffffff ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b 083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff ffffffffffff003600279df9c06485cbffffffffffff Thanks
Hi, Am 2021-07-05 12:45, schrieb Heiko Thiery: > # cat > /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0 > .0/spi-nor/sfdp | xxd -p > 53464450000101ff00000109300000ffc2000104600000ffffffffffffff > ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b > 083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff > ffffffffffff003600279df9c06485cbffffffffffff Ok, I like this output esp, because you can reverse it with "xxd -r -p". Maybe we should also request an md5sum just to be sure we have the same binary. so the requested commands would be something like: # xxd -p /path/to/sfdp # md5sum /path/to/sfdp # cat /path/to/jedec_id # cat /path/to/partname # cat /path/to/manufacturer -michael
Hi Michael, Am Mo., 5. Juli 2021 um 12:59 Uhr schrieb Michael Walle <michael@walle.cc>: > > Hi, > > Am 2021-07-05 12:45, schrieb Heiko Thiery: > > # cat > > /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0 > > .0/spi-nor/sfdp | xxd -p > > 53464450000101ff00000109300000ffc2000104600000ffffffffffffff > > ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b > > 083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff > > ffffffffffff003600279df9c06485cbffffffffffff > > Ok, I like this output esp, because you can reverse it with > "xxd -r -p". Maybe we should also request an md5sum just to > be sure we have the same binary. > > so the requested commands would be something like: > > # xxd -p /path/to/sfdp > # md5sum /path/to/sfdp > # cat /path/to/jedec_id > # cat /path/to/partname > # cat /path/to/manufacturer this will be the output for your suggestion: # P=/sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor && xxd -p $P/sfdp && md5sum $P/sfdp && cat $P/jedec_id && cat $P/partname && cat $P/manufacturer 53464450000101ff00000109300000ffc2000104600000ffffffffffffff ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b 083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff ffffffffffff003600279df9c06485cbffffffffffff f7a09ec0b60c9a0acd01bf2759f0d1f4 /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/sfdp c22018 mx25l12805d macronix
On 7/5/21 1:45 PM, Heiko Thiery wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Tudor, > > Am Mo., 5. Juli 2021 um 09:51 Uhr schrieb <Tudor.Ambarus@microchip.com>: >> >> On 7/2/21 5:41 PM, Tudor Ambarus wrote: >>> + { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP | >>> + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) >> >> I should have removed SECT_4K, since 4k erase should be set when parsing SFDP. >> If SFDP is wrong, we can amend it in the post_bfpt hook. > > But what about the 4K support for the "old" mx25l12805d that does not > support reading SFDP? As far as I understand, the post_bfpt hook is > only called when the SFDP was successfully read and this would mean > the "old" one would never get this setting. You're absolutely correct, we'll keep SECT_4K in order to not break backward compatibility with the old flash. I thought it was a new flash addition. That's me replying to emails while being distracted. Anyway, I'll have a documentation written somewhere about guidelines on how to propose new flash addition or how to handle flash collisions. Everything seems fine here.
On 02/07/21 05:41PM, Tudor Ambarus wrote: > MX25L12835F define SFDP, while MX25L12805D does not. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/macronix.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c > index 83a097708949..38701347bf04 100644 > --- a/drivers/mtd/spi-nor/macronix.c > +++ b/drivers/mtd/spi-nor/macronix.c > @@ -8,6 +8,26 @@ > > #include "core.h" > > +static const char *mx25l12835f = "mx25l12835f"; > + > +static int mx25l12835f_post_bfpt_fixups(struct spi_nor *nor, > + const struct sfdp_parameter_header *bfpt_header, > + const struct sfdp_bfpt *bfpt) > +{ > + /* > + * Macronix has a bad habit of reusing flash IDs: MX25L12835F collides > + * with MX25L12805D. MX25L12835F defines SFDP tables, while the older > + * variant does not. > + */ > + nor->name = mx25l12835f; Same question as patch 3/7. Do we need to declare a separate pointer? > + > + return 0; > +} > + > +static struct spi_nor_fixups mx25l12835f_fixups = { > + .post_bfpt = mx25l12835f_post_bfpt_fixups, > +}; > + > static const char *mx25l3233f = "mx25l3233f"; > > static int mx25l3233f_post_bfpt_fixups(struct spi_nor *nor, > @@ -71,8 +91,9 @@ static const struct flash_info macronix_parts[] = { > { "mx25u4035", INFO(0xc22533, 0, 64 * 1024, 8, SECT_4K) }, > { "mx25u8035", INFO(0xc22534, 0, 64 * 1024, 16, SECT_4K) }, > { "mx25u6435f", INFO(0xc22537, 0, 64 * 1024, 128, SECT_4K) }, > - { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SECT_4K | > - SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) }, > + { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP | Same comment as patch 3/7. Consider adding a comment here listing all flashes with collision on this ID. Other than these two nitpicks, Acked-by: Pratyush Yadav <p.yadav@ti.com> > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) > + .fixups = &mx25l12835f_fixups }, > { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) }, > { "mx25r1635f", INFO(0xc22815, 0, 64 * 1024, 32, > SECT_4K | SPI_NOR_DUAL_READ | > -- > 2.25.1 >
diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c index 83a097708949..38701347bf04 100644 --- a/drivers/mtd/spi-nor/macronix.c +++ b/drivers/mtd/spi-nor/macronix.c @@ -8,6 +8,26 @@ #include "core.h" +static const char *mx25l12835f = "mx25l12835f"; + +static int mx25l12835f_post_bfpt_fixups(struct spi_nor *nor, + const struct sfdp_parameter_header *bfpt_header, + const struct sfdp_bfpt *bfpt) +{ + /* + * Macronix has a bad habit of reusing flash IDs: MX25L12835F collides + * with MX25L12805D. MX25L12835F defines SFDP tables, while the older + * variant does not. + */ + nor->name = mx25l12835f; + + return 0; +} + +static struct spi_nor_fixups mx25l12835f_fixups = { + .post_bfpt = mx25l12835f_post_bfpt_fixups, +}; + static const char *mx25l3233f = "mx25l3233f"; static int mx25l3233f_post_bfpt_fixups(struct spi_nor *nor, @@ -71,8 +91,9 @@ static const struct flash_info macronix_parts[] = { { "mx25u4035", INFO(0xc22533, 0, 64 * 1024, 8, SECT_4K) }, { "mx25u8035", INFO(0xc22534, 0, 64 * 1024, 16, SECT_4K) }, { "mx25u6435f", INFO(0xc22537, 0, 64 * 1024, 128, SECT_4K) }, - { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SECT_4K | - SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) }, + { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP | + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) + .fixups = &mx25l12835f_fixups }, { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) }, { "mx25r1635f", INFO(0xc22815, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_DUAL_READ |
MX25L12835F define SFDP, while MX25L12805D does not. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/macronix.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)