[PATCH v2 3/4] net: ravb: Add RZ/G2L Support

[Marek Vasut]

On 3/24/25 11:12 AM, Paul Barker wrote:
> On 24/03/2025 01:11, Marek Vasut wrote:
>> On 3/19/25 1:03 PM, Paul Barker wrote:
>>
>> [...]
>>
>>> +++ b/drivers/net/Kconfig
>>> @@ -864,7 +864,7 @@ config RENESAS_RAVB
>>>    	select PHY_ETHERNET_ID
>>>    	help
>>>    	  This driver implements support for the Ethernet AVB block in
>>> -	  Renesas M3 and H3 SoCs.
>>> +	  several Renesas R-Car and RZ SoCs.
>>
>> RZ/G instead of RZ , right ?
> 
> This will also be used for RZ/V2L.

Ahh, thank you for clarifying.

[...]

>>> +static void ravb_mac_init_rzg2l(struct udevice *dev)
>>> +{
>>> +	struct ravb_priv *eth = dev_get_priv(dev);
>>> +
>>> +	setbits_32(eth->iobase + RAVB_REG_ECMR,
>>> +		   ECMR_PRM | ECMR_RXF | ECMR_TXF | ECMR_RCPT |
>>> +		   ECMR_TE | ECMR_RE | ECMR_RZPF |
>>> +		   (eth->phydev->duplex ? ECMR_DM : 0));
>>
>> Can you configure the ECMR extras in ravb_config() just before
>> writel(mask, eth->iobase + RAVB_REG_ECMR); based on some private data
>> flag, like '.is_rzg2l' , instead ?
> 
> ravb_config() has been split into ravb_config_rcar() &
> ravb_config_rzg2l() by this patch series. So there is no longer a common
> write to RAVB_REG_ECMR.
> 
>>
>>> +}
>>> +
>>>    /* AVB-DMAC init function */
>>>    static int ravb_dmac_init(struct udevice *dev)
>>>    {
>>> @@ -459,6 +481,14 @@ static void ravb_dmac_init_rcar(struct udevice *dev)
>>>    	writel(mode, eth->iobase + RAVB_REG_APSR);
>>>    }
>>>    
>>> +static void ravb_dmac_init_rzg2l(struct udevice *dev)
>>> +{
>>> +	struct ravb_priv *eth = dev_get_priv(dev);
>>> +
>>> +	/* Set Max Frame Length (RTC) */
>>> +	writel(0x7ffc0000 | RFLR_RFL_MIN, eth->iobase + RAVB_REG_RTC);
>>
>> I assume this register is actually RZ/G2L specific ?
> 
> This register also exists on RZ/G2{H,M,N,E}, but in the Linux kernel
> ravb driver it is only modified for RZ/G2L.

Is this deliberate or is that a bug ?

+CC Geert.

>>> +}
>>> +
>>>    static int ravb_config(struct udevice *dev)
>>>    {
>>>    	struct ravb_device_ops *device_ops =
>>> @@ -501,6 +531,22 @@ static void ravb_config_rcar(struct udevice *dev)
>>>    	writel(mask, eth->iobase + RAVB_REG_ECMR);
>>>    }
>>>    
>>> +static void ravb_config_rzg2l(struct udevice *dev)
>>> +{
>>> +	struct ravb_priv *eth = dev_get_priv(dev);
>>> +	struct phy_device *phy = eth->phydev;
>>> +
>>> +	writel(CSR0_TPE | CSR0_RPE, eth->iobase + RAVB_REG_CSR0);
>>> +
>>> +	/* Set the transfer speed */
>>> +	if (phy->speed == 10)
>>> +		writel(GECMR_SPEED_10M, eth->iobase + RAVB_REG_GECMR);
>>
>> Since this patch adds .has_reset flag, wouldn't it be possible to add
>> another flag called something like .has_10 flag and simply do:
>>
>> if (eth->has_10 && phy->speed == 10)
>>     writel(GECMR_SPEED_10M, eth->iobase + RAVB_REG_GECMR);
>>
>> ?
> 
> The layout of RAVB_REG_GECMR differs between RZ/G2L and RZ/G2{H,M,N,E},
> so we can't use the same constants for all platforms. We also need the
> additional write to RAVB_REG_CSR0 for RZ/G2L so it's worth having the
> ravb_config_rzg2l() function instead of a couple of flags.
Ah, thank you for clarifying. This register layout bit is what I was 
missing. The CSR0 part could have been isolated in some if (..), but the 
register layout is a bit more unpleasant to deal with.