| Message ID | 20220314095449.22089-2-steven_lee@aspeedtech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw: aspeed_scu: Add AST2600 hpll calculation function | expand |
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 */ >
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.
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) { + 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) { 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 */
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. 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(-)