Re: [PATCH] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped FPGA

Re: [PATCH] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped FPGA

[David Daney]

On 08/22/2012 02:45 PM, Timur Tabi wrote:
> An FPGA controls which sub-bus is connected to the master MDIO bus.  The
> FPGA must be memory-mapped and contain only 8-bit registers (which keeps
> things simple).
>
> Tested on a Freescale P5020DS board which uses the "PIXIS" FPGA attached
> to the localbus.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>   .../devicetree/bindings/net/mdio-mux-fpga.txt      |   74 ++++++++
>   drivers/net/phy/Kconfig                            |   13 ++
>   drivers/net/phy/Makefile                           |    1 +
>   drivers/net/phy/mdio-mux-fpga.c                    |  186 ++++++++++++++++++++

I am fine with the general concept of the patch, so I am going to start 
a Bike Shedding session with it over the names of some of the things here.

I wonder if *fpga is really a good name for this.  It is a general 
purpose multiplexer with a memory mapped control register.  I would call 
it something like mdio-mux-mmioreg.


>   4 files changed, 274 insertions(+), 0 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/net/mdio-mux-fpga.txt
>   create mode 100644 drivers/net/phy/mdio-mux-fpga.c
>
> diff --git a/Documentation/devicetree/bindings/net/mdio-mux-fpga.txt b/Documentation/devicetree/bindings/net/mdio-mux-fpga.txt
> new file mode 100644
> index 0000000..ef567c6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mdio-mux-fpga.txt
> @@ -0,0 +1,74 @@
> +Properties for an MDIO bus multiplexer/switch controlled by an FPGA register.
> +
> +This is a special case of a MDIO bus multiplexer.  An FPGA register is used
> +to control which child bus is connected.
> +
> +Required properties in addition to the generic multiplexer properties:
> +
> +- compatible : string, must contain "mdio-mux-fpga"
> +
> +- mdio-mux-device : phandle, points to the FPGA (or similar) node.  This
> +	must be a memory-mapped device with 8-bit registers.

You shouldn't need this.  Just make the multiplexer a child of FPGA node 
to indicate where it lives.


> +
> +- mdio-mux-register : integer, contains the offset of the register that
> +	controls the bus multiplexer.

This should just be the normal "reg" properly


> +
> +- mdio-mux-mask : integer, contains an 8-bit mask that specifies which
> +	bits in the register control the actual bus multiplexer.  The
> +	'reg' property of each child mdio-mux node must be constrained by
> +	this mask.
> +

"reg-mask" ??

Do you need a shift too?


David Daney

Re: [PATCH] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped FPGA

[Timur Tabi]

David Daney wrote:

> I wonder if *fpga is really a good name for this.  It is a general 
> purpose multiplexer with a memory mapped control register.  I would call 
> it something like mdio-mux-mmioreg.

At one point, I thought of using mdio-mux-bitbang, but -mmioreg is better.
 Thanks.

>> +- mdio-mux-device : phandle, points to the FPGA (or similar) node.  This
>> +	must be a memory-mapped device with 8-bit registers.
> 
> You shouldn't need this.  Just make the multiplexer a child of FPGA node 
> to indicate where it lives.

The problem is that we don't normally consider the FPGA node to be a bus,
so its child nodes won't get probed.  That's why I have this:

compatible = "mdio-mux-fpga", "mdio-mux";
                               ^^^^^^^^

This allows me to have multiple mdio-mux parent nodes (which I do, since I
have multiple mdio bus muxes), and they all get registered and probed
properly because I also do this:

static const struct of_device_id of_device_ids[] __devinitconst = {
	{
		.compatible	= "simple-bus"
	},
	{
		.compatible	= "fsl,srio",
	},
...
	{
		.compatible	= "mdio-mux",
	},
	{}
};

The .compatible = "mdio-mux" is what causes all of the mdio-mux nodes to
be registered.  Therefore, it's simpler if all the mdio-mux nodes are root
nodes.

>> +
>> +- mdio-mux-register : integer, contains the offset of the register that
>> +	controls the bus multiplexer.
> 
> This should just be the normal "reg" properly

Ok.

>> +- mdio-mux-mask : integer, contains an 8-bit mask that specifies which
>> +	bits in the register control the actual bus multiplexer.  The
>> +	'reg' property of each child mdio-mux node must be constrained by
>> +	this mask.
>> +
> 
> "reg-mask" ??

Ok.

> 
> Do you need a shift too?

The 'reg' property of the mdio bus child nodes should take the shift into
account.  That's why, in the example, I have mask=0x6 and reg=0 or reg=2.
 There's even code in the driver to make sure that the 'reg' values are
constrained to the mask.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped FPGA

[David Daney]

On 08/22/2012 03:38 PM, Timur Tabi wrote:
> David Daney wrote:
>
>> I wonder if *fpga is really a good name for this.  It is a general
>> purpose multiplexer with a memory mapped control register.  I would call
>> it something like mdio-mux-mmioreg.
>
> At one point, I thought of using mdio-mux-bitbang, but -mmioreg is better.
>   Thanks.
>
>>> +- mdio-mux-device : phandle, points to the FPGA (or similar) node.  This
>>> +	must be a memory-mapped device with 8-bit registers.
>>
>> You shouldn't need this.  Just make the multiplexer a child of FPGA node
>> to indicate where it lives.
>
> The problem is that we don't normally consider the FPGA node to be a bus,
> so its child nodes won't get probed.  That's why I have this:
>

That would seem to be a mistake/error.

You should be able to arrive at any directly addressable register by 
walking down the tree to the children and applying any "ranges" 
properties at each node.  The OF infrastructure will take care of 
resolving all the addresses and you get rid of much of the code you 
added to duplicate its function.

Re: [PATCH] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped FPGA

[Tabi Timur-B04825]

David Daney wrote:
>>
>> The problem is that we don't normally consider the FPGA node to be a bus,
>> so its child nodes won't get probed.  That's why I have this:
>>
>
> That would seem to be a mistake/error.

Maybe I'm not explaining myself well.  A node that has children will not 
automatically probe the children unless something in arch code registers 
the parent as a bus.  If you look in corenet_ds.c, you'll see what we do 
with array of_device_ids[].  We list all of the nodes that are buses.  The 
mdio-mux root-level nodes that I have today won't work unless I add 
.compatible = "mdio-mux" to of_device_ids[].

>
> You should be able to arrive at any directly addressable register by
> walking down the tree to the children and applying any "ranges" properties
> at each node.  The OF infrastructure will take care of resolving all the
> addresses and you get rid of much of the code you added to duplicate its
> function.

That only works for buses that are already registered.  I think if I add 
"simple-bus" to the compatible of the FPGA node, it will work.  I'll try 
that tomorrow.

-- 
Timur Tabi
Linux kernel developer at Freescale