diff mbox series

[v5,2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER

Message ID 1592469493-1549-3-git-send-email-hayashi.kunihiko@socionext.com
State New
Headers show
Series PCI: uniphier: Add features for UniPhier PCIe host controller | expand

Commit Message

Kunihiko Hayashi June 18, 2020, 8:38 a.m. UTC 
The misc interrupts consisting of PME, AER, and Link event, is handled
by INTx handler, however, these interrupts should be also handled by
MSI handler.

This adds the function uniphier_pcie_misc_isr() that handles misc
interrupts, which is called from both INTx and MSI handlers.
This function detects PME and AER interrupts with the status register,
and invoke PME and AER drivers related to MSI.

And this sets the mask for misc interrupts from INTx if MSI is enabled
and sets the mask for misc interrupts from MSI if MSI is disabled.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 57 ++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 11 deletions(-)

Comments

Marc Zyngier June 27, 2020, 9:48 a.m. UTC | #1 
On Thu, 18 Jun 2020 09:38:09 +0100,
Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote:
> 
> The misc interrupts consisting of PME, AER, and Link event, is handled
> by INTx handler, however, these interrupts should be also handled by
> MSI handler.
> 
> This adds the function uniphier_pcie_misc_isr() that handles misc
> interrupts, which is called from both INTx and MSI handlers.
> This function detects PME and AER interrupts with the status register,
> and invoke PME and AER drivers related to MSI.
> 
> And this sets the mask for misc interrupts from INTx if MSI is enabled
> and sets the mask for misc interrupts from MSI if MSI is disabled.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/pci/controller/dwc/pcie-uniphier.c | 57 ++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> index a5401a0..5ce2479 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> @@ -44,7 +44,9 @@
>  #define PCL_SYS_AUX_PWR_DET		BIT(8)
>  
>  #define PCL_RCV_INT			0x8108
> +#define PCL_RCV_INT_ALL_INT_MASK	GENMASK(28, 25)
>  #define PCL_RCV_INT_ALL_ENABLE		GENMASK(20, 17)
> +#define PCL_RCV_INT_ALL_MSI_MASK	GENMASK(12, 9)
>  #define PCL_CFG_BW_MGT_STATUS		BIT(4)
>  #define PCL_CFG_LINK_AUTO_BW_STATUS	BIT(3)
>  #define PCL_CFG_AER_RC_ERR_MSI_STATUS	BIT(2)
> @@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci)
>  
>  static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
>  {
> -	writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
> +	u32 val;
> +
> +	val = PCL_RCV_INT_ALL_ENABLE;
> +	if (pci_msi_enabled())
> +		val |= PCL_RCV_INT_ALL_INT_MASK;
> +	else
> +		val |= PCL_RCV_INT_ALL_MSI_MASK;

Does this affect endpoints? Or just the RC itself?

> +
> +	writel(val, priv->base + PCL_RCV_INT);
>  	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>  }
>  
> @@ -231,32 +241,56 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = {
>  	.map = uniphier_pcie_intx_map,
>  };
>  
> -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
> +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
>  {
> -	struct pcie_port *pp = irq_desc_get_handler_data(desc);
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> -	struct irq_chip *chip = irq_desc_get_chip(desc);
> -	unsigned long reg;
> -	u32 val, bit, virq;
> +	u32 val, virq;
>  
> -	/* INT for debug */
>  	val = readl(priv->base + PCL_RCV_INT);
>  
>  	if (val & PCL_CFG_BW_MGT_STATUS)
>  		dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
> +
>  	if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>  		dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
> -	if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
> -		dev_dbg(pci->dev, "Root Error\n");
> -	if (val & PCL_CFG_PME_MSI_STATUS)
> -		dev_dbg(pci->dev, "PME Interrupt\n");
> +
> +	if (is_msi) {
> +		if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
> +			dev_dbg(pci->dev, "Root Error Status\n");
> +
> +		if (val & PCL_CFG_PME_MSI_STATUS)
> +			dev_dbg(pci->dev, "PME Interrupt\n");
> +
> +		if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
> +			   PCL_CFG_PME_MSI_STATUS)) {
> +			virq = irq_linear_revmap(pp->irq_domain, 0);
> +			generic_handle_irq(virq);
> +		}
> +	}

Please have two handlers: one for interrupts that are from the RC,
another for interrupts coming from the endpoints.

	M.
Kunihiko Hayashi June 29, 2020, 9:49 a.m. UTC | #2 
Hi Marc,

On 2020/06/27 18:48, Marc Zyngier wrote:
> On Thu, 18 Jun 2020 09:38:09 +0100,
> Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote:
>>
>> The misc interrupts consisting of PME, AER, and Link event, is handled
>> by INTx handler, however, these interrupts should be also handled by
>> MSI handler.
>>
>> This adds the function uniphier_pcie_misc_isr() that handles misc
>> interrupts, which is called from both INTx and MSI handlers.
>> This function detects PME and AER interrupts with the status register,
>> and invoke PME and AER drivers related to MSI.
>>
>> And this sets the mask for misc interrupts from INTx if MSI is enabled
>> and sets the mask for misc interrupts from MSI if MSI is disabled.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Jingoo Han <jingoohan1@gmail.com>
>> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-uniphier.c | 57 ++++++++++++++++++++++++------
>>   1 file changed, 46 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
>> index a5401a0..5ce2479 100644
>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>> @@ -44,7 +44,9 @@
>>   #define PCL_SYS_AUX_PWR_DET		BIT(8)
>>   
>>   #define PCL_RCV_INT			0x8108
>> +#define PCL_RCV_INT_ALL_INT_MASK	GENMASK(28, 25)
>>   #define PCL_RCV_INT_ALL_ENABLE		GENMASK(20, 17)
>> +#define PCL_RCV_INT_ALL_MSI_MASK	GENMASK(12, 9)
>>   #define PCL_CFG_BW_MGT_STATUS		BIT(4)
>>   #define PCL_CFG_LINK_AUTO_BW_STATUS	BIT(3)
>>   #define PCL_CFG_AER_RC_ERR_MSI_STATUS	BIT(2)
>> @@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci)
>>   
>>   static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
>>   {
>> -	writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
>> +	u32 val;
>> +
>> +	val = PCL_RCV_INT_ALL_ENABLE;
>> +	if (pci_msi_enabled())
>> +		val |= PCL_RCV_INT_ALL_INT_MASK;
>> +	else
>> +		val |= PCL_RCV_INT_ALL_MSI_MASK;
> 
> Does this affect endpoints? Or just the RC itself?

These interrupts are asserted by RC itself, so this part affects only RC.

>> +
>> +	writel(val, priv->base + PCL_RCV_INT);
>>   	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>>   }
>>   
>> @@ -231,32 +241,56 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = {
>>   	.map = uniphier_pcie_intx_map,
>>   };
>>   
>> -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
>>   {
>> -	struct pcie_port *pp = irq_desc_get_handler_data(desc);
>>   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>   	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
>> -	struct irq_chip *chip = irq_desc_get_chip(desc);
>> -	unsigned long reg;
>> -	u32 val, bit, virq;
>> +	u32 val, virq;
>>   
>> -	/* INT for debug */
>>   	val = readl(priv->base + PCL_RCV_INT);
>>   
>>   	if (val & PCL_CFG_BW_MGT_STATUS)
>>   		dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
>> +
>>   	if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>>   		dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
>> -	if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>> -		dev_dbg(pci->dev, "Root Error\n");
>> -	if (val & PCL_CFG_PME_MSI_STATUS)
>> -		dev_dbg(pci->dev, "PME Interrupt\n");
>> +
>> +	if (is_msi) {
>> +		if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>> +			dev_dbg(pci->dev, "Root Error Status\n");
>> +
>> +		if (val & PCL_CFG_PME_MSI_STATUS)
>> +			dev_dbg(pci->dev, "PME Interrupt\n");
>> +
>> +		if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
>> +			   PCL_CFG_PME_MSI_STATUS)) {
>> +			virq = irq_linear_revmap(pp->irq_domain, 0);
>> +			generic_handle_irq(virq);
>> +		}
>> +	}
> 
> Please have two handlers: one for interrupts that are from the RC,
> another for interrupts coming from the endpoints.
I assume that this handler treats interrupts from the RC only and
this is set on the member ".msi_host_isr" added in the patch 1/6.
I think that the handler for interrupts coming from endpoints should be
treated as a normal case (after calling .msi_host_isr in
dw_handle_msi_irq()).

Thank you,

---
Best Regards
Kunihiko Hayashi
Marc Zyngier June 30, 2020, 1:23 p.m. UTC | #3 
On 2020-06-29 10:49, Kunihiko Hayashi wrote:
> Hi Marc,
> 
> On 2020/06/27 18:48, Marc Zyngier wrote:
>> On Thu, 18 Jun 2020 09:38:09 +0100,
>> Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote:
>>> 
>>> The misc interrupts consisting of PME, AER, and Link event, is 
>>> handled
>>> by INTx handler, however, these interrupts should be also handled by
>>> MSI handler.
>>> 
>>> This adds the function uniphier_pcie_misc_isr() that handles misc
>>> interrupts, which is called from both INTx and MSI handlers.
>>> This function detects PME and AER interrupts with the status 
>>> register,
>>> and invoke PME and AER drivers related to MSI.
>>> 
>>> And this sets the mask for misc interrupts from INTx if MSI is 
>>> enabled
>>> and sets the mask for misc interrupts from MSI if MSI is disabled.
>>> 
>>> Cc: Marc Zyngier <maz@kernel.org>
>>> Cc: Jingoo Han <jingoohan1@gmail.com>
>>> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>>> ---
>>>   drivers/pci/controller/dwc/pcie-uniphier.c | 57 
>>> ++++++++++++++++++++++++------
>>>   1 file changed, 46 insertions(+), 11 deletions(-)
>>> 
>>> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c 
>>> b/drivers/pci/controller/dwc/pcie-uniphier.c
>>> index a5401a0..5ce2479 100644
>>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>>> @@ -44,7 +44,9 @@
>>>   #define PCL_SYS_AUX_PWR_DET		BIT(8)
>>>     #define PCL_RCV_INT			0x8108
>>> +#define PCL_RCV_INT_ALL_INT_MASK	GENMASK(28, 25)
>>>   #define PCL_RCV_INT_ALL_ENABLE		GENMASK(20, 17)
>>> +#define PCL_RCV_INT_ALL_MSI_MASK	GENMASK(12, 9)
>>>   #define PCL_CFG_BW_MGT_STATUS		BIT(4)
>>>   #define PCL_CFG_LINK_AUTO_BW_STATUS	BIT(3)
>>>   #define PCL_CFG_AER_RC_ERR_MSI_STATUS	BIT(2)
>>> @@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct 
>>> dw_pcie *pci)
>>>     static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv 
>>> *priv)
>>>   {
>>> -	writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
>>> +	u32 val;
>>> +
>>> +	val = PCL_RCV_INT_ALL_ENABLE;
>>> +	if (pci_msi_enabled())
>>> +		val |= PCL_RCV_INT_ALL_INT_MASK;
>>> +	else
>>> +		val |= PCL_RCV_INT_ALL_MSI_MASK;
>> 
>> Does this affect endpoints? Or just the RC itself?
> 
> These interrupts are asserted by RC itself, so this part affects only 
> RC.
> 
>>> +
>>> +	writel(val, priv->base + PCL_RCV_INT);
>>>   	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>>>   }
>>>   @@ -231,32 +241,56 @@ static const struct irq_domain_ops 
>>> uniphier_intx_domain_ops = {
>>>   	.map = uniphier_pcie_intx_map,
>>>   };
>>>   -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool 
>>> is_msi)
>>>   {
>>> -	struct pcie_port *pp = irq_desc_get_handler_data(desc);
>>>   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>   	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
>>> -	struct irq_chip *chip = irq_desc_get_chip(desc);
>>> -	unsigned long reg;
>>> -	u32 val, bit, virq;
>>> +	u32 val, virq;
>>>   -	/* INT for debug */
>>>   	val = readl(priv->base + PCL_RCV_INT);
>>>     	if (val & PCL_CFG_BW_MGT_STATUS)
>>>   		dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
>>> +
>>>   	if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>>>   		dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
>>> -	if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>> -		dev_dbg(pci->dev, "Root Error\n");
>>> -	if (val & PCL_CFG_PME_MSI_STATUS)
>>> -		dev_dbg(pci->dev, "PME Interrupt\n");
>>> +
>>> +	if (is_msi) {
>>> +		if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>> +			dev_dbg(pci->dev, "Root Error Status\n");
>>> +
>>> +		if (val & PCL_CFG_PME_MSI_STATUS)
>>> +			dev_dbg(pci->dev, "PME Interrupt\n");
>>> +
>>> +		if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
>>> +			   PCL_CFG_PME_MSI_STATUS)) {
>>> +			virq = irq_linear_revmap(pp->irq_domain, 0);
>>> +			generic_handle_irq(virq);
>>> +		}
>>> +	}
>> 
>> Please have two handlers: one for interrupts that are from the RC,
>> another for interrupts coming from the endpoints.
> I assume that this handler treats interrupts from the RC only and
> this is set on the member ".msi_host_isr" added in the patch 1/6.
> I think that the handler for interrupts coming from endpoints should be
> treated as a normal case (after calling .msi_host_isr in
> dw_handle_msi_irq()).

It looks pretty odd that you end-up dealing with both from the
same "parent" interrupt. I guess this is in keeping with the
rest of the DW PCIe hacks... :-/

It is for Lorenzo to make up his mind about this anyway.

Thanks,

         M.
Kunihiko Hayashi July 1, 2020, 2:18 a.m. UTC | #4 
Hi Marc,

On 2020/06/30 22:23, Marc Zyngier wrote:
> On 2020-06-29 10:49, Kunihiko Hayashi wrote:
>> Hi Marc,
>>
>> On 2020/06/27 18:48, Marc Zyngier wrote:
>>> On Thu, 18 Jun 2020 09:38:09 +0100,
>>> Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote:
>>>>
>>>> The misc interrupts consisting of PME, AER, and Link event, is handled
>>>> by INTx handler, however, these interrupts should be also handled by
>>>> MSI handler.
>>>>
>>>> This adds the function uniphier_pcie_misc_isr() that handles misc
>>>> interrupts, which is called from both INTx and MSI handlers.
>>>> This function detects PME and AER interrupts with the status register,
>>>> and invoke PME and AER drivers related to MSI.
>>>>
>>>> And this sets the mask for misc interrupts from INTx if MSI is enabled
>>>> and sets the mask for misc interrupts from MSI if MSI is disabled.
>>>>
>>>> Cc: Marc Zyngier <maz@kernel.org>
>>>> Cc: Jingoo Han <jingoohan1@gmail.com>
>>>> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>>>> ---
>>>>   drivers/pci/controller/dwc/pcie-uniphier.c | 57 ++++++++++++++++++++++++------
>>>>   1 file changed, 46 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> index a5401a0..5ce2479 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> @@ -44,7 +44,9 @@
>>>>   #define PCL_SYS_AUX_PWR_DET        BIT(8)
>>>>     #define PCL_RCV_INT            0x8108
>>>> +#define PCL_RCV_INT_ALL_INT_MASK    GENMASK(28, 25)
>>>>   #define PCL_RCV_INT_ALL_ENABLE        GENMASK(20, 17)
>>>> +#define PCL_RCV_INT_ALL_MSI_MASK    GENMASK(12, 9)
>>>>   #define PCL_CFG_BW_MGT_STATUS        BIT(4)
>>>>   #define PCL_CFG_LINK_AUTO_BW_STATUS    BIT(3)
>>>>   #define PCL_CFG_AER_RC_ERR_MSI_STATUS    BIT(2)
>>>> @@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci)
>>>>     static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
>>>>   {
>>>> -    writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
>>>> +    u32 val;
>>>> +
>>>> +    val = PCL_RCV_INT_ALL_ENABLE;
>>>> +    if (pci_msi_enabled())
>>>> +        val |= PCL_RCV_INT_ALL_INT_MASK;
>>>> +    else
>>>> +        val |= PCL_RCV_INT_ALL_MSI_MASK;
>>>
>>> Does this affect endpoints? Or just the RC itself?
>>
>> These interrupts are asserted by RC itself, so this part affects only RC.
>>
>>>> +
>>>> +    writel(val, priv->base + PCL_RCV_INT);
>>>>       writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>>>>   }
>>>>   @@ -231,32 +241,56 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = {
>>>>       .map = uniphier_pcie_intx_map,
>>>>   };
>>>>   -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>>>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
>>>>   {
>>>> -    struct pcie_port *pp = irq_desc_get_handler_data(desc);
>>>>       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>>       struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
>>>> -    struct irq_chip *chip = irq_desc_get_chip(desc);
>>>> -    unsigned long reg;
>>>> -    u32 val, bit, virq;
>>>> +    u32 val, virq;
>>>>   -    /* INT for debug */
>>>>       val = readl(priv->base + PCL_RCV_INT);
>>>>         if (val & PCL_CFG_BW_MGT_STATUS)
>>>>           dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
>>>> +
>>>>       if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>>>>           dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
>>>> -    if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>>> -        dev_dbg(pci->dev, "Root Error\n");
>>>> -    if (val & PCL_CFG_PME_MSI_STATUS)
>>>> -        dev_dbg(pci->dev, "PME Interrupt\n");
>>>> +
>>>> +    if (is_msi) {
>>>> +        if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>>> +            dev_dbg(pci->dev, "Root Error Status\n");
>>>> +
>>>> +        if (val & PCL_CFG_PME_MSI_STATUS)
>>>> +            dev_dbg(pci->dev, "PME Interrupt\n");
>>>> +
>>>> +        if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
>>>> +               PCL_CFG_PME_MSI_STATUS)) {
>>>> +            virq = irq_linear_revmap(pp->irq_domain, 0);
>>>> +            generic_handle_irq(virq);
>>>> +        }
>>>> +    }
>>>
>>> Please have two handlers: one for interrupts that are from the RC,
>>> another for interrupts coming from the endpoints.
>> I assume that this handler treats interrupts from the RC only and
>> this is set on the member ".msi_host_isr" added in the patch 1/6.
>> I think that the handler for interrupts coming from endpoints should be
>> treated as a normal case (after calling .msi_host_isr in
>> dw_handle_msi_irq()).
> 
> It looks pretty odd that you end-up dealing with both from the
> same "parent" interrupt. I guess this is in keeping with the
> rest of the DW PCIe hacks... :-/

It might be odd, however, in case of UniPhier SoC,
both MSI interrupts from endpoints and PME/AER interrupts from RC are
asserted by same "parent" interrupt. In other words, PME/AER interrupts
are notified using the parent interrupt for MSI.

MSI interrupts are treated as child interrupts with reference to
the status register in DW core. This is handled in a for-loop in
dw_handle_msi_irq().

PME/AER interrupts are treated with reference to the status register
in UniPhier glue layer, however, this couldn't be handled in the same way
directly.

So I'm trying to add .msi_host_isr function to handle this
with reference to the SoC-specific registers.

This exported function asserts MSI-0 as a shared child interrupt.
As a result, PME/AER are registered like the followings in dmesg:

    pcieport 0000:00:00.0: PME: Signaling with IRQ 25
    pcieport 0000:00:00.0: AER: enabled with IRQ 25

And these interrupts are shared as MSI-0:

    # cat /proc/interrupts | grep 25:
     25:          0          0          0          0   PCI-MSI   0 Edge      PCIe PME, aerdrv

This might be a special case, though, I think that this is needed to handle
interrupts from RC sharing MSI parent.
  
> It is for Lorenzo to make up his mind about this anyway.

I'd like to Lorenzo's opinion, too.

Thank you,

---
Best Regards
Kunihiko Hayashi
Lorenzo Pieralisi July 10, 2020, 4:14 p.m. UTC | #5 
On Wed, Jul 01, 2020 at 11:18:29AM +0900, Kunihiko Hayashi wrote:

[...]

> > > > >   -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
> > > > > +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
> > > > >   {
> > > > > -    struct pcie_port *pp = irq_desc_get_handler_data(desc);
> > > > >       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > >       struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> > > > > -    struct irq_chip *chip = irq_desc_get_chip(desc);
> > > > > -    unsigned long reg;
> > > > > -    u32 val, bit, virq;
> > > > > +    u32 val, virq;
> > > > >   -    /* INT for debug */
> > > > >       val = readl(priv->base + PCL_RCV_INT);
> > > > >         if (val & PCL_CFG_BW_MGT_STATUS)
> > > > >           dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
> > > > > +
> > > > >       if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
> > > > >           dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
> > > > > -    if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
> > > > > -        dev_dbg(pci->dev, "Root Error\n");
> > > > > -    if (val & PCL_CFG_PME_MSI_STATUS)
> > > > > -        dev_dbg(pci->dev, "PME Interrupt\n");
> > > > > +
> > > > > +    if (is_msi) {
> > > > > +        if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
> > > > > +            dev_dbg(pci->dev, "Root Error Status\n");
> > > > > +
> > > > > +        if (val & PCL_CFG_PME_MSI_STATUS)
> > > > > +            dev_dbg(pci->dev, "PME Interrupt\n");
> > > > > +
> > > > > +        if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
> > > > > +               PCL_CFG_PME_MSI_STATUS)) {
> > > > > +            virq = irq_linear_revmap(pp->irq_domain, 0);
> > > > > +            generic_handle_irq(virq);
> > > > > +        }
> > > > > +    }
> > > > 
> > > > Please have two handlers: one for interrupts that are from the RC,
> > > > another for interrupts coming from the endpoints.
> > > I assume that this handler treats interrupts from the RC only and
> > > this is set on the member ".msi_host_isr" added in the patch 1/6.
> > > I think that the handler for interrupts coming from endpoints should be
> > > treated as a normal case (after calling .msi_host_isr in
> > > dw_handle_msi_irq()).
> > 
> > It looks pretty odd that you end-up dealing with both from the
> > same "parent" interrupt. I guess this is in keeping with the
> > rest of the DW PCIe hacks... :-/
> 
> It might be odd, however, in case of UniPhier SoC,
> both MSI interrupts from endpoints and PME/AER interrupts from RC are
> asserted by same "parent" interrupt. In other words, PME/AER interrupts
> are notified using the parent interrupt for MSI.
> 
> MSI interrupts are treated as child interrupts with reference to
> the status register in DW core. This is handled in a for-loop in
> dw_handle_msi_irq().
> 
> PME/AER interrupts are treated with reference to the status register
> in UniPhier glue layer, however, this couldn't be handled in the same way
> directly.
> 
> So I'm trying to add .msi_host_isr function to handle this
> with reference to the SoC-specific registers.
> 
> This exported function asserts MSI-0 as a shared child interrupt.
> As a result, PME/AER are registered like the followings in dmesg:
> 
>    pcieport 0000:00:00.0: PME: Signaling with IRQ 25
>    pcieport 0000:00:00.0: AER: enabled with IRQ 25
> 
> And these interrupts are shared as MSI-0:
> 
>    # cat /proc/interrupts | grep 25:
>     25:          0          0          0          0   PCI-MSI   0 Edge      PCIe PME, aerdrv
> 
> This might be a special case, though, I think that this is needed to handle
> interrupts from RC sharing MSI parent.

Can you please send me (with this series *applied* of course and if
possible with an endpoint MSI/MSI-X capable enabled):

- full dmesg log
- lspci -vv output
- cat /proc/interrupts

I need to understand how this system HW works before commenting any
further.

> > It is for Lorenzo to make up his mind about this anyway.
> 
> I'd like to Lorenzo's opinion, too.

I am trying to understand how the HW is wired up (and that's what Marc
asked as well) so first things first, please send the logs.

Lorenzo
Kunihiko Hayashi July 14, 2020, 9:27 a.m. UTC | #6 
Hi Lorenzo,

On 2020/07/11 1:14, Lorenzo Pieralisi wrote:
> On Wed, Jul 01, 2020 at 11:18:29AM +0900, Kunihiko Hayashi wrote:
> 
> [...]
> 
>>>>>>    -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>>>>>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
>>>>>>    {
>>>>>> -    struct pcie_port *pp = irq_desc_get_handler_data(desc);
>>>>>>        struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>>>>        struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
>>>>>> -    struct irq_chip *chip = irq_desc_get_chip(desc);
>>>>>> -    unsigned long reg;
>>>>>> -    u32 val, bit, virq;
>>>>>> +    u32 val, virq;
>>>>>>    -    /* INT for debug */
>>>>>>        val = readl(priv->base + PCL_RCV_INT);
>>>>>>          if (val & PCL_CFG_BW_MGT_STATUS)
>>>>>>            dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
>>>>>> +
>>>>>>        if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>>>>>>            dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
>>>>>> -    if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>>>>> -        dev_dbg(pci->dev, "Root Error\n");
>>>>>> -    if (val & PCL_CFG_PME_MSI_STATUS)
>>>>>> -        dev_dbg(pci->dev, "PME Interrupt\n");
>>>>>> +
>>>>>> +    if (is_msi) {
>>>>>> +        if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>>>>> +            dev_dbg(pci->dev, "Root Error Status\n");
>>>>>> +
>>>>>> +        if (val & PCL_CFG_PME_MSI_STATUS)
>>>>>> +            dev_dbg(pci->dev, "PME Interrupt\n");
>>>>>> +
>>>>>> +        if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
>>>>>> +               PCL_CFG_PME_MSI_STATUS)) {
>>>>>> +            virq = irq_linear_revmap(pp->irq_domain, 0);
>>>>>> +            generic_handle_irq(virq);
>>>>>> +        }
>>>>>> +    }
>>>>>
>>>>> Please have two handlers: one for interrupts that are from the RC,
>>>>> another for interrupts coming from the endpoints.
>>>> I assume that this handler treats interrupts from the RC only and
>>>> this is set on the member ".msi_host_isr" added in the patch 1/6.
>>>> I think that the handler for interrupts coming from endpoints should be
>>>> treated as a normal case (after calling .msi_host_isr in
>>>> dw_handle_msi_irq()).
>>>
>>> It looks pretty odd that you end-up dealing with both from the
>>> same "parent" interrupt. I guess this is in keeping with the
>>> rest of the DW PCIe hacks... :-/
>>
>> It might be odd, however, in case of UniPhier SoC,
>> both MSI interrupts from endpoints and PME/AER interrupts from RC are
>> asserted by same "parent" interrupt. In other words, PME/AER interrupts
>> are notified using the parent interrupt for MSI.
>>
>> MSI interrupts are treated as child interrupts with reference to
>> the status register in DW core. This is handled in a for-loop in
>> dw_handle_msi_irq().
>>
>> PME/AER interrupts are treated with reference to the status register
>> in UniPhier glue layer, however, this couldn't be handled in the same way
>> directly.
>>
>> So I'm trying to add .msi_host_isr function to handle this
>> with reference to the SoC-specific registers.
>>
>> This exported function asserts MSI-0 as a shared child interrupt.
>> As a result, PME/AER are registered like the followings in dmesg:
>>
>>     pcieport 0000:00:00.0: PME: Signaling with IRQ 25
>>     pcieport 0000:00:00.0: AER: enabled with IRQ 25
>>
>> And these interrupts are shared as MSI-0:
>>
>>     # cat /proc/interrupts | grep 25:
>>      25:          0          0          0          0   PCI-MSI   0 Edge      PCIe PME, aerdrv
>>
>> This might be a special case, though, I think that this is needed to handle
>> interrupts from RC sharing MSI parent.
> 
> Can you please send me (with this series *applied* of course and if
> possible with an endpoint MSI/MSI-X capable enabled):
> 
> - full dmesg log
> - lspci -vv output
> - cat /proc/interrupts
> 
> I need to understand how this system HW works before commenting any
> further.

Okay, I attach all the log to this mail.

This controller has MSI support only, and I attached R8169 ethernet card
to PCIe slot to enable the controller.

> 
>>> It is for Lorenzo to make up his mind about this anyway.
>>
>> I'd like to Lorenzo's opinion, too.
> 
> I am trying to understand how the HW is wired up (and that's what Marc
> asked as well) so first things first, please send the logs.

I hope this will help your understanding.

Thank you,

---
Best Regards
Kunihiko Hayashi
root@akebi96:~# dmesg
[    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd082]
[    0.000000] Linux version 5.8.0-rc4-next-20200710-00007-g868c2f1a3dad (hayashi@ubuntu) (aarch64-linux-gnu-gcc (Linaro GCC 7.5-2019.12) 7.5.0, GNU ld (Linaro_Binutils-2019.12) 2.28.2.20170706) #21 SMP PREEMPT Mon Jul 13 09:48:17 JST 2020
[    0.000000] Machine model: Akebi96
[    0.000000] efi: UEFI not found.
[    0.000000] cma: Reserved 32 MiB at 0x00000000fdc00000
[    0.000000] NUMA: No NUMA configuration found
[    0.000000] NUMA: Faking a node at [mem 0x0000000080000000-0x000000013fffffff]
[    0.000000] NUMA: NODE_DATA [mem 0x13f9f5340-0x13f9f723f]
[    0.000000] Zone ranges:
[    0.000000]   DMA      [mem 0x0000000080000000-0x00000000bfffffff]
[    0.000000]   DMA32    [mem 0x00000000c0000000-0x00000000ffffffff]
[    0.000000]   Normal   [mem 0x0000000100000000-0x000000013fffffff]
[    0.000000] Movable zone start for each node
[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x0000000080000000-0x0000000080ffffff]
[    0.000000]   node   0: [mem 0x0000000082000000-0x00000000bfffffff]
[    0.000000]   node   0: [mem 0x00000000c2000000-0x000000013fffffff]
[    0.000000] Initmem setup node 0 [mem 0x0000000080000000-0x000000013fffffff]
[    0.000000] On node 0 totalpages: 774144
[    0.000000]   DMA zone: 4096 pages used for memmap
[    0.000000]   DMA zone: 0 pages reserved
[    0.000000]   DMA zone: 258048 pages, LIFO batch:63
[    0.000000]   DMA32 zone: 4096 pages used for memmap
[    0.000000]   DMA32 zone: 253952 pages, LIFO batch:63
[    0.000000]   Normal zone: 4096 pages used for memmap
[    0.000000]   Normal zone: 262144 pages, LIFO batch:63
[    0.000000] psci: probing for conduit method from DT.
[    0.000000] psci: PSCIv1.1 detected in firmware.
[    0.000000] psci: Using standard PSCI v0.2 function IDs
[    0.000000] psci: Trusted OS migration not required
[    0.000000] psci: SMC Calling Convention v1.2
[    0.000000] percpu: Embedded 23 pages/cpu s54040 r8192 d31976 u94208
[    0.000000] pcpu-alloc: s54040 r8192 d31976 u94208 alloc=23*4096
[    0.000000] pcpu-alloc: [0] 0 [0] 1 [0] 2 [0] 3
[    0.000000] Detected PIPT I-cache on CPU0
[    0.000000] CPU features: detected: GIC system register CPU interface
[    0.000000] CPU features: detected: EL2 vector hardening
[    0.000000] Speculative Store Bypass Disable mitigation not required
[    0.000000] CPU features: detected: ARM errata 1165522, 1319367, or 1530923
[    0.000000] Built 1 zonelists, mobility grouping on.  Total pages: 761856
[    0.000000] Policy zone: Normal
[    0.000000] Kernel command line:
[    0.000000] Dentry cache hash table entries: 524288 (order: 10, 4194304 bytes, linear)
[    0.000000] Inode-cache hash table entries: 262144 (order: 9, 2097152 bytes, linear)
[    0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
[    0.000000] software IO TLB: mapped [mem 0xbbfff000-0xbffff000] (64MB)
[    0.000000] Memory: 2881608K/3096576K available (14076K kernel code, 2240K rwdata, 7572K rodata, 5696K init, 486K bss, 182200K reserved, 32768K cma-reserved)
[    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1
[    0.000000] rcu: Preemptible hierarchical RCU implementation.
[    0.000000] rcu:     RCU event tracing is enabled.
[    0.000000] rcu:     RCU restricting CPUs from NR_CPUS=256 to nr_cpu_ids=4.
[    0.000000]  Trampoline variant of Tasks RCU enabled.
[    0.000000] rcu: RCU calculated value of scheduler-enlistment delay is 25 jiffies.
[    0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=4
[    0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[    0.000000] GICv3: GIC: Using split EOI/Deactivate mode
[    0.000000] GICv3: 512 SPIs implemented
[    0.000000] GICv3: 0 Extended SPIs implemented
[    0.000000] GICv3: Distributor has no Range Selector support
[    0.000000] GICv3: 16 PPIs implemented
[    0.000000] GICv3: CPU0: found redistributor 0 region 0:0x000000005fe80000
[    0.000000] random: get_random_bytes called from start_kernel+0x314/0x4f0 with crng_init=0
[    0.000000] arch_timer: cp15 timer(s) running at 50.00MHz (phys).
[    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles: 0xb8812736b, max_idle_ns: 440795202655 ns
[    0.000003] sched_clock: 56 bits at 50MHz, resolution 20ns, wraps every 4398046511100ns
[    0.000358] Console: colour dummy device 80x25
[    0.000628] printk: console [tty0] enabled
[    0.000690] Calibrating delay loop (skipped), value calculated using timer frequency.. 100.00 BogoMIPS (lpj=200000)
[    0.000705] pid_max: default: 32768 minimum: 301
[    0.000758] LSM: Security Framework initializing
[    0.000846] Mount-cache hash table entries: 8192 (order: 4, 65536 bytes, linear)
[    0.000905] Mountpoint-cache hash table entries: 8192 (order: 4, 65536 bytes, linear)
[    0.002094] rcu: Hierarchical SRCU implementation.
[    0.003161] EFI services will not be available.
[    0.003357] smp: Bringing up secondary CPUs ...
[    0.028171] Detected PIPT I-cache on CPU1
[    0.028201] GICv3: CPU1: found redistributor 1 region 0:0x000000005fea0000
[    0.028228] CPU1: Booted secondary processor 0x0000000001 [0x410fd082]
[    0.040780] CPU features: detected: ARM erratum 845719
[    0.040794] Detected VIPT I-cache on CPU2
[    0.040821] GICv3: CPU2: found redistributor 100 region 0:0x000000005fec0000
[    0.040852] CPU2: Booted secondary processor 0x0000000100 [0x410fd034]
[    0.053434] Detected VIPT I-cache on CPU3
[    0.053453] GICv3: CPU3: found redistributor 101 region 0:0x000000005fee0000
[    0.053471] CPU3: Booted secondary processor 0x0000000101 [0x410fd034]
[    0.053567] smp: Brought up 1 node, 4 CPUs
[    0.053628] SMP: Total of 4 processors activated.
[    0.053636] CPU features: detected: 32-bit EL0 Support
[    0.053644] CPU features: detected: CRC32 instructions
[    0.053652] CPU features: detected: 32-bit EL1 Support
[    0.069374] CPU: All CPU(s) started at EL2
[    0.069409] alternatives: patching kernel code
[    0.070560] devtmpfs: initialized
[    0.074955] KASLR disabled due to lack of seed
[    0.075508] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 7645041785100000 ns
[    0.075541] futex hash table entries: 1024 (order: 4, 65536 bytes, linear)
[    0.076843] pinctrl core: initialized pinctrl subsystem
[    0.078133] thermal_sys: Registered thermal governor 'step_wise'
[    0.078136] thermal_sys: Registered thermal governor 'power_allocator'
[    0.078489] DMI not present or invalid.
[    0.079115] NET: Registered protocol family 16
[    0.080223] DMA: preallocated 512 KiB GFP_KERNEL pool for atomic allocations
[    0.080365] DMA: preallocated 512 KiB GFP_KERNEL|GFP_DMA pool for atomic allocations
[    0.080608] DMA: preallocated 512 KiB GFP_KERNEL|GFP_DMA32 pool for atomic allocations
[    0.080719] audit: initializing netlink subsys (disabled)
[    0.080920] audit: type=2000 audit(0.080:1): state=initialized audit_enabled=0 res=1
[    0.081732] cpuidle: using governor menu
[    0.081826] hw-breakpoint: found 6 breakpoint and 4 watchpoint registers.
[    0.081907] ASID allocator initialised with 65536 entries
[    0.083251] Serial: AMBA PL011 UART driver
[    0.106086] HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages
[    0.106112] HugeTLB registered 32.0 MiB page size, pre-allocated 0 pages
[    0.106126] HugeTLB registered 2.00 MiB page size, pre-allocated 0 pages
[    0.106140] HugeTLB registered 64.0 KiB page size, pre-allocated 0 pages
[    0.107261] cryptd: max_cpu_qlen set to 1000
[    0.109289] ACPI: Interpreter disabled.
[    0.110385] iommu: Default domain type: Translated
[    0.110624] vgaarb: loaded
[    0.110992] SCSI subsystem initialized
[    0.111140] libata version 3.00 loaded.
[    0.111382] usbcore: registered new interface driver usbfs
[    0.111433] usbcore: registered new interface driver hub
[    0.111502] usbcore: registered new device driver usb
[    0.112240] pps_core: LinuxPPS API ver. 1 registered
[    0.112252] pps_core: Software ver. 5.3.6 - Copyright 2005-2007 Rodolfo Giometti <giometti@linux.it>
[    0.112279] PTP clock support registered
[    0.112413] EDAC MC: Ver: 3.0.0
[    0.113709] FPGA manager framework
[    0.113811] Advanced Linux Sound Architecture Driver Initialized.
[    0.114454] clocksource: Switched to clocksource arch_sys_counter
[    0.114662] VFS: Disk quotas dquot_6.6.0
[    0.114727] VFS: Dquot-cache hash table entries: 512 (order 0, 4096 bytes)
[    0.114943] pnp: PnP ACPI: disabled
[    0.120538] NET: Registered protocol family 2
[    0.120950] tcp_listen_portaddr_hash hash table entries: 2048 (order: 3, 32768 bytes, linear)
[    0.121018] TCP established hash table entries: 32768 (order: 6, 262144 bytes, linear)
[    0.121260] TCP bind hash table entries: 32768 (order: 7, 524288 bytes, linear)
[    0.121802] TCP: Hash tables configured (established 32768 bind 32768)
[    0.121949] UDP hash table entries: 2048 (order: 4, 65536 bytes, linear)
[    0.122047] UDP-Lite hash table entries: 2048 (order: 4, 65536 bytes, linear)
[    0.122263] NET: Registered protocol family 1
[    0.122796] RPC: Registered named UNIX socket transport module.
[    0.122810] RPC: Registered udp transport module.
[    0.122821] RPC: Registered tcp transport module.
[    0.122831] RPC: Registered tcp NFSv4.1 backchannel transport module.
[    0.122850] PCI: CLS 0 bytes, default 64
[    0.123057] Unpacking initramfs...
[    1.191696] Freeing initrd memory: 24236K
[    1.192241] kvm [1]: IPA Size Limit: 40bits
[    1.193026] kvm [1]: GICv3: no GICV resource entry
[    1.193040] kvm [1]: disabling GICv2 emulation
[    1.193069] kvm [1]: GIC system register CPU interface enabled
[    1.193178] kvm [1]: vgic interrupt IRQ1
[    1.193323] kvm [1]: Hyp mode initialized successfully
[    1.196991] Initialise system trusted keyrings
[    1.197142] workingset: timestamp_bits=44 max_order=20 bucket_order=0
[    1.203283] squashfs: version 4.0 (2009/01/31) Phillip Lougher
[    1.203979] NFS: Registering the id_resolver key type
[    1.204017] Key type id_resolver registered
[    1.204027] Key type id_legacy registered
[    1.204046] nfs4filelayout_init: NFSv4 File Layout Driver Registering...
[    1.204229] 9p: Installing v9fs 9p2000 file system support
[    1.244795] Key type asymmetric registered
[    1.244810] Asymmetric key parser 'x509' registered
[    1.244850] Block layer SCSI generic (bsg) driver version 0.4 loaded (major 245)
[    1.244867] io scheduler mq-deadline registered
[    1.244879] io scheduler kyber registered
[    1.253988] gpio-427 (xirq0): hogged as input
[    1.254024] gpio-437 (xirq10): hogged as input
[    1.256150] uniphier-pcie 66000000.pcie: invalid resource
[    1.257038] EINJ: ACPI disabled.
[    1.273041] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
[    1.275558] 54006800.serial: ttyS0 at MMIO 0x54006800 (irq = 7, base_baud = 3676470) is a 16550A
[    2.195244] printk: console [ttyS0] enabled
[    2.200114] 54006a00.serial: ttyS2 at MMIO 0x54006a00 (irq = 8, base_baud = 3676470) is a 16550A
[    2.209472] 54006b00.serial: ttyS3 at MMIO 0x54006b00 (irq = 9, base_baud = 3676470) is a 16550A
[    2.219222] SuperH (H)SCI(F) driver initialized
[    2.224360] msm_serial: driver initialized
[    2.229969] cacheinfo: Unable to detect cache hierarchy for CPU 0
[    2.243361] loop: module loaded
[    2.247520] megasas: 07.714.04.00-rc1
[    2.256366] libphy: Fixed MDIO Bus: probed
[    2.261405] tun: Universal TUN/TAP device driver, 1.6
[    2.267383] thunder_xcv, ver 1.0
[    2.270667] thunder_bgx, ver 1.0
[    2.273932] nicpf, ver 1.0
[    2.277750] hclge is initializing
[    2.281131] hns3: Hisilicon Ethernet Network Driver for Hip08 Family - version
[    2.288386] hns3: Copyright (c) 2017 Huawei Corporation.
[    2.293763] e1000: Intel(R) PRO/1000 Network Driver
[    2.298658] e1000: Copyright (c) 1999-2006 Intel Corporation.
[    2.304465] e1000e: Intel(R) PRO/1000 Network Driver
[    2.309451] e1000e: Copyright(c) 1999 - 2015 Intel Corporation.
[    2.315426] igb: Intel(R) Gigabit Ethernet Network Driver
[    2.320844] igb: Copyright (c) 2007-2014 Intel Corporation.
[    2.326467] igbvf: Intel(R) Gigabit Virtual Function Network Driver
[    2.332752] igbvf: Copyright (c) 2009 - 2012 Intel Corporation.
[    2.339037] sky2: driver version 1.30
[    2.343674] ave 65000000.ethernet: Using random MAC address: 82:4d:4d:25:34:9d
[    2.474822] libphy: uniphier-mdio: probed
[    2.483088] RTL8211E Gigabit Ethernet 65000000.ethernet-ffffffff:00: attached PHY driver [RTL8211E Gigabit Ethernet] (mii_bus:phy_addr=65000000.ethernet-ffffffff:00, irq=POLL)
[    2.499197] ave 65000000.ethernet: Socionext AVE4 Ethernet IP v1.4 (irq=18, phy=rgmii)
[    2.507943] VFIO - User Level meta-driver version: 0.3
[    2.516227] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
[    2.522832] ehci-pci: EHCI PCI platform driver
[    2.527335] ehci-platform: EHCI generic platform driver
[    2.532726] ehci-orion: EHCI orion driver
[    2.536867] ehci-exynos: EHCI Exynos driver
[    2.541158] ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver
[    2.547376] ohci-pci: OHCI PCI platform driver
[    2.551873] ohci-platform: OHCI generic platform driver
[    2.557223] ohci-exynos: OHCI Exynos driver
[    2.562108] usbcore: registered new interface driver usb-storage
[    2.570705] i2c /dev entries driver
[    2.582814] uniphier-wdt 61840000.sysctrl:watchdog: watchdog driver (timeout=64 sec, nowayout=0)
[    2.594503] sdhci: Secure Digital Host Controller Interface driver
[    2.600746] sdhci: Copyright(c) Pierre Ossman
[    2.605573] Synopsys Designware Multimedia Card Interface Driver
[    2.612587] sdhci-pltfm: SDHCI platform and OF driver helper
[    2.618719] sdhci-cdns 5a000000.mmc: allocated mmc-pwrseq
[    2.655708] mmc0: SDHCI controller on 5a000000.mmc [5a000000.mmc] using ADMA 64-bit
[    2.665301] ledtrig-cpu: registered to indicate activity on CPUs
[    2.672070] SMCCC: SOC_ID: ARCH_SOC_ID(0) returned error: ffffffffffffffff
[    2.679816] usbcore: registered new interface driver usbhid
[    2.685435] usbhid: USB HID core driver
[    2.692948] optee: probing for conduit method.
[    2.697452] optee: revision 3.8 (af141c61)
[    2.704614] optee: dynamic shared memory is enabled
[    2.755775] optee: initialized driver
[    2.761336] NET: Registered protocol family 17
[    2.765987] 9pnet: Installing 9P2000 support
[    2.770332] Key type dns_resolver registered
[    2.774840] registered taskstats version 1
[    2.778967] Loading compiled-in X.509 certificates
[    2.791384] mmc0: new HS200 MMC card at address 0001
[    2.797156] mmcblk0: mmc0:0001 016G30 14.7 GiB
[    2.801188] uniphier-pcie 66000000.pcie: invalid resource
[    2.802087] mmcblk0boot0: mmc0:0001 016G30 partition 1 4.00 MiB
[    2.813353] mmcblk0boot1: mmc0:0001 016G30 partition 2 4.00 MiB
[    2.819480] mmcblk0rpmb: mmc0:0001 016G30 partition 3 4.00 MiB, chardev (234:0)
[    2.831986]  mmcblk0: p1 p2 p3 p4 p5
[    3.107392] uniphier-pcie 66000000.pcie: host bridge /soc@0/pcie@66000000 ranges:
[    3.114962] uniphier-pcie 66000000.pcie:       IO 0x002ffe0000..0x002ffeffff -> 0x0000000000
[    3.123457] uniphier-pcie 66000000.pcie:      MEM 0x0020000000..0x002ffdffff -> 0x0020000000
[    3.232144] uniphier-pcie 66000000.pcie: Link up
[    3.236912] uniphier-pcie 66000000.pcie: PCI host bridge to bus 0000:00
[    3.243557] pci_bus 0000:00: root bus resource [bus 00-ff]
[    3.249068] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
[    3.255274] pci_bus 0000:00: root bus resource [mem 0x20000000-0x2ffdffff]
[    3.262206] pci 0000:00:00.0: [50c1:0001] type 01 class 0x060400
[    3.268261] pci 0000:00:00.0: reg 0x10: [mem 0x00000000-0x03ffffff]
[    3.274564] pci 0000:00:00.0: reg 0x38: [mem 0x00000000-0x0000ffff pref]
[    3.281352] pci 0000:00:00.0: supports D1
[    3.285380] pci 0000:00:00.0: PME# supported from D0 D1 D3hot D3cold
[    3.293567] pci 0000:01:00.0: [10ec:8168] type 00 class 0x020000
[    3.299772] pci 0000:01:00.0: reg 0x10: [io  0x0000-0x00ff]
[    3.305503] pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x00000fff 64bit]
[    3.312410] pci 0000:01:00.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit pref]
[    3.320290] pci 0000:01:00.0: supports D1 D2
[    3.324582] pci 0000:01:00.0: PME# supported from D0 D1 D2 D3hot D3cold
[    3.343875] pci 0000:00:00.0: BAR 0: assigned [mem 0x20000000-0x23ffffff]
[    3.350700] pci 0000:00:00.0: BAR 14: assigned [mem 0x24000000-0x240fffff]
[    3.357603] pci 0000:00:00.0: BAR 15: assigned [mem 0x24100000-0x241fffff 64bit pref]
[    3.365462] pci 0000:00:00.0: BAR 6: assigned [mem 0x24200000-0x2420ffff pref]
[    3.372711] pci 0000:00:00.0: BAR 13: assigned [io  0x1000-0x1fff]
[    3.378919] pci 0000:01:00.0: BAR 4: assigned [mem 0x24100000-0x24103fff 64bit pref]
[    3.386762] pci 0000:01:00.0: BAR 2: assigned [mem 0x24000000-0x24000fff 64bit]
[    3.394164] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
[    3.400303] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
[    3.405549] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
[    3.411669] pci 0000:00:00.0:   bridge window [mem 0x24000000-0x240fffff]
[    3.418484] pci 0000:00:00.0:   bridge window [mem 0x24100000-0x241fffff 64bit pref]
[    3.426635] pcieport 0000:00:00.0: PME: Signaling with IRQ 24
[    3.432638] pcieport 0000:00:00.0: AER: enabled with IRQ 24
[    3.438585] r8169 0000:01:00.0: enabling device (0000 -> 0003)
[    3.448382] libphy: r8169: probed
[    3.452575] r8169 0000:01:00.0 eth1: RTL8168e/8111e, 7c:8b:ca:03:1f:33, XID 2c2, IRQ 25
[    3.460628] r8169 0000:01:00.0 eth1: jumbo features [frames: 9194 bytes, tx checksumming: ko]
[    3.478193] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
[    3.483775] xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 1
[    3.491643] xhci-hcd xhci-hcd.0.auto: hcc params 0x0230fe6d hci version 0x110 quirks 0x0000000002010010
[    3.501129] xhci-hcd xhci-hcd.0.auto: irq 19, io mem 0x65a00000
[    3.507936] hub 1-0:1.0: USB hub found
[    3.511732] hub 1-0:1.0: 4 ports detected
[    3.516166] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
[    3.521687] xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 2
[    3.529386] xhci-hcd xhci-hcd.0.auto: Host supports USB 3.0 SuperSpeed
[    3.535993] usb usb2: We don't know the algorithms for LPM for this host, disabling LPM.
[    3.544539] hub 2-0:1.0: USB hub found
[    3.548330] hub 2-0:1.0: 2 ports detected
[    3.553185] ALSA device list:
[    3.556179]   No soundcards found.
[    3.563862] Freeing unused kernel memory: 5696K
[    3.568513] Run /init as init process
[    3.572184]   with arguments:
[    3.572186]     /init
[    3.572188]   with environment:
[    3.572191]     HOME=/
[    3.572193]     TERM=linux
[    3.702799] udevd[151]: starting version 3.2.1
[    3.707631] random: udevd: uninitialized urandom read (16 bytes read)
[    3.714216] random: udevd: uninitialized urandom read (16 bytes read)
[    3.720702] random: udevd: uninitialized urandom read (16 bytes read)
[    3.731790] udevd[152]: starting eudev-3.2.1
[    3.848597] random: fast init done
[    3.854529] usb 1-4: new high-speed USB device number 2 using xhci-hcd
[    4.125374] splice write not supported for file  (pid: 215 comm: cat)
[    4.266321] splice write not supported for file  (pid: 263 comm: cat)
[    4.277542] splice write not supported for file  (pid: 266 comm: cat)
[    4.284170] splice write not supported for file  (pid: 266 comm: cat)
[    4.295556] splice write not supported for file  (pid: 270 comm: cat)
[    4.307134] splice write not supported for file  (pid: 273 comm: cat)
[    4.324074] splice write not supported for file  (pid: 278 comm: cat)
[    4.335313] splice write not supported for file  (pid: 281 comm: cat)
[    4.342004] splice write not supported for file  (pid: 281 comm: cat)
[    4.353363] splice write not supported for file  (pid: 285 comm: cat)
root@akebi96:~# 
root@akebi96:~# lspci -vv
00:00.0 Class 0604: Device 50c1:0001 (rev 01)
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- DisINTx+
        Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
        Latency: 0
        Interrupt: pin A routed to IRQ 24
        Region 0: Memory at 20000000 (32-bit, non-prefetchable) [size=64M]
        Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
        I/O behind bridge: 00001000-00001fff
        Memory behind bridge: 24000000-240fffff
        Prefetchable memory behind bridge: 0000000024100000-00000000241fffff
        Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
        [virtual] Expansion ROM at 24200000 [disabled] [size=64K]
        BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
                PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
        Capabilities: [40] Power Management version 3
                Flags: PMEClk- DSI- D1+ D2- AuxCurrent=375mA PME(D0+,D1+,D2-,D3hot+,D3cold+)
                Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
        Capabilities: [50] MSI: Enable+ Count=1/1 Maskable+ 64bit+
                Address: 00000000bbfff000  Data: 0000
                Masking: 00000000  Pending: 00000000
        Capabilities: [70] Express (v2) Root Port (Slot-), MSI 00
                DevCap: MaxPayload 256 bytes, PhantFunc 0
                        ExtTag- RBE+
                DevCtl: Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+
                        RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop-
                        MaxPayload 128 bytes, MaxReadReq 512 bytes
                DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
                LnkCap: Port #0, Speed 5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <1us, L1 <64us
                        ClockPM- Surprise- LLActRep+ BwNot+ ASPMOptComp+
                LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+
                        ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
                LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt-
                RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible-
                RootCap: CRSVisible-
                RootSta: PME ReqID 0000, PMEStatus- PMEPending-
                DevCap2: Completion Timeout: Range ABCD, TimeoutDis+, LTR+, OBFF Via message/WAKE# ARIFwd-
                DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR+, OBFF Disabled ARIFwd-
                LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis-
                         Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
                         Compliance De-emphasis: -6dB
                LnkSta2: Current De-emphasis Level: -3.5dB, EqualizationComplete-, EqualizationPhase1-
                         EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
        Capabilities: [100 v2] Advanced Error Reporting
                UESta:  DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
                UEMsk:  DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
                UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
                CESta:  RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
                CEMsk:  RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
                AERCap: First Error Pointer: 00, GenCap+ CGenEn- ChkCap+ ChkEn-
        Kernel driver in use: pcieport

01:00.0 Class 0200: Device 10ec:8168 (rev 06)
        Subsystem: Device 7470:3468
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
        Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
        Latency: 0, Cache Line Size: 64 bytes
        Interrupt: pin A routed to IRQ 23
        Region 0: I/O ports at 1000 [size=256]
        Region 2: Memory at 24000000 (64-bit, non-prefetchable) [size=4K]
        Region 4: Memory at 24100000 (64-bit, prefetchable) [size=16K]
        Capabilities: [40] Power Management version 3
                Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
                Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
        Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+
                Address: 0000000000000000  Data: 0000
        Capabilities: [70] Express (v2) Endpoint, MSI 01
                DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us
                        ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset- SlotPowerLimit 0.000W
                DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
                        RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop-
                        MaxPayload 128 bytes, MaxReadReq 512 bytes
                DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
                LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <512ns, L1 <64us
                        ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp-
                LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+
                        ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
                LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
                DevCap2: Completion Timeout: Not Supported, TimeoutDis+, LTR-, OBFF Not Supported
                DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR-, OBFF Disabled
                LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis-
                         Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
                         Compliance De-emphasis: -6dB
                LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-, EqualizationPhase1-
                         EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
        Capabilities: [b0] MSI-X: Enable+ Count=4 Masked-
                Vector table: BAR=4 offset=00000000
                PBA: BAR=4 offset=00000800
        Capabilities: [d0] Vital Product Data Not readable
        Capabilities: [100 v1] Advanced Error Reporting
                UESta:  DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
                UEMsk:  DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
                UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
                CESta:  RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
                CEMsk:  RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
                AERCap: First Error Pointer: 00, GenCap+ CGenEn- ChkCap+ ChkEn-
        Capabilities: [140 v1] Virtual Channel
                Caps:   LPEVC=0 RefClk=100ns PATEntryBits=1
                Arb:    Fixed- WRR32- WRR64- WRR128-
                Ctrl:   ArbSelect=Fixed
                Status: InProgress-
                VC0:    Caps:   PATOffset=00 MaxTimeSlots=1 RejSnoopTrans-
                        Arb:    Fixed- WRR32- WRR64- WRR128- TWRR128- WRR256-
                        Ctrl:   Enable+ ID=0 ArbSelect=Fixed TC/VC=ff
                        Status: NegoPending- InProgress-
        Capabilities: [160 v1] Device Serial Number f8-45-00-00-68-4c-e0-00
        Kernel driver in use: r8169

root@akebi96:~# cat /proc/interrupts
[  189.905575] do_splice_from: 3 callbacks suppressed
[  189.905588] splice write not supported for file /ttyS0 (pid: 383 comm: cat)
           CPU0       CPU1       CPU2       CPU3
  1:          0          0          0          0     GICv3  25 Level     vgic
  3:        539        846       1110       1171     GICv3  30 Level     arch_timer
  4:          0          0          0          0     GICv3  27 Level     kvm guest vtimer
  7:       2078          0          0          0     GICv3  65 Level     ttyS0
 11:          0          0          0          0     GICv3  73 Level     58780000.i2c
 12:          0          0          0          0     GICv3  74 Level     58781000.i2c
 13:          0          0          0          0     GICv3  75 Level     58782000.i2c
 14:          0          0          0          0     GICv3  57 Level     58785000.i2c
 15:        702          0          0          0     GICv3 110 Level     mmc0
 17:          0          0          0          0     GICv3  35 Level     thermal
 18:        355          0          0          0     GICv3  98 Level     eth0
 19:         46          0          0          0     GICv3 166 Level     xhci-hcd:usb1
 24:          0          0          0          0   PCI-MSI   0 Edge      PCIe PME, aerdrv
 25:          0          0          0          0   PCI-MSI 524288 Edge      eth1
IPI0:       375        508        409        457       Rescheduling interrupts
IPI1:       288        275        541        413       Function call interrupts
IPI2:         0          0          0          0       CPU stop interrupts
IPI3:         0          0          0          0       CPU stop (for crash dump) interrupts
IPI4:         0          0          0          0       Timer broadcast interrupts
IPI5:         0          0          0          0       IRQ work interrupts
IPI6:         0          0          0          0       CPU wake-up interrupts
Err:          0
root@akebi96:~#
Lorenzo Pieralisi July 14, 2020, 1:27 p.m. UTC | #7 
On Thu, Jun 18, 2020 at 05:38:09PM +0900, Kunihiko Hayashi wrote:
> The misc interrupts consisting of PME, AER, and Link event, is handled
> by INTx handler, however, these interrupts should be also handled by
> MSI handler.

Define what you mean please.

> This adds the function uniphier_pcie_misc_isr() that handles misc
> interrupts, which is called from both INTx and MSI handlers.
> This function detects PME and AER interrupts with the status register,
> and invoke PME and AER drivers related to MSI.
> 
> And this sets the mask for misc interrupts from INTx if MSI is enabled
> and sets the mask for misc interrupts from MSI if MSI is disabled.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/pci/controller/dwc/pcie-uniphier.c | 57 ++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> index a5401a0..5ce2479 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> @@ -44,7 +44,9 @@
>  #define PCL_SYS_AUX_PWR_DET		BIT(8)
>  
>  #define PCL_RCV_INT			0x8108
> +#define PCL_RCV_INT_ALL_INT_MASK	GENMASK(28, 25)
>  #define PCL_RCV_INT_ALL_ENABLE		GENMASK(20, 17)
> +#define PCL_RCV_INT_ALL_MSI_MASK	GENMASK(12, 9)
>  #define PCL_CFG_BW_MGT_STATUS		BIT(4)
>  #define PCL_CFG_LINK_AUTO_BW_STATUS	BIT(3)
>  #define PCL_CFG_AER_RC_ERR_MSI_STATUS	BIT(2)
> @@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci)
>  
>  static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
>  {
> -	writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
> +	u32 val;
> +
> +	val = PCL_RCV_INT_ALL_ENABLE;
> +	if (pci_msi_enabled())
> +		val |= PCL_RCV_INT_ALL_INT_MASK;
> +	else
> +		val |= PCL_RCV_INT_ALL_MSI_MASK;
> +
> +	writel(val, priv->base + PCL_RCV_INT);
>  	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>  }
>  
> @@ -231,32 +241,56 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = {
>  	.map = uniphier_pcie_intx_map,
>  };
>  
> -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
> +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
>  {
> -	struct pcie_port *pp = irq_desc_get_handler_data(desc);
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> -	struct irq_chip *chip = irq_desc_get_chip(desc);
> -	unsigned long reg;
> -	u32 val, bit, virq;
> +	u32 val, virq;
>  
> -	/* INT for debug */
>  	val = readl(priv->base + PCL_RCV_INT);
>  
>  	if (val & PCL_CFG_BW_MGT_STATUS)
>  		dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
> +
>  	if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>  		dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
> -	if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
> -		dev_dbg(pci->dev, "Root Error\n");
> -	if (val & PCL_CFG_PME_MSI_STATUS)
> -		dev_dbg(pci->dev, "PME Interrupt\n");
> +
> +	if (is_msi) {
> +		if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
> +			dev_dbg(pci->dev, "Root Error Status\n");
> +
> +		if (val & PCL_CFG_PME_MSI_STATUS)
> +			dev_dbg(pci->dev, "PME Interrupt\n");
> +
> +		if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
> +			   PCL_CFG_PME_MSI_STATUS)) {
> +			virq = irq_linear_revmap(pp->irq_domain, 0);

I think this is wrong. pp->irq_domain is the DWC MSI domain, how do
you know that hwirq 0 *is* the AER/PME interrupt ?

It just *works* in this case because the port driver probes and alloc
MSIs before any PCI device has a chance to do it and actually I think
this is just wrong also because hwirq 0 *is* usable by devices but
it can't be used because current code takes it for the PME/AER interrupt
(which AFAICS is an internal signal disconnected from the DWC MSI
interrupt controller).

I think this extra glue logic should be separate MSI domain
otherwise there is no way you can reliably look-up the virq
corresponding to AER/PME.

How does it work in HW ? Is the root port really sending a memory
write to raise an IRQ or it just signal the IRQ through internal
logic ? I think the root port MSI handling is different from the
DWC logic and should be treated separately.

Lorenzo

> +			generic_handle_irq(virq);
> +		}
> +	}
>  
>  	writel(val, priv->base + PCL_RCV_INT);
> +}
> +
> +static void uniphier_pcie_msi_host_isr(struct pcie_port *pp)
> +{
> +	uniphier_pcie_misc_isr(pp, true);
> +}
> +
> +static void uniphier_pcie_irq_handler(struct irq_desc *desc)
> +{
> +	struct pcie_port *pp = irq_desc_get_handler_data(desc);
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long reg;
> +	u32 val, bit, virq;
>  
>  	/* INTx */
>  	chained_irq_enter(chip, desc);
>  
> +	uniphier_pcie_misc_isr(pp, false);
> +
>  	val = readl(priv->base + PCL_RCV_INTX);
>  	reg = FIELD_GET(PCL_RCV_INTX_ALL_STATUS, val);
>  
> @@ -330,6 +364,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp)
>  
>  static const struct dw_pcie_host_ops uniphier_pcie_host_ops = {
>  	.host_init = uniphier_pcie_host_init,
> +	.msi_host_isr = uniphier_pcie_msi_host_isr,
>  };
>  
>  static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
> -- 
> 2.7.4
>
Kunihiko Hayashi July 15, 2020, 10:04 a.m. UTC | #8 
Hi Lorenzo,

On 2020/07/14 22:27, Lorenzo Pieralisi wrote:
> On Thu, Jun 18, 2020 at 05:38:09PM +0900, Kunihiko Hayashi wrote:
>> The misc interrupts consisting of PME, AER, and Link event, is handled
>> by INTx handler, however, these interrupts should be also handled by
>> MSI handler.
> 
> Define what you mean please.

AER/PME signals are assigned to the same signal as MSI by internal logic,
that is, AER/PME and MSI are assigned to the same GIC interrupt number.
So it's necessary to modify the code to call the misc handler from MSI handler.

I'll rewrite it next.

> 
>> This adds the function uniphier_pcie_misc_isr() that handles misc
>> interrupts, which is called from both INTx and MSI handlers.
>> This function detects PME and AER interrupts with the status register,
>> and invoke PME and AER drivers related to MSI.
>>
>> And this sets the mask for misc interrupts from INTx if MSI is enabled
>> and sets the mask for misc interrupts from MSI if MSI is disabled.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Jingoo Han <jingoohan1@gmail.com>
>> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-uniphier.c | 57 ++++++++++++++++++++++++------
>>   1 file changed, 46 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
>> index a5401a0..5ce2479 100644
>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>> @@ -44,7 +44,9 @@
>>   #define PCL_SYS_AUX_PWR_DET		BIT(8)
>>   
>>   #define PCL_RCV_INT			0x8108
>> +#define PCL_RCV_INT_ALL_INT_MASK	GENMASK(28, 25)
>>   #define PCL_RCV_INT_ALL_ENABLE		GENMASK(20, 17)
>> +#define PCL_RCV_INT_ALL_MSI_MASK	GENMASK(12, 9)
>>   #define PCL_CFG_BW_MGT_STATUS		BIT(4)
>>   #define PCL_CFG_LINK_AUTO_BW_STATUS	BIT(3)
>>   #define PCL_CFG_AER_RC_ERR_MSI_STATUS	BIT(2)
>> @@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci)
>>   
>>   static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
>>   {
>> -	writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
>> +	u32 val;
>> +
>> +	val = PCL_RCV_INT_ALL_ENABLE;
>> +	if (pci_msi_enabled())
>> +		val |= PCL_RCV_INT_ALL_INT_MASK;
>> +	else
>> +		val |= PCL_RCV_INT_ALL_MSI_MASK;
>> +
>> +	writel(val, priv->base + PCL_RCV_INT);
>>   	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>>   }
>>   
>> @@ -231,32 +241,56 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = {
>>   	.map = uniphier_pcie_intx_map,
>>   };
>>   
>> -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
>>   {
>> -	struct pcie_port *pp = irq_desc_get_handler_data(desc);
>>   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>   	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
>> -	struct irq_chip *chip = irq_desc_get_chip(desc);
>> -	unsigned long reg;
>> -	u32 val, bit, virq;
>> +	u32 val, virq;
>>   
>> -	/* INT for debug */
>>   	val = readl(priv->base + PCL_RCV_INT);
>>   
>>   	if (val & PCL_CFG_BW_MGT_STATUS)
>>   		dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
>> +
>>   	if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>>   		dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
>> -	if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>> -		dev_dbg(pci->dev, "Root Error\n");
>> -	if (val & PCL_CFG_PME_MSI_STATUS)
>> -		dev_dbg(pci->dev, "PME Interrupt\n");
>> +
>> +	if (is_msi) {
>> +		if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>> +			dev_dbg(pci->dev, "Root Error Status\n");
>> +
>> +		if (val & PCL_CFG_PME_MSI_STATUS)
>> +			dev_dbg(pci->dev, "PME Interrupt\n");
>> +
>> +		if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
>> +			   PCL_CFG_PME_MSI_STATUS)) {
>> +			virq = irq_linear_revmap(pp->irq_domain, 0);
> 
> I think this is wrong. pp->irq_domain is the DWC MSI domain, how do
> you know that hwirq 0 *is* the AER/PME interrupt ?

When AER/PME drivers are probed, AER/PME interrupts are registered
as MSI-0.

The pcie_message_numbers() function refers the following fields of
PCI registers,

   - PCI_EXP_FLAGS_IRQ (for PME)
   - PCI_ERR_ROOT_AER_IRQ (for AER)

and decides AER/PME interrupts numbers in MSI domain.
Initial values of both fields are 0, so these interrupts are set to MSI-0.

However, pcie_uniphier driver doesn't know that these interrupts are MSI-0.
Surely using 0 here is wrong.
I think that the method to get virq for AER/PME from pcieport is needed.

>
> It just *works* in this case because the port driver probes and alloc
> MSIs before any PCI device has a chance to do it and actually I think
> this is just wrong also because hwirq 0 *is* usable by devices but
> it can't be used because current code takes it for the PME/AER interrupt
> (which AFAICS is an internal signal disconnected from the DWC MSI
> interrupt controller).

AER/PME interrupts are with IRQF_SHARED, so hwirq 0 can share
any PCI device, however, the multiple handlers might be called
with other factor, so I don't think it is desiable.

> 
> I think this extra glue logic should be separate MSI domain
> otherwise there is no way you can reliably look-up the virq
> corresponding to AER/PME.

Ok, however, it seems that there is no way to get virq for AER/PME
from pcieport in pcie/portdrv_core.c.

When I try to separate AER/PME interrtups from MSI domain,
how should I get virq for AER/PME?

> 
> How does it work in HW ? Is the root port really sending a memory
> write to raise an IRQ or it just signal the IRQ through internal
> logic ? I think the root port MSI handling is different from the
> DWC logic and should be treated separately.

The internal logic assigns the same signal as MSI interrupt
to AER/PME interrupts.

The MSI handler checks internal status register PCL_RCV_INT,
and know if the signal is AER or PME interrupt. MSI memory write
isn't used for the signal.

Since DWC MSI handler doesn't have a method to check the internal
status register, I added callback .msi_host_isr() to DWC MSI handler
in patch 1/6.

Thank you,

---
Best Regards
Kunihiko Hayashi
Kunihiko Hayashi Aug. 7, 2020, 10:12 a.m. UTC | #9 
On 2020/07/15 19:04, Kunihiko Hayashi wrote:
> Hi Lorenzo,
> 
> On 2020/07/14 22:27, Lorenzo Pieralisi wrote:
>> On Thu, Jun 18, 2020 at 05:38:09PM +0900, Kunihiko Hayashi wrote:
>>> The misc interrupts consisting of PME, AER, and Link event, is handled
>>> by INTx handler, however, these interrupts should be also handled by
>>> MSI handler.
>>
>> Define what you mean please.

[snip]

>> I think this is wrong. pp->irq_domain is the DWC MSI domain, how do
>> you know that hwirq 0 *is* the AER/PME interrupt ?
> 
> When AER/PME drivers are probed, AER/PME interrupts are registered
> as MSI-0.
> 
> The pcie_message_numbers() function refers the following fields of
> PCI registers,
> 
>     - PCI_EXP_FLAGS_IRQ (for PME)
>     - PCI_ERR_ROOT_AER_IRQ (for AER)
> 
> and decides AER/PME interrupts numbers in MSI domain.
> Initial values of both fields are 0, so these interrupts are set to MSI-0.
> 
> However, pcie_uniphier driver doesn't know that these interrupts are MSI-0.
> Surely using 0 here is wrong.
> I think that the method to get virq for AER/PME from pcieport is needed.

To avoid using hard-coded MSI-0, I'll add new function to get a virq number
corresponding to each service (AER/PME) to portdrv.

I'll update the series in v6.

Thank you,
---
Best Regards
Kunihiko Hayashi
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index a5401a0..5ce2479 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -44,7 +44,9 @@ 
 #define PCL_SYS_AUX_PWR_DET		BIT(8)
 
 #define PCL_RCV_INT			0x8108
+#define PCL_RCV_INT_ALL_INT_MASK	GENMASK(28, 25)
 #define PCL_RCV_INT_ALL_ENABLE		GENMASK(20, 17)
+#define PCL_RCV_INT_ALL_MSI_MASK	GENMASK(12, 9)
 #define PCL_CFG_BW_MGT_STATUS		BIT(4)
 #define PCL_CFG_LINK_AUTO_BW_STATUS	BIT(3)
 #define PCL_CFG_AER_RC_ERR_MSI_STATUS	BIT(2)
@@ -167,7 +169,15 @@  static void uniphier_pcie_stop_link(struct dw_pcie *pci)
 
 static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
 {
-	writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
+	u32 val;
+
+	val = PCL_RCV_INT_ALL_ENABLE;
+	if (pci_msi_enabled())
+		val |= PCL_RCV_INT_ALL_INT_MASK;
+	else
+		val |= PCL_RCV_INT_ALL_MSI_MASK;
+
+	writel(val, priv->base + PCL_RCV_INT);
 	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
 }
 
@@ -231,32 +241,56 @@  static const struct irq_domain_ops uniphier_intx_domain_ops = {
 	.map = uniphier_pcie_intx_map,
 };
 
-static void uniphier_pcie_irq_handler(struct irq_desc *desc)
+static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
 {
-	struct pcie_port *pp = irq_desc_get_handler_data(desc);
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	unsigned long reg;
-	u32 val, bit, virq;
+	u32 val, virq;
 
-	/* INT for debug */
 	val = readl(priv->base + PCL_RCV_INT);
 
 	if (val & PCL_CFG_BW_MGT_STATUS)
 		dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
+
 	if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
 		dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
-	if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
-		dev_dbg(pci->dev, "Root Error\n");
-	if (val & PCL_CFG_PME_MSI_STATUS)
-		dev_dbg(pci->dev, "PME Interrupt\n");
+
+	if (is_msi) {
+		if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
+			dev_dbg(pci->dev, "Root Error Status\n");
+
+		if (val & PCL_CFG_PME_MSI_STATUS)
+			dev_dbg(pci->dev, "PME Interrupt\n");
+
+		if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
+			   PCL_CFG_PME_MSI_STATUS)) {
+			virq = irq_linear_revmap(pp->irq_domain, 0);
+			generic_handle_irq(virq);
+		}
+	}
 
 	writel(val, priv->base + PCL_RCV_INT);
+}
+
+static void uniphier_pcie_msi_host_isr(struct pcie_port *pp)
+{
+	uniphier_pcie_misc_isr(pp, true);
+}
+
+static void uniphier_pcie_irq_handler(struct irq_desc *desc)
+{
+	struct pcie_port *pp = irq_desc_get_handler_data(desc);
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long reg;
+	u32 val, bit, virq;
 
 	/* INTx */
 	chained_irq_enter(chip, desc);
 
+	uniphier_pcie_misc_isr(pp, false);
+
 	val = readl(priv->base + PCL_RCV_INTX);
 	reg = FIELD_GET(PCL_RCV_INTX_ALL_STATUS, val);
 
@@ -330,6 +364,7 @@  static int uniphier_pcie_host_init(struct pcie_port *pp)
 
 static const struct dw_pcie_host_ops uniphier_pcie_host_ops = {
 	.host_init = uniphier_pcie_host_init,
+	.msi_host_isr = uniphier_pcie_msi_host_isr,
 };
 
 static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,