Discussion:
[PATCH] pasemi: electra IDE/pata_platform glue
Olof Johansson
2007-05-12 14:50:41 UTC
Permalink
Glue code to hook up the pata_platform on the PA Semi Electra eval board.
CFE sets up device tree entries for the IDE interface, with device type
'ide' and compatible field 'electra-ide'.

We unfortunately need to modify the resources before calling the generic
platform driver, since the device tree only has one register window in
it and the driver expects two. Adding this as an of_platform driver
instead doesn't give us any benefit, it just adds one more layer of
register/probe functions.

Since CONFIG_PATA_PLATFORM depends on CONFIG_EMBEDDED, add that as a
default for PPC_PASEMI.


Signed-off-by: Olof Johansson <olof at lixom.net>


Index: 2.6.21/arch/powerpc/platforms/pasemi/Makefile
===================================================================
--- 2.6.21.orig/arch/powerpc/platforms/pasemi/Makefile
+++ 2.6.21/arch/powerpc/platforms/pasemi/Makefile
@@ -1,3 +1,4 @@
obj-y += setup.o pci.o time.o idle.o powersave.o iommu.o
obj-$(CONFIG_PPC_PASEMI_MDIO) += gpio_mdio.o
+obj-$(CONFIG_ELECTRA_IDE) += electra_ide.o
obj-$(CONFIG_PPC_PASEMI_CPUFREQ) += cpufreq.o
Index: 2.6.21/arch/powerpc/platforms/pasemi/electra_ide.c
===================================================================
--- /dev/null
+++ 2.6.21/arch/powerpc/platforms/pasemi/electra_ide.c
@@ -0,0 +1,96 @@
+/*
+ * Copyright (C) 2007 PA Semi, Inc
+ *
+ * Maintained by: Olof Johansson <olof at lixom.net>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/platform_device.h>
+
+#include <asm/prom.h>
+#include <asm/system.h>
+
+/* The electra IDE interface is incredibly simple: Just a device on the localbus
+ * with interrupts hooked up to one of the GPIOs. The device tree contains the
+ * address window and interrupt mappings already, and the pata_platform driver handles
+ * the rest. We just need to hook the two up.
+ */
+
+#define MAX_IFS 4 /* really, we have only one */
+
+static struct platform_device *pdevs[MAX_IFS];
+
+static int __devinit electra_ide_init(void)
+{
+ struct device_node *np;
+ struct resource r[3];
+ int ret = 0;
+ int i;
+
+ np = of_find_compatible_node(NULL, "ide", "electra-ide");
+ i = 0;
+
+ while (np && i < MAX_IFS) {
+ memset(r, 0, sizeof(r));
+
+ /* pata_platform wants two address ranges: one for the base registers,
+ * another for the control (altstatus). It's located at offset 0x3f6 in
+ * the window, but the device tree only has one large register window
+ * that covers both ranges. So we need to split it up by hand here:
+ */
+
+ ret = of_address_to_resource(np, 0, &r[0]);
+ if (ret)
+ goto out;
+ ret = of_address_to_resource(np, 0, &r[1]);
+ if (ret)
+ goto out;
+
+ r[1].start += 0x3f6;
+ r[0].end = r[1].start-1;
+
+ r[2].start = irq_of_parse_and_map(np, 0);
+ r[2].end = irq_of_parse_and_map(np, 0);
+ r[2].flags = IORESOURCE_IRQ;
+
+ pr_debug("registering platform device at 0x%lx/0x%lx, irq is %ld\n",
+ r[0].start, r[1].start, r[2].start);
+ pdevs[i] = platform_device_register_simple("pata_platform", i, r, 3);
+ if (IS_ERR(pdevs[i])) {
+ ret = PTR_ERR(pdevs[i]);
+ pdevs[i] = NULL;
+ goto out;
+ }
+ np = of_find_compatible_node(np, "ide", "electra-ide");
+ }
+out:
+ return ret;
+}
+module_init(electra_ide_init);
+
+static void __devexit electra_ide_exit(void)
+{
+ int i;
+
+ for (i = 0; i < MAX_IFS; i++)
+ if (pdevs[i])
+ platform_device_unregister(pdevs[i]);
+}
+module_exit(electra_ide_exit);
+
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR ("Olof Johansson <olof at lixom.net>");
+MODULE_DESCRIPTION("PA Semi Electra IDE driver");
Index: 2.6.21/arch/powerpc/platforms/pasemi/Kconfig
===================================================================
--- 2.6.21.orig/arch/powerpc/platforms/pasemi/Kconfig
+++ 2.6.21/arch/powerpc/platforms/pasemi/Kconfig
@@ -6,6 +6,7 @@ config PPC_PASEMI
select PPC_UDBG_16550
select GENERIC_TBSYNC
select PPC_NATIVE
+ select EMBEDDED
help
This option enables support for PA Semi's PWRficient line
of SoC processors, including PA6T-1682M
@@ -26,4 +27,13 @@ config PPC_PASEMI_MDIO
help
Driver for MDIO via GPIO on PWRficient platforms

+config ELECTRA_IDE
+ tristate "Electra IDE driver"
+ default y
+ depends on PPC_PASEMI && ATA
+ select PATA_PLATFORM
+ help
+ This includes driver support for the Electra on-board IDE
+ interface.
+
endmenu
Arnd Bergmann
2007-05-12 23:11:02 UTC
Permalink
Post by Olof Johansson
We unfortunately need to modify the resources before calling the generic
platform driver, since the device tree only has one register window in
it and the driver expects two. Adding this as an of_platform driver
instead doesn't give us any benefit, it just adds one more layer of
register/probe functions.
Why not provide a proper pata_of.c driver based on ata_generic? That
will help the next person that has a builtin ata controller and wants
to get it running as an of_device.

If you simply export generic_sht (rename that to ata_generic_sht)
and generic_port_ops (-> ata_generic_port_ops), then the resulting
of_platform_driver should be no more complicated than the one
you are proposing here.

Arnd <><
Alan Cox
2007-05-12 23:58:30 UTC
Permalink
Post by Arnd Bergmann
Why not provide a proper pata_of.c driver based on ata_generic? That
will help the next person that has a builtin ata controller and wants
to get it running as an of_device.
Easier to use pata_platform I would think ? Just create the OF device and
bind it to pata_platform.

Alan
Arnd Bergmann
2007-05-13 00:48:22 UTC
Permalink
Post by Alan Cox
Post by Arnd Bergmann
Why not provide a proper pata_of.c driver based on ata_generic? That
will help the next person that has a builtin ata controller and wants
to get it running as an of_device.
Easier to use pata_platform I would think ? Just create the OF device and
bind it to pata_platform.
Not sure I understand what you mean. pata_platform expects a platform_device,
which cannot be cast from an of_device.

Actually, it might be possible to merge the common parts of pata_platform,
ata_generic and ata_of into one module, and have one driver for each
of the three bus_types on top of it. Currently, the only difference between
ata_generic and pata_platform is the ->set_mode function, which can
probably be made generic (controlled by a flag in the ata_port).

Olof, have you looked at which of the two ->set_mode functions is more
appropriate for electra? I would guess that you actually want to use
the ata_generic one.

Arnd <><
Benjamin Herrenschmidt
2007-05-13 02:20:10 UTC
Permalink
Post by Arnd Bergmann
Post by Alan Cox
Post by Arnd Bergmann
Why not provide a proper pata_of.c driver based on ata_generic? That
will help the next person that has a builtin ata controller and wants
to get it running as an of_device.
Easier to use pata_platform I would think ? Just create the OF device and
bind it to pata_platform.
Not sure I understand what you mean. pata_platform expects a platform_device,
which cannot be cast from an of_device.
Note that in the long run, we might have less problem with that sort of
thing.

First, every device now has an OF node optionally attached to it (since
I introduced dev_sysdata). This of_device doesn't really bring much more
than OF type/name/compatible properties based probing.

Then, the plans I have in mind for the future of that stuff are around
the idea of registering "constructors" based on bus matches and device
matches respectively.

The kernel will then walk the whole OF device-tree at boot, and will
instanciate devices using those constructors, passing them the struct
device of whatever was the parent.

Thus we could easily have platforms registers constructors for specific
devices that build a platform device off an OF node. We can have a
generic constructor that builds the PCI devices, etc... and we can have
bus constructors to generate of_device's for things where they are
useful, like plb4/5 on 4xx etc...

