lkml.org 
[lkml]   [2020]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER
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.
--
Jazz is not dead. It just smells funny...

\
 
 \ /
  Last update: 2020-06-30 15:25    [W:0.084 / U:1.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site