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

[Paul Walmsley]

Hi Lokesh

I apologize for the delay.

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 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.

> +}
> +
> +/**
> + * 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();
> +}
> -- 
> 2.1.4
> 


- Paul