[PATCH v2] mtd: spi-nor: core: Discard HW capabilities if no enable function

[PATCH v2] mtd: spi-nor: core: Discard HW capabilities if no enable function

[Jaime Liao]

From: JaimeLiao <jaimeliao@mxic.com.tw>

Discard corresponding HW capabilities to prevent carrying the
wrong protocol if no QUAD/Octal DTR enable function hooked.

Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
---
changes in v2
 - Add SNOR_HWCAPS_8_8_8_DTR
 - Restore the enable function judgement in spi_nor_set_octal_dtr()
 - Restore the enable function judgement in spi_nor_quad_enable()
---
 drivers/mtd/spi-nor/core.c  | 7 +++++++
 include/linux/mtd/spi-nor.h | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 1c443fe568cf..14359101c6cf 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor *nor,
 	 */
 	shared_mask = hwcaps->mask & params->hwcaps.mask;
 
+	/* Mask out Octal DTR if no enable function */
+	if (!params->set_octal_dtr)
+		shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR;
+
+	if (!params->quad_enable)
+		shared_mask &= ~SNOR_HWCAPS_4_4_4;
+
 	if (nor->spimem) {
 		/*
 		 * When called from spi_nor_probe(), all caps are set and we
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index cdcfe0fd2e7d..78a119192ee0 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -266,6 +266,12 @@ struct spi_nor_hwcaps {
 #define SNOR_HWCAPS_PP_8_8_8		BIT(22)
 #define SNOR_HWCAPS_PP_8_8_8_DTR	BIT(23)
 
+#define SNOR_HWCAPS_4_4_4	(SNOR_HWCAPS_READ_4_4_4 |	\
+				 SNOR_HWCAPS_PP_4_4_4)
+
+#define SNOR_HWCAPS_8_8_8_DTR	(SNOR_HWCAPS_READ_8_8_8_DTR |	\
+				 SNOR_HWCAPS_PP_8_8_8_DTR)
+
 #define SNOR_HWCAPS_X_X_X	(SNOR_HWCAPS_READ_2_2_2 |	\
 				 SNOR_HWCAPS_READ_4_4_4 |	\
 				 SNOR_HWCAPS_READ_8_8_8 |	\
-- 
2.25.1


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

[PATCH v3] mtd: spi-nor: sfdp: Get the 1-1-8 and 1-8-8 protocol from SFDP

[Jaime Liao]

From: JaimeLiao <jaimeliao@mxic.com.tw>

BFPT 17th DWORD contains the information about 1-1-8
and 1-8-8, parse BFPT 17 DWORD instruction to determine
whether flash support 1-1-8 and 1-8-8, and set its dummy
cycles accordingly.

Protocols 1-1-8 and 1-8-8 support are not determined by a
single bit, so that use 8 bits instruction to determine
whether the protocol support or not.

Macronix spi-nor flash didn't support 1-8-8 read feature,
this patch validated 1-1-8 feature only on Xilinx board
zynq-picozed.

Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
Reviewed-by: Michael Walle <mwalle@kernel.org>
---
Changes in v3:
 - Modify title of patch
 - Parse 1-8-8 read as well
 - Modify the typo descriptions in commit log
---
 drivers/mtd/spi-nor/sfdp.c | 29 +++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/sfdp.h |  7 +++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index b3b11dfed789..3f1eba83a7d8 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -446,6 +446,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 	u32 dword;
 	u16 half;
 	u8 erase_mask;
+	u8 wait_states, mode_clocks, opcode;
 
 	/* JESD216 Basic Flash Parameter Table length is at least 9 DWORDs. */
 	if (bfpt_header->length < BFPT_DWORD_MAX_JESD216)
@@ -631,6 +632,32 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 	if (bfpt_header->length == BFPT_DWORD_MAX_JESD216B)
 		return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt);
 
