mbox series

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

Message ID 20250113-dwc3-refactor-v3-0-d1722075df7b@oss.qualcomm.com
Headers show
Series usb: dwc3: qcom: Flatten dwc3 structure | expand

Message

Bjorn Andersson Jan. 14, 2025, 5: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 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,

Comments

Frank Li Jan. 14, 2025, 7:43 p.m. UTC | #1
On Mon, Jan 13, 2025 at 09:11:41PM -0800, Bjorn Andersson wrote:
> The DWC3 IP block is handled by three distinct device drivers: XHCI,
> DWC3 core and a platform specific (optional) DWC3 glue driver.
>
> This has resulted in, at least in the case of the Qualcomm glue, the
> presence of a number of layering violations, where the glue code either
> can't handle, or has to work around, the fact that core might not probe
> deterministically.
>
> An example of this is that the suspend path should operate slightly
> different depending on the device operating in host or peripheral mode,
> and the only way to determine the operating state is to peek into the
> core's drvdata.
>
> The Qualcomm glue driver is expected to make updates in the qscratch
> register region (the "glue" region) during role switch events, but with
> the glue and core split using the driver model, there is no reasonable
> way to introduce listeners for mode changes.
>
> Split the dwc3 core platform_driver callbacks and their implementation
> and export the implementation, to make it possible to deterministically
> instantiate the dwc3 core as part of the dwc3 glue drivers and to
> allow flattening of the DeviceTree representation.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
>  drivers/usb/dwc3/core.c | 149 ++++++++++++++++++++++++++++++------------------
>  drivers/usb/dwc3/glue.h |  22 +++++++
>  2 files changed, 117 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c22b8678e02e..930d812a9fbb 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -36,6 +36,7 @@
>
>  #include "core.h"
>  #include "gadget.h"
> +#include "glue.h"
>  #include "io.h"
>
>  #include "debug.h"
> @@ -2129,27 +2130,14 @@ static int dwc3_get_num_ports(struct dwc3 *dwc)
>  	return 0;
>  }
>
> -static int dwc3_probe(struct platform_device *pdev)
> +int dwc3_init(struct dwc3 *dwc, struct resource *res)

Maybe need data struct, which included res. It may need pass down more
information in future.

>  {
> -	struct device		*dev = &pdev->dev;
> -	struct resource		*res, dwc_res;
> +	struct device		*dev = dwc->dev;
> +	struct resource		dwc_res;
>  	unsigned int		hw_mode;
>  	void __iomem		*regs;
> -	struct dwc3		*dwc;
>  	int			ret;
>
...

> +++ b/drivers/usb/dwc3/glue.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * glue.h - DesignWare USB3 DRD glue header
> + */
> +
> +#ifndef __DRIVERS_USB_DWC3_GLUE_H
> +#define __DRIVERS_USB_DWC3_GLUE_H
> +
> +#include <linux/types.h>
> +#include "core.h"
> +
> +int dwc3_init(struct dwc3 *dwc, struct resource *res);
> +void dwc3_uninit(struct dwc3 *dwc);
> +
> +int dwc3_runtime_suspend(struct dwc3 *dwc);
> +int dwc3_runtime_resume(struct dwc3 *dwc);
> +int dwc3_runtime_idle(struct dwc3 *dwc);
> +int dwc3_suspend(struct dwc3 *dwc);
> +int dwc3_resume(struct dwc3 *dwc);
> +void dwc3_complete(struct dwc3 *dwc);

dwc3_usb_*()? There may be name polution in future. There are many IPs
created by dwc.

Frank

> +
> +#endif
>
> --
> 2.45.2
>
Bjorn Andersson Jan. 14, 2025, 10:55 p.m. UTC | #2
On Tue, Jan 14, 2025 at 02:43:59PM -0500, Frank Li wrote:
> On Mon, Jan 13, 2025 at 09:11:41PM -0800, Bjorn Andersson wrote:
> > The DWC3 IP block is handled by three distinct device drivers: XHCI,
> > DWC3 core and a platform specific (optional) DWC3 glue driver.
> >
> > This has resulted in, at least in the case of the Qualcomm glue, the
> > presence of a number of layering violations, where the glue code either
> > can't handle, or has to work around, the fact that core might not probe
> > deterministically.
> >
> > An example of this is that the suspend path should operate slightly
> > different depending on the device operating in host or peripheral mode,
> > and the only way to determine the operating state is to peek into the
> > core's drvdata.
> >
> > The Qualcomm glue driver is expected to make updates in the qscratch
> > register region (the "glue" region) during role switch events, but with
> > the glue and core split using the driver model, there is no reasonable
> > way to introduce listeners for mode changes.
> >
> > Split the dwc3 core platform_driver callbacks and their implementation
> > and export the implementation, to make it possible to deterministically
> > instantiate the dwc3 core as part of the dwc3 glue drivers and to
> > allow flattening of the DeviceTree representation.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > ---
> >  drivers/usb/dwc3/core.c | 149 ++++++++++++++++++++++++++++++------------------
> >  drivers/usb/dwc3/glue.h |  22 +++++++
> >  2 files changed, 117 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index c22b8678e02e..930d812a9fbb 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -36,6 +36,7 @@
> >
> >  #include "core.h"
> >  #include "gadget.h"
> > +#include "glue.h"
> >  #include "io.h"
> >
> >  #include "debug.h"
> > @@ -2129,27 +2130,14 @@ static int dwc3_get_num_ports(struct dwc3 *dwc)
> >  	return 0;
> >  }
> >
> > -static int dwc3_probe(struct platform_device *pdev)
> > +int dwc3_init(struct dwc3 *dwc, struct resource *res)
> 
> Maybe need data struct, which included res. It may need pass down more
> information in future.
> 

Per this series we're resolving the race conditions stemming from
dwc3-qcom and core having different lifetimes, but more importantly it
will allow us to propose a mechanism for invoking role-switch events in
the glue code from the core - i.e. we are planning to add at least one
more argument here; so this proposal makes sense.

> >  {
> > -	struct device		*dev = &pdev->dev;
> > -	struct resource		*res, dwc_res;
> > +	struct device		*dev = dwc->dev;
> > +	struct resource		dwc_res;
> >  	unsigned int		hw_mode;
> >  	void __iomem		*regs;
> > -	struct dwc3		*dwc;
> >  	int			ret;
> >
> ...
> 
> > +++ b/drivers/usb/dwc3/glue.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * glue.h - DesignWare USB3 DRD glue header
> > + */
> > +
> > +#ifndef __DRIVERS_USB_DWC3_GLUE_H
> > +#define __DRIVERS_USB_DWC3_GLUE_H
> > +
> > +#include <linux/types.h>
> > +#include "core.h"
> > +
> > +int dwc3_init(struct dwc3 *dwc, struct resource *res);
> > +void dwc3_uninit(struct dwc3 *dwc);
> > +
> > +int dwc3_runtime_suspend(struct dwc3 *dwc);
> > +int dwc3_runtime_resume(struct dwc3 *dwc);
> > +int dwc3_runtime_idle(struct dwc3 *dwc);
> > +int dwc3_suspend(struct dwc3 *dwc);
> > +int dwc3_resume(struct dwc3 *dwc);
> > +void dwc3_complete(struct dwc3 *dwc);
> 
> dwc3_usb_*()? There may be name polution in future. There are many IPs
> created by dwc.
> 

I thought dwc3 was uniquely associated with USB, but I don't have any
objections to your proposal.

Thanks,
Bjorn

> Frank
> 
> > +
> > +#endif
> >
> > --
> > 2.45.2
> >
Bjorn Andersson Jan. 14, 2025, 11:04 p.m. UTC | #3
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
Thinh Nguyen Jan. 15, 2025, 1:56 a.m. UTC | #4
On Tue, Jan 14, 2025, Bjorn Andersson wrote:
> On Tue, Jan 14, 2025 at 02:43:59PM -0500, Frank Li wrote:
> > ...
> > 
> > > +++ b/drivers/usb/dwc3/glue.h
> > > @@ -0,0 +1,22 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * glue.h - DesignWare USB3 DRD glue header
> > > + */
> > > +
> > > +#ifndef __DRIVERS_USB_DWC3_GLUE_H
> > > +#define __DRIVERS_USB_DWC3_GLUE_H
> > > +
> > > +#include <linux/types.h>
> > > +#include "core.h"
> > > +
> > > +int dwc3_init(struct dwc3 *dwc, struct resource *res);
> > > +void dwc3_uninit(struct dwc3 *dwc);
> > > +
> > > +int dwc3_runtime_suspend(struct dwc3 *dwc);
> > > +int dwc3_runtime_resume(struct dwc3 *dwc);
> > > +int dwc3_runtime_idle(struct dwc3 *dwc);
> > > +int dwc3_suspend(struct dwc3 *dwc);
> > > +int dwc3_resume(struct dwc3 *dwc);
> > > +void dwc3_complete(struct dwc3 *dwc);
> > 
> > dwc3_usb_*()? There may be name polution in future. There are many IPs
> > created by dwc.
> > 
> 
> I thought dwc3 was uniquely associated with USB, but I don't have any
> objections to your proposal.
> 
> Thanks,
> Bjorn
> 

Regarding this naming, let's keep them as what Bjorn has as they are
more consistent to what we're doing in the driver. We can revise in the
future should we need to.

A couple of things I would like to rename is the "dwc3_uninit" to
perhaps use dwc3_exit or dwc3_cleanup instead; the other is the
dwc3_complete(), perhaps dwc3_pm_complete() for clarity.

Also, should we add CONFIG_PM_SLEEP guards for these exported pm ops?

Thanks,
Thinh
Thinh Nguyen Jan. 15, 2025, 2:59 a.m. UTC | #5
On Tue, Jan 14, 2025, Thinh Nguyen wrote:
> On Tue, Jan 14, 2025, Bjorn Andersson wrote:
> 
> Also, should we add CONFIG_PM_SLEEP guards for these exported pm ops?

Ignore this. I thought I saw the code outside of the guards.

BR,
Thinh
Rob Herring Jan. 15, 2025, 6:51 p.m. UTC | #6
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