LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* dtc: Generate useful error message for properties after subnodes
From: David Gibson @ 2007-12-04 23:27 UTC (permalink / raw)
  To: Jon Loeliger; +Cc: linuxppc-dev

On several occasions, I've accidentally put properties after subnodes
in a dts file.  I've then spent ages thinking that the resulting
syntax error was because of something else.

This patch arranges for this specific syntax error to generate a more
specific and useful error message.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Index: dtc/tests/prop-after-subnode.dts
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ dtc/tests/prop-after-subnode.dts	2007-12-05 10:24:52.000000000 +1100
@@ -0,0 +1,9 @@
+/dts-v1/;
+
+/ {
+	node1 {
+	};
+	prop;
+	node2 {
+	};
+};
Index: dtc/dtc-parser.y
===================================================================
--- dtc.orig/dtc-parser.y	2007-12-05 10:12:10.000000000 +1100
+++ dtc/dtc-parser.y	2007-12-05 10:18:26.000000000 +1100
@@ -276,6 +276,11 @@
 		{
 			$$ = chain_node($1, $2);
 		}
+	| subnode propdef
+		{
+			yyerror("syntax error: properties must precede subnodes\n");
+			YYERROR;
+		}
 	;
 
 subnode:

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply

* Re: ucc_uart: add support for Freescale QUICCEngine UART
From: Arnd Bergmann @ 2007-12-04 23:26 UTC (permalink / raw)
  To: Timur Tabi; +Cc: linuxppc-dev
In-Reply-To: <4755D73D.7040204@freescale.com>

On Tuesday 04 December 2007, Timur Tabi wrote:
> When I program the DMA controller, I give it a dma_addr_t. =A0And yet, th=
e DMA=20
> controller and the QE are both devices on the SoC. =A0So if the DMA contr=
oller=20
> takes a dma_addr_t, then shouldn't the QE also take one?
>=20

=46rom a code clarity perspective, the interesting point is that dma_addr_t=
 is
what comes back from the functions in dma-mapping.h. If you don't use them,
a physical address is phys_addr_t.

You can argue that the QS is really a DMA device, but in that case you
should convert the driver to use the DMA mapping interfaces correctly,
which I would consider overkill.

	Arnd <><

^ permalink raw reply

* Re: [PATCH v2] Fix hardware IRQ time accounting problem.
From: Jörg Sommer @ 2007-12-04 22:21 UTC (permalink / raw)
  To: linuxppc-dev
In-Reply-To: <20071204055144.GR24243@bakeyournoodle.com>

Hallo Tony,

Tony Breeds <tony@bakeyournoodle.com> wrote:
> The commit fa13a5a1f25f671d084d8884be96fc48d9b68275 (sched: restore
> deterministic CPU accounting on powerpc), unconditionally calls
> update_process_tick() in system context.  In the deterministic accounting case
> this is the correct thing to do.  However, in the non-deterministic accounting
> case we need to not do this, and results in the time accounted as hardware irq
> time being artificially elevated.
>
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> ---
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 41e13f4..b9d8837 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -350,7 +350,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
>  	local_irq_save(flags);
>  
>  	account_system_vtime(current);
> -	account_process_tick(current, 0);
> +	account_process_vtime(current);
>  	calculate_steal_time();

This patch works for me. The high hardware interrupt load is gone. Thanks.

Bye, Jörg.
-- 
Der Klügere gibt so lange nach bis er der Dumme ist.

^ permalink raw reply

* dtc: Trivial lexer cleanups
From: David Gibson @ 2007-12-04 22:50 UTC (permalink / raw)
  To: Jon Loeliger; +Cc: linuxppc-dev

This patch applies a couple of tiny cleanups to the lexer.  The
not-very-useful 'WS' named pattern is removed, and the debugging
printf() for single character tokens is moved to the top of the
action, which results in less confusing output when LEXDEBUG is
switched on (because it goes before the printf()s for possible
resulting lexer state changes).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Index: dtc/dtc-lexer.l
===================================================================
--- dtc.orig/dtc-lexer.l	2007-11-22 17:29:51.000000000 +1100
+++ dtc/dtc-lexer.l	2007-11-22 17:30:55.000000000 +1100
@@ -29,7 +29,6 @@ PROPNODECHAR	[a-zA-Z0-9,._+*#?@-]
 PATHCHAR	({PROPNODECHAR}|[/])
 LEGACYPATHCHAR	[a-zA-Z0-9_@/]
 LABEL		[a-zA-Z_][a-zA-Z0-9_]*
-WS		[[:space:]]
 
 %{
 #include "dtc.h"
@@ -193,7 +192,7 @@ static int dts_version; /* = 0 */
 		}
 
 
-<*>{WS}+	/* eat whitespace */
+<*>[[:space:]]+	/* eat whitespace */
 
 <*>"/*"([^*]|\*+[^*/])*\*+"/"	{
 			yylloc.filenum = srcpos_filenum;
@@ -207,6 +206,8 @@ static int dts_version; /* = 0 */
 <*>.		{
 			yylloc.filenum = srcpos_filenum;
 			yylloc.first_line = yylineno;
+			DPRINT("Char: %c (\\x%02x)\n", yytext[0],
+				(unsigned)yytext[0]);
 			if (yytext[0] == '[') {
 				DPRINT("<BYTESTRING>\n");
 				BEGIN(BYTESTRING);
@@ -216,9 +217,6 @@ static int dts_version; /* = 0 */
 				DPRINT("<PROPNODENAME>\n");
 				BEGIN(PROPNODENAME);
 			}
-			DPRINT("Char: %c (\\x%02x)\n", yytext[0],
-				(unsigned)yytext[0]);
-
 			return yytext[0];
 		}
 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply

* dtc: Convert "name" property checking to new infrastructure
From: David Gibson @ 2007-12-04 22:40 UTC (permalink / raw)
  To: Jon Loeliger; +Cc: linuxppc-dev

This patch removes the old-style checking code for the "name" property
- i.e. verifying that the "name" property, if present, matches the
node name.  It replaces it with a pair of more-or-less equivalent
checks in the new checking framework.

This also promotes this check to a "structural" check, or at least an
error-rather-than-warning test, since the structural/semantic
distinction doesn't really apply in the new framework.

A testcase for the check is also added.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Index: dtc/checks.c
===================================================================
--- dtc.orig/checks.c	2007-12-04 14:14:41.000000000 +1100
+++ dtc/checks.c	2007-12-04 14:41:46.000000000 +1100
@@ -170,6 +170,27 @@ out:
 }
 
 /*
+ * Utility check functions
+ */
+
+static void check_is_string(struct check *c, struct node *root,
+			    struct node *node)
+{
+	struct property *prop;
+	char *propname = c->data;
+
+	prop = get_property(node, propname);
+	if (!prop)
+		return; /* Not present, assumed ok */
+
+	if (!data_is_one_string(prop->val))
+		FAIL(c, "\"%s\" property in %s is not a string",
+		     propname, node->fullpath);
+}
+#define CHECK_IS_STRING(nm, propname, lvl) \
+	CHECK(nm, NULL, check_is_string, NULL, (propname), (lvl))
+
+/*
  * Structural check functions
  */
 
@@ -236,6 +257,23 @@ static void check_explicit_phandles(stru
 }
 NODE_CHECK(explicit_phandles, NULL, ERROR);
 
+static void check_name_properties(struct check *c, struct node *root,
+				  struct node *node)
+{
+	struct property *prop;
+
+	prop = get_property(node, "name");
+	if (!prop)
+		return; /* No name property, that's fine */
+
+	if ((prop->val.len != node->basenamelen+1)
+	    || (memcmp(prop->val.val, node->name, node->basenamelen) != 0))
+		FAIL(c, "\"name\" property in %s is incorrect (\"%s\" instead"
+		     " of base node name)", node->fullpath, prop->val.val);
+}
+CHECK_IS_STRING(name_is_string, "name", ERROR);
+NODE_CHECK(name_properties, NULL, ERROR, &name_is_string);
+
 /*
  * Reference fixup functions
  */
@@ -266,6 +304,7 @@ CHECK(phandle_references, NULL, NULL, fi
 
 static struct check *check_table[] = {
 	&duplicate_node_names, &duplicate_property_names,
+	&name_is_string, &name_properties,
 	&explicit_phandles,
 	&phandle_references,
 };
@@ -350,25 +389,10 @@ static int must_be_string(struct propert
 	return 1;
 }
 
-static int name_prop_check(struct property *prop, struct node *node)
-{
-	if ((prop->val.len != node->basenamelen+1)
-	    || !strneq(prop->val.val, node->name, node->basenamelen)) {
-		ERRMSG("name property \"%s\" does not match node basename in %s\n",
-		       prop->val.val,
-		       node->fullpath);
-		return 0;
-	}
-
-	return 1;
-}
-
 static struct {
 	char *propname;
 	int (*check_fn)(struct property *prop, struct node *node);
 } prop_checker_table[] = {
-	{"name", must_be_string},
-	{"name", name_prop_check},
 	{"linux,phandle", must_be_one_cell},
 	{"#address-cells", must_be_one_cell},
 	{"#size-cells", must_be_one_cell},
Index: dtc/tests/bad-name-property.dts
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ dtc/tests/bad-name-property.dts	2007-12-04 14:28:54.000000000 +1100
@@ -0,0 +1,7 @@
+/dts-v1/;
+
+/ {
+	node@0 {
+		name = "badthing";
+	};
+};
Index: dtc/tests/run_tests.sh
===================================================================
--- dtc.orig/tests/run_tests.sh	2007-12-04 14:29:14.000000000 +1100
+++ dtc/tests/run_tests.sh	2007-12-04 14:29:27.000000000 +1100
@@ -163,6 +163,7 @@ dtc_tests () {
     run_test dtc-checkfails.sh -I dts -O dtb minusone-phandle.dts
     run_test dtc-checkfails.sh -I dts -O dtb nonexist-node-ref.dts
     run_test dtc-checkfails.sh -I dts -O dtb nonexist-label-ref.dts
+    run_test dtc-checkfails.sh -I dts -O dtb bad-name-property.dts
 }
 
 while getopts "vt:m" ARG ; do

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply

* Re: ucc_uart: add support for Freescale QUICCEngine UART
From: Timur Tabi @ 2007-12-04 22:39 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: linuxppc-dev
In-Reply-To: <200712042313.58252.arnd@arndb.de>

Arnd Bergmann wrote:

> I'm guessing that you don't really mean dma_addr_t here, but rather
> phys_addr_t, which is something different.

Now that I think about it, I don't know which is correct.  The value is plugged 
into the pointer register of a buffer descriptor, and the QE performs a DMA-like 
memory transfer from that address into its local memory.  I don't know if the QE 
is considered "external" enough that the address is a DMA address or a physical 
address.

When I program the DMA controller, I give it a dma_addr_t.  And yet, the DMA 
controller and the QE are both devices on the SoC.  So if the DMA controller 
takes a dma_addr_t, then shouldn't the QE also take one?

-- 
Timur Tabi
Linux kernel developer at Freescale

^ permalink raw reply

* Re: Merge dtc
From: Josh Boyer @ 2007-12-04 22:34 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: linuxppc-dev, Gibson, David Woodhouse, David
In-Reply-To: <18261.53985.700234.923526@cargo.ozlabs.ibm.com>

On Wed, 5 Dec 2007 09:21:21 +1100
Paul Mackerras <paulus@samba.org> wrote:

> David Woodhouse writes:
> 
> > I think this is a bad idea -- it's hardly a difficult for those people
> > who _do_ need dts to obtain it separately.
> 
> The trouble is that it's not just people who are making a kernel for a
> specific embedded board that need dtc.  These days anyone who wants to
> try cross-compiling a powerpc kernel and does a make allyesconfig, or
> who picks cell_defconfig or ps3_defconfig to try, needs dtc if their
> kernel build is to go all the way through and give them an exit status
> of 0.  I can see that for people who are trying to do the right thing
> and compile-test their patch across architectures, it's annoying that
> powerpc has an extra external requirement when no other architecture
> does, and it usually just means they don't compile-test on powerpc.
> 
> Of the various options for solving this, including dtc in the kernel
> sources seems best to me.

Using that same reasoning, should we merge a mkimage patch for the
boards that use U-Boot?

(That's a serious question, not a smart-ass response)

josh

^ permalink raw reply

* dtc: Fix FAIL() macro varargs
From: David Gibson @ 2007-12-04 22:34 UTC (permalink / raw)
  To: Jon Loeliger; +Cc: linuxppc-dev

The way the checking subsystem FAIL() macro is currently implemented
it must take at least one paramater after the format string.  This
patch corrects the problem.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Index: dtc/checks.c
===================================================================
--- dtc.orig/checks.c	2007-12-04 16:42:48.000000000 +1100
+++ dtc/checks.c	2007-12-04 17:17:42.000000000 +1100
@@ -101,11 +101,11 @@ static inline void check_msg(struct chec
 	fprintf(stderr, "\n");
 }
 
-#define FAIL(c, fmt, ...) \
+#define FAIL(c, ...) \
 	do { \
 		TRACE((c), "\t\tFAILED at %s:%d", __FILE__, __LINE__); \
 		(c)->status = FAILED; \
-		check_msg((c), fmt, __VA_ARGS__); \
+		check_msg((c), __VA_ARGS__); \
 	} while (0)
 
 static void check_nodes_props(struct check *c, struct node *dt, struct node *node)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply

* Re: ucc_uart: add support for Freescale QUICCEngine UART
From: Timur Tabi @ 2007-12-04 22:33 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: linuxppc-dev
In-Reply-To: <200712042313.58252.arnd@arndb.de>

Arnd Bergmann wrote:

> Can you use the driver on CPUs without this particular bug when it's built
> in Soft-UART mode?

No, you have to build with Soft-UART mode turned off.  Soft-UART mode actually 
uses the HMC mode of the QE and hacks it up to act like a UART.

The only way to know at runtime whether Soft-UART is required is to check the 
SOC model/revision and compare it to a list.

I could add code to check this list and enable Soft-UART if when there's a known 
CPU with the problem.  However, I can't know whether I need to upload the 
microcode.  I also have a U-Boot version of qe_upload_firmware(), so it's 
conceivable that U-Boot could have uploaded the microcode but I still need the 
Linux driver to use Soft-UART.

I also have no control over which Soft-UART microcodes are available.  It's 
possible that some SOCs could have this bug but Freescale won't produce a 
microcode to fix it.

> Try to reduce the number of #ifdefs in your code. In particular, you should
> not do conditional #includes and #defines, as they often lead to subtle bugs.

Ok, I can remove those #defines, but my main concern was to clearly isolate the 
Soft-UART code, since it is an ugly hack.  I didn't want to mesh the Soft-UART 
code with the normal UART code.  I don't know what more I could do to limit the 
number of #ifdefs.


>> +struct ucc_uart_pram {
>> +	struct ucc_slow_pram common;
>> +	u8 res1[8];     	/* reserved */
>> +	__be16 maxidl;  	/* Maximum idle chars */
>> +	__be16 idlc;    	/* temp idle counter */
>> +	__be16 brkcr;   	/* Break count register */
>> +	__be16 parec;   	/* receive parity error counter */
>> +	__be16 frmec;   	/* receive framing error counter */
>> +	__be16 nosec;   	/* receive noise counter */
>> +	__be16 brkec;   	/* receive break condition counter */
>> +	__be16 brkln;   	/* last received break length */
>> +	__be16 uaddr[2];	/* UART address character 1 & 2 */
>> +	__be16 rtemp;   	/* Temp storage */
>> +	__be16 toseq;   	/* Transmit out of sequence char */
>> +	__be16 cchars[8];       /* control characters 1-8 */
>> +	__be16 rccm;    	/* receive control character mask */
>> +	__be16 rccr;    	/* receive control character register */
>> +	__be16 rlbc;    	/* receive last break character */
>> +	__be16 res2;    	/* reserved */
>> +	__be32 res3;    	/* reserved, should be cleared */
>> +	u8 res4;		/* reserved, should be cleared */
>> +	u8 res5[3];     	/* reserved, should be cleared */
>> +	__be32 res6;    	/* reserved, should be cleared */
>> +	__be32 res7;    	/* reserved, should be cleared */
>> +	__be32 res8;    	/* reserved, should be cleared */
>> +	__be32 res9;    	/* reserved, should be cleared */
>> +	__be32 res10;   	/* reserved, should be cleared */
>> +	__be32 res11;   	/* reserved, should be cleared */
>> +	__be32 res12;   	/* reserved, should be cleared */
>> +	__be32 res13;   	/* reserved, should be cleared */
>> +#ifdef CONFIG_SERIAL_QE_SOFT_UART
>> +	__be16 supsmr;  	/* 0x90, Shadow UPSMR */
>> +	__be16 res92;   	/* 0x92, reserved, initialize to 0 */
>> +	__be32 rx_state;	/* 0x94, RX state, initialize to 0 */
>> +	__be32 rx_cnt;  	/* 0x98, RX count, initialize to 0 */
>> +	u8 rx_length;   	/* 0x9C, Char length, set to 1+CL+PEN+1+SL */
>> +	u8 rx_bitmark;  	/* 0x9D, reserved, initialize to 0 */
>> +	u8 rx_temp_dlst_qe;     /* 0x9E, reserved, initialize to 0 */
>> +	u8 res14[0xBC - 0x9F];  /* reserved */
>> +	__be32 dump_ptr;	/* 0xBC, Dump pointer */
>> +	__be32 rx_frame_rem;    /* 0xC0, reserved, initialize to 0 */
>> +	u8 rx_frame_rem_size;   /* 0xC4, reserved, initialize to 0 */
>> +	u8 tx_mode;     	/* 0xC5, mode, 0=AHDLC, 1=UART */
>> +	u16 tx_state;   	/* 0xC6, TX state */
>> +	u8 res15[0xD0 - 0xC8];  /* reserved */
>> +	__be32 resD0;   	/* 0xD0, reserved, initialize to 0 */
>> +	u8 resD4;       	/* 0xD4, reserved, initialize to 0 */
>> +	__be16 resD5;   	/* 0xD5, reserved, initialize to 0 */
>> +#endif
>> +} __attribute__ ((packed));
> 
> The structure is perfectly packed even without your __attribute__ ((packed)),
> so you should leave out the attribute in order to get more efficient code
> accessing it.

If the structure is packed either way, why would the code differ?  I don't see 
how the code would be more efficient if I removed "__attribute__ ((packed))". 
If gcc does something differently, that's news to me.

> Do you really need the debugging function like this in the code?
> Usually they are rather pointless once the code works, and will
> suffer from bitrot because nobody enables the code.

Well, I'm hesitant to remove it because I have a feeling that if I do, the next 
day I'll want it.

>> +#ifdef CONFIG_SERIAL_QE_SOFT_UART
>> +#define UCC_UART_SUPSMR_SL      	0x8000
>> +#define UCC_UART_SUPSMR_RPM_MASK	0x6000
>> +#define UCC_UART_SUPSMR_RPM_ODD 	0x0000
>> +#define UCC_UART_SUPSMR_RPM_LOW 	0x2000
> 
> again, the #ifdef should be left out if it can.

Alright.

> 
>> + * Given the virtual address for a character buffer, this function returns
>> + * the physical (DMA) equivalent.
>> + */
>> +static inline dma_addr_t cpu2qe_addr(void *addr, struct uart_qe_port *qe_port)
>> +{
>> +	if (likely((addr >= qe_port->bd_virt)) &&
>> +	    (addr < (qe_port->bd_virt + qe_port->bd_size)))
>> +		return qe_port->bd_phys + (addr - qe_port->bd_virt);
>> +
>> +	/* something nasty happened */
>> +	printk(KERN_ERR "%s: addr=%p\n", __FUNCTION__, addr);
>> +	BUG();
>> +	return 0;
>> +}
> 
> I'm guessing that you don't really mean dma_addr_t here, but rather
> phys_addr_t, which is something different.

Now that I think about it, I think I really mean unsigned, since the returned 
value is just an offset.  I need to think about that.

-- 
Timur Tabi
Linux kernel developer at Freescale

^ permalink raw reply

* Re: Merge dtc
From: David Woodhouse @ 2007-12-04 22:33 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: linuxppc-dev, David Gibson
In-Reply-To: <18261.53985.700234.923526@cargo.ozlabs.ibm.com>

On Wed, 2007-12-05 at 09:21 +1100, Paul Mackerras wrote:
> David Woodhouse writes:
> 
> > I think this is a bad idea -- it's hardly a difficult for those people
> > who _do_ need dts to obtain it separately.
> 
> The trouble is that it's not just people who are making a kernel for a
> specific embedded board that need dtc.  These days anyone who wants to
> try cross-compiling a powerpc kernel and does a make allyesconfig, or
> who picks cell_defconfig or ps3_defconfig to try, needs dtc if their
> kernel build is to go all the way through and give them an exit status
> of 0.  I can see that for people who are trying to do the right thing
> and compile-test their patch across architectures, it's annoying that
> powerpc has an extra external requirement when no other architecture
> does, and it usually just means they don't compile-test on powerpc.
> 
> Of the various options for solving this, including dtc in the kernel
> sources seems best to me.

Make vmlinux the default target instead of zImage would seem like a
better answer. I'm surprised that it isn't like that already, in fact --
and I'm only inferring that it isn't from your message. I always thought
that if I typed 'make' I got the vmlinux and not a zImage.

-- 
dwmw2

^ permalink raw reply

* Re: Problem compiling sequoia using DENX kernel. Xenomai-patch required?
From: Josh Boyer @ 2007-12-04 22:29 UTC (permalink / raw)
  To: niklaus.giger; +Cc: linuxppc-embedded
In-Reply-To: <200712042314.38498.niklaus.giger@member.fsf.org>

On Tue, 4 Dec 2007 23:14:38 +0100
Niklaus Giger <niklaus.giger@member.fsf.org> wrote:

> Am Dienstag, 4. Dezember 2007 schrieb Wolfgang Denk:
> <...>
> > I'm afraid "normal" here still means arch/ppc  -  hopefully  for  not
> > long any more. Note: a matching Xenomai patch for arch/ppc will be in
> > Xenomai 2.4 when it comes out in a few days.
> Thanks a lot for your explanation. I took 
> ksrc/arch/powerpc/patches/adeos-ipipe-2.6.23-ppc-1.6-00.patch 
> from the xenomai trunk and was able to compile and boot successfully
> the yosemite board (using ARCH=ppc). Will try the sequoaia board tomorrow.
> 
> I hope that this is a good starting point to get my custom PPC440EPx board 
> into shape. I think once the arch/powerpc will work for PPC440 it should not 
> be a lot of work to port it from ppc -> powerpc.

Sequoia is already in arch/powerpc.  At least most of the base support.

josh

^ permalink raw reply

* Re: Merge dtc
From: Paul Mackerras @ 2007-12-04 22:21 UTC (permalink / raw)
  To: David Woodhouse; +Cc: linuxppc-dev, David Gibson
In-Reply-To: <1196733544.13978.201.camel@pmac.infradead.org>

David Woodhouse writes:

> I think this is a bad idea -- it's hardly a difficult for those people
> who _do_ need dts to obtain it separately.

The trouble is that it's not just people who are making a kernel for a
specific embedded board that need dtc.  These days anyone who wants to
try cross-compiling a powerpc kernel and does a make allyesconfig, or
who picks cell_defconfig or ps3_defconfig to try, needs dtc if their
kernel build is to go all the way through and give them an exit status
of 0.  I can see that for people who are trying to do the right thing
and compile-test their patch across architectures, it's annoying that
powerpc has an extra external requirement when no other architecture
does, and it usually just means they don't compile-test on powerpc.

Of the various options for solving this, including dtc in the kernel
sources seems best to me.

Paul.

^ permalink raw reply

* Re: [FDT][PATCH] Print out the total size as part of ftdump
From: David Gibson @ 2007-12-04 22:20 UTC (permalink / raw)
  To: Kumar Gala; +Cc: linuxppc-dev, Jon Loeliger
In-Reply-To: <Pine.LNX.4.64.0712041032580.29317@blarg.am.freescale.net>

On Tue, Dec 04, 2007 at 10:33:20AM -0600, Kumar Gala wrote:
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

You know, the whole batch of fprintf()s of header information in
dt_from_blob() are really just a debugging hack that uglifies dtc's
output.  The whole lot could be moved over to ftdump instead, which
is, after all, *supposed* to be a debugging hack.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply

* Re: [FDT][PATCH] Fix padding options
From: Kumar Gala @ 2007-12-04 22:18 UTC (permalink / raw)
  To: David Gibson; +Cc: linuxppc-dev, Jon Loeliger
In-Reply-To: <20071204220432.GA2521@localhost.localdomain>

I guess it was really git commit  
2b7dc8dce549ad72ad437b254bf756d7ba4c2a5a

Add an option to pad the blob that is generated

- k

On Dec 4, 2007, at 4:04 PM, David Gibson wrote:

> On Tue, Dec 04, 2007 at 10:27:52AM -0600, Kumar Gala wrote:
>> commit 22e787ca2b1e49a9a0f3c43262564ab1038c5c3c broke the padding
>> support.  We were updating the fdt header after writing it.
>
> Uh.. I can't find a commit with that SHA1.  Which one are you
> referring to?
>
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_  
> _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

^ permalink raw reply

* Re: Problem compiling sequoia using DENX kernel. Xenomai-patch required?
From: Niklaus Giger @ 2007-12-04 22:14 UTC (permalink / raw)
  To: Wolfgang Denk; +Cc: linuxppc-embedded
In-Reply-To: <20071203230816.1C502242E9@gemini.denx.de>

Am Dienstag, 4. Dezember 2007 schrieb Wolfgang Denk:
<...>
> I'm afraid "normal" here still means arch/ppc  -  hopefully  for  not
> long any more. Note: a matching Xenomai patch for arch/ppc will be in
> Xenomai 2.4 when it comes out in a few days.
Thanks a lot for your explanation. I took 
ksrc/arch/powerpc/patches/adeos-ipipe-2.6.23-ppc-1.6-00.patch 
from the xenomai trunk and was able to compile and boot successfully
the yosemite board (using ARCH=ppc). Will try the sequoaia board tomorrow.

I hope that this is a good starting point to get my custom PPC440EPx board 
into shape. I think once the arch/powerpc will work for PPC440 it should not 
be a lot of work to port it from ppc -> powerpc.

Best regards

-- 
Niklaus Giger

^ permalink raw reply

* Re: ucc_uart: add support for Freescale QUICCEngine UART
From: Arnd Bergmann @ 2007-12-04 22:13 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Timur Tabi
In-Reply-To: <11967907173600-git-send-email-timur@freescale.com>

On Tuesday 04 December 2007, Timur Tabi wrote:
> Add support for UART serial ports using a Freescale QUICC Engine
> (found on some MPC83xx and MPC85xx SOCs).
> 
> Because of a silicon bug in some QE-enabled SOCs (e.g. 8323 and 8360), a new
> microcode is required. This microcode implements UART via a work-around,
> hence it's called "Soft-UART".  This driver can use QE firmware upload feature
> to upload the correct microcode to the QE.

Can you use the driver on CPUs without this particular bug when it's built
in Soft-UART mode?

> +
> +#ifdef CONFIG_SERIAL_QE_SOFT_UART_UPLOAD
> +#include <linux/firmware.h>
> +#include <asm/reg.h>
> +#endif
> +
> +#ifdef CONFIG_SERIAL_QE_SOFT_UART
> +/*
> + * The GUMR flag for Soft UART.  This would normally be defined in qe.h,
> + * but Soft-UART is a hack and we want to keep everything related to it in
> + * this file.
> + */
> +#define UCC_SLOW_GUMR_H_SUART   	0x00004000      /* Soft UART */
> +
> +/*
> + * firmware_loaded is 1 if the firmware has been loaded, 0 otherwise.
> + */
> +#ifdef CONFIG_SERIAL_QE_SOFT_UART_UPLOAD
> +static int firmware_loaded;
> +#endif

Try to reduce the number of #ifdefs in your code. In particular, you should
not do conditional #includes and #defines, as they often lead to subtle bugs.

> +struct ucc_uart_pram {
> +	struct ucc_slow_pram common;
> +	u8 res1[8];     	/* reserved */
> +	__be16 maxidl;  	/* Maximum idle chars */
> +	__be16 idlc;    	/* temp idle counter */
> +	__be16 brkcr;   	/* Break count register */
> +	__be16 parec;   	/* receive parity error counter */
> +	__be16 frmec;   	/* receive framing error counter */
> +	__be16 nosec;   	/* receive noise counter */
> +	__be16 brkec;   	/* receive break condition counter */
> +	__be16 brkln;   	/* last received break length */
> +	__be16 uaddr[2];	/* UART address character 1 & 2 */
> +	__be16 rtemp;   	/* Temp storage */
> +	__be16 toseq;   	/* Transmit out of sequence char */
> +	__be16 cchars[8];       /* control characters 1-8 */
> +	__be16 rccm;    	/* receive control character mask */
> +	__be16 rccr;    	/* receive control character register */
> +	__be16 rlbc;    	/* receive last break character */
> +	__be16 res2;    	/* reserved */
> +	__be32 res3;    	/* reserved, should be cleared */
> +	u8 res4;		/* reserved, should be cleared */
> +	u8 res5[3];     	/* reserved, should be cleared */
> +	__be32 res6;    	/* reserved, should be cleared */
> +	__be32 res7;    	/* reserved, should be cleared */
> +	__be32 res8;    	/* reserved, should be cleared */
> +	__be32 res9;    	/* reserved, should be cleared */
> +	__be32 res10;   	/* reserved, should be cleared */
> +	__be32 res11;   	/* reserved, should be cleared */
> +	__be32 res12;   	/* reserved, should be cleared */
> +	__be32 res13;   	/* reserved, should be cleared */
> +#ifdef CONFIG_SERIAL_QE_SOFT_UART
> +	__be16 supsmr;  	/* 0x90, Shadow UPSMR */
> +	__be16 res92;   	/* 0x92, reserved, initialize to 0 */
> +	__be32 rx_state;	/* 0x94, RX state, initialize to 0 */
> +	__be32 rx_cnt;  	/* 0x98, RX count, initialize to 0 */
> +	u8 rx_length;   	/* 0x9C, Char length, set to 1+CL+PEN+1+SL */
> +	u8 rx_bitmark;  	/* 0x9D, reserved, initialize to 0 */
> +	u8 rx_temp_dlst_qe;     /* 0x9E, reserved, initialize to 0 */
> +	u8 res14[0xBC - 0x9F];  /* reserved */
> +	__be32 dump_ptr;	/* 0xBC, Dump pointer */
> +	__be32 rx_frame_rem;    /* 0xC0, reserved, initialize to 0 */
> +	u8 rx_frame_rem_size;   /* 0xC4, reserved, initialize to 0 */
> +	u8 tx_mode;     	/* 0xC5, mode, 0=AHDLC, 1=UART */
> +	u16 tx_state;   	/* 0xC6, TX state */
> +	u8 res15[0xD0 - 0xC8];  /* reserved */
> +	__be32 resD0;   	/* 0xD0, reserved, initialize to 0 */
> +	u8 resD4;       	/* 0xD4, reserved, initialize to 0 */
> +	__be16 resD5;   	/* 0xD5, reserved, initialize to 0 */
> +#endif
> +} __attribute__ ((packed));

The structure is perfectly packed even without your __attribute__ ((packed)),
so you should leave out the attribute in order to get more efficient code
accessing it.

> +
> +#ifdef DEBUG
> +static void dump_ucc_uart_pram(struct ucc_uart_pram __iomem *uccup)
> +{
> +	unsigned int i;

Do you really need the debugging function like this in the code?
Usually they are rather pointless once the code works, and will
suffer from bitrot because nobody enables the code.

> +
> +#ifdef CONFIG_SERIAL_QE_SOFT_UART
> +#define UCC_UART_SUPSMR_SL      	0x8000
> +#define UCC_UART_SUPSMR_RPM_MASK	0x6000
> +#define UCC_UART_SUPSMR_RPM_ODD 	0x0000
> +#define UCC_UART_SUPSMR_RPM_LOW 	0x2000

again, the #ifdef should be left out if it can.

> + * Given the virtual address for a character buffer, this function returns
> + * the physical (DMA) equivalent.
> + */
> +static inline dma_addr_t cpu2qe_addr(void *addr, struct uart_qe_port *qe_port)
> +{
> +	if (likely((addr >= qe_port->bd_virt)) &&
> +	    (addr < (qe_port->bd_virt + qe_port->bd_size)))
> +		return qe_port->bd_phys + (addr - qe_port->bd_virt);
> +
> +	/* something nasty happened */
> +	printk(KERN_ERR "%s: addr=%p\n", __FUNCTION__, addr);
> +	BUG();
> +	return 0;
> +}

I'm guessing that you don't really mean dma_addr_t here, but rather
phys_addr_t, which is something different.

> +
> +/*
> + * Physical to virtual address translation.
> + *
> + * Given the physical (DMA) address for a character buffer, this function
> + * returns the virtual equivalent.
> + */
> +static inline void *qe2cpu_addr(dma_addr_t addr, struct uart_qe_port *qe_port)

same here.

	Arnd <><

^ permalink raw reply

* Re: Merge dtc
From: David Gibson @ 2007-12-04 22:12 UTC (permalink / raw)
  To: Kumar Gala
  Cc: linuxppc-dev, Jon Loeliger, Paul Mackerras, David Woodhouse, Paul
In-Reply-To: <E38C9A55-77FC-46DE-AE30-A03A244656EB@kernel.crashing.org>

On Tue, Dec 04, 2007 at 10:04:53AM -0600, Kumar Gala wrote:
> 
> On Dec 4, 2007, at 9:26 AM, Josh Boyer wrote:
> 
> > On Tue, 04 Dec 2007 07:25:57 -0600
> > Jon Loeliger <jdl@jdl.com> wrote:
> >
> >> So, like, the other day David Woodhouse mumbled:
> >>>
> >>> I think this is a bad idea -- it's hardly a difficult for those  
> >>> people
> >>> who _do_ need dts to obtain it separately.
> >>>
> >>> We shouldn't be merging _more_ stuff in.
> >>
> >> Thanks for chiming in here, David W.  As far as I can tell
> >> so far, the only two people who have voiced an opinion on
> >> this issue are Dave G, submitting patches, and me disagreeing
> >> with the approach.  :-)
> >>
> >> Anyone else?
> >
> > I don't see an overwhelmingly great reason to merge it.  It might help
> > test people who do automated rebuilds, etc and aren't used to dealing
> > with powerpc and it's requirements.  Outside of that, I see it as
> > dual-maintenance.
> >
> > But I'm not doing the maintenance, and it doesn't effect me too much.
> > I only ask that a decision is made and executed on soon so we can move
> > on.
> 
> I'm also in disagreement of duplicating dtc in the kernel.
> 
> However, if we are going to do this we should make the path expansion  
> for labels work before we do it.

Since we're going to have to update the in-kernel copy reasonably
frequently anyway, I don't see that there's much point in waiting for
particular features to be implemented.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply

* Re: [PATCH v2 2/2] [POWERPC] Use new machine_xxx_initcall hooks in platform code
From: Arnd Bergmann @ 2007-12-04 22:05 UTC (permalink / raw)
  To: benh; +Cc: Geert Uytterhoeven, linuxppc-dev, olof
In-Reply-To: <1196800273.13230.333.camel@pasglop>

On Tuesday 04 December 2007, Benjamin Herrenschmidt wrote:
> > 2. The call to firmware_has_feature() turns into a compile-time check
> > in
> > many cases, so if the kernel does not contain support for any firmware
> > with the given feature, all the code referenced it can get optimized
> > away by the compiler.
> 
> The machine init stuff will soon get rid of whatever test is in it too,
> as soon as I get to do the ELF magic.

Section magic is often painful and causes a number of problems. Moreover, it
won't do the same thing as what the compiler can do. Consider

static int x __attribute__((section("some_platform.data"))) = 1;
static int f(void) __attribute__((section("some_platform.text")));
{
	if (firmware_has_feature(FOO))
		return x;
	return 0;
}

When firmware_has_feature(FOO) statically evaluates to false, f becomes an
empty function and x is left out from the object file.
If you turn it into a platform_is() check and discard all "some_platform"
sections, you need to have both in the vmlinux file and can discard them
at run time, which is something completely different. You can also
discard them in the linker script, but that's a lot more work than having
the compiler do the right thing.

	Arnd <><

^ permalink raw reply

* Re: [FDT][PATCH] Fix padding options
From: David Gibson @ 2007-12-04 22:04 UTC (permalink / raw)
  To: Kumar Gala; +Cc: linuxppc-dev, Jon Loeliger
In-Reply-To: <Pine.LNX.4.64.0712041027330.29174@blarg.am.freescale.net>

On Tue, Dec 04, 2007 at 10:27:52AM -0600, Kumar Gala wrote:
> commit 22e787ca2b1e49a9a0f3c43262564ab1038c5c3c broke the padding
> support.  We were updating the fdt header after writing it.

Uh.. I can't find a commit with that SHA1.  Which one are you
referring to?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply

* Re: [PATCH v2 2/2] [POWERPC] Use new machine_xxx_initcall hooks in platform code
From: Arnd Bergmann @ 2007-12-04 21:52 UTC (permalink / raw)
  To: benh; +Cc: Geert Uytterhoeven, linuxppc-dev, olof
In-Reply-To: <1196800273.13230.333.camel@pasglop>

On Tuesday 04 December 2007, Benjamin Herrenschmidt wrote:
> On Tue, 2007-12-04 at 20:35 +0100, Arnd Bergmann wrote:
> > 
> > 1. If another platform gets added that uses the same firmware feature,
> > it
> > will automatically do the right thing.
> 
> Yes but is it something that we want to happen ? That is, do we want
> code somewhere in a platform/foo dir to run when using platform/bar
> because they happen to share a feature ?

It should be decided case-by-case of course. My assumption was that
in those places where we currently check the firmware feature, that
is actually the right thing to do.

For the lv1 specific device drivers (vuart, sound, graphics, ...),
my feeling is that they should really check for lv1, not for ps3,
because the check for lv1 is only needed in order to make sure
that you can run the hcall that probes for the actual device.

For some of the iseries checks, it would be more logical to test
the platform instead of the firmware feature IMHO, but I don't see a
significant reason to change that.

	Arnd <><

^ permalink raw reply

* Re: [PATCH v2 2/2] [POWERPC] Use new machine_xxx_initcall hooks in platform code
From: Geoff Levand @ 2007-12-04 20:59 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: Linux/PPC Development, olof
In-Reply-To: <Pine.LNX.4.62.0712041442530.12181@pademelon.sonytel.be>

Geert Uytterhoeven wrote:
> On Tue, 4 Dec 2007, Grant Likely wrote:
>> On 12/4/07, Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
>> > On Sat, 1 Dec 2007, Grant Likely wrote:
>> > > From: Grant Likely <grant.likely@secretlab.ca>
>> > >
>> > > This patch makes the platform code use the new machine-specific initcall
>> > > hooks.  This has the advantage of not needing to explicitly test
>> > > machine_is() at the top of every initcall function.
>> >
>> > You seem to have missed the PS3 *_initcall()s.
>> > Probably because they test for firmware_has_feature(FW_FEATURE_PS3_LV1) instead
>> > of machine_is(ps3).
>> 
>> That's exactly why; I didn't know if 'machine_is(ps3)' was a suitable
>> substitute so I left it alone.
> 
> I think it's OK. But...
> 
> Geoff: is there any specific reason why you used
> firmware_has_feature(FW_FEATURE_PS3_LV1)?

As Arnd pointed out, the code in the firmware_has_feature() conditional
will be removed by the optimizer when the kernel is built without support
for that feature.  This then gives a multi-platform binary that has only
the code for the sub-set of features the user selected. 

-Geoff 

^ permalink raw reply

* Re: [PATCH] pata_of_platform: Move electra-ide support over to new framework
From: Olof Johansson @ 2007-12-04 21:03 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: Arnd Bergmann, linux-ide, linuxppc-dev, Paul Mundt
In-Reply-To: <4755BD7B.6030208@garzik.org>

On Tue, Dec 04, 2007 at 03:50:03PM -0500, Jeff Garzik wrote:
> Olof Johansson wrote:
>> [POWERPC] Move electra-ide support over to new pata_of_platform framework
>> Move electra-ide glue over to the new pata_of_platform framework, and
>> add the quirks needed to that driver.
>> Signed-off-by: Olof Johansson <olof@lixom.net>
>> ---
>> I'll remove the electra-ide stuff from arch/powerpc/platforms/pasemi
>> once this hits a common tree, since otherwise I'd be without IDE until
>> they converge (i.e.  2.6.25 merge window).
>
> FWIW I'm presuming this work will go via a powerpc tree not libata... 
> Generally that's not the case, but here it's largely an arch-specific work.

Ok, that works. I was thinking of letting this patch go through libata,
and do the removal through the powerpc merge path. But I can do both
through powerpc.

-Olof

^ permalink raw reply

* Re: [PATCH] [POWERPC] Xilinx: clear data caches.
From: Grant Likely @ 2007-12-04 20:56 UTC (permalink / raw)
  To: Stephen Neuendorffer; +Cc: linuxppc-dev
In-Reply-To: <20071204204915.AF346C18071@mail45-blu.bigfish.com>

On 12/4/07, Stephen Neuendorffer <stephen.neuendorffer@xilinx.com> wrote:
> This code is needed to boot without a boot loader.
>
> Grant:  I'm not sure where the right place to put this is.  I'm assuming we'll actually need some boot code that is not generic?  Also, note that there is a V4FX errata workaround in arch/ppc/boot/head.S, which probably also needs to get pulled to powerpc.

Thanks Steve, I'll pull this into my tree.  I hope to find some time
to get the raw platform cleaned up to get into mainline.

And, yes, the V4FX errata needs to be merged into arch/powerpc.

Thanks again!
g.

>
> Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
> ---
>  arch/powerpc/boot/raw-platform.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/boot/raw-platform.c b/arch/powerpc/boot/raw-platform.c
> index b9caeee..2a5e493 100644
> --- a/arch/powerpc/boot/raw-platform.c
> +++ b/arch/powerpc/boot/raw-platform.c
> @@ -24,6 +24,28 @@ void platform_init(unsigned long r3, unsigned long r4, unsigned long r5,
>                     unsigned long r6, unsigned long r7)
>  {
>         u64 memsize64 = memsize[0];
> +       static const unsigned long line_size = 32;
> +       static const unsigned long congruence_classes = 256;
> +       unsigned long addr;
> +       unsigned long dccr;
> +
> +       /*
> +        * Invalidate the data cache if the data cache is turned off.
> +        * - The 405 core does not invalidate the data cache on power-up
> +        *   or reset but does turn off the data cache. We cannot assume
> +        *   that the cache contents are valid.
> +        * - If the data cache is turned on this must have been done by
> +        *   a bootloader and we assume that the cache contents are
> +        *   valid.
> +        */
> +       __asm__("mfdccr %0": "=r" (dccr));
> +       if (dccr == 0) {
> +               for (addr = 0;
> +                    addr < (congruence_classes * line_size);
> +                    addr += line_size) {
> +                       __asm__("dccci 0,%0": :"b"(addr));
> +               }
> +       }
>
>         if (mem_size_cells == 2) {
>                 memsize64 <<= 32;
> --
> 1.5.3.4-dirty
>
>
>
>


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

^ permalink raw reply

* Re: [BUG] 2.6.24-rc3-git2 softlockup detected
From: Ingo Molnar @ 2007-12-04 20:55 UTC (permalink / raw)
  To: Kamalesh Babulal
  Cc: Rafael J. Wysocki, linux-scsi, Matthew Wilcox, LKML,
	Kyle McMartin, linuxppc-dev, Andrew Morton, Balbir Singh
In-Reply-To: <47556AE5.8010708@linux.vnet.ibm.com>


* Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> wrote:

> Hi Ingo,
> 
> This softlockup is seen in the 2.6.24-rc4 either and looks like a 
> message because this is seen while running tbench and machine 
> continues running other test's after the softlockup messages and some 
> times seen with the bootup, but the machines reaches the login prompt 
> and able to continue running tests.

do you know whether there's any true delay when this happens, or is it a 
pure softlockup-detector false positive?

	Ingo

^ permalink raw reply

* Re: [PATCH] pata_of_platform: Move electra-ide support over to new framework
From: Jeff Garzik @ 2007-12-04 20:50 UTC (permalink / raw)
  To: Olof Johansson; +Cc: Arnd Bergmann, linux-ide, linuxppc-dev, Paul Mundt
In-Reply-To: <20071204204432.GD8099@lixom.net>

Olof Johansson wrote:
> [POWERPC] Move electra-ide support over to new pata_of_platform framework
> 
> Move electra-ide glue over to the new pata_of_platform framework, and
> add the quirks needed to that driver.
> 
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> 
> ---
> 
> I'll remove the electra-ide stuff from arch/powerpc/platforms/pasemi
> once this hits a common tree, since otherwise I'd be without IDE until
> they converge (i.e.  2.6.25 merge window).

FWIW I'm presuming this work will go via a powerpc tree not libata... 
Generally that's not the case, but here it's largely an arch-specific work.

	Jeff

^ 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