So, after revisiting old mail threads and taking part in a similar discussion
on the USB list, here is what I cooked up to document and ease DMA handling for
I2C within Linux. Please have a look at the documentation introduced in patch 2
for further details.
The branch can be found here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma
And big kudos to Renesas Electronics for funding this work, thank you very much!
Regards,
Wolfram
Changes since RFC:
* the helper i2c_check_msg_for_dma() can optionally take a pointer and then it
will attach a bounce buffer to it in case the message buffer is not DMA safe
* a second helper was added. i2c_release_bounce_buf() will copy data back to
the message buffer and release the bounce buffer
* the documentation has been extended to match the above
* no ifdeffery around CONFIG_DMA_API_DEBUG anymore. The I2C core behaviour
should not depend on such symbols.
* the i2c-sh_mobile driver has been updated to use bounce buffers
Wolfram Sang (4):
i2c: add helpers to ease DMA handling
i2c: add docs to clarify DMA handling
i2c: sh_mobile: use helper to decide if DMA is useful
i2c: rcar: check for DMA-capable buffers
Documentation/i2c/DMA-considerations | 37 ++++++++++++++++++++
drivers/i2c/busses/i2c-rcar.c | 18 +++++++---
drivers/i2c/busses/i2c-sh_mobile.c | 8 +++--
include/linux/i2c.h | 65 ++++++++++++++++++++++++++++++++++++
4 files changed, 121 insertions(+), 7 deletions(-)
create mode 100644 Documentation/i2c/DMA-considerations
--
2.11.0
This ensures that we fall back to PIO if the buffer is too small for DMA
being useful. Otherwise, we use DMA. A bounce buffer might be applied if
the original message buffer is not DMA safe
Signed-off-by: Wolfram Sang <[email protected]>
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-sh_mobile.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 2e097d97d258bc..4b874e508111e7 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -145,6 +145,7 @@ struct sh_mobile_i2c_data {
struct dma_chan *dma_rx;
struct scatterlist sg;
enum dma_data_direction dma_direction;
+ u8 *bounce_buf;
};
struct sh_mobile_dt_config {
@@ -548,6 +549,8 @@ static void sh_mobile_i2c_dma_callback(void *data)
pd->pos = pd->msg->len;
pd->stop_after_dma = true;
+ i2c_release_bounce_buf(pd->msg, pd->bounce_buf);
+
iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE);
}
@@ -595,6 +598,7 @@ static void sh_mobile_i2c_xfer_dma(struct sh_mobile_i2c_data *pd)
struct dma_async_tx_descriptor *txdesc;
dma_addr_t dma_addr;
dma_cookie_t cookie;
+ u8 *dma_buf = pd->bounce_buf ?: pd->msg->buf;
if (PTR_ERR(chan) == -EPROBE_DEFER) {
if (read)
@@ -608,7 +612,7 @@ static void sh_mobile_i2c_xfer_dma(struct sh_mobile_i2c_data *pd)
if (IS_ERR(chan))
return;
- dma_addr = dma_map_single(chan->device->dev, pd->msg->buf, pd->msg->len, dir);
+ dma_addr = dma_map_single(chan->device->dev, dma_buf, pd->msg->len, dir);
if (dma_mapping_error(chan->device->dev, dma_addr)) {
dev_dbg(pd->dev, "dma map failed, using PIO\n");
return;
@@ -665,7 +669,7 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg,
pd->pos = -1;
pd->sr = 0;
- if (pd->msg->len > 8)
+ if (i2c_check_msg_for_dma(pd->msg, 8, &pd->bounce_buf) == 0)
sh_mobile_i2c_xfer_dma(pd);
/* Enable all interrupts to begin with */
--
2.11.0
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 <[email protected]>
Signed-off-by: Wolfram Sang <[email protected]>
---
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
Signed-off-by: Wolfram Sang <[email protected]>
Signed-off-by: Wolfram Sang <[email protected]>
---
Documentation/i2c/DMA-considerations | 37 ++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
create mode 100644 Documentation/i2c/DMA-considerations
diff --git a/Documentation/i2c/DMA-considerations b/Documentation/i2c/DMA-considerations
new file mode 100644
index 00000000000000..92cdb6835ccf95
--- /dev/null
+++ b/Documentation/i2c/DMA-considerations
@@ -0,0 +1,37 @@
+Linux I2C and DMA
+-----------------
+
+Given that I2C is a low-speed bus where largely small messages are transferred,
+it is not considered a prime user of DMA access. At this time of writing, only
+10% of I2C bus master drivers have DMA support implemented. And the vast
+majority of transactions are so small that setting up DMA for it will likely
+add more overhead than a plain PIO transfer.
+
+Therefore, it is *not* mandatory that the buffer of an I2C message is DMA safe.
+It does not seem reasonable to apply additional burdens when the feature is so
+rarely used. However, it is recommended to use a DMA-safe buffer if your
+message size is likely applicable for DMA. Most drivers have this threshold
+around 8 bytes. As of today, this is mostly an educated guess, however.
+
+To support this scenario, drivers wishing to implement DMA can use helper
+functions from the I2C core. One checks if a message is DMA capable in terms of
+size and memory type. It can optionally also create a bounce buffer:
+
+ i2c_check_msg_for_dma(msg, threshold, &bounce_buf);
+
+The other one releases the bounce buffer ensuring data is copied back to the
+message:
+
+ i2c_release_bounce_buf(msg, bounce_buf);
+
+Please check the in-kernel documentation for details. The i2c-sh_mobile driver
+can be used as a reference example.
+
+The bounce buffer handling from the core is generic and simple. It will always
+allocate a new buffer. If you want a more sophisticated buffer handling (e.g.
+reusing pre-allocated buffers), you can skip the generic handling from the core
+and implement your own.
+
+If you plan to use DMA with I2C (or with any other bus, actually) make sure you
+have CONFIG_DMA_API_DEBUG enabled during development. It can help you find
+various issues which can be complex to debug otherwise.
--
2.11.0
One helper checks if DMA is suitable and optionally creates a bounce
buffer, if not. The other function returns the bounce buffer and makes
sure the data is properly copied back to the message.
Signed-off-by: Wolfram Sang <[email protected]>
Signed-off-by: Wolfram Sang <[email protected]>
---
include/linux/i2c.h | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 72d0ece70ed30d..7204d38eaff07c 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -33,7 +33,10 @@
#include <linux/rtmutex.h>
#include <linux/irqdomain.h> /* for Host Notify IRQ */
#include <linux/of.h> /* for struct device_node */
+#include <linux/slab.h>
#include <linux/swab.h> /* for swab16 */
+#include <linux/mm.h>
+#include <linux/sched/task_stack.h>
#include <uapi/linux/i2c.h>
extern struct bus_type i2c_bus_type;
@@ -764,6 +767,68 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
}
+/**
+ * i2c_check_msg_for_dma - check if a message is suitable for DMA
+ * @msg: the message to be checked
+ * @threshold: the amount of byte from which using DMA makes sense
+ * @ptr_for_bounce_buf: if not NULL, a bounce buffer will be attached to this
+ * ptr, if needed. The bounce buffer must be freed by the
+ * caller using i2c_release_bounce_buf().
+ *
+ * Return: -ERANGE if message is smaller than threshold
+ * -EFAULT if message buffer is not DMA capable and no bounce buffer
+ * was requested
+ * -ENOMEM if a bounce buffer could not be created
+ * 0 if message is suitable for DMA
+ *
+ * Note: This function should only be called from process context! It uses
+ * helper functions which work on the 'current' task.
+ */
+static inline int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
+ u8 **ptr_for_bounce_buf)
+{
+ if (ptr_for_bounce_buf)
+ *ptr_for_bounce_buf = NULL;
+
+ if (msg->len < threshold)
+ return -ERANGE;
+
+ if (!virt_addr_valid(msg->buf) || object_is_on_stack(msg->buf)) {
+ pr_debug("msg buffer to 0x%04x is not DMA safe%s\n", msg->addr,
+ ptr_for_bounce_buf ? ", trying bounce buffer" : "");
+ if (ptr_for_bounce_buf) {
+ if (msg->flags & I2C_M_RD)
+ *ptr_for_bounce_buf = kzalloc(msg->len, GFP_KERNEL);
+ else
+ *ptr_for_bounce_buf = kmemdup(msg->buf, msg->len,
+ GFP_KERNEL);
+ if (!*ptr_for_bounce_buf)
+ return -ENOMEM;
+ } else {
+ return -EFAULT;
+ }
+ }
+
+ return 0;
+}
+
+/**
+ * i2c_release_bounce_buf - copy data back from bounce buffer and release it
+ * @msg: the message to be copied back to
+ * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma().
+ * May be NULL.
+ */
+static inline void i2c_release_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf)
+{
+ if (!bounce_buf)
+ return;
+
+ if (msg->flags & I2C_M_RD)
+ memcpy(msg->buf, bounce_buf, msg->len);
+
+ kfree(bounce_buf);
+}
+
int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
/**
* module_i2c_driver() - Helper macro for registering a modular I2C driver
--
2.11.0
On Thu, Jun 15, 2017 at 08:30:35PM +0200, Wolfram Sang wrote:
> So, after revisiting old mail threads and taking part in a similar discussion
> on the USB list, here is what I cooked up to document and ease DMA handling for
> I2C within Linux. Please have a look at the documentation introduced in patch 2
> for further details.
>
> The branch can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma
>
> And big kudos to Renesas Electronics for funding this work, thank you very much!
I forgot to write that those patches have been tested with a Renesas Salvator-X
board (r8a7796/M3-W) and a Renesas Lager board (r8a7790/H2). A more
detailed test description can be found here:
http://elinux.org/Tests:I2C-core-DMA
And sorry for the doubled Signed-offs below the patches. My mistake
here, there should be only the ones from sang-engineering.com.
On Thu, Jun 15, 2017 at 12:30 PM, Wolfram Sang
<[email protected]> 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 <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> 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
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
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
>
Hi Wolfram,
On Thu, Jun 15, 2017 at 8:30 PM, Wolfram Sang
<[email protected]> wrote:
> --- /dev/null
> +++ b/Documentation/i2c/DMA-considerations
> @@ -0,0 +1,37 @@
> +Linux I2C and DMA
> +-----------------
> +
> +Given that I2C is a low-speed bus where largely small messages are transferred,
> +it is not considered a prime user of DMA access. At this time of writing, only
> +10% of I2C bus master drivers have DMA support implemented. And the vast
> +majority of transactions are so small that setting up DMA for it will likely
> +add more overhead than a plain PIO transfer.
> +
> +Therefore, it is *not* mandatory that the buffer of an I2C message is DMA safe.
> +It does not seem reasonable to apply additional burdens when the feature is so
> +rarely used. However, it is recommended to use a DMA-safe buffer if your
> +message size is likely applicable for DMA. Most drivers have this threshold
> +around 8 bytes. As of today, this is mostly an educated guess, however.
> +
> +To support this scenario, drivers wishing to implement DMA can use helper
> +functions from the I2C core. One checks if a message is DMA capable in terms of
> +size and memory type. It can optionally also create a bounce buffer:
> +
> + i2c_check_msg_for_dma(msg, threshold, &bounce_buf);
Obviously the return value must be checked before proceeding.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi Wolfram,
On Thu, Jun 15, 2017 at 8:30 PM, Wolfram Sang
<[email protected]> wrote:
> One helper checks if DMA is suitable and optionally creates a bounce
> buffer, if not. The other function returns the bounce buffer and makes
> sure the data is properly copied back to the message.
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -764,6 +767,68 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
> return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
> }
>
> +/**
> + * i2c_check_msg_for_dma - check if a message is suitable for DMA
> + * @msg: the message to be checked
> + * @threshold: the amount of byte from which using DMA makes sense
> + * @ptr_for_bounce_buf: if not NULL, a bounce buffer will be attached to this
> + * ptr, if needed. The bounce buffer must be freed by the
> + * caller using i2c_release_bounce_buf().
> + *
> + * Return: -ERANGE if message is smaller than threshold
> + * -EFAULT if message buffer is not DMA capable and no bounce buffer
> + * was requested
> + * -ENOMEM if a bounce buffer could not be created
> + * 0 if message is suitable for DMA
> + *
> + * Note: This function should only be called from process context! It uses
> + * helper functions which work on the 'current' task.
> + */
> +static inline int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
> + u8 **ptr_for_bounce_buf)
__must_check?
Isn't this function a bit large to be inlined?
> +/**
> + * i2c_release_bounce_buf - copy data back from bounce buffer and release it
> + * @msg: the message to be copied back to
> + * @bounce_buf: the bounce buffer obtained from i2c_check_msg_for_dma().
> + * May be NULL.
> + */
> +static inline void i2c_release_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf)
This one is smaller.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi Geert,
> > +static inline int i2c_check_msg_for_dma(struct i2c_msg *msg, unsigned int threshold,
> > + u8 **ptr_for_bounce_buf)
>
> __must_check?
Yes, why not.
> Isn't this function a bit large to be inlined?
That can be argued, yes. But I am unsure to bloat the core if the
function is so rarely needed...
> > +static inline void i2c_release_bounce_buf(struct i2c_msg *msg, u8 *bounce_buf)
>
> This one is smaller.
Yup, but of course, they should be put in the same place, whereever that may be.
Thanks,
Wolfram