diff mbox series

[v4,6/6] mtd: spi-nor: spansion: Add s25hl-t/s25hs-t IDs and fixups

Message ID 76a55d9be126067c631065f31e3e970f90b2c080.1616130675.git.Takahiro.Kuwano@infineon.com
State Superseded
Delegated to: Ambarus Tudor
Headers show
Series mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t | expand

Commit Message

Takahiro Kuwano March 19, 2021, 6:58 a.m. UTC 
From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.

For the single-die package parts (512Mb and 1Gb), only bottom 4KB and
uniform sector sizes are supported. For the multi-die package parts (2Gb),
only uniform sector sizes is supprted. This is due to missing or incorrect
entries in SMPT. Fixup for other sector sizes configurations will be
followed up as needed.

Tested on Xilinx Zynq-7000 FPGA board.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
Changes in v4:
  - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
  - Remove USE_CLSR flags from S25HL02GT and S25HS02GT

Changes in v3:
  - Remove S25HL256T and S25HS256T
  - Add S25HL02GT and S25HS02GT 
  - Add support for multi-die package parts support
  - Remove erase_map fix for top/split sector layout
  - Set ECC data unit size (16B) to writesize 

 drivers/mtd/spi-nor/spansion.c | 115 +++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

Comments

Tudor Ambarus April 8, 2021, 5:06 a.m. UTC | #1 
Hi,

On 3/19/21 8:58 AM, tkuw584924@gmail.com wrote:
> +static int
> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
> +                        const struct sfdp_parameter_header *bfpt_header,
> +                        const struct sfdp_bfpt *bfpt,
> +                        struct spi_nor_flash_parameter *params)
> +{
> +       int ret;
> +       u32 addr;
> +       u8 cfr3v;
> +
> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
> +       if (ret)
> +               return ret;
> +       nor->addr_width = 4;

Takahiro, you are bypassing the core functions. spansion_set_4byte_addr_mode()
will be called at set_4byte_addr_mode() time, resulting in an illegal op?

Are we safe to modify the core and do the spi_nor_set_addr_width() and
nor->params->set_4byte_addr_mode() before parsing SFDP?

Cheers,
ta
Takahiro Kuwano April 8, 2021, 8:21 a.m. UTC | #2 
Hi Tudor,

On 4/8/2021 2:06 PM, Tudor.Ambarus@microchip.com wrote:
> Hi,
> 
> On 3/19/21 8:58 AM, tkuw584924@gmail.com wrote:
>> +static int
>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>> +                        const struct sfdp_parameter_header *bfpt_header,
>> +                        const struct sfdp_bfpt *bfpt,
>> +                        struct spi_nor_flash_parameter *params)
>> +{
>> +       int ret;
>> +       u32 addr;
>> +       u8 cfr3v;
>> +
>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>> +       if (ret)
>> +               return ret;
>> +       nor->addr_width = 4;
> 
> Takahiro, you are bypassing the core functions. spansion_set_4byte_addr_mode()
> will be called at set_4byte_addr_mode() time, resulting in an illegal op?
> 
Since the spansion_post_sfdp_fixups() adds the SNOR_F_4B_OPCODES flag,
spansion_set_4byte_addr_mode() is not called actually.

> Are we safe to modify the core and do the spi_nor_set_addr_width() and
> nor->params->set_4byte_addr_mode() before parsing SFDP?
> 
It sounds not safe to me as there are some discussions about addr_width.

https://patchwork.ozlabs.org/project/linux-mtd/patch/20201212115817.5122-1-vigneshr@ti.com/
https://patchwork.ozlabs.org/project/linux-mtd/patch/1611740450-47975-2-git-send-email-yangyicong@hisilicon.com/

Best Regards,
Takahiro
Tudor Ambarus April 8, 2021, 10:03 a.m. UTC | #3 
On 4/8/21 11:21 AM, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,
> 
> On 4/8/2021 2:06 PM, Tudor.Ambarus@microchip.com wrote:
>> Hi,
>>
>> On 3/19/21 8:58 AM, tkuw584924@gmail.com wrote:
>>> +static int
>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>> +                        const struct sfdp_bfpt *bfpt,
>>> +                        struct spi_nor_flash_parameter *params)
>>> +{
>>> +       int ret;
>>> +       u32 addr;
>>> +       u8 cfr3v;
>>> +
>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>> +       if (ret)
>>> +               return ret;
>>> +       nor->addr_width = 4;
>>
>> Takahiro, you are bypassing the core functions. spansion_set_4byte_addr_mode()
>> will be called at set_4byte_addr_mode() time, resulting in an illegal op?
>>
> Since the spansion_post_sfdp_fixups() adds the SNOR_F_4B_OPCODES flag,
> spansion_set_4byte_addr_mode() is not called actually.

right. Do we have to undo this somewhere, i.e, call
spi_nor_set_4byte_addr_mode(nor, false); ?

> 
>> Are we safe to modify the core and do the spi_nor_set_addr_width() and
>> nor->params->set_4byte_addr_mode() before parsing SFDP?
>>
> It sounds not safe to me as there are some discussions about addr_width.

oh, yes, of course, because addr width and 4byte addr mode opcodes are
discovered when parsing SFDP. I need to drink my coffee before writing
emails :D.

> 
> https://patchwork.ozlabs.org/project/linux-mtd/patch/20201212115817.5122-1-vigneshr@ti.com/
> https://patchwork.ozlabs.org/project/linux-mtd/patch/1611740450-47975-2-git-send-email-yangyicong@hisilicon.com/
> 
> Best Regards,
> Takahiro
>
Takahiro Kuwano April 9, 2021, 2:05 a.m. UTC | #4 
On 4/8/2021 7:03 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/8/21 11:21 AM, Takahiro Kuwano wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi Tudor,
>>
>> On 4/8/2021 2:06 PM, Tudor.Ambarus@microchip.com wrote:
>>> Hi,
>>>
>>> On 3/19/21 8:58 AM, tkuw584924@gmail.com wrote:
>>>> +static int
>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>> +                        const struct sfdp_bfpt *bfpt,
>>>> +                        struct spi_nor_flash_parameter *params)
>>>> +{
>>>> +       int ret;
>>>> +       u32 addr;
>>>> +       u8 cfr3v;
>>>> +
>>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +       nor->addr_width = 4;
>>>
>>> Takahiro, you are bypassing the core functions. spansion_set_4byte_addr_mode()
>>> will be called at set_4byte_addr_mode() time, resulting in an illegal op?
>>>
>> Since the spansion_post_sfdp_fixups() adds the SNOR_F_4B_OPCODES flag,
>> spansion_set_4byte_addr_mode() is not called actually.
> 
> right. Do we have to undo this somewhere, i.e, call
> spi_nor_set_4byte_addr_mode(nor, false); ?
> 
No. The Read/Write Any Register commands take 3 or 4 byte address depending
on the Flash's current addr mode. Since the spansion_read/write_any_reg()
use nor->addr_width which is always 4, the Flash's addr mode must always be
in 4 byte mode.

>>
>>> Are we safe to modify the core and do the spi_nor_set_addr_width() and
>>> nor->params->set_4byte_addr_mode() before parsing SFDP?
>>>
>> It sounds not safe to me as there are some discussions about addr_width.
> 
> oh, yes, of course, because addr width and 4byte addr mode opcodes are
> discovered when parsing SFDP. I need to drink my coffee before writing
> emails :D.
> 
:)
Tudor Ambarus April 9, 2021, 2:37 a.m. UTC | #5 
Hi,

On 4/9/21 5:05 AM, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 4/8/2021 7:03 PM, Tudor.Ambarus@microchip.com wrote:
>> On 4/8/21 11:21 AM, Takahiro Kuwano wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hi Tudor,
>>>
>>> On 4/8/2021 2:06 PM, Tudor.Ambarus@microchip.com wrote:
>>>> Hi,
>>>>
>>>> On 3/19/21 8:58 AM, tkuw584924@gmail.com wrote:
>>>>> +static int
>>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>>> +                        const struct sfdp_bfpt *bfpt,
>>>>> +                        struct spi_nor_flash_parameter *params)
>>>>> +{
>>>>> +       int ret;
>>>>> +       u32 addr;
>>>>> +       u8 cfr3v;
>>>>> +
>>>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>>>> +       if (ret)
>>>>> +               return ret;
>>>>> +       nor->addr_width = 4;
>>>>
>>>> Takahiro, you are bypassing the core functions. spansion_set_4byte_addr_mode()
>>>> will be called at set_4byte_addr_mode() time, resulting in an illegal op?
>>>>
>>> Since the spansion_post_sfdp_fixups() adds the SNOR_F_4B_OPCODES flag,
>>> spansion_set_4byte_addr_mode() is not called actually.
>>
>> right. Do we have to undo this somewhere, i.e, call
>> spi_nor_set_4byte_addr_mode(nor, false); ?
>>
> No. The Read/Write Any Register commands take 3 or 4 byte address depending
> on the Flash's current addr mode. Since the spansion_read/write_any_reg()
> use nor->addr_width which is always 4, the Flash's addr mode must always be
> in 4 byte mode.

If you reboot your board via cmdline, and the flash doesn't have a reset opcode,
the flash will remain in 4 byte mode when the driver loads again. Wouldn't that
affect the read sfdp command, since it is sent in 3 byte mode? Can you try a
"reboot" cmd and check what happens?

Cheers,
ta
Takahiro Kuwano April 9, 2021, 3:24 a.m. UTC | #6 
Hi Tudor,

On 4/9/2021 11:37 AM, Tudor.Ambarus@microchip.com wrote:
> Hi,
> 
> On 4/9/21 5:05 AM, Takahiro Kuwano wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 4/8/2021 7:03 PM, Tudor.Ambarus@microchip.com wrote:
>>> On 4/8/21 11:21 AM, Takahiro Kuwano wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Hi Tudor,
>>>>
>>>> On 4/8/2021 2:06 PM, Tudor.Ambarus@microchip.com wrote:
>>>>> Hi,
>>>>>
>>>>> On 3/19/21 8:58 AM, tkuw584924@gmail.com wrote:
>>>>>> +static int
>>>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>>>> +                        const struct sfdp_bfpt *bfpt,
>>>>>> +                        struct spi_nor_flash_parameter *params)
>>>>>> +{
>>>>>> +       int ret;
>>>>>> +       u32 addr;
>>>>>> +       u8 cfr3v;
>>>>>> +
>>>>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>>>>> +       if (ret)
>>>>>> +               return ret;
>>>>>> +       nor->addr_width = 4;
>>>>>
>>>>> Takahiro, you are bypassing the core functions. spansion_set_4byte_addr_mode()
>>>>> will be called at set_4byte_addr_mode() time, resulting in an illegal op?
>>>>>
>>>> Since the spansion_post_sfdp_fixups() adds the SNOR_F_4B_OPCODES flag,
>>>> spansion_set_4byte_addr_mode() is not called actually.
>>>
>>> right. Do we have to undo this somewhere, i.e, call
>>> spi_nor_set_4byte_addr_mode(nor, false); ?
>>>
>> No. The Read/Write Any Register commands take 3 or 4 byte address depending
>> on the Flash's current addr mode. Since the spansion_read/write_any_reg()
>> use nor->addr_width which is always 4, the Flash's addr mode must always be
>> in 4 byte mode.
> 
> If you reboot your board via cmdline, and the flash doesn't have a reset opcode,
> the flash will remain in 4 byte mode when the driver loads again. Wouldn't that
> affect the read sfdp command, since it is sent in 3 byte mode? Can you try a
> "reboot" cmd and check what happens?
> 
The Flash always takes 3 byte address in Read SFDP command regardless of
address mode. I tried reboot and confirmed it's working correctly.

Best Regards,
Takahiro
diff mbox series

Patch

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 04ad8f83dae0..63a10ba28fc1 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -229,6 +229,103 @@  static int spansion_mdp_ready(struct spi_nor *nor, u8 reg_dummy, u32 die_size)
 	return 1;
 }
 
+static int s25hx_t_quad_enable(struct spi_nor *nor)
+{
+	return spansion_quad_enable_volatile(nor, 0, SZ_128M);
+}
+
+static int s25hx_t_mdp_ready(struct spi_nor *nor)
+{
+	return spansion_mdp_ready(nor, 0, SZ_128M);
+}
+
+static int
+s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
+			 const struct sfdp_parameter_header *bfpt_header,
+			 const struct sfdp_bfpt *bfpt,
+			 struct spi_nor_flash_parameter *params)
+{
+	int ret;
+	u32 addr;
+	u8 cfr3v;
+
+	ret = spi_nor_set_4byte_addr_mode(nor, true);
+	if (ret)
+		return ret;
+	nor->addr_width = 4;
+
+	/* Replace Quad Enable with volatile version */
+	params->quad_enable = s25hx_t_quad_enable;
+
+	/*
+	 * The page_size is set to 512B from BFPT, but it actually depends on
+	 * the configuration register. Look up the CFR3V and determine the
+	 * page_size. For multi-die package parts, use 512B only when the all
+	 * dies are configured to 512B buffer.
+	 */
+	for (addr = 0; addr < params->size; addr += SZ_128M) {
+		ret = spansion_read_any_reg(nor,
+					    addr + SPINOR_REG_CYPRESS_CFR3V, 0,
+					    &cfr3v);
+		if (ret)
+			return ret;
+
+		if (!(cfr3v & SPINOR_REG_CYPRESS_CFR3V_PGSZ)) {
+			params->page_size = 256;
+			return 0;
+		}
+	}
+	params->page_size = 512;
+
+	return 0;
+}
+
+void s25hx_t_post_sfdp_fixups(struct spi_nor *nor)
+{
+	/* Fast Read 4B requires mode cycles */
+	nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
+
+	/* The writesize should be ECC data unit size */
+	nor->params->writesize = 16;
+
+	/*
+	 * For the single-die package parts (512Mb and 1Gb), bottom 4KB and
+	 * uniform sector maps are correctly populated in the erase_map
+	 * structure. The table below shows all possible combinations of related
+	 * register bits and its availability in SMPT.
+	 *
+	 *   CFR3[3] | CFR1[6] | CFR1[2] | Sector Map | Available in SMPT?
+	 *  -------------------------------------------------------------------
+	 *      0    |    0    |    0    | Bottom     | YES
+	 *      0    |    0    |    1    | Top        | NO (decoded as Split)
+	 *      0    |    1    |    0    | Split      | NO
+	 *      0    |    1    |    1    | Split      | NO (decoded as Top)
+	 *      1    |    0    |    0    | Uniform    | YES
+	 *      1    |    0    |    1    | Uniform    | NO
+	 *      1    |    1    |    0    | Uniform    | NO
+	 *      1    |    1    |    1    | Uniform    | NO
+	 *  -------------------------------------------------------------------
+	 *
+	 * For the dual-die package parts (2Gb), SMPT parse fails due to
+	 * incorrect SMPT entries and the erase map is populated as 4K uniform
+	 * that does not supported the parts. So it needs to be rolled back to
+	 * 256K uniform that is the factory default of multi-die package parts.
+	 */
+	if (nor->params->size > SZ_128M) {
+		spi_nor_init_uniform_erase_map(&nor->params->erase_map,
+					       BIT(SNOR_ERASE_TYPE_MAX - 1),
+					       nor->params->size);
+
+		/* Need to check status of each die via RDAR command */
+		nor->params->ready = s25hx_t_mdp_ready;
+	}
+}
+
+static struct spi_nor_fixups s25hx_t_fixups = {
+	.post_bfpt = s25hx_t_post_bfpt_fixups,
+	.post_sfdp = s25hx_t_post_sfdp_fixups
+};
+
 /**
  * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
  * @nor:		pointer to a 'struct spi_nor'
@@ -479,6 +576,24 @@  static const struct flash_info spansion_parts[] = {
 	{ "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512,
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 			     SPI_NOR_4B_OPCODES) },
+	{ "s25hl512t",  INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
+	  .fixups = &s25hx_t_fixups },
+	{ "s25hl01gt",  INFO6(0x342a1b, 0x0f0390, 256 * 1024, 512,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
+	  .fixups = &s25hx_t_fixups },
+	{ "s25hl02gt",  INFO6(0x342a1c, 0x0f0090, 256 * 1024, 1024,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+	  .fixups = &s25hx_t_fixups },
+	{ "s25hs512t",  INFO6(0x342b1a, 0x0f0390, 256 * 1024, 256,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
+	  .fixups = &s25hx_t_fixups },
+	{ "s25hs01gt",  INFO6(0x342b1b, 0x0f0390, 256 * 1024, 512,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
+	  .fixups = &s25hx_t_fixups },
+	{ "s25hs02gt",  INFO6(0x342b1c, 0x0f0090, 256 * 1024, 1024,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+	  .fixups = &s25hx_t_fixups },
 	{ "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1,
 			      SPI_NOR_NO_ERASE) },
 	{ "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256,