[U-Boot] [PATCH] usb: add clock support for generic EHCI

[Marek Vasut]

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 at 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