[PATCH] clocksource: fix clocksource_mmio_readX_down

[PATCH] clocksource: fix clocksource_mmio_readX_down

[Xiubo Li]

For some clocksource devices, for example, the registers are 32-bit, while
the lower 16-bit is used for timer counting(And reading the upper 16-bit
will return 0).

For example, when the counter value is 0x00001111, and then the
~readl_relaxed(to_mmio_clksrc(c)->reg) will return the value of 0xFFFFEEEE,
but it should be 0x0000EEEE.

So just using the c->mask to mask the unused bits.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 drivers/clocksource/mmio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
index c0e2512..f17a0d1 100644
--- a/drivers/clocksource/mmio.c
+++ b/drivers/clocksource/mmio.c
@@ -27,7 +27,7 @@ cycle_t clocksource_mmio_readl_up(struct clocksource *c)
 
 cycle_t clocksource_mmio_readl_down(struct clocksource *c)
 {
-	return ~readl_relaxed(to_mmio_clksrc(c)->reg);
+	return ~readl_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
 }
 
 cycle_t clocksource_mmio_readw_up(struct clocksource *c)
@@ -37,7 +37,7 @@ cycle_t clocksource_mmio_readw_up(struct clocksource *c)
 
 cycle_t clocksource_mmio_readw_down(struct clocksource *c)
 {
-	return ~(unsigned)readw_relaxed(to_mmio_clksrc(c)->reg);
+	return ~(unsigned)readw_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
 }
 
 /**
-- 
1.8.4

Re: [PATCH] clocksource: fix clocksource_mmio_readX_down

[Daniel Lezcano]

On 04/17/2014 11:27 AM, Xiubo Li wrote:
> For some clocksource devices, for example, the registers are 32-bit, while
> the lower 16-bit is used for timer counting(And reading the upper 16-bit
> will return 0).
>
> For example, when the counter value is 0x00001111, and then the
> ~readl_relaxed(to_mmio_clksrc(c)->reg) will return the value of 0xFFFFEEEE,
> but it should be 0x0000EEEE.
>
> So just using the c->mask to mask the unused bits.
>
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> ---
>   drivers/clocksource/mmio.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
> index c0e2512..f17a0d1 100644
> --- a/drivers/clocksource/mmio.c
> +++ b/drivers/clocksource/mmio.c
> @@ -27,7 +27,7 @@ cycle_t clocksource_mmio_readl_up(struct clocksource *c)
>
>   cycle_t clocksource_mmio_readl_down(struct clocksource *c)
>   {
> -	return ~readl_relaxed(to_mmio_clksrc(c)->reg);
> +	return ~readl_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
>   }
>
>   cycle_t clocksource_mmio_readw_up(struct clocksource *c)
> @@ -37,7 +37,7 @@ cycle_t clocksource_mmio_readw_up(struct clocksource *c)
>
>   cycle_t clocksource_mmio_readw_down(struct clocksource *c)
>   {
> -	return ~(unsigned)readw_relaxed(to_mmio_clksrc(c)->reg);
> +	return ~(unsigned)readw_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
>   }
>
>   /**


Hi,

I realize there is some type confusion here:

cycle_t -> u64
readl_relaxed -> u32
readw_relaxed -> u16

and clocksource_mmio_readw_down returns a cast to unsigned (u32)

This patch makes sense but it obfuscate more the types in these 
functions. Worth to clarify the functions first ?


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

RE: [PATCH] clocksource: fix clocksource_mmio_readX_down

[Li.Xiubo]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1771 bytes --]

> > diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
> > index c0e2512..f17a0d1 100644
> > --- a/drivers/clocksource/mmio.c
> > +++ b/drivers/clocksource/mmio.c
> > @@ -27,7 +27,7 @@ cycle_t clocksource_mmio_readl_up(struct clocksource *c)
> >
> >   cycle_t clocksource_mmio_readl_down(struct clocksource *c)
> >   {
> > -	return ~readl_relaxed(to_mmio_clksrc(c)->reg);
> > +	return ~readl_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
> >   }
> >
> >   cycle_t clocksource_mmio_readw_up(struct clocksource *c)
> > @@ -37,7 +37,7 @@ cycle_t clocksource_mmio_readw_up(struct clocksource *c)
> >
> >   cycle_t clocksource_mmio_readw_down(struct clocksource *c)
> >   {
> > -	return ~(unsigned)readw_relaxed(to_mmio_clksrc(c)->reg);
> > +	return ~(unsigned)readw_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
> >   }
> >
> >   /**
> 
> 
> Hi,
> 
> I realize there is some type confusion here:
> 
> cycle_t -> u64
> readl_relaxed -> u32
> readw_relaxed -> u16
> 
> and clocksource_mmio_readw_down returns a cast to unsigned (u32)
> 
> This patch makes sense but it obfuscate more the types in these
> functions. Worth to clarify the functions first ?
> 

Yes, though the short type could be converted to the longer type automatically,
It's better and worth to clarify it firstly.

I'll fix this, please see the next version.


Thanks,
BRs
Xiubo


> 
> --
>   <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
> 
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