LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] powerpc: pair loads and stores in copy_4k_page
From: Anton Blanchard @ 2010-02-11  4:07 UTC (permalink / raw)
  To: benh, markn; +Cc: linuxppc-dev


A number of our chips like loads and stores to be paired. A small kernel
module testcase shows the improvement of pairing loads and stores in 
copy_4k_page:

POWER6: +9%
POWER7: +1.5%


#include <linux/module.h>
#include <linux/mm.h>

#define ITERATIONS 10000000

static int __init copypage_init(void)
{
	struct timespec before, after;
	unsigned long i;
	struct page *destpage, *srcpage;
	char *dest, *src;

	destpage = alloc_page(GFP_KERNEL);
	srcpage = alloc_page(GFP_KERNEL);

	dest = page_address(destpage);
	src = page_address(srcpage);

	getnstimeofday(&before);

	for (i = 0; i < ITERATIONS; i++)
		copy_4K_page(dest, src);

	getnstimeofday(&after);

	free_page((unsigned long)dest);
	free_page((unsigned long)src);

	printk(KERN_DEBUG "copy_4K_page loop took %lu ns\n",
		(after.tv_sec - before.tv_sec) * NSEC_PER_SEC +
		(after.tv_nsec - before.tv_nsec));

	return 0;
}

static void __exit copypage_exit(void)
{
}

module_init(copypage_init)
module_exit(copypage_exit)
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Anton Blanchard");


Signed-off-by: Anton Blanchard <anton@samba.org>
---

diff --git a/arch/powerpc/lib/copypage_64.S b/arch/powerpc/lib/copypage_64.S
index 75f3267..22b6c7b 100644
--- a/arch/powerpc/lib/copypage_64.S
+++ b/arch/powerpc/lib/copypage_64.S
@@ -43,62 +43,62 @@ END_FTR_SECTION_IFSET(CPU_FTR_CP_USE_DCBTZ)
 	ld	r7,16(r4)
 	ldu	r8,24(r4)
 1:	std	r5,8(r3)
-	ld	r9,8(r4)
 	std	r6,16(r3)
+	ld	r9,8(r4)
 	ld	r10,16(r4)
 	std	r7,24(r3)
-	ld	r11,24(r4)
 	std	r8,32(r3)
+	ld	r11,24(r4)
 	ld	r12,32(r4)
 	std	r9,40(r3)
-	ld	r5,40(r4)
 	std	r10,48(r3)
+	ld	r5,40(r4)
 	ld	r6,48(r4)
 	std	r11,56(r3)
-	ld	r7,56(r4)
 	std	r12,64(r3)
+	ld	r7,56(r4)
 	ld	r8,64(r4)
 	std	r5,72(r3)
-	ld	r9,72(r4)
 	std	r6,80(r3)
+	ld	r9,72(r4)
 	ld	r10,80(r4)
 	std	r7,88(r3)
-	ld	r11,88(r4)
 	std	r8,96(r3)
+	ld	r11,88(r4)
 	ld	r12,96(r4)
 	std	r9,104(r3)
-	ld	r5,104(r4)
 	std	r10,112(r3)
+	ld	r5,104(r4)
 	ld	r6,112(r4)
 	std	r11,120(r3)
-	ld	r7,120(r4)
 	stdu	r12,128(r3)
+	ld	r7,120(r4)
 	ldu	r8,128(r4)
 	bdnz	1b
 
 	std	r5,8(r3)
-	ld	r9,8(r4)
 	std	r6,16(r3)
+	ld	r9,8(r4)
 	ld	r10,16(r4)
 	std	r7,24(r3)
-	ld	r11,24(r4)
 	std	r8,32(r3)
+	ld	r11,24(r4)
 	ld	r12,32(r4)
 	std	r9,40(r3)
-	ld	r5,40(r4)
 	std	r10,48(r3)
+	ld	r5,40(r4)
 	ld	r6,48(r4)
 	std	r11,56(r3)
-	ld	r7,56(r4)
 	std	r12,64(r3)
+	ld	r7,56(r4)
 	ld	r8,64(r4)
 	std	r5,72(r3)
-	ld	r9,72(r4)
 	std	r6,80(r3)
+	ld	r9,72(r4)
 	ld	r10,80(r4)
 	std	r7,88(r3)
-	ld	r11,88(r4)
 	std	r8,96(r3)
+	ld	r11,88(r4)
 	ld	r12,96(r4)
 	std	r9,104(r3)
 	std	r10,112(r3)

^ permalink raw reply related

* Re: register long sp asm("r1") incorrect
From: Benjamin Herrenschmidt @ 2010-02-11  5:31 UTC (permalink / raw)
  To: Pavel Machek; +Cc: linuxppc-dev, paulus, kernel list
In-Reply-To: <20100209152444.GA30176@atrey.karlin.mff.cuni.cz>

On Tue, 2010-02-09 at 16:24 +0100, Pavel Machek wrote:
> ...according to gcc docs, sp should be global, or placement in
> register is not guaranteed (except at asm boundaries, but there are
> none).

Sorry I'm not sure I grok what you mean.

Cheers,
Ben.

^ permalink raw reply

* Re: [PATCH] powerpc: pair loads and stores in copy_4k_page
From: Mark Nelson @ 2010-02-11  6:25 UTC (permalink / raw)
  To: Anton Blanchard; +Cc: linuxppc-dev
In-Reply-To: <20100211040754.GH3399@kryten>

Hi Anton,

On Thursday 11 February 2010 15:07:54 Anton Blanchard wrote:
> 
> A number of our chips like loads and stores to be paired. A small kernel
> module testcase shows the improvement of pairing loads and stores in 
> copy_4k_page:
> 
> POWER6: +9%
> POWER7: +1.5%

I just tried this on one of our QS22 cell blades and it seems to cause
about half a percent speedup but that looks like it's within the noise
of the results that I'm getting.

In any case it doesn't look like it has a negative effect for cell.

Looks good!

Mark

^ permalink raw reply

* Re: [PATCH 1/6] powerpc: Use lwarx hint in spinlocks
From: Nick Piggin @ 2010-02-11  6:56 UTC (permalink / raw)
  To: Anton Blanchard; +Cc: linuxppc-dev
In-Reply-To: <20100210105728.GA3399@kryten>

On Wed, Feb 10, 2010 at 09:57:28PM +1100, Anton Blanchard wrote:
> 
> Recent versions of the PowerPC architecture added a hint bit to the larx
> instructions to differentiate between an atomic operation and a lock operation:
> 
> > 0 Other programs might attempt to modify the word in storage addressed by EA
> > even if the subsequent Store Conditional succeeds.
> > 
> > 1 Other programs will not attempt to modify the word in storage addressed by
> > EA until the program that has acquired the lock performs a subsequent store
> > releasing the lock.
> 
> To avoid a binutils dependency this patch create macros for the extended lwarx
> format and uses it in the spinlock code. To test this change I used a simple
> test case that acquires and releases a global pthread mutex:
> 
> 	pthread_mutex_lock(&mutex);
> 	pthread_mutex_unlock(&mutex);
> 
> On a 32 core POWER6, running 32 test threads we spend almost all our time in
> the futex spinlock code:
> 
>     94.37%     perf  [kernel]                     [k] ._raw_spin_lock
>                |          
>                |--99.95%-- ._raw_spin_lock
>                |          |          
>                |          |--63.29%-- .futex_wake
>                |          |          
>                |          |--36.64%-- .futex_wait_setup
> 
> Which is a good test for this patch. The results (in lock/unlock operations per
> second) are:
> 
> before: 1538203 ops/sec
> after:  2189219 ops/sec
> 
> An improvement of 42%
> 
> A 32 core POWER7 improves even more:
> 
> before: 1279529 ops/sec
> after:  2282076 ops/sec
> 
> An improvement of 78%

Cool. How does it go when there are significant amount of instructions
between the lock and the unlock? A real(ish) workload, like dbench on
ramdisk (which should hit the dcache lock).

> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
> 
> v2: We do this only for 64bit until we can verify all 32bit CPUs.
> 
> Tested so far: 970 (thanks Ben), POWER5, POWER6, POWER7
> Still to test: RS64, POWER3, POWER4
> 
> Index: powerpc.git/arch/powerpc/include/asm/ppc-opcode.h
> ===================================================================
> --- powerpc.git.orig/arch/powerpc/include/asm/ppc-opcode.h	2010-02-10 15:28:58.453072362 +1100
> +++ powerpc.git/arch/powerpc/include/asm/ppc-opcode.h	2010-02-10 15:33:08.963071793 +1100
> @@ -24,6 +24,7 @@
>  #define PPC_INST_ISEL_MASK		0xfc00003e
>  #define PPC_INST_LSWI			0x7c0004aa
>  #define PPC_INST_LSWX			0x7c00042a
> +#define PPC_INST_LWARX			0x7c000029
>  #define PPC_INST_LWSYNC			0x7c2004ac
>  #define PPC_INST_LXVD2X			0x7c000698
>  #define PPC_INST_MCRXR			0x7c000400
> @@ -55,15 +56,28 @@
>  #define __PPC_RA(a)	(((a) & 0x1f) << 16)
>  #define __PPC_RB(b)	(((b) & 0x1f) << 11)
>  #define __PPC_RS(s)	(((s) & 0x1f) << 21)
> +#define __PPC_RT(s)	__PPC_RS(s)
>  #define __PPC_XS(s)	((((s) & 0x1f) << 21) | (((s) & 0x20) >> 5))
>  #define __PPC_T_TLB(t)	(((t) & 0x3) << 21)
>  #define __PPC_WC(w)	(((w) & 0x3) << 21)
> +/*
> + * Only use the larx hint bit on 64bit CPUs. Once we verify it doesn't have
> + * any side effects on all 32bit processors, we can do this all the time.
> + */
> +#ifdef CONFIG_PPC64
> +#define __PPC_EH(eh)	(((eh) & 0x1) << 0)
> +#else
> +#define __PPC_EH(eh)	0
> +#endif
>  
>  /* Deal with instructions that older assemblers aren't aware of */
>  #define	PPC_DCBAL(a, b)		stringify_in_c(.long PPC_INST_DCBAL | \
>  					__PPC_RA(a) | __PPC_RB(b))
>  #define	PPC_DCBZL(a, b)		stringify_in_c(.long PPC_INST_DCBZL | \
>  					__PPC_RA(a) | __PPC_RB(b))
> +#define PPC_LWARX(t, a, b, eh)	stringify_in_c(.long PPC_INST_LWARX | \
> +					__PPC_RT(t) | __PPC_RA(a) | \
> +					__PPC_RB(b) | __PPC_EH(eh))
>  #define PPC_MSGSND(b)		stringify_in_c(.long PPC_INST_MSGSND | \
>  					__PPC_RB(b))
>  #define PPC_RFCI		stringify_in_c(.long PPC_INST_RFCI)
> Index: powerpc.git/arch/powerpc/include/asm/spinlock.h
> ===================================================================
> --- powerpc.git.orig/arch/powerpc/include/asm/spinlock.h	2010-02-10 15:28:58.473072327 +1100
> +++ powerpc.git/arch/powerpc/include/asm/spinlock.h	2010-02-10 15:29:29.454322618 +1100
> @@ -27,6 +27,7 @@
>  #endif
>  #include <asm/asm-compat.h>
>  #include <asm/synch.h>
> +#include <asm/ppc-opcode.h>
>  
>  #define arch_spin_is_locked(x)		((x)->slock != 0)
>  
> @@ -60,7 +61,7 @@ static inline unsigned long __arch_spin_
>  
>  	token = LOCK_TOKEN;
>  	__asm__ __volatile__(
> -"1:	lwarx		%0,0,%2\n\
> +"1:	" PPC_LWARX(%0,0,%2,1) "\n\
>  	cmpwi		0,%0,0\n\
>  	bne-		2f\n\
>  	stwcx.		%1,0,%2\n\
> @@ -186,7 +187,7 @@ static inline long __arch_read_trylock(a
>  	long tmp;
>  
>  	__asm__ __volatile__(
> -"1:	lwarx		%0,0,%1\n"
> +"1:	" PPC_LWARX(%0,0,%1,1) "\n"
>  	__DO_SIGN_EXTEND
>  "	addic.		%0,%0,1\n\
>  	ble-		2f\n"
> @@ -211,7 +212,7 @@ static inline long __arch_write_trylock(
>  
>  	token = WRLOCK_TOKEN;
>  	__asm__ __volatile__(
> -"1:	lwarx		%0,0,%2\n\
> +"1:	" PPC_LWARX(%0,0,%2,1) "\n\
>  	cmpwi		0,%0,0\n\
>  	bne-		2f\n"
>  	PPC405_ERR77(0,%1)

^ permalink raw reply

* Re: [PATCH 6/6] powerpc: Use lwsync for acquire barrier if CPU supports it
From: Nick Piggin @ 2010-02-11  7:09 UTC (permalink / raw)
  To: Anton Blanchard; +Cc: linuxppc-dev
In-Reply-To: <20100210111025.GF3399@kryten>

On Wed, Feb 10, 2010 at 10:10:25PM +1100, Anton Blanchard wrote:
> 
> Nick Piggin discovered that lwsync barriers around locks were faster than isync
> on 970. That was a long time ago and I completely dropped the ball in testing
> his patches across other ppc64 processors.
> 
> Turns out the idea helps on other chips. Using a microbenchmark that
> uses a lot of threads to contend on a global pthread mutex (and therefore a
> global futex), POWER6 improves 8% and POWER7 improves 2%. I checked POWER5
> and while I couldn't measure an improvement, there was no regression.

Ah, good to see this one come back. I also tested tbench over localhost
btw which actually did show some speedup on the G5. 

BTW. this was the last thing left:
http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg29738.html

Don't know if you took a look at that again, but maybe it's worth
looking at. Hmm, we do actually seem to be growing number of smp_mb*
calls in core kernel too.

^ permalink raw reply

* Endian/__BYTE_ORDER question
From: Joakim Tjernlund @ 2010-02-11 16:17 UTC (permalink / raw)
  To: linuxppc-dev


I am getting confused about on how to test for Endian in the kernel code. In user
space one uses #if __BYTE_ORDER == __LITTLE_ENDIAN or #if __BYTE_ORDER == __BIG_ENDIAN

I can see lots of kernel headers using this test too, but it doesn't seem
to be an arch specific file #defining __BYTE_ORDER. Instead I find files like:
 arch/alpha/math-emu/sfp-util.h
 arch/powerpc/include/asm/sfp-machine.h
 arch/s390/include/asm/sfp-util.h
 arch/sh/math-emu/sfp-util.h

How is this supposed to work?

    Jocke

^ permalink raw reply

* Re: Endian/__BYTE_ORDER question
From: Joakim Tjernlund @ 2010-02-11 16:21 UTC (permalink / raw)
  Cc: linuxppc-dev
In-Reply-To: <OF43B135B0.EF8D9CB9-ONC12576C7.00588245-C12576C7.00598135@transmode.se>

>
> I am getting confused about on how to test for Endian in the kernel code. In user
> space one uses #if __BYTE_ORDER == __LITTLE_ENDIAN or #if __BYTE_ORDER == __BIG_ENDIAN
>
> I can see lots of kernel headers using this test too, but it doesn't seem
> to be an arch specific file #defining __BYTE_ORDER. Instead I find files like:
>  arch/alpha/math-emu/sfp-util.h
>  arch/powerpc/include/asm/sfp-machine.h
>  arch/s390/include/asm/sfp-util.h
>  arch/sh/math-emu/sfp-util.h
>
> How is this supposed to work?
>
>     Jocke

Figured I should mention that a simple:
#include <asm/byteorder.h>

#ifndef __BYTE_ORDER
# error __BYTE_ORDER is not defined
#endif

Doesn't work.

    Jocke

^ permalink raw reply

* Re: Endian/__BYTE_ORDER question
From: Andreas Schwab @ 2010-02-11 16:35 UTC (permalink / raw)
  To: Joakim Tjernlund; +Cc: linuxppc-dev
In-Reply-To: <OF43B135B0.EF8D9CB9-ONC12576C7.00588245-C12576C7.00598135@transmode.se>

Joakim Tjernlund <joakim.tjernlund@transmode.se> writes:

> I am getting confused about on how to test for Endian in the kernel code. In user
> space one uses #if __BYTE_ORDER == __LITTLE_ENDIAN or #if __BYTE_ORDER == __BIG_ENDIAN
>
> I can see lots of kernel headers using this test too

Only outside of __KERNEL__.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply

* Re: Endian/__BYTE_ORDER question
From: Adrian Reber @ 2010-02-11 16:33 UTC (permalink / raw)
  To: Joakim Tjernlund; +Cc: linuxppc-dev
In-Reply-To: <OF43B135B0.EF8D9CB9-ONC12576C7.00588245-C12576C7.00598135@transmode.se>

On Thu, Feb 11, 2010 at 05:17:37PM +0100, Joakim Tjernlund wrote:
> I am getting confused about on how to test for Endian in the kernel code. In user
> space one uses #if __BYTE_ORDER == __LITTLE_ENDIAN or #if __BYTE_ORDER == __BIG_ENDIAN
> 
> I can see lots of kernel headers using this test too, but it doesn't seem
> to be an arch specific file #defining __BYTE_ORDER. Instead I find files like:
>  arch/alpha/math-emu/sfp-util.h
>  arch/powerpc/include/asm/sfp-machine.h
>  arch/s390/include/asm/sfp-util.h
>  arch/sh/math-emu/sfp-util.h
> 
> How is this supposed to work?

I have no idea how it is actually done in the kernel code... but gcc
defines it:

gcc -dM -E -x c - <<<'' | grep ENDIAN
#define __BIG_ENDIAN__ 1
#define _BIG_ENDIAN 1

		Adrian

^ permalink raw reply

* Re: Endian/__BYTE_ORDER question
From: Joakim Tjernlund @ 2010-02-11 16:52 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: linuxppc-dev
In-Reply-To: <m2sk977ms5.fsf@igel.home>

Andreas Schwab <schwab@linux-m68k.org> wrote on 2010/02/11 17:35:54:
>
> Joakim Tjernlund <joakim.tjernlund@transmode.se> writes:
>
> > I am getting confused about on how to test for Endian in the kernel code. In user
> > space one uses #if __BYTE_ORDER == __LITTLE_ENDIAN or #if __BYTE_ORDER == __BIG_ENDIAN
> >
> > I can see lots of kernel headers using this test too
>
> Only outside of __KERNEL__.

Ah, yes now I see. This is unfortunate as if one moves kernel
code to user space and still use the kernel way:
 #ifdef __LITTLE_ENDIAN
one will be in big trouble as including stdlib.h will define
both __LITTLE_ENDIAN and __BIG_ENDIAN. Spent quite a while
chasing that one down :(

      Jocke

^ permalink raw reply

* Re: [PATCH] perf_event: e500 support
From: Scott Wood @ 2010-02-11 16:56 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: linuxppc-dev
In-Reply-To: <20100211030143.GA11245@brick.ozlabs.ibm.com>

Paul Mackerras wrote:
> On Wed, Feb 10, 2010 at 06:06:10PM -0600, Scott Wood wrote:
> 
>> Paul Mackerras wrote:
>>>> Some limitations:
>>>> - No threshold support -- need to figure out how to represent it in
>>>>  the event struct from userspace.
>>> What does "threshold support" mean in this context?  Does it mean
>>> something different from getting an interrupt after N events have been
>>> counted?  Or does it mean counting instances where something takes
>>> longer than a specific number of cycles?
>> The latter.
> 
> OK.  I handled that on classic by using some extra high bits in the
> event config for the threshold value. 

OK.

> If you have a single threshold
> value in hardware but more than one event that uses that threshold
> value, then you will need to add a constraint that all threshold
> events have to specify the same threshold.

There's a separate threshold for each counter.

> So, it sounds like you have a class of events which are the
> thresholding events, and two constraints:
> 
> * at most two events in the thresholding event class
> * at most four events in total
> 
> Are there other constraints?  Apart from the thresholding events, can
> any event go on any counter, or can some events only be counted on one
> particular counter?

No, those are the only constraints.

> If your constraints are just the two listed above (<= 2 threshold
> events, <= 4 events total), then doing it the obvious straightforward
> way is fine.  If there are other constraints as well, such as certain
> events only being available on one specific PMC, then you should
> consider reusing the constraint checking machinery from ppc64.

I'll stick with the straightforward approach then.  If future chips have 
more complicated constraints we can revisit using the more general scheme.

-Scott

^ permalink raw reply

* Re: Endian/__BYTE_ORDER question
From: Joakim Tjernlund @ 2010-02-11 16:55 UTC (permalink / raw)
  To: Adrian Reber; +Cc: linuxppc-dev
In-Reply-To: <20100211163328.GC16348@lisas.de>

Adrian Reber <adrian@lisas.de> wrote on 2010/02/11 17:33:29:
>
> On Thu, Feb 11, 2010 at 05:17:37PM +0100, Joakim Tjernlund wrote:
> > I am getting confused about on how to test for Endian in the kernel code. In user
> > space one uses #if __BYTE_ORDER == __LITTLE_ENDIAN or #if __BYTE_ORDER == __BIG_ENDIAN
> >
> > I can see lots of kernel headers using this test too, but it doesn't seem
> > to be an arch specific file #defining __BYTE_ORDER. Instead I find files like:
> >  arch/alpha/math-emu/sfp-util.h
> >  arch/powerpc/include/asm/sfp-machine.h
> >  arch/s390/include/asm/sfp-util.h
> >  arch/sh/math-emu/sfp-util.h
> >
> > How is this supposed to work?
>
> I have no idea how it is actually done in the kernel code... but gcc
> defines it:
>
> gcc -dM -E -x c - <<<'' | grep ENDIAN
> #define __BIG_ENDIAN__ 1
> #define _BIG_ENDIAN 1

That doesn't define __BYTE_ORDER. Try the same gcc command
on a file that #includes <stdlib.h> and you will get both
__BIG_ENDIAN and __LITTLE_ENDIAN

     Jocke

^ permalink raw reply

* TR: Linux 2.6.x 82xx probe devices with multiple serial drivers
From: Laurent Lagrange @ 2010-02-11 18:39 UTC (permalink / raw)
  To: linuxppc-dev


Hello,

I use a 82xx platform with a device tree which describes 4 SCC ports.
I want to probe these 4 SCC ports against 2 serial drivers (e.g. async and
sync).
Each port is declared as compatible with the 2 drivers.

When the first driver is loaded, all devices are probed and bound
with this driver and cannot be probed against the second driver.

I know that it can be a way to prevent against concurrent accesses but the
configuration is static and depends on how the drivers are loaded.

In fact, I want to load my 2 drivers, probe each entry without requesting
any resource then select how to open my ports using the first or second
driver.

Any tips ?
Thanks
Laurent

^ permalink raw reply

* [PATCH] [V4] net: emaclite: adding MDIO and phy lib support
From: John Linn @ 2010-02-11 20:52 UTC (permalink / raw)
  To: netdev, linuxppc-dev, jgarzik, grant.likely, jwboyer
  Cc: John Linn, john.williams, Sadanand Mutyala

These changes add MDIO and phy lib support to the driver as the
IP core now supports the MDIO bus.

The MDIO bus and phy are added as a child to the emaclite in the device
tree as illustrated below.

mdio {
	#address-cells = <1>;
	#size-cells = <0>;
	phy0: phy@7 {
		compatible = "marvell,88e1111";
		reg = <7>;
	} ;
}

Signed-off-by: Sadanand Mutyala <Sadanand.Mutyala@xilinx.com>
Signed-off-by: John Linn <john.linn@xilinx.com>

---

V2 - updated it for Grant's comments, except I couldn't find any tabs
converted to white space issue, let's see if V2 has it also

V3 - updated it for Grant's comments, aded mutex release when a timeout
happens, and added Grant's acked by.

V4 - removed the mutex as I realized the higher layer mdio calls already
use a mutex and there are no internal calls to the driver except in open.
Didn't integrate John W comments since releasing mutex wasn't needed.
---
 drivers/net/Kconfig           |    1 +
 drivers/net/xilinx_emaclite.c |  381 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 339 insertions(+), 43 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index dd9a09c..9509a36 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -1939,6 +1939,7 @@ config ATL2
 config XILINX_EMACLITE
 	tristate "Xilinx 10/100 Ethernet Lite support"
 	depends on PPC32 || MICROBLAZE
+	select PHYLIB
 	help
 	  This driver supports the 10/100 Ethernet Lite from Xilinx.
 
diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
index 8c777ba..d294ec5 100644
--- a/drivers/net/xilinx_emaclite.c
+++ b/drivers/net/xilinx_emaclite.c
@@ -22,11 +22,17 @@
 
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
 
 #define DRIVER_NAME "xilinx_emaclite"
 
 /* Register offsets for the EmacLite Core */
 #define XEL_TXBUFF_OFFSET 	0x0		/* Transmit Buffer */
+#define XEL_MDIOADDR_OFFSET	0x07E4		/* MDIO Address Register */
+#define XEL_MDIOWR_OFFSET	0x07E8		/* MDIO Write Data Register */
+#define XEL_MDIORD_OFFSET	0x07EC		/* MDIO Read Data Register */
+#define XEL_MDIOCTRL_OFFSET	0x07F0		/* MDIO Control Register */
 #define XEL_GIER_OFFSET		0x07F8		/* GIE Register */
 #define XEL_TSR_OFFSET		0x07FC		/* Tx status */
 #define XEL_TPLR_OFFSET		0x07F4		/* Tx packet length */
@@ -37,6 +43,22 @@
 
 #define XEL_BUFFER_OFFSET	0x0800		/* Next Tx/Rx buffer's offset */
 
+/* MDIO Address Register Bit Masks */
+#define XEL_MDIOADDR_REGADR_MASK  0x0000001F	/* Register Address */
+#define XEL_MDIOADDR_PHYADR_MASK  0x000003E0	/* PHY Address */
+#define XEL_MDIOADDR_PHYADR_SHIFT 5
+#define XEL_MDIOADDR_OP_MASK	  0x00000400	/* RD/WR Operation */
+
+/* MDIO Write Data Register Bit Masks */
+#define XEL_MDIOWR_WRDATA_MASK	  0x0000FFFF	/* Data to be Written */
+
+/* MDIO Read Data Register Bit Masks */
+#define XEL_MDIORD_RDDATA_MASK	  0x0000FFFF	/* Data to be Read */
+
+/* MDIO Control Register Bit Masks */
+#define XEL_MDIOCTRL_MDIOSTS_MASK 0x00000001	/* MDIO Status Mask */
+#define XEL_MDIOCTRL_MDIOEN_MASK  0x00000008	/* MDIO Enable */
+
 /* Global Interrupt Enable Register (GIER) Bit Masks */
 #define XEL_GIER_GIE_MASK	0x80000000 	/* Global Enable */
 
@@ -87,6 +109,12 @@
  * @reset_lock:		lock used for synchronization
  * @deferred_skb:	holds an skb (for transmission at a later time) when the
  *			Tx buffer is not free
+ * @phy_dev:		pointer to the PHY device
+ * @phy_node:		pointer to the PHY device node
+ * @mii_bus:		pointer to the MII bus
+ * @mdio_irqs:		IRQs table for MDIO bus
+ * @last_link:		last link status
+ * @has_mdio:		indicates whether MDIO is included in the HW
  */
 struct net_local {
 
@@ -100,6 +128,15 @@ struct net_local {
 
 	spinlock_t reset_lock;
 	struct sk_buff *deferred_skb;
+
+	struct phy_device *phy_dev;
+	struct device_node *phy_node;
+
+	struct mii_bus *mii_bus;
+	int mdio_irqs[PHY_MAX_ADDR];
+
+	int last_link;
+	bool has_mdio;
 };
 
 
@@ -431,7 +468,7 @@ static u16 xemaclite_recv_data(struct net_local *drvdata, u8 *data)
 }
 
 /**
- * xemaclite_set_mac_address - Set the MAC address for this device
+ * xemaclite_update_address - Update the MAC address in the device
  * @drvdata:	Pointer to the Emaclite device private data
  * @address_ptr:Pointer to the MAC address (MAC address is a 48-bit value)
  *
@@ -441,8 +478,8 @@ static u16 xemaclite_recv_data(struct net_local *drvdata, u8 *data)
  * The MAC address can be programmed using any of the two transmit
  * buffers (if configured).
  */
-static void xemaclite_set_mac_address(struct net_local *drvdata,
-				      u8 *address_ptr)
+static void xemaclite_update_address(struct net_local *drvdata,
+				     u8 *address_ptr)
 {
 	void __iomem *addr;
 	u32 reg_data;
@@ -465,6 +502,30 @@ static void xemaclite_set_mac_address(struct net_local *drvdata,
 }
 
 /**
+ * xemaclite_set_mac_address - Set the MAC address for this device
+ * @dev:	Pointer to the network device instance
+ * @addr:	Void pointer to the sockaddr structure
+ *
+ * This function copies the HW address from the sockaddr strucutre to the
+ * net_device structure and updates the address in HW.
+ *
+ * Return:	Error if the net device is busy or 0 if the addr is set
+ *		successfully
+ */
+static int xemaclite_set_mac_address(struct net_device *dev, void *address)
+{
+	struct net_local *lp = (struct net_local *) netdev_priv(dev);
+	struct sockaddr *addr = address;
+
+	if (netif_running(dev))
+		return -EBUSY;
+
+	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
+	xemaclite_update_address(lp, dev->dev_addr);
+	return 0;
+}
+
+/**
  * xemaclite_tx_timeout - Callback for Tx Timeout
  * @dev:	Pointer to the network device
  *
@@ -641,12 +702,219 @@ static irqreturn_t xemaclite_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/**********************/
+/* MDIO Bus functions */
+/**********************/
+
+/**
+ * xemaclite_mdio_wait - Wait for the MDIO to be ready to use
+ * @lp:		Pointer to the Emaclite device private data
+ *
+ * This function waits till the device is ready to accept a new MDIO
+ * request.
+ *
+ * Return:	0 for success or ETIMEDOUT for a timeout
+ */
+
+static int xemaclite_mdio_wait(struct net_local *lp)
+{
+	long end = jiffies + 2;
+
+	/* wait for the MDIO interface to not be busy or timeout
+	   after some time.
+	*/
+	while (in_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET) &
+			XEL_MDIOCTRL_MDIOSTS_MASK) {
+		if (end - jiffies <= 0) {
+			WARN_ON(1);
+			return -ETIMEDOUT;
+		}
+		msleep(1);
+	}
+	return 0;
+}
+
+/**
+ * xemaclite_mdio_read - Read from a given MII management register
+ * @bus:	the mii_bus struct
+ * @phy_id:	the phy address
+ * @reg:	register number to read from
+ *
+ * This function waits till the device is ready to accept a new MDIO
+ * request and then writes the phy address to the MDIO Address register
+ * and reads data from MDIO Read Data register, when its available.
+ *
+ * Return:	Value read from the MII management register
+ */
+static int xemaclite_mdio_read(struct mii_bus *bus, int phy_id, int reg)
+{
+	struct net_local *lp = bus->priv;
+	u32 ctrl_reg;
+	u32 rc;
+
+	if (xemaclite_mdio_wait(lp))
+		return -ETIMEDOUT;
+
+	/* Write the PHY address, register number and set the OP bit in the
+	 * MDIO Address register. Set the Status bit in the MDIO Control
+	 * register to start a MDIO read transaction.
+	 */
+	ctrl_reg = in_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET);
+	out_be32(lp->base_addr + XEL_MDIOADDR_OFFSET,
+		 XEL_MDIOADDR_OP_MASK |
+		 ((phy_id << XEL_MDIOADDR_PHYADR_SHIFT) | reg));
+	out_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET,
+		 ctrl_reg | XEL_MDIOCTRL_MDIOSTS_MASK);
+
+	if (xemaclite_mdio_wait(lp))
+		return -ETIMEDOUT;
+
+	rc = in_be32(lp->base_addr + XEL_MDIORD_OFFSET);
+
+	dev_dbg(&lp->ndev->dev,
+		"xemaclite_mdio_read(phy_id=%i, reg=%x) == %x\n",
+		phy_id, reg, rc);
+
+	return rc;
+}
+
+/**
+ * xemaclite_mdio_write - Write to a given MII management register
+ * @bus:	the mii_bus struct
+ * @phy_id:	the phy address
+ * @reg:	register number to write to
+ * @val:	value to write to the register number specified by reg
+ *
+ * This fucntion waits till the device is ready to accept a new MDIO
+ * request and then writes the val to the MDIO Write Data register.
+ */
+static int xemaclite_mdio_write(struct mii_bus *bus, int phy_id, int reg,
+				u16 val)
+{
+	struct net_local *lp = bus->priv;
+	u32 ctrl_reg;
+
+	dev_dbg(&lp->ndev->dev,
+		"xemaclite_mdio_write(phy_id=%i, reg=%x, val=%x)\n",
+		phy_id, reg, val);
+
+	if (xemaclite_mdio_wait(lp))
+		return -ETIMEDOUT;
+
+	/* Write the PHY address, register number and clear the OP bit in the
+	 * MDIO Address register and then write the value into the MDIO Write
+	 * Data register. Finally, set the Status bit in the MDIO Control
+	 * register to start a MDIO write transaction.
+	 */
+	ctrl_reg = in_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET);
+	out_be32(lp->base_addr + XEL_MDIOADDR_OFFSET,
+		 ~XEL_MDIOADDR_OP_MASK &
+		 ((phy_id << XEL_MDIOADDR_PHYADR_SHIFT) | reg));
+	out_be32(lp->base_addr + XEL_MDIOWR_OFFSET, val);
+	out_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET,
+		 ctrl_reg | XEL_MDIOCTRL_MDIOSTS_MASK);
+
+	return 0;
+}
+
+/**
+ * xemaclite_mdio_reset - Reset the mdio bus.
+ * @bus:	Pointer to the MII bus
+ *
+ * This function is required(?) as per Documentation/networking/phy.txt.
+ * There is no reset in this device; this function always returns 0.
+ */
+static int xemaclite_mdio_reset(struct mii_bus *bus)
+{
+	return 0;
+}
+
+/**
+ * xemaclite_mdio_setup - Register mii_bus for the Emaclite device
+ * @lp:		Pointer to the Emaclite device private data
+ * @ofdev:	Pointer to OF device structure
+ *
+ * This function enables MDIO bus in the Emaclite device and registers a
+ * mii_bus.
+ *
+ * Return:	0 upon success or a negative error upon failure
+ */
+static int xemaclite_mdio_setup(struct net_local *lp, struct device *dev)
+{
+	struct mii_bus *bus;
+	int rc;
+	struct resource res;
+	struct device_node *np = of_get_parent(lp->phy_node);
+
+	/* Don't register the MDIO bus if the phy_node or its parent node
+	 * can't be found.
+	 */
+	if (!np)
+		return -ENODEV;
+
+	/* Enable the MDIO bus by asserting the enable bit in MDIO Control
+	 * register.
+	 */
+	out_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET,
+		 XEL_MDIOCTRL_MDIOEN_MASK);
+
+	bus = mdiobus_alloc();
+	if (!bus)
+		return -ENOMEM;
+
+	of_address_to_resource(np, 0, &res);
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%.8llx",
+		 (unsigned long long)res.start);
+	bus->priv = lp;
+	bus->name = "Xilinx Emaclite MDIO";
+	bus->read = xemaclite_mdio_read;
+	bus->write = xemaclite_mdio_write;
+	bus->reset = xemaclite_mdio_reset;
+	bus->parent = dev;
+	bus->irq = lp->mdio_irqs; /* preallocated IRQ table */
+
+	lp->mii_bus = bus;
+
+	rc = of_mdiobus_register(bus, np);
+	if (rc)
+		goto err_register;
+
+	return 0;
+
+err_register:
+	mdiobus_free(bus);
+	return rc;
+}
+
+/**
+ * xemaclite_adjust_link - Link state callback for the Emaclite device
+ * @ndev: pointer to net_device struct
+ *
+ * There's nothing in the Emaclite device to be configured when the link
+ * state changes. We just print the status.
+ */
+void xemaclite_adjust_link(struct net_device *ndev)
+{
+	struct net_local *lp = netdev_priv(ndev);
+	struct phy_device *phy = lp->phy_dev;
+	int link_state;
+
+	/* hash together the state values to decide if something has changed */
+	link_state = phy->speed | (phy->duplex << 1) | phy->link;
+
+	if (lp->last_link != link_state) {
+		lp->last_link = link_state;
+		phy_print_status(phy);
+	}
+}
+
 /**
  * xemaclite_open - Open the network device
  * @dev:	Pointer to the network device
  *
  * This function sets the MAC address, requests an IRQ and enables interrupts
  * for the Emaclite device and starts the Tx queue.
+ * It also connects to the phy device, if MDIO is included in Emaclite device.
  */
 static int xemaclite_open(struct net_device *dev)
 {
@@ -656,14 +924,50 @@ static int xemaclite_open(struct net_device *dev)
 	/* Just to be safe, stop the device first */
 	xemaclite_disable_interrupts(lp);
 
+	if (lp->phy_node) {
+		u32 bmcr;
+
+		lp->phy_dev = of_phy_connect(lp->ndev, lp->phy_node,
+					     xemaclite_adjust_link, 0,
+					     PHY_INTERFACE_MODE_MII);
+		if (!lp->phy_dev) {
+			dev_err(&lp->ndev->dev, "of_phy_connect() failed\n");
+			return -ENODEV;
+		}
+
+		/* EmacLite doesn't support giga-bit speeds */
+		lp->phy_dev->supported &= (PHY_BASIC_FEATURES);
+		lp->phy_dev->advertising = lp->phy_dev->supported;
+
+		/* Don't advertise 1000BASE-T Full/Half duplex speeds */
+		xemaclite_mdio_write(lp->mii_bus, lp->phy_dev->addr,
+				     MII_CTRL1000, 0x00);
+		/* Advertise only 10 and 100mbps full/half duplex speeds */
+		xemaclite_mdio_write(lp->mii_bus, lp->phy_dev->addr,
+				     MII_ADVERTISE, ADVERTISE_ALL);
+
+		/* Restart auto negotiation */
+		bmcr = xemaclite_mdio_read(lp->mii_bus,
+					   lp->phy_dev->addr, MII_BMCR);
+		bmcr |= (BMCR_ANENABLE | BMCR_ANRESTART);
+		xemaclite_mdio_write(lp->mii_bus, lp->phy_dev->addr,
+				     MII_BMCR, bmcr);
+
+		phy_start(lp->phy_dev);
+	}
+
 	/* Set the MAC address each time opened */
-	xemaclite_set_mac_address(lp, dev->dev_addr);
+	xemaclite_update_address(lp, dev->dev_addr);
 
 	/* Grab the IRQ */
 	retval = request_irq(dev->irq, xemaclite_interrupt, 0, dev->name, dev);
 	if (retval) {
 		dev_err(&lp->ndev->dev, "Could not allocate interrupt %d\n",
 			dev->irq);
+		if (lp->phy_dev)
+			phy_disconnect(lp->phy_dev);
+		lp->phy_dev = NULL;
+
 		return retval;
 	}
 
@@ -682,6 +986,7 @@ static int xemaclite_open(struct net_device *dev)
  *
  * This function stops the Tx queue, disables interrupts and frees the IRQ for
  * the Emaclite device.
+ * It also disconnects the phy device associated with the Emaclite device.
  */
 static int xemaclite_close(struct net_device *dev)
 {
@@ -691,6 +996,10 @@ static int xemaclite_close(struct net_device *dev)
 	xemaclite_disable_interrupts(lp);
 	free_irq(dev->irq, dev);
 
+	if (lp->phy_dev)
+		phy_disconnect(lp->phy_dev);
+	lp->phy_dev = NULL;
+
 	return 0;
 }
 
@@ -754,42 +1063,6 @@ static int xemaclite_send(struct sk_buff *orig_skb, struct net_device *dev)
 }
 
 /**
- * xemaclite_ioctl - Perform IO Control operations on the network device
- * @dev:	Pointer to the network device
- * @rq:		Pointer to the interface request structure
- * @cmd:	IOCTL command
- *
- * The only IOCTL operation supported by this function is setting the MAC
- * address. An error is reported if any other operations are requested.
- *
- * Return:	0 to indicate success, or a negative error for failure.
- */
-static int xemaclite_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-{
-	struct net_local *lp = (struct net_local *) netdev_priv(dev);
-	struct hw_addr_data *hw_addr = (struct hw_addr_data *) &rq->ifr_hwaddr;
-
-	switch (cmd) {
-	case SIOCETHTOOL:
-		return -EIO;
-
-	case SIOCSIFHWADDR:
-		dev_err(&lp->ndev->dev, "SIOCSIFHWADDR\n");
-
-		/* Copy MAC address in from user space */
-		copy_from_user((void __force *) dev->dev_addr,
-			       (void __user __force *) hw_addr,
-			       IFHWADDRLEN);
-		xemaclite_set_mac_address(lp, dev->dev_addr);
-		break;
-	default:
-		return -EOPNOTSUPP;
-	}
-
-	return 0;
-}
-
-/**
  * xemaclite_remove_ndev - Free the network device
  * @ndev:	Pointer to the network device to be freed
  *
@@ -840,6 +1113,8 @@ static struct net_device_ops xemaclite_netdev_ops;
  * This function probes for the Emaclite device in the device tree.
  * It initializes the driver data structure and the hardware, sets the MAC
  * address and registers the network device.
+ * It also registers a mii_bus for the Emaclite device, if MDIO is included
+ * in the device.
  *
  * Return:	0, if the driver is bound to the Emaclite device, or
  *		a negative error if there is failure.
@@ -880,6 +1155,7 @@ static int __devinit xemaclite_of_probe(struct of_device *ofdev,
 	}
 
 	dev_set_drvdata(dev, ndev);
+	SET_NETDEV_DEV(ndev, &ofdev->dev);
 
 	ndev->irq = r_irq.start;
 	ndev->mem_start = r_mem.start;
@@ -923,7 +1199,12 @@ static int __devinit xemaclite_of_probe(struct of_device *ofdev,
 	out_be32(lp->base_addr + XEL_BUFFER_OFFSET + XEL_TSR_OFFSET, 0);
 
 	/* Set the MAC address in the EmacLite device */
