| Message ID | e14467ff451d76d256e1be017621b68adb758406.1385542228.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 27 November 2013 09:01, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > The reset Value of the CP15 CBAR is a vendor (machine) configurable > property. Define arm_cpu_properties and add it. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > > target-arm/cpu.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index a82fa61..f1c5f6b 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -20,6 +20,7 @@ > > #include "cpu.h" > #include "qemu-common.h" > +#include "hw/qdev-properties.h" > #if !defined(CONFIG_USER_ONLY) > #include "hw/loader.h" > #endif > @@ -847,6 +848,11 @@ typedef struct ARMCPUInfo { > void (*class_init)(ObjectClass *oc, void *data); > } ARMCPUInfo; > > +static Property arm_cpu_properties[] = { > + DEFINE_PROP_UINT32("cbar", ARMCPU, reset_cbar, 0), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static const ARMCPUInfo arm_cpus[] = { > #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64) > { .name = "arm926", .initfn = arm926_initfn }, > @@ -895,6 +901,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) > > acc->parent_realize = dc->realize; > dc->realize = arm_cpu_realizefn; > + dc->props = arm_cpu_properties; > > acc->parent_reset = cc->reset; > cc->reset = arm_cpu_reset; Hmm. This means we end up with a cbar property on every ARM CPU whether that ARM CPU has a cbar or not... -- PMM
Am 27.11.2013 10:14, schrieb Peter Maydell: > On 27 November 2013 09:01, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> The reset Value of the CP15 CBAR is a vendor (machine) configurable >> property. Define arm_cpu_properties and add it. >> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> --- >> >> target-arm/cpu.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c >> index a82fa61..f1c5f6b 100644 >> --- a/target-arm/cpu.c >> +++ b/target-arm/cpu.c >> @@ -20,6 +20,7 @@ >> >> #include "cpu.h" >> #include "qemu-common.h" >> +#include "hw/qdev-properties.h" >> #if !defined(CONFIG_USER_ONLY) >> #include "hw/loader.h" >> #endif >> @@ -847,6 +848,11 @@ typedef struct ARMCPUInfo { >> void (*class_init)(ObjectClass *oc, void *data); >> } ARMCPUInfo; >> >> +static Property arm_cpu_properties[] = { >> + DEFINE_PROP_UINT32("cbar", ARMCPU, reset_cbar, 0), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> static const ARMCPUInfo arm_cpus[] = { >> #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64) >> { .name = "arm926", .initfn = arm926_initfn }, >> @@ -895,6 +901,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) >> >> acc->parent_realize = dc->realize; >> dc->realize = arm_cpu_realizefn; >> + dc->props = arm_cpu_properties; >> >> acc->parent_reset = cc->reset; >> cc->reset = arm_cpu_reset; > > Hmm. This means we end up with a cbar property on every > ARM CPU whether that ARM CPU has a cbar or not... If we turn it into a dynamic property, we could register it conditional to ARM_FEATURE_CBAR. The overall idea of the series makes sense to me. I would caution to think about the naming scheme though - we might want to have a "cbar" property to access the current value whereas we'll likely end up having several reset values to configure over time. Andreas
On 27 November 2013 09:42, Andreas Färber <afaerber@suse.de> wrote: > If we turn it into a dynamic property, we could register it conditional > to ARM_FEATURE_CBAR. Unfortunately feature flags only get set at realize (in the per-cpu init function), so we don't know at the point where we're registering properties whether to have this one or not. The other option would be to define an a9 cpu class init fn to put the property in. > The overall idea of the series makes sense to me. I would caution to > think about the naming scheme though - we might want to have a "cbar" > property to access the current value whereas we'll likely end up having > several reset values to configure over time. Mmm, maybe. The approach I think we've taken in some other places (eg the cache controller) is to have the property names match the silicon's config or control signal names (which in this case would be PERIPHBASE), but they are not always consistent from CPU to CPU, so that might be more confusing than helpful. I don't think I have a strong opinion here, so I'd be happy with any vaguely consistent-looking plan. -- PMM
Am 27.11.2013 11:15, schrieb Peter Maydell: > On 27 November 2013 09:42, Andreas Färber <afaerber@suse.de> wrote: >> If we turn it into a dynamic property, we could register it conditional >> to ARM_FEATURE_CBAR. > > Unfortunately feature flags only get set at realize (in the > per-cpu init function), so we don't know at the point where > we're registering properties whether to have this one or not. 1/6 sets it in instance_init actually. So instance_post_init might do. > The other option would be to define an a9 cpu class init fn to > put the property in. Is it A9-only or would A15, A7, A12, etc. also need it? >> The overall idea of the series makes sense to me. I would caution to >> think about the naming scheme though - we might want to have a "cbar" >> property to access the current value whereas we'll likely end up having >> several reset values to configure over time. > > Mmm, maybe. The approach I think we've taken in some other > places (eg the cache controller) is to have the property names > match the silicon's config or control signal names (which in > this case would be PERIPHBASE), but they are not always > consistent from CPU to CPU, so that might be more confusing > than helpful. I don't think I have a strong opinion here, so I'd > be happy with any vaguely consistent-looking plan. Whatever name you choose, I was rather thinking of whether you may want to call it, e.g., "foo-reset" or "reset-foo" to distinguish from plain "foo". (Igor's x86 properties series uses "feat-foo" on Anthony's suggestion to group and parse feature properties.) Andreas
On 27 November 2013 10:27, Andreas Färber <afaerber@suse.de> wrote: > Am 27.11.2013 11:15, schrieb Peter Maydell: >> On 27 November 2013 09:42, Andreas Färber <afaerber@suse.de> wrote: >>> If we turn it into a dynamic property, we could register it conditional >>> to ARM_FEATURE_CBAR. >> >> Unfortunately feature flags only get set at realize (in the >> per-cpu init function), so we don't know at the point where >> we're registering properties whether to have this one or not. > > 1/6 sets it in instance_init actually. So instance_post_init might do. Oh yes, was confusing init and realize. >> The other option would be to define an a9 cpu class init fn to >> put the property in. > > Is it A9-only or would A15, A7, A12, etc. also need it? A5, A7, A9, and A15 all have this -- basically all the cores with memory-mapped peripherals. It is still technically an IMPDEF register and config signal, though. -- PMM
Hi Andreas, On Wed, Nov 27, 2013 at 8:27 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 27.11.2013 11:15, schrieb Peter Maydell: >> On 27 November 2013 09:42, Andreas Färber <afaerber@suse.de> wrote: >>> If we turn it into a dynamic property, we could register it conditional >>> to ARM_FEATURE_CBAR. >> >> Unfortunately feature flags only get set at realize (in the >> per-cpu init function), so we don't know at the point where >> we're registering properties whether to have this one or not. > > 1/6 sets it in instance_init actually. So instance_post_init might do. > Do you have a candidate example of this pattern that I can work off? >> The other option would be to define an a9 cpu class init fn to >> put the property in. > > Is it A9-only or would A15, A7, A12, etc. also need it? > >>> The overall idea of the series makes sense to me. I would caution to >>> think about the naming scheme though - we might want to have a "cbar" >>> property to access the current value whereas we'll likely end up having >>> several reset values to configure over time. >> >> Mmm, maybe. The approach I think we've taken in some other >> places (eg the cache controller) is to have the property names >> match the silicon's config or control signal names (which in >> this case would be PERIPHBASE), but they are not always >> consistent from CPU to CPU, so that might be more confusing >> than helpful. I don't think I have a strong opinion here, so I'd >> be happy with any vaguely consistent-looking plan. > > Whatever name you choose, I was rather thinking of whether you may want > to call it, e.g., "foo-reset" or "reset-foo" to distinguish from plain > "foo". (Igor's x86 properties series uses "feat-foo" on Anthony's > suggestion to group and parse feature properties.) > Is the "periphbase" ever runtime configurable? If not I'm not sure we need the "reset". Regards, Peter > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >
On 27 November 2013 11:39, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > Is the "periphbase" ever runtime configurable? If not I'm not sure we > need the "reset". You can't runtime configure it (it's a bunch of signals into the core that determine where the decoder sits the peripherals in the memory map). However, the CBAR register which on reset starts out with the value of the base address is a writable register (writing it won't change where the peripherals live, it just reads-as-written). -- PMM
Hi, Am 27.11.2013 12:39, schrieb Peter Crosthwaite: > On Wed, Nov 27, 2013 at 8:27 PM, Andreas Färber <afaerber@suse.de> wrote: >> Am 27.11.2013 11:15, schrieb Peter Maydell: >>> On 27 November 2013 09:42, Andreas Färber <afaerber@suse.de> wrote: >>>> If we turn it into a dynamic property, we could register it conditional >>>> to ARM_FEATURE_CBAR. >>> >>> Unfortunately feature flags only get set at realize (in the >>> per-cpu init function), so we don't know at the point where >>> we're registering properties whether to have this one or not. >> >> 1/6 sets it in instance_init actually. So instance_post_init might do. >> > > Do you have a candidate example of this pattern that I can work off? For instance_post_init the only user so far will be in hw/core/qdev.c. target-i386/cpu.c adds a few dynamic properties for the base X86CPU. Cheers, Andreas
On Wed, Nov 27, 2013 at 9:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 27 November 2013 11:39, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> Is the "periphbase" ever runtime configurable? If not I'm not sure we >> need the "reset". > > You can't runtime configure it (it's a bunch of signals into the > core that determine where the decoder sits the peripherals in > the memory map). So that is a top level signal of MPCore rather than A9 CPUs right? Assuming so, that name "PERIPHBASE" is a ideally property of the the mpcore container device. And in this ideal world that container contains the A9 CPUs themselves and propagates "periphbase" through to the CPU as "cbar" during its own init/realize. Looking at ARM docco, the definition of CBAR == PERIPHBASE is A9MPCore specific. From ARM infocentre: ---- Cortex-A9 Technical Reference ManualRevision: r4p1 Home > System Control > Register descriptions > Configuration Base Address Register 4.3.24. Configuration Base Address Registe ... Configurations In Cortex-A9 uniprocessor implementations the base address is set to zero. In Cortex-A9 MPCore implementations, the base address is reset to PERIPHBASE[31:13] so that software can determine the location of the private memory region" --- So the best name for this register AFAICT is simply CBAR and either MPCore container or whatever are responsible for setting it to an appropriate value. > However, the CBAR register which on reset > starts out with the value of the base address is a writable > register (writing it won't change where the peripherals live, > it just reads-as-written). > So with that in mind i think the "reset-" prefix is appropriate. Regards, Peter > -- PMM >
diff --git a/target-arm/cpu.c b/target-arm/cpu.c index a82fa61..f1c5f6b 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -20,6 +20,7 @@ #include "cpu.h" #include "qemu-common.h" +#include "hw/qdev-properties.h" #if !defined(CONFIG_USER_ONLY) #include "hw/loader.h" #endif @@ -847,6 +848,11 @@ typedef struct ARMCPUInfo { void (*class_init)(ObjectClass *oc, void *data); } ARMCPUInfo; +static Property arm_cpu_properties[] = { + DEFINE_PROP_UINT32("cbar", ARMCPU, reset_cbar, 0), + DEFINE_PROP_END_OF_LIST(), +}; + static const ARMCPUInfo arm_cpus[] = { #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64) { .name = "arm926", .initfn = arm926_initfn }, @@ -895,6 +901,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) acc->parent_realize = dc->realize; dc->realize = arm_cpu_realizefn; + dc->props = arm_cpu_properties; acc->parent_reset = cc->reset; cc->reset = arm_cpu_reset;
The reset Value of the CP15 CBAR is a vendor (machine) configurable property. Define arm_cpu_properties and add it. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- target-arm/cpu.c | 7 +++++++ 1 file changed, 7 insertions(+)