diff mbox series

[v3,1/2] dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller

Message ID 1599644912-29245-2-git-send-email-wuht06@gmail.com
State Changes Requested
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: Add new Unisoc PCIe driver | expand

Commit Message

Hongtao Wu Sept. 9, 2020, 9:48 a.m. UTC
From: Hongtao Wu <billows.wu@unisoc.com>

This series adds PCIe bindings for Unisoc SoCs.
This controller is based on DesignWare PCIe IP.

Signed-off-by: Hongtao Wu <billows.wu@unisoc.com>
---
 .../devicetree/bindings/pci/sprd-pcie.yaml         | 101 +++++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/sprd-pcie.yaml

--
2.7.4

Comments

Rob Herring Sept. 15, 2020, 5:25 p.m. UTC | #1
On Wed, Sep 09, 2020 at 05:48:31PM +0800, Hongtao Wu wrote:
> From: Hongtao Wu <billows.wu@unisoc.com>
> 
> This series adds PCIe bindings for Unisoc SoCs.
> This controller is based on DesignWare PCIe IP.
> 
> Signed-off-by: Hongtao Wu <billows.wu@unisoc.com>
> ---
>  .../devicetree/bindings/pci/sprd-pcie.yaml         | 101 +++++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/sprd-pcie.yaml b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> new file mode 100644
> index 0000000..c52edfb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/sprd-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SoC PCIe Host Controller Device Tree Bindings
> +
> +maintainers:
> +  - Hongtao Wu <billows.wu@unisoc.com>
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: sprd,pcie-rc
> +
> +  reg:
> +    minItems: 2
> +    items:
> +      - description: Controller control and status registers.
> +      - description: PCIe configuration registers.
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: config
> +
> +  ranges:
> +    maxItems: 2
> +
> +  num-lanes:
> +    maximum: 1
> +    description: Number of lanes to use for this port.
> +
> +  interrupts:
> +    minItems: 1
> +    description: Builtin MSI controller and PCIe host controller.
> +
> +  interrupt-names:
> +    items:
> +      - const: msi
> +
> +  sprd-pcie-poweron-syscons:

Doesn't match the example.

> +    minItems: 1
> +    description: Global register.
> +      The first value is the phandle to the global registers required to
> +      confige PCIe phy, clock and so on.
> +      The second value is the global register type which indicates whether it
> +      is a set/clear register or not.
> +      The third value is the time to delay after the global register is set or
> +      cleared.
> +      The fourth value is the global register address.
> +      The fifth value is the the mask value that the global register must
> +      be operate.
> +      The sixth value is the value that will be set to the global register.
> +      Note that Some Unisoc global registers have not been upstreamed.
> +      The global register and its mask can't be found in linux kernel,
> +      so we use an offset address and a number to instead them.

From the example, it looks like you set/clear 2 bits for power on/off. 
What's the worst case you expect here? What do the 2 bits do? If they 
are for clocks, resets, or power domains, then we have bindings for 
those which should be used. This use of phandles to syscons should be 
avoided whenever possible.

If we wanted a language for specifying sequences of register accesses in 
DT, we would have defined that a long time ago.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - num-lanes
> +  - ranges
> +  - interrupts
> +  - interrupt-names
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    ipa {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie0: pcie@2b100000 {
> +            compatible = "sprd,pcie-rc";
> +            reg = <0x0 0x2b100000 0x0 0x2000>,
> +                  <0x2 0x00000000 0x0 0x2000>;
> +            reg-names = "dbi", "config";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +            device_type = "pci";
> +            ranges = <0x01000000 0x0 0x00000000 0x2 0x00002000 0x0 0x00010000>,
> +                     <0x03000000 0x0 0x10000000 0x2 0x10000000 0x1 0xefffffff>;
> +            num-lanes = <1>;
> +            interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "msi";
> +
> +            sprd,pcie-poweron-syscons =
> +                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x40>,
> +                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x20>;
> +            sprd,pcie-poweroff-syscons =

Not documented.

> +                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x0>,
> +                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x0>;
> +        };
> +    };
> --
> 2.7.4
>
Hongtao Wu Sept. 18, 2020, 3:18 a.m. UTC | #2
On Wed, Sep 16, 2020 at 1:25 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Sep 09, 2020 at 05:48:31PM +0800, Hongtao Wu wrote:
> > From: Hongtao Wu <billows.wu@unisoc.com>
> >
> > This series adds PCIe bindings for Unisoc SoCs.
> > This controller is based on DesignWare PCIe IP.
> >
> > Signed-off-by: Hongtao Wu <billows.wu@unisoc.com>
> > ---
> >  .../devicetree/bindings/pci/sprd-pcie.yaml         | 101 +++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/pci/sprd-pcie.yaml b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> > new file mode 100644
> > index 0000000..c52edfb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> > @@ -0,0 +1,101 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/sprd-pcie.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SoC PCIe Host Controller Device Tree Bindings
> > +
> > +maintainers:
> > +  - Hongtao Wu <billows.wu@unisoc.com>
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: sprd,pcie-rc
> > +
> > +  reg:
> > +    minItems: 2
> > +    items:
> > +      - description: Controller control and status registers.
> > +      - description: PCIe configuration registers.
> > +
> > +  reg-names:
> > +    items:
> > +      - const: dbi
> > +      - const: config
> > +
> > +  ranges:
> > +    maxItems: 2
> > +
> > +  num-lanes:
> > +    maximum: 1
> > +    description: Number of lanes to use for this port.
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    description: Builtin MSI controller and PCIe host controller.
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: msi
> > +
> > +  sprd-pcie-poweron-syscons:
>

I am Sorry!
I'll fix it.

> Doesn't match the example.
>
> > +    minItems: 1
> > +    description: Global register.
> > +      The first value is the phandle to the global registers required to
> > +      confige PCIe phy, clock and so on.
> > +      The second value is the global register type which indicates whether it
> > +      is a set/clear register or not.
> > +      The third value is the time to delay after the global register is set or
> > +      cleared.
> > +      The fourth value is the global register address.
> > +      The fifth value is the the mask value that the global register must
> > +      be operate.
> > +      The sixth value is the value that will be set to the global register.
> > +      Note that Some Unisoc global registers have not been upstreamed.
> > +      The global register and its mask can't be found in linux kernel,
> > +      so we use an offset address and a number to instead them.
>
> From the example, it looks like you set/clear 2 bits for power on/off.
> What's the worst case you expect here? What do the 2 bits do? If they
> are for clocks, resets, or power domains, then we have bindings for
> those which should be used. This use of phandles to syscons should be
> avoided whenever possible.
>

There are two kinds of global register ( set/clear registers and
non-set/clear registers )
about PCIe on Unisoc SoCs.
Each set of set/clear registers contain two addresses. One can be
written and the other one
can be read. Different bits in  the set/clear register indicate
different functions, so we
set/clear one bit for power on/off.
The non-set/clear registers are normal which only have one address.

The second value in property 'sprd,pcie-poweron-syscons' is a flag
which indicates whether
the global register is set/clear or not. If this value is 1, we think
that it's a set/clear register.
If this value is 0, we think it's a non-set/clear register.

I wanted to parse all of the global registers about power on/off in an
array (include set/clear
registers and non-set/clear registers). However, it may not be a good idea.
I'll split the property 'sprd,pcie-poweron-syscons' info clocks, power
domains, phy and so on
in the next version.

> If we wanted a language for specifying sequences of register accesses in
> DT, we would have defined that a long time ago.
>

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - num-lanes
> > +  - ranges
> > +  - interrupts
> > +  - interrupt-names
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    ipa {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        pcie0: pcie@2b100000 {
> > +            compatible = "sprd,pcie-rc";
> > +            reg = <0x0 0x2b100000 0x0 0x2000>,
> > +                  <0x2 0x00000000 0x0 0x2000>;
> > +            reg-names = "dbi", "config";
> > +            #address-cells = <3>;
> > +            #size-cells = <2>;
> > +            device_type = "pci";
> > +            ranges = <0x01000000 0x0 0x00000000 0x2 0x00002000 0x0 0x00010000>,
> > +                     <0x03000000 0x0 0x10000000 0x2 0x10000000 0x1 0xefffffff>;
> > +            num-lanes = <1>;
> > +            interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "msi";
> > +
> > +            sprd,pcie-poweron-syscons =
> > +                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x40>,
> > +                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x20>;
> > +            sprd,pcie-poweroff-syscons =
>
> Not documented.
>

Thanks! I'll fix it in the next version.

> > +                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x0>,
> > +                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x0>;
> > +        };
> > +    };
> > --
> > 2.7.4
> >
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/sprd-pcie.yaml b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
new file mode 100644
index 0000000..c52edfb
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
@@ -0,0 +1,101 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/sprd-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SoC PCIe Host Controller Device Tree Bindings
+
+maintainers:
+  - Hongtao Wu <billows.wu@unisoc.com>
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: sprd,pcie-rc
+
+  reg:
+    minItems: 2
+    items:
+      - description: Controller control and status registers.
+      - description: PCIe configuration registers.
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: config
+
+  ranges:
+    maxItems: 2
+
+  num-lanes:
+    maximum: 1
+    description: Number of lanes to use for this port.
+
+  interrupts:
+    minItems: 1
+    description: Builtin MSI controller and PCIe host controller.
+
+  interrupt-names:
+    items:
+      - const: msi
+
+  sprd-pcie-poweron-syscons:
+    minItems: 1
+    description: Global register.
+      The first value is the phandle to the global registers required to
+      confige PCIe phy, clock and so on.
+      The second value is the global register type which indicates whether it
+      is a set/clear register or not.
+      The third value is the time to delay after the global register is set or
+      cleared.
+      The fourth value is the global register address.
+      The fifth value is the the mask value that the global register must
+      be operate.
+      The sixth value is the value that will be set to the global register.
+      Note that Some Unisoc global registers have not been upstreamed.
+      The global register and its mask can't be found in linux kernel,
+      so we use an offset address and a number to instead them.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - num-lanes
+  - ranges
+  - interrupts
+  - interrupt-names
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    ipa {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie0: pcie@2b100000 {
+            compatible = "sprd,pcie-rc";
+            reg = <0x0 0x2b100000 0x0 0x2000>,
+                  <0x2 0x00000000 0x0 0x2000>;
+            reg-names = "dbi", "config";
+            #address-cells = <3>;
+            #size-cells = <2>;
+            device_type = "pci";
+            ranges = <0x01000000 0x0 0x00000000 0x2 0x00002000 0x0 0x00010000>,
+                     <0x03000000 0x0 0x10000000 0x2 0x10000000 0x1 0xefffffff>;
+            num-lanes = <1>;
+            interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "msi";
+
+            sprd,pcie-poweron-syscons =
+                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x40>,
+                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x20>;
+            sprd,pcie-poweroff-syscons =
+                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x0>,
+                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x0>;
+        };
+    };