Re: [PATCH v2] Add support for Realtek RTL838x/RTL839x switch SoCs

* Re: [PATCH v2] Add support for Realtek RTL838x/RTL839x switch SoCs
       [not found] <20201223150648.1633113-1-bert@biot.com>
@ 2020-12-23 16:18 ` Marc Zyngier
  2020-12-26 15:02   ` Bert Vermeulen
  0 siblings, 1 reply; 3+ messages in thread
From: Marc Zyngier @ 2020-12-23 16:18 UTC (permalink / raw)
  To: Bert Vermeulen
  Cc: Thomas Bogendoerfer, Rob Herring, Paul Burton, Thomas Gleixner,
	Damien Le Moal, Mateusz Holenko, Stafford Horne, Pawel Czarnecki,
	Palmer Dabbelt, Cédric Le Goater, Shawn Guo, Joel Stanley,
	Leonard Crestez, Peng Fan, open list:MIPS, open list,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Wed, 23 Dec 2020 15:06:24 +0000,
Bert Vermeulen <bert@biot.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@biot.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@realtek.com>
> + * Copyright (C) 2020 Birger Koblitz <mail@birger-koblitz.de>
> + * Copyright (C) 2020 Bert Vermeulen <bert@biot.com>
> + * Copyright (C) 2020 John Crispin <john@phrozen.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@realtek.com>
> + * Copyright (C) 2020 Birger Koblitz <mail@birger-koblitz.de>
> + * Copyright (C) 2020 Bert Vermeulen <bert@biot.com>
> + * Copyright (C) 2020 John Crispin <john@phrozen.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.

^ permalink raw reply	[flat|nested] 3+ messages in thread