Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1395072 > unrolled thread

[RFC v2 00/13] usb/mmc/power: Fix USB/LAN when TFTP booting

Back to article view | Back to linux.kernel

Contents

  [RFC v2 00/13] usb/mmc/power: Fix USB/LAN when TFTP booting Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
    [RFC v2 03/13] MAINTAINERS: Retain Ulf Hansson as the same maintainer  of pwrseq Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
      Re: [RFC v2 03/13] MAINTAINERS: Retain Ulf Hansson as the same  maintainer of pwrseq Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-05 20:50 +0200
    [RFC v2 08/13] usb: hub: Handle deferred probe Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
    [RFC v2 09/13] power: pwrseq: Add support for USB hubs with external  power Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
      Re: [RFC v2 09/13] power: pwrseq: Add support for USB hubs with  external power Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-05 22:00 +0200
        Re: [RFC v2 09/13] power: pwrseq: Add support for USB hubs with  external power Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-06 08:30 +0200
    [RFC v2 10/13] usb: hub: Power sequence the ports on activation Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
      Re: [RFC v2 10/13] usb: hub: Power sequence the ports on activation Alan Stern <stern@rowland.harvard.edu> - 2016-05-05 16:20 +0200
        Re: [RFC v2 10/13] usb: hub: Power sequence the ports on activation Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 19:40 +0200
      Re: [RFC v2 10/13] usb: hub: Power sequence the ports on activation Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-05 22:00 +0200
    [RFC v2 04/13] power: pwrseq: Enable COMPILE_TEST for drivers Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
      Re: [RFC v2 04/13] power: pwrseq: Enable COMPILE_TEST for drivers Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-05 21:00 +0200
    [RFC v2 06/13] power: pwrseq: Generalize mmc_pwrseq operations by  removing mmc prefix Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
      Re: [RFC v2 06/13] power: pwrseq: Generalize mmc_pwrseq operations by  removing mmc prefix Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-05 21:20 +0200
    [RFC v2 12/13] ARM: dts: exynos: Switch the buck8 to GPIO mode on  Odroid U3 Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
    [RFC v2 11/13] usb: port: Parse pwrseq phandle from Device Tree Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
      Re: [RFC v2 11/13] usb: port: Parse pwrseq phandle from Device Tree Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-05 22:20 +0200
        Re: [RFC v2 11/13] usb: port: Parse pwrseq phandle from Device Tree Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-06 08:30 +0200
    [RFC v2 13/13] ARM: dts: exynos: Fix LAN and HUB after bootloader  initialization on Odroid U3 Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
      Re: [RFC v2 13/13] ARM: dts: exynos: Fix LAN and HUB after bootloader  initialization on Odroid U3 Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-05 22:20 +0200
        Re: [RFC v2 13/13] ARM: dts: exynos: Fix LAN and HUB after bootloader  initialization on Odroid U3 Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-06 08:30 +0200
    [RFC v2 05/13] power: pwrseq: Remove mmc prefix from mmc_pwrseq Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
      Re: [RFC v2 05/13] power: pwrseq: Remove mmc prefix from mmc_pwrseq Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-05 21:20 +0200
        Re: [RFC v2 05/13] power: pwrseq: Remove mmc prefix from mmc_pwrseq Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-06 08:30 +0200
    [RFC v2 07/13] power: pwrseq: simple: Add support for toggling  regulator Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:40 +0200
      Re: [RFC v2 07/13] power: pwrseq: simple: Add support for toggling  regulator Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-05 21:40 +0200
        Re: [RFC v2 07/13] power: pwrseq: simple: Add support for toggling  regulator Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-06 08:30 +0200
    [RFC v2 02/13] power/mmc: Move pwrseq drivers to power/pwrseq Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-05 14:50 +0200
      Re: [RFC v2 02/13] power/mmc: Move pwrseq drivers to power/pwrseq Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-05 20:50 +0200
        Re: [RFC v2 02/13] power/mmc: Move pwrseq drivers to power/pwrseq Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-06 08:20 +0200
    Re: [RFC v2 00/13] usb/mmc/power: Fix USB/LAN when TFTP booting Rob Herring <robh@kernel.org> - 2016-05-06 00:50 +0200
      Re: [RFC v2 00/13] usb/mmc/power: Fix USB/LAN when TFTP booting Peter Chen <hzpeterchen@gmail.com> - 2016-05-06 08:00 +0200
        Re: [RFC v2 00/13] usb/mmc/power: Fix USB/LAN when TFTP booting Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-06 08:20 +0200
          Re: [RFC v2 00/13] usb/mmc/power: Fix USB/LAN when TFTP booting Peter Chen <hzpeterchen@gmail.com> - 2016-05-06 09:30 +0200
      Re: [RFC v2 00/13] usb/mmc/power: Fix USB/LAN when TFTP booting Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-06 08:20 +0200
        Re: [RFC v2 00/13] usb/mmc/power: Fix USB/LAN when TFTP booting Rob Herring <robh@kernel.org> - 2016-05-06 15:10 +0200
      Re: [RFC v2 00/13] usb/mmc/power: Fix USB/LAN when TFTP booting Ulf Hansson <ulf.hansson@linaro.org> - 2016-05-09 09:50 +0200
        Re: [RFC v2 00/13] usb/mmc/power: Fix USB/LAN when TFTP booting Rob Herring <robh@kernel.org> - 2016-05-09 20:20 +0200
          Re: [RFC v2 00/13] usb/mmc/power: Fix USB/LAN when TFTP booting Ulf Hansson <ulf.hansson@linaro.org> - 2016-05-10 13:10 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1395349 — Re: [RFC v2 13/13] ARM: dts: exynos: Fix LAN and HUB after bootloader initialization on Odroid U3

Hello Krzysztof,

On 05/05/2016 08:34 AM, Krzysztof Kozlowski wrote:
> On Odroid U3 (Exynos4412-based) board if USB was initialized by
> bootloader (in U-Boot "usb start" before tftpboot), the HUB (usb3503)
> and LAN (smsc95xx) after after successful probing were not visible in the
> system ("lsusb").
> 
> In such case the devices had to be fully reset before configuring.
> Reset by GPIO (called RESET_N pin) and by RESET field in STCD register
> in usb3503 HUB are not sufficient. Instead full reset has to be done by
> disabling and enabling regulator.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---

[snip]

