lkml.org 
[lkml]   [2020]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/6] irqchip: Add Loongson PCH PIC controller
    On Wed, 22 Apr 2020 22:24:23 +0800
    Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:

    > This controller appears on Loongson-7A family of PCH to transform
    > interrupts from devices into HyperTransport vectorized interrrupts
    > and send them to procrssor's HT vector controller.
    >
    > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
    > ---
    > drivers/irqchip/Kconfig | 8 +
    > drivers/irqchip/Makefile | 1 +
    > drivers/irqchip/irq-loongson-pch-pic.c | 256 +++++++++++++++++++++++++
    > 3 files changed, 265 insertions(+)
    > create mode 100644 drivers/irqchip/irq-loongson-pch-pic.c
    >
    > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
    > index de4564e2ea88..4ab7a9b1a5c2 100644
    > --- a/drivers/irqchip/Kconfig
    > +++ b/drivers/irqchip/Kconfig
    > @@ -540,4 +540,12 @@ config LOONGSON_HTVEC
    > help
    > Support for the Loongson3 HyperTransport Interrupt Vector Controller.
    >
    > +config LOONGSON_PCH_PIC
    > + bool "Loongson PCH PIC Controller"
    > + depends on MACH_LOONGSON64 || COMPILE_TEST
    > + default MACH_LOONGSON64
    > + select IRQ_DOMAIN_HIERARCHY
    > + help
    > + Support for the Loongson PCH PIC Controller.
    > +
    > endmenu
    > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
    > index 74561879f5a7..acc72331cec8 100644
    > --- a/drivers/irqchip/Makefile
    > +++ b/drivers/irqchip/Makefile
    > @@ -108,3 +108,4 @@ obj-$(CONFIG_TI_SCI_INTA_IRQCHIP) += irq-ti-sci-inta.o
    > obj-$(CONFIG_LOONGSON_LIOINTC) += irq-loongson-liointc.o
    > obj-$(CONFIG_LOONGSON_HTPIC) += irq-loongson-htpic.o
    > obj-$(CONFIG_LOONGSON_HTVEC) += irq-loongson-htvec.o
    > +obj-$(CONFIG_LOONGSON_PCH_PIC) += irq-loongson-pch-pic.o
    > diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c
    > new file mode 100644
    > index 000000000000..717ab8335074
    > --- /dev/null
    > +++ b/drivers/irqchip/irq-loongson-pch-pic.c
    > @@ -0,0 +1,256 @@
    > +// SPDX-License-Identifier: GPL-2.0
    > +/*
    > + * Copyright (C) 2020, Jiaxun Yang <jiaxun.yang@flygoat.com>
    > + * Loongson PCH PIC support
    > + */
    > +
    > +#define pr_fmt(fmt) "pch-pic: " fmt
    > +
    > +#include <linux/interrupt.h>
    > +#include <linux/irq.h>
    > +#include <linux/irqchip.h>
    > +#include <linux/irqdomain.h>
    > +#include <linux/kernel.h>
    > +#include <linux/platform_device.h>
    > +#include <linux/of_address.h>
    > +#include <linux/of_irq.h>
    > +#include <linux/of_platform.h>
    > +
    > +/* Registers */
    > +#define PCH_PIC_MASK 0x20
    > +#define PCH_PIC_HTMSI_EN 0x40
    > +#define PCH_PIC_EDGE 0x60
    > +#define PCH_PIC_CLR 0x80
    > +#define PCH_PIC_AUTO0 0xc0
    > +#define PCH_PIC_AUTO1 0xe0
    > +#define PCH_INT_ROUTE(irq) (0x100 + irq)
    > +#define PCH_INT_HTVEC(irq) (0x200 + irq)
    > +#define PCH_PIC_POL 0x3e0
    > +
    > +#define PIC_COUNT_PER_REG 32
    > +#define PIC_REG_COUNT 2
    > +#define PIC_COUNT (PIC_COUNT_PER_REG * PIC_REG_COUNT)
    > +#define PIC_REG_IDX(irq_id) ((irq_id) / PIC_COUNT_PER_REG)
    > +#define PIC_REG_BIT(irq_id) ((irq_id) % PIC_COUNT_PER_REG)
    > +
    > +struct pch_pic {
    > + void __iomem *base;
    > + struct irq_domain *pic_domain;
    > + int ht_vec_base;
    > + raw_spinlock_t pic_lock;
    > +};

    Same comment about the layout of the data structure.

    > +
    > +static void pch_pic_bitset(void __iomem *addr, int bit)
    > +{
    > + u32 reg;
    > +
    > + addr += PIC_REG_IDX(bit) * 4;
    > + reg = readl(addr);
    > + reg |= BIT(PIC_REG_BIT(bit));
    > + writel(reg, addr);
    > +}
    > +
    > +static void pch_pic_bitclr(void __iomem *addr, int bit)
    > +{
    > + u32 reg;
    > +
    > + addr += PIC_REG_IDX(bit) * 4;
    > + reg = readl(addr);
    > + reg &= ~BIT(PIC_REG_BIT(bit));
    > + writel(reg, addr);
    > +}
    > +
    > +static void pch_pic_eoi_irq(struct irq_data *d)
    > +{
    > + struct pch_pic *priv = irq_data_get_irq_chip_data(d);
    > + u32 idx = PIC_REG_IDX(d->hwirq);
    > +
    > + writel(BIT(PIC_REG_BIT(d->hwirq)),
    > + priv->base + PCH_PIC_CLR + idx * 4);
    > +}
    > +
    > +static void pch_pic_mask_irq(struct irq_data *d)
    > +{
    > + struct pch_pic *priv = irq_data_get_irq_chip_data(d);
    > + unsigned long flags;
    > +
    > + raw_spin_lock_irqsave(&priv->pic_lock, flags);
    > + pch_pic_bitset(priv->base + PCH_PIC_MASK, d->hwirq);
    > + raw_spin_unlock_irqrestore(&priv->pic_lock, flags);
    > + irq_chip_mask_parent(d);
    > +}
    > +
    > +static void pch_pic_unmask_irq(struct irq_data *d)
    > +{
    > + struct pch_pic *priv = irq_data_get_irq_chip_data(d);
    > + unsigned long flags;
    > +
    > + raw_spin_lock_irqsave(&priv->pic_lock, flags);
    > + pch_pic_bitclr(priv->base + PCH_PIC_MASK, d->hwirq);
    > + raw_spin_unlock_irqrestore(&priv->pic_lock, flags);
    > + irq_chip_unmask_parent(d);
    > +}
    > +
    > +static int pch_pic_set_type(struct irq_data *d, unsigned int type)
    > +{
    > + struct pch_pic *priv = irq_data_get_irq_chip_data(d);
    > + int ret = 0;
    > + unsigned long flags;
    > +
    > + raw_spin_lock_irqsave(&priv->pic_lock, flags);
    > + switch (type) {
    > + case IRQ_TYPE_EDGE_RISING:
    > + pch_pic_bitset(priv->base + PCH_PIC_EDGE, d->hwirq);
    > + pch_pic_bitclr(priv->base + PCH_PIC_POL, d->hwirq);
    > + break;
    > + case IRQ_TYPE_EDGE_FALLING:
    > + pch_pic_bitset(priv->base + PCH_PIC_EDGE, d->hwirq);
    > + pch_pic_bitset(priv->base + PCH_PIC_POL, d->hwirq);
    > + break;
    > + case IRQ_TYPE_LEVEL_HIGH:
    > + pch_pic_bitclr(priv->base + PCH_PIC_EDGE, d->hwirq);
    > + pch_pic_bitclr(priv->base + PCH_PIC_POL, d->hwirq);
    > + break;
    > + case IRQ_TYPE_LEVEL_LOW:
    > + pch_pic_bitclr(priv->base + PCH_PIC_EDGE, d->hwirq);
    > + pch_pic_bitset(priv->base + PCH_PIC_POL, d->hwirq);
    > + break;
    > + default:
    > + ret = -EINVAL;
    > + break;
    > + }
    > + raw_spin_unlock_irqrestore(&priv->pic_lock, flags);

    It would make a lot more sense if the locking was done in the
    bitclr/bitset functions, unless there is a reason for wanting larger
    critical sections (but I can't see it at the moment).

    > +
    > + return ret;
    > +}
    > +
    > +static void pch_pic_enable_irq(struct irq_data *d)
    > +{
    > + struct pch_pic *priv = irq_data_get_irq_chip_data(d);
    > + u8 htvec = d->hwirq + priv->ht_vec_base;
    > + unsigned long flags;
    > +
    > + raw_spin_lock_irqsave(&priv->pic_lock, flags);
    > + writeb(htvec, priv->base + PCH_INT_HTVEC(d->hwirq));
    > + /* Hardcode to HT0 Lo */
    > + writeb(1, priv->base + PCH_INT_ROUTE(d->hwirq));
    > + /* Clear auto bounce, we don't need that */
    > + pch_pic_bitclr(priv->base + PCH_PIC_AUTO0, d->hwirq);
    > + pch_pic_bitclr(priv->base + PCH_PIC_AUTO1, d->hwirq);
    > + /* Enable HTMSI transformer */
    > + pch_pic_bitset(priv->base + PCH_PIC_HTMSI_EN, d->hwirq);
    > + raw_spin_unlock_irqrestore(&priv->pic_lock, flags);
    > + pch_pic_unmask_irq(d);

    This looks wrong. enable/disable should not call unmask/mask. It is
    also odd that you don't have a proper disable callback that does the
    opposite of enable (you call the mask callback, which is also wrong).

    I suggest you kill enable/disable altogether, and move this code to the
    the probe (or reset) function.

    > +}
    > +
    > +static struct irq_chip pch_pic_irq_chip = {
    > + .name = "PCH PIC",
    > + .irq_eoi = pch_pic_eoi_irq,
    > + .irq_mask = pch_pic_mask_irq,
    > + .irq_unmask = pch_pic_unmask_irq,
    > + .irq_enable = pch_pic_enable_irq,
    > + .irq_disable = pch_pic_mask_irq,
    > + .irq_set_affinity = irq_chip_set_affinity_parent,

    Given that this plugs into the HTVEC irqchip, which doesn't implement
    an affinity method (nor that it could, given that it is itself a
    chained irqchip), I don't see the point...

    > + .irq_set_type = pch_pic_set_type,
    > +};
    > +
    > +static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq,
    > + unsigned int nr_irqs, void *arg)
    > +{
    > + struct pch_pic *priv = domain->host_data;
    > + struct irq_fwspec fwspec;
    > + unsigned long hwirq;
    > + unsigned int type;
    > + int err;
    > +
    > + irq_domain_translate_twocell(domain, arg, &hwirq, &type);
    > +
    > + fwspec.fwnode = domain->parent->fwnode;
    > + fwspec.param_count = 1;
    > + fwspec.param[0] = hwirq + priv->ht_vec_base;
    > +
    > + err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
    > + if (err)
    > + return err;
    > +
    > + irq_domain_set_info(domain, virq, hwirq,
    > + &pch_pic_irq_chip, priv,
    > + handle_fasteoi_irq, NULL, NULL);
    > + irq_set_probe(virq);
    > +
    > + return 0;
    > +}
    > +
    > +static const struct irq_domain_ops pch_pic_domain_ops = {
    > + .translate = irq_domain_translate_twocell,
    > + .alloc = pch_pic_alloc,
    > + .free = irq_domain_free_irqs_parent,
    > +};
    > +
    > +static void pch_pic_reset(struct pch_pic *priv)
    > +{
    > + u32 idx;
    > +
    > + /* Clear IRQ cause registers, mask all interrupts */
    > + for (idx = 0; idx < PIC_REG_COUNT; idx++) {
    > + writel_relaxed(0xFFFFFFFF, priv->base + PCH_PIC_MASK + 4 * idx);
    > + writel_relaxed(0xFFFFFFFF, priv->base + PCH_PIC_CLR + 4 * idx);
    > + }
    > +}
    > +
    > +static int pch_pic_of_init(struct device_node *node,
    > + struct device_node *parent)
    > +{
    > + struct pch_pic *priv;
    > + struct irq_domain *parent_domain;
    > + int err;
    > + u32 ht_vec_base;
    > +
    > + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
    > + if (!priv)
    > + return -ENOMEM;
    > +
    > + raw_spin_lock_init(&priv->pic_lock);
    > + priv->base = of_iomap(node, 0);
    > + if (!priv->base) {
    > + err = -ENOMEM;
    > + goto free_priv;
    > + }
    > +
    > + parent_domain = irq_find_host(parent);
    > + if (!parent_domain) {
    > + pr_err("Failed to find the parent domain\n");
    > + err = -ENXIO;
    > + goto iounmap_base;
    > + }
    > +
    > + if (of_property_read_u32(node, "loongson,pic-base-vec",
    > + &ht_vec_base))
    > + priv->ht_vec_base = 64;

    The binding doesn't describe the property as being optional.

    > + else
    > + priv->ht_vec_base = ht_vec_base;
    > +
    > + priv->pic_domain = irq_domain_create_hierarchy(parent_domain, 0,
    > + PIC_COUNT,
    > + of_node_to_fwnode(node),
    > + &pch_pic_domain_ops,
    > + priv);
    > + if (!priv->pic_domain) {
    > + pr_err("Failed to create IRQ domain\n");
    > + err = -ENOMEM;
    > + goto iounmap_base;
    > + }
    > +
    > + pch_pic_reset(priv);
    > +
    > + return 0;
    > +
    > +iounmap_base:
    > + iounmap(priv->base);
    > +free_priv:
    > + kfree(priv);
    > +
    > + return err;
    > +}
    > +
    > +IRQCHIP_DECLARE(pch_pic, "loongson,pch-pic-1.0", pch_pic_of_init);

    Thanks,

    M.
    --
    Jazz is not dead. It just smells funny...

    \
     
     \ /
      Last update: 2020-04-23 16:24    [W:3.580 / U:0.356 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site