| Message ID | 4EF0DDB5.9040806@calxeda.com |
|---|---|
| State | New |
| Headers | show |
On 20 December 2011 19:10, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: > From: Rob Herring <rob.herring@calxeda.com> > > Signed-off-by: Rob Herring <rob.herring@calxeda.com> > Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com> > > Conflicts: > > target-arm/cpu.h > target-arm/helper.c Commit messages shouldn't have Conflicts notes in them. You could also use more detail here (for a start, you've only added one register). > --- > target-arm/cpu.h | 1 + > target-arm/helper.c | 7 +++++++ > 2 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 129edbb..b89c085 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -152,6 +152,7 @@ typedef struct CPUARMState { > uint32_t c15_i_max; /* Maximum D-cache dirty line index. */ > uint32_t c15_i_min; /* Minimum D-cache dirty line index. */ > uint32_t c15_threadid; /* TI debugger thread-ID. */ > + uint32_t c15_scubase; /* SCU base address. */ > } cp15; > > struct { > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 816c4c4..37110bc 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -2197,6 +2197,13 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t > insn) > * 0x200 << ($rn & 0xfff), when MMU is off. */ > goto bad_reg; > } > + if (ARM_CPUID(env) == ARM_CPUID_CORTEXA9) { > + switch (crm) { > + case 0: > + return env->cp15.c15_scubase; > + } > + goto bad_reg; > + } This is underdecoded: the A9 has two registers in c15,c0: CRn Op1 CRm Op2 Name 15 0 c0 0 Power Control Register 15 4 c0 0 Configuration Base Address I'm guessing you're after the Configuration Base Address register. (please call the struct name something vaguely relating to the official register name, incidentally.) > return 0; > } > bad_reg: This commit leaves the register with a reset value of 0, which isn't right (we only implement A9MP, not A9UP, so the reset value should be settable by the board at init time somehow depending where the a9mpcore_priv device is mapped. Not sure what the cleanest way to do that is.) -- PMM
On 12/20/2011 01:48 PM, Peter Maydell wrote: > On 20 December 2011 19:10, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: >> diff --git a/target-arm/helper.c b/target-arm/helper.c >> index 816c4c4..37110bc 100644 >> --- a/target-arm/helper.c >> +++ b/target-arm/helper.c >> @@ -2197,6 +2197,13 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t >> insn) >> * 0x200 << ($rn & 0xfff), when MMU is off. */ >> goto bad_reg; >> } >> + if (ARM_CPUID(env) == ARM_CPUID_CORTEXA9) { >> + switch (crm) { >> + case 0: >> + return env->cp15.c15_scubase; >> + } >> + goto bad_reg; >> + } > > This is underdecoded: the A9 has two registers in c15,c0: > CRn Op1 CRm Op2 Name > 15 0 c0 0 Power Control Register > 15 4 c0 0 Configuration Base Address > > I'm guessing you're after the Configuration Base Address register. > (please call the struct name something vaguely relating to the > official register name, incidentally.) Apparently it's called scubase in the Linux code, but I'll fix the name for qemu. >> return 0; >> } >> bad_reg: > > This commit leaves the register with a reset value of 0, which > isn't right (we only implement A9MP, not A9UP, so the reset value > should be settable by the board at init time somehow depending > where the a9mpcore_priv device is mapped. Not sure what the > cleanest way to do that is.) I can add a DEFINE_PROP_UINT32 to a9mpcore_priv, which would give anyone who wants to set the property an easy way to do so. I'm not sure how to hook the a9mpcore_priv to the CPUARMState, though. They don't appear to reference each other. --Mark Langsdorf Calxeda, Inc.
On 21 December 2011 16:37, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: > On 12/20/2011 01:48 PM, Peter Maydell wrote: >> This commit leaves the register with a reset value of 0, which >> isn't right (we only implement A9MP, not A9UP, so the reset value >> should be settable by the board at init time somehow depending >> where the a9mpcore_priv device is mapped. Not sure what the >> cleanest way to do that is.) > > I can add a DEFINE_PROP_UINT32 to a9mpcore_priv, which would give > anyone who wants to set the property an easy way to do so. I'm > not sure how to hook the a9mpcore_priv to the CPUARMState, though. > They don't appear to reference each other. Yes, we don't really model the private peripherals very sensibly: they should be an integral part of the CPU but for historical reasons they've been done as a totally separate device. I don't think a property is the right thing, though -- ideally the a9mpcore device should be able to find out for itself where it was mapped. Avi: is there a way for a device (sysbus device in this case) to find out for one of its memory regions where it has been mapped in the address space? The context here is that the Cortex-A9 has a cp15 register whose value is "base address of the private peripherals", and it would be nice not to have to have boards saying both "map mmio region at X" and "set property so register reads as X"... [You could argue that hardware implementations have to do the equivalent of both of these things separately, I suppose, but it's still not very pretty.] thanks -- PMM
On 12/21/2011 11:10 PM, Peter Maydell wrote: > On 21 December 2011 16:37, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote: > > On 12/20/2011 01:48 PM, Peter Maydell wrote: > >> This commit leaves the register with a reset value of 0, which > >> isn't right (we only implement A9MP, not A9UP, so the reset value > >> should be settable by the board at init time somehow depending > >> where the a9mpcore_priv device is mapped. Not sure what the > >> cleanest way to do that is.) > > > > I can add a DEFINE_PROP_UINT32 to a9mpcore_priv, which would give > > anyone who wants to set the property an easy way to do so. I'm > > not sure how to hook the a9mpcore_priv to the CPUARMState, though. > > They don't appear to reference each other. > > Yes, we don't really model the private peripherals very sensibly: > they should be an integral part of the CPU but for historical > reasons they've been done as a totally separate device. I don't > think a property is the right thing, though -- ideally the a9mpcore > device should be able to find out for itself where it was mapped. > > Avi: is there a way for a device (sysbus device in this case) to > find out for one of its memory regions where it has been mapped > in the address space? Where and if. > The context here is that the Cortex-A9 > has a cp15 register whose value is "base address of the private > peripherals", and it would be nice not to have to have boards > saying both "map mmio region at X" and "set property so register > reads as X"... [You could argue that hardware implementations > have to do the equivalent of both of these things separately, > I suppose, but it's still not very pretty.] I don't really follow, can you explain? Anyway, with the new MemoryListener API (not yet merged), you can register a callback to be called when MemoryRegions become visible or invisible. You can filter there for your pet region and get all the info about it.
On 22 December 2011 10:14, Avi Kivity <avi@redhat.com> wrote: > On 12/21/2011 11:10 PM, Peter Maydell wrote: >> Avi: is there a way for a device (sysbus device in this case) to >> find out for one of its memory regions where it has been mapped >> in the address space? > > Where and if. > >> The context here is that the Cortex-A9 >> has a cp15 register whose value is "base address of the private >> peripherals", and it would be nice not to have to have boards >> saying both "map mmio region at X" and "set property so register >> reads as X"... [You could argue that hardware implementations >> have to do the equivalent of both of these things separately, >> I suppose, but it's still not very pretty.] > > I don't really follow, can you explain? So in real hardware, these devices (interrupt controller, timers, etc) are an integral part of the CPU. They appear in the memory map at an address which is configured by hardwiring the CPU's PERIPHBASE signals to specify that address. Since obviously software needs to know where the devices are, there is a coprocessor register which simply returns the value of PERIPHBASE. (So I was wrong that hardware does the mapping and the register separately -- sorry.) Part of the problem we have is that in QEMU we don't model the CPU as a single qdev device which includes both the core and its builtin devices -- the two are completely separate and the board model ends up having to do a lot of the work of wiring things up, and in cases like this where one bit of config affects both the core CPU and the builtin devices you end up having to specify it twice. > Anyway, with the new MemoryListener API (not yet merged), you can > register a callback to be called when MemoryRegions become visible or > invisible. You can filter there for your pet region and get all the > info about it. Mmm, that would work. -- PMM
On 12/22/2011 02:37 PM, Peter Maydell wrote: > On 22 December 2011 10:14, Avi Kivity <avi@redhat.com> wrote: > > On 12/21/2011 11:10 PM, Peter Maydell wrote: > >> Avi: is there a way for a device (sysbus device in this case) to > >> find out for one of its memory regions where it has been mapped > >> in the address space? > > > > Where and if. > > > >> The context here is that the Cortex-A9 > >> has a cp15 register whose value is "base address of the private > >> peripherals", and it would be nice not to have to have boards > >> saying both "map mmio region at X" and "set property so register > >> reads as X"... [You could argue that hardware implementations > >> have to do the equivalent of both of these things separately, > >> I suppose, but it's still not very pretty.] > > > > I don't really follow, can you explain? > > So in real hardware, these devices (interrupt controller, > timers, etc) are an integral part of the CPU. They appear in > the memory map at an address which is configured by hardwiring > the CPU's PERIPHBASE signals to specify that address. Since > obviously software needs to know where the devices are, there > is a coprocessor register which simply returns the value > of PERIPHBASE. (So I was wrong that hardware does the mapping > and the register separately -- sorry.) > > Part of the problem we have is that in QEMU we don't model > the CPU as a single qdev device which includes both the > core and its builtin devices -- the two are completely > separate and the board model ends up having to do a lot of > the work of wiring things up, and in cases like this where > one bit of config affects both the core CPU and the builtin > devices you end up having to specify it twice. Sounds like a qom Link<Blah> or some other magic should handle it? #include "cc/aliguori". > > Anyway, with the new MemoryListener API (not yet merged), you can > > register a callback to be called when MemoryRegions become visible or > > invisible. You can filter there for your pet region and get all the > > info about it. > > Mmm, that would work. Now that I better understand the issue, I think it's overkill. You just want to replicate a register between devices. The MemoryListener API is designed to allow you to see the memory map from the cpu's side, after a region has been transformed by overlapping regions, address transformation by bridges, enables/disables, etc. You just want to copy a value from one object to another, you don't care about any of this.
On 12/22/2011 06:43 AM, Avi Kivity wrote: > On 12/22/2011 02:37 PM, Peter Maydell wrote: >> On 22 December 2011 10:14, Avi Kivity<avi@redhat.com> wrote: >>> On 12/21/2011 11:10 PM, Peter Maydell wrote: >>>> Avi: is there a way for a device (sysbus device in this case) to >>>> find out for one of its memory regions where it has been mapped >>>> in the address space? >>> >>> Where and if. >>> >>>> The context here is that the Cortex-A9 >>>> has a cp15 register whose value is "base address of the private >>>> peripherals", and it would be nice not to have to have boards >>>> saying both "map mmio region at X" and "set property so register >>>> reads as X"... [You could argue that hardware implementations >>>> have to do the equivalent of both of these things separately, >>>> I suppose, but it's still not very pretty.] >>> >>> I don't really follow, can you explain? >> >> So in real hardware, these devices (interrupt controller, >> timers, etc) are an integral part of the CPU. They appear in >> the memory map at an address which is configured by hardwiring >> the CPU's PERIPHBASE signals to specify that address. Since >> obviously software needs to know where the devices are, there >> is a coprocessor register which simply returns the value >> of PERIPHBASE. (So I was wrong that hardware does the mapping >> and the register separately -- sorry.) >> >> Part of the problem we have is that in QEMU we don't model >> the CPU as a single qdev device which includes both the >> core and its builtin devices -- the two are completely >> separate and the board model ends up having to do a lot of >> the work of wiring things up, and in cases like this where >> one bit of config affects both the core CPU and the builtin >> devices you end up having to specify it twice. > > Sounds like a qom Link<Blah> or some other magic should handle it? > #include "cc/aliguori". First, we need to actually model the CPUs. The general trouble with that today is that everything needs to live on a bus after we get QOM merged, that requirement will be relaxed. Once the CPUs are objects, they can create children devices (builtin interrupt controller for instance) in their instance_init function. It would be a child<> property in this case. This is how I want to model the local apic, for instance. Regards, Anthony Liguori
On 22 December 2011 14:25, Anthony Liguori <anthony@codemonkey.ws> wrote: > First, we need to actually model the CPUs. The general trouble with that > today is that everything needs to live on a bus after we get QOM merged, > that requirement will be relaxed. > > Once the CPUs are objects, they can create children devices (builtin > interrupt controller for instance) in their instance_init function. It > would be a child<> property in this case. So, in summary to Mark, if "always reads as zero" is sufficient for your purposes, I'll accept this as a patch which implements that (if you fix the other issues and add a comment that it ought to be the base address of the private peripheral region). We can implement it properly when we have enough object model infrastructure to do so. -- PMM
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 129edbb..b89c085 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -152,6 +152,7 @@ typedef struct CPUARMState { uint32_t c15_i_max; /* Maximum D-cache dirty line index. */ uint32_t c15_i_min; /* Minimum D-cache dirty line index. */ uint32_t c15_threadid; /* TI debugger thread-ID. */ + uint32_t c15_scubase; /* SCU base address. */ } cp15; struct { diff --git a/target-arm/helper.c b/target-arm/helper.c index 816c4c4..37110bc 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -2197,6 +2197,13 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t insn) * 0x200 << ($rn & 0xfff), when MMU is off. */ goto bad_reg; } + if (ARM_CPUID(env) == ARM_CPUID_CORTEXA9) { + switch (crm) { + case 0: + return env->cp15.c15_scubase; + } + goto bad_reg; + } return 0; }