Skip to contentSkip to conversation

usb: add clock support for generic EHCI

PATCH New Masahiro Yamada 2 participants 4 messages Jan 13, 2016

Subsystems (2)
USB USB ehci driver
lore
Share & tools
lore

Masahiro Yamada

author
#0 lore raw

PATCH 1/1 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 <[email protected]>

---

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(+)
drivers/usb/host/ehci-generic.c +15 -0
index 1292caa..44bf70b 100644
@@ -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)));
Signature · 1 line
1.9.1
#1 lore raw

PATCH 1/1 usb: add clock support for generic EHCI

Quoted 43 lines · Masahiro Yamada

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 <[email protected]>

---

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(+)
(quoted diff) +0 -0

b/drivers/usb/host/ehci-generic.c index 1292caa..44bf70b 100644

drivers/usb/host/ehci-generic.c +5 -0
@@ -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];

drivers/usb/host/ehci-generic.c line 32

Quoted 10 lines
drivers/usb/host/ehci-generic.c +4 -0
 };
 
 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.

drivers/usb/host/ehci-generic.c line 33

Quoted 1 line
drivers/usb/host/ehci-generic.c +1 -0
+		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 ?

drivers/usb/host/ehci-generic.c line 36

Quoted 3 lines
drivers/usb/host/ehci-generic.c +3 -0
+		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() {} .

drivers/usb/host/ehci-generic.c line 41

Quoted 5 lines
drivers/usb/host/ehci-generic.c +2 -0
+			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

author
#2 lore raw

PATCH 1/1 usb: add clock support for generic EHCI

Hi Marek,

2016-01-13 23:44 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 9 lines

drivers/usb/host/ehci-generic.c line 32

Quoted 8 lines

static int ehci_usb_probe(struct udevice *dev)
{

drivers/usb/host/ehci-generic.c +4 -0
+     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++) {
Quoted 5 lines

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.

Quoted 3 lines
Quoted 1 line

+ 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.

Quoted 5 lines

drivers/usb/host/ehci-generic.c line 36

Quoted 3 lines
drivers/usb/host/ehci-generic.c +3 -0
+             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.

Signature · 2 lines
Best Regards
Masahiro Yamada
#3 lore raw

PATCH 1/1 usb: add clock support for generic EHCI

Quoted 1 line · Masahiro Yamada

Hi Marek,

Hi!

Quoted 27 lines

2016-01-13 23:44 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 12 lines

drivers/usb/host/ehci-generic.c

Quoted 11 lines

static int ehci_usb_probe(struct udevice *dev)
{

drivers/usb/host/ehci-generic.c +4 -0
+     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++) {
Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

+ 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

Quoted 10 lines
Quoted 5 lines

drivers/usb/host/ehci-generic.c line 36

Quoted 3 lines
drivers/usb/host/ehci-generic.c +3 -0
+             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)

Quoted 6 lines

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