Re: [PATCH 2/3] ARM: OMAP2+: Add support to parse optional clk info from DT

[Rajendra Nayak <rnayak@ti.com>]

On Wednesday 14 August 2013 07:43 PM, Mark Rutland wrote:
> On Wed, Aug 14, 2013 at 03:05:25PM +0100, Rajendra Nayak wrote:
>> On Wednesday 14 August 2013 07:28 PM, Nishanth Menon wrote:
>>> On 08/14/2013 08:49 AM, Mark Rutland wrote:
>>>> [Adding Mike Turquette and dt maintainers]
>>>>
>>>> On Wed, Aug 14, 2013 at 02:39:44PM +0100, Nishanth Menon wrote:
>>>>> On 08/14/2013 08:20 AM, Rajendra Nayak wrote:
>>>>>> On Wednesday 14 August 2013 06:18 PM, Nishanth Menon wrote:
>>>>>>> Hi Rajendra,
>>>>>>>
>>>>>>> On Tue, Jul 23, 2013 at 1:24 AM, Rajendra Nayak <rnayak@ti.com> wrote:
>>>>>>> [..]
>>>>>>>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
>>>>>>>> index 12fa589..e5c804b 100644
>>>>>>>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>>>>>>>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>>>>>>>> @@ -805,6 +805,65 @@ static int _init_interface_clks(struct omap_hwmod *oh)
>>>>>>>>           return ret;
>>>>>>>>    }
>>>>>>>>
>>>>>>>> +static const char **_parse_opt_clks_dt(struct omap_hwmod *oh,
>>>>>>>> +                                      struct device_node *np,
>>>>>>>> +                                      int *opt_clks_cnt)
>>>>>>>> +{
>>>>>>>> +       int i, clks_cnt;
>>>>>>>> +       const char *clk_name;
>>>>>>>> +       const char **opt_clk_names;
>>>>>>>> +
>>>>>>>> +       clks_cnt = of_property_count_strings(np, "clock-names");
>>>>>>>> +       if (!clks_cnt)
>>>>>>>> +               return NULL;
>>>>>>>> +
>>>>>>>> +       opt_clk_names = kzalloc(sizeof(char *)*clks_cnt, GFP_KERNEL);
>>>>>>>> +       if (!opt_clk_names)
>>>>>>>> +               return NULL;
>>>>>>>> +
>>>>>>>> +       for (i = 0; i < clks_cnt; i++) {
>>>>>>>> +               of_property_read_string_index(np, "clock-names", i, &clk_name);
>>>>>>>> +               if (!strcmp(clk_name, "fck"))
>>>>>>>
>>>>>>> Could we instead parse for names that are "optional,role_name" instead
>>>>>>> of assuming anything other than fck is optional clocks?
>>>>>>
>>>>>> you mean look for anything with optional,*? because the role names would change.
>>>>>>
>>>>>
>>>>> yes. the idea being, we now have a meaning to the clock name - there are
>>>>> two types of clocks here.. functional and optional, we *might* have
>>>>> facility to add interface clock(we dont know interface clock handling
>>>>> yet, but something in the future).. we might increase the support for
>>>>> number of functional clocks.. it might help to keep the format such that
>>>>> it is a "bit extendable".
>>>>
>>>> I completely disagree. The only things that should appear in clock-names
>>>> are the names of the clock inputs that appear in the manual for the
>>>> device. The driver should know which ones are optional, as that's a
>>>> fixed property of the IP and the way the driver uses it.
>>>>
>>>> You should not be embedding additional semantics in name properties.
>>>
>>> we use an level of abstraction called omap_device and hwmod to allow devices to use a generic pm_runtime. drivers for specific blocks dont normally need to know about the clocks to deal with. This allows maximum reuse consider concept is generic enough.
>>
>> They do know about the optional clocks though and request and release them when needed. The need for hwmod to know about optional clocks
>> (and enable all) arises from the fact that some of these devices need *some* optional clocks for a successful reset.
>> And given hmwod has no knowledge about which optional ones (if at all) will be needed, it goes ahead and enables all before doing a reset.
>> This is something done only at init time and *not* something thats done every time the device is enabled by the driver using pm_runtime.
> 
> To clarify:
> 
> I was initially confused as to the purpose of the code. I'm not against
> a one-off clock initialisation to put everything into a sane state. If
> we can't trust the bootloaders, that seems like a necessary evil. I'll
> leave Mike to comment on whether and how that should be done.
> 
> I do not think we should be embedding clock semantics in clock-names.
> That's not the way the property is intended to be used, it breaks
> uniformity, and it's an abuse of the system that may come back to bite
> us later.

Mark, that makes sense.

Nishanth, thinking some more of this, the 'optional,role-name' also won't work
for the simple reason that drivers who do clk_get(node, 'role-name') would
then simply fail.

So I guess we need to figure out a better way to handle this.

> 
> Thanks,
> Mark.
>