Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

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

On 10/22/2014 10:03 AM, Ian Campbell wrote:
> On Tue, 2014-10-21 at 23:54 +0200, Samuel Thibault wrote:
>> Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a écrit :
>>> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>>>> +static inline uint32_t REG_READ32(volatile uint32_t *addr)
>>>> +{
>>>> +    uint32_t value;
>>>> +    __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr));
>>>> +    rmb();
>>>
>>> I'm not 100% convinced that you need this rmb().

Most the GIC code doesn't require read barrier but...

>>>
>>>> +    return value;
>>>> +}
>>>> +
>>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value)
>>>> +{
>>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
>>>> +    wmb();
>>>> +}

write barrier may be necessary on some, where we need to wait that all
write has been done before doing this one (such as enable the GIC ...).

So this function is buggy. It should be:

wmb();
__asm__ __volatile__(....).

>>
>> I don't really see why such barriers are needed indeed. Are they needed
>> to actually push the values out?
> 
> That would, I think, require an isb() (instruction barrier) whereas
> wmb() turns into a dsb() (data barrier). I expect you are write and
> these rmb/wmb are not needed, but an isb may be needed in the caller if
> they want to rely on the affect of a write (e.g. enabling the
> controller)

isb is useful for cache and system control registers. We don't use such
things in the GIC code, because the GIC register is memory mapped. We
only need to ensure that the write are ordered when it's necessary (only
few places).

Regards,

-- 
Julien Grall