[PATCH v4 0/6] Fix hw-strap for AST2700

[PATCH v4 0/6] Fix hw-strap for AST2700

[Jamin Lin via]

v1: This patch series is from https://patchwork.kernel.org/project/qemu-devel/cover/20250213033531.3367697-1-jamin_lin@aspeedtech.com/.
   To expedite the review process, I have separated the SCU fix patches
   
a. Fix the hw-strap and revision ID for SCU and SCUIO
b. ix boot issue for AST2700

Jamin Lin (6):
  hw/misc/aspeed_scu: Skipping dram_init in u-boot
  hw/misc/aspeed_scu: Fix the revision ID cannot be set in the SOC layer
    for AST2700
  hw/arm/aspeed Update HW Strap Default Values for AST2700
  hw/misc/aspeed_scu: Fix the hw-strap1 cannot be set in the SOC layer
    for AST2700
  hw/arm/aspeed_ast27x0.c Separate HW Strap Registers for SCU and SCUIO
  hw/arm/aspeed_ast27x0.c Fix boot issue for AST2700

 hw/arm/aspeed.c         |  6 ++++--
 hw/arm/aspeed_ast27x0.c | 13 ++++++++++---
 hw/misc/aspeed_scu.c    |  8 ++++----
 3 files changed, 18 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH v4 1/6] hw/misc/aspeed_scu: Skipping dram_init in u-boot

[Jamin Lin via]

Setting BIT6 in VGA0 SCRATCH register will indicate that the ddr traning
is done, therefore skipping the u-boot-spl dram_init() process.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
---
 hw/misc/aspeed_scu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index bac1441b06..50f74fbabd 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -157,6 +157,7 @@
 #define AST2700_SCU_FREQ_CNTR       TO_REG(0x3b0)
 #define AST2700_SCU_CPU_SCRATCH_0   TO_REG(0x780)
 #define AST2700_SCU_CPU_SCRATCH_1   TO_REG(0x784)
+#define AST2700_SCU_VGA_SCRATCH_0   TO_REG(0x900)
 
 #define AST2700_SCUIO_CLK_STOP_CTL_1    TO_REG(0x240)
 #define AST2700_SCUIO_CLK_STOP_CLR_1    TO_REG(0x244)
@@ -930,6 +931,7 @@ static const uint32_t ast2700_a0_resets[ASPEED_AST2700_SCU_NR_REGS] = {
     [AST2700_SCU_FREQ_CNTR]         = 0x000375eb,
     [AST2700_SCU_CPU_SCRATCH_0]     = 0x00000000,
     [AST2700_SCU_CPU_SCRATCH_1]     = 0x00000004,
+    [AST2700_SCU_VGA_SCRATCH_0]     = 0x00000040,
 };
 
 static void aspeed_ast2700_scu_reset(DeviceState *dev)
-- 
2.34.1

Re: [PATCH v4 1/6] hw/misc/aspeed_scu: Skipping dram_init in u-boot

[Cédric Le Goater]

On 3/3/25 08:35, Jamin Lin wrote:
> Setting BIT6 in VGA0 SCRATCH register will indicate that the ddr traning
> is done, therefore skipping the u-boot-spl dram_init() process.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/misc/aspeed_scu.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index bac1441b06..50f74fbabd 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -157,6 +157,7 @@
>   #define AST2700_SCU_FREQ_CNTR       TO_REG(0x3b0)
>   #define AST2700_SCU_CPU_SCRATCH_0   TO_REG(0x780)
>   #define AST2700_SCU_CPU_SCRATCH_1   TO_REG(0x784)
> +#define AST2700_SCU_VGA_SCRATCH_0   TO_REG(0x900)
>   
>   #define AST2700_SCUIO_CLK_STOP_CTL_1    TO_REG(0x240)
>   #define AST2700_SCUIO_CLK_STOP_CLR_1    TO_REG(0x244)
> @@ -930,6 +931,7 @@ static const uint32_t ast2700_a0_resets[ASPEED_AST2700_SCU_NR_REGS] = {
>       [AST2700_SCU_FREQ_CNTR]         = 0x000375eb,
>       [AST2700_SCU_CPU_SCRATCH_0]     = 0x00000000,
>       [AST2700_SCU_CPU_SCRATCH_1]     = 0x00000004,
> +    [AST2700_SCU_VGA_SCRATCH_0]     = 0x00000040,
>   };
>   
>   static void aspeed_ast2700_scu_reset(DeviceState *dev)

[PATCH v4 2/6] hw/misc/aspeed_scu: Fix the revision ID cannot be set in the SOC layer for AST2700

[Jamin Lin via]

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(-)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 50f74fbabd..545d004749 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -910,7 +910,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,
-- 
2.34.1

Re: [PATCH v4 2/6] hw/misc/aspeed_scu: Fix the revision ID cannot be set in the SOC layer for AST2700

[Cédric Le Goater]

On 3/3/25 08: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

Why are you mentioning bitfield [23:16] of both registers ? I thought
the silicon revision ID was 64-bit value.

> 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(-)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 50f74fbabd..545d004749 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -910,7 +910,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,

RE: [PATCH v4 2/6] hw/misc/aspeed_scu: Fix the revision ID cannot be set in the SOC layer for AST2700

[Jamin Lin]

Hi Cedric,

> Subject: Re: [PATCH v4 2/6] hw/misc/aspeed_scu: Fix the revision ID cannot be
> set in the SOC layer for AST2700
> 
> On 3/3/25 08: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
> 
> Why are you mentioning bitfield [23:16] of both registers ? I thought the silicon
> revision ID was 64-bit value.
> 
Yes, the silicon revision ID was 64-bit value. I will update commit log as following. What do you think?

In the AST2700, the firmware reads the SCU Silicon Revision ID register
(SCU0_000) and the SCUIO Silicon Revision ID register (SCU1_000),
combining them into a single 64-bit value.

The upper 32 bits represent the SCUIO, while the lower 32 bits correspond to the
SCU. For example, the AST2700-A1 revision is represented as 0x0601010306010103.
SCUIO_000 occupies bits [63:32] with a value of 0x06010103 and SCU_000 occupies
bits [31:0] with a value of 0x06010103.

Thanks-Jamin

> > 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/a
> > rch/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
> > 50f74fbabd..545d004749 100644
> > --- a/hw/misc/aspeed_scu.c
> > +++ b/hw/misc/aspeed_scu.c
> > @@ -910,7 +910,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,

Re: [PATCH v4 2/6] hw/misc/aspeed_scu: Fix the revision ID cannot be set in the SOC layer for AST2700

[Cédric Le Goater]

On 3/4/25 03:11, Jamin Lin wrote:
> Hi Cedric,
> 
>> Subject: Re: [PATCH v4 2/6] hw/misc/aspeed_scu: Fix the revision ID cannot be
>> set in the SOC layer for AST2700
>>
>> On 3/3/25 08: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
>>
>> Why are you mentioning bitfield [23:16] of both registers ? I thought the silicon
>> revision ID was 64-bit value.
>>
> Yes, the silicon revision ID was 64-bit value. I will update commit log as following. What do you think?

Looks good to me.


Thanks,

C.


> 
> In the AST2700, the firmware reads the SCU Silicon Revision ID register
> (SCU0_000) and the SCUIO Silicon Revision ID register (SCU1_000),
> combining them into a single 64-bit value.
> 
> The upper 32 bits represent the SCUIO, while the lower 32 bits correspond to the
> SCU. For example, the AST2700-A1 revision is represented as 0x0601010306010103.
> SCUIO_000 occupies bits [63:32] with a value of 0x06010103 and SCU_000 occupies
> bits [31:0] with a value of 0x06010103.
> 
> Thanks-Jamin
> 
>>> 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/a
>>> rch/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
>>> 50f74fbabd..545d004749 100644
>>> --- a/hw/misc/aspeed_scu.c
>>> +++ b/hw/misc/aspeed_scu.c
>>> @@ -910,7 +910,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,
> 

[PATCH v4 3/6] hw/arm/aspeed Update HW Strap Default Values for AST2700

[Jamin Lin via]

Separate HW Strap Registers for SCU and SCUIO.
AST2700_EVB_HW_STRAP1 is used for the SCU (CPU Die) hw-strap1.
AST2700_EVB_HW_STRAP2 is used for the SCUIO (IO Die) hw-strap1.

Additionally, both default values are updated based on the dump from the EVB.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 98bf071139..c6c18596d6 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -181,8 +181,10 @@ struct AspeedMachineState {
 
 #ifdef TARGET_AARCH64
 /* AST2700 evb hardware value */
-#define AST2700_EVB_HW_STRAP1 0x000000C0
-#define AST2700_EVB_HW_STRAP2 0x00000003
+/* SCU HW Strap1 */
+#define AST2700_EVB_HW_STRAP1 0x00000800
+/* SCUIO HW Strap1 */
+#define AST2700_EVB_HW_STRAP2 0x00000700
 #endif
 
 /* Rainier hardware value: (QEMU prototype) */
-- 
2.34.1

Re: [PATCH v4 3/6] hw/arm/aspeed Update HW Strap Default Values for AST2700

[Cédric Le Goater]

On 3/3/25 08:35, Jamin Lin wrote:
> Separate HW Strap Registers for SCU and SCUIO.
> AST2700_EVB_HW_STRAP1 is used for the SCU (CPU Die) hw-strap1.
> AST2700_EVB_HW_STRAP2 is used for the SCUIO (IO Die) hw-strap1.
> 
> Additionally, both default values are updated based on the dump from the EVB.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/arm/aspeed.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 98bf071139..c6c18596d6 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -181,8 +181,10 @@ struct AspeedMachineState {
>   
>   #ifdef TARGET_AARCH64
>   /* AST2700 evb hardware value */
> -#define AST2700_EVB_HW_STRAP1 0x000000C0
> -#define AST2700_EVB_HW_STRAP2 0x00000003
> +/* SCU HW Strap1 */
> +#define AST2700_EVB_HW_STRAP1 0x00000800
> +/* SCUIO HW Strap1 */
> +#define AST2700_EVB_HW_STRAP2 0x00000700
>   #endif
>   
>   /* Rainier hardware value: (QEMU prototype) */

[PATCH v4 4/6] hw/misc/aspeed_scu: Fix the hw-strap1 cannot be set in the SOC layer for AST2700

[Jamin Lin via]

There is one hw_strap1 register in the SCU (CPU DIE) and another hw_strap1
register in the SCUIO (IO DIE).

In the "ast2700_a0_resets" function, the hardcoded value "0x00000800" is set in
SCU hw-strap1 (CPU DIE), and in "ast2700_a0_resets_io" the hardcoded value
"0x00000504" is set in SCUIO hw-strap1 (IO DIE). Both values cannot be set via
the SOC layer.

The value of "s->hw_strap1" is set by the SOC layer via the "hw-strap1" property.
Update the "aspeed_ast2700_scu_reset" function to set the value of "s->hw_strap1"
in both the SCU and SCUIO hw-strap1 registers.

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 545d004749..0581c744f1 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -910,7 +910,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,
@@ -940,6 +939,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)
@@ -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_HW_STRAP1]                 = 0x00000504,
     [AST2700_HW_STRAP1_CLR]             = 0xFFF0FFF0,
     [AST2700_HW_STRAP1_LOCK]            = 0x00000FFF,
     [AST2700_HW_STRAP1_SEC1]            = 0x000000FF,
-- 
2.34.1

Re: [PATCH v4 4/6] hw/misc/aspeed_scu: Fix the hw-strap1 cannot be set in the SOC layer for AST2700

[Cédric Le Goater]

On 3/3/25 08:35, Jamin Lin wrote:
> There is one hw_strap1 register in the SCU (CPU DIE) and another hw_strap1
> register in the SCUIO (IO DIE).
> 
> In the "ast2700_a0_resets" function, the hardcoded value "0x00000800" is set in
> SCU hw-strap1 (CPU DIE), and in "ast2700_a0_resets_io" the hardcoded value
> "0x00000504" is set in SCUIO hw-strap1 (IO DIE). Both values cannot be set via
> the SOC layer.
> 
> The value of "s->hw_strap1" is set by the SOC layer via the "hw-strap1" property.
> Update the "aspeed_ast2700_scu_reset" function to set the value of "s->hw_strap1"
> in both the SCU and SCUIO hw-strap1 registers.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   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 545d004749..0581c744f1 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -910,7 +910,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,
> @@ -940,6 +939,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)
> @@ -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_HW_STRAP1]                 = 0x00000504,
>       [AST2700_HW_STRAP1_CLR]             = 0xFFF0FFF0,
>       [AST2700_HW_STRAP1_LOCK]            = 0x00000FFF,
>       [AST2700_HW_STRAP1_SEC1]            = 0x000000FF,

[PATCH v4 5/6] hw/arm/aspeed_ast27x0.c Separate HW Strap Registers for SCU and SCUIO

[Jamin Lin via]

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 registers should not be
the same. To reuse the current design of hw-strap, hw-strap1 is assigned to the
SCU and sets the value in the SCU hw-strap1 register, while hw-strap2 is
assigned to the SCUIO and sets the value in the SCUIO hw-strap1 register.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index a48f47b74e..8c225d4f90 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -331,16 +331,23 @@ static void aspeed_soc_ast2700_init(Object *obj)
     object_initialize_child(obj, "scu", &s->scu, TYPE_ASPEED_2700_SCU);
     qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev",
                          sc->silicon_rev);
+    /*
+     * There is one hw-strap1 register in the SCU (CPU DIE) and another
+     * hw-strap1 register in the SCUIO (IO DIE). To reuse the current design
+     * of hw-strap, hw-strap1 is assigned to the SCU and sets the value in the
+     * SCU hw-strap1 register, while hw-strap2 is assigned to the SCUIO and
+     * sets the value in the SCUIO hw-strap1 register.
+     */
     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-strap1");
 
     snprintf(typename, sizeof(typename), "aspeed.fmc-%s", socname);
     object_initialize_child(obj, "fmc", &s->fmc, typename);
-- 
2.34.1

Re: [PATCH v4 5/6] hw/arm/aspeed_ast27x0.c Separate HW Strap Registers for SCU and SCUIO

[Cédric Le Goater]

On 3/3/25 08:35, Jamin Lin wrote:
> 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 registers should not be
> the same. To reuse the current design of hw-strap, hw-strap1 is assigned to the
> SCU and sets the value in the SCU hw-strap1 register, while hw-strap2 is
> assigned to the SCUIO and sets the value in the SCUIO hw-strap1 register.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/arm/aspeed_ast27x0.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index a48f47b74e..8c225d4f90 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -331,16 +331,23 @@ static void aspeed_soc_ast2700_init(Object *obj)
>       object_initialize_child(obj, "scu", &s->scu, TYPE_ASPEED_2700_SCU);
>       qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev",
>                            sc->silicon_rev);
> +    /*
> +     * There is one hw-strap1 register in the SCU (CPU DIE) and another
> +     * hw-strap1 register in the SCUIO (IO DIE). To reuse the current design
> +     * of hw-strap, hw-strap1 is assigned to the SCU and sets the value in the
> +     * SCU hw-strap1 register, while hw-strap2 is assigned to the SCUIO and
> +     * sets the value in the SCUIO hw-strap1 register.
> +     */

please move the  comment before

     object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scuio),
                               "hw-strap1");


Thanks,

C.


>       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-strap1");
>   
>       snprintf(typename, sizeof(typename), "aspeed.fmc-%s", socname);
>       object_initialize_child(obj, "fmc", &s->fmc, typename);

RE: [PATCH v4 5/6] hw/arm/aspeed_ast27x0.c Separate HW Strap Registers for SCU and SCUIO

[Jamin Lin]

Hi Cedric,

> Subject: Re: [PATCH v4 5/6] hw/arm/aspeed_ast27x0.c Separate HW Strap
> Registers for SCU and SCUIO
> 
> On 3/3/25 08:35, Jamin Lin wrote:
> > 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
> > registers should not be the same. To reuse the current design of
> > hw-strap, hw-strap1 is assigned to the SCU and sets the value in the
> > SCU hw-strap1 register, while hw-strap2 is assigned to the SCUIO and sets the
> value in the SCUIO hw-strap1 register.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/arm/aspeed_ast27x0.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> > a48f47b74e..8c225d4f90 100644
> > --- a/hw/arm/aspeed_ast27x0.c
> > +++ b/hw/arm/aspeed_ast27x0.c
> > @@ -331,16 +331,23 @@ static void aspeed_soc_ast2700_init(Object *obj)
> >       object_initialize_child(obj, "scu", &s->scu, TYPE_ASPEED_2700_SCU);
> >       qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev",
> >                            sc->silicon_rev);
> > +    /*
> > +     * There is one hw-strap1 register in the SCU (CPU DIE) and another
> > +     * hw-strap1 register in the SCUIO (IO DIE). To reuse the current
> design
> > +     * of hw-strap, hw-strap1 is assigned to the SCU and sets the value in
> the
> > +     * SCU hw-strap1 register, while hw-strap2 is assigned to the SCUIO
> and
> > +     * sets the value in the SCUIO hw-strap1 register.
> > +     */
> 
> please move the  comment before
> 
>      object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scuio),
>                                "hw-strap1");
> 
Thanks for your suggestion and review.
Will update it.
Jamin
> 
> Thanks,
> 
> C.
> 
> 
> >       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-strap1");
> >
> >       snprintf(typename, sizeof(typename), "aspeed.fmc-%s", socname);
> >       object_initialize_child(obj, "fmc", &s->fmc, typename);

[PATCH v4 6/6] hw/arm/aspeed_ast27x0.c Fix boot issue for AST2700

[Jamin Lin via]

Currently, ASPEED_DEV_SPI_BOOT is set to "0x400000000", which is the DRAM start
address, and the QEMU loader is used to load the U-Boot binary into this address.

However, if users want to install FMC flash contents as a boot ROM, the DRAM
address 0x400000000 would be overwritten with Boot ROM data. This causes the
AST2700 to fail to boot because the U-Boot data becomes incorrect.

To fix this, change the ASPEED_DEV_SPI_BOOT address to "0x100000000", which is
the FMC0 memory-mapped start address in the AST2700.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 8c225d4f90..527a5f8245 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -24,7 +24,7 @@
 #include "qemu/log.h"
 
 static const hwaddr aspeed_soc_ast2700_memmap[] = {
-    [ASPEED_DEV_SPI_BOOT]  =  0x400000000,
+    [ASPEED_DEV_SPI_BOOT]  =  0x100000000,
     [ASPEED_DEV_SRAM]      =  0x10000000,
     [ASPEED_DEV_SDMC]      =  0x12C00000,
     [ASPEED_DEV_SCU]       =  0x12C02000,
-- 
2.34.1

Re: [PATCH v4 6/6] hw/arm/aspeed_ast27x0.c Fix boot issue for AST2700

[Cédric Le Goater]

On 3/3/25 08:35, Jamin Lin wrote:
> Currently, ASPEED_DEV_SPI_BOOT is set to "0x400000000", which is the DRAM start
> address, and the QEMU loader is used to load the U-Boot binary into this address.
> 
> However, if users want to install FMC flash contents as a boot ROM, the DRAM
> address 0x400000000 would be overwritten with Boot ROM data. This causes the
> AST2700 to fail to boot because the U-Boot data becomes incorrect.
> 
> To fix this, change the ASPEED_DEV_SPI_BOOT address to "0x100000000", which is
> the FMC0 memory-mapped start address in the AST2700.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/arm/aspeed_ast27x0.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index 8c225d4f90..527a5f8245 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -24,7 +24,7 @@
>   #include "qemu/log.h"
>   
>   static const hwaddr aspeed_soc_ast2700_memmap[] = {
> -    [ASPEED_DEV_SPI_BOOT]  =  0x400000000,
> +    [ASPEED_DEV_SPI_BOOT]  =  0x100000000,
>       [ASPEED_DEV_SRAM]      =  0x10000000,
>       [ASPEED_DEV_SDMC]      =  0x12C00000,
>       [ASPEED_DEV_SCU]       =  0x12C02000,