[PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[Chunfeng Yun]

Due to some old SoCs with shared t-phy only support force-mode switch, and
can't use compatible to distinguish between shared and non-shared t-phy,
add a property to supported it.
But now prefer to use "mediatek,syscon-type" on new SoC as far as possible.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
index 2bb91542e984..eedba5b7025e 100644
--- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
+++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
@@ -235,6 +235,12 @@ patternProperties:
           Specify the flag to enable BC1.2 if support it
         type: boolean
 
+      mediatek,force-mode:
+        description:
+          Use force mode to switch shared phy mode, perfer to use the bellow
+          property "mediatek,syscon-type" if the hardware support it.
+        type: boolean
+
       mediatek,syscon-type:
         $ref: /schemas/types.yaml#/definitions/phandle-array
         maxItems: 1
-- 
2.18.0

[PATCH 2/2] phy: mediatek: tphy: add support force phy mode switch

[Chunfeng Yun]

this is used to be compatible with old SoCs, such as mt8195, which shares t-phy
between ssusb and pcie controller, usually, it's default mode is pcie rc mode,
and could use force mode to switch into ssusb mode, because pericfg layer doesn't
provide mode switch, also no efuse or jumper can be used;
Note: don't use this way on new SoCs, use pericfg layer's mode switch instead
(by perperty "mediatek,syscon-type").

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/phy/mediatek/phy-mtk-tphy.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c b/drivers/phy/mediatek/phy-mtk-tphy.c
index 05eab9014132..a4746f6cb8a1 100644
--- a/drivers/phy/mediatek/phy-mtk-tphy.c
+++ b/drivers/phy/mediatek/phy-mtk-tphy.c
@@ -185,6 +185,10 @@
 #define P3D_RG_CDR_BIR_LTD1		GENMASK(28, 24)
 #define P3D_RG_CDR_BIR_LTD0		GENMASK(12, 8)
 
+#define U3P_U3_PHYD_TOP1		0x100
+#define P3D_RG_PHY_MODE			GENMASK(2, 1)
+#define P3D_RG_FORCE_PHY_MODE		BIT(0)
+
 #define U3P_U3_PHYD_RXDET1		0x128
 #define P3D_RG_RXDET_STB2_SET		GENMASK(17, 9)
 
@@ -327,6 +331,7 @@ struct mtk_phy_instance {
 	int discth;
 	int pre_emphasis;
 	bool bc12_en;
+	bool type_force_mode;
 };
 
 struct mtk_tphy {
@@ -768,6 +773,23 @@ static void u3_phy_instance_init(struct mtk_tphy *tphy,
 	void __iomem *phya = u3_banks->phya;
 	void __iomem *phyd = u3_banks->phyd;
 
+	if (instance->type_force_mode) {
+		/* force phy as usb mode, default is pcie rc mode */
+		mtk_phy_update_field(phyd + U3P_U3_PHYD_TOP1, P3D_RG_PHY_MODE, 1);
+		mtk_phy_set_bits(phyd + U3P_U3_PHYD_TOP1, P3D_RG_FORCE_PHY_MODE);
+		/* power down phy by ip and pipe reset */
+		mtk_phy_set_bits(u3_banks->chip + U3P_U3_CHIP_GPIO_CTLD,
+				 P3C_FORCE_IP_SW_RST | P3C_MCU_BUS_CK_GATE_EN);
+		mtk_phy_set_bits(u3_banks->chip + U3P_U3_CHIP_GPIO_CTLE,
+				 P3C_RG_SWRST_U3_PHYD | P3C_RG_SWRST_U3_PHYD_FORCE_EN);
+		udelay(10);
+		/* power on phy again */
+		mtk_phy_clear_bits(u3_banks->chip + U3P_U3_CHIP_GPIO_CTLD,
+				   P3C_FORCE_IP_SW_RST | P3C_MCU_BUS_CK_GATE_EN);
+		mtk_phy_clear_bits(u3_banks->chip + U3P_U3_CHIP_GPIO_CTLE,
+				   P3C_RG_SWRST_U3_PHYD | P3C_RG_SWRST_U3_PHYD_FORCE_EN);
+	}
+
 	/* gating PCIe Analog XTAL clock */
 	mtk_phy_set_bits(u3_banks->spllc + U3P_SPLLC_XTALCTL3,
 			 XC3_RG_U3_XTAL_RX_PWD | XC3_RG_U3_FRC_XTAL_RX_PWD);
@@ -1120,6 +1142,9 @@ static void phy_parse_property(struct mtk_tphy *tphy,
 {
 	struct device *dev = &instance->phy->dev;
 
+	if (instance->type == PHY_TYPE_USB3)
+		instance->type_force_mode = device_property_read_bool(dev, "mediatek,force-mode");
+
 	if (instance->type != PHY_TYPE_USB2)
 		return;
 
-- 
2.18.0

Re: [PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[Krzysztof Kozlowski]

On 25/11/2023 02:23, Chunfeng Yun wrote:
> Due to some old SoCs with shared t-phy only support force-mode switch, and
> can't use compatible to distinguish between shared and non-shared t-phy,
> add a property to supported it.
> But now prefer to use "mediatek,syscon-type" on new SoC as far as possible.
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>  Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> index 2bb91542e984..eedba5b7025e 100644
> --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> @@ -235,6 +235,12 @@ patternProperties:
>            Specify the flag to enable BC1.2 if support it
>          type: boolean
>  
> +      mediatek,force-mode:
> +        description:
> +          Use force mode to switch shared phy mode, perfer to use the bellow

I still do not understand what is the "force mode" you want to use. What
modes do you have? What are the characteristics of force mode?

Also, please run spellcheck.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[Macpaul Lin]

On 11/25/23 18:37, Krzysztof Kozlowski and Chunfeng Yun wrote:
> 	
> 
> External email : Please do not click links or open attachments until you 
> have verified the sender or the content.
> 
> On 25/11/2023 02:23, Chunfeng Yun wrote:
>> Due to some old SoCs with shared t-phy only support force-mode switch, and
>> can't use compatible to distinguish between shared and non-shared t-phy,
>> add a property to supported it.
>> But now prefer to use "mediatek,syscon-type" on new SoC as far as possible.
>> 
>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
>> ---
>>  Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>> index 2bb91542e984..eedba5b7025e 100644
>> --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>> +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>> @@ -235,6 +235,12 @@ patternProperties:
>>            Specify the flag to enable BC1.2 if support it
>>          type: boolean
>>  
>> +      mediatek,force-mode:
>> +        description:
>> +          Use force mode to switch shared phy mode, perfer to use the bellow
> 
> I still do not understand what is the "force mode" you want to use. What
> modes do you have? What are the characteristics of force mode?
> 
> Also, please run spellcheck.
> 
> Best regards,
> Krzysztof
> 

Thanks!

1. I've tested this patch and it could solve the clock unstable for
XHCI1 on mt8195 or mt8395 during system boot up or during
unload/reload the phy driver.

The error message has been listed as follows.

[   13.849936][   T72] xhci-mtk 11290000.usb: supply vbus not found, 
using dummy regulator
[   13.851300][   T72] xhci-mtk 11290000.usb: uwk - reg:0x400, version:104
[   13.852624][   T72] xhci-mtk 11290000.usb: xHCI Host Controller
[   13.853393][   T72] xhci-mtk 11290000.usb: new USB bus registered, 
assigned bus number 3
[   13.874490][   T72] xhci-mtk 11290000.usb: clocks are not stable (0x3d0f)
[   13.875369][   T72] xhci-mtk 11290000.usb: can't setup: -110
[   13.876091][   T72] xhci-mtk 11290000.usb: USB bus 3 deregistered
[   13.877081][   T72] xhci-mtk: probe of 11290000.usb failed with error 
-110

2. This is a fix patch to XHCI1 since MT8195 has been upstream.
Please add "Fixes:" tags and "Cc: stable@kernel.org" to back ward
porting to previous stable trees.

For example, add
Fixes: 6b5ef194611e5 ("phy: mediatek: tphy: remove macros to prepare 
bitfield")
is suggested since the force-mode was missing in the previous
implementation which causes hardware function was abnormal.
However, add
Fixes: 33d18746fa514 ("phy: phy-mtk-tphy: use new io helpers to access 
register")
will be better since the USB support for mt8195 is already enabled in 
late 2021.

3. How about we revise the description as follows for more precisely?

mediatek,force-mode:
   description:
     The force mode is used to manually switch the shared PHY mode
     between USB and PCIe. When force-mode is set, the USB 3.0 mode
     will be selected. This is typically required for older SoCs
     that do not automatically manage PHY mode switching.
     For newer SoCs that support it, it is preferable to use the
     "mediatek,syscon-type" property instead.
   type: boolean

Thanks,
Macpaul Lin

Re: [PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[Krzysztof Kozlowski]

On 27/11/2023 08:09, Macpaul Lin wrote:
> On 11/25/23 18:37, Krzysztof Kozlowski and Chunfeng Yun wrote:
>> 	
>>
>> External email : Please do not click links or open attachments until you 
>> have verified the sender or the content.
>>
>> On 25/11/2023 02:23, Chunfeng Yun wrote:
>>> Due to some old SoCs with shared t-phy only support force-mode switch, and
>>> can't use compatible to distinguish between shared and non-shared t-phy,
>>> add a property to supported it.
>>> But now prefer to use "mediatek,syscon-type" on new SoC as far as possible.
>>>
>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
>>> ---
>>>  Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>>> index 2bb91542e984..eedba5b7025e 100644
>>> --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>>> +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>>> @@ -235,6 +235,12 @@ patternProperties:
>>>            Specify the flag to enable BC1.2 if support it
>>>          type: boolean
>>>  
>>> +      mediatek,force-mode:
>>> +        description:
>>> +          Use force mode to switch shared phy mode, perfer to use the bellow
>>
>> I still do not understand what is the "force mode" you want to use. What
>> modes do you have? What are the characteristics of force mode?
>>
>> Also, please run spellcheck.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Thanks!
> 
> 1. I've tested this patch and it could solve the clock unstable for
> XHCI1 on mt8195 or mt8395 during system boot up or during
> unload/reload the phy driver.
> 
> The error message has been listed as follows.
> 
> [   13.849936][   T72] xhci-mtk 11290000.usb: supply vbus not found, 
> using dummy regulator
> [   13.851300][   T72] xhci-mtk 11290000.usb: uwk - reg:0x400, version:104
> [   13.852624][   T72] xhci-mtk 11290000.usb: xHCI Host Controller
> [   13.853393][   T72] xhci-mtk 11290000.usb: new USB bus registered, 
> assigned bus number 3
> [   13.874490][   T72] xhci-mtk 11290000.usb: clocks are not stable (0x3d0f)
> [   13.875369][   T72] xhci-mtk 11290000.usb: can't setup: -110
> [   13.876091][   T72] xhci-mtk 11290000.usb: USB bus 3 deregistered
> [   13.877081][   T72] xhci-mtk: probe of 11290000.usb failed with error 
> -110
> 
> 2. This is a fix patch to XHCI1 since MT8195 has been upstream.
> Please add "Fixes:" tags and "Cc: stable@kernel.org" to back ward
> porting to previous stable trees.
> 
> For example, add
> Fixes: 6b5ef194611e5 ("phy: mediatek: tphy: remove macros to prepare 
> bitfield")
> is suggested since the force-mode was missing in the previous
> implementation which causes hardware function was abnormal.
> However, add
> Fixes: 33d18746fa514 ("phy: phy-mtk-tphy: use new io helpers to access 
> register")
> will be better since the USB support for mt8195 is already enabled in 
> late 2021.
> 
> 3. How about we revise the description as follows for more precisely?
> 
> mediatek,force-mode:
>    description:
>      The force mode is used to manually switch the shared PHY mode
>      between USB and PCIe. When force-mode is set, the USB 3.0 mode
>      will be selected. This is typically required for older SoCs
>      that do not automatically manage PHY mode switching.
>      For newer SoCs that support it, it is preferable to use the
>      "mediatek,syscon-type" property instead.
>    type: boolean

Again, what is force-mode? It looks like you wrote bindings for the
driver behavior. Bindings describe hardware, not how the driver should
behave. The property might be reasonable, but you must describe here
hardware characteristics/issue/etc.

Also, your driver change suggests this is compatible specific, so why it
cannot be deduced from compatible?

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[AngeloGioacchino Del Regno]

Il 27/11/23 08:21, Krzysztof Kozlowski ha scritto:
> On 27/11/2023 08:09, Macpaul Lin wrote:
>> On 11/25/23 18:37, Krzysztof Kozlowski and Chunfeng Yun wrote:
>>> 	
>>>
>>> External email : Please do not click links or open attachments until you
>>> have verified the sender or the content.
>>>
>>> On 25/11/2023 02:23, Chunfeng Yun wrote:
>>>> Due to some old SoCs with shared t-phy only support force-mode switch, and
>>>> can't use compatible to distinguish between shared and non-shared t-phy,
>>>> add a property to supported it.
>>>> But now prefer to use "mediatek,syscon-type" on new SoC as far as possible.

Two questions:
1. Why is it *not* possible to use the compatible string to distinguish between
    shared and non-shared T-PHYs?
2. If we really can't use compatibles, what's the reason why we can't use the
    "mediatek,syscon-type" property?

Regards,
Angelo

>>>>
>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
>>>> ---
>>>>   Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>>>> index 2bb91542e984..eedba5b7025e 100644
>>>> --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>>>> +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>>>> @@ -235,6 +235,12 @@ patternProperties:
>>>>             Specify the flag to enable BC1.2 if support it
>>>>           type: boolean
>>>>   
>>>> +      mediatek,force-mode:
>>>> +        description:
>>>> +          Use force mode to switch shared phy mode, perfer to use the bellow
>>>

Re: [PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[Chunfeng Yun (云春峰)]

On Mon, 2023-11-27 at 14:37 +0100, AngeloGioacchino Del Regno wrote:
> Il 27/11/23 08:21, Krzysztof Kozlowski ha scritto:
> > On 27/11/2023 08:09, Macpaul Lin wrote:
> > > On 11/25/23 18:37, Krzysztof Kozlowski and Chunfeng Yun wrote:
> > > > 	
> > > > 
> > > > External email : Please do not click links or open attachments
> > > > until you
> > > > have verified the sender or the content.
> > > > 
> > > > On 25/11/2023 02:23, Chunfeng Yun wrote:
> > > > > Due to some old SoCs with shared t-phy only support force-
> > > > > mode switch, and
> > > > > can't use compatible to distinguish between shared and non-
> > > > > shared t-phy,
> > > > > add a property to supported it.
> > > > > But now prefer to use "mediatek,syscon-type" on new SoC as
> > > > > far as possible.
> 
> Two questions:
> 1. Why is it *not* possible to use the compatible string to
> distinguish between
>     shared and non-shared T-PHYs?
There may be shared t-phy and non-shared t-phy at the same time on the
SoC.

> 2. If we really can't use compatibles, what's the reason why we can't
> use the
>     "mediatek,syscon-type" property?
It need hardware support it, it uses a top pericfg register to tell the
phy that the mode we want before power on the phy controller.


> 
> Regards,
> Angelo
> 
> > > > > 
> > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > ---
> > > > >   Documentation/devicetree/bindings/phy/mediatek,tphy.yaml |
> > > > > 6 ++++++
> > > > >   1 file changed, 6 insertions(+)
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> > > > > b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> > > > > index 2bb91542e984..eedba5b7025e 100644
> > > > > ---
> > > > > a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> > > > > +++
> > > > > b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> > > > > @@ -235,6 +235,12 @@ patternProperties:
> > > > >             Specify the flag to enable BC1.2 if support it
> > > > >           type: boolean
> > > > >   
> > > > > +      mediatek,force-mode:
> > > > > +        description:
> > > > > +          Use force mode to switch shared phy mode, perfer
> > > > > to use the bellow
> 
> 

Re: [PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[Chunfeng Yun (云春峰)]

On Mon, 2023-11-27 at 08:21 +0100, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 27/11/2023 08:09, Macpaul Lin wrote:
> > On 11/25/23 18:37, Krzysztof Kozlowski and Chunfeng Yun wrote:
> >> 
> >>
> >> External email : Please do not click links or open attachments
> until you 
> >> have verified the sender or the content.
> >>
> >> On 25/11/2023 02:23, Chunfeng Yun wrote:
> >>> Due to some old SoCs with shared t-phy only support force-mode
> switch, and
> >>> can't use compatible to distinguish between shared and non-shared 
> t-phy,
> >>> add a property to supported it.
> >>> But now prefer to use "mediatek,syscon-type" on new SoC as far as
> possible.
> >>>
> >>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 6
> ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git
> a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> >>> index 2bb91542e984..eedba5b7025e 100644
> >>> --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> >>> +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> >>> @@ -235,6 +235,12 @@ patternProperties:
> >>>            Specify the flag to enable BC1.2 if support it
> >>>          type: boolean
> >>>  
> >>> +      mediatek,force-mode:
> >>> +        description:
> >>> +          Use force mode to switch shared phy mode, perfer to
> use the bellow
> >>
> >> I still do not understand what is the "force mode" you want to
> use. What
> >> modes do you have? What are the characteristics of force mode?
> >>
> >> Also, please run spellcheck.
> >>
> >> Best regards,
> >> Krzysztof
> >>
> > 
> > Thanks!
> > 
> > 1. I've tested this patch and it could solve the clock unstable for
> > XHCI1 on mt8195 or mt8395 during system boot up or during
> > unload/reload the phy driver.
> > 
> > The error message has been listed as follows.
> > 
> > [   13.849936][   T72] xhci-mtk 11290000.usb: supply vbus not
> found, 
> > using dummy regulator
> > [   13.851300][   T72] xhci-mtk 11290000.usb: uwk - reg:0x400,
> version:104
> > [   13.852624][   T72] xhci-mtk 11290000.usb: xHCI Host Controller
> > [   13.853393][   T72] xhci-mtk 11290000.usb: new USB bus
> registered, 
> > assigned bus number 3
> > [   13.874490][   T72] xhci-mtk 11290000.usb: clocks are not stable
> (0x3d0f)
> > [   13.875369][   T72] xhci-mtk 11290000.usb: can't setup: -110
> > [   13.876091][   T72] xhci-mtk 11290000.usb: USB bus 3
> deregistered
> > [   13.877081][   T72] xhci-mtk: probe of 11290000.usb failed with
> error 
> > -110
> > 
> > 2. This is a fix patch to XHCI1 since MT8195 has been upstream.
> > Please add "Fixes:" tags and "Cc: stable@kernel.org" to back ward
> > porting to previous stable trees.
> > 
> > For example, add
> > Fixes: 6b5ef194611e5 ("phy: mediatek: tphy: remove macros to
> prepare 
> > bitfield")
> > is suggested since the force-mode was missing in the previous
> > implementation which causes hardware function was abnormal.
> > However, add
> > Fixes: 33d18746fa514 ("phy: phy-mtk-tphy: use new io helpers to
> access 
> > register")
> > will be better since the USB support for mt8195 is already enabled
> in 
> > late 2021.
> > 
> > 3. How about we revise the description as follows for more
> precisely?
> > 
> > mediatek,force-mode:
> >    description:
> >      The force mode is used to manually switch the shared PHY mode
> >      between USB and PCIe. When force-mode is set, the USB 3.0 mode
> >      will be selected. This is typically required for older SoCs
> >      that do not automatically manage PHY mode switching.
> >      For newer SoCs that support it, it is preferable to use the
> >      "mediatek,syscon-type" property instead.
> >    type: boolean
> 
> Again, what is force-mode? 
Our DE describe this behavior as force-mode, as you see, the driver
power down controller and reset pipe to set the mode directly we want,
but usually the phy controller switch to the mode automatically
according to the external signal, e.g. trapping pin, efuse etc.

> It looks like you wrote bindings for the
> driver behavior. Bindings describe hardware, not how the driver
> should
> behave. The property might be reasonable, but you must describe here
> hardware characteristics/issue/etc.
> 
> Also, your driver change suggests this is compatible specific, so why
> it
> cannot be deduced from compatible?
that is because on the same SoC, some t-phy supported USB3 are shared
with PCIe, but others are not, use the SoC's compatible can not
distinguish them.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[Krzysztof Kozlowski]

On 30/11/2023 02:51, Chunfeng Yun (云春峰) wrote:
>>> 3. How about we revise the description as follows for more
>> precisely?
>>>
>>> mediatek,force-mode:
>>>    description:
>>>      The force mode is used to manually switch the shared PHY mode
>>>      between USB and PCIe. When force-mode is set, the USB 3.0 mode
>>>      will be selected. This is typically required for older SoCs
>>>      that do not automatically manage PHY mode switching.
>>>      For newer SoCs that support it, it is preferable to use the
>>>      "mediatek,syscon-type" property instead.
>>>    type: boolean
>>
>> Again, what is force-mode? 
> Our DE describe this behavior as force-mode, as you see, the driver

What is "DE"?

> power down controller and reset pipe to set the mode directly we want,

So force-mode is driver behavior?

> but usually the phy controller switch to the mode automatically
> according to the external signal, e.g. trapping pin, efuse etc.
> 
>> It looks like you wrote bindings for the
>> driver behavior. Bindings describe hardware, not how the driver
>> should
>> behave. The property might be reasonable, but you must describe here
>> hardware characteristics/issue/etc.

You must address this, in such case.



Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[Chunfeng Yun (云春峰)]

On Thu, 2023-11-30 at 09:03 +0100, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 30/11/2023 02:51, Chunfeng Yun (云春峰) wrote:
> >>> 3. How about we revise the description as follows for more
> >> precisely?
> >>>
> >>> mediatek,force-mode:
> >>>    description:
> >>>      The force mode is used to manually switch the shared PHY
> mode
> >>>      between USB and PCIe. When force-mode is set, the USB 3.0
> mode
> >>>      will be selected. This is typically required for older SoCs
> >>>      that do not automatically manage PHY mode switching.
> >>>      For newer SoCs that support it, it is preferable to use the
> >>>      "mediatek,syscon-type" property instead.
> >>>    type: boolean
> >>
> >> Again, what is force-mode? 
> > Our DE describe this behavior as force-mode, as you see, the driver
> 
> What is "DE"?
Hardware designer

> 
> > power down controller and reset pipe to set the mode directly we
> want,
> 
> So force-mode is driver behavior?
hardware supported, need software to set some registers

> 
> > but usually the phy controller switch to the mode automatically
> > according to the external signal, e.g. trapping pin, efuse etc.
> > 
> >> It looks like you wrote bindings for the
> >> driver behavior. Bindings describe hardware, not how the driver
> >> should
> >> behave. The property might be reasonable, but you must describe
> here
> >> hardware characteristics/issue/etc.
> 
> You must address this, in such case.
OK, I'll modify the description

Thanks

> 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[Chunfeng Yun (云春峰)]

On Mon, 2023-11-27 at 15:09 +0800, Macpaul Lin wrote:
> On 11/25/23 18:37, Krzysztof Kozlowski and Chunfeng Yun wrote:
> > 	
> > 
> > External email : Please do not click links or open attachments
> > until you 
> > have verified the sender or the content.
> > 
> > On 25/11/2023 02:23, Chunfeng Yun wrote:
> > > Due to some old SoCs with shared t-phy only support force-mode
> > > switch, and
> > > can't use compatible to distinguish between shared and non-shared 
> > > t-phy,
> > > add a property to supported it.
> > > But now prefer to use "mediatek,syscon-type" on new SoC as far as
> > > possible.
> > > 
> > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > ---
> > >  Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 6
> > > ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> > > b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> > > index 2bb91542e984..eedba5b7025e 100644
> > > --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> > > +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
> > > @@ -235,6 +235,12 @@ patternProperties:
> > >            Specify the flag to enable BC1.2 if support it
> > >          type: boolean
> > >  
> > > +      mediatek,force-mode:
> > > +        description:
> > > +          Use force mode to switch shared phy mode, perfer to
> > > use the bellow
> > 
> > I still do not understand what is the "force mode" you want to use.
> > What
> > modes do you have? What are the characteristics of force mode?
> > 
> > Also, please run spellcheck.
> > 
> > Best regards,
> > Krzysztof
> > 
> 
> Thanks!
> 
> 1. I've tested this patch and it could solve the clock unstable for
> XHCI1 on mt8195 or mt8395 during system boot up or during
> unload/reload the phy driver.
> 
> The error message has been listed as follows.
> 
> [   13.849936][   T72] xhci-mtk 11290000.usb: supply vbus not found, 
> using dummy regulator
> [   13.851300][   T72] xhci-mtk 11290000.usb: uwk - reg:0x400,
> version:104
> [   13.852624][   T72] xhci-mtk 11290000.usb: xHCI Host Controller
> [   13.853393][   T72] xhci-mtk 11290000.usb: new USB bus
> registered, 
> assigned bus number 3
> [   13.874490][   T72] xhci-mtk 11290000.usb: clocks are not stable
> (0x3d0f)
> [   13.875369][   T72] xhci-mtk 11290000.usb: can't setup: -110
> [   13.876091][   T72] xhci-mtk 11290000.usb: USB bus 3 deregistered
> [   13.877081][   T72] xhci-mtk: probe of 11290000.usb failed with
> error 
> -110
> 
> 2. This is a fix patch to XHCI1 since MT8195 has been upstream.
> Please add "Fixes:" tags and "Cc: stable@kernel.org" to back ward
> porting to previous stable trees.
> 
> For example, add
> Fixes: 6b5ef194611e5 ("phy: mediatek: tphy: remove macros to prepare 
> bitfield")
> is suggested since the force-mode was missing in the previous
> implementation which causes hardware function was abnormal.
> However, add
> Fixes: 33d18746fa514 ("phy: phy-mtk-tphy: use new io helpers to
> access 
> register")
> will be better since the USB support for mt8195 is already enabled
> in 
> late 2021.
It don't take into account this case, it assume that only use the
default mode (PCIe).

> 
> 3. How about we revise the description as follows for more precisely?
> 
> mediatek,force-mode:
>    description:
>      The force mode is used to manually switch the shared PHY mode
>      between USB and PCIe. When force-mode is set, the USB 3.0 mode
>      will be selected. 
seems not right, the mode/type is selected by the phy consumer.

> This is typically required for older SoCs
>      that do not automatically manage PHY mode switching.
>      For newer SoCs that support it, it is preferable to use the
>      "mediatek,syscon-type" property instead.
>    type: boolean

> 
> Thanks,
> Macpaul Lin