[PATCH] ux500 : decouple/recouple gic from the PRCMU

[Vincent Guittot]

On 7 February 2012 15:15, Daniel Lezcano <daniel.lezcano at linaro.org> wrote:
> On 02/07/2012 10:57 AM, Rickard Andersson wrote:
>>
>> On 02/07/2012 10:22 AM, Daniel Lezcano wrote:
>>>
>>> On 02/07/2012 09:39 AM, Rickard Andersson wrote:
>>>>
>>>> On 02/06/2012 03:10 PM, Daniel Lezcano wrote:
>>>>>
>>>>> On 02/06/2012 10:19 AM, Rickard Andersson wrote:
>>>>>>
>>>>>> Hi!
>>>>>>
>>>>>> Our comments:
>>>>>
>>>>> Thanks Rickard and Jonas for your comments.
>>>>>
>>>>>> - function names don't match commit comment disable/enable vs
>>>>>> recouple/decouple. Decouple is a better name than disable, because GIC
>>>>>> is not really disabled it is just disconnected.
>>>>>
>>>>> Ok, that makes sense.
>>>>>
>>>>>> - there is no reason to place these functions inside the
>>>>>> db8500-prcmu.c
>>>>>> file. There is so much stuff in the PRCMU register base so we can not
>>>>>> have everything in one file. Why not have it as it is?
>>>>>
>>>>> Why spread the prcmu code when it is related to the prcmu ?
>>>>>
>>>>> Linus ? What do you think ?
>>>>>
>>>>>> - why the gic_mask function?
>>>>>
>>>>> Because the register has 31 bits reserved which could be used later
>>>>> without modifying this function.
>>>>
>>>> This is will most likely not happen. If that happens we could add that
>>>> function then. We should remove it for now.
>>>>>>
>>>>>> - The original code has been updated and now looks like this:
>>>>>
>>>>> That is the same code, except the while loop where, if this code
>>>>> assumption is correct, means we will do only one iteration in the loop.
>>>>
>>>> Yes, but in theory you can be stuck forever in that loop, so please
>>>> remove the loop.
>>>
>>> Mmmh, can you elaborate please ? I think a comment and/or an error
>>> should be returned in this case, no ?
>>>
>> We do not need the loop. PRCMU FW does decouple of the GIC and if they
>> for some reason does that at the same time (they should not) we will be
>> stuck.
>
>
> Sorry, but I still don't get it. What is "they" ? Do you mean one cpu can
> decouples the cpu while the other cpu recouples it ?
>

Normally, the cpu decouples the gic and the prcmu recouples it so if
cpu decouples the gic and the prcmu recouples it between the write and
the read access, you will never read the value PRCMU_A9_MASK_REQ_MASK
and stay in your loop. This should not occurs because the prcmu has no
reason to recouple at this moment

> Taking the routine, why is needed the "read" if we don't care the return
> value ?

IMHO, the read access is used to ensure that your write access has
really been done in the register and is not still in the system bus
>
>
> void ux500_pm_gic_decouple(void)
> {
>        prcmu_write_masked(PRCM_A9_MASK_REQ,
>                           PRCM_A9_MASK_REQ_PRCM_A9_MASK_REQ,
>                           PRCM_A9_MASK_REQ_PRCM_A9_MASK_REQ);
>
>        (void)prcmu_read(PRCM_A9_MASK_REQ);
>
>        udelay(GIC_FREEZE_DELAY); /* Wait for the GIC to freeze */
> }
>
> Thanks
>  -- Daniel
>
>
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev