Re: [RESEND PATCH 1/1] usb: dwc2: add multiple clock handling

[Frank Wang]

Hi Heiko, John and Greg,

On 2017/2/7 8:06, Heiko Stuebner wrote:
> Hi Frank,
>
> Am Sonntag, 5. Februar 2017, 10:51:01 CET schrieb Frank Wang:
> > Originally, dwc2 just handle one clock named otg, however, it may have
> > two or more clock need to manage for some new SoCs, so this adds
> > change clk to clk's array of dwc2_hsotg to handle more clocks operation.
> >
> > Signed-off-by: Frank Wang <frank.wang@xxxxxxxxxxxxxx>
> > ---
> >   drivers/usb/dwc2/core.h     |  5 ++++-
> >   drivers/usb/dwc2/platform.c | 39 ++++++++++++++++++++++++++-------------
> >   2 files changed, 30 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> > index 1a7e830..d10a466 100644
> > --- a/drivers/usb/dwc2/core.h
> > +++ b/drivers/usb/dwc2/core.h
> > @@ -121,6 +121,9 @@ static inline void dwc2_writel(u32 value, void __iomem
> > *addr) /* Maximum number of Endpoints/HostChannels */
> >   #define MAX_EPS_CHANNELS	16
> >
> > +/* Maximum number of dwc2 clocks */
> > +#define DWC2_MAX_CLKS 3
> why 3 clocks?

Showing from the chapter 2.4 of dwc otg databook v3.10, it seems there 
should be five clocks, am I right?
hclk/pmu_hclk/utmi_clk/ulpi_clk/utmifs_clk48

> I.e. the binding currently only specifies the "otg" clock, so you should
> definitly amend it to also specifiy this "pmu" clock - in a separate patch
> before this one of course :-) .
> "pmu" also looks like a good name for that clock-binding and these new clocks
> of course should be optional in the binding.

No problem, I will amend the binding when the implementation scheme is 
clear.

> And ideally also just specify this mysterious third clock as well while you're
> at it.
>
> > +
> >   /* dwc2-hsotg declarations */
> >   static const char * const dwc2_hsotg_supply_names[] = {
> >   	"vusb_d",               /* digital USB supply, 1.2V */
> > @@ -913,7 +916,7 @@ struct dwc2_hsotg {
> >   	spinlock_t lock;
> >   	void *priv;
> >   	int     irq;
> > -	struct clk *clk;
> > +	struct clk *clks[DWC2_MAX_CLKS];
> >   	struct reset_control *reset;
> >
> >   	unsigned int queuing_high_bandwidth:1;
> [...]
>
> > @@ -123,17 +123,20 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
> >   static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
> >   {
> >   	struct platform_device *pdev = to_platform_device(hsotg->dev);
> > -	int ret;
> > +	int clk, ret;
> >
> >   	ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
> >   				    hsotg->supplies);
> >   	if (ret)
> >   		return ret;
> >
> > -	if (hsotg->clk) {
> > -		ret = clk_prepare_enable(hsotg->clk);
> > -		if (ret)
> > +	for (clk = 0; clk < DWC2_MAX_CLKS && hsotg->clks[clk]; clk++) {
> > +		ret = clk_prepare_enable(hsotg->clks[clk]);
> > +		if (ret) {
> > +			while (--clk >= 0)
> > +				clk_disable_unprepare(hsotg->clks[clk]);
> >   			return ret;
> > +		}
> >   	}
> >
> >   	if (hsotg->uphy) {
> > @@ -168,7 +171,7 @@ int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
> >   static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
> >   {
> >   	struct platform_device *pdev = to_platform_device(hsotg->dev);
> > -	int ret = 0;
> > +	int clk, ret = 0;
> >
> >   	if (hsotg->uphy) {
> >   		usb_phy_shutdown(hsotg->uphy);
> > @@ -182,8 +185,9 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg
> > *hsotg) if (ret)
> >   		return ret;
> >
> > -	if (hsotg->clk)
> > -		clk_disable_unprepare(hsotg->clk);
> > +	for (clk = DWC2_MAX_CLKS - 1; clk >= 0; clk--)
> > +		if (hsotg->clks[clk])
> > +			clk_disable_unprepare(hsotg->clks[clk]);
> >   	ret = regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
> >   				     hsotg->supplies);
> > @@ -209,7 +213,7 @@ int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
> >
> >   static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
> >   {
> > -	int i, ret;
> > +	int i, clk, ret;
> >
> >   	hsotg->reset = devm_reset_control_get_optional(hsotg->dev, "dwc2");
> >   	if (IS_ERR(hsotg->reset)) {
> > @@ -282,11 +286,20 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg
> > *hsotg) hsotg->phyif = GUSBCFG_PHYIF8;
> >   	}
> >
> > -	/* Clock */
> > -	hsotg->clk = devm_clk_get(hsotg->dev, "otg");
> > -	if (IS_ERR(hsotg->clk)) {
> > -		hsotg->clk = NULL;
> > -		dev_dbg(hsotg->dev, "cannot get otg clock\n");
> > +	/* Clocks */
> > +	for (clk = 0; clk < DWC2_MAX_CLKS; clk++) {
> > +		hsotg->clks[clk] = of_clk_get(hsotg->dev->of_node, clk);
> > +		if (IS_ERR(hsotg->clks[clk])) {
> > +			ret = PTR_ERR(hsotg->clks[clk]);
> > +			if (ret == -EPROBE_DEFER) {
> > +				while (--clk >= 0)
> > +					clk_put(hsotg->clks[clk]);
> > +				return ret;
> > +			}
> > +
> > +			hsotg->clks[clk] = NULL;
> > +			break;
> > +		}
> I guess this depends on the feelings of the usb-people, but for me it might
> make more sense to not carry the clocks in an anonymous array but instead as
> named members in struct dwc2_hsotg - aka clk_otg, clk_pmu, clk_??? .
>
> Because the otg clocks is actually marked as required in the binding while our
> two new clocks are optional and some future code might actually want to
> control these separate clocks to save some power somewhere.
>
>
> Sidenote: I don't really understand why the driver allows the otg clock to be
> missing, as it is a required property in the binding.

Yes, if there are only two clocks need to control, separate member in 
struct dwc2_hsotg is really better,
but for more clocks, the operations of each clock 
(get/prepare/unprepare) may become tedious

How about keeping the original 'otg' clk and adding a new clk array 
member for optional clocks in struct dwc2_hsotg?
because excepting 'otg' clk, the others are all optional clocks.

Of course, if we only consider hclk and pmu_hclk, I guess the separate 
member scheme is still better.

John and Greg, would you like to give some comments please?


BR.
Frank

> Heiko