Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1212055 > unrolled thread

[PATCH linux-next v4 0/5] add driver for Atmel QSPI controller

Back to article view | Back to linux.kernel

Contents

  [PATCH linux-next v4 0/5] add driver for Atmel QSPI controller Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2015-08-24 12:20 +0200
    [PATCH linux-next v4 1/5] mtd: spi-nor: notify (Q)SPI controller about protocol change Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2015-08-24 12:20 +0200
      Re: [PATCH linux-next v4 1/5] mtd: spi-nor: notify (Q)SPI controller about protocol change Marek Vasut <marex@denx.de> - 2015-08-24 12:30 +0200
    [PATCH linux-next v4 4/5] Documentation: atmel-quadspi: add binding file for Atmel QSPI driver Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2015-08-24 12:20 +0200
      Re: [PATCH linux-next v4 4/5] Documentation: atmel-quadspi: add binding file for Atmel QSPI driver Marek Vasut <marex@denx.de> - 2015-08-24 12:30 +0200
    [PATCH linux-next v4 3/5] mtd: spi-nor: allow to tune the number of dummy cycles Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2015-08-24 12:20 +0200
      Re: [PATCH linux-next v4 3/5] mtd: spi-nor: allow to tune the number of dummy cycles Marek Vasut <marex@denx.de> - 2015-08-24 12:50 +0200
        Re: [PATCH linux-next v4 3/5] mtd: spi-nor: allow to tune the number of dummy cycles Marek Vasut <marex@denx.de> - 2015-08-24 18:50 +0200
        Re: [PATCH linux-next v4 3/5] mtd: spi-nor: allow to tune the number  of dummy cycles Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2015-08-24 18:50 +0200

#1212055 — [PATCH linux-next v4 0/5] add driver for Atmel QSPI controller

ChangeLog

v4:
- add "OF && HAS_DMA" dependency in Kconfig for Atmel Quad SPI driver.
- return -ENOMEM instead of the return code of dma_mapping_error() as this
  function returns a boolean on ARM achitecture.
- add "Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>" for Atmel Quad
  SPI driver and its DT binding documentation.

v3:
- reword the comment which explains that spi_nor_set_protocol() is used by
  the spi-nor framework to notify lower layers, especially the (Q)SPI
  controller about a protocol change.
- change the definitions of register/bitfield macros in the Atmel QSPI
  controller driver: get rid of concatenation operator and use BIT and
  GENMASK macros when possible.
- use #define[SPACE] instead of #define[TAB]

v2:
- remove the patches to set the "latency code" of Spansion QSPI memories
  (support of Spansion memories may be submitted in later series).
- rename "qspi" node into "spi" in the DT example to fit ePAPR standard.
- remove the useless "qspi0" label from the DT node example.
- remove the leading 0 from the size of the second memory region to make
  it consistent with the size of the first memory region.
- indent the DT bindings documentation to make it more readable.
- remove the useless ".bus	= &platform_bus_type," line from the
  platform driver definition.

v1:

This series of patches add support for the new Atmel QSPI controller
embedded inside sama5d2x SoCs.

These patches were first developped for linux-3.18-at91 and tested on a
sama5d27 Xplained ultra board, which embeds a Micron n25q128a13 QSPI NOR
flash memory. Then the series was adapted for mainline.

Cyrille Pitchen (5):
  mtd: spi-nor: notify (Q)SPI controller about protocol change
  Documentation: mtd: add a DT property to set the number of dummy
    cycles
  mtd: spi-nor: allow to tune the number of dummy cycles
  Documentation: atmel-quadspi: add binding file for Atmel QSPI driver
  mtd: atmel-quadspi: add driver for Atmel QSPI controller

 .../devicetree/bindings/mtd/atmel-quadspi.txt      |  29 +
 .../devicetree/bindings/mtd/jedec,spi-nor.txt      |   6 +
 drivers/mtd/spi-nor/Kconfig                        |   7 +
 drivers/mtd/spi-nor/Makefile                       |   1 +
 drivers/mtd/spi-nor/atmel-quadspi.c                | 876 +++++++++++++++++++++
 drivers/mtd/spi-nor/spi-nor.c                      | 118 ++-
 include/linux/mtd/spi-nor.h                        |  15 +
 7 files changed, 1033 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
 create mode 100644 drivers/mtd/spi-nor/atmel-quadspi.c

-- 
1.8.2.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1212056 — [PATCH linux-next v4 1/5] mtd: spi-nor: notify (Q)SPI controller about protocol change

Once the Quad SPI mode has been enabled on a Micron flash memory, this
device expects ALL the following commands to use the SPI 4-4-4 protocol.
The (Q)SPI controller needs to be notified about the protocol change so it
can adapt and keep on dialoging with the Micron memory.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 21 +++++++++++++++++++++
 include/linux/mtd/spi-nor.h   | 13 +++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 47df4b5eae2f..e2a6029dc056 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -163,6 +163,22 @@ static inline int write_disable(struct spi_nor *nor)
 	return nor->write_reg(nor, SPINOR_OP_WRDI, NULL, 0, 0);
 }
 
+/*
+ * Let the spi-nor framework notify lower layers, especially the driver of the
+ * (Q)SPI controller, about the new protocol to be used. Indeed, once the
+ * spi-nor framework has sent manufacturer specific commands to a memory to
+ * enable its Quad SPI mode, it should immediately after tell the QSPI
+ * controller to use the very same Quad SPI protocol as expected by the memory.
+ */
+static inline int spi_nor_set_protocol(struct spi_nor *nor,
+				       enum spi_protocol proto)
+{
+	if (nor->set_protocol)
+		return nor->set_protocol(nor, proto);
+
+	return 0;
+}
+
 static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
 {
 	return mtd->priv;
@@ -944,6 +960,11 @@ static int micron_quad_enable(struct spi_nor *nor)
 		return ret;
 	}
 
+	/* switch protocol to Quad CMD 4-4-4 */
+	ret = spi_nor_set_protocol(nor, SPI_PROTO_4_4_4);
+	if (ret)
+		return ret;
+
 	ret = spi_nor_wait_till_ready(nor);
 	if (ret)
 		return ret;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index e5409524bb0a..1bf6f11310ef 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -87,6 +87,16 @@ enum read_mode {
 	SPI_NOR_QUAD,
 };
 
