[v6,0/6] usb: dwc3: qcom: Flatten dwc3 structure

Message

Bjorn Andersson April 11, 2025, 3:50 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 proper 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.

Departing from previous versions' attempts at runtime-convert the
Devicetree representation is swapped out and instead a snapshot of the
current dwc3-qcom driver is proposed to be carried for a limited time.

A patch to convert a single platform - sc8280xp - was included in the
series. This, and others, will be submitted in a separate series as soon
as its introduction won't break bisection.

---
Changes in v6:
- Change legacy driver's name, to avoid collision if both are loaded
- Drop duplicate pm_runtime_{allow,disable}() from dwc3_qcom_remove()
- Drop DeviceTree example patch, as this should be picked up separately
- Replace __maybe_unused for PM and PM_SLEEP functions in dwc3-qcom.c
  with guards, to match the exported functions from the core
- Add missing pm_runtime idle wrapper from dwc3-qcom
- Add missing "dma-coherent" to the qcom,snps-dwc3 binding
- Link to v5: https://lore.kernel.org/r/[e-mail]

Changes in v5:
- Moved the snapshot commit first, to get a clean copy
- Add missing kernel-doc in glue.h
- Used a local "struct device" variable in PM functions to reduce the
  patch size
- Replaced initialization with default values with zero-initalizing the
  dwc3_probe_data in dwc3_probe()
- Add TODO about extcon, as a role-switch callback needs to be
  implemented
- Corrected &usb_2 mmio region length
- Changes the timeline expressed in the commit message to suggest the
  legacy driver to be dropped after next LTS
- Integrated qcom,dwc3.yaml changes since v4
- Link to v4: https://lore.kernel.org/r/[e-mail]

Changes in v4:
- dwc3_{init,uninit}() renamed to dwc3_core_probe() and dwc3_core_remove()
- dwc3_{suspend, resume, complete}() changed to dwc3_pm_*()
- Arguments to dwc3_core_probe() are wrapped in a struct to better
  handle the expected growing list of parameters.
- Add the lost call to dwc3_core_remove() from the Qualcomm glue driver
- Removed now unused cleanup.h, of_address.h, and of_irq.h includes from
  dwc3-qcom.c
- Link to v3: https://lore.kernel.org/r/[e-mail]

Changes in v3:
- Replaced the handcoded migration logic of compatible, reg, interrupts,
  phys with overlays.
- Move the migration logic (and overlays) to a new drivers/of/overlays
  directory and apply this at postcore, so that it takes effect prior to
  the relevant platform_devices are created
- struct dwc3 is embedded in the glue context, rather than having a
  separate object allocated
- The hack of using of_address_to_resource() to avoid platform_resource
  being stale is removed (thanks to applying migration at postcore)
- Link to v2: https://lore.kernel.org/r/[e-mail]

Changes in v2:
- Rewrite after ACPI removal, multiport support and interrupt fixes
- Completely changed strategy for DeviceTree binding, as previous idea
  of using snps,dwc3 as a generic fallback required unreasonable changes
  to that binding.
- Abandoned idea of supporting both flattened and unflattened device
  model in the one driver. As Johan pointed out, it will leave the race
  condition holes and will make the code harder to understand.
  Furthermore, the role switching logic that we intend to introduce
  following this would have depended on the user updating their
  DeviceTree blobs.
- Per above, introduced the dynamic DeviceTree rewrite
- Link to v1: https://lore.kernel.org/all/[e-mail]/

---
Bjorn Andersson (6):
      usb: dwc3: qcom: Snapshot driver for backwards compatibilty
      dt-bindings: usb: Introduce qcom,snps-dwc3
      usb: dwc3: core: Expose core driver as library
      usb: dwc3: core: Don't touch resets and clocks
      usb: dwc3: qcom: Don't rely on drvdata during probe
      usb: dwc3: qcom: Transition to flattened model

 .../devicetree/bindings/usb/qcom,dwc3.yaml         |  13 +-
 .../devicetree/bindings/usb/qcom,snps-dwc3.yaml    | 622 ++++++++++++++
 drivers/usb/dwc3/Makefile                          |   1 +
 drivers/usb/dwc3/core.c                            | 160 ++--
 drivers/usb/dwc3/dwc3-qcom-legacy.c                | 935 +++++++++++++++++++++
 drivers/usb/dwc3/dwc3-qcom.c                       | 182 ++--
 drivers/usb/dwc3/glue.h                            |  35 +
 7 files changed, 1808 insertions(+), 140 deletions(-)
---
base-commit: 29e7bf01ed8033c9a14ed0dc990dfe2736dbcd18
change-id: 20231016-dwc3-refactor-931e3b08a8b9

Best regards,

Comments

Greg Kroah-Hartman April 11, 2025, 1:43 p.m. UTC | #1

On Thu, Apr 10, 2025 at 10:50:11PM -0500, 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 proper 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.
> 
> Departing from previous versions' attempts at runtime-convert the
> Devicetree representation is swapped out and instead a snapshot of the
> current dwc3-qcom driver is proposed to be carried for a limited time.
> 
> A patch to convert a single platform - sc8280xp - was included in the
> series. This, and others, will be submitted in a separate series as soon
> as its introduction won't break bisection.

