| Message ID | 20250213033531.3367697-16-jamin_lin@aspeedtech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support AST2700 A1 | expand |
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. > > Reference: > https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/arch/arm/mach-aspeed/ast2700/cpu-info.c > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > --- > hw/misc/aspeed_scu.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c > index 2d9fe78926..b45a36a555 100644 > --- a/hw/misc/aspeed_scu.c > +++ b/hw/misc/aspeed_scu.c > @@ -911,7 +911,6 @@ static const MemoryRegionOps aspeed_ast2700_scu_ops = { > }; > > 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 ? The above changes could be merged as a fix. Thanks, C. > [AST2700_HW_STRAP1_CLR] = 0xFFF0FFF0, > [AST2700_HW_STRAP1_LOCK] = 0x00000FFF,
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,
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"); > 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. > 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. 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, >
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, > >
diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c index 2d9fe78926..b45a36a555 100644 --- a/hw/misc/aspeed_scu.c +++ b/hw/misc/aspeed_scu.c @@ -911,7 +911,6 @@ static const MemoryRegionOps aspeed_ast2700_scu_ops = { }; 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, [AST2700_HW_STRAP1_CLR] = 0xFFF0FFF0, [AST2700_HW_STRAP1_LOCK] = 0x00000FFF,
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. Reference: https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/arch/arm/mach-aspeed/ast2700/cpu-info.c Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> --- hw/misc/aspeed_scu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)