* [PATCH v2 1/5] pinctrl: imx: detect uninitialized pins
2014-09-23 17:37 [PATCH v2 0/5] vf610: Add GPIO support Stefan Agner
@ 2014-09-23 17:37 ` Stefan Agner
2014-09-25 2:15 ` Shawn Guo
2014-09-23 17:37 ` [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610 Stefan Agner
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2014-09-23 17:37 UTC (permalink / raw)
To: linus.walleij, gnurou, shawn.guo, kernel
Cc: linux-gpio, linux-arm-kernel, linux-kernel, bpringlemeir, stefan
The pinctrl driver initialized the register offsets for the pins
with 0. On Vybrid an offset of 0 is a valid offset for the pinctrl
mux register. So far, this was solved using the ZERO_OFFSET_VALID
flag which allowed offsets of 0. However, this does not allow to
verify whether a pins struct imx_pmx_func was initialized or not.
Use signed offset values for register offsets and initialize those
with -1 in order to detect uninitialized offset values reliable.
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
drivers/pinctrl/pinctrl-imx.c | 9 +++++----
drivers/pinctrl/pinctrl-imx.h | 7 +++----
drivers/pinctrl/pinctrl-vf610.c | 2 +-
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-imx.c b/drivers/pinctrl/pinctrl-imx.c
index 946d594..0d4558b 100644
--- a/drivers/pinctrl/pinctrl-imx.c
+++ b/drivers/pinctrl/pinctrl-imx.c
@@ -204,7 +204,7 @@ static int imx_pmx_enable(struct pinctrl_dev *pctldev, unsigned selector,
pin_id = pin->pin;
pin_reg = &info->pin_regs[pin_id];
- if (!(info->flags & ZERO_OFFSET_VALID) && !pin_reg->mux_reg) {
+ if (pin_reg->mux_reg == -1) {
dev_err(ipctl->dev, "Pin(%s) does not support mux function\n",
info->pins[pin_id].name);
return -EINVAL;
@@ -308,7 +308,7 @@ static int imx_pinconf_get(struct pinctrl_dev *pctldev,
const struct imx_pinctrl_soc_info *info = ipctl->info;
const struct imx_pin_reg *pin_reg = &info->pin_regs[pin_id];
- if (!(info->flags & ZERO_OFFSET_VALID) && !pin_reg->conf_reg) {
+ if (pin_reg->conf_reg == -1) {
dev_err(info->dev, "Pin(%s) does not support config function\n",
info->pins[pin_id].name);
return -EINVAL;
@@ -331,7 +331,7 @@ static int imx_pinconf_set(struct pinctrl_dev *pctldev,
const struct imx_pin_reg *pin_reg = &info->pin_regs[pin_id];
int i;
- if (!(info->flags & ZERO_OFFSET_VALID) && !pin_reg->conf_reg) {
+ if (pin_reg->conf_reg == -1) {
dev_err(info->dev, "Pin(%s) does not support config function\n",
info->pins[pin_id].name);
return -EINVAL;
@@ -586,10 +586,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
if (!ipctl)
return -ENOMEM;
- info->pin_regs = devm_kzalloc(&pdev->dev, sizeof(*info->pin_regs) *
+ info->pin_regs = devm_kmalloc(&pdev->dev, sizeof(*info->pin_regs) *
info->npins, GFP_KERNEL);
if (!info->pin_regs)
return -ENOMEM;
+ memset(info->pin_regs, 0xff, sizeof(*info->pin_regs) * info->npins);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
ipctl->base = devm_ioremap_resource(&pdev->dev, res);
diff --git a/drivers/pinctrl/pinctrl-imx.h b/drivers/pinctrl/pinctrl-imx.h
index db408b0..49e55d3 100644
--- a/drivers/pinctrl/pinctrl-imx.h
+++ b/drivers/pinctrl/pinctrl-imx.h
@@ -67,8 +67,8 @@ struct imx_pmx_func {
* @conf_reg: config register offset
*/
struct imx_pin_reg {
- u16 mux_reg;
- u16 conf_reg;
+ s16 mux_reg;
+ s16 conf_reg;
};
struct imx_pinctrl_soc_info {
@@ -83,8 +83,7 @@ struct imx_pinctrl_soc_info {
unsigned int flags;
};
-#define ZERO_OFFSET_VALID 0x1
-#define SHARE_MUX_CONF_REG 0x2
+#define SHARE_MUX_CONF_REG 0x1
#define NO_MUX 0x0
#define NO_PAD 0x0
diff --git a/drivers/pinctrl/pinctrl-vf610.c b/drivers/pinctrl/pinctrl-vf610.c
index bddd913..b788e15 100644
--- a/drivers/pinctrl/pinctrl-vf610.c
+++ b/drivers/pinctrl/pinctrl-vf610.c
@@ -299,7 +299,7 @@ static const struct pinctrl_pin_desc vf610_pinctrl_pads[] = {
static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
.pins = vf610_pinctrl_pads,
.npins = ARRAY_SIZE(vf610_pinctrl_pads),
- .flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
+ .flags = SHARE_MUX_CONF_REG,
};
static struct of_device_id vf610_pinctrl_of_match[] = {
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 1/5] pinctrl: imx: detect uninitialized pins
2014-09-23 17:37 ` [PATCH v2 1/5] pinctrl: imx: detect uninitialized pins Stefan Agner
@ 2014-09-25 2:15 ` Shawn Guo
0 siblings, 0 replies; 17+ messages in thread
From: Shawn Guo @ 2014-09-25 2:15 UTC (permalink / raw)
To: Stefan Agner
Cc: linus.walleij, gnurou, kernel, linux-gpio, linux-arm-kernel,
linux-kernel, bpringlemeir
On Tue, Sep 23, 2014 at 07:37:53PM +0200, Stefan Agner wrote:
> The pinctrl driver initialized the register offsets for the pins
> with 0. On Vybrid an offset of 0 is a valid offset for the pinctrl
> mux register. So far, this was solved using the ZERO_OFFSET_VALID
> flag which allowed offsets of 0. However, this does not allow to
> verify whether a pins struct imx_pmx_func was initialized or not.
>
> Use signed offset values for register offsets and initialize those
> with -1 in order to detect uninitialized offset values reliable.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
Acked-by: Shawn Guo <shawn.guo@freescale.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610
2014-09-23 17:37 [PATCH v2 0/5] vf610: Add GPIO support Stefan Agner
2014-09-23 17:37 ` [PATCH v2 1/5] pinctrl: imx: detect uninitialized pins Stefan Agner
@ 2014-09-23 17:37 ` Stefan Agner
2014-09-25 2:47 ` Shawn Guo
2014-09-23 17:37 ` [PATCH v2 3/5] gpio: vf610: add gpiolib/IRQ chip driver for Vybrid Stefan Agner
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2014-09-23 17:37 UTC (permalink / raw)
To: linus.walleij, gnurou, shawn.guo, kernel
Cc: linux-gpio, linux-arm-kernel, linux-kernel, bpringlemeir, stefan
Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller.
This is needed since direction configuration is not part of the
GPIO module in Vybrid.
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
drivers/pinctrl/pinctrl-imx.c | 54 +++++++++++++++++++++++++++++++++++++++++
drivers/pinctrl/pinctrl-imx.h | 1 +
drivers/pinctrl/pinctrl-vf610.c | 2 +-
3 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/pinctrl-imx.c b/drivers/pinctrl/pinctrl-imx.c
index 0d4558b..64d1b59 100644
--- a/drivers/pinctrl/pinctrl-imx.c
+++ b/drivers/pinctrl/pinctrl-imx.c
@@ -294,10 +294,59 @@ static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
return 0;
}
+static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
+ struct pinctrl_gpio_range *range, unsigned offset)
+{
+ struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+ const struct imx_pinctrl_soc_info *info = ipctl->info;
+ const struct imx_pin_reg *pin_reg;
+ u32 reg;
+
+ if (!(info->flags & GPIO_CONTROL))
+ return -EINVAL;
+
+ pin_reg = &info->pin_regs[offset];
+ if (pin_reg->mux_reg == -1)
+ return -EINVAL;
+
+ reg = readl(ipctl->base + pin_reg->mux_reg);
+ reg &= ~(0x7 << 20);
+ writel(reg, ipctl->base + pin_reg->mux_reg);
+
+ return 0;
+}
+
+static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+ struct pinctrl_gpio_range *range, unsigned offset, bool input)
+{
+ struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+ const struct imx_pinctrl_soc_info *info = ipctl->info;
+ const struct imx_pin_reg *pin_reg;
+ u32 reg;
+
+ if (!(info->flags & GPIO_CONTROL))
+ return -EINVAL;
+
+ pin_reg = &info->pin_regs[offset];
+ if (pin_reg->mux_reg == -1)
+ return -EINVAL;
+
+ reg = readl(ipctl->base + pin_reg->mux_reg);
+ if (input)
+ reg &= ~0x2;
+ else
+ reg |= 0x2;
+ writel(reg, ipctl->base + pin_reg->mux_reg);
+
+ return 0;
+}
+
static const struct pinmux_ops imx_pmx_ops = {
.get_functions_count = imx_pmx_get_funcs_count,
.get_function_name = imx_pmx_get_func_name,
.get_function_groups = imx_pmx_get_groups,
+ .gpio_request_enable = imx_pmx_gpio_request_enable,
+ .gpio_set_direction = imx_pmx_gpio_set_direction,
.enable = imx_pmx_enable,
};
@@ -579,6 +628,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
dev_err(&pdev->dev, "wrong pinctrl info\n");
return -EINVAL;
}
+
+ /* GPIO control functions only intended for shared mux/conf register */
+ if (info->flags & GPIO_CONTROL)
+ BUG_ON(!(info->flags & SHARE_MUX_CONF_REG));
+
info->dev = &pdev->dev;
/* Create state holders etc for this driver */
diff --git a/drivers/pinctrl/pinctrl-imx.h b/drivers/pinctrl/pinctrl-imx.h
index 49e55d3..8f37ca4 100644
--- a/drivers/pinctrl/pinctrl-imx.h
+++ b/drivers/pinctrl/pinctrl-imx.h
@@ -84,6 +84,7 @@ struct imx_pinctrl_soc_info {
};
#define SHARE_MUX_CONF_REG 0x1
+#define GPIO_CONTROL 0x2
#define NO_MUX 0x0
#define NO_PAD 0x0
diff --git a/drivers/pinctrl/pinctrl-vf610.c b/drivers/pinctrl/pinctrl-vf610.c
index b788e15..fdf5661 100644
--- a/drivers/pinctrl/pinctrl-vf610.c
+++ b/drivers/pinctrl/pinctrl-vf610.c
@@ -299,7 +299,7 @@ static const struct pinctrl_pin_desc vf610_pinctrl_pads[] = {
static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
.pins = vf610_pinctrl_pads,
.npins = ARRAY_SIZE(vf610_pinctrl_pads),
- .flags = SHARE_MUX_CONF_REG,
+ .flags = SHARE_MUX_CONF_REG | GPIO_CONTROL,
};
static struct of_device_id vf610_pinctrl_of_match[] = {
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610
2014-09-23 17:37 ` [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610 Stefan Agner
@ 2014-09-25 2:47 ` Shawn Guo
2014-09-25 7:00 ` Stefan Agner
0 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2014-09-25 2:47 UTC (permalink / raw)
To: Stefan Agner
Cc: linus.walleij, gnurou, kernel, linux-gpio, linux-arm-kernel,
linux-kernel, bpringlemeir
On Tue, Sep 23, 2014 at 07:37:54PM +0200, Stefan Agner wrote:
> Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller.
> This is needed since direction configuration is not part of the
> GPIO module in Vybrid.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> drivers/pinctrl/pinctrl-imx.c | 54 +++++++++++++++++++++++++++++++++++++++++
> drivers/pinctrl/pinctrl-imx.h | 1 +
> drivers/pinctrl/pinctrl-vf610.c | 2 +-
> 3 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/pinctrl-imx.c b/drivers/pinctrl/pinctrl-imx.c
> index 0d4558b..64d1b59 100644
> --- a/drivers/pinctrl/pinctrl-imx.c
> +++ b/drivers/pinctrl/pinctrl-imx.c
> @@ -294,10 +294,59 @@ static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
> return 0;
> }
>
> +static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
> + struct pinctrl_gpio_range *range, unsigned offset)
> +{
> + struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> + const struct imx_pinctrl_soc_info *info = ipctl->info;
> + const struct imx_pin_reg *pin_reg;
> + u32 reg;
> +
> + if (!(info->flags & GPIO_CONTROL))
> + return -EINVAL;
> +
> + pin_reg = &info->pin_regs[offset];
> + if (pin_reg->mux_reg == -1)
> + return -EINVAL;
> +
> + reg = readl(ipctl->base + pin_reg->mux_reg);
> + reg &= ~(0x7 << 20);
> + writel(reg, ipctl->base + pin_reg->mux_reg);
Isn't this setup redundant at all, since imx_pmx_enable() already takes
care of setting mux register including GPIO mode?
> +
> + return 0;
> +}
> +
> +static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
> + struct pinctrl_gpio_range *range, unsigned offset, bool input)
> +{
> + struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> + const struct imx_pinctrl_soc_info *info = ipctl->info;
> + const struct imx_pin_reg *pin_reg;
> + u32 reg;
> +
> + if (!(info->flags & GPIO_CONTROL))
> + return -EINVAL;
> +
> + pin_reg = &info->pin_regs[offset];
> + if (pin_reg->mux_reg == -1)
> + return -EINVAL;
> +
> + reg = readl(ipctl->base + pin_reg->mux_reg);
> + if (input)
> + reg &= ~0x2;
> + else
> + reg |= 0x2;
This is all about Output Buffer Enable (OBE) bit. What about Input
Buffer Enable (IBE) bit? Don't we need to set or clear it as per GPIO
direction as well?
> + writel(reg, ipctl->base + pin_reg->mux_reg);
> +
> + return 0;
> +}
> +
> static const struct pinmux_ops imx_pmx_ops = {
> .get_functions_count = imx_pmx_get_funcs_count,
> .get_function_name = imx_pmx_get_func_name,
> .get_function_groups = imx_pmx_get_groups,
> + .gpio_request_enable = imx_pmx_gpio_request_enable,
> + .gpio_set_direction = imx_pmx_gpio_set_direction,
> .enable = imx_pmx_enable,
> };
>
> @@ -579,6 +628,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
> dev_err(&pdev->dev, "wrong pinctrl info\n");
> return -EINVAL;
> }
> +
> + /* GPIO control functions only intended for shared mux/conf register */
> + if (info->flags & GPIO_CONTROL)
> + BUG_ON(!(info->flags & SHARE_MUX_CONF_REG));
> +
If this is always true, why don't we just use flag SHARE_MUX_CONF_REG
and save GPIO_CONTROL? This check doesn't make too much sense to me if
we choose to have a new flag for GPIO setup. IMO, we should probably
either drop the GPIO_CONTROL flag or the check.
Shawn
> info->dev = &pdev->dev;
>
> /* Create state holders etc for this driver */
> diff --git a/drivers/pinctrl/pinctrl-imx.h b/drivers/pinctrl/pinctrl-imx.h
> index 49e55d3..8f37ca4 100644
> --- a/drivers/pinctrl/pinctrl-imx.h
> +++ b/drivers/pinctrl/pinctrl-imx.h
> @@ -84,6 +84,7 @@ struct imx_pinctrl_soc_info {
> };
>
> #define SHARE_MUX_CONF_REG 0x1
> +#define GPIO_CONTROL 0x2
>
> #define NO_MUX 0x0
> #define NO_PAD 0x0
> diff --git a/drivers/pinctrl/pinctrl-vf610.c b/drivers/pinctrl/pinctrl-vf610.c
> index b788e15..fdf5661 100644
> --- a/drivers/pinctrl/pinctrl-vf610.c
> +++ b/drivers/pinctrl/pinctrl-vf610.c
> @@ -299,7 +299,7 @@ static const struct pinctrl_pin_desc vf610_pinctrl_pads[] = {
> static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
> .pins = vf610_pinctrl_pads,
> .npins = ARRAY_SIZE(vf610_pinctrl_pads),
> - .flags = SHARE_MUX_CONF_REG,
> + .flags = SHARE_MUX_CONF_REG | GPIO_CONTROL,
> };
>
> static struct of_device_id vf610_pinctrl_of_match[] = {
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610
2014-09-25 2:47 ` Shawn Guo
@ 2014-09-25 7:00 ` Stefan Agner
2014-09-25 9:07 ` Shawn Guo
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2014-09-25 7:00 UTC (permalink / raw)
To: Shawn Guo
Cc: linus.walleij, gnurou, kernel, linux-gpio, linux-arm-kernel,
linux-kernel, bpringlemeir
Am 2014-09-25 04:47, schrieb Shawn Guo:
> On Tue, Sep 23, 2014 at 07:37:54PM +0200, Stefan Agner wrote:
>> Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller.
>> This is needed since direction configuration is not part of the
>> GPIO module in Vybrid.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>> drivers/pinctrl/pinctrl-imx.c | 54 +++++++++++++++++++++++++++++++++++++++++
>> drivers/pinctrl/pinctrl-imx.h | 1 +
>> drivers/pinctrl/pinctrl-vf610.c | 2 +-
>> 3 files changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-imx.c b/drivers/pinctrl/pinctrl-imx.c
>> index 0d4558b..64d1b59 100644
>> --- a/drivers/pinctrl/pinctrl-imx.c
>> +++ b/drivers/pinctrl/pinctrl-imx.c
>> @@ -294,10 +294,59 @@ static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
>> return 0;
>> }
>>
>> +static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
>> + struct pinctrl_gpio_range *range, unsigned offset)
>> +{
>> + struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
>> + const struct imx_pinctrl_soc_info *info = ipctl->info;
>> + const struct imx_pin_reg *pin_reg;
>> + u32 reg;
>> +
>> + if (!(info->flags & GPIO_CONTROL))
>> + return -EINVAL;
>> +
>> + pin_reg = &info->pin_regs[offset];
>> + if (pin_reg->mux_reg == -1)
>> + return -EINVAL;
>> +
>> + reg = readl(ipctl->base + pin_reg->mux_reg);
>> + reg &= ~(0x7 << 20);
>> + writel(reg, ipctl->base + pin_reg->mux_reg);
>
> Isn't this setup redundant at all, since imx_pmx_enable() already takes
> care of setting mux register including GPIO mode?
>
Yes currently this is redundant, when a pinmux is actually applied. What
is the expected behaviour? Is a explicit pinmux necessary before we can
use GPIO? If not, maybe it would make more sense to use imx_pmx_enable
here to write all pinctrl settings?
>> +
>> + return 0;
>> +}
>> +
>> +static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
>> + struct pinctrl_gpio_range *range, unsigned offset, bool input)
>> +{
>> + struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
>> + const struct imx_pinctrl_soc_info *info = ipctl->info;
>> + const struct imx_pin_reg *pin_reg;
>> + u32 reg;
>> +
>> + if (!(info->flags & GPIO_CONTROL))
>> + return -EINVAL;
>> +
>> + pin_reg = &info->pin_regs[offset];
>> + if (pin_reg->mux_reg == -1)
>> + return -EINVAL;
>> +
>> + reg = readl(ipctl->base + pin_reg->mux_reg);
>> + if (input)
>> + reg &= ~0x2;
>> + else
>> + reg |= 0x2;
>
> This is all about Output Buffer Enable (OBE) bit. What about Input
> Buffer Enable (IBE) bit? Don't we need to set or clear it as per GPIO
> direction as well?
>
The leave the input buffer doesn't hurt, it allows to read back the
value which is actually "on the wire". If a pin is hard on GND, one can
actually see that.
>> + writel(reg, ipctl->base + pin_reg->mux_reg);
>> +
>> + return 0;
>> +}
>> +
>> static const struct pinmux_ops imx_pmx_ops = {
>> .get_functions_count = imx_pmx_get_funcs_count,
>> .get_function_name = imx_pmx_get_func_name,
>> .get_function_groups = imx_pmx_get_groups,
>> + .gpio_request_enable = imx_pmx_gpio_request_enable,
>> + .gpio_set_direction = imx_pmx_gpio_set_direction,
>> .enable = imx_pmx_enable,
>> };
>>
>> @@ -579,6 +628,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
>> dev_err(&pdev->dev, "wrong pinctrl info\n");
>> return -EINVAL;
>> }
>> +
>> + /* GPIO control functions only intended for shared mux/conf register */
>> + if (info->flags & GPIO_CONTROL)
>> + BUG_ON(!(info->flags & SHARE_MUX_CONF_REG));
>> +
>
> If this is always true, why don't we just use flag SHARE_MUX_CONF_REG
> and save GPIO_CONTROL? This check doesn't make too much sense to me if
> we choose to have a new flag for GPIO setup. IMO, we should probably
> either drop the GPIO_CONTROL flag or the check.
>
Well, this is always true because the vf610 driver configures both
configs. But when somebody accidentally enables GPIO_CONFIG without
understanding the implications... This was more meant like "don't try to
use the GPIO_CONTROL just like that, its Vybird specific".
But I'm ok to remove this runtime check, maybe a comment describing the
flags is more appropriate..?
> Shawn
>
>> info->dev = &pdev->dev;
>>
>> /* Create state holders etc for this driver */
>> diff --git a/drivers/pinctrl/pinctrl-imx.h b/drivers/pinctrl/pinctrl-imx.h
>> index 49e55d3..8f37ca4 100644
>> --- a/drivers/pinctrl/pinctrl-imx.h
>> +++ b/drivers/pinctrl/pinctrl-imx.h
>> @@ -84,6 +84,7 @@ struct imx_pinctrl_soc_info {
>> };
>>
>> #define SHARE_MUX_CONF_REG 0x1
>> +#define GPIO_CONTROL 0x2
>>
>> #define NO_MUX 0x0
>> #define NO_PAD 0x0
>> diff --git a/drivers/pinctrl/pinctrl-vf610.c b/drivers/pinctrl/pinctrl-vf610.c
>> index b788e15..fdf5661 100644
>> --- a/drivers/pinctrl/pinctrl-vf610.c
>> +++ b/drivers/pinctrl/pinctrl-vf610.c
>> @@ -299,7 +299,7 @@ static const struct pinctrl_pin_desc vf610_pinctrl_pads[] = {
>> static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
>> .pins = vf610_pinctrl_pads,
>> .npins = ARRAY_SIZE(vf610_pinctrl_pads),
>> - .flags = SHARE_MUX_CONF_REG,
>> + .flags = SHARE_MUX_CONF_REG | GPIO_CONTROL,
>> };
>>
>> static struct of_device_id vf610_pinctrl_of_match[] = {
>> --
>> 2.1.0
>>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610
2014-09-25 7:00 ` Stefan Agner
@ 2014-09-25 9:07 ` Shawn Guo
2014-09-25 9:36 ` Stefan Agner
0 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2014-09-25 9:07 UTC (permalink / raw)
To: Stefan Agner
Cc: linus.walleij, gnurou, kernel, linux-gpio, linux-arm-kernel,
linux-kernel, bpringlemeir
On Thu, Sep 25, 2014 at 09:00:41AM +0200, Stefan Agner wrote:
> Am 2014-09-25 04:47, schrieb Shawn Guo:
> > On Tue, Sep 23, 2014 at 07:37:54PM +0200, Stefan Agner wrote:
> >> Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller.
> >> This is needed since direction configuration is not part of the
> >> GPIO module in Vybrid.
> >>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >> drivers/pinctrl/pinctrl-imx.c | 54 +++++++++++++++++++++++++++++++++++++++++
> >> drivers/pinctrl/pinctrl-imx.h | 1 +
> >> drivers/pinctrl/pinctrl-vf610.c | 2 +-
> >> 3 files changed, 56 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pinctrl/pinctrl-imx.c b/drivers/pinctrl/pinctrl-imx.c
> >> index 0d4558b..64d1b59 100644
> >> --- a/drivers/pinctrl/pinctrl-imx.c
> >> +++ b/drivers/pinctrl/pinctrl-imx.c
> >> @@ -294,10 +294,59 @@ static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
> >> return 0;
> >> }
> >>
> >> +static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
> >> + struct pinctrl_gpio_range *range, unsigned offset)
> >> +{
> >> + struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> >> + const struct imx_pinctrl_soc_info *info = ipctl->info;
> >> + const struct imx_pin_reg *pin_reg;
> >> + u32 reg;
> >> +
> >> + if (!(info->flags & GPIO_CONTROL))
> >> + return -EINVAL;
> >> +
> >> + pin_reg = &info->pin_regs[offset];
> >> + if (pin_reg->mux_reg == -1)
> >> + return -EINVAL;
> >> +
> >> + reg = readl(ipctl->base + pin_reg->mux_reg);
> >> + reg &= ~(0x7 << 20);
> >> + writel(reg, ipctl->base + pin_reg->mux_reg);
> >
> > Isn't this setup redundant at all, since imx_pmx_enable() already takes
> > care of setting mux register including GPIO mode?
> >
>
> Yes currently this is redundant, when a pinmux is actually applied. What
> is the expected behaviour? Is a explicit pinmux necessary before we can
> use GPIO? If not, maybe it would make more sense to use imx_pmx_enable
> here to write all pinctrl settings?
Okay, as per Documentation/pinctrl.txt, it's required that GPIO and
PINCTRL can be used as orthogonal. That said, your code does the right
thing. Sorry for the noisy comment.
>
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
> >> + struct pinctrl_gpio_range *range, unsigned offset, bool input)
> >> +{
> >> + struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> >> + const struct imx_pinctrl_soc_info *info = ipctl->info;
> >> + const struct imx_pin_reg *pin_reg;
> >> + u32 reg;
> >> +
> >> + if (!(info->flags & GPIO_CONTROL))
> >> + return -EINVAL;
> >> +
> >> + pin_reg = &info->pin_regs[offset];
> >> + if (pin_reg->mux_reg == -1)
> >> + return -EINVAL;
> >> +
> >> + reg = readl(ipctl->base + pin_reg->mux_reg);
> >> + if (input)
> >> + reg &= ~0x2;
> >> + else
> >> + reg |= 0x2;
> >
> > This is all about Output Buffer Enable (OBE) bit. What about Input
> > Buffer Enable (IBE) bit? Don't we need to set or clear it as per GPIO
> > direction as well?
> >
>
> The leave the input buffer doesn't hurt, it allows to read back the
> value which is actually "on the wire". If a pin is hard on GND, one can
> actually see that.
Okay.
>
> >> + writel(reg, ipctl->base + pin_reg->mux_reg);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static const struct pinmux_ops imx_pmx_ops = {
> >> .get_functions_count = imx_pmx_get_funcs_count,
> >> .get_function_name = imx_pmx_get_func_name,
> >> .get_function_groups = imx_pmx_get_groups,
> >> + .gpio_request_enable = imx_pmx_gpio_request_enable,
> >> + .gpio_set_direction = imx_pmx_gpio_set_direction,
> >> .enable = imx_pmx_enable,
> >> };
> >>
> >> @@ -579,6 +628,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
> >> dev_err(&pdev->dev, "wrong pinctrl info\n");
> >> return -EINVAL;
> >> }
> >> +
> >> + /* GPIO control functions only intended for shared mux/conf register */
> >> + if (info->flags & GPIO_CONTROL)
> >> + BUG_ON(!(info->flags & SHARE_MUX_CONF_REG));
> >> +
> >
> > If this is always true, why don't we just use flag SHARE_MUX_CONF_REG
> > and save GPIO_CONTROL? This check doesn't make too much sense to me if
> > we choose to have a new flag for GPIO setup. IMO, we should probably
> > either drop the GPIO_CONTROL flag or the check.
> >
>
> Well, this is always true because the vf610 driver configures both
> configs. But when somebody accidentally enables GPIO_CONFIG without
> understanding the implications... This was more meant like "don't try to
> use the GPIO_CONTROL just like that, its Vybird specific".
But it will become a blocker if some day an i.MX controller (no flag
SHARE_MUX_CONF_REG) needs to use GPIO_CONFIG.
> But I'm ok to remove this runtime check, maybe a comment describing the
> flags is more appropriate..?
Sounds good.
Shawn
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610
2014-09-25 9:07 ` Shawn Guo
@ 2014-09-25 9:36 ` Stefan Agner
2014-09-25 16:43 ` Bill Pringlemeir
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2014-09-25 9:36 UTC (permalink / raw)
To: Shawn Guo
Cc: linus.walleij, gnurou, kernel, linux-gpio, linux-arm-kernel,
linux-kernel, bpringlemeir
Am 2014-09-25 11:07, schrieb Shawn Guo:
> On Thu, Sep 25, 2014 at 09:00:41AM +0200, Stefan Agner wrote:
>> Am 2014-09-25 04:47, schrieb Shawn Guo:
>> > On Tue, Sep 23, 2014 at 07:37:54PM +0200, Stefan Agner wrote:
>> >> Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller.
>> >> This is needed since direction configuration is not part of the
>> >> GPIO module in Vybrid.
>> >>
>> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> >> ---
>> >> drivers/pinctrl/pinctrl-imx.c | 54 +++++++++++++++++++++++++++++++++++++++++
>> >> drivers/pinctrl/pinctrl-imx.h | 1 +
>> >> drivers/pinctrl/pinctrl-vf610.c | 2 +-
>> >> 3 files changed, 56 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/pinctrl/pinctrl-imx.c b/drivers/pinctrl/pinctrl-imx.c
>> >> index 0d4558b..64d1b59 100644
>> >> --- a/drivers/pinctrl/pinctrl-imx.c
>> >> +++ b/drivers/pinctrl/pinctrl-imx.c
>> >> @@ -294,10 +294,59 @@ static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
>> >> return 0;
>> >> }
>> >>
>> >> +static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
>> >> + struct pinctrl_gpio_range *range, unsigned offset)
>> >> +{
>> >> + struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
>> >> + const struct imx_pinctrl_soc_info *info = ipctl->info;
>> >> + const struct imx_pin_reg *pin_reg;
>> >> + u32 reg;
>> >> +
>> >> + if (!(info->flags & GPIO_CONTROL))
>> >> + return -EINVAL;
>> >> +
>> >> + pin_reg = &info->pin_regs[offset];
>> >> + if (pin_reg->mux_reg == -1)
>> >> + return -EINVAL;
>> >> +
>> >> + reg = readl(ipctl->base + pin_reg->mux_reg);
>> >> + reg &= ~(0x7 << 20);
>> >> + writel(reg, ipctl->base + pin_reg->mux_reg);
>> >
>> > Isn't this setup redundant at all, since imx_pmx_enable() already takes
>> > care of setting mux register including GPIO mode?
>> >
>>
>> Yes currently this is redundant, when a pinmux is actually applied. What
>> is the expected behaviour? Is a explicit pinmux necessary before we can
>> use GPIO? If not, maybe it would make more sense to use imx_pmx_enable
>> here to write all pinctrl settings?
>
> Okay, as per Documentation/pinctrl.txt, it's required that GPIO and
> PINCTRL can be used as orthogonal. That said, your code does the right
> thing. Sorry for the noisy comment.
>
I'm happy you came up with this, since its the thing which I'm must
unsure whether this is right.
Right now, if you just export a random pin (which has no pinmux
configured in dt), you get an error since this function fails (mux_reg
== -1). But I guess we can't avoid it, we need the pinctrl framework to
have a valid setting before we fiddle around with the pad.
But now, if there is a pinmux configuration in dt, but its not applied
(using pinctrl-0 = ...), the export will succeed, however the GPIO will
not really be usable since no sane pad configuration is not applied
(input/output buffer disabled, no drive-strength)... So we might well
just don't change the mux here too.
IMHO, fully orthogonal is not possible, since on Vybrid those two depend
on each other. But in order to make it "more orthogonal", we maybe
should force applying the full configuration in
imx_pmx_gpio_request_enable (e.g. drive strenght etc), rather then only
mux the pad to to GPIO...
What do you think?
>>
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
>> >> + struct pinctrl_gpio_range *range, unsigned offset, bool input)
>> >> +{
>> >> + struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
>> >> + const struct imx_pinctrl_soc_info *info = ipctl->info;
>> >> + const struct imx_pin_reg *pin_reg;
>> >> + u32 reg;
>> >> +
>> >> + if (!(info->flags & GPIO_CONTROL))
>> >> + return -EINVAL;
>> >> +
>> >> + pin_reg = &info->pin_regs[offset];
>> >> + if (pin_reg->mux_reg == -1)
>> >> + return -EINVAL;
>> >> +
>> >> + reg = readl(ipctl->base + pin_reg->mux_reg);
>> >> + if (input)
>> >> + reg &= ~0x2;
>> >> + else
>> >> + reg |= 0x2;
>> >
>> > This is all about Output Buffer Enable (OBE) bit. What about Input
>> > Buffer Enable (IBE) bit? Don't we need to set or clear it as per GPIO
>> > direction as well?
>> >
>>
>> The leave the input buffer doesn't hurt, it allows to read back the
>> value which is actually "on the wire". If a pin is hard on GND, one can
>> actually see that.
>
> Okay.
>
>>
>> >> + writel(reg, ipctl->base + pin_reg->mux_reg);
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> static const struct pinmux_ops imx_pmx_ops = {
>> >> .get_functions_count = imx_pmx_get_funcs_count,
>> >> .get_function_name = imx_pmx_get_func_name,
>> >> .get_function_groups = imx_pmx_get_groups,
>> >> + .gpio_request_enable = imx_pmx_gpio_request_enable,
>> >> + .gpio_set_direction = imx_pmx_gpio_set_direction,
>> >> .enable = imx_pmx_enable,
>> >> };
>> >>
>> >> @@ -579,6 +628,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
>> >> dev_err(&pdev->dev, "wrong pinctrl info\n");
>> >> return -EINVAL;
>> >> }
>> >> +
>> >> + /* GPIO control functions only intended for shared mux/conf register */
>> >> + if (info->flags & GPIO_CONTROL)
>> >> + BUG_ON(!(info->flags & SHARE_MUX_CONF_REG));
>> >> +
>> >
>> > If this is always true, why don't we just use flag SHARE_MUX_CONF_REG
>> > and save GPIO_CONTROL? This check doesn't make too much sense to me if
>> > we choose to have a new flag for GPIO setup. IMO, we should probably
>> > either drop the GPIO_CONTROL flag or the check.
>> >
>>
>> Well, this is always true because the vf610 driver configures both
>> configs. But when somebody accidentally enables GPIO_CONFIG without
>> understanding the implications... This was more meant like "don't try to
>> use the GPIO_CONTROL just like that, its Vybird specific".
>
> But it will become a blocker if some day an i.MX controller (no flag
> SHARE_MUX_CONF_REG) needs to use GPIO_CONFIG.
>
>> But I'm ok to remove this runtime check, maybe a comment describing the
>> flags is more appropriate..?
>
> Sounds good.
>
> Shawn
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610
2014-09-25 9:36 ` Stefan Agner
@ 2014-09-25 16:43 ` Bill Pringlemeir
2014-09-26 10:50 ` Stefan Agner
0 siblings, 1 reply; 17+ messages in thread
From: Bill Pringlemeir @ 2014-09-25 16:43 UTC (permalink / raw)
To: Stefan Agner
Cc: Shawn Guo, linus.walleij, gnurou, kernel, linux-gpio,
linux-arm-kernel, linux-kernel
On 25 Sep 2014, stefan@agner.ch wrote:
> Am 2014-09-25 11:07, schrieb Shawn Guo:
>> On Thu, Sep 25, 2014 at 09:00:41AM +0200, Stefan Agner wrote:
>>> Am 2014-09-25 04:47, schrieb Shawn Guo:
>>>> On Tue, Sep 23, 2014 at 07:37:54PM +0200, Stefan Agner wrote:
>>>>> Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller.
>>>>> This is needed since direction configuration is not part of the
>>>>> GPIO module in Vybrid.
>>>>>
>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>> ---
[snip]
>>>>> + reg = readl(ipctl->base + pin_reg->mux_reg);
>>>>> + reg &= ~(0x7 << 20);
>>>>> + writel(reg, ipctl->base + pin_reg->mux_reg);
>>>> Isn't this setup redundant at all, since imx_pmx_enable() already
>>>> takes care of setting mux register including GPIO mode?
>>> Yes currently this is redundant, when a pinmux is actually
>>> applied. What is the expected behaviour? Is a explicit pinmux
>>> necessary before we can use GPIO? If not, maybe it would make more
>>> sense to use imx_pmx_enable here to write all pinctrl settings?
>> Okay, as per Documentation/pinctrl.txt, it's required that GPIO and
>> PINCTRL can be used as orthogonal. That said, your code does the
>> right thing. Sorry for the noisy comment.
> I'm happy you came up with this, since its the thing which I'm must
> unsure whether this is right.
> Right now, if you just export a random pin (which has no pinmux
> configured in dt), you get an error since this function fails (mux_reg
> == -1). But I guess we can't avoid it, we need the pinctrl framework
> to have a valid setting before we fiddle around with the pad.
> But now, if there is a pinmux configuration in dt, but its not applied
> (using pinctrl-0 = ...), the export will succeed, however the GPIO
> will not really be usable since no sane pad configuration is not
> applied (input/output buffer disabled, no drive-strength)... So we
> might well just don't change the mux here too.
> IMHO, fully orthogonal is not possible, since on Vybrid those two
> depend on each other. But in order to make it "more orthogonal", we
> maybe should force applying the full configuration in
> imx_pmx_gpio_request_enable (e.g. drive strenght etc), rather then
> only mux the pad to to GPIO...
> What do you think?
The muxing must have been done, that is correct. However, this could be
by a boot loader, by some other API, etc. People maybe concerned about
Linux affecting 'CAN' (or some 'mission critical' pins) muxing for
instance, but want the A5 to handle GPIO.
If somewhere, somehow the pin was muxed properly and the GPIO code still
works, I believe this is a win. I see that this flexibility makes it
more difficult to get something that just works. I think the device
trees take care of this for normal users. It might be appropriate to
add a DT node that sets 'GPIO_CONTROL' and put a note in the DT-GPIO
document, that pinctrl is needed for a normal case or if some external
muxing is used or not needed, then 'control=false' can be set (or
whatever good DT terminology)? Then the 'GPIO_CONTROL' check would be
in the 'vf610_gpio_direction_input()' functions. The 'pinctrl'
functions are currently compiled to stubs if that is configured out.
Then there is 'multi-machine' support with DT and compile time selects
with CONFIG_PINCTRL and I don't think there is a lot of additional code
or confusion?
Fwiw,
Bill Pringlemeir.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610
2014-09-25 16:43 ` Bill Pringlemeir
@ 2014-09-26 10:50 ` Stefan Agner
2014-09-29 15:05 ` Bill Pringlemeir
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2014-09-26 10:50 UTC (permalink / raw)
To: Bill Pringlemeir
Cc: Shawn Guo, linus.walleij, gnurou, kernel, linux-gpio,
linux-arm-kernel, linux-kernel
Am 2014-09-25 18:43, schrieb Bill Pringlemeir:
> On 25 Sep 2014, stefan@agner.ch wrote:
>
>> Am 2014-09-25 11:07, schrieb Shawn Guo:
>>> On Thu, Sep 25, 2014 at 09:00:41AM +0200, Stefan Agner wrote:
>>>> Am 2014-09-25 04:47, schrieb Shawn Guo:
>>>>> On Tue, Sep 23, 2014 at 07:37:54PM +0200, Stefan Agner wrote:
>
>>>>>> Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller.
>>>>>> This is needed since direction configuration is not part of the
>>>>>> GPIO module in Vybrid.
>>>>>>
>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>>> ---
>
> [snip]
>
>>>>>> + reg = readl(ipctl->base + pin_reg->mux_reg);
>>>>>> + reg &= ~(0x7 << 20);
>>>>>> + writel(reg, ipctl->base + pin_reg->mux_reg);
>
>>>>> Isn't this setup redundant at all, since imx_pmx_enable() already
>>>>> takes care of setting mux register including GPIO mode?
>
>>>> Yes currently this is redundant, when a pinmux is actually
>>>> applied. What is the expected behaviour? Is a explicit pinmux
>>>> necessary before we can use GPIO? If not, maybe it would make more
>>>> sense to use imx_pmx_enable here to write all pinctrl settings?
>
>>> Okay, as per Documentation/pinctrl.txt, it's required that GPIO and
>>> PINCTRL can be used as orthogonal. That said, your code does the
>>> right thing. Sorry for the noisy comment.
>
>> I'm happy you came up with this, since its the thing which I'm must
>> unsure whether this is right.
>
>> Right now, if you just export a random pin (which has no pinmux
>> configured in dt), you get an error since this function fails (mux_reg
>> == -1). But I guess we can't avoid it, we need the pinctrl framework
>> to have a valid setting before we fiddle around with the pad.
>
>> But now, if there is a pinmux configuration in dt, but its not applied
>> (using pinctrl-0 = ...), the export will succeed, however the GPIO
>> will not really be usable since no sane pad configuration is not
>> applied (input/output buffer disabled, no drive-strength)... So we
>> might well just don't change the mux here too.
>
>> IMHO, fully orthogonal is not possible, since on Vybrid those two
>> depend on each other. But in order to make it "more orthogonal", we
>> maybe should force applying the full configuration in
>> imx_pmx_gpio_request_enable (e.g. drive strenght etc), rather then
>> only mux the pad to to GPIO...
>
>> What do you think?
>
> The muxing must have been done, that is correct. However, this could be
> by a boot loader, by some other API, etc. People maybe concerned about
> Linux affecting 'CAN' (or some 'mission critical' pins) muxing for
> instance, but want the A5 to handle GPIO.
>
> If somewhere, somehow the pin was muxed properly and the GPIO code still
> works, I believe this is a win. I see that this flexibility makes it
> more difficult to get something that just works. I think the device
> trees take care of this for normal users. It might be appropriate to
> add a DT node that sets 'GPIO_CONTROL' and put a note in the DT-GPIO
> document, that pinctrl is needed for a normal case or if some external
> muxing is used or not needed, then 'control=false' can be set (or
> whatever good DT terminology)? Then the 'GPIO_CONTROL' check would be
> in the 'vf610_gpio_direction_input()' functions. The 'pinctrl'
> functions are currently compiled to stubs if that is configured out.
Currently pinctrl is not optional for Vybrid (Kconfig mandates pinctrl
by "select PINCTRL_VF610").
IMHO there is no real value having a way to mark a pin as "muxed
externally" (control=false). If we use a pin, we should configure and
mux that properly in Linux, and if we don't have valid configuration/mux
information, we should not touch that pin.
In v2 of this patchset, the GPIO code only worked when there _is_ a
pinctrl entry for that GPIO pin in the DT. This is because I need the
register offset, which is provided by the entry itself. But the pad
configuration is another point, which I did not consider in v2. But in
v3, the GPIO code also writes the pad control.
So it was never possible to just use the GPIO driver for any pin, even
that pin was correctly muxed and configured externally. And, especially
on Vybrid, I think this is a good thing, since the pin could be used by
the M4 core. One has to have at least a proper entry in the device tree.
And in v3, I also check that this entry represents a valid GPIO mux
(VF610_PAD_*GPIO*).
IMHO with v3 we have a quite good and safe solution:
- Using pins as GPIO needs a valid DT entry
- Using that pin only requires exporting the pin, all
configuration/muxing get applied by imx_pmx_gpio_request_enable
> Then there is 'multi-machine' support with DT and compile time selects
> with CONFIG_PINCTRL and I don't think there is a lot of additional code
> or confusion?
I don't understand that.
--
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610
2014-09-26 10:50 ` Stefan Agner
@ 2014-09-29 15:05 ` Bill Pringlemeir
2014-09-29 17:25 ` Stefan Agner
0 siblings, 1 reply; 17+ messages in thread
From: Bill Pringlemeir @ 2014-09-29 15:05 UTC (permalink / raw)
To: Stefan Agner
Cc: gnurou, linus.walleij, linux-kernel, linux-gpio, Shawn Guo,
kernel, linux-arm-kernel
>> On 25 Sep 2014, stefan@agner.ch wrote:
>>> IMHO, fully orthogonal is not possible, since on Vybrid those two
>>> depend on each other. But in order to make it "more orthogonal", we
>>> maybe should force applying the full configuration in
>>> imx_pmx_gpio_request_enable (e.g. drive strenght etc), rather then
>>> only mux the pad to to GPIO...
>>> What do you think?
> Am 2014-09-25 18:43, schrieb Bill Pringlemeir:
>> The muxing must have been done, that is correct. However, this could
>> be by a boot loader, by some other API, etc. People maybe concerned
>> about Linux affecting 'CAN' (or some 'mission critical' pins) muxing
>> for instance, but want the A5 to handle GPIO.
>>
>> If somewhere, somehow the pin was muxed properly and the GPIO code
>> still works, I believe this is a win. I see that this flexibility
>> makes it more difficult to get something that just works. I think
>> the device trees take care of this for normal users. It might be
>> appropriate to add a DT node that sets 'GPIO_CONTROL' and put a note
>> in the DT-GPIO document, that pinctrl is needed for a normal case or
>> if some external muxing is used or not needed, then 'control=false'
>> can be set (or whatever good DT terminology)? Then the
>> 'GPIO_CONTROL' check would be in the 'vf610_gpio_direction_input()'
>> functions. The 'pinctrl' functions are currently compiled to stubs
>> if that is configured out.
On 26 Sep 2014, stefan@agner.ch wrote:
> Currently pinctrl is not optional for Vybrid (Kconfig mandates pinctrl
> by "select PINCTRL_VF610").
> IMHO there is no real value having a way to mark a pin as "muxed
> externally" (control=false). If we use a pin, we should configure and
> mux that properly in Linux, and if we don't have valid
> configuration/mux information, we should not touch that pin.
I didn't mean per-pin, I meant for the GPIO controller as a whole. I
was also looking at a IOMUX controlled by the M4 and/or a boot loader.
I agree it is not possible with the mainline. This just makes it even
less possible. This is great if we know we are going that route.
> In v2 of this patchset, the GPIO code only worked when there _is_ a
> pinctrl entry for that GPIO pin in the DT. This is because I need the
> register offset, which is provided by the entry itself. But the pad
> configuration is another point, which I did not consider in v2. But in
> v3, the GPIO code also writes the pad control.
I was a little concerned about this as the pin may have pull-ups and
pull-downs. We would not want to dynamically change this. However, I
don't fully understand this path.
[snip]
>> Then there is 'multi-machine' support with DT and compile time
>> selects with CONFIG_PINCTRL and I don't think there is a lot of
>> additional code or confusion?
> I don't understand that.
I was thinking of a modified Vybrid which would allow us to not select
PINCTRL. As per Shawn's comment, it is suppose to be
optional/orthogonal to the GPIO controller. If someone made the change
to not select PINCTRL_VF610, then the GPIO code calls would currently
compile to stubs. To get the same things with a multi-machine
(Vybrid/Imx with/without pinctrl), then you could use the DT property
which would avoid the pinctrl calls.
The more required modules we require for each feature, the harder it is
to partition the AIPS peripherals between cores. It sort of makes the
mainline Linux only useful for the VF5xx non-secure parts.
I guess another option is to make another pinctrl module for the Vybrid
which uses some inter-core communications to alter the MUX
configuration. This makes sense for the pinctrl and the CCM/clock
modules. Each and every driver depends on them, so it is probably good
for sanity to require that Linux has some version of these modules. If
the 'M4' was to control the pins and/or clock, then some other modules
could be written that makes some calls to that CPU (or world).
For other AIPS peripherals, it would be nice if they could be either
configured out and/or set to disabled by the device tree.
Regards,
Bill Pringlemeir.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610
2014-09-29 15:05 ` Bill Pringlemeir
@ 2014-09-29 17:25 ` Stefan Agner
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Agner @ 2014-09-29 17:25 UTC (permalink / raw)
To: Bill Pringlemeir
Cc: Shawn Guo, linus.walleij, gnurou, kernel, linux-gpio,
linux-arm-kernel, linux-kernel
Am 2014-09-29 17:05, schrieb Bill Pringlemeir:
>>> On 25 Sep 2014, stefan@agner.ch wrote:
>
>>>> IMHO, fully orthogonal is not possible, since on Vybrid those two
>>>> depend on each other. But in order to make it "more orthogonal", we
>>>> maybe should force applying the full configuration in
>>>> imx_pmx_gpio_request_enable (e.g. drive strenght etc), rather then
>>>> only mux the pad to to GPIO...
>>>> What do you think?
>
>> Am 2014-09-25 18:43, schrieb Bill Pringlemeir:
>
>>> The muxing must have been done, that is correct. However, this could
>>> be by a boot loader, by some other API, etc. People maybe concerned
>>> about Linux affecting 'CAN' (or some 'mission critical' pins) muxing
>>> for instance, but want the A5 to handle GPIO.
>>>
>>> If somewhere, somehow the pin was muxed properly and the GPIO code
>>> still works, I believe this is a win. I see that this flexibility
>>> makes it more difficult to get something that just works. I think
>>> the device trees take care of this for normal users. It might be
>>> appropriate to add a DT node that sets 'GPIO_CONTROL' and put a note
>>> in the DT-GPIO document, that pinctrl is needed for a normal case or
>>> if some external muxing is used or not needed, then 'control=false'
>>> can be set (or whatever good DT terminology)? Then the
>>> 'GPIO_CONTROL' check would be in the 'vf610_gpio_direction_input()'
>>> functions. The 'pinctrl' functions are currently compiled to stubs
>>> if that is configured out.
>
> On 26 Sep 2014, stefan@agner.ch wrote:
>
>> Currently pinctrl is not optional for Vybrid (Kconfig mandates pinctrl
>> by "select PINCTRL_VF610").
>
>> IMHO there is no real value having a way to mark a pin as "muxed
>> externally" (control=false). If we use a pin, we should configure and
>> mux that properly in Linux, and if we don't have valid
>> configuration/mux information, we should not touch that pin.
>
> I didn't mean per-pin, I meant for the GPIO controller as a whole. I
> was also looking at a IOMUX controlled by the M4 and/or a boot loader.
> I agree it is not possible with the mainline. This just makes it even
> less possible. This is great if we know we are going that route.
Ok, but this patch doesn't make it less possible then before: You can
disable the GPIO driver or disable all gpio device tree entries.
>
>> In v2 of this patchset, the GPIO code only worked when there _is_ a
>> pinctrl entry for that GPIO pin in the DT. This is because I need the
>> register offset, which is provided by the entry itself. But the pad
>> configuration is another point, which I did not consider in v2. But in
>> v3, the GPIO code also writes the pad control.
>
> I was a little concerned about this as the pin may have pull-ups and
> pull-downs. We would not want to dynamically change this. However, I
> don't fully understand this path.
>
> [snip]
>
>>> Then there is 'multi-machine' support with DT and compile time
>>> selects with CONFIG_PINCTRL and I don't think there is a lot of
>>> additional code or confusion?
>
>> I don't understand that.
>
> I was thinking of a modified Vybrid which would allow us to not select
> PINCTRL. As per Shawn's comment, it is suppose to be
> optional/orthogonal to the GPIO controller. If someone made the change
> to not select PINCTRL_VF610, then the GPIO code calls would currently
> compile to stubs. To get the same things with a multi-machine
> (Vybrid/Imx with/without pinctrl), then you could use the DT property
> which would avoid the pinctrl calls.
Now I understand, you mean a kernel with PINCTRL support but having an
option to still disable the pinctrl functionality for Vybrids GPIOs.
I think if you descide to use a pin as GPIO from Linux on A5, you also
want that pin to be "pinctrl"ed by Linux. I don't see a reason why you
want a pin configured by something else (bootloader or firmware on M4),
but then use that same pin as GPIO from Linux. IMHO, it's good to get
this as a package (GPIO which does pinctrl).
In the IOMUXC module each pin has its own register, so here it's ok to
have multiple sytems fiddling with it, as long as the systems are not
touching the same pins at the same time.
> The more required modules we require for each feature, the harder it is
> to partition the AIPS peripherals between cores. It sort of makes the
> mainline Linux only useful for the VF5xx non-secure parts.
I agree, having the option to partition AIPS peripherals is useful,
especially on Vybrid (and i.MX6SX). And this is perfectly possible with
device tree. Just disable the gpio device tree sections (which are
compatible = "fsl,vf610-gpio") which you don't plan to use on Linux.
Linux creates 5 instances of the driver when all 5 gpio device tree
entries are active. However, you can create a board file with only one
gpio section, and you get only one instance of the driver which takes
care of the associated 32 pins. Or even no gpio device tree sections at
all, and you have the same situation before this driver was merged...
However, sub GPIO/PORT bank partitioning is not possible... The PORT and
GPIO module have shared register for 32 pins, and some operation might
not be atomic, so locking would be required. Having two system accessing
the same PORT/GPIO bank would certainly impose problems, for instance
when handling a PORT interrupt.
> I guess another option is to make another pinctrl module for the Vybrid
> which uses some inter-core communications to alter the MUX
> configuration. This makes sense for the pinctrl and the CCM/clock
> modules. Each and every driver depends on them, so it is probably good
> for sanity to require that Linux has some version of these modules. If
> the 'M4' was to control the pins and/or clock, then some other modules
> could be written that makes some calls to that CPU (or world).
I also have some M4 firmwares running (eCos, bare-metal), and usually we
just let them access the CCM in a "read-only" mode. It works fairly
well, however I agree this solution is not perfect. The hardware
designers gave us hardware semaphores and CPU to CPU interrupts, so we
could build a configuration interface (and maybe a resource allocation
as well) around this. This would then implicate an ABI, and this
certainly would need some time and discussion to make it good enough for
mainline, if possible at all.
> For other AIPS peripherals, it would be nice if they could be either
> configured out and/or set to disabled by the device tree.
Agreed, we should do this as much as possible for Vybrid.
--
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/5] gpio: vf610: add gpiolib/IRQ chip driver for Vybrid
2014-09-23 17:37 [PATCH v2 0/5] vf610: Add GPIO support Stefan Agner
2014-09-23 17:37 ` [PATCH v2 1/5] pinctrl: imx: detect uninitialized pins Stefan Agner
2014-09-23 17:37 ` [PATCH v2 2/5] pinctrl: imx: add gpio pinmux support for vf610 Stefan Agner
@ 2014-09-23 17:37 ` Stefan Agner
2014-09-25 5:55 ` Shawn Guo
2014-09-23 17:37 ` [PATCH v2 4/5] ARM: dts: vf610: use new GPIO support Stefan Agner
2014-09-23 17:37 ` [PATCH v2 5/5] Documentation: dts: Add bindings for Vybrid GPIO/PORT module Stefan Agner
4 siblings, 1 reply; 17+ messages in thread
From: Stefan Agner @ 2014-09-23 17:37 UTC (permalink / raw)
To: linus.walleij, gnurou, shawn.guo, kernel
Cc: linux-gpio, stefan, linux-kernel, linux-arm-kernel, bpringlemeir
Add a gpiolib and IRQ chip driver for Vybrid ARM SoC using the
Vybrid's GPIO and PORT module. The driver is instanced once per
each GPIO/PORT module pair and handles 32 GPIO's.
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
drivers/gpio/Kconfig | 7 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-vf610.c | 284 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 292 insertions(+)
create mode 100644 drivers/gpio/gpio-vf610.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9de1515..82b38f5 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -334,6 +334,13 @@ config GPIO_TZ1090_PDC
help
Say yes here to support Toumaz Xenif TZ1090 PDC GPIOs.
+config GPIO_VF610
+ def_bool y
+ depends on ARCH_MXC && SOC_VF610
+ select GPIOLIB_IRQCHIP
+ help
+ Say yes here to support Vybrid vf610 GPIOs.
+
config GPIO_XILINX
bool "Xilinx GPIO support"
depends on PPC_OF || MICROBLAZE || ARCH_ZYNQ
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 5d024e3..9893d4c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_GPIO_TWL6040) += gpio-twl6040.o
obj-$(CONFIG_GPIO_TZ1090) += gpio-tz1090.o
obj-$(CONFIG_GPIO_TZ1090_PDC) += gpio-tz1090-pdc.o
obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o
+obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o
obj-$(CONFIG_GPIO_VIPERBOARD) += gpio-viperboard.o
obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o
obj-$(CONFIG_GPIO_VX855) += gpio-vx855.o
diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
new file mode 100644
index 0000000..6649a13
--- /dev/null
+++ b/drivers/gpio/gpio-vf610.c
@@ -0,0 +1,284 @@
+/*
+ * vf610 GPIO support through PORT and GPIO module
+ *
+ * Copyright (c) 2014 Toradex AG.
+ *
+ * Author: Stefan Agner <stefan@agner.ch>.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 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.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/bitops.h>
+#include <linux/gpio.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/module.h>
+#include <asm-generic/bug.h>
+
+
+#define VF610_GPIO_PER_PORT 32
+
+struct vf610_gpio_port {
+ struct gpio_chip gc;
+ void __iomem *base;
+ void __iomem *gpio_base;
+ u8 irqc[VF610_GPIO_PER_PORT];
+};
+
+#define GPIO_PDOR 0x00
+#define GPIO_PSOR 0x04
+#define GPIO_PCOR 0x08
+#define GPIO_PTOR 0x0c
+#define GPIO_PDIR 0x10
+
+#define PORT_PCR(n) (n * 0x4)
+#define PORT_PCR_IRQC_OFFSET 16
+
+#define PORT_ISFR 0xa0
+#define PORT_DFER 0xc0
+#define PORT_DFCR 0xc4
+#define PORT_DFWR 0xc8
+
+#define PORT_INT_OFF 0x0
+#define PORT_INT_LOGIC_ZERO 0x8
+#define PORT_INT_RISING_EDGE 0x9
+#define PORT_INT_FALLING_EDGE 0xa
+#define PORT_INT_EITHER_EDGE 0xb
+#define PORT_INT_LOGIC_ONE 0xc
+
+static const struct of_device_id vf610_gpio_dt_ids[] = {
+ { .compatible = "fsl,vf610-gpio" },
+ { /* sentinel */ }
+};
+
+static inline void vf610_gpio_writel(u32 val, void __iomem *reg)
+{
+ writel_relaxed(val, reg);
+}
+
+static inline u32 vf610_gpio_readl(void __iomem *reg)
+{
+ return readl_relaxed(reg);
+}
+
+static int vf610_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+ return pinctrl_request_gpio(chip->base + offset);
+}
+
+static void vf610_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+ pinctrl_free_gpio(chip->base + offset);
+}
+
+static int vf610_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct vf610_gpio_port *port =
+ container_of(gc, struct vf610_gpio_port, gc);
+
+ return !!(vf610_gpio_readl(port->gpio_base + GPIO_PDIR) & BIT(gpio));
+}
+
+static void vf610_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct vf610_gpio_port *port =
+ container_of(gc, struct vf610_gpio_port, gc);
+ unsigned long mask = BIT(gpio);
+
+ if (val)
+ vf610_gpio_writel(mask, port->gpio_base + GPIO_PSOR);
+ else
+ vf610_gpio_writel(mask, port->gpio_base + GPIO_PCOR);
+}
+
+static int vf610_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
+{
+ return pinctrl_gpio_direction_input(chip->base + gpio);
+}
+
+static int vf610_gpio_direction_output(struct gpio_chip *chip, unsigned gpio,
+ int value)
+{
+ vf610_gpio_set(chip, gpio, value);
+
+ return pinctrl_gpio_direction_output(chip->base + gpio);
+}
+
+static void vf610_gpio_irq_handler(u32 irq, struct irq_desc *desc)
+{
+ struct vf610_gpio_port *port = irq_get_handler_data(irq);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ int pin;
+ unsigned long irq_isfr;
+
+ chained_irq_enter(chip, desc);
+
+ irq_isfr = vf610_gpio_readl(port->base + PORT_ISFR);
+
+ for_each_set_bit(pin, &irq_isfr, VF610_GPIO_PER_PORT) {
+ vf610_gpio_writel(BIT(pin), port->base + PORT_ISFR);
+
+ generic_handle_irq(irq_find_mapping(port->gc.irqdomain, pin));
+ }
+
+ chained_irq_exit(chip, desc);
+}
+
+
+static void vf610_gpio_irq_ack(struct irq_data *d)
+{
+ struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d);
+ int gpio = d->hwirq;
+
+ vf610_gpio_writel(BIT(gpio), port->base + PORT_ISFR);
+}
+
+static int vf610_gpio_irq_set_type(struct irq_data *d, u32 type)
+{
+ struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d);
+ u8 irqc;
+
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ irqc = PORT_INT_RISING_EDGE;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ irqc = PORT_INT_FALLING_EDGE;
+ break;
+ case IRQ_TYPE_EDGE_BOTH:
+ irqc = PORT_INT_EITHER_EDGE;
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ irqc = PORT_INT_LOGIC_ZERO;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ irqc = PORT_INT_LOGIC_ONE;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ port->irqc[d->hwirq] = irqc;
+
+ return 0;
+}
+
+static void vf610_gpio_irq_mask(struct irq_data *d)
+{
+ struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d);
+ void __iomem *pcr_base = port->base + PORT_PCR(d->hwirq);
+
+ vf610_gpio_writel(0, pcr_base);
+}
+
+static void vf610_gpio_irq_unmask(struct irq_data *d)
+{
+ struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d);
+ void __iomem *pcr_base = port->base + PORT_PCR(d->hwirq);
+
+ vf610_gpio_writel(port->irqc[d->hwirq] << PORT_PCR_IRQC_OFFSET,
+ pcr_base);
+}
+
+static struct irq_chip vf610_gpio_irq_chip = {
+ .name = "gpio-vf610",
+ .irq_ack = vf610_gpio_irq_ack,
+ .irq_mask = vf610_gpio_irq_mask,
+ .irq_unmask = vf610_gpio_irq_unmask,
+ .irq_set_type = vf610_gpio_irq_set_type,
+};
+
+static int vf610_gpio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct vf610_gpio_port *port;
+ struct resource *iores;
+ struct gpio_chip *gc;
+ int irq, ret;
+
+ port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
+ if (!port)
+ return -ENOMEM;
+
+ iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ port->base = devm_ioremap_resource(dev, iores);
+ if (IS_ERR(port->base))
+ return PTR_ERR(port->base);
+
+ iores = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ port->gpio_base = devm_ioremap_resource(dev, iores);
+ if (IS_ERR(port->gpio_base))
+ return PTR_ERR(port->gpio_base);
+
+ irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+ if (irq == NO_IRQ)
+ return -ENODEV;
+
+ gc = &port->gc;
+ gc->of_node = np;
+ gc->dev = dev;
+ gc->label = "vf610-gpio",
+ gc->ngpio = VF610_GPIO_PER_PORT,
+ gc->base = of_alias_get_id(np, "gpio") * VF610_GPIO_PER_PORT;
+
+ gc->request = vf610_gpio_request,
+ gc->free = vf610_gpio_free,
+ gc->direction_input = vf610_gpio_direction_input,
+ gc->get = vf610_gpio_get,
+ gc->direction_output = vf610_gpio_direction_output,
+ gc->set = vf610_gpio_set,
+
+ ret = gpiochip_add(gc);
+ if (ret < 0)
+ return ret;
+
+ /* Clear the interrupt status register for all GPIO's */
+ vf610_gpio_writel(~0, port->base + PORT_ISFR);
+
+ ret = gpiochip_irqchip_add(gc, &vf610_gpio_irq_chip, 0,
+ handle_simple_irq, IRQ_TYPE_NONE);
+ if (ret) {
+ dev_err(dev, "failed to add irqchip\n");
+ gpiochip_remove(gc);
+ return ret;
+ }
+ gpiochip_set_chained_irqchip(gc, &vf610_gpio_irq_chip, irq,
+ vf610_gpio_irq_handler);
+
+ return 0;
+}
+
+static struct platform_driver vf610_gpio_driver = {
+ .driver = {
+ .name = "gpio-vf610",
+ .owner = THIS_MODULE,
+ .of_match_table = vf610_gpio_dt_ids,
+ },
+ .probe = vf610_gpio_probe,
+};
+
+static int __init gpio_vf610_init(void)
+{
+ return platform_driver_register(&vf610_gpio_driver);
+}
+subsys_initcall(gpio_vf610_init);
+
+MODULE_AUTHOR("Stefan Agner <stefan@agner.ch>");
+MODULE_DESCRIPTION("Freescale VF610 GPIO");
+MODULE_LICENSE("GPL v2");
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/5] gpio: vf610: add gpiolib/IRQ chip driver for Vybrid
2014-09-23 17:37 ` [PATCH v2 3/5] gpio: vf610: add gpiolib/IRQ chip driver for Vybrid Stefan Agner
@ 2014-09-25 5:55 ` Shawn Guo
2014-09-25 8:10 ` Stefan Agner
0 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2014-09-25 5:55 UTC (permalink / raw)
To: Stefan Agner
Cc: linus.walleij, gnurou, kernel, linux-gpio, linux-arm-kernel,
linux-kernel, bpringlemeir
On Tue, Sep 23, 2014 at 07:37:55PM +0200, Stefan Agner wrote:
> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> new file mode 100644
> index 0000000..6649a13
> --- /dev/null
> +++ b/drivers/gpio/gpio-vf610.c
> @@ -0,0 +1,284 @@
> +/*
> + * vf610 GPIO support through PORT and GPIO module
> + *
> + * Copyright (c) 2014 Toradex AG.
> + *
> + * Author: Stefan Agner <stefan@agner.ch>.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * 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.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>
You might want to have the headers sort alphabetically.
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/module.h>
> +#include <asm-generic/bug.h>
Why do we need this header?
> +
> +
One new line is enough.
> +#define VF610_GPIO_PER_PORT 32
> +
> +struct vf610_gpio_port {
> + struct gpio_chip gc;
> + void __iomem *base;
> + void __iomem *gpio_base;
> + u8 irqc[VF610_GPIO_PER_PORT];
> +};
> +
> +#define GPIO_PDOR 0x00
> +#define GPIO_PSOR 0x04
> +#define GPIO_PCOR 0x08
> +#define GPIO_PTOR 0x0c
> +#define GPIO_PDIR 0x10
> +
> +#define PORT_PCR(n) (n * 0x4)
s/n/(n)
...
> +static int vf610_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct vf610_gpio_port *port;
> + struct resource *iores;
> + struct gpio_chip *gc;
> + int irq, ret;
> +
> + port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + port->base = devm_ioremap_resource(dev, iores);
> + if (IS_ERR(port->base))
> + return PTR_ERR(port->base);
> +
> + iores = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + port->gpio_base = devm_ioremap_resource(dev, iores);
> + if (IS_ERR(port->gpio_base))
> + return PTR_ERR(port->gpio_base);
> +
> + irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
Why not platform_get_irq()?
> + if (irq == NO_IRQ)
> + return -ENODEV;
Instead of using NO_IRQ, we should check if irq is a negative value and
propagate the value as the return, something like:
if (irq < 0)
return irq;
Shawn
> +
> + gc = &port->gc;
> + gc->of_node = np;
> + gc->dev = dev;
> + gc->label = "vf610-gpio",
> + gc->ngpio = VF610_GPIO_PER_PORT,
> + gc->base = of_alias_get_id(np, "gpio") * VF610_GPIO_PER_PORT;
> +
> + gc->request = vf610_gpio_request,
> + gc->free = vf610_gpio_free,
> + gc->direction_input = vf610_gpio_direction_input,
> + gc->get = vf610_gpio_get,
> + gc->direction_output = vf610_gpio_direction_output,
> + gc->set = vf610_gpio_set,
> +
> + ret = gpiochip_add(gc);
> + if (ret < 0)
> + return ret;
> +
> + /* Clear the interrupt status register for all GPIO's */
> + vf610_gpio_writel(~0, port->base + PORT_ISFR);
> +
> + ret = gpiochip_irqchip_add(gc, &vf610_gpio_irq_chip, 0,
> + handle_simple_irq, IRQ_TYPE_NONE);
> + if (ret) {
> + dev_err(dev, "failed to add irqchip\n");
> + gpiochip_remove(gc);
> + return ret;
> + }
> + gpiochip_set_chained_irqchip(gc, &vf610_gpio_irq_chip, irq,
> + vf610_gpio_irq_handler);
> +
> + return 0;
> +}
> +
> +static struct platform_driver vf610_gpio_driver = {
> + .driver = {
> + .name = "gpio-vf610",
> + .owner = THIS_MODULE,
> + .of_match_table = vf610_gpio_dt_ids,
> + },
> + .probe = vf610_gpio_probe,
> +};
> +
> +static int __init gpio_vf610_init(void)
> +{
> + return platform_driver_register(&vf610_gpio_driver);
> +}
> +subsys_initcall(gpio_vf610_init);
> +
> +MODULE_AUTHOR("Stefan Agner <stefan@agner.ch>");
> +MODULE_DESCRIPTION("Freescale VF610 GPIO");
> +MODULE_LICENSE("GPL v2");
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/5] gpio: vf610: add gpiolib/IRQ chip driver for Vybrid
2014-09-25 5:55 ` Shawn Guo
@ 2014-09-25 8:10 ` Stefan Agner
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Agner @ 2014-09-25 8:10 UTC (permalink / raw)
To: Shawn Guo
Cc: linus.walleij, gnurou, kernel, linux-gpio, linux-arm-kernel,
linux-kernel, bpringlemeir
Am 2014-09-25 07:55, schrieb Shawn Guo:
> On Tue, Sep 23, 2014 at 07:37:55PM +0200, Stefan Agner wrote:
>> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
>> new file mode 100644
>> index 0000000..6649a13
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-vf610.c
>> @@ -0,0 +1,284 @@
>> +/*
>> + * vf610 GPIO support through PORT and GPIO module
>> + *
>> + * Copyright (c) 2014 Toradex AG.
>> + *
>> + * Author: Stefan Agner <stefan@agner.ch>.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + * 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.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/irq.h>
>> +#include <linux/bitops.h>
>> +#include <linux/gpio.h>
>
> You might want to have the headers sort alphabetically.
>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/module.h>
>> +#include <asm-generic/bug.h>
>
> Why do we need this header?
>
>> +
>> +
>
> One new line is enough.
>
>> +#define VF610_GPIO_PER_PORT 32
>> +
>> +struct vf610_gpio_port {
>> + struct gpio_chip gc;
>> + void __iomem *base;
>> + void __iomem *gpio_base;
>> + u8 irqc[VF610_GPIO_PER_PORT];
>> +};
>> +
>> +#define GPIO_PDOR 0x00
>> +#define GPIO_PSOR 0x04
>> +#define GPIO_PCOR 0x08
>> +#define GPIO_PTOR 0x0c
>> +#define GPIO_PDIR 0x10
>> +
>> +#define PORT_PCR(n) (n * 0x4)
>
> s/n/(n)
>
> ...
>
>> +static int vf610_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + struct vf610_gpio_port *port;
>> + struct resource *iores;
>> + struct gpio_chip *gc;
>> + int irq, ret;
>> +
>> + port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
>> + if (!port)
>> + return -ENOMEM;
>> +
>> + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + port->base = devm_ioremap_resource(dev, iores);
>> + if (IS_ERR(port->base))
>> + return PTR_ERR(port->base);
>> +
>> + iores = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + port->gpio_base = devm_ioremap_resource(dev, iores);
>> + if (IS_ERR(port->gpio_base))
>> + return PTR_ERR(port->gpio_base);
>> +
>> + irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>
> Why not platform_get_irq()?
I thought we need that mapping, but this is done dynamically when using
gpiochip_irqchip_add.
>
>> + if (irq == NO_IRQ)
>> + return -ENODEV;
>
> Instead of using NO_IRQ, we should check if irq is a negative value and
> propagate the value as the return, something like:
>
> if (irq < 0)
> return irq;
>
Agreed on everything, will fix that.
>
>> +
>> + gc = &port->gc;
>> + gc->of_node = np;
>> + gc->dev = dev;
>> + gc->label = "vf610-gpio",
>> + gc->ngpio = VF610_GPIO_PER_PORT,
>> + gc->base = of_alias_get_id(np, "gpio") * VF610_GPIO_PER_PORT;
>> +
>> + gc->request = vf610_gpio_request,
>> + gc->free = vf610_gpio_free,
>> + gc->direction_input = vf610_gpio_direction_input,
>> + gc->get = vf610_gpio_get,
>> + gc->direction_output = vf610_gpio_direction_output,
>> + gc->set = vf610_gpio_set,
>> +
>> + ret = gpiochip_add(gc);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Clear the interrupt status register for all GPIO's */
>> + vf610_gpio_writel(~0, port->base + PORT_ISFR);
>> +
>> + ret = gpiochip_irqchip_add(gc, &vf610_gpio_irq_chip, 0,
>> + handle_simple_irq, IRQ_TYPE_NONE);
>> + if (ret) {
>> + dev_err(dev, "failed to add irqchip\n");
>> + gpiochip_remove(gc);
>> + return ret;
>> + }
>> + gpiochip_set_chained_irqchip(gc, &vf610_gpio_irq_chip, irq,
>> + vf610_gpio_irq_handler);
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver vf610_gpio_driver = {
>> + .driver = {
>> + .name = "gpio-vf610",
>> + .owner = THIS_MODULE,
>> + .of_match_table = vf610_gpio_dt_ids,
>> + },
>> + .probe = vf610_gpio_probe,
>> +};
>> +
>> +static int __init gpio_vf610_init(void)
>> +{
>> + return platform_driver_register(&vf610_gpio_driver);
>> +}
>> +subsys_initcall(gpio_vf610_init);
>> +
>> +MODULE_AUTHOR("Stefan Agner <stefan@agner.ch>");
>> +MODULE_DESCRIPTION("Freescale VF610 GPIO");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.1.0
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 4/5] ARM: dts: vf610: use new GPIO support
2014-09-23 17:37 [PATCH v2 0/5] vf610: Add GPIO support Stefan Agner
` (2 preceding siblings ...)
2014-09-23 17:37 ` [PATCH v2 3/5] gpio: vf610: add gpiolib/IRQ chip driver for Vybrid Stefan Agner
@ 2014-09-23 17:37 ` Stefan Agner
2014-09-23 17:37 ` [PATCH v2 5/5] Documentation: dts: Add bindings for Vybrid GPIO/PORT module Stefan Agner
4 siblings, 0 replies; 17+ messages in thread
From: Stefan Agner @ 2014-09-23 17:37 UTC (permalink / raw)
To: linus.walleij, gnurou, shawn.guo, kernel
Cc: linux-gpio, linux-arm-kernel, linux-kernel, bpringlemeir, stefan
Use GPIO support by adding SD card detection configuration and
GPIO pinmux for Colibri's standard GPIO pins. Attach the GPIO
pins to the iomuxc node to get the GPIO pin settings applied.
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
arch/arm/boot/dts/vf610-colibri-eval-v3.dts | 5 +++++
arch/arm/boot/dts/vf610-colibri.dtsi | 19 +++++++++++++++++++
arch/arm/boot/dts/vf610-twr.dts | 1 +
arch/arm/boot/dts/vf610.dtsi | 1 +
4 files changed, 26 insertions(+)
diff --git a/arch/arm/boot/dts/vf610-colibri-eval-v3.dts b/arch/arm/boot/dts/vf610-colibri-eval-v3.dts
index 7fb3066..9a0e304 100644
--- a/arch/arm/boot/dts/vf610-colibri-eval-v3.dts
+++ b/arch/arm/boot/dts/vf610-colibri-eval-v3.dts
@@ -33,6 +33,11 @@
status = "okay";
};
+&iomuxc {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_gpio_ext0 &pinctrl_gpio_ext1 &pinctrl_gpio_ext2>;
+};
+
&uart0 {
status = "okay";
};
diff --git a/arch/arm/boot/dts/vf610-colibri.dtsi b/arch/arm/boot/dts/vf610-colibri.dtsi
index 0cd8343..c940e14 100644
--- a/arch/arm/boot/dts/vf610-colibri.dtsi
+++ b/arch/arm/boot/dts/vf610-colibri.dtsi
@@ -31,6 +31,7 @@
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_esdhc1>;
bus-width = <4>;
+ cd-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
};
&fec1 {
@@ -71,6 +72,24 @@
&iomuxc {
vf610-colibri {
+ pinctrl_gpio_ext0: gpio_ext0 {
+ fsl,pins = <
+ VF610_PAD_PTD10__GPIO_89 0x219d
+ >;
+ };
+
+ pinctrl_gpio_ext1: gpio_ext1 {
+ fsl,pins = <
+ VF610_PAD_PTD9__GPIO_88 0x219d
+ >;
+ };
+
+ pinctrl_gpio_ext2: gpio_ext2 {
+ fsl,pins = <
+ VF610_PAD_PTD26__GPIO_68 0x219d
+ >;
+ };
+
pinctrl_esdhc1: esdhc1grp {
fsl,pins = <
VF610_PAD_PTA24__ESDHC1_CLK 0x31ef
diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index 189b697..3fe8a8f 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -116,6 +116,7 @@
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_esdhc1>;
bus-width = <4>;
+ cd-gpios = <&gpio5 6 GPIO_ACTIVE_LOW>;
status = "okay";
};
diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index 4d2ec32..467c97e 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -11,6 +11,7 @@
#include "vf610-pinfunc.h"
#include <dt-bindings/clock/vf610-clock.h>
#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/gpio/gpio.h>
/ {
aliases {
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 5/5] Documentation: dts: Add bindings for Vybrid GPIO/PORT module
2014-09-23 17:37 [PATCH v2 0/5] vf610: Add GPIO support Stefan Agner
` (3 preceding siblings ...)
2014-09-23 17:37 ` [PATCH v2 4/5] ARM: dts: vf610: use new GPIO support Stefan Agner
@ 2014-09-23 17:37 ` Stefan Agner
4 siblings, 0 replies; 17+ messages in thread
From: Stefan Agner @ 2014-09-23 17:37 UTC (permalink / raw)
To: linus.walleij, gnurou, shawn.guo, kernel
Cc: linux-gpio, linux-arm-kernel, linux-kernel, bpringlemeir, stefan
The Vybrid SoC device tree (vf610.dtsi) used this bindings since
its initial commit in May 2013. However, a proper gpiolib driver
was missing so far. With the addition of the gpiolib driver,
the bindings proved to be useful and complete, hence a good time
to add the documentation.
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
.../devicetree/bindings/gpio/gpio-vf610.txt | 56 ++++++++++++++++++++++
1 file changed, 56 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-vf610.txt
diff --git a/Documentation/devicetree/bindings/gpio/gpio-vf610.txt b/Documentation/devicetree/bindings/gpio/gpio-vf610.txt
new file mode 100644
index 0000000..da84121
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-vf610.txt
@@ -0,0 +1,56 @@
+* Freescale VF610 PORT/GPIO module
+
+The Freescale PORT/GPIO modules are two adjacent modules providing GPIO
+functionality. Each pair serves 32 GPIOs. The VF610 has 5 instances of
+each, and each PORT module has its own interrupt.
+
+Required properties for GPIO node:
+- compatible : Should be "fsl,<soc>-gpio", currently "fsl,vf610-gpio"
+- reg : The first reg tuple represents the PORT module, the second tuple
+ the GPIO module.
+- interrupts : Should be the port interrupt shared by all 32 pins.
+- gpio-controller : Marks the device node as a gpio controller.
+- #gpio-cells : Should be two. The first cell is the pin number and
+ the second cell is used to specify the gpio polarity:
+ 0 = active high
+ 1 = active low
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells : Should be 2. The first cell is the GPIO number.
+ The second cell bits[3:0] is used to specify trigger type and level flags:
+ 1 = low-to-high edge triggered.
+ 2 = high-to-low edge triggered.
+ 4 = active high level-sensitive.
+ 8 = active low level-sensitive.
+
+Note: Each GPIO port should have an alias correctly numbered in "aliases"
+node.
+
+Examples:
+
+aliases {
+ gpio0 = &gpio1;
+ gpio1 = &gpio2;
+};
+
+gpio1: gpio@40049000 {
+ compatible = "fsl,vf610-gpio";
+ reg = <0x40049000 0x1000 0x400ff000 0x40>;
+ interrupts = <0 107 IRQ_TYPE_LEVEL_HIGH>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ gpio-ranges = <&iomuxc 0 0 32>;
+};
+
+gpio2: gpio@4004a000 {
+ compatible = "fsl,vf610-gpio";
+ reg = <0x4004a000 0x1000 0x400ff040 0x40>;
+ interrupts = <0 108 IRQ_TYPE_LEVEL_HIGH>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ gpio-ranges = <&iomuxc 0 32 32>;
+};
+
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread