diff mbox

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

Message ID 1375460751-23676-2-git-send-email-t-kristo@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tero Kristo Aug. 2, 2013, 4:25 p.m. UTC
clk_get_sys / clk_get can now find clocks from device-tree. If a DT clock
is found, an entry is added to the clk_lookup list also for subsequent
searches.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Russell King <linux@arm.linux.org.uk>
---
 drivers/clk/clkdev.c |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Tomasz Figa Aug. 3, 2013, 2:02 p.m. UTC | #1
Hi Tero,

On Friday 02 of August 2013 19:25:20 Tero Kristo wrote:
> clk_get_sys / clk_get can now find clocks from device-tree. If a DT
> clock is found, an entry is added to the clk_lookup list also for
> subsequent searches.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> ---
>  drivers/clk/clkdev.c |   33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 442a313..cbeb252 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -93,6 +93,18 @@ struct clk *of_clk_get_by_name(struct device_node
> *np, const char *name) EXPORT_SYMBOL(of_clk_get_by_name);
>  #endif
> 
> +/**
> + * clkdev_add_nolock - add lookup entry for a clock
> + * @cl: pointer to new clock lookup entry
> + *
> + * Non-locking version, used internally by clk_find() to add DT based
> + * clock lookup entries.
> + */
> +static void clkdev_add_nolock(struct clk_lookup *cl)
> +{
> +	list_add_tail(&cl->node, &clocks);
> +}
> +
>  /*
>   * Find the correct struct clk for the device and connection ID.
>   * We do slightly fuzzy matching here:
> @@ -106,6 +118,9 @@ static struct clk_lookup *clk_find(const char
> *dev_id, const char *con_id) {
>  	struct clk_lookup *p, *cl = NULL;
>  	int match, best_found = 0, best_possible = 0;
> +	struct device_node *node;
> +	struct clk *clk;
> +	struct of_phandle_args clkspec;
> 
>  	if (dev_id)
>  		best_possible += 2;
> @@ -133,6 +148,24 @@ static struct clk_lookup *clk_find(const char
> *dev_id, const char *con_id) break;
>  		}
>  	}
> +
> +	if (cl)
> +		return cl;
> +
> +	/* If clock was not found, attempt to look-up from DT */
> +	node = of_find_node_by_name(NULL, con_id);

Why are we introducing the "lookup by name" brokenness to the yet (mostly) 
sane DT world?

We already have a good way of binding things together in DT, which is 
using phandles.

Not even saying that this (or something this patch relies on) breaks the 
ePAPR recommendation about node naming, which states that node names 
should not be used to convey platform-specific data, but instead should be 
as generic as possible to show what kind of hardware is represented by the 
node.

Best regards,
Tomasz

P.S. Added missing DT maintainers to CC.

> +	clkspec.np = node;
> +
> +	clk = of_clk_get_from_provider(&clkspec);
> +
> +	if (!IS_ERR(clk)) {
> +		/* We found a clock, add node to clkdev */
> +		cl = clkdev_alloc(clk, con_id, dev_id);
> +		if (cl)
> +			clkdev_add_nolock(cl);
> +	}
> +
>  	return cl;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Aug. 3, 2013, 6:31 p.m. UTC | #2
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Aug. 3, 2013, 6:35 p.m. UTC | #3
On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa 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);
> 
> Why are we introducing the "lookup by name" brokenness to the yet (mostly) 
> sane DT world?
> 
> We already have a good way of binding things together in DT, which is 
> using phandles.
> 
> Not even saying that this (or something this patch relies on) breaks the 
> ePAPR recommendation about node naming, which states that node names 
> should not be used to convey platform-specific data, but instead should be 
> as generic as possible to show what kind of hardware is represented by the 
> node.

Tell me this: many devices name their clock inputs.  You can find these
input names in data sheets and the like.  These are the names defined
by the hardware.  What is wrong about using those names in DT?

Remember that the second argument to clk_get() is the _clock_ _input_
_name_ on the _device_ passed in the first argument.  It is not *ever*
some random name of the clock producer unless someone's fscked up their
use of this API (in which case they're the ones with the problem.)
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Aug. 3, 2013, 6:39 p.m. UTC | #4
Hi Russell,

On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote:
> On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa 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);
> > 
> > Why are we introducing the "lookup by name" brokenness to the yet
> > (mostly) sane DT world?
> > 
> > We already have a good way of binding things together in DT, which is
> > using phandles.
> > 
> > Not even saying that this (or something this patch relies on) breaks
> > the ePAPR recommendation about node naming, which states that node
> > names should not be used to convey platform-specific data, but
> > instead should be as generic as possible to show what kind of
> > hardware is represented by the node.
> 
> Tell me this: many devices name their clock inputs.  You can find these
> input names in data sheets and the like.  These are the names defined
> by the hardware.  What is wrong about using those names in DT?

Yes, clock inputs. But this is about lookup by clock name, not clock input 
name, as far as I can tell from the patch, based on looking for a node 
with given name over the whole DT.

> Remember that the second argument to clk_get() is the _clock_ _input_
> _name_ on the _device_ passed in the first argument.  It is not *ever*
> some random name of the clock producer unless someone's fscked up their
> use of this API (in which case they're the ones with the problem.)

This is perfectly fine and this is how the generic common clock framework 
DT bindings work - clock-names property is a list of clock input names and 
clocks property contain specifiers of respective clocks.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Aug. 3, 2013, 6:48 p.m. UTC | #5
On Sat, Aug 03, 2013 at 08:39:34PM +0200, Tomasz Figa wrote:
> Hi Russell,
> 
> On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote:
> > On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa 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);
> > > 
> > > Why are we introducing the "lookup by name" brokenness to the yet
> > > (mostly) sane DT world?
> > > 
> > > We already have a good way of binding things together in DT, which is
> > > using phandles.
> > > 
> > > Not even saying that this (or something this patch relies on) breaks
> > > the ePAPR recommendation about node naming, which states that node
> > > names should not be used to convey platform-specific data, but
> > > instead should be as generic as possible to show what kind of
> > > hardware is represented by the node.
> > 
> > Tell me this: many devices name their clock inputs.  You can find these
> > input names in data sheets and the like.  These are the names defined
> > by the hardware.  What is wrong about using those names in DT?
> 
> Yes, clock inputs. But this is about lookup by clock name, not clock input 
> name, as far as I can tell from the patch, based on looking for a node 
> with given name over the whole DT.

"con_id" is supposed to be the clock input name when the clock API is
used correctly.

> > Remember that the second argument to clk_get() is the _clock_ _input_
> > _name_ on the _device_ passed in the first argument.  It is not *ever*
> > some random name of the clock producer unless someone's fscked up their
> > use of this API (in which case they're the ones with the problem.)
> 
> This is perfectly fine and this is how the generic common clock framework 
> DT bindings work - clock-names property is a list of clock input names and 
> clocks property contain specifiers of respective clocks.

There seems to be a some pressure to do clk_get()->of_clk_get()
conversions, and also to find ways to lookup clocks.  It seems to me
that no one understands how DT handles clocks.  Maybe that's where the
problem is - lack of documentation about how DT clocks are handled.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Aug. 3, 2013, 7:04 p.m. UTC | #6
On Saturday 03 of August 2013 19:48:05 Russell King - ARM Linux wrote:
> On Sat, Aug 03, 2013 at 08:39:34PM +0200, Tomasz Figa wrote:
> > Hi Russell,
> > 
> > On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote:
> > > On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa 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);
> > > > 
> > > > Why are we introducing the "lookup by name" brokenness to the yet
> > > > (mostly) sane DT world?
> > > > 
> > > > We already have a good way of binding things together in DT, which
> > > > is
> > > > using phandles.
> > > > 
> > > > Not even saying that this (or something this patch relies on)
> > > > breaks
> > > > the ePAPR recommendation about node naming, which states that node
> > > > names should not be used to convey platform-specific data, but
> > > > instead should be as generic as possible to show what kind of
> > > > hardware is represented by the node.
> > > 
> > > Tell me this: many devices name their clock inputs.  You can find
> > > these
> > > input names in data sheets and the like.  These are the names
> > > defined
> > > by the hardware.  What is wrong about using those names in DT?
> > 
> > Yes, clock inputs. But this is about lookup by clock name, not clock
> > input name, as far as I can tell from the patch, based on looking for
> > a node with given name over the whole DT.
> 
> "con_id" is supposed to be the clock input name when the clock API is
> used correctly.

Right. Still, the code is looking for a node with the same name as the 
supposed input name, which seems strange to me, because clock input names 
are supposed to be defined in consumer's node, inside clock-names 
property.

> > > Remember that the second argument to clk_get() is the _clock_
> > > _input_
> > > _name_ on the _device_ passed in the first argument.  It is not
> > > *ever*
> > > some random name of the clock producer unless someone's fscked up
> > > their
> > > use of this API (in which case they're the ones with the problem.)
> > 
> > This is perfectly fine and this is how the generic common clock
> > framework DT bindings work - clock-names property is a list of clock
> > input names and clocks property contain specifiers of respective
> > clocks.
> 
> There seems to be a some pressure to do clk_get()->of_clk_get()
> conversions, and also to find ways to lookup clocks.

I don't really see any need for this kind of conversion. clk_get() already 
handles lookup using DT correctly and from drivers perspective nothing 
changes.

> It seems to me
> that no one understands how DT handles clocks.  Maybe that's where the
> problem is - lack of documentation about how DT clocks are handled.

Hmm, there is pretty much of description and examples in

Documentation/devicetree/bindings/clock/clock-bindings.txt

For me it was enough to learn how DT based clock lookup works, but I'm 
quite used to DT, so I'm not a good test sample.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tero Kristo Aug. 19, 2013, 9:12 a.m. UTC | #7
Hey,

(Sorry was out for a short vacation, thus late reply.)

On 08/03/2013 10:04 PM, Tomasz Figa wrote:
> On Saturday 03 of August 2013 19:48:05 Russell King - ARM Linux wrote:
>> On Sat, Aug 03, 2013 at 08:39:34PM +0200, Tomasz Figa wrote:
>>> Hi Russell,
>>>
>>> On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote:
>>>> On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa 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);
>>>>>
>>>>> Why are we introducing the "lookup by name" brokenness to the yet
>>>>> (mostly) sane DT world?
>>>>>
>>>>> We already have a good way of binding things together in DT, which
>>>>> is
>>>>> using phandles.
>>>>>
>>>>> Not even saying that this (or something this patch relies on)
>>>>> breaks
>>>>> the ePAPR recommendation about node naming, which states that node
>>>>> names should not be used to convey platform-specific data, but
>>>>> instead should be as generic as possible to show what kind of
>>>>> hardware is represented by the node.
>>>>
>>>> Tell me this: many devices name their clock inputs.  You can find
>>>> these
>>>> input names in data sheets and the like.  These are the names
>>>> defined
>>>> by the hardware.  What is wrong about using those names in DT?
>>>
>>> Yes, clock inputs. But this is about lookup by clock name, not clock
>>> input name, as far as I can tell from the patch, based on looking for
>>> a node with given name over the whole DT.
>>
>> "con_id" is supposed to be the clock input name when the clock API is
>> used correctly.
>
> Right. Still, the code is looking for a node with the same name as the
> supposed input name, which seems strange to me, because clock input names
> are supposed to be defined in consumer's node, inside clock-names
> property.
>
>>>> Remember that the second argument to clk_get() is the _clock_
>>>> _input_
>>>> _name_ on the _device_ passed in the first argument.  It is not
>>>> *ever*
>>>> some random name of the clock producer unless someone's fscked up
>>>> their
>>>> use of this API (in which case they're the ones with the problem.)
>>>
>>> This is perfectly fine and this is how the generic common clock
>>> framework DT bindings work - clock-names property is a list of clock
>>> input names and clocks property contain specifiers of respective
>>> clocks.
>>
>> There seems to be a some pressure to do clk_get()->of_clk_get()
>> conversions, and also to find ways to lookup clocks.
>
> I don't really see any need for this kind of conversion. clk_get() already
> handles lookup using DT correctly and from drivers perspective nothing
> changes.
>
>> It seems to me
>> that no one understands how DT handles clocks.  Maybe that's where the
>> problem is - lack of documentation about how DT clocks are handled.
>
> Hmm, there is pretty much of description and examples in
>
> Documentation/devicetree/bindings/clock/clock-bindings.txt
>
> For me it was enough to learn how DT based clock lookup works, but I'm
> quite used to DT, so I'm not a good test sample.
>
> Best regards,
> Tomasz
>

I think based on these comments this patch needs to go away, I will have 
to find another way to map the devices in and to avoid supporting the 
legacy mappings (hwmod depends on this heavily atm, but Rajendra posted 
some patches to address this.) I'll see what I can do for next rev. I 
might implement some tweak inside the OMAP codebase for this.

-Tero

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tero Kristo Aug. 26, 2013, 2:36 p.m. UTC | #8
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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Aug. 26, 2013, 5:03 p.m. UTC | #9
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tero Kristo Aug. 26, 2013, 6:12 p.m. UTC | #10
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren Aug. 27, 2013, 6:55 a.m. UTC | #11
* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 442a313..cbeb252 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -93,6 +93,18 @@  struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
 EXPORT_SYMBOL(of_clk_get_by_name);
 #endif
 
+/**
+ * clkdev_add_nolock - add lookup entry for a clock
+ * @cl: pointer to new clock lookup entry
+ *
+ * Non-locking version, used internally by clk_find() to add DT based
+ * clock lookup entries.
+ */
+static void clkdev_add_nolock(struct clk_lookup *cl)
+{
+	list_add_tail(&cl->node, &clocks);
+}
+
 /*
  * Find the correct struct clk for the device and connection ID.
  * We do slightly fuzzy matching here:
@@ -106,6 +118,9 @@  static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
 {
 	struct clk_lookup *p, *cl = NULL;
 	int match, best_found = 0, best_possible = 0;
+	struct device_node *node;
+	struct clk *clk;
+	struct of_phandle_args clkspec;
 
 	if (dev_id)
 		best_possible += 2;
@@ -133,6 +148,24 @@  static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
 				break;
 		}
 	}
+
+	if (cl)
+		return cl;
+
+	/* If clock was not found, attempt to look-up from DT */
+	node = of_find_node_by_name(NULL, con_id);
+
+	clkspec.np = node;
+
+	clk = of_clk_get_from_provider(&clkspec);
+
+	if (!IS_ERR(clk)) {
+		/* We found a clock, add node to clkdev */
+		cl = clkdev_alloc(clk, con_id, dev_id);
+		if (cl)
+			clkdev_add_nolock(cl);
+	}
+
 	return cl;
 }