From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Wed, 13 Jan 2016 15:44:05 +0100
Subject: [U-Boot] [PATCH] usb: add clock support for generic EHCI
In-Reply-To: <1452659641-17599-1-git-send-email-yamada.masahiro@socionext.com>
References: <1452659641-17599-1-git-send-email-yamada.masahiro@socionext.com>
Message-ID: <201601131544.05957.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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