Messages in this thread |  | | Date | Sat, 7 Mar 2026 19:47:54 +0100 | | From | Christian Marangi <> | | Subject | Re: [PATCH v6 4/4] phy: airoha: Add support for Airoha AN7581 USB PHY |
| |
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
|  |