* Re: [patch 02/10] powerpc: Move CONFIG_PPC64 into CPU selection
From: Segher Boessenkool @ 2007-06-13 4:57 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, Arnd Bergmann
In-Reply-To: <18031.15747.57370.99388@cargo.ozlabs.ibm.com>
>> It would be good to converge the 32- and 64-bit implementations,
>> and/or allow a single kernel image to work on both 32-bit and
>> 64-bit systems, The current situation is far removed from that,
>> so this patch is a bit premature in that aspect.
>
> There will always be differences in emphasis and focus between 32-bit
> and 64-bit. For example, the vast majority of 32-bit systems are UP,
> with a few 2-way SMP systems, an insignificant number of 4-way
> systems, and nothing bigger than 4-way. In contrast, most 64-bit
> machines are SMP, with enough large SMP machines (64-way and bigger)
> to be worth worrying about. Hence the 64-bit code focuses on SMP
> scalability whereas the 32-bit code focuses on performance for UP and
> 2-way systems - which is why the hash table invalidations get batched
> up on 64-bit machines but not on 32-bit machines, for instance.
> Similarly, 64-bit supports large memory (>4GB) and NUMA machines,
> whereas 32-bit doesn't.
While all this is certainly true, there still are many
differences between 32-bit and 64-bit that aren't really
warranted. Also, things like the scalability "knobs"
should perhaps not be directly keyed of the PPC64 symbol.
Anyway, nothing to worry about now I guess ;-)
Segher
^ permalink raw reply
* Re: [PATCH 1/5] Add the explanation and sample of RapidIO DTS sector to the document of booting-without-of.txt file.
From: Segher Boessenkool @ 2007-06-13 5:02 UTC (permalink / raw)
To: Zhang Wei; +Cc: linuxppc-dev, paulus, linux-kernel
In-Reply-To: <1181638939310-git-send-email-wei.zhang@freescale.com>
> + k) RapidIO
> +
> + Required properties:
> +
> + - device_type : Should be "rapidio"
There is no OF binding, so no.
> + - compatible : Should be "fsl,rapidio-v0.0" or "fsl,rapidio-v1.0"
> + and so on. The version number is got from IP Block Revision
> + Register of RapidIO controller.
It's better to use real device names, just like everyone
else.
> + - #address-cells : Address representation for "rapidio" devices.
> + This field represents the number of cells needed to represent
> + the RapidIO address of the registers. For supporting more than
> + 36-bits RapidIO address, this field should be <2>.
More than 32 bit?
> + - interrupt-parent : the phandle for the interrupt controller that
> + services interrupts for this device.
Not required, depends on the interrupt tree of the system.
> + - interrupts : <a b> where a is the interrupt number and b is a
> + field that represents an encoding of the sense and level
> + information for the interrupt.
No. The format of an "interrupts" entry is defined by
the interrupt domain this device sits in, not by the
device itself.
> For this sector, interrupts order should be
> + <err_irq bell_outb_irq bell_inb_irq msg1_tx_irq msg1_rx_irq
> + msg2_tx_irq msg2_rx_irq ... msgN_tx_irq msgN_rx_irq>.
That's to be defined in the binding for your specific device,
not in a more generic rapidio binding.
> + #address-cells = <2>;
You want a #size-cells as well.
Hope this helps,
Segher
^ permalink raw reply
* Re: [PATCH] powerpc: Create "rom" (MTD) device prpmc2800
From: Segher Boessenkool @ 2007-06-13 5:05 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev, linux-mtd, David Woodhouse, Milton Miller
In-Reply-To: <20070613031615.GC16148@localhost.localdomain>
>>> Of course, Ebony has flash which is JEDEC rather than CFI probed, so
>>> I
>>> have an interest in a binding for that, too.
>>
>> For JEDEC flash, you probably want extra properties
>> for manufacturing ID and chip ID. Also, you need
>> to describe the block sizes in some way (on CFI, this
>> can be probed; on JEDEC, not). Anything else?
>
> Hrm. At the moment it appears to work without supplying this
> information for the Ebony flash... I wonder if there are defaults in
> the mtd layer or something.
MTD does a lot of heuristic probing. It is pretty good,
but it would be nice if you could skip that and hand it
the exact values it needs. Also, this information should
be in the device tree no matter what, even if Linux
doesn't use it yet ;-)
Segher
^ permalink raw reply
* Re: [0/13] Queued patches for 2.6.13
From: Segher Boessenkool @ 2007-06-13 5:10 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <20070613045031.GD16148@localhost.localdomain>
> Here's my current queue of patches which I'm hoping will go in for
> 2.6.13. If they could go into an appropriate branch of powerpc.git in
> the meantime that would be great.
2.6.13?!?
Segher
^ permalink raw reply
* Re: [PATCH v3]: Fix e500 v2 core reboot bug
From: Zang Roy-r61911 @ 2007-06-13 5:45 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <f81c6f149f20c52f547f39ca0cc8d67c@kernel.crashing.org>
On Wed, 2007-06-13 at 12:44, Segher Boessenkool wrote:
> > + global-utilities@e0000 { //global utilities reg
> > + compatible = "fsl,mpc8548-guts";
> > + reg = <e0000 1000>;
> > + fsl,has-rstcr;
> > + };
>
> Looks good.
>
> > + np = of_find_node_by_name(NULL, "global-utilities");
> > + if ((np && of_get_property(np, "fsl,has-rstcr", NULL))) {
> > + const u32 *prop = of_get_property(np, "reg", NULL);
> > + if (prop) {
> > + /* map reset control register
> > + * 0xE00B0 is offset of reset control register
> > + */
> > + rstcr = ioremap(get_immrbase() + *prop + 0xB0,
> 0xff);
> > + if (!rstcr)
> > + printk (KERN_EMERG "Error: reset
> control register not mapped,
> > spinning!\n");
> > + }
> > + else
> > + printk (KERN_INFO "rstcr compatible register does not
> exist!\n");
> > + }
>
> This needs an of_node_put(np). Oh and the indenting
> is off.
It is my oversight!
I had thought to add of_node_put(), but forget.
I just checkout the code and decide to have a small revise.
Thanks.
Roy
^ permalink raw reply
* Re: [PATCH 6/6] [C67x00] Merge c67x00-hub.c into c67x00-hcd.c
From: Peter Korsgaard @ 2007-06-13 5:58 UTC (permalink / raw)
To: Grant Likely; +Cc: linux-usb-devel, linuxppc-embedded
In-Reply-To: <11816893511659-git-send-email-grant.likely@secretlab.ca>
>>>>> "Grant" == Grant Likely <grant.likely@secretlab.ca> writes:
Hi,
Grant> Rather than c67x00-hub.c being compiled seperately, the
Grant> original code had c67x00-hub.c *included* by c67x00-hcd.c.
Grant> This is a very bad idea. Simplest solution is to merge the
Grant> two files into one and be done with it.
Yeah, it isn't exactly pretty, but it's what the other hcd drivers do,
E.G.:
% grep -rs "include.*hub.c" *c
ehci-hcd.c:#include "ehci-hub.c"
ohci-hcd.c:#include "ohci-hub.c"
uhci-hcd.c:#include "uhci-hub.c"
I don't quite know why they do it like that though ..
--
Bye, Peter Korsgaard
^ permalink raw reply
* Re: [0/13] Queued patches for 2.6.23
From: David Gibson @ 2007-06-13 6:09 UTC (permalink / raw)
To: Paul Mackerras, linuxppc-dev
In-Reply-To: <20070613045031.GD16148@localhost.localdomain>
On Wed, Jun 13, 2007 at 02:50:31PM +1000, David Gibson wrote:
> Hi Paul,
>
> Here's my current queue of patches which I'm hoping will go in for
> 2.6.13. If they could go into an appropriate branch of powerpc.git in
> the meantime that would be great.
Blah. That should be 2.6.23, of course.
<looks sheepish>
--
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] powerpc: Create "rom" (MTD) device prpmc2800
From: David Gibson @ 2007-06-13 6:11 UTC (permalink / raw)
To: Segher Boessenkool
Cc: linuxppc-dev, linux-mtd, David Woodhouse, Milton Miller
In-Reply-To: <96f5b23c4f53e986e8caa2156f149cd8@kernel.crashing.org>
On Wed, Jun 13, 2007 at 07:05:31AM +0200, Segher Boessenkool wrote:
> >>> Of course, Ebony has flash which is JEDEC rather than CFI probed, so
> >>> I
> >>> have an interest in a binding for that, too.
> >>
> >> For JEDEC flash, you probably want extra properties
> >> for manufacturing ID and chip ID. Also, you need
> >> to describe the block sizes in some way (on CFI, this
> >> can be probed; on JEDEC, not). Anything else?
> >
> > Hrm. At the moment it appears to work without supplying this
> > information for the Ebony flash... I wonder if there are defaults in
> > the mtd layer or something.
>
> MTD does a lot of heuristic probing. It is pretty good,
> but it would be nice if you could skip that and hand it
> the exact values it needs. Also, this information should
> be in the device tree no matter what, even if Linux
> doesn't use it yet ;-)
I agree. Of course, this means figuring out what the values actually
are for Ebony. I presently have no idea, since the heuristic probing
appears to work without problem.
--
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] powerpc: Create "rom" (MTD) device prpmc2800
From: David Gibson @ 2007-06-13 6:12 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev
In-Reply-To: <9fbd7a7f5cdde58768569ab23c7aec7c@kernel.crashing.org>
On Tue, Jun 12, 2007 at 12:50:25PM +0200, Segher Boessenkool wrote:
> > So, what you're suggesting is a subnode for each described partition?
>
> I'm saying this is a reasonable way to describe the regions
> of flash the firmware itself cares about.
>
> This isn't anything new; it is done like this on some
> Apple systems, for example.
>
> > Seems an awfully verbose way of going about it,
>
> Not verbose, but flexible, and in line with everything
> else about the device tree.
>
> > and I don't see what
> > it buys us over the partitions/partition-names pair of properties.
>
> It is extensible. It makes parsing trivial. It
> represents a flash partition in a way similar to how
> a "whole" flash device is represented.
Hrm, ok. Between you and BenH you've convinced me. I'll try to come
up with something. I think we'll still need to support the old
properties as backwards compatibility thing.
--
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 v3]: Fix e500 v2 core reboot bug
From: Zang Roy-r61911 @ 2007-06-13 6:16 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev list
In-Reply-To: <f81c6f149f20c52f547f39ca0cc8d67c@kernel.crashing.org>
From: roy zang <tie-fei.zang@freescale.com>
Fix the reset bug on 8548CDS board.
>From MPC8548CDS with e500 v2 core, a new reset control register is added.
This register is used for the cpu reset.
Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
---
Please pick up this one.
Thanks for Segher's comment!
arch/powerpc/boot/dts/mpc8548cds.dts | 6 ++++++
arch/powerpc/platforms/85xx/misc.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 37 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/boot/dts/mpc8548cds.dts b/arch/powerpc/boot/dts/mpc8548cds.dts
index ad96381..0550a3c 100644
--- a/arch/powerpc/boot/dts/mpc8548cds.dts
+++ b/arch/powerpc/boot/dts/mpc8548cds.dts
@@ -177,6 +177,12 @@
interrupt-parent = <&mpic>;
};
+ global-utilities@e0000 { //global utilities reg
+ compatible = "fsl,mpc8548-guts";
+ reg = <e0000 1000>;
+ fsl,has-rstcr;
+ };
+
pci1: pci@8000 {
interrupt-map-mask = <1f800 0 0 7>;
interrupt-map = <
diff --git a/arch/powerpc/platforms/85xx/misc.c b/arch/powerpc/platforms/85xx/misc.c
index 3e62fcb..76f66d4 100644
--- a/arch/powerpc/platforms/85xx/misc.c
+++ b/arch/powerpc/platforms/85xx/misc.c
@@ -13,11 +13,42 @@
#include <linux/irq.h>
#include <linux/module.h>
#include <asm/irq.h>
+#include <asm/io.h>
+#include <asm/prom.h>
+#include <sysdev/fsl_soc.h>
+
+static __be32 __iomem *rstcr;
extern void abort(void);
+static int __init mpc85xx_rstcr(void)
+{
+ struct device_node *np;
+ np = of_find_node_by_name(NULL, "global-utilities");
+ if ((np && of_get_property(np, "fsl,has-rstcr", NULL))) {
+ const u32 *prop = of_get_property(np, "reg", NULL);
+ if (prop) {
+ /* map reset control register
+ * 0xE00B0 is offset of reset control register
+ */
+ rstcr = ioremap(get_immrbase() + *prop + 0xB0, 0xff);
+ if (!rstcr)
+ printk (KERN_EMERG "Error: reset control \
+ register not mapped, spinning!\n");
+ }
+ of_node_put(np);
+ } else
+ printk (KERN_INFO "rstcr compatible register does not exist!\n");
+ return 0;
+}
+
+arch_initcall(mpc85xx_rstcr);
+
void mpc85xx_restart(char *cmd)
{
local_irq_disable();
+ if (rstcr)
+ /* set reset control register */
+ out_be32(rstcr, 0x2); /* HRESET_REQ */
abort();
}
--
1.5.1
^ permalink raw reply related
* Re: [PATCH v3]: Fix e500 v2 core reboot bug
From: Zang Roy-r61911 @ 2007-06-13 6:24 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <1181713501.9397.10.camel@localhost.localdomain>
From: roy zang <tie-fei.zang@freescale.com>
Fix the reset bug on 8548CDS board.
>From MPC8548CDS with e500 v2 core, a new reset control register is added.
This register is used for the cpu reset.
Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
---
Please ignore the mail sent just now and pick up this one.
Sorry for the garbage!
Thanks for Segher's comment.
arch/powerpc/boot/dts/mpc8548cds.dts | 6 ++++++
arch/powerpc/platforms/85xx/misc.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 37 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/boot/dts/mpc8548cds.dts b/arch/powerpc/boot/dts/mpc8548cds.dts
index ad96381..0550a3c 100644
--- a/arch/powerpc/boot/dts/mpc8548cds.dts
+++ b/arch/powerpc/boot/dts/mpc8548cds.dts
@@ -177,6 +177,12 @@
interrupt-parent = <&mpic>;
};
+ global-utilities@e0000 { //global utilities reg
+ compatible = "fsl,mpc8548-guts";
+ reg = <e0000 1000>;
+ fsl,has-rstcr;
+ };
+
pci1: pci@8000 {
interrupt-map-mask = <1f800 0 0 7>;
interrupt-map = <
diff --git a/arch/powerpc/platforms/85xx/misc.c b/arch/powerpc/platforms/85xx/misc.c
index 3e62fcb..76f66d4 100644
--- a/arch/powerpc/platforms/85xx/misc.c
+++ b/arch/powerpc/platforms/85xx/misc.c
@@ -13,11 +13,42 @@
#include <linux/irq.h>
#include <linux/module.h>
#include <asm/irq.h>
+#include <asm/io.h>
+#include <asm/prom.h>
+#include <sysdev/fsl_soc.h>
+
+static __be32 __iomem *rstcr;
extern void abort(void);
+static int __init mpc85xx_rstcr(void)
+{
+ struct device_node *np;
+ np = of_find_node_by_name(NULL, "global-utilities");
+ if ((np && of_get_property(np, "fsl,has-rstcr", NULL))) {
+ const u32 *prop = of_get_property(np, "reg", NULL);
+ if (prop) {
+ /* map reset control register
+ * 0xE00B0 is offset of reset control register
+ */
+ rstcr = ioremap(get_immrbase() + *prop + 0xB0, 0xff);
+ if (!rstcr)
+ printk (KERN_EMERG "Error: reset control \
+ register not mapped, spinning!\n");
+ }
+ of_node_put(np);
+ } else
+ printk (KERN_INFO "rstcr compatible register does not exist!\n");
+ return 0;
+}
+
+arch_initcall(mpc85xx_rstcr);
+
void mpc85xx_restart(char *cmd)
{
local_irq_disable();
+ if (rstcr)
+ /* set reset control register */
+ out_be32(rstcr, 0x2); /* HRESET_REQ */
abort();
}
--
1.5.1
^ permalink raw reply related
* Re: [PATCH v3]: Fix e500 v2 core reboot bug
From: Segher Boessenkool @ 2007-06-13 6:28 UTC (permalink / raw)
To: Zang Roy-r61911; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <1181715391.9397.16.camel@localhost.localdomain>
> + np = of_find_node_by_name(NULL, "global-utilities");
> + if ((np && of_get_property(np, "fsl,has-rstcr", NULL))) {
If this of_get_property fails...
> + const u32 *prop = of_get_property(np, "reg", NULL);
> + if (prop) {
> + /* map reset control register
> + * 0xE00B0 is offset of reset control register
> + */
> + rstcr = ioremap(get_immrbase() + *prop + 0xB0, 0xff);
> + if (!rstcr)
> + printk (KERN_EMERG "Error: reset control \
> + register not mapped, spinning!\n");
> + }
> + of_node_put(np);
...you still leak a reference.
Segher
^ permalink raw reply
* Re: [PATCH v3]: Fix e500 v2 core reboot bug
From: Zang Roy-r61911 @ 2007-06-13 6:31 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <4a8562ccb7046ccde1ab81c725139878@kernel.crashing.org>
On Wed, 2007-06-13 at 14:28, Segher Boessenkool wrote:
> > + np = of_find_node_by_name(NULL, "global-utilities");
> > + if ((np && of_get_property(np, "fsl,has-rstcr", NULL))) {
>
> If this of_get_property fails...
>
> > + const u32 *prop = of_get_property(np, "reg", NULL);
> > + if (prop) {
> > + /* map reset control register
> > + * 0xE00B0 is offset of reset control register
> > + */
> > + rstcr = ioremap(get_immrbase() + *prop + 0xB0,
> 0xff);
> > + if (!rstcr)
> > + printk (KERN_EMERG "Error: reset
> control \
> > + register not mapped,
> spinning!\n");
> > + }
> > + of_node_put(np);
>
> ...you still leak a reference.
You are right!!!
Roy
^ permalink raw reply
* Re: [PATCH v3]: Fix e500 v2 core reboot bug
From: Zang Roy-r61911 @ 2007-06-13 6:43 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev list
In-Reply-To: <1181715851.9397.22.camel@localhost.localdomain>
On Wed, 2007-06-13 at 14:24, Zang Roy-r61911 wrote:
> From: roy zang <tie-fei.zang@freescale.com>
>
> Fix the reset bug on 8548CDS board.
> >From MPC8548CDS with e500 v2 core, a new reset control register is
> added.
I am also curious who adds the extra symbol ">" before From, the
mailer(I am using evolution) or else ...
:-)
Roy
^ permalink raw reply
* [PATCH v4]: Fix e500 v2 core reboot bug
From: Zang Roy-r61911 @ 2007-06-13 6:53 UTC (permalink / raw)
To: Segher Boessenkool, Paul Mackerras, Kumar Gala; +Cc: linuxppc-dev list
From: Roy Zang <tie-fei.zang@freescale.com>
Fix the reset bug on 8548CDS board.
>From MPC8548CDS with e500 v2 core, a new reset control register is added.
This register is used for the cpu reset.
Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
---
Please ignore all the previous ones :-(
Segher, do you have any comment?
If yes, I will revise again; if No, pick it up!
Thanks!
arch/powerpc/boot/dts/mpc8548cds.dts | 6 ++++++
arch/powerpc/platforms/85xx/misc.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/boot/dts/mpc8548cds.dts b/arch/powerpc/boot/dts/mpc8548cds.dts
index ad96381..0550a3c 100644
--- a/arch/powerpc/boot/dts/mpc8548cds.dts
+++ b/arch/powerpc/boot/dts/mpc8548cds.dts
@@ -177,6 +177,12 @@
interrupt-parent = <&mpic>;
};
+ global-utilities@e0000 { //global utilities reg
+ compatible = "fsl,mpc8548-guts";
+ reg = <e0000 1000>;
+ fsl,has-rstcr;
+ };
+
pci1: pci@8000 {
interrupt-map-mask = <1f800 0 0 7>;
interrupt-map = <
diff --git a/arch/powerpc/platforms/85xx/misc.c b/arch/powerpc/platforms/85xx/misc.c
index 3e62fcb..8286393 100644
--- a/arch/powerpc/platforms/85xx/misc.c
+++ b/arch/powerpc/platforms/85xx/misc.c
@@ -13,11 +13,43 @@
#include <linux/irq.h>
#include <linux/module.h>
#include <asm/irq.h>
+#include <asm/io.h>
+#include <asm/prom.h>
+#include <sysdev/fsl_soc.h>
+
+static __be32 __iomem *rstcr;
extern void abort(void);
+static int __init mpc85xx_rstcr(void)
+{
+ struct device_node *np;
+ np = of_find_node_by_name(NULL, "global-utilities");
+ if ((np && of_get_property(np, "fsl,has-rstcr", NULL))) {
+ const u32 *prop = of_get_property(np, "reg", NULL);
+ if (prop) {
+ /* map reset control register
+ * 0xE00B0 is offset of reset control register
+ */
+ rstcr = ioremap(get_immrbase() + *prop + 0xB0, 0xff);
+ if (!rstcr)
+ printk (KERN_EMERG "Error: reset control \
+ register not mapped, spinning!\n");
+ }
+ } else
+ printk (KERN_INFO "rstcr compatible register does not exist!\n");
+ if (np)
+ of_node_put(np);
+ return 0;
+}
+
+arch_initcall(mpc85xx_rstcr);
+
void mpc85xx_restart(char *cmd)
{
local_irq_disable();
+ if (rstcr)
+ /* set reset control register */
+ out_be32(rstcr, 0x2); /* HRESET_REQ */
abort();
}
--
1.5.1
^ permalink raw reply related
* Re: [PATCH v3]: Fix e500 v2 core reboot bug
From: Segher Boessenkool @ 2007-06-13 6:57 UTC (permalink / raw)
To: Zang Roy-r61911; +Cc: linuxppc-dev list
In-Reply-To: <1181717035.9397.30.camel@localhost.localdomain>
>> Fix the reset bug on 8548CDS board.
>>> From MPC8548CDS with e500 v2 core, a new reset control register is
>> added.
> I am also curious who adds the extra symbol ">" before From, the
> mailer(I am using evolution) or else ...
The mailer yes, since "From " at the start of line
would be confused for an SMTP transport header.
Mail clients traditionally use ">" to prevent this; some
mail clients do something different though (a space, for
example).
Segher
^ permalink raw reply
* Re: [PATCH v4]: Fix e500 v2 core reboot bug
From: Segher Boessenkool @ 2007-06-13 7:03 UTC (permalink / raw)
To: Zang Roy-r61911; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <1181717615.9397.40.camel@localhost.localdomain>
> Fix the reset bug on 8548CDS board.
> Segher, do you have any comment?
This one looks fine, only some cosmetics left :-)
> If yes, I will revise again; if No, pick it up!
Well it's not me who picks things up around here...
> + printk (KERN_EMERG "Error: reset control \
> + register not mapped, spinning!\n");
...(... "bla bla bla "
"bla bla bla\n");
instead? Or there'll be a lot of whitespace in your string.
I don't see anything spinning like the warning says, btw (or
it must be something in abort()?)
Segher
^ permalink raw reply
* Re: [PATCH v3]: Fix e500 v2 core reboot bug
From: Segher Boessenkool @ 2007-06-13 7:04 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev list
In-Reply-To: <4d678092dfbbed51d8b89bc92259b549@kernel.crashing.org>
> The mailer yes, since "From " at the start of line
> would be confused for an SMTP transport header.
Erm, an mbox header I meant of course.
Segher
^ permalink raw reply
* Re: [PATCH v4]: Fix e500 v2 core reboot bug
From: Zang Roy-r61911 @ 2007-06-13 7:14 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <6dc1593b8ff8442f7b4cf7a01f69c07c@kernel.crashing.org>
On Wed, 2007-06-13 at 15:03, Segher Boessenkool wrote:
> > Fix the reset bug on 8548CDS board.
>
> > Segher, do you have any comment?
>
> This one looks fine, only some cosmetics left :-)
>
> > If yes, I will revise again; if No, pick it up!
>
> Well it's not me who picks things up around here...
I know, it's Paul or Kumar! I just like to hear your valuable comment...
>
> > + printk (KERN_EMERG "Error: reset
> control \
> > + register not mapped,
> spinning!\n");
>
> ...(... "bla bla bla "
> "bla bla bla\n");
>
> instead? Or there'll be a lot of whitespace in your string.
I just do not know the difference of these two style.
> I don't see anything spinning like the warning says, btw (or
> it must be something in abort()?)
You can find the similar thing in 83xx/mis.c
[snip]
} else {
printk (KERN_EMERG "Error: Restart registers not mapped,
spinning!\n");
}
[snip]
Roy
^ permalink raw reply
* Re: [PATCH v4]: Fix e500 v2 core reboot bug
From: Segher Boessenkool @ 2007-06-13 7:26 UTC (permalink / raw)
To: Zang Roy-r61911; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <1181718898.12936.6.camel@localhost.localdomain>
>>> + printk (KERN_EMERG "Error: reset
>> control \
>>> + register not mapped,
>> spinning!\n");
>>
>> ...(... "bla bla bla "
>> "bla bla bla\n");
>>
>> instead? Or there'll be a lot of whitespace in your string.
> I just do not know the difference of these two style.
Your code puts a bunch of extra whitespace in the string (all
the tabs before "register not mapped"). Two strings after
each other (like in my example) are just pasted together.
There is no real reason to break the string in two anyway, it
is fine to have a more-than-80-char line in printk messages.
>> I don't see anything spinning like the warning says, btw (or
>> it must be something in abort()?)
> You can find the similar thing in 83xx/mis.c
> [snip]
> } else {
> printk (KERN_EMERG "Error: Restart registers not mapped,
> spinning!\n");
> }
> [snip]
You shouldn't have snipped, right after that it says
for (;;) ;
Segher
^ permalink raw reply
* Re: [PATCH v4]: Fix e500 v2 core reboot bug
From: Zang Roy-r61911 @ 2007-06-13 7:43 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <cad6a9b0bd58df6298cb5f8402e512e7@kernel.crashing.org>
On Wed, 2007-06-13 at 15:26, Segher Boessenkool wrote:
> >>> + printk (KERN_EMERG "Error: reset
> >> control \
> >>> + register not mapped,
> >> spinning!\n");
> >>
> >> ...(... "bla bla bla "
> >> "bla bla bla\n");
> >>
> >> instead? Or there'll be a lot of whitespace in your string.
> > I just do not know the difference of these two style.
>
> Your code puts a bunch of extra whitespace in the string (all
> the tabs before "register not mapped"). Two strings after
> each other (like in my example) are just pasted together.
>
> There is no real reason to break the string in two anyway, it
> is fine to have a more-than-80-char line in printk messages.
>
I separate it, because I am afraid I will be challenged by 80 char
column rule.
Please check the CodeStyle file and its example:
Statements longer than 80 columns will be broken into sensible chunks.
Descendants are always substantially shorter than the parent and are
placed
substantially to the right. The same applies to function headers with a
long
argument list. Long strings are as well broken into shorter strings.
[snip]
void fun(int a, int b, int c)
{
if (condition)
printk(KERN_WARNING "Warning this is a long printk with "
"3 parameters a: %u b: %u "
"c: %u \n", a, b, c);
else
next_statement;
}
[snip]
If it does not matter, I'd like to put them in one line :-).
Roy
^ permalink raw reply
* RE: [PATCH v4]: Fix e500 v2 core reboot bug
From: Li Yang-r58472 @ 2007-06-13 7:49 UTC (permalink / raw)
To: Zang Roy-r61911, Segher Boessenkool; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <1181720633.12936.19.camel@localhost.localdomain>
> -----Original Message-----
> From: linuxppc-dev-bounces+leoli=3Dfreescale.com@ozlabs.org
[mailto:linuxppc-dev-bounces+leoli=3Dfreescale.com@ozlabs.org] On Behalf
Of
> Zang Roy-r61911
> Sent: Wednesday, June 13, 2007 3:44 PM
> To: Segher Boessenkool
> Cc: linuxppc-dev list; Paul Mackerras
> Subject: Re: [PATCH v4]: Fix e500 v2 core reboot bug
>=20
> On Wed, 2007-06-13 at 15:26, Segher Boessenkool wrote:
> > >>> + printk (KERN_EMERG "Error: reset
> > >> control \
> > >>> + register not mapped,
> > >> spinning!\n");
> > >>
> > >> ...(... "bla bla bla "
> > >> "bla bla bla\n");
> > >>
> > >> instead? Or there'll be a lot of whitespace in your string.
> > > I just do not know the difference of these two style.
> >
> > Your code puts a bunch of extra whitespace in the string (all
> > the tabs before "register not mapped"). Two strings after
> > each other (like in my example) are just pasted together.
> >
> > There is no real reason to break the string in two anyway, it
> > is fine to have a more-than-80-char line in printk messages.
> >
> I separate it, because I am afraid I will be challenged by 80 char
> column rule.
>=20
> Please check the CodeStyle file and its example:
>=20
> Statements longer than 80 columns will be broken into sensible chunks.
> Descendants are always substantially shorter than the parent and are
> placed
> substantially to the right. The same applies to function headers with
a
> long
> argument list. Long strings are as well broken into shorter strings.
> [snip]
> void fun(int a, int b, int c)
> {
> if (condition)
> printk(KERN_WARNING "Warning this is a long printk with
"
> "3 parameters a: %u b:
%u "
> "c: %u \n", a, b, c);
> else
> next_statement;
> }
It will be good if you do:
+ printk (KERN_EMERG "Error: reset
control"
+ "register not mapped,
spinning!\n");
Instead of:
+ printk (KERN_EMERG "Error: reset control
\
+ register not mapped,
spinning!\n");
- Leo
^ permalink raw reply
* Re: [PATCH v4]: Fix e500 v2 core reboot bug
From: Segher Boessenkool @ 2007-06-13 7:54 UTC (permalink / raw)
To: Zang Roy-r61911; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <1181720633.12936.19.camel@localhost.localdomain>
>> There is no real reason to break the string in two anyway, it
>> is fine to have a more-than-80-char line in printk messages.
>>
> I separate it, because I am afraid I will be challenged by 80 char
> column rule.
Yeah, I understand.
> Please check the CodeStyle file and its example:
[snip]
> If it does not matter, I'd like to put them in one line :-).
Current thinking on this seems to be that long literal strings
are fine. You are right that some people might challenge you
on this, of course ;-)
Segher
^ permalink raw reply
* Re: [PATCH v4]: Fix e500 v2 core reboot bug
From: Segher Boessenkool @ 2007-06-13 7:55 UTC (permalink / raw)
To: Li Yang-r58472; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <989B956029373F45A0B8AF0297081890D2AC84@zch01exm26.fsl.freescale.net>
> It will be good if you do:
> + printk (KERN_EMERG "Error: reset
> control"
> + "register not mapped,
> spinning!\n");
>
> Instead of:
> + printk (KERN_EMERG "Error: reset control
> \
> + register not mapped,
> spinning!\n");
Except you forgot a space. Too much whitespace in the
resulting string is bad, but too little doesn't help
either ;-)
Segher
^ permalink raw reply
* Re: [PATCH v4]: Fix e500 v2 core reboot bug
From: Zang Roy-r61911 @ 2007-06-13 8:07 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev list, Paul Mackerras
In-Reply-To: <4470838fec2256a98e512486f8486605@kernel.crashing.org>
On Wed, 2007-06-13 at 15:55, Segher Boessenkool wrote:
> > It will be good if you do:
> > + printk (KERN_EMERG "Error: reset
> > control"
> > + "register not mapped,
> > spinning!\n");
> >
> > Instead of:
> > + printk (KERN_EMERG "Error: reset
> control
> > \
> > + register not mapped,
> > spinning!\n");
>
> Except you forgot a space. Too much whitespace in the
> resulting string is bad, but too little doesn't help
> either ;-)
I noticed that space, except that I am not sure the difference between \
and ".
I decided to leave it as two line with " not \.
Thanks
Roy
^ 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