linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Andreas Färber" <afaerber@suse.de>
To: Marc Zyngier <maz@kernel.org>
Cc: linux-realtek-soc@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Aleix Roca Nonell <kernelrocks@gmail.com>,
	James Tai <james.tai@realtek.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Jason Cooper <jason@lakedaemon.net>
Subject: Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver
Date: Wed, 20 Nov 2019 13:12:31 +0100	[thread overview]
Message-ID: <37b3b5d3-b3c8-b513-f8b5-9054f32a4b53@suse.de> (raw)
In-Reply-To: <8137861d0a89dd246b3334ac596da8be@www.loen.fr>

Am 20.11.19 um 11:18 schrieb Marc Zyngier:
> On 2019-11-19 23:25, Andreas Färber wrote:
>> Am 19.11.19 um 13:01 schrieb Marc Zyngier:
>>> On 2019-11-19 02:19, Andreas Färber wrote:
>>>> diff --git a/drivers/irqchip/irq-rtd1195-mux.c
>>>> b/drivers/irqchip/irq-rtd1195-mux.c
>>>> new file mode 100644
>>>> index 000000000000..e6b08438b23c
>>>> --- /dev/null
>>>> +++ b/drivers/irqchip/irq-rtd1195-mux.c
>> [...]
>>>> +static void rtd1195_mux_irq_handle(struct irq_desc *desc)
>>>> +{
>>>> +    struct rtd1195_irq_mux_data *data =
>>>> irq_desc_get_handler_data(desc);
>>>> +    struct irq_chip *chip = irq_desc_get_chip(desc);
>>>> +    u32 isr, mask;
>>>> +    int i;
>>>> +
>>>> +    chained_irq_enter(chip, desc);
>>>> +
>>>> +    isr = readl_relaxed(data->reg_isr);
>>>> +
>>>> +    while (isr) {
>>>> +        i = __ffs(isr);
>>>> +        isr &= ~BIT(i);
>>>> +
>>>> +        mask = data->info->isr_to_int_en_mask[i];
>>>> +        if (mask && !(data->scpu_int_en & mask))
>>>> +            continue;
>>>> +
>>>> +        if (!generic_handle_irq(irq_find_mapping(data->domain, i)))
>>>> +            writel_relaxed(BIT(i), data->reg_isr);
>>>
>>> What does this write do exactly? It is the same thing as a 'mask',
>>> which is pretty odd. So either:
>>>
>>> - this is not doing anything and your 'mask' callback is bogus
>>>   (otherwise you'd never have more than a single interrupt)
>>>
>>> - or this is an ACK operation, and this should be described as
>>>   such (and then fix the mask/unmask/enable/disable mess that
>>>   results from it).
>>
>> This is supposed to be an ACK, i.e. clear-1-bits operation.
> 
> If it is an ACK, model it as such, and do not open-code it.

I have found an .irq_ack callback - moving this there appears to work.

Alternatively there is an irq_eoi callback and an IRQCHIP_EOI_IF_HANDLED
flag.

It would really help me if you spelled out explicitly where you think I
should be moving code, as the documentation in irq.h is not all that
helpful in terms of when are they called and what should be done there.
In case not obvious, this is my first irqchip driver.

>>
>> The BSP had extended various drivers, such as 8250 UART, to do this ack
>> in their interrupt handler through an additional DT reg region. I tried
>> to clean that up by handling it centrally here in the irqchip driver.
>>
>>>
>>> as I can't see how the same register can be used for both purposes.
>>> You should be able to verify this experimentally, even without
>>> documentation.
>>
>> There are three registers here:
>>
>> MIS_UMSK_ISR    - MISC unmasked interrupt status register
>> MIS_ISR         - MISC   masked interrupt status register
>> MIS_SCPU_INT_EN - MISC SCPU interrupt enable register
>>
>> The latter is a regular R/W register; the former two have a write_data
>> field as BIT(0), with 1 indicating a write vs. 0 indicating clear, RAZ.
>>
>> By enabling/disabling in _SCPU_INT_EN we mask/unmask them in _ISR but
>> not in _UMSK_ISR.
>>
>> Does that shed any more light?
> 
> None whatsoever. Your mask callback doesn't make any sense, since it
> actually acks the interrupt. My gut feeling is that your enable/disable
> should really be mask/unmask.

Renaming enable -> unmask and disable -> mask works okay.

Thanks,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

  reply	other threads:[~2019-11-20 12:12 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-19  2:19 [PATCH v4 0/8] ARM: Realtek RTD1195/RTD1295/RTD1395 IRQ mux Andreas Färber
2019-11-19  2:19 ` [PATCH v4 1/8] dt-bindings: interrupt-controller: Add Realtek RTD1195/RTD1295 mux Andreas Färber
2019-11-19  2:19 ` [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver Andreas Färber
2019-11-19 12:01   ` Marc Zyngier
2019-11-19 20:56     ` Andreas Färber
2019-11-19 22:29       ` Marc Zyngier
2019-11-19 23:33         ` Andreas Färber
2019-11-20 10:20           ` Marc Zyngier
2019-11-20 13:34             ` Andreas Färber
2019-11-20 14:32               ` Marc Zyngier
2019-11-19 23:25     ` Andreas Färber
2019-11-20 10:18       ` Marc Zyngier
2019-11-20 12:12         ` Andreas Färber [this message]
2019-11-20 12:23           ` Marc Zyngier
2019-11-19  2:19 ` [PATCH v4 3/8] arm64: dts: realtek: rtd129x: Add irq muxes and UART interrupts Andreas Färber
2019-11-19  2:19 ` [PATCH v4 4/8] irqchip: rtd1195-mux: Add RTD1195 definitions Andreas Färber
2019-11-19  2:19 ` [PATCH v4 5/8] ARM: dts: rtd1195: Add irq muxes and UART interrupts Andreas Färber
2019-11-19  2:19 ` [PATCH v4 6/8] dt-bindings: interrupt-controller: rtd1195-mux: Add RTD1395 Andreas Färber
2019-11-19  2:19 ` [PATCH v4 7/8] irqchip: rtd1195-mux: Add RTD1395 definitions Andreas Färber
2019-11-19  2:19 ` [PATCH v4 8/8] arm64: dts: realtek: rtd139x: Add irq muxes and UART interrupts Andreas Färber

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=37b3b5d3-b3c8-b513-f8b5-9054f32a4b53@suse.de \
    --to=afaerber@suse.de \
    --cc=james.tai@realtek.com \
    --cc=jason@lakedaemon.net \
    --cc=kernelrocks@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-realtek-soc@lists.infradead.org \
    --cc=maz@kernel.org \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).