Message-ID: <293ed6fe-6e73-f664-c26e-0aef744ce933@socionext.com>
Date:   Wed, 15 Jul 2020 19:04:13 +0900
From:   Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Rob Herring <robh+dt@...nel.org>,
        Yamada, Masahiro/山田 真弘 
        <yamada.masahiro@...ionext.com>, Marc Zyngier <maz@...nel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Masami Hiramatsu <masami.hiramatsu@...aro.org>,
        Jassi Brar <jaswinder.singh@...aro.org>
Subject: Re: [PATCH v5 2/6] PCI: uniphier: Add misc interrupt handler to
 invoke PME and AER

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@...nel.org>
>> Cc: Jingoo Han <jingoohan1@...il.com>
>> Cc: Gustavo Pimentel <gustavo.pimentel@...opsys.com>
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@...ionext.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

Powered by blists - more mailing lists