| Message ID | 20240412125818.17052-2-cassel@kernel.org |
|---|---|
| State | Accepted |
| Commit | 46492d10067660785a09db4ce9244545126a17b8 |
| Headers | show |
| Series | rockchip pcie3-phy separate refclk support | expand |
Hi, On Fri, Apr 12, 2024 at 02:58:15PM +0200, Niklas Cassel wrote: > From the RK3588 Technical Reference Manual, Part1, > section 6.19 PCIe3PHY_GRF Register Description: > "rxX_cmn_refclk_mode" > RX common reference clock mode for lane X. This mode should be enabled > only when the far-end and near-end devices are running with a common > reference clock. > > The hardware reset value for this field is 0x1 (enabled). > Note that this register field is only available on RK3588, not on RK3568. > > The link training either fails or is highly unstable (link state will jump > continuously between L0 and recovery) when this mode is enabled while > using an endpoint running in Separate Reference Clock with No SSC (SRNS) > mode or Separate Reference Clock with SSC (SRIS) mode. > (Which is usually the case when using a real SoC as endpoint, e.g. the > RK3588 PCIe controller can run in both Root Complex and Endpoint mode.) > > Add a rockchip specific property to enable/disable the rxX_cmn_refclk_mode > per lane. (Since this PHY supports bifurcation.) > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > .../devicetree/bindings/phy/rockchip,pcie3-phy.yaml | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml > index c4fbffcde6e4..ba67dca5a446 100644 > --- a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml > +++ b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml > @@ -54,6 +54,16 @@ properties: > $ref: /schemas/types.yaml#/definitions/phandle > description: phandle to the syscon managing the pipe "general register files" > > + rockchip,rx-common-refclk-mode: > + description: which lanes (by position) should be configured to run in > + RX common reference clock mode. 0 means disabled, 1 means enabled. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + minItems: 1 > + maxItems: 16 > + items: > + minimum: 0 > + maximum: 1 Why is this not simply using a single u32 with each bit standing for one PCIe lane? I.e. like this: rockchip,rx-common-refclk-mode: description: bitmap describing which lanes should be configured to run in RX common reference clock mode. Bit offset maps to PCIe lanes and a bit set means enabled, unset bit means disabled. $ref: /schemas/types.yaml#/definitions/uint32 minimum: 0 maximum: 0xffff default: 0xffff Apart from that the PHY only supports up to 4 lanes on all existing hardware, so 0xf should be enough? Greetings, -- Sebastian
On Fri, Apr 12, 2024 at 03:36:11PM +0200, Sebastian Reichel wrote: > Hi, > > On Fri, Apr 12, 2024 at 02:58:15PM +0200, Niklas Cassel wrote: > > From the RK3588 Technical Reference Manual, Part1, > > section 6.19 PCIe3PHY_GRF Register Description: > > "rxX_cmn_refclk_mode" > > RX common reference clock mode for lane X. This mode should be enabled > > only when the far-end and near-end devices are running with a common > > reference clock. > > > > The hardware reset value for this field is 0x1 (enabled). > > Note that this register field is only available on RK3588, not on RK3568. > > > > The link training either fails or is highly unstable (link state will jump > > continuously between L0 and recovery) when this mode is enabled while > > using an endpoint running in Separate Reference Clock with No SSC (SRNS) > > mode or Separate Reference Clock with SSC (SRIS) mode. > > (Which is usually the case when using a real SoC as endpoint, e.g. the > > RK3588 PCIe controller can run in both Root Complex and Endpoint mode.) > > > > Add a rockchip specific property to enable/disable the rxX_cmn_refclk_mode > > per lane. (Since this PHY supports bifurcation.) > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > .../devicetree/bindings/phy/rockchip,pcie3-phy.yaml | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml > > index c4fbffcde6e4..ba67dca5a446 100644 > > --- a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml > > +++ b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml > > @@ -54,6 +54,16 @@ properties: > > $ref: /schemas/types.yaml#/definitions/phandle > > description: phandle to the syscon managing the pipe "general register files" > > > > + rockchip,rx-common-refclk-mode: > > + description: which lanes (by position) should be configured to run in > > + RX common reference clock mode. 0 means disabled, 1 means enabled. > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > + minItems: 1 > > + maxItems: 16 > > + items: > > + minimum: 0 > > + maximum: 1 > > Why is this not simply using a single u32 with each bit standing for > one PCIe lane? I.e. like this: Hello Sebastian, The reason for the existing way to specify each lane in an int32-array is to be consistent with the existing property "data-lanes", which already uses this representation. e.g. data-lanes = <1 1 2 2>; rockchip,rx-common-refclk-mode = <0 0 1 1>; It would be very weird if this was instead: data-lanes = <1 1 2 2>; rockchip,rx-common-refclk-mode = 0xc; > > rockchip,rx-common-refclk-mode: > description: bitmap describing which lanes should be configured to run > in RX common reference clock mode. Bit offset maps to PCIe lanes and > a bit set means enabled, unset bit means disabled. > $ref: /schemas/types.yaml#/definitions/uint32 > minimum: 0 > maximum: 0xffff > default: 0xffff > > Apart from that the PHY only supports up to 4 lanes on all existing hardware, > so 0xf should be enough? Again, in order to be consistent with the existing "data-lanes" property in this binding, which defines: minItems: 2 maxItems: 16 which means that the binding already supports up to 16 lanes. (The reason why "data-lanes" specifies minItems:2 is because bifurcation doesn't make sense if you just have a single lane. The rx-common-refclk-mode property however makes sense even in the case where there is just a single lane.) Kind regards, Niklas
Hi, On Fri, Apr 12, 2024 at 04:03:48PM +0200, Niklas Cassel wrote: > On Fri, Apr 12, 2024 at 03:36:11PM +0200, Sebastian Reichel wrote: > > On Fri, Apr 12, 2024 at 02:58:15PM +0200, Niklas Cassel wrote: > > > From the RK3588 Technical Reference Manual, Part1, > > > section 6.19 PCIe3PHY_GRF Register Description: > > > "rxX_cmn_refclk_mode" > > > RX common reference clock mode for lane X. This mode should be enabled > > > only when the far-end and near-end devices are running with a common > > > reference clock. > > > > > > The hardware reset value for this field is 0x1 (enabled). > > > Note that this register field is only available on RK3588, not on RK3568. > > > > > > The link training either fails or is highly unstable (link state will jump > > > continuously between L0 and recovery) when this mode is enabled while > > > using an endpoint running in Separate Reference Clock with No SSC (SRNS) > > > mode or Separate Reference Clock with SSC (SRIS) mode. > > > (Which is usually the case when using a real SoC as endpoint, e.g. the > > > RK3588 PCIe controller can run in both Root Complex and Endpoint mode.) > > > > > > Add a rockchip specific property to enable/disable the rxX_cmn_refclk_mode > > > per lane. (Since this PHY supports bifurcation.) > > > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > --- > > > .../devicetree/bindings/phy/rockchip,pcie3-phy.yaml | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml > > > index c4fbffcde6e4..ba67dca5a446 100644 > > > --- a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml > > > +++ b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml > > > @@ -54,6 +54,16 @@ properties: > > > $ref: /schemas/types.yaml#/definitions/phandle > > > description: phandle to the syscon managing the pipe "general register files" > > > > > > + rockchip,rx-common-refclk-mode: > > > + description: which lanes (by position) should be configured to run in > > > + RX common reference clock mode. 0 means disabled, 1 means enabled. > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > + minItems: 1 > > > + maxItems: 16 > > > + items: > > > + minimum: 0 > > > + maximum: 1 > > > > Why is this not simply using a single u32 with each bit standing for > > one PCIe lane? I.e. like this: > > Hello Sebastian, > > The reason for the existing way to specify each lane in an int32-array > is to be consistent with the existing property "data-lanes", > which already uses this representation. > > e.g. > data-lanes = <1 1 2 2>; > rockchip,rx-common-refclk-mode = <0 0 1 1>; > > It would be very weird if this was instead: > data-lanes = <1 1 2 2>; > rockchip,rx-common-refclk-mode = 0xc; > > > > > > rockchip,rx-common-refclk-mode: > > description: bitmap describing which lanes should be configured to run > > in RX common reference clock mode. Bit offset maps to PCIe lanes and > > a bit set means enabled, unset bit means disabled. > > $ref: /schemas/types.yaml#/definitions/uint32 > > minimum: 0 > > maximum: 0xffff > > default: 0xffff > > > > Apart from that the PHY only supports up to 4 lanes on all existing hardware, > > so 0xf should be enough? > > Again, in order to be consistent with the existing "data-lanes" property in > this binding, which defines: > minItems: 2 > maxItems: 16 > which means that the binding already supports up to 16 lanes. > (The reason why "data-lanes" specifies minItems:2 is because bifurcation > doesn't make sense if you just have a single lane. The rx-common-refclk-mode > property however makes sense even in the case where there is just a single > lane.) Works for me: Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com> -- Sebastian
diff --git a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml index c4fbffcde6e4..ba67dca5a446 100644 --- a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml +++ b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml @@ -54,6 +54,16 @@ properties: $ref: /schemas/types.yaml#/definitions/phandle description: phandle to the syscon managing the pipe "general register files" + rockchip,rx-common-refclk-mode: + description: which lanes (by position) should be configured to run in + RX common reference clock mode. 0 means disabled, 1 means enabled. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 1 + maxItems: 16 + items: + minimum: 0 + maximum: 1 + required: - compatible - reg