[PATCH 00/11] i2c: imx-lpi2c: New features and bug fixes

Hi,

Here are some patches to add some new features and bug fixes/improvements.
Each patch of these 11 patches is made on the basis of the previous patches.

New features:
 - add bus recovery support
 - add edma mode support
 - add runtime pm support

Improvements:
 - increase PM timeout to avoid operate clk frequently
 - manage irq resource request/release in runtime pm
 - directly retrun ISR when detect a NACK
 - improve i2c driver probe priority
 - add debug message

Bug fixes:
 - fix i2c timing issue
 - fix type char overflow issue when calculating the clock cycle
 - add the missing ipg clk

Best Regards,
Clark Wang

 drivers/i2c/busses/i2c-imx-lpi2c.c | 536 +++++++++++++++++++++++++----
 1 file changed, 478 insertions(+), 58 deletions(-)

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Gao Pan <redacted>

A NACK flag in ISR means i2c bus error. In such codition,
there is no need to do read/write operation. It's better
to return ISR directly and then stop i2c transfer.

Signed-off-by: Gao Pan <redacted>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 9db6ccded5e9..bbf44ac95021 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -507,15 +507,17 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
 	lpi2c_imx_intctrl(lpi2c_imx, 0);
 	temp = readl(lpi2c_imx->base + LPI2C_MSR);
 
+	if (temp & MSR_NDF) {
+		complete(&lpi2c_imx->complete);
+		goto ret;
+	}
+
 	if (temp & MSR_RDF)
 		lpi2c_imx_read_rxfifo(lpi2c_imx);
-
-	if (temp & MSR_TDF)
+	else if (temp & MSR_TDF)
 		lpi2c_imx_write_txfifo(lpi2c_imx);
 
-	if (temp & MSR_NDF)
-		complete(&lpi2c_imx->complete);
-
+ret:
 	return IRQ_HANDLED;
 }
 

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM

A NACK flag in ISR means i2c bus error. In such codition, there is no need to do
read/write operation. It's better to return ISR directly and then stop i2c
transfer.

Signed-off-by: Gao Pan <redacted>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

Regards
Aisheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The lpi2c IP needs two clks: ipg clk and per clk. The old lpi2c
driver missed ipg clk. This patch adds ipg clk for lpi2c driver.

Signed-off-by: Gao Pan <redacted>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Acked-by: Fugang Duan <redacted>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 1e920e7ac7c1..664fcc0dba51 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -94,7 +94,8 @@ enum lpi2c_imx_pincfg {
 
 struct lpi2c_imx_struct {
 	struct i2c_adapter	adapter;
-	struct clk		*clk;
+	struct clk		*clk_per;
+	struct clk		*clk_ipg;
 	void __iomem		*base;
 	__u8			*rx_buf;
 	__u8			*tx_buf;

@@ -563,10 +564,16 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	strlcpy(lpi2c_imx->adapter.name, pdev->name,
 		sizeof(lpi2c_imx->adapter.name));
 
-	lpi2c_imx->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(lpi2c_imx->clk)) {
+	lpi2c_imx->clk_per = devm_clk_get(&pdev->dev, "per");
+	if (IS_ERR(lpi2c_imx->clk_per)) {
 		dev_err(&pdev->dev, "can't get I2C peripheral clock\n");
-		return PTR_ERR(lpi2c_imx->clk);
+		return PTR_ERR(lpi2c_imx->clk_per);
+	}
+
+	lpi2c_imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
+	if (IS_ERR(lpi2c_imx->clk_ipg)) {
+		dev_err(&pdev->dev, "can't get I2C ipg clock\n");
+		return PTR_ERR(lpi2c_imx->clk_ipg);
 	}
 
 	ret = of_property_read_u32(pdev->dev.of_node,

@@ -633,7 +640,8 @@ static int __maybe_unused lpi2c_runtime_suspend(struct device *dev)
 {
 	struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);
 
-	clk_disable_unprepare(lpi2c_imx->clk);
+	clk_disable_unprepare(lpi2c_imx->clk_ipg);
+	clk_disable_unprepare(lpi2c_imx->clk_per);
 	pinctrl_pm_select_idle_state(dev);
 
 	return 0;

@@ -645,12 +653,18 @@ static int __maybe_unused lpi2c_runtime_resume(struct device *dev)
 	int ret;
 
 	pinctrl_pm_select_default_state(dev);
-	ret = clk_prepare_enable(lpi2c_imx->clk);
+	ret = clk_prepare_enable(lpi2c_imx->clk_per);
 	if (ret) {
-		dev_err(dev, "can't enable I2C clock, ret=%d\n", ret);
+		dev_err(dev, "can't enable I2C per clock, ret=%d\n", ret);
 		return ret;
 	}
 
+	ret = clk_prepare_enable(lpi2c_imx->clk_ipg);
+	if (ret) {
+		clk_disable_unprepare(lpi2c_imx->clk_per);
+		dev_err(dev, "can't enable I2C ipg clock, ret=%d\n", ret);
+	}
+
 	return ret;
 }
 

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM

The lpi2c IP needs two clks: ipg clk and per clk. The old lpi2c driver missed ipg
clk. This patch adds ipg clk for lpi2c driver.

Pleas also update dt-binding and sent along with this patchset.(before this one)

Signed-off-by: Gao Pan <redacted>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Acked-by: Fugang Duan <redacted>

You can drop the Ack tag if the patch was changed 

Will this break exist dts?

Regards
Aisheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-----Original Message-----
From: Aisheng Dong <aisheng.dong@nxp.com>
Sent: Friday, March 19, 2021 12:46
To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org;
s.hauer@pengutronix.de
Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org
Subject: RE: [PATCH 03/11] i2c: imx-lpi2c: add ipg clk for lpi2c driver

quoted

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM

The lpi2c IP needs two clks: ipg clk and per clk. The old lpi2c driver
missed ipg clk. This patch adds ipg clk for lpi2c driver.

Pleas also update dt-binding and sent along with this patchset.(before this
one)

Okay, thanks.

quoted

Signed-off-by: Gao Pan <redacted>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Acked-by: Fugang Duan <redacted>

You can drop the Ack tag if the patch was changed

quoted

---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c

b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 1e920e7ac7c1..664fcc0dba51 100644

--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -94,7 +94,8 @@ enum lpi2c_imx_pincfg {

 struct lpi2c_imx_struct {
 	struct i2c_adapter	adapter;
-	struct clk		*clk;
+	struct clk		*clk_per;
+	struct clk		*clk_ipg;
 	void __iomem		*base;
 	__u8			*rx_buf;
 	__u8			*tx_buf;

@@ -563,10 +564,16 @@ static int lpi2c_imx_probe(struct

platform_device
*pdev)
 	strlcpy(lpi2c_imx->adapter.name, pdev->name,
 		sizeof(lpi2c_imx->adapter.name));

-	lpi2c_imx->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(lpi2c_imx->clk)) {
+	lpi2c_imx->clk_per = devm_clk_get(&pdev->dev, "per");
+	if (IS_ERR(lpi2c_imx->clk_per)) {
 		dev_err(&pdev->dev, "can't get I2C peripheral clock\n");
-		return PTR_ERR(lpi2c_imx->clk);
+		return PTR_ERR(lpi2c_imx->clk_per);
+	}
+
+	lpi2c_imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
+	if (IS_ERR(lpi2c_imx->clk_ipg)) {
+		dev_err(&pdev->dev, "can't get I2C ipg clock\n");
+		return PTR_ERR(lpi2c_imx->clk_ipg);
 	}

Will this break exist dts?

It will not break the build. But will break the lpi2c probe for imx7ulp and 
imx8qxp/qm.
I will send two patches to update dts in V2.

Best Regards,
Clark Wang

Regards
Aisheng

quoted

 	ret = of_property_read_u32(pdev->dev.of_node,

@@ -633,7 +640,8 @@ static int __maybe_unused

lpi2c_runtime_suspend(struct device *dev)  {
 	struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);

-	clk_disable_unprepare(lpi2c_imx->clk);
+	clk_disable_unprepare(lpi2c_imx->clk_ipg);
+	clk_disable_unprepare(lpi2c_imx->clk_per);
 	pinctrl_pm_select_idle_state(dev);

 	return 0;

@@ -645,12 +653,18 @@ static int __maybe_unused

lpi2c_runtime_resume(struct device *dev)
 	int ret;

 	pinctrl_pm_select_default_state(dev);
-	ret = clk_prepare_enable(lpi2c_imx->clk);
+	ret = clk_prepare_enable(lpi2c_imx->clk_per);
 	if (ret) {
-		dev_err(dev, "can't enable I2C clock, ret=%d\n", ret);
+		dev_err(dev, "can't enable I2C per clock, ret=%d\n", ret);
 		return ret;
 	}

+	ret = clk_prepare_enable(lpi2c_imx->clk_ipg);
+	if (ret) {
+		clk_disable_unprepare(lpi2c_imx->clk_per);
+		dev_err(dev, "can't enable I2C ipg clock, ret=%d\n", ret);
+	}
+
 	return ret;
 }

--
2.25.1

quoted

quoted

+
+	lpi2c_imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
+	if (IS_ERR(lpi2c_imx->clk_ipg)) {
+		dev_err(&pdev->dev, "can't get I2C ipg clock\n");
+		return PTR_ERR(lpi2c_imx->clk_ipg);
 	}

Will this break exist dts?

It will not break the build. But will break the lpi2c probe for imx7ulp and
imx8qxp/qm.
I will send two patches to update dts in V2.

Please don't break exist platforms.

Regards
Aisheng

Best Regards,
Clark Wang

quoted

Regards
Aisheng

quoted

 	ret = of_property_read_u32(pdev->dev.of_node,

@@ -633,7 +640,8 @@ static int __maybe_unused

lpi2c_runtime_suspend(struct device *dev)  {
 	struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);

-	clk_disable_unprepare(lpi2c_imx->clk);
+	clk_disable_unprepare(lpi2c_imx->clk_ipg);
+	clk_disable_unprepare(lpi2c_imx->clk_per);
 	pinctrl_pm_select_idle_state(dev);

 	return 0;

@@ -645,12 +653,18 @@ static int __maybe_unused

lpi2c_runtime_resume(struct device *dev)
 	int ret;

 	pinctrl_pm_select_default_state(dev);
-	ret = clk_prepare_enable(lpi2c_imx->clk);
+	ret = clk_prepare_enable(lpi2c_imx->clk_per);
 	if (ret) {
-		dev_err(dev, "can't enable I2C clock, ret=%d\n", ret);
+		dev_err(dev, "can't enable I2C per clock, ret=%d\n", ret);
 		return ret;
 	}

+	ret = clk_prepare_enable(lpi2c_imx->clk_ipg);
+	if (ret) {
+		clk_disable_unprepare(lpi2c_imx->clk_per);
+		dev_err(dev, "can't enable I2C ipg clock, ret=%d\n", ret);
+	}
+
 	return ret;
 }

--
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Gao Pan <redacted>

add debug message when i2c peripheral clk rate is 0, then
directly return -EINVAL.

Signed-off-by: Gao Pan <redacted>
Reviewed-by: Andy Duan <redacted>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index e718bb6b2387..8f9dd3dd2951 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -209,7 +209,12 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
 
 	lpi2c_imx_set_mode(lpi2c_imx);
 
-	clk_rate = clk_get_rate(lpi2c_imx->clk);
+	clk_rate = clk_get_rate(lpi2c_imx->clk_per);
+	if (!clk_rate) {
+		dev_dbg(&lpi2c_imx->adapter.dev, "clk_per rate is 0\n");
+		return -EINVAL;
+	}
+
 	if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
 		filt = 0;
 	else

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM

add debug message when i2c peripheral clk rate is 0, then directly return
-EINVAL.

Signed-off-by: Gao Pan <redacted>
Reviewed-by: Andy Duan <redacted>

Drop old review when patch is changed

I guess the kernel can't compile right before this patch because lpi2c_imx->clk was
Removed In former patch
You need double check not break bisect

+	clk_rate = clk_get_rate(lpi2c_imx->clk_per);
+	if (!clk_rate) {
+		dev_dbg(&lpi2c_imx->adapter.dev, "clk_per rate is 0\n");

s/dev_dbg/dev_err

+		return -EINVAL;
+	}
+
 	if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
 		filt = 0;
 	else
--
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-----Original Message-----
From: Aisheng Dong <aisheng.dong@nxp.com>
Sent: Friday, March 19, 2021 12:57
To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org;
s.hauer@pengutronix.de
Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org
Subject: RE: [PATCH 05/11] i2c: imx-lpi2c: add debug message when i2c
peripheral clk doesn't work

quoted

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM

add debug message when i2c peripheral clk rate is 0, then directly
return -EINVAL.

Signed-off-by: Gao Pan <redacted>
Reviewed-by: Andy Duan <redacted>

Drop old review when patch is changed

quoted

---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c

b/drivers/i2c/busses/i2c-imx-lpi2c.c
index e718bb6b2387..8f9dd3dd2951 100644

--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -209,7 +209,12 @@ static int lpi2c_imx_config(struct

lpi2c_imx_struct
*lpi2c_imx)

 	lpi2c_imx_set_mode(lpi2c_imx);

-	clk_rate = clk_get_rate(lpi2c_imx->clk);

I guess the kernel can't compile right before this patch because lpi2c_imx-

quoted

clk was Removed In former patch You need double check not break bisect

Oh, sorry, I miss this clk definition here.
I will fix this in V2.

quoted

+	clk_rate = clk_get_rate(lpi2c_imx->clk_per);
+	if (!clk_rate) {
+		dev_dbg(&lpi2c_imx->adapter.dev, "clk_per rate is 0\n");

s/dev_dbg/dev_err

Will change to dev_err.
Thanks.


Best Regards,
Clark Wang

quoted

+		return -EINVAL;
+	}
+
 	if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
 		filt = 0;
 	else
--
2.25.1

From: Fugang Duan <redacted>

- Add runtime pm support to dynamicly manage the clock.
- Put the suspend to suspend_noirq.
- Call .pm_runtime_force_suspend() to force runtime pm suspended
  in .suspend_noirq().

Signed-off-by: Fugang Duan <redacted>
Signed-off-by: Gao Pan <redacted>
Reviewed-by: Anson Huang <redacted>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 50 ++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index bbf44ac95021..1e920e7ac7c1 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	if (ret)
 		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;
 
-	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
+	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr,
+			       IRQF_NO_SUSPEND,
 			       pdev->name, lpi2c_imx);
 	if (ret) {
 		dev_err(&pdev->dev, "can't claim irq %d\n", irq);

@@ -584,35 +585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);
 
-	ret = clk_prepare_enable(lpi2c_imx->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "clk enable failed %d\n", ret);
-		return ret;
-	}
-
 	pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
 	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_get_noresume(&pdev->dev);
-	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
+		dev_err(&pdev->dev, "failed to enable clock\n");
+		return ret;
+	}
+
 	temp = readl(lpi2c_imx->base + LPI2C_PARAM);
 	lpi2c_imx->txfifosize = 1 << (temp & 0x0f);
 	lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f);
 
+	pm_runtime_put(&pdev->dev);
+
 	ret = i2c_add_adapter(&lpi2c_imx->adapter);
 	if (ret)
 		goto rpm_disable;
 
-	pm_runtime_mark_last_busy(&pdev->dev);
-	pm_runtime_put_autosuspend(&pdev->dev);
-
 	dev_info(&lpi2c_imx->adapter.dev, "LPI2C adapter registered\n");
 
 	return 0;
 
 rpm_disable:
-	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 

@@ -636,7 +634,7 @@ static int __maybe_unused lpi2c_runtime_suspend(struct device *dev)
 	struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);
 
 	clk_disable_unprepare(lpi2c_imx->clk);
-	pinctrl_pm_select_sleep_state(dev);
+	pinctrl_pm_select_idle_state(dev);
 
 	return 0;
 }

@@ -649,16 +647,34 @@ static int __maybe_unused lpi2c_runtime_resume(struct device *dev)
 	pinctrl_pm_select_default_state(dev);
 	ret = clk_prepare_enable(lpi2c_imx->clk);
 	if (ret) {
-		dev_err(dev, "failed to enable I2C clock, ret=%d\n", ret);
+		dev_err(dev, "can't enable I2C clock, ret=%d\n", ret);
 		return ret;
 	}
 
+	return ret;
+}
+
+static int lpi2c_suspend_noirq(struct device *dev)
+{
+	int ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
+
+	pinctrl_pm_select_sleep_state(dev);
+
 	return 0;
 }
 
+static int lpi2c_resume_noirq(struct device *dev)
+{
+	return pm_runtime_force_resume(dev);
+}
+
 static const struct dev_pm_ops lpi2c_pm_ops = {
-	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				      pm_runtime_force_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpi2c_suspend_noirq,
+				     lpi2c_resume_noirq)
 	SET_RUNTIME_PM_OPS(lpi2c_runtime_suspend,
 			   lpi2c_runtime_resume, NULL)
 };

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM
Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

- Add runtime pm support to dynamicly manage the clock.
- Put the suspend to suspend_noirq.
- Call .pm_runtime_force_suspend() to force runtime pm suspended
  in .suspend_noirq().

The patch title needs to be improved as the driver already supports rpm.
And do one thing in one patch.

Signed-off-by: Fugang Duan <redacted>
Signed-off-by: Gao Pan <redacted>
Reviewed-by: Anson Huang <redacted>

Please add your sign-off.

This belongs to a separate patch

 			       pdev->name, lpi2c_imx);
 	if (ret) {
 		dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ -584,35
+585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);

-	ret = clk_prepare_enable(lpi2c_imx->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "clk enable failed %d\n", ret);
-		return ret;
-	}
-
 	pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
 	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_get_noresume(&pdev->dev);
-	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);

+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
+		dev_err(&pdev->dev, "failed to enable clock\n");
+		return ret;
+	}

Can't current clk control via rpm work well?
Please describe why need change.

This belongs to a separate patch

Probably unnecessary change

 		return ret;
 	}

+	return ret;
+}
+
+static int lpi2c_suspend_noirq(struct device *dev) {
+	int ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
+
+	pinctrl_pm_select_sleep_state(dev);
+
 	return 0;
 }

+static int lpi2c_resume_noirq(struct device *dev) {
+	return pm_runtime_force_resume(dev);
+}
+
 static const struct dev_pm_ops lpi2c_pm_ops = {
-	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				      pm_runtime_force_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpi2c_suspend_noirq,
+				     lpi2c_resume_noirq)

Belongs to separate change and explain why need do this

 	SET_RUNTIME_PM_OPS(lpi2c_runtime_suspend,
 			   lpi2c_runtime_resume, NULL)
 };
--
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-----Original Message-----
From: Aisheng Dong <aisheng.dong@nxp.com>
Sent: Friday, March 19, 2021 12:40
To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org;
s.hauer@pengutronix.de
Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org
Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

quoted

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM
Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

- Add runtime pm support to dynamicly manage the clock.
- Put the suspend to suspend_noirq.
- Call .pm_runtime_force_suspend() to force runtime pm suspended
  in .suspend_noirq().

The patch title needs to be improved as the driver already supports rpm.
And do one thing in one patch.

Thanks. I will split this patch into several and resend.

Best Regards,
Clark Wang

quoted

Signed-off-by: Fugang Duan <redacted>
Signed-off-by: Gao Pan <redacted>
Reviewed-by: Anson Huang <redacted>

Please add your sign-off.

quoted

---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 50
++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c

b/drivers/i2c/busses/i2c-imx-lpi2c.c
index bbf44ac95021..1e920e7ac7c1 100644

--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device

*pdev)
 	if (ret)
 		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;

-	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
+	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr,
+			       IRQF_NO_SUSPEND,

This belongs to a separate patch

quoted

 			       pdev->name, lpi2c_imx);
 	if (ret) {
 		dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ -

584,35

quoted

+585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);

-	ret = clk_prepare_enable(lpi2c_imx->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "clk enable failed %d\n", ret);
-		return ret;
-	}
-
 	pm_runtime_set_autosuspend_delay(&pdev->dev,

I2C_PM_TIMEOUT);

quoted

 	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_get_noresume(&pdev->dev);
-	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);

+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
+		dev_err(&pdev->dev, "failed to enable clock\n");
+		return ret;
+	}

Can't current clk control via rpm work well?
Please describe why need change.

quoted

+
 	temp = readl(lpi2c_imx->base + LPI2C_PARAM);
 	lpi2c_imx->txfifosize = 1 << (temp & 0x0f);
 	lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f);

+	pm_runtime_put(&pdev->dev);
+
 	ret = i2c_add_adapter(&lpi2c_imx->adapter);
 	if (ret)
 		goto rpm_disable;

-	pm_runtime_mark_last_busy(&pdev->dev);
-	pm_runtime_put_autosuspend(&pdev->dev);
-
 	dev_info(&lpi2c_imx->adapter.dev, "LPI2C adapter registered\n");

 	return 0;

 rpm_disable:
-	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);

@@ -636,7 +634,7 @@ static int __maybe_unused

lpi2c_runtime_suspend(struct device *dev)
 	struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);

 	clk_disable_unprepare(lpi2c_imx->clk);
-	pinctrl_pm_select_sleep_state(dev);
+	pinctrl_pm_select_idle_state(dev);

This belongs to a separate patch

quoted

 	return 0;
 }

@@ -649,16 +647,34 @@ static int __maybe_unused

lpi2c_runtime_resume(struct device *dev)
 	pinctrl_pm_select_default_state(dev);
 	ret = clk_prepare_enable(lpi2c_imx->clk);
 	if (ret) {
-		dev_err(dev, "failed to enable I2C clock, ret=%d\n", ret);
+		dev_err(dev, "can't enable I2C clock, ret=%d\n", ret);

Probably unnecessary change

quoted

 		return ret;
 	}

+	return ret;
+}
+
+static int lpi2c_suspend_noirq(struct device *dev) {
+	int ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
+
+	pinctrl_pm_select_sleep_state(dev);
+
 	return 0;
 }

+static int lpi2c_resume_noirq(struct device *dev) {
+	return pm_runtime_force_resume(dev); }
+
 static const struct dev_pm_ops lpi2c_pm_ops = {
-	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				      pm_runtime_force_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpi2c_suspend_noirq,
+				     lpi2c_resume_noirq)

Belongs to separate change and explain why need do this

quoted

 	SET_RUNTIME_PM_OPS(lpi2c_runtime_suspend,
 			   lpi2c_runtime_resume, NULL)
 };
--
2.25.1

-----Original Message-----
From: Aisheng Dong <aisheng.dong@nxp.com>
Sent: Friday, March 19, 2021 12:40
To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org;
s.hauer@pengutronix.de
Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org
Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

quoted

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM
Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

- Add runtime pm support to dynamicly manage the clock.
- Put the suspend to suspend_noirq.
- Call .pm_runtime_force_suspend() to force runtime pm suspended
  in .suspend_noirq().

The patch title needs to be improved as the driver already supports rpm.
And do one thing in one patch.

quoted

Signed-off-by: Fugang Duan <redacted>
Signed-off-by: Gao Pan <redacted>
Reviewed-by: Anson Huang <redacted>

Please add your sign-off.

quoted

---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 50
++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c

b/drivers/i2c/busses/i2c-imx-lpi2c.c
index bbf44ac95021..1e920e7ac7c1 100644

--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device

*pdev)
 	if (ret)
 		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;

-	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
+	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr,
+			       IRQF_NO_SUSPEND,

This belongs to a separate patch

quoted

 			       pdev->name, lpi2c_imx);
 	if (ret) {
 		dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ -

584,35

quoted

+585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);

-	ret = clk_prepare_enable(lpi2c_imx->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "clk enable failed %d\n", ret);
-		return ret;
-	}
-
 	pm_runtime_set_autosuspend_delay(&pdev->dev,

I2C_PM_TIMEOUT);

quoted

 	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_get_noresume(&pdev->dev);
-	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);

+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
+		dev_err(&pdev->dev, "failed to enable clock\n");
+		return ret;
+	}

Can't current clk control via rpm work well?
Please describe why need change.

I think the previous patch maker might want to use the return value of 
pm_runtime_get_sync to check whether the clock has been turned on correctly to 
avoid the kernel panic.
Maybe I can change to the method like this.
	pm_runtime_get_noresume(&pdev->dev);
	ret = pm_runtime_set_active(&pdev->dev);
	if (ret < 0)
		goto out;
	pm_runtime_enable(&pdev->dev);

Best Regards,
Clark Wang

-----Original Message-----
From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Friday, March 19, 2021 14:16
To: Aisheng Dong <aisheng.dong@nxp.com>; shawnguo@kernel.org;
s.hauer@pengutronix.de
Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org
Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

quoted

-----Original Message-----
From: Aisheng Dong <aisheng.dong@nxp.com>
Sent: Friday, March 19, 2021 12:40
To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org;
s.hauer@pengutronix.de
Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org
Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

quoted

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM
Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

- Add runtime pm support to dynamicly manage the clock.
- Put the suspend to suspend_noirq.
- Call .pm_runtime_force_suspend() to force runtime pm suspended
  in .suspend_noirq().

The patch title needs to be improved as the driver already supports rpm.
And do one thing in one patch.

quoted

Signed-off-by: Fugang Duan <redacted>
Signed-off-by: Gao Pan <redacted>
Reviewed-by: Anson Huang <redacted>

Please add your sign-off.

quoted

---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 50
++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c

b/drivers/i2c/busses/i2c-imx-lpi2c.c
index bbf44ac95021..1e920e7ac7c1 100644

--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device

*pdev)
 	if (ret)
 		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;

-	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
+	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr,
+			       IRQF_NO_SUSPEND,

This belongs to a separate patch

quoted

 			       pdev->name, lpi2c_imx);
 	if (ret) {
 		dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ -

584,35

quoted

+585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);

-	ret = clk_prepare_enable(lpi2c_imx->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "clk enable failed %d\n", ret);
-		return ret;
-	}
-
 	pm_runtime_set_autosuspend_delay(&pdev->dev,

I2C_PM_TIMEOUT);

quoted

 	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_get_noresume(&pdev->dev);
-	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);

+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
+		dev_err(&pdev->dev, "failed to enable clock\n");
+		return ret;
+	}

Can't current clk control via rpm work well?
Please describe why need change.

I think the previous patch maker might want to use the return value of
pm_runtime_get_sync to check whether the clock has been turned on
correctly to
avoid the kernel panic.
Maybe I can change to the method like this.
	pm_runtime_get_noresume(&pdev->dev);
	ret = pm_runtime_set_active(&pdev->dev);
	if (ret < 0)
		goto out;
	pm_runtime_enable(&pdev->dev);

Best Regards,
Clark Wang

Sorry, I missed the point before.
If we use pm_runtime_get_noresume(&pdev->dev); and 
pm_runtime_set_active(&pdev->dev); here, the clk should be enabled by using 
clk_prepare_enable() in the probe function. However, the call of 
clk_prepare_enable() is already in lpi2c_runtime_resume().
Using get_sync() here can help to reduce the repetitive code, especially ipg 
clk will be added later.
Shall we change to use pm_runtime_get_sync() here?

Regards,
Clark Wang

[...]

quoted

quoted

quoted

 	pm_runtime_set_autosuspend_delay(&pdev->dev,

I2C_PM_TIMEOUT);

quoted

 	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_get_noresume(&pdev->dev);
-	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);

+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
+		dev_err(&pdev->dev, "failed to enable clock\n");
+		return ret;
+	}

Can't current clk control via rpm work well?
Please describe why need change.

I think the previous patch maker might want to use the return value of
pm_runtime_get_sync to check whether the clock has been turned on
correctly to
avoid the kernel panic.
Maybe I can change to the method like this.
	pm_runtime_get_noresume(&pdev->dev);
	ret = pm_runtime_set_active(&pdev->dev);
	if (ret < 0)
		goto out;
	pm_runtime_enable(&pdev->dev);

Best Regards,
Clark Wang

Sorry, I missed the point before.
If we use pm_runtime_get_noresume(&pdev->dev); and
pm_runtime_set_active(&pdev->dev); here, the clk should be enabled by using
clk_prepare_enable() in the probe function. However, the call of
clk_prepare_enable() is already in lpi2c_runtime_resume().
Using get_sync() here can help to reduce the repetitive code, especially ipg
clk will be added later.

Let's not do for this negligible improvement unless there're any other good benefits.
If really care, move clk operation into an inline function instead.
Another benefit if not doing that is the driver still can work even RPM not enabled.

Regards
Aisheng

Shall we change to use pm_runtime_get_sync() here?

Regards,
Clark Wang

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Gao Pan <redacted>

use subsys_initcall for i2c driver to improve i2c driver probe priority

Signed-off-by: Gao Pan <redacted>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 8f9dd3dd2951..86b69852f7be 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -710,7 +710,17 @@ static struct platform_driver lpi2c_imx_driver = {
 	},
 };
 
-module_platform_driver(lpi2c_imx_driver);
+static int __init lpi2c_imx_init(void)
+{
+	return platform_driver_register(&lpi2c_imx_driver);
+}
+subsys_initcall(lpi2c_imx_init);
+
+static void __exit lpi2c_imx_exit(void)
+{
+	platform_driver_unregister(&lpi2c_imx_driver);
+}
+module_exit(lpi2c_imx_exit);
 
 MODULE_AUTHOR("Gao Pan <pandy.gao@nxp.com>");
 MODULE_DESCRIPTION("I2C adapter driver for LPI2C bus");

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM

use subsys_initcall for i2c driver to improve i2c driver probe priority

Will this affect DMA support which will be probed much later compared with subsys_initcall?

Signed-off-by: Gao Pan <redacted>

Add your sign-off

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Mar 17, 2021 at 02:53:54PM +0800, Clark Wang wrote:

From: Gao Pan <redacted>

use subsys_initcall for i2c driver to improve i2c driver probe priority

Signed-off-by: Gao Pan <redacted>

I usually don't take subsys_initcall patches anymore. In most cases, the
client drivers can be fixed instead. If this is not the case for you,
you need to state that explicitly in the commit message.

-----Original Message-----
From: Wolfram Sang <wsa@kernel.org>
Sent: Friday, March 19, 2021 13:39
To: Clark Wang <xiaoning.wang@nxp.com>
Cc: Aisheng Dong <aisheng.dong@nxp.com>; shawnguo@kernel.org;
s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
dl-linux-imx [off-list ref]; sumit.semwal@linaro.org;
christian.koenig@amd.com; linux-i2c@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Subject: [EXT] Re: [PATCH 06/11] i2c: imx-lpi2c: improve i2c driver probe
priority

On Wed, Mar 17, 2021 at 02:53:54PM +0800, Clark Wang wrote:

quoted

From: Gao Pan <redacted>

use subsys_initcall for i2c driver to improve i2c driver probe
priority

Signed-off-by: Gao Pan <redacted>

I usually don't take subsys_initcall patches anymore. In most cases, the

client

drivers can be fixed instead. If this is not the case for you, you need to

state

that explicitly in the commit message.

Okay. Because it is an old patch, I will check if it is necessary. If no,
I will continue to use module_platform_driver(lpi2c_imx_driver);
Thanks.

Best Regards,
Clark Wang

From: Fugang Duan <redacted>

Manage irq resource request/release in runtime pm to save irq domain's
power.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Fugang Duan <redacted>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 664fcc0dba51..e718bb6b2387 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -94,6 +94,7 @@ enum lpi2c_imx_pincfg {
 
 struct lpi2c_imx_struct {
 	struct i2c_adapter	adapter;
+	int			irq;
 	struct clk		*clk_per;
 	struct clk		*clk_ipg;
 	void __iomem		*base;

@@ -543,7 +544,7 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 {
 	struct lpi2c_imx_struct *lpi2c_imx;
 	unsigned int temp;
-	int irq, ret;
+	int ret;
 
 	lpi2c_imx = devm_kzalloc(&pdev->dev, sizeof(*lpi2c_imx), GFP_KERNEL);
 	if (!lpi2c_imx)

@@ -553,9 +554,9 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	if (IS_ERR(lpi2c_imx->base))
 		return PTR_ERR(lpi2c_imx->base);
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
+	lpi2c_imx->irq = platform_get_irq(pdev, 0);
+	if (lpi2c_imx->irq < 0)
+		return lpi2c_imx->irq;
 
 	lpi2c_imx->adapter.owner	= THIS_MODULE;
 	lpi2c_imx->adapter.algo		= &lpi2c_imx_algo;

@@ -581,14 +582,6 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	if (ret)
 		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;
 
-	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr,
-			       IRQF_NO_SUSPEND,
-			       pdev->name, lpi2c_imx);
-	if (ret) {
-		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
-		return ret;
-	}
-
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);
 

@@ -640,6 +633,7 @@ static int __maybe_unused lpi2c_runtime_suspend(struct device *dev)
 {
 	struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);
 
+	devm_free_irq(dev, lpi2c_imx->irq, lpi2c_imx);
 	clk_disable_unprepare(lpi2c_imx->clk_ipg);
 	clk_disable_unprepare(lpi2c_imx->clk_per);
 	pinctrl_pm_select_idle_state(dev);

@@ -665,6 +659,14 @@ static int __maybe_unused lpi2c_runtime_resume(struct device *dev)
 		dev_err(dev, "can't enable I2C ipg clock, ret=%d\n", ret);
 	}
 
+	ret = devm_request_irq(dev, lpi2c_imx->irq, lpi2c_imx_isr,
+			       IRQF_NO_SUSPEND,
+			       dev_name(dev), lpi2c_imx);
+	if (ret) {
+		dev_err(dev, "can't claim irq %d\n", lpi2c_imx->irq);
+		return ret;
+	}
+
 	return ret;
 }
 

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

I guess unnecessary to use devm in rpm

+			       IRQF_NO_SUSPEND,
+			       dev_name(dev), lpi2c_imx);
+	if (ret) {
+		dev_err(dev, "can't claim irq %d\n", lpi2c_imx->irq);
+		return ret;
+	}
+
 	return ret;
 }

--
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-----Original Message-----
From: Aisheng Dong <aisheng.dong@nxp.com>
Sent: Friday, March 19, 2021 12:54
To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org;
s.hauer@pengutronix.de
Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org
Subject: RE: [PATCH 04/11] i2c: imx-lpi2c: manage irq resource
request/release in runtime pm

quoted

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM

Manage irq resource request/release in runtime pm to save irq domain's
power.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Fugang Duan <redacted>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c

b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 664fcc0dba51..e718bb6b2387 100644

--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -94,6 +94,7 @@ enum lpi2c_imx_pincfg {

 struct lpi2c_imx_struct {
 	struct i2c_adapter	adapter;
+	int			irq;
 	struct clk		*clk_per;
 	struct clk		*clk_ipg;
 	void __iomem		*base;

@@ -543,7 +544,7 @@ static int lpi2c_imx_probe(struct platform_device

*pdev)  {
 	struct lpi2c_imx_struct *lpi2c_imx;
 	unsigned int temp;
-	int irq, ret;
+	int ret;

 	lpi2c_imx = devm_kzalloc(&pdev->dev, sizeof(*lpi2c_imx),

GFP_KERNEL);

quoted

 	if (!lpi2c_imx)

@@ -553,9 +554,9 @@ static int lpi2c_imx_probe(struct platform_device

*pdev)
 	if (IS_ERR(lpi2c_imx->base))
 		return PTR_ERR(lpi2c_imx->base);

-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
+	lpi2c_imx->irq = platform_get_irq(pdev, 0);
+	if (lpi2c_imx->irq < 0)
+		return lpi2c_imx->irq;

 	lpi2c_imx->adapter.owner	= THIS_MODULE;
 	lpi2c_imx->adapter.algo		= &lpi2c_imx_algo;

@@ -581,14 +582,6 @@ static int lpi2c_imx_probe(struct platform_device

*pdev)
 	if (ret)
 		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;

-	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr,
-			       IRQF_NO_SUSPEND,
-			       pdev->name, lpi2c_imx);
-	if (ret) {
-		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
-		return ret;
-	}
-
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);

@@ -640,6 +633,7 @@ static int __maybe_unused

lpi2c_runtime_suspend(struct device *dev)  {
 	struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);

+	devm_free_irq(dev, lpi2c_imx->irq, lpi2c_imx);
 	clk_disable_unprepare(lpi2c_imx->clk_ipg);
 	clk_disable_unprepare(lpi2c_imx->clk_per);
 	pinctrl_pm_select_idle_state(dev);

@@ -665,6 +659,14 @@ static int __maybe_unused

lpi2c_runtime_resume(struct device *dev)
 		dev_err(dev, "can't enable I2C ipg clock, ret=%d\n", ret);
 	}

+	ret = devm_request_irq(dev, lpi2c_imx->irq, lpi2c_imx_isr,

I guess unnecessary to use devm in rpm

devm_request_irq() will use device resource management.
Other resource like clk and struct space are all managed by devres.
Maybe we can still use devm_ to let devres manage irq here?

Thanks.

Best Regards,
Clark Wang

quoted

+			       IRQF_NO_SUSPEND,
+			       dev_name(dev), lpi2c_imx);
+	if (ret) {
+		dev_err(dev, "can't claim irq %d\n", lpi2c_imx->irq);
+		return ret;
+	}
+
 	return ret;
 }

--
2.25.1

quoted

quoted

@@ -665,6 +659,14 @@ static int __maybe_unused

lpi2c_runtime_resume(struct device *dev)
 		dev_err(dev, "can't enable I2C ipg clock, ret=%d\n", ret);
 	}

+	ret = devm_request_irq(dev, lpi2c_imx->irq, lpi2c_imx_isr,

I guess unnecessary to use devm in rpm

devm_request_irq() will use device resource management.
Other resource like clk and struct space are all managed by devres.
Maybe we can still use devm_ to let devres manage irq here?

devm_xxx is usually used to auto free resources when probe fail,
driver unbound / device unregister and etc. Not for runtime pm.
I may prefer using request_irq/free_irq directly in runtime.

BTW, current lpi2c_imx_remove seems didn't ensure the device is
In runtime suspend state after removing.
If framework can't guarantee, the driver has to do it.
Anyway, that's another issue and need a separate patch.

Regards
Aisheng

Thanks.

Best Regards,
Clark Wang

quoted

quoted

+			       IRQF_NO_SUSPEND,
+			       dev_name(dev), lpi2c_imx);
+	if (ret) {
+		dev_err(dev, "can't claim irq %d\n", lpi2c_imx->irq);
+		return ret;
+	}
+
 	return ret;
 }

--
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Switching the clock frequently will affect the data transmission
efficiency, and prolong the timeout to reduce autosuspend times for
lpi2c.

Acked-by: Fugang Duan <redacted>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 86b69852f7be..c0cb77c66090 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -75,7 +75,7 @@
 #define I2C_CLK_RATIO	2
 #define CHUNK_DATA	256
 
-#define I2C_PM_TIMEOUT		10 /* ms */
+#define I2C_PM_TIMEOUT		1000 /* ms */
 
 enum lpi2c_imx_mode {
 	STANDARD,	/* 100+Kbps */

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM

Switching the clock frequently will affect the data transmission efficiency, and
prolong the timeout to reduce autosuspend times for lpi2c.

Acked-by: Fugang Duan <redacted>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

Regards
Aisheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Add bus recovery feature for LPI2C.
Need add gpio pinctrl, scl-gpios and sda-gpios configuration in dts.

Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 83 ++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index c0cb77c66090..7216a393095d 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>

@@ -108,6 +109,11 @@ struct lpi2c_imx_struct {
 	unsigned int		txfifosize;
 	unsigned int		rxfifosize;
 	enum lpi2c_imx_mode	mode;
+
+	struct i2c_bus_recovery_info rinfo;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pinctrl_pins_default;
+	struct pinctrl_state *pinctrl_pins_gpio;
 };
 
 static void lpi2c_imx_intctrl(struct lpi2c_imx_struct *lpi2c_imx,

@@ -135,6 +141,8 @@ static int lpi2c_imx_bus_busy(struct lpi2c_imx_struct *lpi2c_imx)
 
 		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
 			dev_dbg(&lpi2c_imx->adapter.dev, "bus not work\n");
+			if (lpi2c_imx->adapter.bus_recovery_info)
+				i2c_recover_bus(&lpi2c_imx->adapter);
 			return -ETIMEDOUT;
 		}
 		schedule();

@@ -192,6 +200,8 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct *lpi2c_imx)
 
 		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
 			dev_dbg(&lpi2c_imx->adapter.dev, "stop timeout\n");
