| Message ID | 20210617194154.2397-5-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Meson-8b and Meson-gxbb USB phy code re-structure | expand |
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, ®); > + 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
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, ®); > > + 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, ®); + 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
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, ®); > + 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
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, ®); > > + 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
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 --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, ®); + 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, ®); - 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))
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(-)