[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 9 Sep 2014 09:34:22 -0500
From: Nishanth Menon <nm@...com>
To: Roger Quadros <rogerq@...com>, <wg@...ndegger.com>,
<mkl@...gutronix.de>
CC: <tony@...mide.com>, <tglx@...utronix.de>, <mugunthanvnm@...com>,
<george.cherian@...com>, <balbi@...com>, <nsekhar@...com>,
<sergei.shtylyov@...entembedded.com>, <linux-omap@...r.kernel.org>,
<linux-can@...r.kernel.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] can: c_can_platform: Fix c_can_hw_raminit_ti()
and add timeout
On 09/09/2014 09:31 AM, Roger Quadros wrote:
> Pass the correct 'mask' and 'value' bits to c_can_hw_raminit_wait_ti().
> They seem to have been swapped in the usage instances.
>
> TI's RAMINIT DONE mechanism is buggy and may not always be
> set after the START bit is set. So add a timeout mechanism to
> c_can_hw_raminit_wait_ti().
>
> Signed-off-by: Roger Quadros <rogerq@...com>
> ---
> drivers/net/can/c_can/c_can_platform.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index 109cb44..b144e71 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -75,10 +75,18 @@ static void c_can_plat_write_reg_aligned_to_32bit(const struct c_can_priv *priv,
> static void c_can_hw_raminit_wait_ti(const struct c_can_priv *priv, u32 mask,
> u32 val)
> {
> + int timeout = 0;
> /* We look only at the bits of our instance. */
> val &= mask;
> - while ((readl(priv->raminit_ctrlreg) & mask) != val)
> + while ((readl(priv->raminit_ctrlreg) & mask) != val) {
> udelay(1);
> + timeout++;
> +
> + if (timeout == 1000) {
How did we come up with this number?
> + dev_err(&priv->dev->dev, "%s: time out\n", __func__);
> + break;
lets say we did timeout..
see below:
> + }
> + }
> }
>
> static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable)
> @@ -97,14 +105,14 @@ static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable)
> ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
> writel(ctrl, priv->raminit_ctrlreg);
> ctrl &= ~CAN_RAMINIT_DONE_MASK(priv->instance);
> - c_can_hw_raminit_wait_ti(priv, ctrl, mask);
> + c_can_hw_raminit_wait_ti(priv, mask, ctrl);
>
> 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_ti(priv, ctrl, mask);
> + c_can_hw_raminit_wait_ti(priv, mask, ctrl);
is it possible for us to continue? does it make sense for us to change
that void to a int and handle error cascading?
> }
> spin_unlock(&raminit_lock);
> }
>
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists