Hi, On 12/14/21 9:58 AM, Vinod Koul wrote: > On 08-12-21, 19:54, Johan Jonker wrote: >> From: Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx> >> >> Add the compatible strings for the Naneng combo PHY found on rockchip SoC. > > Why is this series still tagged RFC..? The phy DT nodes are urgent in need for other USB3, SATA and PCIe follow up series. When the author doesn't respond for some time I can kick the can a bit if it's for 'little' YAML, C style or DT changes. For larger changes it's better to have the hardware tested as well, so I carefully/politely marked it RFC as I don't know the author's intentions. > >> >> Signed-off-by: Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx> >> Signed-off-by: Johan Jonker <jbx6244@xxxxxxxxx> >> --- >> >> Changed V4: >> restyle >> remove some minItems >> add more properties >> remove reset-names >> move #phy-cells >> add rockchip,rk3568-pipe-grf >> add rockchip,rk3568-pipe-phy-grf >> --- >> .../phy/phy-rockchip-naneng-combphy.yaml | 127 ++++++++++++++++++ >> 1 file changed, 127 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml >> >> diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml >> new file mode 100644 >> index 000000000..d309e2008 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml >> @@ -0,0 +1,127 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/phy/phy-rockchip-naneng-combphy.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Rockchip SoC Naneng Combo Phy Device Tree Bindings >> + >> +maintainers: >> + - Heiko Stuebner <heiko@xxxxxxxxx> >> + >> +properties: >> + compatible: >> + enum: >> + - rockchip,rk3568-naneng-combphy >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + items: >> + - description: reference clock >> + - description: apb clock >> + - description: pipe clock > > no maxItems or minItems for this? Documentation/devicetree/bindings/processed-schema.json "clocks": { "items": [ {}, {}, {} ], "type": "array", "minItems": 3, "maxItems": 3, "additionalItems": false }, With 3 items the properties minItems and maxItems are automatically added. Only when the number of clocks varies for example between 1 and 3 one should add minItems. > >> + >> + clock-names: >> + items: >> + - const: ref >> + - const: apb >> + - const: pipe >> + >> + resets: >> + items: >> + - description: exclusive apb reset line >> + - description: exclusive PHY reset line > > Ditto? > >> + >> + rockchip,dis-u3otg0-port: >> + type: boolean >> + description: >> + Disable the u3otg0 port. > > why not make it explicit and say rockchip,disable-u3otg0-port > > Also why should this port be disabled? >From Rockchip RK3568 Datasheet V1.0-20201210 page 16-17: Multi-PHY0 support one of the following interfaces USB3.0 OTG SATA0 Multi-PHY1 support one of the following interfaces USB3.0 Host SATA1 QSGMII/SGMII Multi-PHY2 support one of the following interfaces PCIe2.1 SATA2 QSGMII/SGMII === Rockchip RK3568 TRM Part1 V1.0-20210111 page 233-234 PIPE_GRF_USB3OTG0_CON1 Address: Operational Base + offset (0x0104) usb3otg0_host_u3_port_disable USB 3.0 SS Port Disable control. 1'b0: Port Enabled 1'b1: Port Disabled page 235-236 PIPE_GRF_USB3OTG1_CON1 Address: Operational Base + offset (0x0144) usb3otg1_host_u3_port_disable USB 3.0 SS Port Disable control. 1'b0: Port Enabled 1'b1: Port Disabled === https://www.cnx-software.com/2020/12/01/rockchip-rk3568-processor-to-power-edge-computing-and-nvr-applications/ https://eji4evk5kxx.exactdn.com/wp-content/uploads/2020/12/RK3568-multiplexed-sata-usb-3.0-pcie.jpg?lossy=1&ssl=1 === USB3.0 OTG, USB3.0 HOT, SATA3.0, PCIE2.1, QSGMII are all multiplexed via three Serdes lanes. The driver in it's current state doesn't keep track of which phy[0-2] node it probes I think. Nodes can be probed in random order, so it's not able to tell if usb3otg0_host_u3_port_disable or usb3otg1_host_u3_port_disable should be used. That why the author probably choose to use a property. Please advise if we need more complex logic, state locking, etc. (Any example from the kernel source for that?) (with more complexity I sould better pass this serie to somebody else) Johan > >> + >> + rockchip,dis-u3otg1-port: >> + type: boolean >> + description: >> + Disable the u3otg1 port. > > ditto > >> + >> + rockchip,enable-ssc: >> + type: boolean >> + description: >> + In U3 and SATA mode the SSC option is already disabled by default. >> + In PCIE mode the option SSC can be enabled. >> + If Spread Spectrum Clocking (SSC) is used it is >> + required that a common reference clock is used by the link partners. >> + Most commercially available platforms with PCIe backplanes use >> + SSC to reduce EMI. >> + >> + rockchip,ext-refclk: >> + type: boolean >> + description: >> + Many PCIe connections, especially backplane connections, >> + require a synchronous reference clock between the two link partners. >> + To achieve this a common clock source, referred to as REFCLK in >> + the PCI Express Card Electromechanical Specification, >> + should be used by both ends of the PCIe link. >> + The PCIe PHY provides 100MHz differential clock output >> + (optional with SSC) in RC mode for system applications. >> + >> + rockchip,pipe-grf: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + Some additional phy settings are accessed through GRF regs. >> + >> + rockchip,pipe-phy-grf: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + Some additional pipe settings are accessed through GRF regs. >> + >> + rockchip,sgmii-mac-sel: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 1] >> + default: 0 >> + description: >> + Select gmac0 or gmac1 to be used as SGMII controller. >> + >> + "#phy-cells": >> + const: 1 >> + >> +required: >> + - compatible >> + - reg >> + - clocks >> + - clock-names >> + - resets >> + - rockchip,pipe-grf >> + - rockchip,pipe-phy-grf >> + - "#phy-cells" >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/rk3568-cru.h> >> + >> + pipegrf: syscon@fdc50000 { >> + compatible = "rockchip,rk3568-pipe-grf", "syscon"; >> + reg = <0xfdc50000 0x1000>; >> + }; >> + >> + pipe_phy_grf0: syscon@fdc70000 { >> + compatible = "rockchip,rk3568-pipe-phy-grf", "syscon"; >> + reg = <0xfdc70000 0x1000>; >> + }; >> + >> + combphy0: phy@fe820000 { >> + compatible = "rockchip,rk3568-naneng-combphy"; >> + reg = <0xfe820000 0x100>; >> + clocks = <&pmucru CLK_PCIEPHY0_REF>, >> + <&cru PCLK_PIPEPHY0>, >> + <&cru PCLK_PIPE>; >> + clock-names = "ref", "apb", "pipe"; >> + assigned-clocks = <&pmucru CLK_PCIEPHY0_REF>; >> + assigned-clock-rates = <100000000>; >> + resets = <&cru SRST_P_PIPEPHY0>, <&cru SRST_PIPEPHY0>; >> + rockchip,pipe-grf = <&pipegrf>; >> + rockchip,pipe-phy-grf = <&pipe_phy_grf0>; >> + #phy-cells = <1>; >> + }; >> -- >> 2.20.1 >
