[PATCH 3/5] usb: dwc3-generic: Add dependency on SIMPLE_BUS

[Marek Vasut]

On 1/24/23 03:53, Kunihiko Hayashi wrote:
> Hi Marek,

Hello Hayashi-san,

> On 2023/01/24 0:49, Marek Vasut wrote:
>> On 1/23/23 06:01, Kunihiko Hayashi wrote:
>>> Hi Marek,
>>
>> Hello Hayashi-san,
>>
>> [...]
>>
>>>> On the other hand, the PXS2 controller for example is not a bus:
>>>>
>>>> arch/arm/dts/uniphier-pxs2.dtsi:
>>>> 596 _usb0: usb at 65a00000 {
>>>> 597     compatible = "socionext,uniphier-dwc3", "snps,dwc3";
>>>> 598     status = "disabled";
>>>> 599     reg = <0x65a00000 0xcd00>;
>>>> ...
>>>> 610 };
>>>
>>> I understand. However, this node isn't used in u-boot.
>>> (see below for details)
>>>
>>>>>> Is this needed for socionext dwc3 variant to handle the simple-mfd in
>>>>>> e.g. arch/arm/dts/uniphier-pxs3.dtsi :
>>>>>>
>>>>>> 614 usb-glue at 65b00000 {
>>>>>> 615     compatible = "socionext,uniphier-pxs3-dwc3-glue",
>>>>>> 616              "simple-mfd";
>>>>>>
>>>>>> ?
>>>>>
>>>>> In case of U-Boot, the glue driver is probed by:
>>>>>
>>>>>        /* FIXME: U-Boot own node */
>>>>>        usb at 65b00000 {
>>>>>                compatible = "socionext,uniphier-pxs3-dwc3";
>>>>>
>>>>> And dwc3-uniphier is also declared as UCLASS_SIMPLE_BUS.
>>>>> Even if using "simple-mfd", this is included in
>>>>> drivers/core/simple-bus.c
>>>>> which is declared as UCLASS_SIMPLE_BUS.
>>>>
>>>> If I understand this correctly, node compatible with
>>>> "socionext,uniphier-pxs3-dwc3-glue" is not used at all , right ?
>>>
>>> Yes.
>>> Original uniphier devicetree has the following usb nodes.
>>>
>>>       usb at 65a00000 {
>>>           compatible = "snps,dwc3";
>>>       };
>>>       usb-glue at 65b00000 {
>>>           compatible = "socionext,uniphier-pxs3-dwc3-glue", 
>>> "simple-mfd";
>>>       };
>>>
>>> However, U-Boot dwc3-generic needs to put dwc3 node under the glue node.
>>
>> Have a look at arch/arm/dts/imx8mq.dtsi which does not use any glue
>> node, the snps,dwc3 compatible node is directly placed on the soc at 0 bus:
>>
>> 1417
>> 1418         usb_dwc3_0: usb at 38100000 {
>> 1419             compatible = "fsl,imx8mq-dwc3", "snps,dwc3";
>> 1420             reg = <0x38100000 0x10000>;
>> 1421             clocks = <&clk IMX8MQ_CLK_USB1_CTRL_ROOT>,
>>
>>> Due to this restriction, there is another usb node dedicated to u-boot.
>>>
>>>       /* FIXME: U-Boot own node */
>>>       usb at 65b00000 { /* glue */
>>>           compatible = "socionext,uniphier-pxs3-dwc3";
>>>
>>>           dwc3 at 65a00000 {
>>>               compatible = "snps,dwc3";
>>>           };
>>>       };
>>>
>>> So instead of "socionext,uniphier-pxs3-dwc3-glue", the glue driver
>>> uses "socionext,uniphier-pxs3-dwc3" in U-Boot.
>>
>> Can we use the same method as imx8mq.dtsi uses, to avoid this special
>> node workaround ?
> 
> Umm, it's curious. It seems imx8mq doesn't have any glue registers and 
> defines
> dwc3 core registers directly.
> 
> On the other hand, "fsl,imx8mq-dwc3" is included in dwc3-generic.c, though,
> dwc3_glue_bind() calls the driver that the child node shows.
> 
>      ofnode_for_each_subnode(node, dev_ofnode(parent)) {
>          ...
>          device_bind_driver_to_node(parent, driver, name,
>                                     node, &dev);
>          ...
>      }
> 
> Maybe the child node expects the driver that "snps,dwc3" indicates.
> 
> If we use the same method as imx8mq.dtsi, I think there is no way to probe
> the driver for "snps,dwc3" in dwc3-generic.c.
> 
> BTW, xchi-dwc3.c can handle "snps,dwc3", but it seems this doesn't use
> usb/dwc3/ functions (so I'm confusing it).

Let's try the following, please have a look at this change:

https://source.denx.de/u-boot/custodians/u-boot-usb/-/commit/028ad9536e501986f4e19d27f462f81a9ea70bad

The change is difficult to read, use 'git show -w' to ignore space 
changes, that might help.

The idea is that the dwc3-generic.c (or dwc3-uniphier.c , placement does 
not really matter) binds to "socionext,uniphier-pxs3-dwc3-glue" 
compatible first.

Then, the dwc3_glue_ops is extended with a new callback 
glue_get_ctrl_dev, which returns the pointer to controller DT node (the 
node compatible with "socionext,uniphier-dwc3"). This is used in 
dwc3_glue_bind(), which either uses the current implementation with a 
loop over all subnodes and tries to find the controller subnode, OR, 
calls the glue_get_ctrl_dev callback, obtains controller device node 
pointer that way, and runs the inner loop of dwc3_glue_bind() (now 
dwc3_glue_bind_common()) only on that pointer.

In either case, the dwc3-generic driver is bound to the controller. You 
might need to re-use this trick in dwc3_glue_probe() too.

This should allow you to get rid of the custom DT node too.

Does this work ?