[U-Boot] [PATCH 0/4] mtd: spi: spi-nor-core: Add Microchip SFDP parser

[U-Boot] [PATCH 0/4] mtd: spi: spi-nor-core: Add Microchip SFDP parser

[Tudor.Ambarus at microchip.com]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Parse manufacturer specific SFDP table. The Microchip SFDP table
contains pre-programmed globally unique MAC addresses. Retrieve
the MAC address from the SPI NOR flash and set it in ethaddr in env.

This can go through Eugen's tree if no obiections. Otherwise Eugen
should take care to apply the Microchip specific code after
Jagan's/Vignesh's tree gets merged.

Tudor Ambarus (4):
  mtd: spi: spi-nor-core: Add Microchip SFDP parser
  board: atmel: sama5d27_wlsom1_ek: Set ethaddr from spi-nor flash
  configs: sama5d27_wlsom1_ek: qspiflash: Enable SPI NOR ethaddr
    retrieval
  configs: sama5d27_wlsom1_ek: mmc: Enable SPI NOR ethaddr retrieval

 arch/arm/mach-at91/include/mach/at91_common.h      |   1 +
 board/atmel/common/Makefile                        |   1 +
 board/atmel/common/mac-spi-nor.c                   | 127 +++++++++++++++++++++
 .../atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c  |   3 +
 configs/sama5d27_wlsom1_ek_mmc_defconfig           |   7 +-
 configs/sama5d27_wlsom1_ek_qspiflash_defconfig     |   3 +-
 drivers/mtd/spi/spi-nor-core.c                     |  35 ++++++
 include/linux/mtd/spi-nor.h                        |   2 +
 8 files changed, 177 insertions(+), 2 deletions(-)
 create mode 100644 board/atmel/common/mac-spi-nor.c

-- 
2.9.5

[U-Boot] [PATCH 1/4] mtd: spi: spi-nor-core: Add Microchip SFDP parser

[Tudor.Ambarus at microchip.com]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

JESD216 allow vendors to define their own SFDP tables.

Add Microchip SFDP parser. The vendor table is allocated using
resource-managed kmalloc - the table will be freed on driver detach.
It will be accessible by getting the UCLASS_SPI_FLASH's private data.

The Michrochip's SFDP table is particularly of interest because contains
pre-programmed globally unique EUI-48 and EUI-64 identifiers.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi/spi-nor-core.c | 35 +++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |  2 ++
 2 files changed, 37 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 1acff745d1a2..bade7d8a9f79 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -1417,6 +1417,7 @@ struct sfdp_parameter_header {
 
 #define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
 #define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
+#define SFDP_MICROCHIP_ID	0x01bf	/* Manufacturer specific Table */
 
 #define SFDP_SIGNATURE		0x50444653U
 #define SFDP_JESD216_MAJOR	1
@@ -1797,6 +1798,34 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 }
 
 /**
+ * spi_nor_parse_microchip_sfdp() - parse the Microchip manufacturer specific
+ * SFDP table.
+ * @nor:		pointer to a 'struct spi_nor'.
+ * @param_header:	pointer to the SFDP parameter header.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int
+spi_nor_parse_microchip_sfdp(struct spi_nor *nor,
+			     const struct sfdp_parameter_header *param_header)
+{
+	size_t size;
+	u32 addr;
+	int ret;
+
+	size = param_header->length * sizeof(u32);
+	addr = SFDP_PARAM_HEADER_PTP(param_header);
+
+	nor->manufacturer_sfdp = devm_kmalloc(nor->dev, size, GFP_KERNEL);
+	if (!nor->manufacturer_sfdp)
+		return -ENOMEM;
+
+	ret = spi_nor_read_sfdp(nor, addr, size, nor->manufacturer_sfdp);
+
+	return ret;
+}
+
+/**
  * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
  * @nor:		pointer to a 'struct spi_nor'
  * @params:		pointer to the 'struct spi_nor_flash_parameter' to be
@@ -1892,6 +1921,12 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 			dev_info(dev, "non-uniform erase sector maps are not supported yet.\n");
 			break;
 
+		case SFDP_MICROCHIP_ID:
+			err = spi_nor_parse_microchip_sfdp(nor, param_header);
+			if (err)
+				goto exit;
+			break;
+
 		default:
 			break;
 		}
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 88e80af57941..836a50178925 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -251,6 +251,7 @@ struct flash_info;
  * @lock:		the lock for the read/write/erase/lock/unlock operations
  * @dev:		point to a spi device, or a spi nor controller device.
  * @info:		spi-nor part JDEC MFR id and other info
+ * @manufacturer_sfdp:	manufacturer specific SFDP table
  * @page_size:		the page size of the SPI NOR
  * @addr_width:		number of address bytes
  * @erase_opcode:	the opcode for erasing a sector
@@ -289,6 +290,7 @@ struct spi_nor {
 	struct udevice		*dev;
 	struct spi_slave	*spi;
 	const struct flash_info	*info;
+	u8			*manufacturer_sfdp;
 	u32			page_size;
 	u8			addr_width;
 	u8			erase_opcode;
-- 
2.9.5

[U-Boot] [PATCH 2/4] board: atmel: sama5d27_wlsom1_ek: Set ethaddr from spi-nor flash

[Tudor.Ambarus at microchip.com]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

The SST26VF064BEUI spi-nor flash is programmed at the factory with a
globally unique address stored in the SFDP vendor parameter table and
it is permanently writeprotected. Retrieve the EUI-48 address and set it
as ethaddr env.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 arch/arm/mach-at91/include/mach/at91_common.h      |   1 +
 board/atmel/common/Makefile                        |   1 +
 board/atmel/common/mac-spi-nor.c                   | 127 +++++++++++++++++++++
 .../atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c  |   3 +
 4 files changed, 132 insertions(+)
 create mode 100644 board/atmel/common/mac-spi-nor.c

diff --git a/arch/arm/mach-at91/include/mach/at91_common.h b/arch/arm/mach-at91/include/mach/at91_common.h
index e929b5e1d207..01e00c508a8a 100644
--- a/arch/arm/mach-at91/include/mach/at91_common.h
+++ b/arch/arm/mach-at91/include/mach/at91_common.h
@@ -40,6 +40,7 @@ void configure_ddrcfg_input_buffers(bool open);
 #endif
 
 int at91_set_ethaddr(int offset);
+void at91_spi_nor_set_ethaddr(void);
 int at91_video_show_board_info(void);
 
 #endif /* AT91_COMMON_H */
diff --git a/board/atmel/common/Makefile b/board/atmel/common/Makefile
index 4de0912f22e6..6bc8cabb8d6d 100644
--- a/board/atmel/common/Makefile
+++ b/board/atmel/common/Makefile
@@ -5,4 +5,5 @@
 
 obj-y += board.o
 obj-$(CONFIG_I2C_EEPROM) += mac_eeprom.o
+obj-$(CONFIG_SPI_FLASH_SFDP_SUPPORT) += mac-spi-nor.o
 obj-$(CONFIG_DM_VIDEO) += video_display.o
diff --git a/board/atmel/common/mac-spi-nor.c b/board/atmel/common/mac-spi-nor.c
new file mode 100644
index 000000000000..96343678e0b6
--- /dev/null
+++ b/board/atmel/common/mac-spi-nor.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Tudor Ambarus <tudor.ambarus@microchip.com>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <env.h>
+#include <linux/mtd/spi-nor.h>
+#include <netdev.h>
+
+#define ETH_ADDR_SIZE			6
+
+#ifdef CONFIG_SPI_FLASH_SST
+#define SFDP_MICROCHIP_MANUF_ID		0xbf
+#define SFDP_MICROCHIP_MEM_TYPE		0x26
+#define SFDP_MICROCHIP_DEV_ID		0x43
+
+#define SFDP_MICROCHIP_EUI_OFFSET	0x60
+#define SFDP_MICROCHIP_EUI48		0x30
+
+struct sst26vf064beui {
+	u8 manufacturer_id;
+	u8 memory_type;
+	u8 device_id;
+	u8 reserved;
+};
+
+/**
+ * sst26vf064beui_check() - Check the validity of the EUI-48 information from
+ * the sst26vf064beui SPI NOR Microchip SFDP table.
+ * @manufacturer_sfdp:	pointer to the Microchip manufacturer specific SFDP
+ *			table.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int sst26vf064beui_check(const u8 *manufacturer_sfdp)
+{
+	struct sst26vf064beui *sst26vf064beui =
+		(struct sst26vf064beui *)manufacturer_sfdp;
+
+	if (sst26vf064beui->manufacturer_id != SFDP_MICROCHIP_MANUF_ID)
+		return -EINVAL;
+
+	if (sst26vf064beui->memory_type != SFDP_MICROCHIP_MEM_TYPE)
+		return -EINVAL;
+
+	if (sst26vf064beui->device_id != SFDP_MICROCHIP_DEV_ID)
+		return -EINVAL;
+
+	/*
+	 * Check if the EUI-48 MAC address is programmed in the next six address
+	 * locations.
+	 */
+	if (manufacturer_sfdp[SFDP_MICROCHIP_EUI_OFFSET] !=
+	    SFDP_MICROCHIP_EUI48)
+		return -EINVAL;
+
+	return 0;
+}
+
+/**
+ * sst26vf064beui_get_ethaddr() - Get the ethernet address from the
+ * sst26vf064beui SPI NOR Microchip SFDP table.
+ * @manufacturer_sfdp:	pointer to the Microchip manufacturer specific SFDP
+ *			table.
+ * @ethaddr:		pointer where to fill the ethernet address
+ * @size:		size of the ethernet address.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int sst26vf064beui_get_ethaddr(const u8 *manufacturer_sfdp,
+				      u8 *ethaddr, size_t size)
+{
+	u64 eui_table[2];
+	u64 *p = (u64 *)&manufacturer_sfdp[SFDP_MICROCHIP_EUI_OFFSET];
+	int i, ret;
+
+	ret = sst26vf064beui_check(manufacturer_sfdp);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < 2; i++)
+		eui_table[i] = le64_to_cpu(p[i]);
+
+	/* Ethaddr starts@offset one. */
+	memcpy(ethaddr, &((u8 *)eui_table)[1], size);
+
+	return 0;
+}
+#endif
+
+/**
+ * at91_spi_nor_set_ethaddr() - Retrieve and set the ethernet address from the
+ * SPI NOR manufacturer specific SFDP table.
+ */
+void at91_spi_nor_set_ethaddr(void)
+{
+	struct udevice *dev;
+	struct spi_nor *nor;
+	const char *ethaddr_name = "ethaddr";
+	u8 ethaddr[ETH_ADDR_SIZE] = {0};
+
+	if (env_get(ethaddr_name))
+		return;
+
+	if (uclass_first_device_err(UCLASS_SPI_FLASH, &dev))
+		return;
+
+	nor = dev_get_uclass_priv(dev);
+	if (!nor)
+		return;
+
+	if (!nor->manufacturer_sfdp)
+		return;
+
+#ifdef CONFIG_SPI_FLASH_SST
+	if (sst26vf064beui_get_ethaddr(nor->manufacturer_sfdp, ethaddr,
+				       ETH_ADDR_SIZE))
+		return;
+#endif
+
+	if (is_valid_ethaddr(ethaddr))
+		eth_env_set_enetaddr(ethaddr_name, ethaddr);
+}
diff --git a/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c b/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
index fda06c824d53..fc563ebb7150 100644
--- a/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
+++ b/board/atmel/sama5d27_wlsom1_ek/sama5d27_wlsom1_ek.c
@@ -68,6 +68,9 @@ int board_init(void)
 #ifdef CONFIG_MISC_INIT_R
 int misc_init_r(void)
 {
+#ifdef CONFIG_SPI_FLASH_SFDP_SUPPORT
+	at91_spi_nor_set_ethaddr();
+#endif
 	return 0;
 }
 #endif
-- 
2.9.5

[U-Boot] [PATCH 3/4] configs: sama5d27_wlsom1_ek: qspiflash: Enable SPI NOR ethaddr retrieval

[Tudor.Ambarus at microchip.com]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

CONFIG_SPI_FLASH_SFDP_SUPPORT enables the SFDP Vendor parser,
and for the Microchip case, the retrieval of the ethaddr from
the SPI NOR flash.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 configs/sama5d27_wlsom1_ek_qspiflash_defconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/configs/sama5d27_wlsom1_ek_qspiflash_defconfig b/configs/sama5d27_wlsom1_ek_qspiflash_defconfig
index 82568e286d81..e7a9cc302d3e 100644
--- a/configs/sama5d27_wlsom1_ek_qspiflash_defconfig
+++ b/configs/sama5d27_wlsom1_ek_qspiflash_defconfig
@@ -31,8 +31,8 @@ CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_DISPLAY_PRINT=y
 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set
 CONFIG_SPL_SPI_LOAD=y
-CONFIG_SPL_AT91_MCK_BYPASS=y
 CONFIG_SYS_SPI_U_BOOT_OFFS=0x40000
+CONFIG_SPL_AT91_MCK_BYPASS=y
 CONFIG_HUSH_PARSER=y
 CONFIG_CMD_BOOTZ=y
 # CONFIG_CMD_FLASH is not set
@@ -80,6 +80,7 @@ CONFIG_DM_SPI_FLASH=y
 CONFIG_SPI_FLASH=y
 CONFIG_SF_DEFAULT_BUS=2
 CONFIG_SF_DEFAULT_SPEED=50000000
+CONFIG_SPI_FLASH_SFDP_SUPPORT=y
 CONFIG_SPI_FLASH_ATMEL=y
 CONFIG_SPI_FLASH_MACRONIX=y
 CONFIG_SPI_FLASH_SPANSION=y
-- 
2.9.5

[U-Boot] [PATCH 4/4] configs: sama5d27_wlsom1_ek: mmc: Enable SPI NOR ethaddr retrieval

[Tudor.Ambarus at microchip.com]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Enable the SPI NOR SFDP support and the Microchip QSPI driver.
CONFIG_SPI_FLASH_SFDP_SUPPORT enables the SFDP Vendor parser,
and for the Microchip case, the retrieval of the ethaddr from
the SPI NOR flash.

While touching the SPI NOR logic, sync with the
sama5d27_wlsom1_ek_qspiflash_defconfig and enable
CONFIG_SPI_FLASH_SPANSION.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 configs/sama5d27_wlsom1_ek_mmc_defconfig | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/configs/sama5d27_wlsom1_ek_mmc_defconfig b/configs/sama5d27_wlsom1_ek_mmc_defconfig
index 50a8a8e83ccb..0b2b480e338a 100644
--- a/configs/sama5d27_wlsom1_ek_mmc_defconfig
+++ b/configs/sama5d27_wlsom1_ek_mmc_defconfig
@@ -17,6 +17,7 @@ CONFIG_DEBUG_UART_CLOCK=82000000
 CONFIG_SPL_FS_FAT=y
 CONFIG_SPL_LIBDISK_SUPPORT=y
 CONFIG_DEBUG_UART=y
+CONFIG_SPL_TEXT_BASE=0x200000
 CONFIG_ENV_VARS_UBOOT_CONFIG=y
 CONFIG_FIT=y
 CONFIG_SYS_EXTRA_OPTIONS="SAMA5D2"
@@ -26,7 +27,6 @@ CONFIG_USE_BOOTARGS=y
 CONFIG_MISC_INIT_R=y
 # CONFIG_DISPLAY_BOARDINFO is not set
 CONFIG_DISPLAY_BOARDINFO_LATE=y
-CONFIG_SPL_TEXT_BASE=0x200000
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_DISPLAY_PRINT=y
 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set
@@ -67,8 +67,12 @@ CONFIG_MMC_SDHCI=y
 CONFIG_MMC_SDHCI_ATMEL=y
 CONFIG_DM_SPI_FLASH=y
 CONFIG_SPI_FLASH=y
+CONFIG_SF_DEFAULT_BUS=2
+CONFIG_SF_DEFAULT_SPEED=50000000
+CONFIG_SPI_FLASH_SFDP_SUPPORT=y
 CONFIG_SPI_FLASH_ATMEL=y
 CONFIG_SPI_FLASH_MACRONIX=y
+CONFIG_SPI_FLASH_SPANSION=y
 CONFIG_SPI_FLASH_STMICRO=y
 CONFIG_SPI_FLASH_SST=y
 CONFIG_PHY_MICREL=y
@@ -83,6 +87,7 @@ CONFIG_DEBUG_UART_ANNOUNCE=y
 CONFIG_ATMEL_USART=y
 CONFIG_SPI=y
 CONFIG_DM_SPI=y
+CONFIG_ATMEL_QSPI=y
 CONFIG_TIMER=y
 CONFIG_SPL_TIMER=y
 CONFIG_ATMEL_PIT_TIMER=y
-- 
2.9.5

