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

[Kunihiko Hayashi]

Hi Marek,

On 2023/01/25 10:38, Marek Vasut wrote:
> 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.

Thanks a lot for your work!

> 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.

I understand your patch adds trying to call "glue_get_ctrl_dev" to get
controller device node before finding subnode,

> 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 ?

I'll try this soon.

Thank you,

---
Best Regards
Kunihiko Hayashi