| Message ID | 20240811-dwc3-refactor-v2-6-91f370d61ad2@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | usb: dwc3: qcom: Flatten dwc3 structure | expand |
Hi Bjorn, kernel test robot noticed the following build errors: [auto build test ERROR on 864b1099d16fc7e332c3ad7823058c65f890486c] url: https://github.com/intel-lab-lkp/linux/commits/Bjorn-Andersson/dt-bindings-usb-snps-dwc3-Split-core-description/20240812-111102 base: 864b1099d16fc7e332c3ad7823058c65f890486c patch link: https://lore.kernel.org/r/20240811-dwc3-refactor-v2-6-91f370d61ad2%40quicinc.com patch subject: [PATCH v2 6/7] usb: dwc3: qcom: Transition to flattened model config: i386-buildonly-randconfig-002-20240812 (https://download.01.org/0day-ci/archive/20240812/202408122114.xvZqrjZ7-lkp@intel.com/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240812/202408122114.xvZqrjZ7-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408122114.xvZqrjZ7-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/usb/dwc3/dwc3-qcom.c:28:10: fatal error: 'glue.h' file not found 28 | #include "glue.h" | ^~~~~~~~ 1 error generated. vim +28 drivers/usb/dwc3/dwc3-qcom.c > 28 #include "glue.h" 29
On Sun, Aug 11, 2024 at 08:12:03PM -0700, Bjorn Andersson wrote: > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > The USB IP-block found in most Qualcomm platforms is modelled in the > Linux kernel as 3 different independent device drivers, but as shown by > the already existing layering violations in the Qualcomm glue driver > they can not be operated independently. > > With the current implementation, the glue driver registers the core and > has no way to know when this is done. As a result, e.g. the suspend > callbacks needs to guard against NULL pointer dereferences when trying > to peek into the struct dwc3 found in the drvdata of the child. > Even with these checks, there are no way to fully protect ourselves from > the race conditions that occur if the DWC3 is unbound. > > Missing from the upstream Qualcomm USB support is handling of role > switching, in which the glue needs to be notified upon DRD mode changes. > Several attempts has been made through the years to register callbacks > etc, but they always fall short when it comes to handling of the core's > probe deferral on resources etc. > > Moving to a model where the DWC3 core is instantiated in a synchronous > fashion avoids above described race conditions. > > It is however not feasible to do so without also flattening the > DeviceTree binding, as assumptions are made in the DWC3 core and > frameworks used that the device's associated of_node will the that of > the core. Furthermore, the DeviceTree binding is a direct > representation of the Linux driver model, and doesn't necessarily > describe "the USB IP-block". > > The Qualcomm DWC3 glue driver is therefor transitioned to initialize and > operate the DWC3 within the one device context, in synchronous fashion. > > To handle backwards compatibility, and to remove the two-device model, > of_nodes of the old compatible are converted to the new one, early > during probe. > > This happens in the event that a DWC3 core child node is present, the > content of the reg and interrupt properties of this node are merged into > the shared properties, all remaining properties are copied and the core > node is dropped. Effectively transitioning the node from qcom,dwc3 to > qcom,snps-dwc3. In the past we've done this old binding to new binding with an overlay embedded in the kernel. The overlay would just be the .dts change you've made (we should have a tool that takes 2 DTs and generates an overlay as the diff). I suppose it's a few platforms here, but then it is just data and easily deleted when no longer needed (I think the cases I'm remembering did just that because they are gone now. It was TI display and Renesas media stuff IIRC). > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 310 +++++++++++++++++++++++++++++++++++-------- > 1 file changed, 251 insertions(+), 59 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 33de03f2d782..27b5013cd411 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -6,6 +6,7 @@ > > #include <linux/io.h> > #include <linux/of.h> > +#include <linux/cleanup.h> > #include <linux/clk.h> > #include <linux/irq.h> > #include <linux/of_clk.h> > @@ -13,6 +14,8 @@ > #include <linux/kernel.h> > #include <linux/extcon.h> > #include <linux/interconnect.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> Not a good sign when these are needed... > #include <linux/of_platform.h> Probably don't need this one. > #include <linux/platform_device.h> > #include <linux/phy/phy.h> > @@ -22,6 +25,7 @@ > #include <linux/usb/hcd.h> > #include <linux/usb.h> > #include "core.h" > +#include "glue.h" > > /* USB QSCRATCH Hardware registers */ > #define QSCRATCH_HS_PHY_CTRL 0x10 > @@ -72,7 +76,7 @@ struct dwc3_qcom_port { > struct dwc3_qcom { > struct device *dev; > void __iomem *qscratch_base; > - struct platform_device *dwc3; > + struct dwc3 *dwc; > struct clk **clks; > int num_clocks; > struct reset_control *resets; > @@ -259,7 +263,7 @@ static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom) > goto put_path_ddr; > } > > - max_speed = usb_get_maximum_speed(&qcom->dwc3->dev); > + max_speed = usb_get_maximum_speed(qcom->dwc->dev); > if (max_speed >= USB_SPEED_SUPER || max_speed == USB_SPEED_UNKNOWN) { > ret = icc_set_bw(qcom->icc_path_ddr, > USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW); > @@ -302,25 +306,16 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) > /* Only usable in contexts where the role can not change. */ > static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) > { > - struct dwc3 *dwc; > - > - /* > - * FIXME: Fix this layering violation. > - */ > - dwc = platform_get_drvdata(qcom->dwc3); > - > - /* Core driver may not have probed yet. */ > - if (!dwc) > - return false; > + struct dwc3 *dwc = qcom->dwc; > > return dwc->xhci; > } > > static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, int port_index) > { > - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > struct usb_device *udev; > struct usb_hcd __maybe_unused *hcd; > + struct dwc3 *dwc = qcom->dwc; > > /* > * FIXME: Fix this layering violation. > @@ -497,7 +492,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) > static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data) > { > struct dwc3_qcom *qcom = data; > - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > + struct dwc3 *dwc = qcom->dwc; > > /* If pm_suspended then let pm_resume take care of resuming h/w */ > if (qcom->pm_suspended) > @@ -699,34 +694,198 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count) > return 0; > } > > -static int dwc3_qcom_of_register_core(struct dwc3_qcom *qcom, struct platform_device *pdev) > +static struct property *dwc3_qcom_legacy_prop_concat(const char *name, > + const void *a, size_t a_len, > + const void *b, size_t b_len) > { > - struct device_node *np = pdev->dev.of_node, *dwc3_np; > - struct device *dev = &pdev->dev; > - int ret; > + size_t len = a_len + b_len; > > - dwc3_np = of_get_compatible_child(np, "snps,dwc3"); > - if (!dwc3_np) { > - dev_err(dev, "failed to find dwc3 core child\n"); > - return -ENODEV; > - } > + struct property *prop __free(kfree) = kzalloc(sizeof(*prop), GFP_KERNEL); > + char *prop_name __free(kfree) = kstrdup(name, GFP_KERNEL); > + void *value __free(kfree) = kzalloc(len, GFP_KERNEL); > + if (!prop || !prop_name || !value) > + return NULL; > > - ret = of_platform_populate(np, NULL, NULL, dev); > - if (ret) { > - dev_err(dev, "failed to register dwc3 core - %d\n", ret); > - goto node_put; > + prop->name = no_free_ptr(prop_name); > + prop->value = no_free_ptr(value); > + prop->length = len; We're trying to make struct property opaque or even internal to DT core. Exposing it leaks pointers and then we can't ever free things. This is not helping. The changeset API avoids mucking with struct property. > + > + memcpy(prop->value, a, a_len); > + memcpy(prop->value + a_len, b, b_len); > + > + return_ptr(prop); > +} > + > +/* Replace reg property with base address from dwc3 node and fixed length */ > +static int dwc3_qcom_legacy_update_reg(struct device_node *qcom, > + struct device_node *dwc3) > +{ > + int addr_cells; > + int size_cells; > + u64 dwc3_addr; > + int ret; > + > + ret = of_property_read_reg(dwc3, 0, &dwc3_addr, NULL); > + if (ret < 0) > + return ret; > + > + addr_cells = of_n_addr_cells(qcom); > + size_cells = of_n_addr_cells(qcom); > + > + __be32 *reg __free(kfree) = kcalloc(addr_cells + size_cells, sizeof(__be32), GFP_KERNEL); > + if (!reg) > + return -ENOMEM; > + > + reg[addr_cells - 1] = cpu_to_be32(dwc3_addr); Assuming dwc3_addr fits in 32-bit or that the upper 32-bits matches? > + reg[addr_cells + size_cells - 1] = cpu_to_be32(SDM845_QSCRATCH_BASE_OFFSET + SDM845_QSCRATCH_SIZE); > + > + struct property *prop __free(kfree) = kzalloc(sizeof(*prop), GFP_KERNEL); > + char *name __free(kfree) = kstrdup("reg", GFP_KERNEL); > + if (!prop || !name) > + return -ENOMEM; > + > + prop->name = no_free_ptr(name); > + prop->value = no_free_ptr(reg); > + prop->length = (addr_cells + size_cells) * sizeof(__be32); > + > + return of_update_property(qcom, no_free_ptr(prop)); > +} > + > +/* Prepend dwc_usb3 interrupt to relevant interrupt properties */ > +static int dwc3_qcom_legacy_convert_interrupts(struct device_node *qcom, > + struct property *dwc3_irq) > +{ > + const __be32 *interrupts; > + struct property *new; > + const void *names; > + int interrupts_len; > + int names_len; > + int ret; > + > + if ((interrupts = of_get_property(qcom, "interrupts-extended", &interrupts_len)) != NULL) { > + struct device_node *parent __free(device_node) = of_irq_find_parent(qcom); > + if (!parent) > + return -EINVAL; > + > + __be32 *value __free(kfree) = kzalloc(sizeof(__be32) + dwc3_irq->length, GFP_KERNEL); > + if (!value) > + return -ENOMEM; > + value[0] = cpu_to_be32(parent->phandle); > + memcpy(&value[1], dwc3_irq->value, dwc3_irq->length); > + > + new = dwc3_qcom_legacy_prop_concat("interrupts-extended", > + value, sizeof(__be32) + dwc3_irq->length, > + interrupts, interrupts_len); > + } else if ((interrupts = of_get_property(qcom, "interrupts", &interrupts_len)) != NULL) { > + new = dwc3_qcom_legacy_prop_concat("interrupts", > + dwc3_irq->value, dwc3_irq->length, > + interrupts, interrupts_len); > + } else { > + return -EINVAL; > } > > - qcom->dwc3 = of_find_device_by_node(dwc3_np); > - if (!qcom->dwc3) { > - ret = -ENODEV; > - dev_err(dev, "failed to get dwc3 platform device\n"); > - of_platform_depopulate(dev); > + if (!new) > + return -ENOMEM; > + > + ret = of_update_property(qcom, new); > + if (ret < 0) > + return ret; > + > + names = of_get_property(qcom, "interrupt-names", &names_len); > + if (!names) > + return -EINVAL; > + > + new = dwc3_qcom_legacy_prop_concat("interrupt-names", > + "dwc_usb3", strlen("dwc_usb3") + 1, > + names, names_len); > + if (!new) > + return -ENOMEM; > + > + return of_update_property(qcom, new); > +} > + > +/* Copy property to qcom node */ > +static int dwc3_qcom_legacy_migrate_prop(struct device_node *qcom, > + struct property *prop) > +{ > + struct property *new __free(kfree) = kzalloc(sizeof(*new), GFP_KERNEL); > + char *name __free(kfree) = kstrdup(prop->name, GFP_KERNEL); > + void *value __free(kfree) = kmemdup(prop->value, prop->length, GFP_KERNEL); > + > + if (!new || !name || !value) > + return -ENOMEM; > + > + new->name = no_free_ptr(name); > + new->value = no_free_ptr(value); > + new->length = prop->length; > + > + return of_update_property(qcom, no_free_ptr(new)); > +} > + > +/* Move a child node, with it's properties and children, from dwc3 to qcom node */ > +static int dwc3_qcom_legacy_migrate_child(struct device_node *qcom, > + struct device_node *dwc3, > + const char *name) > +{ > + struct device_node *child; > + > + child = of_get_child_by_name(dwc3, name); > + if (!child) > + return 0; > + > + of_detach_node(child); > + child->parent = qcom; > + of_attach_node(child); > + of_node_put(child); > + > + return 0; > +} > + > +/* Convert dev's DeviceTree representation from qcom,dwc3 to qcom,snps-dwc3 binding */ > +static int dwc3_qcom_convert_legacy_dt(struct device *dev) > +{ > + struct device_node *qcom = dev->of_node; > + struct device_node *dwc3; > + struct property *prop; > + int ret = 0; > + > + dwc3 = of_get_compatible_child(qcom, "snps,dwc3"); > + if (!dwc3) > + return 0; > + > + /* We have a child node, but no support for dynamic OF */ > + if (!IS_ENABLED(CONFIG_OF_DYNAMIC)) > + return -EINVAL; > + > + for_each_property_of_node(dwc3, prop) { > + if (!strcmp(prop->name, "compatible")) > + ; > + else if (!strcmp(prop->name, "reg")) > + ret = dwc3_qcom_legacy_update_reg(qcom, dwc3); > + else if (!strcmp(prop->name, "interrupts")) > + ret = dwc3_qcom_legacy_convert_interrupts(qcom, prop); > + else > + ret = dwc3_qcom_legacy_migrate_prop(qcom, prop); > } > > -node_put: > - of_node_put(dwc3_np); > + if (ret < 0) > + goto err_node_put; > + > + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "port"); > + if (ret) > + goto err_node_put; > + > + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "ports"); > + if (ret) > + goto err_node_put; > + > + of_detach_node(dwc3); > + of_node_put(dwc3); > > + return 0; > + > +err_node_put: > + of_node_put(dwc3); > return ret; > } > > @@ -735,16 +894,21 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > struct device_node *np = pdev->dev.of_node; > struct device *dev = &pdev->dev; > struct dwc3_qcom *qcom; > - struct resource *res; > + struct resource res; > int ret, i; > bool ignore_pipe_clk; > bool wakeup_source; > > + ret = dwc3_qcom_convert_legacy_dt(dev); > + if (ret) { > + dev_err(&pdev->dev, "failed to convert legacy OF node\n"); > + return ret; > + } > + > qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); > if (!qcom) > return -ENOMEM; > > - platform_set_drvdata(pdev, qcom); > qcom->dev = &pdev->dev; > > qcom->resets = devm_reset_control_array_get_optional_exclusive(dev); > @@ -773,10 +937,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > goto reset_assert; > } > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + ret = of_address_to_resource(np, 0, &res); So we just leave the platform device resources stale? The right solution is probably to make platform_get_resource() work on demand. That's what we did for IRQ resources years ago (since those had irqchip driver dependencies). Rob
On Mon, Aug 12, 2024 at 03:21:39PM -0600, Rob Herring wrote: > On Sun, Aug 11, 2024 at 08:12:03PM -0700, Bjorn Andersson wrote: > > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > > > The USB IP-block found in most Qualcomm platforms is modelled in the > > Linux kernel as 3 different independent device drivers, but as shown by > > the already existing layering violations in the Qualcomm glue driver > > they can not be operated independently. > > > > With the current implementation, the glue driver registers the core and > > has no way to know when this is done. As a result, e.g. the suspend > > callbacks needs to guard against NULL pointer dereferences when trying > > to peek into the struct dwc3 found in the drvdata of the child. > > Even with these checks, there are no way to fully protect ourselves from > > the race conditions that occur if the DWC3 is unbound. > > > > Missing from the upstream Qualcomm USB support is handling of role > > switching, in which the glue needs to be notified upon DRD mode changes. > > Several attempts has been made through the years to register callbacks > > etc, but they always fall short when it comes to handling of the core's > > probe deferral on resources etc. > > > > Moving to a model where the DWC3 core is instantiated in a synchronous > > fashion avoids above described race conditions. > > > > It is however not feasible to do so without also flattening the > > DeviceTree binding, as assumptions are made in the DWC3 core and > > frameworks used that the device's associated of_node will the that of > > the core. Furthermore, the DeviceTree binding is a direct > > representation of the Linux driver model, and doesn't necessarily > > describe "the USB IP-block". > > > > The Qualcomm DWC3 glue driver is therefor transitioned to initialize and > > operate the DWC3 within the one device context, in synchronous fashion. > > > > To handle backwards compatibility, and to remove the two-device model, > > of_nodes of the old compatible are converted to the new one, early > > during probe. > > > > This happens in the event that a DWC3 core child node is present, the > > content of the reg and interrupt properties of this node are merged into > > the shared properties, all remaining properties are copied and the core > > node is dropped. Effectively transitioning the node from qcom,dwc3 to > > qcom,snps-dwc3. > > In the past we've done this old binding to new binding with an overlay > embedded in the kernel. The overlay would just be the .dts change you've > made (we should have a tool that takes 2 DTs and generates an overlay as > the diff). I suppose it's a few platforms here, but then it is just data > and easily deleted when no longer needed (I think the cases I'm > remembering did just that because they are gone now. It was TI display > and Renesas media stuff IIRC). > Where and how do you apply this overlay? If I can avoid doing the dynamic translation, I'd be happy to do so. > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > --- > > drivers/usb/dwc3/dwc3-qcom.c | 310 +++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 251 insertions(+), 59 deletions(-) > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c [..] > > -static int dwc3_qcom_of_register_core(struct dwc3_qcom *qcom, struct platform_device *pdev) > > +static struct property *dwc3_qcom_legacy_prop_concat(const char *name, > > + const void *a, size_t a_len, > > + const void *b, size_t b_len) > > { > > - struct device_node *np = pdev->dev.of_node, *dwc3_np; > > - struct device *dev = &pdev->dev; > > - int ret; > > + size_t len = a_len + b_len; > > > > - dwc3_np = of_get_compatible_child(np, "snps,dwc3"); > > - if (!dwc3_np) { > > - dev_err(dev, "failed to find dwc3 core child\n"); > > - return -ENODEV; > > - } > > + struct property *prop __free(kfree) = kzalloc(sizeof(*prop), GFP_KERNEL); > > + char *prop_name __free(kfree) = kstrdup(name, GFP_KERNEL); > > + void *value __free(kfree) = kzalloc(len, GFP_KERNEL); > > + if (!prop || !prop_name || !value) > > + return NULL; > > > > - ret = of_platform_populate(np, NULL, NULL, dev); > > - if (ret) { > > - dev_err(dev, "failed to register dwc3 core - %d\n", ret); > > - goto node_put; > > + prop->name = no_free_ptr(prop_name); > > + prop->value = no_free_ptr(value); > > + prop->length = len; > > We're trying to make struct property opaque or even internal to DT core. > Exposing it leaks pointers and then we can't ever free things. This is > not helping. The changeset API avoids mucking with struct property. > I will review the changeset API! > > + > > + memcpy(prop->value, a, a_len); > > + memcpy(prop->value + a_len, b, b_len); > > + > > + return_ptr(prop); > > +} > > + > > +/* Replace reg property with base address from dwc3 node and fixed length */ > > +static int dwc3_qcom_legacy_update_reg(struct device_node *qcom, > > + struct device_node *dwc3) > > +{ > > + int addr_cells; > > + int size_cells; > > + u64 dwc3_addr; > > + int ret; > > + > > + ret = of_property_read_reg(dwc3, 0, &dwc3_addr, NULL); > > + if (ret < 0) > > + return ret; > > + > > + addr_cells = of_n_addr_cells(qcom); > > + size_cells = of_n_addr_cells(qcom); > > + > > + __be32 *reg __free(kfree) = kcalloc(addr_cells + size_cells, sizeof(__be32), GFP_KERNEL); > > + if (!reg) > > + return -ENOMEM; > > + > > + reg[addr_cells - 1] = cpu_to_be32(dwc3_addr); > > Assuming dwc3_addr fits in 32-bit or that the upper 32-bits matches? > The core resides in the lower 32 bits on all existing targets, and I expect any new targets that possibly changes that assumption would not take the path through this translation (or would need to correct my assumption). > > + reg[addr_cells + size_cells - 1] = cpu_to_be32(SDM845_QSCRATCH_BASE_OFFSET + SDM845_QSCRATCH_SIZE); > > + [..] > > @@ -773,10 +937,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > goto reset_assert; > > } > > > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + ret = of_address_to_resource(np, 0, &res); > > So we just leave the platform device resources stale? The right solution > is probably to make platform_get_resource() work on demand. I did consider updating the resource, only in the case that we rewrite the information, as it would be slightly cleaner not to leave that dangling. But this was cleaner code wise. > That's what > we did for IRQ resources years ago (since those had irqchip driver > dependencies). > Right, for platform_get_irq(), but I presume for platform_get_resource() we would end up with the union of the different resource-specific lookup mechanisms? Regards, Bjorn > Rob >
On Mon, Aug 12, 2024 at 4:16 PM Bjorn Andersson <quic_bjorande@quicinc.com> wrote: > > On Mon, Aug 12, 2024 at 03:21:39PM -0600, Rob Herring wrote: > > On Sun, Aug 11, 2024 at 08:12:03PM -0700, Bjorn Andersson wrote: > > > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > > > > > The USB IP-block found in most Qualcomm platforms is modelled in the > > > Linux kernel as 3 different independent device drivers, but as shown by > > > the already existing layering violations in the Qualcomm glue driver > > > they can not be operated independently. > > > > > > With the current implementation, the glue driver registers the core and > > > has no way to know when this is done. As a result, e.g. the suspend > > > callbacks needs to guard against NULL pointer dereferences when trying > > > to peek into the struct dwc3 found in the drvdata of the child. > > > Even with these checks, there are no way to fully protect ourselves from > > > the race conditions that occur if the DWC3 is unbound. > > > > > > Missing from the upstream Qualcomm USB support is handling of role > > > switching, in which the glue needs to be notified upon DRD mode changes. > > > Several attempts has been made through the years to register callbacks > > > etc, but they always fall short when it comes to handling of the core's > > > probe deferral on resources etc. > > > > > > Moving to a model where the DWC3 core is instantiated in a synchronous > > > fashion avoids above described race conditions. > > > > > > It is however not feasible to do so without also flattening the > > > DeviceTree binding, as assumptions are made in the DWC3 core and > > > frameworks used that the device's associated of_node will the that of > > > the core. Furthermore, the DeviceTree binding is a direct > > > representation of the Linux driver model, and doesn't necessarily > > > describe "the USB IP-block". > > > > > > The Qualcomm DWC3 glue driver is therefor transitioned to initialize and > > > operate the DWC3 within the one device context, in synchronous fashion. > > > > > > To handle backwards compatibility, and to remove the two-device model, > > > of_nodes of the old compatible are converted to the new one, early > > > during probe. > > > > > > This happens in the event that a DWC3 core child node is present, the > > > content of the reg and interrupt properties of this node are merged into > > > the shared properties, all remaining properties are copied and the core > > > node is dropped. Effectively transitioning the node from qcom,dwc3 to > > > qcom,snps-dwc3. > > > > In the past we've done this old binding to new binding with an overlay > > embedded in the kernel. The overlay would just be the .dts change you've > > made (we should have a tool that takes 2 DTs and generates an overlay as > > the diff). I suppose it's a few platforms here, but then it is just data > > and easily deleted when no longer needed (I think the cases I'm > > remembering did just that because they are gone now. It was TI display > > and Renesas media stuff IIRC). > > > > Where and how do you apply this overlay? With "git log -S of_overlay_ drivers/", I found the prior cases. See 841281fe52a7 ("drm: rcar-du: Drop LVDS device tree backward compatibility") and 739acd85ffdb ("drm/tilcdc: Remove obsolete "ti,tilcdc,slave" dts binding support") I would consider putting them in drivers/of/ as well. Then we could better control the order of applying the overlay and creating devices. It avoids a lot of the complexity if the overlay is applied first. > If I can avoid doing the dynamic translation, I'd be happy to do so. > > > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > > --- > > > drivers/usb/dwc3/dwc3-qcom.c | 310 +++++++++++++++++++++++++++++++++++-------- > > > 1 file changed, 251 insertions(+), 59 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > [..] > > > -static int dwc3_qcom_of_register_core(struct dwc3_qcom *qcom, struct platform_device *pdev) > > > +static struct property *dwc3_qcom_legacy_prop_concat(const char *name, > > > + const void *a, size_t a_len, > > > + const void *b, size_t b_len) > > > { > > > - struct device_node *np = pdev->dev.of_node, *dwc3_np; > > > - struct device *dev = &pdev->dev; > > > - int ret; > > > + size_t len = a_len + b_len; > > > > > > - dwc3_np = of_get_compatible_child(np, "snps,dwc3"); > > > - if (!dwc3_np) { > > > - dev_err(dev, "failed to find dwc3 core child\n"); > > > - return -ENODEV; > > > - } > > > + struct property *prop __free(kfree) = kzalloc(sizeof(*prop), GFP_KERNEL); > > > + char *prop_name __free(kfree) = kstrdup(name, GFP_KERNEL); > > > + void *value __free(kfree) = kzalloc(len, GFP_KERNEL); > > > + if (!prop || !prop_name || !value) > > > + return NULL; > > > > > > - ret = of_platform_populate(np, NULL, NULL, dev); > > > - if (ret) { > > > - dev_err(dev, "failed to register dwc3 core - %d\n", ret); > > > - goto node_put; > > > + prop->name = no_free_ptr(prop_name); > > > + prop->value = no_free_ptr(value); > > > + prop->length = len; > > > > We're trying to make struct property opaque or even internal to DT core. > > Exposing it leaks pointers and then we can't ever free things. This is > > not helping. The changeset API avoids mucking with struct property. > > > > I will review the changeset API! > > > > + > > > + memcpy(prop->value, a, a_len); > > > + memcpy(prop->value + a_len, b, b_len); > > > + > > > + return_ptr(prop); > > > +} > > > + > > > +/* Replace reg property with base address from dwc3 node and fixed length */ > > > +static int dwc3_qcom_legacy_update_reg(struct device_node *qcom, > > > + struct device_node *dwc3) > > > +{ > > > + int addr_cells; > > > + int size_cells; > > > + u64 dwc3_addr; > > > + int ret; > > > + > > > + ret = of_property_read_reg(dwc3, 0, &dwc3_addr, NULL); > > > + if (ret < 0) > > > + return ret; > > > + > > > + addr_cells = of_n_addr_cells(qcom); > > > + size_cells = of_n_addr_cells(qcom); > > > + > > > + __be32 *reg __free(kfree) = kcalloc(addr_cells + size_cells, sizeof(__be32), GFP_KERNEL); > > > + if (!reg) > > > + return -ENOMEM; > > > + > > > + reg[addr_cells - 1] = cpu_to_be32(dwc3_addr); > > > > Assuming dwc3_addr fits in 32-bit or that the upper 32-bits matches? > > > > The core resides in the lower 32 bits on all existing targets, and I > expect any new targets that possibly changes that assumption would not > take the path through this translation (or would need to correct my > assumption). > > > > + reg[addr_cells + size_cells - 1] = cpu_to_be32(SDM845_QSCRATCH_BASE_OFFSET + SDM845_QSCRATCH_SIZE); > > > + > [..] > > > @@ -773,10 +937,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > > goto reset_assert; > > > } > > > > > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + ret = of_address_to_resource(np, 0, &res); > > > > So we just leave the platform device resources stale? The right solution > > is probably to make platform_get_resource() work on demand. > > I did consider updating the resource, only in the case that we rewrite > the information, as it would be slightly cleaner not to leave that > dangling. But this was cleaner code wise. > > > That's what > > we did for IRQ resources years ago (since those had irqchip driver > > dependencies). > > > > Right, for platform_get_irq(), but I presume for platform_get_resource() > we would end up with the union of the different resource-specific lookup > mechanisms? You'd just make platform_get_resource() call of_address_to_resource() internally if there's a node pointer. There could be drivers that break and it would be slower if we defer probe since it would parse the DT every probe. But really, an overlay applied earlier is a better approach IMO. Rob
On Sun, Aug 11, 2024 at 08:12:03PM -0700, Bjorn Andersson wrote: > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > The USB IP-block found in most Qualcomm platforms is modelled in the > Linux kernel as 3 different independent device drivers, but as shown by > the already existing layering violations in the Qualcomm glue driver > they can not be operated independently. > > With the current implementation, the glue driver registers the core and > has no way to know when this is done. As a result, e.g. the suspend > callbacks needs to guard against NULL pointer dereferences when trying > to peek into the struct dwc3 found in the drvdata of the child. > Even with these checks, there are no way to fully protect ourselves from > the race conditions that occur if the DWC3 is unbound. > > Missing from the upstream Qualcomm USB support is handling of role > switching, in which the glue needs to be notified upon DRD mode changes. > Several attempts has been made through the years to register callbacks > etc, but they always fall short when it comes to handling of the core's > probe deferral on resources etc. > > Moving to a model where the DWC3 core is instantiated in a synchronous > fashion avoids above described race conditions. > > It is however not feasible to do so without also flattening the > DeviceTree binding, as assumptions are made in the DWC3 core and > frameworks used that the device's associated of_node will the that of > the core. Furthermore, the DeviceTree binding is a direct > representation of the Linux driver model, and doesn't necessarily > describe "the USB IP-block". > > The Qualcomm DWC3 glue driver is therefor transitioned to initialize and > operate the DWC3 within the one device context, in synchronous fashion. > > To handle backwards compatibility, and to remove the two-device model, > of_nodes of the old compatible are converted to the new one, early > during probe. > > This happens in the event that a DWC3 core child node is present, the > content of the reg and interrupt properties of this node are merged into > the shared properties, all remaining properties are copied and the core > node is dropped. Effectively transitioning the node from qcom,dwc3 to > qcom,snps-dwc3. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 310 +++++++++++++++++++++++++++++++++++-------- > 1 file changed, 251 insertions(+), 59 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 33de03f2d782..27b5013cd411 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -6,6 +6,7 @@ > > #include <linux/io.h> > #include <linux/of.h> > +#include <linux/cleanup.h> > #include <linux/clk.h> > #include <linux/irq.h> > #include <linux/of_clk.h> > @@ -13,6 +14,8 @@ > #include <linux/kernel.h> > #include <linux/extcon.h> > #include <linux/interconnect.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/phy/phy.h> > @@ -22,6 +25,7 @@ > #include <linux/usb/hcd.h> > #include <linux/usb.h> > #include "core.h" > +#include "glue.h" > > /* USB QSCRATCH Hardware registers */ > #define QSCRATCH_HS_PHY_CTRL 0x10 > @@ -72,7 +76,7 @@ struct dwc3_qcom_port { > struct dwc3_qcom { > struct device *dev; > void __iomem *qscratch_base; > - struct platform_device *dwc3; > + struct dwc3 *dwc; > struct clk **clks; > int num_clocks; > struct reset_control *resets; > @@ -259,7 +263,7 @@ static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom) > goto put_path_ddr; > } > > - max_speed = usb_get_maximum_speed(&qcom->dwc3->dev); > + max_speed = usb_get_maximum_speed(qcom->dwc->dev); > if (max_speed >= USB_SPEED_SUPER || max_speed == USB_SPEED_UNKNOWN) { > ret = icc_set_bw(qcom->icc_path_ddr, > USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW); > @@ -302,25 +306,16 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) > /* Only usable in contexts where the role can not change. */ > static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) > { > - struct dwc3 *dwc; > - > - /* > - * FIXME: Fix this layering violation. > - */ > - dwc = platform_get_drvdata(qcom->dwc3); > - > - /* Core driver may not have probed yet. */ > - if (!dwc) > - return false; > + struct dwc3 *dwc = qcom->dwc; > > return dwc->xhci; dwc only use once. return qcom->dwc->xhci? > } > > static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, int port_index) > { > - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > struct usb_device *udev; > struct usb_hcd __maybe_unused *hcd; > + struct dwc3 *dwc = qcom->dwc; > > /* > * FIXME: Fix this layering violation. > @@ -497,7 +492,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) > static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data) > { > struct dwc3_qcom *qcom = data; > - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > + struct dwc3 *dwc = qcom->dwc; > > /* If pm_suspended then let pm_resume take care of resuming h/w */ > if (qcom->pm_suspended) > @@ -699,34 +694,198 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count) > return 0; > } > > -static int dwc3_qcom_of_register_core(struct dwc3_qcom *qcom, struct platform_device *pdev) > +static struct property *dwc3_qcom_legacy_prop_concat(const char *name, > + const void *a, size_t a_len, > + const void *b, size_t b_len) > { > - struct device_node *np = pdev->dev.of_node, *dwc3_np; > - struct device *dev = &pdev->dev; > - int ret; > + size_t len = a_len + b_len; > > - dwc3_np = of_get_compatible_child(np, "snps,dwc3"); > - if (!dwc3_np) { > - dev_err(dev, "failed to find dwc3 core child\n"); > - return -ENODEV; > - } > + struct property *prop __free(kfree) = kzalloc(sizeof(*prop), GFP_KERNEL); > + char *prop_name __free(kfree) = kstrdup(name, GFP_KERNEL); > + void *value __free(kfree) = kzalloc(len, GFP_KERNEL); > + if (!prop || !prop_name || !value) > + return NULL; > > - ret = of_platform_populate(np, NULL, NULL, dev); > - if (ret) { > - dev_err(dev, "failed to register dwc3 core - %d\n", ret); > - goto node_put; > + prop->name = no_free_ptr(prop_name); > + prop->value = no_free_ptr(value); > + prop->length = len; > + > + memcpy(prop->value, a, a_len); > + memcpy(prop->value + a_len, b, b_len); > + > + return_ptr(prop); > +} > + > +/* Replace reg property with base address from dwc3 node and fixed length */ > +static int dwc3_qcom_legacy_update_reg(struct device_node *qcom, > + struct device_node *dwc3) > +{ > + int addr_cells; > + int size_cells; > + u64 dwc3_addr; > + int ret; > + > + ret = of_property_read_reg(dwc3, 0, &dwc3_addr, NULL); > + if (ret < 0) > + return ret; > + > + addr_cells = of_n_addr_cells(qcom); > + size_cells = of_n_addr_cells(qcom); > + > + __be32 *reg __free(kfree) = kcalloc(addr_cells + size_cells, sizeof(__be32), GFP_KERNEL); > + if (!reg) > + return -ENOMEM; > + > + reg[addr_cells - 1] = cpu_to_be32(dwc3_addr); > + reg[addr_cells + size_cells - 1] = cpu_to_be32(SDM845_QSCRATCH_BASE_OFFSET + SDM845_QSCRATCH_SIZE); > + > + struct property *prop __free(kfree) = kzalloc(sizeof(*prop), GFP_KERNEL); > + char *name __free(kfree) = kstrdup("reg", GFP_KERNEL); > + if (!prop || !name) > + return -ENOMEM; > + > + prop->name = no_free_ptr(name); > + prop->value = no_free_ptr(reg); > + prop->length = (addr_cells + size_cells) * sizeof(__be32); > + > + return of_update_property(qcom, no_free_ptr(prop)); > +} > + > +/* Prepend dwc_usb3 interrupt to relevant interrupt properties */ > +static int dwc3_qcom_legacy_convert_interrupts(struct device_node *qcom, > + struct property *dwc3_irq) > +{ > + const __be32 *interrupts; > + struct property *new; > + const void *names; > + int interrupts_len; > + int names_len; > + int ret; > + > + if ((interrupts = of_get_property(qcom, "interrupts-extended", &interrupts_len)) != NULL) { > + struct device_node *parent __free(device_node) = of_irq_find_parent(qcom); > + if (!parent) > + return -EINVAL; > + > + __be32 *value __free(kfree) = kzalloc(sizeof(__be32) + dwc3_irq->length, GFP_KERNEL); > + if (!value) > + return -ENOMEM; > + value[0] = cpu_to_be32(parent->phandle); > + memcpy(&value[1], dwc3_irq->value, dwc3_irq->length); > + > + new = dwc3_qcom_legacy_prop_concat("interrupts-extended", > + value, sizeof(__be32) + dwc3_irq->length, > + interrupts, interrupts_len); > + } else if ((interrupts = of_get_property(qcom, "interrupts", &interrupts_len)) != NULL) { > + new = dwc3_qcom_legacy_prop_concat("interrupts", > + dwc3_irq->value, dwc3_irq->length, > + interrupts, interrupts_len); > + } else { > + return -EINVAL; > } > > - qcom->dwc3 = of_find_device_by_node(dwc3_np); > - if (!qcom->dwc3) { > - ret = -ENODEV; > - dev_err(dev, "failed to get dwc3 platform device\n"); > - of_platform_depopulate(dev); > + if (!new) > + return -ENOMEM; > + > + ret = of_update_property(qcom, new); > + if (ret < 0) > + return ret; > + > + names = of_get_property(qcom, "interrupt-names", &names_len); > + if (!names) > + return -EINVAL; > + > + new = dwc3_qcom_legacy_prop_concat("interrupt-names", > + "dwc_usb3", strlen("dwc_usb3") + 1, > + names, names_len); > + if (!new) > + return -ENOMEM; > + > + return of_update_property(qcom, new); > +} > + > +/* Copy property to qcom node */ > +static int dwc3_qcom_legacy_migrate_prop(struct device_node *qcom, > + struct property *prop) > +{ > + struct property *new __free(kfree) = kzalloc(sizeof(*new), GFP_KERNEL); > + char *name __free(kfree) = kstrdup(prop->name, GFP_KERNEL); > + void *value __free(kfree) = kmemdup(prop->value, prop->length, GFP_KERNEL); > + > + if (!new || !name || !value) > + return -ENOMEM; > + > + new->name = no_free_ptr(name); > + new->value = no_free_ptr(value); > + new->length = prop->length; > + > + return of_update_property(qcom, no_free_ptr(new)); > +} > + > +/* Move a child node, with it's properties and children, from dwc3 to qcom node */ > +static int dwc3_qcom_legacy_migrate_child(struct device_node *qcom, > + struct device_node *dwc3, > + const char *name) > +{ > + struct device_node *child; > + > + child = of_get_child_by_name(dwc3, name); > + if (!child) > + return 0; > + > + of_detach_node(child); > + child->parent = qcom; > + of_attach_node(child); > + of_node_put(child); > + > + return 0; > +} > + > +/* Convert dev's DeviceTree representation from qcom,dwc3 to qcom,snps-dwc3 binding */ > +static int dwc3_qcom_convert_legacy_dt(struct device *dev) > +{ > + struct device_node *qcom = dev->of_node; > + struct device_node *dwc3; > + struct property *prop; > + int ret = 0; > + > + dwc3 = of_get_compatible_child(qcom, "snps,dwc3"); > + if (!dwc3) > + return 0; > + > + /* We have a child node, but no support for dynamic OF */ > + if (!IS_ENABLED(CONFIG_OF_DYNAMIC)) > + return -EINVAL; > + > + for_each_property_of_node(dwc3, prop) { > + if (!strcmp(prop->name, "compatible")) > + ; > + else if (!strcmp(prop->name, "reg")) > + ret = dwc3_qcom_legacy_update_reg(qcom, dwc3); > + else if (!strcmp(prop->name, "interrupts")) > + ret = dwc3_qcom_legacy_convert_interrupts(qcom, prop); > + else > + ret = dwc3_qcom_legacy_migrate_prop(qcom, prop); > } > > -node_put: > - of_node_put(dwc3_np); > + if (ret < 0) > + goto err_node_put; > + > + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "port"); > + if (ret) > + goto err_node_put; > + > + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "ports"); > + if (ret) > + goto err_node_put; > + > + of_detach_node(dwc3); > + of_node_put(dwc3); > > + return 0; > + > +err_node_put: > + of_node_put(dwc3); > return ret; > } Look like you copy children dwc3's property into current glue node. Can you passdown dwc3's node into dwc3_probe(), let dwc3_probe to handle these, Or move it into dwc3-core. otherwise, if imx want to do the same thing, the the same code will be dupicated. > > @@ -735,16 +894,21 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > struct device_node *np = pdev->dev.of_node; > struct device *dev = &pdev->dev; > struct dwc3_qcom *qcom; > - struct resource *res; > + struct resource res; > int ret, i; > bool ignore_pipe_clk; > bool wakeup_source; > > + ret = dwc3_qcom_convert_legacy_dt(dev); > + if (ret) { > + dev_err(&pdev->dev, "failed to convert legacy OF node\n"); > + return ret; > + } > + > qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); > if (!qcom) > return -ENOMEM; > > - platform_set_drvdata(pdev, qcom); > qcom->dev = &pdev->dev; > > qcom->resets = devm_reset_control_array_get_optional_exclusive(dev); > @@ -773,10 +937,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > goto reset_assert; > } > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + ret = of_address_to_resource(np, 0, &res); > + if (ret < 0) > + goto clk_disable; > + res.end = res.start + SDM845_QSCRATCH_BASE_OFFSET; > > - qcom->qscratch_base = devm_ioremap_resource(dev, res); > + qcom->qscratch_base = devm_ioremap(dev, res.end, SDM845_QSCRATCH_SIZE); > if (IS_ERR(qcom->qscratch_base)) { > + dev_err(dev, "failed to map qscratch region: %pe\n", qcom->qscratch_base); dev_err_probe()? > ret = PTR_ERR(qcom->qscratch_base); > goto clk_disable; > } > @@ -796,17 +964,17 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > if (ignore_pipe_clk) > dwc3_qcom_select_utmi_clk(qcom); > > - ret = dwc3_qcom_of_register_core(qcom, pdev); > - if (ret) { > - dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret); > + qcom->dwc = dwc3_probe(pdev, &res, true, qcom); > + if (IS_ERR(qcom->dwc)) { > + ret = dev_err_probe(dev, PTR_ERR(qcom->dwc), "failed to register DWC3 Core\n"); > goto clk_disable; > } > > ret = dwc3_qcom_interconnect_init(qcom); > if (ret) > - goto depopulate; > + goto remove_core; > > - qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev); > + qcom->mode = usb_get_dr_mode(dev); > > /* enable vbus override for device mode */ > if (qcom->mode != USB_DR_MODE_HOST) > @@ -819,20 +987,15 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source"); > device_init_wakeup(&pdev->dev, wakeup_source); > - device_init_wakeup(&qcom->dwc3->dev, wakeup_source); > > qcom->is_suspended = false; > - pm_runtime_set_active(dev); > - pm_runtime_enable(dev); > - pm_runtime_forbid(dev); > > return 0; > > interconnect_exit: > dwc3_qcom_interconnect_exit(qcom); > -depopulate: > - of_platform_depopulate(&pdev->dev); > - platform_device_put(qcom->dwc3); > +remove_core: > + dwc3_remove(qcom->dwc); > clk_disable: > for (i = qcom->num_clocks - 1; i >= 0; i--) { > clk_disable_unprepare(qcom->clks[i]); > @@ -846,13 +1009,11 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > static void dwc3_qcom_remove(struct platform_device *pdev) > { > - struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > + struct dwc3 *dwc = platform_get_drvdata(pdev); > + struct dwc3_qcom *qcom = dwc->glue; > struct device *dev = &pdev->dev; > int i; > > - of_platform_depopulate(&pdev->dev); > - platform_device_put(qcom->dwc3); > - > for (i = qcom->num_clocks - 1; i >= 0; i--) { > clk_disable_unprepare(qcom->clks[i]); > clk_put(qcom->clks[i]); > @@ -868,10 +1029,15 @@ static void dwc3_qcom_remove(struct platform_device *pdev) > > static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev) > { > - struct dwc3_qcom *qcom = dev_get_drvdata(dev); > + struct dwc3 *dwc = dev_get_drvdata(dev); > + struct dwc3_qcom *qcom = dwc->glue; > bool wakeup = device_may_wakeup(dev); > int ret; > > + ret = dwc3_suspend(qcom->dwc); > + if (ret) > + return ret; > + > ret = dwc3_qcom_suspend(qcom, wakeup); > if (ret) > return ret; > @@ -883,7 +1049,8 @@ static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev) > > static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev) > { > - struct dwc3_qcom *qcom = dev_get_drvdata(dev); > + struct dwc3 *dwc = dev_get_drvdata(dev); > + struct dwc3_qcom *qcom = dwc->glue; > bool wakeup = device_may_wakeup(dev); > int ret; > > @@ -893,31 +1060,56 @@ static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev) > > qcom->pm_suspended = false; > > + ret = dwc3_suspend(qcom->dwc); > + if (ret) > + return ret; > + > return 0; > } > > static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev) > { > - struct dwc3_qcom *qcom = dev_get_drvdata(dev); > + struct dwc3 *dwc = dev_get_drvdata(dev); > + struct dwc3_qcom *qcom = dwc->glue; > + int ret; > + > + ret = dwc3_runtime_suspend(qcom->dwc); > + if (ret) > + return ret; > > return dwc3_qcom_suspend(qcom, true); > } > > +static void __maybe_unused dwc3_qcom_complete(struct device *dev) > +{ > + struct dwc3 *dwc = dev_get_drvdata(dev); > + > + dwc3_complete(dwc); > +} > + > static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev) > { > - struct dwc3_qcom *qcom = dev_get_drvdata(dev); > + struct dwc3 *dwc = dev_get_drvdata(dev); > + struct dwc3_qcom *qcom = dwc->glue; > + int ret; > + > + ret = dwc3_qcom_resume(qcom, true); > + if (ret) > + return ret; > > - return dwc3_qcom_resume(qcom, true); > + return dwc3_runtime_resume(qcom->dwc); > } > > static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = { > SET_SYSTEM_SLEEP_PM_OPS(dwc3_qcom_pm_suspend, dwc3_qcom_pm_resume) > SET_RUNTIME_PM_OPS(dwc3_qcom_runtime_suspend, dwc3_qcom_runtime_resume, > NULL) > + .complete = dwc3_qcom_complete, > }; > > static const struct of_device_id dwc3_qcom_of_match[] = { > { .compatible = "qcom,dwc3" }, > + { .compatible = "qcom,snps-dwc3" }, > { } > }; > MODULE_DEVICE_TABLE(of, dwc3_qcom_of_match); > > -- > 2.45.2 >
On Tue, Aug 13, 2024 at 02:33:56PM -0400, Frank Li wrote: > On Sun, Aug 11, 2024 at 08:12:03PM -0700, Bjorn Andersson wrote: > > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > drivers/usb/dwc3/dwc3-qcom.c | 310 +++++++++++++++++++++++++++++++++++-------- [..] > > @@ -302,25 +306,16 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) > > /* Only usable in contexts where the role can not change. */ > > static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) > > { > > - struct dwc3 *dwc; > > - > > - /* > > - * FIXME: Fix this layering violation. > > - */ > > - dwc = platform_get_drvdata(qcom->dwc3); > > - > > - /* Core driver may not have probed yet. */ > > - if (!dwc) > > - return false; > > + struct dwc3 *dwc = qcom->dwc; > > > > return dwc->xhci; > > dwc only use once. > > return qcom->dwc->xhci? > I like it, thanks for the suggestion. > > } > > [..] > > +/* Convert dev's DeviceTree representation from qcom,dwc3 to qcom,snps-dwc3 binding */ > > +static int dwc3_qcom_convert_legacy_dt(struct device *dev) > > +{ > > + struct device_node *qcom = dev->of_node; > > + struct device_node *dwc3; > > + struct property *prop; > > + int ret = 0; > > + > > + dwc3 = of_get_compatible_child(qcom, "snps,dwc3"); > > + if (!dwc3) > > + return 0; > > + > > + /* We have a child node, but no support for dynamic OF */ > > + if (!IS_ENABLED(CONFIG_OF_DYNAMIC)) > > + return -EINVAL; > > + > > + for_each_property_of_node(dwc3, prop) { > > + if (!strcmp(prop->name, "compatible")) > > + ; > > + else if (!strcmp(prop->name, "reg")) > > + ret = dwc3_qcom_legacy_update_reg(qcom, dwc3); > > + else if (!strcmp(prop->name, "interrupts")) > > + ret = dwc3_qcom_legacy_convert_interrupts(qcom, prop); > > + else > > + ret = dwc3_qcom_legacy_migrate_prop(qcom, prop); > > } > > > > -node_put: > > - of_node_put(dwc3_np); > > + if (ret < 0) > > + goto err_node_put; > > + > > + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "port"); > > + if (ret) > > + goto err_node_put; > > + > > + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "ports"); > > + if (ret) > > + goto err_node_put; > > + > > + of_detach_node(dwc3); > > + of_node_put(dwc3); > > > > + return 0; > > + > > +err_node_put: > > + of_node_put(dwc3); > > return ret; > > } > > Look like you copy children dwc3's property into current glue node. > Can you passdown dwc3's node into dwc3_probe(), let dwc3_probe to handle > these, Or move it into dwc3-core. otherwise, if imx want to do the same > thing, the the same code will be dupicated. > I tried that, as it would have saved me from having to do the dynamic rewrite. But the dwc3 core and host are full of device_property_read*(), phy_get(), platform_get_irq() etc which operates on the dwc->dev. I think it can be done, but this felt like a cleaner outcome, in particular once we transition the DeviceTree source. As you say, there should be a fair amount of room for duplication here, so perhaps we can move that to a "glue.c" and share it? [..] > > @@ -773,10 +937,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > goto reset_assert; > > } > > > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + ret = of_address_to_resource(np, 0, &res); > > + if (ret < 0) > > + goto clk_disable; > > + res.end = res.start + SDM845_QSCRATCH_BASE_OFFSET; > > > > - qcom->qscratch_base = devm_ioremap_resource(dev, res); > > + qcom->qscratch_base = devm_ioremap(dev, res.end, SDM845_QSCRATCH_SIZE); > > if (IS_ERR(qcom->qscratch_base)) { > > + dev_err(dev, "failed to map qscratch region: %pe\n", qcom->qscratch_base); > > dev_err_probe()? > Sounds good. Thank you, Bjorn
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 33de03f2d782..27b5013cd411 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -6,6 +6,7 @@ #include <linux/io.h> #include <linux/of.h> +#include <linux/cleanup.h> #include <linux/clk.h> #include <linux/irq.h> #include <linux/of_clk.h> @@ -13,6 +14,8 @@ #include <linux/kernel.h> #include <linux/extcon.h> #include <linux/interconnect.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/phy/phy.h> @@ -22,6 +25,7 @@ #include <linux/usb/hcd.h> #include <linux/usb.h> #include "core.h" +#include "glue.h" /* USB QSCRATCH Hardware registers */ #define QSCRATCH_HS_PHY_CTRL 0x10 @@ -72,7 +76,7 @@ struct dwc3_qcom_port { struct dwc3_qcom { struct device *dev; void __iomem *qscratch_base; - struct platform_device *dwc3; + struct dwc3 *dwc; struct clk **clks; int num_clocks; struct reset_control *resets; @@ -259,7 +263,7 @@ static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom) goto put_path_ddr; } - max_speed = usb_get_maximum_speed(&qcom->dwc3->dev); + max_speed = usb_get_maximum_speed(qcom->dwc->dev); if (max_speed >= USB_SPEED_SUPER || max_speed == USB_SPEED_UNKNOWN) { ret = icc_set_bw(qcom->icc_path_ddr, USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW); @@ -302,25 +306,16 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) /* Only usable in contexts where the role can not change. */ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) { - struct dwc3 *dwc; - - /* - * FIXME: Fix this layering violation. - */ - dwc = platform_get_drvdata(qcom->dwc3); - - /* Core driver may not have probed yet. */ - if (!dwc) - return false; + struct dwc3 *dwc = qcom->dwc; return dwc->xhci; } static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, int port_index) { - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); struct usb_device *udev; struct usb_hcd __maybe_unused *hcd; + struct dwc3 *dwc = qcom->dwc; /* * FIXME: Fix this layering violation. @@ -497,7 +492,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data) { struct dwc3_qcom *qcom = data; - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); + struct dwc3 *dwc = qcom->dwc; /* If pm_suspended then let pm_resume take care of resuming h/w */ if (qcom->pm_suspended) @@ -699,34 +694,198 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count) return 0; } -static int dwc3_qcom_of_register_core(struct dwc3_qcom *qcom, struct platform_device *pdev) +static struct property *dwc3_qcom_legacy_prop_concat(const char *name, + const void *a, size_t a_len, + const void *b, size_t b_len) { - struct device_node *np = pdev->dev.of_node, *dwc3_np; - struct device *dev = &pdev->dev; - int ret; + size_t len = a_len + b_len; - dwc3_np = of_get_compatible_child(np, "snps,dwc3"); - if (!dwc3_np) { - dev_err(dev, "failed to find dwc3 core child\n"); - return -ENODEV; - } + struct property *prop __free(kfree) = kzalloc(sizeof(*prop), GFP_KERNEL); + char *prop_name __free(kfree) = kstrdup(name, GFP_KERNEL); + void *value __free(kfree) = kzalloc(len, GFP_KERNEL); + if (!prop || !prop_name || !value) + return NULL; - ret = of_platform_populate(np, NULL, NULL, dev); - if (ret) { - dev_err(dev, "failed to register dwc3 core - %d\n", ret); - goto node_put; + prop->name = no_free_ptr(prop_name); + prop->value = no_free_ptr(value); + prop->length = len; + + memcpy(prop->value, a, a_len); + memcpy(prop->value + a_len, b, b_len); + + return_ptr(prop); +} + +/* Replace reg property with base address from dwc3 node and fixed length */ +static int dwc3_qcom_legacy_update_reg(struct device_node *qcom, + struct device_node *dwc3) +{ + int addr_cells; + int size_cells; + u64 dwc3_addr; + int ret; + + ret = of_property_read_reg(dwc3, 0, &dwc3_addr, NULL); + if (ret < 0) + return ret; + + addr_cells = of_n_addr_cells(qcom); + size_cells = of_n_addr_cells(qcom); + + __be32 *reg __free(kfree) = kcalloc(addr_cells + size_cells, sizeof(__be32), GFP_KERNEL); + if (!reg) + return -ENOMEM; + + reg[addr_cells - 1] = cpu_to_be32(dwc3_addr); + reg[addr_cells + size_cells - 1] = cpu_to_be32(SDM845_QSCRATCH_BASE_OFFSET + SDM845_QSCRATCH_SIZE); + + struct property *prop __free(kfree) = kzalloc(sizeof(*prop), GFP_KERNEL); + char *name __free(kfree) = kstrdup("reg", GFP_KERNEL); + if (!prop || !name) + return -ENOMEM; + + prop->name = no_free_ptr(name); + prop->value = no_free_ptr(reg); + prop->length = (addr_cells + size_cells) * sizeof(__be32); + + return of_update_property(qcom, no_free_ptr(prop)); +} + +/* Prepend dwc_usb3 interrupt to relevant interrupt properties */ +static int dwc3_qcom_legacy_convert_interrupts(struct device_node *qcom, + struct property *dwc3_irq) +{ + const __be32 *interrupts; + struct property *new; + const void *names; + int interrupts_len; + int names_len; + int ret; + + if ((interrupts = of_get_property(qcom, "interrupts-extended", &interrupts_len)) != NULL) { + struct device_node *parent __free(device_node) = of_irq_find_parent(qcom); + if (!parent) + return -EINVAL; + + __be32 *value __free(kfree) = kzalloc(sizeof(__be32) + dwc3_irq->length, GFP_KERNEL); + if (!value) + return -ENOMEM; + value[0] = cpu_to_be32(parent->phandle); + memcpy(&value[1], dwc3_irq->value, dwc3_irq->length); + + new = dwc3_qcom_legacy_prop_concat("interrupts-extended", + value, sizeof(__be32) + dwc3_irq->length, + interrupts, interrupts_len); + } else if ((interrupts = of_get_property(qcom, "interrupts", &interrupts_len)) != NULL) { + new = dwc3_qcom_legacy_prop_concat("interrupts", + dwc3_irq->value, dwc3_irq->length, + interrupts, interrupts_len); + } else { + return -EINVAL; } - qcom->dwc3 = of_find_device_by_node(dwc3_np); - if (!qcom->dwc3) { - ret = -ENODEV; - dev_err(dev, "failed to get dwc3 platform device\n"); - of_platform_depopulate(dev); + if (!new) + return -ENOMEM; + + ret = of_update_property(qcom, new); + if (ret < 0) + return ret; + + names = of_get_property(qcom, "interrupt-names", &names_len); + if (!names) + return -EINVAL; + + new = dwc3_qcom_legacy_prop_concat("interrupt-names", + "dwc_usb3", strlen("dwc_usb3") + 1, + names, names_len); + if (!new) + return -ENOMEM; + + return of_update_property(qcom, new); +} + +/* Copy property to qcom node */ +static int dwc3_qcom_legacy_migrate_prop(struct device_node *qcom, + struct property *prop) +{ + struct property *new __free(kfree) = kzalloc(sizeof(*new), GFP_KERNEL); + char *name __free(kfree) = kstrdup(prop->name, GFP_KERNEL); + void *value __free(kfree) = kmemdup(prop->value, prop->length, GFP_KERNEL); + + if (!new || !name || !value) + return -ENOMEM; + + new->name = no_free_ptr(name); + new->value = no_free_ptr(value); + new->length = prop->length; + + return of_update_property(qcom, no_free_ptr(new)); +} + +/* Move a child node, with it's properties and children, from dwc3 to qcom node */ +static int dwc3_qcom_legacy_migrate_child(struct device_node *qcom, + struct device_node *dwc3, + const char *name) +{ + struct device_node *child; + + child = of_get_child_by_name(dwc3, name); + if (!child) + return 0; + + of_detach_node(child); + child->parent = qcom; + of_attach_node(child); + of_node_put(child); + + return 0; +} + +/* Convert dev's DeviceTree representation from qcom,dwc3 to qcom,snps-dwc3 binding */ +static int dwc3_qcom_convert_legacy_dt(struct device *dev) +{ + struct device_node *qcom = dev->of_node; + struct device_node *dwc3; + struct property *prop; + int ret = 0; + + dwc3 = of_get_compatible_child(qcom, "snps,dwc3"); + if (!dwc3) + return 0; + + /* We have a child node, but no support for dynamic OF */ + if (!IS_ENABLED(CONFIG_OF_DYNAMIC)) + return -EINVAL; + + for_each_property_of_node(dwc3, prop) { + if (!strcmp(prop->name, "compatible")) + ; + else if (!strcmp(prop->name, "reg")) + ret = dwc3_qcom_legacy_update_reg(qcom, dwc3); + else if (!strcmp(prop->name, "interrupts")) + ret = dwc3_qcom_legacy_convert_interrupts(qcom, prop); + else + ret = dwc3_qcom_legacy_migrate_prop(qcom, prop); } -node_put: - of_node_put(dwc3_np); + if (ret < 0) + goto err_node_put; + + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "port"); + if (ret) + goto err_node_put; + + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "ports"); + if (ret) + goto err_node_put; + + of_detach_node(dwc3); + of_node_put(dwc3); + return 0; + +err_node_put: + of_node_put(dwc3); return ret; } @@ -735,16 +894,21 @@ static int dwc3_qcom_probe(struct platform_device *pdev) struct device_node *np = pdev->dev.of_node; struct device *dev = &pdev->dev; struct dwc3_qcom *qcom; - struct resource *res; + struct resource res; int ret, i; bool ignore_pipe_clk; bool wakeup_source; + ret = dwc3_qcom_convert_legacy_dt(dev); + if (ret) { + dev_err(&pdev->dev, "failed to convert legacy OF node\n"); + return ret; + } + qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL); if (!qcom) return -ENOMEM; - platform_set_drvdata(pdev, qcom); qcom->dev = &pdev->dev; qcom->resets = devm_reset_control_array_get_optional_exclusive(dev); @@ -773,10 +937,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev) goto reset_assert; } - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ret = of_address_to_resource(np, 0, &res); + if (ret < 0) + goto clk_disable; + res.end = res.start + SDM845_QSCRATCH_BASE_OFFSET; - qcom->qscratch_base = devm_ioremap_resource(dev, res); + qcom->qscratch_base = devm_ioremap(dev, res.end, SDM845_QSCRATCH_SIZE); if (IS_ERR(qcom->qscratch_base)) { + dev_err(dev, "failed to map qscratch region: %pe\n", qcom->qscratch_base); ret = PTR_ERR(qcom->qscratch_base); goto clk_disable; } @@ -796,17 +964,17 @@ static int dwc3_qcom_probe(struct platform_device *pdev) if (ignore_pipe_clk) dwc3_qcom_select_utmi_clk(qcom); - ret = dwc3_qcom_of_register_core(qcom, pdev); - if (ret) { - dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret); + qcom->dwc = dwc3_probe(pdev, &res, true, qcom); + if (IS_ERR(qcom->dwc)) { + ret = dev_err_probe(dev, PTR_ERR(qcom->dwc), "failed to register DWC3 Core\n"); goto clk_disable; } ret = dwc3_qcom_interconnect_init(qcom); if (ret) - goto depopulate; + goto remove_core; - qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev); + qcom->mode = usb_get_dr_mode(dev); /* enable vbus override for device mode */ if (qcom->mode != USB_DR_MODE_HOST) @@ -819,20 +987,15 @@ static int dwc3_qcom_probe(struct platform_device *pdev) wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source"); device_init_wakeup(&pdev->dev, wakeup_source); - device_init_wakeup(&qcom->dwc3->dev, wakeup_source); qcom->is_suspended = false; - pm_runtime_set_active(dev); - pm_runtime_enable(dev); - pm_runtime_forbid(dev); return 0; interconnect_exit: dwc3_qcom_interconnect_exit(qcom); -depopulate: - of_platform_depopulate(&pdev->dev); - platform_device_put(qcom->dwc3); +remove_core: + dwc3_remove(qcom->dwc); clk_disable: for (i = qcom->num_clocks - 1; i >= 0; i--) { clk_disable_unprepare(qcom->clks[i]); @@ -846,13 +1009,11 @@ static int dwc3_qcom_probe(struct platform_device *pdev) static void dwc3_qcom_remove(struct platform_device *pdev) { - struct dwc3_qcom *qcom = platform_get_drvdata(pdev); + struct dwc3 *dwc = platform_get_drvdata(pdev); + struct dwc3_qcom *qcom = dwc->glue; struct device *dev = &pdev->dev; int i; - of_platform_depopulate(&pdev->dev); - platform_device_put(qcom->dwc3); - for (i = qcom->num_clocks - 1; i >= 0; i--) { clk_disable_unprepare(qcom->clks[i]); clk_put(qcom->clks[i]); @@ -868,10 +1029,15 @@ static void dwc3_qcom_remove(struct platform_device *pdev) static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev) { - struct dwc3_qcom *qcom = dev_get_drvdata(dev); + struct dwc3 *dwc = dev_get_drvdata(dev); + struct dwc3_qcom *qcom = dwc->glue; bool wakeup = device_may_wakeup(dev); int ret; + ret = dwc3_suspend(qcom->dwc); + if (ret) + return ret; + ret = dwc3_qcom_suspend(qcom, wakeup); if (ret) return ret; @@ -883,7 +1049,8 @@ static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev) static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev) { - struct dwc3_qcom *qcom = dev_get_drvdata(dev); + struct dwc3 *dwc = dev_get_drvdata(dev); + struct dwc3_qcom *qcom = dwc->glue; bool wakeup = device_may_wakeup(dev); int ret; @@ -893,31 +1060,56 @@ static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev) qcom->pm_suspended = false; + ret = dwc3_suspend(qcom->dwc); + if (ret) + return ret; + return 0; } static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev) { - struct dwc3_qcom *qcom = dev_get_drvdata(dev); + struct dwc3 *dwc = dev_get_drvdata(dev); + struct dwc3_qcom *qcom = dwc->glue; + int ret; + + ret = dwc3_runtime_suspend(qcom->dwc); + if (ret) + return ret; return dwc3_qcom_suspend(qcom, true); } +static void __maybe_unused dwc3_qcom_complete(struct device *dev) +{ + struct dwc3 *dwc = dev_get_drvdata(dev); + + dwc3_complete(dwc); +} + static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev) { - struct dwc3_qcom *qcom = dev_get_drvdata(dev); + struct dwc3 *dwc = dev_get_drvdata(dev); + struct dwc3_qcom *qcom = dwc->glue; + int ret; + + ret = dwc3_qcom_resume(qcom, true); + if (ret) + return ret; - return dwc3_qcom_resume(qcom, true); + return dwc3_runtime_resume(qcom->dwc); } static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(dwc3_qcom_pm_suspend, dwc3_qcom_pm_resume) SET_RUNTIME_PM_OPS(dwc3_qcom_runtime_suspend, dwc3_qcom_runtime_resume, NULL) + .complete = dwc3_qcom_complete, }; static const struct of_device_id dwc3_qcom_of_match[] = { { .compatible = "qcom,dwc3" }, + { .compatible = "qcom,snps-dwc3" }, { } }; MODULE_DEVICE_TABLE(of, dwc3_qcom_of_match);