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

[Peter Chen]

On 26-07-14 17:32:34, Elson Serrao wrote:
> 
> 
> On 7/5/2026 11:26 PM, Peter Chen wrote:
> > On 26-05-01 10:06:29, 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 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.
> >>
> >> 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 for proper operation.
> 
> [...]
> 
> >> +};
> >> +
> >>  struct eud_chip {
> >>  	struct device			*dev;
> >>  	struct usb_role_switch		*role_sw;
> >>  	void __iomem			*base;
> >> +	struct eud_path			*paths[EUD_MAX_PORTS];
> >>  	phys_addr_t			mode_mgr;
> >>  	unsigned int			int_status;
> >>  	int				irq;
> >>  	bool				enabled;
> >>  	bool				usb_attached;
> >> +	bool				phy_enabled;
> > 
> > There is already a 'enabled' flag, could be leveraged or still need
> > a new 'phy_enabled' flag?
> > 
> enabled reflects the user's sysfs intent, while phy_enabled tracks the
> actual PHY power state. These need to stay independent because a later
> patch in this series ("add host mode coordination") disables/re-enables
> the PHY on host-mode role transitions without touching chip->enabled, so
> as to preserve the user's original sysfs setting across the transition.
> 

Thanks for explaining, I have no other questions.

Reviewed-by: Peter Chen <peter.chen@xxxxxxxxxx>

Peter
> Thanks,
> Elson
> 
> u8				port_idx;
> >>  };
> >>  
> >> +static int eud_phy_enable(struct eud_chip *chip)
> >> +{
> >> +	struct phy *phy;
> >> +	int ret;
> >> +
> >> +	if (chip->phy_enabled)
> >> +		return 0;
> >> +
> >> +	phy = chip->paths[chip->port_idx]->phy;
> >> +
> >> +	ret = phy_init(phy);
> >> +	if (ret) {
> >> +		dev_err(chip->dev, "Failed to initialize USB2 PHY for port %u: %d\n",
> >> +			chip->port_idx, ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = phy_power_on(phy);
> >> +	if (ret) {
> >> +		dev_err(chip->dev, "Failed to power on USB2 PHY for port %u: %d\n",
> >> +			chip->port_idx, ret);
> >> +		phy_exit(phy);
> >> +		return ret;
> >> +	}
> >> +
> >> +	chip->phy_enabled = true;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void eud_phy_disable(struct eud_chip *chip)
> >> +{
> >> +	struct phy *phy;
> >> +
> >> +	if (!chip->phy_enabled)
> >> +		return;
> >> +
> >> +	phy = chip->paths[chip->port_idx]->phy;
> >> +
> >> +	phy_power_off(phy);
> >> +	phy_exit(phy);
> >> +	chip->phy_enabled = false;
> >> +}
> >> +
> >>  static int enable_eud(struct eud_chip *priv)
> >>  {
> >>  	int ret;
> >>  
> >> -	ret = qcom_scm_io_writel(priv->mode_mgr + EUD_REG_EUD_EN2, 1);
> >> +	ret = eud_phy_enable(priv);
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> +	ret = qcom_scm_io_writel(priv->mode_mgr + EUD_REG_EUD_EN2, 1);
> >> +	if (ret) {
> >> +		eud_phy_disable(priv);
> >> +		return ret;
> >> +	}
> >> +
> >>  	writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
> >>  	writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE,
> >>  			priv->base + EUD_REG_INT1_EN_MASK);
> >> @@ -75,6 +135,8 @@ static int disable_eud(struct eud_chip *priv)
> >>  		return ret;
> >>  
> >>  	writel(0, priv->base + EUD_REG_CSR_EUD_EN);
> >> +	eud_phy_disable(priv);
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> @@ -130,6 +192,12 @@ static ssize_t port_store(struct device *dev, struct device_attribute *attr,
> >>  	if (port < 0)
> >>  		return port;
> >>  
> >> +	/* Check if the corresponding path is available */
> >> +	if (!chip->paths[port]) {
> >> +		dev_err(chip->dev, "EUD not supported on selected port\n");
> >> +		return -EOPNOTSUPP;
> >> +	}
> >> +
> >>  	/* Port selection must be done before enabling EUD */
> >>  	if (chip->enabled) {
> >>  		dev_err(chip->dev, "Cannot change port while EUD is enabled\n");
> >> @@ -229,8 +297,54 @@ static void eud_role_switch_release(void *data)
> >>  	usb_role_switch_put(chip->role_sw);
> >>  }
> >>  
> >> +static int eud_init_path(struct eud_chip *chip, struct device_node *np)
> >> +{
> >> +	struct device_node *controller_node;
> >> +	struct eud_path *path;
> >> +	u32 path_num;
> >> +	int ret;
> >> +
> >> +	ret = of_property_read_u32(np, "reg", &path_num);
> >> +	if (ret) {
> >> +		/* Legacy DT uses 'ports' node without 'reg' property; treat as path 0 */
> >> +		if (of_node_name_eq(np, "ports"))
> >> +			path_num = 0;
> >> +		else
> >> +			return dev_err_probe(chip->dev, ret, "unexpected child node '%s'\n",
> >> +					     np->name);
> >> +	}
> >> +
> >> +	if (path_num >= EUD_MAX_PORTS)
> >> +		return dev_err_probe(chip->dev, -EINVAL, "invalid path number: %u (max %d)\n",
> >> +				     path_num, EUD_MAX_PORTS - 1);
> >> +
> >> +	path = devm_kzalloc(chip->dev, sizeof(*path), GFP_KERNEL);
> >> +	if (!path)
> >> +		return -ENOMEM;
> >> +
> >> +	path->chip = chip;
> >> +	path->num = path_num;
> >> +
> >> +	controller_node = of_graph_get_remote_node(np, 0, -1);
> >> +	if (!controller_node)
> >> +		return dev_err_probe(chip->dev, -ENODEV,
> >> +				     "failed to get controller node for path %u\n", path_num);
> >> +
> >> +	path->phy = devm_of_phy_get_by_index(chip->dev, controller_node, 0);
> >> +	of_node_put(controller_node);
> >> +
> >> +	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 eud_chip *chip;
> >>  	struct resource *res;
> >>  	int ret;
> >> @@ -241,6 +355,16 @@ static int eud_probe(struct platform_device *pdev)
> >>  
> >>  	chip->dev = &pdev->dev;
> >>  
> >> +	for_each_child_of_node_scoped(np, child) {
> >> +		ret = eud_init_path(chip, child);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	/* Primary path is mandatory. Secondary is optional */
> >> +	if (!chip->paths[0])
> >> +		return dev_err_probe(chip->dev, -ENODEV, "primary path not found\n");
> >> +
> >>  	chip->role_sw = usb_role_switch_get(&pdev->dev);
> >>  	if (IS_ERR(chip->role_sw))
> >>  		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
> >> -- 
> >> 2.34.1
> >>
> >>
> > 
> 

-- 

Thanks,
Peter Chen