Date:   Wed, 15 Feb 2023 21:16:11 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Rob Herring <robh@kernel.org>
Cc:     Daniel Golle <daniel@makrotopia.org>, devicetree@vger.kernel.org,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        netdev@vger.kernel.org, linux-mediatek@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Lorenzo Bianconi <lorenzo@kernel.org>,
        Mark Lee <Mark-MC.Lee@mediatek.com>,
        John Crispin <john@phrozen.org>, Felix Fietkau <nbd@nbd.name>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        DENG Qingfang <dqfext@gmail.com>,
        Landen Chao <Landen.Chao@mediatek.com>,
        Sean Wang <sean.wang@mediatek.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Vladimir Oltean <olteanv@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Jianhui Zhao <zhaojh329@gmail.com>,
        =?iso-8859-1?Q?Bj=F8rn?= Mork <bjorn@mork.no>
Subject: Re: [PATCH v6 03/12] dt-bindings: arm: mediatek: sgmiisys: Convert
 to DT schema
Message-ID: <Y+1Lm8XZVrtSGTLT@shell.armlinux.org.uk>
References: <cover.1676323692.git.daniel@makrotopia.org>
 <f4b378f4b19064df85d529973ed6c73ae7aa9f2d.1676323692.git.daniel@makrotopia.org>
 <20230215204318.GA517744-robh@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230215204318.GA517744-robh@kernel.org>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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.

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