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 00:25:25 +0100 [thread overview] Message-ID: <0bff78c1-a1d0-9631-fbf4-e0d1ef1264ea@suse.de> (raw) In-Reply-To: <a34e00cac16899b53d0b6445f0e81f4c@www.loen.fr> 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. 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? So given that we're iterating over reg_isr above, we could probably drop the mask check here... In addition there are MIS_[UMSK_]ISR_SWC and MIS_SETTING_SWC registers for Secure World, and MIS_FAST_INT_EN_* and MIS_FAST_ISR as well as various GPIO interrupt registers. Regards, Andreas >> + } >> + >> + chained_irq_exit(chip, desc); >> +} >> + >> +static void rtd1195_mux_mask_irq(struct irq_data *data) >> +{ >> + struct rtd1195_irq_mux_data *mux_data = >> irq_data_get_irq_chip_data(data); >> + >> + writel_relaxed(BIT(data->hwirq), mux_data->reg_isr); >> +} >> + >> +static void rtd1195_mux_unmask_irq(struct irq_data *data) >> +{ >> + struct rtd1195_irq_mux_data *mux_data = >> irq_data_get_irq_chip_data(data); >> + >> + writel_relaxed(BIT(data->hwirq), mux_data->reg_umsk_isr); >> +} >> + >> +static void rtd1195_mux_enable_irq(struct irq_data *data) >> +{ >> + struct rtd1195_irq_mux_data *mux_data = >> irq_data_get_irq_chip_data(data); >> + unsigned long flags; >> + u32 mask; >> + >> + mask = mux_data->info->isr_to_int_en_mask[data->hwirq]; >> + if (!mask) >> + return; > > How can this happen? You've mapped the interrupt, so it exists. > I can't see how you can decide to fail such enable. > >> + >> + raw_spin_lock_irqsave(&mux_data->lock, flags); >> + >> + mux_data->scpu_int_en |= mask; >> + writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en); >> + >> + raw_spin_unlock_irqrestore(&mux_data->lock, flags); >> +} >> + >> +static void rtd1195_mux_disable_irq(struct irq_data *data) >> +{ >> + struct rtd1195_irq_mux_data *mux_data = >> irq_data_get_irq_chip_data(data); >> + unsigned long flags; >> + u32 mask; >> + >> + mask = mux_data->info->isr_to_int_en_mask[data->hwirq]; >> + if (!mask) >> + return; >> + >> + raw_spin_lock_irqsave(&mux_data->lock, flags); >> + >> + mux_data->scpu_int_en &= ~mask; >> + writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en); >> + >> + raw_spin_unlock_irqrestore(&mux_data->lock, flags); >> +} >> + >> +static struct irq_chip rtd1195_mux_irq_chip = { >> + .name = "rtd1195-mux", >> + .irq_mask = rtd1195_mux_mask_irq, >> + .irq_unmask = rtd1195_mux_unmask_irq, >> + .irq_enable = rtd1195_mux_enable_irq, >> + .irq_disable = rtd1195_mux_disable_irq, >> +}; > > [...] > > Although the code is pretty clean, the way you drive the HW looks > suspicious, and requires clarification. > > Thanks, > > M. -- SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer HRB 36809 (AG Nürnberg)
WARNING: multiple messages have this Message-ID (diff)
From: "Andreas Färber" <afaerber@suse.de> To: Marc Zyngier <maz@kernel.org> Cc: James Tai <james.tai@realtek.com>, Aleix Roca Nonell <kernelrocks@gmail.com>, linux-realtek-soc@lists.infradead.org, linux-kernel@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>, linux-arm-kernel@lists.infradead.org, Jason Cooper <jason@lakedaemon.net> Subject: Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver Date: Wed, 20 Nov 2019 00:25:25 +0100 [thread overview] Message-ID: <0bff78c1-a1d0-9631-fbf4-e0d1ef1264ea@suse.de> (raw) In-Reply-To: <a34e00cac16899b53d0b6445f0e81f4c@www.loen.fr> 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. 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? So given that we're iterating over reg_isr above, we could probably drop the mask check here... In addition there are MIS_[UMSK_]ISR_SWC and MIS_SETTING_SWC registers for Secure World, and MIS_FAST_INT_EN_* and MIS_FAST_ISR as well as various GPIO interrupt registers. Regards, Andreas >> + } >> + >> + chained_irq_exit(chip, desc); >> +} >> + >> +static void rtd1195_mux_mask_irq(struct irq_data *data) >> +{ >> + struct rtd1195_irq_mux_data *mux_data = >> irq_data_get_irq_chip_data(data); >> + >> + writel_relaxed(BIT(data->hwirq), mux_data->reg_isr); >> +} >> + >> +static void rtd1195_mux_unmask_irq(struct irq_data *data) >> +{ >> + struct rtd1195_irq_mux_data *mux_data = >> irq_data_get_irq_chip_data(data); >> + >> + writel_relaxed(BIT(data->hwirq), mux_data->reg_umsk_isr); >> +} >> + >> +static void rtd1195_mux_enable_irq(struct irq_data *data) >> +{ >> + struct rtd1195_irq_mux_data *mux_data = >> irq_data_get_irq_chip_data(data); >> + unsigned long flags; >> + u32 mask; >> + >> + mask = mux_data->info->isr_to_int_en_mask[data->hwirq]; >> + if (!mask) >> + return; > > How can this happen? You've mapped the interrupt, so it exists. > I can't see how you can decide to fail such enable. > >> + >> + raw_spin_lock_irqsave(&mux_data->lock, flags); >> + >> + mux_data->scpu_int_en |= mask; >> + writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en); >> + >> + raw_spin_unlock_irqrestore(&mux_data->lock, flags); >> +} >> + >> +static void rtd1195_mux_disable_irq(struct irq_data *data) >> +{ >> + struct rtd1195_irq_mux_data *mux_data = >> irq_data_get_irq_chip_data(data); >> + unsigned long flags; >> + u32 mask; >> + >> + mask = mux_data->info->isr_to_int_en_mask[data->hwirq]; >> + if (!mask) >> + return; >> + >> + raw_spin_lock_irqsave(&mux_data->lock, flags); >> + >> + mux_data->scpu_int_en &= ~mask; >> + writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en); >> + >> + raw_spin_unlock_irqrestore(&mux_data->lock, flags); >> +} >> + >> +static struct irq_chip rtd1195_mux_irq_chip = { >> + .name = "rtd1195-mux", >> + .irq_mask = rtd1195_mux_mask_irq, >> + .irq_unmask = rtd1195_mux_unmask_irq, >> + .irq_enable = rtd1195_mux_enable_irq, >> + .irq_disable = rtd1195_mux_disable_irq, >> +}; > > [...] > > Although the code is pretty clean, the way you drive the HW looks > suspicious, and requires clarification. > > Thanks, > > M. -- SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer HRB 36809 (AG Nürnberg) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-11-19 23:25 UTC|newest] Thread overview: 40+ 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 ` 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 ` Andreas Färber 2019-11-19 2:19 ` [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver Andreas Färber 2019-11-19 2:19 ` Andreas Färber 2019-11-19 12:01 ` Marc Zyngier 2019-11-19 12:01 ` Marc Zyngier 2019-11-19 20:56 ` Andreas Färber 2019-11-19 20:56 ` Andreas Färber 2019-11-19 22:29 ` Marc Zyngier 2019-11-19 22:29 ` Marc Zyngier 2019-11-19 23:33 ` Andreas Färber 2019-11-19 23:33 ` Andreas Färber 2019-11-20 10:20 ` Marc Zyngier 2019-11-20 10:20 ` Marc Zyngier 2019-11-20 13:34 ` Andreas Färber 2019-11-20 13:34 ` Andreas Färber 2019-11-20 14:32 ` Marc Zyngier 2019-11-20 14:32 ` Marc Zyngier 2019-11-19 23:25 ` Andreas Färber [this message] 2019-11-19 23:25 ` Andreas Färber 2019-11-20 10:18 ` Marc Zyngier 2019-11-20 10:18 ` Marc Zyngier 2019-11-20 12:12 ` Andreas Färber 2019-11-20 12:12 ` Andreas Färber 2019-11-20 12:23 ` Marc Zyngier 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 ` 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 ` 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 ` 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 ` 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 ` 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 2019-11-19 2:19 ` 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=0bff78c1-a1d0-9631-fbf4-e0d1ef1264ea@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 \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.