[PATCH] can: c_can: use regmap_update_bits() to modify RAMINIT register

[PATCH] can: c_can: use regmap_update_bits() to modify RAMINIT register

[Roger Quadros]

use of regmap_read() and regmap_write() in c_can_hw_raminit_syscon()
is not safe as the RAMINIT register can be shared between different drivers
at least for TI SoCs.

To make the modification atomic we switch to using regmap_update_bits().

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can_platform.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index f363972..364209a 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -110,6 +110,10 @@ static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
 	 */
 	ctrl &= ~(1 << raminit->bits.start);
 	ctrl |= 1 << raminit->bits.done;
+
+	/* we can't use regmap_update_bits here as it will bypass the write
+	 * if START is already 0 and DONE is already 1.
+	 */
 	regmap_write(raminit->syscon, raminit->reg, ctrl);
 
 	ctrl &= ~(1 << raminit->bits.done);
@@ -118,12 +122,13 @@ static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
 	if (enable) {
 		/* Set start bit and wait for the done bit. */
 		ctrl |= 1 << raminit->bits.start;
-		regmap_write(raminit->syscon, raminit->reg, ctrl);
+		regmap_update_bits(raminit->syscon, raminit->reg, mask, ctrl);
 
 		/* clear START bit if start pulse is needed */
 		if (raminit->needs_pulse) {
 			ctrl &= ~(1 << raminit->bits.start);
-			regmap_write(raminit->syscon, raminit->reg, ctrl);
+			regmap_update_bits(raminit->syscon, raminit->reg,
+					   mask, ctrl);
 		}
 
 		ctrl |= 1 << raminit->bits.done;
-- 
2.1.0

Re: [PATCH] can: c_can: use regmap_update_bits() to modify RAMINIT register

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On 07/01/15 16:32, Roger Quadros wrote:
> use of regmap_read() and regmap_write() in c_can_hw_raminit_syscon()
> is not safe as the RAMINIT register can be shared between different drivers
> at least for TI SoCs.
> 
> To make the modification atomic we switch to using regmap_update_bits().
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/net/can/c_can/c_can_platform.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index f363972..364209a 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -110,6 +110,10 @@ static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
>  	 */
>  	ctrl &= ~(1 << raminit->bits.start);
>  	ctrl |= 1 << raminit->bits.done;
> +
> +	/* we can't use regmap_update_bits here as it will bypass the write
> +	 * if START is already 0 and DONE is already 1.
> +	 */
>  	regmap_write(raminit->syscon, raminit->reg, ctrl);

Can you first use update_bits to change either bit, so that this update
will be done?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] can: c_can: use regmap_update_bits() to modify RAMINIT register

[Roger Quadros]

+Mark,

On 09/01/15 15:50, Tomi Valkeinen wrote:
> On 07/01/15 16:32, Roger Quadros wrote:
>> use of regmap_read() and regmap_write() in c_can_hw_raminit_syscon()
>> is not safe as the RAMINIT register can be shared between different drivers
>> at least for TI SoCs.
>>
>> To make the modification atomic we switch to using regmap_update_bits().
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/net/can/c_can/c_can_platform.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
>> index f363972..364209a 100644
>> --- a/drivers/net/can/c_can/c_can_platform.c
>> +++ b/drivers/net/can/c_can/c_can_platform.c
>> @@ -110,6 +110,10 @@ static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
>>  	 */
>>  	ctrl &= ~(1 << raminit->bits.start);
>>  	ctrl |= 1 << raminit->bits.done;
>> +
>> +	/* we can't use regmap_update_bits here as it will bypass the write
>> +	 * if START is already 0 and DONE is already 1.
>> +	 */
>>  	regmap_write(raminit->syscon, raminit->reg, ctrl);
> 
> Can you first use update_bits to change either bit, so that this update
> will be done?
> 

Initial condition is START=0, DONE=1.

Now setting and clearing START bit initiates the start process and will cause the
DONE bit to be set after a while.
So unfortunately this doesn't work for us.

Mark,

The problem here is we can't use regmap_update_bits() because we need to write a 1 to the DONE bit to clear it and _regmap_update_bits() doesn't allow us to do that because of commit
d91e8db2c3bb regmap: Suppress noop writes in regmap_update_bits()

Is reverting it going to cause other issues? If yes then can we have a flag to specify forced update?

cheers,
-roger
[1] - http://lxr.free-electrons.com/source/drivers/base/regmap/regmap.c#L2332

Re: [PATCH] can: c_can: use regmap_update_bits() to modify RAMINIT register

[Roger Quadros]

+Mark with correct id.

On 12/01/15 11:57, Roger Quadros wrote:
> +Mark,
> 
> On 09/01/15 15:50, Tomi Valkeinen wrote:
>> On 07/01/15 16:32, Roger Quadros wrote:
>>> use of regmap_read() and regmap_write() in c_can_hw_raminit_syscon()
>>> is not safe as the RAMINIT register can be shared between different drivers
>>> at least for TI SoCs.
>>>
>>> To make the modification atomic we switch to using regmap_update_bits().
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> ---
>>>  drivers/net/can/c_can/c_can_platform.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
>>> index f363972..364209a 100644
>>> --- a/drivers/net/can/c_can/c_can_platform.c
>>> +++ b/drivers/net/can/c_can/c_can_platform.c
>>> @@ -110,6 +110,10 @@ static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
>>>  	 */
>>>  	ctrl &= ~(1 << raminit->bits.start);
>>>  	ctrl |= 1 << raminit->bits.done;
>>> +
>>> +	/* we can't use regmap_update_bits here as it will bypass the write
>>> +	 * if START is already 0 and DONE is already 1.
>>> +	 */
>>>  	regmap_write(raminit->syscon, raminit->reg, ctrl);
>>
>> Can you first use update_bits to change either bit, so that this update
>> will be done?
>>
> 
> Initial condition is START=0, DONE=1.
> 
> Now setting and clearing START bit initiates the start process and will cause the
> DONE bit to be set after a while.
> So unfortunately this doesn't work for us.
> 
> Mark,
> 
> The problem here is we can't use regmap_update_bits() because we need to write a 1 to the DONE bit to clear it and _regmap_update_bits() doesn't allow us to do that because of commit
> d91e8db2c3bb regmap: Suppress noop writes in regmap_update_bits()
> 
> Is reverting it going to cause other issues? If yes then can we have a flag to specify forced update?
> 
> cheers,
> -roger
> [1] - http://lxr.free-electrons.com/source/drivers/base/regmap/regmap.c#L2332
> 
> 

Re: [PATCH] can: c_can: use regmap_update_bits() to modify RAMINIT register

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

On Mon, Jan 12, 2015 at 12:02:21PM +0200, Roger Quadros wrote:
> +Mark with correct id.

Please fix your mail client to word wrap within paragraphs, it amkes
your mail more legible.

> > The problem here is we can't use regmap_update_bits() because we need to write a 1 to the DONE bit to clear it and _regmap_update_bits() doesn't allow us to do that because of commit
> > d91e8db2c3bb regmap: Suppress noop writes in regmap_update_bits()
> > 
> > Is reverting it going to cause other issues? If yes then can we have a flag to specify forced update?

The usual thing to do here is an explicit write clearing the latch,
either immediately after setting it or immediately before setting it.
If the register is marked as volatile and the hardware doesn't read back
the latched state that also does the trick.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] can: c_can: use regmap_update_bits() to modify RAMINIT register

[Roger Quadros]

On 12/01/15 14:05, Mark Brown wrote:
> On Mon, Jan 12, 2015 at 12:02:21PM +0200, Roger Quadros wrote:
>> +Mark with correct id.
> 
> Please fix your mail client to word wrap within paragraphs, it amkes
> your mail more legible.

Fixed now. Thanks for pointing out.

> 
>>> The problem here is we can't use regmap_update_bits() because we need to write a 1 to the DONE bit to clear it and _regmap_update_bits() doesn't allow us to do that because of commit
>>> d91e8db2c3bb regmap: Suppress noop writes in regmap_update_bits()
>>>
>>> Is reverting it going to cause other issues? If yes then can we have a flag to specify forced update?
> 
> The usual thing to do here is an explicit write clearing the latch,
> either immediately after setting it or immediately before setting it.
> If the register is marked as volatile and the hardware doesn't read back
> the latched state that also does the trick.
> 

How does this work if driver has access to only 1 bit that can only be
written with 1 to clear a condition? Writing a 0 is no-op.
It can read back 0 or 1 depending on the condition.

I didn't understand the volatile trick :P.

cheers,
-roger

Re: [PATCH] can: c_can: use regmap_update_bits() to modify RAMINIT register

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]

On Mon, Jan 12, 2015 at 02:37:21PM +0200, Roger Quadros wrote:
> On 12/01/15 14:05, Mark Brown wrote:

> > The usual thing to do here is an explicit write clearing the latch,
> > either immediately after setting it or immediately before setting it.
> > If the register is marked as volatile and the hardware doesn't read back
> > the latched state that also does the trick.

