Re: [net-next PATCH RFC v3 2/8] net: phy: add initial support for PHY package in DT

[Christian Marangi]

On Tue, Nov 28, 2023 at 01:39:02AM +0100, Andrew Lunn wrote:
> > +static int of_phy_package(struct phy_device *phydev)
> > +{
> > +	struct device_node *node = phydev->mdio.dev.of_node;
> > +	struct device_node *package_node;
> > +	u32 base_addr;
> > +	int ret;
> > +
> > +	if (!node)
> > +		return 0;
> > +
> > +	package_node = of_get_parent(node);
> > +	if (!package_node)
> > +		return 0;
> > +
> > +	if (!of_device_is_compatible(package_node, "ethernet-phy-package"))
> > +		return 0;
> > +
> > +	if (of_property_read_u32(package_node, "reg", &base_addr))
> > +		return -EINVAL;
> > +
> > +	ret = devm_phy_package_join(&phydev->mdio.dev, phydev,
> > +				    base_addr, 0);
> 
> No don't do this. It is just going to lead to errors. The PHY driver
> knows how many PHYs are in the package. So it can figure out what the
> base address is and create the package. It can add each PHY as they
> probe. That cannot go wrong.
>

No it can't and we experiment this with a funny implementation on the
QSDK. Maybe I'm confused?

Example on QSDK they were all based on a value first_phy_addr. This was
filled with the first phy addr found (for the package).

All OEM followed a template with declaring all sort of bloat and they
probably have no idea what they are actually putting in DT. We reverse
all the properties and we gave a meaning to all the values...

We quikly notice that the parsing was very fragile and on some strange
device (an example a Xiaomi 36000) the first PHY for the package was
actually not attached to anything. Resulting in all this logic of
"first_phy_addr" failing as by removing the first PHY, the value was set
to a wrong addr resulting in all sort of problems.

This changed a lot (the original series used a more robust way with
phandle, but it was asked to use base_addr and use offset in the PHY
addr, less robust still OK)

If we revert to PHY driver making the PHY package then we lose all kind
of ""automation"" of having a correct base addr. In PHY driver we would
have to manually parse with parent (as we do here) and check the value?

Why not do the thing directly on PHY core?

By making the PHY driver creating the package, we are back on all kind
of bloat in the PHY driver doing the same thing (parsing package nodes,
probe_once check, config_once check) instead of handling them directly
in PHY core.

Also just to point out, the way the current PHY driver derive the base
addr is problematic. All of them use special regs to find the base one
but I can totally see a PHY driver in the future assuming the first PHY
being the first in the package to be probed, set the base addr on the
first PHY and also assuming that it's always define in DT.

If we really don't want the OF parsing in PHY core and autojoin them,
is at least OK to introduce an helper to get the base addr from a PHY
package node structure? (to at least try to minimaze the bloat that PHY
driver will have?)

Also with the OF support dropped, I guess also the added API in this
series has to be dropped. (as they would be called after the first PHY
probe and that is problematic if for some reason only one PHY of the
package is declared in DT) (an alternative might be moving the
probe_once after the PHY is probed as we expect the phy_package_join
call done in the PHY probe call)

> If you create the package based on DT you have to validate that the DT
> is correct. You need the same information, the base address, how many
> packages are in the PHY, etc. So DT gains your nothing except more
> potential to get it wrong.
> 

For the sake of package join, only the reg has to be validated and the
validation is just if addrs makes sense. Everything else can be done by
PHY package probe once.

> Please use DT just for properties for the package, nothing else.
> 

-- 
	Ansuel