net: fec: generate warning when using deprecated phy reset
diff mbox series

Message ID 20190718143428.2392-1-TheSven73@gmail.com
State Rejected
Delegated to: David Miller
Headers show
Series
  • net: fec: generate warning when using deprecated phy reset
Related show

Commit Message

Sven Van Asbroeck July 18, 2019, 2:34 p.m. UTC
Allowing the fec to reset its PHY via the phy-reset-gpios
devicetree property is deprecated. To improve developer
awareness, generate a warning whenever the deprecated
property is used.

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Lucas Stach July 18, 2019, 4:47 p.m. UTC | #1
Am Donnerstag, den 18.07.2019, 10:34 -0400 schrieb Sven Van Asbroeck:
> Allowing the fec to reset its PHY via the phy-reset-gpios
> devicetree property is deprecated. To improve developer
> awareness, generate a warning whenever the deprecated
> property is used.

Not really a fan of this. This will cause existing DTs, which are
provided by the firmware in an ideal world and may not change at the
same rate as the kernel, to generate a warning with new kernels. Not
really helpful from the user experience point of view.

Regards,
Lucas

> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 38f10f7dcbc3..00e1b5e4ef71 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3244,6 +3244,12 @@ static int fec_reset_phy(struct platform_device *pdev)
> >  	else if (!gpio_is_valid(phy_reset))
> >  		return 0;
>  
> > +	/* Recommended way to provide a PHY reset:
> > +	 * - create a phy devicetree node, and link it to its fec (phy-handle)
> > +	 * - add your reset gpio to the phy devicetree node
> > +	 */
> > +	dev_warn(&pdev->dev, "devicetree: phy-reset-gpios is deprecated\n");
> +
> >  	err = of_property_read_u32(np, "phy-reset-post-delay", &phy_post_delay);
> >  	/* valid reset duration should be less than 1s */
> >  	if (!err && phy_post_delay > 1000)
Fabio Estevam July 18, 2019, 4:52 p.m. UTC | #2
On Thu, Jul 18, 2019 at 1:49 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Donnerstag, den 18.07.2019, 10:34 -0400 schrieb Sven Van Asbroeck:
> > Allowing the fec to reset its PHY via the phy-reset-gpios
> > devicetree property is deprecated. To improve developer
> > awareness, generate a warning whenever the deprecated
> > property is used.
>
> Not really a fan of this. This will cause existing DTs, which are
> provided by the firmware in an ideal world and may not change at the
> same rate as the kernel, to generate a warning with new kernels. Not
> really helpful from the user experience point of view.

I agree. I don't think this warning is useful.
Sven Van Asbroeck July 18, 2019, 5:21 p.m. UTC | #3
Lucas, Fabio,

On Thu, Jul 18, 2019 at 12:52 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> > Not really a fan of this. This will cause existing DTs, which are
> > provided by the firmware in an ideal world and may not change at the
> > same rate as the kernel, to generate a warning with new kernels. Not
> > really helpful from the user experience point of view.
>
> I agree. I don't think this warning is useful.

Few users watch the dmesg log, But I totally see your point.

The problem I'm trying to address here is this: when I want to
reset the fec phy, I go look at existing devicetrees. Nearly
all of them use phy-reset-gpios, so that's what I'll use. But,
when I try to upstream the patch, the maintainer will tell me:
"no, that is deprecated, use this other method".

Is there a good solution you can think of which would point
the unwary developer to the correct phy reset method?

See my previous thread here:
https://lkml.org/lkml/2019/7/15/1764
David Miller July 18, 2019, 7:06 p.m. UTC | #4
From: Sven Van Asbroeck <thesven73@gmail.com>
Date: Thu, 18 Jul 2019 10:34:28 -0400

> Allowing the fec to reset its PHY via the phy-reset-gpios
> devicetree property is deprecated. To improve developer
> awareness, generate a warning whenever the deprecated
> property is used.
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>

The device tree documentation in the kernel tree is where information
like this belongs.  Yelling at the user in the kernel logs is not.
Sven Van Asbroeck July 18, 2019, 7:26 p.m. UTC | #5
On Thu, Jul 18, 2019 at 3:06 PM David Miller <davem@davemloft.net> wrote:
>
> The device tree documentation in the kernel tree is where information
> like this belongs.  Yelling at the user in the kernel logs is not.

Good point, thank you. I'll post a patch which will do exactly that.
Andrew Lunn July 18, 2019, 7:41 p.m. UTC | #6
On Thu, Jul 18, 2019 at 01:21:36PM -0400, Sven Van Asbroeck wrote:
> Lucas, Fabio,
> 
> On Thu, Jul 18, 2019 at 12:52 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > > Not really a fan of this. This will cause existing DTs, which are
> > > provided by the firmware in an ideal world and may not change at the
> > > same rate as the kernel, to generate a warning with new kernels. Not
> > > really helpful from the user experience point of view.
> >
> > I agree. I don't think this warning is useful.
> 
> Few users watch the dmesg log, But I totally see your point.
> 
> The problem I'm trying to address here is this: when I want to
> reset the fec phy, I go look at existing devicetrees. Nearly
> all of them use phy-reset-gpios, so that's what I'll use. But,
> when I try to upstream the patch, the maintainer will tell me:
> "no, that is deprecated, use this other method".
> 
> Is there a good solution you can think of which would point
> the unwary developer to the correct phy reset method?

Hi Sven

One option would be to submit a patch or a patchset changing all
existing device tree files to make use of the core method. Anybody
cut/pasting will then automatically use the correct core way of doing
it.

There is also a move towards using YAML to verify the correctness of
DT files. It should be possible to mark the old property as
deprecated, so there will be a build time warning, not a boot time
warning.

	Andrew
Sven Van Asbroeck July 18, 2019, 7:52 p.m. UTC | #7
Hi Andrew,

On Thu, Jul 18, 2019 at 3:41 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Hi Sven
>
> One option would be to submit a patch or a patchset changing all
> existing device tree files to make use of the core method. Anybody
> cut/pasting will then automatically use the correct core way of doing
> it.
>
> There is also a move towards using YAML to verify the correctness of
> DT files. It should be possible to mark the old property as
> deprecated, so there will be a build time warning, not a boot time
> warning.
>

Thanks for the helpful suggestions, that makes sense.

What I keep forgetting in my little arm-imx6 world, is that devicetrees
aren't in-kernel apis, but that they have out-of-kernel
dependencies. It makes more sense to to see them as userspace
apis, albeit directed at firmware/bootloaders, right?

So if bootloaders were as varied/uncontrollable as userspace,
then deprecated properties would have to be supported forever...
Andrew Lunn July 18, 2019, 7:59 p.m. UTC | #8
> What I keep forgetting in my little arm-imx6 world, is that devicetrees
> aren't in-kernel apis, but that they have out-of-kernel
> dependencies. It makes more sense to to see them as userspace
> apis, albeit directed at firmware/bootloaders, right?

It is an ongoing debate, but generally they should be considered ABI
and follow the ABI rules about not breaking backwards compatibility.

However, there is also an argument that something like a NAS box
running Debian is going to use the DT blob which came with the kernel,
so deprecated DT properties and the code to support them could be
removed after a period of time.

	Andrew

Patch
diff mbox series

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 38f10f7dcbc3..00e1b5e4ef71 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3244,6 +3244,12 @@  static int fec_reset_phy(struct platform_device *pdev)
 	else if (!gpio_is_valid(phy_reset))
 		return 0;
 
+	/* Recommended way to provide a PHY reset:
+	 * - create a phy devicetree node, and link it to its fec (phy-handle)
+	 * - add your reset gpio to the phy devicetree node
+	 */
+	dev_warn(&pdev->dev, "devicetree: phy-reset-gpios is deprecated\n");
+
 	err = of_property_read_u32(np, "phy-reset-post-delay", &phy_post_delay);
 	/* valid reset duration should be less than 1s */
 	if (!err && phy_post_delay > 1000)