* [PATCH 1/3] ibm_newemac: Allow the "no flow control" EMAC feature to work [not found] <cover.1220540078.git.jwboyer@linux.vnet.ibm.com> @ 2008-09-04 15:01 ` Josh Boyer 2008-09-04 15:02 ` [PATCH 2/3] ibm_newemac: Introduce mal_has_feature Josh Boyer 2008-09-04 15:02 ` [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ Josh Boyer 2 siblings, 0 replies; 10+ messages in thread From: Josh Boyer @ 2008-09-04 15:01 UTC (permalink / raw) To: benh, netdev; +Cc: linuxppc-dev 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] ibm_newemac: Introduce mal_has_feature [not found] <cover.1220540078.git.jwboyer@linux.vnet.ibm.com> 2008-09-04 15:01 ` [PATCH 1/3] ibm_newemac: Allow the "no flow control" EMAC feature to work Josh Boyer @ 2008-09-04 15:02 ` Josh Boyer 2008-09-05 3:19 ` Benjamin Herrenschmidt 2008-09-04 15:02 ` [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ Josh Boyer 2 siblings, 1 reply; 10+ messages in thread From: Josh Boyer @ 2008-09-04 15:02 UTC (permalink / raw) To: benh, netdev; +Cc: linuxppc-dev 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ibm_newemac: Introduce mal_has_feature 2008-09-04 15:02 ` [PATCH 2/3] ibm_newemac: Introduce mal_has_feature Josh Boyer @ 2008-09-05 3:19 ` Benjamin Herrenschmidt 2008-09-05 3:37 ` Josh Boyer 0 siblings, 1 reply; 10+ messages in thread From: Benjamin Herrenschmidt @ 2008-09-05 3:19 UTC (permalink / raw) To: Josh Boyer; +Cc: netdev, linuxppc-dev 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ibm_newemac: Introduce mal_has_feature 2008-09-05 3:19 ` Benjamin Herrenschmidt @ 2008-09-05 3:37 ` Josh Boyer 0 siblings, 0 replies; 10+ messages in thread From: Josh Boyer @ 2008-09-05 3:37 UTC (permalink / raw) To: benh; +Cc: netdev, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ [not found] <cover.1220540078.git.jwboyer@linux.vnet.ibm.com> 2008-09-04 15:01 ` [PATCH 1/3] ibm_newemac: Allow the "no flow control" EMAC feature to work Josh Boyer 2008-09-04 15:02 ` [PATCH 2/3] ibm_newemac: Introduce mal_has_feature Josh Boyer @ 2008-09-04 15:02 ` Josh Boyer 2008-09-05 2:10 ` Simon Horman 2 siblings, 1 reply; 10+ messages in thread From: Josh Boyer @ 2008-09-04 15:02 UTC (permalink / raw) To: benh, netdev; +Cc: linuxppc-dev 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ 2008-09-04 15:02 ` [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ Josh Boyer @ 2008-09-05 2:10 ` Simon Horman 0 siblings, 0 replies; 10+ messages in thread From: Simon Horman @ 2008-09-05 2:10 UTC (permalink / raw) To: Josh Boyer; +Cc: linuxppc-dev, netdev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ @ 2008-09-05 2:10 ` Simon Horman 0 siblings, 0 replies; 10+ messages in thread From: Simon Horman @ 2008-09-05 2:10 UTC (permalink / raw) To: Josh Boyer; +Cc: benh, netdev, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ 2008-09-05 2:10 ` Simon Horman @ 2008-09-05 3:41 ` Josh Boyer -1 siblings, 0 replies; 10+ messages in thread From: Josh Boyer @ 2008-09-05 3:41 UTC (permalink / raw) To: Simon Horman; +Cc: linuxppc-dev, netdev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ @ 2008-09-05 3:41 ` Josh Boyer 0 siblings, 0 replies; 10+ messages in thread From: Josh Boyer @ 2008-09-05 3:41 UTC (permalink / raw) To: Simon Horman; +Cc: benh, netdev, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ 2008-09-05 3:41 ` Josh Boyer (?) @ 2008-09-05 9:34 ` Benjamin Herrenschmidt -1 siblings, 0 replies; 10+ messages in thread From: Benjamin Herrenschmidt @ 2008-09-05 9:34 UTC (permalink / raw) To: Josh Boyer; +Cc: netdev, Simon Horman, linuxppc-dev > > > + 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2008-09-05 9:35 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1220540078.git.jwboyer@linux.vnet.ibm.com>
2008-09-04 15:01 ` [PATCH 1/3] ibm_newemac: Allow the "no flow control" EMAC feature to work Josh Boyer
2008-09-04 15:02 ` [PATCH 2/3] ibm_newemac: Introduce mal_has_feature Josh Boyer
2008-09-05 3:19 ` Benjamin Herrenschmidt
2008-09-05 3:37 ` Josh Boyer
2008-09-04 15:02 ` [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ Josh Boyer
2008-09-05 2:10 ` Simon Horman
2008-09-05 2:10 ` Simon Horman
2008-09-05 3:41 ` Josh Boyer
2008-09-05 3:41 ` Josh Boyer
2008-09-05 9:34 ` Benjamin Herrenschmidt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.