[PATCH 1/2] dt-bindings: net: motorcomm: Add chip mode cfg

[PATCH 1/2] dt-bindings: net: motorcomm: Add chip mode cfg

[Frank.Sae]

 The motorcomm phy (yt8821) supports the ability to
 config the chip mode of serdes.
 The yt8821 serdes could be set to AUTO_BX2500_SGMII or
 FORCE_BX2500.
 In AUTO_BX2500_SGMII mode, SerDes
 speed is determined by UTP, if UTP link up
 at 2.5GBASE-T, SerDes will work as
 2500BASE-X, if UTP link up at
 1000BASE-T/100BASE-Tx/10BASE-T, SerDes will work
 as SGMII.
 In FORCE_BX2500, SerDes always works
 as 2500BASE-X.

Signed-off-by: Frank.Sae <Frank.Sae@motor-comm.com>
---
 .../bindings/net/motorcomm,yt8xxx.yaml          | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
index 26688e2302ea..ba34260f889d 100644
--- a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
+++ b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
@@ -110,6 +110,23 @@ properties:
       Transmit PHY Clock delay train configuration when speed is 1000Mbps.
     type: boolean
 
+  motorcomm,chip-mode:
+    description: |
+      Only for yt8821 2.5G phy, it supports two chip working modes,
+      one is AUTO_BX2500_SGMII, the other is FORCE_BX2500.
+      If this property is not set in device tree node then driver
+      selects chip mode FORCE_BX2500 by default.
+      0: AUTO_BX2500_SGMII
+      1: FORCE_BX2500
+      In AUTO_BX2500_SGMII mode, serdes speed is determined by UTP,
+      if UTP link up at 2.5GBASE-T, serdes will work as 2500BASE-X,
+      if UTP link up at 1000BASE-T/100BASE-Tx/10BASE-T, serdes will
+      work as SGMII.
+      In FORCE_BX2500 mode, serdes always works as 2500BASE-X.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [ 0, 1 ]
+    default: 1
+
 unevaluatedProperties: false
 
 examples:
-- 
2.25.1

Re: [PATCH 1/2] dt-bindings: net: motorcomm: Add chip mode cfg

[Krzysztof Kozlowski]

On 27/07/2024 11:20, Frank.Sae wrote:
>  The motorcomm phy (yt8821) supports the ability to
>  config the chip mode of serdes.
>  The yt8821 serdes could be set to AUTO_BX2500_SGMII or
>  FORCE_BX2500.
>  In AUTO_BX2500_SGMII mode, SerDes
>  speed is determined by UTP, if UTP link up
>  at 2.5GBASE-T, SerDes will work as
>  2500BASE-X, if UTP link up at
>  1000BASE-T/100BASE-Tx/10BASE-T, SerDes will work
>  as SGMII.
>  In FORCE_BX2500, SerDes always works
>  as 2500BASE-X.

Very weird wrapping.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Signed-off-by: Frank.Sae <Frank.Sae@motor-comm.com>

Didn't you copy user-name as you name?

> ---
>  .../bindings/net/motorcomm,yt8xxx.yaml          | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

Also, your threading is completely broken. Use git send-email or b4.

> 
> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
> index 26688e2302ea..ba34260f889d 100644
> --- a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
> @@ -110,6 +110,23 @@ properties:
>        Transmit PHY Clock delay train configuration when speed is 1000Mbps.
>      type: boolean
>  
> +  motorcomm,chip-mode:
> +    description: |
> +      Only for yt8821 2.5G phy, it supports two chip working modes,

Then allOf:if:then disallowing it for the other variant?

> +      one is AUTO_BX2500_SGMII, the other is FORCE_BX2500.
> +      If this property is not set in device tree node then driver
> +      selects chip mode FORCE_BX2500 by default.

Don't repeat constraints in free form text.

> +      0: AUTO_BX2500_SGMII
> +      1: FORCE_BX2500
> +      In AUTO_BX2500_SGMII mode, serdes speed is determined by UTP,
> +      if UTP link up at 2.5GBASE-T, serdes will work as 2500BASE-X,
> +      if UTP link up at 1000BASE-T/100BASE-Tx/10BASE-T, serdes will
> +      work as SGMII.
> +      In FORCE_BX2500 mode, serdes always works as 2500BASE-X.


Explain why this is even needed and why "auto" is not correct in all
cases. In commit msg or property description.

> +    $ref: /schemas/types.yaml#/definitions/uint8

Make it a string, not uint8.


> +    enum: [ 0, 1 ]
> +    default: 1

Why 1 not 0? Auto seems more logical?

> +
>  unevaluatedProperties: false
>  
>  examples:

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: net: motorcomm: Add chip mode cfg

[Andrew Lunn]

On Sat, Jul 27, 2024 at 11:25:25AM +0200, Krzysztof Kozlowski wrote:
> On 27/07/2024 11:20, Frank.Sae wrote:
> >  The motorcomm phy (yt8821) supports the ability to
> >  config the chip mode of serdes.
> >  The yt8821 serdes could be set to AUTO_BX2500_SGMII or
> >  FORCE_BX2500.
> >  In AUTO_BX2500_SGMII mode, SerDes
> >  speed is determined by UTP, if UTP link up
> >  at 2.5GBASE-T, SerDes will work as
> >  2500BASE-X, if UTP link up at
> >  1000BASE-T/100BASE-Tx/10BASE-T, SerDes will work
> >  as SGMII.
> >  In FORCE_BX2500, SerDes always works
> >  as 2500BASE-X.

When the SERDES is forced to 2500BaseX, does it perform rate
adaptation? e.g. does it insert pause frames to slow down the MAC?

Maybe look at air_en8811h.c.

      Andrew

Re: [PATCH 1/2] dt-bindings: net: motorcomm: Add chip mode cfg

[Frank.Sae]

On 7/27/24 04:07, Andrew Lunn wrote:
> On Sat, Jul 27, 2024 at 11:25:25AM +0200, Krzysztof Kozlowski wrote:
>> On 27/07/2024 11:20, Frank.Sae wrote:
>>>   The motorcomm phy (yt8821) supports the ability to
>>>   config the chip mode of serdes.
>>>   The yt8821 serdes could be set to AUTO_BX2500_SGMII or
>>>   FORCE_BX2500.
>>>   In AUTO_BX2500_SGMII mode, SerDes
>>>   speed is determined by UTP, if UTP link up
>>>   at 2.5GBASE-T, SerDes will work as
>>>   2500BASE-X, if UTP link up at
>>>   1000BASE-T/100BASE-Tx/10BASE-T, SerDes will work
>>>   as SGMII.
>>>   In FORCE_BX2500, SerDes always works
>>>   as 2500BASE-X.
> When the SERDES is forced to 2500BaseX, does it perform rate
> adaptation? e.g. does it insert pause frames to slow down the MAC?
>
> Maybe look at air_en8811h.c.
>
>        Andrew

Yes, when the serdes is forced to 2500base-x, it inserts pause frames to
perform rate adaptation to slow down the MAC.

Re: [PATCH 1/2] dt-bindings: net: motorcomm: Add chip mode cfg

[Andrew Lunn]

On Thu, Aug 01, 2024 at 02:49:12AM -0700, Frank.Sae wrote:
> 
> On 7/27/24 02:25, Krzysztof Kozlowski wrote:
> 
>     On 27/07/2024 11:20, Frank.Sae wrote:
> 
>          The motorcomm phy (yt8821) supports the ability to
>          config the chip mode of serdes.
>          The yt8821 serdes could be set to AUTO_BX2500_SGMII or
>          FORCE_BX2500.
>          In AUTO_BX2500_SGMII mode, SerDes
>          speed is determined by UTP, if UTP link up
>          at 2.5GBASE-T, SerDes will work as
>          2500BASE-X, if UTP link up at
>          1000BASE-T/100BASE-Tx/10BASE-T, SerDes will work
>          as SGMII.
>          In FORCE_BX2500, SerDes always works
>          as 2500BASE-X.
> 
>     Very weird wrapping.
> 
>     Please wrap commit message according to Linux coding style / submission
>     process (neither too early nor over the limit):
>     https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
> 
> 
>         Signed-off-by: Frank.Sae <Frank.Sae@motor-comm.com>
> 
>     Didn't you copy user-name as you name?
> 
> sorry, not understand your mean.
> 
>         ---
>          .../bindings/net/motorcomm,yt8xxx.yaml          | 17 +++++++++++++++++
>          1 file changed, 17 insertions(+)
> 
>     Also, your threading is completely broken. Use git send-email or b4.
> 
> sorry, not understand your mean of threading broken. the patch used git
> send-email.

Your indentation of replies it also very odd!

> 
>         +      0: AUTO_BX2500_SGMII
>         +      1: FORCE_BX2500
>         +      In AUTO_BX2500_SGMII mode, serdes speed is determined by UTP,
>         +      if UTP link up at 2.5GBASE-T, serdes will work as 2500BASE-X,
>         +      if UTP link up at 1000BASE-T/100BASE-Tx/10BASE-T, serdes will
>         +      work as SGMII.
>         +      In FORCE_BX2500 mode, serdes always works as 2500BASE-X.
> 
> 
>     Explain why this is even needed and why "auto" is not correct in all
>     cases. In commit msg or property description.
> 
> yt8821 phy does not support strapping to config the serdes mode, so config the
> serdes mode by dts instead.

Strapping does not matter. You can set it on probe.

> even if auto 2500base-x serdes mode is default mode after phy hard reset, and
> auto as default must be make sense, but from most our customers's feedback,
> force 2500base-x serdes mode is used in project usually to adapt to mac's serdes
> settings. for customer's convenience and use simplicity, force 2500base-x serdes
> mode selected as default here.
 
If you are using phylink correctly, the customer should never
know. Both the MAC and the PHY enumerate the capabilities and phylink
will tell you what mode to change to.

I still think this property should be removed, default to auto, and
let phylink tell you if something else should be used.

> 
>         +    $ref: /schemas/types.yaml#/definitions/uint8
> 
>     Make it a string, not uint8.
> 
> 
> why do you suggest string, the property value(uint8) will be wrote to phy
> register.

Device tree is not a list of values to poke into registers. It is a
textual description of the hardware. The driver probably needs to
apply conversions to get register values. e.g. delay are in ns,
voltages are in millivolts, but register typically don't use such
units.

	Andrew

Re: [PATCH 1/2] dt-bindings: net: motorcomm: Add chip mode cfg

[Frank.Sae]

On 8/1/24 06:14, Andrew Lunn wrote:
> On Thu, Aug 01, 2024 at 02:49:12AM -0700, Frank.Sae wrote:
>> On 7/27/24 02:25, Krzysztof Kozlowski wrote:
>>
>>      On 27/07/2024 11:20, Frank.Sae wrote:
>>
>>           The motorcomm phy (yt8821) supports the ability to
>>           config the chip mode of serdes.
>>           The yt8821 serdes could be set to AUTO_BX2500_SGMII or
>>           FORCE_BX2500.
>>           In AUTO_BX2500_SGMII mode, SerDes
>>           speed is determined by UTP, if UTP link up
>>           at 2.5GBASE-T, SerDes will work as
>>           2500BASE-X, if UTP link up at
>>           1000BASE-T/100BASE-Tx/10BASE-T, SerDes will work
>>           as SGMII.
>>           In FORCE_BX2500, SerDes always works
>>           as 2500BASE-X.
>>
>>      Very weird wrapping.
>>
>>      Please wrap commit message according to Linux coding style / submission
>>      process (neither too early nor over the limit):
>>      https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>
>>
>>          Signed-off-by: Frank.Sae <Frank.Sae@motor-comm.com>
>>
>>      Didn't you copy user-name as you name?
>>
>> sorry, not understand your mean.
>>
>>          ---
>>           .../bindings/net/motorcomm,yt8xxx.yaml          | 17 +++++++++++++++++
>>           1 file changed, 17 insertions(+)
>>
>>      Also, your threading is completely broken. Use git send-email or b4.
>>
>> sorry, not understand your mean of threading broken. the patch used git
>> send-email.
> Your indentation of replies it also very odd!
>
>>          +      0: AUTO_BX2500_SGMII
>>          +      1: FORCE_BX2500
>>          +      In AUTO_BX2500_SGMII mode, serdes speed is determined by UTP,
>>          +      if UTP link up at 2.5GBASE-T, serdes will work as 2500BASE-X,
>>          +      if UTP link up at 1000BASE-T/100BASE-Tx/10BASE-T, serdes will
>>          +      work as SGMII.
>>          +      In FORCE_BX2500 mode, serdes always works as 2500BASE-X.
>>
>>
>>      Explain why this is even needed and why "auto" is not correct in all
>>      cases. In commit msg or property description.
>>
>> yt8821 phy does not support strapping to config the serdes mode, so config the
>> serdes mode by dts instead.
> Strapping does not matter. You can set it on probe.
>
>> even if auto 2500base-x serdes mode is default mode after phy hard reset, and
>> auto as default must be make sense, but from most our customers's feedback,
>> force 2500base-x serdes mode is used in project usually to adapt to mac's serdes
>> settings. for customer's convenience and use simplicity, force 2500base-x serdes
>> mode selected as default here.
>   
> If you are using phylink correctly, the customer should never
> know. Both the MAC and the PHY enumerate the capabilities and phylink
> will tell you what mode to change to.
>
> I still think this property should be removed, default to auto, and
> let phylink tell you if something else should be used.

The property motorcomm,chip-mode will be removed in next patch. found that
phy-mode property in dts can be used to configure the serdes mode. when
phy-mode is set to 2500base-x, the serdes is refered as always in
2500base-x, when phy-mode is set to sgmii, the serdes is refered as in
2500base-x or sgmii according to media speed.

>>          +    $ref: /schemas/types.yaml#/definitions/uint8
>>
>>      Make it a string, not uint8.
>>
>>
>> why do you suggest string, the property value(uint8) will be wrote to phy
>> register.
> Device tree is not a list of values to poke into registers. It is a
> textual description of the hardware. The driver probably needs to
> apply conversions to get register values. e.g. delay are in ns,
> voltages are in millivolts, but register typically don't use such
> units.
>
> 	Andrew

Re: [PATCH 1/2] dt-bindings: net: motorcomm: Add chip mode cfg

[Krzysztof Kozlowski]

On 01/08/2024 11:49, Frank.Sae wrote:
> 
> On 7/27/24 02:25, Krzysztof Kozlowski wrote:
>> On 27/07/2024 11:20, Frank.Sae wrote:
>>>   The motorcomm phy (yt8821) supports the ability to
>>>   config the chip mode of serdes.
>>>   The yt8821 serdes could be set to AUTO_BX2500_SGMII or
>>>   FORCE_BX2500.
>>>   In AUTO_BX2500_SGMII mode, SerDes
>>>   speed is determined by UTP, if UTP link up
>>>   at 2.5GBASE-T, SerDes will work as
>>>   2500BASE-X, if UTP link up at
>>>   1000BASE-T/100BASE-Tx/10BASE-T, SerDes will work
>>>   as SGMII.
>>>   In FORCE_BX2500, SerDes always works
>>>   as 2500BASE-X.
>> Very weird wrapping.
>>
>> Please wrap commit message according to Linux coding style / submission
>> process (neither too early nor over the limit):
>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>
>>> Signed-off-by: Frank.Sae<Frank.Sae@motor-comm.com>
>> Didn't you copy user-name as you name?
> 
> sorry, not understand your mean.

Does your ID (e.g. passport) has exactly that name? With dot? Really?

> 
>>> ---
>>>   .../bindings/net/motorcomm,yt8xxx.yaml          | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>> Also, your threading is completely broken. Use git send-email or b4.
> 
> sorry, not understand your mean of threading broken. the patch used git
> send-email.

Email threading is completely broken. It means all reviewers see it as
complete unrelated emails which makes any review annoyingly more difficult.


> 
> 
>>> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
>>> index 26688e2302ea..ba34260f889d 100644
>>> --- a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
>>> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
>>> @@ -110,6 +110,23 @@ properties:
>>>         Transmit PHY Clock delay train configuration when speed is 1000Mbps.
>>>       type: boolean
>>>   
>>> +  motorcomm,chip-mode:
>>> +    description: |
>>> +      Only for yt8821 2.5G phy, it supports two chip working modes,
>> Then allOf:if:then disallowing it for the other variant?
> 
> sorry, not understand your mean.

So you did not understand anything from any comments?

You miss if:then: block which disallows this for other chips. You claim
only one device supports it, so why the property should be valid for
other devices?


> 
> 
>>> +      one is AUTO_BX2500_SGMII, the other is FORCE_BX2500.
>>> +      If this property is not set in device tree node then driver
>>> +      selects chip mode FORCE_BX2500 by default.
>> Don't repeat constraints in free form text.
>>
>>> +      0: AUTO_BX2500_SGMII
>>> +      1: FORCE_BX2500
>>> +      In AUTO_BX2500_SGMII mode, serdes speed is determined by UTP,
>>> +      if UTP link up at 2.5GBASE-T, serdes will work as 2500BASE-X,
>>> +      if UTP link up at 1000BASE-T/100BASE-Tx/10BASE-T, serdes will
>>> +      work as SGMII.
>>> +      In FORCE_BX2500 mode, serdes always works as 2500BASE-X.
>>
>> Explain why this is even needed and why "auto" is not correct in all
>> cases. In commit msg or property description.
> 
> yt8821 phy does not support strapping to config the serdes mode, so config the
> serdes mode by dts instead.
> 
> even if auto 2500base-x serdes mode is default mode after phy hard reset, and
> auto as default must be make sense, but from most our customers's feedback,
> force 2500base-x serdes mode is used in project usually to adapt to mac's serdes
> settings. for customer's convenience and use simplicity, force 2500base-x serdes
> mode selected as default here.

Sorry, this is not a proper sentence and I cannot parse it.

> 
> 
>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>> Make it a string, not uint8.
>>
> why do you suggest string, the property value(uint8) will be wrote to phy
> register.

Because it is much more readable. I don't care about your phy register.

Best regards,
Krzysztof