[U-Boot] [PATCH 1/4] mtd: spi: spi-nor-core: Add Microchip SFDP parser

[Eugen.Hristev at microchip.com]

On 01.10.2019 11:59, Tudor Ambarus - M18064 wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> JESD216 allow vendors to define their own SFDP tables.
> 
> Add Microchip SFDP parser. The vendor table is allocated using
> resource-managed kmalloc - the table will be freed on driver detach.
> It will be accessible by getting the UCLASS_SPI_FLASH's private data.
> 
> The Michrochip's SFDP table is particularly of interest because contains
> pre-programmed globally unique EUI-48 and EUI-64 identifiers.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>   drivers/mtd/spi/spi-nor-core.c | 35 +++++++++++++++++++++++++++++++++++
>   include/linux/mtd/spi-nor.h    |  2 ++
>   2 files changed, 37 insertions(+)
> 

Hello Jagan and Vignesh,

This patch touches the spi nor core, to take it through atmel tree would 
need your Review/Ack when you have the chance.
If you feel otherwise, reassign in patchwork and review through spi tree.

Thanks,
Eugen

> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 1acff745d1a2..bade7d8a9f79 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1417,6 +1417,7 @@ struct sfdp_parameter_header {
>   
>   #define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
>   #define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
> +#define SFDP_MICROCHIP_ID	0x01bf	/* Manufacturer specific Table */
>   
>   #define SFDP_SIGNATURE		0x50444653U
>   #define SFDP_JESD216_MAJOR	1
> @@ -1797,6 +1798,34 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>   }
>   
>   /**
> + * spi_nor_parse_microchip_sfdp() - parse the Microchip manufacturer specific
> + * SFDP table.
> + * @nor:		pointer to a 'struct spi_nor'.
> + * @param_header:	pointer to the SFDP parameter header.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int
> +spi_nor_parse_microchip_sfdp(struct spi_nor *nor,
> +			     const struct sfdp_parameter_header *param_header)
> +{
> +	size_t size;
> +	u32 addr;
> +	int ret;
> +
> +	size = param_header->length * sizeof(u32);
> +	addr = SFDP_PARAM_HEADER_PTP(param_header);
> +
> +	nor->manufacturer_sfdp = devm_kmalloc(nor->dev, size, GFP_KERNEL);
> +	if (!nor->manufacturer_sfdp)
> +		return -ENOMEM;
> +
> +	ret = spi_nor_read_sfdp(nor, addr, size, nor->manufacturer_sfdp);
> +
> +	return ret;
> +}
> +
> +/**
>    * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
>    * @nor:		pointer to a 'struct spi_nor'
>    * @params:		pointer to the 'struct spi_nor_flash_parameter' to be
> @@ -1892,6 +1921,12 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>   			dev_info(dev, "non-uniform erase sector maps are not supported yet.\n");
>   			break;
>   
> +		case SFDP_MICROCHIP_ID:
> +			err = spi_nor_parse_microchip_sfdp(nor, param_header);
> +			if (err)
> +				goto exit;
> +			break;
> +
>   		default:
>   			break;
>   		}
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 88e80af57941..836a50178925 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -251,6 +251,7 @@ struct flash_info;
>    * @lock:		the lock for the read/write/erase/lock/unlock operations
>    * @dev:		point to a spi device, or a spi nor controller device.
>    * @info:		spi-nor part JDEC MFR id and other info
> + * @manufacturer_sfdp:	manufacturer specific SFDP table
>    * @page_size:		the page size of the SPI NOR
>    * @addr_width:		number of address bytes
>    * @erase_opcode:	the opcode for erasing a sector
> @@ -289,6 +290,7 @@ struct spi_nor {
>   	struct udevice		*dev;
>   	struct spi_slave	*spi;
>   	const struct flash_info	*info;
> +	u8			*manufacturer_sfdp;
>   	u32			page_size;
>   	u8			addr_width;
>   	u8			erase_opcode;
> 

[U-Boot] [PATCH 1/4] mtd: spi: spi-nor-core: Add Microchip SFDP parser

[Vignesh Raghavendra]

Hi Tudor,

On 01/10/19 2:29 PM, Tudor.Ambarus at microchip.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> JESD216 allow vendors to define their own SFDP tables.
> 
> Add Microchip SFDP parser. The vendor table is allocated using
> resource-managed kmalloc - the table will be freed on driver detach.
> It will be accessible by getting the UCLASS_SPI_FLASH's private data.
> 
> The Michrochip's SFDP table is particularly of interest because contains
> pre-programmed globally unique EUI-48 and EUI-64 identifiers.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi/spi-nor-core.c | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h    |  2 ++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 1acff745d1a2..bade7d8a9f79 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1417,6 +1417,7 @@ struct sfdp_parameter_header {
>  
>  #define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
>  #define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
> +#define SFDP_MICROCHIP_ID	0x01bf	/* Manufacturer specific Table */
>  

