diff mbox series

[v6,03/12] dt-bindings: arm: mediatek: sgmiisys: Convert to DT schema

Message ID f4b378f4b19064df85d529973ed6c73ae7aa9f2d.1676323692.git.daniel@makrotopia.org
State New
Headers
Series net: ethernet: mtk_eth_soc: various enhancements |

Commit Message

Daniel Golle Feb. 13, 2023, 9:34 p.m. UTC 
  Convert mediatek,sgmiiisys bindings to DT schema format.
Add maintainer Matthias Brugger, no maintainers were listed in the
original documentation.
As this node is also referenced by the Ethernet controller and used
as SGMII PCS add this fact to the description.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../arm/mediatek/mediatek,sgmiisys.txt        | 27 ----------
 .../arm/mediatek/mediatek,sgmiisys.yaml       | 49 +++++++++++++++++++
 2 files changed, 49 insertions(+), 27 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.yaml

Comments

Rob Herring Feb. 15, 2023, 8:43 p.m. UTC | #1 
On Mon, Feb 13, 2023 at 09:34:43PM +0000, Daniel Golle wrote:
> Convert mediatek,sgmiiisys bindings to DT schema format.
> Add maintainer Matthias Brugger, no maintainers were listed in the
> original documentation.
> As this node is also referenced by the Ethernet controller and used
> as SGMII PCS add this fact to the description.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  .../arm/mediatek/mediatek,sgmiisys.txt        | 27 ----------
>  .../arm/mediatek/mediatek,sgmiisys.yaml       | 49 +++++++++++++++++++
>  2 files changed, 49 insertions(+), 27 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.yaml

If you respin or as a follow-up, can you move this to bindings/clock/?

Reviewed-by: Rob Herring <robh@kernel.org>
Daniel Golle Feb. 15, 2023, 9:09 p.m. UTC | #2 
On Wed, Feb 15, 2023 at 02:43:18PM -0600, Rob Herring wrote:
> On Mon, Feb 13, 2023 at 09:34:43PM +0000, Daniel Golle wrote:
> > Convert mediatek,sgmiiisys bindings to DT schema format.
> > Add maintainer Matthias Brugger, no maintainers were listed in the
> > original documentation.
> > As this node is also referenced by the Ethernet controller and used
> > as SGMII PCS add this fact to the description.
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  .../arm/mediatek/mediatek,sgmiisys.txt        | 27 ----------
> >  .../arm/mediatek/mediatek,sgmiisys.yaml       | 49 +++++++++++++++++++
> >  2 files changed, 49 insertions(+), 27 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> >  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.yaml
> 
> If you respin or as a follow-up, can you move this to bindings/clock/?

Just in time, I was about to send v7, now moved the file as requested.

Thank you for reviewing!

> 
> Reviewed-by: Rob Herring <robh@kernel.org>
Russell King (Oracle) Feb. 15, 2023, 9:16 p.m. UTC | #3 
On Wed, Feb 15, 2023 at 02:43:18PM -0600, Rob Herring wrote:
> On Mon, Feb 13, 2023 at 09:34:43PM +0000, Daniel Golle wrote:
> > Convert mediatek,sgmiiisys bindings to DT schema format.
> > Add maintainer Matthias Brugger, no maintainers were listed in the
> > original documentation.
> > As this node is also referenced by the Ethernet controller and used
> > as SGMII PCS add this fact to the description.
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  .../arm/mediatek/mediatek,sgmiisys.txt        | 27 ----------
> >  .../arm/mediatek/mediatek,sgmiisys.yaml       | 49 +++++++++++++++++++
> >  2 files changed, 49 insertions(+), 27 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> >  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.yaml
> 
> If you respin or as a follow-up, can you move this to bindings/clock/?

I'm not sure that's appropriate. Let's take the MT7622 as an example,
here is the extract from the device tree for this:

        sgmiisys: sgmiisys@1b128000 {
                compatible = "mediatek,mt7622-sgmiisys",
                             "syscon";
                reg = <0 0x1b128000 0 0x3000>;
                #clock-cells = <1>;
        };

This makes it look primarily like a clock controller, but when I look
at the MT7622 documentation, this region is described as the
"Serial Gigabit Media Independent Interface".

If we delve a little deeper and look at the code we have in the kernel,
yes, there is a clock driver, but there is also the SGMII code which is
wrapped up into the mtk_eth_soc driver - and the only user of the
clocks provided by the sgmiisys is the ethernet driver.

To me, this looks very much like a case of "lets use the clock API
because it says we have clocks inside this module" followed by "now
how can we make it work with DT with a separate clock driver".

In other words, I believe that describing this hardware as something
that is primarily to do with clocks is wrong. It looks to me more
like the hardware is primarily a PCS that happens to provide some
clocks to the ethernet subsystem that is attached to it.

Why do I say this? There are 23 documented PCS registers in the
0x1b128000 block, and there is one single register which has a bunch
of bits that enable the various clocks that is used by its clock
driver.

Hence, I put forward that:

"The MediaTek SGMIISYS controller provides various clocks to the system."

is quite misleading, and it should be described as:

"The MediaTek SGMIISYS controller provides a SGMII PCS and some clocks
to the ethernet subsystem to which it is attached."

and a PCS providing clocks to the ethernet subsystem is nothing
really new - we just don't use the clk API to describe them, and
thus don't normally need to throw a syscon thing in there to share
the register space between two drivers.

So, in summary, I don't think moving this to "bindings/clock/" makes
any sense what so ever, and that is probably being based on a
misleading description of what this hardware is and the code structure
adopted in the kernel.

Yes, DT describes the hardware. That's exactly the point I'm making.
It seems that the decision here to classify it has a clock driver is
being made based off the kernel implementation, not what the hardware
actually is.
Rob Herring Feb. 15, 2023, 9:29 p.m. UTC | #4 
On Wed, Feb 15, 2023 at 3:16 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Wed, Feb 15, 2023 at 02:43:18PM -0600, Rob Herring wrote:
> > On Mon, Feb 13, 2023 at 09:34:43PM +0000, Daniel Golle wrote:
> > > Convert mediatek,sgmiiisys bindings to DT schema format.
> > > Add maintainer Matthias Brugger, no maintainers were listed in the
> > > original documentation.
> > > As this node is also referenced by the Ethernet controller and used
> > > as SGMII PCS add this fact to the description.
> > >
> > > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > > ---
> > >  .../arm/mediatek/mediatek,sgmiisys.txt        | 27 ----------
> > >  .../arm/mediatek/mediatek,sgmiisys.yaml       | 49 +++++++++++++++++++
> > >  2 files changed, 49 insertions(+), 27 deletions(-)
> > >  delete mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> > >  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.yaml
> >
> > If you respin or as a follow-up, can you move this to bindings/clock/?
>
> I'm not sure that's appropriate. Let's take the MT7622 as an example,
> here is the extract from the device tree for this:
>
>         sgmiisys: sgmiisys@1b128000 {
>                 compatible = "mediatek,mt7622-sgmiisys",
>                              "syscon";
>                 reg = <0 0x1b128000 0 0x3000>;
>                 #clock-cells = <1>;
>         };
>
> This makes it look primarily like a clock controller, but when I look
> at the MT7622 documentation, this region is described as the
> "Serial Gigabit Media Independent Interface".
>
> If we delve a little deeper and look at the code we have in the kernel,
> yes, there is a clock driver, but there is also the SGMII code which is
> wrapped up into the mtk_eth_soc driver - and the only user of the
> clocks provided by the sgmiisys is the ethernet driver.
>
> To me, this looks very much like a case of "lets use the clock API
> because it says we have clocks inside this module" followed by "now
> how can we make it work with DT with a separate clock driver".
>
> In other words, I believe that describing this hardware as something
> that is primarily to do with clocks is wrong. It looks to me more
> like the hardware is primarily a PCS that happens to provide some
> clocks to the ethernet subsystem that is attached to it.
>
> Why do I say this? There are 23 documented PCS registers in the
> 0x1b128000 block, and there is one single register which has a bunch
> of bits that enable the various clocks that is used by its clock
> driver.
>
> Hence, I put forward that:
>
> "The MediaTek SGMIISYS controller provides various clocks to the system."
>
> is quite misleading, and it should be described as:

Indeed I was...

>
> "The MediaTek SGMIISYS controller provides a SGMII PCS and some clocks
> to the ethernet subsystem to which it is attached."

+1

> and a PCS providing clocks to the ethernet subsystem is nothing
> really new - we just don't use the clk API to describe them, and
> thus don't normally need to throw a syscon thing in there to share
> the register space between two drivers.

Humm, yes. Just like phys that provide clocks.

If PCS is the main function, then it should go in the PCS directory:
bindings/net/pcs/

> So, in summary, I don't think moving this to "bindings/clock/" makes
> any sense what so ever, and that is probably being based on a
> misleading description of what this hardware is and the code structure
> adopted in the kernel.
>
> Yes, DT describes the hardware. That's exactly the point I'm making.
> It seems that the decision here to classify it has a clock driver is
> being made based off the kernel implementation, not what the hardware
> actually is.

Right. I'm just trying to get misc blocks out of bindings/arm/.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
deleted file mode 100644
index d2c24c277514..000000000000
--- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
+++ /dev/null
@@ -1,27 +0,0 @@ 
-MediaTek SGMIISYS controller
-============================
-
-The MediaTek SGMIISYS controller provides various clocks to the system.
-
-Required Properties:
-
-- compatible: Should be:
-	- "mediatek,mt7622-sgmiisys", "syscon"
-	- "mediatek,mt7629-sgmiisys", "syscon"
-	- "mediatek,mt7981-sgmiisys_0", "syscon"
-	- "mediatek,mt7981-sgmiisys_1", "syscon"
-	- "mediatek,mt7986-sgmiisys_0", "syscon"
-	- "mediatek,mt7986-sgmiisys_1", "syscon"
-- #clock-cells: Must be 1
-
-The SGMIISYS controller uses the common clk binding from
-Documentation/devicetree/bindings/clock/clock-bindings.txt
-The available clocks are defined in dt-bindings/clock/mt*-clk.h.
-
-Example:
-
-sgmiisys: sgmiisys@1b128000 {
-	compatible = "mediatek,mt7622-sgmiisys", "syscon";
-	reg = <0 0x1b128000 0 0x1000>;
-	#clock-cells = <1>;
-};
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.yaml
new file mode 100644
index 000000000000..6bb84fe23de4
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.yaml
@@ -0,0 +1,49 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/arm/mediatek/mediatek,sgmiisys.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: MediaTek SGMIISYS Controller
+
+maintainers:
+  - Matthias Brugger <matthias.bgg@gmail.com>
+
+description:
+  The MediaTek SGMIISYS controller provides SGMII related clocks to the system
+  and is used by the Ethernet controller as SGMII PCS.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - mediatek,mt7622-sgmiisys
+          - mediatek,mt7629-sgmiisys
+          - mediatek,mt7986-sgmiisys_0
+          - mediatek,mt7986-sgmiisys_1
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+  '#clock-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      sgmiisys: syscon@1b128000 {
+        compatible = "mediatek,mt7622-sgmiisys", "syscon";
+        reg = <0 0x1b128000 0 0x1000>;
+        #clock-cells = <1>;
+      };
+    };