linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] i2c: cadence: Fix regression with bus recovery
@ 2022-11-25 11:51 carsten.haitzler
  2022-11-28  6:24 ` Datta, Shubhrajyoti
  0 siblings, 1 reply; 3+ messages in thread
From: carsten.haitzler @ 2022-11-25 11:51 UTC (permalink / raw)
  To: michal.simek, shubhrajyoti.datta, linux-arm-kernel, linux-i2c,
	linux-kernel
  Cc: torvalds

From: Carsten Haitzler <carsten.haitzler@arm.com>

Commit "i2c: cadence: Add standard bus recovery support" breaks for i2c
devices that have no pinctrl defined. There is no requirement for this
to exist in the DT. This has worked perfectly well without this before in
at least 1 real usage case on hardware (Mali Komeda DPU, Cadence i2c to
talk to a tda99xx phy). Adding the requirement to have pinctrl set up in
the device tree (or otherwise be found) is a regression where the whole
i2c device is lost entirely (in this case dropping entire devices which
then leads to the drm display stack unable to find the phy for display
output, thus having no drm display device and so on down the chain).

This converts the above commit to an enhancement if pinctrl can be found
for the i2c device, providing a timeout on read with recovery, but if not,
do what used to be done rather than a fatal loss of a device.

This restores the mentioned display devices to their working state again.

Fixes: 58b924241d0a ("i2c: cadence: Add standard bus recovery support")
Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
---
Note: This issue was discovered during the porting of the linux kernel
on Morello [1].

[1] https://git.morello-project.org/morello/kernel/linux
---
 drivers/i2c/busses/i2c-cadence.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index fe0cd205502d..40d0cc6bb996 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -852,7 +852,8 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 					 CDNS_I2C_POLL_US, CDNS_I2C_TIMEOUT_US);
 	if (ret) {
 		ret = -EAGAIN;
-		i2c_recover_bus(adap);
+		if (id->adap.bus_recovery_info)
+			i2c_recover_bus(adap);
 		goto out;
 	}
 
@@ -1262,10 +1263,10 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 	}
 
 	id->rinfo.pinctrl = devm_pinctrl_get(&pdev->dev);
-	if (IS_ERR(id->rinfo.pinctrl)) {
+	if (IS_ERR(id->rinfo.pinctrl))
 		dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
-		return PTR_ERR(id->rinfo.pinctrl);
-	}
+	else
+		id->adap.bus_recovery_info = &id->rinfo;
 
 	id->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &r_mem);
 	if (IS_ERR(id->membase))
@@ -1283,7 +1284,6 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 	id->adap.retries = 3;		/* Default retry value. */
 	id->adap.algo_data = id;
 	id->adap.dev.parent = &pdev->dev;
-	id->adap.bus_recovery_info = &id->rinfo;
 	init_completion(&id->xfer_done);
 	snprintf(id->adap.name, sizeof(id->adap.name),
 		 "Cadence I2C at %08lx", (unsigned long)r_mem->start);
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* RE: [PATCH] i2c: cadence: Fix regression with bus recovery
  2022-11-25 11:51 [PATCH] i2c: cadence: Fix regression with bus recovery carsten.haitzler
@ 2022-11-28  6:24 ` Datta, Shubhrajyoti
  2022-11-28 10:47   ` Carsten Haitzler
  0 siblings, 1 reply; 3+ messages in thread
From: Datta, Shubhrajyoti @ 2022-11-28  6:24 UTC (permalink / raw)
  To: carsten.haitzler, michal.simek, shubhrajyoti.datta,
	linux-arm-kernel, linux-i2c, linux-kernel
  Cc: torvalds

[-- Attachment #1: Type: text/plain, Size: 4426 bytes --]

[AMD Official Use Only - General]


Hi ,
Thanks for the patch

> -----Original Message-----
> From: carsten.haitzler@foss.arm.com <carsten.haitzler@foss.arm.com>
> Sent: Friday, November 25, 2022 5:21 PM
> To: michal.simek@xilinx.com; shubhrajyoti.datta@xilinx.com; linux-arm-
> kernel@lists.infradead.org; linux-i2c@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: torvalds@linux-foundation.org
> Subject: [PATCH] i2c: cadence: Fix regression with bus recovery
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> From: Carsten Haitzler <carsten.haitzler@arm.com>
> 
> Commit "i2c: cadence: Add standard bus recovery support" breaks for i2c
> devices that have no pinctrl defined. There is no requirement for this to exist
> in the DT. This has worked perfectly well without this before in at least 1 real
> usage case on hardware (Mali Komeda DPU, Cadence i2c to talk to a tda99xx
> phy). Adding the requirement to have pinctrl set up in the device tree (or
> otherwise be found) is a regression where the whole i2c device is lost
> entirely (in this case dropping entire devices which then leads to the drm
> display stack unable to find the phy for display output, thus having no drm
> display device and so on down the chain).
> 
> This converts the above commit to an enhancement if pinctrl can be found
> for the i2c device, providing a timeout on read with recovery, but if not, do
> what used to be done rather than a fatal loss of a device.
> 
> This restores the mentioned display devices to their working state again.
> 
> Fixes: 58b924241d0a ("i2c: cadence: Add standard bus recovery support")
> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
> ---
> Note: This issue was discovered during the porting of the linux kernel on
> Morello [1].
> 
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> morello-
> project.org%2Fmorello%2Fkernel%2Flinux&amp;data=05%7C01%7Cshubhraj
> yoti.datta%40amd.com%7C651e141296fb497b6c2e08dacedb65ad%7C3dd896
> 1fe4884e608e11a82d994e183d%7C0%7C0%7C638049738949855658%7CUnkn
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik
> 1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ny08zTdgJKxXC
> dwYaCWFS3mwd65LDqFAIYfjao6gO8Y%3D&amp;reserved=0
> ---
>  drivers/i2c/busses/i2c-cadence.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-
> cadence.c
> index fe0cd205502d..40d0cc6bb996 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -852,7 +852,8 @@ static int cdns_i2c_master_xfer(struct i2c_adapter
> *adap, struct i2c_msg *msgs,
>                                          CDNS_I2C_POLL_US, CDNS_I2C_TIMEOUT_US);
>         if (ret) {
>                 ret = -EAGAIN;
> -               i2c_recover_bus(adap);
> +               if (id->adap.bus_recovery_info)
> +                       i2c_recover_bus(adap);
>                 goto out;
>         }
> 
> @@ -1262,10 +1263,10 @@ static int cdns_i2c_probe(struct platform_device
> *pdev)
>         }
> 
>         id->rinfo.pinctrl = devm_pinctrl_get(&pdev->dev);
> -       if (IS_ERR(id->rinfo.pinctrl)) {
> +       if (IS_ERR(id->rinfo.pinctrl))
>                 dev_info(&pdev->dev, "can't get pinctrl, bus recovery not
> supported\n");
> -               return PTR_ERR(id->rinfo.pinctrl);
> -       }
> +       else
> +               id->adap.bus_recovery_info = &id->rinfo;

May be we could differentiate between  ENODEV and  other errors.
As we want to do this for the case when it is not provided.


> 
>         id->membase = devm_platform_get_and_ioremap_resource(pdev, 0,
> &r_mem);
>         if (IS_ERR(id->membase))
> @@ -1283,7 +1284,6 @@ static int cdns_i2c_probe(struct platform_device
> *pdev)
>         id->adap.retries = 3;           /* Default retry value. */
>         id->adap.algo_data = id;
>         id->adap.dev.parent = &pdev->dev;
> -       id->adap.bus_recovery_info = &id->rinfo;
>         init_completion(&id->xfer_done);
>         snprintf(id->adap.name, sizeof(id->adap.name),
>                  "Cadence I2C at %08lx", (unsigned long)r_mem->start);
> --
> 2.32.0

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 18579 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] i2c: cadence: Fix regression with bus recovery
  2022-11-28  6:24 ` Datta, Shubhrajyoti
