[PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[Yicong Yang]

As per SFDP(JESD216D, Section 6.5.3) says of SMPT 1st DWORD, 11b of
bit[23:22] and 1111b of bit[19:16] represent variable
{address length, read latency}, which means "the {address length,
read latency} last set in memory device and this same value is used in the
configuration dectection command". Currently we use address length
and dummy byte of struct spi_nor in such conditions. But the value
are 0 as they are not set at the time, which will lead to
wrong perform of the dectection command.

As the last command is read SFDP(1S-1S-1S, the only mode we use in kernel),
with 3-byte address and 8 dummy cycles, use the same values in
variable situations to perform correct dectection command.

Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/mtd/spi-nor/sfdp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index f6038d3..27a8faa 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -624,7 +624,7 @@ static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
 		return 4;
 	case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
 	default:
-		return nor->addr_width;
+		return 3;
 	}
 }

@@ -641,7 +641,7 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
 	u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);

 	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
-		return nor->read_dummy;
+		return 8;
 	return read_dummy;
 }

--
2.8.1


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

Re: [PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[Alexander Sverdlin]

Hi!

On 21/04/2020 05:30, Yicong Yang wrote:
> As per SFDP(JESD216D, Section 6.5.3) says of SMPT 1st DWORD, 11b of
> bit[23:22] and 1111b of bit[19:16] represent variable
> {address length, read latency}, which means "the {address length,
> read latency} last set in memory device and this same value is used in the
> configuration dectection command". Currently we use address length
> and dummy byte of struct spi_nor in such conditions. But the value
> are 0 as they are not set at the time, which will lead to
> wrong perform of the dectection command.
> 
> As the last command is read SFDP(1S-1S-1S, the only mode we use in kernel),
> with 3-byte address and 8 dummy cycles, use the same values in
> variable situations to perform correct dectection command.

Well this will work and as the maintainers ignore the proper and even recommended by them solution,

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/mtd/spi-nor/sfdp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index f6038d3..27a8faa 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -624,7 +624,7 @@ static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
>  		return 4;
>  	case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
>  	default:
> -		return nor->addr_width;
> +		return 3;
>  	}
>  }
> 
> @@ -641,7 +641,7 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
>  	u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);
> 
>  	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
> -		return nor->read_dummy;
> +		return 8;
>  	return read_dummy;
>  }

-- 
Best regards,
Alexander Sverdlin.

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

