[PATCH V3 03/10] dt-bindings: gpio: vf610: add optional clocks property

[PATCH V3 03/10] dt-bindings: gpio: vf610: add optional clocks property

[A.s. Dong]

On some SoCs(e.g. MX7ULP), GPIO clock is gatable and maybe
disabled by default. Users have to make sure it's enabled before
being able to access controller registers, otherwise an external
abort error may occur. Let's add the optional clocks property to
handle this case.

For ULP GPIO clock, it includes two separate clocks: one is for
GPIO controller Input/Output function clock while another is
GPIO port control clock for interrupt function.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: linux-gpio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v2->v3:
 * no changes
v1->v2:
 * new patch
---
 Documentation/devicetree/bindings/gpio/gpio-vf610.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-vf610.txt b/Documentation/devicetree/bindings/gpio/gpio-vf610.txt
index 0ccbae4..ae254aa 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-vf610.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-vf610.txt
@@ -24,6 +24,12 @@ Required properties for GPIO node:
       4 = active high level-sensitive.
       8 = active low level-sensitive.
 
+Optional properties:
+-clocks:	Must contain an entry for each entry in clock-names.
+		See common clock-bindings.txt for details.
+-clock-names:	A list of clock names. For imx7ulp, it must contain
+		"gpio", "port".
+
 Note: Each GPIO port should have an alias correctly numbered in "aliases"
 node.
 
-- 
2.7.4

Re: [PATCH V3 03/10] dt-bindings: gpio: vf610: add optional clocks property

[Linus Walleij]

On Wed, Oct 31, 2018 at 3:43 PM A.s. Dong <aisheng.dong@nxp.com> wrote:

> On some SoCs(e.g. MX7ULP), GPIO clock is gatable and maybe
> disabled by default. Users have to make sure it's enabled before
> being able to access controller registers, otherwise an external
> abort error may occur. Let's add the optional clocks property to
> handle this case.
>
> For ULP GPIO clock, it includes two separate clocks: one is for
> GPIO controller Input/Output function clock while another is
> GPIO port control clock for interrupt function.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: linux-gpio@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

Patch applied.

Yours,
Linus Walleij

[PATCH V3 04/10] gpio: vf610: add optional clock support

[A.s. Dong]

Some SoCs need the gpio clock to be enabled before accessing
HW registers. This patch add the optional clock handling.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: linux-gpio@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v2->v3:
 * error checking updated according to Russell's suggestion:
   ptr == ERR_PTR(-EPROBE_DEFER)
 * clock independently checking
v1->v2:
 * new patch
---
 drivers/gpio/gpio-vf610.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
index d4ad6d0..02fb7d8 100644
--- a/drivers/gpio/gpio-vf610.c
+++ b/drivers/gpio/gpio-vf610.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/init.h>
@@ -256,6 +257,7 @@ static int vf610_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
+	struct clk *clk_gpio, *clk_port;
 	struct vf610_gpio_port *port;
 	struct resource *iores;
 	struct gpio_chip *gc;
@@ -280,6 +282,28 @@ static int vf610_gpio_probe(struct platform_device *pdev)
 	if (port->irq < 0)
 		return port->irq;
 
+	clk_port = devm_clk_get(&pdev->dev, "port");
+	if (clk_port == ERR_PTR(-EPROBE_DEFER))
+		return -EPROBE_DEFER;
+
+	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
+	if (clk_gpio == ERR_PTR(-EPROBE_DEFER))
+		return -EPROBE_DEFER;
+
+	if (!IS_ERR_OR_NULL(clk_port)) {
+		ret = clk_prepare_enable(clk_port);
+		if (ret)
+			return ret;
+	}
+
+	if (!IS_ERR_OR_NULL(clk_gpio)) {
+		ret = clk_prepare_enable(clk_gpio);
+		if (ret) {
+			clk_disable_unprepare(clk_port);
+			return ret;
+		}
+	}
+
 	gc = &port->gc;
 	gc->of_node = np;
 	gc->parent = dev;
-- 
2.7.4

Re: [PATCH V3 04/10] gpio: vf610: add optional clock support

[Linus Walleij]

On Wed, Oct 31, 2018 at 3:43 PM A.s. Dong <aisheng.dong@nxp.com> wrote:

> +       clk_port = devm_clk_get(&pdev->dev, "port");
> +       if (clk_port == ERR_PTR(-EPROBE_DEFER))
> +               return -EPROBE_DEFER;
> +
> +       clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> +       if (clk_gpio == ERR_PTR(-EPROBE_DEFER))
> +               return -EPROBE_DEFER;
> +
> +       if (!IS_ERR_OR_NULL(clk_port)) {
> +               ret = clk_prepare_enable(clk_port);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       if (!IS_ERR_OR_NULL(clk_gpio)) {
> +               ret = clk_prepare_enable(clk_gpio);
> +               if (ret) {
> +                       clk_disable_unprepare(clk_port);
> +                       return ret;
> +               }
> +       }

I think IS_ERR_OR_NULL() is considered harmful among other
things.

What about this pattern:

        clk = devm_clk_get(dev, "foo");
        if (!IS_ERR(clk)) {
                ret = clk_prepare_enable(clk);
                if (ret)
                        return ret;
        } else if (PTR_ERR(clk) == -EPROBE_DEFER) {
                /*
                 * Percolate deferrals, for anything else,
                 * just live without the clocking.
                 */
                return PTR_ERR(clk);
        }

You also need to introduce code to disable the clock on
.remove() or the clock will always be on after this module
has probed. This applies also to builtin drivers, unless
you suppress the sysfs attributes and use
platform_driver_probe()

Yours,
Linus Walleij

RE: [PATCH V3 04/10] gpio: vf610: add optional clock support

[A.s. Dong]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: Friday, November 2, 2018 5:33 PM
[...]
> On Wed, Oct 31, 2018 at 3:43 PM A.s. Dong <aisheng.dong@nxp.com> wrote:
> 
> > +       clk_port = devm_clk_get(&pdev->dev, "port");
> > +       if (clk_port == ERR_PTR(-EPROBE_DEFER))
> > +               return -EPROBE_DEFER;
> > +
> > +       clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> > +       if (clk_gpio == ERR_PTR(-EPROBE_DEFER))
> > +               return -EPROBE_DEFER;
> > +
> > +       if (!IS_ERR_OR_NULL(clk_port)) {
> > +               ret = clk_prepare_enable(clk_port);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       if (!IS_ERR_OR_NULL(clk_gpio)) {
> > +               ret = clk_prepare_enable(clk_gpio);
> > +               if (ret) {
> > +                       clk_disable_unprepare(clk_port);
> > +                       return ret;
> > +               }
> > +       }
> 
> I think IS_ERR_OR_NULL() is considered harmful among other things.
> 
> What about this pattern:
> 
>         clk = devm_clk_get(dev, "foo");
>         if (!IS_ERR(clk)) {
>                 ret = clk_prepare_enable(clk);
>                 if (ret)
>                         return ret;
>         } else if (PTR_ERR(clk) == -EPROBE_DEFER) {
>                 /*
>                  * Percolate deferrals, for anything else,
>                  * just live without the clocking.
>                  */
>                 return PTR_ERR(clk);
>         }
> 
> You also need to introduce code to disable the clock on
> .remove() or the clock will always be on after this module has probed. This
> applies also to builtin drivers, unless you suppress the sysfs attributes and use
> platform_driver_probe()
> 

Looks good to me. Will update the patch.
Thanks for the suggestion.

Regards
Dong Aisheng

> Yours,
> Linus Walleij

[PATCH V3 05/10] dt-bindings: pinctrl: imx7ulp: back to imx legacy binding for consistency

[A.s. Dong]

We already had an earlier conclusion that all new i.MX Socs will keep
using the legacy i.MX Pinctrl bindings instead of generic pin config.
However, MX7ULP generic pin config binding support has already been in
tree before that time. Per SoC maintainers' suggestions, in order to
get a better consistency for all i.MX devices, we'd like to go back to
imx legacy binding for MX7ULP as well.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: linux-gpio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
 v3: new patch
---
 .../bindings/pinctrl/fsl,imx7ulp-pinctrl.txt       | 66 ++++++++++------------
 1 file changed, 29 insertions(+), 37 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,imx7ulp-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fsl,imx7ulp-pinctrl.txt
index 44ad670a..bfa3703 100644
--- a/Documentation/devicetree/bindings/pinctrl/fsl,imx7ulp-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/fsl,imx7ulp-pinctrl.txt
@@ -7,55 +7,47 @@ Note:
 This binding doc is only for the IOMUXC1 support in A7 Domain and it only
 supports generic pin config.
 
-Please also refer pinctrl-bindings.txt in this directory for generic pinctrl
-binding.
-
-=== Pin Controller Node ===
+Please refer to fsl,imx-pinctrl.txt in this directory for common binding
+part and usage.
 
 Required properties:
-- compatible:	"fsl,imx7ulp-iomuxc1"
-- reg:		Should contain the base physical address and size of the iomuxc
-		registers.
-
-=== Pin Configuration Node ===
-- pinmux: One integers array, represents a group of pins mux setting.
-	The format is pinmux = <PIN_FUNC_ID>, PIN_FUNC_ID is a pin working on
-	a specific function.
-
-	NOTE: i.MX7ULP PIN_FUNC_ID consists of 4 integers as it shares one mux
-	and config register as follows:
-	<mux_conf_reg input_reg mux_mode input_val>
-
-	Refer to imx7ulp-pinfunc.h in in device tree source folder for all
-	available imx7ulp PIN_FUNC_ID.
-
-Optional Properties:
-- drive-strength		Integer. Controls Drive Strength
-					0: Standard
-					1: Hi Driver
-- drive-push-pull		Bool. Enable Pin Push-pull
-- drive-open-drain		Bool. Enable Pin Open-drian
-- slew-rate:			Integer. Controls Slew Rate
-					0: Standard
-					1: Slow
-- bias-disable:			Bool. Pull disabled
-- bias-pull-down:		Bool. Pull down on pin
-- bias-pull-up:			Bool. Pull up on pin
+- compatible:	"fsl,imx7ulp-iomuxc1".
+- fsl,pins:	Each entry consists of 5 integers which represents the mux
+		and config setting for one pin. The first 4 integers
+		<mux_conf_reg input_reg mux_mode input_val> are specified
+		using a PIN_FUNC_ID macro, which can be found in
+		imx7ulp-pinfunc.h in the device tree source folder.
+		The last integer CONFIG is the pad setting value like
+		pull-up on this pin.
+
+		Please refer to i.MX7ULP Reference Manual for detailed
+		CONFIG settings.
+
+CONFIG bits definition:
+PAD_CTL_OBE		(1 << 17)
+PAD_CTL_IBE		(1 << 16)
+PAD_CTL_LK		(1 << 16)
+PAD_CTL_DSE_HI		(1 << 6)
+PAD_CTL_DSE_STD		(0 << 6)
+PAD_CTL_ODE		(1 << 5)
+PAD_CTL_PUSH_PULL	(0 << 5)
+PAD_CTL_SRE_SLOW	(1 << 2)
+PAD_CTL_SRE_STD		(0 << 2)
+PAD_CTL_PE		(1 << 0)
 
 Examples:
 #include "imx7ulp-pinfunc.h"
 
 /* Pin Controller Node */
-iomuxc1: iomuxc@40ac0000 {
+iomuxc1: pinctrl@40ac0000 {
 	compatible = "fsl,imx7ulp-iomuxc1";
 	reg = <0x40ac0000 0x1000>;
 
 	/* Pin Configuration Node */
 	pinctrl_lpuart4: lpuart4grp {
-		pinmux = <
-			IMX7ULP_PAD_PTC3__LPUART4_RX
-			IMX7ULP_PAD_PTC2__LPUART4_TX
+		fsl,pins = <
+			IMX7ULP_PAD_PTC3__LPUART4_RX	0x1
+			IMX7ULP_PAD_PTC2__LPUART4_TX	0x1
 		>;
-		bias-pull-up;
 	};
 };
-- 
2.7.4

[PATCH V3 06/10] pinctrl: fsl: imx7ulp: change to use imx legacy binding

[A.s. Dong]

We already had an earlier conclusion that all new i.MX Socs will keep
using the legacy i.MX Pinctrl bindings instead of generic pin config.
However, MX7ULP generic pin config binding support has already been in
tree before that time. Per SoC maintainers' suggestions, in order to
get a better consistency for all i.MX devices, we'd like to go back to
imx legacy binding for MX7ULP as well.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: linux-gpio@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v3: new patch
---
 drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 42 -----------------------------
 1 file changed, 42 deletions(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
index f521bdb..922ff73 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
@@ -256,46 +256,8 @@ static const struct pinctrl_pin_desc imx7ulp_pinctrl_pads[] = {
 
 #define BM_OBE_ENABLED		BIT(17)
 #define BM_IBE_ENABLED		BIT(16)
-#define BM_LK_ENABLED		BIT(15)
 #define BM_MUX_MODE		0xf00
 #define BP_MUX_MODE		8
-#define BM_PULL_ENABLED		BIT(1)
-
-static const struct imx_cfg_params_decode imx7ulp_cfg_decodes[] = {
-	IMX_CFG_PARAMS_DECODE(PIN_CONFIG_DRIVE_STRENGTH, 		BIT(6), 6),
-	IMX_CFG_PARAMS_DECODE(PIN_CONFIG_DRIVE_PUSH_PULL,		BIT(5), 5),
-	IMX_CFG_PARAMS_DECODE(PIN_CONFIG_SLEW_RATE,			BIT(2), 2),
-	IMX_CFG_PARAMS_DECODE(PIN_CONFIG_BIAS_DISABLE,			BIT(1), 1),
-	IMX_CFG_PARAMS_DECODE(PIN_CONFIG_BIAS_PULL_UP,			BIT(0), 0),
-
-	IMX_CFG_PARAMS_DECODE_INVERT(PIN_CONFIG_DRIVE_OPEN_DRAIN,	BIT(5), 5),
-	IMX_CFG_PARAMS_DECODE_INVERT(PIN_CONFIG_BIAS_PULL_DOWN,		BIT(0), 0),
-};
-
-static void imx7ulp_cfg_params_fixup(unsigned long *configs,
-				    unsigned int num_configs,
-				    u32 *raw_config)
-{
-	enum pin_config_param param;
-	u32 param_val;
-	int i;
-
-	/* lock field disabled */
-	*raw_config &= ~BM_LK_ENABLED;
-
-	for (i = 0; i < num_configs; i++) {
-		param = pinconf_to_config_param(configs[i]);
-		param_val = pinconf_to_config_argument(configs[i]);
-
-		if ((param == PIN_CONFIG_BIAS_PULL_UP) ||
-		    (param == PIN_CONFIG_BIAS_PULL_DOWN)) {
-			/* pull enabled */
-			*raw_config |= BM_PULL_ENABLED;
-
-			return;
-		}
-	}
-}
 
 static int imx7ulp_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 					  struct pinctrl_gpio_range *range,
@@ -326,10 +288,6 @@ static const struct imx_pinctrl_soc_info imx7ulp_pinctrl_info = {
 	.gpio_set_direction = imx7ulp_pmx_gpio_set_direction,
 	.mux_mask = BM_MUX_MODE,
 	.mux_shift = BP_MUX_MODE,
-	.generic_pinconf = true,
-	.decodes = imx7ulp_cfg_decodes,
-	.num_decodes = ARRAY_SIZE(imx7ulp_cfg_decodes),
-	.fixup = imx7ulp_cfg_params_fixup,
 };
 
 static const struct of_device_id imx7ulp_pinctrl_of_match[] = {
-- 
2.7.4