net: phy: fixed link

net: phy: fixed link

[Antony Pavlov]

Hi Sascha!

I have just took a look on Lukas' 
'net: phy: fixed-link: read link parameters from devicetree' patch.
I see two problems with current 'of_phy_register_fixed_link()' implementation
unrelated to Lukas' changes.

Please see current of_phy_register_fixed_link() implementation:

        static struct phy_device *of_phy_register_fixed_link(struct device_node *np,
                                                struct eth_device *edev)
        {
                struct phy_device *phydev;

                phydev = phy_device_create(NULL, 0, 0);

                phydev->dev.parent = &edev->dev;
                phydev->registered = 1;
                phydev->speed = 1000;
                phydev->duplex = 1;
                phydev->pause = phydev->asym_pause = 0;
                phydev->link = 1;

                return phydev;
        }

1. (a trivial think) the line

        phydev->link = 1;

is redundant because phy_device_create() has already set it.

2. (not so trivial think) the line

       phydev->registered = 1;

Why we want to set phydev->registered here? Can we use phy_register_device() instead?

Any sugesstions?

-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: net: phy: fixed link

[Sascha Hauer]

On Mon, Aug 08, 2016 at 07:32:03PM +0300, Antony Pavlov wrote:
> Hi Sascha!
> 
> I have just took a look on Lukas' 
> 'net: phy: fixed-link: read link parameters from devicetree' patch.
> I see two problems with current 'of_phy_register_fixed_link()' implementation
> unrelated to Lukas' changes.
> 
> Please see current of_phy_register_fixed_link() implementation:
> 
>         static struct phy_device *of_phy_register_fixed_link(struct device_node *np,
>                                                 struct eth_device *edev)
>         {
>                 struct phy_device *phydev;
> 
>                 phydev = phy_device_create(NULL, 0, 0);
> 
>                 phydev->dev.parent = &edev->dev;
>                 phydev->registered = 1;
>                 phydev->speed = 1000;
>                 phydev->duplex = 1;
>                 phydev->pause = phydev->asym_pause = 0;
>                 phydev->link = 1;
> 
>                 return phydev;
>         }
> 
> 1. (a trivial think) the line
> 
>         phydev->link = 1;
> 
> is redundant because phy_device_create() has already set it.

Yes, this is redundant and can be removed.

> 
> 2. (not so trivial think) the line
> 
>        phydev->registered = 1;
> 
> Why we want to set phydev->registered here? Can we use phy_register_device() instead?

If we use phy_register_device() here then I think the genphy driver is
attached to the fixed link which probably can't handle this fake device.
You can give it a try, but I assume you will see NULL pointer exceptions
in several places.

phydev->registered = 1 is necessary because without this
phy_register_device() is called in phy_device_attach() which is called
from phy_device_connect() called from the network device driver.

There may be ways to properly register a fixed link as a phy device, but
this would mean some serious rework of the phy layer.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox