[PATCH 0/9] Runtime PM for Exynos pin controller driver

[PATCH 0/9] Runtime PM for Exynos pin controller driver

[Marek Szyprowski]

Hello,

This patchset is a next step to add support for audio power domain on
Exynos5 SoCs.

Audio power domain on Exynos5 SoCs contains following hardware modules:
1. clock controller
2. pin controller
3. PL330 DMA controller
4. I2S audio controller

Till now it was assumed that pin controller is located in the "always on"
power domain and lacked runtime power management. This patch finally
removes such assumption and adds runtime pm support and awareness to this
driver. To achieve this, some changes in the Exynos platform support code
were needed, like moving pad retention control to the pin controller driver.
Some cleanup to the pin controller driver has been also done while changing
the code. This new feature requires some additional information in the
device tree, what is handled by patches 1,2 and 9.

Please note that patches are ordered in such a way that the changes can be
bisected, so the properties are added to dts before the code requiring them.

The other patches related to enabling full support for audio power domain
can be found here:
1. PL330 ADMA controller non-irqsafe runtime PM:
   https://www.spinics.net/lists/arm-kernel/msg550008.html
2. Runtime PM for clock controllers (Exynos Audio subsystem will be added
   in v4 soon): https://www.spinics.net/lists/arm-kernel/msg538122.html

Patches are based on linux-next from 2016.12.22.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Patch summary:

Marek Szyprowski (9):
  ARM: dts: exynos: Add PMU syscon to pinctrl nodes
  ARM: dts: exynos: Add pinctrl sleep state for 542x i2s module
  pinctrl: samsung: Remove dead code
  pinctrl: samsung: Use generic of_device_get_match_data helper
  pinctrl: samsung: Move retention control from mach-exynos to the
    pinctrl driver
  pinctrl: samsung: Replace syscore ops with standard platform device
    pm_ops
  pinctrl: samsung: Add property to mark pad state as suitable for power
    down
  pinctrl: samsung: Add runtime PM support
  ARM: dts: exynos: Add audio power domain support to Exynos542x SoCs

 .../bindings/pinctrl/samsung-pinctrl.txt           |  12 ++
 arch/arm/boot/dts/exynos3250.dtsi                  |   2 +
 arch/arm/boot/dts/exynos4210.dtsi                  |   3 +
 arch/arm/boot/dts/exynos4x12.dtsi                  |   3 +
 arch/arm/boot/dts/exynos5250.dtsi                  |   4 +
 arch/arm/boot/dts/exynos5420-pinctrl.dtsi          |  11 ++
 arch/arm/boot/dts/exynos5420.dtsi                  |  18 ++-
 arch/arm/mach-exynos/suspend.c                     |  64 ---------
 drivers/pinctrl/samsung/pinctrl-exynos.c           | 148 +++++++++++++++++++++
 drivers/pinctrl/samsung/pinctrl-samsung.c          | 126 ++++++++----------
 drivers/pinctrl/samsung/pinctrl-samsung.h          |  15 +++
 11 files changed, 271 insertions(+), 135 deletions(-)

-- 
1.9.1

[PATCH 1/9] ARM: dts: exynos: Add PMU syscon to pinctrl nodes

[Marek Szyprowski]

Access to PMU regmap is needed to properly release pad retention after
suspend/resume cycle.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/boot/dts/exynos3250.dtsi | 2 ++
 arch/arm/boot/dts/exynos4210.dtsi | 3 +++
 arch/arm/boot/dts/exynos4x12.dtsi | 3 +++
 arch/arm/boot/dts/exynos5250.dtsi | 4 ++++
 arch/arm/boot/dts/exynos5420.dtsi | 5 +++++
 5 files changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/exynos3250.dtsi b/arch/arm/boot/dts/exynos3250.dtsi
index ba17ee1eb749..3614b7953fac 100644
--- a/arch/arm/boot/dts/exynos3250.dtsi
+++ b/arch/arm/boot/dts/exynos3250.dtsi
@@ -260,6 +260,7 @@
 			compatible = "samsung,exynos3250-pinctrl";
 			reg = <0x11000000 0x1000>;
 			interrupts = <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,pmu-syscon = <&pmu_system_controller>;
 
 			wakeup-interrupt-controller {
 				compatible = "samsung,exynos4210-wakeup-eint";
@@ -271,6 +272,7 @@
 			compatible = "samsung,exynos3250-pinctrl";
 			reg = <0x11400000 0x1000>;
 			interrupts = <GIC_SPI 240 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,pmu-syscon = <&pmu_system_controller>;
 		};
 
 		jpeg: codec@11830000 {
diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index 7f3a18c8f60f..c719aaf50f3b 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -128,12 +128,14 @@
 		compatible = "samsung,exynos4210-pinctrl";
 		reg = <0x11400000 0x1000>;
 		interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
+		samsung,pmu-syscon = <&pmu_system_controller>;
 	};
 
 	pinctrl_1: pinctrl@11000000 {
 		compatible = "samsung,exynos4210-pinctrl";
 		reg = <0x11000000 0x1000>;
 		interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+		samsung,pmu-syscon = <&pmu_system_controller>;
 
 		wakup_eint: wakeup-interrupt-controller {
 			compatible = "samsung,exynos4210-wakeup-eint";
@@ -145,6 +147,7 @@
 	pinctrl_2: pinctrl@03860000 {
 		compatible = "samsung,exynos4210-pinctrl";
 		reg = <0x03860000 0x1000>;
+		samsung,pmu-syscon = <&pmu_system_controller>;
 	};
 
 	tmu: tmu@100C0000 {
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index 85a7122658f1..54b65c4accd6 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -546,12 +546,14 @@
 	compatible = "samsung,exynos4x12-pinctrl";
 	reg = <0x11400000 0x1000>;
 	interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
+	samsung,pmu-syscon = <&pmu_system_controller>;
 };
 
 &pinctrl_1 {
 	compatible = "samsung,exynos4x12-pinctrl";
 	reg = <0x11000000 0x1000>;
 	interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+	samsung,pmu-syscon = <&pmu_system_controller>;
 
 	wakup_eint: wakeup-interrupt-controller {
 		compatible = "samsung,exynos4210-wakeup-eint";
@@ -565,6 +567,7 @@
 	reg = <0x03860000 0x1000>;
 	interrupt-parent = <&combiner>;
 	interrupts = <10 0>;
+	samsung,pmu-syscon = <&pmu_system_controller>;
 };
 
 &pinctrl_3 {
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index b6d7444d8585..ffff28f89589 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -196,6 +196,7 @@
 			compatible = "samsung,exynos5250-pinctrl";
 			reg = <0x11400000 0x1000>;
 			interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,pmu-syscon = <&pmu_system_controller>;
 
 			wakup_eint: wakeup-interrupt-controller {
 				compatible = "samsung,exynos4210-wakeup-eint";
@@ -208,18 +209,21 @@
 			compatible = "samsung,exynos5250-pinctrl";
 			reg = <0x13400000 0x1000>;
 			interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,pmu-syscon = <&pmu_system_controller>;
 		};
 
 		pinctrl_2: pinctrl@10d10000 {
 			compatible = "samsung,exynos5250-pinctrl";
 			reg = <0x10d10000 0x1000>;
 			interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,pmu-syscon = <&pmu_system_controller>;
 		};
 
 		pinctrl_3: pinctrl@03860000 {
 			compatible = "samsung,exynos5250-pinctrl";
 			reg = <0x03860000 0x1000>;
 			interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,pmu-syscon = <&pmu_system_controller>;
 		};
 
 		pmu_system_controller: system-controller@10040000 {
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 906a1a42a7ea..832cb56c514e 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -321,6 +321,7 @@
 			compatible = "samsung,exynos5420-pinctrl";
 			reg = <0x13400000 0x1000>;
 			interrupts = <0 45 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,pmu-syscon = <&pmu_system_controller>;
 
 			wakeup-interrupt-controller {
 				compatible = "samsung,exynos4210-wakeup-eint";
@@ -333,24 +334,28 @@
 			compatible = "samsung,exynos5420-pinctrl";
 			reg = <0x13410000 0x1000>;
 			interrupts = <0 78 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,pmu-syscon = <&pmu_system_controller>;
 		};
 
 		pinctrl_2: pinctrl@14000000 {
 			compatible = "samsung,exynos5420-pinctrl";
 			reg = <0x14000000 0x1000>;
 			interrupts = <0 46 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,pmu-syscon = <&pmu_system_controller>;
 		};
 
 		pinctrl_3: pinctrl@14010000 {
 			compatible = "samsung,exynos5420-pinctrl";
 			reg = <0x14010000 0x1000>;
 			interrupts = <0 50 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,pmu-syscon = <&pmu_system_controller>;
 		};
 
 		pinctrl_4: pinctrl@03860000 {
 			compatible = "samsung,exynos5420-pinctrl";
 			reg = <0x03860000 0x1000>;
 			interrupts = <0 47 IRQ_TYPE_LEVEL_HIGH>;
+			samsung,pmu-syscon = <&pmu_system_controller>;
 		};
 
 		amba {
-- 
1.9.1

Re: [PATCH 1/9] ARM: dts: exynos: Add PMU syscon to pinctrl nodes

[Tomasz Figa]

Hi Marek,

2016-12-23 21:24 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> Access to PMU regmap is needed to properly release pad retention after
> suspend/resume cycle.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  arch/arm/boot/dts/exynos3250.dtsi | 2 ++
>  arch/arm/boot/dts/exynos4210.dtsi | 3 +++
>  arch/arm/boot/dts/exynos4x12.dtsi | 3 +++
>  arch/arm/boot/dts/exynos5250.dtsi | 4 ++++
>  arch/arm/boot/dts/exynos5420.dtsi | 5 +++++
>  5 files changed, 17 insertions(+)
>

I think the change makes sense, but we should also update binding
documentation. Also will the driver continue to work correctly if the
property is not present?

Best regards,
Tomasz

[PATCH 2/9] ARM: dts: exynos: Add pinctrl sleep state for 542x i2s module

[Marek Szyprowski]

Add a special "sleep" state for Exynos I2S module. This state will be used
to let I2S driver to notify pin controller that it is ready for turning
power off, so the pin controller can also change its runtime state to
suspended and in the result let power domain to turn off.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/boot/dts/exynos5420-pinctrl.dtsi | 11 +++++++++++
 arch/arm/boot/dts/exynos5420.dtsi         |  3 ++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
index 3924b4fafe72..52983b6a6859 100644
--- a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
+++ b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
@@ -720,4 +720,15 @@
 		samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
 		samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
 	};
+
+	i2s0_bus_slp: i2s0-bus-slp {
+		samsung,pins = "gpz-0", "gpz-1", "gpz-2", "gpz-3",
+				"gpz-4", "gpz-5", "gpz-6";
+		samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
+		samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
+		samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
+		samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
+		samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
+		samsung,off-state;
+	};
 };
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 832cb56c514e..0a7ecdd4c5de 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -444,8 +444,9 @@
 			clock-output-names = "i2s_cdclk0";
 			#sound-dai-cells = <1>;
 			samsung,idma-addr = <0x03000000>;
-			pinctrl-names = "default";
+			pinctrl-names = "default", "sleep";
 			pinctrl-0 = <&i2s0_bus>;
+			pinctrl-1 = <&i2s0_bus_slp>;
 			status = "disabled";
 		};
 
-- 
1.9.1

Re: [PATCH 2/9] ARM: dts: exynos: Add pinctrl sleep state for 542x i2s module

[Tomasz Figa]

Hi,

2016-12-23 21:24 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> Add a special "sleep" state for Exynos I2S module. This state will be used
> to let I2S driver to notify pin controller that it is ready for turning
> power off, so the pin controller can also change its runtime state to
> suspended and in the result let power domain to turn off.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5420-pinctrl.dtsi | 11 +++++++++++
>  arch/arm/boot/dts/exynos5420.dtsi         |  3 ++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
> index 3924b4fafe72..52983b6a6859 100644
> --- a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
> @@ -720,4 +720,15 @@
>                 samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
>                 samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
>         };
> +
> +       i2s0_bus_slp: i2s0-bus-slp {
> +               samsung,pins = "gpz-0", "gpz-1", "gpz-2", "gpz-3",
> +                               "gpz-4", "gpz-5", "gpz-6";
> +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> +               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> +               samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
> +               samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;

Is it really okay to make all the pins inputs without any pull?
Wouldn't this cause them to float until the controller is disabled or
this is basically an atomic operation?

> +               samsung,off-state;

Is this property documented in the binding description?

> +       };
>  };
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> index 832cb56c514e..0a7ecdd4c5de 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -444,8 +444,9 @@
>                         clock-output-names = "i2s_cdclk0";
>                         #sound-dai-cells = <1>;
>                         samsung,idma-addr = <0x03000000>;
> -                       pinctrl-names = "default";
> +                       pinctrl-names = "default", "sleep";

Is this state documented in the binding description?

Best regards,
Tomasz

[PATCH 3/9] pinctrl: samsung: Remove dead code

[Marek Szyprowski]

'enable' parameter has been removed a while ago, so all code for handling
it can be simply removed.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 41e62391c33c..4d9262051ff1 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -356,7 +356,7 @@ static void pin_to_reg_bank(struct samsung_pinctrl_drv_data *drvdata,
 
 /* enable or disable a pinmux function */
 static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
-					unsigned group, bool enable)
+					unsigned group)
 {
 	struct samsung_pinctrl_drv_data *drvdata;
 	const struct samsung_pin_bank_type *type;
@@ -386,8 +386,7 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
 
 	data = readl(reg + type->reg_offset[PINCFG_TYPE_FUNC]);
 	data &= ~(mask << shift);
-	if (enable)
-		data |= func->val << shift;
+	data |= func->val << shift;
 	writel(data, reg + type->reg_offset[PINCFG_TYPE_FUNC]);
 
 	spin_unlock_irqrestore(&bank->slock, flags);
@@ -398,7 +397,7 @@ static int samsung_pinmux_set_mux(struct pinctrl_dev *pctldev,
 				  unsigned selector,
 				  unsigned group)
 {
-	samsung_pinmux_setup(pctldev, selector, group, true);
+	samsung_pinmux_setup(pctldev, selector, group);
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH 3/9] pinctrl: samsung: Remove dead code

[Krzysztof Kozlowski]

On Fri, Dec 23, 2016 at 01:24:43PM +0100, Marek Szyprowski wrote:
> 'enable' parameter has been removed a while ago, so all code for handling
> it can be simply removed.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 3/9] pinctrl: samsung: Remove dead code

[Tomasz Figa]

Hi,

2016-12-23 21:24 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> 'enable' parameter has been removed a while ago, so all code for handling
> it can be simply removed.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>

Acked-by: Tomasz Figa <tomasz.figa@gmail.com>

Best regards,
Tomasz

[PATCH 4/9] pinctrl: samsung: Use generic of_device_get_match_data helper

[Marek Szyprowski]

Replace custom code with generic helper.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 4d9262051ff1..a6c2ea74e0f3 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -27,6 +27,7 @@
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/irqdomain.h>
+#include <linux/of_device.h>
 #include <linux/spinlock.h>
 #include <linux/syscore_ops.h>
 
@@ -967,15 +968,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
 	return 0;
 }
 
-static const struct of_device_id samsung_pinctrl_dt_match[];
-
 /* retrieve the soc specific data */
 static const struct samsung_pin_ctrl *
 samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
 			     struct platform_device *pdev)
 {
 	int id;
-	const struct of_device_id *match;
+	const struct samsung_pin_ctrl *match_data;
 	struct device_node *node = pdev->dev.of_node;
 	struct device_node *np;
 	const struct samsung_pin_bank_data *bdata;
@@ -990,8 +989,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
 		dev_err(&pdev->dev, "failed to get alias id\n");
 		return ERR_PTR(-ENOENT);
 	}
-	match = of_match_node(samsung_pinctrl_dt_match, node);
-	ctrl = (struct samsung_pin_ctrl *)match->data + id;
+	match_data = of_device_get_match_data(&pdev->dev);
+	ctrl = match_data + id;
 
 	d->suspend = ctrl->suspend;
 	d->resume = ctrl->resume;
-- 
1.9.1

