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:

quoted

On Thu, Jun 27, 2024 at 04:51:22PM +0100, Conor Dooley wrote:

quoted

On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote:

quoted

+  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

quoted

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.

quoted

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.

quoted

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)