diff mbox

Message ID 1407636153-24864-1-git-send-email-schmitzmic@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Michael Schmitz Aug. 10, 2014, 2:02 a.m. UTC 
new version of the patch adding Atari EtherNEC (IRQ-less ROM port ISA NE2k
adapter) support to ne.c, as announced before.

Thanks,

	Michael Schmitz

From 224ce049f71577d6ec380aeb94a4d25c3c4016a7 Mon Sep 17 00:00:00 2001
From: Michael Schmitz <schmitzmic@gmail.com>
Date: Sat, 6 Apr 2013 13:26:42 +1300
Subject: [PATCH] m68k/atari: EtherNEC - ethernet support (ne)

Support for Atari EtherNEC ROM port adapters in ne.c

Signed-off-by: Michael Schmitz <schmitz@debian.org>
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/net/ethernet/8390/Kconfig |    3 ++-
 drivers/net/ethernet/8390/ne.c    |    2 ++
 2 files changed, 4 insertions(+), 1 deletions(-)

Comments

Paul Gortmaker Aug. 12, 2014, 2:55 p.m. UTC | #1 
[Subject added from patch]

On 14-08-09 10:02 PM, Michael Schmitz wrote:
> new version of the patch adding Atari EtherNEC (IRQ-less ROM port ISA NE2k
> adapter) support to ne.c, as announced before.
> 
> Thanks,
> 
> 	Michael Schmitz

Please don't send subjectless mails by attaching a commit to a mail,
just use git send-email directly.

> 
> From 224ce049f71577d6ec380aeb94a4d25c3c4016a7 Mon Sep 17 00:00:00 2001
> From: Michael Schmitz <schmitzmic@gmail.com>
> Date: Sat, 6 Apr 2013 13:26:42 +1300
> Subject: [PATCH] m68k/atari: EtherNEC - ethernet support (ne)
> 
> Support for Atari EtherNEC ROM port adapters in ne.c

Is that all there is to say about these?  Nothing about which platforms
might have it, or what it was tested on or ... ?

> 
> Signed-off-by: Michael Schmitz <schmitz@debian.org>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  drivers/net/ethernet/8390/Kconfig |    3 ++-
>  drivers/net/ethernet/8390/ne.c    |    2 ++
>  2 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/8390/Kconfig b/drivers/net/ethernet/8390/Kconfig
> index 0988811..2d89bd0 100644
> --- a/drivers/net/ethernet/8390/Kconfig
> +++ b/drivers/net/ethernet/8390/Kconfig
> @@ -91,7 +91,8 @@ config MCF8390
>  
>  config NE2000
>  	tristate "NE2000/NE1000 support"
> -	depends on (ISA || (Q40 && m) || M32R || MACH_TX49XX)
> +	depends on (ISA || (Q40 && m) || M32R || MACH_TX49XX || \
> +		    ATARI_ETHERNEC)
>  	select CRC32
>  	---help---
>  	  If you have a network (Ethernet) card of this type, say Y and read
> diff --git a/drivers/net/ethernet/8390/ne.c b/drivers/net/ethernet/8390/ne.c
> index 58eaa8f..de566fb 100644
> --- a/drivers/net/ethernet/8390/ne.c
> +++ b/drivers/net/ethernet/8390/ne.c
> @@ -169,6 +169,8 @@ bad_clone_list[] __initdata = {
>  #elif defined(CONFIG_PLAT_OAKS32R)  || \
>     defined(CONFIG_MACH_TX49XX)
>  #  define DCR_VAL 0x48		/* 8-bit mode */
> +#elif defined(CONFIG_ATARI)	/* 8-bit mode on Atari, normal on Q40 */
> +#  define DCR_VAL (MACH_IS_ATARI ? 0x48 : 0x49)

This doesn't make sense.  Is not MACH_IS_ATARI set when CONFIG_ATARI
is set?  And even if it isn't then just set 48 for MACH_IS_ATARI and
let the default of 49 below take the ATARI=y and IS_ATARI=n case,
without the need for the ?: operator at all.  On top of that, why
aren't you using the ATARI_ETHERNEC option here for your 48 trigger,
instead of the much higher arch/mach level triggers at all?

P.
--

>  #else
>  #  define DCR_VAL 0x49
>  #endif
> 
--
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
Geert Uytterhoeven Aug. 12, 2014, 3:16 p.m. UTC | #2 
Hi Paul,

On Tue, Aug 12, 2014 at 4:55 PM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>> @@ -169,6 +169,8 @@ bad_clone_list[] __initdata = {
>>  #elif defined(CONFIG_PLAT_OAKS32R)  || \
>>     defined(CONFIG_MACH_TX49XX)
>>  #  define DCR_VAL 0x48               /* 8-bit mode */
>> +#elif defined(CONFIG_ATARI)  /* 8-bit mode on Atari, normal on Q40 */
>> +#  define DCR_VAL (MACH_IS_ATARI ? 0x48 : 0x49)
>
> This doesn't make sense.  Is not MACH_IS_ATARI set when CONFIG_ATARI
> is set?  And even if it isn't then just set 48 for MACH_IS_ATARI and

m68k kernels can be multi-platform, so we need the MACH_IS_ATARI()
runtime check.

> let the default of 49 below take the ATARI=y and IS_ATARI=n case,
> without the need for the ?: operator at all.  On top of that, why
> aren't you using the ATARI_ETHERNEC option here for your 48 trigger,
> instead of the much higher arch/mach level triggers at all?

CONFIG_ATARI_ETHERNEC could be used instead of CONFIG_ATARI,
but as ne.c is used on Atari with Ethernec only, it doesn't matter much.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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
Michael Schmitz Aug. 12, 2014, 9:40 p.m. UTC | #3 
Hi Paul,

On Wed, Aug 13, 2014 at 2:55 AM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
> [Subject added from patch]

Thanks - that was me botching git-send-email --annotate. My sincere
apology for this.

>> From 224ce049f71577d6ec380aeb94a4d25c3c4016a7 Mon Sep 17 00:00:00 2001
>> From: Michael Schmitz <schmitzmic@gmail.com>
>> Date: Sat, 6 Apr 2013 13:26:42 +1300
>> Subject: [PATCH] m68k/atari: EtherNEC - ethernet support (ne)
>>
>> Support for Atari EtherNEC ROM port adapters in ne.c
>
> Is that all there is to say about these?  Nothing about which platforms
> might have it, or what it was tested on or ... ?

All Atari computers have the ROM port. As such, the driver could be
used on MegaST/E, TT or Falcon variants (these having MMU support so
can run m68k linux).

The ne.c driver is also used on the Q40, a m68k platfrem which
includes the ISA bus. Hence the runtime check for whether the driver
runs on Atari, or some other m68k platform below.

>
>>
>> Signed-off-by: Michael Schmitz <schmitz@debian.org>
>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> ---
>>  drivers/net/ethernet/8390/Kconfig |    3 ++-
>>  drivers/net/ethernet/8390/ne.c    |    2 ++
>>  2 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/8390/Kconfig b/drivers/net/ethernet/8390/Kconfig
>> index 0988811..2d89bd0 100644
>> --- a/drivers/net/ethernet/8390/Kconfig
>> +++ b/drivers/net/ethernet/8390/Kconfig
>> @@ -91,7 +91,8 @@ config MCF8390
>>
>>  config NE2000
>>       tristate "NE2000/NE1000 support"
>> -     depends on (ISA || (Q40 && m) || M32R || MACH_TX49XX)
>> +     depends on (ISA || (Q40 && m) || M32R || MACH_TX49XX || \
>> +                 ATARI_ETHERNEC)
>>       select CRC32
>>       ---help---
>>         If you have a network (Ethernet) card of this type, say Y and read
>> diff --git a/drivers/net/ethernet/8390/ne.c b/drivers/net/ethernet/8390/ne.c
>> index 58eaa8f..de566fb 100644
>> --- a/drivers/net/ethernet/8390/ne.c
>> +++ b/drivers/net/ethernet/8390/ne.c
>> @@ -169,6 +169,8 @@ bad_clone_list[] __initdata = {
>>  #elif defined(CONFIG_PLAT_OAKS32R)  || \
>>     defined(CONFIG_MACH_TX49XX)
>>  #  define DCR_VAL 0x48               /* 8-bit mode */
>> +#elif defined(CONFIG_ATARI)  /* 8-bit mode on Atari, normal on Q40 */
>> +#  define DCR_VAL (MACH_IS_ATARI ? 0x48 : 0x49)
>
> This doesn't make sense.  Is not MACH_IS_ATARI set when CONFIG_ATARI

As Geert pointed out, MACH_IS_ATARI is a runtime test on multiplatform
kernels (which are the norm these days, even with m68k).

Would you be OK if I expanded the comment like this?

/* ne.c is used on m68k Atari and Q40 computers - the Atari ROM-port
adapter is 8-bit, Q40 uses ISA */

> is set?  And even if it isn't then just set 48 for MACH_IS_ATARI and
> let the default of 49 below take the ATARI=y and IS_ATARI=n case,
> without the need for the ?: operator at all.  On top of that, why

This would limit the runtime test to the case where it is actually
needed (untested):

#elif defined(CONFIG_ATARI) && defined(CONFIG_Q40)
/* multiplatform m68k kernel - the Atari ROM-port adapter is 8-bit,
Q40 uses ISA */
#  define DCR_VAL (MACH_IS_ATARI ? 0x48 : 0x49)
#elif defined(CONFIG_ATARI) /* no Q40 support - 8-bit mode on Atari ROM port */
#  define DCR_VAL 0x48
#else
#  define DCR_VAL 0x49
#endif

Are there any other m68k platforms that use the ne.c driver, Geert?
apne, hydra and zorro8390 all have their own separate drivers - any
others?

> aren't you using the ATARI_ETHERNEC option here for your 48 trigger,
> instead of the much higher arch/mach level triggers at all?

Fair enough - I can do that. I though using the mach level one made it
clearer this is about multiplatform issues.

We still need the runtime test on kernels configured for both Atari
and Q40 though. Which variant do you prefer?

Thanks,

  Michael


>
> P.
> --
>
>>  #else
>>  #  define DCR_VAL 0x49
>>  #endif
>>
--
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
Geert Uytterhoeven Aug. 13, 2014, 7:15 a.m. UTC | #4 
Hi Michael,

On Tue, Aug 12, 2014 at 11:40 PM, Michael Schmitz <schmitzmic@gmail.com> wrote:
> Would you be OK if I expanded the comment like this?
>
> /* ne.c is used on m68k Atari and Q40 computers - the Atari ROM-port
> adapter is 8-bit, Q40 uses ISA */

DaveM already applied it to his tree, and sent a pull request to Linus.

> This would limit the runtime test to the case where it is actually
> needed (untested):
>
> #elif defined(CONFIG_ATARI) && defined(CONFIG_Q40)
> /* multiplatform m68k kernel - the Atari ROM-port adapter is 8-bit,
> Q40 uses ISA */
> #  define DCR_VAL (MACH_IS_ATARI ? 0x48 : 0x49)
> #elif defined(CONFIG_ATARI) /* no Q40 support - 8-bit mode on Atari ROM port */
> #  define DCR_VAL 0x48
> #else
> #  define DCR_VAL 0x49
> #endif

MACH_IS_ATARI already evaluates to a constant on single-platform
kernels, so this is optimized at compile time.
Hence there's not really a need for this.

> Are there any other m68k platforms that use the ne.c driver, Geert?
> apne, hydra and zorro8390 all have their own separate drivers - any
> others?

Not that I'm aware of. Only Q40 has CONFIG_NE2000 in defconfig.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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
Michael Schmitz Aug. 13, 2014, 8:37 a.m. UTC | #5 
Hi Geert,

>
>> Would you be OK if I expanded the comment like this?
>>
>> /* ne.c is used on m68k Atari and Q40 computers - the Atari ROM-port
>> adapter is 8-bit, Q40 uses ISA */
>>     
>
> DaveM already applied it to his tree, and sent a pull request to Linus.
>   

OK - won't resend then unless explicitly requested.

Many thanks, Dave!

>> This would limit the runtime test to the case where it is actually
>> needed (untested):
>>
>> #elif defined(CONFIG_ATARI) && defined(CONFIG_Q40)
>> /* multiplatform m68k kernel - the Atari ROM-port adapter is 8-bit,
>> Q40 uses ISA */
>> #  define DCR_VAL (MACH_IS_ATARI ? 0x48 : 0x49)
>> #elif defined(CONFIG_ATARI) /* no Q40 support - 8-bit mode on Atari ROM port */
>> #  define DCR_VAL 0x48
>> #else
>> #  define DCR_VAL 0x49
>> #endif
>>     
>
> MACH_IS_ATARI already evaluates to a constant on single-platform
> kernels, so this is optimized at compile time.
> Hence there's not really a need for this.
>   

I was hoping it would. More of an attempt to make the logic easier to 
understand, but a comment would be sufficient (and avoid redundancy).

>> Are there any other m68k platforms that use the ne.c driver, Geert?
>> apne, hydra and zorro8390 all have their own separate drivers - any
>> others?
>>     
>
> Not that I'm aware of. Only Q40 has CONFIG_NE2000 in defconfig.
>   

Good - we can finally put this one to rest.

Cheers,

    Michael


> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>   

--
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
diff mbox

Patch

diff --git a/drivers/net/ethernet/8390/Kconfig b/drivers/net/ethernet/8390/Kconfig
index 0988811..2d89bd0 100644
--- a/drivers/net/ethernet/8390/Kconfig
+++ b/drivers/net/ethernet/8390/Kconfig
@@ -91,7 +91,8 @@  config MCF8390
 
 config NE2000
 	tristate "NE2000/NE1000 support"
-	depends on (ISA || (Q40 && m) || M32R || MACH_TX49XX)
+	depends on (ISA || (Q40 && m) || M32R || MACH_TX49XX || \
+		    ATARI_ETHERNEC)
 	select CRC32
 	---help---
 	  If you have a network (Ethernet) card of this type, say Y and read
diff --git a/drivers/net/ethernet/8390/ne.c b/drivers/net/ethernet/8390/ne.c
index 58eaa8f..de566fb 100644
--- a/drivers/net/ethernet/8390/ne.c
+++ b/drivers/net/ethernet/8390/ne.c
@@ -169,6 +169,8 @@  bad_clone_list[] __initdata = {
 #elif defined(CONFIG_PLAT_OAKS32R)  || \
    defined(CONFIG_MACH_TX49XX)
 #  define DCR_VAL 0x48		/* 8-bit mode */
+#elif defined(CONFIG_ATARI)	/* 8-bit mode on Atari, normal on Q40 */
+#  define DCR_VAL (MACH_IS_ATARI ? 0x48 : 0x49)
 #else
 #  define DCR_VAL 0x49
 #endif