Re: [RFC PATCH v4 03/21] x86/hpet: Calculate ticks-per-second in a separate function
On Tue, 18 Jun 2019, Ricardo Neri wrote: > On Fri, Jun 14, 2019 at 05:54:05PM +0200, Thomas Gleixner wrote: > > > > So we already have ticks per second, aka frequency, right? So why do we > > need yet another function instead of using the value which is computed > > once? The frequency of the HPET channels has to be identical no matter > > what. If it's not HPET is broken beyond repair. > > I don't think it needs to be recomputed again. I missed the fact that > the frequency was already computed here. > > Also, the hpet char driver has its own frequency computation. Perhaps it > could also obtain it from here, right? No. It's separate on purpose. Hint: IA64, aka Itanic Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v4 03/21] x86/hpet: Calculate ticks-per-second in a separate function
On Fri, Jun 14, 2019 at 05:54:05PM +0200, Thomas Gleixner wrote:
> On Thu, 23 May 2019, Ricardo Neri wrote:
> >
> > +u64 hpet_get_ticks_per_sec(u64 hpet_caps)
> > +{
> > + u64 ticks_per_sec, period;
> > +
> > + period = (hpet_caps & HPET_COUNTER_CLK_PERIOD_MASK) >>
> > +HPET_COUNTER_CLK_PERIOD_SHIFT; /* fs, 10^-15 */
> > +
> > + /*
> > +* The frequency is the reciprocal of the period. The period is given
> > +* in femtoseconds per second. Thus, prepare a dividend to obtain the
> > +* frequency in ticks per second.
> > +*/
> > +
> > + /* 10^15 femtoseconds per second */
> > + ticks_per_sec = 1000ULL;
> > + ticks_per_sec += period >> 1; /* round */
> > +
> > + /* The quotient is put in the dividend. We drop the remainder. */
> > + do_div(ticks_per_sec, period);
> > +
> > + return ticks_per_sec;
> > +}
> > +
> > int hpet_alloc(struct hpet_data *hdp)
> > {
> > u64 cap, mcfg;
> > @@ -844,7 +867,6 @@ int hpet_alloc(struct hpet_data *hdp)
> > struct hpets *hpetp;
> > struct hpet __iomem *hpet;
> > static struct hpets *last;
> > - unsigned long period;
> > unsigned long long temp;
> > u32 remainder;
> >
> > @@ -894,12 +916,7 @@ int hpet_alloc(struct hpet_data *hdp)
> >
> > last = hpetp;
> >
> > - period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>
> > - HPET_COUNTER_CLK_PERIOD_SHIFT; /* fs, 10^-15 */
> > - temp = 1000uLL; /* 10^15 femtoseconds per second */
> > - temp += period >> 1; /* round */
> > - do_div(temp, period);
> > - hpetp->hp_tick_freq = temp; /* ticks per second */
> > + hpetp->hp_tick_freq = hpet_get_ticks_per_sec(cap);
>
> Why are we actually computing this over and over?
>
> In hpet_enable() which is the first function invoked we have:
>
> /*
> * The period is a femto seconds value. Convert it to a
> * frequency.
> */
> freq = FSEC_PER_SEC;
> do_div(freq, hpet_period);
> hpet_freq = freq;
>
> So we already have ticks per second, aka frequency, right? So why do we
> need yet another function instead of using the value which is computed
> once? The frequency of the HPET channels has to be identical no matter
> what. If it's not HPET is broken beyond repair.
I don't think it needs to be recomputed again. I missed the fact that
the frequency was already computed here.
Also, the hpet char driver has its own frequency computation. Perhaps it
could also obtain it from here, right?
Thanks and BR,
Ricardo
>
> Thanks,
>
> tglx
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v4 03/21] x86/hpet: Calculate ticks-per-second in a separate function
On Fri, 14 Jun 2019, Thomas Gleixner wrote:
> On Thu, 23 May 2019, Ricardo Neri wrote:
> > int hpet_alloc(struct hpet_data *hdp)
> > {
> > u64 cap, mcfg;
> > @@ -844,7 +867,6 @@ int hpet_alloc(struct hpet_data *hdp)
> > struct hpets *hpetp;
> > struct hpet __iomem *hpet;
> > static struct hpets *last;
> > - unsigned long period;
> > unsigned long long temp;
> > u32 remainder;
> >
> > @@ -894,12 +916,7 @@ int hpet_alloc(struct hpet_data *hdp)
> >
> > last = hpetp;
> >
> > - period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>
> > - HPET_COUNTER_CLK_PERIOD_SHIFT; /* fs, 10^-15 */
> > - temp = 1000uLL; /* 10^15 femtoseconds per second */
> > - temp += period >> 1; /* round */
> > - do_div(temp, period);
> > - hpetp->hp_tick_freq = temp; /* ticks per second */
> > + hpetp->hp_tick_freq = hpet_get_ticks_per_sec(cap);
>
> Why are we actually computing this over and over?
>
> In hpet_enable() which is the first function invoked we have:
>
> /*
> * The period is a femto seconds value. Convert it to a
> * frequency.
> */
> freq = FSEC_PER_SEC;
> do_div(freq, hpet_period);
> hpet_freq = freq;
>
> So we already have ticks per second, aka frequency, right? So why do we
> need yet another function instead of using the value which is computed
> once? The frequency of the HPET channels has to be identical no matter
> what. If it's not HPET is broken beyond repair.
Aside of that this change breaks the IA64 support for /dev/hpet.
Thanks,
tglx
Re: [RFC PATCH v4 03/21] x86/hpet: Calculate ticks-per-second in a separate function
On Thu, 23 May 2019, Ricardo Neri wrote:
>
> +u64 hpet_get_ticks_per_sec(u64 hpet_caps)
> +{
> + u64 ticks_per_sec, period;
> +
> + period = (hpet_caps & HPET_COUNTER_CLK_PERIOD_MASK) >>
> + HPET_COUNTER_CLK_PERIOD_SHIFT; /* fs, 10^-15 */
> +
> + /*
> + * The frequency is the reciprocal of the period. The period is given
> + * in femtoseconds per second. Thus, prepare a dividend to obtain the
> + * frequency in ticks per second.
> + */
> +
> + /* 10^15 femtoseconds per second */
> + ticks_per_sec = 1000ULL;
> + ticks_per_sec += period >> 1; /* round */
> +
> + /* The quotient is put in the dividend. We drop the remainder. */
> + do_div(ticks_per_sec, period);
> +
> + return ticks_per_sec;
> +}
> +
> int hpet_alloc(struct hpet_data *hdp)
> {
> u64 cap, mcfg;
> @@ -844,7 +867,6 @@ int hpet_alloc(struct hpet_data *hdp)
> struct hpets *hpetp;
> struct hpet __iomem *hpet;
> static struct hpets *last;
> - unsigned long period;
> unsigned long long temp;
> u32 remainder;
>
> @@ -894,12 +916,7 @@ int hpet_alloc(struct hpet_data *hdp)
>
> last = hpetp;
>
> - period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>
> - HPET_COUNTER_CLK_PERIOD_SHIFT; /* fs, 10^-15 */
> - temp = 1000uLL; /* 10^15 femtoseconds per second */
> - temp += period >> 1; /* round */
> - do_div(temp, period);
> - hpetp->hp_tick_freq = temp; /* ticks per second */
> + hpetp->hp_tick_freq = hpet_get_ticks_per_sec(cap);
Why are we actually computing this over and over?
In hpet_enable() which is the first function invoked we have:
/*
* The period is a femto seconds value. Convert it to a
* frequency.
*/
freq = FSEC_PER_SEC;
do_div(freq, hpet_period);
hpet_freq = freq;
So we already have ticks per second, aka frequency, right? So why do we
need yet another function instead of using the value which is computed
once? The frequency of the HPET channels has to be identical no matter
what. If it's not HPET is broken beyond repair.
Thanks,
tglx
[RFC PATCH v4 03/21] x86/hpet: Calculate ticks-per-second in a separate function
It is easier to compute the expiration times of an HPET timer by using
its frequency (i.e., the number of times it ticks in a second) than its
period, as given in the capabilities register.
In addition to the HPET char driver, the HPET-based hardlockup detector
will also need to know the timer's frequency. Thus, create a common
function that both can use.
Cc: "H. Peter Anvin"
Cc: Ashok Raj
Cc: Andi Kleen
Cc: Tony Luck
Cc: Clemens Ladisch
Cc: Arnd Bergmann
Cc: Philippe Ombredanne
Cc: Kate Stewart
Cc: "Rafael J. Wysocki"
Cc: Stephane Eranian
Cc: Suravee Suthikulpanit
Cc: "Ravi V. Shankar"
Cc: x...@kernel.org
Signed-off-by: Ricardo Neri
---
drivers/char/hpet.c | 31 ---
include/linux/hpet.h | 1 +
2 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index 3a1e6b3ccd10..747255f552a9 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -836,6 +836,29 @@ static unsigned long hpet_calibrate(struct hpets *hpetp)
return ret;
}
+u64 hpet_get_ticks_per_sec(u64 hpet_caps)
+{
+ u64 ticks_per_sec, period;
+
+ period = (hpet_caps & HPET_COUNTER_CLK_PERIOD_MASK) >>
+HPET_COUNTER_CLK_PERIOD_SHIFT; /* fs, 10^-15 */
+
+ /*
+* The frequency is the reciprocal of the period. The period is given
+* in femtoseconds per second. Thus, prepare a dividend to obtain the
+* frequency in ticks per second.
+*/
+
+ /* 10^15 femtoseconds per second */
+ ticks_per_sec = 1000ULL;
+ ticks_per_sec += period >> 1; /* round */
+
+ /* The quotient is put in the dividend. We drop the remainder. */
+ do_div(ticks_per_sec, period);
+
+ return ticks_per_sec;
+}
+
int hpet_alloc(struct hpet_data *hdp)
{
u64 cap, mcfg;
@@ -844,7 +867,6 @@ int hpet_alloc(struct hpet_data *hdp)
struct hpets *hpetp;
struct hpet __iomem *hpet;
static struct hpets *last;
- unsigned long period;
unsigned long long temp;
u32 remainder;
@@ -894,12 +916,7 @@ int hpet_alloc(struct hpet_data *hdp)
last = hpetp;
- period = (cap & HPET_COUNTER_CLK_PERIOD_MASK) >>
- HPET_COUNTER_CLK_PERIOD_SHIFT; /* fs, 10^-15 */
- temp = 1000uLL; /* 10^15 femtoseconds per second */
- temp += period >> 1; /* round */
- do_div(temp, period);
- hpetp->hp_tick_freq = temp; /* ticks per second */
+ hpetp->hp_tick_freq = hpet_get_ticks_per_sec(cap);
printk(KERN_INFO "hpet%d: at MMIO 0x%lx, IRQ%s",
hpetp->hp_which, hdp->hd_phys_address,
diff --git a/include/linux/hpet.h b/include/linux/hpet.h
index 8604564b985d..e7b36bcf4699 100644
--- a/include/linux/hpet.h
+++ b/include/linux/hpet.h
@@ -107,5 +107,6 @@ static inline void hpet_reserve_timer(struct hpet_data *hd,
int timer)
}
int hpet_alloc(struct hpet_data *);
+u64 hpet_get_ticks_per_sec(u64 hpet_caps);
#endif /* !__HPET__ */
--
2.17.1
