Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Asmaa Mnebhi <asmaa@nvidia.com>
To:     Andrew Lunn <andrew@lunn.ch>
CC:     "andy.shevchenko@gmail.com" <andy.shevchenko@gmail.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "bgolaszewski@baylibre.com" <bgolaszewski@baylibre.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "rjw@rjwysocki.net" <rjw@rjwysocki.net>,
        David Thompson <davthompson@nvidia.com>
Subject: RE: [PATCH v1 1/2] gpio: mlxbf2: Introduce IRQ support
Thread-Topic: [PATCH v1 1/2] gpio: mlxbf2: Introduce IRQ support
Thread-Index: AQHXqoEVVyA+enuCGUKWJgFkwmB1oKumsuKAgAABitCAADi7AIAGX2jQ
Date:   Mon, 20 Sep 2021 20:32:31 +0000
Message-ID: <CH2PR12MB3895ACE512BA0471889E38BBD7A09@CH2PR12MB3895.namprd12.prod.outlook.com>
References: <20210915222847.10239-1-asmaa@nvidia.com>
 <20210915222847.10239-2-asmaa@nvidia.com> <YUNPO9/YacBNr/yQ@lunn.ch>
 <CH2PR12MB3895D5E16EAA1D3E5796C177D7DC9@CH2PR12MB3895.namprd12.prod.outlook.com>
 <YUOAHMmaSf2gs6ho@lunn.ch>
In-Reply-To: <YUOAHMmaSf2gs6ho@lunn.ch>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?tdV+RMU9ITUUMIFrKffPlhpf3pfKLAsNUYDn1SS2GgphV3wuR9Qmrgk+3buk?=
 =?us-ascii?Q?4Heg53YzNsRr0puwlAHcVGeihkj3ZJEjyw6RD8Db7obfM5jXwWU7dL5A86GV?=
 =?us-ascii?Q?TXS/jYHiAV9e/n88WWeneYGY/XKehUZlHbYzWqf9Fsnd89/+727A6ZLB3Xno?=
 =?us-ascii?Q?xKbZKZFW30vsZt3yTE0lkXIUhqOjECsT3wuz37VtMLpXm5zJMugtOAv+Y9i5?=
 =?us-ascii?Q?vLFvk+MXYstZRrcF3B/OpWdmcRDP/qP2d1slkppR2fSl3ZiAKDYzf8xHsqEO?=
 =?us-ascii?Q?puE8NfQ0y62FF4mvfOZ1h+oU99eGB//+9lbYJyqYFdi744yQB373sJVzeloA?=
 =?us-ascii?Q?MRP3mXlmbMQknglGNpcpcIg6hs+OtYaupeqweSTQ0WF2sxSPYCyFDblKLAuf?=
 =?us-ascii?Q?maY5I/0+VUnPcvUwb/XpbNexmPk08LBmbPkL93swWYuU02aPQVz5Uwyzdtvb?=
 =?us-ascii?Q?ZUxWc5CkexZS84lJubnT/ZFSxtf+RVOlLMJ61HdrR0+JIiT/yoFs9C2zZ0jY?=
 =?us-ascii?Q?oKbNX2fRkfGWUHMdMy8Z1GjuVmJe3zFpwNk0nCewTn/1DqebY/qcZUvTF/ZT?=
 =?us-ascii?Q?CPYAIUOuB+abYA8cUa2RcYzr+otdLHzdqxO7T4bj+96qEdI2XhbFasqahwFJ?=
 =?us-ascii?Q?3ib8E1FRsySP5f8IQPwe0Jg4N3s3GMOYnyVFSdYP/b/ADstq1WE5/7EgGz3e?=
 =?us-ascii?Q?/U/LcbujZHMWCBtPRi8qqsQheHAerVRcdhi0SNwdjEIR/cbooXNLcwVjtHPr?=
 =?us-ascii?Q?0sO/oZNsxosnptLh5F9P+GZ5L0bu42ciorbSJe7uFznWb7kfjuabvCSb2sKc?=
 =?us-ascii?Q?9Jbha123v6K/i9/mW3pSFgIVLEsmb8fyCKNiqWPB+RZ2DZnenD5PRW4LZPei?=
 =?us-ascii?Q?wz/aYcR+3s7Qf5/m6E1fUmH4tFHidLKaamjhmt5hOGZ5fX2+N1IHSvMiR3ZV?=
 =?us-ascii?Q?Uc5HJIwCZBjCNwXb8tN411TlxIxpqqrMa23H23kPe57ojrO6/sR4PldtbhhQ?=
 =?us-ascii?Q?qUC86txxS17GebhpVhVSn9DTvHnEGR+uOd+Qbf0ClPHuF3CDxpwEqjfPNjaA?=
 =?us-ascii?Q?h6rBjd+0ipVguUJa3NG0gZGVflpMeToCyNLEmwffb2Cj8z1vdl6fjdVWnZvD?=
 =?us-ascii?Q?Pb4nIUCUTkksSH0I4KevdIx1oKMI1rP40B1MdlJLuDvdHmxXkyxO6ed2sn3L?=
 =?us-ascii?Q?HMIJvNkcXbz8Bb4bFCc8j8UzCgDy613QoibhRRjOomoW29XoQyjBROFrC6ce?=
 =?us-ascii?Q?AASpTBo/t+o8cUeNipJz51+0G7FWZ9Ri9AMavXgZXimFEWJGOqQAeMPALyvo?=
 =?us-ascii?Q?eB+Aur8+7tpNDwoyng1WsImR?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB3895.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c5ede59f-427e-495b-9098-08d97c75c5a4
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Sep 2021 20:32:31.7081
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ARxQJs29NZfo6B0O8O/j5OtYG03tGhVqsDyurWtlMCw9+jDbmmpDu5kyQzDFFuZKoIJIocL9H7Mg1KrIkb19BA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4954
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> > +	val =3D readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
> > +	val |=3D BIT(offset);
> > +	writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
>=20
> What exactly does this do? It appears to clear the interrupt, if i unders=
tand
> mlxbf2_gpio_irq_handler(). I don't know the GPIO framework well enough to=
 know if this is
> correct. It does mean if the interrupt signal is active but masked, and y=
ou enable it, you appear
> to loose the interrupt? Maybe you want the interrupt to fire as soon as i=
t is enabled?
>=20
> Asmaa>>
> YU_GPIO_CAUSE_OR_CLRCAUSE - Makes sure the interrupt is initially cleared=
. Otherwise, we
> will not receive further interrupts.

> If the interrupt status bit is set, as soon as you unmask the interrupt, =
the hardware should fire
> the interrupt. At least, that is how interrupt controllers usually work.

> A typical pattern is that the interrupt fires. You mask it, ack it, and t=
hen do what is needed to
> actually handle the interrupt. While doing the handling, the hardware can=
 indicate the interrupt
> again. But since it is masked nothing happened. This avoids your interrup=
t handler going
> recursive.
> Once the handler has finished, the interrupt is unmasked. At this point i=
t actually fires, triggering
> the interrupt handler again.

Asmaa>> mlxbf2_gpio_irq_enable seems to be called only once when the driver=
 is loaded.
And I will actually remove mlxbf2_gpio_irq_ack because it is not being call=
ed at all.
After further investigation, that function is called via chained_irq_enter =
which is itself invoked in
the interrupt handler. It should have looked something like this:

static irqreturn_t mlxbf2_gpio_irq_handler(int irq, void *ptr)
{
    chained_irq_enter(gc->irq->chip, desc);
    // rest of the code here
    chained_irq_exit(gc->irq->chip, desc);
}

But in our case, we decided to directly request the irq  instead of passing=
 a flow-handler to
gpiochip_set_chained_irqchip, because the irq has to be marked as shared (I=
RQF_SHARED).
gpio-mt7621.c does something similar.
Moreover, whenever an interrupt is fired by HW, it is automatically disable=
d/masked until
it is explicitly cleared by Software. And this line takes care of it in mlx=
bf2_gpio_irq_handler:
writel(pending, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE);

After a HW reset, all gpio interrupts are disabled by default by HW. The HW=
 will not signal
any gpio interrupt as long as all bits in YU_GPIO_CAUSE_OR_EVTEN0 are 0.
In mlxbf2_gpio_irq_enable, we configure a specific gpio as an interrupt by =
writing 1 to
YU_GPIO_CAUSE_OR_EVTEN0. I just wanted to make sure there is no trash value=
 in
YU_GPIO_CAUSE_OR_CLRCAUSE before enabling gpio interrupt support.
So pending interrupts in YU_GPIO_CAUSE_OR_CLRCAUSE only matters if
YU_GPIO_CAUSE_OR_EVTEN0 is set accordingly.
Does this answer your question?

> Please also get your email client fixed. I wrap my emails at around 75 ch=
aracters. Your mailer
> has destroyed it. Your text should also be wrapped at about 75 characters=
.

Asmaa>> Sorry about that. I wrapped my outlook emails around 75 characters,=
 I hope it works.