| Message ID | 1410273070-22485-2-git-send-email-rogerq@ti.com |
|---|---|
| State | New |
| Headers | show |
On 09/09/2014 04:31 PM, 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. Can you split this fix into a seperate patch, please. > 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(). What should happen if the timeout occurs? Marc
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@ti.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); > } >
On 09/09/2014 05:34 PM, Marc Kleine-Budde wrote: > On 09/09/2014 04:31 PM, 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. > > Can you split this fix into a seperate patch, please. OK. > >> 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(). > > What should happen if the timeout occurs? I'm not sure yet. I will verify if the hardware still works or not in that case. If it doesn't work then we should fail. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09/2014 05:34 PM, Nishanth Menon wrote: > 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@ti.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? wild guess ;), that it should be set in a few microseconds and the delay is not too large. Till I don't hear from hardware guys, it will remain a guess. > >> + 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? I will verify this first to check if the hardware works or not in the failing case. Considering we never checked for the DONE bit in our earlier TI releases maybe it works. But I'll verify and get back. > >> } >> spin_unlock(&raminit_lock); >> } >> > > cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09/2014 09:45 AM, Roger Quadros wrote: [...] >>> /* 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? > > wild guess ;), that it should be set in a few microseconds and the delay is not too > large. > > Till I don't hear from hardware guys, it will remain a guess. > in cases like these, I suggest using emperical data as point -> example doing some 10,000 iterations of the operation and picking up the worse case number and double it. Either way, you need to document the same, else a few years down the line, when that number is in question, no one will know what it's basis was..
On 09/09/2014 05:51 PM, Nishanth Menon wrote: > On 09/09/2014 09:45 AM, Roger Quadros wrote: > [...] >>>> /* 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? >> >> wild guess ;), that it should be set in a few microseconds and the delay is not too >> large. >> >> Till I don't hear from hardware guys, it will remain a guess. >> > > in cases like these, I suggest using emperical data as point -> > example doing some 10,000 iterations of the operation and picking up > the worse case number and double it. In my tests the bit was either set immediately or never at all. Not sure if we should increase it further. > > Either way, you need to document the same, else a few years down the > line, when that number is in question, no one will know what it's > basis was.. > OK. I'll add a comment there. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09/2014 04:39 PM, Roger Quadros wrote: > On 09/09/2014 05:34 PM, Marc Kleine-Budde wrote: >> On 09/09/2014 04:31 PM, 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. >> >> Can you split this fix into a seperate patch, please. > > OK. I've added this fix only to the can-tree. Thanks, Marc
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) { + dev_err(&priv->dev->dev, "%s: time out\n", __func__); + break; + } + } } 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); } spin_unlock(&raminit_lock); }
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@ti.com> --- drivers/net/can/c_can/c_can_platform.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)