Re: [PATCHv5 01/31] CLK: clkdev: add support for looking up clocks from DT

Re: [PATCHv5 01/31] CLK: clkdev: add support for looking up clocks from DT

[Tero Kristo]

On 08/03/2013 09:31 PM, Russell King - ARM Linux wrote:
> On Fri, Aug 02, 2013 at 07:25:20PM +0300, Tero Kristo wrote:
>> +
>> +	if (cl)
>> +		return cl;
>> +
>> +	/* If clock was not found, attempt to look-up from DT */
>> +	node = of_find_node_by_name(NULL, con_id);
>
> This is utterly broken if you're looking up purely by a connection ID.
> Please, go back and read clk_get()'s documentation in linux/clk.h.
> It's spelt out very plainly there.
>
> NAK.
>

Hi Russell,

After looking at this a bit more, it seems difficult to get rid of the 
clk_get -> of_clk_get conversion, however based on this comment I am 
somewhat stuck. Do you think it would be acceptable if I change the 
implementation of this patch to work like this:

1) both dev_id + con_id defined:
    a) search for device node named dev_id
    b) check clock-names for matching con_id
    c) return matching clock

    Example in kernel:
       clocksource-nomadik-mtu.c :
           clk_get_sys("mtu0", "apb_pclk");

       ste-nomadik-snt8815.dts:
         mtu0: mtu@101e2000 {
                 /* Nomadik system timer */
                 compatible = "st,nomadik-mtu";
                 reg = <0x101e2000 0x1000>;
                 interrupt-parent = <&vica>;
                 interrupts = <4>;
                 clocks = <&timclk>, <&pclk>;
                 clock-names = "timclk", "apb_pclk";
         };

2) dev_id = NULL, con_id defined (current patch implementation, most of 
the OMAP clocks use this approach):
    a) search for a device node named con_id
    b) return corresponding clock from the node

Alternatively I must implement a regression and break some of the OMAP 
drivers with this set, and also implement omap internal clk_get 
functionality (basically adding a similar wrapper that I have 
implemented in this patch) under mach-omap2 to get some basic OMAP infra 
to work properly (namely, hwmod.) I can probably avoid part of this by 
adding more beef to the *.dts files for the critical parts to get boot 
working at least.

-Tero

Re: [PATCHv5 01/31] CLK: clkdev: add support for looking up clocks from DT

[Russell King - ARM Linux]

On Mon, Aug 26, 2013 at 05:36:15PM +0300, Tero Kristo wrote:
> On 08/03/2013 09:31 PM, Russell King - ARM Linux wrote:
>> On Fri, Aug 02, 2013 at 07:25:20PM +0300, Tero Kristo wrote:
>>> +
>>> +	if (cl)
>>> +		return cl;
>>> +
>>> +	/* If clock was not found, attempt to look-up from DT */
>>> +	node = of_find_node_by_name(NULL, con_id);
>>
>> This is utterly broken if you're looking up purely by a connection ID.
>> Please, go back and read clk_get()'s documentation in linux/clk.h.
>> It's spelt out very plainly there.
>>
>> NAK.
>>
>
> Hi Russell,
>
> After looking at this a bit more, it seems difficult to get rid of the  
> clk_get -> of_clk_get conversion, however based on this comment I am  
> somewhat stuck. Do you think it would be acceptable if I change the  
> implementation of this patch to work like this:

Firstly, for clk_get(), you don't need to do anything - clk_get() already
deals with DT, and if your DT is correct, it should already be returning
the appropriate clock(s).

I guess the problem here is clk_get_sys(), because that doesn't take a
struct device (and it doesn't take a struct device because it's used
from code where there is no struct device to use with it.)

If you have an OF node, what's wrong with using of_clk_get_by_name()?
If you don't have an OF node, how do you expect to find the clock in
the device tree, remembering that clocks are specific to devices, and
the 2nd argument to clk_get()/clk_get_sys() is not a unique identifier.

Re: [PATCHv5 01/31] CLK: clkdev: add support for looking up clocks from DT

[Tero Kristo]

On 08/26/2013 08:03 PM, Russell King - ARM Linux wrote:
> On Mon, Aug 26, 2013 at 05:36:15PM +0300, Tero Kristo wrote:
>> On 08/03/2013 09:31 PM, Russell King - ARM Linux wrote:
>>> On Fri, Aug 02, 2013 at 07:25:20PM +0300, Tero Kristo wrote:
>>>> +
>>>> +	if (cl)
>>>> +		return cl;
>>>> +
>>>> +	/* If clock was not found, attempt to look-up from DT */
>>>> +	node = of_find_node_by_name(NULL, con_id);
>>>
>>> This is utterly broken if you're looking up purely by a connection ID.
>>> Please, go back and read clk_get()'s documentation in linux/clk.h.
>>> It's spelt out very plainly there.
>>>
>>> NAK.
>>>
>>
>> Hi Russell,
>>
>> After looking at this a bit more, it seems difficult to get rid of the
>> clk_get -> of_clk_get conversion, however based on this comment I am
>> somewhat stuck. Do you think it would be acceptable if I change the
>> implementation of this patch to work like this:
>
> Firstly, for clk_get(), you don't need to do anything - clk_get() already
> deals with DT, and if your DT is correct, it should already be returning
> the appropriate clock(s).
>
> I guess the problem here is clk_get_sys(), because that doesn't take a
> struct device (and it doesn't take a struct device because it's used
> from code where there is no struct device to use with it.)

Yeah, most of the OMAP clocks have been traditionally referenced in this 
way, they are using clk_get_sys and with dev=NULL. conn-id is mostly unique.

>
> If you have an OF node, what's wrong with using of_clk_get_by_name()?
> If you don't have an OF node, how do you expect to find the clock in
> the device tree, remembering that clocks are specific to devices, and
> the 2nd argument to clk_get()/clk_get_sys() is not a unique identifier.

In the legacy code, pretty much all the clocks are mapped through 
clk_get_sys(NULL, conn), and large amount of the drivers don't even have 
DT conversion in place yet. I am attempting to handle the cases where we 
don't have OF nodes.... yet at least, without converting everything to 
DT, and without causing regressions (also, I don't even pretend to be 
capable of testing all the potential drivers for this.) Some parts of 
the basic infra which are using clk_get_sys are pretty silly also, like 
the ocp_if parts of the hwmod, I haven't figured out yet how to convert 
that to DT.

Personally I don't mind causing a regressions here though, if I get 
approval for it from Tony.

-Tero

Re: [PATCHv5 01/31] CLK: clkdev: add support for looking up clocks from DT

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [130826 11:19]:
> On 08/26/2013 08:03 PM, Russell King - ARM Linux wrote:
> >On Mon, Aug 26, 2013 at 05:36:15PM +0300, Tero Kristo wrote:
> >>On 08/03/2013 09:31 PM, Russell King - ARM Linux wrote:
> >>>On Fri, Aug 02, 2013 at 07:25:20PM +0300, Tero Kristo wrote:
> >>>>+
> >>>>+	if (cl)
> >>>>+		return cl;
> >>>>+
> >>>>+	/* If clock was not found, attempt to look-up from DT */
> >>>>+	node = of_find_node_by_name(NULL, con_id);
> >>>
> >>>This is utterly broken if you're looking up purely by a connection ID.
> >>>Please, go back and read clk_get()'s documentation in linux/clk.h.
> >>>It's spelt out very plainly there.
> >>>
> >>>NAK.
> >>>
> >>
> >>Hi Russell,
> >>
> >>After looking at this a bit more, it seems difficult to get rid of the
> >>clk_get -> of_clk_get conversion, however based on this comment I am
> >>somewhat stuck. Do you think it would be acceptable if I change the
> >>implementation of this patch to work like this:
> >
> >Firstly, for clk_get(), you don't need to do anything - clk_get() already
> >deals with DT, and if your DT is correct, it should already be returning
> >the appropriate clock(s).
> >
> >I guess the problem here is clk_get_sys(), because that doesn't take a
> >struct device (and it doesn't take a struct device because it's used
> >from code where there is no struct device to use with it.)
> 
> Yeah, most of the OMAP clocks have been traditionally referenced in
> this way, they are using clk_get_sys and with dev=NULL. conn-id is
> mostly unique.
> 
> >
> >If you have an OF node, what's wrong with using of_clk_get_by_name()?
> >If you don't have an OF node, how do you expect to find the clock in
> >the device tree, remembering that clocks are specific to devices, and
> >the 2nd argument to clk_get()/clk_get_sys() is not a unique identifier.
> 
> In the legacy code, pretty much all the clocks are mapped through
> clk_get_sys(NULL, conn), and large amount of the drivers don't even
> have DT conversion in place yet. I am attempting to handle the cases
> where we don't have OF nodes.... yet at least, without converting
> everything to DT, and without causing regressions (also, I don't
> even pretend to be capable of testing all the potential drivers for
> this.) Some parts of the basic infra which are using clk_get_sys are
> pretty silly also, like the ocp_if parts of the hwmod, I haven't
> figured out yet how to convert that to DT.
> 
> Personally I don't mind causing a regressions here though, if I get
> approval for it from Tony.

No, let's not do that. We don't want to break existing drivers.

How about initialize the platform driver clocks in the clock driver
and set up the clock alias? Then when the driver is fixed for DT,
those can be dropped one at a time.

Regards,

Tony