Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1667101 > unrolled thread

[PATCH 4/4] i2c: rcar: check for DMA-capable buffers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/4] i2c: rcar: check for DMA-capable buffers Wolfram Sang <wsa+renesas@sang-engineering.com> - 2017-06-15 20:40 +0200
    Re: [PATCH 4/4] i2c: rcar: check for DMA-capable buffers Shuah Khan <shuahkhan@gmail.com> - 2017-06-15 20:50 +0200
      Re: [PATCH 4/4] i2c: rcar: check for DMA-capable buffers Wolfram Sang <wsa@the-dreams.de> - 2017-06-15 21:20 +0200
        Re: [PATCH 4/4] i2c: rcar: check for DMA-capable buffers Shuah Khan <shuah@kernel.org> - 2017-06-15 21:50 +0200

#1667101 — [PATCH 4/4] i2c: rcar: check for DMA-capable buffers

Handling this is special for this driver. Because the hardware needs to
initialize the next message in interrupt context, we cannot use the
i2c_check_msg_for_dma() directly. This helper only works reliably in
process context. So, we need to check during initial preparation of the
whole transfer and need to disable DMA completely for the whole transfer
once a message with a not-DMA-capable buffer is found.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-rcar.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 8be3e6cb8fe686..2b679ba4b75066 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -111,8 +111,11 @@
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
 /* persistent flags */
+#define ID_P_NODMA	(1 << 30)
 #define ID_P_PM_BLOCKED	(1 << 31)
-#define ID_P_MASK	ID_P_PM_BLOCKED
+#define ID_P_MASK	(ID_P_PM_BLOCKED | ID_P_NODMA)
+
+#define RCAR_DMA_THRESHOLD 8
 
 enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
@@ -358,8 +361,7 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv)
 	unsigned char *buf;
 	int len;
 
-	/* Do not use DMA if it's not available or for messages < 8 bytes */
-	if (IS_ERR(chan) || msg->len < 8)
+	if (IS_ERR(chan) || msg->len < RCAR_DMA_THRESHOLD || priv->flags & ID_P_NODMA)
 		return;
 
 	if (read) {
@@ -657,11 +659,15 @@ static void rcar_i2c_request_dma(struct rcar_i2c_priv *priv,
 				 struct i2c_msg *msg)
 {
 	struct device *dev = rcar_i2c_priv_to_dev(priv);
-	bool read;
+	bool read = msg->flags & I2C_M_RD;
 	struct dma_chan *chan;
 	enum dma_transfer_direction dir;
 
-	read = msg->flags & I2C_M_RD;
+	/* we need to check here because we need the 'current' context */
+	if (i2c_check_msg_for_dma(msg, RCAR_DMA_THRESHOLD, NULL) == -EFAULT) {
+		dev_dbg(dev, "skipping DMA for this whole transfer\n");
+		priv->flags |= ID_P_NODMA;
+	}
 
 	chan = read ? priv->dma_rx : priv->dma_tx;
 	if (PTR_ERR(chan) != -EPROBE_DEFER)
@@ -740,6 +746,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 	if (ret < 0 && ret != -ENXIO)
 		dev_err(dev, "error %d : %x\n", ret, priv->flags);
 
+	priv->flags &= ~ID_P_NODMA;
+
 	return ret;
 }
 
-- 
2.11.0

[toc] | [next] | [standalone]

#1667132

On Thu, Jun 15, 2017 at 12:30 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Handling this is special for this driver. Because the hardware needs to
> initialize the next message in interrupt context, we cannot use the
> i2c_check_msg_for_dma() directly. This helper only works reliably in
> process context. So, we need to check during initial preparation of the
> whole transfer and need to disable DMA completely for the whole transfer
> once a message with a not-DMA-capable buffer is found.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 8be3e6cb8fe686..2b679ba4b75066 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -111,8 +111,11 @@
>  #define ID_ARBLOST     (1 << 3)
>  #define ID_NACK                (1 << 4)
>  /* persistent flags */
> +#define ID_P_NODMA     (1 << 30)
>  #define ID_P_PM_BLOCKED        (1 << 31)
> -#define ID_P_MASK      ID_P_PM_BLOCKED
> +#define ID_P_MASK      (ID_P_PM_BLOCKED | ID_P_NODMA)
> +
> +#define RCAR_DMA_THRESHOLD 8
>
>  enum rcar_i2c_type {
>         I2C_RCAR_GEN1,
> @@ -358,8 +361,7 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv)
>         unsigned char *buf;
>         int len;
>
> -       /* Do not use DMA if it's not available or for messages < 8 bytes */
> -       if (IS_ERR(chan) || msg->len < 8)
> +       if (IS_ERR(chan) || msg->len < RCAR_DMA_THRESHOLD || priv->flags & ID_P_NODMA)

Might be more efficient to check for ID_P_NODMA first instead of msg->len.

>                 return;
>
>         if (read) {
> @@ -657,11 +659,15 @@ static void rcar_i2c_request_dma(struct rcar_i2c_priv *priv,
>                                  struct i2c_msg *msg)
>  {
>         struct device *dev = rcar_i2c_priv_to_dev(priv);
> -       bool read;
> +       bool read = msg->flags & I2C_M_RD;
>         struct dma_chan *chan;
>         enum dma_transfer_direction dir;
>
> -       read = msg->flags & I2C_M_RD;
> +       /* we need to check here because we need the 'current' context */
> +       if (i2c_check_msg_for_dma(msg, RCAR_DMA_THRESHOLD, NULL) == -EFAULT) {
> +               dev_dbg(dev, "skipping DMA for this whole transfer\n");

Adding reason for skipping will be helpful.

> +               priv->flags |= ID_P_NODMA;
> +       }
>
>         chan = read ? priv->dma_rx : priv->dma_tx;
>         if (PTR_ERR(chan) != -EPROBE_DEFER)
> @@ -740,6 +746,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
>         if (ret < 0 && ret != -ENXIO)
>                 dev_err(dev, "error %d : %x\n", ret, priv->flags);
>
> +       priv->flags &= ~ID_P_NODMA;
> +
>         return ret;
>  }
>
> --
> 2.11.0
>

thanks,
-- Shuah

[toc] | [prev] | [next] | [standalone]

#1667175

[Multipart message — attachments visible in raw view] — view raw

Hi,

> > -       /* Do not use DMA if it's not available or for messages < 8 bytes */
> > -       if (IS_ERR(chan) || msg->len < 8)
> > +       if (IS_ERR(chan) || msg->len < RCAR_DMA_THRESHOLD || priv->flags & ID_P_NODMA)
> 
> Might be more efficient to check for ID_P_NODMA first instead of msg->len.

I think most of the I2C transfers are smaller (like reading/writing one
register) than the threshold, so this "should" be more efficient. Plus,
honestly, I also think this is a micro-optimization which is largely
depending on the use-case. Can we agree on that?

> > -       read = msg->flags & I2C_M_RD;
> > +       /* we need to check here because we need the 'current' context */
> > +       if (i2c_check_msg_for_dma(msg, RCAR_DMA_THRESHOLD, NULL) == -EFAULT) {
> > +               dev_dbg(dev, "skipping DMA for this whole transfer\n");
> 
> Adding reason for skipping will be helpful.

The I2C core helper will immediately print before that the buffer is not
DMA capable. Do you think this together will do?

Thanks for your input,

   Wolfram

[toc] | [prev] | [next] | [standalone]

#1667207

On 06/15/2017 01:17 PM, Wolfram Sang wrote:
> Hi,
> 
>>> -       /* Do not use DMA if it's not available or for messages < 8 bytes */
>>> -       if (IS_ERR(chan) || msg->len < 8)
>>> +       if (IS_ERR(chan) || msg->len < RCAR_DMA_THRESHOLD || priv->flags & ID_P_NODMA)
>>
>> Might be more efficient to check for ID_P_NODMA first instead of msg->len.
> 
> I think most of the I2C transfers are smaller (like reading/writing one
> register) than the threshold, so this "should" be more efficient. Plus,
> honestly, I also think this is a micro-optimization which is largely
> depending on the use-case. Can we agree on that?

Makes sense.

> 
>>> -       read = msg->flags & I2C_M_RD;
>>> +       /* we need to check here because we need the 'current' context */
>>> +       if (i2c_check_msg_for_dma(msg, RCAR_DMA_THRESHOLD, NULL) == -EFAULT) {
>>> +               dev_dbg(dev, "skipping DMA for this whole transfer\n");
>>
>> Adding reason for skipping will be helpful.
> 
> The I2C core helper will immediately print before that the buffer is not
> DMA capable. Do you think this together will do?

That is sufficient.

thanks,
-- Shuah

> 
> Thanks for your input,
> 
>    Wolfram
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web