[Qemu-devel] [PATCH 0/3] aspeed: introduce the APB clock settings

[Qemu-devel] [PATCH 0/3] aspeed: introduce the APB clock settings

[Cédric Le Goater]

Hello,

The Aspeed SoC clocks are driven by an input source clock which can
have different frequencies : 24MHz or 25MHz, and also, on the Aspeed
AST2400 SoC, 48MHz. The H-PLL (CPU) clock is defined from a calculation
using parameters in the H-PLL Parameter register or from a predefined
set of frequencies if the setting is strapped by hardware (Aspeed
AST2400 SoC). The other clocks of the SoC are then defined from the
H-PLL using dividers.

We first introduce the APB clock because it drives the timer model.
This fixes a slowdown issue on the palmetto machine (AST2400) when
running Linux. The latest Linux versions take into account more
precisely the SoC settings for the clocks and the APB freq is set to
48MHz but modeled at 24MHz by QEMU.

Thanks,

C.

Cédric Le Goater (3):
  aspeed/scu: introduce clock frequencies
  aspeed: initialize the SCU controller first
  aspeed/timer: use the APB frequency from the SCU

 include/hw/misc/aspeed_scu.h    |  70 ++++++++++++++++++++++++--
 include/hw/timer/aspeed_timer.h |   4 ++
 hw/arm/aspeed_soc.c             |  42 ++++++++--------
 hw/misc/aspeed_scu.c            | 106 ++++++++++++++++++++++++++++++++++++++++
 hw/timer/aspeed_timer.c         |  19 +++++--
 5 files changed, 213 insertions(+), 28 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/3] aspeed/scu: introduce clock frequencies

[Cédric Le Goater]

All Aspeed SoC clocks are driven by an input source clock which can
have different frequencies : 24MHz or 25MHz, and also, on the Aspeed
AST2400 SoC, 48MHz. The H-PLL (CPU) clock is defined from a
calculation using parameters in the H-PLL Parameter register or from a
predefined set of frequencies if the setting is strapped by hardware
(Aspeed AST2400 SoC). The other clocks of the SoC are then defined
from the H-PLL using dividers.

We introduce first the APB clock because it drives the timer.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/misc/aspeed_scu.h |  70 ++++++++++++++++++++++++++--
 hw/misc/aspeed_scu.c         | 106 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 172 insertions(+), 4 deletions(-)

diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h
index d70cc0aeca61..f662c38188f4 100644
--- a/include/hw/misc/aspeed_scu.h
+++ b/include/hw/misc/aspeed_scu.h
@@ -30,6 +30,10 @@ typedef struct AspeedSCUState {
     uint32_t hw_strap1;
     uint32_t hw_strap2;
     uint32_t hw_prot_key;
+
+    uint32_t clkin;
+    uint32_t hpll;
+    uint32_t apb_freq;
 } AspeedSCUState;
 
 #define AST2400_A0_SILICON_REV   0x02000303U
@@ -58,7 +62,64 @@ extern bool is_supported_silicon_rev(uint32_t silicon_rev);
  *       1. 2012/12/29 Ryan Chen Create
  */
 
-/* Hardware Strapping Register definition (for Aspeed AST2400 SOC)
+/* SCU08   Clock Selection Register
+ *
+ *  31     Enable Video Engine clock dynamic slow down
+ *  30:28  Video Engine clock slow down setting
+ *  27     2D Engine GCLK clock source selection
+ *  26     2D Engine GCLK clock throttling enable
+ *  25:23  APB PCLK divider selection
+ *  22:20  LPC Host LHCLK divider selection
+ *  19     LPC Host LHCLK clock generation/output enable control
+ *  18:16  MAC AHB bus clock divider selection
+ *  15     SD/SDIO clock running enable
+ *  14:12  SD/SDIO divider selection
+ *  11     Reserved
+ *  10:8   Video port output clock delay control bit
+ *  7      ARM CPU/AHB clock slow down enable
+ *  6:4    ARM CPU/AHB clock slow down setting
+ *  3:2    ECLK clock source selection
+ *  1      CPU/AHB clock slow down idle timer
+ *  0      CPU/AHB clock dynamic slow down enable (defined in bit[6:4])
+ */
+#define SCU_CLK_GET_PCLK_DIV(x)                    (((x) >> 23) & 0x7)
+
+/* SCU24   H-PLL Parameter Register (for Aspeed AST2400 SOC)
+ *
+ *  18     H-PLL parameter selection
+ *           0: Select H-PLL by strapping resistors
+ *           1: Select H-PLL by the programmed registers (SCU24[17:0])
+ *  17     Enable H-PLL bypass mode
+ *  16     Turn off H-PLL
+ *  10:5   H-PLL Numerator
+ *  4      H-PLL Output Divider
+ *  3:0    H-PLL Denumerator
+ *
+ *  (Output frequency) = 24MHz * (2-OD) * [(Numerator+2) / (Denumerator+1)]
+ */
+
+#define SCU_AST2400_H_PLL_PROGRAMMED               (0x1 << 18)
+#define SCU_AST2400_H_PLL_BYPASS_EN                (0x1 << 17)
+#define SCU_AST2400_H_PLL_OFF                      (0x1 << 16)
+
+/* SCU24   H-PLL Parameter Register (for Aspeed AST2500 SOC)
+ *
+ *  21     Enable H-PLL reset
+ *  20     Enable H-PLL bypass mode
+ *  19     Turn off H-PLL
+ *  18:13  H-PLL Post Divider
+ *  12:5   H-PLL Numerator (M)
+ *  4:0    H-PLL Denumerator (N)
+ *
+ *  (Output frequency) = CLKIN(24MHz) * [(M+1) / (N+1)] / (P+1)
+ *
+ * The default frequency is 792Mhz when CLKIN = 24MHz
+ */
+
+#define SCU_H_PLL_BYPASS_EN                        (0x1 << 20)
+#define SCU_H_PLL_OFF                              (0x1 << 19)
+
+/* SCU70  Hardware Strapping Register definition (for Aspeed AST2400 SOC)
  *
  * 31:29  Software defined strapping registers
  * 28:27  DRAM size setting (for VGA driver use)
@@ -107,12 +168,13 @@ extern bool is_supported_silicon_rev(uint32_t silicon_rev);
 #define SCU_AST2400_HW_STRAP_GET_CLK_SOURCE(x)     (((((x) >> 23) & 0x1) << 1) \
                                                     | (((x) >> 18) & 0x1))
 #define SCU_AST2400_HW_STRAP_CLK_SOURCE_MASK       ((0x1 << 23) | (0x1 << 18))
-#define     AST2400_CLK_25M_IN                         (0x1 << 23)
+#define SCU_HW_STRAP_CLK_25M_IN                    (0x1 << 23)
 #define     AST2400_CLK_24M_IN                         0
 #define     AST2400_CLK_48M_IN                         1
 #define     AST2400_CLK_25M_IN_24M_USB_CKI             2
 #define     AST2400_CLK_25M_IN_48M_USB_CKI             3
 
+#define SCU_HW_STRAP_CLK_48M_IN                    (0x1 << 18)
 #define SCU_HW_STRAP_2ND_BOOT_WDT                  (0x1 << 17)
 #define SCU_HW_STRAP_SUPER_IO_CONFIG               (0x1 << 16)
 #define SCU_HW_STRAP_VGA_CLASS_CODE                (0x1 << 15)
@@ -160,8 +222,8 @@ extern bool is_supported_silicon_rev(uint32_t silicon_rev);
 #define     AST2400_DIS_BOOT                           3
 
 /*
- * Hardware strapping register definition (for Aspeed AST2500 SoC and
- * higher)
+ * SCU70  Hardware strapping register definition (for Aspeed AST2500
+ *        SoC and higher)
  *
  * 31     Enable SPI Flash Strap Auto Fetch Mode
  * 30     Enable GPIO Strap Mode
diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 59315010db9a..a17d50edd78d 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -168,6 +168,27 @@ static uint32_t aspeed_scu_get_random(void)
     return num;
 }
 
+static void aspeed_scu_set_apb_freq(AspeedSCUState *s)
+{
+    uint32_t apb_divider;
+
+    switch (s->silicon_rev) {
+    case AST2400_A0_SILICON_REV:
+    case AST2400_A1_SILICON_REV:
+        apb_divider = 2;
+        break;
+    case AST2500_A0_SILICON_REV:
+    case AST2500_A1_SILICON_REV:
+        apb_divider = 4;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    s->apb_freq = s->hpll / (SCU_CLK_GET_PCLK_DIV(s->regs[CLK_SEL]) + 1)
+        / apb_divider;
+}
+
 static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size)
 {
     AspeedSCUState *s = ASPEED_SCU(opaque);
@@ -222,6 +243,10 @@ static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data,
     case PROT_KEY:
         s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
         return;
+    case CLK_SEL:
+        s->regs[reg] = data;
+        aspeed_scu_set_apb_freq(s);
+        break;
 
     case FREQ_CNTR_EVAL:
     case VGA_SCRATCH1 ... VGA_SCRATCH8:
@@ -247,19 +272,93 @@ static const MemoryRegionOps aspeed_scu_ops = {
     .valid.unaligned = false,
 };
 
+static uint32_t aspeed_scu_get_clkin(AspeedSCUState *s)
+{
+    if (s->hw_strap1 & SCU_HW_STRAP_CLK_25M_IN) {
+        return 25000000;
+    } else if (s->hw_strap1 & SCU_HW_STRAP_CLK_48M_IN) {
+        return 48000000;
+    } else {
+        return 24000000;
+    }
+}
+
+/*
+ * Strapped frequencies for the AST2400 in MHz. They depend on the
+ * clkin frequency.
+ */
+static const uint32_t hpll_ast2400_freqs[][4] = {
+    { 384, 360, 336, 408 }, /* 24MHz or 48MHz */
+    { 400, 375, 350, 425 }, /* 25MHz */
+};
+
+static uint32_t aspeed_scu_calc_hpll_ast2400(AspeedSCUState *s)
+{
+    uint32_t hpll_reg = s->regs[HPLL_PARAM];
+    uint8_t freq_select;
+    bool clk_25m_in;
+
+    if (hpll_reg & SCU_AST2400_H_PLL_OFF) {
+        return 0;
+    }
+
+    if (hpll_reg & SCU_AST2400_H_PLL_PROGRAMMED) {
+        uint32_t multiplier = 1;
+
+        if (!(hpll_reg & SCU_AST2400_H_PLL_BYPASS_EN)) {
+            uint32_t n  = (hpll_reg >> 5) & 0x3f;
+            uint32_t od = (hpll_reg >> 4) & 0x1;
+            uint32_t d  = hpll_reg & 0xf;
+
+            multiplier = (2 - od) * ((n + 2) / (d + 1));
+        }
+
+        return s->clkin * multiplier;
+    }
+
+    /* HW strapping */
+    clk_25m_in = (s->hw_strap1 & SCU_HW_STRAP_CLK_25M_IN);
+    freq_select = SCU_AST2400_HW_STRAP_GET_H_PLL_CLK(s->hw_strap1);
+
+    return hpll_ast2400_freqs[clk_25m_in][freq_select] * 1000000;
+}
+
+static uint32_t aspeed_scu_calc_hpll_ast2500(AspeedSCUState *s)
+{
+    uint32_t hpll_reg   = s->regs[HPLL_PARAM];
+    uint32_t multiplier = 1;
+
+    if (hpll_reg & SCU_H_PLL_OFF) {
+        return 0;
+    }
+
+    if (!(hpll_reg & SCU_H_PLL_BYPASS_EN)) {
+        uint32_t p = (hpll_reg >> 13) & 0x3f;
+        uint32_t m = (hpll_reg >> 5) & 0xff;
+        uint32_t n = hpll_reg & 0x1f;
+
+        multiplier = ((m + 1) / (n + 1)) / (p + 1);
+    }
+
+    return s->clkin * multiplier;
+}
+
 static void aspeed_scu_reset(DeviceState *dev)
 {
     AspeedSCUState *s = ASPEED_SCU(dev);
     const uint32_t *reset;
+    uint32_t (*calc_hpll)(AspeedSCUState *s);
 
     switch (s->silicon_rev) {
     case AST2400_A0_SILICON_REV:
     case AST2400_A1_SILICON_REV:
         reset = ast2400_a0_resets;
+        calc_hpll = aspeed_scu_calc_hpll_ast2400;
         break;
     case AST2500_A0_SILICON_REV:
     case AST2500_A1_SILICON_REV:
         reset = ast2500_a1_resets;
+        calc_hpll = aspeed_scu_calc_hpll_ast2500;
         break;
     default:
         g_assert_not_reached();
@@ -270,6 +369,13 @@ static void aspeed_scu_reset(DeviceState *dev)
     s->regs[HW_STRAP1] = s->hw_strap1;
     s->regs[HW_STRAP2] = s->hw_strap2;
     s->regs[PROT_KEY] = s->hw_prot_key;
+
+    /*
+     * All registers are set. Now compute the frequencies of the main clocks
+     */
+    s->clkin = aspeed_scu_get_clkin(s);
+    s->hpll = calc_hpll(s);
+    aspeed_scu_set_apb_freq(s);
 }
 
 static uint32_t aspeed_silicon_revs[] = {
-- 
2.13.6

Re: [Qemu-devel] [PATCH 1/3] aspeed/scu: introduce clock frequencies

[Joel Stanley]

On 22 June 2018 at 08:09, Cédric Le Goater <clg@kaod.org> wrote:
> All Aspeed SoC clocks are driven by an input source clock which can
> have different frequencies : 24MHz or 25MHz, and also, on the Aspeed
> AST2400 SoC, 48MHz. The H-PLL (CPU) clock is defined from a
> calculation using parameters in the H-PLL Parameter register or from a
> predefined set of frequencies if the setting is strapped by hardware
> (Aspeed AST2400 SoC). The other clocks of the SoC are then defined
> from the H-PLL using dividers.
>
> We introduce first the APB clock because it drives the timer.

Looks good! One small issue below.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/misc/aspeed_scu.h |  70 ++++++++++++++++++++++++++--
>  hw/misc/aspeed_scu.c         | 106 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 172 insertions(+), 4 deletions(-)
>

> +static uint32_t aspeed_scu_calc_hpll_ast2400(AspeedSCUState *s)
> +{
> +    uint32_t hpll_reg = s->regs[HPLL_PARAM];
> +    uint8_t freq_select;
> +    bool clk_25m_in;
> +
> +    if (hpll_reg & SCU_AST2400_H_PLL_OFF) {
> +        return 0;
> +    }
> +
> +    if (hpll_reg & SCU_AST2400_H_PLL_PROGRAMMED) {
> +        uint32_t multiplier = 1;
> +
> +        if (!(hpll_reg & SCU_AST2400_H_PLL_BYPASS_EN)) {
> +            uint32_t n  = (hpll_reg >> 5) & 0x3f;
> +            uint32_t od = (hpll_reg >> 4) & 0x1;
> +            uint32_t d  = hpll_reg & 0xf;
> +
> +            multiplier = (2 - od) * ((n + 2) / (d + 1));
> +        }
> +
> +        return s->clkin * multiplier;
> +    }
> +
> +    /* HW strapping */
> +    clk_25m_in = (s->hw_strap1 & SCU_HW_STRAP_CLK_25M_IN);

I think you want to do !! to this result, or shift it down. Otherwise
you are getting 1 << 23 or zero, when I think you want 1 or 0.

> +    freq_select = SCU_AST2400_HW_STRAP_GET_H_PLL_CLK(s->hw_strap1);
> +
> +    return hpll_ast2400_freqs[clk_25m_in][freq_select] * 1000000;
> +}

Re: [Qemu-devel] [PATCH 1/3] aspeed/scu: introduce clock frequencies

[Cédric Le Goater]

On 06/22/2018 02:57 AM, Joel Stanley wrote:
> On 22 June 2018 at 08:09, Cédric Le Goater <clg@kaod.org> wrote:
>> All Aspeed SoC clocks are driven by an input source clock which can
>> have different frequencies : 24MHz or 25MHz, and also, on the Aspeed
>> AST2400 SoC, 48MHz. The H-PLL (CPU) clock is defined from a
>> calculation using parameters in the H-PLL Parameter register or from a
>> predefined set of frequencies if the setting is strapped by hardware
>> (Aspeed AST2400 SoC). The other clocks of the SoC are then defined
>> from the H-PLL using dividers.
>>
>> We introduce first the APB clock because it drives the timer.
> 
> Looks good! One small issue below.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/misc/aspeed_scu.h |  70 ++++++++++++++++++++++++++--
>>  hw/misc/aspeed_scu.c         | 106 +++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 172 insertions(+), 4 deletions(-)
>>
> 
>> +static uint32_t aspeed_scu_calc_hpll_ast2400(AspeedSCUState *s)
>> +{
>> +    uint32_t hpll_reg = s->regs[HPLL_PARAM];
>> +    uint8_t freq_select;
>> +    bool clk_25m_in;
>> +
>> +    if (hpll_reg & SCU_AST2400_H_PLL_OFF) {
>> +        return 0;
>> +    }
>> +
>> +    if (hpll_reg & SCU_AST2400_H_PLL_PROGRAMMED) {
>> +        uint32_t multiplier = 1;
>> +
>> +        if (!(hpll_reg & SCU_AST2400_H_PLL_BYPASS_EN)) {
>> +            uint32_t n  = (hpll_reg >> 5) & 0x3f;
>> +            uint32_t od = (hpll_reg >> 4) & 0x1;
>> +            uint32_t d  = hpll_reg & 0xf;
>> +
>> +            multiplier = (2 - od) * ((n + 2) / (d + 1));
>> +        }
>> +
>> +        return s->clkin * multiplier;
>> +    }
>> +
>> +    /* HW strapping */
>> +    clk_25m_in = (s->hw_strap1 & SCU_HW_STRAP_CLK_25M_IN);
> 
> I think you want to do !! to this result, or shift it down. Otherwise
> you are getting 1 << 23 or zero, when I think you want 1 or 0.

yes. I will fix that.

Thanks,

C.

>> +    freq_select = SCU_AST2400_HW_STRAP_GET_H_PLL_CLK(s->hw_strap1);
>> +
>> +    return hpll_ast2400_freqs[clk_25m_in][freq_select] * 1000000;
>> +}

[Qemu-devel] [PATCH 2/3] aspeed: initialize the SCU controller first

[Cédric Le Goater]

The System Control Unit should be initialized first as it drives all
the configuration of the SoC and other device models.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed_soc.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 1955a892f4a4..7cc05ee27ea4 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -109,18 +109,6 @@ static void aspeed_soc_init(Object *obj)
     object_initialize(&s->cpu, sizeof(s->cpu), sc->info->cpu_type);
     object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
 
-    object_initialize(&s->vic, sizeof(s->vic), TYPE_ASPEED_VIC);
-    object_property_add_child(obj, "vic", OBJECT(&s->vic), NULL);
-    qdev_set_parent_bus(DEVICE(&s->vic), sysbus_get_default());
-
-    object_initialize(&s->timerctrl, sizeof(s->timerctrl), TYPE_ASPEED_TIMER);
-    object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), NULL);
-    qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default());
-
-    object_initialize(&s->i2c, sizeof(s->i2c), TYPE_ASPEED_I2C);
-    object_property_add_child(obj, "i2c", OBJECT(&s->i2c), NULL);
-    qdev_set_parent_bus(DEVICE(&s->i2c), sysbus_get_default());
-
     object_initialize(&s->scu, sizeof(s->scu), TYPE_ASPEED_SCU);
     object_property_add_child(obj, "scu", OBJECT(&s->scu), NULL);
     qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
@@ -133,6 +121,18 @@ static void aspeed_soc_init(Object *obj)
     object_property_add_alias(obj, "hw-prot-key", OBJECT(&s->scu),
                               "hw-prot-key", &error_abort);
 
+    object_initialize(&s->vic, sizeof(s->vic), TYPE_ASPEED_VIC);
+    object_property_add_child(obj, "vic", OBJECT(&s->vic), NULL);
+    qdev_set_parent_bus(DEVICE(&s->vic), sysbus_get_default());
+
+    object_initialize(&s->timerctrl, sizeof(s->timerctrl), TYPE_ASPEED_TIMER);
+    object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), NULL);
+    qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default());
+
+    object_initialize(&s->i2c, sizeof(s->i2c), TYPE_ASPEED_I2C);
+    object_property_add_child(obj, "i2c", OBJECT(&s->i2c), NULL);
+    qdev_set_parent_bus(DEVICE(&s->i2c), sysbus_get_default());
+
     object_initialize(&s->fmc, sizeof(s->fmc), sc->info->fmc_typename);
     object_property_add_child(obj, "fmc", OBJECT(&s->fmc), NULL);
     qdev_set_parent_bus(DEVICE(&s->fmc), sysbus_get_default());
@@ -195,6 +195,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(get_system_memory(), ASPEED_SOC_SRAM_BASE,
                                 &s->sram);
 
+    /* SCU */
+    object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->scu), 0, ASPEED_SOC_SCU_BASE);
+
     /* VIC */
     object_property_set_bool(OBJECT(&s->vic), true, "realized", &err);
     if (err) {
@@ -219,14 +227,6 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
     }
 
-    /* SCU */
-    object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->scu), 0, ASPEED_SOC_SCU_BASE);
-
     /* UART - attach an 8250 to the IO space as our UART5 */
     if (serial_hd(0)) {
         qemu_irq uart5 = qdev_get_gpio_in(DEVICE(&s->vic), uart_irqs[4]);
-- 
2.13.6

Re: [Qemu-devel] [PATCH 2/3] aspeed: initialize the SCU controller first

[Joel Stanley]

On 22 June 2018 at 08:09, Cédric Le Goater <clg@kaod.org> wrote:
> The System Control Unit should be initialized first as it drives all
> the configuration of the SoC and other device models.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/arm/aspeed_soc.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)

[Qemu-devel] [PATCH 3/3] aspeed/timer: use the APB frequency from the SCU

[Cédric Le Goater]

The timer controller can be driven by either an external 1MHz clock or
by the APB clock. Today, the model makes this assumption that the APB
frequency is 24MHz but this is incorrect on the AST2400 SoC. palmetto
machines use a 48MHz input clock source.

Use the SCU object to get the APB frequency and drive the timers with
the configured freq for the SoC.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/timer/aspeed_timer.h |  4 ++++
 hw/arm/aspeed_soc.c             |  2 ++
 hw/timer/aspeed_timer.c         | 19 +++++++++++++++----
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
index bd6c1a7f9609..040a08873432 100644
--- a/include/hw/timer/aspeed_timer.h
+++ b/include/hw/timer/aspeed_timer.h
@@ -24,6 +24,8 @@
 
 #include "qemu/timer.h"
 
+typedef struct AspeedSCUState AspeedSCUState;
+
 #define ASPEED_TIMER(obj) \
     OBJECT_CHECK(AspeedTimerCtrlState, (obj), TYPE_ASPEED_TIMER);
 #define TYPE_ASPEED_TIMER "aspeed.timer"
@@ -55,6 +57,8 @@ typedef struct AspeedTimerCtrlState {
     uint32_t ctrl;
     uint32_t ctrl2;
     AspeedTimer timers[ASPEED_TIMER_NR_TIMERS];
+
+    AspeedSCUState *scu;
 } AspeedTimerCtrlState;
 
 #endif /* ASPEED_TIMER_H */
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 7cc05ee27ea4..e68911af0f90 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -127,6 +127,8 @@ static void aspeed_soc_init(Object *obj)
 
     object_initialize(&s->timerctrl, sizeof(s->timerctrl), TYPE_ASPEED_TIMER);
     object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), NULL);
