[PATCH v5 09/10] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[Takahiro Kuwano <tkuw584924@gmail.com>]

On 3/8/2021 5:54 PM, Pratyush Yadav wrote:
> On 08/03/21 05:47PM, Takahiro Kuwano wrote:
>> On 2/24/2021 9:40 PM, Pratyush Yadav wrote:
>>> On 19/02/21 10:56AM, tkuw584924 at gmail.com wrote:
>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>
>>>> This patch adds Flash specific fixups and hooks for Cypress
>>>> S25HL-T/S25HS-T family, based on the features introduced in [0][1][2].
>>>
>>> Instead of linking the patches like this, it would be a better idea to 
>>> simply include them in your series. This will make your series 
>>> independent from mine and will make it easier for the maintainer to 
>>> apply it.
>>>
>> Noted with thanks.
>>
>>>>
>>>> The nor->ready() and spansion_sr_ready() introduced in #5 and #6 in this
>>>> series are used for multi-die package parts.
>>>
>>> Nitpick: Once this patch makes it into the Git history there is no patch 
>>> #5 or #6. Probably a better idea to just say "introduced earlier" or 
>>> something similar. 
>>>
>> OK.
>>
>>>>
>>>> The nor->quad_enable() sets the volatile QE bit on each die.
>>>>
>>>> The mtd._erase() is hooked if the device is single-die package and not
>>>> configured to uniform sectors, assuming it is in factory default
>>>> configuration that has 32 x 4KB sectors overlaid on bottom address.
>>>> Other configurations, top and split, are not supported at this point.
>>>> Will submit additional patches to support it as needed.
>>>
>>> Ah, this patch does check for uniform sector map. Nice. I assume you 
>>> have tested with both configurations.
>>>
>> Yes. I tested both.
>>
>>>>
>>>> The post_bfpt/sfdp() fixes the params wrongly advertised in SFDP.
>>>>
>>>> [0] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-7-p.yadav at ti.com/
>>>> [1] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.yadav at ti.com/
>>>> [2] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav at ti.com/
>>>>
>>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>> ---
>>>> Changes in v5:
>>>>   - Add s25hx_t_erase_non_uniform()
>>>>   - Change mtd.writesize and mtd.flags in s25hx_t_setup()
>>>>   - Fix page size and erase size issues in s25hx_t_post_bfpt_fixup()
>>>>
>>>>  drivers/mtd/spi/spi-nor-core.c | 155 +++++++++++++++++++++++++++++++++
>>>>  include/linux/mtd/spi-nor.h    |   3 +
>>>>  2 files changed, 158 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>>>> index 8d63681cb3..315e26ab27 100644
>>>> --- a/drivers/mtd/spi/spi-nor-core.c
>>>> +++ b/drivers/mtd/spi/spi-nor-core.c
> [...]
>>>> +static int s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
>>>> +				   const struct sfdp_parameter_header *header,
>>>> +				   const struct sfdp_bfpt *bfpt,
>>>> +				   struct spi_nor_flash_parameter *params)
>>>> +{
>>>> +	int ret;
>>>> +	u32 addr;
>>>> +	u8 cfr3v;
>>>> +
>>>> +	/* erase size in case it is set to 4K from BFPT */
>>>> +	nor->erase_opcode = SPINOR_OP_SE_4B;
>>>> +	nor->mtd.erasesize = nor->info->sector_size;
>>>> +
>>>> +	/* Enter 4-byte addressing mode for Read Any Register */
>>>> +	ret = set_4byte(nor, nor->info, 1);
>>>> +	if (ret)
>>>> +		return ret;
>>>
>>> You enter 4byte addressing but don't exit it before returning. Is this 
>>> intentional?
>>>
>> Yes. The nor->addr_width must be 4 for read/write/erase to work correctly.
>> I think device's addressing mode should be in sync with nor->addr_width.
> 
> Right. But the comment above implies that 4-byte mode is only enabled 
> for the "Read Any Register" command. Either remove the comment 
> completely or make it clear that you are changing to 4-byte for all 
> further operations, not just the Read Any Register command.
> 
Indeed. The comment is bad. Will fix.

Thanks,
Takahiro