| Message ID | 1416199402-3865-1-git-send-email-lokeshvutla@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Paul, On Monday 17 November 2014 10:13 AM, Lokesh Vutla wrote: > RTC IP have kicker feature which prevents spurious writes to its registers. > In order to write into any of the RTC registers, KICK values has te be > written to KICK registers. Currently hwmod is updating the IDLEMODE in rtc > sysconfig register without writing into any kick register which is a noop. > When autoidle is allowed for rtc, interruts are not received until IDLEMODE > is set to SIDLE_SMART_WKUP. > Adding a reset function to unlock RTC registers so that IDLEMODE is > updated. Ping..!! Is this patch acceptable? Thanks and regards, Lokesh > > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > --- > arch/arm/mach-omap2/omap_hwmod.h | 1 + > arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 1 + > arch/arm/mach-omap2/omap_hwmod_reset.c | 23 +++++++++++++++++++++++ > 3 files changed, 25 insertions(+) > > diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h > index 512f809..3703f99 100644 > --- a/arch/arm/mach-omap2/omap_hwmod.h > +++ b/arch/arm/mach-omap2/omap_hwmod.h > @@ -744,6 +744,7 @@ const char *omap_hwmod_get_main_clk(struct omap_hwmod *oh); > */ > > extern int omap_hwmod_aess_preprogram(struct omap_hwmod *oh); > +int omap_hwmod_rtc_unlock(struct omap_hwmod *oh); > > /* > * Chip variant-specific hwmod init routines - XXX should be converted > diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > index 5684f11..90e1df4 100644 > --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > @@ -1584,6 +1584,7 @@ static struct omap_hwmod_class_sysconfig dra7xx_rtcss_sysc = { > static struct omap_hwmod_class dra7xx_rtcss_hwmod_class = { > .name = "rtcss", > .sysc = &dra7xx_rtcss_sysc, > + .reset = &omap_hwmod_rtc_unlock, > }; > > /* rtcss */ > diff --git a/arch/arm/mach-omap2/omap_hwmod_reset.c b/arch/arm/mach-omap2/omap_hwmod_reset.c > index 65e186c..b825a28 100644 > --- a/arch/arm/mach-omap2/omap_hwmod_reset.c > +++ b/arch/arm/mach-omap2/omap_hwmod_reset.c > @@ -30,6 +30,12 @@ > > #include "omap_hwmod.h" > > +#define RTC_KICK0_REG_OFFSET 0x6c > +#define RTC_KICK1_REG_OFFSET 0x70 > + > +#define RTC_KICK0_VALUE 0x83E70B13 > +#define RTC_KICK1_VALUE 0x95A4F1E0 > + > /** > * omap_hwmod_aess_preprogram - enable AESS internal autogating > * @oh: struct omap_hwmod * > @@ -51,3 +57,20 @@ int omap_hwmod_aess_preprogram(struct omap_hwmod *oh) > > return 0; > } > + > +/** > + * omap_hwmod_rtc_unlock - Reset and unlock the Kicker mechanism. > + * @oh: struct omap_hwmod * > + * > + * RTC IP have kicker feature. This prevents spurious writes to its registers. > + * In order to write into any of the RTC registers, KICK values has te be > + * written in respective KICK registers. This is needed for hwmod to write into > + * sysconfig register. > + */ > +int omap_hwmod_rtc_unlock(struct omap_hwmod *oh) > +{ > + omap_hwmod_write(RTC_KICK0_VALUE, oh, RTC_KICK0_REG_OFFSET); > + omap_hwmod_write(RTC_KICK1_VALUE, oh, RTC_KICK1_REG_OFFSET); > + > + return 0; > +} >
On Thu, 20 Nov 2014, Lokesh Vutla wrote: > On Monday 17 November 2014 10:13 AM, Lokesh Vutla wrote: > > RTC IP have kicker feature which prevents spurious writes to its registers. > > In order to write into any of the RTC registers, KICK values has te be > > written to KICK registers. Currently hwmod is updating the IDLEMODE in rtc > > sysconfig register without writing into any kick register which is a noop. > > When autoidle is allowed for rtc, interruts are not received until IDLEMODE > > is set to SIDLE_SMART_WKUP. > > Adding a reset function to unlock RTC registers so that IDLEMODE is > > updated. > Ping..!! > Is this patch acceptable? Lokesh 1. This looks like a fix. Is this intended to go in as a v3.18-rc patch, or against v3.19? If so it would be very helpful for the maintainers if you were to state that somewhere. 2. Your patch unlocks the RTC registers, but never relocks it. This seems to defeat the point of the spurious write protection. Shouldn't your patch only unlock the RTC immediately before the hwmod code touches the RTC registers, and then relock it immediately afterwards, per SPRUHZ6 section 23.4.3.3? If so then the .reset function pointer is the wrong place for this; I would suggest adding some .lock and .unlock function pointers that are to be called before and after any register write to the IP block. 3. Your macros don't mention DRA7xx specifically. Does this sequence apply to some other TI chips, or just DRA7xx? Please document this in a comment above the macros, and possibly change the name of the macros to refer to DRA7XX. - Paul > > Thanks and regards, > Lokesh > > > > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > > --- > > arch/arm/mach-omap2/omap_hwmod.h | 1 + > > arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 1 + > > arch/arm/mach-omap2/omap_hwmod_reset.c | 23 +++++++++++++++++++++++ > > 3 files changed, 25 insertions(+) > > > > diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h > > index 512f809..3703f99 100644 > > --- a/arch/arm/mach-omap2/omap_hwmod.h > > +++ b/arch/arm/mach-omap2/omap_hwmod.h > > @@ -744,6 +744,7 @@ const char *omap_hwmod_get_main_clk(struct omap_hwmod *oh); > > */ > > > > extern int omap_hwmod_aess_preprogram(struct omap_hwmod *oh); > > +int omap_hwmod_rtc_unlock(struct omap_hwmod *oh); > > > > /* > > * Chip variant-specific hwmod init routines - XXX should be converted > > diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > > index 5684f11..90e1df4 100644 > > --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > > +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > > @@ -1584,6 +1584,7 @@ static struct omap_hwmod_class_sysconfig dra7xx_rtcss_sysc = { > > static struct omap_hwmod_class dra7xx_rtcss_hwmod_class = { > > .name = "rtcss", > > .sysc = &dra7xx_rtcss_sysc, > > + .reset = &omap_hwmod_rtc_unlock, > > }; > > > > /* rtcss */ > > diff --git a/arch/arm/mach-omap2/omap_hwmod_reset.c b/arch/arm/mach-omap2/omap_hwmod_reset.c > > index 65e186c..b825a28 100644 > > --- a/arch/arm/mach-omap2/omap_hwmod_reset.c > > +++ b/arch/arm/mach-omap2/omap_hwmod_reset.c > > @@ -30,6 +30,12 @@ > > > > #include "omap_hwmod.h" > > > > +#define RTC_KICK0_REG_OFFSET 0x6c > > +#define RTC_KICK1_REG_OFFSET 0x70 > > + > > +#define RTC_KICK0_VALUE 0x83E70B13 > > +#define RTC_KICK1_VALUE 0x95A4F1E0 > > + > > /** > > * omap_hwmod_aess_preprogram - enable AESS internal autogating > > * @oh: struct omap_hwmod * > > @@ -51,3 +57,20 @@ int omap_hwmod_aess_preprogram(struct omap_hwmod *oh) > > > > return 0; > > } > > + > > +/** > > + * omap_hwmod_rtc_unlock - Reset and unlock the Kicker mechanism. > > + * @oh: struct omap_hwmod * > > + * > > + * RTC IP have kicker feature. This prevents spurious writes to its registers. > > + * In order to write into any of the RTC registers, KICK values has te be > > + * written in respective KICK registers. This is needed for hwmod to write into > > + * sysconfig register. > > + */ > > +int omap_hwmod_rtc_unlock(struct omap_hwmod *oh) > > +{ > > + omap_hwmod_write(RTC_KICK0_VALUE, oh, RTC_KICK0_REG_OFFSET); > > + omap_hwmod_write(RTC_KICK1_VALUE, oh, RTC_KICK1_REG_OFFSET); > > + > > + return 0; > > +} > > > - Paul
Hi Paul, On Thursday 20 November 2014 10:26 PM, Paul Walmsley wrote: > On Thu, 20 Nov 2014, Lokesh Vutla wrote: > >> On Monday 17 November 2014 10:13 AM, Lokesh Vutla wrote: >>> RTC IP have kicker feature which prevents spurious writes to its registers. >>> In order to write into any of the RTC registers, KICK values has te be >>> written to KICK registers. Currently hwmod is updating the IDLEMODE in rtc >>> sysconfig register without writing into any kick register which is a noop. >>> When autoidle is allowed for rtc, interruts are not received until IDLEMODE >>> is set to SIDLE_SMART_WKUP. >>> Adding a reset function to unlock RTC registers so that IDLEMODE is >>> updated. >> Ping..!! >> Is this patch acceptable? > > Lokesh > > 1. This looks like a fix. Is this intended to go in as a v3.18-rc patch, > or against v3.19? If so it would be very helpful for the maintainers if > you were to state that somewhere. Yes. This is a fix, intended to go in 3.18-rc. Sorry should have mentioned it. > > 2. Your patch unlocks the RTC registers, but never relocks it. This seems > to defeat the point of the spurious write protection. Shouldn't your > patch only unlock the RTC immediately before the hwmod code touches the > RTC registers, and then relock it immediately afterwards, per SPRUHZ6 > section 23.4.3.3? If so then the .reset function pointer is the wrong > place for this; I would suggest adding some .lock and .unlock function > pointers that are to be called before and after any register write to the > IP block. Yeah I agree with you. Currently rtc driver unlocks these kick registers in probe and leaves it unlocks for further use. But if hwmod does unlock and lock for every sysconfig write driver should also implement unlock and lock for every rtc register write, which adds an extra overhead. I am not sure if some one writes into rtc registers other than hwmod and driver. IMO we can leave it unlocked as the driver does. > > 3. Your macros don't mention DRA7xx specifically. Does this sequence > apply to some other TI chips, or just DRA7xx? Please document this in a > comment above the macros, and possibly change the name of the macros to > refer to DRA7XX. This sequence applies to AM43xx and AM33xx also. So made it generic. Ill document it. Thanks and regards, Lokesh > > > - Paul > >> >> Thanks and regards, >> Lokesh >>> >>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >>> --- >>> arch/arm/mach-omap2/omap_hwmod.h | 1 + >>> arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 1 + >>> arch/arm/mach-omap2/omap_hwmod_reset.c | 23 +++++++++++++++++++++++ >>> 3 files changed, 25 insertions(+) >>> >>> diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h >>> index 512f809..3703f99 100644 >>> --- a/arch/arm/mach-omap2/omap_hwmod.h >>> +++ b/arch/arm/mach-omap2/omap_hwmod.h >>> @@ -744,6 +744,7 @@ const char *omap_hwmod_get_main_clk(struct omap_hwmod *oh); >>> */ >>> >>> extern int omap_hwmod_aess_preprogram(struct omap_hwmod *oh); >>> +int omap_hwmod_rtc_unlock(struct omap_hwmod *oh); >>> >>> /* >>> * Chip variant-specific hwmod init routines - XXX should be converted >>> diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c >>> index 5684f11..90e1df4 100644 >>> --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c >>> +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c >>> @@ -1584,6 +1584,7 @@ static struct omap_hwmod_class_sysconfig dra7xx_rtcss_sysc = { >>> static struct omap_hwmod_class dra7xx_rtcss_hwmod_class = { >>> .name = "rtcss", >>> .sysc = &dra7xx_rtcss_sysc, >>> + .reset = &omap_hwmod_rtc_unlock, >>> }; >>> >>> /* rtcss */ >>> diff --git a/arch/arm/mach-omap2/omap_hwmod_reset.c b/arch/arm/mach-omap2/omap_hwmod_reset.c >>> index 65e186c..b825a28 100644 >>> --- a/arch/arm/mach-omap2/omap_hwmod_reset.c >>> +++ b/arch/arm/mach-omap2/omap_hwmod_reset.c >>> @@ -30,6 +30,12 @@ >>> >>> #include "omap_hwmod.h" >>> >>> +#define RTC_KICK0_REG_OFFSET 0x6c >>> +#define RTC_KICK1_REG_OFFSET 0x70 >>> + >>> +#define RTC_KICK0_VALUE 0x83E70B13 >>> +#define RTC_KICK1_VALUE 0x95A4F1E0 >>> + >>> /** >>> * omap_hwmod_aess_preprogram - enable AESS internal autogating >>> * @oh: struct omap_hwmod * >>> @@ -51,3 +57,20 @@ int omap_hwmod_aess_preprogram(struct omap_hwmod *oh) >>> >>> return 0; >>> } >>> + >>> +/** >>> + * omap_hwmod_rtc_unlock - Reset and unlock the Kicker mechanism. >>> + * @oh: struct omap_hwmod * >>> + * >>> + * RTC IP have kicker feature. This prevents spurious writes to its registers. >>> + * In order to write into any of the RTC registers, KICK values has te be >>> + * written in respective KICK registers. This is needed for hwmod to write into >>> + * sysconfig register. >>> + */ >>> +int omap_hwmod_rtc_unlock(struct omap_hwmod *oh) >>> +{ >>> + omap_hwmod_write(RTC_KICK0_VALUE, oh, RTC_KICK0_REG_OFFSET); >>> + omap_hwmod_write(RTC_KICK1_VALUE, oh, RTC_KICK1_REG_OFFSET); >>> + >>> + return 0; >>> +} >>> >> > > > - Paul >
Hi Lokesh On Tue, 25 Nov 2014, Lokesh Vutla wrote: > Hi Paul, > On Thursday 20 November 2014 10:26 PM, Paul Walmsley wrote: > > On Thu, 20 Nov 2014, Lokesh Vutla wrote: > > > >> On Monday 17 November 2014 10:13 AM, Lokesh Vutla wrote: > >>> RTC IP have kicker feature which prevents spurious writes to its registers. > >>> In order to write into any of the RTC registers, KICK values has te be > >>> written to KICK registers. Currently hwmod is updating the IDLEMODE in rtc > >>> sysconfig register without writing into any kick register which is a noop. > >>> When autoidle is allowed for rtc, interruts are not received until IDLEMODE > >>> is set to SIDLE_SMART_WKUP. > >>> Adding a reset function to unlock RTC registers so that IDLEMODE is > >>> updated. > >> Ping..!! > >> Is this patch acceptable? > > > > Lokesh > > > > 1. This looks like a fix. Is this intended to go in as a v3.18-rc patch, > > or against v3.19? If so it would be very helpful for the maintainers if > > you were to state that somewhere. > Yes. This is a fix, intended to go in 3.18-rc. Sorry should have > mentioned it. A few questions. Do you know when this problem started (in terms of kernel versions)? Also: the patch description states that this is only a problem when autoidle is allowed for RTC. What enables autoidle for RTC - the RTCSS driver? Or does hwmod wind up doing this after the RTCSS driver unlocks it? > > 2. Your patch unlocks the RTC registers, but never relocks it. This seems > > to defeat the point of the spurious write protection. Shouldn't your > > patch only unlock the RTC immediately before the hwmod code touches the > > RTC registers, and then relock it immediately afterwards, per SPRUHZ6 > > section 23.4.3.3? If so then the .reset function pointer is the wrong > > place for this; I would suggest adding some .lock and .unlock function > > pointers that are to be called before and after any register write to the > > IP block. > Yeah I agree with you. > Currently rtc driver unlocks these kick registers in probe and leaves it unlocks for > further use. > But if hwmod does unlock and lock for every sysconfig write driver should also > implement unlock and lock for every rtc register write, which adds an extra overhead. > I am not sure if some one writes into rtc registers other than hwmod and driver. > IMO we can leave it unlocked as the driver does. I would think that the best approach would be to set up .lock and .unlock function pointers, then add a temporary hwmod flag that, if set, would prevent the .lock function from ever being called. Then once the driver is fixed, that flag can be dropped. > > 3. Your macros don't mention DRA7xx specifically. Does this sequence > > apply to some other TI chips, or just DRA7xx? Please document this in a > > comment above the macros, and possibly change the name of the macros to > > refer to DRA7XX. > This sequence applies to AM43xx and AM33xx also. So made it generic. > Ill document it. OK but it would need more than just documentation, right? Wouldn't the hwmod data also need to be modified for AM33xx/AM43xx to add the .reset function pointer? Any reason why folks wouldn't have seen this problem on AM33xx/AM43xx? - Paul
Hi Paul, On Wednesday 26 November 2014 12:34 PM, Paul Walmsley wrote: > Hi Lokesh > > On Tue, 25 Nov 2014, Lokesh Vutla wrote: > >> Hi Paul, >> On Thursday 20 November 2014 10:26 PM, Paul Walmsley wrote: >>> On Thu, 20 Nov 2014, Lokesh Vutla wrote: >>> >>>> On Monday 17 November 2014 10:13 AM, Lokesh Vutla wrote: >>>>> RTC IP have kicker feature which prevents spurious writes to its registers. >>>>> In order to write into any of the RTC registers, KICK values has te be >>>>> written to KICK registers. Currently hwmod is updating the IDLEMODE in rtc >>>>> sysconfig register without writing into any kick register which is a noop. >>>>> When autoidle is allowed for rtc, interruts are not received until IDLEMODE >>>>> is set to SIDLE_SMART_WKUP. >>>>> Adding a reset function to unlock RTC registers so that IDLEMODE is >>>>> updated. >>>> Ping..!! >>>> Is this patch acceptable? >>> >>> Lokesh >>> >>> 1. This looks like a fix. Is this intended to go in as a v3.18-rc patch, >>> or against v3.19? If so it would be very helpful for the maintainers if >>> you were to state that somewhere. >> Yes. This is a fix, intended to go in 3.18-rc. Sorry should have >> mentioned it. > > A few questions. Do you know when this problem started (in terms of > kernel versions)? This is introduced in v3.18 by commit (6af16a1da ARM: DRA7: Add hook in SoC initcalls to enable pm initialization) > > Also: the patch description states that this is only a problem when > autoidle is allowed for RTC. What enables autoidle for RTC - the RTCSS > driver? Or does hwmod wind up doing this after the RTCSS driver unlocks > it? Autoidle is enabled by hwmod by omap2_clk_enable_autoidle_all(). The above patch does it. > >>> 2. Your patch unlocks the RTC registers, but never relocks it. This seems >>> to defeat the point of the spurious write protection. Shouldn't your >>> patch only unlock the RTC immediately before the hwmod code touches the >>> RTC registers, and then relock it immediately afterwards, per SPRUHZ6 >>> section 23.4.3.3? If so then the .reset function pointer is the wrong >>> place for this; I would suggest adding some .lock and .unlock function >>> pointers that are to be called before and after any register write to the >>> IP block. >> Yeah I agree with you. >> Currently rtc driver unlocks these kick registers in probe and leaves it unlocks for >> further use. >> But if hwmod does unlock and lock for every sysconfig write driver should also >> implement unlock and lock for every rtc register write, which adds an extra overhead. >> I am not sure if some one writes into rtc registers other than hwmod and driver. >> IMO we can leave it unlocked as the driver does. > > I would think that the best approach would be to set up .lock and .unlock > function pointers, then add a temporary hwmod flag that, if set, would > prevent the .lock function from ever being called. Then once the driver > is fixed, that flag can be dropped. Okay will update it and repost. > >>> 3. Your macros don't mention DRA7xx specifically. Does this sequence >>> apply to some other TI chips, or just DRA7xx? Please document this in a >>> comment above the macros, and possibly change the name of the macros to >>> refer to DRA7XX. >> This sequence applies to AM43xx and AM33xx also. So made it generic. >> Ill document it. > > OK but it would need more than just documentation, right? Wouldn't the > hwmod data also need to be modified for AM33xx/AM43xx to add the .reset > function pointer? Any reason why folks wouldn't have seen this problem on > AM33xx/AM43xx? PRCM in AM33xx and AM43xx does not support HW AUTO for clock domains. It is either SW_SLEEP/SW_WKUP or NO_SLEEP. So RTC is still getting interrupts even IDLEMODE is kept in SMART_IDLE(which is reset value). Thanks and regards, Lokesh > > > - Paul >
Ping. Are you going to redo this one? - Paul On Wed, 26 Nov 2014, Paul Walmsley wrote: > Hi Lokesh > > On Tue, 25 Nov 2014, Lokesh Vutla wrote: > > > Hi Paul, > > On Thursday 20 November 2014 10:26 PM, Paul Walmsley wrote: > > > On Thu, 20 Nov 2014, Lokesh Vutla wrote: > > > > > >> On Monday 17 November 2014 10:13 AM, Lokesh Vutla wrote: > > >>> RTC IP have kicker feature which prevents spurious writes to its registers. > > >>> In order to write into any of the RTC registers, KICK values has te be > > >>> written to KICK registers. Currently hwmod is updating the IDLEMODE in rtc > > >>> sysconfig register without writing into any kick register which is a noop. > > >>> When autoidle is allowed for rtc, interruts are not received until IDLEMODE > > >>> is set to SIDLE_SMART_WKUP. > > >>> Adding a reset function to unlock RTC registers so that IDLEMODE is > > >>> updated. > > >> Ping..!! > > >> Is this patch acceptable? > > > > > > Lokesh > > > > > > 1. This looks like a fix. Is this intended to go in as a v3.18-rc patch, > > > or against v3.19? If so it would be very helpful for the maintainers if > > > you were to state that somewhere. > > Yes. This is a fix, intended to go in 3.18-rc. Sorry should have > > mentioned it. > > A few questions. Do you know when this problem started (in terms of > kernel versions)? > > Also: the patch description states that this is only a problem when > autoidle is allowed for RTC. What enables autoidle for RTC - the RTCSS > driver? Or does hwmod wind up doing this after the RTCSS driver unlocks > it? > > > > 2. Your patch unlocks the RTC registers, but never relocks it. This seems > > > to defeat the point of the spurious write protection. Shouldn't your > > > patch only unlock the RTC immediately before the hwmod code touches the > > > RTC registers, and then relock it immediately afterwards, per SPRUHZ6 > > > section 23.4.3.3? If so then the .reset function pointer is the wrong > > > place for this; I would suggest adding some .lock and .unlock function > > > pointers that are to be called before and after any register write to the > > > IP block. > > Yeah I agree with you. > > Currently rtc driver unlocks these kick registers in probe and leaves it unlocks for > > further use. > > But if hwmod does unlock and lock for every sysconfig write driver should also > > implement unlock and lock for every rtc register write, which adds an extra overhead. > > I am not sure if some one writes into rtc registers other than hwmod and driver. > > IMO we can leave it unlocked as the driver does. > > I would think that the best approach would be to set up .lock and .unlock > function pointers, then add a temporary hwmod flag that, if set, would > prevent the .lock function from ever being called. Then once the driver > is fixed, that flag can be dropped. > > > > 3. Your macros don't mention DRA7xx specifically. Does this sequence > > > apply to some other TI chips, or just DRA7xx? Please document this in a > > > comment above the macros, and possibly change the name of the macros to > > > refer to DRA7XX. > > This sequence applies to AM43xx and AM33xx also. So made it generic. > > Ill document it. > > OK but it would need more than just documentation, right? Wouldn't the > hwmod data also need to be modified for AM33xx/AM43xx to add the .reset > function pointer? Any reason why folks wouldn't have seen this problem on > AM33xx/AM43xx? > > > - Paul > -- > 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 > - Paul
+ Felipe, Nishanth Hi Lokesh, what's the status here? - Paul On Fri, 2 Jan 2015, Paul Walmsley wrote: > Ping. Are you going to redo this one? > > - Paul > > On Wed, 26 Nov 2014, Paul Walmsley wrote: > > > Hi Lokesh > > > > On Tue, 25 Nov 2014, Lokesh Vutla wrote: > > > > > Hi Paul, > > > On Thursday 20 November 2014 10:26 PM, Paul Walmsley wrote: > > > > On Thu, 20 Nov 2014, Lokesh Vutla wrote: > > > > > > > >> On Monday 17 November 2014 10:13 AM, Lokesh Vutla wrote: > > > >>> RTC IP have kicker feature which prevents spurious writes to its registers. > > > >>> In order to write into any of the RTC registers, KICK values has te be > > > >>> written to KICK registers. Currently hwmod is updating the IDLEMODE in rtc > > > >>> sysconfig register without writing into any kick register which is a noop. > > > >>> When autoidle is allowed for rtc, interruts are not received until IDLEMODE > > > >>> is set to SIDLE_SMART_WKUP. > > > >>> Adding a reset function to unlock RTC registers so that IDLEMODE is > > > >>> updated. > > > >> Ping..!! > > > >> Is this patch acceptable? > > > > > > > > Lokesh > > > > > > > > 1. This looks like a fix. Is this intended to go in as a v3.18-rc patch, > > > > or against v3.19? If so it would be very helpful for the maintainers if > > > > you were to state that somewhere. > > > Yes. This is a fix, intended to go in 3.18-rc. Sorry should have > > > mentioned it. > > > > A few questions. Do you know when this problem started (in terms of > > kernel versions)? > > > > Also: the patch description states that this is only a problem when > > autoidle is allowed for RTC. What enables autoidle for RTC - the RTCSS > > driver? Or does hwmod wind up doing this after the RTCSS driver unlocks > > it? > > > > > > 2. Your patch unlocks the RTC registers, but never relocks it. This seems > > > > to defeat the point of the spurious write protection. Shouldn't your > > > > patch only unlock the RTC immediately before the hwmod code touches the > > > > RTC registers, and then relock it immediately afterwards, per SPRUHZ6 > > > > section 23.4.3.3? If so then the .reset function pointer is the wrong > > > > place for this; I would suggest adding some .lock and .unlock function > > > > pointers that are to be called before and after any register write to the > > > > IP block. > > > Yeah I agree with you. > > > Currently rtc driver unlocks these kick registers in probe and leaves it unlocks for > > > further use. > > > But if hwmod does unlock and lock for every sysconfig write driver should also > > > implement unlock and lock for every rtc register write, which adds an extra overhead. > > > I am not sure if some one writes into rtc registers other than hwmod and driver. > > > IMO we can leave it unlocked as the driver does. > > > > I would think that the best approach would be to set up .lock and .unlock > > function pointers, then add a temporary hwmod flag that, if set, would > > prevent the .lock function from ever being called. Then once the driver > > is fixed, that flag can be dropped. > > > > > > 3. Your macros don't mention DRA7xx specifically. Does this sequence > > > > apply to some other TI chips, or just DRA7xx? Please document this in a > > > > comment above the macros, and possibly change the name of the macros to > > > > refer to DRA7XX. > > > This sequence applies to AM43xx and AM33xx also. So made it generic. > > > Ill document it. > > > > OK but it would need more than just documentation, right? Wouldn't the > > hwmod data also need to be modified for AM33xx/AM43xx to add the .reset > > function pointer? Any reason why folks wouldn't have seen this problem on > > AM33xx/AM43xx? > > > > > > - Paul > > -- > > 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 > > > > > - Paul > - Paul
Hi Paul, On Thursday 12 February 2015 10:11 PM, Paul Walmsley wrote: > + Felipe, Nishanth > > Hi Lokesh, > > what's the status here? Sorry for the delayed response. I am currently on a high priority issue. Once I am done with it Ill address your comments and repost the patch. Thanks and regards, Lokesh > > - Paul > > > On Fri, 2 Jan 2015, Paul Walmsley wrote: > >> Ping. Are you going to redo this one? >> >> - Paul >> >> On Wed, 26 Nov 2014, Paul Walmsley wrote: >> >>> Hi Lokesh >>> >>> On Tue, 25 Nov 2014, Lokesh Vutla wrote: >>> >>>> Hi Paul, >>>> On Thursday 20 November 2014 10:26 PM, Paul Walmsley wrote: >>>>> On Thu, 20 Nov 2014, Lokesh Vutla wrote: >>>>> >>>>>> On Monday 17 November 2014 10:13 AM, Lokesh Vutla wrote: >>>>>>> RTC IP have kicker feature which prevents spurious writes to its registers. >>>>>>> In order to write into any of the RTC registers, KICK values has te be >>>>>>> written to KICK registers. Currently hwmod is updating the IDLEMODE in rtc >>>>>>> sysconfig register without writing into any kick register which is a noop. >>>>>>> When autoidle is allowed for rtc, interruts are not received until IDLEMODE >>>>>>> is set to SIDLE_SMART_WKUP. >>>>>>> Adding a reset function to unlock RTC registers so that IDLEMODE is >>>>>>> updated. >>>>>> Ping..!! >>>>>> Is this patch acceptable? >>>>> >>>>> Lokesh >>>>> >>>>> 1. This looks like a fix. Is this intended to go in as a v3.18-rc patch, >>>>> or against v3.19? If so it would be very helpful for the maintainers if >>>>> you were to state that somewhere. >>>> Yes. This is a fix, intended to go in 3.18-rc. Sorry should have >>>> mentioned it. >>> >>> A few questions. Do you know when this problem started (in terms of >>> kernel versions)? >>> >>> Also: the patch description states that this is only a problem when >>> autoidle is allowed for RTC. What enables autoidle for RTC - the RTCSS >>> driver? Or does hwmod wind up doing this after the RTCSS driver unlocks >>> it? >>> >>>>> 2. Your patch unlocks the RTC registers, but never relocks it. This seems >>>>> to defeat the point of the spurious write protection. Shouldn't your >>>>> patch only unlock the RTC immediately before the hwmod code touches the >>>>> RTC registers, and then relock it immediately afterwards, per SPRUHZ6 >>>>> section 23.4.3.3? If so then the .reset function pointer is the wrong >>>>> place for this; I would suggest adding some .lock and .unlock function >>>>> pointers that are to be called before and after any register write to the >>>>> IP block. >>>> Yeah I agree with you. >>>> Currently rtc driver unlocks these kick registers in probe and leaves it unlocks for >>>> further use. >>>> But if hwmod does unlock and lock for every sysconfig write driver should also >>>> implement unlock and lock for every rtc register write, which adds an extra overhead. >>>> I am not sure if some one writes into rtc registers other than hwmod and driver. >>>> IMO we can leave it unlocked as the driver does. >>> >>> I would think that the best approach would be to set up .lock and .unlock >>> function pointers, then add a temporary hwmod flag that, if set, would >>> prevent the .lock function from ever being called. Then once the driver >>> is fixed, that flag can be dropped. >>> >>>>> 3. Your macros don't mention DRA7xx specifically. Does this sequence >>>>> apply to some other TI chips, or just DRA7xx? Please document this in a >>>>> comment above the macros, and possibly change the name of the macros to >>>>> refer to DRA7XX. >>>> This sequence applies to AM43xx and AM33xx also. So made it generic. >>>> Ill document it. >>> >>> OK but it would need more than just documentation, right? Wouldn't the >>> hwmod data also need to be modified for AM33xx/AM43xx to add the .reset >>> function pointer? Any reason why folks wouldn't have seen this problem on >>> AM33xx/AM43xx? >>> >>> >>> - Paul >>> -- >>> 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 >>> >> >> >> - Paul >> > > > - Paul >
diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h index 512f809..3703f99 100644 --- a/arch/arm/mach-omap2/omap_hwmod.h +++ b/arch/arm/mach-omap2/omap_hwmod.h @@ -744,6 +744,7 @@ const char *omap_hwmod_get_main_clk(struct omap_hwmod *oh); */ extern int omap_hwmod_aess_preprogram(struct omap_hwmod *oh); +int omap_hwmod_rtc_unlock(struct omap_hwmod *oh); /* * Chip variant-specific hwmod init routines - XXX should be converted diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c index 5684f11..90e1df4 100644 --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c @@ -1584,6 +1584,7 @@ static struct omap_hwmod_class_sysconfig dra7xx_rtcss_sysc = { static struct omap_hwmod_class dra7xx_rtcss_hwmod_class = { .name = "rtcss", .sysc = &dra7xx_rtcss_sysc, + .reset = &omap_hwmod_rtc_unlock, }; /* rtcss */ diff --git a/arch/arm/mach-omap2/omap_hwmod_reset.c b/arch/arm/mach-omap2/omap_hwmod_reset.c index 65e186c..b825a28 100644 --- a/arch/arm/mach-omap2/omap_hwmod_reset.c +++ b/arch/arm/mach-omap2/omap_hwmod_reset.c @@ -30,6 +30,12 @@ #include "omap_hwmod.h" +#define RTC_KICK0_REG_OFFSET 0x6c +#define RTC_KICK1_REG_OFFSET 0x70 + +#define RTC_KICK0_VALUE 0x83E70B13 +#define RTC_KICK1_VALUE 0x95A4F1E0 + /** * omap_hwmod_aess_preprogram - enable AESS internal autogating * @oh: struct omap_hwmod * @@ -51,3 +57,20 @@ int omap_hwmod_aess_preprogram(struct omap_hwmod *oh) return 0; } + +/** + * omap_hwmod_rtc_unlock - Reset and unlock the Kicker mechanism. + * @oh: struct omap_hwmod * + * + * RTC IP have kicker feature. This prevents spurious writes to its registers. + * In order to write into any of the RTC registers, KICK values has te be + * written in respective KICK registers. This is needed for hwmod to write into + * sysconfig register. + */ +int omap_hwmod_rtc_unlock(struct omap_hwmod *oh) +{ + omap_hwmod_write(RTC_KICK0_VALUE, oh, RTC_KICK0_REG_OFFSET); + omap_hwmod_write(RTC_KICK1_VALUE, oh, RTC_KICK1_REG_OFFSET); + + return 0; +}
RTC IP have kicker feature which prevents spurious writes to its registers. In order to write into any of the RTC registers, KICK values has te be written to KICK registers. Currently hwmod is updating the IDLEMODE in rtc sysconfig register without writing into any kick register which is a noop. When autoidle is allowed for rtc, interruts are not received until IDLEMODE is set to SIDLE_SMART_WKUP. Adding a reset function to unlock RTC registers so that IDLEMODE is updated. Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> --- arch/arm/mach-omap2/omap_hwmod.h | 1 + arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 1 + arch/arm/mach-omap2/omap_hwmod_reset.c | 23 +++++++++++++++++++++++ 3 files changed, 25 insertions(+)