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

[Krzysztof Kozlowski]

On 07/03/2026 19:47, Christian Marangi wrote:
> 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@xxxxxxxxx/
>>
>>
>> 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?

You cannot avoid proper DT structure and DT review by re-implementing
the DT properties by such lookups.

Best regards,
Krzysztof