+			if (lpi2c_imx->adapter.bus_recovery_info)
+				i2c_recover_bus(&lpi2c_imx->adapter);
 			break;
 		}
 		schedule();

@@ -329,6 +339,8 @@ static int lpi2c_imx_txfifo_empty(struct lpi2c_imx_struct *lpi2c_imx)
 
 		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
 			dev_dbg(&lpi2c_imx->adapter.dev, "txfifo empty timeout\n");
+			if (lpi2c_imx->adapter.bus_recovery_info)
+				i2c_recover_bus(&lpi2c_imx->adapter);
 			return -ETIMEDOUT;
 		}
 		schedule();

@@ -528,6 +540,71 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void lpi2c_imx_prepare_recovery(struct i2c_adapter *adap)
+{
+	struct lpi2c_imx_struct *lpi2c_imx;
+
+	lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, adapter);
+
+	pinctrl_select_state(lpi2c_imx->pinctrl, lpi2c_imx->pinctrl_pins_gpio);
+}
+
+static void lpi2c_imx_unprepare_recovery(struct i2c_adapter *adap)
+{
+	struct lpi2c_imx_struct *lpi2c_imx;
+
+	lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, adapter);
+
+	pinctrl_select_state(lpi2c_imx->pinctrl, lpi2c_imx->pinctrl_pins_default);
+}
+
+/*
+ * We switch SCL and SDA to their GPIO function and do some bitbanging
+ * for bus recovery. These alternative pinmux settings can be
+ * described in the device tree by a separate pinctrl state "gpio". If
+ * this is missing this is not a big problem, the only implication is
+ * that we can't do bus recovery.
+ */
+static int lpi2c_imx_init_recovery_info(struct lpi2c_imx_struct *lpi2c_imx,
+		struct platform_device *pdev)
+{
+	struct i2c_bus_recovery_info *rinfo = &lpi2c_imx->rinfo;
+
+	lpi2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!lpi2c_imx->pinctrl || IS_ERR(lpi2c_imx->pinctrl)) {
+		dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
+		return PTR_ERR(lpi2c_imx->pinctrl);
+	}
+
+	lpi2c_imx->pinctrl_pins_default = pinctrl_lookup_state(lpi2c_imx->pinctrl,
+			PINCTRL_STATE_DEFAULT);
+	lpi2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(lpi2c_imx->pinctrl,
+			"gpio");
+	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
+	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
+
+	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
+	    PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) {
+		return -EPROBE_DEFER;
+	} else if (IS_ERR(rinfo->sda_gpiod) ||
+		   IS_ERR(rinfo->scl_gpiod) ||
+		   IS_ERR(lpi2c_imx->pinctrl_pins_default) ||
+		   IS_ERR(lpi2c_imx->pinctrl_pins_gpio)) {
+		dev_dbg(&pdev->dev, "recovery information incomplete\n");
+		return 0;
+	}
+
+	dev_info(&pdev->dev, "using scl%s for recovery\n",
+		 rinfo->sda_gpiod ? ",sda" : "");
+
+	rinfo->prepare_recovery = lpi2c_imx_prepare_recovery;
+	rinfo->unprepare_recovery = lpi2c_imx_unprepare_recovery;
+	rinfo->recover_bus = i2c_generic_scl_recovery;
+	lpi2c_imx->adapter.bus_recovery_info = rinfo;
+
+	return 0;
+}
+
 static u32 lpi2c_imx_func(struct i2c_adapter *adapter)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |

@@ -607,6 +684,12 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 
 	pm_runtime_put(&pdev->dev);
 
+	/* Init optional bus recovery function */
+	ret = lpi2c_imx_init_recovery_info(lpi2c_imx, pdev);
+	/* Give it another chance if pinctrl used is not ready yet */
+	if (ret == -EPROBE_DEFER)
+		goto rpm_disable;
+
 	ret = i2c_add_adapter(&lpi2c_imx->adapter);
 	if (ret)
 		goto rpm_disable;

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM

Add bus recovery feature for LPI2C.
Need add gpio pinctrl, scl-gpios and sda-gpios configuration in dts.

Pls also update dt-binding first

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The clkhi and clklo ratio was not very precise before that can make the
time of START/STOP/HIGH LEVEL out of specification.

Therefore, the calculation of these times has been modified in this patch.
At the same time, the mode rate definition of i2c is corrected.

Reviewed-by: Fugang Duan <redacted>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 7216a393095d..5dbe85126f24 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -73,17 +73,17 @@
 #define MCFGR1_IGNACK	BIT(9)
 #define MRDR_RXEMPTY	BIT(14)
 
-#define I2C_CLK_RATIO	2
+#define I2C_CLK_RATIO	24 / 59
 #define CHUNK_DATA	256
 
 #define I2C_PM_TIMEOUT		1000 /* ms */
 
 enum lpi2c_imx_mode {
-	STANDARD,	/* 100+Kbps */
-	FAST,		/* 400+Kbps */
-	FAST_PLUS,	/* 1.0+Mbps */
-	HS,		/* 3.4+Mbps */
-	ULTRA_FAST,	/* 5.0+Mbps */
+	STANDARD,	/* <=100Kbps */
+	FAST,		/* <=400Kbps */
+	FAST_PLUS,	/* <=1.0Mbps */
+	HS,		/* <=3.4Mbps */
+	ULTRA_FAST,	/* <=5.0Mbps */
 };
 
 enum lpi2c_imx_pincfg {

@@ -156,13 +156,13 @@ static void lpi2c_imx_set_mode(struct lpi2c_imx_struct *lpi2c_imx)
 	unsigned int bitrate = lpi2c_imx->bitrate;
 	enum lpi2c_imx_mode mode;
 
-	if (bitrate < I2C_MAX_FAST_MODE_FREQ)
+	if (bitrate <= I2C_MAX_STANDARD_MODE_FREQ)
 		mode = STANDARD;
-	else if (bitrate < I2C_MAX_FAST_MODE_PLUS_FREQ)
+	else if (bitrate <= I2C_MAX_FAST_MODE_FREQ)
 		mode = FAST;
-	else if (bitrate < I2C_MAX_HIGH_SPEED_MODE_FREQ)
+	else if (bitrate <= I2C_MAX_FAST_MODE_PLUS_FREQ)
 		mode = FAST_PLUS;
-	else if (bitrate < I2C_MAX_ULTRA_FAST_MODE_FREQ)
+	else if (bitrate <= I2C_MAX_HIGH_SPEED_MODE_FREQ)
 		mode = HS;
 	else
 		mode = ULTRA_FAST;

@@ -209,7 +209,8 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct *lpi2c_imx)
 	} while (1);
 }
 
