2014-04-17 10:10:56

by Xiubo Li

[permalink] [raw]
Subject: [PATCH] clocksource: fix clocksource_mmio_readX_down

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 <[email protected]>
---
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


2014-04-22 10:26:43

by Daniel Lezcano

[permalink] [raw]
Subject: Re: [PATCH] clocksource: fix clocksource_mmio_readX_down

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 <[email protected]>
> ---
> 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

2014-04-23 02:27:27

by Xiubo Li

[permalink] [raw]
Subject: RE: [PATCH] clocksource: fix clocksource_mmio_readX_down

> > 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?