Re: [PATCH net-next v3] ravb: Add RZ/G2L MII interface support

From: Sergey Shtylyov <s.shtylyov@omp.ru>
To: Biju Das <biju.das.jz@bp.renesas.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Chris Paterson <Chris.Paterson2@renesas.com>,
	Biju Das <biju.das@bp.renesas.com>
Subject: Re: [PATCH net-next v3] ravb: Add RZ/G2L MII interface support
Date: Wed, 14 Sep 2022 20:30:42 +0300	[thread overview]
Message-ID: <34db8b96-624a-09e5-24e1-9b8be375a6c8@omp.ru> (raw)
In-Reply-To: <OS0PR01MB592297F89124DD62211582AE86469@OS0PR01MB5922.jpnprd01.prod.outlook.com>

On 9/14/22 8:20 PM, Biju Das wrote:

[...]
>>> EMAC IP found on RZ/G2L Gb ethernet supports MII interface.
>>> This patch adds support for selecting MII interface mode.
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> ---
>>> v2->v3:
>>>  * Documented CXR35_HALFCYC_CLKSW1000 and CXR35_SEL_XMII_MII macros.
>>
>>    I definitely didn't mean it done this way...
>>
>> [...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb.h
>>> b/drivers/net/ethernet/renesas/ravb.h
>>> index b980bce763d3..058aceac8c92 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>> [...]
>>> @@ -965,6 +966,11 @@ enum CXR31_BIT {
>>>  	CXR31_SEL_LINK1	= 0x00000008,
>>>  };
>>>
>>> +enum CXR35_BIT {
>>> +	CXR35_HALFCYC_CLKSW1000	= 0x03E80000,	/* 1000 cycle of clk_chi
>> */
>>
>>    No, please just declare:
> 
> 
>>
>> 	CXR35_HALFCYC_CLKSW	= 0xffff0000,
> 
> Q1) Why do you think we should use this value for setting MII?

   Where are you seeing me asking for that? This is just the field declaration,
correct against the manual... we can safely omit it as well...

[...]
>>> +	CXR35_SEL_XMII_MII	= 0x00000002,	/* MII interface is used
>> */
>>
>>    All the other register *enum*s are declared from LSB to MSB. The
>> comment is pretty self-obvious here, please remove it. And declare the
>> whole field too:
>>
>> 	CXR35_SEL_XMII		= 0x00000003,
> 
> Values 1 and 3 are reserved so we cannot use 3.

   Again, this is the filed declaration, correct against the manual...

> I think the current patch holds good as per the hardware manual
> for selecting MII interface.

   It is incomplete, compared against the manual. And declaring
CXR35_HALFCYC_CLKSW1000 just looks completely ridiculous. :-)

> Please recheck and correct me
> if it is wrong.

> Cheers,
> Biju

[...]

MBR, Sergey