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

[Yijie Yang <quic_yijiyang@quicinc.com>]

On 2025-01-08 21:29, Andrew Lunn wrote:
>>> Why is it specific to this board? Does the board have a PHY which is
>>> broken and requires this property? What we are missing are the details
>>> needed to help you get to the correct way to solve the problem you are
>>> facing.
>>>
>>
>> Let me clarify why this bit is necessary and why it's board-specific. The RX
>> programming swap bit can introduce a time delay of half a clock cycle. This
>> bit, along with the clock delay adjustment functionality, is implemented by
>> a module called 'IO Macro.' This is a Qualcomm-specific hardware design
>> located between the MAC and PHY in the SoC, serving the RGMII interface. The
>> bit works in conjunction with delay adjustment to meet the sampling
>> requirements. The sampling of RX data is also handled by this module.
>>
>> During the board design stage, the RGMII requirements may not have been
>> strictly followed, leading to uncertainty in the relationship between the
>> clock and data waveforms when they reach the IO Macro.
> 
> 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?

>> 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?

> 
> 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.

> 
> This scales, since it can be used by an board with poor design, and it
> does not require anything proprietary to Qualcomm, except the extended
> range, and hopefully nobody except Qualcomms broken RDK will require
> it, because obviously you will document the issue with the RDK and
> tell customers how to correctly design their board to be RGMII
> compliant with the clocks.

Yes, I will make a note of it.

> 
> 	Andrew

-- 
Best Regards,
Yijie