2018-04-20 05:29:42

by Honghui Zhang

Subject: [PATCH v6 0/2] PCI: mediatek: Fixups for the IRQ handle routine and MT7622's class code

From: Honghui Zhang <[email protected]>

Two fixups for mediatek's host bridge:
The first patch fixup class type and vendor ID for MT7622.
The second patch fixup the IRQ handle routine by using irq_chip solution
to avoid IRQ reentry which may exist for both MT2712 and MT7622.

Change since v5:
- Make the comments consistend with the code modification in the first patch.
- Using writew to performing a 16-bit write.
- Using irq_chip solution to fix the IRQ issue.

The v5 patchset could be found in:
https://patchwork.kernel.org/patch/10133303
https://patchwork.kernel.org/patch/10133305

Change since v4:
- Only setup vendor ID for MT7622, igorning the device ID since mediatek's
host bridge driver does not cares about the device ID.

Change since v3:
- Setup the class type and vendor ID at the beginning of startup instead
of in a quirk.
- Add mediatek's vendor ID, it could be found in:
https://pcisig.com/membership/member-companies?combine=&page=4

Change since v2:
- Move the initialize of the iterate before the loop to fix an
INTx IRQ issue in the first patch

Change since v1:
- Add the second patch.
- Make the first patch's commit message more standard.
Honghui Zhang (2):
PCI: mediatek: Set up vendor ID and class type for MT7622
PCI: mediatek: Using chained IRQ to setup IRQ handle

drivers/pci/host/pcie-mediatek.c | 220 +++++++++++++++++++++++----------------
include/linux/pci_ids.h | 2 +
2 files changed, 133 insertions(+), 89 deletions(-)

--
2.6.4

2018-04-20 05:29:24

by Honghui Zhang

Subject: [PATCH v6 1/2] PCI: mediatek: Set up vendor ID and class type for MT7622

From: Honghui Zhang <[email protected]>

MT7622's hardware default value of vendor ID and class type is not correct,
fix that by setup the correct values before linkup with Endpoint.

Signed-off-by: Honghui Zhang <[email protected]>
---
drivers/pci/host/pcie-mediatek.c | 30 +++++++++++++++++++++++++++---
include/linux/pci_ids.h | 2 ++
2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
index a8b20c5..c3dc549 100644
--- a/drivers/pci/host/pcie-mediatek.c
+++ b/drivers/pci/host/pcie-mediatek.c
@@ -66,6 +66,10 @@

/* PCIe V2 per-port registers */
#define PCIE_MSI_VECTOR 0x0c0
+
+#define PCIE_CONF_VEND_ID 0x100
+#define PCIE_CONF_CLASS_ID 0x106
+
#define PCIE_INT_MASK 0x420
#define INTX_MASK GENMASK(19, 16)
#define INTX_SHIFT 16
@@ -125,12 +129,14 @@ struct mtk_pcie_port;

/**
* struct mtk_pcie_soc - differentiate between host generations
+ * @need_fix_class_id: whether this host's class ID needed to be fixed or not
* @has_msi: whether this host supports MSI interrupts or not
* @ops: pointer to configuration access functions
* @startup: pointer to controller setting functions
* @setup_irq: pointer to initialize IRQ functions
*/
struct mtk_pcie_soc {
+ bool need_fix_class_id;
bool has_msi;
struct pci_ops *ops;
int (*startup)(struct mtk_pcie_port *port);
@@ -375,6 +381,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
{
struct mtk_pcie *pcie = port->pcie;
struct resource *mem = &pcie->mem;
+ const struct mtk_pcie_soc *soc = port->pcie->soc;
u32 val;
size_t size;
int err;
@@ -403,6 +410,15 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
PCIE_MAC_SRSTB | PCIE_CRSTB;
writel(val, port->base + PCIE_RST_CTRL);

+ /* Set up vendor ID and class code */
+ if (soc->need_fix_class_id) {
+ val = PCI_VENDOR_ID_MEDIATEK;
+ writew(val, port->base + PCIE_CONF_VEND_ID);
+
+ val = PCI_CLASS_BRIDGE_PCI;
+ writew(val, port->base + PCIE_CONF_CLASS_ID);
+ }
+
/* 100ms timeout value should be enough for Gen1/2 training */
err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
!!(val & PCIE_PORT_LINKUP_V2), 20,
@@ -1142,7 +1158,15 @@ static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
.startup = mtk_pcie_startup_port,
};

-static const struct mtk_pcie_soc mtk_pcie_soc_v2 = {
+static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
+ .has_msi = true,
+ .ops = &mtk_pcie_ops_v2,
+ .startup = mtk_pcie_startup_port_v2,
+ .setup_irq = mtk_pcie_setup_irq,
+};
+
+static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
+ .need_fix_class_id = true,
.has_msi = true,
.ops = &mtk_pcie_ops_v2,
.startup = mtk_pcie_startup_port_v2,
@@ -1152,8 +1176,8 @@ static const struct mtk_pcie_soc mtk_pcie_soc_v2 = {
static const struct of_device_id mtk_pcie_ids[] = {
{ .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 },
{ .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 },
- { .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_v2 },
- { .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_v2 },
+ { .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_mt2712 },
+ { .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_mt7622 },
{},
};

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index a6b3066..9d4fca5 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2115,6 +2115,8 @@

#define PCI_VENDOR_ID_MYRICOM 0x14c1

+#define PCI_VENDOR_ID_MEDIATEK 0x14c3
+
#define PCI_VENDOR_ID_TITAN 0x14D2
#define PCI_DEVICE_ID_TITAN_010L 0x8001
#define PCI_DEVICE_ID_TITAN_100L 0x8010
--
2.6.4

2018-04-20 05:29:24

by Honghui Zhang

Subject: [PATCH 2/2] PCI: mediatek: Using chained IRQ to setup IRQ handle

From: Honghui Zhang <[email protected]>

Using irq_chip solution to setup IRQs for the consistent with IRQ framework.

Signed-off-by: Honghui Zhang <[email protected]>
---
drivers/pci/host/pcie-mediatek.c | 192 +++++++++++++++++++++------------------
1 file changed, 105 insertions(+), 87 deletions(-)

diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
index c3dc549..1d9c6f1 100644
--- a/drivers/pci/host/pcie-mediatek.c
+++ b/drivers/pci/host/pcie-mediatek.c
@@ -11,8 +11,10 @@
#include <linux/delay.h>
#include <linux/iopoll.h>
#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
#include <linux/irqdomain.h>
#include <linux/kernel.h>
+#include <linux/msi.h>
#include <linux/of_address.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
@@ -130,14 +132,12 @@ struct mtk_pcie_port;
/**
* struct mtk_pcie_soc - differentiate between host generations
* @need_fix_class_id: whether this host's class ID needed to be fixed or not
- * @has_msi: whether this host supports MSI interrupts or not
* @ops: pointer to configuration access functions
* @startup: pointer to controller setting functions
* @setup_irq: pointer to initialize IRQ functions
*/
struct mtk_pcie_soc {
bool need_fix_class_id;
- bool has_msi;
struct pci_ops *ops;
int (*startup)(struct mtk_pcie_port *port);
int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
@@ -161,7 +161,9 @@ struct mtk_pcie_soc {
* @lane: lane count
* @slot: port slot
* @irq_domain: legacy INTx IRQ domain
+ * @inner_domain: inner IRQ domain
* @msi_domain: MSI IRQ domain
+ * @lock: protect the msi_irq_in_use bitmap
* @msi_irq_in_use: bit map for assigned MSI IRQ
*/
struct mtk_pcie_port {
@@ -179,7 +181,9 @@ struct mtk_pcie_port {
u32 lane;
u32 slot;
struct irq_domain *irq_domain;
+ struct irq_domain *inner_domain;
struct irq_domain *msi_domain;
+ struct mutex lock;
DECLARE_BITMAP(msi_irq_in_use, MTK_MSI_IRQS_NUM);
};

@@ -446,103 +450,122 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
return 0;
}

-static int mtk_pcie_msi_alloc(struct mtk_pcie_port *port)
+static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
{
- int msi;
+ struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
+ phys_addr_t addr;

- msi = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
- if (msi < MTK_MSI_IRQS_NUM)
- set_bit(msi, port->msi_irq_in_use);
- else
- return -ENOSPC;
+ /* MT2712/MT7622 only support 32-bit MSI addresses */
+ addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
+ msg->address_hi = 0;
+ msg->address_lo = lower_32_bits(addr);

- return msi;
+ msg->data = data->hwirq;
+
+ dev_dbg(port->pcie->dev, "msi#%d address_hi %#x address_lo %#x\n",
+ (int)data->hwirq, msg->address_hi, msg->address_lo);
}

-static void mtk_pcie_msi_free(struct mtk_pcie_port *port, unsigned long hwirq)
+static int mtk_msi_set_affinity(struct irq_data *irq_data,
+ const struct cpumask *mask, bool force)
{
- clear_bit(hwirq, port->msi_irq_in_use);
+ return -EINVAL;
}

-static int mtk_pcie_msi_setup_irq(struct msi_controller *chip,
- struct pci_dev *pdev, struct msi_desc *desc)
-{
- struct mtk_pcie_port *port;
- struct msi_msg msg;
- unsigned int irq;
- int hwirq;
- phys_addr_t msg_addr;
+static struct irq_chip mtk_msi_bottom_irq_chip = {
+ .name = "MTK MSI",
+ .irq_compose_msi_msg = mtk_compose_msi_msg,
+ .irq_set_affinity = mtk_msi_set_affinity,
+ .irq_mask = pci_msi_mask_irq,
+ .irq_unmask = pci_msi_unmask_irq,
+};

- port = mtk_pcie_find_port(pdev->bus, pdev->devfn);
- if (!port)
- return -EINVAL;
+static int mtk_pcie_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *args)
+{
+ struct mtk_pcie_port *port = domain->host_data;
+ unsigned long bit;

- hwirq = mtk_pcie_msi_alloc(port);
- if (hwirq < 0)
- return hwirq;
+ WARN_ON(nr_irqs != 1);
+ mutex_lock(&port->lock);

- irq = irq_create_mapping(port->msi_domain, hwirq);
- if (!irq) {
- mtk_pcie_msi_free(port, hwirq);
- return -EINVAL;
+ bit = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
+ if (bit >= MTK_MSI_IRQS_NUM) {
+ mutex_unlock(&port->lock);
+ return -ENOSPC;
}

- chip->dev = &pdev->dev;
-
- irq_set_msi_desc(irq, desc);
+ __set_bit(bit, port->msi_irq_in_use);

- /* MT2712/MT7622 only support 32-bit MSI addresses */
- msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
- msg.address_hi = 0;
- msg.address_lo = lower_32_bits(msg_addr);
- msg.data = hwirq;
+ mutex_unlock(&port->lock);

- pci_write_msi_msg(irq, &msg);
+ irq_domain_set_info(domain, virq, bit, &mtk_msi_bottom_irq_chip,
+ domain->host_data, handle_simple_irq,
+ NULL, NULL);

return 0;
}

-static void mtk_msi_teardown_irq(struct msi_controller *chip, unsigned int irq)
+static void mtk_pcie_irq_domain_free(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs)
{
- struct pci_dev *pdev = to_pci_dev(chip->dev);
- struct irq_data *d = irq_get_irq_data(irq);
- irq_hw_number_t hwirq = irqd_to_hwirq(d);
- struct mtk_pcie_port *port;
+ struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+ struct mtk_pcie_port *port = irq_data_get_irq_chip_data(d);

- port = mtk_pcie_find_port(pdev->bus, pdev->devfn);
- if (!port)
- return;
+ mutex_lock(&port->lock);
+
+ if (!test_bit(d->hwirq, port->msi_irq_in_use))
+ dev_err(port->pcie->dev, "trying to free unused MSI#%lu\n",
+ d->hwirq);
+ else
+ __clear_bit(d->hwirq, port->msi_irq_in_use);

- irq_dispose_mapping(irq);
- mtk_pcie_msi_free(port, hwirq);
+ mutex_unlock(&port->lock);
+
+ irq_domain_free_irqs_parent(domain, virq, nr_irqs);
}

-static struct msi_controller mtk_pcie_msi_chip = {
- .setup_irq = mtk_pcie_msi_setup_irq,
- .teardown_irq = mtk_msi_teardown_irq,
+static const struct irq_domain_ops msi_domain_ops = {
+ .alloc = mtk_pcie_irq_domain_alloc,
+ .free = mtk_pcie_irq_domain_free,
};

static struct irq_chip mtk_msi_irq_chip = {
.name = "MTK PCIe MSI",
- .irq_enable = pci_msi_unmask_irq,
- .irq_disable = pci_msi_mask_irq,
.irq_mask = pci_msi_mask_irq,
.irq_unmask = pci_msi_unmask_irq,
};

-static int mtk_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
- irq_hw_number_t hwirq)
+static struct msi_domain_info mtk_msi_domain_info = {
+ .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+ MSI_FLAG_PCI_MSIX),
+ .chip = &mtk_msi_irq_chip,
+};
+
+static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
{
- irq_set_chip_and_handler(irq, &mtk_msi_irq_chip, handle_simple_irq);
- irq_set_chip_data(irq, domain->host_data);
+ struct fwnode_handle *fwnode = of_node_to_fwnode(port->pcie->dev->of_node);
+
+ mutex_init(&port->lock);
+
+ port->inner_domain = irq_domain_create_linear(fwnode, MTK_MSI_IRQS_NUM,
+ &msi_domain_ops, port);
+ if (!port->inner_domain) {
+ dev_err(port->pcie->dev, "failed to create IRQ domain\n");
+ return -ENOMEM;
+ }
+
+ port->msi_domain = pci_msi_create_irq_domain(fwnode, &mtk_msi_domain_info,
+ port->inner_domain);
+ if (!port->msi_domain) {
+ dev_err(port->pcie->dev, "failed to create MSI domain\n");
+ irq_domain_remove(port->inner_domain);
+ return -ENOMEM;
+ }

return 0;
}

-static const struct irq_domain_ops msi_domain_ops = {
- .map = mtk_pcie_msi_map,
-};
-
static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
{
u32 val;
@@ -575,6 +598,7 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
{
struct device *dev = port->pcie->dev;
struct device_node *pcie_intc_node;
+ int ret;

/* Setup INTx */
pcie_intc_node = of_get_next_child(node, NULL);
@@ -591,27 +615,28 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
}

if (IS_ENABLED(CONFIG_PCI_MSI)) {
- port->msi_domain = irq_domain_add_linear(node, MTK_MSI_IRQS_NUM,
- &msi_domain_ops,
- &mtk_pcie_msi_chip);
- if (!port->msi_domain) {
- dev_err(dev, "failed to create MSI IRQ domain\n");
- return -ENODEV;
- }
+ ret = mtk_pcie_allocate_msi_domains(port);
+ if (ret)
+ return ret;
+
mtk_pcie_enable_msi(port);
}

return 0;
}

-static irqreturn_t mtk_pcie_intr_handler(int irq, void *data)
+static void mtk_pcie_intr_handler(struct irq_desc *desc)
{
- struct mtk_pcie_port *port = (struct mtk_pcie_port *)data;
+ struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
+ struct irq_chip *irqchip = irq_desc_get_chip(desc);
unsigned long status;
u32 virq;
u32 bit = INTX_SHIFT;

- while ((status = readl(port->base + PCIE_INT_STATUS)) & INTX_MASK) {
+ chained_irq_enter(irqchip, desc);
+
+ status = readl(port->base + PCIE_INT_STATUS);
+ if (status & INTX_MASK) {
for_each_set_bit_from(bit, &status, PCI_NUM_INTX + INTX_SHIFT) {
/* Clear the INTx */
writel(1 << bit, port->base + PCIE_INT_STATUS);
@@ -622,14 +647,14 @@ static irqreturn_t mtk_pcie_intr_handler(int irq, void *data)
}

if (IS_ENABLED(CONFIG_PCI_MSI)) {
- while ((status = readl(port->base + PCIE_INT_STATUS)) & MSI_STATUS) {
+ if (status & MSI_STATUS) {
unsigned long imsi_status;

while ((imsi_status = readl(port->base + PCIE_IMSI_STATUS))) {
for_each_set_bit(bit, &imsi_status, MTK_MSI_IRQS_NUM) {
/* Clear the MSI */
writel(1 << bit, port->base + PCIE_IMSI_STATUS);
- virq = irq_find_mapping(port->msi_domain, bit);
+ virq = irq_find_mapping(port->inner_domain, bit);
generic_handle_irq(virq);
}
}
@@ -638,7 +663,9 @@ static irqreturn_t mtk_pcie_intr_handler(int irq, void *data)
}
}

- return IRQ_HANDLED;
+ chained_irq_exit(irqchip, desc);
+
+ return;
}

static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
@@ -649,20 +676,15 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
struct platform_device *pdev = to_platform_device(dev);
int err, irq;

- irq = platform_get_irq(pdev, port->slot);
- err = devm_request_irq(dev, irq, mtk_pcie_intr_handler,
- IRQF_SHARED, "mtk-pcie", port);
- if (err) {
- dev_err(dev, "unable to request IRQ %d\n", irq);
- return err;
- }
-
err = mtk_pcie_init_irq_domain(port, node);
if (err) {
dev_err(dev, "failed to init PCIe IRQ domain\n");
return err;
}

+ irq = platform_get_irq(pdev, port->slot);
+ irq_set_chained_handler_and_data(irq, mtk_pcie_intr_handler, port);
+
return 0;
}

@@ -1096,8 +1118,6 @@ static int mtk_pcie_register_host(struct pci_host_bridge *host)
host->map_irq = of_irq_parse_and_map_pci;
host->swizzle_irq = pci_common_swizzle;
host->sysdata = pcie;
- if (IS_ENABLED(CONFIG_PCI_MSI) && pcie->soc->has_msi)
- host->msi = &mtk_pcie_msi_chip;

err = pci_scan_root_bus_bridge(host);
if (err < 0)
@@ -1159,7 +1179,6 @@ static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
};

static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
- .has_msi = true,
.ops = &mtk_pcie_ops_v2,
.startup = mtk_pcie_startup_port_v2,
.setup_irq = mtk_pcie_setup_irq,
@@ -1167,7 +1186,6 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {

static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
.need_fix_class_id = true,
- .has_msi = true,
.ops = &mtk_pcie_ops_v2,
.startup = mtk_pcie_startup_port_v2,
.setup_irq = mtk_pcie_setup_irq,
--
2.6.4

2018-04-30 02:03:20

by Ryder Lee

Subject: Re: [PATCH v6 0/2] PCI: mediatek: Fixups for the IRQ handle routine and MT7622's class code

On Fri, 2018-04-20 at 13:25 +0800, Honghui Zhang (张洪辉) wrote:
> From: Honghui Zhang <[email protected]>
>
> Two fixups for mediatek's host bridge:
> The first patch fixup class type and vendor ID for MT7622.
> The second patch fixup the IRQ handle routine by using irq_chip solution
> to avoid IRQ reentry which may exist for both MT2712 and MT7622.
>
> Change since v5:
> - Make the comments consistend with the code modification in the first patch.
> - Using writew to performing a 16-bit write.
> - Using irq_chip solution to fix the IRQ issue.
>
> The v5 patchset could be found in:
> https://patchwork.kernel.org/patch/10133303
> https://patchwork.kernel.org/patch/10133305
>
> Change since v4:
> - Only setup vendor ID for MT7622, igorning the device ID since mediatek's
> host bridge driver does not cares about the device ID.
>
> Change since v3:
> - Setup the class type and vendor ID at the beginning of startup instead
> of in a quirk.
> - Add mediatek's vendor ID, it could be found in:
> https://pcisig.com/membership/member-companies?combine=&page=4
>
> Change since v2:
> - Move the initialize of the iterate before the loop to fix an
> INTx IRQ issue in the first patch
>
> Change since v1:
> - Add the second patch.
> - Make the first patch's commit message more standard.
> Honghui Zhang (2):
> PCI: mediatek: Set up vendor ID and class type for MT7622
> PCI: mediatek: Using chained IRQ to setup IRQ handle
>
> drivers/pci/host/pcie-mediatek.c | 220 +++++++++++++++++++++++----------------
> include/linux/pci_ids.h | 2 +
> 2 files changed, 133 insertions(+), 89 deletions(-)


Acked-by: Ryder Lee <[email protected]> for the series.

Thanks

2018-04-30 11:03:44

by Marc Zyngier

Subject: Re: [PATCH 2/2] PCI: mediatek: Using chained IRQ to setup IRQ handle

Hi Zhang,

On 20/04/18 06:25, [email protected] wrote:
> From: Honghui Zhang <[email protected]>
>
> Using irq_chip solution to setup IRQs for the consistent with IRQ framework.
>
> Signed-off-by: Honghui Zhang <[email protected]>
> ---
> drivers/pci/host/pcie-mediatek.c | 192 +++++++++++++++++++++------------------
> 1 file changed, 105 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
> index c3dc549..1d9c6f1 100644
> --- a/drivers/pci/host/pcie-mediatek.c
> +++ b/drivers/pci/host/pcie-mediatek.c
> @@ -11,8 +11,10 @@
> #include <linux/delay.h>
> #include <linux/iopoll.h>
> #include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> #include <linux/irqdomain.h>
> #include <linux/kernel.h>
> +#include <linux/msi.h>
> #include <linux/of_address.h>
> #include <linux/of_pci.h>
> #include <linux/of_platform.h>
> @@ -130,14 +132,12 @@ struct mtk_pcie_port;
> /**
> * struct mtk_pcie_soc - differentiate between host generations
> * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> - * @has_msi: whether this host supports MSI interrupts or not
> * @ops: pointer to configuration access functions
> * @startup: pointer to controller setting functions
> * @setup_irq: pointer to initialize IRQ functions
> */
> struct mtk_pcie_soc {
> bool need_fix_class_id;
> - bool has_msi;
> struct pci_ops *ops;
> int (*startup)(struct mtk_pcie_port *port);
> int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> @@ -161,7 +161,9 @@ struct mtk_pcie_soc {
> * @lane: lane count
> * @slot: port slot
> * @irq_domain: legacy INTx IRQ domain
> + * @inner_domain: inner IRQ domain
> * @msi_domain: MSI IRQ domain
> + * @lock: protect the msi_irq_in_use bitmap
> * @msi_irq_in_use: bit map for assigned MSI IRQ
> */
> struct mtk_pcie_port {
> @@ -179,7 +181,9 @@ struct mtk_pcie_port {
> u32 lane;
> u32 slot;
> struct irq_domain *irq_domain;
> + struct irq_domain *inner_domain;
> struct irq_domain *msi_domain;
> + struct mutex lock;
> DECLARE_BITMAP(msi_irq_in_use, MTK_MSI_IRQS_NUM);
> };
>
> @@ -446,103 +450,122 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> return 0;
> }
>
> -static int mtk_pcie_msi_alloc(struct mtk_pcie_port *port)
> +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> {
> - int msi;
> + struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> + phys_addr_t addr;
>
> - msi = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
> - if (msi < MTK_MSI_IRQS_NUM)
> - set_bit(msi, port->msi_irq_in_use);
> - else
> - return -ENOSPC;
> + /* MT2712/MT7622 only support 32-bit MSI addresses */
> + addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> + msg->address_hi = 0;
> + msg->address_lo = lower_32_bits(addr);
>
> - return msi;
> + msg->data = data->hwirq;
> +
> + dev_dbg(port->pcie->dev, "msi#%d address_hi %#x address_lo %#x\n",
> + (int)data->hwirq, msg->address_hi, msg->address_lo);
> }
>
> -static void mtk_pcie_msi_free(struct mtk_pcie_port *port, unsigned long hwirq)
> +static int mtk_msi_set_affinity(struct irq_data *irq_data,
> + const struct cpumask *mask, bool force)
> {
> - clear_bit(hwirq, port->msi_irq_in_use);
> + return -EINVAL;
> }
>
> -static int mtk_pcie_msi_setup_irq(struct msi_controller *chip,
> - struct pci_dev *pdev, struct msi_desc *desc)
> -{
> - struct mtk_pcie_port *port;
> - struct msi_msg msg;
> - unsigned int irq;
> - int hwirq;
> - phys_addr_t msg_addr;
> +static struct irq_chip mtk_msi_bottom_irq_chip = {
> + .name = "MTK MSI",
> + .irq_compose_msi_msg = mtk_compose_msi_msg,
> + .irq_set_affinity = mtk_msi_set_affinity,
> + .irq_mask = pci_msi_mask_irq,
> + .irq_unmask = pci_msi_unmask_irq,
> +};
>
> - port = mtk_pcie_find_port(pdev->bus, pdev->devfn);
> - if (!port)
> - return -EINVAL;
> +static int mtk_pcie_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs, void *args)
> +{
> + struct mtk_pcie_port *port = domain->host_data;
> + unsigned long bit;
>
> - hwirq = mtk_pcie_msi_alloc(port);
> - if (hwirq < 0)
> - return hwirq;
> + WARN_ON(nr_irqs != 1);
> + mutex_lock(&port->lock);
>
> - irq = irq_create_mapping(port->msi_domain, hwirq);
> - if (!irq) {
> - mtk_pcie_msi_free(port, hwirq);
> - return -EINVAL;
> + bit = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
> + if (bit >= MTK_MSI_IRQS_NUM) {
> + mutex_unlock(&port->lock);
> + return -ENOSPC;
> }
>
> - chip->dev = &pdev->dev;
> -
> - irq_set_msi_desc(irq, desc);
> + __set_bit(bit, port->msi_irq_in_use);
>
> - /* MT2712/MT7622 only support 32-bit MSI addresses */
> - msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> - msg.address_hi = 0;
> - msg.address_lo = lower_32_bits(msg_addr);
> - msg.data = hwirq;
> + mutex_unlock(&port->lock);
>
> - pci_write_msi_msg(irq, &msg);
> + irq_domain_set_info(domain, virq, bit, &mtk_msi_bottom_irq_chip,
> + domain->host_data, handle_simple_irq,

Why don't you handle the interrupt as an edge interrupt
(handle_edge_irq)? That's what it really is, and the fact that you use
"handle_simple_irq" makes me think that wou're papering over some other
issue (see below).

> + NULL, NULL);
>
> return 0;
> }
>
> -static void mtk_msi_teardown_irq(struct msi_controller *chip, unsigned int irq)
> +static void mtk_pcie_irq_domain_free(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs)
> {
> - struct pci_dev *pdev = to_pci_dev(chip->dev);
> - struct irq_data *d = irq_get_irq_data(irq);
> - irq_hw_number_t hwirq = irqd_to_hwirq(d);
> - struct mtk_pcie_port *port;
> + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> + struct mtk_pcie_port *port = irq_data_get_irq_chip_data(d);
>
> - port = mtk_pcie_find_port(pdev->bus, pdev->devfn);
> - if (!port)
> - return;
> + mutex_lock(&port->lock);
> +
> + if (!test_bit(d->hwirq, port->msi_irq_in_use))
> + dev_err(port->pcie->dev, "trying to free unused MSI#%lu\n",
> + d->hwirq);
> + else
> + __clear_bit(d->hwirq, port->msi_irq_in_use);
>
> - irq_dispose_mapping(irq);
> - mtk_pcie_msi_free(port, hwirq);
> + mutex_unlock(&port->lock);
> +
> + irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> }
>
> -static struct msi_controller mtk_pcie_msi_chip = {
> - .setup_irq = mtk_pcie_msi_setup_irq,
> - .teardown_irq = mtk_msi_teardown_irq,
> +static const struct irq_domain_ops msi_domain_ops = {
> + .alloc = mtk_pcie_irq_domain_alloc,
> + .free = mtk_pcie_irq_domain_free,
> };
>
> static struct irq_chip mtk_msi_irq_chip = {
> .name = "MTK PCIe MSI",
> - .irq_enable = pci_msi_unmask_irq,
> - .irq_disable = pci_msi_mask_irq,
> .irq_mask = pci_msi_mask_irq,
> .irq_unmask = pci_msi_unmask_irq,
> };
>
> -static int mtk_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
> - irq_hw_number_t hwirq)
> +static struct msi_domain_info mtk_msi_domain_info = {
> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> + MSI_FLAG_PCI_MSIX),
> + .chip = &mtk_msi_irq_chip,
> +};
> +
> +static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
> {
> - irq_set_chip_and_handler(irq, &mtk_msi_irq_chip, handle_simple_irq);
> - irq_set_chip_data(irq, domain->host_data);
> + struct fwnode_handle *fwnode = of_node_to_fwnode(port->pcie->dev->of_node);
> +
> + mutex_init(&port->lock);
> +
> + port->inner_domain = irq_domain_create_linear(fwnode, MTK_MSI_IRQS_NUM,
> + &msi_domain_ops, port);
> + if (!port->inner_domain) {
> + dev_err(port->pcie->dev, "failed to create IRQ domain\n");
> + return -ENOMEM;
> + }
> +
> + port->msi_domain = pci_msi_create_irq_domain(fwnode, &mtk_msi_domain_info,
> + port->inner_domain);
> + if (!port->msi_domain) {
> + dev_err(port->pcie->dev, "failed to create MSI domain\n");
> + irq_domain_remove(port->inner_domain);
> + return -ENOMEM;
> + }
>
> return 0;
> }
>
> -static const struct irq_domain_ops msi_domain_ops = {
> - .map = mtk_pcie_msi_map,
> -};
> -
> static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
> {
> u32 val;
> @@ -575,6 +598,7 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
> {
> struct device *dev = port->pcie->dev;
> struct device_node *pcie_intc_node;
> + int ret;
>
> /* Setup INTx */
> pcie_intc_node = of_get_next_child(node, NULL);
> @@ -591,27 +615,28 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
> }
>
> if (IS_ENABLED(CONFIG_PCI_MSI)) {
> - port->msi_domain = irq_domain_add_linear(node, MTK_MSI_IRQS_NUM,
> - &msi_domain_ops,
> - &mtk_pcie_msi_chip);
> - if (!port->msi_domain) {
> - dev_err(dev, "failed to create MSI IRQ domain\n");
> - return -ENODEV;
> - }
> + ret = mtk_pcie_allocate_msi_domains(port);
> + if (ret)
> + return ret;
> +
> mtk_pcie_enable_msi(port);
> }
>
> return 0;
> }
>
> -static irqreturn_t mtk_pcie_intr_handler(int irq, void *data)
> +static void mtk_pcie_intr_handler(struct irq_desc *desc)
> {
> - struct mtk_pcie_port *port = (struct mtk_pcie_port *)data;
> + struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
> + struct irq_chip *irqchip = irq_desc_get_chip(desc);
> unsigned long status;
> u32 virq;
> u32 bit = INTX_SHIFT;
>
> - while ((status = readl(port->base + PCIE_INT_STATUS)) & INTX_MASK) {
> + chained_irq_enter(irqchip, desc);
> +
> + status = readl(port->base + PCIE_INT_STATUS);
> + if (status & INTX_MASK) {
> for_each_set_bit_from(bit, &status, PCI_NUM_INTX + INTX_SHIFT) {
> /* Clear the INTx */
> writel(1 << bit, port->base + PCIE_INT_STATUS);
> @@ -622,14 +647,14 @@ static irqreturn_t mtk_pcie_intr_handler(int irq, void *data)
> }
>
> if (IS_ENABLED(CONFIG_PCI_MSI)) {
> - while ((status = readl(port->base + PCIE_INT_STATUS)) & MSI_STATUS) {
> + if (status & MSI_STATUS) {
> unsigned long imsi_status;
>
> while ((imsi_status = readl(port->base + PCIE_IMSI_STATUS))) {
> for_each_set_bit(bit, &imsi_status, MTK_MSI_IRQS_NUM) {
> /* Clear the MSI */
> writel(1 << bit, port->base + PCIE_IMSI_STATUS);

This write suspiciously looks like an "ack" operation on the controller.
Why isn't it part of the irqchip as a first class method?

> - virq = irq_find_mapping(port->msi_domain, bit);
> + virq = irq_find_mapping(port->inner_domain, bit);
> generic_handle_irq(virq);
> }
> }
> @@ -638,7 +663,9 @@ static irqreturn_t mtk_pcie_intr_handler(int irq, void *data)
> }
> }
>
> - return IRQ_HANDLED;
> + chained_irq_exit(irqchip, desc);
> +
> + return;
> }
>
> static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
> @@ -649,20 +676,15 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
> struct platform_device *pdev = to_platform_device(dev);
> int err, irq;
>
> - irq = platform_get_irq(pdev, port->slot);
> - err = devm_request_irq(dev, irq, mtk_pcie_intr_handler,
> - IRQF_SHARED, "mtk-pcie", port);
> - if (err) {
> - dev_err(dev, "unable to request IRQ %d\n", irq);
> - return err;
> - }
> -
> err = mtk_pcie_init_irq_domain(port, node);
> if (err) {
> dev_err(dev, "failed to init PCIe IRQ domain\n");
> return err;
> }
>
> + irq = platform_get_irq(pdev, port->slot);
> + irq_set_chained_handler_and_data(irq, mtk_pcie_intr_handler, port);
> +
> return 0;
> }
>
> @@ -1096,8 +1118,6 @@ static int mtk_pcie_register_host(struct pci_host_bridge *host)
> host->map_irq = of_irq_parse_and_map_pci;
> host->swizzle_irq = pci_common_swizzle;
> host->sysdata = pcie;
> - if (IS_ENABLED(CONFIG_PCI_MSI) && pcie->soc->has_msi)
> - host->msi = &mtk_pcie_msi_chip;
>
> err = pci_scan_root_bus_bridge(host);
> if (err < 0)
> @@ -1159,7 +1179,6 @@ static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> };
>
> static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> - .has_msi = true,
> .ops = &mtk_pcie_ops_v2,
> .startup = mtk_pcie_startup_port_v2,
> .setup_irq = mtk_pcie_setup_irq,
> @@ -1167,7 +1186,6 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
>
> static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> .need_fix_class_id = true,
> - .has_msi = true,
> .ops = &mtk_pcie_ops_v2,
> .startup = mtk_pcie_startup_port_v2,
> .setup_irq = mtk_pcie_setup_irq,
>

Thanks,

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

2018-05-02 10:12:13

by Marc Zyngier

Subject: Re: [PATCH 2/2] PCI: mediatek: Using chained IRQ to setup IRQ handle

On 02/05/18 10:41, Honghui Zhang wrote:
> On Mon, 2018-04-30 at 12:03 +0100, Marc Zyngier wrote:
>> Hi Zhang,
>>
>> On 20/04/18 06:25, [email protected] wrote:
>>> From: Honghui Zhang <[email protected]>
>>>
>>> Using irq_chip solution to setup IRQs for the consistent with IRQ framework.
>>>
>>> Signed-off-by: Honghui Zhang <[email protected]>
>>> ---
>>> drivers/pci/host/pcie-mediatek.c | 192 +++++++++++++++++++++------------------
>>> 1 file changed, 105 insertions(+), 87 deletions(-)
>>>
>>> diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
>>> index c3dc549..1d9c6f1 100644
>>> --- a/drivers/pci/host/pcie-mediatek.c
>>> +++ b/drivers/pci/host/pcie-mediatek.c
>>> @@ -11,8 +11,10 @@
>>> #include <linux/delay.h>
>>> #include <linux/iopoll.h>
>>> #include <linux/irq.h>
>>> +#include <linux/irqchip/chained_irq.h>
>>> #include <linux/irqdomain.h>
>>> #include <linux/kernel.h>
>>> +#include <linux/msi.h>
>>> #include <linux/of_address.h>
>>> #include <linux/of_pci.h>
>>> #include <linux/of_platform.h>
>>> @@ -130,14 +132,12 @@ struct mtk_pcie_port;
>>> /**
>>> * struct mtk_pcie_soc - differentiate between host generations
>>> * @need_fix_class_id: whether this host's class ID needed to be fixed or not
>>> - * @has_msi: whether this host supports MSI interrupts or not
>>> * @ops: pointer to configuration access functions
>>> * @startup: pointer to controller setting functions
>>> * @setup_irq: pointer to initialize IRQ functions
>>> */
>>> struct mtk_pcie_soc {
>>> bool need_fix_class_id;
>>> - bool has_msi;
>>> struct pci_ops *ops;
>>> int (*startup)(struct mtk_pcie_port *port);
>>> int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
>>> @@ -161,7 +161,9 @@ struct mtk_pcie_soc {
>>> * @lane: lane count
>>> * @slot: port slot
>>> * @irq_domain: legacy INTx IRQ domain
>>> + * @inner_domain: inner IRQ domain
>>> * @msi_domain: MSI IRQ domain
>>> + * @lock: protect the msi_irq_in_use bitmap
>>> * @msi_irq_in_use: bit map for assigned MSI IRQ
>>> */
>>> struct mtk_pcie_port {
>>> @@ -179,7 +181,9 @@ struct mtk_pcie_port {
>>> u32 lane;
>>> u32 slot;
>>> struct irq_domain *irq_domain;
>>> + struct irq_domain *inner_domain;
>>> struct irq_domain *msi_domain;
>>> + struct mutex lock;
>>> DECLARE_BITMAP(msi_irq_in_use, MTK_MSI_IRQS_NUM);
>>> };
>>>
>>> @@ -446,103 +450,122 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
>>> return 0;
>>> }
>>>
>>> -static int mtk_pcie_msi_alloc(struct mtk_pcie_port *port)
>>> +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>>> {
>>> - int msi;
>>> + struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
>>> + phys_addr_t addr;
>>>
>>> - msi = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
>>> - if (msi < MTK_MSI_IRQS_NUM)
>>> - set_bit(msi, port->msi_irq_in_use);
>>> - else
>>> - return -ENOSPC;
>>> + /* MT2712/MT7622 only support 32-bit MSI addresses */
>>> + addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
>>> + msg->address_hi = 0;
>>> + msg->address_lo = lower_32_bits(addr);
>>>
>>> - return msi;
>>> + msg->data = data->hwirq;
>>> +
>>> + dev_dbg(port->pcie->dev, "msi#%d address_hi %#x address_lo %#x\n",
>>> + (int)data->hwirq, msg->address_hi, msg->address_lo);
>>> }
>>>
>>> -static void mtk_pcie_msi_free(struct mtk_pcie_port *port, unsigned long hwirq)
>>> +static int mtk_msi_set_affinity(struct irq_data *irq_data,
>>> + const struct cpumask *mask, bool force)
>>> {
>>> - clear_bit(hwirq, port->msi_irq_in_use);
>>> + return -EINVAL;
>>> }
>>>
>>> -static int mtk_pcie_msi_setup_irq(struct msi_controller *chip,
>>> - struct pci_dev *pdev, struct msi_desc *desc)
>>> -{
>>> - struct mtk_pcie_port *port;
>>> - struct msi_msg msg;
>>> - unsigned int irq;
>>> - int hwirq;
>>> - phys_addr_t msg_addr;
>>> +static struct irq_chip mtk_msi_bottom_irq_chip = {
>>> + .name = "MTK MSI",
>>> + .irq_compose_msi_msg = mtk_compose_msi_msg,
>>> + .irq_set_affinity = mtk_msi_set_affinity,
>>> + .irq_mask = pci_msi_mask_irq,
>>> + .irq_unmask = pci_msi_unmask_irq,
>>> +};
>>>
>>> - port = mtk_pcie_find_port(pdev->bus, pdev->devfn);
>>> - if (!port)
>>> - return -EINVAL;
>>> +static int mtk_pcie_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>>> + unsigned int nr_irqs, void *args)
>>> +{
>>> + struct mtk_pcie_port *port = domain->host_data;
>>> + unsigned long bit;
>>>
>>> - hwirq = mtk_pcie_msi_alloc(port);
>>> - if (hwirq < 0)
>>> - return hwirq;
>>> + WARN_ON(nr_irqs != 1);
>>> + mutex_lock(&port->lock);
>>>
>>> - irq = irq_create_mapping(port->msi_domain, hwirq);
>>> - if (!irq) {
>>> - mtk_pcie_msi_free(port, hwirq);
>>> - return -EINVAL;
>>> + bit = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
>>> + if (bit >= MTK_MSI_IRQS_NUM) {
>>> + mutex_unlock(&port->lock);
>>> + return -ENOSPC;
>>> }
>>>
>>> - chip->dev = &pdev->dev;
>>> -
>>> - irq_set_msi_desc(irq, desc);
>>> + __set_bit(bit, port->msi_irq_in_use);
>>>
>>> - /* MT2712/MT7622 only support 32-bit MSI addresses */
>>> - msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
>>> - msg.address_hi = 0;
>>> - msg.address_lo = lower_32_bits(msg_addr);
>>> - msg.data = hwirq;
>>> + mutex_unlock(&port->lock);
>>>
>>> - pci_write_msi_msg(irq, &msg);
>>> + irq_domain_set_info(domain, virq, bit, &mtk_msi_bottom_irq_chip,
>>> + domain->host_data, handle_simple_irq,
>>
>> Why don't you handle the interrupt as an edge interrupt
>> (handle_edge_irq)? That's what it really is, and the fact that you use
>> "handle_simple_irq" makes me think that wou're papering over some other
>> issue (see below).
>
> Hi, Marc,
>
> Thanks very much for your comments.
>
> I guess the MSI irq is more like a level IRQ instead of edge IRQ.
> When a MSI IRQ arrived, the corresponding bit of MSI status register
> will be asserted and stay asserted until it's been cleared by software.

There is some confusion here. The MSI itself is always edge-triggered.
That's by definition, and you cannot change it. The interrupt that is
used to signal that MSIs are pending (the multiplexing interrupt) can
itself be level triggered (and goes low once all the MSIs have been
acknowledged).

> I will try to using handle_level_irq and add some callbacks of irq_ack
> in the next version.

The interrupt above seem to be the MSI itself, and not the multiplexing
interrupt. It should be handled as an edge.

If that doesn't work, we need to understand why.

Thanks,

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

2018-05-02 10:54:33

by Honghui Zhang

Subject: Re: [PATCH 2/2] PCI: mediatek: Using chained IRQ to setup IRQ handle

On Mon, 2018-04-30 at 12:03 +0100, Marc Zyngier wrote:
> Hi Zhang,
>
> On 20/04/18 06:25, [email protected] wrote:
> > From: Honghui Zhang <[email protected]>
> >
> > Using irq_chip solution to setup IRQs for the consistent with IRQ framework.
> >
> > Signed-off-by: Honghui Zhang <[email protected]>
> > ---
> > drivers/pci/host/pcie-mediatek.c | 192 +++++++++++++++++++++------------------
> > 1 file changed, 105 insertions(+), 87 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
> > index c3dc549..1d9c6f1 100644
> > --- a/drivers/pci/host/pcie-mediatek.c
> > +++ b/drivers/pci/host/pcie-mediatek.c
> > @@ -11,8 +11,10 @@
> > #include <linux/delay.h>
> > #include <linux/iopoll.h>
> > #include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h>
> > #include <linux/irqdomain.h>
> > #include <linux/kernel.h>
> > +#include <linux/msi.h>
> > #include <linux/of_address.h>
> > #include <linux/of_pci.h>
> > #include <linux/of_platform.h>
> > @@ -130,14 +132,12 @@ struct mtk_pcie_port;
> > /**
> > * struct mtk_pcie_soc - differentiate between host generations
> > * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> > - * @has_msi: whether this host supports MSI interrupts or not
> > * @ops: pointer to configuration access functions
> > * @startup: pointer to controller setting functions
> > * @setup_irq: pointer to initialize IRQ functions
> > */
> > struct mtk_pcie_soc {
> > bool need_fix_class_id;
> > - bool has_msi;
> > struct pci_ops *ops;
> > int (*startup)(struct mtk_pcie_port *port);
> > int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> > @@ -161,7 +161,9 @@ struct mtk_pcie_soc {
> > * @lane: lane count
> > * @slot: port slot
> > * @irq_domain: legacy INTx IRQ domain
> > + * @inner_domain: inner IRQ domain
> > * @msi_domain: MSI IRQ domain
> > + * @lock: protect the msi_irq_in_use bitmap
> > * @msi_irq_in_use: bit map for assigned MSI IRQ
> > */
> > struct mtk_pcie_port {
> > @@ -179,7 +181,9 @@ struct mtk_pcie_port {
> > u32 lane;
> > u32 slot;
> > struct irq_domain *irq_domain;
> > + struct irq_domain *inner_domain;
> > struct irq_domain *msi_domain;
> > + struct mutex lock;
> > DECLARE_BITMAP(msi_irq_in_use, MTK_MSI_IRQS_NUM);
> > };
> >
> > @@ -446,103 +450,122 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> > return 0;
> > }
> >
> > -static int mtk_pcie_msi_alloc(struct mtk_pcie_port *port)
> > +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> > {
> > - int msi;
> > + struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> > + phys_addr_t addr;
> >
> > - msi = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
> > - if (msi < MTK_MSI_IRQS_NUM)
> > - set_bit(msi, port->msi_irq_in_use);
> > - else
> > - return -ENOSPC;
> > + /* MT2712/MT7622 only support 32-bit MSI addresses */
> > + addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> > + msg->address_hi = 0;
> > + msg->address_lo = lower_32_bits(addr);
> >
> > - return msi;
> > + msg->data = data->hwirq;
> > +
> > + dev_dbg(port->pcie->dev, "msi#%d address_hi %#x address_lo %#x\n",
> > + (int)data->hwirq, msg->address_hi, msg->address_lo);
> > }
> >
> > -static void mtk_pcie_msi_free(struct mtk_pcie_port *port, unsigned long hwirq)
> > +static int mtk_msi_set_affinity(struct irq_data *irq_data,
> > + const struct cpumask *mask, bool force)
> > {
> > - clear_bit(hwirq, port->msi_irq_in_use);
> > + return -EINVAL;
> > }
> >
> > -static int mtk_pcie_msi_setup_irq(struct msi_controller *chip,
> > - struct pci_dev *pdev, struct msi_desc *desc)
> > -{
> > - struct mtk_pcie_port *port;
> > - struct msi_msg msg;
> > - unsigned int irq;
> > - int hwirq;
> > - phys_addr_t msg_addr;
> > +static struct irq_chip mtk_msi_bottom_irq_chip = {
> > + .name = "MTK MSI",
> > + .irq_compose_msi_msg = mtk_compose_msi_msg,
> > + .irq_set_affinity = mtk_msi_set_affinity,
> > + .irq_mask = pci_msi_mask_irq,
> > + .irq_unmask = pci_msi_unmask_irq,
> > +};
> >
> > - port = mtk_pcie_find_port(pdev->bus, pdev->devfn);
> > - if (!port)
> > - return -EINVAL;
> > +static int mtk_pcie_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > + unsigned int nr_irqs, void *args)
> > +{
> > + struct mtk_pcie_port *port = domain->host_data;
> > + unsigned long bit;
> >
> > - hwirq = mtk_pcie_msi_alloc(port);
> > - if (hwirq < 0)
> > - return hwirq;
> > + WARN_ON(nr_irqs != 1);
> > + mutex_lock(&port->lock);
> >
> > - irq = irq_create_mapping(port->msi_domain, hwirq);
> > - if (!irq) {
> > - mtk_pcie_msi_free(port, hwirq);
> > - return -EINVAL;
> > + bit = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
> > + if (bit >= MTK_MSI_IRQS_NUM) {
> > + mutex_unlock(&port->lock);
> > + return -ENOSPC;
> > }
> >
> > - chip->dev = &pdev->dev;
> > -
> > - irq_set_msi_desc(irq, desc);
> > + __set_bit(bit, port->msi_irq_in_use);
> >
> > - /* MT2712/MT7622 only support 32-bit MSI addresses */
> > - msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> > - msg.address_hi = 0;
> > - msg.address_lo = lower_32_bits(msg_addr);
> > - msg.data = hwirq;
> > + mutex_unlock(&port->lock);
> >
> > - pci_write_msi_msg(irq, &msg);
> > + irq_domain_set_info(domain, virq, bit, &mtk_msi_bottom_irq_chip,
> > + domain->host_data, handle_simple_irq,
>
> Why don't you handle the interrupt as an edge interrupt
> (handle_edge_irq)? That's what it really is, and the fact that you use
> "handle_simple_irq" makes me think that wou're papering over some other
> issue (see below).

Hi, Marc,

Thanks very much for your comments.

I guess the MSI irq is more like a level IRQ instead of edge IRQ.
When a MSI IRQ arrived, the corresponding bit of MSI status register
will be asserted and stay asserted until it's been cleared by software.

I will try to using handle_level_irq and add some callbacks of irq_ack
in the next version.

thanks.

>
> > + NULL, NULL);
> >
> > return 0;
> > }
> >
> > -static void mtk_msi_teardown_irq(struct msi_controller *chip, unsigned int irq)
> > +static void mtk_pcie_irq_domain_free(struct irq_domain *domain,
> > + unsigned int virq, unsigned int nr_irqs)
> > {
> > - struct pci_dev *pdev = to_pci_dev(chip->dev);
> > - struct irq_data *d = irq_get_irq_data(irq);
> > - irq_hw_number_t hwirq = irqd_to_hwirq(d);
> > - struct mtk_pcie_port *port;
> > + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > + struct mtk_pcie_port *port = irq_data_get_irq_chip_data(d);
> >
> > - port = mtk_pcie_find_port(pdev->bus, pdev->devfn);
> > - if (!port)
> > - return;
> > + mutex_lock(&port->lock);
> > +
> > + if (!test_bit(d->hwirq, port->msi_irq_in_use))
> > + dev_err(port->pcie->dev, "trying to free unused MSI#%lu\n",
> > + d->hwirq);
> > + else
> > + __clear_bit(d->hwirq, port->msi_irq_in_use);
> >
> > - irq_dispose_mapping(irq);
> > - mtk_pcie_msi_free(port, hwirq);
> > + mutex_unlock(&port->lock);
> > +
> > + irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> > }
> >
> > -static struct msi_controller mtk_pcie_msi_chip = {
> > - .setup_irq = mtk_pcie_msi_setup_irq,
> > - .teardown_irq = mtk_msi_teardown_irq,
> > +static const struct irq_domain_ops msi_domain_ops = {
> > + .alloc = mtk_pcie_irq_domain_alloc,
> > + .free = mtk_pcie_irq_domain_free,
> > };
> >
> > static struct irq_chip mtk_msi_irq_chip = {
> > .name = "MTK PCIe MSI",
> > - .irq_enable = pci_msi_unmask_irq,
> > - .irq_disable = pci_msi_mask_irq,
> > .irq_mask = pci_msi_mask_irq,
> > .irq_unmask = pci_msi_unmask_irq,
> > };
> >
> > -static int mtk_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
> > - irq_hw_number_t hwirq)
> > +static struct msi_domain_info mtk_msi_domain_info = {
> > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > + MSI_FLAG_PCI_MSIX),
> > + .chip = &mtk_msi_irq_chip,
> > +};
> > +
> > +static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
> > {
> > - irq_set_chip_and_handler(irq, &mtk_msi_irq_chip, handle_simple_irq);
> > - irq_set_chip_data(irq, domain->host_data);
> > + struct fwnode_handle *fwnode = of_node_to_fwnode(port->pcie->dev->of_node);
> > +
> > + mutex_init(&port->lock);
> > +
> > + port->inner_domain = irq_domain_create_linear(fwnode, MTK_MSI_IRQS_NUM,
> > + &msi_domain_ops, port);
> > + if (!port->inner_domain) {
> > + dev_err(port->pcie->dev, "failed to create IRQ domain\n");
> > + return -ENOMEM;
> > + }
> > +
> > + port->msi_domain = pci_msi_create_irq_domain(fwnode, &mtk_msi_domain_info,
> > + port->inner_domain);
> > + if (!port->msi_domain) {
> > + dev_err(port->pcie->dev, "failed to create MSI domain\n");
> > + irq_domain_remove(port->inner_domain);
> > + return -ENOMEM;
> > + }
> >
> > return 0;
> > }
> >
> > -static const struct irq_domain_ops msi_domain_ops = {
> > - .map = mtk_pcie_msi_map,
> > -};
> > -
> > static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
> > {
> > u32 val;
> > @@ -575,6 +598,7 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
> > {
> > struct device *dev = port->pcie->dev;
> > struct device_node *pcie_intc_node;
> > + int ret;
> >
> > /* Setup INTx */
> > pcie_intc_node = of_get_next_child(node, NULL);
> > @@ -591,27 +615,28 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
> > }
> >
> > if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > - port->msi_domain = irq_domain_add_linear(node, MTK_MSI_IRQS_NUM,
> > - &msi_domain_ops,
> > - &mtk_pcie_msi_chip);
> > - if (!port->msi_domain) {
> > - dev_err(dev, "failed to create MSI IRQ domain\n");
> > - return -ENODEV;
> > - }
> > + ret = mtk_pcie_allocate_msi_domains(port);
> > + if (ret)
> > + return ret;
> > +
> > mtk_pcie_enable_msi(port);
> > }
> >
> > return 0;
> > }
> >
> > -static irqreturn_t mtk_pcie_intr_handler(int irq, void *data)
> > +static void mtk_pcie_intr_handler(struct irq_desc *desc)
> > {
> > - struct mtk_pcie_port *port = (struct mtk_pcie_port *)data;
> > + struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
> > + struct irq_chip *irqchip = irq_desc_get_chip(desc);
> > unsigned long status;
> > u32 virq;
> > u32 bit = INTX_SHIFT;
> >
> > - while ((status = readl(port->base + PCIE_INT_STATUS)) & INTX_MASK) {
> > + chained_irq_enter(irqchip, desc);
> > +
> > + status = readl(port->base + PCIE_INT_STATUS);
> > + if (status & INTX_MASK) {
> > for_each_set_bit_from(bit, &status, PCI_NUM_INTX + INTX_SHIFT) {
> > /* Clear the INTx */
> > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > @@ -622,14 +647,14 @@ static irqreturn_t mtk_pcie_intr_handler(int irq, void *data)
> > }
> >
> > if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > - while ((status = readl(port->base + PCIE_INT_STATUS)) & MSI_STATUS) {
> > + if (status & MSI_STATUS) {
> > unsigned long imsi_status;
> >
> > while ((imsi_status = readl(port->base + PCIE_IMSI_STATUS))) {
> > for_each_set_bit(bit, &imsi_status, MTK_MSI_IRQS_NUM) {
> > /* Clear the MSI */
> > writel(1 << bit, port->base + PCIE_IMSI_STATUS);
>
> This write suspiciously looks like an "ack" operation on the controller.
> Why isn't it part of the irqchip as a first class method?
>
> > - virq = irq_find_mapping(port->msi_domain, bit);
> > + virq = irq_find_mapping(port->inner_domain, bit);
> > generic_handle_irq(virq);
> > }
> > }
> > @@ -638,7 +663,9 @@ static irqreturn_t mtk_pcie_intr_handler(int irq, void *data)
> > }
> > }
> >
> > - return IRQ_HANDLED;
> > + chained_irq_exit(irqchip, desc);
> > +
> > + return;
> > }
> >
> > static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
> > @@ -649,20 +676,15 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
> > struct platform_device *pdev = to_platform_device(dev);
> > int err, irq;
> >
> > - irq = platform_get_irq(pdev, port->slot);
> > - err = devm_request_irq(dev, irq, mtk_pcie_intr_handler,
> > - IRQF_SHARED, "mtk-pcie", port);
> > - if (err) {
> > - dev_err(dev, "unable to request IRQ %d\n", irq);
> > - return err;
> > - }
> > -
> > err = mtk_pcie_init_irq_domain(port, node);
> > if (err) {
> > dev_err(dev, "failed to init PCIe IRQ domain\n");
> > return err;
> > }
> >
> > + irq = platform_get_irq(pdev, port->slot);
> > + irq_set_chained_handler_and_data(irq, mtk_pcie_intr_handler, port);
> > +
> > return 0;
> > }
> >
> > @@ -1096,8 +1118,6 @@ static int mtk_pcie_register_host(struct pci_host_bridge *host)
> > host->map_irq = of_irq_parse_and_map_pci;
> > host->swizzle_irq = pci_common_swizzle;
> > host->sysdata = pcie;
> > - if (IS_ENABLED(CONFIG_PCI_MSI) && pcie->soc->has_msi)
> > - host->msi = &mtk_pcie_msi_chip;
> >
> > err = pci_scan_root_bus_bridge(host);
> > if (err < 0)
> > @@ -1159,7 +1179,6 @@ static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> > };
> >
> > static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > - .has_msi = true,
> > .ops = &mtk_pcie_ops_v2,
> > .startup = mtk_pcie_startup_port_v2,
> > .setup_irq = mtk_pcie_setup_irq,
> > @@ -1167,7 +1186,6 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> >
> > static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> > .need_fix_class_id = true,
> > - .has_msi = true,
> > .ops = &mtk_pcie_ops_v2,
> > .startup = mtk_pcie_startup_port_v2,
> > .setup_irq = mtk_pcie_setup_irq,
> >
>
> Thanks,
>
> M.

2018-05-03 09:42:30

by Honghui Zhang

Subject: Re: [PATCH 2/2] PCI: mediatek: Using chained IRQ to setup IRQ handle

On Wed, 2018-05-02 at 11:09 +0100, Marc Zyngier wrote:
> On 02/05/18 10:41, Honghui Zhang wrote:
> > On Mon, 2018-04-30 at 12:03 +0100, Marc Zyngier wrote:
> >> Hi Zhang,
> >>
> >> On 20/04/18 06:25, [email protected] wrote:
> >>> From: Honghui Zhang <[email protected]>
> >>>
> >>> Using irq_chip solution to setup IRQs for the consistent with IRQ framework.
> >>>
> >>> Signed-off-by: Honghui Zhang <[email protected]>
> >>> ---
> >>> drivers/pci/host/pcie-mediatek.c | 192 +++++++++++++++++++++------------------
> >>> 1 file changed, 105 insertions(+), 87 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
> >>> index c3dc549..1d9c6f1 100644
> >>> --- a/drivers/pci/host/pcie-mediatek.c
> >>> +++ b/drivers/pci/host/pcie-mediatek.c
> >>> @@ -11,8 +11,10 @@
> >>> #include <linux/delay.h>
> >>> #include <linux/iopoll.h>
> >>> #include <linux/irq.h>
> >>> +#include <linux/irqchip/chained_irq.h>
> >>> #include <linux/irqdomain.h>
> >>> #include <linux/kernel.h>
> >>> +#include <linux/msi.h>
> >>> #include <linux/of_address.h>
> >>> #include <linux/of_pci.h>
> >>> #include <linux/of_platform.h>
> >>> @@ -130,14 +132,12 @@ struct mtk_pcie_port;
> >>> /**
> >>> * struct mtk_pcie_soc - differentiate between host generations
> >>> * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> >>> - * @has_msi: whether this host supports MSI interrupts or not
> >>> * @ops: pointer to configuration access functions
> >>> * @startup: pointer to controller setting functions
> >>> * @setup_irq: pointer to initialize IRQ functions
> >>> */
> >>> struct mtk_pcie_soc {
> >>> bool need_fix_class_id;
> >>> - bool has_msi;
> >>> struct pci_ops *ops;
> >>> int (*startup)(struct mtk_pcie_port *port);
> >>> int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> >>> @@ -161,7 +161,9 @@ struct mtk_pcie_soc {
> >>> * @lane: lane count
> >>> * @slot: port slot
> >>> * @irq_domain: legacy INTx IRQ domain
> >>> + * @inner_domain: inner IRQ domain
> >>> * @msi_domain: MSI IRQ domain
> >>> + * @lock: protect the msi_irq_in_use bitmap
> >>> * @msi_irq_in_use: bit map for assigned MSI IRQ
> >>> */
> >>> struct mtk_pcie_port {
> >>> @@ -179,7 +181,9 @@ struct mtk_pcie_port {
> >>> u32 lane;
> >>> u32 slot;
> >>> struct irq_domain *irq_domain;
> >>> + struct irq_domain *inner_domain;
> >>> struct irq_domain *msi_domain;
> >>> + struct mutex lock;
> >>> DECLARE_BITMAP(msi_irq_in_use, MTK_MSI_IRQS_NUM);
> >>> };
> >>>
> >>> @@ -446,103 +450,122 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> >>> return 0;
> >>> }
> >>>
> >>> -static int mtk_pcie_msi_alloc(struct mtk_pcie_port *port)
> >>> +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> >>> {
> >>> - int msi;
> >>> + struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> >>> + phys_addr_t addr;
> >>>
> >>> - msi = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
> >>> - if (msi < MTK_MSI_IRQS_NUM)
> >>> - set_bit(msi, port->msi_irq_in_use);
> >>> - else
> >>> - return -ENOSPC;
> >>> + /* MT2712/MT7622 only support 32-bit MSI addresses */
> >>> + addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> >>> + msg->address_hi = 0;
> >>> + msg->address_lo = lower_32_bits(addr);
> >>>
> >>> - return msi;
> >>> + msg->data = data->hwirq;
> >>> +
> >>> + dev_dbg(port->pcie->dev, "msi#%d address_hi %#x address_lo %#x\n",
> >>> + (int)data->hwirq, msg->address_hi, msg->address_lo);
> >>> }
> >>>
> >>> -static void mtk_pcie_msi_free(struct mtk_pcie_port *port, unsigned long hwirq)
> >>> +static int mtk_msi_set_affinity(struct irq_data *irq_data,
> >>> + const struct cpumask *mask, bool force)
> >>> {
> >>> - clear_bit(hwirq, port->msi_irq_in_use);
> >>> + return -EINVAL;
> >>> }
> >>>
> >>> -static int mtk_pcie_msi_setup_irq(struct msi_controller *chip,
> >>> - struct pci_dev *pdev, struct msi_desc *desc)
> >>> -{
> >>> - struct mtk_pcie_port *port;
> >>> - struct msi_msg msg;
> >>> - unsigned int irq;
> >>> - int hwirq;
> >>> - phys_addr_t msg_addr;
> >>> +static struct irq_chip mtk_msi_bottom_irq_chip = {
> >>> + .name = "MTK MSI",
> >>> + .irq_compose_msi_msg = mtk_compose_msi_msg,
> >>> + .irq_set_affinity = mtk_msi_set_affinity,
> >>> + .irq_mask = pci_msi_mask_irq,
> >>> + .irq_unmask = pci_msi_unmask_irq,
> >>> +};
> >>>
> >>> - port = mtk_pcie_find_port(pdev->bus, pdev->devfn);
> >>> - if (!port)
> >>> - return -EINVAL;
> >>> +static int mtk_pcie_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> >>> + unsigned int nr_irqs, void *args)
> >>> +{
> >>> + struct mtk_pcie_port *port = domain->host_data;
> >>> + unsigned long bit;
> >>>
> >>> - hwirq = mtk_pcie_msi_alloc(port);
> >>> - if (hwirq < 0)
> >>> - return hwirq;
> >>> + WARN_ON(nr_irqs != 1);
> >>> + mutex_lock(&port->lock);
> >>>
> >>> - irq = irq_create_mapping(port->msi_domain, hwirq);
> >>> - if (!irq) {
> >>> - mtk_pcie_msi_free(port, hwirq);
> >>> - return -EINVAL;
> >>> + bit = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
> >>> + if (bit >= MTK_MSI_IRQS_NUM) {
> >>> + mutex_unlock(&port->lock);
> >>> + return -ENOSPC;
> >>> }
> >>>
> >>> - chip->dev = &pdev->dev;
> >>> -
> >>> - irq_set_msi_desc(irq, desc);
> >>> + __set_bit(bit, port->msi_irq_in_use);
> >>>
> >>> - /* MT2712/MT7622 only support 32-bit MSI addresses */
> >>> - msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> >>> - msg.address_hi = 0;
> >>> - msg.address_lo = lower_32_bits(msg_addr);
> >>> - msg.data = hwirq;
> >>> + mutex_unlock(&port->lock);
> >>>
> >>> - pci_write_msi_msg(irq, &msg);
> >>> + irq_domain_set_info(domain, virq, bit, &mtk_msi_bottom_irq_chip,
> >>> + domain->host_data, handle_simple_irq,
> >>
> >> Why don't you handle the interrupt as an edge interrupt
> >> (handle_edge_irq)? That's what it really is, and the fact that you use
> >> "handle_simple_irq" makes me think that wou're papering over some other
> >> issue (see below).
> >
> > Hi, Marc,
> >
> > Thanks very much for your comments.
> >
> > I guess the MSI irq is more like a level IRQ instead of edge IRQ.
> > When a MSI IRQ arrived, the corresponding bit of MSI status register
> > will be asserted and stay asserted until it's been cleared by software.
>
> There is some confusion here. The MSI itself is always edge-triggered.
> That's by definition, and you cannot change it. The interrupt that is
> used to signal that MSIs are pending (the multiplexing interrupt) can
> itself be level triggered (and goes low once all the MSIs have been
> acknowledged).

Hi, Marc, thanks for your comments.

Yes, PCIe SPEC have defined that the MSI must be edge trigged. But I'm
not sure whether the HW design needed to following some edge irq
standard. In our design, when there's MSI irq trigged(which may is edge
trigged in hw implemented), the corresponding msi irq status bits will
be asserted until it's been cleared by software. It's more like that the
HW IP have translate the edge irq to level status. Do you think using
handle_level_irq or handle_simple_irq(which most of the pci host driver
now doing) is acceptable in this case?

BTW, I'm still struggling in trying the handle_edge_irq solution. Seems
that I could not get the msi index in irq_ack callback from
irq_data->hwirq, the hwirq was set by pci_msi_domain_calc_hwirq() which
I'm still try to figure out the proper routine in my driver.

thanks.
>
> > I will try to using handle_level_irq and add some callbacks of irq_ack
> > in the next version.
>
> The interrupt above seem to be the MSI itself, and not the multiplexing
> interrupt. It should be handled as an edge.
>
> If that doesn't work, we need to understand why.
>
> Thanks,
>
> M.

2018-05-03 13:06:28

by Marc Zyngier

Subject: Re: [PATCH 2/2] PCI: mediatek: Using chained IRQ to setup IRQ handle

On 03/05/18 10:41, Honghui Zhang wrote:
> On Wed, 2018-05-02 at 11:09 +0100, Marc Zyngier wrote:
>> On 02/05/18 10:41, Honghui Zhang wrote:
>>> On Mon, 2018-04-30 at 12:03 +0100, Marc Zyngier wrote:
>>>> Hi Zhang,
>>>>
>>>> On 20/04/18 06:25, [email protected] wrote:
>>>>> From: Honghui Zhang <[email protected]>
>>>>>
>>>>> Using irq_chip solution to setup IRQs for the consistent with IRQ framework.
>>>>>
>>>>> Signed-off-by: Honghui Zhang <[email protected]>
>>>>> ---
>>>>> drivers/pci/host/pcie-mediatek.c | 192 +++++++++++++++++++++------------------
>>>>> 1 file changed, 105 insertions(+), 87 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
>>>>> index c3dc549..1d9c6f1 100644
>>>>> --- a/drivers/pci/host/pcie-mediatek.c
>>>>> +++ b/drivers/pci/host/pcie-mediatek.c
>>>>> @@ -11,8 +11,10 @@
>>>>> #include <linux/delay.h>
>>>>> #include <linux/iopoll.h>
>>>>> #include <linux/irq.h>
>>>>> +#include <linux/irqchip/chained_irq.h>
>>>>> #include <linux/irqdomain.h>
>>>>> #include <linux/kernel.h>
>>>>> +#include <linux/msi.h>
>>>>> #include <linux/of_address.h>
>>>>> #include <linux/of_pci.h>
>>>>> #include <linux/of_platform.h>
>>>>> @@ -130,14 +132,12 @@ struct mtk_pcie_port;
>>>>> /**
>>>>> * struct mtk_pcie_soc - differentiate between host generations
>>>>> * @need_fix_class_id: whether this host's class ID needed to be fixed or not
>>>>> - * @has_msi: whether this host supports MSI interrupts or not
>>>>> * @ops: pointer to configuration access functions
>>>>> * @startup: pointer to controller setting functions
>>>>> * @setup_irq: pointer to initialize IRQ functions
>>>>> */
>>>>> struct mtk_pcie_soc {
>>>>> bool need_fix_class_id;
>>>>> - bool has_msi;
>>>>> struct pci_ops *ops;
>>>>> int (*startup)(struct mtk_pcie_port *port);
>>>>> int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
>>>>> @@ -161,7 +161,9 @@ struct mtk_pcie_soc {
>>>>> * @lane: lane count
>>>>> * @slot: port slot
>>>>> * @irq_domain: legacy INTx IRQ domain
>>>>> + * @inner_domain: inner IRQ domain
>>>>> * @msi_domain: MSI IRQ domain
>>>>> + * @lock: protect the msi_irq_in_use bitmap
>>>>> * @msi_irq_in_use: bit map for assigned MSI IRQ
>>>>> */
>>>>> struct mtk_pcie_port {
>>>>> @@ -179,7 +181,9 @@ struct mtk_pcie_port {
>>>>> u32 lane;
>>>>> u32 slot;
>>>>> struct irq_domain *irq_domain;
>>>>> + struct irq_domain *inner_domain;
>>>>> struct irq_domain *msi_domain;
>>>>> + struct mutex lock;
>>>>> DECLARE_BITMAP(msi_irq_in_use, MTK_MSI_IRQS_NUM);
>>>>> };
>>>>>
>>>>> @@ -446,103 +450,122 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static int mtk_pcie_msi_alloc(struct mtk_pcie_port *port)
>>>>> +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>>>>> {
>>>>> - int msi;
>>>>> + struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
>>>>> + phys_addr_t addr;
>>>>>
>>>>> - msi = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
>>>>> - if (msi < MTK_MSI_IRQS_NUM)
>>>>> - set_bit(msi, port->msi_irq_in_use);
>>>>> - else
>>>>> - return -ENOSPC;
>>>>> + /* MT2712/MT7622 only support 32-bit MSI addresses */
>>>>> + addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
>>>>> + msg->address_hi = 0;
>>>>> + msg->address_lo = lower_32_bits(addr);
>>>>>
>>>>> - return msi;
>>>>> + msg->data = data->hwirq;
>>>>> +
>>>>> + dev_dbg(port->pcie->dev, "msi#%d address_hi %#x address_lo %#x\n",
>>>>> + (int)data->hwirq, msg->address_hi, msg->address_lo);
>>>>> }
>>>>>
>>>>> -static void mtk_pcie_msi_free(struct mtk_pcie_port *port, unsigned long hwirq)
>>>>> +static int mtk_msi_set_affinity(struct irq_data *irq_data,
>>>>> + const struct cpumask *mask, bool force)
>>>>> {
>>>>> - clear_bit(hwirq, port->msi_irq_in_use);
>>>>> + return -EINVAL;
>>>>> }
>>>>>
>>>>> -static int mtk_pcie_msi_setup_irq(struct msi_controller *chip,
>>>>> - struct pci_dev *pdev, struct msi_desc *desc)
>>>>> -{
>>>>> - struct mtk_pcie_port *port;
>>>>> - struct msi_msg msg;
>>>>> - unsigned int irq;
>>>>> - int hwirq;
>>>>> - phys_addr_t msg_addr;
>>>>> +static struct irq_chip mtk_msi_bottom_irq_chip = {
>>>>> + .name = "MTK MSI",
>>>>> + .irq_compose_msi_msg = mtk_compose_msi_msg,
>>>>> + .irq_set_affinity = mtk_msi_set_affinity,
>>>>> + .irq_mask = pci_msi_mask_irq,
>>>>> + .irq_unmask = pci_msi_unmask_irq,
>>>>> +};
>>>>>
>>>>> - port = mtk_pcie_find_port(pdev->bus, pdev->devfn);
>>>>> - if (!port)
>>>>> - return -EINVAL;
>>>>> +static int mtk_pcie_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>>>>> + unsigned int nr_irqs, void *args)
>>>>> +{
>>>>> + struct mtk_pcie_port *port = domain->host_data;
>>>>> + unsigned long bit;
>>>>>
>>>>> - hwirq = mtk_pcie_msi_alloc(port);
>>>>> - if (hwirq < 0)
>>>>> - return hwirq;
>>>>> + WARN_ON(nr_irqs != 1);
>>>>> + mutex_lock(&port->lock);
>>>>>
>>>>> - irq = irq_create_mapping(port->msi_domain, hwirq);
>>>>> - if (!irq) {
>>>>> - mtk_pcie_msi_free(port, hwirq);
>>>>> - return -EINVAL;
>>>>> + bit = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
>>>>> + if (bit >= MTK_MSI_IRQS_NUM) {
>>>>> + mutex_unlock(&port->lock);
>>>>> + return -ENOSPC;
>>>>> }
>>>>>
>>>>> - chip->dev = &pdev->dev;
>>>>> -
>>>>> - irq_set_msi_desc(irq, desc);
>>>>> + __set_bit(bit, port->msi_irq_in_use);
>>>>>
>>>>> - /* MT2712/MT7622 only support 32-bit MSI addresses */
>>>>> - msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
>>>>> - msg.address_hi = 0;
>>>>> - msg.address_lo = lower_32_bits(msg_addr);
>>>>> - msg.data = hwirq;
>>>>> + mutex_unlock(&port->lock);
>>>>>
>>>>> - pci_write_msi_msg(irq, &msg);
>>>>> + irq_domain_set_info(domain, virq, bit, &mtk_msi_bottom_irq_chip,
>>>>> + domain->host_data, handle_simple_irq,
>>>>
>>>> Why don't you handle the interrupt as an edge interrupt
>>>> (handle_edge_irq)? That's what it really is, and the fact that you use
>>>> "handle_simple_irq" makes me think that wou're papering over some other
>>>> issue (see below).
>>>
>>> Hi, Marc,
>>>
>>> Thanks very much for your comments.
>>>
>>> I guess the MSI irq is more like a level IRQ instead of edge IRQ.
>>> When a MSI IRQ arrived, the corresponding bit of MSI status register
>>> will be asserted and stay asserted until it's been cleared by software.
>>
>> There is some confusion here. The MSI itself is always edge-triggered.
>> That's by definition, and you cannot change it. The interrupt that is
>> used to signal that MSIs are pending (the multiplexing interrupt) can
>> itself be level triggered (and goes low once all the MSIs have been
>> acknowledged).
>
> Hi, Marc, thanks for your comments.
>
> Yes, PCIe SPEC have defined that the MSI must be edge trigged. But I'm
> not sure whether the HW design needed to following some edge irq
> standard.


Given that the device is writing to the MSI doorbell, this can only be
an edge (one way to look at it is that you cannot "unwrite" something,
so you can't drop the level).

> In our design, when there's MSI irq trigged(which may is edge
> trigged in hw implemented), the corresponding msi irq status bits will
> be asserted until it's been cleared by software. It's more like that the
> HW IP have translate the edge irq to level status. Do you think using
> handle_level_irq or handle_simple_irq(which most of the pci host driver
> now doing) is acceptable in this case?

This is how it is supposed to work:

Device --edge--> MSI-controller --level--> primary controller

When you're handling the MSI, it has to be edge. When handling the
interrupt between the MSI controller and the main irqchip, this is level.

At the moment, you have handle that first interrupt as level, which is
wrong.

> BTW, I'm still struggling in trying the handle_edge_irq solution. Seems
> that I could not get the msi index in irq_ack callback from
> irq_data->hwirq, the hwirq was set by pci_msi_domain_calc_hwirq() which
> I'm still try to figure out the proper routine in my driver.

That's because you've plugged it in the wrong irqchip. Your top-level
irqchip (the one backing the PCI domain) must be implemented with
irq_chip_ack_parent, and you them implement the ack logic in the
HW-specific irqchip.

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

2018-05-04 03:26:47

by Honghui Zhang

Subject: Re: [PATCH 2/2] PCI: mediatek: Using chained IRQ to setup IRQ handle

On Thu, 2018-05-03 at 14:05 +0100, Marc Zyngier wrote:
> On 03/05/18 10:41, Honghui Zhang wrote:
> > On Wed, 2018-05-02 at 11:09 +0100, Marc Zyngier wrote:
> >> On 02/05/18 10:41, Honghui Zhang wrote:
> >>> On Mon, 2018-04-30 at 12:03 +0100, Marc Zyngier wrote:
> >>>> Hi Zhang,
> >>>>
> >>>> On 20/04/18 06:25, [email protected] wrote:
> >>>>> From: Honghui Zhang <[email protected]>
> >>>>>
> >>>>> Using irq_chip solution to setup IRQs for the consistent with IRQ framework.
> >>>>>
> >>>>> Signed-off-by: Honghui Zhang <[email protected]>
> >>>>> ---
> >>>>> drivers/pci/host/pcie-mediatek.c | 192 +++++++++++++++++++++------------------
> >>>>> 1 file changed, 105 insertions(+), 87 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
> >>>>> index c3dc549..1d9c6f1 100644
> >>>>> --- a/drivers/pci/host/pcie-mediatek.c
> >>>>> +++ b/drivers/pci/host/pcie-mediatek.c
> >>>>> @@ -11,8 +11,10 @@
> >>>>> #include <linux/delay.h>
> >>>>> #include <linux/iopoll.h>
> >>>>> #include <linux/irq.h>
> >>>>> +#include <linux/irqchip/chained_irq.h>
> >>>>> #include <linux/irqdomain.h>
> >>>>> #include <linux/kernel.h>
> >>>>> +#include <linux/msi.h>
> >>>>> #include <linux/of_address.h>
> >>>>> #include <linux/of_pci.h>
> >>>>> #include <linux/of_platform.h>
> >>>>> @@ -130,14 +132,12 @@ struct mtk_pcie_port;
> >>>>> /**
> >>>>> * struct mtk_pcie_soc - differentiate between host generations
> >>>>> * @need_fix_class_id: whether this host's class ID needed to be fixed or not
> >>>>> - * @has_msi: whether this host supports MSI interrupts or not
> >>>>> * @ops: pointer to configuration access functions
> >>>>> * @startup: pointer to controller setting functions
> >>>>> * @setup_irq: pointer to initialize IRQ functions
> >>>>> */
> >>>>> struct mtk_pcie_soc {
> >>>>> bool need_fix_class_id;
> >>>>> - bool has_msi;
> >>>>> struct pci_ops *ops;
> >>>>> int (*startup)(struct mtk_pcie_port *port);
> >>>>> int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> >>>>> @@ -161,7 +161,9 @@ struct mtk_pcie_soc {
> >>>>> * @lane: lane count
> >>>>> * @slot: port slot
> >>>>> * @irq_domain: legacy INTx IRQ domain
> >>>>> + * @inner_domain: inner IRQ domain
> >>>>> * @msi_domain: MSI IRQ domain
> >>>>> + * @lock: protect the msi_irq_in_use bitmap
> >>>>> * @msi_irq_in_use: bit map for assigned MSI IRQ
> >>>>> */
> >>>>> struct mtk_pcie_port {
> >>>>> @@ -179,7 +181,9 @@ struct mtk_pcie_port {
> >>>>> u32 lane;
> >>>>> u32 slot;
> >>>>> struct irq_domain *irq_domain;
> >>>>> + struct irq_domain *inner_domain;
> >>>>> struct irq_domain *msi_domain;
> >>>>> + struct mutex lock;
> >>>>> DECLARE_BITMAP(msi_irq_in_use, MTK_MSI_IRQS_NUM);
> >>>>> };
> >>>>>
> >>>>> @@ -446,103 +450,122 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> -static int mtk_pcie_msi_alloc(struct mtk_pcie_port *port)
> >>>>> +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> >>>>> {
> >>>>> - int msi;
> >>>>> + struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> >>>>> + phys_addr_t addr;
> >>>>>
> >>>>> - msi = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
> >>>>> - if (msi < MTK_MSI_IRQS_NUM)
> >>>>> - set_bit(msi, port->msi_irq_in_use);
> >>>>> - else
> >>>>> - return -ENOSPC;
> >>>>> + /* MT2712/MT7622 only support 32-bit MSI addresses */
> >>>>> + addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> >>>>> + msg->address_hi = 0;
> >>>>> + msg->address_lo = lower_32_bits(addr);
> >>>>>
> >>>>> - return msi;
> >>>>> + msg->data = data->hwirq;
> >>>>> +
> >>>>> + dev_dbg(port->pcie->dev, "msi#%d address_hi %#x address_lo %#x\n",
> >>>>> + (int)data->hwirq, msg->address_hi, msg->address_lo);
> >>>>> }
> >>>>>
> >>>>> -static void mtk_pcie_msi_free(struct mtk_pcie_port *port, unsigned long hwirq)
> >>>>> +static int mtk_msi_set_affinity(struct irq_data *irq_data,
> >>>>> + const struct cpumask *mask, bool force)
> >>>>> {
> >>>>> - clear_bit(hwirq, port->msi_irq_in_use);
> >>>>> + return -EINVAL;
> >>>>> }
> >>>>>
> >>>>> -static int mtk_pcie_msi_setup_irq(struct msi_controller *chip,
> >>>>> - struct pci_dev *pdev, struct msi_desc *desc)
> >>>>> -{
> >>>>> - struct mtk_pcie_port *port;
> >>>>> - struct msi_msg msg;
> >>>>> - unsigned int irq;
> >>>>> - int hwirq;
> >>>>> - phys_addr_t msg_addr;
> >>>>> +static struct irq_chip mtk_msi_bottom_irq_chip = {
> >>>>> + .name = "MTK MSI",
> >>>>> + .irq_compose_msi_msg = mtk_compose_msi_msg,
> >>>>> + .irq_set_affinity = mtk_msi_set_affinity,
> >>>>> + .irq_mask = pci_msi_mask_irq,
> >>>>> + .irq_unmask = pci_msi_unmask_irq,
> >>>>> +};
> >>>>>
> >>>>> - port = mtk_pcie_find_port(pdev->bus, pdev->devfn);
> >>>>> - if (!port)
> >>>>> - return -EINVAL;
> >>>>> +static int mtk_pcie_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> >>>>> + unsigned int nr_irqs, void *args)
> >>>>> +{
> >>>>> + struct mtk_pcie_port *port = domain->host_data;
> >>>>> + unsigned long bit;
> >>>>>
> >>>>> - hwirq = mtk_pcie_msi_alloc(port);
> >>>>> - if (hwirq < 0)
> >>>>> - return hwirq;
> >>>>> + WARN_ON(nr_irqs != 1);
> >>>>> + mutex_lock(&port->lock);
> >>>>>
> >>>>> - irq = irq_create_mapping(port->msi_domain, hwirq);
> >>>>> - if (!irq) {
> >>>>> - mtk_pcie_msi_free(port, hwirq);
> >>>>> - return -EINVAL;
> >>>>> + bit = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
> >>>>> + if (bit >= MTK_MSI_IRQS_NUM) {
> >>>>> + mutex_unlock(&port->lock);
> >>>>> + return -ENOSPC;
> >>>>> }
> >>>>>
> >>>>> - chip->dev = &pdev->dev;
> >>>>> -
> >>>>> - irq_set_msi_desc(irq, desc);
> >>>>> + __set_bit(bit, port->msi_irq_in_use);
> >>>>>
> >>>>> - /* MT2712/MT7622 only support 32-bit MSI addresses */
> >>>>> - msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> >>>>> - msg.address_hi = 0;
> >>>>> - msg.address_lo = lower_32_bits(msg_addr);
> >>>>> - msg.data = hwirq;
> >>>>> + mutex_unlock(&port->lock);
> >>>>>
> >>>>> - pci_write_msi_msg(irq, &msg);
> >>>>> + irq_domain_set_info(domain, virq, bit, &mtk_msi_bottom_irq_chip,
> >>>>> + domain->host_data, handle_simple_irq,
> >>>>
> >>>> Why don't you handle the interrupt as an edge interrupt
> >>>> (handle_edge_irq)? That's what it really is, and the fact that you use
> >>>> "handle_simple_irq" makes me think that wou're papering over some other
> >>>> issue (see below).
> >>>
> >>> Hi, Marc,
> >>>
> >>> Thanks very much for your comments.
> >>>
> >>> I guess the MSI irq is more like a level IRQ instead of edge IRQ.
> >>> When a MSI IRQ arrived, the corresponding bit of MSI status register
> >>> will be asserted and stay asserted until it's been cleared by software.
> >>
> >> There is some confusion here. The MSI itself is always edge-triggered.
> >> That's by definition, and you cannot change it. The interrupt that is
> >> used to signal that MSIs are pending (the multiplexing interrupt) can
> >> itself be level triggered (and goes low once all the MSIs have been
> >> acknowledged).
> >
> > Hi, Marc, thanks for your comments.
> >
> > Yes, PCIe SPEC have defined that the MSI must be edge trigged. But I'm
> > not sure whether the HW design needed to following some edge irq
> > standard.
>
>
> Given that the device is writing to the MSI doorbell, this can only be
> an edge (one way to look at it is that you cannot "unwrite" something,
> so you can't drop the level).
>
Thanks for your explain in this point of view. I will handle it as edge
IRQ in the next version.

> > In our design, when there's MSI irq trigged(which may is edge
> > trigged in hw implemented), the corresponding msi irq status bits will
> > be asserted until it's been cleared by software. It's more like that the
> > HW IP have translate the edge irq to level status. Do you think using
> > handle_level_irq or handle_simple_irq(which most of the pci host driver
> > now doing) is acceptable in this case?
>
> This is how it is supposed to work:
>
> Device --edge--> MSI-controller --level--> primary controller
>
> When you're handling the MSI, it has to be edge. When handling the
> interrupt between the MSI controller and the main irqchip, this is level.
>
> At the moment, you have handle that first interrupt as level, which is
> wrong.
>
> > BTW, I'm still struggling in trying the handle_edge_irq solution. Seems
> > that I could not get the msi index in irq_ack callback from
> > irq_data->hwirq, the hwirq was set by pci_msi_domain_calc_hwirq() which
> > I'm still try to figure out the proper routine in my driver.
>
> That's because you've plugged it in the wrong irqchip. Your top-level
> irqchip (the one backing the PCI domain) must be implemented with
> irq_chip_ack_parent, and you them implement the ack logic in the
> HW-specific irqchip.
>

Thanks very much for your advise, that's exactly the mistake that I
made. Now it's works fine, I will check it again and send the next
version.

thanks.

> M.