Re: armv7/arm64: just a bit || some tc_counter_mask fixes
On Mon, Mar 12, 2018 at 11:25:09PM +0100, Mark Kettenis wrote:
> > Date: Fri, 9 Mar 2018 19:43:42 +0200
> > From: Artturi Alm
> >
> > Hi,
> >
> > lazy retry[0],
> > to give these timers their (likely) copy-paste-robbed MSB back.
> >
> > -Artturi
> >
> > [0] https://marc.info/?l=openbsd-tech&m=150421823512699&w=1
>
> This doesn't actually fix anything; 31 bits are plenty. Leaving off
> the top bit protects against signed/unsigned mistakes although there
> shouldn't be any. So I'm not sure this is worth it.
>
forgot about the 'protects against..':
iirc. that mask is only ever used in sys/kern/kern_tc.c, so within
these timers it does nothing, so i see no protection from it.
i think it would be known by now if kern_tc.c was unable to get
along w/the 32bit rollover.
-Artturi
> > diff --git sys/arch/arm/cortex/agtimer.c sys/arch/arm/cortex/agtimer.c
> > index 8d622c058a4..160e22e6949 100644
> > --- sys/arch/arm/cortex/agtimer.c
> > +++ sys/arch/arm/cortex/agtimer.c
> > @@ -46,7 +46,7 @@ int32_t agtimer_frequency = TIMER_FREQUENCY;
> > u_int agtimer_get_timecount(struct timecounter *);
> >
> > static struct timecounter agtimer_timecounter = {
> > - agtimer_get_timecount, NULL, 0x7fff, 0, "agtimer", 0, NULL
> > + agtimer_get_timecount, NULL, 0x, 0, "agtimer", 0, NULL
> > };
> >
> > #define MAX_ARM_CPUS 8
> > diff --git sys/arch/arm/cortex/amptimer.c sys/arch/arm/cortex/amptimer.c
> > index 81880c1574a..66f0ccfed64 100644
> > --- sys/arch/arm/cortex/amptimer.c
> > +++ sys/arch/arm/cortex/amptimer.c
> > @@ -67,7 +67,7 @@ int32_t amptimer_frequency = TIMER_FREQUENCY;
> > u_int amptimer_get_timecount(struct timecounter *);
> >
> > static struct timecounter amptimer_timecounter = {
> > - amptimer_get_timecount, NULL, 0x7fff, 0, "amptimer", 0, NULL
> > + amptimer_get_timecount, NULL, 0x, 0, "amptimer", 0, NULL
> > };
> >
> > #define MAX_ARM_CPUS 8
> > diff --git sys/arch/arm64/dev/agtimer.c sys/arch/arm64/dev/agtimer.c
> > index 0e6e6a3bc6e..7d9557e706f 100644
> > --- sys/arch/arm64/dev/agtimer.c
> > +++ sys/arch/arm64/dev/agtimer.c
> > @@ -43,7 +43,7 @@ int32_t agtimer_frequency = TIMER_FREQUENCY;
> > u_int agtimer_get_timecount(struct timecounter *);
> >
> > static struct timecounter agtimer_timecounter = {
> > - agtimer_get_timecount, NULL, 0x7fff, 0, "agtimer", 0, NULL
> > + agtimer_get_timecount, NULL, 0x, 0, "agtimer", 0, NULL
> > };
> >
> > #define MAX_ARM_CPUS 8
> > diff --git sys/arch/armv7/omap/gptimer.c sys/arch/armv7/omap/gptimer.c
> > index e87db41106e..2e6c8cf8d42 100644
> > --- sys/arch/armv7/omap/gptimer.c
> > +++ sys/arch/armv7/omap/gptimer.c
> > @@ -117,7 +117,7 @@ int gptimer_irq = 0;
> > u_int gptimer_get_timecount(struct timecounter *);
> >
> > static struct timecounter gptimer_timecounter = {
> > - gptimer_get_timecount, NULL, 0x7fff, 0, "gptimer", 0, NULL
> > + gptimer_get_timecount, NULL, 0x, 0, "gptimer", 0, NULL
> > };
> >
> > volatile u_int32_t nexttickevent;
> >
> >
Re: armv7/arm64: just a bit || some tc_counter_mask fixes
On Mon, Mar 12, 2018 at 11:25:09PM +0100, Mark Kettenis wrote: > > Date: Fri, 9 Mar 2018 19:43:42 +0200 > > From: Artturi Alm > > > > Hi, > > > > lazy retry[0], > > to give these timers their (likely) copy-paste-robbed MSB back. > > > > -Artturi > > > > [0] https://marc.info/?l=openbsd-tech&m=150421823512699&w=1 > > This doesn't actually fix anything; 31 bits are plenty. Leaving off > the top bit protects against signed/unsigned mistakes although there > shouldn't be any. So I'm not sure this is worth it. > I still think the current masks are only brought over(by accident) from some macppc timer actually having had only 31bits in the early 90s. also, fwiw. all these timers have atleast 32bits in use on other BSDs. now if focusing only the documentation issue i'm having with these masks in use, is that either this(comment) should be changed(sys/timetc.h): 47 timecounter_get_t *tc_get_timecount; 48 /* 49 * This function reads the counter. It is not required to 50 * mask any unimplemented bits out, as long as they are 51 * constant. 52 */ or the *tc_get_timecount functions of affected timers be changed to mask out the non-constant bit, which i think would be silly. tbh., i'm only worried about amptimer w/high default freq(396MHz), and only because i'm still optimistic we would see the HZ bump[0] in near future, making the lack of MSB possibly more of an possible issue? -Artturi [0] https://marc.info/?l=openbsd-tech&m=150274123011009&w=1
Re: armv7/arm64: just a bit || some tc_counter_mask fixes
> Date: Fri, 9 Mar 2018 19:43:42 +0200
> From: Artturi Alm
>
> Hi,
>
> lazy retry[0],
> to give these timers their (likely) copy-paste-robbed MSB back.
>
> -Artturi
>
> [0] https://marc.info/?l=openbsd-tech&m=150421823512699&w=1
This doesn't actually fix anything; 31 bits are plenty. Leaving off
the top bit protects against signed/unsigned mistakes although there
shouldn't be any. So I'm not sure this is worth it.
> diff --git sys/arch/arm/cortex/agtimer.c sys/arch/arm/cortex/agtimer.c
> index 8d622c058a4..160e22e6949 100644
> --- sys/arch/arm/cortex/agtimer.c
> +++ sys/arch/arm/cortex/agtimer.c
> @@ -46,7 +46,7 @@ int32_t agtimer_frequency = TIMER_FREQUENCY;
> u_int agtimer_get_timecount(struct timecounter *);
>
> static struct timecounter agtimer_timecounter = {
> - agtimer_get_timecount, NULL, 0x7fff, 0, "agtimer", 0, NULL
> + agtimer_get_timecount, NULL, 0x, 0, "agtimer", 0, NULL
> };
>
> #define MAX_ARM_CPUS 8
> diff --git sys/arch/arm/cortex/amptimer.c sys/arch/arm/cortex/amptimer.c
> index 81880c1574a..66f0ccfed64 100644
> --- sys/arch/arm/cortex/amptimer.c
> +++ sys/arch/arm/cortex/amptimer.c
> @@ -67,7 +67,7 @@ int32_t amptimer_frequency = TIMER_FREQUENCY;
> u_int amptimer_get_timecount(struct timecounter *);
>
> static struct timecounter amptimer_timecounter = {
> - amptimer_get_timecount, NULL, 0x7fff, 0, "amptimer", 0, NULL
> + amptimer_get_timecount, NULL, 0x, 0, "amptimer", 0, NULL
> };
>
> #define MAX_ARM_CPUS 8
> diff --git sys/arch/arm64/dev/agtimer.c sys/arch/arm64/dev/agtimer.c
> index 0e6e6a3bc6e..7d9557e706f 100644
> --- sys/arch/arm64/dev/agtimer.c
> +++ sys/arch/arm64/dev/agtimer.c
> @@ -43,7 +43,7 @@ int32_t agtimer_frequency = TIMER_FREQUENCY;
> u_int agtimer_get_timecount(struct timecounter *);
>
> static struct timecounter agtimer_timecounter = {
> - agtimer_get_timecount, NULL, 0x7fff, 0, "agtimer", 0, NULL
> + agtimer_get_timecount, NULL, 0x, 0, "agtimer", 0, NULL
> };
>
> #define MAX_ARM_CPUS 8
> diff --git sys/arch/armv7/omap/gptimer.c sys/arch/armv7/omap/gptimer.c
> index e87db41106e..2e6c8cf8d42 100644
> --- sys/arch/armv7/omap/gptimer.c
> +++ sys/arch/armv7/omap/gptimer.c
> @@ -117,7 +117,7 @@ int gptimer_irq = 0;
> u_int gptimer_get_timecount(struct timecounter *);
>
> static struct timecounter gptimer_timecounter = {
> - gptimer_get_timecount, NULL, 0x7fff, 0, "gptimer", 0, NULL
> + gptimer_get_timecount, NULL, 0x, 0, "gptimer", 0, NULL
> };
>
> volatile u_int32_t nexttickevent;
>
>
armv7/arm64: just a bit || some tc_counter_mask fixes
Hi,
lazy retry[0],
to give these timers their (likely) copy-paste-robbed MSB back.
-Artturi
[0] https://marc.info/?l=openbsd-tech&m=150421823512699&w=1
diff --git sys/arch/arm/cortex/agtimer.c sys/arch/arm/cortex/agtimer.c
index 8d622c058a4..160e22e6949 100644
--- sys/arch/arm/cortex/agtimer.c
+++ sys/arch/arm/cortex/agtimer.c
@@ -46,7 +46,7 @@ int32_t agtimer_frequency = TIMER_FREQUENCY;
u_int agtimer_get_timecount(struct timecounter *);
static struct timecounter agtimer_timecounter = {
- agtimer_get_timecount, NULL, 0x7fff, 0, "agtimer", 0, NULL
+ agtimer_get_timecount, NULL, 0x, 0, "agtimer", 0, NULL
};
#define MAX_ARM_CPUS 8
diff --git sys/arch/arm/cortex/amptimer.c sys/arch/arm/cortex/amptimer.c
index 81880c1574a..66f0ccfed64 100644
--- sys/arch/arm/cortex/amptimer.c
+++ sys/arch/arm/cortex/amptimer.c
@@ -67,7 +67,7 @@ int32_t amptimer_frequency = TIMER_FREQUENCY;
u_int amptimer_get_timecount(struct timecounter *);
static struct timecounter amptimer_timecounter = {
- amptimer_get_timecount, NULL, 0x7fff, 0, "amptimer", 0, NULL
+ amptimer_get_timecount, NULL, 0x, 0, "amptimer", 0, NULL
};
#define MAX_ARM_CPUS 8
diff --git sys/arch/arm64/dev/agtimer.c sys/arch/arm64/dev/agtimer.c
index 0e6e6a3bc6e..7d9557e706f 100644
--- sys/arch/arm64/dev/agtimer.c
+++ sys/arch/arm64/dev/agtimer.c
@@ -43,7 +43,7 @@ int32_t agtimer_frequency = TIMER_FREQUENCY;
u_int agtimer_get_timecount(struct timecounter *);
static struct timecounter agtimer_timecounter = {
- agtimer_get_timecount, NULL, 0x7fff, 0, "agtimer", 0, NULL
+ agtimer_get_timecount, NULL, 0x, 0, "agtimer", 0, NULL
};
#define MAX_ARM_CPUS 8
diff --git sys/arch/armv7/omap/gptimer.c sys/arch/armv7/omap/gptimer.c
index e87db41106e..2e6c8cf8d42 100644
--- sys/arch/armv7/omap/gptimer.c
+++ sys/arch/armv7/omap/gptimer.c
@@ -117,7 +117,7 @@ int gptimer_irq = 0;
u_int gptimer_get_timecount(struct timecounter *);
static struct timecounter gptimer_timecounter = {
- gptimer_get_timecount, NULL, 0x7fff, 0, "gptimer", 0, NULL
+ gptimer_get_timecount, NULL, 0x, 0, "gptimer", 0, NULL
};
volatile u_int32_t nexttickevent;
