| Message ID | 1452659641-17599-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
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
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)));
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(+)