[PATCH V5] mtd: m25p80: Make fast read configurable via DT

[PATCH V5] mtd: m25p80: Make fast read configurable via DT

[Marek Vasut]

Add DT property "m25p,fast-read" that signalises the particular
chip supports "fast read" opcode.

Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Artem Bityutskiy <artem.bityutskiy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: David Woodhouse <david.woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 Documentation/devicetree/bindings/mtd/m25p80.txt |   29 ++++++++++++++++++
 drivers/mtd/devices/m25p80.c                     |   34 +++++++++++++---------
 2 files changed, 50 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/m25p80.txt

V2: Add documentation for the DT property.
V3: Use of_property_get_bool() instead of of_find_property().
V4: Adjust the documentation's "compatible:" section, make it not
    Linux-specific, but keep the reference to Linux's list of
    supported devices. Also, use the default name, spansion,m25p80
    in the example.
V5: Enclose of_property_read_bool() into CONFIG_OF for systems that do not
    use OF / Device Tree .

diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt b/Documentation/devicetree/bindings/mtd/m25p80.txt
new file mode 100644
index 0000000..701bb58
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/m25p80.txt
@@ -0,0 +1,29 @@
+* MTD SPI driver for ST M25Pxx (and similar) serial flash chips
+
+Required properties:
+- #address-cells, #size-cells : Must be present if the device has sub-nodes
+  representing partitions.
+- compatible : Should be the manufacturer and the name of the chip. Bear in mind
+               the DT binding is not Linux-only, but in case of Linux, see the
+               "m25p_ids" table in drivers/mtd/devices/m25p80.c for the list of
+               supported chips.
+- reg : Chip-Select number
+- spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at
+
+Optional properties:
+- m25p,fast-read : Use the "fast read" opcode to read data from the chip instead
+                   of the usual "read" opcode. This opcode isn not supported by
+                   all chips and support for it can not be detected at runtime.
+                   Refer to your chips' datasheet to check if this is supported
+                   by your chip.
+
+Example:
+
+	flash: m25p80@0 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "spansion,m25p80";
+		reg = <0>;
+		spi-max-frequency = <40000000>;
+		m25p,fast-read;
+	};
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index d16f75c..c433051 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -73,14 +73,6 @@
 #define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max chip erase */
 #define	MAX_CMD_SIZE		5
 
-#ifdef CONFIG_M25PXX_USE_FAST_READ
-#define OPCODE_READ 	OPCODE_FAST_READ
-#define FAST_READ_DUMMY_BYTE 1
-#else
-#define OPCODE_READ 	OPCODE_NORM_READ
-#define FAST_READ_DUMMY_BYTE 0
-#endif
-
 #define JEDEC_MFR(_jedec_id)	((_jedec_id) >> 16)
 
 /****************************************************************************/
@@ -93,6 +85,7 @@ struct m25p {
 	u16			addr_width;
 	u8			erase_opcode;
 	u8			*command;
+	bool			fast_read;
 };
 
 static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
@@ -342,6 +335,7 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 	struct m25p *flash = mtd_to_m25p(mtd);
 	struct spi_transfer t[2];
 	struct spi_message m;
+	uint8_t opcode;
 
 	pr_debug("%s: %s from 0x%08x, len %zd\n", dev_name(&flash->spi->dev),
 			__func__, (u32)from, len);
@@ -354,7 +348,7 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 	 * Should add 1 byte DUMMY_BYTE.
 	 */
 	t[0].tx_buf = flash->command;
-	t[0].len = m25p_cmdsz(flash) + FAST_READ_DUMMY_BYTE;
+	t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0);
 	spi_message_add_tail(&t[0], &m);
 
 	t[1].rx_buf = buf;
@@ -376,12 +370,14 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 	 */
 
 	/* Set up the write data buffer. */
-	flash->command[0] = OPCODE_READ;
+	opcode = flash->fast_read ? OPCODE_FAST_READ : OPCODE_NORM_READ;
+	flash->command[0] = opcode;
 	m25p_addr2cmd(flash, from, flash->command);
 
 	spi_sync(flash->spi, &m);
 
