| Message ID | 2de138a60e612a72f39dce25df726bddf9a6d38c.1389656647.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jan 14, 2014 at 9:48 AM, Alistair Francis <alistair.francis@xilinx.com> wrote: > Convert the MIDR register to a property. This allows boards to later set > a custom MIDR value. This has been done in such a way to maintain > compatibility with all existing CPUs and boards > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > I originally added the properties to the cpu->midr variable in a similar > method to how Peter Crosthwaite did in his 'Fix Support for ARM CBAR and > reset-hivecs' series. > V2: Use dc->props to avoid using qdev_* > > target-arm/cpu.c | 13 +++++++++++++ > 1 files changed, 13 insertions(+), 0 deletions(-) > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 408d207..52f7f06 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -253,6 +253,7 @@ static void arm_cpu_post_init(Object *obj) > { > ARMCPU *cpu = ARM_CPU(obj); > Error *err = NULL; > + uint32_t temp = cpu->midr; > > if (arm_feature(&cpu->env, ARM_FEATURE_CBAR)) { > qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_cbar_property, > @@ -265,6 +266,17 @@ static void arm_cpu_post_init(Object *obj) > &err); > assert_no_error(err); > } > + > + /* > + * Initialise the midr property and set it to the original CPU MIDR > + * This is used to maintain compatibility with boards that don't set > + * a custom MIDR > + */ > + object_property_set_int(OBJECT(cpu), temp, "midr", &err); > + if (err) { > + error_report("%s", error_get_pretty(err)); > + exit(1); > + } Do you even need this now? The normal arrayified dc->props properties are added at device::init time. As TYPE_DEVICE is a parent class, its init function is called before CPUs (normal inits are called in parent->child order, the post_inits are reverse). This means the cpu::init fns should correctly set the device-specific default after device::init, trampling the bogus default defined in the property array. All of this however assumes that MIDR is unconditionally existent for all ARMCPU. Peter, are you able to confirm that thats ok before we commit to this implementation over the conditional post_init approach? Regards, Peter > } > > static void arm_cpu_finalizefn(Object *obj) > @@ -984,6 +996,7 @@ static const ARMCPUInfo arm_cpus[] = { > > static Property arm_cpu_properties[] = { > DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false), > + DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0), > DEFINE_PROP_END_OF_LIST() > }; > > -- > 1.7.1 > >
On 19 January 2014 00:59, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > Do you even need this now? The normal arrayified dc->props properties > are added at device::init time. As TYPE_DEVICE is a parent class, its > init function is called before CPUs (normal inits are called in > parent->child order, the post_inits are reverse). This means the > cpu::init fns should correctly set the device-specific default after > device::init, trampling the bogus default defined in the property > array. > > All of this however assumes that MIDR is unconditionally existent for > all ARMCPU. Peter, are you able to confirm that thats ok before we > commit to this implementation over the conditional post_init approach? IIRC ARMv4 and earlier didn't define the MIDR, but we don't actually emulate any of those. In general, my intent with all these constant fields in the ARMCPU struct was that we'd end up making them just properties available on all ARMCPU objects, and if the particular subtype of ARMCPU happened not to have FP and so didn't need the reset_fpsid, for example, it would just ignore whatever value you set the property to. I don't think we need to tie ourselves in knots to restrict the properties to particular CPUs if it is too implementationally awkward (though it would be nice if we can tie them to ARM_FEATURE_* bits for more or less free). thanks -- PMM
On Sun, Jan 19, 2014 at 11:12 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 January 2014 00:59, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> Do you even need this now? The normal arrayified dc->props properties >> are added at device::init time. As TYPE_DEVICE is a parent class, its >> init function is called before CPUs (normal inits are called in >> parent->child order, the post_inits are reverse). This means the >> cpu::init fns should correctly set the device-specific default after >> device::init, trampling the bogus default defined in the property >> array. >> >> All of this however assumes that MIDR is unconditionally existent for >> all ARMCPU. Peter, are you able to confirm that thats ok before we >> commit to this implementation over the conditional post_init approach? > > IIRC ARMv4 and earlier didn't define the MIDR, but we don't > actually emulate any of those. In general, my intent with all these > constant fields in the ARMCPU struct was that we'd end up making > them just properties available on all ARMCPU objects, and if the > particular subtype of ARMCPU happened not to have FP and so > didn't need the reset_fpsid, for example, it would just ignore whatever > value you set the property to. I don't think we need to tie ourselves > in knots to restrict the properties to particular CPUs if it is too > implementationally awkward (though it would be nice if we can > tie them to ARM_FEATURE_* bits for more or less free). > ARM_FEATURE_V5 exists. Will that do the job? That will exclude just the V4T AFAICT. Worth bothering? Regards, Peter > thanks > -- PMM >
On 19 January 2014 01:46, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sun, Jan 19, 2014 at 11:12 AM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> IIRC ARMv4 and earlier didn't define the MIDR, but we don't >> actually emulate any of those. In general, my intent with all these >> constant fields in the ARMCPU struct was that we'd end up making >> them just properties available on all ARMCPU objects, and if the >> particular subtype of ARMCPU happened not to have FP and so >> didn't need the reset_fpsid, for example, it would just ignore whatever >> value you set the property to. I don't think we need to tie ourselves >> in knots to restrict the properties to particular CPUs if it is too >> implementationally awkward (though it would be nice if we can >> tie them to ARM_FEATURE_* bits for more or less free). >> > > ARM_FEATURE_V5 exists. Will that do the job? That will exclude just > the V4T AFAICT. Worth bothering? Probably not for MIDR, I think, though maybe for some of the others we might like to. thanks -- PMM
On Sun, Jan 19, 2014 at 9:21 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 January 2014 01:46, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Sun, Jan 19, 2014 at 11:12 AM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >>> IIRC ARMv4 and earlier didn't define the MIDR, but we don't >>> actually emulate any of those. In general, my intent with all these >>> constant fields in the ARMCPU struct was that we'd end up making >>> them just properties available on all ARMCPU objects, and if the >>> particular subtype of ARMCPU happened not to have FP and so >>> didn't need the reset_fpsid, for example, it would just ignore whatever >>> value you set the property to. I don't think we need to tie ourselves >>> in knots to restrict the properties to particular CPUs if it is too >>> implementationally awkward (though it would be nice if we can >>> tie them to ARM_FEATURE_* bits for more or less free). >>> >> >> ARM_FEATURE_V5 exists. Will that do the job? That will exclude just >> the V4T AFAICT. Worth bothering? > > Probably not for MIDR, I think, though maybe for some of the > others we might like to. I have removed the object_property_set_int() function as it is no longer required. I also didn't bother restricting the MIDR to ARMv5 > > thanks > -- PMM >
diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 408d207..52f7f06 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -253,6 +253,7 @@ static void arm_cpu_post_init(Object *obj) { ARMCPU *cpu = ARM_CPU(obj); Error *err = NULL; + uint32_t temp = cpu->midr; if (arm_feature(&cpu->env, ARM_FEATURE_CBAR)) { qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_cbar_property, @@ -265,6 +266,17 @@ static void arm_cpu_post_init(Object *obj) &err); assert_no_error(err); } + + /* + * Initialise the midr property and set it to the original CPU MIDR + * This is used to maintain compatibility with boards that don't set + * a custom MIDR + */ + object_property_set_int(OBJECT(cpu), temp, "midr", &err); + if (err) { + error_report("%s", error_get_pretty(err)); + exit(1); + } } static void arm_cpu_finalizefn(Object *obj) @@ -984,6 +996,7 @@ static const ARMCPUInfo arm_cpus[] = { static Property arm_cpu_properties[] = { DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false), + DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0), DEFINE_PROP_END_OF_LIST() };
Convert the MIDR register to a property. This allows boards to later set a custom MIDR value. This has been done in such a way to maintain compatibility with all existing CPUs and boards Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- I originally added the properties to the cpu->midr variable in a similar method to how Peter Crosthwaite did in his 'Fix Support for ARM CBAR and reset-hivecs' series. V2: Use dc->props to avoid using qdev_* target-arm/cpu.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)