Doesn't apply to my usb-next branch, can you rebase and resend?

thanks,

greg k-h

Neil Armstrong April 11, 2025, 2:08 p.m. UTC | #2

On 11/04/2025 05:50, 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 proper 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.
> 
> Departing from previous versions' attempts at runtime-convert the
> Devicetree representation is swapped out and instead a snapshot of the
> current dwc3-qcom driver is proposed to be carried for a limited time.
> 
> A patch to convert a single platform - sc8280xp - was included in the
> series. This, and others, will be submitted in a separate series as soon
> as its introduction won't break bisection.
> 
> ---
> Changes in v6:
> - Change legacy driver's name, to avoid collision if both are loaded
> - Drop duplicate pm_runtime_{allow,disable}() from dwc3_qcom_remove()
> - Drop DeviceTree example patch, as this should be picked up separately
> - Replace __maybe_unused for PM and PM_SLEEP functions in dwc3-qcom.c
>    with guards, to match the exported functions from the core
> - Add missing pm_runtime idle wrapper from dwc3-qcom
> - Add missing "dma-coherent" to the qcom,snps-dwc3 binding
> - Link to v5: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v5:
> - Moved the snapshot commit first, to get a clean copy
> - Add missing kernel-doc in glue.h
> - Used a local "struct device" variable in PM functions to reduce the
>    patch size
> - Replaced initialization with default values with zero-initalizing the
>    dwc3_probe_data in dwc3_probe()
> - Add TODO about extcon, as a role-switch callback needs to be
>    implemented
> - Corrected &usb_2 mmio region length
> - Changes the timeline expressed in the commit message to suggest the
>    legacy driver to be dropped after next LTS
> - Integrated qcom,dwc3.yaml changes since v4
> - Link to v4: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v4:
> - dwc3_{init,uninit}() renamed to dwc3_core_probe() and dwc3_core_remove()
> - dwc3_{suspend, resume, complete}() changed to dwc3_pm_*()
> - Arguments to dwc3_core_probe() are wrapped in a struct to better
>    handle the expected growing list of parameters.
> - Add the lost call to dwc3_core_remove() from the Qualcomm glue driver
> - Removed now unused cleanup.h, of_address.h, and of_irq.h includes from
>    dwc3-qcom.c
> - Link to v3: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v3:
> - Replaced the handcoded migration logic of compatible, reg, interrupts,
>    phys with overlays.
> - Move the migration logic (and overlays) to a new drivers/of/overlays
>    directory and apply this at postcore, so that it takes effect prior to
>    the relevant platform_devices are created
> - struct dwc3 is embedded in the glue context, rather than having a
>    separate object allocated
> - The hack of using of_address_to_resource() to avoid platform_resource
>    being stale is removed (thanks to applying migration at postcore)
> - Link to v2: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v2:
> - Rewrite after ACPI removal, multiport support and interrupt fixes
> - Completely changed strategy for DeviceTree binding, as previous idea
>    of using snps,dwc3 as a generic fallback required unreasonable changes
>    to that binding.
> - Abandoned idea of supporting both flattened and unflattened device
>    model in the one driver. As Johan pointed out, it will leave the race
>    condition holes and will make the code harder to understand.
>    Furthermore, the role switching logic that we intend to introduce
>    following this would have depended on the user updating their
>    DeviceTree blobs.
> - Per above, introduced the dynamic DeviceTree rewrite
> - Link to v1: https://lore.kernel.org/all/[e-mail]/
> 
> ---
> Bjorn Andersson (6):
>        usb: dwc3: qcom: Snapshot driver for backwards compatibilty
>        dt-bindings: usb: Introduce qcom,snps-dwc3
>        usb: dwc3: core: Expose core driver as library
>        usb: dwc3: core: Don't touch resets and clocks
>        usb: dwc3: qcom: Don't rely on drvdata during probe
>        usb: dwc3: qcom: Transition to flattened model
> 
>   .../devicetree/bindings/usb/qcom,dwc3.yaml         |  13 +-
>   .../devicetree/bindings/usb/qcom,snps-dwc3.yaml    | 622 ++++++++++++++
>   drivers/usb/dwc3/Makefile                          |   1 +
>   drivers/usb/dwc3/core.c                            | 160 ++--
>   drivers/usb/dwc3/dwc3-qcom-legacy.c                | 935 +++++++++++++++++++++
>   drivers/usb/dwc3/dwc3-qcom.c                       | 182 ++--
>   drivers/usb/dwc3/glue.h                            |  35 +
>   7 files changed, 1808 insertions(+), 140 deletions(-)
> ---
> base-commit: 29e7bf01ed8033c9a14ed0dc990dfe2736dbcd18
> change-id: 20231016-dwc3-refactor-931e3b08a8b9
> 
> Best regards,

Tested-by: Neil Armstrong <[e-mail]> # on SM8650-QRD

Thanks!
Neil