Re: [PATCH 0/3] Use DELAY_ENABLE macro for RK3328, RK3566/RK3568 and RK3588

[Jonas Karlman]

Hi Dragan,

On 2025-03-06 22:41, Dragan Simic wrote:
> Hello Jonas,
> 
> On 2025-03-06 21:38, Jonas Karlman wrote:
>> Almost all Rockchip GMAC variants use the DELAY_ENABLE macro to help
>> enable or disable use of MAC rx/tx delay. However, RK3328, 
>> RK3566/RK3568
>> and RK3588 GMAC driver does not.
>>
>> Use of the DELAY_ENABLE macro help ensure the MAC rx/tx delay is
>> disabled, instead of being enabled and using a zero delay, when
>> RGMII_ID/RXID/TXID is used.
>>
>> RK3328 driver was merged around the same time as when DELAY_ENABLE was
>> introduced so it is understandable why it was missed. Both 
>> RK3566/RK3568
>> and RK3588 support were introduced much later yet they also missed 
>> using
>> the DELAY_ENABLE macro (so did vendor kernel at that time).
>>
>> This series fixes all these cases to unify how GMAC delay feature is
>> enabled or disabled across the different GMAC variants.
>>
>> Jonas Karlman (3):
>>   net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3328
>>   net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3566/RK3568
>>   net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3588
>>
>>  .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> As far as I can tell, the RV1126 GMAC should also be converted to use
> the DELAY_ENABLE macro, which the vendor kernel already does. [*]  
> Perhaps
> that could be performed in new patch 4/4 in this series?

Good catch, the RV1126 seem to use a slight different M0/M1 variant and
unfortunately I do not have access to any RV1126 board to do any runtime
testing. For RK3328, RK356x and RK3588 I could at least do runtime
testing.

I can add an untested patch for RV1126 in a v2 if needed :-)

> 
> BTW, it would be quite neat to introduce the DELAY_VALUE macro, which
> makes the function calls a bit more compact. [*]

Personally, I do not see a real need for the DELAY_VALUE macro, current
use of the soc_GMAC_CLK_yX_DL_CFG() macro is readable enough.

The ENABLE_DELAY at least helps remove an otherwise more complex enable
or disable conditional handling.

Regards,
Jonas

> 
> [*] 
> https://raw.githubusercontent.com/rockchip-linux/kernel/refs/heads/develop-5.10/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c