| Message ID | 1310685865-3249-1-git-send-email-jon-hunter@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
cc'ing Benoît Hi Jon On Thu, 14 Jul 2011, Jon Hunter wrote: > From: Jon Hunter <jon-hunter@ti.com> > > The parent clock of the OCP_ABE_ICLK is the AESS_FCLK and the > parent clock of the AESS_FCLK is the ABE_FCLK... > > ABE_FCLK --> AESS_FCLK --> OCP_ABE_ICLK > > The AESS_FCLK and OCP_ABE_ICLK clocks both have dividers which > determine their operational frequency. However, the dividers for > the AESS_FCLK and OCP_ABE_ICLK are controlled via a single bit, > which is the CM1_ABE_AESS_CLKCTRL[24] bit.> When this bit is set to > 0, the AESS_FCLK divider is 1 and the OCP_ABE_ICLK divider is 2. > Similarly, when this bit is set to 1, the AESS_FCLK divider is 2 > and the OCP_ABE_ICLK is 1. Sigh. This type of hardware design makes software design difficult :-( > The above relationship between the AESS_FCLK and OCP_ABE_ICLK > dividers ensure that the OCP_ABE_ICLK clock is always half the > frequency of the ABE_CLK... > > OCP_ABE_ICLK = ABE_FCLK/2 > > The divider for the OCP_ABE_ICLK is currently missing so add a > divider that will ensure the OCP_ABE_ICLK frequency is always half > the ABE_FCLK frequency. > > Signed-off-by: Jon Hunter <jon-hunter@ti.com> > --- > arch/arm/mach-omap2/clock44xx_data.c | 16 +++++++++++++++- > 1 files changed, 15 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c > index 8c96567..6e158ce 100644 > --- a/arch/arm/mach-omap2/clock44xx_data.c > +++ b/arch/arm/mach-omap2/clock44xx_data.c > @@ -1301,11 +1301,25 @@ static struct clk mcasp3_fclk = { > .recalc = &followparent_recalc, > }; > > +static const struct clksel_rate div2_2to1_rates[] = { > + { .div = 1, .val = 1, .flags = RATE_IN_4430 }, > + { .div = 2, .val = 0, .flags = RATE_IN_4430 }, > + { .div = 0 }, > +}; > + > +static const struct clksel ocp_abe_iclk_div[] = { > + { .parent = &aess_fclk, .rates = div2_2to1_rates }, > + { .parent = NULL }, > +}; > + > static struct clk ocp_abe_iclk = { > .name = "ocp_abe_iclk", > .parent = &aess_fclk, > + .clksel = ocp_abe_iclk_div, > + .clksel_reg = OMAP4430_CM1_ABE_AESS_CLKCTRL, > + .clksel_mask = OMAP4430_CLKSEL_AESS_FCLK_MASK, > .ops = &clkops_null, > - .recalc = &followparent_recalc, > + .recalc = &omap2_clksel_recalc, > }; > > static struct clk per_abe_24m_fclk = { I guess the reason that this patch can get away with this is because it doesn't allow software to change the rate of ocp_abe_iclk, and the ocp_abe_iclk is a child of aess_fclk, so when aess_fclk is changed, it will recalc ocp_abe_iclk. Benoît, what do you think? Is this a reasonable approach for the script? Or do we need to deal with some kind of 'linked clock' implementation... - Paul
Hi Paul, On 7/15/2011 3:21, Paul Walmsley wrote: > cc'ing Benoît > > Hi Jon > > On Thu, 14 Jul 2011, Jon Hunter wrote: > >> From: Jon Hunter<jon-hunter@ti.com> >> >> The parent clock of the OCP_ABE_ICLK is the AESS_FCLK and the >> parent clock of the AESS_FCLK is the ABE_FCLK... >> >> ABE_FCLK --> AESS_FCLK --> OCP_ABE_ICLK >> >> The AESS_FCLK and OCP_ABE_ICLK clocks both have dividers which >> determine their operational frequency. However, the dividers for >> the AESS_FCLK and OCP_ABE_ICLK are controlled via a single bit, >> which is the CM1_ABE_AESS_CLKCTRL[24] bit.> When this bit is set to >> 0, the AESS_FCLK divider is 1 and the OCP_ABE_ICLK divider is 2. >> Similarly, when this bit is set to 1, the AESS_FCLK divider is 2 >> and the OCP_ABE_ICLK is 1. > > Sigh. This type of hardware design makes software design difficult :-( Hopefully, because this is a interface clock the impact is really minimal...more below... >> The above relationship between the AESS_FCLK and OCP_ABE_ICLK >> dividers ensure that the OCP_ABE_ICLK clock is always half the >> frequency of the ABE_CLK... >> >> OCP_ABE_ICLK = ABE_FCLK/2 >> >> The divider for the OCP_ABE_ICLK is currently missing so add a >> divider that will ensure the OCP_ABE_ICLK frequency is always half >> the ABE_FCLK frequency. >> >> Signed-off-by: Jon Hunter<jon-hunter@ti.com> >> --- >> arch/arm/mach-omap2/clock44xx_data.c | 16 +++++++++++++++- >> 1 files changed, 15 insertions(+), 1 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c >> index 8c96567..6e158ce 100644 >> --- a/arch/arm/mach-omap2/clock44xx_data.c >> +++ b/arch/arm/mach-omap2/clock44xx_data.c >> @@ -1301,11 +1301,25 @@ static struct clk mcasp3_fclk = { >> .recalc =&followparent_recalc, >> }; >> >> +static const struct clksel_rate div2_2to1_rates[] = { >> + { .div = 1, .val = 1, .flags = RATE_IN_4430 }, >> + { .div = 2, .val = 0, .flags = RATE_IN_4430 }, >> + { .div = 0 }, >> +}; >> + >> +static const struct clksel ocp_abe_iclk_div[] = { >> + { .parent =&aess_fclk, .rates = div2_2to1_rates }, >> + { .parent = NULL }, >> +}; >> + >> static struct clk ocp_abe_iclk = { >> .name = "ocp_abe_iclk", >> .parent =&aess_fclk, >> + .clksel = ocp_abe_iclk_div, >> + .clksel_reg = OMAP4430_CM1_ABE_AESS_CLKCTRL, >> + .clksel_mask = OMAP4430_CLKSEL_AESS_FCLK_MASK, >> .ops =&clkops_null, >> - .recalc =&followparent_recalc, >> + .recalc =&omap2_clksel_recalc, >> }; >> >> static struct clk per_abe_24m_fclk = { > > I guess the reason that this patch can get away with this is because it > doesn't allow software to change the rate of ocp_abe_iclk, and the > ocp_abe_iclk is a child of aess_fclk, so when aess_fclk is changed, it > will recalc ocp_abe_iclk. > > Benoît, what do you think? Is this a reasonable approach for the script? > Or do we need to deal with some kind of 'linked clock' implementation... If you want my two cents on this matter, I would say that... 1). People should not need to configure the "ocp_abe_iclk" clock directly, because regardless of the divider setting it is always 1/2 the "abe_fclk". In other words, only the "aess_fclk" frequency is really changing because of the divider and the above relationship ensures that the "ocp_abe_iclk" is always the same frequency. So a user only cares about the "aess_fclk" frequency and the "ocp_abe_iclk" frequency is handled for them. 2). The "ocp_abe_iclk" is an interface clock and is not a parent to any other functional clock and therefore, is not driving any internal logic in a peripheral which would have a direct impact of the speed of that peripheral. However, there does appear to be another bug here in the clock44xx_data.c as it shows that the "ocp_abe_iclk" is parent to the "slimbus1_fck" which I believe is incorrect. According to the TRM, the the ocp_abe_iclk is parent to the slimbus1_iclk. I can send another patch for this. However, I will let Benoit chime in first. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/15/2011 9:34, Jon Hunter wrote: > 2). The "ocp_abe_iclk" is an interface clock and is not a parent to any > other functional clock and therefore, is not driving any internal logic > in a peripheral which would have a direct impact of the speed of that > peripheral. However, there does appear to be another bug here in the > clock44xx_data.c as it shows that the "ocp_abe_iclk" is parent to the > "slimbus1_fck" which I believe is incorrect. According to the TRM, the > the ocp_abe_iclk is parent to the slimbus1_iclk. I can send another > patch for this. However, I will let Benoit chime in first. On further inspection of the clock44xx_data.c, it appears that all interface clocks are called xxx_fck and not xxx_ick. I will ask Benoit about this. However, this particular clock we are dealing with here is an interface clock. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/18/2011 15:57, Jon Hunter wrote: > On further inspection of the clock44xx_data.c, it appears that all > interface clocks are called xxx_fck and not xxx_ick. I will ask Benoit > about this. However, this particular clock we are dealing with here is > an interface clock. Actually, the above it not true. I will ask Benoit about the naming of the slimbus interface clocks. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jon, On 7/15/2011 4:34 PM, Hunter, Jon wrote: > Hi Paul, > > On 7/15/2011 3:21, Paul Walmsley wrote: >> cc'ing Benoît >> >> Hi Jon >> >> On Thu, 14 Jul 2011, Jon Hunter wrote: >> >>> From: Jon Hunter<jon-hunter@ti.com> >>> >>> The parent clock of the OCP_ABE_ICLK is the AESS_FCLK and the >>> parent clock of the AESS_FCLK is the ABE_FCLK... >>> >>> ABE_FCLK --> AESS_FCLK --> OCP_ABE_ICLK >>> >>> The AESS_FCLK and OCP_ABE_ICLK clocks both have dividers which >>> determine their operational frequency. However, the dividers for >>> the AESS_FCLK and OCP_ABE_ICLK are controlled via a single bit, >>> which is the CM1_ABE_AESS_CLKCTRL[24] bit.> When this bit is set to >>> 0, the AESS_FCLK divider is 1 and the OCP_ABE_ICLK divider is 2. >>> Similarly, when this bit is set to 1, the AESS_FCLK divider is 2 >>> and the OCP_ABE_ICLK is 1. >> >> Sigh. This type of hardware design makes software design difficult :-( > > Hopefully, because this is a interface clock the impact is really > minimal...more below... > >>> The above relationship between the AESS_FCLK and OCP_ABE_ICLK >>> dividers ensure that the OCP_ABE_ICLK clock is always half the >>> frequency of the ABE_CLK... >>> >>> OCP_ABE_ICLK = ABE_FCLK/2 >>> >>> The divider for the OCP_ABE_ICLK is currently missing so add a >>> divider that will ensure the OCP_ABE_ICLK frequency is always half >>> the ABE_FCLK frequency. >>> >>> Signed-off-by: Jon Hunter<jon-hunter@ti.com> >>> --- >>> arch/arm/mach-omap2/clock44xx_data.c | 16 +++++++++++++++- >>> 1 files changed, 15 insertions(+), 1 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c >>> index 8c96567..6e158ce 100644 >>> --- a/arch/arm/mach-omap2/clock44xx_data.c >>> +++ b/arch/arm/mach-omap2/clock44xx_data.c >>> @@ -1301,11 +1301,25 @@ static struct clk mcasp3_fclk = { >>> .recalc =&followparent_recalc, >>> }; >>> >>> +static const struct clksel_rate div2_2to1_rates[] = { >>> + { .div = 1, .val = 1, .flags = RATE_IN_4430 }, >>> + { .div = 2, .val = 0, .flags = RATE_IN_4430 }, >>> + { .div = 0 }, >>> +}; >>> + >>> +static const struct clksel ocp_abe_iclk_div[] = { >>> + { .parent =&aess_fclk, .rates = div2_2to1_rates }, >>> + { .parent = NULL }, >>> +}; >>> + >>> static struct clk ocp_abe_iclk = { >>> .name = "ocp_abe_iclk", >>> .parent =&aess_fclk, >>> + .clksel = ocp_abe_iclk_div, >>> + .clksel_reg = OMAP4430_CM1_ABE_AESS_CLKCTRL, >>> + .clksel_mask = OMAP4430_CLKSEL_AESS_FCLK_MASK, >>> .ops =&clkops_null, >>> - .recalc =&followparent_recalc, >>> + .recalc =&omap2_clksel_recalc, >>> }; >>> >>> static struct clk per_abe_24m_fclk = { >> >> I guess the reason that this patch can get away with this is because it >> doesn't allow software to change the rate of ocp_abe_iclk, and the >> ocp_abe_iclk is a child of aess_fclk, so when aess_fclk is changed, it >> will recalc ocp_abe_iclk. >> >> Benoît, what do you think? Is this a reasonable approach for the script? >> Or do we need to deal with some kind of 'linked clock' implementation... > > If you want my two cents on this matter, I would say that... > > 1). People should not need to configure the "ocp_abe_iclk" clock > directly, because regardless of the divider setting it is always 1/2 the > "abe_fclk". In other words, only the "aess_fclk" frequency is really > changing because of the divider and the above relationship ensures that > the "ocp_abe_iclk" is always the same frequency. So a user only cares > about the "aess_fclk" frequency and the "ocp_abe_iclk" frequency is > handled for them. > > 2). The "ocp_abe_iclk" is an interface clock and is not a parent to any > other functional clock and therefore, is not driving any internal logic > in a peripheral which would have a direct impact of the speed of that > peripheral. Since both dividers are linked, I exposed only one to SW on purpose to avoid conflict and confusion. As you said, we should and can only take care of the intermediate clock node. The only drawback of not linking both nodes is that the clock rate of the ocp_abe_iclk will be wrong if the parent clksel is changed. So if the recalc is working well your patch should fix that. My only concern is to find a way to generate that kind of hacky clock node:-( > However, there does appear to be another bug here in the > clock44xx_data.c as it shows that the "ocp_abe_iclk" is parent to the > "slimbus1_fck" which I believe is incorrect. According to the TRM, the > the ocp_abe_iclk is parent to the slimbus1_iclk. I can send another > patch for this. However, I will let Benoit chime in first. This is again a consequence of the fake modulemode clock we introduced initially and I tried to fix in the recent hwmod series. Since the slimbus1 module does not have any main_clk, but instead a bunch of optional clocks, I cannot affect any of them as the parent of the modulemode. That's why the iclk clock was used as the parent. That kind of issue will not be there anymore after the module mode series. Since the modulemode is not really a clock, the _ick / _fck extension was not necessarily accurate previously. Regards, Benoit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Benoît, Jon, On Wed, 27 Jul 2011, Cousson, Benoit wrote: > On 7/15/2011 4:34 PM, Hunter, Jon wrote: > > On 7/15/2011 3:21, Paul Walmsley wrote: > > > On Thu, 14 Jul 2011, Jon Hunter wrote: > > > > > > > From: Jon Hunter<jon-hunter@ti.com> > > > > > > > > The parent clock of the OCP_ABE_ICLK is the AESS_FCLK and the > > > > parent clock of the AESS_FCLK is the ABE_FCLK... > > > > > > > > ABE_FCLK --> AESS_FCLK --> OCP_ABE_ICLK > > > > > > > > The AESS_FCLK and OCP_ABE_ICLK clocks both have dividers which > > > > determine their operational frequency. However, the dividers for > > > > the AESS_FCLK and OCP_ABE_ICLK are controlled via a single bit, > > > > which is the CM1_ABE_AESS_CLKCTRL[24] bit.> When this bit is set to > > > > 0, the AESS_FCLK divider is 1 and the OCP_ABE_ICLK divider is 2. > > > > Similarly, when this bit is set to 1, the AESS_FCLK divider is 2 > > > > and the OCP_ABE_ICLK is 1. > > > > > > Sigh. This type of hardware design makes software design difficult :-( > > > > Hopefully, because this is a interface clock the impact is really > > minimal...more below... > > > > > > The above relationship between the AESS_FCLK and OCP_ABE_ICLK > > > > dividers ensure that the OCP_ABE_ICLK clock is always half the > > > > frequency of the ABE_CLK... > > > > > > > > OCP_ABE_ICLK = ABE_FCLK/2 > > > > > > > > The divider for the OCP_ABE_ICLK is currently missing so add a > > > > divider that will ensure the OCP_ABE_ICLK frequency is always half > > > > the ABE_FCLK frequency. > > > > > > > > Signed-off-by: Jon Hunter<jon-hunter@ti.com> > > > > --- > > > > arch/arm/mach-omap2/clock44xx_data.c | 16 +++++++++++++++- > > > > 1 files changed, 15 insertions(+), 1 deletions(-) > > > > > > > > diff --git a/arch/arm/mach-omap2/clock44xx_data.c > > > > b/arch/arm/mach-omap2/clock44xx_data.c > > > > index 8c96567..6e158ce 100644 > > > > --- a/arch/arm/mach-omap2/clock44xx_data.c > > > > +++ b/arch/arm/mach-omap2/clock44xx_data.c > > > > @@ -1301,11 +1301,25 @@ static struct clk mcasp3_fclk = { > > > > .recalc =&followparent_recalc, > > > > }; > > > > > > > > +static const struct clksel_rate div2_2to1_rates[] = { > > > > + { .div = 1, .val = 1, .flags = RATE_IN_4430 }, > > > > + { .div = 2, .val = 0, .flags = RATE_IN_4430 }, > > > > + { .div = 0 }, > > > > +}; > > > > + > > > > +static const struct clksel ocp_abe_iclk_div[] = { > > > > + { .parent =&aess_fclk, .rates = div2_2to1_rates }, > > > > + { .parent = NULL }, > > > > +}; > > > > + > > > > static struct clk ocp_abe_iclk = { > > > > .name = "ocp_abe_iclk", > > > > .parent =&aess_fclk, > > > > + .clksel = ocp_abe_iclk_div, > > > > + .clksel_reg = OMAP4430_CM1_ABE_AESS_CLKCTRL, > > > > + .clksel_mask = OMAP4430_CLKSEL_AESS_FCLK_MASK, > > > > .ops =&clkops_null, > > > > - .recalc =&followparent_recalc, > > > > + .recalc =&omap2_clksel_recalc, > > > > }; > > > > > > > > static struct clk per_abe_24m_fclk = { > > > > > > I guess the reason that this patch can get away with this is because it > > > doesn't allow software to change the rate of ocp_abe_iclk, and the > > > ocp_abe_iclk is a child of aess_fclk, so when aess_fclk is changed, it > > > will recalc ocp_abe_iclk. > > > > > > Benoît, what do you think? Is this a reasonable approach for the script? > > > Or do we need to deal with some kind of 'linked clock' implementation... > > > > If you want my two cents on this matter, I would say that... > > > > 1). People should not need to configure the "ocp_abe_iclk" clock > > directly, because regardless of the divider setting it is always 1/2 the > > "abe_fclk". In other words, only the "aess_fclk" frequency is really > > changing because of the divider and the above relationship ensures that > > the "ocp_abe_iclk" is always the same frequency. So a user only cares > > about the "aess_fclk" frequency and the "ocp_abe_iclk" frequency is > > handled for them. > > > > 2). The "ocp_abe_iclk" is an interface clock and is not a parent to any > > other functional clock and therefore, is not driving any internal logic > > in a peripheral which would have a direct impact of the speed of that > > peripheral. > > Since both dividers are linked, I exposed only one to SW on purpose to avoid > conflict and confusion. > As you said, we should and can only take care of the intermediate clock node. > > The only drawback of not linking both nodes is that the clock rate of the > ocp_abe_iclk will be wrong if the parent clksel is changed. > So if the recalc is working well your patch should fix that. > > My only concern is to find a way to generate that kind of hacky clock node:-( > > > However, there does appear to be another bug here in the > > clock44xx_data.c as it shows that the "ocp_abe_iclk" is parent to the > > "slimbus1_fck" which I believe is incorrect. According to the TRM, the > > the ocp_abe_iclk is parent to the slimbus1_iclk. I can send another > > patch for this. However, I will let Benoit chime in first. > > This is again a consequence of the fake modulemode clock we introduced > initially and I tried to fix in the recent hwmod series. > > Since the slimbus1 module does not have any main_clk, but instead a bunch of > optional clocks, I cannot affect any of them as the parent of the modulemode. > That's why the iclk clock was used as the parent. That kind of issue will not > be there anymore after the module mode series. > Since the modulemode is not really a clock, the _ick / _fck extension was not > necessarily accurate previously. Just doublechecking on this patch. Does anything need to be changed before we merge this? I can take a look at the autogeneration script if that's the only issue left. - Paul
diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c index 8c96567..6e158ce 100644 --- a/arch/arm/mach-omap2/clock44xx_data.c +++ b/arch/arm/mach-omap2/clock44xx_data.c @@ -1301,11 +1301,25 @@ static struct clk mcasp3_fclk = { .recalc = &followparent_recalc, }; +static const struct clksel_rate div2_2to1_rates[] = { + { .div = 1, .val = 1, .flags = RATE_IN_4430 }, + { .div = 2, .val = 0, .flags = RATE_IN_4430 }, + { .div = 0 }, +}; + +static const struct clksel ocp_abe_iclk_div[] = { + { .parent = &aess_fclk, .rates = div2_2to1_rates }, + { .parent = NULL }, +}; + static struct clk ocp_abe_iclk = { .name = "ocp_abe_iclk", .parent = &aess_fclk, + .clksel = ocp_abe_iclk_div, + .clksel_reg = OMAP4430_CM1_ABE_AESS_CLKCTRL, + .clksel_mask = OMAP4430_CLKSEL_AESS_FCLK_MASK, .ops = &clkops_null, - .recalc = &followparent_recalc, + .recalc = &omap2_clksel_recalc, }; static struct clk per_abe_24m_fclk = {