Message-ID: <6d406ffb-7e2b-4e3c-b316-82a2a6ab57eb@linaro.org>
Date: Fri, 11 Apr 2025 16:08:50 +0200
From: neil.armstrong@...aro.org
To: Bjorn Andersson <bjorn.andersson@....qualcomm.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Felipe Balbi <balbi@...nel.org>,
 Wesley Cheng <quic_wcheng@...cinc.com>,
 Saravana Kannan <saravanak@...gle.com>,
 Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>, Frank Li <Frank.li@....com>
Cc: linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 0/6] usb: dwc3: qcom: Flatten dwc3 structure

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/20250318-dwc3-refactor-v5-0-90ea6e5b3ba4@oss.qualcomm.com
> 
> 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/20250226-dwc3-refactor-v4-0-4415e7111e49@oss.qualcomm.com
> 
> 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/20250113-dwc3-refactor-v3-0-d1722075df7b@oss.qualcomm.com
> 
> 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/20240811-dwc3-refactor-v2-0-91f370d61ad2@quicinc.com
> 
> 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/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/
> 
> ---
> 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 <neil.armstrong@...aro.org> # on SM8650-QRD

Thanks!
Neil

Powered by blists - more mailing lists