diff mbox series

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

Message ID 20210617194154.2397-5-linux.amoon@gmail.com
State RFC
Headers show
Series Meson-8b and Meson-gxbb USB phy code re-structure | expand

Commit Message

Anand Moon June 17, 2021, 7:41 p.m. UTC
Reorder the code for phy set_mode in .set_mode callback function.
For now configure the phy in host mode.

Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 62 ++++++++++++++++++--------
 1 file changed, 43 insertions(+), 19 deletions(-)

Comments

Martin Blumenstingl June 17, 2021, 10:16 p.m. UTC | #1
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.


Best regards,
Martin
Anand Moon June 18, 2021, 1:19 p.m. UTC | #2
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
Martin Blumenstingl June 18, 2021, 8:01 p.m. UTC | #3
Hi Anand,

On Fri, Jun 18, 2021 at 3:19 PM Anand Moon <linux.amoon@gmail.com> wrote:
[...]
> > 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.
first of all: sorry that I have not linked my source of information previously:
- Meson8b: [0]
- Meson8 and Meson8m2: [1]

Unfortunately I don't have any datasheet or "better documentation" of
how the registers should be programmed.
This is why I am a bit defensive when I am asked to change something
there - as I simply have no way of knowing if the changes are good or
not. I can only tell whether they're "identical" or "different" from
what the vendor kernel does.

[...]
> +       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;
According to the vendor kernel this should only be applied to
"host-only" USB_PORT_IDX_B (which is usb1 in the mainline .dtsi).
Based on that I think it's not correct to apply this for DEVICE and OTG modes.

The vendor kernel does not configure REG_ADP_BC_ID_PULLUP anywhere.
Also DCD_ENABLE is only ever set to 0 (while you are enabling it now), see [2].

As mentioned before: all I can say about this patch is that it
programs the registers differently than the vendor kernel does.
From your description I am not sure if you are now getting different
behavior on Odroid-C1 with this patch (compared to what we had
before).


Best regards,
Martin


[0] https://github.com/endlessm/linux-meson/blob/03393bb8e8478626e03ee93b0a2a225d6de242b5/arch/arm/mach-meson8b/usbclock.c#L120
[1] https://github.com/endlessm/linux-meson/blob/03393bb8e8478626e03ee93b0a2a225d6de242b5/arch/arm/mach-meson8/usbclock.c#L120
[2] https://github.com/endlessm/linux-meson/blob/d6e13c220931110fe676ede6da69fc61a7cb04b6/drivers/amlogic/usb/dwc_otg/310/dwc_otg_pcd.c#L71
Anand Moon June 21, 2021, 7:20 a.m. UTC | #4
Hi Martin,

On Sat, 19 Jun 2021 at 01:31, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> Hi Anand,
>
> On Fri, Jun 18, 2021 at 3:19 PM Anand Moon <linux.amoon@gmail.com> wrote:
> [...]
> > > 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.
> first of all: sorry that I have not linked my source of information previously:
> - Meson8b: [0]
> - Meson8 and Meson8m2: [1]
>
> Unfortunately I don't have any datasheet or "better documentation" of
> how the registers should be programmed.
> This is why I am a bit defensive when I am asked to change something
> there - as I simply have no way of knowing if the changes are good or
> not. I can only tell whether they're "identical" or "different" from
> what the vendor kernel does.
>
> [...]
> > +       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;
> According to the vendor kernel this should only be applied to
> "host-only" USB_PORT_IDX_B (which is usb1 in the mainline .dtsi).
> Based on that I think it's not correct to apply this for DEVICE and OTG modes.
>
> The vendor kernel does not configure REG_ADP_BC_ID_PULLUP anywhere.
> Also DCD_ENABLE is only ever set to 0 (while you are enabling it now), see [2].
>
> As mentioned before: all I can say about this patch is that it
> programs the registers differently than the vendor kernel does.
> From your description I am not sure if you are now getting different
> behavior on Odroid-C1 with this patch (compared to what we had
> before).
>

In order to enable USB phy we probably need to do a little bit
differently than the vendor kernel.
Yes I have observed many configuration parameters are missing.

OTG port on Odroid C1+ and Odroid C2 server two purposes
1 > It could act as USB host port.
2 > It could be used as USB power on the devices, just like Raspberry pi.
      What I meant is we need some driver code to protect the power to SbC.

So depending on the mode, it gets configured host mode or PCD mode,
I am not completely sure right now.
So I saw your work on extcon, that's the reason I would like to void
any changes PHY right now.

I observe some failures like below.
[    6.013859] dwc2 c9000000.usb: DWC OTG HCD URB Dequeue
[    6.013897] dwc2 c9000000.usb: Called usb_hcd_giveback_urb()
[    6.013902] dwc2 c9000000.usb:   urb->status = -115

Still investigating this issue,

