Re: [PATCH v3] clocksource: sh_cmt: fixup for 64-bit machines
On 08/09/2018 22:54, Sergei Shtylyov wrote:
> When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
> that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
> /proc/timer_list. It turned out that when calculating it, the driver did
> 1 << 32 (causing what I think was undefined behavior) resulting in a zero
> delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
> to be that the driver abused *unsigned long* for the CMT register values
> (which are 16/32-bit), so that the calculation of 'ch->max_match_value'
> in sh_cmt_setup_channel() used the wrong branch. Using more proper 'u32'
> instead fixed 'max_delta_ns' and even fixed the switching an active
> clocksource to CMT (which caused the system to turn non-interactive
> before).
>
> Signed-off-by: Sergei Shtylyov
>
> ---
> This patch is against the 'tip.git' repo's 'timers/core' branch.
> The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
> enabling building of this driver now, so not sure how urgent is this...
>
> Changes in version 3:
> - made the 'overflow_bit' and 'clear_bits' members of 'struct sh_cmt_info'
> 'u32';
> - made the 2nd parameter of sh_cmt_write_cmstr() 'u32';
> - made the result, the 2nd parameter, and 'o{1|2}' local variables of
> sh_cmt_get_counter() 'u32';
> - made the 'has_wrapped' local variables 'u32' in sh_cmt_clocksource_read()
> and sh_cmt_clock_event_program_verify();
> - fixed a typo in the patch description.
>
> Changes in version 2:
> - completely redid the patch, fixing abuse of *unsigned long* for the CMT
> register values.
>
> drivers/clocksource/sh_cmt.c | 72
> +++
> 1 file changed, 33 insertions(+), 39 deletions(-)
>
Applied for 4.20, thanks.
--
Facebook |
Twitter |
Re: [PATCH v3] clocksource: sh_cmt: fixup for 64-bit machines
On Sat, Sep 8, 2018 at 10:54 PM Sergei Shtylyov
wrote:
> When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
> that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
> /proc/timer_list. It turned out that when calculating it, the driver did
> 1 << 32 (causing what I think was undefined behavior) resulting in a zero
> delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
> to be that the driver abused *unsigned long* for the CMT register values
> (which are 16/32-bit), so that the calculation of 'ch->max_match_value'
> in sh_cmt_setup_channel() used the wrong branch. Using more proper 'u32'
> instead fixed 'max_delta_ns' and even fixed the switching an active
> clocksource to CMT (which caused the system to turn non-interactive
> before).
>
> Signed-off-by: Sergei Shtylyov
>
> ---
> This patch is against the 'tip.git' repo's 'timers/core' branch.
> The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
> enabling building of this driver now, so not sure how urgent is this...
>
> Changes in version 3:
> - made the 'overflow_bit' and 'clear_bits' members of 'struct sh_cmt_info'
> 'u32';
> - made the 2nd parameter of sh_cmt_write_cmstr() 'u32';
> - made the result, the 2nd parameter, and 'o{1|2}' local variables of
> sh_cmt_get_counter() 'u32';
> - made the 'has_wrapped' local variables 'u32' in sh_cmt_clocksource_read()
> and sh_cmt_clock_event_program_verify();
> - fixed a typo in the patch description.
Thanks for the update!
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v3] clocksource: sh_cmt: fixup for 64-bit machines
On 08/09/2018 22:54, Sergei Shtylyov wrote:
> When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
> that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
> /proc/timer_list. It turned out that when calculating it, the driver did
> 1 << 32 (causing what I think was undefined behavior) resulting in a zero
> delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
> to be that the driver abused *unsigned long* for the CMT register values
> (which are 16/32-bit), so that the calculation of 'ch->max_match_value'
> in sh_cmt_setup_channel() used the wrong branch. Using more proper 'u32'
> instead fixed 'max_delta_ns' and even fixed the switching an active
> clocksource to CMT (which caused the system to turn non-interactive
> before).
>
> Signed-off-by: Sergei Shtylyov
Geert any comments ?
Sergei, in the future please Cc people who did comments on your patch.
Thanks
-- Daniel
> ---
> This patch is against the 'tip.git' repo's 'timers/core' branch.
> The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
> enabling building of this driver now, so not sure how urgent is this...
>
> Changes in version 3:
> - made the 'overflow_bit' and 'clear_bits' members of 'struct sh_cmt_info'
> 'u32';
> - made the 2nd parameter of sh_cmt_write_cmstr() 'u32';
> - made the result, the 2nd parameter, and 'o{1|2}' local variables of
> sh_cmt_get_counter() 'u32';
> - made the 'has_wrapped' local variables 'u32' in sh_cmt_clocksource_read()
> and sh_cmt_clock_event_program_verify();
> - fixed a typo in the patch description.
>
> Changes in version 2:
> - completely redid the patch, fixing abuse of *unsigned long* for the CMT
> register values.
>
> drivers/clocksource/sh_cmt.c | 72
> +++
> 1 file changed, 33 insertions(+), 39 deletions(-)
>
> Index: tip/drivers/clocksource/sh_cmt.c
> ===
> --- tip.orig/drivers/clocksource/sh_cmt.c
> +++ tip/drivers/clocksource/sh_cmt.c
> @@ -78,18 +78,17 @@ struct sh_cmt_info {
> unsigned int channels_mask;
>
> unsigned long width; /* 16 or 32 bit version of hardware block */
> - unsigned long overflow_bit;
> - unsigned long clear_bits;
> + u32 overflow_bit;
> + u32 clear_bits;
>
> /* callbacks for CMSTR and CMCSR access */
> - unsigned long (*read_control)(void __iomem *base, unsigned long offs);
> + u32 (*read_control)(void __iomem *base, unsigned long offs);
> void (*write_control)(void __iomem *base, unsigned long offs,
> - unsigned long value);
> + u32 value);
>
> /* callbacks for CMCNT and CMCOR access */
> - unsigned long (*read_count)(void __iomem *base, unsigned long offs);
> - void (*write_count)(void __iomem *base, unsigned long offs,
> - unsigned long value);
> + u32 (*read_count)(void __iomem *base, unsigned long offs);
> + void (*write_count)(void __iomem *base, unsigned long offs, u32 value);
> };
>
> struct sh_cmt_channel {
> @@ -103,9 +102,9 @@ struct sh_cmt_channel {
>
> unsigned int timer_bit;
> unsigned long flags;
> - unsigned long match_value;
> - unsigned long next_match_value;
> - unsigned long max_match_value;
> + u32 match_value;
> + u32 next_match_value;
> + u32 max_match_value;
> raw_spinlock_t lock;
> struct clock_event_device ced;
> struct clocksource cs;
> @@ -160,24 +159,22 @@ struct sh_cmt_device {
> #define SH_CMT32_CMCSR_CKS_RCLK1 (7 << 0)
> #define SH_CMT32_CMCSR_CKS_MASK (7 << 0)
>
> -static unsigned long sh_cmt_read16(void __iomem *base, unsigned long offs)
> +static u32 sh_cmt_read16(void __iomem *base, unsigned long offs)
> {
> return ioread16(base + (offs << 1));
> }
>
> -static unsigned long sh_cmt_read32(void __iomem *base, unsigned long offs)
> +static u32 sh_cmt_read32(void __iomem *base, unsigned long offs)
> {
> return ioread32(base + (offs << 2));
> }
>
> -static void sh_cmt_write16(void __iomem *base, unsigned long offs,
> -unsigned long value)
> +static void sh_cmt_write16(void __iomem *base, unsigned long offs, u32 value)
> {
> iowrite16(value, base + (offs << 1));
> }
>
> -static void sh_cmt_write32(void __iomem *base, unsigned long offs,
> -unsigned long value)
> +static void sh_cmt_write32(void __iomem *base, unsigned long offs, u32 value)
> {
> iowrite32(value, base + (offs << 2));
> }
> @@ -242,7 +239,7 @@ static const struct sh_cmt_info sh_cmt_i
> #define CMCNT 1 /* channel register */
> #define CMCOR 2 /* channel register */
>
> -static inline unsigned long sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
> +static inline u32 sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
> {
> if (ch->iostart)
> return ch->cmt->info->
[PATCH v3] clocksource: sh_cmt: fixup for 64-bit machines
When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
/proc/timer_list. It turned out that when calculating it, the driver did
1 << 32 (causing what I think was undefined behavior) resulting in a zero
delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
to be that the driver abused *unsigned long* for the CMT register values
(which are 16/32-bit), so that the calculation of 'ch->max_match_value'
in sh_cmt_setup_channel() used the wrong branch. Using more proper 'u32'
instead fixed 'max_delta_ns' and even fixed the switching an active
clocksource to CMT (which caused the system to turn non-interactive
before).
Signed-off-by: Sergei Shtylyov
---
This patch is against the 'tip.git' repo's 'timers/core' branch.
The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
enabling building of this driver now, so not sure how urgent is this...
Changes in version 3:
- made the 'overflow_bit' and 'clear_bits' members of 'struct sh_cmt_info'
'u32';
- made the 2nd parameter of sh_cmt_write_cmstr() 'u32';
- made the result, the 2nd parameter, and 'o{1|2}' local variables of
sh_cmt_get_counter() 'u32';
- made the 'has_wrapped' local variables 'u32' in sh_cmt_clocksource_read()
and sh_cmt_clock_event_program_verify();
- fixed a typo in the patch description.
Changes in version 2:
- completely redid the patch, fixing abuse of *unsigned long* for the CMT
register values.
drivers/clocksource/sh_cmt.c | 72 +++
1 file changed, 33 insertions(+), 39 deletions(-)
Index: tip/drivers/clocksource/sh_cmt.c
===
--- tip.orig/drivers/clocksource/sh_cmt.c
+++ tip/drivers/clocksource/sh_cmt.c
@@ -78,18 +78,17 @@ struct sh_cmt_info {
unsigned int channels_mask;
unsigned long width; /* 16 or 32 bit version of hardware block */
- unsigned long overflow_bit;
- unsigned long clear_bits;
+ u32 overflow_bit;
+ u32 clear_bits;
/* callbacks for CMSTR and CMCSR access */
- unsigned long (*read_control)(void __iomem *base, unsigned long offs);
+ u32 (*read_control)(void __iomem *base, unsigned long offs);
void (*write_control)(void __iomem *base, unsigned long offs,
- unsigned long value);
+ u32 value);
/* callbacks for CMCNT and CMCOR access */
- unsigned long (*read_count)(void __iomem *base, unsigned long offs);
- void (*write_count)(void __iomem *base, unsigned long offs,
- unsigned long value);
+ u32 (*read_count)(void __iomem *base, unsigned long offs);
+ void (*write_count)(void __iomem *base, unsigned long offs, u32 value);
};
struct sh_cmt_channel {
@@ -103,9 +102,9 @@ struct sh_cmt_channel {
unsigned int timer_bit;
unsigned long flags;
- unsigned long match_value;
- unsigned long next_match_value;
- unsigned long max_match_value;
+ u32 match_value;
+ u32 next_match_value;
+ u32 max_match_value;
raw_spinlock_t lock;
struct clock_event_device ced;
struct clocksource cs;
@@ -160,24 +159,22 @@ struct sh_cmt_device {
#define SH_CMT32_CMCSR_CKS_RCLK1 (7 << 0)
#define SH_CMT32_CMCSR_CKS_MASK(7 << 0)
-static unsigned long sh_cmt_read16(void __iomem *base, unsigned long offs)
+static u32 sh_cmt_read16(void __iomem *base, unsigned long offs)
{
return ioread16(base + (offs << 1));
}
-static unsigned long sh_cmt_read32(void __iomem *base, unsigned long offs)
+static u32 sh_cmt_read32(void __iomem *base, unsigned long offs)
{
return ioread32(base + (offs << 2));
}
-static void sh_cmt_write16(void __iomem *base, unsigned long offs,
- unsigned long value)
+static void sh_cmt_write16(void __iomem *base, unsigned long offs, u32 value)
{
iowrite16(value, base + (offs << 1));
}
-static void sh_cmt_write32(void __iomem *base, unsigned long offs,
- unsigned long value)
+static void sh_cmt_write32(void __iomem *base, unsigned long offs, u32 value)
{
iowrite32(value, base + (offs << 2));
}
@@ -242,7 +239,7 @@ static const struct sh_cmt_info sh_cmt_i
#define CMCNT 1 /* channel register */
#define CMCOR 2 /* channel register */
-static inline unsigned long sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
+static inline u32 sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
{
if (ch->iostart)
return ch->cmt->info->read_control(ch->iostart, 0);
@@ -250,8 +247,7 @@ static inline unsigned long sh_cmt_read_
return ch->cmt->info->read_control(ch->cmt->mapbase, 0);
}
-static inline void sh_cmt_write_cmstr(struct sh_cmt_channel *ch,
- unsigned long value)
+static inline void
