| Message ID | 1397767775-10965-6-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
On Thu, Apr 17, 2014 at 03:49:21PM -0500, Nishanth Menon wrote:
> Currently we use __raw_readl and writel in this driver, however, there
__raw_* and *_relaxed variants are the same, just have a look <asm/io.h>
297 #define readb_relaxed(c) ({ u8 __r = __raw_readb(c); __r; })
298 #define readw_relaxed(c) ({ u16 __r = le16_to_cpu((__force __le16) \
299 __raw_readw(c)); __r; })
300 #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
301 __raw_readl(c)); __r; })
302
303 #define writeb_relaxed(v,c) __raw_writeb(v,c)
304 #define writew_relaxed(v,c) __raw_writew((__force u16) cpu_to_le16(v),c)
305 #define writel_relaxed(v,c) __raw_writel((__force u32) cpu_to_le32(v),c)
On Thursday 17 April 2014 05:52 PM, Felipe Balbi wrote: > Hi, > > On Thu, Apr 17, 2014 at 03:49:21PM -0500, Nishanth Menon wrote: >> Currently we use __raw_readl and writel in this driver, however, there > > __raw_* and *_relaxed variants are the same, just have a look <asm/io.h> > Except the relaxed version can take care of endian conversion if needed. :-) > 297 #define readb_relaxed(c) ({ u8 __r = __raw_readb(c); __r; }) > 298 #define readw_relaxed(c) ({ u16 __r = le16_to_cpu((__force __le16) \ > 299 __raw_readw(c)); __r; }) > 300 #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \ > 301 __raw_readl(c)); __r; }) > 302 > 303 #define writeb_relaxed(v,c) __raw_writeb(v,c) > 304 #define writew_relaxed(v,c) __raw_writew((__force u16) cpu_to_le16(v),c) > 305 #define writel_relaxed(v,c) __raw_writel((__force u32) cpu_to_le32(v),c) > -- 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 Thu, Apr 17, 2014 at 05:56:15PM -0400, Santosh Shilimkar wrote: > On Thursday 17 April 2014 05:52 PM, Felipe Balbi wrote: > > Hi, > > > > On Thu, Apr 17, 2014 at 03:49:21PM -0500, Nishanth Menon wrote: > >> Currently we use __raw_readl and writel in this driver, however, there > > > > __raw_* and *_relaxed variants are the same, just have a look <asm/io.h> > > > Except the relaxed version can take care of endian conversion if > needed. :-) right, but according to commit log, this commit is more concerned about the memory barriers which writel()/readl() add, not endianness. Just a matter of fixing up commit log.
On 04/17/2014 05:03 PM, Felipe Balbi wrote: > On Thu, Apr 17, 2014 at 05:56:15PM -0400, Santosh Shilimkar wrote: >> On Thursday 17 April 2014 05:52 PM, Felipe Balbi wrote: >>> Hi, >>> >>> On Thu, Apr 17, 2014 at 03:49:21PM -0500, Nishanth Menon wrote: >>>> Currently we use __raw_readl and writel in this driver, however, there >>> >>> __raw_* and *_relaxed variants are the same, just have a look <asm/io.h> >>> >> Except the relaxed version can take care of endian conversion if >> needed. :-) > > right, but according to commit log, this commit is more concerned about > the memory barriers which writel()/readl() add, not endianness. Just a > matter of fixing up commit log. > yep, this patch does replace writel with writel_relaxed there is no strong need for barriers in the operations that we perform here. I agree that the commit message should probably be a little more detailed at this point. How about: Currently we use __raw_readl and writel in this driver. Considering there is no specific need for a memory barrier, replacing writel with endian-neutral writel_relaxed and replacing __raw_readls with the corresponding endian-neutral readl_relaxed allows us to have a standard set of register operations for the driver. While at it, simplify address computation using variables for register.
On Mon, Apr 21, 2014 at 08:16:28AM -0500, Nishanth Menon wrote: > On 04/17/2014 05:03 PM, Felipe Balbi wrote: > > On Thu, Apr 17, 2014 at 05:56:15PM -0400, Santosh Shilimkar wrote: > >> On Thursday 17 April 2014 05:52 PM, Felipe Balbi wrote: > >>> Hi, > >>> > >>> On Thu, Apr 17, 2014 at 03:49:21PM -0500, Nishanth Menon wrote: > >>>> Currently we use __raw_readl and writel in this driver, however, there > >>> > >>> __raw_* and *_relaxed variants are the same, just have a look <asm/io.h> > >>> > >> Except the relaxed version can take care of endian conversion if > >> needed. :-) > > > > right, but according to commit log, this commit is more concerned about > > the memory barriers which writel()/readl() add, not endianness. Just a > > matter of fixing up commit log. > > > > yep, this patch does replace writel with writel_relaxed there is no > strong need for barriers in the operations that we perform here. > > I agree that the commit message should probably be a little more > detailed at this point. > > > How about: > Currently we use __raw_readl and writel in this driver. Considering > there is no specific need for a memory barrier, replacing writel with > endian-neutral writel_relaxed and replacing __raw_readls with the > corresponding endian-neutral readl_relaxed allows us to have a > standard set of register operations for the driver. > > While at it, simplify address computation using variables for register. reads a lot better, thanks Acked-by: Felipe Balbi <balbi@ti.com>
diff --git a/drivers/bus/omap_l3_noc.c b/drivers/bus/omap_l3_noc.c index 37d71b7..c8facb0 100644 --- a/drivers/bus/omap_l3_noc.c +++ b/drivers/bus/omap_l3_noc.c @@ -55,6 +55,7 @@ static irqreturn_t l3_interrupt_handler(int irq, void *_l3) int err_src = 0; u32 std_err_main, err_reg, clear, masterid; void __iomem *base, *l3_targ_base; + void __iomem *l3_targ_stderr, *l3_targ_slvofslsb, *l3_targ_mstaddr; char *target_name, *master_name = "UN IDENTIFIED"; /* Get the Type of interrupt */ @@ -66,8 +67,8 @@ static irqreturn_t l3_interrupt_handler(int irq, void *_l3) * to determine the source */ base = l3->l3_base[i]; - err_reg = __raw_readl(base + l3_flagmux[i] + - + L3_FLAGMUX_REGERR0 + (inttype << 3)); + err_reg = readl_relaxed(base + l3_flagmux[i] + + L3_FLAGMUX_REGERR0 + (inttype << 3)); /* Get the corresponding error and analyse */ if (err_reg) { @@ -76,10 +77,14 @@ static irqreturn_t l3_interrupt_handler(int irq, void *_l3) /* Read the stderrlog_main_source from clk domain */ l3_targ_base = base + *(l3_targ[i] + err_src); - std_err_main = __raw_readl(l3_targ_base + - L3_TARG_STDERRLOG_MAIN); - masterid = __raw_readl(l3_targ_base + - L3_TARG_STDERRLOG_MSTADDR); + l3_targ_stderr = l3_targ_base + L3_TARG_STDERRLOG_MAIN; + l3_targ_slvofslsb = l3_targ_base + + L3_TARG_STDERRLOG_SLVOFSLSB; + l3_targ_mstaddr = l3_targ_base + + L3_TARG_STDERRLOG_MSTADDR; + + std_err_main = readl_relaxed(l3_targ_stderr); + masterid = readl_relaxed(l3_targ_mstaddr); switch (std_err_main & CUSTOM_ERROR) { case STANDARD_ERROR: @@ -87,12 +92,10 @@ static irqreturn_t l3_interrupt_handler(int irq, void *_l3) l3_targ_inst_name[i][err_src]; WARN(true, "L3 standard error: TARGET:%s at address 0x%x\n", target_name, - __raw_readl(l3_targ_base + - L3_TARG_STDERRLOG_SLVOFSLSB)); + readl_relaxed(l3_targ_slvofslsb)); /* clear the std error log*/ clear = std_err_main | CLEAR_STDERR_LOG; - writel(clear, l3_targ_base + - L3_TARG_STDERRLOG_MAIN); + writel_relaxed(clear, l3_targ_stderr); break; case CUSTOM_ERROR: @@ -107,8 +110,7 @@ static irqreturn_t l3_interrupt_handler(int irq, void *_l3) master_name, target_name); /* clear the std error log*/ clear = std_err_main | CLEAR_STDERR_LOG; - writel(clear, l3_targ_base + - L3_TARG_STDERRLOG_MAIN); + writel_relaxed(clear, l3_targ_stderr); break; default:
Currently we use __raw_readl and writel in this driver, however, there is no strict sequencing needs for this driver, hence we should be good with the relaxed variants. While at it, simplify address computation using variables for register. Signed-off-by: Nishanth Menon <nm@ti.com> --- V2: no functional change, just reordering V1: https://patchwork.kernel.org/patch/3984051/ drivers/bus/omap_l3_noc.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)