| Message ID | 20231016-dwc3-refactor-v1-11-ab4a84165470@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: qcom: Flatten dwc3 structure | expand |
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 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,
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);
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 <quic_bjorande@quicinc.com> --- drivers/usb/dwc3/dwc3-qcom.c | 49 +++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 17 deletions(-)