Hi all,
I've been sitting on these patches for some time, but now it appears
that the set_sync() feature is needed elsewhere. So here are the
patches.
Joakim, I think this is what you need.
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
This is needed to set GPIO's values synchronously.
Signed-off-by: Anton Vorontsov <[email protected]>
---
arch/powerpc/include/asm/gpio.h | 11 +++++++++++
arch/powerpc/sysdev/qe_lib/gpio.c | 27 +++++++++++++++++++++++++++
2 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/include/asm/gpio.h b/arch/powerpc/include/asm/gpio.h
index ea04632..899f365 100644
--- a/arch/powerpc/include/asm/gpio.h
+++ b/arch/powerpc/include/asm/gpio.h
@@ -33,6 +33,17 @@ static inline void gpio_set_value(unsigned int gpio, int value)
__gpio_set_value(gpio, value);
}
+static inline int gpio_can_set_values_sync(unsigned num, unsigned *gpios)
+{
+ return __gpio_can_set_values_sync(num, gpios);
+}
+
+static inline void gpio_set_values_sync(unsigned num, unsigned *gpios,
+ int *values)
+{
+ __gpio_set_values_sync(num, gpios, values);
+}
+
static inline int gpio_cansleep(unsigned int gpio)
{
return __gpio_cansleep(gpio);
diff --git a/arch/powerpc/sysdev/qe_lib/gpio.c b/arch/powerpc/sysdev/qe_lib/gpio.c
index 3485288..6cfe784 100644
--- a/arch/powerpc/sysdev/qe_lib/gpio.c
+++ b/arch/powerpc/sysdev/qe_lib/gpio.c
@@ -84,6 +84,32 @@ static void qe_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
spin_unlock_irqrestore(&qe_gc->lock, flags);
}
+static void qe_gpio_set_sync(struct gpio_chip *gc, unsigned int num,
+ unsigned int *gpios, int *vals)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct qe_gpio_chip *qe_gc = to_qe_gpio_chip(mm_gc);
+ struct qe_pio_regs __iomem *regs = mm_gc->regs;
+ unsigned long flags;
+ int i;
+
+ spin_lock_irqsave(&qe_gc->lock, flags);
+
+ for (i = 0; i < num; i++) {
+ unsigned int gpio = gpios[i] - gc->base;
+ u32 pin_mask = 1 << (QE_PIO_PINS - 1 - gpio);
+
+ if (vals[i])
+ qe_gc->cpdata |= pin_mask;
+ else
+ qe_gc->cpdata &= ~pin_mask;
+ }
+
+ out_be32(®s->cpdata, qe_gc->cpdata);
+
+ spin_unlock_irqrestore(&qe_gc->lock, flags);
+}
+
static int qe_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
{
struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
@@ -328,6 +354,7 @@ static int __init qe_add_gpiochips(void)
gc->direction_output = qe_gpio_dir_out;
gc->get = qe_gpio_get;
gc->set = qe_gpio_set;
+ gc->set_sync = qe_gpio_set_sync;
ret = of_mm_gpiochip_add(np, mm_gc);
if (ret)
--
1.6.3.3
Sometimes it's necessary to set GPIO values synchronously, e.g.
when CPU's GPIOs go to a FPGA and the FPGA has no latch-enable line,
so intermediate values on GPIO lines may trigger unnecessary actions
by the FPGA.
Another usage is chip-select muxes, e.g. using X GPIOs as address lines
to mux 2^X chip-selects, and setting some amount of GPIOs at once may
be a huge win.
Note that it's not always possible to set up GPIOs synchronously, so
gpio_can_set_values_sync() call is implemented. Currently it checks
for two things:
1. Whether gpio_chip has .set_sync() callback;
2. Whether all passed GPIOs belong to one particular gpio_chip.
Someday we may want to implement chip-specific .can_set_sync() check,
but currently this isn't needed.
Signed-off-by: Anton Vorontsov <[email protected]>
---
drivers/gpio/gpiolib.c | 57 ++++++++++++++++++++++++++++++++++++++++++++
include/asm-generic/gpio.h | 8 ++++++
include/linux/gpio.h | 14 ++++++++++
3 files changed, 79 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 51a8d41..cf714ad 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1067,6 +1067,53 @@ void __gpio_set_value(unsigned gpio, int value)
EXPORT_SYMBOL_GPL(__gpio_set_value);
/**
+ * __gpio_can_set_values_sync() - check if gpios can be set synchronously
+ * @num: amount of gpios
+ * @gpios: gpios in question
+ * Context: any
+ *
+ * This is used directly or indirectly to implement gpio_can_set_values_sync().
+ * It returns nonzero if an array of gpios can be set simultaneously.
+ */
+int __gpio_can_set_values_sync(unsigned num, unsigned *gpios)
+{
+ struct gpio_chip *chip;
+ unsigned i;
+
+ chip = gpio_to_chip(gpios[0]);
+ if (!chip->set_sync)
+ return 0;
+
+ for (i = 1; i < num; i++) {
+ if (chip != gpio_to_chip(gpios[i]))
+ return 0;
+ }
+
+ return 1;
+}
+EXPORT_SYMBOL_GPL(__gpio_can_set_values_sync);
+
+/**
+ * __gpio_set_values_sync() - assign gpios' values synchronously
+ * @num: amount of gpios
+ * @gpios: gpios whose value will be assigned
+ * @values: value to assign
+ * Context: any
+ *
+ * This is used directly or indirectly to implement gpio_set_values_sync().
+ * It invokes the associated gpio_chip.set_sync() method.
+ */
+void __gpio_set_values_sync(unsigned num, unsigned *gpios, int *values)
+{
+ struct gpio_chip *chip;
+
+ chip = gpio_to_chip(gpios[0]);
+ WARN_ON(extra_checks && chip->can_sleep);
+ chip->set_sync(chip, num, gpios, values);
+}
+EXPORT_SYMBOL_GPL(__gpio_set_values_sync);
+
+/**
* __gpio_cansleep() - report whether gpio value access will sleep
* @gpio: gpio in question
* Context: any
@@ -1129,6 +1176,16 @@ void gpio_set_value_cansleep(unsigned gpio, int value)
}
EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);
+void gpio_set_values_sync_cansleep(unsigned num, unsigned *gpios, int *values)
+{
+ struct gpio_chip *chip;
+
+ might_sleep_if(extra_checks);
+ chip = gpio_to_chip(gpios[0]);
+ chip->set_sync(chip, num, gpios, values);
+}
+EXPORT_SYMBOL_GPL(gpio_set_values_sync_cansleep);
+
#ifdef CONFIG_DEBUG_FS
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index d6c379d..c181623 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -88,6 +88,10 @@ struct gpio_chip {
unsigned offset, int value);
void (*set)(struct gpio_chip *chip,
unsigned offset, int value);
+ /* NOTE: _sync() version accepts gpios, not offsets. */
+ void (*set_sync)(struct gpio_chip *chip,
+ unsigned num, unsigned *gpios,
+ int *values);
int (*to_irq)(struct gpio_chip *chip,
unsigned offset);
@@ -121,6 +125,8 @@ extern int gpio_direction_output(unsigned gpio, int value);
extern int gpio_get_value_cansleep(unsigned gpio);
extern void gpio_set_value_cansleep(unsigned gpio, int value);
+extern void gpio_set_values_sync_cansleep(unsigned num, unsigned *gpios,
+ int *values);
/* A platform's <asm/gpio.h> code may want to inline the I/O calls when
@@ -129,6 +135,8 @@ extern void gpio_set_value_cansleep(unsigned gpio, int value);
*/
extern int __gpio_get_value(unsigned gpio);
extern void __gpio_set_value(unsigned gpio, int value);
+extern int __gpio_can_set_values_sync(unsigned num, unsigned *gpios);
+extern void __gpio_set_values_sync(unsigned num, unsigned *gpios, int *values);
extern int __gpio_cansleep(unsigned gpio);
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index e10c49a..ff62c1e 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -62,6 +62,20 @@ static inline void gpio_set_value(unsigned gpio, int value)
WARN_ON(1);
}
+static inline int gpio_can_set_values_sync(unsigned num, unsigned *gpios)
+{
+ /* GPIO can never have been requested or set as output */
+ WARN_ON(1);
+ return 0;
+}
+
+static inline void gpio_set_values_sync(unsigned num, unsigned *gpios,
+ int *values)
+{
+ /* GPIO can never have been requested or set as output */
+ WARN_ON(1);
+}
+
static inline int gpio_cansleep(unsigned gpio)
{
/* GPIO can never have been requested or set as {in,out}put */
--
1.6.3.3
Anton Vorontsov <[email protected]> wrote on 13/07/2009 17:19:11:
>
> Hi all,
>
> I've been sitting on these patches for some time, but now it appears
> that the set_sync() feature is needed elsewhere. So here are the
> patches.
>
> Joakim, I think this is what you need.
Yes, it sure looks so :) I will have to look closer later as
I will be traveling the next few days.
Question though, have you considered using a bitmask instead of
an array:
static void qe_gpio_set_sync(struct gpio_chip *gc, unsigned int num,
unsigned int gpio_mask, unsigned int vals)
If you want to set bit 0, 3 and 8 you would set positions 0, 3 and 8 in gpio_mask
to ones. Similarly in vals, set bit positions 0, 3 and 8 to requested value.
While being at it, the reason for me needing this is that the spi_mpc83xx driver
was recently converted to a OF only driver so I have no way of defining my own
CS function anymore. While OF is good I don't feel that OF drivers should block the native
method, OF should be a layer on top of the native methods.
Jocke
Joakim Tjernlund/Transmode wrote on 13/07/2009 18:01:02:
>
> Anton Vorontsov <[email protected]> wrote on 13/07/2009 17:19:11:
> >
> > Hi all,
> >
> > I've been sitting on these patches for some time, but now it appears
> > that the set_sync() feature is needed elsewhere. So here are the
> > patches.
> >
> > Joakim, I think this is what you need.
> Yes, it sure looks so :) I will have to look closer later as
> I will be traveling the next few days.
hmm, one must define a new OF syntax (and update the spi_mpc83xx driver) to
use this(I think). Have you given this any thought?
Jocke
On Mon, Jul 13, 2009 at 06:01:02PM +0200, Joakim Tjernlund wrote:
>
> Anton Vorontsov <[email protected]> wrote on 13/07/2009 17:19:11:
> >
> > Hi all,
> >
> > I've been sitting on these patches for some time, but now it appears
> > that the set_sync() feature is needed elsewhere. So here are the
> > patches.
> >
> > Joakim, I think this is what you need.
>
> Yes, it sure looks so :) I will have to look closer later as
> I will be traveling the next few days.
>
> Question though, have you considered using a bitmask instead of
> an array:
> static void qe_gpio_set_sync(struct gpio_chip *gc, unsigned int num,
> unsigned int gpio_mask, unsigned int vals)
> If you want to set bit 0, 3 and 8 you would set positions 0, 3 and 8 in gpio_mask
> to ones. Similarly in vals, set bit positions 0, 3 and 8 to requested value.
Yeah, I thought about it. We could do the u64 masks (to handle up
to 64 bits parallel IO buses).
It's all easy with dumb memory-mapped GPIO controllers, because
we have a 8/16/32/64 bits registers with linear bit<->gpio mapping.
But some gpio controllers aren't that easy. I know at least one
(FPGA-based) gpio controller that won't change any GPIO lines
for real unless changes are "commited". The controller has several
banks (registers) of PIOs (total count > 64 bits), but you can commit
all the changes to the banks at once (synchronously). This isn't
because the controller is uber-cool, it's just the controller has
sequential IO. So with masks approach you won't able to use _sync()
calls that easily for all GPIOs range.
But OK, if we throw away the special cases, I can't imagine any
clear api for this approach, all I can think of is something
along these lines:
int num = 3;
u32 gpios[3];
u64 shifts[3];
/* this implies checks whether we can use _sync() */
if (!gpio_get_shifts(num, gpios, shifts))
return -EINVAL;
gpio_set_values_sync(chip, 1 << shifts[0] | 1 << shifts[1],
val0 << shifts[0] | val1 << shifts[1]).
We can implement it, if that's acceptable. But that's a bit
ugly, I think.
> While being at it, the reason for me needing this is that the spi_mpc83xx driver
> was recently converted to a OF only driver so I have no way of defining my own
> CS function anymore. While OF is good I don't feel that OF drivers should block the native
> method, OF should be a layer on top of the native methods.
Um, I don't get it. You have a mux, which is a sort of GPIO controller.
All you need to do is to write "of-gpio-mux" driver, that will get all
the needed gpios from the underlaying GPIO controller.
In the device tree it'll look like this:
muxed_gpio: gpio-controller {
#gpio-cells = <2>;
compatible = "board-gpio-mux", "generic-gpio-mux";
gpios = <&qe_pio_d 2 0 /* AD0 */
&qe_pio_d 17 0 /* AD1 */
&qe_pio_d 5 0>; /* AD2 */
gpio-controller;
};
spi-controller {
gpios = <&muxed_gpio 0 0
&muxed_gpio 1 0
&muxed_gpio 2 0
&muxed_gpio 3 0
&muxed_gpio 4 0
&muxed_gpio 5 0
&muxed_gpio 6 0
&muxed_gpio 7 0>;
spi-device@0 {
reg = <0>;
};
...
spi-device@7 {
reg = <0>;
};
};
So you don't have to modify the spi driver.
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
Anton Vorontsov <[email protected]> wrote on 13/07/2009 19:34:55:
>
> On Mon, Jul 13, 2009 at 06:01:02PM +0200, Joakim Tjernlund wrote:
> >
> > Anton Vorontsov <[email protected]> wrote on 13/07/2009 17:19:11:
> > >
> > > Hi all,
> > >
> > > I've been sitting on these patches for some time, but now it appears
> > > that the set_sync() feature is needed elsewhere. So here are the
> > > patches.
> > >
> > > Joakim, I think this is what you need.
> >
> > Yes, it sure looks so :) I will have to look closer later as
> > I will be traveling the next few days.
> >
> > Question though, have you considered using a bitmask instead of
> > an array:
> > static void qe_gpio_set_sync(struct gpio_chip *gc, unsigned int num,
> > unsigned int gpio_mask, unsigned int vals)
> > If you want to set bit 0, 3 and 8 you would set positions 0, 3 and 8 in gpio_mask
> > to ones. Similarly in vals, set bit positions 0, 3 and 8 to requested value.
>
> Yeah, I thought about it. We could do the u64 masks (to handle up
> to 64 bits parallel IO buses).
>
> It's all easy with dumb memory-mapped GPIO controllers, because
> we have a 8/16/32/64 bits registers with linear bit<->gpio mapping.
Yes, this is gpio ..
>
> But some gpio controllers aren't that easy. I know at least one
> (FPGA-based) gpio controller that won't change any GPIO lines
> for real unless changes are "commited". The controller has several
> banks (registers) of PIOs (total count > 64 bits), but you can commit
> all the changes to the banks at once (synchronously). This isn't
> because the controller is uber-cool, it's just the controller has
> sequential IO. So with masks approach you won't able to use _sync()
> calls that easily for all GPIOs range.
.. but this one I am not sure qualify as gpio. Feels more like
its own device that should have its own driver.
>
> But OK, if we throw away the special cases, I can't imagine any
> clear api for this approach, all I can think of is something
> along these lines:
>
> int num = 3;
> u32 gpios[3];
> u64 shifts[3];
>
> /* this implies checks whether we can use _sync() */
> if (!gpio_get_shifts(num, gpios, shifts))
> return -EINVAL;
>
> gpio_set_values_sync(chip, 1 << shifts[0] | 1 << shifts[1],
> val0 << shifts[0] | val1 << shifts[1]).
>
> We can implement it, if that's acceptable. But that's a bit
> ugly, I think.
I see, that doesn't look good that either.
>
> > While being at it, the reason for me needing this is that the spi_mpc83xx driver
> > was recently converted to a OF only driver so I have no way of defining my own
> > CS function anymore. While OF is good I don't feel that OF drivers should
> block the native
> > method, OF should be a layer on top of the native methods.
>
> Um, I don't get it. You have a mux, which is a sort of GPIO controller.
> All you need to do is to write "of-gpio-mux" driver, that will get all
> the needed gpios from the underlaying GPIO controller.
Well, I already have a mux controller that is using the native spi methods. I
don't want to write a new one, far more complicated than what I got now.
While the OF system is very powerful and flexible I still think that
one should be able to use native SPI methods too. Why can they not
co-exist?
>
> In the device tree it'll look like this:
I must admit that I just started to look at the GPIO subsystem, but
I do wonder if mpc83xx_spi_cs_control() can do this muxing
without any change.
Very good example BTW.
Jocke
On Mon, Jul 13, 2009 at 09:59:54PM +0200, Joakim Tjernlund wrote:
[...]
> > > While being at it, the reason for me needing this is that the spi_mpc83xx driver
> > > was recently converted to a OF only driver so I have no way of defining my own
> > > CS function anymore. While OF is good I don't feel that OF drivers should
> > block the native
> > > method, OF should be a layer on top of the native methods.
> >
> > Um, I don't get it. You have a mux, which is a sort of GPIO controller.
> > All you need to do is to write "of-gpio-mux" driver, that will get all
> > the needed gpios from the underlaying GPIO controller.
>
> Well, I already have a mux controller that is using the native spi methods. I
> don't want to write a new one, far more complicated than what I got now.
> While the OF system is very powerful and flexible I still think that
> one should be able to use native SPI methods too. Why can they not
> co-exist?
I strongly believe that they can. You just need to post patches
so that we could see them, and maybe discuss how to do things
more generic. But if making things generic will be too hard (see
below), I don't think anybody will object applying a non-generic
solution (even permitting "legacy" bindings in the spi_8xxx driver).
But any users of the legacy bindings should be in-tree.
> > In the device tree it'll look like this:
>
> I must admit that I just started to look at the GPIO subsystem, but
> I do wonder if mpc83xx_spi_cs_control() can do this muxing
> without any change.
>
> Very good example BTW.
Well, there is one caveat: the "GPIOs" aren't independent,
so thinking about it more, I believe we should now discuss
"chip-select framework", not gpio controllers (it could be
that using gpio controllers for this purpose wasn't that
good idea).
http://www.mail-archive.com/[email protected]/msg34738.html
^^^ I'm dreaming about this framework. I.e. true addressing
for chip-selects. :-)
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
-----Anton Vorontsov <[email protected]> skrev: -----
>Till: Joakim Tjernlund <[email protected]>
>Fr?n: Anton Vorontsov <[email protected]>
>Datum: 07/14/2009 00:20
>Kopia: David Brownell <[email protected]>,
>[email protected], [email protected]
>?rende: Re: [PATCH 0/2] Setting GPIOs simultaneously
>
>
>On Mon, Jul 13, 2009 at 09:59:54PM +0200, Joakim Tjernlund wrote:
>[...]
>> > > While being at it, the reason for me needing this is that the spi_mpc83xx
>driver
>> > > was recently converted to a OF only driver so I have no way of defining
>my own
>> > > CS function anymore. While OF is good I don't feel that OF drivers
>should
>> > block the native
>> > > method, OF should be a layer on top of the native methods.
>> >
>> > Um, I don't get it. You have a mux, which is a sort of GPIO controller.
>> > All you need to do is to write "of-gpio-mux" driver, that will get all
>> > the needed gpios from the underlaying GPIO controller.
>>
>> Well, I already have a mux controller that is using the native spi methods.
>I
>> don't want to write a new one, far more complicated than what I got now.
>> While the OF system is very powerful and flexible I still think that
>> one should be able to use native SPI methods too. Why can they not
>> co-exist?
>
>I strongly believe that they can. You just need to post patches
>so that we could see them, and maybe discuss how to do things
>more generic. But if making things generic will be too hard (see
>below), I don't think anybody will object applying a non-generic
>solution (even permitting "legacy" bindings in the spi_8xxx driver).
>
>But any users of the legacy bindings should be in-tree.
ehh, it was working until you made it OF only. Why do call the native
way legacy? It is the method all non OF arch uses.
>
>> > In the device tree it'll look like this:
>>
>> I must admit that I just started to look at the GPIO subsystem, but
>> I do wonder if mpc83xx_spi_cs_control() can do this muxing
>> without any change.
>>
>> Very good example BTW.
>
>Well, there is one caveat: the "GPIOs" aren't independent,
>so thinking about it more, I believe we should now discuss
>"chip-select framework", not gpio controllers (it could be
>that using gpio controllers for this purpose wasn't that
>good idea).
>
>http://www.mail-archive.com/[email protected]/msg34738.html
>^^^ I'm dreaming about this framework. I.e. true addressing
> for chip-selects. :-)
This is probably needed to support most SPI users out there, but until
such framework is in place I think the native methods need to stay, right?
As is now, SPI has regressed w.r.t earlier releases.
Jocke
BTW, I will be offline for a few days as of now.
>
>--
>Anton Vorontsov
>email: [email protected]
>irc://irc.freenode.net/bd2
On Tue, Jul 14, 2009 at 11:20:13PM +0200, Joakim Tjernlund wrote:
[...]
> >But any users of the legacy bindings should be in-tree.
>
> ehh, it was working until you made it OF only. Why do call the native
> way legacy? It is the method all non OF arch uses.
It's legacy because there are no in-tree users anymore. Nowadays
we're trying to pass all needed information via OF, and we're
trying to avoid ugly platform-dependant hacks. Your SPI scheme
can be easily described via OF, but sure, it's hard to implement
it with the current SPI/OF subsystem.
[...]
> >http://www.mail-archive.com/[email protected]/msg34738.html
> >^^^ I'm dreaming about this framework. I.e. true addressing
> > for chip-selects. :-)
>
> This is probably needed to support most SPI users out there, but until
> such framework is in place I think the native methods need to stay, right?
I'm not the right person to ask. I can only express my opinions.
The maintainer make final decision.
But if you ask for my opinion, I don't think that they should stay
unless we'll see a user in the mainline.
> As is now, SPI has regressed w.r.t earlier releases.
Yes and no. Yes, it has "regressed" for out-of-tree code, and no,
I don't feel sorry about that. :-)
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Jul 13, 2009, at 10:19 AM, Anton Vorontsov wrote:
> Hi all,
>
> I've been sitting on these patches for some time, but now it appears
> that the set_sync() feature is needed elsewhere. So here are the
> patches.
>
> Joakim, I think this is what you need.
>
> Thanks,
What this resolved or dropped?
- k
On Thu, Nov 05, 2009 at 08:49:38AM -0600, Kumar Gala wrote:
>
> On Jul 13, 2009, at 10:19 AM, Anton Vorontsov wrote:
>
> >Hi all,
> >
> >I've been sitting on these patches for some time, but now it appears
> >that the set_sync() feature is needed elsewhere. So here are the
> >patches.
> >
> >Joakim, I think this is what you need.
> >
> >Thanks,
>
> What this resolved or dropped?
That was something like an RFC. There are no mainline users for
this support (yet), so we can drop it for now.
Thanks,
p.s.
As David didn't comment on the patches, I'd assume there are no
strong objections against this feature if we'll want to mainline
the support in future. ;-)
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2