| Message ID | 1381503190-5733-1-git-send-email-jonas.jensen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 11, 2013 at 4:53 PM, Jonas Jensen <jonas.jensen@gmail.com> wrote: > I agree it is a bit strange GPIO control is divided in two > separate registers. Unfortunately I can't offer an explanation > because the documentation is not publicly available. > > The register responsible for doing enable/disable is located > at <0x98100100 0x4>, the clock register is very close at > <0x98100000 0x34>. If we don't know we have to guess. This layout makes me think that the IO-window at 0x98100000 is a power-clock-and-reset controller. It contains some register to latch the pins enable/disable them, or if this is even a clock gate? Are you sure about this? Is it now a gated clock, simply, so that this bit should be handled in the clock driver, i.e. this bit gets set by clk_enable() from the GPIO driver? I am very suspicious about this especially since the GPIO driver is lacking clk_get() and friends. If it's not a clock gate, and you are convinced that you must still reach out into this range I think you want something like this: syscon: syscon@98100000 { compatible = "syscon"; reg = <0x98100000 0x1000>; }; gpio: gpio@98700000 { gpio-controller; #gpio-cells = <2>; syscon = <&syscon>; compatible = "moxa,moxart-gpio"; reg = <0x98700000 0xC>, <0x98100100 0x4>; }; Then the driver can use something like: struct device_node *np = pdev->dev.of_node; struct device_node *syscon_np; struct regmap *regmap; int err; syscon_np = of_parse_phandle(np, "syscon", 0); if (!syscon_np) { pr_crit("no syscon node\n"); return -ENODEV; } regmap = syscon_node_to_regmap(syscon_np); if (!regmap) { pr_crit("could not locate syscon regmap\n"); return -ENODEV; } Then update the registers using regmap_update_bits() and friends. > I don't think gpio_poweroff driver is right for this hardware > because the pin is not connected to anything that can do reset. > The old 2.6.9 kernel driver uses timer poll with eventual call > to userspace. > > To test that it works, I added gpio_poweroff anyway, modified > with gpio_export() the pin can then be seen switching between > 0 and 1 (on "cat /sys/class/gpio/gpio25/value"). Hmmmm not quite following this... > +Required properties: > + > +- #gpio-cells : Should be 2, The first cell is the pin number and > + the second cell is used to specify polarity: > + 0 = active high > + 1 = active low Could reference <dt-bindings/gpio/gpio.h> I guess? Oh well, no big deal. The driver as such is looking nice but now I strongly suspect it should clk_get/clk_prepare/clk_enable ... etc. Yours, Linus Walleij
Thank you for the replies. On 11 October 2013 17:44, Linus Walleij <linus.walleij@linaro.org> wrote: >> The register responsible for doing enable/disable is located >> at <0x98100100 0x4>, the clock register is very close at >> <0x98100000 0x34>. > > If we don't know we have to guess. > > This layout makes me think that the IO-window at 0x98100000 is > a power-clock-and-reset controller. It contains some register > to latch the pins enable/disable them, or if this is even a clock > gate? Are you sure about this? Is it now a gated clock, simply, > so that this bit should be handled in the clock driver, i.e. > this bit gets set by clk_enable() from the GPIO driver? The IO-window at 0x98100000 contains registers that are read to determine PLL and APB clock frequencies. Sorry I don't know more than that. This is part of a pending patch adding the clock driver: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/203494.html Arnd made a similar comment suggesting syscon back when MMC mapped the same register: https://groups.google.com/d/msg/linux.kernel/eeS7vhMWMAc/zNYhzyKilh8J I think I prefer to have this in the clock driver opposed to using syscon. The one downside I can see, individual control of the pins would be lost? Does it make sense to enable all pins once? this is acceptable for at least UC-7112-LX, it doesn't need to disable/enable pins beyond the initial clk_enable(). I say the above because I currently have nothing that requires individual pin control. However, I removed one line from MMC that turned out to be unnecessary. That line directly access the "PMU" register disabling pins 10-17 with the following comment: " /* change I/O multiplexing to SD, so the GPIO 17-10 will be fail */ moxart_gpio_mp_clear(0xff << 10); " http://code.google.com/p/linux-2-6-9-moxart/source/browse/drivers/mmc/host/moxasd.c#619 >> I don't think gpio_poweroff driver is right for this hardware >> because the pin is not connected to anything that can do reset. >> The old 2.6.9 kernel driver uses timer poll with eventual call >> to userspace. >> >> To test that it works, I added gpio_poweroff anyway, modified >> with gpio_export() the pin can then be seen switching between >> 0 and 1 (on "cat /sys/class/gpio/gpio25/value"). > > Hmmmm not quite following this... I'll try to elaborate. What happens in gpio_poweroff driver does not look like something that can reset the hardware. Reset on UC-7112-LX is implemented using the same register as the watchdog, in platform code hooked up to arm_pm_restart. The old sources "solved" this by polling the reset pin with eventual call_usermodehelper (/sbin/reboot): http://code.google.com/p/linux-2-6-9-moxart/source/browse/drivers/char/moxa_watchdog.c#174 What was previously in a kernel driver, would now be solved in userspace? gpio_export() allowed me to verify the pin number, pressing reset toggles the value. Adding the gpio-leds driver, that pin was automatically exported to sysfs, that got me thinking: How do I export the reset button to sysfs? Should gpio_export() be added to platform code? from drivers/power/reset/gpio-poweroff.c: static void gpio_poweroff_do_poweroff(void) { BUG_ON(!gpio_is_valid(gpio_num)); /* drive it active, also inactive->active edge */ gpio_direction_output(gpio_num, !gpio_active_low); mdelay(100); /* drive inactive, also active->inactive edge */ gpio_set_value(gpio_num, gpio_active_low); mdelay(100); /* drive it active, also inactive->active edge */ gpio_set_value(gpio_num, !gpio_active_low); /* give it some time */ mdelay(3000); WARN_ON(1); } Regards, Jonas
On Mon, Oct 14, 2013 at 1:15 PM, Jonas Jensen <jonas.jensen@gmail.com> wrote: > On 11 October 2013 17:44, Linus Walleij <linus.walleij@linaro.org> wrote: >>> The register responsible for doing enable/disable is located >>> at <0x98100100 0x4>, the clock register is very close at >>> <0x98100000 0x34>. (...) > Arnd made a similar comment suggesting syscon back when MMC mapped the > same register: > https://groups.google.com/d/msg/linux.kernel/eeS7vhMWMAc/zNYhzyKilh8J > > I think I prefer to have this in the clock driver opposed to using syscon. OK I can live with this. I've seen both approaches in practice. > The one downside I can see, individual control of the pins would be > lost? Does it make sense to enable all pins once? this is acceptable > for at least UC-7112-LX, it doesn't need to disable/enable pins beyond > the initial clk_enable(). > > I say the above because I currently have nothing that requires > individual pin control. However, I removed one line from MMC that > turned out to be unnecessary. That line directly access the "PMU" > register disabling pins 10-17 with the following comment: > > " > /* change I/O multiplexing to SD, so the GPIO 17-10 will be fail */ > moxart_gpio_mp_clear(0xff << 10); > " > http://code.google.com/p/linux-2-6-9-moxart/source/browse/drivers/mmc/host/moxasd.c#619 Sorry I don't fully follow this. But don't poke into pin control registers from other drivers, for separation of concerns. Request and handle pins through the pin control API. What you mean is likely that power-on states and boot loaders have set up the pins for your immediate use cases and everything seems to work, that is of course nice. However as development and requirements progress people start to come in with complex usecases where devices need pins set to states that differ from the power-on defaults, for example for power management. And then you run into this. When/if you need pin control you can wither implement that as a separate driver which is used by this GPIO driver as a "back-end" or you can move this driver over to drivers/pinctrl and extend it with a pin control API there, making it a combined pin control and GPIO driver. >>> I don't think gpio_poweroff driver is right for this hardware >>> because the pin is not connected to anything that can do reset. >>> The old 2.6.9 kernel driver uses timer poll with eventual call >>> to userspace. >>> >>> To test that it works, I added gpio_poweroff anyway, modified >>> with gpio_export() the pin can then be seen switching between >>> 0 and 1 (on "cat /sys/class/gpio/gpio25/value"). >> >> Hmmmm not quite following this... > > I'll try to elaborate. What happens in gpio_poweroff driver does not > look like something that can reset the hardware. Reset on UC-7112-LX > is implemented using the same register as the watchdog, in platform > code hooked up to arm_pm_restart. > > The old sources "solved" this by polling the reset pin with eventual > call_usermodehelper (/sbin/reboot): > http://code.google.com/p/linux-2-6-9-moxart/source/browse/drivers/char/moxa_watchdog.c#174 > > What was previously in a kernel driver, would now be solved in userspace? > > gpio_export() allowed me to verify the pin number, pressing reset > toggles the value. > > Adding the gpio-leds driver, that pin was automatically exported to > sysfs, that got me thinking: > > How do I export the reset button to sysfs? Should gpio_export() be > added to platform code? Aha argh what a mess. No the GPIO poweroff driver is not intended to fix this. I *think* you should do this: - Register the gpio pin as a polled input device using drivers/input/keyboard/gpio_keys_polled.c - Have it emit KEY_POWER (from include/uapi/linux/input.h) - Have your userspace input event (whatever issues select() on the /dev/input/* stuff) react to this input by calling /sbin/reboot or issueing the same thing that does, i.e. call reboot() with magic 0x01234567 I think. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/gpio/moxa,moxart-gpio.txt b/Documentation/devicetree/bindings/gpio/moxa,moxart-gpio.txt new file mode 100644 index 0000000..5039e17 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/moxa,moxart-gpio.txt @@ -0,0 +1,22 @@ +MOXA ART GPIO Controller + +Required properties: + +- #gpio-cells : Should be 2, The first cell is the pin number and + the second cell is used to specify polarity: + 0 = active high + 1 = active low +- compatible : Must be "moxa,moxart-gpio" +- reg : Should contain registers location and length + index 0 : input, output, and direction control + index 1 : enable/disable individual pins, pin 0-31 + +Example: + + gpio: gpio@98700000 { + gpio-controller; + #gpio-cells = <2>; + compatible = "moxa,moxart-gpio"; + reg = <0x98700000 0xC>, + <0x98100100 0x4>; + }; diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index c8b02a5..c5a2767 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -156,6 +156,13 @@ config GPIO_F7188X To compile this driver as a module, choose M here: the module will be called f7188x-gpio. +config GPIO_MOXART + bool "MOXART GPIO support" + depends on ARCH_MOXART + help + Select this option to enable GPIO driver for + MOXA ART SoC devices. + config GPIO_MPC5200 def_bool y depends on PPC_MPC52xx diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 5c353df..a26029d 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -46,6 +46,7 @@ obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o obj-$(CONFIG_GPIO_MCP23S08) += gpio-mcp23s08.o obj-$(CONFIG_GPIO_ML_IOH) += gpio-ml-ioh.o obj-$(CONFIG_GPIO_MM_LANTIQ) += gpio-mm-lantiq.o +obj-$(CONFIG_GPIO_MOXART) += gpio-moxart.o obj-$(CONFIG_GPIO_MPC5200) += gpio-mpc5200.o obj-$(CONFIG_GPIO_MPC8XXX) += gpio-mpc8xxx.o obj-$(CONFIG_GPIO_MSIC) += gpio-msic.o diff --git a/drivers/gpio/gpio-moxart.c b/drivers/gpio/gpio-moxart.c new file mode 100644 index 0000000..5796846 --- /dev/null +++ b/drivers/gpio/gpio-moxart.c @@ -0,0 +1,163 @@ +/* + * MOXA ART SoCs GPIO driver. + * + * Copyright (C) 2013 Jonas Jensen + * + * Jonas Jensen <jonas.jensen@gmail.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/err.h> +#include <linux/init.h> +#include <linux/irq.h> +#include <linux/io.h> +#include <linux/gpio.h> +#include <linux/platform_device.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_gpio.h> +#include <linux/pinctrl/consumer.h> +#include <linux/delay.h> +#include <linux/timer.h> +#include <linux/bitops.h> + +#define GPIO_DATA_OUT 0x00 +#define GPIO_DATA_IN 0x04 +#define GPIO_PIN_DIRECTION 0x08 + +static void __iomem *moxart_pincontrol_base; +static void __iomem *moxart_gpio_base; + +void moxart_gpio_enable(u32 gpio) +{ + writel(readl(moxart_pincontrol_base) | gpio, moxart_pincontrol_base); +} + +void moxart_gpio_disable(u32 gpio) +{ + writel(readl(moxart_pincontrol_base) & ~gpio, moxart_pincontrol_base); +} + +static int moxart_gpio_request(struct gpio_chip *chip, unsigned offset) +{ + moxart_gpio_enable(BIT(offset)); + return pinctrl_request_gpio(offset); +} + +static void moxart_gpio_free(struct gpio_chip *chip, unsigned offset) +{ + pinctrl_free_gpio(offset); + moxart_gpio_disable(BIT(offset)); +} + +static int moxart_gpio_direction_input(struct gpio_chip *chip, unsigned offset) +{ + void __iomem *ioaddr = moxart_gpio_base + GPIO_PIN_DIRECTION; + + writel(readl(ioaddr) & ~BIT(offset), ioaddr); + return 0; +} + +static int moxart_gpio_direction_output(struct gpio_chip *chip, + unsigned offset, int value) +{ + void __iomem *ioaddr = moxart_gpio_base + GPIO_PIN_DIRECTION; + + writel(readl(ioaddr) | BIT(offset), ioaddr); + return 0; +} + +static void moxart_gpio_set(struct gpio_chip *chip, unsigned offset, int value) +{ + void __iomem *ioaddr = moxart_gpio_base + GPIO_DATA_OUT; + u32 reg = readl(ioaddr); + + if (value) + reg = reg | BIT(offset); + else + reg = reg & ~BIT(offset); + + + writel(reg, ioaddr); +} + +static int moxart_gpio_get(struct gpio_chip *chip, unsigned offset) +{ + u32 ret = readl(moxart_gpio_base + GPIO_PIN_DIRECTION); + + if (ret & BIT(offset)) + return !!(readl(moxart_gpio_base + GPIO_DATA_OUT) & + BIT(offset)); + else + return !!(readl(moxart_gpio_base + GPIO_DATA_IN) & + BIT(offset)); +} + +static struct gpio_chip moxart_gpio_chip = { + .label = "moxart-gpio", + .request = moxart_gpio_request, + .free = moxart_gpio_free, + .direction_input = moxart_gpio_direction_input, + .direction_output = moxart_gpio_direction_output, + .set = moxart_gpio_set, + .get = moxart_gpio_get, + .base = 0, + .ngpio = 32, + .can_sleep = 0, +}; + +static int moxart_gpio_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct resource *res; + int ret; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + moxart_gpio_base = devm_ioremap_resource(dev, res); + if (IS_ERR(moxart_gpio_base)) { + dev_err(dev, "%s: devm_ioremap_resource res_gpio failed\n", + dev->of_node->full_name); + return PTR_ERR(moxart_gpio_base); + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + moxart_pincontrol_base = devm_ioremap_resource(dev, res); + if (IS_ERR(moxart_pincontrol_base)) { + dev_err(dev, "%s: devm_ioremap_resource res_pmu failed\n", + dev->of_node->full_name); + return PTR_ERR(moxart_pincontrol_base); + } + + moxart_gpio_chip.dev = dev; + + ret = gpiochip_add(&moxart_gpio_chip); + if (ret) { + dev_err(dev, "%s: gpiochip_add failed\n", + dev->of_node->full_name); + return ret; + } + + return 0; +} + +static const struct of_device_id moxart_gpio_match[] = { + { .compatible = "moxa,moxart-gpio" }, + { } +}; + +static struct platform_driver moxart_gpio_driver = { + .driver = { + .name = "moxart-gpio", + .owner = THIS_MODULE, + .of_match_table = moxart_gpio_match, + }, + .probe = moxart_gpio_probe, +}; +module_platform_driver(moxart_gpio_driver); + +MODULE_DESCRIPTION("MOXART GPIO chip driver"); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Jonas Jensen <jonas.jensen@gmail.com>");
Add GPIO driver for MOXA ART SoCs. Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com> --- Notes: Thanks for the replies, I agree it is a bit strange GPIO control is divided in two separate registers. Unfortunately I can't offer an explanation because the documentation is not publicly available. The register responsible for doing enable/disable is located at <0x98100100 0x4>, the clock register is very close at <0x98100000 0x34>. I don't think gpio_poweroff driver is right for this hardware because the pin is not connected to anything that can do reset. The old 2.6.9 kernel driver uses timer poll with eventual call to userspace. To test that it works, I added gpio_poweroff anyway, modified with gpio_export() the pin can then be seen switching between 0 and 1 (on "cat /sys/class/gpio/gpio25/value"). The DT file I use on UC-7112-LX: clk_pll: pll@98100000 { compatible = "moxa,moxart-pll-clock"; #clock-cells = <0>; reg = <0x98100000 0x34>; clocks = <&ref12>; }; clk_apb: clk_apb@98100000 { compatible = "moxa,moxart-apb-clock"; #clock-cells = <0>; reg = <0x98100000 0x34>; clocks = <&clk_pll>; }; gpio: gpio@98700000 { gpio-controller; #gpio-cells = <2>; compatible = "moxa,moxart-gpio"; reg = <0x98700000 0xC>, <0x98100100 0x4>; }; leds { compatible = "gpio-leds"; user-led { label = "ready-led"; gpios = <&gpio 27 0x1>; default-state = "on"; linux,default-trigger = "default-on"; }; }; gpio_poweroff { compatible = "gpio-poweroff"; pinctrl-names = "default"; input = <1>; gpios = <&gpio 25 0x0>; }; Changes since v4: 1. elaborate DT binding #gpio-cells description 2. remove ready-led / reset-switch from driver and binding 3. use BIT() macro 4. remove ternary operator in moxart_gpio_set() 5. use !!(condition) construct in moxart_gpio_get() 6. replace postcore_initcall() with module_platform_driver() 7. return gpiochip_add() return value on failure Applies to next-20130927 .../devicetree/bindings/gpio/moxa,moxart-gpio.txt | 22 +++ drivers/gpio/Kconfig | 7 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-moxart.c | 163 +++++++++++++++++++++ 4 files changed, 193 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/moxa,moxart-gpio.txt create mode 100644 drivers/gpio/gpio-moxart.c