* Re: [patch 08/18] PS3: Kexec support
From: Geoff Levand @ 2007-06-07 1:33 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <20070607112541.5a296cd1.sfr@canb.auug.org.au>
Stephen Rothwell wrote:
> Hi Geoff,
>
> On Wed, 06 Jun 2007 14:55:00 -0700 Geoff Levand <geoffrey.levand@am.sony.com> wrote:
>>
>> Michael Ellerman wrote:
>> > On Tue, 2007-06-05 at 20:00 -0700, Geoff Levand wrote:
>> >> Fixup the core platform parts needed for kexec to work on the PS3.
>> >> - Setup ps3_hpte_clear correctly.
>> >> - Mask interrupts on irq removal.
>> >> - Release all hypervisor resources.
>> >
>> > The irq changes might be kexec related, but it's a mess to review. You
>> > seem to moving a bunch of code around in the patch as well.
>>
>> Yes, I need to move the static chip_mask routines up so they would be
>> defined before the irq setup/destroy routines.
>
> I think Michael's point then is that if you submitted a separate
> preceding patch that just moves stuff around without any other changes
> (if at all possible) then reviewing the changes in this would be much
> easier (and our confidence would be higher).
Yes, sorry, I understood. I've already split it up for the next round.
-Geoff
^ permalink raw reply
* Re: [patch 08/18] PS3: Kexec support
From: Stephen Rothwell @ 2007-06-07 1:25 UTC (permalink / raw)
To: Geoff Levand; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <46672D34.8010808@am.sony.com>
[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]
Hi Geoff,
On Wed, 06 Jun 2007 14:55:00 -0700 Geoff Levand <geoffrey.levand@am.sony.com> wrote:
>
> Michael Ellerman wrote:
> > On Tue, 2007-06-05 at 20:00 -0700, Geoff Levand wrote:
> >> Fixup the core platform parts needed for kexec to work on the PS3.
> >> - Setup ps3_hpte_clear correctly.
> >> - Mask interrupts on irq removal.
> >> - Release all hypervisor resources.
> >
> > The irq changes might be kexec related, but it's a mess to review. You
> > seem to moving a bunch of code around in the patch as well.
>
> Yes, I need to move the static chip_mask routines up so they would be
> defined before the irq setup/destroy routines.
I think Michael's point then is that if you submitted a separate
preceding patch that just moves stuff around without any other changes
(if at all possible) then reviewing the changes in this would be much
easier (and our confidence would be higher).
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [PATCH] kexec ppc64: fix misaligned cmdline
From: Michael Neuling @ 2007-06-07 1:19 UTC (permalink / raw)
To: Milton Miller, linuxppc-dev, horms, kexec, Santhosh Rao
In-Reply-To: <26608.1180950136@neuling.org>
If the cmdline changes between boots, we can get misalignment of the
bootargs entry, which in turn corrupts our device tree blob and hence
kills our kexec boot. Also, if there was no /chosen/bootargs
previously, we'd never add a new one.
This ignores the bootargs while parsing the tree and inserts it later.
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
This hopefully address issues raised by Milton.
kexec/arch/ppc64/fs2dt.c | 95 +++++++++++++++++++++++++++++++----------------
1 file changed, 63 insertions(+), 32 deletions(-)
Index: kexec-tools-testing/kexec/arch/ppc64/fs2dt.c
===================================================================
--- kexec-tools-testing.orig/kexec/arch/ppc64/fs2dt.c
+++ kexec-tools-testing/kexec/arch/ppc64/fs2dt.c
@@ -18,6 +18,7 @@
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
+#define _GNU_SOURCE
#include <sys/types.h>
#include <sys/stat.h>
@@ -233,6 +234,12 @@ static void putprops(char *fn, struct di
!reuse_initrd)
continue;
+ /* This property will be created later in putnode() So
+ * ignore it now.
+ */
+ if (!strcmp(dp->d_name, "bootargs"))
+ continue;
+
if (! S_ISREG(statbuf.st_mode))
continue;
@@ -257,38 +264,6 @@ static void putprops(char *fn, struct di
checkprop(fn, dt, len);
- /* Get the cmdline from the device-tree and modify it */
- if (!strcmp(dp->d_name, "bootargs")) {
- int cmd_len;
- char temp_cmdline[COMMAND_LINE_SIZE] = { "" };
- char *param = NULL;
- cmd_len = strlen(local_cmdline);
- if (cmd_len != 0) {
- param = strstr(local_cmdline, "crashkernel=");
- if (param)
- crash_param = 1;
- param = strstr(local_cmdline, "root=");
- }
- if (!param) {
- char *old_param;
- memcpy(temp_cmdline, dt, len);
- param = strstr(temp_cmdline, "root=");
- if (param) {
- old_param = strtok(param, " ");
- if (cmd_len != 0)
- strcat(local_cmdline, " ");
- strcat(local_cmdline, old_param);
- }
- }
- strcat(local_cmdline, " ");
- cmd_len = strlen(local_cmdline);
- cmd_len = cmd_len + 1;
- memcpy(dt, local_cmdline,cmd_len);
- len = cmd_len;
- *dt_len = cmd_len;
- fprintf(stderr, "Modified cmdline:%s\n", local_cmdline);
- }
-
dt += (len + 3)/4;
if (!strcmp(dp->d_name, "reg") && usablemem_rgns.size)
add_usable_mem_property(fd, len);
@@ -385,6 +360,62 @@ static void putnode(void)
reserve(initrd_base, initrd_size);
}
+ /* Add cmdline to the second kernel. Check to see if the new
+ * cmdline has a root=. If not, use the old root= cmdline. */
+ if (!strcmp(basename,"/chosen/")) {
+ size_t cmd_len = 0;
+ char *param = NULL;
+
+ cmd_len = strlen(local_cmdline);
+ if (cmd_len != 0) {
+ param = strstr(local_cmdline, "crashkernel=");
+ if (param)
+ crash_param = 1;
+ /* does the new cmdline have a root= ? ... */
+ param = strstr(local_cmdline, "root=");
+ }
+
+ /* ... if not, grab root= from the old command line */
+ if (!param) {
+ char filename[MAXPATH];
+ FILE *fp;
+ char *last_cmdline = NULL;
+ char *old_param;
+
+ strcpy(filename, pathname);
+ strcat(filename, "bootargs");
+ fp = fopen(filename, "r");
+ if (fp) {
+ if (getline(&last_cmdline, &cmd_len, fp) == -1)
+ die("unable to read %s\n", filename);
+
+ param = strstr(last_cmdline, "root=");
+ if (param) {
+ old_param = strtok(param, " ");
+ if (cmd_len != 0)
+ strcat(local_cmdline, " ");
+ strcat(local_cmdline, old_param);
+ }
+ }
+ if (last_cmdline)
+ free(last_cmdline);
+ }
+ strcat(local_cmdline, " ");
+ cmd_len = strlen(local_cmdline);
+ cmd_len = cmd_len + 1;
+
+ /* add new bootargs */
+ *dt++ = 3;
+ *dt++ = cmd_len;
+ *dt++ = propnum("bootargs");
+ if ((cmd_len >= 8) && ((unsigned long)dt & 0x4))
+ dt++;
+ memcpy(dt, local_cmdline,cmd_len);
+ dt += (cmd_len + 3)/4;
+
+ fprintf(stderr, "Modified cmdline:%s\n", local_cmdline);
+ }
+
for (i=0; i < numlist; i++) {
dp = namelist[i];
strcpy(dn, dp->d_name);
^ permalink raw reply
* [RFC] 85XX: Allow 8259 cascade to share an MPIC interrupt line.
From: Randy Vinson @ 2007-06-07 0:47 UTC (permalink / raw)
To: linuxppc-dev@ozlabs.org
The Freescale MPC8555CDS and MPC8548CDS reference hardware has a legacy
8259 interrupt controller pair contained within a VIA VT82C686B Southbridge
on the main carrier board. The processor complex plugs into the carrier
card using a PCI slot which limits the available interrupts to the
INTA-INTD PCI interrupts. The output of the 8259 cascade pair is routed
through a gate array and connected to the PCI INTA interrupt line.
The normal interrupt chaining hook (set_irq_chained_handler) does
not allow sharing of the chained interrupt which prevents the
use of PCI INTA by PCI devices. This patch allows the 8259 cascade
pair to share their interrupt line with PCI devices.
Signed-off-by: Randy Vinson <rvinson@mvista.com>
---
Note that there may very well be a better way of accomplishing this. If someone
has a better alternative, I'm open to it. This was just the simplest way I could
get this to work.
Also, the addition of the .end routine for the MPIC is not strictly necessary for
this patch. It's there so this code will run from within the threaded interrupt
context used by the Real Time patch.
arch/powerpc/platforms/85xx/mpc85xx_cds.c | 32 +++++++++++++++++++++++++---
arch/powerpc/sysdev/mpic.c | 1 +
2 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_cds.c b/arch/powerpc/platforms/85xx/mpc85xx_cds.c
index 1490eb3..431aaa2 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_cds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_cds.c
@@ -127,16 +127,30 @@ static void __init mpc85xx_cds_pcibios_fixup(void)
}
#ifdef CONFIG_PPC_I8259
-#warning The i8259 PIC support is currently broken
-static void mpc85xx_8259_cascade(unsigned int irq, struct irq_desc *desc)
+static void mpc85xx_8259_cascade_handler(unsigned int irq,
+ struct irq_desc *desc)
{
unsigned int cascade_irq = i8259_irq();
if (cascade_irq != NO_IRQ)
+ /* handle an interrupt from the 8259 */
generic_handle_irq(cascade_irq);
- desc->chip->eoi(irq);
+ /* check for any interrupts from the shared IRQ line */
+ handle_fasteoi_irq(irq, desc);
}
+
+static irqreturn_t mpc85xx_8259_cascade_action(int irq, void *dev_id)
+{
+ return IRQ_HANDLED;
+}
+
+static struct irqaction mpc85xxcds_8259_irqaction = {
+ .handler = mpc85xx_8259_cascade_action,
+ .flags = IRQF_SHARED,
+ .mask = CPU_MASK_NONE,
+ .name = "8259 cascade",
+};
#endif /* PPC_I8259 */
#endif /* CONFIG_PCI */
@@ -216,7 +230,17 @@ static void __init mpc85xx_cds_pic_init(void)
i8259_init(cascade_node, 0);
of_node_put(cascade_node);
- set_irq_chained_handler(cascade_irq, mpc85xx_8259_cascade);
+ /*
+ * Hook the interrupt to make sure desc->action is never NULL.
+ * This is required to ensure that the interrupt does not get
+ * disabled when the last user of the shared IRQ line frees their
+ * interrupt.
+ */
+ if (setup_irq(cascade_irq, &mpc85xxcds_8259_irqaction))
+ printk(KERN_ERR "Failed to setup cascade interrupt\n");
+ else
+ /* Success. Connect our low-level cascade handler. */
+ set_irq_handler(cascade_irq, mpc85xx_8259_cascade_handler);
#endif /* CONFIG_PPC_I8259 */
}
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index 75aad38..14e3d1d 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -836,6 +836,7 @@ static struct irq_chip mpic_irq_chip = {
.mask = mpic_mask_irq,
.unmask = mpic_unmask_irq,
.eoi = mpic_end_irq,
+ .end = mpic_unmask_irq,
.set_type = mpic_set_irq_type,
};
--
1.5.0.GIT
^ permalink raw reply related
* Re: [PATCH 2/2] powerpc: Remove 'console=' from cmdline on prpmc2800
From: Mark A. Greer @ 2007-06-07 0:42 UTC (permalink / raw)
To: Mark A. Greer; +Cc: linuxppc-dev
In-Reply-To: <20070607003800.GA4301@mag.az.mvista.com>
Specifying 'console=ttyMM0' on the cmdline for the prmpc2800 is no
longer necessary.
Signed-off-by: Mark A. Greer <mgreer@mvista.com>
---
arch/powerpc/boot/dts/prpmc2800.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/boot/dts/prpmc2800.dts b/arch/powerpc/boot/dts/prpmc2800.dts
index 568965a..699d0df 100644
--- a/arch/powerpc/boot/dts/prpmc2800.dts
+++ b/arch/powerpc/boot/dts/prpmc2800.dts
@@ -309,7 +309,7 @@
};
chosen {
- bootargs = "ip=on console=ttyMM0";
+ bootargs = "ip=on";
linux,stdout-path = "/mv64x60@f1000000/mpsc@8000";
};
};
^ permalink raw reply related
* [PATCH 1/2] powerpc: call add_preferred_console when mpsc is console
From: Mark A. Greer @ 2007-06-07 0:38 UTC (permalink / raw)
To: linuxppc-dev
When a Marvell MPSC (serial controller) port is the specified
/chosen/stdout-path device, call 'add_preferred_console()' so the user
doesn't have to specify a 'console=ttyMMx' cmdline argument.
Signed-off-by: Mark A. Greer <mgreer@mvista.com>
Acked-by: Dale Farnsworth <dale@farnsworth.org>
---
arch/powerpc/sysdev/mv64x60_dev.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/arch/powerpc/sysdev/mv64x60_dev.c b/arch/powerpc/sysdev/mv64x60_dev.c
index 4b0a9c8..b618fa6 100644
--- a/arch/powerpc/sysdev/mv64x60_dev.c
+++ b/arch/powerpc/sysdev/mv64x60_dev.c
@@ -12,6 +12,7 @@
#include <linux/stddef.h>
#include <linux/kernel.h>
#include <linux/init.h>
+#include <linux/console.h>
#include <linux/mv643xx.h>
#include <linux/platform_device.h>
@@ -420,3 +421,30 @@ error:
return err;
}
arch_initcall(mv64x60_device_setup);
+
+static int __init mv64x60_add_mpsc_console(void)
+{
+ struct device_node *np = NULL;
+ const char *prop;
+
+ prop = of_get_property(of_chosen, "linux,stdout-path", NULL);
+ if (prop == NULL)
+ goto not_mpsc;
+
+ np = of_find_node_by_path(prop);
+ if (!np)
+ goto not_mpsc;
+
+ if (!of_device_is_compatible(np, "marvell,mpsc"))
+ goto not_mpsc;
+
+ prop = of_get_property(np, "block-index", NULL);
+ if (!prop)
+ goto not_mpsc;
+
+ add_preferred_console("ttyMM", *(int *)prop, NULL);
+
+not_mpsc:
+ return 0;
+}
+console_initcall(mv64x60_add_mpsc_console);
^ permalink raw reply related
* Re: [PATCH 2.6.22-rc4] ehea: Fixed possible kernel panic on VLAN packet recv
From: Michael Ellerman @ 2007-06-07 0:35 UTC (permalink / raw)
To: Thomas Klein
Cc: Thomas Klein, Jeff Garzik, Jan-Bernd Themann, netdev,
linux-kernel, Stefan Roscher, linux-ppc, Christoph Raisch,
Marcus Eder
In-Reply-To: <200706062053.16986.osstklei@de.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 2127 bytes --]
On Wed, 2007-06-06 at 20:53 +0200, Thomas Klein wrote:
> This patch fixes a possible kernel panic due to not checking the vlan group
> when processing received VLAN packets and a malfunction in VLAN/hypervisor
> registration.
>
>
> Signed-off-by: Thomas Klein <tklein@de.ibm.com>
> ---
>
>
> diff -Nurp -X dontdiff linux-2.6.22-rc4/drivers/net/ehea/ehea.h patched_kernel/drivers/net/ehea/ehea.h
> --- linux-2.6.22-rc4/drivers/net/ehea/ehea.h 2007-06-05 02:57:25.000000000 +0200
> +++ patched_kernel/drivers/net/ehea/ehea.h 2007-06-06 12:53:58.000000000 +0200
> @@ -39,7 +39,7 @@
> #include <asm/io.h>
>
> #define DRV_NAME "ehea"
> -#define DRV_VERSION "EHEA_0061"
> +#define DRV_VERSION "EHEA_0064"
>
> #define EHEA_MSG_DEFAULT (NETIF_MSG_LINK | NETIF_MSG_TIMER \
> | NETIF_MSG_RX_ERR | NETIF_MSG_TX_ERR)
> diff -Nurp -X dontdiff linux-2.6.22-rc4/drivers/net/ehea/ehea_main.c patched_kernel/drivers/net/ehea/ehea_main.c
> --- linux-2.6.22-rc4/drivers/net/ehea/ehea_main.c 2007-06-05 02:57:25.000000000 +0200
> +++ patched_kernel/drivers/net/ehea/ehea_main.c 2007-06-06 12:53:58.000000000 +0200
> @@ -1947,7 +1945,7 @@ static void ehea_vlan_rx_add_vid(struct
> }
>
> index = (vid / 64);
> - cb1->vlan_filter[index] |= ((u64)(1 << (vid & 0x3F)));
> + cb1->vlan_filter[index] |= ((u64)(0x8000000000000000 >> (vid & 0x3F)));
>
> hret = ehea_h_modify_ehea_port(adapter->handle, port->logical_port_id,
> H_PORT_CB1, H_PORT_CB1_ALL, cb1);
> @@ -1982,7 +1980,7 @@ static void ehea_vlan_rx_kill_vid(struct
> }
>
> index = (vid / 64);
> - cb1->vlan_filter[index] &= ~((u64)(1 << (vid & 0x3F)));
> + cb1->vlan_filter[index] &= ~((u64)(0x8000000000000000 >> (vid & 0x3F)));
These two seem ripe for splitting into some sort of helper routine.
Which would leave only one place to get it right.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: MPC8349ea Random Device Generator driver
From: Olof Johansson @ 2007-06-07 0:00 UTC (permalink / raw)
To: Timur Tabi; +Cc: linuxppc-dev, sl, Philippe Lachenal
In-Reply-To: <466739A3.1070201@freescale.com>
On Wed, Jun 06, 2007 at 05:48:03PM -0500, Timur Tabi wrote:
> Olof Johansson wrote:
>
> > typedef __u16 __bitwise __le16;
> > typedef __u16 __bitwise __be16;
> > typedef __u32 __bitwise __le32;
> > typedef __u32 __bitwise __be32;
> > typedef __u64 __bitwise __le64;
> > typedef __u64 __bitwise __be64;
>
> Wait a minute - why are all the endian-specific types __bitwise?
>
> What if I have a 32-bit MMIO register that just contains a number, but it has to be
> written in big-endian? I can't use __be32 to designate it.
>
> For instance, I was going to do this for a new memory-mapped device I'm working with:
>
> struct ccsr_dma {
> u8 res0[0x100];
> struct {
> __be32 mr; /* x00 Mode register */
> __be32 sr; /* x04 Status register */
> __be32 eclndar; /* x08 Current link descriptor extended address register */
> __be32 clndar; /* x0C Current link descriptor address register */
> __be32 satr; /* x10 Source attributes register */
> __be32 sar; /* x14 Source address register */
> ...
> u8 res2[0x38];
> } channel[4];
> }
>
> The SAR register is defined as:
>
> Bits Name Description
> 0?31 SAD Source address. This register contains the low-order bits of the 36-bit source
> address of the DMA transfer. The contents are updated after every DMA write operation
> unless the final stride of a striding operation is less than the stride size, in which
> case it remains equal to the address from which the last stride began.
>
> In other words, the SAR register is just one number, but it must be written as a single
> big-endian number. So is __be32 the wrong type?
It doesn't matter a whole lot what the type is, but you should only ever
read and write registers with the MMIO ops, never directly as variable
references. You should never ever dereference an __iomem pointer (which
is what you get back from ioremap and friends)[1].
If you use in_be32/out_be32() on it, you'll be fine. It will use
appropriate barriers, and it will read and write the whole word at once.
Look at it as if anything ioremap():ed is in a different address space,
and there's only a few valid ways to access that address space. Direct
derefs in code is not one of them.
If in doubt, run sparse on your code (make C=1).
-Olof
[1] there are a few, and very very few exceptions.
^ permalink raw reply
* Re: MPC8349ea Random Device Generator driver
From: Olof Johansson @ 2007-06-06 23:54 UTC (permalink / raw)
To: Timur Tabi; +Cc: linuxppc-dev, sl, Philippe Lachenal
In-Reply-To: <466735F3.7040504@freescale.com>
On Wed, Jun 06, 2007 at 05:32:19PM -0500, Timur Tabi wrote:
> Olof Johansson wrote:
>
> > - you can never assign a __le16 type to any other integer type or any
> > other bitwise type. You'd get a warnign about incompatible types. Makes
> > sense, no?
>
> Then why do the in_be functions return an unsigned int instead of a __be type? Isn't that
> effectively removing the endian-ness from the type?
Because they read a big endian register and returns the contents in the
native byte order for the machine.
-Olof
^ permalink raw reply
* Re: [Linux-fbdev-devel] [patch 17/18] PS3: Frame buffer system-bus rework
From: Antonino A. Daplas @ 2007-06-06 23:37 UTC (permalink / raw)
To: Geoff Levand
Cc: Geert.Uytterhoeven, Linux/PPC Development, linux-fbdev-devel,
Paul Mackerras
In-Reply-To: <46673E57.3020009@am.sony.com>
On Wed, 2007-06-06 at 16:08 -0700, Geoff Levand wrote:
> Hi.
>
> Antonino A. Daplas wrote:
> > On Wed, 2007-06-06 at 08:51 +0200, Geert Uytterhoeven wrote:
> >> This one should go through linux-fbdev-devel (CC) for review, but Paul can
> >> merge it because it depends on the PS3 system bus device rework.
> >>
> >
> >> >
> >> > -static void ps3fb_shutdown(struct platform_device *dev)
> >> > +static int __devexit ps3fb_remove(struct ps3_system_bus_device *dev)
> >> > {
> >> > - ps3fb_flip_ctl(0); /* flip off */
> >> > - ps3fb.dinfo->irq.mask = 0;
> >> > - free_irq(ps3fb.irq_no, ps3fb.dev);
> >> > - ps3_irq_plug_destroy(ps3fb.irq_no);
> >> > - iounmap((u8 __iomem *)ps3fb.dinfo);
> >> > + BUG();
> >
> > Why BUG()?
>
> remove() doesn't work because lack of support from fbcon.
We already have console registration/unregistration support for the past
few kernel versions.
> I'll remove
> this though, since it independent of the fbcon fix.
This is part of module_exit(), correct? Then yes, module-init-tools
will disallow rmmod to remove the module if fbcon is holding a reference
count to it. However, if fbcon is not loaded, or fbcon is not bound to
it (with an echo 0 > /sys/class/vtconsole/vtcon1/bind), then rmmod will
succeed, and so should ps3fb_remove().
Tony
^ permalink raw reply
* Re: [Linux-fbdev-devel] [patch 17/18] PS3: Frame buffer system-bus rework
From: Geoff Levand @ 2007-06-06 23:08 UTC (permalink / raw)
To: Antonino A. Daplas
Cc: Linux/PPC Development, linux-fbdev-devel, Paul Mackerras
In-Reply-To: <1181145418.5023.11.camel@daplas>
Hi.
Antonino A. Daplas wrote:
> On Wed, 2007-06-06 at 08:51 +0200, Geert Uytterhoeven wrote:
>> This one should go through linux-fbdev-devel (CC) for review, but Paul can
>> merge it because it depends on the PS3 system bus device rework.
>>
>
>> >
>> > -static void ps3fb_shutdown(struct platform_device *dev)
>> > +static int __devexit ps3fb_remove(struct ps3_system_bus_device *dev)
>> > {
>> > - ps3fb_flip_ctl(0); /* flip off */
>> > - ps3fb.dinfo->irq.mask = 0;
>> > - free_irq(ps3fb.irq_no, ps3fb.dev);
>> > - ps3_irq_plug_destroy(ps3fb.irq_no);
>> > - iounmap((u8 __iomem *)ps3fb.dinfo);
>> > + BUG();
>
> Why BUG()?
remove() doesn't work because lack of support from fbcon. I'll remove
this though, since it independent of the fbcon fix.
-Geoff
^ permalink raw reply
* Re: MPC8349ea Random Device Generator driver
From: Timur Tabi @ 2007-06-06 22:48 UTC (permalink / raw)
To: Olof Johansson; +Cc: linuxppc-dev, sl, Philippe Lachenal
In-Reply-To: <20070606222456.GA28225@lixom.net>
Olof Johansson wrote:
> typedef __u16 __bitwise __le16;
> typedef __u16 __bitwise __be16;
> typedef __u32 __bitwise __le32;
> typedef __u32 __bitwise __be32;
> typedef __u64 __bitwise __le64;
> typedef __u64 __bitwise __be64;
Wait a minute - why are all the endian-specific types __bitwise?
What if I have a 32-bit MMIO register that just contains a number, but it has to be
written in big-endian? I can't use __be32 to designate it.
For instance, I was going to do this for a new memory-mapped device I'm working with:
struct ccsr_dma {
u8 res0[0x100];
struct {
__be32 mr; /* x00 Mode register */
__be32 sr; /* x04 Status register */
__be32 eclndar; /* x08 Current link descriptor extended address register */
__be32 clndar; /* x0C Current link descriptor address register */
__be32 satr; /* x10 Source attributes register */
__be32 sar; /* x14 Source address register */
...
u8 res2[0x38];
} channel[4];
}
The SAR register is defined as:
Bits Name Description
0–31 SAD Source address. This register contains the low-order bits of the 36-bit source
address of the DMA transfer. The contents are updated after every DMA write operation
unless the final stride of a striding operation is less than the stride size, in which
case it remains equal to the address from which the last stride began.
In other words, the SAR register is just one number, but it must be written as a single
big-endian number. So is __be32 the wrong type?
--
Timur Tabi
Linux Kernel Developer @ Freescale
^ permalink raw reply
* Re: [PATCH] Create add_rtc() function to enable the RTC CMOS driver
From: Arnd Bergmann @ 2007-06-06 22:44 UTC (permalink / raw)
To: linuxppc-dev; +Cc: paulus
In-Reply-To: <1181147873.5674.116.camel@rhino>
T24gV2VkbmVzZGF5IDA2IEp1bmUgMjAwNywgV2FkZSBGYXJuc3dvcnRoIHdyb3RlOgo+ICsKPiAr
oKCgoKCgoG5wID0gb2ZfZmluZF9jb21wYXRpYmxlX25vZGUoTlVMTCwgTlVMTCwgInBucFBOUCxi
MDAiKTsKPiAroKCgoKCgoGlmICghbnApCj4gK6CgoKCgoKCgoKCgoKCgoHJldHVybiAtRU5PREVW
Owo+ICsKPiAroKCgoKCgoGlmIChvZl9hZGRyZXNzX3RvX3Jlc291cmNlKG5wLCAwLCAmcmVzKSkg
ewo+ICugoKCgoKCgoKCgoKCgoKBvZl9ub2RlX3B1dChucCk7Cj4gK6CgoKCgoKCgoKCgoKCgoHJl
dHVybiAtRU5PREVWOwo+ICugoKCgoKCgfQo+ICsKPiAroKCgoKCgoHBkID0gcGxhdGZvcm1fZGV2
aWNlX3JlZ2lzdGVyX3NpbXBsZSgicnRjX2Ntb3MiLCAtMSwKPiAroKCgoKCgoKCgoKCgoKCgoKCg
oKCgoKCgoKCgoKCgoKCgoKCgoKCgIKAgoCAmcmVzLCAxKTsKCkRvZXNuJ3QgdGhpcyBjb25mbGlj
dCB3aXRoIGEgcG90ZW50aWFsIGlzYXBucCBkcml2ZXIgZm9yIHRoZSBzYW1lCmRldmljZT8gSWYg
eW91IGhhdmUgbXVsdGlwbGUgaXNhcG5wIGRldmljZXMsIGl0IG1heSBiZSBiZXR0ZXIKdG8gYWRk
IGEgYnVzIGRyaXZlciBmb3IgdGhlIGlzYXBucCBob3N0LCBhbmQgdXNlIGRldmljZSBkcml2ZXJz
CmZvciB0aGF0IGJ1cyBpbnN0ZWFkIG9mIHBsYXRmb3JtIGRldmljZXMuCgoJQXJuZCA8PjwK
^ permalink raw reply
* Re: [Linux-fbdev-devel] [patch 17/18] PS3: Frame buffer system-bus rework
From: Antonino A. Daplas @ 2007-06-06 15:56 UTC (permalink / raw)
To: linux-fbdev-devel; +Cc: Linux/PPC Development, Paul Mackerras
In-Reply-To: <Pine.LNX.4.62.0706060849530.19499@pademelon.sonytel.be>
On Wed, 2007-06-06 at 08:51 +0200, Geert Uytterhoeven wrote:
> This one should go through linux-fbdev-devel (CC) for review, but Paul can
> merge it because it depends on the PS3 system bus device rework.
>
> >
> > -static void ps3fb_shutdown(struct platform_device *dev)
> > +static int __devexit ps3fb_remove(struct ps3_system_bus_device *dev)
> > {
> > - ps3fb_flip_ctl(0); /* flip off */
> > - ps3fb.dinfo->irq.mask = 0;
> > - free_irq(ps3fb.irq_no, ps3fb.dev);
> > - ps3_irq_plug_destroy(ps3fb.irq_no);
> > - iounmap((u8 __iomem *)ps3fb.dinfo);
> > + BUG();
Why BUG()?
Tony
^ permalink raw reply
* Re: MPC8349ea Random Device Generator driver
From: Timur Tabi @ 2007-06-06 22:38 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Olof Johansson, linuxppc-dev, sl, Philippe Lachenal
In-Reply-To: <200706070035.16150.arnd@arndb.de>
Arnd Bergmann wrote:
> There are there on my system, see in_be64/out_be64/readq/writeq. note that the code
> for those is created by macros, so they don't show up in ctags.
Ah, that explains it. In that case, I think the code should be changed to reference
everything as 64-bit registers and ints.
--
Timur Tabi
Linux Kernel Developer @ Freescale
^ permalink raw reply
* Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN
From: Arnd Bergmann @ 2007-06-06 22:39 UTC (permalink / raw)
To: linuxppc-dev; +Cc: paulus
In-Reply-To: <1181147415.5674.108.camel@rhino>
On Wednesday 06 June 2007, Wade Farnsworth wrote:
> +=A0=A0=A0=A0=A0=A0=A0/* LPC47M192 Super I/O configuration */
> +=A0=A0=A0=A0=A0=A0=A0outb(0x55, 0x4e);=A0=A0=A0=A0=A0=A0=A0/* enter supe=
rio config mode */
> +
> +=A0=A0=A0=A0=A0=A0=A0/* Enable keyboard and mouse */
> +=A0=A0=A0=A0=A0=A0=A0outb(0x07, 0x4e);=A0=A0=A0=A0=A0=A0=A0/* device sel=
ector register */
> +=A0=A0=A0=A0=A0=A0=A0outb(0x07, 0x4f);=A0=A0=A0=A0=A0=A0=A0/* select key=
board registers (device 7) */
> +=A0=A0=A0=A0=A0=A0=A0outb(0x30, 0x4e);=A0=A0=A0=A0=A0=A0=A0/* keyboard a=
ctivation register */
> +=A0=A0=A0=A0=A0=A0=A0outb(0x01, 0x4f);=A0=A0=A0=A0=A0=A0=A0/* activate k=
eyboard */
Hardcoded I/O port numbers always worry me a little. I know that this is
supposed to work in general, but can't you read the I/O port range from
a device tree property?
Arnd <><
^ permalink raw reply
* Re: [patch 03/18] PS3: Use __maybe_unused
From: Geoff Levand @ 2007-06-06 22:37 UTC (permalink / raw)
To: michael; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1181102745.5536.18.camel@concordia.ozlabs.ibm.com>
Michael Ellerman wrote:
> On Tue, 2007-06-05 at 19:59 -0700, Geoff Levand wrote:
>> Change the PS3 debug routines from using the GCC specific
>> '__attribute__ ((unused))' to the preprocessor macro
>> __maybe_unused.
>
> I'm confused. If DEBUG is defined then they're used, if it's not then
> they're not built.
Some are for actual 'debugging' use, calls to be added later, not
just for verbose output. Others are for verbose output when DEBUG
is defined. I have it setup so they are always built to do the
syntax check, but removed by the optimizer when not used. I was
planning to remove these routines sometime in the future, but want
to keep them in until the code gets stable.
-Geoff
^ permalink raw reply
* Re: MPC8349ea Random Device Generator driver
From: Arnd Bergmann @ 2007-06-06 22:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Olof Johansson, sl, Timur Tabi, Philippe Lachenal
In-Reply-To: <466732EE.8080306@freescale.com>
On Thursday 07 June 2007, Timur Tabi wrote:
> Anyway, there's a more fundamental problem. =A0I can't find any 64-bit ve=
rsions of the=20
> in_be/out_be/read/write functions. =A0Because the registers are big-endia=
n, then we have a=20
> few choices:
> 1) Create 64-bit versions of these functions. Can someone tell me why th=
ey don't exist=20
> already?
There are there on my system, see in_be64/out_be64/readq/writeq. note that =
the code
for those is created by macros, so they don't show up in ctags.
Arnd <><
^ permalink raw reply
* Re: MPC8349ea Random Device Generator driver
From: Timur Tabi @ 2007-06-06 22:32 UTC (permalink / raw)
To: Olof Johansson; +Cc: linuxppc-dev, sl, Philippe Lachenal
In-Reply-To: <20070606222456.GA28225@lixom.net>
Olof Johansson wrote:
> - you can never assign a __le16 type to any other integer type or any
> other bitwise type. You'd get a warnign about incompatible types. Makes
> sense, no?
Then why do the in_be functions return an unsigned int instead of a __be type? Isn't that
effectively removing the endian-ness from the type?
--
Timur Tabi
Linux Kernel Developer @ Freescale
^ permalink raw reply
* Re: 83xx GPIO/EXT int in arch/powerpc/
From: Benjamin Herrenschmidt @ 2007-06-06 22:31 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev
In-Reply-To: <3641c2a601dbccc9757fe9443d9330ca@kernel.crashing.org>
On Wed, 2007-06-06 at 11:47 +0200, Segher Boessenkool wrote:
> - You can do it in your interrupt-map. That's probably the cleanest
> > way
> > to do it.
>
> Not really. If the device interrupt is physically connected
> directly to a GPIO or a PIC or whatever, if it doesn't go
> through the PCI bus; then it shouldn't be represented in
> the device tree as if it did. The higher-up bridge has
> nothing whatsoever to do with this device's interrupt.
>
> It's a simple hack of course, it can be useful sometimes.
> It's not "clean" at all however.
I disagree :-)
The "PCI bus" in that sense is just the INT lines. They are connected
wherever you want, doesn't matter. Look at Apple for example, they are
all connected to GPIOs too.
The interrupt-map simply provides for each slot where the interrupt
goes. It's perfectly fine and clean to use it when those interrupts are
hooked to GPIOs and has been a common practice. I really don't see a
problem with that.
> Depending on the rest of the tree, this would often mean
> you have to do a much bigger interrupt-map-mask than
> without this hack, and the corresponding explosion in
> interrupt-map size.
Not really again. Besides, devices normally use only int#A, only bridges
use more.
> Almost always that's how it works yeah. And the "GPIO"
> stage isn't normally represented in the device tree, GPIO
> settings are considered a part of the PCB layout ;-) (one
> more reason why Linux shouldn't have to touch it).
>
> > If that's not the case, you may need some intermediary cascaded
> > interrupt controller or something around those lines.
>
> Yeah, but that's just the more general case of the
> general case :-)
Segher, I feel here that your inability to resist arguing about
everything in that case is confusing what is a fairly simple issue in
the first place :-) Poor guy trying to figure out the right way to do
his stuff will run away screaming and go back hard coding his number in
his platform code before we are finished :-)
So I would say, taking a dicator hat, that the right way to fix his
problem is also the simplest, which is to make sure the interrupt-map
entry for that slot points to the right GPIO interrupt :-)
Easy isn't it ?
Cheers,
Ben.
^ permalink raw reply
* Re: MPC8349ea Random Device Generator driver
From: Timur Tabi @ 2007-06-06 22:19 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Olof Johansson, linuxppc-dev, sl, Philippe Lachenal
In-Reply-To: <200706070011.43347.arnd@arndb.de>
Arnd Bergmann wrote:
> The only code that is allowed to dereference an __iomem pointer is the
> implementation of readl/writel and similar functions.
> The code you gave as an example gives you a warning when building with
> make C=1 using sparse, and missed barriers.
Well, that's why I said "or something like that".
Anyway, there's a more fundamental problem. I can't find any 64-bit versions of the
in_be/out_be/read/write functions. Because the registers are big-endian, then we have a
few choices:
1) Create 64-bit versions of these functions. Can someone tell me why they don't exist
already?
2) Do something stupid like u32 v = in_be32(((void *) &rng->rngisr) + 4);
3) Change the struct like this:
struct sec_rng {
__be32 rngmr_hi;
__be32 rngmr_lo;
u8 res1[8];
__be32 rngdsr_hi;
__be32 rngdsr_lo;
__be32 rngrcr_hi;
__be32 rngrcr_lo;
...
Comments?
--
Timur Tabi
Linux Kernel Developer @ Freescale
^ permalink raw reply
* Re: MPC8349ea Random Device Generator driver
From: Olof Johansson @ 2007-06-06 22:24 UTC (permalink / raw)
To: Timur Tabi; +Cc: linuxppc-dev, sl, Philippe Lachenal
In-Reply-To: <46673009.6000109@freescale.com>
On Wed, Jun 06, 2007 at 05:07:05PM -0500, Timur Tabi wrote:
> Olof Johansson wrote:
>
> >There's nothing wrong with the way he coded that up. Lots of drivers
> >are written that way (all of mine are). It's at least as clear as any
> >structure, and it doesn't cause temptation to do...
>
> That vast majority of Freescale SOC device register maps are handled via a
> structure. He's doing everything via 32-bit operations, even though the
> registers are 64 bits, and therefore he has twice as much macros as he
> needs.
I see only one double access back to back, not a big deal. I do however
fail to see how a couple of constructs are working, separate email with
those questions later though.
> >>>+
> >>>+
> >>>+ /* check for things like FIFO underflow */
> >>>+
> >>>+ u32 v;
> >>>+
> >>>+ v = in_be32(rng_regs + TALITOS_RNGISR_HI);
> >> u64 v;
> >> v = rng->rngisr;
> >>
> >>or something like that. Try to use the built-in support for 64-bit data
> >>types when possible.
> >
> >...this. NO! Don't reference ioremapped memory from regular code like
> >that. The way he's doing it is the preferred way.
>
> Can you explain that better? What is "regular code"?
>From http://kerneltrap.org/node/3848:
---
In the byte-order case, what we have is:
typedef __u16 __bitwise __le16;
typedef __u16 __bitwise __be16;
typedef __u32 __bitwise __le32;
typedef __u32 __bitwise __be32;
typedef __u64 __bitwise __le64;
typedef __u64 __bitwise __be64;
and if you think about the above rules about what is acceptable for
bitwise types, you'll likely immediately notivce that it automatically
means
- you can never assign a __le16 type to any other integer type or any
other bitwise type. You'd get a warnign about incompatible types. Makes
sense, no?
...
---
Note that rngisr is __be64. Sparse should complain loudly over your code.
-Olof
^ permalink raw reply
* Re: MPC8349ea Random Device Generator driver
From: Arnd Bergmann @ 2007-06-06 22:11 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Olof Johansson, sl, Timur Tabi, Philippe Lachenal
In-Reply-To: <46673009.6000109@freescale.com>
On Thursday 07 June 2007, Timur Tabi wrote:
> >> =A0=A0=A0=A0=A0u64 v;
> >> =A0=A0=A0=A0=A0v =3D rng->rngisr;
> >>
> >> or something like that. =A0Try to use the built-in support for 64-bit =
data types when possible.
> >=20
> > ...this. NO! Don't reference ioremapped memory from regular code like
> > that. The way he's doing it is the preferred way.
>=20
> Can you explain that better? =A0What is "regular code"?
The only code that is allowed to dereference an __iomem pointer is the
implementation of readl/writel and similar functions.
The code you gave as an example gives you a warning when building with
make C=3D1 using sparse, and missed barriers.
Arnd <><
^ permalink raw reply
* Re: MPC8349ea Random Device Generator driver
From: Timur Tabi @ 2007-06-06 22:07 UTC (permalink / raw)
To: Olof Johansson; +Cc: linuxppc-dev, sl, Philippe Lachenal
In-Reply-To: <20070606220913.GA27820@lixom.net>
Olof Johansson wrote:
> There's nothing wrong with the way he coded that up. Lots of drivers
> are written that way (all of mine are). It's at least as clear as any
> structure, and it doesn't cause temptation to do...
That vast majority of Freescale SOC device register maps are handled via a structure.
He's doing everything via 32-bit operations, even though the registers are 64 bits, and
therefore he has twice as much macros as he needs.
>>> +
>>> +
>>> + /* check for things like FIFO underflow */
>>> +
>>> + u32 v;
>>> +
>>> + v = in_be32(rng_regs + TALITOS_RNGISR_HI);
>> u64 v;
>> v = rng->rngisr;
>>
>> or something like that. Try to use the built-in support for 64-bit data types when possible.
>
> ...this. NO! Don't reference ioremapped memory from regular code like
> that. The way he's doing it is the preferred way.
Can you explain that better? What is "regular code"?
--
Timur Tabi
Linux Kernel Developer @ Freescale
^ permalink raw reply
* Re: MPC8349ea Random Device Generator driver
From: Olof Johansson @ 2007-06-06 22:09 UTC (permalink / raw)
To: Timur Tabi; +Cc: linuxppc-dev, sl, Philippe Lachenal
In-Reply-To: <46672DB4.80504@freescale.com>
On Wed, Jun 06, 2007 at 04:57:08PM -0500, Timur Tabi wrote:
> > +#include <asm/of_platform.h>
> > +#include <asm/io.h>
> > +
> > +#define TALITOS_RNGSR 0x028 /* RNG status register */
> > +#define TALITOS_RNGSR_HI 0x02c /* RNG status register */
> > +#define TALITOS_RNGSR_HI_RD 0x1 /* RNG Reset done */
> > +#define TALITOS_RNGSR_HI_OFL 0xff0000/* number of dwords in RNG output FIFO*/
> > +#define TALITOS_RNGDSR 0x010 /* RNG data size register */
> > +#define TALITOS_RNGDSR_HI 0x014 /* RNG data size register */
> > +#define TALITOS_RNG_FIFO 0x800 /* RNG FIFO - pool of random numbers */
> > +#define TALITOS_RNGISR 0x030 /* RNG Interrupt status register */
> > +#define TALITOS_RNGISR_HI 0x034 /* RNG Interrupt status register */
> > +#define TALITOS_RNGRCR 0x018 /* RNG Reset control register */
> > +#define TALITOS_RNGRCR_HI 0x01c /* RNG Reset control register */
>
> Please create a structure instead of using macros like this. Example:
>
> struct sec_rng {
> __be64 rngmr;
> u8 res1[8];
> __be64 rngdsr;
> __be64 rngrcr;
> ...
> };
>
> and then ...
There's nothing wrong with the way he coded that up. Lots of drivers
are written that way (all of mine are). It's at least as clear as any
structure, and it doesn't cause temptation to do...
> > +static int talitos_hwrng_data_present(struct hwrng *rng)
> > +{
> > + void __iomem *rng_regs = (void __iomem *)rng->priv;
>
> struct sec_rng __iomem *rng = (struct sec_rng __iomem *) rng->priv;
>
> > +
> > +
> > + /* check for things like FIFO underflow */
> > +
> > + u32 v;
> > +
> > + v = in_be32(rng_regs + TALITOS_RNGISR_HI);
>
> u64 v;
> v = rng->rngisr;
>
> or something like that. Try to use the built-in support for 64-bit data types when possible.
...this. NO! Don't reference ioremapped memory from regular code like
that. The way he's doing it is the preferred way.
-Olof
^ 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