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

[PATCH v5 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

v2:
a. update commit log for "Fix the revision ID cannot be set in the SOC layer for AST2700"
b. update code comments location for "Separate HW Strap Registers for SCU and SCUIO"

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 v5 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>
Reviewed-by: Cédric Le Goater <clg@redhat.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: [v5,1/6] hw/misc/aspeed_scu: Skipping dram_init in u-boot

[Nabih Estefan]

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

Tested-by: Nabih Estefan <nabihestefan@google.com>

Thanks,
Nabih

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

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

[Cédric Le Goater]

Nabih,

On 3/6/25 01:04, Nabih Estefan 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>
> 
> Tested-by: Nabih Estefan <nabihestefan@google.com>
Thanks for the feedback !

Can you tell us a bit more about your tests ?


C.

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

[Nabih Estefan]

Hi Cédric,

We have a custom machine and a custom image using the AST27x0 A0. I ran
some of our internal tests using these patches. They even fixed some of the
errors we’d been seeing recently!

I’m also working on testing through the A1 patches, will reply to those soon.

Thanks,
Nabih

On Thu, Mar 6, 2025 at 12:05 AM Cédric Le Goater <clg@kaod.org> wrote:
>
> Nabih,
>
> On 3/6/25 01:04, Nabih Estefan 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>
> >
> > Tested-by: Nabih Estefan <nabihestefan@google.com>
> Thanks for the feedback !
>
> Can you tell us a bit more about your tests ?
>
>
> C.
>
>

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

[Cédric Le Goater]

Hello,

On 3/6/25 19:05, Nabih Estefan wrote:
> Hi Cédric,
> 
> We have a custom machine and a custom image using the AST27x0 A0. I ran
> some of our internal tests using these patches. They even fixed some of the
> errors we’d been seeing recently!
> 
> I’m also working on testing through the A1 patches, will reply to those soon.

I pushed them on the aspeed-next branch.

	https://github.com/legoater/qemu/commits/aspeed-next/

and should send a PR around the week-end.

Thanks,

C.

[PATCH v5 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), 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.

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 v5 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 07:47, 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), 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.
> 
> 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>


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 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: [v5, 2/6] hw/misc/aspeed_scu: Fix the revision ID cannot be set in the SOC layer for AST2700

[Nabih Estefan 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), 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.
> 
> 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>

Tested-by: Nabih Estefan <nabihestefan@google.com>

Thanks,
Nabih

> ---
>  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 v5 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>
Reviewed-by: Cédric Le Goater <clg@redhat.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: [v5, 3/6] hw/arm/aspeed Update HW Strap Default Values for AST2700

[Nabih Estefan 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>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>

Tested-by: Nabih Estefan <nabihestefan@google.com>

Thanks,
Nabih
> ---
>  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 v5 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>
Reviewed-by: Cédric Le Goater <clg@redhat.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: [v5,4/6] hw/misc/aspeed_scu: Fix the hw-strap1 cannot be set in the SOC layer for AST2700

[Nabih Estefan]

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

Tested-by: Nabih Estefan <nabihestefan@google.com>

Thanks,
Nabih

> ---
>  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 v5 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..92487bf229 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -333,14 +333,21 @@ 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);
+    /*
+     * 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-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 v5 5/6] hw/arm/aspeed_ast27x0.c Separate HW Strap Registers for SCU and SCUIO

[Cédric Le Goater]

On 3/4/25 07:47, 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>


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

Thanks,

C.


> ---
>   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..92487bf229 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -333,14 +333,21 @@ 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);
> +    /*
> +     * 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-strap2", OBJECT(&s->scuio),
> +                                  "hw-strap1");
>   
>       snprintf(typename, sizeof(typename), "aspeed.fmc-%s", socname);
>       object_initialize_child(obj, "fmc", &s->fmc, typename);

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

[Nabih Estefan 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>

Tested-by: Nabih Estefan <nabihestefan@google.com>

Thanks,
Nabih

> ---
>  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..92487bf229 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -333,14 +333,21 @@ 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);
> +    /*
> +     * 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-strap2", OBJECT(&s->scuio),
> +                                  "hw-strap1");
>  
>      snprintf(typename, sizeof(typename), "aspeed.fmc-%s", socname);
>      object_initialize_child(obj, "fmc", &s->fmc, typename);
> 

[PATCH v5 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>
Reviewed-by: Cédric Le Goater <clg@redhat.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 92487bf229..10e1358166 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: [v5,6/6] hw/arm/aspeed_ast27x0.c Fix boot issue for AST2700

[Nabih Estefan]

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

Tested-by: Nabih Estefan <nabihestefan@google.com>

Thanks,
Nabih

> ---
>  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 92487bf229..10e1358166 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,
>

Re: [PATCH v5 0/6] Fix hw-strap for AST2700

[Cédric Le Goater]

On 3/4/25 07:47, Jamin Lin wrote:
> 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
> 
> v2:
> a. update commit log for "Fix the revision ID cannot be set in the SOC layer for AST2700"
> b. update code comments location for "Separate HW Strap Registers for SCU and SCUIO"
> 
> 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(-)
> 


Applied to aspeed-next.

Thanks,

C.