| Message ID | 1584463806-15788-2-git-send-email-martin.fuzzey@flowbird.group |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Fix Wake on lan with FEC on i.MX6 | expand |
From: Martin Fuzzey <martin.fuzzey@flowbird.group> Sent: Wednesday, March 18, 2020 12:50 AM > On some SoCs, such as the i.MX6, it is necessary to set a bit in the SoC level > GPR register before suspending for wake on lan to work. > > The fec platform callback sleep_mode_enable was intended to allow this but > the platform implementation was NAK'd back in 2015 [1] > > This means that, currently, wake on lan is broken on mainline for the i.MX6 at > least. > > So implement the required bit setting in the fec driver by itself by adding a > new optional DT property indicating the register and bit to set. > > [1] > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.s > pinics.net%2Flists%2Fnetdev%2Fmsg310922.html&data=02%7C01%7Cf > ugang.duan%40nxp.com%7Ca9e64936ec9f45edc57108d7ca9340dd%7C686e > a1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637200606109625887&am > p;sdata=%2Bg4NIxZRwNY331k9cq5s6OIm22qD5qstiDIVlSQiL8E%3D&res > erved=0 > > Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group> > --- > drivers/net/ethernet/freescale/fec.h | 7 +++ > drivers/net/ethernet/freescale/fec_main.c | 72 > ++++++++++++++++++++++++++++--- > 2 files changed, 72 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec.h > b/drivers/net/ethernet/freescale/fec.h > index f79e57f..d89568f 100644 > --- a/drivers/net/ethernet/freescale/fec.h > +++ b/drivers/net/ethernet/freescale/fec.h > @@ -488,6 +488,12 @@ struct fec_enet_priv_rx_q { > struct sk_buff *rx_skbuff[RX_RING_SIZE]; }; > > +struct fec_stop_mode_gpr { > + struct regmap *gpr; > + u8 reg; > + u8 bit; > +}; > + > /* The FEC buffer descriptors track the ring buffers. The rx_bd_base and > * tx_bd_base always point to the base of the buffer descriptors. The > * cur_rx and cur_tx point to the currently available buffer. > @@ -562,6 +568,7 @@ struct fec_enet_private { > int hwts_tx_en; > struct delayed_work time_keep; > struct regulator *reg_phy; > + struct fec_stop_mode_gpr stop_gpr; > > unsigned int tx_align; > unsigned int rx_align; > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > index 23c5fef..3c78124 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -62,6 +62,8 @@ > #include <linux/if_vlan.h> > #include <linux/pinctrl/consumer.h> > #include <linux/prefetch.h> > +#include <linux/mfd/syscon.h> > +#include <linux/regmap.h> > #include <soc/imx/cpuidle.h> > > #include <asm/cacheflush.h> > @@ -1092,11 +1094,28 @@ static void fec_enet_reset_skb(struct > net_device *ndev) > > } > > +static void fec_enet_stop_mode(struct fec_enet_private *fep, bool > +enabled) { > + struct fec_platform_data *pdata = fep->pdev->dev.platform_data; > + struct fec_stop_mode_gpr *stop_gpr = &fep->stop_gpr; > + > + if (stop_gpr->gpr) { > + if (enabled) > + regmap_update_bits(stop_gpr->gpr, > stop_gpr->reg, > + BIT(stop_gpr->bit), > + BIT(stop_gpr->bit)); > + else > + regmap_update_bits(stop_gpr->gpr, > stop_gpr->reg, > + BIT(stop_gpr->bit), 0); > + } else if (pdata && pdata->sleep_mode_enable) { > + pdata->sleep_mode_enable(enabled); > + } > +} > + > static void > fec_stop(struct net_device *ndev) > { > struct fec_enet_private *fep = netdev_priv(ndev); > - struct fec_platform_data *pdata = fep->pdev->dev.platform_data; > u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8); > u32 val; > > @@ -1125,9 +1144,7 @@ static void fec_enet_reset_skb(struct net_device > *ndev) > val = readl(fep->hwp + FEC_ECNTRL); > val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); > writel(val, fep->hwp + FEC_ECNTRL); > - > - if (pdata && pdata->sleep_mode_enable) > - pdata->sleep_mode_enable(true); > + fec_enet_stop_mode(fep, true); > } > writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); > > @@ -3397,6 +3414,43 @@ static int fec_enet_get_irq_cnt(struct > platform_device *pdev) > return irq_cnt; > } > > +static int fec_enet_of_parse_stop_mode(struct fec_enet_private *fep, > + struct device_node *np) { > + static const char prop[] = "fsl,stop-mode"; > + struct of_phandle_args args; > + int ret; > + > + ret = of_parse_phandle_with_fixed_args(np, prop, 2, 0, &args); To save memory: ret = of_parse_phandle_with_fixed_args(np, "fsl,stop-mode", 2, 0, &args); > + if (ret == -ENOENT) > + return 0; > + if (ret) > + return ret; > + > + if (args.args_count != 2) { > + dev_err(&fep->pdev->dev, > + "Bad %s args want gpr offset, bit\n", prop); > + ret = -EINVAL; > + goto out; > + } > + > + fep->stop_gpr.gpr = syscon_node_to_regmap(args.np); > + if (IS_ERR(fep->stop_gpr.gpr)) { > + dev_err(&fep->pdev->dev, "could not find gpr regmap\n"); > + ret = PTR_ERR(fep->stop_gpr.gpr); > + fep->stop_gpr.gpr = NULL; > + goto out; > + } > + > + fep->stop_gpr.reg = args.args[0]; > + fep->stop_gpr.bit = args.args[1]; > + > +out: > + of_node_put(args.np); > + > + return ret; > +} > + > static int > fec_probe(struct platform_device *pdev) { @@ -3463,6 +3517,10 @@ > static int fec_enet_get_irq_cnt(struct platform_device *pdev) > if (of_get_property(np, "fsl,magic-packet", NULL)) > fep->wol_flag |= FEC_WOL_HAS_MAGIC_PACKET; > > + ret = fec_enet_of_parse_stop_mode(fep, np); > + if (ret) > + goto failed_stop_mode; > + > phy_node = of_parse_phandle(np, "phy-handle", 0); > if (!phy_node && of_phy_is_fixed_link(np)) { > ret = of_phy_register_fixed_link(np); @@ -3631,6 > +3689,7 @@ static int fec_enet_get_irq_cnt(struct platform_device *pdev) > if (of_phy_is_fixed_link(np)) > of_phy_deregister_fixed_link(np); > of_node_put(phy_node); > +failed_stop_mode: > failed_phy: > dev_id--; > failed_ioremap: > @@ -3708,7 +3767,6 @@ static int __maybe_unused fec_resume(struct > device *dev) { > struct net_device *ndev = dev_get_drvdata(dev); > struct fec_enet_private *fep = netdev_priv(ndev); > - struct fec_platform_data *pdata = fep->pdev->dev.platform_data; > int ret; > int val; > > @@ -3726,8 +3784,8 @@ static int __maybe_unused fec_resume(struct > device *dev) > goto failed_clk; > } > if (fep->wol_flag & FEC_WOL_FLAG_ENABLE) { > - if (pdata && pdata->sleep_mode_enable) > - pdata->sleep_mode_enable(false); > + fec_enet_stop_mode(fep, false); > + > val = readl(fep->hwp + FEC_ECNTRL); > val &= ~(FEC_ECR_MAGICEN | > FEC_ECR_SLEEP); > writel(val, fep->hwp + FEC_ECNTRL); > -- > 1.9.1
On Wed, 18 Mar 2020 at 07:26, Andy Duan <fugang.duan@nxp.com> wrote: > > From: Martin Fuzzey <martin.fuzzey@flowbird.group> Sent: Wednesday, March 18, 2020 12:50 AM > > +static int fec_enet_of_parse_stop_mode(struct fec_enet_private *fep, > > + struct device_node *np) { > > + static const char prop[] = "fsl,stop-mode"; > > + struct of_phandle_args args; > > + int ret; > > + > > + ret = of_parse_phandle_with_fixed_args(np, prop, 2, 0, &args); > To save memory: > > ret = of_parse_phandle_with_fixed_args(np, "fsl,stop-mode", 2, 0, &args); > Why would this save memory? prop is defined static const char[] (and not char *) so there will no be extra pointers. I haven't checked the generated assembler but this should generate the same code as a string litteral I think. It is also reused later in the function in a debug (which is the reason I did it this way to ensure the property name is unique and consistent. Regards, Martin --
From: Fuzzey, Martin <martin.fuzzey@flowbird.group> Sent: Wednesday, March 18, 2020 4:36 PM > On Wed, 18 Mar 2020 at 07:26, Andy Duan <fugang.duan@nxp.com> wrote: > > > > From: Martin Fuzzey <martin.fuzzey@flowbird.group> Sent: Wednesday, > > March 18, 2020 12:50 AM > > > +static int fec_enet_of_parse_stop_mode(struct fec_enet_private *fep, > > > + struct device_node *np) { > > > + static const char prop[] = "fsl,stop-mode"; > > > + struct of_phandle_args args; > > > + int ret; > > > + > > > + ret = of_parse_phandle_with_fixed_args(np, prop, 2, 0, > > > + &args); > > To save memory: > > > > ret = of_parse_phandle_with_fixed_args(np, > > "fsl,stop-mode", 2, 0, &args); > > > > Why would this save memory? > prop is defined static const char[] (and not char *) so there will no be extra > pointers. > > I haven't checked the generated assembler but this should generate the same > code as a string litteral I think. > > It is also reused later in the function in a debug (which is the reason I did it > this way to ensure the property name is unique and consistent. static variable cost memory and never is not freed if the module is built in. the later debug message cannot depend on the variable. > > Regards, > > Martin > > --
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h index f79e57f..d89568f 100644 --- a/drivers/net/ethernet/freescale/fec.h +++ b/drivers/net/ethernet/freescale/fec.h @@ -488,6 +488,12 @@ struct fec_enet_priv_rx_q { struct sk_buff *rx_skbuff[RX_RING_SIZE]; }; +struct fec_stop_mode_gpr { + struct regmap *gpr; + u8 reg; + u8 bit; +}; + /* The FEC buffer descriptors track the ring buffers. The rx_bd_base and * tx_bd_base always point to the base of the buffer descriptors. The * cur_rx and cur_tx point to the currently available buffer. @@ -562,6 +568,7 @@ struct fec_enet_private { int hwts_tx_en; struct delayed_work time_keep; struct regulator *reg_phy; + struct fec_stop_mode_gpr stop_gpr; unsigned int tx_align; unsigned int rx_align; diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 23c5fef..3c78124 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -62,6 +62,8 @@ #include <linux/if_vlan.h> #include <linux/pinctrl/consumer.h> #include <linux/prefetch.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> #include <soc/imx/cpuidle.h> #include <asm/cacheflush.h> @@ -1092,11 +1094,28 @@ static void fec_enet_reset_skb(struct net_device *ndev) } +static void fec_enet_stop_mode(struct fec_enet_private *fep, bool enabled) +{ + struct fec_platform_data *pdata = fep->pdev->dev.platform_data; + struct fec_stop_mode_gpr *stop_gpr = &fep->stop_gpr; + + if (stop_gpr->gpr) { + if (enabled) + regmap_update_bits(stop_gpr->gpr, stop_gpr->reg, + BIT(stop_gpr->bit), + BIT(stop_gpr->bit)); + else + regmap_update_bits(stop_gpr->gpr, stop_gpr->reg, + BIT(stop_gpr->bit), 0); + } else if (pdata && pdata->sleep_mode_enable) { + pdata->sleep_mode_enable(enabled); + } +} + static void fec_stop(struct net_device *ndev) { struct fec_enet_private *fep = netdev_priv(ndev); - struct fec_platform_data *pdata = fep->pdev->dev.platform_data; u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8); u32 val; @@ -1125,9 +1144,7 @@ static void fec_enet_reset_skb(struct net_device *ndev) val = readl(fep->hwp + FEC_ECNTRL); val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); writel(val, fep->hwp + FEC_ECNTRL); - - if (pdata && pdata->sleep_mode_enable) - pdata->sleep_mode_enable(true); + fec_enet_stop_mode(fep, true); } writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); @@ -3397,6 +3414,43 @@ static int fec_enet_get_irq_cnt(struct platform_device *pdev) return irq_cnt; } +static int fec_enet_of_parse_stop_mode(struct fec_enet_private *fep, + struct device_node *np) +{ + static const char prop[] = "fsl,stop-mode"; + struct of_phandle_args args; + int ret; + + ret = of_parse_phandle_with_fixed_args(np, prop, 2, 0, &args); + if (ret == -ENOENT) + return 0; + if (ret) + return ret; + + if (args.args_count != 2) { + dev_err(&fep->pdev->dev, + "Bad %s args want gpr offset, bit\n", prop); + ret = -EINVAL; + goto out; + } + + fep->stop_gpr.gpr = syscon_node_to_regmap(args.np); + if (IS_ERR(fep->stop_gpr.gpr)) { + dev_err(&fep->pdev->dev, "could not find gpr regmap\n"); + ret = PTR_ERR(fep->stop_gpr.gpr); + fep->stop_gpr.gpr = NULL; + goto out; + } + + fep->stop_gpr.reg = args.args[0]; + fep->stop_gpr.bit = args.args[1]; + +out: + of_node_put(args.np); + + return ret; +} + static int fec_probe(struct platform_device *pdev) { @@ -3463,6 +3517,10 @@ static int fec_enet_get_irq_cnt(struct platform_device *pdev) if (of_get_property(np, "fsl,magic-packet", NULL)) fep->wol_flag |= FEC_WOL_HAS_MAGIC_PACKET; + ret = fec_enet_of_parse_stop_mode(fep, np); + if (ret) + goto failed_stop_mode; + phy_node = of_parse_phandle(np, "phy-handle", 0); if (!phy_node && of_phy_is_fixed_link(np)) { ret = of_phy_register_fixed_link(np); @@ -3631,6 +3689,7 @@ static int fec_enet_get_irq_cnt(struct platform_device *pdev) if (of_phy_is_fixed_link(np)) of_phy_deregister_fixed_link(np); of_node_put(phy_node); +failed_stop_mode: failed_phy: dev_id--; failed_ioremap: @@ -3708,7 +3767,6 @@ static int __maybe_unused fec_resume(struct device *dev) { struct net_device *ndev = dev_get_drvdata(dev); struct fec_enet_private *fep = netdev_priv(ndev); - struct fec_platform_data *pdata = fep->pdev->dev.platform_data; int ret; int val; @@ -3726,8 +3784,8 @@ static int __maybe_unused fec_resume(struct device *dev) goto failed_clk; } if (fep->wol_flag & FEC_WOL_FLAG_ENABLE) { - if (pdata && pdata->sleep_mode_enable) - pdata->sleep_mode_enable(false); + fec_enet_stop_mode(fep, false); + val = readl(fep->hwp + FEC_ECNTRL); val &= ~(FEC_ECR_MAGICEN | FEC_ECR_SLEEP); writel(val, fep->hwp + FEC_ECNTRL);
On some SoCs, such as the i.MX6, it is necessary to set a bit in the SoC level GPR register before suspending for wake on lan to work. The fec platform callback sleep_mode_enable was intended to allow this but the platform implementation was NAK'd back in 2015 [1] This means that, currently, wake on lan is broken on mainline for the i.MX6 at least. So implement the required bit setting in the fec driver by itself by adding a new optional DT property indicating the register and bit to set. [1] https://www.spinics.net/lists/netdev/msg310922.html Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group> --- drivers/net/ethernet/freescale/fec.h | 7 +++ drivers/net/ethernet/freescale/fec_main.c | 72 ++++++++++++++++++++++++++++--- 2 files changed, 72 insertions(+), 7 deletions(-)