Re: [PATCH ARM v4 11/12] mini-os: get GIC addresses from FDT

[Julien Grall <julien.grall@linaro.org>]

Hi Thomas,

On 06/19/2014 09:50 AM, Thomas Leonard wrote:
> On 18 June 2014 18:25, Julien Grall <julien.grall@linaro.org> wrote:
>> Hi Thomas,
>>
>> On 06/18/2014 04:08 PM, Thomas Leonard wrote:
>>>  //#define VGIC_DEBUG
>>>  #ifdef VGIC_DEBUG
>>> @@ -168,9 +169,38 @@ static void gic_handler(void) {
>>>  }
>>>
>>>  void gic_init(void) {
>>> -    // FIXME Get from dt!
>>> -    gic.gicd_base = (char *)0x2c001000ULL;
>>> -    gic.gicc_base = (char *)0x2c002000ULL;
>>> +    gic.gicd_base = NULL;
>>
>> Any reason to not fold this patch in patch #7? Or better move the gic
>> code in a separate patch?
> 
> It was previously requested that I split the FDT patch from the main
> ARM one. This patch depends on libfdt being present, so it has to go
> after that.

Please make sure that this "standalone patch" works correctly on Xen
unstable...

> Moving all the GIC code to this patch would mean that the original
> patch wouldn't work on its own.

I'm not sure why you want to have a single big patch to support ARM...
AFAIU, there is some patch requirements to work correctly.


>> You made assumption of the layout of the device tree provided by Xen:
>>         - #address-cells == #size-cells == 2
>>         - regs contains a valid physical address, i.e the device is not under a bus
>>
>> This can be changed by the toolstack in the future and will likely break
>> mini-os.
> 
> Is that likely? Seems like using BUG here is the right thing to do
> until that happens.

You need at least to add a comment about the (len != 32). It's confusing
for people that doesn't know how the device tree has been created.

I still think that a layer for IRQ and MMIO translation would be helpful
for mini-os. So you will directly get the range (base + size), and won't
duplicate checking code on every part of mini-os.

> That would be very helpful - thanks!

You can join #xenarm for any question if you are not already there.

Regards,

-- 
Julien Grall