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@xxxxxxxxxxxxxx>
> >
> >      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