[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Ivan Gorinov]

Adding HPET as an alternative timer for x86 (default is TSC).
HPET counter has constant frequency and does not need calibration.
This change also makes TSC timer driver optional on x86.
If X86_TSC is disabled, early timer functions are provided by HPET.

Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
---
 arch/Kconfig               |   2 +-
 arch/x86/dts/hpet.dtsi     |   7 ++
 drivers/timer/Kconfig      |   6 ++
 drivers/timer/Makefile     |   1 +
 drivers/timer/hpet_timer.c | 191 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 206 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/dts/hpet.dtsi
 create mode 100644 drivers/timer/hpet_timer.c

diff --git a/arch/Kconfig b/arch/Kconfig
index e599e7a..37dabae 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -104,7 +104,7 @@ config X86
 	select DM_PCI
 	select PCI
 	select TIMER
-	select X86_TSC_TIMER
+	imply X86_TSC_TIMER
 	imply BLK
 	imply DM_ETH
 	imply DM_GPIO
diff --git a/arch/x86/dts/hpet.dtsi b/arch/x86/dts/hpet.dtsi
new file mode 100644
index 0000000..037dc7e
--- /dev/null
+++ b/arch/x86/dts/hpet.dtsi
@@ -0,0 +1,7 @@
+/ {
+	hpet0: hpet at fed00000 {
+		compatible = "hpet-x86";
+		u-boot,dm-pre-reloc;
+		reg = <0xfed00000 0x1000>;
+	};
+};
diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
index 2c96896..72ae6c5 100644
--- a/drivers/timer/Kconfig
+++ b/drivers/timer/Kconfig
@@ -65,6 +65,12 @@ config X86_TSC_TIMER
 	help
 	  Select this to enable Time-Stamp Counter (TSC) timer for x86.
 
+config HPET_TIMER
+	bool "High Precision Event Timers (HPET) support"
+	depends on TIMER && X86
+	help
+	  Select this to enable High Precision Event Timers (HPET) for x86.
+
 config OMAP_TIMER
 	bool "Omap timer support"
 	depends on TIMER
diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile
index a6e7832..557fecc 100644
--- a/drivers/timer/Makefile
+++ b/drivers/timer/Makefile
@@ -8,6 +8,7 @@ obj-y += timer-uclass.o
 obj-$(CONFIG_ALTERA_TIMER)	+= altera_timer.o
 obj-$(CONFIG_SANDBOX_TIMER)	+= sandbox_timer.o
 obj-$(CONFIG_X86_TSC_TIMER)	+= tsc_timer.o
+obj-$(CONFIG_HPET_TIMER)	+= hpet_timer.o
 obj-$(CONFIG_OMAP_TIMER)	+= omap-timer.o
 obj-$(CONFIG_AST_TIMER)	+= ast_timer.o
 obj-$(CONFIG_STI_TIMER)		+= sti-timer.o
diff --git a/drivers/timer/hpet_timer.c b/drivers/timer/hpet_timer.c
new file mode 100644
index 0000000..0bef859
--- /dev/null
+++ b/drivers/timer/hpet_timer.c
@@ -0,0 +1,191 @@
+/*
+ * Copyright (c) 2017 Intel Corporation
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <malloc.h>
+#include <timer.h>
+#include <asm/cpu.h>
+#include <asm/io.h>
+#include <asm/u-boot-x86.h>
+
+#define HPET_PERIOD_REG 0x004
+#define HPET_CONFIG_REG 0x010
+#define HPET_MAIN_COUNT_L 0x0f0
+#define HPET_MAIN_COUNT_H 0x0f4
+
+#define HPET_MAX_PERIOD 100000000
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct hpet_timer_priv {
+	void *regs;
+};
+
+/*
+ * Returns HPET clock frequency in HZ
+ * (rounding to the nearest integer),
+ * or 0 if HPET is not available.
+ */
+static inline u32 get_clock_frequency(void *regs)
+{
+	u64 d = 1000000000000000ull;
+	u32 period;
+
+	period = readl(regs + HPET_PERIOD_REG);
+	if (period == 0)
+		return 0;
+	if (period > HPET_MAX_PERIOD)
+		return 0;
+
+	d += period / 2;
+
+	return d / period;
+}
+
+/*
+ * Reset and start the main counter.
+ */
+static void start_main_counter(void *regs)
+{
+	u32 config;
+
+	config = readl(regs + HPET_CONFIG_REG);
+	config &= ~1;
+	writel(config, regs + HPET_CONFIG_REG);
+	writel(0, regs + HPET_MAIN_COUNT_L);
+	writel(0, regs + HPET_MAIN_COUNT_H);
+	config |= 1;
+	writel(config, regs + HPET_CONFIG_REG);
+}
+
+/*
+ * Read the main counter as two 32-bit registers,
+ * repeat if rollover happens.
+ */
+static u64 read_main_counter(void *regs)
+{
+	u64 now_tick;
+	u32 tl, th, th0;
+
+	th = readl(regs + HPET_MAIN_COUNT_H);
+	do {
+		th0 = th;
+		tl = readl(regs + HPET_MAIN_COUNT_L);
+		th = readl(regs + HPET_MAIN_COUNT_H);
+	} while (th != th0);
+
+	now_tick = th;
+	now_tick <<= 32;
+	now_tick |= tl;
+
+	return now_tick;
+}
+
+static int hpet_timer_get_count(struct udevice *dev, u64 *count)
+{
+	struct hpet_timer_priv *priv = dev_get_priv(dev);
+
+	*count = read_main_counter(priv->regs);
+
+	return 0;
+}
+
+static int hpet_timer_probe(struct udevice *dev)
+{
+	struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+	struct hpet_timer_priv *priv = dev_get_priv(dev);
+	u32 rate;
+
+	rate = get_clock_frequency(priv->regs);
+	if (rate == 0)
+		return -ENODEV;
+
+	start_main_counter(priv->regs);
+
+	uc_priv->clock_rate = rate;
+
+	return 0;
+}
+
+#ifndef CONFIG_X86_TSC_TIMER
+
+static void *early_regs = (void *) CONFIG_HPET_ADDRESS;
+
+unsigned long notrace timer_early_get_rate(void)
+{
+	return get_clock_frequency(early_regs);
+}
+
+u64 notrace timer_early_get_count(void)
+{
+	return read_main_counter(early_regs);
+}
+
+int timer_init(void)
+{
+	if (get_clock_frequency(early_regs) == 0)
+		return -ENODEV;
+
+	start_main_counter(early_regs);
+
+	return 0;
+}
+
+ulong timer_get_boot_us(void)
+{
+	u32 period;
+	u64 d;
+
+	period = readl(early_regs + HPET_PERIOD_REG);
+	if (period == 0)
+		return 0;
+	if (period > HPET_MAX_PERIOD)
+		return 0;
+
+	d = read_main_counter(early_regs);
+
+	/*
+	 * Multiplication overflow
+	 * at 2^64 femtoseconds
+	 * (more than 5 hours)
+	 */
+
+	d *= period;
+
+	return d / 1000000000;
+}
+
+#endif /* CONFIG_X86_TSC_TIMER */
+
+static int hpet_timer_ofdata_to_platdata(struct udevice *dev)
+{
+	struct hpet_timer_priv *priv = dev_get_priv(dev);
+	priv->regs = map_physmem(devfdt_get_addr(dev), 0x1000, MAP_NOCACHE);
+
+	return 0;
+}
+
+static const struct timer_ops hpet_timer_ops = {
+	.get_count = hpet_timer_get_count,
+};
+
+static const struct udevice_id hpet_timer_ids[] = {
+	{ .compatible = "hpet-x86", },
+	{ .compatible = "intel,ce4100-hpet", },
+	{ }
+};
+
+U_BOOT_DRIVER(hpet_timer) = {
+	.name	= "hpet_timer",
+	.id	= UCLASS_TIMER,
+	.of_match = hpet_timer_ids,
+	.ofdata_to_platdata = hpet_timer_ofdata_to_platdata,
+	.priv_auto_alloc_size = sizeof(struct hpet_timer_priv),
+	.probe = hpet_timer_probe,
+	.ops	= &hpet_timer_ops,
+	.flags = DM_FLAG_PRE_RELOC,
+};
-- 
2.7.4

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Bin Meng]

Hi Ivan,

On Fri, Mar 30, 2018 at 6:29 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> Adding HPET as an alternative timer for x86 (default is TSC).
> HPET counter has constant frequency and does not need calibration.
> This change also makes TSC timer driver optional on x86.
> If X86_TSC is disabled, early timer functions are provided by HPET.
>
> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
> ---
>  arch/Kconfig               |   2 +-
>  arch/x86/dts/hpet.dtsi     |   7 ++
>  drivers/timer/Kconfig      |   6 ++
>  drivers/timer/Makefile     |   1 +
>  drivers/timer/hpet_timer.c | 191 +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 206 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/dts/hpet.dtsi
>  create mode 100644 drivers/timer/hpet_timer.c
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index e599e7a..37dabae 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -104,7 +104,7 @@ config X86
>         select DM_PCI
>         select PCI
>         select TIMER
> -       select X86_TSC_TIMER
> +       imply X86_TSC_TIMER
>         imply BLK
>         imply DM_ETH
>         imply DM_GPIO
> diff --git a/arch/x86/dts/hpet.dtsi b/arch/x86/dts/hpet.dtsi
> new file mode 100644
> index 0000000..037dc7e
> --- /dev/null
> +++ b/arch/x86/dts/hpet.dtsi
> @@ -0,0 +1,7 @@
> +/ {
> +       hpet0: hpet at fed00000 {

nits: use 'hpet' instead of 'hpet0'? since there is only one hpet
timer in the system.

> +               compatible = "hpet-x86";
> +               u-boot,dm-pre-reloc;
> +               reg = <0xfed00000 0x1000>;
> +       };
> +};
> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
> index 2c96896..72ae6c5 100644
> --- a/drivers/timer/Kconfig
> +++ b/drivers/timer/Kconfig
> @@ -65,6 +65,12 @@ config X86_TSC_TIMER
>         help
>           Select this to enable Time-Stamp Counter (TSC) timer for x86.
>
> +config HPET_TIMER
> +       bool "High Precision Event Timers (HPET) support"
> +       depends on TIMER && X86
> +       help
> +         Select this to enable High Precision Event Timers (HPET) for x86.
> +
>  config OMAP_TIMER
>         bool "Omap timer support"
>         depends on TIMER
> diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile
> index a6e7832..557fecc 100644
> --- a/drivers/timer/Makefile
> +++ b/drivers/timer/Makefile
> @@ -8,6 +8,7 @@ obj-y += timer-uclass.o
>  obj-$(CONFIG_ALTERA_TIMER)     += altera_timer.o
>  obj-$(CONFIG_SANDBOX_TIMER)    += sandbox_timer.o
>  obj-$(CONFIG_X86_TSC_TIMER)    += tsc_timer.o
> +obj-$(CONFIG_HPET_TIMER)       += hpet_timer.o
>  obj-$(CONFIG_OMAP_TIMER)       += omap-timer.o
>  obj-$(CONFIG_AST_TIMER)        += ast_timer.o
>  obj-$(CONFIG_STI_TIMER)                += sti-timer.o
> diff --git a/drivers/timer/hpet_timer.c b/drivers/timer/hpet_timer.c
> new file mode 100644
> index 0000000..0bef859
> --- /dev/null
> +++ b/drivers/timer/hpet_timer.c
> @@ -0,0 +1,191 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <malloc.h>
> +#include <timer.h>
> +#include <asm/cpu.h>
> +#include <asm/io.h>
> +#include <asm/u-boot-x86.h>
> +
> +#define HPET_PERIOD_REG 0x004
> +#define HPET_CONFIG_REG 0x010
> +#define HPET_MAIN_COUNT_L 0x0f0
> +#define HPET_MAIN_COUNT_H 0x0f4
> +
> +#define HPET_MAX_PERIOD 100000000
> +
> +DECLARE_GLOBAL_DATA_PTR;

This is not needed.

> +
> +struct hpet_timer_priv {
> +       void *regs;
> +};
> +
> +/*
> + * Returns HPET clock frequency in HZ
> + * (rounding to the nearest integer),
> + * or 0 if HPET is not available.
> + */
> +static inline u32 get_clock_frequency(void *regs)
> +{
> +       u64 d = 1000000000000000ull;
> +       u32 period;
> +
> +       period = readl(regs + HPET_PERIOD_REG);
> +       if (period == 0)
> +               return 0;
> +       if (period > HPET_MAX_PERIOD)
> +               return 0;
> +
> +       d += period / 2;
> +
> +       return d / period;
> +}
> +
> +/*
> + * Reset and start the main counter.
> + */

nits: use one line comment format

> +static void start_main_counter(void *regs)
> +{
> +       u32 config;
> +
> +       config = readl(regs + HPET_CONFIG_REG);
> +       config &= ~1;
> +       writel(config, regs + HPET_CONFIG_REG);
> +       writel(0, regs + HPET_MAIN_COUNT_L);
> +       writel(0, regs + HPET_MAIN_COUNT_H);
> +       config |= 1;
> +       writel(config, regs + HPET_CONFIG_REG);
> +}
> +
> +/*
> + * Read the main counter as two 32-bit registers,
> + * repeat if rollover happens.
> + */
> +static u64 read_main_counter(void *regs)
> +{
> +       u64 now_tick;
> +       u32 tl, th, th0;
> +
> +       th = readl(regs + HPET_MAIN_COUNT_H);
> +       do {
> +               th0 = th;
> +               tl = readl(regs + HPET_MAIN_COUNT_L);
> +               th = readl(regs + HPET_MAIN_COUNT_H);
> +       } while (th != th0);
> +
> +       now_tick = th;
> +       now_tick <<= 32;
> +       now_tick |= tl;
> +
> +       return now_tick;
> +}
> +
> +static int hpet_timer_get_count(struct udevice *dev, u64 *count)
> +{
> +       struct hpet_timer_priv *priv = dev_get_priv(dev);
> +
> +       *count = read_main_counter(priv->regs);
> +
> +       return 0;
> +}
> +
> +static int hpet_timer_probe(struct udevice *dev)
> +{
> +       struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +       struct hpet_timer_priv *priv = dev_get_priv(dev);
> +       u32 rate;
> +
> +       rate = get_clock_frequency(priv->regs);
> +       if (rate == 0)
> +               return -ENODEV;
> +
> +       start_main_counter(priv->regs);
> +
> +       uc_priv->clock_rate = rate;
> +
> +       return 0;
> +}
> +
> +#ifndef CONFIG_X86_TSC_TIMER

This looks not good. If we support more timers on x86, the #ifdef
logic will become un-maintainable. Can we support such from the timer
library instead?

> +
> +static void *early_regs = (void *) CONFIG_HPET_ADDRESS;
> +
> +unsigned long notrace timer_early_get_rate(void)
> +{
> +       return get_clock_frequency(early_regs);
> +}
> +
> +u64 notrace timer_early_get_count(void)
> +{
> +       return read_main_counter(early_regs);
> +}
> +
> +int timer_init(void)
> +{
> +       if (get_clock_frequency(early_regs) == 0)
> +               return -ENODEV;
> +
> +       start_main_counter(early_regs);
> +
> +       return 0;
> +}
> +
> +ulong timer_get_boot_us(void)
> +{
> +       u32 period;
> +       u64 d;
> +
> +       period = readl(early_regs + HPET_PERIOD_REG);
> +       if (period == 0)
> +               return 0;
> +       if (period > HPET_MAX_PERIOD)
> +               return 0;
> +
> +       d = read_main_counter(early_regs);
> +
> +       /*
> +        * Multiplication overflow
> +        * at 2^64 femtoseconds
> +        * (more than 5 hours)
> +        */
> +
> +       d *= period;
> +
> +       return d / 1000000000;
> +}
> +
> +#endif /* CONFIG_X86_TSC_TIMER */
> +
> +static int hpet_timer_ofdata_to_platdata(struct udevice *dev)
> +{
> +       struct hpet_timer_priv *priv = dev_get_priv(dev);
> +       priv->regs = map_physmem(devfdt_get_addr(dev), 0x1000, MAP_NOCACHE);
> +
> +       return 0;
> +}
> +
> +static const struct timer_ops hpet_timer_ops = {
> +       .get_count = hpet_timer_get_count,
> +};
> +
> +static const struct udevice_id hpet_timer_ids[] = {
> +       { .compatible = "hpet-x86", },
> +       { .compatible = "intel,ce4100-hpet", },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(hpet_timer) = {
> +       .name   = "hpet_timer",
> +       .id     = UCLASS_TIMER,
> +       .of_match = hpet_timer_ids,
> +       .ofdata_to_platdata = hpet_timer_ofdata_to_platdata,
> +       .priv_auto_alloc_size = sizeof(struct hpet_timer_priv),
> +       .probe = hpet_timer_probe,
> +       .ops    = &hpet_timer_ops,
> +       .flags = DM_FLAG_PRE_RELOC,
> +};
> --

I will try to test this patch on several x86 boards.

Regards,
Bin

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Andy Shevchenko]

On Fri, Mar 30, 2018 at 1:29 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> Adding HPET as an alternative timer for x86 (default is TSC).
> HPET counter has constant frequency and does not need calibration.
> This change also makes TSC timer driver optional on x86.
> If X86_TSC is disabled, early timer functions are provided by HPET.
>
> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
> ---

Changelog?

>  arch/Kconfig               |   2 +-

> +config HPET_TIMER
> +       bool "High Precision Event Timers (HPET) support"
> +       depends on TIMER && X86

Does X86 makes any difference from building point of view?

> +       config = readl(regs + HPET_CONFIG_REG);
> +       config &= ~1;
> +       writel(config, regs + HPET_CONFIG_REG);

1  or BIT(0) is magic. Can be fixed in all places?

> +       writel(0, regs + HPET_MAIN_COUNT_L);
> +       writel(0, regs + HPET_MAIN_COUNT_H);

Can we use writeq() here?

> +               tl = readl(regs + HPET_MAIN_COUNT_L);
> +               th = readl(regs + HPET_MAIN_COUNT_H);

Ditto.

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Ivan Gorinov]

On Fri, Mar 30, 2018 at 10:46:40PM +0300, Andy Shevchenko wrote:

> > +       writel(0, regs + HPET_MAIN_COUNT_L);
> > +       writel(0, regs + HPET_MAIN_COUNT_H);
> 
> Can we use writeq() here?

I don't see readq/writeq defined for x86, even x86_64.

> > +               tl = readl(regs + HPET_MAIN_COUNT_L);
> > +               th = readl(regs + HPET_MAIN_COUNT_H);
> 
> Ditto.

If readq() is defined as two read operations in 32-bit code, main counter
rollover (low part overflow, high part increment) can happen between them.

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Andy Shevchenko]

On Sat, Mar 31, 2018 at 4:03 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> On Fri, Mar 30, 2018 at 10:46:40PM +0300, Andy Shevchenko wrote:
>
>> > +       writel(0, regs + HPET_MAIN_COUNT_L);
>> > +       writel(0, regs + HPET_MAIN_COUNT_H);
>>
>> Can we use writeq() here?
>
> I don't see readq/writeq defined for x86, even x86_64.
>

read_le64() and write_le64() in U-Boot source code.

Besides that there are memcpy_toio() / memcpy_fromio() defined (didn't
check if they are really implemented).

>> > +               tl = readl(regs + HPET_MAIN_COUNT_L);
>> > +               th = readl(regs + HPET_MAIN_COUNT_H);
>>
>> Ditto.
>
> If readq() is defined as two read operations in 32-bit code, main counter
> rollover (low part overflow, high part increment) can happen between them.

And how this contradicts ther current code?

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Ivan Gorinov]

On Sat, Mar 31, 2018 at 06:31:03AM -0600, Andy Shevchenko wrote:
> >> > +               tl = readl(regs + HPET_MAIN_COUNT_L);
> >> > +               th = readl(regs + HPET_MAIN_COUNT_H);
> >>
> >> Ditto.
> >
> > If readq() is defined as two read operations in 32-bit code, main counter
> > rollover (low part overflow, high part increment) can happen between them.
> 
> And how this contradicts ther current code?

It just does not make the code simpler, rollover check is
still required if U-Boot is compiled as 32-bit code.

Can we do something like the following?


#ifdef CONFIG_X86_64

static u64 read_main_counter(void *regs)
{
	return readq(regs + HPET_MAIN_COUNT);
}

#else

/*
 * Read the main counter as two 32-bit registers,
 * repeat if rollover happens.
 */
static u64 read_main_counter(void *regs)
{
	u64 now_tick;
	u32 tl, th, th0;

	th = readl(regs + HPET_MAIN_COUNT_H);
	do {
		th0 = th;
		tl = readl(regs + HPET_MAIN_COUNT_L);
		th = readl(regs + HPET_MAIN_COUNT_H);
		now_tick = th;
		now_tick <<= 32;
		now_tick |= tl;
	} while (th != th0);

	return now_tick;
}

#endif

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Andy Shevchenko]

On Tue, Apr 3, 2018 at 2:00 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> On Sat, Mar 31, 2018 at 06:31:03AM -0600, Andy Shevchenko wrote:
>> >> > +               tl = readl(regs + HPET_MAIN_COUNT_L);
>> >> > +               th = readl(regs + HPET_MAIN_COUNT_H);
>> >>
>> >> Ditto.
>> >
>> > If readq() is defined as two read operations in 32-bit code, main counter
>> > rollover (low part overflow, high part increment) can happen between them.
>>
>> And how this contradicts ther current code?
>
> It just does not make the code simpler,

...b/c you misread what I suggested.

> rollover check is
> still required if U-Boot is compiled as 32-bit code.
>
> Can we do something like the following?

Yes, but... why?

What's wrong with replacing two sequential 32-bit reads with one 64-bit?

See below.

> #ifdef CONFIG_X86_64
>
> static u64 read_main_counter(void *regs)
> {
>         return readq(regs + HPET_MAIN_COUNT);
> }
>
> #else
>
> /*
>  * Read the main counter as two 32-bit registers,
>  * repeat if rollover happens.
>  */
> static u64 read_main_counter(void *regs)
> {
>         u64 now_tick;
>         u32 tl, th, th0;
>
>         th = readl(regs + HPET_MAIN_COUNT_H);
>         do {

now_tick = readq();

>                 th0 = th;
>                 tl = readl(regs + HPET_MAIN_COUNT_L);
>                 th = readl(regs + HPET_MAIN_COUNT_H);
>                 now_tick = th;
>                 now_tick <<= 32;
>                 now_tick |= tl;

>         } while (th != th0);

} while (th != (now_tick >> 32));

>
>         return now_tick;
> }
>
> #endif


-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Ivan Gorinov]

On Tue, Apr 03, 2018 at 06:17:42AM -0600, Andy Shevchenko wrote:
> >> > If readq() is defined as two read operations in 32-bit code, main counter
> >> > rollover (low part overflow, high part increment) can happen between them.
> >> And how this contradicts ther current code?
> > It just does not make the code simpler,
> ...b/c you misread what I suggested.
> > rollover check is
> > still required if U-Boot is compiled as 32-bit code.
> > Can we do something like the following?
> Yes, but... why?
> What's wrong with replacing two sequential 32-bit reads with one 64-bit?

Doesn't readX/writeX imply a single I/O operation?
It may be misleading to define it as two.

Assuming MMX or SSE2 to be supported by all x86 processors, 64-bit I/O
registers can be accessed as a single operation even in 32-bit code:

static inline u64 readq(void *addr)
{
	u64 value;

	asm volatile ("movq (%0), %%xmm0" : : "r" (addr));
	asm volatile ("movq %%xmm0, %0" : "=m" (value));

	return value;
}

I can add these definitions to "asm/io.h".

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Bin Meng]

On Wed, Apr 4, 2018 at 7:26 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> On Tue, Apr 03, 2018 at 06:17:42AM -0600, Andy Shevchenko wrote:
>> >> > If readq() is defined as two read operations in 32-bit code, main counter
>> >> > rollover (low part overflow, high part increment) can happen between them.
>> >> And how this contradicts ther current code?
>> > It just does not make the code simpler,
>> ...b/c you misread what I suggested.
>> > rollover check is
>> > still required if U-Boot is compiled as 32-bit code.
>> > Can we do something like the following?
>> Yes, but... why?
>> What's wrong with replacing two sequential 32-bit reads with one 64-bit?
>
> Doesn't readX/writeX imply a single I/O operation?
> It may be misleading to define it as two.
>
> Assuming MMX or SSE2 to be supported by all x86 processors, 64-bit I/O
> registers can be accessed as a single operation even in 32-bit code:
>

Adding such requirement (MMX or SSE2) to U-Boot is not good. Why do we
require MMX or SSE2 for readq? Can we use general purpose registers?

> static inline u64 readq(void *addr)
> {
>         u64 value;
>
>         asm volatile ("movq (%0), %%xmm0" : : "r" (addr));
>         asm volatile ("movq %%xmm0, %0" : "=m" (value));
>
>         return value;
> }
>
> I can add these definitions to "asm/io.h".

Regards,
Bin

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Ivan Gorinov]

On Wed, Apr 04, 2018 at 12:15:24PM +0800, Bin Meng wrote:
> > Doesn't readX/writeX imply a single I/O operation?
> > It may be misleading to define it as two.
> >
> > Assuming MMX or SSE2 to be supported by all x86 processors, 64-bit I/O
> > registers can be accessed as a single operation even in 32-bit code:
> >
> 
> Adding such requirement (MMX or SSE2) to U-Boot is not good. Why do we
> require MMX or SSE2 for readq? Can we use general purpose registers?

In 32-bit code, we can't make a 64-bit memory read operation using only
general purpose registers.

> 
> > static inline u64 readq(void *addr)
> > {
> >         u64 value;
> >
> >         asm volatile ("movq (%0), %%xmm0" : : "r" (addr));
> >         asm volatile ("movq %%xmm0, %0" : "=m" (value));
> >
> >         return value;
> > }
> >
> > I can add these definitions to "asm/io.h".

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Andy Shevchenko]

On Wed, Apr 4, 2018 at 2:26 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> On Tue, Apr 03, 2018 at 06:17:42AM -0600, Andy Shevchenko wrote:
>> >> > If readq() is defined as two read operations in 32-bit code, main counter
>> >> > rollover (low part overflow, high part increment) can happen between them.
>> >> And how this contradicts ther current code?
>> > It just does not make the code simpler,
>> ...b/c you misread what I suggested.
>> > rollover check is
>> > still required if U-Boot is compiled as 32-bit code.
>> > Can we do something like the following?
>> Yes, but... why?
>> What's wrong with replacing two sequential 32-bit reads with one 64-bit?
>
> Doesn't readX/writeX imply a single I/O operation?

Not always.
It depends on atomicity you would like to get.

On 32-bit platform it's impossible to do readq() at once.

> It may be misleading to define it as two.
>
> Assuming MMX or SSE2 to be supported by all x86 processors, 64-bit I/O
> registers can be accessed as a single operation even in 32-bit code:
>
> static inline u64 readq(void *addr)
> {
>         u64 value;
>
>         asm volatile ("movq (%0), %%xmm0" : : "r" (addr));
>         asm volatile ("movq %%xmm0, %0" : "=m" (value));
>
>         return value;
> }
>
> I can add these definitions to "asm/io.h".

Please, no. This is wrong approach.

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2] timer: add High Precision Event Timers (HPET) support

[Andy Shevchenko]

On Wed, Apr 4, 2018 at 7:40 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> On Wed, Apr 04, 2018 at 12:15:24PM +0800, Bin Meng wrote:
>> > Doesn't readX/writeX imply a single I/O operation?
>> > It may be misleading to define it as two.
>> >
>> > Assuming MMX or SSE2 to be supported by all x86 processors, 64-bit I/O
>> > registers can be accessed as a single operation even in 32-bit code:
>> >
>>
>> Adding such requirement (MMX or SSE2) to U-Boot is not good. Why do we
>> require MMX or SSE2 for readq? Can we use general purpose registers?
>
> In 32-bit code, we can't make a 64-bit memory read operation using only
> general purpose registers.

Exactly, that's why you have to use non-atomic variants.

-- 
With Best Regards,
Andy Shevchenko