Skip to contentSkip to conversation

powerpc: add support for ps3 platform

PATCH In review Geoff Levand 8 participants 35 messages Nov 10, 2006 – Nov 21, 2006 PowerPC

lore
Share & tools
lore
#0 lore raw

PATCH 7/16 powerpc: add support for ps3 platform

This adds the core platform support for the PS3 game console and other platforms using the PS3 Platform hypervisor.

---
 MAINTAINERS                             |    7 
 arch/powerpc/Kconfig                    |   11 
 arch/powerpc/platforms/Makefile         |    1 
 arch/powerpc/platforms/ps3pf/Kconfig    |   32 +
 arch/powerpc/platforms/ps3pf/Makefile   |    2 
 arch/powerpc/platforms/ps3pf/mm.c       |  872 ++++++++++++++++++++++++++++++++
 arch/powerpc/platforms/ps3pf/platform.h |   59 ++
 arch/powerpc/platforms/ps3pf/setup.c    |  177 ++++++
 arch/powerpc/platforms/ps3pf/smp.c      |  164 ++++++
 arch/powerpc/platforms/ps3pf/time.c     |  105 +++
 include/asm-powerpc/ps3pf.h             |  240 ++++++++
 11 files changed, 1669 insertions(+), 1 deletion(-)
MAINTAINERS +7 -0
Index: cell--common--6/MAINTAINERS
===================================================================
@@ -2423,6 +2423,13 @@
 W:	http://www.pnd-pc.demon.co.uk/promise/
 S:	Maintained
 
+PS3PF (PS3 PLATFORM) SUPPORT
+P:	Geoff Levand
+M:	[email protected]
+L:	[email protected]
+L:	[email protected]
+S:	Supported
+
 PVRUSB2 VIDEO4LINUX DRIVER
 P:	Mike Isely
 M:	[email protected]
arch/powerpc/Kconfig +10 -1
Index: cell--common--6/arch/powerpc/Kconfig
===================================================================
@@ -461,6 +461,14 @@
 	depends on PPC_RTAS
 	default n
 
+config PS3PF
+	bool "PS3 Platform"
+	depends on PPC_MULTIPLATFORM && PPC64
+	select PPC_CELL
+	help
+	  This option enables support for the PS3 game console
+	  and other platforms using the PS3 Platform hypervisor.
+
 config XICS
 	depends on PPC_PSERIES
 	bool
@@ -604,6 +612,7 @@
 source arch/powerpc/platforms/86xx/Kconfig
 source arch/powerpc/platforms/8xx/Kconfig
 source arch/powerpc/platforms/cell/Kconfig
+source arch/powerpc/platforms/ps3pf/Kconfig
 
 menu "Kernel options"
 
@@ -876,7 +885,7 @@
 	bool "PCI support" if 40x || CPM2 || PPC_83xx || PPC_85xx || PPC_86xx \
 		|| PPC_MPC52xx || (EMBEDDED && PPC_ISERIES) || MPC7448HPC2
 	default y if !40x && !CPM2 && !8xx && !APUS && !PPC_83xx \
-		&& !PPC_85xx && !PPC_86xx
+		&& !PPC_85xx && !PPC_86xx && !PS3PF
 	default PCI_PERMEDIA if !4xx && !CPM2 && !8xx && APUS
 	default PCI_QSPAN if !4xx && !CPM2 && 8xx
 	help
arch/powerpc/platforms/Makefile +1 -0
Index: cell--common--6/arch/powerpc/platforms/Makefile
===================================================================
@@ -15,4 +15,5 @@
 obj-$(CONFIG_PPC_MAPLE)		+= maple/
 obj-$(CONFIG_PPC_PASEMI)		+= pasemi/
 obj-$(CONFIG_PPC_CELL)		+= cell/
+obj-$(CONFIG_PS3PF)		+= ps3pf/
 obj-$(CONFIG_EMBEDDED6xx)	+= embedded6xx/
arch/powerpc/platforms/ps3pf/Kconfig new file +32 -0
Index: cell--common--6/arch/powerpc/platforms/ps3pf/Kconfig
===================================================================
@@ -0,0 +1,32 @@
+menu "PS3 Platform Options"
+	depends on PS3PF
+
+config PS3PF_HTAB_SIZE
+	depends on PS3PF
+	int "PS3 Platform pagetable size"
+	range 18 20
+	default 20
+	help
+	  This option is only for experts who may have the desire to fine
+	  tune the pagetable size on their system.  The value here is
+	  expressed as the log2 of the page table size.  Valid values are
+	  18, 19, and 20, corresponding to 256KB, 512KB and 1MB respectively.
+
+	  If unsure, choose the default (20) with the confidence that your
+	  system will have optimal runtime performance.
+
+config PS3PF_DYNAMIC_DMA
+	depends on PS3PF && EXPERIMENTAL
+	bool "PS3 Platform dynamic DMA page table management"
+	default n
+	help
+	  This option will enable kernel support to take advantage of the
+	  per device dynamic DMA page table management provided by the Cell
+	  processor's IO Controller.  This support incurs some runtime
+	  overhead and also slightly increases kernel memory usage.  The
+	  current implementation should be considered experimental.
+
+	  This support is mainly for Linux kernel development.  If unsure,
+	  say N.
+
+endmenu
arch/powerpc/platforms/ps3pf/Makefile new file +2 -0
Index: cell--common--6/arch/powerpc/platforms/ps3pf/Makefile
===================================================================
@@ -0,0 +1,2 @@
+obj-$(CONFIG_PS3PF) += setup.o mm.o smp.o time.o hvcall.o htab.o repository.o
+obj-$(CONFIG_PS3PF) += interrupt.o exports.o
arch/powerpc/platforms/ps3pf/mm.c new file +872 -0

872 lines not on this page · raw message

arch/powerpc/platforms/ps3pf/platform.h new file +59 -0

59 lines not on this page · raw message

arch/powerpc/platforms/ps3pf/setup.c new file +177 -0

177 lines not on this page · raw message

arch/powerpc/platforms/ps3pf/smp.c new file +164 -0

164 lines not on this page · raw message

arch/powerpc/platforms/ps3pf/time.c new file +105 -0

105 lines not on this page · raw message

include/asm-powerpc/ps3pf.h new file +240 -0

240 lines not on this page · raw message

#1 lore raw

Hi,

Just a couple of small comments, I haven't had time to get deeper into the patchset yet. :-)

Quoted 11 lines · On Fri, 10 Nov 2006 12:02:19 -0800 Geoff Levand <[email protected]>…

This adds the core platform support for the PS3 game console and other platforms using the PS3 Platform hypervisor.

---
 MAINTAINERS                             |    7 
 arch/powerpc/Kconfig                    |   11 
 arch/powerpc/platforms/Makefile         |    1 
 arch/powerpc/platforms/ps3pf/Kconfig    |   32 +

Why are you naming the platform "platform" as in "this is the ps3pf platform" which expands to "this is the ps3 platform platform"? ps3 is quite adequate as the platform name. :-)

arch/powerpc/platforms/ps3pf/Kconfig line 16

Quoted 13 lines
arch/powerpc/platforms/ps3pf/Kconfig new file +13 -0
+config PS3PF_HTAB_SIZE
+	depends on PS3PF
+	int "PS3 Platform pagetable size"
+	range 18 20
+	default 20
+	help
+	  This option is only for experts who may have the desire to fine
+	  tune the pagetable size on their system.  The value here is
+	  expressed as the log2 of the page table size.  Valid values are
+	  18, 19, and 20, corresponding to 256KB, 512KB and 1MB respectively.
+
+	  If unsure, choose the default (20) with the confidence that your
+	  system will have optimal runtime performance.

Wouldn't it be more understandable if you had a "small/medium/large" memory config options (that in itself sets the values) instead of having people pick numbers?

arch/powerpc/platforms/ps3pf/mm.c line 698

Quoted 7 lines
arch/powerpc/platforms/ps3pf/mm.c new file +7 -0
+	result = dma_region_create(r);
+
+	if (result) {
+		pr_debug("%s:%d: ps3pf_dma_region_create failed\n",
+			__func__, __LINE__);
+		BUG();
+	}

You have lots of these. please do BUG_ON(result) instead. :-)

-Olof

#2 lore raw
Quoted 19 lines · Olof Johansson

Hi,

Just a couple of small comments, I haven't had time to get deeper into the patchset yet. :-)

Quoted 11 lines · On Fri, 10 Nov 2006 12:02:19 -0800 Geoff Levand <[email protected]>…

This adds the core platform support for the PS3 game console and other platforms using the PS3 Platform hypervisor.

---
 MAINTAINERS                             |    7 
 arch/powerpc/Kconfig                    |   11 
 arch/powerpc/platforms/Makefile         |    1 
 arch/powerpc/platforms/ps3pf/Kconfig    |   32 +
Quoted 3 lines

Why are you naming the platform "platform" as in "this is the ps3pf platform" which expands to "this is the ps3 platform platform"? ps3 is quite adequate as the platform name. :-)

I had it as ps3, but I was told to change it to ps3pf. I don't like that name though.

-Geoff

#3 lore raw
Quoted 4 lines · Geoff Levand
Quoted 2 lines

platform" which expands to "this is the ps3 platform platform"? ps3 is quite adequate as the platform name. :-)

I had it as ps3, but I was told to change it to ps3pf.

Who told you to use ps3pf? I think we should shoot that person for beein stupid. It's definitly not an acceptable name. I think the most sensible one for now is ps3hv, unless the rumoured cell based workstation from sony can share the port, in which case sonyhv would make a lot more sense.

#4 lore raw
In reply to Geoff Levand
Quoted 2 lines · Geoff Levand

I had it as ps3, but I was told to change it to ps3pf. I don't like that name though.

The "ps3pf" name is pretty meaningless. I think that, at the very least, the directory name should be arch/powerpc/platforms/ps3. Unless Arnd is prepared to argue strongly to the contrary, I will be rejecting patches that create a platforms/ps3pf directory.

Regards,
Paul.

#5 lore raw
Quoted 4 lines · Paul Mackerras

The "ps3pf" name is pretty meaningless. =A0I think that, at the very least, the directory name should be arch/powerpc/platforms/ps3. Unless Arnd is prepared to argue strongly to the contrary, I will be rejecting patches that create a platforms/ps3pf directory.

I don't care about the name at all, either way. However, I think it would be good to be consistant with the directory name and the name of identifiers in the code, so the function names should also be changed if the platform is name 'ps3'.

	Arnd <><
#6 lore raw
Quoted 10 lines · Arnd Bergmann
Quoted 4 lines · Paul Mackerras

The "ps3pf" name is pretty meaningless. �I think that, at the very least, the directory name should be arch/powerpc/platforms/ps3. Unless Arnd is prepared to argue strongly to the contrary, I will be rejecting patches that create a platforms/ps3pf directory.

I don't care about the name at all, either way. However, I think it would be good to be consistant with the directory name and the name of identifiers in the code, so the function names should also be changed if the platform is name 'ps3'.

Indeed, and since we don't like changing all of these back and forth, it would be nice to have a definitive decision soon.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE) [email protected] ------- The Corporate Village, Da Vincilaan 7-D1 Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

#7 lore raw
Quoted 2 lines · Geert Uytterhoeven

Indeed, and since we don't like changing all of these back and forth, it would be nice to have a definitive decision soon.

Put the ps3 support in arch/powerpc/platforms/ps3. If there are hypervisor-specific routines, they can have a ps3pf_ prefix if you want.

Paul.

#8 lore raw
Quoted 8 lines · Paul Mackerras
Quoted 2 lines · Geert Uytterhoeven

Indeed, and since we don't like changing all of these back and forth, it would be nice to have a definitive decision soon.

Put the ps3 support in arch/powerpc/platforms/ps3. If there are hypervisor-specific routines, they can have a ps3pf_ prefix if you want.

The whole port is hypervisor-specific. It should be called ps3hv or sonyhv (in case the plan for more hardware that's crippled by this hypervisor) ps3 should be left for a bare metal port that people may or may not do when they have time at their hands to get rid of the HV that cripples the hardware.

#9 lore raw
Quoted 3 lines · Christoph Hellwig

The whole port is hypervisor-specific. =A0It should be called ps3hv or sonyhv (in case the plan for more hardware that's crippled by this hypervisor)=20

The hypervisor has a name, it's called 'lv1', and the port already uses that identifier for the low-level interfaces.
If the platform is named after the hypervisor, we should not make up a new name but use the one that is already there.

Quoted 1 line

ps3 should be left for a bare metal port that people may or may not do=20

If there will ever be a native port, that will not need its platform directory but can simply go into into common platforms/cell/ and sysdev directories, with a small file for the ppc_md instance. All the interesting files (page table, iommu, interrupt handling) would be shared with native ports to different hardware.

	Arnd <><
#10 lore raw
Quoted 9 lines · Arnd Bergmann
Quoted 3 lines · Christoph Hellwig

The whole port is hypervisor-specific. ?It should be called ps3hv or sonyhv (in case the plan for more hardware that's crippled by this hypervisor)

The hypervisor has a name, it's called 'lv1', and the port already uses that identifier for the low-level interfaces.
If the platform is named after the hypervisor, we should not make up a new name but use the one that is already there.

Actually that makes even more sense, yes.

Quoted 7 lines
Quoted 1 line

ps3 should be left for a bare metal port that people may or may not do

If there will ever be a native port, that will not need its platform directory but can simply go into into common platforms/cell/ and sysdev directories, with a small file for the ppc_md instance. All the interesting files (page table, iommu, interrupt handling) would be shared with native ports to different hardware.

I guess so, yes. We'd still add a CONFIG_PS3 for it to not bloat kernels that don't want support.

#11 lore raw
Quoted 15 lines · Christoph Hellwig
Quoted 9 lines · Arnd Bergmann
Quoted 3 lines · Christoph Hellwig

The whole port is hypervisor-specific. ?It should be called ps3hv or sonyhv (in case the plan for more hardware that's crippled by this hypervisor)

The hypervisor has a name, it's called 'lv1', and the port already uses that identifier for the low-level interfaces.
If the platform is named after the hypervisor, we should not make up a new name but use the one that is already there.

Actually that makes even more sense, yes.

Quoted 2 lines
Quoted 1 line

ps3 should be left for a bare metal port that people may or may not do

Quoted 8 lines
Quoted 5 lines

If there will ever be a native port, that will not need its platform directory but can simply go into into common platforms/cell/ and sysdev directories, with a small file for the ppc_md instance. All the interesting files (page table, iommu, interrupt handling) would be shared with native ports to different hardware.

I guess so, yes. We'd still add a CONFIG_PS3 for it to not bloat kernels that don't want support.

CONFIG_PS3 is most natural to represent the support in 'platforms/ps3', so I will use that symbol.

-Geoff

#12 lore raw
In reply to Arnd Bergmann
Quoted 2 lines · Arnd Bergmann
Quoted 1 line · Christoph Hellwig

The whole port is hypervisor-specific. =EF=BF=BDIt should be called ps=

3hv or

Quoted 7 lines
Quoted 2 lines

sonyhv (in case the plan for more hardware that's crippled by this hypervisor)=20

=20
The hypervisor has a name, it's called 'lv1', and the port already uses that identifier for the low-level interfaces.
If the platform is named after the hypervisor, we should not make up a new name but use the one that is already there.

Actually, the HV name is 'Cell OS LV1'. 'platforms/cell_os_lv1', or even 'platforms/lv1' -- I consider those perhaps even more 'meaningless' than ps3pf.

-Geoff

#13 lore raw
In reply to Christoph Hellwig
Quoted 10 lines · Christoph Hellwig
Quoted 8 lines · Paul Mackerras
Quoted 2 lines · Geert Uytterhoeven

Indeed, and since we don't like changing all of these back and forth, it would be nice to have a definitive decision soon.

Put the ps3 support in arch/powerpc/platforms/ps3. If there are hypervisor-specific routines, they can have a ps3pf_ prefix if you want.

Quoted 5 lines

The whole port is hypervisor-specific. It should be called ps3hv or sonyhv (in case the plan for more hardware that's crippled by this hypervisor) ps3 should be left for a bare metal port that people may or may not do when they have time at their hands to get rid of the HV that cripples the hardware.

I don't see an iserieshv, or pserieshv, or an ibmhv, or a powermacof. I don't think this platform should be treated any differently.

As Paul and others requested, I will use 'platforms/ps3'. I think that is most natural and is what would be expected by almost all users. I don't think anyone can convincingly argue that ps3 is not an acceptable name for this platform.

I will change all ps3pf references in the source to ps3, but will retain the current HV (lv1) call interface. I'll post a new set of patches with these changes, plus a few more to better support multi-platform (IBM Blade + PS3) soon.

-Geoff

#14 lore raw
In reply to Olof Johansson
Quoted 9 lines · Olof Johansson
Quoted 9 lines
arch/powerpc/platforms/ps3pf/Kconfig new file +9 -0
+config PS3PF_HTAB_SIZE
+	depends on PS3PF
+	int "PS3 Platform pagetable size"
+	range 18 20
+	default 20
+	help
+	  This option is only for experts who may have the desire to fine
+	  tune the pagetable size on their system.  The value here is
+	  expressed as the log2 of the page table size.  Valid values are
Quoted 8 lines

arch/powerpc/platforms/ps3pf/Kconfig line 16

Quoted 4 lines
arch/powerpc/platforms/ps3pf/Kconfig new file +4 -0
+	  18, 19, and 20, corresponding to 256KB, 512KB and 1MB respectively.
+
+	  If unsure, choose the default (20) with the confidence that your
+	  system will have optimal runtime performance.

Wouldn't it be more understandable if you had a "small/medium/large" memory config options (that in itself sets the values) instead of having people pick numbers?

In general you'll want the max, 1MB, which is actually small. This was intended for tuning embedded systems where it is expected the user understands the ramifications of pagetable size selection and the log2 values will be familiar.

The trouble I see with having the options you suggest is that if a future version of the HV supports different sizes, then how do you map those into your "small/medium/large" scheme? With numbers it falls out naturally.

-Geoff

#15 lore raw
In reply to Geoff Levand
Quoted 2 lines · Geoff Levand

This adds the core platform support for the PS3 game console and other platforms using the PS3 Platform hypervisor.

Please give the port a less confusing name. I'd expect platforms/ps3 to be the bare metal port once people have revere-engineered the hardware, so your crappy hipervisor code should make it very clear what it is. sonyhv or ps3hv would suite I think.

#16 lore raw
Quoted 8 lines · Christoph Hellwig
Quoted 2 lines · Geoff Levand

This adds the core platform support for the PS3 game console and other platforms using the PS3 Platform hypervisor.

Please give the port a less confusing name. I'd expect platforms/ps3 to be the bare metal port once people have revere-engineered the hardware, so your crappy hipervisor code should make it very clear what it is. sonyhv or ps3hv would suite I think.

Any while we're at it it probably shouldn't be it's own directory under platforms, as platforms/cell contains a lot of generic cell stuff. We should rather have platforms/cell for all cell stuff and then sub-platform prefixed bits. Especially as e.g. the cpbw and native cell port will probably share a lot of code.

#17 lore raw
In reply to Geoff Levand
Quoted 2 lines · Geoff Levand
(quoted diff) +0 -0
@@ -876,7 +885,7 @@

=A0=A0=A0=A0=A0=A0=A0=A0bool "PCI support" if 40x || CPM2 || PPC_83xx || =

PPC_85xx || PPC_86xx \

Quoted 1 line

=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0|| PPC_MPC52xx || (EMBEDD=

ED && PPC_ISERIES) || MPC7448HPC2

Quoted 1 line

=A0=A0=A0=A0=A0=A0=A0=A0default y if !40x && !CPM2 && !8xx && !APUS && !P=

PC_83xx \

Quoted 2 lines

-=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0&& !PPC_85xx && !PPC_86xx +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0&& !PPC_85xx && !PPC_86xx &=

& !PS3PF

Quoted 1 line

=A0=A0=A0=A0=A0=A0=A0=A0default PCI_PERMEDIA if !4xx && !CPM2 && !8xx && =

APUS

Quoted 2 lines

=A0=A0=A0=A0=A0=A0=A0=A0default PCI_QSPAN if !4xx && !CPM2 && 8xx =A0=A0=A0=A0=A0=A0=A0=A0help

This change causes trouble when trying to build a multiplatform kernel that includes PS3PF and others at the same time, because PCI gets disabled unconditionally when PS3PF is selected. As a short-term fix, I'd change this to do

        bool "PCI support" if 40x || CPM2 || PPC_83xx || PPC_85xx || PPC_86=
xx \
=2D               || PPC_MPC52xx || (EMBEDDED && PPC_ISERIES) || MPC7448HPC2
+               || PPC_MPC52xx || (EMBEDDED && PPC_ISERIES) || MPC7448HPC2 \
+               || PS3PF
        default y if !40x && !CPM2 && !8xx && !APUS && !PPC_83xx \
               && !PPC_85xx && !PPC_86xx

Which makes PCI support optional when PS3PF is selected. However, this still means that you can choose broken configurations (e.g. PSERIES=3Dy, PS3PF=3D= y,
PCI=3Dn).

The real fix should be to get rid of the long conditional for CONFIG_PCI, and have PCI selected by the platforms themselves:

config PPC_PSERIES

	select PCI

	Arnd <><
#18 lore raw

QXJuZCBCZXJnbWFubiB3cm90ZToNCj4gT24gRnJpZGF5IDEwIE5vdmVtYmVyIDIwMDYgMjE6MDIs IEdlb2ZmIExldmFuZCB3cm90ZToNCj4+IEBAIC04NzYsNyArODg1LDcgQEANCj4+IO+/ve+/ve+/ ve+/ve+/ve+/ve+/ve+/vWJvb2wgIlBDSSBzdXBwb3J0IiBpZiA0MHggfHwgQ1BNMiB8fCBQUENf ODN4eCB8fCBQUENfODV4eCB8fCBQUENfODZ4eCBcDQo+PiDvv73vv73vv73vv73vv73vv73vv73v v73vv73vv73vv73vv73vv73vv73vv73vv718fCBQUENfTVBDNTJ4eCB8fCAoRU1CRURERUQgJiYg UFBDX0lTRVJJRVMpIHx8IE1QQzc0NDhIUEMyDQo+PiDvv73vv73vv73vv73vv73vv73vv73vv71k ZWZhdWx0IHkgaWYgITQweCAmJiAhQ1BNMiAmJiAhOHh4ICYmICFBUFVTICYmICFQUENfODN4eCBc DQo+PiAt77+977+977+977+977+977+977+977+977+977+977+977+977+977+977+9JiYgIVBQ Q184NXh4ICYmICFQUENfODZ4eA0KPj4gK++/ve+/ve+/ve+/ve+/ve+/ve+/ve+/ve+/ve+/ve+/ ve+/ve+/ve+/ve+/vSYmICFQUENfODV4eCAmJiAhUFBDXzg2eHggJiYgIVBTM1BGDQo+PiDvv73v v73vv73vv73vv73vv73vv73vv71kZWZhdWx0IFBDSV9QRVJNRURJQSBpZiAhNHh4ICYmICFDUE0y ICYmICE4eHggJiYgQVBVUw0KPj4g77+977+977+977+977+977+977+977+9ZGVmYXVsdCBQQ0lf UVNQQU4gaWYgITR4eCAmJiAhQ1BNMiAmJiA4eHgNCj4+IO+/ve+/ve+/ve+/ve+/ve+/ve+/ve+/ vWhlbHANCj4gDQo+IFRoaXMgY2hhbmdlIGNhdXNlcyB0cm91YmxlIHdoZW4gdHJ5aW5nIHRvIGJ1 aWxkIGEgbXVsdGlwbGF0Zm9ybSBrZXJuZWwNCj4gdGhhdCBpbmNsdWRlcyBQUzNQRiBhbmQgb3Ro ZXJzIGF0IHRoZSBzYW1lIHRpbWUsIGJlY2F1c2UgUENJIGdldHMNCj4gZGlzYWJsZWQgdW5jb25k aXRpb25hbGx5IHdoZW4gUFMzUEYgaXMgc2VsZWN0ZWQuDQoNCg0KT0ssIEkgZGlkbid0IGRvIGFu eSBtdWx0aS1wbGF0Zm9ybSB0ZXN0aW5nIHlldCwgYnV0IEknbGwgc3RhcnQuDQoNCg0KQXMgYSBz aG9ydC10ZXJtDQo+IGZpeCwgSSdkIGNoYW5nZSB0aGlzIHRvIGRvDQo+IA0KPiAgICAgICAgIGJv b2wgIlBDSSBzdXBwb3J0IiBpZiA0MHggfHwgQ1BNMiB8fCBQUENfODN4eCB8fCBQUENfODV4eCB8 fCBQUENfODZ4eCBcDQo+IC0gICAgICAgICAgICAgICB8fCBQUENfTVBDNTJ4eCB8fCAoRU1CRURE RUQgJiYgUFBDX0lTRVJJRVMpIHx8IE1QQzc0NDhIUEMyDQo+ICsgICAgICAgICAgICAgICB8fCBQ UENfTVBDNTJ4eCB8fCAoRU1CRURERUQgJiYgUFBDX0lTRVJJRVMpIHx8IE1QQzc0NDhIUEMyIFwN Cj4gKyAgICAgICAgICAgICAgIHx8IFBTM1BGDQo+ICAgICAgICAgZGVmYXVsdCB5IGlmICE0MHgg JiYgIUNQTTIgJiYgITh4eCAmJiAhQVBVUyAmJiAhUFBDXzgzeHggXA0KPiAgICAgICAgICAgICAg ICAmJiAhUFBDXzg1eHggJiYgIVBQQ184Nnh4DQo+IA0KPiBXaGljaCBtYWtlcyBQQ0kgc3VwcG9y dCBvcHRpb25hbCB3aGVuIFBTM1BGIGlzIHNlbGVjdGVkLiBIb3dldmVyLCB0aGlzIHN0aWxsDQo+ IG1lYW5zIHRoYXQgeW91IGNhbiBjaG9vc2UgYnJva2VuIGNvbmZpZ3VyYXRpb25zIChlLmcuIFBT RVJJRVM9eSwgUFMzUEY9eSwNCj4gUENJPW4pLg0KDQoNCk9LLCBJJ2xsIHRyeSBpdC4NCg0KDQo+ IFRoZSByZWFsIGZpeCBzaG91bGQgYmUgdG8gZ2V0IHJpZCBvZiB0aGUgbG9uZyBjb25kaXRpb25h bCBmb3IgQ09ORklHX1BDSSwNCj4gYW5kIGhhdmUgUENJIHNlbGVjdGVkIGJ5IHRoZSBwbGF0Zm9y bXMgdGhlbXNlbHZlczoNCj4gDQo+IGNvbmZpZyBQUENfUFNFUklFUw0KPiAJc2VsZWN0IFBDSQ0K DQoNClllcywgd2UgY2FuIHdvcmsgdG93YXJkcyB0aGF0Lg0KDQotR2VvZmYNCg==

#19 lore raw
In reply to Arnd Bergmann

QXJuZCBCZXJnbWFubiB3cm90ZToNCj4gT24gRnJpZGF5IDEwIE5vdmVtYmVyIDIwMDYgMjE6MDIs IEdlb2ZmIExldmFuZCB3cm90ZToNCj4+IEBAIC04NzYsNyArODg1LDcgQEANCj4+IO+/ve+/ve+/ ve+/ve+/ve+/ve+/ve+/vWJvb2wgIlBDSSBzdXBwb3J0IiBpZiA0MHggfHwgQ1BNMiB8fCBQUENf ODN4eCB8fCBQUENfODV4eCB8fCBQUENfODZ4eCBcDQo+PiDvv73vv73vv73vv73vv73vv73vv73v v73vv73vv73vv73vv73vv73vv73vv73vv718fCBQUENfTVBDNTJ4eCB8fCAoRU1CRURERUQgJiYg UFBDX0lTRVJJRVMpIHx8IE1QQzc0NDhIUEMyDQo+PiDvv73vv73vv73vv73vv73vv73vv73vv71k ZWZhdWx0IHkgaWYgITQweCAmJiAhQ1BNMiAmJiAhOHh4ICYmICFBUFVTICYmICFQUENfODN4eCBc DQo+PiAt77+977+977+977+977+977+977+977+977+977+977+977+977+977+977+9JiYgIVBQ Q184NXh4ICYmICFQUENfODZ4eA0KPj4gK++/ve+/ve+/ve+/ve+/ve+/ve+/ve+/ve+/ve+/ve+/ ve+/ve+/ve+/ve+/vSYmICFQUENfODV4eCAmJiAhUFBDXzg2eHggJiYgIVBTM1BGDQo+PiDvv73v v73vv73vv73vv73vv73vv73vv71kZWZhdWx0IFBDSV9QRVJNRURJQSBpZiAhNHh4ICYmICFDUE0y ICYmICE4eHggJiYgQVBVUw0KPj4g77+977+977+977+977+977+977+977+9ZGVmYXVsdCBQQ0lf UVNQQU4gaWYgITR4eCAmJiAhQ1BNMiAmJiA4eHgNCj4+IO+/ve+/ve+/ve+/ve+/ve+/ve+/ve+/ vWhlbHANCj4gDQo+IFRoaXMgY2hhbmdlIGNhdXNlcyB0cm91YmxlIHdoZW4gdHJ5aW5nIHRvIGJ1 aWxkIGEgbXVsdGlwbGF0Zm9ybSBrZXJuZWwNCj4gdGhhdCBpbmNsdWRlcyBQUzNQRiBhbmQgb3Ro ZXJzIGF0IHRoZSBzYW1lIHRpbWUsIGJlY2F1c2UgUENJIGdldHMNCj4gZGlzYWJsZWQgdW5jb25k aXRpb25hbGx5IHdoZW4gUFMzUEYgaXMgc2VsZWN0ZWQuIEFzIGEgc2hvcnQtdGVybQ0KPiBmaXgs IEknZCBjaGFuZ2UgdGhpcyB0byBkbw0KPiANCj4gICAgICAgICBib29sICJQQ0kgc3VwcG9ydCIg aWYgNDB4IHx8IENQTTIgfHwgUFBDXzgzeHggfHwgUFBDXzg1eHggfHwgUFBDXzg2eHggXA0KPiAt ICAgICAgICAgICAgICAgfHwgUFBDX01QQzUyeHggfHwgKEVNQkVEREVEICYmIFBQQ19JU0VSSUVT KSB8fCBNUEM3NDQ4SFBDMg0KPiArICAgICAgICAgICAgICAgfHwgUFBDX01QQzUyeHggfHwgKEVN QkVEREVEICYmIFBQQ19JU0VSSUVTKSB8fCBNUEM3NDQ4SFBDMiBcDQo+ICsgICAgICAgICAgICAg ICB8fCBQUzNQRg0KPiAgICAgICAgIGRlZmF1bHQgeSBpZiAhNDB4ICYmICFDUE0yICYmICE4eHgg JiYgIUFQVVMgJiYgIVBQQ184M3h4IFwNCj4gICAgICAgICAgICAgICAgJiYgIVBQQ184NXh4ICYm ICFQUENfODZ4eA0KPiANCj4gV2hpY2ggbWFrZXMgUENJIHN1cHBvcnQgb3B0aW9uYWwgd2hlbiBQ UzNQRiBpcyBzZWxlY3RlZC4NCg0KDQpUaGlzIHdvcmtzIE9LLiAgWW91IGFyZSBub3cgZ2l2ZW4g dGhlIG9wdGlvbiB0byBlbmFibGUgUENJIHdpdGggUFMzUEYuDQoNCg0KPiBIb3dldmVyLCB0aGlz IHN0aWxsDQo+IG1lYW5zIHRoYXQgeW91IGNhbiBjaG9vc2UgYnJva2VuIGNvbmZpZ3VyYXRpb25z IChlLmcuIFBTRVJJRVM9eSwgUFMzUEY9eSwNCj4gUENJPW4pLg0KPiANCj4gVGhlIHJlYWwgZml4 IHNob3VsZCBiZSB0byBnZXQgcmlkIG9mIHRoZSBsb25nIGNvbmRpdGlvbmFsIGZvciBDT05GSUdf UENJLA0KPiBhbmQgaGF2ZSBQQ0kgc2VsZWN0ZWQgYnkgdGhlIHBsYXRmb3JtcyB0aGVtc2VsdmVz Og0KPiANCj4gY29uZmlnIFBQQ19QU0VSSUVTDQo+IAlzZWxlY3QgUENJDQoNCg0KWWVzLCB0aGF0 IHNlZW1zIHRoZSB3YXkgdG8gZ28uDQoNCi1HZW9mZg0K

#20 lore raw
In reply to Geoff Levand

arch/powerpc/platforms/ps3pf/mm.c line 2

Quoted 7 lines

Index: cell--common--6/arch/powerpc/platforms/ps3pf/mm.c
===================================================================

arch/powerpc/platforms/ps3pf/mm.c new file +2 -0
@@ -0,0 +1,872 @@
+/*
+ * mm.c - PS3 Platform address space management.

Please never put the filename in the start of file comments, it gets out of date far to easily.

arch/powerpc/platforms/ps3pf/mm.c line 5

Quoted 3 lines
arch/powerpc/platforms/ps3pf/mm.c new file +3 -0
+ *
+ *  Copyright (C) 2006 Sony Computer Entertainment Inc.
+ *  Copyright 2006 Sony Corp.

One with a (C) and one without looks very odd. Who from your corporate mother Sony Corp touched this code anyway?

arch/powerpc/platforms/ps3pf/mm.c line 6

Quoted 1 line
arch/powerpc/platforms/ps3pf/mm.c new file +1 -0
+#undef DEBUG

No need to put this in, it's undefined by default. And having it prevents people from enabling debugging through the compiler flags.

arch/powerpc/platforms/ps3pf/mm.c line 37

Quoted 4 lines
arch/powerpc/platforms/ps3pf/mm.c new file +4 -0
+#if defined(DEBUG)
+#undef pr_debug
+#define pr_debug(fmt...) udbg_printf(fmt)
+#endif

Please don't do this kind of thing. Either you want pr_debug then you use it as-is, or you want something different and you should call it different.

arch/powerpc/platforms/ps3pf/mm.c line 50

Quoted 13 lines
arch/powerpc/platforms/ps3pf/mm.c new file +13 -0
+
+enum {
+#if defined(CONFIG_PS3PF_USE_LPAR_ADDR)
+	USE_LPAR_ADDR = 1,
+#else
+	USE_LPAR_ADDR = 0,
+#endif
+#if defined(CONFIG_PS3PF_DYNAMIC_DMA)
+	USE_DYNAMIC_DMA = 1,
+#else
+	USE_DYNAMIC_DMA = 0,
+#endif
+};

This is really ugly. At least dynamic_dma (or as we'd say it iommu) shoud be a runtime feature. So this should just be an

"int ps3hv_use_iommu"

variable initialize to the proper default.

arch/powerpc/platforms/ps3pf/mm.c line 56

Quoted 6 lines
arch/powerpc/platforms/ps3pf/mm.c new file +6 -0
+
+enum page_size {
+	page_size_4k = 12U,
+	page_size_64k = 16U,
+	page_size_16m = 24U,
+};

Please use ALL_CAPS for such constants.

Quoted 5 lines
arch/powerpc/platforms/ps3pf/mm.c new file +5 -0
+
+enum allocate_memory {
+	/* bit 63: transferability */
+	LV1_AM_TF_NO = 0x00,
+	LV1_AM_TF_YES = 0x01,

arch/powerpc/platforms/ps3pf/mm.c line 71

Quoted 10 lines
arch/powerpc/platforms/ps3pf/mm.c new file +10 -0
+	/* bit 62: destruction scheme */
+	LV1_AM_DS_NO_CONNECTIONS = 0x00,
+	LV1_AM_DS_ANYTIME = 0x02,
+	/* bit 61: fail or alternative */
+	LV1_AM_FA_FAIL = 0x00,
+	LV1_AM_FA_ALTERNATIVE = 0x04,
+	/* bit 60: lpar address */
+	LV1_AM_ADDR_ANY = 0x00,
+	LV1_AM_ADDR_0 = 0x08,
+};

If these are for different its they should be in different enums and use different prefixes.

arch/powerpc/platforms/ps3pf/mm.c line 100

Quoted 12 lines
arch/powerpc/platforms/ps3pf/mm.c new file +12 -0
+/**
+ * struct mem_region - memory region structure
+ * @base: base address
+ * @size: size in bytes
+ * @offset: difference between base and rm.size
+ */
+
+struct mem_region {
+	unsigned long base;
+	unsigned long size;
+	unsigned long offset;
+};

This seems like a duplication of the iseries lmb, please try to reuse that functionality.

Quoted 5 lines
arch/powerpc/platforms/ps3pf/mm.c new file +5 -0
+ * @len: Length in bytes of the area to map.
+ * c_out: A pointer to receive an allocated struct dma_chunk for this area.
+ *
+ * This is the lowest level dma mapping routine, and is the one that will
+ * make the HV call to add the pages into the io controller address space.

arch/powerpc/platforms/ps3pf/mm.c line 467

Quoted 10 lines
arch/powerpc/platforms/ps3pf/mm.c new file +10 -0
+ */
+
+static int dma_map_pages(struct ps3pf_dma_region *r, unsigned long virt_addr,
+	unsigned long len, struct dma_chunk **c_out)
+{
+	int result;
+	unsigned long phys_addr;
+	struct dma_chunk *c;
+
+	phys_addr = is_kernel_addr(virt_addr) ? __pa(virt_addr) : virt_addr;

When do you ever get non-kernel addresses into the dma calls? I in either case this should move into the caller and the routine should get a simple unsigned long phys_addr argument.

But when looking at this whole dma_map code is looks rather bogus to me. Could it be that you try to pre-map dma regions rather than doing it through the dma API?

arch/powerpc/platforms/ps3pf/mm.c line 73

Quoted 2 lines
arch/powerpc/platforms/ps3pf/mm.c new file +2 -0
+#if !defined(_510B7842_EE09_4B12_BE5C_D217383D50C7)
+#define _510B7842_EE09_4B12_BE5C_D217383D50C7

WTF?

arch/powerpc/platforms/ps3pf/mm.c line 83

Quoted 10 lines
arch/powerpc/platforms/ps3pf/mm.c new file +10 -0
+#if defined(CONFIG_BLK_DEV_INITRD)
+ 	if (initrd_start)
+ 		ROOT_DEV = Root_RAM0;
+ 	else
+#endif
+#if defined(CONFIG_ROOT_NFS)
+		ROOT_DEV = Root_NFS;
+#else
+ 		ROOT_DEV = Root_HDA1;
+#endif

Please don't put this auto-select root device idiocy into any new ports (Yeah, I'm pretty sure you just copy & pasted it from somewhere..)

arch/powerpc/platforms/ps3pf/mm.c line 86

Quoted 3 lines
arch/powerpc/platforms/ps3pf/mm.c new file +3 -0
+#if !defined(PPC_MSG_MIGRATE_TASK)
+static const int PPC_MSG_MIGRATE_TASK = 2;
+#endif

This looks rather odd, what is it trying to do?

arch/powerpc/platforms/ps3pf/mm.c line 87

Quoted 1 line
arch/powerpc/platforms/ps3pf/mm.c new file +1 -0
+unsigned long __deprecated ps3pf_legacy_virq_to_outlet(unsigned int virq);

Never put deprecated routines into a new code submission.

#21 lore raw
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines

+ * Copyright (C) 2006 Sony Computer Entertainment Inc.
+ * Copyright 2006 Sony Corp.

One with a (C) and one without looks very odd. Who from your corporate mother Sony Corp touched this code anyway?

Sony Corp. and Sony Computer Entertainment are different companies with different policies. I have no choice here.

Quoted 16 lines

arch/powerpc/platforms/ps3pf/mm.c line 50

Quoted 12 lines
arch/powerpc/platforms/ps3pf/mm.c new file +12 -0
+enum {
+#if defined(CONFIG_PS3PF_USE_LPAR_ADDR)
+	USE_LPAR_ADDR = 1,
+#else
+	USE_LPAR_ADDR = 0,
+#endif
+#if defined(CONFIG_PS3PF_DYNAMIC_DMA)
+	USE_DYNAMIC_DMA = 1,
+#else
+	USE_DYNAMIC_DMA = 0,
+#endif
+};

This is really ugly. At least dynamic_dma (or as we'd say it iommu) shoud be a runtime feature. So this should just be an

Quoted 3 lines

"int ps3hv_use_iommu"

variable initialize to the proper default.

I set it up that way so that the optimizer will remove the unused parts. This machine doesn't have much memory, and the dynamic (that's not my name BTW, came from your side) will normally not be used. I don't really want to set it up as a runtime option unless there is a real user need.

Quoted 9 lines
Quoted 9 lines
arch/powerpc/platforms/ps3pf/mm.c new file +9 -0
+enum allocate_memory {
+	/* bit 63: transferability */
+	LV1_AM_TF_NO = 0x00,
+	LV1_AM_TF_YES = 0x01,
+	/* bit 62: destruction scheme */
+	LV1_AM_DS_NO_CONNECTIONS = 0x00,
+	LV1_AM_DS_ANYTIME = 0x02,
+	/* bit 61: fail or alternative */
+	LV1_AM_FA_FAIL = 0x00,
Quoted 8 lines

arch/powerpc/platforms/ps3pf/mm.c line 71

Quoted 5 lines
arch/powerpc/platforms/ps3pf/mm.c new file +5 -0
+	LV1_AM_FA_ALTERNATIVE = 0x04,
+	/* bit 60: lpar address */
+	LV1_AM_ADDR_ANY = 0x00,
+	LV1_AM_ADDR_0 = 0x08,
+};

If these are for different its they should be in different enums and use different prefixes.

These are all flags for lv1_allocate_memory (hence the the LV1_AM_ prefix). The bits are documented there in the comments, bit 60, 61, etc.

Quoted 7 lines
Quoted 7 lines
arch/powerpc/platforms/ps3pf/mm.c new file +7 -0
+/**
+ * struct mem_region - memory region structure
+ * @base: base address
+ * @size: size in bytes
+ * @offset: difference between base and rm.size
+ */
+
Quoted 8 lines

arch/powerpc/platforms/ps3pf/mm.c line 100

Quoted 5 lines
arch/powerpc/platforms/ps3pf/mm.c new file +5 -0
+struct mem_region {
+	unsigned long base;
+	unsigned long size;
+	unsigned long offset;
+};

This seems like a duplication of the iseries lmb, please try to reuse that functionality.

There is discussion on this ML to do that, or make some more generic support that could be used.

Quoted 13 lines

arch/powerpc/platforms/ps3pf/mm.c line 467

Quoted 8 lines
arch/powerpc/platforms/ps3pf/mm.c new file +8 -0
+static int dma_map_pages(struct ps3pf_dma_region *r, unsigned long virt_addr,
+	unsigned long len, struct dma_chunk **c_out)
+{
+	int result;
+	unsigned long phys_addr;
+	struct dma_chunk *c;
+
+	phys_addr = is_kernel_addr(virt_addr) ? __pa(virt_addr) : virt_addr;

When do you ever get non-kernel addresses into the dma calls? I in either case this should move into the caller and the routine should get a simple unsigned long phys_addr argument.

Quoted 3 lines

But when looking at this whole dma_map code is looks rather bogus to me. Could it be that you try to pre-map dma regions rather than doing it through the dma API?

Yes, I hope to hook it into the existing iommu support now that Ben has made some changes to make it more generic.

Quoted 4 lines

arch/powerpc/platforms/ps3pf/platform.h line 22

Quoted 2 lines
arch/powerpc/platforms/ps3pf/platform.h new file +2 -0
+#if !defined(_510B7842_EE09_4B12_BE5C_D217383D50C7)
+#define _510B7842_EE09_4B12_BE5C_D217383D50C7

WTF?

Considering your criticism earlier in this mail regarding the use of a file name in the file's comments, I'm wondering what your preference is here, since the convention is to use a symbol based on the file name...

BTW, I'm surprised you don't recognize it:
uuidgen | tr 'a-z-' 'A-Z_' | sed -e 's/^/_/'

Quoted 5 lines
Quoted 5 lines
arch/powerpc/platforms/ps3pf/setup.c new file +5 -0
+#if defined(CONFIG_BLK_DEV_INITRD)
+ 	if (initrd_start)
+ 		ROOT_DEV = Root_RAM0;
+ 	else
+#endif
Quoted 8 lines

arch/powerpc/platforms/ps3pf/setup.c line 90

Quoted 5 lines
arch/powerpc/platforms/ps3pf/setup.c new file +5 -0
+#if defined(CONFIG_ROOT_NFS)
+		ROOT_DEV = Root_NFS;
+#else
+ 		ROOT_DEV = Root_HDA1;
+#endif

Please don't put this auto-select root device idiocy into any new ports (Yeah, I'm pretty sure you just copy & pasted it from somewhere..)

I'm glad you gave me this comment, since I only put that in because I thought it was expected.

Quoted 5 lines

arch/powerpc/platforms/ps3pf/smp.c line 39

Quoted 3 lines
arch/powerpc/platforms/ps3pf/smp.c new file +3 -0
+#if !defined(PPC_MSG_MIGRATE_TASK)
+static const int PPC_MSG_MIGRATE_TASK = 2;
+#endif

This looks rather odd, what is it trying to do?

For some reason the definition of PPC_MSG_MIGRATE_TASK is sometimes 2, and sometimes not defined, depending on other config options. It makes the code much simpler if it is always defined as 2. Maybe it makes more sense if I change it to this:

#if !defined(PPC_MSG_MIGRATE_TASK)
#define PPC_MSG_MIGRATE_TASK 2
#endif

Quoted 3 lines
Quoted 1 line

+unsigned long __deprecated ps3pf_legacy_virq_to_outlet(unsigned int virq);

Never put deprecated routines into a new code submission.

Good point! Sorry, that slipped in from another patch when merging.

-Geoff

#22 lore raw
Quoted 9 lines · Geoff Levand
Quoted 5 lines · Christoph Hellwig

arch/powerpc/platforms/ps3pf/smp.c line 39

Quoted 3 lines
arch/powerpc/platforms/ps3pf/smp.c new file +3 -0
+#if !defined(PPC_MSG_MIGRATE_TASK)
+static const int PPC_MSG_MIGRATE_TASK = 2;
+#endif

This looks rather odd, what is it trying to do?

Quoted 8 lines

For some reason the definition of PPC_MSG_MIGRATE_TASK is sometimes 2, and sometimes not defined, depending on other config options. It makes the code much simpler if it is always defined as 2. Maybe it makes more sense if I change it to this:

#if !defined(PPC_MSG_MIGRATE_TASK)
#define PPC_MSG_MIGRATE_TASK 2
#endif

include/asm-powerpc/smp.h says:

/* This is unused now */
#if 0
#define PPC_MSG_MIGRATE_TASK 2
#endif

So it will never be defined. Why do you need it, you don't seem to use it anywhere?

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
#23 lore raw
Quoted 21 lines · Michael Ellerman
Quoted 9 lines · Geoff Levand
Quoted 5 lines · Christoph Hellwig

arch/powerpc/platforms/ps3pf/smp.c line 39

Quoted 3 lines
arch/powerpc/platforms/ps3pf/smp.c new file +3 -0
+#if !defined(PPC_MSG_MIGRATE_TASK)
+static const int PPC_MSG_MIGRATE_TASK = 2;
+#endif

This looks rather odd, what is it trying to do?

Quoted 8 lines

For some reason the definition of PPC_MSG_MIGRATE_TASK is sometimes 2, and sometimes not defined, depending on other config options. It makes the code much simpler if it is always defined as 2. Maybe it makes more sense if I change it to this:

#if !defined(PPC_MSG_MIGRATE_TASK)
#define PPC_MSG_MIGRATE_TASK 2
#endif

include/asm-powerpc/smp.h says:

Quoted 8 lines

/* This is unused now */
#if 0
#define PPC_MSG_MIGRATE_TASK 2
#endif

So it will never be defined. Why do you need it, you don't seem to use it anywhere?

Yes, you are correct. I changed the implementation, but didn't update these bits. I'll get rid of that code.

-Geoff

#24 lore raw
In reply to Geoff Levand
Quoted 10 lines · Geoff Levand
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines

+ * Copyright (C) 2006 Sony Computer Entertainment Inc.
+ * Copyright 2006 Sony Corp.

One with a (C) and one without looks very odd. Who from your corporate mother Sony Corp touched this code anyway?

Sony Corp. and Sony Computer Entertainment are different companies with different policies. I have no choice here.

So did anyone from Sony Corp sactually touch all these files?

Quoted 4 lines

I set it up that way so that the optimizer will remove the unused parts. This machine doesn't have much memory, and the dynamic (that's not my name BTW, came from your side) will normally not be used. I don't really want to set it up as a runtime option unless there is a real user need.

The optimizer also optimizes away variables that can't be anything but
0. I'm also not sure what "my side" (whatever that may be) has anything to do with that.

Quoted 21 lines
Quoted 11 lines
Quoted 11 lines
arch/powerpc/platforms/ps3pf/mm.c new file +9 -0
+enum allocate_memory {
+	/* bit 63: transferability */
+	LV1_AM_TF_NO = 0x00,
+	LV1_AM_TF_YES = 0x01,
+	/* bit 62: destruction scheme */
+	LV1_AM_DS_NO_CONNECTIONS = 0x00,
+	LV1_AM_DS_ANYTIME = 0x02,
+	/* bit 61: fail or alternative */
+	LV1_AM_FA_FAIL = 0x00,
Quoted 8 lines

arch/powerpc/platforms/ps3pf/mm.c line 71

Quoted 5 lines
arch/powerpc/platforms/ps3pf/mm.c new file +5 -0
+	LV1_AM_FA_ALTERNATIVE = 0x04,
+	/* bit 60: lpar address */
+	LV1_AM_ADDR_ANY = 0x00,
+	LV1_AM_ADDR_0 = 0x08,
+};

If these are for different its they should be in different enums and use different prefixes.

Quoted 2 lines

These are all flags for lv1_allocate_memory (hence the the LV1_AM_ prefix). The bits are documented there in the comments, bit 60, 61, etc.

But this is just prone for users to get things wrong. When flags with the same prefix need to go to different locations something is badly wrong.

Quoted 6 lines
Quoted 4 lines

arch/powerpc/platforms/ps3pf/platform.h line 22

Quoted 2 lines
arch/powerpc/platforms/ps3pf/platform.h new file +2 -0
+#if !defined(_510B7842_EE09_4B12_BE5C_D217383D50C7)
+#define _510B7842_EE09_4B12_BE5C_D217383D50C7

WTF?

Quoted 7 lines

Considering your criticism earlier in this mail regarding the use of a file name in the file's comments, I'm wondering what your preference is here, since the convention is to use a symbol based on the file name...

BTW, I'm surprised you don't recognize it:
uuidgen | tr 'a-z-' 'A-Z_' | sed -e 's/^/_/'

There's a good reason we don't use this elsewhere, and if you think it's a good idea propose it for general use on lkml (and get the deserved flames) instead of trying to sneak it in silently. Re the filename comment:
this was mostly a thing for implementation (.c) files that get renamed a lot so it easily gets stale and has no real value. Headers by their nature of beein used from various places tend to stay more stable so this problem is not that bad. And unlike top of file comments inclusion guards actually serve a purpose so we'll have to live with the additional work of renaming them in case.

#25 lore raw
Quoted 1 line · Christoph Hellwig

So did anyone from Sony Corp sactually touch all these files

Geoff works for Sony Corp :-)

#26 lore raw
In reply to Christoph Hellwig
Quoted 9 lines · Christoph Hellwig
Quoted 4 lines · Geoff Levand

I set it up that way so that the optimizer will remove the unused parts. This machine doesn't have much memory, and the dynamic (that's not my name BTW, came from your side) will normally not be used. I don't really want to set it up as a runtime option unless there is a real user need.

The optimizer also optimizes away variables that can't be anything but
0. I'm also not sure what "my side" (whatever that may be) has anything to do with that.

Yes, and that is how I tried to set it up. Actually, I never verified gcc is clever enough though. So, with USE_DYNAMIC_DMA = 1, dma_region_create_linear() is never called, and I assumed dma_region_create_linear() would be removed. Here's the construction:

enum {
#if defined(CONFIG_PS3PF_DYNAMIC_DMA)
USE_DYNAMIC_DMA = 1,
#else
USE_DYNAMIC_DMA = 0,
#endif
};

static int dma_region_create_linear(struct ps3pf_dma_region *r) {
...
}

int ps3pf_dma_region_create(struct ps3pf_dma_region *r)

{
	return (USE_DYNAMIC_DMA)
		? dma_region_create(r)
		: dma_region_create_linear(r);
}
Quoted 19 lines
Quoted 19 lines
Quoted 17 lines
Quoted 4 lines
arch/powerpc/platforms/ps3pf/mm.c new file +4 -0
+enum allocate_memory {
+	/* bit 63: transferability */
+	LV1_AM_TF_NO = 0x00,
+	LV1_AM_TF_YES = 0x01,

arch/powerpc/platforms/ps3pf/mm.c line 71

Quoted 10 lines
arch/powerpc/platforms/ps3pf/mm.c new file +10 -0
+	/* bit 62: destruction scheme */
+	LV1_AM_DS_NO_CONNECTIONS = 0x00,
+	LV1_AM_DS_ANYTIME = 0x02,
+	/* bit 61: fail or alternative */
+	LV1_AM_FA_FAIL = 0x00,
+	LV1_AM_FA_ALTERNATIVE = 0x04,
+	/* bit 60: lpar address */
+	LV1_AM_ADDR_ANY = 0x00,
+	LV1_AM_ADDR_0 = 0x08,
+};

If these are for different its they should be in different enums and use different prefixes.

Quoted 6 lines
Quoted 2 lines

These are all flags for lv1_allocate_memory (hence the the LV1_AM_ prefix). The bits are documented there in the comments, bit 60, 61, etc.

But this is just prone for users to get things wrong. When flags with the same prefix need to go to different locations something is badly wrong.

Anyway, the HV docs were updated, and I found that many of these options are not supported on the game console, so this is now just:

+enum {
+ ALLOCATE_MEMORY_TRY_ALT_UNIT = 0X04,
+ ALLOCATE_MEMORY_ADDR_ZERO = 0X08,
+};

Quoted 14 lines
Quoted 6 lines
Quoted 4 lines

arch/powerpc/platforms/ps3pf/platform.h line 22

Quoted 2 lines
arch/powerpc/platforms/ps3pf/platform.h new file +2 -0
+#if !defined(_510B7842_EE09_4B12_BE5C_D217383D50C7)
+#define _510B7842_EE09_4B12_BE5C_D217383D50C7

WTF?

Quoted 7 lines

Considering your criticism earlier in this mail regarding the use of a file name in the file's comments, I'm wondering what your preference is here, since the convention is to use a symbol based on the file name...

BTW, I'm surprised you don't recognize it:
uuidgen | tr 'a-z-' 'A-Z_' | sed -e 's/^/_/'

Quoted 9 lines

There's a good reason we don't use this elsewhere, and if you think it's a good idea propose it for general use on lkml (and get the deserved flames) instead of trying to sneak it in silently. Re the filename comment:
this was mostly a thing for implementation (.c) files that get renamed a lot so it easily gets stale and has no real value. Headers by their nature of beein used from various places tend to stay more stable so this problem is not that bad. And unlike top of file comments inclusion guards actually serve a purpose so we'll have to live with the additional work of renaming them in case.

No, I am not on some quest with this, I just put that in early in the development when I didn't know what the final file name would be. I just forgot to change it, that's all.

-Geoff

#27 lore raw
In reply to Christoph Hellwig
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · Geoff Levand
Quoted 3 lines · Christoph Hellwig
Quoted 2 lines

+ * Copyright (C) 2006 Sony Computer Entertainment Inc.
+ * Copyright 2006 Sony Corp.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

One with a (C) and one without looks very odd. Who from your corporate mother Sony Corp touched this code anyway?

Sony Corp. and Sony Computer Entertainment are different companies with different policies. I have no choice here.

So did anyone from Sony Corp sactually touch all these files?

Your question makes no sense to me in the context of this thread. That is an announcement of who holds the copyrights. Those two companies hold the copyrights. Who authored the code is completely unrelated to who holds the copyrights. If you are just asking out of curiosity, then I can say that many Sony Corp. employees, including myself, worked on the code.

-Geoff

#28 lore raw
In reply to Christoph Hellwig
Quoted 9 lines

arch/powerpc/platforms/ps3pf/mm.c line 56

Quoted 7 lines
arch/powerpc/platforms/ps3pf/mm.c new file +6 -0
+
+enum page_size {
+	page_size_4k = 12U,
+	page_size_64k = 16U,
+	page_size_16m = 24U,
+};

Please use ALL_CAPS for such constants.

And call them PAGE_SHIFT_* while at it :-)

Ben.

#29 lore raw
Quoted 10 lines · Benjamin Herrenschmidt
Quoted 8 lines

arch/powerpc/platforms/ps3pf/mm.c line 56

Quoted 6 lines
arch/powerpc/platforms/ps3pf/mm.c new file +6 -0
+
+enum page_size {
+	page_size_4k = 12U,
+	page_size_64k = 16U,
+	page_size_16m = 24U,
+};

Please use ALL_CAPS for such constants.

And call them PAGE_SHIFT_* while at it :-)

The HV docs call this page size, so that is what I used for the symbol. I'll look at the usage again and reconsider.

-Geoff

#30 lore raw
Quoted 10 lines · Geoff Levand
Quoted 9 lines · Benjamin Herrenschmidt
Quoted 8 lines

arch/powerpc/platforms/ps3pf/mm.c line 56

Quoted 6 lines
arch/powerpc/platforms/ps3pf/mm.c new file +6 -0
+
+enum page_size {
+	page_size_4k = 12U,
+	page_size_64k = 16U,
+	page_size_16m = 24U,
+};

Please use ALL_CAPS for such constants.

Quoted 4 lines
Quoted 1 line

And call them PAGE_SHIFT_* while at it :-)

The HV docs call this page size, so that is what I used for the symbol. I'll look at the usage again and reconsider.

No need to look at it, it's a shift not a size, the page size is not 12 bytes. Keeping it consistent with the HV docs is a concern, but so is making it consistent with every other part of the kernel - which calls this a shift, see page.h f.e.

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
#31 lore raw
Quoted 13 lines · Michael Ellerman
Quoted 12 lines · Geoff Levand
Quoted 10 lines · Benjamin Herrenschmidt
Quoted 8 lines

arch/powerpc/platforms/ps3pf/mm.c line 56

Quoted 6 lines
arch/powerpc/platforms/ps3pf/mm.c new file +6 -0
+
+enum page_size {
+	page_size_4k = 12U,
+	page_size_64k = 16U,
+	page_size_16m = 24U,
+};

Please use ALL_CAPS for such constants.

And call them PAGE_SHIFT_* while at it :-)

Quoted 7 lines
Quoted 2 lines

The HV docs call this page size, so that is what I used for the symbol. I'll look at the usage again and reconsider.

No need to look at it, it's a shift not a size, the page size is not 12 bytes. Keeping it consistent with the HV docs is a concern, but so is making it consistent with every other part of the kernel - which calls this a shift, see page.h f.e.

Well, I meant reconsider in more of a broad sense, in like, maybe I can just do something to get rid of it...

#32 lore raw
Quoted 17 lines · Geoff Levand
Quoted 16 lines · Michael Ellerman
Quoted 14 lines · Geoff Levand
Quoted 10 lines · Benjamin Herrenschmidt
Quoted 8 lines

arch/powerpc/platforms/ps3pf/mm.c line 56

Quoted 6 lines
arch/powerpc/platforms/ps3pf/mm.c new file +6 -0
+
+enum page_size {
+	page_size_4k = 12U,
+	page_size_64k = 16U,
+	page_size_16m = 24U,
+};

Please use ALL_CAPS for such constants.

And call them PAGE_SHIFT_* while at it :-)

The HV docs call this page size, so that is what I used for the symbol. I'll look at the usage again and reconsider.

Quoted 7 lines
Quoted 4 lines

No need to look at it, it's a shift not a size, the page size is not 12 bytes. Keeping it consistent with the HV docs is a concern, but so is making it consistent with every other part of the kernel - which calls this a shift, see page.h f.e.

Well, I meant reconsider in more of a broad sense, in like, maybe I can just do something to get rid of it...

Yeah ok, that's always a nice solution. My last post probably could have used a smiley in hindsight :)

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
#33 lore raw
Quoted 23 lines · Michael Ellerman
Quoted 22 lines · Geoff Levand
Quoted 20 lines · Michael Ellerman
Quoted 10 lines · Geoff Levand
Quoted 9 lines · Benjamin Herrenschmidt
Quoted 8 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+
+enum page_size {
+	page_size_4k = 12U,
+	page_size_64k = 16U,
+	page_size_16m = 24U,
+};

Please use ALL_CAPS for such constants.

Quoted 4 lines
Quoted 1 line

And call them PAGE_SHIFT_* while at it :-)

The HV docs call this page size, so that is what I used for the symbol. I'll look at the usage again and reconsider.

No need to look at it, it's a shift not a size, the page size is not 12 bytes. Keeping it consistent with the HV docs is a concern, but so is making it consistent with every other part of the kernel - which calls this a shift, see page.h f.e.

Quoted 5 lines
Quoted 2 lines

Well, I meant reconsider in more of a broad sense, in like, maybe I can just do something to get rid of it...

Yeah ok, that's always a nice solution. My last post probably could have used a smiley in hindsight :)

Anyway, I hope everyone's happy...

+enum {
+ PAGE_SHIFT_4K = 12U,
+ PAGE_SHIFT_64K = 16U,
+ PAGE_SHIFT_16M = 24U,
+};

#34 lore raw
In reply to Geoff Levand
Quoted 6 lines · Geoff Levand
arch/powerpc/platforms/ps3pf/setup.c new file +6 -0
+static int __init ps3pf_probe(void)
+{
+=A0=A0=A0=A0=A0=A0=A0unsigned long htab_size;
+
+=A0=A0=A0=A0=A0=A0=A0pr_debug(" -> %s:%d\n", __func__, __LINE__);
+

arch/powerpc/platforms/ps3pf/setup.c line 125

Quoted 10 lines
arch/powerpc/platforms/ps3pf/setup.c new file +10 -0
+=A0=A0=A0=A0=A0=A0=A0powerpc_firmware_features |=3D FW_FEATURE_LPAR;
+
+=A0=A0=A0=A0=A0=A0=A0ps3pf_mm_init();
+=A0=A0=A0=A0=A0=A0=A0ps3pf_mm_vas_create(&htab_size);
+=A0=A0=A0=A0=A0=A0=A0ps3pf_hpte_init(htab_size);
+
+=A0=A0=A0=A0=A0=A0=A0pr_debug(" <- %s:%d\n", __func__, __LINE__);
+=A0=A0=A0=A0=A0=A0=A0return 1;
+}
+

Your probe() function needs to check if you are actually running on appropriate hardware and return 0 if not.

This currently breaks when booting a multiplatform kernel on a non-ps3 system.

	Arnd <><