diff mbox

[PATCHv4,05/33] CLK: omap: add DT duplicate clock registration mechanism

Message ID 51FA6FCA.3050900@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nishanth Menon Aug. 1, 2013, 2:25 p.m. UTC
On 07/31/2013 05:07 AM, Tero Kristo wrote:
> On 07/30/2013 09:40 PM, Nishanth Menon wrote:
>> On 07/23/2013 02:20 AM, Tero Kristo wrote:
>>> Some devices require their clocks to be available with a specific
>>> dev-id con-id mapping. With DT, the clocks can be found by default
>>> only with their name, or alternatively through the device node of
>>> the consumer. With drivers, that don't support DT fully yet, add
>>> mechanism to register specific clock names.
>>>
>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>
>> with this, should it not be enough to add clocks=<&phandle>
>
> Don't know, I am not an expert on DT. :)

That is the usage - 
Documentation/devicetree/bindings/clock/clock-bindings.txt

>
>>
>> I am not sure I understand what specific drivers should need to carry
>> this "old hack" forward. More importantly, why is it preferable to carry
>> the hack forward rather than fixing the drivers.
>
> At least the GP timer seems to need this, and I don't want to touch /
> verify all the potential drivers touched by this so it is easier to
> provide a (semi) temporary tweak.

I see that GP timer nodes seem to be already device tree converted, at 
least I see the nodes in SoC.dtsi

Do we know what is going on for these that need these temporary devices? 
can we do a special node property for these?

I think the entire problem is coz of
timer->fclk = clk_get(NULL, omap_hwmod_get_main_clk(oh)); in case of 
even of_populated.

if we can get rid of usage of omap_hwmod_get_main_clk by catching them 
with [1], then we can force the drivers to pick up based on device node 
clocks= property.

It might be easier to fix 1 driver - timer, rather than introduce am33x, 
omap4, omap5 dra7 specific "SoC clk driver".

with that this entire patch becomes redundant.


[1]
  }

Comments

Tero Kristo Aug. 1, 2013, 3:18 p.m. UTC | #1
On 08/01/2013 05:25 PM, Nishanth Menon wrote:
> On 07/31/2013 05:07 AM, Tero Kristo wrote:
>> On 07/30/2013 09:40 PM, Nishanth Menon wrote:
>>> On 07/23/2013 02:20 AM, Tero Kristo wrote:
>>>> Some devices require their clocks to be available with a specific
>>>> dev-id con-id mapping. With DT, the clocks can be found by default
>>>> only with their name, or alternatively through the device node of
>>>> the consumer. With drivers, that don't support DT fully yet, add
>>>> mechanism to register specific clock names.
>>>>
>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>>
>>> with this, should it not be enough to add clocks=<&phandle>
>>
>> Don't know, I am not an expert on DT. :)
>
> That is the usage -
> Documentation/devicetree/bindings/clock/clock-bindings.txt
>
>>
>>>
>>> I am not sure I understand what specific drivers should need to carry
>>> this "old hack" forward. More importantly, why is it preferable to carry
>>> the hack forward rather than fixing the drivers.
>>
>> At least the GP timer seems to need this, and I don't want to touch /
>> verify all the potential drivers touched by this so it is easier to
>> provide a (semi) temporary tweak.
>
> I see that GP timer nodes seem to be already device tree converted, at
> least I see the nodes in SoC.dtsi

Even if those exist, they don't seem to work. The kernel crashes without 
the alias nodes as of now.

>
> Do we know what is going on for these that need these temporary devices?
> can we do a special node property for these?
>
> I think the entire problem is coz of
> timer->fclk = clk_get(NULL, omap_hwmod_get_main_clk(oh)); in case of
> even of_populated.

I guess so, clk_get tries to look for improper clk which does not exist. 
Might be a bug with hwmod data.

>
> if we can get rid of usage of omap_hwmod_get_main_clk by catching them
> with [1], then we can force the drivers to pick up based on device node
> clocks= property.
>
> It might be easier to fix 1 driver - timer, rather than introduce am33x,
> omap4, omap5 dra7 specific "SoC clk driver".
>
> with that this entire patch becomes redundant.

It is not that simple. Looking at the architectures this set supports, I 
see clock alias nodes at least for following drivers:

- GPT timer
- USB
- DCAN
- EMAC
- VPFE
- UART
- SSI
- DSS
- security
- MMC
- MCBSP
- MCSPI

I am _not_ going to fix all of these during the initial phase. :P

But yes, eventually these should go away.

>
>
> [1]
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c
> b/arch/arm/mach-omap2/omap_hwmod.c
> index da26659..2e90ab4 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -4153,5 +4153,10 @@ const char *omap_hwmod_get_main_clk(struct
> omap_hwmod *oh)
>       if (!oh)
>           return NULL;
>
> +    if (!of_have_populated_dt()) {
> +        WARN(1, "%s hwmod clk node read with OF?:FIXME!\n",
> +             oh->name);
> +    }
> +
>       return oh->main_clk;
>   }

--
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
Nishanth Menon Aug. 1, 2013, 3:24 p.m. UTC | #2
On 08/01/2013 10:18 AM, Tero Kristo wrote:
> On 08/01/2013 05:25 PM, Nishanth Menon wrote:
>> On 07/31/2013 05:07 AM, Tero Kristo wrote:
>>> On 07/30/2013 09:40 PM, Nishanth Menon wrote:
>>>> On 07/23/2013 02:20 AM, Tero Kristo wrote:
[..]
>> if we can get rid of usage of omap_hwmod_get_main_clk by catching them
>> with [1], then we can force the drivers to pick up based on device node
>> clocks= property.
>>
>> It might be easier to fix 1 driver - timer, rather than introduce am33x,
>> omap4, omap5 dra7 specific "SoC clk driver".
>>
>> with that this entire patch becomes redundant.
>
> It is not that simple. Looking at the architectures this set supports, I
> see clock alias nodes at least for following drivers:
>
> - GPT timer
> - USB
> - DCAN
> - EMAC
> - VPFE
> - UART
> - SSI
> - DSS
> - security
> - MMC
> - MCBSP
> - MCSPI
>
> I am _not_ going to fix all of these during the initial phase. :P
>
> But yes, eventually these should go away.

