On 03/18/2014 06:19 PM, Thomas Gleixner wrote: > The function is broken in several ways: > > - The function does not wait for the init to complete. > That can take quite some microseconds. > > - No protection against being called for two chips at the same > time. SMP is such a new thing, right? > > Clear the start and the init done bit unconditionally and wait for > both bits to be clear. > > In the enable path set the init bit and wait > for the init done bit. > > Add proper locking. > > Signed-off-by: Thomas Gleixner <[email protected]> > --- > drivers/net/can/c_can/c_can_platform.c | 48 > ++++++++++++++++++++++++++------- > 1 file changed, 39 insertions(+), 9 deletions(-) > > Index: linux/drivers/net/can/c_can/c_can_platform.c > =================================================================== > --- linux.orig/drivers/net/can/c_can/c_can_platform.c > +++ linux/drivers/net/can/c_can/c_can_platform.c > @@ -37,8 +37,10 @@ > > #include "c_can.h" > > -#define CAN_RAMINIT_START_MASK(i) (1 << (i)) > - > +#define CAN_RAMINIT_START_MASK(i) (0x001 << (i)) > +#define CAN_RAMINIT_DONE_MASK(i) (0x100 << (i)) > +#define CAN_RAMINIT_ALL_MASK(i) (0x101 << (i)) > +static DEFINE_SPINLOCK(raminit_lock); > /* > * 16-bit c_can registers can be arranged differently in the memory > * architecture of different implementations. For example: 16-bit > @@ -69,16 +71,44 @@ static void c_can_plat_write_reg_aligned > writew(val, priv->base + 2 * priv->regs[index]); > } > > +static void c_can_hw_raminit_wait(const struct c_can_priv *priv, u32 mask, > + u32 val) > +{ > + /* We look only at the bits of our instance. */ > + val &= mask; > + while ((readl(priv->raminit_ctrlreg) & mask) != val) > + udelay(1);
Do we have to add a timeout here, or is it "safe" to have a potential
endless loop here? As you have probably tortured the hardware and driver
a lot (or have been tortured by them), I assume you would have added a
timeout check if you had seen a lockup here.
> +}
> +
> static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable)
> {
> - u32 val;
> + u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance);
> + u32 ctrl;
> +
> + spin_lock(&raminit_lock);
>
> - val = readl(priv->raminit_ctrlreg);
> - if (enable)
> - val |= CAN_RAMINIT_START_MASK(priv->instance);
> - else
> - val &= ~CAN_RAMINIT_START_MASK(priv->instance);
> - writel(val, priv->raminit_ctrlreg);
> + ctrl = readl(priv->raminit_ctrlreg);
> + /*
> + * We clear the done and start bit first. The start bit is
> + * looking at the 0 -> transition, but is not self clearing;
> + * And we clear the init done bit as well.
> + */
nitpick: In the driver/net tree multi line comments look different, the
text starts right after the /*. No need to resend, I'll adjust this
while applying the patch.
> + ctrl &= ~CAN_RAMINIT_START_MASK(priv->instance);
> + ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
> + writel(ctrl, priv->raminit_ctrlreg);
> + ctrl &= ~CAN_RAMINIT_DONE_MASK(priv->instance);
> + c_can_hw_raminit_wait(priv, ctrl, mask);
> +
> + if (enable) {
> + /*
> + * Set start bit and wait for the done bit.
> + */
> + ctrl |= CAN_RAMINIT_START_MASK(priv->instance);
> + writel(ctrl, priv->raminit_ctrlreg);
> + ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
> + c_can_hw_raminit_wait(priv, ctrl, mask);
> + }
> + spin_unlock(&raminit_lock);
> }
>
> static struct platform_device_id c_can_id_table[] = {
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
