Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function
From: Anand Moon
Date: Sun Jun 27 2021 - 16:07:29 EST
Hi Martin,
On Thu, 24 Jun 2021 at 20:24, Anand Moon <linux.amoon@xxxxxxxxx> wrote:
>
> Hi Martin,
>
> On Wed, 23 Jun 2021 at 01:42, Martin Blumenstingl
> <martin.blumenstingl@xxxxxxxxxxxxxx> wrote:
> >
> > Hi Anand,
> >
> > On Mon, Jun 21, 2021 at 9:16 AM Anand Moon <linux.amoon@xxxxxxxxx> wrote:
> > [...]
> > > 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.
>
> > [...]
> > > > > > > - 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");
> >
> > 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