Hi Vignesh,
> -----Original Message-----
> From: Vignesh Raghavendra <vigne...@ti.com>
> Sent: Monday, January 27, 2020 10:48 AM
> To: Sagar Kadam <sagar.ka...@sifive.com>; u-boot@lists.denx.de
> Cc: Paul Walmsley ( Sifive) <paul.walms...@sifive.com>;
> anup.pa...@wdc.com; atish.pa...@wdc.com; ja...@amarulasolutions.com;
> Rick Chen <r...@andestech.com>
> Subject: Re: [U-Boot Patch v1 6/7] nor: add post bfpt fix handler for
> is25wp256 device
>
>
>
> On 24/01/20 11:58 am, Sagar Kadam wrote:
> > Hello Vignesh,
> >
> >> -----Original Message-----
> >> From: Vignesh Raghavendra <vigne...@ti.com>
> >> Sent: Friday, January 24, 2020 10:24 AM
> >> To: Sagar Kadam <sagar.ka...@sifive.com>; u-boot@lists.denx.de
> >> Cc: Paul Walmsley ( Sifive) <paul.walms...@sifive.com>;
> >> anup.pa...@wdc.com; i...@andestech.com; atish.pa...@wdc.com;
> >> ja...@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.ka...@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.
> >
>
> Unless there is a real dependency, it would be great to separate out
> SPI-NOR related changes to different series, so that it could go via
> Jagan's SPI tree.
>
> Regards
> Vignesh
>
Yes, I will exclude this patch from this series.
-BR,
Sagar
> >>
> >>> 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
>
> --
> Regards
> Vignesh