On top of that, I want to add some resource management to of_device and
maybe kill things like ebus, vio, etc... device if possible (that is if
the only difference to the base OF device can be resolved at
construction time).

Anyway, still only thoughts but that gives you an idea to where I'm
heading.

Ben.
Olof Johansson
2007-05-13 06:19:54 UTC
Permalink
Post by Benjamin Herrenschmidt
Then, the plans I have in mind for the future of that stuff are around
the idea of registering "constructors" based on bus matches and device
matches respectively.
The kernel will then walk the whole OF device-tree at boot, and will
instanciate devices using those constructors, passing them the struct
device of whatever was the parent.
Thus we could easily have platforms registers constructors for specific
devices that build a platform device off an OF node.
Great. That should fit perfectly with the glue I have now, it'll turn
into the constructor instead.

I honestly don't see what the benefit is of this recent obsession
with creating of_platform devices for every new device that's not
PCI, especially when there's an already well-fitting driver in the
traditional platform model. It's not like the code to link the two is
large and complex.

Also, in this particular case, the bindings are not standardized, and
there's a good chance that whatever new platform uses a similar device
will need to do something slightly different.


-Olof
Benjamin Herrenschmidt
2007-05-13 08:01:50 UTC
Permalink
Post by Olof Johansson
Great. That should fit perfectly with the glue I have now, it'll turn
into the constructor instead.
I honestly don't see what the benefit is of this recent obsession
with creating of_platform devices for every new device that's not
PCI, especially when there's an already well-fitting driver in the
traditional platform model. It's not like the code to link the two is
large and complex.
I agree.
Post by Olof Johansson
Also, in this particular case, the bindings are not standardized, and
there's a good chance that whatever new platform uses a similar device
will need to do something slightly different.
Yup.

Ben.
Segher Boessenkool
2007-05-13 11:17:57 UTC
Permalink
Post by Olof Johansson
Also, in this particular case, the bindings are not standardized, and
there's a good chance that whatever new platform uses a similar device
will need to do something slightly different.
Yeah, everyone else will do the sane thing, and describe
the two register ranges the IDE uses, not a much bigger
range full of register shadows like you have. It is one
thing for the hardware to do partial address decoding;
the device tree shouldn't normally expose this though.

But that's what you've got now, so you have your own
special OF device matching code, which is exactly as it
should be. No need to have "generic" ide OF matching
code until device trees containing such devices show
up :-)


Segher
Alan Cox
2007-05-13 13:39:27 UTC
Permalink
Post by Arnd Bergmann
Post by Alan Cox
Easier to use pata_platform I would think ? Just create the OF device and
bind it to pata_platform.
Not sure I understand what you mean. pata_platform expects a platform_device,
which cannot be cast from an of_device.
Other than the binding and wanting to use the standard ->set_mode they
appear to be identical so a copy of pata_platform ought to turn into a
pata_of very nicely.
Post by Arnd Bergmann
Olof, have you looked at which of the two ->set_mode functions is more
appropriate for electra? I would guess that you actually want to use
the ata_generic one.
You almost certainly simply don't want to provide one. I'm not sure the
pata_platform default one is right at all as it will assume the firmware
tuned everything. ata_generic does this purely because it is a "last
resort" driver for chips we can't tune/program at all.

Alan
Olof Johansson
2007-05-13 21:18:29 UTC
Permalink
Hi,
Post by Arnd Bergmann
Olof, have you looked at which of the two ->set_mode functions is more
appropriate for electra? I would guess that you actually want to use
the ata_generic one.
Actually, the pata_platform driver is _exactly_ what I want at this time,
since we can't do bus master dma and it assumes that. It's a perfect fit,
I see no reason to retool it.

Another reason for why I'm not that excited about doing a generic
of_platform driver is that it's likely that I'll do a new driver that
uses one of the simple dma engines on our SoC to get the data over PIO
(i.e still not bus master dma, but at least offloaded). That'd make the
resulting number of users of a pata_of driver 0 until the next one comes
around, with whatever that means with respect to bit-rot, etc.

That driver will, on the other hand, likely be an of_platform one. But
until then there's not much use in doing one.


-Olof

Loading...