2020-10-14 17:16:06

by Serge Semin

[permalink] [raw]
Subject: Re: [PATCH 20/20] arch: dts: Fix DWC USB3 DT nodes name

On Wed, Oct 14, 2020 at 05:09:37PM +0300, Felipe Balbi wrote:
>
> Hi Serge,
>
> Serge Semin <[email protected]> writes:
> > In accordance with the DWC USB3 bindings the corresponding node name is
> > suppose to comply with Generic USB HCD DT schema, which requires the USB
>

> DWC3 is not a simple HDC, though.

Yeah, strictly speaking it is equipped with a lot of vendor-specific stuff,
which are tuned by the DWC USB3 driver in the kernel. But after that the
controller is registered as xhci-hcd device so it's serviced by the xHCI driver,
which then registers the HCD device so the corresponding DT node is supposed
to be compatible with the next bindings: usb/usb-hcd.yaml, usb/usb-xhci.yaml
and usb/snps,dwc3,yaml. I've created the later one so to validate the denoted
compatibility.

>
> > nodes to have the name acceptable by the regexp: "^usb(@.*)?" . But a lot
> > of the DWC USB3-compatible nodes defined in the ARM/ARM64 DTS files have
> > name as "^dwc3@.*" or "^usb[1-3]@.*" or even "^dwusb@.*", which will cause
> > the dtbs_check procedure failure. Let's fix the nodes naming to be
> > compatible with the DWC USB3 DT schema to make dtbs_check happy.
> >
> > Note we don't change the DWC USB3-compatible nodes names of
> > arch/arm64/boot/dts/apm/{apm-storm.dtsi,apm-shadowcat.dtsi} since the
> > in-source comment says that the nodes name need to be preserved as
> > "^dwusb@.*" for some backward compatibility.
>

> interesting, compatibility with what? Some debugfs files, perhaps? :-)

Don't really know.) In my experience the worst type of such compatibility is
connected with some bootloader magic, which may add/remove/modify properties
to nodes with pre-defined names.

-Sergey

>
> In any case, I don't have any problems with this, so I'll let other
> folks comment.
>
> --
> balbi



2020-10-14 18:38:48

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH 20/20] arch: dts: Fix DWC USB3 DT nodes name

On Wed, Oct 14, 2020 at 9:37 AM Serge Semin
<[email protected]> wrote:
>
> On Wed, Oct 14, 2020 at 05:09:37PM +0300, Felipe Balbi wrote:
> >
> > Hi Serge,
> >
> > Serge Semin <[email protected]> writes:
> > > In accordance with the DWC USB3 bindings the corresponding node name is
> > > suppose to comply with Generic USB HCD DT schema, which requires the USB
> >
>
> > DWC3 is not a simple HDC, though.
>
> Yeah, strictly speaking it is equipped with a lot of vendor-specific stuff,
> which are tuned by the DWC USB3 driver in the kernel. But after that the
> controller is registered as xhci-hcd device so it's serviced by the xHCI driver,
> which then registers the HCD device so the corresponding DT node is supposed
> to be compatible with the next bindings: usb/usb-hcd.yaml, usb/usb-xhci.yaml
> and usb/snps,dwc3,yaml. I've created the later one so to validate the denoted
> compatibility.
>
> >
> > > nodes to have the name acceptable by the regexp: "^usb(@.*)?" . But a lot
> > > of the DWC USB3-compatible nodes defined in the ARM/ARM64 DTS files have
> > > name as "^dwc3@.*" or "^usb[1-3]@.*" or even "^dwusb@.*", which will cause
> > > the dtbs_check procedure failure. Let's fix the nodes naming to be
> > > compatible with the DWC USB3 DT schema to make dtbs_check happy.
> > >
> > > Note we don't change the DWC USB3-compatible nodes names of
> > > arch/arm64/boot/dts/apm/{apm-storm.dtsi,apm-shadowcat.dtsi} since the
> > > in-source comment says that the nodes name need to be preserved as
> > > "^dwusb@.*" for some backward compatibility.
> >
>
> > interesting, compatibility with what? Some debugfs files, perhaps? :-)
>
> Don't really know.) In my experience the worst type of such compatibility is
> connected with some bootloader magic, which may add/remove/modify properties
> to nodes with pre-defined names.

I seriously doubt anyone is using the APM machines with DT (even ACPI
is somewhat doubtful). I say change them. Or remove the dts files and
see what happens. Either way it can always be reverted.

Rob

2020-10-15 07:19:47

by Serge Semin

[permalink] [raw]
Subject: Re: [PATCH 20/20] arch: dts: Fix DWC USB3 DT nodes name

On Wed, Oct 14, 2020 at 01:35:16PM -0500, Rob Herring wrote:
> On Wed, Oct 14, 2020 at 9:37 AM Serge Semin
> <[email protected]> wrote:
> >
> > On Wed, Oct 14, 2020 at 05:09:37PM +0300, Felipe Balbi wrote:
> > >
> > > Hi Serge,
> > >
> > > Serge Semin <[email protected]> writes:
> > > > In accordance with the DWC USB3 bindings the corresponding node name is
> > > > suppose to comply with Generic USB HCD DT schema, which requires the USB
> > >
> >
> > > DWC3 is not a simple HDC, though.
> >
> > Yeah, strictly speaking it is equipped with a lot of vendor-specific stuff,
> > which are tuned by the DWC USB3 driver in the kernel. But after that the
> > controller is registered as xhci-hcd device so it's serviced by the xHCI driver,
> > which then registers the HCD device so the corresponding DT node is supposed
> > to be compatible with the next bindings: usb/usb-hcd.yaml, usb/usb-xhci.yaml
> > and usb/snps,dwc3,yaml. I've created the later one so to validate the denoted
> > compatibility.
> >
> > >
> > > > nodes to have the name acceptable by the regexp: "^usb(@.*)?" . But a lot
> > > > of the DWC USB3-compatible nodes defined in the ARM/ARM64 DTS files have
> > > > name as "^dwc3@.*" or "^usb[1-3]@.*" or even "^dwusb@.*", which will cause
> > > > the dtbs_check procedure failure. Let's fix the nodes naming to be
> > > > compatible with the DWC USB3 DT schema to make dtbs_check happy.
> > > >
> > > > Note we don't change the DWC USB3-compatible nodes names of
> > > > arch/arm64/boot/dts/apm/{apm-storm.dtsi,apm-shadowcat.dtsi} since the
> > > > in-source comment says that the nodes name need to be preserved as
> > > > "^dwusb@.*" for some backward compatibility.
> > >
> >
> > > interesting, compatibility with what? Some debugfs files, perhaps? :-)
> >
> > Don't really know.) In my experience the worst type of such compatibility is
> > connected with some bootloader magic, which may add/remove/modify properties
> > to nodes with pre-defined names.
>

> I seriously doubt anyone is using the APM machines with DT (even ACPI
> is somewhat doubtful). I say change them. Or remove the dts files and
> see what happens. Either way it can always be reverted.

Ok. I'll change them in v3.

-Sergey

>
> Rob

2020-10-15 12:01:56

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH 20/20] arch: dts: Fix DWC USB3 DT nodes name

Serge Semin <[email protected]> writes:

> On Wed, Oct 14, 2020 at 05:09:37PM +0300, Felipe Balbi wrote:
>>
>> Hi Serge,
>>
>> Serge Semin <[email protected]> writes:
>> > In accordance with the DWC USB3 bindings the corresponding node name is
>> > suppose to comply with Generic USB HCD DT schema, which requires the USB
>>
>
>> DWC3 is not a simple HDC, though.
>
> Yeah, strictly speaking it is equipped with a lot of vendor-specific stuff,
> which are tuned by the DWC USB3 driver in the kernel. But after that the
> controller is registered as xhci-hcd device so it's serviced by the xHCI driver,

in Dual-role or host-only builds, that's correct. We can also have
peripheral-only builds (both SW or HW versions) which means xhci isn't
even in the picture.

--
balbi


Attachments:
signature.asc (873.00 B)

2020-10-15 13:55:40

by Serge Semin

[permalink] [raw]
Subject: Re: [PATCH 20/20] arch: dts: Fix DWC USB3 DT nodes name

On Thu, Oct 15, 2020 at 01:15:37PM +0300, Felipe Balbi wrote:
> Serge Semin <[email protected]> writes:
>
> > On Wed, Oct 14, 2020 at 05:09:37PM +0300, Felipe Balbi wrote:
> >>
> >> Hi Serge,
> >>
> >> Serge Semin <[email protected]> writes:
> >> > In accordance with the DWC USB3 bindings the corresponding node name is
> >> > suppose to comply with Generic USB HCD DT schema, which requires the USB
> >>
> >
> >> DWC3 is not a simple HDC, though.
> >
> > Yeah, strictly speaking it is equipped with a lot of vendor-specific stuff,
> > which are tuned by the DWC USB3 driver in the kernel. But after that the
> > controller is registered as xhci-hcd device so it's serviced by the xHCI driver,
>

> in Dual-role or host-only builds, that's correct. We can also have
> peripheral-only builds (both SW or HW versions) which means xhci isn't
> even in the picture.

Hm, good point. In that case perhaps we'll need to apply the xHCI DT schema
conditionally. Like this:

- allOf:
- - $ref: usb-xhci.yaml#
+ allOf:
+ - if:
+ properties:
+ dr_mode:
+ const: peripheral
+ then:
+ $ref: usb-hcd.yaml#
+ else:
+ $ref: usb-xhci.yaml#

Note, we need to have the peripheral device being compatible with the
usb-hcd.yaml schema to support the maximum-speed, dr_mode properties and to
comply with the USB node naming constraint.

-Sergey

>
> --
> balbi


2020-10-15 18:28:25

by Maxime Ripard

[permalink] [raw]
Subject: Re: [PATCH 20/20] arch: dts: Fix DWC USB3 DT nodes name

On Thu, Oct 15, 2020 at 01:15:37PM +0300, Felipe Balbi wrote:
> Serge Semin <[email protected]> writes:
>
> > On Wed, Oct 14, 2020 at 05:09:37PM +0300, Felipe Balbi wrote:
> >>
> >> Hi Serge,
> >>
> >> Serge Semin <[email protected]> writes:
> >> > In accordance with the DWC USB3 bindings the corresponding node name is
> >> > suppose to comply with Generic USB HCD DT schema, which requires the USB
> >>
> >
> >> DWC3 is not a simple HDC, though.
> >
> > Yeah, strictly speaking it is equipped with a lot of vendor-specific stuff,
> > which are tuned by the DWC USB3 driver in the kernel. But after that the
> > controller is registered as xhci-hcd device so it's serviced by the xHCI driver,
>
> in Dual-role or host-only builds, that's correct. We can also have
> peripheral-only builds (both SW or HW versions) which means xhci isn't
> even in the picture.

It doesn't really matter though, or at least it does for what the new
name might be, but the old one currently used is still pretty bad.

The DT spec says that the node name is the class of the device. "usb" as
the HCD binding mandates is one, but the current nodes currently have
completely different names from one DT to another - which is already an
issue - and most of them have dwc3 or some variant of it, which doesn't
really qualify for a class name.

Maxime


Attachments:
(No filename) (1.37 kB)
signature.asc (235.00 B)
Download all attachments