mbox series

[00/12] usb: dwc3: qcom: Flatten dwc3 structure

Message ID 20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com
Headers show
Series usb: dwc3: qcom: Flatten dwc3 structure | expand

Message

Bjorn Andersson Oct. 17, 2023, 3:11 a.m. UTC
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 <quic_bjorande@quicinc.com>
---
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,

Comments

Krzysztof Kozlowski Oct. 17, 2023, 6:05 a.m. UTC | #1
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 <quic_bjorande@quicinc.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Konrad Dybcio Oct. 17, 2023, 4:08 p.m. UTC | #2
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 <quic_bjorande@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
Konrad Dybcio Oct. 17, 2023, 4:14 p.m. UTC | #3
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 <quic_bjorande@quicinc.com>
> ---
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
Thinh Nguyen Oct. 20, 2023, 10:04 p.m. UTC | #4
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 <quic_bjorande@quicinc.com>
> ---
> 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 <quic_bjorande@quicinc.com>
> 

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
Johan Hovold Nov. 22, 2023, 9:48 a.m. UTC | #5
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
Bjorn Andersson Jan. 8, 2024, 4:46 p.m. UTC | #6
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