Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <6560dc65.050a0220.182b5.650c@mx.google.com>
Date:   Fri, 24 Nov 2023 17:25:16 +0100
From:   Christian Marangi <ansuelsmth@gmail.com>
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Andrew Lunn <andrew@lunn.ch>, Rob Herring <robh@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Florian Fainelli <florian.fainelli@broadcom.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@broadcom.com>,
        Daniel Golle <daniel@makrotopia.org>,
        Qingfang Deng <dqfext@gmail.com>,
        SkyLake Huang <SkyLake.Huang@mediatek.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        David Epping <david.epping@missinglinkelectronics.com>,
        "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>,
        Harini Katakam <harini.katakam@amd.com>,
        Simon Horman <horms@kernel.org>,
        Robert Marko <robert.marko@sartura.hr>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org
Subject: Re: [net-next RFC PATCH 03/14] dt-bindings: net: document ethernet
 PHY package nodes
References: <20231120135041.15259-1-ansuelsmth@gmail.com>
 <20231120135041.15259-4-ansuelsmth@gmail.com>
 <c21ff90d-6e05-4afc-b39c-2c71d8976826@lunn.ch>
 <20231121144244.GA1682395-robh@kernel.org>
 <a85d6d0a-1fc9-4c8e-9f91-5054ca902cd1@lunn.ch>
 <655e4939.5d0a0220.d9a9e.0491@mx.google.com>
 <20231124165923.p2iozsrnwlogjzua@skbuf>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231124165923.p2iozsrnwlogjzua@skbuf>
Precedence: bulk

On Fri, Nov 24, 2023 at 06:59:23PM +0200, Vladimir Oltean wrote:
> On Wed, Nov 22, 2023 at 07:32:22PM +0100, Christian Marangi wrote:
> > Sooooo.... Sorry if I insist but I would really love to have something
> > ""stable"" to move this further. (the changes are easy enough so it's
> > really a matter of finding a good DT structure)
> > 
> > Maybe a good idea would be summarize the concern and see what solution
> > was proposed:
> 
> Sorry, I didn't follow the entire discussion. I hope I'm not too far off
> with my understanding of your problems.
> 
> I think you are hitting some of the same points I have hit with DSA.
> The PHY package could be considered an SoC with lots of peripherals on
> it, for which you'd want separate drivers. Just like a DSA switch would.
> I don't think it's exactly phylib's place to deal with that, just like
> it's not DSA's place to deal with complex SoCs, just with the switching
> IP (like the Ethernet PHY IP for phylib).
> https://lore.kernel.org/lkml/20221222134844.lbzyx5hz7z5n763n@skbuf/
> 
> Why does the ethernet-phy-package DT binding attempt to be so grand and
> generic? I would expect the 180 degree opposite. Make it have a _single_
> compatible of "qcom,qca807x" (but don't use an "x" wildcard, do specify
> the full package name).
> 
> Make it have a "reg" property which is the base MDIO address of the package.
> 
> Write an mdio_device driver that probes on that. The PHY core already
> knows that if a child on the MDIO bus has a compatible string of the
> normal form (not like "ethernet-phy-id004d.d0b2"), then it's looking at
> an mdio_device.
> 
> Make the OF node of the package have an "mdio" child with its own
> compatible string, which represents the place where PHYs are. The driver
> for the "mdio" child has a very simple implementation of the mii_bus
> ops, which just calls into the device parent (it can assume a certain
> parent implementation and private data structures).
> 
> Lateral to the "mdio" child node of the "qcom,qca807x" package node, you
> could put any other device tree properties that you want.
> 
> Make the mdio_device driver for "qcom,qca807x" use shared code if you
> want - but keep the device tree structure hardware-specific. Look at the
> compatible strings that e.g. the drivers/mfd/simple-mfd-i2c.c driver
> probes on. You could always change the driver implementation for a
> certain compatible string, but you'll be stuck with the ultra-generic
> compatible = "ethernet-phy-package", which has the problems that you
> mention.
>

The main reason is the fact that PHY package are already a thing and API
are already there (phy_package_join/leave...) so we just lack any way to
support this in DT without using specialized code in the PHY driver.

This is really completing the feature.

The only reason for the generic "ethernet-phy-package" compatible is to
have a solid detection of the node in PHY core. (I assume parsing the
node name might be problematic? Or maybe not now that we require adding
a reg to it)

Also I don't expect tons of special properties for PHY package, with the
current probe/config implementation, a PHY driver have lots of
flexibility in all kind of validation.

Consider that the additional global-phys and global-phy-names are
already expected to be dropped.
(we know the PHY package base address and we can calculate the offset of
the global phy from there in the PHY package probe)

And even the phy-mode has been scrapped for more specific solution...
(some assumption can be done on probe by checking the PHY mode and set
stuff accordingly or even do parsing in the PHY package node as we pass
the OF node of the phy package)

The PHY package node would be reduced to a simple compatible (and even
this can be dropped) and a reg.

I feel there is a big chance here to generalize it and prevent any kind
of mess with all kind of similar/equal code that just do the same thing.
(and we already have an example with the PHY package API usage with
every PHY having the same exact pattern for probe/config and nothing
describing that the PHY are actually a package in DT)

Hope it all makes sense to you.

> > 
> > Concern list:
> > 1. ethernet-phy-package MUST be placed in mdio node (not in ethernet,
> >    the example was wrong anyway) and MUST have an addr
> > 
> >    Current example doesn't have an addr. I would prefer this way but
> >    no problem in changing this.
> > 
> >    Solution:
> >      - Add reg to the ethernet-phy-package node with the base address of
> >        the PHY package (base address = the first PHY address of the
> >        package)
> 
> Correct, what I'm saying is compatible with this.
> 
> > 
> >        We will have a PHY node with the same address of the PHY package
> >        node. Each PHY node in the PHY package node will have reg set to
> >        the REAL address in the mdio bus.
> 
> If the real PHY IPs are children of the package rather than on the same
> level with it, I don't think this will be a problem. I wonder if some
> address translation could be done with the "ranges" device tree property.
> I've never seen this with MDIO though.
> 

I can check it, I would love some way to describe the address used by
the PHY package. (since everything will be handled internally with
offsets, would be good to define in DT that (for example) addrs from 0
to 5 are used). Some PHY might be not attached but still used for global
configuration of the PHY package.

> > 4. Not finding a correct place to put PHY package info.
> > 
> >    I'm still convinced the mdio node is the correct place.
> >    - PHY package are PHY in bundle so they are actual PHY
> >    - We already have in the mdio node special handling (every DSA switch
> >      use custom compatible and PHY ID is not used to probe them
> >      normally)
> >    - Node this way won't be treated as PHY as they won't match the PHY
> >      node name pattern and also won't have the compatible pattern for
> >      PHY.
> > 
> >    Solution:
> >     - ethernet-phy-package node is OK given a reg is defined.
> 
> I agree that it should sit under the MDIO node. I disagree with the idea
> of a standardized binding for PHY packages.

-- 
	Ansuel