[LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer structure.

[LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer structure.

[P L Sai Krishna]

This patch does following things.
1. Added dummy entry in the spi_transfer structure.
2. Assigned dummy cycles to dummy member in the transfer
structure during read operation.

Signed-off-by: P L Sai Krishna <lakshmis@xilinx.com>
---
 drivers/mtd/devices/m25p80.c | 1 +
 include/linux/spi/spi.h      | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index c9c3b7f..7c9fac9 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -139,6 +139,7 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 
 	t[0].tx_buf = flash->command;
 	t[0].len = m25p_cmdsz(nor) + dummy;
+	t[0].dummy = nor->read_dummy;
 	spi_message_add_tail(&t[0], &m);
 
 	t[1].rx_buf = buf;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 857a9a1..3f93526 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -664,6 +664,7 @@ extern void spi_res_release(struct spi_master *master,
  * @len: size of rx and tx buffers (in bytes)
  * @speed_hz: Select a speed other than the device default for this
  *      transfer. If 0 the default (from @spi_device) is used.
+ * @dummy: number of dummy cycles.
  * @bits_per_word: select a bits_per_word other than the device default
  *      for this transfer. If 0 the default (from @spi_device) is used.
  * @cs_change: affects chipselect after this transfer completes
@@ -752,6 +753,7 @@ struct spi_transfer {
 	u8		bits_per_word;
 	u16		delay_usecs;
 	u32		speed_hz;
+	u32		dummy;
 
 	struct list_head transfer_list;
 };
-- 
2.1.2

[LINUX PATCH 2/2] spi:zynqmp:gqspi: Added separate dummy entry.

[P L Sai Krishna]

This patch sends dummy as a separate entry.
Break the Address+Cmd+dummy transfer into multiple transfers.
Address+Cmd as one transfer.
Dummy cycles as another transfer.
As per the controller spec, immediate data field of dummy entry
in the GenFifo represent dummy cycles.
Bus width for dummy cycles transfer should be same as
Rx bus width.

Signed-off-by: P L Sai Krishna <lakshmis@xilinx.com>
---
 drivers/spi/spi-zynqmp-gqspi.c | 57 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index aab9b49..9393b1e 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -134,6 +134,9 @@
 #define GQSPI_SELECT_MODE_QUADSPI	0x4
 #define GQSPI_DMA_UNALIGN		0x3
 #define GQSPI_DEFAULT_NUM_CS	1	/* Default number of chip selects */
+#define GQSPI_RX_BUS_WIDTH_QUAD		0x4
+#define GQSPI_RX_BUS_WIDTH_DUAL		0x2
+#define GQSPI_RX_BUS_WIDTH_SINGLE	0x1
 
 enum mode_type {GQSPI_MODE_IO, GQSPI_MODE_DMA};
 
@@ -169,6 +172,7 @@ struct zynqmp_qspi {
 	u32 genfifobus;
 	u32 dma_rx_bytes;
 	dma_addr_t dma_addr;
+	u32 rx_bus_width;
 	u32 genfifoentry;
 	enum mode_type mode;
 };
@@ -541,6 +545,35 @@ static void zynqmp_qspi_filltxfifo(struct zynqmp_qspi *xqspi, int size)
 }
 
 /**
+ * zynqmp_qspi_preparedummy:	Prepares the dummy entry
+ *
+ * @xqspi:	Pointer to the zynqmp_qspi structure
+ * @transfer:	It is a pointer to the structure containing transfer data.
+ * @genfifoentry:	genfifoentry is pointer to the variable in which
+ *			GENFIFO	mask is returned to calling function
+ */
+static void zynqmp_qspi_preparedummy(struct zynqmp_qspi *xqspi,
+					struct spi_transfer *transfer,
+					u32 *genfifoentry)
+{
+	/* For dummy Tx and Rx are NULL */
+	*genfifoentry &= ~(GQSPI_GENFIFO_TX | GQSPI_GENFIFO_RX);
+
+	/* SPI mode */
+	*genfifoentry &= ~GQSPI_GENFIFO_MODE_QUADSPI;
+	if (xqspi->rx_bus_width == GQSPI_RX_BUS_WIDTH_QUAD)
+		*genfifoentry |= GQSPI_GENFIFO_MODE_QUADSPI;
+	else if (xqspi->rx_bus_width == GQSPI_RX_BUS_WIDTH_DUAL)
+		*genfifoentry |= GQSPI_GENFIFO_MODE_DUALSPI;
+	else
+		*genfifoentry |= GQSPI_GENFIFO_MODE_SPI;
+
+	/* Immediate data */
+	*genfifoentry &= ~GQSPI_GENFIFO_IMM_DATA_MASK;
+	*genfifoentry |= transfer->dummy;
+}
+
+/**
  * zynqmp_qspi_readrxfifo:	Fills the RX FIFO as long as there is room in
  *				the FIFO.
  * @xqspi:	Pointer to the zynqmp_qspi structure
@@ -771,7 +804,7 @@ static void zynqmp_qspi_txrxsetup(struct zynqmp_qspi *xqspi,
 		*genfifoentry |= GQSPI_GENFIFO_TX;
 		*genfifoentry |=
 			zynqmp_qspi_selectspimode(xqspi, transfer->tx_nbits);
-		xqspi->bytes_to_transfer = transfer->len;
+		xqspi->bytes_to_transfer = transfer->len - (transfer->dummy/8);
 		if (xqspi->mode == GQSPI_MODE_DMA) {
 			config_reg = zynqmp_gqspi_read(xqspi,
 							GQSPI_CONFIG_OFST);
@@ -832,13 +865,19 @@ static int zynqmp_qspi_start_transfer(struct spi_master *master,
 	if (xqspi->mode == GQSPI_MODE_DMA)
 		transfer_len = xqspi->dma_rx_bytes;
 	else
-		transfer_len = transfer->len;
+		transfer_len = transfer->len - (transfer->dummy/8);
 
 	xqspi->genfifoentry = genfifoentry;
 	if ((transfer_len) < GQSPI_GENFIFO_IMM_DATA_MASK) {
 		genfifoentry &= ~GQSPI_GENFIFO_IMM_DATA_MASK;
 		genfifoentry |= transfer_len;
 		zynqmp_gqspi_write(xqspi, GQSPI_GEN_FIFO_OFST, genfifoentry);
+		if (transfer->dummy) {
+			zynqmp_qspi_preparedummy(xqspi, transfer,
+					&genfifoentry);
+			zynqmp_gqspi_write(xqspi, GQSPI_GEN_FIFO_OFST,
+					genfifoentry);
+		}
 	} else {
 		int tempcount = transfer_len;
 		u32 exponent = 8;	/* 2^8 = 256 */
@@ -979,6 +1018,8 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 	struct zynqmp_qspi *xqspi;
 	struct resource *res;
 	struct device *dev = &pdev->dev;
+	struct device_node *nc;
+	u32 rx_bus_width;
 
 	master = spi_alloc_master(&pdev->dev, sizeof(*xqspi));
 	if (!master)
@@ -1039,6 +1080,18 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 		goto clk_dis_all;
 	}
 
+	xqspi->rx_bus_width = GQSPI_RX_BUS_WIDTH_SINGLE;
+	for_each_available_child_of_node(pdev->dev.of_node, nc) {
+		ret = of_property_read_u32(nc, "spi-rx-bus-width",
+					&rx_bus_width);
+		if (!ret) {
+			xqspi->rx_bus_width = rx_bus_width;
+			break;
+		}
+	}
+	if (ret)
+		dev_err(dev, "rx bus width not found\n");
+
 	master->num_chipselect = GQSPI_DEFAULT_NUM_CS;
 
 	master->setup = zynqmp_qspi_setup;
-- 
2.1.2

Re: [LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer structure.

[Mark Brown]

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

On Mon, Mar 21, 2016 at 05:50:08PM +0530, P L Sai Krishna wrote:
> This patch does following things.
> 1. Added dummy entry in the spi_transfer structure.
> 2. Assigned dummy cycles to dummy member in the transfer
> structure during read operation.

Please try to follow the patch submission process covered in
SubmittingPatches, in particular please use subject lines reflecting the
style for the subsystem (which helps people identify relevant changes to
review) and...

>  drivers/mtd/devices/m25p80.c | 1 +
>  include/linux/spi/spi.h      | 2 ++
>  2 files changed, 3 insertions(+)

...split things up into individual patches, for example here you're
both adding a new feature and adding a user of that feature in a single
patch.

> + * @dummy: number of dummy cycles.

This needs to be clearer about what a dummy cycle is and where it gets
inserted.  We probably also want a better name, just "dummy" makes it
look like a padding field in the structure.  How about dummy_cycles?

> @@ -752,6 +753,7 @@ struct spi_transfer {
>  	u8		bits_per_word;
>  	u16		delay_usecs;
>  	u32		speed_hz;
> +	u32		dummy;
>  
>  	struct list_head transfer_list;
>  };

This isn't enough to add the feature - a client driver trying to make
use of this needs to be able to tell if the cycles are actually going to
be inserted.  I'd expect to see a capability flag that can be checked
and some error checking so that if we try to do a transfer with dummy
cycles and can't support it we don't silently ignore the dummy cycles,
ideally also something that'll handle multiples of 8 bits with SPI
controllers that don't otherwise support this feature.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer structure.

[Lakshmi Sai Krishna Potthuri]

Hi Mark,

<snip>
>On Mon, Mar 21, 2016 at 05:50:08PM +0530, P L Sai Krishna wrote:
>> This patch does following things.
>> 1. Added dummy entry in the spi_transfer structure.
>> 2. Assigned dummy cycles to dummy member in the transfer structure
>> during read operation.
>
>Please try to follow the patch submission process covered in
>SubmittingPatches, in particular please use subject lines reflecting the style
>for the subsystem (which helps people identify relevant changes to
>review) and...
>
>>  drivers/mtd/devices/m25p80.c | 1 +
>>  include/linux/spi/spi.h      | 2 ++
>>  2 files changed, 3 insertions(+)
>
>...split things up into individual patches, for example here you're both adding a
>new feature and adding a user of that feature in a single patch.

I will split the patch into two with appropriate commit messages.

>
>> + * @dummy: number of dummy cycles.
>
>This needs to be clearer about what a dummy cycle is and where it gets
>inserted.  We probably also want a better name, just "dummy" makes it look
>like a padding field in the structure.  How about dummy_cycles?

dummy_cycles is a better idea.
I will change it and add the usage of this in the description in a detailed manner.

>
>> @@ -752,6 +753,7 @@ struct spi_transfer {
>>      u8              bits_per_word;
>>      u16             delay_usecs;
>>      u32             speed_hz;
>> +    u32             dummy;
>>
>>      struct list_head transfer_list;
>>  };
>
>This isn't enough to add the feature - a client driver trying to make use of this
>needs to be able to tell if the cycles are actually going to be inserted.  I'd
>expect to see a capability flag that can be checked and some error checking so
>that if we try to do a transfer with dummy cycles and can't support it we don't
>silently ignore the dummy cycles, ideally also something that'll handle
>multiples of 8 bits with SPI controllers that don't otherwise support this
>feature.

Currently, all fast reads use 8 cycles or 1 byte of dummy. This generally works.
But it can be vary based on the flash and the type of read command.
Dummy bytes are taken care of in m25p80.c by adjusting the len field:
Length = size of (command + address + dummy byte)

There might be controllers (like ZynqMP GQSPI) that would be able to use
the information that dummy byte(s) were added and the precise number
of dummy cycles. This patch does not disturb the existing implementation
of adjusting length (as described above). It adds an additional optional feature.
So there is no harm to controllers that can't support it - they can ignore it and
still work with the existing "length adjustment" implementation.
If you think there value in adding a capability flag, please let me know.

Thanks for your review.

Regards,
Sai Krishna


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer structure.

[Mark Brown]

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

On Tue, Mar 22, 2016 at 06:39:51AM +0000, Lakshmi Sai Krishna Potthuri wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.  Please also avoid reflowing other text
into longer lengths, this makes things worse.

> >This isn't enough to add the feature - a client driver trying to make use of this
> >needs to be able to tell if the cycles are actually going to be inserted.  I'd
> >expect to see a capability flag that can be checked and some error checking so
> >that if we try to do a transfer with dummy cycles and can't support it we don't
> >silently ignore the dummy cycles, ideally also something that'll handle
> >multiples of 8 bits with SPI controllers that don't otherwise support this
> >feature.

> Currently, all fast reads use 8 cycles or 1 byte of dummy. This generally works.
> But it can be vary based on the flash and the type of read command.
> Dummy bytes are taken care of in m25p80.c by adjusting the len field:
> Length = size of (command + address + dummy byte)

> There might be controllers (like ZynqMP GQSPI) that would be able to use
> the information that dummy byte(s) were added and the precise number
> of dummy cycles. This patch does not disturb the existing implementation
> of adjusting length (as described above). It adds an additional optional feature.
> So there is no harm to controllers that can't support it - they can ignore it and
> still work with the existing "length adjustment" implementation.
> If you think there value in adding a capability flag, please let me know.

This is really not what I'd expect to happen, I'd expect that these
dummy cycles would be in addition to the actual data (see my request for
better documentation...).  If they overlap with the data then what is
the point in specifying this?  It's more work for the host, what benefit
do we get from doing it over just handing it like a normal byte?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer structure.

[Lakshmi Sai Krishna Potthuri]

Hi Mark,

>-----Original Message-----
>From: Mark Brown [mailto:broonie@kernel.org]
>Sent: Tuesday, March 22, 2016 3:36 PM
>To: Lakshmi Sai Krishna Potthuri
>Cc: Michal Simek; Soren Brinkmann; David Woodhouse; Brian Norris; Javier
>Martinez Canillas; Boris Brezillon; Stephen Warren; Geert Uytterhoeven;
>Andrew F. Davis; Marek Vasut; Jagan Teki; Rafał Miłecki; linux-
>mtd@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
>spi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Harini Katakam;
>Punnaiah Choudary Kalluri; Anirudha Sarangi
>Subject: Re: [LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer
>structure.
>
>On Tue, Mar 22, 2016 at 06:39:51AM +0000, Lakshmi Sai Krishna Potthuri
>wrote:
>
>Please fix your mail client to word wrap within paragraphs at something
>substantially less than 80 columns.  Doing this makes your messages much
>easier to read and reply to.  Please also avoid reflowing other text into longer
>lengths, this makes things worse.
>
>> >This isn't enough to add the feature - a client driver trying to make
>> >use of this needs to be able to tell if the cycles are actually going
>> >to be inserted.  I'd expect to see a capability flag that can be
>> >checked and some error checking so that if we try to do a transfer
>> >with dummy cycles and can't support it we don't silently ignore the
>> >dummy cycles, ideally also something that'll handle multiples of 8
>> >bits with SPI controllers that don't otherwise support this feature.
>
>> Currently, all fast reads use 8 cycles or 1 byte of dummy. This generally
>works.
>> But it can be vary based on the flash and the type of read command.
>> Dummy bytes are taken care of in m25p80.c by adjusting the len field:
>> Length = size of (command + address + dummy byte)
>
>> There might be controllers (like ZynqMP GQSPI) that would be able to
>> use the information that dummy byte(s) were added and the precise
>> number of dummy cycles. This patch does not disturb the existing
>> implementation of adjusting length (as described above). It adds an
>additional optional feature.
>> So there is no harm to controllers that can't support it - they can
>> ignore it and still work with the existing "length adjustment"
>implementation.
>> If you think there value in adding a capability flag, please let me know.
>
>This is really not what I'd expect to happen, I'd expect that these dummy
>cycles would be in addition to the actual data (see my request for better
>documentation...).  If they overlap with the data then what is the point in
>specifying this?  It's more work for the host, what benefit do we get from
>doing it over just handing it like a normal byte?

len field in the transfer structure contains dummy bytes along with actual data
bytes, controllers which requires dummy bytes use len field and simply Ignore
the dummy field (contains only no of cycles)added in this patch. Controllers
(like ZynqMP GQSPI) expects dummy in cycles won't work directly by using
len field because host driver doesn't know that len field of a particular transfer
includes dummy bytes or not (and also number of dummy bytes included in len
field). In such cases driver use this dummy field to identify the number of dummy
cycles and based on that it will send the required number of dummy cycles (which
i did in the second patch).

Regards
Sai Krishna


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: [LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer structure.

[Mark Brown]

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

On Fri, Mar 25, 2016 at 01:41:16PM +0000, Lakshmi Sai Krishna Potthuri wrote:

As I'm fairly sure I've said before please fix your mail client to word
wrap within paragraphs at something substantially less than 80 columns.
Doing this makes your messages much easier to read and reply to.

> >This is really not what I'd expect to happen, I'd expect that these dummy
> >cycles would be in addition to the actual data (see my request for better
> >documentation...).  If they overlap with the data then what is the point in
> >specifying this?  It's more work for the host, what benefit do we get from
> >doing it over just handing it like a normal byte?

> len field in the transfer structure contains dummy bytes along with actual data
> bytes, controllers which requires dummy bytes use len field and simply Ignore
> the dummy field (contains only no of cycles)added in this patch. Controllers
> (like ZynqMP GQSPI) expects dummy in cycles won't work directly by using
> len field because host driver doesn't know that len field of a particular transfer
> includes dummy bytes or not (and also number of dummy bytes included in len
> field). In such cases driver use this dummy field to identify the number of dummy
> cycles and based on that it will send the required number of dummy cycles (which
> i did in the second patch).

This doesn't make any sense at all to me.  Why does the controller care
what the bytes being sent to and from the device mean?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer structure.

[Lakshmi Sai Krishna Potthuri]

>-----Original Message-----
>From: Mark Brown [mailto:broonie@kernel.org]
>Sent: Friday, March 25, 2016 8:31 PM
>To: Lakshmi Sai Krishna Potthuri <lakshmis@xilinx.com>
>Cc: Michal Simek <michals@xilinx.com>; Soren Brinkmann
><sorenb@xilinx.com>; David Woodhouse <dwmw2@infradead.org>; Brian
>Norris <computersforpeace@gmail.com>; Javier Martinez Canillas
><javier@osg.samsung.com>; Boris Brezillon <boris.brezillon@free-
>electrons.com>; Stephen Warren <swarren@nvidia.com>; Geert
>Uytterhoeven <geert+renesas@glider.be>; Andrew F. Davis <afd@ti.com>;
>Marek Vasut <marex@denx.de>; Jagan Teki <jteki@openedev.com>; Rafał
>Miłecki <zajec5@gmail.com>; linux-mtd@lists.infradead.org; linux-
>kernel@vger.kernel.org; linux-spi@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; Harini Katakam <harinik@xilinx.com>; Punnaiah
>Choudary Kalluri <punnaia@xilinx.com>; Anirudha Sarangi
><anirudh@xilinx.com>
>Subject: Re: [LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer
>structure.
>
>On Fri, Mar 25, 2016 at 01:41:16PM +0000, Lakshmi Sai Krishna Potthuri wrote:
>
>As I'm fairly sure I've said before please fix your mail client to word
>wrap within paragraphs at something substantially less than 80 columns.
>Doing this makes your messages much easier to read and reply to.
>

Sorry, there was some issue with my mail client, now I corrected it.

>> >This is really not what I'd expect to happen, I'd expect that these dummy
>> >cycles would be in addition to the actual data (see my request for better
>> >documentation...).  If they overlap with the data then what is the point in
>> >specifying this?  It's more work for the host, what benefit do we get from
>> >doing it over just handing it like a normal byte?
>
>> len field in the transfer structure contains dummy bytes along with actual
>data
>> bytes, controllers which requires dummy bytes use len field and simply
>Ignore
>> the dummy field (contains only no of cycles)added in this patch. Controllers
>> (like ZynqMP GQSPI) expects dummy in cycles won't work directly by using
>> len field because host driver doesn't know that len field of a particular
>transfer
>> includes dummy bytes or not (and also number of dummy bytes included in
>len
>> field). In such cases driver use this dummy field to identify the number of
>dummy
>> cycles and based on that it will send the required number of dummy cycles
>(which
>> i did in the second patch).
>
>This doesn't make any sense at all to me.  Why does the controller care
>what the bytes being sent to and from the device mean?

>From the flash point of view, it expects the controller to send the dummy
on 1/2/4 lines based on the command. For Quad commands, flash expects
4 lines to be active during dummy phase. Similarly, 2 lines for dual
Commands and 1 line for normal/fast commands.
Since len field contains total number of cmd+addr+dummy bytes,
host driver should take care of sending these bytes on their respective
bus widths. Knowing when the dummy is being sent also helps in
the correct switching of IO pads (since the data lines are bidirectional)
ZynqMP GQSPI is a generic controller, majorly interfaced to flash devices.
It seems reasonable for it to know the above information from
the flash layer. Adding "dummy" cycles entry should be useful to any
controller that cares about it without affecting other spi/qspi controllers.

Regards
Sai Krishna



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: [LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer structure.

[Geert Uytterhoeven]

On Thu, Mar 31, 2016 at 8:14 AM, Lakshmi Sai Krishna Potthuri
<lakshmi.sai.krishna.potthuri-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote:
>>> >This is really not what I'd expect to happen, I'd expect that these dummy
>>> >cycles would be in addition to the actual data (see my request for better
>>> >documentation...).  If they overlap with the data then what is the point in
>>> >specifying this?  It's more work for the host, what benefit do we get from
>>> >doing it over just handing it like a normal byte?
>>
>>> len field in the transfer structure contains dummy bytes along with actual
>>data
>>> bytes, controllers which requires dummy bytes use len field and simply
>>Ignore
>>> the dummy field (contains only no of cycles)added in this patch. Controllers
>>> (like ZynqMP GQSPI) expects dummy in cycles won't work directly by using
>>> len field because host driver doesn't know that len field of a particular
>>transfer
>>> includes dummy bytes or not (and also number of dummy bytes included in
>>len
>>> field). In such cases driver use this dummy field to identify the number of
>>dummy
>>> cycles and based on that it will send the required number of dummy cycles
>>(which
>>> i did in the second patch).
>>
>>This doesn't make any sense at all to me.  Why does the controller care
>>what the bytes being sent to and from the device mean?
>
> From the flash point of view, it expects the controller to send the dummy
> on 1/2/4 lines based on the command. For Quad commands, flash expects
> 4 lines to be active during dummy phase. Similarly, 2 lines for dual
> Commands and 1 line for normal/fast commands.
> Since len field contains total number of cmd+addr+dummy bytes,
> host driver should take care of sending these bytes on their respective
> bus widths. Knowing when the dummy is being sent also helps in
> the correct switching of IO pads (since the data lines are bidirectional)
> ZynqMP GQSPI is a generic controller, majorly interfaced to flash devices.
> It seems reasonable for it to know the above information from
> the flash layer. Adding "dummy" cycles entry should be useful to any
> controller that cares about it without affecting other spi/qspi controllers.

The m25p80 driver already uses dummy cycles, using real spi_transfer
structs, which have tx_nbits/rx_nbits fields to indicate how many data lines
to use.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer structure.

[Lakshmi Sai Krishna Potthuri]

Hi,

>-----Original Message-----
>From: geert.uytterhoeven@gmail.com
>[mailto:geert.uytterhoeven@gmail.com] On Behalf Of Geert Uytterhoeven
>Sent: Thursday, March 31, 2016 1:58 PM
>To: Lakshmi Sai Krishna Potthuri <lakshmis@xilinx.com>
>Cc: Mark Brown <broonie@kernel.org>; Michal Simek <michals@xilinx.com>;
>Soren Brinkmann <sorenb@xilinx.com>; David Woodhouse
><dwmw2@infradead.org>; Brian Norris <computersforpeace@gmail.com>;
>Javier Martinez Canillas <javier@osg.samsung.com>; Boris Brezillon
><boris.brezillon@free-electrons.com>; Stephen Warren
><swarren@nvidia.com>; Geert Uytterhoeven <geert+renesas@glider.be>;
>Andrew F. Davis <afd@ti.com>; Marek Vasut <marex@denx.de>; Jagan Teki
><jteki@openedev.com>; Rafał Miłecki <zajec5@gmail.com>; linux-
>mtd@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
>spi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Harini Katakam
><harinik@xilinx.com>; Punnaiah Choudary Kalluri <punnaia@xilinx.com>;
>Anirudha Sarangi <anirudh@xilinx.com>; saikrishna12468@gmail.com
>Subject: Re: [LINUX PATCH 1/2] mtd: Added dummy entry in the spi_transfer
>structure.
>
>On Thu, Mar 31, 2016 at 8:14 AM, Lakshmi Sai Krishna Potthuri
><lakshmi.sai.krishna.potthuri@xilinx.com> wrote:
>>>> >This is really not what I'd expect to happen, I'd expect that these
>dummy
>>>> >cycles would be in addition to the actual data (see my request for better
>>>> >documentation...).  If they overlap with the data then what is the point
>in
>>>> >specifying this?  It's more work for the host, what benefit do we get
>from
>>>> >doing it over just handing it like a normal byte?
>>>
>>>> len field in the transfer structure contains dummy bytes along with actual
>>>data
>>>> bytes, controllers which requires dummy bytes use len field and simply
>>>Ignore
>>>> the dummy field (contains only no of cycles)added in this patch.
>Controllers
>>>> (like ZynqMP GQSPI) expects dummy in cycles won't work directly by
>using
>>>> len field because host driver doesn't know that len field of a particular
>>>transfer
>>>> includes dummy bytes or not (and also number of dummy bytes included
>in
>>>len
>>>> field). In such cases driver use this dummy field to identify the number of
>>>dummy
>>>> cycles and based on that it will send the required number of dummy
>cycles
>>>(which
>>>> i did in the second patch).
>>>
>>>This doesn't make any sense at all to me.  Why does the controller care
>>>what the bytes being sent to and from the device mean?
>>
>> From the flash point of view, it expects the controller to send the dummy
>> on 1/2/4 lines based on the command. For Quad commands, flash expects
>> 4 lines to be active during dummy phase. Similarly, 2 lines for dual
>> Commands and 1 line for normal/fast commands.
>> Since len field contains total number of cmd+addr+dummy bytes,
>> host driver should take care of sending these bytes on their respective
>> bus widths. Knowing when the dummy is being sent also helps in
>> the correct switching of IO pads (since the data lines are bidirectional)
>> ZynqMP GQSPI is a generic controller, majorly interfaced to flash devices.
>> It seems reasonable for it to know the above information from
>> the flash layer. Adding "dummy" cycles entry should be useful to any
>> controller that cares about it without affecting other spi/qspi controllers.
>
>The m25p80 driver already uses dummy cycles, using real spi_transfer
>structs, which have tx_nbits/rx_nbits fields to indicate how many data lines
>to use.

m25p80 implementation just send command, address and dummy together
with tx_nbit field as 1 and host driver can't differentiate between them.
Command and address go on one line and dummy should send based
on the command as explained my previous mail.

Regards
Sai Krishna


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.