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

[Paul Walmsley <paul@pwsan.com>]

Hi Lokesh

On Sun, 7 Feb 2016, Paul Walmsley wrote:

> A few comments:
> 
> 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@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.
> 
> > +}
> > +
> > +/**
> > + * omap_hwmod_rtc_lock - Lock the Kicker mechanism.
> > + * @oh: struct omap_hwmod *
> > + *
> > + * RTC IP have kicker feature. This prevents spurious writes to its registers.
> > + * Once the RTC registers are written, KICK mechanism needs to be locked,
> > + * in order to prevent any spurious writes. This function locks back the RTC
> > + * registers once hwmod completes its write into sysconfig register.
> > + */
> > +void omap_hwmod_rtc_lock(struct omap_hwmod *oh)
> > +{
> > +	local_irq_disable();
> > +	omap_rtc_wait_not_busy(oh);
> > +	omap_hwmod_write(0x0, oh, OMAP_RTC_KICK0_REG);
> > +	omap_hwmod_write(0x0, oh, OMAP_RTC_KICK1_REG);
> > +	local_irq_enable();
> > +}

Any update?


- Paul