| Message ID | CAKKbWA6S7KotAFtLO=ow=XYnLL2Ny5Mz2kcgM1cs+j=5mHQNmw@mail.gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | clocksource/drivers/npcm: fix GENMASK and timer operation | expand |
Avi, On Mon, 15 Jul 2019, Avi Fishman wrote: > NPCM7XX_Tx_OPER GENMASK was wrong, That part is already fixed upstream: 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro") > npcm7xx_timer_oneshot() did wrong calculation That changelog is pretty unspecific. It does not tell what is wrong and which consequences that has. Please be a bit more specific. Thanks, tglx
Hi Thomas, Thanks, Avi On Mon, Jul 15, 2019 at 3:37 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > Avi, > > On Mon, 15 Jul 2019, Avi Fishman wrote: > > > NPCM7XX_Tx_OPER GENMASK was wrong, > > That part is already fixed upstream: > > 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro") The automatic fix changed from GENMASK(3, 27) to GENMASK(27, 3) I reviewd again the code to check how it worked so far and saw that it should have been GENMASK(28, 27) - this is a different value than 9bdd7bb3a844 For our fortune this wrong value didn't effect the our final write to the register. But still this should be fixed. > > > npcm7xx_timer_oneshot() did wrong calculation > > That changelog is pretty unspecific. It does not tell what is wrong and > which consequences that has. Please be a bit more specific. OK I will fix > > Thanks, > > tglx
Avi, On Mon, 15 Jul 2019, Avi Fishman wrote: > On Mon, Jul 15, 2019 at 3:37 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro") > > The automatic fix changed from > GENMASK(3, 27) to > GENMASK(27, 3) > I reviewd again the code to check how it worked so far and saw that it > should have been > GENMASK(28, 27) - this is a different value than 9bdd7bb3a844 > For our fortune this wrong value didn't effect the our final write to > the register. > But still this should be fixed. Fair enough. Please explain it proper in the changelog. Thanks, tglx
diff --git a/drivers/clocksource/timer-npcm7xx.c b/drivers/clocksource/timer-npcm7xx.c index 8a30da7f083b..9780ffd8010e 100644 --- a/drivers/clocksource/timer-npcm7xx.c +++ b/drivers/clocksource/timer-npcm7xx.c @@ -32,7 +32,7 @@ #define NPCM7XX_Tx_INTEN BIT(29) #define NPCM7XX_Tx_COUNTEN BIT(30) #define NPCM7XX_Tx_ONESHOT 0x0 -#define NPCM7XX_Tx_OPER GENMASK(27, 3) +#define NPCM7XX_Tx_OPER GENMASK(28, 27) #define NPCM7XX_Tx_MIN_PRESCALE 0x1 #define NPCM7XX_Tx_TDR_MASK_BITS 24 #define NPCM7XX_Tx_MAX_CNT 0xFFFFFF @@ -84,8 +84,6 @@ static int npcm7xx_timer_oneshot(struct clock_event_device *evt) val = readl(timer_of_base(to) + NPCM7XX_REG_TCSR0); val &= ~NPCM7XX_Tx_OPER; - - val = readl(timer_of_base(to) + NPCM7XX_REG_TCSR0); val |= NPCM7XX_START_ONESHOT_Tx; writel(val, timer_of_base(to) + NPCM7XX_REG_TCSR0);
NPCM7XX_Tx_OPER GENMASK was wrong, npcm7xx_timer_oneshot() did wrong calculation Signed-off-by: Avi Fishman <avifishman70@gmail.com> --- drivers/clocksource/timer-npcm7xx.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) @@ -97,12 +95,11 @@ static int npcm7xx_timer_periodic(struct clock_event_device *evt) struct timer_of *to = to_timer_of(evt); u32 val; + writel(timer_of_period(to), timer_of_base(to) + NPCM7XX_REG_TICR0); + val = readl(timer_of_base(to) + NPCM7XX_REG_TCSR0); val &= ~NPCM7XX_Tx_OPER; - - writel(timer_of_period(to), timer_of_base(to) + NPCM7XX_REG_TICR0); val |= NPCM7XX_START_PERIODIC_Tx; - writel(val, timer_of_base(to) + NPCM7XX_REG_TCSR0); return 0;