[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <8bdd7f5f-971c-4d62-8fad-b790ba756d5d@vaisala.com>
Date: Fri, 5 Sep 2025 08:11:13 +0300
From: Tapio Reijonen <tapio.reijonen@...sala.com>
To: Hugo Villeneuve <hugo@...ovil.com>
Cc: Jiri Slaby <jirislaby@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Alexander Shiyan <shc_work@...l.ru>,
Hugo Villeneuve <hvilleneuve@...onoff.com>, linux-kernel@...r.kernel.org,
linux-serial@...r.kernel.org
Subject: Re: [PATCH] serial: max310x: improve interrupt handling
Hi,
On 9/4/25 19:36, Hugo Villeneuve wrote:
> Hi,
>
> On Thu, 4 Sep 2025 14:50:16 +0300
> Tapio Reijonen <tapio.reijonen@...sala.com> wrote:
>
>> Hi,
>>
>> On 9/4/25 10:53, Jiri Slaby wrote:
>>> On 03. 09. 25, 11:23, Tapio Reijonen wrote:
>>>> When there is a heavy load of receiving characters to all
>>>> four UART's, the warning 'Hardware RX FIFO overrun' is
>>>> sometimes detected.
>>>> The current implementation always service first UART3 until
>>>> no more interrupt and then service another UARTs.
>>>>
>>>> This commit improve interrupt service routine to handle all
>>>> interrupt sources, e.g. UARTs when a global IRQ is detected.
>>>>
>>>> Signed-off-by: Tapio Reijonen <tapio.reijonen@...sala.com>
>>>> ---
>>>> drivers/tty/serial/max310x.c | 21 ++++++++++++++++-----
>>>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
>>>> index
>>>> ce260e9949c3c268e706b2615d6fc01adc21e49b..3234ed7c688ff423d25a007ed8b938b249ae0b82 100644
>>>> --- a/drivers/tty/serial/max310x.c
>>>> +++ b/drivers/tty/serial/max310x.c
>>>> @@ -824,15 +824,26 @@ static irqreturn_t max310x_ist(int irq, void
>>>> *dev_id)
>>>> if (s->devtype->nr > 1) {
>>>> do {
>>>> - unsigned int val = ~0;
>>>> + unsigned int val;
>>>> + unsigned int global_irq = ~0;
>>>> + int port;
>>>> WARN_ON_ONCE(regmap_read(s->regmap,
>>>> - MAX310X_GLOBALIRQ_REG, &val));
>>>> - val = ((1 << s->devtype->nr) - 1) & ~val;
>>>> + MAX310X_GLOBALIRQ_REG, &global_irq));
>>>> +
>>>> + val = ((1 << s->devtype->nr) - 1) & ~global_irq;
>>>
>>> This is horrid. Use GENMASK() (or BIT() below) instead. Likely, you want
>>> a local var storing the mask (the first part before the &).
>>>
>> val = GENMASK(s->devtype->nr - 1, 0) & ~global_irq;>> if
>> (!val)
>>>> break;
>>>> - if (max310x_port_irq(s, fls(val) - 1) == IRQ_HANDLED)
>>>> - handled = true;
>>>> +
>>>> + do {
>>>> + port = fls(val) - 1;
>>>> + if (max310x_port_irq(s, port) == IRQ_HANDLED)
>>>> + handled = true;
>>>> +
>>>> + global_irq |= 1 << port;
>>>> + val = ((1 << s->devtype->nr) - 1) & ~global_irq;
>>>> + } while (val);
>>>
>>> Actually, does it have to be from the end? I am thinking of
>>> for_each_and_bit()...
>>>
>>>> } while (1);
>>>> } else {
>>>> if (max310x_port_irq(s, 0) == IRQ_HANDLED)
>
> Combining what I suggested earlier, with Jiri's comments but using
> for_each_clear_bit() allow to get rid of the original horrid mask:
>
Thanks about suggested clear example. I'll test this in HW.
> ---
> if (s->devtype->nr > 1) {
> do {
> unsigned int val = ~0;
> + unsigned long channel;
> + unsigned long irq;
> + bool read_reg_done = true;
>
> WARN_ON_ONCE(regmap_read(s->regmap,
> MAX310X_GLOBALIRQ_REG, &val));
> - val = ((1 << s->devtype->nr) - 1) & ~val;
> - if (!val)
> + irq = val;
> +
> + for_each_clear_bit(channel, &irq, s->devtype->nr) {
> + read_reg_done = false;
> +
> + if (max310x_port_irq(s, channel) == IRQ_HANDLED)
> + handled = true;
> + }
> +
> + if (read_reg_done)
> break;
> - if (max310x_port_irq(s, fls(val) - 1) == IRQ_HANDLED)
> - handled = true;
> } while (1);
> ---
>
>
--
Many thanks,
Tapio Reijonen
Powered by blists - more mailing lists