| Message ID | 20210320181610.680870-1-j.neuschaefer@gmx.net |
|---|---|
| Headers | show |
| Series | Initial support for Nuvoton WPCM450 BMC SoC | expand |
On Sat, Mar 20, 2021 at 07:15:56PM +0100, Jonathan Neuschäfer wrote: > This series adds basic support for the Nuvoton WPCM450 BMC SoC. It's an older > SoC but still commonly found on eBay, mostly in Supermicro X9 server boards. Something I forgot to mention: I wrote a bit of third-party documentation based on the various vendor kernel trees and it's available at: https://github.com/neuschaefer/wpcm450/wiki Best regards, Jonathan Neuschäfer
On Sun, Mar 21, 2021 at 01:07:53PM +0200, Tomer Maimon wrote: > Hi Jonathan, > > Thanks a lot for trying to add WPCM450. > > Hoever WPCM450 is in EOL for several years and we are not supporting this > product anymore. > As you said it is only available in the secondary market. > > Due to it is better not to add the WPCM450 under Nuvoton maintenance. I understand. I will instead add a new, separate section for WPCM450 to the MAINTAINERS file. > Again we highly appreciate your support and time on NPCM750 patches. I expect there will be some more cooperation, because the SoCs share some architectural similarity, for example the 100Mbit Ethernet controller. Best regards, Jonathan
On 20/03/2021 19:16, Jonathan Neuschäfer wrote: > Add a compatible string for WPCM450, which has essentially the same > timer controller. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- Applied, thanks -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
On Sat, 20 Mar 2021 18:16:04 +0000, Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > > The WPCM450 AIC ("Advanced Interrupt Controller") is the interrupt > controller found in the Nuvoton WPCM450 SoC and other Winbond/Nuvoton > SoCs. > > The list of registers if based on the AMI vendor kernel and the > Nuvoton W90N745 datasheet. > > Although the hardware supports other interrupt modes, the driver only > supports high-level interrupts at the moment, because other modes could > not be tested so far. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- > arch/arm/mach-npcm/Kconfig | 1 + > drivers/irqchip/Kconfig | 6 ++ > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-wpcm450-aic.c | 162 ++++++++++++++++++++++++++++++ > 4 files changed, 170 insertions(+) > create mode 100644 drivers/irqchip/irq-wpcm450-aic.c > > diff --git a/arch/arm/mach-npcm/Kconfig b/arch/arm/mach-npcm/Kconfig > index 658c8efb4ca14..a71cf1d189ae5 100644 > --- a/arch/arm/mach-npcm/Kconfig > +++ b/arch/arm/mach-npcm/Kconfig > @@ -10,6 +10,7 @@ config ARCH_WPCM450 > bool "Support for WPCM450 BMC (Hermon)" > depends on ARCH_MULTI_V5 > select CPU_ARM926T > + select WPCM450_AIC > select NPCM7XX_TIMER > help > General support for WPCM450 BMC (Hermon). > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index e74fa206240a1..baf4efec31c67 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -586,4 +586,10 @@ config MST_IRQ > help > Support MStar Interrupt Controller. > > +config WPCM450_AIC > + bool "Nuvoton WPCM450 Advanced Interrupt Controller" > + depends on ARCH_WPCM450 || COMPILE_TEST > + help > + Support for the interrupt controller in the Nuvoton WPCM450 BMC SoC. > + > endmenu > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index c59b95a0532c9..bef57937e7296 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -113,3 +113,4 @@ 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_RTL) += irq-realtek-rtl.o > +obj-$(CONFIG_WPCM450_AIC) += irq-wpcm450-aic.o > diff --git a/drivers/irqchip/irq-wpcm450-aic.c b/drivers/irqchip/irq-wpcm450-aic.c > new file mode 100644 > index 0000000000000..0d6dd8b1fc824 > --- /dev/null > +++ b/drivers/irqchip/irq-wpcm450-aic.c > @@ -0,0 +1,162 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright 2021 Jonathan Neuschäfer > + > +#include <linux/console.h> That's unexpected. Why do you need this? > +#include <linux/irqchip.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > + > +#include <asm/exception.h> > + > +#define AIC_SCR(x) ((x)*4) /* Source control registers */ > +#define AIC_GEN 0x84 > +#define AIC_GRSR 0x88 > +#define AIC_IRSR 0x100 /* Interrupt raw status register */ > +#define AIC_IASR 0x104 /* Interrupt active status register */ > +#define AIC_ISR 0x108 /* Interrupt status register */ > +#define AIC_IPER 0x10c /* Interrupt priority encoding register */ > +#define AIC_ISNR 0x110 /* Interrupt source number register */ > +#define AIC_IMR 0x114 /* Interrupt mask register */ > +#define AIC_OISR 0x118 /* Output interrupt status register */ > +#define AIC_MECR 0x120 /* Mask enable command register */ > +#define AIC_MDCR 0x124 /* Mask disable command register */ > +#define AIC_SSCR 0x128 /* Source set command register */ > +#define AIC_SCCR 0x12c /* Source clear command register */ > +#define AIC_EOSCR 0x130 /* End of service command register */ > + > +#define AIC_SCR_SRCTYPE_LOW_LEVEL (0 << 6) > +#define AIC_SCR_SRCTYPE_HIGH_LEVEL (1 << 6) > +#define AIC_SCR_SRCTYPE_NEG_EDGE (2 << 6) > +#define AIC_SCR_SRCTYPE_POS_EDGE (3 << 6) > +#define AIC_SCR_PRIORITY(x) (x) A mask would be welcomed for this field. > + > +#define IRQS 32 Please use something a bit less generic. > + > +struct wpcm450_aic { > + void __iomem *regs; > + struct irq_domain *domain; > +}; > + > +static struct wpcm450_aic *aic; > + > +static void wpcm450_aic_init_hw(void) > +{ > + int i; > + > + /* Disable (mask) all interrupts */ > + writel(0xffffffff, aic->regs + AIC_MDCR); Consider using relaxed accessors throughout this driver. > + > + /* > + * Make sure the interrupt controller is ready to serve new interrupts. > + * Reading from IPER indicates that the nIRQ signal may be deasserted, > + * and writing to EOSCR indicates that interrupt handling has finished. > + */ > + readl(aic->regs + AIC_IPER); > + writel(0, aic->regs + AIC_EOSCR); > + > + /* Initialize trigger mode and priority of each interrupt source */ > + for (i = 0; i < IRQS; i++) > + writel(AIC_SCR_SRCTYPE_HIGH_LEVEL | AIC_SCR_PRIORITY(7), > + aic->regs + AIC_SCR(i)); > +} > + > +static void __exception_irq_entry wpcm450_aic_handle_irq(struct pt_regs *regs) > +{ > + int hwirq; > + > + /* Determine the interrupt source */ > + /* Read IPER to signal that nIRQ can be de-asserted */ > + hwirq = readl(aic->regs + AIC_IPER) / 4; > + > + handle_domain_irq(aic->domain, hwirq, regs); > +} > + > +static void wpcm450_aic_ack(struct irq_data *d) > +{ > + /* Signal end-of-service */ > + writel(0, aic->regs + AIC_EOSCR); Is that an Ack or an EOI? My gut feeling is that the above read is the Ack, and that this write should actually be an EOI callback. > +} > + > +static void wpcm450_aic_mask(struct irq_data *d) > +{ > + unsigned int mask = 1U << d->hwirq; Consider using BIT(). > + > + /* Disable (mask) the interrupt */ > + writel(mask, aic->regs + AIC_MDCR); > +} > + > +static void wpcm450_aic_unmask(struct irq_data *d) > +{ > + unsigned int mask = 1U << d->hwirq; > + > + /* Enable (unmask) the interrupt */ > + writel(mask, aic->regs + AIC_MECR); > +} > + > +static int wpcm450_aic_set_type(struct irq_data *d, unsigned int flow_type) > +{ > + /* > + * The hardware supports high/low level, as well as rising/falling edge > + * modes, and the DT binding accommodates for that, but as long as > + * other modes than high level mode are not used and can't be tested, > + * they are rejected in this driver. > + */ > + if ((flow_type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_HIGH) { > + pr_err("IRQ mode %#x is not supported\n", flow_type); The core kernel shouts loudly enough, no need for extra messages. > + return -EINVAL; > + } > + > + return 0; > +} > + > +static struct irq_chip wpcm450_aic_chip = { > + .name = "wpcm450-aic", > + .irq_ack = wpcm450_aic_ack, > + .irq_mask = wpcm450_aic_mask, > + .irq_unmask = wpcm450_aic_unmask, > + .irq_set_type = wpcm450_aic_set_type, > +}; > + > +static int wpcm450_aic_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq) > +{ > + if (hwirq > IRQS) > + return -EPERM; > + > + irq_set_chip_and_handler(irq, &wpcm450_aic_chip, handle_level_irq); > + irq_set_chip_data(irq, aic); > + irq_set_probe(irq); > + > + return 0; > +} > + > +static const struct irq_domain_ops wpcm450_aic_ops = { > + .map = wpcm450_aic_map, > + .xlate = irq_domain_xlate_twocell, > +}; > + > +static int __init wpcm450_aic_of_init(struct device_node *node, > + struct device_node *parent) > +{ > + if (parent) > + return -EINVAL; > + > + aic = kzalloc(sizeof(*aic), GFP_KERNEL); > + if (!aic) > + return -ENOMEM; > + > + aic->regs = of_iomap(node, 0); > + if (!aic->regs) { > + pr_err("Failed to map WPCM450 AIC registers\n"); > + return -ENOMEM; > + } > + > + wpcm450_aic_init_hw(); > + > + set_handle_irq(wpcm450_aic_handle_irq); > + > + aic->domain = irq_domain_add_linear(node, IRQS, &wpcm450_aic_ops, aic); > + > + return 0; > +} > + > +IRQCHIP_DECLARE(wpcm450_aic, "nuvoton,wpcm450-aic", wpcm450_aic_of_init); Otherwise, looks good. M. -- Without deviation from the norm, progress is not possible.
On Wed, Mar 24, 2021 at 05:16:35PM +0000, Marc Zyngier wrote: > On Sat, 20 Mar 2021 18:16:04 +0000, > Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > > > > The WPCM450 AIC ("Advanced Interrupt Controller") is the interrupt > > controller found in the Nuvoton WPCM450 SoC and other Winbond/Nuvoton > > SoCs. > > > > The list of registers if based on the AMI vendor kernel and the > > Nuvoton W90N745 datasheet. > > > > Although the hardware supports other interrupt modes, the driver only > > supports high-level interrupts at the moment, because other modes could > > not be tested so far. > > > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > --- [...] > > +// SPDX-License-Identifier: GPL-2.0-only > > +// Copyright 2021 Jonathan Neuschäfer > > + > > +#include <linux/console.h> > > That's unexpected. Why do you need this? I forgot about linux/printk.h. > > +#define AIC_SCR_SRCTYPE_LOW_LEVEL (0 << 6) > > +#define AIC_SCR_SRCTYPE_HIGH_LEVEL (1 << 6) > > +#define AIC_SCR_SRCTYPE_NEG_EDGE (2 << 6) > > +#define AIC_SCR_SRCTYPE_POS_EDGE (3 << 6) > > +#define AIC_SCR_PRIORITY(x) (x) > > A mask would be welcomed for this field. Ok, I'll add +#define AIC_SCR_PRIORITY_MASK 0x7 Should I apply it in AIC_SCR_PRIORITY(x), too? > > + > > +#define IRQS 32 > > Please use something a bit less generic. Ok, I'll rename it to AIC_NUM_IRQS. > > +static void wpcm450_aic_init_hw(void) > > +{ > > + int i; > > + > > + /* Disable (mask) all interrupts */ > > + writel(0xffffffff, aic->regs + AIC_MDCR); > > Consider using relaxed accessors throughout this driver. I'll read up on how to use them correctly. > > +static void __exception_irq_entry wpcm450_aic_handle_irq(struct pt_regs *regs) > > +{ > > + int hwirq; > > + > > + /* Determine the interrupt source */ > > + /* Read IPER to signal that nIRQ can be de-asserted */ > > + hwirq = readl(aic->regs + AIC_IPER) / 4; > > + > > + handle_domain_irq(aic->domain, hwirq, regs); > > +} > > + > > +static void wpcm450_aic_ack(struct irq_data *d) > > +{ > > + /* Signal end-of-service */ > > + writel(0, aic->regs + AIC_EOSCR); > > Is that an Ack or an EOI? My gut feeling is that the above read is the > Ack, and that this write should actually be an EOI callback. I agree that EOSCR (End of Service Command Register) matches the description of EOI. The reading IPER serves a dual purpose, as indicated above. I could move the IPER read to a separate irq_ack function and use ISNR (Interrupt source number register) in the IRQ handler instead. This should work (haven't tested it yet), but I'm not sure it's strictly better. > > +static void wpcm450_aic_mask(struct irq_data *d) > > +{ > > + unsigned int mask = 1U << d->hwirq; > > Consider using BIT(). Will do. > > +static int wpcm450_aic_set_type(struct irq_data *d, unsigned int flow_type) > > +{ > > + /* > > + * The hardware supports high/low level, as well as rising/falling edge > > + * modes, and the DT binding accommodates for that, but as long as > > + * other modes than high level mode are not used and can't be tested, > > + * they are rejected in this driver. > > + */ > > + if ((flow_type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_HIGH) { > > + pr_err("IRQ mode %#x is not supported\n", flow_type); > > The core kernel shouts loudly enough, no need for extra messages. Ok. > Otherwise, looks good. > > M. Thanks for your review! Jonathan Neuschäfer
On Sat, 20 Mar 2021 19:15:57 +0100, Jonathan Neuschäfer wrote: > Super Micro Computer, Inc. (https://www.supermicro.com/en/), commonly > known as Supermicro, is a manufacturer of server hardware. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- > Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ > 1 file changed, 2 insertions(+) > Acked-by: Rob Herring <robh@kernel.org>
On Sat, 20 Mar 2021 19:15:58 +0100, Jonathan Neuschäfer wrote: > The WPCM450 is an older BMC SoC in the Nuvoton NPCM family, originally > marketed as Winbond WPCM450. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- > > This patch requires "dt-bindings: arm: Convert nuvoton,npcm750 binding to YAML" > (https://lore.kernel.org/lkml/20210320164023.614059-1-j.neuschaefer@gmx.net/) > --- > Documentation/devicetree/bindings/arm/npcm/npcm.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > Acked-by: Rob Herring <robh@kernel.org>
On Sat, 20 Mar 2021 19:15:59 +0100, Jonathan Neuschäfer wrote: > The WPCM450 AIC ("Advanced Interrupt Controller") is the interrupt > controller found in the Nuvoton WPCM450 SoC and other Winbond/Nuvoton > SoCs. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- > .../nuvoton,wpcm450-aic.yaml | 39 +++++++++++++++++++ > 1 file changed, 39 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nuvoton,wpcm450-aic.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
On Sat, 20 Mar 2021 19:16:02 +0100, Jonathan Neuschäfer wrote: > Add a compatible string for the WPCM450 SoC, which has the same watchdog > timer. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- > .../devicetree/bindings/watchdog/nuvoton,npcm-wdt.txt | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > Acked-by: Rob Herring <robh@kernel.org>
On Fri, Mar 26, 2021 at 07:52:07PM +0100, Jonathan Neuschäfer wrote: > On Wed, Mar 24, 2021 at 05:16:35PM +0000, Marc Zyngier wrote: > > On Sat, 20 Mar 2021 18:16:04 +0000, Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: [...] > > > + /* Disable (mask) all interrupts */ > > > + writel(0xffffffff, aic->regs + AIC_MDCR); > > > > Consider using relaxed accessors throughout this driver. > > I'll read up on how to use them correctly. Upon further consideration, I decided not to bother with relaxed MMIO access, because it doesn't make a difference on the target platform — Nuvoton WPCM450 with ARM926 (ARMv5). Best regards, Jonathan Neuschäfer