Re: [PATCH v1 1/1] hw: aspeed_scu: Add AST2600 hpll calculation function

[Steven Lee <steven_lee@aspeedtech.com>]

The 03/14/2022 20:21, Cédric Le Goater wrote:
> Hello Steven,
> 
> On 3/14/22 10:54, Steven Lee wrote:
> > AST2600's HPLL register offset and bit definition are different from
> > AST2500. Add a hpll calculation function for ast2600 and modify apb frequency
> > calculation function based on SCU200 register description in ast2600v11.pdf.
> 
> It looks good. A few minor comments on the modeling.
>   
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > ---
> >   hw/misc/aspeed_scu.c         | 43 ++++++++++++++++++++++++++++++++----
> >   include/hw/misc/aspeed_scu.h | 17 ++++++++++++++
> >   2 files changed, 56 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> > index d06e179a6e..3b11e98d66 100644
> > --- a/hw/misc/aspeed_scu.c
> > +++ b/hw/misc/aspeed_scu.c
> > @@ -205,6 +205,8 @@ static const uint32_t ast2500_a1_resets[ASPEED_SCU_NR_REGS] = {
> >        [BMC_DEV_ID]      = 0x00002402U
> >   };
> >   
> > +static uint32_t aspeed_2600_scu_calc_hpll(AspeedSCUState *s, uint32_t hpll_reg);
> > +
> >   static uint32_t aspeed_scu_get_random(void)
> >   {
> >       uint32_t num;
> > @@ -215,9 +217,19 @@ static uint32_t aspeed_scu_get_random(void)
> >   uint32_t aspeed_scu_get_apb_freq(AspeedSCUState *s)
> >   {
> >       AspeedSCUClass *asc = ASPEED_SCU_GET_CLASS(s);
> > -    uint32_t hpll = asc->calc_hpll(s, s->regs[HPLL_PARAM]);
> > +    uint32_t hpll, hpll_reg, clk_sel_reg;
> > +
> > +    if (asc->calc_hpll == aspeed_2600_scu_calc_hpll) {
> 
> That's indeed one way to distinguish the AST2600 from the previous SoCs.
> I would prefer to introduce a new APB freq class handler to deal with
> the differences in the AST2600. aspeed_scu_get_apb_freq() would become :
> 
> 	uint32_t aspeed_scu_get_apb_freq(AspeedSCUState *s)
> 	{
> 	    return ASPEED_SCU_GET_CLASS(s)->get_apb(s);
> 	}
> 
> The current aspeed_scu_get_apb_freq() would become the AST2400 and AST2500
> handler and you would have to introduce a new one for the AST2600.
> 

Hi Cédric,

Thanks for the review.
I was wondering if the following implementation is good to you.

1 Modify aspeed_scu_get_apb_freq() as below
 	uint32_t aspeed_scu_get_apb_freq(AspeedSCUState *s)
 	{
 	    return ASPEED_SCU_GET_CLASS(s)->get_apb(s);
 	}

2. Introduce 2 APB class handlers: aspeed_2400_scu_get_apb_freq() and aspeed_2600_scu_get_apb_freq()

3. Add new attribute get_apb in AspeedSCUClass.

4. In aspeed_2400_scu_class_init() and aspeed_2500_scu_class_init()
	asc->get_apb = aspeed_2400_scu_get_apb_freq;

   In aspeed_2600_scu_class_init()
	asc->get_apb = aspeed_2600_scu_get_apb_freq;

> > +        hpll_reg = s->regs[AST2600_HPLL_PARAM];
> > +        clk_sel_reg = s->regs[AST2600_CLK_SEL];
> > +    } else {
> > +        hpll_reg = s->regs[HPLL_PARAM];
> > +        clk_sel_reg = s->regs[CLK_SEL];
> > +    }
> > +
> > +    hpll = asc->calc_hpll(s, hpll_reg);
> >   
> > -    return hpll / (SCU_CLK_GET_PCLK_DIV(s->regs[CLK_SEL]) + 1)
> > +    return hpll / (SCU_CLK_GET_PCLK_DIV(clk_sel_reg) + 1)
> >           / asc->apb_divider;>   }
> >   
> > @@ -357,7 +369,10 @@ static const MemoryRegionOps aspeed_ast2500_scu_ops = {
> >   
> >   static uint32_t aspeed_scu_get_clkin(AspeedSCUState *s)
> >   {
> > -    if (s->hw_strap1 & SCU_HW_STRAP_CLK_25M_IN) {
> > +    AspeedSCUClass *asc = ASPEED_SCU_GET_CLASS(s);
> > +
> > +    if (s->hw_strap1 & SCU_HW_STRAP_CLK_25M_IN ||
> > +        asc->calc_hpll == aspeed_2600_scu_calc_hpll) {
> 
> Indeed, the AST2600 CLKIN is always 25Mhz. Instead of testing ->calc_hpll,
> I would introduce a class attribute, something like 'bool is_25Mhz'.
> 
> This change should be in a second patch though.
> 

will add a new attribute for clkin in the second patch.

Thanks,
Steven

> Thanks,
> 
> C.
> 
> >           return 25000000;
> >       } else if (s->hw_strap1 & SCU_HW_STRAP_CLK_48M_IN) {
> >           return 48000000;
> > @@ -426,6 +441,26 @@ static uint32_t aspeed_2500_scu_calc_hpll(AspeedSCUState *s, uint32_t hpll_reg)
> >       return clkin * multiplier;
> >   }
> >   
> > +static uint32_t aspeed_2600_scu_calc_hpll(AspeedSCUState *s, uint32_t hpll_reg)
> > +{
> > +    uint32_t multiplier = 1;
> > +    uint32_t clkin = aspeed_scu_get_clkin(s);
> > +
> > +    if (hpll_reg & SCU_AST2600_H_PLL_OFF) {
> > +        return 0;
> > +    }
> > +
> > +    if (!(hpll_reg & SCU_H_PLL_BYPASS_EN)) {
> > +        uint32_t p = (hpll_reg >> 19) & 0xf;
> > +        uint32_t n = (hpll_reg >> 13) & 0x3f;
> > +        uint32_t m = hpll_reg & 0x1fff;
> > +
> > +        multiplier = ((m + 1) / (n + 1)) / (p + 1);
> > +    }
> > +
> > +    return clkin * multiplier;
> > +}
> > +
> >   static void aspeed_scu_reset(DeviceState *dev)
> >   {
> >       AspeedSCUState *s = ASPEED_SCU(dev);
> > @@ -716,7 +751,7 @@ static void aspeed_2600_scu_class_init(ObjectClass *klass, void *data)
> >       dc->desc = "ASPEED 2600 System Control Unit";
> >       dc->reset = aspeed_ast2600_scu_reset;
> >       asc->resets = ast2600_a3_resets;
> > -    asc->calc_hpll = aspeed_2500_scu_calc_hpll; /* No change since AST2500 */
> > +    asc->calc_hpll = aspeed_2600_scu_calc_hpll;
> >       asc->apb_divider = 4;
> >       asc->nr_regs = ASPEED_AST2600_SCU_NR_REGS;
> >       asc->ops = &aspeed_ast2600_scu_ops;
> > diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h
> > index c14aff2bcb..91c500c5bc 100644
> > --- a/include/hw/misc/aspeed_scu.h
> > +++ b/include/hw/misc/aspeed_scu.h
> > @@ -316,4 +316,21 @@ uint32_t aspeed_scu_get_apb_freq(AspeedSCUState *s);
> >           SCU_HW_STRAP_VGA_SIZE_SET(VGA_16M_DRAM) |                       \
> >           SCU_AST2500_HW_STRAP_RESERVED1)
> >   
> > +/* SCU200   H-PLL Parameter Register (for Aspeed AST2600 SOC)
> > + *
> > + *  28:26  H-PLL Parameters
> > + *  25     Enable H-PLL reset
> > + *  24     Enable H-PLL bypass mode
> > + *  23     Turn off H-PLL
> > + *  22:19  H-PLL Post Divider (P)
> > + *  18:13   H-PLL Numerator (M)
> > + *  12:0    H-PLL Denumerator (N)
> > + *
> > + *  (Output frequency) = CLKIN(25MHz) * [(M+1) / (N+1)] / (P+1)
> > + *
> > + * The default frequency is 1200Mhz when CLKIN = 25MHz
> > + */
> > +#define SCU_AST2600_H_PLL_BYPASS_EN                        (0x1 << 24)
> > +#define SCU_AST2600_H_PLL_OFF                              (0x1 << 23)
> > +
> >   #endif /* ASPEED_SCU_H */
>