[PATCH 0/4] Fix Wake on lan with FEC on i.MX6

[PATCH 0/4] Fix Wake on lan with FEC on i.MX6

[Martin Fuzzey]

This series fixes WoL support with the FEC on i.MX6
The support was already in mainline but seems to have bitrotted
somewhat.

Only tested with i.MX6DL


Martin Fuzzey (4):
  net: fec: set GPR bit on suspend by DT connfiguration.
  ARM: dts: imx6: Use gpc for FEC interrupt controller to fix wake on
    LAN.
  dt-bindings: fec: document the new fsl,stop-mode property
  ARM: dts: imx6: add fsl,stop-mode property.

 Documentation/devicetree/bindings/net/fsl-fec.txt |  5 ++
 arch/arm/boot/dts/imx6qdl.dtsi                    |  6 +-
 drivers/net/ethernet/freescale/fec.h              |  7 +++
 drivers/net/ethernet/freescale/fec_main.c         | 72 ++++++++++++++++++++---
 4 files changed, 80 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH 1/4] net: fec: set GPR bit on suspend by DT connfiguration.

[Martin Fuzzey]

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(-)

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);
-- 
1.9.1

[PATCH 2/4] ARM: dts: imx6: Use gpc for FEC interrupt controller to fix wake on LAN.

[Martin Fuzzey]

In order to wake from suspend by ethernet magic packets the GPC must be
used as intc does not have wakeup functionality.

But the FEC DT node currently uses interrupt-extended, specificying intc,
this breaking WoL.

This problem is probably fallout from the stacked domain conversion as
intc used to chain to GPC.

So replace "interrupts-extended" by "interrupts" to use the default
parent which is GPC.

Fixes: b923ff6af0d5 ("ARM: imx6: convert GPC to stacked domains")

Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
---
 arch/arm/boot/dts/imx6qdl.dtsi | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index e6b4b85..bc488df 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -1039,9 +1039,8 @@
 				compatible = "fsl,imx6q-fec";
 				reg = <0x02188000 0x4000>;
 				interrupt-names = "int0", "pps";
-				interrupts-extended =
-					<&intc 0 118 IRQ_TYPE_LEVEL_HIGH>,
-					<&intc 0 119 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <0 118 IRQ_TYPE_LEVEL_HIGH>,
+					     <0 119 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6QDL_CLK_ENET>,
 					 <&clks IMX6QDL_CLK_ENET>,
 					 <&clks IMX6QDL_CLK_ENET_REF>;
-- 
1.9.1

[PATCH 3/4] dt-bindings: fec: document the new fsl,stop-mode property

[Martin Fuzzey]

This property allows the appropriate GPR register bit to be set
for wake on lan support.

Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
---
 Documentation/devicetree/bindings/net/fsl-fec.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
index 5b88fae0..bd0ef5e 100644
--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -19,6 +19,11 @@ Optional properties:
   number to 1.
 - fsl,magic-packet : If present, indicates that the hardware supports waking
   up via magic packet.
+- fsl,stop-mode: register bits of stop mode control, the format is
+		 <&gpr reg bit>.
+		 gpr is the phandle to general purpose register node.
+		 reg is the gpr register offset for the stop request.
+		 bit is the bit offset for the stop request.
 - fsl,err006687-workaround-present: If present indicates that the system has
   the hardware workaround for ERR006687 applied and does not need a software
   workaround.
-- 
1.9.1

[PATCH 4/4] ARM: dts: imx6: add fsl,stop-mode property.

[Martin Fuzzey]

This is required for wake on lan on i.MX6

Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
---
 arch/arm/boot/dts/imx6qdl.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index bc488df..49c0527 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -1045,6 +1045,7 @@
 					 <&clks IMX6QDL_CLK_ENET>,
 					 <&clks IMX6QDL_CLK_ENET_REF>;
 				clock-names = "ipg", "ahb", "ptp";
+				fsl,stop-mode = <&gpr 0x34 27>;
 				status = "disabled";
 			};
 
-- 
1.9.1

RE: [EXT] [PATCH 1/4] net: fec: set GPR bit on suspend by DT connfiguration.

[Andy Duan]

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&amp;data=02%7C01%7Cf
> ugang.duan%40nxp.com%7Ca9e64936ec9f45edc57108d7ca9340dd%7C686e
> a1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637200606109625887&am
> p;sdata=%2Bg4NIxZRwNY331k9cq5s6OIm22qD5qstiDIVlSQiL8E%3D&amp;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

RE: [EXT] [PATCH 0/4] Fix Wake on lan with FEC on i.MX6

[Andy Duan]

From: Martin Fuzzey <martin.fuzzey@flowbird.group> Sent: Wednesday, March 18, 2020 12:50 AM
> This series fixes WoL support with the FEC on i.MX6 The support was already
> in mainline but seems to have bitrotted somewhat.
> 
> Only tested with i.MX6DL

The most of code is reused from nxp internal tree,
If you refer the patches from nxp kernel tree, please
keep the signed-off with original author.

> 
> 
> Martin Fuzzey (4):
>   net: fec: set GPR bit on suspend by DT connfiguration.
>   ARM: dts: imx6: Use gpc for FEC interrupt controller to fix wake on
>     LAN.
>   dt-bindings: fec: document the new fsl,stop-mode property
>   ARM: dts: imx6: add fsl,stop-mode property.
> 
>  Documentation/devicetree/bindings/net/fsl-fec.txt |  5 ++
>  arch/arm/boot/dts/imx6qdl.dtsi                    |  6 +-
>  drivers/net/ethernet/freescale/fec.h              |  7 +++
>  drivers/net/ethernet/freescale/fec_main.c         | 72
> ++++++++++++++++++++---
>  4 files changed, 80 insertions(+), 10 deletions(-)
> 
> --
> 1.9.1

Re: [EXT] [PATCH 0/4] Fix Wake on lan with FEC on i.MX6

[Fuzzey, Martin]

Hi Andy,


On Wed, 18 Mar 2020 at 07:28, Andy Duan <fugang.duan@nxp.com> wrote:
>
>
> The most of code is reused from nxp internal tree,
> If you refer the patches from nxp kernel tree, please
> keep the signed-off with original author.
>

Ok, looks like it was originally from you, should I add your current
email address or the one at the time (B38611@freescale.com)?

Actually I don't have the NXP tree but a Digi tree, which is probably
based on it.

I think, generally, this is a bit of a grey area.

While I would always keep a SoB if I base a patch on an old mailing
list patch submission (and contact the person if possible first to
ask) I'm not sure if a SoB from a "random git tree" indicates they
wish a mainline submission with their name (some people may want
credit and others not want to be considered responsible for bugs...).
I didn't see a submission of this version (with the gpr information
coming from the DT) on the mailing lists, only the initial version
using the platform callback that was NAKd (I may have missed it
though).

Regards,

Martin

RE: [EXT] [PATCH 0/4] Fix Wake on lan with FEC on i.MX6

[Andy Duan]

From: Fuzzey, Martin <martin.fuzzey@flowbird.group> Sent: Wednesday, March 18, 2020 4:28 PM
> Hi Andy,
> 
> 
> On Wed, 18 Mar 2020 at 07:28, Andy Duan <fugang.duan@nxp.com> wrote:
> >
> >
> > The most of code is reused from nxp internal tree, If you refer the
> > patches from nxp kernel tree, please keep the signed-off with original
> > author.
> >

It doesn't matter, I am happy you upstream the patch.
Just keep: Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> 
> Ok, looks like it was originally from you, should I add your current email
> address or the one at the time (B38611@freescale.com)?
> 
> Actually I don't have the NXP tree but a Digi tree, which is probably based on
> it.
> 
> I think, generally, this is a bit of a grey area.
> 
> While I would always keep a SoB if I base a patch on an old mailing list patch
> submission (and contact the person if possible first to
> ask) I'm not sure if a SoB from a "random git tree" indicates they wish a
> mainline submission with their name (some people may want credit and
> others not want to be considered responsible for bugs...).
> I didn't see a submission of this version (with the gpr information coming from
> the DT) on the mailing lists, only the initial version using the platform callback
> that was NAKd (I may have missed it though).
> 
> Regards,
> 
> Martin

Re: [EXT] [PATCH 1/4] net: fec: set GPR bit on suspend by DT connfiguration.

[Fuzzey, Martin]

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

--

RE: [EXT] [PATCH 1/4] net: fec: set GPR bit on suspend by DT connfiguration.

[Andy Duan]

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
> 
> --

Re: [PATCH 3/4] dt-bindings: fec: document the new fsl,stop-mode property

[Andrew Lunn]

On Tue, Mar 17, 2020 at 05:50:05PM +0100, Martin Fuzzey wrote:
> This property allows the appropriate GPR register bit to be set
> for wake on lan support.
> 
> Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
> ---
>  Documentation/devicetree/bindings/net/fsl-fec.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> index 5b88fae0..bd0ef5e 100644
> --- a/Documentation/devicetree/bindings/net/fsl-fec.txt
> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> @@ -19,6 +19,11 @@ Optional properties:
>    number to 1.
>  - fsl,magic-packet : If present, indicates that the hardware supports waking
>    up via magic packet.
> +- fsl,stop-mode: register bits of stop mode control, the format is
> +		 <&gpr reg bit>.
> +		 gpr is the phandle to general purpose register node.
> +		 reg is the gpr register offset for the stop request.
> +		 bit is the bit offset for the stop request.

Hi Martin

You should not be putting registers and values into device tree.

The regmap is fine. But could you add the register and the bit to
fec_devtype[IMX6SX_FEC], fec_devtype[IMX6UL_FEC], etc.

	Andrew

Re: [PATCH 3/4] dt-bindings: fec: document the new fsl,stop-mode property

[Fuzzey, Martin]

Hi Andrew,

On Wed, 18 Mar 2020 at 10:17, Andrew Lunn <andrew@lunn.ch> wrote:
>
> You should not be putting registers and values into device tree.
>
> The regmap is fine. But could you add the register and the bit to
> fec_devtype[IMX6SX_FEC], fec_devtype[IMX6UL_FEC], etc.
>

If that's the consensus I can do it that way.

But I should point out that there is already a precedent in mainline for this:

Documentation/devicetree/bindings/net/can/fsl-flexcan.txt

Regards,

Martin

Re: [PATCH 3/4] dt-bindings: fec: document the new fsl,stop-mode property

[Andrew Lunn]

On Wed, Mar 18, 2020 at 10:28:48AM +0100, Fuzzey, Martin wrote:
> Hi Andrew,
> 
> On Wed, 18 Mar 2020 at 10:17, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > You should not be putting registers and values into device tree.
> >
> > The regmap is fine. But could you add the register and the bit to
> > fec_devtype[IMX6SX_FEC], fec_devtype[IMX6UL_FEC], etc.
> >
> 
> If that's the consensus I can do it that way.
> 
> But I should point out that there is already a precedent in mainline for this:
> 
> Documentation/devicetree/bindings/net/can/fsl-flexcan.txt

Hi Martin

And there are probably hundreds of emails saying don't do this.

	Andrew