LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 6/6] show processor cache information in sysfs
From: Nathan Lynch @ 2008-07-27  5:24 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1217136295-18693-1-git-send-email-ntl@pobox.com>

Collect cache information from the OF device tree and display it in
the cpu hierarchy in sysfs.  This is intended to be compatible at the
userspace level with x86's implementation[1], hence some of the funny
attribute names.  The arrangement of cache info is not immediately
intuitive, but (again) it's for compatibility's sake.

The cache attributes exposed are:

type (Data, Instruction, or Unified)
level (1, 2, 3...)
size
coherency_line_size
number_of_sets
ways_of_associativity

All of these can be derived on platforms that follow the OF PowerPC
Processor binding.  The code "publishes" only those attributes for
which it is able to determine values; attributes for values which
cannot be determined are not created at all.

[1] arch/x86/kernel/cpu/intel_cacheinfo.c

Signed-off-by: Nathan Lynch <ntl@pobox.com>
---
 arch/powerpc/kernel/sysfs.c |  308 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 308 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 1568080..7f56cc8 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -22,6 +22,8 @@
 
 static DEFINE_PER_CPU(struct cpu, cpu_devices);
 
+static DEFINE_PER_CPU(struct kobject *, cache_toplevel);
+
 /* SMT stuff */
 
 #ifdef CONFIG_PPC_MULTIPLATFORM
@@ -297,6 +299,286 @@ static struct sysdev_attribute pa6t_attrs[] = {
 #endif /* CONFIG_DEBUG_KERNEL */
 };
 
+struct cache_desc {
+	struct kobject kobj;
+	struct cache_desc *next;
+	const char *type;	/* Instruction, Data, or Unified */
+	u32 size;		/* total cache size in KB */
+	u32 line_size;		/* in bytes */
+	u32 nr_sets;		/* number of sets */
+	u32 level;		/* e.g. 1, 2, 3... */
+	u32 associativity;	/* e.g. 8-way... 0 is fully associative */
+};
+
+DEFINE_PER_CPU(struct cache_desc *, cache_desc);
+
+static struct cache_desc *kobj_to_cache_desc(struct kobject *k)
+{
+	return container_of(k, struct cache_desc, kobj);
+}
+
+static void cache_desc_release(struct kobject *k)
+{
+	struct cache_desc *desc = kobj_to_cache_desc(k);
+
+	printk("releasing %s\n", kobject_name(k));
+
+	if (desc->next)
+		kobject_put(&desc->next->kobj);
+
+	kfree(kobj_to_cache_desc(k));
+}
+
+static ssize_t cache_desc_show(struct kobject *k, struct attribute *attr, char *buf)
+{
+	struct kobj_attribute *kobj_attr;
+
+	kobj_attr = container_of(attr, struct kobj_attribute, attr);
+
+	return kobj_attr->show(k, kobj_attr, buf);
+}
+
+static struct sysfs_ops cache_desc_sysfs_ops = {
+	.show = cache_desc_show,
+};
+
+static struct kobj_type cache_desc_type = {
+	.release = cache_desc_release,
+	.sysfs_ops = &cache_desc_sysfs_ops,
+};
+
+static ssize_t cache_size_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
+{
+	struct cache_desc *cache = kobj_to_cache_desc(k);
+
+	return sprintf(buf, "%uK\n", cache->size);
+}
+
+static struct kobj_attribute cache_size_attr =
+	__ATTR(size, 0444, cache_size_show, NULL);
+
+static ssize_t cache_line_size_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
+{
+	struct cache_desc *cache = kobj_to_cache_desc(k);
+
+	return sprintf(buf, "%u\n", cache->line_size);
+}
+
+static struct kobj_attribute cache_line_size_attr =
+	__ATTR(coherency_line_size, 0444, cache_line_size_show, NULL);
+
+static ssize_t cache_nr_sets_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
+{
+	struct cache_desc *cache = kobj_to_cache_desc(k);
+
+	return sprintf(buf, "%u\n", cache->nr_sets);
+}
+
+static struct kobj_attribute cache_nr_sets_attr =
+	__ATTR(number_of_sets, 0444, cache_nr_sets_show, NULL);
+
+static ssize_t cache_type_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
+{
+	struct cache_desc *cache = kobj_to_cache_desc(k);
+
+	return sprintf(buf, "%s\n", cache->type);
+}
+
+static struct kobj_attribute cache_type_attr =
+	__ATTR(type, 0444, cache_type_show, NULL);
+
+static ssize_t cache_level_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
+{
+	struct cache_desc *cache = kobj_to_cache_desc(k);
+
+	return sprintf(buf, "%u\n", cache->level);
+}
+
+static struct kobj_attribute cache_level_attr =
+	__ATTR(level, 0444, cache_level_show, NULL);
+
+static ssize_t cache_assoc_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
+{
+	struct cache_desc *cache = kobj_to_cache_desc(k);
+
+	return sprintf(buf, "%u\n", cache->associativity);
+}
+
+static struct kobj_attribute cache_assoc_attr =
+	__ATTR(ways_of_associativity, 0444, cache_assoc_show, NULL);
+
+struct cache_desc_info {
+	const char *type;
+	const char *size_prop;
+	const char *line_size_prop;
+	const char *nr_sets_prop;
+};
+
+/* PowerPC Processor binding says the [di]-cache-* must be equal on
+ * unified caches, so just use d-cache properties. */
+static struct cache_desc_info ucache_info = {
+	.type = "Unified",
+	.size_prop = "d-cache-size",
+	.line_size_prop = "d-cache-line-size",
+	.nr_sets_prop = "d-cache-sets",
+};
+
+static struct cache_desc_info dcache_info = {
+	.type = "Data",
+	.size_prop = "d-cache-size",
+	.line_size_prop = "d-cache-line-size",
+	.nr_sets_prop = "d-cache-sets",
+};
+
+static struct cache_desc_info icache_info = {
+	.type = "Instruction",
+	.size_prop = "i-cache-size",
+	.line_size_prop = "i-cache-line-size",
+	.nr_sets_prop = "i-cache-sets",
+};
+
+static struct cache_desc * __cpuinit create_cache_desc(struct device_node *np, struct kobject *parent, int index, int level, struct cache_desc_info *info)
+{
+	const u32 *cache_line_size;
+	struct cache_desc *new;
+	const u32 *cache_size;
+	const u32 *nr_sets;
+	int rc;
+
+	new = kzalloc(sizeof(*new), GFP_KERNEL);
+	if (!new)
+		return NULL;
+
+	rc = kobject_init_and_add(&new->kobj, &cache_desc_type, parent,
+				  "index%d", index);
+	if (rc)
+		goto err;
+
+	/* type */
+	new->type = info->type;
+	rc = sysfs_create_file(&new->kobj, &cache_type_attr.attr);
+	WARN_ON(rc);
+
+	/* level */
+	new->level = level;
+	rc = sysfs_create_file(&new->kobj, &cache_level_attr.attr);
+	WARN_ON(rc);
+
+	/* size */
+	cache_size = of_get_property(np, info->size_prop, NULL);
+	if (cache_size) {
+		new->size = *cache_size / 1024;
+		rc = sysfs_create_file(&new->kobj,
+				       &cache_size_attr.attr);
+		WARN_ON(rc);
+	}
+
+	/* coherency_line_size */
+	cache_line_size = of_get_property(np, info->line_size_prop, NULL);
+	if (cache_line_size) {
+		new->line_size = *cache_line_size;
+		rc = sysfs_create_file(&new->kobj,
+				       &cache_line_size_attr.attr);
+		WARN_ON(rc);
+	}
+
+	/* number_of_sets */
+	nr_sets = of_get_property(np, info->nr_sets_prop, NULL);
+	if (nr_sets) {
+		new->nr_sets = *nr_sets;
+		rc = sysfs_create_file(&new->kobj,
+				       &cache_nr_sets_attr.attr);
+		WARN_ON(rc);
+	}
+
+	/* ways_of_associativity */
+	if (new->nr_sets == 1) {
+		/* fully associative */
+		new->associativity = 0;
+		goto create_assoc;
+	}
+
+	if (new->nr_sets && new->size && new->line_size) {
+		/* If we have values for all of these we can derive
+		 * the associativity. */
+		new->associativity =
+			((new->size * 1024) / new->nr_sets) / new->line_size;
+create_assoc:
+		rc = sysfs_create_file(&new->kobj,
+				       &cache_assoc_attr.attr);
+		WARN_ON(rc);
+	}
+
+	return new;
+err:
+	kfree(new);
+	return NULL;
+}
+
+static bool cache_is_unified(struct device_node *np)
+{
+	return of_get_property(np, "cache-unified", NULL);
+}
+
+static struct cache_desc * __cpuinit create_cache_index_info(struct device_node *np, struct kobject *parent, int index, int level)
+{
+	const phandle *next_cache_phandle;
+	struct device_node *next_cache;
+	struct cache_desc *new, **end;
+
+	printk("%s(np = %s, index = %d)\n", __func__, np->name, index);
+	if (cache_is_unified(np)) {
+		new = create_cache_desc(np, parent, index, level,
+					&ucache_info);
+	} else {
+		new = create_cache_desc(np, parent, index, level,
+					&dcache_info);
+		if (new) {
+			index++;
+			new->next = create_cache_desc(np, parent, index, level,
+						      &icache_info);
+		}
+	}
+	if (!new)
+		return NULL;
+
+	end = &new->next;
+	while (*end)
+		end = &(*end)->next;
+
+	next_cache_phandle = of_get_property(np, "l2-cache", NULL);
+	if (!next_cache_phandle)
+		goto out;
+
+	next_cache = of_find_node_by_phandle(*next_cache_phandle);
+	if (!next_cache)
+		goto out;
+
+	*end = create_cache_index_info(next_cache, parent, ++index, ++level);
+
+	of_node_put(next_cache);
+out:
+	return new;
+}
+
+static void __cpuinit create_cache_info(struct sys_device *sysdev)
+{
+	struct kobject *cache_toplevel;
+	struct device_node *np = NULL;
+	int cpu = sysdev->id;
+
+	cache_toplevel = kobject_create_and_add("cache", &sysdev->kobj);
+	if (!cache_toplevel)
+		return;
+	per_cpu(cache_toplevel, cpu) = cache_toplevel;
+
+	np = of_get_cpu_node(cpu, NULL);
+	per_cpu(cache_desc, cpu) =
+		create_cache_index_info(np, cache_toplevel, 0, 1);
+	of_node_put(np);
+
+	return;
+}
 
 static void __cpuinit register_cpu_online(unsigned int cpu)
 {
@@ -346,9 +628,33 @@ static void __cpuinit register_cpu_online(unsigned int cpu)
 
 	if (cpu_has_feature(CPU_FTR_DSCR))
 		sysdev_create_file(s, &attr_dscr);
+
+	create_cache_info(s);
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
+static void remove_cache_info(struct sys_device *sysdev)
+{
+	struct kobject *cache_toplevel;
+	struct cache_desc *cache_desc;
+	int cpu = sysdev->id;
+
+	cache_desc = per_cpu(cache_desc, cpu);
+
+	sysfs_remove_file(&cache_desc->kobj, &cache_size_attr.attr);
+	sysfs_remove_file(&cache_desc->kobj, &cache_line_size_attr.attr);
+	sysfs_remove_file(&cache_desc->kobj, &cache_type_attr.attr);
+	sysfs_remove_file(&cache_desc->kobj, &cache_level_attr.attr);
+	sysfs_remove_file(&cache_desc->kobj, &cache_nr_sets_attr.attr);
+	sysfs_remove_file(&cache_desc->kobj, &cache_assoc_attr.attr);
+
+	kobject_put(&cache_desc->kobj);
+
+	cache_toplevel = per_cpu(cache_toplevel, cpu);
+
+	kobject_put(cache_toplevel);
+}
+
 static void unregister_cpu_online(unsigned int cpu)
 {
 	struct cpu *c = &per_cpu(cpu_devices, cpu);
@@ -399,6 +705,8 @@ static void unregister_cpu_online(unsigned int cpu)
 
 	if (cpu_has_feature(CPU_FTR_DSCR))
 		sysdev_remove_file(s, &attr_dscr);
+
+	remove_cache_info(s);
 }
 #endif /* CONFIG_HOTPLUG_CPU */
 
-- 
1.5.6.2

^ permalink raw reply related

* [PATCH 5/6] make core id information available to userspace
From: Nathan Lynch @ 2008-07-27  5:24 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1217136295-18693-1-git-send-email-ntl@pobox.com>

Existing Open Firmware practice is to report each processor core as a
separate node in the device tree.  Report the value of the "reg" OF
property corresponding to a logical CPU's device node as the core_id
attribute in /sys/devices/system/cpu/cpu*/topology/core_id.

Signed-off-by: Nathan Lynch <ntl@pobox.com>
---
 arch/powerpc/kernel/smp.c      |   23 +++++++++++++++++++++++
 include/asm-powerpc/smp.h      |    1 +
 include/asm-powerpc/topology.h |    1 +
 3 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index e8bc6a0..f3a2360 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -380,6 +380,29 @@ int __cpuinit __cpu_up(unsigned int cpu)
 	return 0;
 }
 
+/* Return the value of the reg property corresponding to the given
+ * logical cpu.
+ */
+int cpu_to_core_id(int cpu)
+{
+	struct device_node *np;
+	const int *reg;
+	int id = -1;
+
+	np = of_get_cpu_node(cpu, NULL);
+	if (!np)
+		goto out;
+
+	reg = of_get_property(np, "reg", NULL);
+	if (!reg)
+		goto out;
+
+	id = *reg;
+out:
+	of_node_put(np);
+	return id;
+}
+
 /* Must be called when no change can occur to cpu_present_map,
  * i.e. during cpu online or offline.
  */
diff --git a/include/asm-powerpc/smp.h b/include/asm-powerpc/smp.h
index 32e9100..4d28e1e 100644
--- a/include/asm-powerpc/smp.h
+++ b/include/asm-powerpc/smp.h
@@ -63,6 +63,7 @@ extern int smp_hw_index[];
 
 DECLARE_PER_CPU(cpumask_t, cpu_sibling_map);
 DECLARE_PER_CPU(cpumask_t, cpu_core_map);
+extern int cpu_to_core_id(int cpu);
 
 /* Since OpenPIC has only 4 IPIs, we use slightly different message numbers.
  *
diff --git a/include/asm-powerpc/topology.h b/include/asm-powerpc/topology.h
index f00e8e5..c32da6f 100644
--- a/include/asm-powerpc/topology.h
+++ b/include/asm-powerpc/topology.h
@@ -109,6 +109,7 @@ static inline void sysfs_remove_device_from_node(struct sys_device *dev,
 
 #define topology_thread_siblings(cpu)	(per_cpu(cpu_sibling_map, cpu))
 #define topology_core_siblings(cpu)	(per_cpu(cpu_core_map, cpu))
+#define topology_core_id(cpu)		(cpu_to_core_id(cpu))
 #endif
 #endif
 
-- 
1.5.6.2

^ permalink raw reply related

* [PATCH 3/6] Update cpu_sibling_maps dynamically
From: Nathan Lynch @ 2008-07-27  5:24 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1217136295-18693-1-git-send-email-ntl@pobox.com>

Rather doing one initialization pass over all the per-cpu
cpu_sibling_maps at boot, update the maps at cpu online/offline time.

This is a behavior change -- the thread_siblings attribute now
reflects only online siblings, whereas it would display offline
siblings before.  The new behavior matches that of x86, and is
arguably more useful.

Signed-off-by: Nathan Lynch <ntl@pobox.com>
---
 arch/powerpc/kernel/setup-common.c |   24 ------------------------
 arch/powerpc/kernel/setup_64.c     |    3 ---
 arch/powerpc/kernel/smp.c          |   32 +++++++++++++++++++++++++++++---
 3 files changed, 29 insertions(+), 30 deletions(-)

diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 61a3f41..9cc5a52 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -367,7 +367,6 @@ static void __init cpu_init_thread_core_maps(int tpc)
  * setup_cpu_maps - initialize the following cpu maps:
  *                  cpu_possible_map
  *                  cpu_present_map
- *                  cpu_sibling_map
  *
  * Having the possible map set up early allows us to restrict allocations
  * of things like irqstacks to num_possible_cpus() rather than NR_CPUS.
@@ -475,29 +474,6 @@ void __init smp_setup_cpu_maps(void)
 	 */
 	cpu_init_thread_core_maps(nthreads);
 }
-
-/*
- * Being that cpu_sibling_map is now a per_cpu array, then it cannot
- * be initialized until the per_cpu areas have been created.  This
- * function is now called from setup_per_cpu_areas().
- */
-void __init smp_setup_cpu_sibling_map(void)
-{
-#ifdef CONFIG_PPC64
-	int i, cpu, base;
-
-	for_each_possible_cpu(cpu) {
-		DBG("Sibling map for CPU %d:", cpu);
-		base = cpu_first_thread_in_core(cpu);
-		for (i = 0; i < threads_per_core; i++) {
-			cpu_set(base + i, per_cpu(cpu_sibling_map, cpu));
-			DBG(" %d", base + i);
-		}
-		DBG("\n");
-	}
-
-#endif /* CONFIG_PPC64 */
-}
 #endif /* CONFIG_SMP */
 
 #ifdef CONFIG_PCSPKR_PLATFORM
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 04d8de9..8b25f51 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -611,9 +611,6 @@ void __init setup_per_cpu_areas(void)
 		paca[i].data_offset = ptr - __per_cpu_start;
 		memcpy(ptr, __per_cpu_start, __per_cpu_end - __per_cpu_start);
 	}
-
-	/* Now that per_cpu is setup, initialize cpu_sibling_map */
-	smp_setup_cpu_sibling_map();
 }
 #endif
 
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index f5ae9fa..3c4d07e 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -41,6 +41,7 @@
 #include <asm/smp.h>
 #include <asm/time.h>
 #include <asm/machdep.h>
+#include <asm/cputhreads.h>
 #include <asm/cputable.h>
 #include <asm/system.h>
 #include <asm/mpic.h>
@@ -228,6 +229,7 @@ void __devinit smp_prepare_boot_cpu(void)
 	BUG_ON(smp_processor_id() != boot_cpuid);
 
 	cpu_set(boot_cpuid, cpu_online_map);
+	cpu_set(boot_cpuid, per_cpu(cpu_sibling_map, boot_cpuid));
 #ifdef CONFIG_PPC64
 	paca[boot_cpuid].__current = current;
 #endif
@@ -380,6 +382,7 @@ int __cpuinit __cpu_up(unsigned int cpu)
 int __devinit start_secondary(void *unused)
 {
 	unsigned int cpu = smp_processor_id();
+	int i, base;
 
 	atomic_inc(&init_mm.mm_count);
 	current->active_mm = &init_mm;
@@ -400,6 +403,14 @@ int __devinit start_secondary(void *unused)
 
 	ipi_call_lock();
 	cpu_set(cpu, cpu_online_map);
+	/* Update sibling maps */
+	base = cpu_first_thread_in_core(cpu);
+	for (i = 0; i < threads_per_core; i++) {
+		if (cpu_is_offline(base + i))
+			continue;
+		cpu_set(cpu, per_cpu(cpu_sibling_map, base + i));
+		cpu_set(base + i, per_cpu(cpu_sibling_map, cpu));
+	}
 	ipi_call_unlock();
 
 	local_irq_enable();
@@ -437,10 +448,25 @@ void __init smp_cpus_done(unsigned int max_cpus)
 #ifdef CONFIG_HOTPLUG_CPU
 int __cpu_disable(void)
 {
-	if (smp_ops->cpu_disable)
-		return smp_ops->cpu_disable();
+	int cpu = smp_processor_id();
+	int base, i;
+	int err;
 
-	return -ENOSYS;
+	if (!smp_ops->cpu_disable)
+		return -ENOSYS;
+
+	err = smp_ops->cpu_disable();
+	if (err)
+		return err;
+
+	/* Update sibling maps */
+	base = cpu_first_thread_in_core(cpu);
+	for (i = 0; i < threads_per_core; i++) {
+		cpu_clear(cpu, per_cpu(cpu_sibling_map, base + i));
+		cpu_clear(base + i, per_cpu(cpu_sibling_map, cpu));
+	}
+
+	return 0;
 }
 
 void __cpu_die(unsigned int cpu)
-- 
1.5.6.2

^ permalink raw reply related

* [PATCH 4/6] make core sibling information available to userspace
From: Nathan Lynch @ 2008-07-27  5:24 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1217136295-18693-1-git-send-email-ntl@pobox.com>

Implement the notion of "core siblings" for powerpc.  This makes
/sys/devices/system/cpu/cpu*/topology/core_siblings present sensible
values, indicating online CPUs which share an L2 cache.

Signed-off-by: Nathan Lynch <ntl@pobox.com>
---
 arch/powerpc/kernel/smp.c      |   71 ++++++++++++++++++++++++++++++++++++++++
 include/asm-powerpc/smp.h      |    1 +
 include/asm-powerpc/topology.h |    1 +
 3 files changed, 73 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 3c4d07e..e8bc6a0 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -63,10 +63,12 @@ struct thread_info *secondary_ti;
 cpumask_t cpu_possible_map = CPU_MASK_NONE;
 cpumask_t cpu_online_map = CPU_MASK_NONE;
 DEFINE_PER_CPU(cpumask_t, cpu_sibling_map) = CPU_MASK_NONE;
+DEFINE_PER_CPU(cpumask_t, cpu_core_map) = CPU_MASK_NONE;
 
 EXPORT_SYMBOL(cpu_online_map);
 EXPORT_SYMBOL(cpu_possible_map);
 EXPORT_PER_CPU_SYMBOL(cpu_sibling_map);
+EXPORT_PER_CPU_SYMBOL(cpu_core_map);
 
 /* SMP operations for this machine */
 struct smp_ops_t *smp_ops;
@@ -230,6 +232,7 @@ void __devinit smp_prepare_boot_cpu(void)
 
 	cpu_set(boot_cpuid, cpu_online_map);
 	cpu_set(boot_cpuid, per_cpu(cpu_sibling_map, boot_cpuid));
+	cpu_set(boot_cpuid, per_cpu(cpu_core_map, boot_cpuid));
 #ifdef CONFIG_PPC64
 	paca[boot_cpuid].__current = current;
 #endif
@@ -377,11 +380,43 @@ int __cpuinit __cpu_up(unsigned int cpu)
 	return 0;
 }
 
+/* Must be called when no change can occur to cpu_present_map,
+ * i.e. during cpu online or offline.
+ */
+static struct device_node *cpu_to_l2cache(int cpu)
+{
+	struct device_node *np;
+	const u32 *l2cache_phandle;
+
+	if (!cpu_present(cpu))
+		return NULL;
+
+	np = of_get_cpu_node(cpu, NULL);
+
+	l2cache_phandle = of_get_property(np, "l2-cache", NULL);
+
+	of_node_put(np);
+
+	if (!l2cache_phandle)
+		return NULL;
+
+	np = NULL;
+	for_each_node_by_type(np, "cache") {
+		const u32 *phandle = of_get_property(np, "ibm,phandle", NULL);
+		if (!phandle)
+			continue;
+		if (*phandle == *l2cache_phandle)
+			return np;
+	}
+
+	return NULL;
+}
 
 /* Activate a secondary processor. */
 int __devinit start_secondary(void *unused)
 {
 	unsigned int cpu = smp_processor_id();
+	struct device_node *l2_cache;
 	int i, base;
 
 	atomic_inc(&init_mm.mm_count);
@@ -410,7 +445,26 @@ int __devinit start_secondary(void *unused)
 			continue;
 		cpu_set(cpu, per_cpu(cpu_sibling_map, base + i));
 		cpu_set(base + i, per_cpu(cpu_sibling_map, cpu));
+
+		/* cpu_core_map should be a superset of
+		 * cpu_sibling_map even if we don't have cache
+		 * information, so update the former here, too.
+		 */
+		cpu_set(cpu, per_cpu(cpu_core_map, base +i));
+		cpu_set(base + i, per_cpu(cpu_core_map, cpu));
+	}
+	l2_cache = cpu_to_l2cache(cpu);
+	for_each_online_cpu(i) {
+		struct device_node *np = cpu_to_l2cache(i);
+		if (!np)
+			continue;
+		if (np == l2_cache) {
+			cpu_set(cpu, per_cpu(cpu_core_map, i));
+			cpu_set(i, per_cpu(cpu_core_map, cpu));
+		}
+		of_node_put(np);
 	}
+	of_node_put(l2_cache);
 	ipi_call_unlock();
 
 	local_irq_enable();
@@ -448,6 +502,7 @@ void __init smp_cpus_done(unsigned int max_cpus)
 #ifdef CONFIG_HOTPLUG_CPU
 int __cpu_disable(void)
 {
+	struct device_node *l2_cache;
 	int cpu = smp_processor_id();
 	int base, i;
 	int err;
@@ -464,8 +519,24 @@ int __cpu_disable(void)
 	for (i = 0; i < threads_per_core; i++) {
 		cpu_clear(cpu, per_cpu(cpu_sibling_map, base + i));
 		cpu_clear(base + i, per_cpu(cpu_sibling_map, cpu));
+		cpu_clear(cpu, per_cpu(cpu_core_map, base +i));
+		cpu_clear(base + i, per_cpu(cpu_core_map, cpu));
 	}
 
+	l2_cache = cpu_to_l2cache(cpu);
+	for_each_present_cpu(i) {
+		struct device_node *np = cpu_to_l2cache(i);
+		if (!np)
+			continue;
+		if (np == l2_cache) {
+			cpu_clear(cpu, per_cpu(cpu_core_map, i));
+			cpu_clear(i, per_cpu(cpu_core_map, cpu));
+		}
+		of_node_put(np);
+	}
+	of_node_put(l2_cache);
+
+
 	return 0;
 }
 
diff --git a/include/asm-powerpc/smp.h b/include/asm-powerpc/smp.h
index 416d4c2..32e9100 100644
--- a/include/asm-powerpc/smp.h
+++ b/include/asm-powerpc/smp.h
@@ -62,6 +62,7 @@ extern int smp_hw_index[];
 #endif
 
 DECLARE_PER_CPU(cpumask_t, cpu_sibling_map);
+DECLARE_PER_CPU(cpumask_t, cpu_core_map);
 
 /* Since OpenPIC has only 4 IPIs, we use slightly different message numbers.
  *
diff --git a/include/asm-powerpc/topology.h b/include/asm-powerpc/topology.h
index 100c6fb..f00e8e5 100644
--- a/include/asm-powerpc/topology.h
+++ b/include/asm-powerpc/topology.h
@@ -108,6 +108,7 @@ static inline void sysfs_remove_device_from_node(struct sys_device *dev,
 #include <asm/smp.h>
 
 #define topology_thread_siblings(cpu)	(per_cpu(cpu_sibling_map, cpu))
+#define topology_core_siblings(cpu)	(per_cpu(cpu_core_map, cpu))
 #endif
 #endif
 
-- 
1.5.6.2

^ permalink raw reply related

* [PATCH 2/6] register_cpu_online should be __cpuinit
From: Nathan Lynch @ 2008-07-27  5:24 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1217136295-18693-1-git-send-email-ntl@pobox.com>

It is called only in cpu online paths.

(caught by CONFIG_DEBUG_SECTION_MISMATCH=y)

Signed-off-by: Nathan Lynch <ntl@pobox.com>
---
 arch/powerpc/kernel/sysfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 800e5e9..1568080 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -298,7 +298,7 @@ static struct sysdev_attribute pa6t_attrs[] = {
 };
 
 
-static void register_cpu_online(unsigned int cpu)
+static void __cpuinit register_cpu_online(unsigned int cpu)
 {
 	struct cpu *c = &per_cpu(cpu_devices, cpu);
 	struct sys_device *s = &c->sysdev;
-- 
1.5.6.2

^ permalink raw reply related

* [PATCH 0/6] powerpc topology updates
From: Nathan Lynch @ 2008-07-27  5:24 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras

This series consists of a couple of cleanups and a few feature
additions, all of which are more or less related to system topology
(threads, cores, caches, sysfs...)  These are independent of
each other except for 4 and 5 (core sibling and core id info).

I'd say the highlights are the last three patches, which add core and
cache information to sysfs.  Here is some example output with the
patches applied:

(Power5)
# grep -r . /sys/devices/system/cpu/cpu0/cache/ | sed
  's/\/sys\/devices\/system\/cpu\///'
cpu0/cache/index0/type:Data
cpu0/cache/index0/level:1
cpu0/cache/index0/size:32K
cpu0/cache/index0/coherency_line_size:128
cpu0/cache/index0/number_of_sets:64
cpu0/cache/index0/ways_of_associativity:4
cpu0/cache/index1/type:Instruction
cpu0/cache/index1/level:1
cpu0/cache/index1/size:64K
cpu0/cache/index1/coherency_line_size:128
cpu0/cache/index1/number_of_sets:256
cpu0/cache/index1/ways_of_associativity:2
cpu0/cache/index2/type:Unified
cpu0/cache/index2/level:2
cpu0/cache/index2/size:1920K
cpu0/cache/index2/coherency_line_size:128
cpu0/cache/index2/number_of_sets:1536
cpu0/cache/index2/ways_of_associativity:10
cpu0/cache/index3/type:Unified
cpu0/cache/index3/level:3
cpu0/cache/index3/size:36864K
cpu0/cache/index3/coherency_line_size:128
cpu0/cache/index3/number_of_sets:1
cpu0/cache/index3/ways_of_associativity:0

(Power6)
# grep -r . /sys/devices/system/cpu/cpu0/topology/ | sed \
  's/\/sys\/devices\/system\/cpu\///'
cpu0/topology/physical_package_id:-1
cpu0/topology/core_id:0
cpu0/topology/thread_siblings:00000003
cpu0/topology/thread_siblings_list:0-1
cpu0/topology/core_siblings:0000000f
cpu0/topology/core_siblings_list:0-3

Nathan Lynch (6):
  kill useless SMT code in prom_hold_cpus
  register_cpu_online should be __cpuinit
  Update cpu_sibling_maps dynamically
  make core sibling information available to userspace
  make core id information available to userspace
  show processor cache information in sysfs

 arch/powerpc/kernel/prom_init.c    |   39 +----
 arch/powerpc/kernel/setup-common.c |   24 ---
 arch/powerpc/kernel/setup_64.c     |    3 -
 arch/powerpc/kernel/smp.c          |  126 ++++++++++++++-
 arch/powerpc/kernel/sysfs.c        |  310 +++++++++++++++++++++++++++++++++++-
 include/asm-powerpc/smp.h          |    2 +
 include/asm-powerpc/topology.h     |    2 +
 7 files changed, 439 insertions(+), 67 deletions(-)

^ permalink raw reply

* [PATCH 1/6] kill useless SMT code in prom_hold_cpus
From: Nathan Lynch @ 2008-07-27  5:24 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1217136295-18693-1-git-send-email-ntl@pobox.com>

I think this code that counts SMT threads and compares against NR_CPUS
is an artifact of pre-powerpc-merge ppc64.  We care about starting
only primary threads in the OF client code.

Signed-off-by: Nathan Lynch <ntl@pobox.com>
---
 arch/powerpc/kernel/prom_init.c |   39 +++------------------------------------
 1 files changed, 3 insertions(+), 36 deletions(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index c4ab219..b72849a 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -205,8 +205,6 @@ static int __initdata mem_reserve_cnt;
 static cell_t __initdata regbuf[1024];
 
 
-#define MAX_CPU_THREADS 2
-
 /*
  * Error results ... some OF calls will return "-1" on error, some
  * will return 0, some will return either. To simplify, here are
@@ -1339,10 +1337,6 @@ static void __init prom_hold_cpus(void)
 	unsigned int reg;
 	phandle node;
 	char type[64];
-	int cpuid = 0;
-	unsigned int interrupt_server[MAX_CPU_THREADS];
-	unsigned int cpu_threads, hw_cpu_num;
-	int propsize;
 	struct prom_t *_prom = &RELOC(prom);
 	unsigned long *spinloop
 		= (void *) LOW_ADDR(__secondary_hold_spinloop);
@@ -1386,7 +1380,6 @@ static void __init prom_hold_cpus(void)
 		reg = -1;
 		prom_getprop(node, "reg", &reg, sizeof(reg));
 
-		prom_debug("\ncpuid        = 0x%x\n", cpuid);
 		prom_debug("cpu hw idx   = 0x%x\n", reg);
 
 		/* Init the acknowledge var which will be reset by
@@ -1395,28 +1388,9 @@ static void __init prom_hold_cpus(void)
 		 */
 		*acknowledge = (unsigned long)-1;
 
-		propsize = prom_getprop(node, "ibm,ppc-interrupt-server#s",
-					&interrupt_server,
-					sizeof(interrupt_server));
-		if (propsize < 0) {
-			/* no property.  old hardware has no SMT */
-			cpu_threads = 1;
-			interrupt_server[0] = reg; /* fake it with phys id */
-		} else {
-			/* We have a threaded processor */
-			cpu_threads = propsize / sizeof(u32);
-			if (cpu_threads > MAX_CPU_THREADS) {
-				prom_printf("SMT: too many threads!\n"
-					    "SMT: found %x, max is %x\n",
-					    cpu_threads, MAX_CPU_THREADS);
-				cpu_threads = 1; /* ToDo: panic? */
-			}
-		}
-
-		hw_cpu_num = interrupt_server[0];
-		if (hw_cpu_num != _prom->cpu) {
+		if (reg != _prom->cpu) {
 			/* Primary Thread of non-boot cpu */
-			prom_printf("%x : starting cpu hw idx %x... ", cpuid, reg);
+			prom_printf("starting cpu hw idx %x... ", reg);
 			call_prom("start-cpu", 3, 0, node,
 				  secondary_hold, reg);
 
@@ -1431,17 +1405,10 @@ static void __init prom_hold_cpus(void)
 		}
 #ifdef CONFIG_SMP
 		else
-			prom_printf("%x : boot cpu     %x\n", cpuid, reg);
+			prom_printf("boot cpu hw idx %x\n", reg);
 #endif /* CONFIG_SMP */
-
-		/* Reserve cpu #s for secondary threads.   They start later. */
-		cpuid += cpu_threads;
 	}
 
-	if (cpuid > NR_CPUS)
-		prom_printf("WARNING: maximum CPUs (" __stringify(NR_CPUS)
-			    ") exceeded: ignoring extras\n");
-
 	prom_debug("prom_hold_cpus: end...\n");
 }
 
-- 
1.5.6.2

^ permalink raw reply related

* Re: [PATCH] of: i2c: improve last resort compatible entry selection
From: Jon Smirl @ 2008-07-27  5:05 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <20080727001119.GB12191@secretlab.ca>

On 7/26/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Jul 14, 2008 at 09:54:37PM +0400, Anton Vorontsov wrote:
>  > Currently of_i2c will select first compatible property as a last resort
>  > option. This isn't best choice though, because generic compatible entries
>  > are listed last, not first. For example, two compatible entries given for
>  > the MCU node:
>  >
>  > "fsl,mc9s08qg8-mpc837xrdb", "fsl,mcu-mpc8349emitx";
>  >
>  > Since no sane driver will ever match specific devices, what we want is
>  > to select most generic option (last). Then driver may call
>  > of_device_is_compatible() if it is really interested in details.
>
>
> I highly suspect that this will actually be a rare condition and that
>  most of the time the driver you want will bind against the first entry
>  in the list (I'm basing this on what discussion I've seen on the list
>  and it seems to me that Jiri does want i2c devices to list the exact set
>  of chips that each driver binds against).

Can we put a loop on request_module() and have it try each one down
the list until something matches? request_module() returns errors, but
I can't tell from the source if one of those errors is "no matching
module found" since it invokes a user space helper.

That would work for this compatible string:
compatible = "atmel,24c32wp", "24c32", "eeprom";

request_module will always fail for the first entry. If you have at24
in your system the second one will succeed. If you have eeprom the
third one works. All of those names are valid in a device tree.

If all three fail, create a device with the last entry since someone
may modprobe the driver in later.

