[PATCH net 0/4] net: ethernet: fec: move GPR reigster offset and bit into DT

[PATCH net 0/4] net: ethernet: fec: move GPR reigster offset and bit into DT

[fugang.duan]

From: Fugang Duan <fugang.duan@nxp.com>

The commit da722186f654(net: fec: set GPR bit on suspend by DT configuration)
set the GPR reigster offset and bit in driver for wake on lan feature.

It bring trouble to enable wake-on-lan feature on other i.MX platforms
because imx6ul/imx7d/imx8 has two instances, they have different gpr bit.

So to support wake-on-lan feature on other i.MX platforms, it should
configure the GPR reigster offset and bit from DT.


Fugang Duan (4):
  net: ethernet: fec: move GPR register offset and bit into DT
  dt-bindings: fec: update the gpr property
  ARM: dts: imx6: update fec gpr property to match new format
  ARM: dts: imx6qdl-sabresd: enable fec wake-on-lan

 Documentation/devicetree/bindings/net/fsl-fec.txt |  7 ++++++-
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi            |  1 +
 arch/arm/boot/dts/imx6qdl.dtsi                    |  2 +-
 drivers/net/ethernet/freescale/fec_main.c         | 22 +++++++++++-----------
 4 files changed, 19 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH net 1/4] net: ethernet: fec: move GPR register offset and bit into DT

[fugang.duan]

From: Fugang Duan <fugang.duan@nxp.com>

The commit da722186f654(net: fec: set GPR bit on suspend by DT
configuration) set the GPR reigster offset and bit in driver for
wake on lan feature.

But it introduces two issues here:
- one SOC has two instances, they have different bit
- different SOCs may have different offset and bit

So to support wake-on-lan feature on other i.MX platforms, it should
configure the GPR reigster offset and bit from DT.

Fixes: da722186f654(net: fec: set GPR bit on suspend by DT configuration)
Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 2e20914..9606411 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -88,8 +88,6 @@ static void fec_enet_itr_coal_init(struct net_device *ndev);
 
 struct fec_devinfo {
 	u32 quirks;
-	u8 stop_gpr_reg;
-	u8 stop_gpr_bit;
 };
 
 static const struct fec_devinfo fec_imx25_info = {
@@ -112,8 +110,6 @@ static const struct fec_devinfo fec_imx6q_info = {
 		  FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
 		  FEC_QUIRK_HAS_VLAN | FEC_QUIRK_ERR006358 |
 		  FEC_QUIRK_HAS_RACC,
-	.stop_gpr_reg = 0x34,
-	.stop_gpr_bit = 27,
 };
 
 static const struct fec_devinfo fec_mvf600_info = {
@@ -3476,19 +3472,23 @@ static int fec_enet_get_irq_cnt(struct platform_device *pdev)
 }
 
 static int fec_enet_init_stop_mode(struct fec_enet_private *fep,
-				   struct fec_devinfo *dev_info,
 				   struct device_node *np)
 {
 	struct device_node *gpr_np;
+	u32 out_val[3];
 	int ret = 0;
 
-	if (!dev_info)
-		return 0;
-
 	gpr_np = of_parse_phandle(np, "gpr", 0);
 	if (!gpr_np)
 		return 0;
 
+	ret = of_property_read_u32_array(np, "gpr", out_val,
+					 ARRAY_SIZE(out_val));
+	if (ret) {
+		dev_dbg(&fep->pdev->dev, "no stop mode property\n");
+		return ret;
+	}
+
 	fep->stop_gpr.gpr = syscon_node_to_regmap(gpr_np);
 	if (IS_ERR(fep->stop_gpr.gpr)) {
 		dev_err(&fep->pdev->dev, "could not find gpr regmap\n");
@@ -3497,8 +3497,8 @@ static int fec_enet_init_stop_mode(struct fec_enet_private *fep,
 		goto out;
 	}
 
-	fep->stop_gpr.reg = dev_info->stop_gpr_reg;
-	fep->stop_gpr.bit = dev_info->stop_gpr_bit;
+	fep->stop_gpr.reg = out_val[1];
+	fep->stop_gpr.bit = out_val[2];
 
 out:
 	of_node_put(gpr_np);
@@ -3575,7 +3575,7 @@ fec_probe(struct platform_device *pdev)
 	if (of_get_property(np, "fsl,magic-packet", NULL))
 		fep->wol_flag |= FEC_WOL_HAS_MAGIC_PACKET;
 
-	ret = fec_enet_init_stop_mode(fep, dev_info, np);
+	ret = fec_enet_init_stop_mode(fep, np);
 	if (ret)
 		goto failed_stop_mode;
 
-- 
2.7.4

[PATCH net 2/4] dt-bindings: fec: update the gpr property

[fugang.duan]

From: Fugang Duan <fugang.duan@nxp.com>

Update the gpr property to define gpr register offset and
bit in DT, since different instance have different gpr bit,
and differnet SOC may have different gpr reigster offset.

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
---
 Documentation/devicetree/bindings/net/fsl-fec.txt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
index 26c492a..c2ea818 100644
--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -23,7 +23,12 @@ Optional properties:
   the hardware workaround for ERR006687 applied and does not need a software
   workaround.
 - gpr: phandle of SoC general purpose register mode. Required for wake on LAN
-  on some SoCs
+  on some SoCs. Register bits of stop mode control, the format is
+	<&gpr req_gpr req_bit>.
+	 gpr is the phandle to general purpose register node.
+	 req_gpr is the gpr register offset for ENET stop request.
+	 req_bit is the gpr bit offset for ENET stop request.
+
  -interrupt-names:  names of the interrupts listed in interrupts property in
   the same order. The defaults if not specified are
   __Number of interrupts__   __Default__
-- 
2.7.4

[PATCH net 3/4] ARM: dts: imx6: update fec gpr property to match new format

[fugang.duan]

From: Fugang Duan <fugang.duan@nxp.com>

Update the gpr property to define gpr register offset and
bit in DT.

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
---
 arch/arm/boot/dts/imx6qdl.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 98da446..a4a68b7 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -1045,7 +1045,7 @@
 					 <&clks IMX6QDL_CLK_ENET>,
 					 <&clks IMX6QDL_CLK_ENET_REF>;
 				clock-names = "ipg", "ahb", "ptp";
-				gpr = <&gpr>;
+				gpr = <&gpr 0x34 27>;
 				status = "disabled";
 			};
 
-- 
2.7.4

[PATCH net 4/4] ARM: dts: imx6qdl-sabresd: enable fec wake-on-lan

[fugang.duan]

From: Fugang Duan <fugang.duan@nxp.com>

Enable ethernet wake-on-lan feature for imx6q/dl/qp sabresd
boards since the PHY clock is supplied by exteranl osc.

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
---
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index fe59dde..28b35cc 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -204,6 +204,7 @@
 	pinctrl-0 = <&pinctrl_enet>;
 	phy-mode = "rgmii-id";
 	phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
+	fsl,magic-packet;
 	status = "okay";
 };
 
-- 
2.7.4

Re: [PATCH net 1/4] net: ethernet: fec: move GPR register offset and bit into DT

[Jakub Kicinski]

On Wed, 20 May 2020 16:31:53 +0800 fugang.duan@nxp.com wrote:
> From: Fugang Duan <fugang.duan@nxp.com>
> 
> The commit da722186f654(net: fec: set GPR bit on suspend by DT
> configuration) set the GPR reigster offset and bit in driver for
> wake on lan feature.
> 
> But it introduces two issues here:
> - one SOC has two instances, they have different bit
> - different SOCs may have different offset and bit
> 
> So to support wake-on-lan feature on other i.MX platforms, it should
> configure the GPR reigster offset and bit from DT.
> 
> Fixes: da722186f654(net: fec: set GPR bit on suspend by DT configuration)
> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>

Fixes tag: Fixes: da722186f654(net: fec: set GPR bit on suspend by DT configuration)
Has these problem(s):
	- missing space between the SHA1 and the subject

Re: [PATCH net 3/4] ARM: dts: imx6: update fec gpr property to match new format

[Andrew Lunn]

On Wed, May 20, 2020 at 04:31:55PM +0800, fugang.duan@nxp.com wrote:
> From: Fugang Duan <fugang.duan@nxp.com>
> 
> Update the gpr property to define gpr register offset and
> bit in DT.
> 
> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> ---
>  arch/arm/boot/dts/imx6qdl.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index 98da446..a4a68b7 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -1045,7 +1045,7 @@
>  					 <&clks IMX6QDL_CLK_ENET>,
>  					 <&clks IMX6QDL_CLK_ENET_REF>;
>  				clock-names = "ipg", "ahb", "ptp";
> -				gpr = <&gpr>;
> +				gpr = <&gpr 0x34 27>;
>  				status = "disabled";
>  			};

Hi Andy

This is the same values as hard coded, so no change here.

The next patch does not use grp at all. So it is unclear to me if you
actually make use of what you just added. I don't see anywhere

gpr = <&gpr 0x42 24>;

which is the whole point of this change, being able to specify
different values.

      Andrew

RE: [EXT] Re: [PATCH net 1/4] net: ethernet: fec: move GPR register offset and bit into DT

[Andy Duan]

From: Jakub Kicinski <kuba@kernel.org> Sent: Thursday, May 21, 2020 12:54 AM
> On Wed, 20 May 2020 16:31:53 +0800 fugang.duan@nxp.com wrote:
> > From: Fugang Duan <fugang.duan@nxp.com>
> >
> > The commit da722186f654(net: fec: set GPR bit on suspend by DT
> > configuration) set the GPR reigster offset and bit in driver for wake
> > on lan feature.
> >
> > But it introduces two issues here:
> > - one SOC has two instances, they have different bit
> > - different SOCs may have different offset and bit
> >
> > So to support wake-on-lan feature on other i.MX platforms, it should
> > configure the GPR reigster offset and bit from DT.
> >
> > Fixes: da722186f654(net: fec: set GPR bit on suspend by DT
> > configuration)
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> 
> Fixes tag: Fixes: da722186f654(net: fec: set GPR bit on suspend by DT
> configuration) Has these problem(s):
>         - missing space between the SHA1 and the subject

Thanks, I will update it v2.

RE: [EXT] Re: [PATCH net 3/4] ARM: dts: imx6: update fec gpr property to match new format

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Thursday, May 21, 2020 1:03 AM
> On Wed, May 20, 2020 at 04:31:55PM +0800, fugang.duan@nxp.com wrote:
> > From: Fugang Duan <fugang.duan@nxp.com>
> >
> > Update the gpr property to define gpr register offset and bit in DT.
> >
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> > ---
> >  arch/arm/boot/dts/imx6qdl.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/imx6qdl.dtsi
> > b/arch/arm/boot/dts/imx6qdl.dtsi index 98da446..a4a68b7 100644
> > --- a/arch/arm/boot/dts/imx6qdl.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> > @@ -1045,7 +1045,7 @@
> >                                        <&clks
> IMX6QDL_CLK_ENET>,
> >                                        <&clks
> IMX6QDL_CLK_ENET_REF>;
> >                               clock-names = "ipg", "ahb", "ptp";
> > -                             gpr = <&gpr>;
> > +                             gpr = <&gpr 0x34 27>;
> >                               status = "disabled";
> >                       };
> 
> Hi Andy
> 
> This is the same values as hard coded, so no change here.
> 
> The next patch does not use grp at all. So it is unclear to me if you actually
> make use of what you just added. I don't see anywhere
> 
> gpr = <&gpr 0x42 24>;
> 
> which is the whole point of this change, being able to specify different values.
> 
>       Andrew

Andrew, patch#1 in the series will parse the property to get register offset and bit.
Patch#2 describes the property format as below:
       <&gpr req_gpr req_bit>.
        gpr is the phandle to general purpose register node.
        req_gpr is the gpr register offset for ENET stop request.
        req_bit is the gpr bit offset for ENET stop request.

All i.MX support wake-on-lan, imx6q/dl/qp is the first platforms in upstream to support it.
As you know, most of i.MX chips has two ethernet instances, they have different gpr bit.

gpr is used to enter/exit stop mode for soc. So it can be defined in dtsi file.
"fsl,magic-packet;" property is define the board wakeup capability.

I am not sure whether above information is clear for you why to add the patch set.

Andy

Re: [EXT] Re: [PATCH net 3/4] ARM: dts: imx6: update fec gpr property to match new format

[Andrew Lunn]

> Andrew, patch#1 in the series will parse the property to get register offset and bit.
> Patch#2 describes the property format as below:
>        <&gpr req_gpr req_bit>.
>         gpr is the phandle to general purpose register node.
>         req_gpr is the gpr register offset for ENET stop request.
>         req_bit is the gpr bit offset for ENET stop request.
> 
> All i.MX support wake-on-lan, imx6q/dl/qp is the first platforms in upstream to support it.
> As you know, most of i.MX chips has two ethernet instances, they have different gpr bit.
> 
> gpr is used to enter/exit stop mode for soc. So it can be defined in dtsi file.
> "fsl,magic-packet;" property is define the board wakeup capability.
> 
> I am not sure whether above information is clear for you why to add the patch set.

I understand the patch. What is missing is an actual user, where you
have two interfaces, doing WOL, with different values for gpr. We
don't add new kernel APIs without a user.

    Andrew

RE: [EXT] Re: [PATCH net 3/4] ARM: dts: imx6: update fec gpr property to match new format

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Thursday, May 21, 2020 9:07 PM
> > Andrew, patch#1 in the series will parse the property to get register offset
> and bit.
> > Patch#2 describes the property format as below:
> >        <&gpr req_gpr req_bit>.
> >         gpr is the phandle to general purpose register node.
> >         req_gpr is the gpr register offset for ENET stop request.
> >         req_bit is the gpr bit offset for ENET stop request.
> >
> > All i.MX support wake-on-lan, imx6q/dl/qp is the first platforms in upstream
> to support it.
> > As you know, most of i.MX chips has two ethernet instances, they have
> different gpr bit.
> >
> > gpr is used to enter/exit stop mode for soc. So it can be defined in dtsi file.
> > "fsl,magic-packet;" property is define the board wakeup capability.
> >
> > I am not sure whether above information is clear for you why to add the
> patch set.
> 
> I understand the patch. What is missing is an actual user, where you have two
> interfaces, doing WOL, with different values for gpr. We don't add new kernel
> APIs without a user.
> 
>     Andrew

Andrew, many customers require the wol feature, NXP NPI release always support
the wol feature to match customers requirement.

And some customers' board only design one ethernet instance based on imx6sx/imx7d/
Imx8 serial, but which instance we never know, maybe enet1, maybe enet2. So we should
supply different values for gpr.

So, it is very necessary to support wol feature for multiple instances.

Andy

Re: [EXT] Re: [PATCH net 3/4] ARM: dts: imx6: update fec gpr property to match new format

[Fuzzey, Martin]

Hi Andy,

On Fri, 22 May 2020, 03:01 Andy Duan, <fugang.duan@nxp.com> wrote:
>
> Andrew, many customers require the wol feature, NXP NPI release always support
> the wol feature to match customers requirement.
>
> And some customers' board only design one ethernet instance based on imx6sx/imx7d/
> Imx8 serial, but which instance we never know, maybe enet1, maybe enet2. So we should
> supply different values for gpr.
>
> So, it is very necessary to support wol feature for multiple instances.
>

Yes, I don't think anyone is saying otherwise.

The problem is just that there are already .dtsi files for i.MX chips
having multiple ethernet interfaces
in the mainline kernel (at least imx6ui.dtsi, imx6sx.dts, imx7d.dtsi)
but that this patch series does not
modify those files to use the new DT format.

It currently only modifies the dts files that are already supported by
hardcoded values in the driver.

As to not knowing which instance it shouldn't matter.
The base dtsi can declare both/all ethernet interfaces with the
appropriate GPR bits.
Both set to status = "disabled".

Then the board specific dts file sets status="okay" and activates wol
by adding "
"fsl,magic-packet" if the hardaware supports it
(because that depends on things beyond the SoC, like how the ethernet
PHY is clocked and powered.)

Martin

Re: [EXT] Re: [PATCH net 3/4] ARM: dts: imx6: update fec gpr property to match new format

[Andrew Lunn]

> Yes, I don't think anyone is saying otherwise.

Correct.

> 
> The problem is just that there are already .dtsi files for i.MX chips
> having multiple ethernet interfaces
> in the mainline kernel (at least imx6ui.dtsi, imx6sx.dts, imx7d.dtsi)

Vybrid is one i use a lot with two FECs.

> but that this patch series does not
> modify those files to use the new DT format.
> 
> It currently only modifies the dts files that are already supported by
> hardcoded values in the driver.

Exactly. This patch set itself adds nothing we don't already support.
So the patch set as is, is pointless.

> As to not knowing which instance it shouldn't matter.
> The base dtsi can declare both/all ethernet interfaces with the
> appropriate GPR bits.

I fully agree. All it needs for this patchset to be merged is another
patch which adds GPR properties to all SoC .dtsi files where
appropriate, and optionally to a couple of reference designs which
support WoL on their ports.

	Andrew

Re: [PATCH net 1/4] net: ethernet: fec: move GPR register offset and bit into DT

[Fuzzey, Martin]

Hi Andy,

> Fixes: da722186f654(net: fec: set GPR bit on suspend by DT configuration)

Just a nitpick maybe but I don't really think this need as Fixes: tag.
That commit didn't actually *break* anything AFAIK.
It added WoL support for *some* SoCs that didn't have any in mainline
and didn't hurt the others.
Of course it turned out to be insufficient for the multiple FEC case
so this patch series is a welcome improvement.


>  struct fec_devinfo {
>         u32 quirks;
> -       u8 stop_gpr_reg;
> -       u8 stop_gpr_bit;
>  };

This structure has become redundant now that it only contains a single
u32 quirks field.
So we *could* go back to storing the quirks bitmask directly in
.driver_data as was done before.

It's a slight wastage to keep the, now unnecessary, indirection,
though the size impact is small
and it's only used at probe() time not on a hot path.

But switching back could be seen as code churn too...

I don't have a strong opinion on this, so just noting it to see what
others think.

Martin

Re: [PATCH net 2/4] dt-bindings: fec: update the gpr property

[Fuzzey, Martin]

>  - gpr: phandle of SoC general purpose register mode. Required for wake on LAN
> -  on some SoCs
> +  on some SoCs. Register bits of stop mode control, the format is
> +       <&gpr req_gpr req_bit>.
> +        gpr is the phandle to general purpose register node.
> +        req_gpr is the gpr register offset for ENET stop request.
> +        req_bit is the gpr bit offset for ENET stop request.
>

More of a DT binding changes policy question, do we care about
supporting the old
no argument binding too?

I don't think it actually matters seeing as the no argument gpr node
binding was only added recently anyway.
But it was backported to the stable trees and
Documentation/bindings/ABI.txt says

   "Bindings can be augmented, but the driver shouldn't break when given
     the old binding. ie. add additional properties, but don't change the
     meaning of an existing property. For drivers, default to the original
     behaviour when a newly added property is missing."

Myself I think this is overkill in this case and am fine with just
changing the binding without the driver handling the old case but
that's Rob's call to make I think.

Martin

RE: [EXT] Re: [PATCH net 3/4] ARM: dts: imx6: update fec gpr property to match new format

[Andy Duan]

From: Fuzzey, Martin <martin.fuzzey@flowbird.group> Sent: Saturday, May 23, 2020 2:03 AM
> Hi Andy,
> 
> On Fri, 22 May 2020, 03:01 Andy Duan, <fugang.duan@nxp.com> wrote:
> >
> > Andrew, many customers require the wol feature, NXP NPI release always
> > support the wol feature to match customers requirement.
> >
> > And some customers' board only design one ethernet instance based on
> > imx6sx/imx7d/
> > Imx8 serial, but which instance we never know, maybe enet1, maybe
> > enet2. So we should supply different values for gpr.
> >
> > So, it is very necessary to support wol feature for multiple instances.
> >
> 
> Yes, I don't think anyone is saying otherwise.
> 
> The problem is just that there are already .dtsi files for i.MX chips having
> multiple ethernet interfaces in the mainline kernel (at least imx6ui.dtsi,
> imx6sx.dts, imx7d.dtsi) but that this patch series does not modify those files
> to use the new DT format.
> 
For imx6ul/imx6sx/imx7d/imx8mq/imx8mm/imx8mn chips to support wol, 
I plan to submit another dts patch after the patch set is accepted.

If you think add the dts patch appending to the patch set, I will add it in v2.

> It currently only modifies the dts files that are already supported by
> hardcoded values in the driver.
> 
> As to not knowing which instance it shouldn't matter.
> The base dtsi can declare both/all ethernet interfaces with the appropriate
> GPR bits.
> Both set to status = "disabled".
> 
> Then the board specific dts file sets status="okay" and activates wol by adding
> "
> "fsl,magic-packet" if the hardaware supports it (because that depends on
> things beyond the SoC, like how the ethernet PHY is clocked and powered.)
> 
> Martin

RE: [EXT] Re: [PATCH net 3/4] ARM: dts: imx6: update fec gpr property to match new format

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Saturday, May 23, 2020 7:50 AM
> > Yes, I don't think anyone is saying otherwise.
> 
> Correct.
> 
> >
> > The problem is just that there are already .dtsi files for i.MX chips
> > having multiple ethernet interfaces in the mainline kernel (at least
> > imx6ui.dtsi, imx6sx.dts, imx7d.dtsi)
> 
> Vybrid is one i use a lot with two FECs.
> 
> > but that this patch series does not
> > modify those files to use the new DT format.
> >
> > It currently only modifies the dts files that are already supported by
> > hardcoded values in the driver.
> 
> Exactly. This patch set itself adds nothing we don't already support.
> So the patch set as is, is pointless.
> 
> > As to not knowing which instance it shouldn't matter.
> > The base dtsi can declare both/all ethernet interfaces with the
> > appropriate GPR bits.
> 
> I fully agree. All it needs for this patchset to be merged is another patch which
> adds GPR properties to all SoC .dtsi files where appropriate, and optionally to
> a couple of reference designs which support WoL on their ports.
> 
>         Andrew

Okay, I will add imx6ul/imx6sx/imx7d/imx8mq/imx8mm/imx8mn dts change
into the patch set in v2 version. (before, I plan to submit another patch for them
once the patch set is accepted).

RE: [EXT] Re: [PATCH net 1/4] net: ethernet: fec: move GPR register offset and bit into DT

[Andy Duan]

From: Fuzzey, Martin <martin.fuzzey@flowbird.group> Sent: Saturday, May 23, 2020 5:56 PM
> Hi Andy,
> 
> > Fixes: da722186f654(net: fec: set GPR bit on suspend by DT
> > configuration)
> 
> Just a nitpick maybe but I don't really think this need as Fixes: tag.
> That commit didn't actually *break* anything AFAIK.
> It added WoL support for *some* SoCs that didn't have any in mainline and
> didn't hurt the others.
> Of course it turned out to be insufficient for the multiple FEC case so this
> patch series is a welcome improvement.
Sorry, not to hurt you intentionally, I think the commit da722186f654 break multiple instances.
Totally I accept your suggestion, it should be improvement !

> 
> 
> >  struct fec_devinfo {
> >         u32 quirks;
> > -       u8 stop_gpr_reg;
> > -       u8 stop_gpr_bit;
> >  };
> 
> This structure has become redundant now that it only contains a single
> u32 quirks field.
> So we *could* go back to storing the quirks bitmask directly in .driver_data as
> was done before.
> 
> It's a slight wastage to keep the, now unnecessary, indirection, though the
> size impact is small and it's only used at probe() time not on a hot path.
> 
> But switching back could be seen as code churn too...
> 
> I don't have a strong opinion on this, so just noting it to see what others think.
> 
> Martin

To make code clean, we should switch back. I will change it in V2.
Thanks again for your review.

RE: [EXT] Re: [PATCH net 2/4] dt-bindings: fec: update the gpr property

[Andy Duan]

From: Fuzzey, Martin <martin.fuzzey@flowbird.group> Sent: Saturday, May 23, 2020 6:16 PM
> >  - gpr: phandle of SoC general purpose register mode. Required for
> > wake on LAN
> > -  on some SoCs
> > +  on some SoCs. Register bits of stop mode control, the format is
> > +       <&gpr req_gpr req_bit>.
> > +        gpr is the phandle to general purpose register node.
> > +        req_gpr is the gpr register offset for ENET stop request.
> > +        req_bit is the gpr bit offset for ENET stop request.
> >
> 
> More of a DT binding changes policy question, do we care about supporting
> the old no argument binding too?
> 
> I don't think it actually matters seeing as the no argument gpr node binding
> was only added recently anyway.
> But it was backported to the stable trees and Documentation/bindings/ABI.txt
> says
> 
>    "Bindings can be augmented, but the driver shouldn't break when given
>      the old binding. ie. add additional properties, but don't change the
>      meaning of an existing property. For drivers, default to the original
>      behaviour when a newly added property is missing."
> 
> Myself I think this is overkill in this case and am fine with just changing the
> binding without the driver handling the old case but that's Rob's call to make I
> think.

The patch set is to add argument binding, and driver also doesn't support wol
without argument binding.

As you know, current driver only wol feature requests the property.
I am not understand why we need to support the old without argument binding.

Welcome to your suggestion for the solution.

And 'gpr' string is not good description for stop mode, I will change it to the string:
' fsl,stop-mode'.