[U-Boot Patch v1 6/7] nor: add post bfpt fix handler for is25wp256 device

Sagar Kadam sagar.kadam at sifive.com
Fri Jan 24 07:28:53 CET 2020


Hello Vignesh,

> -----Original Message-----
> From: Vignesh Raghavendra <vigneshr at ti.com>
> Sent: Friday, January 24, 2020 10:24 AM
> To: Sagar Kadam <sagar.kadam at sifive.com>; u-boot at lists.denx.de
> Cc: Paul Walmsley ( Sifive) <paul.walmsley at sifive.com>;
> anup.patel at wdc.com; ick at andestech.com; atish.patra at wdc.com;
> jagan at amarulasolutions.com
> Subject: Re: [U-Boot Patch v1 6/7] nor: add post bfpt fix handler for
> is25wp256 device
> 
> Hi,
> 
> On 24/01/20 1:46 am, Sagar Shrikant Kadam wrote:
> > Update vendor id for ISSI flash, enable SFDP as ISSI flash supports it
> > and add support for spi_nor_fixups similar to that done in linux.
> > Flash vendor specific fixups can be registered in spi_nor_ids, and
> > will be called after BFPT parsing to fix any wrong parameter read from
> > SFDP.
> >
> > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam at sifive.com>
> > ---
> >  board/sifive/fu540/Kconfig     |  1 +
> >  drivers/mtd/spi/sf_internal.h  | 16 +++++++++++
> > drivers/mtd/spi/spi-nor-core.c | 63
> > ++++++++++++++++++++++++++++++++++++++++--
> >  drivers/mtd/spi/spi-nor-ids.c  |  7 ++++-
> >  include/linux/mtd/spi-nor.h    |  1 +
> >  5 files changed, 85 insertions(+), 3 deletions(-)
> >
> > diff --git a/board/sifive/fu540/Kconfig b/board/sifive/fu540/Kconfig
> > index 75661f3..d9e4956 100644
> > --- a/board/sifive/fu540/Kconfig
> > +++ b/board/sifive/fu540/Kconfig
> > @@ -42,6 +42,7 @@ config BOARD_SPECIFIC_OPTIONS # dummy
> >  	imply SPI
> >  	imply SPI_SIFIVE
> >  	imply SPI_FLASH
> > +	imply SPI_FLASH_SFDP_SUPPORT
> >  	imply SPI_FLASH_ISSI
> >  	imply MMC
> >  	imply MMC_SPI
> 
> This does not belong to this patch. Its better that this change goes via SiFive
> SoC tree.

Thanks for your inputs. I will move it.
Just that I understand this correctly shall I add this config change as a separate patch apart from the series
or as a different patch containing only this change within this series.

> 
> > diff --git a/drivers/mtd/spi/sf_internal.h
> > b/drivers/mtd/spi/sf_internal.h index 5c64303..856866f 100644
> > --- a/drivers/mtd/spi/sf_internal.h
> > +++ b/drivers/mtd/spi/sf_internal.h
> > @@ -66,8 +66,24 @@ struct flash_info {
> >  #define SPI_NOR_SKIP_SFDP	BIT(13)	/* Skip parsing of SFDP tables */
> >  #define USE_CLSR		BIT(14)	/* use CLSR command */
> >  #define SPI_NOR_HAS_SST26LOCK	BIT(15)	/* Flash supports lock/unlock
> via BPR */
> > +
> > +#ifdef CONFIG_SPI_FLASH_SFDP_SUPPORT
> 
> Change above ifdef to:
> 
> 	#if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT)
> 
> throughout the patch to take care of both SPL and U-Boot builds
> 
Sure, will modify this and send it in V2.

> > +	/* Part specific fixup hooks */
> > +	const struct spi_nor_fixups	*fixups;
> > +#endif
> >  };
> >
> > +#ifdef CONFIG_SPI_FLASH_SFDP_SUPPORT
> > +/*
> > + * Declare manufacturer specific fixup handlers that
> > + * can be registered as fixup's in flash info table
> > + * so as to update any wrong/broken SFDP parameter.
> > + */
> > +#ifdef CONFIG_SPI_FLASH_ISSI
> > +extern struct spi_nor_fixups is25wp256_fixups; #endif #endif
> > +
> >  extern const struct flash_info spi_nor_ids[];
> >
> >  #define JEDEC_MFR(info)	((info)->id[0])
> > diff --git a/drivers/mtd/spi/spi-nor-core.c
> > b/drivers/mtd/spi/spi-nor-core.c index 6e7fc23..c55116f 100644
> > --- a/drivers/mtd/spi/spi-nor-core.c
> > +++ b/drivers/mtd/spi/spi-nor-core.c
> > @@ -296,7 +296,6 @@ static void spi_nor_set_4byte_opcodes(struct
> spi_nor *nor,
> >  		nor->mtd.erasesize = info->sector_size;
> >  		break;
> >
> > -	default:
> 
> Is this an intentional change?
> 
No this was not intentional, got reverted in 7/7 
I will correct it.

> >  		break;
> >  	}
> >
> > @@ -1800,6 +1799,57 @@ static const struct sfdp_bfpt_erase
> > sfdp_bfpt_erases[] = {
> >
> >  static int spi_nor_hwcaps_read2cmd(u32 hwcaps);
> >
> > +#ifdef CONFIG_SPI_FLASH_SFDP_SUPPORT
> > +/**
> > + * struct spi_nor_fixups - SPI NOR fixup hooks
> > + * @post_bfpt: called after the BFPT table has been parsed
> > + *
> > + * Those hooks can be used to tweak the SPI NOR configuration when
> > +the SFDP
> > + * table is broken or not available.
> > + */
> > +struct spi_nor_fixups {
> > +	int (*post_bfpt)(struct spi_nor *nor,
> > +			 const struct sfdp_parameter_header *bfpt_header,
> > +			 const struct sfdp_bfpt *bfpt,
> > +			 struct spi_nor_flash_parameter *params); };
> > +
> > +static int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
> > +				    const struct sfdp_parameter_header
> > +						*bfpt_header,
> > +				    const struct sfdp_bfpt *bfpt,
> > +				    struct spi_nor_flash_parameter *params) {
> > +	if (nor->info->fixups && nor->info->fixups->post_bfpt)
> > +		return nor->info->fixups->post_bfpt(nor, bfpt_header, bfpt,
> > +				params);
> > +
> > +	return 0;
> > +}
> > +
> > +static int is25wp256_post_bfpt_fixups(struct spi_nor *nor,
> > +				      const struct sfdp_parameter_header
> > +						*bfpt_header,
> > +				      const struct sfdp_bfpt *bfpt,
> > +				      struct spi_nor_flash_parameter *params)
> > +
> > +{
> > +	/* IS25WP256 supports 4B opcodes, but the BFPT advertises a
> > +	 * BFPT_DWORD1_ADDRESS_BYTES_3_ONLY address width.
> > +	 * Overwrite the address width advertised by the BFPT.
> > +	 */
> > +	if ((bfpt->dwords[BFPT_DWORD(1)] &
> BFPT_DWORD1_ADDRESS_BYTES_MASK) ==
> > +			BFPT_DWORD1_ADDRESS_BYTES_3_ONLY)
> > +		nor->addr_width = 4;
> > +
> > +	return 0;
> > +}
> > +
> > +struct spi_nor_fixups is25wp256_fixups = {
> > +	.post_bfpt = is25wp256_post_bfpt_fixups, }; #endif
> > +
> >  /**
> >   * spi_nor_parse_bfpt() - read and parse the Basic Flash Parameter Table.
> >   * @nor:		pointer to a 'struct spi_nor'
> > @@ -1971,7 +2021,13 @@ static int spi_nor_parse_bfpt(struct spi_nor
> *nor,
> >  		return -EINVAL;
> >  	}
> >
> > -	return 0;
> > +#ifdef CONFIG_SPI_FLASH_SFDP_SUPPORT
> > +	err = spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt,
> > +				       params);
> 
> Isn't this function (spi_nor_parse_bfpt()) already under
> CONFIG_IS_ENABLED(CONFIG_SPI_FLASH_SFDP_SUPPORT)?
>

Oh yes, thanks for catching this I will remove this guard here.
 
> > +#else
> > +	err = 0;
> > +#endif
> > +	return err;
> >  }
> >
> >  /**
> > @@ -2209,6 +2265,9 @@ static int spi_nor_init_params(struct spi_nor
> *nor,
> >  	    !(info->flags & SPI_NOR_SKIP_SFDP)) {
> >  		struct spi_nor_flash_parameter sfdp_params;
> >
> > +		/* Update flash structure information to nor structure */
> > +		nor->info = info;
> > +
> 
> Instead, could you update spi_nor_scan() to initialize nor->info before calling
> spi_nor_init_params()?
> 
Yes, I will move it to spi_nor_scan()

Thanks,
-Sagar

> >  		memcpy(&sfdp_params, params, sizeof(sfdp_params));
> >  		if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
> >  			nor->addr_width = 0;
> > diff --git a/drivers/mtd/spi/spi-nor-ids.c
> > b/drivers/mtd/spi/spi-nor-ids.c index 973b6f8..5a29c8b 100644
> > --- a/drivers/mtd/spi/spi-nor-ids.c
> > +++ b/drivers/mtd/spi/spi-nor-ids.c
> > @@ -135,7 +135,12 @@ const struct flash_info spi_nor_ids[] = {
> >  	{ INFO("is25wp128",  0x9d7018, 0, 64 * 1024, 256,
> >  			SECT_4K | SPI_NOR_DUAL_READ |
> SPI_NOR_QUAD_READ) },
> >  	{ INFO("is25wp256",  0x9d7019, 0, 64 * 1024, 512,
> > -			SECT_4K | SPI_NOR_DUAL_READ |
> SPI_NOR_QUAD_READ) },
> > +			SECT_4K | SPI_NOR_4B_OPCODES |
> SPI_NOR_DUAL_READ
> > +			| SPI_NOR_QUAD_READ)
> > +#ifdef CONFIG_SPI_FLASH_SFDP_SUPPORT
> > +			.fixups = &is25wp256_fixups
> > +#endif
> > +	},
> >  #endif
> >  #ifdef CONFIG_SPI_FLASH_MACRONIX	/* MACRONIX */
> >  	/* Macronix */
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index 1d91177..44b7479 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -23,6 +23,7 @@
> >  #define SNOR_MFR_ST		CFI_MFR_ST /* ST Micro <--> Micron
> */
> >  #define SNOR_MFR_MICRON		CFI_MFR_MICRON /* ST
> Micro <--> Micron */
> >  #define SNOR_MFR_MACRONIX	CFI_MFR_MACRONIX
> > +#define SNOR_MFR_ISSI		CFI_MFR_PMC
> >  #define SNOR_MFR_SPANSION	CFI_MFR_AMD
> >  #define SNOR_MFR_SST		CFI_MFR_SST
> >  #define SNOR_MFR_WINBOND	0xef /* Also used by some Spansion
> */
> >
> 
> --
> Regards
> Vignesh


More information about the U-Boot mailing list