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