Hi Cedric,
> Subject: Re: [PATCH v3 15/28] hw/misc/aspeed_scu: Fix the revision ID cannot
> be set in the SOC layer for AST2700
>
> Hello Jamin,
>
> On 2/26/25 07:38, Jamin Lin wrote:
> > Hi Cedric,
> >
> >>
> >> On 2/13/25 04:35, Jamin Lin wrote:
> >>> According to the design of the AST2600, it has a Silicon Revision ID
> >>> Register, specifically SCU004 and SCU014, to set the Revision ID for
> >>> the
> >> AST2600.
> >>> For the AST2600 A3, SCU004 is set to 0x05030303 and SCU014 is set to
> >> 0x05030303.
> >>> In the "aspeed_ast2600_scu_reset" function, the hardcoded value
> >>> "AST2600_A3_SILICON_REV" is set in SCU004, and "s->silicon_rev" is
> >>> set in SCU014. The value of "s->silicon_rev" is set by the SOC layer
> >>> via the "silicon-rev" property.
> >>>
> >>> However, the design of the AST2700 is different. There are two SCU
> >> controllers:
> >>> SCU0 (CPU Die) and SCU1 (IO Die). In the AST2700, the firmware reads
> >>> the SCU Silicon Revision ID register (SCU0_000) and the SCUIO
> >>> Silicon Revision ID register (SCU1_000) and combines them into a 64-bit
> value.
> >>> The combined value of SCU0_000[23:16] and SCU1_000[23:16] represents
> >>> the silicon revision. For example, the AST2700-A1 revision is
> >>> "0x0601010306010103", where
> >>> SCU0_000 should be 06010103 and SCU1_000 should be 06010103.
> >>
> >> Are both these values supposed to be identical ? if not, we should
> >> plan for changes at machine/SoC level too.
> >>
> >
> > Currently, these values are supposed to be identical. Therefore, we
> > can reuse the current design of the silicon_rev in the machine/SoC layer for
> AST2700.
> >
> >>> static const uint32_t
> >>> ast2700_a0_resets[ASPEED_AST2700_SCU_NR_REGS]
> >> = {
> >>> - [AST2700_SILICON_REV] = AST2700_A0_SILICON_REV,
> >>> [AST2700_HW_STRAP1] = 0x00000800,
> >>> [AST2700_HW_STRAP1_CLR] = 0xFFF0FFF0,
> >>> [AST2700_HW_STRAP1_LOCK] = 0x00000FFF,
> >>> @@ -940,6 +939,7 @@ static void aspeed_ast2700_scu_reset(DeviceState
> >> *dev)
> >>> AspeedSCUClass *asc = ASPEED_SCU_GET_CLASS(dev);
> >>>
> >>> memcpy(s->regs, asc->resets, asc->nr_regs * 4);
> >>> + s->regs[AST2700_SILICON_REV] = s->silicon_rev;
> >>>
> >>> }
> >>>
> >>> static void aspeed_2700_scu_class_init(ObjectClass *klass, void
> >>> *data) @@ -1032,7 +1032,6 @@ static const MemoryRegionOps
> >> aspeed_ast2700_scuio_ops = {
> >>> };
> >>>
> >>> static const uint32_t
> >> ast2700_a0_resets_io[ASPEED_AST2700_SCU_NR_REGS] = {
> >>> - [AST2700_SILICON_REV] = 0x06000003,
> >>> [AST2700_HW_STRAP1] = 0x00000504,
> >>
> >> why isn't AST2700_HW_STRAP1 assigned with s->hw_strap1 property ?
> >>
> >
> > This is a bug. The design of the HW_STRAP register has changed in the
> AST2700.
> > There is one hw_strap1 register in the SCU (CPU DIE) and another hw_strap1
> register in the SCUIO (IO DIE).
> > The values of these two hw_strap1 registers should not be the same.
> >
> > To fix this issue, I made the following changes. Do you have any
> > suggestions?
>
> All Aspeed SoC models currently define "hw-strap1" and "hw-strap2"
> properties as aliases on the same properties of the SCU model :
>
> object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu),
> "hw-strap1");
> object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu),
> "hw-strap2");
>
> For the AST2700 SoC, you could change the "hw-strap2" alias to point to the
> SCUIO model :
>
> object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu),
> "hw-strap1");
> object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scuio),
> "hw-strap1");
>
Will fix it.
> > Additionally, would it be possible to submit a separate patch for the SCU
> silicon_rev and SCU hw_strap fix?
>
> yes we should please.
>
> > The patch series for supporting AST2700 A1 is quite large.
>
> yes. That's why I asked you to split it :)
>
> > Thanks-Jamin
> >
> > 1. I dumped the real values of both registers on the EVB
> >
> > root@ast2700-a0-default:~# md 14c02010 1 ----> SCUIO hw_strap1
> > 14c02010: 00000500
> > root@ast2700-a0-default:~# md 12c02010 1 --> SCU hw_strap1
> > 12c02010: 00000800
> >
> > The value AST2700_EVB_HW_STRAP1 0x00000800 is used for the SCU
> hw_strap1.
> > The value AST2700_EVB_HW_STRAP2 0x00000500 is used for the SCUIO
> > hw_strap1
> >
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -181,8 +181,8 @@ struct AspeedMachineState {
> >
> > #ifdef TARGET_AARCH64
> > /* AST2700 evb hardware value */
> > -#define AST2700_EVB_HW_STRAP1 0x000000C0 -#define
> > AST2700_EVB_HW_STRAP2 0x00000003
> > +#define AST2700_EVB_HW_STRAP1 0x00000800 #define
> > +AST2700_EVB_HW_STRAP2 0x00000500
> > #endif
> >
> > 2. Change to set hw_strap2 in the SCUIO model. Note this will modify the
> hw_strap1 register of the SCUIO.
> >
> > +++ b/hw/arm/aspeed_ast27x0.c
> > @@ -410,14 +410,14 @@ static void aspeed_soc_ast2700_init(Object *obj)
> > sc->silicon_rev);
> > object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu),
> > "hw-strap1");
> > - object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu),
> > - "hw-strap2");
> > object_property_add_alias(obj, "hw-prot-key", OBJECT(&s->scu),
> > "hw-prot-key");
> >
> > object_initialize_child(obj, "scuio", &s->scuio,
> TYPE_ASPEED_2700_SCUIO);
> > qdev_prop_set_uint32(DEVICE(&s->scuio), "silicon-rev",
> > sc->silicon_rev);
> > + object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scuio),
> > + "hw-strap2");
>
> Why use "hw-strap2" and not "hw-strap1" ?
>
> Please add comments in the AST2700 SoC and AST2700 SCU models so that the
> information does not get lost.
>
Will add
> > 3. I introduced a new aspeed_ast2700_scuio_reset function for the
> > SCUIO model and set the value of the hw_strap2 property to the HW_STRAP1
> register of the SCUIO model.
> >
> > s->regs[AST2700_HW_STRAP1] = s->hw_strap2;
>
> That's weird. I would use s->hw_strap1.
>
Will fix
Thanks for suggestion and review.
Jamin
> Thanks,
>
> C.
>
>
> > diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c index
> > 259b142850..23ab7526f5 100644
> > --- a/hw/misc/aspeed_scu.c
> > +++ b/hw/misc/aspeed_scu.c
> > @@ -912,7 +912,6 @@ static const MemoryRegionOps
> aspeed_ast2700_scu_ops = {
> > };
> >
> > static const uint32_t ast2700_a0_resets[ASPEED_AST2700_SCU_NR_REGS]
> = {
> > - [AST2700_HW_STRAP1] = 0x00000800,
> > [AST2700_HW_STRAP1_CLR] = 0xFFF0FFF0,
> > [AST2700_HW_STRAP1_LOCK] = 0x00000FFF,
> > [AST2700_HW_STRAP1_SEC1] = 0x000000FF,
> > @@ -942,6 +941,7 @@ static void aspeed_ast2700_scu_reset(DeviceState
> > *dev)
> >
> > memcpy(s->regs, asc->resets, asc->nr_regs * 4);
> > s->regs[AST2700_SILICON_REV] = s->silicon_rev;
> > + s->regs[AST2700_HW_STRAP1] = s->hw_strap1;
> > }
> >
> > static void aspeed_2700_scu_class_init(ObjectClass *klass, void
> > *data) @@ -1034,7 +1034,6 @@ static const MemoryRegionOps
> aspeed_ast2700_scuio_ops = {
> > };
> >
> > static const uint32_t
> ast2700_a0_resets_io[ASPEED_AST2700_SCU_NR_REGS] = {
> > - [AST2700_HW_STRAP1] = 0x00000504,
> > [AST2700_HW_STRAP1_CLR] = 0xFFF0FFF0,
> > [AST2700_HW_STRAP1_LOCK] = 0x00000FFF,
> > [AST2700_HW_STRAP1_SEC1] = 0x000000FF,
> > @@ -1057,13 +1056,23 @@ static const uint32_t
> ast2700_a0_resets_io[ASPEED_AST2700_SCU_NR_REGS] = {
> > [AST2700_SCUIO_CLK_DUTY_MEAS_RST] = 0x0c9100d2,
> > };
> >
> > +static void aspeed_ast2700_scuio_reset(DeviceState *dev) {
> > + AspeedSCUState *s = ASPEED_SCU(dev);
> > + AspeedSCUClass *asc = ASPEED_SCU_GET_CLASS(dev);
> > +
> > + memcpy(s->regs, asc->resets, asc->nr_regs * 4);
> > + s->regs[AST2700_SILICON_REV] = s->silicon_rev;
> > + s->regs[AST2700_HW_STRAP1] = s->hw_strap2; }
> > +
> > static void aspeed_2700_scuio_class_init(ObjectClass *klass, void *data)
> > {
> > DeviceClass *dc = DEVICE_CLASS(klass);
> > AspeedSCUClass *asc = ASPEED_SCU_CLASS(klass);
> >
> > dc->desc = "ASPEED 2700 System Control Unit I/O";
> > - device_class_set_legacy_reset(dc, aspeed_ast2700_scu_reset);
> > + device_class_set_legacy_reset(dc, aspeed_ast2700_scuio_reset);
> > asc->resets = ast2700_a0_resets_io;
> > asc->calc_hpll = aspeed_2600_scu_calc_hpll;
> > asc->get_apb = aspeed_2700_scuio_get_apb_freq;
> >
> >
> >> The above changes could be merged as a fix.
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >>
> >>> [AST2700_HW_STRAP1_CLR] = 0xFFF0FFF0,
> >>> [AST2700_HW_STRAP1_LOCK] = 0x00000FFF,
> >
