| Message ID | 1420641162-8634-1-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
+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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
+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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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;
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(-)