| Message ID | a199380a16761a709f86cff4e019cc884feace1c.1385608859.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 28 November 2013 03:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > Fix the CBAR initialisation by using the newly defined static property. > CBAR is now set before realization, so the intended value is now > actually used. > > So I have kinda tested this. I booted an ARM kernel on Highbank with the > stock Highbank DTB. It doesnt boot (and I will be doing something > wrong), but before this patch I got this: > > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 0 at /workspaces/pcrost/public/linux2.git/arch/arm/mm/ioremap.c:301 __arm_ioremap_pfn_caller+0x180/0x198() > CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 3.13.0-rc1-next-20131126-dirty #2 > [<c0015164>] (unwind_backtrace) from [<c00118c0>] (show_stack+0x10/0x14) > [<c00118c0>] (show_stack) from [<c02bd5fc>] (dump_stack+0x78/0x90) > [<c02bd5fc>] (dump_stack) from [<c001f110>] (warn_slowpath_common+0x68/0x84) > [<c001f110>] (warn_slowpath_common) from [<c001f1f4>] (warn_slowpath_null+0x1c/0x24) > [<c001f1f4>] (warn_slowpath_null) from [<c0017c6c>] (__arm_ioremap_pfn_caller+0x180/0x198) > [<c0017c6c>] (__arm_ioremap_pfn_caller) from [<c0017cd8>] (__arm_ioremap_caller+0x54/0x5c) > [<c0017cd8>] (__arm_ioremap_caller) from [<c0017d10>] (__arm_ioremap+0x18/0x1c) > [<c0017d10>] (__arm_ioremap) from [<c03913c0>] (highbank_init_irq+0x34/0x8c) > [<c03913c0>] (highbank_init_irq) from [<c038c228>] (init_IRQ+0x28/0x2c) > [<c038c228>] (init_IRQ) from [<c03899ec>] (start_kernel+0x234/0x398) > [<c03899ec>] (start_kernel) from [<00008074>] (0x8074) > ---[ end trace 3406ff24bd97382f ]--- > > Which dissappears with this patch. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > changed since v1: > use error report rather than fprintf(stderr > > hw/arm/highbank.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c > index 1d19d8f..cb32325 100644 > --- a/hw/arm/highbank.c > +++ b/hw/arm/highbank.c > @@ -236,14 +236,16 @@ static void calxeda_init(QEMUMachineInitArgs *args, enum cxmachines machine) > > cpu = ARM_CPU(object_new(object_class_get_name(oc))); > > + object_property_set_int(OBJECT(cpu), GIC_BASE_ADDR, "reset-cbar", &err); > + if (err) { > + error_report("%s", error_get_pretty(err)); > + exit(1); > + } It's kind of sad that we need five lines of code just to say "set this property on this object which we should know at compile time to exist" . (I was about to argue we should just assert, but since we don't refuse to start with wacky user-provided cpu-model strings we can't quite get away with that.) More significantly, this will break midway, because cortex-a15 doesn't have this property. Fortunately, the actual A15 does have a CBAR register so you can just add a patch to set the feature bit for it. Caution, this means our semantics for reset-cbar are "actual reset value of register", which is not the same as "base address of peripherals", because for A15 the register has [31:15] bits 31:15 of base-addr [14:8] reserved, zero [7:0] bits 39:32 of base-addr which makes a difference if you were nutty enough to put the GIC above the 4GB boundary. (As with the real hardware, setting this config property doesn't actually change where the GIC & friends live in the address space, incidentally.) > object_property_set_bool(OBJECT(cpu), true, "realized", &err); > if (err) { > error_report("%s", error_get_pretty(err)); > exit(1); > } > - > - /* This will become a QOM property eventually */ > - cpu->reset_cbar = GIC_BASE_ADDR; > cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ); > } thanks -- PMM
On Fri, Nov 29, 2013 at 5:34 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 28 November 2013 03:29, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> Fix the CBAR initialisation by using the newly defined static property. >> CBAR is now set before realization, so the intended value is now >> actually used. >> >> So I have kinda tested this. I booted an ARM kernel on Highbank with the >> stock Highbank DTB. It doesnt boot (and I will be doing something >> wrong), but before this patch I got this: >> >> ------------[ cut here ]------------ >> WARNING: CPU: 0 PID: 0 at /workspaces/pcrost/public/linux2.git/arch/arm/mm/ioremap.c:301 __arm_ioremap_pfn_caller+0x180/0x198() >> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 3.13.0-rc1-next-20131126-dirty #2 >> [<c0015164>] (unwind_backtrace) from [<c00118c0>] (show_stack+0x10/0x14) >> [<c00118c0>] (show_stack) from [<c02bd5fc>] (dump_stack+0x78/0x90) >> [<c02bd5fc>] (dump_stack) from [<c001f110>] (warn_slowpath_common+0x68/0x84) >> [<c001f110>] (warn_slowpath_common) from [<c001f1f4>] (warn_slowpath_null+0x1c/0x24) >> [<c001f1f4>] (warn_slowpath_null) from [<c0017c6c>] (__arm_ioremap_pfn_caller+0x180/0x198) >> [<c0017c6c>] (__arm_ioremap_pfn_caller) from [<c0017cd8>] (__arm_ioremap_caller+0x54/0x5c) >> [<c0017cd8>] (__arm_ioremap_caller) from [<c0017d10>] (__arm_ioremap+0x18/0x1c) >> [<c0017d10>] (__arm_ioremap) from [<c03913c0>] (highbank_init_irq+0x34/0x8c) >> [<c03913c0>] (highbank_init_irq) from [<c038c228>] (init_IRQ+0x28/0x2c) >> [<c038c228>] (init_IRQ) from [<c03899ec>] (start_kernel+0x234/0x398) >> [<c03899ec>] (start_kernel) from [<00008074>] (0x8074) >> ---[ end trace 3406ff24bd97382f ]--- >> >> Which dissappears with this patch. >> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> --- >> changed since v1: >> use error report rather than fprintf(stderr >> >> hw/arm/highbank.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c >> index 1d19d8f..cb32325 100644 >> --- a/hw/arm/highbank.c >> +++ b/hw/arm/highbank.c >> @@ -236,14 +236,16 @@ static void calxeda_init(QEMUMachineInitArgs *args, enum cxmachines machine) >> >> cpu = ARM_CPU(object_new(object_class_get_name(oc))); >> >> + object_property_set_int(OBJECT(cpu), GIC_BASE_ADDR, "reset-cbar", &err); >> + if (err) { >> + error_report("%s", error_get_pretty(err)); >> + exit(1); >> + } > > It's kind of sad that we need five lines of code just to > say "set this property on this object which we should > know at compile time to exist" . (I was about to argue we > should just assert, but since we don't refuse to start with > wacky user-provided cpu-model strings we can't quite get > away with that.) > > More significantly, this will break midway, because > cortex-a15 doesn't have this property. Fortunately, the > actual A15 does have a CBAR register so you can just > add a patch to set the feature bit for it. > Ok will fix - thanks. Should I just feature CBAR for all the CPU families you listed in v1 discussion? > Caution, this means our semantics for reset-cbar > are "actual reset value of register", which is not > the same as "base address of peripherals", That was the intended semantic. > because > for A15 the register has > [31:15] bits 31:15 of base-addr > [14:8] reserved, zero > [7:0] bits 39:32 of base-addr > Yes, so I i'm still of the opinion that this stuff is the responsibility of MPCore not ARM_CPU. Ideally, we move the ARM cpus into MPCore. Then boards set periphbase of mpcores and mpcores set CBARs. Keeps CPUs unaware of periphbase mangling complications like this. And keeps boards unaware of CBARs. Regards, Peter > which makes a difference if you were nutty enough to > put the GIC above the 4GB boundary. > > (As with the real hardware, setting this config property > doesn't actually change where the GIC & friends live > in the address space, incidentally.) > >> object_property_set_bool(OBJECT(cpu), true, "realized", &err); >> if (err) { >> error_report("%s", error_get_pretty(err)); >> exit(1); >> } >> - >> - /* This will become a QOM property eventually */ >> - cpu->reset_cbar = GIC_BASE_ADDR; >> cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ); >> } > > thanks > -- PMM >
On 28 November 2013 23:55, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Fri, Nov 29, 2013 at 5:34 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> More significantly, this will break midway, because >> cortex-a15 doesn't have this property. Fortunately, the >> actual A15 does have a CBAR register so you can just >> add a patch to set the feature bit for it. >> > > Ok will fix - thanks. Should I just feature CBAR for all the CPU > families you listed in v1 discussion? I'm pretty sure the only two cores with CBAR that we implement are A15 and A9. NB that the permissions on the two registers are not the same, though -- check the TRMs. (A15's is always r/o). >> Caution, this means our semantics for reset-cbar >> are "actual reset value of register", which is not >> the same as "base address of peripherals", > > That was the intended semantic. > >> because >> for A15 the register has >> [31:15] bits 31:15 of base-addr >> [14:8] reserved, zero >> [7:0] bits 39:32 of base-addr >> > > Yes, so I i'm still of the opinion that this stuff is the > responsibility of MPCore not ARM_CPU. Ideally, we move the ARM cpus > into MPCore. Then boards set periphbase of mpcores and mpcores set > CBARs. Keeps CPUs unaware of periphbase mangling complications like > this. And keeps boards unaware of CBARs. I'm in favour of eventually moving the cores themselves into the *mpcore containers, though I'm not entirely sure that the CBAR setting and periphbase locations are a single setting on real hardware config (as opposed to two different settings that are supposed to be the same). -- PMM
diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c index 1d19d8f..cb32325 100644 --- a/hw/arm/highbank.c +++ b/hw/arm/highbank.c @@ -236,14 +236,16 @@ static void calxeda_init(QEMUMachineInitArgs *args, enum cxmachines machine) cpu = ARM_CPU(object_new(object_class_get_name(oc))); + object_property_set_int(OBJECT(cpu), GIC_BASE_ADDR, "reset-cbar", &err); + if (err) { + error_report("%s", error_get_pretty(err)); + exit(1); + } object_property_set_bool(OBJECT(cpu), true, "realized", &err); if (err) { error_report("%s", error_get_pretty(err)); exit(1); } - - /* This will become a QOM property eventually */ - cpu->reset_cbar = GIC_BASE_ADDR; cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ); }
Fix the CBAR initialisation by using the newly defined static property. CBAR is now set before realization, so the intended value is now actually used. So I have kinda tested this. I booted an ARM kernel on Highbank with the stock Highbank DTB. It doesnt boot (and I will be doing something wrong), but before this patch I got this: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 0 at /workspaces/pcrost/public/linux2.git/arch/arm/mm/ioremap.c:301 __arm_ioremap_pfn_caller+0x180/0x198() CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 3.13.0-rc1-next-20131126-dirty #2 [<c0015164>] (unwind_backtrace) from [<c00118c0>] (show_stack+0x10/0x14) [<c00118c0>] (show_stack) from [<c02bd5fc>] (dump_stack+0x78/0x90) [<c02bd5fc>] (dump_stack) from [<c001f110>] (warn_slowpath_common+0x68/0x84) [<c001f110>] (warn_slowpath_common) from [<c001f1f4>] (warn_slowpath_null+0x1c/0x24) [<c001f1f4>] (warn_slowpath_null) from [<c0017c6c>] (__arm_ioremap_pfn_caller+0x180/0x198) [<c0017c6c>] (__arm_ioremap_pfn_caller) from [<c0017cd8>] (__arm_ioremap_caller+0x54/0x5c) [<c0017cd8>] (__arm_ioremap_caller) from [<c0017d10>] (__arm_ioremap+0x18/0x1c) [<c0017d10>] (__arm_ioremap) from [<c03913c0>] (highbank_init_irq+0x34/0x8c) [<c03913c0>] (highbank_init_irq) from [<c038c228>] (init_IRQ+0x28/0x2c) [<c038c228>] (init_IRQ) from [<c03899ec>] (start_kernel+0x234/0x398) [<c03899ec>] (start_kernel) from [<00008074>] (0x8074) ---[ end trace 3406ff24bd97382f ]--- Which dissappears with this patch. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- changed since v1: use error report rather than fprintf(stderr hw/arm/highbank.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)