+enum spi_protocol {
+	SPI_PROTO_1_1_1,	/* SPI */
+	SPI_PROTO_1_1_2,	/* Dual Output */
+	SPI_PROTO_1_1_4,	/* Quad Output */
+	SPI_PROTO_1_2_2,	/* Dual IO */
+	SPI_PROTO_1_4_4,	/* Quad IO */
+	SPI_PROTO_2_2_2,	/* Dual Command */
+	SPI_PROTO_4_4_4,	/* Quad Command */
+};
+
 /**
  * struct spi_nor_xfer_cfg - Structure for defining a Serial Flash transfer
  * @wren:		command for "Write Enable", or 0x00 for not required
@@ -149,6 +159,7 @@ enum spi_nor_option_flags {
  *			read/write/erase/lock/unlock operations
  * @read_xfer:		[OPTIONAL] the read fundamental primitive
  * @write_xfer:		[OPTIONAL] the writefundamental primitive
+ * @set_protocol:	[OPTIONAL] notify about protocol change
  * @read_reg:		[DRIVER-SPECIFIC] read out the register
  * @write_reg:		[DRIVER-SPECIFIC] write data to the register
  * @read:		[DRIVER-SPECIFIC] read data from the SPI NOR
@@ -185,6 +196,8 @@ struct spi_nor {
 	int (*write_reg)(struct spi_nor *nor, u8 opcode, u8 *buf, int len,
 			int write_enable);
 
+	int (*set_protocol)(struct spi_nor *nor, enum spi_protocol proto);
+
 	int (*read)(struct spi_nor *nor, loff_t from,
 			size_t len, size_t *retlen, u_char *read_buf);
 	void (*write)(struct spi_nor *nor, loff_t to,
-- 
1.8.2.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212066 — Re: [PATCH linux-next v4 1/5] mtd: spi-nor: notify (Q)SPI controller about protocol change

On Monday, August 24, 2015 at 12:13:56 PM, Cyrille Pitchen wrote:
> Once the Quad SPI mode has been enabled on a Micron flash memory, this
> device expects ALL the following commands to use the SPI 4-4-4 protocol.
> The (Q)SPI controller needs to be notified about the protocol change so it
> can adapt and keep on dialoging with the Micron memory.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>

Awesome,

Acked-by: Marek Vasut <marex@denx.de>

If this could be applied separately (since I want to use the same functionality
for the Cadence QSPI driver), I'd be really happy too :)

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212063 — [PATCH linux-next v4 4/5] Documentation: atmel-quadspi: add binding file for Atmel QSPI driver

This patch documents the DT bindings for the driver of the Atmel QSPI
controller embedded inside sama5d2x SoCs.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 .../devicetree/bindings/mtd/atmel-quadspi.txt      | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/atmel-quadspi.txt

diff --git a/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt b/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
new file mode 100644
index 000000000000..0b8d545bb198
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
@@ -0,0 +1,29 @@
+* Atmel Quad Serial Peripheral Interface (QSPI)
+
+Required properties:
+- compatible:     should be "atmel,sama5d2-qspi"
+- reg:            the first contains the register location and length,
+                  the second contains the memory mapping address and length
+- interrupts:     should contain the interrupt for the device
+- clocks:         the phandle of the clock needed by the QSPI controller
+- #address-cells: should be <1>
+- #size-cells:    should be <0>
+
+Example:
+
+spi@f0020000 {
+	compatible = "atmel,sama5d2-qspi";
+	reg = <0xf0020000 0x100>,
+	      <0xd0000000 0x8000000>;
+	interrupts = <52 IRQ_TYPE_LEVEL_HIGH 7>;
+	clocks = <&spi0_clk>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_spi0_default>;
+	status = "okay";
+
+	m25p80@0 {
+		...
+	};
+};
-- 
1.8.2.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212067 — Re: [PATCH linux-next v4 4/5] Documentation: atmel-quadspi: add binding file for Atmel QSPI driver

On Monday, August 24, 2015 at 12:13:59 PM, Cyrille Pitchen wrote:
> This patch documents the DT bindings for the driver of the Atmel QSPI
> controller embedded inside sama5d2x SoCs.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

If my ack has any value in here, feel free to add it, the bindings look
pretty standard anyway:

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212064 — [PATCH linux-next v4 3/5] mtd: spi-nor: allow to tune the number of dummy cycles

The number of dummy cycles used during Fast Read commands can be reduced
to improve transfer performances. Each manufacturer has a dedicated set of
registers to provide the memory with the exact number of dummy cycles it
should expect. Both the memory and the (Q)SPI controller must agree on
this number of dummy cycles.

The number of dummy cycles can be found into the memory datasheet and
mostly depends on the SPI clock frequency, the Fast Read op code and the
Single/Dual Data Rate mode.

Probing JEDEC Serial Flash Discoverable Parameters (SFDP) tables would
only provide the driver with a high enough number of dummy cycles for each
Fast Read command to be used for all clock frequencies: this solution
would not be optimized.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 97 ++++++++++++++++++++++++++++++++++---------
 include/linux/mtd/spi-nor.h   |  2 +
 2 files changed, 80 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index e2a6029dc056..869e098a6841 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -119,24 +119,6 @@ static int read_cr(struct spi_nor *nor)
 }
 
 /*
- * Dummy Cycle calculation for different type of read.
- * It can be used to support more commands with
- * different dummy cycle requirements.
- */
-static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
-{
-	switch (nor->flash_read) {
-	case SPI_NOR_FAST:
-	case SPI_NOR_DUAL:
-	case SPI_NOR_QUAD:
-		return 8;
-	case SPI_NOR_NORMAL:
-		return 0;
-	}
-	return 0;
-}
-
-/*
  * Write status register 1 byte
  * Returns negative if error occurred.
  */
@@ -1012,6 +994,81 @@ static int set_quad_mode(struct spi_nor *nor, struct flash_info *info)
 	}
 }
 
+static int micron_set_dummy_cycles(struct spi_nor *nor)
+{
+	int ret;
+	u8 val, mask;
+
+	/* read the Volatile Configuration Register (VCR) */
+	ret = nor->read_reg(nor, SPINOR_OP_RD_VCR, &val, 1);
+	if (ret < 0) {
+		dev_err(nor->dev, "error %d reading VCR\n", ret);
+		return ret;
+	}
+
+	write_enable(nor);
+
+	/* update the number of dummy into the VCR */
+	mask = GENMASK(7, 4);
+	val &= ~mask;
+	val |= (nor->read_dummy << 4) & mask;
+	ret = nor->write_reg(nor, SPINOR_OP_WR_VCR, &val, 1, 0);
+	if (ret < 0) {
+		dev_err(nor->dev, "error while writing VCR register\n");
+		return ret;
+	}
+
+	ret = spi_nor_wait_till_ready(nor);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * Dummy Cycle calculation for different type of read.
+ * It can be used to support more commands with
+ * different dummy cycle requirements.
+ */
+static int spi_nor_read_dummy_cycles(struct spi_nor *nor,
+				     const struct flash_info *info)
+{
+	struct device_node *np = nor->dev->of_node;
+	u32 num_dummy_cycles;
+
+	if (np && !of_property_read_u32(np, "m25p,num-dummy-cycles",
+					&num_dummy_cycles)) {
+		nor->read_dummy = num_dummy_cycles;
+
+		/*
+		 * This switch block might be moved after the if...then...else
+		 * statement but it was not tested with all Spansion or Micron
+		 * memories.
+		 * Now the "m25p,num-dummy-cycles" property needs to be
+		 * explicitly set in the device tree so the switch statement is
+		 * executed. This should avoid unwanted side effects and keep
+		 * backward compatibility.
+		 */
+		switch (JEDEC_MFR(info)) {
+		case CFI_MFR_ST:
+			return micron_set_dummy_cycles(nor);
+		default:
+			break;
+		}
+	} else {
+		switch (nor->flash_read) {
+		case SPI_NOR_FAST:
+		case SPI_NOR_DUAL:
+		case SPI_NOR_QUAD:
+			nor->read_dummy = 8;
+		case SPI_NOR_NORMAL:
+			nor->read_dummy = 0;
+		}
+	}
+
+	return 0;
+}
+
 static int spi_nor_check(struct spi_nor *nor)
 {
 	if (!nor->dev || !nor->read || !nor->write ||
@@ -1216,7 +1273,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 		nor->addr_width = 3;
 	}
 
-	nor->read_dummy = spi_nor_read_dummy_cycles(nor);
+	ret = spi_nor_read_dummy_cycles(nor, info);
+	if (ret)
+		return ret;
 
 	dev_info(dev, "%s (%lld Kbytes)\n", id->name,
 			(long long)mtd->size >> 10);
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 1bf6f11310ef..e03a4c4053d3 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -59,6 +59,8 @@
 /* Used for Micron flashes only. */
 #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
 #define SPINOR_OP_WD_EVCR      0x61    /* Write EVCR register */
+#define SPINOR_OP_RD_VCR	0x85	/* Read VCR register */
+#define SPINOR_OP_WR_VCR	0x81	/* Write VCR register */
 
 /* Status Register bits. */
 #define SR_WIP			1	/* Write in progress */
-- 
1.8.2.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212071 — Re: [PATCH linux-next v4 3/5] mtd: spi-nor: allow to tune the number of dummy cycles

On Monday, August 24, 2015 at 12:13:58 PM, Cyrille Pitchen wrote:
> The number of dummy cycles used during Fast Read commands can be reduced
> to improve transfer performances. Each manufacturer has a dedicated set of
> registers to provide the memory with the exact number of dummy cycles it
> should expect. Both the memory and the (Q)SPI controller must agree on
> this number of dummy cycles.
> 
> The number of dummy cycles can be found into the memory datasheet and
> mostly depends on the SPI clock frequency, the Fast Read op code and the
> Single/Dual Data Rate mode.
> 
> Probing JEDEC Serial Flash Discoverable Parameters (SFDP) tables would
> only provide the driver with a high enough number of dummy cycles for each
> Fast Read command to be used for all clock frequencies: this solution
> would not be optimized.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>

Hi!

>  drivers/mtd/spi-nor/spi-nor.c | 97
> ++++++++++++++++++++++++++++++++++--------- include/linux/mtd/spi-nor.h  
> |  2 +
>  2 files changed, 80 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index e2a6029dc056..869e098a6841 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -119,24 +119,6 @@ static int read_cr(struct spi_nor *nor)
>  }
> 
>  /*
> - * Dummy Cycle calculation for different type of read.
> - * It can be used to support more commands with
> - * different dummy cycle requirements.
> - */
> -static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
> -{
> -	switch (nor->flash_read) {
> -	case SPI_NOR_FAST:
> -	case SPI_NOR_DUAL:
> -	case SPI_NOR_QUAD:
> -		return 8;
> -	case SPI_NOR_NORMAL:
> -		return 0;
> -	}
> -	return 0;
> -}

You can probably just soup up this function so that it sets the
nor->read_dummy, no ?

> -/*
>   * Write status register 1 byte
>   * Returns negative if error occurred.
>   */
> @@ -1012,6 +994,81 @@ static int set_quad_mode(struct spi_nor *nor, struct
> flash_info *info) }
>  }
> 
> +static int micron_set_dummy_cycles(struct spi_nor *nor)
> +{
> +	int ret;
> +	u8 val, mask;
> +
> +	/* read the Volatile Configuration Register (VCR) */

NIT: If this is a sentence, start it with capital letter and end it with 
fullstop :)

> +	ret = nor->read_reg(nor, SPINOR_OP_RD_VCR, &val, 1);
> +	if (ret < 0) {
> +		dev_err(nor->dev, "error %d reading VCR\n", ret);
> +		return ret;
> +	}
> +
> +	write_enable(nor);
> +
> +	/* update the number of dummy into the VCR */

DTTO

> +	mask = GENMASK(7, 4);
> +	val &= ~mask;
> +	val |= (nor->read_dummy << 4) & mask;
> +	ret = nor->write_reg(nor, SPINOR_OP_WR_VCR, &val, 1, 0);
> +	if (ret < 0) {
> +		dev_err(nor->dev, "error while writing VCR register\n");
> +		return ret;
> +	}
> +
> +	ret = spi_nor_wait_till_ready(nor);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/*
> + * Dummy Cycle calculation for different type of read.
> + * It can be used to support more commands with
> + * different dummy cycle requirements.
> + */
> +static int spi_nor_read_dummy_cycles(struct spi_nor *nor,
> +				     const struct flash_info *info)
> +{
> +	struct device_node *np = nor->dev->of_node;
> +	u32 num_dummy_cycles;
> +
> +	if (np && !of_property_read_u32(np, "m25p,num-dummy-cycles",
> +					&num_dummy_cycles)) {
> +		nor->read_dummy = num_dummy_cycles;
> +
> +		/*
> +		 * This switch block might be moved after the if...then...else
> +		 * statement but it was not tested with all Spansion or Micron
> +		 * memories.
> +		 * Now the "m25p,num-dummy-cycles" property needs to be
> +		 * explicitly set in the device tree so the switch statement is
> +		 * executed. This should avoid unwanted side effects and keep
> +		 * backward compatibility.
> +		 */
> +		switch (JEDEC_MFR(info)) {
> +		case CFI_MFR_ST:
> +			return micron_set_dummy_cycles(nor);
> +		default:

If you do have m25p,num-dummy-cycles set for non-micron flash, you have a 
problem here I believe.

> +			break;
> +		}
> +	} else {

The solution would be to drop this else {} bit here, so that if you fail in
the DT-based configuration, you fall back to this old behavior. What do you 
think please ? :)

> +		switch (nor->flash_read) {
> +		case SPI_NOR_FAST:
> +		case SPI_NOR_DUAL:
> +		case SPI_NOR_QUAD:
> +			nor->read_dummy = 8;
> +		case SPI_NOR_NORMAL:
> +			nor->read_dummy = 0;
> +		}
> +	}
> +
> +	return 0;
> +}

[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212341 — Re: [PATCH linux-next v4 3/5] mtd: spi-nor: allow to tune the number of dummy cycles

On Monday, August 24, 2015 at 06:42:46 PM, Cyrille Pitchen wrote:
> Hi Marek,

Hi!

[...]

> >> - * Dummy Cycle calculation for different type of read.
> >> - * It can be used to support more commands with
> >> - * different dummy cycle requirements.
> >> - */
> >> -static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
> >> -{
> >> -	switch (nor->flash_read) {
> >> -	case SPI_NOR_FAST:
> >> -	case SPI_NOR_DUAL:
> >> -	case SPI_NOR_QUAD:
> >> -		return 8;
> >> -	case SPI_NOR_NORMAL:
> >> -		return 0;
> >> -	}
> >> -	return 0;
> >> -}
> > 
> > You can probably just soup up this function so that it sets the
> > nor->read_dummy, no ?
> 
> Actually, this is what the patch does: spi_nor_read_dummy_cycles() was
> reused and enhanced few lines below where you've pointed out the
> "switch (nor->flash_read)" block should be move after the else block.

You know what? I'll go get some sleep, coffee doesn't cut it anymore :)

> I think when I wrote the code I've chosen to move the definition of this
> function instead of adding forward declarations of functions such as
> read_cr() or write_sr_cr(), which are now called by
> micron_set_dummy_cycles().

Yep, that's all right, sorry for the confusion.

> >> -/*
> >> 
> >>   * Write status register 1 byte
> >>   * Returns negative if error occurred.
> >>   */
> >> 
> >> @@ -1012,6 +994,81 @@ static int set_quad_mode(struct spi_nor *nor,
> >> struct flash_info *info) }
> >> 
> >>  }

[...]

> >> +/*
> >> + * Dummy Cycle calculation for different type of read.
> >> + * It can be used to support more commands with
> >> + * different dummy cycle requirements.
> >> + */
> >> +static int spi_nor_read_dummy_cycles(struct spi_nor *nor,
> >> +				     const struct flash_info *info)
> >> +{
> >> +	struct device_node *np = nor->dev->of_node;
> >> +	u32 num_dummy_cycles;
> >> +
> >> +	if (np && !of_property_read_u32(np, "m25p,num-dummy-cycles",
> >> +					&num_dummy_cycles)) {
> >> +		nor->read_dummy = num_dummy_cycles;
> >> +
> >> +		/*
> >> +		 * This switch block might be moved after the if...then...else
> >> +		 * statement but it was not tested with all Spansion or Micron
> >> +		 * memories.
> >> +		 * Now the "m25p,num-dummy-cycles" property needs to be
> >> +		 * explicitly set in the device tree so the switch statement is
> >> +		 * executed. This should avoid unwanted side effects and keep
> >> +		 * backward compatibility.
> >> +		 */
> >> +		switch (JEDEC_MFR(info)) {
> >> +		case CFI_MFR_ST:
> >> +			return micron_set_dummy_cycles(nor);
> > 
> >> +		default:
> > If you do have m25p,num-dummy-cycles set for non-micron flash, you have a
> > problem here I believe.
> > 
> >> +			break;
> >> +		}
> >> +	} else {
> > 
> > The solution would be to drop this else {} bit here, so that if you fail
> > in the DT-based configuration, you fall back to this old behavior. What
> > do you think please ? :)
> 
> Good idea!
> I also add a trace for the default case of "switch (JEDEC_MFR(info))":
> 
> dev_warn(dev, "can't set the number of dummy cycles\n");

Maybe change this to "setting the number of dummy cycles not supported by chip, 
ignoring" or something, to be explicit about the fallback and that this is not
supported by the chip. But this is just an idea, feel free to ignore it.

> So the user is notified that the driver could not use the value of
> "m25p,num-dummy-cycles" from the DT before falling back to the legacy
> code.

Yup.

> >> +		switch (nor->flash_read) {
> >> +		case SPI_NOR_FAST:
> >> +		case SPI_NOR_DUAL:
> >> +		case SPI_NOR_QUAD:
> >> +			nor->read_dummy = 8;
> >> +		case SPI_NOR_NORMAL:
> >> +			nor->read_dummy = 0;
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> > 
> > [...]
> 
> thanks for the review!

Im glad it helped ;-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212344 — Re: [PATCH linux-next v4 3/5] mtd: spi-nor: allow to tune the number of dummy cycles

Hi Marek,

Le 24/08/2015 12:48, Marek Vasut a écrit :
> On Monday, August 24, 2015 at 12:13:58 PM, Cyrille Pitchen wrote:
>> The number of dummy cycles used during Fast Read commands can be reduced
>> to improve transfer performances. Each manufacturer has a dedicated set of
>> registers to provide the memory with the exact number of dummy cycles it
>> should expect. Both the memory and the (Q)SPI controller must agree on
>> this number of dummy cycles.
>>
>> The number of dummy cycles can be found into the memory datasheet and
>> mostly depends on the SPI clock frequency, the Fast Read op code and the
>> Single/Dual Data Rate mode.
>>
>> Probing JEDEC Serial Flash Discoverable Parameters (SFDP) tables would
>> only provide the driver with a high enough number of dummy cycles for each
>> Fast Read command to be used for all clock frequencies: this solution
>> would not be optimized.
>>
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> 
> Hi!
> 
>>  drivers/mtd/spi-nor/spi-nor.c | 97
>> ++++++++++++++++++++++++++++++++++--------- include/linux/mtd/spi-nor.h  
>> |  2 +
>>  2 files changed, 80 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index e2a6029dc056..869e098a6841 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -119,24 +119,6 @@ static int read_cr(struct spi_nor *nor)
>>  }
>>
>>  /*
>> - * Dummy Cycle calculation for different type of read.
>> - * It can be used to support more commands with
>> - * different dummy cycle requirements.
>> - */
>> -static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
>> -{
>> -	switch (nor->flash_read) {
>> -	case SPI_NOR_FAST:
>> -	case SPI_NOR_DUAL:
>> -	case SPI_NOR_QUAD:
>> -		return 8;
>> -	case SPI_NOR_NORMAL:
>> -		return 0;
>> -	}
>> -	return 0;
>> -}
> 
> You can probably just soup up this function so that it sets the
> nor->read_dummy, no ?
>

Actually, this is what the patch does: spi_nor_read_dummy_cycles() was reused
and enhanced few lines below where you've pointed out the 
"switch (nor->flash_read)" block should be move after the else block.

I think when I wrote the code I've chosen to move the definition of this
function instead of adding forward declarations of functions such as read_cr()
or write_sr_cr(), which are now called by micron_set_dummy_cycles().

>> -/*
>>   * Write status register 1 byte
>>   * Returns negative if error occurred.
>>   */
>> @@ -1012,6 +994,81 @@ static int set_quad_mode(struct spi_nor *nor, struct
>> flash_info *info) }
>>  }
>>
>> +static int micron_set_dummy_cycles(struct spi_nor *nor)
>> +{
>> +	int ret;
>> +	u8 val, mask;
>> +
>> +	/* read the Volatile Configuration Register (VCR) */
> 
> NIT: If this is a sentence, start it with capital letter and end it with 
> fullstop :)
> 

