Hi Martin,

On Thu, 24 Jun 2021 at 20:24, Anand Moon [off-list ref] wrote:

Hi Martin,

On Wed, 23 Jun 2021 at 01:42, Martin Blumenstingl
[off-list ref] wrote:

quoted

Hi Anand,

On Mon, Jun 21, 2021 at 9:16 AM Anand Moon [off-list ref] wrote:
[...]

quoted

Ok Thanks for the inputs. got your point.

I was also looking into Amlogic source code for reset. (aml_cbus_update_bits)
[0] https://github.com/khadas/linux/blob/khadas-vims-4.9.y/drivers/amlogic/usb/phy/phy-aml-new-usb.c
is there some feature to iomap the USB with cbus?

for that specific code: that's what we do inside drivers/reset/reset-meson.c
Amlogic's vendor kernel uses an increment of 4 bytes per value, so
0x1102 translates to 0x4408

then in mainline's meson8b.dtsi we have:
    compatible = "amlogic,meson8b-reset";
    reg = <0x4404 0x9c>;
as you can see 0x4408 is part of the reset controller node.

next in include/dt-bindings/reset/amlogic,meson8b-reset.h we have:
    #define RESET_USB_OTG                 34

the register used for reset line 34 is translated using:
    0x4404 (first register) + 4 (4 * reset line / 32 = 1) = 0x4408
then the bit inside this register is translated using:
    reset line % 32 = 2

that's how we express aml_cbus_update_bits(0x1102, 0x1<<2, 0x1<<2); in
the mainline kernel (by going through the reset subsystem)

Thank you very much for clearing my long-standing doubt on *reset
logic* on Amlogic SoC.

quoted

[...]

quoted

quoted

quoted

quoted

quoted

-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");

I think this breaks compatibility with existing .dtbs and our
dt-bindings (as we're not documenting a "reset-names" property).
What is the goal of this one?

OK, If we pass NULL over here there is the possibility
USB phy will not get registered.

I don't understand why - with NULL everything is working fine for me.
Also no matter which name you give to the reset line (in reset-names),
it will be the same reset line in all cases. If it's the same reset
line before and after: why is this needed?

I need to investigate this reset feature. With my setup with current changes
after I update the below.
-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
+       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
        if (PTR_ERR(priv->reset) == -EPROBE_DEFER)
                return PTR_ERR(priv->reset);

Reset will break the USB initialization, see below output.

interesting, I have not seen that USB problem before and neither is
Kernel CI seeing it: [0]
Is it only happening with this patch or did you also see it before?

Yes, it could happen with this patch but It could be also linked to
reorder the phy configuration.
See below logs, when core reset fails on USB PHY no USB is getting registered.

[    1.267620] dwc2 c9040000.usb: mapped PA c9040000 to VA (ptrval)
[    1.267768] dwc2 c9040000.usb: Looking up vusb_d-supply from device tree
[    1.267783] dwc2 c9040000.usb: Looking up vusb_d-supply property in
node /soc/usb@c9040000 failed
[    1.267814] dwc2 c9040000.usb: supply vusb_d not found, using dummy regulator
[    1.267940] dwc2 c9040000.usb: Looking up vusb_a-supply from device tree
[    1.267954] dwc2 c9040000.usb: Looking up vusb_a-supply property in
node /soc/usb@c9040000 failed
[    1.267975] dwc2 c9040000.usb: supply vusb_a not found, using dummy regulator
[    1.268037] dwc2 c9040000.usb: registering common handler for irq35
[    1.268090] dwc2 c9040000.usb: Looking up vbus-supply from device tree
[    1.268102] dwc2 c9040000.usb: Looking up vbus-supply property in
node /soc/usb@c9040000 failed
[    1.269267] dwc2 c9040000.usb: Core Release: 3.10a (snpsid=4f54310a)
[    1.273185] dwc2 c9040000.usb: dwc2_core_reset: HANG! Soft Reset
timeout GRSTCTL_CSFTRST
[    1.273510] dwc2: probe of c9040000.usb failed with error -16
[    1.275474] dwc2 c90c0000.usb: mapped PA c90c0000 to VA (ptrval)
[    1.275603] dwc2 c90c0000.usb: Looking up vusb_d-supply from device tree
[    1.275617] dwc2 c90c0000.usb: Looking up vusb_d-supply property in
node /soc/usb@c90c0000 failed
[    1.275646] dwc2 c90c0000.usb: supply vusb_d not found, using dummy regulator
[    1.275784] dwc2 c90c0000.usb: Looking up vusb_a-supply from device tree
[    1.275798] dwc2 c90c0000.usb: Looking up vusb_a-supply property in
node /soc/usb@c90c0000 failed
[    1.275819] dwc2 c90c0000.usb: supply vusb_a not found, using dummy regulator
[    1.275877] dwc2 c90c0000.usb: registering common handler for irq36
[    1.275930] dwc2 c90c0000.usb: Looking up vbus-supply from device tree
[    1.275942] dwc2 c90c0000.usb: Looking up vbus-supply property in
node /soc/usb@c90c0000 failed
[    1.277125] dwc2 c90c0000.usb: Core Release: 3.10a (snpsid=4f54310a)
[    1.281042] dwc2 c90c0000.usb: dwc2_core_reset: HANG! Soft Reset
timeout GRSTCTL_CSFTRST

 > [    1.281353] dwc2: probe of c90c0000.usb failed with error -16
 >

Sorry for the delay.
We could switch the reset logic to
*devm_reset_control_get_optional_exclusive* as below
to fix the reset line, since both the dwc2 c90c0000.usb and c9040000.usb
will have their own context to reset control register, it means the
reset line is not share
between two USB PHY nodes.

-       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+       priv->reset = devm_reset_control_get_optional_exclusive(&pdev->dev,
+                                                               "reset");

quoted

Best regards,
Martin


[0] https://storage.staging.kernelci.org/next/master/next-20210617/arm/multi_v7_defconfig+ltp-ima/gcc-8/lab-baylibre/baseline-meson8b-odroidc1.html

Thanks
-Anand

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy