diff mbox series

[v1,4/9] PCI: microchip: Clean up initialisation of interrupts

Message ID 20221116135504.258687-5-daire.mcnamara@microchip.com (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: microchip: Partition address translations | expand

Commit Message

Daire McNamara Nov. 16, 2022, 1:54 p.m. UTC 
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
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(-)

Comments

Conor Dooley Nov. 23, 2022, 9:58 p.m. UTC | #1 
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
diff mbox series

Patch

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);
+	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;
+	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 = {