* Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller
2022-05-19 12:34 ` [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller Harsh Agarwal
@ 2022-05-21 3:17 ` Bjorn Andersson
2022-05-23 11:53 ` Harsh Agarwal
2022-05-23 2:59 ` Pavan Kondeti
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Bjorn Andersson @ 2022-05-21 3:17 UTC (permalink / raw)
To: Harsh Agarwal
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Felipe Balbi, linux-usb, devicetree, linux-kernel, quic_pkondeti,
quic_ppratap
On Thu 19 May 05:34 PDT 2022, Harsh Agarwal wrote:
> Currently the USB driver supports only single port controller
> which works with 2 PHYs at max ie HS and SS.
>
> But some devices have "multiport" controller where a single
> controller supports multiple ports and each port have their own
> PHYs. Refactor PHY logic to support the same.
>
> This implementation is compatible with existing glue drivers.
>
I would love to see this support land, for various different devices I
have.
But can you please explain how you tested this on an upstream kernel,
given that all the Qualcomm phys are implemented in the generic phy
framework?
Also, when I spoke with Jack about this feature recently, he mentioned
that you need to update GUSB2PHYCFG(i) and GUSB3PIPECTL(i) in e.g.
dwc3_phy_config(). Is this series complete?
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> ---
> drivers/usb/dwc3/core.c | 259 +++++++++++++++++++++++++++++++++++-----------
> drivers/usb/dwc3/core.h | 8 +-
> drivers/usb/dwc3/drd.c | 8 +-
> drivers/usb/dwc3/gadget.c | 4 +-
> 4 files changed, 209 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2682469..8eb6b5b6 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -189,8 +189,8 @@ static void __dwc3_set_mode(struct work_struct *work)
> if (ret) {
> dev_err(dwc->dev, "failed to initialize host\n");
> } else {
> - if (dwc->usb2_phy)
> - otg_set_vbus(dwc->usb2_phy->otg, true);
> + if (dwc->usb2_phy[0])
> + otg_set_vbus(dwc->usb2_phy[0]->otg, true);
> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
> phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
> if (dwc->dis_split_quirk) {
> @@ -204,9 +204,13 @@ static void __dwc3_set_mode(struct work_struct *work)
> dwc3_core_soft_reset(dwc);
>
> dwc3_event_buffers_setup(dwc);
> -
> - if (dwc->usb2_phy)
> - otg_set_vbus(dwc->usb2_phy->otg, false);
> + /*
> + * Multiport Controller works only in host mode.
> + * There will only be one pair of HS and SS PHY for the controller operating in
> + * device mode.
> + */
I think this comment applies to every single place where you operate on
usb2_phy[0] only. But rather than telling the reader of the code that
multiport is valid only for host-only setups, wouldn't it be useful to
prevent the moving to device mode if multiple ports are specified?
Or am I just not finding the place where you do this?
> + if (dwc->usb2_phy[0])
> + otg_set_vbus(dwc->usb2_phy[0]->otg, false);
> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
> phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>
> @@ -825,15 +829,20 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
>
> static void dwc3_core_exit(struct dwc3 *dwc)
> {
> + int i;
> dwc3_event_buffers_cleanup(dwc);
>
> - usb_phy_shutdown(dwc->usb2_phy);
> - usb_phy_shutdown(dwc->usb3_phy);
> + for (i = 0; i < dwc->num_hsphy; i++)
> + usb_phy_shutdown(dwc->usb2_phy[i]);
> + for (i = 0; i < dwc->num_ssphy; i++)
> + usb_phy_shutdown(dwc->usb3_phy[i]);
> phy_exit(dwc->usb2_generic_phy);
> phy_exit(dwc->usb3_generic_phy);
>
> - usb_phy_set_suspend(dwc->usb2_phy, 1);
> - usb_phy_set_suspend(dwc->usb3_phy, 1);
> + for (i = 0; i < dwc->num_hsphy; i++)
> + usb_phy_set_suspend(dwc->usb2_phy[i], 1);
> + for (i = 0; i < dwc->num_ssphy; i++)
> + usb_phy_set_suspend(dwc->usb3_phy[i], 1);
> phy_power_off(dwc->usb2_generic_phy);
> phy_power_off(dwc->usb3_generic_phy);
> dwc3_clk_disable(dwc);
> @@ -1038,7 +1047,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
> {
> unsigned int hw_mode;
> u32 reg;
> - int ret;
> + int ret, i;
>
> hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>
> @@ -1066,8 +1075,10 @@ static int dwc3_core_init(struct dwc3 *dwc)
> dwc->phys_ready = true;
> }
>
> - usb_phy_init(dwc->usb2_phy);
> - usb_phy_init(dwc->usb3_phy);
> + for (i = 0; i < dwc->num_hsphy; i++)
> + usb_phy_init(dwc->usb2_phy[i]);
> + for (i = 0; i < dwc->num_ssphy; i++)
> + usb_phy_init(dwc->usb3_phy[i]);
> ret = phy_init(dwc->usb2_generic_phy);
> if (ret < 0)
> goto err0a;
> @@ -1112,8 +1123,10 @@ static int dwc3_core_init(struct dwc3 *dwc)
>
> dwc3_set_incr_burst_type(dwc);
>
> - usb_phy_set_suspend(dwc->usb2_phy, 0);
> - usb_phy_set_suspend(dwc->usb3_phy, 0);
> + for (i = 0; i < dwc->num_hsphy; i++)
> + usb_phy_set_suspend(dwc->usb2_phy[i], 0);
> + for (i = 0; i < dwc->num_ssphy; i++)
> + usb_phy_set_suspend(dwc->usb3_phy[i], 0);
> ret = phy_power_on(dwc->usb2_generic_phy);
> if (ret < 0)
> goto err2;
> @@ -1234,12 +1247,16 @@ static int dwc3_core_init(struct dwc3 *dwc)
> phy_power_off(dwc->usb2_generic_phy);
>
> err2:
> - usb_phy_set_suspend(dwc->usb2_phy, 1);
> - usb_phy_set_suspend(dwc->usb3_phy, 1);
> + for (i = 0; i < dwc->num_hsphy; i++)
> + usb_phy_set_suspend(dwc->usb2_phy[i], 1);
> + for (i = 0; i < dwc->num_ssphy; i++)
> + usb_phy_set_suspend(dwc->usb3_phy[i], 1);
>
> err1:
> - usb_phy_shutdown(dwc->usb2_phy);
> - usb_phy_shutdown(dwc->usb3_phy);
> + for (i = 0; i < dwc->num_hsphy; i++)
> + usb_phy_shutdown(dwc->usb2_phy[i]);
> + for (i = 0; i < dwc->num_ssphy; i++)
> + usb_phy_shutdown(dwc->usb3_phy[i]);
> phy_exit(dwc->usb2_generic_phy);
> phy_exit(dwc->usb3_generic_phy);
>
> @@ -1250,52 +1267,166 @@ static int dwc3_core_init(struct dwc3 *dwc)
> return ret;
> }
>
> +static struct usb_phy *dwc3_core_get_phy_by_handle_with_node(struct device *dev,
> + const char *phandle, u8 index, struct device_node *lookup_node)
> +{
> + struct device_node *node;
> + struct usb_phy *phy;
> +
> + node = of_parse_phandle(lookup_node, phandle, index);
> + if (!node) {
> + dev_err(dev, "failed to get %s phandle in %pOF node\n", phandle,
> + dev->of_node);
> + return ERR_PTR(-ENODEV);
> + }
> + phy = devm_usb_get_phy_by_node(dev, node, NULL);
> + of_node_put(node);
> + return phy;
> +}
> +
> +static int dwc3_extract_num_phys(struct dwc3 *dwc)
This doesn't extract the number, it allocates the usb2_phy and usb3_phy
arrays.
> +{
> + struct device_node *ports, *port;
> +
> + /* Find if any "multiport" child is present inside DWC3*/
> + for_each_available_child_of_node(dwc->dev->of_node, ports) {
> + if (!strcmp(ports->name, "multiport"))
> + break;
If you break you need of_node_put().
> + }
> + if (!ports) {
> + dwc->num_hsphy = 1;
> + dwc->num_ssphy = 1;
> + } else {
> + for_each_available_child_of_node(ports, port) {
> + dwc->num_hsphy += 1;
> + dwc->num_ssphy += 1;
> + }
> + }
> + dev_info(dwc->dev, "Num of HS and SS PHY are %u %u\n", dwc->num_hsphy, dwc->num_ssphy);
> +
> + dwc->usb2_phy = devm_kzalloc(dwc->dev,
> + sizeof(*dwc->usb2_phy) * dwc->num_hsphy, GFP_KERNEL);
> + if (!dwc->usb2_phy)
> + return -ENOMEM;
> +
> + dwc->usb3_phy = devm_kzalloc(dwc->dev,
> + sizeof(*dwc->usb3_phy) * dwc->num_ssphy, GFP_KERNEL);
> + if (!dwc->usb3_phy)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> static int dwc3_core_get_phy(struct dwc3 *dwc)
> {
> struct device *dev = dwc->dev;
> struct device_node *node = dev->of_node;
> - int ret;
> + struct device_node *ports, *port;
>
> - if (node) {
> - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> - } else {
> - dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> - dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> - }
> + int ret, i = 0;
>
> - if (IS_ERR(dwc->usb2_phy)) {
> - ret = PTR_ERR(dwc->usb2_phy);
> - if (ret == -ENXIO || ret == -ENODEV)
> - dwc->usb2_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + ret = dwc3_extract_num_phys(dwc);
> + if (ret) {
> + dev_err(dwc->dev, "Unable to extract number of PHYs\n");
> + return ret;
> }
>
> - if (IS_ERR(dwc->usb3_phy)) {
> - ret = PTR_ERR(dwc->usb3_phy);
> - if (ret == -ENXIO || ret == -ENODEV)
> - dwc->usb3_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + /* Find if any "multiport" child is present inside DWC3*/
> + for_each_available_child_of_node(node, ports) {
> + if (!strcmp(ports->name, "multiport"))
> + break;
> }
>
> - dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> - if (IS_ERR(dwc->usb2_generic_phy)) {
> - ret = PTR_ERR(dwc->usb2_generic_phy);
> - if (ret == -ENOSYS || ret == -ENODEV)
> - dwc->usb2_generic_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> - }
> + if (!ports) {
> + if (node) {
> + dwc->usb2_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> + dwc->usb3_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> + } else {
> + dwc->usb2_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> + dwc->usb3_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> + }
>
> - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> - if (IS_ERR(dwc->usb3_generic_phy)) {
> - ret = PTR_ERR(dwc->usb3_generic_phy);
> - if (ret == -ENOSYS || ret == -ENODEV)
> - dwc->usb3_generic_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + if (IS_ERR(dwc->usb2_phy[0])) {
> + ret = PTR_ERR(dwc->usb2_phy[0]);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb2_phy[0] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
> +
> + if (IS_ERR(dwc->usb3_phy[0])) {
> + ret = PTR_ERR(dwc->usb3_phy[0]);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb3_phy[0] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + }
> +
> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> + if (IS_ERR(dwc->usb2_generic_phy)) {
> + ret = PTR_ERR(dwc->usb2_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb2_generic_phy = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
> +
> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> + if (IS_ERR(dwc->usb3_generic_phy)) {
> + ret = PTR_ERR(dwc->usb3_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb3_generic_phy = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + }
> +
> + } else {
> + pr_info("Multiport node found\n");
Please remove your debug prints.
> + /* Iterate over each port of the MultiPort */
> + for_each_available_child_of_node(ports, port) {
> + dwc->usb2_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
> + 0, port);
> + dwc->usb3_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
> + 1, port);
> +
> + if (IS_ERR(dwc->usb2_phy[i])) {
> + ret = PTR_ERR(dwc->usb2_phy[i]);
> + pr_err("usb2_phy gone %d\n", ret);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb2_phy[i] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
> +
> + if (IS_ERR(dwc->usb3_phy[i])) {
> + ret = PTR_ERR(dwc->usb3_phy[i]);
> + pr_err("usb3_phy gone %d\n", ret);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb3_phy[i] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + }
> + //TODO Write Generic PHY API
Afaict this isn't a TODO, this is just broken right now?
Regards,
Bjorn
> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> + if (IS_ERR(dwc->usb2_generic_phy)) {
> + ret = PTR_ERR(dwc->usb2_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb2_generic_phy = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
> +
> + //TODO Write Generic PHY API
> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> + if (IS_ERR(dwc->usb3_generic_phy)) {
> + ret = PTR_ERR(dwc->usb3_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb3_generic_phy = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + }
> + i++;
> + }
> }
>
> return 0;
> @@ -1310,8 +1441,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
> case USB_DR_MODE_PERIPHERAL:
> dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>
> - if (dwc->usb2_phy)
> - otg_set_vbus(dwc->usb2_phy->otg, false);
> + if (dwc->usb2_phy[0])
> + otg_set_vbus(dwc->usb2_phy[0]->otg, false);
> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
> phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>
> @@ -1322,8 +1453,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
> case USB_DR_MODE_HOST:
> dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
>
> - if (dwc->usb2_phy)
> - otg_set_vbus(dwc->usb2_phy->otg, true);
> + if (dwc->usb2_phy[0])
> + otg_set_vbus(dwc->usb2_phy[0]->otg, true);
> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
> phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>
> @@ -1673,7 +1804,7 @@ static int dwc3_probe(struct platform_device *pdev)
> struct resource *res, dwc_res;
> struct dwc3 *dwc;
>
> - int ret;
> + int ret, i;
>
> void __iomem *regs;
>
> @@ -1838,13 +1969,17 @@ static int dwc3_probe(struct platform_device *pdev)
> dwc3_debugfs_exit(dwc);
> dwc3_event_buffers_cleanup(dwc);
>
> - usb_phy_shutdown(dwc->usb2_phy);
> - usb_phy_shutdown(dwc->usb3_phy);
> + for (i = 0; i < dwc->num_hsphy; i++)
> + usb_phy_shutdown(dwc->usb2_phy[i]);
> + for (i = 0; i < dwc->num_ssphy; i++)
> + usb_phy_shutdown(dwc->usb3_phy[i]);
> phy_exit(dwc->usb2_generic_phy);
> phy_exit(dwc->usb3_generic_phy);
>
> - usb_phy_set_suspend(dwc->usb2_phy, 1);
> - usb_phy_set_suspend(dwc->usb3_phy, 1);
> + for (i = 0; i < dwc->num_hsphy; i++)
> + usb_phy_set_suspend(dwc->usb2_phy[i], 1);
> + for (i = 0; i < dwc->num_ssphy; i++)
> + usb_phy_set_suspend(dwc->usb3_phy[i], 1);
> phy_power_off(dwc->usb2_generic_phy);
> phy_power_off(dwc->usb3_generic_phy);
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 81c486b..3175ed9 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1020,6 +1020,8 @@ struct dwc3_scratchpad_array {
> * @usb_psy: pointer to power supply interface.
> * @usb2_phy: pointer to USB2 PHY
> * @usb3_phy: pointer to USB3 PHY
> + * @num_hsphy: Number of HS ports controlled by the core
> + * @num_dsphy: Number of SS ports controlled by the core
> * @usb2_generic_phy: pointer to USB2 PHY
> * @usb3_generic_phy: pointer to USB3 PHY
> * @phys_ready: flag to indicate that PHYs are ready
> @@ -1147,8 +1149,10 @@ struct dwc3 {
>
> struct reset_control *reset;
>
> - struct usb_phy *usb2_phy;
> - struct usb_phy *usb3_phy;
> + struct usb_phy **usb2_phy;
> + struct usb_phy **usb3_phy;
> + u32 num_hsphy;
> + u32 num_ssphy;
>
> struct phy *usb2_generic_phy;
> struct phy *usb3_generic_phy;
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index 039bf24..7c9eba6 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -384,8 +384,8 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
> if (ret) {
> dev_err(dwc->dev, "failed to initialize host\n");
> } else {
> - if (dwc->usb2_phy)
> - otg_set_vbus(dwc->usb2_phy->otg, true);
> + if (dwc->usb2_phy[0])
> + otg_set_vbus(dwc->usb2_phy[0]->otg, true);
> if (dwc->usb2_generic_phy)
> phy_set_mode(dwc->usb2_generic_phy,
> PHY_MODE_USB_HOST);
> @@ -398,8 +398,8 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
> dwc3_event_buffers_setup(dwc);
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> - if (dwc->usb2_phy)
> - otg_set_vbus(dwc->usb2_phy->otg, false);
> + if (dwc->usb2_phy[0])
> + otg_set_vbus(dwc->usb2_phy[0]->otg, false);
> if (dwc->usb2_generic_phy)
> phy_set_mode(dwc->usb2_generic_phy,
> PHY_MODE_USB_DEVICE);
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 4f54f0e..c58a67c 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2870,8 +2870,8 @@ static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA)
> union power_supply_propval val = {0};
> int ret;
>
> - if (dwc->usb2_phy)
> - return usb_phy_set_power(dwc->usb2_phy, mA);
> + if (dwc->usb2_phy[0])
> + return usb_phy_set_power(dwc->usb2_phy[0], mA);
>
> if (!dwc->usb_psy)
> return -EOPNOTSUPP;
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller
2022-05-21 3:17 ` Bjorn Andersson
@ 2022-05-23 11:53 ` Harsh Agarwal
0 siblings, 0 replies; 16+ messages in thread
From: Harsh Agarwal @ 2022-05-23 11:53 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Felipe Balbi, linux-usb, devicetree, linux-kernel, quic_pkondeti,
quic_ppratap
On 5/21/2022 8:47 AM, Bjorn Andersson wrote:
> On Thu 19 May 05:34 PDT 2022, Harsh Agarwal wrote:
>
>> Currently the USB driver supports only single port controller
>> which works with 2 PHYs at max ie HS and SS.
>>
>> But some devices have "multiport" controller where a single
>> controller supports multiple ports and each port have their own
>> PHYs. Refactor PHY logic to support the same.
>>
>> This implementation is compatible with existing glue drivers.
>>
> I would love to see this support land, for various different devices I
> have.
>
> But can you please explain how you tested this on an upstream kernel,
> given that all the Qualcomm phys are implemented in the generic phy
> framework?
>
>
> Also, when I spoke with Jack about this feature recently, he mentioned
> that you need to update GUSB2PHYCFG(i) and GUSB3PIPECTL(i) in e.g.
> dwc3_phy_config(). Is this series complete?
The idea here was to make the core driver compatible with multiport and
with the existing single port controller.
We wanted to get feedback on the current approach.
Ofcourse the code needs refactoring.
Yes for multiport we need to accomodate for GUSB2PHYCFG(i) and
GUSB3PIPECTL(i).
Will add those changes as well.
>
>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>> ---
>> drivers/usb/dwc3/core.c | 259 +++++++++++++++++++++++++++++++++++-----------
>> drivers/usb/dwc3/core.h | 8 +-
>> drivers/usb/dwc3/drd.c | 8 +-
>> drivers/usb/dwc3/gadget.c | 4 +-
>> 4 files changed, 209 insertions(+), 70 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 2682469..8eb6b5b6 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -189,8 +189,8 @@ static void __dwc3_set_mode(struct work_struct *work)
>> if (ret) {
>> dev_err(dwc->dev, "failed to initialize host\n");
>> } else {
>> - if (dwc->usb2_phy)
>> - otg_set_vbus(dwc->usb2_phy->otg, true);
>> + if (dwc->usb2_phy[0])
>> + otg_set_vbus(dwc->usb2_phy[0]->otg, true);
>> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>> phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>> if (dwc->dis_split_quirk) {
>> @@ -204,9 +204,13 @@ static void __dwc3_set_mode(struct work_struct *work)
>> dwc3_core_soft_reset(dwc);
>>
>> dwc3_event_buffers_setup(dwc);
>> -
>> - if (dwc->usb2_phy)
>> - otg_set_vbus(dwc->usb2_phy->otg, false);
>> + /*
>> + * Multiport Controller works only in host mode.
>> + * There will only be one pair of HS and SS PHY for the controller operating in
>> + * device mode.
>> + */
> I think this comment applies to every single place where you operate on
> usb2_phy[0] only. But rather than telling the reader of the code that
> multiport is valid only for host-only setups, wouldn't it be useful to
> prevent the moving to device mode if multiple ports are specified?
>
> Or am I just not finding the place where you do this?
Yeah maybe adding comment just in one place might be Odd.
Multiport are host mode only, so transitioning to device mode will lead
to crash or will not be permitted.
We can add checks in dwc3_probe to bail out if it's multiport controller
and dr_mode is device/OTG.
>
>> + if (dwc->usb2_phy[0])
>> + otg_set_vbus(dwc->usb2_phy[0]->otg, false);
>> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
>> phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>>
>> @@ -825,15 +829,20 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
>>
>> static void dwc3_core_exit(struct dwc3 *dwc)
>> {
>> + int i;
>> dwc3_event_buffers_cleanup(dwc);
>>
>> - usb_phy_shutdown(dwc->usb2_phy);
>> - usb_phy_shutdown(dwc->usb3_phy);
>> + for (i = 0; i < dwc->num_hsphy; i++)
>> + usb_phy_shutdown(dwc->usb2_phy[i]);
>> + for (i = 0; i < dwc->num_ssphy; i++)
>> + usb_phy_shutdown(dwc->usb3_phy[i]);
>> phy_exit(dwc->usb2_generic_phy);
>> phy_exit(dwc->usb3_generic_phy);
>>
>> - usb_phy_set_suspend(dwc->usb2_phy, 1);
>> - usb_phy_set_suspend(dwc->usb3_phy, 1);
>> + for (i = 0; i < dwc->num_hsphy; i++)
>> + usb_phy_set_suspend(dwc->usb2_phy[i], 1);
>> + for (i = 0; i < dwc->num_ssphy; i++)
>> + usb_phy_set_suspend(dwc->usb3_phy[i], 1);
>> phy_power_off(dwc->usb2_generic_phy);
>> phy_power_off(dwc->usb3_generic_phy);
>> dwc3_clk_disable(dwc);
>> @@ -1038,7 +1047,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
>> {
>> unsigned int hw_mode;
>> u32 reg;
>> - int ret;
>> + int ret, i;
>>
>> hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>>
>> @@ -1066,8 +1075,10 @@ static int dwc3_core_init(struct dwc3 *dwc)
>> dwc->phys_ready = true;
>> }
>>
>> - usb_phy_init(dwc->usb2_phy);
>> - usb_phy_init(dwc->usb3_phy);
>> + for (i = 0; i < dwc->num_hsphy; i++)
>> + usb_phy_init(dwc->usb2_phy[i]);
>> + for (i = 0; i < dwc->num_ssphy; i++)
>> + usb_phy_init(dwc->usb3_phy[i]);
>> ret = phy_init(dwc->usb2_generic_phy);
>> if (ret < 0)
>> goto err0a;
>> @@ -1112,8 +1123,10 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>
>> dwc3_set_incr_burst_type(dwc);
>>
>> - usb_phy_set_suspend(dwc->usb2_phy, 0);
>> - usb_phy_set_suspend(dwc->usb3_phy, 0);
>> + for (i = 0; i < dwc->num_hsphy; i++)
>> + usb_phy_set_suspend(dwc->usb2_phy[i], 0);
>> + for (i = 0; i < dwc->num_ssphy; i++)
>> + usb_phy_set_suspend(dwc->usb3_phy[i], 0);
>> ret = phy_power_on(dwc->usb2_generic_phy);
>> if (ret < 0)
>> goto err2;
>> @@ -1234,12 +1247,16 @@ static int dwc3_core_init(struct dwc3 *dwc)
>> phy_power_off(dwc->usb2_generic_phy);
>>
>> err2:
>> - usb_phy_set_suspend(dwc->usb2_phy, 1);
>> - usb_phy_set_suspend(dwc->usb3_phy, 1);
>> + for (i = 0; i < dwc->num_hsphy; i++)
>> + usb_phy_set_suspend(dwc->usb2_phy[i], 1);
>> + for (i = 0; i < dwc->num_ssphy; i++)
>> + usb_phy_set_suspend(dwc->usb3_phy[i], 1);
>>
>> err1:
>> - usb_phy_shutdown(dwc->usb2_phy);
>> - usb_phy_shutdown(dwc->usb3_phy);
>> + for (i = 0; i < dwc->num_hsphy; i++)
>> + usb_phy_shutdown(dwc->usb2_phy[i]);
>> + for (i = 0; i < dwc->num_ssphy; i++)
>> + usb_phy_shutdown(dwc->usb3_phy[i]);
>> phy_exit(dwc->usb2_generic_phy);
>> phy_exit(dwc->usb3_generic_phy);
>>
>> @@ -1250,52 +1267,166 @@ static int dwc3_core_init(struct dwc3 *dwc)
>> return ret;
>> }
>>
>> +static struct usb_phy *dwc3_core_get_phy_by_handle_with_node(struct device *dev,
>> + const char *phandle, u8 index, struct device_node *lookup_node)
>> +{
>> + struct device_node *node;
>> + struct usb_phy *phy;
>> +
>> + node = of_parse_phandle(lookup_node, phandle, index);
>> + if (!node) {
>> + dev_err(dev, "failed to get %s phandle in %pOF node\n", phandle,
>> + dev->of_node);
>> + return ERR_PTR(-ENODEV);
>> + }
>> + phy = devm_usb_get_phy_by_node(dev, node, NULL);
>> + of_node_put(node);
>> + return phy;
>> +}
>> +
>> +static int dwc3_extract_num_phys(struct dwc3 *dwc)
> This doesn't extract the number, it allocates the usb2_phy and usb3_phy
> arrays.
I will push another change where for every "mport" node if the number of
USB Phy phandle is 1 then increment only num_hsphy.
If it's 2 then increase both num_hsphy, num_ssphy .
Assumption : Every port will have a HSPHY phandle.
>
>> +{
>> + struct device_node *ports, *port;
>> +
>> + /* Find if any "multiport" child is present inside DWC3*/
>> + for_each_available_child_of_node(dwc->dev->of_node, ports) {
>> + if (!strcmp(ports->name, "multiport"))
>> + break;
> If you break you need of_node_put().
>
>> + }
>> + if (!ports) {
>> + dwc->num_hsphy = 1;
>> + dwc->num_ssphy = 1;
>> + } else {
>> + for_each_available_child_of_node(ports, port) {
>> + dwc->num_hsphy += 1;
>> + dwc->num_ssphy += 1;
>> + }
>> + }
>> + dev_info(dwc->dev, "Num of HS and SS PHY are %u %u\n", dwc->num_hsphy, dwc->num_ssphy);
>> +
>> + dwc->usb2_phy = devm_kzalloc(dwc->dev,
>> + sizeof(*dwc->usb2_phy) * dwc->num_hsphy, GFP_KERNEL);
>> + if (!dwc->usb2_phy)
>> + return -ENOMEM;
>> +
>> + dwc->usb3_phy = devm_kzalloc(dwc->dev,
>> + sizeof(*dwc->usb3_phy) * dwc->num_ssphy, GFP_KERNEL);
>> + if (!dwc->usb3_phy)
>> + return -ENOMEM;
>> +
>> + return 0;
>> +}
>> +
>> static int dwc3_core_get_phy(struct dwc3 *dwc)
>> {
>> struct device *dev = dwc->dev;
>> struct device_node *node = dev->of_node;
>> - int ret;
>> + struct device_node *ports, *port;
>>
>> - if (node) {
>> - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
>> - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
>> - } else {
>> - dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>> - dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
>> - }
>> + int ret, i = 0;
>>
>> - if (IS_ERR(dwc->usb2_phy)) {
>> - ret = PTR_ERR(dwc->usb2_phy);
>> - if (ret == -ENXIO || ret == -ENODEV)
>> - dwc->usb2_phy = NULL;
>> - else
>> - return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> + ret = dwc3_extract_num_phys(dwc);
>> + if (ret) {
>> + dev_err(dwc->dev, "Unable to extract number of PHYs\n");
>> + return ret;
>> }
>>
>> - if (IS_ERR(dwc->usb3_phy)) {
>> - ret = PTR_ERR(dwc->usb3_phy);
>> - if (ret == -ENXIO || ret == -ENODEV)
>> - dwc->usb3_phy = NULL;
>> - else
>> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> + /* Find if any "multiport" child is present inside DWC3*/
>> + for_each_available_child_of_node(node, ports) {
>> + if (!strcmp(ports->name, "multiport"))
>> + break;
>> }
>>
>> - dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>> - if (IS_ERR(dwc->usb2_generic_phy)) {
>> - ret = PTR_ERR(dwc->usb2_generic_phy);
>> - if (ret == -ENOSYS || ret == -ENODEV)
>> - dwc->usb2_generic_phy = NULL;
>> - else
>> - return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> - }
>> + if (!ports) {
>> + if (node) {
>> + dwc->usb2_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
>> + dwc->usb3_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
>> + } else {
>> + dwc->usb2_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>> + dwc->usb3_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
>> + }
>>
>> - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>> - if (IS_ERR(dwc->usb3_generic_phy)) {
>> - ret = PTR_ERR(dwc->usb3_generic_phy);
>> - if (ret == -ENOSYS || ret == -ENODEV)
>> - dwc->usb3_generic_phy = NULL;
>> - else
>> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> + if (IS_ERR(dwc->usb2_phy[0])) {
>> + ret = PTR_ERR(dwc->usb2_phy[0]);
>> + if (ret == -ENXIO || ret == -ENODEV)
>> + dwc->usb2_phy[0] = NULL;
>> + else
>> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> + }
>> +
>> + if (IS_ERR(dwc->usb3_phy[0])) {
>> + ret = PTR_ERR(dwc->usb3_phy[0]);
>> + if (ret == -ENXIO || ret == -ENODEV)
>> + dwc->usb3_phy[0] = NULL;
>> + else
>> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> + }
>> +
>> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>> + if (IS_ERR(dwc->usb2_generic_phy)) {
>> + ret = PTR_ERR(dwc->usb2_generic_phy);
>> + if (ret == -ENOSYS || ret == -ENODEV)
>> + dwc->usb2_generic_phy = NULL;
>> + else
>> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> + }
>> +
>> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>> + if (IS_ERR(dwc->usb3_generic_phy)) {
>> + ret = PTR_ERR(dwc->usb3_generic_phy);
>> + if (ret == -ENOSYS || ret == -ENODEV)
>> + dwc->usb3_generic_phy = NULL;
>> + else
>> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> + }
>> +
>> + } else {
>> + pr_info("Multiport node found\n");
> Please remove your debug prints.
Sure
>
>> + /* Iterate over each port of the MultiPort */
>> + for_each_available_child_of_node(ports, port) {
>> + dwc->usb2_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
>> + 0, port);
>> + dwc->usb3_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
>> + 1, port);
>> +
>> + if (IS_ERR(dwc->usb2_phy[i])) {
>> + ret = PTR_ERR(dwc->usb2_phy[i]);
>> + pr_err("usb2_phy gone %d\n", ret);
>> + if (ret == -ENXIO || ret == -ENODEV)
>> + dwc->usb2_phy[i] = NULL;
>> + else
>> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> + }
>> +
>> + if (IS_ERR(dwc->usb3_phy[i])) {
>> + ret = PTR_ERR(dwc->usb3_phy[i]);
>> + pr_err("usb3_phy gone %d\n", ret);
>> + if (ret == -ENXIO || ret == -ENODEV)
>> + dwc->usb3_phy[i] = NULL;
>> + else
>> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> + }
>> + //TODO Write Generic PHY API
> Afaict this isn't a TODO, this is just broken right now?
I have tested these changes against "usb-phy" and not the generic PHY
approach.
We have "devm_of_get_phy" which can be used for Generic PHY support.
Thanks Pavan for suggesting this.
I will check this on upstream device.
>
> Regards,
> Bjorn
>
>> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>> + if (IS_ERR(dwc->usb2_generic_phy)) {
>> + ret = PTR_ERR(dwc->usb2_generic_phy);
>> + if (ret == -ENOSYS || ret == -ENODEV)
>> + dwc->usb2_generic_phy = NULL;
>> + else
>> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> + }
>> +
>> + //TODO Write Generic PHY API
>> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>> + if (IS_ERR(dwc->usb3_generic_phy)) {
>> + ret = PTR_ERR(dwc->usb3_generic_phy);
>> + if (ret == -ENOSYS || ret == -ENODEV)
>> + dwc->usb3_generic_phy = NULL;
>> + else
>> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> + }
>> + i++;
>> + }
>> }
>>
>> return 0;
>> @@ -1310,8 +1441,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>> case USB_DR_MODE_PERIPHERAL:
>> dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>>
>> - if (dwc->usb2_phy)
>> - otg_set_vbus(dwc->usb2_phy->otg, false);
>> + if (dwc->usb2_phy[0])
>> + otg_set_vbus(dwc->usb2_phy[0]->otg, false);
>> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
>> phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>>
>> @@ -1322,8 +1453,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>> case USB_DR_MODE_HOST:
>> dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
>>
>> - if (dwc->usb2_phy)
>> - otg_set_vbus(dwc->usb2_phy->otg, true);
>> + if (dwc->usb2_phy[0])
>> + otg_set_vbus(dwc->usb2_phy[0]->otg, true);
>> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>> phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>>
>> @@ -1673,7 +1804,7 @@ static int dwc3_probe(struct platform_device *pdev)
>> struct resource *res, dwc_res;
>> struct dwc3 *dwc;
>>
>> - int ret;
>> + int ret, i;
>>
>> void __iomem *regs;
>>
>> @@ -1838,13 +1969,17 @@ static int dwc3_probe(struct platform_device *pdev)
>> dwc3_debugfs_exit(dwc);
>> dwc3_event_buffers_cleanup(dwc);
>>
>> - usb_phy_shutdown(dwc->usb2_phy);
>> - usb_phy_shutdown(dwc->usb3_phy);
>> + for (i = 0; i < dwc->num_hsphy; i++)
>> + usb_phy_shutdown(dwc->usb2_phy[i]);
>> + for (i = 0; i < dwc->num_ssphy; i++)
>> + usb_phy_shutdown(dwc->usb3_phy[i]);
>> phy_exit(dwc->usb2_generic_phy);
>> phy_exit(dwc->usb3_generic_phy);
>>
>> - usb_phy_set_suspend(dwc->usb2_phy, 1);
>> - usb_phy_set_suspend(dwc->usb3_phy, 1);
>> + for (i = 0; i < dwc->num_hsphy; i++)
>> + usb_phy_set_suspend(dwc->usb2_phy[i], 1);
>> + for (i = 0; i < dwc->num_ssphy; i++)
>> + usb_phy_set_suspend(dwc->usb3_phy[i], 1);
>> phy_power_off(dwc->usb2_generic_phy);
>> phy_power_off(dwc->usb3_generic_phy);
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 81c486b..3175ed9 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1020,6 +1020,8 @@ struct dwc3_scratchpad_array {
>> * @usb_psy: pointer to power supply interface.
>> * @usb2_phy: pointer to USB2 PHY
>> * @usb3_phy: pointer to USB3 PHY
>> + * @num_hsphy: Number of HS ports controlled by the core
>> + * @num_dsphy: Number of SS ports controlled by the core
>> * @usb2_generic_phy: pointer to USB2 PHY
>> * @usb3_generic_phy: pointer to USB3 PHY
>> * @phys_ready: flag to indicate that PHYs are ready
>> @@ -1147,8 +1149,10 @@ struct dwc3 {
>>
>> struct reset_control *reset;
>>
>> - struct usb_phy *usb2_phy;
>> - struct usb_phy *usb3_phy;
>> + struct usb_phy **usb2_phy;
>> + struct usb_phy **usb3_phy;
>> + u32 num_hsphy;
>> + u32 num_ssphy;
>>
>> struct phy *usb2_generic_phy;
>> struct phy *usb3_generic_phy;
>> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
>> index 039bf24..7c9eba6 100644
>> --- a/drivers/usb/dwc3/drd.c
>> +++ b/drivers/usb/dwc3/drd.c
>> @@ -384,8 +384,8 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
>> if (ret) {
>> dev_err(dwc->dev, "failed to initialize host\n");
>> } else {
>> - if (dwc->usb2_phy)
>> - otg_set_vbus(dwc->usb2_phy->otg, true);
>> + if (dwc->usb2_phy[0])
>> + otg_set_vbus(dwc->usb2_phy[0]->otg, true);
>> if (dwc->usb2_generic_phy)
>> phy_set_mode(dwc->usb2_generic_phy,
>> PHY_MODE_USB_HOST);
>> @@ -398,8 +398,8 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
>> dwc3_event_buffers_setup(dwc);
>> spin_unlock_irqrestore(&dwc->lock, flags);
>>
>> - if (dwc->usb2_phy)
>> - otg_set_vbus(dwc->usb2_phy->otg, false);
>> + if (dwc->usb2_phy[0])
>> + otg_set_vbus(dwc->usb2_phy[0]->otg, false);
>> if (dwc->usb2_generic_phy)
>> phy_set_mode(dwc->usb2_generic_phy,
>> PHY_MODE_USB_DEVICE);
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 4f54f0e..c58a67c 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2870,8 +2870,8 @@ static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA)
>> union power_supply_propval val = {0};
>> int ret;
>>
>> - if (dwc->usb2_phy)
>> - return usb_phy_set_power(dwc->usb2_phy, mA);
>> + if (dwc->usb2_phy[0])
>> + return usb_phy_set_power(dwc->usb2_phy[0], mA);
>>
>> if (!dwc->usb_psy)
>> return -EOPNOTSUPP;
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller
2022-05-19 12:34 ` [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller Harsh Agarwal
2022-05-21 3:17 ` Bjorn Andersson
@ 2022-05-23 2:59 ` Pavan Kondeti
2022-05-23 7:08 ` Pavan Kondeti
2022-05-23 16:10 ` Brian Masney
3 siblings, 0 replies; 16+ messages in thread
From: Pavan Kondeti @ 2022-05-23 2:59 UTC (permalink / raw)
To: Harsh Agarwal
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Felipe Balbi, linux-usb, devicetree, linux-kernel, quic_pkondeti,
quic_ppratap
Hi Harsh,
I know that device tree bindings are not locked yet. So feel free to
address/respon to these comments after the bindings are locked.
On Thu, May 19, 2022 at 06:04:55PM +0530, Harsh Agarwal wrote:
> Currently the USB driver supports only single port controller
> which works with 2 PHYs at max ie HS and SS.
>
> But some devices have "multiport" controller where a single
> controller supports multiple ports and each port have their own
> PHYs. Refactor PHY logic to support the same.
>
> This implementation is compatible with existing glue drivers.
>
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> ---
> drivers/usb/dwc3/core.c | 259 +++++++++++++++++++++++++++++++++++-----------
> drivers/usb/dwc3/core.h | 8 +-
> drivers/usb/dwc3/drd.c | 8 +-
> drivers/usb/dwc3/gadget.c | 4 +-
> 4 files changed, 209 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2682469..8eb6b5b6 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -189,8 +189,8 @@ static void __dwc3_set_mode(struct work_struct *work)
> if (ret) {
> dev_err(dwc->dev, "failed to initialize host\n");
> } else {
> - if (dwc->usb2_phy)
> - otg_set_vbus(dwc->usb2_phy->otg, true);
> + if (dwc->usb2_phy[0])
> + otg_set_vbus(dwc->usb2_phy[0]->otg, true);
> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
> phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
> if (dwc->dis_split_quirk) {
> @@ -204,9 +204,13 @@ static void __dwc3_set_mode(struct work_struct *work)
> dwc3_core_soft_reset(dwc);
>
> dwc3_event_buffers_setup(dwc);
> -
> - if (dwc->usb2_phy)
> - otg_set_vbus(dwc->usb2_phy->otg, false);
> + /*
> + * Multiport Controller works only in host mode.
> + * There will only be one pair of HS and SS PHY for the controller operating in
> + * device mode.
> + */
> + if (dwc->usb2_phy[0])
> + otg_set_vbus(dwc->usb2_phy[0]->otg, false);
Fair enough. As per my understanding, the DWC3 controller can't keep one port in
device mode and another port in host mode since GCTL[12:13] bits which control
the port capability direction are controller specific but not port specific.
So it is okay to assume that the controller operating in device mode will only
have one USB2 PHY.
Your comment needs some correction though. We don't need both HS and SS PHY.
having just HS PHY is sufficient, in fact the HS PHY is also optional hence
the NULL check here.
> phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
> phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>
<snip>
> @@ -1250,52 +1267,166 @@ static int dwc3_core_init(struct dwc3 *dwc)
> return ret;
> }
>
> +static struct usb_phy *dwc3_core_get_phy_by_handle_with_node(struct device *dev,
> + const char *phandle, u8 index, struct device_node *lookup_node)
> +{
> + struct device_node *node;
> + struct usb_phy *phy;
> +
> + node = of_parse_phandle(lookup_node, phandle, index);
> + if (!node) {
> + dev_err(dev, "failed to get %s phandle in %pOF node\n", phandle,
> + dev->of_node);
> + return ERR_PTR(-ENODEV);
> + }
> + phy = devm_usb_get_phy_by_node(dev, node, NULL);
> + of_node_put(node);
> + return phy;
> +}
> +
> +static int dwc3_extract_num_phys(struct dwc3 *dwc)
> +{
> + struct device_node *ports, *port;
> +
> + /* Find if any "multiport" child is present inside DWC3*/
nit pick. space after DWC3.
> + for_each_available_child_of_node(dwc->dev->of_node, ports) {
> + if (!strcmp(ports->name, "multiport"))
> + break;
> + }
> + if (!ports) {
> + dwc->num_hsphy = 1;
> + dwc->num_ssphy = 1;
> + } else {
> + for_each_available_child_of_node(ports, port) {
> + dwc->num_hsphy += 1;
> + dwc->num_ssphy += 1;
> + }
> + }
Would of_get_child_count() work?
I understand that we need to count the number of HS and SS PHY here for two
reasons here.
1. To allocate USB/Generic PHY structures inside dwc3.
2. We need to loop around the phy count to write into GUSB2PHYCFG/GUSB3PIPECTL
registers.
However, is it not possible that a port is HS only and not have any SS PHY
associated. Incrementing dwc->num_ssphy is not entirely correct for every
child. We need to increment dwc->num_ssphy only when get the correct phandle
for SS PHY.
It may be easier to allocate instances as per the child count but increment
dwc->num_ssphy as and when we process phys.
> + dev_info(dwc->dev, "Num of HS and SS PHY are %u %u\n", dwc->num_hsphy, dwc->num_ssphy);
> +
> + dwc->usb2_phy = devm_kzalloc(dwc->dev,
> + sizeof(*dwc->usb2_phy) * dwc->num_hsphy, GFP_KERNEL);
> + if (!dwc->usb2_phy)
> + return -ENOMEM;
> +
> + dwc->usb3_phy = devm_kzalloc(dwc->dev,
> + sizeof(*dwc->usb3_phy) * dwc->num_ssphy, GFP_KERNEL);
> + if (!dwc->usb3_phy)
> + return -ENOMEM;
> +
> + return 0;
> +}
we have to allocate sufficient instances for generic PHY as well. I understand
that this is a RFC patch at this point but mentioning here so that you plan
accordingly.
> +
> static int dwc3_core_get_phy(struct dwc3 *dwc)
> {
> struct device *dev = dwc->dev;
> struct device_node *node = dev->of_node;
> - int ret;
> + struct device_node *ports, *port;
>
> - if (node) {
> - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> - } else {
> - dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> - dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> - }
> + int ret, i = 0;
>
> - if (IS_ERR(dwc->usb2_phy)) {
> - ret = PTR_ERR(dwc->usb2_phy);
> - if (ret == -ENXIO || ret == -ENODEV)
> - dwc->usb2_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + ret = dwc3_extract_num_phys(dwc);
> + if (ret) {
> + dev_err(dwc->dev, "Unable to extract number of PHYs\n");
> + return ret;
> }
>
> - if (IS_ERR(dwc->usb3_phy)) {
> - ret = PTR_ERR(dwc->usb3_phy);
> - if (ret == -ENXIO || ret == -ENODEV)
> - dwc->usb3_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + /* Find if any "multiport" child is present inside DWC3*/
> + for_each_available_child_of_node(node, ports) {
> + if (!strcmp(ports->name, "multiport"))
> + break;
> }
>
> - dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> - if (IS_ERR(dwc->usb2_generic_phy)) {
> - ret = PTR_ERR(dwc->usb2_generic_phy);
> - if (ret == -ENOSYS || ret == -ENODEV)
> - dwc->usb2_generic_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> - }
> + if (!ports) {
> + if (node) {
> + dwc->usb2_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> + dwc->usb3_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> + } else {
> + dwc->usb2_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> + dwc->usb3_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> + }
>
> - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> - if (IS_ERR(dwc->usb3_generic_phy)) {
> - ret = PTR_ERR(dwc->usb3_generic_phy);
> - if (ret == -ENOSYS || ret == -ENODEV)
> - dwc->usb3_generic_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + if (IS_ERR(dwc->usb2_phy[0])) {
> + ret = PTR_ERR(dwc->usb2_phy[0]);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb2_phy[0] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
> +
> + if (IS_ERR(dwc->usb3_phy[0])) {
> + ret = PTR_ERR(dwc->usb3_phy[0]);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb3_phy[0] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + }
> +
> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> + if (IS_ERR(dwc->usb2_generic_phy)) {
> + ret = PTR_ERR(dwc->usb2_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb2_generic_phy = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
> +
> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> + if (IS_ERR(dwc->usb3_generic_phy)) {
> + ret = PTR_ERR(dwc->usb3_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb3_generic_phy = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + }
> +
Would be good to write a common wrapper that gets the USB PHY phandles
and works for each mport (if present) or the fallback case. That wrapper
function should take required arguments and look for usb2/usb3 phy as well as
generic phy.
> + } else {
> + pr_info("Multiport node found\n");
> + /* Iterate over each port of the MultiPort */
> + for_each_available_child_of_node(ports, port) {
> + dwc->usb2_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
> + 0, port);
> + dwc->usb3_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
> + 1, port);
> +
> + if (IS_ERR(dwc->usb2_phy[i])) {
> + ret = PTR_ERR(dwc->usb2_phy[i]);
> + pr_err("usb2_phy gone %d\n", ret);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb2_phy[i] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
> +
> + if (IS_ERR(dwc->usb3_phy[i])) {
> + ret = PTR_ERR(dwc->usb3_phy[i]);
> + pr_err("usb3_phy gone %d\n", ret);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb3_phy[i] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + }
> + //TODO Write Generic PHY API
> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> + if (IS_ERR(dwc->usb2_generic_phy)) {
> + ret = PTR_ERR(dwc->usb2_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb2_generic_phy = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
> +
> + //TODO Write Generic PHY API
> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> + if (IS_ERR(dwc->usb3_generic_phy)) {
> + ret = PTR_ERR(dwc->usb3_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb3_generic_phy = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + }
> + i++;
> + }
> }
>
> return 0;
<snip>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 81c486b..3175ed9 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1020,6 +1020,8 @@ struct dwc3_scratchpad_array {
> * @usb_psy: pointer to power supply interface.
> * @usb2_phy: pointer to USB2 PHY
> * @usb3_phy: pointer to USB3 PHY
> + * @num_hsphy: Number of HS ports controlled by the core
> + * @num_dsphy: Number of SS ports controlled by the core
> * @usb2_generic_phy: pointer to USB2 PHY
> * @usb3_generic_phy: pointer to USB3 PHY
> * @phys_ready: flag to indicate that PHYs are ready
> @@ -1147,8 +1149,10 @@ struct dwc3 {
>
> struct reset_control *reset;
>
> - struct usb_phy *usb2_phy;
> - struct usb_phy *usb3_phy;
> + struct usb_phy **usb2_phy;
> + struct usb_phy **usb3_phy;
> + u32 num_hsphy;
> + u32 num_ssphy;
>
> struct phy *usb2_generic_phy;
> struct phy *usb3_generic_phy;
The generic phy also needs to be supported/handled.
Thanks,
Pavan
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller
2022-05-19 12:34 ` [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller Harsh Agarwal
2022-05-21 3:17 ` Bjorn Andersson
2022-05-23 2:59 ` Pavan Kondeti
@ 2022-05-23 7:08 ` Pavan Kondeti
2022-05-23 16:10 ` Brian Masney
3 siblings, 0 replies; 16+ messages in thread
From: Pavan Kondeti @ 2022-05-23 7:08 UTC (permalink / raw)
To: Harsh Agarwal
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Felipe Balbi, linux-usb, devicetree, linux-kernel, quic_pkondeti,
quic_ppratap
On Thu, May 19, 2022 at 06:04:55PM +0530, Harsh Agarwal wrote:
> Currently the USB driver supports only single port controller
> which works with 2 PHYs at max ie HS and SS.
>
> But some devices have "multiport" controller where a single
> controller supports multiple ports and each port have their own
> PHYs. Refactor PHY logic to support the same.
>
> This implementation is compatible with existing glue drivers.
>
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
<snip>
> +static struct usb_phy *dwc3_core_get_phy_by_handle_with_node(struct device *dev,
> + const char *phandle, u8 index, struct device_node *lookup_node)
> +{
> + struct device_node *node;
> + struct usb_phy *phy;
> +
> + node = of_parse_phandle(lookup_node, phandle, index);
> + if (!node) {
> + dev_err(dev, "failed to get %s phandle in %pOF node\n", phandle,
> + dev->of_node);
> + return ERR_PTR(-ENODEV);
> + }
> + phy = devm_usb_get_phy_by_node(dev, node, NULL);
> + of_node_put(node);
> + return phy;
> +}
> +
we have devm_of_phy_get() API that takes both device and device_node (could be
other than device-of_node). This API you have to use for the generic PHY, so
we can have a similar wrapper with devm_of_usb_get_phy_by_phandle() which
takes device and device_node as arguments.
> +static int dwc3_extract_num_phys(struct dwc3 *dwc)
> +{
> + struct device_node *ports, *port;
> +
> + /* Find if any "multiport" child is present inside DWC3*/
> + for_each_available_child_of_node(dwc->dev->of_node, ports) {
> + if (!strcmp(ports->name, "multiport"))
> + break;
> + }
> + if (!ports) {
> + dwc->num_hsphy = 1;
> + dwc->num_ssphy = 1;
> + } else {
> + for_each_available_child_of_node(ports, port) {
> + dwc->num_hsphy += 1;
> + dwc->num_ssphy += 1;
> + }
> + }
> + dev_info(dwc->dev, "Num of HS and SS PHY are %u %u\n", dwc->num_hsphy, dwc->num_ssphy);
> +
> + dwc->usb2_phy = devm_kzalloc(dwc->dev,
> + sizeof(*dwc->usb2_phy) * dwc->num_hsphy, GFP_KERNEL);
> + if (!dwc->usb2_phy)
> + return -ENOMEM;
> +
> + dwc->usb3_phy = devm_kzalloc(dwc->dev,
> + sizeof(*dwc->usb3_phy) * dwc->num_ssphy, GFP_KERNEL);
> + if (!dwc->usb3_phy)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> static int dwc3_core_get_phy(struct dwc3 *dwc)
> {
> struct device *dev = dwc->dev;
> struct device_node *node = dev->of_node;
> - int ret;
> + struct device_node *ports, *port;
>
> - if (node) {
> - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> - } else {
> - dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> - dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> - }
> + int ret, i = 0;
>
> - if (IS_ERR(dwc->usb2_phy)) {
> - ret = PTR_ERR(dwc->usb2_phy);
> - if (ret == -ENXIO || ret == -ENODEV)
> - dwc->usb2_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + ret = dwc3_extract_num_phys(dwc);
> + if (ret) {
> + dev_err(dwc->dev, "Unable to extract number of PHYs\n");
> + return ret;
> }
>
> - if (IS_ERR(dwc->usb3_phy)) {
> - ret = PTR_ERR(dwc->usb3_phy);
> - if (ret == -ENXIO || ret == -ENODEV)
> - dwc->usb3_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + /* Find if any "multiport" child is present inside DWC3*/
> + for_each_available_child_of_node(node, ports) {
> + if (!strcmp(ports->name, "multiport"))
> + break;
> }
>
> - dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> - if (IS_ERR(dwc->usb2_generic_phy)) {
> - ret = PTR_ERR(dwc->usb2_generic_phy);
> - if (ret == -ENOSYS || ret == -ENODEV)
> - dwc->usb2_generic_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> - }
> + if (!ports) {
> + if (node) {
> + dwc->usb2_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> + dwc->usb3_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> + } else {
> + dwc->usb2_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> + dwc->usb3_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> + }
>
> - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> - if (IS_ERR(dwc->usb3_generic_phy)) {
> - ret = PTR_ERR(dwc->usb3_generic_phy);
> - if (ret == -ENOSYS || ret == -ENODEV)
> - dwc->usb3_generic_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + if (IS_ERR(dwc->usb2_phy[0])) {
> + ret = PTR_ERR(dwc->usb2_phy[0]);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb2_phy[0] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
> +
> + if (IS_ERR(dwc->usb3_phy[0])) {
> + ret = PTR_ERR(dwc->usb3_phy[0]);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb3_phy[0] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + }
> +
> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> + if (IS_ERR(dwc->usb2_generic_phy)) {
> + ret = PTR_ERR(dwc->usb2_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb2_generic_phy = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
Like I said above, devm_of_phy_get() is what needs to be used if we want to
re-use the same block of code that works for existing and multiport case.
Thanks,
Pavan
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller
2022-05-19 12:34 ` [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller Harsh Agarwal
` (2 preceding siblings ...)
2022-05-23 7:08 ` Pavan Kondeti
@ 2022-05-23 16:10 ` Brian Masney
2022-05-27 10:56 ` Harsh Agarwal
3 siblings, 1 reply; 16+ messages in thread
From: Brian Masney @ 2022-05-23 16:10 UTC (permalink / raw)
To: Harsh Agarwal
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Felipe Balbi, linux-usb, devicetree, linux-kernel, quic_pkondeti,
quic_ppratap
On Thu, May 19, 2022 at 06:04:55PM +0530, Harsh Agarwal wrote:
> - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> - if (IS_ERR(dwc->usb3_generic_phy)) {
> - ret = PTR_ERR(dwc->usb3_generic_phy);
> - if (ret == -ENOSYS || ret == -ENODEV)
> - dwc->usb3_generic_phy = NULL;
> - else
> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + if (IS_ERR(dwc->usb2_phy[0])) {
> + ret = PTR_ERR(dwc->usb2_phy[0]);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb2_phy[0] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
> +
> + if (IS_ERR(dwc->usb3_phy[0])) {
> + ret = PTR_ERR(dwc->usb3_phy[0]);
> + if (ret == -ENXIO || ret == -ENODEV)
> + dwc->usb3_phy[0] = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + }
> +
> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> + if (IS_ERR(dwc->usb2_generic_phy)) {
> + ret = PTR_ERR(dwc->usb2_generic_phy);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb2_generic_phy = NULL;
> + else
> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> + }
I know that this block is a copy and paste move from above, but is the
ENOSYS check really needed? It looks like the phy_get() only returns
-ENODEV.
> @@ -1147,8 +1149,10 @@ struct dwc3 {
>
> struct reset_control *reset;
>
> - struct usb_phy *usb2_phy;
> - struct usb_phy *usb3_phy;
> + struct usb_phy **usb2_phy;
> + struct usb_phy **usb3_phy;
> + u32 num_hsphy;
> + u32 num_ssphy;
Rename num_hsphy / num_ssphy to num_usb2_phy and num_usb3_phy so this is
easier to audit.
Brian
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller
2022-05-23 16:10 ` Brian Masney
@ 2022-05-27 10:56 ` Harsh Agarwal
0 siblings, 0 replies; 16+ messages in thread
From: Harsh Agarwal @ 2022-05-27 10:56 UTC (permalink / raw)
To: Brian Masney
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Felipe Balbi, linux-usb, devicetree, linux-kernel, quic_pkondeti,
quic_ppratap
On 5/23/2022 9:40 PM, Brian Masney wrote:
> On Thu, May 19, 2022 at 06:04:55PM +0530, Harsh Agarwal wrote:
>> - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>> - if (IS_ERR(dwc->usb3_generic_phy)) {
>> - ret = PTR_ERR(dwc->usb3_generic_phy);
>> - if (ret == -ENOSYS || ret == -ENODEV)
>> - dwc->usb3_generic_phy = NULL;
>> - else
>> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> + if (IS_ERR(dwc->usb2_phy[0])) {
>> + ret = PTR_ERR(dwc->usb2_phy[0]);
>> + if (ret == -ENXIO || ret == -ENODEV)
>> + dwc->usb2_phy[0] = NULL;
>> + else
>> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> + }
>> +
>> + if (IS_ERR(dwc->usb3_phy[0])) {
>> + ret = PTR_ERR(dwc->usb3_phy[0]);
>> + if (ret == -ENXIO || ret == -ENODEV)
>> + dwc->usb3_phy[0] = NULL;
>> + else
>> + return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> + }
>> +
>> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>> + if (IS_ERR(dwc->usb2_generic_phy)) {
>> + ret = PTR_ERR(dwc->usb2_generic_phy);
>> + if (ret == -ENOSYS || ret == -ENODEV)
>> + dwc->usb2_generic_phy = NULL;
>> + else
>> + return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> + }
> I know that this block is a copy and paste move from above, but is the
> ENOSYS check really needed? It looks like the phy_get() only returns
> -ENODEV.
sure I got ENOSYS removed in my RFC V2 patch. This was present by
default, so I did not change it earlier.
>> @@ -1147,8 +1149,10 @@ struct dwc3 {
>>
>> struct reset_control *reset;
>>
>> - struct usb_phy *usb2_phy;
>> - struct usb_phy *usb3_phy;
>> + struct usb_phy **usb2_phy;
>> + struct usb_phy **usb3_phy;
>> + u32 num_hsphy;
>> + u32 num_ssphy;
> Rename num_hsphy / num_ssphy to num_usb2_phy and num_usb3_phy so this is
> easier to audit.
Okay will change this in my next Patch.
>
> Brian
>
^ permalink raw reply [flat|nested] 16+ messages in thread