Hi Harsh, On Tue, May 31, 2022 at 01:50:17PM +0530, Harsh Agarwal wrote: > Currently the DWC3 driver supports only single port controller > which requires at most 2 PHYs ie HS and SS PHYs. > > But some SOCs have a "multiport" USB DWC3 controller where a > single controller supports multiple ports and each port have > their own PHYs. Refactor PHY logic to support the same. > > Signed-off-by: Harsh Agarwal <quic_harshq@xxxxxxxxxxx> > --- > drivers/usb/dwc3/core.c | 400 +++++++++++++++++++++++++++++++++------------- > drivers/usb/dwc3/core.h | 12 +- > drivers/usb/dwc3/drd.c | 16 +- > drivers/usb/dwc3/gadget.c | 4 +- > 4 files changed, 305 insertions(+), 127 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 5734219..5cc799e 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work) > { > struct dwc3 *dwc = work_to_dwc(work); > unsigned long flags; > - int ret; > + int i, ret; > u32 reg; > <snip> > -static int dwc3_core_get_phy(struct dwc3 *dwc) > +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; > +} Remove this function definition, since we moved this to PHY library. > + > +static int dwc3_count_phys(struct dwc3 *dwc, struct device_node *lookup_node) > +{ > + int count; > + > + count = of_count_phandle_with_args(lookup_node, "phys", NULL); > + > + if (count == -ENOENT) > + count = of_count_phandle_with_args(lookup_node, "usb-phy", NULL); > + > + if (count == 1) { > + dwc->num_usb2_phy++; > + } else if (count == 2) { > + dwc->num_usb2_phy++; > + dwc->num_usb3_phy++; > + } else { > + return count; > + } > + return 0; > +} > + > +static int dwc3_extract_num_phys(struct dwc3 *dwc) > +{ > + struct device_node *ports, *port; > + int ret; > + > + /* 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_usb2_phy = 1; > + dwc->num_usb3_phy = 1; > + } else { > + for_each_available_child_of_node(ports, port) { > + ret = dwc3_count_phys(dwc, port); > + if (ret) > + return ret; If you break the loop, you have to call of_node_put() explicitly. > + } > + } > + dev_info(dwc->dev, "Num of HS and SS PHY are %u %u\n", dwc->num_usb2_phy, > + dwc->num_usb3_phy); If I declare the multiport node don't specify any phys for any of the ports, we end up coming here with dwc->num_usb2_phy = dwc->num_usb3_phy = 0. That is a problem since we access dwc->usb2_phy[0] directly. Can we bail out in that case? > + > + dwc->usb2_phy = devm_kzalloc(dwc->dev, > + sizeof(*dwc->usb2_phy) * dwc->num_usb2_phy, GFP_KERNEL); > + if (!dwc->usb2_phy) > + return -ENOMEM; > + > + dwc->usb3_phy = devm_kzalloc(dwc->dev, > + sizeof(*dwc->usb3_phy) * dwc->num_usb3_phy, GFP_KERNEL); > + if (!dwc->usb3_phy) > + return -ENOMEM; > + > + dwc->usb2_generic_phy = devm_kzalloc(dwc->dev, > + sizeof(*dwc->usb2_generic_phy) * dwc->num_usb2_phy, GFP_KERNEL); > + if (!dwc->usb2_generic_phy) > + return -ENOMEM; > + > + dwc->usb3_generic_phy = devm_kzalloc(dwc->dev, > + sizeof(*dwc->usb3_generic_phy) * dwc->num_usb3_phy, GFP_KERNEL); > + if (!dwc->usb3_generic_phy) > + return -ENOMEM; > + > + return 0; > +} > + > +static int dwc3_core_get_phy_by_node(struct dwc3 *dwc, > + struct device_node *lookup_node, int i) > { > struct device *dev = dwc->dev; > - struct device_node *node = dev->of_node; > - int ret; > + int ret; > > - 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); > + if (lookup_node) { > + dwc->usb2_phy[i] = devm_of_usb_get_phy_by_phandle(dev, > + "usb-phy", 0, lookup_node); > + dwc->usb3_phy[i] = devm_of_usb_get_phy_by_phandle(dev, > + "usb-phy", 1, lookup_node); > } 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); > + dwc->usb2_phy[i] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); > + dwc->usb3_phy[i] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3); > } > > - if (IS_ERR(dwc->usb2_phy)) { > - ret = PTR_ERR(dwc->usb2_phy); > + if (IS_ERR(dwc->usb2_phy[i])) { > + ret = PTR_ERR(dwc->usb2_phy[i]); > if (ret == -ENXIO || ret == -ENODEV) > - dwc->usb2_phy = NULL; > + dwc->usb2_phy[i] = NULL; > else > return dev_err_probe(dev, ret, "no usb2 phy configured\n"); > } > > - if (IS_ERR(dwc->usb3_phy)) { > - ret = PTR_ERR(dwc->usb3_phy); > + if (IS_ERR(dwc->usb3_phy[i])) { > + ret = PTR_ERR(dwc->usb3_phy[i]); > if (ret == -ENXIO || ret == -ENODEV) > - dwc->usb3_phy = NULL; > + dwc->usb3_phy[i] = 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; > + dwc->usb2_generic_phy[i] = devm_of_phy_get(dev, lookup_node, "usb2-phy"); > + if (IS_ERR(dwc->usb2_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb2_generic_phy[i]); > + if (ret == -ENODEV) > + dwc->usb2_generic_phy[i] = 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; > + dwc->usb3_generic_phy[i] = devm_of_phy_get(dev, lookup_node, "usb3-phy"); > + if (IS_ERR(dwc->usb3_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb3_generic_phy[i]); > + if (ret == -ENODEV) > + dwc->usb3_generic_phy[i] = NULL; > else > return dev_err_probe(dev, ret, "no usb3 phy configured\n"); > } > + return 0; > +} > + > +static int dwc3_core_get_phy(struct dwc3 *dwc) > +{ > + struct device *dev = dwc->dev; > + struct device_node *node = dev->of_node; > + struct device_node *ports, *port; > + int ret, i = 0; > + > + ret = dwc3_extract_num_phys(dwc); > + if (ret) { > + dev_err(dwc->dev, "Unable to extract number of PHYs\n"); > + return ret; > + } > + > + /* Find if any "multiport" child is present inside DWC3*/ > + for_each_available_child_of_node(node, ports) { > + if (!strcmp(ports->name, "multiport")) > + break; > + } > + > + if (!ports) { > + ret = dwc3_core_get_phy_by_node(dwc, node, 0); > + if (ret) > + return ret; > + } else { > + for_each_available_child_of_node(ports, port) { > + ret = dwc3_core_get_phy_by_node(dwc, port, i); > + if (ret) > + return ret; > + i++; > + } > + } > How does this work actually for a case where I have 4 ports with hs_phy=4 and ss_phy=2. Here we end up calling i = {0, 1, 2, 3} and accessing un-allocated ss phy instances in dwc3_core_get_phy_by_node(). you should also handle the case where first two ports are HS only and last two ports are both HS and SS. Thanks, Pavan