cell: abstract spu management routines
Subsystems (2)
PATCH 6/16 cell: abstract spu management routines
This adds a platform specific spu management abstraction and the coresponding routines to support the IBM Cell Blade. It also removes the hypervisor only resources that were included in struct spu.
Three new platform specific routines are introduced, spu_enumerate_spus(), spu_create_spu() and spu_destroy_spu(). The underlining design uses a new type, struct spu_management_ops, to hold function pointers that the platform setup code is expected to initialize to instances appropriate to that platform.
For the IBM Cell Blade support, I put the hypervisor only resources that were in struct spu into a platform specific data structure struct spu_pdata.
---
Michael,
Unfortunately, for your xmon spu support, your DUMP_FIELD is setup in such a way that it is not easy to change to use from inside spu_priv1_mmio.c, so I left of_dump_pdata_fields() empty. We'll need to work on something usable there, or make some other way to abstract those platform specific spu variables.
arch/powerpc/platforms/cell/cbe_thermal.c | 5 arch/powerpc/platforms/cell/setup.c | 3 arch/powerpc/platforms/cell/spu_base.c | 308 +------------------- arch/powerpc/platforms/cell/spu_priv1_mmio.c | 406 +++++++++++++++++++++++++-- arch/powerpc/platforms/cell/spu_priv1_mmio.h | 26 + arch/powerpc/xmon/xmon.c | 8 include/asm-powerpc/spu.h | 5 include/asm-powerpc/spu_priv1.h | 48 ++- 8 files changed, 481 insertions(+), 328 deletions(-)
arch/powerpc/platforms/cell/cbe_thermal.c +3 -2
@@ -29,6 +29,7 @@#include <asm/prom.h> #include "cbe_regs.h" +#include "spu_priv1_mmio.h" static struct cbe_pmd_regs __iomem *get_pmd_regs (struct sys_device *sysdev) {
@@ -36,7 +37,7 @@ spu = container_of(sysdev, struct spu, sysdev); -return cbe_get_pmd_regs (spu->devnode); +return cbe_get_pmd_regs (spu_devnode (spu)); }/* returns the value for a given spu in a given register */
@@ -49,7 +50,7 @@/* getting the id from the reg attribute will not work on future device-tree layouts * in future we should store the id to the spu struct and use it here */ spu = container_of(sysdev, struct spu, sysdev); - id = (unsigned int *)get_property (spu->devnode,"reg" ,NULL ); + id = (unsigned int *)get_property (spu_devnode (spu),"reg" ,NULL ); value.val =in_be64 (®->val);return value.spe[*id];
arch/powerpc/platforms/cell/setup.c +2 -1
@@ -97,7 +97,8 @@static void __init cell_setup_arch (void ) {#ifdef CONFIG_SPU_BASE - spu_priv1_ops = &spu_priv1_mmio_ops; + spu_priv1_ops = &spu_priv1_mmio_ops; + spu_management_ops = &spu_management_of_ops;#endif cbe_regs_init ();
arch/powerpc/platforms/cell/spu_base.c +22 -286
381 lines not on this page · raw message
arch/powerpc/platforms/cell/spu_priv1_mmio.c +383 -23
503 lines not on this page · raw message
arch/powerpc/platforms/cell/spu_priv1_mmio.h new file +26 -0
@@ -0,0 +1,26 @@ +/* +* spu hypervisor abstraction for direct hardware access. +* +* Copyright (C) 2006 Sony Computer Entertainment Inc. +* Copyright 2006 Sony Corp. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; version 2 of the License. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +*/ + +#ifndef SPU_PRIV1_MMIO_H +#define SPU_PRIV1_MMIO_H + +struct device_node *spu_devnode (struct spu *spu); + +#endif /* SPU_PRIV1_MMIO_H */
arch/powerpc/xmon/xmon.c +1 -7
@@ -2769,8 +2769,6 @@DUMP_FIELD (spu,"0x%x" , number);DUMP_FIELD (spu,"%s" , name); -DUMP_FIELD (spu,"%s" , devnode->full_name); -DUMP_FIELD (spu,"0x%x" , nid);DUMP_FIELD (spu,"0x%lx" , local_store_phys);DUMP_FIELD (spu,"0x%p" , local_store);DUMP_FIELD (spu,"0x%lx" , ls_size);
@@ -2794,12 +2792,8 @@DUMP_FIELD (spu,"0x%x" , problem->spu_runcntl_RW);DUMP_FIELD (spu,"0x%x" , problem->spu_status_R);DUMP_FIELD (spu,"0x%x" , problem->spu_npc_RW); -DUMP_FIELD (spu,"0x%p" , priv1); - -if (spu->priv1) -DUMP_FIELD (spu,"0x%lx" , priv1->mfc_sr1_RW); -DUMP_FIELD (spu,"0x%p" , priv2); +spu_dump_pdata_fields (spu); }static int do_spu_cmd (void )
include/asm-powerpc/spu.h +1 -4
@@ -111,13 +111,11 @@u8 *local_store;unsigned long problem_phys;struct spu_problem __iomem *problem; -struct spu_priv1 __iomem *priv1;struct spu_priv2 __iomem *priv2;struct list_head list;struct list_head sched_list;struct list_head full_list;int number; -int nid;unsigned int irqs[3 ];u32 node;u64 flags;
@@ -144,8 +142,7 @@char irq_c1[8 ];char irq_c2[8 ]; -struct device_node *devnode; - +void * pdata;/* platform private data */ struct sys_device sysdev; };
include/asm-powerpc/spu_priv1.h +43 -5
67 lines not on this page · raw message
Quoted 11 lines · Geoff Levand
This adds a platform specific spu management abstraction and the coresponding routines to support the IBM Cell Blade. It also removes the hypervisor only resources that were included in struct spu.
Three new platform specific routines are introduced,
spu_enumerate_spus(),spu_create_spu()andspu_destroy_spu(). The underlining design uses a new type, structspu_management_ops, to hold function pointers that the platform setup code is expected to initialize to instances appropriate to that platform.For the IBM Cell Blade support, I put the hypervisor only resources that were in struct spu into a platform specific data structure struct
spu_pdata.
As far as I can see you haven't posted your HV-backed management ops, is that right?
Why can't your PS3 platform code fake-up device nodes for SPUs? It seems that would simplify this quite a lot.
cheers
Signature · 8 lines
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
Quoted 10 lines · Michael Ellerman
Quoted 9 lines · Geoff Levand
This adds a platform specific spu management abstraction and the coresponding routines to support the IBM Cell Blade. It also removes the hypervisor only resources that were included in struct spu.
Three new platform specific routines are introduced,
spu_enumerate_spus(),spu_create_spu()andspu_destroy_spu(). The underlining design uses a new type, structspu_management_ops, to hold function pointers that the platform setup code is expected to initialize to instances appropriate to that platform.
Quoted 5 lines
Quoted 2 lines
For the IBM Cell Blade support, I put the hypervisor only resources that were in struct spu into a platform specific data structure struct
spu_pdata.As far as I can see you haven't posted your HV-backed management ops, is that right?
Yes, that is in '[PATCH 15/16] cell: add ps3 platform spu support' posted with the other ps3pf patches.
Quoted 2 lines
Why can't your PS3 platform code fake-up device nodes for SPUs? It seems that would simplify this quite a lot.
Seems like a hack to me. My concern is that I just have to keep adding some extra hack for every new spu feature that comes out. I would prefer to make a proper design from the start, but if anyone can be more convincing I am open to suggestions.
-Geoff
Quoted 14 lines · Geoff Levand
Quoted 13 lines · Michael Ellerman
Quoted 11 lines · Geoff Levand
This adds a platform specific spu management abstraction and the coresponding routines to support the IBM Cell Blade. It also removes the hypervisor only resources that were included in struct spu.
Three new platform specific routines are introduced,
spu_enumerate_spus(),spu_create_spu()andspu_destroy_spu(). The underlining design uses a new type, structspu_management_ops, to hold function pointers that the platform setup code is expected to initialize to instances appropriate to that platform.For the IBM Cell Blade support, I put the hypervisor only resources that were in struct spu into a platform specific data structure struct
spu_pdata.
Quoted 6 lines
Quoted 2 lines
As far as I can see you haven't posted your HV-backed management ops, is that right?
Yes, that is in '[PATCH 15/16] cell: add ps3 platform spu support' posted with the other ps3pf patches.
OK, I'll have a look at it.
Quoted 8 lines
Quoted 2 lines
Why can't your PS3 platform code fake-up device nodes for SPUs? It seems that would simplify this quite a lot.
Seems like a hack to me. My concern is that I just have to keep adding some extra hack for every new spu feature that comes out. I would prefer to make a proper design from the start, but if anyone can be more convincing I am open to suggestions.
Well the whole thrust of the flattened-device-tree model, is that we do as much platform-specific hackery in a boot-loader/early-init, and present the hardware in as standard a way as possible to the kernel via the device tree.
The hope is that this isolates most of the kernel from platform specific details, as far as is possible - there will always be some things that need to be abstracted out - for that we have ppc_md and a few other callbacks.
The priv1_ops serve that purpose, providing callbacks, and there's really no way around that - you can't tap the priv1 area when you're running under a HV - fine. But for just finding the spus it strikes me that it would be _nicer_, perhaps not easier :), to have your "enumerate_spus" populate the flat device tree early on - which would leave more of the spu code untouched by the hv/bare-metal issue.
But as I said before, I haven't looked thoroughly at the code, so perhaps there's some obvious reason why that wouldn't work.
cheers
Signature · 8 lines
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
Quoted 4 lines · Michael Ellerman
Quoted 4 lines
Quoted 2 lines
Why can't your PS3 platform code fake-up device nodes for SPUs? It seems that would simplify this quite a lot.
Quoted 8 lines
Quoted 4 lines
Seems like a hack to me. My concern is that I just have to keep adding some extra hack for every new spu feature that comes out. I would prefer to make a proper design from the start, but if anyone can be more convincing I am open to suggestions.
Well the whole thrust of the flattened-device-tree model, is that we do as much platform-specific hackery in a boot-loader/early-init, and present the hardware in as standard a way as possible to the kernel via
The thing is that the spus are visualized, so to create one takes up HV resources, mainly HV memory. Creating spus in the bootloader has several problems. One is that you could be allocating HV memory that would never be used if the kernel is not configured for spu support, and this is memory could be used for other HV support. Another problem is the management of those HV resources across kernel reloads, with kexec for example. If the management is split then both entities need to have knowledge of the other, which complicates things.
Quoted 11 lines
The hope is that this isolates most of the kernel from platform specific details, as far as is possible - there will always be some things that need to be abstracted out - for that we have
ppc_mdand a few other callbacks.The
priv1_opsserve that purpose, providing callbacks, and there's really no way around that - you can't tap the priv1 area when you're running under a HV - fine. But for just finding the spus it strikes me that it would be _nicer_, perhaps not easier :), to have your "enumerate_spus" populate the flat device tree early on - which would leave more of the spu code untouched by the hv/bare-metal issue.
And how many would you like to find? 1? 5? 400? Although there is a current limitation in the HV implementation, these are logical spus. It would seem the kernel could create spus based on the need, and thus better balance resource usage, but this is not at all how the current spu code works though. I don't plan to do any work on this, but it would be nice to keep it open.
-Geoff
Quoted 15 lines · Geoff Levand
Quoted 4 lines · Michael Ellerman
Quoted 4 lines
Quoted 2 lines
Why can't your PS3 platform code fake-up device nodes for SPUs? It seems that would simplify this quite a lot.
Quoted 8 lines
Quoted 4 lines
Seems like a hack to me. My concern is that I just have to keep adding some extra hack for every new spu feature that comes out. I would prefer to make a proper design from the start, but if anyone can be more convincing I am open to suggestions.
Well the whole thrust of the flattened-device-tree model, is that we do as much platform-specific hackery in a boot-loader/early-init, and present the hardware in as standard a way as possible to the kernel via
Quoted 8 lines
The thing is that the spus are visualized, so to create one takes up HV resources, mainly HV memory. Creating spus in the bootloader has several problems. One is that you could be allocating HV memory that would never be used if the kernel is not configured for spu support, and this is memory could be used for other HV support. Another problem is the management of those HV resources across kernel reloads, with kexec for example. If the management is split then both entities need to have knowledge of the other, which complicates things.
Yeah I knew you were going to say that :) How much memory does it take in the HV to create a "logical spu"?
Kexec might complicate things, is it really high on your feature list?
Quoted 13 lines
Quoted 11 lines
The hope is that this isolates most of the kernel from platform specific details, as far as is possible - there will always be some things that need to be abstracted out - for that we have
ppc_mdand a few other callbacks.The
priv1_opsserve that purpose, providing callbacks, and there's really no way around that - you can't tap the priv1 area when you're running under a HV - fine. But for just finding the spus it strikes me that it would be _nicer_, perhaps not easier :), to have your "enumerate_spus" populate the flat device tree early on - which would leave more of the spu code untouched by the hv/bare-metal issue.
Quoted 6 lines
And how many would you like to find? 1? 5? 400? Although there is a current limitation in the HV implementation, these are logical spus. It would seem the kernel could create spus based on the need, and thus better balance resource usage, but this is not at all how the current spu code works though. I don't plan to do any work on this, but it would be nice to keep it open.
Actually I'd like 8, or is it 7. I don't see why having more "logical spus" than "physical spus" is useful - the kernel can already schedule many spu contexts over a smaller number of physical spus. As far as giving unused spus back to the HV .. I'll believe it when I see it :)
cheers
Signature · 8 lines
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
Quoted 25 lines · Michael Ellerman
Quoted 15 lines · Geoff Levand
Quoted 4 lines · Michael Ellerman
Quoted 4 lines
Quoted 2 lines
Why can't your PS3 platform code fake-up device nodes for SPUs? It seems that would simplify this quite a lot.
Quoted 8 lines
Quoted 4 lines
Seems like a hack to me. My concern is that I just have to keep adding some extra hack for every new spu feature that comes out. I would prefer to make a proper design from the start, but if anyone can be more convincing I am open to suggestions.
Well the whole thrust of the flattened-device-tree model, is that we do as much platform-specific hackery in a boot-loader/early-init, and present the hardware in as standard a way as possible to the kernel via
Quoted 8 lines
The thing is that the spus are visualized, so to create one takes up HV resources, mainly HV memory. Creating spus in the bootloader has several problems. One is that you could be allocating HV memory that would never be used if the kernel is not configured for spu support, and this is memory could be used for other HV support. Another problem is the management of those HV resources across kernel reloads, with kexec for example. If the management is split then both entities need to have knowledge of the other, which complicates things.
Quoted 2 lines
Yeah I knew you were going to say that :) How much memory does it take in the HV to create a "logical spu"?
I don't know the exact amount. I guess with the current implementation it is minimal though, since it now only supports one-to-one physical to logical.
Quoted 1 line
Kexec might complicate things, is it really high on your feature list?
Yes, the bootloader needs USB HID for the menu, plus the capability to load the kernel with tftp or from HD, usb storage, etc. The only way currently is with kboot. No other bootloader has been ported, and there is no plan to do any other.
Quoted 20 lines
Quoted 13 lines
Quoted 11 lines
The hope is that this isolates most of the kernel from platform specific details, as far as is possible - there will always be some things that need to be abstracted out - for that we have
ppc_mdand a few other callbacks.The
priv1_opsserve that purpose, providing callbacks, and there's really no way around that - you can't tap the priv1 area when you're running under a HV - fine. But for just finding the spus it strikes me that it would be _nicer_, perhaps not easier :), to have your "enumerate_spus" populate the flat device tree early on - which would leave more of the spu code untouched by the hv/bare-metal issue.Quoted 6 lines
And how many would you like to find? 1? 5? 400? Although there is a current limitation in the HV implementation, these are logical spus. It would seem the kernel could create spus based on the need, and thus better balance resource usage, but this is not at all how the current spu code works though. I don't plan to do any work on this, but it would be nice to keep it open.
Quoted 4 lines
Actually I'd like 8, or is it 7. I don't see why having more "logical spus" than "physical spus" is useful - the kernel can already schedule many spu contexts over a smaller number of physical spus. As far as giving unused spus back to the HV .. I'll believe it when I see it :)
The way it works is that the policy module reserves a number of the physical spus for the partition. So for example, on a machine that has eight spus, it might reserve five for a partition. Now that would mean the guest could have 100% of the five, and the potential to share some of the others with another partition, but the current implementation does not support the sharing part, so you only get the reserved ones. If the guest creates and destroys spus on demand, the unused ones can be used to run shared contexts in other partitions, at least that is what the design allows for.
-Geoff
Quoted 17 lines · Geoff Levand
This adds a platform specific spu management abstraction and the coresponding routines to support the IBM Cell Blade. It also removes the hypervisor only resources that were included in struct spu.
Three new platform specific routines are introduced,
spu_enumerate_spus(),spu_create_spu()andspu_destroy_spu(). The underlining design uses a new type, structspu_management_ops, to hold function pointers that the platform setup code is expected to initialize to instances appropriate to that platform.For the IBM Cell Blade support, I put the hypervisor only resources that were in struct spu into a platform specific data structure struct
spu_pdata.Signed-off-by Geoff Levand <[email protected]>---
Quoted 7 lines
Michael,
Unfortunately, for your xmon spu support, your
DUMP_FIELDis setup in such a way that it is not easy to change to use from insidespu_priv1_mmio.c, so I leftof_dump_pdata_fields()empty. We'll need to work on something usable there, or make some other way to abstract those platform specific spu variables.
OK, back to the task at hand :) For the moment I'd rather see you leave out dump_data_fields(), as neither HV or baremetal implementations do anything. Just put the offending xmon code inside an #ifdef CONFIG_PPC_CELL_NATIVE. eg:
arch/powerpc/xmon/xmon.c 2006-11-14 14:42:35.000000000 +1100 renamed from arch/powerpc/xmon/xmon.c 2006-11-14 14:43:11.000000000 +1100 +4 -5
@@ -2807,12 +2807,11 @@ static void dump_spu_fields(struct spu * in_be32(&spu->problem->spu_status_R)); DUMP_VALUE("0x%x", problem->spu_npc_RW, in_be32(&spu->problem->spu_npc_RW)); +#ifdef CONFIG_PPC_CELL_NATIVE DUMP_FIELD(spu, "0x%p", priv1); - - if (spu->priv1) { - DUMP_VALUE("0x%lx", priv1->mfc_sr1_RW, - in_be64(&spu->priv1->mfc_sr1_RW)); - } + DUMP_VALUE("0x%lx", priv1->mfc_sr1_RW, + in_be64(&spu->priv1->mfc_sr1_RW)); +#endif DUMP_FIELD(spu, "0x%p", priv2); }
cheers
Signature · 8 lines
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
Quoted 7 lines · Michael Ellerman
OK, back to the task at hand :) =A0For the moment I'd rather see you leave out
dump_data_fields(), as neither HV or baremetal implementations do anything. Just put the offending xmon code inside an #ifdefCONFIG_PPC_CELL_NATIVE. eg:
=20
Index:cell/arch/powerpc/xmon/xmon.c=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
Quoted 1 line
---
cell.orig/arch/powerpc/xmon/xmon.c=A02006-11-14 14:43:11.000000000 +=
1100
Quoted 1 line
+++
cell/arch/powerpc/xmon/xmon.c=A0 =A0 =A0 2006-11-14 14:42:35.0000000=
00 +1100
Quoted 2 lines
(quoted diff) +0
-0@@ -2807,12 +2807,11 @@ static void dump_spu_fields(struct spu *=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 in_be32(&spu->problem->sp=
u_status_R));
Quoted 2 lines
=A0 =A0 =A0 =A0 DUMP_VALUE("0x%x",
problem->spu_npc_RW,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 in_be32(&spu->problem->sp=
u_npc_RW));
Quoted 6 lines
+#ifdef
CONFIG_PPC_CELL_NATIVE
=A0 =A0 =A0 =A0DUMP_FIELD(spu, "0x%p", priv1);
-
- =A0 =A0 =A0 if (spu->priv1) {
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 DUMP_VALUE("0x%lx",priv1->mfc_sr1_RW,
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 in_be64(&sp=
u->priv1->mfc_sr1_RW));
Quoted 3 lines
- =A0 =A0 =A0 }
+ =A0 =A0 =A0 DUMP_VALUE("0x%lx",priv1->mfc_sr1_RW,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 in_be64(&spu->priv1->mfc_sr=
1_RW));
Quoted 1 line
+#endif
Oops. Null pointer dereference.
You can't do this if you want to compile in both native and PS3PF support in a single kernel. I think your original code that prints priv1 and the sr1 only if priv1 exists is fine on both ways.
Arnd <><
QXJuZCBCZXJnbWFubiB3cm90ZToNCj4gT24gVHVlc2RheSAxNCBOb3ZlbWJlciAyMDA2IDA0OjQ0 LCBNaWNoYWVsIEVsbGVybWFuIHdyb3RlOg0KPj4gT0ssIGJhY2sgdG8gdGhlIHRhc2sgYXQgaGFu ZCA6KSDvv71Gb3IgdGhlIG1vbWVudCBJJ2QgcmF0aGVyIHNlZSB5b3UgbGVhdmUNCj4+IG91dCBk dW1wX2RhdGFfZmllbGRzKCksIGFzIG5laXRoZXIgSFYgb3IgYmFyZW1ldGFsIGltcGxlbWVudGF0 aW9ucyBkbw0KPj4gYW55dGhpbmcuIEp1c3QgcHV0IHRoZSBvZmZlbmRpbmcgeG1vbiBjb2RlIGlu c2lkZSBhbiAjaWZkZWYNCj4+IENPTkZJR19QUENfQ0VMTF9OQVRJVkUuIGVnOg0KPj4gDQo+PiBJ bmRleDogY2VsbC9hcmNoL3Bvd2VycGMveG1vbi94bW9uLmMNCj4+ID09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCj4+IC0t LSBjZWxsLm9yaWcvYXJjaC9wb3dlcnBjL3htb24veG1vbi5jIO+/vTIwMDYtMTEtMTQgMTQ6NDM6 MTEuMDAwMDAwMDAwICsxMTAwDQo+PiArKysgY2VsbC9hcmNoL3Bvd2VycGMveG1vbi94bW9uLmMg 77+9IO+/vSDvv70gMjAwNi0xMS0xNCAxNDo0MjozNS4wMDAwMDAwMDAgKzExMDANCj4+IEBAIC0y ODA3LDEyICsyODA3LDExIEBAIHN0YXRpYyB2b2lkIGR1bXBfc3B1X2ZpZWxkcyhzdHJ1Y3Qgc3B1 ICoNCj4+IO+/vSDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IGlu X2JlMzIoJnNwdS0+cHJvYmxlbS0+c3B1X3N0YXR1c19SKSk7DQo+PiDvv70g77+9IO+/vSDvv70g RFVNUF9WQUxVRSgiMHgleCIsIHByb2JsZW0tPnNwdV9ucGNfUlcsDQo+PiDvv70g77+9IO+/vSDv v70g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IO+/vSBpbl9iZTMyKCZzcHUtPnByb2JsZW0t PnNwdV9ucGNfUlcpKTsNCj4+ICsjaWZkZWYgQ09ORklHX1BQQ19DRUxMX05BVElWRQ0KPj4g77+9 IO+/vSDvv70g77+9IERVTVBfRklFTEQoc3B1LCAiMHglcCIsIHByaXYxKTsNCj4+IC0NCj4+IC0g 77+9IO+/vSDvv70gaWYgKHNwdS0+cHJpdjEpIHsNCj4+IC0g77+9IO+/vSDvv70g77+9IO+/vSDv v70g77+9IERVTVBfVkFMVUUoIjB4JWx4IiwgcHJpdjEtPm1mY19zcjFfUlcsDQo+PiAtIO+/vSDv v70g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IGlu X2JlNjQoJnNwdS0+cHJpdjEtPm1mY19zcjFfUlcpKTsNCj4+IC0g77+9IO+/vSDvv70gfQ0KPj4g KyDvv70g77+9IO+/vSBEVU1QX1ZBTFVFKCIweCVseCIsIHByaXYxLT5tZmNfc3IxX1JXLA0KPj4g KyDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IGluX2JlNjQoJnNw dS0+cHJpdjEtPm1mY19zcjFfUlcpKTsNCj4+ICsjZW5kaWYNCj4gDQo+IE9vcHMuIE51bGwgcG9p bnRlciBkZXJlZmVyZW5jZS4NCj4gDQo+IFlvdSBjYW4ndCBkbyB0aGlzIGlmIHlvdSB3YW50IHRv IGNvbXBpbGUgaW4gYm90aCBuYXRpdmUgYW5kIFBTM1BGDQo+IHN1cHBvcnQgaW4gYSBzaW5nbGUg a2VybmVsLiBJIHRoaW5rIHlvdXIgb3JpZ2luYWwgY29kZSB0aGF0IHByaW50cw0KPiBwcml2MSBh bmQgdGhlIHNyMSBvbmx5IGlmIHByaXYxIGV4aXN0cyBpcyBmaW5lIG9uIGJvdGggd2F5cy4NCg0K Tm8sIHRoYXQgd29uJ3Qgd29yayBlaXRoZXIgc2luY2UgSSBtb3ZlZCBwcml2MSB0byBzdHJ1Y3Qg c3B1X3BkYXRhLg0KDQorc3RydWN0IHNwdV9wZGF0YSB7DQorCWludCBuaWQ7DQorCXN0cnVjdCBk ZXZpY2Vfbm9kZSAqZGV2bm9kZTsNCisJc3RydWN0IHNwdV9wcml2MSBfX2lvbWVtICpwcml2MTsN Cit9Ow0KDQpUaGUgb25seSB3YXkgSSBzZWUgdGhpcyB3b3JraW5nIGlzIHRvIGhhdmUgYSBwbGF0 Zm9ybSBzcGVjaWZpYw0Kcm91dGluZSB0byBkdW1wIHRob3NlIHNwdV9wZGF0YSB2YXJpYWJsZXMu ICBUaGUgcHJvYmxlbSBpcyB0aGF0DQpvbmx5IHRoZSBzcHUgcGxhdGZvcm0gY29kZSBrbm93cyBh Ym91dCB0aGUgdmFyaWFibGVzLCBhbmQgb25seSB0aGUNCnhtb24gY29kZSBrbm93cyBob3cgdG8g ZG8gdGhlIGR1bXAuICBUaGF0IGlzIHdoeSBJIHN1Z2dlc3RlZCBlYXJsaWVyDQp0byByZS13b3Jr IHRoZSB4bW9uIGNvZGUgdG8gbWFrZSBhIGR1bXAgcm91dGluZSB3aXRoIGdsb2JhbCBzY29wZQ0K dGhhdCBjYW4gYmUgY2FsbGVkIGZyb20gdGhlIHZhcmlvdXMgc3B1IHBsYXRmb3JtIA0Kc3B1X2R1 bXBfcGRhdGFfZmllbGRzKCkgcm91dGluZXMuDQoNCk5vdGUgdGhhdCB0aGlzIGlzIG5vdCBzcGVj aWZpYyB0byB0aGUgbmF0aXZlIHN1cHBvcnQsIHNpbmNlIEkgd2FudA0KdG8gaG9vayBteSBwbGF0 Zm9ybSBjb2RlIGludG8geG1vbiBhbHNvLCBhbmQgSSBuZWVkIHRvIHNvbWVob3cNCmR1bXAgbXkg dmFyaWFibGVzLiAgU28gdGhlIHNvbHV0aW9uIGlzIG5vdCB0byBhZGQgcHJpdjEgYmFjayBpbnRv DQpzdHJ1Y3Qgc3B1Lg0KDQotR2VvZmYNCg==
Quoted 34 lines · Geoff Levand
Quoted 27 lines · Arnd Bergmann
Quoted 13 lines · Michael Ellerman
OK, back to the task at hand :) �For the moment I'd rather see you leave out
dump_data_fields(), as neither HV or baremetal implementations do anything. Just put the offending xmon code inside an #ifdefCONFIG_PPC_CELL_NATIVE. eg:Index:
cell/arch/powerpc/xmon/xmon.c
===================================================================arch/powerpc/xmon/xmon.c � � � 2006-11-14 14:42:35.000000000 +1100 renamed from arch/powerpc/xmon/xmon.c �2006-11-14 14:43:11.000000000 +1100 +0
-0@@ -2807,12 +2807,11 @@ static void dump_spu_fields(struct spu * � � � � � � � � � � � � in_be32(&spu->problem->spu_status_R)); � � � � DUMP_VALUE("0x%x", problem->spu_npc_RW, � � � � � � � � � � � � in_be32(&spu->problem->spu_npc_RW));arch/powerpc/xmon/xmon.c � � � 2006-11-14 14:42:35.000000000 +1100 static void dump_spu_fields(struct spu * · line 2814
Quoted 10 lines
arch/powerpc/xmon/xmon.c � � � 2006-11-14 14:42:35.000000000 +1100 renamed from arch/powerpc/xmon/xmon.c �2006-11-14 14:43:11.000000000 +1100 +4
-5+#ifdef CONFIG_PPC_CELL_NATIVE � � � � DUMP_FIELD(spu, "0x%p", priv1); - - � � � if (spu->priv1) { - � � � � � � � DUMP_VALUE("0x%lx", priv1->mfc_sr1_RW, - � � � � � � � � � � � � � � � in_be64(&spu->priv1->mfc_sr1_RW)); - � � � } + � � � DUMP_VALUE("0x%lx", priv1->mfc_sr1_RW, + � � � � � � � � � � � in_be64(&spu->priv1->mfc_sr1_RW)); +#endifOops. Null pointer dereference.
Quoted 3 lines
You can't do this if you want to compile in both native and PS3PF support in a single kernel. I think your original code that prints priv1 and the sr1 only if priv1 exists is fine on both ways.
No, that won't work either since I moved priv1 to struct
spu_pdata.
Review
Quoted 5 lines
(quoted diff) +5
-0+struct spu_pdata { + int nid; + struct device_node *devnode; + struct spu_priv1 __iomem *priv1; +};
OK, I wasn't very clear. I didn't mean that patch was the final solution
- obviously it needs to take into account the movement of priv1 etc.
Quoted 8 lines
The only way I see this working is to have a platform specific routine to dump those
spu_pdatavariables. The problem is that only the spu platform code knows about the variables, and only the xmon code knows how to do the dump. That is why I suggested earlier to re-work the xmon code to make a dump routine with global scope that can be called from the various spu platformspu_dump_pdata_fields()routines.
Quoted 4 lines
Note that this is not specific to the native support, since I want to hook my platform code into xmon also, and I need to somehow dump my variables. So the solution is not to add priv1 back into struct spu.
I'm not that bothered, but I really don't think we want to spend too much effort engineering interfaces between xmon and the spu code - at the end of the day xmon is just a hackish debugging aid for _kernel developers_.
The simplest solution might just be to dump the address of the pdata pointer, and leave it up to the xmon user to dump that and interpret it as they see fit.
cheers
Signature · 8 lines
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
TWljaGFlbCBFbGxlcm1hbiB3cm90ZToNCj4gT24gVHVlLCAyMDA2LTExLTE0IGF0IDAyOjUwIC0w ODAwLCBHZW9mZiBMZXZhbmQgd3JvdGU6DQo+PiBBcm5kIEJlcmdtYW5uIHdyb3RlOg0KPj4gPiBP biBUdWVzZGF5IDE0IE5vdmVtYmVyIDIwMDYgMDQ6NDQsIE1pY2hhZWwgRWxsZXJtYW4gd3JvdGU6 DQo+PiA+PiBPSywgYmFjayB0byB0aGUgdGFzayBhdCBoYW5kIDopIO+/vUZvciB0aGUgbW9tZW50 IEknZCByYXRoZXIgc2VlIHlvdSBsZWF2ZQ0KPj4gPj4gb3V0IGR1bXBfZGF0YV9maWVsZHMoKSwg YXMgbmVpdGhlciBIViBvciBiYXJlbWV0YWwgaW1wbGVtZW50YXRpb25zIGRvDQo+PiA+PiBhbnl0 aGluZy4gSnVzdCBwdXQgdGhlIG9mZmVuZGluZyB4bW9uIGNvZGUgaW5zaWRlIGFuICNpZmRlZg0K Pj4gPj4gQ09ORklHX1BQQ19DRUxMX05BVElWRS4gZWc6DQo+PiA+PiANCj4+ID4+IEluZGV4OiBj ZWxsL2FyY2gvcG93ZXJwYy94bW9uL3htb24uYw0KPj4gPj4gPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KPj4gPj4gLS0t IGNlbGwub3JpZy9hcmNoL3Bvd2VycGMveG1vbi94bW9uLmMg77+9MjAwNi0xMS0xNCAxNDo0Mzox MS4wMDAwMDAwMDAgKzExMDANCj4+ID4+ICsrKyBjZWxsL2FyY2gvcG93ZXJwYy94bW9uL3htb24u YyDvv70g77+9IO+/vSAyMDA2LTExLTE0IDE0OjQyOjM1LjAwMDAwMDAwMCArMTEwMA0KPj4gPj4g QEAgLTI4MDcsMTIgKzI4MDcsMTEgQEAgc3RhdGljIHZvaWQgZHVtcF9zcHVfZmllbGRzKHN0cnVj dCBzcHUgKg0KPj4gPj4g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IO+/ vSDvv70gaW5fYmUzMigmc3B1LT5wcm9ibGVtLT5zcHVfc3RhdHVzX1IpKTsNCj4+ID4+IO+/vSDv v70g77+9IO+/vSBEVU1QX1ZBTFVFKCIweCV4IiwgcHJvYmxlbS0+c3B1X25wY19SVywNCj4+ID4+ IO+/vSDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IGluX2JlMzIo JnNwdS0+cHJvYmxlbS0+c3B1X25wY19SVykpOw0KPj4gPj4gKyNpZmRlZiBDT05GSUdfUFBDX0NF TExfTkFUSVZFDQo+PiA+PiDvv70g77+9IO+/vSDvv70gRFVNUF9GSUVMRChzcHUsICIweCVwIiwg cHJpdjEpOw0KPj4gPj4gLQ0KPj4gPj4gLSDvv70g77+9IO+/vSBpZiAoc3B1LT5wcml2MSkgew0K Pj4gPj4gLSDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70gRFVNUF9WQUxVRSgiMHglbHgiLCBw cml2MS0+bWZjX3NyMV9SVywNCj4+ID4+IC0g77+9IO+/vSDvv70g77+9IO+/vSDvv70g77+9IO+/ vSDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70gaW5fYmU2NCgmc3B1LT5wcml2MS0+bWZjX3Ny MV9SVykpOw0KPj4gPj4gLSDvv70g77+9IO+/vSB9DQo+PiA+PiArIO+/vSDvv70g77+9IERVTVBf VkFMVUUoIjB4JWx4IiwgcHJpdjEtPm1mY19zcjFfUlcsDQo+PiA+PiArIO+/vSDvv70g77+9IO+/ vSDvv70g77+9IO+/vSDvv70g77+9IO+/vSDvv70gaW5fYmU2NCgmc3B1LT5wcml2MS0+bWZjX3Ny MV9SVykpOw0KPj4gPj4gKyNlbmRpZg0KPj4gPiANCj4+ID4gT29wcy4gTnVsbCBwb2ludGVyIGRl cmVmZXJlbmNlLg0KPj4gPiANCj4+ID4gWW91IGNhbid0IGRvIHRoaXMgaWYgeW91IHdhbnQgdG8g Y29tcGlsZSBpbiBib3RoIG5hdGl2ZSBhbmQgUFMzUEYNCj4+ID4gc3VwcG9ydCBpbiBhIHNpbmds ZSBrZXJuZWwuIEkgdGhpbmsgeW91ciBvcmlnaW5hbCBjb2RlIHRoYXQgcHJpbnRzDQo+PiA+IHBy aXYxIGFuZCB0aGUgc3IxIG9ubHkgaWYgcHJpdjEgZXhpc3RzIGlzIGZpbmUgb24gYm90aCB3YXlz Lg0KPj4gDQo+PiBObywgdGhhdCB3b24ndCB3b3JrIGVpdGhlciBzaW5jZSBJIG1vdmVkIHByaXYx IHRvIHN0cnVjdCBzcHVfcGRhdGEuDQo+PiANCj4+ICtzdHJ1Y3Qgc3B1X3BkYXRhIHsNCj4+ICsJ aW50IG5pZDsNCj4+ICsJc3RydWN0IGRldmljZV9ub2RlICpkZXZub2RlOw0KPj4gKwlzdHJ1Y3Qg c3B1X3ByaXYxIF9faW9tZW0gKnByaXYxOw0KPj4gK307DQo+IA0KPiBPSywgSSB3YXNuJ3QgdmVy eSBjbGVhci4gSSBkaWRuJ3QgbWVhbiB0aGF0IHBhdGNoIHdhcyB0aGUgZmluYWwgc29sdXRpb24N Cj4gLSBvYnZpb3VzbHkgaXQgbmVlZHMgdG8gdGFrZSBpbnRvIGFjY291bnQgdGhlIG1vdmVtZW50 IG9mIHByaXYxIGV0Yy4NCj4gDQo+PiBUaGUgb25seSB3YXkgSSBzZWUgdGhpcyB3b3JraW5nIGlz IHRvIGhhdmUgYSBwbGF0Zm9ybSBzcGVjaWZpYw0KPj4gcm91dGluZSB0byBkdW1wIHRob3NlIHNw dV9wZGF0YSB2YXJpYWJsZXMuICBUaGUgcHJvYmxlbSBpcyB0aGF0DQo+PiBvbmx5IHRoZSBzcHUg cGxhdGZvcm0gY29kZSBrbm93cyBhYm91dCB0aGUgdmFyaWFibGVzLCBhbmQgb25seSB0aGUNCj4+ IHhtb24gY29kZSBrbm93cyBob3cgdG8gZG8gdGhlIGR1bXAuICBUaGF0IGlzIHdoeSBJIHN1Z2dl c3RlZCBlYXJsaWVyDQo+PiB0byByZS13b3JrIHRoZSB4bW9uIGNvZGUgdG8gbWFrZSBhIGR1bXAg cm91dGluZSB3aXRoIGdsb2JhbCBzY29wZQ0KPj4gdGhhdCBjYW4gYmUgY2FsbGVkIGZyb20gdGhl IHZhcmlvdXMgc3B1IHBsYXRmb3JtIA0KPj4gc3B1X2R1bXBfcGRhdGFfZmllbGRzKCkgcm91dGlu ZXMuDQo+PiANCj4+IE5vdGUgdGhhdCB0aGlzIGlzIG5vdCBzcGVjaWZpYyB0byB0aGUgbmF0aXZl IHN1cHBvcnQsIHNpbmNlIEkgd2FudA0KPj4gdG8gaG9vayBteSBwbGF0Zm9ybSBjb2RlIGludG8g eG1vbiBhbHNvLCBhbmQgSSBuZWVkIHRvIHNvbWVob3cNCj4+IGR1bXAgbXkgdmFyaWFibGVzLiAg U28gdGhlIHNvbHV0aW9uIGlzIG5vdCB0byBhZGQgcHJpdjEgYmFjayBpbnRvDQo+PiBzdHJ1Y3Qg c3B1Lg0KPiANCj4gSSdtIG5vdCB0aGF0IGJvdGhlcmVkLCBidXQgSSByZWFsbHkgZG9uJ3QgdGhp bmsgd2Ugd2FudCB0byBzcGVuZCB0b28NCj4gbXVjaCBlZmZvcnQgZW5naW5lZXJpbmcgaW50ZXJm YWNlcyBiZXR3ZWVuIHhtb24gYW5kIHRoZSBzcHUgY29kZSAtIGF0DQo+IHRoZSBlbmQgb2YgdGhl IGRheSB4bW9uIGlzIGp1c3QgYSBoYWNraXNoIGRlYnVnZ2luZyBhaWQgZm9yIF9rZXJuZWwNCj4g ZGV2ZWxvcGVyc18uDQo+IA0KPiBUaGUgc2ltcGxlc3Qgc29sdXRpb24gbWlnaHQganVzdCBiZSB0 byBkdW1wIHRoZSBhZGRyZXNzIG9mIHRoZSBwZGF0YQ0KPiBwb2ludGVyLCBhbmQgbGVhdmUgaXQg dXAgdG8gdGhlIHhtb24gdXNlciB0byBkdW1wIHRoYXQgYW5kIGludGVycHJldCBpdA0KPiBhcyB0 aGV5IHNlZSBmaXQuDQoNCk9LLCB0aGF0J3MgYSBnb29kIHNvbHV0aW9uIGZvciBub3cuICBJJ2xs IHNldCBpdCB1cCB0aGF0IHdheSBpbiBteSBwYXRjaGVzLg0KDQotR2VvZmYgDQo=