| Message ID | 1469638018-17590-2-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: > 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); >
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); >>
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
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/
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/
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);
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(-)