Re: [PATCH RFC v2 2/3] dt-bindings/interrupt-controller: pdc: descibe PDC device binding

On 02/02/18 16:46, Lina Iyer wrote:

On Fri, Feb 02 2018 at 16:28 +0000, Marc Zyngier wrote:

quoted

On 02/02/18 14:21, Lina Iyer wrote:

quoted

From: Archana Sathyakumar <redacted>

Add device binding documentation for the PDC Interrupt controller on
QCOM SoC's like the SDM845. The interrupt-controller can be used to
sense edge low interrupts and wakeup interrupts when the GIC is
non-operational.

Cc: devicetree@vger.kernel.org
Signed-off-by: Archana Sathyakumar <redacted>
Signed-off-by: Lina Iyer <redacted>
---
 .../bindings/interrupt-controller/qcom,pdc.txt     | 78 ++++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
new file mode 100644
index 000000000000..7bf40cb6a4f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt

@@ -0,0 +1,78 @@
+PDC interrupt controller
+
+Qualcomm Technologies Inc. SoCs based on the RPM Hardened architecture have a
+Power Domain Controller (PDC) that is on always-on domain. In addition to
+providing power control for the power domains, the hardware also has an
+interrupt controller that can be used to help detect edge low interrupts as
+well detect interrupts when the GIC is non-operational.
+
+GIC is parent interrupt controller at the highest level. Platform interrupt
+controller PDC is next in hierarchy, followed by others. Drivers requiring
+wakeup capabilities of their device interrupts routed through the PDC, must
+specify PDC as their interrupt controller and request the PDC port associated
+with the GIC interrupt. See example below.
+
+Properties:
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: Should contain "qcom,<soc>-pdc"
+		    - "qcom,sdm845-pdc": For SDM845
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: Specifies the base physical address for PDC hardware.
+
+- interrupt-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: Specifies the number of cells needed to encode an interrupt
+		    source.
+		    The value must match that of the parent interrupt
+		    controller defined in the DT.
+		    The encoding of these cells are same as described in [1].

There shouldn't be such a requirement. These are two independent pieces
of HW, and the first parameter doesn't mean anything for the PDC.

Wouldn't that be confusing - that we have different definitions for
interrupts in the same DT? I agree that they are different interrupt
controllers, but it just feels odd.

I think it feels more bizarre to have pointless fields in the interrupt
specifier. And most DTs have some sort of secondary interrupt controller
that only take two (or even one) parameters. I don't think we should
treat this any differently.

I will change this to just take 2 cells.

Thanks,

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