[PATCH 1/3] ibm_newemac: Allow the "no flow control" EMAC feature to work

[PATCH 1/3] ibm_newemac: Allow the "no flow control" EMAC feature to work

[Josh Boyer]

Some PowerPC 40x chips have errata that force us not to use the integrated
flow control.  We have the feature defined, but it currently can't be used
because it is never added to EMAC_FTRS_POSSIBLE.

This adds a Kconfig option for affected platforms to select and puts the
feature in the EMAC_FTRS_POSSIBLE list.  This is set for PowerPC 405EZ
platforms as well.

Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
---
 drivers/net/ibm_newemac/Kconfig |    4 ++++
 drivers/net/ibm_newemac/core.c  |    2 ++
 drivers/net/ibm_newemac/core.h  |    3 +++
 3 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ibm_newemac/Kconfig b/drivers/net/ibm_newemac/Kconfig
index 70a3272..dfb6547 100644
--- a/drivers/net/ibm_newemac/Kconfig
+++ b/drivers/net/ibm_newemac/Kconfig
@@ -62,3 +62,7 @@ config IBM_NEW_EMAC_TAH
 config IBM_NEW_EMAC_EMAC4
 	bool
 	default n
+
+config IBM_NEW_EMAC_NO_FLOW_CTRL
+	bool
+	default n
diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
index 2e720f2..2442361 100644
--- a/drivers/net/ibm_newemac/core.c
+++ b/drivers/net/ibm_newemac/core.c
@@ -2567,6 +2567,8 @@ static int __devinit emac_init_config(struct emac_instance *dev)
 		if (of_device_is_compatible(np, "ibm,emac-440ep") ||
 		    of_device_is_compatible(np, "ibm,emac-440gr"))
 			dev->features |= EMAC_FTR_440EP_PHY_CLK_FIX;
+		if (of_device_is_compatible(np, "ibm,emac-405ez"))
+			dev->features |= EMAC_FTR_NO_FLOW_CONTROL_40x;
 	}
 
 	/* Fixup some feature bits based on the device tree */
diff --git a/drivers/net/ibm_newemac/core.h b/drivers/net/ibm_newemac/core.h
index 6545e69..59e5a5d 100644
--- a/drivers/net/ibm_newemac/core.h
+++ b/drivers/net/ibm_newemac/core.h
@@ -341,6 +341,9 @@ enum {
 #ifdef CONFIG_IBM_NEW_EMAC_RGMII
 	    EMAC_FTR_HAS_RGMII	|
 #endif
+#ifdef CONFIG_IBM_NEW_EMAC_NO_FLOW_CTRL
+	    EMAC_FTR_NO_FLOW_CONTROL_40x |
+#endif
 	EMAC_FTR_440EP_PHY_CLK_FIX,
 };
 
-- 
1.5.5.1

[PATCH 2/3] ibm_newemac: Introduce mal_has_feature

[Josh Boyer]

There are some PowerPC SoCs that do odd things with the MAL handling.  In
order to accommodate them, we need to introduce a feature mechanism that is
similar to the existing emac_has_feature function.

This adds a feature variable to the mal_instance structure, and adds a
mal_has_feature function with some feature definitions.  These are guarded
by Kconfig options that are selected by the affected platforms.

Signed-of-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
---
 drivers/net/ibm_newemac/Kconfig |    8 ++++++++
 drivers/net/ibm_newemac/mal.h   |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ibm_newemac/Kconfig b/drivers/net/ibm_newemac/Kconfig
index dfb6547..44e5a0e 100644
--- a/drivers/net/ibm_newemac/Kconfig
+++ b/drivers/net/ibm_newemac/Kconfig
@@ -66,3 +66,11 @@ config IBM_NEW_EMAC_EMAC4
 config IBM_NEW_EMAC_NO_FLOW_CTRL
 	bool
 	default n
+
+config IBM_NEW_EMAC_MAL_CLR_ICINTSTAT
+	bool
+	default n
+
+config IBM_NEW_EMAC_MAL_COMMON_ERR
+	bool
+	default n
diff --git a/drivers/net/ibm_newemac/mal.h b/drivers/net/ibm_newemac/mal.h
index eaa7262..50c5c65 100644
--- a/drivers/net/ibm_newemac/mal.h
+++ b/drivers/net/ibm_newemac/mal.h
@@ -213,6 +213,8 @@ struct mal_instance {
 	struct of_device	*ofdev;
 	int			index;
 	spinlock_t		lock;
+
+	unsigned int features;
 };
 
 static inline u32 get_mal_dcrn(struct mal_instance *mal, int reg)
@@ -225,6 +227,41 @@ static inline void set_mal_dcrn(struct mal_instance *mal, int reg, u32 val)
 	dcr_write(mal->dcr_host, reg, val);
 }
 
+/* Features of various MAL implementations */
+
+/* Dummy feature bit so the enum works properly */
+#define MAL_FTR_DUMMY	0x00000001
+
+/* Set if you have interrupt coalescing and you have to clear the SDR
+ * register for TXEOB and RXEOB interrupts to work
+ */
+#define MAL_FTR_CLEAR_ICINTSTAT	0x00000002
+
+/* Set if your MAL has SERR, TXDE, and RXDE OR'd into a single UIC
+ * interrupt
+ */
+#define MAL_FTR_COMMON_ERR_INT	0x00000004
+
+enum {
+	MAL_FTRS_ALWAYS = 0,
+
+	MAL_FTRS_POSSIBLE =
+#ifdef CONFIG_IBM_NEW_EMAC_MAL_CLR_ICINTSTAT
+		MAL_FTR_CLEAR_ICINTSTAT |
+#endif
+#ifdef CONFIG_IBM_NEW_EMAC_MAL_COMMON_ERR
+		MAL_FTR_COMMON_ERR_INT |
+#endif
+		MAL_FTR_DUMMY,
+};
+
+static inline int mal_has_feature(struct mal_instance *dev,
+		unsigned long feature)
+{
+	return (MAL_FTRS_ALWAYS & feature) ||
+		(MAL_FTRS_POSSIBLE & dev->features & feature);
+}
+
 /* Register MAL devices */
 int mal_init(void);
 void mal_exit(void);
-- 
1.5.5.1

[PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ

[Josh Boyer]

The PowerPC 405EZ SoC has some differences in the interrupt layout and
handling for the MAL.  The SERR, TXDE, and RXDE interrupts are OR'd into
a single interrupt.  Also, due to the possibility for interrupt coalescing,
the TXEOB and RXEOB interrupts require an interrupt bit to be cleared in
the ICINTSTAT SDR.

This sets the proper MAL feature bits for 405EZ boards, and adds a common
shared handler for SERR, TXDE, and RXDE.  This has been adapted from code
originally written by Stefan Roese.

Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
---
 drivers/net/ibm_newemac/mal.c |   98 ++++++++++++++++++++++++++++++++--------
 1 files changed, 78 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ibm_newemac/mal.c b/drivers/net/ibm_newemac/mal.c
index 10c267b..3cef534 100644
--- a/drivers/net/ibm_newemac/mal.c
+++ b/drivers/net/ibm_newemac/mal.c
@@ -28,6 +28,7 @@
 #include <linux/delay.h>
 
 #include "core.h"
+#include <asm/dcr-regs.h>
 
 static int mal_count;
 
@@ -279,6 +280,9 @@ static irqreturn_t mal_txeob(int irq, void *dev_instance)
 	mal_schedule_poll(mal);
 	set_mal_dcrn(mal, MAL_TXEOBISR, r);
 
+	if (mal_has_feature(mal, MAL_FTR_CLEAR_ICINTSTAT))
+		mtdcri(SDR0, 0x4510, (mfdcri(SDR0, 0x4510) | 0x60000000));
+
 	return IRQ_HANDLED;
 }
 
@@ -293,6 +297,9 @@ static irqreturn_t mal_rxeob(int irq, void *dev_instance)
 	mal_schedule_poll(mal);
 	set_mal_dcrn(mal, MAL_RXEOBISR, r);
 
+	if (mal_has_feature(mal, MAL_FTR_CLEAR_ICINTSTAT))
+		mtdcri(SDR0, 0x4510, (mfdcri(SDR0, 0x4510) | 0x80000000));
+
 	return IRQ_HANDLED;
 }
 
@@ -336,6 +343,25 @@ static irqreturn_t mal_rxde(int irq, void *dev_instance)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t mal_int(int irq, void *dev_instance)
+{
+	struct mal_instance *mal = dev_instance;
+	u32 esr = get_mal_dcrn(mal, MAL_ESR);
+
+	if (esr & MAL_ESR_EVB) {
+		/* descriptor error */
+		if (esr & MAL_ESR_DE) {
+			if (esr & MAL_ESR_CIDT)
+				return (mal_rxde(irq, dev_instance));
+			else
+				return (mal_txde(irq, dev_instance));
+		} else { /* SERR */
+			return (mal_serr(irq, dev_instance));
+		}
+	}
+	return IRQ_HANDLED;
+}
+
 void mal_poll_disable(struct mal_instance *mal, struct mal_commac *commac)
 {
 	/* Spinlock-type semantics: only one caller disable poll at a time */
@@ -542,11 +568,22 @@ static int __devinit mal_probe(struct of_device *ofdev,
 		goto fail;
 	}
 
-	mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
-	mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
-	mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
-	mal->txde_irq = irq_of_parse_and_map(ofdev->node, 3);
-	mal->rxde_irq = irq_of_parse_and_map(ofdev->node, 4);
+	if (of_device_is_compatible(ofdev->node, "ibm,mcmal-405ez"))
+		mal->features |= (MAL_FTR_CLEAR_ICINTSTAT | MAL_FTR_COMMON_ERR_INT);
+
+	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
+		mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
+		mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
+		mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
+		mal->txde_irq = mal->rxde_irq = mal->serr_irq;
+	} else {
+		mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
+		mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
+		mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
+		mal->txde_irq = irq_of_parse_and_map(ofdev->node, 3);
+		mal->rxde_irq = irq_of_parse_and_map(ofdev->node, 4);
+	}
+
 	if (mal->txeob_irq == NO_IRQ || mal->rxeob_irq == NO_IRQ ||
 	    mal->serr_irq == NO_IRQ || mal->txde_irq == NO_IRQ ||
 	    mal->rxde_irq == NO_IRQ) {
@@ -608,21 +645,42 @@ static int __devinit mal_probe(struct of_device *ofdev,
 			     sizeof(struct mal_descriptor) *
 			     mal_rx_bd_offset(mal, i));
 
-	err = request_irq(mal->serr_irq, mal_serr, 0, "MAL SERR", mal);
-	if (err)
-		goto fail2;
-	err = request_irq(mal->txde_irq, mal_txde, 0, "MAL TX DE", mal);
-	if (err)
-		goto fail3;
-	err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
-	if (err)
-		goto fail4;
-	err = request_irq(mal->rxde_irq, mal_rxde, 0, "MAL RX DE", mal);
-	if (err)
-		goto fail5;
-	err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
-	if (err)
-		goto fail6;
+	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
+		err = request_irq(mal->serr_irq, mal_int, IRQF_SHARED,
+				"MAL SERR", mal);
+		if (err)
+			goto fail2;
+		err = request_irq(mal->txde_irq, mal_int, IRQF_SHARED,
+				"MAL TX DE", mal);
+		if (err)
+			goto fail3;
+		err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
+		if (err)
+			goto fail4;
+		err = request_irq(mal->rxde_irq, mal_int, IRQF_SHARED,
+				"MAL RX DE", mal);
+		if (err)
+			goto fail5;
+		err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
+		if (err)
+			goto fail6;
+	} else {
+		err = request_irq(mal->serr_irq, mal_serr, 0, "MAL SERR", mal);
+		if (err)
+			goto fail2;
+		err = request_irq(mal->txde_irq, mal_txde, 0, "MAL TX DE", mal);
+		if (err)
+			goto fail3;
+		err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
+		if (err)
+			goto fail4;
+		err = request_irq(mal->rxde_irq, mal_rxde, 0, "MAL RX DE", mal);
+		if (err)
+			goto fail5;
+		err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
+		if (err)
+			goto fail6;
+	}
 
 	/* Enable all MAL SERR interrupt sources */
 	if (mal->version == 2)
-- 
1.5.5.1

Re: [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ

[Simon Horman]

On Thu, Sep 04, 2008 at 11:02:16AM -0400, Josh Boyer wrote:
> The PowerPC 405EZ SoC has some differences in the interrupt layout and
> handling for the MAL.  The SERR, TXDE, and RXDE interrupts are OR'd into
> a single interrupt.  Also, due to the possibility for interrupt coalescing,
> the TXEOB and RXEOB interrupts require an interrupt bit to be cleared in
> the ICINTSTAT SDR.
> 
> This sets the proper MAL feature bits for 405EZ boards, and adds a common
> shared handler for SERR, TXDE, and RXDE.  This has been adapted from code
> originally written by Stefan Roese.
> 
> Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
> ---
>  drivers/net/ibm_newemac/mal.c |   98 ++++++++++++++++++++++++++++++++--------
>  1 files changed, 78 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ibm_newemac/mal.c b/drivers/net/ibm_newemac/mal.c
> index 10c267b..3cef534 100644
> --- a/drivers/net/ibm_newemac/mal.c
> +++ b/drivers/net/ibm_newemac/mal.c
> @@ -28,6 +28,7 @@
>  #include <linux/delay.h>
>  
>  #include "core.h"
> +#include <asm/dcr-regs.h>
>  
>  static int mal_count;
>  
> @@ -279,6 +280,9 @@ static irqreturn_t mal_txeob(int irq, void *dev_instance)
>  	mal_schedule_poll(mal);
>  	set_mal_dcrn(mal, MAL_TXEOBISR, r);
>  
> +	if (mal_has_feature(mal, MAL_FTR_CLEAR_ICINTSTAT))
> +		mtdcri(SDR0, 0x4510, (mfdcri(SDR0, 0x4510) | 0x60000000));
> +
>  	return IRQ_HANDLED;
>  }
>  
> @@ -293,6 +297,9 @@ static irqreturn_t mal_rxeob(int irq, void *dev_instance)
>  	mal_schedule_poll(mal);
>  	set_mal_dcrn(mal, MAL_RXEOBISR, r);
>  
> +	if (mal_has_feature(mal, MAL_FTR_CLEAR_ICINTSTAT))
> +		mtdcri(SDR0, 0x4510, (mfdcri(SDR0, 0x4510) | 0x80000000));
> +
>  	return IRQ_HANDLED;
>  }
>  
> @@ -336,6 +343,25 @@ static irqreturn_t mal_rxde(int irq, void *dev_instance)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t mal_int(int irq, void *dev_instance)
> +{
> +	struct mal_instance *mal = dev_instance;
> +	u32 esr = get_mal_dcrn(mal, MAL_ESR);
> +
> +	if (esr & MAL_ESR_EVB) {
> +		/* descriptor error */
> +		if (esr & MAL_ESR_DE) {
> +			if (esr & MAL_ESR_CIDT)
> +				return (mal_rxde(irq, dev_instance));

	Return statements shouldn't be enlosed in brackets according to
	checkpatch.pl. Also in a few other places.

> +			else
> +				return (mal_txde(irq, dev_instance));
> +		} else { /* SERR */
> +			return (mal_serr(irq, dev_instance));
> +		}
> +	}
> +	return IRQ_HANDLED;
> +}
> +
>  void mal_poll_disable(struct mal_instance *mal, struct mal_commac *commac)
>  {
>  	/* Spinlock-type semantics: only one caller disable poll at a time */
> @@ -542,11 +568,22 @@ static int __devinit mal_probe(struct of_device *ofdev,
>  		goto fail;
>  	}
>  
> -	mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
> -	mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
> -	mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
> -	mal->txde_irq = irq_of_parse_and_map(ofdev->node, 3);
> -	mal->rxde_irq = irq_of_parse_and_map(ofdev->node, 4);
> +	if (of_device_is_compatible(ofdev->node, "ibm,mcmal-405ez"))
> +		mal->features |= (MAL_FTR_CLEAR_ICINTSTAT | MAL_FTR_COMMON_ERR_INT);

The above like is >80 characters wide.
But I'm not sure that anyone cares.

> +
> +	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
> +		mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
> +		mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
> +		mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
> +		mal->txde_irq = mal->rxde_irq = mal->serr_irq;
> +	} else {
> +		mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
> +		mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
> +		mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
> +		mal->txde_irq = irq_of_parse_and_map(ofdev->node, 3);
> +		mal->rxde_irq = irq_of_parse_and_map(ofdev->node, 4);
> +	}

It seems that that first three calls to irq_of_parse_and_map() could
be moved outside of the if/else clause.

	mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
	mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
	mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
		mal->txde_irq = mal->rxde_irq = mal->serr_irq;
	} else {
		mal->txde_irq = irq_of_parse_and_map(ofdev->node, 3);
		mal->rxde_irq = irq_of_parse_and_map(ofdev->node, 4);
	}

> +
>  	if (mal->txeob_irq == NO_IRQ || mal->rxeob_irq == NO_IRQ ||
>  	    mal->serr_irq == NO_IRQ || mal->txde_irq == NO_IRQ ||
>  	    mal->rxde_irq == NO_IRQ) {
> @@ -608,21 +645,42 @@ static int __devinit mal_probe(struct of_device *ofdev,
>  			     sizeof(struct mal_descriptor) *
>  			     mal_rx_bd_offset(mal, i));
>  
> -	err = request_irq(mal->serr_irq, mal_serr, 0, "MAL SERR", mal);
> -	if (err)
> -		goto fail2;
> -	err = request_irq(mal->txde_irq, mal_txde, 0, "MAL TX DE", mal);
> -	if (err)
> -		goto fail3;
> -	err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
> -	if (err)
> -		goto fail4;
> -	err = request_irq(mal->rxde_irq, mal_rxde, 0, "MAL RX DE", mal);
> -	if (err)
> -		goto fail5;
> -	err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
> -	if (err)
> -		goto fail6;
> +	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
> +		err = request_irq(mal->serr_irq, mal_int, IRQF_SHARED,
> +				"MAL SERR", mal);
> +		if (err)
> +			goto fail2;
> +		err = request_irq(mal->txde_irq, mal_int, IRQF_SHARED,
> +				"MAL TX DE", mal);
> +		if (err)
> +			goto fail3;
> +		err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
> +		if (err)
> +			goto fail4;
> +		err = request_irq(mal->rxde_irq, mal_int, IRQF_SHARED,
> +				"MAL RX DE", mal);
> +		if (err)
> +			goto fail5;
> +		err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
> +		if (err)
> +			goto fail6;
> +	} else {
> +		err = request_irq(mal->serr_irq, mal_serr, 0, "MAL SERR", mal);
> +		if (err)
> +			goto fail2;
> +		err = request_irq(mal->txde_irq, mal_txde, 0, "MAL TX DE", mal);
> +		if (err)
> +			goto fail3;
> +		err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
> +		if (err)
> +			goto fail4;
> +		err = request_irq(mal->rxde_irq, mal_rxde, 0, "MAL RX DE", mal);
> +		if (err)
> +			goto fail5;
> +		err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
> +		if (err)
> +			goto fail6;
> +	}

There seems to be a lot of repention in the above if/else clauses.
I wonder if something like this might be nicer.

	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
		irqflags = IRQF_SHARED;
		hdlr_serr = hdlr_txde = hdlr_rxde = mal_int;
	} else {
		irqflags = 0;
		hdlr_serr = mal_serr;
		hdlr_txde = mal_txde;
		hdlr_rxde = mal_rxde;
	}
	err = request_irq(mal->serr_irq, hdlr_serr, irqflags, "MAL SERR", mal);
	if (err)
		goto fail2;
	err = request_irq(mal->txde_irq, hdlr_txde, irqflags, "MAL TX DE", mal);
	if (err)
		goto fail3;
	err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
	if (err)
		goto fail4;
	err = request_irq(mal->rxde_irq, hdlr_rxde, irqflags, "MAL RX DE", mal);
	if (err)
		goto fail5;
	err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
	if (err)
		goto fail6;

>  
>  	/* Enable all MAL SERR interrupt sources */
>  	if (mal->version == 2)
> -- 
> 1.5.5.1
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] ibm_newemac: Introduce mal_has_feature

[Benjamin Herrenschmidt]

On Thu, 2008-09-04 at 11:02 -0400, Josh Boyer wrote:
> There are some PowerPC SoCs that do odd things with the MAL handling.  In
> order to accommodate them, we need to introduce a feature mechanism that is
> similar to the existing emac_has_feature function.
> 
> This adds a feature variable to the mal_instance structure, and adds a
> mal_has_feature function with some feature definitions.  These are guarded
> by Kconfig options that are selected by the affected platforms.
> 
> Signed-of-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>

You also add an actual feature (CLR_ICINSTAT). You should document that
or move it to a separate patch.

> +/* Features of various MAL implementations */
> +
> +/* Dummy feature bit so the enum works properly */
> +#define MAL_FTR_DUMMY	0x00000001

Nah. Just stick an | 0 in the enum to make it happy.

Cheers,
Ben.

Re: [PATCH 2/3] ibm_newemac: Introduce mal_has_feature

[Josh Boyer]

On Fri, 05 Sep 2008 13:19:23 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Thu, 2008-09-04 at 11:02 -0400, Josh Boyer wrote:
> > There are some PowerPC SoCs that do odd things with the MAL handling.  In
> > order to accommodate them, we need to introduce a feature mechanism that is
> > similar to the existing emac_has_feature function.
> > 
> > This adds a feature variable to the mal_instance structure, and adds a
> > mal_has_feature function with some feature definitions.  These are guarded
> > by Kconfig options that are selected by the affected platforms.
> > 
> > Signed-of-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
> 
> You also add an actual feature (CLR_ICINSTAT). You should document that
> or move it to a separate patch.

I add two features.  And I so in the commit log, though I didn't
actually say what those do.  I can fix that up.

> 
> > +/* Features of various MAL implementations */
> > +
> > +/* Dummy feature bit so the enum works properly */
> > +#define MAL_FTR_DUMMY	0x00000001
> 
> Nah. Just stick an | 0 in the enum to make it happy.

OK.  I did that originally, but for some reason I wanted to avoid
having FTRS_ALWAYS and FTRS_POSSIBLE be equal if none of the Kconfig
options were set.  I can't remember why though.

josh

Re: [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ

[Josh Boyer]

On Fri, 5 Sep 2008 12:10:37 +1000
Simon Horman <horms@verge.net.au> wrote:
 
> > +static irqreturn_t mal_int(int irq, void *dev_instance)
> > +{
> > +	struct mal_instance *mal = dev_instance;
> > +	u32 esr = get_mal_dcrn(mal, MAL_ESR);
> > +
> > +	if (esr & MAL_ESR_EVB) {
> > +		/* descriptor error */
> > +		if (esr & MAL_ESR_DE) {
> > +			if (esr & MAL_ESR_CIDT)
> > +				return (mal_rxde(irq, dev_instance));
> 
> 	Return statements shouldn't be enlosed in brackets according to
> 	checkpatch.pl. Also in a few other places.

I hate checkpatch, but that's easy enough to fix.  Though I don't see
what other places I add with that mistake.

> > +			else
> > +				return (mal_txde(irq, dev_instance));
> > +		} else { /* SERR */
> > +			return (mal_serr(irq, dev_instance));
> > +		}
> > +	}
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  void mal_poll_disable(struct mal_instance *mal, struct mal_commac *commac)
> >  {
> >  	/* Spinlock-type semantics: only one caller disable poll at a time */
> > @@ -542,11 +568,22 @@ static int __devinit mal_probe(struct of_device *ofdev,
> >  		goto fail;
> >  	}
> >  
> > -	mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
> > -	mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
> > -	mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
> > -	mal->txde_irq = irq_of_parse_and_map(ofdev->node, 3);
> > -	mal->rxde_irq = irq_of_parse_and_map(ofdev->node, 4);
> > +	if (of_device_is_compatible(ofdev->node, "ibm,mcmal-405ez"))
> > +		mal->features |= (MAL_FTR_CLEAR_ICINTSTAT | MAL_FTR_COMMON_ERR_INT);
> 
> The above like is >80 characters wide.
> But I'm not sure that anyone cares.

I don't.  If Ben complains I'll change it.

> > +
> > +	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
> > +		mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
> > +		mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
> > +		mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
> > +		mal->txde_irq = mal->rxde_irq = mal->serr_irq;
> > +	} else {
> > +		mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
> > +		mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
> > +		mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
> > +		mal->txde_irq = irq_of_parse_and_map(ofdev->node, 3);
> > +		mal->rxde_irq = irq_of_parse_and_map(ofdev->node, 4);
> > +	}
> 
> It seems that that first three calls to irq_of_parse_and_map() could
> be moved outside of the if/else clause.
> 
> 	mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
> 	mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
> 	mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
> 	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
> 		mal->txde_irq = mal->rxde_irq = mal->serr_irq;
> 	} else {
> 		mal->txde_irq = irq_of_parse_and_map(ofdev->node, 3);
> 		mal->rxde_irq = irq_of_parse_and_map(ofdev->node, 4);
> 	}

Indeed they could.  Good catch.

> > +
> >  	if (mal->txeob_irq == NO_IRQ || mal->rxeob_irq == NO_IRQ ||
> >  	    mal->serr_irq == NO_IRQ || mal->txde_irq == NO_IRQ ||
> >  	    mal->rxde_irq == NO_IRQ) {
> > @@ -608,21 +645,42 @@ static int __devinit mal_probe(struct of_device *ofdev,
> >  			     sizeof(struct mal_descriptor) *
> >  			     mal_rx_bd_offset(mal, i));
> >  
> > -	err = request_irq(mal->serr_irq, mal_serr, 0, "MAL SERR", mal);
> > -	if (err)
> > -		goto fail2;
> > -	err = request_irq(mal->txde_irq, mal_txde, 0, "MAL TX DE", mal);
> > -	if (err)
> > -		goto fail3;
> > -	err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
> > -	if (err)
> > -		goto fail4;
> > -	err = request_irq(mal->rxde_irq, mal_rxde, 0, "MAL RX DE", mal);
> > -	if (err)
> > -		goto fail5;
> > -	err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
> > -	if (err)
> > -		goto fail6;
> > +	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
> > +		err = request_irq(mal->serr_irq, mal_int, IRQF_SHARED,
> > +				"MAL SERR", mal);
> > +		if (err)
> > +			goto fail2;
> > +		err = request_irq(mal->txde_irq, mal_int, IRQF_SHARED,
> > +				"MAL TX DE", mal);
> > +		if (err)
> > +			goto fail3;
> > +		err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
> > +		if (err)
> > +			goto fail4;
> > +		err = request_irq(mal->rxde_irq, mal_int, IRQF_SHARED,
> > +				"MAL RX DE", mal);
> > +		if (err)
> > +			goto fail5;
> > +		err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
> > +		if (err)
> > +			goto fail6;
> > +	} else {
> > +		err = request_irq(mal->serr_irq, mal_serr, 0, "MAL SERR", mal);
> > +		if (err)
> > +			goto fail2;
> > +		err = request_irq(mal->txde_irq, mal_txde, 0, "MAL TX DE", mal);
> > +		if (err)
> > +			goto fail3;
> > +		err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
> > +		if (err)
> > +			goto fail4;
> > +		err = request_irq(mal->rxde_irq, mal_rxde, 0, "MAL RX DE", mal);
> > +		if (err)
> > +			goto fail5;
> > +		err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
> > +		if (err)
> > +			goto fail6;
> > +	}
> 
> There seems to be a lot of repention in the above if/else clauses.
> I wonder if something like this might be nicer.
> 
> 	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
> 		irqflags = IRQF_SHARED;
> 		hdlr_serr = hdlr_txde = hdlr_rxde = mal_int;
> 	} else {
> 		irqflags = 0;
> 		hdlr_serr = mal_serr;
> 		hdlr_txde = mal_txde;
> 		hdlr_rxde = mal_rxde;
> 	}
> 	err = request_irq(mal->serr_irq, hdlr_serr, irqflags, "MAL SERR", mal);
> 	if (err)
> 		goto fail2;
> 	err = request_irq(mal->txde_irq, hdlr_txde, irqflags, "MAL TX DE", mal);
> 	if (err)
> 		goto fail3;
> 	err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
> 	if (err)
> 		goto fail4;
> 	err = request_irq(mal->rxde_irq, hdlr_rxde, irqflags, "MAL RX DE", mal);
> 	if (err)
> 		goto fail5;
> 	err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
> 	if (err)
> 		goto fail6;

I like it.  Much cleaner.  I'll fix that up too.

josh

Re: [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ

[Benjamin Herrenschmidt]

> > > +	if (of_device_is_compatible(ofdev->node, "ibm,mcmal-405ez"))
> > > +		mal->features |= (MAL_FTR_CLEAR_ICINTSTAT | MAL_FTR_COMMON_ERR_INT);
> > 
> > The above like is >80 characters wide.
> > But I'm not sure that anyone cares.
> 
> I don't.  If Ben complains I'll change it.

I wouldn't do a tatrum over it but if you're going to respin the
patch you may as well put the second constant on the next line.

Cheers,
Ben.