Message-ID: <CAL_JsqKfWOZeXXxqyKtH98cbccXUoV7djRtxzyoq0hA_qx-bpQ@mail.gmail.com>
Date:   Thu, 21 Nov 2019 13:24:39 -0600
From:   Rob Herring <robh+dt@...nel.org>
To:     Grygorii Strashko <grygorii.strashko@...com>
Cc:     netdev <netdev@...r.kernel.org>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        "David S . Miller" <davem@...emloft.net>,
        Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>,
        Jiri Pirko <jiri@...nulli.us>,
        Florian Fainelli <f.fainelli@...il.com>,
        Sekhar Nori <nsekhar@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-omap <linux-omap@...r.kernel.org>,
        Murali Karicheri <m-karicheri2@...com>,
        Ivan Vecera <ivecera@...hat.com>, devicetree@...r.kernel.org
Subject: Re: [PATCH v7 net-next 06/13] dt-bindings: net: ti: add new cpsw
 switch driver bindings

On Tue, Nov 19, 2019 at 4:19 PM Grygorii Strashko
<grygorii.strashko@...com> wrote:
>
> Add bindings for the new TI CPSW switch driver. Comparing to the legacy
> bindings (net/cpsw.txt):
> - ports definition follows DSA bindings (net/dsa/dsa.txt) and ports can be
> marked as "disabled" if not physically wired.
> - all deprecated properties dropped;
> - all legacy propertiies dropped which represent constant HW cpapbilities
> (cpdma_channels, ale_entries, bd_ram_size, mac_control, slaves,
> active_slave)
> - TI CPTS DT properties are reused as is, but grouped in "cpts" sub-node
> - TI Davinci MDIO DT bindings are reused as is, because Davinci MDIO is
> reused.
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@...com>
> ---
>  .../bindings/net/ti,cpsw-switch.yaml          | 240 ++++++++++++++++++
>  1 file changed, 240 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/ti,cpsw-switch.yaml

I see David has applied this already, but it still has numerous
problems. Please send a follow-up.

>
> diff --git a/Documentation/devicetree/bindings/net/ti,cpsw-switch.yaml b/Documentation/devicetree/bindings/net/ti,cpsw-switch.yaml
> new file mode 100644
> index 000000000000..81ae8cafabc1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,cpsw-switch.yaml
> @@ -0,0 +1,240 @@
> +# SPDX-License-Identifier: GPL-2.0

For new bindings, please dual license:

(GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/ti,cpsw-switch.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI SoC Ethernet Switch Controller (CPSW) Device Tree Bindings
> +
> +maintainers:
> +  - Grygorii Strashko <grygorii.strashko@...com>
> +  - Sekhar Nori <nsekhar@...com>
> +
> +description:
> +  The 3-port switch gigabit ethernet subsystem provides ethernet packet
> +  communication and can be configured as an ethernet switch. It provides the
> +  gigabit media independent interface (GMII),reduced gigabit media
> +  independent interface (RGMII), reduced media independent interface (RMII),
> +  the management data input output (MDIO) for physical layer device (PHY)
> +  management.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: ti,cpsw-switch
> +      - items:
> +         - const: ti,am335x-cpsw-switch
> +         - const: ti,cpsw-switch
> +      - items:
> +        - const: ti,am4372-cpsw-switch
> +        - const: ti,cpsw-switch
> +      - items:
> +        - const: ti,dra7-cpsw-switch
> +        - const: ti,cpsw-switch
> +
> +  reg:
> +    maxItems: 1
> +    description:
> +       The physical base address and size of full the CPSW module IO range
> +
> +  ranges: true
> +
> +  clocks:
> +    maxItems: 1
> +    description: CPSW functional clock
> +
> +  clock-names:
> +    maxItems: 1
> +    items:
> +      - const: fck
> +
> +  interrupts:
> +    items:
> +      - description: RX_THRESH interrupt
> +      - description: RX interrupt
> +      - description: TX interrupt
> +      - description: MISC interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: "rx_thresh"
> +      - const: "rx"
> +      - const: "tx"
> +      - const: "misc"
> +
> +  pinctrl-names: true
> +
> +  syscon:
> +    $ref: /schemas/types.yaml#definitions/phandle
> +    description:
> +      Phandle to the system control device node which provides access to
> +      efuse IO range with MAC addresses

Can't you use nvmem binding for this?

> +
> +

Drop the extra blank line.

> +  ethernet-ports:
> +    type: object
> +    properties:
> +      '#address-cells':
> +        const: 1
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      "^port@[0-9]+$":
> +          type: object

> +          minItems: 1
> +          maxItems: 2

These apply to arrays and don't make sense here.

> +          description: CPSW external ports
> +
> +          allOf:
> +            - $ref: ethernet-controller.yaml#
> +
> +          properties:
> +            reg:
> +              maxItems: 1
> +              enum: [1, 2]

You're mixing array and scalar constraints. What you want is:

reg:
  items:
    - enum: [1, 2]

> +              description: CPSW port number
> +
> +            phys:
> +              $ref: /schemas/types.yaml#definitions/phandle-array

Drop this. 'phys' is standard and has a type definition already.

> +              maxItems: 1
> +              description:  phandle on phy-gmii-sel PHY
> +
> +            label:
> +              $ref: /schemas/types.yaml#/definitions/string-array

Same here.

> +              maxItems: 1

This too. 'label' should never be an array.

> +              description: label associated with this port
> +
> +            ti,dual-emac-pvid:
> +              $ref: /schemas/types.yaml#/definitions/uint32
> +              maxItems: 1
> +              minimum: 1
> +              maximum: 1024

allOf:
  - $ref: /schemas/types.yaml#/definitions/uint32
minimum: 1
maximum: 1024

> +              description:
> +                Specifies default PORT VID to be used to segregate
> +                ports. Default value - CPSW port number.
> +
> +          required:
> +            - reg
> +            - phys
> +
> +  mdio:
> +    type: object
> +    allOf:
> +      - $ref: "ti,davinci-mdio.yaml#"
> +    description:
> +      CPSW MDIO bus.
> +
> +  cpts:
> +    type: object
> +    description:
> +      The Common Platform Time Sync (CPTS) module
> +
> +    properties:
> +      clocks:
> +        maxItems: 1
> +        description: CPTS reference clock
> +
> +      clock-names:
> +        maxItems: 1

Can drop this. Implied by 'items' list length.

> +        items:
> +          - const: cpts
> +
> +      cpts_clock_mult:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          Numerator to convert input clock ticks into ns
> +
> +      cpts_clock_shift:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          Denominator to convert input clock ticks into ns.
> +          Mult and shift will be calculated basing on CPTS rftclk frequency if
> +          both cpts_clock_shift and cpts_clock_mult properties are not provided.
> +
> +    required:
> +      - clocks
> +      - clock-names
> +
> +required:
> +  - compatible
> +  - reg
> +  - ranges
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interrupt-names
> +  - '#address-cells'
> +  - '#size-cells'
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/dra7.h>
> +
> +    mac_sw: switch@0 {
> +        compatible = "ti,dra7-cpsw-switch","ti,cpsw-switch";
> +        reg = <0x0 0x4000>;
> +        ranges = <0 0 0x4000>;
> +        clocks = <&gmac_main_clk>;
> +        clock-names = "fck";
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        syscon = <&scm_conf>;
> +        inctrl-names = "default", "sleep";
> +
> +        interrupts = <GIC_SPI 334 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 335 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 336 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 337 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupt-names = "rx_thresh", "rx", "tx", "misc";
> +
> +        ethernet-ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                cpsw_port1: port@1 {
> +                        reg = <1>;
> +                        label = "port1";
> +                        mac-address = [ 00 00 00 00 00 00 ];
> +                        phys = <&phy_gmii_sel 1>;
> +                        phy-handle = <&ethphy0_sw>;
> +                        phy-mode = "rgmii";
> +                        ti,dual_emac_pvid = <1>;
> +                };
> +
> +                cpsw_port2: port@2 {
> +                        reg = <2>;
> +                        label = "wan";
> +                        mac-address = [ 00 00 00 00 00 00 ];
> +                        phys = <&phy_gmii_sel 2>;
> +                        phy-handle = <&ethphy1_sw>;
> +                        phy-mode = "rgmii";
> +                        ti,dual_emac_pvid = <2>;
> +                };
> +        };
> +
> +        davinci_mdio_sw: mdio@...0 {
> +                compatible = "ti,cpsw-mdio","ti,davinci_mdio";
> +                reg = <0x1000 0x100>;
> +                clocks = <&gmac_clkctrl DRA7_GMAC_GMAC_CLKCTRL 0>;
> +                clock-names = "fck";
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                bus_freq = <1000000>;
> +
> +                ethphy0_sw: ethernet-phy@0 {
> +                        reg = <0>;
> +                };
> +
> +                ethphy1_sw: ethernet-phy@1 {
> +                        reg = <1>;
> +                };
> +        };
> +
> +        cpts {
> +                clocks = <&gmac_clkctrl DRA7_GMAC_GMAC_CLKCTRL 25>;
> +                clock-names = "cpts";
> +        };
> +    };
> --
> 2.17.1
>

Powered by blists - more mailing lists