Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <5AF111F0.3030409@rock-chips.com>
Date:   Tue, 08 May 2018 10:56:48 +0800
From:   JeffyChen <jeffy.chen@rock-chips.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20130126 Thunderbird/19.0
MIME-Version: 1.0
To:     Brian Norris <briannorris@chromium.org>
CC:     linux-kernel@vger.kernel.org, heiko@sntech.de,
        linux-rockchip@lists.infradead.org,
        Linus Walleij <linus.walleij@linaro.org>,
        linux-gpio@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Doug Anderson <dianders@chromium.org>
Subject: Re: [RESEND PATCH] pinctrl: rockchip: Disable interrupt when changing
 it's capability
References: <20180503065553.7762-1-jeffy.chen@rock-chips.com> <20180507221511.GA6448@rodete-desktop-imager.corp.google.com> <5AF0FF18.1050905@rock-chips.com> <20180508015623.GA61455@rodete-desktop-imager.corp.google.com> <5AF10C10.1070003@rock-chips.com>
In-Reply-To: <5AF10C10.1070003@rock-chips.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Brian,

On 05/08/2018 10:31 AM, JeffyChen wrote:
> Hi Brian,
>
> On 05/08/2018 09:56 AM, Brian Norris wrote:
>> On Tue, May 08, 2018 at 09:36:24AM +0800, Jeffy Chen wrote:
>>> On 05/08/2018 06:15 AM, Brian Norris wrote:
>>>> On the other hand...this also implies there may be a race condition
>>>> there, where we might lose an interrupt if there is an edge between the
>>>> re-configuration of the polarity in rockchip_irq_demux() and the
>>>> clearing/handling of the interrupt (handle_edge_irq() ->
>>>> chip->irq_ack()). If we have an edge between there, then we might ack
>>>> it, but leave the polarity such that we aren't ready for the next
>>>> (inverted) edge.
>>>
>>> if let me guess, the unexpected irq we saw is the hardware trying to
>>> avoid
>>> losing irq? for example, we set a EDGE_RISING, and the hardware saw
>>> the gpio
>>> is already high, then though it might lost an irq, so fake one for safe?
>>
>> I won't pretend to know what the IC designers were doing, but I don't
>> think that would resolve the problem I'm talking about. The sequence is
>> something like:
>> 1. EDGE_BOTH IRQ occurs (e.g., low to high)
>> 2. reconfigure polarity in rockchip_irq_demux() (polarity=low)
>> 3. continue to handle_edge_irq()
>> 4. another HW edge occurs (e.g., high to low)
>> 5. handle_edge_irq() (from 3) acks (clears) IRQ (before a subsequent
>>     rockchip_irq_demux() gets a chance to run and flip the polarity)
>> ...
>>
>> Now the polarity is still low, but the next trigger should be a
>> low-to-high edge.
>
> oops, i see the problem.
>
> so what if we do these:
> 1/ edge irq triggered
> 2/ read gpio level
> 3/ handle irq(ack irq)
> 4/ toggle edge mode(with a while gpio level check)
>
> if the gpio changed in 2/ -> 3/, the 4/ will trigger an irq when writing
> GPIO_INT_POLARITY(which is what we are trying to avoid in the set_type
> case)
>
> but this would not work if i'm wrong about how the HW fake an irq when
> changing POLARITY...
>
>
> or maybe we could just check the gpio status again after
> handle_edge_irq, and correct the polarity in this case

i saw the pinctrl-msm.c do this in the ack(),

and also at the end of set_type(), which might avoid another race in the 
set_type()

>
>>
>>> i'll try to confirm it with IC guys.
>>
>> Brian
>>
>>
>>
>