From: nm@ti•com (Nishanth Menon)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 2/3] ARM: OMAP2+: Add support to parse optional clk info from DT
Date: Wed, 14 Aug 2013 09:41:42 -0500 [thread overview]
Message-ID: <520B9726.4030505@ti.com> (raw)
In-Reply-To: <520B9236.2090205@ti.com>
On 08/14/2013 09:20 AM, Rajendra Nayak wrote:
> 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.
you are right :( we do have types of clock inputs to a device and
therein lies our problem :(
--
Regards,
Nishanth Menon
next prev parent reply other threads:[~2013-08-14 14:41 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-23 6:24 [PATCH 0/3] OMAP2+: hwmod: Add support to parse clock info from DT Rajendra Nayak
2013-07-23 6:24 ` [PATCH 1/3] ARM: OMAP2+: Add support to parse 'main_clk' " Rajendra Nayak
2013-08-14 12:50 ` menon.nishanth at gmail.com
2013-07-23 6:24 ` [PATCH 2/3] ARM: OMAP2+: Add support to parse optional clk " Rajendra Nayak
2013-08-14 12:48 ` Nishanth Menon
2013-08-14 13:20 ` Rajendra Nayak
2013-08-14 13:39 ` Nishanth Menon
2013-08-14 13:41 ` Rajendra Nayak
2013-08-14 13:49 ` Mark Rutland
2013-08-14 13:57 ` Russell King - ARM Linux
2013-08-14 13:58 ` Nishanth Menon
2013-08-14 14:05 ` Rajendra Nayak
2013-08-14 14:08 ` Rajendra Nayak
2013-08-14 14:13 ` Mark Rutland
2013-08-14 14:20 ` Rajendra Nayak
2013-08-14 14:41 ` Nishanth Menon [this message]
2013-08-14 14:08 ` Mark Rutland
2013-08-14 14:13 ` Rajendra Nayak
2013-08-14 13:45 ` Mark Rutland
2013-08-14 13:54 ` Rajendra Nayak
2013-08-14 13:59 ` Mark Rutland
2013-07-23 6:24 ` [PATCH 3/3] ARM: OMAP4: dts: Add main and optional clock data into DT Rajendra Nayak
2013-08-20 23:57 ` Paul Walmsley
2013-08-21 8:28 ` Rajendra Nayak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=520B9726.4030505@ti.com \
--to=nm@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox