[PATCH v2 03/15] ARM: mxs: Add reset routines

[Uwe Kleine-König]

Hi Shawn,

On Thu, Dec 02, 2010 at 02:02:39PM +0800, Shawn Guo wrote:
> Hi Uwe,
> 
> 2010/11/30 Uwe Kleine-König <u.kleine-koenig at pengutronix.de>:
> > Hello,
> >
> [...]
> >
> >> +     /* Poll CLKGATE set */
> >> +     timeout = 0x400;
> >> +     while ((!(__raw_readl(reset_addr) & MXS_MODULE_CLKGATE)) && --timeout)
> >> +             /* nothing */;
> >> +     if (!timeout)
> >> +             goto error;
> >> +
> >> +     /* Clear SFTRST */
> >> +     val = __raw_readl(reset_addr);
> >> +     val &= ~MXS_MODULE_SFTRST;
> >> +     __raw_writel(val, reset_addr);
> >> +     /* Wait 1us */
> >> +     udelay(1);
> >> +     /* Poll SFTRST cleared */
> >> +     timeout = 0x400;
> >> +     while ((__raw_readl(reset_addr) & MXS_MODULE_SFTRST) && --timeout)
> >> +             /* nothing */;
> >> +     if (!timeout)
> >> +             goto error;
> >> +
> >> +     /* Clear CLKGATE */
> >> +     val = __raw_readl(reset_addr);
> >> +     val &= ~MXS_MODULE_CLKGATE;
> >> +     __raw_writel(val, reset_addr);
> >> +     /* Wait 1us */
> >> +     udelay(1);
> >> +     /* Poll CLKGATE cleared */
> >> +     timeout = 0x400;
> >> +     while ((__raw_readl(reset_addr) & MXS_MODULE_CLKGATE) && --timeout)
> >> +             /* nothing */;
> >> +     if (!timeout)
> >> +             goto error;
> >
> > There are quite some repetitions in this function.  If you could get it
> > down to something that looks like
> >
> >        clear SFTRST
> >        udelay
> >        poll SFTRST cleared
> >
> >        clear CLKGATE
> >        set SFTRST
> >        udelay
> >        poll CLKGATE set
> >
> >        clear SFTRST
> >        udelay
> >        poll SFTRST cleared
> >
> >        clear CLKGATE
> >        udelay
> >        poll CLKGATE cleared
> >
> > it's IMHO better than the comment above.  If "clear CLKGATE" and "set
> > SFTRST" can be done in a single step this might be done like that:
> >
> >        /* comment describing setmask_poll */
> >        static int setmask_poll(void __iomem *addr, u32 set, u32 mask,
> >                u32 pollval, u32 pollmask, u32 *curval)
> >        {
> >                int timeout = 0x400;
> >
> >                *curval &= ~mask;
> >                *curval |= set;
> >                __raw_writel(*curval, addr)
> >
> >                /*
> >                 * some nice comment about 1us being a good idea
> >                 */
> >                udelay(1);
> >
> >                do {
> >                        *curval = __raw_readl(addr);
> >                        if ((*curval & pollmask) == pollval)
> >                                return 0;
> >
> >                } while(--timeout);
> >
> >                return 1;
> >        }
> >
> >        int mxs_reset_block(void __iomem *reset_addr)
> >        {
> >                u32 val = __raw_readl(reset_addr);
> >                int ret;
> >
> >                /* clear SFTRST and poll SFTRST becoming clear */
> >                ret = setmask_poll(reset_addr,
> >                        0, MXS_MODULE_SFTRST,
> >                        0, MXS_MODULE_SFTRST, &val);
> >                if (ret)
> >                        goto error;
> >
> >                /* clear CLKGATE, set SFTRST and poll CLKGATE becoming set */
> >                ret = setmask_poll(reset_addr,
> >                        MXS_MODULE_SFTRST, MXS_MODULE_SFTRST | MXS_MODULE_CLKGATE,
> >                        0, MXS_MODULE_CLKGATE, &val);
> >                if (ret)
> >                        goto error;
> >
> >                ...
> >        }
> >
> > Well, not very nice, but IMHO better that your code.  Maybe
> > mxs_reset_block fits in a Terminal that way without decreasing the font
> > size :-)
> >
> I checked designer about doing "clear CLKGATE" and "set SFTRST" in a
> single step, and got the following feedback.
> 
> "Per HW design, it is not safe to get those two done by write once
> time. By two step write, one instruction period interval can be
> ensured at least for clock  enabling before sync. Soft reset.  Your
> code is only once time write in fact, so it shall not be equal to two
> write operation."
> 
> In this case, how do the following changes look to you?
> 
> /*
>  * Clear the bit and poll it cleared, the bit has to be
>  * SFTRST(bit 31) or CLKGATE (bit 30) of block control register.
for the function it doesn't have to be one of these bits.  Maybe:

	Clear the bit and poll it cleared.  This is usually called with
	a reset address and mask being either SFTRST(bit 31) or CLKGATE
	(bit 30).

>  */
> static int clear_poll_bit(void __iomem *addr, u32 mask)
> {
>         int timeout = 0x400;
> 
>         /* clear the bit */
>         __raw_writel(mask, addr + MXS_CLR_ADDR);
ah, so there is a clr register, that's nice.

> 
>         /*
>          * SFTRST needs 3 GPMI clocks to settle, the reference manual
>          * recommends to wait 1us.
>          */
>         udelay(1);
> 
>         /* poll the bit becoming clear */
>         while ((__raw_readl(addr) & mask) && --timeout)
>                 /* nothing */;
> 
>         if (unlikely(!timeout))
>                 return 1;
> 
>         return 0;
return !timeout; ?

> }
> 
> int mxs_reset_block(void __iomem *reset_addr)
> {
>         int ret;
>         int timeout = 0x400;
> 
>         /* clear and poll SFTRST */
>         ret = clear_poll_bit(reset_addr, MXS_MODULE_SFTRST);
>         if (ret)
>                 goto error;
> 
>         /* clear CLKGATE */
>         __raw_writel(MXS_MODULE_CLKGATE, reset_addr + MXS_CLR_ADDR);
>         /* set SFTRST to reset the block */
>         __raw_writel(MXS_MODULE_SFTRST, reset_addr + MXS_SET_ADDR);
>         udelay(1);
>         /* poll CLKGATE becoming set */
>         while ((!(__raw_readl(reset_addr) & MXS_MODULE_CLKGATE)) && --timeout)
>                 /* nothing */;
>         if (unlikely(!timeout))
>                 goto error;
> 
>         /* clear and poll SFTRST */
>         ret = clear_poll_bit(reset_addr, MXS_MODULE_SFTRST);
>         if (ret)
>                 goto error;
> 
>         /* clear and poll CLKGATE */
>         ret = clear_poll_bit(reset_addr, MXS_MODULE_CLKGATE);
>         if (ret)
>                 goto error;
> 
	return 0;

> error:
>         pr_err("%s(%p): module reset timeout\n", __func__, reset_addr);
>         return -ETIMEDOUT;
> }
Other than that it looks much better IMHO.

Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |