Re: [PATCH 18/42] PCI: aardvark: Correctly clear and unmask all MSI interrupts

[Marc Zyngier]

On Thu, 06 May 2021 16:31:29 +0100,
Pali Rohár <pali@xxxxxxxxxx> wrote:
> 
> Define a new macro PCIE_MSI_ALL_MASK and use it for masking, unmasking and
> clearing all MSI interrupts.
> 
> Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> Reviewed-by: Marek Behún <kabel@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>  drivers/pci/controller/pci-aardvark.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 498810c00b6d..5e0243b2c473 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -117,6 +117,7 @@
>  #define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
>  #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
>  #define PCIE_MSI_MASK_REG			(CONTROL_BASE_ADDR + 0x5C)
> +#define     PCIE_MSI_ALL_MASK			GENMASK(31, 0)
>  #define PCIE_MSI_PAYLOAD_REG			(CONTROL_BASE_ADDR + 0x9C)
>  #define     PCIE_MSI_DATA_MASK			GENMASK(15, 0)
>  
> @@ -386,19 +387,22 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
>  
>  	/* Clear all interrupts */
> +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_STATUS_REG);
>  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_REG);
>  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
>  	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
>  
>  	/* Disable All ISR0/1 Sources */
> -	reg = PCIE_ISR0_ALL_MASK;
> -	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
> -	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
> -
> +	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
>  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
>  
>  	/* Unmask all MSIs */
> -	advk_writel(pcie, 0, PCIE_MSI_MASK_REG);
> +	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);

I really wonder why you'd unmask all MSIs. Yes, the current code does
that already, but I'd expect MSIs to be individually unmasked as they
get enabled by the core code.

Thanks,

	M.
	
> +
> +	/* Unmask summary MSI interrupt */
> +	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
> +	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
> +	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
>  
>  	/* Enable summary interrupt for GIC SPI source */
>  	reg = PCIE_IRQ_ALL_MASK & (~PCIE_IRQ_ENABLE_INTS_MASK);
> @@ -1049,7 +1053,7 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
>  
>  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
>  	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> -	msi_status = msi_val & ~msi_mask;
> +	msi_status = msi_val & ((~msi_mask) & PCIE_MSI_ALL_MASK);
>  
>  	for (msi_idx = 0; msi_idx < MSI_IRQ_NUM; msi_idx++) {
>  		if (!(BIT(msi_idx) & msi_status))
> -- 
> 2.20.1
> 
> 

-- 
Without deviation from the norm, progress is not possible.