>  
> +	lan_pwrseq: pwrseq2 {
> +		compatible = "mmc-pwrseq-simple";

It feels strange to have a "mmc-pwrseq-simple" compatible for a USB power
sequence provider. As I mentioned in the other patch, I think there should
either be a DT binding for the USB pwrseq-simple with a "usb-pwrseq-simple"
compatible that binds to the same pwrseq-simple driver or maybe having a
generic DT binding for any device with a new "pwrseq-simple" compatible.

Patch looks good to me though, so after having a DT binding and changing
the compatible string:

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[toc] | [prev] | [next] | [standalone]

#1395619 — Re: [RFC v2 13/13] ARM: dts: exynos: Fix LAN and HUB after bootloader initialization on Odroid U3

On 05/05/2016 10:16 PM, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 05/05/2016 08:34 AM, Krzysztof Kozlowski wrote:
>> On Odroid U3 (Exynos4412-based) board if USB was initialized by
>> bootloader (in U-Boot "usb start" before tftpboot), the HUB (usb3503)
>> and LAN (smsc95xx) after after successful probing were not visible in the
>> system ("lsusb").
>>
>> In such case the devices had to be fully reset before configuring.
>> Reset by GPIO (called RESET_N pin) and by RESET field in STCD register
>> in usb3503 HUB are not sufficient. Instead full reset has to be done by
>> disabling and enabling regulator.
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> ---
> 
> [snip]
> 
>>  
>> +	lan_pwrseq: pwrseq2 {
>> +		compatible = "mmc-pwrseq-simple";
> 
> It feels strange to have a "mmc-pwrseq-simple" compatible for a USB power
> sequence provider. As I mentioned in the other patch, I think there should
> either be a DT binding for the USB pwrseq-simple with a "usb-pwrseq-simple"
> compatible that binds to the same pwrseq-simple driver or maybe having a
> generic DT binding for any device with a new "pwrseq-simple" compatible.
> 
> Patch looks good to me though, so after having a DT binding and changing
> the compatible string:
> 
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Probably this will change after Rob's feedback. :)

Best regards,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1395083 — [RFC v2 05/13] power: pwrseq: Remove mmc prefix from mmc_pwrseq

The "mmc" prefix is no longer needed after moving the pwrseq core code
from mmc/ to power/.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/power/pwrseq/pwrseq.c        | 18 +++++++++---------
 drivers/power/pwrseq/pwrseq_emmc.c   |  8 ++++----
 drivers/power/pwrseq/pwrseq_simple.c |  8 ++++----
 include/linux/mmc/host.h             |  4 ++--
 include/linux/pwrseq.h               | 20 ++++++++++----------
 5 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/power/pwrseq/pwrseq.c b/drivers/power/pwrseq/pwrseq.c
index 66310d7643cc..9c665821f890 100644
--- a/drivers/power/pwrseq/pwrseq.c
+++ b/drivers/power/pwrseq/pwrseq.c
@@ -21,7 +21,7 @@ static LIST_HEAD(pwrseq_list);
 int mmc_pwrseq_alloc(struct mmc_host *host)
 {
 	struct device_node *np;
-	struct mmc_pwrseq *p;
+	struct pwrseq *p;
 
 	np = of_parse_phandle(host->parent->of_node, "mmc-pwrseq", 0);
 	if (!np)
@@ -54,7 +54,7 @@ EXPORT_SYMBOL_GPL(mmc_pwrseq_alloc);
 
 void mmc_pwrseq_pre_power_on(struct mmc_host *host)
 {
-	struct mmc_pwrseq *pwrseq = host->pwrseq;
+	struct pwrseq *pwrseq = host->pwrseq;
 
 	if (pwrseq && pwrseq->ops->pre_power_on)
 		pwrseq->ops->pre_power_on(host);
@@ -63,7 +63,7 @@ EXPORT_SYMBOL_GPL(mmc_pwrseq_pre_power_on);
 
 void mmc_pwrseq_post_power_on(struct mmc_host *host)
 {
-	struct mmc_pwrseq *pwrseq = host->pwrseq;
+	struct pwrseq *pwrseq = host->pwrseq;
 
 	if (pwrseq && pwrseq->ops->post_power_on)
 		pwrseq->ops->post_power_on(host);
@@ -72,7 +72,7 @@ EXPORT_SYMBOL_GPL(mmc_pwrseq_post_power_on);
 
 void mmc_pwrseq_power_off(struct mmc_host *host)
 {
-	struct mmc_pwrseq *pwrseq = host->pwrseq;
+	struct pwrseq *pwrseq = host->pwrseq;
 
 	if (pwrseq && pwrseq->ops->power_off)
 		pwrseq->ops->power_off(host);
@@ -81,7 +81,7 @@ EXPORT_SYMBOL_GPL(mmc_pwrseq_power_off);
 
 void mmc_pwrseq_free(struct mmc_host *host)
 {
-	struct mmc_pwrseq *pwrseq = host->pwrseq;
+	struct pwrseq *pwrseq = host->pwrseq;
 
 	if (pwrseq) {
 		module_put(pwrseq->owner);
@@ -90,7 +90,7 @@ void mmc_pwrseq_free(struct mmc_host *host)
 }
 EXPORT_SYMBOL_GPL(mmc_pwrseq_free);
 
-int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
+int pwrseq_register(struct pwrseq *pwrseq)
 {
 	if (!pwrseq || !pwrseq->ops || !pwrseq->dev)
 		return -EINVAL;
@@ -101,9 +101,9 @@ int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(mmc_pwrseq_register);
+EXPORT_SYMBOL_GPL(pwrseq_register);
 
-void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq)
+void pwrseq_unregister(struct pwrseq *pwrseq)
 {
 	if (pwrseq) {
 		mutex_lock(&pwrseq_list_mutex);
@@ -111,4 +111,4 @@ void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq)
 		mutex_unlock(&pwrseq_list_mutex);
 	}
 }
-EXPORT_SYMBOL_GPL(mmc_pwrseq_unregister);
+EXPORT_SYMBOL_GPL(pwrseq_unregister);
diff --git a/drivers/power/pwrseq/pwrseq_emmc.c b/drivers/power/pwrseq/pwrseq_emmc.c
index a0583ed46d7f..a68ac9a68e04 100644
--- a/drivers/power/pwrseq/pwrseq_emmc.c
+++ b/drivers/power/pwrseq/pwrseq_emmc.c
@@ -22,7 +22,7 @@
 #include <linux/mmc/host.h>
 
 struct mmc_pwrseq_emmc {
-	struct mmc_pwrseq pwrseq;
+	struct pwrseq pwrseq;
 	struct notifier_block reset_nb;
 	struct gpio_desc *reset_gpio;
 };
@@ -54,7 +54,7 @@ static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
 	return NOTIFY_DONE;
 }
 
-static const struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
+static const struct pwrseq_ops mmc_pwrseq_emmc_ops = {
 	.post_power_on = mmc_pwrseq_emmc_reset,
 };
 
@@ -85,7 +85,7 @@ static int mmc_pwrseq_emmc_probe(struct platform_device *pdev)
 	pwrseq->pwrseq.owner = THIS_MODULE;
 	platform_set_drvdata(pdev, pwrseq);
 
-	return mmc_pwrseq_register(&pwrseq->pwrseq);
+	return pwrseq_register(&pwrseq->pwrseq);
 }
 
 static int mmc_pwrseq_emmc_remove(struct platform_device *pdev)
@@ -93,7 +93,7 @@ static int mmc_pwrseq_emmc_remove(struct platform_device *pdev)
 	struct mmc_pwrseq_emmc *pwrseq = platform_get_drvdata(pdev);
 
 	unregister_restart_handler(&pwrseq->reset_nb);
-	mmc_pwrseq_unregister(&pwrseq->pwrseq);
+	pwrseq_unregister(&pwrseq->pwrseq);
 
 	return 0;
 }
diff --git a/drivers/power/pwrseq/pwrseq_simple.c b/drivers/power/pwrseq/pwrseq_simple.c
index 786f1db53a3f..d5fbd653153e 100644
--- a/drivers/power/pwrseq/pwrseq_simple.c
+++ b/drivers/power/pwrseq/pwrseq_simple.c
@@ -21,7 +21,7 @@
 #include <linux/mmc/host.h>
 
 struct mmc_pwrseq_simple {
-	struct mmc_pwrseq pwrseq;
+	struct pwrseq pwrseq;
 	bool clk_enabled;
 	struct clk *ext_clk;
 	struct gpio_descs *reset_gpios;
@@ -77,7 +77,7 @@ static void mmc_pwrseq_simple_power_off(struct mmc_host *host)
 	}
 }
 
-static const struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
+static const struct pwrseq_ops mmc_pwrseq_simple_ops = {
 	.pre_power_on = mmc_pwrseq_simple_pre_power_on,
 	.post_power_on = mmc_pwrseq_simple_post_power_on,
 	.power_off = mmc_pwrseq_simple_power_off,
@@ -115,14 +115,14 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
 	pwrseq->pwrseq.owner = THIS_MODULE;
 	platform_set_drvdata(pdev, pwrseq);
 
-	return mmc_pwrseq_register(&pwrseq->pwrseq);
+	return pwrseq_register(&pwrseq->pwrseq);
 }
 
 static int mmc_pwrseq_simple_remove(struct platform_device *pdev)
 {
 	struct mmc_pwrseq_simple *pwrseq = platform_get_drvdata(pdev);
 
-	mmc_pwrseq_unregister(&pwrseq->pwrseq);
+	pwrseq_unregister(&pwrseq->pwrseq);
 
 	return 0;
 }
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 85800b48241f..ad281bb3475f 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -203,7 +203,7 @@ struct mmc_context_info {
 };
 
 struct regulator;
-struct mmc_pwrseq;
+struct pwrseq;
 
 struct mmc_supply {
 	struct regulator *vmmc;		/* Card power supply */
@@ -215,7 +215,7 @@ struct mmc_host {
 	struct device		class_dev;
 	int			index;
 	const struct mmc_host_ops *ops;
-	struct mmc_pwrseq	*pwrseq;
+	struct pwrseq		*pwrseq;
 	unsigned int		f_min;
 	unsigned int		f_max;
 	unsigned int		f_init;
diff --git a/include/linux/pwrseq.h b/include/linux/pwrseq.h
index e230670c1d8d..6d2d2a44ad35 100644
--- a/include/linux/pwrseq.h
+++ b/include/linux/pwrseq.h
@@ -5,19 +5,19 @@
  *
  * License terms: GNU General Public License (GPL) version 2
  */
-#ifndef _MMC_CORE_PWRSEQ_H
-#define _MMC_CORE_PWRSEQ_H
+#ifndef _LINUX_PWRSEQ_H
+#define _LINUX_PWRSEQ_H
 
 #include <linux/mmc/host.h>
 
-struct mmc_pwrseq_ops {
+struct pwrseq_ops {
 	void (*pre_power_on)(struct mmc_host *host);
 	void (*post_power_on)(struct mmc_host *host);
 	void (*power_off)(struct mmc_host *host);
 };
 
-struct mmc_pwrseq {
-	const struct mmc_pwrseq_ops *ops;
+struct pwrseq {
+	const struct pwrseq_ops *ops;
 	struct device *dev;
 	struct list_head pwrseq_node;
 	struct module *owner;
@@ -25,8 +25,8 @@ struct mmc_pwrseq {
 
 #ifdef CONFIG_POWER_SEQ
 
-int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq);
-void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq);
+int pwrseq_register(struct pwrseq *pwrseq);
+void pwrseq_unregister(struct pwrseq *pwrseq);
 
 int mmc_pwrseq_alloc(struct mmc_host *host);
 void mmc_pwrseq_pre_power_on(struct mmc_host *host);
@@ -36,11 +36,11 @@ void mmc_pwrseq_free(struct mmc_host *host);
 
 #else /* CONFIG_POWER_SEQ */
 
-static inline int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
+static inline int pwrseq_register(struct pwrseq *pwrseq)
 {
 	return -ENOSYS;
 }
-static inline void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq) {}
+static inline void pwrseq_unregister(struct pwrseq *pwrseq) {}
 static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
 static inline void mmc_pwrseq_pre_power_on(struct mmc_host *host) {}
 static inline void mmc_pwrseq_post_power_on(struct mmc_host *host) {}
@@ -49,4 +49,4 @@ static inline void mmc_pwrseq_free(struct mmc_host *host) {}
 
 #endif /* CONFIG_POWER_SEQ */
 
-#endif
+#endif /* _LINUX_PWRSEQ_H */
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1395309 — Re: [RFC v2 05/13] power: pwrseq: Remove mmc prefix from mmc_pwrseq

Hello Krzysztof,

On 05/05/2016 08:34 AM, Krzysztof Kozlowski wrote:
> The "mmc" prefix is no longer needed after moving the pwrseq core code
> from mmc/ to power/.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---

[snip]

> diff --git a/drivers/power/pwrseq/pwrseq_emmc.c b/drivers/power/pwrseq/pwrseq_emmc.c
> index a0583ed46d7f..a68ac9a68e04 100644
> --- a/drivers/power/pwrseq/pwrseq_emmc.c
> +++ b/drivers/power/pwrseq/pwrseq_emmc.c
> @@ -22,7 +22,7 @@
>  #include <linux/mmc/host.h>
> 

I don't think this header inclusion is needed. At least I didn't see anything
defined there that's used in this driver. This also applies to pwrseq_simple.

I think you could remove those in another preparatory patches before the move.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[toc] | [prev] | [next] | [standalone]

#1395622 — Re: [RFC v2 05/13] power: pwrseq: Remove mmc prefix from mmc_pwrseq

On 05/05/2016 09:09 PM, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 05/05/2016 08:34 AM, Krzysztof Kozlowski wrote:
>> The "mmc" prefix is no longer needed after moving the pwrseq core code
>> from mmc/ to power/.
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> ---
> 
> [snip]
> 
>> diff --git a/drivers/power/pwrseq/pwrseq_emmc.c b/drivers/power/pwrseq/pwrseq_emmc.c
>> index a0583ed46d7f..a68ac9a68e04 100644
>> --- a/drivers/power/pwrseq/pwrseq_emmc.c
>> +++ b/drivers/power/pwrseq/pwrseq_emmc.c
>> @@ -22,7 +22,7 @@
>>  #include <linux/mmc/host.h>
>>
> 
> I don't think this header inclusion is needed. At least I didn't see anything
> defined there that's used in this driver. This also applies to pwrseq_simple.
> 
> I think you could remove those in another preparatory patches before the move.
>
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Good catch, thanks.

BR,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1395084 — [RFC v2 07/13] power: pwrseq: simple: Add support for toggling regulator

Some devices need real hard-reset by cutting the power.  During power
sequence turn off and on the regulator, if it is provided.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 .../devicetree/bindings/mmc/mmc-pwrseq-simple.txt  |  2 +
 drivers/power/pwrseq/pwrseq_simple.c               | 50 ++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
index ce0e76749671..176ff831e7f1 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
@@ -16,6 +16,7 @@ Optional properties:
   See ../clocks/clock-bindings.txt for details.
 - clock-names : Must include the following entry:
   "ext_clock" (External clock provided to the card).
+- ext-supply : External regulator supply
 
 Example:
 
@@ -24,4 +25,5 @@ Example:
 		reset-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
 		clocks = <&clk_32768_ck>;
 		clock-names = "ext_clock";
+		ext-supply = <&buck8>;
 	}
diff --git a/drivers/power/pwrseq/pwrseq_simple.c b/drivers/power/pwrseq/pwrseq_simple.c
index ab0098412690..4d5ea53d3ead 100644
--- a/drivers/power/pwrseq/pwrseq_simple.c
+++ b/drivers/power/pwrseq/pwrseq_simple.c
@@ -16,7 +16,9 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
 #include <linux/pwrseq.h>
+#include <linux/delay.h>
 
 #include <linux/mmc/host.h>
 
@@ -25,6 +27,7 @@ struct mmc_pwrseq_simple {
 	bool clk_enabled;
 	struct clk *ext_clk;
 	struct gpio_descs *reset_gpios;
+	struct regulator *ext_reg;
 };
 
 #define to_pwrseq_simple(p) container_of(p, struct mmc_pwrseq_simple, pwrseq)
@@ -62,6 +65,13 @@ static void mmc_pwrseq_simple_post_power_on(struct pwrseq *_pwrseq)
 {
 	struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(_pwrseq);
 
+	if (pwrseq->ext_reg) {
+		int err;
+
+		err = regulator_enable(pwrseq->ext_reg);
+		WARN_ON_ONCE(err);
+	}
+
 	mmc_pwrseq_simple_set_gpios_value(pwrseq, 0);
 }
 
@@ -75,6 +85,13 @@ static void mmc_pwrseq_simple_power_off(struct pwrseq *_pwrseq)
 		clk_disable_unprepare(pwrseq->ext_clk);
 		pwrseq->clk_enabled = false;
 	}
+
+	if (pwrseq->ext_reg) {
+		int err;
+
+		err = regulator_disable(pwrseq->ext_reg);
+		WARN_ON_ONCE(err);
+	}
 }
 
 static const struct pwrseq_ops mmc_pwrseq_simple_ops = {
@@ -102,6 +119,32 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
 	if (IS_ERR(pwrseq->ext_clk) && PTR_ERR(pwrseq->ext_clk) != -ENOENT)
 		return PTR_ERR(pwrseq->ext_clk);
 
+	/* FIXME: regulator_get_exclusive? */
+	pwrseq->ext_reg = devm_regulator_get_optional(dev, "ext");
+	if (IS_ERR(pwrseq->ext_reg)) {
+		if (PTR_ERR(pwrseq->ext_reg) == -ENODEV)
+			pwrseq->ext_reg = NULL;
+		else
+			return PTR_ERR(pwrseq->ext_reg);
+	} else {
+		int err;
+		/*
+		 * Be sure that regulator is off, before the driver will start
+		 * power sequence. It is likely that regulator is on by default
+		 * and it without toggling it here, it would be disabled much
+		 * later by the core.
+		 */
+
+		err = regulator_enable(pwrseq->ext_reg);
+		WARN_ON_ONCE(err);
+
+		/* FIXME: handle this in a more sensible way */
+		mdelay(10);
+
+		err = regulator_disable(pwrseq->ext_reg);
+		WARN_ON_ONCE(err);
+	}
+
 	pwrseq->reset_gpios = devm_gpiod_get_array(dev, "reset",
 							GPIOD_OUT_HIGH);
 	if (IS_ERR(pwrseq->reset_gpios) &&
@@ -124,6 +167,13 @@ static int mmc_pwrseq_simple_remove(struct platform_device *pdev)
 
 	pwrseq_unregister(&pwrseq->pwrseq);
 
+	if (pwrseq->ext_reg) {
+		int err;
+
+		err = regulator_disable(pwrseq->ext_reg);
+		WARN_ON_ONCE(err);
+	}
+
 	return 0;
 }
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1395324 — Re: [RFC v2 07/13] power: pwrseq: simple: Add support for toggling regulator

Hello Krzysztof,

On 05/05/2016 08:34 AM, Krzysztof Kozlowski wrote:
> Some devices need real hard-reset by cutting the power.  During power
> sequence turn off and on the regulator, if it is provided.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---

[snip]

>  
>  #define to_pwrseq_simple(p) container_of(p, struct mmc_pwrseq_simple, pwrseq)
> @@ -62,6 +65,13 @@ static void mmc_pwrseq_simple_post_power_on(struct pwrseq *_pwrseq)
>  {
>  	struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(_pwrseq);
>  
> +	if (pwrseq->ext_reg) {
> +		int err;
> +
> +		err = regulator_enable(pwrseq->ext_reg);
> +		WARN_ON_ONCE(err);
> +	}
> +

Shouldn't this be in mmc_pwrseq_simple_pre_power_on() instead?

For example, a chip may need to be powered on before attempting to
toggle its reset or power pins using some GPIO lines.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[toc] | [prev] | [next] | [standalone]

#1395617 — Re: [RFC v2 07/13] power: pwrseq: simple: Add support for toggling regulator

On 05/05/2016 09:31 PM, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 05/05/2016 08:34 AM, Krzysztof Kozlowski wrote:
>> Some devices need real hard-reset by cutting the power.  During power
>> sequence turn off and on the regulator, if it is provided.
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> ---
> 
> [snip]
> 
>>  
>>  #define to_pwrseq_simple(p) container_of(p, struct mmc_pwrseq_simple, pwrseq)
>> @@ -62,6 +65,13 @@ static void mmc_pwrseq_simple_post_power_on(struct pwrseq *_pwrseq)
>>  {
>>  	struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(_pwrseq);
>>  
>> +	if (pwrseq->ext_reg) {
>> +		int err;
>> +
>> +		err = regulator_enable(pwrseq->ext_reg);
>> +		WARN_ON_ONCE(err);
>> +	}
>> +
> 
> Shouldn't this be in mmc_pwrseq_simple_pre_power_on() instead?
> 
> For example, a chip may need to be powered on before attempting to
> toggle its reset or power pins using some GPIO lines.

Indeed this should be still sorted out but here the assumption is that
regulator is disabled (by probe()) so it should be turned on with
GPIO-reset set.

This can be done at the end of pre-power-on or here (beginning of
post-power-on). On the other hand I understand these pre/post callbacks
as one starting the reset sequence (pre) and second as finishing it
(post). In case of regulators, finishing power sequence is to turn the
regulator on.

Best regards,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1395087 — [RFC v2 02/13] power/mmc: Move pwrseq drivers to power/pwrseq

The MMC power sequence drivers are useful also outside of MMC world: for
USB devices needed a hard-reset before probing. Before extending and
re-using pwrseq drivers, move them to a new place.

The commit does not introduce significant changes in the pwrseq drivers
code so still all the functions are prefixed with "mmc_pwrseq". However
the MMC-specific pwrseq functions has to be now exported and everything
is hidden not by CONFIG_OF but by new CONFIG_POWER_SEQ option.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/mmc/Kconfig                                |  2 --
 drivers/mmc/core/Makefile                          |  3 ---
 drivers/mmc/core/core.c                            |  2 +-
 drivers/mmc/core/host.c                            |  2 +-
 drivers/power/Kconfig                              |  1 +
 drivers/power/Makefile                             |  1 +
 drivers/{mmc/core => power/pwrseq}/Kconfig         | 17 ++++++++++++-----
 drivers/power/pwrseq/Makefile                      |  3 +++
 drivers/{mmc/core => power/pwrseq}/pwrseq.c        |  8 ++++++--
 drivers/{mmc/core => power/pwrseq}/pwrseq_emmc.c   |  3 +--
 drivers/{mmc/core => power/pwrseq}/pwrseq_simple.c |  3 +--
 {drivers/mmc/core => include/linux}/pwrseq.h       |  6 +++---
 12 files changed, 30 insertions(+), 21 deletions(-)
 rename drivers/{mmc/core => power/pwrseq}/Kconfig (71%)
 create mode 100644 drivers/power/pwrseq/Makefile
 rename drivers/{mmc/core => power/pwrseq}/pwrseq.c (90%)
 rename drivers/{mmc/core => power/pwrseq}/pwrseq_emmc.c (99%)
 rename drivers/{mmc/core => power/pwrseq}/pwrseq_simple.c (99%)
 rename {drivers/mmc/core => include/linux}/pwrseq.h (94%)

diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index f2eeb38efa65..7ade379e0634 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -21,8 +21,6 @@ config MMC_DEBUG
 
 if MMC
 
-source "drivers/mmc/core/Kconfig"
-
 source "drivers/mmc/card/Kconfig"
 
 source "drivers/mmc/host/Kconfig"
diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
index f007151dfdc6..a901d3cd09d3 100644
--- a/drivers/mmc/core/Makefile
+++ b/drivers/mmc/core/Makefile
@@ -8,7 +8,4 @@ mmc_core-y			:= core.o bus.o host.o \
 				   sdio.o sdio_ops.o sdio_bus.o \
 				   sdio_cis.o sdio_io.o sdio_irq.o \
 				   quirks.o slot-gpio.o
-mmc_core-$(CONFIG_OF)		+= pwrseq.o
-obj-$(CONFIG_PWRSEQ_SIMPLE)	+= pwrseq_simple.o
-obj-$(CONFIG_PWRSEQ_EMMC)	+= pwrseq_emmc.o
 mmc_core-$(CONFIG_DEBUG_FS)	+= debugfs.o
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 99275e40bf2f..0f145ff6e4f1 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -25,6 +25,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/pm_wakeup.h>
 #include <linux/suspend.h>
+#include <linux/pwrseq.h>
 #include <linux/fault-inject.h>
 #include <linux/random.h>
 #include <linux/slab.h>
@@ -43,7 +44,6 @@
 #include "bus.h"
 #include "host.h"
 #include "sdio_bus.h"
-#include "pwrseq.h"
 
 #include "mmc_ops.h"
 #include "sd_ops.h"
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index e0a3ee16c0d3..98164a352dfb 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -21,6 +21,7 @@
 #include <linux/export.h>
 #include <linux/leds.h>
 #include <linux/slab.h>
+#include <linux/pwrseq.h>
 
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
@@ -29,7 +30,6 @@
 #include "core.h"
 #include "host.h"
 #include "slot-gpio.h"
-#include "pwrseq.h"
 
 #define cls_dev_to_mmc_host(d)	container_of(d, struct mmc_host, class_dev)
 
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 421770ddafa3..2702aca6cd2c 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -511,5 +511,6 @@ config AXP20X_POWER
 
 endif # POWER_SUPPLY
 
+source "drivers/power/pwrseq/Kconfig"
 source "drivers/power/reset/Kconfig"
 source "drivers/power/avs/Kconfig"
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index e46b75d448a5..02f9d5da2e76 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -71,6 +71,7 @@ obj-$(CONFIG_POWER_AVS)		+= avs/
 obj-$(CONFIG_CHARGER_SMB347)	+= smb347-charger.o
 obj-$(CONFIG_CHARGER_TPS65090)	+= tps65090-charger.o
 obj-$(CONFIG_CHARGER_TPS65217)	+= tps65217_charger.o
+obj-$(CONFIG_POWER_SEQ)		+= pwrseq/
 obj-$(CONFIG_POWER_RESET)	+= reset/
 obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
 obj-$(CONFIG_AXP288_CHARGER)	+= axp288_charger.o
diff --git a/drivers/mmc/core/Kconfig b/drivers/power/pwrseq/Kconfig
similarity index 71%
rename from drivers/mmc/core/Kconfig
rename to drivers/power/pwrseq/Kconfig
index 250f223aaa80..b5d2d6c65f28 100644
--- a/drivers/mmc/core/Kconfig
+++ b/drivers/power/pwrseq/Kconfig
@@ -1,7 +1,12 @@
-#
-# MMC core configuration
-#
-config PWRSEQ_EMMC
+menuconfig POWER_SEQ
+	default y if OF
+	bool "Hardware reset support for specific devices"
+	help
+	  Provides drivers which reset the specific device before...
+
+if POWER_SEQ
+
+config POWER_SEQ_EMMC
 	tristate "HW reset support for eMMC"
 	default y
 	depends on OF
@@ -12,7 +17,7 @@ config PWRSEQ_EMMC
 	  This driver can also be built as a module. If so, the module
 	  will be called pwrseq_emmc.
 
-config PWRSEQ_SIMPLE
+config POWER_SEQ_SIMPLE
 	tristate "Simple HW reset support for MMC"
 	default y
 	depends on OF
@@ -22,3 +27,5 @@ config PWRSEQ_SIMPLE
 
 	  This driver can also be built as a module. If so, the module
 	  will be called pwrseq_simple.
+
+endif
diff --git a/drivers/power/pwrseq/Makefile b/drivers/power/pwrseq/Makefile
new file mode 100644
index 000000000000..9e40e4b9068b
--- /dev/null
+++ b/drivers/power/pwrseq/Makefile
@@ -0,0 +1,3 @@
+obj-$(CONFIG_POWER_SEQ)		+= pwrseq.o
+obj-$(CONFIG_POWER_SEQ_SIMPLE)	+= pwrseq_simple.o
+obj-$(CONFIG_POWER_SEQ_EMMC)	+= pwrseq_emmc.o
diff --git a/drivers/mmc/core/pwrseq.c b/drivers/power/pwrseq/pwrseq.c
similarity index 90%
rename from drivers/mmc/core/pwrseq.c
rename to drivers/power/pwrseq/pwrseq.c
index 9386c4771814..66310d7643cc 100644
--- a/drivers/mmc/core/pwrseq.c
+++ b/drivers/power/pwrseq/pwrseq.c
@@ -11,11 +11,10 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pwrseq.h>
 
 #include <linux/mmc/host.h>
 
-#include "pwrseq.h"
-
 static DEFINE_MUTEX(pwrseq_list_mutex);
 static LIST_HEAD(pwrseq_list);
 
@@ -51,6 +50,7 @@ int mmc_pwrseq_alloc(struct mmc_host *host)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mmc_pwrseq_alloc);
 
 void mmc_pwrseq_pre_power_on(struct mmc_host *host)
 {
@@ -59,6 +59,7 @@ void mmc_pwrseq_pre_power_on(struct mmc_host *host)
 	if (pwrseq && pwrseq->ops->pre_power_on)
 		pwrseq->ops->pre_power_on(host);
 }
+EXPORT_SYMBOL_GPL(mmc_pwrseq_pre_power_on);
 
 void mmc_pwrseq_post_power_on(struct mmc_host *host)
 {
@@ -67,6 +68,7 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host)
 	if (pwrseq && pwrseq->ops->post_power_on)
 		pwrseq->ops->post_power_on(host);
 }
+EXPORT_SYMBOL_GPL(mmc_pwrseq_post_power_on);
 
 void mmc_pwrseq_power_off(struct mmc_host *host)
 {
@@ -75,6 +77,7 @@ void mmc_pwrseq_power_off(struct mmc_host *host)
 	if (pwrseq && pwrseq->ops->power_off)
 		pwrseq->ops->power_off(host);
 }
+EXPORT_SYMBOL_GPL(mmc_pwrseq_power_off);
 
 void mmc_pwrseq_free(struct mmc_host *host)
 {
@@ -85,6 +88,7 @@ void mmc_pwrseq_free(struct mmc_host *host)
 		host->pwrseq = NULL;
 	}
 }
+EXPORT_SYMBOL_GPL(mmc_pwrseq_free);
 
 int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
 {
diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/power/pwrseq/pwrseq_emmc.c
similarity index 99%
rename from drivers/mmc/core/pwrseq_emmc.c
rename to drivers/power/pwrseq/pwrseq_emmc.c
index adc9c0c614fb..a0583ed46d7f 100644
--- a/drivers/mmc/core/pwrseq_emmc.c
+++ b/drivers/power/pwrseq/pwrseq_emmc.c
@@ -17,11 +17,10 @@
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/reboot.h>
+#include <linux/pwrseq.h>
 
 #include <linux/mmc/host.h>
 
-#include "pwrseq.h"
-
 struct mmc_pwrseq_emmc {
 	struct mmc_pwrseq pwrseq;
 	struct notifier_block reset_nb;
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/power/pwrseq/pwrseq_simple.c
similarity index 99%
rename from drivers/mmc/core/pwrseq_simple.c
rename to drivers/power/pwrseq/pwrseq_simple.c
index 450d907c6e6c..786f1db53a3f 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/power/pwrseq/pwrseq_simple.c
@@ -16,11 +16,10 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
+#include <linux/pwrseq.h>
 
 #include <linux/mmc/host.h>
 
-#include "pwrseq.h"
-
 struct mmc_pwrseq_simple {
 	struct mmc_pwrseq pwrseq;
 	bool clk_enabled;
diff --git a/drivers/mmc/core/pwrseq.h b/include/linux/pwrseq.h
similarity index 94%
rename from drivers/mmc/core/pwrseq.h
rename to include/linux/pwrseq.h
index d69e751f148b..e230670c1d8d 100644
--- a/drivers/mmc/core/pwrseq.h
+++ b/include/linux/pwrseq.h
@@ -23,7 +23,7 @@ struct mmc_pwrseq {
 	struct module *owner;
 };
 
-#ifdef CONFIG_OF
+#ifdef CONFIG_POWER_SEQ
 
 int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq);
 void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq);
@@ -34,7 +34,7 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host);
 void mmc_pwrseq_power_off(struct mmc_host *host);
 void mmc_pwrseq_free(struct mmc_host *host);
 
-#else
+#else /* CONFIG_POWER_SEQ */
 
 static inline int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
 {
@@ -47,6 +47,6 @@ static inline void mmc_pwrseq_post_power_on(struct mmc_host *host) {}
 static inline void mmc_pwrseq_power_off(struct mmc_host *host) {}
 static inline void mmc_pwrseq_free(struct mmc_host *host) {}
 
-#endif
+#endif /* CONFIG_POWER_SEQ */
 
 #endif
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1395284 — Re: [RFC v2 02/13] power/mmc: Move pwrseq drivers to power/pwrseq

Hello Krzysztof,

On 05/05/2016 08:34 AM, Krzysztof Kozlowski wrote:
> The MMC power sequence drivers are useful also outside of MMC world: for
> USB devices needed a hard-reset before probing. Before extending and
> re-using pwrseq drivers, move them to a new place.
> 
> The commit does not introduce significant changes in the pwrseq drivers
> code so still all the functions are prefixed with "mmc_pwrseq". However
> the MMC-specific pwrseq functions has to be now exported and everything
> is hidden not by CONFIG_OF but by new CONFIG_POWER_SEQ option.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---

[snip]

> --- a/drivers/mmc/core/Kconfig
> +++ b/drivers/power/pwrseq/Kconfig
> @@ -1,7 +1,12 @@
> -#
> -# MMC core configuration
> -#
> -config PWRSEQ_EMMC
> +menuconfig POWER_SEQ
> +	default y if OF
> +	bool "Hardware reset support for specific devices"
> +	help
> +	  Provides drivers which reset the specific device before...
> +

I think this text could be improved a little bit, maybe something like:

"Provides drivers that implements specific power sequences for chips,
using the generic power sequence management interface".

The rest looks good to me.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[toc] | [prev] | [next] | [standalone]

#1395613 — Re: [RFC v2 02/13] power/mmc: Move pwrseq drivers to power/pwrseq

On 05/05/2016 08:44 PM, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 05/05/2016 08:34 AM, Krzysztof Kozlowski wrote:
>> The MMC power sequence drivers are useful also outside of MMC world: for
>> USB devices needed a hard-reset before probing. Before extending and
>> re-using pwrseq drivers, move them to a new place.
>>
>> The commit does not introduce significant changes in the pwrseq drivers
>> code so still all the functions are prefixed with "mmc_pwrseq". However
>> the MMC-specific pwrseq functions has to be now exported and everything
>> is hidden not by CONFIG_OF but by new CONFIG_POWER_SEQ option.
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> ---
> 
> [snip]
> 
>> --- a/drivers/mmc/core/Kconfig
>> +++ b/drivers/power/pwrseq/Kconfig
>> @@ -1,7 +1,12 @@
>> -#
>> -# MMC core configuration
>> -#
>> -config PWRSEQ_EMMC
>> +menuconfig POWER_SEQ
>> +	default y if OF
>> +	bool "Hardware reset support for specific devices"
>> +	help
>> +	  Provides drivers which reset the specific device before...
>> +
> 
> I think this text could be improved a little bit, maybe something like:
> 
> "Provides drivers that implements specific power sequences for chips,
> using the generic power sequence management interface".
> 
> The rest looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Yes, I wanted to code, not to focus on descriptions, so also commit
messages should be extended.

I'll fix this in next iteration.

BR,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1395427

On Thu, May 05, 2016 at 02:34:13PM +0200, Krzysztof Kozlowski wrote:
> Hi,
> 
> This is a different, second try to fix usb3503+lan on Odroid U3 board
> if it was initialized by bootloader (e.g. for TFTP boot).
> 
> First version:
> http://www.spinics.net/lists/linux-usb/msg140042.html
> 
> 
> Problem
> =======
> When Odroid U3 (usb3503 + smsc95xx + max77686) boots from network (TFTP),
> the usb3503 and LAN smsc95xx do not show up in "lsusb". Hard-reset
> is required, e.g. by suspend to RAM. The actual TFTP boot does
> not have to happen. Just "usb start" from U-Boot is sufficient.
> 
> From the schematics, the regulator is a supply only to LAN, however
> without toggling it off/on, the usb3503 hub won appear neither.
> 
> 
> Solution
> ========
> This is very similar to the MMC pwrseq behavior so the idea is to:
> 1. Move MMC pwrseq drivers to generic place,

You can do that, but I'm going to NAK any use of pwrseq bindings outside 
of MMC. I think it is the wrong way to do things. The DT should describe 
the devices. If they happen to be "simple" then the core can walk the 
tree and do any setup. For example, look for "reset-gpios" and toggle 
that GPIO. There is no need for a special node.

> 2. Extend the pwrseq-simple with regulator toggling,
> 3. Add support to USB hub and port core for pwrseq,

We discussed this for USB already[1] and is why we defined how to add 
USB child devices. The idea is not to add pwrseq to that.

Rob

[1] http://www.spinics.net/lists/linux-usb/msg134082.html

[toc] | [prev] | [next] | [standalone]

#1395608

On Thu, May 05, 2016 at 05:42:40PM -0500, Rob Herring wrote:
> On Thu, May 05, 2016 at 02:34:13PM +0200, Krzysztof Kozlowski wrote:
> > Hi,
> > 
> > This is a different, second try to fix usb3503+lan on Odroid U3 board
> > if it was initialized by bootloader (e.g. for TFTP boot).
> > 
> > First version:
> > http://www.spinics.net/lists/linux-usb/msg140042.html
> > 
> > 
> > Problem
> > =======
> > When Odroid U3 (usb3503 + smsc95xx + max77686) boots from network (TFTP),
> > the usb3503 and LAN smsc95xx do not show up in "lsusb". Hard-reset
> > is required, e.g. by suspend to RAM. The actual TFTP boot does
> > not have to happen. Just "usb start" from U-Boot is sufficient.
> > 
> > From the schematics, the regulator is a supply only to LAN, however
> > without toggling it off/on, the usb3503 hub won appear neither.
> > 
> > 
> > Solution
> > ========
> > This is very similar to the MMC pwrseq behavior so the idea is to:
> > 1. Move MMC pwrseq drivers to generic place,
> 
> You can do that, but I'm going to NAK any use of pwrseq bindings outside 
> of MMC. I think it is the wrong way to do things. The DT should describe 
> the devices. If they happen to be "simple" then the core can walk the 
> tree and do any setup. For example, look for "reset-gpios" and toggle 
> that GPIO. There is no need for a special node.
> 

Oh, I am doing the same thing like this patch set doing. Then, how can
we let things be generic like you mention at [1] if without common
pwrseq driver/library? The properties like "reset-gpios" under
USB child node seems can only be handled by USB driver.

> > 2. Extend the pwrseq-simple with regulator toggling,
> > 3. Add support to USB hub and port core for pwrseq,
> 
> We discussed this for USB already[1] and is why we defined how to add 
> USB child devices. The idea is not to add pwrseq to that.
> 
> Rob
> 
> [1] http://www.spinics.net/lists/linux-usb/msg134082.html

[1] http://www.spinics.net/lists/linux-usb/msg137312.html

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1395616

On 05/06/2016 07:44 AM, Peter Chen wrote:
> On Thu, May 05, 2016 at 05:42:40PM -0500, Rob Herring wrote:
>> On Thu, May 05, 2016 at 02:34:13PM +0200, Krzysztof Kozlowski wrote:
>>> Hi,
>>>
>>> This is a different, second try to fix usb3503+lan on Odroid U3 board
>>> if it was initialized by bootloader (e.g. for TFTP boot).
>>>
>>> First version:
>>> http://www.spinics.net/lists/linux-usb/msg140042.html
>>>
>>>
>>> Problem
>>> =======
>>> When Odroid U3 (usb3503 + smsc95xx + max77686) boots from network (TFTP),
>>> the usb3503 and LAN smsc95xx do not show up in "lsusb". Hard-reset
>>> is required, e.g. by suspend to RAM. The actual TFTP boot does
>>> not have to happen. Just "usb start" from U-Boot is sufficient.
>>>
>>> From the schematics, the regulator is a supply only to LAN, however
>>> without toggling it off/on, the usb3503 hub won appear neither.
>>>
>>>
>>> Solution
>>> ========
>>> This is very similar to the MMC pwrseq behavior so the idea is to:
>>> 1. Move MMC pwrseq drivers to generic place,
>>
>> You can do that, but I'm going to NAK any use of pwrseq bindings outside 
>> of MMC. I think it is the wrong way to do things. The DT should describe 
>> the devices. If they happen to be "simple" then the core can walk the 
>> tree and do any setup. For example, look for "reset-gpios" and toggle 
>> that GPIO. There is no need for a special node.
>>
> 
> Oh, I am doing the same thing like this patch set doing.

Shame on me that I did not use Google before starting the work. I could
just extend your patchset. I think we can combine our efforts.

Best regards,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1395655

On Fri, May 06, 2016 at 08:12:24AM +0200, Krzysztof Kozlowski wrote:
> On 05/06/2016 07:44 AM, Peter Chen wrote:
> > On Thu, May 05, 2016 at 05:42:40PM -0500, Rob Herring wrote:
> >> On Thu, May 05, 2016 at 02:34:13PM +0200, Krzysztof Kozlowski wrote:
> >>> Hi,
> >>>
> >>> This is a different, second try to fix usb3503+lan on Odroid U3 board
> >>> if it was initialized by bootloader (e.g. for TFTP boot).
> >>>
> >>> First version:
> >>> http://www.spinics.net/lists/linux-usb/msg140042.html
> >>>
> >>>
> >>> Problem
> >>> =======
> >>> When Odroid U3 (usb3503 + smsc95xx + max77686) boots from network (TFTP),
> >>> the usb3503 and LAN smsc95xx do not show up in "lsusb". Hard-reset
> >>> is required, e.g. by suspend to RAM. The actual TFTP boot does
> >>> not have to happen. Just "usb start" from U-Boot is sufficient.
> >>>
> >>> From the schematics, the regulator is a supply only to LAN, however
> >>> without toggling it off/on, the usb3503 hub won appear neither.
> >>>
> >>>
> >>> Solution
> >>> ========
> >>> This is very similar to the MMC pwrseq behavior so the idea is to:
> >>> 1. Move MMC pwrseq drivers to generic place,
> >>
> >> You can do that, but I'm going to NAK any use of pwrseq bindings outside 
> >> of MMC. I think it is the wrong way to do things. The DT should describe 
> >> the devices. If they happen to be "simple" then the core can walk the 
> >> tree and do any setup. For example, look for "reset-gpios" and toggle 
> >> that GPIO. There is no need for a special node.
> >>
> > 
> > Oh, I am doing the same thing like this patch set doing.
> 
> Shame on me that I did not use Google before starting the work. I could
> just extend your patchset. I think we can combine our efforts.
> 

Sure :)

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1395615

On 05/06/2016 12:42 AM, Rob Herring wrote:
> On Thu, May 05, 2016 at 02:34:13PM +0200, Krzysztof Kozlowski wrote:
>> Hi,
>>
>> This is a different, second try to fix usb3503+lan on Odroid U3 board
>> if it was initialized by bootloader (e.g. for TFTP boot).
>>
>> First version:
>> http://www.spinics.net/lists/linux-usb/msg140042.html
>>
>>
>> Problem
>> =======
>> When Odroid U3 (usb3503 + smsc95xx + max77686) boots from network (TFTP),
>> the usb3503 and LAN smsc95xx do not show up in "lsusb". Hard-reset
>> is required, e.g. by suspend to RAM. The actual TFTP boot does
>> not have to happen. Just "usb start" from U-Boot is sufficient.
>>
>> From the schematics, the regulator is a supply only to LAN, however
>> without toggling it off/on, the usb3503 hub won appear neither.
>>
>>
>> Solution
>> ========
>> This is very similar to the MMC pwrseq behavior so the idea is to:
>> 1. Move MMC pwrseq drivers to generic place,
> 
> You can do that, but I'm going to NAK any use of pwrseq bindings outside 
> of MMC. I think it is the wrong way to do things. The DT should describe 
> the devices. If they happen to be "simple" then the core can walk the 
> tree and do any setup. For example, look for "reset-gpios" and toggle 
> that GPIO. There is no need for a special node.

Okay, I got it, no node for pwrseq but parse device properties. In case
of reset-gpios it seems quite obvious but also actively used:
$ git grep reset-gpios arch/arm/boot/dts | wc -l
142

Definitely pwrseq shouldn't add itself to all of these devices.

My questions would be then:
1. An additional pwrseq compatible for device is acceptable?
2. How would you name the regulator? We shouldn't toggle off/on every
regulator but probably only some specific ones.

> 
>> 2. Extend the pwrseq-simple with regulator toggling,
>> 3. Add support to USB hub and port core for pwrseq,
> 
> We discussed this for USB already[1] and is why we defined how to add 
> USB child devices. The idea is not to add pwrseq to that.

Yes, I left it for next iteration because it would require much more
changes in USB core. As for now, these bindings are useless for USB
devices which are not yet enumerated (because power sequence has to be
done on them). Making use of these bindings would be a next step... Just
let me do it one step a time.

Best regards,
Krzysztof

> 
> Rob
> 
> [1] http://www.spinics.net/lists/linux-usb/msg134082.html
> 
> 

[toc] | [prev] | [next] | [standalone]

#1395850

On Fri, May 6, 2016 at 1:10 AM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On 05/06/2016 12:42 AM, Rob Herring wrote:
>> On Thu, May 05, 2016 at 02:34:13PM +0200, Krzysztof Kozlowski wrote:
>>> Hi,
>>>
>>> This is a different, second try to fix usb3503+lan on Odroid U3 board
>>> if it was initialized by bootloader (e.g. for TFTP boot).
>>>
>>> First version:
>>> http://www.spinics.net/lists/linux-usb/msg140042.html
>>>
>>>
>>> Problem
>>> =======
>>> When Odroid U3 (usb3503 + smsc95xx + max77686) boots from network (TFTP),
>>> the usb3503 and LAN smsc95xx do not show up in "lsusb". Hard-reset
>>> is required, e.g. by suspend to RAM. The actual TFTP boot does
>>> not have to happen. Just "usb start" from U-Boot is sufficient.
>>>
>>> From the schematics, the regulator is a supply only to LAN, however
>>> without toggling it off/on, the usb3503 hub won appear neither.
>>>
>>>
>>> Solution
>>> ========
>>> This is very similar to the MMC pwrseq behavior so the idea is to:
>>> 1. Move MMC pwrseq drivers to generic place,
>>
>> You can do that, but I'm going to NAK any use of pwrseq bindings outside
>> of MMC. I think it is the wrong way to do things. The DT should describe
>> the devices. If they happen to be "simple" then the core can walk the
>> tree and do any setup. For example, look for "reset-gpios" and toggle
>> that GPIO. There is no need for a special node.
>
> Okay, I got it, no node for pwrseq but parse device properties. In case
> of reset-gpios it seems quite obvious but also actively used:
> $ git grep reset-gpios arch/arm/boot/dts | wc -l
> 142
>
> Definitely pwrseq shouldn't add itself to all of these devices.
>
> My questions would be then:
> 1. An additional pwrseq compatible for device is acceptable?

Perhaps. The issue is whether common or driver specific code handles
this may change over time. In other words, it is purely a kernel
decision. It could move in either direction. It may be better to just
have a whitelist of devices (though it would need to be board specific
somehow).

> 2. How would you name the regulator? We shouldn't toggle off/on every
> regulator but probably only some specific ones.

I'd argue the generic case is just enable all the ones defined. If you
only need to deal with some of them or need a specific sequence, then
it is not generic.

Rob

[toc] | [prev] | [next] | [standalone]

#1396778

On 6 May 2016 at 00:42, Rob Herring <robh@kernel.org> wrote:
> On Thu, May 05, 2016 at 02:34:13PM +0200, Krzysztof Kozlowski wrote:
>> Hi,
>>
>> This is a different, second try to fix usb3503+lan on Odroid U3 board
>> if it was initialized by bootloader (e.g. for TFTP boot).
>>
>> First version:
>> http://www.spinics.net/lists/linux-usb/msg140042.html
>>
>>
>> Problem
>> =======
>> When Odroid U3 (usb3503 + smsc95xx + max77686) boots from network (TFTP),
>> the usb3503 and LAN smsc95xx do not show up in "lsusb". Hard-reset
>> is required, e.g. by suspend to RAM. The actual TFTP boot does
>> not have to happen. Just "usb start" from U-Boot is sufficient.
>>
>> From the schematics, the regulator is a supply only to LAN, however
>> without toggling it off/on, the usb3503 hub won appear neither.
>>
>>
>> Solution
>> ========
>> This is very similar to the MMC pwrseq behavior so the idea is to:
>> 1. Move MMC pwrseq drivers to generic place,
>
> You can do that, but I'm going to NAK any use of pwrseq bindings outside
> of MMC. I think it is the wrong way to do things. The DT should describe

Huh, I didn't know that was your view of the mmc pwrseq bindings. Why
didn't you NAK them before?

> the devices. If they happen to be "simple" then the core can walk the
> tree and do any setup. For example, look for "reset-gpios" and toggle
> that GPIO. There is no need for a special node.
>
>> 2. Extend the pwrseq-simple with regulator toggling,
>> 3. Add support to USB hub and port core for pwrseq,
>
> We discussed this for USB already[1] and is why we defined how to add
> USB child devices. The idea is not to add pwrseq to that.

I am not familiar with the USB discussion.

Still, let me give you some more background to the mmc pwrseq. The
idea from the mmc pwrseq bindings comes from the power-domain DT
bindings, as I thought these things were a bit related.
In both cases they are not directly a property of the device, but more
describing a HW dependency to allow the device to work.

One could probably use a child node instead of a phandle, but that
wasn't chosen back then. Of course you are the DT expert, but could
you perhaps tell me why a child node is better for cases like this?

>
> Rob
>
> [1] http://www.spinics.net/lists/linux-usb/msg134082.html

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1397245

On Mon, May 09, 2016 at 09:46:40AM +0200, Ulf Hansson wrote:
> On 6 May 2016 at 00:42, Rob Herring <robh@kernel.org> wrote:
> > On Thu, May 05, 2016 at 02:34:13PM +0200, Krzysztof Kozlowski wrote:
> >> Hi,
> >>
> >> This is a different, second try to fix usb3503+lan on Odroid U3 board
> >> if it was initialized by bootloader (e.g. for TFTP boot).
> >>
> >> First version:
> >> http://www.spinics.net/lists/linux-usb/msg140042.html
> >>
> >>
> >> Problem
> >> =======
> >> When Odroid U3 (usb3503 + smsc95xx + max77686) boots from network (TFTP),
> >> the usb3503 and LAN smsc95xx do not show up in "lsusb". Hard-reset
> >> is required, e.g. by suspend to RAM. The actual TFTP boot does
> >> not have to happen. Just "usb start" from U-Boot is sufficient.
> >>
> >> From the schematics, the regulator is a supply only to LAN, however
> >> without toggling it off/on, the usb3503 hub won appear neither.
> >>
> >>
> >> Solution
> >> ========
> >> This is very similar to the MMC pwrseq behavior so the idea is to:
> >> 1. Move MMC pwrseq drivers to generic place,
> >
> > You can do that, but I'm going to NAK any use of pwrseq bindings outside
> > of MMC. I think it is the wrong way to do things. The DT should describe
> 
> Huh, I didn't know that was your view of the mmc pwrseq bindings. Why
> didn't you NAK them before?

Unfortunately, either I missed it or it was a time I couldn't spend much 
time on reviews.

> > the devices. If they happen to be "simple" then the core can walk the
> > tree and do any setup. For example, look for "reset-gpios" and toggle
> > that GPIO. There is no need for a special node.
> >
> >> 2. Extend the pwrseq-simple with regulator toggling,
> >> 3. Add support to USB hub and port core for pwrseq,
> >
> > We discussed this for USB already[1] and is why we defined how to add
> > USB child devices. The idea is not to add pwrseq to that.
> 
> I am not familiar with the USB discussion.
> 
> Still, let me give you some more background to the mmc pwrseq. The
> idea from the mmc pwrseq bindings comes from the power-domain DT
> bindings, as I thought these things were a bit related.
> In both cases they are not directly a property of the device, but more
> describing a HW dependency to allow the device to work.

I could see this as a board level power domain. However the difference 
is we are not generally exposing internal SOC details the same way as 
board level components. Perhaps we could extend power domains to board 
level, but that is not what was done here.

> One could probably use a child node instead of a phandle, but that
> wasn't chosen back then. Of course you are the DT expert, but could
> you perhaps tell me why a child node is better for cases like this?

If there is a control path hierarchy, then we try to model that in DT 
with child nodes. In cases of SDIO and USB, there is a clear hierarchy. 
Ignoring the discovery ordering problem, we already have defined ways to 
describe GPIO connections, regulators, etc. to devices. Describing those 
things separately from the device to solve a particular issue that is 
really a kernel limitation is what I don't like.

Rob

[toc] | [prev] | [next] | [standalone]

#1397987

+ Arnd

[...]

>> >> Solution
>> >> ========
>> >> This is very similar to the MMC pwrseq behavior so the idea is to:
>> >> 1. Move MMC pwrseq drivers to generic place,
>> >
>> > You can do that, but I'm going to NAK any use of pwrseq bindings outside
>> > of MMC. I think it is the wrong way to do things. The DT should describe
>>
>> Huh, I didn't know that was your view of the mmc pwrseq bindings. Why
>> didn't you NAK them before?
>
> Unfortunately, either I missed it or it was a time I couldn't spend much
> time on reviews.

Okay, I guess it's common issue among maintainers. The problem with DT
is that it gets really hard to be fixed up later. :-)

>
>> > the devices. If they happen to be "simple" then the core can walk the
>> > tree and do any setup. For example, look for "reset-gpios" and toggle
>> > that GPIO. There is no need for a special node.
>> >
>> >> 2. Extend the pwrseq-simple with regulator toggling,
>> >> 3. Add support to USB hub and port core for pwrseq,
>> >
>> > We discussed this for USB already[1] and is why we defined how to add
>> > USB child devices. The idea is not to add pwrseq to that.
>>
>> I am not familiar with the USB discussion.
>>
>> Still, let me give you some more background to the mmc pwrseq. The
>> idea from the mmc pwrseq bindings comes from the power-domain DT
>> bindings, as I thought these things were a bit related.
>> In both cases they are not directly a property of the device, but more
>> describing a HW dependency to allow the device to work.
>
> I could see this as a board level power domain. However the difference
> is we are not generally exposing internal SOC details the same way as
> board level components. Perhaps we could extend power domains to board
> level, but that is not what was done here.
>
>> One could probably use a child node instead of a phandle, but that
>> wasn't chosen back then. Of course you are the DT expert, but could
>> you perhaps tell me why a child node is better for cases like this?
>
> If there is a control path hierarchy, then we try to model that in DT
> with child nodes. In cases of SDIO and USB, there is a clear hierarchy.
> Ignoring the discovery ordering problem, we already have defined ways to
> describe GPIO connections, regulators, etc. to devices. Describing those
> things separately from the device to solve a particular issue that is
> really a kernel limitation is what I don't like.

Okay, I see.

To move forward in trying to make mmc pwrseq a generic pwrseq, could
we perhaps allow both cases?

In the mmc case, there are already deployed bindings so we need to
cope with these by using the phandle option, but for USB etc we could
force the child node option.
As long as we agree that we keep using a compatible string for the
child node as well, both options should be able to co-exist and we
should probably be able to managed them both from a common pwrseq
driver framework.

Although, I do remember from an older conversations around some of
mine submission for the mmc pwrseq code, that some people (maybe
Arnd?) wasn't keen on adding a new framework for this. Perhaps that
has changed?

Kind regards
Uffe

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web