| Message ID | f4b378f4b19064df85d529973ed6c73ae7aa9f2d.1676323692.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: ethernet: mtk_eth_soc: various enhancements | expand |
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>
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>
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.
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 --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>; + }; + };
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