| Message ID | 20220625214151.547b3570@Cyrus.lan (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [v2,RFC] drivers/usb/host/ehci-fsl: Fix interrupt setup in host mode. | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
Hello! On 6/25/22 11:41 PM, Darren Stevens wrote: > In patch a1a2b7125e10 (Drop static setup of IRQ resource from DT > core) we stopped platform_get_resource() from returning the IRQ, as all In commit a1a2b7125e10 ("Drop static setup of IRQ resource from DT core") > 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. The basic rule (especially for the fixes) is "do one thing per patch". > Fixes: a1a2b7125e10 (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> > --- > v2 - Fixed coding style, removed a couple of unneeded initializations, > cc'd Layerscape maintainers. > > 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..8bd258a 100644 > --- a/drivers/usb/host/ehci-fsl.c > +++ b/drivers/usb/host/ehci-fsl.c [...] > @@ -92,15 +89,18 @@ 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); > + tmp = of_address_to_resource(dn, 0, &res); Hm, why? What does this fix? [...] MBR, Sergey
Hello Sergei On 26/06/2022, Sergei Shtylyov wrote: > Hello! > > On 6/25/22 11:41 PM, Darren Stevens wrote: > >> In patch a1a2b7125e10 (Drop static setup of IRQ resource from DT >> core) we stopped platform_get_resource() from returning the IRQ, as all > > In commit a1a2b7125e10 ("Drop static setup of IRQ resource from DT core") > >> 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. > > The basic rule (especially for the fixes) is "do one thing per patch". I thought I'd done that, this is the minimum amount of changes that fix what changed in the specified commit. > [...] >> @@ -92,15 +89,18 @@ 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); >> + tmp = of_address_to_resource(dn, 0, &res); > > Hm, why? What does this fix? With baseline the mouse and keyboard on our machines don't work - dmesg reports no interrupt. Fixing the interrupt detection throws a 'invalid resoure' error instead (No idea why), which these lines fix. Both problems disappear if we revert the 'fixes' patch. Hmmm, perhaps title shoud be 'fix resource detection in host mode'? Regards Darren
On Sun, Jun 26, 2022 at 2:03 PM Darren Stevens <darren@stevens-zone.net> wrote: > > Hello Sergei > > On 26/06/2022, Sergei Shtylyov wrote: > > Hello! > > > > On 6/25/22 11:41 PM, Darren Stevens wrote: > > > >> In patch a1a2b7125e10 (Drop static setup of IRQ resource from DT > >> core) we stopped platform_get_resource() from returning the IRQ, as all > > > > In commit a1a2b7125e10 ("Drop static setup of IRQ resource from DT core") > > > >> 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. > > > > The basic rule (especially for the fixes) is "do one thing per patch". > > I thought I'd done that, this is the minimum amount of changes that fix what changed in the specified commit. > > > [...] > >> @@ -92,15 +89,18 @@ 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); > >> + tmp = of_address_to_resource(dn, 0, &res); > > > > Hm, why? What does this fix? > > With baseline the mouse and keyboard on our machines don't work - dmesg reports no interrupt. Fixing the interrupt detection throws a 'invalid resoure' error instead (No idea why), which these lines fix. Both problems disappear if we revert the 'fixes' patch. > I see the problem. You need to keep the platform_device_add_resources() call in fsl-mph-dr-of.c so that the memory resource is copied from the parent to the child device. Rob
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c index 385be30..8bd258a 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,9 @@ 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 = res->start; + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; hcd = __usb_create_hcd(&fsl_ehci_hc_driver, pdev->dev.parent, &pdev->dev, dev_name(&pdev->dev), NULL); @@ -92,15 +89,18 @@ 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); + 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)
In patch a1a2b7125e10 (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: a1a2b7125e10 (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> --- v2 - Fixed coding style, removed a couple of unneeded initializations, cc'd Layerscape maintainers. 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)