[PATCH 0/2] irqchip/mchp-eic: add driver for Microchip EIC

[PATCH 0/2] irqchip/mchp-eic: add driver for Microchip EIC

[Claudiu Beznea]

Hi,

This patch adds support for Microchip External Interrupt Controller
present on SAMA7G5. The controller supports for 2 external interrupt
lines, glitch filter capabilities and is connected to GIC as follows:

pinX   +------+ EXT_IRQ0 +------+ int 153 (for pinX) +------+
------>|      |--------->|      |------------------->|      |
pinY   | PIO  | EXT_IRQ1 | EIC  | int 154 (for pinY) | GIC  |
------>|      |--------->|      |------------------->|      |
       +------+          +------+                    +------+

where PIO is the pin controller.

Thank you,
Claudiu Beznea

Claudiu Beznea (2):
  dt-bindings: mchp-eic: add bindings
  irqchip/mchp-eic: add support

 .../interrupt-controller/mchp,eic.yaml        |  74 ++++
 MAINTAINERS                                   |   6 +
 drivers/irqchip/Kconfig                       |   7 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-mchp-eic.c                | 350 ++++++++++++++++++
 5 files changed, 438 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
 create mode 100644 drivers/irqchip/irq-mchp-eic.c

-- 
2.25.1

[PATCH 0/2] irqchip/mchp-eic: add driver for Microchip EIC

[Claudiu Beznea]

Hi,

This patch adds support for Microchip External Interrupt Controller
present on SAMA7G5. The controller supports for 2 external interrupt
lines, glitch filter capabilities and is connected to GIC as follows:

pinX   +------+ EXT_IRQ0 +------+ int 153 (for pinX) +------+
------>|      |--------->|      |------------------->|      |
pinY   | PIO  | EXT_IRQ1 | EIC  | int 154 (for pinY) | GIC  |
------>|      |--------->|      |------------------->|      |
       +------+          +------+                    +------+

where PIO is the pin controller.

Thank you,
Claudiu Beznea

Claudiu Beznea (2):
  dt-bindings: mchp-eic: add bindings
  irqchip/mchp-eic: add support

 .../interrupt-controller/mchp,eic.yaml        |  74 ++++
 MAINTAINERS                                   |   6 +
 drivers/irqchip/Kconfig                       |   7 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-mchp-eic.c                | 350 ++++++++++++++++++
 5 files changed, 438 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
 create mode 100644 drivers/irqchip/irq-mchp-eic.c

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] dt-bindings: mchp-eic: add bindings

[Claudiu Beznea]

Add DT bindings for Microchip External Interrupt Controller.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 .../interrupt-controller/mchp,eic.yaml        | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
new file mode 100644
index 000000000000..5a927817aa7d
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/mchp,eic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip External Interrupt Controller
+
+maintainers:
+  - Claudiu Beznea <claudiu.beznea@microchip.com>
+
+description:
+  This interrupt controller is found in Microchip SoCs (SAMA7G5) and provides
+  support for handling up to 2 external interrupt lines.
+
+properties:
+  compatible:
+    enum:
+      - microchip,sama7g5-eic
+
+  reg:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  '#interrupt-cells':
+    const: 3
+    description:
+      The first cell is the input IRQ number (between 0 and 1), the second cell
+      is the trigger type as defined in interrupt.txt present in this directory
+      and the third cell is the glitch filter (1, 2, 4, 8) in clock cycles
+
+  'interrupts':
+    description: |
+      Contains the GIC SPI IRQs mapped to the external interrupt lines. They
+      should be specified sequentially from output 0 to output 1.
+    minItems: 2
+    maxItems: 2
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: pclk
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - '#interrupt-cells'
+  - 'interrupts'
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/at91.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    eic: eic@e1628000 {
+      compatible = "microchip,sama7g5-eic";
+      reg = <0xe1628000 0x100>;
+      interrupt-parent = <&gic>;
+      interrupt-controller;
+      #interrupt-cells = <3>;
+      interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>,
+                   <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>;
+      clocks = <&pmc PMC_TYPE_PERIPHERAL 37>;
+      clock-names = "pclk";
+    };
+
+...
-- 
2.25.1

[PATCH 1/2] dt-bindings: mchp-eic: add bindings

[Claudiu Beznea]

Add DT bindings for Microchip External Interrupt Controller.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 .../interrupt-controller/mchp,eic.yaml        | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
new file mode 100644
index 000000000000..5a927817aa7d
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/mchp,eic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip External Interrupt Controller
+
+maintainers:
+  - Claudiu Beznea <claudiu.beznea@microchip.com>
+
+description:
+  This interrupt controller is found in Microchip SoCs (SAMA7G5) and provides
+  support for handling up to 2 external interrupt lines.
+
+properties:
+  compatible:
+    enum:
+      - microchip,sama7g5-eic
+
+  reg:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  '#interrupt-cells':
+    const: 3
+    description:
+      The first cell is the input IRQ number (between 0 and 1), the second cell
+      is the trigger type as defined in interrupt.txt present in this directory
+      and the third cell is the glitch filter (1, 2, 4, 8) in clock cycles
+
+  'interrupts':
+    description: |
+      Contains the GIC SPI IRQs mapped to the external interrupt lines. They
+      should be specified sequentially from output 0 to output 1.
+    minItems: 2
+    maxItems: 2
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: pclk
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - '#interrupt-cells'
+  - 'interrupts'
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/at91.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    eic: eic@e1628000 {
+      compatible = "microchip,sama7g5-eic";
+      reg = <0xe1628000 0x100>;
+      interrupt-parent = <&gic>;
+      interrupt-controller;
+      #interrupt-cells = <3>;
+      interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>,
+                   <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>;
+      clocks = <&pmc PMC_TYPE_PERIPHERAL 37>;
+      clock-names = "pclk";
+    };
+
+...
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] dt-bindings: mchp-eic: add bindings

[Marc Zyngier]

On Tue, 02 Mar 2021 10:28:45 +0000,
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
> Add DT bindings for Microchip External Interrupt Controller.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  .../interrupt-controller/mchp,eic.yaml        | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> new file mode 100644
> index 000000000000..5a927817aa7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/mchp,eic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip External Interrupt Controller
> +
> +maintainers:
> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
> +
> +description:
> +  This interrupt controller is found in Microchip SoCs (SAMA7G5) and provides
> +  support for handling up to 2 external interrupt lines.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,sama7g5-eic
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 3
> +    description:
> +      The first cell is the input IRQ number (between 0 and 1), the second cell
> +      is the trigger type as defined in interrupt.txt present in this directory
> +      and the third cell is the glitch filter (1, 2, 4, 8) in clock cycles

This last parameter looks like a very bad idea. How do you plan for
that to be used? Which clock cycles?

In any case, I don't think it should be part of the interrupt
descriptor, but provided as a static configuration at the interrupt
controller level itself.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/2] dt-bindings: mchp-eic: add bindings

[Marc Zyngier]

On Tue, 02 Mar 2021 10:28:45 +0000,
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
> Add DT bindings for Microchip External Interrupt Controller.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  .../interrupt-controller/mchp,eic.yaml        | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> new file mode 100644
> index 000000000000..5a927817aa7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/mchp,eic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip External Interrupt Controller
> +
> +maintainers:
> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
> +
> +description:
> +  This interrupt controller is found in Microchip SoCs (SAMA7G5) and provides
> +  support for handling up to 2 external interrupt lines.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,sama7g5-eic
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 3
> +    description:
> +      The first cell is the input IRQ number (between 0 and 1), the second cell
> +      is the trigger type as defined in interrupt.txt present in this directory
> +      and the third cell is the glitch filter (1, 2, 4, 8) in clock cycles

This last parameter looks like a very bad idea. How do you plan for
that to be used? Which clock cycles?

In any case, I don't think it should be part of the interrupt
descriptor, but provided as a static configuration at the interrupt
controller level itself.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] dt-bindings: mchp-eic: add bindings

[Claudiu.Beznea]

On 02.03.2021 13:32, Marc Zyngier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, 02 Mar 2021 10:28:45 +0000,
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
>>
>> Add DT bindings for Microchip External Interrupt Controller.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  .../interrupt-controller/mchp,eic.yaml        | 74 +++++++++++++++++++
>>  1 file changed, 74 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
>> new file mode 100644
>> index 000000000000..5a927817aa7d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
>> @@ -0,0 +1,74 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/interrupt-controller/mchp,eic.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip External Interrupt Controller
>> +
>> +maintainers:
>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>> +
>> +description:
>> +  This interrupt controller is found in Microchip SoCs (SAMA7G5) and provides
>> +  support for handling up to 2 external interrupt lines.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - microchip,sama7g5-eic
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupt-controller: true
>> +
>> +  '#interrupt-cells':
>> +    const: 3
>> +    description:
>> +      The first cell is the input IRQ number (between 0 and 1), the second cell
>> +      is the trigger type as defined in interrupt.txt present in this directory
>> +      and the third cell is the glitch filter (1, 2, 4, 8) in clock cycles
> 
> This last parameter looks like a very bad idea. How do you plan for
> that to be used? Which clock cycles?

I was in balance weter I should add this parameter here or not. I will
remove it.

> 
> In any case, I don't think it should be part of the interrupt
> descriptor, but provided as a static configuration at the interrupt
> controller level itself.

OK.

> 
> Thanks,
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.
> 

Re: [PATCH 1/2] dt-bindings: mchp-eic: add bindings

[Claudiu.Beznea]

On 02.03.2021 13:32, Marc Zyngier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, 02 Mar 2021 10:28:45 +0000,
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
>>
>> Add DT bindings for Microchip External Interrupt Controller.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  .../interrupt-controller/mchp,eic.yaml        | 74 +++++++++++++++++++
>>  1 file changed, 74 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
>> new file mode 100644
>> index 000000000000..5a927817aa7d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
>> @@ -0,0 +1,74 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/interrupt-controller/mchp,eic.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip External Interrupt Controller
>> +
>> +maintainers:
>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>> +
>> +description:
>> +  This interrupt controller is found in Microchip SoCs (SAMA7G5) and provides
>> +  support for handling up to 2 external interrupt lines.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - microchip,sama7g5-eic
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupt-controller: true
>> +
>> +  '#interrupt-cells':
>> +    const: 3
>> +    description:
>> +      The first cell is the input IRQ number (between 0 and 1), the second cell
>> +      is the trigger type as defined in interrupt.txt present in this directory
>> +      and the third cell is the glitch filter (1, 2, 4, 8) in clock cycles
> 
> This last parameter looks like a very bad idea. How do you plan for
> that to be used? Which clock cycles?

I was in balance weter I should add this parameter here or not. I will
remove it.

> 
> In any case, I don't think it should be part of the interrupt
> descriptor, but provided as a static configuration at the interrupt
> controller level itself.

OK.

> 
> Thanks,
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] dt-bindings: mchp-eic: add bindings

[Nicolas Ferre]

On 02/03/2021 at 11:28, Claudiu Beznea wrote:
> Add DT bindings for Microchip External Interrupt Controller.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>   .../interrupt-controller/mchp,eic.yaml        | 74 +++++++++++++++++++

Nitpicking: use full vendor name in binding file name: microchip,eic.yaml

>   1 file changed, 74 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
  [..]

Regards,
-- 
Nicolas Ferre

Re: [PATCH 1/2] dt-bindings: mchp-eic: add bindings

[Nicolas Ferre]

On 02/03/2021 at 11:28, Claudiu Beznea wrote:
> Add DT bindings for Microchip External Interrupt Controller.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>   .../interrupt-controller/mchp,eic.yaml        | 74 +++++++++++++++++++

Nitpicking: use full vendor name in binding file name: microchip,eic.yaml

>   1 file changed, 74 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
  [..]

Regards,
-- 
Nicolas Ferre

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] dt-bindings: mchp-eic: add bindings

[Rob Herring]

On Tue, Mar 02, 2021 at 12:28:45PM +0200, Claudiu Beznea wrote:
> Add DT bindings for Microchip External Interrupt Controller.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  .../interrupt-controller/mchp,eic.yaml        | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> new file mode 100644
> index 000000000000..5a927817aa7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/mchp,eic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip External Interrupt Controller
> +
> +maintainers:
> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
> +
> +description:
> +  This interrupt controller is found in Microchip SoCs (SAMA7G5) and provides
> +  support for handling up to 2 external interrupt lines.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,sama7g5-eic
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 3
> +    description:
> +      The first cell is the input IRQ number (between 0 and 1), the second cell
> +      is the trigger type as defined in interrupt.txt present in this directory
> +      and the third cell is the glitch filter (1, 2, 4, 8) in clock cycles
> +
> +  'interrupts':

Don't need quotes here.

> +    description: |
> +      Contains the GIC SPI IRQs mapped to the external interrupt lines. They
> +      should be specified sequentially from output 0 to output 1.
> +    minItems: 2
> +    maxItems: 2
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: pclk
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - '#interrupt-cells'
> +  - 'interrupts'

Or here.

> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/at91.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    eic: eic@e1628000 {

interrupt-controller@...

> +      compatible = "microchip,sama7g5-eic";
> +      reg = <0xe1628000 0x100>;
> +      interrupt-parent = <&gic>;
> +      interrupt-controller;
> +      #interrupt-cells = <3>;
> +      interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>,
> +                   <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>;
> +      clocks = <&pmc PMC_TYPE_PERIPHERAL 37>;
> +      clock-names = "pclk";
> +    };
> +
> +...
> -- 
> 2.25.1
> 

Re: [PATCH 1/2] dt-bindings: mchp-eic: add bindings

[Rob Herring]

On Tue, Mar 02, 2021 at 12:28:45PM +0200, Claudiu Beznea wrote:
> Add DT bindings for Microchip External Interrupt Controller.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  .../interrupt-controller/mchp,eic.yaml        | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> new file mode 100644
> index 000000000000..5a927817aa7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mchp,eic.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/mchp,eic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip External Interrupt Controller
> +
> +maintainers:
> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
> +
> +description:
> +  This interrupt controller is found in Microchip SoCs (SAMA7G5) and provides
> +  support for handling up to 2 external interrupt lines.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,sama7g5-eic
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 3
> +    description:
> +      The first cell is the input IRQ number (between 0 and 1), the second cell
> +      is the trigger type as defined in interrupt.txt present in this directory
> +      and the third cell is the glitch filter (1, 2, 4, 8) in clock cycles
> +
> +  'interrupts':

Don't need quotes here.

> +    description: |
> +      Contains the GIC SPI IRQs mapped to the external interrupt lines. They
> +      should be specified sequentially from output 0 to output 1.
> +    minItems: 2
> +    maxItems: 2
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: pclk
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - '#interrupt-cells'
> +  - 'interrupts'

Or here.

> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/at91.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    eic: eic@e1628000 {

interrupt-controller@...

> +      compatible = "microchip,sama7g5-eic";
> +      reg = <0xe1628000 0x100>;
> +      interrupt-parent = <&gic>;
> +      interrupt-controller;
> +      #interrupt-cells = <3>;
> +      interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>,
> +                   <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>;
> +      clocks = <&pmc PMC_TYPE_PERIPHERAL 37>;
> +      clock-names = "pclk";
> +    };
> +
> +...
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] irqchip/mchp-eic: add support

[Claudiu Beznea]

Add support for Microchip External Interrupt Controller.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 MAINTAINERS                    |   6 +
 drivers/irqchip/Kconfig        |   7 +
 drivers/irqchip/Makefile       |   1 +
 drivers/irqchip/irq-mchp-eic.c | 350 +++++++++++++++++++++++++++++++++
 4 files changed, 364 insertions(+)
 create mode 100644 drivers/irqchip/irq-mchp-eic.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a008b70f3c16..ea99e5a7f519 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11585,6 +11585,12 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Supported
 F:	drivers/usb/gadget/udc/atmel_usba_udc.*
 
+MICROCHIP EIC DRIVER
+M:	Claudiu Beznea <claudiu.beznea@microchip.com>
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
+S:	Supported
+F:	drivers/irqchip/irq-mchp-eic.c
+
 MICROCHIP WILC1000 WIFI DRIVER
 M:	Ajay Singh <ajay.kathat@microchip.com>
 M:	Claudiu Beznea <claudiu.beznea@microchip.com>
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 2aa79c32ee22..7c7ca33c3f7d 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -596,4 +596,11 @@ config MST_IRQ
 	help
 	  Support MStar Interrupt Controller.
 
+config MCHP_EIC
+	bool "Microchip External Interrupt Controller"
+	depends on ARCH_AT91 || COMPILE_TEST
+	select IRQ_DOMAIN
+	help
+	  Support for Microchip External Interrupt Controller.
+
 endmenu
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 94c2885882ee..f3df6eb7c4c3 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -114,3 +114,4 @@ obj-$(CONFIG_LOONGSON_PCH_PIC)		+= irq-loongson-pch-pic.o
 obj-$(CONFIG_LOONGSON_PCH_MSI)		+= irq-loongson-pch-msi.o
 obj-$(CONFIG_MST_IRQ)			+= irq-mst-intc.o
 obj-$(CONFIG_SL28CPLD_INTC)		+= irq-sl28cpld.o
+obj-$(CONFIG_MCHP_EIC)			+= irq-mchp-eic.o
diff --git a/drivers/irqchip/irq-mchp-eic.c b/drivers/irqchip/irq-mchp-eic.c
new file mode 100644
index 000000000000..ec01877fc75f
--- /dev/null
+++ b/drivers/irqchip/irq-mchp-eic.c
@@ -0,0 +1,350 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Microchip External Interrupt Controller driver
+ *
+ * Copyright (C) 2021 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Claudiu Beznea <claudiu.beznea@microchip.com>
+ */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/irqchip/chained_irq.h>
+
+#define MCHP_EIC_GFCS			(0x0)
+#define MCHP_EIC_SCFG(x)		(0x4 + (x) * 0x4)
+#define MCHP_EIC_SCFG_FRZ		BIT(31)
+#define MCHP_EIC_SCFG_EN		BIT(16)
+#define MCHP_EIC_SCFG_LVL		BIT(9)
+#define MCHP_EIC_SCFG_POL		BIT(8)
+#define MCHP_EIC_SCFG_GFEN		BIT(4)
+#define MCHP_EIC_SCFG_GFSEL		GENMASK(1, 0)
+
+#define MCHP_EIC_NIRQ			(2)
+#define MCHP_EIC_DEFAULT_WAIT_US	(100)
+
+/**
+ * struct mchp_eic - EIC private data structure
+ * @base: base address
+ * @dev: eic device
+ * @clk: peripheral clock
+ * @chip: irq chip
+ * @domain: irq domain
+ * @irqs: irqs b/w eic and gic
+ * @scfg: backup for scfg registers (necessary for backup and self-refresh mode)
+ * @wakeup_source: wakeup source mask
+ */
+struct mchp_eic {
+	void __iomem *base;
+	struct device *dev;
+	struct clk *clk;
+	struct irq_chip *chip;
+	struct irq_domain *domain;
+	u32 irqs[MCHP_EIC_NIRQ];
+	u32 scfg[MCHP_EIC_NIRQ];
+	u32 wakeup_source;
+};
+
+static void mchp_eic_wait_rdy(struct mchp_eic *eic, unsigned int hwirq,
+			      int delay_us)
+{
+	unsigned int tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));
+
+	while (delay_us >= 0 && tmp & BIT(hwirq)) {
+		udelay(1);
+		delay_us -= 1;
+		tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));
+	}
+
+	if (delay_us < 0)
+		dev_err(eic->dev, "Failed to configure IRQ=%u!\n", hwirq);
+}
+
+static void mchp_eic_irq_mask(struct irq_data *d)
+{
+	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
+	unsigned int tmp;
+
+	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
+	tmp &= ~MCHP_EIC_SCFG_EN;
+	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
+}
+
+static void mchp_eic_irq_unmask(struct irq_data *d)
+{
+	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
+	unsigned int tmp;
+
+	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
+	tmp |= MCHP_EIC_SCFG_EN;
+	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
+	mchp_eic_wait_rdy(eic, d->hwirq, MCHP_EIC_DEFAULT_WAIT_US);
+}
+
+static int mchp_eic_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
+	unsigned int tmp;
+
+	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
+	tmp &= ~(MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL);
+	switch (type) {
+	case IRQ_TYPE_LEVEL_HIGH:
+		irq_set_handler_locked(d, handle_level_irq);
+		tmp |= MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		irq_set_handler_locked(d, handle_level_irq);
+		tmp |= MCHP_EIC_SCFG_LVL;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		irq_set_handler_locked(d, handle_edge_irq);
+		tmp |= MCHP_EIC_SCFG_POL;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		irq_set_handler_locked(d, handle_edge_irq);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
+
+	return 0;
+}
+
+static void mchp_eic_irq_ack(struct irq_data *d)
+{
+	/* Nothing to do here. */
+}
+
+static int mchp_eic_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
+
+	irq_set_irq_wake(eic->irqs[d->hwirq], on);
+	if (on)
+		eic->wakeup_source |= BIT(d->hwirq);
+	else
+		eic->wakeup_source &= BIT(d->hwirq);
+
+	return 0;
+}
+
+static struct irq_chip mchp_eic_chip = {
+	.name		= "eic-irq",
+	.flags		= IRQCHIP_MASK_ON_SUSPEND,
+	.irq_mask	= mchp_eic_irq_mask,
+	.irq_unmask	= mchp_eic_irq_unmask,
+	.irq_set_type	= mchp_eic_irq_set_type,
+	.irq_ack	= mchp_eic_irq_ack,
+	.irq_set_wake	= mchp_eic_irq_set_wake,
+};
+
+static int mchp_eic_domain_map(struct irq_domain *d, unsigned int virq,
+			       irq_hw_number_t hw)
+{
+	struct mchp_eic *eic = d->host_data;
+
+	irq_set_chip_data(virq, eic);
+	irq_set_chip_and_handler(virq, eic->chip, handle_bad_irq);
+	irq_set_probe(virq);
+
+	return 0;
+}
+
+static int mchp_eic_domain_xlate(struct irq_domain *d, struct device_node *node,
+				 const u32 *intspec, unsigned int intsize,
+				 irq_hw_number_t *out_hwirq,
+				 unsigned int *out_type)
+{
+	struct mchp_eic *eic = d->host_data;
+	unsigned int tmp, i;
+
+	if (WARN_ON(intsize < 2))
+		return -EINVAL;
+
+	if (WARN_ON(intspec[0] > MCHP_EIC_NIRQ))
+		return -EINVAL;
+
+	*out_hwirq = intspec[0];
+	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
+
+	if (intsize == 3) {
+		/* Validate against available filters. */
+		for (i = 0; i <= MCHP_EIC_SCFG_GFSEL; i++) {
+			if (intspec[2] == (2 << i))
+				break;
+		}
+		if (i == MCHP_EIC_SCFG_GFSEL + 1)
+			return -EINVAL;
+
+		/* Enable glitch filter. */
+		tmp = readl(eic->base + MCHP_EIC_SCFG(*out_hwirq));
+		tmp |= MCHP_EIC_SCFG_GFEN;
+		tmp &= ~MCHP_EIC_SCFG_GFSEL;
+		tmp |= i;
+		writel(tmp, eic->base + MCHP_EIC_SCFG(*out_hwirq));
+
+		/* Wait for glitch filter to be enabled. */
+		mchp_eic_wait_rdy(eic, *out_hwirq, MCHP_EIC_DEFAULT_WAIT_US);
+	}
+
+	return 0;
+}
+
+static const struct irq_domain_ops mchp_eic_domain_ops = {
+	.map		= mchp_eic_domain_map,
+	.xlate		= mchp_eic_domain_xlate,
+};
+
+static void mchp_eic_irq_handler(struct irq_desc *desc)
+{
+	struct mchp_eic *eic = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned int hwirq, irq = irq_desc_get_irq(desc);
+
+	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
+		if (eic->irqs[hwirq] == irq)
+			break;
+	}
+	if (hwirq == MCHP_EIC_NIRQ)
+		return;
+
+	chained_irq_enter(chip, desc);
+	generic_handle_irq(irq_find_mapping(eic->domain, hwirq));
+	chained_irq_exit(chip, desc);
+}
+
+static int mchp_eic_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct mchp_eic *eic;
+	int ret, i;
+
+	eic = devm_kzalloc(&pdev->dev, sizeof(*eic), GFP_KERNEL);
+	if (!eic)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	eic->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(eic->base))
+		return PTR_ERR(eic->base);
+
+	eic->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(eic->clk))
+		return PTR_ERR(eic->clk);
+
+	ret = clk_prepare_enable(eic->clk);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < MCHP_EIC_NIRQ; i++) {
+		/* Disable it, if any. */
+		writel(0UL, eic->base + MCHP_EIC_SCFG(i));
+
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
+		if (!res) {
+			ret = dev_err_probe(&pdev->dev, -EINVAL,
+					    "Failed to get hwirq=%d\n", i);
+			goto clk_unprepare;
+		}
+
+		eic->irqs[i] = res->start;
+		irq_set_chained_handler(res->start, mchp_eic_irq_handler);
+		irq_set_handler_data(res->start, eic);
+	}
+
+	eic->dev = &pdev->dev;
+	eic->chip = &mchp_eic_chip;
+
+	eic->domain = irq_domain_add_linear(pdev->dev.of_node, MCHP_EIC_NIRQ,
+					    &mchp_eic_domain_ops, eic);
+	if (!eic->domain) {
+		ret = dev_err_probe(&pdev->dev, -ENODEV,
+				    "Failed to regiter irq domain!\n");
+		goto clk_unprepare;
+	}
+	eic->domain->name = "eic";
+
+	platform_set_drvdata(pdev, eic);
+
+	dev_info(&pdev->dev, "EIC registered, nr_irqs %u\n", MCHP_EIC_NIRQ);
+
+	return 0;
+
+clk_unprepare:
+	clk_disable_unprepare(eic->clk);
+	return ret;
+}
+
+static int mchp_eic_remove(struct platform_device *pdev)
+{
+	struct mchp_eic *eic = platform_get_drvdata(pdev);
+
+	irq_domain_remove(eic->domain);
+	clk_disable_unprepare(eic->clk);
+
+	return 0;
+}
+
+static int __maybe_unused mchp_eic_suspend(struct device *dev)
+{
+	struct mchp_eic *eic = dev_get_drvdata(dev);
+	unsigned int hwirq;
+
+	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++)
+		eic->scfg[hwirq] = readl(eic->base + MCHP_EIC_SCFG(hwirq));
+
+	if (!eic->wakeup_source)
+		clk_disable_unprepare(eic->clk);
+
+	return 0;
+}
+
+static int __maybe_unused mchp_eic_resume(struct device *dev)
+{
+	struct mchp_eic *eic = dev_get_drvdata(dev);
+	unsigned int hwirq;
+
+	if (!eic->wakeup_source)
+		clk_prepare_enable(eic->clk);
+
+	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
+		writel(eic->scfg[hwirq], eic->base + MCHP_EIC_SCFG(hwirq));
+		mchp_eic_wait_rdy(eic, hwirq, MCHP_EIC_DEFAULT_WAIT_US);
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops mchp_eic_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mchp_eic_suspend, mchp_eic_resume)
+};
+
+static const struct of_device_id mchp_eic_dt_ids[] = {
+	{ .compatible = "microchip,sama7g5-eic", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mchp_eic_dt_ids);
+
+static struct platform_driver mchp_eic_device_driver = {
+	.probe		= mchp_eic_probe,
+	.remove		= mchp_eic_remove,
+	.driver		= {
+		.name = "mchp-eic",
+		.of_match_table = of_match_ptr(mchp_eic_dt_ids),
+		.pm = pm_ptr(&mchp_eic_pm_ops),
+	},
+};
+module_platform_driver(mchp_eic_device_driver);
+
+MODULE_DESCRIPTION("Microchip External Interrupt Controller");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea@microchip.com>");
-- 
2.25.1

[PATCH 2/2] irqchip/mchp-eic: add support

[Claudiu Beznea]

Add support for Microchip External Interrupt Controller.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 MAINTAINERS                    |   6 +
 drivers/irqchip/Kconfig        |   7 +
 drivers/irqchip/Makefile       |   1 +
 drivers/irqchip/irq-mchp-eic.c | 350 +++++++++++++++++++++++++++++++++
 4 files changed, 364 insertions(+)
 create mode 100644 drivers/irqchip/irq-mchp-eic.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a008b70f3c16..ea99e5a7f519 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11585,6 +11585,12 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Supported
 F:	drivers/usb/gadget/udc/atmel_usba_udc.*
 
+MICROCHIP EIC DRIVER
+M:	Claudiu Beznea <claudiu.beznea@microchip.com>
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
+S:	Supported
+F:	drivers/irqchip/irq-mchp-eic.c
+
 MICROCHIP WILC1000 WIFI DRIVER
 M:	Ajay Singh <ajay.kathat@microchip.com>
 M:	Claudiu Beznea <claudiu.beznea@microchip.com>
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 2aa79c32ee22..7c7ca33c3f7d 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -596,4 +596,11 @@ config MST_IRQ
 	help
 	  Support MStar Interrupt Controller.
 
+config MCHP_EIC
+	bool "Microchip External Interrupt Controller"
+	depends on ARCH_AT91 || COMPILE_TEST
+	select IRQ_DOMAIN
+	help
+	  Support for Microchip External Interrupt Controller.
+
 endmenu
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 94c2885882ee..f3df6eb7c4c3 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -114,3 +114,4 @@ obj-$(CONFIG_LOONGSON_PCH_PIC)		+= irq-loongson-pch-pic.o
 obj-$(CONFIG_LOONGSON_PCH_MSI)		+= irq-loongson-pch-msi.o
 obj-$(CONFIG_MST_IRQ)			+= irq-mst-intc.o
 obj-$(CONFIG_SL28CPLD_INTC)		+= irq-sl28cpld.o
+obj-$(CONFIG_MCHP_EIC)			+= irq-mchp-eic.o
diff --git a/drivers/irqchip/irq-mchp-eic.c b/drivers/irqchip/irq-mchp-eic.c
new file mode 100644
index 000000000000..ec01877fc75f
--- /dev/null
+++ b/drivers/irqchip/irq-mchp-eic.c
@@ -0,0 +1,350 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Microchip External Interrupt Controller driver
+ *
+ * Copyright (C) 2021 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Claudiu Beznea <claudiu.beznea@microchip.com>
+ */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/irqchip/chained_irq.h>
+
+#define MCHP_EIC_GFCS			(0x0)
+#define MCHP_EIC_SCFG(x)		(0x4 + (x) * 0x4)
+#define MCHP_EIC_SCFG_FRZ		BIT(31)
+#define MCHP_EIC_SCFG_EN		BIT(16)
+#define MCHP_EIC_SCFG_LVL		BIT(9)
+#define MCHP_EIC_SCFG_POL		BIT(8)
+#define MCHP_EIC_SCFG_GFEN		BIT(4)
+#define MCHP_EIC_SCFG_GFSEL		GENMASK(1, 0)
+
+#define MCHP_EIC_NIRQ			(2)
+#define MCHP_EIC_DEFAULT_WAIT_US	(100)
+
+/**
+ * struct mchp_eic - EIC private data structure
+ * @base: base address
+ * @dev: eic device
+ * @clk: peripheral clock
+ * @chip: irq chip
+ * @domain: irq domain
+ * @irqs: irqs b/w eic and gic
+ * @scfg: backup for scfg registers (necessary for backup and self-refresh mode)
+ * @wakeup_source: wakeup source mask
+ */
+struct mchp_eic {
+	void __iomem *base;
+	struct device *dev;
+	struct clk *clk;
+	struct irq_chip *chip;
+	struct irq_domain *domain;
+	u32 irqs[MCHP_EIC_NIRQ];
+	u32 scfg[MCHP_EIC_NIRQ];
+	u32 wakeup_source;
+};
+
+static void mchp_eic_wait_rdy(struct mchp_eic *eic, unsigned int hwirq,
+			      int delay_us)
+{
+	unsigned int tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));
+
+	while (delay_us >= 0 && tmp & BIT(hwirq)) {
+		udelay(1);
+		delay_us -= 1;
+		tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));
+	}
+
+	if (delay_us < 0)
+		dev_err(eic->dev, "Failed to configure IRQ=%u!\n", hwirq);
+}
+
+static void mchp_eic_irq_mask(struct irq_data *d)
+{
+	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
+	unsigned int tmp;
+
+	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
+	tmp &= ~MCHP_EIC_SCFG_EN;
+	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
+}
+
+static void mchp_eic_irq_unmask(struct irq_data *d)
+{
+	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
+	unsigned int tmp;
+
+	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
+	tmp |= MCHP_EIC_SCFG_EN;
+	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
+	mchp_eic_wait_rdy(eic, d->hwirq, MCHP_EIC_DEFAULT_WAIT_US);
+}
+
+static int mchp_eic_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
+	unsigned int tmp;
+
+	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
+	tmp &= ~(MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL);
+	switch (type) {
+	case IRQ_TYPE_LEVEL_HIGH:
+		irq_set_handler_locked(d, handle_level_irq);
+		tmp |= MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		irq_set_handler_locked(d, handle_level_irq);
+		tmp |= MCHP_EIC_SCFG_LVL;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		irq_set_handler_locked(d, handle_edge_irq);
+		tmp |= MCHP_EIC_SCFG_POL;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		irq_set_handler_locked(d, handle_edge_irq);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
+
+	return 0;
+}
+
+static void mchp_eic_irq_ack(struct irq_data *d)
+{
+	/* Nothing to do here. */
+}
+
+static int mchp_eic_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
+
+	irq_set_irq_wake(eic->irqs[d->hwirq], on);
+	if (on)
+		eic->wakeup_source |= BIT(d->hwirq);
+	else
+		eic->wakeup_source &= BIT(d->hwirq);
+
+	return 0;
+}
+
+static struct irq_chip mchp_eic_chip = {
+	.name		= "eic-irq",
+	.flags		= IRQCHIP_MASK_ON_SUSPEND,
+	.irq_mask	= mchp_eic_irq_mask,
+	.irq_unmask	= mchp_eic_irq_unmask,
+	.irq_set_type	= mchp_eic_irq_set_type,
+	.irq_ack	= mchp_eic_irq_ack,
+	.irq_set_wake	= mchp_eic_irq_set_wake,
+};
+
+static int mchp_eic_domain_map(struct irq_domain *d, unsigned int virq,
+			       irq_hw_number_t hw)
+{
+	struct mchp_eic *eic = d->host_data;
+
+	irq_set_chip_data(virq, eic);
+	irq_set_chip_and_handler(virq, eic->chip, handle_bad_irq);
+	irq_set_probe(virq);
+
+	return 0;
+}
+
+static int mchp_eic_domain_xlate(struct irq_domain *d, struct device_node *node,
+				 const u32 *intspec, unsigned int intsize,
+				 irq_hw_number_t *out_hwirq,
+				 unsigned int *out_type)
+{
+	struct mchp_eic *eic = d->host_data;
+	unsigned int tmp, i;
+
+	if (WARN_ON(intsize < 2))
+		return -EINVAL;
+
+	if (WARN_ON(intspec[0] > MCHP_EIC_NIRQ))
+		return -EINVAL;
+
+	*out_hwirq = intspec[0];
+	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
+
+	if (intsize == 3) {
+		/* Validate against available filters. */
+		for (i = 0; i <= MCHP_EIC_SCFG_GFSEL; i++) {
+			if (intspec[2] == (2 << i))
+				break;
+		}
+		if (i == MCHP_EIC_SCFG_GFSEL + 1)
+			return -EINVAL;
+
+		/* Enable glitch filter. */
+		tmp = readl(eic->base + MCHP_EIC_SCFG(*out_hwirq));
+		tmp |= MCHP_EIC_SCFG_GFEN;
+		tmp &= ~MCHP_EIC_SCFG_GFSEL;
+		tmp |= i;
+		writel(tmp, eic->base + MCHP_EIC_SCFG(*out_hwirq));
+
+		/* Wait for glitch filter to be enabled. */
+		mchp_eic_wait_rdy(eic, *out_hwirq, MCHP_EIC_DEFAULT_WAIT_US);
+	}
+
+	return 0;
+}
+
+static const struct irq_domain_ops mchp_eic_domain_ops = {
+	.map		= mchp_eic_domain_map,
+	.xlate		= mchp_eic_domain_xlate,
+};
+
+static void mchp_eic_irq_handler(struct irq_desc *desc)
+{
+	struct mchp_eic *eic = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned int hwirq, irq = irq_desc_get_irq(desc);
+
+	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
+		if (eic->irqs[hwirq] == irq)
+			break;
+	}
+	if (hwirq == MCHP_EIC_NIRQ)
+		return;
+
+	chained_irq_enter(chip, desc);
+	generic_handle_irq(irq_find_mapping(eic->domain, hwirq));
+	chained_irq_exit(chip, desc);
+}
+
+static int mchp_eic_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct mchp_eic *eic;
+	int ret, i;
+
+	eic = devm_kzalloc(&pdev->dev, sizeof(*eic), GFP_KERNEL);
+	if (!eic)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	eic->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(eic->base))
+		return PTR_ERR(eic->base);
+
+	eic->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(eic->clk))
+		return PTR_ERR(eic->clk);
+
+	ret = clk_prepare_enable(eic->clk);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < MCHP_EIC_NIRQ; i++) {
+		/* Disable it, if any. */
+		writel(0UL, eic->base + MCHP_EIC_SCFG(i));
+
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
+		if (!res) {
+			ret = dev_err_probe(&pdev->dev, -EINVAL,
+					    "Failed to get hwirq=%d\n", i);
+			goto clk_unprepare;
+		}
+
+		eic->irqs[i] = res->start;
+		irq_set_chained_handler(res->start, mchp_eic_irq_handler);
+		irq_set_handler_data(res->start, eic);
+	}
+
+	eic->dev = &pdev->dev;
+	eic->chip = &mchp_eic_chip;
+
+	eic->domain = irq_domain_add_linear(pdev->dev.of_node, MCHP_EIC_NIRQ,
+					    &mchp_eic_domain_ops, eic);
+	if (!eic->domain) {
+		ret = dev_err_probe(&pdev->dev, -ENODEV,
+				    "Failed to regiter irq domain!\n");
+		goto clk_unprepare;
+	}
+	eic->domain->name = "eic";
+
+	platform_set_drvdata(pdev, eic);
+
+	dev_info(&pdev->dev, "EIC registered, nr_irqs %u\n", MCHP_EIC_NIRQ);
+
+	return 0;
+
+clk_unprepare:
+	clk_disable_unprepare(eic->clk);
+	return ret;
+}
+
+static int mchp_eic_remove(struct platform_device *pdev)
+{
+	struct mchp_eic *eic = platform_get_drvdata(pdev);
+
+	irq_domain_remove(eic->domain);
+	clk_disable_unprepare(eic->clk);
+
+	return 0;
+}
+
+static int __maybe_unused mchp_eic_suspend(struct device *dev)
+{
+	struct mchp_eic *eic = dev_get_drvdata(dev);
+	unsigned int hwirq;
+
+	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++)
+		eic->scfg[hwirq] = readl(eic->base + MCHP_EIC_SCFG(hwirq));
+
+	if (!eic->wakeup_source)
+		clk_disable_unprepare(eic->clk);
+
+	return 0;
+}
+
+static int __maybe_unused mchp_eic_resume(struct device *dev)
+{
+	struct mchp_eic *eic = dev_get_drvdata(dev);
+	unsigned int hwirq;
+
+	if (!eic->wakeup_source)
+		clk_prepare_enable(eic->clk);
+
+	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
+		writel(eic->scfg[hwirq], eic->base + MCHP_EIC_SCFG(hwirq));
+		mchp_eic_wait_rdy(eic, hwirq, MCHP_EIC_DEFAULT_WAIT_US);
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops mchp_eic_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mchp_eic_suspend, mchp_eic_resume)
+};
+
+static const struct of_device_id mchp_eic_dt_ids[] = {
+	{ .compatible = "microchip,sama7g5-eic", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mchp_eic_dt_ids);
+
+static struct platform_driver mchp_eic_device_driver = {
+	.probe		= mchp_eic_probe,
+	.remove		= mchp_eic_remove,
+	.driver		= {
+		.name = "mchp-eic",
+		.of_match_table = of_match_ptr(mchp_eic_dt_ids),
+		.pm = pm_ptr(&mchp_eic_pm_ops),
+	},
+};
+module_platform_driver(mchp_eic_device_driver);
+
+MODULE_DESCRIPTION("Microchip External Interrupt Controller");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea@microchip.com>");
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] irqchip/mchp-eic: add support

