qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH arm-devs v2 5/8] arm/highbank: Fix CBAR intialis


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH arm-devs v2 5/8] arm/highbank: Fix CBAR intialisation
Date: Fri, 29 Nov 2013 09:55:15 +1000

On Fri, Nov 29, 2013 at 5:34 AM, Peter Maydell <address@hidden> wrote:
> On 28 November 2013 03:29, Peter Crosthwaite
> <address@hidden> 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 <address@hidden>
>> ---
>> 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
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]