-	xemaclite_set_mac_address(lp, ndev->dev_addr);
+	xemaclite_update_address(lp, ndev->dev_addr);
+
+	lp->phy_node = of_parse_phandle(ofdev->node, "phy-handle", 0);
+	rc = xemaclite_mdio_setup(lp, &ofdev->dev);
+	if (rc)
+		dev_warn(&ofdev->dev, "error registering MDIO bus\n");
 
 	dev_info(dev,
 		 "MAC address is now %2x:%2x:%2x:%2x:%2x:%2x\n",
@@ -972,12 +1253,25 @@ static int __devexit xemaclite_of_remove(struct of_device *of_dev)
 	struct device *dev = &of_dev->dev;
 	struct net_device *ndev = dev_get_drvdata(dev);
 
+	struct net_local *lp = (struct net_local *) netdev_priv(ndev);
+
+	/* Un-register the mii_bus, if configured */
+	if (lp->has_mdio) {
+		mdiobus_unregister(lp->mii_bus);
+		kfree(lp->mii_bus->irq);
+		mdiobus_free(lp->mii_bus);
+		lp->mii_bus = NULL;
+	}
+
 	unregister_netdev(ndev);
 
+	if (lp->phy_node)
+		of_node_put(lp->phy_node);
+	lp->phy_node = NULL;
+
 	release_mem_region(ndev->mem_start, ndev->mem_end-ndev->mem_start + 1);
 
 	xemaclite_remove_ndev(ndev);
-
 	dev_set_drvdata(dev, NULL);
 
 	return 0;
@@ -987,7 +1281,7 @@ static struct net_device_ops xemaclite_netdev_ops = {
 	.ndo_open		= xemaclite_open,
 	.ndo_stop		= xemaclite_close,
 	.ndo_start_xmit		= xemaclite_send,
-	.ndo_do_ioctl		= xemaclite_ioctl,
+	.ndo_set_mac_address	= xemaclite_set_mac_address,
 	.ndo_tx_timeout		= xemaclite_tx_timeout,
 	.ndo_get_stats		= xemaclite_get_stats,
 };
@@ -999,6 +1293,7 @@ static struct of_device_id xemaclite_of_match[] __devinitdata = {
 	{ .compatible = "xlnx,xps-ethernetlite-1.00.a", },
 	{ .compatible = "xlnx,xps-ethernetlite-2.00.a", },
 	{ .compatible = "xlnx,xps-ethernetlite-2.01.a", },
+	{ .compatible = "xlnx,xps-ethernetlite-3.00.a", },
 	{ /* end of list */ },
 };
 MODULE_DEVICE_TABLE(of, xemaclite_of_match);
-- 
1.6.2.1



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

^ permalink raw reply related

* Re: [PATCH] [V4] net: emaclite: adding MDIO and phy lib support
From: Grant Likely @ 2010-02-11 21:16 UTC (permalink / raw)
  To: John Linn; +Cc: Sadanand Mutyala, netdev, linuxppc-dev, jgarzik, john.williams
In-Reply-To: <e9ea9b59-9435-4712-a854-2c46046eb08e@SG2EHSMHS005.ehs.local>

On Thu, Feb 11, 2010 at 1:52 PM, John Linn <john.linn@xilinx.com> wrote:
> These changes add MDIO and phy lib support to the driver as the
> IP core now supports the MDIO bus.
>
> The MDIO bus and phy are added as a child to the emaclite in the device
> tree as illustrated below.
>
> mdio {
> =A0 =A0 =A0 =A0#address-cells =3D <1>;
> =A0 =A0 =A0 =A0#size-cells =3D <0>;
> =A0 =A0 =A0 =A0phy0: phy@7 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "marvell,88e1111";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <7>;
> =A0 =A0 =A0 =A0} ;
> }
>
> Signed-off-by: Sadanand Mutyala <Sadanand.Mutyala@xilinx.com>
> Signed-off-by: John Linn <john.linn@xilinx.com>
>
> ---
>
> V2 - updated it for Grant's comments, except I couldn't find any tabs
> converted to white space issue, let's see if V2 has it also
>
> V3 - updated it for Grant's comments, aded mutex release when a timeout
> happens, and added Grant's acked by.
>
> V4 - removed the mutex as I realized the higher layer mdio calls already
> use a mutex and there are no internal calls to the driver except in open.
> Didn't integrate John W comments since releasing mutex wasn't needed.
> ---
> =A0drivers/net/Kconfig =A0 =A0 =A0 =A0 =A0 | =A0 =A01 +
> =A0drivers/net/xilinx_emaclite.c | =A0381 +++++++++++++++++++++++++++++++=
+++++-----
> =A02 files changed, 339 insertions(+), 43 deletions(-)
[...]
> =A0/**
> =A0* xemaclite_open - Open the network device
> =A0* @dev: =A0 =A0 =A0 Pointer to the network device
> =A0*
> =A0* This function sets the MAC address, requests an IRQ and enables inte=
rrupts
> =A0* for the Emaclite device and starts the Tx queue.
> + * It also connects to the phy device, if MDIO is included in Emaclite d=
evice.
> =A0*/
> =A0static int xemaclite_open(struct net_device *dev)
> =A0{
> @@ -656,14 +924,50 @@ static int xemaclite_open(struct net_device *dev)
> =A0 =A0 =A0 =A0/* Just to be safe, stop the device first */
> =A0 =A0 =A0 =A0xemaclite_disable_interrupts(lp);
>
> + =A0 =A0 =A0 if (lp->phy_node) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 bmcr;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 lp->phy_dev =3D of_phy_connect(lp->ndev, lp=
->phy_node,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0xemaclite_adjust_link, 0,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0PHY_INTERFACE_MODE_MII);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!lp->phy_dev) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&lp->ndev->dev, "of=
_phy_connect() failed\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENODEV;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* EmacLite doesn't support giga-bit speeds=
 */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 lp->phy_dev->supported &=3D (PHY_BASIC_FEAT=
URES);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 lp->phy_dev->advertising =3D lp->phy_dev->s=
upported;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Don't advertise 1000BASE-T Full/Half dup=
lex speeds */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 xemaclite_mdio_write(lp->mii_bus, lp->phy_d=
ev->addr,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
MII_CTRL1000, 0x00);

