lkml.org 
[lkml]   [2020]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER
On 2020-06-03 09:54, 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.
>
> This adds the function uniphier_pcie_misc_isr() that handles misc
> intterupts, which is called from both INTx and MSI handlers.

interrupts

> This function detects PME and AER interrupts with the status register,
> and invoke PME and AER drivers related to INTx or 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 | 53
> +++++++++++++++++++++++-------
> 1 file changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c
> b/drivers/pci/controller/dwc/pcie-uniphier.c
> index a5401a0..a8dda39 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,28 +241,48 @@ 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)
> {
> - 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?

> + 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?

> + }
>
> writel(val, priv->base + PCL_RCV_INT);
> +}
> +
> +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;
> +
> + /* misc interrupt */
> + uniphier_pcie_misc_isr(pp);

This is a chained handler called outside of a chained_irq_enter/exit
block. It isn't acceptable.

>
> /* INTx */
> chained_irq_enter(chip, desc);
> @@ -330,6 +360,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_misc_isr,
> };
>
> static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,

Thanks,

M.
--
Jazz is not dead. It just smells funny...

\
 
 \ /
  Last update: 2020-06-03 13:24    [W:0.095 / U:1.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site