Re: [PATCH] MII bus API for PHY devices

Re: [PATCH] MII bus API for PHY devices

[Manfred Spraul]

Hi,

I don't like the polling/interrupt setup part:
- for a nic driver, there is no irq line that could be requested by 
mii_phy_irq_enable().
- if the mii bus driver uses it's own timers, then locking within the 
nics will be more difficult.

Could you make that part optional? For a nic driver, I would prefer if I 
could just call the ->startup part without the request_irq. If the nic 
irq handler notices that the nic got an event, then it would call an 
appropriate mii_bus function.

This also applies for something like /dev/phy/xy: With natsemi, it would 
be very tricky to add proper locking. The nic as an internal phy and an 
external mii bus. The internal phy is partially visible on the external 
bus and any accesses to the phy id of the internal phy on the external 
bus cause lockups. No big deal, I just move the internal phy around [the 
phy id doesn't matter], but I would prefer if I have to do that just for 
ethtool, not for multiple interfaces.

--
    Manfred

Re: [PATCH] MII bus API for PHY devices

[Andy Fleming]

On Nov 19, 2004, at 14:18, Manfred Spraul wrote:

> Hi,
>
> I don't like the polling/interrupt setup part:
> - for a nic driver, there is no irq line that could be requested by 
> mii_phy_irq_enable().
> - if the mii bus driver uses it's own timers, then locking within the 
> nics will be more difficult.

I'm not sure I accept the argument that locking will be more difficult. 
  Jason's patch requires that a callback be registered for the interrupt 
or the polling (My update has a similar scheme).  The function you 
register is essentially like an extra interrupt, except it is never 
invoked at interrupt time.  All the function has to do is react 
properly to link state.  If, previously, you checked link state in your 
interrupt handler, you could still do it there, I suspect.

>
> Could you make that part optional? For a nic driver, I would prefer if 
> I could just call the ->startup part without the request_irq. If the 
> nic irq handler notices that the nic got an event, then it would call 
> an appropriate mii_bus function.

I think it would be doable to arrange the interface such that drivers 
could adopt only the PHY configuration infrastructure, and not any of 
the polling/interrupt infrastructure.  Of course, as it is, it is at 
least WHOLLY optional, so no driver has to use it at all.

>
> This also applies for something like /dev/phy/xy: With natsemi, it 
> would be very tricky to add proper locking. The nic as an internal phy 
> and an external mii bus. The internal phy is partially visible on the 
> external bus and any accesses to the phy id of the internal phy on the 
> external bus cause lockups. No big deal, I just move the internal phy 
> around [the phy id doesn't matter], but I would prefer if I have to do 
> that just for ethtool, not for multiple interfaces.

I agree with this point -- Accessing the PHY through /dev registers is 
a recipe for some mess.  Though I could be convinced that it is 
manageable.  I do think, however, that the ethtool interface is 
sufficient to the task.

Re: [PATCH] MII bus API for PHY devices

[Andy Fleming]

Replying this to the netdev list for their perusal.

First, I'm flattered that you've based this on the gianfar phy code, 
which I stole shamelessly from Benh's code, but Benh changed his code, 
and so I stole once more (the sungem_phy code he mentioned).  The 
current 2.6.9 gianfar driver has a completely different PHY 
infrastructure.  One which is a better model, I think, for abstraction 
than my previous code.

Which brings me to item #2: I have taken the code from the previous 
patch, and combined it with my newer PHY code, which should ease the 
way for someone to add some of the WOL features, and such.  I have 
compiled, and tested this code, and it works with the gianfar driver.

However, I've got a few questions for the network device community, on 
how best to finish this off.

The primary issue is how to classify the MII bus.  Should it be a 
proper bus, fitting within the new driver model?  This seems like the 
proper method to me, since the MII bus is, in fact, a bus.  But it's a 
bit of a strange bus.  The bus is attached at two ends.  One end has 
some number of PHY devices, and the corresponding drivers for them.  
The other end has some number of ethernet controllers, and their 
drivers.

So we have 3 implementation decisions which are affected by this:

1) How should we pass initialization information from the system to the 
bus.  Information like which irq to use for each PHY, and what the 
address space for the bus's controls is.  I would like to enforce 
encapsulation so that the ethernet drivers don't need to know this 
information, or pass it to the bus.

2) How should we reflect the dependency of the ethernet driver on the 
mii bus driver?

3) How should we bind ethernet drivers to PHY drivers?

Oh, and a 4th side-issue:
Should each PHY have its own file?  Or should we dump all the PHY 
drivers in one file?  And if so, should THAT file be separate from the 
mii bus implementation file?

Andy Fleming
Open Source Team
Freescale Semiconductor, Inc

Re: [PATCH] MII bus API for PHY devices

[Jason McMullan]

On Thu, 2004-11-18 at 11:52 -0600, Andy Fleming wrote:
> 1) How should we pass initialization information from the system to the 
> bus.  Information like which irq to use for each PHY, and what the 
> address space for the bus's controls is.  I would like to enforce 
> encapsulation so that the ethernet drivers don't need to know this 
> information, or pass it to the bus.

(Just an off-the-cuff answer here)

In line with the OCP->platform work I've been doing, I would think
that creating 'phy' devices on the platform bus would be appropriate,
with 'platform_data' that describes (a) the platform device ethernet
it's bus is on and (b) it's PHY ID on that bus. The PHY's IRQ would
be in it's platform resources.

> 2) How should we reflect the dependency of the ethernet driver on the 
> mii bus driver?

Hmm. Don't really know from a sysfs perspective...


> 3) How should we bind ethernet drivers to PHY drivers?

A PHY 'platform_data' struct like:

struct phy_device_data {
	struct {
		const char *name;
		int id;
	} ethernet_platform_device_parent;
	int	phy_id;
}
	
> Oh, and a 4th side-issue:
> Should each PHY have its own file? 

Actually, each PHY should have it's own device directory, like every
other device. Eventually, PHYs should have /dev/phy* entries, where
user-space can read/write PHY registers.

-- 
Jason McMullan <jason.mcmullan@timesys.com>

Re: [PATCH] MII bus API for PHY devices

[Andy Fleming]

On Nov 18, 2004, at 13:34, Jason McMullan wrote:
>> 3) How should we bind ethernet drivers to PHY drivers?
>
> A PHY 'platform_data' struct like:
>
> struct phy_device_data {
> 	struct {
> 		const char *name;
> 		int id;
> 	} ethernet_platform_device_parent;
> 	int	phy_id;
> }

So you would have each PHY know the controller to which it's attached?  
I would have thought the other way around... Hm.  I will definitely 
have to read up on my driver model stuff

> 	
>> Oh, and a 4th side-issue:
>> Should each PHY have its own file?
>
> Actually, each PHY should have it's own device directory, like every
> other device. Eventually, PHYs should have /dev/phy* entries, where
> user-space can read/write PHY registers.

I think you misunderstood.  Are you talking about sysfs?  I was talking 
about actual source files.  i.e. should there be dm9161.c, m88e1101.c, 
cis8201.c, etc.

Also, do we need user-space to read/write PHY registers.  ethtool has 
this capability, I believe, and the interfaces there are settled.

Andy Fleming

Re: [PATCH] MII bus API for PHY devices

[Jason McMullan]

On Thu, 2004-11-18 at 13:50 -0600, Andy Fleming wrote:
> Jason McMullan said:
> >
> > Actually, each PHY should have it's own device directory, like every
> > other device. Eventually, PHYs should have /dev/phy* entries, where
> > user-space can read/write PHY registers.
> 
> I think you misunderstood.  Are you talking about sysfs?  I was talking 
> about actual source files.  i.e. should there be dm9161.c, m88e1101.c, 
> cis8201.c, etc.

	Yes, I am talking about sysfs. And yes, I think every PHY should have
it's own .c file. (although most people could get away with
using a non-IRQ 'drivers/net/phy/phy-generic.c'

> Also, do we need user-space to read/write PHY registers.  ethtool has 
> this capability, I believe, and the interfaces there are settled.

Doh! I forgot.

-- 
Jason McMullan <jason.mcmullan@timesys.com>

Re: [PATCH] MII bus API for PHY devices

[Benjamin Herrenschmidt]

On Thu, 2004-11-18 at 11:52 -0600, Andy Fleming wrote:

> 1) How should we pass initialization information from the system to the 
> bus.  Information like which irq to use for each PHY, and what the 
> address space for the bus's controls is.  I would like to enforce 
> encapsulation so that the ethernet drivers don't need to know this 
> information, or pass it to the bus.

Unfortunately, this is all quite platform specific and the ethernet
driver may be the only one to know what to do here... add to that
various special cases of the way the PHY is wired or controlled, I think
we can't completely avoid special casing...

> 2) How should we reflect the dependency of the ethernet driver on the 
> mii bus driver?

The ethernet driver can instanciate the PHYs at it's childs, though the
case of several MACs sharing PHYs will be difficult to represent...

> 3) How should we bind ethernet drivers to PHY drivers?

I would have them instanciated by the ethernet driver. Besides, the PHY
driver will need to be able to identify it's "parent" driver in some
ways to deal with special cases. It would be nice to have a library of
utility code to independently deal with link tracking (basically what
drivers like sungem do independently), with a callback to the ethernet
driver to inform it of actual changes (notifier ?). MACs often have
autopoll features and PHY often have interrupts, but from experience,
that's not very useful and a good old timer based polling tend to do a
better job most of the time.

> Oh, and a 4th side-issue:
> Should each PHY have its own file?  Or should we dump all the PHY 
> drivers in one file?  And if so, should THAT file be separate from the 
> mii bus implementation file?

I'd put all bcm5xxx in the same file ... they can be put together by
families...

Also, 

Re: [PATCH] MII bus API for PHY devices

[Jason McMullan]

On Fri, 2004-11-19 at 10:26 +1100, Benjamin Herrenschmidt wrote:
> The ethernet driver can instanciate the PHYs at it's childs, though the
> case of several MACs sharing PHYs will be difficult to represent...

I think *requiring* a binding to ethernet devices is a bad idea. 

For example, on many MPC8260 embedded systems, the MII bus is controlled
by GPIO pins, not an ethernet MDIO system.

Some applications may use the MII bus to control non-PHY devices, such
as Broadcom ethernet switches. I'm working for a general MII bus that
ethernet PHYs and other devices can all use.

-- 
Jason McMullan <jason.mcmullan@timesys.com>

Re: [PATCH] MII bus API for PHY devices

[Andy Fleming]

On Nov 18, 2004, at 17:26, Benjamin Herrenschmidt wrote:

> On Thu, 2004-11-18 at 11:52 -0600, Andy Fleming wrote:
>
>> 1) How should we pass initialization information from the system to 
>> the
>> bus.  Information like which irq to use for each PHY, and what the
>> address space for the bus's controls is.  I would like to enforce
>> encapsulation so that the ethernet drivers don't need to know this
>> information, or pass it to the bus.
>
> Unfortunately, this is all quite platform specific and the ethernet
> driver may be the only one to know what to do here... add to that
> various special cases of the way the PHY is wired or controlled, I 
> think
> we can't completely avoid special casing...

Well, under the system I'm currently envisioning, the driver would be 
able to provide the data needed by the mii bus, but the hope would be 
to enable board files (for when the PHY is soldered on the motherboard, 
and the enet is not -- like on an MPC85xx) to provide this information 
instead, and leave out the enet as middleman.

>
>> 2) How should we reflect the dependency of the ethernet driver on the
>> mii bus driver?
>
> The ethernet driver can instanciate the PHYs at it's childs, though the
> case of several MACs sharing PHYs will be difficult to represent...

I really don't want the driver to intantiate PHYs directly.  The PHY is 
its own device, and the less net drivers have to understand their inner 
workings, the better.  However, I hadn't considered the possibility of 
multiple MACs sharing the same PHY.  It does, as you say, support my 
argument, though.  With some careful design, the mii bus should be able 
to handle this type of setup easily.

One of my goals, personally, is to allow multiple net drivers to share 
the same mii bus, as in the case of the FCC enet controllers' PHYs on 
an 8560 ADS, which can be accessed through TSEC1's MII Management bus.


>
>> 3) How should we bind ethernet drivers to PHY drivers?
>
> I would have them instanciated by the ethernet driver. Besides, the PHY
> driver will need to be able to identify it's "parent" driver in some
> ways to deal with special cases. It would be nice to have a library of
> utility code to independently deal with link tracking (basically what
> drivers like sungem do independently), with a callback to the ethernet
> driver to inform it of actual changes (notifier ?). MACs often have
> autopoll features and PHY often have interrupts, but from experience,
> that's not very useful and a good old timer based polling tend to do a
> better job most of the time.

So when you say instantiated, would you consider calling an "attach" 
function with the phy_id and bus_id of the desired PHY instantiation?  
I'm fine with that.  The PHY would need to be able to send 
notifications to the enet controller (currently done through a 
callback).  I'm interested in ideas on how the notifier could be used 
(I have a distaste for callbacks).

Autopoll features sound pretty neat.  I think the system should support 
that.  PHY interrupts are supported (they work quite well on my 85xx 
system), as is timer-based polling.  Do you really think that there are 
special cases which can't be handled using a library similar to the 
sungem_phy one?
>
>> Oh, and a 4th side-issue:
>> Should each PHY have its own file?  Or should we dump all the PHY
>> drivers in one file?  And if so, should THAT file be separate from the
>> mii bus implementation file?
>
> I'd put all bcm5xxx in the same file ... they can be put together by
> families...

Yeah, that sounds good.


Andy Fleming
Open Source Team
Freescale Semiconductor, Inc

Re: [PATCH] MII bus API for PHY devices

[Benjamin Herrenschmidt]

On Fri, 2004-11-19 at 15:18 -0600, Andy Fleming wrote:

> So when you say instantiated, would you consider calling an "attach" 
> function with the phy_id and bus_id of the desired PHY instantiation?  
> I'm fine with that.  The PHY would need to be able to send 
> notifications to the enet controller (currently done through a 
> callback).  I'm interested in ideas on how the notifier could be used 
> (I have a distaste for callbacks).

Look at the notifier lists in include/linux/notifier.h

> Autopoll features sound pretty neat.  I think the system should support 
> that.

But that becomes MAC-dependant again... That means you'd need 1) a way
for the MAC driver to ask the PHY driver what register it wants
autopolled, and a function in the PHY driver for the MAC to call when it
detects a change. Also, autopoll is broken in some MACs...

>   PHY interrupts are supported (they work quite well on my 85xx 
> system), as is timer-based polling.  Do you really think that there are 
> special cases which can't be handled using a library similar to the 
> sungem_phy one?

Nope. I think timer based polling with a sungem-like fallback mecanism
to forced speeds would be nice.

Ben.

Re: [PATCH] MII bus API for PHY devices

[Andy Fleming]

On Nov 19, 2004, at 16:43, Benjamin Herrenschmidt wrote:

> On Fri, 2004-11-19 at 15:18 -0600, Andy Fleming wrote:
>
>> So when you say instantiated, would you consider calling an "attach"
>> function with the phy_id and bus_id of the desired PHY instantiation?
>> I'm fine with that.  The PHY would need to be able to send
>> notifications to the enet controller (currently done through a
>> callback).  I'm interested in ideas on how the notifier could be used
>> (I have a distaste for callbacks).
>
> Look at the notifier lists in include/linux/notifier.h

Ok, will do.

>
>> Autopoll features sound pretty neat.  I think the system should 
>> support
>> that.
>
> But that becomes MAC-dependant again... That means you'd need 1) a way
> for the MAC driver to ask the PHY driver what register it wants
> autopolled, and a function in the PHY driver for the MAC to call when 
> it
> detects a change. Also, autopoll is broken in some MACs...

What I'm envisioning here is that the driver would be able to tell the 
PHY infrastructure that it's going to do its own thing, and then make 
use of the reading/configuring part of the infrastructure, similar to 
how sungem and gianfar are currently set up.  But they would have the 
option of letting the infrastructure also handle the status updates.  
And, of course, the driver would not go through the effort to use 
autopoll if it were broken.

>
>>   PHY interrupts are supported (they work quite well on my 85xx
>> system), as is timer-based polling.  Do you really think that there 
>> are
>> special cases which can't be handled using a library similar to the
>> sungem_phy one?
>
> Nope. I think timer based polling with a sungem-like fallback mecanism
> to forced speeds would be nice.

Yes, I agree.  The system I currently have does fallback to forced, 
though it doesn't yet support the "magic aneg" feature you mentioned.  
But that should be easy to add, and so it shall be.


Andy Fleming

Re: [PATCH] MII bus API for PHY devices

[Jason McMullan]

I've added pluggable PHY support modules and a bitbanger MII bus
framework. 

New patch at:
	http://www.evillabs.net/~gus/patches/driver-mii-bus.patch

RPX/EP8260 MII bus example for bit-banger:
	http://www.evillabs.net/~gus/patches/rpx8260_mii.c

-- 
Jason McMullan <jason.mcmullan@timesys.com>