> How does this work if driver has access to only 1 bit that can only be
> written with 1 to clear a condition? Writing a 0 is no-op.

The expectation is that for hardware like that writes of zero will be
ignored so you can just do one (generally this is the point of such
things - you write to explicitly clear the bits you want to clear so by
extension other bits are ignored.

> It can read back 0 or 1 depending on the condition.

If the hardware readback has nothing to do with what's read back then
by definition this isn't something you should be updating with
update_bits() - it's doing a read/modify/write cycle so if the read part
doesn't correspond to the write part things will go wrong.

> I didn't understand the volatile trick :P.

If the hardware always reads back zero then if the register isn't cached
(which it sounds like this one shouldn't be) the comparison done by
update_bits() will always show that the latch bit needs writing.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v2] can: c_can: use regmap_update_bits() to modify RAMINIT register

[Roger Quadros]

use of regmap_read() and regmap_write() in c_can_hw_raminit_syscon()
is not safe as the RAMINIT register can be shared between different drivers
at least for TI SoCs.

To make the modification atomic we switch to using regmap_update_bits().

regmap_update_bits() skips writing to the register if it's read content is the
same as what is going to be written. This causes an issue for us when we
need to clear the DONE bit with the initial condition START:0, DONE:1 as
DONE bit must be written with 1 to clear it.

So we defer the clearing of DONE bit to later when we set the START bit.
There we are sure that START bit is changed from 0 to 1 so the write of
1 to already set DONE bit will happen.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can_platform.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index f363972..e36d105 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -103,27 +103,34 @@ static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
 	mask = 1 << raminit->bits.start | 1 << raminit->bits.done;
 	regmap_read(raminit->syscon, raminit->reg, &ctrl);
 
-	/* We clear the done and start bit first. The start bit is
+	/* We clear the 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.
 	 * NOTE: DONE must be written with 1 to clear it.
+	 * We can't clear the DONE bit here using regmap_update_bits()
+	 * as it will bypass the write if initial condition is START:0 DONE:1
+	 * e.g. on DRA7 which needs START pulse.
 	 */
-	ctrl &= ~(1 << raminit->bits.start);
-	ctrl |= 1 << raminit->bits.done;
-	regmap_write(raminit->syscon, raminit->reg, ctrl);
+	ctrl &= ~mask;	/* START = 0, DONE = 0 */
+	regmap_update_bits(raminit->syscon, raminit->reg, mask, ctrl);
 
-	ctrl &= ~(1 << raminit->bits.done);
-	c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
+	/* check if START bit is 0. Ignore DONE bit for now
+	 * as it can be either 0 or 1.
+	 */
+	c_can_hw_raminit_wait_syscon(priv, 1 << raminit->bits.start, ctrl);
 
 	if (enable) {
-		/* Set start bit and wait for the done bit. */
+		/* Clear DONE bit & set START bit. */
 		ctrl |= 1 << raminit->bits.start;
-		regmap_write(raminit->syscon, raminit->reg, ctrl);
-
+		/* DONE must be written with 1 to clear it */
+		ctrl |= 1 << raminit->bits.done;
+		regmap_update_bits(raminit->syscon, raminit->reg, mask, ctrl);
+		/* prevent further clearing of DONE bit */
+		ctrl &= ~(1 << raminit->bits.done);
 		/* clear START bit if start pulse is needed */
 		if (raminit->needs_pulse) {
 			ctrl &= ~(1 << raminit->bits.start);
-			regmap_write(raminit->syscon, raminit->reg, ctrl);
+			regmap_update_bits(raminit->syscon, raminit->reg,
+					   mask, ctrl);
 		}
 
 		ctrl |= 1 << raminit->bits.done;
-- 
2.1.0

Re: [PATCH v2] can: c_can: use regmap_update_bits() to modify RAMINIT register

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]

On 01/13/2015 03:23 PM, Roger Quadros wrote:
> use of regmap_read() and regmap_write() in c_can_hw_raminit_syscon()
> is not safe as the RAMINIT register can be shared between different drivers
> at least for TI SoCs.
> 
> To make the modification atomic we switch to using regmap_update_bits().
> 
> regmap_update_bits() skips writing to the register if it's read content is the
> same as what is going to be written. This causes an issue for us when we
> need to clear the DONE bit with the initial condition START:0, DONE:1 as
> DONE bit must be written with 1 to clear it.
> 
> So we defer the clearing of DONE bit to later when we set the START bit.
> There we are sure that START bit is changed from 0 to 1 so the write of
> 1 to already set DONE bit will happen.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>

Applied to linux-can.

Thanks, 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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]