diff mbox

[3/4] net/macb: fix capabilities configuration

Message ID bd7194896b4e4517a7675ae02fe7bed01f19a02d.1427469791.git.nicolas.ferre@atmel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nicolas Ferre March 27, 2015, 3:34 p.m. UTC
Capabilities configuration by macb_configure_caps() was moved far too late by
421d9df0628b (net/macb: merge at91_ether driver into macb driver) which would
lead to badly configured hardware.
So, move this function to early probe and modify its prototype to re-gain its
original behavior.
DT data retrieval is also moved to simplify the probe code flow.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/net/ethernet/cadence/macb.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

Comments

Boris BREZILLON March 27, 2015, 11:02 p.m. UTC | #1
Hi Nicolas,

On Fri, 27 Mar 2015 16:34:11 +0100
Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

> Capabilities configuration by macb_configure_caps() was moved far too late by
> 421d9df0628b (net/macb: merge at91_ether driver into macb driver) which would
> lead to badly configured hardware.

Indeed, the macb_configure_caps function is called a bit too late,
but ...

> So, move this function to early probe and modify its prototype to re-gain its
> original behavior.
> DT data retrieval is also moved to simplify the probe code flow.

... I'm not happy with these changes.
I tried to keep  specific init steps of macb and at91_ether separated
and you're moving macb_configure_caps call (not required on at91_ether
HW) into macb_probe (the common probe part).

How about moving macb_configure_caps a bit earlier in the macb_init
function [1] ?

Best Regards,

Boris

[1]http://code.bulix.org/8gyi6b-88141
Nicolas Ferre March 30, 2015, 6:33 a.m. UTC | #2
Le 28/03/2015 00:02, Boris Brezillon a écrit :
> Hi Nicolas,
> 
> On Fri, 27 Mar 2015 16:34:11 +0100
> Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> 
>> Capabilities configuration by macb_configure_caps() was moved far too late by
>> 421d9df0628b (net/macb: merge at91_ether driver into macb driver) which would
>> lead to badly configured hardware.
> 
> Indeed, the macb_configure_caps function is called a bit too late,
> but ...
> 
>> So, move this function to early probe and modify its prototype to re-gain its
>> original behavior.
>> DT data retrieval is also moved to simplify the probe code flow.
> 
> ... I'm not happy with these changes.
> I tried to keep  specific init steps of macb and at91_ether separated
> and you're moving macb_configure_caps call (not required on at91_ether
> HW) into macb_probe (the common probe part).

Well, this function is about configuring the capabilities of the
hardware both from the configuration registers and the device tree
entries (this last source applies to all flavors of hardware).

I only see advantages to set these flags early (Cf. above).

> How about moving macb_configure_caps a bit earlier in the macb_init
> function [1] ?

No, it won't be sufficient. The very first function needing the
capabilities set is macb_get_hwaddr() which is pretty early in macb_probe().


Bye,

> Best Regards,
> 
> Boris
> 
> [1]http://code.bulix.org/8gyi6b-88141
>
diff mbox

Patch

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index bc3eab95022f..64e35a50e5c1 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2132,10 +2132,13 @@  static const struct net_device_ops macb_netdev_ops = {
  * Configure peripheral capacities according to device tree
  * and integration options used
  */
-static void macb_configure_caps(struct macb *bp)
+static void macb_configure_caps(struct macb *bp, const struct macb_config *dt_conf)
 {
 	u32 dcfg;
 
+	if (dt_conf)
+		bp->caps = dt_conf->caps;
+
 	if (MACB_BFEXT(IDNUM, macb_readl(bp, MID)) == 0x2)
 		bp->caps |= MACB_CAPS_MACB_IS_GEM;
 
@@ -2313,9 +2316,6 @@  static int macb_init(struct platform_device *pdev)
 
 	macb_or_gem_writel(bp, USRIO, val);
 
-	/* setup capacities */
-	macb_configure_caps(bp);
-
 	/* Set MII management clock divider */
 	val = macb_mdc_clk_div(bp);
 	val |= macb_dbw(bp);
@@ -2720,6 +2720,20 @@  static int macb_probe(struct platform_device *pdev)
 	bp->queue_mask = queue_mask;
 	spin_lock_init(&bp->lock);
 
+	if (np) {
+		const struct of_device_id *match;
+
+		match = of_match_node(macb_dt_ids, np);
+		if (match && match->data) {
+			macb_config = match->data;
+			bp->dma_burst_length = macb_config->dma_burst_length;
+			init = macb_config->init;
+		}
+	}
+
+	/* setup capacities */
+	macb_configure_caps(bp, macb_config);
+
 	platform_set_drvdata(pdev, dev);
 
 	dev->irq = platform_get_irq(pdev, 0);
@@ -2743,20 +2757,6 @@  static int macb_probe(struct platform_device *pdev)
 		bp->phy_interface = err;
 	}
 
-	if (np) {
-		const struct of_device_id *match;
-
-		match = of_match_node(macb_dt_ids, np);
-		if (match)
-			macb_config = match->data;
-	}
-
-	if (macb_config) {
-		bp->caps = macb_config->caps;
-		bp->dma_burst_length = macb_config->dma_burst_length;
-		init = macb_config->init;
-	}
-
 	/* IP specific init */
 	err = init(pdev);
 	if (err)