On Wed, 22 Apr 2020 22:24:23 +0800
Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>
> + * 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...
