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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3B285C3DA6F for ; Fri, 25 Aug 2023 11:58:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:CC:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date :Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=QBD7nxQKgbFFZOtzZ/LfRXyJMa7owQK17x2BpPqra58=; b=2e8tvN/bpxlQeU6sb0/QiM5XF/ O+9aKD+fIi6v+9Y0mXe17z0lWZUwBuoZi9bO17liTyYxYYLPG/sCakxWe2hqImgp/dTOtEKLhnEk4 hyO5gxECOVRRaFUqUbaU+X3S/R3z0VEaaFNksoXXbv0XjJRGhvTlce0J/xehxsd5Qi2oQ7O1zVEXC PUpj6tm+a2t0YLLwpr7s8E12kWkTTJLfg5Ld1MBqPqGJaEzqJAYf3f5npkg9j+/OdrPxHZBvG5Dep gHUXnR0CiwU1YE0eFsi6VTc+nrdnCAbdzNwwclJiqxUxpu80Hsx+QtA/ubWZFX1d0tHCCtwI0FPcr zOVUnoeg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qZVRp-005KAg-2x; Fri, 25 Aug 2023 11:57:53 +0000 Received: from esa.microchip.iphmx.com ([68.232.154.123]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qZVRm-005KAF-1P for linux-riscv@lists.infradead.org; Fri, 25 Aug 2023 11:57:52 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1692964670; x=1724500670; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=PyJ/YRwYJz7mq3U4+yTh/5HPypy5nwmdt+KOBtbkj+c=; b=E1Fz/oeDWc8cCdA9/3j7wM5zJ3iUefPRZX8pKVJrrMYf2FJcAtAjEZqQ 0YawXioKj5yMGvmFfseXR2KGkIVu4cWu1TaCkThEKWP/hoYRxHMYu4LCn YCYm52Je8dIEe9mtfKdxFwQ7RbGkiFtlLoT18dXJVWrsSdCC/9IOtHqjh V5yAVp7fw86U3rfo896X6MJh7uwYIkSQ99HeqrVL52wix89knP4Ck0aTp eTAQkHfnfmesvNUluAN4rjJdA+lbDoWUcp9a30KqNsgkQ59bcQ/riV7ku k8/t01fqHSWPb6aRndQaItMqc/CLZzFUFvAiCrOsSVaE0DO83S1aoObYw Q==; X-IronPort-AV: E=Sophos;i="6.02,195,1688454000"; d="asc'?scan'208";a="168286757" X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN Received: from unknown (HELO email.microchip.com) ([170.129.1.10]) by esa6.microchip.iphmx.com with ESMTP/TLS/AES256-SHA256; 25 Aug 2023 04:57:44 -0700 Received: from chn-vm-ex01.mchp-main.com (10.10.85.143) by chn-vm-ex04.mchp-main.com (10.10.85.152) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.21; Fri, 25 Aug 2023 04:56:47 -0700 Received: from wendy (10.10.85.11) by chn-vm-ex01.mchp-main.com (10.10.85.143) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.21 via Frontend Transport; Fri, 25 Aug 2023 04:56:44 -0700 Date: Fri, 25 Aug 2023 12:56:03 +0100 From: Conor Dooley To: Minda Chen CC: Daire McNamara , Conor Dooley , Rob Herring , Krzysztof Kozlowski , Bjorn Helgaas , Lorenzo Pieralisi , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Emil Renner Berthing , , , , , Pali =?iso-8859-1?Q?Roh=E1r?= , Paul Walmsley , Palmer Dabbelt , Albert Ou , Philipp Zabel , Mason Huo , Leyfoon Tan , Kevin Xie Subject: Re: [PATCH v4 07/11] PCI: microchip: Rename IRQ init function Message-ID: <20230825-undusted-detached-1d67370c0a18@wendy> References: <20230825090129.65721-1-minda.chen@starfivetech.com> <20230825090129.65721-8-minda.chen@starfivetech.com> MIME-Version: 1.0 In-Reply-To: <20230825090129.65721-8-minda.chen@starfivetech.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230825_045750_647944_3472E4C8 X-CRM114-Status: GOOD ( 30.35 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============6038154554904878791==" Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org --===============6038154554904878791== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="XmI2SV5XQAzfiIRv" Content-Disposition: inline --XmI2SV5XQAzfiIRv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hey Minda, I would like Daire to take a look at this, but I have a few more smaller comments on this. On Fri, Aug 25, 2023 at 05:01:25PM +0800, Minda Chen wrote: > Rename IRQ init function and prepare for re-use > IRQ init function. > Add plda_pcie_ops function pointer data structure, > PolarFire PCIe uses function pointer to get > their events num. >=20 > rename list: > mc_init_interrupts() -> plda_init_interrupts() > mc_pcie_init_irq_domain()-> plda_pcie_init_irq_domains() >=20 > Signed-off-by: Minda Chen > --- > .../pci/controller/plda/pcie-microchip-host.c | 49 ++++++++++++++----- > drivers/pci/controller/plda/pcie-plda.h | 14 ++++++ > 2 files changed, 51 insertions(+), 12 deletions(-) >=20 > diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/= pci/controller/plda/pcie-microchip-host.c > index b1d5b5b3cee5..03e8e93ea7e4 100644 > --- a/drivers/pci/controller/plda/pcie-microchip-host.c > +++ b/drivers/pci/controller/plda/pcie-microchip-host.c > @@ -416,7 +416,10 @@ static void mc_handle_event(struct irq_desc *desc) > =20 > chained_irq_enter(chip, desc); > =20 > - events =3D get_events(port); > + if (port->ops && port->ops->get_events) > + events =3D port->ops->get_events(port); > + else > + events =3D get_events(port); I don't really understand why we are going to having two different sorts of event acquirers here. Is there a reason to not "just" register=20 get_events() as the default callback & remove the special casing? > =20 > for_each_set_bit(bit, &events, NUM_EVENTS) > generic_handle_domain_irq(port->event_domain, bit); > @@ -562,11 +565,12 @@ static int mc_pcie_init_clks(struct device *dev) > return 0; > } > =20 > -static int mc_pcie_init_irq_domains(struct plda_pcie_rp *port) > +static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port, struct = plda_evt *evt) Could you just spell out the word "event" in all of these data structures and variables? The existing code seems to always spell it out, so its a boon for consistency too. > { > struct device *dev =3D port->dev; > struct device_node *node =3D dev->of_node; > struct device_node *pcie_intc_node; > + const struct irq_domain_ops *ops; > =20 > /* Setup INTx */ > pcie_intc_node =3D of_get_next_child(node, NULL); > @@ -575,8 +579,9 @@ static int mc_pcie_init_irq_domains(struct plda_pcie_= rp *port) > return -EINVAL; > } > =20 > - port->event_domain =3D irq_domain_add_linear(pcie_intc_node, NUM_EVENTS, > - &event_domain_ops, port); > + ops =3D evt->domain_ops ? evt->domain_ops : &event_domain_ops; > + port->event_domain =3D irq_domain_add_linear(pcie_intc_node, port->num_= events, > + ops, port); > if (!port->event_domain) { > dev_err(dev, "failed to get event domain\n"); > of_node_put(pcie_intc_node); > @@ -661,14 +666,15 @@ static void mc_disable_interrupts(struct mc_pcie *p= ort) > writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST); > } > =20 > -static int mc_init_interrupts(struct platform_device *pdev, struct plda_= pcie_rp *port) > +static int plda_init_interrupts(struct platform_device *pdev, > + struct plda_pcie_rp *port, struct plda_evt *evt) > { > struct device *dev =3D &pdev->dev; > int irq; > int i, intx_irq, msi_irq, event_irq; > int ret; > =20 > - ret =3D mc_pcie_init_irq_domains(port); > + ret =3D plda_pcie_init_irq_domains(port, evt); > if (ret) { > dev_err(dev, "failed creating IRQ domains\n"); > return ret; > @@ -678,15 +684,18 @@ static int mc_init_interrupts(struct platform_devic= e *pdev, struct plda_pcie_rp > if (irq < 0) > return -ENODEV; > =20 > - for (i =3D 0; i < NUM_EVENTS; i++) { > + for (i =3D 0; i < port->num_events; i++) { > event_irq =3D irq_create_mapping(port->event_domain, i); > if (!event_irq) { > dev_err(dev, "failed to map hwirq %d\n", i); > return -ENXIO; > } > =20 > - ret =3D devm_request_irq(dev, event_irq, mc_event_handler, > - 0, event_cause[i].sym, port); > + if (evt->request_evt_irq) > + ret =3D evt->request_evt_irq(port, event_irq, i); > + else > + ret =3D devm_request_irq(dev, event_irq, plda_event_handler, > + 0, NULL, port); > if (ret) { > dev_err(dev, "failed to request IRQ %d\n", event_irq); > return ret; > @@ -694,7 +703,7 @@ static int mc_init_interrupts(struct platform_device = *pdev, struct plda_pcie_rp > } > =20 > intx_irq =3D irq_create_mapping(port->event_domain, > - EVENT_LOCAL_PM_MSI_INT_INTX); > + evt->intx_evt); > if (!intx_irq) { > dev_err(dev, "failed to map INTx interrupt\n"); > return -ENXIO; > @@ -704,7 +713,7 @@ static int mc_init_interrupts(struct platform_device = *pdev, struct plda_pcie_rp > irq_set_chained_handler_and_data(intx_irq, plda_handle_intx, port); > =20 > msi_irq =3D irq_create_mapping(port->event_domain, > - EVENT_LOCAL_PM_MSI_INT_MSI); > + evt->msi_evt); > if (!msi_irq) > return -ENXIO; > =20 > @@ -717,6 +726,17 @@ static int mc_init_interrupts(struct platform_device= *pdev, struct plda_pcie_rp > return 0; > } > =20 > +static int mc_request_evt_irq(struct plda_pcie_rp *plda, int event_irq, > + int evt) > +{ > + return devm_request_irq(plda->dev, event_irq, mc_event_handler, > + 0, event_cause[evt].sym, plda); > +} > + > +static const struct plda_pcie_ops plda_ops =3D { > + .get_events =3D get_events, > +}; This struct gets left behind in the microchip driver file, but is named "plda_ops". That doesn't make sense to me, I think it should have an "mc" prefix like other stuff in the file (and the microchip version of get_events() should also grow a prefix IMO). Thanks, Conor. > + > static int mc_platform_init(struct pci_config_window *cfg) > { > struct device *dev =3D cfg->parent; > @@ -724,6 +744,9 @@ static int mc_platform_init(struct pci_config_window = *cfg) > void __iomem *bridge_base_addr =3D > port->axi_base_addr + MC_PCIE_BRIDGE_ADDR; > struct pci_host_bridge *bridge =3D platform_get_drvdata(pdev); > + struct plda_evt evt =3D {&event_domain_ops, mc_request_evt_irq, > + EVENT_LOCAL_PM_MSI_INT_INTX, > + EVENT_LOCAL_PM_MSI_INT_MSI}; > int ret; > =20 > /* Configure address translation table 0 for PCIe config space */ > @@ -740,7 +763,7 @@ static int mc_platform_init(struct pci_config_window = *cfg) > return ret; > =20 > /* Address translation is up; safe to enable interrupts */ > - ret =3D mc_init_interrupts(pdev, &port->plda); > + ret =3D plda_init_interrupts(pdev, &port->plda, &evt); > if (ret) > return ret; > =20 > @@ -761,6 +784,8 @@ static int mc_host_probe(struct platform_device *pdev) > =20 > plda =3D &port->plda; > plda->dev =3D dev; > + plda->num_events =3D NUM_EVENTS; > + plda->ops =3D &plda_ops; > =20 > port->axi_base_addr =3D devm_platform_ioremap_resource(pdev, 1); > if (IS_ERR(port->axi_base_addr)) > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/contro= ller/plda/pcie-plda.h > index 315d9874b899..ef33c1365b3e 100644 > --- a/drivers/pci/controller/plda/pcie-plda.h > +++ b/drivers/pci/controller/plda/pcie-plda.h > @@ -104,6 +104,12 @@ > =20 > #define PM_MSI_TO_MASK_OFFSET 19 > =20 > +struct plda_pcie_rp; > + > +struct plda_pcie_ops { > + u32 (*get_events)(struct plda_pcie_rp *pcie); > +}; > + > struct plda_msi { > struct mutex lock; /* Protect used bitmap */ > struct irq_domain *msi_domain; > @@ -119,10 +125,18 @@ struct plda_pcie_rp { > struct irq_domain *event_domain; > raw_spinlock_t lock; > struct plda_msi msi; > + const struct plda_pcie_ops *ops; > void __iomem *bridge_addr; > int num_events; > }; > =20 > +struct plda_evt { > + const struct irq_domain_ops *domain_ops; > + int (*request_evt_irq)(struct plda_pcie_rp *pcie, int evt_irq, int even= t); > + int intx_evt; > + int msi_evt; > +}; > + > void plda_handle_msi(struct irq_desc *desc); > int plda_allocate_msi_domains(struct plda_pcie_rp *port); > irqreturn_t plda_event_handler(int irq, void *dev_id); > --=20 > 2.17.1 >=20 --XmI2SV5XQAzfiIRv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZOiW0wAKCRB4tDGHoIJi 0usKAQCoutHEHYtKJN5yeC2bNyPV59aXcleQ9m/tV7cP6smBKAEA3aA/0doWKu6f aT6FsgdV1w4pDO/39o2kuq9ou0X8AQE= =dHsN -----END PGP SIGNATURE----- --XmI2SV5XQAzfiIRv-- --===============6038154554904878791== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv --===============6038154554904878791==--