[PATCH v4 1/3] ARM: hwmod: RTC: Add lock and unlock functions

[Paul Walmsley]

Lokesh,

On Wed, 24 Feb 2016, Lokesh Vutla wrote:

> On Monday 08 February 2016 03:54 AM, Paul Walmsley wrote:
> > On Fri, 5 Feb 2016, 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 to be
> >> written to KICK registers.
> >> Introduce omap_hwmod_rtc_unlock/lock functions, which  writes into these
> >> KICK registers inorder to lock and unlock RTC registers.
> >>
> >> Signed-off-by: Lokesh Vutla <lokeshvutla at ti.com>
> >> ---
> > 
> > ...
> > 
> >> +/**
> >> + * omap_rtc_wait_not_busy - Wait for the RTC BUSY flag
> >> + * @oh: struct omap_hwmod *
> >> + *
> >> + * For updating certain RTC registers, the MPU must wait
> >> + * for the BUSY status in OMAP_RTC_STATUS_REG to become zero.
> >> + * Once the BUSY status is zero, there is a 15-?s access
> > 
> > Probably best just to write out "microseconds" or "us" here to avoid the 
> > high-bit character problem.
> > 
> >> + * period in which the MPU can program.
> >> + */
> >> +static void omap_rtc_wait_not_busy(struct omap_hwmod *oh)
> >> +{
> >> +	int i;
> >> +
> >> +	/* BUSY may stay active for 1/32768 second (~30 usec) */
> >> +	omap_test_timeout(omap_hwmod_read(oh, OMAP_RTC_STATUS_REG)
> >> +			  & OMAP_RTC_STATUS_REG, OMAP_RTC_MAX_READY_TIME, i);
> > 
> > This test looks bogus.  Shouldn't it AND the register value with 
> > OMAP_RTC_STATUS_BUSY?  Right now the code is AND-ing with 0x44, which 
> > doesn't include the BUSY bit.  So I guess the tests that you mentioned in 
> > the first message of the series don't cover the BUSY case?
> > 
> >> +	/* now we have ~15 usec to read/write various registers */
> >> +}
> >> +
> >> +/**
> >> + * omap_hwmod_rtc_unlock - 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.
> >> + */
> >> +void omap_hwmod_rtc_unlock(struct omap_hwmod *oh)
> >> +{
> >> +	local_irq_disable();
> >> +	omap_rtc_wait_not_busy(oh);
> >> +	omap_hwmod_write(OMAP_RTC_KICK0_VALUE, oh, OMAP_RTC_KICK0_REG);
> >> +	omap_hwmod_write(OMAP_RTC_KICK1_VALUE, oh, OMAP_RTC_KICK1_REG);
> >> +	local_irq_enable();
> > 
> > Finally, could you ask the IP block maintainer to confirm the 
> > interpretation that, for any STATUS_REG read where the BUSY bit is 0, that 
> > we are guaranteed to have at least 15 microseconds from that point in time 
> > to access the IP block?  It appears to be this way from my reading of the 
> > TRM; but to me, the phrasing is not explicit.  Another interpretation 
> > could be that the BUSY bit reflects the IP block's current status.  If 
> > this latter case is true, then to ensure that the IP block accesses 
> > complete inside the access window, we'll either need to test the BUSY bit 
> > after the writes to ensure that it is still 0, and otherwise repeat the 
> > busy test and writes; or we'll need to wait for a 0->1 BUSY transition 
> > before we wait for a 1->0 BUSY transition.
> 
> As per the HW folks, once SW sees the transistion from 1->0, it is
> guaranteed that SW has 15.25 us to do all the write transactions for all
> the TC updates.

Great

> Also as per TRM, in RTC there are three types of registers:
> -> TC registers and TC alarm registers:
> -> General registers
> -> Compensation registers.
> 
> MPU must wait for the BUSY bit for accessing the TC registers and  MPU
> can access the general registers at any point of time. SYSCONFIG, KICK
> registers comes under general registers. Since hwmod modifies only the
> sysconfig register, this omap_rtc_wait_not_busy() is not required here.
> We can directly write into sysconfig and KICK registers. Does this sound
> fine to you?
> Shall I resend this patch without omap_rtc_wait_not_busy()?

If there's no need for the unlock/lock sequence to ensure that the IP 
block is completely reset, then there doesn't seem to be much point in 
including it?  


- Paul