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

[Cédric Le Goater]

Hello Steven,

[ ... ]
> 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;

yes. that's the idea.

[ ... ]

> > >    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.

yes. 'clkin_25Mhz' may be.

Thanks,

C.