| Message ID | 20231016-dwc3-refactor-v1-3-ab4a84165470@quicinc.com |
|---|---|
| State | New |
| 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 4d0515b235dec789578d135a5db586b25c5870cb] url: https://github.com/intel-lab-lkp/linux/commits/Bjorn-Andersson/dt-bindings-usb-qcom-dwc3-Add-qcom-sc8180x-dwc3/20231017-160323 base: 4d0515b235dec789578d135a5db586b25c5870cb patch link: https://lore.kernel.org/r/20231016-dwc3-refactor-v1-3-ab4a84165470%40quicinc.com patch subject: [PATCH 03/12] usb: dwc3: qcom: Merge resources from urs_usb device config: csky-randconfig-002-20231020 (https://download.01.org/0day-ci/archive/20231020/202310201318.RPa2yUmS-lkp@intel.com/config) compiler: csky-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231020/202310201318.RPa2yUmS-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/202310201318.RPa2yUmS-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/usb/dwc3/dwc3-qcom.c: In function 'dwc3_qcom_acpi_merge_urs_resources': >> drivers/usb/dwc3/dwc3-qcom.c:795:17: error: implicit declaration of function 'acpi_dev_get_resources'; did you mean 'acpi_get_event_resources'? [-Werror=implicit-function-declaration] 795 | count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); | ^~~~~~~~~~~~~~~~~~~~~~ | acpi_get_event_resources >> drivers/usb/dwc3/dwc3-qcom.c:803:17: error: implicit declaration of function 'acpi_dev_free_resource_list' [-Werror=implicit-function-declaration] 803 | acpi_dev_free_resource_list(&resource_list); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +795 drivers/usb/dwc3/dwc3-qcom.c 763 764 static int dwc3_qcom_acpi_merge_urs_resources(struct platform_device *pdev) 765 { 766 struct device *dev = &pdev->dev; 767 struct list_head resource_list; 768 struct resource_entry *rentry; 769 struct resource *resources; 770 struct fwnode_handle *fwh; 771 struct acpi_device *adev; 772 char name[8]; 773 int count; 774 int ret; 775 int id; 776 int i; 777 778 /* Figure out device id */ 779 ret = sscanf(fwnode_get_name(dev->fwnode), "URS%d", &id); 780 if (!ret) 781 return -EINVAL; 782 783 /* Find the child using name */ 784 snprintf(name, sizeof(name), "USB%d", id); 785 fwh = fwnode_get_named_child_node(dev->fwnode, name); 786 if (!fwh) 787 return 0; 788 789 adev = to_acpi_device_node(fwh); 790 if (!adev) 791 return -EINVAL; 792 793 INIT_LIST_HEAD(&resource_list); 794 > 795 count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); 796 if (count <= 0) 797 return count; 798 799 count += pdev->num_resources; 800 801 resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); 802 if (!resources) { > 803 acpi_dev_free_resource_list(&resource_list); 804 return -ENOMEM; 805 } 806 807 memcpy(resources, pdev->resource, sizeof(struct resource) * pdev->num_resources); 808 count = pdev->num_resources; 809 list_for_each_entry(rentry, &resource_list, node) { 810 /* Avoid inserting duplicate entries, in case this is called more than once */ 811 for (i = 0; i < count; i++) { 812 if (resource_type(&resources[i]) == resource_type(rentry->res) && 813 resources[i].start == rentry->res->start && 814 resources[i].end == rentry->res->end) 815 break; 816 } 817 818 if (i == count) 819 resources[count++] = *rentry->res; 820 } 821 822 ret = platform_device_add_resources(pdev, resources, count); 823 if (ret) 824 dev_err(&pdev->dev, "failed to add resources\n"); 825 826 acpi_dev_free_resource_list(&resource_list); 827 kfree(resources); 828 829 return ret; 830 } 831
On Mon, Oct 16, 2023 at 08:11:11PM -0700, Bjorn Andersson wrote: > With some ACPI DSDT tables, such as the one found in SC8180X devices, > the USB resources are split between the URSn and it's child USBn device > nodes, in particular the interrupts are placed in the child nodes. > > The solution that was chosen for handling this is to allocate a > platform_device from the child node and selectively pick interrupts > from the main platform_device, or from this created child device, when > creating the platform_device for the DWC3 core. > > This does however not work with the upcoming change where the DWC3 core > is instantiated from the same platform_device as the glue, as the DRD > and host code will attempt to resolve their interrupts from the shared > device, and not the child device. > > Work around this by merging the resources of the child device into the > glue device node, to present a single platform_device with all the > resources necessary. Nice approach. An alternative would be to drop ACPI support completely as Konrad suggested. Should simplify both this series and the multiport one. Is anyone really using the ACPI support here anymore? > -static struct platform_device * > -dwc3_qcom_create_urs_usb_platdev(struct device *dev) > +static int dwc3_qcom_acpi_merge_urs_resources(struct platform_device *pdev) > { > + struct device *dev = &pdev->dev; > + struct list_head resource_list; > + struct resource_entry *rentry; > + struct resource *resources; > struct fwnode_handle *fwh; > struct acpi_device *adev; > char name[8]; > + int count; > int ret; > int id; > + int i; > > /* Figure out device id */ > ret = sscanf(fwnode_get_name(dev->fwnode), "URS%d", &id); > if (!ret) > - return NULL; > + return -EINVAL; > > /* Find the child using name */ > snprintf(name, sizeof(name), "USB%d", id); > fwh = fwnode_get_named_child_node(dev->fwnode, name); > if (!fwh) > - return NULL; > + return 0; > > adev = to_acpi_device_node(fwh); > if (!adev) > - return NULL; > + return -EINVAL; This is currently leaking a reference to the fwnode, I fixed that up here: https://lore.kernel.org/linux-usb/20231117173650.21161-4-johan+linaro@kernel.org/ > + INIT_LIST_HEAD(&resource_list); > + > + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); > + if (count <= 0) > + return count; > + > + count += pdev->num_resources; > + > + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); > + if (!resources) { > + acpi_dev_free_resource_list(&resource_list); > + return -ENOMEM; > + } > + > + memcpy(resources, pdev->resource, sizeof(struct resource) * pdev->num_resources); > + count = pdev->num_resources; > + list_for_each_entry(rentry, &resource_list, node) { > + /* Avoid inserting duplicate entries, in case this is called more than once */ Either shorten this one or make it a multiline comment to stay within 80 chars. > + for (i = 0; i < count; i++) { Should this not be pdev->num_resources? > + if (resource_type(&resources[i]) == resource_type(rentry->res) && > + resources[i].start == rentry->res->start && > + resources[i].end == rentry->res->end) > + break; > + } > + > + if (i == count) Same here. > + resources[count++] = *rentry->res; > + } > > - return acpi_create_platform_device(adev, NULL); > + ret = platform_device_add_resources(pdev, resources, count); > + if (ret) > + dev_err(&pdev->dev, "failed to add resources\n"); > + > + acpi_dev_free_resource_list(&resource_list); > + kfree(resources); > + > + return ret; > } > > static int dwc3_qcom_probe(struct platform_device *pdev) > @@ -817,6 +853,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > dev_err(&pdev->dev, "no supporting ACPI device data\n"); > return -EINVAL; > } > + > + if (qcom->acpi_pdata->is_urs) { > + ret = dwc3_qcom_acpi_merge_urs_resources(pdev); > + if (ret < 0) > + goto clk_disable; The clocks have not been enabled here, just return ret. > + } > } > > qcom->resets = devm_reset_control_array_get_optional_exclusive(dev); > @@ -857,18 +899,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > qcom->acpi_pdata->qscratch_base_offset; > parent_res->end = parent_res->start + > qcom->acpi_pdata->qscratch_base_size; > - > - if (qcom->acpi_pdata->is_urs) { > - qcom->urs_usb = dwc3_qcom_create_urs_usb_platdev(dev); > - if (IS_ERR_OR_NULL(qcom->urs_usb)) { > - dev_err(dev, "failed to create URS USB platdev\n"); > - if (!qcom->urs_usb) > - ret = -ENODEV; > - else > - ret = PTR_ERR(qcom->urs_usb); > - goto clk_disable; > - } > - } > } > > qcom->qscratch_base = devm_ioremap_resource(dev, parent_res); Johan
On Wed, Nov 22, 2023 at 11:24:07AM +0100, Johan Hovold wrote: > On Mon, Oct 16, 2023 at 08:11:11PM -0700, Bjorn Andersson wrote: > > With some ACPI DSDT tables, such as the one found in SC8180X devices, > > the USB resources are split between the URSn and it's child USBn device > > nodes, in particular the interrupts are placed in the child nodes. > > > > The solution that was chosen for handling this is to allocate a > > platform_device from the child node and selectively pick interrupts > > from the main platform_device, or from this created child device, when > > creating the platform_device for the DWC3 core. > > > > This does however not work with the upcoming change where the DWC3 core > > is instantiated from the same platform_device as the glue, as the DRD > > and host code will attempt to resolve their interrupts from the shared > > device, and not the child device. > > > > Work around this by merging the resources of the child device into the > > glue device node, to present a single platform_device with all the > > resources necessary. > > Nice approach. > > An alternative would be to drop ACPI support completely as Konrad > suggested. Should simplify both this series and the multiport one. > > Is anyone really using the ACPI support here anymore? > At the introduction of SC8180X and the Lenovo Flex 5G we where able to run the Debian installer off the ACPI support in the kernel. Since then, at least the UFS support has regressed to the point that this would no longer be possible - without anyone noticing. I would like to see ACPI supported again in the future, but I can't really argue for its existence currently. In the end the new flattened code path is mostly shared with the ACPI path, so perhaps it makes sense to drop the support after this refactor, perhaps not. I will re-evaluate this. > > -static struct platform_device * > > -dwc3_qcom_create_urs_usb_platdev(struct device *dev) > > +static int dwc3_qcom_acpi_merge_urs_resources(struct platform_device *pdev) > > { > > + struct device *dev = &pdev->dev; > > + struct list_head resource_list; > > + struct resource_entry *rentry; > > + struct resource *resources; > > struct fwnode_handle *fwh; > > struct acpi_device *adev; > > char name[8]; > > + int count; > > int ret; > > int id; > > + int i; > > > > /* Figure out device id */ > > ret = sscanf(fwnode_get_name(dev->fwnode), "URS%d", &id); > > if (!ret) > > - return NULL; > > + return -EINVAL; > > > > /* Find the child using name */ > > snprintf(name, sizeof(name), "USB%d", id); > > fwh = fwnode_get_named_child_node(dev->fwnode, name); > > if (!fwh) > > - return NULL; > > + return 0; > > > > adev = to_acpi_device_node(fwh); > > if (!adev) > > - return NULL; > > + return -EINVAL; > > This is currently leaking a reference to the fwnode, I fixed that up > here: > > https://lore.kernel.org/linux-usb/20231117173650.21161-4-johan+linaro@kernel.org/ > > > + INIT_LIST_HEAD(&resource_list); > > + > > + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); > > + if (count <= 0) > > + return count; > > + > > + count += pdev->num_resources; > > + > > + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); > > + if (!resources) { > > + acpi_dev_free_resource_list(&resource_list); > > + return -ENOMEM; > > + } > > + > > + memcpy(resources, pdev->resource, sizeof(struct resource) * pdev->num_resources); > > + count = pdev->num_resources; > > + list_for_each_entry(rentry, &resource_list, node) { > > + /* Avoid inserting duplicate entries, in case this is called more than once */ > > Either shorten this one or make it a multiline comment to stay within 80 > chars. > > > + for (i = 0; i < count; i++) { > > Should this not be pdev->num_resources? > count is first used to denote the number of entries to allocate in the new list, it's then reset to pdev->num_resources 3 lines above this and after this list_for_each_entry() it would be the total number of resources in the new list (which could be less than the allocated number of items). I can avoid reusing the variable, to clarify this - if I choose to keep the ACPI support through the series. > > + if (resource_type(&resources[i]) == resource_type(rentry->res) && > > + resources[i].start == rentry->res->start && > > + resources[i].end == rentry->res->end) > > + break; > > + } > > + > > + if (i == count) > > Same here. > > > + resources[count++] = *rentry->res; > > + } > > > > - return acpi_create_platform_device(adev, NULL); > > + ret = platform_device_add_resources(pdev, resources, count); > > + if (ret) > > + dev_err(&pdev->dev, "failed to add resources\n"); > > + > > + acpi_dev_free_resource_list(&resource_list); > > + kfree(resources); > > + > > + return ret; > > } > > > > static int dwc3_qcom_probe(struct platform_device *pdev) > > @@ -817,6 +853,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > dev_err(&pdev->dev, "no supporting ACPI device data\n"); > > return -EINVAL; > > } > > + > > + if (qcom->acpi_pdata->is_urs) { > > + ret = dwc3_qcom_acpi_merge_urs_resources(pdev); > > + if (ret < 0) > > + goto clk_disable; > > The clocks have not been enabled here, just return ret. > Right. Thanks, Bjorn > > + } > > } > > > > qcom->resets = devm_reset_control_array_get_optional_exclusive(dev); > > @@ -857,18 +899,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > qcom->acpi_pdata->qscratch_base_offset; > > parent_res->end = parent_res->start + > > qcom->acpi_pdata->qscratch_base_size; > > - > > - if (qcom->acpi_pdata->is_urs) { > > - qcom->urs_usb = dwc3_qcom_create_urs_usb_platdev(dev); > > - if (IS_ERR_OR_NULL(qcom->urs_usb)) { > > - dev_err(dev, "failed to create URS USB platdev\n"); > > - if (!qcom->urs_usb) > > - ret = -ENODEV; > > - else > > - ret = PTR_ERR(qcom->urs_usb); > > - goto clk_disable; > > - } > > - } > > } > > > > qcom->qscratch_base = devm_ioremap_resource(dev, parent_res); > > Johan
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index a31c3bc1f56e..7c810712d246 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -68,7 +68,6 @@ struct dwc3_qcom { struct device *dev; void __iomem *qscratch_base; struct platform_device *dwc_dev; - struct platform_device *urs_usb; struct clk **clks; int num_clocks; struct reset_control *resets; @@ -522,15 +521,13 @@ static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom) static int dwc3_qcom_get_irq(struct platform_device *pdev, const char *name, int num) { - struct dwc3_qcom *qcom = platform_get_drvdata(pdev); - struct platform_device *pdev_irq = qcom->urs_usb ? qcom->urs_usb : pdev; struct device_node *np = pdev->dev.of_node; int ret; if (np) - ret = platform_get_irq_byname_optional(pdev_irq, name); + ret = platform_get_irq_byname_optional(pdev, name); else - ret = platform_get_irq_optional(pdev_irq, num); + ret = platform_get_irq_optional(pdev, num); return ret; } @@ -667,8 +664,6 @@ static int dwc3_qcom_acpi_register_core(struct platform_device *pdev) struct dwc3_qcom *qcom = platform_get_drvdata(pdev); struct device *dev = &pdev->dev; struct resource *res, *child_res = NULL; - struct platform_device *pdev_irq = qcom->urs_usb ? qcom->urs_usb : - pdev; int irq; int ret; @@ -700,7 +695,7 @@ static int dwc3_qcom_acpi_register_core(struct platform_device *pdev) child_res[0].end = child_res[0].start + qcom->acpi_pdata->dwc3_core_base_size; - irq = platform_get_irq(pdev_irq, 0); + irq = platform_get_irq(pdev, 0); if (irq < 0) { ret = irq; goto out; @@ -766,31 +761,72 @@ static int dwc3_qcom_of_register_core(struct platform_device *pdev) return ret; } -static struct platform_device * -dwc3_qcom_create_urs_usb_platdev(struct device *dev) +static int dwc3_qcom_acpi_merge_urs_resources(struct platform_device *pdev) { + struct device *dev = &pdev->dev; + struct list_head resource_list; + struct resource_entry *rentry; + struct resource *resources; struct fwnode_handle *fwh; struct acpi_device *adev; char name[8]; + int count; int ret; int id; + int i; /* Figure out device id */ ret = sscanf(fwnode_get_name(dev->fwnode), "URS%d", &id); if (!ret) - return NULL; + return -EINVAL; /* Find the child using name */ snprintf(name, sizeof(name), "USB%d", id); fwh = fwnode_get_named_child_node(dev->fwnode, name); if (!fwh) - return NULL; + return 0; adev = to_acpi_device_node(fwh); if (!adev) - return NULL; + return -EINVAL; + + INIT_LIST_HEAD(&resource_list); + + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); + if (count <= 0) + return count; + + count += pdev->num_resources; + + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); + if (!resources) { + acpi_dev_free_resource_list(&resource_list); + return -ENOMEM; + } + + memcpy(resources, pdev->resource, sizeof(struct resource) * pdev->num_resources); + count = pdev->num_resources; + list_for_each_entry(rentry, &resource_list, node) { + /* Avoid inserting duplicate entries, in case this is called more than once */ + for (i = 0; i < count; i++) { + if (resource_type(&resources[i]) == resource_type(rentry->res) && + resources[i].start == rentry->res->start && + resources[i].end == rentry->res->end) + break; + } + + if (i == count) + resources[count++] = *rentry->res; + } - return acpi_create_platform_device(adev, NULL); + ret = platform_device_add_resources(pdev, resources, count); + if (ret) + dev_err(&pdev->dev, "failed to add resources\n"); + + acpi_dev_free_resource_list(&resource_list); + kfree(resources); + + return ret; } static int dwc3_qcom_probe(struct platform_device *pdev) @@ -817,6 +853,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev) dev_err(&pdev->dev, "no supporting ACPI device data\n"); return -EINVAL; } + + if (qcom->acpi_pdata->is_urs) { + ret = dwc3_qcom_acpi_merge_urs_resources(pdev); + if (ret < 0) + goto clk_disable; + } } qcom->resets = devm_reset_control_array_get_optional_exclusive(dev); @@ -857,18 +899,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev) qcom->acpi_pdata->qscratch_base_offset; parent_res->end = parent_res->start + qcom->acpi_pdata->qscratch_base_size; - - if (qcom->acpi_pdata->is_urs) { - qcom->urs_usb = dwc3_qcom_create_urs_usb_platdev(dev); - if (IS_ERR_OR_NULL(qcom->urs_usb)) { - dev_err(dev, "failed to create URS USB platdev\n"); - if (!qcom->urs_usb) - ret = -ENODEV; - else - ret = PTR_ERR(qcom->urs_usb); - goto clk_disable; - } - } } qcom->qscratch_base = devm_ioremap_resource(dev, parent_res);
With some ACPI DSDT tables, such as the one found in SC8180X devices, the USB resources are split between the URSn and it's child USBn device nodes, in particular the interrupts are placed in the child nodes. The solution that was chosen for handling this is to allocate a platform_device from the child node and selectively pick interrupts from the main platform_device, or from this created child device, when creating the platform_device for the DWC3 core. This does however not work with the upcoming change where the DWC3 core is instantiated from the same platform_device as the glue, as the DRD and host code will attempt to resolve their interrupts from the shared device, and not the child device. Work around this by merging the resources of the child device into the glue device node, to present a single platform_device with all the resources necessary. Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> --- drivers/usb/dwc3/dwc3-qcom.c | 82 ++++++++++++++++++++++++++++++-------------- 1 file changed, 56 insertions(+), 26 deletions(-)