[PATCH 1/2] spi: spi-ti-qspi: Fix FLEN and WLEN settings if bits_per_word is overridden

[PATCH 1/2] spi: spi-ti-qspi: Fix FLEN and WLEN settings if bits_per_word is overridden

[Ben Hutchings]

Each transfer can specify 8, 16 or 32 bits per word independently of
the default for the device being addressed.  Currently we always set
the FLEN register field according to the device default.  Also, if
multiple transfers in the same message have differing bits_per_word,
we bitwise-or the different values in the WLEN register field.  Fix
both of these.

Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Ben Hutchings <ben.hutchings-4yDnlxn2s6sWdaTGBSpHTA@public.gmane.org>
---
Unfortunately I don't have a test setup where bits_per_word is
overridden.  But it is straightforward to verify that the driver is no
longer using spi->bits_per_word.

Ben.

 drivers/spi/spi-ti-qspi.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 64318fcfacf2..e5cefaca0589 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -94,6 +94,7 @@ struct ti_qspi {
 #define QSPI_FLEN(n)			((n - 1) << 0)
 #define QSPI_WLEN_MAX_BITS		128
 #define QSPI_WLEN_MAX_BYTES		16
+#define QSPI_WLEN_MASK			QSPI_WLEN(QSPI_WLEN_MAX_BITS)
 
 /* STATUS REGISTER */
 #define BUSY				0x01
@@ -373,7 +374,7 @@ static int ti_qspi_start_transfer_one(struct spi_master *master,
 	struct spi_device *spi = m->spi;
 	struct spi_transfer *t;
 	int status = 0, ret;
-	int frame_length;
+	unsigned int frame_length;
 
 	/* setup device control reg */
 	qspi->dc = 0;
@@ -385,9 +386,10 @@ static int ti_qspi_start_transfer_one(struct spi_master *master,
 	if (spi->mode & SPI_CS_HIGH)
 		qspi->dc |= QSPI_CSPOL(spi->chip_select);
 
-	frame_length = (m->frame_length << 3) / spi->bits_per_word;
-
-	frame_length = clamp(frame_length, 0, QSPI_FRAME);
+	frame_length = 0;
+	list_for_each_entry(t, &m->transfers, transfer_list)
+		frame_length += t->len / (t->bits_per_word >> 3);
+	frame_length = min_t(unsigned int, frame_length, QSPI_FRAME);
 
 	/* setup command reg */
 	qspi->cmd = 0;
@@ -399,7 +401,8 @@ static int ti_qspi_start_transfer_one(struct spi_master *master,
 	mutex_lock(&qspi->list_lock);
 
 	list_for_each_entry(t, &m->transfers, transfer_list) {
-		qspi->cmd |= QSPI_WLEN(t->bits_per_word);
+		qspi->cmd = ((qspi->cmd & ~QSPI_WLEN_MASK) |
+			     QSPI_WLEN(t->bits_per_word));
 
 		ret = qspi_transfer_msg(qspi, t);
 		if (ret) {
-- 
2.8.0.rc3



-- 
Ben Hutchings
Software Developer, Codethink Ltd.


--
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

[PATCH 2/2] spi: spi-ti-qspi: Handle truncated frames properly

[Ben Hutchings]

We clamp frame_length to a maximum of 4096, but do not limit the
number of words written or read properly.  This causes 4K reads (which
some m25p80-like flash chips support) to return garbage for the last
few bytes.

Recalculate the length of each transfer, taking the frame length into
account.  Use this length in qspi_{read,write}_msg(), and to increment
spi_message::actual_length.

Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Ben Hutchings <ben.hutchings-4yDnlxn2s6sWdaTGBSpHTA@public.gmane.org>
---
 drivers/spi/spi-ti-qspi.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index e5cefaca0589..4c0acee7a55f 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -225,16 +225,16 @@ static inline int ti_qspi_poll_wc(struct ti_qspi *qspi)
 	return  -ETIMEDOUT;
 }
 
-static int qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
+static int qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t,
+			  int count)
 {
-	int wlen, count, xfer_len;
+	int wlen, xfer_len;
 	unsigned int cmd;
 	const u8 *txbuf;
 	u32 data;
 
 	txbuf = t->tx_buf;
 	cmd = qspi->cmd | QSPI_WR_SNGL;
-	count = t->len;
 	wlen = t->bits_per_word >> 3;	/* in bytes */
 	xfer_len = wlen;
 
@@ -294,9 +294,10 @@ static int qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 	return 0;
 }
 
-static int qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
+static int qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t,
+			 int count)
 {
-	int wlen, count;
+	int wlen;
 	unsigned int cmd;
 	u8 *rxbuf;
 
@@ -313,7 +314,6 @@ static int qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 		cmd |= QSPI_RD_SNGL;
 		break;
 	}
-	count = t->len;
 	wlen = t->bits_per_word >> 3;	/* in bytes */
 
 	while (count) {
@@ -344,12 +344,13 @@ static int qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 	return 0;
 }
 
-static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
+static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t,
+			     int count)
 {
 	int ret;
 
 	if (t->tx_buf) {
-		ret = qspi_write_msg(qspi, t);
+		ret = qspi_write_msg(qspi, t, count);
 		if (ret) {
 			dev_dbg(qspi->dev, "Error while writing\n");
 			return ret;
@@ -357,7 +358,7 @@ static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 	}
 
 	if (t->rx_buf) {
-		ret = qspi_read_msg(qspi, t);
+		ret = qspi_read_msg(qspi, t, count);
 		if (ret) {
 			dev_dbg(qspi->dev, "Error while reading\n");
 			return ret;
@@ -374,7 +375,8 @@ static int ti_qspi_start_transfer_one(struct spi_master *master,
 	struct spi_device *spi = m->spi;
 	struct spi_transfer *t;
 	int status = 0, ret;
-	unsigned int frame_length;
+	unsigned int frame_length, transfer_length;
+	int wlen;
 
 	/* setup device control reg */
 	qspi->dc = 0;
@@ -404,14 +406,20 @@ static int ti_qspi_start_transfer_one(struct spi_master *master,
 		qspi->cmd = ((qspi->cmd & ~QSPI_WLEN_MASK) |
 			     QSPI_WLEN(t->bits_per_word));
 
-		ret = qspi_transfer_msg(qspi, t);
+		wlen = t->bits_per_word >> 3;
+		transfer_length = min(t->len / wlen, frame_length);
+
+		ret = qspi_transfer_msg(qspi, t, transfer_length * wlen);
 		if (ret) {
 			dev_dbg(qspi->dev, "transfer message failed\n");
 			mutex_unlock(&qspi->list_lock);
 			return -EINVAL;
 		}
 
-		m->actual_length += t->len;
+		m->actual_length += transfer_length * wlen;
+		frame_length -= transfer_length;
+		if (frame_length == 0)
+			break;
 	}
 
 	mutex_unlock(&qspi->list_lock);
-- 
2.8.0.rc3


-- 
Ben Hutchings
Software Developer, Codethink Ltd.


--
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: [PATCH 1/2] spi: spi-ti-qspi: Fix FLEN and WLEN settings if bits_per_word is overridden

[Mark Brown]

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

On Tue, Apr 05, 2016 at 08:45:28PM +0100, Ben Hutchings wrote:

> +	frame_length = 0;
> +	list_for_each_entry(t, &m->transfers, transfer_list)
> +		frame_length += t->len / (t->bits_per_word >> 3);
> +	frame_length = min_t(unsigned int, frame_length, QSPI_FRAME);
>  

This doesn't seem obviously correct - what exactly is the frame length
here and why doesn't it change per transfer like the word length does?
There's nothing in the changelog about this.

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

Re: [PATCH 2/2] spi: spi-ti-qspi: Handle truncated frames properly

[Mark Brown]

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

On Tue, Apr 05, 2016 at 08:48:48PM +0100, Ben Hutchings wrote:

> We clamp frame_length to a maximum of 4096, but do not limit the
> number of words written or read properly.  This causes 4K reads (which
> some m25p80-like flash chips support) to return garbage for the last
> few bytes.

> Recalculate the length of each transfer, taking the frame length into
> account.  Use this length in qspi_{read,write}_msg(), and to increment
> spi_message::actual_length.

I'm having a hard time understanding what's actually wrong here or how
this change is intended to fix it, I think again because I don't know
what frame_length is intended to be.  As far as I can tell frame_length
is the number of words we're going to transfer in the entire message and
the driver has in some places been mixing up words and bytes which is
causing the breakage so the intention is to consistently use words
throught the driver.  Is that correct?

> -		ret = qspi_transfer_msg(qspi, t);
> +		wlen = t->bits_per_word >> 3;

For the benefit of readers:

	wlen = t->bits_per_word / 8.

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

Re: [PATCH 1/2] spi: spi-ti-qspi: Fix FLEN and WLEN settings if bits_per_word is overridden

[Ben Hutchings]

On Tue, 2016-04-12 at 02:30 +0100, Mark Brown wrote:
> On Tue, Apr 05, 2016 at 08:45:28PM +0100, Ben Hutchings wrote:
> 
> > +	frame_length = 0;
> > +	list_for_each_entry(t, &m->transfers, transfer_list)
> > +		frame_length += t->len / (t->bits_per_word >> 3);
> > +	frame_length = min_t(unsigned int, frame_length, QSPI_FRAME);
> >  
> 
> This doesn't seem obviously correct - what exactly is the frame length
> here and why doesn't it change per transfer like the word length does?
> There's nothing in the changelog about this.

The frame_length is the number of words in the frame, as it was before
this change.  Shall I insert a preparatory patch to give the variables
better names?

Ben.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.


--
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: [PATCH 2/2] spi: spi-ti-qspi: Handle truncated frames properly

[Ben Hutchings]

On Tue, 2016-04-12 at 02:58 +0100, Mark Brown wrote:
> On Tue, Apr 05, 2016 at 08:48:48PM +0100, Ben Hutchings wrote:
> 
> > We clamp frame_length to a maximum of 4096, but do not limit the
> > number of words written or read properly.  This causes 4K reads (which
> > some m25p80-like flash chips support) to return garbage for the last
> > few bytes.
> 
> > Recalculate the length of each transfer, taking the frame length into
> > account.  Use this length in qspi_{read,write}_msg(), and to increment
> > spi_message::actual_length.
> 
> I'm having a hard time understanding what's actually wrong here or how
> this change is intended to fix it,

"4K reads...return garbage"  That's wrong, no?

> I think again because I don't know
> what frame_length is intended to be.  As far as I can tell frame_length
> is the number of words we're going to transfer in the entire message

Yes.

> and the driver has in some places been mixing up words and bytes

No, it's been completely ignoring the (potentially truncated)
frame_length and using the original transfer lengths for each transfer.

> which is
> causing the breakage so the intention is to consistently use words
> throught the driver.  Is that correct?

No.

I'll add a renaming of variables to make it clearer.

Ben.

> > -		ret = qspi_transfer_msg(qspi, t);
> > +		wlen = t->bits_per_word >> 3;
> 
> For the benefit of readers:
> 
> 	wlen = t->bits_per_word / 8.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.


--
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: [PATCH 2/2] spi: spi-ti-qspi: Handle truncated frames properly

[Mark Brown]

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

On Tue, Apr 12, 2016 at 12:09:24PM +0100, Ben Hutchings wrote:
> On Tue, 2016-04-12 at 02:58 +0100, Mark Brown wrote:
> > On Tue, Apr 05, 2016 at 08:48:48PM +0100, Ben Hutchings wrote:

> > > We clamp frame_length to a maximum of 4096, but do not limit the
> > > number of words written or read properly.  This causes 4K reads (which
> > > some m25p80-like flash chips support) to return garbage for the last
> > > few bytes.

> > > Recalculate the length of each transfer, taking the frame length into
> > > account.  Use this length in qspi_{read,write}_msg(), and to increment
> > > spi_message::actual_length.

> > I'm having a hard time understanding what's actually wrong here or how
> > this change is intended to fix it,

> "4K reads...return garbage"  That's wrong, no?

It doesn't say what the driver is doing wrong which causes the garbage.

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

Re: [PATCH 1/2] spi: spi-ti-qspi: Fix FLEN and WLEN settings if bits_per_word is overridden

[Mark Brown]

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

On Tue, Apr 12, 2016 at 12:07:29PM +0100, Ben Hutchings wrote:
> On Tue, 2016-04-12 at 02:30 +0100, Mark Brown wrote:

> > This doesn't seem obviously correct - what exactly is the frame length
> > here and why doesn't it change per transfer like the word length does?
> > There's nothing in the changelog about this.

> The frame_length is the number of words in the frame, as it was before
> this change.  Shall I insert a preparatory patch to give the variables
> better names?

I see you did that already.

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