diff mbox series

[net-next,v3,06/10] dt-bindings: net: Add Synopsys DW xPCS bindings

Message ID 20240627004142.8106-7-fancer.lancer@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: pcs: xpcs: Add memory-mapped device support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/apply fail Patch does not apply to net-next-0

Commit Message

Serge Semin June 27, 2024, 12:41 a.m. UTC
Synopsys DesignWare XPCS IP-core is a Physical Coding Sublayer (PCS) layer
providing an interface between the Media Access Control (MAC) and Physical
Medium Attachment Sublayer (PMA) through a Media independent interface.
From software point of view it exposes IEEE std. Clause 45 CSR space and
can be accessible either by MDIO or MCI/APB3 bus interfaces. In the former
case the PCS device is supposed to be defined under the respective MDIO
bus DT-node. In the later case the DW xPCS will be just a normal IO
memory-mapped device.

Besides of that DW XPCS DT-nodes can have an interrupt signal and clock
source properties specified. The former one indicates the Clause 73/37
auto-negotiation events like: negotiation page received, AN is completed
or incompatible link partner. The clock DT-properties can describe up to
three clock sources: peripheral bus clock source, internal reference clock
and the externally connected reference clock.

Finally the DW XPCS IP-core can be optionally synthesized with a
vendor-specific interface connected to the Synopsys PMA (also called
DesignWare Consumer/Enterprise PHY). Alas that isn't auto-detectable in a
portable way. So if the DW XPCS device has the respective PMA attached
then it should be reflected in the DT-node compatible string so the driver
would be aware of the PMA-specific device capabilities (mainly connected
with CSRs available for the fine-tunings).

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---

Changelog v2:
- Drop the Management Interface DT-node bindings. DW xPCS with MCI/APB3
  interface is just a normal memory-mapped device.

Changelog v3:
- Implement the ordered clocks constraint. (@Rob)
---
 .../bindings/net/pcs/snps,dw-xpcs.yaml        | 136 ++++++++++++++++++
 1 file changed, 136 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml

Comments

Conor Dooley June 27, 2024, 3:51 p.m. UTC | #1
On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote:
> +  clocks:
> +    description:
> +      Both MCI and APB3 interfaces are supposed to be equipped with a clock
> +      source connected via the clk_csr_i line.
> +
> +      PCS/PMA layer can be clocked by an internal reference clock source
> +      (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock
> +      generator. Both clocks can be supplied at a time.
> +    minItems: 1
> +    maxItems: 3
> +
> +  clock-names:
> +    oneOf:
> +      - minItems: 1
> +        items:
> +          - enum: [core, pad]
> +          - const: pad
> +      - minItems: 1
> +        items:
> +          - const: pclk
> +          - enum: [core, pad]
> +          - const: pad

While reading this, I'm kinda struggling to map "clk_csr_i" to a clock
name. Is that pclk? And why pclk if it is connected to "clk_csr_i"?
If two interfaces are meant to be "equipped" with that clock, how come
it is optional? I'm probably missing something...

Otherwise this binding looks fine to me.

Wee bit confused,
Conor.
Serge Semin June 27, 2024, 5:10 p.m. UTC | #2
On Thu, Jun 27, 2024 at 04:51:22PM +0100, Conor Dooley wrote:
> On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote:
> > +  clocks:
> > +    description:
> > +      Both MCI and APB3 interfaces are supposed to be equipped with a clock
> > +      source connected via the clk_csr_i line.
> > +
> > +      PCS/PMA layer can be clocked by an internal reference clock source
> > +      (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock
> > +      generator. Both clocks can be supplied at a time.
> > +    minItems: 1
> > +    maxItems: 3
> > +
> > +  clock-names:
> > +    oneOf:
> > +      - minItems: 1
> > +        items:
> > +          - enum: [core, pad]
> > +          - const: pad
> > +      - minItems: 1
> > +        items:
> > +          - const: pclk
> > +          - enum: [core, pad]
> > +          - const: pad
> 

> While reading this, I'm kinda struggling to map "clk_csr_i" to a clock
> name. Is that pclk? And why pclk if it is connected to "clk_csr_i"?

Right. It's "pclk". The reason of using the "pclk" name is that it has
turned to be a de-facto standard name in the DT-bindings for the
peripheral bus clock sources utilized for the CSR-space IO buses.
Moreover the STMMAC driver responsible for the parental DW *MAC
devices handling also has the "pclk" name utilized for the clk_csr_i
signal. So using the "pclk" name in the tightly coupled devices (MAC
and PCS) for the same signal seemed a good idea.

> If two interfaces are meant to be "equipped" with that clock, how come
> it is optional? I'm probably missing something...

MCI and APB3 interfaces are basically the same from the bindings
pointer of view. Both of them can be utilized for the DW XPCS
installed on the SoC system bus, so the device could be accessed using
the simple MMIO ops.

The first "clock-names" schema is meant to be applied on the DW XPCS
accessible over an _MDIO_ bus, which obviously doesn't have any
special CSR IO bus. In that case the DW XPCS device is supposed to be
defined as a subnode of the MDIO-bus DT-node.

The second "clock-names" constraint is supposed to be applied to the
DW XPCS synthesized with the MCI/APB3 CSRs IO interface. The device in
that case should be defined in the DT source file as a normal memory
mapped device.

> 
> Otherwise this binding looks fine to me.

Shall I add a note to the clock description that the "clk_csr_i"
signal is named as "pclk"? I'll need to resubmit the series anyway.

Thanks
-Serge(y)

> 
> Wee bit confused,
> Conor.
Conor Dooley June 28, 2024, 4:42 p.m. UTC | #3
On Thu, Jun 27, 2024 at 08:10:48PM +0300, Serge Semin wrote:
> On Thu, Jun 27, 2024 at 04:51:22PM +0100, Conor Dooley wrote:
> > On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote:
> > > +  clocks:
> > > +    description:
> > > +      Both MCI and APB3 interfaces are supposed to be equipped with a clock
> > > +      source connected via the clk_csr_i line.
> > > +
> > > +      PCS/PMA layer can be clocked by an internal reference clock source
> > > +      (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock
> > > +      generator. Both clocks can be supplied at a time.
> > > +    minItems: 1
> > > +    maxItems: 3
> > > +
> > > +  clock-names:
> > > +    oneOf:
> > > +      - minItems: 1
> > > +        items:
> > > +          - enum: [core, pad]
> > > +          - const: pad
> > > +      - minItems: 1
> > > +        items:
> > > +          - const: pclk
> > > +          - enum: [core, pad]
> > > +          - const: pad
> > 
> 
> > While reading this, I'm kinda struggling to map "clk_csr_i" to a clock
> > name. Is that pclk? And why pclk if it is connected to "clk_csr_i"?
> 
> Right. It's "pclk". The reason of using the "pclk" name is that it has
> turned to be a de-facto standard name in the DT-bindings for the
> peripheral bus clock sources utilized for the CSR-space IO buses.
> Moreover the STMMAC driver responsible for the parental DW *MAC
> devices handling also has the "pclk" name utilized for the clk_csr_i
> signal. So using the "pclk" name in the tightly coupled devices (MAC
> and PCS) for the same signal seemed a good idea.
> 
> > If two interfaces are meant to be "equipped" with that clock, how come
> > it is optional? I'm probably missing something...
> 
> MCI and APB3 interfaces are basically the same from the bindings
> pointer of view. Both of them can be utilized for the DW XPCS
> installed on the SoC system bus, so the device could be accessed using
> the simple MMIO ops.
> 
> The first "clock-names" schema is meant to be applied on the DW XPCS
> accessible over an _MDIO_ bus, which obviously doesn't have any
> special CSR IO bus. In that case the DW XPCS device is supposed to be
> defined as a subnode of the MDIO-bus DT-node.
> 
> The second "clock-names" constraint is supposed to be applied to the
> DW XPCS synthesized with the MCI/APB3 CSRs IO interface. The device in
> that case should be defined in the DT source file as a normal memory
> mapped device.
> 
> > 
> > Otherwise this binding looks fine to me.
> 
> Shall I add a note to the clock description that the "clk_csr_i"
> signal is named as "pclk"? I'll need to resubmit the series anyway.

Better yet, could you mention MDIO? It wasn't clear to me (but I'm just
reviewing bindings not a dwmac-ist) that MCI and APB3 were only two of
the options and that the first clock-names was for MDIO. Maybe something
like:

  clock-names:
    oneOf:
      - minItems: 1
        items: # MDIO
          - enum: [core, pad]
          - const: pad
      - minItems: 1
        items: # MCI or APB
          - const: pclk
          - enum: [core, pad]
          - const: pad
Serge Semin June 28, 2024, 5:06 p.m. UTC | #4
On Fri, Jun 28, 2024 at 05:42:58PM +0100, Conor Dooley wrote:
> On Thu, Jun 27, 2024 at 08:10:48PM +0300, Serge Semin wrote:
> > On Thu, Jun 27, 2024 at 04:51:22PM +0100, Conor Dooley wrote:
> > > On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote:
> > > > +  clocks:
> > > > +    description:
> > > > +      Both MCI and APB3 interfaces are supposed to be equipped with a clock
> > > > +      source connected via the clk_csr_i line.
> > > > +
> > > > +      PCS/PMA layer can be clocked by an internal reference clock source
> > > > +      (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock
> > > > +      generator. Both clocks can be supplied at a time.
> > > > +    minItems: 1
> > > > +    maxItems: 3
> > > > +
> > > > +  clock-names:
> > > > +    oneOf:
> > > > +      - minItems: 1
> > > > +        items:
> > > > +          - enum: [core, pad]
> > > > +          - const: pad
> > > > +      - minItems: 1
> > > > +        items:
> > > > +          - const: pclk
> > > > +          - enum: [core, pad]
> > > > +          - const: pad
> > > 
> > 
> > > While reading this, I'm kinda struggling to map "clk_csr_i" to a clock
> > > name. Is that pclk? And why pclk if it is connected to "clk_csr_i"?
> > 
> > Right. It's "pclk". The reason of using the "pclk" name is that it has
> > turned to be a de-facto standard name in the DT-bindings for the
> > peripheral bus clock sources utilized for the CSR-space IO buses.
> > Moreover the STMMAC driver responsible for the parental DW *MAC
> > devices handling also has the "pclk" name utilized for the clk_csr_i
> > signal. So using the "pclk" name in the tightly coupled devices (MAC
> > and PCS) for the same signal seemed a good idea.
> > 
> > > If two interfaces are meant to be "equipped" with that clock, how come
> > > it is optional? I'm probably missing something...
> > 
> > MCI and APB3 interfaces are basically the same from the bindings
> > pointer of view. Both of them can be utilized for the DW XPCS
> > installed on the SoC system bus, so the device could be accessed using
> > the simple MMIO ops.
> > 
> > The first "clock-names" schema is meant to be applied on the DW XPCS
> > accessible over an _MDIO_ bus, which obviously doesn't have any
> > special CSR IO bus. In that case the DW XPCS device is supposed to be
> > defined as a subnode of the MDIO-bus DT-node.
> > 
> > The second "clock-names" constraint is supposed to be applied to the
> > DW XPCS synthesized with the MCI/APB3 CSRs IO interface. The device in
> > that case should be defined in the DT source file as a normal memory
> > mapped device.
> > 
> > > 
> > > Otherwise this binding looks fine to me.
> > 
> > Shall I add a note to the clock description that the "clk_csr_i"
> > signal is named as "pclk"? I'll need to resubmit the series anyway.
>
 
> Better yet, could you mention MDIO? It wasn't clear to me (but I'm just
> reviewing bindings not a dwmac-ist) that MCI and APB3 were only two of
> the options and that the first clock-names was for MDIO. Maybe something
> like:
> 
>   clock-names:
>     oneOf:
>       - minItems: 1
>         items: # MDIO
>           - enum: [core, pad]
>           - const: pad
>       - minItems: 1
>         items: # MCI or APB
>           - const: pclk
>           - enum: [core, pad]
>           - const: pad

Agreed. I'll add the comments to the oneOf-subschemas as you
suggested.

Thanks,
-Serge(y)
Rob Herring (Arm) June 28, 2024, 10:12 p.m. UTC | #5
On Thu, Jun 27, 2024 at 08:10:48PM +0300, Serge Semin wrote:
> On Thu, Jun 27, 2024 at 04:51:22PM +0100, Conor Dooley wrote:
> > On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote:
> > > +  clocks:
> > > +    description:
> > > +      Both MCI and APB3 interfaces are supposed to be equipped with a clock
> > > +      source connected via the clk_csr_i line.
> > > +
> > > +      PCS/PMA layer can be clocked by an internal reference clock source
> > > +      (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock
> > > +      generator. Both clocks can be supplied at a time.
> > > +    minItems: 1
> > > +    maxItems: 3
> > > +
> > > +  clock-names:
> > > +    oneOf:
> > > +      - minItems: 1
> > > +        items:
> > > +          - enum: [core, pad]
> > > +          - const: pad
> > > +      - minItems: 1
> > > +        items:
> > > +          - const: pclk
> > > +          - enum: [core, pad]
> > > +          - const: pad
> > 
> 
> > While reading this, I'm kinda struggling to map "clk_csr_i" to a clock
> > name. Is that pclk? And why pclk if it is connected to "clk_csr_i"?
> 
> Right. It's "pclk". The reason of using the "pclk" name is that it has
> turned to be a de-facto standard name in the DT-bindings for the
> peripheral bus clock sources utilized for the CSR-space IO buses.
> Moreover the STMMAC driver responsible for the parental DW *MAC
> devices handling also has the "pclk" name utilized for the clk_csr_i
> signal. So using the "pclk" name in the tightly coupled devices (MAC
> and PCS) for the same signal seemed a good idea.

It is? That's really just the name of the bus clock for APB (Arm 
Peripheral Bus). If there's a name that matches the docs, use that. 
Though I'd drop 'clk_' part.

Rob
Serge Semin July 1, 2024, 1:40 a.m. UTC | #6
On Fri, Jun 28, 2024 at 04:12:46PM -0600, Rob Herring wrote:
> On Thu, Jun 27, 2024 at 08:10:48PM +0300, Serge Semin wrote:
> > On Thu, Jun 27, 2024 at 04:51:22PM +0100, Conor Dooley wrote:
> > > On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote:
> > > > +  clocks:
> > > > +    description:
> > > > +      Both MCI and APB3 interfaces are supposed to be equipped with a clock
> > > > +      source connected via the clk_csr_i line.
> > > > +
> > > > +      PCS/PMA layer can be clocked by an internal reference clock source
> > > > +      (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock
> > > > +      generator. Both clocks can be supplied at a time.
> > > > +    minItems: 1
> > > > +    maxItems: 3
> > > > +
> > > > +  clock-names:
> > > > +    oneOf:
> > > > +      - minItems: 1
> > > > +        items:
> > > > +          - enum: [core, pad]
> > > > +          - const: pad
> > > > +      - minItems: 1
> > > > +        items:
> > > > +          - const: pclk
> > > > +          - enum: [core, pad]
> > > > +          - const: pad
> > > 
> > 
> > > While reading this, I'm kinda struggling to map "clk_csr_i" to a clock
> > > name. Is that pclk? And why pclk if it is connected to "clk_csr_i"?
> > 
> > Right. It's "pclk". The reason of using the "pclk" name is that it has
> > turned to be a de-facto standard name in the DT-bindings for the
> > peripheral bus clock sources utilized for the CSR-space IO buses.
> > Moreover the STMMAC driver responsible for the parental DW *MAC
> > devices handling also has the "pclk" name utilized for the clk_csr_i
> > signal. So using the "pclk" name in the tightly coupled devices (MAC
> > and PCS) for the same signal seemed a good idea.
> 

> It is? That's really just the name of the bus clock for APB (Arm 
> Peripheral Bus). If there's a name that matches the docs, use that. 
> Though I'd drop 'clk_' part.

Yes, it's normally should have been utilized for APB, but as I see it
the name utilization has gone wider than to just the ARM Peripheral
bus clock. The DW MAC clock-names DT-property bindings is just one
example of that.

Anyway. Ok. I'll convert the name to "csr". (I'll drop the _i suffix
too since it's obvious that the clock signal is the connected to the
device input pin.)

-Serge(y)

> 
> Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
new file mode 100644
index 000000000000..d0f2c2bfbf35
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
@@ -0,0 +1,136 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pcs/snps,dw-xpcs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys DesignWare Ethernet PCS
+
+maintainers:
+  - Serge Semin <fancer.lancer@gmail.com>
+
+description:
+  Synopsys DesignWare Ethernet Physical Coding Sublayer provides an interface
+  between Media Access Control and Physical Medium Attachment Sublayer through
+  the Media Independent Interface (XGMII, USXGMII, XLGMII, GMII, etc)
+  controlled by means of the IEEE std. Clause 45 registers set. The PCS can be
+  optionally synthesized with a vendor-specific interface connected to
+  Synopsys PMA (also called DesignWare Consumer/Enterprise PHY) although in
+  general it can be used to communicate with any compatible PHY.
+
+  The PCS CSRs can be accessible either over the Ethernet MDIO bus or directly
+  by means of the APB3/MCI interfaces. In the later case the XPCS can be mapped
+  right to the system IO memory space.
+
+properties:
+  compatible:
+    oneOf:
+      - description: Synopsys DesignWare XPCS with none or unknown PMA
+        const: snps,dw-xpcs
+      - description: Synopsys DesignWare XPCS with Consumer Gen1 3G PMA
+        const: snps,dw-xpcs-gen1-3g
+      - description: Synopsys DesignWare XPCS with Consumer Gen2 3G PMA
+        const: snps,dw-xpcs-gen2-3g
+      - description: Synopsys DesignWare XPCS with Consumer Gen2 6G PMA
+        const: snps,dw-xpcs-gen2-6g
+      - description: Synopsys DesignWare XPCS with Consumer Gen4 3G PMA
+        const: snps,dw-xpcs-gen4-3g
+      - description: Synopsys DesignWare XPCS with Consumer Gen4 6G PMA
+        const: snps,dw-xpcs-gen4-6g
+      - description: Synopsys DesignWare XPCS with Consumer Gen5 10G PMA
+        const: snps,dw-xpcs-gen5-10g
+      - description: Synopsys DesignWare XPCS with Consumer Gen5 12G PMA
+        const: snps,dw-xpcs-gen5-12g
+
+  reg:
+    items:
+      - description:
+          In case of the MDIO management interface this just a 5-bits ID
+          of the MDIO bus device. If DW XPCS CSRs space is accessed over the
+          MCI or APB3 management interfaces, then the space mapping can be
+          either 'direct' or 'indirect'. In the former case all Clause 45
+          registers are contiguously mapped within the address space
+          MMD '[20:16]', Reg '[15:0]'. In the later case the space is divided
+          to the multiple 256 register sets. There is a special viewport CSR
+          which is responsible for the set selection. The upper part of
+          the CSR address MMD+REG[20:8] is supposed to be written in there
+          so the corresponding subset would be mapped to the lowest 255 CSRs.
+
+  reg-names:
+    items:
+      - enum: [ direct, indirect ]
+
+  reg-io-width:
+    description:
+      The way the CSRs are mapped to the memory is platform depended. Since
+      each Clause 45 CSR is of 16-bits wide the access instructions must be
+      two bytes aligned at least.
+    default: 2
+    enum: [ 2, 4 ]
+
+  interrupts:
+    description:
+      System interface interrupt output (sbd_intr_o) indicating Clause 73/37
+      auto-negotiation events':' Page received, AN is completed or incompatible
+      link partner.
+    maxItems: 1
+
+  clocks:
+    description:
+      Both MCI and APB3 interfaces are supposed to be equipped with a clock
+      source connected via the clk_csr_i line.
+
+      PCS/PMA layer can be clocked by an internal reference clock source
+      (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock
+      generator. Both clocks can be supplied at a time.
+    minItems: 1
+    maxItems: 3
+
+  clock-names:
+    oneOf:
+      - minItems: 1
+        items:
+          - enum: [core, pad]
+          - const: pad
+      - minItems: 1
+        items:
+          - const: pclk
+          - enum: [core, pad]
+          - const: pad
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    ethernet-pcs@1f05d000 {
+      compatible = "snps,dw-xpcs";
+      reg = <0x1f05d000 0x1000>;
+      reg-names = "indirect";
+
+      reg-io-width = <4>;
+
+      interrupts = <79 IRQ_TYPE_LEVEL_HIGH>;
+
+      clocks = <&ccu_pclk>, <&ccu_core>, <&ccu_pad>;
+      clock-names = "pclk", "core", "pad";
+    };
+  - |
+    mdio-bus {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ethernet-pcs@0 {
+        compatible = "snps,dw-xpcs";
+        reg = <0>;
+
+        clocks = <&ccu_core>, <&ccu_pad>;
+        clock-names = "core", "pad";
+      };
+    };
+...