[PATCH v2 0/3] Register i2c adapter driver earlier and use DMA safe buffers

[PATCH v2 0/3] Register i2c adapter driver earlier and use DMA safe buffers

[Jun Gao]

This patch series based on v4.18-rc1, include i2c adapter driver register time
modification, DMA safe buffer free function and DMA safe buffers used for i2c
transactions.

changes since v1:
- Add the initializations for DMA safe buffer pointers

Jun Gao (3):
  i2c: mediatek: Register i2c adapter driver earlier
  i2c: Add helper to ease DMA handling
  i2c: mediatek: Use DMA safe buffers for i2c transactions

 drivers/i2c/busses/i2c-mt65xx.c | 74 ++++++++++++++++++++++++++++++++++++-----
 drivers/i2c/i2c-core-base.c     | 14 ++++++++
 include/linux/i2c.h             |  1 +
 3 files changed, 81 insertions(+), 8 deletions(-)

--
1.8.1.1

[PATCH v2 1/3] i2c: mediatek: Register i2c adapter driver earlier

[Jun Gao]

From: Jun Gao <jun.gao@mediatek.com>

As i2c adapter, i2c slave devices will depend on it. In order not to
block the initializations of i2c slave devices, register i2c adapter
driver at appropriate time.

Signed-off-by: Jun Gao <jun.gao@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 1e57f58..806e8b90 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -888,7 +888,17 @@ static int mtk_i2c_resume(struct device *dev)
 	},
 };
 
-module_platform_driver(mtk_i2c_driver);
+static int __init mtk_i2c_adap_init(void)
+{
+	return platform_driver_register(&mtk_i2c_driver);
+}
+subsys_initcall(mtk_i2c_adap_init);
+
+static void __exit mtk_i2c_adap_exit(void)
+{
+	platform_driver_unregister(&mtk_i2c_driver);
+}
+module_exit(mtk_i2c_adap_exit);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("MediaTek I2C Bus Driver");
-- 
1.8.1.1

[PATCH v2 1/3] i2c: mediatek: Register i2c adapter driver earlier

[Wolfram Sang]

On Sat, Jul 07, 2018 at 05:29:53PM +0800, Jun Gao wrote:
> From: Jun Gao <jun.gao@mediatek.com>
> 
> As i2c adapter, i2c slave devices will depend on it. In order not to
> block the initializations of i2c slave devices, register i2c adapter
> driver at appropriate time.
> 
> Signed-off-by: Jun Gao <jun.gao@mediatek.com>

We don't do this anymore. Check these threads:

http://patchwork.ozlabs.org/project/linux-i2c/list/?series=&submitter=&state=*&q=initcall&archive=&delegate=

(/me just realized that patchwork makes it super easy to point people to
various discussions. Awesome!)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180808/befb8524/attachment.sig>

[PATCH v2 2/3] i2c: Add helper to ease DMA handling

[Jun Gao]

From: Jun Gao <jun.gao@mediatek.com>

This function is needed by i2c_get_dma_safe_msg_buf() potentially.
It is used to free DMA safe buffer when DMA operation fails.

Signed-off-by: Jun Gao <jun.gao@mediatek.com>
---
 drivers/i2c/i2c-core-base.c | 14 ++++++++++++++
 include/linux/i2c.h         |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 31d16ad..2b518ea 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2288,6 +2288,20 @@ void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
 }
 EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
 
+/**
+ * i2c_free_dma_safe_msg_buf - free DMA safe buffer
+ * @msg: the message related to DMA safe buffer
+ * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
+ */
+void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
+{
+	if (!buf || buf == msg->buf)
+		return;
+
+	kfree(buf);
+}
+EXPORT_SYMBOL_GPL(i2c_free_dma_safe_msg_buf);
+
 MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>");
 MODULE_DESCRIPTION("I2C-Bus main module");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 254cd34..6d62f93 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -860,6 +860,7 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
 
 u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold);
 void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
+void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
 
 int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
 /**
-- 
1.8.1.1

[PATCH v2 2/3] i2c: Add helper to ease DMA handling

[Peter Rosin]

On 2018-07-07 11:29, Jun Gao wrote:
> From: Jun Gao <jun.gao@mediatek.com>
> 
> This function is needed by i2c_get_dma_safe_msg_buf() potentially.
> It is used to free DMA safe buffer when DMA operation fails.
> 
> Signed-off-by: Jun Gao <jun.gao@mediatek.com>
> ---
>  drivers/i2c/i2c-core-base.c | 14 ++++++++++++++
>  include/linux/i2c.h         |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 31d16ad..2b518ea 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2288,6 +2288,20 @@ void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
>  }
>  EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
>  
> +/**
> + * i2c_free_dma_safe_msg_buf - free DMA safe buffer
> + * @msg: the message related to DMA safe buffer
> + * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
> + */
> +void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
> +{
> +	if (!buf || buf == msg->buf)
> +		return;
> +
> +	kfree(buf);

Considering that the i2c-core-smbus.c file does the following for its
DMA safe buffers...

static void i2c_smbus_try_get_dmabuf(struct i2c_msg *msg, u8 init_val)
{
	bool is_read = msg->flags & I2C_M_RD;
	unsigned char *dma_buf;

	dma_buf = kzalloc(I2C_SMBUS_BLOCK_MAX + (is_read ? 2 : 3), GFP_KERNEL);
	if (!dma_buf)
		return;

	msg->buf = dma_buf;
	msg->flags |= I2C_M_DMA_SAFE;

	if (init_val)
		msg->buf[0] = init_val;
}

...I do not think your variant of i2c_release_dma_safe_msg_buf is
appropriate for the i2c-core-base.c file. It's simply not possible to have
central knowledge of the rules for when to free the buffer, and encoding one
set of rules is only confusing (when there are more than one set of rules).
I suggest that you make your variant driver specific.

Cheers,
Peter

> +}
> +EXPORT_SYMBOL_GPL(i2c_free_dma_safe_msg_buf);
> +
>  MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>");
>  MODULE_DESCRIPTION("I2C-Bus main module");
>  MODULE_LICENSE("GPL");
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 254cd34..6d62f93 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -860,6 +860,7 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
>  
>  u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold);
>  void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
> +void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
>  
>  int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
>  /**
> 

[PATCH v2 2/3] i2c: Add helper to ease DMA handling

[Peter Rosin]

On 2018-07-08 13:58, Peter Rosin wrote:
> On 2018-07-07 11:29, Jun Gao wrote:
>> From: Jun Gao <jun.gao@mediatek.com>
>>
>> This function is needed by i2c_get_dma_safe_msg_buf() potentially.
>> It is used to free DMA safe buffer when DMA operation fails.
>>
>> Signed-off-by: Jun Gao <jun.gao@mediatek.com>
>> ---
>>  drivers/i2c/i2c-core-base.c | 14 ++++++++++++++
>>  include/linux/i2c.h         |  1 +
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 31d16ad..2b518ea 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -2288,6 +2288,20 @@ void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
>>  }
>>  EXPORT_SYMBOL_GPL(i2c_release_dma_safe_msg_buf);
>>  
>> +/**
>> + * i2c_free_dma_safe_msg_buf - free DMA safe buffer
>> + * @msg: the message related to DMA safe buffer
>> + * @buf: the buffer obtained from i2c_get_dma_safe_msg_buf(). May be NULL.
>> + */
>> +void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf)
>> +{
>> +	if (!buf || buf == msg->buf)
>> +		return;
>> +
>> +	kfree(buf);
> 
> Considering that the i2c-core-smbus.c file does the following for its
> DMA safe buffers...
> 
> static void i2c_smbus_try_get_dmabuf(struct i2c_msg *msg, u8 init_val)
> {
> 	bool is_read = msg->flags & I2C_M_RD;
> 	unsigned char *dma_buf;
> 
> 	dma_buf = kzalloc(I2C_SMBUS_BLOCK_MAX + (is_read ? 2 : 3), GFP_KERNEL);
> 	if (!dma_buf)
> 		return;
> 
> 	msg->buf = dma_buf;
> 	msg->flags |= I2C_M_DMA_SAFE;
> 
> 	if (init_val)
> 		msg->buf[0] = init_val;
> }
> 
> ...I do not think your variant of i2c_release_dma_safe_msg_buf is
> appropriate for the i2c-core-base.c file. It's simply not possible to have
> central knowledge of the rules for when to free the buffer, and encoding one
> set of rules is only confusing (when there are more than one set of rules).
> I suggest that you make your variant driver specific.

Ignore me. Your i2c_free_dma_safe_msg_buf is of course compatible with
the existing i2c_smbus_try_get_dmabuf. Sorry for the noise.

Cheers,
Peter

( However, the naming of these two functions are not really consistent... )

> Cheers,
> Peter
> 
>> +}
>> +EXPORT_SYMBOL_GPL(i2c_free_dma_safe_msg_buf);
>> +
>>  MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>");
>>  MODULE_DESCRIPTION("I2C-Bus main module");
>>  MODULE_LICENSE("GPL");
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 254cd34..6d62f93 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -860,6 +860,7 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
>>  
>>  u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold);
>>  void i2c_release_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
>> +void i2c_free_dma_safe_msg_buf(struct i2c_msg *msg, u8 *buf);
>>  
>>  int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
>>  /**
>>
> 

[PATCH v2 2/3] i2c: Add helper to ease DMA handling

[Wolfram Sang]

On Sat, Jul 07, 2018 at 05:29:54PM +0800, Jun Gao wrote:
> From: Jun Gao <jun.gao@mediatek.com>
> 
> This function is needed by i2c_get_dma_safe_msg_buf() potentially.
> It is used to free DMA safe buffer when DMA operation fails.
> 
> Signed-off-by: Jun Gao <jun.gao@mediatek.com>

Right, we need something like this. This leaks in the sh_mobile driver,
too :(

I am still thinking if there is a nice way to put this functionality
into i2c_release_dma_safe_msg_buf() itself somehow...

Thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180808/3819ae01/attachment.sig>

[PATCH v2 2/3] i2c: Add helper to ease DMA handling

[Yingjoe Chen]

On Wed, 2018-08-08 at 22:57 +0200, Wolfram Sang wrote:
> On Sat, Jul 07, 2018 at 05:29:54PM +0800, Jun Gao wrote:
> > From: Jun Gao <jun.gao@mediatek.com>
> > 
> > This function is needed by i2c_get_dma_safe_msg_buf() potentially.
> > It is used to free DMA safe buffer when DMA operation fails.
> > 
> > Signed-off-by: Jun Gao <jun.gao@mediatek.com>
> 
> Right, we need something like this. This leaks in the sh_mobile driver,
> too :(
> 
> I am still thinking if there is a nice way to put this functionality
> into i2c_release_dma_safe_msg_buf() itself somehow...

Wolfram,

I have second thought on these API now. Recently, we have saw similar
issue for spi device driver.

I believe the reason for these api is because some arch changed to can
not do DMA on stack recently. Maybe we should have dma_mapping to bounce
buffer like it bounce un-dma-able address for those arch? or we should
have a common driver API for this, not just for i2c?

Joe.C

[PATCH v2 2/3] i2c: Add helper to ease DMA handling

[Wolfram Sang]

Hi,

> I believe the reason for these api is because some arch changed to can
> not do DMA on stack recently. Maybe we should have dma_mapping to bounce

It is not only arch dependent. You can enable virtual stacks in Kconfig,
too.

> buffer like it bounce un-dma-able address for those arch? or we should
> have a common driver API for this, not just for i2c?

I gave a talk about this problem recently and everyone in the room
agreed the best thing would be to have annotated buffers which can be
used kernel-wide. Someone mentioned DMABUF could be a candidate, but I
haven't looked further into that.

So, yes, a bigger solution is needed but I don't see that coming soon,
so I implemented the I2C part for better safety now. I'd be happy to
remove all that again once we have the annotated buffers. But it is an
effort...

Regards,

   Wolfram

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180809/c58411fe/attachment.sig>

[PATCH v2 3/3] i2c: mediatek: Use DMA safe buffers for i2c transactions

[Jun Gao]

From: Jun Gao <jun.gao@mediatek.com>

DMA mode will always be used in i2c transactions, try to allocate
a DMA safe buffer if the buf of struct i2c_msg used is not DMA safe.

Signed-off-by: Jun Gao <jun.gao@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 62 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 806e8b90..c92cae7 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -441,6 +441,8 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 	u16 control_reg;
 	u16 restart_flag = 0;
 	u32 reg_4g_mode;
+	u8 *dma_rd_buf = NULL;
+	u8 *dma_wr_buf = NULL;
 	dma_addr_t rpaddr = 0;
 	dma_addr_t wpaddr = 0;
 	int ret;
@@ -500,10 +502,18 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 	if (i2c->op == I2C_MASTER_RD) {
 		writel(I2C_DMA_INT_FLAG_NONE, i2c->pdmabase + OFFSET_INT_FLAG);
 		writel(I2C_DMA_CON_RX, i2c->pdmabase + OFFSET_CON);
-		rpaddr = dma_map_single(i2c->dev, msgs->buf,
+
+		dma_rd_buf = i2c_get_dma_safe_msg_buf(msgs, 0);
+		if (!dma_rd_buf)
+			return -ENOMEM;
+
+		rpaddr = dma_map_single(i2c->dev, dma_rd_buf,
 					msgs->len, DMA_FROM_DEVICE);
-		if (dma_mapping_error(i2c->dev, rpaddr))
+		if (dma_mapping_error(i2c->dev, rpaddr)) {
+			i2c_free_dma_safe_msg_buf(msgs, dma_rd_buf);
+
 			return -ENOMEM;
+		}
 
 		if (i2c->dev_comp->support_33bits) {
 			reg_4g_mode = mtk_i2c_set_4g_mode(rpaddr);
@@ -515,10 +525,18 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 	} else if (i2c->op == I2C_MASTER_WR) {
 		writel(I2C_DMA_INT_FLAG_NONE, i2c->pdmabase + OFFSET_INT_FLAG);
 		writel(I2C_DMA_CON_TX, i2c->pdmabase + OFFSET_CON);
-		wpaddr = dma_map_single(i2c->dev, msgs->buf,
+
+		dma_wr_buf = i2c_get_dma_safe_msg_buf(msgs, 0);
+		if (!dma_wr_buf)
+			return -ENOMEM;
+
+		wpaddr = dma_map_single(i2c->dev, dma_wr_buf,
 					msgs->len, DMA_TO_DEVICE);
-		if (dma_mapping_error(i2c->dev, wpaddr))
+		if (dma_mapping_error(i2c->dev, wpaddr)) {
+			i2c_free_dma_safe_msg_buf(msgs, dma_wr_buf);
+
 			return -ENOMEM;
+		}
 
 		if (i2c->dev_comp->support_33bits) {
 			reg_4g_mode = mtk_i2c_set_4g_mode(wpaddr);
@@ -530,16 +548,39 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 	} else {
 		writel(I2C_DMA_CLR_FLAG, i2c->pdmabase + OFFSET_INT_FLAG);
 		writel(I2C_DMA_CLR_FLAG, i2c->pdmabase + OFFSET_CON);
-		wpaddr = dma_map_single(i2c->dev, msgs->buf,
+
+		dma_wr_buf = i2c_get_dma_safe_msg_buf(msgs, 0);
+		if (!dma_wr_buf)
+			return -ENOMEM;
+
+		wpaddr = dma_map_single(i2c->dev, dma_wr_buf,
 					msgs->len, DMA_TO_DEVICE);
-		if (dma_mapping_error(i2c->dev, wpaddr))
+		if (dma_mapping_error(i2c->dev, wpaddr)) {
+			i2c_free_dma_safe_msg_buf(msgs, dma_wr_buf);
+
 			return -ENOMEM;
-		rpaddr = dma_map_single(i2c->dev, (msgs + 1)->buf,
+		}
+
+		dma_rd_buf = i2c_get_dma_safe_msg_buf((msgs + 1), 0);
+		if (!dma_rd_buf) {
+			dma_unmap_single(i2c->dev, wpaddr,
+					 msgs->len, DMA_TO_DEVICE);
+
+			i2c_free_dma_safe_msg_buf(msgs, dma_wr_buf);
+
+			return -ENOMEM;
+		}
+
+		rpaddr = dma_map_single(i2c->dev, dma_rd_buf,
 					(msgs + 1)->len,
 					DMA_FROM_DEVICE);
 		if (dma_mapping_error(i2c->dev, rpaddr)) {
 			dma_unmap_single(i2c->dev, wpaddr,
 					 msgs->len, DMA_TO_DEVICE);
+
+			i2c_free_dma_safe_msg_buf(msgs, dma_wr_buf);
+			i2c_free_dma_safe_msg_buf((msgs + 1), dma_rd_buf);
+
 			return -ENOMEM;
 		}
 
@@ -578,14 +619,21 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 	if (i2c->op == I2C_MASTER_WR) {
 		dma_unmap_single(i2c->dev, wpaddr,
 				 msgs->len, DMA_TO_DEVICE);
+
+		i2c_release_dma_safe_msg_buf(msgs, dma_wr_buf);
 	} else if (i2c->op == I2C_MASTER_RD) {
 		dma_unmap_single(i2c->dev, rpaddr,
 				 msgs->len, DMA_FROM_DEVICE);
+
+		i2c_release_dma_safe_msg_buf(msgs, dma_rd_buf);
 	} else {
 		dma_unmap_single(i2c->dev, wpaddr, msgs->len,
 				 DMA_TO_DEVICE);
 		dma_unmap_single(i2c->dev, rpaddr, (msgs + 1)->len,
 				 DMA_FROM_DEVICE);
+
+		i2c_release_dma_safe_msg_buf(msgs, dma_wr_buf);
+		i2c_release_dma_safe_msg_buf((msgs + 1), dma_rd_buf);
 	}
 
 	if (ret == 0) {
-- 
1.8.1.1