[4/6] palmetto-bmc: add board specific configuration

aspeed_init() now uses a board identifier to customize some values
specific to the board, ram base, board revision number, etc.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/palmetto-bmc.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
> aspeed_init() now uses a board identifier to customize some values
> specific to the board, ram base, board revision number, etc.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Looks okay to me, some minor comments below:

> ---
>  hw/arm/palmetto-bmc.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
> index 8a3ff5568575..cd8aa59756b9 100644
> --- a/hw/arm/palmetto-bmc.c
> +++ b/hw/arm/palmetto-bmc.c
> @@ -22,8 +22,6 @@
>  #include "sysemu/blockdev.h"
>  
>  static struct arm_boot_info aspeed_binfo = {
> -    .loader_start = AST2400_SDRAM_BASE,
> -    .board_id = 0,
>      .nb_cpus = 1,
>  };
>  
> @@ -32,6 +30,21 @@ typedef struct AspeedBoardState {
>      MemoryRegion ram;
>  } AspeedBoardState;
>  
> +typedef struct AspeedBoardConfig {
> +    uint32_t hw_strap1;
> +    uint32_t silicon_rev;
> +    hwaddr sdram_base;
> +} AspeedBoardConfig;
> +
> +enum {
> +    PALMETTO_BMC
> +};
> +
> +static const AspeedBoardConfig aspeed_boards[] = {
> +    [ PALMETTO_BMC ] = { 0x120CE416, AST2400_A0_SILICON_REV,
> +                         AST2400_SDRAM_BASE },

I was playing around before and my test scripts noticed checkpatch
complained about the spacing with the array indexing: "[PALMETTO_BMC]"
fixed the error.

> +};
> +
>  static void aspeed_init_flashes(AspeedSMCState *s, const char *flashtype,
>                                  Error **errp)
>  {
> @@ -58,7 +71,7 @@ static void aspeed_init_flashes(AspeedSMCState *s, const char *flashtype,
>      }
>  }
>  
> -static void aspeed_init(MachineState *machine)
> +static void aspeed_init(MachineState *machine, int board_model)

I feel like we should pass a "struct AspeedBoardConfig *" rather than
the "int board_model", cleaning up the repeated indexing into
aspeed_boards the body. Thoughts? </bikeshed>

Andrew

>  {
>      AspeedBoardState *bmc;
>  
> @@ -68,13 +81,16 @@ static void aspeed_init(MachineState *machine)
>                                &error_abort);
>  
>      memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
> -    memory_region_add_subregion(get_system_memory(), AST2400_SDRAM_BASE,
> +    memory_region_add_subregion(get_system_memory(),
> +                                aspeed_boards[board_model].sdram_base,
>                                  &bmc->ram);
>      object_property_add_const_link(OBJECT(&bmc->soc), "ram", OBJECT(&bmc->ram),
>                                     &error_abort);
> -    object_property_set_int(OBJECT(&bmc->soc), 0x120CE416, "hw-strap1",
> -                            &error_abort);
> -    object_property_set_int(OBJECT(&bmc->soc), AST2400_A0_SILICON_REV,
> +    object_property_set_int(OBJECT(&bmc->soc),
> +                            aspeed_boards[board_model].hw_strap1,
> +                            "hw-strap1", &error_abort);
> +    object_property_set_int(OBJECT(&bmc->soc),
> +                            aspeed_boards[board_model].silicon_rev,
>                              "silicon-rev", &error_abort);
>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>                               &error_abort);
> @@ -86,13 +102,15 @@ static void aspeed_init(MachineState *machine)
>      aspeed_binfo.initrd_filename = machine->initrd_filename;
>      aspeed_binfo.kernel_cmdline = machine->kernel_cmdline;
>      aspeed_binfo.ram_size = ram_size;
> +    aspeed_binfo.loader_start = aspeed_boards[board_model].sdram_base,
> +    aspeed_binfo.board_id = aspeed_boards[board_model].silicon_rev,
>      arm_load_kernel(ARM_CPU(first_cpu), &aspeed_binfo);
>  }
>  
>  static void palmetto_bmc_init(MachineState *machine)
>  {
>      machine->cpu_model = "arm926";
> -    aspeed_init(machine);
> +    aspeed_init(machine, PALMETTO_BMC);
>  }
>  
>  static void palmetto_bmc_class_init(ObjectClass *oc, void *data)

On 07/28/2016 04:45 AM, Andrew Jeffery wrote:
> On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
>> aspeed_init() now uses a board identifier to customize some values
>> specific to the board, ram base, board revision number, etc.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Looks okay to me, some minor comments below:
> 
>> ---
>>  hw/arm/palmetto-bmc.c | 34 ++++++++++++++++++++++++++--------
>>  1 file changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
>> index 8a3ff5568575..cd8aa59756b9 100644
>> --- a/hw/arm/palmetto-bmc.c
>> +++ b/hw/arm/palmetto-bmc.c
>> @@ -22,8 +22,6 @@
>>  #include "sysemu/blockdev.h"
>>  
>>  static struct arm_boot_info aspeed_binfo = {
>> -    .loader_start = AST2400_SDRAM_BASE,
>> -    .board_id = 0,
>>      .nb_cpus = 1,
>>  };
>>  
>> @@ -32,6 +30,21 @@ typedef struct AspeedBoardState {
>>      MemoryRegion ram;
>>  } AspeedBoardState;
>>  
>> +typedef struct AspeedBoardConfig {
>> +    uint32_t hw_strap1;
>> +    uint32_t silicon_rev;
>> +    hwaddr sdram_base;
>> +} AspeedBoardConfig;
>> +
>> +enum {
>> +    PALMETTO_BMC
>> +};
>> +
>> +static const AspeedBoardConfig aspeed_boards[] = {
>> +    [ PALMETTO_BMC ] = { 0x120CE416, AST2400_A0_SILICON_REV,
>> +                         AST2400_SDRAM_BASE },
> 
> I was playing around before and my test scripts noticed checkpatch
> complained about the spacing with the array indexing: "[PALMETTO_BMC]"
> fixed the error.

sigh. I am not sure I checkpatched this one.

>> +};
>> +
>>  static void aspeed_init_flashes(AspeedSMCState *s, const char *flashtype,
>>                                  Error **errp)
>>  {
>> @@ -58,7 +71,7 @@ static void aspeed_init_flashes(AspeedSMCState *s, const char *flashtype,
>>      }
>>  }
>>  
>> -static void aspeed_init(MachineState *machine)
>> +static void aspeed_init(MachineState *machine, int board_model)
> 
> I feel like we should pass a "struct AspeedBoardConfig *" rather than
> the "int board_model", cleaning up the repeated indexing into
> aspeed_boards the body. Thoughts? </bikeshed>

yep. I agree. Will change that.

Thanks,

C. 


> Andrew
> 
>>  {
>>      AspeedBoardState *bmc;
>>  
>> @@ -68,13 +81,16 @@ static void aspeed_init(MachineState *machine)
>>                                &error_abort);
>>  
>>      memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
>> -    memory_region_add_subregion(get_system_memory(), AST2400_SDRAM_BASE,
>> +    memory_region_add_subregion(get_system_memory(),
>> +                                aspeed_boards[board_model].sdram_base,
>>                                  &bmc->ram);
>>      object_property_add_const_link(OBJECT(&bmc->soc), "ram", OBJECT(&bmc->ram),
>>                                     &error_abort);
>> -    object_property_set_int(OBJECT(&bmc->soc), 0x120CE416, "hw-strap1",
>> -                            &error_abort);
>> -    object_property_set_int(OBJECT(&bmc->soc), AST2400_A0_SILICON_REV,
>> +    object_property_set_int(OBJECT(&bmc->soc),
>> +                            aspeed_boards[board_model].hw_strap1,
>> +                            "hw-strap1", &error_abort);
>> +    object_property_set_int(OBJECT(&bmc->soc),
>> +                            aspeed_boards[board_model].silicon_rev,
>>                              "silicon-rev", &error_abort);
>>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>>                               &error_abort);
>> @@ -86,13 +102,15 @@ static void aspeed_init(MachineState *machine)
>>      aspeed_binfo.initrd_filename = machine->initrd_filename;
>>      aspeed_binfo.kernel_cmdline = machine->kernel_cmdline;
>>      aspeed_binfo.ram_size = ram_size;
>> +    aspeed_binfo.loader_start = aspeed_boards[board_model].sdram_base,
>> +    aspeed_binfo.board_id = aspeed_boards[board_model].silicon_rev,
>>      arm_load_kernel(ARM_CPU(first_cpu), &aspeed_binfo);
>>  }
>>  
>>  static void palmetto_bmc_init(MachineState *machine)
>>  {
>>      machine->cpu_model = "arm926";
>> -    aspeed_init(machine);
>> +    aspeed_init(machine, PALMETTO_BMC);
>>  }
>>  
>>  static void palmetto_bmc_class_init(ObjectClass *oc, void *data)