Re: [RFC PATCH 4/4] ARM: Xilinx: Adding Xilinx board support

-----Original Message-----
From: Arnd Bergmann [mailto:arnd at arndb.de] 
Sent: Monday, May 02, 2011 2:40 AM
To: linux-arm-kernel at lists.infradead.org
Cc: grant.likely at secretlab.ca; Nicolas Pitre; Russell King;
devicetree-discuss at lists.ozlabs.org; linux-kernel at vger.kernel.org;
John
Linn
Subject: Re: [RFC PATCH 4/4] ARM: Xilinx: Adding Xilinx board support

On Monday 02 May 2011 07:08:00 Grant Likely wrote:

quoted

The 1st board support is minimal to get a system up and running
on the Xilinx platform.

This platform reuses the clock implementation from plat-versatile,

and

quoted

it depends entirely on CONFIG_OF support.  There is only one board
support file which obtains all device information from a device

tree

quoted

dtb file which is passed to the kernel at boot time.

Very cool stuff!

quoted

+
+	amba {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;

Shouldn't we have a way to identify amba buses by the compatible
property
as well, rather than only declaring it simple-bus?

We might be able to do some more automatic probing in the amba
code in cases where the architected registers are actually filled
with meaningful data there.

Is the empty ranges property technically correct? Excuse my poor
knowledge of amba, but I would have assumed that only a range
of addresses is actually put on the bus, while others (e.g. RAM)
are not visible on AMBA.

Grant can comment on the ranges technical correctness, I see it other
places but that's doesn't mean it's right.

AFAIK, everything is visible from this bus including RAM.

quoted

+static struct of_device_id zynq_of_bus_ids[] __initdata = {
+	{ .compatible = "simple-bus", },
+	{}
+};
+
+static struct of_device_id gic_match[] = {
+	{ .compatible = "arm,gic", },
+	{}
+};
+
+/**
+ * xilinx_init_machine() - System specific initialization,

intended
to be

quoted

+ *			   called from board specific initialization.
+ */
+void __init xilinx_init_machine(void)
+{
+	struct device_node *node = of_find_matching_node(NULL,

gic_match);

quoted

+
+	if (node)
+		of_irq_domain_add_simple(node, 0, NR_IRQS);
+
+#ifdef CONFIG_CACHE_L2X0
+	/*
+	 * 64KB way size, 8-way associativity, parity disabled
+	 */
+	l2x0_init(PL310_L2CC_BASE, 0x02060000, 0xF0F0FFFF);
+#endif
+
+/**
+ * xilinx_irq_init() - Interrupt controller initialization for the

GIC.

quoted

+ */
+void __init xilinx_irq_init(void)
+{
+	gic_init(0, 29, SCU_GIC_DIST_BASE, SCU_GIC_CPU_BASE);
+}

I think we can do better than this, there is still more hardcoded
stuff
in here than I think should be. I understand that you tried to
minimize
the size of the patch set for obvious reasons, but none of this
seems too board specific to put into common code in one way or
another.

Of course we can not fix all of this at once, but maybe we can have
an explanation for each hardcoded setting on why it is still needed
and what would have to be done to make it go away.

Yes we could explain hard coded numbers.  Ideally GIC driver would support
device tree it seems like to me.

quoted

+/* The minimum devices needed to be mapped before the VM system is

up
and

quoted

+ * running include the GIC, UART and Timer Counter.
+ */
+
+static struct map_desc io_desc[] __initdata = {
+	{
+		.virtual	= TTC0_VIRT,
+		.pfn		= __phys_to_pfn(TTC0_PHYS),
+		.length		= SZ_4K,
+		.type		= MT_DEVICE,
+	}, {
+		.virtual	= SCU_PERIPH_VIRT,
+		.pfn		= __phys_to_pfn(SCU_PERIPH_PHYS),
+		.length		= SZ_8K,
+		.type		= MT_DEVICE,
+	}, {
+		.virtual	= PL310_L2CC_VIRT,
+		.pfn		= __phys_to_pfn(PL310_L2CC_PHYS),
+		.length		= SZ_4K,
+		.type		= MT_DEVICE,
+	},

I especially dislike the idea of having to set up iotables, these
seem completely counterintuitive when we probe the devices from the
device tree.

AFAICT, all of init_irq, time_init and init_machine are called way
after
mm_init, so you should have ioremap available by the time you need to
access these virtual memory ranges.

Seems like the GIC driver could remap it's own memory but maybe that won't
work across all platforms and then the platform would need to do it.

Yes the timer driver could remap it's own memory.

I can understand why you'd want to special-case PCI I/O space windows
and early serial port access, but I think we should handle them
differently
and give them fixed machine independent virtual addresses.

quoted

--- /dev/null
+++ b/arch/arm/mach-zynq/include/mach/io.h
--- /dev/null
+++ b/arch/arm/mach-zynq/include/mach/irqs.h
--- /dev/null
+++ b/arch/arm/mach-zynq/include/mach/memory.h
--- /dev/null
+++ b/arch/arm/mach-zynq/include/mach/system.h
--- /dev/null
+++ b/arch/arm/mach-zynq/include/mach/timex.h
--- /dev/null
+++ b/arch/arm/mach-zynq/include/mach/vmalloc.h

These are all either completely generic versions that could be reused
on a number of other machines, or they could be turned into such
files.

For the global inlude/asm-generic files, we have just added a
mechanism
to share that kind of file across multiple architectures using some
Makefile magic. Should we perhaps do something similar here to avoid
having to create more of the same files?

quoted

--- /dev/null
+++ b/arch/arm/mach-zynq/include/mach/uart.h
--- /dev/null
+++ b/arch/arm/mach-zynq/include/mach/uncompress.h

For these, it obviously won't work.

quoted

diff --git a/arch/arm/mach-zynq/timer.c

b/arch/arm/mach-zynq/timer.c

quoted

new file mode 100644
index 0000000..c2c96cc

--- /dev/null
+++ b/arch/arm/mach-zynq/timer.c

In the light of the recent discussions of drivers moving out of
platforms into common Linux code, do we expect this to also
happen to timers any time soon?

I'm not sure on the rest. Thanks Arnd, appreciate the help.

-- John

	Arnd


This email and any attachments are intended for the sole use of the
named recipient(s) and contain(s) confidential information that may be
proprietary, privileged or copyrighted under applicable law. If you
are not the intended recipient, do not read, copy, or forward this
email message or any attachments. Delete this email message and any
attachments immediately.

On Monday 02 May 2011 23:27:04 johnlinn at comcast.net wrote:

Yes we could explain hard coded numbers.  Ideally GIC driver would support
device tree it seems like to me.

Seems like the GIC driver could remap it's own memory but maybe that won't
work across all platforms and then the platform would need to do it.

Why not just add a stub in the xilinx platform for now that remaps the
memory using ioremap and calls the regular GIC initialization?

That function can then easily be moved to common code when another
platform uses it.

	Arnd