MT25P vs MT25Q bulk erase difference - how to fix?

[Tim <Ti...@JTi.uk.com>]

As I still get deeper and deeper into why FAT doesn't work on my MT25Q NOR
flash device, I have found a minor error in m25px.c

The M25P devices have a bulk erase command (0xC7) but this is not supported
by the MT25Q, which has a "die erase" command (0xC4) instead and 0xC7 is
meaningless.

There is a #define for this as expected:

#define M25P_BE	0xc7

Adding

#define M25Q_DE	0xc4

Make sense but...

When the device manufacturer ID is read this is actually saved, so it can't
be tested at runtime to choose the right command.

The straightforward fix is to move the #define for M25_BE to within the code
that tests the manufacturer ID (to ensure it's a valid device for these
functions) but that is a bit messy and moves the #define away from all the
other related #defines. Doesn't feel right.

There is no unique CONFIG_ attribute that says whether it is or isn't
expecting to find an M25P or M25Q (which is reasonable) so I can't use
conditional #defines.

And don't want to mess up other people's boards by changing Kconfig in any
way that isn't backwards compatible, although the inviolable "Sometimes Code
Duplication is OK" might suggest it would be better to copy out m25px.c as
m25lx.c and make the change so that Kconfig demands you choose one or the
other or both of the two types...and if I find more errors/differences that
may ultimately make sense of course.

Can anyone suggest the "NuttX way" to do this, assuming I don't find a
myriad of other errors/differences that "force" duplication?

Re: MT25P vs MT25Q bulk erase difference - how to fix?

[Tim Hardisty <Ti...@JTi.uk.com>]

Thanks - makes sense. I am still not 100% convinced the M25P code is 
actually correct for the M25Q. It's a work-in-progress.


This is what I have added, and all other things being equal, I will use 
it in the driver where there are subtle differences needed.


structm25p_dev_s
{
structmtd_dev_smtd; /* MTD interface */
FARstructspi_dev_s*dev; /* Saved SPI interface instance */
uint8_tsectorshift; /* 16 or 18 */
uint8_tpageshift; /* 8 */
uint16_tnsectors; /* 128 or 64 */
uint32_tnpages; /* 32,768 or 65,536 */
***uint16_t****manufacturer**; **/* detected manufacturer */*
***uint16_t****memory**; **/* detected memory type */*
#ifdefCONFIG_M25P_SUBSECTOR_ERASE
uint8_tsubsectorshift; /* 0, 12 or 13 (4K or 8K) */
#endif
};

and

