Re: [PATCH v6 4/4] phy: airoha: Add support for Airoha AN7581 USB PHY

[Christian Marangi <ansuelsmth@gmail.com>]

On Sat, Mar 07, 2026 at 05:38:25PM +0100, Krzysztof Kozlowski wrote:
> On 07/03/2026 11:57, Christian Marangi wrote:
> > On Sat, Mar 07, 2026 at 11:40:48AM +0100, Krzysztof Kozlowski wrote:
> >> On 07/03/2026 11:37, Christian Marangi wrote:
> >>>>> +
> >>>>> +		if (phy_type == PHY_TYPE_USB3) {
> >>>>> +			ret = of_property_read_u32(dev->of_node, "airoha,usb3-serdes",
> >>>>> +						   &priv->serdes_port);
> >>>>> +			if (ret)
> >>>>> +				return dev_err_probe(dev, ret, "missing serdes line for USB 3.0\n");
> >>>>> +
> >>>>> +			priv->scu = syscon_regmap_lookup_by_compatible("airoha,en7581-scu");
> >>>>
> >>>> Nope, you need phandle to express proper device links.
> >>>>
> >>>> Don't sprinkle compatible lookups for new code which does not need to
> >>>> keep things backwards compatible. How do you manage device links
> >>>> without phandle? How do you manage device probe ordering?
> >>>>
> >>>
> >>> Hi,
> >>>
> >>> the phandle to SCU was present in old implementation but later dropped as it was
> >>> said that it wouldn't describe the HW.
> >>
> >>
> >> I went through v3 review and I did not find such said arguments. Can you
> >> point me to it?
> >>
> >>
> > 
> > Here was v2 [0] that was clock + USB PHY. (there were clock stuff as the SCU
> > implementation was different) The link wasn't added in the changelog as it's
> > quite different than this current series.
> > 
> > In patch 7 Rob pointed out that serdes-port + scu was a bit unusual.
> > 
> > Then the implementation changed in favor of a more simple one where it's the PHY
> > that configure everything and in this new version the usage of the SCU phandle
> > is really to just get the regmap and modify the single bit to select the PHY
> > path/mode for USB 3.0. (it's mainly the reason the SCU is referenced directly
> > with a compatible instead of a phandle, in the previous implementation we used a
> > function exposed by the SCU SSR, while in this it's just a regmap that can
> > PROBE_DEFER)
> > 
> > Rob suggested an additional layer (a PHY) to handle this but I don't feel it
> > would actually describe the HW this way as that bit doesn't modify another PHY
> > but it just toggle the mode to the related USB 3.0 PHY.
> > 
> > This is really another case of not-so-organized register mapping on the SoC.
> > 
> > [0] https://lore.kernel.org/all/20250320130054.4804-1-ansuelsmth@gmail.com/
> 
> 
> That's the cover letter. I guess you did not implement Rob's feedback,
> either. This is either phy (for phy) as Rob asked or scu phandle.
> Removing scu phandle and not implementing phy is not what Rob proposed.
> 

It was to give the full series. The specific comment from Rob was [0].

SCU phandle is going back to the old dual binding that Rob was against but PHY
for PHY would not be descriptive of the HW.

I can do that but would it be correct?

[0] https://lore.kernel.org/all/20250324154919.GA101272-robh@kernel.org/

-- 
	Ansuel