| Message ID | 1457682245-22746-3-git-send-email-peppe.cavallaro@st.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
G'day Giuseppe, Sorry for delay in responding. I feel responsible for starting the changes breaking this. See below. On 11/03/2016 3:44 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> > --- > > V2: use is_pseudo_fixed_link > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +++-------- > .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 14 +++++--------- > include/linux/stmmac.h | 1 - > 3 files changed, 8 insertions(+), 18 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_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index 6a52fa1..ed33920 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -138,7 +138,11 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac) > return ERR_PTR(-ENODEV); > > plat->phy_node = of_node_get(np); > - } > + } else > + plat->mdio_bus_data = > + devm_kzalloc(&pdev->dev, > + sizeof(struct stmmac_mdio_bus_data), > + GFP_KERNEL); This breaks my use case where I have a fixed link but also need the mdio bus for talking to a dsa switch. stmmac_mdio_register bails out because mdio_bus_data is never allocated here at this point when a fixed link exists with no phy-handle property. Removing the else solves my use. Possibly it could be allocated in stmmac_mdio_register if snps,dwmac-mdio is found and not already allocated. With the if (!mdio_bus_data) return 0; to after the device tree query. > > /* "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. > @@ -146,14 +150,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac) > 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 > - 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); > > of_property_read_u32(np, "rx-fifo-depth", &plat->rx_fifo_size); > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > index eead8ab..1b4884c 100644 > --- a/include/linux/stmmac.h > +++ b/include/linux/stmmac.h > @@ -94,7 +94,6 @@ struct stmmac_dma_cfg { > }; > > struct plat_stmmacenet_data { > - char *phy_bus_name; > int bus_id; > int phy_addr; > int interface; >
On 3/11/2016 9:16 AM, Phil Reid wrote: > G'day Giuseppe, > > Sorry for delay in responding. > I feel responsible for starting the changes breaking this. > > See below. > > On 11/03/2016 3:44 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> >> --- >> >> V2: use is_pseudo_fixed_link >> >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +++-------- >> .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 14 >> +++++--------- >> include/linux/stmmac.h | 1 - >> 3 files changed, 8 insertions(+), 18 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_platform.c >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> index 6a52fa1..ed33920 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> @@ -138,7 +138,11 @@ stmmac_probe_config_dt(struct platform_device >> *pdev, const char **mac) >> return ERR_PTR(-ENODEV); >> >> plat->phy_node = of_node_get(np); >> - } >> + } else >> + plat->mdio_bus_data = >> + devm_kzalloc(&pdev->dev, >> + sizeof(struct stmmac_mdio_bus_data), >> + GFP_KERNEL); > > This breaks my use case where I have a fixed link but also need the mdio > bus for > talking to a dsa switch. stmmac_mdio_register bails out because > mdio_bus_data is > never allocated here at this point when a fixed link exists with no > phy-handle > property. Removing the else solves my use. ok I get the point. > > Possibly it could be allocated in stmmac_mdio_register if snps,dwmac-mdio > is found and not already allocated. > With the > if (!mdio_bus_data) > return 0; > to after the device tree query. this is a way. Let me do some tests and I send the V3. Thx for your feedback peppe > > >> >> /* "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. >> @@ -146,14 +150,6 @@ stmmac_probe_config_dt(struct platform_device >> *pdev, const char **mac) >> 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 >> - 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); >> >> of_property_read_u32(np, "rx-fifo-depth", &plat->rx_fifo_size); >> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h >> index eead8ab..1b4884c 100644 >> --- a/include/linux/stmmac.h >> +++ b/include/linux/stmmac.h >> @@ -94,7 +94,6 @@ struct stmmac_dma_cfg { >> }; >> >> struct plat_stmmacenet_data { >> - char *phy_bus_name; >> int bus_id; >> int phy_addr; >> int interface; >> > >
Hello Phil On 3/11/2016 9:16 AM, Phil Reid wrote: > G'day Giuseppe, > > Sorry for delay in responding. > I feel responsible for starting the changes breaking this. no pbl at all. I have just sent the V3 where I try to cover all the cases, pls let me know if you see some problem. I successfully tested it on my side with H410 SoC + fixed-link (switch on board) Regards Peppe > > See below. > > On 11/03/2016 3:44 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> >> --- >> >> V2: use is_pseudo_fixed_link >> >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +++-------- >> .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 14 >> +++++--------- >> include/linux/stmmac.h | 1 - >> 3 files changed, 8 insertions(+), 18 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_platform.c >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> index 6a52fa1..ed33920 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> @@ -138,7 +138,11 @@ stmmac_probe_config_dt(struct platform_device >> *pdev, const char **mac) >> return ERR_PTR(-ENODEV); >> >> plat->phy_node = of_node_get(np); >> - } >> + } else >> + plat->mdio_bus_data = >> + devm_kzalloc(&pdev->dev, >> + sizeof(struct stmmac_mdio_bus_data), >> + GFP_KERNEL); > > This breaks my use case where I have a fixed link but also need the mdio > bus for > talking to a dsa switch. stmmac_mdio_register bails out because > mdio_bus_data is > never allocated here at this point when a fixed link exists with no > phy-handle > property. Removing the else solves my use. > > Possibly it could be allocated in stmmac_mdio_register if snps,dwmac-mdio > is found and not already allocated. > With the > if (!mdio_bus_data) > return 0; > to after the device tree query. > > > >> >> /* "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. >> @@ -146,14 +150,6 @@ stmmac_probe_config_dt(struct platform_device >> *pdev, const char **mac) >> 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 >> - 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); >> >> of_property_read_u32(np, "rx-fifo-depth", &plat->rx_fifo_size); >> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h >> index eead8ab..1b4884c 100644 >> --- a/include/linux/stmmac.h >> +++ b/include/linux/stmmac.h >> @@ -94,7 +94,6 @@ struct stmmac_dma_cfg { >> }; >> >> struct plat_stmmacenet_data { >> - char *phy_bus_name; >> int bus_id; >> int phy_addr; >> int interface; >> > >
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_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index 6a52fa1..ed33920 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c @@ -138,7 +138,11 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac) return ERR_PTR(-ENODEV); plat->phy_node = of_node_get(np); - } + } else + plat->mdio_bus_data = + devm_kzalloc(&pdev->dev, + sizeof(struct stmmac_mdio_bus_data), + GFP_KERNEL); /* "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. @@ -146,14 +150,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac) 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 - 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); of_property_read_u32(np, "rx-fifo-depth", &plat->rx_fifo_size); diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index eead8ab..1b4884c 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -94,7 +94,6 @@ struct stmmac_dma_cfg { }; struct plat_stmmacenet_data { - char *phy_bus_name; int bus_id; int phy_addr; int interface;