Re: [PATCH 1/7] mtd: spi-nor: core: Introduce SPI_NOR_PARSE_SFDP

[Pratyush Yadav <p.yadav@ti.com>]

Hi Tudor,

I only have small commit message nitpicks/improvements for this patch.

On 02/07/21 05:41PM, Tudor Ambarus wrote:
> SPI NOR flashes that statically declare one of the
> SPI_NOR_{DUAL, QUAD, OCTAL, OCTAL_DTR}_READ flags, and do not support
> the RDSFDP command, are gratuiously receiving the RDSFDP command,

I think you should drop all the commas in this sentence. They seem 
unnecessary to me.

> in the attempt of parsing the SFDP tables. It is not desirable to issue
> comands that are not supported, so introduce a flag to help on this

s/comands/commands/

> situation.
> 
> New flash additions that support the SFDP standard should be declared
> using SPI_NOR_PARSE_SFDP. Support that can be discovered when parsing
> SFDP should not be duplicated by explicit flags at flash declaration.
> All the flash parameters will be discovered when parsing SFDP.
> Sometimes manufacturers wrongly define some fields in the SFDP tables.
> If that's the case, SFDP data can be ammended with the fixups() hooks.

s/ammended/amended/

> It is not common, but if the SFDP tables are entirely wrong, and it
> does not worth the hassle to tweak the SFDP parameters by using the
> fixups hooks, or if the flash does not define the SFDP tables at all,
> then statically init the flash with the SPI_NOR_SKIP_SFDP flag and
> specify the reset of flash capabilities with the flash info flags.

s/reset/rest/ ?

> 
> With time, we want to convert all flashes to SPI_NOR_PARSE_SFDP and
> stop triggering the SFDP parsing with the
> SPI_NOR_{DUAL, QUAD, OCTAL*}_READ flags. Getting rid of the
> SPI_NOR_{OCTAL, OCTAL_DTR}_READ trigger is easy achievable, the reset

s/easy/easily/

s/reset/rest/ ?

> are a long term goal.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 3 ++-
>  drivers/mtd/spi-nor/core.h | 4 ++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index cc08bd707378..3d9f3698fb7b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2726,7 +2726,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  
>  	spi_nor_manufacturer_init_params(nor);
>  
> -	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +	if ((nor->info->flags & (SPI_NOR_PARSE_SFDP |
> +				 SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |

Ok.

>  				 SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
>  	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
>  		spi_nor_sfdp_init_params(nor);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 3348e1dd1445..55fceb0ec894 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -382,6 +382,10 @@ struct flash_info {
>  					 * protection bits. Usually these will
>  					 * power-up in a write-protected state.
>  					 */
> +#define SPI_NOR_PARSE_SFDP	BIT(23) /*
> +					 * Flash initialized based on the SFDP
> +					 * tables.
> +					 */

We will be running out of bits to use soon at this rate. But I don't 
think it is too much of a worry since it should be easy to make flags 
u64.

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

>  
>  	const struct spi_nor_otp_organization otp_org;
>  
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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