From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5D0C4C43334 for ; Thu, 21 Jul 2022 15:28:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229492AbiGUP2h (ORCPT ); Thu, 21 Jul 2022 11:28:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41582 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229830AbiGUP2g (ORCPT ); Thu, 21 Jul 2022 11:28:36 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B5E028049F; Thu, 21 Jul 2022 08:28:34 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 346A0B82588; Thu, 21 Jul 2022 15:28:33 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 88848C3411E; Thu, 21 Jul 2022 15:28:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1658417311; bh=jXmUhzRZG9wv3BJi3Eu3Yc6JvLmlUmge2/zgUgB/A+Y=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=gc5P3S+UCwQvPX5Ou0GwypC59MlUPCbiHKx0rE02ZKT7rnvvxx2FYDoCR6ZpXDOoO 6KHI2AN90aVB+wO2ULjPhomkzR7ui9S+gT2IxFTvO7pZ1I+cEC0z8ehfpDL8BGPYUK LVP2O77gdyhNqYVMey2TWvEQSjDMFmbd9j1GoUJIAVP6WBZO92cuJFp3lMHbId0EaD gCgvf/9XuLYFSwgxPuDHFfAkjgch4SRHIELgIJR0yP9V2xEKC90mQJmZauG+rmkUnG booxYxve36v/qEXv5WQnihHLyOt32ul3CRUE25eW48bzt7jm/9lYqUZRw4TtmBLz3b aZ61r+geXmBzg== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oEY6H-0097K0-46; Thu, 21 Jul 2022 16:28:29 +0100 Date: Thu, 21 Jul 2022 16:28:28 +0100 Message-ID: <87wnc6xz6r.wl-maz@kernel.org> From: Marc Zyngier To: Frank Li Cc: "jdmason@kudzu.us" , "tglx@linutronix.de" , "robh+dt@kernel.org" , "krzysztof.kozlowski+dt@linaro.org" , "shawnguo@kernel.org" , "s.hauer@pengutronix.de" , "kw@linux.com" , "bhelgaas@google.com" , "kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-pci@vger.kernel.org" , Peng Fan , Aisheng Dong , "kernel@pengutronix.de" , "festevam@gmail.com" , dl-linux-imx , "kishon@ti.com" , "lorenzo.pieralisi@arm.com" , "ntb@lists.linux.dev" Subject: Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi controller In-Reply-To: References: <20220720213036.1738628-1-Frank.Li@nxp.com> <20220720213036.1738628-3-Frank.Li@nxp.com> <874jza525l.wl-maz@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: frank.li@nxp.com, jdmason@kudzu.us, tglx@linutronix.de, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, shawnguo@kernel.org, s.hauer@pengutronix.de, kw@linux.com, bhelgaas@google.com, kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org, peng.fan@nxp.com, aisheng.dong@nxp.com, kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com, kishon@ti.com, lorenzo.pieralisi@arm.com, ntb@lists.linux.dev X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Thu, 21 Jul 2022 16:22:08 +0100, Frank Li wrote: > > > > > -----Original Message----- > > From: Marc Zyngier > > Sent: Thursday, July 21, 2022 2:57 AM > > To: Frank Li > > Cc: jdmason@kudzu.us; tglx@linutronix.de; robh+dt@kernel.org; > > krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org; > > s.hauer@pengutronix.de; kw@linux.com; bhelgaas@google.com; > > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan > > ; Aisheng Dong ; > > kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx > imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com; > > ntb@lists.linux.dev > > Subject: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi controller > > > > Caution: EXT Email > > > > On Wed, 20 Jul 2022 22:30:34 +0100, > > Frank Li wrote: > > > > > > MU support generate irq by write data to a register. > > > This patch make mu worked as msi controller. > > > So MU can do doorbell by using standard msi api. > > > > > > Signed-off-by: Frank Li > > > --- > > > drivers/irqchip/Kconfig | 7 + > > > drivers/irqchip/Makefile | 1 + > > > drivers/irqchip/irq-imx-mu-msi.c | 462 > > +++++++++++++++++++++++++++++++ > > > 3 files changed, 470 insertions(+) > > > create mode 100644 drivers/irqchip/irq-imx-mu-msi.c > > > > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > > > index 5e4e50122777d..4599471d880c0 100644 > > > --- a/drivers/irqchip/Kconfig > > > +++ b/drivers/irqchip/Kconfig > > > @@ -470,6 +470,13 @@ config IMX_INTMUX > > > help > > > Support for the i.MX INTMUX interrupt multiplexer. > > > > > > +config IMX_MU_MSI > > > + bool "i.MX MU work as MSI controller" > > > + default y if ARCH_MXC > > > + select IRQ_DOMAIN > > > + help > > > + MU work as MSI controller to do general doorbell > > > + > > > config LS1X_IRQ > > > bool "Loongson-1 Interrupt Controller" > > > depends on MACH_LOONGSON32 > > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > > > index 5d8e21d3dc6d8..870423746c783 100644 > > > --- a/drivers/irqchip/Makefile > > > +++ b/drivers/irqchip/Makefile > > > @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o > > > obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o > > > obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o > > > obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o > > > +obj-$(CONFIG_IMX_MU_MSI) += irq-imx-mu-msi.o > > > obj-$(CONFIG_MADERA_IRQ) += irq-madera.o > > > obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o > > > obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o > > > diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu- > > msi.c > > > new file mode 100644 > > > index 0000000000000..8277dba857759 > > > --- /dev/null > > > +++ b/drivers/irqchip/irq-imx-mu-msi.c > > > @@ -0,0 +1,462 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * NXP MU worked as MSI controller > > > + * > > > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel > > > > > + * Copyright 2022 NXP > > > + * Frank Li > > > + * Peng Fan > > > + * > > > + * Based on drivers/mailbox/imx-mailbox.c > > > + */ > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > + > > > +#define IMX_MU_CHANS 4 > > > + > > > +enum imx_mu_xcr { > > > + IMX_MU_GIER, > > > + IMX_MU_GCR, > > > + IMX_MU_TCR, > > > + IMX_MU_RCR, > > > + IMX_MU_xCR_MAX, > > > +}; > > > + > > > +enum imx_mu_xsr { > > > + IMX_MU_SR, > > > + IMX_MU_GSR, > > > + IMX_MU_TSR, > > > + IMX_MU_RSR, > > > +}; > > > + > > > +enum imx_mu_type { > > > + IMX_MU_V1 = BIT(0), > > > + IMX_MU_V2 = BIT(1), > > > + IMX_MU_V2_S4 = BIT(15), > > > +}; > > > + > > > +/* Receive Interrupt Enable */ > > > +#define IMX_MU_xCR_RIEn(type, x) (type & IMX_MU_V2 ? BIT(x) : BIT(24 > > + (3 - (x)))) > > > +#define IMX_MU_xSR_RFn(type, x) (type & IMX_MU_V2 ? BIT(x) : BIT(24 + > > (3 - (x)))) > > > + > > > +struct imx_mu_dcfg { > > > + enum imx_mu_type type; > > > + u32 xTR; /* Transmit Register0 */ > > > + u32 xRR; /* Receive Register0 */ > > > + u32 xSR[4]; /* Status Registers */ > > > + u32 xCR[4]; /* Control Registers */ > > > +}; > > > + > > > +struct imx_mu_msi { > > > + spinlock_t lock; > > > + struct platform_device *pdev; > > > + struct irq_domain *parent; > > > + struct irq_domain *msi_domain; > > > + void __iomem *regs; > > > + phys_addr_t msiir_addr; > > > + const struct imx_mu_dcfg *cfg; > > > + unsigned long used; > > > + u32 gic_irq; > > > + struct clk *clk; > > > + struct device *pd_a; > > > + struct device *pd_b; > > > + struct device_link *pd_link_a; > > > + struct device_link *pd_link_b; > > > +}; > > > + > > > +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs) > > > +{ > > > + iowrite32(val, msi_data->regs + offs); > > > +} > > > + > > > +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs) > > > +{ > > > + return ioread32(msi_data->regs + offs); > > > +} > > > + > > > +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum > > imx_mu_xcr type, u32 set, u32 clr) > > > +{ > > > + unsigned long flags; > > > + u32 val; > > > + > > > + spin_lock_irqsave(&msi_data->lock, flags); > > > + val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]); > > > + val &= ~clr; > > > + val |= set; > > > + imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]); > > > + spin_unlock_irqrestore(&msi_data->lock, flags); > > > + > > > + return val; > > > +} > > > + > > > +static void imx_mu_msi_mask_irq(struct irq_data *data) > > > +{ > > > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data- > > >parent_data); > > > > Urgh... No. Please don't go poking into the *parent* stuff. Implement > > the masking at the parent level, and use irq_chip_mask_parent() for > > this level, unless you can explain why you can't do otherwise. > > > > > + > > > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, > > IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq)); > > > > How about making this readable and move the dereference inside the macro? > > > > > +} > > > + > > > +static void imx_mu_msi_unmask_irq(struct irq_data *data) > > > +{ > > > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data- > > >parent_data); > > > + > > > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, > > IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq), 0); > > > +} > > > + > > > +static struct irq_chip imx_mu_msi_irq_chip = { > > > + .name = "MU-MSI", > > > + .irq_mask = imx_mu_msi_mask_irq, > > > + .irq_unmask = imx_mu_msi_unmask_irq, > > > +}; > > > + > > > +static struct msi_domain_ops its_pmsi_ops = { > > > +}; > > > > What does this have to do with the ITS? > > Without this, it will be crashed as access 0 address. Because the *name* of the structure has an influence on the behaviour of the kernel????? > > > > > > + > > > +static struct msi_domain_info imx_mu_msi_domain_info = { > > > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | > > > + MSI_FLAG_USE_DEF_CHIP_OPS | > > > + MSI_FLAG_PCI_MSIX), > > > > What does PCI_MSIX mean in this context? I really wish you used > > copy/paste a bit more carefully. > > > > > + .ops = &its_pmsi_ops, > > > + .chip = &imx_mu_msi_irq_chip, > > > +}; > > > + > > > +static void imx_mu_msi_compose_msg(struct irq_data *data, struct > > msi_msg *msg) > > > +{ > > > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data); > > > + > > > + msg->address_hi = upper_32_bits(msi_data->msiir_addr); > > > + msg->address_lo = lower_32_bits(msi_data->msiir_addr + 4 * data- > > >hwirq); > > > > This is a horrible way of writing this. you should compute the address > > first, and then apply the address split. > > > > > + msg->data = data->hwirq; > > > + > > > + iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg); > > > +} > > > + > > > +static int imx_mu_msi_set_affinity(struct irq_data *irq_data, > > > + const struct cpumask *mask, bool force) > > > + > > > +{ > > > + return IRQ_SET_MASK_OK; > > > +} > > > > Err... What effect does this have if you don't do anything? > > [Frank Li] Without this, it will be crashed as access 0 address. Then you should fix that bug, because what you have here makes absolutely no sense. > > > > > > + > > > +static struct irq_chip imx_mu_msi_parent_chip = { > > > + .name = "MU", > > > + .irq_compose_msi_msg = imx_mu_msi_compose_msg, > > > + .irq_set_affinity = imx_mu_msi_set_affinity, > > > +}; > > > + > > > +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain, > > > + unsigned int virq, > > > + unsigned int nr_irqs, > > > + void *args) > > > +{ > > > + struct imx_mu_msi *msi_data = domain->host_data; > > > + msi_alloc_info_t *info = args; > > > + int pos, err = 0; > > > + > > > + WARN_ON(nr_irqs != 1); > > > + > > > + spin_lock(&msi_data->lock); > > > > Interrupt fires after lock acquisition, handler masks the interrupt. > > Deadlock. > > [Frank Li] you are right, it should be spin_lock_irqsave. > > > > > > + pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS); > > > + if (pos < IMX_MU_CHANS) > > > + __set_bit(pos, &msi_data->used); > > > + else > > > + err = -ENOSPC; > > > + spin_unlock(&msi_data->lock); > > > + > > > + if (err) > > > + return err; > > > + > > > + err = iommu_dma_prepare_msi(info->desc, msi_data->msiir_addr + > > pos * 4); > > > > Does this HW even have an IOMMU? > > [Frank Li] we have a platform with iommu. > > > > > > + if (err) > > > + return err; > > > + > > > + irq_domain_set_info(domain, virq, pos, > > > + &imx_mu_msi_parent_chip, msi_data, > > > + handle_simple_irq, NULL, NULL); > > > > This is an edge interrupt. Please handle it like one. > > [Frank Li] Not sure what your means? A MSI is an edge interrupt. Use handle_edge_irq. > > > > > > + return 0; > > > +} > > > + > > > +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain, > > > + unsigned int virq, unsigned int nr_irqs) > > > +{ > > > + struct irq_data *d = irq_domain_get_irq_data(domain, virq); > > > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d); > > > + > > > + spin_lock(&msi_data->lock); > > > > Same problem. > > > > > + __clear_bit(d->hwirq, &msi_data->used); > > > + spin_unlock(&msi_data->lock); > > > +} > > > + > > > +static const struct irq_domain_ops imx_mu_msi_domain_ops = { > > > + .alloc = imx_mu_msi_domain_irq_alloc, > > > + .free = imx_mu_msi_domain_irq_free, > > > +}; > > > + > > > +static void imx_mu_msi_irq_handler(struct irq_desc *desc) > > > +{ > > > + struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc); > > > + u32 status; > > > + int i; > > > + > > > + status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]); > > > + > > > + chained_irq_enter(irq_desc_get_chip(desc), desc); > > > + for (i = 0; i < IMX_MU_CHANS; i++) { > > > + if (status & IMX_MU_xSR_RFn(msi_data->cfg->type, i)) { > > > + imx_mu_read(msi_data, msi_data->cfg->xRR + i * 4); > > > + generic_handle_domain_irq(msi_data->parent, i); > > > > Why the parent? You must start at the top of the hierarchy. > > > > > + } > > > + } > > > + chained_irq_exit(irq_desc_get_chip(desc), desc); > > > > If your MSIs are a chained interrupt, why do you even provide an > > affinity setting callback? > > [Frank Li] it will be crash if no affinity setting callback. Then you have to fix your driver. M. -- Without deviation from the norm, progress is not possible.