* [U-Boot] [PATCH] usb: add clock support for generic EHCI
@ 2016-01-13 4:34 Masahiro Yamada
2016-01-13 14:44 ` Marek Vasut
0 siblings, 1 reply; 4+ messages in thread
From: Masahiro Yamada @ 2016-01-13 4:34 UTC (permalink / raw)
To: u-boot
This driver is designed so generic that clock should also be handled
in a generic way.
Like the one in Linux (drivers/usb/host/ehci-platform.c), get and
enable clock(s) via Device Tree if present.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
This patch depends on new features that are still under review:
http://patchwork.ozlabs.org/patch/566809/
http://patchwork.ozlabs.org/patch/566812/
Please hold this one until they go in.
drivers/usb/host/ehci-generic.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
index 1292caa..44bf70b 100644
--- a/drivers/usb/host/ehci-generic.c
+++ b/drivers/usb/host/ehci-generic.c
@@ -5,9 +5,12 @@
*/
#include <common.h>
+#include <clk.h>
#include <dm.h>
#include "ehci.h"
+#define EHCI_MAX_CLKS 3
+
/*
* Even though here we don't explicitly use "struct ehci_ctrl"
* ehci_register() expects it to be the first thing that resides in
@@ -15,12 +18,24 @@
*/
struct generic_ehci {
struct ehci_ctrl ctrl;
+ struct udevice *clk_devs[EHCI_MAX_CLKS];
+ int clk_ids[EHCI_MAX_CLKS];
};
static int ehci_usb_probe(struct udevice *dev)
{
+ struct generic_ehci *priv = dev_get_priv(dev);
struct ehci_hccr *hccr = (struct ehci_hccr *)dev_get_addr(dev);
struct ehci_hcor *hcor;
+ int i;
+
+ for (i = 0; i < EHCI_MAX_CLKS; i++) {
+ priv->clk_ids[i] = fdt_clk_get(dev, i, &priv->clk_devs[i]);
+ if (priv->clk_ids[i] < 0)
+ break;
+ if (clk_enable(priv->clk_devs[i], priv->clk_ids[i]))
+ printf("failed to enable clock %d\n", priv->clk_ids[i]);
+ }
hcor = (struct ehci_hcor *)((uintptr_t)hccr +
HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] usb: add clock support for generic EHCI
2016-01-13 4:34 [U-Boot] [PATCH] usb: add clock support for generic EHCI Masahiro Yamada
@ 2016-01-13 14:44 ` Marek Vasut
2016-01-13 17:08 ` Masahiro Yamada
0 siblings, 1 reply; 4+ messages in thread
From: Marek Vasut @ 2016-01-13 14:44 UTC (permalink / raw)
To: u-boot
On Wednesday, January 13, 2016 at 05:34:01 AM, Masahiro Yamada wrote:
> This driver is designed so generic that clock should also be handled
> in a generic way.
>
> Like the one in Linux (drivers/usb/host/ehci-platform.c), get and
> enable clock(s) via Device Tree if present.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> This patch depends on new features that are still under review:
>
> http://patchwork.ozlabs.org/patch/566809/
> http://patchwork.ozlabs.org/patch/566812/
>
> Please hold this one until they go in.
>
>
> drivers/usb/host/ehci-generic.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-generic.c
> b/drivers/usb/host/ehci-generic.c index 1292caa..44bf70b 100644
> --- a/drivers/usb/host/ehci-generic.c
> +++ b/drivers/usb/host/ehci-generic.c
> @@ -5,9 +5,12 @@
> */
>
> #include <common.h>
> +#include <clk.h>
> #include <dm.h>
> #include "ehci.h"
>
> +#define EHCI_MAX_CLKS 3
> +
> /*
> * Even though here we don't explicitly use "struct ehci_ctrl"
> * ehci_register() expects it to be the first thing that resides in
> @@ -15,12 +18,24 @@
> */
> struct generic_ehci {
> struct ehci_ctrl ctrl;
> + struct udevice *clk_devs[EHCI_MAX_CLKS];
> + int clk_ids[EHCI_MAX_CLKS];
> };
>
> static int ehci_usb_probe(struct udevice *dev)
> {
> + struct generic_ehci *priv = dev_get_priv(dev);
> struct ehci_hccr *hccr = (struct ehci_hccr *)dev_get_addr(dev);
> struct ehci_hcor *hcor;
> + int i;
> +
> + for (i = 0; i < EHCI_MAX_CLKS; i++) {
Hi!
Can't you dynamically figure out how many clock are in the clocks property
and allocate the clk_ids array based on that ? This would remove the need
for this EHCI_MAX_CLKS ad-hoc constant.
> + priv->clk_ids[i] = fdt_clk_get(dev, i, &priv->clk_devs[i]);
Do you need to stort clk_devs in the priv structure at all ?
> + if (priv->clk_ids[i] < 0)
> + break;
> + if (clk_enable(priv->clk_devs[i], priv->clk_ids[i]))
I think you should also disable the clock in ehci_usb_remove() {} .
> + printf("failed to enable clock %d\n", priv->clk_ids[i]);
> + }
>
> hcor = (struct ehci_hcor *)((uintptr_t)hccr +
> HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] usb: add clock support for generic EHCI
2016-01-13 14:44 ` Marek Vasut
@ 2016-01-13 17:08 ` Masahiro Yamada
2016-01-13 23:58 ` Marek Vasut
0 siblings, 1 reply; 4+ messages in thread
From: Masahiro Yamada @ 2016-01-13 17:08 UTC (permalink / raw)
To: u-boot
Hi Marek,
2016-01-13 23:44 GMT+09:00 Marek Vasut <marex@denx.de>:
>> static int ehci_usb_probe(struct udevice *dev)
>> {
>> + struct generic_ehci *priv = dev_get_priv(dev);
>> struct ehci_hccr *hccr = (struct ehci_hccr *)dev_get_addr(dev);
>> struct ehci_hcor *hcor;
>> + int i;
>> +
>> + for (i = 0; i < EHCI_MAX_CLKS; i++) {
>
> Hi!
>
> Can't you dynamically figure out how many clock are in the clocks property
> and allocate the clk_ids array based on that ? This would remove the need
> for this EHCI_MAX_CLKS ad-hoc constant.
It is possible, but costly.
We have to know "#clock-cells" of each clock provider to do that.
Effectively, we have to call fdt_clk_get() until it fails
in order to get the number of the clocks.
Then, needed memory is allocated.
Again, we have to call fdt_clk_get(),
this time, in order to really get and store the clocks.
I think this is why the Linux one also uses the fixed value for the array size.
See linux/drivers/usb/host/ehci-platform.c.
>> + priv->clk_ids[i] = fdt_clk_get(dev, i, &priv->clk_devs[i]);
>
> Do you need to stort clk_devs in the priv structure at all ?
Yes, if we call clock_disable() in the .remove callback.
No, if we do not do that.
>> + if (priv->clk_ids[i] < 0)
>> + break;
>> + if (clk_enable(priv->clk_devs[i], priv->clk_ids[i]))
>
> I think you should also disable the clock in ehci_usb_remove() {} .
Generally speaking, disabling clocks is more difficult than enabling them
because we should have enable_count.
(For example, assume two clock consumers share one clock gate)
Unlike Linux's clock framework, U-Boot's clock-uclass does not support
the enable_count.
Having said that, it makes sense to add clock_disable() in the .remove callback.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] usb: add clock support for generic EHCI
2016-01-13 17:08 ` Masahiro Yamada
@ 2016-01-13 23:58 ` Marek Vasut
0 siblings, 0 replies; 4+ messages in thread
From: Marek Vasut @ 2016-01-13 23:58 UTC (permalink / raw)
To: u-boot
On Wednesday, January 13, 2016 at 06:08:34 PM, Masahiro Yamada wrote:
> Hi Marek,
Hi!
> 2016-01-13 23:44 GMT+09:00 Marek Vasut <marex@denx.de>:
> >> static int ehci_usb_probe(struct udevice *dev)
> >> {
> >>
> >> + struct generic_ehci *priv = dev_get_priv(dev);
> >>
> >> struct ehci_hccr *hccr = (struct ehci_hccr *)dev_get_addr(dev);
> >> struct ehci_hcor *hcor;
> >>
> >> + int i;
> >> +
> >> + for (i = 0; i < EHCI_MAX_CLKS; i++) {
> >
> > Hi!
> >
> > Can't you dynamically figure out how many clock are in the clocks
> > property and allocate the clk_ids array based on that ? This would
> > remove the need for this EHCI_MAX_CLKS ad-hoc constant.
>
> It is possible, but costly.
>
> We have to know "#clock-cells" of each clock provider to do that.
> Effectively, we have to call fdt_clk_get() until it fails
> in order to get the number of the clocks.
>
> Then, needed memory is allocated.
>
> Again, we have to call fdt_clk_get(),
> this time, in order to really get and store the clocks.
>
> I think this is why the Linux one also uses the fixed value for the array
> size.
>
> See linux/drivers/usb/host/ehci-platform.c.
OK, let's go with this. Thanks for explaining.
> >> + priv->clk_ids[i] = fdt_clk_get(dev, i,
> >> &priv->clk_devs[i]);
> >
> > Do you need to stort clk_devs in the priv structure at all ?
>
> Yes, if we call clock_disable() in the .remove callback.
> No, if we do not do that.
OK
> >> + if (priv->clk_ids[i] < 0)
> >> + break;
> >> + if (clk_enable(priv->clk_devs[i], priv->clk_ids[i]))
> >
> > I think you should also disable the clock in ehci_usb_remove() {} .
>
> Generally speaking, disabling clocks is more difficult than enabling them
> because we should have enable_count.
> (For example, assume two clock consumers share one clock gate)
>
> Unlike Linux's clock framework, U-Boot's clock-uclass does not support
> the enable_count.
>
>
> Having said that, it makes sense to add clock_disable() in the .remove
> callback.
CCing Simon, let's see what he has to say.
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-01-13 23:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-13 4:34 [U-Boot] [PATCH] usb: add clock support for generic EHCI Masahiro Yamada
2016-01-13 14:44 ` Marek Vasut
2016-01-13 17:08 ` Masahiro Yamada
2016-01-13 23:58 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.