[PATCH 0/3]Add quad/memory mapped support for SPI flash.

[PATCH 0/3]Add quad/memory mapped support for SPI flash.

[Sourav Poddar]

This patch add quad/memory mapped support in flash driver(m25p80).

Added support for quad memory mapped for TI qspi controller also.

This patch has been tested with some internal dts data.

Sourav Poddar (3):
  spi/qspi: Add memory mapped read support.
  drivers: mtd: devices: Add quad read support.
  drivers: mtd: devices: Add memory mapped read support.

 drivers/mtd/devices/m25p80.c |  193 ++++++++++++++++++++++++++++++++++++++++--
 drivers/spi/spi-ti-qspi.c    |  140 +++++++++++++++++++++++++++---
 include/linux/spi/spi.h      |    2 +
 3 files changed, 312 insertions(+), 23 deletions(-)

[PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

Qspi controller also supports memory mapped read. Patch
adds support for the same.
In memory mapped read, controller need to be switched to a
memory mapped port using a control module register and a qspi
specific register or just a qspi register.
Then the read need to be happened from the memory mapped
address space.

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 drivers/spi/spi-ti-qspi.c |  140 ++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 125 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 0b71270..2722840 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -46,6 +46,8 @@ struct ti_qspi {
 
 	struct spi_master	*master;
 	void __iomem            *base;
+	void __iomem            *ctrl_base;
+	void __iomem            *mmap_base;
 	struct clk		*fclk;
 	struct device           *dev;
 
@@ -54,6 +56,9 @@ struct ti_qspi {
 	u32 spi_max_frequency;
 	u32 cmd;
 	u32 dc;
+
+	bool memory_mapped;
+	bool ctrl_mod;
 };
 
 #define QSPI_PID			(0x0)
@@ -109,6 +114,23 @@ struct ti_qspi {
 #define QSPI_CSPOL(n)			(1 << (1 + n * 8))
 #define QSPI_CKPOL(n)			(1 << (n * 8))
 
+#define	MM_SWITCH	0x01
+#define	MEM_CS		0x100
+#define	MEM_CS_DIS	0xfffff0ff
+
+#define	QSPI_CMD_RD		(0x3 << 0)
+#define	QSPI_CMD_DUAL_RD	(0x3b << 0)
+#define	QSPI_CMD_QUAD_RD	(0x6b << 0)
+#define	QSPI_CMD_READ_FAST	(0x0b << 0)
+#define	QSPI_SETUP0_A_BYTES	(0x3 << 8)
+#define	QSPI_SETUP0_NO_BITS	(0x0 << 10)
+#define	QSPI_SETUP0_8_BITS	(0x1 << 10)
+#define	QSPI_SETUP0_RD_NORMAL	(0x0 << 12)
+#define	QSPI_SETUP0_RD_DUAL	(0x1 << 12)
+#define	QSPI_SETUP0_RD_QUAD	(0x3 << 12)
+#define	QSPI_CMD_WRITE		(0x2 << 16)
+#define	QSPI_NUM_DUMMY_BITS	(0x0 << 24)
+
 #define	QSPI_FRAME			4096
 
 #define QSPI_AUTOSUSPEND_TIMEOUT         2000
@@ -125,12 +147,37 @@ static inline void ti_qspi_write(struct ti_qspi *qspi,
 	writel(val, qspi->base + reg);
 }
 
+void enable_qspi_memory_mapped(struct ti_qspi *qspi)
+{
+	u32 val;
+
+	ti_qspi_write(qspi, MM_SWITCH, QSPI_SPI_SWITCH_REG);
+	if (qspi->ctrl_mod) {
+		val = readl(qspi->ctrl_base);
+		val |= MEM_CS;
+		writel(val, qspi->ctrl_base);
+	}
+}
+
+void disable_qspi_memory_mapped(struct ti_qspi *qspi)
+{
+	u32 val;
+
+	ti_qspi_write(qspi, ~MM_SWITCH, QSPI_SPI_SWITCH_REG);
+	if (qspi->ctrl_mod) {
+		val = readl(qspi->ctrl_base);
+		val |= MEM_CS_DIS;
+		writel(val, qspi->ctrl_base);
+	}
+}
+
 static int ti_qspi_setup(struct spi_device *spi)
 {
 	struct ti_qspi	*qspi = spi_master_get_devdata(spi->master);
 	struct ti_qspi_regs *ctx_reg = &qspi->ctx_reg;
 	int clk_div = 0, ret;
-	u32 clk_ctrl_reg, clk_rate, clk_mask;
+	u32 clk_ctrl_reg, clk_rate, clk_mask, memval = 0;
+	qspi->dc = 0;
 
 	if (spi->master->busy) {
 		dev_dbg(qspi->dev, "master busy doing other trasnfers\n");
@@ -178,6 +225,37 @@ static int ti_qspi_setup(struct spi_device *spi)
 	ti_qspi_write(qspi, clk_mask, QSPI_SPI_CLOCK_CNTRL_REG);
 	ctx_reg->clkctrl = clk_mask;
 
+	if (spi->mode & SPI_CPHA)
+		qspi->dc |= QSPI_CKPHA(spi->chip_select);
+	if (spi->mode & SPI_CPOL)
+		qspi->dc |= QSPI_CKPOL(spi->chip_select);
+	if (spi->mode & SPI_CS_HIGH)
+		qspi->dc |= QSPI_CSPOL(spi->chip_select);
+
+	ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);
+
+	if (qspi->memory_mapped) {
+		switch (spi->mode) {
+		case SPI_TX_DUAL:
+			memval |= (QSPI_CMD_DUAL_RD | QSPI_SETUP0_A_BYTES |
+				QSPI_SETUP0_8_BITS | QSPI_SETUP0_RD_DUAL |
+				QSPI_CMD_WRITE | QSPI_NUM_DUMMY_BITS);
+			break;
+		case SPI_TX_QUAD:
+			memval |= (QSPI_CMD_QUAD_RD | QSPI_SETUP0_A_BYTES |
+				QSPI_SETUP0_8_BITS | QSPI_SETUP0_RD_QUAD |
+				QSPI_CMD_WRITE | QSPI_NUM_DUMMY_BITS);
+			break;
+		default:
+			memval |= (QSPI_CMD_RD | QSPI_SETUP0_A_BYTES |
+				QSPI_SETUP0_NO_BITS | QSPI_SETUP0_RD_NORMAL |
+				QSPI_CMD_WRITE | QSPI_NUM_DUMMY_BITS);
+			break;
+		}
+		ti_qspi_write(qspi, memval, QSPI_SPI_SETUP0_REG);
+		spi->mode |= SPI_RX_MMAP;
+	}
+
 	pm_runtime_mark_last_busy(qspi->dev);
 	ret = pm_runtime_put_autosuspend(qspi->dev);
 	if (ret < 0) {
@@ -340,16 +418,7 @@ static int ti_qspi_start_transfer_one(struct spi_master *master,
 	struct spi_transfer *t;
 	int status = 0, ret;
 	int frame_length;
-
-	/* setup device control reg */
-	qspi->dc = 0;
-
-	if (spi->mode & SPI_CPHA)
-		qspi->dc |= QSPI_CKPHA(spi->chip_select);
-	if (spi->mode & SPI_CPOL)
-		qspi->dc |= QSPI_CKPOL(spi->chip_select);
-	if (spi->mode & SPI_CS_HIGH)
-		qspi->dc |= QSPI_CSPOL(spi->chip_select);
+	size_t from = 0;
 
 	frame_length = (m->frame_length << 3) / spi->bits_per_word;
 
@@ -362,11 +431,21 @@ static int ti_qspi_start_transfer_one(struct spi_master *master,
 	qspi->cmd |= QSPI_WC_CMD_INT_EN;
 
 	ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
-	ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);
 
 	mutex_lock(&qspi->list_lock);
 
 	list_for_each_entry(t, &m->transfers, transfer_list) {
+		if (t->memory_map) {
+			if (t->tx_buf) {
+				from = t->len;
+				continue;
+			}
+			enable_qspi_memory_mapped(qspi);
+			memcpy(t->rx_buf, qspi->mmap_base + from, t->len);
+			disable_qspi_memory_mapped(qspi);
+			goto out;
+		}
+
 		qspi->cmd |= QSPI_WLEN(t->bits_per_word);
 
 		ret = qspi_transfer_msg(qspi, t);
@@ -379,6 +458,7 @@ static int ti_qspi_start_transfer_one(struct spi_master *master,
 		m->actual_length += t->len;
 	}
 
+out:
 	mutex_unlock(&qspi->list_lock);
 
 	m->status = status;
@@ -437,7 +517,7 @@ static int ti_qspi_probe(struct platform_device *pdev)
 {
 	struct  ti_qspi *qspi;
 	struct spi_master *master;
-	struct resource         *r;
+	struct resource         *r, *res_ctrl, *res_mmap;
 	struct device_node *np = pdev->dev.of_node;
 	u32 max_freq;
 	int ret = 0, num_cs, irq;
@@ -446,7 +526,8 @@ static int ti_qspi_probe(struct platform_device *pdev)
 	if (!master)
 		return -ENOMEM;
 
-	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD;
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
+					SPI_RX_MMAP;
 
 	master->bus_num = -1;
 	master->flags = SPI_MASTER_HALF_DUPLEX;
@@ -465,7 +546,16 @@ static int ti_qspi_probe(struct platform_device *pdev)
 	qspi->master = master;
 	qspi->dev = &pdev->dev;
 
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_base");
+	if (r == NULL) {
+		dev_err(&pdev->dev, "missing platform resources data\n");
+		return -ENODEV;
+	}
+
+	res_mmap = platform_get_resource_byname(pdev,
+				IORESOURCE_MEM, "qspi_mmap");
+	res_ctrl = platform_get_resource_byname(pdev,
+				IORESOURCE_MEM, "qspi_ctrlmod");
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
@@ -481,6 +571,23 @@ static int ti_qspi_probe(struct platform_device *pdev)
 		goto free_master;
 	}
 
+	if (res_ctrl) {
+		qspi->ctrl_mod = true;
+		qspi->ctrl_base = devm_ioremap_resource(&pdev->dev, res_ctrl);
+		if (IS_ERR(qspi->ctrl_base)) {
+			ret = PTR_ERR(qspi->ctrl_base);
+			goto free_master;
+		}
+	}
+
+	if (res_mmap) {
+		qspi->mmap_base = devm_ioremap_resource(&pdev->dev, res_mmap);
+		if (IS_ERR(qspi->mmap_base)) {
+			ret = PTR_ERR(qspi->mmap_base);
+			goto free_master;
+		}
+	}
+
 	ret = devm_request_irq(&pdev->dev, irq, ti_qspi_isr, 0,
 			dev_name(&pdev->dev), qspi);
 	if (ret < 0) {
@@ -504,6 +611,9 @@ static int ti_qspi_probe(struct platform_device *pdev)
 	if (!of_property_read_u32(np, "spi-max-frequency", &max_freq))
 		qspi->spi_max_frequency = max_freq;
 
+	if (of_property_read_bool(np, "mmap_read"))
+		qspi->memory_mapped = true;
+
 	ret = devm_spi_register_master(&pdev->dev, master);
 	if (ret)
 		goto free_master;
-- 
1.7.1

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

On Wed, Oct 09, 2013 at 08:54:42PM +0530, Sourav Poddar wrote:

> Qspi controller also supports memory mapped read. Patch
> adds support for the same.
> In memory mapped read, controller need to be switched to a
> memory mapped port using a control module register and a qspi
> specific register or just a qspi register.
> Then the read need to be happened from the memory mapped
> address space.

Can you provide more details on what exactly this means?  Looking at the
code it looks awfully like this has the same problem that the Freescale
code had with needing to know the commands the flash needs?

I'm also concerned about the interface here, it looks like this is being
made visible to SPI devices (via a dependency on patch 3/3...) but only
as a flag they can set - how would devices know to enable this and why
would they want to avoid it?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

On Wednesday 09 October 2013 09:37 PM, Mark Brown wrote:
> On Wed, Oct 09, 2013 at 08:54:42PM +0530, Sourav Poddar wrote:
>
>> Qspi controller also supports memory mapped read. Patch
>> adds support for the same.
>> In memory mapped read, controller need to be switched to a
>> memory mapped port using a control module register and a qspi
>> specific register or just a qspi register.
>> Then the read need to be happened from the memory mapped
>> address space.
> Can you provide more details on what exactly this means?  Looking at the
> code it looks awfully like this has the same problem that the Freescale
> code had with needing to know the commands the flash needs?
Here is the exact feature usecase..
TI qspi controller supports memory mapped read. These memory
mapped read configuration depends on the set_up_register, which
can be configured once during in setup apis based on the dt node
specifying whether the qspi controller supports memory mapped read
or not.
Once, the qspi controller is configured for a memory mapped read, the qspi
controller does not depend on the flash command that comes from the mtd 
layer.
Because, this command are already configured in QSPI set up register.

To use qspi in memory mapped mode, we need to switch to memory mapped port
using certain registers(for am43x its only qspi register, while for 
dra7x its qspi register
as well as control module register) and once the memory mapped read is 
done, switch
back to configuration mode register.

Basically, its not the commands which need to be communicated from the 
mtd layer,its just
the buffer to fill, len of buffer, offset from where to fill need to be 
communicated.

> I'm also concerned about the interface here, it looks like this is being
> made visible to SPI devices (via a dependency on patch 3/3...) but only
> as a flag they can set - how would devices know to enable this and why
> would they want to avoid it?
Set  spi->mode in qspi driver based on dt entry and use that in mtd layer to
decide whether to use memory mapped or not.

The idea is whenever, we call mtd_read api from mtd layer, if memory 
mapped is set
then sending the commands does not matter, what matters is the len to 
read, buffer to fill and
"from" offset to read. Then, the intention was to use the memory_map 
transfer parameter to
communicate to the driver that memory mapped read is used so that we can 
just use memcopy and
return without going through the entire SPI based xfer function.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3085 bytes --]

On Wed, Oct 09, 2013 at 10:24:33PM +0530, Sourav Poddar wrote:

> Here is the exact feature usecase..
> TI qspi controller supports memory mapped read. These memory
> mapped read configuration depends on the set_up_register, which
> can be configured once during in setup apis based on the dt node
> specifying whether the qspi controller supports memory mapped read
> or not.

What is "set_up_register"?

> Once, the qspi controller is configured for a memory mapped read, the qspi
> controller does not depend on the flash command that comes from the
> mtd layer.
> Because, this command are already configured in QSPI set up register.

So this does depend on the flash commands for the specific chip, which
means that this has all the same problems as the Freescale chip had with
requiring the user to replicate the information about the commands that
the chip supports into the device tree.  It therefore seems like all the
same concerns should apply, though in this case it seems like it's
harder for the driver to infer things from looking at the operations
being sent to it.

Presumably this also only works for flash chips, or things that look
like them...

> Basically, its not the commands which need to be communicated from
> the mtd layer,its just
> the buffer to fill, len of buffer, offset from where to fill need to
> be communicated.

This appears to be based on an assumption that the commands would be
replicated into the device trees which seems like it's both more work
for users and harder to deploy.

> >I'm also concerned about the interface here, it looks like this is being
> >made visible to SPI devices (via a dependency on patch 3/3...) but only
> >as a flag they can set - how would devices know to enable this and why
> >would they want to avoid it?

> Set  spi->mode in qspi driver based on dt entry and use that in mtd layer to
> decide whether to use memory mapped or not.

But why would anything not want to use memory mapped mode if it can and
why is this something that should be hard coded into the device tree?
Especially with the current API...

> The idea is whenever, we call mtd_read api from mtd layer, if memory
> mapped is set
> then sending the commands does not matter, what matters is the len
> to read, buffer to fill and
> "from" offset to read. Then, the intention was to use the memory_map
> transfer parameter to
> communicate to the driver that memory mapped read is used so that we
> can just use memcopy and
> return without going through the entire SPI based xfer function.

I'm not convinced that this is the most useful API, it sounds like the
hardware can "memory map" the entire flash chip so the whole SPI
framework seems like overhead.

It also seems seems like it's going to involve the CPU being stalled
waiting for reads to complete instead of asking the SPI controller to
DMA the data to RAM and allowing the CPU to get on with other things -
replacing the explicit transmission of commands with memory to memory
DMAs might be advantageous but replacing DMA with memcpy() would need
numbers to show that it was a win.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

On Wednesday 09 October 2013 11:10 PM, Mark Brown wrote:
> On Wed, Oct 09, 2013 at 10:24:33PM +0530, Sourav Poddar wrote:
>
>> Here is the exact feature usecase..
>> TI qspi controller supports memory mapped read. These memory
>> mapped read configuration depends on the set_up_register, which
>> can be configured once during in setup apis based on the dt node
>> specifying whether the qspi controller supports memory mapped read
>> or not.
> What is "set_up_register"?
set_up_register is a memory mapped specific register with the
following fields:
- RCMD (Read command, which is actually the flash read command)
- NUmber of Address bytes
- Number of dummy bytes
- Read type(Single, dual and quad read).
- Write command.
>> Once, the qspi controller is configured for a memory mapped read, the qspi
>> controller does not depend on the flash command that comes from the
>> mtd layer.
>> Because, this command are already configured in QSPI set up register.
> So this does depend on the flash commands for the specific chip, which
> means that this has all the same problems as the Freescale chip had with
> requiring the user to replicate the information about the commands that
> the chip supports into the device tree.  It therefore seems like all the
> same concerns should apply, though in this case it seems like it's
> harder for the driver to infer things from looking at the operations
> being sent to it.
>
> Presumably this also only works for flash chips, or things that look
> like them...
>
Yes, true, it depends on flash command, though it is getting filled now in
my driver itself.
>> Basically, its not the commands which need to be communicated from
>> the mtd layer,its just
>> the buffer to fill, len of buffer, offset from where to fill need to
>> be communicated.
> This appears to be based on an assumption that the commands would be
> replicated into the device trees which seems like it's both more work
> for users and harder to deploy.
>
>>> I'm also concerned about the interface here, it looks like this is being
>>> made visible to SPI devices (via a dependency on patch 3/3...) but only
>>> as a flag they can set - how would devices know to enable this and why
>>> would they want to avoid it?
>> Set  spi->mode in qspi driver based on dt entry and use that in mtd layer to
>> decide whether to use memory mapped or not.
> But why would anything not want to use memory mapped mode if it can and
> why is this something that should be hard coded into the device tree?
> Especially with the current API...
>
Thats true, by default also we can keep the memory mapped read. Though, 
according
to the current implementation spi->mode can be set so that in mtd layer, 
we might
use that to selectively used t[o].memory_map.
>> The idea is whenever, we call mtd_read api from mtd layer, if memory
>> mapped is set
>> then sending the commands does not matter, what matters is the len
>> to read, buffer to fill and
>> "from" offset to read. Then, the intention was to use the memory_map
>> transfer parameter to
>> communicate to the driver that memory mapped read is used so that we
>> can just use memcopy and
>> return without going through the entire SPI based xfer function.
> I'm not convinced that this is the most useful API, it sounds like the
> hardware can "memory map" the entire flash chip so the whole SPI
> framework seems like overhead.
>
But this memory map read will work only with read opcodes.(mtd_read
path). For all other operations, normal SPI operations will be used.

As for this, I also though of bypassing the SPI frameowrk, and doing a 
memcopy
at the beginning of the mtd_read api. But, then before doing a memory mapped
read -
1. Controller need to be switched to memory mapped port using control module
      register and ti qspi switch register.
2. There is SOC specific memory mapped address space from where read 
should happen,
     this is SOC specific and should be known to mtd layer the adreess 
to read for.
So, I thought of going this way using t.memory map flag.
> It also seems seems like it's going to involve the CPU being stalled
> waiting for reads to complete instead of asking the SPI controller to
> DMA the data to RAM and allowing the CPU to get on with other things -
> replacing the explicit transmission of commands with memory to memory
> DMAs might be advantageous but replacing DMA with memcpy() would need
> numbers to show that it was a win.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1709 bytes --]

On Wed, Oct 09, 2013 at 11:45:46PM +0530, Sourav Poddar wrote:
> On Wednesday 09 October 2013 11:10 PM, Mark Brown wrote:

> >I'm not convinced that this is the most useful API, it sounds like the
> >hardware can "memory map" the entire flash chip so the whole SPI
> >framework seems like overhead.

> But this memory map read will work only with read opcodes.(mtd_read
> path). For all other operations, normal SPI operations will be used.

> As for this, I also though of bypassing the SPI frameowrk, and doing
> a memcopy
> at the beginning of the mtd_read api. But, then before doing a memory mapped
> read -
> 1. Controller need to be switched to memory mapped port using control module
>      register and ti qspi switch register.
> 2. There is SOC specific memory mapped address space from where read
> should happen,
>     this is SOC specific and should be known to mtd layer the
> adreess to read for.
> So, I thought of going this way using t.memory map flag.

Sure, but these things sound like we should be able to support them
without having to bounce over to the SPI thread all the time and...

> >It also seems seems like it's going to involve the CPU being stalled
> >waiting for reads to complete instead of asking the SPI controller to
> >DMA the data to RAM and allowing the CPU to get on with other things -
> >replacing the explicit transmission of commands with memory to memory
> >DMAs might be advantageous but replacing DMA with memcpy() would need
> >numbers to show that it was a win.

...like I say it's not clear to me that this is actually a change that's
going to be beneficial.  I'd really like to see some analysis of the
performance showing that this helps and why it helps.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Peter Korsgaard]

>>>>> "Mark" == Mark Brown <broonie@kernel.org> writes:

Hi,

 Mark> I'm not convinced that this is the most useful API, it sounds like the
 Mark> hardware can "memory map" the entire flash chip so the whole SPI
 Mark> framework seems like overhead.

 Mark> It also seems seems like it's going to involve the CPU being
 Mark> stalled waiting for reads to complete instead of asking the SPI
 Mark> controller to DMA the data to RAM and allowing the CPU to get on
 Mark> with other things - replacing the explicit transmission of
 Mark> commands with memory to memory DMAs might be advantageous but
 Mark> replacing DMA with memcpy() would need numbers to show that it
 Mark> was a win.

Indeed. I can see how such a feature could be useful in E.G. a lowlevel
bootloader (because of simplicity), but am less convinced about it in
Linux where we could conceivable do something else useful while waiting
on the spi controller.

But if there's number to prove otherwise..

-- 
Bye, Peter Korsgaard

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

On Wed, Oct 09, 2013 at 09:01:59PM +0200, Peter Korsgaard wrote:

> Indeed. I can see how such a feature could be useful in E.G. a lowlevel
> bootloader (because of simplicity), but am less convinced about it in
> Linux where we could conceivable do something else useful while waiting
> on the spi controller.

> But if there's number to prove otherwise..

I can see it being useful if there's a DMA controller that can do the
transfer as a memory to memory DMA - the hardware can probably issue
flash read commands a bit faster than the AP can so you should be able
to get closer to saturating the bus.  The patches didn't try to do that
or make it possible for something further up the stack to do it and the
differences should end up being fairly small with optimisations like
those in the pxa2xx or pl022 drivers (which I'm currently working on
pulling out into the framework).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Trent Piepho]

On Wed, Oct 9, 2013 at 12:01 PM, Peter Korsgaard <peter@korsgaard.com> wrote:
>>>>>> "Mark" == Mark Brown <broonie@kernel.org> writes:
>  Mark> I'm not convinced that this is the most useful API, it sounds like the
>  Mark> hardware can "memory map" the entire flash chip so the whole SPI
>  Mark> framework seems like overhead.
>
>  Mark> It also seems seems like it's going to involve the CPU being
>  Mark> stalled waiting for reads to complete instead of asking the SPI
>  Mark> controller to DMA the data to RAM and allowing the CPU to get on
>  Mark> with other things - replacing the explicit transmission of
>  Mark> commands with memory to memory DMAs might be advantageous but
>  Mark> replacing DMA with memcpy() would need numbers to show that it
>  Mark> was a win.
>
> Indeed. I can see how such a feature could be useful in E.G. a lowlevel
> bootloader (because of simplicity), but am less convinced about it in
> Linux where we could conceivable do something else useful while waiting
> on the spi controller.

I've found that the SPI layer adds rather a lot of overhead to SPI
transactions.  It appears to come mostly from using another thread to
run the queue.  A fast SPI message of a few dozen bytes ends up having
more overhead from the SPI layer than the time it takes the driver to
do the actual transfer.

So memory mapped mode via some kind of SPI hack seems like a bad
design.  All the SPI layer overhead and you don't get DMA.  Memory
mapped SPI could be a win, but I think you'd need to do it at the MTD
layer with a mapping driver that could read the mmapped SPI flash
directly.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

Hi All,
On Thursday 10 October 2013 07:57 AM, Trent Piepho wrote:
> On Wed, Oct 9, 2013 at 12:01 PM, Peter Korsgaard<peter@korsgaard.com>  wrote:
>>>>>>> "Mark" == Mark Brown<broonie@kernel.org>  writes:
>>   Mark>  I'm not convinced that this is the most useful API, it sounds like the
>>   Mark>  hardware can "memory map" the entire flash chip so the whole SPI
>>   Mark>  framework seems like overhead.
>>
>>   Mark>  It also seems seems like it's going to involve the CPU being
>>   Mark>  stalled waiting for reads to complete instead of asking the SPI
>>   Mark>  controller to DMA the data to RAM and allowing the CPU to get on
>>   Mark>  with other things - replacing the explicit transmission of
>>   Mark>  commands with memory to memory DMAs might be advantageous but
>>   Mark>  replacing DMA with memcpy() would need numbers to show that it
>>   Mark>  was a win.
>>
>> Indeed. I can see how such a feature could be useful in E.G. a lowlevel
>> bootloader (because of simplicity), but am less convinced about it in
>> Linux where we could conceivable do something else useful while waiting
>> on the spi controller.
> I've found that the SPI layer adds rather a lot of overhead to SPI
> transactions.  It appears to come mostly from using another thread to
> run the queue.  A fast SPI message of a few dozen bytes ends up having
> more overhead from the SPI layer than the time it takes the driver to
> do the actual transfer.
>
> So memory mapped mode via some kind of SPI hack seems like a bad
> design.  All the SPI layer overhead and you don't get DMA.  Memory
> mapped SPI could be a win, but I think you'd need to do it at the MTD
> layer with a mapping driver that could read the mmapped SPI flash
> directly.
Yes, you are correct in all your comments above. I also feel that SPI 
framework
should be bypassed. But the subject patch is derived based on the 
following points/limitation:

1. There is a setup register in QSPI, that need to be filled, as of now 
I am filling it
     in my driver as a MACRO.
2. Controller repsonds to memory mapped read for read opcodes, so during the
read path we should tell the controller to switch to memory mapped port.

[Trent]: With mapping driver, I believe you are hinting at 
drivers/mtd/maps? I had
a look at it and what I got is that it is used/suitable for parallel 
flashes and not the
serial flashes.

All in all, Just at the beginning of the read api, I could have done 
memory mapped read and bypass
the spi framework. But, prior to that above 1, 2 point need to be 
executed and that need to be
communicated to controller driver.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

On Thu, Oct 10, 2013 at 02:22:28PM +0530, Sourav Poddar wrote:

> [Trent]: With mapping driver, I believe you are hinting at
> drivers/mtd/maps? I had
> a look at it and what I got is that it is used/suitable for parallel
> flashes and not the
> serial flashes.

Essentially what it looks like this hardware is trying to do is adapt a
serial flash so it looks more like a parallel flash.  It's not clear
that this is a good idea if we are already able to understand serial
flash though.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

On Thursday 10 October 2013 03:44 PM, Mark Brown wrote:
> On Thu, Oct 10, 2013 at 02:22:28PM +0530, Sourav Poddar wrote:
>
>> [Trent]: With mapping driver, I believe you are hinting at
>> drivers/mtd/maps? I had
>> a look at it and what I got is that it is used/suitable for parallel
>> flashes and not the
>> serial flashes.
> Essentially what it looks like this hardware is trying to do is adapt a
> serial flash so it looks more like a parallel flash.  It's not clear
> that this is a good idea if we are already able to understand serial
> flash though.
hmm..
one more point I want to add is that QSPI controller has
two parts to it:
1. SPI mode (used for SPI based external devices)
2. SFI mode (Serial flash interface) used for flash devices attached.
     Memory mapped mode is the one more applicable to the second one.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

Hi Mark,
On Thursday 10 October 2013 03:44 PM, Mark Brown wrote:
> On Thu, Oct 10, 2013 at 02:22:28PM +0530, Sourav Poddar wrote:
>
>> [Trent]: With mapping driver, I believe you are hinting at
>> drivers/mtd/maps? I had
>> a look at it and what I got is that it is used/suitable for parallel
>> flashes and not the
>> serial flashes.
> Essentially what it looks like this hardware is trying to do is adapt a
> serial flash so it looks more like a parallel flash.  It's not clear
> that this is a good idea if we are already able to understand serial
> flash though.
Do you have any idea of how to go about implementing it in a more
cleaner way?(taking care of all what the spi controller hardware needs to
do for the memory mapped mode.). I understand doing a memcpy in the caller
itself, but how to tackle the spi controller configuration at that point 
of time.
Memory mapped is  a spi controller feature rather than a flash.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

On Thu, Oct 10, 2013 at 04:38:19PM +0530, Sourav Poddar wrote:
> On Thursday 10 October 2013 03:44 PM, Mark Brown wrote:

> >Essentially what it looks like this hardware is trying to do is adapt a
> >serial flash so it looks more like a parallel flash.  It's not clear
> >that this is a good idea if we are already able to understand serial
> >flash though.

> Do you have any idea of how to go about implementing it in a more
> cleaner way?(taking care of all what the spi controller hardware needs to
> do for the memory mapped mode.). I understand doing a memcpy in the caller
> itself, but how to tackle the spi controller configuration at that
> point of time.

You'd need to lock the bus and return the address for the caller to use
until it released the lock.  Like I say I'd want to see numbers on this
helping though.

> Memory mapped is  a spi controller feature rather than a flash.

The way it appears to be implemented is pretty flash specific isn't it?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

Hi Mark,
On Friday 11 October 2013 03:38 PM, Mark Brown wrote:
> On Thu, Oct 10, 2013 at 04:38:19PM +0530, Sourav Poddar wrote:
>> On Thursday 10 October 2013 03:44 PM, Mark Brown wrote:
>>> Essentially what it looks like this hardware is trying to do is adapt a
>>> serial flash so it looks more like a parallel flash.  It's not clear
>>> that this is a good idea if we are already able to understand serial
>>> flash though.
>> Do you have any idea of how to go about implementing it in a more
>> cleaner way?(taking care of all what the spi controller hardware needs to
>> do for the memory mapped mode.). I understand doing a memcpy in the caller
>> itself, but how to tackle the spi controller configuration at that
>> point of time.
> You'd need to lock the bus and return the address for the caller to use
> until it released the lock.  Like I say I'd want to see numbers on this
> helping though.
>
I explored the whole scenario at hand and tried to create something 
based on previous
feedbacks that memcpy should be done only at the flash layer and the entire
spi framework should be bypassed.

But there is one problem which I faced while trying to achieve the 
above. Currently, spi
framework takes care of the runtime clock control part in 
pump_message(spi.c). So, at the
beginning of each transfer, there is a "get_sync" while at the end there 
is a "put_sync". Now, if
I try to do a memcpy in flash and bypass the SPI framework, there is a 
data abort as the SPI
controller clocks are cut.

>> Memory mapped is  a spi controller feature rather than a flash.
> The way it appears to be implemented is pretty flash specific isn't it?

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

On Tue, Oct 15, 2013 at 11:36:47AM +0530, Sourav Poddar wrote:

> But there is one problem which I faced while trying to achieve the
> above. Currently, spi
> framework takes care of the runtime clock control part in
> pump_message(spi.c). So, at the
> beginning of each transfer, there is a "get_sync" while at the end
> there is a "put_sync". Now, if
> I try to do a memcpy in flash and bypass the SPI framework, there is
> a data abort as the SPI
> controller clocks are cut.

Can you fix this by enabling the clock is enabled when you return the
buffer to the MTD layer and then disabling the clock when the buffer is
released?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

Hi Mark,
On Tuesday 15 October 2013 04:46 PM, Mark Brown wrote:
> On Tue, Oct 15, 2013 at 11:36:47AM +0530, Sourav Poddar wrote:
>
>> But there is one problem which I faced while trying to achieve the
>> above. Currently, spi
>> framework takes care of the runtime clock control part in
>> pump_message(spi.c). So, at the
>> beginning of each transfer, there is a "get_sync" while at the end
>> there is a "put_sync". Now, if
>> I try to do a memcpy in flash and bypass the SPI framework, there is
>> a data abort as the SPI
>> controller clocks are cut.
> Can you fix this by enabling the clock is enabled when you return the
> buffer to the MTD layer and then disabling the clock when the buffer is
> released?
Sorry, I did not get you here. With memory mapped read, there is no
buffer exchanged, everything takes place at the mtd layer only, what gets
exchanged is just the memory mapped address.

Here is the patch which I did on top of the subject patch series, wherein my
controller is in default memory mapped mode, while doing a read in mtd layer
its just does a memcopy and return.

This gives me a data abort as pm_runtime_get_sync is not called on the 
qspi controller.


Index: linux/drivers/mtd/devices/m25p80.c
===================================================================
--- linux.orig/drivers/mtd/devices/m25p80.c    2013-10-15 
17:08:15.000000000 +0530
+++ linux/drivers/mtd/devices/m25p80.c    2013-10-15 17:08:26.000000000 
+0530
@@ -102,7 +102,7 @@
      u8            *command;
      bool            fast_read;
      bool            quad_read;
-    bool            mmap_read;
+    void            *mmap_read;
  };

  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
@@ -438,18 +438,21 @@
      pr_debug("%s: %s from 0x%08x, len %zd\n", dev_name(&flash->spi->dev),
              __func__, (u32)from, len);
      printk("enter mtd quad..\n");
+
+    if (flash->mmap_read) {
+        printk("memory map=%x\n", flash->mmap_read);
+        memcpy(buf, flash->mmap_read + from, len);
+        *retlen = len;
+        return 0;
+    }
+
      spi_message_init(&m);
      memset(t, 0, (sizeof(t)));

-    if (flash->mmap_read)
-        t[0].memory_map = 1;
      t[0].tx_buf = flash->command;
-    t[0].len = flash->mmap_read ? from : (m25p_cmdsz(flash) + 
(flash->quad_read ? 1 : 0));
-    printk("t[0].len=%d\n", t[0].len);
+    t[0].len = (m25p_cmdsz(flash) + (flash->quad_read ? 1 : 0));
      spi_message_add_tail(&t[0], &m);

-        if (flash->mmap_read)
-        t[1].memory_map = 1;
      t[1].rx_buf = buf;
      t[1].len = len;
      t[1].rx_nbits = SPI_NBITS_QUAD;
@@ -476,7 +479,7 @@

      spi_sync(flash->spi, &m);

-    *retlen = flash->mmap_read ? len : (m.actual_length - 
m25p_cmdsz(flash) -
+    *retlen = (m.actual_length - m25p_cmdsz(flash) -
              (flash->quad_read ? 1 : 0));

      mutex_unlock(&flash->lock);
@@ -1215,7 +1218,7 @@

      if (spi->mode && SPI_RX_MMAP) {
          printk("memory mapped mode set\n");
-        flash->mmap_read = true;
+        flash->mmap_read = spi->memory_map;
      }
      flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 :
                  (flash->quad_read ? 1 : 0)), GFP_KERNEL);
Index: linux/drivers/spi/spi-ti-qspi.c
===================================================================
--- linux.orig/drivers/spi/spi-ti-qspi.c    2013-10-15 
17:08:15.000000000 +0530
+++ linux/drivers/spi/spi-ti-qspi.c    2013-10-15 17:15:52.000000000 +0530
@@ -256,6 +256,8 @@
              break;
          }
          ti_qspi_write(qspi, memval, QSPI_SPI_SETUP0_REG);
+        enable_qspi_memory_mapped(qspi);
+        spi->memory_map = qspi->mmap_base;
          spi->mode |= SPI_RX_MMAP;
      }

@@ -433,22 +435,13 @@
      qspi->cmd |= QSPI_FLEN(frame_length);
      qspi->cmd |= QSPI_WC_CMD_INT_EN;

+    disable_qspi_memory_mapped(qspi);
+
      ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);

      mutex_lock(&qspi->list_lock);

      list_for_each_entry(t, &m->transfers, transfer_list) {
-               if (t->memory_map) {
-                       if (t->tx_buf) {
-                               from = t->len;
-                               continue;
-                       }
-                       enable_qspi_memory_mapped(qspi);
-                       memcpy(t->rx_buf, qspi->mmap_base + from, t->len);
-                       disable_qspi_memory_mapped(qspi);
-                       goto out;
-               }
-
          qspi->cmd |= QSPI_WLEN(t->bits_per_word);

          ret = qspi_transfer_msg(qspi, t);
@@ -461,13 +454,13 @@
          m->actual_length += t->len;
      }

-out:
      mutex_unlock(&qspi->list_lock);

      m->status = status;
      spi_finalize_current_message(master);

      ti_qspi_write(qspi, qspi->cmd | QSPI_INVAL, QSPI_SPI_CMD_REG);
+    enable_qspi_memory_mapped(qspi);

      return status;
  }
@@ -599,6 +592,7 @@
      if (res_mmap) {
          printk("mmap_available\n");
          qspi->mmap_base = devm_ioremap_resource(&pdev->dev, res_mmap);
+        printk("qspi->mmap_base=%x\n", qspi->mmap_base);
          if (IS_ERR(qspi->mmap_base)) {
              ret = PTR_ERR(qspi->mmap_base);
              goto free_master;
Index: linux/include/linux/spi/spi.h
===================================================================
--- linux.orig/include/linux/spi/spi.h    2013-10-15 17:08:15.000000000 
+0530
+++ linux/include/linux/spi/spi.h    2013-10-15 17:08:26.000000000 +0530
@@ -94,6 +94,7 @@
  #define SPI_RX_MMAP    0x1000            /* Memory mapped read */
      u8            bits_per_word;
      int            irq;
+     void __iomem        *memory_map;
      void            *controller_state;
      void            *controller_data;
      char            modalias[SPI_NAME_SIZE];
@@ -556,7 +557,6 @@
      u16        delay_usecs;
      u32        speed_hz;

-    bool    memory_map;
      struct list_head transfer_list;
  };

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Tue, Oct 15, 2013 at 05:19:07PM +0530, Sourav Poddar wrote:
> On Tuesday 15 October 2013 04:46 PM, Mark Brown wrote:

> >Can you fix this by enabling the clock is enabled when you return the
> >buffer to the MTD layer and then disabling the clock when the buffer is
> >released?

> Sorry, I did not get you here. With memory mapped read, there is no
> buffer exchanged, everything takes place at the mtd layer only, what gets
> exchanged is just the memory mapped address.

The buffer is the memory mapped address - part of getting the address
should be preparing the hardware for it.

>      if (spi->mode && SPI_RX_MMAP) {
>          printk("memory mapped mode set\n");
> -        flash->mmap_read = true;
> +        flash->mmap_read = spi->memory_map;

So this probably needs to be a function call to get the buffer (and a
corresponding one to free it).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

On Tuesday 15 October 2013 06:16 PM, Mark Brown wrote:
> On Tue, Oct 15, 2013 at 05:19:07PM +0530, Sourav Poddar wrote:
>> On Tuesday 15 October 2013 04:46 PM, Mark Brown wrote:
>>> Can you fix this by enabling the clock is enabled when you return the
>>> buffer to the MTD layer and then disabling the clock when the buffer is
>>> released?
>> Sorry, I did not get you here. With memory mapped read, there is no
>> buffer exchanged, everything takes place at the mtd layer only, what gets
>> exchanged is just the memory mapped address.
> The buffer is the memory mapped address - part of getting the address
> should be preparing the hardware for it.
>
>>       if (spi->mode&&  SPI_RX_MMAP) {
>>           printk("memory mapped mode set\n");
>> -        flash->mmap_read = true;
>> +        flash->mmap_read = spi->memory_map;
> So this probably needs to be a function call to get the buffer (and a
> corresponding one to free it).
So, the flow can be something like this:

drivers/mtd/devices/m25p80.c
get_flash_buf()
{
     lock();

      t[0] = GET_BUFFER;
      t[1] = buf;
      ......

      spi_sync();

     unlock();
}

mtd_read
{
     get_flash_buf();

     if (flash->buf) {
         memcpy();
        return 0;
   }
}

Not sure, if free buf is needed as devm_* variant is used to allocate that
memory.


}

RE: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Gupta, Pekon]

> From:  Poddar, Sourav
> > On Tuesday 15 October 2013 06:16 PM, Mark Brown wrote:
> > On Tue, Oct 15, 2013 at 05:19:07PM +0530, Sourav Poddar wrote:
> >> On Tuesday 15 October 2013 04:46 PM, Mark Brown wrote:
> >>> Can you fix this by enabling the clock is enabled when you return the
> >>> buffer to the MTD layer and then disabling the clock when the buffer is
> >>> released?
> >> Sorry, I did not get you here. With memory mapped read, there is no
> >> buffer exchanged, everything takes place at the mtd layer only, what gets
> >> exchanged is just the memory mapped address.
> > The buffer is the memory mapped address - part of getting the address
> > should be preparing the hardware for it.
> >
> >>       if (spi->mode&&  SPI_RX_MMAP) {
> >>           printk("memory mapped mode set\n");
> >> -        flash->mmap_read = true;
> >> +        flash->mmap_read = spi->memory_map;
> > So this probably needs to be a function call to get the buffer (and a
> > corresponding one to free it).
> So, the flow can be something like this:
> 
> drivers/mtd/devices/m25p80.c
> get_flash_buf()
> {
>      lock();
> 
>       t[0] = GET_BUFFER;
>       t[1] = buf;
>       ......
> 
>       spi_sync();
> 
>      unlock();
> }
> 
Problem here..
spi_sync() is not blocking, that means it would just add a spi_message
to queue and return. And it depends on kthread_worker when it pumps
this spi_message to QPSI controller driver for actual configuration.
So this is actually a race-condition. You cannot use spi_sync() to configure.

(refer my comments in previous emails).
If you really want to configure QSPI controller just before memcpy(),
Then you need to somehow prevent spi kthead_worker from accessing.
This you can do by locking the spi_meesage queue/list at that time.


> mtd_read
> {
>      get_flash_buf();
> 
>      if (flash->buf) {
>          memcpy();
>         return 0;
>    }
> }
> 
> Not sure, if free buf is needed as devm_* variant is used to allocate that
> memory.
> 
> 
> }

With regards, pekon

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

On Tue, Oct 15, 2013 at 03:33:19PM +0000, Gupta, Pekon wrote:

> Problem here..
> spi_sync() is not blocking, that means it would just add a spi_message
> to queue and return. And it depends on kthread_worker when it pumps
> this spi_message to QPSI controller driver for actual configuration.
> So this is actually a race-condition. You cannot use spi_sync() to configure.

No, spi_sync() is blocking - spi_async() is non-blocking.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Gupta, Pekon]

> From: Mark Brown [mailto:broonie@kernel.org]
> > On Tue, Oct 15, 2013 at 03:33:19PM +0000, Gupta, Pekon wrote:
> > Problem here..
> > spi_sync() is not blocking, that means it would just add a spi_message
> > to queue and return. And it depends on kthread_worker when it pumps
> > this spi_message to QPSI controller driver for actual configuration.
> > So this is actually a race-condition. You cannot use spi_sync() to configure.
> 
> No, spi_sync() is blocking - spi_async() is non-blocking.

Sorry my bad, I must have been dreaming or something.
Yes, it's spi_sync() is blocking, its clearly written in its comments too..

with regards, pekon

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On Tue, Oct 15, 2013 at 06:53:46PM +0530, Sourav Poddar wrote:

> Not sure, if free buf is needed as devm_* variant is used to allocate that
> memory.

I would expect the buffer to only be requested by the flash driver when
actively in use.  This will enable the SPI controller driver to save
power (by stopping the clock for example as in your case) when the flash
isn't actively being used.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Brian Norris]

Hi Sourav,

On Tue, Oct 15, 2013 at 06:53:46PM +0530, Sourav Poddar wrote:
> On Tuesday 15 October 2013 06:16 PM, Mark Brown wrote:
> >On Tue, Oct 15, 2013 at 05:19:07PM +0530, Sourav Poddar wrote:
> >>On Tuesday 15 October 2013 04:46 PM, Mark Brown wrote:
> >>>Can you fix this by enabling the clock is enabled when you return the
> >>>buffer to the MTD layer and then disabling the clock when the buffer is
> >>>released?
> >>Sorry, I did not get you here. With memory mapped read, there is no
> >>buffer exchanged, everything takes place at the mtd layer only, what gets
> >>exchanged is just the memory mapped address.
> >The buffer is the memory mapped address - part of getting the address
> >should be preparing the hardware for it.
> >
> >>      if (spi->mode&&  SPI_RX_MMAP) {
> >>          printk("memory mapped mode set\n");
> >>-        flash->mmap_read = true;
> >>+        flash->mmap_read = spi->memory_map;
> >So this probably needs to be a function call to get the buffer (and a
> >corresponding one to free it).
> So, the flow can be something like this:
> 
> drivers/mtd/devices/m25p80.c
> get_flash_buf()
> {
>     lock();
> 
>      t[0] = GET_BUFFER;
>      t[1] = buf;
>      ......
> 
>      spi_sync();
> 
>     unlock();
> }
> 
> mtd_read
> {
>     get_flash_buf();
> 
>     if (flash->buf) {
>         memcpy();
>        return 0;
>   }
> }
> 
> Not sure, if free buf is needed as devm_* variant is used to allocate that
> memory.

I believe you are misplacing the discussion of devm_* variants. devm_*
is only useful for resources allocated/mapped released/unmapped at probe
and release time. They do not magically remove the burden of resource
management for I/O and other dynamic operations.

In this case, you are not working at probe time, and you are not
actually allocating any memory--your 'get_flash_buf()' and corresponding
'release_flash_buf()' would not be allocating memory but would be
ensuring that the HW and driver is in the correct state.

Brian

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

Hi Brian,
On Tuesday 15 October 2013 11:31 PM, Brian Norris wrote:
> Hi Sourav,
>
> On Tue, Oct 15, 2013 at 06:53:46PM +0530, Sourav Poddar wrote:
>> On Tuesday 15 October 2013 06:16 PM, Mark Brown wrote:
>>> On Tue, Oct 15, 2013 at 05:19:07PM +0530, Sourav Poddar wrote:
>>>> On Tuesday 15 October 2013 04:46 PM, Mark Brown wrote:
>>>>> Can you fix this by enabling the clock is enabled when you return the
>>>>> buffer to the MTD layer and then disabling the clock when the buffer is
>>>>> released?
>>>> Sorry, I did not get you here. With memory mapped read, there is no
>>>> buffer exchanged, everything takes place at the mtd layer only, what gets
>>>> exchanged is just the memory mapped address.
>>> The buffer is the memory mapped address - part of getting the address
>>> should be preparing the hardware for it.
>>>
>>>>       if (spi->mode&&   SPI_RX_MMAP) {
>>>>           printk("memory mapped mode set\n");
>>>> -        flash->mmap_read = true;
>>>> +        flash->mmap_read = spi->memory_map;
>>> So this probably needs to be a function call to get the buffer (and a
>>> corresponding one to free it).
>> So, the flow can be something like this:
>>
>> drivers/mtd/devices/m25p80.c
>> get_flash_buf()
>> {
>>      lock();
>>
>>       t[0] = GET_BUFFER;
>>       t[1] = buf;
>>       ......
>>
>>       spi_sync();
>>
>>      unlock();
>> }
>>
>> mtd_read
>> {
>>      get_flash_buf();
>>
>>      if (flash->buf) {
>>          memcpy();
>>         return 0;
>>    }
>> }
>>
>> Not sure, if free buf is needed as devm_* variant is used to allocate that
>> memory.
> I believe you are misplacing the discussion of devm_* variants. devm_*
> is only useful for resources allocated/mapped released/unmapped at probe
> and release time. They do not magically remove the burden of resource
> management for I/O and other dynamic operations.
>
> In this case, you are not working at probe time, and you are not
> actually allocating any memory--your 'get_flash_buf()' and corresponding
> 'release_flash_buf()' would not be allocating memory but would be
> ensuring that the HW and driver is in the correct state.
>
> Brian
Thanks for the reply. I got this.

It will be helpful if you can also comment on the thought process till 
now, as you can see from the
series that enabling quad and memory mapped support will effect both the 
SPI controller and
m25p80 side. If the changes proposed on m25p80 looks good to you.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Trent Piepho]

On Tue, Oct 15, 2013 at 11:01 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
>> So, the flow can be something like this:
>>
>> drivers/mtd/devices/m25p80.c
>> get_flash_buf()
>> {
>>     lock();
>>
>>      t[0] = GET_BUFFER;
>>      t[1] = buf;
>>      ......
>>
>>      spi_sync();
>>
>>     unlock();
>> }
>>
>> mtd_read
>> {
>>     get_flash_buf();
>>
>>     if (flash->buf) {
>>         memcpy();
>>        return 0;
>>   }
>> }
>>
>> Not sure, if free buf is needed as devm_* variant is used to allocate that
>> memory.
>
> I believe you are misplacing the discussion of devm_* variants. devm_*
> is only useful for resources allocated/mapped released/unmapped at probe
> and release time. They do not magically remove the burden of resource
> management for I/O and other dynamic operations.

Are there any numbers to show if memory mapped read support is a
benefit in Linux?  There is some question as to whether it's useful at
all or not.

If it is, I think low latency for small reads is probably one of the
only advantages.  To do that, you aren't going to want to deal with
device PM for every single read.  It would make more sense to turn the
hardware on when the MTD device is opened and leave it on until
closed.

RE: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Gupta, Pekon]

> From: Trent Piepho
> Are there any numbers to show if memory mapped read support is a
> benefit in Linux?  There is some question as to whether it's useful at
> all or not.
> 
> If it is, I think low latency for small reads is probably one of the
> only advantages.  To do that, you aren't going to want to deal with
> device PM for every single read.  It would make more sense to turn the
> hardware on when the MTD device is opened and leave it on until
> closed.
> 
+1

Therefore early suggestions were to make 'MM_MODE' as default
(if device enables it via DT). This means:
(1) switch to 'SPI_MODE' _only_ when required for commands like
 mtd_erase, etc. and switch back to 'MM_MODE' when done.
(2) And keep your controller clocks on.

This would ensure that you do minimum config-switching when using
MM_MODE. And would thus achieve low latency, and no driver intervention.

Yes, real thruput numbers would help clear the picture here ..

with regards, pekon

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Tue, Oct 15, 2013 at 06:33:23PM +0000, Gupta, Pekon wrote:

> Therefore early suggestions were to make 'MM_MODE' as default
> (if device enables it via DT). This means:
> (1) switch to 'SPI_MODE' _only_ when required for commands like
>  mtd_erase, etc. and switch back to 'MM_MODE' when done.
> (2) And keep your controller clocks on.

This sounds like a policy decision, I don't see any reason for it to be
in DT.  What works well with one application stack may not be the best
choice for another and future developments may change what's most
sensible for a given system, it shouldn't be fixed in the DT.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Trent Piepho]

On Tue, Oct 15, 2013 at 1:52 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Oct 15, 2013 at 06:33:23PM +0000, Gupta, Pekon wrote:
>
>> Therefore early suggestions were to make 'MM_MODE' as default
>> (if device enables it via DT). This means:
>> (1) switch to 'SPI_MODE' _only_ when required for commands like
>>  mtd_erase, etc. and switch back to 'MM_MODE' when done.
>> (2) And keep your controller clocks on.
>
> This sounds like a policy decision, I don't see any reason for it to be
> in DT.  What works well with one application stack may not be the best
> choice for another and future developments may change what's most
> sensible for a given system, it shouldn't be fixed in the DT.

I could see the driver using the transfer size to decide, memory
mapped or SPI with DMA.  Small transfers via memory mapped access and
larger would use DMA.  The spi-mxs driver does this to decide PIO or
DMA.  The threshold size is hard coded in the driver.  If you wanted
to be able to change it, I would think a sysfs attribute would be the
way to do that.

It does get tricky when you're dealing with flash roms, as people
often want to boot from those really fast, so you want the kernel to
know how to use them in the fastest way before it boots as opposed to
after booting when it's obviously too late to configure the kernel to
boot faster.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

On Tue, Oct 15, 2013 at 02:03:01PM -0700, Trent Piepho wrote:

> It does get tricky when you're dealing with flash roms, as people
> often want to boot from those really fast, so you want the kernel to
> know how to use them in the fastest way before it boots as opposed to
> after booting when it's obviously too late to configure the kernel to
> boot faster.

I'd guess that having fast as the default would be sane enough, though.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

Hi All,
On Wednesday 16 October 2013 12:03 AM, Gupta, Pekon wrote:
>> From: Trent Piepho
>> Are there any numbers to show if memory mapped read support is a
>> benefit in Linux?  There is some question as to whether it's useful at
>> all or not.
>>
>> If it is, I think low latency for small reads is probably one of the
>> only advantages.  To do that, you aren't going to want to deal with
>> device PM for every single read.  It would make more sense to turn the
>> hardware on when the MTD device is opened and leave it on until
>> closed.
>>
> +1
>
> Therefore early suggestions were to make 'MM_MODE' as default
> (if device enables it via DT). This means:
> (1) switch to 'SPI_MODE' _only_ when required for commands like
>   mtd_erase, etc. and switch back to 'MM_MODE' when done.
> (2) And keep your controller clocks on.
>
> This would ensure that you do minimum config-switching when using
> MM_MODE. And would thus achieve low latency, and no driver intervention.
>
> Yes, real thruput numbers would help clear the picture here ..
>
> with regards, pekon
I did some throughput measurement to get some number on the
read side. Here are my observations:

Case1:
--------
  Using SPI framework.

   Setup:
     Here, the actual memcpy is done in the spi controller, and flash
     communicates to the qspi controller to do the memcpy using the
     SPI framework. This is what is propsed in the $subject patch.

   Measurement method:
     used jiffies_to_msecs  at the beginning and at the end of the 
mtd_read api
    and calculated the difference.

   Result:
      Tried a transfer of 32KB, which takes around 20 ms of time to read.


Case2:
--------
  Bypassing SPI framework.

   Setup:
     Here, the actual memcpy is done in the mtd read api itself, by 
getting the
     memmap address from the spi controller.

   Measurement method:
     used jiffies_to_msecs  before and after memcpy and calculated the 
difference.

   Result:
      Tried a transfer of 32KB, which takes around 10 ms of time to read.

So, time reduced almost to half while bypassing the SPI framework.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On Thu, Oct 17, 2013 at 05:54:53PM +0530, Sourav Poddar wrote:

>   Setup:
>     Here, the actual memcpy is done in the spi controller, and flash
>     communicates to the qspi controller to do the memcpy using the
>     SPI framework. This is what is propsed in the $subject patch.

>   Setup:
>     Here, the actual memcpy is done in the mtd read api itself, by
> getting the
>     memmap address from the spi controller.

> So, time reduced almost to half while bypassing the SPI framework.

The interesting case for benchmarking here is more a comparison between
normal DMA driven transfers and the memcpy().  Some consideration of the
CPU load would also be interesting here, if the SoC is waiting for the
flash then it's probably useful if it can make progress on other things.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Gupta, Pekon]

Hi Mark,

> From: Mark Brown [mailto:broonie@kernel.org]
> > On Thu, Oct 17, 2013 at 05:54:53PM +0530, Sourav Poddar wrote:
> >   Setup:
> >     Here, the actual memcpy is done in the spi controller, and flash
> >     communicates to the qspi controller to do the memcpy using the
> >     SPI framework. This is what is propsed in the $subject patch.
> 
> >   Setup:
> >     Here, the actual memcpy is done in the mtd read api itself, by
> > getting the
> >     memmap address from the spi controller.
> 
> > So, time reduced almost to half while bypassing the SPI framework.
> 
> The interesting case for benchmarking here is more a comparison between
> normal DMA driven transfers and the memcpy().  Some consideration of the
> CPU load would also be interesting here, if the SoC is waiting for the
> flash then it's probably useful if it can make progress on other things.
> 
So in this CASE-2:  SPI framework is bypassed: mtd_read() becomes
mtd_read() {
	if (flash->mmap_mode)
		if (dma_available)
			read_via_dma(destination, source, length);
		else
			memcpy(destination, source, length);
	else
		/* use spi frame-work by default */
}

Are you looking for comparison between  read_via_dma() v/s memcpy() ?

If yes, then unfortunately we are bit constrained because our controller
does not support DMA. So, we have to depend on CPU based memcpy()
only. However, use of DMA can be added as an independent patch on
top of this CASE-2 patch.

So will the base patch for CASE-2  (with SPI framework is bypassed) help ?


with regards, pekon

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On Thu, Oct 17, 2013 at 01:03:26PM +0000, Gupta, Pekon wrote:

> mtd_read() {
> 	if (flash->mmap_mode)
> 		if (dma_available)
> 			read_via_dma(destination, source, length);
> 		else
> 			memcpy(destination, source, length);
> 	else
> 		/* use spi frame-work by default */
> }

> Are you looking for comparison between  read_via_dma() v/s memcpy() ?

No, I'm looking for a comparison of normal SPI mode (which I'd have
expected to DMA) and the memcpy() mode.

> If yes, then unfortunately we are bit constrained because our controller
> does not support DMA. So, we have to depend on CPU based memcpy()
> only. However, use of DMA can be added as an independent patch on
> top of this CASE-2 patch.

However if the controller can't DMA at all then that's not going to be
possible...  am I understanding you correctly that normal SPI can't DMA?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

On Friday 18 October 2013 05:12 AM, Mark Brown wrote:
> On Thu, Oct 17, 2013 at 01:03:26PM +0000, Gupta, Pekon wrote:
>
>> mtd_read() {
>> 	if (flash->mmap_mode)
>> 		if (dma_available)
>> 			read_via_dma(destination, source, length);
>> 		else
>> 			memcpy(destination, source, length);
>> 	else
>> 		/* use spi frame-work by default */
>> }
>> Are you looking for comparison between  read_via_dma() v/s memcpy() ?
> No, I'm looking for a comparison of normal SPI mode (which I'd have
> expected to DMA) and the memcpy() mode.
>
>> If yes, then unfortunately we are bit constrained because our controller
>> does not support DMA. So, we have to depend on CPU based memcpy()
>> only. However, use of DMA can be added as an independent patch on
>> top of this CASE-2 patch.
> However if the controller can't DMA at all then that's not going to be
> possible...  am I understanding you correctly that normal SPI can't DMA?
Yes, you are correct, the normal SPI cant DMA.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Trent Piepho]

On Thu, Oct 17, 2013 at 9:06 PM, Sourav Poddar <sourav.poddar@ti.com> wrote:
> On Friday 18 October 2013 05:12 AM, Mark Brown wrote:
>>> Are you looking for comparison between  read_via_dma() v/s memcpy() ?
>>
>> No, I'm looking for a comparison of normal SPI mode (which I'd have
>> expected to DMA) and the memcpy() mode.
>>
>>> If yes, then unfortunately we are bit constrained because our controller
>>> does not support DMA. So, we have to depend on CPU based memcpy()
>>> only. However, use of DMA can be added as an independent patch on
>>> top of this CASE-2 patch.
>>
>> However if the controller can't DMA at all then that's not going to be
>> possible...  am I understanding you correctly that normal SPI can't DMA?
>
> Yes, you are correct, the normal SPI cant DMA.

Hardware limitation or driver limitation?  Adding DMA support to the
driver might be much more useful than adding memory mapped read
support.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

On Friday 18 October 2013 11:26 AM, Trent Piepho wrote:
> On Thu, Oct 17, 2013 at 9:06 PM, Sourav Poddar<sourav.poddar@ti.com>  wrote:
>> On Friday 18 October 2013 05:12 AM, Mark Brown wrote:
>>>> Are you looking for comparison between  read_via_dma() v/s memcpy() ?
>>> No, I'm looking for a comparison of normal SPI mode (which I'd have
>>> expected to DMA) and the memcpy() mode.
>>>
>>>> If yes, then unfortunately we are bit constrained because our controller
>>>> does not support DMA. So, we have to depend on CPU based memcpy()
>>>> only. However, use of DMA can be added as an independent patch on
>>>> top of this CASE-2 patch.
>>> However if the controller can't DMA at all then that's not going to be
>>> possible...  am I understanding you correctly that normal SPI can't DMA?
>> Yes, you are correct, the normal SPI cant DMA.
> Hardware limitation or driver limitation?  Adding DMA support to the
> driver might be much more useful than adding memory mapped read
> support.
Its a hardware limitation, the qspi controller does not support DMA.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

Hi,
On Friday 18 October 2013 11:40 AM, Sourav Poddar wrote:
> On Friday 18 October 2013 11:26 AM, Trent Piepho wrote:
>> On Thu, Oct 17, 2013 at 9:06 PM, Sourav Poddar<sourav.poddar@ti.com>  
>> wrote:
>>> On Friday 18 October 2013 05:12 AM, Mark Brown wrote:
>>>>> Are you looking for comparison between  read_via_dma() v/s memcpy() ?
>>>> No, I'm looking for a comparison of normal SPI mode (which I'd have
>>>> expected to DMA) and the memcpy() mode.
>>>>
>>>>> If yes, then unfortunately we are bit constrained because our 
>>>>> controller
>>>>> does not support DMA. So, we have to depend on CPU based memcpy()
>>>>> only. However, use of DMA can be added as an independent patch on
>>>>> top of this CASE-2 patch.
>>>> However if the controller can't DMA at all then that's not going to be
>>>> possible...  am I understanding you correctly that normal SPI can't 
>>>> DMA?
>>> Yes, you are correct, the normal SPI cant DMA.
>> Hardware limitation or driver limitation?  Adding DMA support to the
>> driver might be much more useful than adding memory mapped read
>> support.
> Its a hardware limitation, the qspi controller does not support DMA.
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
So, consolidating all the comments, this is what can be done to improve the
$subject patch..

1. Keep the qspi controller by default in memory mapped mode.

2. While doing operations other than memory mapped, change the
     controller into config mode and at the end change it into memory mapped
     mode.

3. For filling memory mapped register in qspi controller, we can pass 
that information
     from dt rather than hardcoding as macros.

4. For memory mapped read, do the memcpy in the mtd_read itself and 
bypass the
     SPI framework.
      This has two points to be considered -

     4a.
         Roadblock:
          To get the mem buf from the spi controller to mtd layer
          Solution:
            As suggested by Mark, we can define some apis like
              get_buf/free_buf  in mtd driver and use that to get the
              buf.

     4b.
         Roadblock:
         Runtime clock management is handle by SPI framework, so while
         doing memory read, where we bypass SPI framework, clocks will be
         disable and we will get an abort while doing memcpy.

        Possible solution:
            As suggested by Trent, we can keep the SPI controller clocks 
always ON ?


If the above proposal looks good, should I go ahead and post the next 
updated
patch series?

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

On Fri, Oct 18, 2013 at 12:57:51PM +0530, Sourav Poddar wrote:

> 3. For filling memory mapped register in qspi controller, we can
> pass that information
>     from dt rather than hardcoding as macros.

Or from the flash driver at runtime...

>     4b.
>         Roadblock:
>         Runtime clock management is handle by SPI framework, so while
>         doing memory read, where we bypass SPI framework, clocks will be
>         disable and we will get an abort while doing memcpy.

>        Possible solution:
>            As suggested by Trent, we can keep the SPI controller
> clocks always ON ?

At the SPI layer I would keep the clocks on while the driver has a
region mapped for the flash layer.  The flash layer can then decide when
to keep the region mapped, for example it could do so whenever the
device is opened.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

On Friday 18 October 2013 04:01 PM, Mark Brown wrote:
> On Fri, Oct 18, 2013 at 12:57:51PM +0530, Sourav Poddar wrote:
>
>> 3. For filling memory mapped register in qspi controller, we can
>> pass that information
>>      from dt rather than hardcoding as macros.
> Or from the flash driver at runtime...
>
No, as for now, its better to keep mtd framework independent of this
spi controller configuration and pass the information via dt.

Since we dont have a common protocol, If we start putting this information
in mtd, it will clutter the mtd space and it may happen that we may end up
with differnet if defs for all the kind of flash devices.
>>      4b.
>>          Roadblock:
>>          Runtime clock management is handle by SPI framework, so while
>>          doing memory read, where we bypass SPI framework, clocks will be
>>          disable and we will get an abort while doing memcpy.
>>         Possible solution:
>>             As suggested by Trent, we can keep the SPI controller
>> clocks always ON ?
> At the SPI layer I would keep the clocks on while the driver has a
> region mapped for the flash layer.  The flash layer can then decide when
> to keep the region mapped, for example it could do so whenever the
> device is opened.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

On Fri, Oct 18, 2013 at 05:18:15PM +0530, Sourav Poddar wrote:
> On Friday 18 October 2013 04:01 PM, Mark Brown wrote:
> >On Fri, Oct 18, 2013 at 12:57:51PM +0530, Sourav Poddar wrote:

> >>3. For filling memory mapped register in qspi controller, we can
> >>pass that information
> >>     from dt rather than hardcoding as macros.

> >Or from the flash driver at runtime...

> No, as for now, its better to keep mtd framework independent of this
> spi controller configuration and pass the information via dt.

> Since we dont have a common protocol, If we start putting this information
> in mtd, it will clutter the mtd space and it may happen that we may end up
> with differnet if defs for all the kind of flash devices.

I thought this was the information for the flash commands that the
driver would need to know anyway for use with SPI controllers without
explicit flash support?  But perhaps I'm misunderstanding.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Sourav Poddar]

On Friday 18 October 2013 06:38 PM, Mark Brown wrote:
> On Fri, Oct 18, 2013 at 05:18:15PM +0530, Sourav Poddar wrote:
>> On Friday 18 October 2013 04:01 PM, Mark Brown wrote:
>>> On Fri, Oct 18, 2013 at 12:57:51PM +0530, Sourav Poddar wrote:
>>>> 3. For filling memory mapped register in qspi controller, we can
>>>> pass that information
>>>>      from dt rather than hardcoding as macros.
>>> Or from the flash driver at runtime...
>> No, as for now, its better to keep mtd framework independent of this
>> spi controller configuration and pass the information via dt.
>> Since we dont have a common protocol, If we start putting this information
>> in mtd, it will clutter the mtd space and it may happen that we may end up
>> with differnet if defs for all the kind of flash devices.
> I thought this was the information for the flash commands that the
> driver would need to know anyway for use with SPI controllers without
> explicit flash support?  But perhaps I'm misunderstanding.
Flash driver need to know them, but as of now, everything is done in a 
api mode
stuff in flash driver. As in, we have write/erase/read, quad_read 
(proposed through this
series) where we provide the commands directly. And to handle different 
flash types, we
use macros to decide which command set is used.

May be, this can come as a part of a seperate discussion to modify flash 
driver also to
take data(commands, other parameters) through dt.

Though, for the case in hand,  better to set up spi mem map specific 
registers with dts from
spi controller node itself.  There might be different varieties of 
flashes, with different commands
and cluttering mtd flash to configure a controller specific register 
does not look to be a good idea.

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

On Tue, Oct 15, 2013 at 11:13:38AM -0700, Trent Piepho wrote:

> If it is, I think low latency for small reads is probably one of the
> only advantages.  To do that, you aren't going to want to deal with
> device PM for every single read.  It would make more sense to turn the
> hardware on when the MTD device is opened and leave it on until
> closed.

Autosuspend *might* be efficient enough.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Gupta, Pekon]

Hi,

> From: Poddar, Sourav
> > On Thursday 10 October 2013 07:57 AM, Trent Piepho wrote:
> > I've found that the SPI layer adds rather a lot of overhead to SPI
> > transactions.  It appears to come mostly from using another thread to
> > run the queue.  A fast SPI message of a few dozen bytes ends up having
> > more overhead from the SPI layer than the time it takes the driver to
> > do the actual transfer.
> >
> > So memory mapped mode via some kind of SPI hack seems like a bad
> > design.  All the SPI layer overhead and you don't get DMA.  Memory
> > mapped SPI could be a win, but I think you'd need to do it at the MTD
> > layer with a mapping driver that could read the mmapped SPI flash
> > directly.
> Yes, you are correct in all your comments above. I also feel that SPI
> framework
> should be bypassed. But the subject patch is derived based on the
> following points/limitation:
> 
> 1. There is a setup register in QSPI, that need to be filled, as of now
> I am filling it
>      in my driver as a MACRO.
> 
Based on you previous information of set_up_register..
> > What is "set_up_register"?
> > set_up_register is a memory mapped specific register with the
> > following fields:
> > - RCMD (Read command, which is actually the flash read command)
> > - NUmber of Address bytes
> > - Number of dummy bytes
> > - Read type(Single, dual and quad read).
> > - Write command.

set_up_register should be filled from DT not from Macros, as these
value change from NAND device to device, and based on that
populate 'struct m25p' in m25p_probe().Otherwise it might end-up
in similar situation as in fsl_spinor driver.
Refer below as an example.
http://lists.infradead.org/pipermail/linux-mtd/2013-September/048552.html



> 2. Controller repsonds to memory mapped read for read opcodes,
>  so during the read path we should tell the controller to switch to
>  memory mapped port.
> 
As you would be know from DT when to enable MMODE, so you  can
actually make MM_MODE as your *default mode*. And only when
SPI framework is used you selectively switch to SPI_MODE and
revert back to MM_MODE while returning.

This way you can use memcpy() or dma_copy() directly in flash driver
like m25p80.c, and avoid getting SPI generic framework delays.
So, it should be like this..

/* Make MM_MODE your default mode in controller driver */
qspi_driver.c: configure_qspi_controller(mode) {
if (mode == MM_MODE)
	- do all your controller configurations for MM_MODE
	- do all OPCODE selections for MM_MODE
}

qspi_controller_probe()
	if (of_mode_property("spi-flash-memory-map") {
		spi->mode |= MM_MODE;
		/* configure_qspi_controller (MM_MODE) */
	} else {
		/* configure_qspi_controller(SPI_MODE) */
	}


/* Case 1: MM_MODE=enabled: Flash driver calls memcpy() */
m25p80.c: m25p80_quad_read() {
	if (flash->mmap_read)
		/* controller is already in MM_MODE by default */
		memcpy(*from, *to, length);
	else
		/* usual call to spi_framework */
}


/* Case 2: SPI_MODE=enabled: Flash driver follows spi framework */
m25p80.c: m25p80_quad_read() {
	if (flash->mmap_read)
		/* ignored */
	else
		spi_add_message_to_tail();
}	

qspi_driver.c: prepare_transfer_hardware()
	if (spi->mode & MM_MODE) {
		/* controller is in MM_MODE by default, so switch
		  * to controller to SPI_MODE */
		configure_qspi_controller (SPI_MODE);
	} else {
		/* controller is already in SPI_MODE always */
	}

qspi_driver.c: transfer_one_message ()
	if (spi->mode & MM_MODE) {
		/* controller was switched to SPI_MODE in
		* prepare_transfer_hardware(),so revert back
		  * back to MM_MODE */
		configure_qspi_controller (MM_MODE);
	} else {
		/* controller is already in SPI_MODE always*/
	}
}

*Important*
But you need to be careful, because  you need to synchronize
with kthread_worker running inside SPI generic framework.
So, lock your spi_controller while doing MMODE accesses.
else you might end up in situation where a starving kthead_worker
suddenly woke-up  and changed your configurations from MMODE to
SPI_MODE in between ongoing memcpy() or dma_cpy().

(Request David W, and Mark B to please review this proposal
 based on m25p80.c and SPI generic framework, as I may be
 missing some understanding here).


with regards, pekon

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]

On Wed, Oct 09, 2013 at 07:27:11PM -0700, Trent Piepho wrote:

> I've found that the SPI layer adds rather a lot of overhead to SPI
> transactions.  It appears to come mostly from using another thread to
> run the queue.  A fast SPI message of a few dozen bytes ends up having
> more overhead from the SPI layer than the time it takes the driver to
> do the actual transfer.

Yeah, though of course at the minute the implementation of that thread
is pretty much up to the individual drivers which isn't triumph - and
the quality of implementation does vary rather a lot.  I'm currently
working on trying to factor more of this out, hopefully then it'll be
easier to push out improvements.  It may be nice to be able to kick off
the first DMA transfer from within the caller for example.

> So memory mapped mode via some kind of SPI hack seems like a bad
> design.  All the SPI layer overhead and you don't get DMA.  Memory
> mapped SPI could be a win, but I think you'd need to do it at the MTD
> layer with a mapping driver that could read the mmapped SPI flash
> directly.

Yes, exactly and even then I'm not convinced that it's going to be much
advantage for anything except small transfers without DMA.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] spi/qspi: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

On Thu, Oct 10, 2013 at 04:53:46PM -0700, Trent Piepho wrote:
> On Thu, Oct 10, 2013 at 3:10 AM, Mark Brown <broonie@kernel.org> wrote:

> > Yeah, though of course at the minute the implementation of that thread
> > is pretty much up to the individual drivers which isn't triumph - and
> > the quality of implementation does vary rather a lot. I'm currently
> > working on trying to factor more of this out, hopefully then it'll be
> > easier to push out improvements. It may be nice to be able to kick off
> > the first DMA transfer from within the caller for example.

> I did testing with the mxs driver, which uses transfer_one
> _message and the spi core queue pumping code.  For small messages the
> overhead of queuing work to the pump_messages queue and waiting for
> completion is rather more than the time the actual transfer takes.  Which
> makes using a kthread rather pointless.  Part of the problem could be the
> high context switch cost for ARMv5.

Indeed - that's what I'd expect to happen.  Sufficiently small PIO
transfers and DMA initiation should both be faster if done from the
calling context.  The thread starts to get more useful once you start
building up work (at which point it should reduce context switches,
especially if multiple threads are using the bus) and if the system is
very heavily loaded when you can raise the thread priority so it can
drive data in more quickly.  

That said it's also helpful in terms of making the code simpler to just
have the one data path.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCHv3 2/3] drivers: mtd: devices: Add quad read support.

[Sourav Poddar]

Some flash also support quad read mode.
Adding support for adding quad mode in m25p80
for spansion and macronix flash.

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
v2->v3:
Add macronix flash support
 drivers/mtd/devices/m25p80.c |  184 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 176 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 26b14f9..dc9bcbf 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -41,6 +41,7 @@
 #define	OPCODE_WRSR		0x01	/* Write status register 1 byte */
 #define	OPCODE_NORM_READ	0x03	/* Read data bytes (low frequency) */
 #define	OPCODE_FAST_READ	0x0b	/* Read data bytes (high frequency) */
+#define	OPCODE_QUAD_READ        0x6b    /* QUAD READ */
 #define	OPCODE_PP		0x02	/* Page program (up to 256 bytes) */
 #define	OPCODE_BE_4K		0x20	/* Erase 4KiB block */
 #define	OPCODE_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
@@ -48,6 +49,7 @@
 #define	OPCODE_CHIP_ERASE	0xc7	/* Erase whole flash chip */
 #define	OPCODE_SE		0xd8	/* Sector erase (usually 64KiB) */
 #define	OPCODE_RDID		0x9f	/* Read JEDEC ID */
+#define	OPCODE_RDCR		0x35    /* Read configuration register */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
 #define	OPCODE_NORM_READ_4B	0x13	/* Read data bytes (low frequency) */
@@ -76,6 +78,10 @@
 #define	SR_BP2			0x10	/* Block protect 2 */
 #define	SR_SRWD			0x80	/* SR write protect */
 
+/* Configuration Register bits. */
+#define SPAN_QUAD_CR_EN		0x2	/* Spansion Quad I/O */
+#define MACR_QUAD_SR_EN		0x40	/* Macronix Quad I/O */
+
 /* Define max times to check status register before we give up. */
 #define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max chip erase */
 #define	MAX_CMD_SIZE		5
@@ -95,6 +101,7 @@ struct m25p {
 	u8			program_opcode;
 	u8			*command;
 	bool			fast_read;
+	bool			quad_read;
 };
 
 static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
@@ -163,6 +170,25 @@ static inline int write_disable(struct m25p *flash)
 	return spi_write_then_read(flash->spi, &code, 1, NULL, 0);
 }
 
+/* Read the configuration register, returning its value in the location
+ * Return the configuration register value.
+ * Returns negative if error occurred.
+*/
+static int read_cr(struct m25p *flash)
+{
+	u8 code = OPCODE_RDCR;
+	int ret;
+	u8 val;
+
+	ret = spi_write_then_read(flash->spi, &code, 1, &val, 1);
+	if (ret < 0) {
+		dev_err(&flash->spi->dev, "error %d reading CR\n", ret);
+		return ret;
+	}
+
+	return val;
+}
+
 /*
  * Enable/disable 4-byte addressing mode.
  */
@@ -336,6 +362,122 @@ static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
 	return 0;
 }
 
+/* Write status register and configuration register with 2 bytes
+* The first byte will be written to the status register, while the second byte
+* will be written to the configuration register.
+* Returns negative if error occurred.
+*/
+static int write_sr_cr(struct m25p *flash, u16 val)
+{
+	flash->command[0] = OPCODE_WRSR;
+	flash->command[1] = val & 0xff;
+	flash->command[2] = (val >> 8);
+
+	return spi_write(flash->spi, flash->command, 3);
+}
+
+static int macronix_quad_enable(struct m25p *flash)
+{
+	int ret, val;
+	u8 cmd[2];
+	cmd[0] = OPCODE_WRSR;
+
+	val = read_sr(flash);
+	cmd[1] = val | MACR_QUAD_SR_EN;
+	write_enable(flash);
+
+	spi_write(flash->spi, &cmd, 2);
+
+	if (wait_till_ready(flash))
+		return 1;
+
+	ret = read_sr(flash);
+	if (!(ret > 0 && (ret & MACR_QUAD_SR_EN))) {
+		dev_err(&flash->spi->dev,
+			"Macronix Quad bit not set");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int spansion_quad_enable(struct m25p *flash)
+{
+	int ret;
+	int quad_en = SPAN_QUAD_CR_EN << 8;
+
+	write_enable(flash);
+
+	ret = write_sr_cr(flash, quad_en);
+	if (ret < 0) {
+		dev_err(&flash->spi->dev,
+			"error while writing configuration register");
+		return -EINVAL;
+	}
+
+	/* read back and check it */
+	ret = read_cr(flash);
+	if (!(ret > 0 && (ret & SPAN_QUAD_CR_EN))) {
+		dev_err(&flash->spi->dev,
+			"Spansion Quad bit not set");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int m25p80_quad_read(struct mtd_info *mtd, loff_t from, size_t len,
+	size_t *retlen, u_char *buf)
+{
+	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);
+
+	spi_message_init(&m);
+	memset(t, 0, (sizeof(t)));
+
+	t[0].tx_buf = flash->command;
+	t[0].len = m25p_cmdsz(flash) + (flash->quad_read ? 1 : 0);
+	spi_message_add_tail(&t[0], &m);
+
+	t[1].rx_buf = buf;
+	t[1].len = len;
+	t[1].rx_nbits = SPI_NBITS_QUAD;
+	spi_message_add_tail(&t[1], &m);
+
+	mutex_lock(&flash->lock);
+
+	/* Wait till previous write/erase is done. */
+	if (wait_till_ready(flash)) {
+		/* REVISIT status return?? */
+		mutex_unlock(&flash->lock);
+		return 1;
+	}
+
+	/* FIXME switch to OPCODE_QUAD_READ.  It's required for higher
+	 * clocks; and at this writing, every chip this driver handles
+	 * supports that opcode.
+	*/
+
+	/* Set up the write data buffer. */
+	opcode = flash->read_opcode;
+	flash->command[0] = opcode;
+	m25p_addr2cmd(flash, from, flash->command);
+
+	spi_sync(flash->spi, &m);
+
+	*retlen = m.actual_length - m25p_cmdsz(flash) -
+			(flash->quad_read ? 1 : 0);
+
+	mutex_unlock(&flash->lock);
+
+	return 0;
+}
+
 /*
  * Read an address range from the flash chip.  The address range
  * may be any size provided it is within the physical boundaries.
@@ -928,6 +1070,7 @@ static int m25p_probe(struct spi_device *spi)
 	unsigned			i;
 	struct mtd_part_parser_data	ppdata;
 	struct device_node __maybe_unused *np = spi->dev.of_node;
+	int ret;
 
 #ifdef CONFIG_MTD_OF_PARTS
 	if (!of_device_is_available(np))
@@ -979,15 +1122,9 @@ static int m25p_probe(struct spi_device *spi)
 		}
 	}
 
-	flash = kzalloc(sizeof *flash, GFP_KERNEL);
+	flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL);
 	if (!flash)
 		return -ENOMEM;
-	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 : 0),
-					GFP_KERNEL);
-	if (!flash->command) {
-		kfree(flash);
-		return -ENOMEM;
-	}
 
 	flash->spi = spi;
 	mutex_init(&flash->lock);
@@ -1015,7 +1152,6 @@ static int m25p_probe(struct spi_device *spi)
 	flash->mtd.flags = MTD_CAP_NORFLASH;
 	flash->mtd.size = info->sector_size * info->n_sectors;
 	flash->mtd._erase = m25p80_erase;
-	flash->mtd._read = m25p80_read;
 
 	/* flash protection support for STmicro chips */
 	if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {
@@ -1067,6 +1203,38 @@ static int m25p_probe(struct spi_device *spi)
 
 	flash->program_opcode = OPCODE_PP;
 
+	flash->quad_read = false;
+	if (spi->mode && SPI_RX_QUAD)
+		flash->quad_read = true;
+
+	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 :
+				(flash->quad_read ? 1 : 0)), GFP_KERNEL);
+	if (!flash->command) {
+		kfree(flash);
+		return -ENOMEM;
+	}
+
+	if (flash->quad_read) {
+		if (of_property_read_bool(np, "macronix,quad_enable")) {
+			ret = macronix_quad_enable(flash);
+			if (ret) {
+				dev_err(&spi->dev,
+					"error enabling quad");
+				return -EINVAL;
+			}
+		} else if (of_property_read_bool(np, "spansion,quad_enable")) {
+			ret = spansion_quad_enable(flash);
+			if (ret) {
+				dev_err(&spi->dev,
+					"error enabling quad");
+				return -EINVAL;
+			}
+		} else
+			dev_dbg(&spi->dev, "quad enable not supported");
+		flash->mtd._read = m25p80_quad_read;
+	} else
+		flash->mtd._read = m25p80_read;
+
 	if (info->addr_width)
 		flash->addr_width = info->addr_width;
 	else if (flash->mtd.size > 0x1000000) {
-- 
1.7.1

Re: [PATCHv3 2/3] drivers: mtd: devices: Add quad read support.

[Jagan Teki]

On Wed, Oct 9, 2013 at 8:54 PM, Sourav Poddar <sourav.poddar@ti.com> wrote:
> Some flash also support quad read mode.
> Adding support for adding quad mode in m25p80
> for spansion and macronix flash.

I am some how not happy with this approach, adding a flags on flash
param table for supporting
quad operation in specific flash is may be a better idea.

But reading SFDP, is a good idea to get the details of flash - quad
support is one of them.
and we can assign quad read or fast read based on that.

Any comments.

>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
> v2->v3:
> Add macronix flash support
>  drivers/mtd/devices/m25p80.c |  184 ++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 176 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 26b14f9..dc9bcbf 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -41,6 +41,7 @@
>  #define        OPCODE_WRSR             0x01    /* Write status register 1 byte */
>  #define        OPCODE_NORM_READ        0x03    /* Read data bytes (low frequency) */
>  #define        OPCODE_FAST_READ        0x0b    /* Read data bytes (high frequency) */
> +#define        OPCODE_QUAD_READ        0x6b    /* QUAD READ */
>  #define        OPCODE_PP               0x02    /* Page program (up to 256 bytes) */
>  #define        OPCODE_BE_4K            0x20    /* Erase 4KiB block */
>  #define        OPCODE_BE_4K_PMC        0xd7    /* Erase 4KiB block on PMC chips */
> @@ -48,6 +49,7 @@
>  #define        OPCODE_CHIP_ERASE       0xc7    /* Erase whole flash chip */
>  #define        OPCODE_SE               0xd8    /* Sector erase (usually 64KiB) */
>  #define        OPCODE_RDID             0x9f    /* Read JEDEC ID */
> +#define        OPCODE_RDCR             0x35    /* Read configuration register */
>
>  /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
>  #define        OPCODE_NORM_READ_4B     0x13    /* Read data bytes (low frequency) */
> @@ -76,6 +78,10 @@
>  #define        SR_BP2                  0x10    /* Block protect 2 */
>  #define        SR_SRWD                 0x80    /* SR write protect */
>
> +/* Configuration Register bits. */
> +#define SPAN_QUAD_CR_EN                0x2     /* Spansion Quad I/O */
> +#define MACR_QUAD_SR_EN                0x40    /* Macronix Quad I/O */
> +
>  /* Define max times to check status register before we give up. */
>  #define        MAX_READY_WAIT_JIFFIES  (40 * HZ)       /* M25P16 specs 40s max chip erase */
>  #define        MAX_CMD_SIZE            5
> @@ -95,6 +101,7 @@ struct m25p {
>         u8                      program_opcode;
>         u8                      *command;
>         bool                    fast_read;
> +       bool                    quad_read;
>  };
>
>  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> @@ -163,6 +170,25 @@ static inline int write_disable(struct m25p *flash)
>         return spi_write_then_read(flash->spi, &code, 1, NULL, 0);
>  }
>
> +/* Read the configuration register, returning its value in the location
> + * Return the configuration register value.
> + * Returns negative if error occurred.
> +*/
> +static int read_cr(struct m25p *flash)
> +{
> +       u8 code = OPCODE_RDCR;
> +       int ret;
> +       u8 val;
> +
> +       ret = spi_write_then_read(flash->spi, &code, 1, &val, 1);
> +       if (ret < 0) {
> +               dev_err(&flash->spi->dev, "error %d reading CR\n", ret);
> +               return ret;
> +       }
> +
> +       return val;
> +}
> +
>  /*
>   * Enable/disable 4-byte addressing mode.
>   */
> @@ -336,6 +362,122 @@ static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
>         return 0;
>  }
>
> +/* Write status register and configuration register with 2 bytes
> +* The first byte will be written to the status register, while the second byte
> +* will be written to the configuration register.
> +* Returns negative if error occurred.
> +*/
> +static int write_sr_cr(struct m25p *flash, u16 val)
> +{
> +       flash->command[0] = OPCODE_WRSR;
> +       flash->command[1] = val & 0xff;
> +       flash->command[2] = (val >> 8);
> +
> +       return spi_write(flash->spi, flash->command, 3);
> +}
> +
> +static int macronix_quad_enable(struct m25p *flash)
> +{
> +       int ret, val;
> +       u8 cmd[2];
> +       cmd[0] = OPCODE_WRSR;
> +
> +       val = read_sr(flash);
> +       cmd[1] = val | MACR_QUAD_SR_EN;
> +       write_enable(flash);
> +
> +       spi_write(flash->spi, &cmd, 2);
> +
> +       if (wait_till_ready(flash))
> +               return 1;
> +
> +       ret = read_sr(flash);
> +       if (!(ret > 0 && (ret & MACR_QUAD_SR_EN))) {
> +               dev_err(&flash->spi->dev,
> +                       "Macronix Quad bit not set");
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int spansion_quad_enable(struct m25p *flash)
> +{
> +       int ret;
> +       int quad_en = SPAN_QUAD_CR_EN << 8;
> +
> +       write_enable(flash);
> +
> +       ret = write_sr_cr(flash, quad_en);
> +       if (ret < 0) {
> +               dev_err(&flash->spi->dev,
> +                       "error while writing configuration register");
> +               return -EINVAL;
> +       }
> +
> +       /* read back and check it */
> +       ret = read_cr(flash);
> +       if (!(ret > 0 && (ret & SPAN_QUAD_CR_EN))) {
> +               dev_err(&flash->spi->dev,
> +                       "Spansion Quad bit not set");
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int m25p80_quad_read(struct mtd_info *mtd, loff_t from, size_t len,
> +       size_t *retlen, u_char *buf)
> +{
> +       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);
> +
> +       spi_message_init(&m);
> +       memset(t, 0, (sizeof(t)));
> +
> +       t[0].tx_buf = flash->command;
> +       t[0].len = m25p_cmdsz(flash) + (flash->quad_read ? 1 : 0);
> +       spi_message_add_tail(&t[0], &m);
> +
> +       t[1].rx_buf = buf;
> +       t[1].len = len;
> +       t[1].rx_nbits = SPI_NBITS_QUAD;
> +       spi_message_add_tail(&t[1], &m);
> +
> +       mutex_lock(&flash->lock);
> +
> +       /* Wait till previous write/erase is done. */
> +       if (wait_till_ready(flash)) {
> +               /* REVISIT status return?? */
> +               mutex_unlock(&flash->lock);
> +               return 1;
> +       }
> +
> +       /* FIXME switch to OPCODE_QUAD_READ.  It's required for higher
> +        * clocks; and at this writing, every chip this driver handles
> +        * supports that opcode.
> +       */
> +
> +       /* Set up the write data buffer. */
> +       opcode = flash->read_opcode;
> +       flash->command[0] = opcode;
> +       m25p_addr2cmd(flash, from, flash->command);
> +
> +       spi_sync(flash->spi, &m);
> +
> +       *retlen = m.actual_length - m25p_cmdsz(flash) -
> +                       (flash->quad_read ? 1 : 0);
> +
> +       mutex_unlock(&flash->lock);
> +
> +       return 0;
> +}
> +
>  /*
>   * Read an address range from the flash chip.  The address range
>   * may be any size provided it is within the physical boundaries.
> @@ -928,6 +1070,7 @@ static int m25p_probe(struct spi_device *spi)
>         unsigned                        i;
>         struct mtd_part_parser_data     ppdata;
>         struct device_node __maybe_unused *np = spi->dev.of_node;
> +       int ret;
>
>  #ifdef CONFIG_MTD_OF_PARTS
>         if (!of_device_is_available(np))
> @@ -979,15 +1122,9 @@ static int m25p_probe(struct spi_device *spi)
>                 }
>         }
>
> -       flash = kzalloc(sizeof *flash, GFP_KERNEL);
> +       flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL);
>         if (!flash)
>                 return -ENOMEM;
> -       flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 : 0),
> -                                       GFP_KERNEL);
> -       if (!flash->command) {
> -               kfree(flash);
> -               return -ENOMEM;
> -       }
>
>         flash->spi = spi;
>         mutex_init(&flash->lock);
> @@ -1015,7 +1152,6 @@ static int m25p_probe(struct spi_device *spi)
>         flash->mtd.flags = MTD_CAP_NORFLASH;
>         flash->mtd.size = info->sector_size * info->n_sectors;
>         flash->mtd._erase = m25p80_erase;
> -       flash->mtd._read = m25p80_read;
>
>         /* flash protection support for STmicro chips */
>         if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {
> @@ -1067,6 +1203,38 @@ static int m25p_probe(struct spi_device *spi)
>
>         flash->program_opcode = OPCODE_PP;
>
> +       flash->quad_read = false;
> +       if (spi->mode && SPI_RX_QUAD)
> +               flash->quad_read = true;
> +
> +       flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 :
> +                               (flash->quad_read ? 1 : 0)), GFP_KERNEL);
> +       if (!flash->command) {
> +               kfree(flash);
> +               return -ENOMEM;
> +       }
> +
> +       if (flash->quad_read) {
> +               if (of_property_read_bool(np, "macronix,quad_enable")) {
> +                       ret = macronix_quad_enable(flash);
> +                       if (ret) {
> +                               dev_err(&spi->dev,
> +                                       "error enabling quad");
> +                               return -EINVAL;
> +                       }
> +               } else if (of_property_read_bool(np, "spansion,quad_enable")) {
> +                       ret = spansion_quad_enable(flash);
> +                       if (ret) {
> +                               dev_err(&spi->dev,
> +                                       "error enabling quad");
> +                               return -EINVAL;
> +                       }
> +               } else
> +                       dev_dbg(&spi->dev, "quad enable not supported");
> +               flash->mtd._read = m25p80_quad_read;
> +       } else
> +               flash->mtd._read = m25p80_read;
> +
>         if (info->addr_width)
>                 flash->addr_width = info->addr_width;
>         else if (flash->mtd.size > 0x1000000) {
> --
> 1.7.1
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

-- 
Thanks,
Jagan.
--------
Jagannadha Sutradharudu Teki,
E: jagannadh.teki@gmail.com, P: +91-9676773388
Engineer - System Software Hacker
U-boot - SPI Custodian and Zynq APSOC
Ln: http://www.linkedin.com/in/jaganteki

RE: [PATCHv3 2/3] drivers: mtd: devices: Add quad read support.

[Gupta, Pekon]

Hi,

> From: Jagan Teki
> > Sourav Poddar <sourav.poddar@ti.com>
> > Some flash also support quad read mode.
> > Adding support for adding quad mode in m25p80
> > for spansion and macronix flash.
> 
> I am some how not happy with this approach, adding a flags on flash
> param table for supporting
> quad operation in specific flash is may be a better idea.
> 
> But reading SFDP, is a good idea to get the details of flash - quad
> support is one of them.
> and we can assign quad read or fast read based on that.
> 
> Any comments.
> 
Currently SFDP is mostly supported only on Micron devices, that
too the newer ones. So we don't know whether SFDP would be
accepted by other major SPI NOR manufactures like:
Spansion, Nymonix, Micronix, Toshiba, Samsung..

Thus, it might end-up like CFI (common flash interface) where some
vendors support it while others did not. So unless there are more
devices on SFDP from different vendors, I would prefer defer its
implementation in generic driver.

with regards, pekon

Re: [PATCHv3 2/3] drivers: mtd: devices: Add quad read support.

[Brian Norris]

+ others

On Wed, Oct 09, 2013 at 08:54:43PM +0530, Sourav Poddar wrote:
> Some flash also support quad read mode.
> Adding support for adding quad mode in m25p80
> for spansion and macronix flash.
> 
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
> v2->v3:
> Add macronix flash support
>  drivers/mtd/devices/m25p80.c |  184 ++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 176 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 26b14f9..dc9bcbf 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -41,6 +41,7 @@
>  #define	OPCODE_WRSR		0x01	/* Write status register 1 byte */
>  #define	OPCODE_NORM_READ	0x03	/* Read data bytes (low frequency) */
>  #define	OPCODE_FAST_READ	0x0b	/* Read data bytes (high frequency) */
> +#define	OPCODE_QUAD_READ        0x6b    /* QUAD READ */
>  #define	OPCODE_PP		0x02	/* Page program (up to 256 bytes) */
>  #define	OPCODE_BE_4K		0x20	/* Erase 4KiB block */
>  #define	OPCODE_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
> @@ -48,6 +49,7 @@
>  #define	OPCODE_CHIP_ERASE	0xc7	/* Erase whole flash chip */
>  #define	OPCODE_SE		0xd8	/* Sector erase (usually 64KiB) */
>  #define	OPCODE_RDID		0x9f	/* Read JEDEC ID */
> +#define	OPCODE_RDCR		0x35    /* Read configuration register */
>  
>  /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
>  #define	OPCODE_NORM_READ_4B	0x13	/* Read data bytes (low frequency) */
> @@ -76,6 +78,10 @@
>  #define	SR_BP2			0x10	/* Block protect 2 */
>  #define	SR_SRWD			0x80	/* SR write protect */
>  
> +/* Configuration Register bits. */
> +#define SPAN_QUAD_CR_EN		0x2	/* Spansion Quad I/O */
> +#define MACR_QUAD_SR_EN		0x40	/* Macronix Quad I/O */

Perhaps CR_ can be the prefix, like the status register SR_ macros? So:

  CR_QUAD_EN_SPAN
  CR_QUAD_EN_MACR

> +
>  /* Define max times to check status register before we give up. */
>  #define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max chip erase */
>  #define	MAX_CMD_SIZE		5
> @@ -95,6 +101,7 @@ struct m25p {
>  	u8			program_opcode;
>  	u8			*command;
>  	bool			fast_read;
> +	bool			quad_read;
>  };
>  
>  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> @@ -163,6 +170,25 @@ static inline int write_disable(struct m25p *flash)
>  	return spi_write_then_read(flash->spi, &code, 1, NULL, 0);
>  }
>  
> +/* Read the configuration register, returning its value in the location
> + * Return the configuration register value.
> + * Returns negative if error occurred.
> +*/
> +static int read_cr(struct m25p *flash)
> +{
> +	u8 code = OPCODE_RDCR;
> +	int ret;
> +	u8 val;
> +
> +	ret = spi_write_then_read(flash->spi, &code, 1, &val, 1);
> +	if (ret < 0) {
> +		dev_err(&flash->spi->dev, "error %d reading CR\n", ret);
> +		return ret;
> +	}
> +
> +	return val;
> +}
> +
>  /*
>   * Enable/disable 4-byte addressing mode.
>   */
> @@ -336,6 +362,122 @@ static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
>  	return 0;
>  }
>  
> +/* Write status register and configuration register with 2 bytes
> +* The first byte will be written to the status register, while the second byte
> +* will be written to the configuration register.
> +* Returns negative if error occurred.
> +*/

Not quite the correct multi-line comment style.

/*
 * It should be something like this. Note the asterisk alignment. You
 * also could wrap the right edge neatly to nearly 80 characters.
 */

> +static int write_sr_cr(struct m25p *flash, u16 val)
> +{
> +	flash->command[0] = OPCODE_WRSR;
> +	flash->command[1] = val & 0xff;
> +	flash->command[2] = (val >> 8);
> +
> +	return spi_write(flash->spi, flash->command, 3);
> +}
> +
> +static int macronix_quad_enable(struct m25p *flash)
> +{
> +	int ret, val;
> +	u8 cmd[2];
> +	cmd[0] = OPCODE_WRSR;
> +
> +	val = read_sr(flash);
> +	cmd[1] = val | MACR_QUAD_SR_EN;
> +	write_enable(flash);
> +
> +	spi_write(flash->spi, &cmd, 2);
> +
> +	if (wait_till_ready(flash))
> +		return 1;
> +
> +	ret = read_sr(flash);
> +	if (!(ret > 0 && (ret & MACR_QUAD_SR_EN))) {
> +		dev_err(&flash->spi->dev,
> +			"Macronix Quad bit not set");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int spansion_quad_enable(struct m25p *flash)
> +{
> +	int ret;
> +	int quad_en = SPAN_QUAD_CR_EN << 8;
> +
> +	write_enable(flash);
> +
> +	ret = write_sr_cr(flash, quad_en);
> +	if (ret < 0) {
> +		dev_err(&flash->spi->dev,
> +			"error while writing configuration register");
> +		return -EINVAL;
> +	}
> +
> +	/* read back and check it */
> +	ret = read_cr(flash);
> +	if (!(ret > 0 && (ret & SPAN_QUAD_CR_EN))) {
> +		dev_err(&flash->spi->dev,
> +			"Spansion Quad bit not set");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int m25p80_quad_read(struct mtd_info *mtd, loff_t from, size_t len,
> +	size_t *retlen, u_char *buf)
> +{

This function only has 2 meaningful lines difference from m25p80_read(),
no? I'd consider combining them. You only need a simple bool/flag to
tell whether we're in quad mode + you can re-use the 'read_opcode' field
of struct m25p.

> +	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);
> +
> +	spi_message_init(&m);
> +	memset(t, 0, (sizeof(t)));
> +
> +	t[0].tx_buf = flash->command;
> +	t[0].len = m25p_cmdsz(flash) + (flash->quad_read ? 1 : 0);

This is the first of 2 lines that are different from m25p80_read(). It
can easily be combined with the existing read implementation.

> +	spi_message_add_tail(&t[0], &m);
> +
> +	t[1].rx_buf = buf;
> +	t[1].len = len;
> +	t[1].rx_nbits = SPI_NBITS_QUAD;

This is the second of 2 different lines. You can change m25p80_read() to
have something like this:

	t[1].rx_nbits = flash->quad_read ? SPI_NBITS_QUAD : 1;

> +	spi_message_add_tail(&t[1], &m);
> +
> +	mutex_lock(&flash->lock);
> +
> +	/* Wait till previous write/erase is done. */
> +	if (wait_till_ready(flash)) {
> +		/* REVISIT status return?? */
> +		mutex_unlock(&flash->lock);
> +		return 1;
> +	}
> +
> +	/* FIXME switch to OPCODE_QUAD_READ.  It's required for higher
> +	 * clocks; and at this writing, every chip this driver handles
> +	 * supports that opcode.
> +	*/

What? It seems you blindly copied/edited the already-out-of-date comment
from m25p80_read().

> +
> +	/* Set up the write data buffer. */
> +	opcode = flash->read_opcode;
> +	flash->command[0] = opcode;
> +	m25p_addr2cmd(flash, from, flash->command);
> +
> +	spi_sync(flash->spi, &m);
> +
> +	*retlen = m.actual_length - m25p_cmdsz(flash) -
> +			(flash->quad_read ? 1 : 0);
> +
> +	mutex_unlock(&flash->lock);
> +
> +	return 0;
> +}
> +
>  /*
>   * Read an address range from the flash chip.  The address range
>   * may be any size provided it is within the physical boundaries.
> @@ -928,6 +1070,7 @@ static int m25p_probe(struct spi_device *spi)
>  	unsigned			i;
>  	struct mtd_part_parser_data	ppdata;
>  	struct device_node __maybe_unused *np = spi->dev.of_node;
> +	int ret;
>  
>  #ifdef CONFIG_MTD_OF_PARTS
>  	if (!of_device_is_available(np))
> @@ -979,15 +1122,9 @@ static int m25p_probe(struct spi_device *spi)
>  		}
>  	}
>  
> -	flash = kzalloc(sizeof *flash, GFP_KERNEL);
> +	flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL);
>  	if (!flash)
>  		return -ENOMEM;
> -	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 : 0),
> -					GFP_KERNEL);
> -	if (!flash->command) {
> -		kfree(flash);
> -		return -ENOMEM;
> -	}

You're combining a bug fix with your feature addition. The size may be
off-by-one (which is insignificant in this case, I think, but still...)
so the kmalloc() does needs to move down, but it should be done before
this feature patch. (Sorry, I've had a patch queued up but didn't send
it out for a while. I think somebody sorta tried to fix this a while ago
but didn't send a proper patch.)

>  
>  	flash->spi = spi;
>  	mutex_init(&flash->lock);
> @@ -1015,7 +1152,6 @@ static int m25p_probe(struct spi_device *spi)
>  	flash->mtd.flags = MTD_CAP_NORFLASH;
>  	flash->mtd.size = info->sector_size * info->n_sectors;
>  	flash->mtd._erase = m25p80_erase;
> -	flash->mtd._read = m25p80_read;
>  
>  	/* flash protection support for STmicro chips */
>  	if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {
> @@ -1067,6 +1203,38 @@ static int m25p_probe(struct spi_device *spi)
>  
>  	flash->program_opcode = OPCODE_PP;
>  
> +	flash->quad_read = false;
> +	if (spi->mode && SPI_RX_QUAD)

You're looking for bitwise '&', not logical '&&'.

> +		flash->quad_read = true;

But you can just replace the previous 3 lines with:

	flash->quad_read = spi->mode & SPI_RX_QUAD;

or this, if you really want be careful about the bit position:

	flash->quad_read = !!(spi->mode & SPI_RX_QUAD);

> +
> +	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 :
> +				(flash->quad_read ? 1 : 0)), GFP_KERNEL);

That's an ugly conditional. Maybe we just want to increase MAX_CMD_SIZE
and be done with it? Saving an extra byte is not helping anyone (and I
think pretty much everyone always has fast_read==true anyway).

> +	if (!flash->command) {
> +		kfree(flash);
> +		return -ENOMEM;
> +	}
> +
> +	if (flash->quad_read) {
> +		if (of_property_read_bool(np, "macronix,quad_enable")) {

As Jagan mentioned, I think we want this to be discoverable from within
m25p80.c. I don't think we should require it to be in DT. (We could
still support a DT binding just in case, but I think the majority
use-case should be easier to have a flag in the ID table; and if we ever
support SFDP, that could complement the flag approach nicely.)

Also, be sure to add a documentation patch for the DT binding if you
really need the binding.

> +			ret = macronix_quad_enable(flash);
> +			if (ret) {
> +				dev_err(&spi->dev,
> +					"error enabling quad");
> +				return -EINVAL;
> +			}
> +		} else if (of_property_read_bool(np, "spansion,quad_enable")) {

Ditto on the binding. I don't think it's necessary, and I would prefer
we go with the ID table flag or SFDP. But if you need it, document it.

> +			ret = spansion_quad_enable(flash);
> +			if (ret) {
> +				dev_err(&spi->dev,
> +					"error enabling quad");
> +				return -EINVAL;
> +			}
> +		} else
> +			dev_dbg(&spi->dev, "quad enable not supported");

...and if quad enable is not supported, we just blaze on to use quad
mode anyway?? No, I think this needs to be rewritten so that we only set
flash->quad_read = true when all of the following are true:

(1) the SPI controller supports quad I/O
(2) the flash supports it (i.e., after we see that the device supports
    it in the ID table/DT/SFDP) and
(3) we have successfully run one of the quad-enable commands

Then if you follow my advice on unifying m25p80_quad_read() and
m25p80_read(), you'll never have a mismatch between flash->quad_read,
the state of the flash, and the assigned flash->mtd._read callback
function. We will trivially fall back to single-I/O read if anything
fails, too.

> +		flash->mtd._read = m25p80_quad_read;
> +	} else
> +		flash->mtd._read = m25p80_read;

This mtd._read callback assignment should not need to be touched.

> +
>  	if (info->addr_width)
>  		flash->addr_width = info->addr_width;
>  	else if (flash->mtd.size > 0x1000000) {

Brian

Re: [PATCHv3 2/3] drivers: mtd: devices: Add quad read support.

[Sourav Poddar]

Hi Brian,

Thanks for the review, my reply inlined.
On Thursday 24 October 2013 06:36 AM, Brian Norris wrote:
> + others
>
> On Wed, Oct 09, 2013 at 08:54:43PM +0530, Sourav Poddar wrote:
>> Some flash also support quad read mode.
>> Adding support for adding quad mode in m25p80
>> for spansion and macronix flash.
>>
>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>> ---
>> v2->v3:
>> Add macronix flash support
>>   drivers/mtd/devices/m25p80.c |  184 ++++++++++++++++++++++++++++++++++++++++--
>>   1 files changed, 176 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 26b14f9..dc9bcbf 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -41,6 +41,7 @@
>>   #define	OPCODE_WRSR		0x01	/* Write status register 1 byte */
>>   #define	OPCODE_NORM_READ	0x03	/* Read data bytes (low frequency) */
>>   #define	OPCODE_FAST_READ	0x0b	/* Read data bytes (high frequency) */
>> +#define	OPCODE_QUAD_READ        0x6b    /* QUAD READ */
>>   #define	OPCODE_PP		0x02	/* Page program (up to 256 bytes) */
>>   #define	OPCODE_BE_4K		0x20	/* Erase 4KiB block */
>>   #define	OPCODE_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
>> @@ -48,6 +49,7 @@
>>   #define	OPCODE_CHIP_ERASE	0xc7	/* Erase whole flash chip */
>>   #define	OPCODE_SE		0xd8	/* Sector erase (usually 64KiB) */
>>   #define	OPCODE_RDID		0x9f	/* Read JEDEC ID */
>> +#define	OPCODE_RDCR		0x35    /* Read configuration register */
>>
>>   /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
>>   #define	OPCODE_NORM_READ_4B	0x13	/* Read data bytes (low frequency) */
>> @@ -76,6 +78,10 @@
>>   #define	SR_BP2			0x10	/* Block protect 2 */
>>   #define	SR_SRWD			0x80	/* SR write protect */
>>
>> +/* Configuration Register bits. */
>> +#define SPAN_QUAD_CR_EN		0x2	/* Spansion Quad I/O */
>> +#define MACR_QUAD_SR_EN		0x40	/* Macronix Quad I/O */
> Perhaps CR_ can be the prefix, like the status register SR_ macros? So:
>
>    CR_QUAD_EN_SPAN
>    CR_QUAD_EN_MACR
Yes, CR/SR can come as a prefix for Spansion. But, to enable quad mode in
macronix, we need to write to status register. So, things should be like..
   CR_QUAD_EN_SPAN
   SR_QUAD_EN_MACR


>> +
>>   /* Define max times to check status register before we give up. */
>>   #define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max chip erase */
>>   #define	MAX_CMD_SIZE		5
>> @@ -95,6 +101,7 @@ struct m25p {
>>   	u8			program_opcode;
>>   	u8			*command;
>>   	bool			fast_read;
>> +	bool			quad_read;
>>   };
>>
>>   static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
>> @@ -163,6 +170,25 @@ static inline int write_disable(struct m25p *flash)
>>   	return spi_write_then_read(flash->spi,&code, 1, NULL, 0);
>>   }
>>
>> +/* Read the configuration register, returning its value in the location
>> + * Return the configuration register value.
>> + * Returns negative if error occurred.
>> +*/
>> +static int read_cr(struct m25p *flash)
>> +{
>> +	u8 code = OPCODE_RDCR;
>> +	int ret;
>> +	u8 val;
>> +
>> +	ret = spi_write_then_read(flash->spi,&code, 1,&val, 1);
>> +	if (ret<  0) {
>> +		dev_err(&flash->spi->dev, "error %d reading CR\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return val;
>> +}
>> +
>>   /*
>>    * Enable/disable 4-byte addressing mode.
>>    */
>> @@ -336,6 +362,122 @@ static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
>>   	return 0;
>>   }
>>
>> +/* Write status register and configuration register with 2 bytes
>> +* The first byte will be written to the status register, while the second byte
>> +* will be written to the configuration register.
>> +* Returns negative if error occurred.
>> +*/
> Not quite the correct multi-line comment style.
>
> /*
>   * It should be something like this. Note the asterisk alignment. You
>   * also could wrap the right edge neatly to nearly 80 characters.
>   */
>
Ok. I will fix this in next version.
>> +static int write_sr_cr(struct m25p *flash, u16 val)
>> +{
>> +	flash->command[0] = OPCODE_WRSR;
>> +	flash->command[1] = val&  0xff;
>> +	flash->command[2] = (val>>  8);
>> +
>> +	return spi_write(flash->spi, flash->command, 3);
>> +}
>> +
>> +static int macronix_quad_enable(struct m25p *flash)
>> +{
>> +	int ret, val;
>> +	u8 cmd[2];
>> +	cmd[0] = OPCODE_WRSR;
>> +
>> +	val = read_sr(flash);
>> +	cmd[1] = val | MACR_QUAD_SR_EN;
>> +	write_enable(flash);
>> +
>> +	spi_write(flash->spi,&cmd, 2);
>> +
>> +	if (wait_till_ready(flash))
>> +		return 1;
>> +
>> +	ret = read_sr(flash);
>> +	if (!(ret>  0&&  (ret&  MACR_QUAD_SR_EN))) {
>> +		dev_err(&flash->spi->dev,
>> +			"Macronix Quad bit not set");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int spansion_quad_enable(struct m25p *flash)
>> +{
>> +	int ret;
>> +	int quad_en = SPAN_QUAD_CR_EN<<  8;
>> +
>> +	write_enable(flash);
>> +
>> +	ret = write_sr_cr(flash, quad_en);
>> +	if (ret<  0) {
>> +		dev_err(&flash->spi->dev,
>> +			"error while writing configuration register");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* read back and check it */
>> +	ret = read_cr(flash);
>> +	if (!(ret>  0&&  (ret&  SPAN_QUAD_CR_EN))) {
>> +		dev_err(&flash->spi->dev,
>> +			"Spansion Quad bit not set");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int m25p80_quad_read(struct mtd_info *mtd, loff_t from, size_t len,
>> +	size_t *retlen, u_char *buf)
>> +{
> This function only has 2 meaningful lines difference from m25p80_read(),
> no? I'd consider combining them. You only need a simple bool/flag to
> tell whether we're in quad mode + you can re-use the 'read_opcode' field
> of struct m25p.
>
Yes, my only motto of keeping a seperate API was that quad supports
different sort of commands, with different dummy cycle requirements.
So, I thought it might get a bit clumsy to handle, if someone later want 
to add a
particular command along with its dummy cycle requirements.

But, yes, you are true, as of now, it can be club into one.

>> +	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);
>> +
>> +	spi_message_init(&m);
>> +	memset(t, 0, (sizeof(t)));
>> +
>> +	t[0].tx_buf = flash->command;
>> +	t[0].len = m25p_cmdsz(flash) + (flash->quad_read ? 1 : 0);
> This is the first of 2 lines that are different from m25p80_read(). It
> can easily be combined with the existing read implementation.
>
>> +	spi_message_add_tail(&t[0],&m);
>> +
>> +	t[1].rx_buf = buf;
>> +	t[1].len = len;
>> +	t[1].rx_nbits = SPI_NBITS_QUAD;
> This is the second of 2 different lines. You can change m25p80_read() to
> have something like this:
>
> 	t[1].rx_nbits = flash->quad_read ? SPI_NBITS_QUAD : 1;
>   
    True, t[0] len can be written as..

     t[0].len = m25p_cmdsz(flash) + (flash->quad_read ? 1 : 
(flash->fast_read ? 1 : 0));
>> +	spi_message_add_tail(&t[1],&m);
>> +
>> +	mutex_lock(&flash->lock);
>> +
>> +	/* Wait till previous write/erase is done. */
>> +	if (wait_till_ready(flash)) {
>> +		/* REVISIT status return?? */
>> +		mutex_unlock(&flash->lock);
>> +		return 1;
>> +	}
>> +
>> +	/* FIXME switch to OPCODE_QUAD_READ.  It's required for higher
>> +	 * clocks; and at this writing, every chip this driver handles
>> +	 * supports that opcode.
>> +	*/
> What? It seems you blindly copied/edited the already-out-of-date comment
> from m25p80_read().
>
Sorry for this, my bad. I will update this in next version.
>> +
>> +	/* Set up the write data buffer. */
>> +	opcode = flash->read_opcode;
>> +	flash->command[0] = opcode;
>> +	m25p_addr2cmd(flash, from, flash->command);
>> +
>> +	spi_sync(flash->spi,&m);
>> +
>> +	*retlen = m.actual_length - m25p_cmdsz(flash) -
>> +			(flash->quad_read ? 1 : 0);
>> +
>> +	mutex_unlock(&flash->lock);
>> +
>> +	return 0;
>> +}
>> +
>>   /*
>>    * Read an address range from the flash chip.  The address range
>>    * may be any size provided it is within the physical boundaries.
>> @@ -928,6 +1070,7 @@ static int m25p_probe(struct spi_device *spi)
>>   	unsigned			i;
>>   	struct mtd_part_parser_data	ppdata;
>>   	struct device_node __maybe_unused *np = spi->dev.of_node;
>> +	int ret;
>>
>>   #ifdef CONFIG_MTD_OF_PARTS
>>   	if (!of_device_is_available(np))
>> @@ -979,15 +1122,9 @@ static int m25p_probe(struct spi_device *spi)
>>   		}
>>   	}
>>
>> -	flash = kzalloc(sizeof *flash, GFP_KERNEL);
>> +	flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL);
>>   	if (!flash)
>>   		return -ENOMEM;
>> -	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 : 0),
>> -					GFP_KERNEL);
>> -	if (!flash->command) {
>> -		kfree(flash);
>> -		return -ENOMEM;
>> -	}
> You're combining a bug fix with your feature addition. The size may be
> off-by-one (which is insignificant in this case, I think, but still...)
> so the kmalloc() does needs to move down, but it should be done before
> this feature patch. (Sorry, I've had a patch queued up but didn't send
> it out for a while. I think somebody sorta tried to fix this a while ago
> but didn't send a proper patch.)
>
Yes, true. I can break this patch into two with first patch as the bug 
fix and
second as the feature. Is it ok?
>>
>>   	flash->spi = spi;
>>   	mutex_init(&flash->lock);
>> @@ -1015,7 +1152,6 @@ static int m25p_probe(struct spi_device *spi)
>>   	flash->mtd.flags = MTD_CAP_NORFLASH;
>>   	flash->mtd.size = info->sector_size * info->n_sectors;
>>   	flash->mtd._erase = m25p80_erase;
>> -	flash->mtd._read = m25p80_read;
>>
>>   	/* flash protection support for STmicro chips */
>>   	if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {
>> @@ -1067,6 +1203,38 @@ static int m25p_probe(struct spi_device *spi)
>>
>>   	flash->program_opcode = OPCODE_PP;
>>
>> +	flash->quad_read = false;
>> +	if (spi->mode&&  SPI_RX_QUAD)
> You're looking for bitwise '&', not logical'&&'.
>
>> +		flash->quad_read = true;
> But you can just replace the previous 3 lines with:
>
> 	flash->quad_read = spi->mode&  SPI_RX_QUAD;
>
> or this, if you really want be careful about the bit position:
>
> 	flash->quad_read = !!(spi->mode&  SPI_RX_QUAD);
>
True, will fix.
>> +
>> +	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 :
>> +				(flash->quad_read ? 1 : 0)), GFP_KERNEL);
> That's an ugly conditional. Maybe we just want to increase MAX_CMD_SIZE
> and be done with it? Saving an extra byte is not helping anyone (and I
> think pretty much everyone always has fast_read==true anyway).
Yes, this can be done. But, I was not sure about increasing the macro just
on coding perspective. If it sounds Ok, I will increase the macro and make
the corresponding cleanups.
>> +	if (!flash->command) {
>> +		kfree(flash);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	if (flash->quad_read) {
>> +		if (of_property_read_bool(np, "macronix,quad_enable")) {
> As Jagan mentioned, I think we want this to be discoverable from within
> m25p80.c. I don't think we should require it to be in DT. (We could
> still support a DT binding just in case, but I think the majority
> use-case should be easier to have a flag in the ID table; and if we ever
> support SFDP, that could complement the flag approach nicely.)
>
> Also, be sure to add a documentation patch for the DT binding if you
> really need the binding.
>
>> +			ret = macronix_quad_enable(flash);
>> +			if (ret) {
>> +				dev_err(&spi->dev,
>> +					"error enabling quad");
>> +				return -EINVAL;
>> +			}
>> +		} else if (of_property_read_bool(np, "spansion,quad_enable")) {
> Ditto on the binding. I don't think it's necessary, and I would prefer
> we go with the ID table flag or SFDP. But if you need it, document it.
>
>> +			ret = spansion_quad_enable(flash);
>> +			if (ret) {
>> +				dev_err(&spi->dev,
>> +					"error enabling quad");
>> +				return -EINVAL;
>> +			}
>> +		} else
>> +			dev_dbg(&spi->dev, "quad enable not supported");
> ...and if quad enable is not supported, we just blaze on to use quad
> mode anyway?? No, I think this needs to be rewritten so that we only set
> flash->quad_read = true when all of the following are true:
>
> (1) the SPI controller supports quad I/O
> (2) the flash supports it (i.e., after we see that the device supports
>      it in the ID table/DT/SFDP) and
> (3) we have successfully run one of the quad-enable commands
I got your idea here and to sum up, my below diff can be done to
better clean up the code.

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index f180ffd..6e32f6a 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -851,7 +851,7 @@ static const struct spi_device_id m25p_ids[] = {
         { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
         { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
         { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
-       { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) },
+       { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, MX_QUAD_EN) },

         /* Micron */
         { "n25q064",  INFO(0x20ba17, 0, 64 * 1024, 128, 0) },
@@ -870,7 +870,7 @@ static const struct spi_device_id m25p_ids[] = {
         { "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
         { "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
         { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
-       { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, 0) },
+       { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, 
SP_QUAD_EN) },
         { "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, 0) },
         { "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
         { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
@@ -1094,6 +1094,25 @@ static int m25p_probe(struct spi_device *spi)
         flash->mtd.size = info->sector_size * info->n_sectors;
         flash->mtd._erase = m25p80_erase;

+       if (spi->mode & SPI_RX_QUAD) {
+               if (info->flags & SP_QUAD_EN) {
+                       ret = spansion_quad_enable(flash);
+                       if (ret) {
+                               dev_err(&spi->dev, "error enabling quad");
+                               return -EINVAL;
+                       }
+                       flash->quad_read = true;
+               } else if (info->flags & MX_QUAD_EN) {
+                       ret = spansion_quad_enable(flash);
+                       if (ret) {
+                               dev_err(&spi->dev, "error enabling quad");
+                               return -EINVAL;
+                       }
+                       flash->quad_read = true;
+               } else
+                       dev_dbg(&spi->dev, "quad enable not supported");
+       }
+

> Then if you follow my advice on unifying m25p80_quad_read() and
> m25p80_read(), you'll never have a mismatch between flash->quad_read,
> the state of the flash, and the assigned flash->mtd._read callback
> function. We will trivially fall back to single-I/O read if anything
> fails, too.
>
>> +		flash->mtd._read = m25p80_quad_read;
>> +	} else
>> +		flash->mtd._read = m25p80_read;
> This mtd._read callback assignment should not need to be touched.
Ok, with your above suggestion on clubbing quad with original read 
support, this
will automatically go.
>> +
>>   	if (info->addr_width)
>>   		flash->addr_width = info->addr_width;
>>   	else if (flash->mtd.size>  0x1000000) {
> Brian

Re: [PATCHv3 2/3] drivers: mtd: devices: Add quad read support.

[Brian Norris]

On Thu, Oct 24, 2013 at 11:14:24AM +0530, Sourav Poddar wrote:
> On Thursday 24 October 2013 06:36 AM, Brian Norris wrote:
> >On Wed, Oct 09, 2013 at 08:54:43PM +0530, Sourav Poddar wrote:
> >>--- a/drivers/mtd/devices/m25p80.c
> >>+++ b/drivers/mtd/devices/m25p80.c
> >>@@ -76,6 +78,10 @@
> >>  #define	SR_BP2			0x10	/* Block protect 2 */
> >>  #define	SR_SRWD			0x80	/* SR write protect */
> >>
> >>+/* Configuration Register bits. */
> >>+#define SPAN_QUAD_CR_EN		0x2	/* Spansion Quad I/O */
> >>+#define MACR_QUAD_SR_EN		0x40	/* Macronix Quad I/O */
> >Perhaps CR_ can be the prefix, like the status register SR_ macros? So:
> >
> >   CR_QUAD_EN_SPAN
> >   CR_QUAD_EN_MACR
> Yes, CR/SR can come as a prefix for Spansion. But, to enable quad mode in
> macronix, we need to write to status register. So, things should be like..
>   CR_QUAD_EN_SPAN
>   SR_QUAD_EN_MACR

Yes, I missed the CR vs. SR. My bad.

> >>+
> >>  /* Define max times to check status register before we give up. */
> >>  #define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max chip erase */
> >>  #define	MAX_CMD_SIZE		5
> >>@@ -95,6 +101,7 @@ struct m25p {
> >>@@ -163,6 +170,25 @@ static inline int write_disable(struct m25p *flash)

...

> >>+static int m25p80_quad_read(struct mtd_info *mtd, loff_t from, size_t len,
> >>+	size_t *retlen, u_char *buf)
> >>+{
> >This function only has 2 meaningful lines difference from m25p80_read(),
> >no? I'd consider combining them. You only need a simple bool/flag to
> >tell whether we're in quad mode + you can re-use the 'read_opcode' field
> >of struct m25p.
> >
> Yes, my only motto of keeping a seperate API was that quad supports
> different sort of commands, with different dummy cycle requirements.
> So, I thought it might get a bit clumsy to handle, if someone later
> want to add a
> particular command along with its dummy cycle requirements.
> 
> But, yes, you are true, as of now, it can be club into one.
> 
> >>+	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);
> >>+
> >>+	spi_message_init(&m);
> >>+	memset(t, 0, (sizeof(t)));
> >>+
> >>+	t[0].tx_buf = flash->command;
> >>+	t[0].len = m25p_cmdsz(flash) + (flash->quad_read ? 1 : 0);
> >This is the first of 2 lines that are different from m25p80_read(). It
> >can easily be combined with the existing read implementation.
> >
> >>+	spi_message_add_tail(&t[0],&m);
> >>+
> >>+	t[1].rx_buf = buf;
> >>+	t[1].len = len;
> >>+	t[1].rx_nbits = SPI_NBITS_QUAD;
> >This is the second of 2 different lines. You can change m25p80_read() to
> >have something like this:
> >
> >	t[1].rx_nbits = flash->quad_read ? SPI_NBITS_QUAD : 1;
>    True, t[0] len can be written as..
> 
>     t[0].len = m25p_cmdsz(flash) + (flash->quad_read ? 1 :
> (flash->fast_read ? 1 : 0));

Yikes. I'd go easy with the embedded '?:' operators.

If you're going for really fun ways to shorten things, though, you can
do:

        t[0].len = m25p_cmdsz(flash) + !!(flash->quad_read || flash->fast_read);

But seriously, I think it may be better to be more verbose to make it 
clearer. Like:

	t[0].len = m25p_cmdsz(flash);
	/* Dummy cycle */
	if (flash->quad_read || flash->fast_read)
		t[0].len++;

If you're really looking at making dummy cycles more modular, though, 
you can add an extra function like this:

static inline int m25p80_dummy_cycles_read(struct m25p *flash)
{
	if (flash->quad_read || flash->fast_read)
		return 1;
	return 0;
}

Then it's:

	t[0].len = m25p_cmdsz(flash) + m25p80_dummy_cycles_read(flash);

Then you can convert this to represent the # of bits of dummy cycle if
you ever do more exotic commands and implement a proper SPI dummy cycle
API.

But please, don't just embed a bunch of '?:'.

> >>+	spi_message_add_tail(&t[1],&m);
> >>+
> >>+	mutex_lock(&flash->lock);
> >>+
> >>+	/* Wait till previous write/erase is done. */
> >>+	if (wait_till_ready(flash)) {
> >>+		/* REVISIT status return?? */
> >>+		mutex_unlock(&flash->lock);
> >>+		return 1;
> >>+	}
> >>+
> >>+	/* FIXME switch to OPCODE_QUAD_READ.  It's required for higher
> >>+	 * clocks; and at this writing, every chip this driver handles
> >>+	 * supports that opcode.
> >>+	*/
> >What? It seems you blindly copied/edited the already-out-of-date comment
> >from m25p80_read().
> >
> Sorry for this, my bad. I will update this in next version.
> >>+
> >>+	/* Set up the write data buffer. */
> >>+	opcode = flash->read_opcode;
> >>+	flash->command[0] = opcode;
> >>+	m25p_addr2cmd(flash, from, flash->command);
> >>+
> >>+	spi_sync(flash->spi,&m);
> >>+
> >>+	*retlen = m.actual_length - m25p_cmdsz(flash) -
> >>+			(flash->quad_read ? 1 : 0);
> >>+
> >>+	mutex_unlock(&flash->lock);
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>  /*
> >>   * Read an address range from the flash chip.  The address range
> >>   * may be any size provided it is within the physical boundaries.
> >>@@ -979,15 +1122,9 @@ static int m25p_probe(struct spi_device *spi)
> >>  		}
> >>  	}
> >>
> >>-	flash = kzalloc(sizeof *flash, GFP_KERNEL);
> >>+	flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL);
> >>  	if (!flash)
> >>  		return -ENOMEM;
> >>-	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 : 0),
> >>-					GFP_KERNEL);
> >>-	if (!flash->command) {
> >>-		kfree(flash);
> >>-		return -ENOMEM;
> >>-	}
> >You're combining a bug fix with your feature addition. The size may be
> >off-by-one (which is insignificant in this case, I think, but still...)
> >so the kmalloc() does needs to move down, but it should be done before
> >this feature patch. (Sorry, I've had a patch queued up but didn't send
> >it out for a while. I think somebody sorta tried to fix this a while ago
> >but didn't send a proper patch.)
> >
> Yes, true. I can break this patch into two with first patch as the
> bug fix and
> second as the feature. Is it ok?

I just submitted my cleanups which (among other things) fixes this bug.
I believe I CC'd you. Go ahead and review it, and if it works to your
liking, please just base your work on top of it. I'll apply some or all
of that series if no one objects.

> >>
> >>  	flash->spi = spi;
> >>  	mutex_init(&flash->lock);
> >>@@ -1067,6 +1203,38 @@ static int m25p_probe(struct spi_device *spi)
> >>
> >>  	flash->program_opcode = OPCODE_PP;
> >>
> >>+	flash->quad_read = false;
> >>+	if (spi->mode&&  SPI_RX_QUAD)
> >You're looking for bitwise '&', not logical'&&'.
> >
> >>+		flash->quad_read = true;
> >But you can just replace the previous 3 lines with:
> >
> >	flash->quad_read = spi->mode&  SPI_RX_QUAD;
> >
> >or this, if you really want be careful about the bit position:
> >
> >	flash->quad_read = !!(spi->mode&  SPI_RX_QUAD);
> >
> True, will fix.
> >>+
> >>+	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 :
> >>+				(flash->quad_read ? 1 : 0)), GFP_KERNEL);
> >That's an ugly conditional. Maybe we just want to increase MAX_CMD_SIZE
> >and be done with it? Saving an extra byte is not helping anyone (and I
> >think pretty much everyone always has fast_read==true anyway).
> Yes, this can be done. But, I was not sure about increasing the macro just
> on coding perspective. If it sounds Ok, I will increase the macro and make
> the corresponding cleanups.

Just take a look at my patch for this issue, please.

> >>+		if (of_property_read_bool(np, "macronix,quad_enable")) {
...
> >>+		} else if (of_property_read_bool(np, "spansion,quad_enable")) {
...
> >>+		} else
> >>+			dev_dbg(&spi->dev, "quad enable not supported");
> >...and if quad enable is not supported, we just blaze on to use quad
> >mode anyway?? No, I think this needs to be rewritten so that we only set
> >flash->quad_read = true when all of the following are true:
> >
> >(1) the SPI controller supports quad I/O
> >(2) the flash supports it (i.e., after we see that the device supports
> >     it in the ID table/DT/SFDP) and
> >(3) we have successfully run one of the quad-enable commands
> I got your idea here and to sum up, my below diff can be done to
> better clean up the code.

Looks a little better. A few comments on it below.

> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index f180ffd..6e32f6a 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -851,7 +851,7 @@ static const struct spi_device_id m25p_ids[] = {
>         { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
>         { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
>         { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
> -       { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) },
> +       { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, MX_QUAD_EN) },

I don't think we need a separate Macronix and Spansion quad flag; I
think we can get by with just a M25P_QUAD flag (or some other name like
that). We can differentiate MXIC and Spansion via just the manufacturer
by it's ID, right? (See the similarity to set_4byte().)

Do all parts that support quad read also support all the other quad
opcodes (like quad program)?

> 
>         /* Micron */
>         { "n25q064",  INFO(0x20ba17, 0, 64 * 1024, 128, 0) },
> @@ -870,7 +870,7 @@ static const struct spi_device_id m25p_ids[] = {
>         { "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
>         { "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
>         { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
> -       { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, 0) },
> +       { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512,
> SP_QUAD_EN) },
>         { "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, 0) },
>         { "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>         { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
> @@ -1094,6 +1094,25 @@ static int m25p_probe(struct spi_device *spi)
>         flash->mtd.size = info->sector_size * info->n_sectors;
>         flash->mtd._erase = m25p80_erase;
> 
> +       if (spi->mode & SPI_RX_QUAD) {
> +               if (info->flags & SP_QUAD_EN) {
> +                       ret = spansion_quad_enable(flash);
> +                       if (ret) {
> +                               dev_err(&spi->dev, "error enabling quad");
> +                               return -EINVAL;
> +                       }
> +                       flash->quad_read = true;
> +               } else if (info->flags & MX_QUAD_EN) {
> +                       ret = spansion_quad_enable(flash);
> +                       if (ret) {
> +                               dev_err(&spi->dev, "error enabling quad");
> +                               return -EINVAL;
> +                       }
> +                       flash->quad_read = true;
> +               } else
> +                       dev_dbg(&spi->dev, "quad enable not supported");
> +       }
> +

I think we'll want a separate function set_quad_mode(), so we can just do:

	if (spi->mode & SPI_RX_QUAD && info->flags & M25P_QUAD) {
		ret = set_quad_mode(flash, info->jedec_id, 1);
		if (ret) {
			dev_err(...);
			return ret;
		}
		flash->quad_read = true;
	}

Then the set_quad_mode() function can do things very similarly to
set_4byte(), based on the manufacturer JEDEC ID.

Do you plan on supporting Quad Page Program? How about the dedicated
4-byte addressing variants of these Quad commands?

Are there any other important side effects when enabling quad mode? I'm
reading some things about changes in WP# and HOLD# behavior, but I'm not
quite sure right now if that has ramifications on the rest of the
driver.

Brian

Re: [PATCHv3 2/3] drivers: mtd: devices: Add quad read support.

[Sourav Poddar]

Hi Brian,
On Thursday 24 October 2013 01:04 PM, Brian Norris wrote:
> On Thu, Oct 24, 2013 at 11:14:24AM +0530, Sourav Poddar wrote:
>> On Thursday 24 October 2013 06:36 AM, Brian Norris wrote:
>>> On Wed, Oct 09, 2013 at 08:54:43PM +0530, Sourav Poddar wrote:
>>>> --- a/drivers/mtd/devices/m25p80.c
>>>> +++ b/drivers/mtd/devices/m25p80.c
>>>> @@ -76,6 +78,10 @@
>>>>   #define	SR_BP2			0x10	/* Block protect 2 */
>>>>   #define	SR_SRWD			0x80	/* SR write protect */
>>>>
>>>> +/* Configuration Register bits. */
>>>> +#define SPAN_QUAD_CR_EN		0x2	/* Spansion Quad I/O */
>>>> +#define MACR_QUAD_SR_EN		0x40	/* Macronix Quad I/O */
>>> Perhaps CR_ can be the prefix, like the status register SR_ macros? So:
>>>
>>>    CR_QUAD_EN_SPAN
>>>    CR_QUAD_EN_MACR
>> Yes, CR/SR can come as a prefix for Spansion. But, to enable quad mode in
>> macronix, we need to write to status register. So, things should be like..
>>    CR_QUAD_EN_SPAN
>>    SR_QUAD_EN_MACR
> Yes, I missed the CR vs. SR. My bad.
>
>>>> +
>>>>   /* Define max times to check status register before we give up. */
>>>>   #define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max chip erase */
>>>>   #define	MAX_CMD_SIZE		5
>>>> @@ -95,6 +101,7 @@ struct m25p {
>>>> @@ -163,6 +170,25 @@ static inline int write_disable(struct m25p *flash)
> ...
>
>>>> +static int m25p80_quad_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>> +	size_t *retlen, u_char *buf)
>>>> +{
>>> This function only has 2 meaningful lines difference from m25p80_read(),
>>> no? I'd consider combining them. You only need a simple bool/flag to
>>> tell whether we're in quad mode + you can re-use the 'read_opcode' field
>>> of struct m25p.
>>>
>> Yes, my only motto of keeping a seperate API was that quad supports
>> different sort of commands, with different dummy cycle requirements.
>> So, I thought it might get a bit clumsy to handle, if someone later
>> want to add a
>> particular command along with its dummy cycle requirements.
>>
>> But, yes, you are true, as of now, it can be club into one.
>>
>>>> +	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);
>>>> +
>>>> +	spi_message_init(&m);
>>>> +	memset(t, 0, (sizeof(t)));
>>>> +
>>>> +	t[0].tx_buf = flash->command;
>>>> +	t[0].len = m25p_cmdsz(flash) + (flash->quad_read ? 1 : 0);
>>> This is the first of 2 lines that are different from m25p80_read(). It
>>> can easily be combined with the existing read implementation.
>>>
>>>> +	spi_message_add_tail(&t[0],&m);
>>>> +
>>>> +	t[1].rx_buf = buf;
>>>> +	t[1].len = len;
>>>> +	t[1].rx_nbits = SPI_NBITS_QUAD;
>>> This is the second of 2 different lines. You can change m25p80_read() to
>>> have something like this:
>>>
>>> 	t[1].rx_nbits = flash->quad_read ? SPI_NBITS_QUAD : 1;
>>     True, t[0] len can be written as..
>>
>>      t[0].len = m25p_cmdsz(flash) + (flash->quad_read ? 1 :
>> (flash->fast_read ? 1 : 0));
> Yikes. I'd go easy with the embedded '?:' operators.
>
> If you're going for really fun ways to shorten things, though, you can
> do:
>
>          t[0].len = m25p_cmdsz(flash) + !!(flash->quad_read || flash->fast_read);
>
> But seriously, I think it may be better to be more verbose to make it
> clearer. Like:
>
> 	t[0].len = m25p_cmdsz(flash);
> 	/* Dummy cycle */
> 	if (flash->quad_read || flash->fast_read)
> 		t[0].len++;
>
> If you're really looking at making dummy cycles more modular, though,
> you can add an extra function like this:
>
> static inline int m25p80_dummy_cycles_read(struct m25p *flash)
> {
> 	if (flash->quad_read || flash->fast_read)
> 		return 1;
> 	return 0;
> }
>
> Then it's:
>
> 	t[0].len = m25p_cmdsz(flash) + m25p80_dummy_cycles_read(flash);
>
> Then you can convert this to represent the # of bits of dummy cycle if
> you ever do more exotic commands and implement a proper SPI dummy cycle
> API.
>
> But please, don't just embed a bunch of '?:'.
>
The above suggestion looks quite clean and verbose. I will go this way in my
next version.
>>>> +	spi_message_add_tail(&t[1],&m);
>>>> +
>>>> +	mutex_lock(&flash->lock);
>>>> +
>>>> +	/* Wait till previous write/erase is done. */
>>>> +	if (wait_till_ready(flash)) {
>>>> +		/* REVISIT status return?? */
>>>> +		mutex_unlock(&flash->lock);
>>>> +		return 1;
>>>> +	}
>>>> +
>>>> +	/* FIXME switch to OPCODE_QUAD_READ.  It's required for higher
>>>> +	 * clocks; and at this writing, every chip this driver handles
>>>> +	 * supports that opcode.
>>>> +	*/
>>> What? It seems you blindly copied/edited the already-out-of-date comment
>> >from m25p80_read().
>> Sorry for this, my bad. I will update this in next version.
>>>> +
>>>> +	/* Set up the write data buffer. */
>>>> +	opcode = flash->read_opcode;
>>>> +	flash->command[0] = opcode;
>>>> +	m25p_addr2cmd(flash, from, flash->command);
>>>> +
>>>> +	spi_sync(flash->spi,&m);
>>>> +
>>>> +	*retlen = m.actual_length - m25p_cmdsz(flash) -
>>>> +			(flash->quad_read ? 1 : 0);
>>>> +
>>>> +	mutex_unlock(&flash->lock);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>   /*
>>>>    * Read an address range from the flash chip.  The address range
>>>>    * may be any size provided it is within the physical boundaries.
>>>> @@ -979,15 +1122,9 @@ static int m25p_probe(struct spi_device *spi)
>>>>   		}
>>>>   	}
>>>>
>>>> -	flash = kzalloc(sizeof *flash, GFP_KERNEL);
>>>> +	flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL);
>>>>   	if (!flash)
>>>>   		return -ENOMEM;
>>>> -	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 : 0),
>>>> -					GFP_KERNEL);
>>>> -	if (!flash->command) {
>>>> -		kfree(flash);
>>>> -		return -ENOMEM;
>>>> -	}
>>> You're combining a bug fix with your feature addition. The size may be
>>> off-by-one (which is insignificant in this case, I think, but still...)
>>> so the kmalloc() does needs to move down, but it should be done before
>>> this feature patch. (Sorry, I've had a patch queued up but didn't send
>>> it out for a while. I think somebody sorta tried to fix this a while ago
>>> but didn't send a proper patch.)
>>>
>> Yes, true. I can break this patch into two with first patch as the
>> bug fix and
>> second as the feature. Is it ok?
> I just submitted my cleanups which (among other things) fixes this bug.
> I believe I CC'd you. Go ahead and review it, and if it works to your
> liking, please just base your work on top of it. I'll apply some or all
> of that series if no one objects.
>
Ok.
>>>>   	flash->spi = spi;
>>>>   	mutex_init(&flash->lock);
>>>> @@ -1067,6 +1203,38 @@ static int m25p_probe(struct spi_device *spi)
>>>>
>>>>   	flash->program_opcode = OPCODE_PP;
>>>>
>>>> +	flash->quad_read = false;
>>>> +	if (spi->mode&&   SPI_RX_QUAD)
>>> You're looking for bitwise '&', not logical'&&'.
>>>
>>>> +		flash->quad_read = true;
>>> But you can just replace the previous 3 lines with:
>>>
>>> 	flash->quad_read = spi->mode&   SPI_RX_QUAD;
>>>
>>> or this, if you really want be careful about the bit position:
>>>
>>> 	flash->quad_read = !!(spi->mode&   SPI_RX_QUAD);
>>>
>> True, will fix.
>>>> +
>>>> +	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 :
>>>> +				(flash->quad_read ? 1 : 0)), GFP_KERNEL);
>>> That's an ugly conditional. Maybe we just want to increase MAX_CMD_SIZE
>>> and be done with it? Saving an extra byte is not helping anyone (and I
>>> think pretty much everyone always has fast_read==true anyway).
>> Yes, this can be done. But, I was not sure about increasing the macro just
>> on coding perspective. If it sounds Ok, I will increase the macro and make
>> the corresponding cleanups.
> Just take a look at my patch for this issue, please.
>
Ok.
>>>> +		if (of_property_read_bool(np, "macronix,quad_enable")) {
> ...
>>>> +		} else if (of_property_read_bool(np, "spansion,quad_enable")) {
> ...
>>>> +		} else
>>>> +			dev_dbg(&spi->dev, "quad enable not supported");
>>> ...and if quad enable is not supported, we just blaze on to use quad
>>> mode anyway?? No, I think this needs to be rewritten so that we only set
>>> flash->quad_read = true when all of the following are true:
>>>
>>> (1) the SPI controller supports quad I/O
>>> (2) the flash supports it (i.e., after we see that the device supports
>>>      it in the ID table/DT/SFDP) and
>>> (3) we have successfully run one of the quad-enable commands
>> I got your idea here and to sum up, my below diff can be done to
>> better clean up the code.
> Looks a little better. A few comments on it below.
>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index f180ffd..6e32f6a 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -851,7 +851,7 @@ static const struct spi_device_id m25p_ids[] = {
>>          { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
>>          { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
>>          { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
>> -       { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) },
>> +       { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, MX_QUAD_EN) },
> I don't think we need a separate Macronix and Spansion quad flag; I
> think we can get by with just a M25P_QUAD flag (or some other name like
> that). We can differentiate MXIC and Spansion via just the manufacturer
> by it's ID, right? (See the similarity to set_4byte().)
>
Yes, I believe this can be done on similar lines to set_4byte. I will try
this out in my next version.


> Do all parts that support quad read also support all the other quad
> opcodes (like quad program)?
>
I have Spansion and Macronix flash with me.

For spansion,
supported quad commands are:
QOR(0x6b) which is what I am using.
QIOR(0xeb)
DDR QIOR(0xed)

For macronix,
QOR(0x6b) which is what I am using.
QIOR(0xeb)

>>          /* Micron */
>>          { "n25q064",  INFO(0x20ba17, 0, 64 * 1024, 128, 0) },
>> @@ -870,7 +870,7 @@ static const struct spi_device_id m25p_ids[] = {
>>          { "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
>>          { "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
>>          { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
>> -       { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, 0) },
>> +       { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512,
>> SP_QUAD_EN) },
>>          { "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, 0) },
>>          { "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>>          { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>> @@ -1094,6 +1094,25 @@ static int m25p_probe(struct spi_device *spi)
>>          flash->mtd.size = info->sector_size * info->n_sectors;
>>          flash->mtd._erase = m25p80_erase;
>>
>> +       if (spi->mode&  SPI_RX_QUAD) {
>> +               if (info->flags&  SP_QUAD_EN) {
>> +                       ret = spansion_quad_enable(flash);
>> +                       if (ret) {
>> +                               dev_err(&spi->dev, "error enabling quad");
>> +                               return -EINVAL;
>> +                       }
>> +                       flash->quad_read = true;
>> +               } else if (info->flags&  MX_QUAD_EN) {
>> +                       ret = spansion_quad_enable(flash);
>> +                       if (ret) {
>> +                               dev_err(&spi->dev, "error enabling quad");
>> +                               return -EINVAL;
>> +                       }
>> +                       flash->quad_read = true;
>> +               } else
>> +                       dev_dbg(&spi->dev, "quad enable not supported");
>> +       }
>> +
> I think we'll want a separate function set_quad_mode(), so we can just do:
>
> 	if (spi->mode&  SPI_RX_QUAD&&  info->flags&  M25P_QUAD) {
> 		ret = set_quad_mode(flash, info->jedec_id, 1);
> 		if (ret) {
> 			dev_err(...);
> 			return ret;
> 		}
> 		flash->quad_read = true;
> 	}
>
> Then the set_quad_mode() function can do things very similarly to
> set_4byte(), based on the manufacturer JEDEC ID.
Make sense, I will implement this.
> Do you plan on supporting Quad Page Program?
Currently, I am planning to add Quad output read(8 dummy cycle) only.
>   How about the dedicated
> 4-byte addressing variants of these Quad commands?
>
Hmm, seems like I missed this. I will add this.
> Are there any other important side effects when enabling quad mode? I'm
> reading some things about changes in WP# and HOLD# behavior, but I'm not
> quite sure right now if that has ramifications on the rest of the
> driver.
>
Nope, I have not faced any. WP# and HOLD# are the IO2 and IO3 lines
respectively in quad mode.
> Brian

Re: [PATCHv3 2/3] drivers: mtd: devices: Add quad read support.

[Brian Norris]

On Thu, Oct 24, 2013 at 02:14:59PM +0530, Sourav Poddar wrote:
> On Thursday 24 October 2013 01:04 PM, Brian Norris wrote:
> >I just submitted my cleanups which (among other things) fixes this bug.
> >I believe I CC'd you. Go ahead and review it, and if it works to your
> >liking, please just base your work on top of it. I'll apply some or all
> >of that series if no one objects.
> >
> Ok.

I'll push parts the reviewed parts of my patch series now, so you can
base off them. (I'm still open to comments on them, though.)

> >Do all parts that support quad read also support all the other quad
> >opcodes (like quad program)?
> >
> I have Spansion and Macronix flash with me.
> 
> For spansion,
> supported quad commands are:
> QOR(0x6b) which is what I am using.
> QIOR(0xeb)
> DDR QIOR(0xed)
> 
> For macronix,
> QOR(0x6b) which is what I am using.
> QIOR(0xeb)

So it seems there are flash which support quad read but not quad
program. So the flag I recommended (M25P80_QUAD) should be made specific
to quad read (M25P80_QUAD_READ) I guess, since we may support page
program eventually.

Brian

Re: [PATCHv3 2/3] drivers: mtd: devices: Add quad read support.

[Sourav Poddar]

Hi Brian,
On Thursday 24 October 2013 10:37 PM, Brian Norris wrote:
> On Thu, Oct 24, 2013 at 02:14:59PM +0530, Sourav Poddar wrote:
>> On Thursday 24 October 2013 01:04 PM, Brian Norris wrote:
>>> I just submitted my cleanups which (among other things) fixes this bug.
>>> I believe I CC'd you. Go ahead and review it, and if it works to your
>>> liking, please just base your work on top of it. I'll apply some or all
>>> of that series if no one objects.
>>>
>> Ok.
> I'll push parts the reviewed parts of my patch series now, so you can
> base off them. (I'm still open to comments on them, though.)
>
>>> Do all parts that support quad read also support all the other quad
>>> opcodes (like quad program)?
>>>
>> I have Spansion and Macronix flash with me.
>>
>> For spansion,
>> supported quad commands are:
>> QOR(0x6b) which is what I am using.
>> QIOR(0xeb)
>> DDR QIOR(0xed)
>>
>> For macronix,
>> QOR(0x6b) which is what I am using.
>> QIOR(0xeb)
> So it seems there are flash which support quad read but not quad
> program. So the flag I recommended (M25P80_QUAD) should be made specific
> to quad read (M25P80_QUAD_READ) I guess, since we may support page
> program eventually.
>
Sounds good. I will take care of this in my next version.


~Sourav
> Brian

[RFC/PATCH 3/3] drivers: mtd: devices: Add memory mapped read support.

[Sourav Poddar]

Add memory mapped flash read support. In memory mapped, only the len, from
and t->rx_buf is required from the flash side, while other configuration
will be taken care of from the respective controller side with the help
of the transfer flag(memory_map).

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---

 drivers/mtd/devices/m25p80.c |   13 +++++++++++--
 include/linux/spi/spi.h      |    2 ++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index dc9bcbf..9d09bad 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -102,6 +102,7 @@ struct m25p {
 	u8			*command;
 	bool			fast_read;
 	bool			quad_read;
+	bool			mmap_read;
 };
 
 static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
@@ -440,10 +441,15 @@ static int m25p80_quad_read(struct mtd_info *mtd, loff_t from, size_t len,
 	spi_message_init(&m);
 	memset(t, 0, (sizeof(t)));
 
+	if (flash->mmap_read)
+		t[0].memory_map = 1;
 	t[0].tx_buf = flash->command;
-	t[0].len = m25p_cmdsz(flash) + (flash->quad_read ? 1 : 0);
+	t[0].len = flash->mmap_read ? from : m25p_cmdsz(flash) +
+				(flash->quad_read ? 1 : 0);
 	spi_message_add_tail(&t[0], &m);
 
+	if (flash->mmap_read)
+		t[1].memory_map = 1;
 	t[1].rx_buf = buf;
 	t[1].len = len;
 	t[1].rx_nbits = SPI_NBITS_QUAD;
@@ -470,7 +476,7 @@ static int m25p80_quad_read(struct mtd_info *mtd, loff_t from, size_t len,
 
 	spi_sync(flash->spi, &m);
 
-	*retlen = m.actual_length - m25p_cmdsz(flash) -
+	*retlen = flash->mmap_read ? len : m.actual_length - m25p_cmdsz(flash) -
 			(flash->quad_read ? 1 : 0);
 
 	mutex_unlock(&flash->lock);
@@ -1207,6 +1213,9 @@ static int m25p_probe(struct spi_device *spi)
 	if (spi->mode && SPI_RX_QUAD)
 		flash->quad_read = true;
 
+	if (spi->mode && SPI_RX_MMAP)
+		flash->mmap_read = true;
+
 	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 :
 				(flash->quad_read ? 1 : 0)), GFP_KERNEL);
 	if (!flash->command) {
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 4d634d6..a6ffb52 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -91,6 +91,7 @@ struct spi_device {
 #define	SPI_TX_QUAD	0x200			/* transmit with 4 wires */
 #define	SPI_RX_DUAL	0x400			/* receive with 2 wires */
 #define	SPI_RX_QUAD	0x800			/* receive with 4 wires */
+#define SPI_RX_MMAP	0x1000			/* Memory mapped read */
 	u8			bits_per_word;
 	int			irq;
 	void			*controller_state;
@@ -557,6 +558,7 @@ struct spi_transfer {
 	u16		delay_usecs;
 	u32		speed_hz;
 
+	bool	memory_map;
 	struct list_head transfer_list;
 };
 
-- 
1.7.1

Re: [RFC/PATCH 3/3] drivers: mtd: devices: Add memory mapped read support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 240 bytes --]

On Wed, Oct 09, 2013 at 08:54:44PM +0530, Sourav Poddar wrote:

>  drivers/mtd/devices/m25p80.c |   13 +++++++++++--
>  include/linux/spi/spi.h      |    2 ++

You shouldn't be adding the SPI feature in the same change as you add
the user.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3]Add quad/memory mapped support for SPI flash.

[Brian Norris]

Hi Sourav,

I must admit, I missed some of this series due to a combination of
strange Gmail filtering (I just learned that a filter for "PATCH"
doesn't match "PATCHv3" in Gmail...) and the inconsistent naming in your
patch series. (I see PATCH, PATCHv3, and RFC/PATCH.) I'll try to prevent
that in the future.

On Wed, Oct 09, 2013 at 08:54:41PM +0530, Sourav Poddar wrote:
> This patch add quad/memory mapped support in flash driver(m25p80).

Does "memory mapped" supoprt need to be in the same series as quad read
support? Those are very different topics, it seems.

> Added support for quad memory mapped for TI qspi controller also.
> 
> This patch has been tested with some internal dts data.

Your patches use new DT bindings which are not documented. If you are
adding bindings, please document them in
Documentation/devicetree/bindings/ and CC devicetree@vger.kernel.org.
(You only need to CC them for the binding documenation, not necessarily
all your code changes.)

That being said, I'm not sure you need some of the bindings that you're
using. I'll comment on the affected patch(es).

Brian

Re: [PATCH 0/3]Add quad/memory mapped support for SPI flash.

[Sourav Poddar]

Hi Brian,

Thanks for the reply.
On Thursday 24 October 2013 05:52 AM, Brian Norris wrote:
> Hi Sourav,
>
> I must admit, I missed some of this series due to a combination of
> strange Gmail filtering (I just learned that a filter for "PATCH"
> doesn't match "PATCHv3" in Gmail...) and the inconsistent naming in your
> patch series. (I see PATCH, PATCHv3, and RFC/PATCH.) I'll try to prevent
> that in the future.
>
Sorry for this, I tried to keep the tag according to the status of
the patch. I will try to be uniform in my patch subject.
> On Wed, Oct 09, 2013 at 08:54:41PM +0530, Sourav Poddar wrote:
>> This patch add quad/memory mapped support in flash driver(m25p80).
> Does "memory mapped" supoprt need to be in the same series as quad read
> support? Those are very different topics, it seems.
>
Yes, this are different topic and I can send you the quad support
seperately as a single patch ?
>> Added support for quad memory mapped for TI qspi controller also.
>>
>> This patch has been tested with some internal dts data.
> Your patches use new DT bindings which are not documented. If you are
> adding bindings, please document them in
> Documentation/devicetree/bindings/ and CC devicetree@vger.kernel.org.
> (You only need to CC them for the binding documenation, not necessarily
> all your code changes.)
Ok. I will take care of this.
> That being said, I'm not sure you need some of the bindings that you're
> using. I'll comment on the affected patch(es).
>
> Brian