This patch adds misc interrupt handler to detect and invoke PME/AER event.
In UniPhier PCIe controller, PME/AER signals are assigned to the same
signal as MSI by the internal logic. These signals should be detected by
the internal register, however, DWC MSI handler can't handle these signals.
DWC MSI handler calls .msi_host_isr() callback function, that detects
PME/AER signals using the internal register and invokes the interrupt
with PME/AER IRQ numbers.
These IRQ numbers is obtained by uniphier_pcie_port_get_irq() function,
that finds the device that matches PME/AER from the devices associated
with Root Port, and returns its IRQ number.
Cc: Marc Zyngier <[email protected]>
Cc: Jingoo Han <[email protected]>
Cc: Gustavo Pimentel <[email protected]>
Cc: Lorenzo Pieralisi <[email protected]>
Signed-off-by: Kunihiko Hayashi <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
drivers/pci/controller/dwc/pcie-uniphier.c | 105 +++++++++++++++++++++++++----
1 file changed, 91 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index 7e8bad3..dcd8fa8 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -21,6 +21,7 @@
#include <linux/reset.h>
#include "pcie-designware.h"
+#include "../../pcie/portdrv.h"
#define PCL_PINCTRL0 0x002c
#define PCL_PERST_PLDN_REGEN BIT(12)
@@ -44,7 +45,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)
@@ -68,6 +71,8 @@ struct uniphier_pcie_priv {
struct reset_control *rst;
struct phy *phy;
struct irq_domain *legacy_irq_domain;
+ int aer_irq;
+ int pme_irq;
};
#define to_uniphier_pcie(x) dev_get_drvdata((x)->dev)
@@ -164,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);
}
@@ -228,28 +241,51 @@ 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;
- /* 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 (priv->aer_irq)
+ generic_handle_irq(priv->aer_irq);
+ }
+
+ if (val & PCL_CFG_PME_MSI_STATUS) {
+ dev_dbg(pci->dev, "PME Interrupt\n");
+ if (priv->pme_irq)
+ generic_handle_irq(priv->pme_irq);
+ }
+ }
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, irq;
+
+ uniphier_pcie_misc_isr(pp, false);
/* INTx */
chained_irq_enter(chip, desc);
@@ -258,8 +294,8 @@ static void uniphier_pcie_irq_handler(struct irq_desc *desc)
reg = FIELD_GET(PCL_RCV_INTX_ALL_STATUS, val);
for_each_set_bit(bit, ®, PCI_NUM_INTX) {
- virq = irq_linear_revmap(priv->legacy_irq_domain, bit);
- generic_handle_irq(virq);
+ irq = irq_linear_revmap(priv->legacy_irq_domain, bit);
+ generic_handle_irq(irq);
}
chained_irq_exit(chip, desc);
@@ -317,8 +353,45 @@ static int uniphier_pcie_host_init(struct pcie_port *pp)
return 0;
}
+static int uniphier_pcie_port_get_irq(struct pcie_port *pp, u32 service)
+{
+ struct pci_dev *pcidev;
+ int irq = 0;
+
+ if (!IS_ENABLED(CONFIG_PCIEAER) && !IS_ENABLED(CONFIG_PCIE_PME))
+ return 0;
+
+ /*
+ * Finds the device that matches 'service' from the devices
+ * associated with Root Port, and returns its IRQ number.
+ */
+ list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
+ irq = pcie_port_service_get_irq(pcidev, service);
+ if (irq)
+ break;
+ }
+
+ return irq;
+}
+
+static int uniphier_pcie_host_init_complete(struct pcie_port *pp)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
+
+ if (IS_ENABLED(CONFIG_PCIE_PME))
+ priv->pme_irq =
+ uniphier_pcie_port_get_irq(pp, PCIE_PORT_SERVICE_PME);
+ if (IS_ENABLED(CONFIG_PCIEAER))
+ priv->aer_irq =
+ uniphier_pcie_port_get_irq(pp, PCIE_PORT_SERVICE_AER);
+
+ return 0;
+}
+
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_pcie_host_enable(struct uniphier_pcie_priv *priv)
@@ -398,7 +471,11 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
priv->pci.pp.ops = &uniphier_pcie_host_ops;
- return dw_pcie_host_init(&priv->pci.pp);
+ ret = dw_pcie_host_init(&priv->pci.pp);
+ if (ret)
+ return ret;
+
+ return uniphier_pcie_host_init_complete(&priv->pci.pp);
}
static const struct of_device_id uniphier_pcie_match[] = {
--
2.7.4
Hello Kunihiko!
On Friday 23 April 2021 02:04:57 Kunihiko Hayashi wrote:
> This patch adds misc interrupt handler to detect and invoke PME/AER event.
>
> In UniPhier PCIe controller, PME/AER signals are assigned to the same
> signal as MSI by the internal logic. These signals should be detected by
> the internal register, however, DWC MSI handler can't handle these signals.
>
> DWC MSI handler calls .msi_host_isr() callback function, that detects
> PME/AER signals using the internal register and invokes the interrupt
> with PME/AER IRQ numbers.
>
> These IRQ numbers is obtained by uniphier_pcie_port_get_irq() function,
> that finds the device that matches PME/AER from the devices associated
> with Root Port, and returns its IRQ number.
If I understood this issue correctly, it means that your PCIe controller
does not issue regular MSI interrupt for PME and AER events, but rather
it issue controller specific interrupt and you need to figure out what
kind of controller-specific event happened (e.g. PME or AER or something
else).
But if your controller supports PME or AER then it expose in its PCIe
Root Port capabilities register MSI number for these PME and AER events.
Kernel PCIe PME and AER drivers read from capabilities register these
numbers and register irq functions to be called when interrupt happens.
So it means that you do not need to implement uniphier_pcie_port_get_irq
function via this "ugly" foreach and call pcie_port_service_get_irq. But
you can read this MSI interrupt number directly from your controller in
this pcie-uniphier.c driver and then use irq_find_mapping() to convert
hw MSI number to kernel's virq (used in generic_handle_irq()).
Because currently you use in pcie-uniphier.c call to function
pcie_port_service_get_irq() which returns cached interrupt number value
which was read from PCIe Root Port capability register by PCI subsystem
callbacked back to the pcie-uniphier.c driver.
For me this looks like "ugly" if you need to do something in
"complicated" way and add dependency e.g. on compile options like
"if (!IS_ENABLED(CONFIG_PCIEAER) && !IS_ENABLED(CONFIG_PCIE_PME))" if it
can be easily avoided.
I'm writing this because I was solving exactly same problem for aardvark
PCIe controller with PME, AER and HP interrupts (patches are on ML). So
I think that this pcie-uniphier.c implementation can be simplified
without need to use checks for CONFIG_* options and calling
pcie_port_service_get_irq() in list_for_each_entry loop.
Could you please post output of 'lspci -nn -vv'? In my opinion MSI
numbers for AER and PME in Root Port could be constant so it may
simplify implementation even more. (Just to note that in my case
aardvark returns zero as MSI number and it is also documented in spec).
> Cc: Marc Zyngier <[email protected]>
> Cc: Jingoo Han <[email protected]>
> Cc: Gustavo Pimentel <[email protected]>
> Cc: Lorenzo Pieralisi <[email protected]>
> Signed-off-by: Kunihiko Hayashi <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-uniphier.c | 105 +++++++++++++++++++++++++----
> 1 file changed, 91 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> index 7e8bad3..dcd8fa8 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> @@ -21,6 +21,7 @@
> #include <linux/reset.h>
>
> #include "pcie-designware.h"
> +#include "../../pcie/portdrv.h"
>
> #define PCL_PINCTRL0 0x002c
> #define PCL_PERST_PLDN_REGEN BIT(12)
> @@ -44,7 +45,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)
> @@ -68,6 +71,8 @@ struct uniphier_pcie_priv {
> struct reset_control *rst;
> struct phy *phy;
> struct irq_domain *legacy_irq_domain;
> + int aer_irq;
> + int pme_irq;
> };
>
> #define to_uniphier_pcie(x) dev_get_drvdata((x)->dev)
> @@ -164,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);
> }
>
> @@ -228,28 +241,51 @@ 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;
>
> - /* 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 (priv->aer_irq)
> + generic_handle_irq(priv->aer_irq);
> + }
> +
> + if (val & PCL_CFG_PME_MSI_STATUS) {
> + dev_dbg(pci->dev, "PME Interrupt\n");
> + if (priv->pme_irq)
> + generic_handle_irq(priv->pme_irq);
> + }
> + }
>
> 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, irq;
> +
> + uniphier_pcie_misc_isr(pp, false);
>
> /* INTx */
> chained_irq_enter(chip, desc);
> @@ -258,8 +294,8 @@ static void uniphier_pcie_irq_handler(struct irq_desc *desc)
> reg = FIELD_GET(PCL_RCV_INTX_ALL_STATUS, val);
>
> for_each_set_bit(bit, ®, PCI_NUM_INTX) {
> - virq = irq_linear_revmap(priv->legacy_irq_domain, bit);
> - generic_handle_irq(virq);
> + irq = irq_linear_revmap(priv->legacy_irq_domain, bit);
> + generic_handle_irq(irq);
> }
>
> chained_irq_exit(chip, desc);
> @@ -317,8 +353,45 @@ static int uniphier_pcie_host_init(struct pcie_port *pp)
> return 0;
> }
>
> +static int uniphier_pcie_port_get_irq(struct pcie_port *pp, u32 service)
> +{
> + struct pci_dev *pcidev;
> + int irq = 0;
> +
> + if (!IS_ENABLED(CONFIG_PCIEAER) && !IS_ENABLED(CONFIG_PCIE_PME))
> + return 0;
> +
> + /*
> + * Finds the device that matches 'service' from the devices
> + * associated with Root Port, and returns its IRQ number.
> + */
> + list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
> + irq = pcie_port_service_get_irq(pcidev, service);
> + if (irq)
> + break;
> + }
> +
> + return irq;
> +}
> +
> +static int uniphier_pcie_host_init_complete(struct pcie_port *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> +
> + if (IS_ENABLED(CONFIG_PCIE_PME))
> + priv->pme_irq =
> + uniphier_pcie_port_get_irq(pp, PCIE_PORT_SERVICE_PME);
> + if (IS_ENABLED(CONFIG_PCIEAER))
> + priv->aer_irq =
> + uniphier_pcie_port_get_irq(pp, PCIE_PORT_SERVICE_AER);
> +
> + return 0;
> +}
> +
> 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_pcie_host_enable(struct uniphier_pcie_priv *priv)
> @@ -398,7 +471,11 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
>
> priv->pci.pp.ops = &uniphier_pcie_host_ops;
>
> - return dw_pcie_host_init(&priv->pci.pp);
> + ret = dw_pcie_host_init(&priv->pci.pp);
> + if (ret)
> + return ret;
> +
> + return uniphier_pcie_host_init_complete(&priv->pci.pp);
> }
>
> static const struct of_device_id uniphier_pcie_match[] = {
> --
> 2.7.4
>
Hi Pali,
Thank you for considering about my patch.
On 2021/07/18 9:26, Pali Rohar wrote:
> Hello Kunihiko!
>
> On Friday 23 April 2021 02:04:57 Kunihiko Hayashi wrote:
> > This patch adds misc interrupt handler to detect and invoke PME/AER event.
> >
> > In UniPhier PCIe controller, PME/AER signals are assigned to the same
> > signal as MSI by the internal logic. These signals should be detected by
> > the internal register, however, DWC MSI handler can't handle these signals.
> >
> > DWC MSI handler calls .msi_host_isr() callback function, that detects
> > PME/AER signals using the internal register and invokes the interrupt
> > with PME/AER IRQ numbers.
> >
> > These IRQ numbers is obtained by uniphier_pcie_port_get_irq() function,
> > that finds the device that matches PME/AER from the devices associated
> > with Root Port, and returns its IRQ number.
>
> If I understood this issue correctly, it means that your PCIe controller
> does not issue regular MSI interrupt for PME and AER events, but rather
> it issue controller specific interrupt and you need to figure out what
> kind of controller-specific event happened (e.g. PME or AER or something
> else).
Your view is almost correct.
This controller consists of Synopsys DWC and the glue logic, and regular
MSI interrupt is handled in dw_pcie_msi_isr() for DWC.
The interrupt for PME/AER event is issued to CPU as the same interrupt
as MSI, though, PME/AER event is detected by the glue logic instead of DWC.
So the regular MSI handler can't handle the interrupt for PME/AER event
directly.
> But if your controller supports PME or AER then it expose in its PCIe
> Root Port capabilities register MSI number for these PME and AER events.
> Kernel PCIe PME and AER drivers read from capabilities register these
> numbers and register irq functions to be called when interrupt happens.
Yes, the controller also has the MSI number for PME/AER in Root Port
capability register (defined as PCI_ERR_ROOT_AER_IRQ and PCI_EXP_FLAGS_IRQ).
These interrupts are registered with these capability values in
pcie_port_enable_irq_vec().
> So it means that you do not need to implement uniphier_pcie_port_get_irq
> function via this "ugly" foreach and call pcie_port_service_get_irq. But
> you can read this MSI interrupt number directly from your controller in
> this pcie-uniphier.c driver and then use irq_find_mapping() to convert
> hw MSI number to kernel's virq (used in generic_handle_irq()).
>
> Because currently you use in pcie-uniphier.c call to function
> pcie_port_service_get_irq() which returns cached interrupt number value
> which was read from PCIe Root Port capability register by PCI subsystem
> callbacked back to the pcie-uniphier.c driver.
>
> For me this looks like "ugly" if you need to do something in
> "complicated" way and add dependency e.g. on compile options like
> "if (!IS_ENABLED(CONFIG_PCIEAER) && !IS_ENABLED(CONFIG_PCIE_PME))" if it
> can be easily avoided.
>
> I'm writing this because I was solving exactly same problem for aardvark
> PCIe controller with PME, AER and HP interrupts (patches are on ML). So
> I think that this pcie-uniphier.c implementation can be simplified
> without need to use checks for CONFIG_* options and calling
> pcie_port_service_get_irq() in list_for_each_entry loop.
The interrupt for PME/AER event is detected by the glue logic.
When the handler needs to read the status register for PME/AER in the glue
logic and issue the correspond MSI interrupt using generic_handle_irq().
If the driver gets the MSI interrupt number directly like
pcie_message_numbers() function, I think this complicated method is
no longer necessary, too.
> Could you please post output of 'lspci -nn -vv'? In my opinion MSI
> numbers for AER and PME in Root Port could be constant so it may
> simplify implementation even more. (Just to note that in my case
> aardvark returns zero as MSI number and it is also documented in spec).
I already posted the lspci output[1].
[1]
https://lore.kernel.org/linux-pci/[email protected]/T/#e1145dab891debed1eadcddbf2b9f5fabb357f8b0
According to the spec, the initial MSI number for PME/AER is zero.
And this series up to v5 used fixed zero as the MSI number for PME/AER.
Thank you,
---
Best Regards
Kunihiko Hayashi