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

[Conor Dooley <conor@kernel.org>]

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