[PATCH 1/3] net: fec: set phy reset time to 1ms

[PATCH 1/3] net: fec: set phy reset time to 1ms

[Stefan Christ]

According to the device tree bindings in dts/Bindings/net/fsl-fec.txt
the default phy-reset time is 1ms.

Signed-off-by: Stefan Christ <s.christ@phytec.de>
---
 drivers/net/fec_imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 78ccb85..f19d046 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -692,7 +692,7 @@ static int fec_probe(struct device_d *dev)
 		if (ret)
 			goto err_free;
 
-		udelay(10);
+		mdelay(1);
 		gpio_set_value(phy_reset, 1);
 	}
 
-- 
1.9.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] net: fec: implement dtb property phy-reset-duration

[Stefan Christ]

Implement device tree property phy-reset-duration to adjust length of
phy reset. Resetting the reset time to 1ms when it's greater than 1000ms
is the documented behavior in dts/Bindings/net/fsl-fec.txt and in the
linux kernel.

Signed-off-by: Stefan Christ <s.christ@phytec.de>
---
 drivers/net/fec_imx.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index f19d046..39a5cea 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -654,6 +654,7 @@ static int fec_probe(struct device_d *dev)
 	int ret;
 	enum fec_type type;
 	int phy_reset;
+	u32 msec = 1;
 
 	ret = dev_get_drvdata(dev, (const void **)&type);
 	if (ret)
@@ -684,6 +685,11 @@ static int fec_probe(struct device_d *dev)
 
 	phy_reset = of_get_named_gpio(dev->device_node, "phy-reset-gpios", 0);
 	if (gpio_is_valid(phy_reset)) {
+		of_property_read_u32(dev->device_node, "phy-reset-duration", &msec);
+		/* A sane reset duration should not be longer than 1s */
+		if (msec > 1000)
+			msec = 1;
+
 		ret = gpio_request(phy_reset, "phy-reset");
 		if (ret)
 			goto err_free;
@@ -692,7 +698,7 @@ static int fec_probe(struct device_d *dev)
 		if (ret)
 			goto err_free;
 
-		mdelay(1);
+		mdelay(msec);
 		gpio_set_value(phy_reset, 1);
 	}
 
-- 
1.9.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/3] net: fec: implement dtb property phy-reset-duration

[Stefan Christ]

Hi,
  
> shouldn't this be msec = 1000.
> 
> Here you set it to 1ms if it used to be over 1s.

Yeah, that would be the obvious and plausible thing to do, but the driver in
the linux kernel does it this way and it's documented in the device tree
bindings.  So I'm not sure if we can change the default behavior here.

Mit freundlichen Grüßen / Kind regards,
	Stefan Christ

On Mon, Nov 16, 2015 at 11:03:32AM +0100, Holger Schurig wrote:
> 2015-11-16 10:00 GMT+01:00 Stefan Christ <s.christ@phytec.de>:
> 
> > +               /* A sane reset duration should not be longer than 1s */
> > +               if (msec > 1000)
> > +                       msec = 1;
> >
> 
> Hi Stefan,
> 
> shouldn't this be msec = 1000.
> 
> Here you set it to 1ms if it used to be over 1s.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/3] net: fec: implement dtb property phy-reset-duration

[Sascha Hauer]

On Mon, Nov 16, 2015 at 11:17:05AM +0100, Stefan Christ wrote:
> Hi,
>   
> > shouldn't this be msec = 1000.
> > 
> > Here you set it to 1ms if it used to be over 1s.
> 
> Yeah, that would be the obvious and plausible thing to do, but the driver in
> the linux kernel does it this way and it's documented in the device tree
> bindings.  So I'm not sure if we can change the default behavior here.

Yes, we can ;)

The rationale behind this code may be: The reset duration is
unreasonably high (>1s), so that it to a sane default (1ms).

I suggest to drop the check completely. If someone ever starts barebox
with an insanely high value (maybe he thought the value is in us) then
he won't notice the mistake if barebox silently falls back to 1ms as
duration. If instead barebox waits for multiple seconds then he will
notice and it the device tree which is what we want.

Sascha


> 
> Mit freundlichen Grüßen / Kind regards,
> 	Stefan Christ
> 
> On Mon, Nov 16, 2015 at 11:03:32AM +0100, Holger Schurig wrote:
> > 2015-11-16 10:00 GMT+01:00 Stefan Christ <s.christ@phytec.de>:
> > 
> > > +               /* A sane reset duration should not be longer than 1s */
> > > +               if (msec > 1000)
> > > +                       msec = 1;
> > >
> > 
> > Hi Stefan,
> > 
> > shouldn't this be msec = 1000.
> > 
> > Here you set it to 1ms if it used to be over 1s.
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/3] net: fec: implement dtb property phy-reset-durationy

[Stefan Christ]

Hi,

> > Yeah, that would be the obvious and plausible thing to do, but the driver in
> > the linux kernel does it this way and it's documented in the device tree
> > bindings.  So I'm not sure if we can change the default behavior here.
> 
> Yes, we can ;)
> 
> The rationale behind this code may be: The reset duration is
> unreasonably high (>1s), so that it to a sane default (1ms).
> 
> I suggest to drop the check completely. If someone ever starts barebox
> with an insanely high value (maybe he thought the value is in us) then
> he won't notice the mistake if barebox silently falls back to 1ms as
> duration. If instead barebox waits for multiple seconds then he will
> notice and it the device tree which is what we want.

Ok. So what about printing a warn message that case? Then everybody should
notice.

When I send a v2 patch, should the description in the device tree binding be
updated, too?

Mit freundlichen Grüßen / Kind regards,
	Stefan Christ

On Tue, Nov 17, 2015 at 10:32:13AM +0100, Sascha Hauer wrote:
> On Mon, Nov 16, 2015 at 11:17:05AM +0100, Stefan Christ wrote:
> > Hi,
> >   
> > > shouldn't this be msec = 1000.
> > > 
> > > Here you set it to 1ms if it used to be over 1s.
> > 
> > Yeah, that would be the obvious and plausible thing to do, but the driver in
> > the linux kernel does it this way and it's documented in the device tree
> > bindings.  So I'm not sure if we can change the default behavior here.
> 
> Yes, we can ;)
> 
> The rationale behind this code may be: The reset duration is
> unreasonably high (>1s), so that it to a sane default (1ms).
> 
> I suggest to drop the check completely. If someone ever starts barebox
> with an insanely high value (maybe he thought the value is in us) then
> he won't notice the mistake if barebox silently falls back to 1ms as
> duration. If instead barebox waits for multiple seconds then he will
> notice and it the device tree which is what we want.
> 
> Sascha
> 
> 
> > 
> > Mit freundlichen Grüßen / Kind regards,
> > 	Stefan Christ
> > 
> > On Mon, Nov 16, 2015 at 11:03:32AM +0100, Holger Schurig wrote:
> > > 2015-11-16 10:00 GMT+01:00 Stefan Christ <s.christ@phytec.de>:
> > > 
> > > > +               /* A sane reset duration should not be longer than 1s */
> > > > +               if (msec > 1000)
> > > > +                       msec = 1;
> > > >
> > > 
> > > Hi Stefan,
> > > 
> > > shouldn't this be msec = 1000.
> > > 
> > > Here you set it to 1ms if it used to be over 1s.
> > 
> > _______________________________________________
> > barebox mailing list
> > barebox@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/barebox
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/3] net: fec: implement dtb property phy-reset-durationy

[Sascha Hauer]

On Tue, Nov 17, 2015 at 03:38:43PM +0100, Stefan Christ wrote:
> Hi,
> 
> > > Yeah, that would be the obvious and plausible thing to do, but the driver in
> > > the linux kernel does it this way and it's documented in the device tree
> > > bindings.  So I'm not sure if we can change the default behavior here.
> > 
> > Yes, we can ;)
> > 
> > The rationale behind this code may be: The reset duration is
> > unreasonably high (>1s), so that it to a sane default (1ms).
> > 
> > I suggest to drop the check completely. If someone ever starts barebox
> > with an insanely high value (maybe he thought the value is in us) then
> > he won't notice the mistake if barebox silently falls back to 1ms as
> > duration. If instead barebox waits for multiple seconds then he will
> > notice and it the device tree which is what we want.
> 
> Ok. So what about printing a warn message that case? Then everybody should
> notice.

No, I don't think that's necessary. Such an overlong delay should be
noticeable already and give the right hint. Let's save the binary space.

> 
> When I send a v2 patch, should the description in the device tree binding be
> updated, too?

No, the bindings docs are directly taken from the kernel and changes
will be lost the next I update them.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] net: fec: fix indentation and whitspaces

[Stefan Christ]

Signed-off-by: Stefan Christ <s.christ@phytec.de>
---
 drivers/net/fec_imx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 39a5cea..37e28d9 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -259,7 +259,7 @@ static int fec_set_hwaddr(struct eth_device *dev, const unsigned char *mac)
 	writel((mac[0] << 24) + (mac[1] << 16) + (mac[2] << 8) + mac[3], fec->regs + FEC_PADDR1);
 	writel((mac[4] << 24) + (mac[5] << 16) + 0x8808, fec->regs + FEC_PADDR2);
 
-        return 0;
+	return 0;
 }
 
 static int fec_init(struct eth_device *dev)
@@ -647,8 +647,8 @@ static int fec_probe_dt(struct device_d *dev, struct fec_priv *fec)
 #endif
 static int fec_probe(struct device_d *dev)
 {
-        struct fec_platform_data *pdata = (struct fec_platform_data *)dev->platform_data;
-        struct eth_device *edev;
+	struct fec_platform_data *pdata = (struct fec_platform_data *)dev->platform_data;
+	struct eth_device *edev;
 	struct fec_priv *fec;
 	void *base;
 	int ret;
@@ -781,7 +781,7 @@ static __maybe_unused struct of_device_id imx_fec_dt_ids[] = {
 	}, {
 		.compatible = "fsl,imx6q-fec",
 		.data = (void *)FEC_TYPE_IMX6,
-	},  {
+	}, {
 		.compatible = "fsl,imx6sx-fec",
 		.data = (void *)FEC_TYPE_IMX6,
 	}, {
-- 
1.9.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox