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.
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.
Furhtermore, the DeviceTree binding is a direct representation of the
Linux driver model, and doesn't necessarily describe "the USB IP-block".
This series therefor attempts to flatten the driver split, and operate
the glue and core out of the same platform_device instance. And in order
to do this, the DeviceTree representation of the IP block is flattened.
As a side effect, much of the ACPI integration code is dropped.
Signed-off-by: Bjorn Andersson <[email protected]>
---
Bjorn Andersson (12):
dt-bindings: usb: qcom,dwc3: Add qcom,sc8180x-dwc3
usb: dwc3: qcom: Rename dwc3 platform_device reference
usb: dwc3: qcom: Merge resources from urs_usb device
usb: dwc3: Expose core driver as library
usb: dwc3: Override end of dwc3 memory resource
usb: dwc3: qcom: Add dwc3 core reference in driver state
usb: dwc3: qcom: Instantiate dwc3 core directly
usb: dwc3: qcom: Inline the qscratch constants
dt-bindings: usb: qcom,dwc3: Rename to "glue"
dt-bindings: usb: qcom,dwc3: Introduce flattened qcom,dwc3 binding
usb: dwc3: qcom: Flatten the Qualcomm dwc3 binding and implementation
arm64: dts: qcom: sc8180x: flatten usb_sec node
.../devicetree/bindings/usb/qcom,dwc3-glue.yaml | 626 +++++++++++++++++++++
.../devicetree/bindings/usb/qcom,dwc3.yaml | 321 ++++-------
.../devicetree/bindings/usb/snps,dwc3.yaml | 14 +-
.../arm64/boot/dts/qcom/sc8180x-lenovo-flex-5g.dts | 6 +-
arch/arm64/boot/dts/qcom/sc8180x-primus.dts | 6 +-
arch/arm64/boot/dts/qcom/sc8180x.dtsi | 34 +-
drivers/usb/dwc3/core.c | 136 +++--
drivers/usb/dwc3/core.h | 10 +
drivers/usb/dwc3/dwc3-qcom.c | 328 ++++++-----
9 files changed, 1057 insertions(+), 424 deletions(-)
---
base-commit: 4d0515b235dec789578d135a5db586b25c5870cb
change-id: 20231016-dwc3-refactor-931e3b08a8b9
Best regards,
--
Bjorn Andersson <[email protected]>
In the case that the dwc3 core driver is instantiated from the same
memory resource information as the glue driver, the dwc_res memory
region will overlap with the memory region already mapped by the glue.
As the DWC3 core driver already does math on the passed memory region to
exclude the XHCI region, also adjust the end address, to avoid having to
pass an adjusted region from the glue explicitly.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/usb/dwc3/core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 71e376bebb16..5d86b803fab0 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1908,6 +1908,7 @@ struct dwc3 *dwc3_probe(struct platform_device *pdev)
*/
dwc_res = *res;
dwc_res.start += DWC3_GLOBALS_REGS_START;
+ dwc_res.end = res->start + DWC3_OTG_REGS_END;
if (dev->of_node) {
struct device_node *parent = of_get_parent(dev->of_node);
@@ -1915,6 +1916,7 @@ struct dwc3 *dwc3_probe(struct platform_device *pdev)
if (of_device_is_compatible(parent, "realtek,rtd-dwc3")) {
dwc_res.start -= DWC3_GLOBALS_REGS_START;
dwc_res.start += DWC3_RTK_RTD_GLOBALS_REGS_START;
+ dwc_res.end = dwc_res.start + DWC3_OTG_REGS_END;
}
of_node_put(parent);
--
2.25.1
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 <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 82 ++++++++++++++++++++++++++++++--------------
1 file changed, 56 insertions(+), 26 deletions(-)
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);
--
2.25.1
In the coming changes the Qualcomm DWC3 glue will be able to either
manage the DWC3 core as a child platform_device, or directly instantiate
it within its own context.
Introduce a reference to the dwc3 core state and make the driver
reference the dwc3 core either the child device or this new reference.
As the new member isn't assigned, and qcom->dwc_dev is assigned in all
current cases, the change should have no functional impact.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 100 +++++++++++++++++++++++++++++++++++--------
1 file changed, 83 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 7c810712d246..901e5050363b 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -67,7 +67,8 @@ struct dwc3_acpi_pdata {
struct dwc3_qcom {
struct device *dev;
void __iomem *qscratch_base;
- struct platform_device *dwc_dev;
+ struct platform_device *dwc_dev; /* only used when core is separate device */
+ struct dwc3 *dwc; /* not used when core is separate device */
struct clk **clks;
int num_clocks;
struct reset_control *resets;
@@ -263,7 +264,11 @@ static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
goto put_path_ddr;
}
- max_speed = usb_get_maximum_speed(&qcom->dwc_dev->dev);
+ if (qcom->dwc_dev)
+ max_speed = usb_get_maximum_speed(&qcom->dwc_dev->dev);
+ else
+ max_speed = usb_get_maximum_speed(qcom->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);
@@ -311,7 +316,10 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
/*
* FIXME: Fix this layering violation.
*/
- dwc = platform_get_drvdata(qcom->dwc_dev);
+ if (qcom->dwc_dev)
+ dwc = platform_get_drvdata(qcom->dwc_dev);
+ else
+ dwc = qcom->dwc;
/* Core driver may not have probed yet. */
if (!dwc)
@@ -322,10 +330,14 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
{
- struct dwc3 *dwc = platform_get_drvdata(qcom->dwc_dev);
struct usb_device *udev;
struct usb_hcd __maybe_unused *hcd;
+ struct dwc3 *dwc;
+ if (qcom->dwc_dev)
+ dwc = platform_get_drvdata(qcom->dwc_dev);
+ else
+ dwc = qcom->dwc;
/*
* FIXME: Fix this layering violation.
*/
@@ -485,12 +497,17 @@ 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->dwc_dev);
+ struct dwc3 *dwc;
/* If pm_suspended then let pm_resume take care of resuming h/w */
if (qcom->pm_suspended)
return IRQ_HANDLED;
+ if (qcom->dwc_dev)
+ dwc = platform_get_drvdata(qcom->dwc_dev);
+ else
+ dwc = qcom->dwc;
+
/*
* This is safe as role switching is done from a freezable workqueue
* and the wakeup interrupts are disabled as part of resume.
@@ -936,25 +953,33 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
if (ret)
goto depopulate;
- qcom->mode = usb_get_dr_mode(&qcom->dwc_dev->dev);
+ if (qcom->dwc_dev)
+ qcom->mode = usb_get_dr_mode(&qcom->dwc_dev->dev);
+ else
+ qcom->mode = usb_get_dr_mode(dev);
/* enable vbus override for device mode */
if (qcom->mode != USB_DR_MODE_HOST)
dwc3_qcom_vbus_override_enable(qcom, true);
- /* register extcon to override sw_vbus on Vbus change later */
- ret = dwc3_qcom_register_extcon(qcom);
- if (ret)
- goto interconnect_exit;
+ if (qcom->dwc_dev) {
+ /* register extcon to override sw_vbus on Vbus change later */
+ ret = dwc3_qcom_register_extcon(qcom);
+ if (ret)
+ goto interconnect_exit;
+ }
wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
device_init_wakeup(&pdev->dev, wakeup_source);
- device_init_wakeup(&qcom->dwc_dev->dev, wakeup_source);
+ if (qcom->dwc_dev)
+ device_init_wakeup(&qcom->dwc_dev->dev, wakeup_source);
qcom->is_suspended = false;
- pm_runtime_set_active(dev);
- pm_runtime_enable(dev);
- pm_runtime_forbid(dev);
+ if (qcom->dwc_dev) {
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+ pm_runtime_forbid(dev);
+ }
return 0;
@@ -983,6 +1008,9 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
struct device *dev = &pdev->dev;
int i;
+ if (qcom->dwc)
+ dwc3_remove(qcom->dwc);
+
device_remove_software_node(&qcom->dwc_dev->dev);
if (np)
of_platform_depopulate(&pdev->dev);
@@ -998,8 +1026,10 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
dwc3_qcom_interconnect_exit(qcom);
reset_control_assert(qcom->resets);
- pm_runtime_allow(dev);
- pm_runtime_disable(dev);
+ if (qcom->dwc_dev) {
+ pm_runtime_allow(dev);
+ pm_runtime_disable(dev);
+ }
}
static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
@@ -1008,6 +1038,12 @@ static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
bool wakeup = device_may_wakeup(dev);
int ret;
+ if (qcom->dwc) {
+ ret = dwc3_suspend(qcom->dwc);
+ if (ret)
+ return ret;
+ }
+
ret = dwc3_qcom_suspend(qcom, wakeup);
if (ret)
return ret;
@@ -1029,12 +1065,33 @@ static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
qcom->pm_suspended = false;
+ if (qcom->dwc) {
+ ret = dwc3_resume(qcom->dwc);
+ if (ret)
+ return ret;
+ }
+
return 0;
}
+static void dwc3_qcom_complete(struct device *dev)
+{
+ struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+
+ if (qcom->dwc)
+ dwc3_complete(qcom->dwc);
+}
+
static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
{
struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+ int ret;
+
+ if (qcom->dwc) {
+ ret = dwc3_runtime_suspend(qcom->dwc);
+ if (ret)
+ return ret;
+ }
return dwc3_qcom_suspend(qcom, true);
}
@@ -1042,12 +1099,21 @@ static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev)
{
struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+ int ret;
+
+ ret = dwc3_qcom_resume(qcom, true);
+ if (ret)
+ return ret;
- return dwc3_qcom_resume(qcom, true);
+ if (qcom->dwc)
+ return dwc3_runtime_resume(qcom->dwc);
+
+ return 0;
}
static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(dwc3_qcom_pm_suspend, dwc3_qcom_pm_resume)
+ .complete = dwc3_qcom_complete,
SET_RUNTIME_PM_OPS(dwc3_qcom_runtime_suspend, dwc3_qcom_runtime_resume,
NULL)
};
--
2.25.1
The DWC3 IP block is handled by three distinct device drivers: XHCI,
DWC3 core and a platform specific (optional) DWC3 glue driver.
This has resulted in, at least in the case of the Qualcomm glue, the
presence of a number of layering violations, where the glue code either
can't handle, or has to work around, the fact that core might not probe
deterministically.
An example of this is that the suspend path should operate slightly
different depending on the device operating in host or peripheral mode,
and the only way to determine the operating state is to peek into the
core's drvdata.
The Qualcomm glue driver is expected to make updates in the qscratch
register region (the "glue" region) during role switch events, but with
the glue and core split using the driver model, there is no reasonable
way to introduce listeners for mode changes.
Split the dwc3 core platfrom_driver callbacks and their implementation
and export the implementation, to make it possible to deterministically
instantiate the dwc3 core as part of the dwc3 glue drivers and to
allow flattening of the DeviceTree representation.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/usb/dwc3/core.c | 134 ++++++++++++++++++++++++++++++++----------------
drivers/usb/dwc3/core.h | 10 ++++
2 files changed, 100 insertions(+), 44 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index d25490965b27..71e376bebb16 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1876,7 +1876,7 @@ static int dwc3_get_clocks(struct dwc3 *dwc)
return 0;
}
-static int dwc3_probe(struct platform_device *pdev)
+struct dwc3 *dwc3_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct resource *res, dwc_res;
@@ -1886,14 +1886,14 @@ static int dwc3_probe(struct platform_device *pdev)
dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
if (!dwc)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
dwc->dev = dev;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
dev_err(dev, "missing memory resource\n");
- return -ENODEV;
+ return ERR_PTR(-ENODEV);
}
dwc->xhci_resources[0].start = res->start;
@@ -1922,7 +1922,7 @@ static int dwc3_probe(struct platform_device *pdev)
regs = devm_ioremap_resource(dev, &dwc_res);
if (IS_ERR(regs))
- return PTR_ERR(regs);
+ return ERR_CAST(regs);
dwc->regs = regs;
dwc->regs_size = resource_size(&dwc_res);
@@ -1953,7 +1953,6 @@ static int dwc3_probe(struct platform_device *pdev)
goto err_disable_clks;
}
- platform_set_drvdata(pdev, dwc);
dwc3_cache_hwparams(dwc);
if (!dwc->sysdev_is_parent &&
@@ -2006,7 +2005,7 @@ static int dwc3_probe(struct platform_device *pdev)
pm_runtime_put(dev);
- return 0;
+ return dwc;
err_exit_debugfs:
dwc3_debugfs_exit(dwc);
@@ -2030,14 +2029,26 @@ static int dwc3_probe(struct platform_device *pdev)
if (dwc->usb_psy)
power_supply_put(dwc->usb_psy);
- return ret;
+ return ERR_PTR(ret);
}
+EXPORT_SYMBOL_GPL(dwc3_probe);
-static void dwc3_remove(struct platform_device *pdev)
+static int dwc3_plat_probe(struct platform_device *pdev)
{
- struct dwc3 *dwc = platform_get_drvdata(pdev);
+ struct dwc3 *dwc;
+
+ dwc = dwc3_probe(pdev);
+ if (IS_ERR(dwc))
+ return PTR_ERR(dwc);
+
+ platform_set_drvdata(pdev, dwc);
+
+ return 0;
+}
- pm_runtime_get_sync(&pdev->dev);
+void dwc3_remove(struct dwc3 *dwc)
+{
+ pm_runtime_get_sync(dwc->dev);
dwc3_core_exit_mode(dwc);
dwc3_debugfs_exit(dwc);
@@ -2045,22 +2056,28 @@ static void dwc3_remove(struct platform_device *pdev)
dwc3_core_exit(dwc);
dwc3_ulpi_exit(dwc);
- pm_runtime_allow(&pdev->dev);
- pm_runtime_disable(&pdev->dev);
- pm_runtime_dont_use_autosuspend(&pdev->dev);
- pm_runtime_put_noidle(&pdev->dev);
+ pm_runtime_allow(dwc->dev);
+ pm_runtime_disable(dwc->dev);
+ pm_runtime_dont_use_autosuspend(dwc->dev);
+ pm_runtime_put_noidle(dwc->dev);
/*
* HACK: Clear the driver data, which is currently accessed by parent
* glue drivers, before allowing the parent to suspend.
*/
- platform_set_drvdata(pdev, NULL);
- pm_runtime_set_suspended(&pdev->dev);
+ dev_set_drvdata(dwc->dev, NULL);
+ pm_runtime_set_suspended(dwc->dev);
dwc3_free_event_buffers(dwc);
if (dwc->usb_psy)
power_supply_put(dwc->usb_psy);
}
+EXPORT_SYMBOL_GPL(dwc3_remove);
+
+static void dwc3_plat_remove(struct platform_device *pdev)
+{
+ dwc3_remove(platform_get_drvdata(pdev));
+}
#ifdef CONFIG_PM
static int dwc3_core_init_for_resume(struct dwc3 *dwc)
@@ -2227,9 +2244,8 @@ static int dwc3_runtime_checks(struct dwc3 *dwc)
return 0;
}
-static int dwc3_runtime_suspend(struct device *dev)
+int dwc3_runtime_suspend(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
int ret;
if (dwc3_runtime_checks(dwc))
@@ -2241,10 +2257,10 @@ static int dwc3_runtime_suspend(struct device *dev)
return 0;
}
+EXPORT_SYMBOL_GPL(dwc3_runtime_suspend);
-static int dwc3_runtime_resume(struct device *dev)
+int dwc3_runtime_resume(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
int ret;
ret = dwc3_resume_common(dwc, PMSG_AUTO_RESUME);
@@ -2261,15 +2277,14 @@ static int dwc3_runtime_resume(struct device *dev)
break;
}
- pm_runtime_mark_last_busy(dev);
+ pm_runtime_mark_last_busy(dwc->dev);
return 0;
}
+EXPORT_SYMBOL_GPL(dwc3_runtime_resume);
-static int dwc3_runtime_idle(struct device *dev)
+int dwc3_runtime_idle(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
-
switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
if (dwc3_runtime_checks(dwc))
@@ -2281,49 +2296,64 @@ static int dwc3_runtime_idle(struct device *dev)
break;
}
- pm_runtime_mark_last_busy(dev);
- pm_runtime_autosuspend(dev);
+ pm_runtime_mark_last_busy(dwc->dev);
+ pm_runtime_autosuspend(dwc->dev);
return 0;
}
+EXPORT_SYMBOL_GPL(dwc3_runtime_idle);
+
+static int dwc3_plat_runtime_suspend(struct device *dev)
+{
+ return dwc3_runtime_suspend(dev_get_drvdata(dev));
+}
+
+static int dwc3_plat_runtime_resume(struct device *dev)
+{
+ return dwc3_runtime_resume(dev_get_drvdata(dev));
+}
+
+static int dwc3_plat_runtime_idle(struct device *dev)
+{
+ return dwc3_runtime_idle(dev_get_drvdata(dev));
+}
#endif /* CONFIG_PM */
#ifdef CONFIG_PM_SLEEP
-static int dwc3_suspend(struct device *dev)
+int dwc3_suspend(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
int ret;
ret = dwc3_suspend_common(dwc, PMSG_SUSPEND);
if (ret)
return ret;
- pinctrl_pm_select_sleep_state(dev);
+ pinctrl_pm_select_sleep_state(dwc->dev);
return 0;
}
+EXPORT_SYMBOL_GPL(dwc3_suspend);
-static int dwc3_resume(struct device *dev)
+int dwc3_resume(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
int ret;
- pinctrl_pm_select_default_state(dev);
+ pinctrl_pm_select_default_state(dwc->dev);
ret = dwc3_resume_common(dwc, PMSG_RESUME);
if (ret)
return ret;
- pm_runtime_disable(dev);
- pm_runtime_set_active(dev);
- pm_runtime_enable(dev);
+ pm_runtime_disable(dwc->dev);
+ pm_runtime_set_active(dwc->dev);
+ pm_runtime_enable(dwc->dev);
return 0;
}
+EXPORT_SYMBOL_GPL(dwc3_resume);
-static void dwc3_complete(struct device *dev)
+void dwc3_complete(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
u32 reg;
if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST &&
@@ -2333,15 +2363,31 @@ static void dwc3_complete(struct device *dev)
dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
}
}
+EXPORT_SYMBOL_GPL(dwc3_complete);
+
+static int dwc3_plat_suspend(struct device *dev)
+{
+ return dwc3_suspend(dev_get_drvdata(dev));
+}
+
+static int dwc3_plat_resume(struct device *dev)
+{
+ return dwc3_resume(dev_get_drvdata(dev));
+}
+
+static void dwc3_plat_complete(struct device *dev)
+{
+ dwc3_complete(dev_get_drvdata(dev));
+}
#else
-#define dwc3_complete NULL
+#define dwc3_plat_complete NULL
#endif /* CONFIG_PM_SLEEP */
static const struct dev_pm_ops dwc3_dev_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
- .complete = dwc3_complete,
- SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume,
- dwc3_runtime_idle)
+ SET_SYSTEM_SLEEP_PM_OPS(dwc3_plat_suspend, dwc3_plat_resume)
+ .complete = dwc3_plat_complete,
+ SET_RUNTIME_PM_OPS(dwc3_plat_runtime_suspend, dwc3_plat_runtime_resume,
+ dwc3_plat_runtime_idle)
};
#ifdef CONFIG_OF
@@ -2369,8 +2415,8 @@ MODULE_DEVICE_TABLE(acpi, dwc3_acpi_match);
#endif
static struct platform_driver dwc3_driver = {
- .probe = dwc3_probe,
- .remove_new = dwc3_remove,
+ .probe = dwc3_plat_probe,
+ .remove_new = dwc3_plat_remove,
.driver = {
.name = "dwc3",
.of_match_table = of_match_ptr(of_dwc3_match),
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index c6c87acbd376..f5e22559bb73 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1568,6 +1568,16 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc);
int dwc3_core_soft_reset(struct dwc3 *dwc);
+struct dwc3 *dwc3_probe(struct platform_device *pdev);
+void dwc3_remove(struct dwc3 *dwc);
+
+int dwc3_runtime_suspend(struct dwc3 *dwc);
+int dwc3_runtime_resume(struct dwc3 *dwc);
+int dwc3_runtime_idle(struct dwc3 *dwc);
+int dwc3_suspend(struct dwc3 *dwc);
+int dwc3_resume(struct dwc3 *dwc);
+void dwc3_complete(struct dwc3 *dwc);
+
#if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
int dwc3_host_init(struct dwc3 *dwc);
void dwc3_host_exit(struct dwc3 *dwc);
--
2.25.1
Add the missing compatible for the SC8180X DWC3 glue binding.
Signed-off-by: Bjorn Andersson <[email protected]>
---
Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index 51a5581ce692..727abe41c422 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -29,6 +29,7 @@ properties:
- qcom,sa8775p-dwc3
- qcom,sc7180-dwc3
- qcom,sc7280-dwc3
+ - qcom,sc8180x-dwc3
- qcom,sc8280xp-dwc3
- qcom,sc8280xp-dwc3-mp
- qcom,sdm660-dwc3
@@ -314,6 +315,7 @@ allOf:
contains:
enum:
- qcom,qcm2290-dwc3
+ - qcom,sc8180x-dwc3
- qcom,sm6115-dwc3
- qcom,sm6125-dwc3
- qcom,sm8150-dwc3
@@ -366,6 +368,7 @@ allOf:
- qcom,msm8994-dwc3
- qcom,qcs404-dwc3
- qcom,sc7180-dwc3
+ - qcom,sc8180x-dwc3
- qcom,sdm670-dwc3
- qcom,sdm845-dwc3
- qcom,sdx55-dwc3
--
2.25.1
In preparation for the introduction of a direct reference to the struct
dwc3 in the dwc3_qcom struct, rename the generically named "dwc3" to
reduce the risk for confusion.
No functional change.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 46 ++++++++++++++++++++++----------------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 3de43df6bbe8..a31c3bc1f56e 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -67,7 +67,7 @@ struct dwc3_acpi_pdata {
struct dwc3_qcom {
struct device *dev;
void __iomem *qscratch_base;
- struct platform_device *dwc3;
+ struct platform_device *dwc_dev;
struct platform_device *urs_usb;
struct clk **clks;
int num_clocks;
@@ -264,7 +264,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->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);
@@ -312,7 +312,7 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
/*
* FIXME: Fix this layering violation.
*/
- dwc = platform_get_drvdata(qcom->dwc3);
+ dwc = platform_get_drvdata(qcom->dwc_dev);
/* Core driver may not have probed yet. */
if (!dwc)
@@ -323,7 +323,7 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
{
- struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+ struct dwc3 *dwc = platform_get_drvdata(qcom->dwc_dev);
struct usb_device *udev;
struct usb_hcd __maybe_unused *hcd;
@@ -486,7 +486,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 = platform_get_drvdata(qcom->dwc_dev);
/* If pm_suspended then let pm_resume take care of resuming h/w */
if (qcom->pm_suspended)
@@ -672,19 +672,19 @@ static int dwc3_qcom_acpi_register_core(struct platform_device *pdev)
int irq;
int ret;
- qcom->dwc3 = platform_device_alloc("dwc3", PLATFORM_DEVID_AUTO);
- if (!qcom->dwc3)
+ qcom->dwc_dev = platform_device_alloc("dwc3", PLATFORM_DEVID_AUTO);
+ if (!qcom->dwc_dev)
return -ENOMEM;
- qcom->dwc3->dev.parent = dev;
- qcom->dwc3->dev.type = dev->type;
- qcom->dwc3->dev.dma_mask = dev->dma_mask;
- qcom->dwc3->dev.dma_parms = dev->dma_parms;
- qcom->dwc3->dev.coherent_dma_mask = dev->coherent_dma_mask;
+ qcom->dwc_dev->dev.parent = dev;
+ qcom->dwc_dev->dev.type = dev->type;
+ qcom->dwc_dev->dev.dma_mask = dev->dma_mask;
+ qcom->dwc_dev->dev.dma_parms = dev->dma_parms;
+ qcom->dwc_dev->dev.coherent_dma_mask = dev->coherent_dma_mask;
child_res = kcalloc(2, sizeof(*child_res), GFP_KERNEL);
if (!child_res) {
- platform_device_put(qcom->dwc3);
+ platform_device_put(qcom->dwc_dev);
return -ENOMEM;
}
@@ -708,29 +708,29 @@ static int dwc3_qcom_acpi_register_core(struct platform_device *pdev)
child_res[1].flags = IORESOURCE_IRQ;
child_res[1].start = child_res[1].end = irq;
- ret = platform_device_add_resources(qcom->dwc3, child_res, 2);
+ ret = platform_device_add_resources(qcom->dwc_dev, child_res, 2);
if (ret) {
dev_err(&pdev->dev, "failed to add resources\n");
goto out;
}
- ret = device_add_software_node(&qcom->dwc3->dev, &dwc3_qcom_swnode);
+ ret = device_add_software_node(&qcom->dwc_dev->dev, &dwc3_qcom_swnode);
if (ret < 0) {
dev_err(&pdev->dev, "failed to add properties\n");
goto out;
}
- ret = platform_device_add(qcom->dwc3);
+ ret = platform_device_add(qcom->dwc_dev);
if (ret) {
dev_err(&pdev->dev, "failed to add device\n");
- device_remove_software_node(&qcom->dwc3->dev);
+ device_remove_software_node(&qcom->dwc_dev->dev);
goto out;
}
kfree(child_res);
return 0;
out:
- platform_device_put(qcom->dwc3);
+ platform_device_put(qcom->dwc_dev);
kfree(child_res);
return ret;
}
@@ -754,8 +754,8 @@ static int dwc3_qcom_of_register_core(struct platform_device *pdev)
goto node_put;
}
- qcom->dwc3 = of_find_device_by_node(dwc3_np);
- if (!qcom->dwc3) {
+ qcom->dwc_dev = of_find_device_by_node(dwc3_np);
+ if (!qcom->dwc_dev) {
ret = -ENODEV;
dev_err(dev, "failed to get dwc3 platform device\n");
}
@@ -906,7 +906,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
if (ret)
goto depopulate;
- qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
+ qcom->mode = usb_get_dr_mode(&qcom->dwc_dev->dev);
/* enable vbus override for device mode */
if (qcom->mode != USB_DR_MODE_HOST)
@@ -919,7 +919,7 @@ 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);
+ device_init_wakeup(&qcom->dwc_dev->dev, wakeup_source);
qcom->is_suspended = false;
pm_runtime_set_active(dev);
@@ -953,7 +953,7 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
struct device *dev = &pdev->dev;
int i;
- device_remove_software_node(&qcom->dwc3->dev);
+ device_remove_software_node(&qcom->dwc_dev->dev);
if (np)
of_platform_depopulate(&pdev->dev);
else
--
2.25.1
The Qualcomm USB block consists of three intertwined parts, the XHCI,
the DWC3 core and the Qualcomm DWC3 glue. The three parts can not be
operated independently, but the binding was for historical reasons split
to mimic the Linux driver implementation.
The split binding also makes it hard to alter the implementation, as
properties and resources are split between the two nodes, in some cases
with some duplication.
Introduce a new binding, with a single representation of the whole USB
block in one node.
Signed-off-by: Bjorn Andersson <[email protected]>
---
.../devicetree/bindings/usb/qcom,dwc3.yaml | 482 +++++++++++++++++++++
.../devicetree/bindings/usb/snps,dwc3.yaml | 14 +-
2 files changed, 491 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
new file mode 100644
index 000000000000..cb50261c6a36
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -0,0 +1,482 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SuperSpeed DWC3 USB SoC controller
+
+maintainers:
+ - Wesley Cheng <[email protected]>
+
+select:
+ properties:
+ compatible:
+ items:
+ - enum:
+ - qcom,ipq4019-dwc3
+ - qcom,ipq5018-dwc3
+ - qcom,ipq5332-dwc3
+ - qcom,ipq6018-dwc3
+ - qcom,ipq8064-dwc3
+ - qcom,ipq8074-dwc3
+ - qcom,ipq9574-dwc3
+ - qcom,msm8953-dwc3
+ - qcom,msm8994-dwc3
+ - qcom,msm8996-dwc3
+ - qcom,msm8998-dwc3
+ - qcom,qcm2290-dwc3
+ - qcom,qcs404-dwc3
+ - qcom,sa8775p-dwc3
+ - qcom,sc7180-dwc3
+ - qcom,sc7280-dwc3
+ - qcom,sc8180x-dwc3
+ - qcom,sc8280xp-dwc3
+ - qcom,sc8280xp-dwc3-mp
+ - qcom,sdm660-dwc3
+ - qcom,sdm670-dwc3
+ - qcom,sdm845-dwc3
+ - qcom,sdx55-dwc3
+ - qcom,sdx65-dwc3
+ - qcom,sdx75-dwc3
+ - qcom,sm4250-dwc3
+ - qcom,sm6115-dwc3
+ - qcom,sm6125-dwc3
+ - qcom,sm6350-dwc3
+ - qcom,sm6375-dwc3
+ - qcom,sm8150-dwc3
+ - qcom,sm8250-dwc3
+ - qcom,sm8350-dwc3
+ - qcom,sm8450-dwc3
+ - qcom,sm8550-dwc3
+ - const: qcom,dwc3
+ - const: snps,dwc3
+ required:
+ - compatible
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - qcom,ipq4019-dwc3
+ - qcom,ipq5018-dwc3
+ - qcom,ipq5332-dwc3
+ - qcom,ipq6018-dwc3
+ - qcom,ipq8064-dwc3
+ - qcom,ipq8074-dwc3
+ - qcom,ipq9574-dwc3
+ - qcom,msm8953-dwc3
+ - qcom,msm8994-dwc3
+ - qcom,msm8996-dwc3
+ - qcom,msm8998-dwc3
+ - qcom,qcm2290-dwc3
+ - qcom,qcs404-dwc3
+ - qcom,sa8775p-dwc3
+ - qcom,sc7180-dwc3
+ - qcom,sc7280-dwc3
+ - qcom,sc8180x-dwc3
+ - qcom,sc8280xp-dwc3
+ - qcom,sc8280xp-dwc3-mp
+ - qcom,sdm660-dwc3
+ - qcom,sdm670-dwc3
+ - qcom,sdm845-dwc3
+ - qcom,sdx55-dwc3
+ - qcom,sdx65-dwc3
+ - qcom,sdx75-dwc3
+ - qcom,sm4250-dwc3
+ - qcom,sm6115-dwc3
+ - qcom,sm6125-dwc3
+ - qcom,sm6350-dwc3
+ - qcom,sm6375-dwc3
+ - qcom,sm8150-dwc3
+ - qcom,sm8250-dwc3
+ - qcom,sm8350-dwc3
+ - qcom,sm8450-dwc3
+ - qcom,sm8550-dwc3
+ - const: qcom,dwc3
+ - const: snps,dwc3
+
+ reg:
+ description: Offset and length of register set for QSCRATCH wrapper
+ maxItems: 1
+
+ power-domains:
+ description: specifies a phandle to PM domain provider node
+ maxItems: 1
+
+ required-opps:
+ maxItems: 1
+
+ clocks:
+ description: |
+ Several clocks are used, depending on the variant. Typical ones are::
+ - cfg_noc:: System Config NOC clock.
+ - core:: Master/Core clock, has to be >= 125 MHz for SS operation and >=
+ 60MHz for HS operation.
+ - iface:: System bus AXI clock.
+ - sleep:: Sleep clock, used for wakeup when USB3 core goes into low
+ power mode (U3).
+ - mock_utmi:: Mock utmi clock needed for ITP/SOF generation in host
+ mode. Its frequency should be 19.2MHz.
+ minItems: 1
+ maxItems: 9
+
+ clock-names:
+ minItems: 1
+ maxItems: 9
+
+ resets:
+ maxItems: 1
+
+ interconnects:
+ maxItems: 2
+
+ interconnect-names:
+ items:
+ - const: usb-ddr
+ - const: apps-usb
+
+ interrupts:
+ minItems: 2
+ maxItems: 5
+
+ interrupt-names:
+ minItems: 2
+ maxItems: 5
+
+ qcom,select-utmi-as-pipe-clk:
+ description:
+ If present, disable USB3 pipe_clk requirement.
+ Used when dwc3 operates without SSPHY and only
+ HS/FS/LS modes are supported.
+ type: boolean
+
+ wakeup-source: true
+
+# Required child node:
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - interrupts
+ - interrupt-names
+
+allOf:
+ - $ref: snps,dwc3.yaml#
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq4019-dwc3
+ then:
+ properties:
+ clocks:
+ maxItems: 3
+ clock-names:
+ items:
+ - const: core
+ - const: sleep
+ - const: mock_utmi
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq8064-dwc3
+ then:
+ properties:
+ clocks:
+ items:
+ - description: Master/Core clock, has to be >= 125 MHz
+ for SS operation and >= 60MHz for HS operation.
+ clock-names:
+ items:
+ - const: core
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq9574-dwc3
+ - qcom,msm8953-dwc3
+ - qcom,msm8996-dwc3
+ - qcom,msm8998-dwc3
+ - qcom,sa8775p-dwc3
+ - qcom,sc7180-dwc3
+ - qcom,sc7280-dwc3
+ - qcom,sdm670-dwc3
+ - qcom,sdm845-dwc3
+ - qcom,sdx55-dwc3
+ - qcom,sdx65-dwc3
+ - qcom,sdx75-dwc3
+ - qcom,sm6350-dwc3
+ then:
+ properties:
+ clocks:
+ maxItems: 5
+ clock-names:
+ items:
+ - const: cfg_noc
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq6018-dwc3
+ then:
+ properties:
+ clocks:
+ minItems: 3
+ maxItems: 4
+ clock-names:
+ oneOf:
+ - items:
+ - const: core
+ - const: sleep
+ - const: mock_utmi
+ - items:
+ - const: cfg_noc
+ - const: core
+ - const: sleep
+ - const: mock_utmi
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq8074-dwc3
+ then:
+ properties:
+ clocks:
+ maxItems: 4
+ clock-names:
+ items:
+ - const: cfg_noc
+ - const: core
+ - const: sleep
+ - const: mock_utmi
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq5018-dwc3
+ - qcom,ipq5332-dwc3
+ - qcom,msm8994-dwc3
+ - qcom,qcs404-dwc3
+ then:
+ properties:
+ clocks:
+ maxItems: 4
+ clock-names:
+ items:
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sc8280xp-dwc3
+ - qcom,sc8280xp-dwc3-mp
+ then:
+ properties:
+ clocks:
+ maxItems: 9
+ clock-names:
+ items:
+ - const: cfg_noc
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+ - const: noc_aggr
+ - const: noc_aggr_north
+ - const: noc_aggr_south
+ - const: noc_sys
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sdm660-dwc3
+ then:
+ properties:
+ clocks:
+ minItems: 5
+ maxItems: 6
+ clock-names:
+ oneOf:
+ - items:
+ - const: cfg_noc
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+ - const: bus
+ - items:
+ - const: cfg_noc
+ - const: core
+ - const: sleep
+ - const: mock_utmi
+ - const: bus
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,qcm2290-dwc3
+ - qcom,sc8180x-dwc3
+ - qcom,sm6115-dwc3
+ - qcom,sm6125-dwc3
+ - qcom,sm8150-dwc3
+ - qcom,sm8250-dwc3
+ - qcom,sm8450-dwc3
+ - qcom,sm8550-dwc3
+ then:
+ properties:
+ clocks:
+ minItems: 6
+ clock-names:
+ items:
+ - const: cfg_noc
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+ - const: xo
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sm8350-dwc3
+ then:
+ properties:
+ clocks:
+ minItems: 5
+ maxItems: 6
+ clock-names:
+ minItems: 5
+ items:
+ - const: cfg_noc
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+ - const: xo
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sc8280xp-dwc3-mp
+ then:
+ properties:
+ interrupts:
+ maxItems: 14
+ interrupt-names:
+ items:
+ - const: pwr_event_1
+ - const: pwr_event_2
+ - const: pwr_event_3
+ - const: pwr_event_4
+ - const: dp_hs_phy_1
+ - const: dm_hs_phy_1
+ - const: dp_hs_phy_2
+ - const: dm_hs_phy_2
+ - const: dp_hs_phy_3
+ - const: dm_hs_phy_3
+ - const: dp_hs_phy_4
+ - const: dm_hs_phy_4
+ - const: ss_phy_1
+ - const: ss_phy_2
+ else:
+ properties:
+ interrupts:
+ minItems: 1
+ items:
+ - description: Common DWC3 interrupt
+ - description: The interrupt that is asserted
+ when a wakeup event is received on USB2 bus.
+ - description: The interrupt that is asserted
+ when a wakeup event is received on USB3 bus.
+ - description: Wakeup event on DM line.
+ - description: Wakeup event on DP line.
+
+ interrupt-names:
+ minItems: 1
+ items:
+ - const: dwc_usb3
+ - const: hs_phy_irq
+ - const: ss_phy_irq
+ - const: dm_hs_phy_irq
+ - const: dp_hs_phy_irq
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ usb@a600000 {
+ compatible = "qcom,sdm845-dwc3", "qcom,dwc3", "snps,dwc3";
+ reg = <0x0a600000 0x200000>;
+
+ clocks = <&gcc GCC_CFG_NOC_USB3_PRIM_AXI_CLK>,
+ <&gcc GCC_USB30_PRIM_MASTER_CLK>,
+ <&gcc GCC_AGGRE_USB3_PRIM_AXI_CLK>,
+ <&gcc GCC_USB30_PRIM_SLEEP_CLK>,
+ <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>;
+ clock-names = "cfg_noc",
+ "core",
+ "iface",
+ "sleep",
+ "mock_utmi";
+
+ assigned-clocks = <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>,
+ <&gcc GCC_USB30_PRIM_MASTER_CLK>;
+ assigned-clock-rates = <19200000>, <150000000>;
+
+ interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 488 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 489 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "dwc_usb3", "hs_phy_irq", "ss_phy_irq",
+ "dm_hs_phy_irq", "dp_hs_phy_irq";
+
+ power-domains = <&gcc USB30_PRIM_GDSC>;
+
+ iommus = <&apps_smmu 0x740 0>;
+
+ resets = <&gcc GCC_USB30_PRIM_BCR>;
+
+ snps,dis_u2_susphy_quirk;
+ snps,dis_enblslpm_quirk;
+ phys = <&usb_1_hsphy>, <&usb_1_ssphy>;
+ phy-names = "usb2-phy", "usb3-phy";
+
+ };
+...
diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index d81c2e849ca9..d6914b8cef6a 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -44,14 +44,18 @@ properties:
It's either a single common DWC3 interrupt (dwc_usb3) or individual
interrupts for the host, gadget and DRD modes.
minItems: 1
- maxItems: 4
+ maxItems: 5
interrupt-names:
- minItems: 1
- maxItems: 4
oneOf:
- - const: dwc_usb3
- - items:
+ - minItems: 1
+ maxItems: 5
+ items:
+ - const: dwc_usb3
+ additionalItems: true
+ - minItems: 1
+ maxItems: 4
+ items:
enum: [host, peripheral, otg, wakeup]
clocks:
--
2.25.1
The Qualcomm DWC3 glue builds up a platform_device programmatically in
order to probe the DWC3 core when running off ACPI data. But with the
newly introduced support for instantiating the core directly from the
glue, this code can be replaced with a single function call.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 85 ++++++++------------------------------------
1 file changed, 15 insertions(+), 70 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 901e5050363b..cc0fe010ee8c 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -56,7 +56,6 @@
struct dwc3_acpi_pdata {
u32 qscratch_base_offset;
u32 qscratch_base_size;
- u32 dwc3_core_base_size;
int hs_phy_irq_index;
int dp_hs_phy_irq_index;
int dm_hs_phy_irq_index;
@@ -676,75 +675,17 @@ static const struct software_node dwc3_qcom_swnode = {
.properties = dwc3_qcom_acpi_properties,
};
-static int dwc3_qcom_acpi_register_core(struct platform_device *pdev)
+static int dwc3_qcom_probe_core(struct platform_device *pdev, struct dwc3_qcom *qcom)
{
- struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
- struct device *dev = &pdev->dev;
- struct resource *res, *child_res = NULL;
- int irq;
- int ret;
-
- qcom->dwc_dev = platform_device_alloc("dwc3", PLATFORM_DEVID_AUTO);
- if (!qcom->dwc_dev)
- return -ENOMEM;
-
- qcom->dwc_dev->dev.parent = dev;
- qcom->dwc_dev->dev.type = dev->type;
- qcom->dwc_dev->dev.dma_mask = dev->dma_mask;
- qcom->dwc_dev->dev.dma_parms = dev->dma_parms;
- qcom->dwc_dev->dev.coherent_dma_mask = dev->coherent_dma_mask;
-
- child_res = kcalloc(2, sizeof(*child_res), GFP_KERNEL);
- if (!child_res) {
- platform_device_put(qcom->dwc_dev);
- return -ENOMEM;
- }
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- dev_err(&pdev->dev, "failed to get memory resource\n");
- ret = -ENODEV;
- goto out;
- }
-
- child_res[0].flags = res->flags;
- child_res[0].start = res->start;
- child_res[0].end = child_res[0].start +
- qcom->acpi_pdata->dwc3_core_base_size;
-
- irq = platform_get_irq(pdev, 0);
- if (irq < 0) {
- ret = irq;
- goto out;
- }
- child_res[1].flags = IORESOURCE_IRQ;
- child_res[1].start = child_res[1].end = irq;
+ struct dwc3 *dwc;
- ret = platform_device_add_resources(qcom->dwc_dev, child_res, 2);
- if (ret) {
- dev_err(&pdev->dev, "failed to add resources\n");
- goto out;
- }
+ dwc = dwc3_probe(pdev);
+ if (IS_ERR(dwc))
+ return PTR_ERR(dwc);
- ret = device_add_software_node(&qcom->dwc_dev->dev, &dwc3_qcom_swnode);
- if (ret < 0) {
- dev_err(&pdev->dev, "failed to add properties\n");
- goto out;
- }
+ qcom->dwc = dwc;
- ret = platform_device_add(qcom->dwc_dev);
- if (ret) {
- dev_err(&pdev->dev, "failed to add device\n");
- device_remove_software_node(&qcom->dwc_dev->dev);
- goto out;
- }
- kfree(child_res);
return 0;
-
-out:
- platform_device_put(qcom->dwc_dev);
- kfree(child_res);
- return ret;
}
static int dwc3_qcom_of_register_core(struct platform_device *pdev)
@@ -871,6 +812,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
return -EINVAL;
}
+ ret = device_add_software_node(&pdev->dev, &dwc3_qcom_swnode);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to add properties\n");
+ return ret;
+ }
+
if (qcom->acpi_pdata->is_urs) {
ret = dwc3_qcom_acpi_merge_urs_resources(pdev);
if (ret < 0)
@@ -942,7 +889,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
if (np)
ret = dwc3_qcom_of_register_core(pdev);
else
- ret = dwc3_qcom_acpi_register_core(pdev);
+ ret = dwc3_qcom_probe_core(pdev, qcom);
if (ret) {
dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret);
@@ -986,10 +933,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
interconnect_exit:
dwc3_qcom_interconnect_exit(qcom);
depopulate:
- if (np)
+ if (qcom->dwc_dev)
of_platform_depopulate(&pdev->dev);
else
- platform_device_put(pdev);
+ dwc3_remove(qcom->dwc);
clk_disable:
for (i = qcom->num_clocks - 1; i >= 0; i--) {
clk_disable_unprepare(qcom->clks[i]);
@@ -1128,7 +1075,6 @@ MODULE_DEVICE_TABLE(of, dwc3_qcom_of_match);
static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
.qscratch_base_size = SDM845_QSCRATCH_SIZE,
- .dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
.hs_phy_irq_index = 1,
.dp_hs_phy_irq_index = 4,
.dm_hs_phy_irq_index = 3,
@@ -1138,7 +1084,6 @@ static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
static const struct dwc3_acpi_pdata sdm845_acpi_urs_pdata = {
.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
.qscratch_base_size = SDM845_QSCRATCH_SIZE,
- .dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
.hs_phy_irq_index = 1,
.dp_hs_phy_irq_index = 4,
.dm_hs_phy_irq_index = 3,
--
2.25.1
Flatten one of the USB controllers in the SC8180X platform, as an
example.
Signed-off-by: Bjorn Andersson <[email protected]>
---
.../arm64/boot/dts/qcom/sc8180x-lenovo-flex-5g.dts | 6 ++--
arch/arm64/boot/dts/qcom/sc8180x-primus.dts | 6 ++--
arch/arm64/boot/dts/qcom/sc8180x.dtsi | 34 +++++++++-------------
3 files changed, 17 insertions(+), 29 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc8180x-lenovo-flex-5g.dts b/arch/arm64/boot/dts/qcom/sc8180x-lenovo-flex-5g.dts
index 3ea07d094b60..91a9d822ea43 100644
--- a/arch/arm64/boot/dts/qcom/sc8180x-lenovo-flex-5g.dts
+++ b/arch/arm64/boot/dts/qcom/sc8180x-lenovo-flex-5g.dts
@@ -607,11 +607,9 @@ &usb_sec_role_switch {
};
&usb_sec {
- status = "okay";
-};
-
-&usb_sec_dwc3 {
dr_mode = "host";
+
+ status = "okay";
};
&wifi {
diff --git a/arch/arm64/boot/dts/qcom/sc8180x-primus.dts b/arch/arm64/boot/dts/qcom/sc8180x-primus.dts
index fd2fab4895b3..a17c69b5aa57 100644
--- a/arch/arm64/boot/dts/qcom/sc8180x-primus.dts
+++ b/arch/arm64/boot/dts/qcom/sc8180x-primus.dts
@@ -684,11 +684,9 @@ &usb_sec_role_switch {
};
&usb_sec {
- status = "okay";
-};
-
-&usb_sec_dwc3 {
dr_mode = "host";
+
+ status = "okay";
};
&wifi {
diff --git a/arch/arm64/boot/dts/qcom/sc8180x.dtsi b/arch/arm64/boot/dts/qcom/sc8180x.dtsi
index a34f438ef2d9..f5e427789ad8 100644
--- a/arch/arm64/boot/dts/qcom/sc8180x.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8180x.dtsi
@@ -2605,8 +2605,8 @@ usb_prim_role_switch: endpoint {
};
usb_sec: usb@a8f8800 {
- compatible = "qcom,sc8180x-dwc3", "qcom,dwc3";
- reg = <0 0x0a8f8800 0 0x400>;
+ compatible = "qcom,sc8180x-dwc3", "qcom,dwc3", "snps,dwc3";
+ reg = <0 0x0a800000 0 0x200000>;
clocks = <&gcc GCC_CFG_NOC_USB3_SEC_AXI_CLK>,
<&gcc GCC_USB30_SEC_MASTER_CLK>,
@@ -2622,11 +2622,12 @@ usb_sec: usb@a8f8800 {
"xo";
resets = <&gcc GCC_USB30_SEC_BCR>;
power-domains = <&gcc USB30_SEC_GDSC>;
- interrupts = <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>,
+ interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 487 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 490 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 491 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "hs_phy_irq", "ss_phy_irq",
+ interrupt-names = "dwc_usb3", "hs_phy_irq", "ss_phy_irq",
"dm_hs_phy_irq", "dp_hs_phy_irq";
assigned-clocks = <&gcc GCC_USB30_SEC_MOCK_UTMI_CLK>,
@@ -2637,26 +2638,17 @@ usb_sec: usb@a8f8800 {
<&gem_noc MASTER_AMPSS_M0 0 &config_noc SLAVE_USB3_1 0>;
interconnect-names = "usb-ddr", "apps-usb";
- #address-cells = <2>;
- #size-cells = <2>;
- ranges;
- dma-ranges;
+ iommus = <&apps_smmu 0x160 0>;
- status = "disabled";
+ snps,dis_u2_susphy_quirk;
+ snps,dis_enblslpm_quirk;
+ phys = <&usb_sec_hsphy>, <&usb_sec_ssphy>;
+ phy-names = "usb2-phy", "usb3-phy";
- usb_sec_dwc3: usb@a800000 {
- compatible = "snps,dwc3";
- reg = <0 0x0a800000 0 0xcd00>;
- interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
- iommus = <&apps_smmu 0x160 0>;
- snps,dis_u2_susphy_quirk;
- snps,dis_enblslpm_quirk;
- phys = <&usb_sec_hsphy>, <&usb_sec_ssphy>;
- phy-names = "usb2-phy", "usb3-phy";
+ status = "disabled";
- port {
- usb_sec_role_switch: endpoint {
- };
+ port {
+ usb_sec_role_switch: endpoint {
};
};
};
--
2.25.1
The USB block found in most Qualcomm platforms is modelled as three
different independent device drivers, and represented in DeviceTree as
two layered nodes. But as shown by the already existing layering
violations in the Qualcomm glue driver they can not be operated
independently.
In the current model, the probing of the core is asynchronous, and in a
number of places there's risk that the driver dereferences NULL
pointers, as it peeks into the core's drvdata.
There is also no way, in the current design to make the core notify the
glue upon DRD mode changes. Among the past proposals have been attempts
to provide a callback registration API, but as there is no way to know
when the core is probed this doesn't work.
Based on the recent refactoring its now possible to instantiate the glue
and core from a single representation of the DWC3 IP-block. This will
also allow for the glue to pass a callback to be called for DRD mode
changes.
The only overlapping handling between the Qualcomm glue and the core is
the release of reset, which is left to the core to handle.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 49 +++++++++++++++++++++++++++++---------------
1 file changed, 32 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index cf6c391ba498..3c9a2b5cd559 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -686,6 +686,16 @@ static int dwc3_qcom_probe_core(struct platform_device *pdev, struct dwc3_qcom *
return 0;
}
+static bool dwc3_qcom_has_separate_dwc3_of_node(struct device *dev)
+{
+ struct device_node *np;
+
+ np = of_get_compatible_child(dev->of_node, "snps,dwc3");
+ of_node_put(np);
+
+ return !!np;
+}
+
static int dwc3_qcom_of_register_core(struct platform_device *pdev)
{
struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
@@ -795,11 +805,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
int ret, i;
bool ignore_pipe_clk;
bool wakeup_source;
+ bool legacy_binding;
qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
if (!qcom)
return -ENOMEM;
+ legacy_binding = dwc3_qcom_has_separate_dwc3_of_node(dev);
+
platform_set_drvdata(pdev, qcom);
qcom->dev = &pdev->dev;
@@ -823,24 +836,26 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
}
}
- qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
- if (IS_ERR(qcom->resets)) {
- return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
- "failed to get resets\n");
- }
+ if (legacy_binding) {
+ qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
+ if (IS_ERR(qcom->resets)) {
+ return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
+ "failed to get resets\n");
+ }
- ret = reset_control_assert(qcom->resets);
- if (ret) {
- dev_err(&pdev->dev, "failed to assert resets, err=%d\n", ret);
- return ret;
- }
+ ret = reset_control_assert(qcom->resets);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to assert resets, err=%d\n", ret);
+ return ret;
+ }
- usleep_range(10, 1000);
+ usleep_range(10, 1000);
- ret = reset_control_deassert(qcom->resets);
- if (ret) {
- dev_err(&pdev->dev, "failed to deassert resets, err=%d\n", ret);
- goto reset_assert;
+ ret = reset_control_deassert(qcom->resets);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to deassert resets, err=%d\n", ret);
+ goto reset_assert;
+ }
}
ret = dwc3_qcom_clk_init(qcom, of_clk_get_parent_count(np));
@@ -851,7 +866,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (np) {
+ if (legacy_binding) {
parent_res = res;
} else {
memcpy(&local_res, res, sizeof(struct resource));
@@ -882,7 +897,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
if (ignore_pipe_clk)
dwc3_qcom_select_utmi_clk(qcom);
- if (np)
+ if (legacy_binding)
ret = dwc3_qcom_of_register_core(pdev);
else
ret = dwc3_qcom_probe_core(pdev, qcom);
--
2.25.1
The two constants for the offset and size of the qscratch block within
the DWC3 region has the same value in all supported variants, so they
don't necessarily need to be picked dynamically.
By replacing the lookup with the constants it's possible to reuse the
same code path without the ACPI pdata structure in the upcoming commit.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index cc0fe010ee8c..cf6c391ba498 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -54,8 +54,6 @@
#define APPS_USB_PEAK_BW MBps_to_icc(40)
struct dwc3_acpi_pdata {
- u32 qscratch_base_offset;
- u32 qscratch_base_size;
int hs_phy_irq_index;
int dp_hs_phy_irq_index;
int dm_hs_phy_irq_index;
@@ -859,10 +857,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
memcpy(&local_res, res, sizeof(struct resource));
parent_res = &local_res;
- parent_res->start = res->start +
- qcom->acpi_pdata->qscratch_base_offset;
- parent_res->end = parent_res->start +
- qcom->acpi_pdata->qscratch_base_size;
+ parent_res->start = res->start + SDM845_QSCRATCH_BASE_OFFSET;
+ parent_res->end = parent_res->start + SDM845_QSCRATCH_SIZE;
}
qcom->qscratch_base = devm_ioremap_resource(dev, parent_res);
@@ -1073,8 +1069,6 @@ MODULE_DEVICE_TABLE(of, dwc3_qcom_of_match);
#ifdef CONFIG_ACPI
static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
- .qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
- .qscratch_base_size = SDM845_QSCRATCH_SIZE,
.hs_phy_irq_index = 1,
.dp_hs_phy_irq_index = 4,
.dm_hs_phy_irq_index = 3,
@@ -1082,8 +1076,6 @@ static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
};
static const struct dwc3_acpi_pdata sdm845_acpi_urs_pdata = {
- .qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
- .qscratch_base_size = SDM845_QSCRATCH_SIZE,
.hs_phy_irq_index = 1,
.dp_hs_phy_irq_index = 4,
.dm_hs_phy_irq_index = 3,
--
2.25.1
The Qualcomm USB block consists of three intertwined parts, the XHCI,
the DWC3 core and the Qualcomm DWC3 glue. The exsting binding represents
the Qualcomm glue part, with the other two represented as in a child
node.
Rename the qcom,dwc3 binding, to represent that this is indeed only the
glue part, to make room for a combined binding.
The large "select" is included to avoid the schema to be selected for
validation with the upcoming flattened binding - which includes
snps,dwc3 in the compatible.
Signed-off-by: Bjorn Andersson <[email protected]>
---
.../usb/{qcom,dwc3.yaml => qcom,dwc3-glue.yaml} | 54 +++++++++++++++++++++-
1 file changed, 52 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3-glue.yaml
similarity index 89%
rename from Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
rename to Documentation/devicetree/bindings/usb/qcom,dwc3-glue.yaml
index 727abe41c422..bec0151b41d2 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3-glue.yaml
@@ -1,14 +1,64 @@
# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
%YAML 1.2
---
-$id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#
+$id: http://devicetree.org/schemas/usb/qcom,dwc3-glue.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: Qualcomm SuperSpeed DWC3 USB SoC controller
+title: Qualcomm SuperSpeed DWC3 USB SoC controller glue
+
+description:
+ This describes the Qualcomm glue-section of the SuperSpeed DWC3 USB
+ controller found in many Qualcomm platforms, with the XHCI and DWC3 core
+ portions described as a separate child device.
+ The combined representation, defined by qcom,dwc3.yaml is preferred.
maintainers:
- Wesley Cheng <[email protected]>
+select:
+ properties:
+ compatible:
+ items:
+ - enum:
+ - qcom,ipq4019-dwc3
+ - qcom,ipq5018-dwc3
+ - qcom,ipq5332-dwc3
+ - qcom,ipq6018-dwc3
+ - qcom,ipq8064-dwc3
+ - qcom,ipq8074-dwc3
+ - qcom,ipq9574-dwc3
+ - qcom,msm8953-dwc3
+ - qcom,msm8994-dwc3
+ - qcom,msm8996-dwc3
+ - qcom,msm8998-dwc3
+ - qcom,qcm2290-dwc3
+ - qcom,qcs404-dwc3
+ - qcom,sa8775p-dwc3
+ - qcom,sc7180-dwc3
+ - qcom,sc7280-dwc3
+ - qcom,sc8180x-dwc3
+ - qcom,sc8280xp-dwc3
+ - qcom,sc8280xp-dwc3-mp
+ - qcom,sdm660-dwc3
+ - qcom,sdm670-dwc3
+ - qcom,sdm845-dwc3
+ - qcom,sdx55-dwc3
+ - qcom,sdx65-dwc3
+ - qcom,sdx75-dwc3
+ - qcom,sm4250-dwc3
+ - qcom,sm6115-dwc3
+ - qcom,sm6125-dwc3
+ - qcom,sm6350-dwc3
+ - qcom,sm6375-dwc3
+ - qcom,sm8150-dwc3
+ - qcom,sm8250-dwc3
+ - qcom,sm8350-dwc3
+ - qcom,sm8450-dwc3
+ - qcom,sm8550-dwc3
+ - const: qcom,dwc3
+ required:
+ - compatible
+
properties:
compatible:
items:
--
2.25.1
On 17/10/2023 05:11, Bjorn Andersson wrote:
> Add the missing compatible for the SC8180X DWC3 glue binding.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 3 +++
> 1 file changed, 3 insertions(+)
This was already sent:
https://lore.kernel.org/all/[email protected]/
Best regards,
Krzysztof
On 17/10/2023 05:11, Bjorn Andersson wrote:
> The Qualcomm USB block consists of three intertwined parts, the XHCI,
> the DWC3 core and the Qualcomm DWC3 glue. The exsting binding represents
> the Qualcomm glue part, with the other two represented as in a child
> node.
>
> Rename the qcom,dwc3 binding, to represent that this is indeed only the
> glue part, to make room for a combined binding.
>
> The large "select" is included to avoid the schema to be selected for
> validation with the upcoming flattened binding - which includes
> snps,dwc3 in the compatible.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 17/10/2023 05:11, Bjorn Andersson wrote:
> The Qualcomm USB block consists of three intertwined parts, the XHCI,
> the DWC3 core and the Qualcomm DWC3 glue. The three parts can not be
> operated independently, but the binding was for historical reasons split
> to mimic the Linux driver implementation.
>
> The split binding also makes it hard to alter the implementation, as
> properties and resources are split between the two nodes, in some cases
> with some duplication.
>
> Introduce a new binding, with a single representation of the whole USB
> block in one node.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> .../devicetree/bindings/usb/qcom,dwc3.yaml | 482 +++++++++++++++++++++
> .../devicetree/bindings/usb/snps,dwc3.yaml | 14 +-
> 2 files changed, 491 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> new file mode 100644
> index 000000000000..cb50261c6a36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -0,0 +1,482 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SuperSpeed DWC3 USB SoC controller
> +
> +maintainers:
> + - Wesley Cheng <[email protected]>
> +
> +select:
> + properties:
> + compatible:
> + items:
> + - enum:
> + - qcom,ipq4019-dwc3
> + - qcom,ipq5018-dwc3
> + - qcom,ipq5332-dwc3
> + - qcom,ipq6018-dwc3
> + - qcom,ipq8064-dwc3
> + - qcom,ipq8074-dwc3
> + - qcom,ipq9574-dwc3
> + - qcom,msm8953-dwc3
> + - qcom,msm8994-dwc3
> + - qcom,msm8996-dwc3
> + - qcom,msm8998-dwc3
> + - qcom,qcm2290-dwc3
> + - qcom,qcs404-dwc3
> + - qcom,sa8775p-dwc3
> + - qcom,sc7180-dwc3
> + - qcom,sc7280-dwc3
> + - qcom,sc8180x-dwc3
> + - qcom,sc8280xp-dwc3
> + - qcom,sc8280xp-dwc3-mp
> + - qcom,sdm660-dwc3
> + - qcom,sdm670-dwc3
> + - qcom,sdm845-dwc3
> + - qcom,sdx55-dwc3
> + - qcom,sdx65-dwc3
> + - qcom,sdx75-dwc3
> + - qcom,sm4250-dwc3
> + - qcom,sm6115-dwc3
> + - qcom,sm6125-dwc3
> + - qcom,sm6350-dwc3
> + - qcom,sm6375-dwc3
> + - qcom,sm8150-dwc3
> + - qcom,sm8250-dwc3
> + - qcom,sm8350-dwc3
> + - qcom,sm8450-dwc3
> + - qcom,sm8550-dwc3
This enum could be replaced with '{}'. Alternatively, drop enum entire
select replaced with:
- contains
- items:
- const: qcom,dwc3
- const: snps,dwc3
> + - const: qcom,dwc3
> + - const: snps,dwc3
> + required:
> + - compatible
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - qcom,ipq4019-dwc3
> + - qcom,ipq5018-dwc3
> + - qcom,ipq5332-dwc3
> + - qcom,ipq6018-dwc3
> + - qcom,ipq8064-dwc3
> + - qcom,ipq8074-dwc3
> + - qcom,ipq9574-dwc3
> + - qcom,msm8953-dwc3
> + - qcom,msm8994-dwc3
> + - qcom,msm8996-dwc3
> + - qcom,msm8998-dwc3
> + - qcom,qcm2290-dwc3
> + - qcom,qcs404-dwc3
> + - qcom,sa8775p-dwc3
> + - qcom,sc7180-dwc3
> + - qcom,sc7280-dwc3
> + - qcom,sc8180x-dwc3
> + - qcom,sc8280xp-dwc3
> + - qcom,sc8280xp-dwc3-mp
> + - qcom,sdm660-dwc3
> + - qcom,sdm670-dwc3
> + - qcom,sdm845-dwc3
> + - qcom,sdx55-dwc3
> + - qcom,sdx65-dwc3
> + - qcom,sdx75-dwc3
> + - qcom,sm4250-dwc3
> + - qcom,sm6115-dwc3
> + - qcom,sm6125-dwc3
> + - qcom,sm6350-dwc3
> + - qcom,sm6375-dwc3
> + - qcom,sm8150-dwc3
> + - qcom,sm8250-dwc3
> + - qcom,sm8350-dwc3
> + - qcom,sm8450-dwc3
> + - qcom,sm8550-dwc3
> + - const: qcom,dwc3
> + - const: snps,dwc3
> +
> + reg:
> + description: Offset and length of register set for QSCRATCH wrapper
> + maxItems: 1
> +
> + power-domains:
> + description: specifies a phandle to PM domain provider node
Drop description
> + maxItems: 1
> +
> + required-opps:
> + maxItems: 1
> +
> + clocks:
> + description: |
> + Several clocks are used, depending on the variant. Typical ones are::
> + - cfg_noc:: System Config NOC clock.
> + - core:: Master/Core clock, has to be >= 125 MHz for SS operation and >=
> + 60MHz for HS operation.
> + - iface:: System bus AXI clock.
> + - sleep:: Sleep clock, used for wakeup when USB3 core goes into low
> + power mode (U3).
> + - mock_utmi:: Mock utmi clock needed for ITP/SOF generation in host
> + mode. Its frequency should be 19.2MHz.
> + minItems: 1
> + maxItems: 9
> +
> + clock-names:
> + minItems: 1
> + maxItems: 9
> +
> + resets:
> + maxItems: 1
> +
> + interconnects:
> + maxItems: 2
> +
> + interconnect-names:
> + items:
> + - const: usb-ddr
> + - const: apps-usb
> +
> + interrupts:
> + minItems: 2
> + maxItems: 5
> +
> + interrupt-names:
> + minItems: 2
> + maxItems: 5
> +
> + qcom,select-utmi-as-pipe-clk:
> + description:
> + If present, disable USB3 pipe_clk requirement.
> + Used when dwc3 operates without SSPHY and only
> + HS/FS/LS modes are supported.
> + type: boolean
> +
> + wakeup-source: true
> +
> +# Required child node:
Drop
...
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index d81c2e849ca9..d6914b8cef6a 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -44,14 +44,18 @@ properties:
> It's either a single common DWC3 interrupt (dwc_usb3) or individual
> interrupts for the host, gadget and DRD modes.
> minItems: 1
> - maxItems: 4
> + maxItems: 5
>
> interrupt-names:
> - minItems: 1
> - maxItems: 4
> oneOf:
> - - const: dwc_usb3
> - - items:
> + - minItems: 1
> + maxItems: 5
> + items:
> + - const: dwc_usb3
> + additionalItems: true
This is not correct change. Before, one dwc_usb3 interrupt was combined
allowed, or a set of host+peripheral+otg+wakeup. Now, you allow combined
dwc_usb3 with anything.
Best regards,
Krzysztof
On 10/17/23 05:11, Bjorn Andersson wrote:
> In preparation for the introduction of a direct reference to the struct
> dwc3 in the dwc3_qcom struct, rename the generically named "dwc3" to
> reduce the risk for confusion.
>
> No functional change.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
On 10/17/23 05:11, Bjorn Andersson wrote:
> In the case that the dwc3 core driver is instantiated from the same
> memory resource information as the glue driver, the dwc_res memory
> region will overlap with the memory region already mapped by the glue.
>
> As the DWC3 core driver already does math on the passed memory region to
> exclude the XHCI region, also adjust the end address, to avoid having to
> pass an adjusted region from the glue explicitly.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
Acked-by: Konrad Dybcio <[email protected]>
Konrad
On 10/17/23 05:11, Bjorn Andersson wrote:
> The two constants for the offset and size of the qscratch block within
> the DWC3 region has the same value in all supported variants, so they
> don't necessarily need to be picked dynamically.
>
> By replacing the lookup with the constants it's possible to reuse the
> same code path without the ACPI pdata structure in the upcoming commit.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
If we don't want more non-flagship-SoC-based post-sdm845 ACPI-supported
platforms, using just one of N USB controllers
which I am very happy that we don't
so much so that I'd be even happy to drop this acpi thing altogether
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
On Mon, Oct 16, 2023 at 08:11:18PM -0700, Bjorn Andersson wrote:
> The Qualcomm USB block consists of three intertwined parts, the XHCI,
> the DWC3 core and the Qualcomm DWC3 glue. The three parts can not be
> operated independently, but the binding was for historical reasons split
> to mimic the Linux driver implementation.
>
> The split binding also makes it hard to alter the implementation, as
> properties and resources are split between the two nodes, in some cases
> with some duplication.
>
> Introduce a new binding, with a single representation of the whole USB
> block in one node.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> .../devicetree/bindings/usb/qcom,dwc3.yaml | 482 +++++++++++++++++++++
> .../devicetree/bindings/usb/snps,dwc3.yaml | 14 +-
> 2 files changed, 491 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> new file mode 100644
> index 000000000000..cb50261c6a36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -0,0 +1,482 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SuperSpeed DWC3 USB SoC controller
> +
> +maintainers:
> + - Wesley Cheng <[email protected]>
> +
> +select:
> + properties:
> + compatible:
> + items:
> + - enum:
> + - qcom,ipq4019-dwc3
> + - qcom,ipq5018-dwc3
> + - qcom,ipq5332-dwc3
> + - qcom,ipq6018-dwc3
> + - qcom,ipq8064-dwc3
> + - qcom,ipq8074-dwc3
> + - qcom,ipq9574-dwc3
> + - qcom,msm8953-dwc3
> + - qcom,msm8994-dwc3
> + - qcom,msm8996-dwc3
> + - qcom,msm8998-dwc3
> + - qcom,qcm2290-dwc3
> + - qcom,qcs404-dwc3
> + - qcom,sa8775p-dwc3
> + - qcom,sc7180-dwc3
> + - qcom,sc7280-dwc3
> + - qcom,sc8180x-dwc3
> + - qcom,sc8280xp-dwc3
> + - qcom,sc8280xp-dwc3-mp
> + - qcom,sdm660-dwc3
> + - qcom,sdm670-dwc3
> + - qcom,sdm845-dwc3
> + - qcom,sdx55-dwc3
> + - qcom,sdx65-dwc3
> + - qcom,sdx75-dwc3
> + - qcom,sm4250-dwc3
> + - qcom,sm6115-dwc3
> + - qcom,sm6125-dwc3
> + - qcom,sm6350-dwc3
> + - qcom,sm6375-dwc3
> + - qcom,sm8150-dwc3
> + - qcom,sm8250-dwc3
> + - qcom,sm8350-dwc3
> + - qcom,sm8450-dwc3
> + - qcom,sm8550-dwc3
> + - const: qcom,dwc3
> + - const: snps,dwc3
> + required:
> + - compatible
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - qcom,ipq4019-dwc3
> + - qcom,ipq5018-dwc3
> + - qcom,ipq5332-dwc3
> + - qcom,ipq6018-dwc3
> + - qcom,ipq8064-dwc3
> + - qcom,ipq8074-dwc3
> + - qcom,ipq9574-dwc3
> + - qcom,msm8953-dwc3
> + - qcom,msm8994-dwc3
> + - qcom,msm8996-dwc3
> + - qcom,msm8998-dwc3
> + - qcom,qcm2290-dwc3
> + - qcom,qcs404-dwc3
> + - qcom,sa8775p-dwc3
> + - qcom,sc7180-dwc3
> + - qcom,sc7280-dwc3
> + - qcom,sc8180x-dwc3
> + - qcom,sc8280xp-dwc3
> + - qcom,sc8280xp-dwc3-mp
> + - qcom,sdm660-dwc3
> + - qcom,sdm670-dwc3
> + - qcom,sdm845-dwc3
> + - qcom,sdx55-dwc3
> + - qcom,sdx65-dwc3
> + - qcom,sdx75-dwc3
> + - qcom,sm4250-dwc3
> + - qcom,sm6115-dwc3
> + - qcom,sm6125-dwc3
> + - qcom,sm6350-dwc3
> + - qcom,sm6375-dwc3
> + - qcom,sm8150-dwc3
> + - qcom,sm8250-dwc3
> + - qcom,sm8350-dwc3
> + - qcom,sm8450-dwc3
> + - qcom,sm8550-dwc3
> + - const: qcom,dwc3
> + - const: snps,dwc3
> +
> + reg:
> + description: Offset and length of register set for QSCRATCH wrapper
> + maxItems: 1
Isn't this more things now? Or the description is wrong.
Rob
On Tue, Oct 17, 2023 at 01:31:36PM -0500, Rob Herring wrote:
> On Mon, Oct 16, 2023 at 08:11:18PM -0700, Bjorn Andersson wrote:
> > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
[..]
> > + reg:
> > + description: Offset and length of register set for QSCRATCH wrapper
> > + maxItems: 1
>
> Isn't this more things now? Or the description is wrong.
>
The description is wrong, the single cell is now intended to cover the
whole USB block, which spans XHCI, DWC3 and Qualcomm glue parts.
Regards,
Bjorn
On Tue, Oct 17, 2023 at 08:11:45AM +0200, Krzysztof Kozlowski wrote:
> On 17/10/2023 05:11, Bjorn Andersson wrote:
> > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
[..]
> > +select:
> > + properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - qcom,ipq4019-dwc3
[..]
> > + - qcom,sm8550-dwc3
>
> This enum could be replaced with '{}'. Alternatively, drop enum entire
> select replaced with:
> - contains
> - items:
> - const: qcom,dwc3
> - const: snps,dwc3
>
I thought this would be what I needed as well, but unfortunately this
select matches either qcom,dwc3, snps,dwc3, or both. With the result
that e.g. the example in the snps,dwc3 binding matches against this and
as expected fails when validated against this binding.
Taking yet another look at this, and reading more about json validation
I figured out that the following matches nodes with both the
compatibles:
select:
properties:
compatible:
items:
- const: qcom,dwc3
- const: snps,dwc3
required:
- compatible
[..]
> > +
> > +# Required child node:
>
> Drop
>
Of course.
>
> ...
>
> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > index d81c2e849ca9..d6914b8cef6a 100644
> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > @@ -44,14 +44,18 @@ properties:
> > It's either a single common DWC3 interrupt (dwc_usb3) or individual
> > interrupts for the host, gadget and DRD modes.
> > minItems: 1
> > - maxItems: 4
> > + maxItems: 5
> >
> > interrupt-names:
> > - minItems: 1
> > - maxItems: 4
> > oneOf:
> > - - const: dwc_usb3
> > - - items:
> > + - minItems: 1
> > + maxItems: 5
> > + items:
> > + - const: dwc_usb3
> > + additionalItems: true
>
> This is not correct change. Before, one dwc_usb3 interrupt was combined
> allowed, or a set of host+peripheral+otg+wakeup. Now, you allow combined
> dwc_usb3 with anything.
>
My intention here is to make below list of 5 strings be valid according
to the snps,dwc3 (i.e. dwc_usb3 being the first item), and valid
according to the qcom,dwc3 binding with all 5 defined.
interrupt-names = "dwc_usb3", "hs_phy_irq", "ss_phy_irq",
"dm_hs_phy_irq", "dp_hs_phy_irq";
When I express this as:
interrupt-names:
minItems: 1
maxItems: 5
oneOf:
- const: dwc_usb3
- items:
enum: [host, peripheral, otg, wakeup]
I get:
/local/mnt/workspace/bjorande/linux/Documentation/devicetree/bindings/usb/qcom,dwc3.example.dtb: usb@a600000: interrupt-names: 'oneOf' conditional failed, one must be fixed:
['dwc_usb3', 'hs_phy_irq', 'ss_phy_irq', 'dm_hs_phy_irq', 'dp_hs_phy_irq'] is too long
'dwc_usb3' is not one of ['host', 'peripheral', 'otg', 'wakeup']
'hs_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
'ss_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
'dm_hs_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
'dp_hs_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
from schema $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml#
Which to me sounds like the two oneOf branches allow me a single entry,
or items from the set given here. In contrast, I believe that my
proposal allow 1-5 items, where the first needs to be dwc_usb3.
But the proposal does look messy, so I'd appreciate some guidance on
this one.
Thanks,
Bjorn
On 18/10/2023 00:54, Bjorn Andersson wrote:
> On Tue, Oct 17, 2023 at 08:11:45AM +0200, Krzysztof Kozlowski wrote:
>> On 17/10/2023 05:11, Bjorn Andersson wrote:
>>> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> [..]
>>> +select:
>>> + properties:
>>> + compatible:
>>> + items:
>>> + - enum:
>>> + - qcom,ipq4019-dwc3
> [..]
>>> + - qcom,sm8550-dwc3
>>
>> This enum could be replaced with '{}'. Alternatively, drop enum entire
>> select replaced with:
>> - contains
>> - items:
>> - const: qcom,dwc3
>> - const: snps,dwc3
>>
>
> I thought this would be what I needed as well, but unfortunately this
> select matches either qcom,dwc3, snps,dwc3, or both. With the result
> that e.g. the example in the snps,dwc3 binding matches against this and
> as expected fails when validated against this binding.
>
> Taking yet another look at this, and reading more about json validation
> I figured out that the following matches nodes with both the
> compatibles:
>
> select:
> properties:
> compatible:
> items:
> - const: qcom,dwc3
> - const: snps,dwc3
> required:
> - compatible
>
> [..]
>>> +
>>> +# Required child node:
>>
>> Drop
>>
>
> Of course.
>
>>
>> ...
>>
>>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> index d81c2e849ca9..d6914b8cef6a 100644
>>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> @@ -44,14 +44,18 @@ properties:
>>> It's either a single common DWC3 interrupt (dwc_usb3) or individual
>>> interrupts for the host, gadget and DRD modes.
>>> minItems: 1
>>> - maxItems: 4
>>> + maxItems: 5
>>>
>>> interrupt-names:
>>> - minItems: 1
>>> - maxItems: 4
>>> oneOf:
>>> - - const: dwc_usb3
>>> - - items:
>>> + - minItems: 1
>>> + maxItems: 5
>>> + items:
>>> + - const: dwc_usb3
>>> + additionalItems: true
>>
>> This is not correct change. Before, one dwc_usb3 interrupt was combined
>> allowed, or a set of host+peripheral+otg+wakeup. Now, you allow combined
>> dwc_usb3 with anything.
>>
>
> My intention here is to make below list of 5 strings be valid according
> to the snps,dwc3 (i.e. dwc_usb3 being the first item), and valid
> according to the qcom,dwc3 binding with all 5 defined.
>
> interrupt-names = "dwc_usb3", "hs_phy_irq", "ss_phy_irq",
> "dm_hs_phy_irq", "dp_hs_phy_irq";
>
> When I express this as:
>
> interrupt-names:
> minItems: 1
> maxItems: 5
> oneOf:
> - const: dwc_usb3
> - items:
> enum: [host, peripheral, otg, wakeup]
>
> I get:
>
> /local/mnt/workspace/bjorande/linux/Documentation/devicetree/bindings/usb/qcom,dwc3.example.dtb: usb@a600000: interrupt-names: 'oneOf' conditional failed, one must be fixed:
> ['dwc_usb3', 'hs_phy_irq', 'ss_phy_irq', 'dm_hs_phy_irq', 'dp_hs_phy_irq'] is too long
> 'dwc_usb3' is not one of ['host', 'peripheral', 'otg', 'wakeup']
> 'hs_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
> 'ss_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
> 'dm_hs_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
> 'dp_hs_phy_irq' is not one of ['host', 'peripheral', 'otg', 'wakeup']
> from schema $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml#
>
> Which to me sounds like the two oneOf branches allow me a single entry,
> or items from the set given here. In contrast, I believe that my
> proposal allow 1-5 items, where the first needs to be dwc_usb3.
>
> But the proposal does look messy, so I'd appreciate some guidance on
> this one.
Then you need three branches I guess, with your branch listing the Qcom
specific interrupts. The point is that dwc_usb3 should not be allowed
with host/peripheral/otg/wakeup.
Best regards,
Krzysztof
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/[email protected]/config)
compiler: csky-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231020/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Bjorn,
On Mon, Oct 16, 2023, Bjorn Andersson wrote:
> 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.
>
> 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.
>
> Furhtermore, the DeviceTree binding is a direct representation of the
> Linux driver model, and doesn't necessarily describe "the USB IP-block".
>
> This series therefor attempts to flatten the driver split, and operate
> the glue and core out of the same platform_device instance. And in order
> to do this, the DeviceTree representation of the IP block is flattened.
>
> As a side effect, much of the ACPI integration code is dropped.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> Bjorn Andersson (12):
> dt-bindings: usb: qcom,dwc3: Add qcom,sc8180x-dwc3
> usb: dwc3: qcom: Rename dwc3 platform_device reference
> usb: dwc3: qcom: Merge resources from urs_usb device
> usb: dwc3: Expose core driver as library
> usb: dwc3: Override end of dwc3 memory resource
> usb: dwc3: qcom: Add dwc3 core reference in driver state
> usb: dwc3: qcom: Instantiate dwc3 core directly
> usb: dwc3: qcom: Inline the qscratch constants
> dt-bindings: usb: qcom,dwc3: Rename to "glue"
> dt-bindings: usb: qcom,dwc3: Introduce flattened qcom,dwc3 binding
> usb: dwc3: qcom: Flatten the Qualcomm dwc3 binding and implementation
> arm64: dts: qcom: sc8180x: flatten usb_sec node
>
> .../devicetree/bindings/usb/qcom,dwc3-glue.yaml | 626 +++++++++++++++++++++
> .../devicetree/bindings/usb/qcom,dwc3.yaml | 321 ++++-------
> .../devicetree/bindings/usb/snps,dwc3.yaml | 14 +-
> .../arm64/boot/dts/qcom/sc8180x-lenovo-flex-5g.dts | 6 +-
> arch/arm64/boot/dts/qcom/sc8180x-primus.dts | 6 +-
> arch/arm64/boot/dts/qcom/sc8180x.dtsi | 34 +-
> drivers/usb/dwc3/core.c | 136 +++--
> drivers/usb/dwc3/core.h | 10 +
> drivers/usb/dwc3/dwc3-qcom.c | 328 ++++++-----
> 9 files changed, 1057 insertions(+), 424 deletions(-)
> ---
> base-commit: 4d0515b235dec789578d135a5db586b25c5870cb
> change-id: 20231016-dwc3-refactor-931e3b08a8b9
>
> Best regards,
> --
> Bjorn Andersson <[email protected]>
>
First of all, thanks for the work.
I just did a quick review through the changes, and I think it's great!
(This will address some issues I also have with dwc3 currently too.)
BR,
Thinh
On Mon, Oct 16, 2023, Bjorn Andersson wrote:
> In the case that the dwc3 core driver is instantiated from the same
> memory resource information as the glue driver, the dwc_res memory
> region will overlap with the memory region already mapped by the glue.
>
> As the DWC3 core driver already does math on the passed memory region to
> exclude the XHCI region, also adjust the end address, to avoid having to
> pass an adjusted region from the glue explicitly.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/usb/dwc3/core.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 71e376bebb16..5d86b803fab0 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1908,6 +1908,7 @@ struct dwc3 *dwc3_probe(struct platform_device *pdev)
> */
> dwc_res = *res;
> dwc_res.start += DWC3_GLOBALS_REGS_START;
> + dwc_res.end = res->start + DWC3_OTG_REGS_END;
DWC3_OTG_REGS_END shouldn't really be the end. What offset does qcom
start overlapping?
Can the end be 0xda00 for now? (This can change)
BR,
Thinh
>
> if (dev->of_node) {
> struct device_node *parent = of_get_parent(dev->of_node);
> @@ -1915,6 +1916,7 @@ struct dwc3 *dwc3_probe(struct platform_device *pdev)
> if (of_device_is_compatible(parent, "realtek,rtd-dwc3")) {
> dwc_res.start -= DWC3_GLOBALS_REGS_START;
> dwc_res.start += DWC3_RTK_RTD_GLOBALS_REGS_START;
> + dwc_res.end = dwc_res.start + DWC3_OTG_REGS_END;
> }
>
> of_node_put(parent);
>
> --
> 2.25.1
>
On Mon, Oct 16, 2023, Bjorn Andersson wrote:
> The DWC3 IP block is handled by three distinct device drivers: XHCI,
> DWC3 core and a platform specific (optional) DWC3 glue driver.
>
> This has resulted in, at least in the case of the Qualcomm glue, the
> presence of a number of layering violations, where the glue code either
> can't handle, or has to work around, the fact that core might not probe
> deterministically.
>
> An example of this is that the suspend path should operate slightly
> different depending on the device operating in host or peripheral mode,
> and the only way to determine the operating state is to peek into the
> core's drvdata.
>
> The Qualcomm glue driver is expected to make updates in the qscratch
> register region (the "glue" region) during role switch events, but with
> the glue and core split using the driver model, there is no reasonable
> way to introduce listeners for mode changes.
>
> Split the dwc3 core platfrom_driver callbacks and their implementation
> and export the implementation, to make it possible to deterministically
> instantiate the dwc3 core as part of the dwc3 glue drivers and to
> allow flattening of the DeviceTree representation.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/usb/dwc3/core.c | 134 ++++++++++++++++++++++++++++++++----------------
> drivers/usb/dwc3/core.h | 10 ++++
> 2 files changed, 100 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index d25490965b27..71e376bebb16 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1876,7 +1876,7 @@ static int dwc3_get_clocks(struct dwc3 *dwc)
> return 0;
> }
>
> -static int dwc3_probe(struct platform_device *pdev)
> +struct dwc3 *dwc3_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct resource *res, dwc_res;
> @@ -1886,14 +1886,14 @@ static int dwc3_probe(struct platform_device *pdev)
>
> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
> if (!dwc)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> dwc->dev = dev;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res) {
> dev_err(dev, "missing memory resource\n");
> - return -ENODEV;
> + return ERR_PTR(-ENODEV);
> }
>
> dwc->xhci_resources[0].start = res->start;
> @@ -1922,7 +1922,7 @@ static int dwc3_probe(struct platform_device *pdev)
>
> regs = devm_ioremap_resource(dev, &dwc_res);
> if (IS_ERR(regs))
> - return PTR_ERR(regs);
> + return ERR_CAST(regs);
>
> dwc->regs = regs;
> dwc->regs_size = resource_size(&dwc_res);
> @@ -1953,7 +1953,6 @@ static int dwc3_probe(struct platform_device *pdev)
> goto err_disable_clks;
> }
>
> - platform_set_drvdata(pdev, dwc);
> dwc3_cache_hwparams(dwc);
>
> if (!dwc->sysdev_is_parent &&
> @@ -2006,7 +2005,7 @@ static int dwc3_probe(struct platform_device *pdev)
>
> pm_runtime_put(dev);
>
> - return 0;
> + return dwc;
>
> err_exit_debugfs:
> dwc3_debugfs_exit(dwc);
> @@ -2030,14 +2029,26 @@ static int dwc3_probe(struct platform_device *pdev)
> if (dwc->usb_psy)
> power_supply_put(dwc->usb_psy);
>
> - return ret;
> + return ERR_PTR(ret);
> }
> +EXPORT_SYMBOL_GPL(dwc3_probe);
>
> -static void dwc3_remove(struct platform_device *pdev)
> +static int dwc3_plat_probe(struct platform_device *pdev)
> {
> - struct dwc3 *dwc = platform_get_drvdata(pdev);
> + struct dwc3 *dwc;
> +
> + dwc = dwc3_probe(pdev);
> + if (IS_ERR(dwc))
> + return PTR_ERR(dwc);
> +
> + platform_set_drvdata(pdev, dwc);
> +
> + return 0;
> +}
>
> - pm_runtime_get_sync(&pdev->dev);
> +void dwc3_remove(struct dwc3 *dwc)
> +{
> + pm_runtime_get_sync(dwc->dev);
>
> dwc3_core_exit_mode(dwc);
> dwc3_debugfs_exit(dwc);
> @@ -2045,22 +2056,28 @@ static void dwc3_remove(struct platform_device *pdev)
> dwc3_core_exit(dwc);
> dwc3_ulpi_exit(dwc);
>
> - pm_runtime_allow(&pdev->dev);
> - pm_runtime_disable(&pdev->dev);
> - pm_runtime_dont_use_autosuspend(&pdev->dev);
> - pm_runtime_put_noidle(&pdev->dev);
> + pm_runtime_allow(dwc->dev);
> + pm_runtime_disable(dwc->dev);
> + pm_runtime_dont_use_autosuspend(dwc->dev);
> + pm_runtime_put_noidle(dwc->dev);
> /*
> * HACK: Clear the driver data, which is currently accessed by parent
> * glue drivers, before allowing the parent to suspend.
> */
> - platform_set_drvdata(pdev, NULL);
> - pm_runtime_set_suspended(&pdev->dev);
> + dev_set_drvdata(dwc->dev, NULL);
> + pm_runtime_set_suspended(dwc->dev);
>
> dwc3_free_event_buffers(dwc);
>
> if (dwc->usb_psy)
> power_supply_put(dwc->usb_psy);
> }
> +EXPORT_SYMBOL_GPL(dwc3_remove);
> +
> +static void dwc3_plat_remove(struct platform_device *pdev)
> +{
> + dwc3_remove(platform_get_drvdata(pdev));
> +}
>
> #ifdef CONFIG_PM
> static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> @@ -2227,9 +2244,8 @@ static int dwc3_runtime_checks(struct dwc3 *dwc)
> return 0;
> }
>
> -static int dwc3_runtime_suspend(struct device *dev)
> +int dwc3_runtime_suspend(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
> int ret;
>
> if (dwc3_runtime_checks(dwc))
> @@ -2241,10 +2257,10 @@ static int dwc3_runtime_suspend(struct device *dev)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(dwc3_runtime_suspend);
>
> -static int dwc3_runtime_resume(struct device *dev)
> +int dwc3_runtime_resume(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
> int ret;
>
> ret = dwc3_resume_common(dwc, PMSG_AUTO_RESUME);
> @@ -2261,15 +2277,14 @@ static int dwc3_runtime_resume(struct device *dev)
> break;
> }
>
> - pm_runtime_mark_last_busy(dev);
> + pm_runtime_mark_last_busy(dwc->dev);
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(dwc3_runtime_resume);
>
> -static int dwc3_runtime_idle(struct device *dev)
> +int dwc3_runtime_idle(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
> -
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_DEVICE:
> if (dwc3_runtime_checks(dwc))
> @@ -2281,49 +2296,64 @@ static int dwc3_runtime_idle(struct device *dev)
> break;
> }
>
> - pm_runtime_mark_last_busy(dev);
> - pm_runtime_autosuspend(dev);
> + pm_runtime_mark_last_busy(dwc->dev);
> + pm_runtime_autosuspend(dwc->dev);
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(dwc3_runtime_idle);
> +
> +static int dwc3_plat_runtime_suspend(struct device *dev)
> +{
> + return dwc3_runtime_suspend(dev_get_drvdata(dev));
> +}
> +
> +static int dwc3_plat_runtime_resume(struct device *dev)
> +{
> + return dwc3_runtime_resume(dev_get_drvdata(dev));
> +}
> +
> +static int dwc3_plat_runtime_idle(struct device *dev)
> +{
> + return dwc3_runtime_idle(dev_get_drvdata(dev));
> +}
> #endif /* CONFIG_PM */
>
> #ifdef CONFIG_PM_SLEEP
> -static int dwc3_suspend(struct device *dev)
> +int dwc3_suspend(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
> int ret;
>
> ret = dwc3_suspend_common(dwc, PMSG_SUSPEND);
> if (ret)
> return ret;
>
> - pinctrl_pm_select_sleep_state(dev);
> + pinctrl_pm_select_sleep_state(dwc->dev);
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(dwc3_suspend);
>
> -static int dwc3_resume(struct device *dev)
> +int dwc3_resume(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
> int ret;
>
> - pinctrl_pm_select_default_state(dev);
> + pinctrl_pm_select_default_state(dwc->dev);
>
> ret = dwc3_resume_common(dwc, PMSG_RESUME);
> if (ret)
> return ret;
>
> - pm_runtime_disable(dev);
> - pm_runtime_set_active(dev);
> - pm_runtime_enable(dev);
> + pm_runtime_disable(dwc->dev);
> + pm_runtime_set_active(dwc->dev);
> + pm_runtime_enable(dwc->dev);
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(dwc3_resume);
>
> -static void dwc3_complete(struct device *dev)
> +void dwc3_complete(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
> u32 reg;
>
> if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST &&
> @@ -2333,15 +2363,31 @@ static void dwc3_complete(struct device *dev)
> dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
> }
> }
> +EXPORT_SYMBOL_GPL(dwc3_complete);
> +
> +static int dwc3_plat_suspend(struct device *dev)
> +{
> + return dwc3_suspend(dev_get_drvdata(dev));
> +}
> +
> +static int dwc3_plat_resume(struct device *dev)
> +{
> + return dwc3_resume(dev_get_drvdata(dev));
> +}
> +
> +static void dwc3_plat_complete(struct device *dev)
> +{
> + dwc3_complete(dev_get_drvdata(dev));
> +}
> #else
> -#define dwc3_complete NULL
> +#define dwc3_plat_complete NULL
> #endif /* CONFIG_PM_SLEEP */
>
> static const struct dev_pm_ops dwc3_dev_pm_ops = {
> - SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
> - .complete = dwc3_complete,
> - SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume,
> - dwc3_runtime_idle)
> + SET_SYSTEM_SLEEP_PM_OPS(dwc3_plat_suspend, dwc3_plat_resume)
> + .complete = dwc3_plat_complete,
> + SET_RUNTIME_PM_OPS(dwc3_plat_runtime_suspend, dwc3_plat_runtime_resume,
> + dwc3_plat_runtime_idle)
> };
>
> #ifdef CONFIG_OF
> @@ -2369,8 +2415,8 @@ MODULE_DEVICE_TABLE(acpi, dwc3_acpi_match);
> #endif
>
> static struct platform_driver dwc3_driver = {
> - .probe = dwc3_probe,
> - .remove_new = dwc3_remove,
> + .probe = dwc3_plat_probe,
> + .remove_new = dwc3_plat_remove,
> .driver = {
> .name = "dwc3",
> .of_match_table = of_match_ptr(of_dwc3_match),
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index c6c87acbd376..f5e22559bb73 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1568,6 +1568,16 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc);
>
> int dwc3_core_soft_reset(struct dwc3 *dwc);
>
> +struct dwc3 *dwc3_probe(struct platform_device *pdev);
> +void dwc3_remove(struct dwc3 *dwc);
> +
> +int dwc3_runtime_suspend(struct dwc3 *dwc);
> +int dwc3_runtime_resume(struct dwc3 *dwc);
> +int dwc3_runtime_idle(struct dwc3 *dwc);
> +int dwc3_suspend(struct dwc3 *dwc);
> +int dwc3_resume(struct dwc3 *dwc);
> +void dwc3_complete(struct dwc3 *dwc);
> +
Can we make this new interface clear? It's being buried under core.h
this way. Perhaps add some comments and divider, or move to another
header file?
Thanks,
Thinh
> #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
> int dwc3_host_init(struct dwc3 *dwc);
> void dwc3_host_exit(struct dwc3 *dwc);
>
> --
> 2.25.1
>
On Mon, Oct 16, 2023 at 08:11:08PM -0700, Bjorn Andersson wrote:
> 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.
>
> 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.
Nice to see this finally being worked on. It's not clear why mode-change
notifications would be a problem though, as if you get such a
notification, you know that core has been probed.
> Furhtermore, the DeviceTree binding is a direct representation of the
> Linux driver model, and doesn't necessarily describe "the USB IP-block".
True.
Johan
On Mon, Oct 16, 2023 at 08:11:10PM -0700, Bjorn Andersson wrote:
> In preparation for the introduction of a direct reference to the struct
> dwc3 in the dwc3_qcom struct, rename the generically named "dwc3" to
> reduce the risk for confusion.
>
> No functional change.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 46 ++++++++++++++++++++++----------------------
> 1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 3de43df6bbe8..a31c3bc1f56e 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -67,7 +67,7 @@ struct dwc3_acpi_pdata {
> struct dwc3_qcom {
> struct device *dev;
> void __iomem *qscratch_base;
> - struct platform_device *dwc3;
> + struct platform_device *dwc_dev;
Since "dev" is so overloaded, please name this one "dwc_pdev" instead.
> struct platform_device *urs_usb;
> struct clk **clks;
> int num_clocks;
> static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> {
> - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc_dev);
> struct usb_device *udev;
> struct usb_hcd __maybe_unused *hcd;
>
> @@ -486,7 +486,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 = platform_get_drvdata(qcom->dwc_dev);
Perhaps you can drop the tab while changing this line.
Johan
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/[email protected]/
> + 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 Mon, Oct 16, 2023 at 08:11:12PM -0700, Bjorn Andersson wrote:
> The DWC3 IP block is handled by three distinct device drivers: XHCI,
> DWC3 core and a platform specific (optional) DWC3 glue driver.
>
> This has resulted in, at least in the case of the Qualcomm glue, the
> presence of a number of layering violations, where the glue code either
> can't handle, or has to work around, the fact that core might not probe
> deterministically.
>
> An example of this is that the suspend path should operate slightly
> different depending on the device operating in host or peripheral mode,
> and the only way to determine the operating state is to peek into the
> core's drvdata.
>
> The Qualcomm glue driver is expected to make updates in the qscratch
> register region (the "glue" region) during role switch events, but with
> the glue and core split using the driver model, there is no reasonable
> way to introduce listeners for mode changes.
>
> Split the dwc3 core platfrom_driver callbacks and their implementation
> and export the implementation, to make it possible to deterministically
> instantiate the dwc3 core as part of the dwc3 glue drivers and to
> allow flattening of the DeviceTree representation.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/usb/dwc3/core.c | 134 ++++++++++++++++++++++++++++++++----------------
> drivers/usb/dwc3/core.h | 10 ++++
> 2 files changed, 100 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index d25490965b27..71e376bebb16 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1876,7 +1876,7 @@ static int dwc3_get_clocks(struct dwc3 *dwc)
> return 0;
> }
>
> -static int dwc3_probe(struct platform_device *pdev)
> +struct dwc3 *dwc3_probe(struct platform_device *pdev)
Perhaps you should move allocation of struct dwc3 to the caller as well
as you are going to need some way to pass in callback to core which need
to be set before you register the xhci platform device.
There could be other ways, like passing in a struct of callbacks, which
can be added incrementally but it may be good think this through from
the start.
> {
> struct device *dev = &pdev->dev;
> struct resource *res, dwc_res;
> @@ -1886,14 +1886,14 @@ static int dwc3_probe(struct platform_device *pdev)
>
> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
> if (!dwc)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> dwc->dev = dev;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res) {
> dev_err(dev, "missing memory resource\n");
> - return -ENODEV;
> + return ERR_PTR(-ENODEV);
> }
>
> dwc->xhci_resources[0].start = res->start;
> @@ -1922,7 +1922,7 @@ static int dwc3_probe(struct platform_device *pdev)
>
> regs = devm_ioremap_resource(dev, &dwc_res);
> if (IS_ERR(regs))
> - return PTR_ERR(regs);
> + return ERR_CAST(regs);
>
> dwc->regs = regs;
> dwc->regs_size = resource_size(&dwc_res);
> @@ -1953,7 +1953,6 @@ static int dwc3_probe(struct platform_device *pdev)
> goto err_disable_clks;
> }
>
> - platform_set_drvdata(pdev, dwc);
This is broken however as the pm ops access the data driver data and can
be called as soon as you call pm_runtime_put() below.
> dwc3_cache_hwparams(dwc);
>
> if (!dwc->sysdev_is_parent &&
> @@ -2006,7 +2005,7 @@ static int dwc3_probe(struct platform_device *pdev)
>
> pm_runtime_put(dev);
That is here.
> - return 0;
> + return dwc;
> -static void dwc3_remove(struct platform_device *pdev)
> +static int dwc3_plat_probe(struct platform_device *pdev)
> {
> - struct dwc3 *dwc = platform_get_drvdata(pdev);
> + struct dwc3 *dwc;
> +
> + dwc = dwc3_probe(pdev);
> + if (IS_ERR(dwc))
> + return PTR_ERR(dwc);
And that leaves a window, for example, here where you can hit a NULL
pointer dereference.
> + platform_set_drvdata(pdev, dwc);
> +
> + return 0;
> +}
Johan
On Mon, Oct 16, 2023 at 08:11:14PM -0700, Bjorn Andersson wrote:
> In the coming changes the Qualcomm DWC3 glue will be able to either
> manage the DWC3 core as a child platform_device, or directly instantiate
> it within its own context.
>
> Introduce a reference to the dwc3 core state and make the driver
> reference the dwc3 core either the child device or this new reference.
>
> As the new member isn't assigned, and qcom->dwc_dev is assigned in all
> current cases, the change should have no functional impact.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 100 +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 83 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 7c810712d246..901e5050363b 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -67,7 +67,8 @@ struct dwc3_acpi_pdata {
> struct dwc3_qcom {
> struct device *dev;
> void __iomem *qscratch_base;
> - struct platform_device *dwc_dev;
> + struct platform_device *dwc_dev; /* only used when core is separate device */
> + struct dwc3 *dwc; /* not used when core is separate device */
Hmm. This quickly become really messy and hard to maintain. It may be
fine as an intermediate step as part of this series, but why can't you
do the conversion fully so that the Qualcomm glue driver never registers
a core platform device? Is it just about where the core driver looks for
DT properties?
Johan
On Mon, Oct 16, 2023 at 08:11:15PM -0700, Bjorn Andersson wrote:
> The Qualcomm DWC3 glue builds up a platform_device programmatically in
> order to probe the DWC3 core when running off ACPI data. But with the
> newly introduced support for instantiating the core directly from the
> glue, this code can be replaced with a single function call.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> @@ -986,10 +933,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> interconnect_exit:
> dwc3_qcom_interconnect_exit(qcom);
> depopulate:
> - if (np)
> + if (qcom->dwc_dev)
> of_platform_depopulate(&pdev->dev);
> else
> - platform_device_put(pdev);
> + dwc3_remove(qcom->dwc);
The current code was broken here too:
https://lore.kernel.org/linux-usb/[email protected]/
Johan
On Mon, Oct 16, 2023 at 08:11:17PM -0700, Bjorn Andersson wrote:
> The Qualcomm USB block consists of three intertwined parts, the XHCI,
> the DWC3 core and the Qualcomm DWC3 glue. The exsting binding represents
> the Qualcomm glue part, with the other two represented as in a child
> node.
>
> Rename the qcom,dwc3 binding, to represent that this is indeed only the
> glue part, to make room for a combined binding.
>
> The large "select" is included to avoid the schema to be selected for
> validation with the upcoming flattened binding - which includes
> snps,dwc3 in the compatible.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> -title: Qualcomm SuperSpeed DWC3 USB SoC controller
> +title: Qualcomm SuperSpeed DWC3 USB SoC controller glue
> +
> +description:
> + This describes the Qualcomm glue-section of the SuperSpeed DWC3 USB
> + controller found in many Qualcomm platforms, with the XHCI and DWC3 core
This should be "xHCI" throughout the series.
> + portions described as a separate child device.
> + The combined representation, defined by qcom,dwc3.yaml is preferred.
Johan
On Mon, Oct 16, 2023 at 08:11:18PM -0700, Bjorn Andersson wrote:
> The Qualcomm USB block consists of three intertwined parts, the XHCI,
> the DWC3 core and the Qualcomm DWC3 glue. The three parts can not be
> operated independently, but the binding was for historical reasons split
> to mimic the Linux driver implementation.
>
> The split binding also makes it hard to alter the implementation, as
> properties and resources are split between the two nodes, in some cases
> with some duplication.
>
> Introduce a new binding, with a single representation of the whole USB
> block in one node.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - qcom,sc8280xp-dwc3
> + - qcom,sc8280xp-dwc3-mp
The multiport implementation is not ready yet and this part of the
binding has been reverted (similar for the multiport interrupts below).
> + then:
> + properties:
> + clocks:
> + maxItems: 9
> + clock-names:
> + items:
> + - const: cfg_noc
> + - const: core
> + - const: iface
> + - const: sleep
> + - const: mock_utmi
> + - const: noc_aggr
> + - const: noc_aggr_north
> + - const: noc_aggr_south
> + - const: noc_sys
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - qcom,sc8280xp-dwc3-mp
> + then:
> + properties:
> + interrupts:
> + maxItems: 14
> + interrupt-names:
> + items:
> + - const: pwr_event_1
> + - const: pwr_event_2
> + - const: pwr_event_3
> + - const: pwr_event_4
> + - const: dp_hs_phy_1
> + - const: dm_hs_phy_1
> + - const: dp_hs_phy_2
> + - const: dm_hs_phy_2
> + - const: dp_hs_phy_3
> + - const: dm_hs_phy_3
> + - const: dp_hs_phy_4
> + - const: dm_hs_phy_4
> + - const: ss_phy_1
> + - const: ss_phy_2
So same here.
> + else:
> + properties:
> + interrupts:
> + minItems: 1
> + items:
> + - description: Common DWC3 interrupt
> + - description: The interrupt that is asserted
> + when a wakeup event is received on USB2 bus.
> + - description: The interrupt that is asserted
> + when a wakeup event is received on USB3 bus.
> + - description: Wakeup event on DM line.
> + - description: Wakeup event on DP line.
I guess you may have copied this from the current binding but the
descriptions here are not correct. The HS/SS interrupt comes from the
PHYs in case the corresponding events have been enabled. I assume it can
be used for connect/disconnect events as well as remote wakeup and
whether to actually wake the system up on those is an implementation
detail.
Similar for DM/DP which represents the state of the data lines and that
can be used to detect all sorts of events, not just remote wakeup.
> +
> + interrupt-names:
> + minItems: 1
> + items:
> + - const: dwc_usb3
> + - const: hs_phy_irq
> + - const: ss_phy_irq
> + - const: dm_hs_phy_irq
> + - const: dp_hs_phy_irq
And here you are now defining all of these interrupts for all the
current SoCs it seems, despite not all of them actually having all of
these at once. (The order also does not match the current devicetrees.)
Some only have HS/SS, and it's not clear whether the HS interrupts are
actually functional when a SoC is also using DP/DM.
We're currently discussing this here:
https://lore.kernel.org/lkml/[email protected]/
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + usb@a600000 {
> + compatible = "qcom,sdm845-dwc3", "qcom,dwc3", "snps,dwc3";
> + reg = <0x0a600000 0x200000>;
> + snps,dis_u2_susphy_quirk;
> + snps,dis_enblslpm_quirk;
> + phys = <&usb_1_hsphy>, <&usb_1_ssphy>;
> + phy-names = "usb2-phy", "usb3-phy";
> +
Stray newline.
> + };
> +...
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/[email protected]/
>
> > + 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
On Wed, Nov 22, 2023 at 12:57:37PM +0100, Johan Hovold wrote:
> On Mon, Oct 16, 2023 at 08:11:12PM -0700, Bjorn Andersson wrote:
> > The DWC3 IP block is handled by three distinct device drivers: XHCI,
> > DWC3 core and a platform specific (optional) DWC3 glue driver.
> >
> > This has resulted in, at least in the case of the Qualcomm glue, the
> > presence of a number of layering violations, where the glue code either
> > can't handle, or has to work around, the fact that core might not probe
> > deterministically.
> >
> > An example of this is that the suspend path should operate slightly
> > different depending on the device operating in host or peripheral mode,
> > and the only way to determine the operating state is to peek into the
> > core's drvdata.
> >
> > The Qualcomm glue driver is expected to make updates in the qscratch
> > register region (the "glue" region) during role switch events, but with
> > the glue and core split using the driver model, there is no reasonable
> > way to introduce listeners for mode changes.
> >
> > Split the dwc3 core platfrom_driver callbacks and their implementation
> > and export the implementation, to make it possible to deterministically
> > instantiate the dwc3 core as part of the dwc3 glue drivers and to
> > allow flattening of the DeviceTree representation.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > drivers/usb/dwc3/core.c | 134 ++++++++++++++++++++++++++++++++----------------
> > drivers/usb/dwc3/core.h | 10 ++++
> > 2 files changed, 100 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index d25490965b27..71e376bebb16 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1876,7 +1876,7 @@ static int dwc3_get_clocks(struct dwc3 *dwc)
> > return 0;
> > }
> >
> > -static int dwc3_probe(struct platform_device *pdev)
> > +struct dwc3 *dwc3_probe(struct platform_device *pdev)
>
> Perhaps you should move allocation of struct dwc3 to the caller as well
> as you are going to need some way to pass in callback to core which need
> to be set before you register the xhci platform device.
>
> There could be other ways, like passing in a struct of callbacks, which
> can be added incrementally but it may be good think this through from
> the start.
>
My intention was to have callbacks and quirks passed through additional
arguments in an incremental patch.
IMHO passing such information through a pre-allocated and partially
initialized struct is more obscure than passing that information as
explicit parameters to the function...
> > {
> > struct device *dev = &pdev->dev;
> > struct resource *res, dwc_res;
> > @@ -1886,14 +1886,14 @@ static int dwc3_probe(struct platform_device *pdev)
> >
> > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
> > if (!dwc)
> > - return -ENOMEM;
> > + return ERR_PTR(-ENOMEM);
> >
> > dwc->dev = dev;
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (!res) {
> > dev_err(dev, "missing memory resource\n");
> > - return -ENODEV;
> > + return ERR_PTR(-ENODEV);
> > }
> >
> > dwc->xhci_resources[0].start = res->start;
> > @@ -1922,7 +1922,7 @@ static int dwc3_probe(struct platform_device *pdev)
> >
> > regs = devm_ioremap_resource(dev, &dwc_res);
> > if (IS_ERR(regs))
> > - return PTR_ERR(regs);
> > + return ERR_CAST(regs);
> >
> > dwc->regs = regs;
> > dwc->regs_size = resource_size(&dwc_res);
> > @@ -1953,7 +1953,6 @@ static int dwc3_probe(struct platform_device *pdev)
> > goto err_disable_clks;
> > }
> >
> > - platform_set_drvdata(pdev, dwc);
>
> This is broken however as the pm ops access the data driver data and can
> be called as soon as you call pm_runtime_put() below.
>
You're right, thanks for spotting that.
Regards,
Bjorn
> > dwc3_cache_hwparams(dwc);
> >
> > if (!dwc->sysdev_is_parent &&
> > @@ -2006,7 +2005,7 @@ static int dwc3_probe(struct platform_device *pdev)
> >
> > pm_runtime_put(dev);
>
> That is here.
>
> > - return 0;
> > + return dwc;
>
> > -static void dwc3_remove(struct platform_device *pdev)
> > +static int dwc3_plat_probe(struct platform_device *pdev)
> > {
> > - struct dwc3 *dwc = platform_get_drvdata(pdev);
> > + struct dwc3 *dwc;
> > +
> > + dwc = dwc3_probe(pdev);
> > + if (IS_ERR(dwc))
> > + return PTR_ERR(dwc);
>
> And that leaves a window, for example, here where you can hit a NULL
> pointer dereference.
>
> > + platform_set_drvdata(pdev, dwc);
> > +
> > + return 0;
> > +}
>
> Johan
>
On Wed, Nov 22, 2023 at 10:48:50AM +0100, Johan Hovold wrote:
> On Mon, Oct 16, 2023 at 08:11:08PM -0700, Bjorn Andersson wrote:
> > 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.
> >
> > 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.
>
> Nice to see this finally being worked on. It's not clear why mode-change
> notifications would be a problem though, as if you get such a
> notification, you know that core has been probed.
>
The problem here is that the usb_role_switch is implemented in the core,
but the glue needs to act upon the notification as well - and there's
currently no way to have the core notify the glue about such changes.
You can see this "solved" in the case of extcon, where both the Qualcomm
glue and core listens to and act upon the extcon updates. This isn't
pretty, but with the of-graph based role-switching description (and good
judgement) it's not possible to replicate this on modern platforms.
Which means that this leaves a TODO to investigate if we can drop the
extcon support from dwc3-qcom.c
Regards,
Bjorn
> > Furhtermore, the DeviceTree binding is a direct representation of the
> > Linux driver model, and doesn't necessarily describe "the USB IP-block".
>
> True.
>
> Johan
On Wed, Nov 22, 2023 at 01:18:33PM +0100, Johan Hovold wrote:
> On Mon, Oct 16, 2023 at 08:11:14PM -0700, Bjorn Andersson wrote:
> > In the coming changes the Qualcomm DWC3 glue will be able to either
> > manage the DWC3 core as a child platform_device, or directly instantiate
> > it within its own context.
> >
> > Introduce a reference to the dwc3 core state and make the driver
> > reference the dwc3 core either the child device or this new reference.
> >
> > As the new member isn't assigned, and qcom->dwc_dev is assigned in all
> > current cases, the change should have no functional impact.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > drivers/usb/dwc3/dwc3-qcom.c | 100 +++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 83 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 7c810712d246..901e5050363b 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -67,7 +67,8 @@ struct dwc3_acpi_pdata {
> > struct dwc3_qcom {
> > struct device *dev;
> > void __iomem *qscratch_base;
> > - struct platform_device *dwc_dev;
> > + struct platform_device *dwc_dev; /* only used when core is separate device */
> > + struct dwc3 *dwc; /* not used when core is separate device */
>
> Hmm. This quickly become really messy and hard to maintain. It may be
> fine as an intermediate step as part of this series, but why can't you
> do the conversion fully so that the Qualcomm glue driver never registers
> a core platform device? Is it just about where the core driver looks for
> DT properties?
>
In the new driver model, pdev->dev.of_node needs to contain the
resources for both the glue and the core. For most of the information,
that's a matter of copying properties and child nodes from the child
of_node, but e.g. reg and interrupts needs to be merged.
As mentioned in my other reply, extcon is serviced to both nodes, so
without the callbacks that will break, at least - and I'd have to check
to see if the of_graphs can be handled...
That said, part of the reason for doing this shuffle is to make sure
that dwc is always a valid pointer, and while keeping this scheme of two
modes we will not be able to assume this anywhere in the code - and
hence continue to rely on luck.
One way around this would be to follow the of_platform_populate() with a
check to see if the core was registered and if so grab the dwc pointer,
otherwise of_platform_depopulate() the core again and probe defer.
It will come with a penalty for devices running on the old binding, and
we don't protect ourselves from the core being unbound while we're
holding a pointer to its internal data. But it looks like a much better
position to me.
(In this case I think dwc_dev becomes a local variable using during
probe, and the rest of the code would operate on dwc)
Regards,
Bjorn
On 10/17/2023 8:41 AM, Bjorn Andersson wrote:
> The USB block found in most Qualcomm platforms is modelled as three
> different independent device drivers, and represented in DeviceTree as
> two layered nodes. But as shown by the already existing layering
> violations in the Qualcomm glue driver they can not be operated
> independently.
>
> In the current model, the probing of the core is asynchronous, and in a
> number of places there's risk that the driver dereferences NULL
> pointers, as it peeks into the core's drvdata.
>
> There is also no way, in the current design to make the core notify the
> glue upon DRD mode changes. Among the past proposals have been attempts
> to provide a callback registration API, but as there is no way to know
> when the core is probed this doesn't work.
>
> Based on the recent refactoring its now possible to instantiate the glue
> and core from a single representation of the DWC3 IP-block. This will
> also allow for the glue to pass a callback to be called for DRD mode
> changes.
>
> The only overlapping handling between the Qualcomm glue and the core is
> the release of reset, which is left to the core to handle.
>
Hi Bjorn,
I think the reset has to be handled by glue itself. I was testing this
series and found one issue:
During suspend, we suspend core first which will assert the reset and
then suspend the glue which will disable the clocks. This path doesn't
seem to have a problem somehow even in flattened implementation.
During resume, we resume the glue first and then resume the core.
During resume of glue, we enable the clocks and at this point, the reset
is still kept asserted causing the clocks to never turn ON leading to a
crash. This is the case in flattened implementation only as in normal
case, the reset is handled by glue and we never meddle with reset other
than the time of probing.
I tried to check if we explicitly de-assert the reset during start of
resume sequence of glue (in addition to the de-assertion present in
core) and things worked out fine. But if I try to balance the reset
count and add an assert at end of suspend sequence of glue (in addition
to the assertion present in core), then it crashes complaining a double
assertion happened. So double de-asserting is not causing a problem but
double asserting is causing an issue.
Regards,
Krishna,
On 10/17/2023 8:41 AM, Bjorn Andersson wrote:
> The Qualcomm DWC3 glue builds up a platform_device programmatically in
> order to probe the DWC3 core when running off ACPI data. But with the
> newly introduced support for instantiating the core directly from the
> glue, this code can be replaced with a single function call.
>
> @@ -942,7 +889,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> if (np)
> ret = dwc3_qcom_of_register_core(pdev);
> else
> - ret = dwc3_qcom_acpi_register_core(pdev);
> + ret = dwc3_qcom_probe_core(pdev, qcom);
>
> if (ret) {
> dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret);
> @@ -986,10 +933,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> interconnect_exit:
> dwc3_qcom_interconnect_exit(qcom);
> depopulate:
> - if (np)
> + if (qcom->dwc_dev)
> of_platform_depopulate(&pdev->dev);
> else
> - platform_device_put(pdev);
> + dwc3_remove(qcom->dwc);
Hi Bjorn
I was testing this patch and I suspect there is one issue.
In the event dwc3_qcom_probe_core fails because dwc3_probe failed,
then the variable (qcom->dwc) is NULL. We then exercise the depopulate
path and we call dwc_remove(NULL) causing a crash.
Regards,
Krishna,
On Wed, Jan 10, 2024 at 08:43:23AM +0530, Krishna Kurapati PSSNV wrote:
>
>
> On 10/17/2023 8:41 AM, Bjorn Andersson wrote:
> > The USB block found in most Qualcomm platforms is modelled as three
> > different independent device drivers, and represented in DeviceTree as
> > two layered nodes. But as shown by the already existing layering
> > violations in the Qualcomm glue driver they can not be operated
> > independently.
> >
> > In the current model, the probing of the core is asynchronous, and in a
> > number of places there's risk that the driver dereferences NULL
> > pointers, as it peeks into the core's drvdata.
> >
> > There is also no way, in the current design to make the core notify the
> > glue upon DRD mode changes. Among the past proposals have been attempts
> > to provide a callback registration API, but as there is no way to know
> > when the core is probed this doesn't work.
> >
> > Based on the recent refactoring its now possible to instantiate the glue
> > and core from a single representation of the DWC3 IP-block. This will
> > also allow for the glue to pass a callback to be called for DRD mode
> > changes.
> >
> > The only overlapping handling between the Qualcomm glue and the core is
> > the release of reset, which is left to the core to handle.
> >
>
> Hi Bjorn,
>
> I think the reset has to be handled by glue itself. I was testing this
> series and found one issue:
>
> During suspend, we suspend core first which will assert the reset and then
> suspend the glue which will disable the clocks. This path doesn't seem to
> have a problem somehow even in flattened implementation.
>
> During resume, we resume the glue first and then resume the core. During
> resume of glue, we enable the clocks and at this point, the reset is still
> kept asserted causing the clocks to never turn ON leading to a crash. This
> is the case in flattened implementation only as in normal case, the reset is
> handled by glue and we never meddle with reset other than the time of
> probing.
>
> I tried to check if we explicitly de-assert the reset during start of resume
> sequence of glue (in addition to the de-assertion present in core) and
> things worked out fine. But if I try to balance the reset count and add an
> assert at end of suspend sequence of glue (in addition to the assertion
> present in core), then it crashes complaining a double assertion happened.
> So double de-asserting is not causing a problem but double asserting is
> causing an issue.
>
You're right. I looked at it briefly but ended up moving the reset
handling in the wrong direction...
I expect that in any scenario where a glue driver is used the core can
not control the reset. So far we've dealt with this by just not telling
the core about the reset.
Thanks,
Bjorn
> Regards,
> Krishna,