Re: [PATCH v1 11/18] hw/misc/aspeed_scu: Add Support for AST2700/AST2750 A1 Silicon Revisions

[Andrew Jeffery <andrew@codeconstruct.com.au>]

On Tue, 2025-01-21 at 15:04 +0800, Jamin Lin wrote:
> Added new definitions for AST2700_A1_SILICON_REV and
> AST2750_A1_SILICON_REV to
> identify the A1 silicon revisions.
> 
> Update "aspeed_ast2700_scu_reset" to set the silicon_rev field in the
> SCU
> registers.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>  hw/misc/aspeed_scu.c         | 3 +++
>  include/hw/misc/aspeed_scu.h | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index bac1441b06..f049a9fd96 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -559,6 +559,8 @@ static uint32_t aspeed_silicon_revs[] = {
>      AST2700_A0_SILICON_REV,
>      AST2720_A0_SILICON_REV,
>      AST2750_A0_SILICON_REV,
> +    AST2700_A1_SILICON_REV,
> +    AST2750_A1_SILICON_REV,
>  };
>  
>  bool is_supported_silicon_rev(uint32_t silicon_rev)
> @@ -938,6 +940,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;

Has s->silicon_rev been set?

Should we now remove the AST2700_SILICON_REV entry from asc->resets?

This seems like a fix regardless. Perhaps separate it from the addition
of the new silicon IDs?

>  }
>  
>  static void aspeed_2700_scu_class_init(ObjectClass *klass, void
> *data)
> diff --git a/include/hw/misc/aspeed_scu.h
> b/include/hw/misc/aspeed_scu.h
> index 356be95e45..684b48b722 100644
> --- a/include/hw/misc/aspeed_scu.h
> +++ b/include/hw/misc/aspeed_scu.h
> @@ -54,6 +54,8 @@ struct AspeedSCUState {
>  #define AST2700_A0_SILICON_REV   0x06000103U
>  #define AST2720_A0_SILICON_REV   0x06000203U
>  #define AST2750_A0_SILICON_REV   0x06000003U
> +#define AST2700_A1_SILICON_REV   0x06010103U
> +#define AST2750_A1_SILICON_REV   0x06010003U

These look fine.

Andrew

>  
>  #define ASPEED_IS_AST2500(si_rev)     ((((si_rev) >> 24) & 0xff) ==
> 0x04)
>