| Message ID | 1459885728.26669.7.camel@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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.
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);
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@vger.kernel.org Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> --- drivers/spi/spi-ti-qspi.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)