[RFC 0/3] arm64: Realtek RTD1295 IRQ mux

[RFC 0/3] arm64: Realtek RTD1295 IRQ mux

[Andreas Färber]

Hello,

This series adds two IRQ muxes for the Realtek RTD1295 SoC.
Based on the reset controller series.

There being no public source code for RTD1295, the implementation is based on
register offsets seen in the DT, split up into two separate nodes.

A workaround for hanging timer initialization was taken from QNAP's rtk119x
GPL code dump and is not yet fully understood; that code also contains a quirk
for i2c3 in the iso mux that is not yet verified to be needed on RTD1295,
for lack of i2c driver, assuming a linear intr_status/intr_en mapping for now.

My convention here is to use a compatible string for the model that I've
tested (RTD1295) but a file name of rtd119x to indicate its RTD1195 heritage.

More experimental patches at:
https://github.com/afaerber/linux/commits/rtd1295-next

Have a lot of fun!

Cheers,
Andreas

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Roc He <hepeng@zidoo.tv>
Cc: 蒋丽琴 <jiang.liqin@geniatech.com>
Cc: devicetree@vger.kernel.org

Andreas Färber (3):
  dt-bindings: interrupt-controller: Add Realtek RTD1295
  arm64: dts: realtek: Add irq mux to RTD1295
  irqchip: Add Realtek RTD1295 mux driver

 .../interrupt-controller/realtek,rtd119x-mux.txt   |  28 +++
 arch/arm64/boot/dts/realtek/rtd1295.dtsi           |  22 +++
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-rtd119x-mux.c                  | 201 +++++++++++++++++++++
 4 files changed, 252 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
 create mode 100644 drivers/irqchip/irq-rtd119x-mux.c

-- 
2.12.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC 1/3] dt-bindings: interrupt-controller: Add Realtek RTD1295

[Andreas Färber]

Add binding for Realtek RTD1295 IRQ mux.

Signed-off-by: Andreas Färber <afaerber-l3A5Bk7waGM@public.gmane.org>
---
 .../interrupt-controller/realtek,rtd119x-mux.txt   | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
new file mode 100644
index 000000000000..4b43370e3d74
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
@@ -0,0 +1,28 @@
+Realtek RTD119x/129x IRQ Mux Controller
+=======================================
+
+Required properties:
+
+- compatible           :  Should be one of the following:
+                          - "realtek,rtd1295-irq-mux"
+                          - "realtek,rtd1295-iso-irq-mux"
+- reg                  :  Specifies base physical address and size of the registers.
+- interrupts           :  Specifies the interrupt line which is mux'ed.
+- interrupt-controller :  Presence indicates the node as interrupt controller.
+- #interrupt-cells     :  Shall be 1. See common bindings in interrupt.txt.
+
+
+Optional properties:
+
+See interrupt.txt for common bindings.
+
+
+Example:
+
+	interrupt-controller@98007000 {
+		compatible = "realtek,rtd1295-iso-irq-mux";
+		reg = <0x98007000 0x100>;
+		interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+	};
-- 
2.12.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/3] dt-bindings: interrupt-controller: Add Realtek RTD1295

[Rob Herring]

On Thu, Aug 17, 2017 at 12:11:38PM +0200, Andreas Färber wrote:
> Add binding for Realtek RTD1295 IRQ mux.
> 
> Signed-off-by: Andreas Färber <afaerber-l3A5Bk7waGM@public.gmane.org>
> ---
>  .../interrupt-controller/realtek,rtd119x-mux.txt   | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
> new file mode 100644
> index 000000000000..4b43370e3d74
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
> @@ -0,0 +1,28 @@
> +Realtek RTD119x/129x IRQ Mux Controller
> +=======================================
> +
> +Required properties:
> +
> +- compatible           :  Should be one of the following:
> +                          - "realtek,rtd1295-irq-mux"
> +                          - "realtek,rtd1295-iso-irq-mux"
> +- reg                  :  Specifies base physical address and size of the registers.
> +- interrupts           :  Specifies the interrupt line which is mux'ed.
> +- interrupt-controller :  Presence indicates the node as interrupt controller.
> +- #interrupt-cells     :  Shall be 1. See common bindings in interrupt.txt.
> +
> +
> +Optional properties:
> +
> +See interrupt.txt for common bindings.

What else would apply here? interrupt-parent is the only thing I see, 
but that's really implicit anyway.

> +
> +
> +Example:
> +
> +	interrupt-controller@98007000 {
> +		compatible = "realtek,rtd1295-iso-irq-mux";
> +		reg = <0x98007000 0x100>;
> +		interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +	};
> -- 
> 2.12.3
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/3] dt-bindings: interrupt-controller: Add Realtek RTD1295

[Andreas Färber]

Am 22.08.2017 um 04:14 schrieb Rob Herring:
> On Thu, Aug 17, 2017 at 12:11:38PM +0200, Andreas Färber wrote:
>> Add binding for Realtek RTD1295 IRQ mux.
>>
>> Signed-off-by: Andreas Färber <afaerber-l3A5Bk7waGM@public.gmane.org>
>> ---
>>  .../interrupt-controller/realtek,rtd119x-mux.txt   | 28 ++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
>> new file mode 100644
>> index 000000000000..4b43370e3d74
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd119x-mux.txt
>> @@ -0,0 +1,28 @@
>> +Realtek RTD119x/129x IRQ Mux Controller
>> +=======================================
>> +
>> +Required properties:
>> +
>> +- compatible           :  Should be one of the following:
>> +                          - "realtek,rtd1295-irq-mux"
>> +                          - "realtek,rtd1295-iso-irq-mux"
>> +- reg                  :  Specifies base physical address and size of the registers.
>> +- interrupts           :  Specifies the interrupt line which is mux'ed.
>> +- interrupt-controller :  Presence indicates the node as interrupt controller.
>> +- #interrupt-cells     :  Shall be 1. See common bindings in interrupt.txt.
>> +
>> +
>> +Optional properties:
>> +
>> +See interrupt.txt for common bindings.
> 
> What else would apply here? interrupt-parent is the only thing I see, 
> but that's really implicit anyway.

interrupt-names would be another optional property not needed today.
interrupt-controller and #interrupt-cells are common properties, too.

I'll drop both lines if you prefer. Otherwise okay?

Regards,
Andreas

>> +
>> +
>> +Example:
>> +
>> +	interrupt-controller@98007000 {
>> +		compatible = "realtek,rtd1295-iso-irq-mux";
>> +		reg = <0x98007000 0x100>;
>> +		interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
>> +		interrupt-controller;
>> +		#interrupt-cells = <1>;
>> +	};
>> -- 
>> 2.12.3

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 2/3] arm64: dts: realtek: Add irq mux to RTD1295

[Andreas Färber]

Update UART nodes with interrupts.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 arch/arm64/boot/dts/realtek/rtd1295.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/realtek/rtd1295.dtsi b/arch/arm64/boot/dts/realtek/rtd1295.dtsi
index 2d2d84b573e3..77063e984db9 100644
--- a/arch/arm64/boot/dts/realtek/rtd1295.dtsi
+++ b/arch/arm64/boot/dts/realtek/rtd1295.dtsi
@@ -112,6 +112,14 @@
 			#reset-cells = <1>;
 		};
 
+		iso_irq_mux: interrupt-controller@98007000 {
+			compatible = "realtek,rtd1295-iso-irq-mux";
+			reg = <0x98007000 0x100>;
+			interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
 		iso_reset: reset-controller@98007088 {
 			compatible = "realtek,rtd1295-reset";
 			reg = <0x98007088 0x4>;
@@ -124,16 +132,28 @@
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clock-frequency = <27000000>;
+			interrupt-parent = <&iso_irq_mux>;
+			interrupts = <2>;
 			resets = <&iso_reset RTD1295_ISO_RSTN_UR0>;
 			status = "disabled";
 		};
 
+		irq_mux: interrupt-controller@9801b000 {
+			compatible = "realtek,rtd1295-irq-mux";
+			reg = <0x9801b000 0x100>;
+			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
 		uart1: serial@9801b200 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x9801b200 0x100>;
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clock-frequency = <432000000>;
+			interrupt-parent = <&irq_mux>;
+			interrupts = <3>, <5>;
 			resets = <&reset2 RTD1295_RSTN_UR1>;
 			status = "disabled";
 		};
@@ -144,6 +164,8 @@
 			reg-shift = <2>;
 			reg-io-width = <4>;
 			clock-frequency = <432000000>;
+			interrupt-parent = <&irq_mux>;
+			interrupts = <8>, <13>;
 			resets = <&reset2 RTD1295_RSTN_UR2>;
 			status = "disabled";
 		};
-- 
2.12.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC 3/3] irqchip: Add Realtek RTD1295 mux driver

[Andreas Färber]

Hi Marc,

Am 18.08.2017 um 12:53 schrieb Marc Zyngier:
> On 17/08/17 11:11, Andreas Färber wrote:
>> This irq mux driver is derived from the RTD1295 vendor DT and assumes a linear
>> mapping between intr_en and intr_status registers. Code for RTD119x indicates
>> this may not always be the case (i2c_3).
>>
>> The register initialization was copied from QNAP's mach-rtk119x/rtk_irq_mux.c
>> as a boot fix, without full insights into what exactly this is changing (TODO).
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  drivers/irqchip/Makefile          |   1 +
>>  drivers/irqchip/irq-rtd119x-mux.c | 201 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 202 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-rtd119x-mux.c
>>
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index e88d856cc09c..46202a0b7d96 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
>>  obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
>>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>> +obj-$(CONFIG_ARCH_REALTEK)		+= irq-rtd119x-mux.o

Should this get its own config option? It would get shared by ARM64 and
future potential ARM, so no strict need for composite expressions, and
there's examples of both ways above.

BTW I was surprised that the Makefile does not seem to have a consistent
order, leading to potential conflicts when adding things in the bottom.
Have you guys thought about enforcing alphabetical order by either
config option or file name?

>> diff --git a/drivers/irqchip/irq-rtd119x-mux.c b/drivers/irqchip/irq-rtd119x-mux.c
>> new file mode 100644
>> index 000000000000..c6c1ba126bf3
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-rtd119x-mux.c
>> @@ -0,0 +1,201 @@
>> +/*
>> + * Realtek RTD129x IRQ mux
>> + *
>> + * Copyright (c) 2017 Andreas Färber
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <linux/io.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/slab.h>
>> +
>> +struct rtd119x_irq_mux_info {
>> +	unsigned intr_status;
>> +	unsigned intr_en;
> 
> nit: these would be better named with a "_offset" suffix, in order to
> better distinguish them from the structure below.

True, renamed. See also discussion in the bottom.

>> +};
>> +
>> +struct rtd119x_irq_mux_data {
>> +	void __iomem *intr_status;
>> +	void __iomem *intr_en;
>> +	int irq;
>> +	struct irq_domain *domain;
>> +	spinlock_t lock;
>> +};
>> +
>> +static void rtd119x_mux_irq_handle(struct irq_desc *desc)
>> +{
>> +	struct rtd119x_irq_mux_data *data = irq_desc_get_handler_data(desc);
>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	u32 intr_en, intr_status, status;
>> +	int ret;
>> +
>> +	chained_irq_enter(chip, desc);
>> +
>> +	spin_lock(&data->lock);
>> +	intr_en     = readl(data->intr_en);
>> +	intr_status = readl(data->intr_status);
> 
> All the readl/writel should be turned into their _relaxed versions.

Hmm, so I use readl/writel by default. The old downstream code was using
__raw_readl/__raw_writel, but my initial code appeared to work, so I
left it that way.

Can you explain why _relaxed, so that I can properly choose next time?
I see the difference is __iormb(), but when do I need that? Thanks.

>> +	spin_unlock(&data->lock);
>> +
>> +	status = intr_status & intr_en;
>> +	if (status != 0) {
>> +		unsigned irq = __ffs(status);
>> +		ret = generic_handle_irq(irq_find_mapping(data->domain, irq));
>> +		if (ret == 0) {
>> +			spin_lock(&data->lock);
>> +			intr_status = readl(data->intr_status);
>> +			intr_status |= BIT(irq - 1);
>> +			writel(intr_status, data->intr_status);
>> +			spin_unlock(&data->lock);
>> +		}
>> +	}
> 
> And what if status is 0? We keep the lock held forever?

Which lock? Locking is only done for consistently reading en and status,
and for the read-modify-write of status. Both look balanced to me, and
no hangs have been observed in testing.

__ffs: "Undefined if no bit exists, so code should check against 0 first."

If status is 0 then there's no pending enabled interrupt and thus
nothing to do.

The downstream code modified all drivers (e.g., 8250 serial) to
acknowledge their interrupts inside the drivers, and instead it had code
that complained and cleared them here.
I wanted to keep the irqchip abstraction, so I am always acknowledging
the interrupt if generic_handle_irq does not run into an error - I
understood that this is not the same as actually having been
successfully handled, but I did not see a better way for how to do this
in generic code.
(I am facing a similar mux situation with FM4, so input appreciated on
whether these are right hooks to use - for FM4 I got stuck trying to
adopt the hierarchical API and resorted to the simpler approach here.)

Would you rather have realtek,intr_en and realtek,intr_status properties
in the relevant device nodes and drivers and acknowledge the interrupts
from their respective interrupt handlers and then ignore it here?

>> +
>> +	chained_irq_exit(chip, desc);
>> +}
>> +
>> +static void rtd119x_mux_mask_irq(struct irq_data *data)
>> +{
>> +	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
>> +	u32 intr_status;
>> +
>> +	intr_status = readl(mux_data->intr_status);
>> +	intr_status |= BIT(data->hwirq);
>> +	writel(intr_status, mux_data->intr_status);
> 
> So you need a lock in the chained handler, but you don't need one here?
> It doesn't feel right.

Good point. I had started without locks; should add them here and below.

>> +}
>> +
>> +static void rtd119x_mux_unmask_irq(struct irq_data *data)
>> +{
>> +	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
>> +	u32 intr_en;
>> +
>> +	intr_en = readl(mux_data->intr_en);
>> +	intr_en |= BIT(data->hwirq);
>> +	writel(intr_en, mux_data->intr_en);
> 
> Same thing here.
> 
>> +}
>> +
>> +static int rtd119x_mux_set_affinity(struct irq_data *d,
>> +			const struct cpumask *mask_val, bool force)
>> +{
>> +	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(d);
>> +	struct irq_chip *chip = irq_get_chip(mux_data->irq);
>> +	struct irq_data *data = irq_get_irq_data(mux_data->irq);
>> +
>> +	if (chip && chip->irq_set_affinity)
>> +		return chip->irq_set_affinity(data, mask_val, force);
> 
> I'm always worried when I see this. It means that all of the interrupts
> on the secondary chip move in one go, without the core code noticing.

You mean it affects all 32 mux'ed IRQs as a side effect here? True.

> One of these days, we'll have to address it.

Note that currently I have only a single core up on RTD1295, so affinity
is not really tested. (They use a custom "rtk-spin-table" implementation
that I have no source code or documentation for, and I haven't made
guesses yet, nor do I think that would be acceptable in mainline arm64.)

This hook appears to be optional, so we could just drop it for now, or
always return -EINVAL if preferred over forwarding to the GIC?

>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static struct irq_chip rtd119x_mux_irq_chip = {
>> +	.name			= "rtd119x-mux",
>> +	.irq_mask		= rtd119x_mux_mask_irq,
>> +	.irq_unmask		= rtd119x_mux_unmask_irq,
>> +	.irq_set_affinity	= rtd119x_mux_set_affinity,
>> +};
>> +
>> +static int rtd119x_mux_irq_domain_xlate(struct irq_domain *d,
>> +		struct device_node *controller,
>> +		const u32 *intspec, unsigned int intsize,
>> +		unsigned long *out_hwirq,
>> +		unsigned int *out_type)
>> +{
>> +	if (irq_domain_get_of_node(d) != controller)
>> +		return -EINVAL;
>> +
>> +	if (intsize < 1)
>> +		return -EINVAL;
>> +
>> +	*out_hwirq = intspec[0];
>> +	*out_type = 0;
>> +
>> +	return 0;
>> +}
> 
> Please use irq_domain_xlate_onecell() instead.

Thanks for the pointer, done.

>> +
>> +static int rtd119x_mux_irq_domain_map(struct irq_domain *d,
>> +		unsigned int irq, irq_hw_number_t hw)
>> +{
>> +	struct rtd119x_irq_mux_data *data = d->host_data;
>> +
>> +	irq_set_chip_and_handler(irq, &rtd119x_mux_irq_chip, handle_level_irq);
>> +	irq_set_chip_data(irq, data);
>> +	irq_set_probe(irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct irq_domain_ops rtd119x_mux_irq_domain_ops = {
>> +	.xlate	= rtd119x_mux_irq_domain_xlate,
>> +	.map	= rtd119x_mux_irq_domain_map,
>> +};
[...]
>> +static const struct rtd119x_irq_mux_info rtd1295_iso_irq_mux_info = {
>> +	.intr_status	= 0x0,
>> +	.intr_en	= 0x40,
>> +};
>> +
>> +static int __init rtd1295_iso_irq_mux_init(struct device_node *node,
>> +					   struct device_node *parent)
>> +{
>> +	return rtd119x_irq_mux_init(node, parent, &rtd1295_iso_irq_mux_info);
>> +}
>> +IRQCHIP_DECLARE(rtd1295_iso_mux, "realtek,rtd1295-iso-irq-mux", rtd1295_iso_irq_mux_init);
>> +
>> +static const struct rtd119x_irq_mux_info rtd1295_irq_mux_info = {
>> +	.intr_status	= 0xc,
>> +	.intr_en	= 0x80,
>> +};
>> +
>> +static int __init rtd1295_irq_mux_init(struct device_node *node,
>> +				       struct device_node *parent)
>> +{
>> +	return rtd119x_irq_mux_init(node, parent, &rtd1295_irq_mux_info);
>> +}
>> +IRQCHIP_DECLARE(rtd1295_mux, "realtek,rtd1295-irq-mux", rtd1295_irq_mux_init);

This was a quick hack to cover both instances. I am not yet sure how to
best model the case of e.g. en bit 28 corresponding to status bit 23 in
the struct. An index array of all 32 bits with special value for absent,
to iteratively or the bits instead of masking them?

We might use of_match_node() to get the info pointer in one generic init
function instead of having per-compatible init functions down here, but
then we'd have the compatibles duplicated (compared to platform_driver
reusing the same of_device_id array).

For the bit twiddling we could use of_device_is_compatible() and avoid
this info struct altogether. That would allow to implement more complex
logic - doesn't rule out using this struct for the offsets of course.

Preferences?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [RFC 3/3] irqchip: Add Realtek RTD1295 mux driver

[Marc Zyngier]

On 18/08/17 18:21, Andreas Färber wrote:
> Hi Marc,
> 
> Am 18.08.2017 um 12:53 schrieb Marc Zyngier:
>> On 17/08/17 11:11, Andreas Färber wrote:
>>> This irq mux driver is derived from the RTD1295 vendor DT and assumes a linear
>>> mapping between intr_en and intr_status registers. Code for RTD119x indicates
>>> this may not always be the case (i2c_3).
>>>
>>> The register initialization was copied from QNAP's mach-rtk119x/rtk_irq_mux.c
>>> as a boot fix, without full insights into what exactly this is changing (TODO).
>>>
>>> Signed-off-by: Andreas Färber <afaerber-l3A5Bk7waGM@public.gmane.org>
>>> ---
>>>  drivers/irqchip/Makefile          |   1 +
>>>  drivers/irqchip/irq-rtd119x-mux.c | 201 ++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 202 insertions(+)
>>>  create mode 100644 drivers/irqchip/irq-rtd119x-mux.c
>>>
>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>> index e88d856cc09c..46202a0b7d96 100644
>>> --- a/drivers/irqchip/Makefile
>>> +++ b/drivers/irqchip/Makefile
>>> @@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
>>>  obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
>>>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>>>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>>> +obj-$(CONFIG_ARCH_REALTEK)		+= irq-rtd119x-mux.o
> 
> Should this get its own config option? It would get shared by ARM64 and
> future potential ARM, so no strict need for composite expressions, and
> there's examples of both ways above.

If another platform type comes up with this particular gem, we can
always create a specific symbol. In the meantime, you can keep simple.

> BTW I was surprised that the Makefile does not seem to have a consistent
> order, leading to potential conflicts when adding things in the bottom.
> Have you guys thought about enforcing alphabetical order by either
> config option or file name?

Not really. I cannot bring myself to care about that kind of thing...

> 
>>> diff --git a/drivers/irqchip/irq-rtd119x-mux.c b/drivers/irqchip/irq-rtd119x-mux.c
>>> new file mode 100644
>>> index 000000000000..c6c1ba126bf3
>>> --- /dev/null
>>> +++ b/drivers/irqchip/irq-rtd119x-mux.c
>>> @@ -0,0 +1,201 @@
>>> +/*
>>> + * Realtek RTD129x IRQ mux
>>> + *
>>> + * Copyright (c) 2017 Andreas Färber
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0+
>>> + */
>>> +
>>> +#include <linux/io.h>
>>> +#include <linux/irqchip.h>
>>> +#include <linux/irqchip/chained_irq.h>
>>> +#include <linux/irqdomain.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/slab.h>
>>> +
>>> +struct rtd119x_irq_mux_info {
>>> +	unsigned intr_status;
>>> +	unsigned intr_en;
>>
>> nit: these would be better named with a "_offset" suffix, in order to
>> better distinguish them from the structure below.
> 
> True, renamed. See also discussion in the bottom.
> 
>>> +};
>>> +
>>> +struct rtd119x_irq_mux_data {
>>> +	void __iomem *intr_status;
>>> +	void __iomem *intr_en;
>>> +	int irq;
>>> +	struct irq_domain *domain;
>>> +	spinlock_t lock;
>>> +};
>>> +
>>> +static void rtd119x_mux_irq_handle(struct irq_desc *desc)
>>> +{
>>> +	struct rtd119x_irq_mux_data *data = irq_desc_get_handler_data(desc);
>>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>>> +	u32 intr_en, intr_status, status;
>>> +	int ret;
>>> +
>>> +	chained_irq_enter(chip, desc);
>>> +
>>> +	spin_lock(&data->lock);
>>> +	intr_en     = readl(data->intr_en);
>>> +	intr_status = readl(data->intr_status);
>>
>> All the readl/writel should be turned into their _relaxed versions.
> 
> Hmm, so I use readl/writel by default. The old downstream code was using
> __raw_readl/__raw_writel, but my initial code appeared to work, so I
> left it that way.
> 
> Can you explain why _relaxed, so that I can properly choose next time?
> I see the difference is __iormb(), but when do I need that? Thanks.

You can use the _relaxed version when there is no dependency with a
memory access before (for a write) that needs to be propagated to the
device before the MMIO access takes place. In your case, the device (the
irqchip) never reads anything from memory, so there is strictly no need
to issue a dsb on each MMIO write.

> 
>>> +	spin_unlock(&data->lock);
>>> +
>>> +	status = intr_status & intr_en;
>>> +	if (status != 0) {
>>> +		unsigned irq = __ffs(status);
>>> +		ret = generic_handle_irq(irq_find_mapping(data->domain, irq));
>>> +		if (ret == 0) {
>>> +			spin_lock(&data->lock);
>>> +			intr_status = readl(data->intr_status);
>>> +			intr_status |= BIT(irq - 1);
>>> +			writel(intr_status, data->intr_status);
>>> +			spin_unlock(&data->lock);
>>> +		}
>>> +	}
>>
>> And what if status is 0? We keep the lock held forever?
> 
> Which lock? Locking is only done for consistently reading en and status,
> and for the read-modify-write of status. Both look balanced to me, and
> no hangs have been observed in testing.

Forget it. I read the above "unlock" as a "lock", and everything went
downhill. Too much crap code today, I see it everywhere.

> 
> __ffs: "Undefined if no bit exists, so code should check against 0 first."
> 
> If status is 0 then there's no pending enabled interrupt and thus
> nothing to do.
> 
> The downstream code modified all drivers (e.g., 8250 serial) to
> acknowledge their interrupts inside the drivers, and instead it had code
> that complained and cleared them here.

Well, good luck to them.

> I wanted to keep the irqchip abstraction, so I am always acknowledging
> the interrupt if generic_handle_irq does not run into an error - I
> understood that this is not the same as actually having been
> successfully handled, but I did not see a better way for how to do this
> in generic code.
> (I am facing a similar mux situation with FM4, so input appreciated on
> whether these are right hooks to use - for FM4 I got stuck trying to
> adopt the hierarchical API and resorted to the simpler approach here.)
> 
> Would you rather have realtek,intr_en and realtek,intr_status properties
> in the relevant device nodes and drivers and acknowledge the interrupts
> from their respective interrupt handlers and then ignore it here?

Certainly not. Dealing with the interrupt controller is the job of the
irqchip driver, hence no need to pollute the drivers. What you're doing
is fine, though masking the interrupt on error is a bit heavy handed...

>>> +
>>> +	chained_irq_exit(chip, desc);
>>> +}
>>> +
>>> +static void rtd119x_mux_mask_irq(struct irq_data *data)
>>> +{
>>> +	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
>>> +	u32 intr_status;
>>> +
>>> +	intr_status = readl(mux_data->intr_status);
>>> +	intr_status |= BIT(data->hwirq);
>>> +	writel(intr_status, mux_data->intr_status);
>>
>> So you need a lock in the chained handler, but you don't need one here?
>> It doesn't feel right.
> 
> Good point. I had started without locks; should add them here and below.
> 
>>> +}
>>> +
>>> +static void rtd119x_mux_unmask_irq(struct irq_data *data)
>>> +{
>>> +	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(data);
>>> +	u32 intr_en;
>>> +
>>> +	intr_en = readl(mux_data->intr_en);
>>> +	intr_en |= BIT(data->hwirq);
>>> +	writel(intr_en, mux_data->intr_en);
>>
>> Same thing here.
>>
>>> +}
>>> +
>>> +static int rtd119x_mux_set_affinity(struct irq_data *d,
>>> +			const struct cpumask *mask_val, bool force)
>>> +{
>>> +	struct rtd119x_irq_mux_data *mux_data = irq_data_get_irq_chip_data(d);
>>> +	struct irq_chip *chip = irq_get_chip(mux_data->irq);
>>> +	struct irq_data *data = irq_get_irq_data(mux_data->irq);
>>> +
>>> +	if (chip && chip->irq_set_affinity)
>>> +		return chip->irq_set_affinity(data, mask_val, force);
>>
>> I'm always worried when I see this. It means that all of the interrupts
>> on the secondary chip move in one go, without the core code noticing.
> 
> You mean it affects all 32 mux'ed IRQs as a side effect here? True.
> 
>> One of these days, we'll have to address it.
> 
> Note that currently I have only a single core up on RTD1295, so affinity
> is not really tested. (They use a custom "rtk-spin-table" implementation
> that I have no source code or documentation for, and I haven't made
> guesses yet, nor do I think that would be acceptable in mainline arm64.)

Indeed.

> This hook appears to be optional, so we could just drop it for now, or
> always return -EINVAL if preferred over forwarding to the GIC?

I'd feel more comfortable with that. Just return -EINVAL.

>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static struct irq_chip rtd119x_mux_irq_chip = {
>>> +	.name			= "rtd119x-mux",
>>> +	.irq_mask		= rtd119x_mux_mask_irq,
>>> +	.irq_unmask		= rtd119x_mux_unmask_irq,
>>> +	.irq_set_affinity	= rtd119x_mux_set_affinity,
>>> +};
>>> +
>>> +static int rtd119x_mux_irq_domain_xlate(struct irq_domain *d,
>>> +		struct device_node *controller,
>>> +		const u32 *intspec, unsigned int intsize,
>>> +		unsigned long *out_hwirq,
>>> +		unsigned int *out_type)
>>> +{
>>> +	if (irq_domain_get_of_node(d) != controller)
>>> +		return -EINVAL;
>>> +
>>> +	if (intsize < 1)
>>> +		return -EINVAL;
>>> +
>>> +	*out_hwirq = intspec[0];
>>> +	*out_type = 0;
>>> +
>>> +	return 0;
>>> +}
>>
>> Please use irq_domain_xlate_onecell() instead.
> 
> Thanks for the pointer, done.
> 
>>> +
>>> +static int rtd119x_mux_irq_domain_map(struct irq_domain *d,
>>> +		unsigned int irq, irq_hw_number_t hw)
>>> +{
>>> +	struct rtd119x_irq_mux_data *data = d->host_data;
>>> +
>>> +	irq_set_chip_and_handler(irq, &rtd119x_mux_irq_chip, handle_level_irq);
>>> +	irq_set_chip_data(irq, data);
>>> +	irq_set_probe(irq);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct irq_domain_ops rtd119x_mux_irq_domain_ops = {
>>> +	.xlate	= rtd119x_mux_irq_domain_xlate,
>>> +	.map	= rtd119x_mux_irq_domain_map,
>>> +};
> [...]
>>> +static const struct rtd119x_irq_mux_info rtd1295_iso_irq_mux_info = {
>>> +	.intr_status	= 0x0,
>>> +	.intr_en	= 0x40,
>>> +};
>>> +
>>> +static int __init rtd1295_iso_irq_mux_init(struct device_node *node,
>>> +					   struct device_node *parent)
>>> +{
>>> +	return rtd119x_irq_mux_init(node, parent, &rtd1295_iso_irq_mux_info);
>>> +}
>>> +IRQCHIP_DECLARE(rtd1295_iso_mux, "realtek,rtd1295-iso-irq-mux", rtd1295_iso_irq_mux_init);
>>> +
>>> +static const struct rtd119x_irq_mux_info rtd1295_irq_mux_info = {
>>> +	.intr_status	= 0xc,
>>> +	.intr_en	= 0x80,
>>> +};
>>> +
>>> +static int __init rtd1295_irq_mux_init(struct device_node *node,
>>> +				       struct device_node *parent)
>>> +{
>>> +	return rtd119x_irq_mux_init(node, parent, &rtd1295_irq_mux_info);
>>> +}
>>> +IRQCHIP_DECLARE(rtd1295_mux, "realtek,rtd1295-irq-mux", rtd1295_irq_mux_init);
> 
> This was a quick hack to cover both instances. I am not yet sure how to
> best model the case of e.g. en bit 28 corresponding to status bit 23 in
> the struct. An index array of all 32 bits with special value for absent,
> to iteratively or the bits instead of masking them?
> 
> We might use of_match_node() to get the info pointer in one generic init
> function instead of having per-compatible init functions down here, but
> then we'd have the compatibles duplicated (compared to platform_driver
> reusing the same of_device_id array).
> 
> For the bit twiddling we could use of_device_is_compatible() and avoid
> this info struct altogether. That would allow to implement more complex
> logic - doesn't rule out using this struct for the offsets of course.
> 
> Preferences?

None. This looks like fishy/undocumented HW, so whatever floats your
boat will work for me. The struct seem slightly overkill given the
information it provides, but that's your choice.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html