Re: [RFCv1 4/8] phy: amlogic: meson8b-usb2: Use phy set_mode callback function

From: Anand Moon <linux.amoon@gmail.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.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 4/8] phy: amlogic: meson8b-usb2: Use phy set_mode callback function
Date: Fri, 18 Jun 2021 18:49:05 +0530	[thread overview]
Message-ID: <CANAwSgQVgKUoTpfaJyfxdphqc6M=Oq6jj5zZ7An9St7PdzQHYA@mail.gmail.com> (raw)
In-Reply-To: <CAFBinCB1rrmJ5=M0tSGS_47BarFcrs2Kz5qFzrHw8+OEYxX3DA@mail.gmail.com>

hi Martin.

Thanks for your review comments.

On Fri, 18 Jun 2021 at 03:46, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> Hi Anand,
>
> On Thu, Jun 17, 2021 at 9:43 PM Anand Moon <linux.amoon@gmail.com> wrote:
> >
> > Reorder the code for phy set_mode in .set_mode callback function.
> > For now configure the phy in host mode.
> as mentioned in the cover-letter: to my knowledge these register bits
> are "static"
> The settings for dr_mode == USB_DR_MODE_HOST mainly apply to the
> second PHY (usb1_phy)
>
> [...]
> > +static int phy_meson8b_usb2_setmode(struct phy *phy, enum phy_mode mode,
> > +                                   int submode)
> >  {
> >         struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
> >         u32 reg;
> >
> > +       switch (mode) {
> > +       case PHY_MODE_USB_HOST:
> > +               if (priv->match->host_enable_aca) {
> > +                       regmap_update_bits(priv->regmap, REG_ADP_BC,
> > +                                          REG_ADP_BC_ACA_ENABLE,
> > +                                          REG_ADP_BC_ACA_ENABLE);
> > +
> > +                       udelay(ACA_ENABLE_COMPLETE_TIME);
> > +
> > +                       regmap_read(priv->regmap, REG_ADP_BC, &reg);
> > +                       if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
> > +                               dev_warn(&phy->dev, "USB ID detect failed!\n");
> > +                               return -EINVAL;
> > +                       }
> > +               }
> > +               break;
> > +       default:
> > +               dev_warn(&phy->dev, "USB ID detect failed to setnode! %d\n", mode);
> > +               return -EINVAL;
> I have tested this driver already with PHY_MODE_USB_DEVICE (on my
> Odroid-C1) so I don't see why we should drop support for this
> Also if we want runtime mode switching in this driver then we would
> need to undo the changes from "case PHY_MODE_USB_HOST" above
>
> I suggest dropping this patch until we know for sure if and which
> registers need to be updated based on the DR mode.

Yes, I have observed this, Can you give these small changes a try?
With the below changes, I got the  PHY_MODE_USB_DEVICE support working.

Here is the boot log of odroid c1+
[0] https://pastebin.com/pCXLS5Vu

$ lsusb -t
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
        |__ Port 3: Dev 4, If 0, Class=Mass Storage, Driver=usb-storage, 480M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Human Interface Device, Driver=usbhid, 12M
    |__ Port 1: Dev 2, If 1, Class=Human Interface Device, Driver=usbhid, 12M

 git diff drivers/phy/amlogic/phy-meson8b-usb2.c

diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c
b/drivers/phy/amlogic/phy-meson8b-usb2.c
index bd624781d914..9b79e86d7a0d 100644
--- a/drivers/phy/amlogic/phy-meson8b-usb2.c
+++ b/drivers/phy/amlogic/phy-meson8b-usb2.c
@@ -204,6 +204,22 @@ static int phy_meson8b_usb2_setmode(struct phy
*phy, enum phy_mode mode,
                        }
                }
                break;
+       case PHY_MODE_USB_DEVICE:
+       case PHY_MODE_USB_OTG:
+               regmap_update_bits(priv->regmap, REG_ADP_BC,
+                                  REG_ADP_BC_DCD_ENABLE,
+                                  REG_ADP_BC_DCD_ENABLE);
+
+               udelay(ACA_ENABLE_COMPLETE_TIME);
+
+               regmap_read(priv->regmap, REG_ADP_BC, &reg);
+               if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
+                       dev_warn(&phy->dev, "USB ID detect failed!\n");
+                       return -EINVAL;
+               }
+               regmap_update_bits(priv->regmap, REG_ADP_BC,
+                                  REG_ADP_BC_ID_PULLUP, REG_ADP_BC_ID_PULLUP);
+               break;
        default:
                dev_warn(&phy->dev, "USB ID detect failed to setnode!
%d\n", mode);
                return -EINVAL;
>
>
> Best regards,
> Martin

Thanks


-Anand