done for the next version

>> +	ret = nor->read_reg(nor, SPINOR_OP_RD_VCR, &val, 1);
>> +	if (ret < 0) {
>> +		dev_err(nor->dev, "error %d reading VCR\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	write_enable(nor);
>> +
>> +	/* update the number of dummy into the VCR */
> 
> DTTO
> 

done for the next version

>> +	mask = GENMASK(7, 4);
>> +	val &= ~mask;
>> +	val |= (nor->read_dummy << 4) & mask;
>> +	ret = nor->write_reg(nor, SPINOR_OP_WR_VCR, &val, 1, 0);
>> +	if (ret < 0) {
>> +		dev_err(nor->dev, "error while writing VCR register\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = spi_nor_wait_till_ready(nor);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Dummy Cycle calculation for different type of read.
>> + * It can be used to support more commands with
>> + * different dummy cycle requirements.
>> + */
>> +static int spi_nor_read_dummy_cycles(struct spi_nor *nor,
>> +				     const struct flash_info *info)
>> +{
>> +	struct device_node *np = nor->dev->of_node;
>> +	u32 num_dummy_cycles;
>> +
>> +	if (np && !of_property_read_u32(np, "m25p,num-dummy-cycles",
>> +					&num_dummy_cycles)) {
>> +		nor->read_dummy = num_dummy_cycles;
>> +
>> +		/*
>> +		 * This switch block might be moved after the if...then...else
>> +		 * statement but it was not tested with all Spansion or Micron
>> +		 * memories.
>> +		 * Now the "m25p,num-dummy-cycles" property needs to be
>> +		 * explicitly set in the device tree so the switch statement is
>> +		 * executed. This should avoid unwanted side effects and keep
>> +		 * backward compatibility.
>> +		 */
>> +		switch (JEDEC_MFR(info)) {
>> +		case CFI_MFR_ST:
>> +			return micron_set_dummy_cycles(nor);
>> +		default:
> 
> If you do have m25p,num-dummy-cycles set for non-micron flash, you have a 
> problem here I believe.
> 
>> +			break;
>> +		}
>> +	} else {
> 
> The solution would be to drop this else {} bit here, so that if you fail in
> the DT-based configuration, you fall back to this old behavior. What do you 
> think please ? :)
> 

Good idea!
I also add a trace for the default case of "switch (JEDEC_MFR(info))":

dev_warn(dev, "can't set the number of dummy cycles\n");

So the user is notified that the driver could not use the value of
"m25p,num-dummy-cycles" from the DT before falling back to the legacy
code.

>> +		switch (nor->flash_read) {
>> +		case SPI_NOR_FAST:
>> +		case SPI_NOR_DUAL:
>> +		case SPI_NOR_QUAD:
>> +			nor->read_dummy = 8;
>> +		case SPI_NOR_NORMAL:
>> +			nor->read_dummy = 0;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> 
> [...]
> 

thanks for the review!

Best regards,

Cyrille
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web