diff mbox series

[v3,2/6] dt-bindings: pci: add the samsung,exynos-pcie binding

Message ID 20201029134017.27400-3-m.szyprowski@samsung.com
State Accepted
Commit eea23e4a00d479eea6d15a78b79f0c58e8ee4467
Headers show
Series Add DW PCIe support for Exynos5433 SoCs | expand

Commit Message

Marek Szyprowski Oct. 29, 2020, 1:40 p.m. UTC 
Add dt-bindings for the Samsung Exynos PCIe controller (Exynos5433
variant). Based on the text dt-binding posted by Jaehoon Chung.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 .../bindings/pci/samsung,exynos-pcie.yaml     | 119 ++++++++++++++++++
 1 file changed, 119 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml

Comments

Rob Herring (Arm) Nov. 4, 2020, 9:35 p.m. UTC | #1 
On Thu, Oct 29, 2020 at 02:40:13PM +0100, Marek Szyprowski wrote:
> Add dt-bindings for the Samsung Exynos PCIe controller (Exynos5433
> variant). Based on the text dt-binding posted by Jaehoon Chung.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  .../bindings/pci/samsung,exynos-pcie.yaml     | 119 ++++++++++++++++++
>  1 file changed, 119 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml b/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> new file mode 100644
> index 000000000000..1810bf722350
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> @@ -0,0 +1,119 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/samsung,exynos-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung SoC series PCIe Host Controller Device Tree Bindings
> +
> +maintainers:
> +  - Marek Szyprowski <m.szyprowski@samsung.com>
> +  - Jaehoon Chung <jh80.chung@samsung.com>
> +
> +description: |+
> +  Exynos5433 SoC PCIe host controller is based on the Synopsys DesignWare
> +  PCIe IP and thus inherits all the common properties defined in
> +  designware-pcie.txt.
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +
> +properties:
> +  compatible:
> +    const: samsung,exynos5433-pcie
> +
> +  reg:
> +    items:
> +      - description: Data Bus Interface (DBI) registers.
> +      - description: External Local Bus interface (ELBI) registers.
> +      - description: PCIe configuration space region.
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: elbi
> +      - const: config
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: PCIe bridge clock
> +      - description: PCIe bus clock
> +
> +  clock-names:
> +    items:
> +      - const: pcie
> +      - const: pcie_bus
> +
> +  phys:
> +    maxItems: 1
> +
> +  vdd10-supply:
> +    description:
> +      Phandle to a regulator that provides 1.0V power to the PCIe block.
> +
> +  vdd18-supply:
> +    description:
> +      Phandle to a regulator that provides 1.8V power to the PCIe block.
> +
> +  num-lanes:
> +    const: 1
> +
> +  num-viewport:
> +    const: 3

I'm confused why you need this. This is only used with the iATU except 
for keystone. Platforms like Exynos with their own child bus config 
space accessors don't have an iATU. 

BTW, for cases with an iATU, I'm working on making the number of 
viewports runtime detected.

> +
> +required:
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - "#address-cells"
> +  - "#size-cells"
> +  - "#interrupt-cells"
> +  - interrupt-map
> +  - interrupt-map-mask
> +  - ranges
> +  - bus-range
> +  - device_type
> +  - num-lanes
> +  - num-viewport
> +  - clocks
> +  - clock-names
> +  - phys
> +  - vdd10-supply
> +  - vdd18-supply
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/exynos5433.h>
> +
> +    pcie: pcie@15700000 {
> +        compatible = "samsung,exynos5433-pcie";
> +        reg = <0x15700000 0x1000>, <0x156b0000 0x1000>, <0x0c000000 0x1000>;
> +        reg-names = "dbi", "elbi", "config";
> +        #address-cells = <3>;
> +        #size-cells = <2>;
> +        #interrupt-cells = <1>;
> +        device_type = "pci";
> +        interrupts = <GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&cmu_fsys CLK_PCIE>, <&cmu_fsys CLK_PCLK_PCIE_PHY>;
> +        clock-names = "pcie", "pcie_bus";
> +        phys = <&pcie_phy>;
> +        pinctrl-0 = <&pcie_bus &pcie_wlanen>;
> +        pinctrl-names = "default";
> +        num-lanes = <1>;
> +        num-viewport = <3>;
> +        bus-range = <0x00 0xff>;
> +        ranges = <0x81000000 0 0	  0x0c001000 0 0x00010000>,
> +                 <0x82000000 0 0x0c011000 0x0c011000 0 0x03feefff>;
> +        vdd10-supply = <&ldo6_reg>;
> +        vdd18-supply = <&ldo7_reg>;
> +        interrupt-map-mask = <0 0 0 0>;
> +        interrupt-map = <0 0 0 0 &gic GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +...
> -- 
> 2.17.1
>
Marek Szyprowski Nov. 5, 2020, 8:33 a.m. UTC | #2 
Hi Rob,

On 04.11.2020 22:35, Rob Herring wrote:
> On Thu, Oct 29, 2020 at 02:40:13PM +0100, Marek Szyprowski wrote:
>> Add dt-bindings for the Samsung Exynos PCIe controller (Exynos5433
>> variant). Based on the text dt-binding posted by Jaehoon Chung.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
>> ---
>>   .../bindings/pci/samsung,exynos-pcie.yaml     | 119 ++++++++++++++++++
>>   1 file changed, 119 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml

>> ...

>> +  num-viewport:
>> +    const: 3
> I'm confused why you need this. This is only used with the iATU except
> for keystone. Platforms like Exynos with their own child bus config
> space accessors don't have an iATU.

Frankly I have no idea, I don't know much about the PCI internals. After 
rebasing onto your latest DW PCI changes I've noticed a following 
warning message:

exynos-pcie 15700000.pcie: Resources exceed number of ATU entries (2)

Here is a complete log:

# dmesg | grep pci
ehci-pci: EHCI PCI platform driver
ohci-pci: OHCI PCI platform driver
exynos-pcie 15700000.pcie: host bridge /soc@0/pcie@15700000 ranges:
exynos-pcie 15700000.pcie:       IO 0x000c001000..0x000c010fff -> 
0x0000000000
exynos-pcie 15700000.pcie:      MEM 0x000c011000..0x000ffffffe -> 
0x000c011000
exynos-pcie 15700000.pcie: Resources exceed number of ATU entries (2)
exynos-pcie 15700000.pcie: Link up
exynos-pcie 15700000.pcie: PCI host bridge to bus 0000:00
pci_bus 0000:00: root bus resource [bus 00-ff]
pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
pci_bus 0000:00: root bus resource [mem 0x0c011000-0x0ffffffe]
pci 0000:00:00.0: [144d:a5e3] type 01 class 0x060400
pci 0000:00:00.0: PME# supported from D0 D3hot D3cold
pci 0000:01:00.0: [14e4:43e9] type 00 class 0x028000
pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x00007fff 64bit]
pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x003fffff 64bit]
pci 0000:01:00.0: supports D1 D2
pci 0000:01:00.0: PME# supported from D0 D1 D2 D3hot D3cold
pci 0000:00:00.0: BAR 14: assigned [mem 0x0c200000-0x0c7fffff]
pci 0000:01:00.0: BAR 2: assigned [mem 0x0c400000-0x0c7fffff 64bit]
pci 0000:01:00.0: BAR 0: assigned [mem 0x0c200000-0x0c207fff 64bit]
pci 0000:00:00.0: PCI bridge to [bus 01-ff]
pci 0000:00:00.0:   bridge window [mem 0x0c200000-0x0c7fffff]
pci 0000:00:00.0: MSI quirk detected; MSI disabled
pcieport 0000:00:00.0: PME: Signaling with IRQ 97
brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac4358-pcie for chip 
BCM4358/1

When I've increased the numer of viewports it has gone.

If this is not the proper solution, I will removed it.

Best regards
Rob Herring (Arm) Nov. 5, 2020, 3:27 p.m. UTC | #3 
On Thu, Nov 5, 2020 at 2:33 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Rob,
>
> On 04.11.2020 22:35, Rob Herring wrote:
> > On Thu, Oct 29, 2020 at 02:40:13PM +0100, Marek Szyprowski wrote:
> >> Add dt-bindings for the Samsung Exynos PCIe controller (Exynos5433
> >> variant). Based on the text dt-binding posted by Jaehoon Chung.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> >> ---
> >>   .../bindings/pci/samsung,exynos-pcie.yaml     | 119 ++++++++++++++++++
> >>   1 file changed, 119 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
>
> >> ...
>
> >> +  num-viewport:
> >> +    const: 3
> > I'm confused why you need this. This is only used with the iATU except
> > for keystone. Platforms like Exynos with their own child bus config
> > space accessors don't have an iATU.
>
> Frankly I have no idea, I don't know much about the PCI internals.

Sorry, I was confused. It's fine.

Reviewed-by: Rob Herring <robh@kernel.org>

Rob
Han Jingoo Nov. 5, 2020, 5:14 p.m. UTC | #4 
On 11/5/20, 10:27 AM, Rob Herring wrote:
> 
> On Thu, Nov 5, 2020 at 2:33 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> >
> > Hi Rob,
> >
> > On 04.11.2020 22:35, Rob Herring wrote:
> > > On Thu, Oct 29, 2020 at 02:40:13PM +0100, Marek Szyprowski wrote:
> > >> Add dt-bindings for the Samsung Exynos PCIe controller (Exynos5433
> > >> variant). Based on the text dt-binding posted by Jaehoon Chung.
> > >>
> > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > >> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> > >> ---
> > >>   .../bindings/pci/samsung,exynos-pcie.yaml     | 119 ++++++++++++++++++
> > >>   1 file changed, 119 insertions(+)
> > >>   create mode 100644 Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
> >
> > >> ...
> >
> > >> +  num-viewport:
> > >> +    const: 3
> > > I'm confused why you need this. This is only used with the iATU except
> > > for keystone. Platforms like Exynos with their own child bus config
> > > space accessors don't have an iATU.
> >
> > Frankly I have no idea, I don't know much about the PCI internals.
>
> Sorry, I was confused. It's fine.

I was confused, too. But, as far as I remember, I also think that viewpoint-related
setting was necessary for Exynos PCIe.
Thank you.

Best regards,
Jingoo Han

>
> Reviewed-by: Rob Herring <robh@kernel.org>
>
> Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml b/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
new file mode 100644
index 000000000000..1810bf722350
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/samsung,exynos-pcie.yaml
@@ -0,0 +1,119 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/samsung,exynos-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung SoC series PCIe Host Controller Device Tree Bindings
+
+maintainers:
+  - Marek Szyprowski <m.szyprowski@samsung.com>
+  - Jaehoon Chung <jh80.chung@samsung.com>
+
+description: |+
+  Exynos5433 SoC PCIe host controller is based on the Synopsys DesignWare
+  PCIe IP and thus inherits all the common properties defined in
+  designware-pcie.txt.
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+properties:
+  compatible:
+    const: samsung,exynos5433-pcie
+
+  reg:
+    items:
+      - description: Data Bus Interface (DBI) registers.
+      - description: External Local Bus interface (ELBI) registers.
+      - description: PCIe configuration space region.
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: elbi
+      - const: config
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: PCIe bridge clock
+      - description: PCIe bus clock
+
+  clock-names:
+    items:
+      - const: pcie
+      - const: pcie_bus
+
+  phys:
+    maxItems: 1
+
+  vdd10-supply:
+    description:
+      Phandle to a regulator that provides 1.0V power to the PCIe block.
+
+  vdd18-supply:
+    description:
+      Phandle to a regulator that provides 1.8V power to the PCIe block.
+
+  num-lanes:
+    const: 1
+
+  num-viewport:
+    const: 3
+
+required:
+  - reg
+  - reg-names
+  - interrupts
+  - "#address-cells"
+  - "#size-cells"
+  - "#interrupt-cells"
+  - interrupt-map
+  - interrupt-map-mask
+  - ranges
+  - bus-range
+  - device_type
+  - num-lanes
+  - num-viewport
+  - clocks
+  - clock-names
+  - phys
+  - vdd10-supply
+  - vdd18-supply
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/exynos5433.h>
+
+    pcie: pcie@15700000 {
+        compatible = "samsung,exynos5433-pcie";
+        reg = <0x15700000 0x1000>, <0x156b0000 0x1000>, <0x0c000000 0x1000>;
+        reg-names = "dbi", "elbi", "config";
+        #address-cells = <3>;
+        #size-cells = <2>;
+        #interrupt-cells = <1>;
+        device_type = "pci";
+        interrupts = <GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&cmu_fsys CLK_PCIE>, <&cmu_fsys CLK_PCLK_PCIE_PHY>;
+        clock-names = "pcie", "pcie_bus";
+        phys = <&pcie_phy>;
+        pinctrl-0 = <&pcie_bus &pcie_wlanen>;
+        pinctrl-names = "default";
+        num-lanes = <1>;
+        num-viewport = <3>;
+        bus-range = <0x00 0xff>;
+        ranges = <0x81000000 0 0	  0x0c001000 0 0x00010000>,
+                 <0x82000000 0 0x0c011000 0x0c011000 0 0x03feefff>;
+        vdd10-supply = <&ldo6_reg>;
+        vdd18-supply = <&ldo7_reg>;
+        interrupt-map-mask = <0 0 0 0>;
+        interrupt-map = <0 0 0 0 &gic GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH>;
+    };
+...