Versatile Express infrastructure

Clocks must be prepared before enabling and unprepared after disabling. Without that clk_enable() fails with warning.

Signed-off-by Pawel Moll <[email protected]>

---
 drivers/input/serio/ambakmi.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

drivers/input/serio/ambakmi.c +0 -0

index 2ffd110..ab2d25b 100644
@@ -72,10 +72,14 @@ static int amba_kmi_open(struct serio *io)
 	unsigned int divisor;
 	int ret;

Quoted 5 lines

drivers/input/serio/ambakmi.c +0 -0

@@ -72,10 +72,14 @@ static int amba_kmi_open(struct serio *io)
 	unsigned int divisor;
 	int ret;

drivers/input/serio/ambakmi.c static int amba_kmi_open(struct serio *io) · line 82

Quoted 10 lines

drivers/input/serio/ambakmi.c +5 -1

 
-	ret = clk_enable(kmi->clk);
+	ret = clk_prepare(kmi->clk);
 	if (ret)
 		goto out;
 
+	ret = clk_enable(kmi->clk);
+	if (ret)
+		goto clk_unprepare;
+

What about using clk_prepare_enable() here?

drivers/input/serio/ambakmi.c static void amba_kmi_close(struct serio *io) · line 114

Quoted 3 lines

drivers/input/serio/ambakmi.c +1 -0

 	free_irq(kmi->irq, kmi);
 	clk_disable(kmi->clk);
+	clk_unprepare(kmi->clk);

And clk_disable_unprepare() here?

Documentation/devicetree/bindings/arm/vexpress.txt +5 -0

+	dcc at 0 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#interrupt-cells = <0>;
+		arm,vexpress,site = <0xff>; /* Master site */

drivers/misc/Kconfig +1 -0

@@ -517,4 +517,5 @@ source "drivers/misc/lis3lv02d/Kconfig"
 source "drivers/misc/carma/Kconfig"
 source "drivers/misc/altera-stapl/Kconfig"
 source "drivers/misc/mei/Kconfig"
+source "drivers/misc/vexpress/Kconfig"
 endmenu

drivers/misc/Makefile +0 -0

index b88df7a..49964fd 100644

drivers/misc/Makefile +11 -0

+static int vexpress_config_match(struct device *dev, struct device_driver *drv)
+{
+	struct vexpress_config_device *vecdev = to_vexpress_config_device(dev);
+	struct vexpress_config_driver *vecdrv = to_vexpress_config_driver(drv);
+
+	if (vecdrv->funcs) {
+		const unsigned *func = vecdrv->funcs;
+
+		while (*func) {
+			if (*func == vecdev->func)
+				return 1;

drivers/misc/Makefile +11 -0

+#define VEXPRESS_COMPATIBLE_TO_FUNC(_compatible, _func) \
+	{ \
+		.compatible = "arm,vexpress-config," _compatible, \
+		.data = (void *)VEXPRESS_CONFIG_FUNC_##_func \
+	}
+
+static struct of_device_id vexpress_config_devices_matches[] = {
+	VEXPRESS_COMPATIBLE_TO_FUNC("osc", OSC),
+	VEXPRESS_COMPATIBLE_TO_FUNC("volt", VOLT),
+	VEXPRESS_COMPATIBLE_TO_FUNC("amp", AMP),
+	VEXPRESS_COMPATIBLE_TO_FUNC("temp", TEMP),

drivers/misc/Makefile +50 -0

+static void vexpress_config_of_device_add(struct device_node *node)
+{
+	int err;
+	struct vexpress_config_device *vecdev;
+	const struct of_device_id *match;
+	u32 value;
+
+	if (!of_device_is_available(node))
+		return;
+
+	vecdev = kzalloc(sizeof(*vecdev), GFP_KERNEL);
+	if (WARN_ON(!vecdev))
+		return;
+
+	vecdev->dev.of_node = of_node_get(node);
+
+	vecdev->name = node->name;
+
+	match = of_match_node(vexpress_config_devices_matches, node);
+	vecdev->func = (unsigned)match->data;
+
+	err = of_property_read_u32(node->parent, "arm,vexpress,site", &value);
+	if (!err)
+		vecdev->addr.site = value;
+
+	err = of_property_read_u32(node->parent, "arm,vexpress,position",
+			&value);
+	if (!err)
+		vecdev->addr.position = value;
+
+	err = of_property_read_u32(node->parent, "arm,vexpress,dcc", &value);
+	if (!err)
+		vecdev->addr.dcc = value;
+
+	err = of_property_read_u32(node, "reg", &value);
+	if (!err) {
+		vecdev->addr.device = value;
+	} else {
+		pr_err("Invalid reg property in '%s'! (%d)\n",
+				node->full_name, err);
+		kfree(vecdev);
+		return;
+	}
+
+	err = vexpress_config_device_register(vecdev);
+	if (err) {
+		pr_err("Failed to add OF device '%s'! (%d)\n",
+				node->full_name, err);
+		kfree(vecdev);
+		return;

drivers/misc/Makefile +23 -0

+int vexpress_config_device_register(struct vexpress_config_device *vecdev)
+{
+	pr_debug("Registering %sdevice '%s." ADDR_FMT "'\n",
+			vexpress_config_early ? "early " : "",
+			vecdev->name, ADDR_ARGS(vecdev));
+
+	if (vecdev->addr.site == VEXPRESS_SITE_MASTER)
+		vecdev->addr.site = vexpress_config_get_master_site();
+
+	if (!vecdev->bridge) {
+		spin_lock(&vexpress_config_bridges_lock);
+		vexpress_config_bridge_find(&vecdev->dev, NULL);
+		spin_unlock(&vexpress_config_bridges_lock);
+	}
+
+	if (vexpress_config_early) {
+		list_add(&vecdev->early, &vexpress_config_early_devices);
+		vexpress_config_early_bind();
+
+		return 0;
+	}
+
+	device_initialize(&vecdev->dev);

Quoted 9 lines · Pawel Moll

Documentation/devicetree/bindings/arm/vexpress.txt +9 -0

+	dcc at 0 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#interrupt-cells = <0>;
+		arm,vexpress,site = <0xff>; /* Master site */
+
+		osc at 0 {
+			compatible = "arm,vexpress-config,osc";
+			reg = <0>;

Documentation/devicetree/bindings/arm/vexpress.txt Abbreviated example: · line 296

Quoted 6 lines

Documentation/devicetree/bindings/arm/vexpress.txt +5 -0

+			freq-range = <50000000 100000000>;
+			#clock-cells = <1>;
+			clock-output-names = "oscclk0";
+		};
+	};
 };

The #interrupt-cells property seems misplaced here.

Quoted 12 lines

drivers/misc/Kconfig +1 -0

@@ -517,4 +517,5 @@ source "drivers/misc/lis3lv02d/Kconfig"
 source "drivers/misc/carma/Kconfig"
 source "drivers/misc/altera-stapl/Kconfig"
 source "drivers/misc/mei/Kconfig"
+source "drivers/misc/vexpress/Kconfig"
 endmenu

drivers/misc/Makefile +0 -0

index b88df7a..49964fd 100644

drivers/misc/Makefile obj-y += carma/ · line 53

Quoted 5 lines

drivers/misc/Makefile +1 -0

@@ -50,3 +50,4 @@ obj-y				+= carma/
 obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
 obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
 obj-$(CONFIG_INTEL_MEI)		+= mei/
+obj-y				+= vexpress/

This does not look like something that should go to drivers/misc (well, basically nothing ever does). How about drivers/mfd or drivers/bus instead?

drivers/misc/Makefile

Quoted 5 lines

drivers/misc/Makefile +5 -0

+#define ADDR_FMT "%u.%x:%x:%x:%x"
+
+#define ADDR_ARGS(_ptr) \
+		_ptr->func, _ptr->addr.site, _ptr->addr.position, \
+		_ptr->addr.dcc, _ptr->addr.device

Can't you use dev_printk() to print the device name in the normal format?

Quoted 5 lines

drivers/misc/Makefile +5 -0

+#define ADDR_TO_U64(addr) \
+		(((u64)(addr).site << 32) | ((addr).position << 24) | \
+		((addr).dcc << 16) | (addr).device)
+
+static bool vexpress_config_early = true;

drivers/misc/Makefile

Quoted 3 lines

drivers/misc/Makefile +3 -0

+static DEFINE_MUTEX(vexpress_config_early_mutex);
+static LIST_HEAD(vexpress_config_early_drivers);
+static LIST_HEAD(vexpress_config_early_devices);

What is the reason for needing early devices that you have to keep in a list like this? If it's only for setup purposes, it's probably easier to have a platform hook that probes the hardware you want to initialize at boot time and only start using the device method at device init time.

Quoted 15 lines

drivers/misc/Makefile +15 -0

+static int vexpress_config_match(struct device *dev, struct device_driver *drv)
+{
+	struct vexpress_config_device *vecdev = to_vexpress_config_device(dev);
+	struct vexpress_config_driver *vecdrv = to_vexpress_config_driver(drv);
+
+	if (vecdrv->funcs) {
+		const unsigned *func = vecdrv->funcs;
+
+		while (*func) {
+			if (*func == vecdev->func)
+				return 1;
+			func++;
+		}
+	}
+

drivers/misc/Makefile

Quoted 6 lines

drivers/misc/Makefile +6 -0

+	return 0;
+}
+
+static struct device vexpress_config_bus = {
+	.init_name = "vexpress-config",
+};

No static devices in new code please. Just put it into the device tree.

drivers/misc/Makefile

Quoted 4 lines

drivers/misc/Makefile +4 -0

+struct bus_type vexpress_config_bus_type = {
+	.name = "vexpress-config",
+	.match = vexpress_config_match,
+};

What is the reason for having a separate bus_type here? Is this a discoverable bus? If it is, why do you need a device tree binding for the child devices?

Quoted 16 lines

drivers/misc/Makefile +16 -0

+#define VEXPRESS_COMPATIBLE_TO_FUNC(_compatible, _func) \
+	{ \
+		.compatible = "arm,vexpress-config," _compatible, \
+		.data = (void *)VEXPRESS_CONFIG_FUNC_##_func \
+	}
+
+static struct of_device_id vexpress_config_devices_matches[] = {
+	VEXPRESS_COMPATIBLE_TO_FUNC("osc", OSC),
+	VEXPRESS_COMPATIBLE_TO_FUNC("volt", VOLT),
+	VEXPRESS_COMPATIBLE_TO_FUNC("amp", AMP),
+	VEXPRESS_COMPATIBLE_TO_FUNC("temp", TEMP),
+	VEXPRESS_COMPATIBLE_TO_FUNC("reset", RESET),
+	VEXPRESS_COMPATIBLE_TO_FUNC("scc", SCC),
+	VEXPRESS_COMPATIBLE_TO_FUNC("muxfpga", MUXFPGA),
+	VEXPRESS_COMPATIBLE_TO_FUNC("shutdown", SHUTDOWN),
+	VEXPRESS_COMPATIBLE_TO_FUNC("reboot", REBOOT),

drivers/misc/Makefile

Quoted 5 lines

drivers/misc/Makefile +5 -0

+	VEXPRESS_COMPATIBLE_TO_FUNC("dvimode", DVIMODE),
+	VEXPRESS_COMPATIBLE_TO_FUNC("power", POWER),
+	VEXPRESS_COMPATIBLE_TO_FUNC("energy", ENERGY),
+	{},
+};

What is the purpose of this lookup? Can't you make the child devices get probed by the compatible value?

Quoted 56 lines

drivers/misc/Makefile +56 -0

+static void vexpress_config_of_device_add(struct device_node *node)
+{
+	int err;
+	struct vexpress_config_device *vecdev;
+	const struct of_device_id *match;
+	u32 value;
+
+	if (!of_device_is_available(node))
+		return;
+
+	vecdev = kzalloc(sizeof(*vecdev), GFP_KERNEL);
+	if (WARN_ON(!vecdev))
+		return;
+
+	vecdev->dev.of_node = of_node_get(node);
+
+	vecdev->name = node->name;
+
+	match = of_match_node(vexpress_config_devices_matches, node);
+	vecdev->func = (unsigned)match->data;
+
+	err = of_property_read_u32(node->parent, "arm,vexpress,site", &value);
+	if (!err)
+		vecdev->addr.site = value;
+
+	err = of_property_read_u32(node->parent, "arm,vexpress,position",
+			&value);
+	if (!err)
+		vecdev->addr.position = value;
+
+	err = of_property_read_u32(node->parent, "arm,vexpress,dcc", &value);
+	if (!err)
+		vecdev->addr.dcc = value;
+
+	err = of_property_read_u32(node, "reg", &value);
+	if (!err) {
+		vecdev->addr.device = value;
+	} else {
+		pr_err("Invalid reg property in '%s'! (%d)\n",
+				node->full_name, err);
+		kfree(vecdev);
+		return;
+	}
+
+	err = vexpress_config_device_register(vecdev);
+	if (err) {
+		pr_err("Failed to add OF device '%s'! (%d)\n",
+				node->full_name, err);
+		kfree(vecdev);
+		return;
+	}
+}
+
+void vexpress_config_of_populate(void)
+{
+	struct device_node *node;

drivers/misc/Makefile

Quoted 4 lines

drivers/misc/Makefile +4 -0

+
+	for_each_matching_node(node, vexpress_config_devices_matches)
+		vexpress_config_of_device_add(node);
+}

This is unusual. Why do you only add the matching devices rather than all of them? Doing it your way also means O(n^2) rather than O(n) traversal through the list of children.

Quoted 28 lines

drivers/misc/Makefile +28 -0

+int vexpress_config_device_register(struct vexpress_config_device *vecdev)
+{
+	pr_debug("Registering %sdevice '%s." ADDR_FMT "'\n",
+			vexpress_config_early ? "early " : "",
+			vecdev->name, ADDR_ARGS(vecdev));
+
+	if (vecdev->addr.site == VEXPRESS_SITE_MASTER)
+		vecdev->addr.site = vexpress_config_get_master_site();
+
+	if (!vecdev->bridge) {
+		spin_lock(&vexpress_config_bridges_lock);
+		vexpress_config_bridge_find(&vecdev->dev, NULL);
+		spin_unlock(&vexpress_config_bridges_lock);
+	}
+
+	if (vexpress_config_early) {
+		list_add(&vecdev->early, &vexpress_config_early_devices);
+		vexpress_config_early_bind();
+
+		return 0;
+	}
+
+	device_initialize(&vecdev->dev);
+	vecdev->dev.bus = &vexpress_config_bus_type;
+	if (!vecdev->dev.parent)
+		vecdev->dev.parent = &vexpress_config_bus;
+
+	dev_set_name(&vecdev->dev, "%s." ADDR_FMT,

drivers/misc/Makefile

Quoted 5 lines

drivers/misc/Makefile +5 -0

+			vecdev->name, ADDR_ARGS(vecdev));
+
+	return device_add(&vecdev->dev);
+}
+EXPORT_SYMBOL(vexpress_config_device_register);

Why is this exported to non-GPL drivers? It looks like the only caller should be in this file.

Quoted 14 lines · Arnd Bergmann

Quoted 12 lines · Pawel Moll

drivers/misc/Kconfig +1 -0

@@ -517,4 +517,5 @@ source "drivers/misc/lis3lv02d/Kconfig"
 source "drivers/misc/carma/Kconfig"
 source "drivers/misc/altera-stapl/Kconfig"
 source "drivers/misc/mei/Kconfig"
+source "drivers/misc/vexpress/Kconfig"
 endmenu

drivers/misc/Makefile +0 -0

index b88df7a..49964fd 100644

Quoted 8 lines

drivers/misc/Makefile obj-y += carma/ · line 53

Quoted 5 lines

drivers/misc/Makefile +1 -0

@@ -50,3 +50,4 @@ obj-y				+= carma/
 obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
 obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
 obj-$(CONFIG_INTEL_MEI)		+= mei/
+obj-y				+= vexpress/

This does not look like something that should go to drivers/misc (well, basically nothing ever does). How about drivers/mfd or drivers/bus instead?

I don't see drivers/bus in 3.6-rc4? If there will be such thing, I guess I can move config_bus.c to drivers/bus/vexpress-config.c, display.c to drivers/video/vexpress-display.c (see my other answer), sysreg.c to drivers/mfd/vexpress-sysreg.c (it is a multifunction device indeed, not argument about that), but reset.c seems to me should stay as drivers/misc/vexpress-reset.c - it's hardly a mfd...

Quoted 5 lines

Quoted 5 lines

drivers/misc/Makefile +5 -0

+#define ADDR_TO_U64(addr) \
+		(((u64)(addr).site << 32) | ((addr).position << 24) | \
+		((addr).dcc << 16) | (addr).device)
+
+static bool vexpress_config_early = true;

Quoted 8 lines

drivers/misc/Makefile

Quoted 3 lines

drivers/misc/Makefile +3 -0

+static DEFINE_MUTEX(vexpress_config_early_mutex);
+static LIST_HEAD(vexpress_config_early_drivers);
+static LIST_HEAD(vexpress_config_early_devices);

What is the reason for needing early devices that you have to keep in a list like this? If it's only for setup purposes, it's probably easier to have a platform hook that probes the hardware you want to initialize at boot time and only start using the device method at device init time.

Funnily enough the first version didn't have anything "early related"... Till I actually defined the real clock dependency in the tree :-(

So it's all about clocks, that must be available really early. The particular problem I faced was the amba_probe() trying to enable apb_pclk of each device and failing...

Now, during the clock registration the device model is not initialized yet, so I can't do normal devices registration. I've stolen some ideas from the early platform bus code...

Quoted 15 lines

Quoted 15 lines

drivers/misc/Makefile +15 -0

+static int vexpress_config_match(struct device *dev, struct device_driver *drv)
+{
+	struct vexpress_config_device *vecdev = to_vexpress_config_device(dev);
+	struct vexpress_config_driver *vecdrv = to_vexpress_config_driver(drv);
+
+	if (vecdrv->funcs) {
+		const unsigned *func = vecdrv->funcs;
+
+		while (*func) {
+			if (*func == vecdev->func)
+				return 1;
+			func++;
+		}
+	}
+

Quoted 8 lines

drivers/misc/Makefile

Quoted 6 lines

drivers/misc/Makefile +6 -0

+	return 0;
+}
+
+static struct device vexpress_config_bus = {
+	.init_name = "vexpress-config",
+};

No static devices in new code please. Just put it into the device tree.

Hm. This is just a dummy device serving as a default parent, similarly to:

struct device platform_bus = {
.init_name = "platform",
};
EXPORT_SYMBOL_GPL(platform_bus);

Without that I can see two options:

1. Create some kind of a device (platform?) for the dcc/mcc node and use it as a parent.
2. Scan the device tree "downwards" searching for a node that has a device already associated with it (but this may not work at the early stage).

Quoted 8 lines

drivers/misc/Makefile

Quoted 4 lines

drivers/misc/Makefile +4 -0

+struct bus_type vexpress_config_bus_type = {
+	.name = "vexpress-config",
+	.match = vexpress_config_match,
+};

What is the reason for having a separate bus_type here? Is this a discoverable bus? If it is, why do you need a device tree binding for the child devices?

It is not a discoverable bus, but the devices are very different from the "normal" platform devices and have specific read/write interface, so it seemed to me that a separate bus would make sense. And I didn't want to reinvent the wheel with my own "device/driver model".

Quoted 16 lines

Quoted 16 lines

drivers/misc/Makefile +16 -0

+#define VEXPRESS_COMPATIBLE_TO_FUNC(_compatible, _func) \
+	{ \
+		.compatible = "arm,vexpress-config," _compatible, \
+		.data = (void *)VEXPRESS_CONFIG_FUNC_##_func \
+	}
+
+static struct of_device_id vexpress_config_devices_matches[] = {
+	VEXPRESS_COMPATIBLE_TO_FUNC("osc", OSC),
+	VEXPRESS_COMPATIBLE_TO_FUNC("volt", VOLT),
+	VEXPRESS_COMPATIBLE_TO_FUNC("amp", AMP),
+	VEXPRESS_COMPATIBLE_TO_FUNC("temp", TEMP),
+	VEXPRESS_COMPATIBLE_TO_FUNC("reset", RESET),
+	VEXPRESS_COMPATIBLE_TO_FUNC("scc", SCC),
+	VEXPRESS_COMPATIBLE_TO_FUNC("muxfpga", MUXFPGA),
+	VEXPRESS_COMPATIBLE_TO_FUNC("shutdown", SHUTDOWN),
+	VEXPRESS_COMPATIBLE_TO_FUNC("reboot", REBOOT),

Quoted 8 lines

drivers/misc/Makefile

Quoted 5 lines

drivers/misc/Makefile +5 -0

+	VEXPRESS_COMPATIBLE_TO_FUNC("dvimode", DVIMODE),
+	VEXPRESS_COMPATIBLE_TO_FUNC("power", POWER),
+	VEXPRESS_COMPATIBLE_TO_FUNC("energy", ENERGY),
+	{},
+};

What is the purpose of this lookup? Can't you make the child devices get probed by the compatible value?

Ok, there are two reasons for this table existence:

1. vexpress_config_of_populate() below - I need a comprehensive list of compatible values to be able to search the tree and create respective devices.

2. Non-DT static devices - I need something to be able to match a driver with a device, and the "functions list" seemed appropriate.

Quoted 56 lines

Quoted 56 lines

drivers/misc/Makefile +56 -0

+static void vexpress_config_of_device_add(struct device_node *node)
+{
+	int err;
+	struct vexpress_config_device *vecdev;
+	const struct of_device_id *match;
+	u32 value;
+
+	if (!of_device_is_available(node))
+		return;
+
+	vecdev = kzalloc(sizeof(*vecdev), GFP_KERNEL);
+	if (WARN_ON(!vecdev))
+		return;
+
+	vecdev->dev.of_node = of_node_get(node);
+
+	vecdev->name = node->name;
+
+	match = of_match_node(vexpress_config_devices_matches, node);
+	vecdev->func = (unsigned)match->data;
+
+	err = of_property_read_u32(node->parent, "arm,vexpress,site", &value);
+	if (!err)
+		vecdev->addr.site = value;
+
+	err = of_property_read_u32(node->parent, "arm,vexpress,position",
+			&value);
+	if (!err)
+		vecdev->addr.position = value;
+
+	err = of_property_read_u32(node->parent, "arm,vexpress,dcc", &value);
+	if (!err)
+		vecdev->addr.dcc = value;
+
+	err = of_property_read_u32(node, "reg", &value);
+	if (!err) {
+		vecdev->addr.device = value;
+	} else {
+		pr_err("Invalid reg property in '%s'! (%d)\n",
+				node->full_name, err);
+		kfree(vecdev);
+		return;
+	}
+
+	err = vexpress_config_device_register(vecdev);
+	if (err) {
+		pr_err("Failed to add OF device '%s'! (%d)\n",
+				node->full_name, err);
+		kfree(vecdev);
+		return;
+	}
+}
+
+void vexpress_config_of_populate(void)
+{
+	struct device_node *node;

Quoted 8 lines

drivers/misc/Makefile

Quoted 4 lines

drivers/misc/Makefile +4 -0

+
+	for_each_matching_node(node, vexpress_config_devices_matches)
+		vexpress_config_of_device_add(node);
+}

This is unusual. Why do you only add the matching devices rather than all of them? Doing it your way also means O(n^2) rather than O(n) traversal through the list of children.

Em, I'm not sure what do you mean... The idea is shamelessly stolen from of_irq_init() and of_clk_init()...

Quoted 4 lines

Quoted 1 line

+EXPORT_SYMBOL(vexpress_config_device_register);

Why is this exported to non-GPL drivers? It looks like the only caller should be in this file.

Hm. There's no hidden agenda behind the non-GPL export, if that's what you are afraid of ;-) Now, why it is exported at all? Because platform_device_register is, I suppose (you can tell that I was looking at the platform bus code ;-). The non-DT platform code is using it, so it can't be static, but I wouldn't really expect any module to use it. So I can make it EXPORT_SYMBOL_GPL or drop it, no problem.

We're adding drivers/bus in v3.7, I already have another bus driver in the arm-soc tree.

The reset code can probably just go to arch/arm/mach-vexpress though, we do the same for the reset code on all other platforms.

Quoted 7 lines

So it's all about clocks, that must be available really early. The particular problem I faced was the amba_probe() trying to enable apb_pclk of each device and failing...

Now, during the clock registration the device model is not initialized yet, so I can't do normal devices registration. I've stolen some ideas from the early platform bus code...

Maybe you can change amba_probe() to provide a -EPROBE_DEFER return code to the caller when the clock is not yet there, it should then just come back later.

Quoted 9 lines

Quoted 8 lines

drivers/misc/vexpress/config_bus.c line 67

Quoted 4 lines

drivers/misc/vexpress/config_bus.c new file +4 -0

+struct bus_type vexpress_config_bus_type = {
+	.name = "vexpress-config",
+	.match = vexpress_config_match,
+};

What is the reason for having a separate bus_type here? Is this a discoverable bus? If it is, why do you need a device tree binding for the child devices?

Quoted 4 lines

It is not a discoverable bus, but the devices are very different from the "normal" platform devices and have specific read/write interface, so it seemed to me that a separate bus would make sense. And I didn't want to reinvent the wheel with my own "device/driver model".

Not introducing a different way to do devices is good, but I don't think that using something else than platform devices buys you much. If it's not discoverable, this driver does not look all that different from an MFD (which is based on platform devices).

A new bus type is typically used only for cases where you have multiple different bus drivers and multiple different device drivers, and want a bus layer to proxy between them.

Quoted 5 lines

Quoted 2 lines

What is the purpose of this lookup? Can't you make the child devices get probed by the compatible value?

Ok, there are two reasons for this table existence:

Quoted 3 lines

1. vexpress_config_of_populate() below - I need a comprehensive list of compatible values to be able to search the tree and create respective devices.

I don't see why you need this list. Just call of_platform_populate or a copy of that. It does not require the compatible values of the devices, just the one for the parent.

Quoted 2 lines

2. Non-DT static devices - I need something to be able to match a driver with a device, and the "functions list" seemed appropriate.

How important is this case really, given that the driver has never been supported and that the non-DT case is going away soon?

Quoted 2 lines

Em, I'm not sure what do you mean... The idea is shamelessly stolen from of_irq_init() and of_clk_init()...

But those are not buses, they are infrastructure that is used across buses. The regular way to do this is to register a driver for your parent node and then just iterate over the children, in the way that we do for e.g. i2c or spi buses.

Signed-off-by Pawel Moll <[email protected]>

---
 Documentation/devicetree/bindings/arm/vexpress.txt |   19 ++
 drivers/misc/vexpress/Makefile                     |    1 +
 drivers/misc/vexpress/display.c                    |  197 ++++++++++++++++++++
 3 files changed, 217 insertions(+)
 create mode 100644 drivers/misc/vexpress/display.c

Quoted 8 lines · Pawel Moll

Signed-off-by Pawel Moll <[email protected]>

---
 Documentation/devicetree/bindings/arm/vexpress.txt |   19 ++
 drivers/misc/vexpress/Makefile                     |    1 +
 drivers/misc/vexpress/display.c                    |  197 ++++++++++++++++++++
 3 files changed, 217 insertions(+)
 create mode 100644 drivers/misc/vexpress/display.c

Shouldn't this go into the drivers/video directory?

drivers/clk/versatile/clk-vexpress-osc.c new file +8 -0

+static int vexpress_osc_probe(struct vexpress_config_device *vecdev)
+{
+       int err;
+       struct device_node *node = vecdev->dev.of_node;
+       struct vexpress_osc_info *info = vecdev->dev.platform_data;
+       struct clk_init_data init;
+       struct vexpress_osc *osc;
+       struct clk *clk;

Quoting Pawel Moll (2012-09-03 09:25:25)

Quoted 8 lines

drivers/clk/versatile/clk-vexpress-osc.c new file +8 -0

+static int vexpress_osc_probe(struct vexpress_config_device *vecdev)
+{
+       int err;
+       struct device_node *node = vecdev->dev.of_node;
+       struct vexpress_osc_info *info = vecdev->dev.platform_data;
+       struct clk_init_data init;
+       struct vexpress_osc *osc;
+       struct clk *clk;

drivers/clk/versatile/clk-vexpress-osc.c line 87

Quoted 10 lines

drivers/clk/versatile/clk-vexpress-osc.c new file +10 -0

+       const char * const *dev_ids = NULL;
+       u32 range[2];
+
+       if (vecdev->status & VEXPRESS_CONFIG_DEVICE_PROBED_EARLY)
+               return 0;
+
+       osc = kzalloc(sizeof(*osc), GFP_KERNEL);
+       if (!osc) {
+               err = -ENOMEM;
+               goto error;

Minor nitpick: the error label tries to free osc, which in this case shouldn't be freed because it is NULL.

<snip>

Quoted 3 lines

drivers/clk/versatile/clk-vexpress-osc.c new file +3 -0

+error:
+       kfree(osc);
+       return err;

Otherwise patch looks good to me. There are some changes to headers in this patch, and it is part of a larger series. How did you want the common clk patches to get merged?

On Mon, 2012-09-10 at 20:14 +0100, Mike Turquette wrote:

Quoted 1 line

Quoted 1 line

+ osc = kzalloc(sizeof(*osc), GFP_KERNEL);

Quoted 4 lines

Minor nitpick: the error label tries to free osc, which in this case shouldn't be freed because it is NULL.

Quoted 1 line

+ kfree(osc);

$ grep kfree scripts/checkpatch.pl

$ grep -B2 kfree Documentation/CodingStyle

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ osc = kzalloc(sizeof(*osc), GFP_KERNEL);

Can't you just use devm_kzalloc(&vecdev->dev, ...) and be done with it?

I'm re-working the series right now, but generally I planned to push drivers/clk changes via your tree, unless you want me to get it in through arm-soc.

Cheers!

Pawel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

drivers/clk/versatile/clk-vexpress.c line 87

Quoted 9 lines · Pawel Moll

drivers/clk/versatile/clk-vexpress.c new file +9 -0

+void __init vexpress_clk_init(void __iomem *sp810_base)
+{
+       struct clk *clk;
+       int i;
+
+       clk = clk_register_fixed_rate(NULL, "dummy_apb_pclk", NULL,
+                       CLK_IS_ROOT, 0);
+       WARN_ON(clk_register_clkdev(clk, "apb_pclk", NULL));
+

Isn't it time to just remove the non-DT parts of vexpress? That would also simplify this driver.

But we are still "CLCD driver away" from the full features replacement. Ryan Harkin is working on this,

Quoted 2 lines · Pawel Moll

But we are still "CLCD driver away" from the full features replacement. Ryan Harkin is working on this,

Will be useful for Integrator, Versatile, RealView, and some non-ARM SoCs too I think, so very interesting!

This patch adds a DT and non-DT based implementation of the common clock infrastructure for Versatile Express platform. It registers (statically or using DT) all required fixed clocks, initialises motherboard's SP810 cell (that provides clocks for SP804 timers) and explicitely registers VE "osc" driver, to make the clock generators availalable early.

Signed-off-by Pawel Moll <[email protected]>

Cc Mike Turquette <[email protected]>

Cc Linus Walleij <[email protected]>

Add description of all functions provided by Versatile Express motherboard and daughterboards configuration controllers and clock dependencies between devices.

Signed-off-by Pawel Moll <[email protected]>

---
 arch/arm/boot/dts/vexpress-v2m-rs1.dtsi     |  136 ++++++++++++++++++++-
 arch/arm/boot/dts/vexpress-v2m.dtsi         |  136 ++++++++++++++++++++-
 arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts |  103 ++++++++++++++++
 arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts  |  169 +++++++++++++++++++++++++++
 arch/arm/boot/dts/vexpress-v2p-ca5s.dts     |   71 +++++++++++
 arch/arm/boot/dts/vexpress-v2p-ca9.dts      |  121 +++++++++++++++++++
 6 files changed, 732 insertions(+), 4 deletions(-)

arch/arm/boot/dts/vexpress-v2m-rs1.dtsi +1 -1

index d8a827b..9cc2a56 100644
@@ -72,14 +72,20 @@
 			#size-cells = <1>;
 			ranges = <0 3 0 0x200000>;
 
-			sysreg at 010000 {
+			v2m_sysreg: sysreg at 010000 {
 				compatible = "arm,vexpress-sysreg";
 				reg = <0x010000 0x1000>;

arch/arm/boot/dts/vexpress-v2m-rs1.dtsi line 86

Quoted 6 lines

arch/arm/boot/dts/vexpress-v2m-rs1.dtsi +3 -1

-			sysctl at 020000 {
+			v2m_sysctl: sysctl at 020000 {
 				compatible = "arm,sp810", "arm,primecell";
 				reg = <0x020000 0x1000>;
+				clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&v2m_osc_clk0>;
+				clock-names = "refclk", "timclk", "apb_pclk";

See Documentation/devicetree/bindings/arm/primecell.txt

apb_pclk should be first in the list.

Quoted 9 lines · Rob Herring

arch/arm/boot/dts/vexpress-v2m-rs1.dtsi line 86

Quoted 6 lines

arch/arm/boot/dts/vexpress-v2m-rs1.dtsi +3 -1

-			sysctl at 020000 {
+			v2m_sysctl: sysctl at 020000 {
 				compatible = "arm,sp810", "arm,primecell";
 				reg = <0x020000 0x1000>;
+				clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&v2m_osc_clk0>;
+				clock-names = "refclk", "timclk", "apb_pclk";

See Documentation/devicetree/bindings/arm/primecell.txt

Quoted 1 line

apb_pclk should be first in the list.

Hm. Why, if you don't mind me asking? The amba_get_enable_pclk() explicitly asks for "apb_pclk" id:

struct clk *pclk = clk_get(&pcdev->dev, "apb_pclk");

Now, let's have a look at MMCI:

                        mmci at 050000 {
                                compatible = "arm,pl180", "arm,primecell";
                                reg = <0x050000 0x1000>;
                                interrupts = <9 10>;

arch/arm/boot/dts/vexpress-v2m-rs1.dtsi +6 -0

+                               cd-gpios = <&v2m_sysreg 0 0>;
+                               wp-gpios = <&v2m_sysreg 1 0>;
+                               max-frequency = <12000000>;
+                               vmmc-supply = <&v2m_fixed_3v3>;
+                               clocks = <&v2m_clk_24mhz>, <&v2m_osc_clk0>;
+                               clock-names = "mclk", "apb_pclk";
                        };

and mmci_probe() does:

        host->clk = clk_get(&dev->dev, NULL);
        if (IS_ERR(host->clk)) {
                ret = PTR_ERR(host->clk);
                host->clk = NULL;
                goto host_free;
        }       

Now, if I put "apb_pclk" first, before "mclk", the driver will get the wrong clock.

Pawe?

Quoted 10 lines · Pawel Moll

Quoted 9 lines · Rob Herring

arch/arm/boot/dts/vexpress-v2m-rs1.dtsi line 86

Quoted 6 lines

arch/arm/boot/dts/vexpress-v2m-rs1.dtsi +3 -1

-			sysctl at 020000 {
+			v2m_sysctl: sysctl at 020000 {
 				compatible = "arm,sp810", "arm,primecell";
 				reg = <0x020000 0x1000>;
+				clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&v2m_osc_clk0>;
+				clock-names = "refclk", "timclk", "apb_pclk";

See Documentation/devicetree/bindings/arm/primecell.txt

Quoted 4 lines

Quoted 1 line

apb_pclk should be first in the list.

Hm. Why, if you don't mind me asking? The amba_get_enable_pclk() explicitly asks for "apb_pclk" id:

So apb_pclk is always in the same position in the list and I had to define something. Clock names are supposed to be optional, so we have to be able to identify which clock is the bus clock without the name.

You're getting lucky with how clk_get is implemented. The driver should be more specific with which clock it wants if there is more than 1. Perhaps we need a clk_get_by_index() function?

Quoted 19 lines · Rob Herring

Quoted 10 lines · Pawel Moll

Quoted 9 lines · Rob Herring

arch/arm/boot/dts/vexpress-v2m-rs1.dtsi line 86

Quoted 6 lines

arch/arm/boot/dts/vexpress-v2m-rs1.dtsi +3 -1

-			sysctl at 020000 {
+			v2m_sysctl: sysctl at 020000 {
 				compatible = "arm,sp810", "arm,primecell";
 				reg = <0x020000 0x1000>;
+				clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&v2m_osc_clk0>;
+				clock-names = "refclk", "timclk", "apb_pclk";

See Documentation/devicetree/bindings/arm/primecell.txt

Quoted 4 lines

Quoted 1 line

apb_pclk should be first in the list.

Hm. Why, if you don't mind me asking? The amba_get_enable_pclk() explicitly asks for "apb_pclk" id:

So apb_pclk is always in the same position in the list and I had to define something. Clock names are supposed to be optional, so we have to be able to identify which clock is the bus clock without the name.

[snip]

Quoted 3 lines

You're getting lucky with how clk_get is implemented. The driver should be more specific with which clock it wants if there is more than 1. Perhaps we need a clk_get_by_index() function?

But the clocking framework doesn't know what "index", while it knows exactly what is "id" (string). How about making the clock names obligatory in the tree when there a node has more than one clock input? This sounds much simpler to me and seems to be in line with the clocking API. And the apb_pclk wouldn't have to be first/last/whatever.

I did say they are *supposed* to be optional, but the amba bus code essentially makes them required. Ideally, Linux doesn't dictate what goes in the dtb or not, but I don't have a problem if specific users of the clock binding require the names.