SPI_SEND(priv->dev, M25P_RDID);
manufacturer= SPI_SEND(priv->dev, M25P_DUMMY);
memory= SPI_SEND(priv->dev, M25P_DUMMY);
capacity= SPI_SEND(priv->dev, M25P_DUMMY);
/* Deselect the FLASH and unlock the bus */
SPI_SELECT(priv->dev, SPIDEV_FLASH(0), false);
m25p_unlock(priv->dev);
/* save the manufacturer and memory type */
***priv**->**manufacturer**= **manufacturer**;*
***priv**->**memory**= **memory**;*
and in the erase function
/* Send the "Bulk Erase (BE)" instruction or "Die Erase" as appropriate*/
if(priv->manufacturer== M25P_MANUFACTURER&&
(priv->memory== MT25Q_MEMORY_TYPE|| priv->memory== MT25QU_MEMORY_TYPE))
{
SPI_SEND(priv->dev, M25Q_DE);
}
else
{
SPI_SEND(priv->dev, M25P_BE);
}
On 18/01/2022 16:36, Sebastien Lorquet wrote:
> Hello,
>
> If the Die Erase is similar in function to the bulk erase, then it can
> be used instead, but this has to be done at runtime to support all the
> chips in the same driver. So no @define or CONFIG_ option, because
> choosing one would excude the others, and if you have both on a board
> this would be bad.
>
> The runtime fix is not heavy. My recommended way is:
>
> -add a uint8_t device_manufacturer field in the m25px structure,
> initialized when the JEDEC ID is obtained from the device (IIRC,
> m25px_initialize or something like that)
>
> -then, use this field in the m25px_erase method to choose the correct
> command(, and parameters if needed).
>
>
> I woud appreciate if you agreed to share your changes beforehand (eg
> pull request) so I can review this simple change, because i'm using this
> driver in a professional project and I would like to make sure there are
> no regressions.
>
> Thank you,
>
> Sebastien
>
> Le 18/01/2022 à 16:41, Tim a écrit :
>> As I still get deeper and deeper into why FAT doesn't work on my 
>> MT25Q NOR
>> flash device, I have found a minor error in m25px.c
>>
>> The M25P devices have a bulk erase command (0xC7) but this is not 
>> supported
>> by the MT25Q, which has a "die erase" command (0xC4) instead and 0xC7 is
>> meaningless.
>>
>> There is a #define for this as expected:
>>
>> #define M25P_BE    0xc7
>>
>> Adding
>>
>> #define M25Q_DE    0xc4
>>
>> Make sense but...
>>
>> When the device manufacturer ID is read this is actually saved, so it 
>> can't
>> be tested at runtime to choose the right command.
>>
>> The straightforward fix is to move the #define for M25_BE to within 
>> the code
>> that tests the manufacturer ID (to ensure it's a valid device for these
>> functions) but that is a bit messy and moves the #define away from 
>> all the
>> other related #defines. Doesn't feel right.
>>
>> There is no unique CONFIG_ attribute that says whether it is or isn't
>> expecting to find an M25P or M25Q (which is reasonable) so I can't use
>> conditional #defines.
>>
>> And don't want to mess up other people's boards by changing Kconfig 
>> in any
>> way that isn't backwards compatible, although the inviolable 
>> "Sometimes Code
>> Duplication is OK" might suggest it would be better to copy out 
>> m25px.c as
>> m25lx.c and make the change so that Kconfig demands you choose one or 
>> the
>> other or both of the two types...and if I find more 
>> errors/differences that
>> may ultimately make sense of course.
>>
>> Can anyone suggest the "NuttX way" to do this, assuming I don't find a
>> myriad of other errors/differences that "force" duplication?
>>
>>
>>

Re: MT25P vs MT25Q bulk erase difference - how to fix?

[Sebastien Lorquet <se...@lorquet.fr>]

Hello,

If the Die Erase is similar in function to the bulk erase, then it can 
be used instead, but this has to be done at runtime to support all the 
chips in the same driver. So no @define or CONFIG_ option, because 
choosing one would excude the others, and if you have both on a board 
this would be bad.

The runtime fix is not heavy. My recommended way is:

-add a uint8_t device_manufacturer field in the m25px structure, 
initialized when the JEDEC ID is obtained from the device (IIRC, 
m25px_initialize or something like that)

-then, use this field in the m25px_erase method to choose the correct 
command(, and parameters if needed).


I woud appreciate if you agreed to share your changes beforehand (eg 
pull request) so I can review this simple change, because i'm using this 
driver in a professional project and I would like to make sure there are 
no regressions.

Thank you,

Sebastien

Le 18/01/2022 à 16:41, Tim a écrit :
> As I still get deeper and deeper into why FAT doesn't work on my MT25Q NOR
> flash device, I have found a minor error in m25px.c
>
> The M25P devices have a bulk erase command (0xC7) but this is not supported
> by the MT25Q, which has a "die erase" command (0xC4) instead and 0xC7 is
> meaningless.
>
> There is a #define for this as expected:
>
> #define M25P_BE	0xc7
>
> Adding
>
> #define M25Q_DE	0xc4
>
> Make sense but...
>
> When the device manufacturer ID is read this is actually saved, so it can't
> be tested at runtime to choose the right command.
>
> The straightforward fix is to move the #define for M25_BE to within the code
> that tests the manufacturer ID (to ensure it's a valid device for these
> functions) but that is a bit messy and moves the #define away from all the
> other related #defines. Doesn't feel right.
>
> There is no unique CONFIG_ attribute that says whether it is or isn't
> expecting to find an M25P or M25Q (which is reasonable) so I can't use
> conditional #defines.
>
> And don't want to mess up other people's boards by changing Kconfig in any
> way that isn't backwards compatible, although the inviolable "Sometimes Code
> Duplication is OK" might suggest it would be better to copy out m25px.c as
> m25lx.c and make the change so that Kconfig demands you choose one or the
> other or both of the two types...and if I find more errors/differences that
> may ultimately make sense of course.
>
> Can anyone suggest the "NuttX way" to do this, assuming I don't find a
> myriad of other errors/differences that "force" duplication?
>
>
>