Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Yijie Yang <quic_yijiyang@quicinc.com>]

On 2025-01-21 00:49, Andrew Lunn wrote:
>>> So this indicates any board might need this feature, not just this one
>>> board. Putting the board name in the driver then does not scale.
>>>
>>
>> Should I ignore this if I choose to use the following standard properties?
> 
> You should always follow standard properties unless they don't
> work. And if they don't work, your commit message needs to explain why
> they don't work forcing your to do something special.
> 
>>>> This means the time
>>>> delay introduced by the PC board may not be zero. Therefore, it's necessary
>>>> for software developers to tune both the RX programming swap bit and the
>>>> delay to ensure correct sampling.
>>>
>>> O.K. Now look at how other boards tune their delays. There are
>>> standard properties for this:
>>>
>>>           rx-internal-delay-ps:
>>>             description:
>>>               RGMII Receive Clock Delay defined in pico seconds. This is used for
>>>               controllers that have configurable RX internal delays. If this
>>>               property is present then the MAC applies the RX delay.
>>>           tx-internal-delay-ps:
>>>             description:
>>>               RGMII Transmit Clock Delay defined in pico seconds. This is used for
>>>               controllers that have configurable TX internal delays. If this
>>>               property is present then the MAC applies the TX delay.
>>>
>>> I think you can use these properties, maybe with an additional comment
>>> in the binding. RGMII running at 1G has a clock of 125MHz. That is a
>>> period of 8ns. So a half clock cycle delay is then 4ns.
>>>
>>> So an rx-internal-delay-ps of 0-2000 means this clock invert should be
>>> disabled. A rx-internal-delay-ps of 4000-6000 means the clock invert
>>> should be enabled.
>>
>> This board was designed to operate at different speed rates, not a fixed
>> speed, and the clock rate varies for each speed. Thus, the delay introduced
>> by inverting the clock is not fixed. Additionally, I noticed that some
>> vendors apply the same routine for this property across all speeds in their
>> driver code. Can this property be used just as a flag, regardless of its
>> actual value?
> 
> Maybe you should go read the RGMII standard, and then think about how
> your hardware actually works.
> 
> RGMII always has a variable clock, with different clock speeds for
> 10/100/1G. So your board design is just plain normal, not
> special. Does the standard talk about different delays for different
> speeds? As you say, other drivers apply the same delay for all
> speeds. Why should your hardware be special?
> 
> RGMII has been around for 25 years. Do you really think your RGMII
> implementation needs something special which no other implementation
> has needed in the last 25 years?

I do not intend to violate the regulations of the RGMII standard and aim 
to maintain the same delay across all speeds. But the RX programming 
swap bit can only introduce a delay of 180 degrees. Should I assume the 
1G speed clock to calculate and determine if this bit should be enabled 
for all speeds?

> 
>>> Now, ideally, you want the PHY to add the RGMII delays, that is what i
>>> request all MAC/PHY pairs do, so we have a uniform setup across all
>>> boards. So unless the PHY does not support RGMII delays, you would
>>> expect rx-internal-delay-ps to be either just a small number of
>>> picoseconds for fine tuning, or a small number of picoseconds + 4ns
>>> for fine tuning.
>>
>> The delay for both TX and RX sides is added by the MAC in the Qualcomm
>> driver, which was introduced by the initial patch. I believe there may be a
>> refactor in the future to ensure it follows the requirements.
>   
> You can do it in the MAC. But you probably want to clearly document
> this, that your design is different to > 95% of systems in Linux which
> have the PHY do the delays.
> 
> 	Andrew

-- 
Best Regards,
Yijie