Re: [PATCH 4/9] pinctrl: samsung: Use generic of_device_get_match_data helper

[Krzysztof Kozlowski]

On Fri, Dec 23, 2016 at 01:24:44PM +0100, Marek Szyprowski wrote:
> Replace custom code with generic helper.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 4d9262051ff1..a6c2ea74e0f3 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -27,6 +27,7 @@
>  #include <linux/err.h>
>  #include <linux/gpio.h>
>  #include <linux/irqdomain.h>
> +#include <linux/of_device.h>
>  #include <linux/spinlock.h>
>  #include <linux/syscore_ops.h>
>  
> @@ -967,15 +968,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
>  	return 0;
>  }
>  
> -static const struct of_device_id samsung_pinctrl_dt_match[];
> -
>  /* retrieve the soc specific data */
>  static const struct samsung_pin_ctrl *
>  samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
>  			     struct platform_device *pdev)
>  {
>  	int id;
> -	const struct of_device_id *match;
> +	const struct samsung_pin_ctrl *match_data;
>  	struct device_node *node = pdev->dev.of_node;
>  	struct device_node *np;
>  	const struct samsung_pin_bank_data *bdata;
> @@ -990,8 +989,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
>  		dev_err(&pdev->dev, "failed to get alias id\n");
>  		return ERR_PTR(-ENOENT);
>  	}
> -	match = of_match_node(samsung_pinctrl_dt_match, node);
> -	ctrl = (struct samsung_pin_ctrl *)match->data + id;
> +	match_data = of_device_get_match_data(&pdev->dev);
> +	ctrl = match_data + id;

Either you need a check for match_data != NULL or just remove match_data
and:
	ctrl = of_device_get_match_data();
	ctrl += id;

Actually match_data can be removed even with the check for non-NULL...

Best regards,
Krzysztof

>  
>  	d->suspend = ctrl->suspend;
>  	d->resume = ctrl->resume;
> -- 
> 1.9.1
> 

Re: [PATCH 4/9] pinctrl: samsung: Use generic of_device_get_match_data helper

[Tomasz Figa]

2016-12-25 21:56 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
> On Fri, Dec 23, 2016 at 01:24:44PM +0100, Marek Szyprowski wrote:
>> Replace custom code with generic helper.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
>> index 4d9262051ff1..a6c2ea74e0f3 100644
>> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
>> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
>> @@ -27,6 +27,7 @@
>>  #include <linux/err.h>
>>  #include <linux/gpio.h>
>>  #include <linux/irqdomain.h>
>> +#include <linux/of_device.h>
>>  #include <linux/spinlock.h>
>>  #include <linux/syscore_ops.h>
>>
>> @@ -967,15 +968,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
>>       return 0;
>>  }
>>
>> -static const struct of_device_id samsung_pinctrl_dt_match[];
>> -
>>  /* retrieve the soc specific data */
>>  static const struct samsung_pin_ctrl *
>>  samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
>>                            struct platform_device *pdev)
>>  {
>>       int id;
>> -     const struct of_device_id *match;
>> +     const struct samsung_pin_ctrl *match_data;
>>       struct device_node *node = pdev->dev.of_node;
>>       struct device_node *np;
>>       const struct samsung_pin_bank_data *bdata;
>> @@ -990,8 +989,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
>>               dev_err(&pdev->dev, "failed to get alias id\n");
>>               return ERR_PTR(-ENOENT);
>>       }
>> -     match = of_match_node(samsung_pinctrl_dt_match, node);
>> -     ctrl = (struct samsung_pin_ctrl *)match->data + id;
>> +     match_data = of_device_get_match_data(&pdev->dev);
>> +     ctrl = match_data + id;
>
> Either you need a check for match_data != NULL or just remove match_data
> and:
>         ctrl = of_device_get_match_data();
>         ctrl += id;
>
> Actually match_data can be removed even with the check for non-NULL...

I don't think this function can ever return NULL if the match array
contains a non-NULL value for each compatible string and the driver
can be probed only by DT.
But you still need to cast the match data pointer to the correct type
and using a variable for it IMHO makes the code cleaner.

Acked-by: Tomasz Figa <tomasz.figa@gmail.com>

Best regards,
Tomasz

Re: [PATCH 4/9] pinctrl: samsung: Use generic of_device_get_match_data helper

[Krzysztof Kozlowski]

On Mon, Dec 26, 2016 at 02:44:26PM +0900, Tomasz Figa wrote:
> 2016-12-25 21:56 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
> > On Fri, Dec 23, 2016 at 01:24:44PM +0100, Marek Szyprowski wrote:
> >> Replace custom code with generic helper.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >>  drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> >> index 4d9262051ff1..a6c2ea74e0f3 100644
> >> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> >> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> >> @@ -27,6 +27,7 @@
> >>  #include <linux/err.h>
> >>  #include <linux/gpio.h>
> >>  #include <linux/irqdomain.h>
> >> +#include <linux/of_device.h>
> >>  #include <linux/spinlock.h>
> >>  #include <linux/syscore_ops.h>
> >>
> >> @@ -967,15 +968,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
> >>       return 0;
> >>  }
> >>
> >> -static const struct of_device_id samsung_pinctrl_dt_match[];
> >> -
> >>  /* retrieve the soc specific data */
> >>  static const struct samsung_pin_ctrl *
> >>  samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
> >>                            struct platform_device *pdev)
> >>  {
> >>       int id;
> >> -     const struct of_device_id *match;
> >> +     const struct samsung_pin_ctrl *match_data;
> >>       struct device_node *node = pdev->dev.of_node;
> >>       struct device_node *np;
> >>       const struct samsung_pin_bank_data *bdata;
> >> @@ -990,8 +989,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
> >>               dev_err(&pdev->dev, "failed to get alias id\n");
> >>               return ERR_PTR(-ENOENT);
> >>       }
> >> -     match = of_match_node(samsung_pinctrl_dt_match, node);
> >> -     ctrl = (struct samsung_pin_ctrl *)match->data + id;
> >> +     match_data = of_device_get_match_data(&pdev->dev);
> >> +     ctrl = match_data + id;
> >
> > Either you need a check for match_data != NULL or just remove match_data
> > and:
> >         ctrl = of_device_get_match_data();
> >         ctrl += id;
> >
> > Actually match_data can be removed even with the check for non-NULL...
> 
> I don't think this function can ever return NULL if the match array
> contains a non-NULL value for each compatible string and the driver
> can be probed only by DT.

Practically it cannot (or it should not) but defensive coding is a good
practice...

> But you still need to cast the match data pointer to the correct type
> and using a variable for it IMHO makes the code cleaner.

What do you mean by casting through variable? match_data and ctrl are
the same type so there is no change by intermediate variable. It just
obfuscates the code.

BR,
Krzysztof

Re: [PATCH 4/9] pinctrl: samsung: Use generic of_device_get_match_data helper

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday, December 26, 2016 11:41:42 AM Krzysztof Kozlowski wrote:
> On Mon, Dec 26, 2016 at 02:44:26PM +0900, Tomasz Figa wrote:
> > 2016-12-25 21:56 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
> > > On Fri, Dec 23, 2016 at 01:24:44PM +0100, Marek Szyprowski wrote:
> > >> Replace custom code with generic helper.
> > >>
> > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > >> ---
> > >>  drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++-----
> > >>  1 file changed, 4 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> > >> index 4d9262051ff1..a6c2ea74e0f3 100644
> > >> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> > >> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> > >> @@ -27,6 +27,7 @@
> > >>  #include <linux/err.h>
> > >>  #include <linux/gpio.h>
> > >>  #include <linux/irqdomain.h>
> > >> +#include <linux/of_device.h>
> > >>  #include <linux/spinlock.h>
> > >>  #include <linux/syscore_ops.h>
> > >>
> > >> @@ -967,15 +968,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
> > >>       return 0;
> > >>  }
> > >>
> > >> -static const struct of_device_id samsung_pinctrl_dt_match[];
> > >> -
> > >>  /* retrieve the soc specific data */
> > >>  static const struct samsung_pin_ctrl *
> > >>  samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
> > >>                            struct platform_device *pdev)
> > >>  {
> > >>       int id;
> > >> -     const struct of_device_id *match;
> > >> +     const struct samsung_pin_ctrl *match_data;
> > >>       struct device_node *node = pdev->dev.of_node;
> > >>       struct device_node *np;
> > >>       const struct samsung_pin_bank_data *bdata;
> > >> @@ -990,8 +989,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
> > >>               dev_err(&pdev->dev, "failed to get alias id\n");
> > >>               return ERR_PTR(-ENOENT);
> > >>       }
> > >> -     match = of_match_node(samsung_pinctrl_dt_match, node);
> > >> -     ctrl = (struct samsung_pin_ctrl *)match->data + id;
> > >> +     match_data = of_device_get_match_data(&pdev->dev);
> > >> +     ctrl = match_data + id;
> > >
> > > Either you need a check for match_data != NULL or just remove match_data
> > > and:
> > >         ctrl = of_device_get_match_data();
> > >         ctrl += id;
> > >
> > > Actually match_data can be removed even with the check for non-NULL...
> > 
> > I don't think this function can ever return NULL if the match array
> > contains a non-NULL value for each compatible string and the driver
> > can be probed only by DT.
> 
> Practically it cannot (or it should not) but defensive coding is a good
> practice...

Defensive coding is not a good practice, please don't recommend it.

It usually just obfuscates code and hides underlying issue.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 5/9] pinctrl: samsung: Move retention control from mach-exynos to the pinctrl driver

[Marek Szyprowski]

Pad retention control after suspend/resume cycle should be done from pin
controller driver instead of PMU (power management unit) driver to avoid
possible ordering and logical dependencies. Till now it worked fine only
because PMU driver registered its sys_ops after pin controller.

This patch moves pad retention control from PMU driver to Exynos pin
controller driver. This is a preparation for adding new features to Exynos
pin controller driver, like runtime power management and suspending
individual pin controllers, which might be a part of some power domain.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 .../bindings/pinctrl/samsung-pinctrl.txt           |   4 +
 arch/arm/mach-exynos/suspend.c                     |  64 ---------
 drivers/pinctrl/samsung/pinctrl-exynos.c           | 148 +++++++++++++++++++++
 drivers/pinctrl/samsung/pinctrl-samsung.c          |  16 ++-
 drivers/pinctrl/samsung/pinctrl-samsung.h          |  13 ++
 5 files changed, 178 insertions(+), 67 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
index 1baf19eecabf..b7bd2e12a269 100644
--- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
@@ -43,6 +43,10 @@ Required Properties:
 		};
 	};
 
+- samsung,pmu-syscon: Phandle to the PMU system controller, to let driver
+  to control pad retention after system suspend/resume cycle (only for Exynos
+  SoC series).
+
 - Pin banks as child nodes: Pin banks of the controller are represented by child
   nodes of the controller node. Bank name is taken from name of the node. Each
   bank node must contain following properties:
diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
index 06332f626565..10bc753624be 100644
--- a/arch/arm/mach-exynos/suspend.c
+++ b/arch/arm/mach-exynos/suspend.c
@@ -57,7 +57,6 @@ struct exynos_wkup_irq {
 struct exynos_pm_data {
 	const struct exynos_wkup_irq *wkup_irq;
 	unsigned int wake_disable_mask;
-	unsigned int *release_ret_regs;
 
 	void (*pm_prepare)(void);
 	void (*pm_resume_prepare)(void);
@@ -95,47 +94,6 @@ struct exynos_pm_data {
 	{ /* sentinel */ },
 };
 
-static unsigned int exynos_release_ret_regs[] = {
-	S5P_PAD_RET_MAUDIO_OPTION,
-	S5P_PAD_RET_GPIO_OPTION,
-	S5P_PAD_RET_UART_OPTION,
-	S5P_PAD_RET_MMCA_OPTION,
-	S5P_PAD_RET_MMCB_OPTION,
-	S5P_PAD_RET_EBIA_OPTION,
-	S5P_PAD_RET_EBIB_OPTION,
-	REG_TABLE_END,
-};
-
-static unsigned int exynos3250_release_ret_regs[] = {
-	S5P_PAD_RET_MAUDIO_OPTION,
-	S5P_PAD_RET_GPIO_OPTION,
-	S5P_PAD_RET_UART_OPTION,
-	S5P_PAD_RET_MMCA_OPTION,
-	S5P_PAD_RET_MMCB_OPTION,
-	S5P_PAD_RET_EBIA_OPTION,
-	S5P_PAD_RET_EBIB_OPTION,
-	S5P_PAD_RET_MMC2_OPTION,
-	S5P_PAD_RET_SPI_OPTION,
-	REG_TABLE_END,
-};
-
-static unsigned int exynos5420_release_ret_regs[] = {
-	EXYNOS_PAD_RET_DRAM_OPTION,
-	EXYNOS_PAD_RET_MAUDIO_OPTION,
-	EXYNOS_PAD_RET_JTAG_OPTION,
-	EXYNOS5420_PAD_RET_GPIO_OPTION,
-	EXYNOS5420_PAD_RET_UART_OPTION,
-	EXYNOS5420_PAD_RET_MMCA_OPTION,
-	EXYNOS5420_PAD_RET_MMCB_OPTION,
-	EXYNOS5420_PAD_RET_MMCC_OPTION,
-	EXYNOS5420_PAD_RET_HSI_OPTION,
-	EXYNOS_PAD_RET_EBIA_OPTION,
-	EXYNOS_PAD_RET_EBIB_OPTION,
-	EXYNOS5420_PAD_RET_SPI_OPTION,
-	EXYNOS5420_PAD_RET_DRAM_COREBLK_OPTION,
-	REG_TABLE_END,
-};
-
 static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
 {
 	const struct exynos_wkup_irq *wkup_irq;
@@ -442,15 +400,6 @@ static int exynos5420_pm_suspend(void)
 	return 0;
 }
 
-static void exynos_pm_release_retention(void)
-{
-	unsigned int i;
-
-	for (i = 0; (pm_data->release_ret_regs[i] != REG_TABLE_END); i++)
-		pmu_raw_writel(EXYNOS_WAKEUP_FROM_LOWPWR,
-				pm_data->release_ret_regs[i]);
-}
-
 static void exynos_pm_resume(void)
 {
 	u32 cpuid = read_cpuid_part();
@@ -458,9 +407,6 @@ static void exynos_pm_resume(void)
 	if (exynos_pm_central_resume())
 		goto early_wakeup;
 
-	/* For release retention */
-	exynos_pm_release_retention();
-
 	if (cpuid == ARM_CPU_PART_CORTEX_A9)
 		scu_enable(S5P_VA_SCU);
 
@@ -482,9 +428,6 @@ static void exynos3250_pm_resume(void)
 	if (exynos_pm_central_resume())
 		goto early_wakeup;
 
-	/* For release retention */
-	exynos_pm_release_retention();
-
 	pmu_raw_writel(S5P_USE_STANDBY_WFI_ALL, S5P_CENTRAL_SEQ_OPTION);
 
 	if (call_firmware_op(resume) == -ENOSYS
@@ -522,9 +465,6 @@ static void exynos5420_pm_resume(void)
 	if (exynos_pm_central_resume())
 		goto early_wakeup;
 
-	/* For release retention */
-	exynos_pm_release_retention();
-
 	pmu_raw_writel(exynos_pmu_spare3, S5P_PMU_SPARE3);
 
 early_wakeup:
@@ -637,7 +577,6 @@ static void exynos_suspend_finish(void)
 static const struct exynos_pm_data exynos3250_pm_data = {
 	.wkup_irq	= exynos3250_wkup_irq,
 	.wake_disable_mask = ((0xFF << 8) | (0x1F << 1)),
-	.release_ret_regs = exynos3250_release_ret_regs,
 	.pm_suspend	= exynos_pm_suspend,
 	.pm_resume	= exynos3250_pm_resume,
 	.pm_prepare	= exynos3250_pm_prepare,
@@ -647,7 +586,6 @@ static void exynos_suspend_finish(void)
 static const struct exynos_pm_data exynos4_pm_data = {
 	.wkup_irq	= exynos4_wkup_irq,
 	.wake_disable_mask = ((0xFF << 8) | (0x1F << 1)),
-	.release_ret_regs = exynos_release_ret_regs,
 	.pm_suspend	= exynos_pm_suspend,
 	.pm_resume	= exynos_pm_resume,
 	.pm_prepare	= exynos_pm_prepare,
@@ -657,7 +595,6 @@ static void exynos_suspend_finish(void)
 static const struct exynos_pm_data exynos5250_pm_data = {
 	.wkup_irq	= exynos5250_wkup_irq,
 	.wake_disable_mask = ((0xFF << 8) | (0x1F << 1)),
-	.release_ret_regs = exynos_release_ret_regs,
 	.pm_suspend	= exynos_pm_suspend,
 	.pm_resume	= exynos_pm_resume,
 	.pm_prepare	= exynos_pm_prepare,
@@ -667,7 +604,6 @@ static void exynos_suspend_finish(void)
 static const struct exynos_pm_data exynos5420_pm_data = {
 	.wkup_irq	= exynos5250_wkup_irq,
 	.wake_disable_mask = (0x7F << 7) | (0x1F << 1),
-	.release_ret_regs = exynos5420_release_ret_regs,
 	.pm_resume_prepare = exynos5420_prepare_pm_resume,
 	.pm_resume	= exynos5420_pm_resume,
 	.pm_suspend	= exynos5420_pm_suspend,
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index 12f7d1eb65bc..55c1104a1ccf 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -24,11 +24,15 @@
 #include <linux/irqdomain.h>
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of_irq.h>
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/regmap.h>
 #include <linux/err.h>
+#include <linux/soc/samsung/exynos-regs-pmu.h>
+
 
 #include "pinctrl-samsung.h"
 #include "pinctrl-exynos.h"
@@ -633,6 +637,46 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 			exynos_pinctrl_resume_bank(drvdata, bank);
 }
 
+static atomic_t exynos_retention_refcnt;
+static struct regmap *pmu_regs;
+
+static int exynos_retention_init(struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct device *dev = drvdata->dev;
+
+	pmu_regs = syscon_regmap_lookup_by_phandle(dev->of_node,
+						   "samsung,pmu-syscon");
+	if (IS_ERR(pmu_regs)) {
+		dev_err(dev, "failed to lookup PMU regmap, no support for pad retention\n");
+		return PTR_ERR(pmu_regs);
+	}
+	return 0;
+}
+
+static void exynos_retention_on(struct samsung_pinctrl_drv_data *drvdata)
+{
+	atomic_inc(&exynos_retention_refcnt);
+}
+
+static void exynos_retention_off(struct samsung_pinctrl_drv_data *drvdata)
+{
+	int i;
+
+	if (!atomic_dec_and_test(&exynos_retention_refcnt) || IS_ERR(pmu_regs))
+		return;
+
+	for (i = 0; i < drvdata->nr_retention_regs; i++)
+		regmap_write(pmu_regs, drvdata->retention_regs[i],
+			     EXYNOS_WAKEUP_FROM_LOWPWR);
+}
+
+static void exynos_retention_audio_off(struct samsung_pinctrl_drv_data *drvdata)
+{
+	if (!IS_ERR(pmu_regs))
+		regmap_write(pmu_regs, S5P_PAD_RET_MAUDIO_OPTION,
+			     EXYNOS_WAKEUP_FROM_LOWPWR);
+}
+
 /* pin banks of s5pv210 pin-controller */
 static const struct samsung_pin_bank_data s5pv210_pin_bank[] __initconst = {
 	EXYNOS_PIN_BANK_EINTG(8, 0x000, "gpa0", 0x00),
@@ -714,6 +758,18 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 	EXYNOS_PIN_BANK_EINTW(8, 0xc60, "gpx3", 0x0c),
 };
 
+static const u32 exynos3250_retention_regs[] = {
+	S5P_PAD_RET_MAUDIO_OPTION,
+	S5P_PAD_RET_GPIO_OPTION,
+	S5P_PAD_RET_UART_OPTION,
+	S5P_PAD_RET_MMCA_OPTION,
+	S5P_PAD_RET_MMCB_OPTION,
+	S5P_PAD_RET_EBIA_OPTION,
+	S5P_PAD_RET_EBIB_OPTION,
+	S5P_PAD_RET_MMC2_OPTION,
+	S5P_PAD_RET_SPI_OPTION,
+};
+
 /*
  * Samsung pinctrl driver data for Exynos3250 SoC. Exynos3250 SoC includes
  * two gpio/pin-mux/pinconfig controllers.
@@ -726,6 +782,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_regs = exynos3250_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos3250_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	}, {
 		/* pin-controller instance 1 data */
 		.pin_banks	= exynos3250_pin_banks1,
@@ -734,6 +795,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_wkup_init = exynos_eint_wkup_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_regs = exynos3250_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos3250_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	},
 };
 
@@ -786,6 +852,15 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 	EXYNOS_PIN_BANK_EINTN(7, 0x000, "gpz"),
 };
 
+static const u32 exynos4_retention_regs[] = {
+	S5P_PAD_RET_GPIO_OPTION,
+	S5P_PAD_RET_UART_OPTION,
+	S5P_PAD_RET_MMCA_OPTION,
+	S5P_PAD_RET_MMCB_OPTION,
+	S5P_PAD_RET_EBIA_OPTION,
+	S5P_PAD_RET_EBIB_OPTION,
+};
+
 /*
  * Samsung pinctrl driver data for Exynos4210 SoC. Exynos4210 SoC includes
  * three gpio/pin-mux/pinconfig controllers.
@@ -798,6 +873,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_regs = exynos4_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	}, {
 		/* pin-controller instance 1 data */
 		.pin_banks	= exynos4210_pin_banks1,
@@ -806,10 +886,17 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_wkup_init = exynos_eint_wkup_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_regs = exynos4_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	}, {
 		/* pin-controller instance 2 data */
 		.pin_banks	= exynos4210_pin_banks2,
 		.nr_banks	= ARRAY_SIZE(exynos4210_pin_banks2),
+		.retention_init = exynos_retention_init,
+		.retention_off   = exynos_retention_audio_off,
 	},
 };
 
@@ -883,6 +970,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_regs = exynos4_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	}, {
 		/* pin-controller instance 1 data */
 		.pin_banks	= exynos4x12_pin_banks1,
@@ -891,6 +983,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_wkup_init = exynos_eint_wkup_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_regs = exynos4_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	}, {
 		/* pin-controller instance 2 data */
 		.pin_banks	= exynos4x12_pin_banks2,
@@ -898,6 +995,8 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_init = exynos_retention_init,
+		.retention_off   = exynos_retention_audio_off,
 	}, {
 		/* pin-controller instance 3 data */
 		.pin_banks	= exynos4x12_pin_banks3,
@@ -1052,6 +1151,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_wkup_init = exynos_eint_wkup_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_regs = exynos4_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	}, {
 		/* pin-controller instance 1 data */
 		.pin_banks	= exynos5250_pin_banks1,
@@ -1059,6 +1163,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_regs = exynos4_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	}, {
 		/* pin-controller instance 2 data */
 		.pin_banks	= exynos5250_pin_banks2,
@@ -1073,6 +1182,8 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_init = exynos_retention_init,
+		.retention_off   = exynos_retention_audio_off,
 	},
 };
 
@@ -1299,6 +1410,21 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 	EXYNOS_PIN_BANK_EINTG(7, 0x000, "gpz", 0x00),
 };
 
+static const u32 exynos5420_retention_regs[] = {
+	EXYNOS_PAD_RET_DRAM_OPTION,
+	EXYNOS_PAD_RET_JTAG_OPTION,
+	EXYNOS5420_PAD_RET_GPIO_OPTION,
+	EXYNOS5420_PAD_RET_UART_OPTION,
+	EXYNOS5420_PAD_RET_MMCA_OPTION,
+	EXYNOS5420_PAD_RET_MMCB_OPTION,
+	EXYNOS5420_PAD_RET_MMCC_OPTION,
+	EXYNOS5420_PAD_RET_HSI_OPTION,
+	EXYNOS_PAD_RET_EBIA_OPTION,
+	EXYNOS_PAD_RET_EBIB_OPTION,
+	EXYNOS5420_PAD_RET_SPI_OPTION,
+	EXYNOS5420_PAD_RET_DRAM_COREBLK_OPTION,
+};
+
 /*
  * Samsung pinctrl driver data for Exynos5420 SoC. Exynos5420 SoC includes
  * four gpio/pin-mux/pinconfig controllers.
@@ -1310,26 +1436,48 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks0),
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.eint_wkup_init = exynos_eint_wkup_init,
+		.retention_regs = exynos5420_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos5420_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	}, {
 		/* pin-controller instance 1 data */
 		.pin_banks	= exynos5420_pin_banks1,
 		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks1),
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.retention_regs = exynos5420_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos5420_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	}, {
 		/* pin-controller instance 2 data */
 		.pin_banks	= exynos5420_pin_banks2,
 		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks2),
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.retention_regs = exynos5420_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos5420_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	}, {
 		/* pin-controller instance 3 data */
 		.pin_banks	= exynos5420_pin_banks3,
 		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks3),
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.retention_regs = exynos5420_retention_regs,
+		.nr_retention_regs = ARRAY_SIZE(exynos5420_retention_regs),
+		.retention_init = exynos_retention_init,
+		.retention_on   = exynos_retention_on,
+		.retention_off   = exynos_retention_off,
 	}, {
 		/* pin-controller instance 4 data */
 		.pin_banks	= exynos5420_pin_banks4,
 		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks4),
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.retention_init = exynos_retention_init,
+		.retention_off  = exynos_retention_audio_off,
 	},
 };
 
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index a6c2ea74e0f3..cb425e6837f9 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -1011,6 +1011,11 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
 			return ERR_PTR(-EIO);
 	}
 
+	d->retention_regs = ctrl->retention_regs;
+	d->nr_retention_regs = ctrl->nr_retention_regs;
+	d->retention_on = ctrl->retention_on;
+	d->retention_off = ctrl->retention_off;
+
 	bank = d->pin_banks;
 	bdata = ctrl->pin_banks;
 	for (i = 0; i < ctrl->nr_banks; ++i, ++bdata, ++bank) {
@@ -1087,6 +1092,8 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 		ctrl->eint_gpio_init(drvdata);
 	if (ctrl->eint_wkup_init)
 		ctrl->eint_wkup_init(drvdata);
+	if (ctrl->retention_init)
+		ctrl->retention_init(drvdata);
 
 	platform_set_drvdata(pdev, drvdata);
 
@@ -1139,15 +1146,15 @@ static void samsung_pinctrl_suspend_dev(
 
 	if (drvdata->suspend)
 		drvdata->suspend(drvdata);
+	if (drvdata->retention_on)
+		drvdata->retention_on(drvdata);
+
 }
 
 /**
  * samsung_pinctrl_resume_dev - restore pinctrl state from suspend for a device
  *
  * Restore one of the banks that was saved during suspend.
- *
- * We don't bother doing anything complicated to avoid glitching lines since
- * we're called before pad retention is turned off.
  */
 static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 {
@@ -1186,6 +1193,9 @@ static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 			if (widths[type])
 				writel(bank->pm_save[type], reg + offs[type]);
 	}
+
+	if (drvdata->retention_off)
+		drvdata->retention_off(drvdata);
 }
 
 /**
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 043cb6c11180..32b949e2a89b 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -199,10 +199,17 @@ struct samsung_pin_ctrl {
 	u32		nr_banks;
 	int		nr_ext_resources;
 
+	const u32	*retention_regs;
+	int		nr_retention_regs;
+
 	int		(*eint_gpio_init)(struct samsung_pinctrl_drv_data *);
 	int		(*eint_wkup_init)(struct samsung_pinctrl_drv_data *);
 	void		(*suspend)(struct samsung_pinctrl_drv_data *);
 	void		(*resume)(struct samsung_pinctrl_drv_data *);
+
+	int		(*retention_init)(struct samsung_pinctrl_drv_data *);
+	void		(*retention_on)(struct samsung_pinctrl_drv_data *);
+	void		(*retention_off)(struct samsung_pinctrl_drv_data *);
 };
 
 /**
@@ -238,6 +245,12 @@ struct samsung_pinctrl_drv_data {
 	unsigned int			pin_base;
 	unsigned int			nr_pins;
 
+	const u32			*retention_regs;
+	int				nr_retention_regs;
+
+	void (*retention_on)(struct samsung_pinctrl_drv_data *);
+	void (*retention_off)(struct samsung_pinctrl_drv_data *);
+
 	void (*suspend)(struct samsung_pinctrl_drv_data *);
 	void (*resume)(struct samsung_pinctrl_drv_data *);
 };
-- 
1.9.1

Re: [PATCH 5/9] pinctrl: samsung: Move retention control from mach-exynos to the pinctrl driver

[Krzysztof Kozlowski]

On Fri, Dec 23, 2016 at 01:24:45PM +0100, Marek Szyprowski wrote:
> Pad retention control after suspend/resume cycle should be done from pin
> controller driver instead of PMU (power management unit) driver to avoid
> possible ordering and logical dependencies. Till now it worked fine only
> because PMU driver registered its sys_ops after pin controller.
> 
> This patch moves pad retention control from PMU driver to Exynos pin
> controller driver. This is a preparation for adding new features to Exynos
> pin controller driver, like runtime power management and suspending
> individual pin controllers, which might be a part of some power domain.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  .../bindings/pinctrl/samsung-pinctrl.txt           |   4 +
>  arch/arm/mach-exynos/suspend.c                     |  64 ---------
>  drivers/pinctrl/samsung/pinctrl-exynos.c           | 148 +++++++++++++++++++++
>  drivers/pinctrl/samsung/pinctrl-samsung.c          |  16 ++-
>  drivers/pinctrl/samsung/pinctrl-samsung.h          |  13 ++
>  5 files changed, 178 insertions(+), 67 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> index 1baf19eecabf..b7bd2e12a269 100644
> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> @@ -43,6 +43,10 @@ Required Properties:
>  		};
>  	};
>  
> +- samsung,pmu-syscon: Phandle to the PMU system controller, to let driver
> +  to control pad retention after system suspend/resume cycle (only for Exynos
> +  SoC series).
> +
>  - Pin banks as child nodes: Pin banks of the controller are represented by child
>    nodes of the controller node. Bank name is taken from name of the node. Each
>    bank node must contain following properties:
> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
> index 06332f626565..10bc753624be 100644
> --- a/arch/arm/mach-exynos/suspend.c
> +++ b/arch/arm/mach-exynos/suspend.c
> @@ -57,7 +57,6 @@ struct exynos_wkup_irq {
>  struct exynos_pm_data {
>  	const struct exynos_wkup_irq *wkup_irq;
>  	unsigned int wake_disable_mask;
> -	unsigned int *release_ret_regs;
>  
>  	void (*pm_prepare)(void);
>  	void (*pm_resume_prepare)(void);
> @@ -95,47 +94,6 @@ struct exynos_pm_data {
>  	{ /* sentinel */ },
>  };
>  
> -static unsigned int exynos_release_ret_regs[] = {
> -	S5P_PAD_RET_MAUDIO_OPTION,
> -	S5P_PAD_RET_GPIO_OPTION,
> -	S5P_PAD_RET_UART_OPTION,
> -	S5P_PAD_RET_MMCA_OPTION,
> -	S5P_PAD_RET_MMCB_OPTION,
> -	S5P_PAD_RET_EBIA_OPTION,
> -	S5P_PAD_RET_EBIB_OPTION,
> -	REG_TABLE_END,
> -};
> -
> -static unsigned int exynos3250_release_ret_regs[] = {
> -	S5P_PAD_RET_MAUDIO_OPTION,
> -	S5P_PAD_RET_GPIO_OPTION,
> -	S5P_PAD_RET_UART_OPTION,
> -	S5P_PAD_RET_MMCA_OPTION,
> -	S5P_PAD_RET_MMCB_OPTION,
> -	S5P_PAD_RET_EBIA_OPTION,
> -	S5P_PAD_RET_EBIB_OPTION,
> -	S5P_PAD_RET_MMC2_OPTION,
> -	S5P_PAD_RET_SPI_OPTION,
> -	REG_TABLE_END,
> -};
> -
> -static unsigned int exynos5420_release_ret_regs[] = {
> -	EXYNOS_PAD_RET_DRAM_OPTION,
> -	EXYNOS_PAD_RET_MAUDIO_OPTION,
> -	EXYNOS_PAD_RET_JTAG_OPTION,
> -	EXYNOS5420_PAD_RET_GPIO_OPTION,
> -	EXYNOS5420_PAD_RET_UART_OPTION,
> -	EXYNOS5420_PAD_RET_MMCA_OPTION,
> -	EXYNOS5420_PAD_RET_MMCB_OPTION,
> -	EXYNOS5420_PAD_RET_MMCC_OPTION,
> -	EXYNOS5420_PAD_RET_HSI_OPTION,
> -	EXYNOS_PAD_RET_EBIA_OPTION,
> -	EXYNOS_PAD_RET_EBIB_OPTION,
> -	EXYNOS5420_PAD_RET_SPI_OPTION,
> -	EXYNOS5420_PAD_RET_DRAM_COREBLK_OPTION,
> -	REG_TABLE_END,
> -};
> -
>  static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
>  {
>  	const struct exynos_wkup_irq *wkup_irq;
> @@ -442,15 +400,6 @@ static int exynos5420_pm_suspend(void)
>  	return 0;
>  }
>  
> -static void exynos_pm_release_retention(void)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; (pm_data->release_ret_regs[i] != REG_TABLE_END); i++)
> -		pmu_raw_writel(EXYNOS_WAKEUP_FROM_LOWPWR,
> -				pm_data->release_ret_regs[i]);
> -}
> -
>  static void exynos_pm_resume(void)
>  {
>  	u32 cpuid = read_cpuid_part();
> @@ -458,9 +407,6 @@ static void exynos_pm_resume(void)
>  	if (exynos_pm_central_resume())
>  		goto early_wakeup;
>  
> -	/* For release retention */
> -	exynos_pm_release_retention();
> -
>  	if (cpuid == ARM_CPU_PART_CORTEX_A9)
>  		scu_enable(S5P_VA_SCU);
>  
> @@ -482,9 +428,6 @@ static void exynos3250_pm_resume(void)
>  	if (exynos_pm_central_resume())
>  		goto early_wakeup;
>  
> -	/* For release retention */
> -	exynos_pm_release_retention();
> -
>  	pmu_raw_writel(S5P_USE_STANDBY_WFI_ALL, S5P_CENTRAL_SEQ_OPTION);
>  
>  	if (call_firmware_op(resume) == -ENOSYS
> @@ -522,9 +465,6 @@ static void exynos5420_pm_resume(void)
>  	if (exynos_pm_central_resume())
>  		goto early_wakeup;
>  
> -	/* For release retention */
> -	exynos_pm_release_retention();
> -
>  	pmu_raw_writel(exynos_pmu_spare3, S5P_PMU_SPARE3);
>  
>  early_wakeup:
> @@ -637,7 +577,6 @@ static void exynos_suspend_finish(void)
>  static const struct exynos_pm_data exynos3250_pm_data = {
>  	.wkup_irq	= exynos3250_wkup_irq,
>  	.wake_disable_mask = ((0xFF << 8) | (0x1F << 1)),
> -	.release_ret_regs = exynos3250_release_ret_regs,
>  	.pm_suspend	= exynos_pm_suspend,
>  	.pm_resume	= exynos3250_pm_resume,
>  	.pm_prepare	= exynos3250_pm_prepare,
> @@ -647,7 +586,6 @@ static void exynos_suspend_finish(void)
>  static const struct exynos_pm_data exynos4_pm_data = {
>  	.wkup_irq	= exynos4_wkup_irq,
>  	.wake_disable_mask = ((0xFF << 8) | (0x1F << 1)),
> -	.release_ret_regs = exynos_release_ret_regs,
>  	.pm_suspend	= exynos_pm_suspend,
>  	.pm_resume	= exynos_pm_resume,
>  	.pm_prepare	= exynos_pm_prepare,
> @@ -657,7 +595,6 @@ static void exynos_suspend_finish(void)
>  static const struct exynos_pm_data exynos5250_pm_data = {
>  	.wkup_irq	= exynos5250_wkup_irq,
>  	.wake_disable_mask = ((0xFF << 8) | (0x1F << 1)),
> -	.release_ret_regs = exynos_release_ret_regs,
>  	.pm_suspend	= exynos_pm_suspend,
>  	.pm_resume	= exynos_pm_resume,
>  	.pm_prepare	= exynos_pm_prepare,
> @@ -667,7 +604,6 @@ static void exynos_suspend_finish(void)
>  static const struct exynos_pm_data exynos5420_pm_data = {
>  	.wkup_irq	= exynos5250_wkup_irq,
>  	.wake_disable_mask = (0x7F << 7) | (0x1F << 1),
> -	.release_ret_regs = exynos5420_release_ret_regs,
>  	.pm_resume_prepare = exynos5420_prepare_pm_resume,
>  	.pm_resume	= exynos5420_pm_resume,
>  	.pm_suspend	= exynos5420_pm_suspend,
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
> index 12f7d1eb65bc..55c1104a1ccf 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> @@ -24,11 +24,15 @@
>  #include <linux/irqdomain.h>
>  #include <linux/irq.h>
>  #include <linux/irqchip/chained_irq.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/of_irq.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/regmap.h>
>  #include <linux/err.h>
> +#include <linux/soc/samsung/exynos-regs-pmu.h>
> +
>  
>  #include "pinctrl-samsung.h"
>  #include "pinctrl-exynos.h"
> @@ -633,6 +637,46 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  			exynos_pinctrl_resume_bank(drvdata, bank);
>  }
>  
> +static atomic_t exynos_retention_refcnt;
> +static struct regmap *pmu_regs;
> +
> +static int exynos_retention_init(struct samsung_pinctrl_drv_data *drvdata)
> +{
> +	struct device *dev = drvdata->dev;
> +
> +	pmu_regs = syscon_regmap_lookup_by_phandle(dev->of_node,
> +						   "samsung,pmu-syscon");
> +	if (IS_ERR(pmu_regs)) {
> +		dev_err(dev, "failed to lookup PMU regmap, no support for pad retention\n");
> +		return PTR_ERR(pmu_regs);
> +	}
> +	return 0;
> +}
> +
> +static void exynos_retention_on(struct samsung_pinctrl_drv_data *drvdata)
> +{
> +	atomic_inc(&exynos_retention_refcnt);

As this does not imply memory barriers, so maybe you need smp_mb__after_atomic()?

You didn't describe the need behind this barrier. What do you want to
protect? Do you expect suspend (or resume) happening multiple times for
one device? Or maybe you expect even mixing of these by different CPUs?

> +}
> +
> +static void exynos_retention_off(struct samsung_pinctrl_drv_data *drvdata)
> +{
> +	int i;
> +
> +	if (!atomic_dec_and_test(&exynos_retention_refcnt) || IS_ERR(pmu_regs))
> +		return;
> +
> +	for (i = 0; i < drvdata->nr_retention_regs; i++)
> +		regmap_write(pmu_regs, drvdata->retention_regs[i],
> +			     EXYNOS_WAKEUP_FROM_LOWPWR);
> +}
> +
> +static void exynos_retention_audio_off(struct samsung_pinctrl_drv_data *drvdata)
> +{
> +	if (!IS_ERR(pmu_regs))
> +		regmap_write(pmu_regs, S5P_PAD_RET_MAUDIO_OPTION,
> +			     EXYNOS_WAKEUP_FROM_LOWPWR);
> +}
> +
>  /* pin banks of s5pv210 pin-controller */
>  static const struct samsung_pin_bank_data s5pv210_pin_bank[] __initconst = {
>  	EXYNOS_PIN_BANK_EINTG(8, 0x000, "gpa0", 0x00),
> @@ -714,6 +758,18 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  	EXYNOS_PIN_BANK_EINTW(8, 0xc60, "gpx3", 0x0c),
>  };
>  
> +static const u32 exynos3250_retention_regs[] = {
> +	S5P_PAD_RET_MAUDIO_OPTION,
> +	S5P_PAD_RET_GPIO_OPTION,
> +	S5P_PAD_RET_UART_OPTION,
> +	S5P_PAD_RET_MMCA_OPTION,
> +	S5P_PAD_RET_MMCB_OPTION,
> +	S5P_PAD_RET_EBIA_OPTION,
> +	S5P_PAD_RET_EBIB_OPTION,
> +	S5P_PAD_RET_MMC2_OPTION,
> +	S5P_PAD_RET_SPI_OPTION,
> +};
> +
>  /*
>   * Samsung pinctrl driver data for Exynos3250 SoC. Exynos3250 SoC includes
>   * two gpio/pin-mux/pinconfig controllers.
> @@ -726,6 +782,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  		.eint_gpio_init = exynos_eint_gpio_init,
>  		.suspend	= exynos_pinctrl_suspend,
>  		.resume		= exynos_pinctrl_resume,
> +		.retention_regs = exynos3250_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos3250_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,
>  	}, {
>  		/* pin-controller instance 1 data */
>  		.pin_banks	= exynos3250_pin_banks1,
> @@ -734,6 +795,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  		.eint_wkup_init = exynos_eint_wkup_init,
>  		.suspend	= exynos_pinctrl_suspend,
>  		.resume		= exynos_pinctrl_resume,
> +		.retention_regs = exynos3250_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos3250_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,
>  	},
>  };
>  
> @@ -786,6 +852,15 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  	EXYNOS_PIN_BANK_EINTN(7, 0x000, "gpz"),
>  };
>  
> +static const u32 exynos4_retention_regs[] = {
> +	S5P_PAD_RET_GPIO_OPTION,
> +	S5P_PAD_RET_UART_OPTION,
> +	S5P_PAD_RET_MMCA_OPTION,
> +	S5P_PAD_RET_MMCB_OPTION,
> +	S5P_PAD_RET_EBIA_OPTION,
> +	S5P_PAD_RET_EBIB_OPTION,
> +};
> +
>  /*
>   * Samsung pinctrl driver data for Exynos4210 SoC. Exynos4210 SoC includes
>   * three gpio/pin-mux/pinconfig controllers.
> @@ -798,6 +873,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  		.eint_gpio_init = exynos_eint_gpio_init,
>  		.suspend	= exynos_pinctrl_suspend,
>  		.resume		= exynos_pinctrl_resume,
> +		.retention_regs = exynos4_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,
>  	}, {
>  		/* pin-controller instance 1 data */
>  		.pin_banks	= exynos4210_pin_banks1,
> @@ -806,10 +886,17 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  		.eint_wkup_init = exynos_eint_wkup_init,
>  		.suspend	= exynos_pinctrl_suspend,
>  		.resume		= exynos_pinctrl_resume,
> +		.retention_regs = exynos4_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,
>  	}, {
>  		/* pin-controller instance 2 data */
>  		.pin_banks	= exynos4210_pin_banks2,
>  		.nr_banks	= ARRAY_SIZE(exynos4210_pin_banks2),
> +		.retention_init = exynos_retention_init,
> +		.retention_off   = exynos_retention_audio_off,
>  	},
>  };
>  
> @@ -883,6 +970,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  		.eint_gpio_init = exynos_eint_gpio_init,
>  		.suspend	= exynos_pinctrl_suspend,
>  		.resume		= exynos_pinctrl_resume,
> +		.retention_regs = exynos4_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,
>  	}, {
>  		/* pin-controller instance 1 data */
>  		.pin_banks	= exynos4x12_pin_banks1,
> @@ -891,6 +983,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  		.eint_wkup_init = exynos_eint_wkup_init,
>  		.suspend	= exynos_pinctrl_suspend,
>  		.resume		= exynos_pinctrl_resume,
> +		.retention_regs = exynos4_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,
>  	}, {
>  		/* pin-controller instance 2 data */
>  		.pin_banks	= exynos4x12_pin_banks2,
> @@ -898,6 +995,8 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  		.eint_gpio_init = exynos_eint_gpio_init,
>  		.suspend	= exynos_pinctrl_suspend,
>  		.resume		= exynos_pinctrl_resume,
> +		.retention_init = exynos_retention_init,
> +		.retention_off   = exynos_retention_audio_off,
>  	}, {
>  		/* pin-controller instance 3 data */
>  		.pin_banks	= exynos4x12_pin_banks3,
> @@ -1052,6 +1151,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  		.eint_wkup_init = exynos_eint_wkup_init,
>  		.suspend	= exynos_pinctrl_suspend,
>  		.resume		= exynos_pinctrl_resume,
> +		.retention_regs = exynos4_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,
>  	}, {
>  		/* pin-controller instance 1 data */
>  		.pin_banks	= exynos5250_pin_banks1,
> @@ -1059,6 +1163,11 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  		.eint_gpio_init = exynos_eint_gpio_init,
>  		.suspend	= exynos_pinctrl_suspend,
>  		.resume		= exynos_pinctrl_resume,
> +		.retention_regs = exynos4_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos4_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,
>  	}, {
>  		/* pin-controller instance 2 data */
>  		.pin_banks	= exynos5250_pin_banks2,
> @@ -1073,6 +1182,8 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  		.eint_gpio_init = exynos_eint_gpio_init,
>  		.suspend	= exynos_pinctrl_suspend,
>  		.resume		= exynos_pinctrl_resume,
> +		.retention_init = exynos_retention_init,
> +		.retention_off   = exynos_retention_audio_off,
>  	},
>  };
>  
> @@ -1299,6 +1410,21 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  	EXYNOS_PIN_BANK_EINTG(7, 0x000, "gpz", 0x00),
>  };
>  
> +static const u32 exynos5420_retention_regs[] = {
> +	EXYNOS_PAD_RET_DRAM_OPTION,
> +	EXYNOS_PAD_RET_JTAG_OPTION,
> +	EXYNOS5420_PAD_RET_GPIO_OPTION,
> +	EXYNOS5420_PAD_RET_UART_OPTION,
> +	EXYNOS5420_PAD_RET_MMCA_OPTION,
> +	EXYNOS5420_PAD_RET_MMCB_OPTION,
> +	EXYNOS5420_PAD_RET_MMCC_OPTION,
> +	EXYNOS5420_PAD_RET_HSI_OPTION,
> +	EXYNOS_PAD_RET_EBIA_OPTION,
> +	EXYNOS_PAD_RET_EBIB_OPTION,
> +	EXYNOS5420_PAD_RET_SPI_OPTION,
> +	EXYNOS5420_PAD_RET_DRAM_COREBLK_OPTION,
> +};
> +
>  /*
>   * Samsung pinctrl driver data for Exynos5420 SoC. Exynos5420 SoC includes
>   * four gpio/pin-mux/pinconfig controllers.
> @@ -1310,26 +1436,48 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks0),
>  		.eint_gpio_init = exynos_eint_gpio_init,
>  		.eint_wkup_init = exynos_eint_wkup_init,
> +		.retention_regs = exynos5420_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos5420_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,

The indent here:                ^
looks weird. Did you indent it with a tab? Similar in the code below.

>  	}, {
>  		/* pin-controller instance 1 data */
>  		.pin_banks	= exynos5420_pin_banks1,
>  		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks1),
>  		.eint_gpio_init = exynos_eint_gpio_init,
> +		.retention_regs = exynos5420_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos5420_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,
>  	}, {
>  		/* pin-controller instance 2 data */
>  		.pin_banks	= exynos5420_pin_banks2,
>  		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks2),
>  		.eint_gpio_init = exynos_eint_gpio_init,
> +		.retention_regs = exynos5420_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos5420_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,
>  	}, {
>  		/* pin-controller instance 3 data */
>  		.pin_banks	= exynos5420_pin_banks3,
>  		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks3),
>  		.eint_gpio_init = exynos_eint_gpio_init,
> +		.retention_regs = exynos5420_retention_regs,
> +		.nr_retention_regs = ARRAY_SIZE(exynos5420_retention_regs),
> +		.retention_init = exynos_retention_init,
> +		.retention_on   = exynos_retention_on,
> +		.retention_off   = exynos_retention_off,
>  	}, {
>  		/* pin-controller instance 4 data */
>  		.pin_banks	= exynos5420_pin_banks4,
>  		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks4),
>  		.eint_gpio_init = exynos_eint_gpio_init,
> +		.retention_init = exynos_retention_init,
> +		.retention_off  = exynos_retention_audio_off,
>  	},
>  };
>  
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index a6c2ea74e0f3..cb425e6837f9 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -1011,6 +1011,11 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
>  			return ERR_PTR(-EIO);
>  	}
>  
> +	d->retention_regs = ctrl->retention_regs;
> +	d->nr_retention_regs = ctrl->nr_retention_regs;
> +	d->retention_on = ctrl->retention_on;
> +	d->retention_off = ctrl->retention_off;
> +
>  	bank = d->pin_banks;
>  	bdata = ctrl->pin_banks;
>  	for (i = 0; i < ctrl->nr_banks; ++i, ++bdata, ++bank) {
> @@ -1087,6 +1092,8 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
>  		ctrl->eint_gpio_init(drvdata);
>  	if (ctrl->eint_wkup_init)
>  		ctrl->eint_wkup_init(drvdata);
> +	if (ctrl->retention_init)
> +		ctrl->retention_init(drvdata);
>  
>  	platform_set_drvdata(pdev, drvdata);
>  
> @@ -1139,15 +1146,15 @@ static void samsung_pinctrl_suspend_dev(
>  
>  	if (drvdata->suspend)
>  		drvdata->suspend(drvdata);
> +	if (drvdata->retention_on)
> +		drvdata->retention_on(drvdata);
> +

Empty line is not needed.

Best regards,
Krzysztof

Re: [PATCH 5/9] pinctrl: samsung: Move retention control from mach-exynos to the pinctrl driver

[Marek Szyprowski]

Hi Krzysztof,


On 2016-12-25 14:42, Krzysztof Kozlowski wrote:
> On Fri, Dec 23, 2016 at 01:24:45PM +0100, Marek Szyprowski wrote:
>> Pad retention control after suspend/resume cycle should be done from pin
>> controller driver instead of PMU (power management unit) driver to avoid
>> possible ordering and logical dependencies. Till now it worked fine only
>> because PMU driver registered its sys_ops after pin controller.
>>
>> This patch moves pad retention control from PMU driver to Exynos pin
>> controller driver. This is a preparation for adding new features to Exynos
>> pin controller driver, like runtime power management and suspending
>> individual pin controllers, which might be a part of some power domain.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   .../bindings/pinctrl/samsung-pinctrl.txt           |   4 +
>>   arch/arm/mach-exynos/suspend.c                     |  64 ---------
>>   drivers/pinctrl/samsung/pinctrl-exynos.c           | 148 +++++++++++++++++++++
>>   drivers/pinctrl/samsung/pinctrl-samsung.c          |  16 ++-
>>   drivers/pinctrl/samsung/pinctrl-samsung.h          |  13 ++
>>   5 files changed, 178 insertions(+), 67 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> index 1baf19eecabf..b7bd2e12a269 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> @@ -43,6 +43,10 @@ Required Properties:
>>   		};
>>   	};
>>   
>> +- samsung,pmu-syscon: Phandle to the PMU system controller, to let driver
>> +  to control pad retention after system suspend/resume cycle (only for Exynos
>> +  SoC series).
>> +
>>   - Pin banks as child nodes: Pin banks of the controller are represented by child
>>     nodes of the controller node. Bank name is taken from name of the node. Each
>>     bank node must contain following properties:
>> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c

 > [...]

>> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
>> index 12f7d1eb65bc..55c1104a1ccf 100644
>> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
>> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
>> @@ -24,11 +24,15 @@
>>   #include <linux/irqdomain.h>
>>   #include <linux/irq.h>
>>   #include <linux/irqchip/chained_irq.h>
>> +#include <linux/mfd/syscon.h>
>>   #include <linux/of_irq.h>
>>   #include <linux/io.h>
>>   #include <linux/slab.h>
>>   #include <linux/spinlock.h>
>> +#include <linux/regmap.h>
>>   #include <linux/err.h>
>> +#include <linux/soc/samsung/exynos-regs-pmu.h>
>> +
>>   
>>   #include "pinctrl-samsung.h"
>>   #include "pinctrl-exynos.h"
>> @@ -633,6 +637,46 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>>   			exynos_pinctrl_resume_bank(drvdata, bank);
>>   }
>>   
>> +static atomic_t exynos_retention_refcnt;
>> +static struct regmap *pmu_regs;
>> +
>> +static int exynos_retention_init(struct samsung_pinctrl_drv_data *drvdata)
>> +{
>> +	struct device *dev = drvdata->dev;
>> +
>> +	pmu_regs = syscon_regmap_lookup_by_phandle(dev->of_node,
>> +						   "samsung,pmu-syscon");
>> +	if (IS_ERR(pmu_regs)) {
>> +		dev_err(dev, "failed to lookup PMU regmap, no support for pad retention\n");
>> +		return PTR_ERR(pmu_regs);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static void exynos_retention_on(struct samsung_pinctrl_drv_data *drvdata)
>> +{
>> +	atomic_inc(&exynos_retention_refcnt);
> As this does not imply memory barriers, so maybe you need smp_mb__after_atomic()?

exynos_retention_refcnt will be read/tested after suspend/resume cycle, 
so I doubt
that any kind of barrier is needed here.

>
> You didn't describe the need behind this barrier. What do you want to
> protect? Do you expect suspend (or resume) happening multiple times for
> one device? Or maybe you expect even mixing of these by different CPUs?

There are more than one instance of pinctrl devices on Exynos4+ SoCs and 
they have
common retention regs. All those controllers might be resumed in 
parallel, so this
atomic counting is needed to ensure that retention will be disabled when 
the last
instance is being resumed.

>> +}
>> +
>> +static void exynos_retention_off(struct samsung_pinctrl_drv_data *drvdata)
>> +{
>> +	int i;
>> +
>> +	if (!atomic_dec_and_test(&exynos_retention_refcnt) || IS_ERR(pmu_regs))
>> +		return;
>> +
>> +	for (i = 0; i < drvdata->nr_retention_regs; i++)
>> +		regmap_write(pmu_regs, drvdata->retention_regs[i],
>> +			     EXYNOS_WAKEUP_FROM_LOWPWR);
>> +}
>> +
>> +static void exynos_retention_audio_off(struct samsung_pinctrl_drv_data *drvdata)
>> +{
>> +	if (!IS_ERR(pmu_regs))
>> +		regmap_write(pmu_regs, S5P_PAD_RET_MAUDIO_OPTION,
>> +			     EXYNOS_WAKEUP_FROM_LOWPWR);
>> +}
>> +

 > [...]

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 5/9] pinctrl: samsung: Move retention control from mach-exynos to the pinctrl driver

[Tomasz Figa]

Hi,

2016-12-23 21:24 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> Pad retention control after suspend/resume cycle should be done from pin
> controller driver instead of PMU (power management unit) driver to avoid
> possible ordering and logical dependencies. Till now it worked fine only
> because PMU driver registered its sys_ops after pin controller.
>
> This patch moves pad retention control from PMU driver to Exynos pin
> controller driver. This is a preparation for adding new features to Exynos
> pin controller driver, like runtime power management and suspending
> individual pin controllers, which might be a part of some power domain.
>

It looks like this change will essentially break the compatibility
with DTBs that don't have the pmu syscon specified in pin controller
nodes.

On the other hand, moving this code to where it actually belongs
really makes sense, so maybe we could just avoid the need of having
this property, by looking up the PMU manually, by hardcoded string or
so, if the proper property is not present?

[...]

> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> index 1baf19eecabf..b7bd2e12a269 100644
> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> @@ -43,6 +43,10 @@ Required Properties:
>                 };
>         };
>
> +- samsung,pmu-syscon: Phandle to the PMU system controller, to let driver
> +  to control pad retention after system suspend/resume cycle (only for Exynos
> +  SoC series).
> +

Ah, here it is. I think adding relevant binding documentation at the
beginning of the series would make it much easier for reviewers to
understand the change.

>  - Pin banks as child nodes: Pin banks of the controller are represented by child
>    nodes of the controller node. Bank name is taken from name of the node. Each
>    bank node must contain following properties:

[...]

> +static void exynos_retention_audio_off(struct samsung_pinctrl_drv_data *drvdata)
> +{
> +       if (!IS_ERR(pmu_regs))

nit: Negated conditions make the code more difficult to read. Also it
would be consistent with exynos_retention_off() to just bail out if
(IS_ERR(pmu_regs)).

> +               regmap_write(pmu_regs, S5P_PAD_RET_MAUDIO_OPTION,
> +                            EXYNOS_WAKEUP_FROM_LOWPWR);
> +}

[...]

> @@ -1139,15 +1146,15 @@ static void samsung_pinctrl_suspend_dev(
>
>         if (drvdata->suspend)
>                 drvdata->suspend(drvdata);
> +       if (drvdata->retention_on)
> +               drvdata->retention_on(drvdata);
> +

nit: Unnecessary blank line.

Best regards,
Tomasz

Re: [PATCH 5/9] pinctrl: samsung: Move retention control from mach-exynos to the pinctrl driver

[Marek Szyprowski]

Hi Tomasz,


On 2016-12-26 06:55, Tomasz Figa wrote:
> 2016-12-23 21:24 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
>> Pad retention control after suspend/resume cycle should be done from pin
>> controller driver instead of PMU (power management unit) driver to avoid
>> possible ordering and logical dependencies. Till now it worked fine only
>> because PMU driver registered its sys_ops after pin controller.
>>
>> This patch moves pad retention control from PMU driver to Exynos pin
>> controller driver. This is a preparation for adding new features to Exynos
>> pin controller driver, like runtime power management and suspending
>> individual pin controllers, which might be a part of some power domain.
>>
> It looks like this change will essentially break the compatibility
> with DTBs that don't have the pmu syscon specified in pin controller
> nodes.
>
> On the other hand, moving this code to where it actually belongs
> really makes sense, so maybe we could just avoid the need of having
> this property, by looking up the PMU manually, by hardcoded string or
> so, if the proper property is not present?

Well, once again the topic of mythical device tree compatibility appearch.

There are imho following possibilities:

1. https://patchwork.kernel.org/patch/9477963/ ("Explicitly mark Samsung
    Exynos SoC bindings as unstable"), simply apply this approach and ignore
    users, who don't update their device tree blobs (are there any??).

2. Switch to syscon_regmap_lookup_by_compatible() lookup and hardcode PMU
    compatible id for all Exynos SoCs in the pin control driver. Then maybe,
    while unifying the code, switch other Exynos drivers to this approach
    and remove PMU phandles from device tree to make the code a bit more
    consistent across drivers and easier to understand.

3. Mixed approach, which combines drawbacks of both approaches. Additional
    dead code for handling mythical compatibility and harder to understand
    relations between the drivers.


> [...]
>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> index 1baf19eecabf..b7bd2e12a269 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> @@ -43,6 +43,10 @@ Required Properties:
>>                  };
>>          };
>>
>> +- samsung,pmu-syscon: Phandle to the PMU system controller, to let driver
>> +  to control pad retention after system suspend/resume cycle (only for Exynos
>> +  SoC series).
>> +
> Ah, here it is. I think adding relevant binding documentation at the
> beginning of the series would make it much easier for reviewers to
> understand the change.

I already pointed that patches are ordered to make the changes 
bisectable, what
usually means that new properties are added first, before being required 
by the
drivers.

>
>>   - Pin banks as child nodes: Pin banks of the controller are represented by child
>>     nodes of the controller node. Bank name is taken from name of the node. Each
>>     bank node must contain following properties:
> [...]
>
>> +static void exynos_retention_audio_off(struct samsung_pinctrl_drv_data *drvdata)
>> +{
>> +       if (!IS_ERR(pmu_regs))
> nit: Negated conditions make the code more difficult to read. Also it
> would be consistent with exynos_retention_off() to just bail out if
> (IS_ERR(pmu_regs)).
>
>> +               regmap_write(pmu_regs, S5P_PAD_RET_MAUDIO_OPTION,
>> +                            EXYNOS_WAKEUP_FROM_LOWPWR);
>> +}
> [...]
>
>> @@ -1139,15 +1146,15 @@ static void samsung_pinctrl_suspend_dev(
>>
>>          if (drvdata->suspend)
>>                  drvdata->suspend(drvdata);
>> +       if (drvdata->retention_on)
>> +               drvdata->retention_on(drvdata);
>> +
> nit: Unnecessary blank line.
>
> Best regards,
> Tomasz
>
>
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 5/9] pinctrl: samsung: Move retention control from mach-exynos to the pinctrl driver

[Krzysztof Kozlowski]

On Tue, Dec 27, 2016 at 11:12:26AM +0100, Marek Szyprowski wrote:
> Hi Tomasz,
> 
> 
> On 2016-12-26 06:55, Tomasz Figa wrote:
> > 2016-12-23 21:24 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> > > Pad retention control after suspend/resume cycle should be done from pin
> > > controller driver instead of PMU (power management unit) driver to avoid
> > > possible ordering and logical dependencies. Till now it worked fine only
> > > because PMU driver registered its sys_ops after pin controller.
> > > 
> > > This patch moves pad retention control from PMU driver to Exynos pin
> > > controller driver. This is a preparation for adding new features to Exynos
> > > pin controller driver, like runtime power management and suspending
> > > individual pin controllers, which might be a part of some power domain.
> > > 
> > It looks like this change will essentially break the compatibility
> > with DTBs that don't have the pmu syscon specified in pin controller
> > nodes.
> > 
> > On the other hand, moving this code to where it actually belongs
> > really makes sense, so maybe we could just avoid the need of having
> > this property, by looking up the PMU manually, by hardcoded string or
> > so, if the proper property is not present?
> 
> Well, once again the topic of mythical device tree compatibility appearch.
> 
> There are imho following possibilities:
> 
> 1. https://patchwork.kernel.org/patch/9477963/ ("Explicitly mark Samsung
>    Exynos SoC bindings as unstable"), simply apply this approach and ignore
>    users, who don't update their device tree blobs (are there any??).

That is not how it works. Assuming that there was this "mythical
compatibility" then you would have to wait a few moments between marking
something "to be broken" and actually breaking it. In other words, if
you wanted to silence the "breaking compatibility" folks, then the patch
above should have been sent a while ago.

> 2. Switch to syscon_regmap_lookup_by_compatible() lookup and hardcode PMU
>    compatible id for all Exynos SoCs in the pin control driver. Then maybe,
>    while unifying the code, switch other Exynos drivers to this approach
>    and remove PMU phandles from device tree to make the code a bit more
>    consistent across drivers and easier to understand.
> 
> 3. Mixed approach, which combines drawbacks of both approaches. Additional
>    dead code for handling mythical compatibility and harder to understand
>    relations between the drivers.
> 
> 
> > [...]
> > 
> > > diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > index 1baf19eecabf..b7bd2e12a269 100644
> > > --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > @@ -43,6 +43,10 @@ Required Properties:
> > >                  };
> > >          };
> > > 
> > > +- samsung,pmu-syscon: Phandle to the PMU system controller, to let driver
> > > +  to control pad retention after system suspend/resume cycle (only for Exynos
> > > +  SoC series).
> > > +
> > Ah, here it is. I think adding relevant binding documentation at the
> > beginning of the series would make it much easier for reviewers to
> > understand the change.
> 
> I already pointed that patches are ordered to make the changes bisectable,
> what
> usually means that new properties are added first, before being required by
> the
> drivers.

I saw the explanation for this order and I don't have problems with it.
However you can always split to separate patch the documentation for
bindings. It won't break bisectability.

Best regards,
Krzysztof

Re: [PATCH 5/9] pinctrl: samsung: Move retention control from mach-exynos to the pinctrl driver

[Linus Walleij]

On Fri, Dec 23, 2016 at 1:24 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:

> Pad retention control after suspend/resume cycle should be done from pin
> controller driver instead of PMU (power management unit) driver to avoid
> possible ordering and logical dependencies. Till now it worked fine only
> because PMU driver registered its sys_ops after pin controller.
>
> This patch moves pad retention control from PMU driver to Exynos pin
> controller driver. This is a preparation for adding new features to Exynos
> pin controller driver, like runtime power management and suspending
> individual pin controllers, which might be a part of some power domain.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Clear and elegant.

I will need an ACK from some ARM SoC person on this so they are
aware that we rip code out of mach-* to pinctrl.

Yours,
Linus Walleij

[PATCH 6/9] pinctrl: samsung: Replace syscore ops with standard platform device pm_ops

[Marek Szyprowski]

Once the dependency on PMU driver (for pad retention control) has been
removed, there is no reason to use syscore_ops based suspend/resume.
This patch replaces it with standard platform device pm_ops based solution.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 72 ++++++-------------------------
 1 file changed, 13 insertions(+), 59 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index cb425e6837f9..a7b7d75373f2 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -29,7 +29,6 @@
 #include <linux/irqdomain.h>
 #include <linux/of_device.h>
 #include <linux/spinlock.h>
-#include <linux/syscore_ops.h>
 
 #include "../core.h"
 #include "pinctrl-samsung.h"
@@ -49,9 +48,6 @@
 	{ "samsung,pin-val", PINCFG_TYPE_DAT },
 };
 
-/* Global list of devices (struct samsung_pinctrl_drv_data) */
-static LIST_HEAD(drvdata_list);
-
 static unsigned int pin_base;
 
 static int samsung_get_group_count(struct pinctrl_dev *pctldev)
@@ -1097,22 +1093,18 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, drvdata);
 
-	/* Add to the global list */
-	list_add_tail(&drvdata->node, &drvdata_list);
-
 	return 0;
 }
 
 #ifdef CONFIG_PM
-
 /**
- * samsung_pinctrl_suspend_dev - save pinctrl state for suspend for a device
+ * samsung_pinctrl_suspend - save pinctrl state for suspend
  *
  * Save data for all banks handled by this device.
  */
-static void samsung_pinctrl_suspend_dev(
-	struct samsung_pinctrl_drv_data *drvdata)
+static int samsung_pinctrl_suspend(struct device *dev)
 {
+	struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
 	int i;
 
 	for (i = 0; i < drvdata->nr_banks; i++) {
@@ -1149,15 +1141,17 @@ static void samsung_pinctrl_suspend_dev(
 	if (drvdata->retention_on)
 		drvdata->retention_on(drvdata);
 
+	return 0;
 }
 
 /**
- * samsung_pinctrl_resume_dev - restore pinctrl state from suspend for a device
+ * samsung_pinctrl_resume - restore pinctrl state from suspend
  *
  * Restore one of the banks that was saved during suspend.
  */
-static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
+static int samsung_pinctrl_resume(struct device *dev)
 {
+	struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
 	int i;
 
 	if (drvdata->resume)
@@ -1196,48 +1190,10 @@ static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 
 	if (drvdata->retention_off)
 		drvdata->retention_off(drvdata);
-}
-
-/**
- * samsung_pinctrl_suspend - save pinctrl state for suspend
- *
- * Save data for all banks across all devices.
- */
-static int samsung_pinctrl_suspend(void)
-{
-	struct samsung_pinctrl_drv_data *drvdata;
-
-	list_for_each_entry(drvdata, &drvdata_list, node) {
-		samsung_pinctrl_suspend_dev(drvdata);
-	}
-
 	return 0;
 }
-
-/**
- * samsung_pinctrl_resume - restore pinctrl state for suspend
- *
- * Restore data for all banks across all devices.
- */
-static void samsung_pinctrl_resume(void)
-{
-	struct samsung_pinctrl_drv_data *drvdata;
-
-	list_for_each_entry_reverse(drvdata, &drvdata_list, node) {
-		samsung_pinctrl_resume_dev(drvdata);
-	}
-}
-
-#else
-#define samsung_pinctrl_suspend		NULL
-#define samsung_pinctrl_resume		NULL
 #endif
 
-static struct syscore_ops samsung_pinctrl_syscore_ops = {
-	.suspend	= samsung_pinctrl_suspend,
-	.resume		= samsung_pinctrl_resume,
-};
-
 static const struct of_device_id samsung_pinctrl_dt_match[] = {
 #ifdef CONFIG_PINCTRL_EXYNOS
 	{ .compatible = "samsung,exynos3250-pinctrl",
@@ -1281,25 +1237,23 @@ static void samsung_pinctrl_resume(void)
 };
 MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);
 
+static const struct dev_pm_ops samsung_pinctrl_pm_ops = {
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(samsung_pinctrl_suspend,
+				     samsung_pinctrl_resume)
+};
+
 static struct platform_driver samsung_pinctrl_driver = {
 	.probe		= samsung_pinctrl_probe,
 	.driver = {
 		.name	= "samsung-pinctrl",
 		.of_match_table = samsung_pinctrl_dt_match,
 		.suppress_bind_attrs = true,
+		.pm = &samsung_pinctrl_pm_ops,
 	},
 };
 
 static int __init samsung_pinctrl_drv_register(void)
 {
-	/*
-	 * Register syscore ops for save/restore of registers across suspend.
-	 * It's important to ensure that this driver is running at an earlier
-	 * initcall level than any arch-specific init calls that install syscore
-	 * ops that turn off pad retention (like exynos_pm_resume).
-	 */
-	register_syscore_ops(&samsung_pinctrl_syscore_ops);
-
 	return platform_driver_register(&samsung_pinctrl_driver);
 }
 postcore_initcall(samsung_pinctrl_drv_register);
-- 
1.9.1

Re: [PATCH 6/9] pinctrl: samsung: Replace syscore ops with standard platform device pm_ops

[Krzysztof Kozlowski]

On Fri, Dec 23, 2016 at 01:24:46PM +0100, Marek Szyprowski wrote:
> Once the dependency on PMU driver (for pad retention control) has been
> removed, there is no reason to use syscore_ops based suspend/resume.
> This patch replaces it with standard platform device pm_ops based solution.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 72 ++++++-------------------------
>  1 file changed, 13 insertions(+), 59 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 6/9] pinctrl: samsung: Replace syscore ops with standard platform device pm_ops

[Tomasz Figa]

Hi,

2016-12-23 21:24 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> Once the dependency on PMU driver (for pad retention control) has been
> removed, there is no reason to use syscore_ops based suspend/resume.

Does it also hold true for other platforms using this driver? I.e.
s3c24xx, s3c64xx and s5pv210?

Best regards,
Tomasz

Re: [PATCH 6/9] pinctrl: samsung: Replace syscore ops with standard platform device pm_ops

[Marek Szyprowski]

Hi Krzysztof,


On 2016-12-26 06:57, Tomasz Figa wrote:
> 2016-12-23 21:24 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
>> Once the dependency on PMU driver (for pad retention control) has been
>> removed, there is no reason to use syscore_ops based suspend/resume.
> Does it also hold true for other platforms using this driver? I.e.
> s3c24xx, s3c64xx and s5pv210?

I've checked and I didn't find any code related to retention control in
s3c24xx and 23c64xx, but you are right that I need to add support for
s5pv210 (retention is controlled via S5P_OTHERS register). Thanks for
pointing this.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH 7/9] pinctrl: samsung: Add property to mark pad state as suitable for power down

[Marek Szyprowski]

Add support for special property "samsung,off-state", which indicates a special
state suitable for device's "sleep" state. Its pin values/properties should
match the configuration in power down mode. It indicates that pin controller
can notify runtime power management subsystem, that it is ready for runtime
suspend if its all pins are configured for such state. This in turn might
allow to turn respective power domain off to reduce power consumption.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
 drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
 drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
 3 files changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
index b7bd2e12a269..354eea0e7798 100644
--- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
@@ -105,6 +105,7 @@ Required Properties:
   - samsung,pin-drv: Drive strength configuration.
   - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
   - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
+  - samsung,off-state: Mark this configuration as suitable for bank power off.
 
   The values specified by these config properties should be derived from the
   hardware manual and these values are programmed as-is into the pin
@@ -113,6 +114,13 @@ Required Properties:
   Note: A child should include atleast a pin function selection property or
   pin configuration property (one or more) or both.
 
+  Note: Special property "samsung,off-state" indicates that this state can
+  be used for device's "sleep" pins state. Its pin values/properties should
+  match the configuration in power down mode. It indicates that pin control
+  can notify runtime power management subsystem, that it is ready for runtime
+  suspend if its all pins are configured for such state. This in turn might
+  allow to turn respective power domain off to reduce power consumption.
+
   The client nodes that require a particular pin function selection and/or
   pin configuration should use the bindings listed in the "pinctrl-bindings.txt"
   file.
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index a7b7d75373f2..301169d2b6e1 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -692,6 +692,10 @@ static int samsung_pinctrl_create_function(struct device *dev,
 	}
 
 	func->name = func_np->full_name;
+	if (of_property_read_bool(func_np, "samsung,off-state"))
+		func->rpm_active = false;
+	else
+		func->rpm_active = true;
 
 	func->groups = devm_kzalloc(dev, npins * sizeof(char *), GFP_KERNEL);
 	if (!func->groups)
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 32b949e2a89b..edeafa00abd3 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -280,6 +280,7 @@ struct samsung_pmx_func {
 	const char		**groups;
 	u8			num_groups;
 	u32			val;
+	bool			rpm_active;
 };
 
 /* list of all exported SoC specific data */
-- 
1.9.1

Re: [PATCH 7/9] pinctrl: samsung: Add property to mark pad state as suitable for power down

[Krzysztof Kozlowski]

On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
> Add support for special property "samsung,off-state", which indicates a special
> state suitable for device's "sleep" state. Its pin values/properties should
> match the configuration in power down mode. It indicates that pin controller
> can notify runtime power management subsystem, that it is ready for runtime
> suspend if its all pins are configured for such state. This in turn might
> allow to turn respective power domain off to reduce power consumption.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
>  drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
>  drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
>  3 files changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> index b7bd2e12a269..354eea0e7798 100644
> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> @@ -105,6 +105,7 @@ Required Properties:
>    - samsung,pin-drv: Drive strength configuration.
>    - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
>    - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
> +  - samsung,off-state: Mark this configuration as suitable for bank power off.
>  
>    The values specified by these config properties should be derived from the
>    hardware manual and these values are programmed as-is into the pin
> @@ -113,6 +114,13 @@ Required Properties:
>    Note: A child should include atleast a pin function selection property or
>    pin configuration property (one or more) or both.
>  
> +  Note: Special property "samsung,off-state" indicates that this state can
> +  be used for device's "sleep" pins state. Its pin values/properties should
> +  match the configuration in power down mode.

Why power down values cannot be used for sleep state? Why you need
separate pin control state? If pins values should match power down
configuration, then they could just be added to default state, couldn't
they?

In the patch 2/9, existing configuration:
716         i2s0_bus: i2s0-bus {
(...)
719                 samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
720                 samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
721                 samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
722         };

additional configuration:
+       i2s0_bus_slp: i2s0-bus-slp {
+               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
+               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
+               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
+               samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
+               samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
+               samsung,off-state;
+       };

> It indicates that pin control
> +  can notify runtime power management subsystem, that it is ready for runtime
> +  suspend if its all pins are configured for such state. This in turn might
> +  allow to turn respective power domain off to reduce power consumption.

What do you mean by "notifying RPM subsystem"? Either this is
description of hardware in certain mode (sleep state) or this is not
device tree property.

Best regards,
Krzysztof

> +
>    The client nodes that require a particular pin function selection and/or
>    pin configuration should use the bindings listed in the "pinctrl-bindings.txt"
>    file.
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index a7b7d75373f2..301169d2b6e1 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -692,6 +692,10 @@ static int samsung_pinctrl_create_function(struct device *dev,
>  	}
>  
>  	func->name = func_np->full_name;
> +	if (of_property_read_bool(func_np, "samsung,off-state"))
> +		func->rpm_active = false;
> +	else
> +		func->rpm_active = true;
>  
>  	func->groups = devm_kzalloc(dev, npins * sizeof(char *), GFP_KERNEL);
>  	if (!func->groups)
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
> index 32b949e2a89b..edeafa00abd3 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> @@ -280,6 +280,7 @@ struct samsung_pmx_func {
>  	const char		**groups;
>  	u8			num_groups;
>  	u32			val;
> +	bool			rpm_active;
>  };
>  
>  /* list of all exported SoC specific data */
> -- 
> 1.9.1
> 

Re: [PATCH 7/9] pinctrl: samsung: Add property to mark pad state as suitable for power down

[Tomasz Figa]

2016-12-26 4:19 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
> On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
>> Add support for special property "samsung,off-state", which indicates a special
>> state suitable for device's "sleep" state. Its pin values/properties should
>> match the configuration in power down mode. It indicates that pin controller
>> can notify runtime power management subsystem, that it is ready for runtime
>> suspend if its all pins are configured for such state. This in turn might
>> allow to turn respective power domain off to reduce power consumption.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
>>  drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
>>  drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
>>  3 files changed, 13 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> index b7bd2e12a269..354eea0e7798 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> @@ -105,6 +105,7 @@ Required Properties:
>>    - samsung,pin-drv: Drive strength configuration.
>>    - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
>>    - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
>> +  - samsung,off-state: Mark this configuration as suitable for bank power off.
>>
>>    The values specified by these config properties should be derived from the
>>    hardware manual and these values are programmed as-is into the pin
>> @@ -113,6 +114,13 @@ Required Properties:
>>    Note: A child should include atleast a pin function selection property or
>>    pin configuration property (one or more) or both.
>>
>> +  Note: Special property "samsung,off-state" indicates that this state can
>> +  be used for device's "sleep" pins state. Its pin values/properties should
>> +  match the configuration in power down mode.
>
> Why power down values cannot be used for sleep state? Why you need
> separate pin control state? If pins values should match power down
> configuration, then they could just be added to default state, couldn't
> they?
>
> In the patch 2/9, existing configuration:
> 716         i2s0_bus: i2s0-bus {
> (...)
> 719                 samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> 720                 samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> 721                 samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> 722         };
>
> additional configuration:
> +       i2s0_bus_slp: i2s0-bus-slp {
> +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> +               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> +               samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
> +               samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
> +               samsung,off-state;
> +       };

+1

If I'm not missing something, it should be reasonably easy to
determine if a state is suitable for power off by its configuration,
by comparing pin-function and pin-pud with pin-con-pdn and
pin-pud-pdn.

Best regards,
Tomasz

Re: [PATCH 7/9] pinctrl: samsung: Add property to mark pad state as suitable for power down

[Marek Szyprowski]

Hi Krzysztof,


On 2016-12-25 20:19, Krzysztof Kozlowski wrote:
> On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
>> Add support for special property "samsung,off-state", which indicates a special
>> state suitable for device's "sleep" state. Its pin values/properties should
>> match the configuration in power down mode. It indicates that pin controller
>> can notify runtime power management subsystem, that it is ready for runtime
>> suspend if its all pins are configured for such state. This in turn might
>> allow to turn respective power domain off to reduce power consumption.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
>>   drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
>>   drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
>>   3 files changed, 13 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> index b7bd2e12a269..354eea0e7798 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> @@ -105,6 +105,7 @@ Required Properties:
>>     - samsung,pin-drv: Drive strength configuration.
>>     - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
>>     - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
>> +  - samsung,off-state: Mark this configuration as suitable for bank power off.
>>   
>>     The values specified by these config properties should be derived from the
>>     hardware manual and these values are programmed as-is into the pin
>> @@ -113,6 +114,13 @@ Required Properties:
>>     Note: A child should include atleast a pin function selection property or
>>     pin configuration property (one or more) or both.
>>   
>> +  Note: Special property "samsung,off-state" indicates that this state can
>> +  be used for device's "sleep" pins state. Its pin values/properties should
>> +  match the configuration in power down mode.
> Why power down values cannot be used for sleep state? Why you need
> separate pin control state? If pins values should match power down
> configuration, then they could just be added to default state, couldn't
> they?

Separate sleep state is needed because of the pin control bindings and 
design.

A separate sleep state is required to let pin control client driver (in 
this case
Exynos I2S driver) let to choose when it is okay to switch pads into sleep
state and I see no other way to achieve this.

> In the patch 2/9, existing configuration:
> 716         i2s0_bus: i2s0-bus {
> (...)
> 719                 samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> 720                 samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> 721                 samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> 722         };
>
> additional configuration:
> +       i2s0_bus_slp: i2s0-bus-slp {
> +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> +               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> +               samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
> +               samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
> +               samsung,off-state;
> +       };

I agree that it would be possible to get rid of "samsung,off-state" 
property and
just detect off state when func/pud pair matches power down func/pud.

>> It indicates that pin control
>> +  can notify runtime power management subsystem, that it is ready for runtime
>> +  suspend if its all pins are configured for such state. This in turn might
>> +  allow to turn respective power domain off to reduce power consumption.
> What do you mean by "notifying RPM subsystem"? Either this is
> description of hardware in certain mode (sleep state) or this is not
> device tree property.

Maybe I wrote the description with a view too limited to the kernel 
operation
perspective, but if we remove the need to mark state as off, the above 
description
will not be needed.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 7/9] pinctrl: samsung: Add property to mark pad state as suitable for power down

[Krzysztof Kozlowski]

On Tue, Dec 27, 2016 at 11:30:57AM +0100, Marek Szyprowski wrote:
> Hi Krzysztof,
> 
> 
> On 2016-12-25 20:19, Krzysztof Kozlowski wrote:
> > On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
> > > Add support for special property "samsung,off-state", which indicates a special
> > > state suitable for device's "sleep" state. Its pin values/properties should
> > > match the configuration in power down mode. It indicates that pin controller
> > > can notify runtime power management subsystem, that it is ready for runtime
> > > suspend if its all pins are configured for such state. This in turn might
> > > allow to turn respective power domain off to reduce power consumption.
> > > 
> > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > ---
> > >   Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
> > >   drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
> > >   drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
> > >   3 files changed, 13 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > index b7bd2e12a269..354eea0e7798 100644
> > > --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > @@ -105,6 +105,7 @@ Required Properties:
> > >     - samsung,pin-drv: Drive strength configuration.
> > >     - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
> > >     - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
> > > +  - samsung,off-state: Mark this configuration as suitable for bank power off.
> > >     The values specified by these config properties should be derived from the
> > >     hardware manual and these values are programmed as-is into the pin
> > > @@ -113,6 +114,13 @@ Required Properties:
> > >     Note: A child should include atleast a pin function selection property or
> > >     pin configuration property (one or more) or both.
> > > +  Note: Special property "samsung,off-state" indicates that this state can
> > > +  be used for device's "sleep" pins state. Its pin values/properties should
> > > +  match the configuration in power down mode.
> > Why power down values cannot be used for sleep state? Why you need
> > separate pin control state? If pins values should match power down
> > configuration, then they could just be added to default state, couldn't
> > they?
> 
> Separate sleep state is needed because of the pin control bindings and
> design.
> 
> A separate sleep state is required to let pin control client driver (in this
> case
> Exynos I2S driver) let to choose when it is okay to switch pads into sleep
> state and I see no other way to achieve this.

Maybe the pinctrl API should be extended for this? Doing this in DTS
just for purpose of passing information between drivers (consumer and
provider) looks odd.

Anyway, you are proposing a new binding so please Cc devicetree mailing
list and device tree maintainers.

> > In the patch 2/9, existing configuration:
> > 716         i2s0_bus: i2s0-bus {
> > (...)
> > 719                 samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> > 720                 samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> > 721                 samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> > 722         };
> > 
> > additional configuration:
> > +       i2s0_bus_slp: i2s0-bus-slp {
> > +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> > +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> > +               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> > +               samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
> > +               samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
> > +               samsung,off-state;
> > +       };
> 
> I agree that it would be possible to get rid of "samsung,off-state" property
> and
> just detect off state when func/pud pair matches power down func/pud.
> 
> > > It indicates that pin control
> > > +  can notify runtime power management subsystem, that it is ready for runtime
> > > +  suspend if its all pins are configured for such state. This in turn might
> > > +  allow to turn respective power domain off to reduce power consumption.
> > What do you mean by "notifying RPM subsystem"? Either this is
> > description of hardware in certain mode (sleep state) or this is not
> > device tree property.
> 
> Maybe I wrote the description with a view too limited to the kernel
> operation
> perspective, but if we remove the need to mark state as off, the above
> description
> will not be needed.

But still it wouldn't be describing any hardware property, would it?

Best regards,
Krzysztof

Re: [PATCH 7/9] pinctrl: samsung: Add property to mark pad state as suitable for power down

[Linus Walleij]

On Tue, Dec 27, 2016 at 4:33 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Dec 27, 2016 at 11:30:57AM +0100, Marek Szyprowski wrote:

>> Separate sleep state is needed because of the pin control bindings and
>> design.
>>
>> A separate sleep state is required to let pin control client driver (in this
>> case
>> Exynos I2S driver) let to choose when it is okay to switch pads into sleep
>> state and I see no other way to achieve this.
>
> Maybe the pinctrl API should be extended for this? Doing this in DTS
> just for purpose of passing information between drivers (consumer and
> provider) looks odd.

I don't understand what is going on but you are all smart people so I
guess you're right :)

Tell me if I can help clarifying something...

Yours,
Linus Walleij

Re: [PATCH 7/9] pinctrl: samsung: Add property to mark pad state as suitable for power down

[Marek Szyprowski]

Hi Krzysztof,

On 2016-12-27 16:33, Krzysztof Kozlowski wrote:
> On Tue, Dec 27, 2016 at 11:30:57AM +0100, Marek Szyprowski wrote:
>> On 2016-12-25 20:19, Krzysztof Kozlowski wrote:
>>> On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
>>>> Add support for special property "samsung,off-state", which indicates a special
>>>> state suitable for device's "sleep" state. Its pin values/properties should
>>>> match the configuration in power down mode. It indicates that pin controller
>>>> can notify runtime power management subsystem, that it is ready for runtime
>>>> suspend if its all pins are configured for such state. This in turn might
>>>> allow to turn respective power domain off to reduce power consumption.
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
>>>>    drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
>>>>    drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
>>>>    3 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>>>> index b7bd2e12a269..354eea0e7798 100644
>>>> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>>>> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>>>> @@ -105,6 +105,7 @@ Required Properties:
>>>>      - samsung,pin-drv: Drive strength configuration.
>>>>      - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
>>>>      - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
>>>> +  - samsung,off-state: Mark this configuration as suitable for bank power off.
>>>>      The values specified by these config properties should be derived from the
>>>>      hardware manual and these values are programmed as-is into the pin
>>>> @@ -113,6 +114,13 @@ Required Properties:
>>>>      Note: A child should include atleast a pin function selection property or
>>>>      pin configuration property (one or more) or both.
>>>> +  Note: Special property "samsung,off-state" indicates that this state can
>>>> +  be used for device's "sleep" pins state. Its pin values/properties should
>>>> +  match the configuration in power down mode.
>>> Why power down values cannot be used for sleep state? Why you need
>>> separate pin control state? If pins values should match power down
>>> configuration, then they could just be added to default state, couldn't
>>> they?
>> Separate sleep state is needed because of the pin control bindings and
>> design.
>>
>> A separate sleep state is required to let pin control client driver (in this
>> case
>> Exynos I2S driver) let to choose when it is okay to switch pads into sleep
>> state and I see no other way to achieve this.
> Maybe the pinctrl API should be extended for this? Doing this in DTS
> just for purpose of passing information between drivers (consumer and
> provider) looks odd.

Well, I don't know if it is odd or not, but that's how it is used now 
and I see
no reason to reinvent wheel. Please check it yourself:
$ git grep \"sleep\" arch/arm/boot/dts | wc -l
101

> Anyway, you are proposing a new binding so please Cc devicetree mailing
> list and device tree maintainers.

I'm just using the generic pinctrl bindings, but it might make some sense to
add a note to Exynos i2s driver that a sleep pin control state is needed if
one wants to get power domain to be turned off.

>>> In the patch 2/9, existing configuration:
>>> 716         i2s0_bus: i2s0-bus {
>>> (...)
>>> 719                 samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
>>> 720                 samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
>>> 721                 samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
>>> 722         };
>>>
>>> additional configuration:
>>> +       i2s0_bus_slp: i2s0-bus-slp {
>>> +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
>>> +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
>>> +               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
>>> +               samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
>>> +               samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
>>> +               samsung,off-state;
>>> +       };
>> I agree that it would be possible to get rid of "samsung,off-state" property
>> and
>> just detect off state when func/pud pair matches power down func/pud.
>>
>>>> It indicates that pin control
>>>> +  can notify runtime power management subsystem, that it is ready for runtime
>>>> +  suspend if its all pins are configured for such state. This in turn might
>>>> +  allow to turn respective power domain off to reduce power consumption.
>>> What do you mean by "notifying RPM subsystem"? Either this is
>>> description of hardware in certain mode (sleep state) or this is not
>>> device tree property.
>> Maybe I wrote the description with a view too limited to the kernel
>> operation
>> perspective, but if we remove the need to mark state as off, the above
>> description
>> will not be needed.
> But still it wouldn't be describing any hardware property, would it?

Well, it somehow describes the hardware, because the pin state when it 
is allowed
to turn off the power domain is board specific. I should move that part 
to the
Odroid dts, because there might be a board which require other pin 
values in power
down mode.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 7/9] pinctrl: samsung: Add property to mark pad state as suitable for power down

[Krzysztof Kozlowski]

On Fri, Dec 30, 2016 at 12:55:27PM +0100, Marek Szyprowski wrote:
> Hi Krzysztof,
> 
> On 2016-12-27 16:33, Krzysztof Kozlowski wrote:
> > On Tue, Dec 27, 2016 at 11:30:57AM +0100, Marek Szyprowski wrote:
> > > On 2016-12-25 20:19, Krzysztof Kozlowski wrote:
> > > > On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
> > > > > Add support for special property "samsung,off-state", which indicates a special
> > > > > state suitable for device's "sleep" state. Its pin values/properties should
> > > > > match the configuration in power down mode. It indicates that pin controller
> > > > > can notify runtime power management subsystem, that it is ready for runtime
> > > > > suspend if its all pins are configured for such state. This in turn might
> > > > > allow to turn respective power domain off to reduce power consumption.
> > > > > 
> > > > > Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > > ---
> > > > >    Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
> > > > >    drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
> > > > >    drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
> > > > >    3 files changed, 13 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > > > index b7bd2e12a269..354eea0e7798 100644
> > > > > --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > > > +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > > > @@ -105,6 +105,7 @@ Required Properties:
> > > > >      - samsung,pin-drv: Drive strength configuration.
> > > > >      - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
> > > > >      - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
> > > > > +  - samsung,off-state: Mark this configuration as suitable for bank power off.
> > > > >      The values specified by these config properties should be derived from the
> > > > >      hardware manual and these values are programmed as-is into the pin
> > > > > @@ -113,6 +114,13 @@ Required Properties:
> > > > >      Note: A child should include atleast a pin function selection property or
> > > > >      pin configuration property (one or more) or both.
> > > > > +  Note: Special property "samsung,off-state" indicates that this state can
> > > > > +  be used for device's "sleep" pins state. Its pin values/properties should
> > > > > +  match the configuration in power down mode.
> > > > Why power down values cannot be used for sleep state? Why you need
> > > > separate pin control state? If pins values should match power down
> > > > configuration, then they could just be added to default state, couldn't
> > > > they?
> > > Separate sleep state is needed because of the pin control bindings and
> > > design.
> > > 
> > > A separate sleep state is required to let pin control client driver (in this
> > > case
> > > Exynos I2S driver) let to choose when it is okay to switch pads into sleep
> > > state and I see no other way to achieve this.
> > Maybe the pinctrl API should be extended for this? Doing this in DTS
> > just for purpose of passing information between drivers (consumer and
> > provider) looks odd.
> 
> Well, I don't know if it is odd or not, but that's how it is used now and I
> see
> no reason to reinvent wheel. Please check it yourself:
> $ git grep \"sleep\" arch/arm/boot/dts | wc -l
> 101

These drivers, at least not all of them, are not using the existence of
sleep mode configuration as a indication of possible runtime sleep. You
are mixing here different ideas.

> 
> > Anyway, you are proposing a new binding so please Cc devicetree mailing
> > list and device tree maintainers.
> 
> I'm just using the generic pinctrl bindings, but it might make some sense to
> add a note to Exynos i2s driver that a sleep pin control state is needed if
> one wants to get power domain to be turned off.

The samsung,off-state is a extension of the existing binding, so please
Cc the devicetree and maintainers. Why you see a problem in it?

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/9] pinctrl: samsung: Add runtime PM support

[Marek Szyprowski]

This patch adds runtime power management support to Samsung pin controller
driver. It uses recently introduced property to mark some pin states as
suitable for power off. When all pins for given controller are set to this
special state, the controller is able to enter runtime suspend state. This
in turn might allow to turn respective power domain off to reduce power
consumption.

This patch moves saving driver state to runtime pm callbacks and implements
system sleep suspend/resume callbacks with pm_runtime_force_suspend/resume
helpers to keep the runtime pm state consistent with the hardware both
during runtime and system sleep transitions.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 24 ++++++++++++++++++++++--
 drivers/pinctrl/samsung/pinctrl-samsung.h |  1 +
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 301169d2b6e1..c741e93d65b8 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -28,6 +28,7 @@
 #include <linux/gpio.h>
 #include <linux/irqdomain.h>
 #include <linux/of_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/spinlock.h>
 
 #include "../core.h"
@@ -378,6 +379,17 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
 		shift -= 32;
 		reg += 4;
 	}
+	if (func->rpm_active) {
+		if (!(bank->rpm_map & (1 << pin_offset))) {
+			bank->rpm_map |= 1 << pin_offset;
+			pm_runtime_get_sync(drvdata->dev);
+		}
+	} else {
+		if ((bank->rpm_map & (1 << pin_offset))) {
+			bank->rpm_map &= ~(1 << pin_offset);
+			pm_runtime_put(drvdata->dev);
+		}
+	}
 
 	spin_lock_irqsave(&bank->slock, flags);
 
@@ -427,6 +439,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
 	if (cfg_type >= PINCFG_TYPE_NUM || !type->fld_width[cfg_type])
 		return -EINVAL;
 
+	pm_runtime_get_sync(drvdata->dev);
+
 	width = type->fld_width[cfg_type];
 	cfg_reg = type->reg_offset[cfg_type];
 
@@ -449,6 +463,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
 
 	spin_unlock_irqrestore(&bank->slock, flags);
 
+	pm_runtime_put(drvdata->dev);
+
 	return 0;
 }
 
@@ -1096,6 +1112,8 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 		ctrl->retention_init(drvdata);
 
 	platform_set_drvdata(pdev, drvdata);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
 
 	return 0;
 }
@@ -1242,8 +1260,10 @@ static int samsung_pinctrl_resume(struct device *dev)
 MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);
 
 static const struct dev_pm_ops samsung_pinctrl_pm_ops = {
-	SET_LATE_SYSTEM_SLEEP_PM_OPS(samsung_pinctrl_suspend,
-				     samsung_pinctrl_resume)
+	SET_RUNTIME_PM_OPS(samsung_pinctrl_suspend,
+			   samsung_pinctrl_resume, NULL)
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				     pm_runtime_force_resume)
 };
 
 static struct platform_driver samsung_pinctrl_driver = {
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index edeafa00abd3..ccb24ec46796 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -172,6 +172,7 @@ struct samsung_pin_bank {
 	const char	*name;
 
 	u32		pin_base;
+	u32		rpm_map;
 	void		*soc_priv;
 	struct device_node *of_node;
 	struct samsung_pinctrl_drv_data *drvdata;
-- 
1.9.1

Re: [PATCH 8/9] pinctrl: samsung: Add runtime PM support

[Krzysztof Kozlowski]

On Fri, Dec 23, 2016 at 01:24:48PM +0100, Marek Szyprowski wrote:
> This patch adds runtime power management support to Samsung pin controller
> driver. It uses recently introduced property to mark some pin states as
> suitable for power off. When all pins for given controller are set to this
> special state, the controller is able to enter runtime suspend state. This
> in turn might allow to turn respective power domain off to reduce power
> consumption.
> 
> This patch moves saving driver state to runtime pm callbacks and implements
> system sleep suspend/resume callbacks with pm_runtime_force_suspend/resume
> helpers to keep the runtime pm state consistent with the hardware both
> during runtime and system sleep transitions.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 24 ++++++++++++++++++++++--
>  drivers/pinctrl/samsung/pinctrl-samsung.h |  1 +
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 301169d2b6e1..c741e93d65b8 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -28,6 +28,7 @@
>  #include <linux/gpio.h>
>  #include <linux/irqdomain.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/spinlock.h>
>  
>  #include "../core.h"
> @@ -378,6 +379,17 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
>  		shift -= 32;
>  		reg += 4;
>  	}
> +	if (func->rpm_active) {
> +		if (!(bank->rpm_map & (1 << pin_offset))) {
> +			bank->rpm_map |= 1 << pin_offset;
> +			pm_runtime_get_sync(drvdata->dev);
> +		}
> +	} else {
> +		if ((bank->rpm_map & (1 << pin_offset))) {
> +			bank->rpm_map &= ~(1 << pin_offset);
> +			pm_runtime_put(drvdata->dev);
> +		}
> +	}
>  
>  	spin_lock_irqsave(&bank->slock, flags);
>  
> @@ -427,6 +439,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
>  	if (cfg_type >= PINCFG_TYPE_NUM || !type->fld_width[cfg_type])
>  		return -EINVAL;
>  
> +	pm_runtime_get_sync(drvdata->dev);
> +
>  	width = type->fld_width[cfg_type];
>  	cfg_reg = type->reg_offset[cfg_type];
>  
> @@ -449,6 +463,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
>  
>  	spin_unlock_irqrestore(&bank->slock, flags);
>  
> +	pm_runtime_put(drvdata->dev);
> +
>  	return 0;
>  }
>  
> @@ -1096,6 +1112,8 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
>  		ctrl->retention_init(drvdata);
>  
>  	platform_set_drvdata(pdev, drvdata);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
>  
>  	return 0;
>  }
> @@ -1242,8 +1260,10 @@ static int samsung_pinctrl_resume(struct device *dev)
>  MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);
>  
>  static const struct dev_pm_ops samsung_pinctrl_pm_ops = {
> -	SET_LATE_SYSTEM_SLEEP_PM_OPS(samsung_pinctrl_suspend,
> -				     samsung_pinctrl_resume)
> +	SET_RUNTIME_PM_OPS(samsung_pinctrl_suspend,
> +			   samsung_pinctrl_resume, NULL)
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				     pm_runtime_force_resume)
>  };
>  
>  static struct platform_driver samsung_pinctrl_driver = {
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
> index edeafa00abd3..ccb24ec46796 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> @@ -172,6 +172,7 @@ struct samsung_pin_bank {

A nit:
Missing kernel doc update.

It took me a while to understand the logic behind the rpm_active (the
code is simple but logic was not that easy to see) but it looks good and
sensible. I didn't find any issues with that approach except the usage
of DT configuration as a "notification" mechanism.

For this itself:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


>  	const char	*name;
>  
>  	u32		pin_base;
> +	u32		rpm_map;
>  	void		*soc_priv;
>  	struct device_node *of_node;
>  	struct samsung_pinctrl_drv_data *drvdata;
> -- 
> 1.9.1
> 

Re: [PATCH 8/9] pinctrl: samsung: Add runtime PM support

[Tomasz Figa]

Hi,

2016-12-23 21:24 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> This patch adds runtime power management support to Samsung pin controller
> driver. It uses recently introduced property to mark some pin states as
> suitable for power off. When all pins for given controller are set to this
> special state, the controller is able to enter runtime suspend state. This
> in turn might allow to turn respective power domain off to reduce power
> consumption.
>
> This patch moves saving driver state to runtime pm callbacks and implements
> system sleep suspend/resume callbacks with pm_runtime_force_suspend/resume
> helpers to keep the runtime pm state consistent with the hardware both
> during runtime and system sleep transitions.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 24 ++++++++++++++++++++++--
>  drivers/pinctrl/samsung/pinctrl-samsung.h |  1 +
>  2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 301169d2b6e1..c741e93d65b8 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -28,6 +28,7 @@
>  #include <linux/gpio.h>
>  #include <linux/irqdomain.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/spinlock.h>
>
>  #include "../core.h"
> @@ -378,6 +379,17 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
>                 shift -= 32;
>                 reg += 4;
>         }

nit: IMHO a blank line here would look better.

> +       if (func->rpm_active) {
> +               if (!(bank->rpm_map & (1 << pin_offset))) {

I think rpm_map could benefit from generic bitmap helpers.

> +                       bank->rpm_map |= 1 << pin_offset;
> +                       pm_runtime_get_sync(drvdata->dev);
> +               }
> +       } else {
> +               if ((bank->rpm_map & (1 << pin_offset))) {
> +                       bank->rpm_map &= ~(1 << pin_offset);
> +                       pm_runtime_put(drvdata->dev);
> +               }
> +       }

Then, the above could be simplified into:

        if (func->rpm_active && !test_bit(pin_offset, &bank->rpm_map)) {
                __set_bit(pin_offset, &bank->rpm_map);
                pm_runtime_get_sync(drvdata->dev);
        } else if (!func->rpm_active && test_bit(pin_offset, &bank->rpm_map) {
                __clear_bit(pin_offset, &bank->rpm_map);
                pm_runtime_put(drvdata->dev);
        }

>
>         spin_lock_irqsave(&bank->slock, flags);
>
> @@ -427,6 +439,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
>         if (cfg_type >= PINCFG_TYPE_NUM || !type->fld_width[cfg_type])
>                 return -EINVAL;
>
> +       pm_runtime_get_sync(drvdata->dev);
> +
>         width = type->fld_width[cfg_type];
>         cfg_reg = type->reg_offset[cfg_type];
>
> @@ -449,6 +463,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
>
>         spin_unlock_irqrestore(&bank->slock, flags);
>
> +       pm_runtime_put(drvdata->dev);
> +
>         return 0;
>  }
>
> @@ -1096,6 +1112,8 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
>                 ctrl->retention_init(drvdata);
>
>         platform_set_drvdata(pdev, drvdata);
> +       pm_runtime_set_active(dev);
> +       pm_runtime_enable(dev);

Can we really enable this unconditionally on all platforms?

>
>         return 0;
>  }
> @@ -1242,8 +1260,10 @@ static int samsung_pinctrl_resume(struct device *dev)
>  MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);
>
>  static const struct dev_pm_ops samsung_pinctrl_pm_ops = {
> -       SET_LATE_SYSTEM_SLEEP_PM_OPS(samsung_pinctrl_suspend,
> -                                    samsung_pinctrl_resume)
> +       SET_RUNTIME_PM_OPS(samsung_pinctrl_suspend,
> +                          samsung_pinctrl_resume, NULL)
> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                                    pm_runtime_force_resume)
>  };
>
>  static struct platform_driver samsung_pinctrl_driver = {
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
> index edeafa00abd3..ccb24ec46796 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> @@ -172,6 +172,7 @@ struct samsung_pin_bank {
>         const char      *name;
>
>         u32             pin_base;
> +       u32             rpm_map;

This could be an unsigned long instead and then could benefit from the
bitmap helpers.

Best regards,
Tomasz

[PATCH 9/9] ARM: dts: exynos: Add audio power domain support to Exynos542x SoCs

[Marek Szyprowski]

Audio power domain includes following hardware modules: Pin controller
for GPZ bank, AudioSS clock controller, PL330 ADMA device and Exynos I2S
controller.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/boot/dts/exynos5420.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 0a7ecdd4c5de..1b6bd9aa42d1 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -188,6 +188,7 @@
 			clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MAU_EPLL>,
 				 <&clock CLK_SCLK_MAUDIO0>, <&clock CLK_SCLK_MAUPCM0>;
 			clock-names = "pll_ref", "pll_in", "sclk_audio", "sclk_pcm_in";
+			power-domains = <&mau_pd>;
 		};
 
 		mfc: codec@11000000 {
@@ -317,6 +318,12 @@
 			clock-names = "oscclk", "clk0", "clk1", "clk2", "asb0", "asb1";
 		};
 
+		mau_pd: power-domain@100440E0 {
+			compatible = "samsung,exynos4210-pd";
+			reg = <0x100440E0 0x20>;
+			#power-domain-cells = <0>;
+		};
+
 		pinctrl_0: pinctrl@13400000 {
 			compatible = "samsung,exynos5420-pinctrl";
 			reg = <0x13400000 0x1000>;
@@ -356,6 +363,7 @@
 			reg = <0x03860000 0x1000>;
 			interrupts = <0 47 IRQ_TYPE_LEVEL_HIGH>;
 			samsung,pmu-syscon = <&pmu_system_controller>;
+			power-domains = <&mau_pd>;
 		};
 
 		amba {
@@ -374,6 +382,7 @@
 				#dma-cells = <1>;
 				#dma-channels = <6>;
 				#dma-requests = <16>;
+				power-domains = <&mau_pd>;
 			};
 
 			pdma0: pdma@121A0000 {
@@ -447,6 +456,7 @@
 			pinctrl-names = "default", "sleep";
 			pinctrl-0 = <&i2s0_bus>;
 			pinctrl-1 = <&i2s0_bus_slp>;
+			power-domains = <&mau_pd>;
 			status = "disabled";
 		};
 
-- 
1.9.1

Re: [PATCH 0/9] Runtime PM for Exynos pin controller driver

[Anand Moon]

Hi Marek

On 23 December 2016 at 17:54, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> Hello,
>
> This patchset is a next step to add support for audio power domain on
> Exynos5 SoCs.
>
> Audio power domain on Exynos5 SoCs contains following hardware modules:
> 1. clock controller
> 2. pin controller
> 3. PL330 DMA controller
> 4. I2S audio controller
>
> Till now it was assumed that pin controller is located in the "always on"
> power domain and lacked runtime power management. This patch finally
> removes such assumption and adds runtime pm support and awareness to this
> driver. To achieve this, some changes in the Exynos platform support code
> were needed, like moving pad retention control to the pin controller driver.
> Some cleanup to the pin controller driver has been also done while changing
> the code. This new feature requires some additional information in the
> device tree, what is handled by patches 1,2 and 9.
>
> Please note that patches are ordered in such a way that the changes can be
> bisected, so the properties are added to dts before the code requiring them.
>
> The other patches related to enabling full support for audio power domain
> can be found here:
> 1. PL330 ADMA controller non-irqsafe runtime PM:
>    https://www.spinics.net/lists/arm-kernel/msg550008.html
> 2. Runtime PM for clock controllers (Exynos Audio subsystem will be added
>    in v4 soon): https://www.spinics.net/lists/arm-kernel/msg538122.html
>
> Patches are based on linux-next from 2016.12.22.
>
> Best regards
> Marek Szyprowski
> Samsung R&D Institute Poland
>
>
> Patch summary:
>
> Marek Szyprowski (9):
>   ARM: dts: exynos: Add PMU syscon to pinctrl nodes
>   ARM: dts: exynos: Add pinctrl sleep state for 542x i2s module
>   pinctrl: samsung: Remove dead code
>   pinctrl: samsung: Use generic of_device_get_match_data helper
>   pinctrl: samsung: Move retention control from mach-exynos to the
>     pinctrl driver
>   pinctrl: samsung: Replace syscore ops with standard platform device
>     pm_ops
>   pinctrl: samsung: Add property to mark pad state as suitable for power
>     down
>   pinctrl: samsung: Add runtime PM support
>   ARM: dts: exynos: Add audio power domain support to Exynos542x SoCs
>
>  .../bindings/pinctrl/samsung-pinctrl.txt           |  12 ++
>  arch/arm/boot/dts/exynos3250.dtsi                  |   2 +
>  arch/arm/boot/dts/exynos4210.dtsi                  |   3 +
>  arch/arm/boot/dts/exynos4x12.dtsi                  |   3 +
>  arch/arm/boot/dts/exynos5250.dtsi                  |   4 +
>  arch/arm/boot/dts/exynos5420-pinctrl.dtsi          |  11 ++
>  arch/arm/boot/dts/exynos5420.dtsi                  |  18 ++-
>  arch/arm/mach-exynos/suspend.c                     |  64 ---------
>  drivers/pinctrl/samsung/pinctrl-exynos.c           | 148 +++++++++++++++++++++
>  drivers/pinctrl/samsung/pinctrl-samsung.c          | 126 ++++++++----------
>  drivers/pinctrl/samsung/pinctrl-samsung.h          |  15 +++
>  11 files changed, 271 insertions(+), 135 deletions(-)
>

Is their core configuration missing to enable audio through HDMI on
Odroid Boards.
I could not get the sound working on Odroid XU4.
.
-Best Regards
Anand Moon

Re: [PATCH 0/9] Runtime PM for Exynos pin controller driver

[Marek Szyprowski]

Hi Anand,


On 2016-12-24 11:10, Anand Moon wrote:
> Hi Marek
>
> On 23 December 2016 at 17:54, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> Hello,
>>
>> This patchset is a next step to add support for audio power domain on
>> Exynos5 SoCs.
>>
>> Audio power domain on Exynos5 SoCs contains following hardware modules:
>> 1. clock controller
>> 2. pin controller
>> 3. PL330 DMA controller
>> 4. I2S audio controller
>>
>> Till now it was assumed that pin controller is located in the "always on"
>> power domain and lacked runtime power management. This patch finally
>> removes such assumption and adds runtime pm support and awareness to this
>> driver. To achieve this, some changes in the Exynos platform support code
>> were needed, like moving pad retention control to the pin controller driver.
>> Some cleanup to the pin controller driver has been also done while changing
>> the code. This new feature requires some additional information in the
>> device tree, what is handled by patches 1,2 and 9.
>>
>> Please note that patches are ordered in such a way that the changes can be
>> bisected, so the properties are added to dts before the code requiring them.
>>
>> The other patches related to enabling full support for audio power domain
>> can be found here:
>> 1. PL330 ADMA controller non-irqsafe runtime PM:
>>     https://www.spinics.net/lists/arm-kernel/msg550008.html
>> 2. Runtime PM for clock controllers (Exynos Audio subsystem will be added
>>     in v4 soon): https://www.spinics.net/lists/arm-kernel/msg538122.html
>>
>> Patches are based on linux-next from 2016.12.22.
>>
>> Best regards
>> Marek Szyprowski
>> Samsung R&D Institute Poland
>>
>>
>> Patch summary:
>>
>> Marek Szyprowski (9):
>>    ARM: dts: exynos: Add PMU syscon to pinctrl nodes
>>    ARM: dts: exynos: Add pinctrl sleep state for 542x i2s module
>>    pinctrl: samsung: Remove dead code
>>    pinctrl: samsung: Use generic of_device_get_match_data helper
>>    pinctrl: samsung: Move retention control from mach-exynos to the
>>      pinctrl driver
>>    pinctrl: samsung: Replace syscore ops with standard platform device
>>      pm_ops
>>    pinctrl: samsung: Add property to mark pad state as suitable for power
>>      down
>>    pinctrl: samsung: Add runtime PM support
>>    ARM: dts: exynos: Add audio power domain support to Exynos542x SoCs
>>
>>   .../bindings/pinctrl/samsung-pinctrl.txt           |  12 ++
>>   arch/arm/boot/dts/exynos3250.dtsi                  |   2 +
>>   arch/arm/boot/dts/exynos4210.dtsi                  |   3 +
>>   arch/arm/boot/dts/exynos4x12.dtsi                  |   3 +
>>   arch/arm/boot/dts/exynos5250.dtsi                  |   4 +
>>   arch/arm/boot/dts/exynos5420-pinctrl.dtsi          |  11 ++
>>   arch/arm/boot/dts/exynos5420.dtsi                  |  18 ++-
>>   arch/arm/mach-exynos/suspend.c                     |  64 ---------
>>   drivers/pinctrl/samsung/pinctrl-exynos.c           | 148 +++++++++++++++++++++
>>   drivers/pinctrl/samsung/pinctrl-samsung.c          | 126 ++++++++----------
>>   drivers/pinctrl/samsung/pinctrl-samsung.h          |  15 +++
>>   11 files changed, 271 insertions(+), 135 deletions(-)
>>
> Is their core configuration missing to enable audio through HDMI on
> Odroid Boards.
> I could not get the sound working on Odroid XU4.

Audio support for HDMI on Exynos requires some additional code. We will take
a look at this too.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland