From: Conor Dooley <conor@kernel.org>
To: daire.mcnamara@microchip.com
Cc: conor.dooley@microchip.com, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, paul.walmsley@sifive.com,
palmer@dabbelt.com, aou@eecs.berkeley.edu, lpieralisi@kernel.org,
kw@linux.com, bhelgaas@google.com,
linux-riscv@lists.infradead.org, devicetree@vger.kernel.org,
linux-pci@vger.kernel.org
Subject: Re: [PATCH v1 4/9] PCI: microchip: Clean up initialisation of interrupts
Date: Wed, 23 Nov 2022 21:58:26 +0000 [thread overview]
Message-ID: <Y36Xgr7NobqA2BCh@spud> (raw)
In-Reply-To: <20221116135504.258687-5-daire.mcnamara@microchip.com>
On Wed, Nov 16, 2022 at 01:54:59PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
>
> Refactor interrupt handling in _init() function into
> disable_interrupts(), init_interrupts(), clear_sec_errors() and clear
> ded_errors(). It was unwieldy and prone to bugs. Then clearly disable
> interrupts as soon as possible and only enable interrupts after address
> translation errors are setup to prevent spurious axi2pcie and pcie2axi
^^^^^^
Is that meant to read "after address translation is" or "after address
translation handling is"?
> translation errors being reported
>
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> drivers/pci/controller/pcie-microchip-host.c | 148 ++++++++++++-------
> 1 file changed, 92 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index a81e6d25e347..ecd4d3f3e3d4 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -986,39 +986,65 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
> return 0;
> }
>
> -static int mc_platform_init(struct pci_config_window *cfg)
> +static inline void mc_clear_secs(struct mc_pcie *port)
> {
> - struct device *dev = cfg->parent;
> - struct platform_device *pdev = to_platform_device(dev);
> - struct mc_pcie *port;
> - void __iomem *bridge_base_addr;
> - void __iomem *ctrl_base_addr;
> - int ret;
> - int irq;
> - int i, intx_irq, msi_irq, event_irq;
> + void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> +
> + writel_relaxed(GENMASK(15, 0), ctrl_base_addr + SEC_ERROR_INT);
I think it'd be nice if the GENMASK()s in this function and below were
#defined above somewhere.
> + writel_relaxed(0, ctrl_base_addr + SEC_ERROR_EVENT_CNT);
> +}
> +
> +static inline void mc_clear_deds(struct mc_pcie *port)
> +{
> + void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> +
> + writel_relaxed(GENMASK(15, 0), ctrl_base_addr + DED_ERROR_INT);
> + writel_relaxed(0, ctrl_base_addr + DED_ERROR_EVENT_CNT);
> +}
> +
> +static void mc_disable_interrupts(struct mc_pcie *port)
> +{
> + void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> + void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> u32 val;
> - int err;
>
> - port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> - if (!port)
> - return -ENOMEM;
> - port->dev = dev;
> + /* ensure ecc bypass is enabled */
> + val = ECC_CONTROL_TX_RAM_ECC_BYPASS | ECC_CONTROL_RX_RAM_ECC_BYPASS |
> + ECC_CONTROL_PCIE2AXI_RAM_ECC_BYPASS | ECC_CONTROL_AXI2PCIE_RAM_ECC_BYPASS;
Pedantic maybe, but could we format this as:
ECC_CONTROL_TX_RAM_ECC_BYPASS |
ECC_CONTROL_RX_RAM_ECC_BYPASS |
ECC_CONTROL_PCIE2AXI_RAM_ECC_BYPASS |
ECC_CONTROL_AXI2PCIE_RAM_ECC_BYPASS;
And the same below for the PCIE_EVENT_FOO stuff, I think it'd make
things a bit easier on the eye.
Anyways, SEC and DED stuff that I usually see on startup are gone - at
least on the setup I have :)
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> + writel_relaxed(val, ctrl_base_addr + ECC_CONTROL);
>
> - ret = mc_pcie_init_clks(dev);
> - if (ret) {
> - dev_err(dev, "failed to get clock resources, error %d\n", ret);
> - return -ENODEV;
> - }
> + /* disable sec errors and clear any outstanding */
> + writel_relaxed(GENMASK(15, 0), ctrl_base_addr + SEC_ERROR_INT_MASK);
> + mc_clear_secs(port);
>
> - port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
> - if (IS_ERR(port->axi_base_addr))
> - return PTR_ERR(port->axi_base_addr);
> + /* disable ded errors and clear any outstanding */
> + writel_relaxed(GENMASK(15, 0), ctrl_base_addr + DED_ERROR_INT_MASK);
> + mc_clear_deds(port);
>
> - bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> - ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> + /* disable local interrupts and clear any outstanding */
> + writel_relaxed(0, bridge_base_addr + IMASK_LOCAL);
> + writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_LOCAL);
> + writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_MSI);
> +
> + /* disable PCIe events and clear any outstanding */
> + val = PCIE_EVENT_INT_L2_EXIT_INT | PCIE_EVENT_INT_HOTRST_EXIT_INT |
> + PCIE_EVENT_INT_DLUP_EXIT_INT | PCIE_EVENT_INT_L2_EXIT_INT_MASK |
> + PCIE_EVENT_INT_HOTRST_EXIT_INT_MASK |
> + PCIE_EVENT_INT_DLUP_EXIT_INT_MASK;
> + writel_relaxed(val, ctrl_base_addr + PCIE_EVENT_INT);
> +
> + /* disable host interrupts and clear any outstanding */
> + writel_relaxed(0, bridge_base_addr + IMASK_HOST);
> + writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST);
> +}
> +
> +static int mc_init_interrupts(struct platform_device *pdev, struct mc_pcie *port)
> +{
> + struct device *dev = &pdev->dev;
> + int irq;
> + int i, intx_irq, msi_irq, event_irq;
> + int ret;
>
> - port->msi.vector_phy = MSI_ADDR;
> - port->msi.num_vectors = MC_NUM_MSI_IRQS;
> ret = mc_pcie_init_irq_domains(port);
> if (ret) {
> dev_err(dev, "failed creating IRQ domains\n");
> @@ -1036,11 +1062,11 @@ static int mc_platform_init(struct pci_config_window *cfg)
> return -ENXIO;
> }
>
> - err = devm_request_irq(dev, event_irq, mc_event_handler,
> + ret = devm_request_irq(dev, event_irq, mc_event_handler,
> 0, event_cause[i].sym, port);
> - if (err) {
> + if (ret) {
> dev_err(dev, "failed to request IRQ %d\n", event_irq);
> - return err;
> + return ret;
> }
> }
>
> @@ -1065,44 +1091,54 @@ static int mc_platform_init(struct pci_config_window *cfg)
> /* Plug the main event chained handler */
> irq_set_chained_handler_and_data(irq, mc_handle_event, port);
>
> - /* Hardware doesn't setup MSI by default */
> - mc_pcie_enable_msi(port, cfg->win);
> + return 0;
> +}
>
> - val = readl_relaxed(bridge_base_addr + IMASK_LOCAL);
> - val |= PM_MSI_INT_INTX_MASK;
> - writel_relaxed(val, bridge_base_addr + IMASK_LOCAL);
> +static int mc_platform_init(struct pci_config_window *cfg)
> +{
> + struct device *dev = cfg->parent;
> + struct platform_device *pdev = to_platform_device(dev);
> + struct mc_pcie *port;
> + void __iomem *bridge_base_addr;
> + void __iomem *ctrl_base_addr;
> + int ret;
>
> - writel_relaxed(val, ctrl_base_addr + ECC_CONTROL);
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> + port->dev = dev;
>
> - val = PCIE_EVENT_INT_L2_EXIT_INT |
> - PCIE_EVENT_INT_HOTRST_EXIT_INT |
> - PCIE_EVENT_INT_DLUP_EXIT_INT;
> - writel_relaxed(val, ctrl_base_addr + PCIE_EVENT_INT);
> + ret = mc_pcie_init_clks(dev);
> + if (ret) {
> + dev_err(dev, "failed to get clock resources, error %d\n", ret);
> + return -ENODEV;
> + }
>
> - val = SEC_ERROR_INT_TX_RAM_SEC_ERR_INT |
> - SEC_ERROR_INT_RX_RAM_SEC_ERR_INT |
> - SEC_ERROR_INT_PCIE2AXI_RAM_SEC_ERR_INT |
> - SEC_ERROR_INT_AXI2PCIE_RAM_SEC_ERR_INT;
> - writel_relaxed(val, ctrl_base_addr + SEC_ERROR_INT);
> - writel_relaxed(0, ctrl_base_addr + SEC_ERROR_INT_MASK);
> - writel_relaxed(0, ctrl_base_addr + SEC_ERROR_EVENT_CNT);
> + port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
> + if (IS_ERR(port->axi_base_addr))
> + return PTR_ERR(port->axi_base_addr);
>
> - val = DED_ERROR_INT_TX_RAM_DED_ERR_INT |
> - DED_ERROR_INT_RX_RAM_DED_ERR_INT |
> - DED_ERROR_INT_PCIE2AXI_RAM_DED_ERR_INT |
> - DED_ERROR_INT_AXI2PCIE_RAM_DED_ERR_INT;
> - writel_relaxed(val, ctrl_base_addr + DED_ERROR_INT);
> - writel_relaxed(0, ctrl_base_addr + DED_ERROR_INT_MASK);
> - writel_relaxed(0, ctrl_base_addr + DED_ERROR_EVENT_CNT);
> + mc_disable_interrupts(port);
>
> - writel_relaxed(0, bridge_base_addr + IMASK_HOST);
> - writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST);
> + bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> + ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> +
> + port->msi.vector_phy = MSI_ADDR;
> + port->msi.num_vectors = MC_NUM_MSI_IRQS;
> +
> + /* Hardware doesn't setup MSI by default */
> + mc_pcie_enable_msi(port, cfg->win);
>
> /* Configure Address Translation Table 0 for PCIe config space */
> mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start & 0xffffffff,
> cfg->res.start, resource_size(&cfg->res));
>
> - return mc_pcie_setup_windows(pdev, port);
> + ret = mc_pcie_setup_windows(pdev, port);
> + if (ret)
> + return ret;
> +
> + /* address translation is up; safe to enable interrupts */
> + return mc_init_interrupts(pdev, port);
> }
>
> static const struct pci_ecam_ops mc_ecam_ops = {
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2022-11-23 21:58 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-16 13:54 [PATCH v1 0/9] PCI: microchip: Partition address translations daire.mcnamara
2022-11-16 13:54 ` [PATCH v1 1/9] PCI: microchip: Align register, offset, and mask names with hw docs daire.mcnamara
2022-11-23 21:09 ` Conor Dooley
2022-11-16 13:54 ` [PATCH v1 2/9] PCI: microchip: Correct the DED and SEC interrupt bit offsets daire.mcnamara
2022-11-16 15:19 ` Conor Dooley
2022-11-23 21:28 ` Conor Dooley
2022-11-16 13:54 ` [PATCH v1 3/9] PCI: microchip: Enable event handlers to access bridge and ctrl ptrs daire.mcnamara
2022-11-23 21:34 ` Conor Dooley
2022-11-16 13:54 ` [PATCH v1 4/9] PCI: microchip: Clean up initialisation of interrupts daire.mcnamara
2022-11-23 21:58 ` Conor Dooley [this message]
2022-11-16 13:55 ` [PATCH v1 5/9] PCI: microchip: Gather MSI information from hardware config registers daire.mcnamara
2022-11-16 16:41 ` Bjorn Helgaas
2022-11-23 22:09 ` Conor Dooley
2022-11-16 13:55 ` [PATCH v1 6/9] PCI: microchip: Re-partition code between probe() and init() daire.mcnamara
2022-11-23 22:39 ` Conor Dooley
2022-11-16 13:55 ` [PATCH v1 7/9] PCI: microchip: Partition outbound address translation daire.mcnamara
2022-11-23 22:44 ` Conor Dooley
2022-11-16 13:55 ` [PATCH v1 8/9] PCI: microchip: Partition inbound " daire.mcnamara
2022-11-16 16:49 ` Bjorn Helgaas
2022-11-16 17:01 ` Conor Dooley
2022-11-23 23:05 ` Conor Dooley
2022-11-16 13:55 ` [PATCH v1 9/9] riscv: dts: microchip: add parent ranges and dma-ranges for IKRD v2022.09 daire.mcnamara
2022-11-23 22:14 ` Conor Dooley
2022-11-23 23:15 ` [PATCH v1 0/9] PCI: microchip: Partition address translations Conor Dooley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y36Xgr7NobqA2BCh@spud \
--to=conor@kernel.org \
--cc=aou@eecs.berkeley.edu \
--cc=bhelgaas@google.com \
--cc=conor.dooley@microchip.com \
--cc=daire.mcnamara@microchip.com \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=kw@linux.com \
--cc=linux-pci@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=lpieralisi@kernel.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).