On Tue, 2007-07-24 at 11:36 -0300, Andre Detsch wrote:
> Subject: cell - move SPU affinity init code to a separate file
>
> From: Andre Detsch <adetsch at br.ibm.com>
>
> This patch moves affinity initialization code from spu_base.c to a
> new spu_affinity.c file. This file is not compiled for PS3,
> fixing a linking issue which was happening when compiling for this
> platform.
>
> SPU affinity initialization must be run _after_ spu_base.c
> initialization. This is ensured through the order of the object files
> set in the Makefile.
Hi André,
You're lucky I woke up in a bad mood this morning, so I thought I'd
review your code ;) ...
> Index: linux-2.6.22/arch/powerpc/platforms/cell/Makefile
> ===================================================================
> --- linux-2.6.22.orig/arch/powerpc/platforms/cell/Makefile
> +++ linux-2.6.22/arch/powerpc/platforms/cell/Makefile
> @@ -16,8 +16,8 @@ endif
> spufs-modular-$(CONFIG_SPU_FS) += spu_syscalls.o
> spu-priv1-$(CONFIG_PPC_CELL_NATIVE) += spu_priv1_mmio.o
>
> -spu-manage-$(CONFIG_PPC_CELLEB) += spu_manage.o
> -spu-manage-$(CONFIG_PPC_CELL_NATIVE) += spu_manage.o
> +spu-manage-$(CONFIG_PPC_CELLEB) += spu_manage.o spu_affinity.o
Does it really work on CELLEB? Why is CELLEB stuff in here anyway?
> +spu-manage-$(CONFIG_PPC_CELL_NATIVE) += spu_manage.o spu_affinity.o
>
> obj-$(CONFIG_SPU_BASE) += spu_callbacks.o spu_base.o \
> spu_coredump.o \
> Index: linux-2.6.22/arch/powerpc/platforms/cell/spu_affinity.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6.22/arch/powerpc/platforms/cell/spu_affinity.c
> @@ -0,0 +1,189 @@
> +/*
> + * SPU affinity initialization
> + *
> + * (C) Copyright IBM 2007
> + *
> + * Author: Andre Destch <adetsch at br.ibm.com>
> + *
> + * 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; either version 2, or (at your option)
> + * any later version.
> + *
> + * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/ptrace.h>
> +#include <linux/slab.h>
> +#include <linux/wait.h>
> +#include <linux/mm.h>
> +#include <linux/io.h>
> +#include <linux/mutex.h>
> +#include <linux/linux_logo.h>
> +#include <asm/spu.h>
> +#include <asm/spu_priv1.h>
> +#include <asm/xmon.h>
> +#include <asm/prom.h>
> +#include "spu_priv1_mmio.h"
> +
> +/* Hardcoded affinity idxs for QS20 */
> +#define SPES_PER_BE 8
Is this not defined somewhere else already? And it should really be
called QS20_SPES_PER_BE.
> +static int QS20_reg_idxs[SPES_PER_BE] = { 0, 2, 4, 6, 7, 5, 3, 1 };
> +static int QS20_reg_memory[SPES_PER_BE] = { 1, 1, 0, 0, 0, 0, 0, 0 };
I'd rather see these be qs20_reg_foo, but whatever.
> +static struct spu *spu_lookup_reg(int node, u32 reg)
> +{
> + struct spu *spu;
> +
> + list_for_each_entry(spu, &cbe_spu_info[node].spus, cbe_list) {
> + if (*(u32 *)get_property(spu_devnode(spu), "reg", NULL) == reg)
That's ugly, use a variable. Isn't it called of_get_property() ?
> + return spu;
> + }
> + return NULL;
> +}
> +
> +static void init_aff_QS20_harcoded(void)
How about init_affinity_qs20_hardcoded()
> +{
> + int node, i;
> + struct spu *last_spu, *spu;
> + u32 reg;
> +
> + for (node = 0; node < MAX_NUMNODES; node++) {
> + last_spu = NULL;
> + for (i = 0; i < SPES_PER_BE; i++) {
> + reg = QS20_reg_idxs[i];
> + spu = spu_lookup_reg(node, reg);
> + if (!spu)
> + continue;
> + spu->has_mem_affinity = QS20_reg_memory[reg];
> + if (last_spu)
> + list_add_tail(&spu->aff_list,
> + &last_spu->aff_list);
> + last_spu = spu;
> + }
> + }
> +}
> +
> +static int of_has_vicinity(void)
> +{
> + struct spu* spu;
> +
> + spu = list_entry(cbe_spu_info[0].spus.next, struct spu, cbe_list);
Use list_first_entry() here.
> + return of_find_property(spu_devnode(spu), "vicinity", NULL) != NULL;
> +}
What is the difference between vicinity and affinity?
> +
> +static struct spu *aff_devnode_spu(int cbe, struct device_node *dn)
> +{
> + struct spu *spu;
> +
> + list_for_each_entry(spu, &cbe_spu_info[cbe].spus, cbe_list)
> + if (spu_devnode(spu) == dn)
> + return spu;
> + return NULL;
> +}
This has nothing to do with affinity does it? It's just a generic helper
to find a spu based on the devnode.
> +
> +static struct spu *
> +aff_node_next_to(int cbe, struct device_node *target, struct device_node *avoid)
I read the "next" in here as a verb, perhaps use neighbour or something
instead.
> +{
> + struct spu *spu;
> + const phandle *vic_handles;
> + int lenp, i;
> +
> + list_for_each_entry(spu, &cbe_spu_info[cbe].spus, cbe_list) {
> + if (spu_devnode(spu) == avoid)
> + continue;
> + vic_handles = get_property(spu_devnode(spu), "vicinity", &lenp);
> + for (i=0; i < (lenp / sizeof(phandle)); i++) {
> + if (vic_handles[i] == target->linux_phandle)
> + return spu;
> + }
> + }
> + return NULL;
> +}
> +
> +static void init_aff_fw_vicinity_node(int cbe)
init_affinity_node() ?
This function could really use some comments.
> +{
> + struct spu *spu, *last_spu;
> + struct device_node *vic_dn, *last_spu_dn;
> + phandle avoid_ph;
> + const phandle *vic_handles;
> + const char *name;
> + int lenp, i, added, mem_aff;
> +
> + last_spu = list_entry(cbe_spu_info[cbe].spus.next, struct spu, cbe_list);
list_first_entry() again.
> + avoid_ph = 0;
> + for (added = 1; added < cbe_spu_info[cbe].n_spus; added++) {
> + last_spu_dn = spu_devnode(last_spu);
> + vic_handles = get_property(last_spu_dn, "vicinity", &lenp);
> +
> + for (i = 0; i < (lenp / sizeof(phandle)); i++) {
> + if (vic_handles[i] == avoid_ph)
> + continue;
> +
> + vic_dn = of_find_node_by_phandle(vic_handles[i]);
> + if (!vic_dn)
> + continue;
> +
> + name = get_property(vic_dn, "name", NULL);
name? not device_type/compatible ?
> + if (strcmp(name, "spe") == 0) {
> + spu = aff_devnode_spu(cbe, vic_dn);
> + avoid_ph = last_spu_dn->linux_phandle;
> + }
} else {
> + else {
> + mem_aff = strcmp(name, "mic-tm") == 0;
You don't seem to use mem_aff anywhere except the if below, so just do
the strcmp there.
> + spu = aff_node_next_to(cbe, vic_dn, last_spu_dn);
> + if (!spu)
> + continue;
> + if (mem_aff) {
> + last_spu->has_mem_affinity = 1;
> + spu->has_mem_affinity = 1;
> + }
> + avoid_ph = vic_dn->linux_phandle;
> + }
> + list_add_tail(&spu->aff_list, &last_spu->aff_list);
> + last_spu = spu;
> + break;
> + }
> + }
> +}
> +
> +static void init_aff_fw_vicinity(void)
> +{
> + int cbe;
> +
> + /*
* sets has_mem_affinity for each spu, as long as the
> + * spu->aff_list list, linking each spu to its neighbors
> + */
Comment doesn't quite make sense.
> + for (cbe = 0; cbe < MAX_NUMNODES; cbe++)
> + init_aff_fw_vicinity_node(cbe);
> +}
> +
> +
> +
> +static int init_spu_affinity(void)
> +{
> + if (of_has_vicinity()) {
> + init_aff_fw_vicinity();
What happens if init_aff_fw_vicinity() fails? Shouldn't we drop back to
the QS20 hardcoding? I prefer the model where we try
init_aff_fw_vicinity() and if there's any error we drop back to hard
coding.
> + } else {
> + long root = of_get_flat_dt_root();
> + if (of_flat_dt_is_compatible(root, "IBM,CPBW-1.0"))
> + init_aff_QS20_harcoded();
else
printk("No affinity configuration found") ?
> + }
> +
> + return 0;
> +}
> +
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/cbe-oss-dev/attachments/20070725/47591f4e/attachment.pgp>