-/* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */
+/* CLKLO = (1 - I2C_CLK_RATIO) * clk_cycle, SETHOLD = CLKHI, DATAVD = CLKHI/2
+   CLKHI = I2C_CLK_RATIO * clk_cycle */
 static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
 {
 	u8 prescale, filt, sethold, clkhi, clklo, datavd;

@@ -232,8 +233,8 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
 
 	for (prescale = 0; prescale <= 7; prescale++) {
 		clk_cycle = clk_rate / ((1 << prescale) * lpi2c_imx->bitrate)
-			    - 3 - (filt >> 1);
-		clkhi = (clk_cycle + I2C_CLK_RATIO) / (I2C_CLK_RATIO + 1);
+			    - (2 + filt) / (1 << prescale);
+		clkhi = clk_cycle * I2C_CLK_RATIO;
 		clklo = clk_cycle - clkhi;
 		if (clklo < 64)
 			break;

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Where is this ratio coming from?
Can you describe why use it in commit message?

Regards
Aisheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-----Original Message-----
From: Aisheng Dong <aisheng.dong@nxp.com>
Sent: Friday, March 19, 2021 13:15
To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org;
s.hauer@pengutronix.de
Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org
Subject: RE: [PATCH 09/11] i2c: imx-lpi2c: fix i2c timing issue

quoted

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM

The clkhi and clklo ratio was not very precise before that can make
the time of START/STOP/HIGH LEVEL out of specification.

Therefore, the calculation of these times has been modified in this patch.
At the same time, the mode rate definition of i2c is corrected.

Reviewed-by: Fugang Duan <redacted>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c

b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 7216a393095d..5dbe85126f24 100644

--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -73,17 +73,17 @@
 #define MCFGR1_IGNACK	BIT(9)
 #define MRDR_RXEMPTY	BIT(14)

-#define I2C_CLK_RATIO	2
+#define I2C_CLK_RATIO	24 / 59

Where is this ratio coming from?
Can you describe why use it in commit message?

This ratio is a value obtained after passing the test.
I2C's Tlow should longer than the spec.
For example, in FAST mode, Tlow should be longer than 1.3us.
The previous calculation violated the spec.
So I re-write the calculation of clk_cycle by referring the RM. Then test the 
ratio to let Tlow match the spec by catching the waveform.

Best Regards,
Clark Wang

Regards
Aisheng

quoted

 #define CHUNK_DATA	256

 #define I2C_PM_TIMEOUT		1000 /* ms */

 enum lpi2c_imx_mode {
-	STANDARD,	/* 100+Kbps */
-	FAST,		/* 400+Kbps */
-	FAST_PLUS,	/* 1.0+Mbps */
-	HS,		/* 3.4+Mbps */
-	ULTRA_FAST,	/* 5.0+Mbps */
+	STANDARD,	/* <=100Kbps */
+	FAST,		/* <=400Kbps */
+	FAST_PLUS,	/* <=1.0Mbps */
+	HS,		/* <=3.4Mbps */
+	ULTRA_FAST,	/* <=5.0Mbps */
 };

 enum lpi2c_imx_pincfg {

@@ -156,13 +156,13 @@ static void lpi2c_imx_set_mode(struct

lpi2c_imx_struct *lpi2c_imx)
 	unsigned int bitrate = lpi2c_imx->bitrate;
 	enum lpi2c_imx_mode mode;

-	if (bitrate < I2C_MAX_FAST_MODE_FREQ)
+	if (bitrate <= I2C_MAX_STANDARD_MODE_FREQ)
 		mode = STANDARD;
-	else if (bitrate < I2C_MAX_FAST_MODE_PLUS_FREQ)
+	else if (bitrate <= I2C_MAX_FAST_MODE_FREQ)
 		mode = FAST;
-	else if (bitrate < I2C_MAX_HIGH_SPEED_MODE_FREQ)
+	else if (bitrate <= I2C_MAX_FAST_MODE_PLUS_FREQ)
 		mode = FAST_PLUS;
-	else if (bitrate < I2C_MAX_ULTRA_FAST_MODE_FREQ)
+	else if (bitrate <= I2C_MAX_HIGH_SPEED_MODE_FREQ)
 		mode = HS;
 	else
 		mode = ULTRA_FAST;

@@ -209,7 +209,8 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct

*lpi2c_imx)
 	} while (1);
 }

-/* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2
*/
+/* CLKLO = (1 - I2C_CLK_RATIO) * clk_cycle, SETHOLD = CLKHI, DATAVD =
CLKHI/2
+   CLKHI = I2C_CLK_RATIO * clk_cycle */
 static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)  {
 	u8 prescale, filt, sethold, clkhi, clklo, datavd; @@ -232,8 +233,8

@@ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)

 	for (prescale = 0; prescale <= 7; prescale++) {
 		clk_cycle = clk_rate / ((1 << prescale) * lpi2c_imx->bitrate)
-			    - 3 - (filt >> 1);
-		clkhi = (clk_cycle + I2C_CLK_RATIO) / (I2C_CLK_RATIO + 1);
+			    - (2 + filt) / (1 << prescale);
+		clkhi = clk_cycle * I2C_CLK_RATIO;
 		clklo = clk_cycle - clkhi;
 		if (clklo < 64)
 			break;
--

2.25.1

Claim clkhi and clklo as integer type to avoid possible calculation
errors caused by data overflow.

Reviewed-by: Fugang Duan <redacted>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 5dbe85126f24..1e26672d47bf 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -213,8 +213,8 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct *lpi2c_imx)
    CLKHI = I2C_CLK_RATIO * clk_cycle */
 static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
 {
-	u8 prescale, filt, sethold, clkhi, clklo, datavd;
-	unsigned int clk_rate, clk_cycle;
+	u8 prescale, filt, sethold, datavd;
+	unsigned int clk_rate, clk_cycle, clkhi, clklo;
 	enum lpi2c_imx_pincfg pincfg;
 	unsigned int temp;
 

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Clark Wang <xiaoning.wang@nxp.com>
Sent: Wednesday, March 17, 2021 2:54 PM

Claim clkhi and clklo as integer type to avoid possible calculation errors caused
by data overflow.

Reviewed-by: Fugang Duan <redacted>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

Regards
Aisheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Add eDMA receive and send mode support.
Support to read and write data larger than 256 bytes in one frame.

Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Reviewed-by: Li Jun <redacted>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 291 ++++++++++++++++++++++++++++-
 1 file changed, 289 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 1e26672d47bf..6d920bf0dbd4 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

@@ -8,6 +8,8 @@
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>

@@ -31,6 +33,7 @@
 #define LPI2C_MCR	0x10	/* i2c contrl register */
 #define LPI2C_MSR	0x14	/* i2c status register */
 #define LPI2C_MIER	0x18	/* i2c interrupt enable */
+#define LPI2C_MDER	0x1C	/* i2c DMA enable */
 #define LPI2C_MCFGR0	0x20	/* i2c master configuration */
 #define LPI2C_MCFGR1	0x24	/* i2c master configuration */
 #define LPI2C_MCFGR2	0x28	/* i2c master configuration */

@@ -72,11 +75,15 @@
 #define MCFGR1_AUTOSTOP	BIT(8)
 #define MCFGR1_IGNACK	BIT(9)
 #define MRDR_RXEMPTY	BIT(14)
+#define MDER_TDDE	BIT(0)
+#define MDER_RDDE	BIT(1)
 
 #define I2C_CLK_RATIO	24 / 59
 #define CHUNK_DATA	256
 
 #define I2C_PM_TIMEOUT		1000 /* ms */
+#define I2C_DMA_THRESHOLD	16 /* bytes */
+#define I2C_USE_PIO		(-150)
 
 enum lpi2c_imx_mode {
 	STANDARD,	/* <=100Kbps */

@@ -95,6 +102,7 @@ enum lpi2c_imx_pincfg {
 
 struct lpi2c_imx_struct {
 	struct i2c_adapter	adapter;
+	resource_size_t		phy_addr;
 	int			irq;
 	struct clk		*clk_per;
 	struct clk		*clk_ipg;

@@ -114,6 +122,17 @@ struct lpi2c_imx_struct {
 	struct pinctrl *pinctrl;
 	struct pinctrl_state *pinctrl_pins_default;
 	struct pinctrl_state *pinctrl_pins_gpio;
+
+	bool			can_use_dma;
+	bool			using_dma;
+	bool			xferred;
+	struct i2c_msg		*msg;
+	dma_addr_t		dma_addr;
+	struct dma_chan		*dma_tx;
+	struct dma_chan		*dma_rx;
+	enum dma_data_direction dma_direction;
+	u8			*dma_buf;
+	unsigned int		dma_len;
 };
 
 static void lpi2c_imx_intctrl(struct lpi2c_imx_struct *lpi2c_imx,

@@ -289,6 +308,9 @@ static int lpi2c_imx_master_enable(struct lpi2c_imx_struct *lpi2c_imx)
 	if (ret)
 		goto rpm_put;
 
+	if (lpi2c_imx->can_use_dma)
+		writel(MDER_TDDE | MDER_RDDE, lpi2c_imx->base + LPI2C_MDER);
+
 	temp = readl(lpi2c_imx->base + LPI2C_MCR);
 	temp |= MCR_MEN;
 	writel(temp, lpi2c_imx->base + LPI2C_MCR);

@@ -462,6 +484,154 @@ static void lpi2c_imx_read(struct lpi2c_imx_struct *lpi2c_imx,
 	lpi2c_imx_intctrl(lpi2c_imx, MIER_RDIE | MIER_NDIE);
 }
 
+static void lpi2c_dma_unmap(struct lpi2c_imx_struct *lpi2c_imx)
+{
+	struct dma_chan *chan = lpi2c_imx->dma_direction == DMA_FROM_DEVICE
+				? lpi2c_imx->dma_rx : lpi2c_imx->dma_tx;
+
+	dma_unmap_single(chan->device->dev, lpi2c_imx->dma_addr,
+			 lpi2c_imx->dma_len, lpi2c_imx->dma_direction);
+
+	lpi2c_imx->dma_direction = DMA_NONE;
+}
+
+static void lpi2c_cleanup_dma(struct lpi2c_imx_struct *lpi2c_imx)
+{
+	if (lpi2c_imx->dma_direction == DMA_NONE)
+		return;
+	else if (lpi2c_imx->dma_direction == DMA_FROM_DEVICE)
+		dmaengine_terminate_all(lpi2c_imx->dma_rx);
+	else if (lpi2c_imx->dma_direction == DMA_TO_DEVICE)
+		dmaengine_terminate_all(lpi2c_imx->dma_tx);
+
+	lpi2c_dma_unmap(lpi2c_imx);
+}
+
+static void lpi2c_dma_callback(void *data)
+{
+	struct lpi2c_imx_struct *lpi2c_imx = (struct lpi2c_imx_struct *)data;
+
+	lpi2c_dma_unmap(lpi2c_imx);
+	writel(GEN_STOP << 8, lpi2c_imx->base + LPI2C_MTDR);
+	lpi2c_imx->xferred = true;
+
+	complete(&lpi2c_imx->complete);
+}
+
+static int lpi2c_dma_submit(struct lpi2c_imx_struct *lpi2c_imx,
+			   struct i2c_msg *msg)
+{
+	bool read = msg->flags & I2C_M_RD;
+	enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+	struct dma_chan *chan = read ? lpi2c_imx->dma_rx : lpi2c_imx->dma_tx;
+	struct dma_async_tx_descriptor *txdesc;
+	dma_cookie_t cookie;
+
+	lpi2c_imx->dma_len = read ? msg->len - 1 : msg->len;
+	lpi2c_imx->msg = msg;
+	lpi2c_imx->dma_direction = dir;
+
+	if (IS_ERR(chan))
+		return PTR_ERR(chan);
+
+	lpi2c_imx->dma_addr = dma_map_single(chan->device->dev,
+					     lpi2c_imx->dma_buf,
+					     lpi2c_imx->dma_len, dir);
+	if (dma_mapping_error(chan->device->dev, lpi2c_imx->dma_addr)) {
+		dev_err(&lpi2c_imx->adapter.dev, "dma map failed, use pio\n");
+		return -EINVAL;
+	}
+
+	txdesc = dmaengine_prep_slave_single(chan, lpi2c_imx->dma_addr,
+					lpi2c_imx->dma_len, read ?
+					DMA_DEV_TO_MEM : DMA_MEM_TO_DEV,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!txdesc) {
+		dev_err(&lpi2c_imx->adapter.dev, "dma prep slave sg failed, use pio\n");
+		lpi2c_cleanup_dma(lpi2c_imx);
+		return -EINVAL;
+	}
+
+	reinit_completion(&lpi2c_imx->complete);
+	txdesc->callback = lpi2c_dma_callback;
+	txdesc->callback_param = (void *)lpi2c_imx;
+
+	cookie = dmaengine_submit(txdesc);
+	if (dma_submit_error(cookie)) {
+		dev_err(&lpi2c_imx->adapter.dev, "submitting dma failed, use pio\n");
+		lpi2c_cleanup_dma(lpi2c_imx);
+		return -EINVAL;
+	}
+
+	lpi2c_imx_intctrl(lpi2c_imx, MIER_NDIE);
+
+	dma_async_issue_pending(chan);
+
+	return 0;
+}
+
+static bool is_use_dma(struct lpi2c_imx_struct *lpi2c_imx, struct i2c_msg *msg)
+{
+	if (!lpi2c_imx->can_use_dma)
+		return false;
+
+	if (msg->len < I2C_DMA_THRESHOLD)
+		return false;
+
+	return true;
+}
+
+static int lpi2c_imx_push_rx_cmd(struct lpi2c_imx_struct *lpi2c_imx,
+				 struct i2c_msg *msg)
+{
+	unsigned int temp, rx_remain;
+	unsigned long orig_jiffies = jiffies;
+
+	if ((msg->flags & I2C_M_RD)) {
+		rx_remain = msg->len;
+		do {
+			temp = rx_remain > CHUNK_DATA ?
+				CHUNK_DATA - 1 : rx_remain - 1;
+			temp |= (RECV_DATA << 8);
+			while ((readl(lpi2c_imx->base + LPI2C_MFSR) & 0xff) > 2) {
+				if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(1000))) {
+					dev_dbg(&lpi2c_imx->adapter.dev, "txfifo empty timeout\n");
+					if (lpi2c_imx->adapter.bus_recovery_info)
+						i2c_recover_bus(&lpi2c_imx->adapter);
+					return -ETIMEDOUT;
+				}
+				schedule();
+			}
+			writel(temp, lpi2c_imx->base + LPI2C_MTDR);
+			rx_remain = rx_remain - (temp & 0xff) - 1;
+		} while (rx_remain > 0);
+	}
+
+	return 0;
+}
+
+static int lpi2c_dma_xfer(struct lpi2c_imx_struct *lpi2c_imx,
+			   struct i2c_msg *msg)
+{
+	int result;
+
+	result = lpi2c_dma_submit(lpi2c_imx, msg);
+	if (!result) {
+		result = lpi2c_imx_push_rx_cmd(lpi2c_imx, msg);
+		if (result)
+			return result;
+		result = lpi2c_imx_msg_complete(lpi2c_imx);
+		return result;
+	}
+
+	/* DMA xfer failed, try to use PIO, clean up dma things */
+	i2c_put_dma_safe_msg_buf(lpi2c_imx->dma_buf, lpi2c_imx->msg,
+				 lpi2c_imx->xferred);
+	lpi2c_cleanup_dma(lpi2c_imx);
+
+	return I2C_USE_PIO;
+}
+
 static int lpi2c_imx_xfer(struct i2c_adapter *adapter,
 			  struct i2c_msg *msgs, int num)
 {

@@ -474,6 +644,9 @@ static int lpi2c_imx_xfer(struct i2c_adapter *adapter,
 		return result;
 
 	for (i = 0; i < num; i++) {
+		lpi2c_imx->xferred = false;
+		lpi2c_imx->using_dma = false;
+
 		result = lpi2c_imx_start(lpi2c_imx, &msgs[i]);
 		if (result)
 			goto disable;

@@ -482,9 +655,24 @@ static int lpi2c_imx_xfer(struct i2c_adapter *adapter,
 		if (num == 1 && msgs[0].len == 0)
 			goto stop;
 
+		if (is_use_dma(lpi2c_imx, &msgs[i])) {
+			lpi2c_imx->using_dma = true;
+
+			writel(0x1, lpi2c_imx->base + LPI2C_MFCR);
+
+			lpi2c_imx->dma_buf = i2c_get_dma_safe_msg_buf(&msgs[i],
+							    I2C_DMA_THRESHOLD);
+			if (lpi2c_imx->dma_buf) {
+				result = lpi2c_dma_xfer(lpi2c_imx, &msgs[i]);
+				if (result != I2C_USE_PIO)
+					goto stop;
+			}
+		}
+
+		lpi2c_imx->using_dma = false;
 		lpi2c_imx->delivered = 0;
 		lpi2c_imx->msglen = msgs[i].len;
-		init_completion(&lpi2c_imx->complete);
+		reinit_completion(&lpi2c_imx->complete);
 
 		if (msgs[i].flags & I2C_M_RD)
 			lpi2c_imx_read(lpi2c_imx, &msgs[i]);