Is this ID unique enough such that no other vendor will use the same? I
recall that MSB byte should be assigned to Vendor and 0x01 does not seem
to be Microchip specific? Or did I miss something?

Regards
Vignesh

>  #define SFDP_SIGNATURE		0x50444653U
>  #define SFDP_JESD216_MAJOR	1
> @@ -1797,6 +1798,34 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  }
>  
>  /**
> + * spi_nor_parse_microchip_sfdp() - parse the Microchip manufacturer specific
> + * SFDP table.
> + * @nor:		pointer to a 'struct spi_nor'.
> + * @param_header:	pointer to the SFDP parameter header.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int
> +spi_nor_parse_microchip_sfdp(struct spi_nor *nor,
> +			     const struct sfdp_parameter_header *param_header)
> +{
> +	size_t size;
> +	u32 addr;
> +	int ret;
> +
> +	size = param_header->length * sizeof(u32);
> +	addr = SFDP_PARAM_HEADER_PTP(param_header);
> +
> +	nor->manufacturer_sfdp = devm_kmalloc(nor->dev, size, GFP_KERNEL);
> +	if (!nor->manufacturer_sfdp)
> +		return -ENOMEM;
> +
> +	ret = spi_nor_read_sfdp(nor, addr, size, nor->manufacturer_sfdp);
> +
> +	return ret;
> +}
> +
> +/**
>   * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
>   * @nor:		pointer to a 'struct spi_nor'
>   * @params:		pointer to the 'struct spi_nor_flash_parameter' to be
> @@ -1892,6 +1921,12 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  			dev_info(dev, "non-uniform erase sector maps are not supported yet.\n");
>  			break;
>  
> +		case SFDP_MICROCHIP_ID:
> +			err = spi_nor_parse_microchip_sfdp(nor, param_header);
> +			if (err)
> +				goto exit;
> +			break;
> +
>  		default:
>  			break;
>  		}
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 88e80af57941..836a50178925 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -251,6 +251,7 @@ struct flash_info;
>   * @lock:		the lock for the read/write/erase/lock/unlock operations
>   * @dev:		point to a spi device, or a spi nor controller device.
>   * @info:		spi-nor part JDEC MFR id and other info
> + * @manufacturer_sfdp:	manufacturer specific SFDP table
>   * @page_size:		the page size of the SPI NOR
>   * @addr_width:		number of address bytes
>   * @erase_opcode:	the opcode for erasing a sector
> @@ -289,6 +290,7 @@ struct spi_nor {
>  	struct udevice		*dev;
>  	struct spi_slave	*spi;
>  	const struct flash_info	*info;
> +	u8			*manufacturer_sfdp;
>  	u32			page_size;
>  	u8			addr_width;
>  	u8			erase_opcode;
> 

-- 
Regards
Vignesh

[U-Boot] [PATCH 1/4] mtd: spi: spi-nor-core: Add Microchip SFDP parser

[Tudor.Ambarus at microchip.com]

Hi, Vignesh,

On 10/09/2019 03:04 PM, Vignesh Raghavendra wrote:
> External E-Mail
> 
> 
> Hi Tudor,
> 
> On 01/10/19 2:29 PM, Tudor.Ambarus at microchip.com wrote:
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> JESD216 allow vendors to define their own SFDP tables.
>>
>> Add Microchip SFDP parser. The vendor table is allocated using
>> resource-managed kmalloc - the table will be freed on driver detach.
>> It will be accessible by getting the UCLASS_SPI_FLASH's private data.
>>
>> The Michrochip's SFDP table is particularly of interest because contains
>> pre-programmed globally unique EUI-48 and EUI-64 identifiers.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi/spi-nor-core.c | 35 +++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/spi-nor.h    |  2 ++
>>  2 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 1acff745d1a2..bade7d8a9f79 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -1417,6 +1417,7 @@ struct sfdp_parameter_header {
>>  
>>  #define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
>>  #define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
>> +#define SFDP_MICROCHIP_ID	0x01bf	/* Manufacturer specific Table */
>>  
> Is this ID unique enough such that no other vendor will use the same? I
> recall that MSB byte should be assigned to Vendor and 0x01 does not seem
> to be Microchip specific? Or did I miss something?
> 

It is unique.

Quoting form JESD216 rev D, section "6.3.3 Definition of Parameter ID Field":
"The original JESD216 specification used only a one byte ID field to identify
the parameter table owner.
 JESD216 revision A expanded the ID field to two bytes, MSB and LSB, because a
single byte is
 insufficient to uniquely identify all manufacturers ( vendors). The original
single byte parameter ID is
 now referred to as the parameter ID LSB."

In the Microchip's case, 01h is the Parameter ID MSB (respects the 01h-7fh
interval) and bfh is the Parameter ID LSB (has odd parity, which is correct).
Now I'm looking in the JEP106AZ standard (Standard Manufacturer’s
Identification Code): 01h MSB indicates the bank number (one), and bfh LSB
indicates the Manufacturer Identification code (which is bfh for SST).

Cheers,
ta

[U-Boot] [PATCH 1/4] mtd: spi: spi-nor-core: Add Microchip SFDP parser

[Tudor.Ambarus at microchip.com]

Hi, Vignesh,

On 10/01/2019 11:59 AM, Tudor Ambarus - M18064 wrote:
> @@ -1892,6 +1921,12 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  			dev_info(dev, "non-uniform erase sector maps are not supported yet.\n");
>  			break;
>  
> +		case SFDP_MICROCHIP_ID:
> +			err = spi_nor_parse_microchip_sfdp(nor, param_header);
> +			if (err)
> +				goto exit;

This can be improved though. At this point the BFPT parser succeeded, and it
would be a pity if we ignore the BFPT data when the vendor specific parser
fails. The goto exit should be replaced by a break and a warning message to
indicate when an optional parameter table fails.

Please let me know if there are other comments, I'll send v2 if all the rest are ok.

> +			break;
> +
>  		default:
>  			break;
>  		}

[U-Boot] [PATCH 1/4] mtd: spi: spi-nor-core: Add Microchip SFDP parser

[Vignesh Raghavendra]

On 09-Oct-19 9:20 PM, Tudor.Ambarus at microchip.com wrote:
> Hi, Vignesh,
> 
> On 10/09/2019 03:04 PM, Vignesh Raghavendra wrote:
>> External E-Mail
>>
>>
>> Hi Tudor,
>>
>> On 01/10/19 2:29 PM, Tudor.Ambarus at microchip.com wrote:
>>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>
>>> JESD216 allow vendors to define their own SFDP tables.
>>>
>>> Add Microchip SFDP parser. The vendor table is allocated using
>>> resource-managed kmalloc - the table will be freed on driver detach.
>>> It will be accessible by getting the UCLASS_SPI_FLASH's private data.
>>>
>>> The Michrochip's SFDP table is particularly of interest because contains
>>> pre-programmed globally unique EUI-48 and EUI-64 identifiers.
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>> ---
>>>  drivers/mtd/spi/spi-nor-core.c | 35 +++++++++++++++++++++++++++++++++++
>>>  include/linux/mtd/spi-nor.h    |  2 ++
>>>  2 files changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>>> index 1acff745d1a2..bade7d8a9f79 100644
>>> --- a/drivers/mtd/spi/spi-nor-core.c
>>> +++ b/drivers/mtd/spi/spi-nor-core.c
>>> @@ -1417,6 +1417,7 @@ struct sfdp_parameter_header {
>>>  
>>>  #define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
>>>  #define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
>>> +#define SFDP_MICROCHIP_ID	0x01bf	/* Manufacturer specific Table */
>>>  
>> Is this ID unique enough such that no other vendor will use the same? I
>> recall that MSB byte should be assigned to Vendor and 0x01 does not seem
>> to be Microchip specific? Or did I miss something?
>>
> 
> It is unique.
> 
> Quoting form JESD216 rev D, section "6.3.3 Definition of Parameter ID Field":
> "The original JESD216 specification used only a one byte ID field to identify
> the parameter table owner.
>  JESD216 revision A expanded the ID field to two bytes, MSB and LSB, because a
> single byte is
>  insufficient to uniquely identify all manufacturers ( vendors). The original
> single byte parameter ID is
>  now referred to as the parameter ID LSB."
> 
> In the Microchip's case, 01h is the Parameter ID MSB (respects the 01h-7fh
> interval) and bfh is the Parameter ID LSB (has odd parity, which is correct).
> Now I'm looking in the JEP106AZ standard (Standard Manufacturer’s
> Identification Code): 01h MSB indicates the bank number (one), and bfh LSB
> indicates the Manufacturer Identification code (which is bfh for SST).
> 

