Re: [PATCH 1/2 v2] mtd: spi-nor: add 4bit block protection support

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

+Uwe, he showed interest in this.

Hi, Jungseung,

On 12/12/2018 12:25 PM, Jungseung Lee wrote:
> Currently, we are supporting block protection only for
> flash chips with 3 block protection bits in the SR register.
> This patch enables block protection support for some flash with
> 4 block protection bits (bp0-3).
> 
> Because this feature is not universal to all flash that support
> lock/unlock, control it via a new flag.
> 
> Signed-off-by: Jungseung Lee <js07.lee@xxxxxxxxxxx>
> ---
> ChangeLog v1->v2:
> - Rebase on latest MTD development branch
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 61 ++++++++++++++++++++++++++++++-----
>  include/linux/mtd/spi-nor.h   |  4 +++
>  2 files changed, 57 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 6e13bbd1aaa5..c33b72eeae12 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -96,6 +96,7 @@ enum spi_nor_pp_command_index {
>  struct spi_nor_flash_parameter {
>  	u64				size;
>  	u32				page_size;
> +	u16				n_sectors;
>  
>  	struct spi_nor_hwcaps		hwcaps;
>  	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
> @@ -278,6 +279,7 @@ struct flash_info {
>  #define NO_CHIP_ERASE		BIT(12) /* Chip does not support chip erase */
>  #define SPI_NOR_SKIP_SFDP	BIT(13)	/* Skip parsing of SFDP tables */
>  #define USE_CLSR		BIT(14)	/* use CLSR command */
> +#define SPI_NOR_HAS_BP3		BIT(15)	/* use 4 bits field for block protect */
>  
>  	/* Part specific fixup hooks. */
>  	const struct spi_nor_fixups *fixups;
> @@ -1087,18 +1089,36 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
>  	int shift = ffs(mask) - 1;
>  	int pow;
>  
> +	if (nor->flags & SNOR_F_HAS_SR_BP3)
> +		mask |= SR_BP3;

I asked myself if the shift variable is still conclusive after updating the
mask. I assume we can add a macro for it, to skip this kind of questions.
> +
>  	if (!(sr & mask)) {
>  		/* No protection */
>  		*ofs = 0;
>  		*len = 0;
> +		return;
> +	}
> +
> +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> +		u8 temp = sr & mask;
> +
> +		if (temp & SR_BP3)
> +			temp = (temp & ~SR_BP3) | BIT(5);

N25Q512A defines SR_BP3 at BIT(6) indeed, but W25M512JV defines it directly at
BIT(5) and moves the TB definition at BIT(6). We will need to know how the large
majority of manufacturers are implementing BP3, and based on that to decide how
we will implement the "generic" BP3 support. I would look at spi_nor_ids[],
check which manufacturers have flashes that set SPI_NOR_HAS_LOCK and I would
look at those manufacturers for newer flashes that might have BP3 support. Would
you please make this study and tell us your findings?

> +
> +		pow = ilog2(nor->n_sectors) + 1 - (temp >> shift);
> +		if (pow > 0)
> +			*len = mtd->size >> pow;
> +		else
> +			*len = mtd->size;

having negative values for pow is not so intuitive. Use Brian's way of computing
pow, and it will result in nicer code: pow = ((sr & mask) ^ mask) >> shift;

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