Re: [PATCH v2 3/9] usb: misc: qcom_eud: add per-path High-Speed PHY control

[Elson Serrao <elson.serrao@oss.qualcomm.com>]

On 2/4/2026 5:21 AM, Konrad Dybcio wrote:
> On 1/27/26 12:38 AM, Elson Serrao wrote:
>> EUD hardware can support multiple High-Speed USB paths, each routed
>> through its own PHY. The active path is selected in hardware via the
>> EUD_PORT_SEL register. As a High-Speed hub, EUD requires access to the
>> High-Speed PHY associated with the active UTMI path. To support this
>> multi-path capability, the driver must manage PHY resources on a per-path
>> basis, ensuring that the PHY for the currently selected path is properly
>> initialized and powered.
>>
>> This patch restructures the driver to implement per-path PHY management.
>> The driver now powers the appropriate PHY based on the selected and
>> enabled UTMI path, ensuring correct operation when EUD is enabled.
>>
>> Supporting this requires describing the available UTMI paths and their
>> corresponding PHYs in Device Tree. This updates DT requirements and is
>> not backward compatible with older DTs that lacked this description.
>> Historically, EUD appeared to work on single-path systems because the
>> USB controller kept the PHY initialized. However, EUD is designed to
>> operate independently of the USB controller and therefore requires
>> explicit PHY control.
>>
>> Signed-off-by: Elson Serrao <elson.serrao@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> +static int eud_phy_enable(struct eud_chip *chip)
>> +{
>> +	struct eud_path *path;
>> +	struct phy *phy;
>> +	int ret;
>> +
>> +	if (chip->phy_enabled)
>> +		return 0;
>> +
>> +	path = chip->paths[chip->port_idx];
>> +	if (!path || !path->phy) {
> 
> I think neither are possible - path is != NULL since we can't enter into
> this function without failing the check in _store and !path->phy would error
> out in probe()->eud_init_path()
> 
> [...]
> 
>> +static void eud_phy_disable(struct eud_chip *chip)
>> +{
>> +	struct eud_path *path;
>> +	struct phy *phy;
>> +
>> +	if (!chip->phy_enabled)
>> +		return;
>> +
>> +	path = chip->paths[chip->port_idx];
>> +	if (!path || !path->phy)
> 
> Likewise
> 
> [...]
> 
>> +static int eud_init_path(struct eud_chip *chip, struct device_node *np)
>> +{
>> +	struct eud_path *path;
>> +	u32 path_num;
>> +	int ret;
>> +
>> +	ret = of_property_read_u32(np, "reg", &path_num);
>> +	if (ret) {
>> +		dev_err(chip->dev, "Missing 'reg' property in path node\n");
>> +		return ret;
> 
> You can use return dev_err_probe like you did a little below
> 
>> +	}
>> +
>> +	if (path_num >= EUD_MAX_PORTS) {
>> +		dev_err(chip->dev, "Invalid path number: %u (max %d)\n",
>> +			path_num, EUD_MAX_PORTS - 1);
>> +		return -EINVAL;
>> +	}
>> +
>> +	path = devm_kzalloc(chip->dev, sizeof(*path), GFP_KERNEL);
>> +	if (!path)
>> +		return -ENOMEM;
>> +
>> +	path->chip = chip;
>> +	path->num = path_num;
>> +
>> +	path->phy = devm_of_phy_get(chip->dev, np, NULL);
>> +	if (IS_ERR(path->phy))
>> +		return dev_err_probe(chip->dev, PTR_ERR(path->phy),
>> +				     "Failed to get PHY for path %d\n", path_num);
>> +
>> +	chip->paths[path_num] = path;
>> +
>> +	return 0;
>> +}
>> +
>>  static int eud_probe(struct platform_device *pdev)
>>  {
>> +	struct device_node *np = pdev->dev.of_node;
>> +	struct device_node *child;
>>  	struct eud_chip *chip;
>>  	struct resource *res;
>>  	int ret;
>> @@ -252,6 +368,18 @@ static int eud_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>  
>> +	for_each_child_of_node(np, child) {
> 
> With for_each_child_of_node_scoped(), you can dispose of the manual
> _put()
> 
>> +		ret = eud_init_path(chip, child);
>> +		if (ret) {
>> +			of_node_put(child);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	/* Primary path is mandatory. Secondary is optional */
>> +	if (!chip->paths[0])
>> +		return -ENODEV;
> 
> I'm going to assume we don't have any funny chips that violate this :)
> 


On all current SoCs, EUD_PORT_SEL defaults to 0, and the hardware guide
defines this as mapping to USB port 0. So the primary path being mandatory
is a valid assumption for existing hardware.

Thanks,
Elson