+    object_property_add_const_link(OBJECT(&s->timerctrl), "scu",
+                                   OBJECT(&s->scu), &error_abort);
     qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default());
 
     object_initialize(&s->i2c, sizeof(s->i2c), TYPE_ASPEED_I2C);
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 1e31e22b6f1f..5e3f51b66b43 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -10,8 +10,10 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "hw/timer/aspeed_timer.h"
+#include "hw/misc/aspeed_scu.h"
 #include "qemu-common.h"
 #include "qemu/bitops.h"
 #include "qemu/timer.h"
@@ -26,7 +28,6 @@
 #define TIMER_CLOCK_USE_EXT true
 #define TIMER_CLOCK_EXT_HZ 1000000
 #define TIMER_CLOCK_USE_APB false
-#define TIMER_CLOCK_APB_HZ 24000000
 
 #define TIMER_REG_STATUS 0
 #define TIMER_REG_RELOAD 1
@@ -80,11 +81,11 @@ static inline bool timer_external_clock(AspeedTimer *t)
     return timer_ctrl_status(t, op_external_clock);
 }
 
-static uint32_t clock_rates[] = { TIMER_CLOCK_APB_HZ, TIMER_CLOCK_EXT_HZ };
-
 static inline uint32_t calculate_rate(struct AspeedTimer *t)
 {
-    return clock_rates[timer_external_clock(t)];
+    AspeedTimerCtrlState *s = timer_to_ctrl(t);
+
+    return timer_external_clock(t) ? TIMER_CLOCK_EXT_HZ : s->scu->apb_freq;
 }
 
 static inline uint32_t calculate_ticks(struct AspeedTimer *t, uint64_t now_ns)
@@ -449,6 +450,16 @@ static void aspeed_timer_realize(DeviceState *dev, Error **errp)
     int i;
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     AspeedTimerCtrlState *s = ASPEED_TIMER(dev);
+    Object *obj;
+    Error *err = NULL;
+
+    obj = object_property_get_link(OBJECT(dev), "scu", &err);
+    if (!obj) {
+        error_propagate(errp, err);
+        error_prepend(errp, "required link 'scu' not found: ");
+        return;
+    }
+    s->scu = ASPEED_SCU(obj);
 
     for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
         aspeed_init_one_timer(s, i);
-- 
2.13.6

Re: [Qemu-devel] [PATCH 3/3] aspeed/timer: use the APB frequency from the SCU

[Joel Stanley]

On 22 June 2018 at 08:09, Cédric Le Goater <clg@kaod.org> wrote:
> The timer controller can be driven by either an external 1MHz clock or
> by the APB clock. Today, the model makes this assumption that the APB
> frequency is 24MHz but this is incorrect on the AST2400 SoC. palmetto
> machines use a 48MHz input clock source.

So this fixes it to use a apb frequency from the scu for all platform,
not just the 2400?

Can you put something in the commit message about the change in guest
behaviour - the output of Linux's clk_summary or similar - so we know
what bug this fixed?

The code looks good though.

Reviewed-by: Joel Stanley <joel@jms.id.au>

> Use the SCU object to get the APB frequency and drive the timers with
> the configured freq for the SoC.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/timer/aspeed_timer.h |  4 ++++
>  hw/arm/aspeed_soc.c             |  2 ++
>  hw/timer/aspeed_timer.c         | 19 +++++++++++++++----
>  3 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
> index bd6c1a7f9609..040a08873432 100644
> --- a/include/hw/timer/aspeed_timer.h
> +++ b/include/hw/timer/aspeed_timer.h
> @@ -24,6 +24,8 @@
>
>  #include "qemu/timer.h"
>
> +typedef struct AspeedSCUState AspeedSCUState;
> +
>  #define ASPEED_TIMER(obj) \
>      OBJECT_CHECK(AspeedTimerCtrlState, (obj), TYPE_ASPEED_TIMER);
>  #define TYPE_ASPEED_TIMER "aspeed.timer"
> @@ -55,6 +57,8 @@ typedef struct AspeedTimerCtrlState {
>      uint32_t ctrl;
>      uint32_t ctrl2;
>      AspeedTimer timers[ASPEED_TIMER_NR_TIMERS];
> +
> +    AspeedSCUState *scu;
>  } AspeedTimerCtrlState;
>
>  #endif /* ASPEED_TIMER_H */
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 7cc05ee27ea4..e68911af0f90 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -127,6 +127,8 @@ static void aspeed_soc_init(Object *obj)
>
>      object_initialize(&s->timerctrl, sizeof(s->timerctrl), TYPE_ASPEED_TIMER);
>      object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), NULL);
> +    object_property_add_const_link(OBJECT(&s->timerctrl), "scu",
> +                                   OBJECT(&s->scu), &error_abort);
>      qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default());
>
>      object_initialize(&s->i2c, sizeof(s->i2c), TYPE_ASPEED_I2C);
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 1e31e22b6f1f..5e3f51b66b43 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -10,8 +10,10 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  #include "hw/sysbus.h"
>  #include "hw/timer/aspeed_timer.h"
> +#include "hw/misc/aspeed_scu.h"
>  #include "qemu-common.h"
>  #include "qemu/bitops.h"
>  #include "qemu/timer.h"
> @@ -26,7 +28,6 @@
>  #define TIMER_CLOCK_USE_EXT true
>  #define TIMER_CLOCK_EXT_HZ 1000000
>  #define TIMER_CLOCK_USE_APB false
> -#define TIMER_CLOCK_APB_HZ 24000000
>
>  #define TIMER_REG_STATUS 0
>  #define TIMER_REG_RELOAD 1
> @@ -80,11 +81,11 @@ static inline bool timer_external_clock(AspeedTimer *t)
>      return timer_ctrl_status(t, op_external_clock);
>  }
>
> -static uint32_t clock_rates[] = { TIMER_CLOCK_APB_HZ, TIMER_CLOCK_EXT_HZ };
> -
>  static inline uint32_t calculate_rate(struct AspeedTimer *t)
>  {
> -    return clock_rates[timer_external_clock(t)];
> +    AspeedTimerCtrlState *s = timer_to_ctrl(t);
> +
> +    return timer_external_clock(t) ? TIMER_CLOCK_EXT_HZ : s->scu->apb_freq;
>  }
>
>  static inline uint32_t calculate_ticks(struct AspeedTimer *t, uint64_t now_ns)
> @@ -449,6 +450,16 @@ static void aspeed_timer_realize(DeviceState *dev, Error **errp)
>      int i;
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      AspeedTimerCtrlState *s = ASPEED_TIMER(dev);
> +    Object *obj;
> +    Error *err = NULL;
> +
> +    obj = object_property_get_link(OBJECT(dev), "scu", &err);
> +    if (!obj) {
> +        error_propagate(errp, err);
> +        error_prepend(errp, "required link 'scu' not found: ");
> +        return;
> +    }
> +    s->scu = ASPEED_SCU(obj);
>
>      for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
>          aspeed_init_one_timer(s, i);
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH 3/3] aspeed/timer: use the APB frequency from the SCU

[Cédric Le Goater]

On 06/22/2018 03:01 AM, Joel Stanley wrote:
> On 22 June 2018 at 08:09, Cédric Le Goater <clg@kaod.org> wrote:
>> The timer controller can be driven by either an external 1MHz clock or
>> by the APB clock. Today, the model makes this assumption that the APB
>> frequency is 24MHz but this is incorrect on the AST2400 SoC. palmetto
>> machines use a 48MHz input clock source.
> 
> So this fixes it to use a apb frequency from the scu for all platform,
> not just the 2400?

Yes. This is true all, platforms are impacted. The AST2500 settings 
are still using the default 24MHz freq emulated by QEMU. So they seem
not impacted but they are. I will update the commit log.

> Can you put something in the commit message about the change in guest
> behaviour - the output of Linux's clk_summary or similar - so we know
> what bug this fixed?

Ah ! the command 'sleep 1' does now effectively last one second and 
not two. Difficult to show in an email :) 

> The code looks good though.
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>

Thanks,

C.
 
>> Use the SCU object to get the APB frequency and drive the timers with
>> the configured freq for the SoC.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/timer/aspeed_timer.h |  4 ++++
>>  hw/arm/aspeed_soc.c             |  2 ++
>>  hw/timer/aspeed_timer.c         | 19 +++++++++++++++----
>>  3 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
>> index bd6c1a7f9609..040a08873432 100644
>> --- a/include/hw/timer/aspeed_timer.h
>> +++ b/include/hw/timer/aspeed_timer.h
>> @@ -24,6 +24,8 @@
>>
>>  #include "qemu/timer.h"
>>
>> +typedef struct AspeedSCUState AspeedSCUState;
>> +
>>  #define ASPEED_TIMER(obj) \
>>      OBJECT_CHECK(AspeedTimerCtrlState, (obj), TYPE_ASPEED_TIMER);
>>  #define TYPE_ASPEED_TIMER "aspeed.timer"
>> @@ -55,6 +57,8 @@ typedef struct AspeedTimerCtrlState {
>>      uint32_t ctrl;
>>      uint32_t ctrl2;
>>      AspeedTimer timers[ASPEED_TIMER_NR_TIMERS];
>> +
>> +    AspeedSCUState *scu;
>>  } AspeedTimerCtrlState;
>>
>>  #endif /* ASPEED_TIMER_H */
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index 7cc05ee27ea4..e68911af0f90 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -127,6 +127,8 @@ static void aspeed_soc_init(Object *obj)
>>
>>      object_initialize(&s->timerctrl, sizeof(s->timerctrl), TYPE_ASPEED_TIMER);
>>      object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), NULL);
>> +    object_property_add_const_link(OBJECT(&s->timerctrl), "scu",
>> +                                   OBJECT(&s->scu), &error_abort);
>>      qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default());
>>
>>      object_initialize(&s->i2c, sizeof(s->i2c), TYPE_ASPEED_I2C);
>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>> index 1e31e22b6f1f..5e3f51b66b43 100644
>> --- a/hw/timer/aspeed_timer.c
>> +++ b/hw/timer/aspeed_timer.c
>> @@ -10,8 +10,10 @@
>>   */
>>
>>  #include "qemu/osdep.h"
>> +#include "qapi/error.h"
>>  #include "hw/sysbus.h"
>>  #include "hw/timer/aspeed_timer.h"
>> +#include "hw/misc/aspeed_scu.h"
>>  #include "qemu-common.h"
>>  #include "qemu/bitops.h"
>>  #include "qemu/timer.h"
>> @@ -26,7 +28,6 @@
>>  #define TIMER_CLOCK_USE_EXT true
>>  #define TIMER_CLOCK_EXT_HZ 1000000
>>  #define TIMER_CLOCK_USE_APB false
>> -#define TIMER_CLOCK_APB_HZ 24000000
>>
>>  #define TIMER_REG_STATUS 0
>>  #define TIMER_REG_RELOAD 1
>> @@ -80,11 +81,11 @@ static inline bool timer_external_clock(AspeedTimer *t)
>>      return timer_ctrl_status(t, op_external_clock);
>>  }
>>
>> -static uint32_t clock_rates[] = { TIMER_CLOCK_APB_HZ, TIMER_CLOCK_EXT_HZ };
>> -
>>  static inline uint32_t calculate_rate(struct AspeedTimer *t)
>>  {
>> -    return clock_rates[timer_external_clock(t)];
>> +    AspeedTimerCtrlState *s = timer_to_ctrl(t);
>> +
>> +    return timer_external_clock(t) ? TIMER_CLOCK_EXT_HZ : s->scu->apb_freq;
>>  }
>>
>>  static inline uint32_t calculate_ticks(struct AspeedTimer *t, uint64_t now_ns)
>> @@ -449,6 +450,16 @@ static void aspeed_timer_realize(DeviceState *dev, Error **errp)
>>      int i;
>>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>      AspeedTimerCtrlState *s = ASPEED_TIMER(dev);
>> +    Object *obj;
>> +    Error *err = NULL;
>> +
>> +    obj = object_property_get_link(OBJECT(dev), "scu", &err);
>> +    if (!obj) {
>> +        error_propagate(errp, err);
>> +        error_prepend(errp, "required link 'scu' not found: ");
>> +        return;
>> +    }
>> +    s->scu = ASPEED_SCU(obj);
>>
>>      for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
>>          aspeed_init_one_timer(s, i);
>> --
>> 2.13.6
>>