[PATCH 4/4] phy: Realtek Otto Serdes: add devicetree documentation

[Krzysztof Kozlowski]

On 04/10/2024 21:56, Markus Stockhausen wrote:
> To help others to integrate the driver provide the devicetree documentation.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

> ---
>  .../bindings/phy/realtek,otto-serdes.yaml     | 167 ++++++++++++++++++
>  1 file changed, 167 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/realtek,otto-serdes.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/realtek,otto-serdes.yaml b/Documentation/devicetree/bindings/phy/realtek,otto-serdes.yaml
> new file mode 100644
> index 000000000000..b6dad1089c6f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/realtek,otto-serdes.yaml
> @@ -0,0 +1,167 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/realtek,otto-serdes.yaml#

Use compatible as filename.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek Otto SerDes controller
> +
> +maintainers:
> +  - Markus Stockhausen <markus.stockhausen at gmx.de>
> +
> +description: |
> +  The MIPS based Realtek Switch SoCs of the Realtek RTL838x, RTL839x, RTL930x and
> +  RTL931x series have multiple SerDes built in. They are linked to different single,
> +  quad or octa PHYs like the RTL8218B, RTL8218D or RTL8214FC and are the integral
> +  part of the up-to-52-port switch architecture.
> +  Although these SerDes controllers have common basics they behave differently on
> +  the SoC families and rely on heavy register magic. To keep the driver clean it can
> +  load patch sequences from devictree and execute them during the controller actions
> +  like phy_init(), ...
> +  The driver exposes the SerDes registers different from the hardware but instead
> +  gives a consistent view and programming interface. So the RTL838x series has 6 ports
> +  and 4 pages, the RTL839x has 14 ports and 12 pages, the RTL930x has 12 ports and
> +  64 pages and the RTL931x has 14 ports and 192 pages.

Wrap according to Linux coding style.

> +
> +properties:
> +  $nodename:
> +    pattern: "^serdes@[0-9a-f]+$"
> +
> +  compatible:
> +    items:
> +      - enum:
> +          - realtek,rtl8380-serdes
> +          - realtek,rtl8390-serdes
> +          - realtek,rtl9300-serdes
> +          - realtek,rtl9310-serdes
> +
> +  reg:
> +    items:
> +      - description: |

Do not need '|' unless you need to preserve formatting.

> +        The primary serdes register memory location. Other SerDes control and
> +        management registers are distributed all over the I/O memory space and
> +        identified by the driver automatically.
> +
> +  controlled-ports:
> +    description: |
> +      A bit mask defining the ports that are actively controlled by the driver. In
> +      case a bit is not set the driver will only process read operations on the
> +      SerDes. If not set the driver will run all ports in read only mode.

You have never tested it.

> +
> +  "#phy-cells":
> +    const: 4
> +    description: |
> +      The first number defines the SerDes to use. The second number a linked
> +      SerDes. E.g. if a octa 1G PHY is attached to two QSGMII SerDes. The third
> +      number is the first switch port this SerDes is working for, the fourth number
> +      is the last switch port the SerDes is working for.
> +
> +  cmd-setup:
> +    description: |
> +      A field of 16 bit values that contain a patch/command sequence to run on the
> +      SerDes registers during driver setup.

None of these fields match DT. Drop all of driver related stuff.

Due to broken and untested code, that was only limited review.

> +    serdes: serdes at 1b00a000 {
> +      compatible = "realtek,rtl8390-serdes", "realtek,otto-serdes";
> +      reg = <0x1b00a000 0x1c00>;
> +      controlled-ports = <0x3fff>;
> +      #phy-cells = <2>;
> +      cmd-setup = /bits/ 16 <
> +        /*
> +         * set clock edge bit 14 during driver setup for ports 10-11 on page 0,
> +         * register 7. Wait 128 ms. Afterwards set whole register 0 on page 10
> +         * of ports 8, 9, 12, 13 to 0x5800.
> +         */
> +        _MASK_ 0x0c00 0x00 0x07 0x4000 0x4000
> +        _WAIT_ 0x0c00 0x00 0x00 0x0080 0x0000
> +        _MASK_ 0x3300 0x0a 0x00 0x5800 0xffff
> +      >;
> +    };
> \ No newline at end of file

fix errors in your patches.
> --
> 2.44.0
> 
> 

Best regards,
Krzysztof