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

[Masahiro Yamada]

Hi Marek,

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.




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