| Message ID | 20230103181646.55711-14-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Toward class init of cpu features | expand |
On 3/1/23 19:16, Richard Henderson wrote: > There was even a TODO comment that we ought to be using a cpu > property, but we failed to update when the property was added. > Use ARM_AFF1_SHIFT instead of the bare constant 8. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > hw/arm/bcm2836.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c > index 24354338ca..abbb3689d0 100644 > --- a/hw/arm/bcm2836.c > +++ b/hw/arm/bcm2836.c > @@ -130,8 +130,11 @@ static void bcm2836_realize(DeviceState *dev, Error **errp) > qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-fiq", 0)); > > for (n = 0; n < BCM283X_NCPUS; n++) { > - /* TODO: this should be converted to a property of ARM_CPU */ > - s->cpu[n].core.mp_affinity = (bc->clusterid << 8) | n; > + if (!object_property_set_int(OBJECT(&s->cpu[n].core), "mp-affinity", > + (bc->clusterid << ARM_AFF1_SHIFT) | n, > + errp)) { > + return; > + } > > /* set periphbase/CBAR value for CPU-local registers */ > if (!object_property_set_int(OBJECT(&s->cpu[n].core), "reset-cbar", Eh I have almost the same patch locally: -- >8 -- $ git show 5f675655c844154f3760967296e82adf5d8d7c24 diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c index 24354338ca..6f964a3b31 100644 --- a/hw/arm/bcm2836.c +++ b/hw/arm/bcm2836.c @@ -130,8 +130,8 @@ static void bcm2836_realize(DeviceState *dev, Error **errp) qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-fiq", 0)); for (n = 0; n < BCM283X_NCPUS; n++) { - /* TODO: this should be converted to a property of ARM_CPU */ - s->cpu[n].core.mp_affinity = (bc->clusterid << 8) | n; + object_property_set_int(OBJECT(&s->cpu[n].core), "mp-affinity", + (bc->clusterid << 8) | n, &error_abort); /* set periphbase/CBAR value for CPU-local registers */ if (!object_property_set_int(OBJECT(&s->cpu[n].core), "reset-cbar", --- Yours is better (ARM_AFF1_SHIFT & checks return value). Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 5/1/23 22:48, Philippe Mathieu-Daudé wrote: > On 3/1/23 19:16, Richard Henderson wrote: >> There was even a TODO comment that we ought to be using a cpu >> property, but we failed to update when the property was added. >> Use ARM_AFF1_SHIFT instead of the bare constant 8. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> hw/arm/bcm2836.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c >> index 24354338ca..abbb3689d0 100644 >> --- a/hw/arm/bcm2836.c >> +++ b/hw/arm/bcm2836.c >> @@ -130,8 +130,11 @@ static void bcm2836_realize(DeviceState *dev, >> Error **errp) >> qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-fiq", 0)); >> for (n = 0; n < BCM283X_NCPUS; n++) { >> - /* TODO: this should be converted to a property of ARM_CPU */ >> - s->cpu[n].core.mp_affinity = (bc->clusterid << 8) | n; >> + if (!object_property_set_int(OBJECT(&s->cpu[n].core), >> "mp-affinity", >> + (bc->clusterid << >> ARM_AFF1_SHIFT) | n, >> + errp)) { >> + return; >> + } > Eh I have almost the same patch locally: > Yours is better (ARM_AFF1_SHIFT & checks return value). Cherry-picking your patch I had to add "target/arm/cpu-qom.h" to avoid: ../hw/arm/bcm2836.c:146:56: error: use of undeclared identifier 'ARM_AFF1_SHIFT' (bc->clusterid << ARM_AFF1_SHIFT) | n, ^ This definition is not QOM related, I guess I'll move it to "hw/arm/cpu-defs.h" along with ARM_CPU_vIRQ/FIQ and GTIMER* definitions from "cpu.h".
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c index 24354338ca..abbb3689d0 100644 --- a/hw/arm/bcm2836.c +++ b/hw/arm/bcm2836.c @@ -130,8 +130,11 @@ static void bcm2836_realize(DeviceState *dev, Error **errp) qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-fiq", 0)); for (n = 0; n < BCM283X_NCPUS; n++) { - /* TODO: this should be converted to a property of ARM_CPU */ - s->cpu[n].core.mp_affinity = (bc->clusterid << 8) | n; + if (!object_property_set_int(OBJECT(&s->cpu[n].core), "mp-affinity", + (bc->clusterid << ARM_AFF1_SHIFT) | n, + errp)) { + return; + } /* set periphbase/CBAR value for CPU-local registers */ if (!object_property_set_int(OBJECT(&s->cpu[n].core), "reset-cbar",
There was even a TODO comment that we ought to be using a cpu property, but we failed to update when the property was added. Use ARM_AFF1_SHIFT instead of the bare constant 8. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- hw/arm/bcm2836.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)