ARM: OMAP3+: am33xx: Add CM, clock and clockdomain support

AM33XX PRCM module consist of, various clockdomains, in all total we have 18 clockdomains available, with following controlling options,
- NO Sleep: sleep transition can not be initiated,
- SW Sleep: sw forced sleep transition,
- SW Wakeup: sw forced wakeup transition,
- HW Auto: transitions are based upon hw conditions.

This patch adds all available clockdomain data, respective clockdomain operations for AM33XX family of device, and also integrates it into existing OMAP framework.

Signed-off-by Vaibhav Hiremath <[email protected]>

Signed-off-by Afzal Mohammed <[email protected]>

Signed-off-by Vaibhav Bedia <[email protected]>

Hello Vaibhav, Afzal, Vaibhav,

A few questions while reviewing this patch:

Quoted 15 lines · Vaibhav Hiremath

AM33XX PRCM module consist of, various clockdomains, in all total we have 18 clockdomains available, with following controlling options,
- NO Sleep: sleep transition can not be initiated,
- SW Sleep: sw forced sleep transition,
- SW Wakeup: sw forced wakeup transition,
- HW Auto: transitions are based upon hw conditions.

This patch adds all available clockdomain data, respective clockdomain operations for AM33XX family of device, and also integrates it into existing OMAP framework.

Signed-off-by Vaibhav Hiremath <[email protected]>

Signed-off-by Afzal Mohammed <[email protected]>

Signed-off-by Vaibhav Bedia <[email protected]>

...

arch/arm/mach-omap2/clockdomains33xx_data.c line 31

Quoted 7 lines

arch/arm/mach-omap2/clockdomains33xx_data.c new file +7 -0

+static struct clockdomain l4ls_am33xx_clkdm = {
+	.name		= "l4ls_clkdm",
+	.pwrdm		= { .name = "per_pwrdm" },
+	.cm_inst	= AM33XX_CM_PER_MOD,
+	.clkdm_offs	= AM33XX_CM_PER_L4LS_CLKSTCTRL_OFFSET,
+	.clktrctrl_mask	= AM33XX_CLKTRCTRL_MASK,
+	.flags		= (CLKDM_CAN_SWSUP | CLKDM_NO_AUTODEPS),

It looks to me like we don't need the .clktrctrl_mask field, since according to the clockdomains code, the CLKTRCTRL field is at the same bit shift for each clockdomain.

Also, since we're not defining any autodeps for the AM335x platform, we shouldn't need the CLKDM_NO_AUTODEPS flag either? Since no autodeps are defined, the default will be to not set any autodeps.

Another question is about the CLKTRCTRL bitfields. According to _AM335x ARM Cortex-A8 Microprocessors (MPUs) Technical Reference Manual_ Rev. D (SPRUH73D), most of the clockdomains support NO_SLEEP mode (i.e., CLKTRCTRL = 0x0). That means that technically, we should also set the CLKDM_CAN_DISABLE_AUTO flag. Unless the documentation is incorrect here? In another section (Table 8-9 "Clock Transition Mode Settings"), it claims that CLKTRCTRL = 0 is not supported...

Also, a question about the L4_WKUP clockdomain:

arch/arm/mach-omap2/clockdomains33xx_data.c line 122

Quoted 8 lines

arch/arm/mach-omap2/clockdomains33xx_data.c new file +8 -0

+static struct clockdomain l4_wkup_am33xx_clkdm = {
+	.name		= "l4_wkup_clkdm",
+	.pwrdm		= { .name = "wkup_pwrdm" },
+	.cm_inst	= AM33XX_CM_WKUP_MOD,
+	.clkdm_offs	= AM33XX_CM_WKUP_CLKSTCTRL_OFFSET,
+	.clktrctrl_mask	= AM33XX_CLKTRCTRL_MASK,
+	.flags		= (CLKDM_CAN_SWSUP | CLKDM_NO_AUTODEPS),
+};

Table 8-89 "CM_WKUP_CLKSTCTRL Register Field Descriptions" of SPRUH73D claims that this clockdomain supports hardware-supervised automatic clockdomain transitions. Again this conflicts with Table 8-9. Is this a documentation bug, or should we update the patch?

- Paul

Quoted 22 lines · Paul Walmsley

A few questions while reviewing this patch:

Quoted 15 lines · Vaibhav Hiremath

AM33XX PRCM module consist of, various clockdomains, in all total we have 18 clockdomains available, with following controlling options,
- NO Sleep: sleep transition can not be initiated,
- SW Sleep: sw forced sleep transition,
- SW Wakeup: sw forced wakeup transition,
- HW Auto: transitions are based upon hw conditions.

This patch adds all available clockdomain data, respective clockdomain operations for AM33XX family of device, and also integrates it into existing OMAP framework.

Signed-off-by Vaibhav Hiremath <[email protected]>

Signed-off-by Afzal Mohammed <[email protected]>

Signed-off-by Vaibhav Bedia <[email protected]>

...

Quoted 12 lines

arch/arm/mach-omap2/clockdomains33xx_data.c line 31

Quoted 7 lines

arch/arm/mach-omap2/clockdomains33xx_data.c new file +7 -0

+static struct clockdomain l4ls_am33xx_clkdm = {
+	.name		= "l4ls_clkdm",
+	.pwrdm		= { .name = "per_pwrdm" },
+	.cm_inst	= AM33XX_CM_PER_MOD,
+	.clkdm_offs	= AM33XX_CM_PER_L4LS_CLKSTCTRL_OFFSET,
+	.clktrctrl_mask	= AM33XX_CLKTRCTRL_MASK,
+	.flags		= (CLKDM_CAN_SWSUP | CLKDM_NO_AUTODEPS),

It looks to me like we don't need the .clktrctrl_mask field, since according to the clockdomains code, the CLKTRCTRL field is at the same bit shift for each clockdomain.

Yeah, we actually don't need it, probably I added for completeness. I will remove it in next version.

Quoted 4 lines

Also, since we're not defining any autodeps for the AM335x platform, we shouldn't need the CLKDM_NO_AUTODEPS flag either? Since no autodeps are defined, the default will be to not set any autodeps.

This is required to avoid few "pr_debug", if you don't provide it. For example, without this flag set, you will get,

pr_debug("clockdomain: hardware cannot set/clear sleep "

		"dependency affecting %s from %s\n", clkdm1->name,
             clkdm2->name);

Refer to the file omap_hwmod.c, function, _enable(), the call sequence is,

_enable() => _add_initiator_dep() => clkdm_add_sleepdep() =>leads to warning

Quoted 8 lines

Another question is about the CLKTRCTRL bitfields. According to _AM335x ARM Cortex-A8 Microprocessors (MPUs) Technical Reference Manual_ Rev. D (SPRUH73D), most of the clockdomains support NO_SLEEP mode (i.e., CLKTRCTRL = 0x0). That means that technically, we should also set the CLKDM_CAN_DISABLE_AUTO flag. Unless the documentation is incorrect here? In another section (Table 8-9 "Clock Transition Mode Settings"), it claims that CLKTRCTRL = 0 is not supported...

It is not supported, and should be considered as documentation issue.

Quoted 36 lines · Vaibhav

Quoted 34 lines · Paul Walmsley

A few questions while reviewing this patch:

Quoted 15 lines · Vaibhav Hiremath

AM33XX PRCM module consist of, various clockdomains, in all total we have 18 clockdomains available, with following controlling options,
- NO Sleep: sleep transition can not be initiated,
- SW Sleep: sw forced sleep transition,
- SW Wakeup: sw forced wakeup transition,
- HW Auto: transitions are based upon hw conditions.

This patch adds all available clockdomain data, respective clockdomain operations for AM33XX family of device, and also integrates it into existing OMAP framework.

Signed-off-by Vaibhav Hiremath <[email protected]>

Signed-off-by Afzal Mohammed <[email protected]>

Signed-off-by Vaibhav Bedia <[email protected]>

...

arch/arm/mach-omap2/clockdomains33xx_data.c line 31

Quoted 7 lines

arch/arm/mach-omap2/clockdomains33xx_data.c new file +7 -0

+static struct clockdomain l4ls_am33xx_clkdm = {
+	.name		= "l4ls_clkdm",
+	.pwrdm		= { .name = "per_pwrdm" },
+	.cm_inst	= AM33XX_CM_PER_MOD,
+	.clkdm_offs	= AM33XX_CM_PER_L4LS_CLKSTCTRL_OFFSET,
+	.clktrctrl_mask	= AM33XX_CLKTRCTRL_MASK,
+	.flags		= (CLKDM_CAN_SWSUP | CLKDM_NO_AUTODEPS),

It looks to me like we don't need the .clktrctrl_mask field, since according to the clockdomains code, the CLKTRCTRL field is at the same bit shift for each clockdomain.

Quoted 2 lines

Yeah, we actually don't need it, probably I added for completeness. I will remove it in next version.

I've removed them here.

Quoted 12 lines

Quoted 4 lines

Also, since we're not defining any autodeps for the AM335x platform, we shouldn't need the CLKDM_NO_AUTODEPS flag either? Since no autodeps are defined, the default will be to not set any autodeps.

This is required to avoid few "pr_debug", if you don't provide it. For example, without this flag set, you will get,

pr_debug("clockdomain: hardware cannot set/clear sleep "

		"dependency affecting %s from %s\n", clkdm1->name,
             clkdm2->name);

Quoted 3 lines

Refer to the file omap_hwmod.c, function, _enable(), the call sequence is,

_enable() => _add_initiator_dep() => clkdm_add_sleepdep() =>leads to warning

Seems like the better thing to do is to just avoid calling _{add,del}_initiator_dep() on the AM335x.

Quoted 9 lines

Quoted 8 lines

Another question is about the CLKTRCTRL bitfields. According to _AM335x ARM Cortex-A8 Microprocessors (MPUs) Technical Reference Manual_ Rev. D (SPRUH73D), most of the clockdomains support NO_SLEEP mode (i.e., CLKTRCTRL = 0x0). That means that technically, we should also set the CLKDM_CAN_DISABLE_AUTO flag. Unless the documentation is incorrect here? In another section (Table 8-9 "Clock Transition Mode Settings"), it claims that CLKTRCTRL = 0 is not supported...

Quoted 1 line

It is not supported, and should be considered as documentation issue.

Okay so I guess the description for this patch (quoted above) is wrong then also?

- Paul

On Thu, Apr 26, 2012 at 08:49:28, Paul Walmsley wrote:

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

Also, since we're not defining any autodeps for the AM335x platform, we shouldn't need the CLKDM_NO_AUTODEPS flag either? Since no autodeps are defined, the default will be to not set any autodeps.

This is required to avoid few "pr_debug", if you don't provide it. For example, without this flag set, you will get,

pr_debug("clockdomain: hardware cannot set/clear sleep "

		"dependency affecting %s from %s\n", clkdm1->name,
             clkdm2->name);

Refer to the file omap_hwmod.c, function, _enable(), the call sequence is,

Quoted 5 lines

Quoted 1 line

_enable() => _add_initiator_dep() => clkdm_add_sleepdep() =>leads to warning

Seems like the better thing to do is to just avoid calling _{add,del}_initiator_dep() on the AM335x.

Don't you think, if flag is doing all the job, why to pollute code with cpu_is_am33xx() checks.

Yeah, I realized it, after your comment; its copy thing...Will correct in next version.

I am thinking of separating clocktree patch (PATCH-V3 3/3) from this series, so that clockdomain stuff can get in independently, and clocktree anyway will follow them (it may take some time in review cycle).

Quoted 1 line · Vaibhav

On Thu, Apr 26, 2012 at 08:49:28, Paul Walmsley wrote:

(attribution lost)

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

Also, since we're not defining any autodeps for the AM335x platform, we shouldn't need the CLKDM_NO_AUTODEPS flag either? Since no autodeps are defined, the default will be to not set any autodeps.

This is required to avoid few "pr_debug", if you don't provide it. For example, without this flag set, you will get,

pr_debug("clockdomain: hardware cannot set/clear sleep "

		"dependency affecting %s from %s\n", clkdm1->name,
             clkdm2->name);

Refer to the file omap_hwmod.c, function, _enable(), the call sequence is,

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

_enable() => _add_initiator_dep() => clkdm_add_sleepdep() =>leads to warning

Seems like the better thing to do is to just avoid calling _{add,del}_initiator_dep() on the AM335x.

Don't you think, if flag is doing all the job, why to pollute code with cpu_is_am33xx() checks.

That's not what that flag was intended to do. It was originally intended for OMAP3xxx, to allow autodeps to be disabled for some clockdomains, while leaving them enabled for others.

If autodeps are completely unneeded, as they are for AM335x, for slower cores like the Cortex-A8, it seems best to avoid executing any superfluous code.

Quoted 2 lines

Yeah, I realized it, after your comment; its copy thing...Will correct in next version.

I've dropped those lines from the description in the local copy here.

Quoted 3 lines

I am thinking of separating clocktree patch (PATCH-V3 3/3) from this series, so that clockdomain stuff can get in independently, and clocktree anyway will follow them (it may take some time in review cycle).

Yes, I was thinking of doing this too. The only aspect that gave me pause is that the clockdomain changes are not 100% separate from the clock tree. So we may have to update the clockdomain data as the clock tree changes.

On Thu, Apr 26, 2012 at 14:27:20, Paul Walmsley wrote:

Quoted 7 lines

Quoted 3 lines

I am thinking of separating clocktree patch (PATCH-V3 3/3) from this series, so that clockdomain stuff can get in independently, and clocktree anyway will follow them (it may take some time in review cycle).

Yes, I was thinking of doing this too. The only aspect that gave me pause is that the clockdomain changes are not 100% separate from the clock tree. So we may have to update the clockdomain data as the clock tree changes.

Why do you say that, clockdomain changes are not 100% separate from clocktree? It is completely independent...

Quoted 1 line · Vaibhav

On Thu, Apr 26, 2012 at 14:27:20, Paul Walmsley wrote:

(attribution lost)

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

I am thinking of separating clocktree patch (PATCH-V3 3/3) from this series, so that clockdomain stuff can get in independently, and clocktree anyway will follow them (it may take some time in review cycle).

Yes, I was thinking of doing this too. The only aspect that gave me pause is that the clockdomain changes are not 100% separate from the clock tree. So we may have to update the clockdomain data as the clock tree changes.

Quoted 2 lines

Why do you say that, clockdomain changes are not 100% separate from clocktree? It is completely independent...

Clock enables and disables call clkdm_clk_{enable,disable}() which undertakes the clockdomain enable sequence on the first clock enabled inside the clockdomain, and the clockdomain disable sequence on the last clock inside the clockdomain. The clockdomain is associated with the clock via the struct clk .clkdm and .clkdm_name fields.

From clockdomain perspective, it is still independent and definite set of

So yes, they are interconnected; particularly if the clkdiv32k MODULEMODE "clock" is really needed.

AM33XX clock implementation is different than any existing OMAP family of devices. Although DPLL module is similar to OMAP4 device, but the usage is very much different than OMAP4. AM33XX has different peripheral set and each module gets integrated to the clock framework differently than OMAP family of devices.

This patch adds full Clock tree data for AM33XX family of devices and also integrates it into existing OMAP framework.

Hello Vaibhav, Afzal, Vaibhav,

Quoted 9 lines · Vaibhav Hiremath

AM33XX clock implementation is different than any existing OMAP family of devices. Although DPLL module is similar to OMAP4 device, but the usage is very much different than OMAP4. AM33XX has different peripheral set and each module gets integrated to the clock framework differently than OMAP family of devices.

This patch adds full Clock tree data for AM33XX family of devices and also integrates it into existing OMAP framework.

What do you think about the possibility of removing all of the leaf clocks that have AM33XX_MODULEMODE_SWCTRL as their .enable_bit, assuming there are no .fixed_div or .clksel* fields associated with the clocks?

In theory, I don't think they are needed. The drivers should be using runtime PM, and that should enable and disable the module via the hwmod code, rather than the clock code.

Of course some clocks would still be needed for the main_clk fields for the hwmods, but the hwmods should be able to use the leaf clock's parent clocks for that.

That would save quite a few lines of data. And I think Beno?t is planning to do that for OMAP4+.

What do you think?

- Paul

Quoted 22 lines · Paul Walmsley

Hello Vaibhav, Afzal, Vaibhav,

Quoted 9 lines · Vaibhav Hiremath

AM33XX clock implementation is different than any existing OMAP family of devices. Although DPLL module is similar to OMAP4 device, but the usage is very much different than OMAP4. AM33XX has different peripheral set and each module gets integrated to the clock framework differently than OMAP family of devices.

This patch adds full Clock tree data for AM33XX family of devices and also integrates it into existing OMAP framework.

What do you think about the possibility of removing all of the leaf clocks that have AM33XX_MODULEMODE_SWCTRL as their .enable_bit, assuming there are no .fixed_div or .clksel* fields associated with the clocks?

In theory, I don't think they are needed. The drivers should be using runtime PM, and that should enable and disable the module via the hwmod code, rather than the clock code.

Quoted 9 lines

Of course some clocks would still be needed for the main_clk fields for the hwmods, but the hwmods should be able to use the leaf clock's parent clocks for that.

That would save quite a few lines of data. And I think Beno?t is planning to do that for OMAP4+.

What do you think?

Paul,

Yes, theoretically it is possible to do it. But it will also break some of the existing things, like,

1. DebugFS clock interface

I believe, with this change, you will not have all the leaf nodes as part of clock tree, so they will not be populated in /debug/clock/

2. Enable and disable of the module is one part, but what about, changing the rate of the clock (followparent_recalc)?
With the proper and complete clock tree, you can traverse the clock and driver code doesn't need to know about parent.
Driver can simply call clk_set_rate() on leaf clock, and clock tree will handle it.

If at all we take this path, we have to build the clk node runtime (on-the-fly), AND/OR add new pm_runtime_set_rate() api.

Are you available on IRC chat anytime? We can discuss on this and align quickly.
I am available on linux-omap irc channel (with the name = "hvaibhav")

Hi Vaibhav,

Quoted 49 lines · Vaibhav

Quoted 22 lines · Paul Walmsley

Hello Vaibhav, Afzal, Vaibhav,

Quoted 9 lines · Vaibhav Hiremath

AM33XX clock implementation is different than any existing OMAP family of devices. Although DPLL module is similar to OMAP4 device, but the usage is very much different than OMAP4. AM33XX has different peripheral set and each module gets integrated to the clock framework differently than OMAP family of devices.

This patch adds full Clock tree data for AM33XX family of devices and also integrates it into existing OMAP framework.

What do you think about the possibility of removing all of the leaf clocks that have AM33XX_MODULEMODE_SWCTRL as their .enable_bit, assuming there are no .fixed_div or .clksel* fields associated with the clocks?

In theory, I don't think they are needed. The drivers should be using runtime PM, and that should enable and disable the module via the hwmod code, rather than the clock code.

Quoted 9 lines

Of course some clocks would still be needed for the main_clk fields for the hwmods, but the hwmods should be able to use the leaf clock's parent clocks for that.

That would save quite a few lines of data. And I think Beno?t is planning to do that for OMAP4+.

What do you think?

Paul,

Yes, theoretically it is possible to do it. But it will also break some of the existing things, like,

1. DebugFS clock interface

I believe, with this change, you will not have all the leaf nodes as part of clock tree, so they will not be populated in /debug/clock/

2. Enable and disable of the module is one part, but what about, changing the rate of the clock (followparent_recalc)?
With the proper and complete clock tree, you can traverse the clock and driver code doesn't need to know about parent.
Driver can simply call clk_set_rate() on leaf clock, and clock tree will handle it.

Quoted 2 lines

If at all we take this path, we have to build the clk node runtime (on-the-fly), AND/OR add new pm_runtime_set_rate() api.

Not at all. You just have to get the fck of that hwmod and use the clock API.

Quoted 3 lines

Are you available on IRC chat anytime? We can discuss on this and align quickly.
I am available on linux-omap irc channel (with the name = "hvaibhav")

That will not change anything, the point is that MODULEMODE_SWCTRL is uses for module control, not for clock directly, and that's why it is handled by the hwmod.

That will just replace the main_clk from the hwmod with the parent of the current modulemode nodes. Only the enable/disable part will be handled, if that node used to have a div, then the parent will handle that.

Today this module mode clock node is just a duplication of the hwmod node. By removing it, you will reduce the size of the data and have a much mode accurate representation of the reality.

Using the clock tree to handle these nodes was a hack we had to do because the hwmod fmwk was not ready when OMAP4 was introduced and because most drivers were not using pm_runtime.

Regards,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

On Wed, Apr 25, 2012 at 14:10:49, Cousson, Benoit wrote:

Quoted 8 lines

That will not change anything, the point is that MODULEMODE_SWCTRL is uses for module control, not for clock directly, and that's why it is handled by the hwmod.

That will just replace the main_clk from the hwmod with the parent of the current modulemode nodes. Only the enable/disable part will be handled, if that node used to have a div, then the parent will handle that.

Quoted 8 lines

Today this module mode clock node is just a duplication of the hwmod node. By removing it, you will reduce the size of the data and have a much mode accurate representation of the reality.

Using the clock tree to handle these nodes was a hack we had to do because the hwmod fmwk was not ready when OMAP4 was introduced and because most drivers were not using pm_runtime.

Benoit,

I completely understand what are trying to explain here, and I completely agree with you on the fact that, MODULEMODE_SWCTRL is for module control and is clock node is just a duplication.

Module enable and disable is one part, and I am not at all referring to this. My point is, more from other piece of code which are dependent on clocktree. In order to have proper and complete debugfs representation of all the clocks, somebody has to maintain the information of leaf clocks to parent relation, Isn't it?

For example, take OMAP44xx "dss_dss_clk" clock, OR any clock with "followparent_recalc" field, the question I am raising here is,

How would I know the rate of this clock in driver? Say for example, I want to configure my internal divider based on input rate?
OR
I want to change the rate of dss_clk itself?
OR
Looking at debugfs, would I get the rate of dss_clk?

OR
Will I have dss_clk present in /debug/clock/?
OR
Are we expecting user to know parent of such leaf clocks?

OR
Are we planning to export hwmod->main_clk (which is parent clk here) to User in debugfs or something?

Quoted 1 line · Vaibhav

On Wed, Apr 25, 2012 at 14:10:49, Cousson, Benoit wrote:

...

Quoted 7 lines

How would I know the rate of this clock in driver? Say for example, I want to configure my internal divider based on input rate?
OR
I want to change the rate of dss_clk itself?
OR
Looking at debugfs, would I get the rate of dss_clk?

No because that clock will not exist anymore, but you will have the real clock rate from the "dss_dss_clk".

Quoted 5 lines · Benoit

Quoted 1 line · Vaibhav

On Wed, Apr 25, 2012 at 14:10:49, Cousson, Benoit wrote:

...

<snip>

Quoted 11 lines

Quoted 7 lines

How would I know the rate of this clock in driver? Say for example, I want to configure my internal divider based on input rate?
OR
I want to change the rate of dss_clk itself?
OR
Looking at debugfs, would I get the rate of dss_clk?

No because that clock will not exist anymore, but you will have the real clock rate from the "dss_dss_clk".

Thanks Benoit, I think I understand your perspective on Module Vs leaf node and now able to digest as well.
Probably Last Q, which I think clarify all my doubts,

Assume we have complete hwmod instance and built a device using omap_device_build() api, and also the driver is completely using runtime pm api's,
How can driver get the clk handle (required to get the rate)? Is there any api already available for this?

Quoted 23 lines · Vaibhav

Quoted 5 lines · Benoit

Quoted 1 line · Vaibhav

On Wed, Apr 25, 2012 at 14:10:49, Cousson, Benoit wrote:

...

<snip>

Quoted 11 lines

Quoted 7 lines

How would I know the rate of this clock in driver? Say for example, I want to configure my internal divider based on input rate?
OR
I want to change the rate of dss_clk itself?
OR
Looking at debugfs, would I get the rate of dss_clk?

No because that clock will not exist anymore, but you will have the real clock rate from the "dss_dss_clk".

Thanks Benoit, I think I understand your perspective on Module Vs leaf node and now able to digest as well.
Probably Last Q, which I think clarify all my doubts,

Quoted 5 lines

Assume we have complete hwmod instance and built a device using omap_device_build() api, and also the driver is completely using runtime pm api's,
How can driver get the clk handle (required to get the rate)? Is there any api already available for this?

The omap_device fmwk will auto-magically create a "fck" clkdev entry for the main_clk.

The API is thus the standard: clk_get(dev, "fck"), to get the clock handle and then you can use the other clock API.

The idea is to enforce the usage of clock alias local to the device and not trying anymore to get the real clock name. It thus allows driver to work on various variant / revision without having to modify the driver.

The fmwk will also create clock alias for any opt_clock present in the hwmod.

Thanks for describing it for me. I will change AM33XX clock tree for this And submit the next version soon.

Hi Vaibhav,

Quoted 2 lines · Vaibhav

Thanks for describing it for me. I will change AM33XX clock tree for this And submit the next version soon.

Well I can just remove those leaf clock entries from my copy here, if you're okay with that ?

Quoted 5 lines · Paul Walmsley

Quoted 2 lines · Vaibhav

Thanks for describing it for me. I will change AM33XX clock tree for this And submit the next version soon.

Quoted 3 lines

Well I can just remove those leaf clock entries from my copy here, if you're okay with that ?

Great...

More that OK :)

I have taken a first pass at this. The updated patch is below. It has been compile-tested only. Could you please review this and try testing it? It is also in the branch 'am33xx_support_3.5' of
git://git.pwsan.com/linux-2.6.

Hi Vaibhav,

Quoted 2 lines · Vaibhav

Thanks for describing it for me. I will change AM33XX clock tree for this And submit the next version soon.

Well I can just remove those leaf clock entries from my copy here, if you're okay with that ?

Please take care of changing the hwmod main_clk as well. But maybe that's not part of that series.

Quoted 2 lines · Benoit

Please take care of changing the hwmod main_clk as well. But maybe that's not part of that series.

It's not part of the series yet.

Vaibhav, could you take care of changing the main_clk in your hwmod data patches, and send those to the list?

- Paul

Quoted 2 lines · Benoit

Please take care of changing the hwmod main_clk as well. But maybe that's not part of that series.

It's not part of the series yet.

Vaibhav, could you take care of changing the main_clk in your hwmod data patches, and send those to the list?

I used your cleaned version of clocktree data, where you have removed all leaf-nodes and merged multiple clocks nodes into one; but it did not work. I attempted to review the cleanup and tried to debug, but found it bit hard to come back to working clocktree from stripped version. So moved back to my submitted clocktree and started stripping the clock leaf-nodes, same way you had done it. Now I reached to the stage where I have working clocktree without peripheral leaf-nodes and booting on BeagleBone.

Just FYI (may need your help), I got into some issues (still open) during this cleanup -

1. In cases where we have clock selection option for node-leafs, like, Timers, I removed enable_reg entry from the node, which results into a kernel error message from function omap2_dflt_clk_disable()

clock: clk_disable called on independent clock %s which has no enable_reg

2. clkdm_name entry:
The issue is, two entries available for clockdomain associated for module/clock
- hwmod_data.main_clk and
- clk.clkdm_name

They must match, right? The parent/root clocks flows from one domain to another domain, so I have to keep the nodes just for sake of matching clockdomain entry present in associated main_clk data.

I am still reviewing the code from Rajendra, but one of Rajendra's patch actually will help here, made it to use hwmod_data.clkdm (if available) always, instead of directly using clk.clkdm.

3. If I understand correctly, hwmod_data.main_clk is functional clock and hwmod_ocp_if.clk is interface clock, right?

So currently, I have mentioned same entry in both the places (especially for Peripherals/modules).
I am planning to remove ocp_if/clk entry data for all modules..

Hi Vaibhav

Quoted 7 lines · Vaibhav

I used your cleaned version of clocktree data, where you have removed all leaf-nodes and merged multiple clocks nodes into one; but it did not work. I attempted to review the cleanup and tried to debug, but found it bit hard to come back to working clocktree from stripped version. So moved back to my submitted clocktree and started stripping the clock leaf-nodes, same way you had done it. Now I reached to the stage where I have working clocktree without peripheral leaf-nodes and booting on BeagleBone.

Okay great, please post that to the list so we can diff it against the version that I did, and also so we can queue it for merging in 3.6?

Quoted 8 lines

Just FYI (may need your help), I got into some issues (still open) during this cleanup -

1. In cases where we have clock selection option for node-leafs, like, Timers, I removed enable_reg entry from the node, which results into a kernel error message from function omap2_dflt_clk_disable()

clock: clk_disable called on independent clock %s which has no enable_reg

Shouldn't those clocks use clkops_null then?

Quoted 6 lines

2. clkdm_name entry:
The issue is, two entries available for clockdomain associated for module/clock
- hwmod_data.main_clk and
- clk.clkdm_name

Quoted 7 lines

They must match, right? The parent/root clocks flows from one domain to another domain, so I have to keep the nodes just for sake of matching clockdomain entry present in associated main_clk data.

I am still reviewing the code from Rajendra, but one of Rajendra's patch actually will help here, made it to use hwmod_data.clkdm (if available) always, instead of directly using clk.clkdm.

Well because of this CLKDIV32K snafu, we'll need to keep the clock framework-based clockdomain control for AM33xx, at the very least for the CLKDIV32K clock.

I'd suggest starting with specifying a clockdomain name for each clock. If nothing else, this should help think through the power management behavior for each clock. Then those can be easily removed later with a simple grep.

Quoted 2 lines

3. If I understand correctly, hwmod_data.main_clk is functional clock and hwmod_ocp_if.clk is interface clock, right?

Basically, yes. To be more precise, in cases where
modules have multiple functional clocks, hwmod_data.main_clk is the functional clock that is needed in order for register reads and writes to the IP block to succeed.

Quoted 3 lines

So currently, I have mentioned same entry in both the places (especially for Peripherals/modules).
I am planning to remove ocp_if/clk entry data for all modules..

Hmmm, could you explain this further?

Every ocp_if structure that you create should have an interface clock specified. And almost all of your hwmods should have a main_clk specified.

- Paul

I believe register read/write to IP block is depends on only interface Clocks? Atleast in case of OMAP3, it was like that, right??

Another issues is,

I came across situation where, two modules fall into different clock domains but their functional clock is happened to be coming from same source/dpll-divider. So in order to satisfy clkdm match between them, I have kept nodes without enable_regs.

Quoted 5 lines

Quoted 3 lines

So currently, I have mentioned same entry in both the places (especially for Peripherals/modules).
I am planning to remove ocp_if/clk entry data for all modules..

Hmmm, could you explain this further?

what if, module only has interface clock? Should it be present as main_clk or ocp_if.clk or both?? Example would be, TPCC, TPTC, smartreflex, etc...

Hi

Quoted 2 lines · Vaibhav

I believe register read/write to IP block is depends on only interface Clocks? Atleast in case of OMAP3, it was like that, right??

No, on OMAP3, most modules need both the interface clock enabled, and at least one of their functional clocks. For modules with multiple functional clocks like the OMAP3 USBHOST, we had to use trial and error to determine which functional clock was the main clock, since it wasn't documented. If we got it wrong, then register accesses to the module would result in an abort.

The AM33xx/OMAP4 behavior should be a little easier in this regard, though, since the MODULEMODE bits should just control everything for a given module, and that should be handled by the hwmod code. Nevertheless it is still good to specify a main_clk so we know how fast the module's registers are clocked and to locate the module in the power management hierarchy.

Quoted 6 lines

Another issues is,

I came across situation where, two modules fall into different clock domains but their functional clock is happened to be coming from same source/dpll-divider. So in order to satisfy clkdm match between them, I have kept nodes without enable_regs.

Could you please provide an example?

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

So currently, I have mentioned same entry in both the places (especially for Peripherals/modules).
I am planning to remove ocp_if/clk entry data for all modules..

Hmmm, could you explain this further?

Quoted 3 lines

what if, module only has interface clock? Should it be present as main_clk or ocp_if.clk or both?? Example would be, TPCC, TPTC, smartreflex, etc...

Well it definitely should be present as the ocp_if.clk. I personally think it would be good to list the same clock as the hwmod's main_clk too, but it's currently not strictly necessary. There are some examples in the omap_hwmod_44xx_data.c file, like omap44xx_mailbox_hwmod.

Quoted 8 lines · Paul Walmsley

Quoted 4 lines · Vaibhav

I came across situation where, two modules fall into different clock domains but their functional clock is happened to be coming from same source/dpll-divider. So in order to satisfy clkdm match between them, I have kept nodes without enable_regs.

Could you please provide an example?

In case of AM33xx, clock architecture is,

sysclk1 -> L4_wakeup - wakeup domain modules

sysclk1 -> L4 HS - L4 HS domain modules

sysclk1 -> L4 LS - L4 LS domain modules

and so on...

Now with leaf node cleanup, we are moving upward in the clocktree, more close to dpll output, and is the issue related to clockdomain.

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

So currently, I have mentioned same entry in both the places (especially for Peripherals/modules).
I am planning to remove ocp_if/clk entry data for all modules..

Hmmm, could you explain this further?

what if, module only has interface clock? Should it be present as main_clk or ocp_if.clk or both?? Example would be, TPCC, TPTC, smartreflex, etc...

Quoted 4 lines

Well it definitely should be present as the ocp_if.clk. I personally think it would be good to list the same clock as the hwmod's main_clk too, but it's currently not strictly necessary. There are some examples in the omap_hwmod_44xx_data.c file, like omap44xx_mailbox_hwmod.

omap44xx_mailbox_hwmod doesn't have main_clk exported in the hwmod_data, so I think I should also follow same thing, right?

You can access the code at -
https://github.com/hvaibhav/am335x-linux am335x-upstream-staging

Hi,

arch/arm/mach-omap2/clock33xx_data.c line 724

Quoted 12 lines

arch/arm/mach-omap2/clock33xx_data.c new file +12 -0

+/*
+ * clkdiv32 is generated from fixed division of 732.4219
+ */
+static struct clk clkdiv32k_ick = {
+	.name		= "clkdiv32k_ick",
+	.clkdm_name	= "clk_24mhz_clkdm",
+	.rate		= 32768,
+	.parent		= &clk_24mhz,
+	.enable_reg	= AM33XX_CM_PER_CLKDIV32K_CLKCTRL,
+	.enable_bit	= AM33XX_MODULEMODE_SWCTRL,
+	.ops		= &clkops_omap2_dflt,
+};

While working on this file, this clock seemed quite perplexing. Perhaps you might be able to answer some questions about it.

- The fractional fixed division seems a little bogus. Is this actually an M,N counter? A few moments with PARI revealed a likely M,N of 64,46875. Could you please confirm that this is the case for this clock?

- This clock structure makes this clock appear to be a fixed-frequency clock. But according to SPRUH73D Figure 8-10 "Peripheral PLL Structure", the divider feeding this clock can be switched between an M,N of 64,46875 and 32,46875 depending on the value of CONTROL_CLK32K_DIVRATIO_CTRL. So shouldn't we implement that?

- This clock is feeding downstream clocks, but it's using the MODULEMODE control interface, as if it were a standalone IP block. Do you know why it's using the MODULEMODE control method rather than some optional clock control bits, like OMAP4 does?

- Paul

Unfortunately, this is fractional divider, assuming input clock of 24MHz.

24MHz / 732.4219 = 32KHz (Normal OPP100)
24MHz / 366.2109 = 32KHz (OPP50)

As per the Spec, we have some known HW/Si issues to run whole system at OPP50, that's where if you refer to the TRM Table 8-22. Per PLL Typical Frequencies (MHz), it only talks about OPP100, which is 732.4219 divider.

So, technically, if both OPP's (OPP100 and OPP50) works fine, then yes, we should handle it. But given the fact that, OPP50 is not fully functional due to HW/Si issues, from CORE and PER perspective we will always stay at OPP100.

To summarize, I had to make it fixed-frequency due to,

- Fractional divisor value.

Quoted 4 lines

- This clock is feeding downstream clocks, but it's using the MODULEMODE control interface, as if it were a standalone IP block. Do you know why it's using the MODULEMODE control method rather than some optional clock control bits, like OMAP4 does?

Yes, this clock is feeding to timers, gpio debounce clocks, etc... Honestly, I do not know the answer to why part of it. (May be another integration/design issue)

Quoted 2 lines · Vaibhav

Unfortunately, this is fractional divider, assuming input clock of 24MHz.

Quoted 2 lines

24MHz / 732.4219 = 32KHz (Normal OPP100)
24MHz / 366.2109 = 32KHz (OPP50)

Yes, I already saw that in the TRM. The question is, how is the fractional divider implemented in the hardware? Is it implemented as an M,N counter? It's rather unlikely to be dividing by 732.4219 directly -- in fact that value isn't even accurate.

Quoted 7 lines

As per the Spec, we have some known HW/Si issues to run whole system at OPP50, that's where if you refer to the TRM Table 8-22. Per PLL Typical Frequencies (MHz), it only talks about OPP100, which is 732.4219 divider.

So, technically, if both OPP's (OPP100 and OPP50) works fine, then yes, we should handle it. But given the fact that, OPP50 is not fully functional due to HW/Si issues, from CORE and PER perspective we will always stay at OPP100.

I see.

Quoted 3 lines

To summarize, I had to make it fixed-frequency due to,

- Fractional divisor value.

This could be implemented as an M,N counter, unless the hardware is doing something more unusual.

Quoted 8 lines

Quoted 4 lines

- This clock is feeding downstream clocks, but it's using the MODULEMODE control interface, as if it were a standalone IP block. Do you know why it's using the MODULEMODE control method rather than some optional clock control bits, like OMAP4 does?

Yes, this clock is feeding to timers, gpio debounce clocks, etc... Honestly, I do not know the answer to why part of it. (May be another integration/design issue)

Could you please check with the AM335x PRCM designers about this to find out what that MODULEMODE bit is actually doing (i.e., is it actually enabling the clock, and if so, why did they not use the OPTFCLKEN bits) ? The AM335x TRM has almost no documentation on what those MODULEMODE bits are doing in this case.

We're trying to remove all of these MODULEMODE clocks from the clock framework, and also remove the accompanying clockdomain control sequence from the clock code. But now it's looking like it may be impossible due to this "clock," which means we'll need to keep that code around, which is unfortunate.

- Paul

Quoted 11 lines · Paul Walmsley

Quoted 4 lines · Vaibhav

Unfortunately, this is fractional divider, assuming input clock of 24MHz.

24MHz / 732.4219 = 32KHz (Normal OPP100)
24MHz / 366.2109 = 32KHz (OPP50)

Yes, I already saw that in the TRM. The question is, how is the fractional divider implemented in the hardware? Is it implemented as an M,N counter? It's rather unlikely to be dividing by 732.4219 directly -- in fact that value isn't even accurate.

Not sure how it is implemented in HW, in any case we do not have control over it. The control is only provided using,
- MODULEMODE (module enable/disable)
- CONTROL_CLK32K_DIVRATIO_CTRL (divisor selection)

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

- This clock is feeding downstream clocks, but it's using the MODULEMODE control interface, as if it were a standalone IP block. Do you know why it's using the MODULEMODE control method rather than some optional clock control bits, like OMAP4 does?

Yes, this clock is feeding to timers, gpio debounce clocks, etc... Honestly, I do not know the answer to why part of it. (May be another integration/design issue)

Quoted 5 lines

Could you please check with the AM335x PRCM designers about this to find out what that MODULEMODE bit is actually doing (i.e., is it actually enabling the clock, and if so, why did they not use the OPTFCLKEN bits) ? The AM335x TRM has almost no documentation on what those MODULEMODE bits are doing in this case.

I can check, I am certain that, the answer would be to consider this as a separate independent module.

And same as other modules, control is provided to the SW.

Hi Vaibhav, Afzal, Vaibhav,

while working on clock33xx_data.c, it became clear that we would not be able to remove the MODULEMODE leaf clocks for several IP blocks that share driver code with other DaVinci chips. This is because:

1. the drivers for these IP blocks only use the clock framework, rather than runtime PM;

and

2. the clk_get() calls in those drivers do not specify a con_id parameter, which could cause problems with the clkdev clk_find()'s fuzzy matching algorithm.

So perhaps your group can work on converting the drivers to use runtime PM? Discussing this with Kevin, it sounds like DaVinci doesn't have runtime PM hooks implemented, but you can probably use OMAP1 as an example of how to do it.

And then if clk_get() is still needed in the drivers, a specific con_id should be supplied, such as "fck", which can also be supplied by the OMAP codebase.

Quoted 14 lines · Paul Walmsley

while working on clock33xx_data.c, it became clear that we would not be able to remove the MODULEMODE leaf clocks for several IP blocks that share driver code with other DaVinci chips. This is because:

1. the drivers for these IP blocks only use the clock framework, rather than runtime PM;

and

2. the clk_get() calls in those drivers do not specify a con_id parameter, which could cause problems with the clkdev clk_find()'s fuzzy matching algorithm.

Quoted 8 lines

So perhaps your group can work on converting the drivers to use runtime PM? Discussing this with Kevin, it sounds like DaVinci doesn't have runtime PM hooks implemented, but you can probably use OMAP1 as an example of how to do it.

And then if clk_get() is still needed in the drivers, a specific con_id should be supplied, such as "fck", which can also be supplied by the OMAP codebase.

Honestly, I don't want to block AM33xx stuff getting into mainline due to missing feature in Davinci.

Also, since AM33xx is not fully supported in the mainline (from peripheral perspective), it really should not break anything as of now.

I am expecting, AM33xx core patches will get in by v3.5 and then while adding peripheral support we will take Davinci migration activity, and can make sure that nothing breaks (for all Davinci, OMAP and AM33xx).

Hello Vaibhav,

looking at Table 1-1 "Device Features" of SPRUH73D, it seems that some AM33xx family chips come with some features disabled, such as the PRU-ICSS, the SGX, Ethernet, or USB. How will this affect the clock tree? For example, is it correct for us to include the PRU-ICSS clock control on a clock tree that is booted on an AM3352 or an AM3354?

Quoted 5 lines · Paul Walmsley

looking at Table 1-1 "Device Features" of SPRUH73D, it seems that some AM33xx family chips come with some features disabled, such as the PRU-ICSS, the SGX, Ethernet, or USB. How will this affect the clock tree? For example, is it correct for us to include the PRU-ICSS clock control on a clock tree that is booted on an AM3352 or an AM3354?

I thought of this, but if you look at the existing way of handling such Si version based clock registration is supported by "cpu_clkflg", like CK_3XXX, CK_AM35XX, etc...

During my baseport patch submission, Tony had removed CK_AM33XX flag while merging them. I believe, we are trying to get rid of these flags, and that's where Tony had removed them form my baseport patch.
I was not sure on how to handle this, so I decided to continue with full chip support (AM3358 and AM3359).

Quoted 11 lines · Vaibhav

Quoted 5 lines · Paul Walmsley

looking at Table 1-1 "Device Features" of SPRUH73D, it seems that some AM33xx family chips come with some features disabled, such as the PRU-ICSS, the SGX, Ethernet, or USB. How will this affect the clock tree? For example, is it correct for us to include the PRU-ICSS clock control on a clock tree that is booted on an AM3352 or an AM3354?

I thought of this, but if you look at the existing way of handling such Si version based clock registration is supported by "cpu_clkflg", like CK_3XXX, CK_AM35XX, etc...

Quoted 5 lines

During my baseport patch submission, Tony had removed CK_AM33XX flag while merging them. I believe, we are trying to get rid of these flags, and that's where Tony had removed them form my baseport patch.
I was not sure on how to handle this, so I decided to continue with full chip support (AM3358 and AM3359).

Okay, no problem. Sounds like we should start with full support and then remove clocks later if they cause problems on the lower-end devices.