Re: [PATCH net-next v2 07/10] net: bcmgenet: add MDIO routines

[Florian Fainelli <f.fainelli@gmail.com>]

Hi Mark,

2014-02-13 3:50 GMT-08:00 Mark Rutland <mark.rutland@arm.com>:
> On Thu, Feb 13, 2014 at 05:29:52AM +0000, Florian Fainelli wrote:
>> This patch adds support for configuring the port multiplexer hardware
>> which resides in front of the GENET Ethernet MAC controller. This allows
>> us to support:
>>
>> - internal PHYs (using drivers/net/phy/bcm7xxx.c)
>> - MoCA PHYs which are an entirely separate hardware block not covered
>>   here
>> - external PHYs and switches
>>
>> Note that MoCA and switches are currently supported using the emulated
>> "fixed PHY" driver.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> Changes since v1:
>> - fixed MDIO crash/warning when Device Tree probing fails
>> - removed the use of priv->phy_type and use priv->phy_interface
>>   directly
>>
>>  drivers/net/ethernet/broadcom/genet/bcmmii.c | 481 +++++++++++++++++++++++++++
>>  1 file changed, 481 insertions(+)
>>  create mode 100644 drivers/net/ethernet/broadcom/genet/bcmmii.c
>
> [...]
>
>> +static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
>> +{
>> +       struct device_node *dn = priv->pdev->dev.of_node;
>> +       struct device *kdev = &priv->pdev->dev;
>> +       struct device_node *mdio_dn;
>> +       const __be32 *fixed_link;
>
> This looks a bit odd. Could we not have a common parser for fixed-link
> properties?

Do you remember the fixed-link revamp that Thomas Petazzoni submitted
a while ago? I was planning on using it, but there were still some
disagreements on how to do it, so I ended up using the good old
"fixed-link" property as-is.

>
>> +       u32 propval;
>> +       int ret, sz;
>> +
>> +       mdio_dn = of_get_next_child(dn, NULL);
>> +       if (!mdio_dn) {
>> +               dev_err(kdev, "unable to find MDIO bus node\n");
>> +               return -ENODEV;
>> +       }
>
> Could you please check that this is the node you expect (by looking at
> the compatible string list).

Makes sense.

>
>> +
>> +       ret = of_mdiobus_register(priv->mii_bus, mdio_dn);
>> +       if (ret) {
>> +               dev_err(kdev, "failed to register MDIO bus\n");
>> +               return ret;
>> +       }
>> +
>> +       /* Check if we have an internal or external PHY */
>> +       priv->phy_dn = of_parse_phandle(dn, "phy-handle", 0);
>> +       if (priv->phy_dn) {
>> +               if (!of_property_read_u32(priv->phy_dn, "max-speed", &propval))
>> +                       priv->phy_speed = propval;
>
> Is there no way to find this out without reading values directly off of
> the PHY? It seems like something we should have an abstraction for.

In fact, this is a remnant from when I did not submit the patch doing
this in of_mdiobus_register(), so this is now useless.

>
>> +       } else {
>> +               /* Read the link speed from the fixed-link property */
>> +               fixed_link = of_get_property(dn, "fixed-link", &sz);
>> +               if (!fixed_link || sz < sizeof(*fixed_link)) {
>> +                       ret = -ENODEV;
>> +                       goto out;
>> +               }
>> +
>> +               priv->phy_speed = be32_to_cpu(fixed_link[2]);
>
> Similarly can we not have a common fixed-link parser? Or abstraction
> such that you query the phy regardless of what it is and how its binding
> represents this?

I do not think I need this line anymore. I will do some more testing
and will remove this parsing. I do agree that we need a common
fixed-link parser, but that is part of the discussion initiated by
Thomas Petazzoni.

Thanks for the review!
-- 
Florian