2018-09-10 03:52:18

by Pi-Hsun Shih

[permalink] [raw]
Subject: [PATCH] spi: mediatek: Don't modify spi_transfer when transfer.

Mediatek SPI driver modifies some fields (tx_buf, rx_buf, len, tx_dma,
rx_dma) of the spi_transfer* passed in when doing transfer_one and in
interrupt handler. This is somewhat unexpected, and there are some
caller (e.g. Cr50 spi driver) that reuse the spi_transfer for multiple
messages. Add a field to record how many bytes have been transferred,
and calculate the right len / buffer based on it instead.

BUG=b:113973051
TEST=None

Signed-off-by: Pi-Hsun Shih <[email protected]>

Change-Id: I23e218cd964f16c0b2b26127d4a5ca6529867673
---
drivers/spi/spi-mt65xx.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 86bf45667a040..3dc31627c6558 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -98,6 +98,7 @@ struct mtk_spi {
struct clk *parent_clk, *sel_clk, *spi_clk;
struct spi_transfer *cur_transfer;
u32 xfer_len;
+ u32 num_xfered;
struct scatterlist *tx_sgl, *rx_sgl;
u32 tx_sgl_len, rx_sgl_len;
const struct mtk_spi_compatible *dev_comp;
@@ -385,6 +386,7 @@ static int mtk_spi_fifo_transfer(struct spi_master *master,

mdata->cur_transfer = xfer;
mdata->xfer_len = min(MTK_SPI_MAX_FIFO_SIZE, xfer->len);
+ mdata->num_xfered = 0;
mtk_spi_prepare_transfer(master, xfer);
mtk_spi_setup_packet(master);

@@ -415,6 +417,7 @@ static int mtk_spi_dma_transfer(struct spi_master *master,
mdata->tx_sgl_len = 0;
mdata->rx_sgl_len = 0;
mdata->cur_transfer = xfer;
+ mdata->num_xfered = 0;

mtk_spi_prepare_transfer(master, xfer);

@@ -482,7 +485,7 @@ static int mtk_spi_setup(struct spi_device *spi)

static irqreturn_t mtk_spi_interrupt(int irq, void *dev_id)
{
- u32 cmd, reg_val, cnt, remainder;
+ u32 cmd, reg_val, cnt, remainder, len;
struct spi_master *master = dev_id;
struct mtk_spi *mdata = spi_master_get_devdata(master);
struct spi_transfer *trans = mdata->cur_transfer;
@@ -497,36 +500,38 @@ static irqreturn_t mtk_spi_interrupt(int irq, void *dev_id)
if (trans->rx_buf) {
cnt = mdata->xfer_len / 4;
ioread32_rep(mdata->base + SPI_RX_DATA_REG,
- trans->rx_buf, cnt);
+ trans->rx_buf + mdata->num_xfered, cnt);
remainder = mdata->xfer_len % 4;
if (remainder > 0) {
reg_val = readl(mdata->base + SPI_RX_DATA_REG);
- memcpy(trans->rx_buf + (cnt * 4),
- &reg_val, remainder);
+ memcpy(trans->rx_buf +
+ mdata->num_xfered +
+ (cnt * 4),
+ &reg_val,
+ remainder);
}
}

- trans->len -= mdata->xfer_len;
- if (!trans->len) {
+ mdata->num_xfered += mdata->xfer_len;
+ if (mdata->num_xfered == trans->len) {
spi_finalize_current_transfer(master);
return IRQ_HANDLED;
}

- if (trans->tx_buf)
- trans->tx_buf += mdata->xfer_len;
- if (trans->rx_buf)
- trans->rx_buf += mdata->xfer_len;
-
- mdata->xfer_len = min(MTK_SPI_MAX_FIFO_SIZE, trans->len);
+ len = trans->len - mdata->num_xfered;
+ mdata->xfer_len = min(MTK_SPI_MAX_FIFO_SIZE, len);
mtk_spi_setup_packet(master);

- cnt = trans->len / 4;
- iowrite32_rep(mdata->base + SPI_TX_DATA_REG, trans->tx_buf, cnt);
+ cnt = len / 4;
+ iowrite32_rep(mdata->base + SPI_TX_DATA_REG,
+ trans->tx_buf + mdata->num_xfered, cnt);

- remainder = trans->len % 4;
+ remainder = len % 4;
if (remainder > 0) {
reg_val = 0;
- memcpy(&reg_val, trans->tx_buf + (cnt * 4), remainder);
+ memcpy(&reg_val,
+ trans->tx_buf + (cnt * 4) + mdata->num_xfered,
+ remainder);
writel(reg_val, mdata->base + SPI_TX_DATA_REG);
}

--
2.19.0.rc2.392.g5ba43deb5a-goog



2018-09-10 03:56:19

by Pi-Hsun Shih

[permalink] [raw]
Subject: [PATCH v2] spi: mediatek: Don't modify spi_transfer when transfer.

Mediatek SPI driver modifies some fields (tx_buf, rx_buf, len, tx_dma,
rx_dma) of the spi_transfer* passed in when doing transfer_one and in
interrupt handler. This is somewhat unexpected, and there are some
caller (e.g. Cr50 spi driver) that reuse the spi_transfer for multiple
messages. Add a field to record how many bytes have been transferred,
and calculate the right len / buffer based on it instead.

Signed-off-by: Pi-Hsun Shih <[email protected]>

Change-Id: I23e218cd964f16c0b2b26127d4a5ca6529867673
---
drivers/spi/spi-mt65xx.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 86bf45667a040..3dc31627c6558 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -98,6 +98,7 @@ struct mtk_spi {
struct clk *parent_clk, *sel_clk, *spi_clk;
struct spi_transfer *cur_transfer;
u32 xfer_len;
+ u32 num_xfered;
struct scatterlist *tx_sgl, *rx_sgl;
u32 tx_sgl_len, rx_sgl_len;
const struct mtk_spi_compatible *dev_comp;
@@ -385,6 +386,7 @@ static int mtk_spi_fifo_transfer(struct spi_master *master,

mdata->cur_transfer = xfer;
mdata->xfer_len = min(MTK_SPI_MAX_FIFO_SIZE, xfer->len);
+ mdata->num_xfered = 0;
mtk_spi_prepare_transfer(master, xfer);
mtk_spi_setup_packet(master);

@@ -415,6 +417,7 @@ static int mtk_spi_dma_transfer(struct spi_master *master,
mdata->tx_sgl_len = 0;
mdata->rx_sgl_len = 0;
mdata->cur_transfer = xfer;
+ mdata->num_xfered = 0;

mtk_spi_prepare_transfer(master, xfer);

@@ -482,7 +485,7 @@ static int mtk_spi_setup(struct spi_device *spi)

static irqreturn_t mtk_spi_interrupt(int irq, void *dev_id)
{
- u32 cmd, reg_val, cnt, remainder;
+ u32 cmd, reg_val, cnt, remainder, len;
struct spi_master *master = dev_id;
struct mtk_spi *mdata = spi_master_get_devdata(master);
struct spi_transfer *trans = mdata->cur_transfer;
@@ -497,36 +500,38 @@ static irqreturn_t mtk_spi_interrupt(int irq, void *dev_id)
if (trans->rx_buf) {
cnt = mdata->xfer_len / 4;
ioread32_rep(mdata->base + SPI_RX_DATA_REG,
- trans->rx_buf, cnt);
+ trans->rx_buf + mdata->num_xfered, cnt);
remainder = mdata->xfer_len % 4;
if (remainder > 0) {
reg_val = readl(mdata->base + SPI_RX_DATA_REG);
- memcpy(trans->rx_buf + (cnt * 4),
- &reg_val, remainder);
+ memcpy(trans->rx_buf +
+ mdata->num_xfered +
+ (cnt * 4),
+ &reg_val,
+ remainder);
}
}

- trans->len -= mdata->xfer_len;
- if (!trans->len) {
+ mdata->num_xfered += mdata->xfer_len;
+ if (mdata->num_xfered == trans->len) {
spi_finalize_current_transfer(master);
return IRQ_HANDLED;
}

- if (trans->tx_buf)
- trans->tx_buf += mdata->xfer_len;
- if (trans->rx_buf)
- trans->rx_buf += mdata->xfer_len;
-
- mdata->xfer_len = min(MTK_SPI_MAX_FIFO_SIZE, trans->len);
+ len = trans->len - mdata->num_xfered;
+ mdata->xfer_len = min(MTK_SPI_MAX_FIFO_SIZE, len);
mtk_spi_setup_packet(master);

- cnt = trans->len / 4;
- iowrite32_rep(mdata->base + SPI_TX_DATA_REG, trans->tx_buf, cnt);
+ cnt = len / 4;
+ iowrite32_rep(mdata->base + SPI_TX_DATA_REG,
+ trans->tx_buf + mdata->num_xfered, cnt);

- remainder = trans->len % 4;
+ remainder = len % 4;
if (remainder > 0) {
reg_val = 0;
- memcpy(&reg_val, trans->tx_buf + (cnt * 4), remainder);
+ memcpy(&reg_val,
+ trans->tx_buf + (cnt * 4) + mdata->num_xfered,
+ remainder);
writel(reg_val, mdata->base + SPI_TX_DATA_REG);
}

--
2.19.0.rc2.392.g5ba43deb5a-goog


2018-09-18 16:16:25

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v2] spi: mediatek: Don't modify spi_transfer when transfer.

On Mon, Sep 10, 2018 at 11:54:21AM +0800, Peter Shih wrote:

> Change-Id: I23e218cd964f16c0b2b26127d4a5ca6529867673

Please don't include noise like this in upstream submissions, it doesn't
mean anything outside of your company's systems.


Attachments:
(No filename) (245.00 B)
signature.asc (499.00 B)
Download all attachments

2018-09-19 06:30:43

by Pi-Hsun Shih

[permalink] [raw]
Subject: [PATCH v3] spi: mediatek: Don't modify spi_transfer when transfer.

Mediatek SPI driver modifies some fields (tx_buf, rx_buf, len, tx_dma,
rx_dma) of the spi_transfer* passed in when doing transfer_one and in
interrupt handler. This is somewhat unexpected, and there are some
caller (e.g. Cr50 spi driver) that reuse the spi_transfer for multiple
messages. Add a field to record how many bytes have been transferred,
and calculate the right len / buffer based on it instead.

Signed-off-by: Pi-Hsun Shih <[email protected]>
---
drivers/spi/spi-mt65xx.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 86bf45667a040..3dc31627c6558 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -98,6 +98,7 @@ struct mtk_spi {
struct clk *parent_clk, *sel_clk, *spi_clk;
struct spi_transfer *cur_transfer;
u32 xfer_len;
+ u32 num_xfered;
struct scatterlist *tx_sgl, *rx_sgl;
u32 tx_sgl_len, rx_sgl_len;
const struct mtk_spi_compatible *dev_comp;
@@ -385,6 +386,7 @@ static int mtk_spi_fifo_transfer(struct spi_master *master,

mdata->cur_transfer = xfer;
mdata->xfer_len = min(MTK_SPI_MAX_FIFO_SIZE, xfer->len);
+ mdata->num_xfered = 0;
mtk_spi_prepare_transfer(master, xfer);
mtk_spi_setup_packet(master);

@@ -415,6 +417,7 @@ static int mtk_spi_dma_transfer(struct spi_master *master,
mdata->tx_sgl_len = 0;
mdata->rx_sgl_len = 0;
mdata->cur_transfer = xfer;
+ mdata->num_xfered = 0;

mtk_spi_prepare_transfer(master, xfer);

@@ -482,7 +485,7 @@ static int mtk_spi_setup(struct spi_device *spi)

static irqreturn_t mtk_spi_interrupt(int irq, void *dev_id)
{
- u32 cmd, reg_val, cnt, remainder;
+ u32 cmd, reg_val, cnt, remainder, len;
struct spi_master *master = dev_id;
struct mtk_spi *mdata = spi_master_get_devdata(master);
struct spi_transfer *trans = mdata->cur_transfer;
@@ -497,36 +500,38 @@ static irqreturn_t mtk_spi_interrupt(int irq, void *dev_id)
if (trans->rx_buf) {
cnt = mdata->xfer_len / 4;
ioread32_rep(mdata->base + SPI_RX_DATA_REG,
- trans->rx_buf, cnt);
+ trans->rx_buf + mdata->num_xfered, cnt);
remainder = mdata->xfer_len % 4;
if (remainder > 0) {
reg_val = readl(mdata->base + SPI_RX_DATA_REG);
- memcpy(trans->rx_buf + (cnt * 4),
- &reg_val, remainder);
+ memcpy(trans->rx_buf +
+ mdata->num_xfered +
+ (cnt * 4),
+ &reg_val,
+ remainder);
}
}

- trans->len -= mdata->xfer_len;
- if (!trans->len) {
+ mdata->num_xfered += mdata->xfer_len;
+ if (mdata->num_xfered == trans->len) {
spi_finalize_current_transfer(master);
return IRQ_HANDLED;
}

- if (trans->tx_buf)
- trans->tx_buf += mdata->xfer_len;
- if (trans->rx_buf)
- trans->rx_buf += mdata->xfer_len;
-
- mdata->xfer_len = min(MTK_SPI_MAX_FIFO_SIZE, trans->len);
+ len = trans->len - mdata->num_xfered;
+ mdata->xfer_len = min(MTK_SPI_MAX_FIFO_SIZE, len);
mtk_spi_setup_packet(master);

- cnt = trans->len / 4;
- iowrite32_rep(mdata->base + SPI_TX_DATA_REG, trans->tx_buf, cnt);
+ cnt = len / 4;
+ iowrite32_rep(mdata->base + SPI_TX_DATA_REG,
+ trans->tx_buf + mdata->num_xfered, cnt);

- remainder = trans->len % 4;
+ remainder = len % 4;
if (remainder > 0) {
reg_val = 0;
- memcpy(&reg_val, trans->tx_buf + (cnt * 4), remainder);
+ memcpy(&reg_val,
+ trans->tx_buf + (cnt * 4) + mdata->num_xfered,
+ remainder);
writel(reg_val, mdata->base + SPI_TX_DATA_REG);
}

--
2.19.0.rc2.392.g5ba43deb5a-goog


2018-09-19 17:32:56

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v3] spi: mediatek: Don't modify spi_transfer when transfer.

On Wed, Sep 19, 2018 at 02:29:27PM +0800, Peter Shih wrote:
> Mediatek SPI driver modifies some fields (tx_buf, rx_buf, len, tx_dma,
> rx_dma) of the spi_transfer* passed in when doing transfer_one and in
> interrupt handler. This is somewhat unexpected, and there are some

Please do not submit new versions of already applied patches, please
submit incremental updates to the existing code. Modifying existing
commits creates problems for other users building on top of those
commits so it's best practice to only change pubished git commits if
absolutely essential.


Attachments:
(No filename) (580.00 B)
signature.asc (499.00 B)
Download all attachments