Re: [PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[Tudor.Ambarus]

On Tuesday, April 21, 2020 10:54:45 AM EEST Alexander Sverdlin wrote:
> Well this will work and as the maintainers ignore the proper and even
> recommended by them solution,

There will be no patch left behind or ignored. Your patch is still pending in 
the review state because I need some time to study the problem and I couldn't 
allocate time for this yet. But I will address it in a week or so.

Cheers,
ta


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

Re: [PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[Alexander Sverdlin]

Hi!

On 21/04/2020 11:58, Tudor.Ambarus@microchip.com wrote:
>> Well this will work and as the maintainers ignore the proper and even
>> recommended by them solution,
> There will be no patch left behind or ignored. Your patch is still pending in 
> the review state because I need some time to study the problem and I couldn't 
> allocate time for this yet. But I will address it in a week or so.

With all the rework of spi-nor it barely makes sense and if I rework the patches
there is no way they can be backported to stable. I wonder why at all subsystem
rework is prioritized over bugfixes in MTD.

-- 
Best regards,
Alexander Sverdlin.

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

Re: [PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[Tudor.Ambarus]

On Tuesday, April 21, 2020 1:39:14 PM EEST Alexander Sverdlin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi!

Hi,

> 
> On 21/04/2020 11:58, Tudor.Ambarus@microchip.com wrote:
> >> Well this will work and as the maintainers ignore the proper and even
> >> recommended by them solution,
> > 
> > There will be no patch left behind or ignored. Your patch is still pending
> > in the review state because I need some time to study the problem and I
> > couldn't allocate time for this yet. But I will address it in a week or
> > so.
> With all the rework of spi-nor it barely makes sense and if I rework the
> patches there is no way they can be backported to stable. I wonder why at
> all subsystem rework is prioritized over bugfixes in MTD.
 
You don't have to send a new version, I can review it in the form it is now. 
Also, I can port the patch to the latest spi-nor/next if I'll find it good, 
and you'll not feel like respining. Once we agree on the final form of the 
patch, we can backport it to stable if you care.

I prioritized the subsystem rework because it will be easier/clearer for 
others to write new code, and because it eased the maintainance. Maybe it was 
better to fix all the known bugs before reworking the subsystem, but that is a 
lesson to learn.

Cheers,
ta


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

Re: [PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[Pratyush Yadav]

On 21/04/20 11:30AM, Yicong Yang wrote:
> As per SFDP(JESD216D, Section 6.5.3) says of SMPT 1st DWORD, 11b of
> bit[23:22] and 1111b of bit[19:16] represent variable
> {address length, read latency}, which means "the {address length,
> read latency} last set in memory device and this same value is used in the
> configuration dectection command". Currently we use address length
> and dummy byte of struct spi_nor in such conditions. But the value
> are 0 as they are not set at the time, which will lead to
> wrong perform of the dectection command.
> 
> As the last command is read SFDP(1S-1S-1S, the only mode we use in kernel),
> with 3-byte address and 8 dummy cycles, use the same values in
> variable situations to perform correct dectection command.
> 
> Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

FWIW, I posted a fix for the address width problem here [0], though in a 
slightly different way. I'll re-roll the series soon, since it is based 
on the code structure before the split.

> ---
>  drivers/mtd/spi-nor/sfdp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index f6038d3..27a8faa 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -624,7 +624,7 @@ static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
>  		return 4;
>  	case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
>  	default:
> -		return nor->addr_width;
> +		return 3;

If in the future someone implements SFDP parsing in 8D-8D-8D mode, they 
would want to use an address length of 4. Similarly, if someone has a 
use-case where they have to configure their flash to a 4-byte addressing 
mode in a non-volatile way, they would set nor->addr_width in a 
flash-specific hook (like the post-BFPT hook) and would expect that 
width to be used here as well.

So I think instead of setting it in stone like this, we should instead 
set the default nor->addr_width to 3 if it is configurable, and then let 
flashes fix it up if they need to. This is what my patch [0] does.

>  	}
>  }
> 
> @@ -641,7 +641,7 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
>  	u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);
> 
>  	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
> -		return nor->read_dummy;
> +		return 8;

Same comment here. Set nor->read_dummy to a sane default (== 8) and then 
let flash-specific hooks change it if they need to. This is tricky 
though, since BFPT doesn't tell us the dummy cycles needed. Still, I 
think if we set it in say spi_nor_parse_smpt(), it would be a bit more 
flexible.

>  	return read_dummy;
>  }

[0] https://lore.kernel.org/linux-mtd/20200313154645.29293-6-p.yadav@ti.com/

-- 
Regards,
Pratyush Yadav

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

Re: [PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[Yicong Yang]

Hi Pratyush,


On 2020/4/21 19:44, Pratyush Yadav wrote:
> On 21/04/20 11:30AM, Yicong Yang wrote:
>> As per SFDP(JESD216D, Section 6.5.3) says of SMPT 1st DWORD, 11b of
>> bit[23:22] and 1111b of bit[19:16] represent variable
>> {address length, read latency}, which means "the {address length,
>> read latency} last set in memory device and this same value is used in the
>> configuration dectection command". Currently we use address length
>> and dummy byte of struct spi_nor in such conditions. But the value
>> are 0 as they are not set at the time, which will lead to
>> wrong perform of the dectection command.
>>
>> As the last command is read SFDP(1S-1S-1S, the only mode we use in kernel),
>> with 3-byte address and 8 dummy cycles, use the same values in
>> variable situations to perform correct dectection command.
>>
>> Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> FWIW, I posted a fix for the address width problem here [0], though in a 
> slightly different way. I'll re-roll the series soon, since it is based 
> on the code structure before the split.
>
>> ---
>>  drivers/mtd/spi-nor/sfdp.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index f6038d3..27a8faa 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -624,7 +624,7 @@ static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
>>  		return 4;
>>  	case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
>>  	default:
>> -		return nor->addr_width;
>> +		return 3;
> If in the future someone implements SFDP parsing in 8D-8D-8D mode, they 
> would want to use an address length of 4. Similarly, if someone has a 
> use-case where they have to configure their flash to a 4-byte addressing 
> mode in a non-volatile way, they would set nor->addr_width in a 
> flash-specific hook (like the post-BFPT hook) and would expect that 
> width to be used here as well.
>
> So I think instead of setting it in stone like this, we should instead 
> set the default nor->addr_width to 3 if it is configurable, and then let 
> flashes fix it up if they need to. This is what my patch [0] does.

As I mentioned in the commit, *currently* we use 1S-1S-1S mode to read SFDP, which is also
the last operation we did here. So I use the same address byte and dummy cycles here.
I think we won't meet the exception you mentioned, as it will fail when parsing SFDP
and won't come here.

I agree that currently we don't meet all the conditions. But your patch seems alike the way
here. AS you set address width when parsing BFPT, but it'll not be changed before parsing SMPT
here, we parse smpt once bfpt is parsed and don't do specific vendor operations, etc.


>
>>  	}
>>  }
>>
>> @@ -641,7 +641,7 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
>>  	u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);
>>
>>  	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
>> -		return nor->read_dummy;
>> +		return 8;
> Same comment here. Set nor->read_dummy to a sane default (== 8) and then 
> let flash-specific hooks change it if they need to. This is tricky 
> though, since BFPT doesn't tell us the dummy cycles needed. Still, I 
> think if we set it in say spi_nor_parse_smpt(), it would be a bit more 
> flexible.

As I've mentioned above, it follows the behaviour in the current framework. What do you think of
introduce new field in spi_nor to provide more flexibility:

    struct spi_nor {
        ...
        sfdp_addr_width;
        sfdp_read_dummy;
        ...
    }

we can initialized it in the quite beginning(perhap in spi_nor_scan()) and let others to modify it.
It will also provide sfdp access and flash access with different parameter if necessary.


>>  	return read_dummy;
>>  }
> [0] https://lore.kernel.org/linux-mtd/20200313154645.29293-6-p.yadav@ti.com/
>


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

Re: [PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[Pratyush Yadav]

On 21/04/20 08:25PM, Yicong Yang wrote:
> Hi Pratyush,
> 
> 
> On 2020/4/21 19:44, Pratyush Yadav wrote:
> > On 21/04/20 11:30AM, Yicong Yang wrote:
> >> As per SFDP(JESD216D, Section 6.5.3) says of SMPT 1st DWORD, 11b of
> >> bit[23:22] and 1111b of bit[19:16] represent variable
> >> {address length, read latency}, which means "the {address length,
> >> read latency} last set in memory device and this same value is used in the
> >> configuration dectection command". Currently we use address length
> >> and dummy byte of struct spi_nor in such conditions. But the value
> >> are 0 as they are not set at the time, which will lead to
> >> wrong perform of the dectection command.
> >>
> >> As the last command is read SFDP(1S-1S-1S, the only mode we use in kernel),
> >> with 3-byte address and 8 dummy cycles, use the same values in
> >> variable situations to perform correct dectection command.
> >>
> >> Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> > FWIW, I posted a fix for the address width problem here [0], though in a 
> > slightly different way. I'll re-roll the series soon, since it is based 
> > on the code structure before the split.
> >
> >> ---
> >>  drivers/mtd/spi-nor/sfdp.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> >> index f6038d3..27a8faa 100644
> >> --- a/drivers/mtd/spi-nor/sfdp.c
> >> +++ b/drivers/mtd/spi-nor/sfdp.c
> >> @@ -624,7 +624,7 @@ static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
> >>  		return 4;
> >>  	case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
> >>  	default:
> >> -		return nor->addr_width;
> >> +		return 3;
> > If in the future someone implements SFDP parsing in 8D-8D-8D mode, they 
> > would want to use an address length of 4. Similarly, if someone has a 
> > use-case where they have to configure their flash to a 4-byte addressing 
> > mode in a non-volatile way, they would set nor->addr_width in a 
> > flash-specific hook (like the post-BFPT hook) and would expect that 
> > width to be used here as well.
> >
> > So I think instead of setting it in stone like this, we should instead 
> > set the default nor->addr_width to 3 if it is configurable, and then let 
> > flashes fix it up if they need to. This is what my patch [0] does.
> 
> As I mentioned in the commit, *currently* we use 1S-1S-1S mode to read SFDP, which is also
> the last operation we did here. So I use the same address byte and dummy cycles here.
> I think we won't meet the exception you mentioned, as it will fail when parsing SFDP
> and won't come here.
> 
> I agree that currently we don't meet all the conditions. But your patch seems alike the way
> here. AS you set address width when parsing BFPT, but it'll not be changed before parsing SMPT
> here, we parse smpt once bfpt is parsed and don't do specific vendor operations, etc.
 
Even if we don't do it right now, I don't see the harm in making a 
future dev's life just a little bit easier by not forcing them to find 
this problem and fix this just the way I suggested. If you do it that 
way, you still end up using an address width of 3 so it makes no 
difference to you.

