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

[Peter Chen <peter.chen@kernel.org>]

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.
> 
> Signed-off-by: Elson Serrao <elson.serrao@oss.qualcomm.com>
> ---
>  drivers/usb/misc/qcom_eud.c | 126 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 125 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 4aa49f0f58c0..a624c44d5d59 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -11,6 +11,8 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> @@ -39,26 +41,84 @@ static const char * const eud_port_names[] = {
>  	"secondary",
>  };
>  
> +struct eud_path {
> +	struct eud_chip		*chip;
> +	struct phy		*phy;
> +	u8			num;
> +};
> +
>  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?

Peter

>  	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