[PATCH 1/2] net: dwc_eth_qos_rockchip: Fix disable of RX/TX delay for RK356x

[Quentin Schulz]

Hi Jonas,

On 2/11/25 5:46 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2025-02-11 13:20, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 2/10/25 12:27 AM, Jonas Karlman wrote:
>>> When rgmii-rxid/txid/id phy-mode is used the MAC should not add RX
>>> and/or TX delay. Currently RX/TX delay is configured as enabled using
>>> zero as delay value for the rgmii-rxid/txid/id modes.
>>>
>>> Change to disable RX and/or TX delay and using zero as delay value.
>>>
>>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>>> ---
>>>    drivers/net/dwc_eth_qos_rockchip.c | 7 +++++--
>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/dwc_eth_qos_rockchip.c b/drivers/net/dwc_eth_qos_rockchip.c
>>> index 9fc8c686b889..3e10e07403c1 100644
>>> --- a/drivers/net/dwc_eth_qos_rockchip.c
>>> +++ b/drivers/net/dwc_eth_qos_rockchip.c
>>> @@ -46,6 +46,10 @@ struct rockchip_platform_data {
>>>    #define GRF_BIT(nr)	(BIT(nr) | BIT((nr) + 16))
>>>    #define GRF_CLR_BIT(nr)	(BIT((nr) + 16))
>>>    
>>> +#define DELAY_ENABLE(soc, tx, rx) \
>>> +	(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) | \
>>> +	 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
>>> +
>>
>> Considering this won't be used for RK3588 in the next patch and that I
>> found it obfuscating the constants that are used, can't we simply do...
>>
>>>    #define RK3568_GRF_GMAC0_CON0		0x0380
>>>    #define RK3568_GRF_GMAC0_CON1		0x0384
>>>    #define RK3568_GRF_GMAC1_CON0		0x0388
>>> @@ -85,8 +89,7 @@ static int rk3568_set_to_rgmii(struct udevice *dev,
>>>    
>>>    	regmap_write(data->grf, con1,
>>>    		     RK3568_GMAC_PHY_INTF_SEL_RGMII |
>>> -		     RK3568_GMAC_RXCLK_DLY_ENABLE |
>>> -		     RK3568_GMAC_TXCLK_DLY_ENABLE);
>>> +		     DELAY_ENABLE(RK3568, tx_delay, rx_delay));
>>
>> (tx_delay ? RK3568_GMAC_TXCLK_DLY_ENABLE : RK3568_GMAC_TXCLK_DLY_DISABLE) |
>> (rx_delay ? RK3568_GMAC_RXCLK_DLY_ENABLE : RK3568_GMAC_RXCLK_DLY_DISABLE));
>>
>> instead?
> 
> Sorry for not being clear in the cover letter or in commit message. The
> DELAY_ENABLE macro match a similar macro in gmac_rockchip.c and in the
> mainline Linux driver and will/should be used by pending RK3528 and
> RK3576 support.
> 

Ah, this is what I was missing :)

The commit in Linux seems to be eaf70ad14cbb ("net: stmmac: dwmac-rk: 
Add handling for RGMII_ID/RXID/TXID"). Maybe add this to the commit log?

>>
>> Also, if tx_delay or rx_delay is 0, does it actually make sense to write
>> the delay config in there? (so the regmap_write before this one).
> 
> It seem to be what is done for the other SoCs GMAC in both mainline and
> vendor U-Boot. Not sure we should deviate too much from what seem to be
> working.
> 

Didn't know there was a reference, thanks for the pointer.

>>
>> Note that the config is set before the enable on rk3568, but the
>> opposite on rk3588 which I find quite odd, you usually want to configure
>> something before it's enabled?
> 
> I also think this look a bit strange, however it is what is also done
> for the other SoCs GMAC in both mainline and vendor U-Boot. Hopefully
> the reg write order does not matter for the hw.
> 

/me shrugs.

Maybe update the commit log to point at the change from the Linux kernel 
as reference in a v2 if you don't mind?

Reviewed-by: Quentin Schulz <quentin.schulz at cherry.de>

Thanks!
Quentin