Re: [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings

[Giuseppe CAVALLARO <peppe.cavallaro@st.com>]

On 3/11/2016 4:14 PM, Phil Reid wrote:
> G'day Giuseppe,
>
> I wont be able to test until Monday.
> Concept looks ok to me except for comment below.
>
> On 11/03/2016 9:33 PM, Giuseppe Cavallaro wrote:
>> Initially the phy_bus_name was added to manipulate the
>> driver name but It was recently just used to manage the
>> fixed-link and then to take some decision at run-time
>> inside the main (for example to skip EEE).
>> So the patch uses the is_pseudo_fixed_link and removes
>> removes the phy_bus_name variable not necessary anymore.
>>
>> The driver can manage the mdio registration by using phy-handle,
>> dwmac-mdio and own parameter e.g. snps,phy-addr.
>> This patch takes care about all these possible configurations
>> and fixes the mdio registration in case of there is a real
>> transceiver or a switch (that needs to be managed by using
>> fixed-link).
>>
>> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> Tested-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> Cc: Gabriel Fernandez <gabriel.fernandez@linaro.org>
>> Cc: Dinh Nguyen <dinh.linux@gmail.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Phil Reid <preid@electromag.com.au>
>> ---
>>
>> V2: use is_pseudo_fixed_link
>> V3: parse device-tree driver parameters to allocate PHY resources
>> considering
>>      DSA case (+ fixed-link).
>>
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |   11 +--
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c  |   19 +-----
>>   .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |   77
>> ++++++++++++++++----
>>   include/linux/stmmac.h                             |    2 +-
>>   4 files changed, 68 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index c21015b..389d7d0 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -271,7 +271,6 @@ static void stmmac_eee_ctrl_timer(unsigned long arg)
>>    */
>>   bool stmmac_eee_init(struct stmmac_priv *priv)
>>   {
>> -    char *phy_bus_name = priv->plat->phy_bus_name;
>>       unsigned long flags;
>>       bool ret = false;
>>
>> @@ -283,7 +282,7 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
>>           goto out;
>>
>>       /* Never init EEE in case of a switch is attached */
>> -    if (phy_bus_name && (!strcmp(phy_bus_name, "fixed")))
>> +    if (priv->phydev->is_pseudo_fixed_link)
>>           goto out;
>>
>>       /* MAC core supports the EEE feature. */
>> @@ -820,12 +819,8 @@ static int stmmac_init_phy(struct net_device *dev)
>>           phydev = of_phy_connect(dev, priv->plat->phy_node,
>>                       &stmmac_adjust_link, 0, interface);
>>       } else {
>> -        if (priv->plat->phy_bus_name)
>> -            snprintf(bus_id, MII_BUS_ID_SIZE, "%s-%x",
>> -                 priv->plat->phy_bus_name, priv->plat->bus_id);
>> -        else
>> -            snprintf(bus_id, MII_BUS_ID_SIZE, "stmmac-%x",
>> -                 priv->plat->bus_id);
>> +        snprintf(bus_id, MII_BUS_ID_SIZE, "stmmac-%x",
>> +             priv->plat->bus_id);
>>
>>           snprintf(phy_id_fmt, MII_BUS_ID_SIZE + 3, PHY_ID_FMT, bus_id,
>>                priv->plat->phy_addr);
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>> index 0faf163..3f5512f 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>> @@ -198,29 +198,12 @@ int stmmac_mdio_register(struct net_device *ndev)
>>       struct mii_bus *new_bus;
>>       struct stmmac_priv *priv = netdev_priv(ndev);
>>       struct stmmac_mdio_bus_data *mdio_bus_data =
>> priv->plat->mdio_bus_data;
>> +    struct device_node *mdio_node = priv->plat->mdio_node;
>>       int addr, found;
>> -    struct device_node *mdio_node = NULL;
>> -    struct device_node *child_node = NULL;
>>
>>       if (!mdio_bus_data)
>>           return 0;
>>
>> -    if (IS_ENABLED(CONFIG_OF)) {
>> -        for_each_child_of_node(priv->device->of_node, child_node) {
>> -            if (of_device_is_compatible(child_node,
>> -                            "snps,dwmac-mdio")) {
>> -                mdio_node = child_node;
>> -                break;
>> -            }
>> -        }
>> -
>> -        if (mdio_node) {
>> -            netdev_dbg(ndev, "FOUND MDIO subnode\n");
>> -        } else {
>> -            netdev_warn(ndev, "No MDIO subnode found\n");
>> -        }
>> -    }
>> -
>>       new_bus = mdiobus_alloc();
>>       if (new_bus == NULL)
>>           return -ENOMEM;
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> index 6a52fa1..d2322e9 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> @@ -96,6 +96,66 @@ static int dwmac1000_validate_ucast_entries(int
>> ucast_entries)
>>   }
>>
>>   /**
>> + * stmmac_dt_phy - parse device-tree driver parameters to allocate
>> PHY resources
>> + * @plat: driver data platform structure
>> + * @np: device tree node
>> + * @dev: device pointer
>> + * Description:
>> + * The mdio bus will be allocated in case of a phy transceiver is on
>> board;
>> + * it will be NULL if the fixed-link is configured.
>> + * If there is the "snps,dwmac-mdio" sub-node the mdio will be allocated
>> + * in any case (for DSA, mdio must be registered even if fixed-link).
>> + * The table below sums the supported configurations:
>> + *    -------------------------------
>> + *    snps,phy-addr    |     Y
>> + *    -------------------------------
>> + *    phy-handle    |     Y
>> + *    -------------------------------
>> + *    fixed-link    |     N
>> + *    -------------------------------
>> + *    snps,dwmac-mdio    |
>> + *      even if    |     Y
>> + *    fixed-link    |
>> + *    -------------------------------
>> + *
>> + * It returns true in case of the mdio needs to be registered in the
>> main.
>> + */
>> +static bool stmmac_dt_phy(struct plat_stmmacenet_data *plat,
>> +              struct device_node *np, struct device *dev)
>> +{
>> +    bool ret = true;
>> +
>> +    /* If phy-handle property is passed from DT, use it as the PHY */
>> +    plat->phy_node = of_parse_phandle(np, "phy-handle", 0);
>> +    if (plat->phy_node)
>> +        dev_dbg(dev, "Found phy-handle subnode\n");
>> +
>> +    /* If phy-handle is not specified, check if we have a fixed-phy */
>> +    if (!plat->phy_node && of_phy_is_fixed_link(np)) {
>> +        if ((of_phy_register_fixed_link(np) < 0))
>> +            return -ENODEV;
>> +
>> +        dev_dbg(dev, "Found fixed-link subnode\n");
>> +        plat->phy_node = of_node_get(np);
>> +
>> +        ret = false;
>> +    }
>> +
>> +    /* If snps,dwmac-mdio is passed from DT, always register the MDIO */
>> +    for_each_child_of_node(np, plat->mdio_node) {
>> +        if (of_device_is_compatible(plat->mdio_node, "snps,dwmac-mdio"))
>> +            break;
>> +    }
>> +
>
> Won't this always result in plat->mdio_node being assigned to something
> if np has a child.
> Regardless of the compatible string.
> Which is why I had the child_node temp.
> Still learning so may be missing something.

hmm, i think so, let me know as soon as you test it so I can
rework the patch and send a v4.

Thanks
peppe

>
>> +    if (plat->mdio_node) {
>> +        dev_dbg(dev, "Found MDIO subnode\n");
>> +        ret = true;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +/**
>>    * stmmac_probe_config_dt - parse device-tree driver parameters
>>    * @pdev: platform_device structure
>>    * @plat: driver data platform structure
>> @@ -129,30 +189,19 @@ stmmac_probe_config_dt(struct platform_device
>> *pdev, const char **mac)
>>       /* Default to phy auto-detection */
>>       plat->phy_addr = -1;
>>
>> -    /* If we find a phy-handle property, use it as the PHY */
>> -    plat->phy_node = of_parse_phandle(np, "phy-handle", 0);
>> -
>> -    /* If phy-handle is not specified, check if we have a fixed-phy */
>> -    if (!plat->phy_node && of_phy_is_fixed_link(np)) {
>> -        if ((of_phy_register_fixed_link(np) < 0))
>> -            return ERR_PTR(-ENODEV);
>> -
>> -        plat->phy_node = of_node_get(np);
>> -    }
>> -
>>       /* "snps,phy-addr" is not a standard property. Mark it as
>> deprecated
>>        * and warn of its use. Remove this when phy node support is added.
>>        */
>>       if (of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr)
>> == 0)
>>           dev_warn(&pdev->dev, "snps,phy-addr property is deprecated\n");
>>
>> -    if ((plat->phy_node && !of_phy_is_fixed_link(np)) ||
>> plat->phy_bus_name)
>> -        plat->mdio_bus_data = NULL;
>> -    else
>> +    /* To Configure PHY by using all device-tree supported properties */
>> +    if (stmmac_dt_phy(plat, np, &pdev->dev)) {
>>           plat->mdio_bus_data =
>>               devm_kzalloc(&pdev->dev,
>>                        sizeof(struct stmmac_mdio_bus_data),
>>                        GFP_KERNEL);
>> +    }
>>
>>       of_property_read_u32(np, "tx-fifo-depth", &plat->tx_fifo_size);
>>
>> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
>> index eead8ab..8b1ff2b 100644
>> --- a/include/linux/stmmac.h
>> +++ b/include/linux/stmmac.h
>> @@ -94,12 +94,12 @@ struct stmmac_dma_cfg {
>>   };
>>
>>   struct plat_stmmacenet_data {
>> -    char *phy_bus_name;
>>       int bus_id;
>>       int phy_addr;
>>       int interface;
>>       struct stmmac_mdio_bus_data *mdio_bus_data;
>>       struct device_node *phy_node;
>> +    struct device_node *mdio_node;
>>       struct stmmac_dma_cfg *dma_cfg;
>>       int clk_csr;
>>       int has_gmac;
>>
>
>