+	/* Parse 1-1-8 read instruction */
+	opcode = FIELD_GET(BFPT_DWORD17_RD_1_1_8_CMD, bfpt.dwords[SFDP_DWORD(17)]);
+	if (opcode) {
+		mode_clocks = FIELD_GET(BFPT_DWORD17_RD_1_1_8_MODE_CLOCKS,
+					bfpt.dwords[SFDP_DWORD(17)]);
+		wait_states = FIELD_GET(BFPT_DWORD17_RD_1_1_8_WAIT_STATES,
+					bfpt.dwords[SFDP_DWORD(17)]);
+		nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_8;
+		spi_nor_set_read_settings(&nor->params->reads[SNOR_CMD_READ_1_1_8],
+					  mode_clocks, wait_states, opcode,
+					  SNOR_PROTO_1_1_8);
+	}
+
+	/* Parse 1-8-8 read instruction */
+	opcode = FIELD_GET(BFPT_DWORD17_RD_1_8_8_CMD, bfpt.dwords[SFDP_DWORD(17)]);
+	if (opcode) {
+		mode_clocks = FIELD_GET(BFPT_DWORD17_RD_1_8_8_MODE_CLOCKS,
+					bfpt.dwords[SFDP_DWORD(17)]);
+		wait_states = FIELD_GET(BFPT_DWORD17_RD_1_8_8_WAIT_STATES,
+					bfpt.dwords[SFDP_DWORD(17)]);
+		nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_1_8_8;
+		spi_nor_set_read_settings(&nor->params->reads[SNOR_CMD_READ_1_8_8],
+					  mode_clocks, wait_states, opcode,
+					  SNOR_PROTO_1_8_8);
+	}
+
 	/* 8D-8D-8D command extension. */
 	switch (bfpt.dwords[SFDP_DWORD(18)] & BFPT_DWORD18_CMD_EXT_MASK) {
 	case BFPT_DWORD18_CMD_EXT_REP:
@@ -968,6 +995,8 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
 		{ SNOR_HWCAPS_READ_1_1_1_DTR,	BIT(13) },
 		{ SNOR_HWCAPS_READ_1_2_2_DTR,	BIT(14) },
 		{ SNOR_HWCAPS_READ_1_4_4_DTR,	BIT(15) },
+		{ SNOR_HWCAPS_READ_1_1_8,	BIT(20) },
+		{ SNOR_HWCAPS_READ_1_8_8,	BIT(21) },
 	};
 	static const struct sfdp_4bait programs[] = {
 		{ SNOR_HWCAPS_PP,		BIT(6) },
diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
index 6eb99e1cdd61..da0fe5aa9bb0 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -118,6 +118,13 @@ struct sfdp_bfpt {
 	(BFPT_DWORD16_EN4B_EN4B | BFPT_DWORD16_EX4B_EX4B)
 #define BFPT_DWORD16_SWRST_EN_RST		BIT(12)
 
+#define BFPT_DWORD17_RD_1_1_8_CMD		GENMASK(31, 24)
+#define BFPT_DWORD17_RD_1_1_8_MODE_CLOCKS	GENMASK(23, 21)
+#define BFPT_DWORD17_RD_1_1_8_WAIT_STATES	GENMASK(20, 16)
+#define BFPT_DWORD17_RD_1_8_8_CMD		GENMASK(15, 8)
+#define BFPT_DWORD17_RD_1_8_8_MODE_CLOCKS	GENMASK(7, 5)
+#define BFPT_DWORD17_RD_1_8_8_WAIT_STATES	GENMASK(4, 0)
+
 #define BFPT_DWORD18_CMD_EXT_MASK		GENMASK(30, 29)
 #define BFPT_DWORD18_CMD_EXT_REP		(0x0UL << 29) /* Repeat */
 #define BFPT_DWORD18_CMD_EXT_INV		(0x1UL << 29) /* Invert */
-- 
2.25.1


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

Re: [PATCH v3] mtd: spi-nor: sfdp: Get the 1-1-8 and 1-8-8 protocol from SFDP

[Michael Walle]

Hi,

For the future, please send separate patches as separate mails,
not in one thread. This will break b4 and probably patchwork.

> From: JaimeLiao <jaimeliao@mxic.com.tw>
> 
> BFPT 17th DWORD contains the information about 1-1-8
> and 1-8-8, parse BFPT 17 DWORD instruction to determine
> whether flash support 1-1-8 and 1-8-8, and set its dummy
> cycles accordingly.
> 
> Protocols 1-1-8 and 1-8-8 support are not determined by a
> single bit, so that use 8 bits instruction to determine
> whether the protocol support or not.
> 
> Macronix spi-nor flash didn't support 1-8-8 read feature,
> this patch validated 1-1-8 feature only on Xilinx board
> zynq-picozed.
> 
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> Reviewed-by: Michael Walle <mwalle@kernel.org>

Also for the future, if you do significant changes or adding
further features, you should drop the Rb tag.

Anyway, this looks good, so my Rb tag is ok.

-michael

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

Re: [PATCH v3] mtd: spi-nor: sfdp: Get the 1-1-8 and 1-8-8 protocol from SFDP

[Tudor Ambarus]

On 19.12.2023 12:21, Jaime Liao wrote:
> From: JaimeLiao <jaimeliao@mxic.com.tw>
> 
> BFPT 17th DWORD contains the information about 1-1-8
> and 1-8-8, parse BFPT 17 DWORD instruction to determine
> whether flash support 1-1-8 and 1-8-8, and set its dummy
> cycles accordingly.
> 
> Protocols 1-1-8 and 1-8-8 support are not determined by a
> single bit, so that use 8 bits instruction to determine
> whether the protocol support or not.
> 
> Macronix spi-nor flash didn't support 1-8-8 read feature,
> this patch validated 1-1-8 feature only on Xilinx board
> zynq-picozed.
> 
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> Reviewed-by: Michael Walle <mwalle@kernel.org>
> ---
> Changes in v3:
>  - Modify title of patch
>  - Parse 1-8-8 read as well
>  - Modify the typo descriptions in commit log
> ---
>  drivers/mtd/spi-nor/sfdp.c | 29 +++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/sfdp.h |  7 +++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index b3b11dfed789..3f1eba83a7d8 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -446,6 +446,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  	u32 dword;
>  	u16 half;
>  	u8 erase_mask;
> +	u8 wait_states, mode_clocks, opcode;
>  
>  	/* JESD216 Basic Flash Parameter Table length is at least 9 DWORDs. */
>  	if (bfpt_header->length < BFPT_DWORD_MAX_JESD216)
> @@ -631,6 +632,32 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  	if (bfpt_header->length == BFPT_DWORD_MAX_JESD216B)
>  		return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt);
>  
> +	/* Parse 1-1-8 read instruction */
> +	opcode = FIELD_GET(BFPT_DWORD17_RD_1_1_8_CMD, bfpt.dwords[SFDP_DWORD(17)]);
> +	if (opcode) {
> +		mode_clocks = FIELD_GET(BFPT_DWORD17_RD_1_1_8_MODE_CLOCKS,
> +					bfpt.dwords[SFDP_DWORD(17)]);
> +		wait_states = FIELD_GET(BFPT_DWORD17_RD_1_1_8_WAIT_STATES,
> +					bfpt.dwords[SFDP_DWORD(17)]);
> +		nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_8;
> +		spi_nor_set_read_settings(&nor->params->reads[SNOR_CMD_READ_1_1_8],
> +					  mode_clocks, wait_states, opcode,
> +					  SNOR_PROTO_1_1_8);
> +	}
> +
> +	/* Parse 1-8-8 read instruction */
> +	opcode = FIELD_GET(BFPT_DWORD17_RD_1_8_8_CMD, bfpt.dwords[SFDP_DWORD(17)]);
> +	if (opcode) {
> +		mode_clocks = FIELD_GET(BFPT_DWORD17_RD_1_8_8_MODE_CLOCKS,
> +					bfpt.dwords[SFDP_DWORD(17)]);
> +		wait_states = FIELD_GET(BFPT_DWORD17_RD_1_8_8_WAIT_STATES,
> +					bfpt.dwords[SFDP_DWORD(17)]);
> +		nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_1_8_8;
> +		spi_nor_set_read_settings(&nor->params->reads[SNOR_CMD_READ_1_8_8],
> +					  mode_clocks, wait_states, opcode,
> +					  SNOR_PROTO_1_8_8);
> +	}

you can use params directly instead of nor->params. And it could have
been written in a generic way, considering the other read settings
parsed from SFDP. I'll get rid of the extra dereference and queue it,
you can address how settings are parsed later on if you care. Or I'll do
it myself if I won't be able to bear it.

> +
>  	/* 8D-8D-8D command extension. */
>  	switch (bfpt.dwords[SFDP_DWORD(18)] & BFPT_DWORD18_CMD_EXT_MASK) {
>  	case BFPT_DWORD18_CMD_EXT_REP:
> @@ -968,6 +995,8 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>  		{ SNOR_HWCAPS_READ_1_1_1_DTR,	BIT(13) },
>  		{ SNOR_HWCAPS_READ_1_2_2_DTR,	BIT(14) },
>  		{ SNOR_HWCAPS_READ_1_4_4_DTR,	BIT(15) },
> +		{ SNOR_HWCAPS_READ_1_1_8,	BIT(20) },
> +		{ SNOR_HWCAPS_READ_1_8_8,	BIT(21) },
>  	};
>  	static const struct sfdp_4bait programs[] = {
>  		{ SNOR_HWCAPS_PP,		BIT(6) },
> diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
> index 6eb99e1cdd61..da0fe5aa9bb0 100644
> --- a/drivers/mtd/spi-nor/sfdp.h
> +++ b/drivers/mtd/spi-nor/sfdp.h
> @@ -118,6 +118,13 @@ struct sfdp_bfpt {
>  	(BFPT_DWORD16_EN4B_EN4B | BFPT_DWORD16_EX4B_EX4B)
>  #define BFPT_DWORD16_SWRST_EN_RST		BIT(12)
>  
> +#define BFPT_DWORD17_RD_1_1_8_CMD		GENMASK(31, 24)
> +#define BFPT_DWORD17_RD_1_1_8_MODE_CLOCKS	GENMASK(23, 21)
> +#define BFPT_DWORD17_RD_1_1_8_WAIT_STATES	GENMASK(20, 16)
> +#define BFPT_DWORD17_RD_1_8_8_CMD		GENMASK(15, 8)
> +#define BFPT_DWORD17_RD_1_8_8_MODE_CLOCKS	GENMASK(7, 5)
> +#define BFPT_DWORD17_RD_1_8_8_WAIT_STATES	GENMASK(4, 0)
> +
>  #define BFPT_DWORD18_CMD_EXT_MASK		GENMASK(30, 29)
>  #define BFPT_DWORD18_CMD_EXT_REP		(0x0UL << 29) /* Repeat */
>  #define BFPT_DWORD18_CMD_EXT_INV		(0x1UL << 29) /* Invert */

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

Re (subset): [PATCH v3] mtd: spi-nor: sfdp: Get the 1-1-8 and 1-8-8 protocol from SFDP

[Tudor Ambarus]

On Tue, 19 Dec 2023 18:21:03 +0800, Jaime Liao wrote:
> BFPT 17th DWORD contains the information about 1-1-8
> and 1-8-8, parse BFPT 17 DWORD instruction to determine
> whether flash support 1-1-8 and 1-8-8, and set its dummy
> cycles accordingly.
> 
> Protocols 1-1-8 and 1-8-8 support are not determined by a
> single bit, so that use 8 bits instruction to determine
> whether the protocol support or not.
> 
> [...]

Made some changes, see commit mesage.
Applied to git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git,
spi-nor/next branch. Thanks!

[1/1] mtd: spi-nor: sfdp: Get the 1-1-8 and 1-8-8 protocol from SFDP
      https://git.kernel.org/mtd/c/af2792abd455

Cheers,
-- 
Tudor Ambarus <tudor.ambarus@linaro.org>

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

Re: [PATCH v2] mtd: spi-nor: core: Discard HW capabilities if no enable function

[Michael Walle]

Am 2023-12-19 11:21, schrieb Jaime Liao:
> From: JaimeLiao <jaimeliao@mxic.com.tw>
> 
> Discard corresponding HW capabilities to prevent carrying the
> wrong protocol if no QUAD/Octal DTR enable function hooked.
> 
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>

Reviewed-by: Michael Walle <michael@walle.cc>

-michael

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

Re: [PATCH v2] mtd: spi-nor: core: Discard HW capabilities if no enable function

[Tudor Ambarus]

On 19.12.2023 12:21, Jaime Liao wrote:
> From: JaimeLiao <jaimeliao@mxic.com.tw>
> 
> Discard corresponding HW capabilities to prevent carrying the
> wrong protocol if no QUAD/Octal DTR enable function hooked.
> 
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> ---
> changes in v2
>  - Add SNOR_HWCAPS_8_8_8_DTR
>  - Restore the enable function judgement in spi_nor_set_octal_dtr()
>  - Restore the enable function judgement in spi_nor_quad_enable()
> ---
>  drivers/mtd/spi-nor/core.c  | 7 +++++++
>  include/linux/mtd/spi-nor.h | 6 ++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1c443fe568cf..14359101c6cf 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>  	 */
>  	shared_mask = hwcaps->mask & params->hwcaps.mask;
>  
> +	/* Mask out Octal DTR if no enable function */
> +	if (!params->set_octal_dtr)
> +		shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR;
> +
> +	if (!params->quad_enable)
> +		shared_mask &= ~SNOR_HWCAPS_4_4_4;
> +
>  	if (nor->spimem) {
>  		/*
>  		 * When called from spi_nor_probe(), all caps are set and we
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index cdcfe0fd2e7d..78a119192ee0 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -266,6 +266,12 @@ struct spi_nor_hwcaps {
>  #define SNOR_HWCAPS_PP_8_8_8		BIT(22)
>  #define SNOR_HWCAPS_PP_8_8_8_DTR	BIT(23)
>  
> +#define SNOR_HWCAPS_4_4_4	(SNOR_HWCAPS_READ_4_4_4 |	\
> +				 SNOR_HWCAPS_PP_4_4_4)

quad enable applies for 1-1-4 and 1-4-4 as well. How about changing this to:

#define SNOR_HWCAPS_QUAD	(SNOR_HWCAPS_READ_QUAD |	\
				 SNOR_HWCAPS_PP_QUAD)
> +
> +#define SNOR_HWCAPS_8_8_8_DTR	(SNOR_HWCAPS_READ_8_8_8_DTR |	\
> +				 SNOR_HWCAPS_PP_8_8_8_DTR)
> +
>  #define SNOR_HWCAPS_X_X_X	(SNOR_HWCAPS_READ_2_2_2 |	\
>  				 SNOR_HWCAPS_READ_4_4_4 |	\
>  				 SNOR_HWCAPS_READ_8_8_8 |	\

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

Re: [PATCH v2] mtd: spi-nor: core: Discard HW capabilities if no enable function

[Michael Walle]

Am 2023-12-20 09:07, schrieb Tudor Ambarus:
> On 19.12.2023 12:21, Jaime Liao wrote:
>> From: JaimeLiao <jaimeliao@mxic.com.tw>
>> 
>> Discard corresponding HW capabilities to prevent carrying the
>> wrong protocol if no QUAD/Octal DTR enable function hooked.
>> 
>> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
>> ---
>> changes in v2
>>  - Add SNOR_HWCAPS_8_8_8_DTR
>>  - Restore the enable function judgement in spi_nor_set_octal_dtr()
>>  - Restore the enable function judgement in spi_nor_quad_enable()
>> ---
>>  drivers/mtd/spi-nor/core.c  | 7 +++++++
>>  include/linux/mtd/spi-nor.h | 6 ++++++
>>  2 files changed, 13 insertions(+)
>> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 1c443fe568cf..14359101c6cf 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor 
>> *nor,
>>  	 */
>>  	shared_mask = hwcaps->mask & params->hwcaps.mask;
>> 
>> +	/* Mask out Octal DTR if no enable function */
>> +	if (!params->set_octal_dtr)
>> +		shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR;
>> +
>> +	if (!params->quad_enable)
>> +		shared_mask &= ~SNOR_HWCAPS_4_4_4;
>> +
>>  	if (nor->spimem) {
>>  		/*
>>  		 * When called from spi_nor_probe(), all caps are set and we
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index cdcfe0fd2e7d..78a119192ee0 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -266,6 +266,12 @@ struct spi_nor_hwcaps {
>>  #define SNOR_HWCAPS_PP_8_8_8		BIT(22)
>>  #define SNOR_HWCAPS_PP_8_8_8_DTR	BIT(23)
>> 
>> +#define SNOR_HWCAPS_4_4_4	(SNOR_HWCAPS_READ_4_4_4 |	\
>> +				 SNOR_HWCAPS_PP_4_4_4)
> 
> quad enable applies for 1-1-4 and 1-4-4 as well. How about changing 
> this to:

Of course. You are right. I'm not even sure 4_4_4 falls into the
category for our quad_enable, because there's a special mode,
sometimes called qpi, for opcodes transferred in quad mode.

-michael

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

Re: [PATCH v2] mtd: spi-nor: core: Discard HW capabilities if no enable function

[Tudor Ambarus]

On 19.12.2023 12:21, Jaime Liao wrote:
> From: JaimeLiao <jaimeliao@mxic.com.tw>
> 
> Discard corresponding HW capabilities to prevent carrying the
> wrong protocol if no QUAD/Octal DTR enable function hooked.
> 
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> ---
> changes in v2
>  - Add SNOR_HWCAPS_8_8_8_DTR
>  - Restore the enable function judgement in spi_nor_set_octal_dtr()
>  - Restore the enable function judgement in spi_nor_quad_enable()
> ---
>  drivers/mtd/spi-nor/core.c  | 7 +++++++
>  include/linux/mtd/spi-nor.h | 6 ++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1c443fe568cf..14359101c6cf 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>  	 */
>  	shared_mask = hwcaps->mask & params->hwcaps.mask;
>  
> +	/* Mask out Octal DTR if no enable function */
> +	if (!params->set_octal_dtr)
> +		shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR;
> +
> +	if (!params->quad_enable)
> +		shared_mask &= ~SNOR_HWCAPS_4_4_4;

and these should have been in the late init hook, and instead discard
them from the params->hwcaps.mask.

And why is this extra check needed in the first place? For octal I
assume it's needed by macronix, where the flash is octal capable, but
there's no octal enable method defined yet. What about the quad mode? Is
there any flash that has no quad enable method defined but still caries
quad mode in its hwcaps?


> +
>  	if (nor->spimem) {
>  		/*
>  		 * When called from spi_nor_probe(), all caps are set and we
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index cdcfe0fd2e7d..78a119192ee0 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -266,6 +266,12 @@ struct spi_nor_hwcaps {
>  #define SNOR_HWCAPS_PP_8_8_8		BIT(22)
>  #define SNOR_HWCAPS_PP_8_8_8_DTR	BIT(23)
>  
> +#define SNOR_HWCAPS_4_4_4	(SNOR_HWCAPS_READ_4_4_4 |	\
> +				 SNOR_HWCAPS_PP_4_4_4)
> +
> +#define SNOR_HWCAPS_8_8_8_DTR	(SNOR_HWCAPS_READ_8_8_8_DTR |	\
> +				 SNOR_HWCAPS_PP_8_8_8_DTR)
> +
>  #define SNOR_HWCAPS_X_X_X	(SNOR_HWCAPS_READ_2_2_2 |	\
>  				 SNOR_HWCAPS_READ_4_4_4 |	\
>  				 SNOR_HWCAPS_READ_8_8_8 |	\

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

Re: [PATCH v2] mtd: spi-nor: core: Discard HW capabilities if no enable function

[Michael Walle]

Am 2023-12-20 09:20, schrieb Tudor Ambarus:
> On 19.12.2023 12:21, Jaime Liao wrote:
>> From: JaimeLiao <jaimeliao@mxic.com.tw>
>> 
>> Discard corresponding HW capabilities to prevent carrying the
>> wrong protocol if no QUAD/Octal DTR enable function hooked.
>> 
>> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
>> ---
>> changes in v2
>>  - Add SNOR_HWCAPS_8_8_8_DTR
>>  - Restore the enable function judgement in spi_nor_set_octal_dtr()
>>  - Restore the enable function judgement in spi_nor_quad_enable()
>> ---
>>  drivers/mtd/spi-nor/core.c  | 7 +++++++
>>  include/linux/mtd/spi-nor.h | 6 ++++++
>>  2 files changed, 13 insertions(+)
>> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 1c443fe568cf..14359101c6cf 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor 
>> *nor,
>>  	 */
>>  	shared_mask = hwcaps->mask & params->hwcaps.mask;
>> 
>> +	/* Mask out Octal DTR if no enable function */
>> +	if (!params->set_octal_dtr)
>> +		shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR;
>> +
>> +	if (!params->quad_enable)
>> +		shared_mask &= ~SNOR_HWCAPS_4_4_4;
> 
> and these should have been in the late init hook, and instead discard
> them from the params->hwcaps.mask.

Maybe there is a better place to mask these bits. But IMHO the core
should do it on itself and we shouldn't need to provide an extra
hook function for every driver ourselves. The core knows that there
is no .octal_enable op and thus it shouldn't even try to enable
this mode.

> And why is this extra check needed in the first place? For octal I
> assume it's needed by macronix, where the flash is octal capable, but
> there's no octal enable method defined yet.

Because it is the generic spi nor flash driver, which doesn't provide
any of these enable x-mode helpers.

> What about the quad mode? Is
> there any flash that has no quad enable method defined but still caries
> quad mode in its hwcaps?

No it is for completeness and correctness. At the moment we are always
setting a (random) default quad enable op, due to legacy reasons.

-michael

>> +
>>  	if (nor->spimem) {
>>  		/*
>>  		 * When called from spi_nor_probe(), all caps are set and we
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index cdcfe0fd2e7d..78a119192ee0 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -266,6 +266,12 @@ struct spi_nor_hwcaps {
>>  #define SNOR_HWCAPS_PP_8_8_8		BIT(22)
>>  #define SNOR_HWCAPS_PP_8_8_8_DTR	BIT(23)
>> 
>> +#define SNOR_HWCAPS_4_4_4	(SNOR_HWCAPS_READ_4_4_4 |	\
>> +				 SNOR_HWCAPS_PP_4_4_4)
>> +
>> +#define SNOR_HWCAPS_8_8_8_DTR	(SNOR_HWCAPS_READ_8_8_8_DTR |	\
>> +				 SNOR_HWCAPS_PP_8_8_8_DTR)
>> +
>>  #define SNOR_HWCAPS_X_X_X	(SNOR_HWCAPS_READ_2_2_2 |	\
>>  				 SNOR_HWCAPS_READ_4_4_4 |	\
>>  				 SNOR_HWCAPS_READ_8_8_8 |	\

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

Re: [PATCH v2] mtd: spi-nor: core: Discard HW capabilities if no enable function

[Tudor Ambarus]

On 20.12.2023 10:50, Michael Walle wrote:
> Am 2023-12-20 09:20, schrieb Tudor Ambarus:
>> On 19.12.2023 12:21, Jaime Liao wrote:
>>> From: JaimeLiao <jaimeliao@mxic.com.tw>
>>>
>>> Discard corresponding HW capabilities to prevent carrying the
>>> wrong protocol if no QUAD/Octal DTR enable function hooked.
>>>
>>> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
>>> ---
>>> changes in v2
>>>  - Add SNOR_HWCAPS_8_8_8_DTR
>>>  - Restore the enable function judgement in spi_nor_set_octal_dtr()
>>>  - Restore the enable function judgement in spi_nor_quad_enable()
>>> ---
>>>  drivers/mtd/spi-nor/core.c  | 7 +++++++
>>>  include/linux/mtd/spi-nor.h | 6 ++++++
>>>  2 files changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 1c443fe568cf..14359101c6cf 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct
>>> spi_nor *nor,
>>>       */
>>>      shared_mask = hwcaps->mask & params->hwcaps.mask;
>>>
>>> +    /* Mask out Octal DTR if no enable function */
>>> +    if (!params->set_octal_dtr)
>>> +        shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR;
>>> +
>>> +    if (!params->quad_enable)
>>> +        shared_mask &= ~SNOR_HWCAPS_4_4_4;
>>
>> and these should have been in the late init hook, and instead discard
>> them from the params->hwcaps.mask.
> 
> Maybe there is a better place to mask these bits. But IMHO the core
> should do it on itself and we shouldn't need to provide an extra
> hook function for every driver ourselves. The core knows that there
> is no .octal_enable op and thus it shouldn't even try to enable
> this mode.

I meant in the core, at the end of the late_init_params(). Here:
https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n2962
> 
>> And why is this extra check needed in the first place? For octal I
>> assume it's needed by macronix, where the flash is octal capable, but
>> there's no octal enable method defined yet.
> 
> Because it is the generic spi nor flash driver, which doesn't provide
> any of these enable x-mode helpers.

right, that was my guess too. The commit message has to be updated.
> 
>> What about the quad mode? Is
>> there any flash that has no quad enable method defined but still caries
>> quad mode in its hwcaps?
> 
> No it is for completeness and correctness. At the moment we are always
> setting a (random) default quad enable op, due to legacy reasons.
> 

Still, the commit message shall indicate this. Can't add extra checks
out of the blue. Especially since nobody is affected.

Cheers,
ta

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

Re: [PATCH v2] mtd: spi-nor: core: Discard HW capabilities if no enable function

[Michael Walle]

Hi,

>>> and these should have been in the late init hook, and instead discard
>>> them from the params->hwcaps.mask.
>> 
>> Maybe there is a better place to mask these bits. But IMHO the core
>> should do it on itself and we shouldn't need to provide an extra
>> hook function for every driver ourselves. The core knows that there
>> is no .octal_enable op and thus it shouldn't even try to enable
>> this mode.
> 
> I meant in the core, at the end of the late_init_params(). Here:
> https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n2962

Sounds good.

>> No it is for completeness and correctness. At the moment we are always
>> setting a (random) default quad enable op, due to legacy reasons.
>> 
> 
> Still, the commit message shall indicate this. Can't add extra checks
> out of the blue. Especially since nobody is affected.

Ok :)

-michael

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