From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 67908757EA; Wed, 15 Jul 2026 05:33:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784093602; cv=none; b=QhpMsf93G+uy/Xb+hznkU2RwGJFeBh5kNp1QYyUhSgBEVSvrGZDT1MsO1Xpj5Olxapv8DuII8jdMmg9oP7lK7jmwqRII9qNyE7XKy4EpUbjttgWTPEUFr4iFQaCa362n4YkggwA2ouIPBSDNMdlU9+3tiMKWZKsMfN33R8baKx4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784093602; c=relaxed/simple; bh=XYgakFKwsRTNmmzAVAmkVmR0AIruII9xqKBuP/xoqMs=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Mj3X3ca4lRdExbZ8KT8gHjRH7H7BvG8UIyJWQWQd/ZKmXqJQ/OKj5bkz/6syqxLwtMj+KO4wGsW94cPm6a7IHkWbQfJ2K6mUZQqOMrkbf1yOLVB6LihRISZhxj2PbwPpcCSakVT5rm1raBzt3a1fYrR2HQGAb9oHb0a5kXlCuuE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=bKfISOvW; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="bKfISOvW" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 85F5C1F000E9; Wed, 15 Jul 2026 05:33:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1784093601; bh=ub1dSpkN7bCGP5dpQmCwxrc51eS+OQh0ansOg9qf4KQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=bKfISOvW60NwonJtPahJyj1/ExU1nq+pM1YOCfIJ+CyDZ425UOZMpsUjsbjYOCkvK wp3yf4jUuSrqwMm6NcxAkhU8AerAlscNVN7k0UjvmDK6TitqVo3peeN4C0XtUfuIi+ cgwP77mRYngypVHVkDd2a8pgXw8BqTetZxnwtRd6TUWQVW0rWeAQdYGkH4z7m4KlvM Vf0/QbkXalkMWFT4Of0FFHx/XxIDkVw2NMyS/y5ZT8ZgsUwYznyNx3icEI6YPc57jW QNcws48lXstLBWPYHzM1SYIc38B1G4PZR5CXilI3DnrC3eLHmLHkAwRvdviJxX1apJ OJqL83VanFxGQ== Date: Tue, 14 Jul 2026 22:33:19 -0700 From: Peter Chen To: Elson Serrao Cc: Bjorn Andersson , Dmitry Baryshkov , Greg Kroah-Hartman , Konrad Dybcio , Krzysztof Kozlowski , Rob Herring , Conor Dooley , Souradeep Chowdhury , linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 3/9] usb: misc: qcom_eud: add per-path High-Speed PHY control Message-ID: References: <20260501170635.2641748-1-elson.serrao@oss.qualcomm.com> <20260501170635.2641748-4-elson.serrao@oss.qualcomm.com> <4ffd0645-dc50-4157-b63b-5e93fd7520a7@oss.qualcomm.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4ffd0645-dc50-4157-b63b-5e93fd7520a7@oss.qualcomm.com> 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 > 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