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 8C353C433EF for ; Sat, 9 Jul 2022 08:16:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229589AbiGIIQY (ORCPT ); Sat, 9 Jul 2022 04:16:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55614 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229588AbiGIIQX (ORCPT ); Sat, 9 Jul 2022 04:16:23 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9E92C7479D; Sat, 9 Jul 2022 01:16:22 -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 dfw.source.kernel.org (Postfix) with ESMTPS id 3B35960DD1; Sat, 9 Jul 2022 08:16:22 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 58FCDC3411C; Sat, 9 Jul 2022 08:16:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1657354581; bh=Kl4LrwMcV0jEgVeyaN0UKSr1LebWTO1m6bXIl4hmN3U=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=lV+MuBE5WlQTVkJkX6idN54XiOIhhYN0KigZFnazxeQixH2FmsVaS1EA5nRIoWMPb 6UBhOLp73j4iFbJ/sGcSp2F01CRHAODTtzCpGemi9B8HDbIAz5vWF2+bMl3PLWBhGC shKKs/yzeWqwl1pjVkLTLjQMfcwcHDEv+sOOMFZAYHOyu34eWti/0jRRuBMnqxbXUF 9A0y/QkwzeF5Zy/1yZaPZmNiE+GvyI+GlevUI5toqBRWCHeD9mNcoTG4y1WJObxxTX vEKKF/r5IrCm7Kajyfj2+tfEzLhlhFSkdwUA6zO2SVoPFbgcNj0niDdX4hxjMHc9xY Qx2bOxYqcgnww== 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 1oA5dT-006K8H-0k; Sat, 09 Jul 2022 09:16:19 +0100 Date: Sat, 09 Jul 2022 09:16:18 +0100 Message-ID: <87fsja1z1p.wl-maz@kernel.org> From: Marc Zyngier To: Frank Li Cc: "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" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-pci@vger.kernel.org" , Peng Fan , Aisheng Dong , "jdmason@kudzu.us" , "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 1/3] irqchip: imx mu worked as msi controller In-Reply-To: References: <20220707210238.917477-1-Frank.Li@nxp.com> <87r12wkmkm.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, 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, linux-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, jdmason@kudzu.us, 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 Fri, 08 Jul 2022 17:26:33 +0100, Frank Li wrote: > > > > > -----Original Message----- > > From: Marc Zyngier > > Sent: Friday, July 8, 2022 3:59 AM > > To: Frank Li > > Cc: 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; linux- > > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan > > ; Aisheng Dong ; > > jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux- > > imx ; kishon@ti.com; lorenzo.pieralisi@arm.com; > > ntb@lists.linux.dev > > Subject: [EXT] Re: [PATCH 1/3] irqchip: imx mu worked as msi controller > > > > Caution: EXT Email > > > > On Thu, 07 Jul 2022 22:02:36 +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 | 490 > > +++++++++++++++++++++++++++++++ > > > 3 files changed, 498 insertions(+) > > > create mode 100644 drivers/irqchip/irq-imx-mu-msi.c > > > [...] > > > +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); > > > + > > > + pci_msi_mask_irq(data); > > > > What is this? Below, you create a platform MSI domain. Either you > > support PCI, and you create a PCI/MSI domain (and the above may make > > sense), or you are doing platform MSI, and the above is non-sense. > > [Frank Li] You are right. This work as platform msi. Needn't call pci_msi_irq() OK, hold that thought and see below. > > > +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; > > > + > > > + pm_runtime_get_sync(&msi_data->pdev->dev); > > > > The core code already deals with runtime PM. What prevents it from > > working, other than the fact you don't populate the device in the > > top-level domain? > > [Frank Li] Do you means power domain or irq domain? IRQ domain. See irq_domain_set_pm_device() and how PM is used on interrupt request. [...] > > > +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); > > > + int pos; > > > + > > > + pos = d->hwirq; > > > + if (pos < 0 || pos >= msi_data->irqs_num) { > > > + pr_err("failed to teardown msi. Invalid hwirq %d\n", pos); > > > + return; > > > + } > > > > How can this happen? > > I just copy from irq-ls-scfg-msi.c I wish you didn't do that. > It should be impossible happen if everything work as expected. Then it should go. [...] > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = { > > > + .xTR = 0x0, > > > + .xRR = 0x10, > > > + .xSR = {0x20, 0x20, 0x20, 0x20}, > > > + .xCR = {0x24, 0x24, 0x24, 0x24}, > > > +}; > > > + > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = { > > > + .xTR = 0x20, > > > + .xRR = 0x40, > > > + .xSR = {0x60, 0x60, 0x60, 0x60}, > > > + .xCR = {0x64, 0x64, 0x64, 0x64}, > > > +}; > > > + > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = { > > > + .type = IMX_MU_V2, > > > + .xTR = 0x200, > > > + .xRR = 0x280, > > > + .xSR = {0xC, 0x118, 0x124, 0x12C}, > > > + .xCR = {0x110, 0x114, 0x120, 0x128}, > > > +}; > > > + > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = { > > > + .type = IMX_MU_V2 | IMX_MU_V2_S4, > > > + .xTR = 0x200, > > > + .xRR = 0x280, > > > + .xSR = {0xC, 0x118, 0x124, 0x12C}, > > > + .xCR = {0x110, 0x114, 0x120, 0x128}, > > > +}; > > > > What are these? We really don't need more magic numbers. > > It is register offset. The difference version MU hardware's > register map is difference. Then please document what this is, what the various registers are, and correctly set type everywhere. [...] > > If that's hardcoded, why do we need an extra variable? I also question > > the usefulness of this driver if the HW can only deal with *4* MSIs... > > This looks a bit like a joke. > > MU don't really MSI controller. Each MU have 4 channel. > I.MX have several MU units. Then is it really useful to model that as a MSI controller? This smells of a mailbox controller to me instead. And I really worry that this device doesn't correctly preserve the ordering between a device doing DMA and generating an interrupt to indicate completion of the DMA transaction... Does this block offers such a guarantee? > PCI EP driver need an address as doorbell, so PCI RC side can write > This address to trigger irq. Ideally, it use GIC-ITS. But our i.MX chip > Have not ITS support yet now. So we can use MU as simple MSI controller. Is that an integrated EP on the same SoC? Or are you talking of two SoCs connected over PCIe? Also, you explicitly said that this was *not* a PCI/MSI controller. So what is this all about? [...] > > > +static int imx_mu_msi_remove(struct platform_device *pdev) > > > +{ > > > + struct imx_mu_msi *msi_data = platform_get_drvdata(pdev); > > > + > > > + imx_mu_msi_teardown_hwirq(msi_data); > > > + > > > + irq_domain_remove(msi_data->msi_domain); > > > + irq_domain_remove(msi_data->parent); > > > > How do you ensure that no device is still holding interrupts? Let me > > give you a hint: you can't. So removing an interrupt controller module > > should not be possible. > > [Frank Li] I agree. But there are many *_remove under irqchip. That doesn't make it right. Thanks, M. -- Without deviation from the norm, progress is not possible.