-	*retlen = m.actual_length - m25p_cmdsz(flash) - FAST_READ_DUMMY_BYTE;
+	*retlen = m.actual_length - m25p_cmdsz(flash) -
+			(flash->fast_read ? 1 : 0);
 
 	mutex_unlock(&flash->lock);
 
@@ -804,9 +800,10 @@ static int __devinit m25p_probe(struct spi_device *spi)
 	struct flash_info		*info;
 	unsigned			i;
 	struct mtd_part_parser_data	ppdata;
+	struct device_node		*np = spi->dev.of_node;
 
 #ifdef CONFIG_MTD_OF_PARTS
-	if (!of_device_is_available(spi->dev.of_node))
+	if (!of_device_is_available(np))
 		return -ENODEV;
 #endif
 
@@ -858,7 +855,8 @@ static int __devinit m25p_probe(struct spi_device *spi)
 	flash = kzalloc(sizeof *flash, GFP_KERNEL);
 	if (!flash)
 		return -ENOMEM;
-	flash->command = kmalloc(MAX_CMD_SIZE + FAST_READ_DUMMY_BYTE, GFP_KERNEL);
+	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 : 0),
+					GFP_KERNEL);
 	if (!flash->command) {
 		kfree(flash);
 		return -ENOMEM;
@@ -915,6 +913,16 @@ static int __devinit m25p_probe(struct spi_device *spi)
 	flash->page_size = info->page_size;
 	flash->mtd.writebufsize = flash->page_size;
 
+	flash->fast_read = false;
+#ifdef CONFIG_OF
+	if (np && of_property_read_bool(np, "m25p,fast-read"))
+		flash->fast_read = true;
+#endif
+
+#ifdef CONFIG_M25PXX_USE_FAST_READ
+	flash->fast_read = true;
+#endif
+
 	if (info->addr_width)
 		flash->addr_width = info->addr_width;
 	else {
-- 
1.7.10.4

Re: [PATCH V5] mtd: m25p80: Make fast read configurable via DT

[Kevin Cernekee]

On Sat, Aug 25, 2012 at 12:36 PM, Marek Vasut <marex@denx.de> wrote:
> Add DT property "m25p,fast-read" that signalises the particular
> chip supports "fast read" opcode.

This might be slightly clearer if it is rephrased as:

Add DT property "m25p,fast-read" that signifies whether the "fast
read" opcode is supported.

> +Optional properties:
> +- m25p,fast-read : Use the "fast read" opcode to read data from the chip instead
> +                   of the usual "read" opcode. This opcode isn not supported by
> +                   all chips and support for it can not be detected at runtime.

"is not supported"

Are there any modern SPI flash parts that can't handle FAST_READ, or
is this mostly for compatibility with legacy systems?

> +       bool                    fast_read;

> -       t[0].len = m25p_cmdsz(flash) + FAST_READ_DUMMY_BYTE;
> +       t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0);

Newer devices support a variable number of dummy cycles; increasing
the number of dummy cycles can allow for higher interface speeds.  One
example is the Macronix MX25L25635F:

http://bit.ly/P9s7UM

It might be worth thinking about how to capture this sort of
information in the DT properties, even if current versions of m25p80
only use a small subset of the device capabilities.

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

Re: [PATCH V5] mtd: m25p80: Make fast read configurable via DT

[Marek Vasut]

Dear Kevin Cernekee,

> On Sat, Aug 25, 2012 at 12:36 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> > Add DT property "m25p,fast-read" that signalises the particular
> > chip supports "fast read" opcode.
> 
> This might be slightly clearer if it is rephrased as:
> 
> Add DT property "m25p,fast-read" that signifies whether the "fast
> read" opcode is supported.

What's the difference?

> > +Optional properties:
> > +- m25p,fast-read : Use the "fast read" opcode to read data from the chip
> > instead +                   of the usual "read" opcode. This opcode isn
> > not supported by +                   all chips and support for it can
> > not be detected at runtime.
> 
> "is not supported"

Thanks

> Are there any modern SPI flash parts that can't handle FAST_READ, or
> is this mostly for compatibility with legacy systems?

We have to support all possibilities, not only "modern systems" .

> > +       bool                    fast_read;
> > 
> > -       t[0].len = m25p_cmdsz(flash) + FAST_READ_DUMMY_BYTE;
> > +       t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0);
> 
> Newer devices support a variable number of dummy cycles; increasing
> the number of dummy cycles can allow for higher interface speeds.  One
> example is the Macronix MX25L25635F:
> 
> http://bit.ly/P9s7UM

Good, but I don't see it supported in the driver, how is this different from 
MX25L256E?

Besides, I noticed the fast-read isn't even properly excersized, as it always 
reads blocks of 512 bytes, even if it can read any basically length. Why is 
that, MTD layer stuff or something?

> It might be worth thinking about how to capture this sort of
> information in the DT properties, even if current versions of m25p80
> only use a small subset of the device capabilities.

They can be added as additional DT props, that should be easy.

Best regards,
Marek Vasut

Re: [PATCH V5] mtd: m25p80: Make fast read configurable via DT

[Kevin Cernekee]

On Sat, Aug 25, 2012 at 1:40 PM, Marek Vasut <marex@denx.de> wrote:
>> Are there any modern SPI flash parts that can't handle FAST_READ, or
>> is this mostly for compatibility with legacy systems?
>
> We have to support all possibilities, not only "modern systems" .

If the !FAST_READ option only exists to support a tiny number of rare,
ancient flash chips, then it's probably worth pointing that out to the
user in the help text.  Similar to "most users will say 'N' here" in
Kconfig.  Or the help text for CONFIG_MTD_NAND_MUSEUM_IDS.

> Good, but I don't see it supported in the driver, how is this different from
> MX25L256E?

> They can be added as additional DT props, that should be easy.

It's easy to add new features to m25p80, but it's hard to change DT
properties once they are widely used.  They will be baked into
bootloader images and could be utilized by non-Linux operating
systems.

If we already know today that a boolean property is insufficient for
current generations of SPI flash chips, it would not be a good thing
to write it in stone now and be forced to hack around it later.

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

Re: [PATCH V5] mtd: m25p80: Make fast read configurable via DT

[Marek Vasut]

Dear Kevin Cernekee,

> On Sat, Aug 25, 2012 at 1:40 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> >> Are there any modern SPI flash parts that can't handle FAST_READ, or
> >> is this mostly for compatibility with legacy systems?
> > 
> > We have to support all possibilities, not only "modern systems" .
> 
> If the !FAST_READ option only exists to support a tiny number of rare,
> ancient flash chips, then it's probably worth pointing that out to the
> user in the help text.  Similar to "most users will say 'N' here" in
> Kconfig.  Or the help text for CONFIG_MTD_NAND_MUSEUM_IDS.
> 
> > Good, but I don't see it supported in the driver, how is this different
> > from MX25L256E?
> > 
> > They can be added as additional DT props, that should be easy.
> 
> It's easy to add new features to m25p80, but it's hard to change DT
> properties once they are widely used.  They will be baked into
> bootloader images and could be utilized by non-Linux operating
> systems.
> 
> If we already know today that a boolean property is insufficient for
> current generations of SPI flash chips, it would not be a good thing
> to write it in stone now and be forced to hack around it later.

Actually, fast read opcode will always be fast read opcode. I do see your 
concern, but then, until there is a way to probe for supported opcodes and 
features, you'll end up having to describe your chip properly.

Of course, the list of IDs built into Linux is cool and can be used to toggle 
the fast read I think, but what about running older linux kernel with a new chip 
it doesn't support? The DT props could be easily used to describe even the new 
chip then and there won't be any problem -- compared to "internal ID table".

Best regards,
Marek Vasut

Re: [PATCH V5] mtd: m25p80: Make fast read configurable via DT

[Marek Vasut]

Hi Artem,

> Add DT property "m25p,fast-read" that signalises the particular
> chip supports "fast read" opcode.

Do you think it's aplicable now ?

> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Cc: Artem Bityutskiy <artem.bityutskiy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: David Woodhouse <david.woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

[...]

Best regards,
Marek Vasut

Re: [PATCH V5] mtd: m25p80: Make fast read configurable via DT

[Marek Vasut]

Bump, do you think we can get this into 3.7 please?

> Add DT property "m25p,fast-read" that signalises the particular
> chip supports "fast read" opcode.
> 
> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Cc: Artem Bityutskiy <artem.bityutskiy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: David Woodhouse <david.woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/mtd/m25p80.txt |   29 ++++++++++++++++++
>  drivers/mtd/devices/m25p80.c                     |   34
> +++++++++++++--------- 2 files changed, 50 insertions(+), 13 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mtd/m25p80.txt
> 
> V2: Add documentation for the DT property.
> V3: Use of_property_get_bool() instead of of_find_property().
> V4: Adjust the documentation's "compatible:" section, make it not
>     Linux-specific, but keep the reference to Linux's list of
>     supported devices. Also, use the default name, spansion,m25p80
>     in the example.
> V5: Enclose of_property_read_bool() into CONFIG_OF for systems that do not
>     use OF / Device Tree .
> 
> diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt
> b/Documentation/devicetree/bindings/mtd/m25p80.txt new file mode 100644
> index 0000000..701bb58
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/m25p80.txt
> @@ -0,0 +1,29 @@
> +* MTD SPI driver for ST M25Pxx (and similar) serial flash chips
> +
> +Required properties:
> +- #address-cells, #size-cells : Must be present if the device has
> sub-nodes +  representing partitions.
> +- compatible : Should be the manufacturer and the name of the chip. Bear
> in mind +               the DT binding is not Linux-only, but in case of
> Linux, see the +               "m25p_ids" table in
> drivers/mtd/devices/m25p80.c for the list of +               supported
> chips.
> +- reg : Chip-Select number
> +- spi-max-frequency : Maximum frequency of the SPI bus the chip can
> operate at +
> +Optional properties:
> +- m25p,fast-read : Use the "fast read" opcode to read data from the chip
> instead +                   of the usual "read" opcode. This opcode isn
> not supported by +                   all chips and support for it can not
> be detected at runtime. +                   Refer to your chips' datasheet
> to check if this is supported +                   by your chip.
> +
> +Example:
> +
> +	flash: m25p80@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "spansion,m25p80";
> +		reg = <0>;
> +		spi-max-frequency = <40000000>;
> +		m25p,fast-read;
> +	};
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index d16f75c..c433051 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -73,14 +73,6 @@
>  #define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max 
chip
> erase */ #define	MAX_CMD_SIZE		5
> 
> -#ifdef CONFIG_M25PXX_USE_FAST_READ
> -#define OPCODE_READ 	OPCODE_FAST_READ
> -#define FAST_READ_DUMMY_BYTE 1
> -#else
> -#define OPCODE_READ 	OPCODE_NORM_READ
> -#define FAST_READ_DUMMY_BYTE 0
> -#endif
> -
>  #define JEDEC_MFR(_jedec_id)	((_jedec_id) >> 16)
> 
>  /*************************************************************************
> ***/ @@ -93,6 +85,7 @@ struct m25p {
>  	u16			addr_width;
>  	u8			erase_opcode;
>  	u8			*command;
> +	bool			fast_read;
>  };
> 
>  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> @@ -342,6 +335,7 @@ static int m25p80_read(struct mtd_info *mtd, loff_t
> from, size_t len, struct m25p *flash = mtd_to_m25p(mtd);
>  	struct spi_transfer t[2];
>  	struct spi_message m;
> +	uint8_t opcode;
> 
>  	pr_debug("%s: %s from 0x%08x, len %zd\n", dev_name(&flash->spi->dev),
>  			__func__, (u32)from, len);
> @@ -354,7 +348,7 @@ static int m25p80_read(struct mtd_info *mtd, loff_t
> from, size_t len, * Should add 1 byte DUMMY_BYTE.
>  	 */
>  	t[0].tx_buf = flash->command;
> -	t[0].len = m25p_cmdsz(flash) + FAST_READ_DUMMY_BYTE;
> +	t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0);
>  	spi_message_add_tail(&t[0], &m);
> 
>  	t[1].rx_buf = buf;
> @@ -376,12 +370,14 @@ static int m25p80_read(struct mtd_info *mtd, loff_t
> from, size_t len, */
> 
>  	/* Set up the write data buffer. */
> -	flash->command[0] = OPCODE_READ;
> +	opcode = flash->fast_read ? OPCODE_FAST_READ : OPCODE_NORM_READ;
> +	flash->command[0] = opcode;
>  	m25p_addr2cmd(flash, from, flash->command);
> 
>  	spi_sync(flash->spi, &m);
> 
> -	*retlen = m.actual_length - m25p_cmdsz(flash) - FAST_READ_DUMMY_BYTE;
> +	*retlen = m.actual_length - m25p_cmdsz(flash) -
> +			(flash->fast_read ? 1 : 0);
> 
>  	mutex_unlock(&flash->lock);
> 
> @@ -804,9 +800,10 @@ static int __devinit m25p_probe(struct spi_device
> *spi) struct flash_info		*info;
>  	unsigned			i;
>  	struct mtd_part_parser_data	ppdata;
> +	struct device_node		*np = spi->dev.of_node;
> 
>  #ifdef CONFIG_MTD_OF_PARTS
> -	if (!of_device_is_available(spi->dev.of_node))
> +	if (!of_device_is_available(np))
>  		return -ENODEV;
>  #endif
> 
> @@ -858,7 +855,8 @@ static int __devinit m25p_probe(struct spi_device *spi)
>  	flash = kzalloc(sizeof *flash, GFP_KERNEL);
>  	if (!flash)
>  		return -ENOMEM;
> -	flash->command = kmalloc(MAX_CMD_SIZE + FAST_READ_DUMMY_BYTE,
> GFP_KERNEL); +	flash->command = kmalloc(MAX_CMD_SIZE + (flash-
>fast_read ?
> 1 : 0), +					GFP_KERNEL);
>  	if (!flash->command) {
>  		kfree(flash);
>  		return -ENOMEM;
> @@ -915,6 +913,16 @@ static int __devinit m25p_probe(struct spi_device
> *spi) flash->page_size = info->page_size;
>  	flash->mtd.writebufsize = flash->page_size;
> 
> +	flash->fast_read = false;
> +#ifdef CONFIG_OF
> +	if (np && of_property_read_bool(np, "m25p,fast-read"))
> +		flash->fast_read = true;
> +#endif
> +
> +#ifdef CONFIG_M25PXX_USE_FAST_READ
> +	flash->fast_read = true;
> +#endif
> +
>  	if (info->addr_width)
>  		flash->addr_width = info->addr_width;
>  	else {

Re: [PATCH V5] mtd: m25p80: Make fast read configurable via DT

[Artem Bityutskiy]

[-- Attachment #1.1: Type: text/plain, Size: 613 bytes --]

On Mon, 2012-09-17 at 16:10 +0200, Marek Vasut wrote:
> Bump, do you think we can get this into 3.7 please?

Aiaiai!

--------------------------------------------------------------------------------

Successfully built configuration "i386_defconfig,i386,", results:

--- before_patching.log
+++ after_patching.log
@@ @@
+drivers/mtd/devices/m25p80.c: In function ‘m25p_probe’:
+drivers/mtd/devices/m25p80.c:808:23: warning: unused variable ‘np’ [-Wunused-variable]

--------------------------------------------------------------------------------


-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH V5] mtd: m25p80: Make fast read configurable via DT

[Marek Vasut]

Dear Artem Bityutskiy,

> On Mon, 2012-09-17 at 16:10 +0200, Marek Vasut wrote:
> > Bump, do you think we can get this into 3.7 please?
> 
> Aiaiai!

Damn I'm really lucky with this one ...

Best regards,
Marek Vasut