| Message ID | 1456312517-24211-2-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Reinder,
[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.5-rc5 next-20160223]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Hans-de-Goede/ehci-platform-Add-support-for-controllers-with-multiple-reset-lines/20160224-191812
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next
config: x86_64-randconfig-x004-201608 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
drivers/usb/host/ehci-platform.c: In function 'ehci_platform_probe':
>> drivers/usb/host/ehci-platform.c:239:5: error: implicit declaration of function 'devm_reset_control_get_shared_by_index' [-Werror=implicit-function-declaration]
devm_reset_control_get_shared_by_index(
^
>> drivers/usb/host/ehci-platform.c:238:22: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
priv->resets[rst] =
^
cc1: some warnings being treated as errors
vim +/devm_reset_control_get_shared_by_index +239 drivers/usb/host/ehci-platform.c
232 priv->clks[clk] = NULL;
233 break;
234 }
235 }
236
237 for (rst = 0; rst < EHCI_MAX_RESETS; rst++) {
> 238 priv->resets[rst] =
> 239 devm_reset_control_get_shared_by_index(
240 &dev->dev, rst);
241 if (IS_ERR(priv->resets[rst])) {
242 err = PTR_ERR(priv->resets[rst]);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi, On 24/02/16 13:15, Hans de Goede wrote: > From: Reinder de Haan <patchesrdh@mveas.com> > > At least the EHCI/OHCI found on the Allwinnner H3 SoC needs multiple > reset lines, the controller will not initialize while the reset for > its companion is still asserted, which means we need to de-assert > 2 resets for the controller to work. > > Signed-off-by: Reinder de Haan <patchesrdh@mveas.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > -Use the new reset_control_[de]assert_shared reset-controller functions > Changes in v3: > -Adjust for changes to shared-reset reset-controller functions > --- > Documentation/devicetree/bindings/usb/usb-ehci.txt | 2 +- > drivers/usb/host/ehci-platform.c | 41 ++++++++++++---------- > 2 files changed, 24 insertions(+), 19 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt > index a12d601..0701812 100644 > --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt > +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt > @@ -18,7 +18,7 @@ Optional properties: > - clocks : a list of phandle + clock specifier pairs > - phys : phandle + phy specifier pair > - phy-names : "usb" > - - resets : phandle + reset specifier pair > + - resets : a list of phandle + reset specifier pairs > > Example (Sequoia 440EPx): > ehci@e0000300 { > diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c > index bd7082f2..3f195ba 100644 > --- a/drivers/usb/host/ehci-platform.c > +++ b/drivers/usb/host/ehci-platform.c > @@ -39,11 +39,12 @@ > > #define DRIVER_DESC "EHCI generic platform driver" > #define EHCI_MAX_CLKS 3 > +#define EHCI_MAX_RESETS 2 > #define hcd_to_ehci_priv(h) ((struct ehci_platform_priv *)hcd_to_ehci(h)->priv) > > struct ehci_platform_priv { > struct clk *clks[EHCI_MAX_CLKS]; > - struct reset_control *rst; > + struct reset_control *resets[EHCI_MAX_RESETS]; > struct phy **phys; > int num_phys; > bool reset_on_resume; > @@ -149,7 +150,7 @@ static int ehci_platform_probe(struct platform_device *dev) > struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev); > struct ehci_platform_priv *priv; > struct ehci_hcd *ehci; > - int err, irq, phy_num, clk = 0; > + int err, irq, phy_num, clk = 0, rst = 0; > > if (usb_disabled()) > return -ENODEV; > @@ -232,18 +233,22 @@ static int ehci_platform_probe(struct platform_device *dev) > break; > } > } > - } > > - priv->rst = devm_reset_control_get_optional(&dev->dev, NULL); > - if (IS_ERR(priv->rst)) { > - err = PTR_ERR(priv->rst); > - if (err == -EPROBE_DEFER) > - goto err_put_clks; > - priv->rst = NULL; > - } else { > - err = reset_control_deassert(priv->rst); > - if (err) > - goto err_put_clks; > + for (rst = 0; rst < EHCI_MAX_RESETS; rst++) { > + priv->resets[rst] = > + devm_reset_control_get_shared_by_index( > + &dev->dev, rst); > + if (IS_ERR(priv->resets[rst])) { > + err = PTR_ERR(priv->resets[rst]); > + if (err == -EPROBE_DEFER) > + goto err_reset; > + priv->resets[rst] = NULL; > + break; > + } > + err = reset_control_deassert(priv->resets[rst]); > + if (err) > + goto err_reset; > + } Why is this code within "if (dev->of.node)" ? Previous code looked like it worked for non-DT boots as well. > } > > if (pdata->big_endian_desc) > @@ -300,8 +305,8 @@ err_power: > if (pdata->power_off) > pdata->power_off(dev); > err_reset: > - if (priv->rst) > - reset_control_assert(priv->rst); > + while (--rst >= 0) > + reset_control_assert(priv->resets[rst]); > err_put_clks: > while (--clk >= 0) > clk_put(priv->clks[clk]); > @@ -319,15 +324,15 @@ static int ehci_platform_remove(struct platform_device *dev) > struct usb_hcd *hcd = platform_get_drvdata(dev); > struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev); > struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd); > - int clk; > + int clk, rst; > > usb_remove_hcd(hcd); > > if (pdata->power_off) > pdata->power_off(dev); > > - if (priv->rst) > - reset_control_assert(priv->rst); > + for (rst = 0; rst < EHCI_MAX_RESETS && priv->resets[rst]; rst++) > + reset_control_assert(priv->resets[rst]); > > for (clk = 0; clk < EHCI_MAX_CLKS && priv->clks[clk]; clk++) > clk_put(priv->clks[clk]); > cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 24-02-16 12:37, Roger Quadros wrote: > Hi, > > On 24/02/16 13:15, Hans de Goede wrote: >> From: Reinder de Haan <patchesrdh@mveas.com> >> >> At least the EHCI/OHCI found on the Allwinnner H3 SoC needs multiple >> reset lines, the controller will not initialize while the reset for >> its companion is still asserted, which means we need to de-assert >> 2 resets for the controller to work. >> >> Signed-off-by: Reinder de Haan <patchesrdh@mveas.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2: >> -Use the new reset_control_[de]assert_shared reset-controller functions >> Changes in v3: >> -Adjust for changes to shared-reset reset-controller functions >> --- >> Documentation/devicetree/bindings/usb/usb-ehci.txt | 2 +- >> drivers/usb/host/ehci-platform.c | 41 ++++++++++++---------- >> 2 files changed, 24 insertions(+), 19 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt >> index a12d601..0701812 100644 >> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt >> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt >> @@ -18,7 +18,7 @@ Optional properties: >> - clocks : a list of phandle + clock specifier pairs >> - phys : phandle + phy specifier pair >> - phy-names : "usb" >> - - resets : phandle + reset specifier pair >> + - resets : a list of phandle + reset specifier pairs >> >> Example (Sequoia 440EPx): >> ehci@e0000300 { >> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c >> index bd7082f2..3f195ba 100644 >> --- a/drivers/usb/host/ehci-platform.c >> +++ b/drivers/usb/host/ehci-platform.c >> @@ -39,11 +39,12 @@ >> >> #define DRIVER_DESC "EHCI generic platform driver" >> #define EHCI_MAX_CLKS 3 >> +#define EHCI_MAX_RESETS 2 >> #define hcd_to_ehci_priv(h) ((struct ehci_platform_priv *)hcd_to_ehci(h)->priv) >> >> struct ehci_platform_priv { >> struct clk *clks[EHCI_MAX_CLKS]; >> - struct reset_control *rst; >> + struct reset_control *resets[EHCI_MAX_RESETS]; >> struct phy **phys; >> int num_phys; >> bool reset_on_resume; >> @@ -149,7 +150,7 @@ static int ehci_platform_probe(struct platform_device *dev) >> struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev); >> struct ehci_platform_priv *priv; >> struct ehci_hcd *ehci; >> - int err, irq, phy_num, clk = 0; >> + int err, irq, phy_num, clk = 0, rst = 0; >> >> if (usb_disabled()) >> return -ENODEV; >> @@ -232,18 +233,22 @@ static int ehci_platform_probe(struct platform_device *dev) >> break; >> } >> } >> - } >> >> - priv->rst = devm_reset_control_get_optional(&dev->dev, NULL); >> - if (IS_ERR(priv->rst)) { >> - err = PTR_ERR(priv->rst); >> - if (err == -EPROBE_DEFER) >> - goto err_put_clks; >> - priv->rst = NULL; >> - } else { >> - err = reset_control_deassert(priv->rst); >> - if (err) >> - goto err_put_clks; >> + for (rst = 0; rst < EHCI_MAX_RESETS; rst++) { >> + priv->resets[rst] = >> + devm_reset_control_get_shared_by_index( >> + &dev->dev, rst); >> + if (IS_ERR(priv->resets[rst])) { >> + err = PTR_ERR(priv->resets[rst]); >> + if (err == -EPROBE_DEFER) >> + goto err_reset; >> + priv->resets[rst] = NULL; >> + break; >> + } >> + err = reset_control_deassert(priv->resets[rst]); >> + if (err) >> + goto err_reset; >> + } > > Why is this code within "if (dev->of.node)" ? > Previous code looked like it worked for non-DT boots as well. Because devm_reset_control_get_shared_by_index() only works for DT platforms (it is safe to call on non DT platforms, it will just always return -EINVAL there) and the old reset paths were ever only used / only ever worked with DT too. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 24 Feb 2016, Roger Quadros wrote: > Hi, > > On 24/02/16 13:15, Hans de Goede wrote: > > From: Reinder de Haan <patchesrdh@mveas.com> > > > > At least the EHCI/OHCI found on the Allwinnner H3 SoC needs multiple > > reset lines, the controller will not initialize while the reset for > > its companion is still asserted, which means we need to de-assert > > 2 resets for the controller to work. > > > > Signed-off-by: Reinder de Haan <patchesrdh@mveas.com> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > - } else { > > - err = reset_control_deassert(priv->rst); > > - if (err) > > - goto err_put_clks; > > + for (rst = 0; rst < EHCI_MAX_RESETS; rst++) { > > + priv->resets[rst] = > > + devm_reset_control_get_shared_by_index( > > + &dev->dev, rst); Ugly continuation line. The convention in these files is to indent continuation lines by two tab stops beyond the original source line. Same comment applies to the 2/2 patch. Once that is changed, you can add: Acked-by: Alan Stern <stern@rowland.harvard.edu> to both. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt index a12d601..0701812 100644 --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt @@ -18,7 +18,7 @@ Optional properties: - clocks : a list of phandle + clock specifier pairs - phys : phandle + phy specifier pair - phy-names : "usb" - - resets : phandle + reset specifier pair + - resets : a list of phandle + reset specifier pairs Example (Sequoia 440EPx): ehci@e0000300 { diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c index bd7082f2..3f195ba 100644 --- a/drivers/usb/host/ehci-platform.c +++ b/drivers/usb/host/ehci-platform.c @@ -39,11 +39,12 @@ #define DRIVER_DESC "EHCI generic platform driver" #define EHCI_MAX_CLKS 3 +#define EHCI_MAX_RESETS 2 #define hcd_to_ehci_priv(h) ((struct ehci_platform_priv *)hcd_to_ehci(h)->priv) struct ehci_platform_priv { struct clk *clks[EHCI_MAX_CLKS]; - struct reset_control *rst; + struct reset_control *resets[EHCI_MAX_RESETS]; struct phy **phys; int num_phys; bool reset_on_resume; @@ -149,7 +150,7 @@ static int ehci_platform_probe(struct platform_device *dev) struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev); struct ehci_platform_priv *priv; struct ehci_hcd *ehci; - int err, irq, phy_num, clk = 0; + int err, irq, phy_num, clk = 0, rst = 0; if (usb_disabled()) return -ENODEV; @@ -232,18 +233,22 @@ static int ehci_platform_probe(struct platform_device *dev) break; } } - } - priv->rst = devm_reset_control_get_optional(&dev->dev, NULL); - if (IS_ERR(priv->rst)) { - err = PTR_ERR(priv->rst); - if (err == -EPROBE_DEFER) - goto err_put_clks; - priv->rst = NULL; - } else { - err = reset_control_deassert(priv->rst); - if (err) - goto err_put_clks; + for (rst = 0; rst < EHCI_MAX_RESETS; rst++) { + priv->resets[rst] = + devm_reset_control_get_shared_by_index( + &dev->dev, rst); + if (IS_ERR(priv->resets[rst])) { + err = PTR_ERR(priv->resets[rst]); + if (err == -EPROBE_DEFER) + goto err_reset; + priv->resets[rst] = NULL; + break; + } + err = reset_control_deassert(priv->resets[rst]); + if (err) + goto err_reset; + } } if (pdata->big_endian_desc) @@ -300,8 +305,8 @@ err_power: if (pdata->power_off) pdata->power_off(dev); err_reset: - if (priv->rst) - reset_control_assert(priv->rst); + while (--rst >= 0) + reset_control_assert(priv->resets[rst]); err_put_clks: while (--clk >= 0) clk_put(priv->clks[clk]); @@ -319,15 +324,15 @@ static int ehci_platform_remove(struct platform_device *dev) struct usb_hcd *hcd = platform_get_drvdata(dev); struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev); struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd); - int clk; + int clk, rst; usb_remove_hcd(hcd); if (pdata->power_off) pdata->power_off(dev); - if (priv->rst) - reset_control_assert(priv->rst); + for (rst = 0; rst < EHCI_MAX_RESETS && priv->resets[rst]; rst++) + reset_control_assert(priv->resets[rst]); for (clk = 0; clk < EHCI_MAX_CLKS && priv->clks[clk]; clk++) clk_put(priv->clks[clk]);