* [RESEND PATCH 0/1] add multiple clock handling for dwc2 driver @ 2017-02-05 2:51 Frank Wang 2017-02-05 2:51 ` [RESEND PATCH 1/1] usb: dwc2: add multiple clock handling Frank Wang 2017-02-05 9:41 ` [RESEND PATCH 0/1] add multiple clock handling for dwc2 driver Heiko Stuebner 0 siblings, 2 replies; 8+ messages in thread From: Frank Wang @ 2017-02-05 2:51 UTC (permalink / raw) To: johnyoun, gregkh, heiko Cc: linux-kernel, linux-usb, linux-rockchip, huangtao, kever.yang, william.wu, frank.wang The original posting on Jan 19th have not received any responses, so I resend them. The Current default dwc2 just handle one clock named otg, however, it may have two or more clock need to manage for some new SoCs(such as RK3328), so this adds change clk to clk's array of dwc2_hsotg to handle more clocks operation. Frank Wang (1): usb: dwc2: add multiple clock handling drivers/usb/dwc2/core.h | 5 ++++- drivers/usb/dwc2/platform.c | 39 ++++++++++++++++++++++++++------------- 2 files changed, 30 insertions(+), 14 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RESEND PATCH 1/1] usb: dwc2: add multiple clock handling 2017-02-05 2:51 [RESEND PATCH 0/1] add multiple clock handling for dwc2 driver Frank Wang @ 2017-02-05 2:51 ` Frank Wang 2017-02-07 0:06 ` Heiko Stuebner 2017-02-05 9:41 ` [RESEND PATCH 0/1] add multiple clock handling for dwc2 driver Heiko Stuebner 1 sibling, 1 reply; 8+ messages in thread From: Frank Wang @ 2017-02-05 2:51 UTC (permalink / raw) To: johnyoun, gregkh, heiko Cc: linux-kernel, linux-usb, linux-rockchip, huangtao, kever.yang, william.wu, 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@rock-chips.com> --- 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 + /* 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; diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index 9564bc7..795fc43b 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -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; + } } /* Regulators */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 1/1] usb: dwc2: add multiple clock handling 2017-02-05 2:51 ` [RESEND PATCH 1/1] usb: dwc2: add multiple clock handling Frank Wang @ 2017-02-07 0:06 ` Heiko Stuebner 2017-02-07 3:18 ` Frank Wang 0 siblings, 1 reply; 8+ messages in thread From: Heiko Stuebner @ 2017-02-07 0:06 UTC (permalink / raw) To: Frank Wang Cc: johnyoun, gregkh, linux-kernel, linux-usb, linux-rockchip, huangtao, kever.yang, william.wu 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@rock-chips.com> > --- > 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? 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. 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. Heiko ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 1/1] usb: dwc2: add multiple clock handling 2017-02-07 0:06 ` Heiko Stuebner @ 2017-02-07 3:18 ` Frank Wang 2017-02-07 9:51 ` John Youn 0 siblings, 1 reply; 8+ messages in thread From: Frank Wang @ 2017-02-07 3:18 UTC (permalink / raw) To: Heiko Stuebner, johnyoun, gregkh Cc: linux-kernel, linux-usb, linux-rockchip, huangtao, kever.yang, william.wu, 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@rock-chips.com> >> --- >> 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 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 1/1] usb: dwc2: add multiple clock handling 2017-02-07 3:18 ` Frank Wang @ 2017-02-07 9:51 ` John Youn 0 siblings, 0 replies; 8+ messages in thread From: John Youn @ 2017-02-07 9:51 UTC (permalink / raw) To: Frank Wang, Heiko Stuebner, John.Youn, gregkh Cc: linux-kernel, linux-usb, linux-rockchip, huangtao, kever.yang, william.wu On 2/6/2017 7:18 PM, Frank Wang wrote: > 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@rock-chips.com> >>> --- >>> 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_??? . This is fine with me. >> >> 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. This should be optional since not every platform is going to need to specify it. And it is implemented that way now so I think this doc needs to be updated. > > 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. I'd like to keep all the clocks optional and all accessed in a similar manner. Regards, John > > 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 >> >> >> > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 0/1] add multiple clock handling for dwc2 driver 2017-02-05 2:51 [RESEND PATCH 0/1] add multiple clock handling for dwc2 driver Frank Wang 2017-02-05 2:51 ` [RESEND PATCH 1/1] usb: dwc2: add multiple clock handling Frank Wang @ 2017-02-05 9:41 ` Heiko Stuebner 2017-02-06 1:40 ` Frank Wang 1 sibling, 1 reply; 8+ messages in thread From: Heiko Stuebner @ 2017-02-05 9:41 UTC (permalink / raw) To: Frank Wang Cc: johnyoun, gregkh, linux-kernel, linux-usb, linux-rockchip, huangtao, kever.yang, william.wu Hi Frank, Am Sonntag, 5. Februar 2017, 10:51:00 CET schrieb Frank Wang: > The original posting on Jan 19th have not received any responses, so I > resend them. > > The Current default dwc2 just handle one clock named otg, however, it may > have two or more clock need to manage for some new SoCs(such as RK3328), so > this adds change clk to clk's array of dwc2_hsotg to handle more clocks > operation. can you please give a bit more detail on the specific layout. I guess you're talking about hclk_otg_pmu, right? What component does it supply, because I didn't find anything in the partial TRM in the PMU section relating to the "otg". This meant to make sure, you're actually controlling some part of the dwc2 with that second/third/... clock and not some separate component. Heiko ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 0/1] add multiple clock handling for dwc2 driver 2017-02-05 9:41 ` [RESEND PATCH 0/1] add multiple clock handling for dwc2 driver Heiko Stuebner @ 2017-02-06 1:40 ` Frank Wang 2017-02-06 23:53 ` Heiko Stuebner 0 siblings, 1 reply; 8+ messages in thread From: Frank Wang @ 2017-02-06 1:40 UTC (permalink / raw) To: Heiko Stuebner Cc: johnyoun, gregkh, linux-kernel, linux-usb, linux-rockchip, huangtao, kever.yang, william.wu, elaine.zhang, frank.wang Hi Heiko, On 2017/2/5 17:41, Heiko Stuebner wrote: > Hi Frank, > > Am Sonntag, 5. Februar 2017, 10:51:00 CET schrieb Frank Wang: >> The original posting on Jan 19th have not received any responses, so I >> resend them. >> >> The Current default dwc2 just handle one clock named otg, however, it may >> have two or more clock need to manage for some new SoCs(such as RK3328), so >> this adds change clk to clk's array of dwc2_hsotg to handle more clocks >> operation. > can you please give a bit more detail on the specific layout. > > I guess you're talking about hclk_otg_pmu, right? What component does it > supply, because I didn't find anything in the partial TRM in the PMU section > relating to the "otg". Yes, it is hclk_otg_pmu. The rock-chip hclk_otg_pmu is an input clock for dwc2 PMU module which named pmu_hclk in chapter 2.4 of dwc otg databook v3.10. Please refer to the following simple clock tree from CRU part of rk3328 TRM. _ _ _ ... | |- - -> G19_8 - - - hclk_otg - - - - - - -> hclk_peri_pre - - ->| |- - -> G19_9 - - - hclk_otg_pmu - - -> |_ _ _ ... BR. Frank > This meant to make sure, you're actually controlling some part of the dwc2 > with that second/third/... clock and not some separate component. > > > Heiko > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 0/1] add multiple clock handling for dwc2 driver 2017-02-06 1:40 ` Frank Wang @ 2017-02-06 23:53 ` Heiko Stuebner 0 siblings, 0 replies; 8+ messages in thread From: Heiko Stuebner @ 2017-02-06 23:53 UTC (permalink / raw) To: Frank Wang Cc: johnyoun, gregkh, linux-kernel, linux-usb, linux-rockchip, huangtao, kever.yang, william.wu, elaine.zhang Hi Frank, Am Montag, 6. Februar 2017, 09:40:35 CET schrieb Frank Wang: > On 2017/2/5 17:41, Heiko Stuebner wrote: > > Am Sonntag, 5. Februar 2017, 10:51:00 CET schrieb Frank Wang: > >> The original posting on Jan 19th have not received any responses, so I > >> resend them. > >> > >> The Current default dwc2 just handle one clock named otg, however, it may > >> have two or more clock need to manage for some new SoCs(such as RK3328), > >> so > >> this adds change clk to clk's array of dwc2_hsotg to handle more clocks > >> operation. > > > > can you please give a bit more detail on the specific layout. > > > > I guess you're talking about hclk_otg_pmu, right? What component does it > > supply, because I didn't find anything in the partial TRM in the PMU > > section relating to the "otg". > > Yes, it is hclk_otg_pmu. > > The rock-chip hclk_otg_pmu is an input clock for dwc2 PMU module which > named pmu_hclk in chapter 2.4 of dwc otg databook v3.10. ok great, on establishing that this is a actual part of the IP block. I'm going to comment on the actual code change in a minute, so see you over there :-) Heiko ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-02-07 9:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-05 2:51 [RESEND PATCH 0/1] add multiple clock handling for dwc2 driver Frank Wang 2017-02-05 2:51 ` [RESEND PATCH 1/1] usb: dwc2: add multiple clock handling Frank Wang 2017-02-07 0:06 ` Heiko Stuebner 2017-02-07 3:18 ` Frank Wang 2017-02-07 9:51 ` John Youn 2017-02-05 9:41 ` [RESEND PATCH 0/1] add multiple clock handling for dwc2 driver Heiko Stuebner 2017-02-06 1:40 ` Frank Wang 2017-02-06 23:53 ` Heiko Stuebner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).