Date:   Wed, 23 Dec 2020 16:18:16 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Bert Vermeulen <bert@...t.com>
Cc:     Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Rob Herring <robh+dt@...nel.org>,
        Paul Burton <paulburton@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Damien Le Moal <damien.lemoal@....com>,
        Mateusz Holenko <mholenko@...micro.com>,
        Stafford Horne <shorne@...il.com>,
        Pawel Czarnecki <pczarnecki@...ernships.antmicro.com>,
        Palmer Dabbelt <palmerdabbelt@...gle.com>,
        Cédric Le Goater <clg@...d.org>,
        Shawn Guo <shawnguo@...nel.org>, Joel Stanley <joel@....id.au>,
        Leonard Crestez <leonard.crestez@....com>,
        Peng Fan <peng.fan@....com>,
        linux-mips@...r.kernel.org (open list:MIPS),
        linux-kernel@...r.kernel.org (open list),
        devicetree@...r.kernel.org (open list:OPEN FIRMWARE AND FLATTENED
        DEVICE TREE BINDINGS)
Subject: Re: [PATCH v2] Add support for Realtek RTL838x/RTL839x switch SoCs

On Wed, 23 Dec 2020 15:06:24 +0000,
Bert Vermeulen <bert@...t.com> wrote:
> 
> This adds basic system support for the Realtek RTL838x/RTL839x switch
> SoCs. These are used in many inexpensive switches.
> 
> This patch also paves the way for the RTL930x/RTL931x series.
> 
> Signed-off-by: Bert Vermeulen <bert@...t.com>
> ---
> v2:
> - Removed all new arch/mips/ code, using arch/mips/generic/ instead.
> - Use device tree ranges instead of hardcoded addresses for ioremap.
> - Moved IRQ driver to drivers/irqchip/
> - Removed reset handling code, will be replaced by device tree config.
> - All SoC family id code moved to new soc driver.
> - Header moved to realtek/ instead of mach-realtek/
> - As more of the base system now depends on device tree, a sample
>   dts for the Cisco SG220-26 switch is included. This will be further
>   filled out, and bindings documented, as drivers get merged.
> 
>  arch/mips/Kconfig                             |  31 +++
>  arch/mips/boot/dts/Makefile                   |   1 +
>  arch/mips/boot/dts/realtek/Makefile           |   2 +
>  arch/mips/boot/dts/realtek/cisco_sg220-26.dts |  25 +++
>  arch/mips/boot/dts/realtek/rtl838x.dtsi       |  28 +++
>  arch/mips/boot/dts/realtek/rtl839x.dtsi       |  28 +++
>  arch/mips/boot/dts/realtek/rtl83xx.dtsi       |  74 +++++++
>  arch/mips/generic/Platform                    |   1 +
>  arch/mips/include/asm/realtek/ioremap.h       |  48 +++++
>  arch/mips/include/asm/realtek/mach-realtek.h  |  94 +++++++++
>  drivers/irqchip/Makefile                      |   1 +
>  drivers/irqchip/irq-realtek.c                 | 148 ++++++++++++++
>  drivers/soc/Kconfig                           |   1 +
>  drivers/soc/Makefile                          |   1 +
>  drivers/soc/realtek/Kconfig                   |  14 ++
>  drivers/soc/realtek/Makefile                  |   3 +
>  drivers/soc/realtek/realtek-chipid.c          | 187 ++++++++++++++++++
>  17 files changed, 687 insertions(+)
>  create mode 100644 arch/mips/boot/dts/realtek/Makefile
>  create mode 100644 arch/mips/boot/dts/realtek/cisco_sg220-26.dts
>  create mode 100644 arch/mips/boot/dts/realtek/rtl838x.dtsi
>  create mode 100644 arch/mips/boot/dts/realtek/rtl839x.dtsi
>  create mode 100644 arch/mips/boot/dts/realtek/rtl83xx.dtsi
>  create mode 100644 arch/mips/include/asm/realtek/ioremap.h
>  create mode 100644 arch/mips/include/asm/realtek/mach-realtek.h
>  create mode 100644 drivers/irqchip/irq-realtek.c
>  create mode 100644 drivers/soc/realtek/Kconfig
>  create mode 100644 drivers/soc/realtek/Makefile
>  create mode 100644 drivers/soc/realtek/realtek-chipid.c

For a start, please split this very large patch into multiple patches:
- DT stuff (preferably multiple patches)
- irqchip code
- chipid code

[...]

> diff --git a/arch/mips/include/asm/realtek/ioremap.h b/arch/mips/include/asm/realtek/ioremap.h
> new file mode 100644
> index 000000000000..9141283d116c
> --- /dev/null
> +++ b/arch/mips/include/asm/realtek/ioremap.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _RTL8380_IOREMAP_H_
> +#define _RTL8380_IOREMAP_H_
> +
> +#include <linux/of.h>
> +
> +static inline int is_rtl8380_internal_registers(phys_addr_t offset)
> +{
> +	struct device_node *np = NULL;
> +	const __be32 *prop;
> +	int lenp;
> +	u32 start, stop;
> +
> +	if (offset & BIT(31))
> +		/* already mapped into register space */
> +		return 1;
> +
> +	do {
> +		np = of_find_node_with_property(np, "ranges");
> +		if (!np)
> +			continue;
> +		prop = of_get_property(np, "ranges", &lenp);
> +		if (lenp != 12)
> +			continue;

This all looks terribly cumbersome, and despite not being a DT expert,
I have the ugly feeling that you are reinventing the wheel. Surely the
node providing this address range has a compatible you could match,
and isn't some random node?

And do we need this to be *inlined*? Probably not. It looks like an
attempt at DT-fying some code, without building the required
infrastructure (it cannot be built as a multi-platform kernel
anyway). To be honest, I'd rather see something like
arch/mips/include/asm/mach-bcm63xx/ioremap.h, which plainly shows that
multi-platform builds is the least of their concern.

> +		start = be32_to_cpup(prop + 1);
> +		stop = start + be32_to_cpup(prop + 2);
> +		of_node_put(np);
> +		if (offset >= start && offset < stop)
> +			return 1;
> +
> +	} while (np);
> +	return 0;
> +}
> +
> +static inline void __iomem *plat_ioremap(phys_addr_t offset, unsigned long size,
> +					 unsigned long flags)
> +{
> +	if (is_rtl8380_internal_registers(offset))
> +		return (void __iomem *)offset;
> +	return NULL;
> +}
> +
> +static inline int plat_iounmap(const volatile void __iomem *addr)
> +{
> +	return is_rtl8380_internal_registers((unsigned long)addr);
> +}
> +
> +#endif /* _RTL8380_IOREMAP_H_ */
> diff --git a/arch/mips/include/asm/realtek/mach-realtek.h b/arch/mips/include/asm/realtek/mach-realtek.h
> new file mode 100644
> index 000000000000..446c99b19411
> --- /dev/null
> +++ b/arch/mips/include/asm/realtek/mach-realtek.h
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2006-2012 Tony Wu <tonywu@...ltek.com>
> + * Copyright (C) 2020 Birger Koblitz <mail@...ger-koblitz.de>
> + * Copyright (C) 2020 Bert Vermeulen <bert@...t.com>
> + * Copyright (C) 2020 John Crispin <john@...ozen.org>
> + */
> +
> +#ifndef _MACH_REALTEK_H_
> +#define _MACH_REALTEK_H_
> +
> +struct realtek_soc_info {
> +	unsigned char *name;
> +	unsigned int id;
> +	unsigned int family;
> +};
> +
> +/* Interrupt numbers/bits */
> +#define RTL8380_IRQ_UART0		31
> +#define RTL8380_IRQ_UART1		30
> +#define RTL8380_IRQ_TC0			29
> +#define RTL8380_IRQ_TC1			28
> +#define RTL8380_IRQ_OCPTO		27
> +#define RTL8380_IRQ_HLXTO		26
> +#define RTL8380_IRQ_SLXTO		25
> +#define RTL8380_IRQ_NIC			24
> +#define RTL8380_IRQ_GPIO_ABCD		23
> +#define RTL8380_IRQ_GPIO_EFGH		22
> +#define RTL8380_IRQ_RTC			21
> +#define RTL8380_IRQ_SWCORE		20
> +#define RTL8380_IRQ_WDT_IP1		19
> +#define RTL8380_IRQ_WDT_IP2		18

Why do we need any of this? The mapping should be explicit in the DT.

> +
> +/* Global Interrupt Mask Register */
> +#define RTL8380_ICTL_GIMR	0x00
> +/* Global Interrupt Status Register */
> +#define RTL8380_ICTL_GISR	0x04
> +
> +/* Cascaded interrupts */
> +#define RTL8380_CPU_IRQ_SHARED0		(MIPS_CPU_IRQ_BASE + 2)
> +#define RTL8380_CPU_IRQ_UART		(MIPS_CPU_IRQ_BASE + 3)
> +#define RTL8380_CPU_IRQ_SWITCH		(MIPS_CPU_IRQ_BASE + 4)
> +#define RTL8380_CPU_IRQ_SHARED1		(MIPS_CPU_IRQ_BASE + 5)
> +#define RTL8380_CPU_IRQ_EXTERNAL	(MIPS_CPU_IRQ_BASE + 6)
> +#define RTL8380_CPU_IRQ_COUNTER		(MIPS_CPU_IRQ_BASE + 7)
> +
> +
> +/* Interrupt routing register */
> +#define RTL8380_IRR0		0x08
> +#define RTL8380_IRR1		0x0c
> +#define RTL8380_IRR2		0x10
> +#define RTL8380_IRR3		0x14
> +
> +/* Cascade map */
> +#define RTL8380_IRQ_CASCADE_UART0	2
> +#define RTL8380_IRQ_CASCADE_UART1	1
> +#define RTL8380_IRQ_CASCADE_TC0		5
> +#define RTL8380_IRQ_CASCADE_TC1		1
> +#define RTL8380_IRQ_CASCADE_OCPTO	1
> +#define RTL8380_IRQ_CASCADE_HLXTO	1
> +#define RTL8380_IRQ_CASCADE_SLXTO	1
> +#define RTL8380_IRQ_CASCADE_NIC		4
> +#define RTL8380_IRQ_CASCADE_GPIO_ABCD	4
> +#define RTL8380_IRQ_CASCADE_GPIO_EFGH	4
> +#define RTL8380_IRQ_CASCADE_RTC		4
> +#define RTL8380_IRQ_CASCADE_SWCORE	3
> +#define RTL8380_IRQ_CASCADE_WDT_IP1	4
> +#define RTL8380_IRQ_CASCADE_WDT_IP2	5
> +
> +/* Pack cascade map into interrupt routing registers */
> +#define RTL8380_IRR0_SETTING (\
> +	(RTL8380_IRQ_CASCADE_UART0	<< 28) | \
> +	(RTL8380_IRQ_CASCADE_UART1	<< 24) | \
> +	(RTL8380_IRQ_CASCADE_TC0	<< 20) | \
> +	(RTL8380_IRQ_CASCADE_TC1	<< 16) | \
> +	(RTL8380_IRQ_CASCADE_OCPTO	<< 12) | \
> +	(RTL8380_IRQ_CASCADE_HLXTO	<< 8)  | \
> +	(RTL8380_IRQ_CASCADE_SLXTO	<< 4)  | \
> +	(RTL8380_IRQ_CASCADE_NIC	<< 0))
> +#define RTL8380_IRR1_SETTING (\
> +	(RTL8380_IRQ_CASCADE_GPIO_ABCD	<< 28) | \
> +	(RTL8380_IRQ_CASCADE_GPIO_EFGH	<< 24) | \
> +	(RTL8380_IRQ_CASCADE_RTC	<< 20) | \
> +	(RTL8380_IRQ_CASCADE_SWCORE	<< 16))
> +#define RTL8380_IRR2_SETTING	0
> +#define RTL8380_IRR3_SETTING	0
> +
> +/* Used to detect address length pin strapping on RTL833x/RTL838x */
> +#define RTL8380_INT_RW_CTRL		(RTL8380_SWITCH_BASE + 0x58)
> +#define RTL8380_EXT_VERSION		(RTL8380_SWITCH_BASE + 0xD0)
> +#define RTL8380_PLL_CML_CTRL		(RTL8380_SWITCH_BASE + 0xFF8)
> +#define RTL8380_STRAP_DBG		(RTL8380_SWITCH_BASE + 0x100C)
> +
> +#endif /* _MACH_RTL8380_H_ */
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 0ac93bfaec61..9c4acb4e9b5f 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -113,3 +113,4 @@ obj-$(CONFIG_LOONGSON_PCH_PIC)		+= irq-loongson-pch-pic.o
>  obj-$(CONFIG_LOONGSON_PCH_MSI)		+= irq-loongson-pch-msi.o
>  obj-$(CONFIG_MST_IRQ)			+= irq-mst-intc.o
>  obj-$(CONFIG_SL28CPLD_INTC)		+= irq-sl28cpld.o
> +obj-$(CONFIG_MACH_REALTEK)		+= irq-realtek.o
> diff --git a/drivers/irqchip/irq-realtek.c b/drivers/irqchip/irq-realtek.c
> new file mode 100644
> index 000000000000..827a6d891b76
> --- /dev/null
> +++ b/drivers/irqchip/irq-realtek.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2006-2012 Tony Wu <tonywu@...ltek.com>
> + * Copyright (C) 2020 Birger Koblitz <mail@...ger-koblitz.de>
> + * Copyright (C) 2020 Bert Vermeulen <bert@...t.com>
> + * Copyright (C) 2020 John Crispin <john@...ozen.org>
> + */
> +
> +#include <linux/irqchip.h>
> +#include <linux/spinlock.h>
> +#include <linux/of_address.h>
> +#include <asm/irq_cpu.h>
> +#include <linux/of_irq.h>
> +#include <asm/cevt-r4k.h>
> +
> +#include <mach-realtek.h>
> +
> +#define REG(x)		(realtek_ictl_base + x)
> +
> +static DEFINE_RAW_SPINLOCK(irq_lock);
> +static void __iomem *realtek_ictl_base;
> +
> +
> +static void realtek_ictl_enable_irq(struct irq_data *i)
> +{
> +	unsigned long flags;
> +	u32 value;
> +
> +	raw_spin_lock_irqsave(&irq_lock, flags);
> +
> +	value = readl(REG(RTL8380_ICTL_GIMR));
> +	value |= BIT(i->hwirq);
> +	writel(value, REG(RTL8380_ICTL_GIMR));
> +
> +	raw_spin_unlock_irqrestore(&irq_lock, flags);
> +}
> +
> +static void realtek_ictl_disable_irq(struct irq_data *i)
> +{
> +	unsigned long flags;
> +	u32 value;
> +
> +	raw_spin_lock_irqsave(&irq_lock, flags);
> +
> +	value = readl(REG(RTL8380_ICTL_GIMR));
> +	value &= ~BIT(i->hwirq);
> +	writel(value, REG(RTL8380_ICTL_GIMR));
> +
> +	raw_spin_unlock_irqrestore(&irq_lock, flags);
> +}
> +
> +static struct irq_chip realtek_ictl_irq = {
> +	.name = "rtl8380",
> +	.irq_enable = realtek_ictl_enable_irq,
> +	.irq_disable = realtek_ictl_disable_irq,
> +	.irq_ack = realtek_ictl_disable_irq,

No. irq_ack() isn't a disable. Please consult the documentation for
your SoC to understand the is the expected flow for this interrupt
controller.

> +	.irq_mask = realtek_ictl_disable_irq,
> +	.irq_unmask = realtek_ictl_enable_irq,

Having both enable/disable and mask/unmask is pointless. Please drop
the former.

> +	.irq_eoi = realtek_ictl_enable_irq,

Neither. Please don't make things up, as this is very unlikely to
reliably work as is.

> +};

No affinity setting? Is this system purely UP?

> +
> +static int intc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> +{
> +	irq_set_chip_and_handler(hw, &realtek_ictl_irq, handle_level_irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops irq_domain_ops = {
> +	.map = intc_map,
> +	.xlate = irq_domain_xlate_onecell,
> +};
> +
> +static void realtek_irq_dispatch(struct irq_desc *desc)
> +{
> +	unsigned int pending = readl(REG(RTL8380_ICTL_GIMR)) & readl(REG(RTL8380_ICTL_GISR));
> +
> +	if (pending) {
> +		struct irq_domain *domain = irq_desc_get_handler_data(desc);
> +		generic_handle_irq(irq_find_mapping(domain, __ffs(pending)));
> +	} else {
> +		spurious_interrupt();
> +	}

This isn't how a chained interrupt handler is supposed to be
written. You are missing the chained_irq_{enter,exit} calls.

> +}
> +
> +asmlinkage void plat_irq_dispatch(void)
> +{
> +	unsigned int pending;
> +
> +	pending =  read_c0_cause() & read_c0_status() & ST0_IM;
> +
> +	if (pending & CAUSEF_IP7)
> +		do_IRQ(RTL8380_CPU_IRQ_COUNTER);
> +
> +	else if (pending & CAUSEF_IP6)
> +		do_IRQ(RTL8380_CPU_IRQ_EXTERNAL);
> +
> +	else if (pending & CAUSEF_IP5)
> +		do_IRQ(RTL8380_CPU_IRQ_SHARED1);
> +
> +	else if (pending & CAUSEF_IP4)
> +		do_IRQ(RTL8380_CPU_IRQ_SWITCH);
> +
> +	else if (pending & CAUSEF_IP3)
> +		do_IRQ(RTL8380_CPU_IRQ_UART);
> +
> +	else if (pending & CAUSEF_IP2)
> +		do_IRQ(RTL8380_CPU_IRQ_SHARED0);
> +
> +	else
> +		spurious_interrupt();

Why isn't this a lookup in an irqdomain?

> +}
> +
> +static int __init rtl8380_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	struct irq_domain *domain;
> +
> +	domain = irq_domain_add_simple(node, 32, 0,
> +				       &irq_domain_ops, NULL);
> +        irq_set_chained_handler_and_data(2, realtek_irq_dispatch, domain);
> +        irq_set_chained_handler_and_data(5, realtek_irq_dispatch, domain);
> +

Indentation.

> +	realtek_ictl_base = of_iomap(node, 0);
> +	if (!realtek_ictl_base)
> +		return -ENXIO;
> +
> +	/* Disable all cascaded interrupts */
> +	writel(0, REG(RTL8380_ICTL_GIMR));
> +
> +	/* Set up interrupt routing */
> +	writel(RTL8380_IRR0_SETTING, REG(RTL8380_IRR0));
> +	writel(RTL8380_IRR1_SETTING, REG(RTL8380_IRR1));
> +	writel(RTL8380_IRR2_SETTING, REG(RTL8380_IRR2));
> +	writel(RTL8380_IRR3_SETTING, REG(RTL8380_IRR3));

What is this doing?

> +
> +	/* Clear timer interrupt */
> +	write_c0_compare(0);
> +
> +	/* Enable all CPU interrupts */
> +	write_c0_status(read_c0_status() | ST0_IM);
> +
> +	/* Enable timer0 and uart0 interrupts */
> +	writel(BIT(RTL8380_IRQ_TC0) | BIT(RTL8380_IRQ_UART0), REG(RTL8380_ICTL_GIMR));
> +

Interrupts are supposed to be enabled when they are requested. Not before.

> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(realtek_rtl8380_intc, "realtek,rtl8380-intc", rtl8380_of_init);

Where is the binding documentation?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Powered by blists - more mailing lists