All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] mtd: spi-nor: Respect flash's hwcaps in spi_nor_adjust_hwcaps()
@ 2021-07-29 11:58 Bin Meng
  2021-07-29 11:58 ` [PATCH 2/2] mtd: spi-nor: Mask out fast read if not requested in DT Bin Meng
  2021-07-29 12:08 ` [PATCH 1/2] mtd: spi-nor: Respect flash's hwcaps in spi_nor_adjust_hwcaps() Pratyush Yadav
  0 siblings, 2 replies; 5+ messages in thread
From: Bin Meng @ 2021-07-29 11:58 UTC (permalink / raw)
  To: Jagan Teki, Vignesh Raghavendra, Pratyush Yadav, u-boot

The smart spi_nor_adjust_hwcaps() does not respect the SPI flash's
hwcaps, and only looks to the controller on what can be supported.

The flash's hwcaps needs to be AND'ed before checking.

Fixes: 71025f013ccb ("mtd: spi-nor-core: Rework hwcaps selection")
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi/spi-nor-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 99e2f16349..2883d092fc 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -2861,7 +2861,7 @@ spi_nor_adjust_hwcaps(struct spi_nor *nor,
 	 * Enable all caps by default. We will mask them after checking what's
 	 * really supported using spi_mem_supports_op().
 	 */
-	*hwcaps = SNOR_HWCAPS_ALL;
+	*hwcaps = SNOR_HWCAPS_ALL & params->hwcaps.mask;
 
 	/* X-X-X modes are not supported yet, mask them all. */
 	*hwcaps &= ~SNOR_HWCAPS_X_X_X;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2] mtd: spi-nor: Mask out fast read if not requested in DT
  2021-07-29 11:58 [PATCH 1/2] mtd: spi-nor: Respect flash's hwcaps in spi_nor_adjust_hwcaps() Bin Meng
@ 2021-07-29 11:58 ` Bin Meng
  2021-07-30  7:24   ` Pratyush Yadav
  2021-07-29 12:08 ` [PATCH 1/2] mtd: spi-nor: Respect flash's hwcaps in spi_nor_adjust_hwcaps() Pratyush Yadav
  1 sibling, 1 reply; 5+ messages in thread
From: Bin Meng @ 2021-07-29 11:58 UTC (permalink / raw)
  To: Jagan Teki, Vignesh Raghavendra, Pratyush Yadav, u-boot

The DT bindings of "jedec,spi-nor" [1] defines "m25p,fast-read" property
to indicate that "fast read" opcode can be used to read data from the
chip instead of the usual "read" opcode.

If this property is not present in DT, mask out fast read in
spi_nor_init_params(). This change mirrors the same logic in
spi_nor_info_init_params() in drivers/mtd/spi-nor/core.c in
the Linux kernel v5.14-rc3.

[1] Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml in the kernel tree

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi/spi-nor-core.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 2883d092fc..d6a0ed39c5 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -2597,6 +2597,8 @@ static int spi_nor_init_params(struct spi_nor *nor,
 			       const struct flash_info *info,
 			       struct spi_nor_flash_parameter *params)
 {
+	struct spi_slave *spi = nor->spi;
+
 	/* Set legacy flash parameters as default. */
 	memset(params, 0, sizeof(*params));
 
@@ -2604,18 +2606,25 @@ static int spi_nor_init_params(struct spi_nor *nor,
 	params->size = info->sector_size * info->n_sectors;
 	params->page_size = info->page_size;
 
+	if (!(info->flags & SPI_NOR_NO_FR)) {
+		/* Default to Fast Read for DT and non-DT platform devices. */
+		params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
+
+		/* Mask out Fast Read if not requested at DT instantiation. */
+		if (!ofnode_read_bool(dev_ofnode(spi->dev), "m25p,fast-read"))
+			params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
+	}
+
 	/* (Fast) Read settings. */
 	params->hwcaps.mask |= SNOR_HWCAPS_READ;
 	spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ],
 				  0, 0, SPINOR_OP_READ,
 				  SNOR_PROTO_1_1_1);
 
-	if (!(info->flags & SPI_NOR_NO_FR)) {
-		params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
+	if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)
 		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST],
 					  0, 8, SPINOR_OP_READ_FAST,
 					  SNOR_PROTO_1_1_1);
-	}
 
 	if (info->flags & SPI_NOR_DUAL_READ) {
 		params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: Respect flash's hwcaps in spi_nor_adjust_hwcaps()
  2021-07-29 11:58 [PATCH 1/2] mtd: spi-nor: Respect flash's hwcaps in spi_nor_adjust_hwcaps() Bin Meng
  2021-07-29 11:58 ` [PATCH 2/2] mtd: spi-nor: Mask out fast read if not requested in DT Bin Meng
@ 2021-07-29 12:08 ` Pratyush Yadav
  2021-07-29 12:28   ` Bin Meng
  1 sibling, 1 reply; 5+ messages in thread
From: Pratyush Yadav @ 2021-07-29 12:08 UTC (permalink / raw)
  To: Bin Meng; +Cc: Jagan Teki, Vignesh Raghavendra, u-boot

Hi Bin,

On 29/07/21 07:58PM, Bin Meng wrote:
> The smart spi_nor_adjust_hwcaps() does not respect the SPI flash's
> hwcaps, and only looks to the controller on what can be supported.
> 
> The flash's hwcaps needs to be AND'ed before checking.
> 
> Fixes: 71025f013ccb ("mtd: spi-nor-core: Rework hwcaps selection")
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  drivers/mtd/spi/spi-nor-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 99e2f16349..2883d092fc 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -2861,7 +2861,7 @@ spi_nor_adjust_hwcaps(struct spi_nor *nor,
>  	 * Enable all caps by default. We will mask them after checking what's
>  	 * really supported using spi_mem_supports_op().
>  	 */

Please update the comment above. Maybe:

  Start by assuming the controller supports every cap. We will mask 
  them...

> -	*hwcaps = SNOR_HWCAPS_ALL;
> +	*hwcaps = SNOR_HWCAPS_ALL & params->hwcaps.mask;

This is practically equivalent to *hwcaps = params->hwcaps.mask but I 
think having SNOR_HWCAPS_ALL here to signify that we are assuming 
controller supports everything is not a bad idea IMO.

With the above comment updated,

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Thanks.

>  
>  	/* X-X-X modes are not supported yet, mask them all. */
>  	*hwcaps &= ~SNOR_HWCAPS_X_X_X;
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: Respect flash's hwcaps in spi_nor_adjust_hwcaps()
  2021-07-29 12:08 ` [PATCH 1/2] mtd: spi-nor: Respect flash's hwcaps in spi_nor_adjust_hwcaps() Pratyush Yadav
@ 2021-07-29 12:28   ` Bin Meng
  0 siblings, 0 replies; 5+ messages in thread
From: Bin Meng @ 2021-07-29 12:28 UTC (permalink / raw)
  To: Pratyush Yadav; +Cc: Jagan Teki, Vignesh Raghavendra, U-Boot Mailing List

Hi Pratyush,

On Thu, Jul 29, 2021 at 8:08 PM Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Hi Bin,
>
> On 29/07/21 07:58PM, Bin Meng wrote:
> > The smart spi_nor_adjust_hwcaps() does not respect the SPI flash's
> > hwcaps, and only looks to the controller on what can be supported.
> >
> > The flash's hwcaps needs to be AND'ed before checking.
> >
> > Fixes: 71025f013ccb ("mtd: spi-nor-core: Rework hwcaps selection")
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  drivers/mtd/spi/spi-nor-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> > index 99e2f16349..2883d092fc 100644
> > --- a/drivers/mtd/spi/spi-nor-core.c
> > +++ b/drivers/mtd/spi/spi-nor-core.c
> > @@ -2861,7 +2861,7 @@ spi_nor_adjust_hwcaps(struct spi_nor *nor,
> >        * Enable all caps by default. We will mask them after checking what's
> >        * really supported using spi_mem_supports_op().
> >        */
>
> Please update the comment above. Maybe:
>
>   Start by assuming the controller supports every cap. We will mask
>   them...
>
> > -     *hwcaps = SNOR_HWCAPS_ALL;
> > +     *hwcaps = SNOR_HWCAPS_ALL & params->hwcaps.mask;
>
> This is practically equivalent to *hwcaps = params->hwcaps.mask but I
> think having SNOR_HWCAPS_ALL here to signify that we are assuming
> controller supports everything is not a bad idea IMO.
>
> With the above comment updated,
>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>

Thanks for the review! Will update comments in v2.

Regards,
Bin

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: Mask out fast read if not requested in DT
  2021-07-29 11:58 ` [PATCH 2/2] mtd: spi-nor: Mask out fast read if not requested in DT Bin Meng
@ 2021-07-30  7:24   ` Pratyush Yadav
  0 siblings, 0 replies; 5+ messages in thread
From: Pratyush Yadav @ 2021-07-30  7:24 UTC (permalink / raw)
  To: Bin Meng; +Cc: Jagan Teki, Vignesh Raghavendra, u-boot

Hi Bin,

On 29/07/21 07:58PM, Bin Meng wrote:
> The DT bindings of "jedec,spi-nor" [1] defines "m25p,fast-read" property
> to indicate that "fast read" opcode can be used to read data from the
> chip instead of the usual "read" opcode.
> 
> If this property is not present in DT, mask out fast read in
> spi_nor_init_params(). This change mirrors the same logic in
> spi_nor_info_init_params() in drivers/mtd/spi-nor/core.c in
> the Linux kernel v5.14-rc3.
> 
> [1] Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml in the kernel tree
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  drivers/mtd/spi/spi-nor-core.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 2883d092fc..d6a0ed39c5 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -2597,6 +2597,8 @@ static int spi_nor_init_params(struct spi_nor *nor,
>  			       const struct flash_info *info,
>  			       struct spi_nor_flash_parameter *params)
>  {
> +	struct spi_slave *spi = nor->spi;
> +
>  	/* Set legacy flash parameters as default. */
>  	memset(params, 0, sizeof(*params));
>  
> @@ -2604,18 +2606,25 @@ static int spi_nor_init_params(struct spi_nor *nor,
>  	params->size = info->sector_size * info->n_sectors;
>  	params->page_size = info->page_size;
>  
> +	if (!(info->flags & SPI_NOR_NO_FR)) {
> +		/* Default to Fast Read for DT and non-DT platform devices. */
> +		params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
> +
> +		/* Mask out Fast Read if not requested at DT instantiation. */
> +		if (!ofnode_read_bool(dev_ofnode(spi->dev), "m25p,fast-read"))

Do you need to check if dev_ofnode() is valid? Can spi-nor be 
instantiated from a non-DT platform (like platdata maybe)? I am not very 
familiar with DT/platdata stuff, other than the basics.

> +			params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
> +	}
> +
>  	/* (Fast) Read settings. */
>  	params->hwcaps.mask |= SNOR_HWCAPS_READ;
>  	spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ],
>  				  0, 0, SPINOR_OP_READ,
>  				  SNOR_PROTO_1_1_1);
>  
> -	if (!(info->flags & SPI_NOR_NO_FR)) {
> -		params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
> +	if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)
>  		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST],
>  					  0, 8, SPINOR_OP_READ_FAST,
>  					  SNOR_PROTO_1_1_1);
> -	}

The patch looks good to me other than the point above.

>  
>  	if (info->flags & SPI_NOR_DUAL_READ) {
>  		params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2;
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-07-30  7:24 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-29 11:58 [PATCH 1/2] mtd: spi-nor: Respect flash's hwcaps in spi_nor_adjust_hwcaps() Bin Meng
2021-07-29 11:58 ` [PATCH 2/2] mtd: spi-nor: Mask out fast read if not requested in DT Bin Meng
2021-07-30  7:24   ` Pratyush Yadav
2021-07-29 12:08 ` [PATCH 1/2] mtd: spi-nor: Respect flash's hwcaps in spi_nor_adjust_hwcaps() Pratyush Yadav
2021-07-29 12:28   ` Bin Meng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.