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

[Michael Walle <michael@walle.cc>]

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?

>>> +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.

>> 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.

>>> +}
>>> +
>>> +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.

-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/