| Message ID | 1389161742-10533-2-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 08, 2014 at 11:45:38AM +0530, Roger Quadros wrote: > diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt > index b381fa6..5635202 100644 > --- a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt > +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt > @@ -32,6 +32,10 @@ Optional properties: > - single-ulpi-bypass: Must be present if the controller contains a single > ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 > > +- clocks: phandle to 60MHz functional clock to the USB Host module. > + > +- clock-names: must be "init_60m_fclk" > + > Required properties if child node exists: > > - #address-cells: Must be 1 I have some questions: What about the other clocks acquired in drivers/mfd/omap-usb-host.c? Shouldn't all of those be provided by via the DT phandle? Should the clk_get be changed to of_clk_get()/of_clk_get_by_name() in the driver? This would potentially remove the need of the init_60m_fclk name. $ grep clk_get drivers/mfd/omap-usb-host.c omap->ehci_logic_fck = clk_get(dev, "ehci_logic_fck"); omap->utmi_p1_gfclk = clk_get(dev, "utmi_p1_gfclk"); omap->utmi_p2_gfclk = clk_get(dev, "utmi_p2_gfclk"); omap->xclk60mhsp1_ck = clk_get(dev, "xclk60mhsp1_ck"); omap->xclk60mhsp2_ck = clk_get(dev, "xclk60mhsp2_ck"); omap->init_60m_fclk = clk_get(dev, "init_60m_fclk"); omap->utmi_clk[i] = clk_get(dev, clkname); omap->hsic480m_clk[i] = clk_get(dev, clkname); omap->hsic60m_clk[i] = clk_get(dev, clkname); -- Sebastian
On Wed, Jan 08, 2014 at 06:15:38AM +0000, Roger Quadros wrote: > The omap-usb-host driver expects the 60MHz functional clock > of the USB host module to be named as "init_60m_fclk". > Add this information in the DT binding document. > > CC: Lee Jones <lee.jones@linaro.org> > CC: Samuel Ortiz <sameo@linux.intel.com> > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > Documentation/devicetree/bindings/mfd/omap-usb-host.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt > index b381fa6..5635202 100644 > --- a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt > +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt > @@ -32,6 +32,10 @@ Optional properties: > - single-ulpi-bypass: Must be present if the controller contains a single > ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 > > +- clocks: phandle to 60MHz functional clock to the USB Host module. > + > +- clock-names: must be "init_60m_fclk" > + Nit: clocks aren't just phandles, there's a clock-specifier too. Also, it would be nicer if clocks was defined in terms of clock-names, as it makes the relationship between clocks and clock-names clear, and makes it easier to extend the list in future. Something like: - clocks: a list of phandles + clock-specifiers, one for each entry in clock-names - clock-names: should include: * "init_60m_fclk" - the 60MHz functional clock to the USB host module. Thanks, Mark.
+Tero Hi Sebastian, On 01/08/2014 02:38 PM, Sebastian Reichel wrote: > On Wed, Jan 08, 2014 at 11:45:38AM +0530, Roger Quadros wrote: >> diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> index b381fa6..5635202 100644 >> --- a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> @@ -32,6 +32,10 @@ Optional properties: >> - single-ulpi-bypass: Must be present if the controller contains a single >> ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 >> >> +- clocks: phandle to 60MHz functional clock to the USB Host module. >> + >> +- clock-names: must be "init_60m_fclk" >> + >> Required properties if child node exists: >> >> - #address-cells: Must be 1 > > I have some questions: > > What about the other clocks acquired in drivers/mfd/omap-usb-host.c? Shouldn't > all of those be provided by via the DT phandle? > All those clocks are identically named across the OMAP SoCs and are unique for each SoC, so providing DT phandle for all of them is not required. The init_60m_fclk was renamed to l3init_60m_fclk in OMAP5, and hence the need for this binding. > Should the clk_get be changed to of_clk_get()/of_clk_get_by_name() in the > driver? This would potentially remove the need of the init_60m_fclk name. > If we use of_clk_xxx() then we'll need to update DT nodes for OMAP4 and OMAP3 as well to explicitly provide the clock phandle. For now we make use of the fact that SoC clock data names the clock rightly i.e. "init_60m_fclk". > $ grep clk_get drivers/mfd/omap-usb-host.c > omap->ehci_logic_fck = clk_get(dev, "ehci_logic_fck"); > omap->utmi_p1_gfclk = clk_get(dev, "utmi_p1_gfclk"); > omap->utmi_p2_gfclk = clk_get(dev, "utmi_p2_gfclk"); > omap->xclk60mhsp1_ck = clk_get(dev, "xclk60mhsp1_ck"); > omap->xclk60mhsp2_ck = clk_get(dev, "xclk60mhsp2_ck"); > omap->init_60m_fclk = clk_get(dev, "init_60m_fclk"); > omap->utmi_clk[i] = clk_get(dev, clkname); > omap->hsic480m_clk[i] = clk_get(dev, clkname); > omap->hsic60m_clk[i] = clk_get(dev, clkname); > cheers, -roger
Hi Mark, On 01/08/2014 03:26 PM, Mark Rutland wrote: > On Wed, Jan 08, 2014 at 06:15:38AM +0000, Roger Quadros wrote: >> The omap-usb-host driver expects the 60MHz functional clock >> of the USB host module to be named as "init_60m_fclk". >> Add this information in the DT binding document. >> >> CC: Lee Jones <lee.jones@linaro.org> >> CC: Samuel Ortiz <sameo@linux.intel.com> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> Documentation/devicetree/bindings/mfd/omap-usb-host.txt | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> index b381fa6..5635202 100644 >> --- a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> @@ -32,6 +32,10 @@ Optional properties: >> - single-ulpi-bypass: Must be present if the controller contains a single >> ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 >> >> +- clocks: phandle to 60MHz functional clock to the USB Host module. >> + >> +- clock-names: must be "init_60m_fclk" >> + > > Nit: clocks aren't just phandles, there's a clock-specifier too. > > Also, it would be nicer if clocks was defined in terms of clock-names, > as it makes the relationship between clocks and clock-names clear, and > makes it easier to extend the list in future. Something like: > > - clocks: a list of phandles + clock-specifiers, one for each entry in > clock-names > > - clock-names: should include: > * "init_60m_fclk" - the 60MHz functional clock to the USB host module. > OK, I'll update it as per your suggestion. cheers, -roger
On Wednesday 08 January 2014 15:39:36 Roger Quadros wrote: > > What about the other clocks acquired in drivers/mfd/omap-usb-host.c? Shouldn't > > all of those be provided by via the DT phandle? > > > > All those clocks are identically named across the OMAP SoCs and are unique for each > SoC, so providing DT phandle for all of them is not required. > > The init_60m_fclk was renamed to l3init_60m_fclk in OMAP5, and hence the need for > this binding. What do you mean it was renamed? Is it a different version of the omap-usb-host device then? > > Should the clk_get be changed to of_clk_get()/of_clk_get_by_name() in the > > driver? This would potentially remove the need of the init_60m_fclk name. > > > > If we use of_clk_xxx() then we'll need to update DT nodes for OMAP4 and OMAP3 as > well to explicitly provide the clock phandle. For now we make use of the fact that > SoC clock data names the clock rightly i.e. "init_60m_fclk". I'm getting the feeling that init_60m_fclk is not the name of the clock input of the omap-usb-host device at all, but rather the name of a signal on the omap soc, which would not be an appropriate identifier for the binding. Arnd
On 01/08/2014 03:49 PM, Arnd Bergmann wrote: > On Wednesday 08 January 2014 15:39:36 Roger Quadros wrote: >>> What about the other clocks acquired in drivers/mfd/omap-usb-host.c? Shouldn't >>> all of those be provided by via the DT phandle? >>> >> >> All those clocks are identically named across the OMAP SoCs and are unique for each >> SoC, so providing DT phandle for all of them is not required. >> >> The init_60m_fclk was renamed to l3init_60m_fclk in OMAP5, and hence the need for >> this binding. > > What do you mean it was renamed? Is it a different version of the omap-usb-host > device then? I meant the clock gates name on the SoC was renamed. The omap-usb-host device version is revised as well. >>> Should the clk_get be changed to of_clk_get()/of_clk_get_by_name() in the >>> driver? This would potentially remove the need of the init_60m_fclk name. >>> >> >> If we use of_clk_xxx() then we'll need to update DT nodes for OMAP4 and OMAP3 as >> well to explicitly provide the clock phandle. For now we make use of the fact that >> SoC clock data names the clock rightly i.e. "init_60m_fclk". > > I'm getting the feeling that init_60m_fclk is not the name of the clock input > of the omap-usb-host device at all, but rather the name of a signal on the > omap soc, which would not be an appropriate identifier for the binding. It is a clock gate defined like so in the DT clock data on OMAP4 init_60m_fclk: init_60m_fclk { #clock-cells = <0>; compatible = "ti,divider-clock"; clocks = <&dpll_usb_m2_ck>; reg = <0x0104>; ti,dividers = <1>, <8>; }; on OMAP5 l3init_60m_fclk: l3init_60m_fclk { #clock-cells = <0>; compatible = "ti,divider-clock"; clocks = <&dpll_usb_m2_ck>; reg = <0x0104>; ti,dividers = <1>, <8>; }; So you can see that it is the same thing with a different name. cheers, -roger
On Wednesday 08 January 2014 15:57:19 Roger Quadros wrote: > It is a clock gate defined like so in the DT clock data > > on OMAP4 > init_60m_fclk: init_60m_fclk { > #clock-cells = <0>; > compatible = "ti,divider-clock"; > clocks = <&dpll_usb_m2_ck>; > reg = <0x0104>; > ti,dividers = <1>, <8>; > }; > > on OMAP5 > l3init_60m_fclk: l3init_60m_fclk { > #clock-cells = <0>; > compatible = "ti,divider-clock"; > clocks = <&dpll_usb_m2_ck>; > reg = <0x0104>; > ti,dividers = <1>, <8>; > }; > > So you can see that it is the same thing with a different name. Right, but init_60m_fclk is the name of the clock output of the divider here, which is /not/ what you should put in the "clock-names" property of the consumer. The clock input name should reflect what the clock is used for instead. Arnd
Hi, On Wed, Jan 08, 2014 at 03:39:36PM +0530, Roger Quadros wrote: > > What about the other clocks acquired in drivers/mfd/omap-usb-host.c? Shouldn't > > all of those be provided by via the DT phandle? > > All those clocks are identically named across the OMAP SoCs and are unique for each > SoC, so providing DT phandle for all of them is not required. > > The init_60m_fclk was renamed to l3init_60m_fclk in OMAP5, and hence the need for > this binding. I understand the intention of this patch. I was just wondering if all the clocks should be referenced from DT even if that is not strictly needed at the moment. This would make clocks similar to other resources like regulators, gpios, irqs, ... Having the clocks referenced from DT looks cleaner to me. It means I can check the DT file for any resources used by a driver. It also creates some kind of consistency in the kernel. > > Should the clk_get be changed to of_clk_get()/of_clk_get_by_name() in the > > driver? This would potentially remove the need of the init_60m_fclk name. > > If we use of_clk_xxx() then we'll need to update DT nodes for OMAP4 and OMAP3 as > well to explicitly provide the clock phandle. I'm aware of this. -- Sebastian
On Wednesday 08 January 2014 11:52:44 Sebastian Reichel wrote: > > On Wed, Jan 08, 2014 at 03:39:36PM +0530, Roger Quadros wrote: > > > What about the other clocks acquired in drivers/mfd/omap-usb-host.c? Shouldn't > > > all of those be provided by via the DT phandle? > > > > All those clocks are identically named across the OMAP SoCs and are unique for each > > SoC, so providing DT phandle for all of them is not required. > > > > The init_60m_fclk was renamed to l3init_60m_fclk in OMAP5, and hence the need for > > this binding. > > I understand the intention of this patch. I was just wondering if > all the clocks should be referenced from DT even if that is not > strictly needed at the moment. This would make clocks similar to > other resources like regulators, gpios, irqs, ... > > Having the clocks referenced from DT looks cleaner to me. It means I > can check the DT file for any resources used by a driver. It also > creates some kind of consistency in the kernel. I think that would be best, yes. AFAIK most other platforms do this already, OMAP is a bit behind because it started using clocks when the infrastructure for doing this right was still incomplete. Arnd
On 01/08/2014 04:10 PM, Arnd Bergmann wrote: > On Wednesday 08 January 2014 15:57:19 Roger Quadros wrote: >> It is a clock gate defined like so in the DT clock data >> >> on OMAP4 >> init_60m_fclk: init_60m_fclk { >> #clock-cells = <0>; >> compatible = "ti,divider-clock"; >> clocks = <&dpll_usb_m2_ck>; >> reg = <0x0104>; >> ti,dividers = <1>, <8>; >> }; >> >> on OMAP5 >> l3init_60m_fclk: l3init_60m_fclk { >> #clock-cells = <0>; >> compatible = "ti,divider-clock"; >> clocks = <&dpll_usb_m2_ck>; >> reg = <0x0104>; >> ti,dividers = <1>, <8>; >> }; >> >> So you can see that it is the same thing with a different name. > > Right, but init_60m_fclk is the name of the clock output of the > divider here, which is /not/ what you should put in the "clock-names" > property of the consumer. The clock input name should reflect what > the clock is used for instead. Ah, now I get it. :). I agree that the name should reflect the function. Looking more closely, the driver doesn't enable/disable the init_60m_fclk but just uses it to configure the parent of utmi_p1_gfclk which is a clock input to the USB module. Based on this I would call it "refclk_int" for internal reference clock. cheers, -roger
On 01/08/2014 04:25 PM, Arnd Bergmann wrote: > On Wednesday 08 January 2014 11:52:44 Sebastian Reichel wrote: >> >> On Wed, Jan 08, 2014 at 03:39:36PM +0530, Roger Quadros wrote: >>>> What about the other clocks acquired in drivers/mfd/omap-usb-host.c? Shouldn't >>>> all of those be provided by via the DT phandle? >>> >>> All those clocks are identically named across the OMAP SoCs and are unique for each >>> SoC, so providing DT phandle for all of them is not required. >>> >>> The init_60m_fclk was renamed to l3init_60m_fclk in OMAP5, and hence the need for >>> this binding. >> >> I understand the intention of this patch. I was just wondering if >> all the clocks should be referenced from DT even if that is not >> strictly needed at the moment. This would make clocks similar to >> other resources like regulators, gpios, irqs, ... >> >> Having the clocks referenced from DT looks cleaner to me. It means I >> can check the DT file for any resources used by a driver. It also >> creates some kind of consistency in the kernel. > > I think that would be best, yes. AFAIK most other platforms do this > already, OMAP is a bit behind because it started using clocks when the > infrastructure for doing this right was still incomplete. > OK. I'll update the binding information to reflect all the clocks. But what about clk_get() vs of_clk_get_by_name() ? cheers, -roger
On Wednesday 08 January 2014, Roger Quadros wrote: > OK. I'll update the binding information to reflect all the clocks. > > But what about clk_get() vs of_clk_get_by_name() ? I think the convention these days is to just use clk_get(), which calls of_clk_get_by_name() internally. Not sure if that's what you are asking. Arnd
On 01/08/2014 05:05 PM, Arnd Bergmann wrote: > On Wednesday 08 January 2014, Roger Quadros wrote: >> OK. I'll update the binding information to reflect all the clocks. >> >> But what about clk_get() vs of_clk_get_by_name() ? > > I think the convention these days is to just use clk_get(), which calls > of_clk_get_by_name() internally. Not sure if that's what you are asking. > OK fine. I'll continue to use clk_get() then. cheers, -roger
diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt index b381fa6..5635202 100644 --- a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt @@ -32,6 +32,10 @@ Optional properties: - single-ulpi-bypass: Must be present if the controller contains a single ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 +- clocks: phandle to 60MHz functional clock to the USB Host module. + +- clock-names: must be "init_60m_fclk" + Required properties if child node exists: - #address-cells: Must be 1
The omap-usb-host driver expects the 60MHz functional clock of the USB host module to be named as "init_60m_fclk". Add this information in the DT binding document. CC: Lee Jones <lee.jones@linaro.org> CC: Samuel Ortiz <sameo@linux.intel.com> Signed-off-by: Roger Quadros <rogerq@ti.com> --- Documentation/devicetree/bindings/mfd/omap-usb-host.txt | 4 ++++ 1 file changed, 4 insertions(+)