Date:	Mon, 15 Jun 2015 01:43:39 +0000
From:	Duan Andy <fugang.duan@...escale.com>
To:	Florian Fainelli <f.fainelli@...il.com>,
	Andrew Lunn <andrew@...n.ch>
CC:	David Miller <davem@...emloft.net>,
	Cory Tusar <cory.tusar@...1solutions.com>,
	netdev <netdev@...r.kernel.org>
Subject: RE: [PATCH] net: fec: Ensure clocks are enabled while using mdio bus

From: Florian Fainelli <f.fainelli@...il.com> Sent: Monday, June 15, 2015 3:32 AM
> To: Andrew Lunn; Duan Fugang-B38611
> Cc: David Miller; Cory Tusar; netdev
> Subject: Re: [PATCH] net: fec: Ensure clocks are enabled while using mdio
> bus
> 
> Le 06/14/15 07:41, Andrew Lunn a écrit :
> > On Sun, Jun 14, 2015 at 08:07:12AM +0000, Duan Andy wrote:
> >> From: Andrew Lunn <andrew@...n.ch> Sent: Friday, June 12, 2015 11:39
> >> PM
> >>> To: David Miller
> >>> Cc: Duan Fugang-B38611; Cory Tusar; netdev; Andrew Lunn
> >>> Subject: [PATCH] net: fec: Ensure clocks are enabled while using
> >>> mdio bus
> >>>
> >>> When a switch is attached to the mdio bus, the mdio bus can be used
> >>> while the interface is not open. If the clocks are not enabled, MDIO
> >>> reads/writes will simply time out. So enable the clocks before
> >>> starting a transaction, and disable them afterwards. The CCF
> >>> performs reference counting so the clocks will only be disabled if
> there are no other users.
> >>>
> >>> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> >>> ---
> >>
> >> NAK the patch.
> >
> >> i.MX series MDIO bus is a part of ENET controller. If the eth
> >> interface is not open, all clocks including MDIO bus clock are not
> >> enabled for power saving.
> 
> Which is fine, but is an assumption you can only make when your ENET
> controller is connected to an Ethernet PHY, with a switch, things get a
> little different see below.
> 
> >
> > Where do you see a power saving regression in this code? It is not as
> > if i just unconditionally turn the clocks on. As the comment says, at
> > the start of an MDIO transaction, the clocks are enabled. At the end
> > of a transaction, they are disabled again. If you don't have a switch
> > connected, there will be no transactions, hence no change to power
> > savings.
> >
> >> In general, if you want to use mdio bus net interface must be running
> >> status.
> >
> > This is not true for a number of Ethernet devices. All those currently
> > used with DSA allow MDIO transactions at any time.
> 
As I said, the MDIO bus is just one function of ENET controller, not dependent HW/IP.


> Right, the typical sequence looks like this:
> 
> - ethernet device is probed and registered first, for power savings
> purposes, the probe function might turn off clocks since we do not know
> whether the interface will be used at all, so until ndo_open is called,
> such clocks can be disabled
> 
> - DSA is probed second, and probes the switches using MDIO bus accesses,
> and if a switch is found, will configure it via MDIO bus reads/writes, by
> then, the network interface is guaranteed to be down
> 
> - last, you could disable the parent network device, but still issue MDIO
> reads/writes towards the switch to collect statistics, make it run in an
> unmanaged mode with the CPU interface down/powered off
> 
> Since clocks are reference counted, I really do not see much problem with
> Andrew's approach here.
> 
> If you are running without a switch and just a PHY, you will get an
> occasional clock reference count > 1 during MDIO reads/writes, and past
> your ndo_close() if the clock reference count is 1, aka still turned on,
> then that means you are not using the PHY library properly and you have
> dangling MDIO accesses.
> --
> Florian

Thanks for your detail explain why do you want the change.

If so, it just need to enable ENET ipg clock for your cases, not all enet clocks.
Ipg clock is for MDIO bus read/write, for ENET registers accessing. (fep->clk_ipg)

Which switch phy do you use ? I tried BCM54220 switch on the driver that works fine.

Powered by blists - more mailing lists