[PATCH v2] ARM: OMAP3: PM: fix I/O wakeup and I/O chain clock control detection

[Paul Walmsley]

On Thu, 6 Oct 2011, Russell King - ARM Linux wrote:

> On Thu, Oct 06, 2011 at 01:47:22PM -0600, Paul Walmsley wrote:
> > +	if ((omap_rev() == OMAP3430_REV_ES3_1 ||
> > +	     omap_rev() == OMAP3430_REV_ES3_1_2) ||
> > +	    cpu_is_omap3630())
> > +		omap_features |= OMAP3_HAS_IO_CHAIN_CTRL;
> 
> 	(a || b) || c === a || b || c
> 
> IOW, no need for the additional parens.

Thanks; patch updated.

> > diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> > index 7255d9b..a6156bd 100644
> > --- a/arch/arm/mach-omap2/pm34xx.c
> > +++ b/arch/arm/mach-omap2/pm34xx.c
> > @@ -99,31 +99,28 @@ static void omap3_enable_io_chain(void)
> >  {
> >  	int timeout = 0;
> >  
> > -	if (omap_rev() >= OMAP3430_REV_ES3_1) {
> > -		omap2_prm_set_mod_reg_bits(OMAP3430_EN_IO_CHAIN_MASK, WKUP_MOD,
> > -				     PM_WKEN);
> > -		/* Do a readback to assure write has been done */
> > -		omap2_prm_read_mod_reg(WKUP_MOD, PM_WKEN);
> > -
> > -		while (!(omap2_prm_read_mod_reg(WKUP_MOD, PM_WKEN) &
> > -			 OMAP3430_ST_IO_CHAIN_MASK)) {
> > -			timeout++;
> > -			if (timeout > 1000) {
> > -				printk(KERN_ERR "Wake up daisy chain "
> > -				       "activation failed.\n");
> > -				return;
> > -			}
> > -			omap2_prm_set_mod_reg_bits(OMAP3430_ST_IO_CHAIN_MASK,
> > -					     WKUP_MOD, PM_WKEN);
> > +	omap2_prm_set_mod_reg_bits(OMAP3430_EN_IO_CHAIN_MASK, WKUP_MOD,
> > +				   PM_WKEN);
> > +	/* Do a readback to assure write has been done */
> > +	omap2_prm_read_mod_reg(WKUP_MOD, PM_WKEN);
> > +
> > +	while (!(omap2_prm_read_mod_reg(WKUP_MOD, PM_WKEN) &
> > +		 OMAP3430_ST_IO_CHAIN_MASK)) {
> > +		timeout++;
> > +		if (timeout > 1000) {
> > +			printk(KERN_ERR "Wake up daisy chain "
> > +			       "activation failed.\n");
> > +			return;
> >  		}
> > +		omap2_prm_set_mod_reg_bits(OMAP3430_ST_IO_CHAIN_MASK,
> > +					   WKUP_MOD, PM_WKEN);
> >  	}
> 
> This should've been caught before.  Two things:
> 
> 1. Do you know how long it takes this to time out?

No, I don't; and I doubt the original author did either.

Really there should be at least a udelay(1) in that code; since as it 
stands right now, there's at least a partial CPU/interconnect/high 
frequency oscillator speed dependency in that loop.

But this patch was intended to be a minimal fix, avoiding code changes 
that are unrelated to the I/O chain feature detection change.  Those lines 
just showed up due to the indentation level change...
 
> 2. Don't wrap error printks, it prevents them being grepped for.

Thanks; patch updated.


- Paul