by Daniel Golle

[permalink] [raw]

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

2023-02-15 21:16:30

by Russell King (Oracle)

[permalink] [raw]

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

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 <[email protected]>
> > ---
> > .../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.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2023-02-15 21:29:52

by Rob Herring (Arm)

[permalink] [raw]

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

On Wed, Feb 15, 2023 at 3:16 PM Russell King (Oracle)
<[email protected]> 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 <[email protected]>
> > > ---
> > > .../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