* Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support
From: Stephen Boyd @ 2010-12-01 20:20 UTC (permalink / raw)
To: Daniel Walker, Tony Lindgren
Cc: Randy Dunlap, Mike Frysinger, Arnd Bergmann, Nicolas Pitre,
linux-arm-msm, Greg Kroah-Hartman, linux-kernel, FUJITA Tomonori,
Andrew Morton, linuxppc-dev, Alan Cox
In-Reply-To: <1291229696.14404.39.camel@c-dwalke-linux.qualcomm.com>
On 12/01/2010 10:54 AM, Daniel Walker wrote:
> Are you talking about __dcc_getstatus only? I don't think adding
> volatile is going to hurt anything, if not having it causes problems.
>
Just marking __dcc_getstatus volatile gives me
00000038 <hvc_dcc_get_chars>:
38: ee10fe11 mrc 14, 0, pc, cr0, cr1, {0}
3c: 1afffffd bne 38 <hvc_dcc_get_chars>
40: ee103e15 mrc 14, 0, r3, cr0, cr5, {0}
44: e3a00000 mov r0, #0 ; 0x0
48: e6ef3073 uxtb r3, r3
4c: ea000004 b 64 <hvc_dcc_get_chars+0x2c>
50: ee10ce11 mrc 14, 0, ip, cr0, cr1, {0}
54: e31c0101 tst ip, #1073741824 ; 0x40000000
58: 012fff1e bxeq lr
5c: e7c13000 strb r3, [r1, r0]
60: e2800001 add r0, r0, #1 ; 0x1
64: e1500002 cmp r0, r2
68: bafffff8 blt 50 <hvc_dcc_get_chars+0x18>
6c: e12fff1e bx lr
Seems the compiler keeps the value of __dcc_getchar() in r3 for the
duration of the loop. So we need to mark that one volatile too. I don't
think __dcc_putchar() needs to be marked volatile but it probably
doesn't hurt.
>
> We could maybe drop the looping for TX, but RX has no C based looping
> even tho for v7 it's recommended that we loop (presumably for v6 it's
> not recommended).
>
Definitely for TX since it seems like a redundant loop, but I agree RX
code has changed. Instead of
If RX buffer full
Poll for RX buffer full
Read character from RX buffer
we would have
If RX buffer full
Read character from RX buffer
which doesn't seem all that different assuming the RX buffer doesn't go
from full to empty between the If and Poll steps. Hopefully Tony knows more.
> Like this?
>
> for (i = 0; i < count; ++i) {
>
> if (__dcc_getstatus() & DCC_STATUS_RX)
> buf[i] = __dcc_getchar();
> else
> break;
> }
>
> It's a micro clean up I guess ..
Yes, it's much clearer that way.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply
* [PATCH 4/4] microblaze/of: Use generic rule to build dtb's
From: dirk.brandewie @ 2010-12-01 19:41 UTC (permalink / raw)
To: linux-serial
Cc: linux-arch, mmarek, microblaze-uclinux, devicetree-discuss,
sodaville, Dirk Brandewie, linuxppc-dev
In-Reply-To: <cover.1291231792.git.dirk.brandewie@gmail.com>
From: Dirk Brandewie <dirk.brandewie@gmail.com>
Modify arch/powerpc/boot/Makefile to use dtc command in
scripts/Makefile.lib
Signed-off-by: Dirk Brandewie <dirk.brandewie@gmail.com>
---
arch/microblaze/boot/Makefile | 10 ++--------
1 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/arch/microblaze/boot/Makefile b/arch/microblaze/boot/Makefile
index be01d78..52430e5 100644
--- a/arch/microblaze/boot/Makefile
+++ b/arch/microblaze/boot/Makefile
@@ -10,9 +10,6 @@ targets := linux.bin linux.bin.gz simpleImage.%
OBJCOPYFLAGS := -O binary
-# Where the DTS files live
-dtstree := $(srctree)/$(src)/dts
-
# Ensure system.dtb exists
$(obj)/linked_dtb.o: $(obj)/system.dtb
@@ -51,14 +48,11 @@ $(obj)/simpleImage.%: vmlinux FORCE
$(call if_changed,strip)
@echo 'Kernel: $@ is ready' ' (#'`cat .version`')'
-# Rule to build device tree blobs
-DTC = $(objtree)/scripts/dtc/dtc
# Rule to build device tree blobs
-quiet_cmd_dtc = DTC $@
- cmd_dtc = $(DTC) -O dtb -o $(obj)/$*.dtb -b 0 -p 1024 $(dtstree)/$*.dts
+DTC_FLAGS := -p 1024
-$(obj)/%.dtb: $(dtstree)/%.dts FORCE
+$(obj)/%.dtb: $(src)/dts/%.dts FORCE
$(call if_changed,dtc)
clean-files += *.dtb simpleImage.*.unstrip linux.bin.ub
--
1.7.2.3
^ permalink raw reply related
* [PATCH 3/4] of/powerpc: Use generic rule to build dtb's
From: dirk.brandewie @ 2010-12-01 19:41 UTC (permalink / raw)
To: linux-serial
Cc: linux-arch, mmarek, microblaze-uclinux, devicetree-discuss,
sodaville, Dirk Brandewie, linuxppc-dev
In-Reply-To: <cover.1291231792.git.dirk.brandewie@gmail.com>
From: Dirk Brandewie <dirk.brandewie@gmail.com>
Modify arch/powerpc/boot/Makefile to use dtc command in
scripts/Makefile.lib
Signed-off-by: Dirk Brandewie <dirk.brandewie@gmail.com>
---
arch/powerpc/boot/Makefile | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
index fae8192..3afb33a 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -35,7 +35,7 @@ endif
BOOTCFLAGS += -I$(obj) -I$(srctree)/$(obj)
-DTS_FLAGS ?= -p 1024
+DTC_FLAGS ?= -p 1024
$(obj)/4xx.o: BOOTCFLAGS += -mcpu=405
$(obj)/ebony.o: BOOTCFLAGS += -mcpu=405
@@ -332,10 +332,8 @@ $(obj)/treeImage.%: vmlinux $(obj)/%.dtb $(wrapperbits)
$(call if_changed,wrap,treeboot-$*,,$(obj)/$*.dtb)
# Rule to build device tree blobs
-DTC = $(objtree)/scripts/dtc/dtc
-
-$(obj)/%.dtb: $(dtstree)/%.dts
- $(DTC) -O dtb -o $(obj)/$*.dtb -b 0 $(DTS_FLAGS) $(dtstree)/$*.dts
+$(obj)/%.dtb: $(src)/dts/%.dts
+ $(call if_changed,dtc)
# If there isn't a platform selected then just strip the vmlinux.
ifeq (,$(image-y))
--
1.7.2.3
^ permalink raw reply related
* [PATCH 2/4] x86/of: Add building device tree blob(s) into image.
From: dirk.brandewie @ 2010-12-01 19:41 UTC (permalink / raw)
To: linux-serial
Cc: linux-arch, mmarek, microblaze-uclinux, devicetree-discuss,
sodaville, Dirk Brandewie, linuxppc-dev
In-Reply-To: <cover.1291231792.git.dirk.brandewie@gmail.com>
From: Dirk Brandewie <dirk.brandewie@gmail.com>
This patch adds linking device tree blob into vmlinux. DTB's are
added by adding the blob object name to list of objects to be linked
into the image.
Signed-off-by: Dirk Brandewie <dirk.brandewie@gmail.com>
---
arch/x86/platform/ce4100/Makefile | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/arch/x86/platform/ce4100/Makefile b/arch/x86/platform/ce4100/Makefile
index 91fc929..3b49187 100644
--- a/arch/x86/platform/ce4100/Makefile
+++ b/arch/x86/platform/ce4100/Makefile
@@ -1 +1,11 @@
obj-$(CONFIG_X86_INTEL_CE) += ce4100.o
+clean-files := *dtb.S
+
+ifdef CONFIG_X86_OF
+###
+# device tree blob
+obj-$(CONFIG_X86_INTEL_CE) += ce4100.dtb.o
+
+$(obj)/%.dtb: $(src)/%.dts
+ $(call if_changed,dtc)
+endif
--
1.7.2.3
^ permalink raw reply related
* [PATCH 1/4] of: Add support for linking device tree blobs into vmlinux
From: dirk.brandewie @ 2010-12-01 19:41 UTC (permalink / raw)
To: linux-serial
Cc: linux-arch, mmarek, microblaze-uclinux, devicetree-discuss,
sodaville, Dirk Brandewie, linuxppc-dev
In-Reply-To: <cover.1291231792.git.dirk.brandewie@gmail.com>
From: Dirk Brandewie <dirk.brandewie@gmail.com>
This patch adds support for linking device tree blob(s) into
vmlinux. Modifies asm-generic/vmlinux.lds.h to add linking
.dtb sections into vmlinux. To maintain compatiblity with the of/fdt
driver code platforms MUST copy the blob to a non-init memory location
before the kernel frees the .init.* sections in the image.
Modifies scripts/Makefile.lib to add a kbuild command to
compile DTS files to device tree blobs and a rule to create objects to
wrap the blobs for linking.
STRUCT_ALIGNMENT is defined in vmlinux.lds.h for use in the rule to
create wrapper objects for the dtb in Makefile.lib. The
STRUCT_ALIGN() macro in vmlinux.lds.h is modified to use the
STRUCT_ALIGNMENT definition.
The DTB's are placed on 32 byte boundries to allow parsing the blob
with driver/of/fdt.c during early boot without having to copy the blob
to get the structure alignment GCC expects.
A DTB is linked in by adding the DTB object to the list of objects to
be linked into vmlinux in the archtecture specific Makefile using
obj-y += foo.dtb.o
Signed-off-by: Dirk Brandewie <dirk.brandewie@gmail.com>
---
Documentation/kbuild/makefiles.txt | 15 +++++++++++++++
include/asm-generic/vmlinux.lds.h | 15 ++++++++++++---
scripts/Makefile.lib | 21 ++++++++++++++++++++-
3 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/Documentation/kbuild/makefiles.txt b/Documentation/kbuild/makefiles.txt
index 0ef00bd..fc18bb1 100644
--- a/Documentation/kbuild/makefiles.txt
+++ b/Documentation/kbuild/makefiles.txt
@@ -1136,6 +1136,21 @@ When kbuild executes, the following steps are followed (roughly):
resulting in the target file being recompiled for no
obvious reason.
+ dtc
+ Create flattend device tree blob object suitable for linking
+ into vmlinux. Device tree blobs linked into vmlinux are placed
+ in an init section in the image. Platform code *must* copy the
+ blob to non-init memory prior to calling unflatten_device_tree().
+
+ Example:
+ #arch/x86/platform/ce4100/Makefile
+ clean-files := *dtb.S
+
+ DTC_FLAGS := -p 1024
+ obj-y += foo.dtb.o
+
+ $(obj)/%.dtb: $(src)/%.dts
+ $(call if_changed,dtc)
--- 6.7 Custom kbuild commands
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index bd69d79..024d3b9 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -23,7 +23,7 @@
* _etext = .;
*
* _sdata = .;
- * RO_DATA_SECTION(PAGE_SIZE)
+* RO_DATA_SECTION(PAGE_SIZE)
* RW_DATA_SECTION(...)
* _edata = .;
*
@@ -67,7 +67,8 @@
* Align to a 32 byte boundary equal to the
* alignment gcc 4.5 uses for a struct
*/
-#define STRUCT_ALIGN() . = ALIGN(32)
+#define STRUCT_ALIGNMENT 32
+#define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
/* The actual configuration determine if the init/exit sections
* are handled as text/data or they can be discarded (which
@@ -146,6 +147,13 @@
#define TRACE_SYSCALLS()
#endif
+
+#define KERNEL_DTB() \
+ STRUCT_ALIGN(); \
+ VMLINUX_SYMBOL(__dtb_start) = .; \
+ *(.dtb.init.rodata) \
+ VMLINUX_SYMBOL(__dtb_end) = .;
+
/* .data section */
#define DATA_DATA \
*(.data) \
@@ -468,7 +476,8 @@
MCOUNT_REC() \
DEV_DISCARD(init.rodata) \
CPU_DISCARD(init.rodata) \
- MEM_DISCARD(init.rodata)
+ MEM_DISCARD(init.rodata) \
+ KERNEL_DTB()
#define INIT_TEXT \
*(.init.text) \
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 4c72c11..937eabbb 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -200,7 +200,26 @@ quiet_cmd_gzip = GZIP $@
cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
(rm -f $@ ; false)
-
+# DTC
+# ---------------------------------------------------------------------------
+
+# Generate an assembly file to wrap the output of the device tree compiler
+$(obj)/%.dtb.S: $(obj)/%.dtb
+ @echo '#include <asm-generic/vmlinux.lds.h>' > $@
+ @echo '.section .dtb.init.rodata,"a"' >> $@
+ @echo '.balign STRUCT_ALIGNMENT' >> $@
+ @echo '.global __dtb_$(*F)_begin' >> $@
+ @echo '__dtb_$(*F)_begin:' >> $@
+ @echo '.incbin "$<" ' >> $@
+ @echo '__dtb_$(*F)_end:' >> $@
+ @echo '.global __dtb_$(*F)_end' >> $@
+ @echo '.balign STRUCT_ALIGNMENT' >> $@
+
+DTC = $(objtree)/scripts/dtc/dtc
+
+quiet_cmd_dtc = DTC $@
+ cmd_dtc = $(DTC) -O dtb -o $@ -b 0 $(DTC_FLAGS) $<
+ooo
# Bzip2
# ---------------------------------------------------------------------------
--
1.7.2.3
^ permalink raw reply related
* [PATCH 0/4] V2 Add ability to link device blob(s) into vmlinux
From: dirk.brandewie @ 2010-12-01 19:41 UTC (permalink / raw)
To: linux-serial
Cc: linux-arch, mmarek, microblaze-uclinux, devicetree-discuss,
sodaville, Dirk Brandewie, linuxppc-dev
In-Reply-To: <cover.1289943240.git.dirk.brandewie@gmail.com>
From: Dirk Brandewie <dirk.brandewie@gmail.com>
This patch set adds the ability to link device tree blobs into
vmlinux.
Patch 1 implements the changes to include/asm-generic/vmlinux.lds.h and
adds a generic rule for generating DTB objects to be linked vmlinux.
Patch 2 implements linking a DTB into an x86 image.
Patch 3-4 move {powerpc,microblaze}/boot/Makefile to use the dtc rule
in patch 1.
This patch set has been tested on x86.
Powerpc and Microblaze have been compile tested with and without patch
3 and 4 applied.
Changes from V1:
Documentation added for dtc command in Makefile.lib to
Documentation/kbuild/makefiles.txt
Separate DTB_ALIGNMENT define removed.
FORCE removed from dtc rule.
Removed hardcoded path to dts files from dtc command.
Moved %.dtb: %.dts rule to arch specific makefiles.
Patch for adding kernel command line option to pass in dtb_compat
string dropped from this set will be submitted seperately.
Dirk Brandewie (4):
of: Add support for linking device tree blobs into vmlinux
x86/of: Add building device tree blob(s) into image.
of/powerpc: Use generic rule to build dtb's
microblaze/of: Use generic rule to build dtb's
Documentation/kbuild/makefiles.txt | 15 +++++++++++++++
arch/microblaze/boot/Makefile | 10 ++--------
arch/powerpc/boot/Makefile | 8 +++-----
arch/x86/platform/ce4100/Makefile | 10 ++++++++++
include/asm-generic/vmlinux.lds.h | 15 ++++++++++++---
scripts/Makefile.lib | 21 ++++++++++++++++++++-
6 files changed, 62 insertions(+), 17 deletions(-)
--
1.7.2.3
^ permalink raw reply
* Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support
From: Greg KH @ 2010-12-01 19:28 UTC (permalink / raw)
To: Daniel Walker
Cc: Randy Dunlap, Mike Frysinger, Arnd Bergmann, Nicolas Pitre,
Tony Lindgren, linux-arm-msm, Stephen Boyd, linux-kernel,
FUJITA Tomonori, Andrew Morton, linuxppc-dev, Alan Cox
In-Reply-To: <1291229696.14404.39.camel@c-dwalke-linux.qualcomm.com>
On Wed, Dec 01, 2010 at 10:54:56AM -0800, Daniel Walker wrote:
> On Tue, 2010-11-30 at 21:30 -0800, Stephen Boyd wrote:
> > On 11/30/2010 11:25 AM, Daniel Walker wrote:
> > > @@ -682,6 +682,15 @@ config HVC_UDBG
> > > select HVC_DRIVER
> > > default n
> > >
> > > +config HVC_DCC
> > > + bool "ARM JTAG DCC console"
> > > + depends on ARM
> > > + select HVC_DRIVER
> > > + help
> > > + This console uses the JTAG DCC on ARM to create a console under the HVC
> >
> > Looks like you added one too many spaces for indent here.
>
> The first line is fine, but the other two might need an extra space.
For this, or any other changes you want, I'll gladly take a follow-on
patch as this one is already in my tty-next tree.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support
From: Daniel Walker @ 2010-12-01 18:54 UTC (permalink / raw)
To: Stephen Boyd
Cc: Randy Dunlap, Mike Frysinger, Arnd Bergmann, Nicolas Pitre,
Tony Lindgren, linux-arm-msm, Greg Kroah-Hartman, linux-kernel,
FUJITA Tomonori, Andrew Morton, linuxppc-dev, Alan Cox
In-Reply-To: <4CF5DD63.40803@codeaurora.org>
On Tue, 2010-11-30 at 21:30 -0800, Stephen Boyd wrote:
> On 11/30/2010 11:25 AM, Daniel Walker wrote:
> > @@ -682,6 +682,15 @@ config HVC_UDBG
> > select HVC_DRIVER
> > default n
> >
> > +config HVC_DCC
> > + bool "ARM JTAG DCC console"
> > + depends on ARM
> > + select HVC_DRIVER
> > + help
> > + This console uses the JTAG DCC on ARM to create a console under the HVC
>
> Looks like you added one too many spaces for indent here.
The first line is fine, but the other two might need an extra space.
> > diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
> > new file mode 100644
> > index 0000000..6470f63
> > --- /dev/null
> > +++ b/drivers/char/hvc_dcc.c
> > +static inline u32 __dcc_getstatus(void)
> > +{
> > + u32 __ret;
> > +
> > + asm("mrc p14, 0, %0, c0, c1, 0 @ read comms ctrl reg"
> > + : "=r" (__ret) : : "cc");
> > +
> > + return __ret;
> > +}
>
> Without marking this asm volatile my compiler decides it can cache the
> value of __ret in a register and then check the value of it continually
> in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and
> fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).
>
>
> 00000000 <hvc_dcc_put_chars>:
> 0: ee103e11 mrc 14, 0, r3, cr0, cr1, {0}
> 4: e3a0c000 mov ip, #0 ; 0x0
> 8: e2033202 and r3, r3, #536870912 ; 0x20000000
> c: ea000006 b 2c <hvc_dcc_put_chars+0x2c>
> 10: e3530000 cmp r3, #0 ; 0x0
> 14: 1afffffd bne 10 <hvc_dcc_put_chars+0x10>
> 18: e7d1000c ldrb r0, [r1, ip]
> 1c: ee10fe11 mrc 14, 0, pc, cr0, cr1, {0}
> 20: 2afffffd bcs 1c <hvc_dcc_put_chars+0x1c>
> 24: ee000e15 mcr 14, 0, r0, cr0, cr5, {0}
> 28: e28cc001 add ip, ip, #1 ; 0x1
> 2c: e15c0002 cmp ip, r2
> 30: bafffff6 blt 10 <hvc_dcc_put_chars+0x10>
> 34: e1a00002 mov r0, r2
> 38: e12fff1e bx lr
>
> As you can see, the value of the mrc is checked against DCC_STATUS_TX
> (bit 29) and then stored in r3 for later use. Marking this volatile
> produces the following:
>
> 00000000 <hvc_dcc_put_chars>:
> 0: e3a03000 mov r3, #0 ; 0x0
> 4: ea000007 b 28 <hvc_dcc_put_chars+0x28>
> 8: ee100e11 mrc 14, 0, r0, cr0, cr1, {0}
> c: e3100202 tst r0, #536870912 ; 0x20000000
> 10: 1afffffc bne 8 <hvc_dcc_put_chars+0x8>
> 14: e7d10003 ldrb r0, [r1, r3]
> 18: ee10fe11 mrc 14, 0, pc, cr0, cr1, {0}
> 1c: 2afffffd bcs 18 <hvc_dcc_put_chars+0x18>
> 20: ee000e15 mcr 14, 0, r0, cr0, cr5, {0}
> 24: e2833001 add r3, r3, #1 ; 0x1
> 28: e1530002 cmp r3, r2
> 2c: bafffff5 blt 8 <hvc_dcc_put_chars+0x8>
> 30: e1a00002 mov r0, r2
> 34: e12fff1e bx lr
>
> which looks better.
>
> I marked all the asm in this driver as volatile. Is that correct?
Are you talking about __dcc_getstatus only? I don't think adding
volatile is going to hurt anything, if not having it causes problems.
> > +#if defined(CONFIG_CPU_V7)
> > +static inline char __dcc_getchar(void)
> > +{
> > + char __c;
> > +
> > + asm("get_wait: mrc p14, 0, pc, c0, c1, 0 \n\
> > + bne get_wait \n\
> > + mrc p14, 0, %0, c0, c5, 0 @ read comms data reg"
> > + : "=r" (__c) : : "cc");
> > +
> > + return __c;
> > +}
> > +#else
> > +static inline char __dcc_getchar(void)
> > +{
> > + char __c;
> > +
> > + asm("mrc p14, 0, %0, c0, c5, 0 @ read comms data reg"
> > + : "=r" (__c));
> > +
> > + return __c;
> > +}
> > +#endif
> > +
> > +#if defined(CONFIG_CPU_V7)
> > +static inline void __dcc_putchar(char c)
> > +{
> > + asm("put_wait: mrc p14, 0, pc, c0, c1, 0 \n\
> > + bcs put_wait \n\
> > + mcr p14, 0, %0, c0, c5, 0 "
> > + : : "r" (c) : "cc");
> > +}
> > +#else
> > +static inline void __dcc_putchar(char c)
> > +{
> > + asm("mcr p14, 0, %0, c0, c5, 0 @ write a char"
> > + : /* no output register */
> > + : "r" (c));
> > +}
> > +#endif
> > +
>
> I don't think both the v7 and v6 functions are necessary. It seems I can
> get away with just the second version of __dcc_(get|put)char() on a v7.
> The mrc p14, 0, pc, c0, c1, 0 will assign the top 4 bits (31-28) to the
> condition codes NZCV on v7. It also looks like on an ARM11 (a v6) will
> also do the same thing if I read the manuals right. The test in the
> inline assembly is saying, wait for a character to be ready or wait for
> a character to be read then actually write a character or read one. The
> code in hvc_dcc_put_chars() is already doing the same thing, albeit in a
> slightly different form. Instead of getting the status bits put into the
> condition codes and looping with bne or bcs it will read the register,
> and it with bit 29 or bit 28 to see if it should wait and then continue
> with the writing/reading. I think you can just drop the looping for the
> v7 version of the functions and have this driver work on v6 and v7.
We could maybe drop the looping for TX, but RX has no C based looping
even tho for v7 it's recommended that we loop (presumably for v6 it's
not recommended).
> Alternatively, you can make some function that says tx buffer is empty,
> rx buffer is full or something but I don't see how saving a couple
> instructions buys us much when we can have one driver for v6 and v7.
>
> I see that Tony Lindgren modified the DCC macros for v7 in commit
> 200b7a8 (ARM: 5884/1: arm: Fix DCC console for v7, 2010-01-19). I'm not
> sure why though, since it seems that a v6 and a v7 should really do the
> same thing by waiting for the buffers to be ready before filling them or
> reading them. Which probably means we can get low-level dcc debugging on
> all targets if I'm not mistaken.
This is primarily why these function look they way they do. I'd like to
hear from Tony, because he apparent didn't think they did the same
thing.
> > +static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < count; i++) {
> > + while (__dcc_getstatus() & DCC_STATUS_TX)
> > + cpu_relax();
> > +
> > + __dcc_putchar((char)(buf[i] & 0xFF));
>
> Is this & 0xFF and cast to char unnecessary? buf is a char array, and
> chars are always 8 bits. Can't we just do __dcc_putchar(buf[i])?
Yeah, doesn't seem like it.
> > +static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < count; ++i) {
> > + int c = -1;
> > +
> > + if (__dcc_getstatus() & DCC_STATUS_RX)
> > + c = __dcc_getchar();
> > + if (c < 0)
> > + break;
> > + buf[i] = c;
> > + }
>
> I think this for loop can be simplified. __dcc_getchar() returns a char.
> It never returns -1, so the check for c < 0 can't be taken if
> __dcc_getstatus() & DCC_STATUS_RX is true. The only case you break the
> loop in then is if __dcc_getstatus() & DCC_STATUS_RX is false. So you
> can have a simple if-else and assign buf[i] in the if branch and break
> in the else branch.
>
Like this?
for (i = 0; i < count; ++i) {
if (__dcc_getstatus() & DCC_STATUS_RX)
buf[i] = __dcc_getchar();
else
break;
}
It's a micro clean up I guess ..
Daniel
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply
* Re: Getting the IRQ number (Was: Basic driver devel questions ?)
From: Scott Wood @ 2010-12-01 18:41 UTC (permalink / raw)
To: Guillaume Dargaud; +Cc: linuxppc-dev
In-Reply-To: <201012011735.58769.dargaud@lpsc.in2p3.fr>
On Wed, 1 Dec 2010 17:35:58 +0100
Guillaume Dargaud <dargaud@lpsc.in2p3.fr> wrote:
> OK, here goes then: how do I get the IRQ number so that I can install an
> interrupt handler on it ?
>
> In my dts file I have:
> xps_acqui_data_0: xps-acqui-data@c9800000 {
> compatible = "xlnx,xps-acqui-data-3.00.a";
> interrupt-parent = <&xps_intc_0>;
> interrupts = < 0 2 >;
> reg = < 0xc9800000 0x10000 >;
> xlnx,family = "virtex4";
> xlnx,include-dphase-timer = <0x1>;
> xlnx,mplb-awidth = <0x20>;
> xlnx,mplb-clk-period-ps = <0x2710>;
> xlnx,mplb-dwidth = <0x40>;
> xlnx,mplb-native-dwidth = <0x40>;
> xlnx,mplb-p2p = <0x0>;
> xlnx,mplb-smallest-slave = <0x20>;
> } ;
>
> In my minimal driver init, I have:
> first = MKDEV (my_major, my_minor);
> register_chrdev_region(first, count, NAME);
> cdev_init(my_cdev, &fops);
> cdev_add (my_cdev, first, count);
> So far so good.
>
> Now how do I connect the dots between the hardware definitions from the dts and
> my driver ?
How was your driver probed? If you can get a pointer to the device
node, use irq_of_parse_and_map() to get a virtual irq that you can pass
to request_irq().
> But first I'm not sure where to find the IRQ in there, and also I'm not sure if
> reading the filesystem from a module is allowed.
There's no need; there are much easier ways to access the device tree
from within the kernel.
> How do I know if this interrupt is shared or not (is it important ?)
Can your driver tolerate it being shared? If so, request it as shared.
-Scott
^ permalink raw reply
* Re: Getting the IRQ number (Was: Basic driver devel questions ?)
From: Philipp Ittershagen @ 2010-12-01 18:29 UTC (permalink / raw)
To: Guillaume Dargaud; +Cc: linuxppc-dev
In-Reply-To: <201012011735.58769.dargaud@lpsc.in2p3.fr>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 12/01/2010 05:35 PM, Guillaume Dargaud wrote:
> Now how do I connect the dots between the hardware definitions from the dts and
> my driver ?
You can get the interrupt number from the dt by calling
irq_of_parse_and_map(). Be sure to pass the node of your device to this
function.
Then you have to request the interrupt by calling request_irq. This is
where you specify the interrupt handler.
> But first I'm not sure where to find the IRQ in there, and also I'm not sure if
> reading the filesystem from a module is allowed.
Why do you want to read the file system?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAkz2lCEACgkQCG4q0RxCsY4GpgCgiQFRhiF7jjhUdZcUBc4Y5ScJ
E0AAn0VxcCaVexepjrah64ZSS+Xhbed8
=h97e
-----END PGP SIGNATURE-----
^ permalink raw reply
* Getting the IRQ number (Was: Basic driver devel questions ?)
From: Guillaume Dargaud @ 2010-12-01 16:35 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1291205941.11595.9.camel@concordia>
> I guess it depends how basic they are :)
>
> If they're basic _powerpc_ driver questions then this is probably the
> right place.
>
> But I'd say just ask and maybe someone will be able to help, or maybe
> they'll point you somewhere else.
OK, here goes then: how do I get the IRQ number so that I can install an
interrupt handler on it ?
In my dts file I have:
xps_acqui_data_0: xps-acqui-data@c9800000 {
compatible = "xlnx,xps-acqui-data-3.00.a";
interrupt-parent = <&xps_intc_0>;
interrupts = < 0 2 >;
reg = < 0xc9800000 0x10000 >;
xlnx,family = "virtex4";
xlnx,include-dphase-timer = <0x1>;
xlnx,mplb-awidth = <0x20>;
xlnx,mplb-clk-period-ps = <0x2710>;
xlnx,mplb-dwidth = <0x40>;
xlnx,mplb-native-dwidth = <0x40>;
xlnx,mplb-p2p = <0x0>;
xlnx,mplb-smallest-slave = <0x20>;
} ;
In my minimal driver init, I have:
first = MKDEV (my_major, my_minor);
register_chrdev_region(first, count, NAME);
cdev_init(my_cdev, &fops);
cdev_add (my_cdev, first, count);
So far so good.
Now how do I connect the dots between the hardware definitions from the dts and
my driver ?
I have:
# ll /proc/device-tree/plb@0/xps-acqui-data@c9800000/
-r--r--r-- 1 root root 27 Dec 1 16:26 compatible
-r--r--r-- 1 root root 4 Dec 1 16:26 interrupt-parent
-r--r--r-- 1 root root 8 Dec 1 16:26 interrupts
-r--r--r-- 1 root root 15 Dec 1 16:26 name
-r--r--r-- 1 root root 8 Dec 1 16:26 reg
-r--r--r-- 1 root root 8 Dec 1 16:26 xlnx,family
-r--r--r-- 1 root root 4 Dec 1 16:26 xlnx,include-dphase-
timer
-r--r--r-- 1 root root 4 Dec 1 16:26 xlnx,mplb-awidth
-r--r--r-- 1 root root 4 Dec 1 16:26 xlnx,mplb-clk-period-ps
-r--r--r-- 1 root root 4 Dec 1 16:26 xlnx,mplb-dwidth
-r--r--r-- 1 root root 4 Dec 1 16:26 xlnx,mplb-native-dwidth
-r--r--r-- 1 root root 4 Dec 1 16:26 xlnx,mplb-p2p
-r--r--r-- 1 root root 4 Dec 1 16:26 xlnx,mplb-smallest-
slave
But first I'm not sure where to find the IRQ in there, and also I'm not sure if
reading the filesystem from a module is allowed.
How do I know if this interrupt is shared or not (is it important ?)
Thanks
--
Guillaume Dargaud
http://www.gdargaud.net/
^ permalink raw reply
* Re: [MPC52xx]Latency issue with DMA on FEC
From: Jean-Michel Hautbois @ 2010-12-01 15:15 UTC (permalink / raw)
To: David Laight; +Cc: linuxppc-dev, Eric Dumazet, linux-rt-users, Steven Rostedt
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6D8ABDB@saturn3.aculab.com>
2010/12/1 David Laight <David.Laight@aculab.com>:
>
>> A mb() is usually used if you do a write to device and read from it.
>> With out it, the CPU could perform the read before the write, which
>> would give you an incorrect result. There's no other way around that.
>
> Possibly the synchronisation functions are doing significantly
> more work than is required.
>
> I was looking at the in_le32() and out_le32() functions for the
> ppc e300 (and maybe others).
>
> The out_le32() contains a 'sync' instruction - this may only
> be needed after a series of writes (eg just before a command).
>
> The iosync() function just adds a 'sync' and can be used as needed.
>
> The in_le32() not only contains the unwanted 'sync', but also
> a 'twi' (trap immediate - NFI exactly what this does) and 'isync'.
> The 'isync' is particularly horrid and unnecessary (aborts
> the instruction queue and refeches the opcode bytes).
>
> The very slow in_le32() might be there to give semi-synchronous
> traps on address fault - but unless the hardware is being probed
> that really isn't necessary.
>
> I did find st_le32() and ld_le32() in arch/powerpc/include/asm/swab.h
> but had difficulty #including that version of swab.h!
> =C2=A0 =C2=A0#include <../arch/powerpc/include/asm/swab.h>
> worked - but isn't that nice.
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0David
Yes, I was also looking at in_be16 and out_be16, and my thoughts were
exactly the same.
I think the HW I am using is not a good one, but this is not
sufficient to explain the behaviour.
These instructions are called, for instance, from bcom_enable_task().
^ permalink raw reply
* RE: [MPC52xx]Latency issue with DMA on FEC
From: David Laight @ 2010-12-01 15:09 UTC (permalink / raw)
To: Steven Rostedt, Jean-Michel Hautbois
Cc: linuxppc-dev, linux-rt-users, Eric Dumazet
In-Reply-To: <1291215130.4881.55.camel@gandalf.stny.rr.com>
=20
> A mb() is usually used if you do a write to device and read from it.
> With out it, the CPU could perform the read before the write, which
> would give you an incorrect result. There's no other way around that.
Possibly the synchronisation functions are doing significantly
more work than is required.
I was looking at the in_le32() and out_le32() functions for the
ppc e300 (and maybe others).
The out_le32() contains a 'sync' instruction - this may only
be needed after a series of writes (eg just before a command).
The iosync() function just adds a 'sync' and can be used as needed.
The in_le32() not only contains the unwanted 'sync', but also
a 'twi' (trap immediate - NFI exactly what this does) and 'isync'.
The 'isync' is particularly horrid and unnecessary (aborts
the instruction queue and refeches the opcode bytes).
The very slow in_le32() might be there to give semi-synchronous
traps on address fault - but unless the hardware is being probed
that really isn't necessary.
I did find st_le32() and ld_le32() in arch/powerpc/include/asm/swab.h
but had difficulty #including that version of swab.h!
#include <../arch/powerpc/include/asm/swab.h>
worked - but isn't that nice.
David
^ permalink raw reply
* Re: [MPC52xx]Latency issue with DMA on FEC
From: Steven Rostedt @ 2010-12-01 14:52 UTC (permalink / raw)
To: Jean-Michel Hautbois; +Cc: linuxppc-dev, linux-rt-users, Eric Dumazet
In-Reply-To: <AANLkTi=vAEcPKLqJ_yJt7NLz5JoYvEnt8S8QB99ZBNSX@mail.gmail.com>
On Wed, 2010-12-01 at 09:16 +0100, Jean-Michel Hautbois wrote:
> Hi lists !
>
> I measured the latency and the jitter of the RX and TX ethernet paths
> on my MPC5200 board.
> The RX path is quite good, but the TX path can be slow.
>
> [ 1218.976762] [mpc52xx_fec_start_xmit]Delay >30us for dma_map_single
> => 76364 ns
> [ 1219.188405] [mpc52xx_fec_tx_interrupt]Delay >30us for
> dma_unmap_single => 34515 ns
> [ 1220.628785] [mpc52xx_fec_start_xmit]Delay >30us for
> bcom_submit_next_buffer => 97273 ns
> [ 1225.776784] [mpc52xx_fec_tx_interrupt]Delay >30us for
> dma_unmap_single => 95273 ns
>
> As one can see, this is obviously problematic.
> The first function I analyzed is bcom_submit_next_buffer() => This
> function doesn't do lots of things, except a call to mb().
>
> I have been looking to the "MPC603e RISC Microprocessor User's Manual"
> and especially the chapter named "2.3.4.7 Memory Synchronization
> Instructions—UISA".
>
> Here is a paragraph which explains a lot :
>
> "The functions performed by the sync instruction normally take a
> significant amount of time
> to complete; as a result, frequent use of this instruction may
> adversely affect performance.
> In addition, the number of cycles required to complete a sync
> instruction depends on system
> parameters and on the processor's state when the instruction is issued."
>
> I am using a real time kernel, and this is a problem, as it is not
> deterministic to use this instruction.
> Is there a way to avoid this ?
Don't use that hardware.
When working with drivers there are times you must sync with the device.
And if the device is nondeterministic, then find another set of hardware
to use. Unfortunately, I think you may not find any.
A mb() is usually used if you do a write to device and read from it.
With out it, the CPU could perform the read before the write, which
would give you an incorrect result. There's no other way around that.
-- Steve
>
> I will now focus on the dma_map_single() and dma_unmap_single functions...
>
> Thanks in advance for your help,
> Best Regards,
>
> JM
^ permalink raw reply
* Re: _extending_ platform support options?
From: Josh Boyer @ 2010-12-01 13:47 UTC (permalink / raw)
To: Joachim Foerster; +Cc: linuxppc-dev
In-Reply-To: <4CF64CD7.2060802@gmx.de>
On Wed, Dec 01, 2010 at 02:25:43PM +0100, Joachim Foerster wrote:
>Hi all,
>
>currently I'm wondering what the preferred/recommend way of
>_extending_ an existing "Platform support" option is?
>
>We are working with custom design/boards based on Virtex4/5. So we
>are primarily using the CONFIG_XILINX_VIRTEX*_GENERIC_BOARD options.
>In our case we have some special needs, like custom ppc_md.restart(),
>ppc_md.power_off() or ppc_md.show_cpuinfo().
>
>Till now, we just duplicated arch/powerpc/platforms/4?x/virtex.c and
>added our special stuff. Properly renaming everything, etc ...
>
>An alternative could be to add a virtex_my.c which extends virtex.c, like this
>(also like virtex_ml510.c extends virtex.c):
>
>static void virtex_my_show_cpuinfo(struct seq_file *m)
>{
> seq_printf(m, something);
>}
>
>static int __init virtex_mle_init(void)
>{
> ppc_md.show_cpuinfo = virtex_my_show_cpuinfo;
> return 0;
>}
>machine_core_initcall(virtex, virtex_my_init);
>
>Though, to me, it does not seem really OK to assign ppc_md members
>that way. The original struct machdep for "virtex" (which is defined
>in virtex.c with define_machine()) is not adjusted either. Ok, we
>could modify that one, too.
>Especially I'm not sure if it is OK to use machine_core_initcall() for such modifications.
>
>So my question is: Is there any recommended way for doing such
>"extensions"? Or is it OK to just duplicate virtex.c (which does not
>seem really OK, too)?
Duplicate it as you have done, naming the file something unique. We try
to prevent unnecessary duplication of code, but sometimes it's cleaner
to just have a separate board file instead.
josh
^ permalink raw reply
* _extending_ platform support options?
From: Joachim Foerster @ 2010-12-01 13:25 UTC (permalink / raw)
To: linuxppc-dev
Hi all,
currently I'm wondering what the preferred/recommend way of _extending_ an existing
"Platform support" option is?
We are working with custom design/boards based on Virtex4/5. So we are primarily using the
CONFIG_XILINX_VIRTEX*_GENERIC_BOARD options. In our case we have some special needs,
like custom ppc_md.restart(), ppc_md.power_off() or ppc_md.show_cpuinfo().
Till now, we just duplicated arch/powerpc/platforms/4?x/virtex.c and added our special
stuff. Properly renaming everything, etc ...
An alternative could be to add a virtex_my.c which extends virtex.c, like this
(also like virtex_ml510.c extends virtex.c):
static void virtex_my_show_cpuinfo(struct seq_file *m)
{
seq_printf(m, something);
}
static int __init virtex_mle_init(void)
{
ppc_md.show_cpuinfo = virtex_my_show_cpuinfo;
return 0;
}
machine_core_initcall(virtex, virtex_my_init);
Though, to me, it does not seem really OK to assign ppc_md members that way. The original
struct machdep for "virtex" (which is defined in virtex.c with define_machine()) is not
adjusted either. Ok, we could modify that one, too.
Especially I'm not sure if it is OK to use machine_core_initcall() for such modifications.
So my question is: Is there any recommended way for doing such "extensions"? Or is it OK
to just duplicate virtex.c (which does not seem really OK, too)?
Thanks,
Joachim
^ permalink raw reply
* Re: Basic driver devel questions ?
From: Michael Ellerman @ 2010-12-01 12:19 UTC (permalink / raw)
To: Guillaume Dargaud; +Cc: linuxppc-dev
In-Reply-To: <201012011115.00960.dargaud@lpsc.in2p3.fr>
[-- Attachment #1: Type: text/plain, Size: 459 bytes --]
On Wed, 2010-12-01 at 11:15 +0100, Guillaume Dargaud wrote:
> Hello all,
> is it OK if I ask basic driver development questions here ?
> Could you recommend a better forum for that maybe ?
Hi Guillaume,
I guess it depends how basic they are :)
If they're basic _powerpc_ driver questions then this is probably the
right place.
But I'd say just ask and maybe someone will be able to help, or maybe
they'll point you somewhere else.
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Basic driver devel questions ?
From: Guillaume Dargaud @ 2010-12-01 10:15 UTC (permalink / raw)
To: linuxppc-dev
Hello all,
is it OK if I ask basic driver development questions here ?
Could you recommend a better forum for that maybe ?
Thanks
--
Guillaume Dargaud
http://www.gdargaud.net/
^ permalink raw reply
* [PATCH] eSPI: change the read behavior of the SPIRF
From: Mingkai Hu @ 2010-12-01 9:29 UTC (permalink / raw)
To: linuxppc-dev, spi-devel-general; +Cc: kumar.gala, Mingkai Hu
The user must read N bytes of SPIRF (1 <= N <= 4) that do not exceed the
amount of data in the receive FIFO, so read the SPIRF byte by byte when
the data in receive FIFO is less than 4 bytes.
On Simics, when read N bytes that exceed the amout of data in receive
FIFO, we can't read the data out, that is we can't clear the rx FIFO,
then the CPU will loop on the espi rx interrupt.
Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
---
drivers/spi/spi_fsl_espi.c | 19 ++++++++++++++++---
1 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi_fsl_espi.c b/drivers/spi/spi_fsl_espi.c
index e3b4f64..ae78926 100644
--- a/drivers/spi/spi_fsl_espi.c
+++ b/drivers/spi/spi_fsl_espi.c
@@ -507,16 +507,29 @@ void fsl_espi_cpu_irq(struct mpc8xxx_spi *mspi, u32 events)
/* We need handle RX first */
if (events & SPIE_NE) {
- u32 rx_data;
+ u32 rx_data, tmp;
+ u8 rx_data_8;
/* Spin until RX is done */
while (SPIE_RXCNT(events) < min(4, mspi->len)) {
cpu_relax();
events = mpc8xxx_spi_read_reg(®_base->event);
}
- mspi->len -= 4;
- rx_data = mpc8xxx_spi_read_reg(®_base->receive);
+ if (mspi->len >= 4) {
+ rx_data = mpc8xxx_spi_read_reg(®_base->receive);
+ } else {
+ tmp = mspi->len;
+ rx_data = 0;
+ while (tmp--) {
+ rx_data_8 = in_8((u8 *)®_base->receive);
+ rx_data |= (rx_data_8 << (tmp * 8));
+ }
+
+ rx_data <<= (4 - mspi->len) * 8;
+ }
+
+ mspi->len -= 4;
if (mspi->rx)
mspi->get_rx(rx_data, mspi);
--
1.7.0.4
^ permalink raw reply related
* Re: [MPC52xx]Latency issue with DMA on FEC
From: Jean-Michel Hautbois @ 2010-12-01 9:59 UTC (permalink / raw)
To: linuxppc-dev, linux-rt-users; +Cc: Eric Dumazet, Steven Rostedt
In-Reply-To: <AANLkTi=vAEcPKLqJ_yJt7NLz5JoYvEnt8S8QB99ZBNSX@mail.gmail.com>
2010/12/1 Jean-Michel Hautbois <jhautbois@gmail.com>:
> Hi lists !
>
> I measured the latency and the jitter of the RX and TX ethernet paths
> on my MPC5200 board.
> The RX path is quite good, but the TX path can be slow.
>
> [ 1218.976762] [mpc52xx_fec_start_xmit]Delay >30us for dma_map_single
> =3D> 76364 ns
> [ 1219.188405] [mpc52xx_fec_tx_interrupt]Delay >30us for
> dma_unmap_single =3D> 34515 ns
> [ 1220.628785] [mpc52xx_fec_start_xmit]Delay >30us for
> bcom_submit_next_buffer =3D> 97273 ns
> [ 1225.776784] [mpc52xx_fec_tx_interrupt]Delay >30us for
> dma_unmap_single =3D> 95273 ns
>
> As one can see, this is obviously problematic.
> The first function I analyzed is bcom_submit_next_buffer() =3D> This
> function doesn't do lots of things, except a call to mb().
>
> I have been looking to the "MPC603e RISC Microprocessor User's Manual"
> and especially the chapter named "2.3.4.7 Memory Synchronization
> Instructions=E2=80=94UISA".
>
> Here is a paragraph which explains a lot :
>
> "The functions performed by the sync instruction normally take a
> signi=EF=AC=81cant amount of time
> to complete; as a result, frequent use of this instruction may
> adversely affect performance.
> In addition, the number of cycles required to complete a sync
> instruction depends on system
> parameters and on the processor's state when the instruction is issued."
>
> I am using a real time kernel, and this is a problem, as it is not
> deterministic to use this instruction.
> Is there a way to avoid this ?
>
> I will now focus on the dma_map_single() and dma_unmap_single functions..=
.
>
> Thanks in advance for your help,
> Best Regards,
>
> JM
>
dma_map_single() and dma_unmap_single() have the same instruction set
used inside (sync) because there is a cleaning of cache.
eieio instruction doesn't seem to be faster and I think that because
cache is not inhibited, this is not a good way to do that.
The delay introduced by the use of these instructions can be really
big (about 70-90=C2=B5s) whereas in most cases it is relatively good (about
10-20=C2=B5s).
This jitter is a problem in my use case, and I think I am not the only one =
:).
One other thing to say : I am using little packets (about 200 bytes).
JM
^ permalink raw reply
* [PATCH] of/address: use propper endianess in get_flags
From: Sebastian Andrzej Siewior @ 2010-12-01 9:54 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sodaville
This patch changes u32 to __be32 for all "ranges", "prop" and "addr" and
such. Those variables are pointing to the device tree which containts
intergers in big endian format.
Most functions are doing it right because of_read_number() is doing the
right thing for them. of_bus_isa_get_flags(), of_bus_pci_get_flags() and
of_bus_isa_map() were accessing the data directly and were doing it wrong.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
arch/powerpc/include/asm/prom.h | 2 +-
drivers/of/address.c | 54 ++++++++++++++++++++------------------
include/linux/of_address.h | 6 ++--
3 files changed, 32 insertions(+), 30 deletions(-)
diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
index ae26f2e..ab34f60 100644
--- a/arch/powerpc/include/asm/prom.h
+++ b/arch/powerpc/include/asm/prom.h
@@ -42,7 +42,7 @@ extern void pci_create_OF_bus_map(void);
/* Translate a DMA address from device space to CPU space */
extern u64 of_translate_dma_address(struct device_node *dev,
- const u32 *in_addr);
+ const __be32 *in_addr);
#ifdef CONFIG_PCI
extern unsigned long pci_address_to_pio(phys_addr_t address);
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 3a1c7e7..b4559c5 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -12,13 +12,13 @@
(ns) > 0)
static struct of_bus *of_match_bus(struct device_node *np);
-static int __of_address_to_resource(struct device_node *dev, const u32 *addrp,
- u64 size, unsigned int flags,
+static int __of_address_to_resource(struct device_node *dev,
+ const __be32 *addrp, u64 size, unsigned int flags,
struct resource *r);
/* Debug utility */
#ifdef DEBUG
-static void of_dump_addr(const char *s, const u32 *addr, int na)
+static void of_dump_addr(const char *s, const __be32 *addr, int na)
{
printk(KERN_DEBUG "%s", s);
while (na--)
@@ -26,7 +26,7 @@ static void of_dump_addr(const char *s, const u32 *addr, int na)
printk("\n");
}
#else
-static void of_dump_addr(const char *s, const u32 *addr, int na) { }
+static void of_dump_addr(const char *s, const __be32 *addr, int na) { }
#endif
/* Callbacks for bus specific translators */
@@ -36,10 +36,10 @@ struct of_bus {
int (*match)(struct device_node *parent);
void (*count_cells)(struct device_node *child,
int *addrc, int *sizec);
- u64 (*map)(u32 *addr, const u32 *range,
+ u64 (*map)(u32 *addr, const __be32 *range,
int na, int ns, int pna);
int (*translate)(u32 *addr, u64 offset, int na);
- unsigned int (*get_flags)(const u32 *addr);
+ unsigned int (*get_flags)(const __be32 *addr);
};
/*
@@ -55,7 +55,7 @@ static void of_bus_default_count_cells(struct device_node *dev,
*sizec = of_n_size_cells(dev);
}
-static u64 of_bus_default_map(u32 *addr, const u32 *range,
+static u64 of_bus_default_map(u32 *addr, const __be32 *range,
int na, int ns, int pna)
{
u64 cp, s, da;
@@ -85,7 +85,7 @@ static int of_bus_default_translate(u32 *addr, u64 offset, int na)
return 0;
}
-static unsigned int of_bus_default_get_flags(const u32 *addr)
+static unsigned int of_bus_default_get_flags(const __be32 *addr)
{
return IORESOURCE_MEM;
}
@@ -110,10 +110,10 @@ static void of_bus_pci_count_cells(struct device_node *np,
*sizec = 2;
}
-static unsigned int of_bus_pci_get_flags(const u32 *addr)
+static unsigned int of_bus_pci_get_flags(const __be32 *addr)
{
unsigned int flags = 0;
- u32 w = addr[0];
+ u32 w = be32_to_cpup(addr);
switch((w >> 24) & 0x03) {
case 0x01:
@@ -129,7 +129,8 @@ static unsigned int of_bus_pci_get_flags(const u32 *addr)
return flags;
}
-static u64 of_bus_pci_map(u32 *addr, const u32 *range, int na, int ns, int pna)
+static u64 of_bus_pci_map(u32 *addr, const __be32 *range, int na, int ns,
+ int pna)
{
u64 cp, s, da;
unsigned int af, rf;
@@ -160,7 +161,7 @@ static int of_bus_pci_translate(u32 *addr, u64 offset, int na)
return of_bus_default_translate(addr + 1, offset, na - 1);
}
-const u32 *of_get_pci_address(struct device_node *dev, int bar_no, u64 *size,
+const __be32 *of_get_pci_address(struct device_node *dev, int bar_no, u64 *size,
unsigned int *flags)
{
const __be32 *prop;
@@ -207,7 +208,7 @@ EXPORT_SYMBOL(of_get_pci_address);
int of_pci_address_to_resource(struct device_node *dev, int bar,
struct resource *r)
{
- const u32 *addrp;
+ const __be32 *addrp;
u64 size;
unsigned int flags;
@@ -237,12 +238,13 @@ static void of_bus_isa_count_cells(struct device_node *child,
*sizec = 1;
}
-static u64 of_bus_isa_map(u32 *addr, const u32 *range, int na, int ns, int pna)
+static u64 of_bus_isa_map(u32 *addr, const __be32 *range, int na, int ns,
+ int pna)
{
u64 cp, s, da;
/* Check address type match */
- if ((addr[0] ^ range[0]) & 0x00000001)
+ if ((addr[0] ^ range[0]) & cpu_to_be32(1))
return OF_BAD_ADDR;
/* Read address values, skipping high cell */
@@ -264,10 +266,10 @@ static int of_bus_isa_translate(u32 *addr, u64 offset, int na)
return of_bus_default_translate(addr + 1, offset, na - 1);
}
-static unsigned int of_bus_isa_get_flags(const u32 *addr)
+static unsigned int of_bus_isa_get_flags(const __be32 *addr)
{
unsigned int flags = 0;
- u32 w = addr[0];
+ u32 w = be32_to_cpup(addr);
if (w & 1)
flags |= IORESOURCE_IO;
@@ -330,7 +332,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
struct of_bus *pbus, u32 *addr,
int na, int ns, int pna, const char *rprop)
{
- const u32 *ranges;
+ const __be32 *ranges;
unsigned int rlen;
int rone;
u64 offset = OF_BAD_ADDR;
@@ -398,7 +400,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
* that can be mapped to a cpu physical address). This is not really specified
* that way, but this is traditionally the way IBM at least do things
*/
-u64 __of_translate_address(struct device_node *dev, const u32 *in_addr,
+u64 __of_translate_address(struct device_node *dev, const __be32 *in_addr,
const char *rprop)
{
struct device_node *parent = NULL;
@@ -475,22 +477,22 @@ u64 __of_translate_address(struct device_node *dev, const u32 *in_addr,
return result;
}
-u64 of_translate_address(struct device_node *dev, const u32 *in_addr)
+u64 of_translate_address(struct device_node *dev, const __be32 *in_addr)
{
return __of_translate_address(dev, in_addr, "ranges");
}
EXPORT_SYMBOL(of_translate_address);
-u64 of_translate_dma_address(struct device_node *dev, const u32 *in_addr)
+u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr)
{
return __of_translate_address(dev, in_addr, "dma-ranges");
}
EXPORT_SYMBOL(of_translate_dma_address);
-const u32 *of_get_address(struct device_node *dev, int index, u64 *size,
+const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
unsigned int *flags)
{
- const u32 *prop;
+ const __be32 *prop;
unsigned int psize;
struct device_node *parent;
struct of_bus *bus;
@@ -525,8 +527,8 @@ const u32 *of_get_address(struct device_node *dev, int index, u64 *size,
}
EXPORT_SYMBOL(of_get_address);
-static int __of_address_to_resource(struct device_node *dev, const u32 *addrp,
- u64 size, unsigned int flags,
+static int __of_address_to_resource(struct device_node *dev,
+ const __be32 *addrp, u64 size, unsigned int flags,
struct resource *r)
{
u64 taddr;
@@ -564,7 +566,7 @@ static int __of_address_to_resource(struct device_node *dev, const u32 *addrp,
int of_address_to_resource(struct device_node *dev, int index,
struct resource *r)
{
- const u32 *addrp;
+ const __be32 *addrp;
u64 size;
unsigned int flags;
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 8aea06f..2feda6e 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -3,7 +3,7 @@
#include <linux/ioport.h>
#include <linux/of.h>
-extern u64 of_translate_address(struct device_node *np, const u32 *addr);
+extern u64 of_translate_address(struct device_node *np, const __be32 *addr);
extern int of_address_to_resource(struct device_node *dev, int index,
struct resource *r);
extern void __iomem *of_iomap(struct device_node *device, int index);
@@ -21,7 +21,7 @@ static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
#endif
#ifdef CONFIG_PCI
-extern const u32 *of_get_pci_address(struct device_node *dev, int bar_no,
+extern const __be32 *of_get_pci_address(struct device_node *dev, int bar_no,
u64 *size, unsigned int *flags);
extern int of_pci_address_to_resource(struct device_node *dev, int bar,
struct resource *r);
@@ -32,7 +32,7 @@ static inline int of_pci_address_to_resource(struct device_node *dev, int bar,
return -ENOSYS;
}
-static inline const u32 *of_get_pci_address(struct device_node *dev,
+static inline const __be32 *of_get_pci_address(struct device_node *dev,
int bar_no, u64 *size, unsigned int *flags)
{
return NULL;
--
1.7.3.2
^ permalink raw reply related
* Re: PHY/FEC Network adapter failed to initialize on MPC52xx Board
From: Peter Kuennemann@Crane-Soft @ 2010-12-01 9:06 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <4CF31492.7040809@windriver.com>
Tanks to all for the reply to this thread. I got the tip from the thread Tiejun
mentioned.
Quotation: 'St. Strobel' in Xenomai-help
"I had this problem too. In my case the problem was caused by an incorrect port multiplex configuration in U-Boot, see CONIFG_SYS_GPS_PORT_CONFIG (I use 0x xxx5xxxx, which corresponds to 10/100Mbit
Eth with MD). "
That fixed it for me.
Regards, Peter
Am 29.11.2010 03:48, schrieb tiejun.chen:
> Peter wrote:
>> Hi all
>>
>> I got completely stuck with a network adapter problem on my
>> ppc board (MPC52xx style). The ntwork adapter does not seem
>> to intialize correctly when booted without 'help from uboot'
>>
> Looks your problem is very similar to one I replied here not long ago :) That is
> also issued from MPC5200. And I remember there was a wrong Port Multiplex
> Configuration.
>
> If possible maybe you can check the email subjected "Problem Ethernet
> Initialization MPC5200 + LXT971A" on linuxppc-dev list.
>
> Hope its useful.
>
> Tiejun
>
>> The adapter works properly when I first use it with uboot. E.g.
>> using tftp to load the kernel or just issuing a dummy sntp
>> command. It does not get intialized if I boot linux without
>> using any network relevant command in ubboot
>>
>> The difference manifests on the boot message: (working)
>> PHY working
>> ...
>> mpc52xx MII bus: probed
>> TCP cubic registered
>> NET: Registered protocol family 17
>> IP-Config: Complete:
>> device=eth0, addr=192.168.1.245, mask=255.254.0.0, gw=192.168.1.2,
>> host=192.168.1.245, domain=, nis-domain=(none),
>> bootserver=192.168.1.244, rootserver=192.168.1.244, rootpath=
>> Looking up port of RPC 100003/2 on 192.168.1.244
>> Looking up port of RPC 100005/1 on 192.168.1.244
>> VFS: Mounted root (nfs filesystem) on device 0:11.
>> Freeing unused kernel memory: 124k init
>> PHY: f0003000:00 - Link is Up - 100/Full
>>
>> # ping 192.168.1.2 returns proper results.
>>
>> PHY Not working:
>> ...
>> mpc52xx MII bus: probed
>> TCP cubic registered
>> NET: Registered protocol family 17
>> IP-Config: Complete:
>> device=eth0, addr=192.168.1.245, mask=255.254.0.0, gw=192.168.1.2,
>> host=192.168.1.245, domain=, nis-domain=(none),
>> bootserver=192.168.1.244, rootserver=192.168.1.244, rootpath=
>> VFS: Mounted root (squashfs filesystem) readonly on device 31:3.
>> Freeing unused kernel memory: 124k init
>>
>> # ping 192.168.1.2 hangs
>>
>>
>> The second snipped does not have "Looking up.." messages because it
>> boots from flash. Main difference is "PHY: f0003000:00 - Link is Up - 100/Full"
>> which does not appear at the failing case.
>>
>> Linux Version is 2.6.35.7 patched with xenomai 2.5
>> U-Boot 2010.06 (Aug 05 2010 - 19:54:45)
>>
>> Linux configuration see below: ( i left most entries out that are not set)
>> I also experimented with different settings but finally only
>> CONFIG_FEC_MPC52xx=y and CONFIG_FEC_MPC52xx_MDIO=y
>> seem to be of any relevance. If both are set, the adapter works
>> when initialized by uboot.
>>
>> Any help or tips will be very much appreciated,
>>
>> Regards, Peter
>>
>>
>> Linux .config
>>
>> ...
>> #
>> # Platform support
>> #
>> # CONFIG_PPC_CHRP is not set
>> # CONFIG_MPC5121_ADS is not set
>> # CONFIG_MPC5121_GENERIC is not set
>> CONFIG_PPC_MPC52xx=y
>> CONFIG_PPC_MPC5200_SIMPLE=y
>> # CONFIG_PPC_EFIKA is not set
>> CONFIG_PPC_LITE5200=y
>> # CONFIG_PPC_MEDIA5200 is not set
>> CONFIG_PPC_MPC5200_BUGFIX=y
>> # CONFIG_PPC_MPC5200_GPIO is not set
>> CONFIG_PPC_MPC5200_LPBFIFO=y
>>
>> CONFIG_PPC_BESTCOMM=y
>> CONFIG_PPC_BESTCOMM_FEC=y
>> CONFIG_PPC_BESTCOMM_GEN_BD=y
>> # CONFIG_SIMPLE_GPIO is not set
>> ..
>> # Bus options
>> #
>> CONFIG_ZONE_DMA=y
>> CONFIG_NEED_SG_DMA_LENGTH=y
>> CONFIG_GENERIC_ISA_DMA=y
>> CONFIG_PPC_PCI_CHOICE=y
>> ...
>> #
>> # Generic Driver Options
>> #
>> CONFIG_STANDALONE=y
>> CONFIG_PREVENT_FIRMWARE_BUILD=y
>> CONFIG_MTD=y
>> CONFIG_MTD_PARTITIONS=y
>> CONFIG_MTD_CMDLINE_PARTS=y
>>
>> #
>> # MII PHY device drivers
>> #
>> CONFIG_LXT_PHY=y ## Does not seem to have any influence
>> CONFIG_NET_ETHERNET=y
>> CONFIG_MII=y
>> CONFIG_ETHOC=y ## Does not seem to have any influence
>> CONFIG_FEC_MPC52xx=y ## Must be Y in roder to get adapter working with uboot's init
>> CONFIG_FEC_MPC52xx_MDIO=y ## Must be Y in roder to get adapter working with uboot's init
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* [MPC52xx]Latency issue with DMA on FEC
From: Jean-Michel Hautbois @ 2010-12-01 8:16 UTC (permalink / raw)
To: linuxppc-dev, linux-rt-users; +Cc: Eric Dumazet, Steven Rostedt
Hi lists !
I measured the latency and the jitter of the RX and TX ethernet paths
on my MPC5200 board.
The RX path is quite good, but the TX path can be slow.
[ 1218.976762] [mpc52xx_fec_start_xmit]Delay >30us for dma_map_single
=3D> 76364 ns
[ 1219.188405] [mpc52xx_fec_tx_interrupt]Delay >30us for
dma_unmap_single =3D> 34515 ns
[ 1220.628785] [mpc52xx_fec_start_xmit]Delay >30us for
bcom_submit_next_buffer =3D> 97273 ns
[ 1225.776784] [mpc52xx_fec_tx_interrupt]Delay >30us for
dma_unmap_single =3D> 95273 ns
As one can see, this is obviously problematic.
The first function I analyzed is bcom_submit_next_buffer() =3D> This
function doesn't do lots of things, except a call to mb().
I have been looking to the "MPC603e RISC Microprocessor User's Manual"
and especially the chapter named "2.3.4.7 Memory Synchronization
Instructions=E2=80=94UISA".
Here is a paragraph which explains a lot :
"The functions performed by the sync instruction normally take a
signi=EF=AC=81cant amount of time
to complete; as a result, frequent use of this instruction may
adversely affect performance.
In addition, the number of cycles required to complete a sync
instruction depends on system
parameters and on the processor's state when the instruction is issued."
I am using a real time kernel, and this is a problem, as it is not
deterministic to use this instruction.
Is there a way to avoid this ?
I will now focus on the dma_map_single() and dma_unmap_single functions...
Thanks in advance for your help,
Best Regards,
JM
^ permalink raw reply
* Re: [PATCH 01/13] powerpc/nvram: Move things out of asm/nvram.h
From: Jim Keniston @ 2010-12-01 7:24 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev, jkensito
In-Reply-To: <1291096063-14386-1-git-send-email-benh@kernel.crashing.org>
On Tue, 2010-11-30 at 16:47 +1100, benh@kernel.crashing.org wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> This moves a bunch of definitions out of asm/nvram.h to the files
> that use them or just outright remove completely unused stuff.
>
> We leave the partition signatures definitions, they will be useful
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
I applied the whole 13-patch set, and built and booted the result.
Seems fine.
Tested-by: Jim Keniston <jkenisto@us.ibm.com>
Jim
^ permalink raw reply
* Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support
From: Stephen Boyd @ 2010-12-01 5:30 UTC (permalink / raw)
To: Daniel Walker
Cc: Randy Dunlap, Mike Frysinger, Arnd Bergmann, Nicolas Pitre,
Tony Lindgren, linux-arm-msm, Greg Kroah-Hartman, linux-kernel,
FUJITA Tomonori, Andrew Morton, linuxppc-dev, Alan Cox
In-Reply-To: <1291145141-18301-1-git-send-email-dwalker@codeaurora.org>
On 11/30/2010 11:25 AM, Daniel Walker wrote:
> @@ -682,6 +682,15 @@ config HVC_UDBG
> select HVC_DRIVER
> default n
>
> +config HVC_DCC
> + bool "ARM JTAG DCC console"
> + depends on ARM
> + select HVC_DRIVER
> + help
> + This console uses the JTAG DCC on ARM to create a console under the HVC
Looks like you added one too many spaces for indent here.
> diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
> new file mode 100644
> index 0000000..6470f63
> --- /dev/null
> +++ b/drivers/char/hvc_dcc.c
> +static inline u32 __dcc_getstatus(void)
> +{
> + u32 __ret;
> +
> + asm("mrc p14, 0, %0, c0, c1, 0 @ read comms ctrl reg"
> + : "=r" (__ret) : : "cc");
> +
> + return __ret;
> +}
Without marking this asm volatile my compiler decides it can cache the
value of __ret in a register and then check the value of it continually
in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and
fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).
00000000 <hvc_dcc_put_chars>:
0: ee103e11 mrc 14, 0, r3, cr0, cr1, {0}
4: e3a0c000 mov ip, #0 ; 0x0
8: e2033202 and r3, r3, #536870912 ; 0x20000000
c: ea000006 b 2c <hvc_dcc_put_chars+0x2c>
10: e3530000 cmp r3, #0 ; 0x0
14: 1afffffd bne 10 <hvc_dcc_put_chars+0x10>
18: e7d1000c ldrb r0, [r1, ip]
1c: ee10fe11 mrc 14, 0, pc, cr0, cr1, {0}
20: 2afffffd bcs 1c <hvc_dcc_put_chars+0x1c>
24: ee000e15 mcr 14, 0, r0, cr0, cr5, {0}
28: e28cc001 add ip, ip, #1 ; 0x1
2c: e15c0002 cmp ip, r2
30: bafffff6 blt 10 <hvc_dcc_put_chars+0x10>
34: e1a00002 mov r0, r2
38: e12fff1e bx lr
As you can see, the value of the mrc is checked against DCC_STATUS_TX
(bit 29) and then stored in r3 for later use. Marking this volatile
produces the following:
00000000 <hvc_dcc_put_chars>:
0: e3a03000 mov r3, #0 ; 0x0
4: ea000007 b 28 <hvc_dcc_put_chars+0x28>
8: ee100e11 mrc 14, 0, r0, cr0, cr1, {0}
c: e3100202 tst r0, #536870912 ; 0x20000000
10: 1afffffc bne 8 <hvc_dcc_put_chars+0x8>
14: e7d10003 ldrb r0, [r1, r3]
18: ee10fe11 mrc 14, 0, pc, cr0, cr1, {0}
1c: 2afffffd bcs 18 <hvc_dcc_put_chars+0x18>
20: ee000e15 mcr 14, 0, r0, cr0, cr5, {0}
24: e2833001 add r3, r3, #1 ; 0x1
28: e1530002 cmp r3, r2
2c: bafffff5 blt 8 <hvc_dcc_put_chars+0x8>
30: e1a00002 mov r0, r2
34: e12fff1e bx lr
which looks better.
I marked all the asm in this driver as volatile. Is that correct?
> +#if defined(CONFIG_CPU_V7)
> +static inline char __dcc_getchar(void)
> +{
> + char __c;
> +
> + asm("get_wait: mrc p14, 0, pc, c0, c1, 0 \n\
> + bne get_wait \n\
> + mrc p14, 0, %0, c0, c5, 0 @ read comms data reg"
> + : "=r" (__c) : : "cc");
> +
> + return __c;
> +}
> +#else
> +static inline char __dcc_getchar(void)
> +{
> + char __c;
> +
> + asm("mrc p14, 0, %0, c0, c5, 0 @ read comms data reg"
> + : "=r" (__c));
> +
> + return __c;
> +}
> +#endif
> +
> +#if defined(CONFIG_CPU_V7)
> +static inline void __dcc_putchar(char c)
> +{
> + asm("put_wait: mrc p14, 0, pc, c0, c1, 0 \n\
> + bcs put_wait \n\
> + mcr p14, 0, %0, c0, c5, 0 "
> + : : "r" (c) : "cc");
> +}
> +#else
> +static inline void __dcc_putchar(char c)
> +{
> + asm("mcr p14, 0, %0, c0, c5, 0 @ write a char"
> + : /* no output register */
> + : "r" (c));
> +}
> +#endif
> +
I don't think both the v7 and v6 functions are necessary. It seems I can
get away with just the second version of __dcc_(get|put)char() on a v7.
The mrc p14, 0, pc, c0, c1, 0 will assign the top 4 bits (31-28) to the
condition codes NZCV on v7. It also looks like on an ARM11 (a v6) will
also do the same thing if I read the manuals right. The test in the
inline assembly is saying, wait for a character to be ready or wait for
a character to be read then actually write a character or read one. The
code in hvc_dcc_put_chars() is already doing the same thing, albeit in a
slightly different form. Instead of getting the status bits put into the
condition codes and looping with bne or bcs it will read the register,
and it with bit 29 or bit 28 to see if it should wait and then continue
with the writing/reading. I think you can just drop the looping for the
v7 version of the functions and have this driver work on v6 and v7.
Alternatively, you can make some function that says tx buffer is empty,
rx buffer is full or something but I don't see how saving a couple
instructions buys us much when we can have one driver for v6 and v7.
I see that Tony Lindgren modified the DCC macros for v7 in commit
200b7a8 (ARM: 5884/1: arm: Fix DCC console for v7, 2010-01-19). I'm not
sure why though, since it seems that a v6 and a v7 should really do the
same thing by waiting for the buffers to be ready before filling them or
reading them. Which probably means we can get low-level dcc debugging on
all targets if I'm not mistaken.
> +static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
> +{
> + int i;
> +
> + for (i = 0; i < count; i++) {
> + while (__dcc_getstatus() & DCC_STATUS_TX)
> + cpu_relax();
> +
> + __dcc_putchar((char)(buf[i] & 0xFF));
Is this & 0xFF and cast to char unnecessary? buf is a char array, and
chars are always 8 bits. Can't we just do __dcc_putchar(buf[i])?
> +static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
> +{
> + int i;
> +
> + for (i = 0; i < count; ++i) {
> + int c = -1;
> +
> + if (__dcc_getstatus() & DCC_STATUS_RX)
> + c = __dcc_getchar();
> + if (c < 0)
> + break;
> + buf[i] = c;
> + }
I think this for loop can be simplified. __dcc_getchar() returns a char.
It never returns -1, so the check for c < 0 can't be taken if
__dcc_getstatus() & DCC_STATUS_RX is true. The only case you break the
loop in then is if __dcc_getstatus() & DCC_STATUS_RX is false. So you
can have a simple if-else and assign buf[i] in the if branch and break
in the else branch.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox