[U-Boot] [PATCH] usb: add clock support for generic EHCI
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)));
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
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.
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
participants (2)
-
Marek Vasut -
Masahiro Yamada