Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To: Anand Moon <linux.amoon@gmail.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>,
	Vinod Koul <vkoul@kernel.org>,
	 Neil Armstrong <narmstrong@baylibre.com>,
	Kevin Hilman <khilman@baylibre.com>,
	 Jerome Brunet <jbrunet@baylibre.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	 linux-phy@lists.infradead.org,
	 linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux-amlogic@lists.infradead.org,
	 Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function
Date: Tue, 22 Jun 2021 22:11:55 +0200	[thread overview]
Message-ID: <CAFBinCD91eYNXSqnmDKoAvJHWqqRbXVjnaq9RuRNCnip9kKqkw@mail.gmail.com> (raw)
In-Reply-To: <CANAwSgRABOyWYJPrrw64Wa6j2D94T4tybn7MHGCTbBowt7UncA@mail.gmail.com>

Hi Anand,

On Mon, Jun 21, 2021 at 9:16 AM Anand Moon <linux.amoon@gmail.com> 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)

[...]
> > > > > -       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?

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel