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

From: Krzysztof Kozlowski
Date: Thu Aug 01 2024 - 10:24:14 EST


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