Re: [PATCH] mtd: spi-nor: spansion: Add support for Infineon S25FS256T

[Takahiro Kuwano <tkuw584924@gmail.com>]

On 12/20/2022 7:01 PM, Michael Walle wrote:
> Am 2022-12-20 09:31, schrieb Takahiro Kuwano:
> 
> 
>>>> +static int
>>>> +s25fs256t_post_bfpt_fixup(struct spi_nor *nor,
>>>> +              const struct sfdp_parameter_header *bfpt_header,
>>>> +              const struct sfdp_bfpt *bfpt)
>>>> +{
>>>> +    struct spi_mem_op op;
>>>> +    int ret;
>>>> +
>>>> +    /* 4-byte address mode is enabled by default */
>>>> +    nor->params->addr_mode_nbytes = 4;
>>>
>>> Shouldn't this already be set in spi_nor_parse_4bait()?
>>>
>> No, params->addr_mode_nbytes is initialized in spi_nor_parse_bfpt().
>> In spi_nor_parse_4bait(), params->addr_nbytes is set to 4.
> 
> Ah right, my bad. In any case, shouldn't this read the actual
> mode from the flash given that this is a non-volatile setting?
> 
To read out current address mode, we need to issue Read Any Reg with correct
address length (chicken-and-egg).

Please refer to commit description in:
https://patchwork.ozlabs.org/project/linux-mtd/patch/20220725092505.446315-6-tudor.ambarus@microchip.com/

And discussion about address mode discovery:
https://patchwork.ozlabs.org/project/linux-mtd/patch/e47ed0aa3e1a6fdca7689434ce7dea99ff4826e7.1659764848.git.Takahiro.Kuwano@infineon.com/

Unfortunately, this device does not support CRC...
https://patchwork.ozlabs.org/project/linux-mtd/patch/53d37eead9bdd09744af555b93381af477643a46.1660291281.git.Takahiro.Kuwano@infineon.com/

Until we develop good solution, I would rely on factory default setting.

>>>> +static void s25fs256t_post_sfdp_fixup(struct spi_nor *nor)
>>>> +{
>>>> +    struct spi_nor_flash_parameter *params = nor->params;
>>>> +
>>>> +    /*
>>>> +     * READ_FAST is omitted in 4BAIT parse since OP_READ_FAST_4B(0Ch) is not
>>>> +     * supported. Enable OP_READ_FAST(0Bh) that can work in 4-byte address
>>>> +     * mode.
>>>> +     */
>>>> +    params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
>>>> +    spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST], 0, 8,
>>>> +                  SPINOR_OP_READ_FAST, SNOR_PROTO_1_1_1);
>>>
>>> Mh, this requires mode switching, the advantage of the opcodes in
>>> the 4bait table are that they don't require mode switching.
>>> OP_READ_FAST doesn't work here if the address mode is set to 3. (I
>>> know this flash defaults to 1, but there is also a non-volatile
>>> setting for this).
>>>
>>> Regarding mode switching, I guess this is wrong for this flash, because
>>> it is set to spansion_set_4byte_addr_mode() by default while it should
>>> really be set to spi_nor_set_4byte_addr_mode().
>>>
>> Let me just drop fast read support so far and mention this in commit
>> description.
> 
> Then the flash will likely be impossible to be used in x1 mode, because
> we don't support different frequencies for now. And the SPI frequency
> will likely be too fast for the normal read command.
> If you skip that, then please make sure, the flash doesn't probe at all.
> I.e. returning ENODEV like above.
> 
> Maybe we can use the spi bus max_frequency as an indicator if you can
> use the normal read opcode (4b variant of course). I've just looked
> at some spi drivers and not all are using the speed_hz property of
> an SPI transfer message, but just use the max_speed_hz of the SPI
> device. Sigh.
> 
I would like to leave it to spi_nor_spimem_check_readop(). If controller is
running or can adapt frequency to normal read, it is possible to use in x1
mode. If not, spi_nor_spimem_check_readop() should return error and probe
will be failed.

>>> Also I'm not sure when set_4byte_addr_mode() is called during init.
>>> It seems slightly wrong to me because it will check wether
>>> SNOR_F_4B_OPCODES is set. But in the restore path, it is checked for
>>> !SNOR_F_4B_OPCODES before 3 byte mode is enabled again. Mhh.
>>>
>>>> +
>>>> +    /* PP_1_1_4_4B is supported but missing in SFDP. */
>>>> +    params->hwcaps.mask |= SNOR_HWCAPS_PP_1_1_4;
>>>> +    spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_1_1_4],
>>>> +                SPINOR_OP_PP_1_1_4_4B,
>>>> +                SNOR_PROTO_1_1_4);
>>>> +}
>>>> +
>>>> +static void s25fs256t_late_init(struct spi_nor *nor)
>>>> +{
>>>> +    /* The writesize should be ECC data unit size */
>>>> +    nor->params->writesize = 16;
>>>
>>> The datasheets mentions, a PP should be either 128 or 256 (for best
>>> performance). So why 16?
>>>
>> writesize is used as minimum IO size in UBI.
>> Please see:
>> https://lore.kernel.org/linux-mtd/20201118182459.18197-1-p.yadav@ti.com/
> 
> Ok. maybe it would make sense to amend that comment that the program
> granularity is 16bytes for this flash, otherwise ECC will be disabled
> if the same page is programmed again without an erase. See ch. 5.3.1
> in the datasheet.
> 
OK.

>>>> +}
>>>> +
>>>> +static struct spi_nor_fixups s25fs256t_fixups = {
>>>> +    .post_bfpt = s25fs256t_post_bfpt_fixup,
>>>> +    .post_sfdp = s25fs256t_post_sfdp_fixup,
>>>> +    .late_init = s25fs256t_late_init,
>>>> +};
>>>> +
>>>>  static int
>>>>  s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
>>>>              const struct sfdp_parameter_header *bfpt_header,
>>>> @@ -441,6 +502,9 @@ static const struct flash_info spansion_nor_parts[] = {
>>>>      { "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512)
>>>>          NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>>>>          FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
>>>> +    { "s25fs256t",  INFO6(0x342b19, 0x0f0890, 128 * 1024, 256)
>>>
>>> Does INFO6(0x342b19, 0x0f0890, 0, 0) work for you?
>>>
>> Yes, it works.
> 
> Please use that one then. I'm planning to replace all the INFO()/INFO6()
> in the future and drop the sizes because they will get fetched from
> SFDP.
> 
OK.

> -michael
> 
>>>> +        PARSE_SFDP
>>>> +        .fixups = &s25fs256t_fixups },
>>>>      { "s25hl512t",  INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256)
>>>>          PARSE_SFDP
>>>>          MFR_FLAGS(USE_CLSR)
>>
>> Thanks,
>> Takahiro

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/