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

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

[Clark Wang]

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

[PATCH 01/11] i2c: imx-lpi2c: directly retrun ISR when detect a NACK

[Clark Wang]

From: Gao Pan <pandy.gao@nxp.com>

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 <pandy.gao@nxp.com>
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

RE: [PATCH 01/11] i2c: imx-lpi2c: directly retrun ISR when detect a NACK

[Aisheng Dong]

> 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 <pandy.gao@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>

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

Regards
Aisheng

> ---
>  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

[PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

[Clark Wang]

From: Fugang Duan <fugang.duan@nxp.com>

- 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 <fugang.duan@nxp.com>
Signed-off-by: Gao Pan <pandy.gao@nxp.com>
Reviewed-by: Anson Huang <Anson.Huang@nxp.com>
---
 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

RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

[Aisheng Dong]

> 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 <fugang.duan@nxp.com>
> Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> Reviewed-by: Anson Huang <Anson.Huang@nxp.com>

Please add your sign-off.

> ---
>  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

>  			       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.

> +
>  	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

> 
>  	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

>  		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

RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

[Clark Wang]

[-- Attachment #1.1: Type: text/plain, Size: 5162 bytes --]


> -----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
> 
> > 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
> 
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> > Reviewed-by: Anson Huang <Anson.Huang@nxp.com>
> 
> Please add your sign-off.
> 
> > ---
> >  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
> 
> >  			       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.
> 
> > +
> >  	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
> 
> >
> >  	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
> 
> >  		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


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

[Clark Wang]

[-- Attachment #1.1: Type: text/plain, Size: 3252 bytes --]


> -----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
>
> > 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 <fugang.duan@nxp.com>
> > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> > Reviewed-by: Anson Huang <Anson.Huang@nxp.com>
>
> Please add your sign-off.
>
> > ---
> >  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
>
> >  			       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.

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


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

[Clark Wang]

[-- Attachment #1.1: Type: text/plain, Size: 4409 bytes --]


> -----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
>
>
> > -----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
> >
> > > 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 <fugang.duan@nxp.com>
> > > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> > > Reviewed-by: Anson Huang <Anson.Huang@nxp.com>
> >
> > Please add your sign-off.
> >
> > > ---
> > >  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
> >
> > >  			       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.
>
> 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



[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

[Aisheng Dong]

[...]

> > > >  	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.
> >
> > 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

[PATCH 03/11] i2c: imx-lpi2c: add ipg clk for lpi2c driver

[Clark Wang]

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 <pandy.gao@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Acked-by: Fugang Duan <fugang.duan@nxp.com>
---
 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

RE: [PATCH 03/11] i2c: imx-lpi2c: add ipg clk for lpi2c driver

[Aisheng Dong]

> 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 <pandy.gao@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> Acked-by: Fugang Duan <fugang.duan@nxp.com>

You can drop the Ack tag if the patch was changed 

> ---
>  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?

Regards
Aisheng

> 
>  	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

RE: [PATCH 03/11] i2c: imx-lpi2c: add ipg clk for lpi2c driver

[Clark Wang]

[-- Attachment #1.1: Type: text/plain, Size: 3762 bytes --]


> -----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
>
> > 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.

>
> > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > Acked-by: Fugang Duan <fugang.duan@nxp.com>
>
> You can drop the Ack tag if the patch was changed
>
> > ---
> >  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
>
> >
> >  	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


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

RE: [PATCH 03/11] i2c: imx-lpi2c: add ipg clk for lpi2c driver

[Aisheng Dong]

> > > +
> > > +	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
> 
> >
> > Regards
> > Aisheng
> >
> > >
> > >  	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

[PATCH 04/11] i2c: imx-lpi2c: manage irq resource request/release in runtime pm

[Clark Wang]

From: Fugang Duan <fugang.duan@nxp.com>

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 <fugang.duan@nxp.com>
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

RE: [PATCH 04/11] i2c: imx-lpi2c: manage irq resource request/release in runtime pm

[Aisheng Dong]

> 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 <fugang.duan@nxp.com>
> 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,

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

RE: [PATCH 04/11] i2c: imx-lpi2c: manage irq resource request/release in runtime pm

[Clark Wang]

[-- Attachment #1.1: Type: text/plain, Size: 3795 bytes --]


> -----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
> 
> > 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 <fugang.duan@nxp.com>
> > 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,
> 
> 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


> 
> > +			       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


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

RE: [PATCH 04/11] i2c: imx-lpi2c: manage irq resource request/release in runtime pm

[Aisheng Dong]

> > > @@ -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
> 
> 
> >
> > > +			       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

[PATCH 05/11] i2c: imx-lpi2c: add debug message when i2c peripheral clk doesn't work

[Clark Wang]

From: Gao Pan <pandy.gao@nxp.com>

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

Signed-off-by: Gao Pan <pandy.gao@nxp.com>
Reviewed-by: Andy Duan <fugang.duan@nxp.com>
---
 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

RE: [PATCH 05/11] i2c: imx-lpi2c: add debug message when i2c peripheral clk doesn't work

[Aisheng Dong]

> 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 <pandy.gao@nxp.com>
> Reviewed-by: Andy Duan <fugang.duan@nxp.com>

Drop old review when patch is changed

> ---
>  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->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

RE: [PATCH 05/11] i2c: imx-lpi2c: add debug message when i2c peripheral clk doesn't work

[Clark Wang]

[-- Attachment #1.1: Type: text/plain, Size: 2003 bytes --]


> -----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
> 
> > 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 <pandy.gao@nxp.com>
> > Reviewed-by: Andy Duan <fugang.duan@nxp.com>
> 
> Drop old review when patch is changed
> 
> > ---
> >  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-
> >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.

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


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH 06/11] i2c: imx-lpi2c: improve i2c driver probe priority

[Clark Wang]

From: Gao Pan <pandy.gao@nxp.com>

use subsys_initcall for i2c driver to improve i2c driver probe priority

Signed-off-by: Gao Pan <pandy.gao@nxp.com>
---
 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

RE: [PATCH 06/11] i2c: imx-lpi2c: improve i2c driver probe priority

[Aisheng Dong]

> 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 <pandy.gao@nxp.com>

Add your sign-off

> ---
>  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

Re: [PATCH 06/11] i2c: imx-lpi2c: improve i2c driver probe priority

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 434 bytes --]

On Wed, Mar 17, 2021 at 02:53:54PM +0800, Clark Wang wrote:
> From: Gao Pan <pandy.gao@nxp.com>
> 
> use subsys_initcall for i2c driver to improve i2c driver probe priority
> 
> Signed-off-by: Gao Pan <pandy.gao@nxp.com>

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.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

RE: [EXT] Re: [PATCH 06/11] i2c: imx-lpi2c: improve i2c driver probe priority

[Clark Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1173 bytes --]


> -----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 <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: [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:
> > From: Gao Pan <pandy.gao@nxp.com>
> >
> > use subsys_initcall for i2c driver to improve i2c driver probe
> > priority
> >
> > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> 
> 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

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH 07/11] i2c: imx-lpi2c: increase PM timeout to avoid operate clk frequently

[Clark Wang]

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

Acked-by: Fugang Duan <fugang.duan@nxp.com>
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

RE: [PATCH 07/11] i2c: imx-lpi2c: increase PM timeout to avoid operate clk frequently

[Aisheng Dong]

> 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 <fugang.duan@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>

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

Regards
Aisheng

> ---
>  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

[PATCH 08/11] i2c: imx-lpi2c: add bus recovery feature

[Clark Wang]

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

RE: [PATCH 08/11] i2c: imx-lpi2c: add bus recovery feature

[Aisheng Dong]

> 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

> 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

[PATCH 09/11] i2c: imx-lpi2c: fix i2c timing issue

[Clark Wang]

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 <fugang.duan@nxp.com>
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

RE: [PATCH 09/11] i2c: imx-lpi2c: fix i2c timing issue

[Aisheng Dong]

> 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 <fugang.duan@nxp.com>
> 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?

Regards
Aisheng

>  #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

RE: [PATCH 09/11] i2c: imx-lpi2c: fix i2c timing issue

[Clark Wang]

[-- Attachment #1.1: Type: text/plain, Size: 4117 bytes --]


> -----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
>
> > 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 <fugang.duan@nxp.com>
> > 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
>
> >  #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


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

[PATCH 10/11] i2c: imx-lpi2c: fix type char overflow issue when calculating the clock cycle

[Clark Wang]

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

Reviewed-by: Fugang Duan <fugang.duan@nxp.com>
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

RE: [PATCH 10/11] i2c: imx-lpi2c: fix type char overflow issue when calculating the clock cycle

[Aisheng Dong]

> 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 <fugang.duan@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>

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

Regards
Aisheng

> ---
>  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

[PATCH 11/11] i2c: imx-lpi2c: add edma mode support

[Clark Wang]

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 <jun.li@nxp.com>
---
 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]);
@@ -503,7 +691,16 @@ static int lpi2c_imx_xfer(struct i2c_adapter *adapter,
 	}
 
 stop:
-	lpi2c_imx_stop(lpi2c_imx);
+	if (!lpi2c_imx->using_dma)
+		lpi2c_imx_stop(lpi2c_imx);
+	else {
+		i2c_put_dma_safe_msg_buf(lpi2c_imx->dma_buf, lpi2c_imx->msg,
+					 lpi2c_imx->xferred);
+		if (result) {
+			lpi2c_cleanup_dma(lpi2c_imx);
+			writel(GEN_STOP << 8, lpi2c_imx->base + LPI2C_MTDR);
+		}
+	}
 
 	temp = readl(lpi2c_imx->base + LPI2C_MSR);
 	if ((temp & MSR_NDF) && !result)
@@ -528,6 +725,10 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
 	temp = readl(lpi2c_imx->base + LPI2C_MSR);
 
 	if (temp & MSR_NDF) {
+		if (lpi2c_imx->using_dma) {
+			lpi2c_cleanup_dma(lpi2c_imx);
+			writel(GEN_STOP << 8, lpi2c_imx->base + LPI2C_MTDR);
+		}
 		complete(&lpi2c_imx->complete);
 		goto ret;
 	}
@@ -623,20 +824,94 @@ static const struct of_device_id lpi2c_imx_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, lpi2c_imx_of_match);
 
+static void lpi2c_dma_exit(struct lpi2c_imx_struct *lpi2c_imx)
+{
+	if (lpi2c_imx->dma_rx) {
+		dma_release_channel(lpi2c_imx->dma_rx);
+		lpi2c_imx->dma_rx = NULL;
+	}
+
+	if (lpi2c_imx->dma_tx) {
+		dma_release_channel(lpi2c_imx->dma_tx);
+		lpi2c_imx->dma_tx = NULL;
+	}
+}
+
+static int lpi2c_dma_init(struct device *dev,
+			  struct lpi2c_imx_struct *lpi2c_imx)
+{
+	int ret;
+	struct dma_slave_config dma_sconfig;
+
+	/* Prepare for TX DMA: */
+	lpi2c_imx->dma_tx = dma_request_chan(dev, "tx");
+	if (IS_ERR(lpi2c_imx->dma_tx)) {
+		ret = PTR_ERR(lpi2c_imx->dma_tx);
+		dev_err(dev, "can't get the TX DMA channel, error %d!\n", ret);
+		lpi2c_imx->dma_tx = NULL;
+		goto err;
+	}
+
+	dma_sconfig.dst_addr = lpi2c_imx->phy_addr + LPI2C_MTDR;
+	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_maxburst = 1;
+	dma_sconfig.direction = DMA_MEM_TO_DEV;
+	ret = dmaengine_slave_config(lpi2c_imx->dma_tx, &dma_sconfig);
+	if (ret < 0) {
+		dev_err(dev, "can't configure tx channel (%d)\n", ret);
+		goto fail_tx;
+	}
+
+	/* Prepare for RX DMA: */
+	lpi2c_imx->dma_rx = dma_request_chan(dev, "rx");
+	if (IS_ERR(lpi2c_imx->dma_rx)) {
+		ret = PTR_ERR(lpi2c_imx->dma_rx);
+		dev_err(dev, "can't get the RX DMA channel, error %d\n", ret);
+		lpi2c_imx->dma_rx = NULL;
+		goto fail_tx;
+	}
+
+	dma_sconfig.src_addr = lpi2c_imx->phy_addr + LPI2C_MRDR;
+	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.src_maxburst = 1;
+	dma_sconfig.direction = DMA_DEV_TO_MEM;
+	ret = dmaengine_slave_config(lpi2c_imx->dma_rx, &dma_sconfig);
+	if (ret < 0) {
+		dev_err(dev, "can't configure rx channel (%d)\n", ret);
+		goto fail_rx;
+	}
+
+	lpi2c_imx->can_use_dma = true;
+	lpi2c_imx->using_dma = false;
+
+	return 0;
+fail_rx:
+	dma_release_channel(lpi2c_imx->dma_rx);
+fail_tx:
+	dma_release_channel(lpi2c_imx->dma_tx);
+err:
+	lpi2c_dma_exit(lpi2c_imx);
+	lpi2c_imx->can_use_dma = false;
+	return ret;
+}
+
 static int lpi2c_imx_probe(struct platform_device *pdev)
 {
 	struct lpi2c_imx_struct *lpi2c_imx;
 	unsigned int temp;
 	int ret;
+	struct resource *res;
 
 	lpi2c_imx = devm_kzalloc(&pdev->dev, sizeof(*lpi2c_imx), GFP_KERNEL);
 	if (!lpi2c_imx)
 		return -ENOMEM;
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	lpi2c_imx->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(lpi2c_imx->base))
 		return PTR_ERR(lpi2c_imx->base);
 
+	lpi2c_imx->phy_addr = (dma_addr_t)res->start;
 	lpi2c_imx->irq = platform_get_irq(pdev, 0);
 	if (lpi2c_imx->irq < 0)
 		return lpi2c_imx->irq;
@@ -691,6 +966,18 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	if (ret == -EPROBE_DEFER)
 		goto rpm_disable;
 
+	/* Init DMA */
+	lpi2c_imx->dma_direction = DMA_NONE;
+	lpi2c_imx->dma_rx = lpi2c_imx->dma_tx = NULL;
+	ret = lpi2c_dma_init(&pdev->dev, lpi2c_imx);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "dma setup error %d, use pio\n", ret);
+		if (ret == -EPROBE_DEFER)
+			goto rpm_disable;
+	}
+
+	init_completion(&lpi2c_imx->complete);
+
 	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

RE: [PATCH 11/11] i2c: imx-lpi2c: add edma mode support

[Aisheng Dong]

> From: Clark Wang <xiaoning.wang@nxp.com>
> Sent: Wednesday, March 17, 2021 2:54 PM
> 
> 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 <jun.li@nxp.com>
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 291 ++++++++++++++++++++++++++++-

Pease update dt-binding first

>  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)

Can you clarify a bit why need using this strange value?

> 
>  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]); @@ -503,7 +691,16 @@
> static int lpi2c_imx_xfer(struct i2c_adapter *adapter,
>  	}
> 
>  stop:
> -	lpi2c_imx_stop(lpi2c_imx);
> +	if (!lpi2c_imx->using_dma)
> +		lpi2c_imx_stop(lpi2c_imx);
> +	else {
> +		i2c_put_dma_safe_msg_buf(lpi2c_imx->dma_buf, lpi2c_imx->msg,
> +					 lpi2c_imx->xferred);
> +		if (result) {
> +			lpi2c_cleanup_dma(lpi2c_imx);
> +			writel(GEN_STOP << 8, lpi2c_imx->base + LPI2C_MTDR);
> +		}
> +	}
> 
>  	temp = readl(lpi2c_imx->base + LPI2C_MSR);
>  	if ((temp & MSR_NDF) && !result)
> @@ -528,6 +725,10 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
>  	temp = readl(lpi2c_imx->base + LPI2C_MSR);
> 
>  	if (temp & MSR_NDF) {
> +		if (lpi2c_imx->using_dma) {
> +			lpi2c_cleanup_dma(lpi2c_imx);
> +			writel(GEN_STOP << 8, lpi2c_imx->base + LPI2C_MTDR);
> +		}
>  		complete(&lpi2c_imx->complete);
>  		goto ret;
>  	}
> @@ -623,20 +824,94 @@ static const struct of_device_id
> lpi2c_imx_of_match[] = {  };  MODULE_DEVICE_TABLE(of,
> lpi2c_imx_of_match);
> 
> +static void lpi2c_dma_exit(struct lpi2c_imx_struct *lpi2c_imx) {
> +	if (lpi2c_imx->dma_rx) {
> +		dma_release_channel(lpi2c_imx->dma_rx);
> +		lpi2c_imx->dma_rx = NULL;
> +	}
> +
> +	if (lpi2c_imx->dma_tx) {
> +		dma_release_channel(lpi2c_imx->dma_tx);
> +		lpi2c_imx->dma_tx = NULL;
> +	}
> +}
> +
> +static int lpi2c_dma_init(struct device *dev,
> +			  struct lpi2c_imx_struct *lpi2c_imx) {
> +	int ret;
> +	struct dma_slave_config dma_sconfig;
> +
> +	/* Prepare for TX DMA: */
> +	lpi2c_imx->dma_tx = dma_request_chan(dev, "tx");
> +	if (IS_ERR(lpi2c_imx->dma_tx)) {
> +		ret = PTR_ERR(lpi2c_imx->dma_tx);
> +		dev_err(dev, "can't get the TX DMA channel, error %d!\n", ret);
> +		lpi2c_imx->dma_tx = NULL;
> +		goto err;
> +	}
> +
> +	dma_sconfig.dst_addr = lpi2c_imx->phy_addr + LPI2C_MTDR;
> +	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +	dma_sconfig.dst_maxburst = 1;
> +	dma_sconfig.direction = DMA_MEM_TO_DEV;
> +	ret = dmaengine_slave_config(lpi2c_imx->dma_tx, &dma_sconfig);
> +	if (ret < 0) {
> +		dev_err(dev, "can't configure tx channel (%d)\n", ret);
> +		goto fail_tx;
> +	}
> +
> +	/* Prepare for RX DMA: */
> +	lpi2c_imx->dma_rx = dma_request_chan(dev, "rx");
> +	if (IS_ERR(lpi2c_imx->dma_rx)) {
> +		ret = PTR_ERR(lpi2c_imx->dma_rx);
> +		dev_err(dev, "can't get the RX DMA channel, error %d\n", ret);
> +		lpi2c_imx->dma_rx = NULL;
> +		goto fail_tx;
> +	}
> +
> +	dma_sconfig.src_addr = lpi2c_imx->phy_addr + LPI2C_MRDR;
> +	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +	dma_sconfig.src_maxburst = 1;
> +	dma_sconfig.direction = DMA_DEV_TO_MEM;
> +	ret = dmaengine_slave_config(lpi2c_imx->dma_rx, &dma_sconfig);
> +	if (ret < 0) {
> +		dev_err(dev, "can't configure rx channel (%d)\n", ret);
> +		goto fail_rx;
> +	}
> +
> +	lpi2c_imx->can_use_dma = true;
> +	lpi2c_imx->using_dma = false;
> +
> +	return 0;
> +fail_rx:
> +	dma_release_channel(lpi2c_imx->dma_rx);
> +fail_tx:
> +	dma_release_channel(lpi2c_imx->dma_tx);
> +err:
> +	lpi2c_dma_exit(lpi2c_imx);
> +	lpi2c_imx->can_use_dma = false;
> +	return ret;
> +}
> +
>  static int lpi2c_imx_probe(struct platform_device *pdev)  {
>  	struct lpi2c_imx_struct *lpi2c_imx;
>  	unsigned int temp;
>  	int ret;
> +	struct resource *res;
> 
>  	lpi2c_imx = devm_kzalloc(&pdev->dev, sizeof(*lpi2c_imx), GFP_KERNEL);
>  	if (!lpi2c_imx)
>  		return -ENOMEM;
> 
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	lpi2c_imx->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(lpi2c_imx->base))
>  		return PTR_ERR(lpi2c_imx->base);
> 
> +	lpi2c_imx->phy_addr = (dma_addr_t)res->start;
>  	lpi2c_imx->irq = platform_get_irq(pdev, 0);
>  	if (lpi2c_imx->irq < 0)
>  		return lpi2c_imx->irq;
> @@ -691,6 +966,18 @@ static int lpi2c_imx_probe(struct platform_device
> *pdev)
>  	if (ret == -EPROBE_DEFER)
>  		goto rpm_disable;
> 
> +	/* Init DMA */
> +	lpi2c_imx->dma_direction = DMA_NONE;
> +	lpi2c_imx->dma_rx = lpi2c_imx->dma_tx = NULL;

Unnecessary line

> +	ret = lpi2c_dma_init(&pdev->dev, lpi2c_imx);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "dma setup error %d, use pio\n",
> ret);
> +		if (ret == -EPROBE_DEFER)
> +			goto rpm_disable;
> +	}
> +
> +	init_completion(&lpi2c_imx->complete);
> +
>  	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