But this hypothetical situation aside, there is the problem of when 
someone wants to use a flash with the address width configured to 4 on 
boot. Once the BFPT is parsed, the post-BFPT hook runs. Here, if you are 
using a non-default address width, you'd check the flash configuration 
to see if 4-byte addressing is used, and update nor->addr_width 
accordingly. If you hard-code 3 there, then SMPT parsing would fail in 
such a situation. At $DAYJOB, we have a use-case like this where we use 
a flash configured in 4-byte addressing.

> >> @@ -641,7 +641,7 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
> >>  	u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);
> >>
> >>  	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
> >> -		return nor->read_dummy;
> >> +		return 8;
> > Same comment here. Set nor->read_dummy to a sane default (== 8) and then 
> > let flash-specific hooks change it if they need to. This is tricky 
> > though, since BFPT doesn't tell us the dummy cycles needed. Still, I 
> > think if we set it in say spi_nor_parse_smpt(), it would be a bit more 
> > flexible.
> 
> As I've mentioned above, it follows the behaviour in the current framework. What do you think of
> introduce new field in spi_nor to provide more flexibility:
> 
>     struct spi_nor {
>         ...
>         sfdp_addr_width;
>         sfdp_read_dummy;
>         ...
>     }
> 
> we can initialized it in the quite beginning(perhap in spi_nor_scan()) and let others to modify it.
> It will also provide sfdp access and flash access with different parameter if necessary.

I think you've misunderstood what the address width and dummy cycles are 
for. They are used for the configuration detection command, which is 
used to find out which configuration the flash is in, and then use the 
appropriate sector map for that configuration.

This configuration is unlikely to be in the SFDP table. AFAIK, SFDP 
table is static and won't change based on flash config. Instead, this 
configuration will likely be in a flash register. For example, on the 
Cypress S28 flash family, it lies in the register CFR3.

The latency cycles needed for accessing these registers are completely 
unrelated to the latency cycles needed for SFDP. As an example, on the 
S28 family, you need 8 latency cycles for reading SFDP, but need 3-6 
(configurable) cycles for reading a volatile register, like CFR3. So, 
using the SFDP address width and the SFDP latency cycles for these 
commands would be completely incorrect, and would only work by chance.

You should instead populate the dummy cycles for your flash in 
nor->read_dummy, and the address width in nor->addr_width. If these 
can't be properly detected by BFPT, please use the post-BFPT hook 
instead, which runs before SMPT (or any optional table for that matter) 
parsing.
 
> >>  	return read_dummy;
> >>  }
> > [0] https://lore.kernel.org/linux-mtd/20200313154645.29293-6-p.yadav@ti.com/
> >

-- 
Regards,
Pratyush Yadav

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

Re: [PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[Yicong Yang]

On 2020/4/22 21:51, Pratyush Yadav wrote:
> On 21/04/20 08:25PM, Yicong Yang wrote:
>> Hi Pratyush,
>>
>>
>> On 2020/4/21 19:44, Pratyush Yadav wrote:
>>> On 21/04/20 11:30AM, Yicong Yang wrote:
>>>> As per SFDP(JESD216D, Section 6.5.3) says of SMPT 1st DWORD, 11b of
>>>> bit[23:22] and 1111b of bit[19:16] represent variable
>>>> {address length, read latency}, which means "the {address length,
>>>> read latency} last set in memory device and this same value is used in the
>>>> configuration dectection command". Currently we use address length
>>>> and dummy byte of struct spi_nor in such conditions. But the value
>>>> are 0 as they are not set at the time, which will lead to
>>>> wrong perform of the dectection command.
>>>>
>>>> As the last command is read SFDP(1S-1S-1S, the only mode we use in kernel),
>>>> with 3-byte address and 8 dummy cycles, use the same values in
>>>> variable situations to perform correct dectection command.
>>>>
>>>> Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
>>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>>> FWIW, I posted a fix for the address width problem here [0], though in a 
>>> slightly different way. I'll re-roll the series soon, since it is based 
>>> on the code structure before the split.
>>>
>>>> ---
>>>>  drivers/mtd/spi-nor/sfdp.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>>> index f6038d3..27a8faa 100644
>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>> @@ -624,7 +624,7 @@ static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
>>>>  		return 4;
>>>>  	case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
>>>>  	default:
>>>> -		return nor->addr_width;
>>>> +		return 3;
>>> If in the future someone implements SFDP parsing in 8D-8D-8D mode, they 
>>> would want to use an address length of 4. Similarly, if someone has a 
>>> use-case where they have to configure their flash to a 4-byte addressing 
>>> mode in a non-volatile way, they would set nor->addr_width in a 
>>> flash-specific hook (like the post-BFPT hook) and would expect that 
>>> width to be used here as well.
>>>
>>> So I think instead of setting it in stone like this, we should instead 
>>> set the default nor->addr_width to 3 if it is configurable, and then let 
>>> flashes fix it up if they need to. This is what my patch [0] does.
>> As I mentioned in the commit, *currently* we use 1S-1S-1S mode to read SFDP, which is also
>> the last operation we did here. So I use the same address byte and dummy cycles here.
>> I think we won't meet the exception you mentioned, as it will fail when parsing SFDP
>> and won't come here.
>>
>> I agree that currently we don't meet all the conditions. But your patch seems alike the way
>> here. AS you set address width when parsing BFPT, but it'll not be changed before parsing SMPT
>> here, we parse smpt once bfpt is parsed and don't do specific vendor operations, etc.
>  
> Even if we don't do it right now, I don't see the harm in making a 
> future dev's life just a little bit easier by not forcing them to find 
> this problem and fix this just the way I suggested. If you do it that 
> way, you still end up using an address width of 3 so it makes no 
> difference to you.

