Subject: Re: [RFC PATCH v4 2/4] dt-bindings: phy: rockchip: Add Naneng combo
 PHY bindings
To:     Vinod Koul <vkoul@kernel.org>
Cc:     heiko@sntech.de, robh+dt@kernel.org, kishon@ti.com,
        p.zabel@pengutronix.de, yifeng.zhao@rock-chips.com,
        kever.yang@rock-chips.com, cl@rock-chips.com,
        linux-phy@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20211208185449.16763-1-jbx6244@gmail.com>
 <20211208185449.16763-3-jbx6244@gmail.com> <Ybhc0VW6JeJ4CNY9@matsya>
From:   Johan Jonker <jbx6244@gmail.com>
Message-ID: <2aef2271-6667-f6bf-0d7d-399b8ec450bf@gmail.com>
Date:   Tue, 14 Dec 2021 13:20:24 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <Ybhc0VW6JeJ4CNY9@matsya>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@rock-chips.com>
>>
>> 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@rock-chips.com>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>>
>> 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@sntech.de>
>> +
>> +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
>