mbox series

[v2,0/7] usb: dwc3: qcom: Flatten dwc3 structure

Message ID 20240811-dwc3-refactor-v2-0-91f370d61ad2@quicinc.com
Headers show
Series usb: dwc3: qcom: Flatten dwc3 structure | expand

Message

Bjorn Andersson Aug. 12, 2024, 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.

---
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 (7):
      dt-bindings: usb: snps,dwc3: Split core description
      dt-bindings: usb: Introduce qcom,snps-dwc3
      of: dynamic: Don't discard children upon node attach
      usb: dwc3: core: Expose core driver as library
      usb: dwc3: qcom: Don't reply 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    | 608 +++++++++++++++++++++
 .../devicetree/bindings/usb/snps,dwc3-common.yaml  | 417 ++++++++++++++
 .../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 +-
 .../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts     |  11 +-
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi             | 138 +++--
 drivers/of/dynamic.c                               |   1 -
 drivers/usb/dwc3/core.c                            | 169 ++++--
 drivers/usb/dwc3/core.h                            |   3 +
 drivers/usb/dwc3/dwc3-qcom.c                       | 323 ++++++++---
 13 files changed, 1483 insertions(+), 620 deletions(-)
---
base-commit: 864b1099d16fc7e332c3ad7823058c65f890486c
change-id: 20231016-dwc3-refactor-931e3b08a8b9

Best regards,

Comments

Rob Herring Aug. 12, 2024, 8:24 p.m. UTC | #1
On Sun, Aug 11, 2024 at 9:07 PM Bjorn Andersson <andersson@kernel.org> wrote:
>
> From: Bjorn Andersson <quic_bjorande@quicinc.com>
>
> When dynamically modifying DeviceTree it's useful to be able to reparent
> nodes, but of_attach_node() clear the child pointer and hence discards
> any child nodes.

of_attach_node() is kind of the legacy API. You should be using
changeset API. But I guess you really mean __of_attach_node() here
which both use.

> Retain the child pointer upon attach, so that the client code doesn't
> need to manually rebuild the tree.
>
> Current users of of_attach_node() either avoids attaching nodes with
> children or explicitly attaches nodes without children, so no impact is
> expected to current users.
>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  drivers/of/dynamic.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 110104a936d9..32e1dffd9f96 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -221,7 +221,6 @@ static void __of_attach_node(struct device_node *np)
>                         np->phandle = 0;
>         }
>
> -       np->child = NULL;
>         np->sibling = np->parent->child;
>         np->parent->child = np;
>         of_node_clear_flag(np, OF_DETACHED);

Before OF_DETACHED had a clear meaning. Now, are child nodes detached
or not? If it means not attached to the root tree, then it is
redundant having it per node. If it means parent and sibling aren't
set, then what's the point as we can just check for NULL ptrs.

This all seems fragile on top of what's already fragile.

Rob
Frank Li Aug. 13, 2024, 6:07 p.m. UTC | #2
On Sun, Aug 11, 2024 at 08:11:57PM -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.
>
> 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.

Thanks, we faced the same problem. Can you cc me next time?

Frank
>
> ---
> 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 (7):
>       dt-bindings: usb: snps,dwc3: Split core description
>       dt-bindings: usb: Introduce qcom,snps-dwc3
>       of: dynamic: Don't discard children upon node attach
>       usb: dwc3: core: Expose core driver as library
>       usb: dwc3: qcom: Don't reply 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    | 608 +++++++++++++++++++++
>  .../devicetree/bindings/usb/snps,dwc3-common.yaml  | 417 ++++++++++++++
>  .../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 +-
>  .../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts     |  11 +-
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi             | 138 +++--
>  drivers/of/dynamic.c                               |   1 -
>  drivers/usb/dwc3/core.c                            | 169 ++++--
>  drivers/usb/dwc3/core.h                            |   3 +
>  drivers/usb/dwc3/dwc3-qcom.c                       | 323 ++++++++---
>  13 files changed, 1483 insertions(+), 620 deletions(-)
> ---
> base-commit: 864b1099d16fc7e332c3ad7823058c65f890486c
> change-id: 20231016-dwc3-refactor-931e3b08a8b9
>
> Best regards,
> --
> Bjorn Andersson <quic_bjorande@quicinc.com>
>
Frank Li Oct. 8, 2024, 8:09 p.m. UTC | #3
On Tue, Aug 13, 2024 at 02:07:01PM -0400, Frank Li wrote:
> On Sun, Aug 11, 2024 at 08:11:57PM -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.
> >
> > 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.
>

Bjorn Andersson:
	Any follow up on this thread?

Frank

> Thanks, we faced the same problem. Can you cc me next time?
>
> Frank
> >
> > ---
> > 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 (7):
> >       dt-bindings: usb: snps,dwc3: Split core description
> >       dt-bindings: usb: Introduce qcom,snps-dwc3
> >       of: dynamic: Don't discard children upon node attach
> >       usb: dwc3: core: Expose core driver as library
> >       usb: dwc3: qcom: Don't reply 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    | 608 +++++++++++++++++++++
> >  .../devicetree/bindings/usb/snps,dwc3-common.yaml  | 417 ++++++++++++++
> >  .../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 +-
> >  .../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts     |  11 +-
> >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi             | 138 +++--
> >  drivers/of/dynamic.c                               |   1 -
> >  drivers/usb/dwc3/core.c                            | 169 ++++--
> >  drivers/usb/dwc3/core.h                            |   3 +
> >  drivers/usb/dwc3/dwc3-qcom.c                       | 323 ++++++++---
> >  13 files changed, 1483 insertions(+), 620 deletions(-)
> > ---
> > base-commit: 864b1099d16fc7e332c3ad7823058c65f890486c
> > change-id: 20231016-dwc3-refactor-931e3b08a8b9
> >
> > Best regards,
> > --
> > Bjorn Andersson <quic_bjorande@quicinc.com>
> >
Bjorn Andersson Oct. 9, 2024, 2:54 a.m. UTC | #4
On Tue, Oct 08, 2024 at 04:09:45PM -0400, Frank Li wrote:
> On Tue, Aug 13, 2024 at 02:07:01PM -0400, Frank Li wrote:
> > On Sun, Aug 11, 2024 at 08:11:57PM -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.
> > >
> > > 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.
> >
> 
> Bjorn Andersson:
> 	Any follow up on this thread?
> 

Thanks for reaching out, Frank. I did pick this up again a few days
back.

I'm struggling with Rob's request for not peeking into struct property
and/or utilizing overlays. Hoping to figure this out shortly, so I can
get v3 in shape.

Regards,
Bjorn