[RFC] phy: phy-mtk-tphy: Make USB PHY work on MT6735

Commit Message

Yassine Oudjana Aug. 15, 2022, 9:58 p.m. UTC

The USB PHY on MT6735 seems to be a TPHY V1, but the FMREG base
here is 0xf00 rather than 0x100. Since it is definitely not a V2
or later as it has shared registers, would this mean that it is
an even earlier revision, or one between V1 and V2? Or is the
address used currently in this driver wrong?

Furthermore, there is one additional step needed in the power on
sequence to make USB work on MT6735, which is to set the 10th bit
of U3P_U2PHYDTM1. This was found through trial and error by adding
the power on sequence from the downstream driver then stripping
parts of it until a critical part is found. This specific part[1]
was found under a comment saying "force enter device mode", but
there are other bits that are cleared/set under the same comment,
some of which are set in the same write as this bit, so it is
unclear whether this bit is the one (or the only one) responsible
for "forcing enter device mode". There is no documentation on this
register (or any port registers for that matter) so this bit's
name and function are unknown. Does anyone have more information
on this? And if no information were to be found, would leaving
this as a magic value, or with the placeholder name currently used
(P2C_FORCE_ENTER_DEVICE_MODE) be acceptable?

[1] https://gitlab.com/Tooniis/linux-samsung-grandpplte/-/blob/master/drivers/misc/mediatek/usb20/mt6735/usb20_phy.c#L404
---
 drivers/phy/mediatek/phy-mtk-tphy.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Chunfeng Yun (云春峰) Aug. 16, 2022, 7:58 a.m. UTC | #1

On Mon, 2022-08-15 at 22:58 +0100, Yassine Oudjana wrote:
> The USB PHY on MT6735 seems to be a TPHY V1, but the FMREG base
> here is 0xf00 rather than 0x100. Since it is definitely not a V2
> or later as it has shared registers, would this mean that it is
> an even earlier revision, or one between V1 and V2? Or is the
> address used currently in this driver wrong?
> 
> Furthermore, there is one additional step needed in the power on
> sequence to make USB work on MT6735, which is to set the 10th bit
> of U3P_U2PHYDTM1. This was found through trial and error by adding
> the power on sequence from the downstream driver then stripping
> parts of it until a critical part is found. This specific part[1]
> was found under a comment saying "force enter device mode", but
> there are other bits that are cleared/set under the same comment,
> some of which are set in the same write as this bit, so it is
> unclear whether this bit is the one (or the only one) responsible
> for "forcing enter device mode". There is no documentation on this
> register (or any port registers for that matter) so this bit's
> name and function are unknown. Does anyone have more information
> on this? And if no information were to be found, would leaving
> this as a magic value, or with the placeholder name currently used
> (P2C_FORCE_ENTER_DEVICE_MODE) be acceptable?

Can't use force mode, there is side-effect with dual-role controller.

> 
> [1] 
> https://urldefense.com/v3/__https://gitlab.com/Tooniis/linux-samsung-grandpplte/-/blob/master/drivers/misc/mediatek/usb20/mt6735/usb20_phy.c*L404__;Iw!!CTRNKA9wMg0ARbw!26SfFWZy9e7D178mgOIWZ6iv4hM4OCTq4cndOspiaTxIQgvATHYlMsPCd4YiWq3S3Fal$
>  
> ---
>  drivers/phy/mediatek/phy-mtk-tphy.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c
> b/drivers/phy/mediatek/phy-mtk-tphy.c
> index 8ee7682b8e93..f54b8a9872bf 100644
> --- a/drivers/phy/mediatek/phy-mtk-tphy.c
> +++ b/drivers/phy/mediatek/phy-mtk-tphy.c
> @@ -23,7 +23,7 @@
>  /* version V1 sub-banks offset base address */
>  /* banks shared by multiple phys */
>  #define SSUSB_SIFSLV_V1_SPLLC		0x000	/* shared by
> u3 phys */
> -#define SSUSB_SIFSLV_V1_U2FREQ		0x100	/* shared by
> u2 phys */
> +#define SSUSB_SIFSLV_V1_U2FREQ		0xf00	/* shared by
> u2 phys */
>  #define SSUSB_SIFSLV_V1_CHIP		0x300	/* shared by u3 phys
> */
>  /* u2 phy bank */
>  #define SSUSB_SIFSLV_V1_U2PHY_COM	0x000
> @@ -119,6 +119,7 @@
>  
>  #define U3P_U2PHYDTM1		0x06C
>  #define P2C_RG_UART_EN			BIT(16)
> +#define P2C_FORCE_ENTER_DEVICE_MODE	BIT(10)
>  #define P2C_FORCE_IDDIG		BIT(9)
>  #define P2C_RG_VBUSVALID		BIT(5)
>  #define P2C_RG_SESSEND			BIT(4)
> @@ -579,6 +580,8 @@ static void u2_phy_instance_power_on(struct
> mtk_tphy *tphy,
>  		mtk_phy_set_bits(com + U3P_U2PHYDTM0, P2C_RG_SUSPENDM |
> P2C_FORCE_SUSPENDM);
>  	}
>  	dev_dbg(tphy->dev, "%s(%d)\n", __func__, index);
> +
> +	mtk_phy_set_bits(com + U3P_U2PHYDTM1,
> P2C_FORCE_ENTER_DEVICE_MODE);
>  }
>  
>  static void u2_phy_instance_power_off(struct mtk_tphy *tphy,

Patch

diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c b/drivers/phy/mediatek/phy-mtk-tphy.c
index 8ee7682b8e93..f54b8a9872bf 100644
--- a/drivers/phy/mediatek/phy-mtk-tphy.c
+++ b/drivers/phy/mediatek/phy-mtk-tphy.c
@@ -23,7 +23,7 @@ 
 /* version V1 sub-banks offset base address */
 /* banks shared by multiple phys */
 #define SSUSB_SIFSLV_V1_SPLLC		0x000	/* shared by u3 phys */
-#define SSUSB_SIFSLV_V1_U2FREQ		0x100	/* shared by u2 phys */
+#define SSUSB_SIFSLV_V1_U2FREQ		0xf00	/* shared by u2 phys */
 #define SSUSB_SIFSLV_V1_CHIP		0x300	/* shared by u3 phys */
 /* u2 phy bank */
 #define SSUSB_SIFSLV_V1_U2PHY_COM	0x000
@@ -119,6 +119,7 @@ 
 
 #define U3P_U2PHYDTM1		0x06C
 #define P2C_RG_UART_EN			BIT(16)
+#define P2C_FORCE_ENTER_DEVICE_MODE	BIT(10)
 #define P2C_FORCE_IDDIG		BIT(9)
 #define P2C_RG_VBUSVALID		BIT(5)
 #define P2C_RG_SESSEND			BIT(4)
@@ -579,6 +580,8 @@  static void u2_phy_instance_power_on(struct mtk_tphy *tphy,
 		mtk_phy_set_bits(com + U3P_U2PHYDTM0, P2C_RG_SUSPENDM | P2C_FORCE_SUSPENDM);
 	}
 	dev_dbg(tphy->dev, "%s(%d)\n", __func__, index);
+
+	mtk_phy_set_bits(com + U3P_U2PHYDTM1, P2C_FORCE_ENTER_DEVICE_MODE);
 }
 
 static void u2_phy_instance_power_off(struct mtk_tphy *tphy,