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

From: Hongtao Wu <wuht06@gmail.com>
To: Rob Herring <robh@kernel.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Orson Zhai <orsonzhai@gmail.com>,
	Baolin Wang <baolin.wang7@gmail.com>,
	Chunyan Zhang <zhang.lyra@gmail.com>,
	linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, Hongtao Wu <billows.wu@unisoc.com>
Subject: Re: [PATCH v3 1/2] dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller
Date: Fri, 18 Sep 2020 11:18:19 +0800	[thread overview]
Message-ID: <CAG_R4_Xv86KE3NQtYEq-3mJjMucrULLVA0=JUkWAVDJPjucKKA@mail.gmail.com> (raw)
In-Reply-To: <20200915172501.GA2146778@bogus>

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
> >