>
> Best regards,
> Martin
>
Yes, I will go through the features for debugging in the future.
>
> [0] https://github.com/endlessm/linux-meson/blob/03393bb8e8478626e03ee93b0a2a225d6de242b5/arch/arm/mach-meson8b/usbclock.c#L120
> [1] https://github.com/endlessm/linux-meson/blob/03393bb8e8478626e03ee93b0a2a225d6de242b5/arch/arm/mach-meson8/usbclock.c#L120
> [2] https://github.com/endlessm/linux-meson/blob/d6e13c220931110fe676ede6da69fc61a7cb04b6/drivers/amlogic/usb/dwc_otg/310/dwc_otg_pcd.c#L71

Thanks
-Anand
Martin Blumenstingl June 22, 2021, 8:27 p.m. UTC | #5
Hi Anand,

On Mon, Jun 21, 2021 at 9:20 AM Anand Moon <linux.amoon@gmail.com> wrote:
[...]
> In order to enable USB phy we probably need to do a little bit
> differently than the vendor kernel.
I agree with you here, the vendor kernel is skipping most of the
frameworks which we have available in mainline Linux

> OTG port on Odroid C1+ and Odroid C2 server two purposes
> 1 > It could act as USB host port.
> 2 > It could be used as USB power on the devices, just like Raspberry pi.
>       What I meant is we need some driver code to protect the power to SbC.
yep, so we need something that controls mode switching
depending on the mode the VBUS regulator needs to be enabled (HOST) or
disabled (DEVICE/PERIPHERAL).
VBUS control however is not part of the PHY - in mainline Linux either
the dwc2 driver or a USB connector driver are taking care of it.

> So I saw your work on extcon, that's the reason I would like to void
> any changes PHY right now.
I believe that this specific PHY is unrelated to mode switching.
Either it automatically detects the mode and changes it's settings
internally or the PHY settings are the same for both modes.
Since VBUS control is not the responsibility of the PHY this is
certainly possible

My changes are not ready yet but I'll ping you once I have something to test

> I observe some failures like below.
> [    6.013859] dwc2 c9000000.usb: DWC OTG HCD URB Dequeue
> [    6.013897] dwc2 c9000000.usb: Called usb_hcd_giveback_urb()
> [    6.013902] dwc2 c9000000.usb:   urb->status = -115
-115 is -EINPROGRESS but I am not sure if/when that's a bad thing or
to be expected


Best regards,
Martin
diff mbox series

Patch

diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c b/drivers/phy/amlogic/phy-meson8b-usb2.c
index 2b32a3eabccf..18e0986f6ed2 100644
--- a/drivers/phy/amlogic/phy-meson8b-usb2.c
+++ b/drivers/phy/amlogic/phy-meson8b-usb2.c
@@ -173,11 +173,43 @@  static int phy_meson8b_usb2_exit(struct phy *phy)
 	return 0;
 }
 
-static int phy_meson8b_usb2_power_on(struct phy *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;
+	}
+
+	priv->dr_mode = mode;
+
+	return 0;
+}
+
+static int phy_meson8b_usb2_power_on(struct phy *phy)
+{
+	struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
+	int ret;
+
 	regmap_update_bits(priv->regmap, REG_CONFIG, REG_CONFIG_CLK_32k_ALTSEL,
 			   REG_CONFIG_CLK_32k_ALTSEL);
 
@@ -197,24 +229,12 @@  static int phy_meson8b_usb2_power_on(struct phy *phy)
 	regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_SOF_TOGGLE_OUT,
 			   REG_CTRL_SOF_TOGGLE_OUT);
 
-	if (priv->dr_mode == USB_DR_MODE_HOST) {
-		regmap_update_bits(priv->regmap, REG_DBG_UART,
-				   REG_DBG_UART_SET_IDDQ, 0);
-
-		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");
-				clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
-				return -EINVAL;
-			}
-		}
+	ret = phy_meson8b_usb2_setmode(phy, priv->dr_mode, 0);
+	if (ret) {
+		phy_meson8b_usb2_power_off(phy);
+		dev_err(&phy->dev, "Failed to initialize PHY with mode %d\n",
+			priv->dr_mode);
+		return ret;
 	}
 
 	return 0;
@@ -238,6 +258,7 @@  static const struct phy_ops phy_meson8b_usb2_ops = {
 	.exit           = phy_meson8b_usb2_exit,
 	.power_on	= phy_meson8b_usb2_power_on,
 	.power_off	= phy_meson8b_usb2_power_off,
+	.set_mode	= phy_meson8b_usb2_setmode,
 	.owner		= THIS_MODULE,
 };
 
@@ -261,6 +282,9 @@  static int phy_meson8b_usb2_probe(struct platform_device *pdev)
 	if (!priv->match)
 		return -ENODEV;
 
+	/* start in host mode */
+	priv->dr_mode = PHY_MODE_USB_HOST;
+
 	priv->regmap = devm_regmap_init_mmio(&pdev->dev, base,
 					     &phy_meson8b_usb2_regmap_conf);
 	if (IS_ERR(priv->regmap))