* Re: [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings
2016-03-11 13:33 ` [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings Giuseppe Cavallaro
@ 2016-03-11 15:14 ` Phil Reid
2016-03-11 15:32 ` Giuseppe CAVALLARO
2016-03-12 10:50 ` Frank Schäfer
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Phil Reid @ 2016-03-11 15:14 UTC (permalink / raw)
To: Giuseppe Cavallaro, netdev
Cc: gabriel.fernandez, afaerber, fschaefer.oss, dinh.linux, davem
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.
> + 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;
>
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings
2016-03-11 15:14 ` Phil Reid
@ 2016-03-11 15:32 ` Giuseppe CAVALLARO
2016-03-14 0:50 ` Phil Reid
0 siblings, 1 reply; 13+ messages in thread
From: Giuseppe CAVALLARO @ 2016-03-11 15:32 UTC (permalink / raw)
To: Phil Reid, netdev
Cc: gabriel.fernandez, afaerber, fschaefer.oss, dinh.linux, davem
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;
>>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings
2016-03-11 15:32 ` Giuseppe CAVALLARO
@ 2016-03-14 0:50 ` Phil Reid
0 siblings, 0 replies; 13+ messages in thread
From: Phil Reid @ 2016-03-14 0:50 UTC (permalink / raw)
To: Giuseppe CAVALLARO, netdev
Cc: gabriel.fernandez, afaerber, fschaefer.oss, dinh.linux, davem
G'day Giuseppe,
On 11/03/2016 11:32 PM, Giuseppe CAVALLARO wrote:
> 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.
>
Tested-by: Phil Reid <preid@electromag.com.au>
Ignore my comment above. This works fine.
Test with / without compatible flag and it works as expected.
At the end of the loop for_each_child_of_node sets the node to NULL.
>
>>
>>> + 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;
>>>
>>
>>
>
>
>
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings
2016-03-11 13:33 ` [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings Giuseppe Cavallaro
2016-03-11 15:14 ` Phil Reid
@ 2016-03-12 10:50 ` Frank Schäfer
2016-03-14 9:14 ` Gabriel Fernandez
2016-03-15 0:54 ` Andreas Färber
3 siblings, 0 replies; 13+ messages in thread
From: Frank Schäfer @ 2016-03-12 10:50 UTC (permalink / raw)
To: Giuseppe Cavallaro, netdev
Cc: gabriel.fernandez, afaerber, dinh.linux, davem, preid
Hi Giuseppe,
Am 11.03.2016 um 14:33 schrieb Giuseppe Cavallaro:
> 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;
> + }
> +
> + 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;
v3 fixes the "libphy: PHY stmmac-0:ffffffff not found" issue on the
Banana Pi board, too.
Regards,
Frank
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings
2016-03-11 13:33 ` [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings Giuseppe Cavallaro
2016-03-11 15:14 ` Phil Reid
2016-03-12 10:50 ` Frank Schäfer
@ 2016-03-14 9:14 ` Gabriel Fernandez
2016-03-14 9:28 ` Giuseppe CAVALLARO
2016-03-15 0:54 ` Andreas Färber
3 siblings, 1 reply; 13+ messages in thread
From: Gabriel Fernandez @ 2016-03-14 9:14 UTC (permalink / raw)
To: Giuseppe Cavallaro
Cc: netdev, Andreas Färber, fschaefer.oss, Dinh Nguyen,
David S. Miller, Phil Reid
Hi Peppe,
Just one remark below
> 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
[snip]
> +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;
> +
stmmac_dt_phy() function should return a Boolean
Best Regards.
Gabriel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings
2016-03-14 9:14 ` Gabriel Fernandez
@ 2016-03-14 9:28 ` Giuseppe CAVALLARO
0 siblings, 0 replies; 13+ messages in thread
From: Giuseppe CAVALLARO @ 2016-03-14 9:28 UTC (permalink / raw)
To: Gabriel Fernandez
Cc: netdev, Andreas Färber, fschaefer.oss, Dinh Nguyen,
David S. Miller, Phil Reid
On 3/14/2016 10:14 AM, Gabriel Fernandez wrote:
> Hi Peppe,
>
> Just one remark below
>
>> 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
>
> [snip]
>
>> +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;
>> +
> stmmac_dt_phy() function should return a Boolean
Thx Gabriel, I will fix return value in V4.
peppe
>
>
> Best Regards.
>
> Gabriel
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings
2016-03-11 13:33 ` [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings Giuseppe Cavallaro
` (2 preceding siblings ...)
2016-03-14 9:14 ` Gabriel Fernandez
@ 2016-03-15 0:54 ` Andreas Färber
2016-03-15 15:53 ` Giuseppe CAVALLARO
3 siblings, 1 reply; 13+ messages in thread
From: Andreas Färber @ 2016-03-15 0:54 UTC (permalink / raw)
To: Giuseppe Cavallaro
Cc: netdev, gabriel.fernandez, fschaefer.oss, dinh.linux, davem,
preid
Hi Peppe,
Am 11.03.2016 um 14:33 schrieb Giuseppe Cavallaro:
> Initially the phy_bus_name was added to manipulate the
> driver name but It was recently just used to manage the
"it"
> fixed-link and then to take some decision at run-time
> inside the main (for example to skip EEE).
Word missing after "main"? ("function"?)
> So the patch uses the is_pseudo_fixed_link and removes
> removes the phy_bus_name variable not necessary anymore.
Duplicate "removes".
>
> 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).
For next-20160314 I needed "i2c: immediately mark ourselves as
registered" plus this build fix:
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -866,9 +866,8 @@ static int stmmac_init_phy(struct net_device *dev)
}
/* If attached to a switch, there is no reason to poll phy
handler */
- if (priv->plat->phy_bus_name)
- if (!strcmp(priv->plat->phy_bus_name, "fixed"))
- phydev->irq = PHY_IGNORE_INTERRUPT;
+ if (phydev->is_pseudo_fixed_link)
+ phydev->irq = PHY_IGNORE_INTERRUPT;
pr_debug("stmmac_init_phy: %s: attached to PHY (UID 0x%x)"
" Link = %d\n", dev->name, phydev->phy_id, phydev->link);
It then fixes the PHY error on GeekBox, so for this mini-series:
Tested-by: Andreas Färber <afaerber@suse.de>
The connectivity issue still remains. Kernel log snippet:
[ +0.001117] rk_gmac-dwmac ff290000.ethernet: Looking up phy-supply
from device tree
[ +0.000028] rk808 0-001b: Looking up vcc12-supply from device tree
[ +0.000014] vcc_lan: supplied by vcc_io
[ +0.000101] rk_gmac-dwmac ff290000.ethernet: clock input or output?
(input).
[ +0.000009] rk_gmac-dwmac ff290000.ethernet: TX delay(0x30).
[ +0.000008] rk_gmac-dwmac ff290000.ethernet: RX delay(0x10).
[ +0.000014] rk_gmac-dwmac ff290000.ethernet: init for RGMII
[ +0.000104] rk_gmac-dwmac ff290000.ethernet: clock input from PHY
[ +0.005063] rk_gmac-dwmac ff290000.ethernet: no reset control found
[ +0.000007] stmmac - user ID: 0x10, Synopsys ID: 0x35
[ +0.000002] Ring mode enabled
[ +0.000006] DMA HW capability register supported
[ +0.000000] Normal descriptors
[ +0.000003] RX Checksum Offload Engine supported (type 2)
[ +0.000002] TX Checksum insertion supported
[ +0.000002] Wake-Up On Lan supported
[ +0.000053] Enable RX Mitigation via HW Watchdog Timer
[ +0.000771] of_get_named_gpiod_flags: can't parse 'snps,reset-gpio'
property of node '/ethernet@ff290000[0]'
[ +0.004250] libphy: stmmac: probed
[ +0.000009] eth0: PHY ID 001cc915 at 0 IRQ POLL (stmmac-0:00) active
[ +0.000005] eth0: PHY ID 001cc915 at 1 IRQ POLL (stmmac-0:01)
As before, reverting "stmmac: first frame prep at the end of xmit
routine" fixes it. My test cases are `ping 192.168.1.1` and `zypper up`.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings
2016-03-15 0:54 ` Andreas Färber
@ 2016-03-15 15:53 ` Giuseppe CAVALLARO
2016-03-16 9:47 ` Andreas Färber
0 siblings, 1 reply; 13+ messages in thread
From: Giuseppe CAVALLARO @ 2016-03-15 15:53 UTC (permalink / raw)
To: Andreas Färber
Cc: netdev, gabriel.fernandez, fschaefer.oss, dinh.linux, davem,
preid, Tomeu Vizoso
[-- Attachment #1: Type: text/plain, Size: 4708 bytes --]
Hi Andreas
On 3/15/2016 1:54 AM, Andreas Färber wrote:
> Hi Peppe,
>
> Am 11.03.2016 um 14:33 schrieb Giuseppe Cavallaro:
>> Initially the phy_bus_name was added to manipulate the
>> driver name but It was recently just used to manage the
>
> "it"
>
>> fixed-link and then to take some decision at run-time
>> inside the main (for example to skip EEE).
>
> Word missing after "main"? ("function"?)
>
>> So the patch uses the is_pseudo_fixed_link and removes
>> removes the phy_bus_name variable not necessary anymore.
>
> Duplicate "removes".
Sure I will fix them in the v4
>
>>
>> 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).
>
> For next-20160314 I needed "i2c: immediately mark ourselves as
> registered" plus this build fix:
yes I will send it in a separate set for net-next.
>
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -866,9 +866,8 @@ static int stmmac_init_phy(struct net_device *dev)
> }
>
> /* If attached to a switch, there is no reason to poll phy
> handler */
> - if (priv->plat->phy_bus_name)
> - if (!strcmp(priv->plat->phy_bus_name, "fixed"))
> - phydev->irq = PHY_IGNORE_INTERRUPT;
> + if (phydev->is_pseudo_fixed_link)
> + phydev->irq = PHY_IGNORE_INTERRUPT;
>
> pr_debug("stmmac_init_phy: %s: attached to PHY (UID 0x%x)"
> " Link = %d\n", dev->name, phydev->phy_id, phydev->link);
>
> It then fixes the PHY error on GeekBox, so for this mini-series:
>
> Tested-by: Andreas Färber <afaerber@suse.de>
thx a lot for having tested it.
>
> The connectivity issue still remains. Kernel log snippet:
>
> [ +0.001117] rk_gmac-dwmac ff290000.ethernet: Looking up phy-supply
> from device tree
> [ +0.000028] rk808 0-001b: Looking up vcc12-supply from device tree
> [ +0.000014] vcc_lan: supplied by vcc_io
> [ +0.000101] rk_gmac-dwmac ff290000.ethernet: clock input or output?
> (input).
> [ +0.000009] rk_gmac-dwmac ff290000.ethernet: TX delay(0x30).
> [ +0.000008] rk_gmac-dwmac ff290000.ethernet: RX delay(0x10).
> [ +0.000014] rk_gmac-dwmac ff290000.ethernet: init for RGMII
> [ +0.000104] rk_gmac-dwmac ff290000.ethernet: clock input from PHY
> [ +0.005063] rk_gmac-dwmac ff290000.ethernet: no reset control found
> [ +0.000007] stmmac - user ID: 0x10, Synopsys ID: 0x35
> [ +0.000002] Ring mode enabled
> [ +0.000006] DMA HW capability register supported
> [ +0.000000] Normal descriptors
^^^^^^^^^^^^^^^^^^
> [ +0.000003] RX Checksum Offload Engine supported (type 2)
> [ +0.000002] TX Checksum insertion supported
> [ +0.000002] Wake-Up On Lan supported
> [ +0.000053] Enable RX Mitigation via HW Watchdog Timer
> [ +0.000771] of_get_named_gpiod_flags: can't parse 'snps,reset-gpio'
> property of node '/ethernet@ff290000[0]'
> [ +0.004250] libphy: stmmac: probed
> [ +0.000009] eth0: PHY ID 001cc915 at 0 IRQ POLL (stmmac-0:00) active
> [ +0.000005] eth0: PHY ID 001cc915 at 1 IRQ POLL (stmmac-0:01)
>
> As before, reverting "stmmac: first frame prep at the end of xmit
> routine" fixes it. My test cases are `ping 192.168.1.1` and `zypper up`.
so on your side, this revert fixes the issue and you do not see any
tx watchdog as Tomeu's raised.
I have fixed some problems on top of
"stmmac: first frame prep at the end of xmit ..." please
see patch attached for net-next.
Indeed, the normal tx descriptors are well filled w/o
"stmmac: first frame prep at the end of xmit ..."
I wonder if you could try the attachment in order to understand if we
have to actually revert the patch (I ask you to not revert the
patch "stmmac: first frame...").
I cannot test on an HW with Normal descriptors, unfortunately.
I am continuing to review the code to try to find other issues
on this configuration.
Let me know.
Regards
peppe
>
> Regards,
> Andreas
>
[-- Attachment #2: 0001-stmmac-fix-TX-normal-DESC.patch --]
[-- Type: text/x-patch, Size: 1490 bytes --]
>From 036b21b0396d6df42bd8e507be1449fbc7935bce Mon Sep 17 00:00:00 2001
From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date: Tue, 15 Mar 2016 16:54:51 +0100
Subject: [PATCH (net-next.git)] stmmac: fix TX normal DESC
This patch is to fix the normal descriptor that has
a broken len and the OWN bit never set.
Signed-off-by: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
---
drivers/net/ethernet/stmicro/stmmac/norm_desc.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
index e13228f..011386f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
@@ -199,11 +199,6 @@ static void ndesc_prepare_tx_desc(struct dma_desc *p, int is_fs, int len,
{
unsigned int tdes1 = p->des1;
- if (mode == STMMAC_CHAIN_MODE)
- norm_set_tx_desc_len_on_chain(p, len);
- else
- norm_set_tx_desc_len_on_ring(p, len);
-
if (is_fs)
tdes1 |= TDES1_FIRST_SEGMENT;
else
@@ -217,10 +212,15 @@ static void ndesc_prepare_tx_desc(struct dma_desc *p, int is_fs, int len,
if (ls)
tdes1 |= TDES1_LAST_SEGMENT;
- if (tx_own)
- tdes1 |= TDES0_OWN;
-
p->des1 = tdes1;
+
+ if (mode == STMMAC_CHAIN_MODE)
+ norm_set_tx_desc_len_on_chain(p, len);
+ else
+ norm_set_tx_desc_len_on_ring(p, len);
+
+ if (tx_own)
+ p->des0 |= TDES0_OWN;
}
static void ndesc_set_tx_ic(struct dma_desc *p)
--
1.7.4.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings
2016-03-15 15:53 ` Giuseppe CAVALLARO
@ 2016-03-16 9:47 ` Andreas Färber
2016-03-16 10:18 ` Giuseppe CAVALLARO
0 siblings, 1 reply; 13+ messages in thread
From: Andreas Färber @ 2016-03-16 9:47 UTC (permalink / raw)
To: Giuseppe CAVALLARO
Cc: netdev, gabriel.fernandez, fschaefer.oss, dinh.linux, davem,
preid, Tomeu Vizoso
Hi Peppe,
Am 15.03.2016 um 16:53 schrieb Giuseppe CAVALLARO:
> On 3/15/2016 1:54 AM, Andreas Färber wrote:
>> The connectivity issue still remains. Kernel log snippet:
>>
>> [ +0.001117] rk_gmac-dwmac ff290000.ethernet: Looking up phy-supply
>> from device tree
>> [ +0.000028] rk808 0-001b: Looking up vcc12-supply from device tree
>> [ +0.000014] vcc_lan: supplied by vcc_io
>> [ +0.000101] rk_gmac-dwmac ff290000.ethernet: clock input or output?
>> (input).
>> [ +0.000009] rk_gmac-dwmac ff290000.ethernet: TX delay(0x30).
>> [ +0.000008] rk_gmac-dwmac ff290000.ethernet: RX delay(0x10).
>> [ +0.000014] rk_gmac-dwmac ff290000.ethernet: init for RGMII
>> [ +0.000104] rk_gmac-dwmac ff290000.ethernet: clock input from PHY
>> [ +0.005063] rk_gmac-dwmac ff290000.ethernet: no reset control found
>> [ +0.000007] stmmac - user ID: 0x10, Synopsys ID: 0x35
>> [ +0.000002] Ring mode enabled
>> [ +0.000006] DMA HW capability register supported
>> [ +0.000000] Normal descriptors
>
> ^^^^^^^^^^^^^^^^^^
>
>> [ +0.000003] RX Checksum Offload Engine supported (type 2)
>> [ +0.000002] TX Checksum insertion supported
>> [ +0.000002] Wake-Up On Lan supported
>> [ +0.000053] Enable RX Mitigation via HW Watchdog Timer
>> [ +0.000771] of_get_named_gpiod_flags: can't parse 'snps,reset-gpio'
>> property of node '/ethernet@ff290000[0]'
>> [ +0.004250] libphy: stmmac: probed
>> [ +0.000009] eth0: PHY ID 001cc915 at 0 IRQ POLL (stmmac-0:00) active
>> [ +0.000005] eth0: PHY ID 001cc915 at 1 IRQ POLL (stmmac-0:01)
>>
>> As before, reverting "stmmac: first frame prep at the end of xmit
>> routine" fixes it. My test cases are `ping 192.168.1.1` and `zypper up`.
>
> so on your side, this revert fixes the issue and you do not see any
> tx watchdog as Tomeu's raised.
Confirmed, the system ran stable over night. I don't use NFS on that
system myself, having GMAC working only since v2.
> I have fixed some problems on top of
> "stmmac: first frame prep at the end of xmit ..." please
> see patch attached for net-next.
>
> Indeed, the normal tx descriptors are well filled w/o
> "stmmac: first frame prep at the end of xmit ..."
>
> I wonder if you could try the attachment in order to understand if we
> have to actually revert the patch (I ask you to not revert the
> patch "stmmac: first frame...").
Works great on top of next-20160314 with i2c plus this series, please
add my Tested-by. Thanks a lot for investigating!
Cheers,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv3 (net.git) 2/2] stmmac: fix MDIO settings
2016-03-16 9:47 ` Andreas Färber
@ 2016-03-16 10:18 ` Giuseppe CAVALLARO
0 siblings, 0 replies; 13+ messages in thread
From: Giuseppe CAVALLARO @ 2016-03-16 10:18 UTC (permalink / raw)
To: Andreas Färber
Cc: netdev, gabriel.fernandez, fschaefer.oss, dinh.linux, davem,
preid, Tomeu Vizoso
Hi Andreas
On 3/16/2016 10:47 AM, Andreas Färber wrote:
> Hi Peppe,
>
> Am 15.03.2016 um 16:53 schrieb Giuseppe CAVALLARO:
>> On 3/15/2016 1:54 AM, Andreas Färber wrote:
>>> The connectivity issue still remains. Kernel log snippet:
>>>
>>> [ +0.001117] rk_gmac-dwmac ff290000.ethernet: Looking up phy-supply
>>> from device tree
>>> [ +0.000028] rk808 0-001b: Looking up vcc12-supply from device tree
>>> [ +0.000014] vcc_lan: supplied by vcc_io
>>> [ +0.000101] rk_gmac-dwmac ff290000.ethernet: clock input or output?
>>> (input).
>>> [ +0.000009] rk_gmac-dwmac ff290000.ethernet: TX delay(0x30).
>>> [ +0.000008] rk_gmac-dwmac ff290000.ethernet: RX delay(0x10).
>>> [ +0.000014] rk_gmac-dwmac ff290000.ethernet: init for RGMII
>>> [ +0.000104] rk_gmac-dwmac ff290000.ethernet: clock input from PHY
>>> [ +0.005063] rk_gmac-dwmac ff290000.ethernet: no reset control found
>>> [ +0.000007] stmmac - user ID: 0x10, Synopsys ID: 0x35
>>> [ +0.000002] Ring mode enabled
>>> [ +0.000006] DMA HW capability register supported
>>> [ +0.000000] Normal descriptors
>>
>> ^^^^^^^^^^^^^^^^^^
>>
>>> [ +0.000003] RX Checksum Offload Engine supported (type 2)
>>> [ +0.000002] TX Checksum insertion supported
>>> [ +0.000002] Wake-Up On Lan supported
>>> [ +0.000053] Enable RX Mitigation via HW Watchdog Timer
>>> [ +0.000771] of_get_named_gpiod_flags: can't parse 'snps,reset-gpio'
>>> property of node '/ethernet@ff290000[0]'
>>> [ +0.004250] libphy: stmmac: probed
>>> [ +0.000009] eth0: PHY ID 001cc915 at 0 IRQ POLL (stmmac-0:00) active
>>> [ +0.000005] eth0: PHY ID 001cc915 at 1 IRQ POLL (stmmac-0:01)
>>>
>>> As before, reverting "stmmac: first frame prep at the end of xmit
>>> routine" fixes it. My test cases are `ping 192.168.1.1` and `zypper up`.
>>
>> so on your side, this revert fixes the issue and you do not see any
>> tx watchdog as Tomeu's raised.
>
> Confirmed, the system ran stable over night. I don't use NFS on that
> system myself, having GMAC working only since v2.
>
>> I have fixed some problems on top of
>> "stmmac: first frame prep at the end of xmit ..." please
>> see patch attached for net-next.
>>
>> Indeed, the normal tx descriptors are well filled w/o
>> "stmmac: first frame prep at the end of xmit ..."
>>
>> I wonder if you could try the attachment in order to understand if we
>> have to actually revert the patch (I ask you to not revert the
>> patch "stmmac: first frame...").
>
> Works great on top of next-20160314 with i2c plus this series, please
> add my Tested-by. Thanks a lot for investigating!
you are welcome, I have just sent the patch
"[PATCH (net-next.git)] stmmac: fix TX normal DESC"
do not hesitate to contact me for further checks
and if I have missed something.
Cheers
peppe
>
> Cheers,
> Andreas
>
^ permalink raw reply [flat|nested] 13+ messages in thread