diff mbox series

[v6] i2c: imx: support DMA defer probing

Message ID 20241223034416.544022-1-carlos.song@nxp.com
State New
Headers show
Series [v6] i2c: imx: support DMA defer probing | expand

Commit Message

Carlos Song Dec. 23, 2024, 3:44 a.m. UTC 
Return -EPROBE_DEFER when dma_request_slave_channel() because DMA driver
have not ready yet.

Move i2c_imx_dma_request() before registering I2C adapter to avoid
infinite loop of .probe() calls to the same driver, see "e8c220fac415
Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()""
and "Documentation/driver-api/driver-model/driver.rst".

Use CPU mode to avoid stuck registering i2c adapter when DMA resources
are unavailable.

Signed-off-by: Carlos Song <carlos.song@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Acked-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
Change for V6:
- According to Ahmad and Oleksij's comment, use dev_err to print error
  log in failing setting up DMA path and simplify code comment.
Change for V5:
- Add Ahmad Acked-by. No code change.
Change for V4:
- Output "Only use PIO mode" log in debug level when no DMA configure.
Change for V3:
- According to Marc's comment, remove error print when defer probe.
  Add info log when DMA has not been enabled and add error log when
  DMA error, both won't stuck the i2c adapter register, just for reminding,
  i2c adapter is working only in PIO mode.
Change for V2:
- According to Frank's comments, wrap at 75 char and Simplify fix code
  at i2c_imx_dma_request().
- Use strict patch check, fix alignment warning at i2c_imx_dma_request()
---
 drivers/i2c/busses/i2c-imx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

Comments

Andi Shyti Dec. 23, 2024, 11:13 p.m. UTC | #1 
Hi Carlos,

...

> @@ -1802,6 +1803,18 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	if (ret == -EPROBE_DEFER)
>  		goto clk_notifier_unregister;
>  
> +	/* As we can always fall back to PIO, let's ignore the error setting up DMA. */
> +	ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			goto clk_notifier_unregister;
> +		else if (ret == -ENODEV)
> +			dev_dbg(&pdev->dev, "Only use PIO mode\n");
> +		else
> +			dev_err(&pdev->dev, "Failed to setup DMA (%pe), only use PIO mode\n",
> +				ERR_PTR(ret));

My question here is not just about the use of dev_err vs
dev_err_probe, but why don't we exit the probe if we get an
error.

We should use PIO only in case of ENODEV, in all the other cases
I think we should just leave. E.g. why don't we exit if we meet
ret == -ENOMEM?

