From: Marc Zyngier <maz@kernel.org>
To: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
Jingoo Han <jingoohan1@gmail.com>,
Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
Rob Herring <robh+dt@kernel.org>,
Masahiro Yamada <yamada.masahiro@socionext.com>,
linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org,
Masami Hiramatsu <masami.hiramatsu@linaro.org>,
Jassi Brar <jaswinder.singh@linaro.org>
Subject: Re: [PATCH v3 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER
Date: Thu, 04 Jun 2020 11:11:34 +0100 [thread overview]
Message-ID: <9cbfdacba32c5e351fd9e14444768666@kernel.org> (raw)
In-Reply-To: <2e07d3d3-515b-57e1-0a36-8892bc38bb7b@socionext.com>
On 2020-06-04 10:43, Kunihiko Hayashi wrote:
[...]
>>> -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp)
>>> {
>>> - 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 (pci_msi_enabled()) {
>>
>> This checks whether the kernel supports MSIs. Not that they are
>> enabled in your controller. Is that really what you want to do?
>
> The below two status bits are valid when the interrupt for MSI is
> asserted.
> That is, pci_msi_enabled() is wrong.
>
> I'll modify the function to check the two bits only if this function is
> called from MSI handler.
>
>>
>>> + if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) {
>>> + dev_dbg(pci->dev, "Root Error Status\n");
>>> + virq = irq_linear_revmap(pp->irq_domain, 0);
>>> + generic_handle_irq(virq);
>>> + }
>>> +
>>> + if (val & PCL_CFG_PME_MSI_STATUS) {
>>> + dev_dbg(pci->dev, "PME Interrupt\n");
>>> + virq = irq_linear_revmap(pp->irq_domain, 0);
>>> + generic_handle_irq(virq);
>>> + }
>>
>> These two cases do the exact same thing, calling the same interrupt.
>> What is the point of dealing with them independently?
>
> Both PME and AER are asserted from MSI-0, and each handler checks its
> own
> status bit in the PCIe register (aer_irq() in pcie/aer.c and
> pcie_pme_irq()
> in pcie/pme.c).
> So I think this handler calls generic_handle_irq() for the same MSI-0.
So what is wrong with
if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
PCL_CFG_PME_MSI_STATUS)) {
// handle interrupt
}
?
If you have two handlers for the same interrupt, this is a shared
interrupt and each handler will be called in turn.
M.
--
Jazz is not dead. It just smells funny...
next prev parent reply other threads:[~2020-06-04 10:11 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-03 8:54 [PATCH v3 0/6] PCI: uniphier: Add features for UniPhier PCIe host controller Kunihiko Hayashi
2020-06-03 8:54 ` [PATCH v3 1/6] PCI: dwc: Add msi_host_isr() callback Kunihiko Hayashi
2020-06-03 11:15 ` Marc Zyngier
2020-06-04 9:43 ` Kunihiko Hayashi
2020-06-03 8:54 ` [PATCH v3 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER Kunihiko Hayashi
2020-06-03 11:22 ` Marc Zyngier
2020-06-04 9:43 ` Kunihiko Hayashi
2020-06-04 10:11 ` Marc Zyngier [this message]
2020-06-05 2:36 ` Kunihiko Hayashi
2020-06-03 8:54 ` [PATCH v3 3/6] dt-bindings: PCI: uniphier: Add iATU register description Kunihiko Hayashi
2020-06-03 8:54 ` [PATCH v3 4/6] PCI: uniphier: Add iATU register support Kunihiko Hayashi
2020-06-03 8:54 ` [PATCH v3 5/6] PCI: uniphier: Add error message when failed to get phy Kunihiko Hayashi
2020-06-03 8:54 ` [PATCH v3 6/6] PCI: uniphier: Use devm_platform_ioremap_resource_byname() Kunihiko Hayashi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9cbfdacba32c5e351fd9e14444768666@kernel.org \
--to=maz@kernel.org \
--cc=bhelgaas@google.com \
--cc=devicetree@vger.kernel.org \
--cc=gustavo.pimentel@synopsys.com \
--cc=hayashi.kunihiko@socionext.com \
--cc=jaswinder.singh@linaro.org \
--cc=jingoohan1@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=masami.hiramatsu@linaro.org \
--cc=robh+dt@kernel.org \
--cc=yamada.masahiro@socionext.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).