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

[Joel Stanley]

On 22 June 2018 at 08:09, Cédric Le Goater <address@hidden> 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 <address@hidden>
> ---
>  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;
> +}