Re: [patch 02/12] can: c_can: Fix hardware raminit function

[Thomas Gleixner]

On Wed, 19 Mar 2014, Oliver Hartkopp wrote:
> On 18.03.2014 23:15, Thomas Gleixner wrote:
> > On Tue, 18 Mar 2014, Marc Kleine-Budde wrote:
> >> On 03/18/2014 06:19 PM, Thomas Gleixner wrote:
> >>> +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.
> > 
> > I haven't seen any failure on that. We could add a timeout for
> > paranoia reasons. I'm quite sure that the raminit works as advertised
> > when we do it the right way. The only way to wreckage it so far is by
> > not waiting for it to complete.
> 
> As long as it is 100% guaranteed that we
> 
> 1. really work on a valid C_CAN core
> 2. this CAN controller can not be unplugged
> 
> not adding a timeout would be ok.

The raminit port is a D_CAN specific thing. But yeah, adding a timeout
wouldn't hurt at all.

Though the driver is incomplete anyway as not all D_CAN incarnations
use the raminit port. There is another way to do that via the CFR
register, which is not implemented in the driver.

It seems the chip integrator can chose between raminit port and CFR.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/