[Marc Zyngier]

On Tue, 02 Mar 2021 10:28:46 +0000,
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
> Add support for Microchip External Interrupt Controller.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  MAINTAINERS                    |   6 +
>  drivers/irqchip/Kconfig        |   7 +
>  drivers/irqchip/Makefile       |   1 +
>  drivers/irqchip/irq-mchp-eic.c | 350 +++++++++++++++++++++++++++++++++
>  4 files changed, 364 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mchp-eic.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a008b70f3c16..ea99e5a7f519 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11585,6 +11585,12 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>  S:	Supported
>  F:	drivers/usb/gadget/udc/atmel_usba_udc.*
>  
> +MICROCHIP EIC DRIVER
> +M:	Claudiu Beznea <claudiu.beznea@microchip.com>
> +L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> +S:	Supported
> +F:	drivers/irqchip/irq-mchp-eic.c
> +
>  MICROCHIP WILC1000 WIFI DRIVER
>  M:	Ajay Singh <ajay.kathat@microchip.com>
>  M:	Claudiu Beznea <claudiu.beznea@microchip.com>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 2aa79c32ee22..7c7ca33c3f7d 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -596,4 +596,11 @@ config MST_IRQ
>  	help
>  	  Support MStar Interrupt Controller.
>  
> +config MCHP_EIC
> +	bool "Microchip External Interrupt Controller"
> +	depends on ARCH_AT91 || COMPILE_TEST
> +	select IRQ_DOMAIN
> +	help
> +	  Support for Microchip External Interrupt Controller.
> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 94c2885882ee..f3df6eb7c4c3 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -114,3 +114,4 @@ obj-$(CONFIG_LOONGSON_PCH_PIC)		+= irq-loongson-pch-pic.o
>  obj-$(CONFIG_LOONGSON_PCH_MSI)		+= irq-loongson-pch-msi.o
>  obj-$(CONFIG_MST_IRQ)			+= irq-mst-intc.o
>  obj-$(CONFIG_SL28CPLD_INTC)		+= irq-sl28cpld.o
> +obj-$(CONFIG_MCHP_EIC)			+= irq-mchp-eic.o
> diff --git a/drivers/irqchip/irq-mchp-eic.c b/drivers/irqchip/irq-mchp-eic.c
> new file mode 100644
> index 000000000000..ec01877fc75f
> --- /dev/null
> +++ b/drivers/irqchip/irq-mchp-eic.c
> @@ -0,0 +1,350 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Microchip External Interrupt Controller driver
> + *
> + * Copyright (C) 2021 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Claudiu Beznea <claudiu.beznea@microchip.com>
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/irqchip/chained_irq.h>
> +
> +#define MCHP_EIC_GFCS			(0x0)
> +#define MCHP_EIC_SCFG(x)		(0x4 + (x) * 0x4)
> +#define MCHP_EIC_SCFG_FRZ		BIT(31)
> +#define MCHP_EIC_SCFG_EN		BIT(16)
> +#define MCHP_EIC_SCFG_LVL		BIT(9)
> +#define MCHP_EIC_SCFG_POL		BIT(8)
> +#define MCHP_EIC_SCFG_GFEN		BIT(4)
> +#define MCHP_EIC_SCFG_GFSEL		GENMASK(1, 0)
> +
> +#define MCHP_EIC_NIRQ			(2)
> +#define MCHP_EIC_DEFAULT_WAIT_US	(100)
> +
> +/**
> + * struct mchp_eic - EIC private data structure
> + * @base: base address
> + * @dev: eic device
> + * @clk: peripheral clock
> + * @chip: irq chip
> + * @domain: irq domain
> + * @irqs: irqs b/w eic and gic
> + * @scfg: backup for scfg registers (necessary for backup and self-refresh mode)
> + * @wakeup_source: wakeup source mask
> + */
> +struct mchp_eic {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct clk *clk;
> +	struct irq_chip *chip;
> +	struct irq_domain *domain;
> +	u32 irqs[MCHP_EIC_NIRQ];
> +	u32 scfg[MCHP_EIC_NIRQ];
> +	u32 wakeup_source;
> +};
> +
> +static void mchp_eic_wait_rdy(struct mchp_eic *eic, unsigned int hwirq,
> +			      int delay_us)
> +{
> +	unsigned int tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));

Please use relaxed accessors everywhere unless you need ordering with
things like DMA (you don't). Specially if oyu are going to use that
in loops as below.

> +
> +	while (delay_us >= 0 && tmp & BIT(hwirq)) {
> +		udelay(1);
> +		delay_us -= 1;
> +		tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));
> +	}
> +
> +	if (delay_us < 0)
> +		dev_err(eic->dev, "Failed to configure IRQ=%u!\n", hwirq);

Given that this is used on some interrupt paths, you should *at least*
rate-limit it.

> +}
> +
> +static void mchp_eic_irq_mask(struct irq_data *d)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +	unsigned int tmp;
> +
> +	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	tmp &= ~MCHP_EIC_SCFG_EN;
> +	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
> +}
> +
> +static void mchp_eic_irq_unmask(struct irq_data *d)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +	unsigned int tmp;
> +
> +	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	tmp |= MCHP_EIC_SCFG_EN;
> +	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	mchp_eic_wait_rdy(eic, d->hwirq, MCHP_EIC_DEFAULT_WAIT_US);
> +}
> +
> +static int mchp_eic_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +	unsigned int tmp;
> +
> +	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	tmp &= ~(MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL);
> +	switch (type) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		irq_set_handler_locked(d, handle_level_irq);
> +		tmp |= MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		irq_set_handler_locked(d, handle_level_irq);
> +		tmp |= MCHP_EIC_SCFG_LVL;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		irq_set_handler_locked(d, handle_edge_irq);
> +		tmp |= MCHP_EIC_SCFG_POL;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		irq_set_handler_locked(d, handle_edge_irq);

How about the parent interrupt? Is the output of this irqchip always
LEVEL_HIGH, and not influenced by the triggering of its input?

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
> +
> +	return 0;
> +}
> +
> +static void mchp_eic_irq_ack(struct irq_data *d)
> +{
> +	/* Nothing to do here. */
> +}

Are you sure there is really nothing to do, even for an edge
interrupt? This is pretty unlikely. The interrupt controller must be
told when to stop coalescing edges, and without an Ack, it can't.

> +
> +static int mchp_eic_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +
> +	irq_set_irq_wake(eic->irqs[d->hwirq], on);
> +	if (on)
> +		eic->wakeup_source |= BIT(d->hwirq);
> +	else
> +		eic->wakeup_source &= BIT(d->hwirq);
> +
> +	return 0;
> +}
> +
> +static struct irq_chip mchp_eic_chip = {
> +	.name		= "eic-irq",
> +	.flags		= IRQCHIP_MASK_ON_SUSPEND,
> +	.irq_mask	= mchp_eic_irq_mask,
> +	.irq_unmask	= mchp_eic_irq_unmask,
> +	.irq_set_type	= mchp_eic_irq_set_type,
> +	.irq_ack	= mchp_eic_irq_ack,
> +	.irq_set_wake	= mchp_eic_irq_set_wake,
> +};
> +
> +static int mchp_eic_domain_map(struct irq_domain *d, unsigned int virq,
> +			       irq_hw_number_t hw)
> +{
> +	struct mchp_eic *eic = d->host_data;
> +
> +	irq_set_chip_data(virq, eic);
> +	irq_set_chip_and_handler(virq, eic->chip, handle_bad_irq);
> +	irq_set_probe(virq);
> +
> +	return 0;
> +}
> +
> +static int mchp_eic_domain_xlate(struct irq_domain *d, struct device_node *node,
> +				 const u32 *intspec, unsigned int intsize,
> +				 irq_hw_number_t *out_hwirq,
> +				 unsigned int *out_type)
> +{
> +	struct mchp_eic *eic = d->host_data;
> +	unsigned int tmp, i;
> +
> +	if (WARN_ON(intsize < 2))
> +		return -EINVAL;
> +
> +	if (WARN_ON(intspec[0] > MCHP_EIC_NIRQ))
> +		return -EINVAL;
> +
> +	*out_hwirq = intspec[0];
> +	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
> +
> +	if (intsize == 3) {

What if intsize isn't 3?

> +		/* Validate against available filters. */
> +		for (i = 0; i <= MCHP_EIC_SCFG_GFSEL; i++) {
> +			if (intspec[2] == (2 << i))
> +				break;
> +		}

Why don't you just take the log2() of the input value, and use that?

> +		if (i == MCHP_EIC_SCFG_GFSEL + 1)
> +			return -EINVAL;
> +
> +		/* Enable glitch filter. */
> +		tmp = readl(eic->base + MCHP_EIC_SCFG(*out_hwirq));
> +		tmp |= MCHP_EIC_SCFG_GFEN;
> +		tmp &= ~MCHP_EIC_SCFG_GFSEL;
> +		tmp |= i;
> +		writel(tmp, eic->base + MCHP_EIC_SCFG(*out_hwirq));

Please move this out of the xlate path. For a start, xlate shouldn't
have any effect on the HW. Also, this should be a static configuration
(see my comment in the first patch).

> +
> +		/* Wait for glitch filter to be enabled. */
> +		mchp_eic_wait_rdy(eic, *out_hwirq, MCHP_EIC_DEFAULT_WAIT_US);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops mchp_eic_domain_ops = {
> +	.map		= mchp_eic_domain_map,
> +	.xlate		= mchp_eic_domain_xlate,
> +};
> +
> +static void mchp_eic_irq_handler(struct irq_desc *desc)
> +{
> +	struct mchp_eic *eic = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned int hwirq, irq = irq_desc_get_irq(desc);
> +
> +	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
> +		if (eic->irqs[hwirq] == irq)
> +			break;
> +	}

Why don't you just use the GIC input INTID as a base, and use the
offset from that base to compute the hwirq?

> +	if (hwirq == MCHP_EIC_NIRQ)
> +		return;
> +
> +	chained_irq_enter(chip, desc);
> +	generic_handle_irq(irq_find_mapping(eic->domain, hwirq));
> +	chained_irq_exit(chip, desc);

Same question as above: are you sure the output of this block is
always LEVEL_HIGH? because if it isn't this, should be treated as a
hierarchical irqchip, and not a chained one.

> +}
> +
> +static int mchp_eic_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct mchp_eic *eic;
> +	int ret, i;
> +
> +	eic = devm_kzalloc(&pdev->dev, sizeof(*eic), GFP_KERNEL);
> +	if (!eic)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	eic->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(eic->base))
> +		return PTR_ERR(eic->base);
> +
> +	eic->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(eic->clk))
> +		return PTR_ERR(eic->clk);
> +
> +	ret = clk_prepare_enable(eic->clk);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < MCHP_EIC_NIRQ; i++) {
> +		/* Disable it, if any. */
> +		writel(0UL, eic->base + MCHP_EIC_SCFG(i));
> +
> +		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
> +		if (!res) {
> +			ret = dev_err_probe(&pdev->dev, -EINVAL,
> +					    "Failed to get hwirq=%d\n", i);
> +			goto clk_unprepare;
> +		}
> +
> +		eic->irqs[i] = res->start;
> +		irq_set_chained_handler(res->start, mchp_eic_irq_handler);
> +		irq_set_handler_data(res->start, eic);
> +	}
> +
> +	eic->dev = &pdev->dev;
> +	eic->chip = &mchp_eic_chip;
> +
> +	eic->domain = irq_domain_add_linear(pdev->dev.of_node, MCHP_EIC_NIRQ,
> +					    &mchp_eic_domain_ops, eic);
> +	if (!eic->domain) {
> +		ret = dev_err_probe(&pdev->dev, -ENODEV,
> +				    "Failed to regiter irq domain!\n");
> +		goto clk_unprepare;
> +	}
> +	eic->domain->name = "eic";

Why this override? The only benefits for that are a memory leak and a
memory corruption if we ever try to free the domain...

> +
> +	platform_set_drvdata(pdev, eic);
> +
> +	dev_info(&pdev->dev, "EIC registered, nr_irqs %u\n", MCHP_EIC_NIRQ);
> +
> +	return 0;
> +
> +clk_unprepare:
> +	clk_disable_unprepare(eic->clk);
> +	return ret;
> +}
> +
> +static int mchp_eic_remove(struct platform_device *pdev)
> +{
> +	struct mchp_eic *eic = platform_get_drvdata(pdev);
> +
> +	irq_domain_remove(eic->domain);
> +	clk_disable_unprepare(eic->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mchp_eic_suspend(struct device *dev)
> +{
> +	struct mchp_eic *eic = dev_get_drvdata(dev);
> +	unsigned int hwirq;
> +
> +	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++)
> +		eic->scfg[hwirq] = readl(eic->base + MCHP_EIC_SCFG(hwirq));
> +
> +	if (!eic->wakeup_source)
> +		clk_disable_unprepare(eic->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mchp_eic_resume(struct device *dev)
> +{
> +	struct mchp_eic *eic = dev_get_drvdata(dev);
> +	unsigned int hwirq;
> +
> +	if (!eic->wakeup_source)
> +		clk_prepare_enable(eic->clk);
> +
> +	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
> +		writel(eic->scfg[hwirq], eic->base + MCHP_EIC_SCFG(hwirq));
> +		mchp_eic_wait_rdy(eic, hwirq, MCHP_EIC_DEFAULT_WAIT_US);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops mchp_eic_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mchp_eic_suspend, mchp_eic_resume)
> +};
> +
> +static const struct of_device_id mchp_eic_dt_ids[] = {
> +	{ .compatible = "microchip,sama7g5-eic", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, mchp_eic_dt_ids);
> +
> +static struct platform_driver mchp_eic_device_driver = {
> +	.probe		= mchp_eic_probe,
> +	.remove		= mchp_eic_remove,
> +	.driver		= {
> +		.name = "mchp-eic",
> +		.of_match_table = of_match_ptr(mchp_eic_dt_ids),
> +		.pm = pm_ptr(&mchp_eic_pm_ops),
> +	},
> +};
> +module_platform_driver(mchp_eic_device_driver);
> +
> +MODULE_DESCRIPTION("Microchip External Interrupt Controller");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea@microchip.com>");
> -- 
> 2.25.1
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 2/2] irqchip/mchp-eic: add support

[Marc Zyngier]

On Tue, 02 Mar 2021 10:28:46 +0000,
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
> Add support for Microchip External Interrupt Controller.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  MAINTAINERS                    |   6 +
>  drivers/irqchip/Kconfig        |   7 +
>  drivers/irqchip/Makefile       |   1 +
>  drivers/irqchip/irq-mchp-eic.c | 350 +++++++++++++++++++++++++++++++++
>  4 files changed, 364 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mchp-eic.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a008b70f3c16..ea99e5a7f519 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11585,6 +11585,12 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>  S:	Supported
>  F:	drivers/usb/gadget/udc/atmel_usba_udc.*
>  
> +MICROCHIP EIC DRIVER
> +M:	Claudiu Beznea <claudiu.beznea@microchip.com>
> +L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> +S:	Supported
> +F:	drivers/irqchip/irq-mchp-eic.c
> +
>  MICROCHIP WILC1000 WIFI DRIVER
>  M:	Ajay Singh <ajay.kathat@microchip.com>
>  M:	Claudiu Beznea <claudiu.beznea@microchip.com>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 2aa79c32ee22..7c7ca33c3f7d 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -596,4 +596,11 @@ config MST_IRQ
>  	help
>  	  Support MStar Interrupt Controller.
>  
> +config MCHP_EIC
> +	bool "Microchip External Interrupt Controller"
> +	depends on ARCH_AT91 || COMPILE_TEST
> +	select IRQ_DOMAIN
> +	help
> +	  Support for Microchip External Interrupt Controller.
> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 94c2885882ee..f3df6eb7c4c3 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -114,3 +114,4 @@ obj-$(CONFIG_LOONGSON_PCH_PIC)		+= irq-loongson-pch-pic.o
>  obj-$(CONFIG_LOONGSON_PCH_MSI)		+= irq-loongson-pch-msi.o
>  obj-$(CONFIG_MST_IRQ)			+= irq-mst-intc.o
>  obj-$(CONFIG_SL28CPLD_INTC)		+= irq-sl28cpld.o
> +obj-$(CONFIG_MCHP_EIC)			+= irq-mchp-eic.o
> diff --git a/drivers/irqchip/irq-mchp-eic.c b/drivers/irqchip/irq-mchp-eic.c
> new file mode 100644
> index 000000000000..ec01877fc75f
> --- /dev/null
> +++ b/drivers/irqchip/irq-mchp-eic.c
> @@ -0,0 +1,350 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Microchip External Interrupt Controller driver
> + *
> + * Copyright (C) 2021 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Claudiu Beznea <claudiu.beznea@microchip.com>
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/irqchip/chained_irq.h>
> +
> +#define MCHP_EIC_GFCS			(0x0)
> +#define MCHP_EIC_SCFG(x)		(0x4 + (x) * 0x4)
> +#define MCHP_EIC_SCFG_FRZ		BIT(31)
> +#define MCHP_EIC_SCFG_EN		BIT(16)
> +#define MCHP_EIC_SCFG_LVL		BIT(9)
> +#define MCHP_EIC_SCFG_POL		BIT(8)
> +#define MCHP_EIC_SCFG_GFEN		BIT(4)
> +#define MCHP_EIC_SCFG_GFSEL		GENMASK(1, 0)
> +
> +#define MCHP_EIC_NIRQ			(2)
> +#define MCHP_EIC_DEFAULT_WAIT_US	(100)
> +
> +/**
> + * struct mchp_eic - EIC private data structure
> + * @base: base address
> + * @dev: eic device
> + * @clk: peripheral clock
> + * @chip: irq chip
> + * @domain: irq domain
> + * @irqs: irqs b/w eic and gic
> + * @scfg: backup for scfg registers (necessary for backup and self-refresh mode)
> + * @wakeup_source: wakeup source mask
> + */
> +struct mchp_eic {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct clk *clk;
> +	struct irq_chip *chip;
> +	struct irq_domain *domain;
> +	u32 irqs[MCHP_EIC_NIRQ];
> +	u32 scfg[MCHP_EIC_NIRQ];
> +	u32 wakeup_source;
> +};
> +
> +static void mchp_eic_wait_rdy(struct mchp_eic *eic, unsigned int hwirq,
> +			      int delay_us)
> +{
> +	unsigned int tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));

Please use relaxed accessors everywhere unless you need ordering with
things like DMA (you don't). Specially if oyu are going to use that
in loops as below.

> +
> +	while (delay_us >= 0 && tmp & BIT(hwirq)) {
> +		udelay(1);
> +		delay_us -= 1;
> +		tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));
> +	}
> +
> +	if (delay_us < 0)
> +		dev_err(eic->dev, "Failed to configure IRQ=%u!\n", hwirq);

Given that this is used on some interrupt paths, you should *at least*
rate-limit it.

> +}
> +
> +static void mchp_eic_irq_mask(struct irq_data *d)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +	unsigned int tmp;
> +
> +	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	tmp &= ~MCHP_EIC_SCFG_EN;
> +	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
> +}
> +
> +static void mchp_eic_irq_unmask(struct irq_data *d)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +	unsigned int tmp;
> +
> +	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	tmp |= MCHP_EIC_SCFG_EN;
> +	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	mchp_eic_wait_rdy(eic, d->hwirq, MCHP_EIC_DEFAULT_WAIT_US);
> +}
> +
> +static int mchp_eic_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +	unsigned int tmp;
> +
> +	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	tmp &= ~(MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL);
> +	switch (type) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		irq_set_handler_locked(d, handle_level_irq);
> +		tmp |= MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		irq_set_handler_locked(d, handle_level_irq);
> +		tmp |= MCHP_EIC_SCFG_LVL;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		irq_set_handler_locked(d, handle_edge_irq);
> +		tmp |= MCHP_EIC_SCFG_POL;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		irq_set_handler_locked(d, handle_edge_irq);

How about the parent interrupt? Is the output of this irqchip always
LEVEL_HIGH, and not influenced by the triggering of its input?

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
> +
> +	return 0;
> +}
> +
> +static void mchp_eic_irq_ack(struct irq_data *d)
> +{
> +	/* Nothing to do here. */
> +}

Are you sure there is really nothing to do, even for an edge
interrupt? This is pretty unlikely. The interrupt controller must be
told when to stop coalescing edges, and without an Ack, it can't.

> +
> +static int mchp_eic_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +
> +	irq_set_irq_wake(eic->irqs[d->hwirq], on);
> +	if (on)
> +		eic->wakeup_source |= BIT(d->hwirq);
> +	else
> +		eic->wakeup_source &= BIT(d->hwirq);
> +
> +	return 0;
> +}
> +
> +static struct irq_chip mchp_eic_chip = {
> +	.name		= "eic-irq",
> +	.flags		= IRQCHIP_MASK_ON_SUSPEND,
> +	.irq_mask	= mchp_eic_irq_mask,
> +	.irq_unmask	= mchp_eic_irq_unmask,
> +	.irq_set_type	= mchp_eic_irq_set_type,
> +	.irq_ack	= mchp_eic_irq_ack,
> +	.irq_set_wake	= mchp_eic_irq_set_wake,
> +};
> +
> +static int mchp_eic_domain_map(struct irq_domain *d, unsigned int virq,
> +			       irq_hw_number_t hw)
> +{
> +	struct mchp_eic *eic = d->host_data;
> +
> +	irq_set_chip_data(virq, eic);
> +	irq_set_chip_and_handler(virq, eic->chip, handle_bad_irq);
> +	irq_set_probe(virq);
> +
> +	return 0;
> +}
> +
> +static int mchp_eic_domain_xlate(struct irq_domain *d, struct device_node *node,
> +				 const u32 *intspec, unsigned int intsize,
> +				 irq_hw_number_t *out_hwirq,
> +				 unsigned int *out_type)
> +{
> +	struct mchp_eic *eic = d->host_data;
> +	unsigned int tmp, i;
> +
> +	if (WARN_ON(intsize < 2))
> +		return -EINVAL;
> +
> +	if (WARN_ON(intspec[0] > MCHP_EIC_NIRQ))
> +		return -EINVAL;
> +
> +	*out_hwirq = intspec[0];
> +	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
> +
> +	if (intsize == 3) {

What if intsize isn't 3?

> +		/* Validate against available filters. */
> +		for (i = 0; i <= MCHP_EIC_SCFG_GFSEL; i++) {
> +			if (intspec[2] == (2 << i))
> +				break;
> +		}

Why don't you just take the log2() of the input value, and use that?

> +		if (i == MCHP_EIC_SCFG_GFSEL + 1)
> +			return -EINVAL;
> +
> +		/* Enable glitch filter. */
> +		tmp = readl(eic->base + MCHP_EIC_SCFG(*out_hwirq));
> +		tmp |= MCHP_EIC_SCFG_GFEN;
> +		tmp &= ~MCHP_EIC_SCFG_GFSEL;
> +		tmp |= i;
> +		writel(tmp, eic->base + MCHP_EIC_SCFG(*out_hwirq));

Please move this out of the xlate path. For a start, xlate shouldn't
have any effect on the HW. Also, this should be a static configuration
(see my comment in the first patch).

> +
> +		/* Wait for glitch filter to be enabled. */
> +		mchp_eic_wait_rdy(eic, *out_hwirq, MCHP_EIC_DEFAULT_WAIT_US);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops mchp_eic_domain_ops = {
> +	.map		= mchp_eic_domain_map,
> +	.xlate		= mchp_eic_domain_xlate,
> +};
> +
> +static void mchp_eic_irq_handler(struct irq_desc *desc)
> +{
> +	struct mchp_eic *eic = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned int hwirq, irq = irq_desc_get_irq(desc);
> +
> +	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
> +		if (eic->irqs[hwirq] == irq)
> +			break;
> +	}

Why don't you just use the GIC input INTID as a base, and use the
offset from that base to compute the hwirq?

> +	if (hwirq == MCHP_EIC_NIRQ)
> +		return;
> +
> +	chained_irq_enter(chip, desc);
> +	generic_handle_irq(irq_find_mapping(eic->domain, hwirq));
> +	chained_irq_exit(chip, desc);

Same question as above: are you sure the output of this block is
always LEVEL_HIGH? because if it isn't this, should be treated as a
hierarchical irqchip, and not a chained one.

> +}
> +
> +static int mchp_eic_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct mchp_eic *eic;
> +	int ret, i;
> +
> +	eic = devm_kzalloc(&pdev->dev, sizeof(*eic), GFP_KERNEL);
> +	if (!eic)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	eic->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(eic->base))
> +		return PTR_ERR(eic->base);
> +
> +	eic->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(eic->clk))
> +		return PTR_ERR(eic->clk);
> +
> +	ret = clk_prepare_enable(eic->clk);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < MCHP_EIC_NIRQ; i++) {
> +		/* Disable it, if any. */
> +		writel(0UL, eic->base + MCHP_EIC_SCFG(i));
> +
> +		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
> +		if (!res) {
> +			ret = dev_err_probe(&pdev->dev, -EINVAL,
> +					    "Failed to get hwirq=%d\n", i);
> +			goto clk_unprepare;
> +		}
> +
> +		eic->irqs[i] = res->start;
> +		irq_set_chained_handler(res->start, mchp_eic_irq_handler);
> +		irq_set_handler_data(res->start, eic);
> +	}
> +
> +	eic->dev = &pdev->dev;
> +	eic->chip = &mchp_eic_chip;
> +
> +	eic->domain = irq_domain_add_linear(pdev->dev.of_node, MCHP_EIC_NIRQ,
> +					    &mchp_eic_domain_ops, eic);
> +	if (!eic->domain) {
> +		ret = dev_err_probe(&pdev->dev, -ENODEV,
> +				    "Failed to regiter irq domain!\n");
> +		goto clk_unprepare;
> +	}
> +	eic->domain->name = "eic";

Why this override? The only benefits for that are a memory leak and a
memory corruption if we ever try to free the domain...

> +
> +	platform_set_drvdata(pdev, eic);
> +
> +	dev_info(&pdev->dev, "EIC registered, nr_irqs %u\n", MCHP_EIC_NIRQ);
> +
> +	return 0;
> +
> +clk_unprepare:
> +	clk_disable_unprepare(eic->clk);
> +	return ret;
> +}
> +
> +static int mchp_eic_remove(struct platform_device *pdev)
> +{
> +	struct mchp_eic *eic = platform_get_drvdata(pdev);
> +
> +	irq_domain_remove(eic->domain);
> +	clk_disable_unprepare(eic->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mchp_eic_suspend(struct device *dev)
> +{
> +	struct mchp_eic *eic = dev_get_drvdata(dev);
> +	unsigned int hwirq;
> +
> +	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++)
> +		eic->scfg[hwirq] = readl(eic->base + MCHP_EIC_SCFG(hwirq));
> +
> +	if (!eic->wakeup_source)
> +		clk_disable_unprepare(eic->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mchp_eic_resume(struct device *dev)
> +{
> +	struct mchp_eic *eic = dev_get_drvdata(dev);
> +	unsigned int hwirq;
> +
> +	if (!eic->wakeup_source)
> +		clk_prepare_enable(eic->clk);
> +
> +	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
> +		writel(eic->scfg[hwirq], eic->base + MCHP_EIC_SCFG(hwirq));
> +		mchp_eic_wait_rdy(eic, hwirq, MCHP_EIC_DEFAULT_WAIT_US);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops mchp_eic_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mchp_eic_suspend, mchp_eic_resume)
> +};
> +
> +static const struct of_device_id mchp_eic_dt_ids[] = {
> +	{ .compatible = "microchip,sama7g5-eic", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, mchp_eic_dt_ids);
> +
> +static struct platform_driver mchp_eic_device_driver = {
> +	.probe		= mchp_eic_probe,
> +	.remove		= mchp_eic_remove,
> +	.driver		= {
> +		.name = "mchp-eic",
> +		.of_match_table = of_match_ptr(mchp_eic_dt_ids),
> +		.pm = pm_ptr(&mchp_eic_pm_ops),
> +	},
> +};
> +module_platform_driver(mchp_eic_device_driver);
> +
> +MODULE_DESCRIPTION("Microchip External Interrupt Controller");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea@microchip.com>");
> -- 
> 2.25.1
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] irqchip/mchp-eic: add support

[Claudiu.Beznea]

Hi Mark,

Appologies for late reply, I had to double check some of your questions w/
IP designer.

On 02.03.2021 14:02, Marc Zyngier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, 02 Mar 2021 10:28:46 +0000,
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
>>
>> Add support for Microchip External Interrupt Controller.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  MAINTAINERS                    |   6 +
>>  drivers/irqchip/Kconfig        |   7 +
>>  drivers/irqchip/Makefile       |   1 +
>>  drivers/irqchip/irq-mchp-eic.c | 350 +++++++++++++++++++++++++++++++++
>>  4 files changed, 364 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-mchp-eic.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a008b70f3c16..ea99e5a7f519 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11585,6 +11585,12 @@ L:   linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>>  S:   Supported
>>  F:   drivers/usb/gadget/udc/atmel_usba_udc.*
>>
>> +MICROCHIP EIC DRIVER
>> +M:   Claudiu Beznea <claudiu.beznea@microchip.com>
>> +L:   linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>> +S:   Supported
>> +F:   drivers/irqchip/irq-mchp-eic.c
>> +
>>  MICROCHIP WILC1000 WIFI DRIVER
>>  M:   Ajay Singh <ajay.kathat@microchip.com>
>>  M:   Claudiu Beznea <claudiu.beznea@microchip.com>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 2aa79c32ee22..7c7ca33c3f7d 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -596,4 +596,11 @@ config MST_IRQ
>>       help
>>         Support MStar Interrupt Controller.
>>
>> +config MCHP_EIC
>> +     bool "Microchip External Interrupt Controller"
>> +     depends on ARCH_AT91 || COMPILE_TEST
>> +     select IRQ_DOMAIN
>> +     help
>> +       Support for Microchip External Interrupt Controller.
>> +
>>  endmenu
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 94c2885882ee..f3df6eb7c4c3 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -114,3 +114,4 @@ obj-$(CONFIG_LOONGSON_PCH_PIC)            += irq-loongson-pch-pic.o
>>  obj-$(CONFIG_LOONGSON_PCH_MSI)               += irq-loongson-pch-msi.o
>>  obj-$(CONFIG_MST_IRQ)                        += irq-mst-intc.o
>>  obj-$(CONFIG_SL28CPLD_INTC)          += irq-sl28cpld.o
>> +obj-$(CONFIG_MCHP_EIC)                       += irq-mchp-eic.o
>> diff --git a/drivers/irqchip/irq-mchp-eic.c b/drivers/irqchip/irq-mchp-eic.c
>> new file mode 100644
>> index 000000000000..ec01877fc75f
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-mchp-eic.c
>> @@ -0,0 +1,350 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Microchip External Interrupt Controller driver
>> + *
>> + * Copyright (C) 2021 Microchip Technology Inc. and its subsidiaries
>> + *
>> + * Author: Claudiu Beznea <claudiu.beznea@microchip.com>
>> + */
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +
>> +#define MCHP_EIC_GFCS                        (0x0)
>> +#define MCHP_EIC_SCFG(x)             (0x4 + (x) * 0x4)
>> +#define MCHP_EIC_SCFG_FRZ            BIT(31)
>> +#define MCHP_EIC_SCFG_EN             BIT(16)
>> +#define MCHP_EIC_SCFG_LVL            BIT(9)
>> +#define MCHP_EIC_SCFG_POL            BIT(8)
>> +#define MCHP_EIC_SCFG_GFEN           BIT(4)
>> +#define MCHP_EIC_SCFG_GFSEL          GENMASK(1, 0)
>> +
>> +#define MCHP_EIC_NIRQ                        (2)
>> +#define MCHP_EIC_DEFAULT_WAIT_US     (100)
>> +
>> +/**
>> + * struct mchp_eic - EIC private data structure
>> + * @base: base address
>> + * @dev: eic device
>> + * @clk: peripheral clock
>> + * @chip: irq chip
>> + * @domain: irq domain
>> + * @irqs: irqs b/w eic and gic
>> + * @scfg: backup for scfg registers (necessary for backup and self-refresh mode)
>> + * @wakeup_source: wakeup source mask
>> + */
>> +struct mchp_eic {
>> +     void __iomem *base;
>> +     struct device *dev;
>> +     struct clk *clk;
>> +     struct irq_chip *chip;
>> +     struct irq_domain *domain;
>> +     u32 irqs[MCHP_EIC_NIRQ];
>> +     u32 scfg[MCHP_EIC_NIRQ];
>> +     u32 wakeup_source;
>> +};
>> +
>> +static void mchp_eic_wait_rdy(struct mchp_eic *eic, unsigned int hwirq,
>> +                           int delay_us)
>> +{
>> +     unsigned int tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));
> 
> Please use relaxed accessors everywhere unless you need ordering with
> things like DMA (you don't). Specially if oyu are going to use that
> in loops as below.

OK.

> 
>> +
>> +     while (delay_us >= 0 && tmp & BIT(hwirq)) {
>> +             udelay(1);
>> +             delay_us -= 1;
>> +             tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));
>> +     }
>> +
>> +     if (delay_us < 0)
>> +             dev_err(eic->dev, "Failed to configure IRQ=%u!\n", hwirq);
> 
> Given that this is used on some interrupt paths, you should *at least*
> rate-limit it.

I used the MCHP_EIC_DEFAULT_WAIT_US as the rate limiter for this. I will
move it inside the function to be more clear.

> 
>> +}
>> +
>> +static void mchp_eic_irq_mask(struct irq_data *d)
>> +{
>> +     struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
>> +     unsigned int tmp;
>> +
>> +     tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +     tmp &= ~MCHP_EIC_SCFG_EN;
>> +     writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +}
>> +
>> +static void mchp_eic_irq_unmask(struct irq_data *d)
>> +{
>> +     struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
>> +     unsigned int tmp;
>> +
>> +     tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +     tmp |= MCHP_EIC_SCFG_EN;
>> +     writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +     mchp_eic_wait_rdy(eic, d->hwirq, MCHP_EIC_DEFAULT_WAIT_US);
>> +}
>> +
>> +static int mchp_eic_irq_set_type(struct irq_data *d, unsigned int type)
>> +{
>> +     struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
>> +     unsigned int tmp;
>> +
>> +     tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +     tmp &= ~(MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL);
>> +     switch (type) {
>> +     case IRQ_TYPE_LEVEL_HIGH:
>> +             irq_set_handler_locked(d, handle_level_irq);
>> +             tmp |= MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL;
>> +             break;
>> +     case IRQ_TYPE_LEVEL_LOW:
>> +             irq_set_handler_locked(d, handle_level_irq);
>> +             tmp |= MCHP_EIC_SCFG_LVL;
>> +             break;
>> +     case IRQ_TYPE_EDGE_RISING:
>> +             irq_set_handler_locked(d, handle_edge_irq);
>> +             tmp |= MCHP_EIC_SCFG_POL;
>> +             break;
>> +     case IRQ_TYPE_EDGE_FALLING:
>> +             irq_set_handler_locked(d, handle_edge_irq);
> 
> How about the parent interrupt? Is the output of this irqchip always
> LEVEL_HIGH, and not influenced by the triggering of its input?

According to the discusion I had with the IP designer, yes, the output
active level of this IP is always 1.
The level or edge detection refers to the input signal of EIC. No matter
the input edge detection for EIC, the EIC will generate a high pulse for GIC.

> 
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>> +     writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +
>> +     return 0;
>> +}
>> +
>> +static void mchp_eic_irq_ack(struct irq_data *d)
>> +{
>> +     /* Nothing to do here. */
>> +}
> 
> Are you sure there is really nothing to do, even for an edge
> interrupt? This is pretty unlikely. The interrupt controller must be
> told when to stop coalescing edges, and without an Ack, it can't.

There is nothing to be done to acknowledge this (I double checked with the
designer). The IP has only 2 registers:
1/ Glitch Filter Configuration Status Register: with 2 readable bits (bit 0
for interrupt 0, bit 1 for interrupt 1); reading a 1 to any of these bits
means the interrupt line (0 or 1) glitch filter configuration is done and
the glitch filter is active. The glitch filter can be programmed in SCFGR.

2/ Source Configuration Register (SCFGR): with bits having the following
meaning:
- bit 31: writing a 1 freezes the SCFGR values until hardware reset.
- bit 16: writing a 1 enables the source level/edge detection; writing a 0
          disables the source level/edge detection
- bit 9: for input level configuration detection
- bit 8: for input polarity configuration detection
- bit 4: for enabling/disbabling glitch filter
- bit 1..0: glitch filter settings

> 
>> +
>> +static int mchp_eic_irq_set_wake(struct irq_data *d, unsigned int on)
>> +{
>> +     struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
>> +
>> +     irq_set_irq_wake(eic->irqs[d->hwirq], on);
>> +     if (on)
>> +             eic->wakeup_source |= BIT(d->hwirq);
>> +     else
>> +             eic->wakeup_source &= BIT(d->hwirq);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct irq_chip mchp_eic_chip = {
>> +     .name           = "eic-irq",
>> +     .flags          = IRQCHIP_MASK_ON_SUSPEND,
>> +     .irq_mask       = mchp_eic_irq_mask,
>> +     .irq_unmask     = mchp_eic_irq_unmask,
>> +     .irq_set_type   = mchp_eic_irq_set_type,
>> +     .irq_ack        = mchp_eic_irq_ack,
>> +     .irq_set_wake   = mchp_eic_irq_set_wake,
>> +};
>> +
>> +static int mchp_eic_domain_map(struct irq_domain *d, unsigned int virq,
>> +                            irq_hw_number_t hw)
>> +{
>> +     struct mchp_eic *eic = d->host_data;
>> +
>> +     irq_set_chip_data(virq, eic);
>> +     irq_set_chip_and_handler(virq, eic->chip, handle_bad_irq);
>> +     irq_set_probe(virq);
>> +
>> +     return 0;
>> +}
>> +
>> +static int mchp_eic_domain_xlate(struct irq_domain *d, struct device_node *node,
>> +                              const u32 *intspec, unsigned int intsize,
>> +                              irq_hw_number_t *out_hwirq,
>> +                              unsigned int *out_type)
>> +{
>> +     struct mchp_eic *eic = d->host_data;
>> +     unsigned int tmp, i;
>> +
>> +     if (WARN_ON(intsize < 2))
>> +             return -EINVAL;
>> +
>> +     if (WARN_ON(intspec[0] > MCHP_EIC_NIRQ))
>> +             return -EINVAL;
>> +
>> +     *out_hwirq = intspec[0];
>> +     *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
>> +
>> +     if (intsize == 3) {
> 
> What if intsize isn't 3?

This escaped me.

> 
>> +             /* Validate against available filters. */
>> +             for (i = 0; i <= MCHP_EIC_SCFG_GFSEL; i++) {
>> +                     if (intspec[2] == (2 << i))
>> +                             break;
>> +             }
> 
> Why don't you just take the log2() of the input value, and use that?

I'll do it in the next version.

> 
>> +             if (i == MCHP_EIC_SCFG_GFSEL + 1)
>> +                     return -EINVAL;
>> +
>> +             /* Enable glitch filter. */
>> +             tmp = readl(eic->base + MCHP_EIC_SCFG(*out_hwirq));
>> +             tmp |= MCHP_EIC_SCFG_GFEN;
>> +             tmp &= ~MCHP_EIC_SCFG_GFSEL;
>> +             tmp |= i;
>> +             writel(tmp, eic->base + MCHP_EIC_SCFG(*out_hwirq));
> 
> Please move this out of the xlate path. For a start, xlate shouldn't
> have any effect on the HW. Also, this should be a static configuration
> (see my comment in the first patch).

Sure, I'll address it in the next version.

> 
>> +
>> +             /* Wait for glitch filter to be enabled. */
>> +             mchp_eic_wait_rdy(eic, *out_hwirq, MCHP_EIC_DEFAULT_WAIT_US);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct irq_domain_ops mchp_eic_domain_ops = {
>> +     .map            = mchp_eic_domain_map,
>> +     .xlate          = mchp_eic_domain_xlate,
>> +};
>> +
>> +static void mchp_eic_irq_handler(struct irq_desc *desc)
>> +{
>> +     struct mchp_eic *eic = irq_desc_get_handler_data(desc);
>> +     struct irq_chip *chip = irq_desc_get_chip(desc);
>> +     unsigned int hwirq, irq = irq_desc_get_irq(desc);
>> +
>> +     for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
>> +             if (eic->irqs[hwirq] == irq)
>> +                     break;
>> +     }
> 
> Why don't you just use the GIC input INTID as a base, and use the
> offset from that base to compute the hwirq?

I'll dig into this.

> 
>> +     if (hwirq == MCHP_EIC_NIRQ)
>> +             return;
>> +
>> +     chained_irq_enter(chip, desc);
>> +     generic_handle_irq(irq_find_mapping(eic->domain, hwirq));
>> +     chained_irq_exit(chip, desc);
> 
> Same question as above: are you sure the output of this block is
> always LEVEL_HIGH? because if it isn't this, should be treated as a
> hierarchical irqchip, and not a chained one.

Yes, as per discusion with designer on our side the output of EIC is always
high.

> 
>> +}
>> +
>> +static int mchp_eic_probe(struct platform_device *pdev)
>> +{
>> +     struct resource *res;
>> +     struct mchp_eic *eic;
>> +     int ret, i;
>> +
>> +     eic = devm_kzalloc(&pdev->dev, sizeof(*eic), GFP_KERNEL);
>> +     if (!eic)
>> +             return -ENOMEM;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     eic->base = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(eic->base))
>> +             return PTR_ERR(eic->base);
>> +
>> +     eic->clk = devm_clk_get(&pdev->dev, NULL);
>> +     if (IS_ERR(eic->clk))
>> +             return PTR_ERR(eic->clk);
>> +
>> +     ret = clk_prepare_enable(eic->clk);
>> +     if (ret)
>> +             return ret;
>> +
>> +     for (i = 0; i < MCHP_EIC_NIRQ; i++) {
>> +             /* Disable it, if any. */
>> +             writel(0UL, eic->base + MCHP_EIC_SCFG(i));
>> +
>> +             res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
>> +             if (!res) {
>> +                     ret = dev_err_probe(&pdev->dev, -EINVAL,
>> +                                         "Failed to get hwirq=%d\n", i);
>> +                     goto clk_unprepare;
>> +             }
>> +
>> +             eic->irqs[i] = res->start;
>> +             irq_set_chained_handler(res->start, mchp_eic_irq_handler);
>> +             irq_set_handler_data(res->start, eic);
>> +     }
>> +
>> +     eic->dev = &pdev->dev;
>> +     eic->chip = &mchp_eic_chip;
>> +
>> +     eic->domain = irq_domain_add_linear(pdev->dev.of_node, MCHP_EIC_NIRQ,
>> +                                         &mchp_eic_domain_ops, eic);
>> +     if (!eic->domain) {
>> +             ret = dev_err_probe(&pdev->dev, -ENODEV,
>> +                                 "Failed to regiter irq domain!\n");
>> +             goto clk_unprepare;
>> +     }
>> +     eic->domain->name = "eic";
> 
> Why this override? The only benefits for that are a memory leak and a
> memory corruption if we ever try to free the domain...

I used the approach from another driver and forgot to double check it. Will
be removed in the next version.

Thank you for your review,
Claudiu Beznea

> 
>> +
>> +     platform_set_drvdata(pdev, eic);
>> +
>> +     dev_info(&pdev->dev, "EIC registered, nr_irqs %u\n", MCHP_EIC_NIRQ);
>> +
>> +     return 0;
>> +
>> +clk_unprepare:
>> +     clk_disable_unprepare(eic->clk);
>> +     return ret;
>> +}
>> +
>> +static int mchp_eic_remove(struct platform_device *pdev)
>> +{
>> +     struct mchp_eic *eic = platform_get_drvdata(pdev);
>> +
>> +     irq_domain_remove(eic->domain);
>> +     clk_disable_unprepare(eic->clk);
>> +
>> +     return 0;
>> +}
>> +
>> +static int __maybe_unused mchp_eic_suspend(struct device *dev)
>> +{
>> +     struct mchp_eic *eic = dev_get_drvdata(dev);
>> +     unsigned int hwirq;
>> +
>> +     for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++)
>> +             eic->scfg[hwirq] = readl(eic->base + MCHP_EIC_SCFG(hwirq));
>> +
>> +     if (!eic->wakeup_source)
>> +             clk_disable_unprepare(eic->clk);
>> +
>> +     return 0;
>> +}
>> +
>> +static int __maybe_unused mchp_eic_resume(struct device *dev)
>> +{
>> +     struct mchp_eic *eic = dev_get_drvdata(dev);
>> +     unsigned int hwirq;
>> +
>> +     if (!eic->wakeup_source)
>> +             clk_prepare_enable(eic->clk);
>> +
>> +     for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
>> +             writel(eic->scfg[hwirq], eic->base + MCHP_EIC_SCFG(hwirq));
>> +             mchp_eic_wait_rdy(eic, hwirq, MCHP_EIC_DEFAULT_WAIT_US);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct dev_pm_ops mchp_eic_pm_ops = {
>> +     SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mchp_eic_suspend, mchp_eic_resume)
>> +};
>> +
>> +static const struct of_device_id mchp_eic_dt_ids[] = {
>> +     { .compatible = "microchip,sama7g5-eic", },
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(of, mchp_eic_dt_ids);
>> +
>> +static struct platform_driver mchp_eic_device_driver = {
>> +     .probe          = mchp_eic_probe,
>> +     .remove         = mchp_eic_remove,
>> +     .driver         = {
>> +             .name = "mchp-eic",
>> +             .of_match_table = of_match_ptr(mchp_eic_dt_ids),
>> +             .pm = pm_ptr(&mchp_eic_pm_ops),
>> +     },
>> +};
>> +module_platform_driver(mchp_eic_device_driver);
>> +
>> +MODULE_DESCRIPTION("Microchip External Interrupt Controller");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea@microchip.com>");
>> --
>> 2.25.1
>>
>>
> 
> Thanks,
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.
> 

Re: [PATCH 2/2] irqchip/mchp-eic: add support

[Claudiu.Beznea]

Hi Mark,

Appologies for late reply, I had to double check some of your questions w/
IP designer.

On 02.03.2021 14:02, Marc Zyngier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, 02 Mar 2021 10:28:46 +0000,
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
>>
>> Add support for Microchip External Interrupt Controller.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  MAINTAINERS                    |   6 +
>>  drivers/irqchip/Kconfig        |   7 +
>>  drivers/irqchip/Makefile       |   1 +
>>  drivers/irqchip/irq-mchp-eic.c | 350 +++++++++++++++++++++++++++++++++
>>  4 files changed, 364 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-mchp-eic.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a008b70f3c16..ea99e5a7f519 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11585,6 +11585,12 @@ L:   linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>>  S:   Supported
>>  F:   drivers/usb/gadget/udc/atmel_usba_udc.*
>>
>> +MICROCHIP EIC DRIVER
>> +M:   Claudiu Beznea <claudiu.beznea@microchip.com>
>> +L:   linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>> +S:   Supported
>> +F:   drivers/irqchip/irq-mchp-eic.c
>> +
>>  MICROCHIP WILC1000 WIFI DRIVER
>>  M:   Ajay Singh <ajay.kathat@microchip.com>
>>  M:   Claudiu Beznea <claudiu.beznea@microchip.com>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 2aa79c32ee22..7c7ca33c3f7d 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -596,4 +596,11 @@ config MST_IRQ
>>       help
>>         Support MStar Interrupt Controller.
>>
>> +config MCHP_EIC
>> +     bool "Microchip External Interrupt Controller"
>> +     depends on ARCH_AT91 || COMPILE_TEST
>> +     select IRQ_DOMAIN
>> +     help
>> +       Support for Microchip External Interrupt Controller.
>> +
>>  endmenu
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 94c2885882ee..f3df6eb7c4c3 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -114,3 +114,4 @@ obj-$(CONFIG_LOONGSON_PCH_PIC)            += irq-loongson-pch-pic.o
>>  obj-$(CONFIG_LOONGSON_PCH_MSI)               += irq-loongson-pch-msi.o
>>  obj-$(CONFIG_MST_IRQ)                        += irq-mst-intc.o
>>  obj-$(CONFIG_SL28CPLD_INTC)          += irq-sl28cpld.o
>> +obj-$(CONFIG_MCHP_EIC)                       += irq-mchp-eic.o
>> diff --git a/drivers/irqchip/irq-mchp-eic.c b/drivers/irqchip/irq-mchp-eic.c
>> new file mode 100644
>> index 000000000000..ec01877fc75f
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-mchp-eic.c
>> @@ -0,0 +1,350 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Microchip External Interrupt Controller driver
>> + *
>> + * Copyright (C) 2021 Microchip Technology Inc. and its subsidiaries
>> + *
>> + * Author: Claudiu Beznea <claudiu.beznea@microchip.com>
>> + */
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +
>> +#define MCHP_EIC_GFCS                        (0x0)
>> +#define MCHP_EIC_SCFG(x)             (0x4 + (x) * 0x4)
>> +#define MCHP_EIC_SCFG_FRZ            BIT(31)
>> +#define MCHP_EIC_SCFG_EN             BIT(16)
>> +#define MCHP_EIC_SCFG_LVL            BIT(9)
>> +#define MCHP_EIC_SCFG_POL            BIT(8)
>> +#define MCHP_EIC_SCFG_GFEN           BIT(4)
>> +#define MCHP_EIC_SCFG_GFSEL          GENMASK(1, 0)
>> +
>> +#define MCHP_EIC_NIRQ                        (2)
>> +#define MCHP_EIC_DEFAULT_WAIT_US     (100)
>> +
>> +/**
>> + * struct mchp_eic - EIC private data structure
>> + * @base: base address
>> + * @dev: eic device
>> + * @clk: peripheral clock
>> + * @chip: irq chip
>> + * @domain: irq domain
>> + * @irqs: irqs b/w eic and gic
>> + * @scfg: backup for scfg registers (necessary for backup and self-refresh mode)
>> + * @wakeup_source: wakeup source mask
>> + */
>> +struct mchp_eic {
>> +     void __iomem *base;
>> +     struct device *dev;
>> +     struct clk *clk;
>> +     struct irq_chip *chip;
>> +     struct irq_domain *domain;
>> +     u32 irqs[MCHP_EIC_NIRQ];
>> +     u32 scfg[MCHP_EIC_NIRQ];
>> +     u32 wakeup_source;
>> +};
>> +
>> +static void mchp_eic_wait_rdy(struct mchp_eic *eic, unsigned int hwirq,
>> +                           int delay_us)
>> +{
>> +     unsigned int tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));
> 
> Please use relaxed accessors everywhere unless you need ordering with
> things like DMA (you don't). Specially if oyu are going to use that
> in loops as below.

OK.

> 
>> +
>> +     while (delay_us >= 0 && tmp & BIT(hwirq)) {
>> +             udelay(1);
>> +             delay_us -= 1;
>> +             tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));
>> +     }
>> +
>> +     if (delay_us < 0)
>> +             dev_err(eic->dev, "Failed to configure IRQ=%u!\n", hwirq);
> 
> Given that this is used on some interrupt paths, you should *at least*
> rate-limit it.

I used the MCHP_EIC_DEFAULT_WAIT_US as the rate limiter for this. I will
move it inside the function to be more clear.

> 
>> +}
>> +
>> +static void mchp_eic_irq_mask(struct irq_data *d)
>> +{
>> +     struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
>> +     unsigned int tmp;
>> +
>> +     tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +     tmp &= ~MCHP_EIC_SCFG_EN;
>> +     writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +}
>> +
>> +static void mchp_eic_irq_unmask(struct irq_data *d)
>> +{
>> +     struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
>> +     unsigned int tmp;
>> +
>> +     tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +     tmp |= MCHP_EIC_SCFG_EN;
>> +     writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +     mchp_eic_wait_rdy(eic, d->hwirq, MCHP_EIC_DEFAULT_WAIT_US);
>> +}
>> +
>> +static int mchp_eic_irq_set_type(struct irq_data *d, unsigned int type)
>> +{
>> +     struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
>> +     unsigned int tmp;
>> +
>> +     tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +     tmp &= ~(MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL);
>> +     switch (type) {
>> +     case IRQ_TYPE_LEVEL_HIGH:
>> +             irq_set_handler_locked(d, handle_level_irq);
>> +             tmp |= MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL;
>> +             break;
>> +     case IRQ_TYPE_LEVEL_LOW:
>> +             irq_set_handler_locked(d, handle_level_irq);
>> +             tmp |= MCHP_EIC_SCFG_LVL;
>> +             break;
>> +     case IRQ_TYPE_EDGE_RISING:
>> +             irq_set_handler_locked(d, handle_edge_irq);
>> +             tmp |= MCHP_EIC_SCFG_POL;
>> +             break;
>> +     case IRQ_TYPE_EDGE_FALLING:
>> +             irq_set_handler_locked(d, handle_edge_irq);
> 
> How about the parent interrupt? Is the output of this irqchip always
> LEVEL_HIGH, and not influenced by the triggering of its input?

According to the discusion I had with the IP designer, yes, the output
active level of this IP is always 1.
The level or edge detection refers to the input signal of EIC. No matter
the input edge detection for EIC, the EIC will generate a high pulse for GIC.

> 
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>> +     writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
>> +
>> +     return 0;
>> +}
>> +
>> +static void mchp_eic_irq_ack(struct irq_data *d)
>> +{
>> +     /* Nothing to do here. */
>> +}
> 
> Are you sure there is really nothing to do, even for an edge
> interrupt? This is pretty unlikely. The interrupt controller must be
> told when to stop coalescing edges, and without an Ack, it can't.

There is nothing to be done to acknowledge this (I double checked with the
designer). The IP has only 2 registers:
1/ Glitch Filter Configuration Status Register: with 2 readable bits (bit 0
for interrupt 0, bit 1 for interrupt 1); reading a 1 to any of these bits
means the interrupt line (0 or 1) glitch filter configuration is done and
the glitch filter is active. The glitch filter can be programmed in SCFGR.

2/ Source Configuration Register (SCFGR): with bits having the following
meaning:
- bit 31: writing a 1 freezes the SCFGR values until hardware reset.
- bit 16: writing a 1 enables the source level/edge detection; writing a 0
          disables the source level/edge detection
- bit 9: for input level configuration detection
- bit 8: for input polarity configuration detection
- bit 4: for enabling/disbabling glitch filter
- bit 1..0: glitch filter settings

> 
>> +
>> +static int mchp_eic_irq_set_wake(struct irq_data *d, unsigned int on)
>> +{
>> +     struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
>> +
>> +     irq_set_irq_wake(eic->irqs[d->hwirq], on);
>> +     if (on)
>> +             eic->wakeup_source |= BIT(d->hwirq);
>> +     else
>> +             eic->wakeup_source &= BIT(d->hwirq);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct irq_chip mchp_eic_chip = {
>> +     .name           = "eic-irq",
>> +     .flags          = IRQCHIP_MASK_ON_SUSPEND,
>> +     .irq_mask       = mchp_eic_irq_mask,
>> +     .irq_unmask     = mchp_eic_irq_unmask,
>> +     .irq_set_type   = mchp_eic_irq_set_type,
>> +     .irq_ack        = mchp_eic_irq_ack,
>> +     .irq_set_wake   = mchp_eic_irq_set_wake,
>> +};
>> +
>> +static int mchp_eic_domain_map(struct irq_domain *d, unsigned int virq,
>> +                            irq_hw_number_t hw)
>> +{
>> +     struct mchp_eic *eic = d->host_data;
>> +
>> +     irq_set_chip_data(virq, eic);
>> +     irq_set_chip_and_handler(virq, eic->chip, handle_bad_irq);
>> +     irq_set_probe(virq);
>> +
>> +     return 0;
>> +}
>> +
>> +static int mchp_eic_domain_xlate(struct irq_domain *d, struct device_node *node,
>> +                              const u32 *intspec, unsigned int intsize,
>> +                              irq_hw_number_t *out_hwirq,
>> +                              unsigned int *out_type)
>> +{
>> +     struct mchp_eic *eic = d->host_data;
>> +     unsigned int tmp, i;
>> +
>> +     if (WARN_ON(intsize < 2))
>> +             return -EINVAL;
>> +
>> +     if (WARN_ON(intspec[0] > MCHP_EIC_NIRQ))
>> +             return -EINVAL;
>> +
>> +     *out_hwirq = intspec[0];
>> +     *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
>> +
>> +     if (intsize == 3) {
> 
> What if intsize isn't 3?

This escaped me.

> 
>> +             /* Validate against available filters. */
>> +             for (i = 0; i <= MCHP_EIC_SCFG_GFSEL; i++) {
>> +                     if (intspec[2] == (2 << i))
>> +                             break;
>> +             }
> 
> Why don't you just take the log2() of the input value, and use that?

I'll do it in the next version.

> 
>> +             if (i == MCHP_EIC_SCFG_GFSEL + 1)
>> +                     return -EINVAL;
>> +
>> +             /* Enable glitch filter. */
>> +             tmp = readl(eic->base + MCHP_EIC_SCFG(*out_hwirq));
>> +             tmp |= MCHP_EIC_SCFG_GFEN;
>> +             tmp &= ~MCHP_EIC_SCFG_GFSEL;
>> +             tmp |= i;
>> +             writel(tmp, eic->base + MCHP_EIC_SCFG(*out_hwirq));
> 
> Please move this out of the xlate path. For a start, xlate shouldn't
> have any effect on the HW. Also, this should be a static configuration
> (see my comment in the first patch).

Sure, I'll address it in the next version.

> 
>> +
>> +             /* Wait for glitch filter to be enabled. */
>> +             mchp_eic_wait_rdy(eic, *out_hwirq, MCHP_EIC_DEFAULT_WAIT_US);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct irq_domain_ops mchp_eic_domain_ops = {
>> +     .map            = mchp_eic_domain_map,
>> +     .xlate          = mchp_eic_domain_xlate,
>> +};
>> +
>> +static void mchp_eic_irq_handler(struct irq_desc *desc)
>> +{
>> +     struct mchp_eic *eic = irq_desc_get_handler_data(desc);
>> +     struct irq_chip *chip = irq_desc_get_chip(desc);
>> +     unsigned int hwirq, irq = irq_desc_get_irq(desc);
>> +
>> +     for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
>> +             if (eic->irqs[hwirq] == irq)
>> +                     break;
>> +     }
> 
> Why don't you just use the GIC input INTID as a base, and use the
> offset from that base to compute the hwirq?

I'll dig into this.

> 
>> +     if (hwirq == MCHP_EIC_NIRQ)
>> +             return;
>> +
>> +     chained_irq_enter(chip, desc);
>> +     generic_handle_irq(irq_find_mapping(eic->domain, hwirq));
>> +     chained_irq_exit(chip, desc);
> 
> Same question as above: are you sure the output of this block is
> always LEVEL_HIGH? because if it isn't this, should be treated as a
> hierarchical irqchip, and not a chained one.

Yes, as per discusion with designer on our side the output of EIC is always
high.

> 
>> +}
>> +
>> +static int mchp_eic_probe(struct platform_device *pdev)
>> +{
>> +     struct resource *res;
>> +     struct mchp_eic *eic;
>> +     int ret, i;
>> +
>> +     eic = devm_kzalloc(&pdev->dev, sizeof(*eic), GFP_KERNEL);
>> +     if (!eic)
>> +             return -ENOMEM;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     eic->base = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(eic->base))
>> +             return PTR_ERR(eic->base);
>> +
>> +     eic->clk = devm_clk_get(&pdev->dev, NULL);
>> +     if (IS_ERR(eic->clk))
>> +             return PTR_ERR(eic->clk);
>> +
>> +     ret = clk_prepare_enable(eic->clk);
>> +     if (ret)
>> +             return ret;
>> +
>> +     for (i = 0; i < MCHP_EIC_NIRQ; i++) {
>> +             /* Disable it, if any. */
>> +             writel(0UL, eic->base + MCHP_EIC_SCFG(i));
>> +
>> +             res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
>> +             if (!res) {
>> +                     ret = dev_err_probe(&pdev->dev, -EINVAL,
>> +                                         "Failed to get hwirq=%d\n", i);
>> +                     goto clk_unprepare;
>> +             }
>> +
>> +             eic->irqs[i] = res->start;
>> +             irq_set_chained_handler(res->start, mchp_eic_irq_handler);
>> +             irq_set_handler_data(res->start, eic);
>> +     }
>> +
>> +     eic->dev = &pdev->dev;
>> +     eic->chip = &mchp_eic_chip;
>> +
>> +     eic->domain = irq_domain_add_linear(pdev->dev.of_node, MCHP_EIC_NIRQ,
>> +                                         &mchp_eic_domain_ops, eic);
>> +     if (!eic->domain) {
>> +             ret = dev_err_probe(&pdev->dev, -ENODEV,
>> +                                 "Failed to regiter irq domain!\n");
>> +             goto clk_unprepare;
>> +     }
>> +     eic->domain->name = "eic";
> 
> Why this override? The only benefits for that are a memory leak and a
> memory corruption if we ever try to free the domain...

I used the approach from another driver and forgot to double check it. Will
be removed in the next version.

Thank you for your review,
Claudiu Beznea

> 
>> +
>> +     platform_set_drvdata(pdev, eic);
>> +
>> +     dev_info(&pdev->dev, "EIC registered, nr_irqs %u\n", MCHP_EIC_NIRQ);
>> +
>> +     return 0;
>> +
>> +clk_unprepare:
>> +     clk_disable_unprepare(eic->clk);
>> +     return ret;
>> +}
>> +
>> +static int mchp_eic_remove(struct platform_device *pdev)
>> +{
>> +     struct mchp_eic *eic = platform_get_drvdata(pdev);
>> +
>> +     irq_domain_remove(eic->domain);
>> +     clk_disable_unprepare(eic->clk);
>> +
>> +     return 0;
>> +}
>> +
>> +static int __maybe_unused mchp_eic_suspend(struct device *dev)
>> +{
>> +     struct mchp_eic *eic = dev_get_drvdata(dev);
>> +     unsigned int hwirq;
>> +
>> +     for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++)
>> +             eic->scfg[hwirq] = readl(eic->base + MCHP_EIC_SCFG(hwirq));
>> +
>> +     if (!eic->wakeup_source)
>> +             clk_disable_unprepare(eic->clk);
>> +
>> +     return 0;
>> +}
>> +
>> +static int __maybe_unused mchp_eic_resume(struct device *dev)
>> +{
>> +     struct mchp_eic *eic = dev_get_drvdata(dev);
>> +     unsigned int hwirq;
>> +
>> +     if (!eic->wakeup_source)
>> +             clk_prepare_enable(eic->clk);
>> +
>> +     for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
>> +             writel(eic->scfg[hwirq], eic->base + MCHP_EIC_SCFG(hwirq));
>> +             mchp_eic_wait_rdy(eic, hwirq, MCHP_EIC_DEFAULT_WAIT_US);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct dev_pm_ops mchp_eic_pm_ops = {
>> +     SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mchp_eic_suspend, mchp_eic_resume)
>> +};
>> +
>> +static const struct of_device_id mchp_eic_dt_ids[] = {
>> +     { .compatible = "microchip,sama7g5-eic", },
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(of, mchp_eic_dt_ids);
>> +
>> +static struct platform_driver mchp_eic_device_driver = {
>> +     .probe          = mchp_eic_probe,
>> +     .remove         = mchp_eic_remove,
>> +     .driver         = {
>> +             .name = "mchp-eic",
>> +             .of_match_table = of_match_ptr(mchp_eic_dt_ids),
>> +             .pm = pm_ptr(&mchp_eic_pm_ops),
>> +     },
>> +};
>> +module_platform_driver(mchp_eic_device_driver);
>> +
>> +MODULE_DESCRIPTION("Microchip External Interrupt Controller");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea@microchip.com>");
>> --
>> 2.25.1
>>
>>
> 
> Thanks,
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel