| Message ID | 20220416135458.104048-2-linux@fw-web.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RK3568 PCIe V3 support | expand |
On 16/04/2022 15:54, Frank Wunderlich wrote: > From: Frank Wunderlich <frank-w@public-files.de> > > Add a new binding file for Rockchip PCIe V3 phy driver. Thank you for your patch. There is something to discuss/improve. > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > --- > .../bindings/phy/rockchip-pcie3-phy.yaml | 77 +++++++++++++++++++ > 1 file changed, 77 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml > > diff --git a/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml b/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml > new file mode 100644 > index 000000000000..58a8ce175f13 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml Filename: vendor,hardware so for example "rockchip,pcie3-phy" although Rob proposed recently for other bindings using compatible as a base: https://lore.kernel.org/linux-devicetree/YlhkwvGdcf4ozTzG@robh.at.kernel.org/ > @@ -0,0 +1,77 @@ > +# SPDX-License-Identifier: GPL-2.0 > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/rockchip-pcie3-phy.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Rockchip PCIe v3 phy > + > +maintainers: > + - Heiko Stuebner <heiko@sntech.de> > + > +properties: > + compatible: > + enum: > + - rockchip,rk3568-pcie3-phy > + - rockchip,rk3588-pcie3-phy > + > + reg: > + maxItems: 2 > + > + clocks: > + minItems: 1 > + maxItems: 3 > + > + clock-names: > + contains: > + anyOf: > + - enum: [ refclk_m, refclk_n, pclk ] The list should be strictly ordered (defined), so: items: - const: ... - const: ... - const: ... minItems: 1 However the question is - why the clocks have different amount? Is it per different SoC implementation? > + > + "#phy-cells": > + const: 0 > + > + resets: > + maxItems: 1 > + > + reset-names: > + const: phy > + > + rockchip,phy-grf: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: phandle to the syscon managing the phy "general register files" > + > + rockchip,pipe-grf: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: phandle to the syscon managing the pipe "general register files" > + > + rockchip,pcie30-phymode: > + $ref: '/schemas/types.yaml#/definitions/uint32' > + description: | > + use PHY_MODE_PCIE_AGGREGATION if not defined I don't understand the description. Do you mean here a case when the variable is missing? > + minimum: 0x0 > + maximum: 0x4 Please explain these values. Register values should not be part of bindings, but instead some logical behavior of hardware or its logic. > + > + Just one blank line. > +required: > + - compatible > + - reg > + - rockchip,phy-grf phy-cells as well > + > +additionalProperties: false > + > +unevaluatedProperties: false Just one please, additionalProperties. > + > +examples: > + - | > + #include <dt-bindings/clock/rk3568-cru.h> > + pcie30phy: phy@fe8c0000 { > + compatible = "rockchip,rk3568-pcie3-phy"; > + reg = <0x0 0xfe8c0000 0x0 0x20000>; > + #phy-cells = <0>; > + clocks = <&pmucru CLK_PCIE30PHY_REF_M>, <&pmucru CLK_PCIE30PHY_REF_N>, > + <&cru PCLK_PCIE30PHY>; Align the entry with opening '<'. Usually the most readable is one clock per line. > + clock-names = "refclk_m", "refclk_n", "pclk"; > + resets = <&cru SRST_PCIE30PHY>; > + reset-names = "phy"; > + rockchip,phy-grf = <&pcie30_phy_grf>; > + }; Best regards, Krzysztof
> Gesendet: Montag, 18. April 2022 um 17:52 Uhr > Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> > > diff --git a/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml b/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml > > new file mode 100644 > > index 000000000000..58a8ce175f13 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml > > Filename: vendor,hardware > so for example "rockchip,pcie3-phy" although Rob proposed recently for > other bindings using compatible as a base: > https://lore.kernel.org/linux-devicetree/YlhkwvGdcf4ozTzG@robh.at.kernel.org/ ok, i rename > > @@ -0,0 +1,77 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/phy/rockchip-pcie3-phy.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Rockchip PCIe v3 phy > > + > > +maintainers: > > + - Heiko Stuebner <heiko@sntech.de> > > + > > +properties: > > + compatible: > > + enum: > > + - rockchip,rk3568-pcie3-phy > > + - rockchip,rk3588-pcie3-phy > > + > > + reg: > > + maxItems: 2 > > + > > + clocks: > > + minItems: 1 > > + maxItems: 3 > > + > > + clock-names: > > + contains: > > + anyOf: > > + - enum: [ refclk_m, refclk_n, pclk ] > > The list should be strictly ordered (defined), so: > items: > - const: ... > - const: ... > - const: ... > minItems: 1 > > However the question is - why the clocks have different amount? Is it > per different SoC implementation? i only know the rk3568, which needs the clocks defined here, don't know about rk3588 yet. in rk3568 TPM i have the pcie-part seems missing (at least the specific register definition), so i had used the driver as i got it from the downstream kernel. not yet looked if i find a rk3588 TPM and if this part is there as i cannot test it (one of the reasons this is a rfc/rft). > > + > > + "#phy-cells": > > + const: 0 > > + > > + resets: > > + maxItems: 1 > > + > > + reset-names: > > + const: phy > > + > > + rockchip,phy-grf: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: phandle to the syscon managing the phy "general register files" > > + > > + rockchip,pipe-grf: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: phandle to the syscon managing the pipe "general register files" > > + > > + rockchip,pcie30-phymode: > > + $ref: '/schemas/types.yaml#/definitions/uint32' > > + description: | > > + use PHY_MODE_PCIE_AGGREGATION if not defined > > I don't understand the description. Do you mean here a case when the > variable is missing? yes, if the property is not set, then value is PHY_MODE_PCIE_AGGREGATION = 4 > > + minimum: 0x0 > > + maximum: 0x4 > > Please explain these values. Register values should not be part of > bindings, but instead some logical behavior of hardware or its logic. it's a bitmask, so maybe description: | bit0: bifurcation for port 0 bit1: bifurcation for port 1 bit2: aggregation use PHY_MODE_PCIE_AGGREGATION (4) as default > > + > > + > > Just one blank line. > > > +required: > > + - compatible > > + - reg > > + - rockchip,phy-grf > > phy-cells as well > > > + > > +additionalProperties: false > > + > > +unevaluatedProperties: false > > Just one please, additionalProperties. ok > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/rk3568-cru.h> > > + pcie30phy: phy@fe8c0000 { > > + compatible = "rockchip,rk3568-pcie3-phy"; > > + reg = <0x0 0xfe8c0000 0x0 0x20000>; > > + #phy-cells = <0>; > > + clocks = <&pmucru CLK_PCIE30PHY_REF_M>, <&pmucru CLK_PCIE30PHY_REF_N>, > > + <&cru PCLK_PCIE30PHY>; > > Align the entry with opening '<'. Usually the most readable is one clock > per line. ok > > + clock-names = "refclk_m", "refclk_n", "pclk"; > > + resets = <&cru SRST_PCIE30PHY>; > > + reset-names = "phy"; > > + rockchip,phy-grf = <&pcie30_phy_grf>; > > + }; regards Frank
On 19/04/2022 19:49, Frank Wunderlich wrote: >> The list should be strictly ordered (defined), so: >> items: >> - const: ... >> - const: ... >> - const: ... >> minItems: 1 >> >> However the question is - why the clocks have different amount? Is it >> per different SoC implementation? > > i only know the rk3568, which needs the clocks defined here, don't know about rk3588 yet. > in rk3568 TPM i have the pcie-part seems missing (at least the specific register definition), so i had used the driver as i got it from the downstream kernel. > > not yet looked if i find a rk3588 TPM and if this part is there as i cannot test it (one of the reasons this is a rfc/rft). You can skip RK3588 compatible or define it this strictly also for that chip. > >>> + >>> + "#phy-cells": >>> + const: 0 >>> + >>> + resets: >>> + maxItems: 1 >>> + >>> + reset-names: >>> + const: phy >>> + >>> + rockchip,phy-grf: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: phandle to the syscon managing the phy "general register files" >>> + >>> + rockchip,pipe-grf: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: phandle to the syscon managing the pipe "general register files" >>> + >>> + rockchip,pcie30-phymode: >>> + $ref: '/schemas/types.yaml#/definitions/uint32' >>> + description: | >>> + use PHY_MODE_PCIE_AGGREGATION if not defined >> >> I don't understand the description. Do you mean here a case when the >> variable is missing? > > yes, if the property is not set, then value is PHY_MODE_PCIE_AGGREGATION = 4 Then just use "default: 4" > >>> + minimum: 0x0 >>> + maximum: 0x4 >> >> Please explain these values. Register values should not be part of >> bindings, but instead some logical behavior of hardware or its logic. > > it's a bitmask, so maybe > > description: | > bit0: bifurcation for port 0 > bit1: bifurcation for port 1 > bit2: aggregation That's good. I got impression you have a header with these values. If yes - mention it here. > use PHY_MODE_PCIE_AGGREGATION (4) as default Just use default as I wrote above. Best regards, Krzysztof
> Gesendet: Dienstag, 19. April 2022 um 21:43 Uhr > Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> > An: "Frank Wunderlich" <frank-w@public-files.de> > Cc: "Frank Wunderlich" <linux@fw-web.de>, linux-rockchip@lists.infradead.org, "Kishon Vijay Abraham I" <kishon@ti.com>, "Vinod Koul" <vkoul@kernel.org>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzk+dt@kernel.org>, "Heiko Stuebner" <heiko@sntech.de>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, "Krzysztof Wilczyński" <kw@linux.com>, "Bjorn Helgaas" <bhelgaas@google.com>, "Philipp Zabel" <p.zabel@pengutronix.de>, "Johan Jonker" <jbx6244@gmail.com>, "Peter Geis" <pgwipeout@gmail.com>, "Michael Riesch" <michael.riesch@wolfvision.net>, linux-phy@lists.infradead.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org > Betreff: Re: Aw: Re: [RFC/RFT 1/6] dt-bindings: phy: rockchip: add pcie3 phy > > On 19/04/2022 19:49, Frank Wunderlich wrote: > >> The list should be strictly ordered (defined), so: > >> items: > >> - const: ... > >> - const: ... > >> - const: ... > >> minItems: 1 > >> > >> However the question is - why the clocks have different amount? Is it > >> per different SoC implementation? > > > > i only know the rk3568, which needs the clocks defined here, don't know about rk3588 yet. > > in rk3568 TPM i have the pcie-part seems missing (at least the specific register definition), so i had used the driver as i got it from the downstream kernel. > > > > not yet looked if i find a rk3588 TPM and if this part is there as i cannot test it (one of the reasons this is a rfc/rft). > > You can skip RK3588 compatible or define it this strictly also for that > chip. currently driver does clk_bulk initialization so i would define it like you suggested (without any SoC specific switch): clocks: minItems: 1 maxItems: 3 clock-names: items: - const: "refclk_m" - const: "refclk_n" - const: "pclk" minItems: 1 > >>> + > >>> + "#phy-cells": > >>> + const: 0 > >>> + > >>> + resets: > >>> + maxItems: 1 > >>> + > >>> + reset-names: > >>> + const: phy > >>> + > >>> + rockchip,phy-grf: > >>> + $ref: /schemas/types.yaml#/definitions/phandle > >>> + description: phandle to the syscon managing the phy "general register files" > >>> + > >>> + rockchip,pipe-grf: > >>> + $ref: /schemas/types.yaml#/definitions/phandle > >>> + description: phandle to the syscon managing the pipe "general register files" > >>> + > >>> + rockchip,pcie30-phymode: > >>> + $ref: '/schemas/types.yaml#/definitions/uint32' > >>> + description: | > >>> + use PHY_MODE_PCIE_AGGREGATION if not defined > >> > >> I don't understand the description. Do you mean here a case when the > >> variable is missing? > > > > yes, if the property is not set, then value is PHY_MODE_PCIE_AGGREGATION = 4 > > Then just use "default: 4" > > > > >>> + minimum: 0x0 > >>> + maximum: 0x4 > >> > >> Please explain these values. Register values should not be part of > >> bindings, but instead some logical behavior of hardware or its logic. > > > > it's a bitmask, so maybe > > > > description: | > > bit0: bifurcation for port 0 > > bit1: bifurcation for port 1 > > bit2: aggregation > > That's good. I got impression you have a header with these values. If > yes - mention it here. > > > use PHY_MODE_PCIE_AGGREGATION (4) as default > > Just use default as I wrote above. so like this? rockchip,pcie30-phymode: $ref: '/schemas/types.yaml#/definitions/uint32' description: | set the phy-mode for enabling bifurcation bit0: bifurcation for port 0 bit1: bifurcation for port 1 bit2: aggregation constants are defined in the dt-bindings/phy/phy-rockchip-pcie3.h minimum: 0x0 maximum: 0x4 default: 0x4 regards Frank
On 19/04/2022 22:36, Frank Wunderlich wrote: >> Just use default as I wrote above. > > so like this? > > rockchip,pcie30-phymode: > $ref: '/schemas/types.yaml#/definitions/uint32' > description: | > set the phy-mode for enabling bifurcation > bit0: bifurcation for port 0 > bit1: bifurcation for port 1 > bit2: aggregation > constants are defined in the dt-bindings/phy/phy-rockchip-pcie3.h > minimum: 0x0 > maximum: 0x4 > default: 0x4 Yes. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml b/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml new file mode 100644 index 000000000000..58a8ce175f13 --- /dev/null +++ b/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml @@ -0,0 +1,77 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/rockchip-pcie3-phy.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Rockchip PCIe v3 phy + +maintainers: + - Heiko Stuebner <heiko@sntech.de> + +properties: + compatible: + enum: + - rockchip,rk3568-pcie3-phy + - rockchip,rk3588-pcie3-phy + + reg: + maxItems: 2 + + clocks: + minItems: 1 + maxItems: 3 + + clock-names: + contains: + anyOf: + - enum: [ refclk_m, refclk_n, pclk ] + + "#phy-cells": + const: 0 + + resets: + maxItems: 1 + + reset-names: + const: phy + + rockchip,phy-grf: + $ref: /schemas/types.yaml#/definitions/phandle + description: phandle to the syscon managing the phy "general register files" + + rockchip,pipe-grf: + $ref: /schemas/types.yaml#/definitions/phandle + description: phandle to the syscon managing the pipe "general register files" + + rockchip,pcie30-phymode: + $ref: '/schemas/types.yaml#/definitions/uint32' + description: | + use PHY_MODE_PCIE_AGGREGATION if not defined + minimum: 0x0 + maximum: 0x4 + + +required: + - compatible + - reg + - rockchip,phy-grf + +additionalProperties: false + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/rk3568-cru.h> + pcie30phy: phy@fe8c0000 { + compatible = "rockchip,rk3568-pcie3-phy"; + reg = <0x0 0xfe8c0000 0x0 0x20000>; + #phy-cells = <0>; + clocks = <&pmucru CLK_PCIE30PHY_REF_M>, <&pmucru CLK_PCIE30PHY_REF_N>, + <&cru PCLK_PCIE30PHY>; + clock-names = "refclk_m", "refclk_n", "pclk"; + resets = <&cru SRST_PCIE30PHY>; + reset-names = "phy"; + rockchip,phy-grf = <&pcie30_phy_grf>; + };