[PATCH 0/2] Add check of 4-byte when parsing SFDP 4bait table

[PATCH 0/2] Add check of 4-byte when parsing SFDP 4bait table

[Yicong Yang]

Add check of 4-byte address mode support when parsing SFDP 4bait. Some
flash will provide a 4bait table and the spi-nor core will convert the
address mode to 4-byte without checking whether it's actually supported
or not by the controller. For example, the 16M s25fs128s1 provides the
4bait and will be convert to 4-byte address mode, which makes it unusable
on hisi-sfc-v3xx on hip08 platform.

Add the check before convert the address mode when parsing the 4bait will
solve this issue.

Yicong Yang (2):
  mtd: spi-nor: check 4-byte address support when parsing 4bait
  spi: hisi-sfc-v3xx: add address mode check

 drivers/mtd/spi-nor/sfdp.c      | 48 +++++++++++++++++++++++++++++++++++++++++
 drivers/spi/spi-hisi-sfc-v3xx.c | 25 ++++++++++++++++++++-
 2 files changed, 72 insertions(+), 1 deletion(-)

-- 
2.8.1

[PATCH 0/2] Add check of 4-byte when parsing SFDP 4bait table

[Yicong Yang]

Add check of 4-byte address mode support when parsing SFDP 4bait. Some
flash will provide a 4bait table and the spi-nor core will convert the
address mode to 4-byte without checking whether it's actually supported
or not by the controller. For example, the 16M s25fs128s1 provides the
4bait and will be convert to 4-byte address mode, which makes it unusable
on hisi-sfc-v3xx on hip08 platform.

Add the check before convert the address mode when parsing the 4bait will
solve this issue.

Yicong Yang (2):
  mtd: spi-nor: check 4-byte address support when parsing 4bait
  spi: hisi-sfc-v3xx: add address mode check

 drivers/mtd/spi-nor/sfdp.c      | 48 +++++++++++++++++++++++++++++++++++++++++
 drivers/spi/spi-hisi-sfc-v3xx.c | 25 ++++++++++++++++++++-
 2 files changed, 72 insertions(+), 1 deletion(-)

-- 
2.8.1


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

[PATCH 1/2] mtd: spi-nor: check 4-byte address support when parsing 4bait

[Yicong Yang]

The spi-nor core will convert the address mode to 4-btye,
without checking whether 4-byte address is supported or not.
For example, the 16M s25fs128s1 can work under both 3-byte
and 4-byte address and provides a 4bait table. The spi-nor
will drive the flash under 4-byte address mode after parsing
the 4bait and will cause it unusable on platforms doesn't
support 4-byte.

Add checking of 4-byte address support when parsing the 4bait
table, stop converting the address mode if it's not supported.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/mtd/spi-nor/sfdp.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 6ee7719..fdafc9b 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -940,6 +940,27 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
 	return ret;
 }
 
+static int spi_nor_spimem_check_4byte_addr(struct spi_nor *nor,
+					   const struct spi_nor_read_command *read)
+{
+	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(read->opcode, 1),
+					  SPI_MEM_OP_ADDR(4, 0, 1),
+					  SPI_MEM_OP_DUMMY(0, 1),
+					  SPI_MEM_OP_DATA_IN(0, NULL, 1));
+
+	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(read->proto);
+	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(read->proto);
+	op.data.buswidth = spi_nor_get_protocol_data_nbits(read->proto);
+	op.dummy.buswidth = op.addr.buswidth;
+	op.dummy.nbytes = (read->num_mode_clocks + read->num_wait_states) *
+			  op.dummy.buswidth / 8;
+
+	if (!spi_mem_supports_op(nor->spimem, &op))
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
 /**
  * spi_nor_parse_4bait() - parse the 4-Byte Address Instruction Table
  * @nor:		pointer to a 'struct spi_nor'.
@@ -1061,6 +1082,33 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
 		goto out;
 
 	/*
+	 * Check whether the 4-byte address is supported before converting
+	 * the instruction set to 4-byte.
+	 */
+	if (nor->spimem) {
+		bool support = false;
+
+		for (i = 0; i < SNOR_CMD_READ_MAX; i++) {
+			struct spi_nor_read_command read_cmd;
+
+			memcpy(&read_cmd, &params->reads[i], sizeof(read_cmd));
+
+			read_cmd.opcode = spi_nor_convert_3to4_read(read_cmd.opcode);
+			if (!spi_nor_spimem_check_4byte_addr(nor, &read_cmd)) {
+				support = true;
+				break;
+			}
+		}
+
+		/*
+		 * No supported 4-byte instruction is found, stop parsing the
+		 * 4bait table.
+		 */
+		if (!support)
+			goto out;
+	}
+
+	/*
 	 * Discard all operations from the 4-byte instruction set which are
 	 * not supported by this memory.
 	 */
-- 
2.8.1

[PATCH 1/2] mtd: spi-nor: check 4-byte address support when parsing 4bait

[Yicong Yang]

The spi-nor core will convert the address mode to 4-btye,
without checking whether 4-byte address is supported or not.
For example, the 16M s25fs128s1 can work under both 3-byte
and 4-byte address and provides a 4bait table. The spi-nor
will drive the flash under 4-byte address mode after parsing
the 4bait and will cause it unusable on platforms doesn't
support 4-byte.

Add checking of 4-byte address support when parsing the 4bait
table, stop converting the address mode if it's not supported.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/mtd/spi-nor/sfdp.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 6ee7719..fdafc9b 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -940,6 +940,27 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
 	return ret;
 }
 
+static int spi_nor_spimem_check_4byte_addr(struct spi_nor *nor,
+					   const struct spi_nor_read_command *read)
+{
+	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(read->opcode, 1),
+					  SPI_MEM_OP_ADDR(4, 0, 1),
+					  SPI_MEM_OP_DUMMY(0, 1),
+					  SPI_MEM_OP_DATA_IN(0, NULL, 1));
+
+	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(read->proto);
+	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(read->proto);
+	op.data.buswidth = spi_nor_get_protocol_data_nbits(read->proto);
+	op.dummy.buswidth = op.addr.buswidth;
+	op.dummy.nbytes = (read->num_mode_clocks + read->num_wait_states) *
+			  op.dummy.buswidth / 8;
+
+	if (!spi_mem_supports_op(nor->spimem, &op))
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
 /**
  * spi_nor_parse_4bait() - parse the 4-Byte Address Instruction Table
  * @nor:		pointer to a 'struct spi_nor'.
@@ -1061,6 +1082,33 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
 		goto out;
 
 	/*
+	 * Check whether the 4-byte address is supported before converting
+	 * the instruction set to 4-byte.
+	 */
+	if (nor->spimem) {
+		bool support = false;
+
+		for (i = 0; i < SNOR_CMD_READ_MAX; i++) {
+			struct spi_nor_read_command read_cmd;
+
+			memcpy(&read_cmd, &params->reads[i], sizeof(read_cmd));
+
+			read_cmd.opcode = spi_nor_convert_3to4_read(read_cmd.opcode);
+			if (!spi_nor_spimem_check_4byte_addr(nor, &read_cmd)) {
+				support = true;
+				break;
+			}
+		}
+
+		/*
+		 * No supported 4-byte instruction is found, stop parsing the
+		 * 4bait table.
+		 */
+		if (!support)
+			goto out;
+	}
+
+	/*
 	 * Discard all operations from the 4-byte instruction set which are
 	 * not supported by this memory.
 	 */
-- 
2.8.1


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

[PATCH 2/2] spi: hisi-sfc-v3xx: add address mode check

[Yicong Yang]

The address mode is either 3 or 4 for the controller, which is configured
by the firmware and cannot be modified in the OS driver. Get the
firmware configuration and add address mode check in the .supports_op()
to block invalid operations.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Acked-by: John Garry <john.garry@huawei.com>
---
 drivers/spi/spi-hisi-sfc-v3xx.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-hisi-sfc-v3xx.c b/drivers/spi/spi-hisi-sfc-v3xx.c
index 4650b48..f71b780 100644
--- a/drivers/spi/spi-hisi-sfc-v3xx.c
+++ b/drivers/spi/spi-hisi-sfc-v3xx.c
@@ -19,6 +19,8 @@
 
 #define HISI_SFC_V3XX_VERSION (0x1f8)
 
+#define HISI_SFC_V3XX_GLB_CFG (0x100)
+#define HISI_SFC_V3XX_GLB_CFG_CS0_ADDR_MODE BIT(2)
 #define HISI_SFC_V3XX_RAW_INT_STAT (0x120)
 #define HISI_SFC_V3XX_INT_STAT (0x124)
 #define HISI_SFC_V3XX_INT_MASK (0x128)
@@ -75,6 +77,7 @@ struct hisi_sfc_v3xx_host {
 	void __iomem *regbase;
 	int max_cmd_dword;
 	struct completion *completion;
+	u8 address_mode;
 	int irq;
 };
 
@@ -168,10 +171,18 @@ static int hisi_sfc_v3xx_adjust_op_size(struct spi_mem *mem,
 static bool hisi_sfc_v3xx_supports_op(struct spi_mem *mem,
 				      const struct spi_mem_op *op)
 {
+	struct spi_device *spi = mem->spi;
+	struct hisi_sfc_v3xx_host *host;
+
+	host = spi_controller_get_devdata(spi->master);
+
 	if (op->data.buswidth > 4 || op->dummy.buswidth > 4 ||
 	    op->addr.buswidth > 4 || op->cmd.buswidth > 4)
 		return false;
 
+	if (op->addr.nbytes != host->address_mode && op->addr.nbytes)
+		return false;
+
 	return spi_mem_default_supports_op(mem, op);
 }
 
@@ -416,7 +427,7 @@ static int hisi_sfc_v3xx_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct hisi_sfc_v3xx_host *host;
 	struct spi_controller *ctlr;
-	u32 version;
+	u32 version, glb_config;
 	int ret;
 
 	ctlr = spi_alloc_master(&pdev->dev, sizeof(*host));
@@ -463,6 +474,18 @@ static int hisi_sfc_v3xx_probe(struct platform_device *pdev)
 	ctlr->num_chipselect = 1;
 	ctlr->mem_ops = &hisi_sfc_v3xx_mem_ops;
 
+	/*
+	 * The address mode of the controller is either 3 or 4,
+	 * which is indicated by the address mode bit in
+	 * the global config register. The register is read only
+	 * for the OS driver.
+	 */
+	glb_config = readl(host->regbase + HISI_SFC_V3XX_GLB_CFG);
+	if (glb_config & HISI_SFC_V3XX_GLB_CFG_CS0_ADDR_MODE)
+		host->address_mode = 4;
+	else
+		host->address_mode = 3;
+
 	version = readl(host->regbase + HISI_SFC_V3XX_VERSION);
 
 	switch (version) {
-- 
2.8.1

[PATCH 2/2] spi: hisi-sfc-v3xx: add address mode check

[Yicong Yang]

The address mode is either 3 or 4 for the controller, which is configured
by the firmware and cannot be modified in the OS driver. Get the
firmware configuration and add address mode check in the .supports_op()
to block invalid operations.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Acked-by: John Garry <john.garry@huawei.com>
---
 drivers/spi/spi-hisi-sfc-v3xx.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-hisi-sfc-v3xx.c b/drivers/spi/spi-hisi-sfc-v3xx.c
index 4650b48..f71b780 100644
--- a/drivers/spi/spi-hisi-sfc-v3xx.c
+++ b/drivers/spi/spi-hisi-sfc-v3xx.c
@@ -19,6 +19,8 @@
 
 #define HISI_SFC_V3XX_VERSION (0x1f8)
 
+#define HISI_SFC_V3XX_GLB_CFG (0x100)
+#define HISI_SFC_V3XX_GLB_CFG_CS0_ADDR_MODE BIT(2)
 #define HISI_SFC_V3XX_RAW_INT_STAT (0x120)
 #define HISI_SFC_V3XX_INT_STAT (0x124)
 #define HISI_SFC_V3XX_INT_MASK (0x128)
@@ -75,6 +77,7 @@ struct hisi_sfc_v3xx_host {
 	void __iomem *regbase;
 	int max_cmd_dword;
 	struct completion *completion;
+	u8 address_mode;
 	int irq;
 };
 
@@ -168,10 +171,18 @@ static int hisi_sfc_v3xx_adjust_op_size(struct spi_mem *mem,
 static bool hisi_sfc_v3xx_supports_op(struct spi_mem *mem,
 				      const struct spi_mem_op *op)
 {
+	struct spi_device *spi = mem->spi;
+	struct hisi_sfc_v3xx_host *host;
+
+	host = spi_controller_get_devdata(spi->master);
+
 	if (op->data.buswidth > 4 || op->dummy.buswidth > 4 ||
 	    op->addr.buswidth > 4 || op->cmd.buswidth > 4)
 		return false;
 
+	if (op->addr.nbytes != host->address_mode && op->addr.nbytes)
+		return false;
+
 	return spi_mem_default_supports_op(mem, op);
 }
 
@@ -416,7 +427,7 @@ static int hisi_sfc_v3xx_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct hisi_sfc_v3xx_host *host;
 	struct spi_controller *ctlr;
-	u32 version;
+	u32 version, glb_config;
 	int ret;
 
 	ctlr = spi_alloc_master(&pdev->dev, sizeof(*host));
@@ -463,6 +474,18 @@ static int hisi_sfc_v3xx_probe(struct platform_device *pdev)
 	ctlr->num_chipselect = 1;
 	ctlr->mem_ops = &hisi_sfc_v3xx_mem_ops;
 
+	/*
+	 * The address mode of the controller is either 3 or 4,
+	 * which is indicated by the address mode bit in
+	 * the global config register. The register is read only
+	 * for the OS driver.
+	 */
+	glb_config = readl(host->regbase + HISI_SFC_V3XX_GLB_CFG);
+	if (glb_config & HISI_SFC_V3XX_GLB_CFG_CS0_ADDR_MODE)
+		host->address_mode = 4;
+	else
+		host->address_mode = 3;
+
 	version = readl(host->regbase + HISI_SFC_V3XX_VERSION);
 
 	switch (version) {
-- 
2.8.1


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

Re: (subset) [PATCH 0/2] Add check of 4-byte when parsing SFDP 4bait table

[Mark Brown]

On Wed, 27 Jan 2021 17:40:48 +0800, Yicong Yang wrote:
> Add check of 4-byte address mode support when parsing SFDP 4bait. Some
> flash will provide a 4bait table and the spi-nor core will convert the
> address mode to 4-byte without checking whether it's actually supported
> or not by the controller. For example, the 16M s25fs128s1 provides the
> 4bait and will be convert to 4-byte address mode, which makes it unusable
> on hisi-sfc-v3xx on hip08 platform.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[2/2] spi: hisi-sfc-v3xx: add address mode check
      commit: 6d2386e36440165da782dbc5c0de40f31665e108

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: (subset) [PATCH 0/2] Add check of 4-byte when parsing SFDP 4bait table

[Mark Brown]

On Wed, 27 Jan 2021 17:40:48 +0800, Yicong Yang wrote:
> Add check of 4-byte address mode support when parsing SFDP 4bait. Some
> flash will provide a 4bait table and the spi-nor core will convert the
> address mode to 4-byte without checking whether it's actually supported
> or not by the controller. For example, the 16M s25fs128s1 provides the
> 4bait and will be convert to 4-byte address mode, which makes it unusable
> on hisi-sfc-v3xx on hip08 platform.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[2/2] spi: hisi-sfc-v3xx: add address mode check
      commit: 6d2386e36440165da782dbc5c0de40f31665e108

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

Re: [PATCH 1/2] mtd: spi-nor: check 4-byte address support when parsing 4bait

[Pratyush Yadav]

Hi Yicong,

On 27/01/21 05:40PM, Yicong Yang wrote:
> The spi-nor core will convert the address mode to 4-btye,
> without checking whether 4-byte address is supported or not.
> For example, the 16M s25fs128s1 can work under both 3-byte
> and 4-byte address and provides a 4bait table. The spi-nor
> will drive the flash under 4-byte address mode after parsing
> the 4bait and will cause it unusable on platforms doesn't
> support 4-byte.

Another problem caused by 4BAIT parser prematurely selecting the address 
width. See [0].
 
Let's fix this 4BAIT problem once and for all. Instead of setting 
nor->addr_width to 4 in spi_nor_parse_4bait(), just set SNOR_F_HAS_4BAIT 
(which is already being done). Then in spi_nor_default_setup(), use this 
information when negotiating the read/write/program commands with the 
controller to determine the correct address width to use.

This refactor is easier said than done. We don't associate address width 
information with a command. Just protocol, opcode, and dummy cycles 
(only for read commands). A new mechanism needs to be added where we 
associate a set of supported addresses with a command and then the 
command negotiation can use all this information to arrive at the 
optimal set of commands.

With this in mind, it would be great if you can come up with a patch to 
add such a mechanism. But I would also be OK with this fix with the 
condition that it is clearly marked as a temporary fix, and mentions 
what should ideally be done.

> Add checking of 4-byte address support when parsing the 4bait
> table, stop converting the address mode if it's not supported.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/mtd/spi-nor/sfdp.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 6ee7719..fdafc9b 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -940,6 +940,27 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
>  	return ret;
>  }
>  
> +static int spi_nor_spimem_check_4byte_addr(struct spi_nor *nor,
> +					   const struct spi_nor_read_command *read)
> +{
> +	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(read->opcode, 1),
> +					  SPI_MEM_OP_ADDR(4, 0, 1),
> +					  SPI_MEM_OP_DUMMY(0, 1),
> +					  SPI_MEM_OP_DATA_IN(0, NULL, 1));

Set buswidths to 0 here...

> +
> +	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(read->proto);
> +	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(read->proto);
> +	op.data.buswidth = spi_nor_get_protocol_data_nbits(read->proto);
> +	op.dummy.buswidth = op.addr.buswidth;

... and use spi_nor_spimem_setup_op() to do this. It will also take care 
of setting up DTR ops.

> +	op.dummy.nbytes = (read->num_mode_clocks + read->num_wait_states) *
> +			  op.dummy.buswidth / 8;
> +
> +	if (!spi_mem_supports_op(nor->spimem, &op))
> +		return -EOPNOTSUPP;
> +
> +	return 0;
> +}
> +
>  /**
>   * spi_nor_parse_4bait() - parse the 4-Byte Address Instruction Table
>   * @nor:		pointer to a 'struct spi_nor'.
> @@ -1061,6 +1082,33 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>  		goto out;
>  
>  	/*
> +	 * Check whether the 4-byte address is supported before converting
> +	 * the instruction set to 4-byte.
> +	 */
> +	if (nor->spimem) {
> +		bool support = false;
> +
> +		for (i = 0; i < SNOR_CMD_READ_MAX; i++) {
> +			struct spi_nor_read_command read_cmd;
> +
> +			memcpy(&read_cmd, &params->reads[i], sizeof(read_cmd));
> +
> +			read_cmd.opcode = spi_nor_convert_3to4_read(read_cmd.opcode);
> +			if (!spi_nor_spimem_check_4byte_addr(nor, &read_cmd)) {
> +				support = true;
> +				break;
> +			}
> +		}
> +
> +		/*
> +		 * No supported 4-byte instruction is found, stop parsing the
> +		 * 4bait table.
> +		 */
> +		if (!support)
> +			goto out;
> +	}
> +
> +	/*
>  	 * Discard all operations from the 4-byte instruction set which are
>  	 * not supported by this memory.
>  	 */

[0] https://lore.kernel.org/linux-mtd/20201212115817.5122-1-vigneshr@ti.com/

-- 
Regards,
Pratyush Yadav
Texas Instruments India

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

Re: [PATCH 1/2] mtd: spi-nor: check 4-byte address support when parsing 4bait

[Yicong Yang]

Hi Pratyush,

Thanks for the comments. Some replies below.

On 2021/1/29 19:16, Pratyush Yadav wrote:
> Hi Yicong,
> 
> On 27/01/21 05:40PM, Yicong Yang wrote:
>> The spi-nor core will convert the address mode to 4-btye,
>> without checking whether 4-byte address is supported or not.
>> For example, the 16M s25fs128s1 can work under both 3-byte
>> and 4-byte address and provides a 4bait table. The spi-nor
>> will drive the flash under 4-byte address mode after parsing
>> the 4bait and will cause it unusable on platforms doesn't
>> support 4-byte.
> 
> Another problem caused by 4BAIT parser prematurely selecting the address 
> width. See [0].
>  
> Let's fix this 4BAIT problem once and for all. Instead of setting 
> nor->addr_width to 4 in spi_nor_parse_4bait(), just set SNOR_F_HAS_4BAIT 
> (which is already being done). Then in spi_nor_default_setup(), use this 
> information when negotiating the read/write/program commands with the 
> controller to determine the correct address width to use.

The idea is great. ideally we should check the buswidth and commands in
spi_nor_default_setup() and finally set address width in
spi_nor_set_addr_width().

> 
> This refactor is easier said than done. We don't associate address width 
> information with a command. Just protocol, opcode, and dummy cycles 
> (only for read commands). A new mechanism needs to be added where we 
> associate a set of supported addresses with a command and then the 
> command negotiation can use all this information to arrive at the 
> optimal set of commands.
> 
> With this in mind, it would be great if you can come up with a patch to 
> add such a mechanism. But I would also be OK with this fix with the 
> condition that it is clearly marked as a temporary fix, and mentions 
> what should ideally be done.
> 

i'd like to get this issue fixed first to make the flash work properly,
if possible. then to do the refactor as it may take me sometime.

>> Add checking of 4-byte address support when parsing the 4bait
>> table, stop converting the address mode if it's not supported.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/mtd/spi-nor/sfdp.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index 6ee7719..fdafc9b 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -940,6 +940,27 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
>>  	return ret;
>>  }
>>  
>> +static int spi_nor_spimem_check_4byte_addr(struct spi_nor *nor,
>> +					   const struct spi_nor_read_command *read)
>> +{
>> +	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(read->opcode, 1),
>> +					  SPI_MEM_OP_ADDR(4, 0, 1),
>> +					  SPI_MEM_OP_DUMMY(0, 1),
>> +					  SPI_MEM_OP_DATA_IN(0, NULL, 1));
> 
> Set buswidths to 0 here...

ok.

> 
>> +
>> +	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(read->proto);
>> +	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(read->proto);
>> +	op.data.buswidth = spi_nor_get_protocol_data_nbits(read->proto);
>> +	op.dummy.buswidth = op.addr.buswidth;
> 
> ... and use spi_nor_spimem_setup_op() to do this. It will also take care 
> of setting up DTR ops.

will change to the new function, i didn't notice it. :)

Thanks,
Yicong

> 
>> +	op.dummy.nbytes = (read->num_mode_clocks + read->num_wait_states) *
>> +			  op.dummy.buswidth / 8;
>> +
>> +	if (!spi_mem_supports_op(nor->spimem, &op))
>> +		return -EOPNOTSUPP;
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * spi_nor_parse_4bait() - parse the 4-Byte Address Instruction Table
>>   * @nor:		pointer to a 'struct spi_nor'.
>> @@ -1061,6 +1082,33 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>>  		goto out;
>>  
>>  	/*
>> +	 * Check whether the 4-byte address is supported before converting
>> +	 * the instruction set to 4-byte.
>> +	 */
>> +	if (nor->spimem) {
>> +		bool support = false;
>> +
>> +		for (i = 0; i < SNOR_CMD_READ_MAX; i++) {
>> +			struct spi_nor_read_command read_cmd;
>> +
>> +			memcpy(&read_cmd, &params->reads[i], sizeof(read_cmd));
>> +
>> +			read_cmd.opcode = spi_nor_convert_3to4_read(read_cmd.opcode);
>> +			if (!spi_nor_spimem_check_4byte_addr(nor, &read_cmd)) {
>> +				support = true;
>> +				break;
>> +			}
>> +		}
>> +
>> +		/*
>> +		 * No supported 4-byte instruction is found, stop parsing the
>> +		 * 4bait table.
>> +		 */
>> +		if (!support)
>> +			goto out;
>> +	}
>> +
>> +	/*
>>  	 * Discard all operations from the 4-byte instruction set which are
>>  	 * not supported by this memory.
>>  	 */
> 
> [0] https://lore.kernel.org/linux-mtd/20201212115817.5122-1-vigneshr@ti.com/
> 

Re: [PATCH 1/2] mtd: spi-nor: check 4-byte address support when parsing 4bait

[Yicong Yang]

Hi Pratyush,

Thanks for the comments. Some replies below.

On 2021/1/29 19:16, Pratyush Yadav wrote:
> Hi Yicong,
> 
> On 27/01/21 05:40PM, Yicong Yang wrote:
>> The spi-nor core will convert the address mode to 4-btye,
>> without checking whether 4-byte address is supported or not.
>> For example, the 16M s25fs128s1 can work under both 3-byte
>> and 4-byte address and provides a 4bait table. The spi-nor
>> will drive the flash under 4-byte address mode after parsing
>> the 4bait and will cause it unusable on platforms doesn't
>> support 4-byte.
> 
> Another problem caused by 4BAIT parser prematurely selecting the address 
> width. See [0].
>  
> Let's fix this 4BAIT problem once and for all. Instead of setting 
> nor->addr_width to 4 in spi_nor_parse_4bait(), just set SNOR_F_HAS_4BAIT 
> (which is already being done). Then in spi_nor_default_setup(), use this 
> information when negotiating the read/write/program commands with the 
> controller to determine the correct address width to use.

The idea is great. ideally we should check the buswidth and commands in
spi_nor_default_setup() and finally set address width in
spi_nor_set_addr_width().

> 
> This refactor is easier said than done. We don't associate address width 
> information with a command. Just protocol, opcode, and dummy cycles 
> (only for read commands). A new mechanism needs to be added where we 
> associate a set of supported addresses with a command and then the 
> command negotiation can use all this information to arrive at the 
> optimal set of commands.
> 
> With this in mind, it would be great if you can come up with a patch to 
> add such a mechanism. But I would also be OK with this fix with the 
> condition that it is clearly marked as a temporary fix, and mentions 
> what should ideally be done.
> 

i'd like to get this issue fixed first to make the flash work properly,
if possible. then to do the refactor as it may take me sometime.

>> Add checking of 4-byte address support when parsing the 4bait
>> table, stop converting the address mode if it's not supported.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/mtd/spi-nor/sfdp.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index 6ee7719..fdafc9b 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -940,6 +940,27 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
>>  	return ret;
>>  }
>>  
>> +static int spi_nor_spimem_check_4byte_addr(struct spi_nor *nor,
>> +					   const struct spi_nor_read_command *read)
>> +{
>> +	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(read->opcode, 1),
>> +					  SPI_MEM_OP_ADDR(4, 0, 1),
>> +					  SPI_MEM_OP_DUMMY(0, 1),
>> +					  SPI_MEM_OP_DATA_IN(0, NULL, 1));
> 
> Set buswidths to 0 here...

ok.

> 
>> +
>> +	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(read->proto);
>> +	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(read->proto);
>> +	op.data.buswidth = spi_nor_get_protocol_data_nbits(read->proto);
>> +	op.dummy.buswidth = op.addr.buswidth;
> 
> ... and use spi_nor_spimem_setup_op() to do this. It will also take care 
> of setting up DTR ops.

will change to the new function, i didn't notice it. :)

Thanks,
Yicong

> 
>> +	op.dummy.nbytes = (read->num_mode_clocks + read->num_wait_states) *
>> +			  op.dummy.buswidth / 8;
>> +
>> +	if (!spi_mem_supports_op(nor->spimem, &op))
>> +		return -EOPNOTSUPP;
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * spi_nor_parse_4bait() - parse the 4-Byte Address Instruction Table
>>   * @nor:		pointer to a 'struct spi_nor'.
>> @@ -1061,6 +1082,33 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>>  		goto out;
>>  
>>  	/*
>> +	 * Check whether the 4-byte address is supported before converting
>> +	 * the instruction set to 4-byte.
>> +	 */
>> +	if (nor->spimem) {
>> +		bool support = false;
>> +
>> +		for (i = 0; i < SNOR_CMD_READ_MAX; i++) {
>> +			struct spi_nor_read_command read_cmd;
>> +
>> +			memcpy(&read_cmd, &params->reads[i], sizeof(read_cmd));
>> +
>> +			read_cmd.opcode = spi_nor_convert_3to4_read(read_cmd.opcode);
>> +			if (!spi_nor_spimem_check_4byte_addr(nor, &read_cmd)) {
>> +				support = true;
>> +				break;
>> +			}
>> +		}
>> +
>> +		/*
>> +		 * No supported 4-byte instruction is found, stop parsing the
>> +		 * 4bait table.
>> +		 */
>> +		if (!support)
>> +			goto out;
>> +	}
>> +
>> +	/*
>>  	 * Discard all operations from the 4-byte instruction set which are
>>  	 * not supported by this memory.
>>  	 */
> 
> [0] https://lore.kernel.org/linux-mtd/20201212115817.5122-1-vigneshr@ti.com/
> 


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