On 10/15/07, Domen Puncer [off-list ref] wrote:

On 14/10/07 16:05 -0600, Grant Likely wrote:

quoted

On 10/14/07, Domen Puncer [off-list ref] wrote:

quoted

PHY part of the driver for mpc5200(b) ethernet.

Assuming I understand correctly, this comment is not correct and this
patch just adds an MDIO bus driver.  PHY drivers are in phylib and
data transfer is setup via the core driver, correct?

Right.

quoted

It is conceivable that the PHY is connected to an alternate MDIO bus,
or the MDIO bus is used for a PHY connected to an external Ethernet
controller.

Speaking of which, is it possible to use this MDIO bus without the
core FEC being initialized?

IIRC fec doesn't need any initialization for MDIO bus registers to work.

quoted

quoted

+static struct of_device_id fec_mdio_match[] = {
+       {
+               .type = "mdio",
+               .compatible = "mpc5200b-fec-phy",

This is not a phy; it's an MDIO bus.  Also, shouldn't this be
"mpc5200-..." instead of "mpc5200b-..."?

Didn't know if it's ok for mpc5200, guess it is?

I believe it is.

quoted

quoted

+       },
+       {},
+};
+
+struct of_platform_driver mpc52xx_fec_mdio_driver = {
+       .name = "mpc5200b-fec-phy",
+       .probe = fec_mdio_probe,
+       .remove = fec_mdio_remove,
+       .match_table = fec_mdio_match,

Inconsistent naming.  Please use the same prefix on all global/static
symbols (ie. use "mpc52xx_mdio_" instead of the mix of
"mpc52xx_fec_mdio_", "fec_mdio_", etc.)  I also thing that "fec_mdio_"
is too generic because there are a number of different incompatible
FEC devices.

OK.

quoted

quoted

+};
+
+/* let fec driver call it, since this has to be registered before it */
+EXPORT_SYMBOL_GPL(mpc52xx_fec_mdio_driver);

Why not have a module_init()/module_exit() in this file?  I don't
think the FEC driver calls this driver's functions directly anymore,
and it's still dependent on the of_platform bus probe order anyway.

It was one way of making sure mdio driver is registered before fec.
(and of_platform bus probe order won't work for modules)
Nicer alternatives?

However, that assumption only works when the PHY is accessed via the
on-chip MDIO controller.  What happens with a different MDIO bus is
used?

...

Need to take a look at how phylib handles the probing order problem;
but really this is something that PHYLIB needs to handle; not the FEC
driver.

quoted

quoted

--- linux.git.orig/drivers/net/fec_mpc52xx/Kconfig
+++ linux.git/drivers/net/fec_mpc52xx/Kconfig

@@ -11,5 +11,18 @@ config FEC_MPC52xx
        ---help---
          This option enables support for the MPC5200's on-chip
          Fast Ethernet Controller
+         If compiled as module, it will be called 'fec_mpc52xx.ko'.

Drop this line and make the help text the same format as the other eth
drivers in drivers/net.

How exactly is it different now?
And most of them have the "Module will be called xxx" line.

Okay, you're right.  There is a real hodgepodge of help text format in
drivers/net/Kconfig.  Do what seems best to you.

quoted

quoted

+
+config FEC_MPC52xx_MDIO
+       bool "FEC MII PHY driver"
+       depends on FEC_MPC52xx
+       default y
+       ---help---
+         The MPC5200's FEC can connect to the Ethernet either with
+         an external MII PHY chip or 10 Mbps 7-wire interface
+         (Motorola? industry standard).
+         If your board uses an external PHY, enable this.

Not strictly true.  This enables talking to a PHY using the internal
MDIO controller.  PHY register access could just as easily be accessed
via an alternate interface.

Not just that is also selects which mode will it use.
If you don't enable this, fec will be set up as 7-wire
( 696                 rcntrl |= FEC_RCNTRL_MII_MODE;)

Looking at fec.c:

That line is dependent on priv->has_phy.  priv->has_phy is set on line
949 which is conditional on the node having the property "phy-handle".
 It doesn't look like it is conditional on CONFIG_FEC_MPC52xx_MDIO at
all.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195