Hi Anand,
On Sat, Dec 19, 2020 at 8:53 PM Anand Moon <linux.am...@gmail.com> wrote:
[...]
> I was also looking into this issue so I made some changes in the
> phy driver to resolve the issue. Plz share your thoughts on the changes below.
first I have some questions :-)
1. do you see the same problem that Otto sees? this means: a) USB
hotplug works as long as at least one device is plugged in at boot b)
(if I understand Otto correctly then) it breaks once all USB devices
have been removed
2. does the mainline u-boot patch mentioned by Otto fix the problem
for you? according to him it fixes the problem and he did not have to
modify the USB PHY driver
> amoon@ThinkPad-T440s:~/mainline/linux-aml-5.y-devel$ git diff
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> index 7c029f552a23..363dd2ac17e6 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> @@ -20,6 +20,7 @@ usb0_phy: phy@c0000000 {
> #phy-cells = <0>;
> reg = <0x0 0xc0000000 0x0 0x20>;
> resets = <&reset RESET_USB_OTG>;
> + reset-names = "phy-reset";
> clocks = <&clkc CLKID_USB>, <&clkc CLKID_USB0>;
> clock-names = "usb_general", "usb";
> status = "disabled";
> @@ -30,6 +31,7 @@ usb1_phy: phy@c0000020 {
> #phy-cells = <0>;
> reg = <0x0 0xc0000020 0x0 0x20>;
> resets = <&reset RESET_USB_OTG>;
> + reset-names = "phy-reset";
> clocks = <&clkc CLKID_USB>, <&clkc CLKID_USB1>;
> clock-names = "usb_general", "usb";
> status = "disabled";
I don't see why the above two changes are needed
see my comment about of_reset_control_get_shared below
> diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c
> b/drivers/phy/amlogic/phy-meson8b-usb2.c
> index 03c061dd5f0d..31523becc878 100644
> --- a/drivers/phy/amlogic/phy-meson8b-usb2.c
> +++ b/drivers/phy/amlogic/phy-meson8b-usb2.c
> @@ -143,14 +143,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
> u32 reg;
> int ret;
>
> - if (!IS_ERR_OR_NULL(priv->reset)) {
> - ret = reset_control_reset(priv->reset);
> - if (ret) {
> - dev_err(&phy->dev, "Failed to trigger USB reset\n");
> - return ret;
> - }
> - }
> -
> ret = clk_prepare_enable(priv->clk_usb_general);
> if (ret) {
> dev_err(&phy->dev, "Failed to enable USB general clock\n");
> @@ -222,9 +214,23 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
> return 0;
> }
>
> +static int phy_meson8b_usb2_reset(struct phy *phy)
> +{
> + struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
> +
> + if (priv->reset) {
> + reset_control_assert(priv->reset);
> + udelay(10);
> + reset_control_deassert(priv->reset);
> + }
> +
> + return 0;
> +}
> +
> static const struct phy_ops phy_meson8b_usb2_ops = {
> .power_on = phy_meson8b_usb2_power_on,
> .power_off = phy_meson8b_usb2_power_off,
> + .reset = phy_meson8b_usb2_reset,
> .owner = THIS_MODULE,
> };
I tested this on my Odroid-C1: phy_meson8b_usb2_reset is never called
after checking the dwc2 code this is expected: only in one very
specific case the dwc2 driver calls phy_reset
can you please find out how phy_meson8b_usb2_reset is called in your kernel?
> @@ -271,6 +277,10 @@ static int phy_meson8b_usb2_probe(struct
> platform_device *pdev)
> return -EINVAL;
> }
>
> + priv->reset = of_reset_control_get_shared(pdev->dev.of_node,
> "phy-reset");
this causes a memory-leak upon driver removal
also a few lines above we are already getting the reset line, so why
is this needed?
Best regards,
Martin
