* [v4] usb: uhci: Add clk support to uhci-platform
@ 2018-01-14 22:59 Benjamin Herrenschmidt
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2018-01-14 22:59 UTC (permalink / raw)
To: Alan Stern; +Cc: linux-usb, Greg KH, Joel Stanley
The Aspeed SoCs use uhci-platform. With the new dynamic clock
control framework, the corresponding IP block clock must be
properly enabled.
This is a simplified variant of what ehci-platform does, it
looks for *one* clock attached to the device, and if it's
there, enables it.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
---
v4. Remove unnecessary NULL checks
v3. Fix tab vs. spaces and probe error path
v2. Don't forget to git add latest changes :-) This adds the
part where we turn the clock off when removing the driver.
---
drivers/usb/host/uhci-hcd.h | 3 +++
drivers/usb/host/uhci-platform.c | 20 ++++++++++++++++++--
2 files changed, 21 insertions(+), 2 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
index f1cc47292a59..7f9f33c8c232 100644
--- a/drivers/usb/host/uhci-hcd.h
+++ b/drivers/usb/host/uhci-hcd.h
@@ -4,6 +4,7 @@
#include <linux/list.h>
#include <linux/usb.h>
+#include <linux/clk.h>
#define usb_packetid(pipe) (usb_pipein(pipe) ? USB_PID_IN : USB_PID_OUT)
#define PIPE_DEVEP_MASK 0x0007ff00
@@ -447,6 +448,8 @@ struct uhci_hcd {
int total_load; /* Sum of array values */
short load[MAX_PHASE]; /* Periodic allocations */
+ struct clk *clk; /* (optional) clock source */
+
/* Reset host controller */
void (*reset_hc) (struct uhci_hcd *uhci);
int (*check_and_reset_hc) (struct uhci_hcd *uhci);
diff --git a/drivers/usb/host/uhci-platform.c b/drivers/usb/host/uhci-platform.c
index 6cb16d4b2257..55c2d99bf7ec 100644
--- a/drivers/usb/host/uhci-platform.c
+++ b/drivers/usb/host/uhci-platform.c
@@ -89,6 +89,8 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
if (!hcd)
return -ENOMEM;
+ uhci = hcd_to_uhci(hcd);
+
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
hcd->regs = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(hcd->regs)) {
@@ -98,8 +100,6 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
hcd->rsrc_start = res->start;
hcd->rsrc_len = resource_size(res);
- uhci = hcd_to_uhci(hcd);
-
uhci->regs = hcd->regs;
/* Grab some things from the device-tree */
@@ -119,6 +119,19 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
"Enabled Aspeed implementation workarounds\n");
}
}
+
+ /* Get and enable clock if any specified */
+ uhci->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(uhci->clk)) {
+ ret = PTR_ERR(uhci->clk);
+ goto err_rmr;
+ }
+ ret = clk_prepare_enable(uhci->clk);
+ if (ret) {
+ dev_err(&pdev->dev, "Error couldn't enable clock (%d)\n", ret);
+ goto err_rmr;
+ }
+
ret = usb_add_hcd(hcd, pdev->resource[1].start, IRQF_SHARED);
if (ret)
goto err_rmr;
@@ -127,6 +140,7 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
return 0;
err_rmr:
+ clk_disable_unprepare(uhci->clk);
usb_put_hcd(hcd);
return ret;
@@ -135,7 +149,9 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
static int uhci_hcd_platform_remove(struct platform_device *pdev)
{
struct usb_hcd *hcd = platform_get_drvdata(pdev);
+ struct uhci_hcd *uhci = hcd_to_uhci(hcd);
+ clk_disable_unprepare(uhci->clk);
usb_remove_hcd(hcd);
usb_put_hcd(hcd);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [v4] usb: uhci: Add clk support to uhci-platform
@ 2018-01-15 8:41 Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2018-01-15 8:41 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Alan Stern; +Cc: linux-usb, Greg KH, Joel Stanley
Hello!
On 1/15/2018 1:59 AM, Benjamin Herrenschmidt wrote:
> The Aspeed SoCs use uhci-platform. With the new dynamic clock
> control framework, the corresponding IP block clock must be
> properly enabled.
>
> This is a simplified variant of what ehci-platform does, it
> looks for *one* clock attached to the device, and if it's
> there, enables it.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> ---
>
> v4. Remove unnecessary NULL checks
>
> v3. Fix tab vs. spaces and probe error path
>
> v2. Don't forget to git add latest changes :-) This adds the
> part where we turn the clock off when removing the driver.
> ---
> drivers/usb/host/uhci-hcd.h | 3 +++
> drivers/usb/host/uhci-platform.c | 20 ++++++++++++++++++--
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> index f1cc47292a59..7f9f33c8c232 100644
> --- a/drivers/usb/host/uhci-hcd.h
> +++ b/drivers/usb/host/uhci-hcd.h
> @@ -4,6 +4,7 @@
>
> #include <linux/list.h>
> #include <linux/usb.h>
> +#include <linux/clk.h>
>
> #define usb_packetid(pipe) (usb_pipein(pipe) ? USB_PID_IN : USB_PID_OUT)
> #define PIPE_DEVEP_MASK 0x0007ff00
> @@ -447,6 +448,8 @@ struct uhci_hcd {
> int total_load; /* Sum of array values */
> short load[MAX_PHASE]; /* Periodic allocations */
>
> + struct clk *clk; /* (optional) clock source */
> +
> /* Reset host controller */
> void (*reset_hc) (struct uhci_hcd *uhci);
> int (*check_and_reset_hc) (struct uhci_hcd *uhci);
> diff --git a/drivers/usb/host/uhci-platform.c b/drivers/usb/host/uhci-platform.c
> index 6cb16d4b2257..55c2d99bf7ec 100644
> --- a/drivers/usb/host/uhci-platform.c
> +++ b/drivers/usb/host/uhci-platform.c
> @@ -89,6 +89,8 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> if (!hcd)
> return -ENOMEM;
>
> + uhci = hcd_to_uhci(hcd);
> +
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> hcd->regs = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(hcd->regs)) {
> @@ -98,8 +100,6 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> hcd->rsrc_start = res->start;
> hcd->rsrc_len = resource_size(res);
>
> - uhci = hcd_to_uhci(hcd);
> -
> uhci->regs = hcd->regs;
>
> /* Grab some things from the device-tree */
> @@ -119,6 +119,19 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> "Enabled Aspeed implementation workarounds\n");
> }
> }
> +
> + /* Get and enable clock if any specified */
> + uhci->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(uhci->clk)) {
> + ret = PTR_ERR(uhci->clk);
> + goto err_rmr;
> + }
> + ret = clk_prepare_enable(uhci->clk);
> + if (ret) {
> + dev_err(&pdev->dev, "Error couldn't enable clock (%d)\n", ret);
> + goto err_rmr;
> + }
> +
> ret = usb_add_hcd(hcd, pdev->resource[1].start, IRQF_SHARED);
> if (ret)
> goto err_rmr;
> @@ -127,6 +140,7 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> return 0;
>
> err_rmr:
> + clk_disable_unprepare(uhci->clk);
Really? You haven't enabled the clock before either *goto* above, why
disable it here?
> usb_put_hcd(hcd);
>
> return ret;
[...]
MBR, Sergei
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v4] usb: uhci: Add clk support to uhci-platform
@ 2018-01-15 21:53 Benjamin Herrenschmidt
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2018-01-15 21:53 UTC (permalink / raw)
To: Sergei Shtylyov, Alan Stern; +Cc: linux-usb, Greg KH, Joel Stanley
On Mon, 2018-01-15 at 11:41 +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 1/15/2018 1:59 AM, Benjamin Herrenschmidt wrote:
>
> > The Aspeed SoCs use uhci-platform. With the new dynamic clock
> > control framework, the corresponding IP block clock must be
> > properly enabled.
> >
> > This is a simplified variant of what ehci-platform does, it
> > looks for *one* clock attached to the device, and if it's
> > there, enables it.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > ---
> >
> > v4. Remove unnecessary NULL checks
> >
> > v3. Fix tab vs. spaces and probe error path
> >
> > v2. Don't forget to git add latest changes :-) This adds the
> > part where we turn the clock off when removing the driver.
> > ---
> > drivers/usb/host/uhci-hcd.h | 3 +++
> > drivers/usb/host/uhci-platform.c | 20 ++++++++++++++++++--
> > 2 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> > index f1cc47292a59..7f9f33c8c232 100644
> > --- a/drivers/usb/host/uhci-hcd.h
> > +++ b/drivers/usb/host/uhci-hcd.h
> > @@ -4,6 +4,7 @@
> >
> > #include <linux/list.h>
> > #include <linux/usb.h>
> > +#include <linux/clk.h>
> >
> > #define usb_packetid(pipe) (usb_pipein(pipe) ? USB_PID_IN : USB_PID_OUT)
> > #define PIPE_DEVEP_MASK 0x0007ff00
> > @@ -447,6 +448,8 @@ struct uhci_hcd {
> > int total_load; /* Sum of array values */
> > short load[MAX_PHASE]; /* Periodic allocations */
> >
> > + struct clk *clk; /* (optional) clock source */
> > +
> > /* Reset host controller */
> > void (*reset_hc) (struct uhci_hcd *uhci);
> > int (*check_and_reset_hc) (struct uhci_hcd *uhci);
> > diff --git a/drivers/usb/host/uhci-platform.c b/drivers/usb/host/uhci-platform.c
> > index 6cb16d4b2257..55c2d99bf7ec 100644
> > --- a/drivers/usb/host/uhci-platform.c
> > +++ b/drivers/usb/host/uhci-platform.c
> > @@ -89,6 +89,8 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > if (!hcd)
> > return -ENOMEM;
> >
> > + uhci = hcd_to_uhci(hcd);
> > +
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > hcd->regs = devm_ioremap_resource(&pdev->dev, res);
> > if (IS_ERR(hcd->regs)) {
> > @@ -98,8 +100,6 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > hcd->rsrc_start = res->start;
> > hcd->rsrc_len = resource_size(res);
> >
> > - uhci = hcd_to_uhci(hcd);
> > -
> > uhci->regs = hcd->regs;
> >
> > /* Grab some things from the device-tree */
> > @@ -119,6 +119,19 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > "Enabled Aspeed implementation workarounds\n");
> > }
> > }
> > +
> > + /* Get and enable clock if any specified */
> > + uhci->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(uhci->clk)) {
> > + ret = PTR_ERR(uhci->clk);
> > + goto err_rmr;
> > + }
> > + ret = clk_prepare_enable(uhci->clk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Error couldn't enable clock (%d)\n", ret);
> > + goto err_rmr;
> > + }
> > +
> > ret = usb_add_hcd(hcd, pdev->resource[1].start, IRQF_SHARED);
> > if (ret)
> > goto err_rmr;
> > @@ -127,6 +140,7 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > return 0;
> >
> > err_rmr:
> > + clk_disable_unprepare(uhci->clk);
>
> Really? You haven't enabled the clock before either *goto* above, why
> disable it here?
If clk is NULL or an error, disable will do nothing. The only case
would be if enable at failed but then disable will do nothing in that
case.
The above change was done at the suggestion of Alan. If you really
prefer I could add a clk_put and clearing of "clk" in the error path of
clk_prepare_enable, but that's about it.
BTW. DO we really need to bike shed such a simple patch to death like
that ?
>
> > usb_put_hcd(hcd);
> >
> > return ret;
>
> [...]
>
> MBR, Sergei
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v4] usb: uhci: Add clk support to uhci-platform
@ 2018-01-15 22:17 Joel Stanley
0 siblings, 0 replies; 6+ messages in thread
From: Joel Stanley @ 2018-01-15 22:17 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Benjamin Herrenschmidt, Alan Stern, linux-usb, Greg KH,
Joel Stanley
On Mon, Jan 15, 2018 at 2:41 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello!
>
>
> On 1/15/2018 1:59 AM, Benjamin Herrenschmidt wrote:
>
>> The Aspeed SoCs use uhci-platform. With the new dynamic clock
>> control framework, the corresponding IP block clock must be
>> properly enabled.
>>
>> This is a simplified variant of what ehci-platform does, it
>> looks for *one* clock attached to the device, and if it's
>> there, enables it.
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Acked-by: Alan Stern <stern@rowland.harvard.edu>
>> +
>> + /* Get and enable clock if any specified */
>> + uhci->clk = devm_clk_get(&pdev->dev, NULL);
>> + if (IS_ERR(uhci->clk)) {
>> + ret = PTR_ERR(uhci->clk);
>> + goto err_rmr;
>> + }
>> + ret = clk_prepare_enable(uhci->clk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Error couldn't enable clock (%d)\n",
>> ret);
>> + goto err_rmr;
>> + }
>> +
>> ret = usb_add_hcd(hcd, pdev->resource[1].start, IRQF_SHARED);
>> if (ret)
>> goto err_rmr;
>> @@ -127,6 +140,7 @@ static int uhci_hcd_platform_probe(struct
>> platform_device *pdev)
>> return 0;
>> err_rmr:
>> + clk_disable_unprepare(uhci->clk);
>
>
> Really? You haven't enabled the clock before either *goto* above, why
> disable it here?
This is fine; clk_disable_unprepare checks to see if struct clk * is
NULL, and if so, silently continues.
Reviewed-by: Joel Stanley <joel@jms.id.au>
I tested the patch on the ASPEED ast2500 platform.
Cheers,
Joel
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v4] usb: uhci: Add clk support to uhci-platform
@ 2018-01-16 8:36 Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2018-01-16 8:36 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Alan Stern; +Cc: linux-usb, Greg KH, Joel Stanley
Hello!
On 1/16/2018 12:53 AM, Benjamin Herrenschmidt wrote:
>>> The Aspeed SoCs use uhci-platform. With the new dynamic clock
>>> control framework, the corresponding IP block clock must be
>>> properly enabled.
>>>
>>> This is a simplified variant of what ehci-platform does, it
>>> looks for *one* clock attached to the device, and if it's
>>> there, enables it.
>>>
>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Acked-by: Alan Stern <stern@rowland.harvard.edu>
>>> ---
>>>
>>> v4. Remove unnecessary NULL checks
>>>
>>> v3. Fix tab vs. spaces and probe error path
>>>
>>> v2. Don't forget to git add latest changes :-) This adds the
>>> part where we turn the clock off when removing the driver.
>>> ---
>>> drivers/usb/host/uhci-hcd.h | 3 +++
>>> drivers/usb/host/uhci-platform.c | 20 ++++++++++++++++++--
>>> 2 files changed, 21 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
>>> index f1cc47292a59..7f9f33c8c232 100644
>>> --- a/drivers/usb/host/uhci-hcd.h
>>> +++ b/drivers/usb/host/uhci-hcd.h
>>> @@ -4,6 +4,7 @@
>>>
>>> #include <linux/list.h>
>>> #include <linux/usb.h>
>>> +#include <linux/clk.h>
>>>
>>> #define usb_packetid(pipe) (usb_pipein(pipe) ? USB_PID_IN : USB_PID_OUT)
>>> #define PIPE_DEVEP_MASK 0x0007ff00
>>> @@ -447,6 +448,8 @@ struct uhci_hcd {
>>> int total_load; /* Sum of array values */
>>> short load[MAX_PHASE]; /* Periodic allocations */
>>>
>>> + struct clk *clk; /* (optional) clock source */
>>> +
>>> /* Reset host controller */
>>> void (*reset_hc) (struct uhci_hcd *uhci);
>>> int (*check_and_reset_hc) (struct uhci_hcd *uhci);
>>> diff --git a/drivers/usb/host/uhci-platform.c b/drivers/usb/host/uhci-platform.c
>>> index 6cb16d4b2257..55c2d99bf7ec 100644
>>> --- a/drivers/usb/host/uhci-platform.c
>>> +++ b/drivers/usb/host/uhci-platform.c
>>> @@ -89,6 +89,8 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
>>> if (!hcd)
>>> return -ENOMEM;
>>>
>>> + uhci = hcd_to_uhci(hcd);
>>> +
>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> hcd->regs = devm_ioremap_resource(&pdev->dev, res);
>>> if (IS_ERR(hcd->regs)) {
>>> @@ -98,8 +100,6 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
>>> hcd->rsrc_start = res->start;
>>> hcd->rsrc_len = resource_size(res);
>>>
>>> - uhci = hcd_to_uhci(hcd);
>>> -
>>> uhci->regs = hcd->regs;
>>>
>>> /* Grab some things from the device-tree */
>>> @@ -119,6 +119,19 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
>>> "Enabled Aspeed implementation workarounds\n");
>>> }
>>> }
>>> +
>>> + /* Get and enable clock if any specified */
>>> + uhci->clk = devm_clk_get(&pdev->dev, NULL);
>>> + if (IS_ERR(uhci->clk)) {
>>> + ret = PTR_ERR(uhci->clk);
>>> + goto err_rmr;
>>> + }
>>> + ret = clk_prepare_enable(uhci->clk);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Error couldn't enable clock (%d)\n", ret);
>>> + goto err_rmr;
>>> + }
>>> +
>>> ret = usb_add_hcd(hcd, pdev->resource[1].start, IRQF_SHARED);
>>> if (ret)
>>> goto err_rmr;
>>> @@ -127,6 +140,7 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
>>> return 0;
>>>
>>> err_rmr:
>>> + clk_disable_unprepare(uhci->clk);
>>
>> Really? You haven't enabled the clock before either *goto* above, why
>> disable it here?
>
> If clk is NULL or an error, disable will do nothing. The only case
The rule of thumb was that you don't pass the error pointers to APIs but
indeed I'm now seeing the NULL check extended to the error pointers too...
> would be if enable at failed but then disable will do nothing in that
> case.
Are you sure there'll be no WARN_ON() triggered by clk_disable() with the
enable count == 0?
> The above change was done at the suggestion of Alan. If you really
I'd probably missed his mail.
> prefer I could add a clk_put and clearing of "clk" in the error path of
> clk_prepare_enable, but that's about it.
>
> BTW. DO we really need to bike shed such a simple patch to death like
> that ?
It was not about bike shedding, just about the correctness...
[...]
MBR, Sergei
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* [v4] usb: uhci: Add clk support to uhci-platform
@ 2018-01-16 9:48 Benjamin Herrenschmidt
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2018-01-16 9:48 UTC (permalink / raw)
To: Sergei Shtylyov, Alan Stern; +Cc: linux-usb, Greg KH, Joel Stanley
On Tue, 2018-01-16 at 11:36 +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 1/16/2018 12:53 AM, Benjamin Herrenschmidt wrote:
>
> > > > The Aspeed SoCs use uhci-platform. With the new dynamic clock
> > > > control framework, the corresponding IP block clock must be
> > > > properly enabled.
> > > >
> > > > This is a simplified variant of what ehci-platform does, it
> > > > looks for *one* clock attached to the device, and if it's
> > > > there, enables it.
> > > >
> > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > > Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > > > ---
> > > >
> > > > v4. Remove unnecessary NULL checks
> > > >
> > > > v3. Fix tab vs. spaces and probe error path
> > > >
> > > > v2. Don't forget to git add latest changes :-) This adds the
> > > > part where we turn the clock off when removing the driver.
> > > > ---
> > > > drivers/usb/host/uhci-hcd.h | 3 +++
> > > > drivers/usb/host/uhci-platform.c | 20 ++++++++++++++++++--
> > > > 2 files changed, 21 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> > > > index f1cc47292a59..7f9f33c8c232 100644
> > > > --- a/drivers/usb/host/uhci-hcd.h
> > > > +++ b/drivers/usb/host/uhci-hcd.h
> > > > @@ -4,6 +4,7 @@
> > > >
> > > > #include <linux/list.h>
> > > > #include <linux/usb.h>
> > > > +#include <linux/clk.h>
> > > >
> > > > #define usb_packetid(pipe) (usb_pipein(pipe) ? USB_PID_IN : USB_PID_OUT)
> > > > #define PIPE_DEVEP_MASK 0x0007ff00
> > > > @@ -447,6 +448,8 @@ struct uhci_hcd {
> > > > int total_load; /* Sum of array values */
> > > > short load[MAX_PHASE]; /* Periodic allocations */
> > > >
> > > > + struct clk *clk; /* (optional) clock source */
> > > > +
> > > > /* Reset host controller */
> > > > void (*reset_hc) (struct uhci_hcd *uhci);
> > > > int (*check_and_reset_hc) (struct uhci_hcd *uhci);
> > > > diff --git a/drivers/usb/host/uhci-platform.c b/drivers/usb/host/uhci-platform.c
> > > > index 6cb16d4b2257..55c2d99bf7ec 100644
> > > > --- a/drivers/usb/host/uhci-platform.c
> > > > +++ b/drivers/usb/host/uhci-platform.c
> > > > @@ -89,6 +89,8 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > > > if (!hcd)
> > > > return -ENOMEM;
> > > >
> > > > + uhci = hcd_to_uhci(hcd);
> > > > +
> > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > hcd->regs = devm_ioremap_resource(&pdev->dev, res);
> > > > if (IS_ERR(hcd->regs)) {
> > > > @@ -98,8 +100,6 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > > > hcd->rsrc_start = res->start;
> > > > hcd->rsrc_len = resource_size(res);
> > > >
> > > > - uhci = hcd_to_uhci(hcd);
> > > > -
> > > > uhci->regs = hcd->regs;
> > > >
> > > > /* Grab some things from the device-tree */
> > > > @@ -119,6 +119,19 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > > > "Enabled Aspeed implementation workarounds\n");
> > > > }
> > > > }
> > > > +
> > > > + /* Get and enable clock if any specified */
> > > > + uhci->clk = devm_clk_get(&pdev->dev, NULL);
> > > > + if (IS_ERR(uhci->clk)) {
> > > > + ret = PTR_ERR(uhci->clk);
> > > > + goto err_rmr;
> > > > + }
> > > > + ret = clk_prepare_enable(uhci->clk);
> > > > + if (ret) {
> > > > + dev_err(&pdev->dev, "Error couldn't enable clock (%d)\n", ret);
> > > > + goto err_rmr;
> > > > + }
> > > > +
> > > > ret = usb_add_hcd(hcd, pdev->resource[1].start, IRQF_SHARED);
> > > > if (ret)
> > > > goto err_rmr;
> > > > @@ -127,6 +140,7 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > > > return 0;
> > > >
> > > > err_rmr:
> > > > + clk_disable_unprepare(uhci->clk);
> > >
> > > Really? You haven't enabled the clock before either *goto* above, why
> > > disable it here?
> >
> > If clk is NULL or an error, disable will do nothing. The only case
>
> The rule of thumb was that you don't pass the error pointers to APIs but
> indeed I'm now seeing the NULL check extended to the error pointers too...
>
> > would be if enable at failed but then disable will do nothing in that
> > case.
>
> Are you sure there'll be no WARN_ON() triggered by clk_disable() with the
> enable count == 0?
There is, and while we won't hit that on Aspeed as it won't fail, we
should probably make it right.
> > The above change was done at the suggestion of Alan. If you really
>
> I'd probably missed his mail.
>
> > prefer I could add a clk_put and clearing of "clk" in the error path of
> > clk_prepare_enable, but that's about it.
I think the best is separate goto labels. I'll respin tomorrow.
Cheers,
Ben.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-16 9:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-14 22:59 [v4] usb: uhci: Add clk support to uhci-platform Benjamin Herrenschmidt
-- strict thread matches above, loose matches on Subject: below --
2018-01-15 8:41 Sergei Shtylyov
2018-01-15 21:53 Benjamin Herrenschmidt
2018-01-15 22:17 Joel Stanley
2018-01-16 8:36 Sergei Shtylyov
2018-01-16 9:48 Benjamin Herrenschmidt
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).