| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 25 Jan 2019 18:56:33 +0800
From: Jiaxun Yang <jiaxun.yang@...goat.com>
To: Marc Zyngier <marc.zyngier@....com>
Cc: linux-mips@...r.kernel.org, tglx@...utronix.de,
jason@...edaemon.net, robh+dt@...nel.org, mark.rutland@....com,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 1/2] irqchip: Add driver for Loongson-1 interrupt
controller
Hi Marc
Thanks for your suggestions, I'm working on v4 and I would like to ask
if it is better to have a driver for only one irqchip
and create dt nodes for each chip, or just register all the chips in a
single driver with only one dt node.
在 2019/1/24 下午5:54, Marc Zyngier 写道:
> On Thu, 24 Jan 2019 03:27:29 +0000,
> Jiaxun Yang <jiaxun.yang@...goat.com> wrote:
>> This controller appeared on Loongson-1 family MCUs
>> including Loongson-1B and Loongson-1C.
>>
>> Signed-off-by: Jiaxun Yang <jiaxun.yang@...goat.com>
>> ---
>> drivers/irqchip/Kconfig | 9 ++
>> drivers/irqchip/Makefile | 1 +
>> drivers/irqchip/irq-ls1x.c | 176 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 186 insertions(+)
>> create mode 100644 drivers/irqchip/irq-ls1x.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 3d1e60779078..5dcb5456cd14 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -406,6 +406,15 @@ config IMX_IRQSTEER
>> help
>> Support for the i.MX IRQSTEER interrupt multiplexer/remapper.
>>
>> +config LS1X_IRQ
>> + bool "Loongson-1 Interrupt Controller"
>> + depends on MACH_LOONGSON32
>> + default y
>> + select IRQ_DOMAIN
>> + select GENERIC_IRQ_CHIP
>> + help
>> + Support for the Loongson-1 platform Interrupt Controller.
>> +
>> endmenu
>>
>> config SIFIVE_PLIC
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index c93713d24b86..7acd0e36d0b4 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -94,3 +94,4 @@ obj-$(CONFIG_CSKY_APB_INTC) += irq-csky-apb-intc.o
>> obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o
>> obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o
>> obj-$(CONFIG_MADERA_IRQ) += irq-madera.o
>> +obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o
>> diff --git a/drivers/irqchip/irq-ls1x.c b/drivers/irqchip/irq-ls1x.c
>> new file mode 100644
>> index 000000000000..de92ca04cf9f
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-ls1x.c
>> @@ -0,0 +1,176 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019, Jiaxun Yang <jiaxun.yang@...goat.com>
>> + * Loongson-1 platform IRQ support
>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/types.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/ioport.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/io.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +
>> +#include <asm/mach-loongson32/irq.h>
>> +
>> +
>> +#define MAX_CHIPS 5
>> +#define LS_REG_INTC_STATUS 0x00
>> +#define LS_REG_INTC_EN 0x04
>> +#define LS_REG_INTC_SET 0x08
>> +#define LS_REG_INTC_CLR 0x0c
>> +#define LS_REG_INTC_POL 0x10
>> +#define LS_REG_INTC_EDGE 0x14
>> +#define CHIP_SIZE 0x18
>> +
>> +static void ls_chained_handle_irq(struct irq_desc *desc)
>> +{
>> + struct irq_chip_generic *gc = irq_desc_get_handler_data(desc);
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + u32 pending;
>> +
>> + chained_irq_enter(chip, desc);
>> + pending = readl(gc->reg_base + LS_REG_INTC_STATUS) &
>> + readl(gc->reg_base + LS_REG_INTC_EN);
>> +
>> + if (!pending) {
>> + spurious_interrupt();
>> + chained_irq_exit(chip, desc);
>> + return;
>> + }
> Given the context, this is the same as writing:
>
> if (!pending)
> spurious_interrupt();
>
> and let it go through.
>
>> +
>> + while (pending) {
>> + int bit = __ffs(pending);
>> +
>> + generic_handle_irq(gc->irq_base + bit);
>> + pending &= ~BIT(bit);
>> + }
>> +
>> + chained_irq_exit(chip, desc);
>> +}
>> +
>> +static void ls_intc_set_bit(struct irq_chip_generic *gc,
>> + unsigned int offset,
>> + u32 mask, bool set)
>> +{
>> + if (set)
>> + writel(readl(gc->reg_base + offset) | mask,
>> + gc->reg_base + offset);
>> + else
>> + writel(readl(gc->reg_base + offset) & ~mask,
>> + gc->reg_base + offset);
>> +}
>> +
>> +static int ls_intc_set_type(struct irq_data *data, unsigned int type)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
>> + u32 mask = data->mask;
>> +
>> + switch (type) {
>> + case IRQ_TYPE_LEVEL_HIGH:
>> + ls_intc_set_bit(gc, LS_REG_INTC_EDGE, mask, false);
>> + ls_intc_set_bit(gc, LS_REG_INTC_POL, mask, true);
>> + break;
>> + case IRQ_TYPE_LEVEL_LOW:
>> + ls_intc_set_bit(gc, LS_REG_INTC_EDGE, mask, false);
>> + ls_intc_set_bit(gc, LS_REG_INTC_POL, mask, false);
>> + break;
>> + case IRQ_TYPE_EDGE_RISING:
>> + ls_intc_set_bit(gc, LS_REG_INTC_EDGE, mask, true);
>> + ls_intc_set_bit(gc, LS_REG_INTC_POL, mask, true);
>> + break;
>> + case IRQ_TYPE_EDGE_FALLING:
>> + ls_intc_set_bit(gc, LS_REG_INTC_EDGE, mask, true);
>> + ls_intc_set_bit(gc, LS_REG_INTC_POL, mask, false);
>> + break;
>> + case IRQ_TYPE_NONE:
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
> I just realised that in your DT binding, you define the interrupt
> specifier as having a single cell. Which means that you never describe
> the interrupt trigger there. Why?
>
> Does it mean that all the drivers have to configure their interrupt
> trigger themselves, and cannot rely on DT to do it? This seems quite
> backward.
My fault, will fix it later.
>
>> +
>> + return 0;
>> +}
>> +
>> +
>> +static int __init ls_intc_of_init(struct device_node *node,
>> + struct device_node *parent)
>> +{
>> + struct irq_chip_generic *gc[MAX_CHIPS];
>> + struct irq_chip_type *ct;
>> + struct irq_domain *domain;
>> + void __iomem *base;
>> + int parent_irq[MAX_CHIPS], err = 0;
>> + unsigned int i = 0;
>> +
>> + unsigned int num_chips = of_irq_count(node);
>> +
>> + base = of_iomap(node, 0);
>> + if (!base)
>> + return -ENODEV;
>> +
>> + for (i = 0; i < num_chips; i++) {
>> + /* Mask all irqs */
>> + writel(0x0, base + (i * CHIP_SIZE) +
>> + LS_REG_INTC_EN);
>> +
>> + /* Set all irqs to high level triggered */
>> + writel(0xffffffff, base + (i * CHIP_SIZE) +
>> + LS_REG_INTC_POL);
>> +
>> + parent_irq[i] = irq_of_parse_and_map(node, i);
>> +
>> + if (!parent_irq[i]) {
>> + pr_warn("unable to get parent irq for irqchip %u\n", i);
>> + goto out_err;
>> + }
>> +
>> + gc[i] = irq_alloc_generic_chip("INTC", 1,
>> + LS1X_IRQ_BASE + (i * 32),
>> + base + (i * CHIP_SIZE),
>> + handle_level_irq);
>> +
>> + ct = gc[i]->chip_types;
>> + ct->regs.mask = LS_REG_INTC_EN;
>> + ct->regs.ack = LS_REG_INTC_CLR;
>> + ct->chip.irq_unmask = irq_gc_mask_set_bit;
>> + ct->chip.irq_mask = irq_gc_mask_clr_bit;
>> + ct->chip.irq_ack = irq_gc_ack_set_bit;
>> + ct->chip.irq_set_type = ls_intc_set_type;
>> +
>> + irq_setup_generic_chip(gc[i], IRQ_MSK(32), 0, 0,
>> + IRQ_NOPROBE | IRQ_LEVEL);
>> + }
>> +
>> + domain = irq_domain_add_legacy(node, num_chips * 32, LS1X_IRQ_BASE, 0,
>> + &irq_domain_simple_ops, NULL);
> Why a legacy domain? This is usually reserved to old drivers that are
> converted to a new infrastructure, while needing some form of platform
> hacks. I don't see this being the case here.
>
> It is also worrying that although you have up to 5 irqchips, they all
> share a single domain. What does this mean? each irqchip is expected
> to have its own domain.
Yes, I do like this for backward compatible reason. I'm turning
a legacy platform device mach(arch/mips/loongson32) in to
dt based generic mach and I would like to do it step by step rather than
one time.
So I use legacy domain in order to keep IRQ same with the
old driver exist on arch/mips/loongson32/common/irq.c
>> + if (!domain) {
>> + pr_warn("unable to register IRQ domain\n");
>> + err = -EINVAL;
>> + goto out_err;
>> + }
>> +
>> + for (i = 0; i < num_chips; i++)
>> + irq_set_chained_handler_and_data(parent_irq[i],
>> + ls_chained_handle_irq, gc[i]);
>> +
>> + pr_info("ls1x-irq: %u chips registered\n", num_chips);
>> +
>> + return 0;
>> +
>> +out_err:
>> + for (i = 0; i < MAX_CHIPS; i++) {
>> + if (gc[i])
> But you've never initialised gc[], nor parent_irq[]. So you'll get
> whatever the stack had at this point. Not great.
>
>> + irq_destroy_generic_chip(gc[i], IRQ_MSK(32),
>> + IRQ_NOPROBE | IRQ_LEVEL, 0);
>> + if (parent_irq[i])
>> + irq_dispose_mapping(parent_irq[i]);
>> + }
>> + return err;
>> +}
>> +
>> +IRQCHIP_DECLARE(ls1x_intc, "loongson,ls1x-intc", ls_intc_of_init);
>> --
>> 2.20.1
>>
> Thanks,
>
> M.
>
Powered by blists - more mailing lists