[PATCH RFC] drivers/usb/ehci-fsl: Fix interrupt setup in host mode.

[PATCH RFC] drivers/usb/ehci-fsl: Fix interrupt setup in host mode.

[Darren Stevens]

In patch a1a2b7125e1079 (Drop static setup of IRQ resource from DT
core) we stopped platform_get_resource() from returning the IRQ, as all
drivers were supposed to have switched to platform_get_irq()
Unfortunately the Freescale EHCI driver in host mode got missed. Fix
it. Also fix allocation of resources to work with current kernel.

Fixes:a1a2b7125e1079 (Drop static setup of IRQ resource from DT core)
Reported-by Christian Zigotzky <chzigotzky@xenosoft.de>
Signed-off-by Darren Stevens <darren@stevens-zone.net>
---
Tested on AmigaOne X5000/20 and X5000/40 not sure if this is entirely
correct fix though. Contains code by Rob Herring (in fsl-mph-dr-of.c)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 385be30..d0bf7fb 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/fsl_devices.h>
 #include <linux/of_platform.h>
+#include <linux/of_address.h>
 #include <linux/io.h>
 
 #include "ehci.h"
@@ -46,9 +47,10 @@ static struct hc_driver __read_mostly
fsl_ehci_hc_driver; */
 static int fsl_ehci_drv_probe(struct platform_device *pdev)
 {
+	struct device_node *dn = pdev->dev.of_node;
 	struct fsl_usb2_platform_data *pdata;
 	struct usb_hcd *hcd;
-	struct resource *res;
+	struct resource res;
 	int irq;
 	int retval;
 	u32 tmp;
@@ -76,14 +78,10 @@ static int fsl_ehci_drv_probe(struct
platform_device *pdev) return -ENODEV;
 	}
 
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!res) {
-		dev_err(&pdev->dev,
-			"Found HC with no IRQ. Check %s setup!\n",
-			dev_name(&pdev->dev));
-		return -ENODEV;
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		return irq;
 	}
-	irq = res->start;
 
 	hcd = __usb_create_hcd(&fsl_ehci_hc_driver, pdev->dev.parent,
 			       &pdev->dev, dev_name(&pdev->dev), NULL);
@@ -92,15 +90,21 @@ static int fsl_ehci_drv_probe(struct
platform_device *pdev) goto err1;
 	}
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	hcd->regs = devm_ioremap_resource(&pdev->dev, res);
+	platform_set_drvdata(pdev, hcd);
+	pdev->dev.platform_data = pdata;
+
+	tmp = of_address_to_resource(dn, 0, &res);
+	if (tmp)
+		return tmp;
+
+	hcd->regs = devm_ioremap_resource(&pdev->dev, &res);
 	if (IS_ERR(hcd->regs)) {
 		retval = PTR_ERR(hcd->regs);
 		goto err2;
 	}
 
-	hcd->rsrc_start = res->start;
-	hcd->rsrc_len = resource_size(res);
+	hcd->rsrc_start = res.start;
+	hcd->rsrc_len = resource_size(&res);
 
 	pdata->regs = hcd->regs;
 
diff --git a/drivers/usb/host/fsl-mph-dr-of.c
b/drivers/usb/host/fsl-mph-dr-of.c index 44a7e58..766e4ab 100644
--- a/drivers/usb/host/fsl-mph-dr-of.c
+++ b/drivers/usb/host/fsl-mph-dr-of.c
@@ -80,8 +80,6 @@ static struct platform_device
*fsl_usb2_device_register( const char *name, int id)
 {
 	struct platform_device *pdev;
-	const struct resource *res = ofdev->resource;
-	unsigned int num = ofdev->num_resources;
 	int retval;
 
 	pdev = platform_device_alloc(name, id);
@@ -106,11 +104,8 @@ static struct platform_device
*fsl_usb2_device_register( if (retval)
 		goto error;
 
-	if (num) {
-		retval = platform_device_add_resources(pdev, res, num);
-		if (retval)
-			goto error;
-	}
+	pdev->dev.of_node = ofdev->dev.of_node;
+	pdev->dev.of_node_reused = true;
 
 	retval = platform_device_add(pdev);
 	if (retval)

Re: [PATCH RFC] drivers/usb/ehci-fsl: Fix interrupt setup in host mode.

[Greg KH]

On Tue, Jun 21, 2022 at 11:09:41PM +0100, Darren Stevens wrote:
> In patch a1a2b7125e1079 (Drop static setup of IRQ resource from DT
> core) we stopped platform_get_resource() from returning the IRQ, as all
> drivers were supposed to have switched to platform_get_irq()
> Unfortunately the Freescale EHCI driver in host mode got missed. Fix
> it. Also fix allocation of resources to work with current kernel.
> 
> Fixes:a1a2b7125e1079 (Drop static setup of IRQ resource from DT core)

Nit, please put a space after the :.

Also not that many characters are needed, as you can see in our
documentation, this is the proper format:

Fixes: a1a2b7125e10 ("of/platform: Drop static setup of IRQ resource from DT core")

> Reported-by Christian Zigotzky <chzigotzky@xenosoft.de>
> Signed-off-by Darren Stevens <darren@stevens-zone.net>
> ---
> Tested on AmigaOne X5000/20 and X5000/40 not sure if this is entirely
> correct fix though. Contains code by Rob Herring (in fsl-mph-dr-of.c)
> 
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 385be30..d0bf7fb 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -23,6 +23,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/fsl_devices.h>
>  #include <linux/of_platform.h>
> +#include <linux/of_address.h>
>  #include <linux/io.h>
>  
>  #include "ehci.h"
> @@ -46,9 +47,10 @@ static struct hc_driver __read_mostly
> fsl_ehci_hc_driver; */
>  static int fsl_ehci_drv_probe(struct platform_device *pdev)
>  {
> +	struct device_node *dn = pdev->dev.of_node;
>  	struct fsl_usb2_platform_data *pdata;
>  	struct usb_hcd *hcd;
> -	struct resource *res;
> +	struct resource res;
>  	int irq;
>  	int retval;
>  	u32 tmp;
> @@ -76,14 +78,10 @@ static int fsl_ehci_drv_probe(struct
> platform_device *pdev) return -ENODEV;
>  	}
>  
> -	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev,
> -			"Found HC with no IRQ. Check %s setup!\n",
> -			dev_name(&pdev->dev));
> -		return -ENODEV;
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		return irq;
>  	}

Did you run checkpatch on this?  Coding style is not correct :(

thanks,

greg k-h

Re: [PATCH RFC] drivers/usb/ehci-fsl: Fix interrupt setup in host mode.

[Michael Ellerman]

Darren Stevens <darren@stevens-zone.net> writes:
> In patch a1a2b7125e1079 (Drop static setup of IRQ resource from DT
> core) we stopped platform_get_resource() from returning the IRQ, as all
> drivers were supposed to have switched to platform_get_irq()
> Unfortunately the Freescale EHCI driver in host mode got missed. Fix
> it. Also fix allocation of resources to work with current kernel.
>
> Fixes:a1a2b7125e1079 (Drop static setup of IRQ resource from DT core)
> Reported-by Christian Zigotzky <chzigotzky@xenosoft.de>
> Signed-off-by Darren Stevens <darren@stevens-zone.net>
> ---
> Tested on AmigaOne X5000/20 and X5000/40 not sure if this is entirely
> correct fix though. Contains code by Rob Herring (in fsl-mph-dr-of.c)

It looks like this driver is used on some arm/arm64 boards:

  $ git grep -l fsl-usb2-dr arch/arm*/boot/dts
  arch/arm/boot/dts/ls1021a.dtsi
  arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi

Which is for the "Layerscape-1012A family SoC".

Have we heard of any bug reports from users of those boards? Is it wired
up differently or otherwise immune to the problem?

I've added the Layerscape maintainers to Cc.

cheers

> diff --git a/drivers/usb/host/fsl-mph-dr-of.c
> b/drivers/usb/host/fsl-mph-dr-of.c index 44a7e58..766e4ab 100644
> --- a/drivers/usb/host/fsl-mph-dr-of.c
> +++ b/drivers/usb/host/fsl-mph-dr-of.c
> @@ -80,8 +80,6 @@ static struct platform_device
> *fsl_usb2_device_register( const char *name, int id)
>  {
>  	struct platform_device *pdev;
> -	const struct resource *res = ofdev->resource;
> -	unsigned int num = ofdev->num_resources;
>  	int retval;
>  
>  	pdev = platform_device_alloc(name, id);
> @@ -106,11 +104,8 @@ static struct platform_device
> *fsl_usb2_device_register( if (retval)
>  		goto error;
>  
> -	if (num) {
> -		retval = platform_device_add_resources(pdev, res, num);
> -		if (retval)
> -			goto error;
> -	}
> +	pdev->dev.of_node = ofdev->dev.of_node;
> +	pdev->dev.of_node_reused = true;
>  
>  	retval = platform_device_add(pdev);
>  	if (retval)

Re: [PATCH RFC] drivers/usb/ehci-fsl: Fix interrupt setup in host mode.

[Rob Herring]

On Tue, Jun 21, 2022 at 4:09 PM Darren Stevens <darren@stevens-zone.net> wrote:
>
> In patch a1a2b7125e1079 (Drop static setup of IRQ resource from DT
> core) we stopped platform_get_resource() from returning the IRQ, as all
> drivers were supposed to have switched to platform_get_irq()
> Unfortunately the Freescale EHCI driver in host mode got missed. Fix
> it. Also fix allocation of resources to work with current kernel.
>
> Fixes:a1a2b7125e1079 (Drop static setup of IRQ resource from DT core)
> Reported-by Christian Zigotzky <chzigotzky@xenosoft.de>
> Signed-off-by Darren Stevens <darren@stevens-zone.net>
> ---
> Tested on AmigaOne X5000/20 and X5000/40 not sure if this is entirely
> correct fix though. Contains code by Rob Herring (in fsl-mph-dr-of.c)
>
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 385be30..d0bf7fb 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -23,6 +23,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/fsl_devices.h>
>  #include <linux/of_platform.h>
> +#include <linux/of_address.h>
>  #include <linux/io.h>
>
>  #include "ehci.h"
> @@ -46,9 +47,10 @@ static struct hc_driver __read_mostly
> fsl_ehci_hc_driver; */
>  static int fsl_ehci_drv_probe(struct platform_device *pdev)
>  {
> +       struct device_node *dn = pdev->dev.of_node;
>         struct fsl_usb2_platform_data *pdata;
>         struct usb_hcd *hcd;
> -       struct resource *res;
> +       struct resource res;
>         int irq;
>         int retval;
>         u32 tmp;
> @@ -76,14 +78,10 @@ static int fsl_ehci_drv_probe(struct
> platform_device *pdev) return -ENODEV;
>         }
>
> -       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

Ah, this was the part I was missing...

> -       if (!res) {
> -               dev_err(&pdev->dev,
> -                       "Found HC with no IRQ. Check %s setup!\n",
> -                       dev_name(&pdev->dev));
> -               return -ENODEV;
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0) {
> +               return irq;
>         }
> -       irq = res->start;
>
>         hcd = __usb_create_hcd(&fsl_ehci_hc_driver, pdev->dev.parent,
>                                &pdev->dev, dev_name(&pdev->dev), NULL);
> @@ -92,15 +90,21 @@ static int fsl_ehci_drv_probe(struct
> platform_device *pdev) goto err1;
>         }
>
> -       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -       hcd->regs = devm_ioremap_resource(&pdev->dev, res);
> +       platform_set_drvdata(pdev, hcd);
> +       pdev->dev.platform_data = pdata;
> +
> +       tmp = of_address_to_resource(dn, 0, &res);

There's no need to change this. platform_get_resource() still works
for IORESOURCE_MEM.

Plus, drivers shouldn't use of_address_to_resource().

> +       if (tmp)
> +               return tmp;
> +
> +       hcd->regs = devm_ioremap_resource(&pdev->dev, &res);
>         if (IS_ERR(hcd->regs)) {
>                 retval = PTR_ERR(hcd->regs);
>                 goto err2;
>         }
>
> -       hcd->rsrc_start = res->start;
> -       hcd->rsrc_len = resource_size(res);
> +       hcd->rsrc_start = res.start;
> +       hcd->rsrc_len = resource_size(&res);
>
>         pdata->regs = hcd->regs;
>
> diff --git a/drivers/usb/host/fsl-mph-dr-of.c
> b/drivers/usb/host/fsl-mph-dr-of.c index 44a7e58..766e4ab 100644
> --- a/drivers/usb/host/fsl-mph-dr-of.c
> +++ b/drivers/usb/host/fsl-mph-dr-of.c
> @@ -80,8 +80,6 @@ static struct platform_device
> *fsl_usb2_device_register( const char *name, int id)
>  {
>         struct platform_device *pdev;
> -       const struct resource *res = ofdev->resource;
> -       unsigned int num = ofdev->num_resources;
>         int retval;
>
>         pdev = platform_device_alloc(name, id);
> @@ -106,11 +104,8 @@ static struct platform_device
> *fsl_usb2_device_register( if (retval)
>                 goto error;
>
> -       if (num) {
> -               retval = platform_device_add_resources(pdev, res, num);
> -               if (retval)
> -                       goto error;
> -       }
> +       pdev->dev.of_node = ofdev->dev.of_node;
> +       pdev->dev.of_node_reused = true;
>
>         retval = platform_device_add(pdev);
>         if (retval)