netdev - RE: [PATCH net-next 3/3] enetc: Add ENETC PF level external MDIO 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <VI1PR04MB4880C8E3E302C7D3856DB68396670@VI1PR04MB4880.eurprd04.prod.outlook.com>
Date:   Thu, 14 Feb 2019 15:48:44 +0000
From:   Claudiu Manoil <claudiu.manoil@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Shawn Guo <shawnguo@...nel.org>, Leo Li <leoyang.li@....com>,
        "David S . Miller" <davem@...emloft.net>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Alexandru Marginean <alexandru.marginean@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net-next 3/3] enetc: Add ENETC PF level external MDIO
 support

>-----Original Message-----
>From: Andrew Lunn <andrew@...n.ch>
>Sent: Wednesday, February 13, 2019 8:13 PM
>To: Claudiu Manoil <claudiu.manoil@....com>
>Cc: Shawn Guo <shawnguo@...nel.org>; Leo Li <leoyang.li@....com>; David S .
>Miller <davem@...emloft.net>; devicetree@...r.kernel.org; Alexandru
>Marginean <alexandru.marginean@....com>; linux-kernel@...r.kernel.org;
>linux-arm-kernel@...ts.infradead.org; netdev@...r.kernel.org
>Subject: Re: [PATCH net-next 3/3] enetc: Add ENETC PF level external MDIO
>support
>
[...]
>> +static int enetc_mdio_write(struct mii_bus *bus, int phy_id, int regnum,
>> +			    u16 value)
>> +{
>> +	struct enetc_mdio_regs __iomem *regs = bus_to_enetc_regs(bus);
>> +	u32 mdio_ctl, mdio_cfg;
>> +	u16 dev_addr;
>> +	int ret;
>> +
>> +	mdio_cfg = MDIO_CFG_CLKDIV(ENETC_MDC_DIV) | MDIO_CFG_NEG;
>
>What does MDIO_CFG_NEG mean?
>

I'll add a comment in the code to clarify this define.
It means the MDIO is driven by master on MDC negative edge, which is how 
the external mdio busses work on this hardware. For internal MDIO CFG_NEG is 0.

 [...]
>
>Maybe MDIO_CFG_ENC could be called MDIO_CFG_C45? Assuming that is what
>it actually means?
>
You're right, in the hardware manual this bit is actually called "ENC45", so
I'll change the define name to that.  Should be more clear like this.

[...]
>
>It is a good idea to have an mdio node which contains the list of
>PHYs. You can get into odd situations if you don't do that.
>

Hopefully this doesn't complicate things since these are not platform devices.
It's not as easy as adding new platform device driver for mdio...

Ok for the rest of the comments too.
Thanks for the review.

Claudiu