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

["Cédric Le Goater" <clg@kaod.org>]

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;
>> +}