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?
Additionally, would it be possible to submit a separate patch for the SCU
silicon_rev and SCU hw_strap fix?
The patch series for supporting AST2700 A1 is quite large.
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");
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;
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,