@ 2022-11-28 10:47   ` Carsten Haitzler
  0 siblings, 0 replies; 3+ messages in thread
From: Carsten Haitzler @ 2022-11-28 10:47 UTC (permalink / raw)
  To: Datta, Shubhrajyoti, michal.simek, shubhrajyoti.datta,
	linux-arm-kernel, linux-i2c, linux-kernel
  Cc: torvalds



On 11/28/22 06:24, Datta, Shubhrajyoti wrote:
> [AMD Official Use Only - General]
> 
> 
> Hi ,
> Thanks for the patch
> 
>> -----Original Message-----
>> From: carsten.haitzler@foss.arm.com <carsten.haitzler@foss.arm.com>
>> Sent: Friday, November 25, 2022 5:21 PM
>> To: michal.simek@xilinx.com; shubhrajyoti.datta@xilinx.com; linux-arm-
>> kernel@lists.infradead.org; linux-i2c@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Cc: torvalds@linux-foundation.org
>> Subject: [PATCH] i2c: cadence: Fix regression with bus recovery
>>
>> CAUTION: This message has originated from an External Source. Please use
>> proper judgment and caution when opening attachments, clicking links, or
>> responding to this email.
>>
>>
>> From: Carsten Haitzler <carsten.haitzler@arm.com>
>>
>> Commit "i2c: cadence: Add standard bus recovery support" breaks for i2c
>> devices that have no pinctrl defined. There is no requirement for this to exist
>> in the DT. This has worked perfectly well without this before in at least 1 real
>> usage case on hardware (Mali Komeda DPU, Cadence i2c to talk to a tda99xx
>> phy). Adding the requirement to have pinctrl set up in the device tree (or
>> otherwise be found) is a regression where the whole i2c device is lost
>> entirely (in this case dropping entire devices which then leads to the drm
>> display stack unable to find the phy for display output, thus having no drm
>> display device and so on down the chain).
>>
>> This converts the above commit to an enhancement if pinctrl can be found
>> for the i2c device, providing a timeout on read with recovery, but if not, do
>> what used to be done rather than a fatal loss of a device.
>>
>> This restores the mentioned display devices to their working state again.
>>
>> Fixes: 58b924241d0a ("i2c: cadence: Add standard bus recovery support")
>> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
>> ---
>> Note: This issue was discovered during the porting of the linux kernel on
>> Morello [1].
>>
>> [1]
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
>> morello-
>> project.org%2Fmorello%2Fkernel%2Flinux&amp;data=05%7C01%7Cshubhraj
>> yoti.datta%40amd.com%7C651e141296fb497b6c2e08dacedb65ad%7C3dd896
>> 1fe4884e608e11a82d994e183d%7C0%7C0%7C638049738949855658%7CUnkn
>> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik
>> 1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ny08zTdgJKxXC
>> dwYaCWFS3mwd65LDqFAIYfjao6gO8Y%3D&amp;reserved=0
>> ---
>>   drivers/i2c/busses/i2c-cadence.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-
>> cadence.c
>> index fe0cd205502d..40d0cc6bb996 100644
>> --- a/drivers/i2c/busses/i2c-cadence.c
>> +++ b/drivers/i2c/busses/i2c-cadence.c
>> @@ -852,7 +852,8 @@ static int cdns_i2c_master_xfer(struct i2c_adapter
>> *adap, struct i2c_msg *msgs,
>>                                           CDNS_I2C_POLL_US, CDNS_I2C_TIMEOUT_US);
>>          if (ret) {
>>                  ret = -EAGAIN;
>> -               i2c_recover_bus(adap);
>> +               if (id->adap.bus_recovery_info)
>> +                       i2c_recover_bus(adap);
>>                  goto out;
>>          }
>>
>> @@ -1262,10 +1263,10 @@ static int cdns_i2c_probe(struct platform_device
>> *pdev)
>>          }
>>
>>          id->rinfo.pinctrl = devm_pinctrl_get(&pdev->dev);
>> -       if (IS_ERR(id->rinfo.pinctrl)) {
>> +       if (IS_ERR(id->rinfo.pinctrl))
>>                  dev_info(&pdev->dev, "can't get pinctrl, bus recovery not
>> supported\n");
>> -               return PTR_ERR(id->rinfo.pinctrl);
>> -       }
>> +       else
>> +               id->adap.bus_recovery_info = &id->rinfo;
> 
> May be we could differentiate between  ENODEV and  other errors.
> As we want to do this for the case when it is not provided.

We could, but just keep in mind this patch is essentially restoring 
behavior on any pinctrl error to previous behavior - so trying to undo 
the regression as strictly as possible.

Certainly checking specifically for ENODEV does fix it for me which is 
the case of no pinctrl. I'll send a v2.

>>          id->membase = devm_platform_get_and_ioremap_resource(pdev, 0,
>> &r_mem);
>>          if (IS_ERR(id->membase))
>> @@ -1283,7 +1284,6 @@ static int cdns_i2c_probe(struct platform_device
>> *pdev)
>>          id->adap.retries = 3;           /* Default retry value. */
>>          id->adap.algo_data = id;
>>          id->adap.dev.parent = &pdev->dev;
>> -       id->adap.bus_recovery_info = &id->rinfo;
>>          init_completion(&id->xfer_done);
>>          snprintf(id->adap.name, sizeof(id->adap.name),
>>                   "Cadence I2C at %08lx", (unsigned long)r_mem->start);
>> --
>> 2.32.0

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-11-28 10:48 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-25 11:51 [PATCH] i2c: cadence: Fix regression with bus recovery carsten.haitzler
2022-11-28  6:24 ` Datta, Shubhrajyoti
2022-11-28 10:47   ` Carsten Haitzler

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).