[PATCH 0/4] net/macb: fixes after big driver update

[PATCH 0/4] net/macb: fixes after big driver update

[Nicolas Ferre]

The recent modifications to the macb driver lead to issues with the probe
function code flow. Here are some attempt to fix them.
The series is written on top of net-next.

Nicolas Ferre (4):
  net/macb: only probe queues once and use stored values
  net/macb: add comment in macb_probe_queues
  net/macb: fix capabilities configuration
  net/macb: trivial: correct wording of for caps

 drivers/net/ethernet/cadence/macb.c | 55 +++++++++++++++++++------------------
 drivers/net/ethernet/cadence/macb.h |  1 +
 2 files changed, 30 insertions(+), 26 deletions(-)

-- 
2.1.3

[PATCH 0/4] net/macb: fixes after big driver update

[Nicolas Ferre]

The recent modifications to the macb driver lead to issues with the probe
function code flow. Here are some attempt to fix them.
The series is written on top of net-next.

Nicolas Ferre (4):
  net/macb: only probe queues once and use stored values
  net/macb: add comment in macb_probe_queues
  net/macb: fix capabilities configuration
  net/macb: trivial: correct wording of for caps

 drivers/net/ethernet/cadence/macb.c | 55 +++++++++++++++++++------------------
 drivers/net/ethernet/cadence/macb.h |  1 +
 2 files changed, 30 insertions(+), 26 deletions(-)

-- 
2.1.3

[PATCH 1/4] net/macb: only probe queues once and use stored values

[Nicolas Ferre]

When merging at91_ether and macb driver during 421d9df0628b (net/macb: merge
at91_ether driver into macb driver) the probe function has been split. The code
dealing with initialization of queues is now moved in macb_init() which needs
information computed in the parent macb_probe() function.
So, add the queue_mask information to the private structure and use it when
needed in macb_init().

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 | 9 ++++-----
 drivers/net/ethernet/cadence/macb.h | 1 +
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index a0a04b3638e6..b710768172d9 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2180,7 +2180,7 @@ static void macb_probe_queues(void __iomem *mem,
 static int macb_init(struct platform_device *pdev)
 {
 	struct net_device *dev = platform_get_drvdata(pdev);
-	unsigned int hw_q, queue_mask, q, num_queues;
+	unsigned int hw_q, q;
 	struct macb *bp = netdev_priv(dev);
 	struct macb_queue *queue;
 	int err;
@@ -2226,10 +2226,8 @@ static int macb_init(struct platform_device *pdev)
 	 * register mapping but we don't want to test the queue index then
 	 * compute the corresponding register offset at run time.
 	 */
-	macb_probe_queues(bp->regs, &queue_mask, &num_queues);
-
-	for (hw_q = 0, q = 0; hw_q < MACB_MAX_QUEUES; ++hw_q) {
-		if (!(queue_mask & (1 << hw_q)))
+	for (hw_q = 0, q = 0; hw_q < bp->num_queues; ++hw_q) {
+		if (!(bp->queue_mask & (1 << hw_q)))
 			continue;
 
 		queue = &bp->queues[q];
@@ -2715,6 +2713,7 @@ static int macb_probe(struct platform_device *pdev)
 	bp->dev = dev;
 	bp->regs = mem;
 	bp->num_queues = num_queues;
+	bp->queue_mask = queue_mask;
 	spin_lock_init(&bp->lock);
 
 	platform_set_drvdata(pdev, dev);
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index bc6e35c40822..0b6afac91bfe 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -785,6 +785,7 @@ struct macb {
 	size_t			rx_buffer_size;
 
 	unsigned int		num_queues;
+	unsigned int		queue_mask;
 	struct macb_queue	queues[MACB_MAX_QUEUES];
 
 	spinlock_t		lock;
-- 
2.1.3

[PATCH 1/4] net/macb: only probe queues once and use stored values

[Nicolas Ferre]

When merging at91_ether and macb driver during 421d9df0628b (net/macb: merge
at91_ether driver into macb driver) the probe function has been split. The code
dealing with initialization of queues is now moved in macb_init() which needs
information computed in the parent macb_probe() function.
So, add the queue_mask information to the private structure and use it when
needed in macb_init().

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 | 9 ++++-----
 drivers/net/ethernet/cadence/macb.h | 1 +
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index a0a04b3638e6..b710768172d9 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2180,7 +2180,7 @@ static void macb_probe_queues(void __iomem *mem,
 static int macb_init(struct platform_device *pdev)
 {
 	struct net_device *dev = platform_get_drvdata(pdev);
-	unsigned int hw_q, queue_mask, q, num_queues;
+	unsigned int hw_q, q;
 	struct macb *bp = netdev_priv(dev);
 	struct macb_queue *queue;
 	int err;
@@ -2226,10 +2226,8 @@ static int macb_init(struct platform_device *pdev)
 	 * register mapping but we don't want to test the queue index then
 	 * compute the corresponding register offset at run time.
 	 */
-	macb_probe_queues(bp->regs, &queue_mask, &num_queues);
-
-	for (hw_q = 0, q = 0; hw_q < MACB_MAX_QUEUES; ++hw_q) {
-		if (!(queue_mask & (1 << hw_q)))
+	for (hw_q = 0, q = 0; hw_q < bp->num_queues; ++hw_q) {
+		if (!(bp->queue_mask & (1 << hw_q)))
 			continue;
 
 		queue = &bp->queues[q];
@@ -2715,6 +2713,7 @@ static int macb_probe(struct platform_device *pdev)
 	bp->dev = dev;
 	bp->regs = mem;
 	bp->num_queues = num_queues;
+	bp->queue_mask = queue_mask;
 	spin_lock_init(&bp->lock);
 
 	platform_set_drvdata(pdev, dev);
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index bc6e35c40822..0b6afac91bfe 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -785,6 +785,7 @@ struct macb {
 	size_t			rx_buffer_size;
 
 	unsigned int		num_queues;
+	unsigned int		queue_mask;
 	struct macb_queue	queues[MACB_MAX_QUEUES];
 
 	spinlock_t		lock;
-- 
2.1.3

[PATCH 2/4] net/macb: add comment in macb_probe_queues

[Nicolas Ferre]

As we access the MID register directly, we need to tell why
we don't use the macb_is_gem() dedicated function.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index b710768172d9..bc3eab95022f 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2161,9 +2161,13 @@ static void macb_probe_queues(void __iomem *mem,
 	*queue_mask = 0x1;
 	*num_queues = 1;
 
-	/* is it macb or gem ? */
+	/* is it macb or gem ?
+	 *
+	 * We need to read directly from the hardware here because
+	 * we are early in the probe process and don't have the
+	 * MACB_CAPS_MACB_IS_GEM flag positioned
+	 */
 	mid = readl_relaxed(mem + MACB_MID);
-
 	if (MACB_BFEXT(IDNUM, mid) < 0x2)
 		return;
 
-- 
2.1.3

[PATCH 2/4] net/macb: add comment in macb_probe_queues

[Nicolas Ferre]

As we access the MID register directly, we need to tell why
we don't use the macb_is_gem() dedicated function.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index b710768172d9..bc3eab95022f 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2161,9 +2161,13 @@ static void macb_probe_queues(void __iomem *mem,
 	*queue_mask = 0x1;
 	*num_queues = 1;
 
-	/* is it macb or gem ? */
+	/* is it macb or gem ?
+	 *
+	 * We need to read directly from the hardware here because
+	 * we are early in the probe process and don't have the
+	 * MACB_CAPS_MACB_IS_GEM flag positioned
+	 */
 	mid = readl_relaxed(mem + MACB_MID);
-
 	if (MACB_BFEXT(IDNUM, mid) < 0x2)
 		return;
 
-- 
2.1.3

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

[Nicolas Ferre]

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(-)

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)
-- 
2.1.3

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

[Nicolas Ferre]

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(-)

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)
-- 
2.1.3

[PATCH 4/4] net/macb: trivial: correct wording of for caps

[Nicolas Ferre]

As a non-native English speaker, I would correct "capacities" of the macb
peripheral to "capabilities": correct me if I'm wrong!

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 64e35a50e5c1..3032ae052755 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2129,7 +2129,7 @@ static const struct net_device_ops macb_netdev_ops = {
 };
 
 /*
- * Configure peripheral capacities according to device tree
+ * Configure peripheral capabilities according to device tree
  * and integration options used
  */
 static void macb_configure_caps(struct macb *bp, const struct macb_config *dt_conf)
@@ -2731,7 +2731,7 @@ static int macb_probe(struct platform_device *pdev)
 		}
 	}
 
-	/* setup capacities */
+	/* setup capabilities */
 	macb_configure_caps(bp, macb_config);
 
 	platform_set_drvdata(pdev, dev);
-- 
2.1.3

[PATCH 4/4] net/macb: trivial: correct wording of for caps

[Nicolas Ferre]

As a non-native English speaker, I would correct "capacities" of the macb
peripheral to "capabilities": correct me if I'm wrong!

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 64e35a50e5c1..3032ae052755 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2129,7 +2129,7 @@ static const struct net_device_ops macb_netdev_ops = {
 };
 
 /*
- * Configure peripheral capacities according to device tree
+ * Configure peripheral capabilities according to device tree
  * and integration options used
  */
 static void macb_configure_caps(struct macb *bp, const struct macb_config *dt_conf)
@@ -2731,7 +2731,7 @@ static int macb_probe(struct platform_device *pdev)
 		}
 	}
 
-	/* setup capacities */
+	/* setup capabilities */
 	macb_configure_caps(bp, macb_config);
 
 	platform_set_drvdata(pdev, dev);
-- 
2.1.3

Re: [PATCH 1/4] net/macb: only probe queues once and use stored values

[Cyrille Pitchen]

Hi Nicolas,

There is a little mistake in this patch, which will prevent the GEM from 
working properly:

Le 27/03/2015 16:34, Nicolas Ferre a écrit :
> When merging at91_ether and macb driver during 421d9df0628b (net/macb: merge
> at91_ether driver into macb driver) the probe function has been split. The code
> dealing with initialization of queues is now moved in macb_init() which needs
> information computed in the parent macb_probe() function.
> So, add the queue_mask information to the private structure and use it when
> needed in macb_init().
> 
> 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 | 9 ++++-----
>  drivers/net/ethernet/cadence/macb.h | 1 +
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index a0a04b3638e6..b710768172d9 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2180,7 +2180,7 @@ static void macb_probe_queues(void __iomem *mem,
>  static int macb_init(struct platform_device *pdev)
>  {
>  	struct net_device *dev = platform_get_drvdata(pdev);
> -	unsigned int hw_q, queue_mask, q, num_queues;
> +	unsigned int hw_q, q;
>  	struct macb *bp = netdev_priv(dev);
>  	struct macb_queue *queue;
>  	int err;
> @@ -2226,10 +2226,8 @@ static int macb_init(struct platform_device *pdev)
>  	 * register mapping but we don't want to test the queue index then
>  	 * compute the corresponding register offset at run time.
>  	 */
> -	macb_probe_queues(bp->regs, &queue_mask, &num_queues);
> -
> -	for (hw_q = 0, q = 0; hw_q < MACB_MAX_QUEUES; ++hw_q) {
> -		if (!(queue_mask & (1 << hw_q)))
> +	for (hw_q = 0, q = 0; hw_q < bp->num_queues; ++hw_q) {
> +		if (!(bp->queue_mask & (1 << hw_q)))
>  			continue;

The exit condition of the for loop must remain "hw_q < MACB_MAX_QUEUES" and 
should not be replaced by "hw_q < bp->num_queues".
Indeed there can be only 2 queues, for instance queue0 and queue7. So if you
change the exit conditon of the loop, queue7 won't be initialized and HRESP 
errors will occur.
>  
>  		queue = &bp->queues[q];
> @@ -2715,6 +2713,7 @@ static int macb_probe(struct platform_device *pdev)
>  	bp->dev = dev;
>  	bp->regs = mem;
>  	bp->num_queues = num_queues;
> +	bp->queue_mask = queue_mask;
>  	spin_lock_init(&bp->lock);
>  
>  	platform_set_drvdata(pdev, dev);
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index bc6e35c40822..0b6afac91bfe 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -785,6 +785,7 @@ struct macb {
>  	size_t			rx_buffer_size;
>  
>  	unsigned int		num_queues;
> +	unsigned int		queue_mask;
>  	struct macb_queue	queues[MACB_MAX_QUEUES];
>  
>  	spinlock_t		lock;
> 

Best Regards,

Cyrille

[PATCH 1/4] net/macb: only probe queues once and use stored values

[Cyrille Pitchen]

Hi Nicolas,

There is a little mistake in this patch, which will prevent the GEM from 
working properly:

Le 27/03/2015 16:34, Nicolas Ferre a ?crit :
> When merging at91_ether and macb driver during 421d9df0628b (net/macb: merge
> at91_ether driver into macb driver) the probe function has been split. The code
> dealing with initialization of queues is now moved in macb_init() which needs
> information computed in the parent macb_probe() function.
> So, add the queue_mask information to the private structure and use it when
> needed in macb_init().
> 
> 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 | 9 ++++-----
>  drivers/net/ethernet/cadence/macb.h | 1 +
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index a0a04b3638e6..b710768172d9 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2180,7 +2180,7 @@ static void macb_probe_queues(void __iomem *mem,
>  static int macb_init(struct platform_device *pdev)
>  {
>  	struct net_device *dev = platform_get_drvdata(pdev);
> -	unsigned int hw_q, queue_mask, q, num_queues;
> +	unsigned int hw_q, q;
>  	struct macb *bp = netdev_priv(dev);
>  	struct macb_queue *queue;
>  	int err;
> @@ -2226,10 +2226,8 @@ static int macb_init(struct platform_device *pdev)
>  	 * register mapping but we don't want to test the queue index then
>  	 * compute the corresponding register offset at run time.
>  	 */
> -	macb_probe_queues(bp->regs, &queue_mask, &num_queues);
> -
> -	for (hw_q = 0, q = 0; hw_q < MACB_MAX_QUEUES; ++hw_q) {
> -		if (!(queue_mask & (1 << hw_q)))
> +	for (hw_q = 0, q = 0; hw_q < bp->num_queues; ++hw_q) {
> +		if (!(bp->queue_mask & (1 << hw_q)))
>  			continue;

The exit condition of the for loop must remain "hw_q < MACB_MAX_QUEUES" and 
should not be replaced by "hw_q < bp->num_queues".
Indeed there can be only 2 queues, for instance queue0 and queue7. So if you
change the exit conditon of the loop, queue7 won't be initialized and HRESP 
errors will occur.
>  
>  		queue = &bp->queues[q];
> @@ -2715,6 +2713,7 @@ static int macb_probe(struct platform_device *pdev)
>  	bp->dev = dev;
>  	bp->regs = mem;
>  	bp->num_queues = num_queues;
> +	bp->queue_mask = queue_mask;
>  	spin_lock_init(&bp->lock);
>  
>  	platform_set_drvdata(pdev, dev);
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index bc6e35c40822..0b6afac91bfe 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -785,6 +785,7 @@ struct macb {
>  	size_t			rx_buffer_size;
>  
>  	unsigned int		num_queues;
> +	unsigned int		queue_mask;
>  	struct macb_queue	queues[MACB_MAX_QUEUES];
>  
>  	spinlock_t		lock;
> 

Best Regards,

Cyrille

Re: [PATCH 1/4] net/macb: only probe queues once and use stored values

[Boris Brezillon]

Hi Nicolas,

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

> When merging at91_ether and macb driver during 421d9df0628b (net/macb: merge
> at91_ether driver into macb driver) the probe function has been split. The code
> dealing with initialization of queues is now moved in macb_init() which needs
> information computed in the parent macb_probe() function.
> So, add the queue_mask information to the private structure and use it when
> needed in macb_init().

Just to be sure: this is not a bug fix, right ?
It just improves the initialization process by calling
macb_probe_queues only once (in macb_probe).

Anyway, once you've addressed Cyrille's comment, you can add my

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> 
> 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 | 9 ++++-----
>  drivers/net/ethernet/cadence/macb.h | 1 +
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index a0a04b3638e6..b710768172d9 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2180,7 +2180,7 @@ static void macb_probe_queues(void __iomem *mem,
>  static int macb_init(struct platform_device *pdev)
>  {
>  	struct net_device *dev = platform_get_drvdata(pdev);
> -	unsigned int hw_q, queue_mask, q, num_queues;
> +	unsigned int hw_q, q;
>  	struct macb *bp = netdev_priv(dev);
>  	struct macb_queue *queue;
>  	int err;
> @@ -2226,10 +2226,8 @@ static int macb_init(struct platform_device *pdev)
>  	 * register mapping but we don't want to test the queue index then
>  	 * compute the corresponding register offset at run time.
>  	 */
> -	macb_probe_queues(bp->regs, &queue_mask, &num_queues);
> -
> -	for (hw_q = 0, q = 0; hw_q < MACB_MAX_QUEUES; ++hw_q) {
> -		if (!(queue_mask & (1 << hw_q)))
> +	for (hw_q = 0, q = 0; hw_q < bp->num_queues; ++hw_q) {
> +		if (!(bp->queue_mask & (1 << hw_q)))
>  			continue;
>  
>  		queue = &bp->queues[q];
> @@ -2715,6 +2713,7 @@ static int macb_probe(struct platform_device *pdev)
>  	bp->dev = dev;
>  	bp->regs = mem;
>  	bp->num_queues = num_queues;
> +	bp->queue_mask = queue_mask;
>  	spin_lock_init(&bp->lock);
>  
>  	platform_set_drvdata(pdev, dev);
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index bc6e35c40822..0b6afac91bfe 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -785,6 +785,7 @@ struct macb {
>  	size_t			rx_buffer_size;
>  
>  	unsigned int		num_queues;
> +	unsigned int		queue_mask;
>  	struct macb_queue	queues[MACB_MAX_QUEUES];
>  
>  	spinlock_t		lock;



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 1/4] net/macb: only probe queues once and use stored values

[Boris Brezillon]

Hi Nicolas,

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

> When merging at91_ether and macb driver during 421d9df0628b (net/macb: merge
> at91_ether driver into macb driver) the probe function has been split. The code
> dealing with initialization of queues is now moved in macb_init() which needs
> information computed in the parent macb_probe() function.
> So, add the queue_mask information to the private structure and use it when
> needed in macb_init().

Just to be sure: this is not a bug fix, right ?
It just improves the initialization process by calling
macb_probe_queues only once (in macb_probe).

Anyway, once you've addressed Cyrille's comment, you can add my

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> 
> 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 | 9 ++++-----
>  drivers/net/ethernet/cadence/macb.h | 1 +
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index a0a04b3638e6..b710768172d9 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2180,7 +2180,7 @@ static void macb_probe_queues(void __iomem *mem,
>  static int macb_init(struct platform_device *pdev)
>  {
>  	struct net_device *dev = platform_get_drvdata(pdev);
> -	unsigned int hw_q, queue_mask, q, num_queues;
> +	unsigned int hw_q, q;
>  	struct macb *bp = netdev_priv(dev);
>  	struct macb_queue *queue;
>  	int err;
> @@ -2226,10 +2226,8 @@ static int macb_init(struct platform_device *pdev)
>  	 * register mapping but we don't want to test the queue index then
>  	 * compute the corresponding register offset at run time.
>  	 */
> -	macb_probe_queues(bp->regs, &queue_mask, &num_queues);
> -
> -	for (hw_q = 0, q = 0; hw_q < MACB_MAX_QUEUES; ++hw_q) {
> -		if (!(queue_mask & (1 << hw_q)))
> +	for (hw_q = 0, q = 0; hw_q < bp->num_queues; ++hw_q) {
> +		if (!(bp->queue_mask & (1 << hw_q)))
>  			continue;
>  
>  		queue = &bp->queues[q];
> @@ -2715,6 +2713,7 @@ static int macb_probe(struct platform_device *pdev)
>  	bp->dev = dev;
>  	bp->regs = mem;
>  	bp->num_queues = num_queues;
> +	bp->queue_mask = queue_mask;
>  	spin_lock_init(&bp->lock);
>  
>  	platform_set_drvdata(pdev, dev);
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index bc6e35c40822..0b6afac91bfe 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -785,6 +785,7 @@ struct macb {
>  	size_t			rx_buffer_size;
>  
>  	unsigned int		num_queues;
> +	unsigned int		queue_mask;
>  	struct macb_queue	queues[MACB_MAX_QUEUES];
>  
>  	spinlock_t		lock;



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Boris Brezillon]

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

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Boris Brezillon]

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

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/4] net/macb: only probe queues once and use stored values

[Nicolas Ferre]

Le 27/03/2015 17:24, Cyrille Pitchen a écrit :
> Hi Nicolas,
> 
> There is a little mistake in this patch, which will prevent the GEM from 
> working properly:
> 
> Le 27/03/2015 16:34, Nicolas Ferre a écrit :
>> When merging at91_ether and macb driver during 421d9df0628b (net/macb: merge
>> at91_ether driver into macb driver) the probe function has been split. The code
>> dealing with initialization of queues is now moved in macb_init() which needs
>> information computed in the parent macb_probe() function.
>> So, add the queue_mask information to the private structure and use it when
>> needed in macb_init().
>>
>> 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 | 9 ++++-----
>>  drivers/net/ethernet/cadence/macb.h | 1 +
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>> index a0a04b3638e6..b710768172d9 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -2180,7 +2180,7 @@ static void macb_probe_queues(void __iomem *mem,
>>  static int macb_init(struct platform_device *pdev)
>>  {
>>  	struct net_device *dev = platform_get_drvdata(pdev);
>> -	unsigned int hw_q, queue_mask, q, num_queues;
>> +	unsigned int hw_q, q;
>>  	struct macb *bp = netdev_priv(dev);
>>  	struct macb_queue *queue;
>>  	int err;
>> @@ -2226,10 +2226,8 @@ static int macb_init(struct platform_device *pdev)
>>  	 * register mapping but we don't want to test the queue index then
>>  	 * compute the corresponding register offset at run time.
>>  	 */
>> -	macb_probe_queues(bp->regs, &queue_mask, &num_queues);
>> -
>> -	for (hw_q = 0, q = 0; hw_q < MACB_MAX_QUEUES; ++hw_q) {
>> -		if (!(queue_mask & (1 << hw_q)))
>> +	for (hw_q = 0, q = 0; hw_q < bp->num_queues; ++hw_q) {
>> +		if (!(bp->queue_mask & (1 << hw_q)))
>>  			continue;
> 
> The exit condition of the for loop must remain "hw_q < MACB_MAX_QUEUES" and 
> should not be replaced by "hw_q < bp->num_queues".
> Indeed there can be only 2 queues, for instance queue0 and queue7. So if you
> change the exit conditon of the loop, queue7 won't be initialized and HRESP 
> errors will occur.

Absolutely. Sorry to have misunderstood this condition.

I'll re-spin a v2 series.

Bye,

>>  
>>  		queue = &bp->queues[q];
>> @@ -2715,6 +2713,7 @@ static int macb_probe(struct platform_device *pdev)
>>  	bp->dev = dev;
>>  	bp->regs = mem;
>>  	bp->num_queues = num_queues;
>> +	bp->queue_mask = queue_mask;
>>  	spin_lock_init(&bp->lock);
>>  
>>  	platform_set_drvdata(pdev, dev);
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index bc6e35c40822..0b6afac91bfe 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -785,6 +785,7 @@ struct macb {
>>  	size_t			rx_buffer_size;
>>  
>>  	unsigned int		num_queues;
>> +	unsigned int		queue_mask;
>>  	struct macb_queue	queues[MACB_MAX_QUEUES];
>>  
>>  	spinlock_t		lock;
>>
> 
> Best Regards,
> 
> Cyrille
> 


-- 
Nicolas Ferre

[PATCH 1/4] net/macb: only probe queues once and use stored values

[Nicolas Ferre]

Le 27/03/2015 17:24, Cyrille Pitchen a ?crit :
> Hi Nicolas,
> 
> There is a little mistake in this patch, which will prevent the GEM from 
> working properly:
> 
> Le 27/03/2015 16:34, Nicolas Ferre a ?crit :
>> When merging at91_ether and macb driver during 421d9df0628b (net/macb: merge
>> at91_ether driver into macb driver) the probe function has been split. The code
>> dealing with initialization of queues is now moved in macb_init() which needs
>> information computed in the parent macb_probe() function.
>> So, add the queue_mask information to the private structure and use it when
>> needed in macb_init().
>>
>> 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 | 9 ++++-----
>>  drivers/net/ethernet/cadence/macb.h | 1 +
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>> index a0a04b3638e6..b710768172d9 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -2180,7 +2180,7 @@ static void macb_probe_queues(void __iomem *mem,
>>  static int macb_init(struct platform_device *pdev)
>>  {
>>  	struct net_device *dev = platform_get_drvdata(pdev);
>> -	unsigned int hw_q, queue_mask, q, num_queues;
>> +	unsigned int hw_q, q;
>>  	struct macb *bp = netdev_priv(dev);
>>  	struct macb_queue *queue;
>>  	int err;
>> @@ -2226,10 +2226,8 @@ static int macb_init(struct platform_device *pdev)
>>  	 * register mapping but we don't want to test the queue index then
>>  	 * compute the corresponding register offset at run time.
>>  	 */
>> -	macb_probe_queues(bp->regs, &queue_mask, &num_queues);
>> -
>> -	for (hw_q = 0, q = 0; hw_q < MACB_MAX_QUEUES; ++hw_q) {
>> -		if (!(queue_mask & (1 << hw_q)))
>> +	for (hw_q = 0, q = 0; hw_q < bp->num_queues; ++hw_q) {
>> +		if (!(bp->queue_mask & (1 << hw_q)))
>>  			continue;
> 
> The exit condition of the for loop must remain "hw_q < MACB_MAX_QUEUES" and 
> should not be replaced by "hw_q < bp->num_queues".
> Indeed there can be only 2 queues, for instance queue0 and queue7. So if you
> change the exit conditon of the loop, queue7 won't be initialized and HRESP 
> errors will occur.

Absolutely. Sorry to have misunderstood this condition.

I'll re-spin a v2 series.

Bye,

>>  
>>  		queue = &bp->queues[q];
>> @@ -2715,6 +2713,7 @@ static int macb_probe(struct platform_device *pdev)
>>  	bp->dev = dev;
>>  	bp->regs = mem;
>>  	bp->num_queues = num_queues;
>> +	bp->queue_mask = queue_mask;
>>  	spin_lock_init(&bp->lock);
>>  
>>  	platform_set_drvdata(pdev, dev);
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index bc6e35c40822..0b6afac91bfe 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -785,6 +785,7 @@ struct macb {
>>  	size_t			rx_buffer_size;
>>  
>>  	unsigned int		num_queues;
>> +	unsigned int		queue_mask;
>>  	struct macb_queue	queues[MACB_MAX_QUEUES];
>>  
>>  	spinlock_t		lock;
>>
> 
> Best Regards,
> 
> Cyrille
> 


-- 
Nicolas Ferre

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

[Nicolas Ferre]

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
> 


-- 
Nicolas Ferre

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

[Nicolas Ferre]

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
> 


-- 
Nicolas Ferre