Wait!  No.  This is wrong.  Sorry, I was all ready to ack this patch,
but I just realized what was bothering me (and I should have clued
into it earlier).  The driver cannot assume that the PHY is on an
xemaclite MDIO bus.  It might be on a GPIO driven MDIO bus.  Or
something else.  Calling xemaclite_mdio_{write,read} is not the right
thing to do here.

Instead, the driver should call the phy library function
phy_write(lp->phy_dev, MII_CTRL1000, 0);  The phylib code will route
it to the correct mdio bus.  It also takes care of grabbing the mutex
lock.

Make this fix, and then I can ack the driver.

Cheers,
g.


--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* RE: [PATCH] [V4] net: emaclite: adding MDIO and phy lib support
From: John Linn @ 2010-02-11 21:22 UTC (permalink / raw)
  To: Grant Likely
  Cc: linuxppc-dev, netdev, Sadanand Mutyala, jgarzik, john.williams
In-Reply-To: <fa686aa41002111316n13ce32bcyc8e5186c4776e1b4@mail.gmail.com>

> -----Original Message-----
> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Gra=
nt Likely
> Sent: Thursday, February 11, 2010 2:16 PM
> To: John Linn
> Cc: netdev@vger.kernel.org; linuxppc-dev@ozlabs.org; jgarzik@pobox.com; j=
wboyer@linux.vnet.ibm.com;
> john.williams@petalogix.com; Sadanand Mutyala
> Subject: Re: [PATCH] [V4] net: emaclite: adding MDIO and phy lib support
> =

> On Thu, Feb 11, 2010 at 1:52 PM, John Linn <john.linn@xilinx.com> wrote:
> > These changes add MDIO and phy lib support to the driver as the
> > IP core now supports the MDIO bus.
> >
> > The MDIO bus and phy are added as a child to the emaclite in the device=

> > tree as illustrated below.
> >
> > mdio {
> > =A0 =A0 =A0 =A0#address-cells =3D <1>;
> > =A0 =A0 =A0 =A0#size-cells =3D <0>;
> > =A0 =A0 =A0 =A0phy0: phy@7 {
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "marvell,88e1111";
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <7>;
> > =A0 =A0 =A0 =A0} ;
> > }
> >
> > Signed-off-by: Sadanand Mutyala <Sadanand.Mutyala@xilinx.com>
> > Signed-off-by: John Linn <john.linn@xilinx.com>
> >
> > ---
> >
> > V2 - updated it for Grant's comments, except I couldn't find any tabs
> > converted to white space issue, let's see if V2 has it also
> >
> > V3 - updated it for Grant's comments, aded mutex release when a timeout=

> > happens, and added Grant's acked by.
> >
> > V4 - removed the mutex as I realized the higher layer mdio calls alread=
y
> > use a mutex and there are no internal calls to the driver except in ope=
n.
> > Didn't integrate John W comments since releasing mutex wasn't needed.
> > ---
> > =A0drivers/net/Kconfig =A0 =A0 =A0 =A0 =A0 | =A0 =A01 +
> > =A0drivers/net/xilinx_emaclite.c | =A0381 +++++++++++++++++++++++++++++=
+++++++-----
> > =A02 files changed, 339 insertions(+), 43 deletions(-)
> [...]
> > =A0/**
> > =A0* xemaclite_open - Open the network device
> > =A0* @dev: =A0 =A0 =A0 Pointer to the network device
> > =A0*
> > =A0* This function sets the MAC address, requests an IRQ and enables in=
terrupts
> > =A0* for the Emaclite device and starts the Tx queue.
> > + * It also connects to the phy device, if MDIO is included in Emaclite=
 device.
> > =A0*/
> > =A0static int xemaclite_open(struct net_device *dev)
> > =A0{
> > @@ -656,14 +924,50 @@ static int xemaclite_open(struct net_device *dev)=

> > =A0 =A0 =A0 =A0/* Just to be safe, stop the device first */
> > =A0 =A0 =A0 =A0xemaclite_disable_interrupts(lp);
> >
> > + =A0 =A0 =A0 if (lp->phy_node) {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 bmcr;
> > +
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 lp->phy_dev =3D of_phy_connect(lp->ndev, =
lp->phy_node,
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0xemaclite_adjust_link, 0,
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0PHY_INTERFACE_MODE_MII);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!lp->phy_dev) {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&lp->ndev->dev, "=
of_phy_connect() failed\n");
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENODEV;
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> > +
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* EmacLite doesn't support giga-bit spee=
ds */
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 lp->phy_dev->supported &=3D (PHY_BASIC_FE=
ATURES);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 lp->phy_dev->advertising =3D lp->phy_dev-=
>supported;
> > +
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Don't advertise 1000BASE-T Full/Half d=
uplex speeds */
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 xemaclite_mdio_write(lp->mii_bus, lp->phy=
_dev->addr,
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0MII_CTRL1000, 0x00);
> =

> Wait!  No.  This is wrong.  Sorry, I was all ready to ack this patch,
> but I just realized what was bothering me (and I should have clued
> into it earlier).  The driver cannot assume that the PHY is on an
> xemaclite MDIO bus.  It might be on a GPIO driven MDIO bus.  Or
> something else.  Calling xemaclite_mdio_{write,read} is not the right
> thing to do here.
> =

> Instead, the driver should call the phy library function
> phy_write(lp->phy_dev, MII_CTRL1000, 0);  The phylib code will route
> it to the correct mdio bus.  It also takes care of grabbing the mutex
> lock.
> =

> Make this fix, and then I can ack the driver.
> =


Ok, no worries, one more spin, I'm getting pretty good at spinning it.

Will make those changes to the calls in open, retest and respin.

Thanks,
John

> Cheers,
> g.
> =

> =

> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.


This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.

^ permalink raw reply

* Re: Endian/__BYTE_ORDER question
From: Wolfgang Denk @ 2010-02-11 21:39 UTC (permalink / raw)
  To: Joakim Tjernlund; +Cc: linuxppc-dev
In-Reply-To: <OF918AA866.3ED427EB-ONC12576C7.005CBEE4-C12576C7.005CF730@transmode.se>

Dear Joakim Tjernlund,

In message <OF918AA866.3ED427EB-ONC12576C7.005CBEE4-C12576C7.005CF730@transmode.se> you wrote:
>
> > I have no idea how it is actually done in the kernel code... but gcc
> > defines it:
> >
> > gcc -dM -E -x c - <<<'' | grep ENDIAN
> > #define __BIG_ENDIAN__ 1
> > #define _BIG_ENDIAN 1
> 
> That doesn't define __BYTE_ORDER. Try the same gcc command
> on a file that #includes <stdlib.h> and you will get both
> __BIG_ENDIAN and __LITTLE_ENDIAN

For me this appears to work:

On x86:

	$ echo '#include <stdlib.h>' | gcc -dM -E -x c - | grep ENDIAN
	#define _ENDIAN_H 1
	#define PDP_ENDIAN __PDP_ENDIAN
	#define __PDP_ENDIAN 3412
	#define BIG_ENDIAN __BIG_ENDIAN
	#define __BYTE_ORDER __LITTLE_ENDIAN
	#define __LITTLE_ENDIAN 1234
	#define __BIG_ENDIAN 4321
	#define LITTLE_ENDIAN __LITTLE_ENDIAN

On PowerPC:

	$ echo '#include <stdlib.h>' | gcc -dM -E -x c - | grep ENDIAN
	#define __BIG_ENDIAN__ 1
	#define __PDP_ENDIAN 3412
	#define __LITTLE_ENDIAN 1234
	#define BIG_ENDIAN __BIG_ENDIAN
	#define _BIG_ENDIAN 1
	#define __BYTE_ORDER __BIG_ENDIAN
	#define _ENDIAN_H 1
	#define __BIG_ENDIAN 4321
	#define PDP_ENDIAN __PDP_ENDIAN
	#define LITTLE_ENDIAN __LITTLE_ENDIAN

In both cases __BYTE_ORDER is set to a sane value.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Copy from one, it's plagiarism; copy from two, it's research.

^ permalink raw reply

* [PATCH] [V5] net: emaclite: adding MDIO and phy lib support
From: John Linn @ 2010-02-11 22:12 UTC (permalink / raw)
  To: netdev, linuxppc-dev, jgarzik, grant.likely, jwboyer
  Cc: John Linn, john.williams, Sadanand Mutyala

These changes add MDIO and phy lib support to the driver as the
IP core now supports the MDIO bus.

The MDIO bus and phy are added as a child to the emaclite in the device
tree as illustrated below.

mdio {
	#address-cells = <1>;
	#size-cells = <0>;
	phy0: phy@7 {
		compatible = "marvell,88e1111";
		reg = <7>;
	} ;
}

Signed-off-by: Sadanand Mutyala <Sadanand.Mutyala@xilinx.com>
Signed-off-by: John Linn <john.linn@xilinx.com>

---

V2 - updated it for Grant's comments, except I couldn't find any tabs
converted to white space issue, let's see if V2 has it also

V3 - updated it for Grant's comments, aded mutex release when a timeout
happens, and added Grant's acked by.

V4 - removed the mutex as I realized the higher layer mdio calls already
use a mutex and there are no internal calls to the driver except in open.
Didn't integrate John W comments since releasing mutex wasn't needed.

V5 - changed calls to mdio functions in the driver to use the phy lib
functions so the correct mdio bus gets used, based on Grant's comments.
---
 drivers/net/Kconfig           |    1 +
 drivers/net/xilinx_emaclite.c |  378 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 336 insertions(+), 43 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index dd9a09c..9509a36 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -1939,6 +1939,7 @@ config ATL2
 config XILINX_EMACLITE
 	tristate "Xilinx 10/100 Ethernet Lite support"
 	depends on PPC32 || MICROBLAZE
+	select PHYLIB
 	help
 	  This driver supports the 10/100 Ethernet Lite from Xilinx.
 
diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
index 8c777ba..844da1e 100644
--- a/drivers/net/xilinx_emaclite.c
+++ b/drivers/net/xilinx_emaclite.c
@@ -22,11 +22,17 @@
 
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
 
 #define DRIVER_NAME "xilinx_emaclite"
 
 /* Register offsets for the EmacLite Core */
 #define XEL_TXBUFF_OFFSET 	0x0		/* Transmit Buffer */
+#define XEL_MDIOADDR_OFFSET	0x07E4		/* MDIO Address Register */
+#define XEL_MDIOWR_OFFSET	0x07E8		/* MDIO Write Data Register */
+#define XEL_MDIORD_OFFSET	0x07EC		/* MDIO Read Data Register */
+#define XEL_MDIOCTRL_OFFSET	0x07F0		/* MDIO Control Register */
 #define XEL_GIER_OFFSET		0x07F8		/* GIE Register */
 #define XEL_TSR_OFFSET		0x07FC		/* Tx status */
 #define XEL_TPLR_OFFSET		0x07F4		/* Tx packet length */
@@ -37,6 +43,22 @@
 
 #define XEL_BUFFER_OFFSET	0x0800		/* Next Tx/Rx buffer's offset */
 
+/* MDIO Address Register Bit Masks */
+#define XEL_MDIOADDR_REGADR_MASK  0x0000001F	/* Register Address */
+#define XEL_MDIOADDR_PHYADR_MASK  0x000003E0	/* PHY Address */
+#define XEL_MDIOADDR_PHYADR_SHIFT 5
+#define XEL_MDIOADDR_OP_MASK	  0x00000400	/* RD/WR Operation */
+
+/* MDIO Write Data Register Bit Masks */
+#define XEL_MDIOWR_WRDATA_MASK	  0x0000FFFF	/* Data to be Written */
+
+/* MDIO Read Data Register Bit Masks */
+#define XEL_MDIORD_RDDATA_MASK	  0x0000FFFF	/* Data to be Read */
+
+/* MDIO Control Register Bit Masks */
+#define XEL_MDIOCTRL_MDIOSTS_MASK 0x00000001	/* MDIO Status Mask */
+#define XEL_MDIOCTRL_MDIOEN_MASK  0x00000008	/* MDIO Enable */
+
 /* Global Interrupt Enable Register (GIER) Bit Masks */
 #define XEL_GIER_GIE_MASK	0x80000000 	/* Global Enable */
 
@@ -87,6 +109,12 @@
  * @reset_lock:		lock used for synchronization
  * @deferred_skb:	holds an skb (for transmission at a later time) when the
  *			Tx buffer is not free
+ * @phy_dev:		pointer to the PHY device
+ * @phy_node:		pointer to the PHY device node
+ * @mii_bus:		pointer to the MII bus
+ * @mdio_irqs:		IRQs table for MDIO bus
+ * @last_link:		last link status
+ * @has_mdio:		indicates whether MDIO is included in the HW
  */
 struct net_local {
 
@@ -100,6 +128,15 @@ struct net_local {
 
 	spinlock_t reset_lock;
 	struct sk_buff *deferred_skb;
+
+	struct phy_device *phy_dev;
+	struct device_node *phy_node;
+
+	struct mii_bus *mii_bus;
+	int mdio_irqs[PHY_MAX_ADDR];
+
+	int last_link;
+	bool has_mdio;
 };
 
 
@@ -431,7 +468,7 @@ static u16 xemaclite_recv_data(struct net_local *drvdata, u8 *data)
 }
 
 /**
- * xemaclite_set_mac_address - Set the MAC address for this device
+ * xemaclite_update_address - Update the MAC address in the device
  * @drvdata:	Pointer to the Emaclite device private data
  * @address_ptr:Pointer to the MAC address (MAC address is a 48-bit value)
  *
@@ -441,8 +478,8 @@ static u16 xemaclite_recv_data(struct net_local *drvdata, u8 *data)
  * The MAC address can be programmed using any of the two transmit
  * buffers (if configured).
  */
-static void xemaclite_set_mac_address(struct net_local *drvdata,
-				      u8 *address_ptr)
+static void xemaclite_update_address(struct net_local *drvdata,
+				     u8 *address_ptr)
 {
 	void __iomem *addr;
 	u32 reg_data;
@@ -465,6 +502,30 @@ static void xemaclite_set_mac_address(struct net_local *drvdata,
 }
 
 /**
+ * xemaclite_set_mac_address - Set the MAC address for this device
+ * @dev:	Pointer to the network device instance
+ * @addr:	Void pointer to the sockaddr structure
+ *
+ * This function copies the HW address from the sockaddr strucutre to the
+ * net_device structure and updates the address in HW.
+ *
+ * Return:	Error if the net device is busy or 0 if the addr is set
+ *		successfully
+ */
+static int xemaclite_set_mac_address(struct net_device *dev, void *address)
+{
+	struct net_local *lp = (struct net_local *) netdev_priv(dev);
+	struct sockaddr *addr = address;
+
+	if (netif_running(dev))
+		return -EBUSY;
+
+	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
+	xemaclite_update_address(lp, dev->dev_addr);
+	return 0;
+}
+
+/**
  * xemaclite_tx_timeout - Callback for Tx Timeout
  * @dev:	Pointer to the network device
  *
@@ -641,12 +702,219 @@ static irqreturn_t xemaclite_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/**********************/
+/* MDIO Bus functions */
+/**********************/
+
+/**
+ * xemaclite_mdio_wait - Wait for the MDIO to be ready to use
+ * @lp:		Pointer to the Emaclite device private data
+ *
+ * This function waits till the device is ready to accept a new MDIO
+ * request.
+ *
+ * Return:	0 for success or ETIMEDOUT for a timeout
+ */
+
+static int xemaclite_mdio_wait(struct net_local *lp)
+{
+	long end = jiffies + 2;
+
+	/* wait for the MDIO interface to not be busy or timeout
+	   after some time.
+	*/
+	while (in_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET) &
+			XEL_MDIOCTRL_MDIOSTS_MASK) {
+		if (end - jiffies <= 0) {
+			WARN_ON(1);
+			return -ETIMEDOUT;
+		}
+		msleep(1);
+	}
+	return 0;
+}
+
+/**
+ * xemaclite_mdio_read - Read from a given MII management register
+ * @bus:	the mii_bus struct
+ * @phy_id:	the phy address
+ * @reg:	register number to read from
+ *
+ * This function waits till the device is ready to accept a new MDIO
+ * request and then writes the phy address to the MDIO Address register
+ * and reads data from MDIO Read Data register, when its available.
+ *
+ * Return:	Value read from the MII management register
+ */
+static int xemaclite_mdio_read(struct mii_bus *bus, int phy_id, int reg)
+{
+	struct net_local *lp = bus->priv;
+	u32 ctrl_reg;
+	u32 rc;
+
+	if (xemaclite_mdio_wait(lp))
+		return -ETIMEDOUT;
+
+	/* Write the PHY address, register number and set the OP bit in the
+	 * MDIO Address register. Set the Status bit in the MDIO Control
+	 * register to start a MDIO read transaction.
+	 */
+	ctrl_reg = in_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET);
+	out_be32(lp->base_addr + XEL_MDIOADDR_OFFSET,
+		 XEL_MDIOADDR_OP_MASK |
+		 ((phy_id << XEL_MDIOADDR_PHYADR_SHIFT) | reg));
+	out_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET,
+		 ctrl_reg | XEL_MDIOCTRL_MDIOSTS_MASK);
+
+	if (xemaclite_mdio_wait(lp))
+		return -ETIMEDOUT;
+
+	rc = in_be32(lp->base_addr + XEL_MDIORD_OFFSET);
+
+	dev_dbg(&lp->ndev->dev,
+		"xemaclite_mdio_read(phy_id=%i, reg=%x) == %x\n",
+		phy_id, reg, rc);
+
+	return rc;
+}
+
+/**
+ * xemaclite_mdio_write - Write to a given MII management register
+ * @bus:	the mii_bus struct
+ * @phy_id:	the phy address
+ * @reg:	register number to write to
+ * @val:	value to write to the register number specified by reg
+ *
+ * This fucntion waits till the device is ready to accept a new MDIO
+ * request and then writes the val to the MDIO Write Data register.
+ */
+static int xemaclite_mdio_write(struct mii_bus *bus, int phy_id, int reg,
+				u16 val)
+{
+	struct net_local *lp = bus->priv;
+	u32 ctrl_reg;
+
+	dev_dbg(&lp->ndev->dev,
+		"xemaclite_mdio_write(phy_id=%i, reg=%x, val=%x)\n",
+		phy_id, reg, val);
+
+	if (xemaclite_mdio_wait(lp))
+		return -ETIMEDOUT;
+
+	/* Write the PHY address, register number and clear the OP bit in the
+	 * MDIO Address register and then write the value into the MDIO Write
+	 * Data register. Finally, set the Status bit in the MDIO Control
+	 * register to start a MDIO write transaction.
+	 */
+	ctrl_reg = in_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET);
+	out_be32(lp->base_addr + XEL_MDIOADDR_OFFSET,
+		 ~XEL_MDIOADDR_OP_MASK &
+		 ((phy_id << XEL_MDIOADDR_PHYADR_SHIFT) | reg));
+	out_be32(lp->base_addr + XEL_MDIOWR_OFFSET, val);
+	out_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET,
+		 ctrl_reg | XEL_MDIOCTRL_MDIOSTS_MASK);
+
+	return 0;
+}
+
+/**
+ * xemaclite_mdio_reset - Reset the mdio bus.
+ * @bus:	Pointer to the MII bus
+ *
+ * This function is required(?) as per Documentation/networking/phy.txt.
+ * There is no reset in this device; this function always returns 0.
+ */
+static int xemaclite_mdio_reset(struct mii_bus *bus)
+{
+	return 0;
+}
+
+/**
+ * xemaclite_mdio_setup - Register mii_bus for the Emaclite device
+ * @lp:		Pointer to the Emaclite device private data
+ * @ofdev:	Pointer to OF device structure
+ *
+ * This function enables MDIO bus in the Emaclite device and registers a
+ * mii_bus.
+ *
+ * Return:	0 upon success or a negative error upon failure
+ */
+static int xemaclite_mdio_setup(struct net_local *lp, struct device *dev)
+{
+	struct mii_bus *bus;
+	int rc;
+	struct resource res;
+	struct device_node *np = of_get_parent(lp->phy_node);
+
+	/* Don't register the MDIO bus if the phy_node or its parent node
+	 * can't be found.
+	 */
+	if (!np)
+		return -ENODEV;
+
+	/* Enable the MDIO bus by asserting the enable bit in MDIO Control
+	 * register.
+	 */
+	out_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET,
+		 XEL_MDIOCTRL_MDIOEN_MASK);
+
+	bus = mdiobus_alloc();
+	if (!bus)
+		return -ENOMEM;
+
+	of_address_to_resource(np, 0, &res);
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%.8llx",
+		 (unsigned long long)res.start);
+	bus->priv = lp;
+	bus->name = "Xilinx Emaclite MDIO";
+	bus->read = xemaclite_mdio_read;
+	bus->write = xemaclite_mdio_write;
+	bus->reset = xemaclite_mdio_reset;
+	bus->parent = dev;
+	bus->irq = lp->mdio_irqs; /* preallocated IRQ table */
+
+	lp->mii_bus = bus;
+
+	rc = of_mdiobus_register(bus, np);
+	if (rc)
+		goto err_register;
+
+	return 0;
+
+err_register:
+	mdiobus_free(bus);
+	return rc;
+}
+
+/**
+ * xemaclite_adjust_link - Link state callback for the Emaclite device
+ * @ndev: pointer to net_device struct
+ *
+ * There's nothing in the Emaclite device to be configured when the link
+ * state changes. We just print the status.
+ */
+void xemaclite_adjust_link(struct net_device *ndev)
+{
+	struct net_local *lp = netdev_priv(ndev);
+	struct phy_device *phy = lp->phy_dev;
+	int link_state;
+
+	/* hash together the state values to decide if something has changed */
+	link_state = phy->speed | (phy->duplex << 1) | phy->link;
+
+	if (lp->last_link != link_state) {
+		lp->last_link = link_state;
+		phy_print_status(phy);
+	}
+}
+
 /**
  * xemaclite_open - Open the network device
  * @dev:	Pointer to the network device
  *
  * This function sets the MAC address, requests an IRQ and enables interrupts
  * for the Emaclite device and starts the Tx queue.
+ * It also connects to the phy device, if MDIO is included in Emaclite device.
  */
 static int xemaclite_open(struct net_device *dev)
 {
@@ -656,14 +924,47 @@ static int xemaclite_open(struct net_device *dev)
 	/* Just to be safe, stop the device first */
 	xemaclite_disable_interrupts(lp);
 
+	if (lp->phy_node) {
+		u32 bmcr;
+
+		lp->phy_dev = of_phy_connect(lp->ndev, lp->phy_node,
+					     xemaclite_adjust_link, 0,
+					     PHY_INTERFACE_MODE_MII);
+		if (!lp->phy_dev) {
+			dev_err(&lp->ndev->dev, "of_phy_connect() failed\n");
+			return -ENODEV;
+		}
+
+		/* EmacLite doesn't support giga-bit speeds */
+		lp->phy_dev->supported &= (PHY_BASIC_FEATURES);
+		lp->phy_dev->advertising = lp->phy_dev->supported;
+
+		/* Don't advertise 1000BASE-T Full/Half duplex speeds */
+		phy_write(lp->phy_dev, MII_CTRL1000, 0);
+
+		/* Advertise only 10 and 100mbps full/half duplex speeds */
+		phy_write(lp->phy_dev, MII_ADVERTISE, ADVERTISE_ALL);
+
+		/* Restart auto negotiation */
+		bmcr = phy_read(lp->phy_dev, MII_BMCR);
+		bmcr |= (BMCR_ANENABLE | BMCR_ANRESTART);
+		phy_write(lp->phy_dev, MII_BMCR, bmcr);
+
+		phy_start(lp->phy_dev);
+	}
+
 	/* Set the MAC address each time opened */
-	xemaclite_set_mac_address(lp, dev->dev_addr);
+	xemaclite_update_address(lp, dev->dev_addr);
 
 	/* Grab the IRQ */
 	retval = request_irq(dev->irq, xemaclite_interrupt, 0, dev->name, dev);
 	if (retval) {
 		dev_err(&lp->ndev->dev, "Could not allocate interrupt %d\n",
 			dev->irq);
+		if (lp->phy_dev)
+			phy_disconnect(lp->phy_dev);
+		lp->phy_dev = NULL;
+
 		return retval;
 	}
 
@@ -682,6 +983,7 @@ static int xemaclite_open(struct net_device *dev)
  *
  * This function stops the Tx queue, disables interrupts and frees the IRQ for
  * the Emaclite device.
+ * It also disconnects the phy device associated with the Emaclite device.
  */
 static int xemaclite_close(struct net_device *dev)
 {
@@ -691,6 +993,10 @@ static int xemaclite_close(struct net_device *dev)
 	xemaclite_disable_interrupts(lp);
 	free_irq(dev->irq, dev);
 
+	if (lp->phy_dev)
+		phy_disconnect(lp->phy_dev);
+	lp->phy_dev = NULL;
+
 	return 0;
 }
 
@@ -754,42 +1060,6 @@ static int xemaclite_send(struct sk_buff *orig_skb, struct net_device *dev)
 }
 
 /**
- * xemaclite_ioctl - Perform IO Control operations on the network device
- * @dev:	Pointer to the network device
- * @rq:		Pointer to the interface request structure
- * @cmd:	IOCTL command
- *
- * The only IOCTL operation supported by this function is setting the MAC
- * address. An error is reported if any other operations are requested.
- *
- * Return:	0 to indicate success, or a negative error for failure.
- */
-static int xemaclite_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-{
-	struct net_local *lp = (struct net_local *) netdev_priv(dev);
-	struct hw_addr_data *hw_addr = (struct hw_addr_data *) &rq->ifr_hwaddr;
-
-	switch (cmd) {
-	case SIOCETHTOOL:
-		return -EIO;
-
-	case SIOCSIFHWADDR:
-		dev_err(&lp->ndev->dev, "SIOCSIFHWADDR\n");
-
-		/* Copy MAC address in from user space */
-		copy_from_user((void __force *) dev->dev_addr,
-			       (void __user __force *) hw_addr,
-			       IFHWADDRLEN);
-		xemaclite_set_mac_address(lp, dev->dev_addr);
-		break;
-	default:
-		return -EOPNOTSUPP;
-	}
-
-	return 0;
-}
-
-/**
  * xemaclite_remove_ndev - Free the network device
  * @ndev:	Pointer to the network device to be freed
  *
@@ -840,6 +1110,8 @@ static struct net_device_ops xemaclite_netdev_ops;
  * This function probes for the Emaclite device in the device tree.
  * It initializes the driver data structure and the hardware, sets the MAC
  * address and registers the network device.
+ * It also registers a mii_bus for the Emaclite device, if MDIO is included
+ * in the device.
  *
  * Return:	0, if the driver is bound to the Emaclite device, or
  *		a negative error if there is failure.
@@ -880,6 +1152,7 @@ static int __devinit xemaclite_of_probe(struct of_device *ofdev,
 	}
 
 	dev_set_drvdata(dev, ndev);
+	SET_NETDEV_DEV(ndev, &ofdev->dev);
 
 	ndev->irq = r_irq.start;
 	ndev->mem_start = r_mem.start;
@@ -923,7 +1196,12 @@ static int __devinit xemaclite_of_probe(struct of_device *ofdev,
 	out_be32(lp->base_addr + XEL_BUFFER_OFFSET + XEL_TSR_OFFSET, 0);
 
 	/* Set the MAC address in the EmacLite device */
-	xemaclite_set_mac_address(lp, ndev->dev_addr);
+	xemaclite_update_address(lp, ndev->dev_addr);
+
+	lp->phy_node = of_parse_phandle(ofdev->node, "phy-handle", 0);
+	rc = xemaclite_mdio_setup(lp, &ofdev->dev);
+	if (rc)
+		dev_warn(&ofdev->dev, "error registering MDIO bus\n");
 
 	dev_info(dev,
 		 "MAC address is now %2x:%2x:%2x:%2x:%2x:%2x\n",
@@ -972,12 +1250,25 @@ static int __devexit xemaclite_of_remove(struct of_device *of_dev)
 	struct device *dev = &of_dev->dev;
 	struct net_device *ndev = dev_get_drvdata(dev);
 
+	struct net_local *lp = (struct net_local *) netdev_priv(ndev);
+
+	/* Un-register the mii_bus, if configured */
+	if (lp->has_mdio) {
+		mdiobus_unregister(lp->mii_bus);
+		kfree(lp->mii_bus->irq);
+		mdiobus_free(lp->mii_bus);
+		lp->mii_bus = NULL;
+	}
+
 	unregister_netdev(ndev);
 
+	if (lp->phy_node)
+		of_node_put(lp->phy_node);
+	lp->phy_node = NULL;
+
 	release_mem_region(ndev->mem_start, ndev->mem_end-ndev->mem_start + 1);
 
 	xemaclite_remove_ndev(ndev);
-
 	dev_set_drvdata(dev, NULL);
 
 	return 0;
@@ -987,7 +1278,7 @@ static struct net_device_ops xemaclite_netdev_ops = {
 	.ndo_open		= xemaclite_open,
 	.ndo_stop		= xemaclite_close,
 	.ndo_start_xmit		= xemaclite_send,
-	.ndo_do_ioctl		= xemaclite_ioctl,
+	.ndo_set_mac_address	= xemaclite_set_mac_address,
 	.ndo_tx_timeout		= xemaclite_tx_timeout,
 	.ndo_get_stats		= xemaclite_get_stats,
 };
@@ -999,6 +1290,7 @@ static struct of_device_id xemaclite_of_match[] __devinitdata = {
 	{ .compatible = "xlnx,xps-ethernetlite-1.00.a", },
 	{ .compatible = "xlnx,xps-ethernetlite-2.00.a", },
 	{ .compatible = "xlnx,xps-ethernetlite-2.01.a", },
+	{ .compatible = "xlnx,xps-ethernetlite-3.00.a", },
 	{ /* end of list */ },
 };
 MODULE_DEVICE_TABLE(of, xemaclite_of_match);
-- 
1.6.2.1



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

^ permalink raw reply related

* Re: [PATCH] Restrict initial stack space expansion to rlimit
From: Helge Deller @ 2010-02-11 22:16 UTC (permalink / raw)
  To: Michael Neuling
  Cc: linux-parisc, stable, aeb, Oleg Nesterov, miltonm, James Morris,
	linuxppc-dev, Paul Mackerras, Anton Blanchard, KOSAKI Motohiro,
	Serge Hallyn, linux-fsdevel, Americo Wang, Andrew Morton,
	Linus Torvalds, Ingo Molnar, linux-kernel, Alexander Viro
In-Reply-To: <17323.1265779867@neuling.org>

On 02/10/2010 06:31 AM, Michael Neuling wrote:
> In message<20100210141016.4D18.A69D9226@jp.fujitsu.com>  you wrote:
>>> On 02/09/2010 10:51 PM, Michael Neuling wrote:
>>>>>> I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it
>>>>>> as well.
>>>>>
>>>>> There's only one CONFIG_GROWSUP arch - parisc.
>>>>> Could someone please test it on parisc?
>>>
>>> I did.
>>>
>>>> How about doing:
>>>>     'ulimit -s 15; ls'
>>>> before and after the patch is applied.  Before it's applied, 'ls' should
>>>> be killed.  After the patch is applied, 'ls' should no longer be killed.
>>>>
>>>> I'm suggesting a stack limit of 15KB since it's small enough to trigger
>>>> 20*PAGE_SIZE.  Also 15KB not a multiple of PAGE_SIZE, which is a trickier
>>>> case to handle correctly with this code.
>>>>
>>>> 4K pages on parisc should be fine to test with.
>>>
>>> Mikey, thanks for the suggested test plan.
>>>
>>> I'm not sure if your patch does it correct for parisc/stack-grows-up-case.
>>>
>>> I tested your patch on  a 4k pages kernel:
>>> root@c3000:~# uname -a
>>> Linux c3000 2.6.33-rc7-32bit #221 Tue Feb 9 23:17:06 CET 2010 parisc GNU/Li
> nux
>>>
>>> Without your patch:
>>> root@c3000:~# ulimit -s 15; ls
>>> Killed
>>> ->  correct.
>>>
>>> With your patch:
>>> root@c3000:~# ulimit -s 15; ls
>>> Killed
>>> _or_:
>>> root@c3000:~# ulimit -s 15; ls
>>> Segmentation fault
>>> ->  ??
>>>
>>> Any idea?
>>
>> My x86_64 box also makes segmentation fault. I think "ulimit -s 15" is too sm
> all stack for ls.
>> "ulimit -s 27; ls "  wroks perfectly fine.
>
> Arrh.  I asked Helge offline earlier to check what use to work on parisc
> on 2.6.31.
>
> I guess PPC has a nice clean non-bloated ABI :-D

Hi Mikey,

I tested again, and it works for me with "ulimit -s 27" as well (on a 4k, 32bit kernel).
Still, I'm not 100%  sure if your patch is correct.
Anyway, it seems to work.

But what makes me wonder is, why EXTRA_STACK_VM_PAGES is defined in pages at all.
You wrote in your patch description:
> This bug means that when limiting the stack to less the 20*PAGE_SIZE (eg.
> 80K on 4K pages or 'ulimit -s 79') all processes will be killed before
> they start.  This is particularly bad with 64K pages, where a ulimit below
> 1280K will kill every process.

Wouldn't it make sense to define and use EXTRA_STACK_VM_SIZE instead (e.g. as 20*4096 = 80k)?
This extra stack reservation should IMHO be independend of the actual kernel page size.

Helge

^ permalink raw reply

* Re: [PATCH] Restrict initial stack space expansion to rlimit
From: Michael Neuling @ 2010-02-11 22:22 UTC (permalink / raw)
  To: Helge Deller
  Cc: linux-parisc, stable, aeb, Oleg Nesterov, miltonm, James Morris,
	linuxppc-dev, Paul Mackerras, Anton Blanchard, KOSAKI Motohiro,
	Serge Hallyn, linux-fsdevel, Americo Wang, Andrew Morton,
	Linus Torvalds, Ingo Molnar, linux-kernel, Alexander Viro
In-Reply-To: <4B7481A6.7080300@gmx.de>

In message <4B7481A6.7080300@gmx.de> you wrote:
> On 02/10/2010 06:31 AM, Michael Neuling wrote:
> > In message<20100210141016.4D18.A69D9226@jp.fujitsu.com>  you wrote:
> >>> On 02/09/2010 10:51 PM, Michael Neuling wrote:
> >>>>>> I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it
> >>>>>> as well.
> >>>>>
> >>>>> There's only one CONFIG_GROWSUP arch - parisc.
> >>>>> Could someone please test it on parisc?
> >>>
> >>> I did.
> >>>
> >>>> How about doing:
> >>>>     'ulimit -s 15; ls'
> >>>> before and after the patch is applied.  Before it's applied, 'ls' should
> >>>> be killed.  After the patch is applied, 'ls' should no longer be killed.
> >>>>
> >>>> I'm suggesting a stack limit of 15KB since it's small enough to trigger
> >>>> 20*PAGE_SIZE.  Also 15KB not a multiple of PAGE_SIZE, which is a trickie
r
> >>>> case to handle correctly with this code.
> >>>>
> >>>> 4K pages on parisc should be fine to test with.
> >>>
> >>> Mikey, thanks for the suggested test plan.
> >>>
> >>> I'm not sure if your patch does it correct for parisc/stack-grows-up-case
.
> >>>
> >>> I tested your patch on  a 4k pages kernel:
> >>> root@c3000:~# uname -a
> >>> Linux c3000 2.6.33-rc7-32bit #221 Tue Feb 9 23:17:06 CET 2010 parisc GNU/
Li
> > nux
> >>>
> >>> Without your patch:
> >>> root@c3000:~# ulimit -s 15; ls
> >>> Killed
> >>> ->  correct.
> >>>
> >>> With your patch:
> >>> root@c3000:~# ulimit -s 15; ls
> >>> Killed
> >>> _or_:
> >>> root@c3000:~# ulimit -s 15; ls
> >>> Segmentation fault
> >>> ->  ??
> >>>
> >>> Any idea?
> >>
> >> My x86_64 box also makes segmentation fault. I think "ulimit -s 15" is too
 sm
> > all stack for ls.
> >> "ulimit -s 27; ls "  wroks perfectly fine.
> >
> > Arrh.  I asked Helge offline earlier to check what use to work on parisc
> > on 2.6.31.
> >
> > I guess PPC has a nice clean non-bloated ABI :-D
> 
> Hi Mikey,
> 
> I tested again, and it works for me with "ulimit -s 27" as well (on a
> 4k, 32bit kernel).
> Still, I'm not 100%  sure if your patch is correct.

Thanks for retesting

Did "ulimit -s 27" fail before you applied?

> Anyway, it seems to work.
> 
> But what makes me wonder is, why EXTRA_STACK_VM_PAGES is defined in pages at 
all.
> You wrote in your patch description:
> > This bug means that when limiting the stack to less the 20*PAGE_SIZE (eg.
> > 80K on 4K pages or 'ulimit -s 79') all processes will be killed before
> > they start.  This is particularly bad with 64K pages, where a ulimit below
> > 1280K will kill every process.
> 
> Wouldn't it make sense to define and use EXTRA_STACK_VM_SIZE instead
> (e.g. as 20*4096 = 80k)?  This extra stack reservation should IMHO be
> independend of the actual kernel page size.

If you look back through this thread, that has already been noted but
it's a separate issue to this bug, so that change will be deferred till
2.6.34.

Mikey

^ permalink raw reply

* Re: [PATCH] [V5] net: emaclite: adding MDIO and phy lib support
From: Grant Likely @ 2010-02-11 22:40 UTC (permalink / raw)
  To: John Linn; +Cc: Sadanand Mutyala, netdev, linuxppc-dev, jgarzik, john.williams
In-Reply-To: <38677d6a-3f33-483c-8fbe-5f5c46d70140@VA3EHSMHS027.ehs.local>

On Thu, Feb 11, 2010 at 3:12 PM, John Linn <john.linn@xilinx.com> wrote:
> These changes add MDIO and phy lib support to the driver as the
> IP core now supports the MDIO bus.
>
> The MDIO bus and phy are added as a child to the emaclite in the device
> tree as illustrated below.
>
> mdio {
> =A0 =A0 =A0 =A0#address-cells =3D <1>;
> =A0 =A0 =A0 =A0#size-cells =3D <0>;
> =A0 =A0 =A0 =A0phy0: phy@7 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "marvell,88e1111";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <7>;
> =A0 =A0 =A0 =A0} ;
> }
>
> Signed-off-by: Sadanand Mutyala <Sadanand.Mutyala@xilinx.com>
> Signed-off-by: John Linn <john.linn@xilinx.com>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

I've noticed one more problem below, but I'm okay with this being
merged as-is and fixed up with a follow-on patch later.

g.

> +/**
> + * xemaclite_mdio_setup - Register mii_bus for the Emaclite device
> + * @lp: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Pointer to the Emaclite device pr=
ivate data
> + * @ofdev: =A0 =A0 Pointer to OF device structure
> + *
> + * This function enables MDIO bus in the Emaclite device and registers a
> + * mii_bus.
> + *
> + * Return: =A0 =A0 0 upon success or a negative error upon failure
> + */
> +static int xemaclite_mdio_setup(struct net_local *lp, struct device *dev=
)
> +{
> + =A0 =A0 =A0 struct mii_bus *bus;
> + =A0 =A0 =A0 int rc;
> + =A0 =A0 =A0 struct resource res;
> + =A0 =A0 =A0 struct device_node *np =3D of_get_parent(lp->phy_node);
> +
> + =A0 =A0 =A0 /* Don't register the MDIO bus if the phy_node or its paren=
t node
> + =A0 =A0 =A0 =A0* can't be found.
> + =A0 =A0 =A0 =A0*/
> + =A0 =A0 =A0 if (!np)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENODEV;

This doesn't make sense.  The MDIO bus registration should not be
conditional on whether or not this particular xemaclite instance has a
phy on this particular bus.  Instead of following the mac
--(phandle)--> phy-device --(parent)--> mdio node linkage, this
function should look at the mac nodes children to find the mdio node.

In fact, now that I think about it, the code is actually dangerous,
because it may try and register a different mdio bus node as its own.

> +
> + =A0 =A0 =A0 /* Enable the MDIO bus by asserting the enable bit in MDIO =
Control
> + =A0 =A0 =A0 =A0* register.
> + =A0 =A0 =A0 =A0*/
> + =A0 =A0 =A0 out_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0XEL_MDIOCTRL_MDIOEN_MASK);
> +
> + =A0 =A0 =A0 bus =3D mdiobus_alloc();
> + =A0 =A0 =A0 if (!bus)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> +
> + =A0 =A0 =A0 of_address_to_resource(np, 0, &res);
> + =A0 =A0 =A0 snprintf(bus->id, MII_BUS_ID_SIZE, "%.8llx",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(unsigned long long)res.start);
> + =A0 =A0 =A0 bus->priv =3D lp;
> + =A0 =A0 =A0 bus->name =3D "Xilinx Emaclite MDIO";
> + =A0 =A0 =A0 bus->read =3D xemaclite_mdio_read;
> + =A0 =A0 =A0 bus->write =3D xemaclite_mdio_write;
> + =A0 =A0 =A0 bus->reset =3D xemaclite_mdio_reset;
> + =A0 =A0 =A0 bus->parent =3D dev;
> + =A0 =A0 =A0 bus->irq =3D lp->mdio_irqs; /* preallocated IRQ table */
> +
> + =A0 =A0 =A0 lp->mii_bus =3D bus;
> +
> + =A0 =A0 =A0 rc =3D of_mdiobus_register(bus, np);
> + =A0 =A0 =A0 if (rc)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err_register;
> +
> + =A0 =A0 =A0 return 0;
> +
> +err_register:
> + =A0 =A0 =A0 mdiobus_free(bus);
> + =A0 =A0 =A0 return rc;
> +}