Andi
Carlos Song Dec. 24, 2024, 2:55 a.m. UTC | #2 
> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Tuesday, December 24, 2024 7:14 AM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: o.rempel@pengutronix.de; kernel@pengutronix.de; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; linux-i2c@vger.kernel.org;
> imx@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; Clark Wang <xiaoning.wang@nxp.com>; Ahmad
> Fatoum <a.fatoum@pengutronix.de>
> Subject: [EXT] Re: [PATCH v6] i2c: imx: support DMA defer probing
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> Hi Carlos,
> 
> ...
> 
> > @@ -1802,6 +1803,18 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
> >       if (ret == -EPROBE_DEFER)
> >               goto clk_notifier_unregister;
> >
> > +     /* As we can always fall back to PIO, let's ignore the error setting up
> DMA. */
> > +     ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> > +     if (ret) {
> > +             if (ret == -EPROBE_DEFER)
> > +                     goto clk_notifier_unregister;
> > +             else if (ret == -ENODEV)
> > +                     dev_dbg(&pdev->dev, "Only use PIO mode\n");
> > +             else
> > +                     dev_err(&pdev->dev, "Failed to setup DMA (%pe),
> only use PIO mode\n",
> > +                             ERR_PTR(ret));
> 
> My question here is not just about the use of dev_err vs dev_err_probe, but why
> don't we exit the probe if we get an error.
> 
> We should use PIO only in case of ENODEV, in all the other cases I think we
> should just leave. E.g. why don't we exit if we meet ret == -ENOMEM?

Hi, Andi

Thank you! From my point, I2C is critical bus so it should be available as much as possible.
-ENOMEM or other unknown errors all are from i2c_imx_dma_request(). So error happened in enable DMA mode process.

As I comment at previous mail[1]:
DMA mode should be optional for i2c-imx[2], i2c-imx can accept DMA mode not enabled.
Even though DMA mode can not be enabled by some known/unknown issue, I2C still can work in PIO mode in all time for all cases.
As a result, don't exit the I2C probe and only print error to show i2c DMA error.

This patch just is used to make i2c-imx can support defer probe to use DMA resources as much as possible.

If meet a DMA error then exit i2c probe, which means binding I2C to DMA, this is not what we expect. Once the DMA encounters a problem,
the entire I2C bus and peripherals will not be able to start, this is not a small damage, so we use current logic.

[1]: https://lore.kernel.org/imx/AM0PR0402MB39374E34FD6133B5E3D414D7E82F2@AM0PR0402MB3937.eurprd04.prod.outlook.com/
[2]:
commit ce1a78840ff7ab846065d5b65eaac959bafe1949
Author: Yao Yuan <yao.yuan@freescale.com>
Date:   Tue Nov 18 18:31:06 2014 +0800

    i2c: imx: add DMA support for freescale i2c driver

    Add dma support for i2c. This function depend on DMA driver.
    You can turn on it by write both the dmas and dma-name properties in dts node.
    DMA is optional, even DMA request unsuccessfully, i2c can also work well.

    Signed-off-by: Yuan Yao <yao.yuan@freescale.com>
    Signed-off-by: Wolfram Sang <wsa@the-dreams.de>

Carlos
> 
> Andi
Andi Shyti Dec. 24, 2024, 8:32 a.m. UTC | #3 
> > > @@ -1802,6 +1803,18 @@ static int i2c_imx_probe(struct platform_device
> > *pdev)
> > >       if (ret == -EPROBE_DEFER)
> > >               goto clk_notifier_unregister;
> > >
> > > +     /* As we can always fall back to PIO, let's ignore the error setting up
> > DMA. */
> > > +     ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> > > +     if (ret) {
> > > +             if (ret == -EPROBE_DEFER)
> > > +                     goto clk_notifier_unregister;
> > > +             else if (ret == -ENODEV)
> > > +                     dev_dbg(&pdev->dev, "Only use PIO mode\n");
> > > +             else
> > > +                     dev_err(&pdev->dev, "Failed to setup DMA (%pe),
> > only use PIO mode\n",
> > > +                             ERR_PTR(ret));
> > 
> > My question here is not just about the use of dev_err vs dev_err_probe, but why
> > don't we exit the probe if we get an error.
> > 
> > We should use PIO only in case of ENODEV, in all the other cases I think we
> > should just leave. E.g. why don't we exit if we meet ret == -ENOMEM?
> 
> Hi, Andi
> 
> Thank you! From my point, I2C is critical bus so it should be available as much as possible.
> -ENOMEM or other unknown errors all are from i2c_imx_dma_request(). So error happened in enable DMA mode process.

OK, makes sense, it's the idea of "let things fail on their own,
I'll move forward as much as I can"; we need to be aware of
the choice. Please add a comment above.

But then it's not an error, but a warning. With errors we bail
out, with warnings we tell users that something went wrong.

Sorry for keeping you on this point for so long, but do you mind
swapping this dev_err in dev_warn, with a comment explaining the
reason we decided not to leave?

Thanks,
Andi
Carlos Song Dec. 25, 2024, 10:56 a.m. UTC | #4 
> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Tuesday, December 24, 2024 4:33 PM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: o.rempel@pengutronix.de; kernel@pengutronix.de; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; linux-i2c@vger.kernel.org;
> imx@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; Clark Wang <xiaoning.wang@nxp.com>; Ahmad
> Fatoum <a.fatoum@pengutronix.de>
> Subject: [EXT] Re: [PATCH v6] i2c: imx: support DMA defer probing
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> > > > @@ -1802,6 +1803,18 @@ static int i2c_imx_probe(struct
> > > > platform_device
> > > *pdev)
> > > >       if (ret == -EPROBE_DEFER)
> > > >               goto clk_notifier_unregister;
> > > >
> > > > +     /* As we can always fall back to PIO, let's ignore the error
> > > > + setting up
> > > DMA. */
> > > > +     ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> > > > +     if (ret) {
> > > > +             if (ret == -EPROBE_DEFER)
> > > > +                     goto clk_notifier_unregister;
> > > > +             else if (ret == -ENODEV)
> > > > +                     dev_dbg(&pdev->dev, "Only use PIO mode\n");
> > > > +             else
> > > > +                     dev_err(&pdev->dev, "Failed to setup DMA
> > > > + (%pe),
> > > only use PIO mode\n",
> > > > +                             ERR_PTR(ret));
> > >
> > > My question here is not just about the use of dev_err vs
> > > dev_err_probe, but why don't we exit the probe if we get an error.
> > >
> > > We should use PIO only in case of ENODEV, in all the other cases I
> > > think we should just leave. E.g. why don't we exit if we meet ret ==
> -ENOMEM?
> >
> > Hi, Andi
> >
> > Thank you! From my point, I2C is critical bus so it should be available as much
> as possible.
> > -ENOMEM or other unknown errors all are from i2c_imx_dma_request(). So
> error happened in enable DMA mode process.
> 
> OK, makes sense, it's the idea of "let things fail on their own, I'll move forward as
> much as I can"; we need to be aware of the choice. Please add a comment
> above.
> 
> But then it's not an error, but a warning. With errors we bail out, with warnings
> we tell users that something went wrong.
> 
> Sorry for keeping you on this point for so long, but do you mind swapping this
> dev_err in dev_warn, with a comment explaining the reason we decided not to
> leave?
> 

Hi, Andi

It doesn't matter! I am very happy to receive so many suggestions to help enhance the patch.
I will do following things at V7:
1. Change dev_err to dev_warn
2. Use a more detailed comment to explain why we decided not to leave when meet DMA error.

Thank you.
Carlos

> Thanks,
> Andi
Andi Shyti Dec. 25, 2024, 10:42 p.m. UTC | #5 
Hi Carlos,

On Wed, Dec 25, 2024 at 10:56:34AM +0000, Carlos Song wrote:
> > > > > @@ -1802,6 +1803,18 @@ static int i2c_imx_probe(struct
> > > > > platform_device
> > > > *pdev)
> > > > >       if (ret == -EPROBE_DEFER)
> > > > >               goto clk_notifier_unregister;
> > > > >
> > > > > +     /* As we can always fall back to PIO, let's ignore the error
> > > > > + setting up
> > > > DMA. */
> > > > > +     ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> > > > > +     if (ret) {
> > > > > +             if (ret == -EPROBE_DEFER)
> > > > > +                     goto clk_notifier_unregister;
> > > > > +             else if (ret == -ENODEV)
> > > > > +                     dev_dbg(&pdev->dev, "Only use PIO mode\n");
> > > > > +             else
> > > > > +                     dev_err(&pdev->dev, "Failed to setup DMA
> > > > > + (%pe),
> > > > only use PIO mode\n",
> > > > > +                             ERR_PTR(ret));
> > > >
> > > > My question here is not just about the use of dev_err vs
> > > > dev_err_probe, but why don't we exit the probe if we get an error.
> > > >
> > > > We should use PIO only in case of ENODEV, in all the other cases I
> > > > think we should just leave. E.g. why don't we exit if we meet ret ==
> > -ENOMEM?
> > >
> > > Hi, Andi
> > >
> > > Thank you! From my point, I2C is critical bus so it should be available as much
> > as possible.
> > > -ENOMEM or other unknown errors all are from i2c_imx_dma_request(). So
> > error happened in enable DMA mode process.
> > 
> > OK, makes sense, it's the idea of "let things fail on their own, I'll move forward as
> > much as I can"; we need to be aware of the choice. Please add a comment
> > above.
> > 
> > But then it's not an error, but a warning. With errors we bail out, with warnings
> > we tell users that something went wrong.
> > 
> > Sorry for keeping you on this point for so long, but do you mind swapping this
> > dev_err in dev_warn, with a comment explaining the reason we decided not to
> > leave?
> > 
> 
> Hi, Andi
> 
> It doesn't matter! I am very happy to receive so many suggestions to help enhance the patch.
> I will do following things at V7:
> 1. Change dev_err to dev_warn
> 2. Use a more detailed comment to explain why we decided not to leave when meet DMA error.

Thank you! I appreciate your responsiveness!

Andi
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 1f441227bfdc..9ac02640fdff 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -397,17 +397,16 @@  static void i2c_imx_reset_regs(struct imx_i2c_struct *i2c_imx)
 }
 
 /* Functions for DMA support */
-static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
-						dma_addr_t phy_addr)
+static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dma_addr_t phy_addr)
 {
 	struct imx_i2c_dma *dma;
 	struct dma_slave_config dma_sconfig;
-	struct device *dev = &i2c_imx->adapter.dev;
+	struct device *dev = i2c_imx->adapter.dev.parent;
 	int ret;
 
 	dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
 	if (!dma)
-		return;
+		return -ENOMEM;
 
 	dma->chan_tx = dma_request_chan(dev, "tx");
 	if (IS_ERR(dma->chan_tx)) {
@@ -452,7 +451,7 @@  static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n",
 		dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx));
 
-	return;
+	return 0;
 
 fail_rx:
 	dma_release_channel(dma->chan_rx);
@@ -460,6 +459,8 @@  static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	dma_release_channel(dma->chan_tx);
 fail_al:
 	devm_kfree(dev, dma);
+
+	return ret;
 }
 
 static void i2c_imx_dma_callback(void *arg)
@@ -1802,6 +1803,18 @@  static int i2c_imx_probe(struct platform_device *pdev)
 	if (ret == -EPROBE_DEFER)
 		goto clk_notifier_unregister;
 
+	/* As we can always fall back to PIO, let's ignore the error setting up DMA. */
+	ret = i2c_imx_dma_request(i2c_imx, phy_addr);
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			goto clk_notifier_unregister;
+		else if (ret == -ENODEV)
+			dev_dbg(&pdev->dev, "Only use PIO mode\n");
+		else
+			dev_err(&pdev->dev, "Failed to setup DMA (%pe), only use PIO mode\n",
+				ERR_PTR(ret));
+	}
+
 	/* Add I2C adapter */
 	ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
 	if (ret < 0)
@@ -1816,9 +1829,6 @@  static int i2c_imx_probe(struct platform_device *pdev)
 		i2c_imx->adapter.name);
 	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
 
-	/* Init DMA config if supported */
-	i2c_imx_dma_request(i2c_imx, phy_addr);
-
 	return 0;   /* Return OK */
 
 clk_notifier_unregister: