| Message ID | 20250113-dwc3-refactor-v3-0-d1722075df7b@oss.qualcomm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | usb: dwc3: qcom: Flatten dwc3 structure | expand |
On Mon, Jan 13, 2025 at 09:11:33PM -0800, 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. > > To avoid littering the dwc3-qcom driver with the migration code - which > we should be able to drop again in a LTS or two - this is now placed in > drivers/of/overlays. > > A patch to convert a single platform - sc8280xp - is included in the > series. The broader conversion will be submitted in a follow up series. Is it not possible to use the same overlays also fixup the .dts files at build time? Rob
On Tue, Jan 14, 2025 at 11:44:52AM -0600, Rob Herring wrote: > On Mon, Jan 13, 2025 at 09:11:33PM -0800, 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. > > > > To avoid littering the dwc3-qcom driver with the migration code - which > > we should be able to drop again in a LTS or two - this is now placed in > > drivers/of/overlays. > > > > A patch to convert a single platform - sc8280xp - is included in the > > series. The broader conversion will be submitted in a follow up series. > > Is it not possible to use the same overlays also fixup the .dts files at > build time? > I presume so. What would the benefit of that be, over fixing up the source asap? Regards, Bjorn
On Tue, Jan 14, 2025 at 5:04 PM Bjorn Andersson <andersson@kernel.org> wrote: > > On Tue, Jan 14, 2025 at 11:44:52AM -0600, Rob Herring wrote: > > On Mon, Jan 13, 2025 at 09:11:33PM -0800, 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. > > > > > > To avoid littering the dwc3-qcom driver with the migration code - which > > > we should be able to drop again in a LTS or two - this is now placed in > > > drivers/of/overlays. > > > > > > A patch to convert a single platform - sc8280xp - is included in the > > > series. The broader conversion will be submitted in a follow up series. > > > > Is it not possible to use the same overlays also fixup the .dts files at > > build time? > > > > I presume so. What would the benefit of that be, over fixing up the > source asap? The overlays would live with all the other dts files (I think kbuild can add built-in dtbs from arch/*/boot/dts/). We can test at build time they actually apply, and ensure the new dtb matches what the fixup overlay creates. Rob
On Wed, Jan 15, 2025 at 12:51:42PM -0600, Rob Herring wrote: > On Tue, Jan 14, 2025 at 5:04 PM Bjorn Andersson <andersson@kernel.org> wrote: > > > > On Tue, Jan 14, 2025 at 11:44:52AM -0600, Rob Herring wrote: > > > On Mon, Jan 13, 2025 at 09:11:33PM -0800, 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. > > > > > > > > To avoid littering the dwc3-qcom driver with the migration code - which > > > > we should be able to drop again in a LTS or two - this is now placed in > > > > drivers/of/overlays. > > > > > > > > A patch to convert a single platform - sc8280xp - is included in the > > > > series. The broader conversion will be submitted in a follow up series. > > > > > > Is it not possible to use the same overlays also fixup the .dts files at > > > build time? > > > > > > > I presume so. What would the benefit of that be, over fixing up the > > source asap? > > The overlays would live with all the other dts files (I think kbuild > can add built-in dtbs from arch/*/boot/dts/). We can test at build > time they actually apply, and ensure the new dtb matches what the > fixup overlay creates. > That does sounds tempting, in particular since it sounds like it would provide us with dt-validation of the end result. But, the build-time overlaid dtb files wouldn't be complete, as I programmatically transition some of the properties - to "fix" that I'd have to provide an overlay per board. Second, it was my intention to transition all the boards to the new binding as soon as possible, to avoid adding more overlays when new boards are added. So any support-system we build up for this, would be immediately obsoleted. Regards, Bjorn
On Wed, Jan 22, 2025 at 09:07:43PM -0600, Bjorn Andersson wrote: > On Wed, Jan 15, 2025 at 12:51:42PM -0600, Rob Herring wrote: > > On Tue, Jan 14, 2025 at 5:04 PM Bjorn Andersson <andersson@kernel.org> wrote: > > > > > > On Tue, Jan 14, 2025 at 11:44:52AM -0600, Rob Herring wrote: > > > > On Mon, Jan 13, 2025 at 09:11:33PM -0800, 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. > > > > > > > > > > To avoid littering the dwc3-qcom driver with the migration code - which > > > > > we should be able to drop again in a LTS or two - this is now placed in > > > > > drivers/of/overlays. > > > > > > > > > > A patch to convert a single platform - sc8280xp - is included in the > > > > > series. The broader conversion will be submitted in a follow up series. > > > > > > > > Is it not possible to use the same overlays also fixup the .dts files at > > > > build time? > > > > > > > > > > I presume so. What would the benefit of that be, over fixing up the > > > source asap? > > > > The overlays would live with all the other dts files (I think kbuild > > can add built-in dtbs from arch/*/boot/dts/). We can test at build > > time they actually apply, and ensure the new dtb matches what the > > fixup overlay creates. > > > > That does sounds tempting, in particular since it sounds like it would > provide us with dt-validation of the end result. > > But, the build-time overlaid dtb files wouldn't be complete, as I > programmatically transition some of the properties - to "fix" that I'd > have to provide an overlay per board. > > Second, it was my intention to transition all the boards to the new > binding as soon as possible, to avoid adding more overlays when new > boards are added. So any support-system we build up for this, would be > immediately obsoleted. Ok, fair enough. I would still prefer the overlays live in arch/*/boot/dts/qcom/ even if we don't do the rest. Rob
On Mon, Jan 13, 2025 at 09:11:33PM -0800, 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. > > To avoid littering the dwc3-qcom driver with the migration code - which > we should be able to drop again in a LTS or two - this is now placed in > drivers/of/overlays. > > A patch to convert a single platform - sc8280xp - is included in the > series. The broader conversion will be submitted in a follow up series. > > [...] > --- > Bjorn Andersson (12): > dt-bindings: usb: snps,dwc3: Split core description > dt-bindings: usb: Introduce qcom,snps-dwc3 > of: dynamic: Add of_changeset_add_prop_copy() > of: overlays: Introduce dwc3 flattening overlay > of: overlays: dwc3-flattening: Add Qualcomm Arm32 overlays > of: overlays: dwc3-flattening: Add Qualcomm Arm64 board overlays > of: overlays: dwc3-flattening: Provide overlay symbols > 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 > arm64: dts: qcom: sc8280x: Flatten the USB nodes > > .../devicetree/bindings/usb/qcom,dwc3.yaml | 13 +- > .../devicetree/bindings/usb/qcom,snps-dwc3.yaml | 618 ++++++++ > .../devicetree/bindings/usb/snps,dwc3-common.yaml | 415 ++++++ > .../devicetree/bindings/usb/snps,dwc3.yaml | 391 +---- > arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 12 +- > arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 5 +- > arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 12 +- > .../boot/dts/qcom/sc8280xp-huawei-gaokun3.dts | 10 +- > .../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 11 +- > .../boot/dts/qcom/sc8280xp-microsoft-arcata.dts | 10 +- > .../boot/dts/qcom/sc8280xp-microsoft-blackrock.dts | 18 +- > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 157 +- > drivers/of/Kconfig | 2 + > drivers/of/Makefile | 2 + > drivers/of/dynamic.c | 20 + > drivers/of/overlays/Kconfig | 15 + > drivers/of/overlays/Makefile | 3 + > drivers/of/overlays/dwc3-flattening/Makefile | 94 ++ > .../of/overlays/dwc3-flattening/dwc3-flattening.c | 1552 ++++++++++++++++++++ > .../of/overlays/dwc3-flattening/dwc3-flattening.h | 188 +++ > .../overlays/dwc3-flattening/dwc3-qcom_apq8094.dts | 32 + > .../overlays/dwc3-flattening/dwc3-qcom_apq8096.dts | 60 + > .../dwc3-qcom_apq8096_inforce_ifc6640.dts | 58 + > .../overlays/dwc3-flattening/dwc3-qcom_ipq4018.dts | 36 + > .../dwc3-qcom_ipq4018_8dev_jalapeno.dts | 38 + > .../overlays/dwc3-flattening/dwc3-qcom_ipq4019.dts | 38 + > .../overlays/dwc3-flattening/dwc3-qcom_ipq5018.dts | 28 + > .../overlays/dwc3-flattening/dwc3-qcom_ipq5332.dts | 32 + > .../overlays/dwc3-flattening/dwc3-qcom_ipq5424.dts | 58 + > .../overlays/dwc3-flattening/dwc3-qcom_ipq6018.dts | 54 + > .../overlays/dwc3-flattening/dwc3-qcom_ipq8064.dts | 40 + > .../overlays/dwc3-flattening/dwc3-qcom_ipq8074.dts | 58 + > .../overlays/dwc3-flattening/dwc3-qcom_ipq9574.dts | 29 + > .../overlays/dwc3-flattening/dwc3-qcom_msm8953.dts | 32 + > .../overlays/dwc3-flattening/dwc3-qcom_msm8992.dts | 32 + > .../overlays/dwc3-flattening/dwc3-qcom_msm8994.dts | 32 + > .../overlays/dwc3-flattening/dwc3-qcom_msm8996.dts | 58 + > .../dwc3-qcom_msm8996_oneplus_oneplus3.dts | 56 + > .../dwc3-qcom_msm8996_oneplus_oneplus3t.dts | 56 + > .../dwc3-qcom_msm8996_sony_dora_row.dts | 57 + > .../dwc3-qcom_msm8996_sony_kagura_row.dts | 57 + > .../dwc3-qcom_msm8996_sony_keyaki_row.dts | 57 + > .../dwc3-qcom_msm8996_xiaomi_gemini.dts | 56 + > .../dwc3-qcom_msm8996_xiaomi_natrium.dts | 56 + > .../dwc3-qcom_msm8996_xiaomi_scorpio.dts | 56 + > .../overlays/dwc3-flattening/dwc3-qcom_msm8998.dts | 34 + > .../dwc3-qcom_msm8998_fxtec_pro1.dts | 35 + > .../dwc3-qcom_msm8998_oneplus_cheeseburger.dts | 32 + > .../dwc3-qcom_msm8998_oneplus_dumpling.dts | 32 + > .../dwc3-qcom_msm8998_sony_xperia_lilac.dts | 35 + > .../dwc3-qcom_msm8998_sony_xperia_maple.dts | 35 + > .../dwc3-qcom_msm8998_sony_xperia_poplar.dts | 35 + > .../dwc3-qcom_msm8998_xiaomi_sagit.dts | 32 + > .../overlays/dwc3-flattening/dwc3-qcom_qcm2290.dts | 32 + > .../overlays/dwc3-flattening/dwc3-qcom_qcm6490.dts | 63 + > .../overlays/dwc3-flattening/dwc3-qcom_qcs404.dts | 56 + > .../overlays/dwc3-flattening/dwc3-qcom_qcs615.dts | 62 + > .../overlays/dwc3-flattening/dwc3-qcom_qcs8300.dts | 62 + > .../overlays/dwc3-flattening/dwc3-qcom_qdu1000.dts | 38 + > .../overlays/dwc3-flattening/dwc3-qcom_qru1000.dts | 38 + > .../overlays/dwc3-flattening/dwc3-qcom_sa8155p.dts | 71 + > .../overlays/dwc3-flattening/dwc3-qcom_sa8540p.dts | 129 ++ > .../overlays/dwc3-flattening/dwc3-qcom_sa8775p.dts | 90 ++ > .../dwc3-flattening/dwc3-qcom_sar2130p.dts | 39 + > .../overlays/dwc3-flattening/dwc3-qcom_sc7180.dts | 39 + > .../overlays/dwc3-flattening/dwc3-qcom_sc7280.dts | 63 + > .../overlays/dwc3-flattening/dwc3-qcom_sc8180x.dts | 109 ++ > .../dwc3-flattening/dwc3-qcom_sc8280xp.dts | 129 ++ > .../dwc3-qcom_sc8280xp_microsoft_blackrock.dts | 121 ++ > .../overlays/dwc3-flattening/dwc3-qcom_sda660.dts | 59 + > .../overlays/dwc3-flattening/dwc3-qcom_sdm450.dts | 33 + > .../overlays/dwc3-flattening/dwc3-qcom_sdm630.dts | 57 + > .../overlays/dwc3-flattening/dwc3-qcom_sdm632.dts | 32 + > .../overlays/dwc3-flattening/dwc3-qcom_sdm636.dts | 59 + > .../overlays/dwc3-flattening/dwc3-qcom_sdm660.dts | 57 + > .../overlays/dwc3-flattening/dwc3-qcom_sdm670.dts | 36 + > .../overlays/dwc3-flattening/dwc3-qcom_sdm845.dts | 64 + > .../dwc3-qcom_sdm845_lenovo_yoga_c630.dts | 67 + > .../dwc3-flattening/dwc3-qcom_sdm845_lg_judyln.dts | 67 + > .../dwc3-flattening/dwc3-qcom_sdm845_lg_judyp.dts | 67 + > .../dwc3-qcom_sdm845_qcom_sdm845_mtp.dts | 67 + > .../dwc3-qcom_sdm845_samsung_starqltechn.dts | 67 + > .../dwc3-qcom_sdm845_samsung_w737.dts | 67 + > .../dwc3-qcom_sdm845_shift_axolotl.dts | 67 + > .../dwc3-qcom_sdm845_thundercomm_db845c.dts | 67 + > .../dwc3-qcom_sdm845_xiaomi_beryllium.dts | 67 + > .../dwc3-qcom_sdm845_xiaomi_beryllium_ebbg.dts | 67 + > .../overlays/dwc3-flattening/dwc3-qcom_sdx55.dts | 38 + > .../overlays/dwc3-flattening/dwc3-qcom_sdx65.dts | 38 + > .../overlays/dwc3-flattening/dwc3-qcom_sdx75.dts | 36 + > .../overlays/dwc3-flattening/dwc3-qcom_sm4250.dts | 37 + > .../dwc3-qcom_sm4250_oneplus_billie2.dts | 35 + > .../overlays/dwc3-flattening/dwc3-qcom_sm6115.dts | 37 + > .../dwc3-qcom_sm6115_lenovo_j606f.dts | 35 + > .../overlays/dwc3-flattening/dwc3-qcom_sm6125.dts | 36 + > .../overlays/dwc3-flattening/dwc3-qcom_sm6350.dts | 39 + > .../overlays/dwc3-flattening/dwc3-qcom_sm6375.dts | 36 + > .../overlays/dwc3-flattening/dwc3-qcom_sm7125.dts | 39 + > .../overlays/dwc3-flattening/dwc3-qcom_sm7225.dts | 39 + > .../overlays/dwc3-flattening/dwc3-qcom_sm7325.dts | 60 + > .../overlays/dwc3-flattening/dwc3-qcom_sm8150.dts | 67 + > .../overlays/dwc3-flattening/dwc3-qcom_sm8250.dts | 67 + > .../dwc3-qcom_sm8250_xiaomi_elish.dts | 64 + > .../overlays/dwc3-flattening/dwc3-qcom_sm8350.dts | 67 + > .../dwc3-qcom_sm8350_microsoft_surface_duo2.dts | 67 + > .../dwc3-qcom_sm8350_qcom_sm8350_hdk.dts | 69 + > .../dwc3-qcom_sm8350_qcom_sm8350_mtp.dts | 67 + > .../dwc3-qcom_sm8350_sony_pdx214_generic.dts | 67 + > .../dwc3-qcom_sm8350_sony_pdx215_generic.dts | 67 + > .../overlays/dwc3-flattening/dwc3-qcom_sm8450.dts | 39 + > .../overlays/dwc3-flattening/dwc3-qcom_sm8550.dts | 39 + > .../overlays/dwc3-flattening/dwc3-qcom_sm8650.dts | 39 + > .../dwc3-flattening/dwc3-qcom_x1e80100.dts | 153 ++ > .../dwc3-qcom_x1e80100_hp_omnibook_x14.dts | 149 ++ > drivers/usb/dwc3/core.c | 167 ++- > drivers/usb/dwc3/dwc3-qcom.c | 149 +- > drivers/usb/dwc3/glue.h | 22 + > include/linux/of.h | 3 + > 118 files changed, 8389 insertions(+), 670 deletions(-) > --- This is quite a lot of code and new files for a temporary migration. It's also difficult to test these changes fully, since there are separate overlays for each SoC and sometimes even each board. Would it be easier to just duplicate the dwc3-qcom driver for now? Making a copy of the current dwc3-qcom.c would be just 1000 lines of extra code, compared to more than 7000 for the overlay approach. The copy (e.g. dwc3-qcom-legacy.c) would keep handling the old bindings with the existing code (that is known to work to some extent). We can then improve upon the main version without risk of breaking any old DTBs. If we decide to drop support for the old DTBs at some point, we can just drop dwc3-qcom-legacy. This approach is also not pretty, but I think the risk and effort would be lower than making sure the overlay approach works on all the affected targets. Thanks, Stephan
On Mon, Jan 27, 2025 at 06:57:41PM +0100, Stephan Gerhold wrote: > On Mon, Jan 13, 2025 at 09:11:33PM -0800, Bjorn Andersson wrote: [..] > > 118 files changed, 8389 insertions(+), 670 deletions(-) > > --- > > This is quite a lot of code and new files for a temporary migration. > It's also difficult to test these changes fully, since there are > separate overlays for each SoC and sometimes even each board. > > Would it be easier to just duplicate the dwc3-qcom driver for now? > Making a copy of the current dwc3-qcom.c would be just 1000 lines of > extra code, compared to more than 7000 for the overlay approach. > > The copy (e.g. dwc3-qcom-legacy.c) would keep handling the old bindings > with the existing code (that is known to work to some extent). We can > then improve upon the main version without risk of breaking any old > DTBs. If we decide to drop support for the old DTBs at some point, we > can just drop dwc3-qcom-legacy. > > This approach is also not pretty, but I think the risk and effort would > be lower than making sure the overlay approach works on all the affected > targets. > I like this suggestion. It's much more isolated and we know the current state of the driver works with the current dtbs out there - so backwards compatibility would be handled. I also did end up having to use separate compatibles for the old and new binding/driver, so this should be quite clean - i.e. nicer than the overlay-based path... The one drawback would be that devices that isn't updated to a new dtb would not gain the upcoming improved support for role switching, or any of the improvements in pm_runtime-support (as I assume we'd only care about the new driver). But I think that's worth the saving in complexity. Regards, Bjorn
On 27.01.2025 11:40 PM, Bjorn Andersson wrote: > On Mon, Jan 27, 2025 at 06:57:41PM +0100, Stephan Gerhold wrote: >> On Mon, Jan 13, 2025 at 09:11:33PM -0800, Bjorn Andersson wrote: > [..] >>> 118 files changed, 8389 insertions(+), 670 deletions(-) >>> --- >> >> This is quite a lot of code and new files for a temporary migration. >> It's also difficult to test these changes fully, since there are >> separate overlays for each SoC and sometimes even each board. >> >> Would it be easier to just duplicate the dwc3-qcom driver for now? >> Making a copy of the current dwc3-qcom.c would be just 1000 lines of >> extra code, compared to more than 7000 for the overlay approach. >> >> The copy (e.g. dwc3-qcom-legacy.c) would keep handling the old bindings >> with the existing code (that is known to work to some extent). We can >> then improve upon the main version without risk of breaking any old >> DTBs. If we decide to drop support for the old DTBs at some point, we >> can just drop dwc3-qcom-legacy. >> >> This approach is also not pretty, but I think the risk and effort would >> be lower than making sure the overlay approach works on all the affected >> targets. >> > > I like this suggestion. > > It's much more isolated and we know the current state of the driver > works with the current dtbs out there - so backwards compatibility would > be handled. I also did end up having to use separate compatibles for the > old and new binding/driver, so this should be quite clean - i.e. > nicer than the overlay-based path... > > The one drawback would be that devices that isn't updated to a new dtb > would not gain the upcoming improved support for role switching, or any > of the improvements in pm_runtime-support (as I assume we'd only care > about the new driver). But I think that's worth the saving in > complexity. If a user explicitly decides not to update, there's only so much we can do Konrad
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. To avoid littering the dwc3-qcom driver with the migration code - which we should be able to drop again in a LTS or two - this is now placed in drivers/of/overlays. A patch to convert a single platform - sc8280xp - is included in the series. The broader conversion will be submitted in a follow up series. --- 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 (12): dt-bindings: usb: snps,dwc3: Split core description dt-bindings: usb: Introduce qcom,snps-dwc3 of: dynamic: Add of_changeset_add_prop_copy() of: overlays: Introduce dwc3 flattening overlay of: overlays: dwc3-flattening: Add Qualcomm Arm32 overlays of: overlays: dwc3-flattening: Add Qualcomm Arm64 board overlays of: overlays: dwc3-flattening: Provide overlay symbols 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 arm64: dts: qcom: sc8280x: Flatten the USB nodes .../devicetree/bindings/usb/qcom,dwc3.yaml | 13 +- .../devicetree/bindings/usb/qcom,snps-dwc3.yaml | 618 ++++++++ .../devicetree/bindings/usb/snps,dwc3-common.yaml | 415 ++++++ .../devicetree/bindings/usb/snps,dwc3.yaml | 391 +---- arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 12 +- arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 5 +- arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 12 +- .../boot/dts/qcom/sc8280xp-huawei-gaokun3.dts | 10 +- .../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 11 +- .../boot/dts/qcom/sc8280xp-microsoft-arcata.dts | 10 +- .../boot/dts/qcom/sc8280xp-microsoft-blackrock.dts | 18 +- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 157 +- drivers/of/Kconfig | 2 + drivers/of/Makefile | 2 + drivers/of/dynamic.c | 20 + drivers/of/overlays/Kconfig | 15 + drivers/of/overlays/Makefile | 3 + drivers/of/overlays/dwc3-flattening/Makefile | 94 ++ .../of/overlays/dwc3-flattening/dwc3-flattening.c | 1552 ++++++++++++++++++++ .../of/overlays/dwc3-flattening/dwc3-flattening.h | 188 +++ .../overlays/dwc3-flattening/dwc3-qcom_apq8094.dts | 32 + .../overlays/dwc3-flattening/dwc3-qcom_apq8096.dts | 60 + .../dwc3-qcom_apq8096_inforce_ifc6640.dts | 58 + .../overlays/dwc3-flattening/dwc3-qcom_ipq4018.dts | 36 + .../dwc3-qcom_ipq4018_8dev_jalapeno.dts | 38 + .../overlays/dwc3-flattening/dwc3-qcom_ipq4019.dts | 38 + .../overlays/dwc3-flattening/dwc3-qcom_ipq5018.dts | 28 + .../overlays/dwc3-flattening/dwc3-qcom_ipq5332.dts | 32 + .../overlays/dwc3-flattening/dwc3-qcom_ipq5424.dts | 58 + .../overlays/dwc3-flattening/dwc3-qcom_ipq6018.dts | 54 + .../overlays/dwc3-flattening/dwc3-qcom_ipq8064.dts | 40 + .../overlays/dwc3-flattening/dwc3-qcom_ipq8074.dts | 58 + .../overlays/dwc3-flattening/dwc3-qcom_ipq9574.dts | 29 + .../overlays/dwc3-flattening/dwc3-qcom_msm8953.dts | 32 + .../overlays/dwc3-flattening/dwc3-qcom_msm8992.dts | 32 + .../overlays/dwc3-flattening/dwc3-qcom_msm8994.dts | 32 + .../overlays/dwc3-flattening/dwc3-qcom_msm8996.dts | 58 + .../dwc3-qcom_msm8996_oneplus_oneplus3.dts | 56 + .../dwc3-qcom_msm8996_oneplus_oneplus3t.dts | 56 + .../dwc3-qcom_msm8996_sony_dora_row.dts | 57 + .../dwc3-qcom_msm8996_sony_kagura_row.dts | 57 + .../dwc3-qcom_msm8996_sony_keyaki_row.dts | 57 + .../dwc3-qcom_msm8996_xiaomi_gemini.dts | 56 + .../dwc3-qcom_msm8996_xiaomi_natrium.dts | 56 + .../dwc3-qcom_msm8996_xiaomi_scorpio.dts | 56 + .../overlays/dwc3-flattening/dwc3-qcom_msm8998.dts | 34 + .../dwc3-qcom_msm8998_fxtec_pro1.dts | 35 + .../dwc3-qcom_msm8998_oneplus_cheeseburger.dts | 32 + .../dwc3-qcom_msm8998_oneplus_dumpling.dts | 32 + .../dwc3-qcom_msm8998_sony_xperia_lilac.dts | 35 + .../dwc3-qcom_msm8998_sony_xperia_maple.dts | 35 + .../dwc3-qcom_msm8998_sony_xperia_poplar.dts | 35 + .../dwc3-qcom_msm8998_xiaomi_sagit.dts | 32 + .../overlays/dwc3-flattening/dwc3-qcom_qcm2290.dts | 32 + .../overlays/dwc3-flattening/dwc3-qcom_qcm6490.dts | 63 + .../overlays/dwc3-flattening/dwc3-qcom_qcs404.dts | 56 + .../overlays/dwc3-flattening/dwc3-qcom_qcs615.dts | 62 + .../overlays/dwc3-flattening/dwc3-qcom_qcs8300.dts | 62 + .../overlays/dwc3-flattening/dwc3-qcom_qdu1000.dts | 38 + .../overlays/dwc3-flattening/dwc3-qcom_qru1000.dts | 38 + .../overlays/dwc3-flattening/dwc3-qcom_sa8155p.dts | 71 + .../overlays/dwc3-flattening/dwc3-qcom_sa8540p.dts | 129 ++ .../overlays/dwc3-flattening/dwc3-qcom_sa8775p.dts | 90 ++ .../dwc3-flattening/dwc3-qcom_sar2130p.dts | 39 + .../overlays/dwc3-flattening/dwc3-qcom_sc7180.dts | 39 + .../overlays/dwc3-flattening/dwc3-qcom_sc7280.dts | 63 + .../overlays/dwc3-flattening/dwc3-qcom_sc8180x.dts | 109 ++ .../dwc3-flattening/dwc3-qcom_sc8280xp.dts | 129 ++ .../dwc3-qcom_sc8280xp_microsoft_blackrock.dts | 121 ++ .../overlays/dwc3-flattening/dwc3-qcom_sda660.dts | 59 + .../overlays/dwc3-flattening/dwc3-qcom_sdm450.dts | 33 + .../overlays/dwc3-flattening/dwc3-qcom_sdm630.dts | 57 + .../overlays/dwc3-flattening/dwc3-qcom_sdm632.dts | 32 + .../overlays/dwc3-flattening/dwc3-qcom_sdm636.dts | 59 + .../overlays/dwc3-flattening/dwc3-qcom_sdm660.dts | 57 + .../overlays/dwc3-flattening/dwc3-qcom_sdm670.dts | 36 + .../overlays/dwc3-flattening/dwc3-qcom_sdm845.dts | 64 + .../dwc3-qcom_sdm845_lenovo_yoga_c630.dts | 67 + .../dwc3-flattening/dwc3-qcom_sdm845_lg_judyln.dts | 67 + .../dwc3-flattening/dwc3-qcom_sdm845_lg_judyp.dts | 67 + .../dwc3-qcom_sdm845_qcom_sdm845_mtp.dts | 67 + .../dwc3-qcom_sdm845_samsung_starqltechn.dts | 67 + .../dwc3-qcom_sdm845_samsung_w737.dts | 67 + .../dwc3-qcom_sdm845_shift_axolotl.dts | 67 + .../dwc3-qcom_sdm845_thundercomm_db845c.dts | 67 + .../dwc3-qcom_sdm845_xiaomi_beryllium.dts | 67 + .../dwc3-qcom_sdm845_xiaomi_beryllium_ebbg.dts | 67 + .../overlays/dwc3-flattening/dwc3-qcom_sdx55.dts | 38 + .../overlays/dwc3-flattening/dwc3-qcom_sdx65.dts | 38 + .../overlays/dwc3-flattening/dwc3-qcom_sdx75.dts | 36 + .../overlays/dwc3-flattening/dwc3-qcom_sm4250.dts | 37 + .../dwc3-qcom_sm4250_oneplus_billie2.dts | 35 + .../overlays/dwc3-flattening/dwc3-qcom_sm6115.dts | 37 + .../dwc3-qcom_sm6115_lenovo_j606f.dts | 35 + .../overlays/dwc3-flattening/dwc3-qcom_sm6125.dts | 36 + .../overlays/dwc3-flattening/dwc3-qcom_sm6350.dts | 39 + .../overlays/dwc3-flattening/dwc3-qcom_sm6375.dts | 36 + .../overlays/dwc3-flattening/dwc3-qcom_sm7125.dts | 39 + .../overlays/dwc3-flattening/dwc3-qcom_sm7225.dts | 39 + .../overlays/dwc3-flattening/dwc3-qcom_sm7325.dts | 60 + .../overlays/dwc3-flattening/dwc3-qcom_sm8150.dts | 67 + .../overlays/dwc3-flattening/dwc3-qcom_sm8250.dts | 67 + .../dwc3-qcom_sm8250_xiaomi_elish.dts | 64 + .../overlays/dwc3-flattening/dwc3-qcom_sm8350.dts | 67 + .../dwc3-qcom_sm8350_microsoft_surface_duo2.dts | 67 + .../dwc3-qcom_sm8350_qcom_sm8350_hdk.dts | 69 + .../dwc3-qcom_sm8350_qcom_sm8350_mtp.dts | 67 + .../dwc3-qcom_sm8350_sony_pdx214_generic.dts | 67 + .../dwc3-qcom_sm8350_sony_pdx215_generic.dts | 67 + .../overlays/dwc3-flattening/dwc3-qcom_sm8450.dts | 39 + .../overlays/dwc3-flattening/dwc3-qcom_sm8550.dts | 39 + .../overlays/dwc3-flattening/dwc3-qcom_sm8650.dts | 39 + .../dwc3-flattening/dwc3-qcom_x1e80100.dts | 153 ++ .../dwc3-qcom_x1e80100_hp_omnibook_x14.dts | 149 ++ drivers/usb/dwc3/core.c | 167 ++- drivers/usb/dwc3/dwc3-qcom.c | 149 +- drivers/usb/dwc3/glue.h | 22 + include/linux/of.h | 3 + 118 files changed, 8389 insertions(+), 670 deletions(-) --- base-commit: 6ecd20965bdc21b265a0671ccf36d9ad8043f5ab change-id: 20231016-dwc3-refactor-931e3b08a8b9 Best regards,