--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* Re: Endian/__BYTE_ORDER question
From: Joakim Tjernlund @ 2010-02-11 23:09 UTC (permalink / raw)
  To: Wolfgang Denk; +Cc: linuxppc-dev
In-Reply-To: <20100211213900.CF5ECE8CCE4@gemini.denx.de>

Wolfgang Denk <wd@denx.de> wrote on 2010/02/11 22:39:00:
>
> Dear Joakim Tjernlund,
>
> In message <OF918AA866.3ED427EB-ONC12576C7.005CBEE4-C12576C7.
> 005CF730@transmode.se> you wrote:
> >
> > > I have no idea how it is actually done in the kernel code... but gcc
> > > defines it:
> > >
> > > gcc -dM -E -x c - <<<'' | grep ENDIAN
> > > #define __BIG_ENDIAN__ 1
> > > #define _BIG_ENDIAN 1
> >
> > That doesn't define __BYTE_ORDER. Try the same gcc command
> > on a file that #includes <stdlib.h> and you will get both
> > __BIG_ENDIAN and __LITTLE_ENDIAN
>
> For me this appears to work:
>
> On x86:
>
>    $ echo '#include <stdlib.h>' | gcc -dM -E -x c - | grep ENDIAN
>    #define _ENDIAN_H 1
>    #define PDP_ENDIAN __PDP_ENDIAN
>    #define __PDP_ENDIAN 3412
>    #define BIG_ENDIAN __BIG_ENDIAN
>    #define __BYTE_ORDER __LITTLE_ENDIAN
>    #define __LITTLE_ENDIAN 1234
>    #define __BIG_ENDIAN 4321
>    #define LITTLE_ENDIAN __LITTLE_ENDIAN
>
> On PowerPC:
>
>    $ echo '#include <stdlib.h>' | gcc -dM -E -x c - | grep ENDIAN
>    #define __BIG_ENDIAN__ 1
>    #define __PDP_ENDIAN 3412
>    #define __LITTLE_ENDIAN 1234
>    #define BIG_ENDIAN __BIG_ENDIAN
>    #define _BIG_ENDIAN 1
>    #define __BYTE_ORDER __BIG_ENDIAN
>    #define _ENDIAN_H 1
>    #define __BIG_ENDIAN 4321
>    #define PDP_ENDIAN __PDP_ENDIAN
>    #define LITTLE_ENDIAN __LITTLE_ENDIAN
>
> In both cases __BYTE_ORDER is set to a sane value.

Yes, but that was not what I meant. If you look closer you see that both
__BIG_ENDIAN and __LITTLE_ENDIAN are defined in each compiler. This means
you cannot use these two defines directly to tell if your arch is BE or LE.
Guess what the kernel uses :(

I think the kernel should #define __BYTE_ORDER properly so one
can use __BYTE_ORDER in the kernel too.
Just grep for __BYTE_ORDER in the linux tree to see some ugly
workarounds just because __BYTE_ORDER isn't defined.

 Jocke

^ permalink raw reply

* Re: Endian/__BYTE_ORDER question
From: Joakim Tjernlund @ 2010-02-12 10:48 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: linuxppc-dev, geert, Wolfgang Denk
In-Reply-To: <alpine.LRH.2.00.1002121132010.3816@mink.sonytel.be>

geert@sonytel.be wrote on 2010/02/12 11:33:02:
>
> On Fri, 12 Feb 2010, Joakim Tjernlund wrote:
> > Wolfgang Denk <wd@denx.de> wrote on 2010/02/11 22:39:00:
> > > Dear Joakim Tjernlund,
> > >
> > > In message <OF918AA866.3ED427EB-ONC12576C7.005CBEE4-C12576C7.
> > > 005CF730@transmode.se> you wrote:
> > > >
> > > > > I have no idea how it is actually done in the kernel code... but gcc
> > > > > defines it:
> > > > >
> > > > > gcc -dM -E -x c - <<<'' | grep ENDIAN
> > > > > #define __BIG_ENDIAN__ 1
> > > > > #define _BIG_ENDIAN 1
> > > >
> > > > That doesn't define __BYTE_ORDER. Try the same gcc command
> > > > on a file that #includes <stdlib.h> and you will get both
> > > > __BIG_ENDIAN and __LITTLE_ENDIAN
> > >
> > > For me this appears to work:
> > >
> > > On x86:
> > >
> > >    $ echo '#include <stdlib.h>' | gcc -dM -E -x c - | grep ENDIAN
> > >    #define _ENDIAN_H 1
> > >    #define PDP_ENDIAN __PDP_ENDIAN
> > >    #define __PDP_ENDIAN 3412
> > >    #define BIG_ENDIAN __BIG_ENDIAN
> > >    #define __BYTE_ORDER __LITTLE_ENDIAN
> > >    #define __LITTLE_ENDIAN 1234
> > >    #define __BIG_ENDIAN 4321
> > >    #define LITTLE_ENDIAN __LITTLE_ENDIAN
> > >
> > > On PowerPC:
> > >
> > >    $ echo '#include <stdlib.h>' | gcc -dM -E -x c - | grep ENDIAN
> > >    #define __BIG_ENDIAN__ 1
> > >    #define __PDP_ENDIAN 3412
> > >    #define __LITTLE_ENDIAN 1234
> > >    #define BIG_ENDIAN __BIG_ENDIAN
> > >    #define _BIG_ENDIAN 1
> > >    #define __BYTE_ORDER __BIG_ENDIAN
> > >    #define _ENDIAN_H 1
> > >    #define __BIG_ENDIAN 4321
> > >    #define PDP_ENDIAN __PDP_ENDIAN
> > >    #define LITTLE_ENDIAN __LITTLE_ENDIAN
> > >
> > > In both cases __BYTE_ORDER is set to a sane value.
> >
> > Yes, but that was not what I meant. If you look closer you see that both
> > __BIG_ENDIAN and __LITTLE_ENDIAN are defined in each compiler. This means
> > you cannot use these two defines directly to tell if your arch is BE or LE.
>
> The proper tests are:
>   - #if __BYTE_ORDER == __BIG_ENDIAN
>   - #if __BYTE_ORDER == __LITTLE_ENDIAN
> i.e. don't just test for the presence of __BIG_ENDIAN or __LITTLE_ENDIAN.

Yes, but my point is that __BYTE_ORDER isn't defined in the kernel so
there the correct test actually is to test __BIG_ENDIAN/__LITTLE_ENDIAN
directly. Look at lib/crc32.c, it also includes some test code
so one can verify the algo. If you enable the test code and move
it all to user space(some minor hacks needed to do that) one fail
the test on BE machines because both __BIG_ENDIAN and __LITTLE_ENDIAN
are defined in user space so the crc code thinks it still is LE.

There is some ugly code that messes with __BYTE_ORDER in the kernel,
all just because the kernel doesn't define __BYTE_ORDER. Is there
some rule that prevents the kernel to define __BYTE_ORDER?

    Jocke
   Jocke

^ permalink raw reply

* Re: Endian/__BYTE_ORDER question
From: Geert Uytterhoeven @ 2010-02-12 10:33 UTC (permalink / raw)
  To: Joakim Tjernlund; +Cc: linuxppc-dev, Wolfgang Denk
In-Reply-To: <OFC480F58E.3391EA2B-ONC12576C7.007E7F6B-C12576C7.007F3EE7@transmode.se>

On Fri, 12 Feb 2010, Joakim Tjernlund wrote:
> Wolfgang Denk <wd@denx.de> wrote on 2010/02/11 22:39:00:
> > Dear Joakim Tjernlund,
> >
> > In message <OF918AA866.3ED427EB-ONC12576C7.005CBEE4-C12576C7.
> > 005CF730@transmode.se> you wrote:
> > >
> > > > I have no idea how it is actually done in the kernel code... but gc=
c
> > > > defines it:
> > > >
> > > > gcc -dM -E -x c - <<<'' | grep ENDIAN
> > > > #define __BIG_ENDIAN__ 1
> > > > #define _BIG_ENDIAN 1
> > >
> > > That doesn't define __BYTE_ORDER. Try the same gcc command
> > > on a file that #includes <stdlib.h> and you will get both
> > > __BIG_ENDIAN and __LITTLE_ENDIAN
> >
> > For me this appears to work:
> >
> > On x86:
> >
> >    $ echo '#include <stdlib.h>' | gcc -dM -E -x c - | grep ENDIAN
> >    #define _ENDIAN_H 1
> >    #define PDP_ENDIAN __PDP_ENDIAN
> >    #define __PDP_ENDIAN 3412
> >    #define BIG_ENDIAN __BIG_ENDIAN
> >    #define __BYTE_ORDER __LITTLE_ENDIAN
> >    #define __LITTLE_ENDIAN 1234
> >    #define __BIG_ENDIAN 4321
> >    #define LITTLE_ENDIAN __LITTLE_ENDIAN
> >
> > On PowerPC:
> >
> >    $ echo '#include <stdlib.h>' | gcc -dM -E -x c - | grep ENDIAN
> >    #define __BIG_ENDIAN__ 1
> >    #define __PDP_ENDIAN 3412
> >    #define __LITTLE_ENDIAN 1234
> >    #define BIG_ENDIAN __BIG_ENDIAN
> >    #define _BIG_ENDIAN 1
> >    #define __BYTE_ORDER __BIG_ENDIAN
> >    #define _ENDIAN_H 1
> >    #define __BIG_ENDIAN 4321
> >    #define PDP_ENDIAN __PDP_ENDIAN
> >    #define LITTLE_ENDIAN __LITTLE_ENDIAN
> >
> > In both cases __BYTE_ORDER is set to a sane value.
>=20
> Yes, but that was not what I meant. If you look closer you see that both
> __BIG_ENDIAN and __LITTLE_ENDIAN are defined in each compiler. This means
> you cannot use these two defines directly to tell if your arch is BE or L=
E.

The proper tests are:
  - #if __BYTE_ORDER =3D=3D __BIG_ENDIAN
  - #if __BYTE_ORDER =3D=3D __LITTLE_ENDIAN
i.e. don't just test for the presence of __BIG_ENDIAN or __LITTLE_ENDIAN.

With kind regards,

Geert Uytterhoeven
Software Architect
Techsoft Centre

Technology and Software Centre Europe
The Corporate Village =B7 Da Vincilaan 7-D1 =B7 B-1935 Zaventem =B7 Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 =B7 RPR Brussels
Fortis =B7 BIC GEBABEBB =B7 IBAN BE41293037680010

^ permalink raw reply

* Please pull 'next' branch of 4xx tree
From: Josh Boyer @ 2010-02-12 13:02 UTC (permalink / raw)
  To: benh; +Cc: linuxppc-dev

Hi Ben,

Some updates for various boards from Stefan.  I don't have anything else at
the moment, so I thought I'd get this rolled into your tree now.

josh

The following changes since commit b919ee827e048826786fd7e889a2b04f63382fe6:
  Anton Blanchard (1):
        powerpc: Only print clockevent settings once

are available in the git repository at:

  ssh://master.kernel.org/pub/scm/linux/kernel/git/jwboyer/powerpc-4xx.git next

Stefan Roese (6):
      powerpc/44x: Fix L2-cache support for 460GT
      powerpc/44x: Add MTD support (NOR FLASH) to Katmai dts
      powerpc/44x: Update Arches dts
      powerpc/44x: Update Glacier dts
      powerpc/44x: Add MTD support to katmai defconfig
      powerpc/40x: Add support for PPC40x boards with > 512MB SDRAM

 arch/powerpc/boot/dts/arches.dts          |   12 ++++
 arch/powerpc/boot/dts/glacier.dts         |   76 ++++++++++++++++++++---
 arch/powerpc/boot/dts/katmai.dts          |   71 ++++++++++++++++------
 arch/powerpc/configs/44x/katmai_defconfig |   94 +++++++++++++++++++++++++++--
 arch/powerpc/mm/40x_mmu.c                 |    4 +-
 arch/powerpc/sysdev/ppc4xx_soc.c          |    3 +-
 6 files changed, 224 insertions(+), 36 deletions(-)

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox