| Message ID | 1469638018-17590-7-git-send-email-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote: > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/arm/palmetto-bmc.c | 32 +++++++++++++++++++++++++++++++- > include/hw/arm/ast2400.h | 5 +++++ > 2 files changed, 36 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c > index cd8aa59756b9..8d8bfeb571e2 100644 > --- a/hw/arm/palmetto-bmc.c > +++ b/hw/arm/palmetto-bmc.c > @@ -37,12 +37,15 @@ typedef struct AspeedBoardConfig { > } AspeedBoardConfig; > > enum { > - PALMETTO_BMC > + PALMETTO_BMC, > + AST2500_EDK It was called 'ast2500-edk' in the out-of-tree patches, but can we rename it 'ast2500-evb'? This would make it consistent with patches we have in our Linux trees. > }; > > static const AspeedBoardConfig aspeed_boards[] = { > [ PALMETTO_BMC ] = { 0x120CE416, AST2400_A0_SILICON_REV, > AST2400_SDRAM_BASE }, > + [ AST2500_EDK ] = { 0x00000200, AST2500_A1_SILICON_REV, > + AST2500_SDRAM_BASE }, Can we include the strap value from the board for completeness? Also, the meaning of the bits have changed from the AST2400 - they probably should be documented somewhere? Finally, checkpatch complained here too regarding the whitespace, I ran into the issue replacing the strap value. > }; > > static void aspeed_init_flashes(AspeedSMCState *s, const char *flashtype, > @@ -133,9 +136,36 @@ static const TypeInfo palmetto_bmc_type = { > .class_init = palmetto_bmc_class_init, > }; > > +static void ast2500_edk_init(MachineState *machine) > +{ > + machine->cpu_model = "arm1176"; > + aspeed_init(machine, AST2500_EDK); > +} > + > +static void ast2500_edk_class_init(ObjectClass *oc, void *data) > +{ > + MachineClass *mc = MACHINE_CLASS(oc); > + > + mc->desc = "Aspeed AST2500 EDK (ARM1176)"; > + mc->init = ast2500_edk_init; > + mc->max_cpus = 1; > + mc->no_sdcard = 1; > + mc->no_floppy = 1; > + mc->no_cdrom = 1; > + mc->no_sdcard = 1; mc->no_sdcard is already assigned a couple of lines up. I think this may be the case for palmetto config as well... Cheers, Andrew > + mc->no_parallel = 1; > +} > + > +static const TypeInfo ast2500_edk_type = { > + .name = MACHINE_TYPE_NAME("ast2500-edk"), > + .parent = TYPE_MACHINE, > + .class_init = ast2500_edk_class_init, > +}; > + > static void aspeed_machine_init(void) > { > type_register_static(&palmetto_bmc_type); > + type_register_static(&ast2500_edk_type); > } > > type_init(aspeed_machine_init) > diff --git a/include/hw/arm/ast2400.h b/include/hw/arm/ast2400.h > index e68807d475b7..2e6864f88790 100644 > --- a/include/hw/arm/ast2400.h > +++ b/include/hw/arm/ast2400.h > @@ -41,4 +41,9 @@ typedef struct AST2400State { > > #define AST2400_SDRAM_BASE 0x40000000 > > +/* > + * for Aspeed AST2500 SOC and higher > + */ > +#define AST2500_SDRAM_BASE 0x80000000 > + > #endif /* AST2400_H */
On 07/28/2016 07:11 AM, Andrew Jeffery wrote: > On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote: >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/arm/palmetto-bmc.c | 32 +++++++++++++++++++++++++++++++- >> include/hw/arm/ast2400.h | 5 +++++ >> 2 files changed, 36 insertions(+), 1 deletion(-) >> >> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c >> index cd8aa59756b9..8d8bfeb571e2 100644 >> --- a/hw/arm/palmetto-bmc.c >> +++ b/hw/arm/palmetto-bmc.c >> @@ -37,12 +37,15 @@ typedef struct AspeedBoardConfig { >> } AspeedBoardConfig; >> >> enum { >> - PALMETTO_BMC >> + PALMETTO_BMC, >> + AST2500_EDK > > It was called 'ast2500-edk' in the out-of-tree patches, but can we > rename it 'ast2500-evb'? This would make it consistent with patches we > have in our Linux trees. yes. I feel the same also. > >> }; >> >> static const AspeedBoardConfig aspeed_boards[] = { >> [ PALMETTO_BMC ] = { 0x120CE416, AST2400_A0_SILICON_REV, >> AST2400_SDRAM_BASE }, >> + [ AST2500_EDK ] = { 0x00000200, AST2500_A1_SILICON_REV, >> + AST2500_SDRAM_BASE }, > > Can we include the strap value from the board for completeness? > > Also, the meaning of the bits have changed from the AST2400 - they > probably should be documented somewhere? So you want me send to an updated version of : http://lists.nongnu.org/archive/html/qemu-arm/2016-06/msg00698.html as a prereq ? Now that we have done the cleanups in U-Boot, we can pull from : https://github.com/openbmc/u-boot/blob/v2016.07-aspeed-openbmc/arch/arm/include/asm/arch-aspeed/regs-scu.h to get the definitions. I will add that. > Finally, checkpatch complained here too regarding the whitespace, I ran > into the issue replacing the strap value. ok. >> }; >> >> static void aspeed_init_flashes(AspeedSMCState *s, const char *flashtype, >> @@ -133,9 +136,36 @@ static const TypeInfo palmetto_bmc_type = { >> .class_init = palmetto_bmc_class_init, >> }; >> >> +static void ast2500_edk_init(MachineState *machine) >> +{ >> + machine->cpu_model = "arm1176"; >> + aspeed_init(machine, AST2500_EDK); >> +} >> + >> +static void ast2500_edk_class_init(ObjectClass *oc, void *data) >> +{ >> + MachineClass *mc = MACHINE_CLASS(oc); >> + >> + mc->desc = "Aspeed AST2500 EDK (ARM1176)"; >> + mc->init = ast2500_edk_init; >> + mc->max_cpus = 1; >> + mc->no_sdcard = 1; >> + mc->no_floppy = 1; >> + mc->no_cdrom = 1; >> + mc->no_sdcard = 1; > > mc->no_sdcard is already assigned a couple of lines up. I think this > may be the case for palmetto config as well... That was a blind copy paste. I will remove the extra sdcard. Thanks, C. > Cheers, > > Andrew > >> + mc->no_parallel = 1; >> +} >> + >> +static const TypeInfo ast2500_edk_type = { >> + .name = MACHINE_TYPE_NAME("ast2500-edk"), >> + .parent = TYPE_MACHINE, >> + .class_init = ast2500_edk_class_init, >> +}; >> + >> static void aspeed_machine_init(void) >> { >> type_register_static(&palmetto_bmc_type); >> + type_register_static(&ast2500_edk_type); >> } >> >> type_init(aspeed_machine_init) >> diff --git a/include/hw/arm/ast2400.h b/include/hw/arm/ast2400.h >> index e68807d475b7..2e6864f88790 100644 >> --- a/include/hw/arm/ast2400.h >> +++ b/include/hw/arm/ast2400.h >> @@ -41,4 +41,9 @@ typedef struct AST2400State { >> >> #define AST2400_SDRAM_BASE 0x40000000 >> >> +/* >> + * for Aspeed AST2500 SOC and higher >> + */ >> +#define AST2500_SDRAM_BASE 0x80000000 >> + >> #endif /* AST2400_H */
On Thu, 2016-07-28 at 09:15 +0200, Cédric Le Goater wrote: > > > > Also, the meaning of the bits have changed from the AST2400 - they > > probably should be documented somewhere? > > So you want me send to an updated version of : > > http://lists.nongnu.org/archive/html/qemu-arm/2016-06/msg00698.html > > as a prereq ? I mentioned this in passing due to the discussion on my original patch. I think we discussed this separately and concluded the macros were pretty verbose given they are sort-of single-use given the value doesn't change. Maybe just comments as Peter was requesting? You have the patch below but some of the macros will be different for the AST2500. I'm probably leaning towards comments over macros, but don't feel strongly either way. Andrew > > Now that we have done the cleanups in U-Boot, we can pull from : > > https://github.com/openbmc/u-boot/blob/v2016.07-aspeed-openbmc/arch/arm/include/asm/arch-aspeed/regs-scu.h > > to get the definitions. I will add that.
On 07/28/2016 09:58 AM, Andrew Jeffery wrote: > On Thu, 2016-07-28 at 09:15 +0200, Cédric Le Goater wrote: >>> >>> Also, the meaning of the bits have changed from the AST2400 - they >>> probably should be documented somewhere? >> >> So you want me send to an updated version of : >> >> http://lists.nongnu.org/archive/html/qemu-arm/2016-06/msg00698.html >> >> as a prereq ? > > I mentioned this in passing due to the discussion on my original patch. > I think we discussed this separately and concluded the macros were > pretty verbose given they are sort-of single-use given the value > doesn't change. Maybe just comments as Peter was requesting? You have > the patch below but some of the macros will be different for the > AST2500. yes. > I'm probably leaning towards comments over macros, but don't feel > strongly either way. ok. having a correct value is a minimum and this is not the case in this patch. I think I will go for the comments for now as We have not merged anything in mainline uboot yet. Thanks, C.
On 07/28/2016 10:03 AM, Cédric Le Goater wrote: > On 07/28/2016 09:58 AM, Andrew Jeffery wrote: >> On Thu, 2016-07-28 at 09:15 +0200, Cédric Le Goater wrote: >>>> >>>> Also, the meaning of the bits have changed from the AST2400 - they >>>> probably should be documented somewhere? >>> >>> So you want me send to an updated version of : >>> >>> http://lists.nongnu.org/archive/html/qemu-arm/2016-06/msg00698.html >>> >>> as a prereq ? >> >> I mentioned this in passing due to the discussion on my original patch. >> I think we discussed this separately and concluded the macros were >> pretty verbose given they are sort-of single-use given the value >> doesn't change. Maybe just comments as Peter was requesting? You have >> the patch below but some of the macros will be different for the >> AST2500. > > yes. > >> I'm probably leaning towards comments over macros, but don't feel >> strongly either way. > > ok. having a correct value is a minimum and this is not the case > in this patch. I think I will go for the comments for now as We > have not merged anything in mainline uboot yet. I gave comments a try and honestly, macros are cleaner to check which bits you are setting. less prone to errors. So I will send a v2 with macros. Cheers, C.
diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c index cd8aa59756b9..8d8bfeb571e2 100644 --- a/hw/arm/palmetto-bmc.c +++ b/hw/arm/palmetto-bmc.c @@ -37,12 +37,15 @@ typedef struct AspeedBoardConfig { } AspeedBoardConfig; enum { - PALMETTO_BMC + PALMETTO_BMC, + AST2500_EDK }; static const AspeedBoardConfig aspeed_boards[] = { [ PALMETTO_BMC ] = { 0x120CE416, AST2400_A0_SILICON_REV, AST2400_SDRAM_BASE }, + [ AST2500_EDK ] = { 0x00000200, AST2500_A1_SILICON_REV, + AST2500_SDRAM_BASE }, }; static void aspeed_init_flashes(AspeedSMCState *s, const char *flashtype, @@ -133,9 +136,36 @@ static const TypeInfo palmetto_bmc_type = { .class_init = palmetto_bmc_class_init, }; +static void ast2500_edk_init(MachineState *machine) +{ + machine->cpu_model = "arm1176"; + aspeed_init(machine, AST2500_EDK); +} + +static void ast2500_edk_class_init(ObjectClass *oc, void *data) +{ + MachineClass *mc = MACHINE_CLASS(oc); + + mc->desc = "Aspeed AST2500 EDK (ARM1176)"; + mc->init = ast2500_edk_init; + mc->max_cpus = 1; + mc->no_sdcard = 1; + mc->no_floppy = 1; + mc->no_cdrom = 1; + mc->no_sdcard = 1; + mc->no_parallel = 1; +} + +static const TypeInfo ast2500_edk_type = { + .name = MACHINE_TYPE_NAME("ast2500-edk"), + .parent = TYPE_MACHINE, + .class_init = ast2500_edk_class_init, +}; + static void aspeed_machine_init(void) { type_register_static(&palmetto_bmc_type); + type_register_static(&ast2500_edk_type); } type_init(aspeed_machine_init) diff --git a/include/hw/arm/ast2400.h b/include/hw/arm/ast2400.h index e68807d475b7..2e6864f88790 100644 --- a/include/hw/arm/ast2400.h +++ b/include/hw/arm/ast2400.h @@ -41,4 +41,9 @@ typedef struct AST2400State { #define AST2400_SDRAM_BASE 0x40000000 +/* + * for Aspeed AST2500 SOC and higher + */ +#define AST2500_SDRAM_BASE 0x80000000 + #endif /* AST2400_H */
Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/arm/palmetto-bmc.c | 32 +++++++++++++++++++++++++++++++- include/hw/arm/ast2400.h | 5 +++++ 2 files changed, 36 insertions(+), 1 deletion(-)