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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 14 Sep 2022 17:35:55 +0000
From:   Biju Das <biju.das.jz@...renesas.com>
To:     Sergey Shtylyov <s.shtylyov@....ru>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
CC:     Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Chris Paterson <Chris.Paterson2@...esas.com>,
        Biju Das <biju.das@...renesas.com>
Subject: RE: [PATCH net-next v3] ravb: Add RZ/G2L MII interface support

Hi Sergey,

> Subject: Re: [PATCH net-next v3] ravb: Add RZ/G2L MII interface support
> 
> 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@...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...

OK will keep it as field declaration and use actual value during setting.

> 
> [...]
> >>> +	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...

OK. Will add it.

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

Ok. All good now. Will send 4.

Cheers,
Biju