I don't mean to not solve the issue, I just want to explain why I do it in such a way here.

> But this hypothetical situation aside, there is the problem of when 
> someone wants to use a flash with the address width configured to 4 on 
> boot. Once the BFPT is parsed, the post-BFPT hook runs. Here, if you are 
> using a non-default address width, you'd check the flash configuration 
> to see if 4-byte addressing is used, and update nor->addr_width 
> accordingly. If you hard-code 3 there, then SMPT parsing would fail in 
> such a situation. At $DAYJOB, we have a use-case like this where we use 
> a flash configured in 4-byte addressing.
>>>> @@ -641,7 +641,7 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
>>>>  	u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);
>>>>
>>>>  	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
>>>> -		return nor->read_dummy;
>>>> +		return 8;
>>> Same comment here. Set nor->read_dummy to a sane default (== 8) and then 
>>> let flash-specific hooks change it if they need to. This is tricky 
>>> though, since BFPT doesn't tell us the dummy cycles needed. Still, I 
>>> think if we set it in say spi_nor_parse_smpt(), it would be a bit more 
>>> flexible.
>> As I've mentioned above, it follows the behaviour in the current framework. What do you think of
>> introduce new field in spi_nor to provide more flexibility:
>>
>>     struct spi_nor {
>>         ...
>>         sfdp_addr_width;
>>         sfdp_read_dummy;
>>         ...
>>     }
>>
>> we can initialized it in the quite beginning(perhap in spi_nor_scan()) and let others to modify it.
>> It will also provide sfdp access and flash access with different parameter if necessary.
> I think you've misunderstood what the address width and dummy cycles are 
> for. They are used for the configuration detection command, which is 
> used to find out which configuration the flash is in, and then use the 
> appropriate sector map for that configuration.
> This configuration is unlikely to be in the SFDP table. AFAIK, SFDP 
> table is static and won't change based on flash config. Instead, this 
> configuration will likely be in a flash register. For example, on the 
> Cypress S28 flash family, it lies in the register CFR3.
>
> The latency cycles needed for accessing these registers are completely 
> unrelated to the latency cycles needed for SFDP. As an example, on the 
> S28 family, you need 8 latency cycles for reading SFDP, but need 3-6 
> (configurable) cycles for reading a volatile register, like CFR3. So, 
> using the SFDP address width and the SFDP latency cycles for these 
> commands would be completely incorrect, and would only work by chance.

I get you point the latency and address width can be configured.

Seems I misunderstood "last set" to "last used". Thanks for pointing
it out.

>
> You should instead populate the dummy cycles for your flash in 
> nor->read_dummy, and the address width in nor->addr_width. If these 
> can't be properly detected by BFPT, please use the post-BFPT hook 
> instead, which runs before SMPT (or any optional table for that matter) 
> parsing.

I think it's the right method. Thanks.

Regards,
Yicong


>  
>>>>  	return read_dummy;
>>>>  }
>>> [0] https://lore.kernel.org/linux-mtd/20200313154645.29293-6-p.yadav@ti.com/
>>>


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