* [PATCH v9 0/4] PCI EP driver support MSI doorbell from host
@ 2022-09-07 3:48 Frank Li
2022-09-07 3:48 ` [PATCH v9 1/4] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END Frank Li
` (3 more replies)
0 siblings, 4 replies; 24+ messages in thread
From: Frank Li @ 2022-09-07 3:48 UTC (permalink / raw)
To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
kw, bhelgaas
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam
┌───────┐ ┌──────────┐
│ │ │ │
┌─────────────┐ │ │ │ PCI Host │
│ MSI │◄┐ │ │ │ │
│ Controller │ │ │ │ │ │
└─────────────┘ └─┼───────┼──────────┼─Bar0 │
│ PCI │ │ Bar1 │
│ Func │ │ Bar2 │
│ │ │ Bar3 │
│ │ │ Bar4 │
│ ├─────────►│ │
└───────┘ └──────────┘
Many PCI controllers provided Endpoint functions.
Generally PCI endpoint is hardware, which is not running a rich OS,
like linux.
But Linux also supports endpoint functions. PCI Host write BAR<n> space
like write to memory. The EP side can't know memory changed by the Host
driver.
PCI Spec has not defined a standard method to do that. Only define
MSI(x) to let EP notified RC status change.
The basic idea is to trigger an IRQ when PCI RC writes to a memory
address. That's what MSI controller provided. EP drivers just need to
request a platform MSI interrupt, struct MSI_msg *msg will pass down a
memory address and data. EP driver will map such memory address to
one of PCI BAR<n>. Host just writes such an address to trigger EP side
IRQ.
If system have gic-its, only need update PCI EP side driver. But i.MX
have not chip support gic-its yet. So we have to use MU to simulate a
MSI controller. Although only 4 MSI IRQs are simulated, it matched
vntb(pci-epf-vntb) network requirement.
After enable MSI, ping delay reduce < 1ms from ~8ms
IRQchip: imx mu worked as MSI controller:
let imx mu worked as MSI controllers. Although IP is not design
as MSI controller, we still can use it if limited IRQ number to 4.
pcie: endpoint: pci-epf-vntb: add endpoint MSI support
Based on ntb-next branch. https://github.com/jonmason/ntb/commits/ntb-next
Using MSI as door bell registers
This patch is totally independent on previous on. It can be
applied to ntb-next seperately.
i.MX EP function driver is upstreaming by Richard Zhu.
Some dts change missed at this patches. below is reference dts change
--- a/arch/arm64/boot/dts/freescale/imx8-ss-hsio.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8-ss-hsio.dtsi
@@ -160,5 +160,6 @@ pcieb_ep: pcie_ep@5f010000 {
num-ib-windows = <6>;
num-ob-windows = <6>;
status = "disabled";
+ MSI-parent = <&lsio_mu12>;
};
--- a/arch/arm64/boot/dts/freescale/imx8-ss-lsio.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8-ss-lsio.dtsi
@@ -172,6 +172,19 @@ lsio_mu6: mailbox@5d210000 {
status = "disabled";
};
+ lsio_mu12: mailbox@5d270000 {
+ compatible = "fsl,imx6sx-mu-MSI";
+ msi-controller;
+ interrupt-controller;
+ reg = <0x5d270000 0x10000>, /* A side */
+ <0x5d300000 0x10000>; /* B side */
+ reg-names = "a", "b";
+ interrupts = <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>;
+ power-domains = <&pd IMX_SC_R_MU_12A>,
+ <&pd IMX_SC_R_MU_12B>;
+ power-domain-names = "a", "b";
+ };
+
Change Log
- Change from v8 to v9
fix dt_bind_check error
- Change from v7 to v8
irqchip: using name process-a-side as resource bind name
pcie: endpoint:
- fix build error reported by kernel test robot <lkp@intel.com>
- rename epf_db_phy to epf_db_phys
- rework error message
- rework commit message
- change ntb to vtb at apply irq.
- kept name msi_virqbase because it is msi irq base number,
not base address.
- Change from v6 to v7
pcie: endpoint: add endpoint MSI support
Fine tuning commit message
Fixed issues, reviewed by Bjorn Helgaas
- Change from v5 to v6
Fixed build error found by kernel test robot
- Change from v4 to v5
Fixed dt-binding document
add msi-cell
add interrupt max number
update naming reg-names and power-domain-names.
Fixed irqchip-Add-IMX-MU-MSI-controller-driver.patch
rework commit message
remove some field in struct imx_mu_dcfg
error handle when link power domain failure.
add irq_domain_update_bus_token
- Change from v3 to v4
Fixed dt-binding document according to Krzysztof Kozlowski's feedback
Fixed irqchip-imx-mu-worked-as-msi-controller according to Marc Zyngier's
comments.
There are still two important points, which I am not sure.
1. clean irq_set_affinity after platform_msi_create_irq_domain.
Some function, like platform_msi_write_msg() is static.
so I have to set MSI_FLAG_USE_DEF_CHIP_OPS flags, which will
set irq_set_affinity to default one.
2. about comments
> + msi_data->msi_domain = platform_msi_create_irq_domain(
> + of_node_to_fwnode(msi_data->pdev->dev.of_node),
> + &imx_mu_msi_domain_info,
> + msi_data->parent);
"And you don't get an error due to the fact that you use the same
fwnode for both domains without overriding the domain bus token?"
I did not understand yet.
Fixed static check warning, reported by Dan Carpenter
pcie: endpoint: pci-epf-vntb: add endpoint MSI support
- Change from v2 to v3
Fixed dt-binding docment check failure
Fixed typo a cover letter.
Change according Bjorn's comments at patch
pcie: endpoint: pci-epf-vntb: add endpoint MSI support
- from V1 to V2
Fixed fsl,mu-msi.yaml's problem
Fixed irq-imx-mu-msi.c problem according Marc Zyngier's feeback
Added a new patch to allow pass down .pm by IRQCHIP_PLATFORM_DRIVER_END
--
2.35.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v9 1/4] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END
2022-09-07 3:48 [PATCH v9 0/4] PCI EP driver support MSI doorbell from host Frank Li
@ 2022-09-07 3:48 ` Frank Li
2022-09-07 3:48 ` [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver Frank Li
` (2 subsequent siblings)
3 siblings, 0 replies; 24+ messages in thread
From: Frank Li @ 2022-09-07 3:48 UTC (permalink / raw)
To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
kw, bhelgaas
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam
IRQCHIP_PLATFORM_DRIVER_* compilation define platform_driver
for irqchip. But can't set .pm field of platform_driver.
Added variadic macros to set .pm field or other field if need.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
include/linux/irqchip.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h
index 3a091d0710ae1..d5e6024cb2a8c 100644
--- a/include/linux/irqchip.h
+++ b/include/linux/irqchip.h
@@ -44,7 +44,8 @@ static const struct of_device_id drv_name##_irqchip_match_table[] = {
#define IRQCHIP_MATCH(compat, fn) { .compatible = compat, \
.data = typecheck_irq_init_cb(fn), },
-#define IRQCHIP_PLATFORM_DRIVER_END(drv_name) \
+
+#define IRQCHIP_PLATFORM_DRIVER_END(drv_name, ...) \
{}, \
}; \
MODULE_DEVICE_TABLE(of, drv_name##_irqchip_match_table); \
@@ -56,6 +57,7 @@ static struct platform_driver drv_name##_driver = { \
.owner = THIS_MODULE, \
.of_match_table = drv_name##_irqchip_match_table, \
.suppress_bind_attrs = true, \
+ __VA_ARGS__ \
}, \
}; \
builtin_platform_driver(drv_name##_driver)
--
2.35.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-07 3:48 [PATCH v9 0/4] PCI EP driver support MSI doorbell from host Frank Li
2022-09-07 3:48 ` [PATCH v9 1/4] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END Frank Li
@ 2022-09-07 3:48 ` Frank Li
2022-09-08 0:03 ` kernel test robot
2022-09-08 7:39 ` Marc Zyngier
2022-09-07 3:48 ` [PATCH v9 3/4] dt-bindings: irqchip: imx mu work as msi controller Frank Li
2022-09-07 3:48 ` [PATCH v9 4/4] PCI: endpoint: Add vNTB MSI support Frank Li
3 siblings, 2 replies; 24+ messages in thread
From: Frank Li @ 2022-09-07 3:48 UTC (permalink / raw)
To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
kw, bhelgaas
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam
The MU block found in a number of Freescale/NXP SoCs supports generating
IRQs by writing data to a register
This enables the MU block to be used as a MSI controller, by leveraging
the platform-MSI API
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
drivers/irqchip/Kconfig | 9 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-imx-mu-msi.c | 451 +++++++++++++++++++++++++++++++
3 files changed, 461 insertions(+)
create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 5e4e50122777d..e04c6521dce55 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -470,6 +470,15 @@ config IMX_INTMUX
help
Support for the i.MX INTMUX interrupt multiplexer.
+config IMX_MU_MSI
+ bool "i.MX MU work as MSI controller"
+ default y if ARCH_MXC
+ select IRQ_DOMAIN
+ select IRQ_DOMAIN_HIERARCHY
+ select GENERIC_MSI_IRQ_DOMAIN
+ help
+ MU work as MSI controller to do general doorbell
+
config LS1X_IRQ
bool "Loongson-1 Interrupt Controller"
depends on MACH_LOONGSON32
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 5d8e21d3dc6d8..870423746c783 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o
obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o
obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o
obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o
+obj-$(CONFIG_IMX_MU_MSI) += irq-imx-mu-msi.o
obj-$(CONFIG_MADERA_IRQ) += irq-madera.o
obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o
obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o
diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-msi.c
new file mode 100644
index 0000000000000..82b55f6d87266
--- /dev/null
+++ b/drivers/irqchip/irq-imx-mu-msi.c
@@ -0,0 +1,451 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Freescale MU worked as MSI controller
+ *
+ * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de>
+ * Copyright 2022 NXP
+ * Frank Li <Frank.Li@nxp.com>
+ * Peng Fan <peng.fan@nxp.com>
+ *
+ * Based on drivers/mailbox/imx-mailbox.c
+ */
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/spinlock.h>
+#include <linux/dma-iommu.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm_domain.h>
+
+
+#define IMX_MU_CHANS 4
+
+enum imx_mu_xcr {
+ IMX_MU_GIER,
+ IMX_MU_GCR,
+ IMX_MU_TCR,
+ IMX_MU_RCR,
+ IMX_MU_xCR_MAX,
+};
+
+enum imx_mu_xsr {
+ IMX_MU_SR,
+ IMX_MU_GSR,
+ IMX_MU_TSR,
+ IMX_MU_RSR,
+};
+
+enum imx_mu_type {
+ IMX_MU_V1 = BIT(0),
+ IMX_MU_V2 = BIT(1),
+ IMX_MU_V2_S4 = BIT(15),
+};
+
+/* Receive Interrupt Enable */
+#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
+#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
+
+struct imx_mu_dcfg {
+ enum imx_mu_type type;
+ u32 xTR; /* Transmit Register0 */
+ u32 xRR; /* Receive Register0 */
+ u32 xSR[4]; /* Status Registers */
+ u32 xCR[4]; /* Control Registers */
+};
+
+struct imx_mu_msi {
+ spinlock_t lock;
+ raw_spinlock_t reglock;
+ struct irq_domain *msi_domain;
+ void __iomem *regs;
+ phys_addr_t msiir_addr;
+ const struct imx_mu_dcfg *cfg;
+ unsigned long used;
+ struct clk *clk;
+};
+
+static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
+{
+ iowrite32(val, msi_data->regs + offs);
+}
+
+static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
+{
+ return ioread32(msi_data->regs + offs);
+}
+
+static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum imx_mu_xcr type, u32 set, u32 clr)
+{
+ unsigned long flags;
+ u32 val;
+
+ raw_spin_lock_irqsave(&msi_data->reglock, flags);
+ val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
+ val &= ~clr;
+ val |= set;
+ imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
+ raw_spin_unlock_irqrestore(&msi_data->reglock, flags);
+
+ return val;
+}
+
+static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
+{
+ struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+ imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data, data->hwirq));
+}
+
+static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
+{
+ struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+ imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
+}
+
+static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
+{
+ struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+
+ imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
+}
+
+static struct irq_chip imx_mu_msi_irq_chip = {
+ .name = "MU-MSI",
+ .irq_ack = irq_chip_ack_parent,
+};
+
+static struct msi_domain_ops imx_mu_msi_irq_ops = {
+};
+
+static struct msi_domain_info imx_mu_msi_domain_info = {
+ .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
+ .ops = &imx_mu_msi_irq_ops,
+ .chip = &imx_mu_msi_irq_chip,
+};
+
+static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
+ struct msi_msg *msg)
+{
+ struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
+ u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
+
+ msg->address_hi = upper_32_bits(addr);
+ msg->address_lo = lower_32_bits(addr);
+ msg->data = data->hwirq;
+}
+
+static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
+ const struct cpumask *mask, bool force)
+{
+ return -EINVAL;
+}
+
+static struct irq_chip imx_mu_msi_parent_chip = {
+ .name = "MU",
+ .irq_mask = imx_mu_msi_parent_mask_irq,
+ .irq_unmask = imx_mu_msi_parent_unmask_irq,
+ .irq_ack = imx_mu_msi_parent_ack_irq,
+ .irq_compose_msi_msg = imx_mu_msi_parent_compose_msg,
+ .irq_set_affinity = imx_mu_msi_parent_set_affinity,
+};
+
+static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
+ unsigned int virq,
+ unsigned int nr_irqs,
+ void *args)
+{
+ struct imx_mu_msi *msi_data = domain->host_data;
+ unsigned long flags;
+ int pos, err = 0;
+
+ WARN_ON(nr_irqs != 1);
+
+ spin_lock_irqsave(&msi_data->lock, flags);
+ pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
+ if (pos < IMX_MU_CHANS)
+ __set_bit(pos, &msi_data->used);
+ else
+ err = -ENOSPC;
+ spin_unlock_irqrestore(&msi_data->lock, flags);
+
+ if (err)
+ return err;
+
+ irq_domain_set_info(domain, virq, pos,
+ &imx_mu_msi_parent_chip, msi_data,
+ handle_edge_irq, NULL, NULL);
+ return 0;
+}
+
+static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs)
+{
+ struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+ struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
+ unsigned long flags;
+
+ spin_lock_irqsave(&msi_data->lock, flags);
+ __clear_bit(d->hwirq, &msi_data->used);
+ spin_unlock_irqrestore(&msi_data->lock, flags);
+}
+
+static const struct irq_domain_ops imx_mu_msi_domain_ops = {
+ .alloc = imx_mu_msi_domain_irq_alloc,
+ .free = imx_mu_msi_domain_irq_free,
+};
+
+static void imx_mu_msi_irq_handler(struct irq_desc *desc)
+{
+ struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ u32 status;
+ int i;
+
+ status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
+
+ chained_irq_enter(chip, desc);
+ for (i = 0; i < IMX_MU_CHANS; i++) {
+ if (status & IMX_MU_xSR_RFn(msi_data, i))
+ generic_handle_domain_irq(msi_data->msi_domain, i);
+ }
+ chained_irq_exit(chip, desc);
+}
+
+static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct device *dev)
+{
+ struct fwnode_handle *fwnodes = dev_fwnode(dev);
+ struct irq_domain *parent;
+
+ /* Initialize MSI domain parent */
+ parent = irq_domain_create_linear(fwnodes,
+ IMX_MU_CHANS,
+ &imx_mu_msi_domain_ops,
+ msi_data);
+ if (!parent) {
+ dev_err(dev, "failed to create IRQ domain\n");
+ return -ENOMEM;
+ }
+
+ irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
+
+ msi_data->msi_domain = platform_msi_create_irq_domain(
+ fwnodes,
+ &imx_mu_msi_domain_info,
+ parent);
+
+ if (!msi_data->msi_domain) {
+ dev_err(dev, "failed to create MSI domain\n");
+ irq_domain_remove(parent);
+ return -ENOMEM;
+ }
+
+ irq_domain_set_pm_device(msi_data->msi_domain, dev);
+
+ return 0;
+}
+
+/* Register offset of different version MU IP */
+static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
+ .xTR = 0x0,
+ .xRR = 0x10,
+ .xSR = {0x20, 0x20, 0x20, 0x20},
+ .xCR = {0x24, 0x24, 0x24, 0x24},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
+ .xTR = 0x20,
+ .xRR = 0x40,
+ .xSR = {0x60, 0x60, 0x60, 0x60},
+ .xCR = {0x64, 0x64, 0x64, 0x64},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
+ .type = IMX_MU_V2,
+ .xTR = 0x200,
+ .xRR = 0x280,
+ .xSR = {0xC, 0x118, 0x124, 0x12C},
+ .xCR = {0x110, 0x114, 0x120, 0x128},
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
+
+ .type = IMX_MU_V2 | IMX_MU_V2_S4,
+ .xTR = 0x200,
+ .xRR = 0x280,
+ .xSR = {0xC, 0x118, 0x124, 0x12C},
+ .xCR = {0x110, 0x114, 0x120, 0x128},
+};
+
+static int __init imx_mu_of_init(struct device_node *dn,
+ struct device_node *parent,
+ const struct imx_mu_dcfg *cfg
+ )
+{
+ struct platform_device *pdev = of_find_device_by_node(dn);
+ struct device_link *pd_link_a;
+ struct device_link *pd_link_b;
+ struct imx_mu_msi *msi_data;
+ struct resource *res;
+ struct device *pd_a;
+ struct device *pd_b;
+ struct device *dev;
+ int ret;
+ int irq;
+
+ if (!pdev)
+ return -ENODEV;
+
+ dev = &pdev->dev;
+
+ msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
+ if (!msi_data)
+ return -ENOMEM;
+
+ msi_data->cfg = cfg;
+
+ msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "processor-a-side");
+ if (IS_ERR(msi_data->regs)) {
+ dev_err(&pdev->dev, "failed to initialize 'regs'\n");
+ return PTR_ERR(msi_data->regs);
+ }
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "processor-b-side");
+ if (!res)
+ return -EIO;
+
+ msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq <= 0)
+ return -ENODEV;
+
+ platform_set_drvdata(pdev, msi_data);
+
+ msi_data->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(msi_data->clk)) {
+ if (PTR_ERR(msi_data->clk) != -ENOENT)
+ return PTR_ERR(msi_data->clk);
+
+ msi_data->clk = NULL;
+ }
+
+ pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
+ if (IS_ERR(pd_a))
+ return PTR_ERR(pd_a);
+
+ pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
+ if (IS_ERR(pd_b))
+ return PTR_ERR(pd_b);
+
+ pd_link_a = device_link_add(dev, pd_a,
+ DL_FLAG_STATELESS |
+ DL_FLAG_PM_RUNTIME |
+ DL_FLAG_RPM_ACTIVE);
+
+ if (!pd_link_a) {
+ dev_err(dev, "Failed to add device_link to mu a.\n");
+ goto err_pd_a;
+ }
+
+ pd_link_b = device_link_add(dev, pd_b,
+ DL_FLAG_STATELESS |
+ DL_FLAG_PM_RUNTIME |
+ DL_FLAG_RPM_ACTIVE);
+
+
+ if (!pd_link_b) {
+ dev_err(dev, "Failed to add device_link to mu a.\n");
+ goto err_pd_b;
+ }
+
+ ret = imx_mu_msi_domains_init(msi_data, dev);
+ if (ret)
+ goto err_dm_init;
+
+ irq_set_chained_handler_and_data(irq,
+ imx_mu_msi_irq_handler,
+ msi_data);
+
+ pm_runtime_enable(dev);
+
+ return 0;
+
+err_dm_init:
+ device_link_remove(dev, pd_b);
+err_pd_b:
+ device_link_remove(dev, pd_a);
+err_pd_a:
+ return -EINVAL;
+}
+
+static int __maybe_unused imx_mu_runtime_suspend(struct device *dev)
+{
+ struct imx_mu_msi *priv = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(priv->clk);
+
+ return 0;
+}
+
+static int __maybe_unused imx_mu_runtime_resume(struct device *dev)
+{
+ struct imx_mu_msi *priv = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ dev_err(dev, "failed to enable clock\n");
+
+ return ret;
+}
+
+static const struct dev_pm_ops imx_mu_pm_ops = {
+ SET_RUNTIME_PM_OPS(imx_mu_runtime_suspend,
+ imx_mu_runtime_resume, NULL)
+};
+
+static int __init imx_mu_imx7ulp_of_init(struct device_node *dn,
+ struct device_node *parent)
+{
+ return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx7ulp);
+}
+
+static int __init imx_mu_imx6sx_of_init(struct device_node *dn,
+ struct device_node *parent)
+{
+ return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx6sx);
+}
+
+static int __init imx_mu_imx8ulp_of_init(struct device_node *dn,
+ struct device_node *parent)
+{
+ return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp);
+}
+
+static int __init imx_mu_imx8ulp_s4_of_init(struct device_node *dn,
+ struct device_node *parent)
+{
+ return imx_mu_of_init(dn, parent, &imx_mu_cfg_imx8ulp_s4);
+}
+
+IRQCHIP_PLATFORM_DRIVER_BEGIN(imx_mu_msi)
+IRQCHIP_MATCH("fsl,imx7ulp-mu-msi", imx_mu_imx7ulp_of_init)
+IRQCHIP_MATCH("fsl,imx6sx-mu-msi", imx_mu_imx6sx_of_init)
+IRQCHIP_MATCH("fsl,imx8ulp-mu-msi", imx_mu_imx8ulp_of_init)
+IRQCHIP_MATCH("fsl,imx8ulp-mu-msi-s4", imx_mu_imx8ulp_s4_of_init)
+IRQCHIP_PLATFORM_DRIVER_END(imx_mu_msi, .pm = &imx_mu_pm_ops)
+
+
+MODULE_AUTHOR("Frank Li <Frank.Li@nxp.com>");
+MODULE_DESCRIPTION("Freescale MU MSI controller driver");
+MODULE_LICENSE("GPL");
--
2.35.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v9 3/4] dt-bindings: irqchip: imx mu work as msi controller
2022-09-07 3:48 [PATCH v9 0/4] PCI EP driver support MSI doorbell from host Frank Li
2022-09-07 3:48 ` [PATCH v9 1/4] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END Frank Li
2022-09-07 3:48 ` [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver Frank Li
@ 2022-09-07 3:48 ` Frank Li
2022-09-09 1:43 ` Rob Herring
2022-09-07 3:48 ` [PATCH v9 4/4] PCI: endpoint: Add vNTB MSI support Frank Li
3 siblings, 1 reply; 24+ messages in thread
From: Frank Li @ 2022-09-07 3:48 UTC (permalink / raw)
To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
kw, bhelgaas
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam
I.MX mu support generate irq by write a register. Provide msi controller
support so other driver such as PCI EP can use it by standard msi
interface as doorbell.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
.../interrupt-controller/fsl,mu-msi.yaml | 99 +++++++++++++++++++
1 file changed, 99 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
new file mode 100644
index 0000000000000..799ae5c3e32ae
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/fsl,mu-msi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale/NXP i.MX Messaging Unit (MU) work as msi controller
+
+maintainers:
+ - Frank Li <Frank.Li@nxp.com>
+
+description: |
+ The Messaging Unit module enables two processors within the SoC to
+ communicate and coordinate by passing messages (e.g. data, status
+ and control) through the MU interface. The MU also provides the ability
+ for one processor (A side) to signal the other processor (B side) using
+ interrupts.
+
+ Because the MU manages the messaging between processors, the MU uses
+ different clocks (from each side of the different peripheral buses).
+ Therefore, the MU must synchronize the accesses from one side to the
+ other. The MU accomplishes synchronization using two sets of matching
+ registers (Processor A-side, Processor B-side).
+
+ MU can work as msi interrupt controller to do doorbell
+
+allOf:
+ - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+ compatible:
+ enum:
+ - fsl,imx6sx-mu-msi
+ - fsl,imx7ulp-mu-msi
+ - fsl,imx8ulp-mu-msi
+ - fsl,imx8ulp-mu-msi-s4
+
+ reg:
+ items:
+ - description: a side register base address
+ - description: b side register base address
+
+ reg-names:
+ items:
+ - const: processor-a-side
+ - const: processor-b-side
+
+ interrupts:
+ description: a side interrupt number.
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ power-domains:
+ items:
+ - description: a side power domain
+ - description: b side power domain
+
+ power-domain-names:
+ items:
+ - const: processor-a-side
+ - const: processor-b-side
+
+ interrupt-controller: true
+
+ msi-controller: true
+
+ "#msi-cells":
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - interrupt-controller
+ - msi-controller
+ - "#msi-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/firmware/imx/rsrc.h>
+
+ msi-controller@5d270000 {
+ compatible = "fsl,imx6sx-mu-msi";
+ msi-controller;
+ #msi-cells = <0>;
+ interrupt-controller;
+ reg = <0x5d270000 0x10000>, /* A side */
+ <0x5d300000 0x10000>; /* B side */
+ reg-names = "processor-a-side", "processor-b-side";
+ interrupts = <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>;
+ power-domains = <&pd IMX_SC_R_MU_12A>,
+ <&pd IMX_SC_R_MU_12B>;
+ power-domain-names = "processor-a-side", "processor-b-side";
+ };
--
2.35.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v9 4/4] PCI: endpoint: Add vNTB MSI support
2022-09-07 3:48 [PATCH v9 0/4] PCI EP driver support MSI doorbell from host Frank Li
` (2 preceding siblings ...)
2022-09-07 3:48 ` [PATCH v9 3/4] dt-bindings: irqchip: imx mu work as msi controller Frank Li
@ 2022-09-07 3:48 ` Frank Li
2022-09-07 20:43 ` kernel test robot
2022-09-13 17:24 ` Manivannan Sadhasivam
3 siblings, 2 replies; 24+ messages in thread
From: Frank Li @ 2022-09-07 3:48 UTC (permalink / raw)
To: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
kw, bhelgaas
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-pci, peng.fan,
aisheng.dong, jdmason, kernel, festevam, linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam
┌───────┐ ┌──────────┐
│ │ │ │
┌─────────────┐ │ PCI │ │ PCI Host │
│ MSI │◄┐ │ EP │ │ │
│ Controller │ │ │ │ 3.MSI Write │ │
└────────┬────┘ └─┼───────┼───────────────────┤ │
▲ │ │ │ ├─BAR_n │
│ └────────┼───────┼──────────────────►│ │
│ │ │ 2.Call Back │ │
│ │ │ write_msi_msg() │ │
│ │ │ │ │
│ └───┬───┘ └──────────┘
│ │
└───────────────────┘
1.platform_msi_domain_alloc_irqs()
There is no defined way of raising IRQs by PCI host to the PCI endpoint.
Only define MSI/MSI-X to let EP notified RC status change.
The memory assigned for BAR region by the PCI host is mapped to the
message address of platform msi interrupt controller in PCI Endpoint.
Such that, whenever the PCI host writes to the BAR region, it will
trigger an IRQ in the Endpoint.
Basic working follow as
1. EP function driver call platform_msi_domain_alloc_irqs() alloc a
MSI irq from MSI controller with call back function write_msi_msg();
2. write_msg_msg will config BAR and map to address defined in msi_msg;
3. Host side trigger an IRQ in Endpoint by write to BAR region.
Add MSI support for pci-epf-vntb. Query if system has an MSI controller.
Set up doorbell address according to struct msi_msg.
So PCI RC can write this doorbell address to trigger EP side's IRQ.
If no MSI controller exists, fall back to software polling.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
drivers/pci/endpoint/functions/pci-epf-vntb.c | 155 +++++++++++++++---
1 file changed, 128 insertions(+), 27 deletions(-)
diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 1466dd1904175..426205b980a09 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -44,6 +44,7 @@
#include <linux/pci-epc.h>
#include <linux/pci-epf.h>
#include <linux/ntb.h>
+#include <linux/msi.h>
static struct workqueue_struct *kpcintb_workqueue;
@@ -136,13 +137,15 @@ struct epf_ntb {
struct epf_ntb_ctrl *reg;
- phys_addr_t epf_db_phy;
+ phys_addr_t epf_db_phys;
void __iomem *epf_db;
phys_addr_t vpci_mw_phy[MAX_MW];
void __iomem *vpci_mw_addr[MAX_MW];
struct delayed_work cmd_handler;
+
+ int msi_virqbase;
};
#define to_epf_ntb(epf_group) container_of((epf_group), struct epf_ntb, group)
@@ -253,13 +256,15 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
ntb = container_of(work, struct epf_ntb, cmd_handler.work);
- for (i = 1; i < ntb->db_count; i++) {
- if (readl(ntb->epf_db + i * 4)) {
- if (readl(ntb->epf_db + i * 4))
- ntb->db |= 1 << (i - 1);
+ if (!ntb->epf_db_phys) {
+ for (i = 1; i < ntb->db_count; i++) {
+ if (readl(ntb->epf_db + i * 4)) {
+ if (readl(ntb->epf_db + i * 4))
+ ntb->db |= 1 << (i - 1);
- ntb_db_event(&ntb->ntb, i);
- writel(0, ntb->epf_db + i * 4);
+ ntb_db_event(&ntb->ntb, i);
+ writel(0, ntb->epf_db + i * 4);
+ }
}
}
@@ -454,11 +459,9 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
ctrl->num_mws = ntb->num_mws;
ntb->spad_size = spad_size;
- ctrl->db_entry_size = 4;
-
for (i = 0; i < ntb->db_count; i++) {
ntb->reg->db_data[i] = 1 + i;
- ntb->reg->db_offset[i] = 0;
+ ntb->reg->db_offset[i] = 4 * i;
}
return 0;
@@ -509,6 +512,28 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
return 0;
}
+static int epf_ntb_db_size(struct epf_ntb *ntb)
+{
+ const struct pci_epc_features *epc_features;
+ size_t size = 4 * ntb->db_count;
+ u32 align;
+
+ epc_features = pci_epc_get_features(ntb->epf->epc,
+ ntb->epf->func_no,
+ ntb->epf->vfunc_no);
+ align = epc_features->align;
+
+ if (size < 128)
+ size = 128;
+
+ if (align)
+ size = ALIGN(size, align);
+ else
+ size = roundup_pow_of_two(size);
+
+ return size;
+}
+
/**
* epf_ntb_db_bar_init() - Configure Doorbell window BARs
* @ntb: NTB device that facilitates communication between HOST and vHOST
@@ -522,33 +547,32 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
struct pci_epf_bar *epf_bar;
void __iomem *mw_addr;
enum pci_barno barno;
- size_t size = 4 * ntb->db_count;
+ size_t size;
epc_features = pci_epc_get_features(ntb->epf->epc,
ntb->epf->func_no,
ntb->epf->vfunc_no);
align = epc_features->align;
-
- if (size < 128)
- size = 128;
-
- if (align)
- size = ALIGN(size, align);
- else
- size = roundup_pow_of_two(size);
+ size = epf_ntb_db_size(ntb);
barno = ntb->epf_ntb_bar[BAR_DB];
+ epf_bar = &ntb->epf->bar[barno];
- mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
- if (!mw_addr) {
- dev_err(dev, "Failed to allocate OB address\n");
- return -ENOMEM;
+ if (ntb->epf_db_phys) {
+ mw_addr = NULL;
+ epf_bar->phys_addr = ntb->epf_db_phys;
+ epf_bar->barno = barno;
+ epf_bar->size = size;
+ } else {
+ mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
+ if (!mw_addr) {
+ dev_err(dev, "Failed to allocate door bell address\n");
+ return -ENOMEM;
+ }
}
ntb->epf_db = mw_addr;
- epf_bar = &ntb->epf->bar[barno];
-
ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no, epf_bar);
if (ret) {
dev_err(dev, "Doorbell BAR set failed\n");
@@ -704,6 +728,82 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
return 0;
}
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+static void epf_ntb_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
+{
+ struct epf_ntb *ntb = dev_get_drvdata(desc->dev);
+ struct epf_ntb_ctrl *reg = ntb->reg;
+ int size = epf_ntb_db_size(ntb);
+ u64 addr;
+
+ addr = msg->address_hi;
+ addr <<= 32;
+ addr |= msg->address_lo;
+
+ reg->db_data[desc->msi_index] = msg->data;
+
+ if (desc->msi_index == 0)
+ ntb->epf_db_phys = round_down(addr, size);
+
+ reg->db_offset[desc->msi_index] = addr - ntb->epf_db_phys;
+}
+#endif
+
+static irqreturn_t epf_ntb_interrupt_handler(int irq, void *data)
+{
+ struct epf_ntb *ntb = data;
+ int index;
+
+ index = irq - ntb->msi_virqbase;
+ ntb->db |= 1 << (index - 1);
+ ntb_db_event(&ntb->ntb, index);
+
+ return IRQ_HANDLED;
+}
+
+static void epf_ntb_epc_msi_init(struct epf_ntb *ntb)
+{
+ struct device *dev = &ntb->epf->dev;
+ struct irq_domain *domain;
+ int virq;
+ int ret;
+ int i;
+
+ domain = dev_get_msi_domain(ntb->epf->epc->dev.parent);
+ if (!domain)
+ return;
+
+ dev_set_msi_domain(dev, domain);
+
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+ if (platform_msi_domain_alloc_irqs(&ntb->epf->dev,
+ ntb->db_count,
+ epf_ntb_write_msi_msg)) {
+ dev_info(dev, "Can't allocate MSI, fall back to poll mode\n");
+ return;
+ }
+#else
+ return;
+#endif
+ dev_info(dev, "vntb use MSI as doorbell\n");
+
+ for (i = 0; i < ntb->db_count; i++) {
+ virq = msi_get_virq(dev, i);
+ ret = devm_request_irq(dev, virq,
+ epf_ntb_interrupt_handler, 0,
+ "vntb", ntb);
+
+ if (ret) {
+ dev_err(dev, "devm_request_irq() failure, fall back to poll mode\n");
+ ntb->epf_db_phys = 0;
+ break;
+ }
+
+ if (!i)
+ ntb->msi_virqbase = virq;
+ }
+}
+
/**
* epf_ntb_epc_init() - Initialize NTB interface
* @ntb: NTB device that facilitates communication between HOST and vHOST2
@@ -1299,14 +1399,15 @@ static int epf_ntb_bind(struct pci_epf *epf)
goto err_bar_alloc;
}
+ epf_set_drvdata(epf, ntb);
+ epf_ntb_epc_msi_init(ntb);
+
ret = epf_ntb_epc_init(ntb);
if (ret) {
dev_err(dev, "Failed to initialize EPC\n");
goto err_bar_alloc;
}
- epf_set_drvdata(epf, ntb);
-
pci_space[0] = (ntb->vntb_pid << 16) | ntb->vntb_vid;
pci_vntb_table[0].vendor = ntb->vntb_vid;
pci_vntb_table[0].device = ntb->vntb_pid;
--
2.35.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v9 4/4] PCI: endpoint: Add vNTB MSI support
2022-09-07 3:48 ` [PATCH v9 4/4] PCI: endpoint: Add vNTB MSI support Frank Li
@ 2022-09-07 20:43 ` kernel test robot
2022-09-13 17:24 ` Manivannan Sadhasivam
1 sibling, 0 replies; 24+ messages in thread
From: kernel test robot @ 2022-09-07 20:43 UTC (permalink / raw)
To: Frank Li, maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo,
s.hauer, kw, bhelgaas
Cc: kbuild-all, linux-kernel, devicetree, linux-arm-kernel,
linux-pci, peng.fan, aisheng.dong, jdmason, kernel, festevam,
linux-imx, kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
Hi Frank,
I love your patch! Perhaps something to improve:
[auto build test WARNING on jonmason-ntb/ntb-next]
[also build test WARNING on robh/for-next linus/master v6.0-rc4 next-20220907]
[cannot apply to tip/irq/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Frank-Li/PCI-EP-driver-support-MSI-doorbell-from-host/20220907-115114
base: https://github.com/jonmason/ntb ntb-next
config: loongarch-randconfig-s052-20220906 (https://download.01.org/0day-ci/archive/20220908/202209080437.cNpaoZXx-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/0fe017b7ce1e4748acce80d9ddb81b3cd456adbb
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Frank-Li/PCI-EP-driver-support-MSI-doorbell-from-host/20220907-115114
git checkout 0fe017b7ce1e4748acce80d9ddb81b3cd456adbb
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/pci/endpoint/functions/pci-epf-vntb.c:567:25: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected void [noderef] __iomem *[assigned] mw_addr @@ got void * @@
drivers/pci/endpoint/functions/pci-epf-vntb.c:567:25: sparse: expected void [noderef] __iomem *[assigned] mw_addr
drivers/pci/endpoint/functions/pci-epf-vntb.c:567:25: sparse: got void *
drivers/pci/endpoint/functions/pci-epf-vntb.c:600:41: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected void *addr @@ got void [noderef] __iomem *epf_db @@
drivers/pci/endpoint/functions/pci-epf-vntb.c:600:41: sparse: expected void *addr
drivers/pci/endpoint/functions/pci-epf-vntb.c:600:41: sparse: got void [noderef] __iomem *epf_db
drivers/pci/endpoint/functions/pci-epf-vntb.c:1206:33: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void [noderef] __iomem *base @@ got struct epf_ntb_ctrl *reg @@
drivers/pci/endpoint/functions/pci-epf-vntb.c:1206:33: sparse: expected void [noderef] __iomem *base
drivers/pci/endpoint/functions/pci-epf-vntb.c:1206:33: sparse: got struct epf_ntb_ctrl *reg
drivers/pci/endpoint/functions/pci-epf-vntb.c:1217:33: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void [noderef] __iomem *base @@ got struct epf_ntb_ctrl *reg @@
drivers/pci/endpoint/functions/pci-epf-vntb.c:1217:33: sparse: expected void [noderef] __iomem *base
drivers/pci/endpoint/functions/pci-epf-vntb.c:1217:33: sparse: got struct epf_ntb_ctrl *reg
drivers/pci/endpoint/functions/pci-epf-vntb.c:1228:33: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void [noderef] __iomem *base @@ got struct epf_ntb_ctrl *reg @@
drivers/pci/endpoint/functions/pci-epf-vntb.c:1228:33: sparse: expected void [noderef] __iomem *base
drivers/pci/endpoint/functions/pci-epf-vntb.c:1228:33: sparse: got struct epf_ntb_ctrl *reg
drivers/pci/endpoint/functions/pci-epf-vntb.c:1240:33: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void [noderef] __iomem *base @@ got struct epf_ntb_ctrl *reg @@
drivers/pci/endpoint/functions/pci-epf-vntb.c:1240:33: sparse: expected void [noderef] __iomem *base
drivers/pci/endpoint/functions/pci-epf-vntb.c:1240:33: sparse: got struct epf_ntb_ctrl *reg
vim +567 drivers/pci/endpoint/functions/pci-epf-vntb.c
0fe017b7ce1e47 Frank Li 2022-09-06 536
e35f56bb03304a Frank Li 2022-02-22 537 /**
e35f56bb03304a Frank Li 2022-02-22 538 * epf_ntb_db_bar_init() - Configure Doorbell window BARs
e35f56bb03304a Frank Li 2022-02-22 539 * @ntb: NTB device that facilitates communication between HOST and vHOST
e35f56bb03304a Frank Li 2022-02-22 540 */
e35f56bb03304a Frank Li 2022-02-22 541 static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
e35f56bb03304a Frank Li 2022-02-22 542 {
e35f56bb03304a Frank Li 2022-02-22 543 const struct pci_epc_features *epc_features;
e35f56bb03304a Frank Li 2022-02-22 544 u32 align;
e35f56bb03304a Frank Li 2022-02-22 545 struct device *dev = &ntb->epf->dev;
e35f56bb03304a Frank Li 2022-02-22 546 int ret;
e35f56bb03304a Frank Li 2022-02-22 547 struct pci_epf_bar *epf_bar;
e35f56bb03304a Frank Li 2022-02-22 548 void __iomem *mw_addr;
e35f56bb03304a Frank Li 2022-02-22 549 enum pci_barno barno;
0fe017b7ce1e47 Frank Li 2022-09-06 550 size_t size;
e35f56bb03304a Frank Li 2022-02-22 551
e35f56bb03304a Frank Li 2022-02-22 552 epc_features = pci_epc_get_features(ntb->epf->epc,
e35f56bb03304a Frank Li 2022-02-22 553 ntb->epf->func_no,
e35f56bb03304a Frank Li 2022-02-22 554 ntb->epf->vfunc_no);
e35f56bb03304a Frank Li 2022-02-22 555 align = epc_features->align;
0fe017b7ce1e47 Frank Li 2022-09-06 556 size = epf_ntb_db_size(ntb);
e35f56bb03304a Frank Li 2022-02-22 557
e35f56bb03304a Frank Li 2022-02-22 558 barno = ntb->epf_ntb_bar[BAR_DB];
0fe017b7ce1e47 Frank Li 2022-09-06 559 epf_bar = &ntb->epf->bar[barno];
e35f56bb03304a Frank Li 2022-02-22 560
0fe017b7ce1e47 Frank Li 2022-09-06 561 if (ntb->epf_db_phys) {
0fe017b7ce1e47 Frank Li 2022-09-06 562 mw_addr = NULL;
0fe017b7ce1e47 Frank Li 2022-09-06 563 epf_bar->phys_addr = ntb->epf_db_phys;
0fe017b7ce1e47 Frank Li 2022-09-06 564 epf_bar->barno = barno;
0fe017b7ce1e47 Frank Li 2022-09-06 565 epf_bar->size = size;
0fe017b7ce1e47 Frank Li 2022-09-06 566 } else {
e35f56bb03304a Frank Li 2022-02-22 @567 mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
e35f56bb03304a Frank Li 2022-02-22 568 if (!mw_addr) {
0fe017b7ce1e47 Frank Li 2022-09-06 569 dev_err(dev, "Failed to allocate door bell address\n");
e35f56bb03304a Frank Li 2022-02-22 570 return -ENOMEM;
e35f56bb03304a Frank Li 2022-02-22 571 }
0fe017b7ce1e47 Frank Li 2022-09-06 572 }
e35f56bb03304a Frank Li 2022-02-22 573
e35f56bb03304a Frank Li 2022-02-22 574 ntb->epf_db = mw_addr;
e35f56bb03304a Frank Li 2022-02-22 575
e35f56bb03304a Frank Li 2022-02-22 576 ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no, epf_bar);
e35f56bb03304a Frank Li 2022-02-22 577 if (ret) {
e35f56bb03304a Frank Li 2022-02-22 578 dev_err(dev, "Doorbell BAR set failed\n");
e35f56bb03304a Frank Li 2022-02-22 579 goto err_alloc_peer_mem;
e35f56bb03304a Frank Li 2022-02-22 580 }
e35f56bb03304a Frank Li 2022-02-22 581 return ret;
e35f56bb03304a Frank Li 2022-02-22 582
e35f56bb03304a Frank Li 2022-02-22 583 err_alloc_peer_mem:
e35f56bb03304a Frank Li 2022-02-22 584 pci_epc_mem_free_addr(ntb->epf->epc, epf_bar->phys_addr, mw_addr, epf_bar->size);
e35f56bb03304a Frank Li 2022-02-22 585 return -1;
e35f56bb03304a Frank Li 2022-02-22 586 }
e35f56bb03304a Frank Li 2022-02-22 587
--
0-DAY CI Kernel Test Service
https://01.org/lkp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-07 3:48 ` [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver Frank Li
@ 2022-09-08 0:03 ` kernel test robot
2022-09-08 7:02 ` Marc Zyngier
2022-09-08 7:39 ` Marc Zyngier
1 sibling, 1 reply; 24+ messages in thread
From: kernel test robot @ 2022-09-08 0:03 UTC (permalink / raw)
To: Frank Li, maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo,
s.hauer, kw, bhelgaas
Cc: kbuild-all, linux-kernel, devicetree, linux-arm-kernel,
linux-pci, peng.fan, aisheng.dong, jdmason, kernel, festevam,
linux-imx, kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
Hi Frank,
I love your patch! Yet something to improve:
[auto build test ERROR on jonmason-ntb/ntb-next]
[also build test ERROR on robh/for-next linus/master v6.0-rc4 next-20220907]
[cannot apply to tip/irq/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Frank-Li/PCI-EP-driver-support-MSI-doorbell-from-host/20220907-115114
base: https://github.com/jonmason/ntb ntb-next
config: s390-randconfig-s033-20220907 (https://download.01.org/0day-ci/archive/20220908/202209080757.hQMfrrfm-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/c1f079e633c10b4f2f1f3c8f52e447d13fda8ddb
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Frank-Li/PCI-EP-driver-support-MSI-doorbell-from-host/20220907-115114
git checkout c1f079e633c10b4f2f1f3c8f52e447d13fda8ddb
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=s390 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
s390-linux-ld: drivers/irqchip/irq-imx-mu-msi.o: in function `imx_mu_of_init':
>> drivers/irqchip/irq-imx-mu-msi.c:316: undefined reference to `devm_platform_ioremap_resource_byname'
vim +316 drivers/irqchip/irq-imx-mu-msi.c
288
289 static int __init imx_mu_of_init(struct device_node *dn,
290 struct device_node *parent,
291 const struct imx_mu_dcfg *cfg
292 )
293 {
294 struct platform_device *pdev = of_find_device_by_node(dn);
295 struct device_link *pd_link_a;
296 struct device_link *pd_link_b;
297 struct imx_mu_msi *msi_data;
298 struct resource *res;
299 struct device *pd_a;
300 struct device *pd_b;
301 struct device *dev;
302 int ret;
303 int irq;
304
305 if (!pdev)
306 return -ENODEV;
307
308 dev = &pdev->dev;
309
310 msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
311 if (!msi_data)
312 return -ENOMEM;
313
314 msi_data->cfg = cfg;
315
> 316 msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "processor-a-side");
317 if (IS_ERR(msi_data->regs)) {
318 dev_err(&pdev->dev, "failed to initialize 'regs'\n");
319 return PTR_ERR(msi_data->regs);
320 }
321
322 res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "processor-b-side");
323 if (!res)
324 return -EIO;
325
326 msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
327
328 irq = platform_get_irq(pdev, 0);
329 if (irq <= 0)
330 return -ENODEV;
331
332 platform_set_drvdata(pdev, msi_data);
333
334 msi_data->clk = devm_clk_get(dev, NULL);
335 if (IS_ERR(msi_data->clk)) {
336 if (PTR_ERR(msi_data->clk) != -ENOENT)
337 return PTR_ERR(msi_data->clk);
338
339 msi_data->clk = NULL;
340 }
341
342 pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
343 if (IS_ERR(pd_a))
344 return PTR_ERR(pd_a);
345
346 pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
347 if (IS_ERR(pd_b))
348 return PTR_ERR(pd_b);
349
350 pd_link_a = device_link_add(dev, pd_a,
351 DL_FLAG_STATELESS |
352 DL_FLAG_PM_RUNTIME |
353 DL_FLAG_RPM_ACTIVE);
354
355 if (!pd_link_a) {
356 dev_err(dev, "Failed to add device_link to mu a.\n");
357 goto err_pd_a;
358 }
359
360 pd_link_b = device_link_add(dev, pd_b,
361 DL_FLAG_STATELESS |
362 DL_FLAG_PM_RUNTIME |
363 DL_FLAG_RPM_ACTIVE);
364
365
366 if (!pd_link_b) {
367 dev_err(dev, "Failed to add device_link to mu a.\n");
368 goto err_pd_b;
369 }
370
371 ret = imx_mu_msi_domains_init(msi_data, dev);
372 if (ret)
373 goto err_dm_init;
374
375 irq_set_chained_handler_and_data(irq,
376 imx_mu_msi_irq_handler,
377 msi_data);
378
379 pm_runtime_enable(dev);
380
381 return 0;
382
383 err_dm_init:
384 device_link_remove(dev, pd_b);
385 err_pd_b:
386 device_link_remove(dev, pd_a);
387 err_pd_a:
388 return -EINVAL;
389 }
390
--
0-DAY CI Kernel Test Service
https://01.org/lkp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-08 0:03 ` kernel test robot
@ 2022-09-08 7:02 ` Marc Zyngier
2022-09-08 14:26 ` [EXT] " Frank Li
0 siblings, 1 reply; 24+ messages in thread
From: Marc Zyngier @ 2022-09-08 7:02 UTC (permalink / raw)
To: kernel test robot, Frank Li
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, kbuild-all, linux-kernel, devicetree, linux-arm-kernel,
linux-pci, peng.fan, aisheng.dong, jdmason, kernel, festevam,
linux-imx, kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
Frank,
On Thu, 08 Sep 2022 01:03:27 +0100,
kernel test robot <lkp@intel.com> wrote:
>
> Hi Frank,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on jonmason-ntb/ntb-next]
> [also build test ERROR on robh/for-next linus/master v6.0-rc4 next-20220907]
> [cannot apply to tip/irq/core]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Frank-Li/PCI-EP-driver-support-MSI-doorbell-from-host/20220907-115114
> base: https://github.com/jonmason/ntb ntb-next
> config: s390-randconfig-s033-20220907 (https://download.01.org/0day-ci/archive/20220908/202209080757.hQMfrrfm-lkp@intel.com/config)
> compiler: s390-linux-gcc (GCC) 12.1.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # apt-get install sparse
> # sparse version: v0.6.4-39-gce1a6720-dirty
> # https://github.com/intel-lab-lkp/linux/commit/c1f079e633c10b4f2f1f3c8f52e447d13fda8ddb
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Frank-Li/PCI-EP-driver-support-MSI-doorbell-from-host/20220907-115114
> git checkout c1f079e633c10b4f2f1f3c8f52e447d13fda8ddb
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=s390 SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
> s390-linux-ld: drivers/irqchip/irq-imx-mu-msi.o: in function `imx_mu_of_init':
> >> drivers/irqchip/irq-imx-mu-msi.c:316: undefined reference to `devm_platform_ioremap_resource_byname'
This is about the 4th time this breakage gets reported. You keep
reposting this series without addressing it. What is it going to take
for you to finally fix it? Clearly, I'm not going to bother taking a
series that has pending build breakages.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-07 3:48 ` [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver Frank Li
2022-09-08 0:03 ` kernel test robot
@ 2022-09-08 7:39 ` Marc Zyngier
2022-09-08 14:23 ` [EXT] " Frank Li
` (2 more replies)
1 sibling, 3 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-09-08 7:39 UTC (permalink / raw)
To: Frank Li
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
peng.fan, aisheng.dong, jdmason, kernel, festevam, linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
On Wed, 07 Sep 2022 04:48:54 +0100,
Frank Li <Frank.Li@nxp.com> wrote:
>
> The MU block found in a number of Freescale/NXP SoCs supports generating
> IRQs by writing data to a register
>
> This enables the MU block to be used as a MSI controller, by leveraging
> the platform-MSI API
Missing full stop after each sentence.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> drivers/irqchip/Kconfig | 9 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-imx-mu-msi.c | 451 +++++++++++++++++++++++++++++++
> 3 files changed, 461 insertions(+)
> create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 5e4e50122777d..e04c6521dce55 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -470,6 +470,15 @@ config IMX_INTMUX
> help
> Support for the i.MX INTMUX interrupt multiplexer.
>
> +config IMX_MU_MSI
> + bool "i.MX MU work as MSI controller"
Why bool? Doesn't it also work as a module?
> + default y if ARCH_MXC
Why would this be selected by default?
> + select IRQ_DOMAIN
> + select IRQ_DOMAIN_HIERARCHY
> + select GENERIC_MSI_IRQ_DOMAIN
> + help
> + MU work as MSI controller to do general doorbell
I'm not sure this is that generic. It really is limited to CPU-to-CPU
interrupts.
> +
> config LS1X_IRQ
> bool "Loongson-1 Interrupt Controller"
> depends on MACH_LOONGSON32
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 5d8e21d3dc6d8..870423746c783 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o
> obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o
> obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o
> obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o
> +obj-$(CONFIG_IMX_MU_MSI) += irq-imx-mu-msi.o
> obj-$(CONFIG_MADERA_IRQ) += irq-madera.o
> obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o
> obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o
> diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-msi.c
> new file mode 100644
> index 0000000000000..82b55f6d87266
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-mu-msi.c
> @@ -0,0 +1,451 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Freescale MU worked as MSI controller
s/worked/used/
> + *
> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de>
> + * Copyright 2022 NXP
> + * Frank Li <Frank.Li@nxp.com>
> + * Peng Fan <peng.fan@nxp.com>
> + *
> + * Based on drivers/mailbox/imx-mailbox.c
> + */
> +#include <linux/clk.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/spinlock.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pm_domain.h>
Keep this list in alphabetical order.
> +
> +
> +#define IMX_MU_CHANS 4
> +
> +enum imx_mu_xcr {
> + IMX_MU_GIER,
> + IMX_MU_GCR,
> + IMX_MU_TCR,
> + IMX_MU_RCR,
> + IMX_MU_xCR_MAX,
What is this last enum used for?
> +};
> +
> +enum imx_mu_xsr {
> + IMX_MU_SR,
> + IMX_MU_GSR,
> + IMX_MU_TSR,
> + IMX_MU_RSR,
> +};
> +
> +enum imx_mu_type {
> + IMX_MU_V1 = BIT(0),
This is never used. Why?
> + IMX_MU_V2 = BIT(1),
> + IMX_MU_V2_S4 = BIT(15),
Same thing.
> +};
> +
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
> +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ? BIT(x) : BIT(24 + (3 - (x))))
> +
> +struct imx_mu_dcfg {
> + enum imx_mu_type type;
> + u32 xTR; /* Transmit Register0 */
> + u32 xRR; /* Receive Register0 */
> + u32 xSR[4]; /* Status Registers */
> + u32 xCR[4]; /* Control Registers */
> +};
> +
> +struct imx_mu_msi {
> + spinlock_t lock;
> + raw_spinlock_t reglock;
Why two locks? Isn't one enough to protect both MSI allocation (which
happens once in a blue moon) and register access?
Also, where are these locks initialised?
> + struct irq_domain *msi_domain;
> + void __iomem *regs;
> + phys_addr_t msiir_addr;
> + const struct imx_mu_dcfg *cfg;
> + unsigned long used;
> + struct clk *clk;
> +};
> +
> +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
> +{
> + iowrite32(val, msi_data->regs + offs);
> +}
> +
> +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> +{
> + return ioread32(msi_data->regs + offs);
> +}
> +
> +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum imx_mu_xcr type, u32 set, u32 clr)
> +{
> + unsigned long flags;
> + u32 val;
> +
> + raw_spin_lock_irqsave(&msi_data->reglock, flags);
> + val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> + val &= ~clr;
> + val |= set;
> + imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> + raw_spin_unlock_irqrestore(&msi_data->reglock, flags);
> +
> + return val;
> +}
> +
> +static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
> +{
> + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> +}
> +
> +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> +{
> + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> +}
> +
> +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> +{
> + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> +
> + imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> +}
> +
> +static struct irq_chip imx_mu_msi_irq_chip = {
> + .name = "MU-MSI",
> + .irq_ack = irq_chip_ack_parent,
> +};
> +
> +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> +};
> +
> +static struct msi_domain_info imx_mu_msi_domain_info = {
> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
> + .ops = &imx_mu_msi_irq_ops,
> + .chip = &imx_mu_msi_irq_chip,
> +};
> +
> +static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
> + struct msi_msg *msg)
> +{
> + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> + u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> +
> + msg->address_hi = upper_32_bits(addr);
> + msg->address_lo = lower_32_bits(addr);
> + msg->data = data->hwirq;
> +}
> +
> +static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
> + const struct cpumask *mask, bool force)
> +{
> + return -EINVAL;
> +}
> +
> +static struct irq_chip imx_mu_msi_parent_chip = {
> + .name = "MU",
> + .irq_mask = imx_mu_msi_parent_mask_irq,
> + .irq_unmask = imx_mu_msi_parent_unmask_irq,
> + .irq_ack = imx_mu_msi_parent_ack_irq,
> + .irq_compose_msi_msg = imx_mu_msi_parent_compose_msg,
> + .irq_set_affinity = imx_mu_msi_parent_set_affinity,
> +};
> +
> +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> + unsigned int virq,
> + unsigned int nr_irqs,
> + void *args)
> +{
> + struct imx_mu_msi *msi_data = domain->host_data;
> + unsigned long flags;
> + int pos, err = 0;
> +
> + WARN_ON(nr_irqs != 1);
> +
> + spin_lock_irqsave(&msi_data->lock, flags);
> + pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> + if (pos < IMX_MU_CHANS)
> + __set_bit(pos, &msi_data->used);
> + else
> + err = -ENOSPC;
> + spin_unlock_irqrestore(&msi_data->lock, flags);
> +
> + if (err)
> + return err;
> +
> + irq_domain_set_info(domain, virq, pos,
> + &imx_mu_msi_parent_chip, msi_data,
> + handle_edge_irq, NULL, NULL);
> + return 0;
> +}
> +
> +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs)
> +{
> + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&msi_data->lock, flags);
> + __clear_bit(d->hwirq, &msi_data->used);
> + spin_unlock_irqrestore(&msi_data->lock, flags);
> +}
> +
> +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> + .alloc = imx_mu_msi_domain_irq_alloc,
> + .free = imx_mu_msi_domain_irq_free,
> +};
> +
> +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> +{
> + struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + u32 status;
> + int i;
> +
> + status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> +
> + chained_irq_enter(chip, desc);
> + for (i = 0; i < IMX_MU_CHANS; i++) {
> + if (status & IMX_MU_xSR_RFn(msi_data, i))
> + generic_handle_domain_irq(msi_data->msi_domain, i);
> + }
> + chained_irq_exit(chip, desc);
> +}
> +
> +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct device *dev)
> +{
> + struct fwnode_handle *fwnodes = dev_fwnode(dev);
> + struct irq_domain *parent;
> +
> + /* Initialize MSI domain parent */
> + parent = irq_domain_create_linear(fwnodes,
> + IMX_MU_CHANS,
> + &imx_mu_msi_domain_ops,
> + msi_data);
> + if (!parent) {
> + dev_err(dev, "failed to create IRQ domain\n");
> + return -ENOMEM;
> + }
> +
> + irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
> +
> + msi_data->msi_domain = platform_msi_create_irq_domain(
> + fwnodes,
> + &imx_mu_msi_domain_info,
> + parent);
nit: move the first argument after the opening bracket (longer lines
are fine).
> +
> + if (!msi_data->msi_domain) {
> + dev_err(dev, "failed to create MSI domain\n");
> + irq_domain_remove(parent);
> + return -ENOMEM;
> + }
> +
> + irq_domain_set_pm_device(msi_data->msi_domain, dev);
> +
> + return 0;
> +}
> +
> +/* Register offset of different version MU IP */
> +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
Why doesn't this have a type?
> + .xTR = 0x0,
> + .xRR = 0x10,
> + .xSR = {0x20, 0x20, 0x20, 0x20},
Since you defined enums for all the register offsets, please be
consistent and use them everywhere:
.xSR = {
[IMX_MU_SR] = 0x20,
[IMX_MU_GSR] = 0x20,
[...]
},
> + .xCR = {0x24, 0x24, 0x24, 0x24},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> + .xTR = 0x20,
> + .xRR = 0x40,
> + .xSR = {0x60, 0x60, 0x60, 0x60},
> + .xCR = {0x64, 0x64, 0x64, 0x64},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> + .type = IMX_MU_V2,
> + .xTR = 0x200,
> + .xRR = 0x280,
> + .xSR = {0xC, 0x118, 0x124, 0x12C},
> + .xCR = {0x110, 0x114, 0x120, 0x128},
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> +
> + .type = IMX_MU_V2 | IMX_MU_V2_S4,
> + .xTR = 0x200,
> + .xRR = 0x280,
> + .xSR = {0xC, 0x118, 0x124, 0x12C},
> + .xCR = {0x110, 0x114, 0x120, 0x128},
> +};
> +
> +static int __init imx_mu_of_init(struct device_node *dn,
> + struct device_node *parent,
> + const struct imx_mu_dcfg *cfg
> + )
Move closing bracket after 'cfg'.
> +{
> + struct platform_device *pdev = of_find_device_by_node(dn);
> + struct device_link *pd_link_a;
> + struct device_link *pd_link_b;
> + struct imx_mu_msi *msi_data;
> + struct resource *res;
> + struct device *pd_a;
> + struct device *pd_b;
> + struct device *dev;
> + int ret;
> + int irq;
> +
> + if (!pdev)
> + return -ENODEV;
How can that happen?
> +
> + dev = &pdev->dev;
> +
> + msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> + if (!msi_data)
> + return -ENOMEM;
> +
> + msi_data->cfg = cfg;
> +
> + msi_data->regs = devm_platform_ioremap_resource_byname(pdev, "processor-a-side");
> + if (IS_ERR(msi_data->regs)) {
> + dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> + return PTR_ERR(msi_data->regs);
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "processor-b-side");
> + if (!res)
> + return -EIO;
> +
> + msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq <= 0)
> + return -ENODEV;
> +
> + platform_set_drvdata(pdev, msi_data);
> +
> + msi_data->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(msi_data->clk)) {
> + if (PTR_ERR(msi_data->clk) != -ENOENT)
> + return PTR_ERR(msi_data->clk);
> +
> + msi_data->clk = NULL;
Why is it acceptable to continue with no clock?
> + }
> +
> + pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
> + if (IS_ERR(pd_a))
> + return PTR_ERR(pd_a);
> +
> + pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
> + if (IS_ERR(pd_b))
> + return PTR_ERR(pd_b);
> +
> + pd_link_a = device_link_add(dev, pd_a,
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
> +
> + if (!pd_link_a) {
> + dev_err(dev, "Failed to add device_link to mu a.\n");
> + goto err_pd_a;
> + }
> +
> + pd_link_b = device_link_add(dev, pd_b,
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
> +
> +
> + if (!pd_link_b) {
> + dev_err(dev, "Failed to add device_link to mu a.\n");
> + goto err_pd_b;
> + }
> +
> + ret = imx_mu_msi_domains_init(msi_data, dev);
> + if (ret)
> + goto err_dm_init;
> +
> + irq_set_chained_handler_and_data(irq,
> + imx_mu_msi_irq_handler,
> + msi_data);
> +
> + pm_runtime_enable(dev);
Shouldn't you enable the device PM before registering the chained
handler?
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-08 7:39 ` Marc Zyngier
@ 2022-09-08 14:23 ` Frank Li
2022-09-08 14:51 ` Marc Zyngier
2022-09-09 14:52 ` Frank Li
2022-09-12 16:17 ` Frank Li
2 siblings, 1 reply; 24+ messages in thread
From: Frank Li @ 2022-09-08 14:23 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
Peng Fan, Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, September 8, 2022 2:40 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: tglx@linutronix.de; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kw@linux.com; bhelgaas@google.com; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan
> <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev; lznuaa@gmail.com; imx@lists.linux.dev;
> manivannan.sadhasivam@linaro.org
> Subject: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
>
> Caution: EXT Email
>
> On Wed, 07 Sep 2022 04:48:54 +0100,
> Frank Li <Frank.Li@nxp.com> wrote:
> >
> > The MU block found in a number of Freescale/NXP SoCs supports
> generating
> > IRQs by writing data to a register
> >
> > This enables the MU block to be used as a MSI controller, by leveraging
> > the platform-MSI API
>
> Missing full stop after each sentence.
[Frank Li] Do you means missed "."?
>
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > drivers/irqchip/Kconfig | 9 +
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-imx-mu-msi.c | 451
> +++++++++++++++++++++++++++++++
> > 3 files changed, 461 insertions(+)
> > create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 5e4e50122777d..e04c6521dce55 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -470,6 +470,15 @@ config IMX_INTMUX
> > help
> > Support for the i.MX INTMUX interrupt multiplexer.
> >
> > +config IMX_MU_MSI
> > + bool "i.MX MU work as MSI controller"
>
> Why bool? Doesn't it also work as a module?
[Frank Li] I remember you said that irq-chip can't be removed.
So I am not sure why need build as module.
>
> > + default y if ARCH_MXC
>
> Why would this be selected by default?
>
> > + select IRQ_DOMAIN
> > + select IRQ_DOMAIN_HIERARCHY
> > + select GENERIC_MSI_IRQ_DOMAIN
> > + help
> > + MU work as MSI controller to do general doorbell
>
> I'm not sure this is that generic. It really is limited to CPU-to-CPU
> interrupts.
>
> > +
> > config LS1X_IRQ
> > bool "Loongson-1 Interrupt Controller"
> > depends on MACH_LOONGSON32
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 5d8e21d3dc6d8..870423746c783 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o
> > obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o
> > obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o
> > obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o
> > +obj-$(CONFIG_IMX_MU_MSI) += irq-imx-mu-msi.o
> > obj-$(CONFIG_MADERA_IRQ) += irq-madera.o
> > obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o
> > obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o
> > diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-
> msi.c
> > new file mode 100644
> > index 0000000000000..82b55f6d87266
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > @@ -0,0 +1,451 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Freescale MU worked as MSI controller
>
> s/worked/used/
>
> > + *
> > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> <o.rempel@pengutronix.de>
> > + * Copyright 2022 NXP
> > + * Frank Li <Frank.Li@nxp.com>
> > + * Peng Fan <peng.fan@nxp.com>
> > + *
> > + * Based on drivers/mailbox/imx-mailbox.c
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/dma-iommu.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/pm_domain.h>
>
> Keep this list in alphabetical order.
>
> > +
> > +
> > +#define IMX_MU_CHANS 4
> > +
> > +enum imx_mu_xcr {
> > + IMX_MU_GIER,
> > + IMX_MU_GCR,
> > + IMX_MU_TCR,
> > + IMX_MU_RCR,
> > + IMX_MU_xCR_MAX,
>
> What is this last enum used for?
>
> > +};
> > +
> > +enum imx_mu_xsr {
> > + IMX_MU_SR,
> > + IMX_MU_GSR,
> > + IMX_MU_TSR,
> > + IMX_MU_RSR,
> > +};
> > +
> > +enum imx_mu_type {
> > + IMX_MU_V1 = BIT(0),
>
> This is never used. Why?
>
> > + IMX_MU_V2 = BIT(1),
> > + IMX_MU_V2_S4 = BIT(15),
>
> Same thing.
>
> > +};
> > +
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +
> > +struct imx_mu_dcfg {
> > + enum imx_mu_type type;
> > + u32 xTR; /* Transmit Register0 */
> > + u32 xRR; /* Receive Register0 */
> > + u32 xSR[4]; /* Status Registers */
> > + u32 xCR[4]; /* Control Registers */
> > +};
> > +
> > +struct imx_mu_msi {
> > + spinlock_t lock;
> > + raw_spinlock_t reglock;
>
> Why two locks? Isn't one enough to protect both MSI allocation (which
> happens once in a blue moon) and register access?
[Frank Li] Previously your comment, ask me to use raw_spinlock for read\write register access.
I don't think raw_spinlock is good for MSI allocation.
>
> Also, where are these locks initialised?
>
[Frank Li] struct imx_mu_msi is fill zero when allocated.
Does it still need additional initialization for spinlock?
> > + struct irq_domain *msi_domain;
> > + void __iomem *regs;
> > + phys_addr_t msiir_addr;
> > + const struct imx_mu_dcfg *cfg;
> > + unsigned long used;
> > + struct clk *clk;
> > +};
> > +
> > +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
> > +{
> > + iowrite32(val, msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> > +{
> > + return ioread32(msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum
> imx_mu_xcr type, u32 set, u32 clr)
> > +{
> > + unsigned long flags;
> > + u32 val;
> > +
> > + raw_spin_lock_irqsave(&msi_data->reglock, flags);
> > + val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> > + val &= ~clr;
> > + val |= set;
> > + imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> > + raw_spin_unlock_irqrestore(&msi_data->reglock, flags);
> > +
> > + return val;
> > +}
> > +
> > +static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> > +}
> > +
> > +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> > +}
> > +
> > +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_irq_chip = {
> > + .name = "MU-MSI",
> > + .irq_ack = irq_chip_ack_parent,
> > +};
> > +
> > +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> > +};
> > +
> > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS |
> MSI_FLAG_USE_DEF_CHIP_OPS),
> > + .ops = &imx_mu_msi_irq_ops,
> > + .chip = &imx_mu_msi_irq_chip,
> > +};
> > +
> > +static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
> > + struct msi_msg *msg)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > + u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> > +
> > + msg->address_hi = upper_32_bits(addr);
> > + msg->address_lo = lower_32_bits(addr);
> > + msg->data = data->hwirq;
> > +}
> > +
> > +static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
> > + const struct cpumask *mask, bool force)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_parent_chip = {
> > + .name = "MU",
> > + .irq_mask = imx_mu_msi_parent_mask_irq,
> > + .irq_unmask = imx_mu_msi_parent_unmask_irq,
> > + .irq_ack = imx_mu_msi_parent_ack_irq,
> > + .irq_compose_msi_msg = imx_mu_msi_parent_compose_msg,
> > + .irq_set_affinity = imx_mu_msi_parent_set_affinity,
> > +};
> > +
> > +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> > + unsigned int virq,
> > + unsigned int nr_irqs,
> > + void *args)
> > +{
> > + struct imx_mu_msi *msi_data = domain->host_data;
> > + unsigned long flags;
> > + int pos, err = 0;
> > +
> > + WARN_ON(nr_irqs != 1);
> > +
> > + spin_lock_irqsave(&msi_data->lock, flags);
> > + pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> > + if (pos < IMX_MU_CHANS)
> > + __set_bit(pos, &msi_data->used);
> > + else
> > + err = -ENOSPC;
> > + spin_unlock_irqrestore(&msi_data->lock, flags);
> > +
> > + if (err)
> > + return err;
> > +
> > + irq_domain_set_info(domain, virq, pos,
> > + &imx_mu_msi_parent_chip, msi_data,
> > + handle_edge_irq, NULL, NULL);
> > + return 0;
> > +}
> > +
> > +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> > + unsigned int virq, unsigned int nr_irqs)
> > +{
> > + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&msi_data->lock, flags);
> > + __clear_bit(d->hwirq, &msi_data->used);
> > + spin_unlock_irqrestore(&msi_data->lock, flags);
> > +}
> > +
> > +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> > + .alloc = imx_mu_msi_domain_irq_alloc,
> > + .free = imx_mu_msi_domain_irq_free,
> > +};
> > +
> > +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> > +{
> > + struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + u32 status;
> > + int i;
> > +
> > + status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> > +
> > + chained_irq_enter(chip, desc);
> > + for (i = 0; i < IMX_MU_CHANS; i++) {
> > + if (status & IMX_MU_xSR_RFn(msi_data, i))
> > + generic_handle_domain_irq(msi_data->msi_domain, i);
> > + }
> > + chained_irq_exit(chip, desc);
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct
> device *dev)
> > +{
> > + struct fwnode_handle *fwnodes = dev_fwnode(dev);
> > + struct irq_domain *parent;
> > +
> > + /* Initialize MSI domain parent */
> > + parent = irq_domain_create_linear(fwnodes,
> > + IMX_MU_CHANS,
> > + &imx_mu_msi_domain_ops,
> > + msi_data);
> > + if (!parent) {
> > + dev_err(dev, "failed to create IRQ domain\n");
> > + return -ENOMEM;
> > + }
> > +
> > + irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
> > +
> > + msi_data->msi_domain = platform_msi_create_irq_domain(
> > + fwnodes,
> > + &imx_mu_msi_domain_info,
> > + parent);
>
> nit: move the first argument after the opening bracket (longer lines
> are fine).
>
> > +
> > + if (!msi_data->msi_domain) {
> > + dev_err(dev, "failed to create MSI domain\n");
> > + irq_domain_remove(parent);
> > + return -ENOMEM;
> > + }
> > +
> > + irq_domain_set_pm_device(msi_data->msi_domain, dev);
> > +
> > + return 0;
> > +}
> > +
> > +/* Register offset of different version MU IP */
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
>
> Why doesn't this have a type?
>
> > + .xTR = 0x0,
> > + .xRR = 0x10,
> > + .xSR = {0x20, 0x20, 0x20, 0x20},
>
> Since you defined enums for all the register offsets, please be
> consistent and use them everywhere:
>
> .xSR = {
> [IMX_MU_SR] = 0x20,
> [IMX_MU_GSR] = 0x20,
> [...]
> },
>
> > + .xCR = {0x24, 0x24, 0x24, 0x24},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > + .xTR = 0x20,
> > + .xRR = 0x40,
> > + .xSR = {0x60, 0x60, 0x60, 0x60},
> > + .xCR = {0x64, 0x64, 0x64, 0x64},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > + .type = IMX_MU_V2,
> > + .xTR = 0x200,
> > + .xRR = 0x280,
> > + .xSR = {0xC, 0x118, 0x124, 0x12C},
> > + .xCR = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > +
> > + .type = IMX_MU_V2 | IMX_MU_V2_S4,
> > + .xTR = 0x200,
> > + .xRR = 0x280,
> > + .xSR = {0xC, 0x118, 0x124, 0x12C},
> > + .xCR = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static int __init imx_mu_of_init(struct device_node *dn,
> > + struct device_node *parent,
> > + const struct imx_mu_dcfg *cfg
> > + )
>
> Move closing bracket after 'cfg'.
>
> > +{
> > + struct platform_device *pdev = of_find_device_by_node(dn);
> > + struct device_link *pd_link_a;
> > + struct device_link *pd_link_b;
> > + struct imx_mu_msi *msi_data;
> > + struct resource *res;
> > + struct device *pd_a;
> > + struct device *pd_b;
> > + struct device *dev;
> > + int ret;
> > + int irq;
> > +
> > + if (!pdev)
> > + return -ENODEV;
>
> How can that happen?
>
[Frank Li] Not sure, many driver check as it.
> > +
> > + dev = &pdev->dev;
> > +
> > + msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> > + if (!msi_data)
> > + return -ENOMEM;
> > +
> > + msi_data->cfg = cfg;
> > +
> > + msi_data->regs = devm_platform_ioremap_resource_byname(pdev,
> "processor-a-side");
> > + if (IS_ERR(msi_data->regs)) {
> > + dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> > + return PTR_ERR(msi_data->regs);
> > + }
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "processor-b-side");
> > + if (!res)
> > + return -EIO;
> > +
> > + msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq <= 0)
> > + return -ENODEV;
> > +
> > + platform_set_drvdata(pdev, msi_data);
> > +
> > + msi_data->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(msi_data->clk)) {
> > + if (PTR_ERR(msi_data->clk) != -ENOENT)
> > + return PTR_ERR(msi_data->clk);
> > +
> > + msi_data->clk = NULL;
>
> Why is it acceptable to continue with no clock?
>
> > + }
> > +
> > + pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
> > + if (IS_ERR(pd_a))
> > + return PTR_ERR(pd_a);
> > +
> > + pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
> > + if (IS_ERR(pd_b))
> > + return PTR_ERR(pd_b);
> > +
> > + pd_link_a = device_link_add(dev, pd_a,
> > + DL_FLAG_STATELESS |
> > + DL_FLAG_PM_RUNTIME |
> > + DL_FLAG_RPM_ACTIVE);
> > +
> > + if (!pd_link_a) {
> > + dev_err(dev, "Failed to add device_link to mu a.\n");
> > + goto err_pd_a;
> > + }
> > +
> > + pd_link_b = device_link_add(dev, pd_b,
> > + DL_FLAG_STATELESS |
> > + DL_FLAG_PM_RUNTIME |
> > + DL_FLAG_RPM_ACTIVE);
> > +
> > +
> > + if (!pd_link_b) {
> > + dev_err(dev, "Failed to add device_link to mu a.\n");
> > + goto err_pd_b;
> > + }
> > +
> > + ret = imx_mu_msi_domains_init(msi_data, dev);
> > + if (ret)
> > + goto err_dm_init;
> > +
> > + irq_set_chained_handler_and_data(irq,
> > + imx_mu_msi_irq_handler,
> > + msi_data);
> > +
> > + pm_runtime_enable(dev);
>
> Shouldn't you enable the device PM before registering the chained
> handler?
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-08 7:02 ` Marc Zyngier
@ 2022-09-08 14:26 ` Frank Li
2022-09-08 14:35 ` Marc Zyngier
0 siblings, 1 reply; 24+ messages in thread
From: Frank Li @ 2022-09-08 14:26 UTC (permalink / raw)
To: Marc Zyngier, kernel test robot
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, kbuild-all, linux-kernel, devicetree, linux-arm-kernel,
linux-pci, Peng Fan, Aisheng Dong, jdmason, kernel, festevam,
dl-linux-imx, kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, September 8, 2022 2:02 AM
> To: kernel test robot <lkp@intel.com>; Frank Li <frank.li@nxp.com>
> Cc: tglx@linutronix.de; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kw@linux.com; bhelgaas@google.com; kbuild-
> all@lists.01.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan
> <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev; lznuaa@gmail.com; imx@lists.linux.dev;
> manivannan.sadhasivam@linaro.org
> Subject: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
>
> Caution: EXT Email
>
> Frank,
>
> On Thu, 08 Sep 2022 01:03:27 +0100,
> kernel test robot <lkp@intel.com> wrote:
> >
> > Hi Frank,
> >
> > I love your patch! Yet something to improve:
> >
> > [auto build test ERROR on jonmason-ntb/ntb-next]
> > [also build test ERROR on robh/for-next linus/master v6.0-rc4 next-
> 20220907]
> > [cannot apply to tip/irq/core]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit-
> scm.com%2Fdocs%2Fgit-format-
> patch%23_base_tree_information&data=05%7C01%7CFrank.Li%40nxp.c
> om%7Cc409668bc0994f1df11708da91681c6f%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C637982173598672297%7CUnknown%7CTWFpbGZsb
> 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D%7C3000%7C%7C%7C&sdata=LgRkCWQ%2BX0GLT5qUaLvIS9SuYMB
> 6tX%2FerPNl3KwW7Tc%3D&reserved=0]
> >
> > url:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2Fintel-lab-lkp%2Flinux%2Fcommits%2FFrank-Li%2FPCI-EP-driver-
> support-MSI-doorbell-from-host%2F20220907-
> 115114&data=05%7C01%7CFrank.Li%40nxp.com%7Cc409668bc0994f1d
> f11708da91681c6f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> 37982173598672297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7
> C&sdata=hpU6XQmibxCGo0S8J7VuPuZDhe6OwCzR92ld9UvODYw%3D&
> amp;reserved=0
> > base:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2Fjonmason%2Fntb&data=05%7C01%7CFrank.Li%40nxp.com%7C
> c409668bc0994f1df11708da91681c6f%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C637982173598672297%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C3000%7C%7C%7C&sdata=3y7%2BH6wcN%2FxcV8swP3QR0lUihWhqw
> qjlTtgUAVAPVg8%3D&reserved=0 ntb-next
> > config: s390-randconfig-s033-20220907
> (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdow
> nload.01.org%2F0day-
> ci%2Farchive%2F20220908%2F202209080757.hQMfrrfm-
> lkp%40intel.com%2Fconfig&data=05%7C01%7CFrank.Li%40nxp.com%7C
> c409668bc0994f1df11708da91681c6f%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C637982173598672297%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C3000%7C%7C%7C&sdata=lFaWJd2d3ob06d3qNilgFovocFU%2FN4Goz
> 7jBTXLvCss%3D&reserved=0)
> > compiler: s390-linux-gcc (GCC) 12.1.0
> > reproduce:
> > wget
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fraw.gi
> thubusercontent.com%2Fintel%2Flkp-
> tests%2Fmaster%2Fsbin%2Fmake.cross&data=05%7C01%7CFrank.Li%40
> nxp.com%7Cc409668bc0994f1df11708da91681c6f%7C686ea1d3bc2b4c6fa92
> cd99c5c301635%7C0%7C0%7C637982173598672297%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C3000%7C%7C%7C&sdata=8C2pQ57ym0gNi0wHXT6KEj8%
> 2BAgMQKph8UhK6GPo%2BNIc%3D&reserved=0 -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # apt-get install sparse
> > # sparse version: v0.6.4-39-gce1a6720-dirty
> > #
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2Fintel-lab-
> lkp%2Flinux%2Fcommit%2Fc1f079e633c10b4f2f1f3c8f52e447d13fda8ddb&a
> mp;data=05%7C01%7CFrank.Li%40nxp.com%7Cc409668bc0994f1df11708da9
> 1681c6f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379821735
> 98672297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sda
> ta=oBYu8TZJ15AxtGO2IdtGJdE80fYIJTwF4RlkBkeO6hA%3D&reserved=0
> > git remote add linux-review
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2Fintel-lab-
> lkp%2Flinux&data=05%7C01%7CFrank.Li%40nxp.com%7Cc409668bc099
> 4f1df11708da91681c6f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C637982173598672297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
> C%7C&sdata=AQn3FPjP13rnvvkA7TxRs3gpfd26siSoKM%2B%2Fwzw2J94
> %3D&reserved=0
> > git fetch --no-tags linux-review Frank-Li/PCI-EP-driver-support-MSI-
> doorbell-from-host/20220907-115114
> > git checkout c1f079e633c10b4f2f1f3c8f52e447d13fda8ddb
> > # save the config file
> > mkdir build_dir && cp config build_dir/.config
> > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0
> make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir
> ARCH=s390 SHELL=/bin/bash
> >
> > If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All errors (new ones prefixed by >>):
> >
> > s390-linux-ld: drivers/irqchip/irq-imx-mu-msi.o: in function
> `imx_mu_of_init':
> > >> drivers/irqchip/irq-imx-mu-msi.c:316: undefined reference to
> `devm_platform_ioremap_resource_byname'
>
> This is about the 4th time this breakage gets reported. You keep
> reposting this series without addressing it. What is it going to take
> for you to finally fix it? Clearly, I'm not going to bother taking a
> series that has pending build breakages.
[Frank Li] I also frustrate it now. Robot use random config and can't
Report all problems once. Recently update to gcc 12.x. Build broken
Happen at other place at my environment.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-08 14:26 ` [EXT] " Frank Li
@ 2022-09-08 14:35 ` Marc Zyngier
0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-09-08 14:35 UTC (permalink / raw)
To: Frank Li
Cc: kernel test robot, tglx, robh+dt, krzysztof.kozlowski+dt,
shawnguo, s.hauer, kw, bhelgaas, kbuild-all, linux-kernel,
devicetree, linux-arm-kernel, linux-pci, Peng Fan, Aisheng Dong,
jdmason, kernel, festevam, dl-linux-imx, kishon,
lorenzo.pieralisi, ntb, lznuaa, imx, manivannan.sadhasivam
On Thu, 08 Sep 2022 15:26:50 +0100,
Frank Li <frank.li@nxp.com> wrote:
>
> > > s390-linux-ld: drivers/irqchip/irq-imx-mu-msi.o: in function
> > `imx_mu_of_init':
> > > >> drivers/irqchip/irq-imx-mu-msi.c:316: undefined reference to
> > `devm_platform_ioremap_resource_byname'
> >
> > This is about the 4th time this breakage gets reported. You keep
> > reposting this series without addressing it. What is it going to take
> > for you to finally fix it? Clearly, I'm not going to bother taking a
> > series that has pending build breakages.
>
> [Frank Li] I also frustrate it now. Robot use random config and can't
> Report all problems once. Recently update to gcc 12.x. Build broken
> Happen at other place at my environment.
Well, that's your job to address them. Honestly, cross-compiling for a
few extra architectures isn't that hard.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-08 14:23 ` [EXT] " Frank Li
@ 2022-09-08 14:51 ` Marc Zyngier
2022-09-08 15:35 ` Frank Li
2022-09-12 15:53 ` Frank Li
0 siblings, 2 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-09-08 14:51 UTC (permalink / raw)
To: Frank Li
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
Peng Fan, Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
On Thu, 08 Sep 2022 15:23:53 +0100,
Frank Li <frank.li@nxp.com> wrote:
>
> >
> > On Wed, 07 Sep 2022 04:48:54 +0100,
> > Frank Li <Frank.Li@nxp.com> wrote:
> > >
> > > The MU block found in a number of Freescale/NXP SoCs supports
> > generating
> > > IRQs by writing data to a register
> > >
> > > This enables the MU block to be used as a MSI controller, by leveraging
> > > the platform-MSI API
> >
> > Missing full stop after each sentence.
>
> [Frank Li] Do you means missed "."?
Yes.
> > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > index 5e4e50122777d..e04c6521dce55 100644
> > > --- a/drivers/irqchip/Kconfig
> > > +++ b/drivers/irqchip/Kconfig
> > > @@ -470,6 +470,15 @@ config IMX_INTMUX
> > > help
> > > Support for the i.MX INTMUX interrupt multiplexer.
> > >
> > > +config IMX_MU_MSI
> > > + bool "i.MX MU work as MSI controller"
> >
> > Why bool? Doesn't it also work as a module?
>
> [Frank Li] I remember you said that irq-chip can't be removed.
> So I am not sure why need build as module.
Not being removed doesn't mean it cannot be built as a module and
loaded on demand. Why should I be forced to have this driver built-in
if my kernel is used on a variety of systems, only one of them having
this device?
> > > +
> > > +struct imx_mu_msi {
> > > + spinlock_t lock;
> > > + raw_spinlock_t reglock;
> >
> > Why two locks? Isn't one enough to protect both MSI allocation (which
> > happens once in a blue moon) and register access?
>
> [Frank Li] Previously your comment, ask me to use raw_spinlock for
> read\write register access. I don't think raw_spinlock is good for
> MSI allocation.
Why wouldn't it be good enough? I'd really like to know.
>
> >
> > Also, where are these locks initialised?
> >
>
> [Frank Li] struct imx_mu_msi is fill zero when allocated.
> Does it still need additional initialization for spinlock?
Have you heard of lockdep? Or CONFIG_DEBUG_SPINLOCK? Maybe you should
try it.
> > > + if (!pdev)
> > > + return -ENODEV;
> >
> > How can that happen?
> >
> [Frank Li] Not sure, many driver check as it.
And? Just because someone does something pointless, you have to
imitate them?
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-08 14:51 ` Marc Zyngier
@ 2022-09-08 15:35 ` Frank Li
2022-09-09 12:07 ` Marc Zyngier
2022-09-12 15:53 ` Frank Li
1 sibling, 1 reply; 24+ messages in thread
From: Frank Li @ 2022-09-08 15:35 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
Peng Fan, Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, September 8, 2022 9:52 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: tglx@linutronix.de; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kw@linux.com; bhelgaas@google.com; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan
> <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev; lznuaa@gmail.com; imx@lists.linux.dev;
> manivannan.sadhasivam@linaro.org
> Subject: Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller
> driver
>
> Caution: EXT Email
>
> On Thu, 08 Sep 2022 15:23:53 +0100,
> Frank Li <frank.li@nxp.com> wrote:
> >
> > >
> > > On Wed, 07 Sep 2022 04:48:54 +0100,
> > > Frank Li <Frank.Li@nxp.com> wrote:
> > > >
> > > > The MU block found in a number of Freescale/NXP SoCs supports
> > > generating
> > > > IRQs by writing data to a register
> > > >
> > > > This enables the MU block to be used as a MSI controller, by leveraging
> > > > the platform-MSI API
> > >
> > > Missing full stop after each sentence.
> >
> > [Frank Li] Do you means missed "."?
>
> Yes.
>
> > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > > index 5e4e50122777d..e04c6521dce55 100644
> > > > --- a/drivers/irqchip/Kconfig
> > > > +++ b/drivers/irqchip/Kconfig
> > > > @@ -470,6 +470,15 @@ config IMX_INTMUX
> > > > help
> > > > Support for the i.MX INTMUX interrupt multiplexer.
> > > >
> > > > +config IMX_MU_MSI
> > > > + bool "i.MX MU work as MSI controller"
> > >
> > > Why bool? Doesn't it also work as a module?
> >
> > [Frank Li] I remember you said that irq-chip can't be removed.
> > So I am not sure why need build as module.
>
> Not being removed doesn't mean it cannot be built as a module and
> loaded on demand. Why should I be forced to have this driver built-in
> if my kernel is used on a variety of systems, only one of them having
> this device?
>
> > > > +
> > > > +struct imx_mu_msi {
> > > > + spinlock_t lock;
> > > > + raw_spinlock_t reglock;
> > >
> > > Why two locks? Isn't one enough to protect both MSI allocation (which
> > > happens once in a blue moon) and register access?
> >
> > [Frank Li] Previously your comment, ask me to use raw_spinlock for
> > read\write register access. I don't think raw_spinlock is good for
> > MSI allocation.
>
> Why wouldn't it be good enough? I'd really like to know.[Frank Li] '
[Frank Li] According to my understand, raw_spinlock skip some lockdep
/debug feature to get better performance, which should be used when
Frequently call, such as irq handle\polling thread.
Spinlock have DEBUG feature to check wrong use lock. Allocate MSI generally
only is call once when driver probe.
The basic principle, lock should be used only when necessary. Access reg and
Allocate msi is totally independence events.
For this case, it is not big detail.
1. change spin_lock to raw_spin_lock at allocate msi function.
2. kept define spinlock_t lock;
In register modify function using
Raw_spin_lock(spinlock_check(lock), flags).
>
> >
> > >
> > > Also, where are these locks initialised?
> > >
> >
> > [Frank Li] struct imx_mu_msi is fill zero when allocated.
> > Does it still need additional initialization for spinlock?
>
> Have you heard of lockdep? Or CONFIG_DEBUG_SPINLOCK? Maybe you
> should
> try it.
>
> > > > + if (!pdev)
> > > > + return -ENODEV;
> > >
> > > How can that happen?
> > >
> > [Frank Li] Not sure, many driver check as it.
>
> And? Just because someone does something pointless, you have to
> imitate them?
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v9 3/4] dt-bindings: irqchip: imx mu work as msi controller
2022-09-07 3:48 ` [PATCH v9 3/4] dt-bindings: irqchip: imx mu work as msi controller Frank Li
@ 2022-09-09 1:43 ` Rob Herring
0 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2022-09-09 1:43 UTC (permalink / raw)
To: Frank Li
Cc: lorenzo.pieralisi, linux-pci, maz, ntb, robh+dt, kishon, imx,
bhelgaas, festevam, peng.fan, jdmason, manivannan.sadhasivam, kw,
aisheng.dong, kernel, shawnguo, linux-imx,
krzysztof.kozlowski+dt, s.hauer, devicetree, tglx, linux-kernel,
linux-arm-kernel, lznuaa
On Tue, 06 Sep 2022 22:48:55 -0500, Frank Li wrote:
> I.MX mu support generate irq by write a register. Provide msi controller
> support so other driver such as PCI EP can use it by standard msi
> interface as doorbell.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> .../interrupt-controller/fsl,mu-msi.yaml | 99 +++++++++++++++++++
> 1 file changed, 99 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,mu-msi.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-08 15:35 ` Frank Li
@ 2022-09-09 12:07 ` Marc Zyngier
2022-09-09 14:59 ` Frank Li
0 siblings, 1 reply; 24+ messages in thread
From: Marc Zyngier @ 2022-09-09 12:07 UTC (permalink / raw)
To: Frank Li
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
Peng Fan, Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
On Thu, 08 Sep 2022 16:35:20 +0100,
Frank Li <frank.li@nxp.com> wrote:
> > > > > +struct imx_mu_msi {
> > > > > + spinlock_t lock;
> > > > > + raw_spinlock_t reglock;
> > > >
> > > > Why two locks? Isn't one enough to protect both MSI allocation (which
> > > > happens once in a blue moon) and register access?
> > >
> > > [Frank Li] Previously your comment, ask me to use raw_spinlock for
> > > read\write register access. I don't think raw_spinlock is good for
> > > MSI allocation.
> >
> > Why wouldn't it be good enough? I'd really like to know.[Frank Li] '
>
> [Frank Li] According to my understand, raw_spinlock skip some lockdep
> /debug feature to get better performance, which should be used when
> Frequently call, such as irq handle\polling thread.
I'm afraid you are terribly misguided. They both have the same debug
features because they are both using the same core implementation, and
the only difference is whether this is preemptible for RT purposes or
not.
> Spinlock have DEBUG feature to check wrong use lock. Allocate MSI generally
> only is call once when driver probe.
Again, you should really read the code and the documentation and stop
making things up.
>
> The basic principle, lock should be used only when necessary. Access reg and
> Allocate msi is totally independence events.
Independent events that do not occur simultaneously. So no harm in
sharing the same lock.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-08 7:39 ` Marc Zyngier
2022-09-08 14:23 ` [EXT] " Frank Li
@ 2022-09-09 14:52 ` Frank Li
2022-09-10 14:40 ` Marc Zyngier
2022-09-12 16:17 ` Frank Li
2 siblings, 1 reply; 24+ messages in thread
From: Frank Li @ 2022-09-09 14:52 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
Peng Fan, Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, September 8, 2022 2:40 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: tglx@linutronix.de; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kw@linux.com; bhelgaas@google.com; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan
> <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev; lznuaa@gmail.com; imx@lists.linux.dev;
> manivannan.sadhasivam@linaro.org
> Subject: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
>
> Caution: EXT Email
>
> On Wed, 07 Sep 2022 04:48:54 +0100,
> Frank Li <Frank.Li@nxp.com> wrote:
> >
> > The MU block found in a number of Freescale/NXP SoCs supports
> generating
> > IRQs by writing data to a register
> >
> > This enables the MU block to be used as a MSI controller, by leveraging
> > the platform-MSI API
>
> Missing full stop after each sentence.
>
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > drivers/irqchip/Kconfig | 9 +
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-imx-mu-msi.c | 451
> +++++++++++++++++++++++++++++++
> > 3 files changed, 461 insertions(+)
> > create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 5e4e50122777d..e04c6521dce55 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -470,6 +470,15 @@ config IMX_INTMUX
> > help
> > Support for the i.MX INTMUX interrupt multiplexer.
> >
> > +config IMX_MU_MSI
> > + bool "i.MX MU work as MSI controller"
>
> Why bool? Doesn't it also work as a module?
>
> > + default y if ARCH_MXC
>
> Why would this be selected by default?
>
> > + select IRQ_DOMAIN
> > + select IRQ_DOMAIN_HIERARCHY
> > + select GENERIC_MSI_IRQ_DOMAIN
> > + help
> > + MU work as MSI controller to do general doorbell
>
> I'm not sure this is that generic. It really is limited to CPU-to-CPU
> interrupts.
[Frank Li] I think the only limitation is only 4 irq numbers.
The principle CPU to CPU irq is the same as MSI.
What's your preferred help description?
>
> > +
> > config LS1X_IRQ
> > bool "Loongson-1 Interrupt Controller"
> > depends on MACH_LOONGSON32
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 5d8e21d3dc6d8..870423746c783 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o
> > obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o
> > obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o
> > obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o
> > +obj-$(CONFIG_IMX_MU_MSI) += irq-imx-mu-msi.o
> > obj-$(CONFIG_MADERA_IRQ) += irq-madera.o
> > obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o
> > obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o
> > diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-
> msi.c
> > new file mode 100644
> > index 0000000000000..82b55f6d87266
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > @@ -0,0 +1,451 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Freescale MU worked as MSI controller
>
> s/worked/used/
>
> > + *
> > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> <o.rempel@pengutronix.de>
> > + * Copyright 2022 NXP
> > + * Frank Li <Frank.Li@nxp.com>
> > + * Peng Fan <peng.fan@nxp.com>
> > + *
> > + * Based on drivers/mailbox/imx-mailbox.c
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/dma-iommu.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/pm_domain.h>
>
> Keep this list in alphabetical order.
>
> > +
> > +
> > +#define IMX_MU_CHANS 4
> > +
> > +enum imx_mu_xcr {
> > + IMX_MU_GIER,
> > + IMX_MU_GCR,
> > + IMX_MU_TCR,
> > + IMX_MU_RCR,
> > + IMX_MU_xCR_MAX,
>
> What is this last enum used for?
>
> > +};
> > +
> > +enum imx_mu_xsr {
> > + IMX_MU_SR,
> > + IMX_MU_GSR,
> > + IMX_MU_TSR,
> > + IMX_MU_RSR,
> > +};
> > +
> > +enum imx_mu_type {
> > + IMX_MU_V1 = BIT(0),
>
> This is never used. Why?
>
> > + IMX_MU_V2 = BIT(1),
> > + IMX_MU_V2_S4 = BIT(15),
>
> Same thing.
>
> > +};
> > +
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +
> > +struct imx_mu_dcfg {
> > + enum imx_mu_type type;
> > + u32 xTR; /* Transmit Register0 */
> > + u32 xRR; /* Receive Register0 */
> > + u32 xSR[4]; /* Status Registers */
> > + u32 xCR[4]; /* Control Registers */
> > +};
> > +
> > +struct imx_mu_msi {
> > + spinlock_t lock;
> > + raw_spinlock_t reglock;
>
> Why two locks? Isn't one enough to protect both MSI allocation (which
> happens once in a blue moon) and register access?
>
> Also, where are these locks initialised?
>
> > + struct irq_domain *msi_domain;
> > + void __iomem *regs;
> > + phys_addr_t msiir_addr;
> > + const struct imx_mu_dcfg *cfg;
> > + unsigned long used;
> > + struct clk *clk;
> > +};
> > +
> > +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
> > +{
> > + iowrite32(val, msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> > +{
> > + return ioread32(msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum
> imx_mu_xcr type, u32 set, u32 clr)
> > +{
> > + unsigned long flags;
> > + u32 val;
> > +
> > + raw_spin_lock_irqsave(&msi_data->reglock, flags);
> > + val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> > + val &= ~clr;
> > + val |= set;
> > + imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> > + raw_spin_unlock_irqrestore(&msi_data->reglock, flags);
> > +
> > + return val;
> > +}
> > +
> > +static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> > +}
> > +
> > +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> > +}
> > +
> > +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_irq_chip = {
> > + .name = "MU-MSI",
> > + .irq_ack = irq_chip_ack_parent,
> > +};
> > +
> > +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> > +};
> > +
> > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS |
> MSI_FLAG_USE_DEF_CHIP_OPS),
> > + .ops = &imx_mu_msi_irq_ops,
> > + .chip = &imx_mu_msi_irq_chip,
> > +};
> > +
> > +static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
> > + struct msi_msg *msg)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > + u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> > +
> > + msg->address_hi = upper_32_bits(addr);
> > + msg->address_lo = lower_32_bits(addr);
> > + msg->data = data->hwirq;
> > +}
> > +
> > +static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
> > + const struct cpumask *mask, bool force)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_parent_chip = {
> > + .name = "MU",
> > + .irq_mask = imx_mu_msi_parent_mask_irq,
> > + .irq_unmask = imx_mu_msi_parent_unmask_irq,
> > + .irq_ack = imx_mu_msi_parent_ack_irq,
> > + .irq_compose_msi_msg = imx_mu_msi_parent_compose_msg,
> > + .irq_set_affinity = imx_mu_msi_parent_set_affinity,
> > +};
> > +
> > +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> > + unsigned int virq,
> > + unsigned int nr_irqs,
> > + void *args)
> > +{
> > + struct imx_mu_msi *msi_data = domain->host_data;
> > + unsigned long flags;
> > + int pos, err = 0;
> > +
> > + WARN_ON(nr_irqs != 1);
> > +
> > + spin_lock_irqsave(&msi_data->lock, flags);
> > + pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> > + if (pos < IMX_MU_CHANS)
> > + __set_bit(pos, &msi_data->used);
> > + else
> > + err = -ENOSPC;
> > + spin_unlock_irqrestore(&msi_data->lock, flags);
> > +
> > + if (err)
> > + return err;
> > +
> > + irq_domain_set_info(domain, virq, pos,
> > + &imx_mu_msi_parent_chip, msi_data,
> > + handle_edge_irq, NULL, NULL);
> > + return 0;
> > +}
> > +
> > +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> > + unsigned int virq, unsigned int nr_irqs)
> > +{
> > + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&msi_data->lock, flags);
> > + __clear_bit(d->hwirq, &msi_data->used);
> > + spin_unlock_irqrestore(&msi_data->lock, flags);
> > +}
> > +
> > +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> > + .alloc = imx_mu_msi_domain_irq_alloc,
> > + .free = imx_mu_msi_domain_irq_free,
> > +};
> > +
> > +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> > +{
> > + struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + u32 status;
> > + int i;
> > +
> > + status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> > +
> > + chained_irq_enter(chip, desc);
> > + for (i = 0; i < IMX_MU_CHANS; i++) {
> > + if (status & IMX_MU_xSR_RFn(msi_data, i))
> > + generic_handle_domain_irq(msi_data->msi_domain, i);
> > + }
> > + chained_irq_exit(chip, desc);
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct
> device *dev)
> > +{
> > + struct fwnode_handle *fwnodes = dev_fwnode(dev);
> > + struct irq_domain *parent;
> > +
> > + /* Initialize MSI domain parent */
> > + parent = irq_domain_create_linear(fwnodes,
> > + IMX_MU_CHANS,
> > + &imx_mu_msi_domain_ops,
> > + msi_data);
> > + if (!parent) {
> > + dev_err(dev, "failed to create IRQ domain\n");
> > + return -ENOMEM;
> > + }
> > +
> > + irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
> > +
> > + msi_data->msi_domain = platform_msi_create_irq_domain(
> > + fwnodes,
> > + &imx_mu_msi_domain_info,
> > + parent);
>
> nit: move the first argument after the opening bracket (longer lines
> are fine).
>
> > +
> > + if (!msi_data->msi_domain) {
> > + dev_err(dev, "failed to create MSI domain\n");
> > + irq_domain_remove(parent);
> > + return -ENOMEM;
> > + }
> > +
> > + irq_domain_set_pm_device(msi_data->msi_domain, dev);
> > +
> > + return 0;
> > +}
> > +
> > +/* Register offset of different version MU IP */
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
>
> Why doesn't this have a type?
>
> > + .xTR = 0x0,
> > + .xRR = 0x10,
> > + .xSR = {0x20, 0x20, 0x20, 0x20},
>
> Since you defined enums for all the register offsets, please be
> consistent and use them everywhere:
>
> .xSR = {
> [IMX_MU_SR] = 0x20,
> [IMX_MU_GSR] = 0x20,
> [...]
> },
>
> > + .xCR = {0x24, 0x24, 0x24, 0x24},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > + .xTR = 0x20,
> > + .xRR = 0x40,
> > + .xSR = {0x60, 0x60, 0x60, 0x60},
> > + .xCR = {0x64, 0x64, 0x64, 0x64},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > + .type = IMX_MU_V2,
> > + .xTR = 0x200,
> > + .xRR = 0x280,
> > + .xSR = {0xC, 0x118, 0x124, 0x12C},
> > + .xCR = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > +
> > + .type = IMX_MU_V2 | IMX_MU_V2_S4,
> > + .xTR = 0x200,
> > + .xRR = 0x280,
> > + .xSR = {0xC, 0x118, 0x124, 0x12C},
> > + .xCR = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static int __init imx_mu_of_init(struct device_node *dn,
> > + struct device_node *parent,
> > + const struct imx_mu_dcfg *cfg
> > + )
>
> Move closing bracket after 'cfg'.
>
> > +{
> > + struct platform_device *pdev = of_find_device_by_node(dn);
> > + struct device_link *pd_link_a;
> > + struct device_link *pd_link_b;
> > + struct imx_mu_msi *msi_data;
> > + struct resource *res;
> > + struct device *pd_a;
> > + struct device *pd_b;
> > + struct device *dev;
> > + int ret;
> > + int irq;
> > +
> > + if (!pdev)
> > + return -ENODEV;
>
> How can that happen?
>
> > +
> > + dev = &pdev->dev;
> > +
> > + msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> > + if (!msi_data)
> > + return -ENOMEM;
> > +
> > + msi_data->cfg = cfg;
> > +
> > + msi_data->regs = devm_platform_ioremap_resource_byname(pdev,
> "processor-a-side");
> > + if (IS_ERR(msi_data->regs)) {
> > + dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> > + return PTR_ERR(msi_data->regs);
> > + }
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "processor-b-side");
> > + if (!res)
> > + return -EIO;
> > +
> > + msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq <= 0)
> > + return -ENODEV;
> > +
> > + platform_set_drvdata(pdev, msi_data);
> > +
> > + msi_data->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(msi_data->clk)) {
> > + if (PTR_ERR(msi_data->clk) != -ENOENT)
> > + return PTR_ERR(msi_data->clk);
> > +
> > + msi_data->clk = NULL;
>
> Why is it acceptable to continue with no clock?
>
> > + }
> > +
> > + pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
> > + if (IS_ERR(pd_a))
> > + return PTR_ERR(pd_a);
> > +
> > + pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
> > + if (IS_ERR(pd_b))
> > + return PTR_ERR(pd_b);
> > +
> > + pd_link_a = device_link_add(dev, pd_a,
> > + DL_FLAG_STATELESS |
> > + DL_FLAG_PM_RUNTIME |
> > + DL_FLAG_RPM_ACTIVE);
> > +
> > + if (!pd_link_a) {
> > + dev_err(dev, "Failed to add device_link to mu a.\n");
> > + goto err_pd_a;
> > + }
> > +
> > + pd_link_b = device_link_add(dev, pd_b,
> > + DL_FLAG_STATELESS |
> > + DL_FLAG_PM_RUNTIME |
> > + DL_FLAG_RPM_ACTIVE);
> > +
> > +
> > + if (!pd_link_b) {
> > + dev_err(dev, "Failed to add device_link to mu a.\n");
> > + goto err_pd_b;
> > + }
> > +
> > + ret = imx_mu_msi_domains_init(msi_data, dev);
> > + if (ret)
> > + goto err_dm_init;
> > +
> > + irq_set_chained_handler_and_data(irq,
> > + imx_mu_msi_irq_handler,
> > + msi_data);
> > +
> > + pm_runtime_enable(dev);
>
> Shouldn't you enable the device PM before registering the chained
> handler?
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-09 12:07 ` Marc Zyngier
@ 2022-09-09 14:59 ` Frank Li
2022-09-10 14:35 ` Marc Zyngier
0 siblings, 1 reply; 24+ messages in thread
From: Frank Li @ 2022-09-09 14:59 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
Peng Fan, Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Friday, September 9, 2022 7:08 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: tglx@linutronix.de; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kw@linux.com; bhelgaas@google.com; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan
> <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev; lznuaa@gmail.com; imx@lists.linux.dev;
> manivannan.sadhasivam@linaro.org
> Subject: Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller
> driver
>
> Caution: EXT Email
>
> On Thu, 08 Sep 2022 16:35:20 +0100,
> Frank Li <frank.li@nxp.com> wrote:
>
> > > > > > +struct imx_mu_msi {
> > > > > > + spinlock_t lock;
> > > > > > + raw_spinlock_t reglock;
> > > > >
> > > > > Why two locks? Isn't one enough to protect both MSI allocation
> (which
> > > > > happens once in a blue moon) and register access?
> > > >
> > > > [Frank Li] Previously your comment, ask me to use raw_spinlock for
> > > > read\write register access. I don't think raw_spinlock is good for
> > > > MSI allocation.
> > >
> > > Why wouldn't it be good enough? I'd really like to know.[Frank Li] '
> >
> > [Frank Li] According to my understand, raw_spinlock skip some lockdep
> > /debug feature to get better performance, which should be used when
> > Frequently call, such as irq handle\polling thread.
>
> I'm afraid you are terribly misguided. They both have the same debug
> features because they are both using the same core implementation, and
> the only difference is whether this is preemptible for RT purposes or
> not.
>
> > Spinlock have DEBUG feature to check wrong use lock. Allocate MSI
> generally
> > only is call once when driver probe.
>
> Again, you should really read the code and the documentation and stop
> making things up.
[Frank Li] Thanks. You give me the correct direction. Some stackoverflow's
Doc was misleaded. I double checked spin_lock implementation. PREEMPT_RT
Kernel map spin_lock to rt_mutex.
I am curious why exist spin_lock_irqsave and raw_spin_lock_irqsave before
PREEMTP_RT merge into kernel tree.
>
> >
> > The basic principle, lock should be used only when necessary. Access reg
> and
> > Allocate msi is totally independence events.
>
> Independent events that do not occur simultaneously. So no harm in
> sharing the same lock.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-09 14:59 ` Frank Li
@ 2022-09-10 14:35 ` Marc Zyngier
0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-09-10 14:35 UTC (permalink / raw)
To: Frank Li
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
Peng Fan, Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
On Fri, 09 Sep 2022 15:59:01 +0100,
Frank Li <frank.li@nxp.com> wrote:
>
> > >
> > > [Frank Li] According to my understand, raw_spinlock skip some lockdep
> > > /debug feature to get better performance, which should be used when
> > > Frequently call, such as irq handle\polling thread.
> >
> > I'm afraid you are terribly misguided. They both have the same debug
> > features because they are both using the same core implementation, and
> > the only difference is whether this is preemptible for RT purposes or
> > not.
> >
> > > Spinlock have DEBUG feature to check wrong use lock. Allocate MSI
> > generally
> > > only is call once when driver probe.
> >
> > Again, you should really read the code and the documentation and stop
> > making things up.
>
> [Frank Li] Thanks. You give me the correct direction. Some stackoverflow's
> Doc was misleaded. I double checked spin_lock implementation. PREEMPT_RT
> Kernel map spin_lock to rt_mutex.
>
> I am curious why exist spin_lock_irqsave and raw_spin_lock_irqsave before
> PREEMTP_RT merge into kernel tree.
Because the RT merge has been going on for 10 years or so, long before
CONFIG_RT was merged. Also, a mutex has a spin lock at its core, and
it makes sense to have a single primitive for all these lock types.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-09 14:52 ` Frank Li
@ 2022-09-10 14:40 ` Marc Zyngier
0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-09-10 14:40 UTC (permalink / raw)
To: Frank Li
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
Peng Fan, Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
On Fri, 09 Sep 2022 15:52:45 +0100,
Frank Li <frank.li@nxp.com> wrote:
>
> > > + select IRQ_DOMAIN
> > > + select IRQ_DOMAIN_HIERARCHY
> > > + select GENERIC_MSI_IRQ_DOMAIN
> > > + help
> > > + MU work as MSI controller to do general doorbell
> >
> > I'm not sure this is that generic. It really is limited to CPU-to-CPU
> > interrupts.
>
> [Frank Li] I think the only limitation is only 4 irq numbers.
> The principle CPU to CPU irq is the same as MSI.
Not quite. Normal MSIs are device-to-CPU. CPU-to-CPU are normally
IPIs, and this one falls in the middle.
> What's your preferred help description?
<quote>
Provide a driver for the MU block used as a CPU-to-CPU MSI
controller. This requires a specially crafted DT to make use
of this driver.
If unsure, say N.
</quote>
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-08 14:51 ` Marc Zyngier
2022-09-08 15:35 ` Frank Li
@ 2022-09-12 15:53 ` Frank Li
2022-09-13 17:44 ` Marc Zyngier
1 sibling, 1 reply; 24+ messages in thread
From: Frank Li @ 2022-09-12 15:53 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
Peng Fan, Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, September 8, 2022 9:52 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: tglx@linutronix.de; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kw@linux.com; bhelgaas@google.com; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan
> <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev; lznuaa@gmail.com; imx@lists.linux.dev;
> manivannan.sadhasivam@linaro.org
> Subject: Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller
> driver
>
> Caution: EXT Email
>
> On Thu, 08 Sep 2022 15:23:53 +0100,
> Frank Li <frank.li@nxp.com> wrote:
> >
> > >
> > > On Wed, 07 Sep 2022 04:48:54 +0100,
> > > Frank Li <Frank.Li@nxp.com> wrote:
> > > >
> > > > The MU block found in a number of Freescale/NXP SoCs supports
> > > generating
> > > > IRQs by writing data to a register
> > > >
> > > > This enables the MU block to be used as a MSI controller, by leveraging
> > > > the platform-MSI API
> > >
> > > Missing full stop after each sentence.
> >
> > [Frank Li] Do you means missed "."?
>
> Yes.
>
> > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > > index 5e4e50122777d..e04c6521dce55 100644
> > > > --- a/drivers/irqchip/Kconfig
> > > > +++ b/drivers/irqchip/Kconfig
> > > > @@ -470,6 +470,15 @@ config IMX_INTMUX
> > > > help
> > > > Support for the i.MX INTMUX interrupt multiplexer.
> > > >
> > > > +config IMX_MU_MSI
> > > > + bool "i.MX MU work as MSI controller"
> > >
> > > Why bool? Doesn't it also work as a module?
> >
> > [Frank Li] I remember you said that irq-chip can't be removed.
> > So I am not sure why need build as module.
>
> Not being removed doesn't mean it cannot be built as a module and
> loaded on demand. Why should I be forced to have this driver built-in
> if my kernel is used on a variety of systems, only one of them having
> this device?
[Frank Li] A problem, platform_msi_create_irq_domain have NOT export to let module
Call it. https://elixir.bootlin.com/linux/latest/source/drivers/base/platform-msi.c#L122
Do you want to me add EXPORT_SYMBOL_GPL for it OR keep "bool" here?
>
> > > > +
> > > > +struct imx_mu_msi {
> > > > + spinlock_t lock;
> > > > + raw_spinlock_t reglock;
> > >
> > > Why two locks? Isn't one enough to protect both MSI allocation (which
> > > happens once in a blue moon) and register access?
> >
> > [Frank Li] Previously your comment, ask me to use raw_spinlock for
> > read\write register access. I don't think raw_spinlock is good for
> > MSI allocation.
>
> Why wouldn't it be good enough? I'd really like to know.
>
> >
> > >
> > > Also, where are these locks initialised?
> > >
> >
> > [Frank Li] struct imx_mu_msi is fill zero when allocated.
> > Does it still need additional initialization for spinlock?
>
> Have you heard of lockdep? Or CONFIG_DEBUG_SPINLOCK? Maybe you
> should
> try it.
>
> > > > + if (!pdev)
> > > > + return -ENODEV;
> > >
> > > How can that happen?
> > >
> > [Frank Li] Not sure, many driver check as it.
>
> And? Just because someone does something pointless, you have to
> imitate them?
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-08 7:39 ` Marc Zyngier
2022-09-08 14:23 ` [EXT] " Frank Li
2022-09-09 14:52 ` Frank Li
@ 2022-09-12 16:17 ` Frank Li
2 siblings, 0 replies; 24+ messages in thread
From: Frank Li @ 2022-09-12 16:17 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
Peng Fan, Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, September 8, 2022 2:40 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: tglx@linutronix.de; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kw@linux.com; bhelgaas@google.com; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pci@vger.kernel.org; Peng Fan
> <peng.fan@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> jdmason@kudzu.us; kernel@pengutronix.de; festevam@gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kishon@ti.com; lorenzo.pieralisi@arm.com;
> ntb@lists.linux.dev; lznuaa@gmail.com; imx@lists.linux.dev;
> manivannan.sadhasivam@linaro.org
> Subject: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
>
> Caution: EXT Email
>
> On Wed, 07 Sep 2022 04:48:54 +0100,
> Frank Li <Frank.Li@nxp.com> wrote:
> >
> > The MU block found in a number of Freescale/NXP SoCs supports
> generating
> > IRQs by writing data to a register
> >
> > This enables the MU block to be used as a MSI controller, by leveraging
> > the platform-MSI API
>
> Missing full stop after each sentence.
>
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > drivers/irqchip/Kconfig | 9 +
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-imx-mu-msi.c | 451
> +++++++++++++++++++++++++++++++
> > 3 files changed, 461 insertions(+)
> > create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 5e4e50122777d..e04c6521dce55 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -470,6 +470,15 @@ config IMX_INTMUX
> > help
> > Support for the i.MX INTMUX interrupt multiplexer.
> >
> > +config IMX_MU_MSI
> > + bool "i.MX MU work as MSI controller"
>
> Why bool? Doesn't it also work as a module?
>
> > + default y if ARCH_MXC
>
> Why would this be selected by default?
>
> > + select IRQ_DOMAIN
> > + select IRQ_DOMAIN_HIERARCHY
> > + select GENERIC_MSI_IRQ_DOMAIN
> > + help
> > + MU work as MSI controller to do general doorbell
>
> I'm not sure this is that generic. It really is limited to CPU-to-CPU
> interrupts.
>
> > +
> > config LS1X_IRQ
> > bool "Loongson-1 Interrupt Controller"
> > depends on MACH_LOONGSON32
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 5d8e21d3dc6d8..870423746c783 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o
> > obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o
> > obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o
> > obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o
> > +obj-$(CONFIG_IMX_MU_MSI) += irq-imx-mu-msi.o
> > obj-$(CONFIG_MADERA_IRQ) += irq-madera.o
> > obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o
> > obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o
> > diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-mu-
> msi.c
> > new file mode 100644
> > index 0000000000000..82b55f6d87266
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > @@ -0,0 +1,451 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Freescale MU worked as MSI controller
>
> s/worked/used/
>
> > + *
> > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> <o.rempel@pengutronix.de>
> > + * Copyright 2022 NXP
> > + * Frank Li <Frank.Li@nxp.com>
> > + * Peng Fan <peng.fan@nxp.com>
> > + *
> > + * Based on drivers/mailbox/imx-mailbox.c
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/dma-iommu.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/pm_domain.h>
>
> Keep this list in alphabetical order.
>
> > +
> > +
> > +#define IMX_MU_CHANS 4
> > +
> > +enum imx_mu_xcr {
> > + IMX_MU_GIER,
> > + IMX_MU_GCR,
> > + IMX_MU_TCR,
> > + IMX_MU_RCR,
> > + IMX_MU_xCR_MAX,
>
> What is this last enum used for?
[Frank Li] I will replace 4 in "u32 xCR[4]; " with IMX_MU_xCR_MAX
>
> > +};
> > +
> > +enum imx_mu_xsr {
> > + IMX_MU_SR,
> > + IMX_MU_GSR,
> > + IMX_MU_TSR,
> > + IMX_MU_RSR,
> > +};
> > +
> > +enum imx_mu_type {
> > + IMX_MU_V1 = BIT(0),
>
> This is never used. Why?
>
> > + IMX_MU_V2 = BIT(1),
> > + IMX_MU_V2_S4 = BIT(15),
>
> Same thing.
>
> > +};
> > +
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +#define IMX_MU_xSR_RFn(data, x) ((data->cfg->type) & IMX_MU_V2 ?
> BIT(x) : BIT(24 + (3 - (x))))
> > +
> > +struct imx_mu_dcfg {
> > + enum imx_mu_type type;
> > + u32 xTR; /* Transmit Register0 */
> > + u32 xRR; /* Receive Register0 */
> > + u32 xSR[4]; /* Status Registers */
> > + u32 xCR[4]; /* Control Registers */
> > +};
> > +
> > +struct imx_mu_msi {
> > + spinlock_t lock;
> > + raw_spinlock_t reglock;
>
> Why two locks? Isn't one enough to protect both MSI allocation (which
> happens once in a blue moon) and register access?
>
> Also, where are these locks initialised?
>
> > + struct irq_domain *msi_domain;
> > + void __iomem *regs;
> > + phys_addr_t msiir_addr;
> > + const struct imx_mu_dcfg *cfg;
> > + unsigned long used;
> > + struct clk *clk;
> > +};
> > +
> > +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32 offs)
> > +{
> > + iowrite32(val, msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> > +{
> > + return ioread32(msi_data->regs + offs);
> > +}
> > +
> > +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum
> imx_mu_xcr type, u32 set, u32 clr)
> > +{
> > + unsigned long flags;
> > + u32 val;
> > +
> > + raw_spin_lock_irqsave(&msi_data->reglock, flags);
> > + val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> > + val &= ~clr;
> > + val |= set;
> > + imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> > + raw_spin_unlock_irqrestore(&msi_data->reglock, flags);
> > +
> > + return val;
> > +}
> > +
> > +static void imx_mu_msi_parent_mask_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq));
> > +}
> > +
> > +static void imx_mu_msi_parent_unmask_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> IMX_MU_xCR_RIEn(msi_data, data->hwirq), 0);
> > +}
> > +
> > +static void imx_mu_msi_parent_ack_irq(struct irq_data *data)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > +
> > + imx_mu_read(msi_data, msi_data->cfg->xRR + data->hwirq * 4);
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_irq_chip = {
> > + .name = "MU-MSI",
> > + .irq_ack = irq_chip_ack_parent,
> > +};
> > +
> > +static struct msi_domain_ops imx_mu_msi_irq_ops = {
> > +};
> > +
> > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS |
> MSI_FLAG_USE_DEF_CHIP_OPS),
> > + .ops = &imx_mu_msi_irq_ops,
> > + .chip = &imx_mu_msi_irq_chip,
> > +};
> > +
> > +static void imx_mu_msi_parent_compose_msg(struct irq_data *data,
> > + struct msi_msg *msg)
> > +{
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > + u64 addr = msi_data->msiir_addr + 4 * data->hwirq;
> > +
> > + msg->address_hi = upper_32_bits(addr);
> > + msg->address_lo = lower_32_bits(addr);
> > + msg->data = data->hwirq;
> > +}
> > +
> > +static int imx_mu_msi_parent_set_affinity(struct irq_data *irq_data,
> > + const struct cpumask *mask, bool force)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +static struct irq_chip imx_mu_msi_parent_chip = {
> > + .name = "MU",
> > + .irq_mask = imx_mu_msi_parent_mask_irq,
> > + .irq_unmask = imx_mu_msi_parent_unmask_irq,
> > + .irq_ack = imx_mu_msi_parent_ack_irq,
> > + .irq_compose_msi_msg = imx_mu_msi_parent_compose_msg,
> > + .irq_set_affinity = imx_mu_msi_parent_set_affinity,
> > +};
> > +
> > +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> > + unsigned int virq,
> > + unsigned int nr_irqs,
> > + void *args)
> > +{
> > + struct imx_mu_msi *msi_data = domain->host_data;
> > + unsigned long flags;
> > + int pos, err = 0;
> > +
> > + WARN_ON(nr_irqs != 1);
> > +
> > + spin_lock_irqsave(&msi_data->lock, flags);
> > + pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> > + if (pos < IMX_MU_CHANS)
> > + __set_bit(pos, &msi_data->used);
> > + else
> > + err = -ENOSPC;
> > + spin_unlock_irqrestore(&msi_data->lock, flags);
> > +
> > + if (err)
> > + return err;
> > +
> > + irq_domain_set_info(domain, virq, pos,
> > + &imx_mu_msi_parent_chip, msi_data,
> > + handle_edge_irq, NULL, NULL);
> > + return 0;
> > +}
> > +
> > +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> > + unsigned int virq, unsigned int nr_irqs)
> > +{
> > + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > + struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&msi_data->lock, flags);
> > + __clear_bit(d->hwirq, &msi_data->used);
> > + spin_unlock_irqrestore(&msi_data->lock, flags);
> > +}
> > +
> > +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> > + .alloc = imx_mu_msi_domain_irq_alloc,
> > + .free = imx_mu_msi_domain_irq_free,
> > +};
> > +
> > +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> > +{
> > + struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + u32 status;
> > + int i;
> > +
> > + status = imx_mu_read(msi_data, msi_data->cfg->xSR[IMX_MU_RSR]);
> > +
> > + chained_irq_enter(chip, desc);
> > + for (i = 0; i < IMX_MU_CHANS; i++) {
> > + if (status & IMX_MU_xSR_RFn(msi_data, i))
> > + generic_handle_domain_irq(msi_data->msi_domain, i);
> > + }
> > + chained_irq_exit(chip, desc);
> > +}
> > +
> > +static int imx_mu_msi_domains_init(struct imx_mu_msi *msi_data, struct
> device *dev)
> > +{
> > + struct fwnode_handle *fwnodes = dev_fwnode(dev);
> > + struct irq_domain *parent;
> > +
> > + /* Initialize MSI domain parent */
> > + parent = irq_domain_create_linear(fwnodes,
> > + IMX_MU_CHANS,
> > + &imx_mu_msi_domain_ops,
> > + msi_data);
> > + if (!parent) {
> > + dev_err(dev, "failed to create IRQ domain\n");
> > + return -ENOMEM;
> > + }
> > +
> > + irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
> > +
> > + msi_data->msi_domain = platform_msi_create_irq_domain(
> > + fwnodes,
> > + &imx_mu_msi_domain_info,
> > + parent);
>
> nit: move the first argument after the opening bracket (longer lines
> are fine).
>
> > +
> > + if (!msi_data->msi_domain) {
> > + dev_err(dev, "failed to create MSI domain\n");
> > + irq_domain_remove(parent);
> > + return -ENOMEM;
> > + }
> > +
> > + irq_domain_set_pm_device(msi_data->msi_domain, dev);
> > +
> > + return 0;
> > +}
> > +
> > +/* Register offset of different version MU IP */
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
>
> Why doesn't this have a type?
>
> > + .xTR = 0x0,
> > + .xRR = 0x10,
> > + .xSR = {0x20, 0x20, 0x20, 0x20},
>
> Since you defined enums for all the register offsets, please be
> consistent and use them everywhere:
>
> .xSR = {
> [IMX_MU_SR] = 0x20,
> [IMX_MU_GSR] = 0x20,
> [...]
> },
>
> > + .xCR = {0x24, 0x24, 0x24, 0x24},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > + .xTR = 0x20,
> > + .xRR = 0x40,
> > + .xSR = {0x60, 0x60, 0x60, 0x60},
> > + .xCR = {0x64, 0x64, 0x64, 0x64},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp = {
> > + .type = IMX_MU_V2,
> > + .xTR = 0x200,
> > + .xRR = 0x280,
> > + .xSR = {0xC, 0x118, 0x124, 0x12C},
> > + .xCR = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8ulp_s4 = {
> > +
> > + .type = IMX_MU_V2 | IMX_MU_V2_S4,
> > + .xTR = 0x200,
> > + .xRR = 0x280,
> > + .xSR = {0xC, 0x118, 0x124, 0x12C},
> > + .xCR = {0x110, 0x114, 0x120, 0x128},
> > +};
> > +
> > +static int __init imx_mu_of_init(struct device_node *dn,
> > + struct device_node *parent,
> > + const struct imx_mu_dcfg *cfg
> > + )
>
> Move closing bracket after 'cfg'.
>
> > +{
> > + struct platform_device *pdev = of_find_device_by_node(dn);
> > + struct device_link *pd_link_a;
> > + struct device_link *pd_link_b;
> > + struct imx_mu_msi *msi_data;
> > + struct resource *res;
> > + struct device *pd_a;
> > + struct device *pd_b;
> > + struct device *dev;
> > + int ret;
> > + int irq;
> > +
> > + if (!pdev)
> > + return -ENODEV;
>
> How can that happen?
>
> > +
> > + dev = &pdev->dev;
> > +
> > + msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data),
> GFP_KERNEL);
> > + if (!msi_data)
> > + return -ENOMEM;
> > +
> > + msi_data->cfg = cfg;
> > +
> > + msi_data->regs = devm_platform_ioremap_resource_byname(pdev,
> "processor-a-side");
> > + if (IS_ERR(msi_data->regs)) {
> > + dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> > + return PTR_ERR(msi_data->regs);
> > + }
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "processor-b-side");
> > + if (!res)
> > + return -EIO;
> > +
> > + msi_data->msiir_addr = res->start + msi_data->cfg->xTR;
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq <= 0)
> > + return -ENODEV;
> > +
> > + platform_set_drvdata(pdev, msi_data);
> > +
> > + msi_data->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(msi_data->clk)) {
> > + if (PTR_ERR(msi_data->clk) != -ENOENT)
> > + return PTR_ERR(msi_data->clk);
> > +
> > + msi_data->clk = NULL;
>
> Why is it acceptable to continue with no clock?
>
> > + }
> > +
> > + pd_a = dev_pm_domain_attach_by_name(dev, "processor-a-side");
> > + if (IS_ERR(pd_a))
> > + return PTR_ERR(pd_a);
> > +
> > + pd_b = dev_pm_domain_attach_by_name(dev, "processor-b-side");
> > + if (IS_ERR(pd_b))
> > + return PTR_ERR(pd_b);
> > +
> > + pd_link_a = device_link_add(dev, pd_a,
> > + DL_FLAG_STATELESS |
> > + DL_FLAG_PM_RUNTIME |
> > + DL_FLAG_RPM_ACTIVE);
> > +
> > + if (!pd_link_a) {
> > + dev_err(dev, "Failed to add device_link to mu a.\n");
> > + goto err_pd_a;
> > + }
> > +
> > + pd_link_b = device_link_add(dev, pd_b,
> > + DL_FLAG_STATELESS |
> > + DL_FLAG_PM_RUNTIME |
> > + DL_FLAG_RPM_ACTIVE);
> > +
> > +
> > + if (!pd_link_b) {
> > + dev_err(dev, "Failed to add device_link to mu a.\n");
> > + goto err_pd_b;
> > + }
> > +
> > + ret = imx_mu_msi_domains_init(msi_data, dev);
> > + if (ret)
> > + goto err_dm_init;
> > +
> > + irq_set_chained_handler_and_data(irq,
> > + imx_mu_msi_irq_handler,
> > + msi_data);
> > +
> > + pm_runtime_enable(dev);
>
> Shouldn't you enable the device PM before registering the chained
> handler?
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v9 4/4] PCI: endpoint: Add vNTB MSI support
2022-09-07 3:48 ` [PATCH v9 4/4] PCI: endpoint: Add vNTB MSI support Frank Li
2022-09-07 20:43 ` kernel test robot
@ 2022-09-13 17:24 ` Manivannan Sadhasivam
1 sibling, 0 replies; 24+ messages in thread
From: Manivannan Sadhasivam @ 2022-09-13 17:24 UTC (permalink / raw)
To: Frank Li
Cc: maz, tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer,
kw, bhelgaas, linux-kernel, devicetree, linux-arm-kernel,
linux-pci, peng.fan, aisheng.dong, jdmason, kernel, festevam,
linux-imx, kishon, lorenzo.pieralisi, ntb, lznuaa, imx
On Tue, Sep 06, 2022 at 10:48:56PM -0500, Frank Li wrote:
> ┌───────┐ ┌──────────┐
> │ │ │ │
> ┌─────────────┐ │ PCI │ │ PCI Host │
> │ MSI │◄┐ │ EP │ │ │
> │ Controller │ │ │ │ 3.MSI Write │ │
> └────────┬────┘ └─┼───────┼───────────────────┤ │
> ▲ │ │ │ ├─BAR_n │
> │ └────────┼───────┼──────────────────►│ │
> │ │ │ 2.Call Back │ │
> │ │ │ write_msi_msg() │ │
> │ │ │ │ │
> │ └───┬───┘ └──────────┘
> │ │
> └───────────────────┘
> 1.platform_msi_domain_alloc_irqs()
>
> There is no defined way of raising IRQs by PCI host to the PCI endpoint.
> Only define MSI/MSI-X to let EP notified RC status change.
>
> The memory assigned for BAR region by the PCI host is mapped to the
> message address of platform msi interrupt controller in PCI Endpoint.
> Such that, whenever the PCI host writes to the BAR region, it will
> trigger an IRQ in the Endpoint.
>
> Basic working follow as
> 1. EP function driver call platform_msi_domain_alloc_irqs() alloc a
> MSI irq from MSI controller with call back function write_msi_msg();
> 2. write_msg_msg will config BAR and map to address defined in msi_msg;
> 3. Host side trigger an IRQ in Endpoint by write to BAR region.
>
> Add MSI support for pci-epf-vntb. Query if system has an MSI controller.
> Set up doorbell address according to struct msi_msg.
>
> So PCI RC can write this doorbell address to trigger EP side's IRQ.
>
> If no MSI controller exists, fall back to software polling.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> drivers/pci/endpoint/functions/pci-epf-vntb.c | 155 +++++++++++++++---
> 1 file changed, 128 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 1466dd1904175..426205b980a09 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -44,6 +44,7 @@
> #include <linux/pci-epc.h>
> #include <linux/pci-epf.h>
> #include <linux/ntb.h>
> +#include <linux/msi.h>
>
> static struct workqueue_struct *kpcintb_workqueue;
>
> @@ -136,13 +137,15 @@ struct epf_ntb {
>
> struct epf_ntb_ctrl *reg;
>
> - phys_addr_t epf_db_phy;
> + phys_addr_t epf_db_phys;
This should be part of a separate patch.
> void __iomem *epf_db;
>
> phys_addr_t vpci_mw_phy[MAX_MW];
> void __iomem *vpci_mw_addr[MAX_MW];
>
> struct delayed_work cmd_handler;
> +
> + int msi_virqbase;
> };
>
> #define to_epf_ntb(epf_group) container_of((epf_group), struct epf_ntb, group)
> @@ -253,13 +256,15 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
>
> ntb = container_of(work, struct epf_ntb, cmd_handler.work);
>
> - for (i = 1; i < ntb->db_count; i++) {
> - if (readl(ntb->epf_db + i * 4)) {
> - if (readl(ntb->epf_db + i * 4))
> - ntb->db |= 1 << (i - 1);
> + if (!ntb->epf_db_phys) {
> + for (i = 1; i < ntb->db_count; i++) {
> + if (readl(ntb->epf_db + i * 4)) {
> + if (readl(ntb->epf_db + i * 4))
Why are you reading twice? And why cannot you use _relaxed() variant
here and below?
> + ntb->db |= 1 << (i - 1);
>
> - ntb_db_event(&ntb->ntb, i);
> - writel(0, ntb->epf_db + i * 4);
> + ntb_db_event(&ntb->ntb, i);
> + writel(0, ntb->epf_db + i * 4);
> + }
> }
> }
>
> @@ -454,11 +459,9 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
> ctrl->num_mws = ntb->num_mws;
> ntb->spad_size = spad_size;
>
> - ctrl->db_entry_size = 4;
> -
> for (i = 0; i < ntb->db_count; i++) {
> ntb->reg->db_data[i] = 1 + i;
> - ntb->reg->db_offset[i] = 0;
> + ntb->reg->db_offset[i] = 4 * i;
4 should be defined as a macro of what it represents.
> }
>
> return 0;
> @@ -509,6 +512,28 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
> return 0;
> }
>
> +static int epf_ntb_db_size(struct epf_ntb *ntb)
> +{
> + const struct pci_epc_features *epc_features;
> + size_t size = 4 * ntb->db_count;
Here also.
> + u32 align;
> +
> + epc_features = pci_epc_get_features(ntb->epf->epc,
> + ntb->epf->func_no,
> + ntb->epf->vfunc_no);
> + align = epc_features->align;
> +
> + if (size < 128)
> + size = 128;
> +
> + if (align)
> + size = ALIGN(size, align);
> + else
> + size = roundup_pow_of_two(size);
> +
> + return size;
> +}
> +
> /**
> * epf_ntb_db_bar_init() - Configure Doorbell window BARs
> * @ntb: NTB device that facilitates communication between HOST and vHOST
> @@ -522,33 +547,32 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
> struct pci_epf_bar *epf_bar;
> void __iomem *mw_addr;
> enum pci_barno barno;
> - size_t size = 4 * ntb->db_count;
> + size_t size;
>
> epc_features = pci_epc_get_features(ntb->epf->epc,
> ntb->epf->func_no,
> ntb->epf->vfunc_no);
> align = epc_features->align;
> -
> - if (size < 128)
> - size = 128;
> -
> - if (align)
> - size = ALIGN(size, align);
> - else
> - size = roundup_pow_of_two(size);
> + size = epf_ntb_db_size(ntb);
>
> barno = ntb->epf_ntb_bar[BAR_DB];
> + epf_bar = &ntb->epf->bar[barno];
>
> - mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
> - if (!mw_addr) {
> - dev_err(dev, "Failed to allocate OB address\n");
> - return -ENOMEM;
> + if (ntb->epf_db_phys) {
> + mw_addr = NULL;
> + epf_bar->phys_addr = ntb->epf_db_phys;
> + epf_bar->barno = barno;
> + epf_bar->size = size;
> + } else {
> + mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
> + if (!mw_addr) {
> + dev_err(dev, "Failed to allocate door bell address\n");
doorbell
> + return -ENOMEM;
> + }
> }
>
> ntb->epf_db = mw_addr;
>
> - epf_bar = &ntb->epf->bar[barno];
> -
> ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no, ntb->epf->vfunc_no, epf_bar);
> if (ret) {
> dev_err(dev, "Doorbell BAR set failed\n");
> @@ -704,6 +728,82 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
> return 0;
> }
>
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> +static void epf_ntb_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> + struct epf_ntb *ntb = dev_get_drvdata(desc->dev);
> + struct epf_ntb_ctrl *reg = ntb->reg;
> + int size = epf_ntb_db_size(ntb);
> + u64 addr;
> +
> + addr = msg->address_hi;
> + addr <<= 32;
> + addr |= msg->address_lo;
> +
> + reg->db_data[desc->msi_index] = msg->data;
> +
> + if (desc->msi_index == 0)
if (!desc->msi_index)
> + ntb->epf_db_phys = round_down(addr, size);
> +
> + reg->db_offset[desc->msi_index] = addr - ntb->epf_db_phys;
> +}
> +#endif
> +
> +static irqreturn_t epf_ntb_interrupt_handler(int irq, void *data)
> +{
> + struct epf_ntb *ntb = data;
> + int index;
> +
> + index = irq - ntb->msi_virqbase;
> + ntb->db |= 1 << (index - 1);
> + ntb_db_event(&ntb->ntb, index);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void epf_ntb_epc_msi_init(struct epf_ntb *ntb)
Why cannot you guard this whole function with CONFIG_GENERIC_MSI_IRQ_DOMAIN?
> +{
> + struct device *dev = &ntb->epf->dev;
> + struct irq_domain *domain;
> + int virq;
> + int ret;
> + int i;
> +
> + domain = dev_get_msi_domain(ntb->epf->epc->dev.parent);
> + if (!domain)
> + return;
> +
> + dev_set_msi_domain(dev, domain);
> +
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> + if (platform_msi_domain_alloc_irqs(&ntb->epf->dev,
> + ntb->db_count,
> + epf_ntb_write_msi_msg)) {
> + dev_info(dev, "Can't allocate MSI, fall back to poll mode\n");
falling back to polling mode
Should this be dev_err?
> + return;
> + }
> +#else
> + return;
> +#endif
> + dev_info(dev, "vntb use MSI as doorbell\n");
Using MSI as a doorbell
> +
> + for (i = 0; i < ntb->db_count; i++) {
> + virq = msi_get_virq(dev, i);
> + ret = devm_request_irq(dev, virq,
> + epf_ntb_interrupt_handler, 0,
> + "vntb", ntb);
s/vntb/pci_epf_vntb
> +
> + if (ret) {
> + dev_err(dev, "devm_request_irq() failure, fall back to poll mode\n");
Failed to request doorbell IRQ! Falling back to polling mode
> + ntb->epf_db_phys = 0;
> + break;
> + }
> +
> + if (!i)
> + ntb->msi_virqbase = virq;
A comment here would be helpful
Thanks,
Mani
> + }
> +}
> +
> /**
> * epf_ntb_epc_init() - Initialize NTB interface
> * @ntb: NTB device that facilitates communication between HOST and vHOST2
> @@ -1299,14 +1399,15 @@ static int epf_ntb_bind(struct pci_epf *epf)
> goto err_bar_alloc;
> }
>
> + epf_set_drvdata(epf, ntb);
> + epf_ntb_epc_msi_init(ntb);
> +
> ret = epf_ntb_epc_init(ntb);
> if (ret) {
> dev_err(dev, "Failed to initialize EPC\n");
> goto err_bar_alloc;
> }
>
> - epf_set_drvdata(epf, ntb);
> -
> pci_space[0] = (ntb->vntb_pid << 16) | ntb->vntb_vid;
> pci_vntb_table[0].vendor = ntb->vntb_vid;
> pci_vntb_table[0].device = ntb->vntb_pid;
> --
> 2.35.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [EXT] Re: [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver
2022-09-12 15:53 ` Frank Li
@ 2022-09-13 17:44 ` Marc Zyngier
0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-09-13 17:44 UTC (permalink / raw)
To: Frank Li
Cc: tglx, robh+dt, krzysztof.kozlowski+dt, shawnguo, s.hauer, kw,
bhelgaas, linux-kernel, devicetree, linux-arm-kernel, linux-pci,
Peng Fan, Aisheng Dong, jdmason, kernel, festevam, dl-linux-imx,
kishon, lorenzo.pieralisi, ntb, lznuaa, imx,
manivannan.sadhasivam
On Mon, 12 Sep 2022 16:53:40 +0100,
Frank Li <frank.li@nxp.com> wrote:
>
> > > [Frank Li] I remember you said that irq-chip can't be removed.
> > > So I am not sure why need build as module.
> >
> > Not being removed doesn't mean it cannot be built as a module and
> > loaded on demand. Why should I be forced to have this driver built-in
> > if my kernel is used on a variety of systems, only one of them having
> > this device?
>
> [Frank Li] A problem, platform_msi_create_irq_domain have NOT export
> to let module Call it.
> https://elixir.bootlin.com/linux/latest/source/drivers/base/platform-msi.c#L122
>
> Do you want to me add EXPORT_SYMBOL_GPL for it OR keep "bool" here?
Please add a patch exporting the missing symbols, and make the think
modular.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-09-13 17:45 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-07 3:48 [PATCH v9 0/4] PCI EP driver support MSI doorbell from host Frank Li
2022-09-07 3:48 ` [PATCH v9 1/4] irqchip: allow pass down .pm field at IRQCHIP_PLATFORM_DRIVER_END Frank Li
2022-09-07 3:48 ` [PATCH v9 2/4] irqchip: Add IMX MU MSI controller driver Frank Li
2022-09-08 0:03 ` kernel test robot
2022-09-08 7:02 ` Marc Zyngier
2022-09-08 14:26 ` [EXT] " Frank Li
2022-09-08 14:35 ` Marc Zyngier
2022-09-08 7:39 ` Marc Zyngier
2022-09-08 14:23 ` [EXT] " Frank Li
2022-09-08 14:51 ` Marc Zyngier
2022-09-08 15:35 ` Frank Li
2022-09-09 12:07 ` Marc Zyngier
2022-09-09 14:59 ` Frank Li
2022-09-10 14:35 ` Marc Zyngier
2022-09-12 15:53 ` Frank Li
2022-09-13 17:44 ` Marc Zyngier
2022-09-09 14:52 ` Frank Li
2022-09-10 14:40 ` Marc Zyngier
2022-09-12 16:17 ` Frank Li
2022-09-07 3:48 ` [PATCH v9 3/4] dt-bindings: irqchip: imx mu work as msi controller Frank Li
2022-09-09 1:43 ` Rob Herring
2022-09-07 3:48 ` [PATCH v9 4/4] PCI: endpoint: Add vNTB MSI support Frank Li
2022-09-07 20:43 ` kernel test robot
2022-09-13 17:24 ` Manivannan Sadhasivam
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).