PowerPC: implement GPIO API

arch/powerpc/kernel/prom_parse.c +4 -0

+int __of_parse_gpio_bank_pin(struct device_node *np, int index,
+			     int bank_width, int max_bank)
+{
+	int bank;

Quoted 8 lines

arch/powerpc/kernel/prom_parse.c +8 -0

+int __of_parse_gpio_bank_pin(struct device_node *np, int index,
+			     int bank_width, int max_bank)
+{
+	int bank;
+	int pin;
+	const u32 *gpios;
+
+	/*

arch/powerpc/kernel/prom_parse.c void __iomem *of_iomap(struct device_node *np, int index) · line 1097

Quoted 6 lines

arch/powerpc/kernel/prom_parse.c +6 -0

+	 * We can get there only if of_get_gpio() succeeded, thus
+	 * no need checking for "gpios" existence.
+	 */
+	gpios = of_get_property(np, "gpios", NULL);
+	bank = gpios[index * 2];
+	pin = gpios[index * 2 + 1];

If you stick with the #gpio-cells plan, here is where you should use it.

I think part of what's happening here is due to the patch's history.

Obviously a
driver for a particular gpio controller would generally need to assert that the controller's #gpio-cells has the correct value for this controller type, after which code like the above would be acceptable.

+static spinlock_t cpm2_port_lock = __SPIN_LOCK_UNLOCKED(cpm2_port_lock);

arch/powerpc/sysdev/cpm2_common.c +10 -0

+int cpm2_init_par_io(void)
+{
+	struct device_node *np;
+	const u32 *num_ports;
+
+	np = of_find_compatible_node(NULL, NULL, "fsl,cpm2-pario");
+	if (!np)
+		return -ENOENT;
+
+	num_ports = of_get_property(np, "num-ports", NULL);

Quoted 2 lines · Anton Vorontsov

+static spinlock_t cpm2_port_lock = __SPIN_LOCK_UNLOCKED(cpm2_port_lock);

This needs to be

static DEFINE_SPINLOCK(cpm2_port_lock);

I think at least lockdep doesn't work the way you do it here.

Quoted 16 lines

arch/powerpc/sysdev/cpm2_common.c +16 -0

+int cpm2_init_par_io(void)
+{
+	struct device_node *np;
+	const u32 *num_ports;
+
+	np = of_find_compatible_node(NULL, NULL, "fsl,cpm2-pario");
+	if (!np)
+		return -ENOENT;
+
+	num_ports = of_get_property(np, "num-ports", NULL);
+	if (!num_ports) {
+		of_node_put(np);
+		return -ENOENT;
+	}
+	cpm2_num_ports = *num_ports;
+

arch/powerpc/sysdev/cpm2_common.c cpm2_map_t __iomem *cpm2_immr; · line 97

Quoted 4 lines

arch/powerpc/sysdev/cpm2_common.c +4 -0

+	np->data = &of_gpio_chip;
+
+	return 0;
+}

This function should also do the call to of_iomap, so you don't need to pull the address out of the cpm2_immr, which I believe we're trying to get rid of.

Jochen, I kept your Signed-off-by, though this isn't your original patch. Hope you're okay with it. I also hope you'll test it. ;-)

I also hope you'll test it. ;-)

Hi Anton,

Quoted 1 line

I also hope you'll test it. ;-)

yes.

arch/powerpc/sysdev/commproc.c static cpic8xx_t __iomem *cpic_reg; · line 81

Quoted 9 lines

arch/powerpc/sysdev/commproc.c +9 -0

+int cpm_init_par_io(void)
+{
+       struct device_node *np;
+       const u32 *num_ports;
+
+       np = of_find_node_by_name(NULL, "fsl,cpm1-pario");
+       if (!np)
+               return -ENOENT;
+

I guess this should be:

np = of_find_compatible_node(NULL, NULL, "fsl,cpm1-pario");

With this modification it works OK for me :)

Also, in the upcoming kernels, there will be GPIOLIB[1] addition to the generic GPIO API, to support off-chip GPIO expanders (like MFDs on I2C/LBC). But so far we support on-chip GPIOs only, with single controller built-in.

Changes since RFC:
- Implemented #gpio-cells handling;
- Per-bank spinlocks removed;
- Added a patch which implements GPIO API for CPM1;
- Few minor fixes.

Quoted 10 lines · Anton Vorontsov

Also, in the upcoming kernels, there will be GPIOLIB[1] addition to the generic GPIO API, to support off-chip GPIO expanders (like MFDs on I2C/LBC). But so far we support on-chip GPIOs only, with single controller built-in.

Changes since RFC:
- Implemented #gpio-cells handling;
- Per-bank spinlocks removed;
- Added a patch which implements GPIO API for CPM1;
- Few minor fixes.

Also need to add documentation to booting-without-of.txt.

In general this looks like a good direction, but I do not like the hard linking for QE and CPM gpios to the 'top level' gpio API. I think I'd prefer this stuff to stay out of mainline until the gpiolib

stuff gets merged (which should be soon IIRC).

As I've probably said once already: if there are plans to build single kernel with QE+CPM1+CPM2 inside tomorrow -- then of course, I'd better wait.

But if these plans are distant enough, I see no reason why we can't enjoy of current API.

Quoted 4 lines · Anton Vorontsov

As I've probably said once already: if there are plans to build single kernel with QE+CPM1+CPM2 inside tomorrow -- then of course, I'd better wait.

Quoted 2 lines

But if these plans are distant enough, I see no reason why we can't enjoy of current API.

Oh, I'm not saying don't enjoy it. :-) I'm just saying keep it out of mainline. I've got a bunch of Virtex stuff that falls into that category. Those who are interested can pick the non mainlined patches out of my git tree.

OF device tree GPIOs bindings are similar to IRQs:

pario0: gpio-controller@0 {
	#gpio-cells = <2>;

	num-ports = <7>;

device@0 {
	gpios = <bank pin bank pin bank pin>;
	gpio-parent = <&pario0>;

Quoted 1 line

OF device tree GPIOs bindings are similar to IRQs:

But GPIOs are a very different thing. Most importantly, the "number" of a GPIO is completely local to the GPIO controller.

Quoted 2 lines

pario0: gpio-controller@0 {
	#gpio-cells = <2>;

Your Linux code doesn't actually use this. Why is it needed, anyway? You should be able to encode a GPIO identifier in a single 32-bit word, for any possible GPIO controller.

Quoted 1 line

	num-ports = <7>;

What is this? What is a "port"? This doesn't belong in a generic GPIO binding.

Quoted 3 lines

device@0 {
	gpios = <bank pin bank pin bank pin>;
	gpio-parent = <&pario0>;

Not every GPIO controller has banks.

Not every device uses GPIOs
on a single GPIO controller. It is inconvenient to force all bindings to use the same name ("gpios") for its property that shows the GPIOs (and for it to have only one such property).

So I recommend:

-- Advise (in the generic GPIO binding) people to use
< phandle-of-gpio-controller gpio-id-on-that-controller >
to refer to a GPIO from some device node;

-- And either:
-- Define (in the generic GPIO binding) that a "gpio-id" is a single
32-bit cell;
or
-- Define (in the generic GPIO binding) that a "gpio-id" is a number
of 32-bit cells, and that that number of cells is encoded as a
32-bit
integer in the "#gpio-cells" property in the device node of the respective GPIO controller.

(I like the first option better, unless someone can think of some reasonable
situation where some specific GPIO controller binding needs more than 32 bits
to encode GPIO #).

Quoted 4 lines

Quoted 1 line

OF device tree GPIOs bindings are similar to IRQs:

But GPIOs are a very different thing. Most importantly, the "number" of a GPIO is completely local to the GPIO controller.

Yes... just as interrupt specifiers are local to their interrupt domain.

Quoted 5 lines

Quoted 3 lines

device@0 {
	gpios = <bank pin bank pin bank pin>;
	gpio-parent = <&pario0>;

Not every GPIO controller has banks.

That's just bad terminology in the example. "bank pin" means an arbitrary format gpio specifier.

Quoted 7 lines

Not every device uses GPIOs
on a single GPIO controller. It is inconvenient to force all bindings to use the same name ("gpios") for its property that shows the GPIOs (and for it to have only one such property).

So I recommend:

Quoted 3 lines

-- Advise (in the generic GPIO binding) people to use
< phandle-of-gpio-controller gpio-id-on-that-controller >
to refer to a GPIO from some device node;

Ah, yes, that's a good point. Given the ugly workarounds we need to deal with devices which have interrupts from multiple domains, we don't want to copy that limitation to the GPIO scheme.

Quoted 5 lines

-- Define (in the generic GPIO binding) that a "gpio-id" is a number
of 32-bit cells, and that that number of cells is encoded as a
32-bit
integer in the "#gpio-cells" property in the device node of the respective GPIO controller.

This option was the idea; the "bank pin" information has a format local to the gpio controller. I agree the terminology needs to change to "gpio specifier" by analogy with the interrupt tree, though.

Quoted 5 lines

(I like the first option better, unless someone can think of some reasonable
situation where some specific GPIO controller binding needs more than 32 bits
to encode GPIO #).

I can't think of a situation where it would be strictly speaking necessary, but I can think of several where it would be more convenient. GPIO controllers that do have a bank/pin arrangement is one. GPIO controllers than have a pin number, plus some sort of direction or level information is another.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

device@0 {
	gpios = <bank pin bank pin bank pin>;
	gpio-parent = <&pario0>;

Not every GPIO controller has banks.

That's just bad terminology in the example. "bank pin" means an arbitrary format gpio specifier.

Okay. Don't split it into two things then, just say "gpio-spec" or such.

Quoted 11 lines

Quoted 10 lines

Not every device uses GPIOs
on a single GPIO controller. It is inconvenient to force all bindings to use the same name ("gpios") for its property that shows the GPIOs (and for it to have only one such property).

So I recommend:

-- Advise (in the generic GPIO binding) people to use
< phandle-of-gpio-controller gpio-id-on-that-controller >
to refer to a GPIO from some device node;

Quoted 3 lines

Ah, yes, that's a good point. Given the ugly workarounds we need to deal with devices which have interrupts from multiple domains, we don't want to copy that limitation to the GPIO scheme.

Yeah, and we even know for a fact that devices exist that do GPIOs on multiple GPIO controllers. In the interrupt case, no one thought anyone would be crazy enough to route their IRQs like that :-)

Quoted 6 lines

Quoted 5 lines

-- Define (in the generic GPIO binding) that a "gpio-id" is a number
of 32-bit cells, and that that number of cells is encoded as a
32-bit
integer in the "#gpio-cells" property in the device node of the respective GPIO controller.

Quoted 3 lines

This option was the idea; the "bank pin" information has a format local to the gpio controller. I agree the terminology needs to change to "gpio specifier" by analogy with the interrupt tree, though.

Right, with that cleared up, and the binding doc expanded a bit, you won't hear complaints from me :-)

Quoted 11 lines

Quoted 5 lines

(I like the first option better, unless someone can think of some reasonable
situation where some specific GPIO controller binding needs more than 32 bits
to encode GPIO #).

I can't think of a situation where it would be strictly speaking necessary, but I can think of several where it would be more convenient. GPIO controllers that do have a bank/pin arrangement is one. GPIO controllers than have a pin number, plus some sort of direction or level information is another.

Ah yes, a second word for pin "type" information makes a lot of sense. #gpio-cells it is, then. Let's please make sure that we put that "type" thing in the documentation (as an example), and that the first controller
bindings we put in use it.