Ah, thanks for the explanation! I was confused with bank number
initially. Was also confused by the fact that there is an entry for
Microchip Technology in JEP106AZ that reads 29. May be rename the macro
to indicate SFDP_SST_ID?

Regards
Vignesh

[U-Boot] [PATCH 1/4] mtd: spi: spi-nor-core: Add Microchip SFDP parser

[Tudor.Ambarus at microchip.com]

On 10/09/2019 07:25 PM, Vignesh Raghavendra wrote:
> External E-Mail
> 
> 
> 
> On 09-Oct-19 9:20 PM, Tudor.Ambarus at microchip.com wrote:
>> Hi, Vignesh,
>>
>> On 10/09/2019 03:04 PM, Vignesh Raghavendra wrote:
>>> External E-Mail
>>>
>>>
>>> Hi Tudor,
>>>
>>> On 01/10/19 2:29 PM, Tudor.Ambarus at microchip.com wrote:
>>>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>>
>>>> JESD216 allow vendors to define their own SFDP tables.
>>>>
>>>> Add Microchip SFDP parser. The vendor table is allocated using
>>>> resource-managed kmalloc - the table will be freed on driver detach.
>>>> It will be accessible by getting the UCLASS_SPI_FLASH's private data.
>>>>
>>>> The Michrochip's SFDP table is particularly of interest because contains
>>>> pre-programmed globally unique EUI-48 and EUI-64 identifiers.
>>>>
>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>> ---
>>>>  drivers/mtd/spi/spi-nor-core.c | 35 +++++++++++++++++++++++++++++++++++
>>>>  include/linux/mtd/spi-nor.h    |  2 ++
>>>>  2 files changed, 37 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>>>> index 1acff745d1a2..bade7d8a9f79 100644
>>>> --- a/drivers/mtd/spi/spi-nor-core.c
>>>> +++ b/drivers/mtd/spi/spi-nor-core.c
>>>> @@ -1417,6 +1417,7 @@ struct sfdp_parameter_header {
>>>>  
>>>>  #define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
>>>>  #define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
>>>> +#define SFDP_MICROCHIP_ID	0x01bf	/* Manufacturer specific Table */
>>>>  
>>> Is this ID unique enough such that no other vendor will use the same? I
>>> recall that MSB byte should be assigned to Vendor and 0x01 does not seem
>>> to be Microchip specific? Or did I miss something?
>>>
>>
>> It is unique.
>>
>> Quoting form JESD216 rev D, section "6.3.3 Definition of Parameter ID Field":
>> "The original JESD216 specification used only a one byte ID field to identify
>> the parameter table owner.
>>  JESD216 revision A expanded the ID field to two bytes, MSB and LSB, because a
>> single byte is
>>  insufficient to uniquely identify all manufacturers ( vendors). The original
>> single byte parameter ID is
>>  now referred to as the parameter ID LSB."
>>
>> In the Microchip's case, 01h is the Parameter ID MSB (respects the 01h-7fh
>> interval) and bfh is the Parameter ID LSB (has odd parity, which is correct).
>> Now I'm looking in the JEP106AZ standard (Standard Manufacturer’s
>> Identification Code): 01h MSB indicates the bank number (one), and bfh LSB
>> indicates the Manufacturer Identification code (which is bfh for SST).
>>
> 
> Ah, thanks for the explanation! I was confused with bank number
> initially. Was also confused by the fact that there is an entry for
> Microchip Technology in JEP106AZ that reads 29. May be rename the macro
> to indicate SFDP_SST_ID?

Ok, will do so. Thanks!