[Qemu-devel] [PATCH 0/6] arm: add ast2500 support

[Qemu-devel] [PATCH 0/6] arm: add ast2500 support

[Cédric Le Goater]

The ast2500 soc being very close to the ast2400 soc, the goal of the
changes below is to modify the existing platform 'palmetto-bmc' and
existing soc 'ast2400' to take into account the small differences and
avoid code duplication. This is mostly inspired by the realview
platform.

First patches rework the palmetto-bmc platform and the ast2400 soc
models to provide room to other platforms and socs which have a common
design. Being able to set the 'silicon-rev' and the cpu model are the
primary motivation.

I tried to link all the silicon-rev properties to a common one (in the
SCU controller) but I failed to find the right pattern. I am not sure
it is possible to have multiple aliases on the same property. You will
see what I came up with in the first patch.

The last patches add support for the new ast2500 soc in the required
controller (sdmc and scu) and define a new platform for an Aspeed
evaluation board.

The 'palmetto-bmc.c' file could be renamed to 'aspeed.c' now that it
contains more than one platform, but I don't really like the idea as
it breaks history. As for the ast2400.c file, I think we are fine as
we are just adding a configurable cpu. Nothing major. Please advise on
that topic.


On the ast2500, I am still having a little issue under uboot which
sets the vbar doing :

	mcr     p15, 0, r0, c12, c0, 0  /* Set VBAR */

and this is trapped as an undefined instruction by qemu.

Looking at hw/arm/helper.c, the VBAR register seems to be defined only
for feature ARM_FEATURE_V7 (v7_cp_reginfo). The ast2500 soc uses a
arm1176 which defines ARM_FEATURE_EL3 which gives us a VBAR_EL3.
According to th specs, the arm1176jzf-s has a Vector Base Address
Register. So am I missing something in the board definition or is
uboot being too optimistic on the cpu features ? This is confusing for
me, some direction would be welcomed :)


A part from that, the soc behaves fine.

Thanks,


Cédric Le Goater (6):
  palmetto-bmc: add a "silicon-rev" property at the soc level
  palmetto-bmc: replace palmetto_bmc with aspeed
  ast2400: use machine cpu_model to initialize the soc cpu
  palmetto-bmc: add board specific configuration
  aspeed/scu: add ast2500 support
  arm: add support for an ast2500 evaluation board

 hw/arm/ast2400.c             |  26 +++++++---
 hw/arm/palmetto-bmc.c        | 115 ++++++++++++++++++++++++++++++++++---------
 hw/misc/aspeed_scu.c         |  45 ++++++++++++++++-
 hw/misc/aspeed_sdmc.c        |   1 +
 include/hw/arm/ast2400.h     |   5 ++
 include/hw/misc/aspeed_scu.h |   1 +
 6 files changed, 163 insertions(+), 30 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/6] palmetto-bmc: add a "silicon-rev" property at the soc level

[Cédric Le Goater]

The SCU controler holds the board revision number in its 0x7C
register. Let's use an alias to link a "silicon-rev" property of the
soc to the "silicon-rev" property of the SCU controler.

The SDMC controler "silicon-rev" property is derived from the SCU one
at realize time. I did not find a better way to handle this part.
Links and aliases being a one-to-one relation, I could not use one of
them. I might wrong though.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/ast2400.c      | 18 +++++++++++++-----
 hw/arm/palmetto-bmc.c |  2 ++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
index 136bf6464e1d..fa535065f765 100644
--- a/hw/arm/ast2400.c
+++ b/hw/arm/ast2400.c
@@ -84,8 +84,8 @@ static void ast2400_init(Object *obj)
     object_initialize(&s->scu, sizeof(s->scu), TYPE_ASPEED_SCU);
     object_property_add_child(obj, "scu", OBJECT(&s->scu), NULL);
     qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
-    qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev",
-                         AST2400_A0_SILICON_REV);
+    object_property_add_alias(obj, "silicon-rev", OBJECT(&s->scu),
+                              "silicon-rev", &error_abort);
     object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu),
                               "hw-strap1", &error_abort);
     object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu),
@@ -102,8 +102,6 @@ static void ast2400_init(Object *obj)
     object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
     object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
     qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
-    qdev_prop_set_uint32(DEVICE(&s->sdmc), "silicon-rev",
-                         AST2400_A0_SILICON_REV);
 }
 
 static void ast2400_realize(DeviceState *dev, Error **errp)
@@ -111,6 +109,7 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
     int i;
     AST2400State *s = AST2400(dev);
     Error *err = NULL, *local_err = NULL;
+    uint32_t silicon_rev;
 
     /* IO space */
     memory_region_init_io(&s->iomem, NULL, &ast2400_io_ops, NULL,
@@ -192,7 +191,16 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 1, AST2400_SPI_FLASH_BASE);
 
     /* SDMC - SDRAM Memory Controller */
-    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err);
+    silicon_rev = (uint32_t)
+        object_property_get_int(OBJECT(&s->scu), "silicon-rev", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    object_property_set_int(OBJECT(&s->sdmc), silicon_rev, "silicon-rev", &err);
+    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &local_err);
+    error_propagate(&err, local_err);
     if (err) {
         error_propagate(errp, err);
         return;
diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
index 54e29a865d88..1ee13d578899 100644
--- a/hw/arm/palmetto-bmc.c
+++ b/hw/arm/palmetto-bmc.c
@@ -74,6 +74,8 @@ static void palmetto_bmc_init(MachineState *machine)
                                    &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,
+                            "silicon-rev", &error_abort);
     object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
                              &error_abort);
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/6] palmetto-bmc: add a "silicon-rev" property at the soc level

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4784 bytes --]

On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
> The SCU controler holds the board revision number in its 0x7C
> register. Let's use an alias to link a "silicon-rev" property of the
> soc to the "silicon-rev" property of the SCU controler.
> 
> The SDMC controler "silicon-rev" property is derived from the SCU one
> at realize time. I did not find a better way to handle this part.
> Links and aliases being a one-to-one relation, I could not use one of
> them. I might wrong though.

Are we trying to over-use the silicon-rev value (it would seem so at
least in the face of the link/alias constraints)? We know which SDMC
revision we need for each SoC and we'll be modelling an explicit SoC
revision, so should we instead set a separate property on the SDMC in
the SoCs' respective initialise functions (and leave silicon-rev to the
SCU)? My thought was the silicon-rev value is reflective of the SoC
design rather than the other way around - but maybe that's splitting
hairs. It would also be trading off a bit of ugliness in this patch for
potential bugs if the properties get out of sync.

Cheers,

Andrew

> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/arm/ast2400.c      | 18 +++++++++++++-----
>  hw/arm/palmetto-bmc.c |  2 ++
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
> index 136bf6464e1d..fa535065f765 100644
> --- a/hw/arm/ast2400.c
> +++ b/hw/arm/ast2400.c
> @@ -84,8 +84,8 @@ static void ast2400_init(Object *obj)
>      object_initialize(&s->scu, sizeof(s->scu), TYPE_ASPEED_SCU);
>      object_property_add_child(obj, "scu", OBJECT(&s->scu), NULL);
>      qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
> -    qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev",
> -                         AST2400_A0_SILICON_REV);
> +    object_property_add_alias(obj, "silicon-rev", OBJECT(&s->scu),
> +                              "silicon-rev", &error_abort);
>      object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu),
>                                "hw-strap1", &error_abort);
>      object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu),
> @@ -102,8 +102,6 @@ static void ast2400_init(Object *obj)
>      object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
>      object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
>      qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
> -    qdev_prop_set_uint32(DEVICE(&s->sdmc), "silicon-rev",
> -                         AST2400_A0_SILICON_REV);
>  }
>  
>  static void ast2400_realize(DeviceState *dev, Error **errp)
> @@ -111,6 +109,7 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
>      int i;
>      AST2400State *s = AST2400(dev);
>      Error *err = NULL, *local_err = NULL;
> +    uint32_t silicon_rev;
>  
>      /* IO space */
>      memory_region_init_io(&s->iomem, NULL, &ast2400_io_ops, NULL,
> @@ -192,7 +191,16 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 1, AST2400_SPI_FLASH_BASE);
>  
>      /* SDMC - SDRAM Memory Controller */
> -    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err);
> +    silicon_rev = (uint32_t)
> +        object_property_get_int(OBJECT(&s->scu), "silicon-rev", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    object_property_set_int(OBJECT(&s->sdmc), silicon_rev, "silicon-rev", &err);
> +    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &local_err);
> +    error_propagate(&err, local_err);
>      if (err) {
>          error_propagate(errp, err);
>          return;
> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
> index 54e29a865d88..1ee13d578899 100644
> --- a/hw/arm/palmetto-bmc.c
> +++ b/hw/arm/palmetto-bmc.c
> @@ -74,6 +74,8 @@ static void palmetto_bmc_init(MachineState *machine)
>                                     &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,
> +                            "silicon-rev", &error_abort);
>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>                               &error_abort);
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/6] palmetto-bmc: add a "silicon-rev" property at the soc level

[Cédric Le Goater]

On 07/28/2016 04:14 AM, Andrew Jeffery wrote:
> On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
>> The SCU controler holds the board revision number in its 0x7C
>> register. Let's use an alias to link a "silicon-rev" property of the
>> soc to the "silicon-rev" property of the SCU controler.
>>
>> The SDMC controler "silicon-rev" property is derived from the SCU one
>> at realize time. I did not find a better way to handle this part.
>> Links and aliases being a one-to-one relation, I could not use one of
>> them. I might wrong though.
> 
> Are we trying to over-use the silicon-rev value (it would seem so at
> least in the face of the link/alias constraints)? We know which SDMC
> revision we need for each SoC and we'll be modelling an explicit SoC
> revision, so should we instead set a separate property on the SDMC in
> the SoCs' respective initialise functions (and leave silicon-rev to the
> SCU)? 

This is the case. no ? 

SCU holds the silicon-rev value. The patch adds a property alias to the 
SCU 'silicon-rev' property at the soc level. This is convenient for the
platform to initialize the soc. This is similar to what the rpi2 does,
which goes one level in the aliasing.

Then, at initialize time, the SCU 'silicon-rev' property value is read
to initialize the SDMC controller. If we have more controllers in the 
future needing 'silicon-rev,  we could follow the same pattern. Not 
saying this is perfect. 

What I would have liked to do, is to link all the 'silicon-rev' do
the SCU one. I did not find a way.

> My thought was the silicon-rev value is reflective of the SoC
> design rather than the other way around - but maybe that's splitting
> hairs. 

ah. is your concern about which object is holding the value ? If so,
I thought that keeping it where it belongs on real HW was the better 
option, that is in SCU, and then build from there.

> It would also be trading off a bit of ugliness in this patch for
> potential bugs if the properties get out of sync.

This is the exact the purpose of the patch ! I failed to make it feel
that way :) 

Thanks,

C. 

>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/arm/ast2400.c      | 18 +++++++++++++-----
>>  hw/arm/palmetto-bmc.c |  2 ++
>>  2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
>> index 136bf6464e1d..fa535065f765 100644
>> --- a/hw/arm/ast2400.c
>> +++ b/hw/arm/ast2400.c
>> @@ -84,8 +84,8 @@ static void ast2400_init(Object *obj)
>>      object_initialize(&s->scu, sizeof(s->scu), TYPE_ASPEED_SCU);
>>      object_property_add_child(obj, "scu", OBJECT(&s->scu), NULL);
>>      qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
>> -    qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev",
>> -                         AST2400_A0_SILICON_REV);
>> +    object_property_add_alias(obj, "silicon-rev", OBJECT(&s->scu),
>> +                              "silicon-rev", &error_abort);
>>      object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu),
>>                                "hw-strap1", &error_abort);
>>      object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu),
>> @@ -102,8 +102,6 @@ static void ast2400_init(Object *obj)
>>      object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
>>      object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
>>      qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
>> -    qdev_prop_set_uint32(DEVICE(&s->sdmc), "silicon-rev",
>> -                         AST2400_A0_SILICON_REV);
>>  }
>>  
>>  static void ast2400_realize(DeviceState *dev, Error **errp)
>> @@ -111,6 +109,7 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
>>      int i;
>>      AST2400State *s = AST2400(dev);
>>      Error *err = NULL, *local_err = NULL;
>> +    uint32_t silicon_rev;
>>  
>>      /* IO space */
>>      memory_region_init_io(&s->iomem, NULL, &ast2400_io_ops, NULL,
>> @@ -192,7 +191,16 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
>>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 1, AST2400_SPI_FLASH_BASE);
>>  
>>      /* SDMC - SDRAM Memory Controller */
>> -    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err);
>> +    silicon_rev = (uint32_t)
>> +        object_property_get_int(OBJECT(&s->scu), "silicon-rev", &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +
>> +    object_property_set_int(OBJECT(&s->sdmc), silicon_rev, "silicon-rev", &err);
>> +    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &local_err);
>> +    error_propagate(&err, local_err);
>>      if (err) {
>>          error_propagate(errp, err);
>>          return;
>> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
>> index 54e29a865d88..1ee13d578899 100644
>> --- a/hw/arm/palmetto-bmc.c
>> +++ b/hw/arm/palmetto-bmc.c
>> @@ -74,6 +74,8 @@ static void palmetto_bmc_init(MachineState *machine)
>>                                     &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,
>> +                            "silicon-rev", &error_abort);
>>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>>                               &error_abort);
>>  

Re: [Qemu-devel] [PATCH 1/6] palmetto-bmc: add a "silicon-rev" property at the soc level

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4239 bytes --]

On Thu, 2016-07-28 at 09:51 +0200, Cédric Le Goater wrote:
> On 07/28/2016 04:14 AM, Andrew Jeffery wrote:
> > 
> > On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
> > > 
> > > The SCU controler holds the board revision number in its 0x7C
> > > register. Let's use an alias to link a "silicon-rev" property of the
> > > soc to the "silicon-rev" property of the SCU controler.
> > > 
> > > The SDMC controler "silicon-rev" property is derived from the SCU one
> > > at realize time. I did not find a better way to handle this part.
> > > Links and aliases being a one-to-one relation, I could not use one of
> > > them. I might wrong though.
> > Are we trying to over-use the silicon-rev value (it would seem so at
> > least in the face of the link/alias constraints)? We know which SDMC
> > revision we need for each SoC and we'll be modelling an explicit SoC
> > revision, so should we instead set a separate property on the SDMC in
> > the SoCs' respective initialise functions (and leave silicon-rev to the
> > SCU)? 
> This is the case. no ? 

No, because you are selecting the SDMC configuration from the silicon-
rev rather than letting e.g. the SDMC configuration define which
silicon-rev the SoC takes.

My thinking is the silicon rev is a property of the SoC. The platform
should select a SoC to use and not be setting silicon revision values,
that is I think it's a layering violation for the platform to be
setting the silicon-rev (and also the CPU).

We also wind up in a situation where the ast2500 soc identifies as an
ast2400 in TypeInfo.name due to the approach to reuse by this series.

> 
> SCU holds the silicon-rev value. The patch adds a property alias to the 
> SCU 'silicon-rev' property at the soc level. This is convenient

Right, but "convenient" here is a bit of a stretch given we are
subsequently fetching the value out of the SCU to select the SDMC
configuration. You might argue that it's due to limitations of the
property alias/link API, but maybe we could rearrange things so this
goes away.

>  for the
> platform to initialize the soc. This is similar to what the rpi2 does,
> which goes one level in the aliasing.

Okay, maybe I'm barking up trees unnecessarily.

> 
> Then, at initialize time, the SCU 'silicon-rev' property value is read
> to initialize the SDMC controller. If we have more controllers in the 
> future needing 'silicon-rev,  we could follow the same pattern. Not 
> saying this is perfect. 
> 
> What I would have liked to do, is to link all the 'silicon-rev' do
> the SCU one. I did not find a way.

Yes, that would be nice. I did briefly poke around to see if there was
a solution to the link/alias issue but it seems not. 

> 
> > 
> > My thought was the silicon-rev value is reflective of the SoC
> > design rather than the other way around - but maybe that's splitting
> > hairs. 
> ah. is your concern about which object is holding the value ? If so,
> I thought that keeping it where it belongs on real HW was the better 
> option, that is in SCU, and then build from there.

No, that's not my concern, but I agree that it would not reflect the
hardware if there was a property on the SoC (i.e. there is nowhere
besides the SCU that the value is held).

> 
> > 
> > It would also be trading off a bit of ugliness in this patch for
> > potential bugs if the properties get out of sync.
> This is the exact the purpose of the patch ! I failed to make it feel
> that way :)

Right. I think we need another layer of abstraction, essentially a soc
configuration struct that is accessed by what are currently the
ast2400_{init,realise}() functions. This will capture differences like
changes in IO addresses, changes to IP behaviour, the CPU types and
ultimately the silicon-rev value. What is now ast2400_init() and
ast2400_realise() can become generic aspeed_soc_{init,realise}(), and
then we wrap calls to this up in SoC-specific
ast2{4,5}00_{init,realise}() where we set the configuration struct. It
is a bit more work but I think the result would better reflect the
hardware and avoid introducing what feel to me like layering
violations.

Thoughts?

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/6] palmetto-bmc: add a "silicon-rev" property at the soc level

[Cédric Le Goater]

On 07/29/2016 03:16 AM, Andrew Jeffery wrote:
> On Thu, 2016-07-28 at 09:51 +0200, Cédric Le Goater wrote:
>> On 07/28/2016 04:14 AM, Andrew Jeffery wrote:
>>>
>>> On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
>>>>
>>>> The SCU controler holds the board revision number in its 0x7C
>>>> register. Let's use an alias to link a "silicon-rev" property of the
>>>> soc to the "silicon-rev" property of the SCU controler.
>>>>
>>>> The SDMC controler "silicon-rev" property is derived from the SCU one
>>>> at realize time. I did not find a better way to handle this part.
>>>> Links and aliases being a one-to-one relation, I could not use one of
>>>> them. I might wrong though.
>>> Are we trying to over-use the silicon-rev value (it would seem so at
>>> least in the face of the link/alias constraints)? We know which SDMC
>>> revision we need for each SoC and we'll be modelling an explicit SoC
>>> revision, so should we instead set a separate property on the SDMC in
>>> the SoCs' respective initialise functions (and leave silicon-rev to the
>>> SCU)? 
>> This is the case. no ? 
> 
> No, because you are selecting the SDMC configuration from the silicon-
> rev rather than letting e.g. the SDMC configuration define which
> silicon-rev the SoC takes.
> 
> My thinking is the silicon rev is a property of the SoC. The platform
> should select a SoC to use and not be setting silicon revision values,
> that is I think it's a layering violation for the platform to be
> setting the silicon-rev (and also the CPU).
> 
> We also wind up in a situation where the ast2500 soc identifies as an
> ast2400 in TypeInfo.name due to the approach to reuse by this series.

OK. To be cleaner, we could add a AspeedSoCClass and a set of predefined 
subclasses for each revision we support, that we would instantiate at the 
platform level.
  
The AspeedSocClass would be similar to the AspeedBoardConfig struct in 
the current patchset, plus the cpu model. How would that be ?   

>> SCU holds the silicon-rev value. The patch adds a property alias to the 
>> SCU 'silicon-rev' property at the soc level. This is convenient
> 
> Right, but "convenient" here is a bit of a stretch given we are
> subsequently fetching the value out of the SCU to select the SDMC
> configuration. You might argue that it's due to limitations of the
> property alias/link API, but maybe we could rearrange things so this
> goes away.

yes that would be nicer not to have to re-set the silicon rev of the 
controllers of a SoC.

>>  for the
>> platform to initialize the soc. This is similar to what the rpi2 does,
>> which goes one level in the aliasing.
> 
> Okay, maybe I'm barking up trees unnecessarily.

No. your point on the SoC reuse is very valid. I try not to overspecify 
too early but I agree I took a little shortcut. I will kick a v3 with
the above. It should not be too much of a change.

>> Then, at initialize time, the SCU 'silicon-rev' property value is read
>> to initialize the SDMC controller. If we have more controllers in the 
>> future needing 'silicon-rev,  we could follow the same pattern. Not 
>> saying this is perfect. 
>>
>> What I would have liked to do, is to link all the 'silicon-rev' do
>> the SCU one. I did not find a way.
> 
> Yes, that would be nice. I did briefly poke around to see if there was
> a solution to the link/alias issue but it seems not. 
> 
>>
>>>
>>> My thought was the silicon-rev value is reflective of the SoC
>>> design rather than the other way around - but maybe that's splitting
>>> hairs. 
>> ah. is your concern about which object is holding the value ? If so,
>> I thought that keeping it where it belongs on real HW was the better 
>> option, that is in SCU, and then build from there.
> 
> No, that's not my concern, but I agree that it would not reflect the
> hardware if there was a property on the SoC (i.e. there is nowhere
> besides the SCU that the value is held).

So I understand that the 'silicon-rev' location is not the biggest concern
and we can keep it as it is in the patchset. Being able to alias the 
different 'silicon-rev' properties to a common one would be nice though.
 
>>> It would also be trading off a bit of ugliness in this patch for
>>> potential bugs if the properties get out of sync.
>> This is the exact the purpose of the patch ! I failed to make it feel
>> that way :)
> 
> Right. I think we need another layer of abstraction, essentially a soc
> configuration struct that is accessed by what are currently the
> ast2400_{init,realise}() functions. This will capture differences like
> changes in IO addresses, changes to IP behaviour, the CPU types and
> ultimately the silicon-rev value. What is now ast2400_init() and
> ast2400_realise() can become generic aspeed_soc_{init,realise}(), and
> then we wrap calls to this up in SoC-specific
> ast2{4,5}00_{init,realise}() where we set the configuration struct. It
> is a bit more work but I think the result would better reflect the
> hardware and avoid introducing what feel to me like layering
> violations.
>
> Thoughts?

Looks good. However I don't think there is no need for init() and realize() 
ops right now. A .class_data should be sufficient. see this patch [1]. I still 
need to rework that i2c patch btw.  

We can rework the SoC realize() if the need arise.

Cheers,

C.

[1] https://patchwork.kernel.org/patch/9129887/

Re: [Qemu-devel] [PATCH 1/6] palmetto-bmc: add a "silicon-rev" property at the soc level

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 6585 bytes --]

On Sat, 2016-07-30 at 10:35 +0200, Cédric Le Goater wrote:
> On 07/29/2016 03:16 AM, Andrew Jeffery wrote:
> > 
> > On Thu, 2016-07-28 at 09:51 +0200, Cédric Le Goater wrote:
> > > 
> > > On 07/28/2016 04:14 AM, Andrew Jeffery wrote:
> > > > 
> > > > 
> > > > On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
> > > > > 
> > > > > 
> > > > > The SCU controler holds the board revision number in its 0x7C
> > > > > register. Let's use an alias to link a "silicon-rev" property of the
> > > > > soc to the "silicon-rev" property of the SCU controler.
> > > > > 
> > > > > The SDMC controler "silicon-rev" property is derived from the SCU one
> > > > > at realize time. I did not find a better way to handle this part.
> > > > > Links and aliases being a one-to-one relation, I could not use one of
> > > > > them. I might wrong though.
> > > > Are we trying to over-use the silicon-rev value (it would seem so at
> > > > least in the face of the link/alias constraints)? We know which SDMC
> > > > revision we need for each SoC and we'll be modelling an explicit SoC
> > > > revision, so should we instead set a separate property on the SDMC in
> > > > the SoCs' respective initialise functions (and leave silicon-rev to the
> > > > SCU)? 
> > > This is the case. no ? 
> > No, because you are selecting the SDMC configuration from the silicon-
> > rev rather than letting e.g. the SDMC configuration define which
> > silicon-rev the SoC takes.
> > 
> > My thinking is the silicon rev is a property of the SoC. The platform
> > should select a SoC to use and not be setting silicon revision values,
> > that is I think it's a layering violation for the platform to be
> > setting the silicon-rev (and also the CPU).
> > 
> > We also wind up in a situation where the ast2500 soc identifies as an
> > ast2400 in TypeInfo.name due to the approach to reuse by this series.
> OK. To be cleaner, we could add a AspeedSoCClass and a set of predefined 
> subclasses for each revision we support, that we would instantiate at the 
> platform level.

Yes - an AspeedSocClass is the way I went when I briefly started to
sketch out a patch to make my thoughts more concrete.

>   
> The AspeedSocClass would be similar to the AspeedBoardConfig struct in 
> the current patchset, plus the cpu model. How would that be ?  

Sounds good to me, though hw-strap1 still needs to be set by the
platform and not the SoC.

>  
> 
> > 
> > > 
> > > SCU holds the silicon-rev value. The patch adds a property alias to the 
> > > SCU 'silicon-rev' property at the soc level. This is convenient
> > Right, but "convenient" here is a bit of a stretch given we are
> > subsequently fetching the value out of the SCU to select the SDMC
> > configuration. You might argue that it's due to limitations of the
> > property alias/link API, but maybe we could rearrange things so this
> > goes away.
> yes that would be nicer not to have to re-set the silicon rev of the 
> controllers of a SoC.
> 
> > 
> > > 
> > >  for the
> > > platform to initialize the soc. This is similar to what the rpi2 does,
> > > which goes one level in the aliasing.
> > Okay, maybe I'm barking up trees unnecessarily.
> No. your point on the SoC reuse is very valid. I try not to overspecify 
> too early but I agree I took a little shortcut. I will kick a v3 with
> the above. It should not be too much of a change.
> 
> > 
> > > 
> > > Then, at initialize time, the SCU 'silicon-rev' property value is read
> > > to initialize the SDMC controller. If we have more controllers in the 
> > > future needing 'silicon-rev,  we could follow the same pattern. Not 
> > > saying this is perfect. 
> > > 
> > > What I would have liked to do, is to link all the 'silicon-rev' do
> > > the SCU one. I did not find a way.
> > Yes, that would be nice. I did briefly poke around to see if there was
> > a solution to the link/alias issue but it seems not. 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > My thought was the silicon-rev value is reflective of the SoC
> > > > design rather than the other way around - but maybe that's splitting
> > > > hairs. 
> > > ah. is your concern about which object is holding the value ? If so,
> > > I thought that keeping it where it belongs on real HW was the better 
> > > option, that is in SCU, and then build from there.
> > No, that's not my concern, but I agree that it would not reflect the
> > hardware if there was a property on the SoC (i.e. there is nowhere
> > besides the SCU that the value is held).
> So I understand that the 'silicon-rev' location is not the biggest concern
> and we can keep it as it is in the patchset.

Yes, sounds good. Though with this approach we shouldn't need to fetch
the value out and poke it into the SDMC...

> Being able to alias the 
> different 'silicon-rev' properties to a common one would be nice though.

... which means we shouldn't need this behaviour. But it sounds nice
all-the-same.

>  
> > 
> > > 
> > > > 
> > > > It would also be trading off a bit of ugliness in this patch for
> > > > potential bugs if the properties get out of sync.
> > > This is the exact the purpose of the patch ! I failed to make it feel
> > > that way :)
> > Right. I think we need another layer of abstraction, essentially a soc
> > configuration struct that is accessed by what are currently the
> > ast2400_{init,realise}() functions. This will capture differences like
> > changes in IO addresses, changes to IP behaviour, the CPU types and
> > ultimately the silicon-rev value. What is now ast2400_init() and
> > ast2400_realise() can become generic aspeed_soc_{init,realise}(), and
> > then we wrap calls to this up in SoC-specific
> > ast2{4,5}00_{init,realise}() where we set the configuration struct. It
> > is a bit more work but I think the result would better reflect the
> > hardware and avoid introducing what feel to me like layering
> > violations.
> > 
> > Thoughts?
> Looks good. However I don't think there is no need for init() and realize() 
> ops right now. A .class_data should be sufficient. 

Right - we should do what is elegant. I didn't poke too deeply, I was
just sketching out something that I thought would demonstrate the
layering.

Cheers,

Andrew

> see this patch [1]. I still 
> need to rework that i2c patch btw.  
> 
> We can rework the SoC realize() if the need arise.
> 
> Cheers,
> 
> C.
> 
> [1] https://patchwork.kernel.org/patch/9129887/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 2/6] palmetto-bmc: replace palmetto_bmc with aspeed

[Cédric Le Goater]

This is mostly a name replacement to prepare ground for other socs
specificities. It also adds a specific TypeInfo struct for the
palmetto_bmc board with a custom initialization for the same reason.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 Should we change the name of the file to aspeed.c ? I am not found of
 such renames as it is then difficult to track code changes.

 hw/arm/palmetto-bmc.c | 54 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
index 1ee13d578899..f80a15733864 100644
--- a/hw/arm/palmetto-bmc.c
+++ b/hw/arm/palmetto-bmc.c
@@ -21,19 +21,19 @@
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 
-static struct arm_boot_info palmetto_bmc_binfo = {
+static struct arm_boot_info aspeed_binfo = {
     .loader_start = AST2400_SDRAM_BASE,
     .board_id = 0,
     .nb_cpus = 1,
 };
 
-typedef struct PalmettoBMCState {
+typedef struct AspeedBoardState {
     AST2400State soc;
     MemoryRegion ram;
-} PalmettoBMCState;
+} AspeedBoardState;
 
-static void palmetto_bmc_init_flashes(AspeedSMCState *s, const char *flashtype,
-                                      Error **errp)
+static void aspeed_init_flashes(AspeedSMCState *s, const char *flashtype,
+                                Error **errp)
 {
     int i ;
 
@@ -58,11 +58,11 @@ static void palmetto_bmc_init_flashes(AspeedSMCState *s, const char *flashtype,
     }
 }
 
-static void palmetto_bmc_init(MachineState *machine)
+static void aspeed_init(MachineState *machine)
 {
-    PalmettoBMCState *bmc;
+    AspeedBoardState *bmc;
 
-    bmc = g_new0(PalmettoBMCState, 1);
+    bmc = g_new0(AspeedBoardState, 1);
     object_initialize(&bmc->soc, (sizeof(bmc->soc)), TYPE_AST2400);
     object_property_add_child(OBJECT(machine), "soc", OBJECT(&bmc->soc),
                               &error_abort);
@@ -79,19 +79,26 @@ static void palmetto_bmc_init(MachineState *machine)
     object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
                              &error_abort);
 
-    palmetto_bmc_init_flashes(&bmc->soc.smc, "n25q256a", &error_abort);
-    palmetto_bmc_init_flashes(&bmc->soc.spi, "mx25l25635e", &error_abort);
+    aspeed_init_flashes(&bmc->soc.smc, "n25q256a", &error_abort);
+    aspeed_init_flashes(&bmc->soc.spi, "mx25l25635e", &error_abort);
+
+    aspeed_binfo.kernel_filename = machine->kernel_filename;
+    aspeed_binfo.initrd_filename = machine->initrd_filename;
+    aspeed_binfo.kernel_cmdline = machine->kernel_cmdline;
+    aspeed_binfo.ram_size = ram_size;
+    arm_load_kernel(ARM_CPU(first_cpu), &aspeed_binfo);
+}
 
-    palmetto_bmc_binfo.kernel_filename = machine->kernel_filename;
-    palmetto_bmc_binfo.initrd_filename = machine->initrd_filename;
-    palmetto_bmc_binfo.kernel_cmdline = machine->kernel_cmdline;
-    palmetto_bmc_binfo.ram_size = ram_size;
-    arm_load_kernel(ARM_CPU(first_cpu), &palmetto_bmc_binfo);
+static void palmetto_bmc_init(MachineState *machine)
+{
+    aspeed_init(machine);
 }
 
-static void palmetto_bmc_machine_init(MachineClass *mc)
+static void palmetto_bmc_class_init(ObjectClass *oc, void *data)
 {
-    mc->desc = "OpenPOWER Palmetto BMC";
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->desc = "OpenPOWER Palmetto BMC (ARM926EJ-S)";
     mc->init = palmetto_bmc_init;
     mc->max_cpus = 1;
     mc->no_sdcard = 1;
@@ -101,4 +108,15 @@ static void palmetto_bmc_machine_init(MachineClass *mc)
     mc->no_parallel = 1;
 }
 
-DEFINE_MACHINE("palmetto-bmc", palmetto_bmc_machine_init);
+static const TypeInfo palmetto_bmc_type = {
+    .name = MACHINE_TYPE_NAME("palmetto-bmc"),
+    .parent = TYPE_MACHINE,
+    .class_init = palmetto_bmc_class_init,
+};
+
+static void aspeed_machine_init(void)
+{
+    type_register_static(&palmetto_bmc_type);
+}
+
+type_init(aspeed_machine_init)
-- 
2.1.4

Re: [Qemu-devel] [PATCH 2/6] palmetto-bmc: replace palmetto_bmc with aspeed

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 5080 bytes --]

On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
> This is mostly a name replacement to prepare ground for other socs
> specificities. It also adds a specific TypeInfo struct for the
> palmetto_bmc board with a custom initialization for the same reason.

I think we should rename the file, it feels a bit confusing having the
ast2500 machine glue (added later in the series) in palmetto-bmc.c. You
mentioned in the cover letter that moving it would break history but it
isn't necessarily so, you can follow renames in the logs with `git log
--follow`. It's a git switch that feels like it should be a default but
isn't :/

Maybe create a commit that renames the file, then add these changes
after?

Andrew

> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  Should we change the name of the file to aspeed.c ? I am not found of
>  such renames as it is then difficult to track code changes.
> 
>  hw/arm/palmetto-bmc.c | 54 ++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
> index 1ee13d578899..f80a15733864 100644
> --- a/hw/arm/palmetto-bmc.c
> +++ b/hw/arm/palmetto-bmc.c
> @@ -21,19 +21,19 @@
>  #include "sysemu/block-backend.h"
>  #include "sysemu/blockdev.h"
>  
> -static struct arm_boot_info palmetto_bmc_binfo = {
> +static struct arm_boot_info aspeed_binfo = {
>      .loader_start = AST2400_SDRAM_BASE,
>      .board_id = 0,
>      .nb_cpus = 1,
>  };
>  
> -typedef struct PalmettoBMCState {
> +typedef struct AspeedBoardState {
>      AST2400State soc;
>      MemoryRegion ram;
> -} PalmettoBMCState;
> +} AspeedBoardState;
>  
> -static void palmetto_bmc_init_flashes(AspeedSMCState *s, const char *flashtype,
> -                                      Error **errp)
> +static void aspeed_init_flashes(AspeedSMCState *s, const char *flashtype,
> +                                Error **errp)
>  {
>      int i ;
>  
> @@ -58,11 +58,11 @@ static void palmetto_bmc_init_flashes(AspeedSMCState *s, const char *flashtype,
>      }
>  }
>  
> -static void palmetto_bmc_init(MachineState *machine)
> +static void aspeed_init(MachineState *machine)
>  {
> -    PalmettoBMCState *bmc;
> +    AspeedBoardState *bmc;
>  
> -    bmc = g_new0(PalmettoBMCState, 1);
> +    bmc = g_new0(AspeedBoardState, 1);
>      object_initialize(&bmc->soc, (sizeof(bmc->soc)), TYPE_AST2400);
>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&bmc->soc),
>                                &error_abort);
> @@ -79,19 +79,26 @@ static void palmetto_bmc_init(MachineState *machine)
>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>                               &error_abort);
>  
> -    palmetto_bmc_init_flashes(&bmc->soc.smc, "n25q256a", &error_abort);
> -    palmetto_bmc_init_flashes(&bmc->soc.spi, "mx25l25635e", &error_abort);
> +    aspeed_init_flashes(&bmc->soc.smc, "n25q256a", &error_abort);
> +    aspeed_init_flashes(&bmc->soc.spi, "mx25l25635e", &error_abort);
> +
> +    aspeed_binfo.kernel_filename = machine->kernel_filename;
> +    aspeed_binfo.initrd_filename = machine->initrd_filename;
> +    aspeed_binfo.kernel_cmdline = machine->kernel_cmdline;
> +    aspeed_binfo.ram_size = ram_size;
> +    arm_load_kernel(ARM_CPU(first_cpu), &aspeed_binfo);
> +}
>  
> -    palmetto_bmc_binfo.kernel_filename = machine->kernel_filename;
> -    palmetto_bmc_binfo.initrd_filename = machine->initrd_filename;
> -    palmetto_bmc_binfo.kernel_cmdline = machine->kernel_cmdline;
> -    palmetto_bmc_binfo.ram_size = ram_size;
> -    arm_load_kernel(ARM_CPU(first_cpu), &palmetto_bmc_binfo);
> +static void palmetto_bmc_init(MachineState *machine)
> +{
> +    aspeed_init(machine);
>  }
>  
> -static void palmetto_bmc_machine_init(MachineClass *mc)
> +static void palmetto_bmc_class_init(ObjectClass *oc, void *data)
>  {
> -    mc->desc = "OpenPOWER Palmetto BMC";
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    mc->desc = "OpenPOWER Palmetto BMC (ARM926EJ-S)";
>      mc->init = palmetto_bmc_init;
>      mc->max_cpus = 1;
>      mc->no_sdcard = 1;
> @@ -101,4 +108,15 @@ static void palmetto_bmc_machine_init(MachineClass *mc)
>      mc->no_parallel = 1;
>  }
>  
> -DEFINE_MACHINE("palmetto-bmc", palmetto_bmc_machine_init);
> +static const TypeInfo palmetto_bmc_type = {
> +    .name = MACHINE_TYPE_NAME("palmetto-bmc"),
> +    .parent = TYPE_MACHINE,
> +    .class_init = palmetto_bmc_class_init,
> +};
> +
> +static void aspeed_machine_init(void)
> +{
> +    type_register_static(&palmetto_bmc_type);
> +}
> +
> +type_init(aspeed_machine_init)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/6] palmetto-bmc: replace palmetto_bmc with aspeed

[Cédric Le Goater]

On 07/28/2016 06:48 AM, Andrew Jeffery wrote:
> On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
>> This is mostly a name replacement to prepare ground for other socs
>> specificities. It also adds a specific TypeInfo struct for the
>> palmetto_bmc board with a custom initialization for the same reason.
> 
> I think we should rename the file, it feels a bit confusing having the
> ast2500 machine glue (added later in the series) in palmetto-bmc.c. You
> mentioned in the cover letter that moving it would break history but it
> isn't necessarily so, you can follow renames in the logs with `git log
> --follow`. It's a git switch that feels like it should be a default but
> isn't :/

Ah. nice option indeed :) I was not aware of it.

> Maybe create a commit that renames the file, then add these changes
> after?

yes. rename then add changes, I will do that in v2.

Thanks,

C. 

> Andrew
> 
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  Should we change the name of the file to aspeed.c ? I am not found of
>>  such renames as it is then difficult to track code changes.
>>
>>  hw/arm/palmetto-bmc.c | 54 ++++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 36 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
>> index 1ee13d578899..f80a15733864 100644
>> --- a/hw/arm/palmetto-bmc.c
>> +++ b/hw/arm/palmetto-bmc.c
>> @@ -21,19 +21,19 @@
>>  #include "sysemu/block-backend.h"
>>  #include "sysemu/blockdev.h"
>>  
>> -static struct arm_boot_info palmetto_bmc_binfo = {
>> +static struct arm_boot_info aspeed_binfo = {
>>      .loader_start = AST2400_SDRAM_BASE,
>>      .board_id = 0,
>>      .nb_cpus = 1,
>>  };
>>  
>> -typedef struct PalmettoBMCState {
>> +typedef struct AspeedBoardState {
>>      AST2400State soc;
>>      MemoryRegion ram;
>> -} PalmettoBMCState;
>> +} AspeedBoardState;
>>  
>> -static void palmetto_bmc_init_flashes(AspeedSMCState *s, const char *flashtype,
>> -                                      Error **errp)
>> +static void aspeed_init_flashes(AspeedSMCState *s, const char *flashtype,
>> +                                Error **errp)
>>  {
>>      int i ;
>>  
>> @@ -58,11 +58,11 @@ static void palmetto_bmc_init_flashes(AspeedSMCState *s, const char *flashtype,
>>      }
>>  }
>>  
>> -static void palmetto_bmc_init(MachineState *machine)
>> +static void aspeed_init(MachineState *machine)
>>  {
>> -    PalmettoBMCState *bmc;
>> +    AspeedBoardState *bmc;
>>  
>> -    bmc = g_new0(PalmettoBMCState, 1);
>> +    bmc = g_new0(AspeedBoardState, 1);
>>      object_initialize(&bmc->soc, (sizeof(bmc->soc)), TYPE_AST2400);
>>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&bmc->soc),
>>                                &error_abort);
>> @@ -79,19 +79,26 @@ static void palmetto_bmc_init(MachineState *machine)
>>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>>                               &error_abort);
>>  
>> -    palmetto_bmc_init_flashes(&bmc->soc.smc, "n25q256a", &error_abort);
>> -    palmetto_bmc_init_flashes(&bmc->soc.spi, "mx25l25635e", &error_abort);
>> +    aspeed_init_flashes(&bmc->soc.smc, "n25q256a", &error_abort);
>> +    aspeed_init_flashes(&bmc->soc.spi, "mx25l25635e", &error_abort);
>> +
>> +    aspeed_binfo.kernel_filename = machine->kernel_filename;
>> +    aspeed_binfo.initrd_filename = machine->initrd_filename;
>> +    aspeed_binfo.kernel_cmdline = machine->kernel_cmdline;
>> +    aspeed_binfo.ram_size = ram_size;
>> +    arm_load_kernel(ARM_CPU(first_cpu), &aspeed_binfo);
>> +}
>>  
>> -    palmetto_bmc_binfo.kernel_filename = machine->kernel_filename;
>> -    palmetto_bmc_binfo.initrd_filename = machine->initrd_filename;
>> -    palmetto_bmc_binfo.kernel_cmdline = machine->kernel_cmdline;
>> -    palmetto_bmc_binfo.ram_size = ram_size;
>> -    arm_load_kernel(ARM_CPU(first_cpu), &palmetto_bmc_binfo);
>> +static void palmetto_bmc_init(MachineState *machine)
>> +{
>> +    aspeed_init(machine);
>>  }
>>  
>> -static void palmetto_bmc_machine_init(MachineClass *mc)
>> +static void palmetto_bmc_class_init(ObjectClass *oc, void *data)
>>  {
>> -    mc->desc = "OpenPOWER Palmetto BMC";
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +
>> +    mc->desc = "OpenPOWER Palmetto BMC (ARM926EJ-S)";
>>      mc->init = palmetto_bmc_init;
>>      mc->max_cpus = 1;
>>      mc->no_sdcard = 1;
>> @@ -101,4 +108,15 @@ static void palmetto_bmc_machine_init(MachineClass *mc)
>>      mc->no_parallel = 1;
>>  }
>>  
>> -DEFINE_MACHINE("palmetto-bmc", palmetto_bmc_machine_init);
>> +static const TypeInfo palmetto_bmc_type = {
>> +    .name = MACHINE_TYPE_NAME("palmetto-bmc"),
>> +    .parent = TYPE_MACHINE,
>> +    .class_init = palmetto_bmc_class_init,
>> +};
>> +
>> +static void aspeed_machine_init(void)
>> +{
>> +    type_register_static(&palmetto_bmc_type);
>> +}
>> +
>> +type_init(aspeed_machine_init)

[Qemu-devel] [PATCH 3/6] ast2400: use machine cpu_model to initialize the soc cpu

[Cédric Le Goater]

It will be easier to specify a different cpu for other soc derived
from the ast2400 soc.

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

diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
index fa535065f765..7f3517a2c6c6 100644
--- a/hw/arm/ast2400.c
+++ b/hw/arm/ast2400.c
@@ -15,6 +15,7 @@
 #include "qemu-common.h"
 #include "cpu.h"
 #include "exec/address-spaces.h"
+#include "hw/boards.h"
 #include "hw/arm/ast2400.h"
 #include "hw/char/serial.h"
 #include "qemu/log.h"
@@ -65,9 +66,14 @@ static const MemoryRegionOps ast2400_io_ops = {
 
 static void ast2400_init(Object *obj)
 {
+    const char *cpu_model = current_machine->cpu_model;
     AST2400State *s = AST2400(obj);
 
-    s->cpu = cpu_arm_init("arm926");
+    if (!cpu_model) {
+        cpu_model = "arm926";
+    }
+
+    s->cpu = cpu_arm_init(cpu_model);
 
     object_initialize(&s->vic, sizeof(s->vic), TYPE_ASPEED_VIC);
     object_property_add_child(obj, "vic", OBJECT(&s->vic), NULL);
diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
index f80a15733864..8a3ff5568575 100644
--- a/hw/arm/palmetto-bmc.c
+++ b/hw/arm/palmetto-bmc.c
@@ -91,6 +91,7 @@ static void aspeed_init(MachineState *machine)
 
 static void palmetto_bmc_init(MachineState *machine)
 {
+    machine->cpu_model = "arm926";
     aspeed_init(machine);
 }
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 3/6] ast2400: use machine cpu_model to initialize the soc cpu

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2250 bytes --]

On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
> It will be easier to specify a different cpu for other soc derived
> from the ast2400 soc.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/arm/ast2400.c      | 8 +++++++-
>  hw/arm/palmetto-bmc.c | 1 +
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
> index fa535065f765..7f3517a2c6c6 100644
> --- a/hw/arm/ast2400.c
> +++ b/hw/arm/ast2400.c
> @@ -15,6 +15,7 @@
>  #include "qemu-common.h"
>  #include "cpu.h"
>  #include "exec/address-spaces.h"
> +#include "hw/boards.h"
>  #include "hw/arm/ast2400.h"
>  #include "hw/char/serial.h"
>  #include "qemu/log.h"
> @@ -65,9 +66,14 @@ static const MemoryRegionOps ast2400_io_ops = {
>  
>  static void ast2400_init(Object *obj)
>  {
> +    const char *cpu_model = current_machine->cpu_model;
>      AST2400State *s = AST2400(obj);
>  
> -    s->cpu = cpu_arm_init("arm926");
> +    if (!cpu_model) {
> +        cpu_model = "arm926";
> +    }
> +
> +    s->cpu = cpu_arm_init(cpu_model);

I did a similar thing in the series introducing the AST2400 SoC, and
Peter had a comment on the approach[1]:

    What we do now is not let the user override the cpu model at all;
    presumably this SoC only ever has an ARM926 and it doesn't make
    any sense to have some frankenstein "this SoC but with a different
    CPU in it" config.

Given this is the ast2400_init() it looks to me like we should be
hardwiring the CPU rather than leaving it to the machine to define.

[1] https://patchwork.kernel.org/patch/8325651/ 

>  
>      object_initialize(&s->vic, sizeof(s->vic), TYPE_ASPEED_VIC);
>      object_property_add_child(obj, "vic", OBJECT(&s->vic), NULL);
> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
> index f80a15733864..8a3ff5568575 100644
> --- a/hw/arm/palmetto-bmc.c
> +++ b/hw/arm/palmetto-bmc.c
> @@ -91,6 +91,7 @@ static void aspeed_init(MachineState *machine)
>  
>  static void palmetto_bmc_init(MachineState *machine)
>  {
> +    machine->cpu_model = "arm926";
>      aspeed_init(machine);
>  }
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 3/6] ast2400: use machine cpu_model to initialize the soc cpu

[Cédric Le Goater]

On 07/28/2016 04:37 AM, Andrew Jeffery wrote:
> I did a similar thing in the series introducing the AST2400 SoC, and
> Peter had a comment on the approach[1]:
> 
>     What we do now is not let the user override the cpu model at all;
>     presumably this SoC only ever has an ARM926 and it doesn't make
>     any sense to have some frankenstein "this SoC but with a different
>     CPU in it" config.
> 
> Given this is the ast2400_init() it looks to me like we should be
> hardwiring the CPU rather than leaving it to the machine to define.

ok. so if we consider that the platform did the setting, we can reduce 
the patch to :

-    s->cpu = cpu_arm_init("arm926");
+    s->cpu = cpu_arm_init(current_machine->cpu_model);

Cheers,

C.

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

[Cédric Le Goater]

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

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 },
+};
+
 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)
 {
     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)
-- 
2.1.4

Re: [Qemu-devel] [PATCH 4/6] palmetto-bmc: add board specific configuration

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4467 bytes --]

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)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 4/6] palmetto-bmc: add board specific configuration

[Cédric Le Goater]

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)

[Qemu-devel] [PATCH 5/6] aspeed: add ast2500 support to scu and sdmc controllers

[Cédric Le Goater]

Based on previous work done by Andrew Jeffery <andrew@aj.id.au>.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/misc/aspeed_scu.c         | 45 +++++++++++++++++++++++++++++++++++++++++++-
 hw/misc/aspeed_sdmc.c        |  1 +
 include/hw/misc/aspeed_scu.h |  1 +
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index c7e2c8263f55..6dd7e1085420 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -120,6 +120,41 @@ static const uint32_t ast2400_a0_resets[ASPEED_SCU_NR_REGS] = {
      [BMC_DEV_ID]      = 0x00002402U
 };
 
+/* SCU70 bit 23: 0 24Mhz. bit 11:9: 0b001 AXI:ABH ratio 2:1 */
+/* AST2500 revision A1 */
+
+static const uint32_t ast2500_a1_resets[ASPEED_SCU_NR_REGS] = {
+     [SYS_RST_CTRL]    = 0xFFCFFEDCU,
+     [CLK_SEL]         = 0xF3F40000U,
+     [CLK_STOP_CTRL]   = 0x19FC3E8BU,
+     [D2PLL_PARAM]     = 0x00026108U,
+     [MPLL_PARAM]      = 0x00030291U,
+     [HPLL_PARAM]      = 0x93000400U,
+     [MISC_CTRL1]      = 0x00000010U,
+     [PCI_CTRL1]       = 0x20001A03U,
+     [PCI_CTRL2]       = 0x20001A03U,
+     [PCI_CTRL3]       = 0x04000030U,
+     [SYS_RST_STATUS]  = 0x00000001U,
+     [SOC_SCRATCH1]    = 0x000000C0U, /* SoC completed DRAM init */
+     [MISC_CTRL2]      = 0x00000023U,
+     [RNG_CTRL]        = 0x0000000EU,
+     [PINMUX_CTRL2]    = 0x0000F000U,
+     [PINMUX_CTRL3]    = 0x03000000U,
+     [PINMUX_CTRL4]    = 0x00000000U,
+     [PINMUX_CTRL5]    = 0x0000A000U,
+     [WDT_RST_CTRL]    = 0x023FFFF3U,
+     [PINMUX_CTRL8]    = 0xFFFF0000U,
+     [PINMUX_CTRL9]    = 0x000FFFFFU,
+     [FREE_CNTR4]      = 0x000000FFU,
+     [FREE_CNTR4_EXT]  = 0x000000FFU,
+     [CPU2_BASE_SEG1]  = 0x80000000U,
+     [CPU2_BASE_SEG4]  = 0x1E600000U,
+     [CPU2_BASE_SEG5]  = 0xC0000000U,
+     [UART_HPLL_CLK]   = 0x00001903U,
+     [PCIE_CTRL]       = 0x0000007BU,
+     [BMC_DEV_ID]      = 0x00002402U
+};
+
 static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size)
 {
     AspeedSCUState *s = ASPEED_SCU(opaque);
@@ -198,6 +233,10 @@ static void aspeed_scu_reset(DeviceState *dev)
     case AST2400_A0_SILICON_REV:
         reset = ast2400_a0_resets;
         break;
+    case AST2500_A0_SILICON_REV:
+    case AST2500_A1_SILICON_REV:
+        reset = ast2500_a1_resets;
+        break;
     default:
         g_assert_not_reached();
     }
@@ -208,7 +247,11 @@ static void aspeed_scu_reset(DeviceState *dev)
     s->regs[HW_STRAP2] = s->hw_strap2;
 }
 
-static uint32_t aspeed_silicon_revs[] = { AST2400_A0_SILICON_REV, };
+static uint32_t aspeed_silicon_revs[] = {
+    AST2400_A0_SILICON_REV,
+    AST2500_A0_SILICON_REV,
+    AST2500_A1_SILICON_REV
+};
 
 bool is_supported_silicon_rev(uint32_t silicon_rev)
 {
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 6cc0301a6331..621d166890fa 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -196,6 +196,7 @@ static void aspeed_sdmc_reset(DeviceState *dev)
         break;
 
     case AST2500_A0_SILICON_REV:
+    case AST2500_A1_SILICON_REV:
         s->regs[R_CONF] |=
             ASPEED_SDMC_HW_VERSION(1) |
             ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h
index fdfd982288f2..e2e4d1864e34 100644
--- a/include/hw/misc/aspeed_scu.h
+++ b/include/hw/misc/aspeed_scu.h
@@ -33,6 +33,7 @@ typedef struct AspeedSCUState {
 
 #define AST2400_A0_SILICON_REV   0x02000303U
 #define AST2500_A0_SILICON_REV   0x04000303U
+#define AST2500_A1_SILICON_REV   0x04010303U
 
 extern bool is_supported_silicon_rev(uint32_t silicon_rev);
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 5/6] aspeed: add ast2500 support to scu and sdmc controllers

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4431 bytes --]

On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
> Based on previous work done by Andrew Jeffery <andrew@aj.id.au>.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  hw/misc/aspeed_scu.c         | 45 +++++++++++++++++++++++++++++++++++++++++++-
>  hw/misc/aspeed_sdmc.c        |  1 +
>  include/hw/misc/aspeed_scu.h |  1 +
>  3 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index c7e2c8263f55..6dd7e1085420 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -120,6 +120,41 @@ static const uint32_t ast2400_a0_resets[ASPEED_SCU_NR_REGS] = {
>       [BMC_DEV_ID]      = 0x00002402U
>  };
>  
> +/* SCU70 bit 23: 0 24Mhz. bit 11:9: 0b001 AXI:ABH ratio 2:1 */
> +/* AST2500 revision A1 */
> +
> +static const uint32_t ast2500_a1_resets[ASPEED_SCU_NR_REGS] = {
> +     [SYS_RST_CTRL]    = 0xFFCFFEDCU,
> +     [CLK_SEL]         = 0xF3F40000U,
> +     [CLK_STOP_CTRL]   = 0x19FC3E8BU,
> +     [D2PLL_PARAM]     = 0x00026108U,
> +     [MPLL_PARAM]      = 0x00030291U,
> +     [HPLL_PARAM]      = 0x93000400U,
> +     [MISC_CTRL1]      = 0x00000010U,
> +     [PCI_CTRL1]       = 0x20001A03U,
> +     [PCI_CTRL2]       = 0x20001A03U,
> +     [PCI_CTRL3]       = 0x04000030U,
> +     [SYS_RST_STATUS]  = 0x00000001U,
> +     [SOC_SCRATCH1]    = 0x000000C0U, /* SoC completed DRAM init */
> +     [MISC_CTRL2]      = 0x00000023U,
> +     [RNG_CTRL]        = 0x0000000EU,
> +     [PINMUX_CTRL2]    = 0x0000F000U,
> +     [PINMUX_CTRL3]    = 0x03000000U,
> +     [PINMUX_CTRL4]    = 0x00000000U,
> +     [PINMUX_CTRL5]    = 0x0000A000U,
> +     [WDT_RST_CTRL]    = 0x023FFFF3U,
> +     [PINMUX_CTRL8]    = 0xFFFF0000U,
> +     [PINMUX_CTRL9]    = 0x000FFFFFU,
> +     [FREE_CNTR4]      = 0x000000FFU,
> +     [FREE_CNTR4_EXT]  = 0x000000FFU,
> +     [CPU2_BASE_SEG1]  = 0x80000000U,
> +     [CPU2_BASE_SEG4]  = 0x1E600000U,
> +     [CPU2_BASE_SEG5]  = 0xC0000000U,
> +     [UART_HPLL_CLK]   = 0x00001903U,
> +     [PCIE_CTRL]       = 0x0000007BU,
> +     [BMC_DEV_ID]      = 0x00002402U
> +};
> +
>  static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      AspeedSCUState *s = ASPEED_SCU(opaque);
> @@ -198,6 +233,10 @@ static void aspeed_scu_reset(DeviceState *dev)
>      case AST2400_A0_SILICON_REV:
>          reset = ast2400_a0_resets;
>          break;
> +    case AST2500_A0_SILICON_REV:
> +    case AST2500_A1_SILICON_REV:
> +        reset = ast2500_a1_resets;
> +        break;
>      default:
>          g_assert_not_reached();
>      }
> @@ -208,7 +247,11 @@ static void aspeed_scu_reset(DeviceState *dev)
>      s->regs[HW_STRAP2] = s->hw_strap2;
>  }
>  
> -static uint32_t aspeed_silicon_revs[] = { AST2400_A0_SILICON_REV, };
> +static uint32_t aspeed_silicon_revs[] = {
> +    AST2400_A0_SILICON_REV,
> +    AST2500_A0_SILICON_REV,
> +    AST2500_A1_SILICON_REV
> +};
>  
>  bool is_supported_silicon_rev(uint32_t silicon_rev)
>  {
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 6cc0301a6331..621d166890fa 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -196,6 +196,7 @@ static void aspeed_sdmc_reset(DeviceState *dev)
>          break;
>  
>      case AST2500_A0_SILICON_REV:
> +    case AST2500_A1_SILICON_REV:
>          s->regs[R_CONF] |=
>              ASPEED_SDMC_HW_VERSION(1) |
>              ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h
> index fdfd982288f2..e2e4d1864e34 100644
> --- a/include/hw/misc/aspeed_scu.h
> +++ b/include/hw/misc/aspeed_scu.h
> @@ -33,6 +33,7 @@ typedef struct AspeedSCUState {
>  
>  #define AST2400_A0_SILICON_REV   0x02000303U
>  #define AST2500_A0_SILICON_REV   0x04000303U
> +#define AST2500_A1_SILICON_REV   0x04010303U
>  
>  extern bool is_supported_silicon_rev(uint32_t silicon_rev);
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 6/6] arm: add support for an ast2500 evaluation board

[Cédric Le Goater]

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
 };
 
 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 */
-- 
2.1.4

Re: [Qemu-devel] [PATCH 6/6] arm: add support for an ast2500 evaluation board

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 3271 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 6/6] arm: add support for an ast2500 evaluation board

[Cédric Le Goater]

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

Re: [Qemu-devel] [PATCH 6/6] arm: add support for an ast2500 evaluation board

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 6/6] arm: add support for an ast2500 evaluation board

[Cédric Le Goater]

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.

Re: [Qemu-devel] [PATCH 6/6] arm: add support for an ast2500 evaluation board

[Cédric Le Goater]

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.