| Message ID | 1589963516-26703-1-git-send-email-fugang.duan@nxp.com |
|---|---|
| Headers | show |
| Series | net: ethernet: fec: move GPR reigster offset and bit into DT | expand |
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
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
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.
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
> 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
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
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
> 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
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
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
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).
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.
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(-)