[PATCH 0/6] Add support for the ICU unit in Marvell Armada 7K/8K

[PATCH 0/6] Add support for the ICU unit in Marvell Armada 7K/8K

[Thomas Petazzoni]

Hello,

The Marvell Armada 7K/8K SoCs are composed of two parts: the AP (which
contains the CPU cores) and the CP (which contains most
peripherals). The 7K SoCs have one CP, while the 8K SoCs have two CPs,
doubling the number of available peripherals.

In terms of interrupt handling, all devices in the CPs are connected
through wired interrupt to a unit called ICU located in each CP. This
unit converts the wired interrupts from the devices into memory
transactions.

Inside the AP, there is a GIC extension called GICP, which allows a
memory write transaction to trigger a GIC SPI interrupt. The ICUs in
each CP are therefore configured to trigger a memory write into the
appropriate GICP register so that a wired interrupt from a CP device
is converted into a memory write, itself converted into a regular GIC
SPI interrupt.

Until now, the configuration of the ICU was done statically by the
firmware, and therefore the Device Tree files in Linux were specifying
directly GIC interrupts for the interrupts of CP devices. However,
with the growing number of devices in the CP, a static allocation
scheme doesn't work for the long term.

This patch series therefore makes Linux aware of the ICU: GIC SPI
interrupts are dynamically allocated, and the ICU is configured
accordingly to route a CP wired interrupt to the allocated GIC SPI
interrupt.

In detail:

 - The first two patches are the Device Tree binding patches

 - The third patch is a minimal driver for the GICP unit. All it does
   is clear interrupts that may have been left pending by the
   firmware.

 - The fourth patch is the most important done, which adds the driver
   for the ICU itself.

 - The fifth patch adjust Kconfig.platforms to select the GICP and ICU
   drivers.

 - The last patch adjusts the Device Tree files of the Armada 7K/8K to
   use the ICU.

Best regards,

Thomas

Thomas Petazzoni (6):
  dt-bindings: interrupt-controller: add DT binding for the Marvell GICP
  dt-bindings: interrupt-controller: add DT binding for the Marvell ICU
  irqchip: irq-mvebu-gicp: new driver for Marvell GICP
  irqchip: irq-mvebu-icu: new driver for Marvell ICU
  arm64: marvell: enable ICU and GICP drivers
  arm64: dts: marvell: enable GICP and ICU on Armada 7K/8K

 .../bindings/interrupt-controller/marvell,gicp.txt |  20 ++
 .../bindings/interrupt-controller/marvell,icu.txt  |  57 ++++
 arch/arm64/Kconfig.platforms                       |   2 +
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi      |   5 +
 .../boot/dts/marvell/armada-cp110-master.dtsi      |  60 ++--
 .../arm64/boot/dts/marvell/armada-cp110-slave.dtsi |  54 +--
 drivers/irqchip/Kconfig                            |   6 +
 drivers/irqchip/Makefile                           |   2 +
 drivers/irqchip/irq-mvebu-gicp.c                   |  53 +++
 drivers/irqchip/irq-mvebu-icu.c                    | 373 +++++++++++++++++++++
 .../dt-bindings/interrupt-controller/mvebu-icu.h   |  15 +
 11 files changed, 599 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,gicp.txt
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
 create mode 100644 drivers/irqchip/irq-mvebu-gicp.c
 create mode 100644 drivers/irqchip/irq-mvebu-icu.c
 create mode 100644 include/dt-bindings/interrupt-controller/mvebu-icu.h

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/6] dt-bindings: interrupt-controller: add DT binding for the Marvell GICP

[Thomas Petazzoni]

This commit adds the Device Tree binding documentation for the Marvell
GICP, an extension to the GIC that allows to trigger GIC SPI interrupts
using memory transactions. It is used by the ICU unit in the Marvell
CP110 block to turn wired interrupts inside the CP into SPI interrupts
at the GIC level in the AP.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../bindings/interrupt-controller/marvell,gicp.txt   | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,gicp.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,gicp.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,gicp.txt
new file mode 100644
index 0000000..996bdbf
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,gicp.txt
@@ -0,0 +1,20 @@
+Marvell GICP Controller
+-----------------------
+
+GICP is a Marvell extension of the GIC that allows to trigger GIC SPI
+interrupts by doing a memory transaction. It is used by the ICU
+located in the Marvell CP110 to turn wired interrupts inside the CP
+into GIC SPI interrupts.
+
+Required properties:
+
+- compatible: Must be "marvell,gicp"
+
+- reg: Must be the address and size of the GICP SPI registers
+
+Example:
+
+gicp_spi: gicp-spi@3f0040 {
+	compatible = "marvell,gicp";
+	reg = <0x3f0040 0x10>;
+};
-- 
2.7.4

[PATCH 2/6] dt-bindings: interrupt-controller: add DT binding for the Marvell ICU

[Thomas Petazzoni]

This commit adds the Device Tree binding documentation for the Marvell
ICU interrupt controller, which collects wired interrupts from the
devices located into the CP110 hardware block of Marvell Armada 7K/8K,
and converts them into SPI interrupts in the GIC located in the AP
hardware block, using the GICP extension.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../bindings/interrupt-controller/marvell,icu.txt  | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
new file mode 100644
index 0000000..e0b4068
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
@@ -0,0 +1,57 @@
+Marvell ICU Interrupt Controller
+--------------------------------
+
+The Marvell ICU (Interrupt Consolidation Unit) controller is
+responsible for collecting all wired-interrupt sources in the CP and
+communicating them to the GIC in the AP, the unit translates interrupt
+requests on input wires to MSG memory mapped transactions to the GIC.
+
+The interrupts from the ICU to the GIC can be mapped to one of the following groups:
+
+- Shared Peripheral Interrupt - Non-Secured (SPI_NSR)
+- Shared Peripheral Interrupt - Secured (SPI_SR)
+- System Error Interrupt (SEI)
+- RAM Error Interrupt (REI)
+
+Required properties:
+
+- compatible: Should be "marvell,icu"
+
+- reg: Should contain ICU registers location and length.
+
+- #interrupt-cells: Specifies the number of cells needed to encode an
+  interrupt source. The type shall be a <u32> and the value shall be
+  3.
+
+  The 1st cell is the group type of the ICU interrupt (SPI_NSR,
+  SPI_SR, SEI, and REI).
+
+  The 2nd cell is the index of the interrupt in the ICU unit.
+
+  The 3rd cell is the type of the interrupt. See arm,gic.txt for
+  details.
+
+- interrupt-controller: Identifies the node as an interrupt
+  controller.
+
+- interrupt-parent: Indicates the node of the parent interrupt
+  controller. Should be pointer to the GIC.
+
+- gicp: Should point to the GICP controller, the GIC extension that
+  allows to trigger interrupts using MSG memory mapped transactions.
+
+Example:
+
+icu: interrupt-controller@1e0000 {
+	compatible = "marvell,icu";
+	reg = <0x1e0000 0x10>;
+	#interrupt-cells = <2>;
+	interrupt-controller;
+	interrupt-parent = <&gic>;
+	gicp = <&gicp>;
+};
+
+usb3h0: usb3@500000 {
+	interrupt-parent = <&icu>;
+	interrupts = <ICU_GRP_NSR 106 IRQ_TYPE_LEVEL_HIGH>;
+};
-- 
2.7.4

Re: [PATCH 2/6] dt-bindings: interrupt-controller: add DT binding for the Marvell ICU

[Marc Zyngier]

On 30/05/17 10:16, Thomas Petazzoni wrote:
> This commit adds the Device Tree binding documentation for the Marvell
> ICU interrupt controller, which collects wired interrupts from the
> devices located into the CP110 hardware block of Marvell Armada 7K/8K,
> and converts them into SPI interrupts in the GIC located in the AP
> hardware block, using the GICP extension.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../bindings/interrupt-controller/marvell,icu.txt  | 57 ++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
> new file mode 100644
> index 0000000..e0b4068
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt
> @@ -0,0 +1,57 @@
> +Marvell ICU Interrupt Controller
> +--------------------------------
> +
> +The Marvell ICU (Interrupt Consolidation Unit) controller is
> +responsible for collecting all wired-interrupt sources in the CP and
> +communicating them to the GIC in the AP, the unit translates interrupt
> +requests on input wires to MSG memory mapped transactions to the GIC.
> +
> +The interrupts from the ICU to the GIC can be mapped to one of the following groups:
> +
> +- Shared Peripheral Interrupt - Non-Secured (SPI_NSR)
> +- Shared Peripheral Interrupt - Secured (SPI_SR)
> +- System Error Interrupt (SEI)
> +- RAM Error Interrupt (REI)
> +
> +Required properties:
> +
> +- compatible: Should be "marvell,icu"
> +
> +- reg: Should contain ICU registers location and length.
> +
> +- #interrupt-cells: Specifies the number of cells needed to encode an
> +  interrupt source. The type shall be a <u32> and the value shall be
> +  3.

Yup...

> +
> +  The 1st cell is the group type of the ICU interrupt (SPI_NSR,
> +  SPI_SR, SEI, and REI).

Is it worth documenting what these are?

> +
> +  The 2nd cell is the index of the interrupt in the ICU unit.
> +
> +  The 3rd cell is the type of the interrupt. See arm,gic.txt for
> +  details.
> +
> +- interrupt-controller: Identifies the node as an interrupt
> +  controller.
> +
> +- interrupt-parent: Indicates the node of the parent interrupt
> +  controller. Should be pointer to the GIC.
> +
> +- gicp: Should point to the GICP controller, the GIC extension that
> +  allows to trigger interrupts using MSG memory mapped transactions.
> +
> +Example:
> +
> +icu: interrupt-controller@1e0000 {
> +	compatible = "marvell,icu";
> +	reg = <0x1e0000 0x10>;
> +	#interrupt-cells = <2>;

Oh wait...

> +	interrupt-controller;
> +	interrupt-parent = <&gic>;
> +	gicp = <&gicp>;

Should this be prefixed with a vendor specific identifier, just in case?

> +};
> +
> +usb3h0: usb3@500000 {
> +	interrupt-parent = <&icu>;
> +	interrupts = <ICU_GRP_NSR 106 IRQ_TYPE_LEVEL_HIGH>;
> +};
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] dt-bindings: interrupt-controller: add DT binding for the Marvell ICU

[Thomas Petazzoni]

Hello,

Thanks for the quick review!

On Tue, 30 May 2017 11:37:10 +0100, Marc Zyngier wrote:

> > +  The 1st cell is the group type of the ICU interrupt (SPI_NSR,
> > +  SPI_SR, SEI, and REI).  
> 
> Is it worth documenting what these are?

Sure, will do.

> > +icu: interrupt-controller@1e0000 {
> > +	compatible = "marvell,icu";
> > +	reg = <0x1e0000 0x10>;
> > +	#interrupt-cells = <2>;  
> 
> Oh wait...

Gaah, I did spot this mistake, but then forgot to fix it. Will do.

> > +	interrupt-controller;
> > +	interrupt-parent = <&gic>;
> > +	gicp = <&gicp>;  
> 
> Should this be prefixed with a vendor specific identifier, just in case?

Would marvell,gicp be preferable?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP

[Thomas Petazzoni]

This commit adds a simple driver for the Marvell GICP, a hardware unit
that converts memory writes into GIC SPI interrupts. The driver doesn't
do anything but clear all interrupts at boot time, to avoid spurious
interrupts left by the firmware.

The GICP registers are directly written to in hardware by the ICU unit,
which is configured in a separate driver.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/irqchip/Kconfig          |  3 +++
 drivers/irqchip/Makefile         |  1 +
 drivers/irqchip/irq-mvebu-gicp.c | 53 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+)
 create mode 100644 drivers/irqchip/irq-mvebu-gicp.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 478f8ac..e527ee5 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -268,6 +268,9 @@ config IRQ_MXS
 	select IRQ_DOMAIN
 	select STMP_DEVICE
 
+config MVEBU_GICP
+	bool
+
 config MVEBU_ODMI
 	bool
 	select GENERIC_MSI_IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b64c59b..11eb858 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
 obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
 obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
 obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
+obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.o
 obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
 obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
 obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
diff --git a/drivers/irqchip/irq-mvebu-gicp.c b/drivers/irqchip/irq-mvebu-gicp.c
new file mode 100644
index 0000000..4effed4
--- /dev/null
+++ b/drivers/irqchip/irq-mvebu-gicp.c
@@ -0,0 +1,53 @@
+/*
+ * Copyright (C) 2017 Marvell
+ *
+ * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/io.h>
+#include <linux/platform_device.h>
+
+#define GICP_SETSPI_NSR_OFFSET	0x0
+#define GICP_CLRSPI_NSR_OFFSET	0x8
+
+#define GICP_INT_COUNT		128
+
+static int mvebu_gicp_probe(struct platform_device *pdev)
+{
+	void __iomem *regs;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	regs = ioremap(res->start, resource_size(res));
+	if (!regs)
+		return -ENOMEM;
+
+	for (i = 0; i < GICP_INT_COUNT; i++)
+		writel(i, regs + GICP_CLRSPI_NSR_OFFSET);
+
+	iounmap(regs);
+
+	return 0;
+}
+
+static const struct of_device_id mvebu_gicp_of_match[] = {
+	{ .compatible = "marvell,gicp", },
+	{},
+};
+
+static struct platform_driver mvebu_gicp_driver = {
+	.probe  = mvebu_gicp_probe,
+	.driver = {
+		.name = "mvebu-gicp",
+		.of_match_table = mvebu_gicp_of_match,
+	},
+};
+builtin_platform_driver(mvebu_gicp_driver);
-- 
2.7.4

Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP

[Marc Zyngier]

On 30/05/17 10:16, Thomas Petazzoni wrote:
> This commit adds a simple driver for the Marvell GICP, a hardware unit
> that converts memory writes into GIC SPI interrupts. The driver doesn't
> do anything but clear all interrupts at boot time, to avoid spurious
> interrupts left by the firmware.
> 
> The GICP registers are directly written to in hardware by the ICU unit,
> which is configured in a separate driver.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/irqchip/Kconfig          |  3 +++
>  drivers/irqchip/Makefile         |  1 +
>  drivers/irqchip/irq-mvebu-gicp.c | 53 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mvebu-gicp.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 478f8ac..e527ee5 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -268,6 +268,9 @@ config IRQ_MXS
>  	select IRQ_DOMAIN
>  	select STMP_DEVICE
>  
> +config MVEBU_GICP
> +	bool
> +
>  config MVEBU_ODMI
>  	bool
>  	select GENERIC_MSI_IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b64c59b..11eb858 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
>  obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
>  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
>  obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
> +obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.o
>  obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
>  obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
> diff --git a/drivers/irqchip/irq-mvebu-gicp.c b/drivers/irqchip/irq-mvebu-gicp.c
> new file mode 100644
> index 0000000..4effed4
> --- /dev/null
> +++ b/drivers/irqchip/irq-mvebu-gicp.c
> @@ -0,0 +1,53 @@
> +/*
> + * Copyright (C) 2017 Marvell
> + *
> + * Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +
> +#define GICP_SETSPI_NSR_OFFSET	0x0
> +#define GICP_CLRSPI_NSR_OFFSET	0x8
> +
> +#define GICP_INT_COUNT		128
> +
> +static int mvebu_gicp_probe(struct platform_device *pdev)
> +{
> +	void __iomem *regs;
> +	struct resource *res;
> +	int i;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	regs = ioremap(res->start, resource_size(res));
> +	if (!regs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < GICP_INT_COUNT; i++)
> +		writel(i, regs + GICP_CLRSPI_NSR_OFFSET);

What does this do on an edge interrupt? I bet this doesn't have any
effect, so you may want to use the irq_set_irqchip_state() API to clear
a potential pending state instead (and you may want to wire it in the
ICU driver itself as well).

> +
> +	iounmap(regs);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mvebu_gicp_of_match[] = {
> +	{ .compatible = "marvell,gicp", },
> +	{},
> +};
> +
> +static struct platform_driver mvebu_gicp_driver = {
> +	.probe  = mvebu_gicp_probe,
> +	.driver = {
> +		.name = "mvebu-gicp",
> +		.of_match_table = mvebu_gicp_of_match,
> +	},
> +};
> +builtin_platform_driver(mvebu_gicp_driver);
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP

[Thomas Petazzoni]

Hello,

On Tue, 30 May 2017 14:55:57 +0100, Marc Zyngier wrote:

> > +	for (i = 0; i < GICP_INT_COUNT; i++)
> > +		writel(i, regs + GICP_CLRSPI_NSR_OFFSET);  
> 
> What does this do on an edge interrupt?

I guess nothing. What the ICU does is:

 * For level interrupts: when the interrupt wire is asserted, write to
   SETNSR, when the interrupt wire is deasserted, write to CLRNSR

 * For edge interrupts: only the interrupt assertion causes a write to
   SETNSR.

> I bet this doesn't have any effect

Indeed. But do we care? Can an edge interrupt be left pending from the
firmware?

>, so you may want to use the irq_set_irqchip_state() API to clear a
> potential pending state instead (and you may want to wire it in the
> ICU driver itself as well).

I'm not sure how to use this irq_set_irqchip_state() API. I guess it
needs a virq that corresponds to the GIC SPI interrupt, and I'm not
sure how to get that.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP

[Marc Zyngier]

On 30/05/17 15:54, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 30 May 2017 14:55:57 +0100, Marc Zyngier wrote:
> 
>>> +	for (i = 0; i < GICP_INT_COUNT; i++)
>>> +		writel(i, regs + GICP_CLRSPI_NSR_OFFSET);  
>>
>> What does this do on an edge interrupt?
> 
> I guess nothing. What the ICU does is:
> 
>  * For level interrupts: when the interrupt wire is asserted, write to
>    SETNSR, when the interrupt wire is deasserted, write to CLRNSR
> 
>  * For edge interrupts: only the interrupt assertion causes a write to
>    SETNSR.
> 
>> I bet this doesn't have any effect
> 
> Indeed. But do we care? Can an edge interrupt be left pending from the
> firmware?

I cannot see why not. It is just as likely as a level interrupt.

> 
>> , so you may want to use the irq_set_irqchip_state() API to clear a
>> potential pending state instead (and you may want to wire it in the
>> ICU driver itself as well).
> 
> I'm not sure how to use this irq_set_irqchip_state() API. I guess it
> needs a virq that corresponds to the GIC SPI interrupt, and I'm not
> sure how to get that.

You do have the virtual interrupt when doing the allocation (it is
passed as a parameter). So you could perform the mapping (call into the
lower layers), and clear the pending bit using the above API.

But maybe you don't have any edge interrupt on this SoC, and it doesn't
matter.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP

[Thomas Petazzoni]

Hello,

On Tue, 30 May 2017 16:17:41 +0100, Marc Zyngier wrote:

> > Indeed. But do we care? Can an edge interrupt be left pending from the
> > firmware?  
> 
> I cannot see why not. It is just as likely as a level interrupt.

OK.

> > I'm not sure how to use this irq_set_irqchip_state() API. I guess it
> > needs a virq that corresponds to the GIC SPI interrupt, and I'm not
> > sure how to get that.  
> 
> You do have the virtual interrupt when doing the allocation (it is
> passed as a parameter). So you could perform the mapping (call into the
> lower layers), and clear the pending bit using the above API.

So in mvebu_icu_irq_domain_alloc(), if I do:

	irq_set_irqchip_state(virq, IRQCHIP_STATE_MASKED, true);

this will go all the way to the ->irq_set_irqchip_state() in the GIC? I
thought the virq we had was referring to an irq from the ICU domain,
not from the GIC one. But maybe I'm still getting confused by all these
irq domains.

> But maybe you don't have any edge interrupt on this SoC, and it doesn't
> matter.

We currently don't have any in the devices we support in the SoC, but
since the ICU does support edge interrupts explicitly, it's nicer if we
can get this right. Plus if this actually works, we don't need the
marvell,gicp "driver" anymore.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP

[Marc Zyngier]

On 30/05/17 16:25, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 30 May 2017 16:17:41 +0100, Marc Zyngier wrote:
> 
>>> Indeed. But do we care? Can an edge interrupt be left pending from the
>>> firmware?  
>>
>> I cannot see why not. It is just as likely as a level interrupt.
> 
> OK.
> 
>>> I'm not sure how to use this irq_set_irqchip_state() API. I guess it
>>> needs a virq that corresponds to the GIC SPI interrupt, and I'm not
>>> sure how to get that.  
>>
>> You do have the virtual interrupt when doing the allocation (it is
>> passed as a parameter). So you could perform the mapping (call into the
>> lower layers), and clear the pending bit using the above API.
> 
> So in mvebu_icu_irq_domain_alloc(), if I do:
> 
> 	irq_se_irqchip_state(virq, IRQCHIP_STATE_MASKED, true);

That would be

	irq_set_irqchip_state(virq, IRQCHIP_STATE_PENDING, false);

instead. It is expected that the interrupt is already masked (otherwise,
you could be in trouble).

> this will go all the way to the ->irq_set_irqchip_state() in the GIC? I

Yup.

> thought the virq we had was referring to an irq from the ICU domain,
> not from the GIC one. But maybe I'm still getting confused by all these
> irq domains.

I'm afraid you are... ;-) This is a hierarchical domain, so the virq is
constant across the whole stack, only the irqchip-specific identifier
(hwirq) changes (see how you get it from the core code, and propatage it
to the parent domain). If you implement irq_set_irqchip_state at the ICU
level by directly calling into the parent, you should be just fine.

> 
>> But maybe you don't have any edge interrupt on this SoC, and it doesn't
>> matter.
> 
> We currently don't have any in the devices we support in the SoC, but
> since the ICU does support edge interrupts explicitly, it's nicer if we
> can get this right. Plus if this actually works, we don't need the
> marvell,gicp "driver" anymore.

That'd be great.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Thomas Petazzoni]

The Marvell ICU unit is found in the CP110 block of the Marvell Armada
7K and 8K SoCs. It collects the wired interrupts of the devices located
in the CP110 and turns them into SPI interrupts in the GIC located in
the AP806 side of the SoC, by using a memory transaction.

Until now, the ICU was configured in a static fashion by the firmware,
and Linux was relying on this static configuration. By having Linux
configure the ICU, we are more flexible, and we can allocate dynamically
the GIC SPI interrupts only for devices that are actually in use.

The driver was initially written by Hanna Hawa <hannah@marvell.com>.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/irqchip/Kconfig                            |   3 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-mvebu-icu.c                    | 373 +++++++++++++++++++++
 .../dt-bindings/interrupt-controller/mvebu-icu.h   |  15 +
 4 files changed, 392 insertions(+)
 create mode 100644 drivers/irqchip/irq-mvebu-icu.c
 create mode 100644 include/dt-bindings/interrupt-controller/mvebu-icu.h

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index e527ee5..676232a 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -271,6 +271,9 @@ config IRQ_MXS
 config MVEBU_GICP
 	bool
 
+config MVEBU_ICU
+	bool
+
 config MVEBU_ODMI
 	bool
 	select GENERIC_MSI_IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 11eb858..18fa5fa 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
 obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
 obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
 obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.o
+obj-$(CONFIG_MVEBU_ICU)			+= irq-mvebu-icu.o
 obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
 obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
 obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
new file mode 100644
index 0000000..8a5c217
--- /dev/null
+++ b/drivers/irqchip/irq-mvebu-icu.c
@@ -0,0 +1,373 @@
+/*
+ * Copyright (C) 2017 Marvell
+ *
+ * Hanna Hawa <hannah@marvell.com>
+ * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/interrupt-controller/mvebu-icu.h>
+
+/* ICU registers */
+#define ICU_SETSPI_NSR_AL	0x10
+#define ICU_SETSPI_NSR_AH	0x14
+#define ICU_CLRSPI_NSR_AL	0x18
+#define ICU_CLRSPI_NSR_AH	0x1c
+#define ICU_INT_CFG(x)          (0x100 + 4 * x)
+#define   ICU_INT_ENABLE	BIT(24)
+#define   ICU_IS_EDGE		BIT(28)
+#define   ICU_GROUP_SHIFT	29
+
+/* GICP registers */
+#define GICP_SETSPI_NSR_OFFSET	0x0
+#define GICP_CLRSPI_NSR_OFFSET	0x8
+
+/* ICU definitions */
+#define ICU_MAX_IRQS		207
+#define IRQS_PER_ICU		64
+#define ICU_MAX_SPI_IRQ_IN_GIC	(2 * IRQS_PER_ICU)
+#define ICU_GIC_SPI_BASE0	64
+#define ICU_GIC_SPI_BASE1	288
+
+#define ICU_SATA0_IRQ_INT	109
+#define ICU_SATA1_IRQ_INT	107
+
+struct mvebu_icu {
+	struct irq_chip irq_chip;
+	void __iomem *base;
+	struct irq_domain *domain;
+	struct device *dev;
+};
+
+static DEFINE_SPINLOCK(icu_lock);
+static DECLARE_BITMAP(icu_irq_alloc, ICU_MAX_SPI_IRQ_IN_GIC);
+
+static unsigned int
+mvebu_icu_icuidx_to_gicspi(unsigned int icuidx)
+{
+	if (icuidx < ICU_GIC_SPI_BASE0)
+		return ICU_GIC_SPI_BASE0 + icuidx;
+	else
+		return ICU_GIC_SPI_BASE1 + (icuidx - IRQS_PER_ICU);
+}
+
+static unsigned int
+mvebu_icu_gicspi_to_icuidx(unsigned int gicspi)
+{
+	if (gicspi < ICU_GIC_SPI_BASE1)
+		return gicspi - ICU_GIC_SPI_BASE0;
+	else
+		return gicspi - ICU_GIC_SPI_BASE1 + IRQS_PER_ICU;
+}
+
+static int
+mvebu_icu_irq_parent_domain_alloc(struct irq_domain *domain,
+				  unsigned int virq, unsigned int type,
+				  int *irq_msg_num)
+{
+	struct mvebu_icu *icu = domain->host_data;
+	struct irq_fwspec fwspec;
+	int ret;
+
+	/* Find first free interrupt in ICU pool */
+	spin_lock(&icu_lock);
+	*irq_msg_num = find_first_zero_bit(icu_irq_alloc,
+					   ICU_MAX_SPI_IRQ_IN_GIC);
+	if (*irq_msg_num == ICU_MAX_SPI_IRQ_IN_GIC) {
+		dev_err(icu->dev, "No free ICU interrupt found\n");
+		spin_unlock(&icu_lock);
+		return -ENOSPC;
+	}
+	set_bit(*irq_msg_num, icu_irq_alloc);
+	spin_unlock(&icu_lock);
+
+	fwspec.fwnode = domain->parent->fwnode;
+	fwspec.param_count = 3;
+	fwspec.param[0] = GIC_SPI;
+	fwspec.param[1] = mvebu_icu_icuidx_to_gicspi(*irq_msg_num) - 32;
+	fwspec.param[2] = type;
+
+	/* Allocate the IRQ in the parent */
+	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
+	if (ret) {
+		spin_lock(&icu_lock);
+		clear_bit(*irq_msg_num, icu_irq_alloc);
+		spin_unlock(&icu_lock);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void
+mvebu_icu_irq_parent_domain_free(struct irq_domain *domain,
+				 unsigned int virq,
+				 int irq_msg_num)
+{
+	irq_domain_free_irqs_parent(domain, virq, 1);
+
+	spin_lock(&icu_lock);
+	clear_bit(irq_msg_num, icu_irq_alloc);
+	spin_unlock(&icu_lock);
+}
+
+static int
+mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
+			       unsigned long *hwirq, unsigned int *type)
+{
+	struct mvebu_icu *icu = d->host_data;
+	unsigned int icu_group;
+
+	/* Check the count of the parameters in dt */
+	if (WARN_ON(fwspec->param_count < 3)) {
+		dev_err(icu->dev, "wrong ICU parameter count %d\n",
+			fwspec->param_count);
+		return -EINVAL;
+	}
+
+	/* Only ICU group type is handled */
+	icu_group = fwspec->param[0];
+	if (icu_group != ICU_GRP_NSR && icu_group != ICU_GRP_SR &&
+	    icu_group != ICU_GRP_SEI && icu_group != ICU_GRP_REI) {
+		dev_err(icu->dev, "wrong ICU type %x\n", icu_group);
+		return -EINVAL;
+	}
+
+	*hwirq = fwspec->param[1];
+	if (*hwirq < 0) {
+		dev_err(icu->dev, "invalid interrupt number %ld\n", *hwirq);
+		return -EINVAL;
+	}
+
+	/* Mask the type to prevent wrong DT configuration */
+	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+
+	return 0;
+}
+
+static int
+mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+			   unsigned int nr_irqs, void *args)
+{
+	int err = 0, irq_msg_num = 0;
+	unsigned long hwirq;
+	unsigned int type = 0;
+	unsigned int icu_group, icu_int;
+	struct irq_fwspec *fwspec = args;
+	struct mvebu_icu *icu = domain->host_data;
+
+	err = mvebu_icu_irq_domain_translate(domain, fwspec, &hwirq, &type);
+	if (err) {
+		dev_err(icu->dev, "failed to translate ICU parameters\n");
+		return err;
+	}
+
+	icu_group = fwspec->param[0];
+
+	err = mvebu_icu_irq_parent_domain_alloc(domain, virq, type,
+						&irq_msg_num);
+	if (err) {
+		dev_err(icu->dev, "failed to allocate ICU interrupt in parent domain\n");
+		return err;
+	}
+
+	/* Configure the ICU with irq number & type */
+	icu_int = irq_msg_num | ICU_INT_ENABLE;
+	if (type & IRQ_TYPE_EDGE_RISING)
+		icu_int |= ICU_IS_EDGE;
+	icu_int |= icu_group << ICU_GROUP_SHIFT;
+	writel(icu_int, icu->base + ICU_INT_CFG(hwirq));
+
+	/*
+	 * The SATA unit has 2 ports, and a dedicated ICU entry per
+	 * port. The ahci sata driver supports only one irq interrupt
+	 * per SATA unit. To solve this conflict, we configure the 2
+	 * SATA wired interrupts in the south bridge into 1 GIC
+	 * interrupt in the north bridge. Even if only a single port
+	 * is enabled, if sata node is enabled, both interrupts are
+	 * configured (regardless of which port is actually in use).
+	 * The ICU index of SATA0 = 107, SATA1 = 109
+	 */
+	if (hwirq == ICU_SATA0_IRQ_INT || hwirq == ICU_SATA1_IRQ_INT) {
+		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA0_IRQ_INT));
+		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA1_IRQ_INT));
+	}
+
+	err = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					    &icu->irq_chip, icu);
+	if (err) {
+		dev_err(icu->dev, "failed to set the data to IRQ domain\n");
+		mvebu_icu_irq_parent_domain_free(domain, virq, irq_msg_num);
+		return err;
+	}
+
+	return 0;
+}
+
+static void
+mvebu_icu_irq_domain_free(struct irq_domain *domain, unsigned int virq,
+			  unsigned int nr_irqs)
+{
+	struct mvebu_icu *icu = domain->host_data;
+	struct irq_data *irq = irq_get_irq_data(virq);
+	struct irq_data *irq_parent = irq->parent_data;
+	int irq_msg_num = mvebu_icu_gicspi_to_icuidx(irqd_to_hwirq(irq_parent));
+
+	WARN_ON(nr_irqs != 1);
+
+	writel(0, icu->base + ICU_INT_CFG(irqd_to_hwirq(irq)));
+
+	mvebu_icu_irq_parent_domain_free(domain, virq, irq_msg_num);
+}
+
+static const struct irq_domain_ops mvebu_icu_domain_ops = {
+	.translate = mvebu_icu_irq_domain_translate,
+	.alloc     = mvebu_icu_irq_domain_alloc,
+	.free      = mvebu_icu_irq_domain_free,
+};
+
+static int mvebu_icu_probe(struct platform_device *pdev)
+{
+	struct mvebu_icu *icu;
+	struct irq_domain *parent_domain;
+	struct device_node *node = pdev->dev.of_node;
+	struct platform_device *gicp_pdev;
+	struct device_node *parent_irq_dn;
+	struct device_node *gicp_dn;
+	struct resource *res;
+	struct resource *gicp_res;
+	u32 i, icu_int;
+	int ret;
+
+	icu = devm_kzalloc(&pdev->dev, sizeof(struct mvebu_icu),
+			   GFP_KERNEL);
+	if (!icu)
+		return -ENOMEM;
+
+	icu->dev = &pdev->dev;
+
+	icu->irq_chip.name = kstrdup(dev_name(&pdev->dev), GFP_KERNEL);
+	icu->irq_chip.irq_mask = irq_chip_mask_parent;
+	icu->irq_chip.irq_unmask = irq_chip_unmask_parent;
+	icu->irq_chip.irq_eoi = irq_chip_eoi_parent;
+	icu->irq_chip.irq_set_type = irq_chip_set_type_parent;
+#ifdef CONFIG_SMP
+	icu->irq_chip.irq_set_affinity = irq_chip_set_affinity_parent;
+#endif
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	icu->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(icu->base)) {
+		dev_err(&pdev->dev, "Failed to map icu base address.\n");
+		return PTR_ERR(icu->base);
+	}
+
+	gicp_dn = of_parse_phandle(node, "gicp", 0);
+	if (!gicp_dn) {
+		dev_err(&pdev->dev, "Missing gicp property.\n");
+		return -ENODEV;
+	}
+
+	gicp_pdev = of_find_device_by_node(gicp_dn);
+	if (!gicp_pdev) {
+		dev_err(&pdev->dev, "Cannot find gicp device.\n");
+		return -ENODEV;
+	}
+
+	get_device(&gicp_pdev->dev);
+
+	/*
+	 * We really want the GICP to have cleared all interrupts
+	 * potentially left pending by the firmware before probing the
+	 * ICUs.
+	 */
+	if (!device_is_bound(&gicp_pdev->dev)) {
+		ret = -EPROBE_DEFER;
+		goto fail;
+	}
+
+	gicp_res = platform_get_resource(gicp_pdev, IORESOURCE_MEM, 0);
+	if (!gicp_res) {
+		dev_err(&pdev->dev, "Failed to get gicp resource\n");
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	parent_irq_dn = of_irq_find_parent(node);
+	if (!parent_irq_dn) {
+		dev_err(&pdev->dev, "failed to find parent IRQ node\n");
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	parent_domain = irq_find_host(parent_irq_dn);
+	if (!parent_domain) {
+		dev_err(&pdev->dev, "Unable to locate ICU parent domain\n");
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	/* Set Clear/Set ICU SPI message address in AP */
+	writel(upper_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
+	       icu->base + ICU_SETSPI_NSR_AH);
+	writel(lower_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
+	       icu->base + ICU_SETSPI_NSR_AL);
+	writel(upper_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
+	       icu->base + ICU_CLRSPI_NSR_AH);
+	writel(lower_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
+	       icu->base + ICU_CLRSPI_NSR_AL);
+
+	/*
+	 * Clean all ICU interrupts with type SPI_NSR, required to
+	 * avoid unpredictable SPI assignments done by firmware.
+	 */
+	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {
+		icu_int = readl(icu->base + ICU_INT_CFG(i));
+		if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR)
+			writel(0x0, icu->base + ICU_INT_CFG(i));
+	}
+
+	icu->domain = irq_domain_add_hierarchy(parent_domain, 0,
+					       ICU_MAX_SPI_IRQ_IN_GIC, node,
+					       &mvebu_icu_domain_ops, icu);
+	if (!icu->domain) {
+		dev_err(&pdev->dev, "Failed to create ICU domain\n");
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	return 0;
+fail:
+	put_device(&gicp_pdev->dev);
+	return ret;
+}
+
+static const struct of_device_id mvebu_icu_of_match[] = {
+	{ .compatible = "marvell,icu", },
+	{},
+};
+
+static struct platform_driver mvebu_icu_driver = {
+	.probe  = mvebu_icu_probe,
+	.driver = {
+		.name = "mvebu-icu",
+		.of_match_table = mvebu_icu_of_match,
+	},
+};
+builtin_platform_driver(mvebu_icu_driver);
diff --git a/include/dt-bindings/interrupt-controller/mvebu-icu.h b/include/dt-bindings/interrupt-controller/mvebu-icu.h
new file mode 100644
index 0000000..8249558
--- /dev/null
+++ b/include/dt-bindings/interrupt-controller/mvebu-icu.h
@@ -0,0 +1,15 @@
+/*
+ * This header provides constants for the MVEBU ICU driver.
+ */
+
+#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H
+#define _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H
+
+/* interrupt specifier cell 0 */
+
+#define ICU_GRP_NSR		0x0
+#define ICU_GRP_SR		0x1
+#define ICU_GRP_SEI		0x4
+#define ICU_GRP_REI		0x5
+
+#endif
-- 
2.7.4

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Marc Zyngier]

Hi Thomas,

Thanks for that, looks pretty interesting. A couple of comments below.

On 30/05/17 10:16, Thomas Petazzoni wrote:
> The Marvell ICU unit is found in the CP110 block of the Marvell Armada
> 7K and 8K SoCs. It collects the wired interrupts of the devices located
> in the CP110 and turns them into SPI interrupts in the GIC located in
> the AP806 side of the SoC, by using a memory transaction.
> 
> Until now, the ICU was configured in a static fashion by the firmware,
> and Linux was relying on this static configuration. By having Linux
> configure the ICU, we are more flexible, and we can allocate dynamically
> the GIC SPI interrupts only for devices that are actually in use.
> 
> The driver was initially written by Hanna Hawa <hannah@marvell.com>.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/irqchip/Kconfig                            |   3 +
>  drivers/irqchip/Makefile                           |   1 +
>  drivers/irqchip/irq-mvebu-icu.c                    | 373 +++++++++++++++++++++
>  .../dt-bindings/interrupt-controller/mvebu-icu.h   |  15 +
>  4 files changed, 392 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mvebu-icu.c
>  create mode 100644 include/dt-bindings/interrupt-controller/mvebu-icu.h
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index e527ee5..676232a 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -271,6 +271,9 @@ config IRQ_MXS
>  config MVEBU_GICP
>  	bool
>  
> +config MVEBU_ICU
> +	bool
> +
>  config MVEBU_ODMI
>  	bool
>  	select GENERIC_MSI_IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 11eb858..18fa5fa 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
>  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
>  obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
>  obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.o
> +obj-$(CONFIG_MVEBU_ICU)			+= irq-mvebu-icu.o
>  obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
>  obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
> new file mode 100644
> index 0000000..8a5c217
> --- /dev/null
> +++ b/drivers/irqchip/irq-mvebu-icu.c
> @@ -0,0 +1,373 @@
> +/*
> + * Copyright (C) 2017 Marvell
> + *
> + * Hanna Hawa <hannah@marvell.com>
> + * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/mvebu-icu.h>
> +
> +/* ICU registers */
> +#define ICU_SETSPI_NSR_AL	0x10
> +#define ICU_SETSPI_NSR_AH	0x14
> +#define ICU_CLRSPI_NSR_AL	0x18
> +#define ICU_CLRSPI_NSR_AH	0x1c
> +#define ICU_INT_CFG(x)          (0x100 + 4 * x)
> +#define   ICU_INT_ENABLE	BIT(24)
> +#define   ICU_IS_EDGE		BIT(28)
> +#define   ICU_GROUP_SHIFT	29
> +
> +/* GICP registers */
> +#define GICP_SETSPI_NSR_OFFSET	0x0
> +#define GICP_CLRSPI_NSR_OFFSET	0x8

Shouldn't that live in some gicp-specific include file?

> +
> +/* ICU definitions */
> +#define ICU_MAX_IRQS		207
> +#define IRQS_PER_ICU		64
> +#define ICU_MAX_SPI_IRQ_IN_GIC	(2 * IRQS_PER_ICU)

What's the relationship between ICU_MAX_IRQS and
IRQS_PER_ICU/ICU_MAX_SPI_IRQ_IN_GIC, if any? Is there only a single ICU?
Or can you have multiple ones?

> +#define ICU_GIC_SPI_BASE0	64
> +#define ICU_GIC_SPI_BASE1	288

My own gut feeling is that there will be another version of this IP one
of these days, with different bases. Should we bite the bullet right
away and put those in DT?

> +
> +#define ICU_SATA0_IRQ_INT	109
> +#define ICU_SATA1_IRQ_INT	107
> +
> +struct mvebu_icu {
> +	struct irq_chip irq_chip;
> +	void __iomem *base;
> +	struct irq_domain *domain;
> +	struct device *dev;
> +};
> +
> +static DEFINE_SPINLOCK(icu_lock);
> +static DECLARE_BITMAP(icu_irq_alloc, ICU_MAX_SPI_IRQ_IN_GIC);
> +
> +static unsigned int
> +mvebu_icu_icuidx_to_gicspi(unsigned int icuidx)
> +{
> +	if (icuidx < ICU_GIC_SPI_BASE0)
> +		return ICU_GIC_SPI_BASE0 + icuidx;
> +	else
> +		return ICU_GIC_SPI_BASE1 + (icuidx - IRQS_PER_ICU);

A small comment on how ICU indexes and GIC SPIs map would be good.
Correct me if I'm wrong, but it really looks like you're dealing with
two devices here, each with their own SPI window.

> +}
> +
> +static unsigned int
> +mvebu_icu_gicspi_to_icuidx(unsigned int gicspi)
> +{
> +	if (gicspi < ICU_GIC_SPI_BASE1)
> +		return gicspi - ICU_GIC_SPI_BASE0;
> +	else
> +		return gicspi - ICU_GIC_SPI_BASE1 + IRQS_PER_ICU;
> +}
> +
> +static int
> +mvebu_icu_irq_parent_domain_alloc(struct irq_domain *domain,
> +				  unsigned int virq, unsigned int type,
> +				  int *irq_msg_num)
> +{
> +	struct mvebu_icu *icu = domain->host_data;
> +	struct irq_fwspec fwspec;
> +	int ret;
> +
> +	/* Find first free interrupt in ICU pool */
> +	spin_lock(&icu_lock);
> +	*irq_msg_num = find_first_zero_bit(icu_irq_alloc,
> +					   ICU_MAX_SPI_IRQ_IN_GIC);
> +	if (*irq_msg_num == ICU_MAX_SPI_IRQ_IN_GIC) {
> +		dev_err(icu->dev, "No free ICU interrupt found\n");
> +		spin_unlock(&icu_lock);
> +		return -ENOSPC;
> +	}
> +	set_bit(*irq_msg_num, icu_irq_alloc);
> +	spin_unlock(&icu_lock);
> +
> +	fwspec.fwnode = domain->parent->fwnode;
> +	fwspec.param_count = 3;
> +	fwspec.param[0] = GIC_SPI;
> +	fwspec.param[1] = mvebu_icu_icuidx_to_gicspi(*irq_msg_num) - 32;
> +	fwspec.param[2] = type;
> +
> +	/* Allocate the IRQ in the parent */
> +	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
> +	if (ret) {
> +		spin_lock(&icu_lock);
> +		clear_bit(*irq_msg_num, icu_irq_alloc);
> +		spin_unlock(&icu_lock);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void
> +mvebu_icu_irq_parent_domain_free(struct irq_domain *domain,
> +				 unsigned int virq,
> +				 int irq_msg_num)
> +{
> +	irq_domain_free_irqs_parent(domain, virq, 1);
> +
> +	spin_lock(&icu_lock);
> +	clear_bit(irq_msg_num, icu_irq_alloc);
> +	spin_unlock(&icu_lock);
> +}
> +
> +static int
> +mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
> +			       unsigned long *hwirq, unsigned int *type)
> +{
> +	struct mvebu_icu *icu = d->host_data;
> +	unsigned int icu_group;
> +
> +	/* Check the count of the parameters in dt */
> +	if (WARN_ON(fwspec->param_count < 3)) {
> +		dev_err(icu->dev, "wrong ICU parameter count %d\n",
> +			fwspec->param_count);
> +		return -EINVAL;
> +	}
> +
> +	/* Only ICU group type is handled */
> +	icu_group = fwspec->param[0];
> +	if (icu_group != ICU_GRP_NSR && icu_group != ICU_GRP_SR &&
> +	    icu_group != ICU_GRP_SEI && icu_group != ICU_GRP_REI) {
> +		dev_err(icu->dev, "wrong ICU type %x\n", icu_group);
> +		return -EINVAL;
> +	}
> +
> +	*hwirq = fwspec->param[1];
> +	if (*hwirq < 0) {
> +		dev_err(icu->dev, "invalid interrupt number %ld\n", *hwirq);
> +		return -EINVAL;
> +	}
> +
> +	/* Mask the type to prevent wrong DT configuration */
> +	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +
> +	return 0;
> +}
> +
> +static int
> +mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +			   unsigned int nr_irqs, void *args)
> +{
> +	int err = 0, irq_msg_num = 0;
> +	unsigned long hwirq;
> +	unsigned int type = 0;
> +	unsigned int icu_group, icu_int;
> +	struct irq_fwspec *fwspec = args;
> +	struct mvebu_icu *icu = domain->host_data;
> +
> +	err = mvebu_icu_irq_domain_translate(domain, fwspec, &hwirq, &type);
> +	if (err) {
> +		dev_err(icu->dev, "failed to translate ICU parameters\n");
> +		return err;
> +	}
> +
> +	icu_group = fwspec->param[0];
> +
> +	err = mvebu_icu_irq_parent_domain_alloc(domain, virq, type,
> +						&irq_msg_num);
> +	if (err) {
> +		dev_err(icu->dev, "failed to allocate ICU interrupt in parent domain\n");
> +		return err;
> +	}
> +
> +	/* Configure the ICU with irq number & type */
> +	icu_int = irq_msg_num | ICU_INT_ENABLE;
> +	if (type & IRQ_TYPE_EDGE_RISING)
> +		icu_int |= ICU_IS_EDGE;
> +	icu_int |= icu_group << ICU_GROUP_SHIFT;
> +	writel(icu_int, icu->base + ICU_INT_CFG(hwirq));

You can use a writel_relaxed here.

> +
> +	/*
> +	 * The SATA unit has 2 ports, and a dedicated ICU entry per
> +	 * port. The ahci sata driver supports only one irq interrupt
> +	 * per SATA unit. To solve this conflict, we configure the 2
> +	 * SATA wired interrupts in the south bridge into 1 GIC
> +	 * interrupt in the north bridge. Even if only a single port
> +	 * is enabled, if sata node is enabled, both interrupts are
> +	 * configured (regardless of which port is actually in use).
> +	 * The ICU index of SATA0 = 107, SATA1 = 109

You can drop that last comment about the indexes, the #defines are good
enough.

> +	 */
> +	if (hwirq == ICU_SATA0_IRQ_INT || hwirq == ICU_SATA1_IRQ_INT) {
> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA0_IRQ_INT));
> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA1_IRQ_INT));
> +	}

Aren't you wasting an SPI here? If you configure SATA0 first, then
SATA1, SATA0's allocated SPI will be wasted (not to mention the
corresponding Linux interrupt too). Can't this be worked around at the
AHCI level? It is not like we don't have any quirk there already...

> +
> +	err = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +					    &icu->irq_chip, icu);
> +	if (err) {
> +		dev_err(icu->dev, "failed to set the data to IRQ domain\n");
> +		mvebu_icu_irq_parent_domain_free(domain, virq, irq_msg_num);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static void
> +mvebu_icu_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> +			  unsigned int nr_irqs)
> +{
> +	struct mvebu_icu *icu = domain->host_data;
> +	struct irq_data *irq = irq_get_irq_data(virq);
> +	struct irq_data *irq_parent = irq->parent_data;
> +	int irq_msg_num = mvebu_icu_gicspi_to_icuidx(irqd_to_hwirq(irq_parent));
> +
> +	WARN_ON(nr_irqs != 1);
> +
> +	writel(0, icu->base + ICU_INT_CFG(irqd_to_hwirq(irq)));

writel_relaxed (everywhere in this file).

> +
> +	mvebu_icu_irq_parent_domain_free(domain, virq, irq_msg_num);
> +}
> +
> +static const struct irq_domain_ops mvebu_icu_domain_ops = {
> +	.translate = mvebu_icu_irq_domain_translate,
> +	.alloc     = mvebu_icu_irq_domain_alloc,
> +	.free      = mvebu_icu_irq_domain_free,
> +};
> +
> +static int mvebu_icu_probe(struct platform_device *pdev)
> +{
> +	struct mvebu_icu *icu;
> +	struct irq_domain *parent_domain;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct platform_device *gicp_pdev;
> +	struct device_node *parent_irq_dn;
> +	struct device_node *gicp_dn;
> +	struct resource *res;
> +	struct resource *gicp_res;
> +	u32 i, icu_int;
> +	int ret;
> +
> +	icu = devm_kzalloc(&pdev->dev, sizeof(struct mvebu_icu),
> +			   GFP_KERNEL);
> +	if (!icu)
> +		return -ENOMEM;
> +
> +	icu->dev = &pdev->dev;
> +
> +	icu->irq_chip.name = kstrdup(dev_name(&pdev->dev), GFP_KERNEL);
> +	icu->irq_chip.irq_mask = irq_chip_mask_parent;
> +	icu->irq_chip.irq_unmask = irq_chip_unmask_parent;
> +	icu->irq_chip.irq_eoi = irq_chip_eoi_parent;
> +	icu->irq_chip.irq_set_type = irq_chip_set_type_parent;
> +#ifdef CONFIG_SMP
> +	icu->irq_chip.irq_set_affinity = irq_chip_set_affinity_parent;
> +#endif
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	icu->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(icu->base)) {
> +		dev_err(&pdev->dev, "Failed to map icu base address.\n");
> +		return PTR_ERR(icu->base);
> +	}

Careful here, you're leaking the memory from kstrdup above. Consider
using devm_kstrdup or/and moving all the potential failures (including
hooking on GICP) before doign any memory allocation.

> +
> +	gicp_dn = of_parse_phandle(node, "gicp", 0);
> +	if (!gicp_dn) {
> +		dev_err(&pdev->dev, "Missing gicp property.\n");
> +		return -ENODEV;
> +	}
> +
> +	gicp_pdev = of_find_device_by_node(gicp_dn);
> +	if (!gicp_pdev) {
> +		dev_err(&pdev->dev, "Cannot find gicp device.\n");
> +		return -ENODEV;
> +	}
> +
> +	get_device(&gicp_pdev->dev);
> +
> +	/*
> +	 * We really want the GICP to have cleared all interrupts
> +	 * potentially left pending by the firmware before probing the
> +	 * ICUs.
> +	 */
> +	if (!device_is_bound(&gicp_pdev->dev)) {
> +		ret = -EPROBE_DEFER;
> +		goto fail;
> +	}
> +
> +	gicp_res = platform_get_resource(gicp_pdev, IORESOURCE_MEM, 0);
> +	if (!gicp_res) {
> +		dev_err(&pdev->dev, "Failed to get gicp resource\n");
> +		ret = -ENODEV;
> +		goto fail;
> +	}
> +
> +	parent_irq_dn = of_irq_find_parent(node);
> +	if (!parent_irq_dn) {
> +		dev_err(&pdev->dev, "failed to find parent IRQ node\n");
> +		ret = -ENODEV;
> +		goto fail;
> +	}
> +
> +	parent_domain = irq_find_host(parent_irq_dn);
> +	if (!parent_domain) {
> +		dev_err(&pdev->dev, "Unable to locate ICU parent domain\n");
> +		ret = -ENODEV;
> +		goto fail;
> +	}
> +
> +	/* Set Clear/Set ICU SPI message address in AP */
> +	writel(upper_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
> +	       icu->base + ICU_SETSPI_NSR_AH);
> +	writel(lower_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
> +	       icu->base + ICU_SETSPI_NSR_AL);
> +	writel(upper_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
> +	       icu->base + ICU_CLRSPI_NSR_AH);
> +	writel(lower_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
> +	       icu->base + ICU_CLRSPI_NSR_AL);

I wonder if it wouldn't be better to have a proper function call into
the GICP code to retrieve this information. Or move the whole GICP
probing in here, because even if it is a separate piece of HW, it serves
no real purpose on its own.

> +
> +	/*
> +	 * Clean all ICU interrupts with type SPI_NSR, required to
> +	 * avoid unpredictable SPI assignments done by firmware.
> +	 */
> +	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {
> +		icu_int = readl(icu->base + ICU_INT_CFG(i));
> +		if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR)
> +			writel(0x0, icu->base + ICU_INT_CFG(i));

Erm... Does it mean that non-secure can write to the configuration of a
secure interrupt? If that's the case, that's pretty... interesting.

> +	}
> +
> +	icu->domain = irq_domain_add_hierarchy(parent_domain, 0,
> +					       ICU_MAX_SPI_IRQ_IN_GIC, node,
> +					       &mvebu_icu_domain_ops, icu);
> +	if (!icu->domain) {
> +		dev_err(&pdev->dev, "Failed to create ICU domain\n");
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	return 0;
> +fail:
> +	put_device(&gicp_pdev->dev);
> +	return ret;
> +}
> +
> +static const struct of_device_id mvebu_icu_of_match[] = {
> +	{ .compatible = "marvell,icu", },
> +	{},
> +};
> +
> +static struct platform_driver mvebu_icu_driver = {
> +	.probe  = mvebu_icu_probe,
> +	.driver = {
> +		.name = "mvebu-icu",
> +		.of_match_table = mvebu_icu_of_match,
> +	},
> +};
> +builtin_platform_driver(mvebu_icu_driver);
> diff --git a/include/dt-bindings/interrupt-controller/mvebu-icu.h b/include/dt-bindings/interrupt-controller/mvebu-icu.h
> new file mode 100644
> index 0000000..8249558
> --- /dev/null
> +++ b/include/dt-bindings/interrupt-controller/mvebu-icu.h
> @@ -0,0 +1,15 @@
> +/*
> + * This header provides constants for the MVEBU ICU driver.
> + */
> +
> +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H
> +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H
> +
> +/* interrupt specifier cell 0 */
> +
> +#define ICU_GRP_NSR		0x0
> +#define ICU_GRP_SR		0x1
> +#define ICU_GRP_SEI		0x4
> +#define ICU_GRP_REI		0x5
> +
> +#endif
> 

Thanks,

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

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Thomas Petazzoni]

Hello,

On Tue, 30 May 2017 12:10:29 +0100, Marc Zyngier wrote:

> Thanks for that, looks pretty interesting. A couple of comments below.

Thanks again for the review!

> > +/* GICP registers */
> > +#define GICP_SETSPI_NSR_OFFSET	0x0
> > +#define GICP_CLRSPI_NSR_OFFSET	0x8  
> 
> Shouldn't that live in some gicp-specific include file?

Would drivers/irqchip/irq-mvebu-gicp.h, included by both
irq-mvebu-gicp.c and irq-mvebu-icu.c be fine for you?

> > +/* ICU definitions */
> > +#define ICU_MAX_IRQS		207
> > +#define IRQS_PER_ICU		64
> > +#define ICU_MAX_SPI_IRQ_IN_GIC	(2 * IRQS_PER_ICU)  
> 
> What's the relationship between ICU_MAX_IRQS and
> IRQS_PER_ICU/ICU_MAX_SPI_IRQ_IN_GIC, if any? Is there only a single ICU?
> Or can you have multiple ones?

There is one ICU per CP. The Armada 7K SoC has one CP, the Armada 8K
SoC has two CPs. Therefore on Armada 8K, you have two ICUs, one per CP.

But I see your point: there is in fact no direct relation between the
number of GICP SPI interrupts reserved and the number of ICUs and
interrupts per ICU.

The number of GICP SPI interrupts (128) should be moved into
irq-mvebu-gicp.h, together with the GICP register offsets.

> > +#define ICU_GIC_SPI_BASE0	64
> > +#define ICU_GIC_SPI_BASE1	288  
> 
> My own gut feeling is that there will be another version of this IP one
> of these days, with different bases. Should we bite the bullet right
> away and put those in DT?

Where should these properties go? Under the gicp DT node, or under the
ICU DT node?

> > +static DEFINE_SPINLOCK(icu_lock);
> > +static DECLARE_BITMAP(icu_irq_alloc, ICU_MAX_SPI_IRQ_IN_GIC);
> > +
> > +static unsigned int
> > +mvebu_icu_icuidx_to_gicspi(unsigned int icuidx)
> > +{
> > +	if (icuidx < ICU_GIC_SPI_BASE0)
> > +		return ICU_GIC_SPI_BASE0 + icuidx;
> > +	else
> > +		return ICU_GIC_SPI_BASE1 + (icuidx - IRQS_PER_ICU);  
> 
> A small comment on how ICU indexes and GIC SPIs map would be good.

ACK.

> Correct me if I'm wrong, but it really looks like you're dealing with
> two devices here, each with their own SPI window.

We in fact don't really care about how many ICUs we have here. We have
128 GICP SPI interrupts available, in ranges:

 - ICU_GIC_SPI_BASE0 ; ICU_GIC_SPI_BASE0 + 64

 - ICU_GIC_SPI_BASE1 ; ICU_GIC_SPI_BASE1 + 64

The icu_irq_alloc bitmap is a global one, which allows to allocate one
GICP SPI interrupts amongst the available 128 interrupts, and this
function simply allows to map the index in this bitmap (from 0 to 127)
to the corresponding GICP SPI interrupt.

> > +	writel(icu_int, icu->base + ICU_INT_CFG(hwirq));  
> 
> You can use a writel_relaxed here.

ACK.

> > +	/*
> > +	 * The SATA unit has 2 ports, and a dedicated ICU entry per
> > +	 * port. The ahci sata driver supports only one irq interrupt
> > +	 * per SATA unit. To solve this conflict, we configure the 2
> > +	 * SATA wired interrupts in the south bridge into 1 GIC
> > +	 * interrupt in the north bridge. Even if only a single port
> > +	 * is enabled, if sata node is enabled, both interrupts are
> > +	 * configured (regardless of which port is actually in use).
> > +	 * The ICU index of SATA0 = 107, SATA1 = 109  
> 
> You can drop that last comment about the indexes, the #defines are good
> enough.

Agreed.

> > +	 */
> > +	if (hwirq == ICU_SATA0_IRQ_INT || hwirq == ICU_SATA1_IRQ_INT) {
> > +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA0_IRQ_INT));
> > +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA1_IRQ_INT));
> > +	}  
> 
> Aren't you wasting an SPI here?

No: we allocate a single SPI, icu_int. What we're doing here is that we
have two different wired interrupts in the CP that are "connected" to
the same GICP SPI interrupt.

> If you configure SATA0 first, then SATA1, SATA0's allocated SPI will
> be wasted (not to mention the corresponding Linux interrupt too).
> Can't this be worked around at the AHCI level? It is not like we
> don't have any quirk there already...

This is something I wanted to look at, but at a follow-up work, as I
believe the proposed work around is reasonable, and does not affect the
DT binding.

> > +	writel(0, icu->base + ICU_INT_CFG(irqd_to_hwirq(irq)));  
> 
> writel_relaxed (everywhere in this file).

ACK.


> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	icu->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(icu->base)) {
> > +		dev_err(&pdev->dev, "Failed to map icu base address.\n");
> > +		return PTR_ERR(icu->base);
> > +	}  
> 
> Careful here, you're leaking the memory from kstrdup above. Consider
> using devm_kstrdup or/and moving all the potential failures (including
> hooking on GICP) before doign any memory allocation.

Well spotted, will fix.


> > +	/* Set Clear/Set ICU SPI message address in AP */
> > +	writel(upper_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
> > +	       icu->base + ICU_SETSPI_NSR_AH);
> > +	writel(lower_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
> > +	       icu->base + ICU_SETSPI_NSR_AL);
> > +	writel(upper_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
> > +	       icu->base + ICU_CLRSPI_NSR_AH);
> > +	writel(lower_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
> > +	       icu->base + ICU_CLRSPI_NSR_AL);  
> 
> I wonder if it wouldn't be better to have a proper function call into
> the GICP code to retrieve this information.

I've never been a big fan of random cross function calls between
drivers, but this can be done.

> Or move the whole GICP probing in here, because even if it is a
> separate piece of HW, it serves no real purpose on its own.

So you suggest to merge the whole irq-mvebu-gicp.c stuff inside
irq-mvebu-icu.c ?

> > +	/*
> > +	 * Clean all ICU interrupts with type SPI_NSR, required to
> > +	 * avoid unpredictable SPI assignments done by firmware.
> > +	 */
> > +	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {
> > +		icu_int = readl(icu->base + ICU_INT_CFG(i));
> > +		if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR)
> > +			writel(0x0, icu->base + ICU_INT_CFG(i));  
> 
> Erm... Does it mean that non-secure can write to the configuration of
> a secure interrupt? If that's the case, that's pretty... interesting.

I'll let Hannah and Yehuda answer this question, they know the ICU
details much better than I do.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Marc Zyngier]

On 30/05/17 13:05, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 30 May 2017 12:10:29 +0100, Marc Zyngier wrote:
> 
>> Thanks for that, looks pretty interesting. A couple of comments below.
> 
> Thanks again for the review!
> 
>>> +/* GICP registers */
>>> +#define GICP_SETSPI_NSR_OFFSET	0x0
>>> +#define GICP_CLRSPI_NSR_OFFSET	0x8  
>>
>> Shouldn't that live in some gicp-specific include file?
> 
> Would drivers/irqchip/irq-mvebu-gicp.h, included by both
> irq-mvebu-gicp.c and irq-mvebu-icu.c be fine for you?

Sure, that'd be fine, assuming that it is necessary (see below).

> 
>>> +/* ICU definitions */
>>> +#define ICU_MAX_IRQS		207
>>> +#define IRQS_PER_ICU		64
>>> +#define ICU_MAX_SPI_IRQ_IN_GIC	(2 * IRQS_PER_ICU)  
>>
>> What's the relationship between ICU_MAX_IRQS and
>> IRQS_PER_ICU/ICU_MAX_SPI_IRQ_IN_GIC, if any? Is there only a single ICU?
>> Or can you have multiple ones?
> 
> There is one ICU per CP. The Armada 7K SoC has one CP, the Armada 8K
> SoC has two CPs. Therefore on Armada 8K, you have two ICUs, one per CP.

OK. Is there any restriction on which SPI an ICU can generate?

> But I see your point: there is in fact no direct relation between the
> number of GICP SPI interrupts reserved and the number of ICUs and
> interrupts per ICU.

Indeed. And maybe we should have an instance of the ICU device per CP.

> The number of GICP SPI interrupts (128) should be moved into
> irq-mvebu-gicp.h, together with the GICP register offsets.
> 
>>> +#define ICU_GIC_SPI_BASE0	64
>>> +#define ICU_GIC_SPI_BASE1	288  
>>
>> My own gut feeling is that there will be another version of this IP one
>> of these days, with different bases. Should we bite the bullet right
>> away and put those in DT?
> 
> Where should these properties go? Under the gicp DT node, or under the
> ICU DT node?

If the ICU has no knowledge of the SPI it can generate, I'd rather put
that in the GICP node.

> 
>>> +static DEFINE_SPINLOCK(icu_lock);
>>> +static DECLARE_BITMAP(icu_irq_alloc, ICU_MAX_SPI_IRQ_IN_GIC);
>>> +
>>> +static unsigned int
>>> +mvebu_icu_icuidx_to_gicspi(unsigned int icuidx)
>>> +{
>>> +	if (icuidx < ICU_GIC_SPI_BASE0)
>>> +		return ICU_GIC_SPI_BASE0 + icuidx;
>>> +	else
>>> +		return ICU_GIC_SPI_BASE1 + (icuidx - IRQS_PER_ICU);  
>>
>> A small comment on how ICU indexes and GIC SPIs map would be good.
> 
> ACK.
> 
>> Correct me if I'm wrong, but it really looks like you're dealing with
>> two devices here, each with their own SPI window.
> 
> We in fact don't really care about how many ICUs we have here. We have
> 128 GICP SPI interrupts available, in ranges:
> 
>  - ICU_GIC_SPI_BASE0 ; ICU_GIC_SPI_BASE0 + 64
> 
>  - ICU_GIC_SPI_BASE1 ; ICU_GIC_SPI_BASE1 + 64
> 
> The icu_irq_alloc bitmap is a global one, which allows to allocate one
> GICP SPI interrupts amongst the available 128 interrupts, and this
> function simply allows to map the index in this bitmap (from 0 to 127)
> to the corresponding GICP SPI interrupt.

That makes a lot more sense now, thanks.

> 
>>> +	writel(icu_int, icu->base + ICU_INT_CFG(hwirq));  
>>
>> You can use a writel_relaxed here.
> 
> ACK.
> 
>>> +	/*
>>> +	 * The SATA unit has 2 ports, and a dedicated ICU entry per
>>> +	 * port. The ahci sata driver supports only one irq interrupt
>>> +	 * per SATA unit. To solve this conflict, we configure the 2
>>> +	 * SATA wired interrupts in the south bridge into 1 GIC
>>> +	 * interrupt in the north bridge. Even if only a single port
>>> +	 * is enabled, if sata node is enabled, both interrupts are
>>> +	 * configured (regardless of which port is actually in use).
>>> +	 * The ICU index of SATA0 = 107, SATA1 = 109  
>>
>> You can drop that last comment about the indexes, the #defines are good
>> enough.
> 
> Agreed.
> 
>>> +	 */
>>> +	if (hwirq == ICU_SATA0_IRQ_INT || hwirq == ICU_SATA1_IRQ_INT) {
>>> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA0_IRQ_INT));
>>> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA1_IRQ_INT));
>>> +	}  
>>
>> Aren't you wasting an SPI here?
> 
> No: we allocate a single SPI, icu_int. What we're doing here is that we
> have two different wired interrupts in the CP that are "connected" to
> the same GICP SPI interrupt.

But if both ports are enabled, you're going to allocate one SPI per call
to this function, and the last one wins (you never "remember" that you
have configured one port already, and always allocate a new interrupt).

>> If you configure SATA0 first, then SATA1, SATA0's allocated SPI will
>> be wasted (not to mention the corresponding Linux interrupt too).
>> Can't this be worked around at the AHCI level? It is not like we
>> don't have any quirk there already...
> 
> This is something I wanted to look at, but at a follow-up work, as I
> believe the proposed work around is reasonable, and does not affect the
> DT binding.

Sure.

>>> +	writel(0, icu->base + ICU_INT_CFG(irqd_to_hwirq(irq)));  
>>
>> writel_relaxed (everywhere in this file).
> 
> ACK.
> 
> 
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	icu->base = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(icu->base)) {
>>> +		dev_err(&pdev->dev, "Failed to map icu base address.\n");
>>> +		return PTR_ERR(icu->base);
>>> +	}  
>>
>> Careful here, you're leaking the memory from kstrdup above. Consider
>> using devm_kstrdup or/and moving all the potential failures (including
>> hooking on GICP) before doign any memory allocation.
> 
> Well spotted, will fix.
> 
> 
>>> +	/* Set Clear/Set ICU SPI message address in AP */
>>> +	writel(upper_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
>>> +	       icu->base + ICU_SETSPI_NSR_AH);
>>> +	writel(lower_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
>>> +	       icu->base + ICU_SETSPI_NSR_AL);
>>> +	writel(upper_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
>>> +	       icu->base + ICU_CLRSPI_NSR_AH);
>>> +	writel(lower_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
>>> +	       icu->base + ICU_CLRSPI_NSR_AL);  
>>
>> I wonder if it wouldn't be better to have a proper function call into
>> the GICP code to retrieve this information.
> 
> I've never been a big fan of random cross function calls between
> drivers, but this can be done.
> 
>> Or move the whole GICP probing in here, because even if it is a
>> separate piece of HW, it serves no real purpose on its own.
> 
> So you suggest to merge the whole irq-mvebu-gicp.c stuff inside
> irq-mvebu-icu.c ?

Given how small that code is, it wouldn't be a big deal.

>>> +	/*
>>> +	 * Clean all ICU interrupts with type SPI_NSR, required to
>>> +	 * avoid unpredictable SPI assignments done by firmware.
>>> +	 */
>>> +	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {
>>> +		icu_int = readl(icu->base + ICU_INT_CFG(i));
>>> +		if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR)
>>> +			writel(0x0, icu->base + ICU_INT_CFG(i));  
>>
>> Erm... Does it mean that non-secure can write to the configuration of
>> a secure interrupt? If that's the case, that's pretty... interesting.
> 
> I'll let Hannah and Yehuda answer this question, they know the ICU
> details much better than I do.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Thomas Petazzoni]

Hello,

On Tue, 30 May 2017 14:06:52 +0100, Marc Zyngier wrote:

> > Would drivers/irqchip/irq-mvebu-gicp.h, included by both
> > irq-mvebu-gicp.c and irq-mvebu-icu.c be fine for you?  
> 
> Sure, that'd be fine, assuming that it is necessary (see below).

Right, if we merge everything into one file, then it's simpler :)

> >> What's the relationship between ICU_MAX_IRQS and
> >> IRQS_PER_ICU/ICU_MAX_SPI_IRQ_IN_GIC, if any? Is there only a single ICU?
> >> Or can you have multiple ones?  
> > 
> > There is one ICU per CP. The Armada 7K SoC has one CP, the Armada 8K
> > SoC has two CPs. Therefore on Armada 8K, you have two ICUs, one per CP.  
> 
> OK. Is there any restriction on which SPI an ICU can generate?

Not that I'm aware of. Even though the fact that there are two ranges
of SPI interrupts, which might make one think there is one range per
ICU, this is not the case: any ICU can trigger any SPI within those
ranges. Which is why the ICU driver handles the 128 available SPIs
through a single global bitmap rather than a per-ICU bitmap.

> > But I see your point: there is in fact no direct relation between the
> > number of GICP SPI interrupts reserved and the number of ICUs and
> > interrupts per ICU.  
> 
> Indeed. And maybe we should have an instance of the ICU device per CP.

Not sure what you mean here: we already have one instance of the ICU
device per CP.

armada-cp110-master.dtsi describes the ICU in the master CP,
armada-cp110-slave.dtsi describes the ICU in the slave CP. So in the
patch series I have posted, on an Armada 8K that has two CPs, the
->probe() of irq-mvebu-icu.c gets called twice, once per ICU, and we
have one instance of the ICU per CP, as expected.

Am I missing something?

> >>> +#define ICU_GIC_SPI_BASE0	64
> >>> +#define ICU_GIC_SPI_BASE1	288    
> >>
> >> My own gut feeling is that there will be another version of this IP one
> >> of these days, with different bases. Should we bite the bullet right
> >> away and put those in DT?  
> > 
> > Where should these properties go? Under the gicp DT node, or under the
> > ICU DT node?  
> 
> If the ICU has no knowledge of the SPI it can generate, I'd rather put
> that in the GICP node.

Something like:

	marvell,spi-ranges = <64 64>, <288 64>;

And then the ICU ->probe() routine walks the marvell,gicp phandle, find
the gicp node and parses this information?

> > We in fact don't really care about how many ICUs we have here. We have
> > 128 GICP SPI interrupts available, in ranges:
> > 
> >  - ICU_GIC_SPI_BASE0 ; ICU_GIC_SPI_BASE0 + 64
> > 
> >  - ICU_GIC_SPI_BASE1 ; ICU_GIC_SPI_BASE1 + 64
> > 
> > The icu_irq_alloc bitmap is a global one, which allows to allocate one
> > GICP SPI interrupts amongst the available 128 interrupts, and this
> > function simply allows to map the index in this bitmap (from 0 to 127)
> > to the corresponding GICP SPI interrupt.  
> 
> That makes a lot more sense now, thanks.

I should probably add a comment explaining this in the driver.

> >>> +	 */
> >>> +	if (hwirq == ICU_SATA0_IRQ_INT || hwirq == ICU_SATA1_IRQ_INT) {
> >>> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA0_IRQ_INT));
> >>> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA1_IRQ_INT));
> >>> +	}    
> >>
> >> Aren't you wasting an SPI here?  
> > 
> > No: we allocate a single SPI, icu_int. What we're doing here is that we
> > have two different wired interrupts in the CP that are "connected" to
> > the same GICP SPI interrupt.  
> 
> But if both ports are enabled, you're going to allocate one SPI per call
> to this function, and the last one wins (you never "remember" that you
> have configured one port already, and always allocate a new interrupt).

Yes, but no, because the DT only declares one of the two interrupts
currently:

 			cpm_sata0: sata@540000 {
 				compatible = "marvell,armada-8k-ahci",
 					     "generic-ahci";
 				reg = <0x540000 0x30000>;
-				interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 107 IRQ_TYPE_LEVEL_HIGH>;

Yes, needs to be fixed, with proper changes to the AHCI driver, but
that's a separate matter.

> > So you suggest to merge the whole irq-mvebu-gicp.c stuff inside
> > irq-mvebu-icu.c ?  
> 
> Given how small that code is, it wouldn't be a big deal.

OK.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Marc Zyngier]

On 30/05/17 14:17, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 30 May 2017 14:06:52 +0100, Marc Zyngier wrote:
> 
>>> Would drivers/irqchip/irq-mvebu-gicp.h, included by both
>>> irq-mvebu-gicp.c and irq-mvebu-icu.c be fine for you?  
>>
>> Sure, that'd be fine, assuming that it is necessary (see below).
> 
> Right, if we merge everything into one file, then it's simpler :)
> 
>>>> What's the relationship between ICU_MAX_IRQS and
>>>> IRQS_PER_ICU/ICU_MAX_SPI_IRQ_IN_GIC, if any? Is there only a single ICU?
>>>> Or can you have multiple ones?  
>>>
>>> There is one ICU per CP. The Armada 7K SoC has one CP, the Armada 8K
>>> SoC has two CPs. Therefore on Armada 8K, you have two ICUs, one per CP.  
>>
>> OK. Is there any restriction on which SPI an ICU can generate?
> 
> Not that I'm aware of. Even though the fact that there are two ranges
> of SPI interrupts, which might make one think there is one range per
> ICU, this is not the case: any ICU can trigger any SPI within those
> ranges. Which is why the ICU driver handles the 128 available SPIs
> through a single global bitmap rather than a per-ICU bitmap.

OK.

> 
>>> But I see your point: there is in fact no direct relation between the
>>> number of GICP SPI interrupts reserved and the number of ICUs and
>>> interrupts per ICU.  
>>
>> Indeed. And maybe we should have an instance of the ICU device per CP.
> 
> Not sure what you mean here: we already have one instance of the ICU
> device per CP.
> 
> armada-cp110-master.dtsi describes the ICU in the master CP,
> armada-cp110-slave.dtsi describes the ICU in the slave CP. So in the
> patch series I have posted, on an Armada 8K that has two CPs, the
> ->probe() of irq-mvebu-icu.c gets called twice, once per ICU, and we
> have one instance of the ICU per CP, as expected.
> 
> Am I missing something?

No, I'm just being remarkably thick today. Sorry about the noise.

> 
>>>>> +#define ICU_GIC_SPI_BASE0	64
>>>>> +#define ICU_GIC_SPI_BASE1	288    
>>>>
>>>> My own gut feeling is that there will be another version of this IP one
>>>> of these days, with different bases. Should we bite the bullet right
>>>> away and put those in DT?  
>>>
>>> Where should these properties go? Under the gicp DT node, or under the
>>> ICU DT node?  
>>
>> If the ICU has no knowledge of the SPI it can generate, I'd rather put
>> that in the GICP node.
> 
> Something like:
> 
> 	marvell,spi-ranges = <64 64>, <288 64>;
> 
> And then the ICU ->probe() routine walks the marvell,gicp phandle, find
> the gicp node and parses this information?

Either that, or you keep a separate GICP probing. Up to you, really.

> 
>>> We in fact don't really care about how many ICUs we have here. We have
>>> 128 GICP SPI interrupts available, in ranges:
>>>
>>>  - ICU_GIC_SPI_BASE0 ; ICU_GIC_SPI_BASE0 + 64
>>>
>>>  - ICU_GIC_SPI_BASE1 ; ICU_GIC_SPI_BASE1 + 64
>>>
>>> The icu_irq_alloc bitmap is a global one, which allows to allocate one
>>> GICP SPI interrupts amongst the available 128 interrupts, and this
>>> function simply allows to map the index in this bitmap (from 0 to 127)
>>> to the corresponding GICP SPI interrupt.  
>>
>> That makes a lot more sense now, thanks.
> 
> I should probably add a comment explaining this in the driver.

Yeah, that'd help.

>>>>> +	 */
>>>>> +	if (hwirq == ICU_SATA0_IRQ_INT || hwirq == ICU_SATA1_IRQ_INT) {
>>>>> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA0_IRQ_INT));
>>>>> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA1_IRQ_INT));
>>>>> +	}    
>>>>
>>>> Aren't you wasting an SPI here?  
>>>
>>> No: we allocate a single SPI, icu_int. What we're doing here is that we
>>> have two different wired interrupts in the CP that are "connected" to
>>> the same GICP SPI interrupt.  
>>
>> But if both ports are enabled, you're going to allocate one SPI per call
>> to this function, and the last one wins (you never "remember" that you
>> have configured one port already, and always allocate a new interrupt).
> 
> Yes, but no, because the DT only declares one of the two interrupts
> currently:
> 
>  			cpm_sata0: sata@540000 {
>  				compatible = "marvell,armada-8k-ahci",
>  					     "generic-ahci";
>  				reg = <0x540000 0x30000>;
> -				interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> +				interrupts = <ICU_GRP_NSR 107 IRQ_TYPE_LEVEL_HIGH>;
> 
> Yes, needs to be fixed, with proper changes to the AHCI driver, but
> that's a separate matter.

OK. Can you expose both interrupts in the DT already, assuming this
doesn't break anything? Wasting an SPI is not that big a deal, and I
want to make sure we'll have a smooth upgrade path when transitioning
from the irqchip hack to the ahci solution.

Thanks,

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

RE: [EXT] Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Yehuda Yitschak]

> -----Original Message-----
> From: Thomas Petazzoni [mailto:thomas.petazzoni@free-electrons.com]
> Sent: Tuesday, May 30, 2017 15:06
> To: Marc Zyngier
> Cc: Thomas Gleixner; Jason Cooper; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Rob Herring; Ian Campbell; Pawel Moll; Mark
> Rutland; Kumar Gala; Andrew Lunn; Sebastian Hesselbarth; Gregory
> Clement; linux-arm-kernel@lists.infradead.org; Nadav Haklai; Hanna Hawa;
> Yehuda Yitschak; Antoine Tenart
> Subject: [EXT] Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell
> ICU
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hello,
> 
> On Tue, 30 May 2017 12:10:29 +0100, Marc Zyngier wrote:
> 
> > Thanks for that, looks pretty interesting. A couple of comments below.
> 
> Thanks again for the review!
> 
> > > +/* GICP registers */
> > > +#define GICP_SETSPI_NSR_OFFSET	0x0
> > > +#define GICP_CLRSPI_NSR_OFFSET	0x8
> >
> > Shouldn't that live in some gicp-specific include file?
> 
> Would drivers/irqchip/irq-mvebu-gicp.h, included by both irq-mvebu-gicp.c
> and irq-mvebu-icu.c be fine for you?
> 
> > > +/* ICU definitions */
> > > +#define ICU_MAX_IRQS		207
> > > +#define IRQS_PER_ICU		64
> > > +#define ICU_MAX_SPI_IRQ_IN_GIC	(2 * IRQS_PER_ICU)
> >
> > What's the relationship between ICU_MAX_IRQS and
> > IRQS_PER_ICU/ICU_MAX_SPI_IRQ_IN_GIC, if any? Is there only a single
> ICU?
> > Or can you have multiple ones?
> 
> There is one ICU per CP. The Armada 7K SoC has one CP, the Armada 8K SoC
> has two CPs. Therefore on Armada 8K, you have two ICUs, one per CP.
> 
> But I see your point: there is in fact no direct relation between the number of
> GICP SPI interrupts reserved and the number of ICUs and interrupts per ICU.
> 
> The number of GICP SPI interrupts (128) should be moved into irq-mvebu-
> gicp.h, together with the GICP register offsets.
> 
> > > +#define ICU_GIC_SPI_BASE0	64
> > > +#define ICU_GIC_SPI_BASE1	288
> >
> > My own gut feeling is that there will be another version of this IP
> > one of these days, with different bases. Should we bite the bullet
> > right away and put those in DT?
> 
> Where should these properties go? Under the gicp DT node, or under the
> ICU DT node?
> 
> > > +static DEFINE_SPINLOCK(icu_lock);
> > > +static DECLARE_BITMAP(icu_irq_alloc, ICU_MAX_SPI_IRQ_IN_GIC);
> > > +
> > > +static unsigned int
> > > +mvebu_icu_icuidx_to_gicspi(unsigned int icuidx) {
> > > +	if (icuidx < ICU_GIC_SPI_BASE0)
> > > +		return ICU_GIC_SPI_BASE0 + icuidx;
> > > +	else
> > > +		return ICU_GIC_SPI_BASE1 + (icuidx - IRQS_PER_ICU);
> >
> > A small comment on how ICU indexes and GIC SPIs map would be good.
> 
> ACK.
> 
> > Correct me if I'm wrong, but it really looks like you're dealing with
> > two devices here, each with their own SPI window.
> 
> We in fact don't really care about how many ICUs we have here. We have
> 128 GICP SPI interrupts available, in ranges:
> 
>  - ICU_GIC_SPI_BASE0 ; ICU_GIC_SPI_BASE0 + 64
> 
>  - ICU_GIC_SPI_BASE1 ; ICU_GIC_SPI_BASE1 + 64
> 
> The icu_irq_alloc bitmap is a global one, which allows to allocate one GICP SPI
> interrupts amongst the available 128 interrupts, and this function simply
> allows to map the index in this bitmap (from 0 to 127) to the corresponding
> GICP SPI interrupt.
> 
> > > +	writel(icu_int, icu->base + ICU_INT_CFG(hwirq));
> >
> > You can use a writel_relaxed here.
> 
> ACK.
> 
> > > +	/*
> > > +	 * The SATA unit has 2 ports, and a dedicated ICU entry per
> > > +	 * port. The ahci sata driver supports only one irq interrupt
> > > +	 * per SATA unit. To solve this conflict, we configure the 2
> > > +	 * SATA wired interrupts in the south bridge into 1 GIC
> > > +	 * interrupt in the north bridge. Even if only a single port
> > > +	 * is enabled, if sata node is enabled, both interrupts are
> > > +	 * configured (regardless of which port is actually in use).
> > > +	 * The ICU index of SATA0 = 107, SATA1 = 109
> >
> > You can drop that last comment about the indexes, the #defines are
> > good enough.
> 
> Agreed.
> 
> > > +	 */
> > > +	if (hwirq == ICU_SATA0_IRQ_INT || hwirq == ICU_SATA1_IRQ_INT) {
> > > +		writel(icu_int, icu->base +
> ICU_INT_CFG(ICU_SATA0_IRQ_INT));
> > > +		writel(icu_int, icu->base +
> ICU_INT_CFG(ICU_SATA1_IRQ_INT));
> > > +	}
> >
> > Aren't you wasting an SPI here?
> 
> No: we allocate a single SPI, icu_int. What we're doing here is that we have
> two different wired interrupts in the CP that are "connected" to the same
> GICP SPI interrupt.
> 
> > If you configure SATA0 first, then SATA1, SATA0's allocated SPI will
> > be wasted (not to mention the corresponding Linux interrupt too).
> > Can't this be worked around at the AHCI level? It is not like we don't
> > have any quirk there already...
> 
> This is something I wanted to look at, but at a follow-up work, as I believe the
> proposed work around is reasonable, and does not affect the DT binding.
> 
> > > +	writel(0, icu->base + ICU_INT_CFG(irqd_to_hwirq(irq)));
> >
> > writel_relaxed (everywhere in this file).
> 
> ACK.
> 
> 
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	icu->base = devm_ioremap_resource(&pdev->dev, res);
> > > +	if (IS_ERR(icu->base)) {
> > > +		dev_err(&pdev->dev, "Failed to map icu base address.\n");
> > > +		return PTR_ERR(icu->base);
> > > +	}
> >
> > Careful here, you're leaking the memory from kstrdup above. Consider
> > using devm_kstrdup or/and moving all the potential failures (including
> > hooking on GICP) before doign any memory allocation.
> 
> Well spotted, will fix.
> 
> 
> > > +	/* Set Clear/Set ICU SPI message address in AP */
> > > +	writel(upper_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
> > > +	       icu->base + ICU_SETSPI_NSR_AH);
> > > +	writel(lower_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
> > > +	       icu->base + ICU_SETSPI_NSR_AL);
> > > +	writel(upper_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
> > > +	       icu->base + ICU_CLRSPI_NSR_AH);
> > > +	writel(lower_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
> > > +	       icu->base + ICU_CLRSPI_NSR_AL);
> >
> > I wonder if it wouldn't be better to have a proper function call into
> > the GICP code to retrieve this information.
> 
> I've never been a big fan of random cross function calls between drivers, but
> this can be done.
> 
> > Or move the whole GICP probing in here, because even if it is a
> > separate piece of HW, it serves no real purpose on its own.
> 
> So you suggest to merge the whole irq-mvebu-gicp.c stuff inside irq-mvebu-
> icu.c ?
> 
> > > +	/*
> > > +	 * Clean all ICU interrupts with type SPI_NSR, required to
> > > +	 * avoid unpredictable SPI assignments done by firmware.
> > > +	 */
> > > +	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {
> > > +		icu_int = readl(icu->base + ICU_INT_CFG(i));
> > > +		if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR)
> > > +			writel(0x0, icu->base + ICU_INT_CFG(i));
> >
> > Erm... Does it mean that non-secure can write to the configuration of
> > a secure interrupt? If that's the case, that's pretty... interesting.
> 
> I'll let Hannah and Yehuda answer this question, they know the ICU details
> much better than I do.

In case it's still relevant. The ICU has a limitation where the entire unit can be either secured or non-secured.
If the entire ICU is secured, It's practically useless in Linux. In the non-secure setup, Linux can technically modify configurations made by secure FW.
This means the secure FW can't reliably use the ICU for secure interrupts.
This limitation should be fixed in future revisions to allow secure FW to use the ICU for generating secure interrupts.

Regards

Yehuda 

> 
> Thanks!
> 
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Antoine Tenart]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

Hi Thomas,

Very small comment,

On Tue, May 30, 2017 at 11:16:09AM +0200, Thomas Petazzoni wrote:
> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
[...]
> +#define ICU_INT_CFG(x)          (0x100 + 4 * x)

You should use (x) here, to be safe.

Thanks!
Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Russell King - ARM Linux]

I see we're still duplicating work.

Yes, I know you'll reply with your "your tree is a private tree" blah
blah, which is fine if you want to keep re-doing work that others have
done, but it's incredibly inefficient.

What you call my private tree is the public ARM git tree - hardly
private.  It's the one which core ARM changes go through.  The mcbin
changes are in a separate branch of that tree - hardly private by
any definition of that term.  It's as private as your own
free-electrons git tree.

Given that during the previous rounds I was willing to work with you,
testing your patches and helping the effort along, I find your attitude
towards me to be very counter-productive - you seem to have a very big
NIH problem.

How about working _together_ sharing the effort, rather than competing?

I have a 31 patch series in my tree cleaning up the Marvell ICU support
and adding PM support to the driver - obviously too large to post as-is
for mainline, but intended for the changes to be reviewable.  Obviously
that was now a total waste of my time.

I don't see why I should get involved in the future with Armada 8k, it
seems whatever effort I put in is wasted.

On Tue, May 30, 2017 at 11:16:09AM +0200, Thomas Petazzoni wrote:
> The Marvell ICU unit is found in the CP110 block of the Marvell Armada
> 7K and 8K SoCs. It collects the wired interrupts of the devices located
> in the CP110 and turns them into SPI interrupts in the GIC located in
> the AP806 side of the SoC, by using a memory transaction.
> 
> Until now, the ICU was configured in a static fashion by the firmware,
> and Linux was relying on this static configuration. By having Linux
> configure the ICU, we are more flexible, and we can allocate dynamically
> the GIC SPI interrupts only for devices that are actually in use.
> 
> The driver was initially written by Hanna Hawa <hannah-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/irqchip/Kconfig                            |   3 +
>  drivers/irqchip/Makefile                           |   1 +
>  drivers/irqchip/irq-mvebu-icu.c                    | 373 +++++++++++++++++++++
>  .../dt-bindings/interrupt-controller/mvebu-icu.h   |  15 +
>  4 files changed, 392 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mvebu-icu.c
>  create mode 100644 include/dt-bindings/interrupt-controller/mvebu-icu.h
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index e527ee5..676232a 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -271,6 +271,9 @@ config IRQ_MXS
>  config MVEBU_GICP
>  	bool
>  
> +config MVEBU_ICU
> +	bool
> +
>  config MVEBU_ODMI
>  	bool
>  	select GENERIC_MSI_IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 11eb858..18fa5fa 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
>  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
>  obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
>  obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.o
> +obj-$(CONFIG_MVEBU_ICU)			+= irq-mvebu-icu.o
>  obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
>  obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
> new file mode 100644
> index 0000000..8a5c217
> --- /dev/null
> +++ b/drivers/irqchip/irq-mvebu-icu.c
> @@ -0,0 +1,373 @@
> +/*
> + * Copyright (C) 2017 Marvell
> + *
> + * Hanna Hawa <hannah-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> + * Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/mvebu-icu.h>
> +
> +/* ICU registers */
> +#define ICU_SETSPI_NSR_AL	0x10
> +#define ICU_SETSPI_NSR_AH	0x14
> +#define ICU_CLRSPI_NSR_AL	0x18
> +#define ICU_CLRSPI_NSR_AH	0x1c
> +#define ICU_INT_CFG(x)          (0x100 + 4 * x)
> +#define   ICU_INT_ENABLE	BIT(24)
> +#define   ICU_IS_EDGE		BIT(28)
> +#define   ICU_GROUP_SHIFT	29
> +
> +/* GICP registers */
> +#define GICP_SETSPI_NSR_OFFSET	0x0
> +#define GICP_CLRSPI_NSR_OFFSET	0x8
> +
> +/* ICU definitions */
> +#define ICU_MAX_IRQS		207
> +#define IRQS_PER_ICU		64
> +#define ICU_MAX_SPI_IRQ_IN_GIC	(2 * IRQS_PER_ICU)
> +#define ICU_GIC_SPI_BASE0	64
> +#define ICU_GIC_SPI_BASE1	288
> +
> +#define ICU_SATA0_IRQ_INT	109
> +#define ICU_SATA1_IRQ_INT	107
> +
> +struct mvebu_icu {
> +	struct irq_chip irq_chip;
> +	void __iomem *base;
> +	struct irq_domain *domain;
> +	struct device *dev;
> +};
> +
> +static DEFINE_SPINLOCK(icu_lock);
> +static DECLARE_BITMAP(icu_irq_alloc, ICU_MAX_SPI_IRQ_IN_GIC);
> +
> +static unsigned int
> +mvebu_icu_icuidx_to_gicspi(unsigned int icuidx)
> +{
> +	if (icuidx < ICU_GIC_SPI_BASE0)
> +		return ICU_GIC_SPI_BASE0 + icuidx;
> +	else
> +		return ICU_GIC_SPI_BASE1 + (icuidx - IRQS_PER_ICU);
> +}
> +
> +static unsigned int
> +mvebu_icu_gicspi_to_icuidx(unsigned int gicspi)
> +{
> +	if (gicspi < ICU_GIC_SPI_BASE1)
> +		return gicspi - ICU_GIC_SPI_BASE0;
> +	else
> +		return gicspi - ICU_GIC_SPI_BASE1 + IRQS_PER_ICU;
> +}
> +
> +static int
> +mvebu_icu_irq_parent_domain_alloc(struct irq_domain *domain,
> +				  unsigned int virq, unsigned int type,
> +				  int *irq_msg_num)
> +{
> +	struct mvebu_icu *icu = domain->host_data;
> +	struct irq_fwspec fwspec;
> +	int ret;
> +
> +	/* Find first free interrupt in ICU pool */
> +	spin_lock(&icu_lock);
> +	*irq_msg_num = find_first_zero_bit(icu_irq_alloc,
> +					   ICU_MAX_SPI_IRQ_IN_GIC);
> +	if (*irq_msg_num == ICU_MAX_SPI_IRQ_IN_GIC) {
> +		dev_err(icu->dev, "No free ICU interrupt found\n");
> +		spin_unlock(&icu_lock);
> +		return -ENOSPC;
> +	}
> +	set_bit(*irq_msg_num, icu_irq_alloc);
> +	spin_unlock(&icu_lock);
> +
> +	fwspec.fwnode = domain->parent->fwnode;
> +	fwspec.param_count = 3;
> +	fwspec.param[0] = GIC_SPI;
> +	fwspec.param[1] = mvebu_icu_icuidx_to_gicspi(*irq_msg_num) - 32;
> +	fwspec.param[2] = type;
> +
> +	/* Allocate the IRQ in the parent */
> +	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
> +	if (ret) {
> +		spin_lock(&icu_lock);
> +		clear_bit(*irq_msg_num, icu_irq_alloc);
> +		spin_unlock(&icu_lock);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void
> +mvebu_icu_irq_parent_domain_free(struct irq_domain *domain,
> +				 unsigned int virq,
> +				 int irq_msg_num)
> +{
> +	irq_domain_free_irqs_parent(domain, virq, 1);
> +
> +	spin_lock(&icu_lock);
> +	clear_bit(irq_msg_num, icu_irq_alloc);
> +	spin_unlock(&icu_lock);
> +}
> +
> +static int
> +mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
> +			       unsigned long *hwirq, unsigned int *type)
> +{
> +	struct mvebu_icu *icu = d->host_data;
> +	unsigned int icu_group;
> +
> +	/* Check the count of the parameters in dt */
> +	if (WARN_ON(fwspec->param_count < 3)) {
> +		dev_err(icu->dev, "wrong ICU parameter count %d\n",
> +			fwspec->param_count);
> +		return -EINVAL;
> +	}
> +
> +	/* Only ICU group type is handled */
> +	icu_group = fwspec->param[0];
> +	if (icu_group != ICU_GRP_NSR && icu_group != ICU_GRP_SR &&
> +	    icu_group != ICU_GRP_SEI && icu_group != ICU_GRP_REI) {
> +		dev_err(icu->dev, "wrong ICU type %x\n", icu_group);
> +		return -EINVAL;
> +	}
> +
> +	*hwirq = fwspec->param[1];
> +	if (*hwirq < 0) {
> +		dev_err(icu->dev, "invalid interrupt number %ld\n", *hwirq);
> +		return -EINVAL;
> +	}
> +
> +	/* Mask the type to prevent wrong DT configuration */
> +	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +
> +	return 0;
> +}
> +
> +static int
> +mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +			   unsigned int nr_irqs, void *args)
> +{
> +	int err = 0, irq_msg_num = 0;
> +	unsigned long hwirq;
> +	unsigned int type = 0;
> +	unsigned int icu_group, icu_int;
> +	struct irq_fwspec *fwspec = args;
> +	struct mvebu_icu *icu = domain->host_data;
> +
> +	err = mvebu_icu_irq_domain_translate(domain, fwspec, &hwirq, &type);
> +	if (err) {
> +		dev_err(icu->dev, "failed to translate ICU parameters\n");
> +		return err;
> +	}
> +
> +	icu_group = fwspec->param[0];
> +
> +	err = mvebu_icu_irq_parent_domain_alloc(domain, virq, type,
> +						&irq_msg_num);
> +	if (err) {
> +		dev_err(icu->dev, "failed to allocate ICU interrupt in parent domain\n");
> +		return err;
> +	}
> +
> +	/* Configure the ICU with irq number & type */
> +	icu_int = irq_msg_num | ICU_INT_ENABLE;
> +	if (type & IRQ_TYPE_EDGE_RISING)
> +		icu_int |= ICU_IS_EDGE;
> +	icu_int |= icu_group << ICU_GROUP_SHIFT;
> +	writel(icu_int, icu->base + ICU_INT_CFG(hwirq));
> +
> +	/*
> +	 * The SATA unit has 2 ports, and a dedicated ICU entry per
> +	 * port. The ahci sata driver supports only one irq interrupt
> +	 * per SATA unit. To solve this conflict, we configure the 2
> +	 * SATA wired interrupts in the south bridge into 1 GIC
> +	 * interrupt in the north bridge. Even if only a single port
> +	 * is enabled, if sata node is enabled, both interrupts are
> +	 * configured (regardless of which port is actually in use).
> +	 * The ICU index of SATA0 = 107, SATA1 = 109
> +	 */
> +	if (hwirq == ICU_SATA0_IRQ_INT || hwirq == ICU_SATA1_IRQ_INT) {
> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA0_IRQ_INT));
> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA1_IRQ_INT));
> +	}
> +
> +	err = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +					    &icu->irq_chip, icu);
> +	if (err) {
> +		dev_err(icu->dev, "failed to set the data to IRQ domain\n");
> +		mvebu_icu_irq_parent_domain_free(domain, virq, irq_msg_num);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static void
> +mvebu_icu_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> +			  unsigned int nr_irqs)
> +{
> +	struct mvebu_icu *icu = domain->host_data;
> +	struct irq_data *irq = irq_get_irq_data(virq);
> +	struct irq_data *irq_parent = irq->parent_data;
> +	int irq_msg_num = mvebu_icu_gicspi_to_icuidx(irqd_to_hwirq(irq_parent));
> +
> +	WARN_ON(nr_irqs != 1);
> +
> +	writel(0, icu->base + ICU_INT_CFG(irqd_to_hwirq(irq)));
> +
> +	mvebu_icu_irq_parent_domain_free(domain, virq, irq_msg_num);
> +}
> +
> +static const struct irq_domain_ops mvebu_icu_domain_ops = {
> +	.translate = mvebu_icu_irq_domain_translate,
> +	.alloc     = mvebu_icu_irq_domain_alloc,
> +	.free      = mvebu_icu_irq_domain_free,
> +};
> +
> +static int mvebu_icu_probe(struct platform_device *pdev)
> +{
> +	struct mvebu_icu *icu;
> +	struct irq_domain *parent_domain;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct platform_device *gicp_pdev;
> +	struct device_node *parent_irq_dn;
> +	struct device_node *gicp_dn;
> +	struct resource *res;
> +	struct resource *gicp_res;
> +	u32 i, icu_int;
> +	int ret;
> +
> +	icu = devm_kzalloc(&pdev->dev, sizeof(struct mvebu_icu),
> +			   GFP_KERNEL);
> +	if (!icu)
> +		return -ENOMEM;
> +
> +	icu->dev = &pdev->dev;
> +
> +	icu->irq_chip.name = kstrdup(dev_name(&pdev->dev), GFP_KERNEL);
> +	icu->irq_chip.irq_mask = irq_chip_mask_parent;
> +	icu->irq_chip.irq_unmask = irq_chip_unmask_parent;
> +	icu->irq_chip.irq_eoi = irq_chip_eoi_parent;
> +	icu->irq_chip.irq_set_type = irq_chip_set_type_parent;
> +#ifdef CONFIG_SMP
> +	icu->irq_chip.irq_set_affinity = irq_chip_set_affinity_parent;
> +#endif
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	icu->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(icu->base)) {
> +		dev_err(&pdev->dev, "Failed to map icu base address.\n");
> +		return PTR_ERR(icu->base);
> +	}
> +
> +	gicp_dn = of_parse_phandle(node, "gicp", 0);
> +	if (!gicp_dn) {
> +		dev_err(&pdev->dev, "Missing gicp property.\n");
> +		return -ENODEV;
> +	}
> +
> +	gicp_pdev = of_find_device_by_node(gicp_dn);
> +	if (!gicp_pdev) {
> +		dev_err(&pdev->dev, "Cannot find gicp device.\n");
> +		return -ENODEV;
> +	}
> +
> +	get_device(&gicp_pdev->dev);
> +
> +	/*
> +	 * We really want the GICP to have cleared all interrupts
> +	 * potentially left pending by the firmware before probing the
> +	 * ICUs.
> +	 */
> +	if (!device_is_bound(&gicp_pdev->dev)) {
> +		ret = -EPROBE_DEFER;
> +		goto fail;
> +	}
> +
> +	gicp_res = platform_get_resource(gicp_pdev, IORESOURCE_MEM, 0);
> +	if (!gicp_res) {
> +		dev_err(&pdev->dev, "Failed to get gicp resource\n");
> +		ret = -ENODEV;
> +		goto fail;
> +	}
> +
> +	parent_irq_dn = of_irq_find_parent(node);
> +	if (!parent_irq_dn) {
> +		dev_err(&pdev->dev, "failed to find parent IRQ node\n");
> +		ret = -ENODEV;
> +		goto fail;
> +	}
> +
> +	parent_domain = irq_find_host(parent_irq_dn);
> +	if (!parent_domain) {
> +		dev_err(&pdev->dev, "Unable to locate ICU parent domain\n");
> +		ret = -ENODEV;
> +		goto fail;
> +	}
> +
> +	/* Set Clear/Set ICU SPI message address in AP */
> +	writel(upper_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
> +	       icu->base + ICU_SETSPI_NSR_AH);
> +	writel(lower_32_bits(gicp_res->start + GICP_SETSPI_NSR_OFFSET),
> +	       icu->base + ICU_SETSPI_NSR_AL);
> +	writel(upper_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
> +	       icu->base + ICU_CLRSPI_NSR_AH);
> +	writel(lower_32_bits(gicp_res->start + GICP_CLRSPI_NSR_OFFSET),
> +	       icu->base + ICU_CLRSPI_NSR_AL);
> +
> +	/*
> +	 * Clean all ICU interrupts with type SPI_NSR, required to
> +	 * avoid unpredictable SPI assignments done by firmware.
> +	 */
> +	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {
> +		icu_int = readl(icu->base + ICU_INT_CFG(i));
> +		if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR)
> +			writel(0x0, icu->base + ICU_INT_CFG(i));
> +	}
> +
> +	icu->domain = irq_domain_add_hierarchy(parent_domain, 0,
> +					       ICU_MAX_SPI_IRQ_IN_GIC, node,
> +					       &mvebu_icu_domain_ops, icu);
> +	if (!icu->domain) {
> +		dev_err(&pdev->dev, "Failed to create ICU domain\n");
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	return 0;
> +fail:
> +	put_device(&gicp_pdev->dev);
> +	return ret;
> +}
> +
> +static const struct of_device_id mvebu_icu_of_match[] = {
> +	{ .compatible = "marvell,icu", },
> +	{},
> +};
> +
> +static struct platform_driver mvebu_icu_driver = {
> +	.probe  = mvebu_icu_probe,
> +	.driver = {
> +		.name = "mvebu-icu",
> +		.of_match_table = mvebu_icu_of_match,
> +	},
> +};
> +builtin_platform_driver(mvebu_icu_driver);
> diff --git a/include/dt-bindings/interrupt-controller/mvebu-icu.h b/include/dt-bindings/interrupt-controller/mvebu-icu.h
> new file mode 100644
> index 0000000..8249558
> --- /dev/null
> +++ b/include/dt-bindings/interrupt-controller/mvebu-icu.h
> @@ -0,0 +1,15 @@
> +/*
> + * This header provides constants for the MVEBU ICU driver.
> + */
> +
> +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H
> +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H
> +
> +/* interrupt specifier cell 0 */
> +
> +#define ICU_GRP_NSR		0x0
> +#define ICU_GRP_SR		0x1
> +#define ICU_GRP_SEI		0x4
> +#define ICU_GRP_REI		0x5
> +
> +#endif
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Thomas Petazzoni]

Hello,

On Tue, 30 May 2017 13:19:07 +0100, Russell King - ARM Linux wrote:

> I see we're still duplicating work.
> 
> Yes, I know you'll reply with your "your tree is a private tree" blah
> blah, which is fine if you want to keep re-doing work that others have
> done, but it's incredibly inefficient.
> 
> What you call my private tree is the public ARM git tree - hardly
> private.  It's the one which core ARM changes go through.  The mcbin
> changes are in a separate branch of that tree - hardly private by
> any definition of that term.  It's as private as your own
> free-electrons git tree.

None of the changes in this series are core ARM changes, they touch
Device Tree bindings, irqchip drivers and Device Tree files, none of
which you are maintainer for. Your tree therefore has no higher
authority than any other tree, so using the fact that the core ARM
changes go through your tree as an argument to make people believe your
tree is more important than the contributions from other developers is
a very fallacious argument.

> Given that during the previous rounds I was willing to work with you,
> testing your patches and helping the effort along, I find your attitude
> towards me to be very counter-productive - you seem to have a very big
> NIH problem.
> 
> How about working _together_ sharing the effort, rather than competing?
> 
> I have a 31 patch series in my tree cleaning up the Marvell ICU support
> and adding PM support to the driver - obviously too large to post as-is
> for mainline, but intended for the changes to be reviewable.  Obviously
> that was now a total waste of my time.
> 
> I don't see why I should get involved in the future with Armada 8k, it
> seems whatever effort I put in is wasted.

The problem is that you do tons of work on your side, but never submit
anything. Or when you submit something, it's enormous patch series that
hardly get reviewed because they are too big.

I'm sorry, but we can't wait for you to decide if/when you will send
your patch series. There are some topics that we need to make progress
on, and since you're not sending anything, we have to do it at some
point.

It's exactly the same story as the one with the patches sent by Marcin.
The fact that you have a wonderful set of patches in your private
branch doesn't make the upstream kernel move forward.

Once again: patches not submitted to the mailing list simply don't
exist. So if you continue to keep those huge stack of patches out of
tree and don't submit your work more regularly, in fine-grained patches
series, the situation we have today will continue to happen.

We knew that you were doing some work on the ICU, we knew you were
doing some work on gpio/pinctrl, but none of this was ever submitted.
After several months, it was time to move forward on those topics.

If you want to avoid this situation, submit your patches. Early and
often.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Russell King - ARM Linux]

On Tue, May 30, 2017 at 02:33:50PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 30 May 2017 13:19:07 +0100, Russell King - ARM Linux wrote:
> 
> > I see we're still duplicating work.
> > 
> > Yes, I know you'll reply with your "your tree is a private tree" blah
> > blah, which is fine if you want to keep re-doing work that others have
> > done, but it's incredibly inefficient.
> > 
> > What you call my private tree is the public ARM git tree - hardly
> > private.  It's the one which core ARM changes go through.  The mcbin
> > changes are in a separate branch of that tree - hardly private by
> > any definition of that term.  It's as private as your own
> > free-electrons git tree.
> 
> None of the changes in this series are core ARM changes, they touch
> Device Tree bindings, irqchip drivers and Device Tree files, none of
> which you are maintainer for. Your tree therefore has no higher
> authority than any other tree, so using the fact that the core ARM
> changes go through your tree as an argument to make people believe your
> tree is more important than the contributions from other developers is
> a very fallacious argument.
> 
> > Given that during the previous rounds I was willing to work with you,
> > testing your patches and helping the effort along, I find your attitude
> > towards me to be very counter-productive - you seem to have a very big
> > NIH problem.
> > 
> > How about working _together_ sharing the effort, rather than competing?
> > 
> > I have a 31 patch series in my tree cleaning up the Marvell ICU support
> > and adding PM support to the driver - obviously too large to post as-is
> > for mainline, but intended for the changes to be reviewable.  Obviously
> > that was now a total waste of my time.
> > 
> > I don't see why I should get involved in the future with Armada 8k, it
> > seems whatever effort I put in is wasted.
> 
> The problem is that you do tons of work on your side, but never submit
> anything. Or when you submit something, it's enormous patch series that
> hardly get reviewed because they are too big.

The choice is between a large series of patches, where each patch makes
one incremental change, or a smaller series where each patch makes a
big number of changes combined.

It is my belief (and also requested by others) that patches that make
only one incremental change at a time are easier to review than a smaller
set of patches that make many changes.  This means large patch series.

> I'm sorry, but we can't wait for you to decide if/when you will send
> your patch series. There are some topics that we need to make progress
> on, and since you're not sending anything, we have to do it at some
> point.

I don't have the bandwidth to constantly post and re-post patches all
the time, especially when it's not clear whether they're even appropriate.
In the case of the ICU, I had come to the conclusion that we don't
need support for the ICU, as the default mappings setup by the firmware
appear to be sufficient for everything.

> Once again: patches not submitted to the mailing list simply don't
> exist. So if you continue to keep those huge stack of patches out of
> tree and don't submit your work more regularly, in fine-grained patches
> series, the situation we have today will continue to happen.

Given the number of patch sets that I have, that is simply an
impossibility to do on a continual basis - I have close to 500
patches, and there's simply no way to post that number of patches.

> We knew that you were doing some work on the ICU, we knew you were
> doing some work on gpio/pinctrl, but none of this was ever submitted.
> After several months, it was time to move forward on those topics.

Right, so try _talking_ to me - like Hans had done over the long
weekend, asking what was happening with the dw-hdmi CEC driver
(which I'm now working on publishing a patch set for.)  The only
way to deal with the amount of patches I have is based on requests
from people.

> If you want to avoid this situation, submit your patches. Early and
> often.

That's quite impossible for me as explained above - I've tried for
years to reduce the number of patches, and I've yet to succeed.  The
only way I think I can do it is to basically stop and delete the
git tree, and walk away from the kernel completely.  That's obviously
not going to happen.

What I'm asking for is some give-and-take co-operation.  I've pulled
patches from your tree for the SDHCI and ethernet support that you
haven't emailed out in order to keep things moving forward, yet you
seem to be completely unwilling to reciprocate.  That makes it very
difficult to work co-operatively.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Andrew Lunn]

> > Once again: patches not submitted to the mailing list simply don't
> > exist. So if you continue to keep those huge stack of patches out of
> > tree and don't submit your work more regularly, in fine-grained patches
> > series, the situation we have today will continue to happen.
> 
> Given the number of patch sets that I have, that is simply an
> impossibility to do on a continual basis - I have close to 500
> patches, and there's simply no way to post that number of patches.

So you don't expect to every have your 500 patches merged?

What might help is that you ask for help getting them merged. I know
of at least 4 groups of people interested in your devlink and 10G PHY
code. If you handed those patches over to these people, i'm sure we
could find somebody to post them to the list, deal with the feedback,
etc, to get them merged. It just needs you to say it is O.K. for
somebody to take patches from your git tree.

You are great at writing new code, but terrible at getting it
merged. So try to find somebody who you trust, who you can hand the
patches over to, to do the merge work.

     Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Thomas Petazzoni]

Hello,

On Tue, 30 May 2017 15:27:35 +0200, Andrew Lunn wrote:

> > Given the number of patch sets that I have, that is simply an
> > impossibility to do on a continual basis - I have close to 500
> > patches, and there's simply no way to post that number of patches.  
> 
> So you don't expect to every have your 500 patches merged?
> 
> What might help is that you ask for help getting them merged. I know
> of at least 4 groups of people interested in your devlink and 10G PHY
> code. If you handed those patches over to these people, i'm sure we
> could find somebody to post them to the list, deal with the feedback,
> etc, to get them merged. It just needs you to say it is O.K. for
> somebody to take patches from your git tree.
> 
> You are great at writing new code, but terrible at getting it
> merged. So try to find somebody who you trust, who you can hand the
> patches over to, to do the merge work.

For the record: my colleague Boris Brezillon had some time available to
work on SFP support and was willing to help Russell moving things
forward. So he talked with Russell, who as usual said "I'm working on
it".

That was in February. We're in May. Nothing happened.

So I can only agree with Andrew here: Russell you're doing awesome
technical work in terms of writing code, but you don't know how to get
it merged, and you block people trying to help you. So don't be
surprised if other people reinvent the same wheel and get it merged
before you.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Russell King - ARM Linux]

On Tue, May 30, 2017 at 03:27:35PM +0200, Andrew Lunn wrote:
> So you don't expect to every have your 500 patches merged?

Correct.

> What might help is that you ask for help getting them merged. I know
> of at least 4 groups of people interested in your devlink and 10G PHY
> code.

As I've repeatedly explained - and it's called phylink, not devlink -
the SFP support is not ready due to the SFP+ stuff needing a complete
rewrite of that code, which has been delayed because of delays on
SolidRun's side to provide SFP+ hardware.

Much of the changes for that happened during the last month, but are
currently rather dirty and incomplete, but are in a working state.
As you know, I had been pushing out the 10G PHY changes as quickly as
netdev copes with them during the previous kernel cycle.  The rate of
patch merging was completely insufficient to get the 10G support into
netdev - with it taking a week or longer between patches posted and
the merge happening.

> You are great at writing new code, but terrible at getting it
> merged.

Yes, because I find it very very time consuming and frustrating to
deal with other people - it's really not easy.

What am I working on right now - I'm trying to get the dw-hdmi CEC
driver out there.  I've been working on that all morning so far,
but it's taking hours, despite having a previously working patch
set... and the latest problem I find is that iMX6 appears to silently
fail to boot.  So, this afternoon, I'm debugging why 4.12-rc3 on
iMX6 seems to be broken, and while I'm doing that I won't be doing
anything _else_, probably not even looking at any email.  So this
will probably be the last email you see from me until I have the
dw-hdmi stuff sorted.  That means I won't be doing _anything_ else
while I sort out a set of patches for this.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Andrew Lunn]

1;4601;0cOn Tue, May 30, 2017 at 02:42:26PM +0100, Russell King - ARM Linux wrote:
> On Tue, May 30, 2017 at 03:27:35PM +0200, Andrew Lunn wrote:
> > So you don't expect to every have your 500 patches merged?
> 
> Correct.
> 
> > What might help is that you ask for help getting them merged. I know
> > of at least 4 groups of people interested in your devlink and 10G PHY
> > code.
> 
> As I've repeatedly explained - and it's called phylink, not devlink -

Sorry, my error. To many different -link things in the network stack.

> the SFP support is not ready due to the SFP+ stuff needing a complete
> rewrite of that code, which has been delayed because of delays on
> SolidRun's side to provide SFP+ hardware.

Linux has a long history of reworking stuff in tree, when it has been
shown to be inadequate in its first version. So long as the device
tree binding does not need incompatible changes, this reworking is not
an issue. My guess is, a lot of people have SFP sockets, not
SFP+. Lets get SFP merged, and then rework it in tree to add SFP+.

> Much of the changes for that happened during the last month, but are
> currently rather dirty and incomplete, but are in a working state.
> As you know, I had been pushing out the 10G PHY changes as quickly as
> netdev copes with them during the previous kernel cycle.  The rate of
> patch merging was completely insufficient to get the 10G support into
> netdev - with it taking a week or longer between patches posted and
> the merge happening.

netdev is the fastest tree i've worked in to get stuff merged. It
averages around 3 days. But it requires both sides to be very active,
submitters and reviewers.

> > You are great at writing new code, but terrible at getting it
> > merged.
> 
> Yes, because I find it very very time consuming and frustrating to
> deal with other people - it's really not easy.

So as i suggested, outsource that part of the work.

   Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Russell King - ARM Linux]

On Tue, May 30, 2017 at 04:03:20PM +0200, Andrew Lunn wrote:
> Linux has a long history of reworking stuff in tree, when it has been
> shown to be inadequate in its first version. So long as the device
> tree binding does not need incompatible changes, this reworking is not
> an issue. My guess is, a lot of people have SFP sockets, not
> SFP+. Lets get SFP merged, and then rework it in tree to add SFP+.

Unfortunately, it _does_ require an incompatible DT change, and an
incompatible change with the MAC drivers.

The DT change is needed because the current DT model (modelled from
DSA) connects the SFP cage to the MAC device using (eg):

	sfp {
		...
		sfp,ethernet = <&eth2>;
	};

This completely breaks when you have SFP connected to a PHY, as is
the case with SFP+.  So the current binding is unusable for this
case.

Instead, what I have (and what I will propose) is to get rid of that
property entirely, replacing it with a property in the upstream device
(being a MAC or PHY), eg:

&eth2 {
	sfp = <&sfp>;
};

        p1_phy: ethernet-phy@8 {
                sfp = <&sfp_eth1>;
        };

The code changes behind this would make maintaining support for the
previous binding rather difficult, as the way the SFP code finds the
netdevice changes completely - I now have a separate "sfp-bus", which
both phylink and SFP sockets register into, and which is responsible
for connecting the two together.

This change would not be possible had SFP support already been merged.
The old binding was just wrong.

I pushed out some updates to the SFP support last week, and now that
I have dw-hdmi out of the way, I'm about to merge these incompatible
changes into the SFP branch, and as the branch is currently at 24
patches, I'm probably going to squash a lot of the patches in there
together at the same time - which'll make me feel a bit sorry for
anyone who's making use of the existing code, because they won't be
able to see what the changes have been.  However, that's the only way
to stop the patch set going over 30...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Thomas Petazzoni]

Hello,

On Tue, 30 May 2017 14:42:26 +0100, Russell King - ARM Linux wrote:

> As I've repeatedly explained - and it's called phylink, not devlink -
> the SFP support is not ready due to the SFP+ stuff needing a complete
> rewrite of that code, which has been delayed because of delays on
> SolidRun's side to provide SFP+ hardware.

This is a very good illustration of why your contribution process
doesn't work: you wait to have the full-featured perfect solution
before submitting anything. Due to that, either you have huge series
that nobody ever reviews, or you never reach the point were things are
perfect enough to your taste, and therefore nothing ever gets submitted.

Don't be surprised if others take the lead, work on the same topics and
submit patches.

> Much of the changes for that happened during the last month, but are
> currently rather dirty and incomplete, but are in a working state.
> As you know, I had been pushing out the 10G PHY changes as quickly as
> netdev copes with them during the previous kernel cycle.  The rate of
> patch merging was completely insufficient to get the 10G support into
> netdev - with it taking a week or longer between patches posted and
> the merge happening.

A week is *nothing*. On other topics, you have to wait months to get
reviews from maintainers, and still people manage to get things merged
upstream.

> > You are great at writing new code, but terrible at getting it
> > merged.  
> 
> Yes, because I find it very very time consuming and frustrating to
> deal with other people - it's really not easy.

Fair enough. But in this case, please stop complaining when other
people are willing to take the time and effort to submit patches.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Russell King - ARM Linux]

On Tue, May 30, 2017 at 03:27:35PM +0200, Andrew Lunn wrote:
> What might help is that you ask for help getting them merged. I know
> of at least 4 groups of people interested in your devlink and 10G PHY
> code. If you handed those patches over to these people, i'm sure we
> could find somebody to post them to the list, deal with the feedback,
> etc, to get them merged. It just needs you to say it is O.K. for
> somebody to take patches from your git tree.

Is that not what I'm trying to do with these A8k changes?  What I'm
instead getting is "your tree is private, we're not touching it".

Okay, so let me put it another way: please help me merge the A8k
changes that I have in my tree.

There, I've said it.  However, I know what the answer will be, since
it's already effectively been given in this thread: "piss off".

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Andrew Lunn]

On Tue, May 30, 2017 at 03:39:02PM +0100, Russell King - ARM Linux wrote:
> On Tue, May 30, 2017 at 03:27:35PM +0200, Andrew Lunn wrote:
> > What might help is that you ask for help getting them merged. I know
> > of at least 4 groups of people interested in your devlink and 10G PHY
> > code. If you handed those patches over to these people, i'm sure we
> > could find somebody to post them to the list, deal with the feedback,
> > etc, to get them merged. It just needs you to say it is O.K. for
> > somebody to take patches from your git tree.
> 
> Is that not what I'm trying to do with these A8k changes?  What I'm
> instead getting is "your tree is private, we're not touching it".
> 
> Okay, so let me put it another way: please help me merge the A8k
> changes that I have in my tree.

Hi Russell

So you are happy for Free-Electrons, or anybody else, to take patches
from your tree, submit them, handle comments, resubmit them, etc?

     Andrew

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Russell King - ARM Linux]

On Tue, May 30, 2017 at 04:49:34PM +0200, Andrew Lunn wrote:
> On Tue, May 30, 2017 at 03:39:02PM +0100, Russell King - ARM Linux wrote:
> > On Tue, May 30, 2017 at 03:27:35PM +0200, Andrew Lunn wrote:
> > > What might help is that you ask for help getting them merged. I know
> > > of at least 4 groups of people interested in your devlink and 10G PHY
> > > code. If you handed those patches over to these people, i'm sure we
> > > could find somebody to post them to the list, deal with the feedback,
> > > etc, to get them merged. It just needs you to say it is O.K. for
> > > somebody to take patches from your git tree.
> > 
> > Is that not what I'm trying to do with these A8k changes?  What I'm
> > instead getting is "your tree is private, we're not touching it".
> > 
> > Okay, so let me put it another way: please help me merge the A8k
> > changes that I have in my tree.
> 
> Hi Russell
> 
> So you are happy for Free-Electrons, or anybody else, to take patches
> from your tree, submit them, handle comments, resubmit them, etc?

I am happy for the A8k changes, provided there's coordination, so we
reasonably avoid to duplicate effort, but I think we're already at
the point where most of my effort has already been duplicated.

If someone wants to take (eg) my ICU patches and fit what's appropriate
on top of Thomas', then I'd welcome that effort.

Obviously, the PP2x changes in my tree are no-hopers for mainline as
the decision has been made to improve the existing PP2 driver, and
I think the gpio/pinctrl changes are now no-hopers as that work has
already been done independently.

I'm not sure there's much other stuff left out of the 63 patches that
comprise my "mvebu" branch as far as core A8k changes go.

For the mcbin changes (that come after the mvebu/phy merge), I think
everything up to and including "arm64: dts: marvell: mcbin: add GPIO"
is probably mainline-candidates (except for the PP2 driver obviously),
the sdhci DTS patch has already been picked up.

"arm64: dts: marvell: mcbin: workaround wrongly wired i2c1 bus" is
definitely not for mainline, as that is only for early revision
boards, and SR does not want that merged into mainline (as stated
in its commit log.)

"arm64: dts: marvell: mcbin: add remainder of pinctrls" and
"usb: host: xhci: mvebu: add reset on resume quirk" might also be
candidates, I'm not sure without looking further.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Thomas Petazzoni]

Hello,

On Tue, 30 May 2017 13:56:43 +0100, Russell King - ARM Linux wrote:

> > The problem is that you do tons of work on your side, but never submit
> > anything. Or when you submit something, it's enormous patch series that
> > hardly get reviewed because they are too big.  
> 
> The choice is between a large series of patches, where each patch makes
> one incremental change, or a smaller series where each patch makes a
> big number of changes combined.
> 
> It is my belief (and also requested by others) that patches that make
> only one incremental change at a time are easier to review than a smaller
> set of patches that make many changes.  This means large patch series.

I was obviously not suggesting to artificially reduce the number of
patches by making each patch bigger.

Instead, I was suggesting to post things more regularly. I.e instead of
developing a full-featured solution that solves all problems, but
remains behind the scenes for months, post a minimal solution first
(which is often a smaller patch series) and sent post incremental patch
series to improve on top of it.

> I don't have the bandwidth to constantly post and re-post patches all
> the time, especially when it's not clear whether they're even appropriate.
> In the case of the ICU, I had come to the conclusion that we don't
> need support for the ICU, as the default mappings setup by the firmware
> appear to be sufficient for everything.

So why on earth do you care and yell about me posting ICU-related
patches ?

We do have reasons to believe that it is necessary, and that the
default mappings setup by the firmware will not be sufficient on the
long term, which is why we want to add ICU support.

> > Once again: patches not submitted to the mailing list simply don't
> > exist. So if you continue to keep those huge stack of patches out of
> > tree and don't submit your work more regularly, in fine-grained patches
> > series, the situation we have today will continue to happen.  
> 
> Given the number of patch sets that I have, that is simply an
> impossibility to do on a continual basis - I have close to 500
> patches, and there's simply no way to post that number of patches.

So why do you complain when others submit patches if you do not?

What you're doing with 500 patches out of tree that you do not have the
bandwidth to upstream is exactly what vendors are doing with their evil
vendor trees. But at least they don't complain when others actually
submit patches to get them merged upstream.

> > We knew that you were doing some work on the ICU, we knew you were
> > doing some work on gpio/pinctrl, but none of this was ever submitted.
> > After several months, it was time to move forward on those topics.  
> 
> Right, so try _talking_ to me - like Hans had done over the long
> weekend, asking what was happening with the dw-hdmi CEC driver
> (which I'm now working on publishing a patch set for.)  The only
> way to deal with the amount of patches I have is based on requests
> from people.

We have already talked about the ICU months ago, you were working on
it. Several months later, still no patches on the mailing list.

If I had asked you again, you would have told me "I have ICU patches I
intend to send". But since you haven't submitted them for several
months, it was time to do something about it.

Sorry, but your behavior of "I have things available and supported in
my private branch, so don't step on my toes" simply doesn't work,
because you never submit your work, as you admit yourself.

> That's quite impossible for me as explained above - I've tried for
> years to reduce the number of patches, and I've yet to succeed.  The
> only way I think I can do it is to basically stop and delete the
> git tree, and walk away from the kernel completely.  That's obviously
> not going to happen.
> 
> What I'm asking for is some give-and-take co-operation.  I've pulled
> patches from your tree for the SDHCI and ethernet support that you
> haven't emailed out in order to keep things moving forward, yet you
> seem to be completely unwilling to reciprocate.  That makes it very
> difficult to work co-operatively.

The big difference is that the SDHCI and Ethernet patches we have done
have been submitted and merged, just like everything we do for the
support of Marvell platforms.

On the other end, your ICU patches have never been submitted and merged.
So whenever you tell me that you are working on a topic, I don't trust
that this topic will actually move forward upstream because you keep
everything in your private branch, which is totally useless.

So until you insist keeping everything in your private branch, people
will keep ignoring whatever you are doing.

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] arm64: dts: marvell: enable GICP and ICU on Armada 7K/8K

[Thomas Petazzoni]

This commit modifies the Marvell EBU Armada 7K and 8K Device Tree files
to describe the ICU and GICP units, and use ICU interrupts for all
devices in the CP110 blocks.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi      |  5 ++
 .../boot/dts/marvell/armada-cp110-master.dtsi      | 60 +++++++++++++---------
 .../arm64/boot/dts/marvell/armada-cp110-slave.dtsi | 54 ++++++++++---------
 3 files changed, 71 insertions(+), 48 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
index fe41bf9..cb87831 100644
--- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
@@ -146,6 +146,11 @@
 				marvell,spi-base = <128>, <136>, <144>, <152>;
 			};
 
+			gicp: gicp@3f0040 {
+				compatible = "marvell,gicp";
+				reg = <0x3f0040 0x10>;
+			};
+
 			pic: interrupt-controller@3f0100 {
 				compatible = "marvell,armada-8k-pic";
 				reg = <0x3f0100 0x10>;
diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
index ac8df52..fe899e8 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
@@ -44,19 +44,20 @@
  * Device Tree file for Marvell Armada CP110 Master.
  */
 
+#include <dt-bindings/interrupt-controller/mvebu-icu.h>
+
 / {
 	cp110-master {
 		#address-cells = <2>;
 		#size-cells = <2>;
 		compatible = "simple-bus";
-		interrupt-parent = <&gic>;
+		interrupt-parent = <&cpm_icu>;
 		ranges;
 
 		config-space@f2000000 {
 			#address-cells = <1>;
 			#size-cells = <1>;
 			compatible = "simple-bus";
-			interrupt-parent = <&gic>;
 			ranges = <0x0 0x0 0xf2000000 0x2000000>;
 
 			cpm_ethernet: ethernet@0 {
@@ -68,21 +69,21 @@
 				dma-coherent;
 
 				cpm_eth0: eth0 {
-					interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 39 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <0>;
 					gop-port-id = <0>;
 					status = "disabled";
 				};
 
 				cpm_eth1: eth1 {
-					interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 40 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <1>;
 					gop-port-id = <2>;
 					status = "disabled";
 				};
 
 				cpm_eth2: eth2 {
-					interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 41 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <2>;
 					gop-port-id = <3>;
 					status = "disabled";
@@ -96,6 +97,15 @@
 				reg = <0x12a200 0x10>;
 			};
 
+			cpm_icu: interrupt-controller@1e0000 {
+				compatible = "marvell,icu";
+				reg = <0x1e0000 0x10>;
+				#interrupt-cells = <3>;
+				interrupt-controller;
+				interrupt-parent = <&gic>;
+				gicp = <&gicp>;
+			};
+
 			cpm_syscon0: system-controller@440000 {
 				compatible = "marvell,cp110-system-controller0",
 					     "syscon";
@@ -120,14 +130,14 @@
 				compatible = "marvell,armada-8k-rtc";
 				reg = <0x284000 0x20>, <0x284080 0x24>;
 				reg-names = "rtc", "rtc-soc";
-				interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
 			cpm_sata0: sata@540000 {
 				compatible = "marvell,armada-8k-ahci",
 					     "generic-ahci";
 				reg = <0x540000 0x30000>;
-				interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 107 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 15>;
 				status = "disabled";
 			};
@@ -137,7 +147,7 @@
 					     "generic-xhci";
 				reg = <0x500000 0x4000>;
 				dma-coherent;
-				interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 106 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 22>;
 				status = "disabled";
 			};
@@ -147,7 +157,7 @@
 					     "generic-xhci";
 				reg = <0x510000 0x4000>;
 				dma-coherent;
-				interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 105 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 23>;
 				status = "disabled";
 			};
@@ -195,7 +205,7 @@
 				reg = <0x701000 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
-				interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 120 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 21>;
 				status = "disabled";
 			};
@@ -205,7 +215,7 @@
 				reg = <0x701100 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
-				interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 121 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 21>;
 				status = "disabled";
 			};
@@ -213,7 +223,7 @@
 			cpm_trng: trng@760000 {
 				compatible = "marvell,armada-8k-rng", "inside-secure,safexcel-eip76";
 				reg = <0x760000 0x7d>;
-				interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 95 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 25>;
 				status = "okay";
 			};
@@ -221,7 +231,7 @@
 			cpm_sdhci0: sdhci@780000 {
 				compatible = "marvell,armada-cp110-sdhci";
 				reg = <0x780000 0x300>;
-				interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 27 IRQ_TYPE_LEVEL_HIGH>;
 				clock-names = "core";
 				clocks = <&cpm_syscon0 1 4>;
 				dma-coherent;
@@ -231,13 +241,13 @@
 			cpm_crypto: crypto@800000 {
 				compatible = "inside-secure,safexcel-eip197";
 				reg = <0x800000 0x200000>;
-				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING
+				interrupts = <ICU_GRP_NSR 87 (IRQ_TYPE_EDGE_RISING
 				| IRQ_TYPE_LEVEL_HIGH)>,
-					     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
+					     <ICU_GRP_NSR 88 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 89 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 90 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 91 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 92 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-names = "mem", "ring0", "ring1",
 				"ring2", "ring3", "eip";
 				clocks = <&cpm_syscon0 1 26>;
@@ -264,8 +274,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xf6000000 0  0xf6000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
 			num-lanes = <1>;
 			clocks = <&cpm_syscon0 1 13>;
 			status = "disabled";
@@ -290,8 +300,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xf7000000 0  0xf7000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
 			clocks = <&cpm_syscon0 1 11>;
@@ -317,8 +327,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xf8000000 0  0xf8000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
 			clocks = <&cpm_syscon0 1 12>;
diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
index 7740a75..f9cf4f3 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
@@ -49,14 +49,13 @@
 		#address-cells = <2>;
 		#size-cells = <2>;
 		compatible = "simple-bus";
-		interrupt-parent = <&gic>;
+		interrupt-parent = <&cps_icu>;
 		ranges;
 
 		config-space@f4000000 {
 			#address-cells = <1>;
 			#size-cells = <1>;
 			compatible = "simple-bus";
-			interrupt-parent = <&gic>;
 			ranges = <0x0 0x0 0xf4000000 0x2000000>;
 
 			cps_rtc: rtc@284000 {
@@ -75,21 +74,21 @@
 				dma-coherent;
 
 				cps_eth0: eth0 {
-					interrupts = <GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 39 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <0>;
 					gop-port-id = <0>;
 					status = "disabled";
 				};
 
 				cps_eth1: eth1 {
-					interrupts = <GIC_SPI 262 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 40 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <1>;
 					gop-port-id = <2>;
 					status = "disabled";
 				};
 
 				cps_eth2: eth2 {
-					interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 41 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <2>;
 					gop-port-id = <3>;
 					status = "disabled";
@@ -103,6 +102,15 @@
 				reg = <0x12a200 0x10>;
 			};
 
+			cps_icu: interrupt-controller@1e0000 {
+				compatible = "marvell,icu";
+				reg = <0x1e0000 0x10>;
+				#interrupt-cells = <3>;
+				interrupt-controller;
+				interrupt-parent = <&gic>;
+				gicp = <&gicp>;
+			};
+
 			cps_syscon0: system-controller@440000 {
 				compatible = "marvell,cp110-system-controller0",
 					     "syscon";
@@ -127,7 +135,7 @@
 				compatible = "marvell,armada-8k-ahci",
 					     "generic-ahci";
 				reg = <0x540000 0x30000>;
-				interrupts = <GIC_SPI 287 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 107 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 15>;
 				status = "disabled";
 			};
@@ -137,7 +145,7 @@
 					     "generic-xhci";
 				reg = <0x500000 0x4000>;
 				dma-coherent;
-				interrupts = <GIC_SPI 286 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 106 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 22>;
 				status = "disabled";
 			};
@@ -147,7 +155,7 @@
 					     "generic-xhci";
 				reg = <0x510000 0x4000>;
 				dma-coherent;
-				interrupts = <GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 105 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 23>;
 				status = "disabled";
 			};
@@ -195,7 +203,7 @@
 				reg = <0x701000 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
-				interrupts = <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 120 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 21>;
 				status = "disabled";
 			};
@@ -205,7 +213,7 @@
 				reg = <0x701100 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
-				interrupts = <GIC_SPI 311 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 121 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 21>;
 				status = "disabled";
 			};
@@ -213,7 +221,7 @@
 			cps_trng: trng@760000 {
 				compatible = "marvell,armada-8k-rng", "inside-secure,safexcel-eip76";
 				reg = <0x760000 0x7d>;
-				interrupts = <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 95 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 25>;
 				status = "okay";
 			};
@@ -221,13 +229,13 @@
 			cps_crypto: crypto@800000 {
 				compatible = "inside-secure,safexcel-eip197";
 				reg = <0x800000 0x200000>;
-				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING
+				interrupts = <ICU_GRP_NSR 87 (IRQ_TYPE_EDGE_RISING
 				| IRQ_TYPE_LEVEL_HIGH)>,
-					     <GIC_SPI 278 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 279 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 280 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 281 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>;
+					     <ICU_GRP_NSR 88 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 89 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 90 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 91 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 92 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-names = "mem", "ring0", "ring1",
 						  "ring2", "ring3", "eip";
 				clocks = <&cps_syscon0 1 26>;
@@ -254,8 +262,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xfa000000 0  0xfa000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cps_icu 0 ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
 			num-lanes = <1>;
 			clocks = <&cps_syscon0 1 13>;
 			status = "disabled";
@@ -280,8 +288,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xfb000000 0  0xfb000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cps_icu 0 ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
 			clocks = <&cps_syscon0 1 11>;
@@ -307,8 +315,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xfc000000 0  0xfc000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cps_icu 0 ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
 			clocks = <&cps_syscon0 1 12>;
-- 
2.7.4

Re: [PATCH 6/6] arm64: dts: marvell: enable GICP and ICU on Armada 7K/8K

[Marc Zyngier]

On 30/05/17 10:16, Thomas Petazzoni wrote:
> This commit modifies the Marvell EBU Armada 7K and 8K Device Tree files
> to describe the ICU and GICP units, and use ICU interrupts for all
> devices in the CP110 blocks.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm64/boot/dts/marvell/armada-ap806.dtsi      |  5 ++
>  .../boot/dts/marvell/armada-cp110-master.dtsi      | 60 +++++++++++++---------
>  .../arm64/boot/dts/marvell/armada-cp110-slave.dtsi | 54 ++++++++++---------
>  3 files changed, 71 insertions(+), 48 deletions(-)
> 

[...]

> @@ -221,13 +229,13 @@
>  			cps_crypto: crypto@800000 {
>  				compatible = "inside-secure,safexcel-eip197";
>  				reg = <0x800000 0x200000>;
> -				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING
> +				interrupts = <ICU_GRP_NSR 87 (IRQ_TYPE_EDGE_RISING
>  				| IRQ_TYPE_LEVEL_HIGH)>,

I've already mentioned this in a separate thread to Antoine, but it'd be
good to fix this non-sensical configuration.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] arm64: dts: marvell: enable GICP and ICU on Armada 7K/8K

[Thomas Petazzoni]

Hello,

On Tue, 30 May 2017 15:02:34 +0100, Marc Zyngier wrote:

> > @@ -221,13 +229,13 @@
> >  			cps_crypto: crypto@800000 {
> >  				compatible = "inside-secure,safexcel-eip197";
> >  				reg = <0x800000 0x200000>;
> > -				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING
> > +				interrupts = <ICU_GRP_NSR 87 (IRQ_TYPE_EDGE_RISING
> >  				| IRQ_TYPE_LEVEL_HIGH)>,  
> 
> I've already mentioned this in a separate thread to Antoine, but it'd be
> good to fix this non-sensical configuration.

Antoine has already submitted a patch, and it has been merged by
Grégory already [1]. I simply didn't base my patches on top of the
latest patch from Antoine, but Grégory will handle the merge conflict
when applying the DT changes.

Thanks!

Thomas

[1] http://git.infradead.org/linux-mvebu.git/commitdiff/44f73dc42c11398d7b84e94365a485ebd6420798
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] arm64: dts: marvell: enable GICP and ICU

[Thomas Petazzoni]

This commit modifies the Marvell EBU Armada 7K and 8K Device Tree files
to describe the ICU and GICP units, and use ICU interrupts for all
devices in the CP110 blocks.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi      |  5 ++
 .../boot/dts/marvell/armada-cp110-master.dtsi      | 60 +++++++++++++---------
 .../arm64/boot/dts/marvell/armada-cp110-slave.dtsi | 54 ++++++++++---------
 3 files changed, 71 insertions(+), 48 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
index fe41bf9..cb87831 100644
--- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
@@ -146,6 +146,11 @@
 				marvell,spi-base = <128>, <136>, <144>, <152>;
 			};
 
+			gicp: gicp@3f0040 {
+				compatible = "marvell,gicp";
+				reg = <0x3f0040 0x10>;
+			};
+
 			pic: interrupt-controller@3f0100 {
 				compatible = "marvell,armada-8k-pic";
 				reg = <0x3f0100 0x10>;
diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
index ac8df52..fe899e8 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
@@ -44,19 +44,20 @@
  * Device Tree file for Marvell Armada CP110 Master.
  */
 
+#include <dt-bindings/interrupt-controller/mvebu-icu.h>
+
 / {
 	cp110-master {
 		#address-cells = <2>;
 		#size-cells = <2>;
 		compatible = "simple-bus";
-		interrupt-parent = <&gic>;
+		interrupt-parent = <&cpm_icu>;
 		ranges;
 
 		config-space@f2000000 {
 			#address-cells = <1>;
 			#size-cells = <1>;
 			compatible = "simple-bus";
-			interrupt-parent = <&gic>;
 			ranges = <0x0 0x0 0xf2000000 0x2000000>;
 
 			cpm_ethernet: ethernet@0 {
@@ -68,21 +69,21 @@
 				dma-coherent;
 
 				cpm_eth0: eth0 {
-					interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 39 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <0>;
 					gop-port-id = <0>;
 					status = "disabled";
 				};
 
 				cpm_eth1: eth1 {
-					interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 40 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <1>;
 					gop-port-id = <2>;
 					status = "disabled";
 				};
 
 				cpm_eth2: eth2 {
-					interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 41 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <2>;
 					gop-port-id = <3>;
 					status = "disabled";
@@ -96,6 +97,15 @@
 				reg = <0x12a200 0x10>;
 			};
 
+			cpm_icu: interrupt-controller@1e0000 {
+				compatible = "marvell,icu";
+				reg = <0x1e0000 0x10>;
+				#interrupt-cells = <3>;
+				interrupt-controller;
+				interrupt-parent = <&gic>;
+				gicp = <&gicp>;
+			};
+
 			cpm_syscon0: system-controller@440000 {
 				compatible = "marvell,cp110-system-controller0",
 					     "syscon";
@@ -120,14 +130,14 @@
 				compatible = "marvell,armada-8k-rtc";
 				reg = <0x284000 0x20>, <0x284080 0x24>;
 				reg-names = "rtc", "rtc-soc";
-				interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
 			cpm_sata0: sata@540000 {
 				compatible = "marvell,armada-8k-ahci",
 					     "generic-ahci";
 				reg = <0x540000 0x30000>;
-				interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 107 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 15>;
 				status = "disabled";
 			};
@@ -137,7 +147,7 @@
 					     "generic-xhci";
 				reg = <0x500000 0x4000>;
 				dma-coherent;
-				interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 106 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 22>;
 				status = "disabled";
 			};
@@ -147,7 +157,7 @@
 					     "generic-xhci";
 				reg = <0x510000 0x4000>;
 				dma-coherent;
-				interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 105 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 23>;
 				status = "disabled";
 			};
@@ -195,7 +205,7 @@
 				reg = <0x701000 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
-				interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 120 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 21>;
 				status = "disabled";
 			};
@@ -205,7 +215,7 @@
 				reg = <0x701100 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
-				interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 121 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 21>;
 				status = "disabled";
 			};
@@ -213,7 +223,7 @@
 			cpm_trng: trng@760000 {
 				compatible = "marvell,armada-8k-rng", "inside-secure,safexcel-eip76";
 				reg = <0x760000 0x7d>;
-				interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 95 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cpm_syscon0 1 25>;
 				status = "okay";
 			};
@@ -221,7 +231,7 @@
 			cpm_sdhci0: sdhci@780000 {
 				compatible = "marvell,armada-cp110-sdhci";
 				reg = <0x780000 0x300>;
-				interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 27 IRQ_TYPE_LEVEL_HIGH>;
 				clock-names = "core";
 				clocks = <&cpm_syscon0 1 4>;
 				dma-coherent;
@@ -231,13 +241,13 @@
 			cpm_crypto: crypto@800000 {
 				compatible = "inside-secure,safexcel-eip197";
 				reg = <0x800000 0x200000>;
-				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING
+				interrupts = <ICU_GRP_NSR 87 (IRQ_TYPE_EDGE_RISING
 				| IRQ_TYPE_LEVEL_HIGH)>,
-					     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
+					     <ICU_GRP_NSR 88 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 89 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 90 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 91 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 92 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-names = "mem", "ring0", "ring1",
 				"ring2", "ring3", "eip";
 				clocks = <&cpm_syscon0 1 26>;
@@ -264,8 +274,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xf6000000 0  0xf6000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
 			num-lanes = <1>;
 			clocks = <&cpm_syscon0 1 13>;
 			status = "disabled";
@@ -290,8 +300,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xf7000000 0  0xf7000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
 			clocks = <&cpm_syscon0 1 11>;
@@ -317,8 +327,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xf8000000 0  0xf8000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
 			clocks = <&cpm_syscon0 1 12>;
diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
index 7740a75..f9cf4f3 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
@@ -49,14 +49,13 @@
 		#address-cells = <2>;
 		#size-cells = <2>;
 		compatible = "simple-bus";
-		interrupt-parent = <&gic>;
+		interrupt-parent = <&cps_icu>;
 		ranges;
 
 		config-space@f4000000 {
 			#address-cells = <1>;
 			#size-cells = <1>;
 			compatible = "simple-bus";
-			interrupt-parent = <&gic>;
 			ranges = <0x0 0x0 0xf4000000 0x2000000>;
 
 			cps_rtc: rtc@284000 {
@@ -75,21 +74,21 @@
 				dma-coherent;
 
 				cps_eth0: eth0 {
-					interrupts = <GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 39 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <0>;
 					gop-port-id = <0>;
 					status = "disabled";
 				};
 
 				cps_eth1: eth1 {
-					interrupts = <GIC_SPI 262 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 40 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <1>;
 					gop-port-id = <2>;
 					status = "disabled";
 				};
 
 				cps_eth2: eth2 {
-					interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH>;
+					interrupts = <ICU_GRP_NSR 41 IRQ_TYPE_LEVEL_HIGH>;
 					port-id = <2>;
 					gop-port-id = <3>;
 					status = "disabled";
@@ -103,6 +102,15 @@
 				reg = <0x12a200 0x10>;
 			};
 
+			cps_icu: interrupt-controller@1e0000 {
+				compatible = "marvell,icu";
+				reg = <0x1e0000 0x10>;
+				#interrupt-cells = <3>;
+				interrupt-controller;
+				interrupt-parent = <&gic>;
+				gicp = <&gicp>;
+			};
+
 			cps_syscon0: system-controller@440000 {
 				compatible = "marvell,cp110-system-controller0",
 					     "syscon";
@@ -127,7 +135,7 @@
 				compatible = "marvell,armada-8k-ahci",
 					     "generic-ahci";
 				reg = <0x540000 0x30000>;
-				interrupts = <GIC_SPI 287 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 107 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 15>;
 				status = "disabled";
 			};
@@ -137,7 +145,7 @@
 					     "generic-xhci";
 				reg = <0x500000 0x4000>;
 				dma-coherent;
-				interrupts = <GIC_SPI 286 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 106 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 22>;
 				status = "disabled";
 			};
@@ -147,7 +155,7 @@
 					     "generic-xhci";
 				reg = <0x510000 0x4000>;
 				dma-coherent;
-				interrupts = <GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 105 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 23>;
 				status = "disabled";
 			};
@@ -195,7 +203,7 @@
 				reg = <0x701000 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
-				interrupts = <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 120 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 21>;
 				status = "disabled";
 			};
@@ -205,7 +213,7 @@
 				reg = <0x701100 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
-				interrupts = <GIC_SPI 311 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 121 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 21>;
 				status = "disabled";
 			};
@@ -213,7 +221,7 @@
 			cps_trng: trng@760000 {
 				compatible = "marvell,armada-8k-rng", "inside-secure,safexcel-eip76";
 				reg = <0x760000 0x7d>;
-				interrupts = <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <ICU_GRP_NSR 95 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&cps_syscon0 1 25>;
 				status = "okay";
 			};
@@ -221,13 +229,13 @@
 			cps_crypto: crypto@800000 {
 				compatible = "inside-secure,safexcel-eip197";
 				reg = <0x800000 0x200000>;
-				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING
+				interrupts = <ICU_GRP_NSR 87 (IRQ_TYPE_EDGE_RISING
 				| IRQ_TYPE_LEVEL_HIGH)>,
-					     <GIC_SPI 278 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 279 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 280 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 281 IRQ_TYPE_LEVEL_HIGH>,
-					     <GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>;
+					     <ICU_GRP_NSR 88 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 89 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 90 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 91 IRQ_TYPE_LEVEL_HIGH>,
+					     <ICU_GRP_NSR 92 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-names = "mem", "ring0", "ring1",
 						  "ring2", "ring3", "eip";
 				clocks = <&cps_syscon0 1 26>;
@@ -254,8 +262,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xfa000000 0  0xfa000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cps_icu 0 ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
 			num-lanes = <1>;
 			clocks = <&cps_syscon0 1 13>;
 			status = "disabled";
@@ -280,8 +288,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xfb000000 0  0xfb000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cps_icu 0 ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
 			clocks = <&cps_syscon0 1 11>;
@@ -307,8 +315,8 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xfc000000 0  0xfc000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>;
-			interrupts = <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cps_icu 0 ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
 			clocks = <&cps_syscon0 1 12>;
-- 
2.7.4

Re: [PATCH 6/6] arm64: dts: marvell: enable GICP and ICU

[Thomas Petazzoni]

Hello,

On Tue, 30 May 2017 11:16:12 +0200, Thomas Petazzoni wrote:
> This commit modifies the Marvell EBU Armada 7K and 8K Device Tree files
> to describe the ICU and GICP units, and use ICU interrupts for all
> devices in the CP110 blocks.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Please disregard this patch. I should remember all patches before using
"git format-patch", this would avoid me sending stale versions of
patches.

Basically the other PATCH 6/6 is exactly the same, with just the title
changed (and is the more correct one).

Sorry about that,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] arm64: marvell: enable ICU and GICP drivers

[Thomas Petazzoni]

This commit enables the newly introduced Marvell GICP and ICUs driver
for the 64-bit Marvell EBU platforms.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm64/Kconfig.platforms | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 4afcffc..bf3b505 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -106,6 +106,8 @@ config ARCH_MVEBU
 	select ARMADA_AP806_SYSCON
 	select ARMADA_CP110_SYSCON
 	select ARMADA_37XX_CLK
+	select MVEBU_GICP
+	select MVEBU_ICU
 	select MVEBU_ODMI
 	select MVEBU_PIC
 	help
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] Add support for the ICU unit in Marvell Armada 7K/8K

[Marc Zyngier]

Hi Thomas,

On 30/05/17 10:16, Thomas Petazzoni wrote:
> Hello,
> 
> The Marvell Armada 7K/8K SoCs are composed of two parts: the AP (which
> contains the CPU cores) and the CP (which contains most
> peripherals). The 7K SoCs have one CP, while the 8K SoCs have two CPs,
> doubling the number of available peripherals.
> 
> In terms of interrupt handling, all devices in the CPs are connected
> through wired interrupt to a unit called ICU located in each CP. This
> unit converts the wired interrupts from the devices into memory
> transactions.
> 
> Inside the AP, there is a GIC extension called GICP, which allows a
> memory write transaction to trigger a GIC SPI interrupt. The ICUs in
> each CP are therefore configured to trigger a memory write into the
> appropriate GICP register so that a wired interrupt from a CP device
> is converted into a memory write, itself converted into a regular GIC
> SPI interrupt.
> 
> Until now, the configuration of the ICU was done statically by the
> firmware, and therefore the Device Tree files in Linux were specifying
> directly GIC interrupts for the interrupts of CP devices. However,
> with the growing number of devices in the CP, a static allocation
> scheme doesn't work for the long term.
> 
> This patch series therefore makes Linux aware of the ICU: GIC SPI
> interrupts are dynamically allocated, and the ICU is configured
> accordingly to route a CP wired interrupt to the allocated GIC SPI
> interrupt.
> 
> In detail:
> 
>  - The first two patches are the Device Tree binding patches
> 
>  - The third patch is a minimal driver for the GICP unit. All it does
>    is clear interrupts that may have been left pending by the
>    firmware.
> 
>  - The fourth patch is the most important done, which adds the driver
>    for the ICU itself.
> 
>  - The fifth patch adjust Kconfig.platforms to select the GICP and ICU
>    drivers.
> 
>  - The last patch adjusts the Device Tree files of the Armada 7K/8K to
>    use the ICU.

For a first drop, this looks quite good, and the few comments I've had
should be pretty easy to address. Looking forward to reviewing v2.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html