| Message ID | 1346230576-20004-3-git-send-email-rnayak@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 29, 2012 at 02:26:15PM +0530, Rajendra Nayak wrote: > Moving to Common clk framework for OMAP would mean we no longer use > internal lookup mechanism like omap_clk_get_by_name(). > get rid of all its usage mostly from hwmod and omap_device > code. > > Also use IS_ERR_OR_NULL() for error checking. This is wrong. IS_ERR() is the only check you should ever use with clk_get().
On Wed, Aug 29, 2012 at 1:56 AM, Rajendra Nayak <rnayak@ti.com> wrote: > diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c > index d7f55e4..a3831a2 100644 > --- a/arch/arm/mach-omap2/clock44xx_data.c > +++ b/arch/arm/mach-omap2/clock44xx_data.c > @@ -3156,6 +3156,7 @@ static struct omap_clk omap44xx_clks[] = { > CLK(NULL, "dss_tv_clk", &dss_tv_clk, CK_443X), > CLK(NULL, "dss_48mhz_clk", &dss_48mhz_clk, CK_443X), > CLK(NULL, "dss_dss_clk", &dss_dss_clk, CK_443X), > + CLK(NULL, "dss_fck", &dss_fck, CK_443X), > CLK("omapdss_dss", "ick", &dss_fck, CK_443X), Is it right to re-use dss_fck for the "ick" here? I think it is due to omap4 modulemode stuff but I don't have DM in front of me and wanted a double-check... Regards, Mike
On Thursday 30 August 2012 05:38 AM, Russell King - ARM Linux wrote: > On Wed, Aug 29, 2012 at 02:26:15PM +0530, Rajendra Nayak wrote: >> Moving to Common clk framework for OMAP would mean we no longer use >> internal lookup mechanism like omap_clk_get_by_name(). >> get rid of all its usage mostly from hwmod and omap_device >> code. >> >> Also use IS_ERR_OR_NULL() for error checking. > > This is wrong. IS_ERR() is the only check you should ever use with clk_get(). okay, thanks, will fix.
On Thursday 30 August 2012 05:45 AM, Turquette, Mike wrote: > On Wed, Aug 29, 2012 at 1:56 AM, Rajendra Nayak<rnayak@ti.com> wrote: >> diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c >> index d7f55e4..a3831a2 100644 >> --- a/arch/arm/mach-omap2/clock44xx_data.c >> +++ b/arch/arm/mach-omap2/clock44xx_data.c >> @@ -3156,6 +3156,7 @@ static struct omap_clk omap44xx_clks[] = { >> CLK(NULL, "dss_tv_clk",&dss_tv_clk, CK_443X), >> CLK(NULL, "dss_48mhz_clk",&dss_48mhz_clk, CK_443X), >> CLK(NULL, "dss_dss_clk",&dss_dss_clk, CK_443X), >> + CLK(NULL, "dss_fck",&dss_fck, CK_443X), >> CLK("omapdss_dss", "ick",&dss_fck, CK_443X), > > Is it right to re-use dss_fck for the "ick" here? I think it is due > to omap4 modulemode stuff but I don't have DM in front of me and > wanted a double-check... yes, its because of the wierdness of DSS clocks where a so-called optional clock is actually a function clock. > > Regards, > Mike
On 08/30/2012 10:39 AM, Rajendra Nayak wrote: > On Thursday 30 August 2012 05:45 AM, Turquette, Mike wrote: >> On Wed, Aug 29, 2012 at 1:56 AM, Rajendra Nayak<rnayak@ti.com> wrote: >>> diff --git a/arch/arm/mach-omap2/clock44xx_data.c >>> b/arch/arm/mach-omap2/clock44xx_data.c >>> index d7f55e4..a3831a2 100644 >>> --- a/arch/arm/mach-omap2/clock44xx_data.c >>> +++ b/arch/arm/mach-omap2/clock44xx_data.c >>> @@ -3156,6 +3156,7 @@ static struct omap_clk omap44xx_clks[] = { >>> CLK(NULL, "dss_tv_clk",&dss_tv_clk, CK_443X), >>> CLK(NULL, "dss_48mhz_clk",&dss_48mhz_clk, CK_443X), >>> CLK(NULL, "dss_dss_clk",&dss_dss_clk, CK_443X), >>> + CLK(NULL, "dss_fck",&dss_fck, CK_443X), >>> CLK("omapdss_dss", "ick",&dss_fck, CK_443X), >> >> Is it right to re-use dss_fck for the "ick" here? I think it is due >> to omap4 modulemode stuff but I don't have DM in front of me and >> wanted a double-check... > > yes, its because of the wierdness of DSS clocks where a so-called > optional clock is actually a function clock. Not only, it is a hack to allow the DSS to be enabled whenever a DSS submodule has to be enabled. Since they are sharing the ick, it will enable the modulemode when the DISPC will be enabled. That hack should disappear as soon as the DSS will be able to handle the PM dependency between DSS submodules and the DSS itself using pm_runtime parent/child link. And then we will have to remove this fake ick modulemode clock node and let hwmod handle that properly. Tomi already cleaned the DSS part, I guess it should be upstreamed soon, if this is not already the case. Regards, Benoit
On Thu, 2012-08-30 at 13:57 +0200, Benoit Cousson wrote: > On 08/30/2012 10:39 AM, Rajendra Nayak wrote: > > On Thursday 30 August 2012 05:45 AM, Turquette, Mike wrote: > >> On Wed, Aug 29, 2012 at 1:56 AM, Rajendra Nayak<rnayak@ti.com> wrote: > >>> diff --git a/arch/arm/mach-omap2/clock44xx_data.c > >>> b/arch/arm/mach-omap2/clock44xx_data.c > >>> index d7f55e4..a3831a2 100644 > >>> --- a/arch/arm/mach-omap2/clock44xx_data.c > >>> +++ b/arch/arm/mach-omap2/clock44xx_data.c > >>> @@ -3156,6 +3156,7 @@ static struct omap_clk omap44xx_clks[] = { > >>> CLK(NULL, "dss_tv_clk",&dss_tv_clk, CK_443X), > >>> CLK(NULL, "dss_48mhz_clk",&dss_48mhz_clk, CK_443X), > >>> CLK(NULL, "dss_dss_clk",&dss_dss_clk, CK_443X), > >>> + CLK(NULL, "dss_fck",&dss_fck, CK_443X), > >>> CLK("omapdss_dss", "ick",&dss_fck, CK_443X), > >> > >> Is it right to re-use dss_fck for the "ick" here? I think it is due > >> to omap4 modulemode stuff but I don't have DM in front of me and > >> wanted a double-check... > > > > yes, its because of the wierdness of DSS clocks where a so-called > > optional clock is actually a function clock. > > Not only, it is a hack to allow the DSS to be enabled whenever a DSS > submodule has to be enabled. Since they are sharing the ick, it will > enable the modulemode when the DISPC will be enabled. > > That hack should disappear as soon as the DSS will be able to handle the > PM dependency between DSS submodules and the DSS itself using pm_runtime > parent/child link. > And then we will have to remove this fake ick modulemode clock node and > let hwmod handle that properly. > > Tomi already cleaned the DSS part, I guess it should be upstreamed soon, > if this is not already the case. This is in 3.5. omapdss_dss is now the parent for the rest of the dss submodules, and runtime PM is being used. I think we should now revisit this dss clock handling to see if there are any other changes required in the omapdss side, but I hope they are already according to our discussions and correct. Tomi
On Thursday 30 August 2012 10:12 PM, Tomi Valkeinen wrote: > On Thu, 2012-08-30 at 13:57 +0200, Benoit Cousson wrote: >> On 08/30/2012 10:39 AM, Rajendra Nayak wrote: >>> On Thursday 30 August 2012 05:45 AM, Turquette, Mike wrote: >>>> On Wed, Aug 29, 2012 at 1:56 AM, Rajendra Nayak<rnayak@ti.com> wrote: >>>>> diff --git a/arch/arm/mach-omap2/clock44xx_data.c >>>>> b/arch/arm/mach-omap2/clock44xx_data.c >>>>> index d7f55e4..a3831a2 100644 >>>>> --- a/arch/arm/mach-omap2/clock44xx_data.c >>>>> +++ b/arch/arm/mach-omap2/clock44xx_data.c >>>>> @@ -3156,6 +3156,7 @@ static struct omap_clk omap44xx_clks[] = { >>>>> CLK(NULL, "dss_tv_clk",&dss_tv_clk, CK_443X), >>>>> CLK(NULL, "dss_48mhz_clk",&dss_48mhz_clk, CK_443X), >>>>> CLK(NULL, "dss_dss_clk",&dss_dss_clk, CK_443X), >>>>> + CLK(NULL, "dss_fck",&dss_fck, CK_443X), >>>>> CLK("omapdss_dss", "ick",&dss_fck, CK_443X), >>>> >>>> Is it right to re-use dss_fck for the "ick" here? I think it is due >>>> to omap4 modulemode stuff but I don't have DM in front of me and >>>> wanted a double-check... >>> >>> yes, its because of the wierdness of DSS clocks where a so-called >>> optional clock is actually a function clock. >> >> Not only, it is a hack to allow the DSS to be enabled whenever a DSS >> submodule has to be enabled. Since they are sharing the ick, it will >> enable the modulemode when the DISPC will be enabled. >> >> That hack should disappear as soon as the DSS will be able to handle the >> PM dependency between DSS submodules and the DSS itself using pm_runtime >> parent/child link. >> And then we will have to remove this fake ick modulemode clock node and >> let hwmod handle that properly. >> >> Tomi already cleaned the DSS part, I guess it should be upstreamed soon, >> if this is not already the case. > > This is in 3.5. omapdss_dss is now the parent for the rest of the dss > submodules, and runtime PM is being used. > > I think we should now revisit this dss clock handling to see if there > are any other changes required in the omapdss side, but I hope they are > already according to our discussions and correct. The only little problem was that during bootup, when hwmods are setup, only the 'parent' hwmod was able to get reset properly, all the other 'child' hwmods don't have modulemode bits tied to them, and hence weren't able to reset. So we got some error prints. Once DSS driver kicks in, the driver ensures the parent is enabled for any child to be enabled, so we don't face the issue again. So, if DSS driver is not built in, and if the bootloader left DSS in a bad state, the DSS clocks might remain messed up all the time since hwmod fwk wasn't able to reset them. I think this is why we didn't proceed with remove "dss_fck" as a slave clock. If this issue is minor, we could go ahead and remove it. Archit
On Fri, 2012-08-31 at 11:53 +0530, Archit Taneja wrote: > The only little problem was that during bootup, when hwmods are setup, > only the 'parent' hwmod was able to get reset properly, all the other > 'child' hwmods don't have modulemode bits tied to them, and hence > weren't able to reset. So we got some error prints. > > Once DSS driver kicks in, the driver ensures the parent is enabled for > any child to be enabled, so we don't face the issue again. > > So, if DSS driver is not built in, and if the bootloader left DSS in a > bad state, the DSS clocks might remain messed up all the time since > hwmod fwk wasn't able to reset them. > > I think this is why we didn't proceed with remove "dss_fck" as a slave > clock. If this issue is minor, we could go ahead and remove it. I wonder if we could handle this with a custom reset function. We already have a reset func for dss core. If I remember right, the main point for that is the fact that omap4 doesn't have a softreset for dss core, so we manually write the default values to registers. For omap2/3 this would be simple: skip the resets for all other dss submodules, and dss core's reset would enable all the clocks and set the softreset bit. This would reset all the submodules also. Omap4 is more tricky. I guess we'd need to enable all the clocks, clear manually dss core's registers, and then set softreset bits in all the submodules. So in this case dss core would need to have information about the other submodules. Tomi
On Friday 31 August 2012 12:45 PM, Tomi Valkeinen wrote: > On Fri, 2012-08-31 at 11:53 +0530, Archit Taneja wrote: > >> The only little problem was that during bootup, when hwmods are setup, >> only the 'parent' hwmod was able to get reset properly, all the other >> 'child' hwmods don't have modulemode bits tied to them, and hence >> weren't able to reset. So we got some error prints. >> >> Once DSS driver kicks in, the driver ensures the parent is enabled for >> any child to be enabled, so we don't face the issue again. >> >> So, if DSS driver is not built in, and if the bootloader left DSS in a >> bad state, the DSS clocks might remain messed up all the time since >> hwmod fwk wasn't able to reset them. >> >> I think this is why we didn't proceed with remove "dss_fck" as a slave >> clock. If this issue is minor, we could go ahead and remove it. > > I wonder if we could handle this with a custom reset function. We > already have a reset func for dss core. If I remember right, the main > point for that is the fact that omap4 doesn't have a softreset for dss > core, so we manually write the default values to registers. > > For omap2/3 this would be simple: skip the resets for all other dss > submodules, and dss core's reset would enable all the clocks and set the > softreset bit. This would reset all the submodules also. > > Omap4 is more tricky. I guess we'd need to enable all the clocks, clear > manually dss core's registers, and then set softreset bits in all the > submodules. So in this case dss core would need to have information > about the other submodules. The is a good idea. I don't clearly understand your approach though. Are you saying we have a custom reset function for only dss core? And reset the submodules in it manually? An alternative approach would be to implement custom reset functions for each submodule(or each hwmod), and in the beginning of every reset function, add a hack to enable MODULEMODE bits(since we don't want hwmod fwk to touch MODULEMODE for the DSS submodules), and then set the soft reset bits. Your approach would ensure that we get a clean reset of DSS, but it would still give the annoying prints when each of the submodule tries to reset itself. Archit
On Fri, 2012-08-31 at 13:50 +0530, Archit Taneja wrote: > On Friday 31 August 2012 12:45 PM, Tomi Valkeinen wrote: > > On Fri, 2012-08-31 at 11:53 +0530, Archit Taneja wrote: > > > >> The only little problem was that during bootup, when hwmods are setup, > >> only the 'parent' hwmod was able to get reset properly, all the other > >> 'child' hwmods don't have modulemode bits tied to them, and hence > >> weren't able to reset. So we got some error prints. > >> > >> Once DSS driver kicks in, the driver ensures the parent is enabled for > >> any child to be enabled, so we don't face the issue again. > >> > >> So, if DSS driver is not built in, and if the bootloader left DSS in a > >> bad state, the DSS clocks might remain messed up all the time since > >> hwmod fwk wasn't able to reset them. > >> > >> I think this is why we didn't proceed with remove "dss_fck" as a slave > >> clock. If this issue is minor, we could go ahead and remove it. > > > > I wonder if we could handle this with a custom reset function. We > > already have a reset func for dss core. If I remember right, the main > > point for that is the fact that omap4 doesn't have a softreset for dss > > core, so we manually write the default values to registers. > > > > For omap2/3 this would be simple: skip the resets for all other dss > > submodules, and dss core's reset would enable all the clocks and set the > > softreset bit. This would reset all the submodules also. > > > > Omap4 is more tricky. I guess we'd need to enable all the clocks, clear > > manually dss core's registers, and then set softreset bits in all the > > submodules. So in this case dss core would need to have information > > about the other submodules. > > The is a good idea. I don't clearly understand your approach though. Are > you saying we have a custom reset function for only dss core? And reset > the submodules in it manually? Yes. > An alternative approach would be to implement custom reset functions for > each submodule(or each hwmod), and in the beginning of every reset > function, add a hack to enable MODULEMODE bits(since we don't want hwmod > fwk to touch MODULEMODE for the DSS submodules), and then set the soft > reset bits. I thought about that also. We'd need reset functions for all of them, and for omap2/3 we'd just reset the submodules again as they have already been reset with the dss core reset. The dss submodule resets are a bit linked. For omap2/3 the connection is obvious as dss core reset resets also the submodules, and for omap4 we have this requirement for the modulemode. That's why I though it'd be perhaps cleaner to handle the reset of the DSS block as a whole, in one place. > Your approach would ensure that we get a clean reset of DSS, but it > would still give the annoying prints when each of the submodule tries to > reset itself. The other submodules would not be reset by the hwmod framework at all, so there wouldn't be prints. I think there's a flag for that. Tomi
On Friday 31 August 2012 01:57 PM, Tomi Valkeinen wrote: > On Fri, 2012-08-31 at 13:50 +0530, Archit Taneja wrote: >> On Friday 31 August 2012 12:45 PM, Tomi Valkeinen wrote: >>> On Fri, 2012-08-31 at 11:53 +0530, Archit Taneja wrote: >>> >>>> The only little problem was that during bootup, when hwmods are setup, >>>> only the 'parent' hwmod was able to get reset properly, all the other >>>> 'child' hwmods don't have modulemode bits tied to them, and hence >>>> weren't able to reset. So we got some error prints. >>>> >>>> Once DSS driver kicks in, the driver ensures the parent is enabled for >>>> any child to be enabled, so we don't face the issue again. >>>> >>>> So, if DSS driver is not built in, and if the bootloader left DSS in a >>>> bad state, the DSS clocks might remain messed up all the time since >>>> hwmod fwk wasn't able to reset them. >>>> >>>> I think this is why we didn't proceed with remove "dss_fck" as a slave >>>> clock. If this issue is minor, we could go ahead and remove it. >>> >>> I wonder if we could handle this with a custom reset function. We >>> already have a reset func for dss core. If I remember right, the main >>> point for that is the fact that omap4 doesn't have a softreset for dss >>> core, so we manually write the default values to registers. >>> >>> For omap2/3 this would be simple: skip the resets for all other dss >>> submodules, and dss core's reset would enable all the clocks and set the >>> softreset bit. This would reset all the submodules also. >>> >>> Omap4 is more tricky. I guess we'd need to enable all the clocks, clear >>> manually dss core's registers, and then set softreset bits in all the >>> submodules. So in this case dss core would need to have information >>> about the other submodules. >> >> The is a good idea. I don't clearly understand your approach though. Are >> you saying we have a custom reset function for only dss core? And reset >> the submodules in it manually? > > Yes. > >> An alternative approach would be to implement custom reset functions for >> each submodule(or each hwmod), and in the beginning of every reset >> function, add a hack to enable MODULEMODE bits(since we don't want hwmod >> fwk to touch MODULEMODE for the DSS submodules), and then set the soft >> reset bits. > > I thought about that also. We'd need reset functions for all of them, > and for omap2/3 we'd just reset the submodules again as they have > already been reset with the dss core reset. > > The dss submodule resets are a bit linked. For omap2/3 the connection is > obvious as dss core reset resets also the submodules, and for omap4 we > have this requirement for the modulemode. That's why I though it'd be > perhaps cleaner to handle the reset of the DSS block as a whole, in one > place. > >> Your approach would ensure that we get a clean reset of DSS, but it >> would still give the annoying prints when each of the submodule tries to >> reset itself. > > The other submodules would not be reset by the hwmod framework at all, > so there wouldn't be prints. I think there's a flag for that. Oh, yeah. I didn't think of it that way, we could just 'not reset' the DSS submodule hwmod using this flag. If we do that, then your approach sounds good. Archit
Hi, On Friday 31 August 2012 01:58 PM, Archit Taneja wrote: > On Friday 31 August 2012 01:57 PM, Tomi Valkeinen wrote: >> On Fri, 2012-08-31 at 13:50 +0530, Archit Taneja wrote: >>> On Friday 31 August 2012 12:45 PM, Tomi Valkeinen wrote: >>>> On Fri, 2012-08-31 at 11:53 +0530, Archit Taneja wrote: >>>> >>>>> The only little problem was that during bootup, when hwmods are setup, >>>>> only the 'parent' hwmod was able to get reset properly, all the other >>>>> 'child' hwmods don't have modulemode bits tied to them, and hence >>>>> weren't able to reset. So we got some error prints. >>>>> >>>>> Once DSS driver kicks in, the driver ensures the parent is enabled for >>>>> any child to be enabled, so we don't face the issue again. >>>>> >>>>> So, if DSS driver is not built in, and if the bootloader left DSS in a >>>>> bad state, the DSS clocks might remain messed up all the time since >>>>> hwmod fwk wasn't able to reset them. >>>>> >>>>> I think this is why we didn't proceed with remove "dss_fck" as a slave >>>>> clock. If this issue is minor, we could go ahead and remove it. >>>> >>>> I wonder if we could handle this with a custom reset function. We >>>> already have a reset func for dss core. If I remember right, the main >>>> point for that is the fact that omap4 doesn't have a softreset for dss >>>> core, so we manually write the default values to registers. >>>> >>>> For omap2/3 this would be simple: skip the resets for all other dss >>>> submodules, and dss core's reset would enable all the clocks and set >>>> the >>>> softreset bit. This would reset all the submodules also. >>>> >>>> Omap4 is more tricky. I guess we'd need to enable all the clocks, clear >>>> manually dss core's registers, and then set softreset bits in all the >>>> submodules. So in this case dss core would need to have information >>>> about the other submodules. >>> >>> The is a good idea. I don't clearly understand your approach though. Are >>> you saying we have a custom reset function for only dss core? And reset >>> the submodules in it manually? >> >> Yes. >> >>> An alternative approach would be to implement custom reset functions for >>> each submodule(or each hwmod), and in the beginning of every reset >>> function, add a hack to enable MODULEMODE bits(since we don't want hwmod >>> fwk to touch MODULEMODE for the DSS submodules), and then set the soft >>> reset bits. >> >> I thought about that also. We'd need reset functions for all of them, >> and for omap2/3 we'd just reset the submodules again as they have >> already been reset with the dss core reset. >> >> The dss submodule resets are a bit linked. For omap2/3 the connection is >> obvious as dss core reset resets also the submodules, and for omap4 we >> have this requirement for the modulemode. That's why I though it'd be >> perhaps cleaner to handle the reset of the DSS block as a whole, in one >> place. >> >>> Your approach would ensure that we get a clean reset of DSS, but it >>> would still give the annoying prints when each of the submodule tries to >>> reset itself. >> >> The other submodules would not be reset by the hwmod framework at all, >> so there wouldn't be prints. I think there's a flag for that. Sorry for bringing up an old thread. I was working on cleaning up the OMAP4 DSS related clock/pm issues, hence brought it up. We were discussing here on how to setting up and reset the OMAP4 DSS submodules correctly without tying the MODULEMODE bits to the corresponding hwmods. Tomi, your suggestion was to do soft resets for the submodules manually in the dss_core hwmod's custom reset function itself, and use the flag HWMOD_INIT_NO_RESET to prevent _reset() being called. However, this won't still resolve the issue of the errors we see a bootup. The function _setup_reset() looks like this: static int _setup_reset(struct omap_hwmod *oh) { ... r = _enable(oh); if (r) { pr_warning("omap_hwmod: %s: cannot be enabled for reset (%d)\n", oh->name, oh->_state); return -EINVAL; } ... if (!(oh->flags & HWMOD_INIT_NO_RESET)) r = _reset(oh); ... } So, even if we have ask hwmod not to reset the DSS submodules, it will still try to enable them, and we can't enable them since MODULEMODE isn't tied to them. I don't see how we can get a clean reset done for the DSS submodules without making some changes in hwmod framework. Archit
Hi Archit, On 10/05/2012 11:46 AM, Archit Taneja wrote: > Hi, > > On Friday 31 August 2012 01:58 PM, Archit Taneja wrote: >> On Friday 31 August 2012 01:57 PM, Tomi Valkeinen wrote: >>> On Fri, 2012-08-31 at 13:50 +0530, Archit Taneja wrote: >>>> On Friday 31 August 2012 12:45 PM, Tomi Valkeinen wrote: >>>>> On Fri, 2012-08-31 at 11:53 +0530, Archit Taneja wrote: >>>>> >>>>>> The only little problem was that during bootup, when hwmods are >>>>>> setup, >>>>>> only the 'parent' hwmod was able to get reset properly, all the other >>>>>> 'child' hwmods don't have modulemode bits tied to them, and hence >>>>>> weren't able to reset. So we got some error prints. >>>>>> >>>>>> Once DSS driver kicks in, the driver ensures the parent is enabled >>>>>> for >>>>>> any child to be enabled, so we don't face the issue again. >>>>>> >>>>>> So, if DSS driver is not built in, and if the bootloader left DSS >>>>>> in a >>>>>> bad state, the DSS clocks might remain messed up all the time since >>>>>> hwmod fwk wasn't able to reset them. >>>>>> >>>>>> I think this is why we didn't proceed with remove "dss_fck" as a >>>>>> slave >>>>>> clock. If this issue is minor, we could go ahead and remove it. >>>>> >>>>> I wonder if we could handle this with a custom reset function. We >>>>> already have a reset func for dss core. If I remember right, the main >>>>> point for that is the fact that omap 4 doesn't have a softreset for dss >>>>> core, so we manually write the default values to registers. >>>>> >>>>> For omap2/3 this would be simple: skip the resets for all other dss >>>>> submodules, and dss core's reset would enable all the clocks and set >>>>> the >>>>> softreset bit. This would reset all the submodules also. >>>>> >>>>> Omap4 is more tricky. I guess we'd need to enable all the clocks, >>>>> clear >>>>> manually dss core's registers, and then set softreset bits in all the >>>>> submodules. So in this case dss core would need to have information >>>>> about the other submodules. >>>> >>>> The is a good idea. I don't clearly understand your approach though. >>>> Are >>>> you saying we have a custom reset function for only dss core? And reset >>>> the submodules in it manually? >>> >>> Yes. >>> >>>> An alternative approach would be to implement custom reset functions >>>> for >>>> each submodule(or each hwmod), and in the beginning of every reset >>>> function, add a hack to enable MODULEMODE bits(since we don't want >>>> hwmod >>>> fwk to touch MODULEMODE for the DSS submodules), and then set the soft >>>> reset bits. >>> >>> I thought about that also. We'd need reset functions for all of them, >>> and for omap2/3 we'd just reset the submodules again as they have >>> already been reset with the dss core reset. >>> >>> The dss submodule resets are a bit linked. For omap2/3 the connection is >>> obvious as dss core reset resets also the submodules, and for omap4 we >>> have this requirement for the modulemode. That's why I though it'd be >>> perhaps cleaner to handle the reset of the DSS block as a whole, in one >>> place. >>> >>>> Your approach would ensure that we get a clean reset of DSS, but it >>>> would still give the annoying prints when each of the submodule >>>> tries to >>>> reset itself. >>> >>> The other submodules would not be reset by the hwmod framework at all, >>> so there wouldn't be prints. I think there's a flag for that. > > Sorry for bringing up an old thread. I was working on cleaning up the > OMAP4 DSS related clock/pm issues, hence brought it up. > > We were discussing here on how to setting up and reset the OMAP4 DSS > submodules correctly without tying the MODULEMODE bits to the > corresponding hwmods. > > Tomi, your suggestion was to do soft resets for the submodules manually > in the dss_core hwmod's custom reset function itself, and use the flag > HWMOD_INIT_NO_RESET to prevent _reset() being called. Yep, that's the right approach. > However, this won't still resolve the issue of the errors we see a > bootup. The function _setup_reset() looks like this: > > static int _setup_reset(struct omap_hwmod *oh) > { > ... > r = _enable(oh); > if (r) { > pr_warning("omap_hwmod: %s: cannot be enabled for > reset (%d)\n", oh->name, oh->_state); > return -EINVAL; > } > ... > > if (!(oh->flags & HWMOD_INIT_NO_RESET)) > r = _reset(oh); > > ... > } > > So, even if we have ask hwmod not to reset the DSS submodules, it will > still try to enable them, and we can't enable them since MODULEMODE > isn't tied to them. I don't see how we can get a clean reset done for > the DSS submodules without making some changes in hwmod framework. Yeah, I do agree. Some module cannot but enabled automatically in the fmwk due to PM dependency. This is the case as well for MCPDM, IPU, DSP, ISS, FDIF... In that case the early setup should just be skipped and the DSS driver should take care of that during probe / pm_runtime_enable. I already have a WIP series that delay the setup until the driver probe the device. It will allow the setup to work properly in the case of the DSS assuming the DISPC and other sub IPs are setup in the context of DSS probe. At that time the DSS will be enabled already and thus every sub IPs will be able to get enabled. It is done in the context of DT boot, but should work as well for legacy boot. I can share the current crappy patches as soon as I fixed a couple of regression introduced by the patches :-( Regards, Benoit
On Friday 05 October 2012 05:50 PM, Benoit Cousson wrote: > Hi Archit, > > On 10/05/2012 11:46 AM, Archit Taneja wrote: >> Hi, >> >> On Friday 31 August 2012 01:58 PM, Archit Taneja wrote: >>> On Friday 31 August 2012 01:57 PM, Tomi Valkeinen wrote: >>>> On Fri, 2012-08-31 at 13:50 +0530, Archit Taneja wrote: >>>>> On Friday 31 August 2012 12:45 PM, Tomi Valkeinen wrote: >>>>>> On Fri, 2012-08-31 at 11:53 +0530, Archit Taneja wrote: >>>>>> >>>>>>> The only little problem was that during bootup, when hwmods are >>>>>>> setup, >>>>>>> only the 'parent' hwmod was able to get reset properly, all the other >>>>>>> 'child' hwmods don't have modulemode bits tied to them, and hence >>>>>>> weren't able to reset. So we got some error prints. >>>>>>> >>>>>>> Once DSS driver kicks in, the driver ensures the parent is enabled >>>>>>> for >>>>>>> any child to be enabled, so we don't face the issue again. >>>>>>> >>>>>>> So, if DSS driver is not built in, and if the bootloader left DSS >>>>>>> in a >>>>>>> bad state, the DSS clocks might remain messed up all the time since >>>>>>> hwmod fwk wasn't able to reset them. >>>>>>> >>>>>>> I think this is why we didn't proceed with remove "dss_fck" as a >>>>>>> slave >>>>>>> clock. If this issue is minor, we could go ahead and remove it. >>>>>> >>>>>> I wonder if we could handle this with a custom reset function. We >>>>>> already have a reset func for dss core. If I remember right, the main >>>>>> point for that is the fact that omap 4 doesn't have a softreset for dss >>>>>> core, so we manually write the default values to registers. >>>>>> >>>>>> For omap2/3 this would be simple: skip the resets for all other dss >>>>>> submodules, and dss core's reset would enable all the clocks and set >>>>>> the >>>>>> softreset bit. This would reset all the submodules also. >>>>>> >>>>>> Omap4 is more tricky. I guess we'd need to enable all the clocks, >>>>>> clear >>>>>> manually dss core's registers, and then set softreset bits in all the >>>>>> submodules. So in this case dss core would need to have information >>>>>> about the other submodules. >>>>> >>>>> The is a good idea. I don't clearly understand your approach though. >>>>> Are >>>>> you saying we have a custom reset function for only dss core? And reset >>>>> the submodules in it manually? >>>> >>>> Yes. >>>> >>>>> An alternative approach would be to implement custom reset functions >>>>> for >>>>> each submodule(or each hwmod), and in the beginning of every reset >>>>> function, add a hack to enable MODULEMODE bits(since we don't want >>>>> hwmod >>>>> fwk to touch MODULEMODE for the DSS submodules), and then set the soft >>>>> reset bits. >>>> >>>> I thought about that also. We'd need reset functions for all of them, >>>> and for omap2/3 we'd just reset the submodules again as they have >>>> already been reset with the dss core reset. >>>> >>>> The dss submodule resets are a bit linked. For omap2/3 the connection is >>>> obvious as dss core reset resets also the submodules, and for omap4 we >>>> have this requirement for the modulemode. That's why I though it'd be >>>> perhaps cleaner to handle the reset of the DSS block as a whole, in one >>>> place. >>>> >>>>> Your approach would ensure that we get a clean reset of DSS, but it >>>>> would still give the annoying prints when each of the submodule >>>>> tries to >>>>> reset itself. >>>> >>>> The other submodules would not be reset by the hwmod framework at all, >>>> so there wouldn't be prints. I think there's a flag for that. >> >> Sorry for bringing up an old thread. I was working on cleaning up the >> OMAP4 DSS related clock/pm issues, hence brought it up. >> >> We were discussing here on how to setting up and reset the OMAP4 DSS >> submodules correctly without tying the MODULEMODE bits to the >> corresponding hwmods. >> >> Tomi, your suggestion was to do soft resets for the submodules manually >> in the dss_core hwmod's custom reset function itself, and use the flag >> HWMOD_INIT_NO_RESET to prevent _reset() being called. > > Yep, that's the right approach. > >> However, this won't still resolve the issue of the errors we see a >> bootup. The function _setup_reset() looks like this: >> >> static int _setup_reset(struct omap_hwmod *oh) >> { >> ... >> r = _enable(oh); >> if (r) { >> pr_warning("omap_hwmod: %s: cannot be enabled for >> reset (%d)\n", oh->name, oh->_state); >> return -EINVAL; >> } >> ... >> >> if (!(oh->flags & HWMOD_INIT_NO_RESET)) >> r = _reset(oh); >> >> ... >> } >> >> So, even if we have ask hwmod not to reset the DSS submodules, it will >> still try to enable them, and we can't enable them since MODULEMODE >> isn't tied to them. I don't see how we can get a clean reset done for >> the DSS submodules without making some changes in hwmod framework. > > Yeah, I do agree. Some module cannot but enabled automatically in the fmwk due to PM dependency. > This is the case as well for MCPDM, IPU, DSP, ISS, FDIF... > > In that case the early setup should just be skipped and the DSS driver should take care of that during probe / pm_runtime_enable. > > I already have a WIP series that delay the setup until the driver probe the device. It will allow the setup to work properly in the case of the DSS assuming the DISPC and other sub IPs are setup in the context of DSS probe. At that time the DSS will be enabled already and thus every sub IPs will be able to get enabled. > > It is done in the context of DT boot, but should work as well for legacy boot. > I can share the current crappy patches as soon as I fixed a couple of regression introduced by the patches :-( Ah, that will be great! The other not so good option to make DSS PM work would be to add OCPIF_SWSUP_IDLE flag to our l3_main_2__dss_* slave interfaces(which have the hack "dss_fck" as slave clock). I gave this approach a try, that too isn't working so well. When I disable DSS, I get CM_DSS_DSS_CLKCTRL.IDLEST as 0x1, and CM_DSS_CLKSTCTRL.CLKACTIVITY_DSS_L3_ICLK is set. I wonder why that's happening. Thanks, Archit
On Friday 05 October 2012 05:59 PM, Archit Taneja wrote: > The other not so good option to make DSS PM work would be to add > OCPIF_SWSUP_IDLE flag to our l3_main_2__dss_* slave interfaces(which > have the hack "dss_fck" as slave clock). I gave this approach a try, > that too isn't working so well. When I disable DSS, I get > CM_DSS_DSS_CLKCTRL.IDLEST as 0x1, and > CM_DSS_CLKSTCTRL.CLKACTIVITY_DSS_L3_ICLK is set. I wonder why that's > happening. I have seen DSS get stuck in transition, with just a clkdm state toggle (from say HWSUP to SWWKUP) while its optional clocks are not running. Thats probably whats happening now. Did you try keeping the modulemode enabled and see if it really gates DSS/system sleep. I remember testing with Teros CORE ret/off patches and I was always seeing DSS modulemode enabled but it wasn't gating sleep.
On Friday 05 October 2012 06:07 PM, Rajendra Nayak wrote: > On Friday 05 October 2012 05:59 PM, Archit Taneja wrote: >> The other not so good option to make DSS PM work would be to add >> OCPIF_SWSUP_IDLE flag to our l3_main_2__dss_* slave interfaces(which >> have the hack "dss_fck" as slave clock). I gave this approach a try, >> that too isn't working so well. When I disable DSS, I get >> CM_DSS_DSS_CLKCTRL.IDLEST as 0x1, and >> CM_DSS_CLKSTCTRL.CLKACTIVITY_DSS_L3_ICLK is set. I wonder why that's >> happening. > > I have seen DSS get stuck in transition, with just a clkdm state toggle > (from say HWSUP to SWWKUP) while its optional clocks are not running. > Thats probably whats happening now. Oh ok, I can notice that too. So in the _idle() path, the clocks are disabled first, and then we try to change the clkdm state. I guess that could be the reason why DSS doesn't sleep. But then, I don't understand why this problem isn't seen if I try the alternative option of removing the fake dss_fck slave clock, and tie modulemode to only the parent hwmod. There DSS IDLEST is 0x3 when I disable DSS. I think with this approach, the problem is with _disable_clocks(), in disable_clocks, main_clk is disabled first, and then the slave clocks. That translates to DSS_FCK opt clock getting disabled first, and then MODULEMODE bits. I think DSS doesn't transition to sleep with this disable sequence. > > Did you try keeping the modulemode enabled and see if it really gates > DSS/system sleep. I remember testing with Teros CORE ret/off patches > and I was always seeing DSS modulemode enabled but it wasn't gating > sleep. If the clkdm is in HW_AUTO, I can get DSS in sleep(STBYST and IDLEST all set). Is this helpful? Can we just leave modulemode on all the time? That'll be the best :) Anyway, I guess it would be the best to have a custom _setup function(or skip them all together) for DSS as Benoit suggested. Archit
> If the clkdm is in HW_AUTO, I can get DSS in sleep(STBYST and IDLEST all > set). Is this helpful? Can we just leave modulemode on all the time? > That'll be the best :) Is everything around DSS enabled by default in omap2plus? If so, I haven't seen Tero (who has been working on getting OMAP4 to sleep) complain about DSS causing him any trouble. So you should be good with whats already there atleast from 'not-gating-sleep' point of view.
On Friday 05 October 2012 07:01 PM, Rajendra Nayak wrote: >> If the clkdm is in HW_AUTO, I can get DSS in sleep(STBYST and IDLEST all >> set). Is this helpful? Can we just leave modulemode on all the time? >> That'll be the best :) > > Is everything around DSS enabled by default in omap2plus? If so, I > haven't seen Tero (who has been working on getting OMAP4 to sleep) > complain about DSS causing him any trouble. So you should be good > with whats already there atleast from 'not-gating-sleep' point of > view. DSS is selected only as a module in omap2plus_defconfig, so the DSS driver would never kick in with the defconfig. The DSS hwmods would be initialised though. If I boot up linux-next with omap2plus_defconfig, I get: CM_DSS_CLKSTCTRL 0x3 CM_DSS_DSS_CLKCTRL 0x00060002 So the module is in standby, but IDLEST is 0x2, which says DSS is idle only with respect to the interconnect. In the bootloader, IDLEST was 0x3. So I don't know if that's a good thing or not. Archit
On 10/05/2012 03:20 PM, Archit Taneja wrote: > On Friday 05 October 2012 06:07 PM, Rajendra Nayak wrote: >> On Friday 05 October 2012 05:59 PM, Archit Taneja wrote: >>> The other not so good option to make DSS PM work would be to add >>> OCPIF_SWSUP_IDLE flag to our l3_main_2__dss_* slave interfaces(which >>> have the hack "dss_fck" as slave clock). I gave this approach a try, >>> that too isn't working so well. When I disable DSS, I get >>> CM_DSS_DSS_CLKCTRL.IDLEST as 0x1, and >>> CM_DSS_CLKSTCTRL.CLKACTIVITY_DSS_L3_ICLK is set. I wonder why that's >>> happening. >> >> I have seen DSS get stuck in transition, with just a clkdm state toggle >> (from say HWSUP to SWWKUP) while its optional clocks are not running. >> Thats probably whats happening now. > > Oh ok, I can notice that too. So in the _idle() path, the clocks are > disabled first, and then we try to change the clkdm state. I guess that > could be the reason why DSS doesn't sleep. > > But then, I don't understand why this problem isn't seen if I try the > alternative option of removing the fake dss_fck slave clock, and tie > modulemode to only the parent hwmod. There DSS IDLEST is 0x3 when I > disable DSS. > > I think with this approach, the problem is with _disable_clocks(), in > disable_clocks, main_clk is disabled first, and then the slave clocks. > That translates to DSS_FCK opt clock getting disabled first, and then > MODULEMODE bits. I think DSS doesn't transition to sleep with this > disable sequence. > >> >> Did you try keeping the modulemode enabled and see if it really gates >> DSS/system sleep. I remember testing with Teros CORE ret/off patches >> and I was always seeing DSS modulemode enabled but it wasn't gating >> sleep. > > If the clkdm is in HW_AUTO, I can get DSS in sleep(STBYST and IDLEST all > set). Is this helpful? Can we just leave modulemode on all the time? > That'll be the best :) Well, it is true that in the case of the DSS the modulemode is only managing the interface clock. And since the interface clock is doing autogating, it will not prevent clockdomain transition. But I will not recommend using that, it should not be useful assuming the clocks are properly managed by the DSS. I think we still have issue dur to the presence of that fake modulemode clock node. Regards, Benoit
diff --git a/arch/arm/mach-omap2/clock2420_data.c b/arch/arm/mach-omap2/clock2420_data.c index 0027451..49da781 100644 --- a/arch/arm/mach-omap2/clock2420_data.c +++ b/arch/arm/mach-omap2/clock2420_data.c @@ -1804,6 +1804,7 @@ static struct omap_clk omap2420_clks[] = { CLK(NULL, "gfx_ick", &gfx_ick, CK_242X), /* DSS domain clocks */ CLK("omapdss_dss", "ick", &dss_ick, CK_242X), + CLK(NULL, "dss_ick", &dss_ick, CK_242X), CLK(NULL, "dss1_fck", &dss1_fck, CK_242X), CLK(NULL, "dss2_fck", &dss2_fck, CK_242X), CLK(NULL, "dss_54m_fck", &dss_54m_fck, CK_242X), @@ -1843,12 +1844,16 @@ static struct omap_clk omap2420_clks[] = { CLK(NULL, "gpt12_ick", &gpt12_ick, CK_242X), CLK(NULL, "gpt12_fck", &gpt12_fck, CK_242X), CLK("omap-mcbsp.1", "ick", &mcbsp1_ick, CK_242X), + CLK(NULL, "mcbsp1_ick", &mcbsp1_ick, CK_242X), CLK(NULL, "mcbsp1_fck", &mcbsp1_fck, CK_242X), CLK("omap-mcbsp.2", "ick", &mcbsp2_ick, CK_242X), + CLK(NULL, "mcbsp2_ick", &mcbsp2_ick, CK_242X), CLK(NULL, "mcbsp2_fck", &mcbsp2_fck, CK_242X), CLK("omap2_mcspi.1", "ick", &mcspi1_ick, CK_242X), + CLK(NULL, "mcspi1_ick", &mcspi1_ick, CK_242X), CLK(NULL, "mcspi1_fck", &mcspi1_fck, CK_242X), CLK("omap2_mcspi.2", "ick", &mcspi2_ick, CK_242X), + CLK(NULL, "mcspi2_ick", &mcspi2_ick, CK_242X), CLK(NULL, "mcspi2_fck", &mcspi2_fck, CK_242X), CLK(NULL, "uart1_ick", &uart1_ick, CK_242X), CLK(NULL, "uart1_fck", &uart1_fck, CK_242X), @@ -1859,12 +1864,15 @@ static struct omap_clk omap2420_clks[] = { CLK(NULL, "gpios_ick", &gpios_ick, CK_242X), CLK(NULL, "gpios_fck", &gpios_fck, CK_242X), CLK("omap_wdt", "ick", &mpu_wdt_ick, CK_242X), + CLK(NULL, "mpu_wdt_ick", &mpu_wdt_ick, CK_242X), CLK(NULL, "mpu_wdt_fck", &mpu_wdt_fck, CK_242X), CLK(NULL, "sync_32k_ick", &sync_32k_ick, CK_242X), CLK(NULL, "wdt1_ick", &wdt1_ick, CK_242X), CLK(NULL, "omapctrl_ick", &omapctrl_ick, CK_242X), CLK("omap24xxcam", "fck", &cam_fck, CK_242X), + CLK(NULL, "cam_fck", &cam_fck, CK_242X), CLK("omap24xxcam", "ick", &cam_ick, CK_242X), + CLK(NULL, "cam_ick", &cam_ick, CK_242X), CLK(NULL, "mailboxes_ick", &mailboxes_ick, CK_242X), CLK(NULL, "wdt4_ick", &wdt4_ick, CK_242X), CLK(NULL, "wdt4_fck", &wdt4_fck, CK_242X), @@ -1873,16 +1881,22 @@ static struct omap_clk omap2420_clks[] = { CLK(NULL, "mspro_ick", &mspro_ick, CK_242X), CLK(NULL, "mspro_fck", &mspro_fck, CK_242X), CLK("mmci-omap.0", "ick", &mmc_ick, CK_242X), + CLK(NULL, "mmc_ick", &mmc_ick, CK_242X), CLK("mmci-omap.0", "fck", &mmc_fck, CK_242X), + CLK(NULL, "mmc_fck", &mmc_fck, CK_242X), CLK(NULL, "fac_ick", &fac_ick, CK_242X), CLK(NULL, "fac_fck", &fac_fck, CK_242X), CLK(NULL, "eac_ick", &eac_ick, CK_242X), CLK(NULL, "eac_fck", &eac_fck, CK_242X), CLK("omap_hdq.0", "ick", &hdq_ick, CK_242X), + CLK(NULL, "hdq_ick", &hdq_ick, CK_242X), CLK("omap_hdq.0", "fck", &hdq_fck, CK_242X), + CLK(NULL, "hdq_fck", &hdq_fck, CK_242X), CLK("omap_i2c.1", "ick", &i2c1_ick, CK_242X), + CLK(NULL, "i2c1_ick", &i2c1_ick, CK_242X), CLK(NULL, "i2c1_fck", &i2c1_fck, CK_242X), CLK("omap_i2c.2", "ick", &i2c2_ick, CK_242X), + CLK(NULL, "i2c2_ick", &i2c2_ick, CK_242X), CLK(NULL, "i2c2_fck", &i2c2_fck, CK_242X), CLK(NULL, "gpmc_fck", &gpmc_fck, CK_242X), CLK(NULL, "sdma_fck", &sdma_fck, CK_242X), @@ -1892,8 +1906,11 @@ static struct omap_clk omap2420_clks[] = { CLK(NULL, "vlynq_fck", &vlynq_fck, CK_242X), CLK(NULL, "des_ick", &des_ick, CK_242X), CLK("omap-sham", "ick", &sha_ick, CK_242X), + CLK(NULL, "sha_ick", &sha_ick, CK_242X), CLK("omap_rng", "ick", &rng_ick, CK_242X), + CLK(NULL, "rng_ick", &rng_ick, CK_242X), CLK("omap-aes", "ick", &aes_ick, CK_242X), + CLK(NULL, "aes_ick", &aes_ick, CK_242X), CLK(NULL, "pka_ick", &pka_ick, CK_242X), CLK(NULL, "usb_fck", &usb_fck, CK_242X), CLK("musb-hdrc", "fck", &osc_ck, CK_242X), diff --git a/arch/arm/mach-omap2/clock2430_data.c b/arch/arm/mach-omap2/clock2430_data.c index cacabb0..bcf86a7 100644 --- a/arch/arm/mach-omap2/clock2430_data.c +++ b/arch/arm/mach-omap2/clock2430_data.c @@ -1887,6 +1887,7 @@ static struct omap_clk omap2430_clks[] = { CLK(NULL, "mdm_osc_ck", &mdm_osc_ck, CK_243X), /* DSS domain clocks */ CLK("omapdss_dss", "ick", &dss_ick, CK_243X), + CLK(NULL, "dss_ick", &dss_ick, CK_243X), CLK(NULL, "dss1_fck", &dss1_fck, CK_243X), CLK(NULL, "dss2_fck", &dss2_fck, CK_243X), CLK(NULL, "dss_54m_fck", &dss_54m_fck, CK_243X), @@ -1926,20 +1927,28 @@ static struct omap_clk omap2430_clks[] = { CLK(NULL, "gpt12_ick", &gpt12_ick, CK_243X), CLK(NULL, "gpt12_fck", &gpt12_fck, CK_243X), CLK("omap-mcbsp.1", "ick", &mcbsp1_ick, CK_243X), + CLK(NULL, "mcbsp1_ick", &mcbsp1_ick, CK_243X), CLK(NULL, "mcbsp1_fck", &mcbsp1_fck, CK_243X), CLK("omap-mcbsp.2", "ick", &mcbsp2_ick, CK_243X), + CLK(NULL, "mcbsp2_ick", &mcbsp2_ick, CK_243X), CLK(NULL, "mcbsp2_fck", &mcbsp2_fck, CK_243X), CLK("omap-mcbsp.3", "ick", &mcbsp3_ick, CK_243X), + CLK(NULL, "mcbsp3_ick", &mcbsp3_ick, CK_243X), CLK(NULL, "mcbsp3_fck", &mcbsp3_fck, CK_243X), CLK("omap-mcbsp.4", "ick", &mcbsp4_ick, CK_243X), + CLK(NULL, "mcbsp4_ick", &mcbsp4_ick, CK_243X), CLK(NULL, "mcbsp4_fck", &mcbsp4_fck, CK_243X), CLK("omap-mcbsp.5", "ick", &mcbsp5_ick, CK_243X), + CLK(NULL, "mcbsp5_ick", &mcbsp5_ick, CK_243X), CLK(NULL, "mcbsp5_fck", &mcbsp5_fck, CK_243X), CLK("omap2_mcspi.1", "ick", &mcspi1_ick, CK_243X), + CLK(NULL, "mcspi1_ick", &mcspi1_ick, CK_243X), CLK(NULL, "mcspi1_fck", &mcspi1_fck, CK_243X), CLK("omap2_mcspi.2", "ick", &mcspi2_ick, CK_243X), + CLK(NULL, "mcspi2_ick", &mcspi2_ick, CK_243X), CLK(NULL, "mcspi2_fck", &mcspi2_fck, CK_243X), CLK("omap2_mcspi.3", "ick", &mcspi3_ick, CK_243X), + CLK(NULL, "mcspi3_ick", &mcspi3_ick, CK_243X), CLK(NULL, "mcspi3_fck", &mcspi3_fck, CK_243X), CLK(NULL, "uart1_ick", &uart1_ick, CK_243X), CLK(NULL, "uart1_fck", &uart1_fck, CK_243X), @@ -1950,13 +1959,16 @@ static struct omap_clk omap2430_clks[] = { CLK(NULL, "gpios_ick", &gpios_ick, CK_243X), CLK(NULL, "gpios_fck", &gpios_fck, CK_243X), CLK("omap_wdt", "ick", &mpu_wdt_ick, CK_243X), + CLK(NULL, "mpu_wdt_ick", &mpu_wdt_ick, CK_243X), CLK(NULL, "mpu_wdt_fck", &mpu_wdt_fck, CK_243X), CLK(NULL, "sync_32k_ick", &sync_32k_ick, CK_243X), CLK(NULL, "wdt1_ick", &wdt1_ick, CK_243X), CLK(NULL, "omapctrl_ick", &omapctrl_ick, CK_243X), CLK(NULL, "icr_ick", &icr_ick, CK_243X), CLK("omap24xxcam", "fck", &cam_fck, CK_243X), + CLK(NULL, "cam_fck", &cam_fck, CK_243X), CLK("omap24xxcam", "ick", &cam_ick, CK_243X), + CLK(NULL, "cam_ick", &cam_ick, CK_243X), CLK(NULL, "mailboxes_ick", &mailboxes_ick, CK_243X), CLK(NULL, "wdt4_ick", &wdt4_ick, CK_243X), CLK(NULL, "wdt4_fck", &wdt4_fck, CK_243X), @@ -1965,10 +1977,14 @@ static struct omap_clk omap2430_clks[] = { CLK(NULL, "fac_ick", &fac_ick, CK_243X), CLK(NULL, "fac_fck", &fac_fck, CK_243X), CLK("omap_hdq.0", "ick", &hdq_ick, CK_243X), + CLK(NULL, "hdq_ick", &hdq_ick, CK_243X), CLK("omap_hdq.1", "fck", &hdq_fck, CK_243X), + CLK(NULL, "hdq_fck", &hdq_fck, CK_243X), CLK("omap_i2c.1", "ick", &i2c1_ick, CK_243X), + CLK(NULL, "i2c1_ick", &i2c1_ick, CK_243X), CLK(NULL, "i2chs1_fck", &i2chs1_fck, CK_243X), CLK("omap_i2c.2", "ick", &i2c2_ick, CK_243X), + CLK(NULL, "i2c2_ick", &i2c2_ick, CK_243X), CLK(NULL, "i2chs2_fck", &i2chs2_fck, CK_243X), CLK(NULL, "gpmc_fck", &gpmc_fck, CK_243X), CLK(NULL, "sdma_fck", &sdma_fck, CK_243X), @@ -1981,15 +1997,20 @@ static struct omap_clk omap2430_clks[] = { CLK(NULL, "pka_ick", &pka_ick, CK_243X), CLK(NULL, "usb_fck", &usb_fck, CK_243X), CLK("musb-omap2430", "ick", &usbhs_ick, CK_243X), + CLK(NULL, "usbhs_ick", &usbhs_ick, CK_243X), CLK("omap_hsmmc.0", "ick", &mmchs1_ick, CK_243X), + CLK(NULL, "mmchs1_ick", &mmchs1_ick, CK_243X), CLK(NULL, "mmchs1_fck", &mmchs1_fck, CK_243X), CLK("omap_hsmmc.1", "ick", &mmchs2_ick, CK_243X), + CLK(NULL, "mmchs2_ick", &mmchs2_ick, CK_243X), CLK(NULL, "mmchs2_fck", &mmchs2_fck, CK_243X), CLK(NULL, "gpio5_ick", &gpio5_ick, CK_243X), CLK(NULL, "gpio5_fck", &gpio5_fck, CK_243X), CLK(NULL, "mdm_intc_ick", &mdm_intc_ick, CK_243X), CLK("omap_hsmmc.0", "mmchsdb_fck", &mmchsdb1_fck, CK_243X), + CLK(NULL, "mmchsdb1_fck", &mmchsdb1_fck, CK_243X), CLK("omap_hsmmc.1", "mmchsdb_fck", &mmchsdb2_fck, CK_243X), + CLK(NULL, "mmchsdb2_fck", &mmchsdb2_fck, CK_243X), CLK(NULL, "timer_32k_ck", &func_32k_ck, CK_243X), CLK(NULL, "timer_sys_ck", &sys_ck, CK_243X), CLK(NULL, "timer_ext_ck", &alt_ck, CK_243X), diff --git a/arch/arm/mach-omap2/clock3xxx_data.c b/arch/arm/mach-omap2/clock3xxx_data.c index 83bed9a..405f4b6 100644 --- a/arch/arm/mach-omap2/clock3xxx_data.c +++ b/arch/arm/mach-omap2/clock3xxx_data.c @@ -3247,6 +3247,7 @@ static struct omap_clk omap3xxx_clks[] = { CLK(NULL, "dpll4_x2_ck", &dpll4_x2_ck, CK_3XXX), CLK(NULL, "omap_192m_alwon_fck", &omap_192m_alwon_fck, CK_36XX), CLK(NULL, "omap_96m_alwon_fck", &omap_96m_alwon_fck, CK_3XXX), + CLK(NULL, "omap_96m_alwon_fck_3630", &omap_96m_alwon_fck_3630, CK_3XXX), CLK(NULL, "omap_96m_fck", &omap_96m_fck, CK_3XXX), CLK(NULL, "cm_96m_fck", &cm_96m_fck, CK_3XXX), CLK(NULL, "omap_54m_fck", &omap_54m_fck, CK_3XXX), @@ -3314,6 +3315,7 @@ static struct omap_clk omap3xxx_clks[] = { CLK(NULL, "fshostusb_fck", &fshostusb_fck, CK_3430ES1), CLK(NULL, "core_12m_fck", &core_12m_fck, CK_3XXX), CLK("omap_hdq.0", "fck", &hdq_fck, CK_3XXX), + CLK(NULL, "hdq_fck", &hdq_fck, CK_3XXX), CLK(NULL, "ssi_ssr_fck", &ssi_ssr_fck_3430es1, CK_3430ES1), CLK(NULL, "ssi_ssr_fck", &ssi_ssr_fck_3430es2, CK_3430ES2PLUS | CK_36XX), CLK(NULL, "ssi_sst_fck", &ssi_sst_fck_3430es1, CK_3430ES1), @@ -3321,6 +3323,8 @@ static struct omap_clk omap3xxx_clks[] = { CLK(NULL, "core_l3_ick", &core_l3_ick, CK_3XXX), CLK("musb-omap2430", "ick", &hsotgusb_ick_3430es1, CK_3430ES1), CLK("musb-omap2430", "ick", &hsotgusb_ick_3430es2, CK_3430ES2PLUS | CK_36XX), + CLK(NULL, "hsotgusb_ick", &hsotgusb_ick_3430es1, CK_3430ES1), + CLK(NULL, "hsotgusb_ick", &hsotgusb_ick_3430es2, CK_3430ES2PLUS | CK_36XX), CLK(NULL, "sdrc_ick", &sdrc_ick, CK_3XXX), CLK(NULL, "gpmc_fck", &gpmc_fck, CK_3XXX), CLK(NULL, "security_l3_ick", &security_l3_ick, CK_34XX | CK_36XX), @@ -3329,27 +3333,40 @@ static struct omap_clk omap3xxx_clks[] = { CLK(NULL, "usbtll_ick", &usbtll_ick, CK_3430ES2PLUS | CK_AM35XX | CK_36XX), CLK("usbhs_omap", "usbtll_ick", &usbtll_ick, CK_3430ES2PLUS | CK_AM35XX | CK_36XX), CLK("omap_hsmmc.2", "ick", &mmchs3_ick, CK_3430ES2PLUS | CK_AM35XX | CK_36XX), + CLK(NULL, "mmchs3_ick", &mmchs3_ick, CK_3430ES2PLUS | CK_AM35XX | CK_36XX), CLK(NULL, "icr_ick", &icr_ick, CK_34XX | CK_36XX), CLK("omap-aes", "ick", &aes2_ick, CK_34XX | CK_36XX), CLK("omap-sham", "ick", &sha12_ick, CK_34XX | CK_36XX), CLK(NULL, "des2_ick", &des2_ick, CK_34XX | CK_36XX), CLK("omap_hsmmc.1", "ick", &mmchs2_ick, CK_3XXX), CLK("omap_hsmmc.0", "ick", &mmchs1_ick, CK_3XXX), + CLK(NULL, "mmchs2_ick", &mmchs2_ick, CK_3XXX), + CLK(NULL, "mmchs1_ick", &mmchs1_ick, CK_3XXX), CLK(NULL, "mspro_ick", &mspro_ick, CK_34XX | CK_36XX), CLK("omap_hdq.0", "ick", &hdq_ick, CK_3XXX), + CLK(NULL, "hdq_ick", &hdq_ick, CK_3XXX), CLK("omap2_mcspi.4", "ick", &mcspi4_ick, CK_3XXX), CLK("omap2_mcspi.3", "ick", &mcspi3_ick, CK_3XXX), CLK("omap2_mcspi.2", "ick", &mcspi2_ick, CK_3XXX), CLK("omap2_mcspi.1", "ick", &mcspi1_ick, CK_3XXX), + CLK(NULL, "mcspi4_ick", &mcspi4_ick, CK_3XXX), + CLK(NULL, "mcspi3_ick", &mcspi3_ick, CK_3XXX), + CLK(NULL, "mcspi2_ick", &mcspi2_ick, CK_3XXX), + CLK(NULL, "mcspi1_ick", &mcspi1_ick, CK_3XXX), CLK("omap_i2c.3", "ick", &i2c3_ick, CK_3XXX), CLK("omap_i2c.2", "ick", &i2c2_ick, CK_3XXX), CLK("omap_i2c.1", "ick", &i2c1_ick, CK_3XXX), + CLK(NULL, "i2c3_ick", &i2c3_ick, CK_3XXX), + CLK(NULL, "i2c2_ick", &i2c2_ick, CK_3XXX), + CLK(NULL, "i2c1_ick", &i2c1_ick, CK_3XXX), CLK(NULL, "uart2_ick", &uart2_ick, CK_3XXX), CLK(NULL, "uart1_ick", &uart1_ick, CK_3XXX), CLK(NULL, "gpt11_ick", &gpt11_ick, CK_3XXX), CLK(NULL, "gpt10_ick", &gpt10_ick, CK_3XXX), CLK("omap-mcbsp.5", "ick", &mcbsp5_ick, CK_3XXX), CLK("omap-mcbsp.1", "ick", &mcbsp1_ick, CK_3XXX), + CLK(NULL, "mcbsp5_ick", &mcbsp5_ick, CK_3XXX), + CLK(NULL, "mcbsp1_ick", &mcbsp1_ick, CK_3XXX), CLK(NULL, "fac_ick", &fac_ick, CK_3430ES1), CLK(NULL, "mailboxes_ick", &mailboxes_ick, CK_34XX | CK_36XX), CLK(NULL, "omapctrl_ick", &omapctrl_ick, CK_3XXX), @@ -3368,7 +3385,9 @@ static struct omap_clk omap3xxx_clks[] = { CLK(NULL, "dss_96m_fck", &dss_96m_fck, CK_3XXX), CLK(NULL, "dss2_alwon_fck", &dss2_alwon_fck, CK_3XXX), CLK("omapdss_dss", "ick", &dss_ick_3430es1, CK_3430ES1), + CLK(NULL, "dss_ick", &dss_ick_3430es1, CK_3430ES1), CLK("omapdss_dss", "ick", &dss_ick_3430es2, CK_3430ES2PLUS | CK_AM35XX | CK_36XX), + CLK(NULL, "dss_ick", &dss_ick_3430es2, CK_3430ES2PLUS | CK_AM35XX | CK_36XX), CLK(NULL, "cam_mclk", &cam_mclk, CK_34XX | CK_36XX), CLK(NULL, "cam_ick", &cam_ick, CK_34XX | CK_36XX), CLK(NULL, "csi2_96m_fck", &csi2_96m_fck, CK_34XX | CK_36XX), @@ -3393,6 +3412,7 @@ static struct omap_clk omap3xxx_clks[] = { CLK(NULL, "wkup_l4_ick", &wkup_l4_ick, CK_34XX | CK_36XX), CLK(NULL, "usim_ick", &usim_ick, CK_3430ES2PLUS | CK_36XX), CLK("omap_wdt", "ick", &wdt2_ick, CK_3XXX), + CLK(NULL, "wdt2_ick", &wdt2_ick, CK_3XXX), CLK(NULL, "wdt1_ick", &wdt1_ick, CK_3XXX), CLK(NULL, "gpio1_ick", &gpio1_ick, CK_3XXX), CLK(NULL, "omap_32ksync_ick", &omap_32ksync_ick, CK_3XXX), @@ -3438,6 +3458,9 @@ static struct omap_clk omap3xxx_clks[] = { CLK("omap-mcbsp.2", "ick", &mcbsp2_ick, CK_3XXX), CLK("omap-mcbsp.3", "ick", &mcbsp3_ick, CK_3XXX), CLK("omap-mcbsp.4", "ick", &mcbsp4_ick, CK_3XXX), + CLK(NULL, "mcbsp4_ick", &mcbsp2_ick, CK_3XXX), + CLK(NULL, "mcbsp3_ick", &mcbsp3_ick, CK_3XXX), + CLK(NULL, "mcbsp2_ick", &mcbsp4_ick, CK_3XXX), CLK(NULL, "mcbsp2_fck", &mcbsp2_fck, CK_3XXX), CLK(NULL, "mcbsp3_fck", &mcbsp3_fck, CK_3XXX), CLK(NULL, "mcbsp4_fck", &mcbsp4_fck, CK_3XXX), diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c index d7f55e4..a3831a2 100644 --- a/arch/arm/mach-omap2/clock44xx_data.c +++ b/arch/arm/mach-omap2/clock44xx_data.c @@ -3156,6 +3156,7 @@ static struct omap_clk omap44xx_clks[] = { CLK(NULL, "dss_tv_clk", &dss_tv_clk, CK_443X), CLK(NULL, "dss_48mhz_clk", &dss_48mhz_clk, CK_443X), CLK(NULL, "dss_dss_clk", &dss_dss_clk, CK_443X), + CLK(NULL, "dss_fck", &dss_fck, CK_443X), CLK("omapdss_dss", "ick", &dss_fck, CK_443X), CLK(NULL, "efuse_ctrl_cust_fck", &efuse_ctrl_cust_fck, CK_443X), CLK(NULL, "emif1_fck", &emif1_fck, CK_443X), @@ -3212,6 +3213,7 @@ static struct omap_clk omap44xx_clks[] = { CLK(NULL, "ocp2scp_usb_phy_phy_48m", &ocp2scp_usb_phy_phy_48m, CK_443X), CLK(NULL, "ocp2scp_usb_phy_ick", &ocp2scp_usb_phy_ick, CK_443X), CLK(NULL, "ocp_wp_noc_ick", &ocp_wp_noc_ick, CK_443X), + CLK(NULL, "rng_ick", &rng_ick, CK_443X), CLK("omap_rng", "ick", &rng_ick, CK_443X), CLK(NULL, "sha2md5_fck", &sha2md5_fck, CK_443X), CLK(NULL, "sl2if_ick", &sl2if_ick, CK_443X), @@ -3243,6 +3245,7 @@ static struct omap_clk omap44xx_clks[] = { CLK(NULL, "uart3_fck", &uart3_fck, CK_443X), CLK(NULL, "uart4_fck", &uart4_fck, CK_443X), CLK("usbhs_omap", "fs_fck", &usb_host_fs_fck, CK_443X), + CLK(NULL, "usb_host_fs_fck", &usb_host_fs_fck, CK_443X), CLK(NULL, "utmi_p1_gfclk", &utmi_p1_gfclk, CK_443X), CLK(NULL, "usb_host_hs_utmi_p1_clk", &usb_host_hs_utmi_p1_clk, CK_443X), CLK(NULL, "utmi_p2_gfclk", &utmi_p2_gfclk, CK_443X), @@ -3253,14 +3256,17 @@ static struct omap_clk omap44xx_clks[] = { CLK(NULL, "usb_host_hs_hsic60m_p2_clk", &usb_host_hs_hsic60m_p2_clk, CK_443X), CLK(NULL, "usb_host_hs_hsic480m_p2_clk", &usb_host_hs_hsic480m_p2_clk, CK_443X), CLK(NULL, "usb_host_hs_func48mclk", &usb_host_hs_func48mclk, CK_443X), + CLK(NULL, "usb_host_hs_fck", &usb_host_hs_fck, CK_443X), CLK("usbhs_omap", "hs_fck", &usb_host_hs_fck, CK_443X), CLK(NULL, "otg_60m_gfclk", &otg_60m_gfclk, CK_443X), CLK(NULL, "usb_otg_hs_xclk", &usb_otg_hs_xclk, CK_443X), + CLK(NULL, "usb_otg_hs_ick", &usb_otg_hs_ick, CK_443X), CLK("musb-omap2430", "ick", &usb_otg_hs_ick, CK_443X), CLK(NULL, "usb_phy_cm_clk32k", &usb_phy_cm_clk32k, CK_443X), CLK(NULL, "usb_tll_hs_usb_ch2_clk", &usb_tll_hs_usb_ch2_clk, CK_443X), CLK(NULL, "usb_tll_hs_usb_ch0_clk", &usb_tll_hs_usb_ch0_clk, CK_443X), CLK(NULL, "usb_tll_hs_usb_ch1_clk", &usb_tll_hs_usb_ch1_clk, CK_443X), + CLK(NULL, "usb_tll_hs_ick", &usb_tll_hs_ick, CK_443X), CLK("usbhs_omap", "usbtll_ick", &usb_tll_hs_ick, CK_443X), CLK(NULL, "usim_ck", &usim_ck, CK_443X), CLK(NULL, "usim_fclk", &usim_fclk, CK_443X), diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index fb3e110..c77ab5b 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -677,8 +677,8 @@ static int _init_main_clk(struct omap_hwmod *oh) if (!oh->main_clk) return 0; - oh->_clk = omap_clk_get_by_name(oh->main_clk); - if (!oh->_clk) { + oh->_clk = clk_get(NULL, oh->main_clk); + if (IS_ERR_OR_NULL(oh->_clk)) { pr_warning("omap_hwmod: %s: cannot clk_get main_clk %s\n", oh->name, oh->main_clk); return -EINVAL; @@ -722,8 +722,8 @@ static int _init_interface_clks(struct omap_hwmod *oh) if (!os->clk) continue; - c = omap_clk_get_by_name(os->clk); - if (!c) { + c = clk_get(NULL, os->clk); + if (IS_ERR_OR_NULL(c)) { pr_warning("omap_hwmod: %s: cannot clk_get interface_clk %s\n", oh->name, os->clk); ret = -EINVAL; @@ -758,8 +758,8 @@ static int _init_opt_clks(struct omap_hwmod *oh) int ret = 0; for (i = oh->opt_clks_cnt, oc = oh->opt_clks; i > 0; i--, oc++) { - c = omap_clk_get_by_name(oc->clk); - if (!c) { + c = clk_get(NULL, oc->clk); + if (IS_ERR_OR_NULL(c)) { pr_warning("omap_hwmod: %s: cannot clk_get opt_clk %s\n", oh->name, oc->clk); ret = -EINVAL; diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c index 706b7e2..9d7ac20 100644 --- a/arch/arm/plat-omap/clock.c +++ b/arch/arm/plat-omap/clock.c @@ -312,33 +312,6 @@ void clk_enable_init_clocks(void) } } -/** - * omap_clk_get_by_name - locate OMAP struct clk by its name - * @name: name of the struct clk to locate - * - * Locate an OMAP struct clk by its name. Assumes that struct clk - * names are unique. Returns NULL if not found or a pointer to the - * struct clk if found. - */ -struct clk *omap_clk_get_by_name(const char *name) -{ - struct clk *c; - struct clk *ret = NULL; - - mutex_lock(&clocks_mutex); - - list_for_each_entry(c, &clocks, node) { - if (!strcmp(c->name, name)) { - ret = c; - break; - } - } - - mutex_unlock(&clocks_mutex); - - return ret; -} - int omap_clk_enable_autoidle_all(void) { struct clk *c; diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c index c490240..af822d5 100644 --- a/arch/arm/plat-omap/omap_device.c +++ b/arch/arm/plat-omap/omap_device.c @@ -266,10 +266,10 @@ static void _add_clkdev(struct omap_device *od, const char *clk_alias, return; } - r = omap_clk_get_by_name(clk_name); - if (IS_ERR(r)) { + r = clk_get(NULL, clk_name); + if (IS_ERR_OR_NULL(r)) { dev_err(&od->pdev->dev, - "omap_clk_get_by_name for %s failed\n", clk_name); + "clk_get for %s failed\n", clk_name); return; }
Moving to Common clk framework for OMAP would mean we no longer use internal lookup mechanism like omap_clk_get_by_name(). get rid of all its usage mostly from hwmod and omap_device code. Also use IS_ERR_OR_NULL() for error checking. Moving to clk_get() also means the respective platforms need the clkdev tables updated with an entry for all clocks used by hwmod to have clock name same as the alias. Based on original changes from Mike Turquette. Signed-off-by: Rajendra Nayak <rnayak@ti.com> --- arch/arm/mach-omap2/clock2420_data.c | 17 +++++++++++++++++ arch/arm/mach-omap2/clock2430_data.c | 21 +++++++++++++++++++++ arch/arm/mach-omap2/clock3xxx_data.c | 23 +++++++++++++++++++++++ arch/arm/mach-omap2/clock44xx_data.c | 6 ++++++ arch/arm/mach-omap2/omap_hwmod.c | 12 ++++++------ arch/arm/plat-omap/clock.c | 27 --------------------------- arch/arm/plat-omap/omap_device.c | 6 +++--- 7 files changed, 76 insertions(+), 36 deletions(-)