Re: [PATCH 3/4] mtd: spi-nor-core: Rework default_init() to take flash_parameter
On 4/15/2024 4:27 PM, Tudor Ambarus wrote: > > > On 4/15/24 08:09, Takahiro Kuwano wrote: >> Hi Tudor, > > Hi! > >> >> On 4/15/2024 3:47 PM, Tudor Ambarus wrote: >>> >>> >>> On 4/15/24 05:33, tkuw584...@gmail.com wrote: >>>> From: Takahiro Kuwano >>>> >>>> default_init() fixup hook should be used to initialize flash parameters >>>> when its information is not provided in SFDP. To support that case, it >>>> needs to take flash_parameter structure like as other hooks. >>>> >>>> Signed-off-by: Takahiro Kuwano >>>> --- >>> >>> I'd like to get rid of the default_init hook, let's not extend it if >>> possible. Can you use the late_init hook instead? >>> >> It looks easy to migrate from default_init to late_init so I will do it. >> Could you provide the links to related discussion in Linux MTD side so that >> I can summarize it in commit message? >> > > I can't, I don't remember if I brought this up or when, but I can > explain why. > > default_init() is wrong, it contributes to the maze of initializing > flash parameters. We'd like to get rid of it because the flash > parameters that it initializes are not really used at SFDP parsing time, > thus they can be initialized later. > > Ideally we want SFDP to initialize all the flash parameters. If (when) > SFDP tables are wrong, we fix them with the post_sfdp/bfpt hooks, to > emphasize that SFDP is indeed wrong. When there are parameters that are > not covered by SFDP, we initialize them in late_init() - these > parameters have nothing to do with SFDP and they are not needed earlier. > With this we'll have a clearer view of who initializes what. > > Feel free to use this in the commit message if you think it helps. > Cheers, > ta Of course it helps!
Re: [PATCH 3/4] mtd: spi-nor-core: Rework default_init() to take flash_parameter
Hi Tudor, On 4/15/2024 3:47 PM, Tudor Ambarus wrote: > > > On 4/15/24 05:33, tkuw584...@gmail.com wrote: >> From: Takahiro Kuwano >> >> default_init() fixup hook should be used to initialize flash parameters >> when its information is not provided in SFDP. To support that case, it >> needs to take flash_parameter structure like as other hooks. >> >> Signed-off-by: Takahiro Kuwano >> --- > > I'd like to get rid of the default_init hook, let's not extend it if > possible. Can you use the late_init hook instead? > It looks easy to migrate from default_init to late_init so I will do it. Could you provide the links to related discussion in Linux MTD side so that I can summarize it in commit message? Thanks, Takahiro
Re: [PATCH 0/4] mtd: Make sure UBIFS does not do multi-pass page programming on flashes that don't support it
On 4/15/2024 3:56 PM, Tudor Ambarus wrote: > > > On 4/15/24 05:33, tkuw584...@gmail.com wrote: >> From: Takahiro Kuwano >> >> This series is equivalent to the one for Linux MTD submitted by >> Pratyush Yadav. >> >> https://patchwork.ozlabs.org/project/linux-mtd/list/?series=217759=* > > Ah, I see you specified it here. I'd argue it's better to mention it in > the commit message itself, it spares people searching on the ml archive. Noted, thanks!
Re: [PATCH v2 3/3] mtd: spi-nor-id: Add S25FS064S, S25FS128S, S25FS256S IDs
On 4/9/2024 8:54 PM, Pratyush Yadav wrote:
> On Tue, Apr 09 2024, tkuw584...@gmail.com wrote:
>
>> From: Takahiro Kuwano
>>
>> The S25FS064S, S25FS128S, and S25FS256S are the same family of SPI NOR
>> Flash devices with S25FS512S. Some difference depending on the device
>> densities are taken care in post SFDP fixup.
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> drivers/mtd/spi/spi-nor-core.c | 24 ++--
>> drivers/mtd/spi/spi-nor-ids.c | 3 +++
>> 2 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 9620852817..9b81b31e8e 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -3356,12 +3356,24 @@ static int s25fs_s_post_bfpt_fixup(struct spi_nor
>> *nor,
>> static void s25fs_s_post_sfdp_fixup(struct spi_nor *nor,
>> struct spi_nor_flash_parameter *params)
>> {
>> -/* READ_1_1_2 is not supported */
>> -params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_2;
>> -/* READ_1_1_4 is not supported */
>> -params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_4;
>> -/* PP_1_1_4 is not supported */
>> -params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4;
>> +/*
>> + * The S25FS064S(8MB) supports 1-1-2 and 1-1-4 commands, but params for
>> + * read ops in SFDP are wrong. The other density parts do not support
>> + * 1-1-2 and 1-1-4 commands.
>> + */
>> +if (params->size == SZ_8M) {
>> +spi_nor_set_read_settings(>reads[SNOR_CMD_READ_1_1_2],
>> + 0, 8, SPINOR_OP_READ_1_1_2,
>> + SNOR_PROTO_1_1_2);
>> +spi_nor_set_read_settings(>reads[SNOR_CMD_READ_1_1_4],
>> + 0, 8, SPINOR_OP_READ_1_1_4,
>> + SNOR_PROTO_1_1_4);
>> +} else {
>> +params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_2;
>> +params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_4;
>> +params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4;
>> +}
>> +
>
> Reviewed-by: Pratyush Yadav
>
> BTW (not directly related to this patch), I looked at the datasheet you
> provided in the cover letter and it says dual and quad I/O is supported
> for the 16 MB and 32 MB parts as well. Why do you mask them out here
> then?
>
In the datasheet,
1-2-2 and 1-4-4 read are named as dual and quad I/O read.
1-1-2 and 1-1-4 read are named as dual and quad output read.
Dual and quad I/O read (1-2-2 and 1-4-4) is supported in all S25FS-S parts.
Dual and quad output read (1-1-2 and 1-1-4) is only supported in S25FS064S.
Thank you for reviewing!
Takahiro
Re: [PATCH] mtd: spi-nor: Add support for Infineon S25FS-S family
On 4/5/2024 7:10 PM, Pratyush Yadav wrote:
> Hi,
>
> Just a couple small comments. Looks good otherwise.
>
> On Fri, Apr 05 2024, tkuw584...@gmail.com wrote:
>
>> From: Takahiro Kuwano
>>
>> The S25FS064S, S25FS128S, and S25FS256S are the same family of SPI NOR
>> Flash devices with S25FS512S. Some difference depending on the device
>> densities are taken care in fixup hooks.
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> Datasheets:
>> https://www.infineon.com/dgdl/Infineon-S25FS064S_64_Mb_8_MB_FS-S_Flash_SPI_Multi-I_O_1-DataSheet-v10_00-EN.pdf?fileId=8ac78c8c7d0d8da4017d0ed526b25412
>> https://www.infineon.com/dgdl/Infineon-S25FS128S_S25FS256S_1.8_V_Serial_Peripheral_Interface_with_Multi-I_O_MirrorBit(R)_Non-Volatile_Flash-DataSheet-v15_00-EN.pdf?fileId=8ac78c8c7d0d8da4017d0ed6b5ab5758
>> ---
>> drivers/mtd/spi/spi-nor-core.c | 28 +---
>> drivers/mtd/spi/spi-nor-ids.c | 7 +--
>> 2 files changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index f86003ca8c..9b81b31e8e 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -3299,8 +3299,10 @@ static int s25fs_s_quad_enable(struct spi_nor *nor)
>>
>> static int s25fs_s_erase_non_uniform(struct spi_nor *nor, loff_t addr)
>> {
>> +u8 opcode = nor->addr_width == 4 ? SPINOR_OP_BE_4K_4B : SPINOR_OP_BE_4K;
>> +
>> /* Support 8 x 4KB sectors at bottom */
>> -return spansion_erase_non_uniform(nor, addr, SPINOR_OP_BE_4K_4B, 0,
>> SZ_32K);
>> +return spansion_erase_non_uniform(nor, addr, opcode, 0, SZ_32K);
>
> Looks like an unrelated bugfix. Should this be a separate patch?
>
The existing S25FS512S uses 4-byte address so it's OK with 4B opcode.
But, yes, this is a bugfix and should be separated.
>> }
>>
>> static int s25fs_s_setup(struct spi_nor *nor, const struct flash_info *info,
>> @@ -3354,12 +3356,24 @@ static int s25fs_s_post_bfpt_fixup(struct spi_nor
>> *nor,
>> static void s25fs_s_post_sfdp_fixup(struct spi_nor *nor,
>> struct spi_nor_flash_parameter *params)
>> {
>> -/* READ_1_1_2 is not supported */
>> -params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_2;
>> -/* READ_1_1_4 is not supported */
>> -params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_4;
>> -/* PP_1_1_4 is not supported */
>> -params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4;
>> +/*
>> + * The S25FS064S(8MB) supports 1-1-2 and 1-1-4 commands, but params for
>> + * read ops in SFDP are wrong. The other density parts do not support
>> + * 1-1-2 and 1-1-4 commands.
>> + */
>> +if (params->size == SZ_8M) {
>> +spi_nor_set_read_settings(>reads[SNOR_CMD_READ_1_1_2],
>> + 0, 8, SPINOR_OP_READ_1_1_2,
>> + SNOR_PROTO_1_1_2);
>> +spi_nor_set_read_settings(>reads[SNOR_CMD_READ_1_1_4],
>> + 0, 8, SPINOR_OP_READ_1_1_4,
>> + SNOR_PROTO_1_1_4);
>> +} else {
>> +params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_2;
>> +params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_4;
>> +params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4;
>> +}
>> +
>> /* Use volatile register to enable quad */
>> params->quad_enable = s25fs_s_quad_enable;
>> }
>> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
>> index 4e83b8c94c..9ca1f244f0 100644
>> --- a/drivers/mtd/spi/spi-nor-ids.c
>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>> @@ -338,9 +338,12 @@ const struct flash_info spi_nor_ids[] = {
>> */
>> { INFO("s25sl032p", 0x010215, 0x4d00, 64 * 1024, 64,
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>> { INFO("s25sl064p", 0x010216, 0x4d00, 64 * 1024, 128,
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>> -{ INFO("s25fl256s0", 0x010219, 0x4d00, 256 * 1024, 128,
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>> -{ INFO("s25fl256s1", 0x010219, 0x4d01, 64 * 1024, 512,
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>> +{ INFO6("s25fl256s0", 0x010219, 0x4d0080, 256 * 1024, 128,
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>> +{ INFO6("s25fl256s1", 0x010219, 0x4d0180, 64 * 1024,
Re: [PATCH] mtd: spi-nor-core: Add fixups for s25fs512s
On 9/18/2021 2:01 AM, Pratyush Yadav wrote:
> On 15/09/21 12:49PM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> The current S25FS512S support has following issues that need to be fixed.
>>
>> - Non-uniform sectors by factory default. The setting needs to be
>> checked and assign erase hook as needed.
>> - Page size is wrongly advertised in SFDP.
>> - READ_1_1_2 (3Bh/3Ch), READ_1_1_4 (6Bh/6Ch), and PP_1_1_4 (32h/34h)
>> are not supported.
>> - Bank Address Register (BAR) is not supported.
>>
>> In addtion, volatile version of Quad Enable is used for safety.
>>
>> For future use, the fixups is assigned for S25FS-S family.
>>
>> The datasheet can be found in the following link.
>> https://www.cypress.com/file/216376/download
>>
>> Tested on Xilinx Zynq-7000 FPGA board.
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> drivers/mtd/spi/spi-nor-core.c | 101 +
>> 1 file changed, 101 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index d5d905fa5a..5e847ebf6a 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -3097,6 +3097,102 @@ static int spi_nor_setup(struct spi_nor *nor, const
>> struct flash_info *info,
>> }
>>
>> #ifdef CONFIG_SPI_FLASH_SPANSION
>> +static int s25fs_s_quad_enable(struct spi_nor *nor)
>> +{
>> +return spansion_quad_enable_volatile(nor, 0, 8);
>> +}
>> +
>> +static int s25fs_s_erase_non_uniform(struct spi_nor *nor, loff_t addr)
>> +{
>> +/* Support 8 x 4KB sectors at bottom */
>> +return spansion_erase_non_uniform(nor, addr, SPINOR_OP_BE_4K_4B, 0,
>> + SZ_32K);
>> +}
>> +
>> +static int s25fs_s_setup(struct spi_nor *nor, const struct flash_info *info,
>> + const struct spi_nor_flash_parameter *params)
>> +{
>> +int ret;
>> +u8 cfr3v;
>> +
>> +#ifdef CONFIG_SPI_FLASH_BAR
>
> Avoid using #ifdef. Prefer if (CONFIG_IS_ENABLED(SPI_FLASH_BAR) instead.
>
OK, I will do it.
>> +return -ENOTSUPP; /* Bank Address Register is not supported */
>
> Same question as the other patch I just reviewed. What would happen if
> we don't do this?
>
If we don't do this, the spi-nor tries to use BAR with 3-byte addressing
and the ops to the >16MB memory space will be performed in the first 16MB
memory space.
>> +#endif
>> +/*
>> + * Read CR3V to check if uniform sector is selected. If not, assign an
>> + * erase hook that supports non-uniform erase.
>> + */
>> +ret = spansion_read_any_reg(nor, SPINOR_REG_ADDR_CFR3V, 8, );
>
> Don't hard-code the 8. Use a #define instead.
>
I will do it.
>> +if (ret)
>> +return ret;
>> +if (!(cfr3v & CFR3V_UNHYSA))
>> +nor->erase = s25fs_s_erase_non_uniform;
>
> Ok, but you still don't check for top/bottom configuration. This is the
> same as the S25Hx flashes, right?
>
Yes, right.
>> +
>> +return spi_nor_default_setup(nor, info, params);
>> +}
>> +
>> +static void s25fs_s_default_init(struct spi_nor *nor)
>> +{
>> +nor->setup = s25fs_s_setup;
>> +}
>> +
>> +static int s25fs_s_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;
>> +u8 cfr3v;
>> +
>> +/* The erase size is set to 4K from BFPT, but it's wrong. Fix it. */
>> +nor->erase_opcode = SPINOR_OP_SE;
>> +nor->mtd.erasesize = nor->info->sector_size;
>> +
>> +if (params->size > SZ_16M) {
>> +ret = nor->write_reg(nor, SPINOR_OP_EN4B, NULL, 0);
>
> set_4byte() should call this as well, right? Why call it here?
>
The s25fs-s has SNOR_MFR_SPANSION (0x01). The set_4byte() issues
SPINOR_OP_BRWR for SNOR_MFR_SPANSION but the s25fs-s does not support it.
The s25fs-s support SPINOR_OP_EN4B instead.
>> +if (ret)
>> +return ret;
>> +nor->addr_width = 4;
>> +} else {
>> +nor->addr_width = 3;
>> +}
>> +
>> +/*
>> + * The page_size is set to 512B from BFPT, but it actually depends on
>> + * the configuration register. Look up the CF
Re: [PATCH 2/2] mtd: spi-nor-core: Add fixups for Spansion S25FL256L
On 9/18/2021 1:51 AM, Pratyush Yadav wrote:
> On 08/09/21 05:47PM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> The S25FL256L is 32MB NOR Flash that does not support Bank Address
>> Register. This fixup is activated if CONFIG_SPI_FLASH_BAR is enabled and
>> returns ENOTSUPP in setup() hook to avoid further ops.
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> drivers/mtd/spi/spi-nor-core.c | 21 +
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index d5d905fa5a..4940b35682 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -3222,6 +3222,23 @@ static struct spi_nor_fixups s25hx_t_fixups = {
>> .post_bfpt = s25hx_t_post_bfpt_fixup,
>> .post_sfdp = s25hx_t_post_sfdp_fixup,
>> };
>> +
>> +#ifdef CONFIG_SPI_FLASH_BAR
>> +static int s25fl256l_setup(struct spi_nor *nor, const struct flash_info
>> *info,
>> + const struct spi_nor_flash_parameter *params)
>> +{
>> +return -ENOTSUPP; /* Bank Address Register is not supported */
>
> I am not very familiar with BAR. Can you please explain what would
> happen if we don't do this?
>
The Bank Address Register (BAR) enables the 3-byte addressing opcodes to access
beyond 16MB memory space by switching 16MB banks. If we don't do this, the
spi-nor tries to use BAR with 3-byte addressing and the ops to the second half
of 32MB will be performed in the first half of 32MB.
>> +}
>> +
>> +static void s25fl256l_default_init(struct spi_nor *nor)
>> +{
>> +nor->setup = s25fl256l_setup;
>> +}
>> +
>> +static struct spi_nor_fixups s25fl256l_fixups = {
>> +.default_init = s25fl256l_default_init,
>> +};
>> +#endif
>> #endif
>>
>> #ifdef CONFIG_SPI_FLASH_S28HS512T
>> @@ -3644,6 +3661,10 @@ void spi_nor_set_fixups(struct spi_nor *nor)
>> break;
>> }
>> }
>> +#ifdef CONFIG_SPI_FLASH_BAR
>
> We should avoid using #ifdef as much as possible. Change this to
>
> if (CONFIG_IS_ENABLED(SPI_FLASH_BAR) && !strcmp())
>
>> +if (!strcmp(nor->info->name, "s25fl256l"))
>> +nor->fixups = _fixups;
>> +#endif
>
> *sigh* we really need to find a better way to specify fixups. Let me see
> if I can figure something out. In the meantime, this should be fine.
>
>> #endif
>>
>> #ifdef CONFIG_SPI_FLASH_S28HS512T
>> --
>> 2.25.1
>>
>
Re: [PATCH v7 00/14] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t
On 4/28/2021 3:44 PM, Takahiro Kuwano wrote: > On 4/27/2021 7:14 PM, Jagan Teki wrote: >> On Tue, Apr 27, 2021 at 7:54 AM wrote: >>> >>> From: Takahiro Kuwano >>> >>> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI. >>> >>> The summary datasheets can be found in the following links. >>> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die) >>> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die) >>> >>> The full version can be found in the following links (registration >>> required). >>> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522 >>> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503 >>> >>> Tested on Xilinx Zynq-7000 FPGA board. >>> >>> Changes since v7: >>> - Fixed return type of s25hx_t_erase_non_uniform() to 'int' >> >> If you have core changes that indeed affect most, better to send them >> only if they are fine with all builds. >> >> https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/7323 >> >> Jagan. >> > The problem is caused by a wrong #ifdef directive usage. > I will fix it, check with all builds, then submit again. > I also found the changes break x530 build. I will take another patch from Pratyush's series to prevent that. https://patchwork.ozlabs.org/project/uboot/patch/20210401193133.18129-9-p.ya...@ti.com/
Re: [PATCH v7 00/14] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t
On 4/27/2021 7:14 PM, Jagan Teki wrote: > On Tue, Apr 27, 2021 at 7:54 AM wrote: >> >> From: Takahiro Kuwano >> >> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI. >> >> The summary datasheets can be found in the following links. >> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die) >> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die) >> >> The full version can be found in the following links (registration >> required). >> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522 >> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503 >> >> Tested on Xilinx Zynq-7000 FPGA board. >> >> Changes since v7: >> - Fixed return type of s25hx_t_erase_non_uniform() to 'int' > > If you have core changes that indeed affect most, better to send them > only if they are fine with all builds. > > https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/7323 > > Jagan. > The problem is caused by a wrong #ifdef directive usage. I will fix it, check with all builds, then submit again. Best Regards, Takahiro
Re: [PATCH v6 00/14] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t
On 4/26/2021 10:19 PM, Pratyush Yadav wrote: > On 26/04/21 06:25PM, Jagan Teki wrote: >> On Mon, Apr 26, 2021 at 6:19 PM Pratyush Yadav wrote: >>> >>> On 26/04/21 06:04PM, Jagan Teki wrote: >>>> On Mon, Apr 26, 2021 at 12:30 PM Takahiro Kuwano >>>> wrote: >>>>> >>>>> Hi Jagan, >>>>> >>>>> On 4/25/2021 9:42 PM, Jagan Teki wrote: >>>>>> On Tue, Apr 20, 2021 at 9:56 AM Takahiro Kuwano >>>>>> wrote: >>>>>>> >>>>>>> On 4/19/2021 4:13 PM, Jagan Teki wrote: >>>>>>>> On Wed, Apr 7, 2021 at 9:01 AM wrote: >>>>>>>>> >>>>>>>>> From: Takahiro Kuwano >>>>>>>>> >>>>>>>>> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI. >>>>>>>>> >>>>>>>>> The summary datasheets can be found in the following links. >>>>>>>>> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single >>>>>>>>> die) >>>>>>>>> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die) >>>>>>>>> >>>>>>>>> The full version can be found in the following links (registration >>>>>>>>> required). >>>>>>>>> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522 >>>>>>>>> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503 >>>>>>>>> >>>>>>>>> Tested on Xilinx Zynq-7000 FPGA board. >>>>>>>> >>>>>>>> Any details about footprint statistics? >>>>>>>> >>>>>>> Please see arm-none-eabi-size output below. >>>>>>> >>>>>>> BEFORE patching: >>>>>>>textdata bss dec hex filename >>>>>>>6353 0 0635318d1 drivers/mtd/spi/spi-nor-core.o >>>>>>> 922 0 0 922 39a drivers/mtd/spi/spi-nor-ids.o >>>>>>>1879 0 01879 757 >>>>>>> spl/drivers/mtd/spi/spi-nor-tiny.o >>>>>>> >>>>>>> AFTER patching: >>>>>>>textdata bss dec hex filename >>>>>>>7705 12 077171e25 drivers/mtd/spi/spi-nor-core.o >>>>>>>1150 0 01150 47e drivers/mtd/spi/spi-nor-ids.o >>>>>>>1919 0 01919 77f >>>>>>> spl/drivers/mtd/spi/spi-nor-tiny.o >>>>>>> >>>>>>> arm-none-eabi-gcc (GCC) 8.2.0, Optimization level -O2. >>>>>> >>>>>> Does this series depend on any other patches or Octal DTR? >>>>> >>>>> No. This series is for Quad SPI parts and independent from any other >>>>> patches. >>>>> The #1, #2, and #3 patches in this series come from Pratyush's series [0] >>>>> that adds Octal DTR support. However, those three patches introduce >>>>> generic >>>>> hooks and do not contain any specific changes for Octal DTR. >>>> >>>> nor->erase seems improper, please have a look at the issue here. >>>> https://source.denx.de/u-boot/custodians/u-boot-spi/-/jobs/260932 >>> >>> I get a 404 on this link. Can you elaborate on what is wrong with >>> nor->erase? >> >> Building current source for 1 boards (0 threads, 40 jobs per thread) >> sandbox: w+ sandbox_spl >> + nor->erase = s25hx_t_erase_non_uniform; >> + ^ >> w+drivers/mtd/spi/spi-nor-core.c: In function 's25hx_t_setup': >> w+drivers/mtd/spi/spi-nor-core.c:2733:14: warning: assignment from >> incompatible pointer type [-Wincompatible-pointer-types] >> 0 1 0 /1 sandbox_spl > > Sandbox defines ssize_t as long and ARM defines it as int and so on. > nor->erase() returns int, so this would build fine on 32-bit platforms > but not on 64-bit ones. Changing the return type of s25hx_t_setup() to > int should fix this problem. > I will fix it. Thank you for working on this. >> >> Can you able to open this link at least? >> >> https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/7306 > > Yes. > Best Regards, Takahiro
Re: [PATCH v6 00/14] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t
Hi Jagan, On 4/25/2021 9:42 PM, Jagan Teki wrote: > On Tue, Apr 20, 2021 at 9:56 AM Takahiro Kuwano wrote: >> >> On 4/19/2021 4:13 PM, Jagan Teki wrote: >>> On Wed, Apr 7, 2021 at 9:01 AM wrote: >>>> >>>> From: Takahiro Kuwano >>>> >>>> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI. >>>> >>>> The summary datasheets can be found in the following links. >>>> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die) >>>> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die) >>>> >>>> The full version can be found in the following links (registration >>>> required). >>>> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522 >>>> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503 >>>> >>>> Tested on Xilinx Zynq-7000 FPGA board. >>> >>> Any details about footprint statistics? >>> >> Please see arm-none-eabi-size output below. >> >> BEFORE patching: >>textdata bss dec hex filename >>6353 0 0635318d1 drivers/mtd/spi/spi-nor-core.o >> 922 0 0 922 39a drivers/mtd/spi/spi-nor-ids.o >>1879 0 01879 757 spl/drivers/mtd/spi/spi-nor-tiny.o >> >> AFTER patching: >>textdata bss dec hex filename >>7705 12 077171e25 drivers/mtd/spi/spi-nor-core.o >>1150 0 01150 47e drivers/mtd/spi/spi-nor-ids.o >>1919 0 01919 77f spl/drivers/mtd/spi/spi-nor-tiny.o >> >> arm-none-eabi-gcc (GCC) 8.2.0, Optimization level -O2. > > Does this series depend on any other patches or Octal DTR? No. This series is for Quad SPI parts and independent from any other patches. The #1, #2, and #3 patches in this series come from Pratyush's series [0] that adds Octal DTR support. However, those three patches introduce generic hooks and do not contain any specific changes for Octal DTR. [0] https://patchwork.ozlabs.org/project/uboot/list/?series=237040=* Best Regards, Takahiro
Re: [PATCH v6 00/14] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t
On 4/19/2021 4:13 PM, Jagan Teki wrote: > On Wed, Apr 7, 2021 at 9:01 AM wrote: >> >> From: Takahiro Kuwano >> >> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI. >> >> The summary datasheets can be found in the following links. >> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die) >> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die) >> >> The full version can be found in the following links (registration >> required). >> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522 >> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503 >> >> Tested on Xilinx Zynq-7000 FPGA board. > > Any details about footprint statistics? > Please see arm-none-eabi-size output below. BEFORE patching: textdata bss dec hex filename 6353 0 0635318d1 drivers/mtd/spi/spi-nor-core.o 922 0 0 922 39a drivers/mtd/spi/spi-nor-ids.o 1879 0 01879 757 spl/drivers/mtd/spi/spi-nor-tiny.o AFTER patching: textdata bss dec hex filename 7705 12 077171e25 drivers/mtd/spi/spi-nor-core.o 1150 0 01150 47e drivers/mtd/spi/spi-nor-ids.o 1919 0 01919 77f spl/drivers/mtd/spi/spi-nor-tiny.o arm-none-eabi-gcc (GCC) 8.2.0, Optimization level -O2. Best Regards, Takahiro
Re: [PATCH v5 10/10] mtd: spi-nor-tiny: Add fixups for Cypress s25hl-t/s25hs-t
On 2/24/2021 9:43 PM, Pratyush Yadav wrote: > On 19/02/21 10:56AM, tkuw584...@gmail.com wrote: >> From: Takahiro Kuwano >> >> Fixes mode clocks for SPINOR_OP_READ_FAST_4B and volatile QE bit in tiny. >> The volatile QE bit function, spansion_quad_enable_volatile() supports >> dual/quad die package parts, by taking 'die_size' parameter that is used >> to iterate register update for all dies in the device. > > As explained in v4 [0], > > Nacked-by: Pratyush Yadav > > [0] > https://patchwork.ozlabs.org/project/uboot/patch/a5c3cf1353d9a621379e2fcfefc51fb44c9680c5.1611729896.git.takahiro.kuw...@infineon.com/ > I will remove this in v6.
Re: [PATCH v8 26/28] mtd: spi-nor-core: Add non-uniform erase for Spansion/Cypress
On 4/2/2021 4:31 AM, Pratyush Yadav wrote:
> From: Takahiro Kuwano
>
> Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or
> bottom, depending on the device configuration, while U-Boot supports
> uniform sector layout only.
>
> The spansion_erase_non_uniform() erases overlaid 4KB sectors,
> non-overlaid portion of normal sector, and remaining normal sectors, by
> selecting correct erase command and size based on the address to erase
> and size of overlaid portion in parameters. Since different Spansion
> flashes can use different opcode for erasing the 4K sectors, the opcode
> must pe passed in as a parameter based on the flash being used.
>
Typo: "must be passed"
> Signed-off-by: Takahiro Kuwano
> Signed-off-by: Pratyush Yadav
> [p.ya...@ti.com: Refactor the function to be compatible with nor->erase,
> make 4K opcode customizable, call spi_nor_setup_op() before executing
> the op.]
> ---
>
Thank you for refactoring. I would like to take this for the next revision
of my series. I think I need to remove spi_nor_setup_op() for my series.
> Unfortunately there is a race between this and Takahiro's series [0].
> Some of his patches depend on this series and I am taking this patch
> from his series. Not sure how to resolve this problem but I figure it is
> worth pointing out.
>
> BTW, I changed the #ifdef to CONFIG_SPI_FLASH_S28HS512T instead of
> CONFIG_SPI_FLASH_SPANSION otherwise there would be build warnings when
> the S28 config is not enabled. Once Takahiro's series lands it should be
> changed back to CONFIG_SPI_FLASH_SPANSION.
>
> [0]
> https://patchwork.ozlabs.org/project/uboot/list/?series=230084=both=*
>
> drivers/mtd/spi/spi-nor-core.c | 61 ++
> 1 file changed, 61 insertions(+)
>
> --
> 2.30.0
>
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 8934d65ce2..7637539087 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -887,6 +887,67 @@ erase_err:
> return ret;
> }
>
> +#ifdef CONFIG_SPI_FLASH_S28HS512T
> +/**
> + * spansion_erase_non_uniform() - erase non-uniform sectors for
> Spansion/Cypress
> + *chips
> + * @nor: pointer to a 'struct spi_nor'
> + * @addr:address of the sector to erase
> + * @opcode_4k: opcode for 4K sector erase
> + * @ovlsz_top: size of overlaid portion at the top address
> + * @ovlsz_btm: size of overlaid portion at the bottom address
> + *
> + * Erase an address range on the nor chip that can contain 4KB sectors
> overlaid
> + * on top and/or bottom. The appropriate erase opcode and size are chosen by
> + * address to erase and size of overlaid portion.
> + *
> + * Return: number of bytes erased on success, -errno otherwise.
> + */
> +static int spansion_erase_non_uniform(struct spi_nor *nor, u32 addr,
> + u8 opcode_4k, u32 ovlsz_top,
> + u32 ovlsz_btm)
> +{
> + struct spi_mem_op op =
> + SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 0),
> +SPI_MEM_OP_ADDR(nor->addr_width, addr, 0),
> +SPI_MEM_OP_NO_DUMMY,
> +SPI_MEM_OP_NO_DATA);
> + struct mtd_info *mtd = >mtd;
> + u32 erasesize;
> + int ret;
> +
> + /* 4KB sectors */
> + if (op.addr.val < ovlsz_btm ||
> + op.addr.val >= mtd->size - ovlsz_top) {
> + op.cmd.opcode = opcode_4k;
> + erasesize = SZ_4K;
> +
> + /* Non-overlaid portion in the normal sector at the bottom */
> + } else if (op.addr.val == ovlsz_btm) {
> + op.cmd.opcode = nor->erase_opcode;
> + erasesize = mtd->erasesize - ovlsz_btm;
> +
> + /* Non-overlaid portion in the normal sector at the top */
> + } else if (op.addr.val == mtd->size - mtd->erasesize) {
> + op.cmd.opcode = nor->erase_opcode;
> + erasesize = mtd->erasesize - ovlsz_top;
> +
> + /* Normal sectors */
> + } else {
> + op.cmd.opcode = nor->erase_opcode;
> + erasesize = mtd->erasesize;
> + }
> +
> + spi_nor_setup_op(nor, , nor->write_proto);
> +
> + ret = spi_mem_exec_op(nor->spi, );
> + if (ret)
> + return ret;
> +
> + return erasesize;
> +}
> +#endif
> +
> #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
> /* Write status register and ensure bits in mask match written values */
> static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
>
Best Regards,
Takahiro
Re: [PATCH v5 04/10] mtd: spi-nor-core: Add support for volatile QE bit
Hi Jagan,
On 2/26/2021 7:42 PM, Jagan Teki wrote:
> On Fri, Feb 19, 2021 at 7:26 AM wrote:
>>
>> From: Takahiro Kuwano
>>
>> Some of Spansion/Cypress chips support volatile version of configuration
>> registers and it is recommended to update volatile registers in the field
>> application due to a risk of the non-volatile registers corruption by
>> power interrupt. This patch adds a function to set Quad Enable bit in CFR1
>> volatile.
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> Changes in v5:
>> - Fix register address calculation, 'base | offset' -> 'base + offset'
>>
>> drivers/mtd/spi/spi-nor-core.c | 53 ++
>> include/linux/mtd/spi-nor.h| 1 +
>> 2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 2803536ed5..87c9fce408 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -1576,6 +1576,59 @@ static int spansion_read_cr_quad_enable(struct
>> spi_nor *nor)
>> return 0;
>> }
>>
>> +/**
>> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile
>> register.
>> + * @nor: pointer to a 'struct spi_nor'
>> + * @addr_base: base address of register (can be >0 in multi-die parts)
>> + * @dummy: number of dummy cycles for register read
>> + *
>> + * It is recommended to update volatile registers in the field application
>> due
>> + * to a risk of the non-volatile registers corruption by power interrupt.
>> This
>> + * function sets Quad Enable bit in CFR1 volatile.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 addr_base,
>> +u8 dummy)
>> +{
>> + u32 addr = addr_base + SPINOR_REG_ADDR_CFR1V;
>> +
>> + u8 cr;
>> + int ret;
>> +
>> + /* Check current Quad Enable bit value. */
>> + ret = spansion_read_any_reg(nor, addr, dummy, );
>
> What if we can use the exiting quad_enable hook by identifying
> volatile QE at the function beginning instead of having a separate
> call?
>
Do you mean something like this?
static int spansion_read_cr_quad_enable(struct spi_nor *nor)
{
u8 sr_cr[2];
int ret;
if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) {
u32 base;
for (base = 0; base < nor->mtd.size; base += SZ_128M) {
u32 addr = base + SPINOR_REG_ADDR_CFR1V;
/* Check current Quad Enable bit value. */
ret = spansion_read_any_reg(nor, addr, 0, _cr[1]);
[...]
ret = spansion_write_any_reg(nor, addr, sr_cr[1]);
[...]
/* Read back and check it. */
ret = spansion_read_any_reg(nor, addr, 0, _cr[1]);
[...]
}
return 0;
}
/* Check current Quad Enable bit value. */
ret = read_cr(nor);
if (ret < 0) {
dev_dbg(nor->dev,
"error while reading configuration register\n");
return -EINVAL;
}
[...]
}
Or defining a new flag like 'SNOR_F_HAS_VOLATILE_QE'?
> Jagan.
>
Best Regards,
Takahiro
Re: [PATCH v5 09/10] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t
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, tkuw584...@gmail.com wrote:
>>>> From: Takahiro Kuwano
>>>>
>>>> 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.ya...@ti.com/
>>>> [1]
>>>> https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.ya...@ti.com/
>>>> [2]
>>>> https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.ya...@ti.com/
>>>>
>>>> Signed-off-by: Takahiro Kuwano
>>>> ---
>>>> 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
Re: [PATCH v5 02/10] mtd: spi-nor-ids: Add Cypress s25hl-t/s25hs-t
Hi Jagan,
On 2/26/2021 7:35 PM, Jagan Teki wrote:
> On Fri, Feb 19, 2021 at 7:26 AM wrote:
>>
>> From: Takahiro Kuwano
>>
>> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.
>>
>> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die)
>> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)
>>
>> The full version can be found in the following links (registration
>> required).
>> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522
>> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503
>>
>> Tested on Xilinx Zynq-7000 FPGA board.
>>
>> Signed-off-by: Takahiro Kuwano
>> Reviewed-by: Pratyush Yadav
>> ---
>> Changes in v5:
>> - Remove 256Mb and 4Gb parts
>>
>> drivers/mtd/spi/spi-nor-ids.c | 27 +++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
>> index 5bd5dd3003..052a0b62ee 100644
>> --- a/drivers/mtd/spi/spi-nor-ids.c
>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>> @@ -217,6 +217,33 @@ const struct flash_info spi_nor_ids[] = {
>> { INFO("s25fl208k", 0x014014, 0, 64 * 1024, 16, SECT_4K |
>> SPI_NOR_DUAL_READ) },
>> { INFO("s25fl064l", 0x016017, 0, 64 * 1024, 128, SECT_4K |
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> { INFO("s25fl128l", 0x016018, 0, 64 * 1024, 256, SECT_4K |
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> +
>> + /*
>> +* S25HL/HS-T (Semper Flash with Quad SPI) Family has
>> user-configurable
>> +* sector architecture. By default, the 512Mb and 1Gb, single-die
>> +* package parts are configured to non-uniform that 4KB sectors
>> overlaid
>> +* on bottom address. To support this, an erase hook makes overlaid
>> +* sectors appear as uniform sectors. The 2Gb, dual-die package parts
>> +* are configured to uniform by default.
>> +*/
>
> Nice to have these comments in commit instead of id's sections.
>
I will move it to commit in v6.
Thanks,
Takahiro
Re: [PATCH v5 09/10] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t
On 2/24/2021 9:40 PM, Pratyush Yadav wrote:
> On 19/02/21 10:56AM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> 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.ya...@ti.com/
>> [1]
>> https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.ya...@ti.com/
>> [2]
>> https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.ya...@ti.com/
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> 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
>> @@ -2677,8 +2677,163 @@ static int spi_nor_init(struct spi_nor *nor)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +static int s25hx_t_mdp_ready(struct spi_nor *nor)
>> +{
>> +u32 addr;
>> +int ret;
>> +
>> +for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
>> +ret = spansion_sr_ready(nor, addr, 0);
>> +if (ret != 1)
>
> Nitpick: Use if (!ret) since spansion_sr_ready() returns a boolean
> value.
>
Will fix.
>> +return ret;
>> +}
>> +
>> +return 1;
>> +}
>> +
>> +static int s25hx_t_quad_enable(struct spi_nor *nor)
>> +{
>> +u32 addr;
>> +int ret;
>> +
>> +for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
>> +ret = spansion_quad_enable_volatile(nor, addr, 0);
>> +if (ret)
>> +return ret;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static int s25hx_t_erase_non_uniform(struct mtd_info *mtd,
>> + struct erase_info *instr)
>> +{
>> +/* Support factory default configuration (32 x 4KB sectors at bottom) */
>> +return spansion_erase_non_uniform(mtd, instr, 0, SZ_128K);
>> +}
>> +
>> +static int s25hx_t_setup(struct spi_nor *nor, const struct flash_info *info,
>> + const struct spi_nor_flash_parameter *params,
>> + const struct spi_nor_hwcaps *hwcaps)
>> +{
>> +#ifdef CONFIG_SPI_FLASH_BAR
>> +return -ENOTSUPP; /* Bank Address Register is not supported */
>> +#endif
>> +/*
>> + * The Cypress Semper family has transparent ECC. To preserve
>> + * ECC enabled, multi-pass programming within the same 16-byte
>> + * ECC data unit needs to be avoided.
>> + */
>> +nor->mtd.writesize = 16;
>
> Sorry, I forgot what your end goal was with setting the writesize. Which
> layer is doing multi-pass writes? You would need to check if those
> layers actually respect this value. In Linux some consumers of the MTD
>
Re: [PATCH v5 08/10] mtd: spi-nor-core: Add Cypress manufacturer ID in set_4byte
On 2/24/2021 9:11 PM, Pratyush Yadav wrote: > On 19/02/21 10:56AM, tkuw584...@gmail.com wrote: >> From: Takahiro Kuwano >> >> Cypress chips support SPINOR_OP_EN4B(B7h)/SPINOR_OP_EX4B(E9h) to > > The datasheet says the EN4B command is indeed B7h but EX4B is listed as > B8h. The command E9h is for "Password Unlock". So exiting 4 byte mode > will do something completely different. > How stupid of me! Thank you for thorough review.
Re: [PATCH v5 07/10] mtd: spi-nor-core: Add non-uniform erase for Spansion/Cypress
Hi Pratyush,
On 2/24/2021 9:05 PM, Pratyush Yadav wrote:
> On 19/02/21 10:56AM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or
>> bottom, depending on the device configuration, while U-Boot supports
>> uniform sector layout only.
>>
>> The spansion_erase_non_uniform() erases overlaid 4KB sectors,
>> non-overlaid portion of normal sector, and remaining normal sectors, by
>> selecting correct erase command and size based on the address to erase
>> and size of overlaid portion in parameters.
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> Changes in v5:
>> - New in v5, introduce spansion_erase_non_uniform() as a replacement
>> for spansion_overlaid_erase() in v4
>>
>> drivers/mtd/spi/spi-nor-core.c | 72 ++
>> 1 file changed, 72 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index e5fc0e7965..46948ed41b 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -793,6 +793,78 @@ erase_err:
>> return ret;
>> }
>>
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/**
>> + * spansion_erase_non_uniform() - erase non-uniform sectors for
>> Spansion/Cypress
>> + *chips
>> + * @mtd:pointer to a 'struct mtd_info'
>> + * @instr: pointer to a 'struct erase_info'
>> + * @ovlsz_top: size of overlaid portion at the top address
>> + * @ovlsz_btm: size of overlaid portion at the bottom address
>> + *
>> + * Erase an address range on the nor chip that can contain 4KB sectors
>> overlaid
>> + * on top and/or bottom. The appropriate erase opcode and size are chosen by
>> + * address to erase and size of overlaid portion.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_erase_non_uniform(struct mtd_info *mtd,
>> + struct erase_info *instr, u32 ovlsz_top,
>> + u32 ovlsz_btm)
>
> Is there any reason why you are not using the nor->erase() hook? As far
> as I can see it should also be able to perform the same erase while
> avoiding all the boilerplate needed for keeping track of address,
> remaining erase, write enable, error handling, etc.
>
I tried to use the nor-erase() hook, but I saw the erasesize problem
you mentioned below and didn't think about changing the return value of
existing function.
> One problem I can see is that you don't always increment address by
> nor->erasesize. That can change depending on which sector got erased.
> spi_nor_erase() can't currently handle that. But IMO a reasonable fix
> for this would be to return the size actually erased in nor->erase(),
> like how it is done for the unix read() and write() system calls. A
> negative value would still mean an error but now a positive return value
> will tell the caller how much data was actually erased.
>
> I think it is a relatively easy refactor and worth doing.
>
I agree. Let me introduce it in v6.
>> +{
>> +struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +struct spi_mem_op op =
>> +SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
>> + SPI_MEM_OP_ADDR(nor->addr_width, instr->addr, 1),
>> + SPI_MEM_OP_NO_DUMMY,
>> + SPI_MEM_OP_NO_DATA);
>> +u32 len = instr->len;
>> +u32 erasesize;
>> +int ret;
>
> spi_nor_erase() does some sanity checking for instr->len. Even more
> reason to use nor->erase() so we don't have to duplicate that code.
>
>> +
>> +while (len) {
>> +/* 4KB sectors */
>> +if (op.addr.val < ovlsz_btm ||
>> +op.addr.val >= mtd->size - ovlsz_top) {
>> +op.cmd.opcode = SPINOR_OP_BE_4K;
>> +erasesize = SZ_4K;
>
> Ok.
>
>> +
>> +/* Non-overlaid portion in the normal sector at the bottom */
>> +} else if (op.addr.val == ovlsz_btm) {
>> +op.cmd.opcode = nor->erase_opcode;
>> +erasesize = mtd->erasesize - ovlsz_btm;
>> +
>> +/* Non-overlaid portion in the normal sector at the top */
>> +} else if (op.addr.val == mtd->size - mtd->erasesize) {
>> +op.cmd.opcode = nor->erase_opcode;
>> +erasesize = mtd->era
Re: [PATCH v5 05/10] mtd: spi-nor-core: Add the ->ready() hook
On 2/19/2021 6:57 PM, Pratyush Yadav wrote: > On 19/02/21 10:55AM, tkuw584...@gmail.com wrote: >> From: Takahiro Kuwano >> >> For dual/quad die package devices from Spansion/Cypress, the device's >> status needs to be checked by reading status registers in all dies, by >> using Read Any Register command. To support this, a Flash specific hook >> that can overwrite the legacy status check is needed. >> >> The change in spi-nor.h depends on: >> https://patchwork.ozlabs.org/project/uboot/patch/20210205041119.145784-4-sean...@gmail.com/ >> > > This line should not be a part of the commit message. This is "metadata" > that says how the patch should be applied. One it is applied this has no > use in the Git history. So it should go below the 3 dashed lines, like > the changelog. > > Other than this, > > Reviewed-by: Pratyush Yadav > I will make it goes below the "---" line. Thank you.
Re: [PATCH v4 8/9] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t
Hi Pratyush,
On 2/2/2021 4:22 AM, Pratyush Yadav wrote:
> On 28/01/21 01:37PM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> Add nor->setup() and fixup hooks to overwrite:
>> - volatile QE bit
>> - the ->ready() hook for dual/quad die package parts
>> - overlaid erase
>> - spi_nor_flash_parameter
>> - mtd_info
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> drivers/mtd/spi/spi-nor-core.c | 108 +
>> 1 file changed, 108 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index ef49328a28..3d8cb9c333 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -2648,8 +2648,116 @@ static int spi_nor_init(struct spi_nor *nor)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +static int s25hx_t_mdp_ready(struct spi_nor *nor)
>> +{
>> +u32 addr;
>> +int ret;
>> +
>> +for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
>> +ret = spansion_sr_ready(nor, addr, 0);
>> +if (ret != 1)
>> +return ret;
>> +}
>> +
>> +return 1;
>> +}
>> +
>> +static int s25hx_t_quad_enable(struct spi_nor *nor)
>> +{
>> +u32 addr;
>> +int ret;
>> +
>> +for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
>> +ret = spansion_quad_enable_volatile(nor, addr, 0);
>
> So you need to set the QE bit on each die. Ok.
>
> Out of curiosity, what will happen if you only set the QE bit on the
> first die? Will reads from first die work in quad mode and rest in
> single mode?
>
If the host issues quad read command, only the first die works and rest
do not respond to the quad read command.
>> +if (ret)
>> +return ret;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static int s25hx_t_setup(struct spi_nor *nor, const struct flash_info *info,
>> + const struct spi_nor_flash_parameter *params,
>> + const struct spi_nor_hwcaps *hwcaps)
>> +{
>> +#ifdef CONFIG_SPI_FLASH_BAR
>> +return -ENOTSUPP; /* Bank Address Register is not supported */
>> +#endif
>> +/*
>> + * The Cypress Semper family has transparent ECC. To preserve
>> + * ECC enabled, multi-pass programming within the same 16-byte
>> + * ECC data unit needs to be avoided. Set writesize to the page
>> + * size and remove the MTD_BIT_WRITEABLE flag in mtd_info to
>> + * prevent multi-pass programming.
>> + */
>> +nor->mtd.writesize = params->page_size;
>
> The writesize should be set to 16. See [0][1][2].
>
>> +nor->mtd.flags &= ~MTD_BIT_WRITEABLE;
>
> Not needed. See discussions pointed to above.
>
OK, thank you for the information.
>> +
>> +/* Emulate uniform sector architecure by this erase hook*/
>> +nor->mtd._erase = spansion_overlaid_erase;
>> +
>> +/* For 2Gb (dual die) and 4Gb (quad die) parts */
>> +if (nor->mtd.size > SZ_128M)
>> +nor->ready = s25hx_t_mdp_ready;
>> +
>> +/* Enter 4-byte addressing mode for WRAR used in quad_enable */
>> +set_4byte(nor, info, true);
>> +
>> +return spi_nor_default_setup(nor, info, params, hwcaps);
>> +}
>> +
>> +static void s25hx_t_default_init(struct spi_nor *nor)
>> +{
>> +nor->setup = s25hx_t_setup;
>> +}
>> +
>> +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)
>> +{
>> +/* Default page size is 256-byte, but BFPT reports 512-byte */
>> +params->page_size = 256;
>
> Read the page size from the register, like it is done on Linux for S28
> flash family.
>
Will fix.
>> +/* Reset erase size in case it is set to 4K from BFPT */
>> +nor->mtd.erasesize = 0;
>
> What does erasesize of 0 mean? I would take that to mean that the flash
> does not support erases. I can't find any mention of 0 erase size in the
> documentation of struct mtd_info.
>
In this device, the erasesize is wrongly configured to 4K through BFPT
parse. I would reset it to 0 expecting the correct value is set in
spi_nor_select_erase() afterwards. But I shoul
Re: [PATCH v4 9/9] mtd: spi-nor-tiny: Add fixups for Cypress s25hl-t/s25hs-t
Hi Pratyush,
On 2/2/2021 4:40 AM, Pratyush Yadav wrote:
> On 28/01/21 01:37PM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> Fixes mode clocks for SPINOR_OP_READ_FAST_4B and volatile QE bit in tiny.
>> The volatile QE bit function, spansion_quad_enable_volatile() supports
>> dual/quad die package parts, by taking 'die_size' parameter that is used
>> to iterate register update for all dies in the device.
>
> I'm not so sure if this is a good idea. spi-nor-tiny should be the
> minimal set of functionality to get the bootloader to the next stage.
> 1S-1S-1S mode is sufficient for that. Adding quad enable functions of
> all the flashes will increase the size quite a bit. I know that some
> flashes already have their quad enable hooks, and I don't think they
> should be there either.
>
> Of course, the maintainers have the final call, but from my side,
>
> Nacked-by: Pratyush Yadav
>
I understand your point. Let's leave the decision up to the maintainers.
> Anyway, comments below in case the maintainers do plan on picking this
> patch up.
>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> drivers/mtd/spi/spi-nor-tiny.c | 89 ++
>> 1 file changed, 89 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
>> index 5cc2b7d996..66680df5a9 100644
>> --- a/drivers/mtd/spi/spi-nor-tiny.c
>> +++ b/drivers/mtd/spi/spi-nor-tiny.c
>> @@ -555,6 +555,85 @@ static int spansion_read_cr_quad_enable(struct spi_nor
>> *nor)
>> }
>> #endif /* CONFIG_SPI_FLASH_SPANSION */
>>
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/**
>> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile
>> register.
>> + * @nor:pointer to a 'struct spi_nor'
>> + * @die_size: maximum number of bytes per die ('mtd.size' >
>> 'die_size' in
>> + * multi die package parts).
>> + * @dummy: number of dummy cycles for register read
>> + *
>> + * It is recommended to update volatile registers in the field application
>> due
>> + * to a risk of the non-volatile registers corruption by power interrupt.
>> This
>> + * function sets Quad Enable bit in CFR1 volatile.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 die_size,
>> + u8 dummy)
>> +{
>> +struct spi_mem_op op =
>> +SPI_MEM_OP(SPI_MEM_OP_CMD(0, 1),
>> + SPI_MEM_OP_ADDR(4, 0, 1),
>> + SPI_MEM_OP_DUMMY(0, 1),
>> + SPI_MEM_OP_DATA_IN(1, NULL, 1));
>> +u32 addr;
>> +u8 cr;
>> +int ret;
>> +
>> +/* Use 4-byte address for RDAR/WRAR */
>> +ret = spi_nor_write_reg(nor, SPINOR_OP_EN4B, NULL, 0);
>> +if (ret < 0) {
>> +dev_dbg(nor->dev,
>> +"error while enabling 4-byte address\n");
>> +return ret;
>> +}
>> +
>> +for (addr = 0; addr < nor->mtd.size; addr += die_size) {
>> +op.addr.val = addr + SPINOR_REG_ADDR_CFR1V;
>
> So here you add the register offset to the base address, instead of
> ORing it. Ok.
>
>> +
>> +/* Check current Quad Enable bit value. */
>> +op.cmd.opcode = SPINOR_OP_RDAR;
>> +op.dummy.nbytes = dummy / 8;
>> +op.data.dir = SPI_MEM_DATA_IN;
>> +ret = spi_nor_read_write_reg(nor, , );
>> +if (ret < 0) {
>> +dev_dbg(nor->dev,
>> +"error while reading configuration register\n");
>> +return -EINVAL;
>> +}
>> +
>> +if (cr & CR_QUAD_EN_SPAN)
>> +return 0;
>> +
>> +/* Write new value. */
>> +cr |= CR_QUAD_EN_SPAN;
>> +op.cmd.opcode = SPINOR_OP_WRAR;
>> +op.dummy.nbytes = 0;
>> +op.data.dir = SPI_MEM_DATA_OUT;
>> +write_enable(nor);
>> +ret = spi_nor_read_write_reg(nor, , );
>> +if (ret < 0) {
>> +dev_dbg(nor->dev,
>> +"error while writing configuration register\n");
>> +return -EINVAL;
>> +}
>> +
>> +/*
Re: [PATCH v4 6/9] mtd: spi-nor-core: Add overlaid sector erase feature
Hi Pratyush,
On 2/2/2021 3:56 AM, Pratyush Yadav wrote:
> Hi Takahiro,
>
> On 28/01/21 01:36PM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or
>> bottom, depending on the device configuration, while U-Boot supports
>> uniform sector layout only. This patch adds an erase hook that emulates
>> uniform sector layout.
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> drivers/mtd/spi/spi-nor-core.c | 48 ++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 1c0ba5abf9..70da0081b6 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -788,6 +788,54 @@ erase_err:
>> return ret;
>> }
>>
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/*
>> + * Erase for Spansion/Cypress Flash devices that has overlaid 4KB sectors at
>> + * the top and/or bottom.
>> + */
>> +static int spansion_overlaid_erase(struct mtd_info *mtd,
>> + struct erase_info *instr)
>> +{
>> +struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +struct erase_info instr_4k;
>> +u8 opcode;
>> +u32 erasesize;
>> +int ret;
>> +
>> +/* Perform default erase operation (non-overlaid portion is erased) */
>> +ret = spi_nor_erase(mtd, instr);
>> +if (ret)
>> +return ret;
>> +
>> +/* Backup default erase opcode and size */
>> +opcode = nor->erase_opcode;
>> +erasesize = mtd->erasesize;
>> +
>> +/*
>> + * Erase 4KB sectors. Use the possible max length of 4KB sector region.
>> + * The Flash just ignores the command if the address is not configured
>> + * as 4KB sector and reports ready status immediately.
>> + */
>> +instr_4k.len = SZ_128K;
>> +nor->erase_opcode = SPINOR_OP_BE_4K_4B;
>> +mtd->erasesize = SZ_4K;
>> +if (instr->addr == 0) {
>> +instr_4k.addr = 0;
>> +ret = spi_nor_erase(mtd, _4k);
>> +}
>> +if (!ret && instr->addr + instr->len == mtd->size) {
>> +instr_4k.addr = mtd->size - instr_4k.len;
>> +ret = spi_nor_erase(mtd, _4k);
>> +}
>
> This feels like a hack to me. Does the flash datasheet explicitly say
> that erasing the overlaid area with the "normal" erase opcode is a
> no-op?
>
Sorry, I have noticed the datasheet I mentioned in the cover letter is a
summary version. Here is the link to he full version, but you need
registration to access.
https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522
So, let me quote the description about erasing overlaid area:
"If a sector erase transaction is applied to a 256 KB sector that is
overlaid by 4 KB sectors, the overlaid 4 KB sectors are not affected
by the erase. Only the visible (non-overlaid) portion of the 128 KB
or 192 KB sector is erased."
And about 4 KB sector erase:
"This transaction is ignored when the device is configured for uniform
sector only (CFR3V[3] = 1). If the Erase 4K B sector transaction is issued
to a non-4 KB sector address, the device will abort the operation and will
not set the ERSERR status fail bit."
> I don't see a big reason to run this hack. You are already in a
> flash-specific erase hook. Why not just directly issue the correct erase
> commands to the sectors? That is, why not issue 4k erase to overlaid
> sectors and normal erase to the rest? Why do you need to emulate uniform
> erase?
>
Thanks for pointing this out. I should probably hook nor->erase() instead
of mtd->_erase(), then issue 4k or normal erase depending on the address.
I will introduce that in v5.
>> +
>> +/* Restore erase opcode and size */
>> +nor->erase_opcode = opcode;
>> +mtd->erasesize = erasesize;
>> +
>> +return ret;
>> +}
>> +#endif
>> +
>> #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
>> /* Write status register and ensure bits in mask match written values */
>> static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
>> --
>> 2.25.1
>>
>
Best Regards,
Takahiro
Re: [PATCH v4 5/9] mtd: spi-nor-core: Add the ->ready() hook
Hi Pratyush,
Thank you for the feedback. I will address this in v5.
On 1/30/2021 3:49 AM, Pratyush Yadav wrote:
> Hi,
>
> On 28/01/21 01:36PM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> For dual/quad die package devices from Spansion/Cypress, the device's
>> status needs to be checked by reading status registers in all dies, by
>> using Read Any Register command. To support this, a Flash specific hook
>> that can overwrite the legacy status check is needed.
>>
>> The spansion_sr_ready() reads status register 1 by Read Any Register
>> commnad. This function is called from Flash specific hook with die address
>> and dummy cycles.
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> drivers/mtd/spi/spi-nor-core.c | 32
>> include/linux/mtd/spi-nor.h| 4 +++-
>> 2 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 624e730524..1c0ba5abf9 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -522,6 +522,35 @@ static int set_4byte(struct spi_nor *nor, const struct
>> flash_info *info,
>> }
>> }
>>
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/*
>> + * Read status register 1 by using Read Any Register command to support
>> multi
>> + * die package parts.
>> + */
>> +static int spansion_sr_ready(struct spi_nor *nor, u32 addr_base, u8 dummy)
>> +{
>> +u32 reg_addr = addr_base + SPINOR_REG_ADDR_STR1V;
>> +u8 sr;
>> +int ret;
>> +
>> +ret = spansion_read_any_reg(nor, reg_addr, dummy, );
>> +if (ret < 0)
>> +return ret;
>> +
>> +if (sr & (SR_E_ERR | SR_P_ERR)) {
>> +if (sr & SR_E_ERR)
>> +dev_dbg(nor->dev, "Erase Error occurred\n");
>> +else
>> +dev_dbg(nor->dev, "Programming Error occurred\n");
>> +
>> +nor->write_reg(nor, SPINOR_OP_CLSR, NULL, 0);
>> +return -EIO;
>> +}
>> +
>> +return !(sr & SR_WIP);
>> +}
>> +#endif
>> +
>
> Do not add this function in this patch. Just add the hook here. Add it
> in the patch that actually uses it.
>
>> static int spi_nor_sr_ready(struct spi_nor *nor)
>> {
>> int sr = read_sr(nor);
>> @@ -570,6 +599,9 @@ static int spi_nor_ready(struct spi_nor *nor)
>> {
>> int sr, fsr;
>>
>> +if (nor->ready)
>> +return nor->ready(nor);
>> +
>
> Move the code below in a separate function and call it something like
> spi_nor_default_ready(). Then call that function from here.
>
>> sr = spi_nor_sr_ready(nor);
>> if (sr < 0)
>> return sr;
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index e31073eb24..25234177de 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -434,8 +434,9 @@ struct flash_info;
>> * @flash_lock: [FLASH-SPECIFIC] lock a region of the SPI NOR
>> * @flash_unlock: [FLASH-SPECIFIC] unlock a region of the SPI NOR
>> * @flash_is_locked:[FLASH-SPECIFIC] check if a region of the SPI
>> NOR is
>> - * @quad_enable:[FLASH-SPECIFIC] enables SPI NOR quad mode
>> * completely locked
>> + * @quad_enable:[FLASH-SPECIFIC] enables SPI NOR quad mode
>> + * @ready: [FLASH-SPECIFIC] check if the flash is ready
>> * @priv: the private data
>> */
>> struct spi_nor {
>> @@ -481,6 +482,7 @@ struct spi_nor {
>> int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>> int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>> int (*quad_enable)(struct spi_nor *nor);
>> +int (*ready)(struct spi_nor *nor);
>>
>> void *priv;
>> /* Compatibility for spi_flash, remove once sf layer is merged with mtd */
>> --
>> 2.25.1
>>
>
Best Regards,
Takahiro
Re: [PATCH v4 4/9] mtd: spi-nor-core: Add support for volatile QE bit
Hi Pratyush,
On 1/30/2021 3:40 AM, Pratyush Yadav wrote:
> Hi,
>
> On 28/01/21 01:36PM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> Some of Spansion/Cypress chips support volatile version of configuration
>> registers and it is recommended to update volatile registers in the field
>> application due to a risk of the non-volatile registers corruption by
>> power interrupt. This patch adds a function to set Quad Enable bit in CFR1
>> volatile.
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> drivers/mtd/spi/spi-nor-core.c | 53 ++
>> 1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 2803536ed5..624e730524 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -1576,6 +1576,59 @@ static int spansion_read_cr_quad_enable(struct
>> spi_nor *nor)
>> return 0;
>> }
>>
>> +/**
>> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile
>> register.
>> + * @nor:pointer to a 'struct spi_nor'
>> + * @addr_base: base address of register (can be >0 in multi-die parts)
>> + * @dummy: number of dummy cycles for register read
>> + *
>> + * It is recommended to update volatile registers in the field application
>> due
>> + * to a risk of the non-volatile registers corruption by power interrupt.
>> This
>> + * function sets Quad Enable bit in CFR1 volatile.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 addr_base,
>> + u8 dummy)
>> +{
>> +u32 addr = addr_base | SPINOR_REG_ADDR_CFR1V;
>
> Why do you OR the register offset with the base? Shouldn't you be adding
> to it?
>
I missed it during review... I will fix.
>> +
>> +u8 cr;
>> +int ret;
>> +
>> +/* Check current Quad Enable bit value. */
>> +ret = spansion_read_any_reg(nor, addr, dummy, );
>> +if (ret < 0) {
>> +dev_dbg(nor->dev,
>> +"error while reading configuration register\n");
>> +return -EINVAL;
>> +}
>> +
>> +if (cr & CR_QUAD_EN_SPAN)
>> +return 0;
>> +
>> +cr |= CR_QUAD_EN_SPAN;
>> +
>> +write_enable(nor);
>> +
>> +ret = spansion_write_any_reg(nor, addr, cr);
>> +
>> +if (ret < 0) {
>> +dev_dbg(nor->dev,
>> +"error while writing configuration register\n");
>> +return -EINVAL;
>> +}
>> +
>> +/* Read back and check it. */
>> +ret = spansion_read_any_reg(nor, addr, dummy, );
>> +if (ret || !(cr & CR_QUAD_EN_SPAN)) {
>> +dev_dbg(nor->dev, "Spansion Quad bit not set\n");
>> +return -EINVAL;
>> +}
>> +
>> +return 0;
>> +}
>> +
>
> Rest of the patch LGTM.
>
>> #if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT)
>> /**
>> * spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
>> --
>> 2.25.1
>>
>
Best Regards,
Takahiro
Re: [PATCH v4 3/9] mtd: spi-nor-core: Add support for Read/Write Any Register
On 1/30/2021 3:17 AM, Pratyush Yadav wrote:
> On 28/01/21 01:36PM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> Some of Spansion/Cypress chips support Read/Write Any Register commands.
>> These commands are mainly used to write volatile registers and access to
>> the registers in second and subsequent die for multi-die package parts.
>>
>> The Read Any Register instruction (65h) is followed by register address
>> and dummy cycles, then the selected register byte is returned.
>>
>> The Write Any Register instruction (71h) is followed by register address
>> and register byte to write.
>>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> drivers/mtd/spi/spi-nor-core.c | 25 +
>> include/linux/mtd/spi-nor.h| 4
>> 2 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 34c15f1561..2803536ed5 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -211,6 +211,31 @@ static int spi_nor_write_reg(struct spi_nor *nor, u8
>> opcode, u8 *buf, int len)
>> return spi_nor_read_write_reg(nor, , buf);
>> }
>>
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +static int spansion_read_any_reg(struct spi_nor *nor, u32 addr, u8 dummy,
>> + u8 *val)
>> +{
>> +struct spi_mem_op op =
>> +SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDAR, 1),
>> + SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),
>> + SPI_MEM_OP_DUMMY(dummy / 8, 1),
>> + SPI_MEM_OP_DATA_IN(1, NULL, 1));
>> +
>> +return spi_nor_read_write_reg(nor, , val);
>> +}
>> +
>> +static int spansion_write_any_reg(struct spi_nor *nor, u32 addr, u8 val)
>> +{
>> +struct spi_mem_op op =
>> +SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRAR, 1),
>> + SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),
>> + SPI_MEM_OP_NO_DUMMY,
>> + SPI_MEM_OP_DATA_OUT(1, NULL, 1));
>> +
>> +return spi_nor_read_write_reg(nor, , );
>> +}
>> +#endif
>> +
>> static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t
>> len,
>> u_char *buf)
>> {
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index 89e7a4fdcd..e31073eb24 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -121,6 +121,10 @@
>> #define SPINOR_OP_BRWR 0x17/* Bank register write */
>> #define SPINOR_OP_BRRD 0x16/* Bank register read */
>> #define SPINOR_OP_CLSR 0x30/* Clear status register 1 */
>> +#define SPINOR_OP_RDAR 0x65/* Read any register */
>> +#define SPINOR_OP_WRAR 0x71/* Write any register */
>> +#define SPINOR_REG_ADDR_STR1V 0x0080
>> +#define SPINOR_REG_ADDR_CFR1V 0x0082
>
> These two defines are not used by this patch. Remove them from this one
> and add them to the one that actually uses them for the first time.
>
I will fix it, thank you.
> With this fixed,
>
> Reviewed-by: Pratyush Yadav
>
>>
>> /* Used for Micron flashes only. */
>> #define SPINOR_OP_RD_EVCR 0x65/* Read EVCR register */
>> --
>> 2.25.1
>>
>
Best Regards,
Takahiro
Re: [PATCH v4 2/9] mtd: spi-nor-ids: Add Cypress s25hl-t/s25hs-t
Hi Pratyush,
On 1/30/2021 3:08 AM, Pratyush Yadav wrote:
> On 28/01/21 01:36PM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.
>> The datasheets can be found in the following links.
>>
>> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die)
>> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)
>>
>> Tested 512Mb/1Gb/2Gb parts on Xilinx Zynq-7000 FPGA board.
>
> Why not test the 256 Mb and 4 Gb parts as well? They might work
> perfectly well but adding support for untested hardware sounds wrong to
> me.
The 256Mb and 4Gb parts are not yet officially sampled, although they are
described in the datasheet. They should work like other tested ones, but
as you commented, only tested devices should be added. I will remove 256Mb
and 4Gb in v5.
>
>> Signed-off-by: Takahiro Kuwano
>> ---
>> drivers/mtd/spi/spi-nor-ids.c | 36 +++
>> 1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
>> index 5bd5dd3003..b78d13e980 100644
>> --- a/drivers/mtd/spi/spi-nor-ids.c
>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>> @@ -217,6 +217,42 @@ const struct flash_info spi_nor_ids[] = {
>> { INFO("s25fl208k", 0x014014, 0, 64 * 1024, 16, SECT_4K |
>> SPI_NOR_DUAL_READ) },
>> { INFO("s25fl064l", 0x016017, 0, 64 * 1024, 128, SECT_4K |
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> { INFO("s25fl128l", 0x016018, 0, 64 * 1024, 256, SECT_4K |
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> +
>> +/* S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB
>
> Nitpick: Please leave first line of multi-line comments blank like so:
>
> /*
> * S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB
> * ...
> */
>
I will fix it.
>> + * sectors at top and/or bottom, depending on the device configuration.
>> + * To support this, an erase hook makes overlaid sectors appear as
>> + * uniform sectors.
>> + */
>> +{ INFO6("s25hl256t", 0x342a19, 0x0f0390, 256 * 1024, 128,
>> +SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +USE_CLSR) },
>> +{ INFO6("s25hl512t", 0x342a1a, 0x0f0390, 256 * 1024, 256,
>> +SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +USE_CLSR) },
>> +{ INFO6("s25hl01gt", 0x342a1b, 0x0f0390, 256 * 1024, 512,
>> +SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +USE_CLSR) },
>> +{ INFO6("s25hl02gt", 0x342a1c, 0x0f0090, 256 * 1024, 1024,
>> +SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +USE_CLSR) },
>> +{ INFO6("s25hl04gt", 0x342a1d, 0x0f0090, 256 * 1024, 2048,
>> +SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +USE_CLSR) },
>> +{ INFO6("s25hs256t", 0x342b19, 0x0f0390, 256 * 1024, 128,
>> +SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +USE_CLSR) },
>> +{ INFO6("s25hs512t", 0x342b1a, 0x0f0390, 256 * 1024, 256,
>> +SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +USE_CLSR) },
>> +{ INFO6("s25hs01gt", 0x342b1b, 0x0f0390, 256 * 1024, 512,
>> +SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +USE_CLSR) },
>> +{ INFO6("s25hs02gt", 0x342b1c, 0x0f0090, 256 * 1024, 1024,
>> +SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +USE_CLSR) },
>> +{ INFO6("s25hs04gt", 0x342b1d, 0x0f0090, 256 * 1024, 2048,
>> +SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +USE_CLSR) },
>
> The datasheets you have linked do not specify a mapping between device
> ID and part number. So I can't verify that you are using the correct IDs
> here. But I'll trust you to get them right :-)
>
Thank you :-)
> With the above comment addressed and 256 Mb and 4 Gb parts tested,
> please add
>
> Reviewed-by: Pratyush Yadav
>
>> #endif
>> #ifdef CONFIG_SPI_FLASH_SST /* SST */
>> /* SST -- large erase sizes are "overlays", "sectors" are 4K */
>> --
>> 2.25.1
>>
>
Best Regards,
Takahiro