>  I'm inclined to leave it as is for now and instead handle the mpc837x
>  case with the fixup table in this file.  (Actually, this function has
>  been moved to of/base.c; so handle it in that function)
>
>  For the longer term, I think that this whole match method is a stop gap
>  solution until we figure out a tidy way to bind and provide device tree
>  data to i2c, SPI and platform devices.
>
>
>  g.
>
>  >
>  > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>  > ---
>  >
>  > Other options are: start filling the exceptions list, or add  "linux,..."
>  > compatible entry to the device tree.
>  >
>  >  drivers/of/of_i2c.c |   17 +++++++++++------
>  >  1 files changed, 11 insertions(+), 6 deletions(-)
>  >
>  > diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
>  > index b2ccdcb..0d35a0a 100644
>  > --- a/drivers/of/of_i2c.c
>  > +++ b/drivers/of/of_i2c.c
>  > @@ -29,6 +29,7 @@ static int of_find_i2c_driver(struct device_node *node,
>  >       int i, cplen;
>  >       const char *compatible;
>  >       const char *p;
>  > +     const char *last_wcomma = NULL;
>  >
>  >       /* 1. search for exception list entry */
>  >       for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
>  > @@ -45,7 +46,7 @@ static int of_find_i2c_driver(struct device_node *node,
>  >       if (!compatible)
>  >               return -ENODEV;
>  >
>  > -     /* 2. search for linux,<i2c-type> entry */
>  > +     /* 2. search for linux,<i2c-type> entry, or remember last w/ comma */
>  >       p = compatible;
>  >       while (cplen > 0) {
>  >               if (!strncmp(p, "linux,", 6)) {
>  > @@ -54,6 +55,12 @@ static int of_find_i2c_driver(struct device_node *node,
>  >                                   I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  >                               return -ENOMEM;
>  >                       return 0;
>  > +             } else {
>  > +                     const char *comma;
>  > +
>  > +                     comma = strchr(p, ',');
>  > +                     if (comma)
>  > +                             last_wcomma = comma + 1;
>  >               }
>  >
>  >               i = strlen(p) + 1;
>  > @@ -61,12 +68,10 @@ static int of_find_i2c_driver(struct device_node *node,
>  >               cplen -= i;
>  >       }
>  >
>  > -     /* 3. take fist compatible entry and strip manufacturer */
>  > -     p = strchr(compatible, ',');
>  > -     if (!p)
>  > +     /* 3. take last compatible entry w/ comma, manufacturer stripped */
>  > +     if (!last_wcomma)
>  >               return -ENODEV;
>  > -     p++;
>  > -     if (strlcpy(info->type, p, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  > +     if (strlcpy(info->type, last_wcomma, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  >               return -ENOMEM;
>  >       return 0;
>  >  }
>  > --
>  > 1.5.5.4
>


-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply

* Re: [PATCH 1/2] Support internal I2S clock sources on MPC5200
From: Jon Smirl @ 2008-07-27  4:48 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, alsa-devel
In-Reply-To: <20080727020536.GL12191@secretlab.ca>

On 7/26/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue, Jul 22, 2008 at 07:53:49PM -0400, Jon Smirl wrote:
>
> > Support internal I2S clock sources on MPC5200
>  >
>  > Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
>
>
> I'll play with this when I get home.  In the mean time I've got some
>  comments below.  Overall, it looks right to me.
>
>  g.
>
>
>
>  > ---
>  >
>  >  sound/soc/fsl/mpc5200_psc_i2s.c |   58 ++++++++++++++++++++++++++++++++++-----
>  >  sound/soc/fsl/mpc5200_psc_i2s.h |   13 +++++++++
>  >  2 files changed, 64 insertions(+), 7 deletions(-)
>  >  create mode 100644 sound/soc/fsl/mpc5200_psc_i2s.h
>  >
>  > diff --git a/sound/soc/fsl/mpc5200_psc_i2s.c b/sound/soc/fsl/mpc5200_psc_i2s.c
>  > index 8692329..f028f61 100644
>  > --- a/sound/soc/fsl/mpc5200_psc_i2s.c
>  > +++ b/sound/soc/fsl/mpc5200_psc_i2s.c
>  > @@ -23,8 +23,12 @@
>  >
>  >  #include <sysdev/bestcomm/bestcomm.h>
>  >  #include <sysdev/bestcomm/gen_bd.h>
>  > +#include <asm/time.h>
>  > +#include <asm/mpc52xx.h>
>  >  #include <asm/mpc52xx_psc.h>
>  >
>  > +#include "mpc5200_psc_i2s.h"
>  > +
>  >  MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
>  >  MODULE_DESCRIPTION("Freescale MPC5200 PSC in I2S mode ASoC Driver");
>  >  MODULE_LICENSE("GPL");
>  > @@ -93,6 +97,7 @@ struct psc_i2s {
>  >       struct snd_soc_dai dai;
>  >       spinlock_t lock;
>  >       u32 sicr;
>  > +     uint sysclk;
>  >
>  >       /* per-stream data */
>  >       struct psc_i2s_stream playback;
>  > @@ -224,6 +229,7 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream,
>  >  {
>  >       struct snd_soc_pcm_runtime *rtd = substream->private_data;
>  >       struct psc_i2s *psc_i2s = rtd->dai->cpu_dai->private_data;
>  > +     uint bits, framesync, bitclk, value;
>  >       u32 mode;
>  >
>  >       dev_dbg(psc_i2s->dev, "%s(substream=%p) p_size=%i p_bytes=%i"
>  > @@ -235,15 +241,19 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream,
>  >       switch (params_format(params)) {
>  >       case SNDRV_PCM_FORMAT_S8:
>  >               mode = MPC52xx_PSC_SICR_SIM_CODEC_8;
>  > +             bits = 8;
>  >               break;
>  >       case SNDRV_PCM_FORMAT_S16_BE:
>  >               mode = MPC52xx_PSC_SICR_SIM_CODEC_16;
>  > +             bits = 16;
>  >               break;
>  >       case SNDRV_PCM_FORMAT_S24_BE:
>  >               mode = MPC52xx_PSC_SICR_SIM_CODEC_24;
>  > +             bits = 24;
>  >               break;
>  >       case SNDRV_PCM_FORMAT_S32_BE:
>  >               mode = MPC52xx_PSC_SICR_SIM_CODEC_32;
>  > +             bits = 32;
>  >               break;
>  >       default:
>  >               dev_dbg(psc_i2s->dev, "invalid format\n");
>  > @@ -251,7 +261,24 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream,
>  >       }
>  >       out_be32(&psc_i2s->psc_regs->sicr, psc_i2s->sicr | mode);
>  >
>  > -     snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
>  > +     if (psc_i2s->sysclk) {
>  > +             framesync = bits * 2;
>  > +             bitclk = (psc_i2s->sysclk) / (params_rate(params) * framesync);
>  > +
>  > +             /* bitclk field is byte swapped due to mpc5200/b compatibility */
>  > +             value = ((framesync - 1) << 24) |
>  > +                     (((bitclk - 1) & 0xFF) << 16) | ((bitclk - 1) & 0xFF00);
>  > +
>  > +             dev_dbg(psc_i2s->dev, "%s(substream=%p) rate=%i sysclk=%i"
>  > +                     " framesync=%i bitclk=%i reg=%X\n",
>  > +                     __FUNCTION__, substream, params_rate(params), psc_i2s->sysclk,
>  > +                     framesync, bitclk, value);
>  > +
>  > +             out_be32(&psc_i2s->psc_regs->ccr, value);
>  > +             out_8(&psc_i2s->psc_regs->ctur, bits - 1);
>  > +     }
>
>
> This should probably only be executed if the psc is the clock master;
>  just like you've done in psc_i2s_set_sysclk()
>

psc_i2s->sysclk can only get set if there is a clock master. Note that
there are two ways to be a clock master, internal and cellslave.

i2s_set_sysclk() is dynamically called from my fabric driver to adjust
the timebase to match the music.

>
>  > +
>  > +     snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
>  >
>  >       return 0;
>  >  }
>  > @@ -429,9 +456,29 @@ static int psc_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
>  >                             int clk_id, unsigned int freq, int dir)
>  >  {
>  >       struct psc_i2s *psc_i2s = cpu_dai->private_data;
>  > -     dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(cpu_dai=%p, dir=%i)\n",
>  > -                             cpu_dai, dir);
>  > -     return (dir == SND_SOC_CLOCK_IN) ? 0 : -EINVAL;
>  > +     int clkdiv, err;
>  > +     dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(cpu_dai=%p, freq=%u, dir=%i)\n",
>  > +                             cpu_dai, freq, dir);
>  > +     if (dir == SND_SOC_CLOCK_OUT) {
>  > +             psc_i2s->sysclk = freq;
>  > +             if (clk_id == MPC52xx_CLK_CELLSLAVE) {
>  > +                     psc_i2s->sicr |= MPC52xx_PSC_SICR_CELLSLAVE | MPC52xx_PSC_SICR_GENCLK;
>  > +             } else { /* MPC52xx_CLK_INTERNAL */
>  > +                     psc_i2s->sicr &= ~MPC52xx_PSC_SICR_CELLSLAVE;
>  > +                     psc_i2s->sicr |= MPC52xx_PSC_SICR_GENCLK;
>  > +
>  > +                     clkdiv = ppc_proc_freq / freq;
>  > +                     err = ppc_proc_freq % freq;
>  > +                     if (err > freq / 2)
>  > +                             clkdiv++;
>  > +
>  > +                     dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(clkdiv %d freq error=%ldHz)\n",
>  > +                                     clkdiv, (ppc_proc_freq / clkdiv - freq));
>  > +
>  > +                     return mpc52xx_set_psc_clkdiv(psc_i2s->dai.id + 1, clkdiv);
>  > +             }
>  > +     }
>  > +     return 0;
>  >  }
>  >
>  >  /**
>  > @@ -784,9 +831,6 @@ static int __devinit psc_i2s_of_probe(struct of_device *op,
>  >       /* Configure the serial interface mode; defaulting to CODEC8 mode */
>  >       psc_i2s->sicr = MPC52xx_PSC_SICR_DTS1 | MPC52xx_PSC_SICR_I2S |
>  >                       MPC52xx_PSC_SICR_CLKPOL;
>  > -     if (of_get_property(op->node, "fsl,cellslave", NULL))
>  > -             psc_i2s->sicr |= MPC52xx_PSC_SICR_CELLSLAVE |
>  > -                              MPC52xx_PSC_SICR_GENCLK;
>  >       out_be32(&psc_i2s->psc_regs->sicr,
>  >                psc_i2s->sicr | MPC52xx_PSC_SICR_SIM_CODEC_8);
>  >
>  > diff --git a/sound/soc/fsl/mpc5200_psc_i2s.h b/sound/soc/fsl/mpc5200_psc_i2s.h
>  > new file mode 100644
>  > index 0000000..0e0a84e
>  > --- /dev/null
>  > +++ b/sound/soc/fsl/mpc5200_psc_i2s.h
>  > @@ -0,0 +1,13 @@
>  > +/*
>  > + * Freescale MPC5200 PSC in I2S mode
>  > + * ALSA SoC Digital Audio Interface (DAI) driver
>  > + *
>  > + */
>  > +
>  > +#ifndef __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__
>  > +#define __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__
>  > +
>  > +#define MPC52xx_CLK_INTERNAL 0
>  > +#define MPC52xx_CLK_CELLSLAVE 1
>  > +
>  > +#endif /* __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__ */
>  >
>
> > _______________________________________________
>  > Linuxppc-dev mailing list
>  > Linuxppc-dev@ozlabs.org
>  > https://ozlabs.org/mailman/listinfo/linuxppc-dev
>


-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply

* Re: [PATCH 2/2] Allow a custom ASOC machine driver with soc-of-simple
From: Jon Smirl @ 2008-07-27  4:44 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, alsa-devel
In-Reply-To: <20080727011206.GD12191@secretlab.ca>

On 7/26/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue, Jul 22, 2008 at 07:53:51PM -0400, Jon Smirl wrote:
>
> > Allow a custom ASOC machine driver with soc-of-simple
>  >
>  > Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
>
>
> Sorry I didn't respond earlier.  OLS kept me pretty distracted.
>
>  Need a more detailed comment block about how your changing things and
>  why.
>
>
>  > ---
>  >
>  >  include/sound/soc-of-simple.h |    2 ++
>  >  sound/soc/fsl/soc-of-simple.c |   26 +++++++++++++++++++++-----
>  >  2 files changed, 23 insertions(+), 5 deletions(-)
>  >
>  > diff --git a/include/sound/soc-of-simple.h b/include/sound/soc-of-simple.h
>  > index 696fc51..1e83f2f 100644
>  > --- a/include/sound/soc-of-simple.h
>  > +++ b/include/sound/soc-of-simple.h
>  > @@ -18,4 +18,6 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform,
>  >                                struct device_node *node,
>  >                                struct snd_soc_dai *cpu_dai);
>  >
>  > +void of_snd_soc_register_machine(char *name, struct snd_soc_ops *ops);
>  > +
>  >  #endif /* _INCLUDE_SOC_OF_H_ */
>  > diff --git a/sound/soc/fsl/soc-of-simple.c b/sound/soc/fsl/soc-of-simple.c
>  > index 0382fda..dd2fa23 100644
>  > --- a/sound/soc/fsl/soc-of-simple.c
>  > +++ b/sound/soc/fsl/soc-of-simple.c
>  > @@ -38,8 +38,8 @@ struct of_snd_soc_device {
>  >       struct device_node *codec_node;
>  >  };
>  >
>  > -static struct snd_soc_ops of_snd_soc_ops = {
>  > -};
>  > +static struct snd_soc_ops *machine_ops = NULL;
>  > +static char *machine_name = NULL;
>
>
> Doing this prevents multiple instances of this machine driver (which is
>  exactly what I have on my board).  To register a machine driver it
>  creates a 3-way bind condition instead of the existing 2-way one.  Right
>  now it is easy to match up codec and platform drivers because a common
>  key is available in the device tree.
>
>  Alternately, it might be okay to only allow for a single machine driver
>  that is able to create multiple sound card instances, but this current
>  code just uses the same name and ops for each registered device.
>
We need to call them fabric drivers since we already have machine
drivers in arch/.... It is too confusing.

My fabric driver was getting probed after the rest of ASOC bound,
that's why I had to add the third condition.

An important feature for me is the ability to compile in multiple
fabric drivers and then get the right one selected based on the
machine name in the device tree.  I'm doing via my machine driver.

Could we dynamically build an OF fabric device entry with a compatible
string like this: compatible="machinename-fabric,generic-fabric"
Now a hard requirement for a fabric driver is ok since one of those
two will load for sure. generic-fabric is just a do nothing driver
that is always built in.

/* Trigger the platform specific ASOC driver to load */
static struct platform_device platform = {
        .name           = "dspeak01-fabric",
        .id             = -1,
};

static struct platform_device *devices[] = {
        &platform,
};

static void __init digispeaker_declare_platform_devices(void)
{
        mpc52xx_declare_of_platform_devices();
        platform_add_devices(&devices[0], ARRAY_SIZE(devices));
}

define_machine(digispeaker_platform) {
        .name           = "dspeak01",
        .probe          = digispeaker_probe,
        .setup_arch     = digispeaker_setup_arch,
        .init           = digispeaker_declare_platform_devices,
        .init_IRQ       = mpc52xx_init_irq,
        .get_irq        = mpc52xx_get_irq,
        .restart        = mpc52xx_restart,
        .calibrate_decr = generic_calibrate_decr,
};


>
>  >
>  >  static struct of_snd_soc_device *
>  >  of_snd_soc_get_device(struct device_node *codec_node)
>  > @@ -61,7 +61,7 @@ of_snd_soc_get_device(struct device_node *codec_node)
>  >       of_soc->machine.dai_link = &of_soc->dai_link;
>  >       of_soc->machine.num_links = 1;
>  >       of_soc->device.machine = &of_soc->machine;
>  > -     of_soc->dai_link.ops = &of_snd_soc_ops;
>  > +     of_soc->dai_link.ops = machine_ops;
>  >       list_add(&of_soc->list, &of_snd_soc_device_list);
>  >
>  >       return of_soc;
>  > @@ -74,7 +74,7 @@ static void of_snd_soc_register_device(struct of_snd_soc_device *of_soc)
>  >
>  >       /* Only register the device if both the codec and platform have
>  >        * been registered */
>  > -     if ((!of_soc->device.codec_data) || (!of_soc->platform_node))
>  > +     if ((!of_soc->device.codec_data) || (!of_soc->platform_node) || !machine_name)
>
>
> I'm not thrilled with the hard requirement for a machine driver, but I
>  see what you're trying to do.  I want to find a clean way to trigger
>  this behaviour in the device tree without resorting to encoding linux
>  internal details into the data.  Need to think about this more.
>
>
>  >               return;
>  >
>  >       pr_info("platform<-->codec match achieved; registering machine\n");
>  > @@ -159,7 +159,7 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform,
>  >       of_soc->platform_node = node;
>  >       of_soc->dai_link.cpu_dai = cpu_dai;
>  >       of_soc->device.platform = platform;
>  > -     of_soc->machine.name = of_soc->dai_link.cpu_dai->name;
>  > +     of_soc->machine.name = machine_name;
>
>
> As mentioned above, either there needs to be multiple machine drivers
>  or the ability to change the name for each platform--codec pair.
>
>
>  >
>  >       /* Now try to register the SoC device */
>  >       of_snd_soc_register_device(of_soc);
>  > @@ -169,3 +169,19 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform,
>  >       return rc;
>  >  }
>  >  EXPORT_SYMBOL_GPL(of_snd_soc_register_platform);
>  > +
>  > +void of_snd_soc_register_machine(char *name, struct snd_soc_ops *ops)
>  > +{
>  > +     struct of_snd_soc_device *of_soc;
>  > +
>  > +     machine_name = name;
>  > +     machine_ops = ops;
>  > +
>  > +     list_for_each_entry(of_soc, &of_snd_soc_device_list, list) {
>  > +             of_soc->dai_link.ops = machine_ops;
>  > +             of_soc->machine.name = machine_name;
>  > +             of_snd_soc_register_device(of_soc);
>  > +     }
>  > +
>  > +}
>
>
> You need to hold the mutex when manipulating the list.
>


-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply

* Re: [PATCH 1/2] Support internal I2S clock sources on MPC5200
From: Grant Likely @ 2008-07-27  2:05 UTC (permalink / raw)
  To: Jon Smirl; +Cc: linuxppc-dev, alsa-devel
In-Reply-To: <20080722235349.11648.90112.stgit@terra>

On Tue, Jul 22, 2008 at 07:53:49PM -0400, Jon Smirl wrote:
> Support internal I2S clock sources on MPC5200
> 
> Signed-off-by: Jon Smirl <jonsmirl@gmail.com>

I'll play with this when I get home.  In the mean time I've got some
comments below.  Overall, it looks right to me.

g.


> ---
> 
>  sound/soc/fsl/mpc5200_psc_i2s.c |   58 ++++++++++++++++++++++++++++++++++-----
>  sound/soc/fsl/mpc5200_psc_i2s.h |   13 +++++++++
>  2 files changed, 64 insertions(+), 7 deletions(-)
>  create mode 100644 sound/soc/fsl/mpc5200_psc_i2s.h
> 
> diff --git a/sound/soc/fsl/mpc5200_psc_i2s.c b/sound/soc/fsl/mpc5200_psc_i2s.c
> index 8692329..f028f61 100644
> --- a/sound/soc/fsl/mpc5200_psc_i2s.c
> +++ b/sound/soc/fsl/mpc5200_psc_i2s.c
> @@ -23,8 +23,12 @@
>  
>  #include <sysdev/bestcomm/bestcomm.h>
>  #include <sysdev/bestcomm/gen_bd.h>
> +#include <asm/time.h>
> +#include <asm/mpc52xx.h>
>  #include <asm/mpc52xx_psc.h>
>  
> +#include "mpc5200_psc_i2s.h"
> +
>  MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
>  MODULE_DESCRIPTION("Freescale MPC5200 PSC in I2S mode ASoC Driver");
>  MODULE_LICENSE("GPL");
> @@ -93,6 +97,7 @@ struct psc_i2s {
>  	struct snd_soc_dai dai;
>  	spinlock_t lock;
>  	u32 sicr;
> +	uint sysclk;
>  
>  	/* per-stream data */
>  	struct psc_i2s_stream playback;
> @@ -224,6 +229,7 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream,
>  {
>  	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>  	struct psc_i2s *psc_i2s = rtd->dai->cpu_dai->private_data;
> +	uint bits, framesync, bitclk, value;
>  	u32 mode;
>  
>  	dev_dbg(psc_i2s->dev, "%s(substream=%p) p_size=%i p_bytes=%i"
> @@ -235,15 +241,19 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream,
>  	switch (params_format(params)) {
>  	case SNDRV_PCM_FORMAT_S8:
>  		mode = MPC52xx_PSC_SICR_SIM_CODEC_8;
> +		bits = 8;
>  		break;
>  	case SNDRV_PCM_FORMAT_S16_BE:
>  		mode = MPC52xx_PSC_SICR_SIM_CODEC_16;
> +		bits = 16;
>  		break;
>  	case SNDRV_PCM_FORMAT_S24_BE:
>  		mode = MPC52xx_PSC_SICR_SIM_CODEC_24;
> +		bits = 24;
>  		break;
>  	case SNDRV_PCM_FORMAT_S32_BE:
>  		mode = MPC52xx_PSC_SICR_SIM_CODEC_32;
> +		bits = 32;
>  		break;
>  	default:
>  		dev_dbg(psc_i2s->dev, "invalid format\n");
> @@ -251,7 +261,24 @@ static int psc_i2s_hw_params(struct snd_pcm_substream *substream,
>  	}
>  	out_be32(&psc_i2s->psc_regs->sicr, psc_i2s->sicr | mode);
>  
> -	snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
> +	if (psc_i2s->sysclk) {
> +		framesync = bits * 2;
> +		bitclk = (psc_i2s->sysclk) / (params_rate(params) * framesync);
> +		
> +		/* bitclk field is byte swapped due to mpc5200/b compatibility */
> +		value = ((framesync - 1) << 24) |
> +			(((bitclk - 1) & 0xFF) << 16) | ((bitclk - 1) & 0xFF00);
> +		
> +		dev_dbg(psc_i2s->dev, "%s(substream=%p) rate=%i sysclk=%i"
> +			" framesync=%i bitclk=%i reg=%X\n",
> +			__FUNCTION__, substream, params_rate(params), psc_i2s->sysclk,
> +			framesync, bitclk, value);
> +		
> +		out_be32(&psc_i2s->psc_regs->ccr, value);
> +		out_8(&psc_i2s->psc_regs->ctur, bits - 1);
> +	}

This should probably only be executed if the psc is the clock master;
just like you've done in psc_i2s_set_sysclk()

> +	
> +  	snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
>  
>  	return 0;
>  }
> @@ -429,9 +456,29 @@ static int psc_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
>  			      int clk_id, unsigned int freq, int dir)
>  {
>  	struct psc_i2s *psc_i2s = cpu_dai->private_data;
> -	dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(cpu_dai=%p, dir=%i)\n",
> -				cpu_dai, dir);
> -	return (dir == SND_SOC_CLOCK_IN) ? 0 : -EINVAL;
> +	int clkdiv, err; 
> +	dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(cpu_dai=%p, freq=%u, dir=%i)\n",
> +				cpu_dai, freq, dir);
> +	if (dir == SND_SOC_CLOCK_OUT) {
> +		psc_i2s->sysclk = freq;
> +		if (clk_id == MPC52xx_CLK_CELLSLAVE) {
> +			psc_i2s->sicr |= MPC52xx_PSC_SICR_CELLSLAVE | MPC52xx_PSC_SICR_GENCLK;
> +		} else { /* MPC52xx_CLK_INTERNAL */
> +			psc_i2s->sicr &= ~MPC52xx_PSC_SICR_CELLSLAVE;
> +			psc_i2s->sicr |= MPC52xx_PSC_SICR_GENCLK;
> +
> +			clkdiv = ppc_proc_freq / freq;
> +			err = ppc_proc_freq % freq;
> +			if (err > freq / 2)
> +				clkdiv++;
> +
> +			dev_dbg(psc_i2s->dev, "psc_i2s_set_sysclk(clkdiv %d freq error=%ldHz)\n",
> +					clkdiv, (ppc_proc_freq / clkdiv - freq));
> +				
> +			return mpc52xx_set_psc_clkdiv(psc_i2s->dai.id + 1, clkdiv); 
> +		}
> +	}
> +	return 0;
>  }
>  
>  /**
> @@ -784,9 +831,6 @@ static int __devinit psc_i2s_of_probe(struct of_device *op,
>  	/* Configure the serial interface mode; defaulting to CODEC8 mode */
>  	psc_i2s->sicr = MPC52xx_PSC_SICR_DTS1 | MPC52xx_PSC_SICR_I2S |
>  			MPC52xx_PSC_SICR_CLKPOL;
> -	if (of_get_property(op->node, "fsl,cellslave", NULL))
> -		psc_i2s->sicr |= MPC52xx_PSC_SICR_CELLSLAVE |
> -				 MPC52xx_PSC_SICR_GENCLK;
>  	out_be32(&psc_i2s->psc_regs->sicr,
>  		 psc_i2s->sicr | MPC52xx_PSC_SICR_SIM_CODEC_8);
>  
> diff --git a/sound/soc/fsl/mpc5200_psc_i2s.h b/sound/soc/fsl/mpc5200_psc_i2s.h
> new file mode 100644
> index 0000000..0e0a84e
> --- /dev/null
> +++ b/sound/soc/fsl/mpc5200_psc_i2s.h
> @@ -0,0 +1,13 @@
> +/*
> + * Freescale MPC5200 PSC in I2S mode
> + * ALSA SoC Digital Audio Interface (DAI) driver
> + *
> + */
> + 
> +#ifndef __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__
> +#define __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__
> +
> +#define MPC52xx_CLK_INTERNAL 0
> +#define MPC52xx_CLK_CELLSLAVE 1
> +
> +#endif /* __SOUND_SOC_FSL_MPC52xx_PSC_I2S_H__ */
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

^ permalink raw reply

* Re: [PATCH v3 5/5] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver
From: Grant Likely @ 2008-07-27  0:20 UTC (permalink / raw)
  To: Jon Smirl; +Cc: linuxppc-dev, dbrownell, spi-devel-general
In-Reply-To: <9e4733910807140621g69707c46tc23d26e9cf030c5c@mail.gmail.com>

On Mon, Jul 14, 2008 at 09:21:08AM -0400, Jon Smirl wrote:
> On 7/12/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> > From: Grant Likely <grant.likely@secretlab.ca>
> >
> >  Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> >  SoC.
> 
> Can you adjust the existing PSC based SPI driver to use this device
> tree code? It will be confusing if there are two different ways to do
> SPI on the mpc5200.

Yes, I'll do that and post the patch

g.

^ permalink raw reply

* Re: [PATCH 2/2] Allow a custom ASOC machine driver with soc-of-simple
From: Grant Likely @ 2008-07-27  1:12 UTC (permalink / raw)
  To: Jon Smirl; +Cc: linuxppc-dev, alsa-devel
In-Reply-To: <20080722235351.11648.56387.stgit@terra>

On Tue, Jul 22, 2008 at 07:53:51PM -0400, Jon Smirl wrote:
> Allow a custom ASOC machine driver with soc-of-simple
> 
> Signed-off-by: Jon Smirl <jonsmirl@gmail.com>

Sorry I didn't respond earlier.  OLS kept me pretty distracted.

Need a more detailed comment block about how your changing things and
why.

> ---
> 
>  include/sound/soc-of-simple.h |    2 ++
>  sound/soc/fsl/soc-of-simple.c |   26 +++++++++++++++++++++-----
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sound/soc-of-simple.h b/include/sound/soc-of-simple.h
> index 696fc51..1e83f2f 100644
> --- a/include/sound/soc-of-simple.h
> +++ b/include/sound/soc-of-simple.h
> @@ -18,4 +18,6 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform,
>  				 struct device_node *node,
>  				 struct snd_soc_dai *cpu_dai);
>  
> +void of_snd_soc_register_machine(char *name, struct snd_soc_ops *ops);
> +
>  #endif /* _INCLUDE_SOC_OF_H_ */
> diff --git a/sound/soc/fsl/soc-of-simple.c b/sound/soc/fsl/soc-of-simple.c
> index 0382fda..dd2fa23 100644
> --- a/sound/soc/fsl/soc-of-simple.c
> +++ b/sound/soc/fsl/soc-of-simple.c
> @@ -38,8 +38,8 @@ struct of_snd_soc_device {
>  	struct device_node *codec_node;
>  };
>  
> -static struct snd_soc_ops of_snd_soc_ops = {
> -};
> +static struct snd_soc_ops *machine_ops = NULL;
> +static char *machine_name = NULL;

Doing this prevents multiple instances of this machine driver (which is
exactly what I have on my board).  To register a machine driver it
creates a 3-way bind condition instead of the existing 2-way one.  Right
now it is easy to match up codec and platform drivers because a common
key is available in the device tree.

Alternately, it might be okay to only allow for a single machine driver
that is able to create multiple sound card instances, but this current
code just uses the same name and ops for each registered device.

>  
>  static struct of_snd_soc_device *
>  of_snd_soc_get_device(struct device_node *codec_node)
> @@ -61,7 +61,7 @@ of_snd_soc_get_device(struct device_node *codec_node)
>  	of_soc->machine.dai_link = &of_soc->dai_link;
>  	of_soc->machine.num_links = 1;
>  	of_soc->device.machine = &of_soc->machine;
> -	of_soc->dai_link.ops = &of_snd_soc_ops;
> +	of_soc->dai_link.ops = machine_ops;
>  	list_add(&of_soc->list, &of_snd_soc_device_list);
>  
>  	return of_soc;
> @@ -74,7 +74,7 @@ static void of_snd_soc_register_device(struct of_snd_soc_device *of_soc)
>  
>  	/* Only register the device if both the codec and platform have
>  	 * been registered */
> -	if ((!of_soc->device.codec_data) || (!of_soc->platform_node))
> +	if ((!of_soc->device.codec_data) || (!of_soc->platform_node) || !machine_name)

I'm not thrilled with the hard requirement for a machine driver, but I
see what you're trying to do.  I want to find a clean way to trigger
this behaviour in the device tree without resorting to encoding linux
internal details into the data.  Need to think about this more.

>  		return;
>  
>  	pr_info("platform<-->codec match achieved; registering machine\n");
> @@ -159,7 +159,7 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform,
>  	of_soc->platform_node = node;
>  	of_soc->dai_link.cpu_dai = cpu_dai;
>  	of_soc->device.platform = platform;
> -	of_soc->machine.name = of_soc->dai_link.cpu_dai->name;
> +	of_soc->machine.name = machine_name;

As mentioned above, either there needs to be multiple machine drivers
or the ability to change the name for each platform--codec pair.

>  
>  	/* Now try to register the SoC device */
>  	of_snd_soc_register_device(of_soc);
> @@ -169,3 +169,19 @@ int of_snd_soc_register_platform(struct snd_soc_platform *platform,
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(of_snd_soc_register_platform);
> +
> +void of_snd_soc_register_machine(char *name, struct snd_soc_ops *ops)
> +{
> +	struct of_snd_soc_device *of_soc;
> +	
> +	machine_name = name;
> +	machine_ops = ops;
> +	
> +	list_for_each_entry(of_soc, &of_snd_soc_device_list, list) {
> +		of_soc->dai_link.ops = machine_ops;
> +		of_soc->machine.name = machine_name;
> +		of_snd_soc_register_device(of_soc);
> +	}
> +
> +}

You need to hold the mutex when manipulating the list.

^ permalink raw reply

* Re: [PATCH 1/2] leds: make the default trigger name const
From: Grant Likely @ 2008-07-27  2:08 UTC (permalink / raw)
  To: Trent Piepho; +Cc: Stephen Rothwell, linux-kernel, linuxppc-dev, Richard Purdie
In-Reply-To: <1217019705-24244-1-git-send-email-tpiepho@freescale.com>

On Fri, Jul 25, 2008 at 02:01:44PM -0700, Trent Piepho wrote:
> The default_trigger fields of struct gpio_led and thus struct led_classdev
> are pretty much always assigned from a string literal, which means the
> string can't be modified.  Which is fine, since there is no reason to
> modify the string and in fact it never is.
> 
> But they should be marked const to prevent such code from being added, to
> prevent warnings if -Wwrite-strings is used and when assigned from a
> constant string other than a string literal (which produces a warning under
> current kernel compiler flags), and for general good coding practices.
> 
> Signed-off-by: Trent Piepho <tpiepho@freescale.com>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>  include/linux/leds.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 519df72..defe693 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -48,7 +48,7 @@ struct led_classdev {
>  
>  	struct device		*dev;
>  	struct list_head	 node;			/* LED Device list */
> -	char			*default_trigger;	/* Trigger to use */
> +	const char		*default_trigger;	/* Trigger to use */
>  
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	/* Protects the trigger data below */
> @@ -121,7 +121,7 @@ extern void ledtrig_ide_activity(void);
>  /* For the leds-gpio driver */
>  struct gpio_led {
>  	const char *name;
> -	char *default_trigger;
> +	const char *default_trigger;
>  	unsigned 	gpio;
>  	u8 		active_low;
>  };
> -- 
> 1.5.4.3
> 

^ permalink raw reply

* Re: [alsa-devel] [PATCH 2/2] Allow a custom ASOC machine driver with soc-of-simple
From: Grant Likely @ 2008-07-27  1:14 UTC (permalink / raw)
  To: Jon Smirl, linuxppc-dev, alsa-devel
In-Reply-To: <20080723151431.GA22926@sirena.org.uk>

On Wed, Jul 23, 2008 at 04:14:32PM +0100, Mark Brown wrote:
> On Wed, Jul 23, 2008 at 10:09:01AM -0400, Jon Smirl wrote:
> > On 7/23/08, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> 
> > > ...this doesn't just allow a custom machine driver, it requires that
> > >  something configures at least the machine name.  That's not a problem
> > >  from the ASoC point of view but possibly from the PowerPC side?
> 
> > You have to configure at least the name. Otherwise if the machine
> > driver is the last to register, how do you know to hold off the final
> > registration and wait for the machine driver to appear?
> 
> I understand why you have made this change but it's a substantial change
> which should at least be documented in the changelog (I'd expect to see
> some mention of how this is supposed to be configured, for example).
> I'd also expect something to handle the existing user.
> 
> Like I said, I'm not entirely sure that you're supposed to be using this
> driver if you want a machine driver: this is a machine driver and I'm
> not sure if it's supposed to cover all cases or not.  Grant?

As I replied to the patch itself, I'm not thrilled with it, but I don't
have an immediate solution.  I do want this behaviour to at least
trigger on something in the device tree; not to become a hard
requirement.

g.

^ permalink raw reply

* Re: [PATCH 2/2] leds: Support OpenFirmware led bindings
From: Grant Likely @ 2008-07-27  2:21 UTC (permalink / raw)
  To: Trent Piepho; +Cc: Stephen Rothwell, linux-kernel, linuxppc-dev, Richard Purdie
In-Reply-To: <1217019705-24244-2-git-send-email-tpiepho@freescale.com>

On Fri, Jul 25, 2008 at 02:01:45PM -0700, Trent Piepho wrote:
> Add bindings to support LEDs defined as of_platform devices in addition to
> the existing bindings for platform devices.

(adding devicetree-discuss@ozlabs.org to the cc list)

> New options in Kconfig allow the platform binding code and/or the
> of_platform code to be turned on.  The of_platform code is of course only
> available on archs that have OF support.
> 
> The existing probe and remove methods are refactored to use new functions
> create_gpio_led(), to create and register one led, and delete_gpio_led(),
> to unregister and free one led.  The new probe and remove methods for the
> of_platform driver can then share most of the common probe and remove code
> with the platform driver.
> 
> The suspend and resume methods aren't shared, but they are very short.  The
> actual led driving code is the same for LEDs created by either binding.

I like this approach.  I think it is a good pattern.

> The OF bindings are based on patch by Anton Vorontsov
> <avorontsov@ru.mvista.com>.  They have been extended to allow multiple LEDs
> per device.
> 
> Signed-off-by: Trent Piepho <tpiepho@freescale.com>
> ---
>  Documentation/powerpc/dts-bindings/gpio/led.txt |   44 ++++-
>  drivers/leds/Kconfig                            |   21 ++-
>  drivers/leds/leds-gpio.c                        |  225 ++++++++++++++++++-----
>  3 files changed, 236 insertions(+), 54 deletions(-)
> 
> diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt b/Documentation/powerpc/dts-bindings/gpio/led.txt
> index ff51f4c..ed01297 100644
> --- a/Documentation/powerpc/dts-bindings/gpio/led.txt
> +++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
> @@ -1,15 +1,39 @@
> -LED connected to GPIO
> +LEDs connected to GPIO lines
>  
>  Required properties:
> -- compatible : should be "gpio-led".
> -- label : (optional) the label for this LED. If omitted, the label is
> -  taken from the node name (excluding the unit address).
> -- gpios : should specify LED GPIO.
> +- compatible : should be "gpio-leds".
>  
> -Example:
> +Each LED is represented as a sub-node of the gpio-leds device.  Each
> +node's name represents the name of the corresponding LED.
>  
> -led@0 {
> -	compatible = "gpio-led";
> -	label = "hdd";
> -	gpios = <&mcu_pio 0 1>;
> +LED node properties:
> +- gpios :  Should specify the LED GPIO.

Question: it is possible/desirable for a single LED to be assigned
multiple GPIO pins?  Say, for a tri-color LED?  (I'm fishing for
opinions; I really don't know if it would be a good idea or not)

> +- label :  (optional) The label for this LED.  If omitted, the label
> +  is taken from the node name (excluding the unit address).
> +- function :  (optional) This parameter, if present, is a string
> +  defining the function of the LED.  It can be used to put the LED
> +  under software control, e.g. Linux LED triggers like "heartbeat",
> +  "ide-disk", and "timer".  Or it could be used to attach a hardware
> +  signal to the LED, e.g. a SoC that can configured to put a SATA
> +  activity signal on a GPIO line.

This makes me nervous.  It exposes Linux internal implementation details
into the device tree data.  If you want to have a property that
describes the LED usage, then the possible values and meanings should be
documented here.

> +	memset(&led, 0, sizeof(led));
> +	for_each_child_of_node(np, child) {
> +		led.gpio = of_get_gpio(child, 0);
> +		led.name = of_get_property(child, "label", NULL) ? : child->name;

> +		led.default_trigger =
> +			of_get_property(child, "linux,default-trigger", NULL);

This isn't in the documented binding.  I assume that you mean 'function'
here?

Otherwise, the code looks pretty good to me.

g.

^ permalink raw reply

* Re: [PATCH] of: i2c: improve last resort compatible entry selection
From: Grant Likely @ 2008-07-27  0:11 UTC (permalink / raw)
  To: Anton Vorontsov; +Cc: linuxppc-dev
In-Reply-To: <20080714175437.GA5230@polina.dev.rtsoft.ru>

On Mon, Jul 14, 2008 at 09:54:37PM +0400, Anton Vorontsov wrote:
> Currently of_i2c will select first compatible property as a last resort
> option. This isn't best choice though, because generic compatible entries
> are listed last, not first. For example, two compatible entries given for
> the MCU node:
> 
> "fsl,mc9s08qg8-mpc837xrdb", "fsl,mcu-mpc8349emitx";
> 
> Since no sane driver will ever match specific devices, what we want is
> to select most generic option (last). Then driver may call
> of_device_is_compatible() if it is really interested in details.
 
I highly suspect that this will actually be a rare condition and that
most of the time the driver you want will bind against the first entry
in the list (I'm basing this on what discussion I've seen on the list
and it seems to me that Jiri does want i2c devices to list the exact set
of chips that each driver binds against).

I'm inclined to leave it as is for now and instead handle the mpc837x
case with the fixup table in this file.  (Actually, this function has
been moved to of/base.c; so handle it in that function)

For the longer term, I think that this whole match method is a stop gap
solution until we figure out a tidy way to bind and provide device tree
data to i2c, SPI and platform devices.

g.

> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> 
> Other options are: start filling the exceptions list, or add  "linux,..."
> compatible entry to the device tree.
> 
>  drivers/of/of_i2c.c |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> index b2ccdcb..0d35a0a 100644
> --- a/drivers/of/of_i2c.c
> +++ b/drivers/of/of_i2c.c
> @@ -29,6 +29,7 @@ static int of_find_i2c_driver(struct device_node *node,
>  	int i, cplen;
>  	const char *compatible;
>  	const char *p;
> +	const char *last_wcomma = NULL;
>  
>  	/* 1. search for exception list entry */
>  	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
> @@ -45,7 +46,7 @@ static int of_find_i2c_driver(struct device_node *node,
>  	if (!compatible)
>  		return -ENODEV;
>  
> -	/* 2. search for linux,<i2c-type> entry */
> +	/* 2. search for linux,<i2c-type> entry, or remember last w/ comma */
>  	p = compatible;
>  	while (cplen > 0) {
>  		if (!strncmp(p, "linux,", 6)) {
> @@ -54,6 +55,12 @@ static int of_find_i2c_driver(struct device_node *node,
>  				    I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  				return -ENOMEM;
>  			return 0;
> +		} else {
> +			const char *comma;
> +
> +			comma = strchr(p, ',');
> +			if (comma)
> +				last_wcomma = comma + 1;
>  		}
>  
>  		i = strlen(p) + 1;
> @@ -61,12 +68,10 @@ static int of_find_i2c_driver(struct device_node *node,
>  		cplen -= i;
>  	}
>  
> -	/* 3. take fist compatible entry and strip manufacturer */
> -	p = strchr(compatible, ',');
> -	if (!p)
> +	/* 3. take last compatible entry w/ comma, manufacturer stripped */
> +	if (!last_wcomma)
>  		return -ENODEV;
> -	p++;
> -	if (strlcpy(info->type, p, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
> +	if (strlcpy(info->type, last_wcomma, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  		return -ENOMEM;
>  	return 0;
>  }
> -- 
> 1.5.5.4

^ permalink raw reply

* Re: [BUG] fec_mpc52xx: Don't call mpc52xx_fec_reset() in ISR
From: Grant Likely @ 2008-07-27  1:31 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linuxppc-dev, domen.puncer
In-Reply-To: <20080710123909.GA4275@pengutronix.de>

On Thu, Jul 10, 2008 at 02:39:09PM +0200, Wolfram Sang wrote:
> Hello,
> 
> today, I was debugging a kernel crash on a board with a MPC5200B using
> 2.6.26-rc9. I found the following code in drivers/net/fec_mpc52xx.c:
> 
> static irqreturn_t mpc52xx_fec_interrupt(int irq, void *dev_id)
> {
> [...]
> 	/* on fifo error, soft-reset fec */
> 	if (ievent & (FEC_IEVENT_RFIFO_ERROR | FEC_IEVENT_XFIFO_ERROR)) {
> 
> 		if (net_ratelimit() && (ievent & FEC_IEVENT_RFIFO_ERROR))
> 			dev_warn(&dev->dev, "FEC_IEVENT_RFIFO_ERROR\n");
> 		if (net_ratelimit() && (ievent & FEC_IEVENT_XFIFO_ERROR))
> 			dev_warn(&dev->dev, "FEC_IEVENT_XFIFO_ERROR\n");
> 
> 		mpc52xx_fec_reset(dev);
> 
> 		netif_wake_queue(dev);
> 		return IRQ_HANDLED;
> 	}
> [...]
> }
> 
> Calling mpc52xx_fec_reset() from interrupt context is bad, at least
> because
> 
> a) it calls phy_write, which contains BUG_ON(in_interrupt())
> b) it calls mpc52xx_fec_hw_init, which has a delay-loop to check
>    if the reset was successful (1..50 us)
> 
> I assume the proper thing to do is to set a flag in the ISR and handle
> the soft reset later in some other context. Having never dealt with the
> network core and its drivers so far, I am not sure which place would be
> the right one to perform the soft reset. To not make things worse, I
> hope people with more insight to network stuff can deliver a suitable
> solution to this problem.
> 
> All the best,
> 
>    Wolfram

Hi Wolfram,

I'm not an expert on the FEC driver, but I'll take a look when I get back
home to Calgary.  There has also been other churn in this area and I
need to get myself up to speed.

g.

^ permalink raw reply

* Re: XSysAce: Queue is plugged
From: Grant Likely @ 2008-07-27  1:36 UTC (permalink / raw)
  To: Joachim Meyer; +Cc: linuxppc-embedded
In-Reply-To: <870343913@web.de>

On Thu, Jul 10, 2008 at 04:49:21PM +0200, Joachim Meyer wrote:
> Hi
> 
> XSysAce: Queue is plugged
> Can someone say me what this means and how to get rid of it?
> I use a 2.6.23 xilinx Kernel on a XUPv2p. In the Kernel config-menu are 2 sysace drivers:
> < > Xilinx SystemACE support
> and 
> < > Xilinx SystemACE support (old driver)
> 
> I'm using the old driver because if I try to compile with the other it ends in this:
> 

The new driver compiles fine in 2.6.26.  I think it is more robust than
the old sysace driver.  See if you can port forward to a more recent
kernel; or you could try plucking the current driver out of Linus' tree
and trying it on your older kernel (but you won't get much support that
way).

g.

^ permalink raw reply

* Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Grant Likely @ 2008-07-27  1:27 UTC (permalink / raw)
  To: Timur Tabi; +Cc: Scott Wood, Linuxppc-dev
In-Reply-To: <4889FD1D.4010804@freescale.com>

On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
> Wolfgang Grandegger wrote:
> 
> > I know but we still need an algorithm for MPC52xx and MPC82xx as well.
> 
> That's true, but I still think hard-coding values of DFSR and FDR in the device
> tree is not a good way to do this.

I agree, it should encode real frequencies, not raw register values.

g.

^ permalink raw reply

* Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Grant Likely @ 2008-07-27  1:25 UTC (permalink / raw)
  To: Wolfgang Grandegger; +Cc: Scott Wood, Linuxppc-dev
In-Reply-To: <4889F299.3080403@grandegger.com>

On Fri, Jul 25, 2008 at 05:34:49PM +0200, Wolfgang Grandegger wrote:
> Grant Likely wrote:
>> On Fri, Jul 25, 2008 at 5:04 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>> I was also thinking to just overtake the U-Boot settings if fdt and dfsrr is
>>> not defined for the I2C node (instead of the debatable default values).
>>
>> This is a perfectly valid option.  Personally, I'd prefer it encoded
>> in the device tree, but if it looks like a valid speed has already
>> been programmed in then I'm cool with the driver just preserving that.
>>  If it turns out to causes problems the we can always change the code
>> to be more conservative later.
>
> How should the Linux driver decide if the registers have been already  
> set by the boot-loader? The reset-values might be good as well.  
> Therefore, if "clock-frequency" is not specified, the driver may simply  
> not touch the fdr and dfsr registers (overtaking the values from the  
> boot-loader).

I'm okay with that.  If the property isn't present then it is probably
okay to just assume that it is usable.

g.

^ permalink raw reply

* Re: Cleanup for i2c driver changes.
From: Grant Likely @ 2008-07-26 18:28 UTC (permalink / raw)
  To: Sean MacLennan; +Cc: linuxppc-dev
In-Reply-To: <20080725214510.400c6ce8@lappy.seanm.ca>

On Fri, Jul 25, 2008 at 09:45:10PM -0400, Sean MacLennan wrote:
> This patch removes the i2c code which is now obsolete due to the new
> ibm iic driver walking the device tree for child nodes.
> 
> There are two other small cleanups that came indirectly from the ad7414
> code review. Make sure Tlow is correct and handle the case where
> i2c_smbus_read_word_data fails.
> 
> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>

Looks okay to me, but you should really cc: the 4xx maintainer (Josh
Boyer) when sending out 4xx patches.

Cheers,
g.

> ---
> diff --git a/arch/powerpc/platforms/44x/warp.c b/arch/powerpc/platforms/44x/warp.c
> index 9565995..960edf8 100644
> --- a/arch/powerpc/platforms/44x/warp.c
> +++ b/arch/powerpc/platforms/44x/warp.c
> @@ -30,18 +30,6 @@ static __initdata struct of_device_id warp_of_bus[] = {
>  	{},
>  };
>  
> -static __initdata struct i2c_board_info warp_i2c_info[] = {
> -	{ I2C_BOARD_INFO("ad7414", 0x4a) }
> -};
> -
> -static int __init warp_arch_init(void)
> -{
> -	/* This should go away once support is moved to the dts. */
> -	i2c_register_board_info(0, warp_i2c_info, ARRAY_SIZE(warp_i2c_info));
> -	return 0;
> -}
> -machine_arch_initcall(warp, warp_arch_init);
> -
>  static int __init warp_device_probe(void)
>  {
>  	of_platform_bus_probe(NULL, warp_of_bus, NULL);
> @@ -223,7 +211,7 @@ static void pika_setup_critical_temp(struct i2c_client *client)
>  
>  	/* These registers are in 1 degree increments. */
>  	i2c_smbus_write_byte_data(client, 2, 65); /* Thigh */
> -	i2c_smbus_write_byte_data(client, 3, 55); /* Tlow */
> +	i2c_smbus_write_byte_data(client, 3,  0); /* Tlow */
>  
>  	np = of_find_compatible_node(NULL, NULL, "adi,ad7414");
>  	if (np == NULL) {
> @@ -289,8 +277,15 @@ found_it:
>  	printk(KERN_INFO "PIKA DTM thread running.\n");
>  
>  	while (!kthread_should_stop()) {
> -		u16 temp = swab16(i2c_smbus_read_word_data(client, 0));
> -		out_be32(fpga + 0x20, temp);
> +		int val;
> +
> +		val = i2c_smbus_read_word_data(client, 0);
> +		if (val < 0)
> +			dev_dbg(&client->dev, "DTM read temp failed.\n");
> +		else {
> +			s16 temp = swab16(val);
> +			out_be32(fpga + 0x20, temp);
> +		}
>  
>  		pika_dtm_check_fan(fpga);
>  
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

^ permalink raw reply

* [PATCH] powerpc: Fix 8xx build failure
From: Kumar Gala @ 2008-07-26 17:57 UTC (permalink / raw)
  To: linuxppc-dev

The 'powerpc ioremap_prot' broke 8xx builds:

include2/asm/pgtable-ppc32.h:555: error: '_PAGE_WRITETHRU' undeclared (first use in this function)
include2/asm/pgtable-ppc32.h:555: error: (Each undeclared identifier is reported only once
include2/asm/pgtable-ppc32.h:555: error: for each function it appears in.)

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---

in my powerpc-next tree.

- k

 include/asm-powerpc/pgtable-ppc32.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/asm-powerpc/pgtable-ppc32.h b/include/asm-powerpc/pgtable-ppc32.h
index bdbab72..6fe39e3 100644
--- a/include/asm-powerpc/pgtable-ppc32.h
+++ b/include/asm-powerpc/pgtable-ppc32.h
@@ -401,6 +401,9 @@ extern int icache_44x_need_flush;
 #ifndef _PAGE_COHERENT
 #define _PAGE_COHERENT	0
 #endif
+#ifndef _PAGE_WRITETHRU
+#define _PAGE_WRITETHRU	0
+#endif
 #ifndef _PMD_PRESENT_MASK
 #define _PMD_PRESENT_MASK	_PMD_PRESENT
 #endif
-- 
1.5.5.1

^ permalink raw reply related

* Re: [RFC,PATCH] scripts/package: add powerpc images to tarball
From: Grant Likely @ 2008-07-26 17:53 UTC (permalink / raw)
  To: Milton Miller; +Cc: linuxppc-dev, linux-kernel, Jeremy Kerr
In-Reply-To: <913573e2092839fbe37a0bfa37119eb6@bga.com>

On Sat, Jul 26, 2008 at 1:20 PM, Milton Miller <miltonm@bga.com> wrote:
> On Jul 25, 2008, at 11:55 PM, Grant Likely wrote:
>>
>> On Thu, Jul 24, 2008 at 9:08 PM, Milton Miller <miltonm@bga.com> wrote:
>>>>
>>>> Add support for powerpc builds in the buildtar script, to include
>>>> a few default images.
>>>> ---
>>>> RFC: any requests for more/less boot images?
>>>
>>> ..
>>>>
>>>> +             for img in zImage zImage.pseries zImage.iseries \
>>>> +                             dtbImage dtbImage.ps3
>>>
>>> Yes.  How about all dtbImage, zImage, cuboot, treeboot, etc
>>> that are newer than vmlinux?
>>
>> dtbImage is not a buildable image.  Neither is cuImage, treeImage or
>> simpleImage.  All of those targets embed a device tree which is
>> specified by adding the .dts filename to the target name.
>
> I intended "all dtbImage" as a wildcard for dtbImage.*, etc.

Yeah, okay.  That's a good idea.

>> so, for example, 'make cuImage' fails.  Instead you do 'make
>> cuImage.lite5200b' which pulls in dts/lite5200b.dts.
>
> The user does make zImage which makes cuImage.lite5200b based on Kconfig.
> Or it was that way until your change in 2.6.25 to allow the later, but they
> can still do the former.

Yes, both methods should work (at least they both work for me).  There
is also now a Kconfig option (CONFIG_EXTRA_TARGETS) which can be used
to add additional images to the zImage target list.  Should be useful
for some of the defconfigs.

> Hmm, we really need to fix our calls to make boot images with make -j.
>  Something for my todo list.

Good idea

>
>> Also, zImage is a 'meta' target that builds all the default image
>> targets (the $image-y list).  The zImage is actually just a symlink to
>> the first file in the list of default images.  So zImage can actually
>> point to any kind of kernel image depending on how the kernel is
>> configured.  I wonder if we should just remove the zImage file
>> entirely, or at least make it always linked to one particular image
>> type.
>
> I think its fine as it is.   It says "make what is configured" that lets
> cross-platform building scripts be dumb and not need to know specificially
> what image makes sense to make.  Perhaps we should make it default to our
> additional target in Kconfig if specified, this allowing the user to specify
> which version he gets.

It makes sense if only one target is ever built at a time; but since
we build multiple targets, I don't want users to get caught just using
zImage and then get bitten when a a config or source change causes it
to be linked to a different kernel type.

Come to think of it, I'd support just removing the zImage symlink
entirely to eliminate confusion so that everyone knows that 'make
zImage' is the 'make all' for default image targets.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* Re: [RFC,PATCH] scripts/package: add powerpc images to tarball
From: Milton Miller @ 2008-07-26 17:20 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, linux-kernel, Jeremy Kerr
In-Reply-To: <fa686aa40807252155l4f785920qdb14d117067b30f4@mail.gmail.com>

On Jul 25, 2008, at 11:55 PM, Grant Likely wrote:
> On Thu, Jul 24, 2008 at 9:08 PM, Milton Miller <miltonm@bga.com> wrote:
>>> Add support for powerpc builds in the buildtar script, to include
>>> a few default images.
>>> ---
>>> RFC: any requests for more/less boot images?
>> ..
>>> +             for img in zImage zImage.pseries zImage.iseries \
>>> +                             dtbImage dtbImage.ps3
>>
>> Yes.  How about all dtbImage, zImage, cuboot, treeboot, etc
>> that are newer than vmlinux?
>
> dtbImage is not a buildable image.  Neither is cuImage, treeImage or
> simpleImage.  All of those targets embed a device tree which is
> specified by adding the .dts filename to the target name.

I intended "all dtbImage" as a wildcard for dtbImage.*, etc.

> so, for example, 'make cuImage' fails.  Instead you do 'make
> cuImage.lite5200b' which pulls in dts/lite5200b.dts.

The user does make zImage which makes cuImage.lite5200b based on 
Kconfig.   Or it was that way until your change in 2.6.25 to allow the 
later, but they can still do the former.

Hmm, we really need to fix our calls to make boot images with make -j.  
Something for my todo list.

> Also, zImage is a 'meta' target that builds all the default image
> targets (the $image-y list).  The zImage is actually just a symlink to
> the first file in the list of default images.  So zImage can actually
> point to any kind of kernel image depending on how the kernel is
> configured.  I wonder if we should just remove the zImage file
> entirely, or at least make it always linked to one particular image
> type.

I think its fine as it is.   It says "make what is configured" that 
lets cross-platform building scripts be dumb and not need to know 
specificially what image makes sense to make.  Perhaps we should make 
it default to our additional target in Kconfig if specified, this 
allowing the user to specify which version he gets.

milton

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox