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

["Frank.Sae" <Frank.Sae@motor-comm.com>]

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