* [PATCH] PCI: exynos: add support for MSI
@ 2013-08-12 8:56 Jingoo Han
2013-08-12 9:12 ` Sachin Kamat
2013-08-12 10:56 ` Thierry Reding
0 siblings, 2 replies; 6+ messages in thread
From: Jingoo Han @ 2013-08-12 8:56 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-samsung-soc, Kukjin Kim, Pratyush Anand,
Mohit KUMAR, Siva Reddy Kallam,
'SRIKANTH TUMKUR SHIVANAND',
Arnd Bergmann, 'Sean Cross',
'Kishon Vijay Abraham I', 'Thierry Reding',
'Thomas Petazzoni',
linux-kernel, devicetree, Jingoo Han
This patch adds support for Message Signaled Interrupt in the
Exynops PCIe diver using Synopsys designware PCIe core IP.
Signed-off-by: Siva Reddy Kallam <siva.kallam@samsung.com>
Signed-off-by: Srikanth T Shivanand <ts.srikanth@samsung.com>
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
Cc: Pratyush Anand <pratyush.anand@st.com>
Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
---
arch/arm/boot/dts/exynos5440.dtsi | 2 +
arch/arm/mach-exynos/Kconfig | 1 +
drivers/pci/host/pci-exynos.c | 60 ++++++++++
drivers/pci/host/pcie-designware.c | 213 ++++++++++++++++++++++++++++++++++++
drivers/pci/host/pcie-designware.h | 8 ++
5 files changed, 284 insertions(+)
diff --git a/arch/arm/boot/dts/exynos5440.dtsi b/arch/arm/boot/dts/exynos5440.dtsi
index 586134e..3746835 100644
--- a/arch/arm/boot/dts/exynos5440.dtsi
+++ b/arch/arm/boot/dts/exynos5440.dtsi
@@ -249,6 +249,7 @@
interrupt-map-mask = <0 0 0 0>;
interrupt-map = <0x0 0 &gic 53>;
num-lanes = <4>;
+ msi-base = <200>;
};
pcie@2a0000 {
@@ -269,5 +270,6 @@
interrupt-map-mask = <0 0 0 0>;
interrupt-map = <0x0 0 &gic 56>;
num-lanes = <4>;
+ msi-base = <232>;
};
};
diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
index 855d4a7..9ef1c95 100644
--- a/arch/arm/mach-exynos/Kconfig
+++ b/arch/arm/mach-exynos/Kconfig
@@ -93,6 +93,7 @@ config SOC_EXYNOS5440
default y
depends on ARCH_EXYNOS5
select ARCH_HAS_OPP
+ select ARCH_SUPPORTS_MSI
select HAVE_ARM_ARCH_TIMER
select AUTO_ZRELADDR
select MIGHT_HAVE_PCI
diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
index 012ca8a..d0477d0 100644
--- a/drivers/pci/host/pci-exynos.c
+++ b/drivers/pci/host/pci-exynos.c
@@ -48,6 +48,7 @@ struct exynos_pcie {
#define PCIE_IRQ_SPECIAL 0x008
#define PCIE_IRQ_EN_PULSE 0x00c
#define PCIE_IRQ_EN_LEVEL 0x010
+#define IRQ_MSI_ENABLE (0x1 << 2)
#define PCIE_IRQ_EN_SPECIAL 0x014
#define PCIE_PWR_RESET 0x018
#define PCIE_CORE_RESET 0x01c
@@ -320,9 +321,51 @@ static irqreturn_t exynos_pcie_irq_handler(int irq, void *arg)
return IRQ_HANDLED;
}
+#ifdef CONFIG_PCI_MSI
+static void exynos_pcie_clear_irq_level(struct pcie_port *pp)
+{
+ u32 val;
+ struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
+ void __iomem *elbi_base = exynos_pcie->elbi_base;
+
+ val = readl(elbi_base + PCIE_IRQ_LEVEL);
+ writel(val, elbi_base + PCIE_IRQ_LEVEL);
+ return;
+}
+
+static irqreturn_t exynos_pcie_msi_irq_handler(int irq, void *arg)
+{
+ struct pcie_port *pp = arg;
+
+ /* handle msi irq */
+ dw_handle_msi_irq(pp);
+ exynos_pcie_clear_irq_level(pp);
+
+ return IRQ_HANDLED;
+}
+
+static void exynos_pcie_msi_init(struct pcie_port *pp)
+{
+ u32 val;
+ struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
+ void __iomem *elbi_base = exynos_pcie->elbi_base;
+
+ dw_pcie_msi_init(pp);
+
+ /* enable MSI interrupt */
+ val = readl(elbi_base + PCIE_IRQ_EN_LEVEL);
+ val |= IRQ_MSI_ENABLE;
+ writel(val, elbi_base + PCIE_IRQ_EN_LEVEL);
+ return;
+}
+#endif
+
static void exynos_pcie_enable_interrupts(struct pcie_port *pp)
{
exynos_pcie_enable_irq_pulse(pp);
+#ifdef CONFIG_PCI_MSI
+ exynos_pcie_msi_init(pp);
+#endif
return;
}
@@ -408,6 +451,23 @@ static int add_pcie_port(struct pcie_port *pp, struct platform_device *pdev)
return ret;
}
+#ifdef CONFIG_PCI_MSI
+ pp->msi_irq = platform_get_irq(pdev, 0);
+
+ if (!pp->msi_irq) {
+ dev_err(&pdev->dev, "failed to get msi irq\n");
+ return -ENODEV;
+ }
+
+ ret = devm_request_irq(&pdev->dev, pp->msi_irq,
+ exynos_pcie_msi_irq_handler,
+ IRQF_SHARED, "exynos-pcie", pp);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to request msi irq\n");
+ return ret;
+ }
+#endif
+
pp->root_bus_nr = -1;
pp->ops = &exynos_pcie_host_ops;
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 77b0c25..5a47f11 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -11,8 +11,10 @@
* published by the Free Software Foundation.
*/
+#include <linux/irq.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/msi.h>
#include <linux/of_address.h>
#include <linux/pci.h>
#include <linux/pci_regs.h>
@@ -62,6 +64,14 @@
#define PCIE_ATU_FUNC(x) (((x) & 0x7) << 16)
#define PCIE_ATU_UPPER_TARGET 0x91C
+#ifdef CONFIG_PCI_MSI
+#define MAX_MSI_IRQS 32
+#define MAX_MSI_CTRLS 8
+
+static unsigned int msi_data;
+static DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
+#endif
+
static struct hw_pci dw_pci;
unsigned long global_io_offset;
@@ -144,6 +154,202 @@ int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
return ret;
}
+#ifdef CONFIG_PCI_MSI
+static struct irq_chip dw_msi_chip = {
+ .name = "PCI-MSI",
+ .irq_enable = unmask_msi_irq,
+ .irq_disable = mask_msi_irq,
+ .irq_mask = mask_msi_irq,
+ .irq_unmask = unmask_msi_irq,
+};
+
+/* MSI int handler */
+void dw_handle_msi_irq(struct pcie_port *pp)
+{
+ unsigned long val;
+ int i, pos;
+
+ for (i = 0; i < MAX_MSI_CTRLS; i++) {
+ dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
+ (u32 *)&val);
+ if (val) {
+ pos = 0;
+ while ((pos = find_next_bit(&val, 32, pos)) != 32) {
+ generic_handle_irq(pp->msi_irq_start
+ + (i * 32) + pos);
+ pos++;
+ }
+ }
+ dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4, val);
+ }
+}
+
+void dw_pcie_msi_init(struct pcie_port *pp)
+{
+ /* program the msi_data */
+ dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
+ __virt_to_phys((u32)(&msi_data)));
+ dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4, 0);
+}
+
+static int find_valid_pos0(int msgvec, int pos, int *pos0)
+{
+ int flag = 1;
+
+ do {
+ pos = find_next_zero_bit(msi_irq_in_use,
+ MAX_MSI_IRQS, pos);
+ /*if you have reached to the end then get out from here.*/
+ if (pos == MAX_MSI_IRQS)
+ return -ENOSPC;
+ /*
+ * Check if this position is at correct offset.nvec is always a
+ * power of two. pos0 must be nvec bit alligned.
+ */
+ if (pos % msgvec)
+ pos += msgvec - (pos % msgvec);
+ else
+ flag = 0;
+ } while (flag);
+
+ *pos0 = pos;
+ return 0;
+}
+
+/* Dynamic irq allocate and deallocation */
+static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
+{
+ int res, bit, irq, pos0, pos1, i;
+ u32 val;
+ struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);
+
+ if (!pp) {
+ BUG();
+ return -EINVAL;
+ }
+
+ pos0 = find_first_zero_bit(msi_irq_in_use,
+ MAX_MSI_IRQS);
+ if (pos0 % no_irqs) {
+ if (find_valid_pos0(no_irqs, pos0, &pos0))
+ goto no_valid_irq;
+ }
+ if (no_irqs > 1) {
+ pos1 = find_next_bit(msi_irq_in_use,
+ MAX_MSI_IRQS, pos0);
+ /* there must be nvec number of consecutive free bits */
+ while ((pos1 - pos0) < no_irqs) {
+ if (find_valid_pos0(no_irqs, pos1, &pos0))
+ goto no_valid_irq;
+ pos1 = find_next_bit(msi_irq_in_use,
+ MAX_MSI_IRQS, pos0);
+ }
+ }
+
+ irq = (pp->msi_irq_start + pos0);
+
+ if ((irq + no_irqs) > (pp->msi_irq_start + MAX_MSI_IRQS-1))
+ goto no_valid_irq;
+
+ i = 0;
+ while (i < no_irqs) {
+ set_bit(pos0 + i, msi_irq_in_use);
+ irq_alloc_descs((irq + i), (irq + i), 1, 0);
+ irq_set_msi_desc(irq + i, desc);
+ irq_set_chip_and_handler(irq + i, &dw_msi_chip,
+ handle_simple_irq);
+ set_irq_flags(irq + i, IRQF_VALID);
+ /*Enable corresponding interrupt in MSI interrupt controller */
+ res = ((pos0 + i) / 32) * 12;
+ bit = (pos0 + i) % 32;
+ dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val);
+ val |= 1 << bit;
+ dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val);
+ i++;
+ }
+
+ *pos = pos0;
+ return irq;
+
+no_valid_irq:
+ *pos = pos0;
+ return -ENOSPC;
+}
+
+static void clear_irq(unsigned int irq)
+{
+ int res, bit, val, pos;
+ struct irq_desc *desc;
+ struct msi_desc *msi;
+ struct pcie_port *pp;
+
+ /* get the port structure */
+ desc = irq_to_desc(irq);
+ msi = irq_desc_get_msi_desc(desc);
+ pp = sys_to_pcie(msi->dev->bus->sysdata);
+ if (!pp) {
+ BUG();
+ return;
+ }
+
+ pos = irq - pp->msi_irq_start;
+
+ irq_free_desc(irq);
+
+ clear_bit(pos, msi_irq_in_use);
+
+ /* Disable corresponding interrupt on MSI interrupt controller */
+ res = (pos / 32) * 12;
+ bit = pos % 32;
+ dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val);
+ val &= ~(1 << bit);
+ dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val);
+}
+
+int arch_setup_msi_irq(struct pci_dev *pdev, struct msi_desc *desc)
+{
+ int irq, pos, msgvec;
+ u16 msg_ctr;
+ struct msi_msg msg;
+ struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
+
+ if (!pp) {
+ BUG();
+ return -EINVAL;
+ }
+
+ pci_read_config_word(pdev, desc->msi_attrib.pos+PCI_MSI_FLAGS,
+ &msg_ctr);
+ msgvec = (msg_ctr&PCI_MSI_FLAGS_QSIZE) >> 4;
+ if (msgvec == 0)
+ msgvec = (msg_ctr & PCI_MSI_FLAGS_QMASK) >> 1;
+ if (msgvec > 5)
+ msgvec = 0;
+
+ irq = assign_irq((1 << msgvec), desc, &pos);
+ if (irq < 0)
+ return irq;
+
+ msg_ctr &= ~PCI_MSI_FLAGS_QSIZE;
+ msg_ctr |= msgvec << 4;
+ pci_write_config_word(pdev, desc->msi_attrib.pos + PCI_MSI_FLAGS,
+ msg_ctr);
+ desc->msi_attrib.multiple = msgvec;
+
+ msg.address_hi = 0x0;
+ msg.address_lo = __virt_to_phys((u32)(&msi_data));
+ msg.data = pos;
+ write_msi_msg(irq, &msg);
+
+ return 0;
+}
+
+void arch_teardown_msi_irq(unsigned int irq)
+{
+ clear_irq(irq);
+}
+#endif
+
int dw_pcie_link_up(struct pcie_port *pp)
{
if (pp->ops->link_up)
@@ -225,6 +431,13 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
return -EINVAL;
}
+#ifdef CONFIG_PCI_MSI
+ if (of_property_read_u32(np, "msi-base", &pp->msi_irq_start)) {
+ dev_err(pp->dev, "Failed to parse the number of lanes\n");
+ return -EINVAL;
+ }
+#endif
+
if (pp->ops->host_init)
pp->ops->host_init(pp);
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index 133820f..fcff10e 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -38,6 +38,10 @@ struct pcie_port {
int irq;
u32 lanes;
struct pcie_host_ops *ops;
+#ifdef CONFIG_PCI_MSI
+ int msi_irq;
+ int msi_irq_start;
+#endif
};
struct pcie_host_ops {
@@ -57,6 +61,10 @@ int cfg_read(void __iomem *addr, int where, int size, u32 *val);
int cfg_write(void __iomem *addr, int where, int size, u32 val);
int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, u32 val);
int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, u32 *val);
+#ifdef CONFIG_PCI_MSI
+void dw_handle_msi_irq(struct pcie_port *pp);
+void dw_pcie_msi_init(struct pcie_port *pp);
+#endif
int dw_pcie_link_up(struct pcie_port *pp);
void dw_pcie_setup_rc(struct pcie_port *pp);
int dw_pcie_host_init(struct pcie_port *pp);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: exynos: add support for MSI
2013-08-12 8:56 [PATCH] PCI: exynos: add support for MSI Jingoo Han
@ 2013-08-12 9:12 ` Sachin Kamat
2013-08-22 5:25 ` Jingoo Han
2013-08-12 10:56 ` Thierry Reding
1 sibling, 1 reply; 6+ messages in thread
From: Sachin Kamat @ 2013-08-12 9:12 UTC (permalink / raw)
To: Jingoo Han
Cc: Bjorn Helgaas, linux-pci, linux-samsung-soc, Kukjin Kim,
Pratyush Anand, Mohit KUMAR, Siva Reddy Kallam,
SRIKANTH TUMKUR SHIVANAND, Arnd Bergmann, Sean Cross,
Kishon Vijay Abraham I, Thierry Reding, Thomas Petazzoni,
linux-kernel, devicetree
Hi Jingoo,
On 12 August 2013 14:26, Jingoo Han <jg1.han@samsung.com> wrote:
> This patch adds support for Message Signaled Interrupt in the
> Exynops PCIe diver using Synopsys designware PCIe core IP.
s/Exynops PCIe diver/Exynos PCIe driver
>
> Signed-off-by: Siva Reddy Kallam <siva.kallam@samsung.com>
> Signed-off-by: Srikanth T Shivanand <ts.srikanth@samsung.com>
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> Cc: Pratyush Anand <pratyush.anand@st.com>
> Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> ---
> arch/arm/boot/dts/exynos5440.dtsi | 2 +
> arch/arm/mach-exynos/Kconfig | 1 +
> drivers/pci/host/pci-exynos.c | 60 ++++++++++
> drivers/pci/host/pcie-designware.c | 213 ++++++++++++++++++++++++++++++++++++
> drivers/pci/host/pcie-designware.h | 8 ++
> 5 files changed, 284 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos5440.dtsi b/arch/arm/boot/dts/exynos5440.dtsi
> index 586134e..3746835 100644
> --- a/arch/arm/boot/dts/exynos5440.dtsi
> +++ b/arch/arm/boot/dts/exynos5440.dtsi
> @@ -249,6 +249,7 @@
> interrupt-map-mask = <0 0 0 0>;
> interrupt-map = <0x0 0 &gic 53>;
> num-lanes = <4>;
> + msi-base = <200>;
Please update the bindings documentation too.
> };
>
> pcie@2a0000 {
> @@ -269,5 +270,6 @@
[snip]
>
> +#ifdef CONFIG_PCI_MSI
> +static void exynos_pcie_clear_irq_level(struct pcie_port *pp)
> +{
> + u32 val;
> + struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
> + void __iomem *elbi_base = exynos_pcie->elbi_base;
> +
> + val = readl(elbi_base + PCIE_IRQ_LEVEL);
> + writel(val, elbi_base + PCIE_IRQ_LEVEL);
Sorry, I did not get this. Writing the value read from the same
register without any operation.
--
With warm regards,
Sachin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: exynos: add support for MSI
2013-08-12 8:56 [PATCH] PCI: exynos: add support for MSI Jingoo Han
2013-08-12 9:12 ` Sachin Kamat
@ 2013-08-12 10:56 ` Thierry Reding
2013-08-12 11:47 ` Pratyush Anand
2013-08-23 4:58 ` Jingoo Han
1 sibling, 2 replies; 6+ messages in thread
From: Thierry Reding @ 2013-08-12 10:56 UTC (permalink / raw)
To: Jingoo Han
Cc: Bjorn Helgaas, linux-pci, linux-samsung-soc, Kukjin Kim,
Pratyush Anand, Mohit KUMAR, Siva Reddy Kallam,
'SRIKANTH TUMKUR SHIVANAND',
Arnd Bergmann, 'Sean Cross',
'Kishon Vijay Abraham I', 'Thomas Petazzoni',
linux-kernel, devicetree
[-- Attachment #1: Type: text/plain, Size: 10189 bytes --]
On Mon, Aug 12, 2013 at 05:56:47PM +0900, Jingoo Han wrote:
[...]
> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> index 855d4a7..9ef1c95 100644
> --- a/arch/arm/mach-exynos/Kconfig
> +++ b/arch/arm/mach-exynos/Kconfig
> @@ -93,6 +93,7 @@ config SOC_EXYNOS5440
> default y
> depends on ARCH_EXYNOS5
> select ARCH_HAS_OPP
> + select ARCH_SUPPORTS_MSI
This symbol goes away in Thomas Petazzoni's MSI patch series which is
targetted at 3.12, so I don't think you should add that here.
> +#ifdef CONFIG_PCI_MSI
> +static void exynos_pcie_clear_irq_level(struct pcie_port *pp)
> +{
> + u32 val;
> + struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
> + void __iomem *elbi_base = exynos_pcie->elbi_base;
> +
> + val = readl(elbi_base + PCIE_IRQ_LEVEL);
> + writel(val, elbi_base + PCIE_IRQ_LEVEL);
> + return;
> +}
I'm a little confused by this: the above code seems to access the PCIe
controller registers to clear an interrupt, but you pass in a PCIe
port...
> +static irqreturn_t exynos_pcie_msi_irq_handler(int irq, void *arg)
> +{
> + struct pcie_port *pp = arg;
> +
> + /* handle msi irq */
> + dw_handle_msi_irq(pp);
> + exynos_pcie_clear_irq_level(pp);
... so here dw_handle_msi_irq() seems to operate on a single port, while
clearing the IRQ is done on a per-controller basis.
I see that the Exynos PCIe driver hasn't made it into linux-next yet, so
I don't have full context surrounding this, but it strikes me as odd
that MSI's would be handled per-port instead of per-controller. And
furthermore that the DesignWare part handles it per-port yet the Exynos
specific part handles it per-controller.
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void exynos_pcie_msi_init(struct pcie_port *pp)
> +{
> + u32 val;
> + struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
> + void __iomem *elbi_base = exynos_pcie->elbi_base;
> +
> + dw_pcie_msi_init(pp);
> +
> + /* enable MSI interrupt */
> + val = readl(elbi_base + PCIE_IRQ_EN_LEVEL);
> + val |= IRQ_MSI_ENABLE;
> + writel(val, elbi_base + PCIE_IRQ_EN_LEVEL);
> + return;
> +}
This function is called per-port, yet operates on per-controller
registers. It's not terribly bad in this case because it only sets one
bit, but it could eventually lead to problems in case you need to extend
this function in the future to do more, which could then potentially be
run multiple times and cause problems.
> +#endif
> +
> static void exynos_pcie_enable_interrupts(struct pcie_port *pp)
> {
> exynos_pcie_enable_irq_pulse(pp);
> +#ifdef CONFIG_PCI_MSI
> + exynos_pcie_msi_init(pp);
> +#endif
> return;
> }
Instead of the whole #ifdef business above, can't you just use something
like this in exynos_pcie_enable_interrupts():
if (IS_ENABLED(CONFIG_PCI_MSI))
exynos_pcie_msi_init(pp);
Now you can drop the #ifdef guards and the compiler will throw away all
the related code automatically if PCI_MSI is not selected because the
functions are all static and unused. This has the advantage of compiling
all the code whether or not PCI_MSI is selected or not, therefore
increasing compile coverage of the driver.
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
[...]
> @@ -62,6 +64,14 @@
> #define PCIE_ATU_FUNC(x) (((x) & 0x7) << 16)
> #define PCIE_ATU_UPPER_TARGET 0x91C
>
> +#ifdef CONFIG_PCI_MSI
> +#define MAX_MSI_IRQS 32
> +#define MAX_MSI_CTRLS 8
> +
> +static unsigned int msi_data;
> +static DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
> +#endif
> +
> static struct hw_pci dw_pci;
>
> unsigned long global_io_offset;
> @@ -144,6 +154,202 @@ int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
> return ret;
> }
>
> +#ifdef CONFIG_PCI_MSI
> +static struct irq_chip dw_msi_chip = {
> + .name = "PCI-MSI",
> + .irq_enable = unmask_msi_irq,
> + .irq_disable = mask_msi_irq,
> + .irq_mask = mask_msi_irq,
> + .irq_unmask = unmask_msi_irq,
> +};
> +
> +/* MSI int handler */
> +void dw_handle_msi_irq(struct pcie_port *pp)
> +{
> + unsigned long val;
> + int i, pos;
> +
> + for (i = 0; i < MAX_MSI_CTRLS; i++) {
> + dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
> + (u32 *)&val);
> + if (val) {
> + pos = 0;
> + while ((pos = find_next_bit(&val, 32, pos)) != 32) {
> + generic_handle_irq(pp->msi_irq_start
> + + (i * 32) + pos);
> + pos++;
> + }
> + }
> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4, val);
> + }
> +}
> +
> +void dw_pcie_msi_init(struct pcie_port *pp)
> +{
> + /* program the msi_data */
> + dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
> + __virt_to_phys((u32)(&msi_data)));
That's slightly odd. You convert the virtual address of a local variable
(local to the file) to a physical address and program that into a
register. I assume that it works since you've probably tested this, but
I wonder if it's safe to do this. Perhaps a better way would be to
allocate a single free page (__get_free_pages(GFP_KERNEL, 0)) and write
the physical address of that into the register instead.
> +static int find_valid_pos0(int msgvec, int pos, int *pos0)
> +{
> + int flag = 1;
> +
> + do {
> + pos = find_next_zero_bit(msi_irq_in_use,
> + MAX_MSI_IRQS, pos);
> + /*if you have reached to the end then get out from here.*/
> + if (pos == MAX_MSI_IRQS)
> + return -ENOSPC;
> + /*
> + * Check if this position is at correct offset.nvec is always a
> + * power of two. pos0 must be nvec bit alligned.
> + */
> + if (pos % msgvec)
> + pos += msgvec - (pos % msgvec);
> + else
> + flag = 0;
> + } while (flag);
> +
> + *pos0 = pos;
> + return 0;
> +}
> +
> +/* Dynamic irq allocate and deallocation */
> +static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
> +{
> + int res, bit, irq, pos0, pos1, i;
> + u32 val;
> + struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);
> +
> + if (!pp) {
> + BUG();
> + return -EINVAL;
> + }
> +
> + pos0 = find_first_zero_bit(msi_irq_in_use,
> + MAX_MSI_IRQS);
> + if (pos0 % no_irqs) {
> + if (find_valid_pos0(no_irqs, pos0, &pos0))
> + goto no_valid_irq;
> + }
> + if (no_irqs > 1) {
> + pos1 = find_next_bit(msi_irq_in_use,
> + MAX_MSI_IRQS, pos0);
> + /* there must be nvec number of consecutive free bits */
> + while ((pos1 - pos0) < no_irqs) {
> + if (find_valid_pos0(no_irqs, pos1, &pos0))
> + goto no_valid_irq;
> + pos1 = find_next_bit(msi_irq_in_use,
> + MAX_MSI_IRQS, pos0);
> + }
> + }
> +
> + irq = (pp->msi_irq_start + pos0);
> +
> + if ((irq + no_irqs) > (pp->msi_irq_start + MAX_MSI_IRQS-1))
> + goto no_valid_irq;
> +
> + i = 0;
> + while (i < no_irqs) {
> + set_bit(pos0 + i, msi_irq_in_use);
> + irq_alloc_descs((irq + i), (irq + i), 1, 0);
> + irq_set_msi_desc(irq + i, desc);
> + irq_set_chip_and_handler(irq + i, &dw_msi_chip,
> + handle_simple_irq);
> + set_irq_flags(irq + i, IRQF_VALID);
> + /*Enable corresponding interrupt in MSI interrupt controller */
> + res = ((pos0 + i) / 32) * 12;
> + bit = (pos0 + i) % 32;
> + dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val);
> + val |= 1 << bit;
> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val);
> + i++;
> + }
> +
> + *pos = pos0;
> + return irq;
> +
> +no_valid_irq:
> + *pos = pos0;
> + return -ENOSPC;
> +}
There was some discussion about this already, and I think eventually we
really should refactor some of this code. Currently all three ARM PCIe
drivers (Marvell, Tegra and Exynos/DesignWare) use a similar bitmap-
based allocator for this. Benjamin Herrenschmidt pointed out that the
same exists for PowerPC as well, so we should look at converging on one
implementation eventually. But I think that can be done subsequently and
shouldn't have to be done prior to this patch.
> +static void clear_irq(unsigned int irq)
> +{
> + int res, bit, val, pos;
> + struct irq_desc *desc;
> + struct msi_desc *msi;
> + struct pcie_port *pp;
> +
> + /* get the port structure */
> + desc = irq_to_desc(irq);
> + msi = irq_desc_get_msi_desc(desc);
> + pp = sys_to_pcie(msi->dev->bus->sysdata);
> + if (!pp) {
> + BUG();
> + return;
> + }
> +
> + pos = irq - pp->msi_irq_start;
> +
> + irq_free_desc(irq);
> +
> + clear_bit(pos, msi_irq_in_use);
> +
> + /* Disable corresponding interrupt on MSI interrupt controller */
> + res = (pos / 32) * 12;
> + bit = pos % 32;
> + dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val);
> + val &= ~(1 << bit);
> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val);
> +}
Oh, and you should probably look into using an IRQ domain for the MSI
support in this driver.
> +int arch_setup_msi_irq(struct pci_dev *pdev, struct msi_desc *desc)
> +{
> + int irq, pos, msgvec;
> + u16 msg_ctr;
> + struct msi_msg msg;
> + struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
> +
> + if (!pp) {
> + BUG();
> + return -EINVAL;
> + }
> +
> + pci_read_config_word(pdev, desc->msi_attrib.pos+PCI_MSI_FLAGS,
> + &msg_ctr);
> + msgvec = (msg_ctr&PCI_MSI_FLAGS_QSIZE) >> 4;
> + if (msgvec == 0)
> + msgvec = (msg_ctr & PCI_MSI_FLAGS_QMASK) >> 1;
> + if (msgvec > 5)
> + msgvec = 0;
> +
> + irq = assign_irq((1 << msgvec), desc, &pos);
> + if (irq < 0)
> + return irq;
> +
> + msg_ctr &= ~PCI_MSI_FLAGS_QSIZE;
> + msg_ctr |= msgvec << 4;
> + pci_write_config_word(pdev, desc->msi_attrib.pos + PCI_MSI_FLAGS,
> + msg_ctr);
> + desc->msi_attrib.multiple = msgvec;
> +
> + msg.address_hi = 0x0;
> + msg.address_lo = __virt_to_phys((u32)(&msi_data));
> + msg.data = pos;
> + write_msi_msg(irq, &msg);
> +
> + return 0;
> +}
> +
> +void arch_teardown_msi_irq(unsigned int irq)
> +{
> + clear_irq(irq);
> +}
And we've reworked this largely so that drivers no longer provide arch_*
functions because that prevents multi-platform support. So I think you
need to port this to the new msi_chip infrastructure that's being
introduced in 3.12.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: exynos: add support for MSI
2013-08-12 10:56 ` Thierry Reding
@ 2013-08-12 11:47 ` Pratyush Anand
2013-08-23 4:58 ` Jingoo Han
1 sibling, 0 replies; 6+ messages in thread
From: Pratyush Anand @ 2013-08-12 11:47 UTC (permalink / raw)
To: Thierry Reding, Jingoo Han
Cc: Bjorn Helgaas, linux-pci, linux-samsung-soc, Kukjin Kim,
Mohit KUMAR DCG, Siva Reddy Kallam,
'SRIKANTH TUMKUR SHIVANAND',
Arnd Bergmann, 'Sean Cross',
'Kishon Vijay Abraham I', 'Thomas Petazzoni',
linux-kernel, devicetree
On Mon, Aug 12, 2013 at 06:56:40PM +0800, Thierry Reding wrote:
> On Mon, Aug 12, 2013 at 05:56:47PM +0900, Jingoo Han wrote:
> [...]
> > diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> > index 855d4a7..9ef1c95 100644
> > --- a/arch/arm/mach-exynos/Kconfig
> > +++ b/arch/arm/mach-exynos/Kconfig
> > @@ -93,6 +93,7 @@ config SOC_EXYNOS5440
> > default y
> > depends on ARCH_EXYNOS5
> > select ARCH_HAS_OPP
> > + select ARCH_SUPPORTS_MSI
>
> This symbol goes away in Thomas Petazzoni's MSI patch series which is
> targetted at 3.12, so I don't think you should add that here.
>
> > +#ifdef CONFIG_PCI_MSI
> > +static void exynos_pcie_clear_irq_level(struct pcie_port *pp)
> > +{
> > + u32 val;
> > + struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
> > + void __iomem *elbi_base = exynos_pcie->elbi_base;
> > +
> > + val = readl(elbi_base + PCIE_IRQ_LEVEL);
> > + writel(val, elbi_base + PCIE_IRQ_LEVEL);
> > + return;
> > +}
>
> I'm a little confused by this: the above code seems to access the PCIe
> controller registers to clear an interrupt, but you pass in a PCIe
> port...
>
One struct pcie_port is associated with one controller and it has been
assumed that there is only one root port per controller.
[...]
> > +void dw_pcie_msi_init(struct pcie_port *pp)
> > +{
> > + /* program the msi_data */
> > + dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
> > + __virt_to_phys((u32)(&msi_data)));
>
> That's slightly odd. You convert the virtual address of a local variable
> (local to the file) to a physical address and program that into a
> register. I assume that it works since you've probably tested this, but
> I wonder if it's safe to do this. Perhaps a better way would be to
> allocate a single free page (__get_free_pages(GFP_KERNEL, 0)) and write
> the physical address of that into the register instead.
>
also msi_data must be different for different controller. Something
like &msi_data[pp->port].
[...]
> > +void arch_teardown_msi_irq(unsigned int irq)
> > +{
> > + clear_irq(irq);
> > +}
>
> And we've reworked this largely so that drivers no longer provide arch_*
> functions because that prevents multi-platform support. So I think you
> need to port this to the new msi_chip infrastructure that's being
> introduced in 3.12.
Yes, its needed.
Regards
Pratyush
>
> Thierry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: exynos: add support for MSI
2013-08-12 9:12 ` Sachin Kamat
@ 2013-08-22 5:25 ` Jingoo Han
0 siblings, 0 replies; 6+ messages in thread
From: Jingoo Han @ 2013-08-22 5:25 UTC (permalink / raw)
To: 'Sachin Kamat'
Cc: 'Bjorn Helgaas',
linux-pci, linux-samsung-soc, 'Kukjin Kim',
'Pratyush Anand', 'Mohit KUMAR',
'Siva Reddy Kallam', 'SRIKANTH TUMKUR SHIVANAND',
'Arnd Bergmann', 'Sean Cross',
'Kishon Vijay Abraham I', 'Thierry Reding',
'Thomas Petazzoni',
linux-kernel, devicetree, 'Jingoo Han'
On Monday, August 12, 2013 6:13 PM, Sachin Kamat wrote:
> On 12 August 2013 14:26, Jingoo Han <jg1.han@samsung.com> wrote:
> > This patch adds support for Message Signaled Interrupt in the
> > Exynops PCIe diver using Synopsys designware PCIe core IP.
>
> s/Exynops PCIe diver/Exynos PCIe driver
OK, I will fix this typo.
> > Signed-off-by: Siva Reddy Kallam <siva.kallam@samsung.com>
> > Signed-off-by: Srikanth T Shivanand <ts.srikanth@samsung.com>
> > Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> > Cc: Pratyush Anand <pratyush.anand@st.com>
> > Cc: Mohit KUMAR <Mohit.KUMAR@st.com>
> > ---
> > arch/arm/boot/dts/exynos5440.dtsi | 2 +
> > arch/arm/mach-exynos/Kconfig | 1 +
> > drivers/pci/host/pci-exynos.c | 60 ++++++++++
> > drivers/pci/host/pcie-designware.c | 213 ++++++++++++++++++++++++++++++++++++
> > drivers/pci/host/pcie-designware.h | 8 ++
> > 5 files changed, 284 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/exynos5440.dtsi b/arch/arm/boot/dts/exynos5440.dtsi
> > index 586134e..3746835 100644
> > --- a/arch/arm/boot/dts/exynos5440.dtsi
> > +++ b/arch/arm/boot/dts/exynos5440.dtsi
> > @@ -249,6 +249,7 @@
> > interrupt-map-mask = <0 0 0 0>;
> > interrupt-map = <0x0 0 &gic 53>;
> > num-lanes = <4>;
> > + msi-base = <200>;
>
> Please update the bindings documentation too.
OK, I will updated the bindings documentation.
[.....]
> > +#ifdef CONFIG_PCI_MSI
> > +static void exynos_pcie_clear_irq_level(struct pcie_port *pp)
> > +{
> > + u32 val;
> > + struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
> > + void __iomem *elbi_base = exynos_pcie->elbi_base;
> > +
> > + val = readl(elbi_base + PCIE_IRQ_LEVEL);
> > + writel(val, elbi_base + PCIE_IRQ_LEVEL);
>
> Sorry, I did not get this. Writing the value read from the same
> register without any operation.
It was intended to clear the bits by writing 1 of each bit.
But I will remove this function.
My coworker, Srikanth T Shivanand found that this function is
unnecessary. This is because PCIE_IRQ_LEVEL register is read-only
register. Also, PCIE_IRQ_LEVEL register is already cleared before
this function is called.
Thank you for your comment.
Best regards,
Jingoo Han
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: exynos: add support for MSI
2013-08-12 10:56 ` Thierry Reding
2013-08-12 11:47 ` Pratyush Anand
@ 2013-08-23 4:58 ` Jingoo Han
1 sibling, 0 replies; 6+ messages in thread
From: Jingoo Han @ 2013-08-23 4:58 UTC (permalink / raw)
To: 'Thierry Reding'
Cc: 'Bjorn Helgaas',
linux-pci, linux-samsung-soc, 'Kukjin Kim',
'Pratyush Anand', 'Mohit KUMAR',
'Siva Reddy Kallam', 'SRIKANTH TUMKUR SHIVANAND',
'Arnd Bergmann', 'Sean Cross',
'Kishon Vijay Abraham I', 'Thomas Petazzoni',
linux-kernel, devicetree, 'Jingoo Han'
On Monday, August 12, 2013 7:57 PM, Thierry Reding wrote:
> On Mon, Aug 12, 2013 at 05:56:47PM +0900, Jingoo Han wrote:
> [...]
> > diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> > index 855d4a7..9ef1c95 100644
> > --- a/arch/arm/mach-exynos/Kconfig
> > +++ b/arch/arm/mach-exynos/Kconfig
> > @@ -93,6 +93,7 @@ config SOC_EXYNOS5440
> > default y
> > depends on ARCH_EXYNOS5
> > select ARCH_HAS_OPP
> > + select ARCH_SUPPORTS_MSI
>
> This symbol goes away in Thomas Petazzoni's MSI patch series which is
> targetted at 3.12, so I don't think you should add that here.
OK, I see.
I will remove ARCH_SUPPORTS_MSI.
[.....]
> > +#endif
> > +
> > static void exynos_pcie_enable_interrupts(struct pcie_port *pp)
> > {
> > exynos_pcie_enable_irq_pulse(pp);
> > +#ifdef CONFIG_PCI_MSI
> > + exynos_pcie_msi_init(pp);
> > +#endif
> > return;
> > }
>
> Instead of the whole #ifdef business above, can't you just use something
> like this in exynos_pcie_enable_interrupts():
>
> if (IS_ENABLED(CONFIG_PCI_MSI))
> exynos_pcie_msi_init(pp);
>
> Now you can drop the #ifdef guards and the compiler will throw away all
> the related code automatically if PCI_MSI is not selected because the
> functions are all static and unused. This has the advantage of compiling
> all the code whether or not PCI_MSI is selected or not, therefore
> increasing compile coverage of the driver.
OK, I see.
I will use 'if IS_ENABLED(CONFIG_PCI_MSI))', and remove #ifdef guards.
[.....]
> > +
> > +void arch_teardown_msi_irq(unsigned int irq)
> > +{
> > + clear_irq(irq);
> > +}
>
> And we've reworked this largely so that drivers no longer provide arch_*
> functions because that prevents multi-platform support. So I think you
> need to port this to the new msi_chip infrastructure that's being
> introduced in 3.12.
OK, I have looked at the new msi_chip infrastructure made by Thomas Petazzoni.
I will use this msi_chip.
I really appreciate your comment. :)
Thank you.
Best regards,
Jingoo Han
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-23 4:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-12 8:56 [PATCH] PCI: exynos: add support for MSI Jingoo Han
2013-08-12 9:12 ` Sachin Kamat
2013-08-22 5:25 ` Jingoo Han
2013-08-12 10:56 ` Thierry Reding
2013-08-12 11:47 ` Pratyush Anand
2013-08-23 4:58 ` Jingoo Han
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).