How many of these are needed to boot? what functionality do we expect 
with the series -> we can constraint saying that remaining drivers 
should fix themselves the right way, else dont have the feature - 
example cpufreq- fix it the right way, or wont see the feature enabled.

introducing a "way out" for all of these just invites more guys to screw 
around claiming "it was done before - see here"..

lets just fix the darned basic ones, refuse to provide "way out" and let 
the others fix themselves.
Tero Kristo Aug. 1, 2013, 3:30 p.m. UTC | #3
On 08/01/2013 06:24 PM, Nishanth Menon wrote:
> On 08/01/2013 10:18 AM, Tero Kristo wrote:
>> On 08/01/2013 05:25 PM, Nishanth Menon wrote:
>>> On 07/31/2013 05:07 AM, Tero Kristo wrote:
>>>> On 07/30/2013 09:40 PM, Nishanth Menon wrote:
>>>>> On 07/23/2013 02:20 AM, Tero Kristo wrote:
> [..]
>>> if we can get rid of usage of omap_hwmod_get_main_clk by catching them
>>> with [1], then we can force the drivers to pick up based on device node
>>> clocks= property.
>>>
>>> It might be easier to fix 1 driver - timer, rather than introduce am33x,
>>> omap4, omap5 dra7 specific "SoC clk driver".
>>>
>>> with that this entire patch becomes redundant.
>>
>> It is not that simple. Looking at the architectures this set supports, I
>> see clock alias nodes at least for following drivers:
>>
>> - GPT timer
>> - USB
>> - DCAN
>> - EMAC
>> - VPFE
>> - UART
>> - SSI
>> - DSS
>> - security
>> - MMC
>> - MCBSP
>> - MCSPI
>>
>> I am _not_ going to fix all of these during the initial phase. :P
>>
>> But yes, eventually these should go away.
>
> How many of these are needed to boot? what functionality do we expect
> with the series -> we can constraint saying that remaining drivers
> should fix themselves the right way, else dont have the feature -
> example cpufreq- fix it the right way, or wont see the feature enabled.
>
> introducing a "way out" for all of these just invites more guys to screw
> around claiming "it was done before - see here"..
>
> lets just fix the darned basic ones, refuse to provide "way out" and let
> the others fix themselves.
>

Yea, that would be one way. I guess someone like Tony should comment on 
this.

--
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. 2, 2013, 7:22 a.m. UTC | #4
* Tero Kristo <t-kristo@ti.com> [130801 08:37]:
> On 08/01/2013 06:24 PM, Nishanth Menon wrote:
> >On 08/01/2013 10:18 AM, Tero Kristo wrote:
> >>On 08/01/2013 05:25 PM, Nishanth Menon wrote:
> >>>On 07/31/2013 05:07 AM, Tero Kristo wrote:
> >>>>On 07/30/2013 09:40 PM, Nishanth Menon wrote:
> >>>>>On 07/23/2013 02:20 AM, Tero Kristo wrote:
> >[..]
> >>>if we can get rid of usage of omap_hwmod_get_main_clk by catching them
> >>>with [1], then we can force the drivers to pick up based on device node
> >>>clocks= property.
> >>>
> >>>It might be easier to fix 1 driver - timer, rather than introduce am33x,
> >>>omap4, omap5 dra7 specific "SoC clk driver".
> >>>
> >>>with that this entire patch becomes redundant.
> >>
> >>It is not that simple. Looking at the architectures this set supports, I
> >>see clock alias nodes at least for following drivers:
> >>
> >>- GPT timer
> >>- USB
> >>- DCAN
> >>- EMAC
> >>- VPFE
> >>- UART
> >>- SSI
> >>- DSS
> >>- security
> >>- MMC
> >>- MCBSP
> >>- MCSPI
> >>
> >>I am _not_ going to fix all of these during the initial phase. :P
> >>
> >>But yes, eventually these should go away.
> >
> >How many of these are needed to boot? what functionality do we expect
> >with the series -> we can constraint saying that remaining drivers
> >should fix themselves the right way, else dont have the feature -
> >example cpufreq- fix it the right way, or wont see the feature enabled.
> >
> >introducing a "way out" for all of these just invites more guys to screw
> >around claiming "it was done before - see here"..
> >
> >lets just fix the darned basic ones, refuse to provide "way out" and let
> >the others fix themselves.
> >
> 
> Yea, that would be one way. I guess someone like Tony should comment
> on this.

Well how about add some dev_warns so we can keep things working and
know which ones to fix? Otherwise it seems that things will not work
properly for many devices.

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/arch/arm/mach-omap2/omap_hwmod.c 
b/arch/arm/mach-omap2/omap_hwmod.c
index da26659..2e90ab4 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -4153,5 +4153,10 @@  const char *omap_hwmod_get_main_clk(struct 
omap_hwmod *oh)
  	if (!oh)
  		return NULL;

+	if (!of_have_populated_dt()) {
+		WARN(1, "%s hwmod clk node read with OF?:FIXME!\n",
+		     oh->name);
+	}
+
  	return oh->main_clk;