[PATCH v15 6/8] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Michael Walle]

Am 2022-05-14 05:51, schrieb Takahiro Kuwano:
> On 5/13/2022 6:40 PM, Michael Walle wrote:
>> [btw the subject still has the old name of the addr_width]
>> 
> Yes, it must be fixed in next rev.
> 
>> Am 2022-05-13 03:26, schrieb Takahiro Kuwano:
>>> On 5/13/2022 7:14 AM, Michael Walle wrote:
>>>> Am 2022-05-10 00:10, schrieb tkuw584924 at gmail.com:
>>>>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>>>> 
>>>>> In 4BAIT parse, keep nor->params->addr_width because it may be used 
>>>>> as
>>>>> current address mode in SMPT parse later on.
>>>> 
>>>> Mh I'm not sure this is needed at all.
>>>> 
>>>> SFDP spec says
>>>>   Variable address length (the current setting of the address
>>>>   length mode defines the address length)
>>>> 
>>>> and
>>>>   When the length is defined as variable, the software or hardware
>>>>   controlling the memory is aware of the address length mode last
>>>>   set in the memory device and this same length of address.
>>>> 
>>>> We don't set any address mode until all the SFDP parsing is
>>>> over. Therefore we should always be in 3 byte mode, no?
>>>> 
>>> Actually there are some devices that have variable address length but
>>> 4 byte mode by default (I will work on those devices after this 
>>> series
>>> is settled). To support such case, I prefer to use 
>>> params->addr_nbytes
>>> as current address mode so that I can fix it in post_bfpt_fixup() 
>>> hook.
>> 
>> Are there public datasheets available? So these devices have a 3 byte
> I will send datasheets to you in another email. At this point, only
> summary datasheet is available in website.
> 
>> and a 4 byte mode, but after reset, they are in the 4 byte mode? Looks
> Yes.
> 
>> like it should be fixed in a different way. I'm not sure the "current
>> mode" handling is correct.
>> 
> Yes, we may want to introduce a new flag like SPI_NOR_4BAM_DEFAULT and 
> check
> the flag in BFPT parse. Once I send another series, please review.
> 
>> We need to differentiate between the mode the flash currently is using
>> (nor->addr_nbytes) and the mode parsed by SFDP (params->addr_nbytes).
>> 
> The flash's address mode affects the address length of Non-4B opcodes,
> including read/write any register ops used in SMPT parse and Infineon
> (spansion) specific hooks.
> 
> The 4B opcodes always take address length of 4 regardless of flash's
> address mode. In these Infineon chips, 4B opcodes for read/program/
> erase are available and 4BAIT advertises them. We don't have to enter
> 4 byte address mode for read/program/erase.

btw. this is a pity. you are using the stateless 4b opcodes but
then you don't provide stateless opcodes for the read any register
op :/

> So, I think we need to differentiate between address length for
> read/program/erase and flash's default address mode.

Or we keep them in sync. E.g. switch to 4bytes mode if we are
using the 4 byte. Granted, that sounds like a hack :)

> Obviously we are using nor->addr_nbytes as address length for read/
> program/erase and should keep this usage.

Yes, I wasn't aware that we actually two different runtime
parameters:
  - the read/program/erase address width, also used with the
    4b opcodes
  - internal mode 3b/4b. Up until now, this wasn't an issue
    because either the mode was switched or the 4b opcodes
    were used. So this was mutually exclusive. Now we have
    flashes which uses 4b opcodes _and_ we need the state
    of the internal mode.

I can't think of a good solution for now. Need to think
more about this, but I'm pretty busy at the moment.
What I think is clear is that we need two different modes
here in the spi_nor struct. nor->addr_nbytes for the
read/program/erase opcodes and nor->address_mode or similar
which tracks the SPI flash's internal address mode.

> For flash's default address mode, my preference is to use
> params->addr_nbytes, but I should rename it to something like
> params->def_addr_nbytes and rework spi_nor_set_addr_nbytes().

IMHO params should only be used to store the parsed (or
hardcoded) parameters.

-michael

>  static int spi_nor_set_addr_nbytes(struct spi_nor *nor)
>  {
> 	if (nor->flags & SNOR_F_HAS_4BAIT) {
> 		nor->addr_nbytes = 4;
> 	} else if (nor->params->def_addr_nbytes) {
>  		nor->addr_nbytes = nor->params->def_addr_nbytes;
> 
>> At some point, the mode is switched and nor->addr_nbytes becomes
>> params->addr_nbytes. It seems in your case nor->addr_nbytes should
>> be 4 right from the beginning. Which also means nor->addr_nbytes
>> should be 3 for the other cases (and probably not 0).
>> 
> With param->def_addr_nbytes, I think we can keep nor->addr_nbytes = 0
> during SFDP parse.
> 
> Thanks,
> Takahiro