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 11AF7C88CB2 for ; Fri, 25 Aug 2023 11:58:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S244462AbjHYL6A (ORCPT ); Fri, 25 Aug 2023 07:58:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60892 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244535AbjHYL5r (ORCPT ); Fri, 25 Aug 2023 07:57:47 -0400 Received: from esa.microchip.iphmx.com (esa.microchip.iphmx.com [68.232.154.123]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8FEB410F3; Fri, 25 Aug 2023 04:57:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1692964665; x=1724500665; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=PyJ/YRwYJz7mq3U4+yTh/5HPypy5nwmdt+KOBtbkj+c=; b=aBSUdX3R3ouyFobB8ivXPRLOljAU9SkRNTkuD6WEN1KuPbayM/CUYOQQ /S5LEg+kobraJ+NxDTtzYOK+YxoujrJUiysFAo8VEkqE9plsMXZ4RMuet CfonDIPWgWTylE02KdlClIhenmHjAk838CVUR1063TvNpZUKD+NA4jqBK qZVCxAYWTxdXsmDKDO+q94pabEsxyiRoH7/xt4KA/r+ETDhx8TRXOpV8K 1WgJxOvOmBKjH4niHyDLnT6Nq7t4ZvvABIFQsSA9jfq1UkMJZNVsSjA4P JSeOcG4j36GlwxAFVyBs0NIhbP69ERl8vaffz8pQ5HJ2kB3zP3qdJuNZz 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 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="XmI2SV5XQAzfiIRv" Content-Disposition: inline In-Reply-To: <20230825090129.65721-8-minda.chen@starfivetech.com> Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org --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--