Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
This can be abstracted to other chipsets if needed in the future.
Cc: Tony Lindgren <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Matt Ranostay <[email protected]>
---
.../devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt | 14 +++
.../bindings/net/wireless/marvell-sd8xxx.txt | 4 +
drivers/mmc/core/Kconfig | 10 ++
drivers/mmc/core/Makefile | 1 +
drivers/mmc/core/pwrseq_sd8787.c | 117 +++++++++++++++++++++
5 files changed, 146 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
new file mode 100644
index 000000000000..1b658351629b
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
@@ -0,0 +1,14 @@
+* Marvell SD8787 power sequence provider
+
+Required properties:
+- compatible: must be "mmc-pwrseq-sd8787".
+- pwndn-gpio: contains a power down GPIO specifier.
+- reset-gpio: contains a reset GPIO specifier.
+
+Example:
+
+ wifi_pwrseq: wifi_pwrseq {
+ compatible = "mmc-pwrseq-sd8787";
+ pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
+ reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
+ }
diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
index c421aba0a5bc..08fd65d35725 100644
--- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
+++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
@@ -32,6 +32,9 @@ Optional properties:
so that the wifi chip can wakeup host platform under certain condition.
during system resume, the irq will be disabled to make sure
unnecessary interrupt is not received.
+ - vmmc-supply: a phandle of a regulator, supplying VCC to the card
+ - mmc-pwrseq: phandle to the MMC power sequence node. See "mmc-pwrseq-*"
+ for documentation of MMC power sequence bindings.
Example:
@@ -44,6 +47,7 @@ so that firmware can wakeup host using this device side pin.
&mmc3 {
status = "okay";
vmmc-supply = <&wlan_en_reg>;
+ mmc-pwrseq = <&wifi_pwrseq>;
bus-width = <4>;
cap-power-off-card;
keep-power-in-suspend;
diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
index 250f223aaa80..cf61d676ac06 100644
--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -12,6 +12,16 @@ config PWRSEQ_EMMC
This driver can also be built as a module. If so, the module
will be called pwrseq_emmc.
+config PWRSEQ_SD8787
+ tristate "HW reset support for SD8787 BT + Wifi module"
+ depends on OF && (MWIFIEX || BT_MRVL_SDIO)
+ help
+ This selects hardware reset support for the SD8787 BT + Wifi
+ module. By default this option is set to n.
+
+ This driver can also be built as a module. If so, the module
+ will be called pwrseq_sd8787.
+
config PWRSEQ_SIMPLE
tristate "Simple HW reset support for MMC"
default y
diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
index f007151dfdc6..f1c060e56d4e 100644
--- a/drivers/mmc/core/Makefile
+++ b/drivers/mmc/core/Makefile
@@ -10,5 +10,6 @@ mmc_core-y := core.o bus.o host.o \
quirks.o slot-gpio.o
mmc_core-$(CONFIG_OF) += pwrseq.o
obj-$(CONFIG_PWRSEQ_SIMPLE) += pwrseq_simple.o
+obj-$(CONFIG_PWRSEQ_SD8787) += pwrseq_sd8787.o
obj-$(CONFIG_PWRSEQ_EMMC) += pwrseq_emmc.o
mmc_core-$(CONFIG_DEBUG_FS) += debugfs.o
diff --git a/drivers/mmc/core/pwrseq_sd8787.c b/drivers/mmc/core/pwrseq_sd8787.c
new file mode 100644
index 000000000000..f4080fe6439e
--- /dev/null
+++ b/drivers/mmc/core/pwrseq_sd8787.c
@@ -0,0 +1,117 @@
+/*
+ * pwrseq_sd8787.c - power sequence support for Marvell SD8787 BT + Wifi chip
+ *
+ * Copyright (C) 2016 Matt Ranostay <[email protected]>
+ *
+ * Based on the original work pwrseq_simple.c
+ * Copyright (C) 2014 Linaro Ltd
+ * Author: Ulf Hansson <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+
+#include <linux/mmc/host.h>
+
+#include "pwrseq.h"
+
+struct mmc_pwrseq_sd8787 {
+ struct mmc_pwrseq pwrseq;
+ struct gpio_desc *reset_gpio;
+ struct gpio_desc *pwrdn_gpio;
+};
+
+#define to_pwrseq_sd8787(p) container_of(p, struct mmc_pwrseq_sd8787, pwrseq)
+
+static void mmc_pwrseq_sd8787_pre_power_on(struct mmc_host *host)
+{
+ struct mmc_pwrseq_sd8787 *pwrseq = to_pwrseq_sd8787(host->pwrseq);
+
+ gpiod_set_value_cansleep(pwrseq->reset_gpio, 1);
+
+ msleep(300);
+ gpiod_set_value_cansleep(pwrseq->pwrdn_gpio, 1);
+}
+
+static void mmc_pwrseq_sd8787_power_off(struct mmc_host *host)
+{
+ struct mmc_pwrseq_sd8787 *pwrseq = to_pwrseq_sd8787(host->pwrseq);
+
+ gpiod_set_value_cansleep(pwrseq->pwrdn_gpio, 0);
+ gpiod_set_value_cansleep(pwrseq->reset_gpio, 0);
+}
+
+static const struct mmc_pwrseq_ops mmc_pwrseq_sd8787_ops = {
+ .pre_power_on = mmc_pwrseq_sd8787_pre_power_on,
+ .power_off = mmc_pwrseq_sd8787_power_off,
+};
+
+static const struct of_device_id mmc_pwrseq_sd8787_of_match[] = {
+ { .compatible = "mmc-pwrseq-sd8787",},
+ {/* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, mmc_pwrseq_sd8787_of_match);
+
+static int mmc_pwrseq_sd8787_probe(struct platform_device *pdev)
+{
+ struct mmc_pwrseq_sd8787 *pwrseq;
+ struct device *dev = &pdev->dev;
+
+ pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
+ if (!pwrseq)
+ return -ENOMEM;
+
+ pwrseq->pwrdn_gpio = devm_gpiod_get(dev, "pwrdn", GPIOD_OUT_LOW);
+ if (IS_ERR(pwrseq->pwrdn_gpio))
+ return PTR_ERR(pwrseq->pwrdn_gpio);
+
+ pwrseq->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(pwrseq->reset_gpio))
+ return PTR_ERR(pwrseq->reset_gpio);
+
+ pwrseq->pwrseq.dev = dev;
+ pwrseq->pwrseq.ops = &mmc_pwrseq_sd8787_ops;
+ pwrseq->pwrseq.owner = THIS_MODULE;
+ platform_set_drvdata(pdev, pwrseq);
+
+ return mmc_pwrseq_register(&pwrseq->pwrseq);
+}
+
+static int mmc_pwrseq_sd8787_remove(struct platform_device *pdev)
+{
+ struct mmc_pwrseq_sd8787 *pwrseq = platform_get_drvdata(pdev);
+
+ mmc_pwrseq_unregister(&pwrseq->pwrseq);
+
+ return 0;
+}
+
+static struct platform_driver mmc_pwrseq_sd8787_driver = {
+ .probe = mmc_pwrseq_sd8787_probe,
+ .remove = mmc_pwrseq_sd8787_remove,
+ .driver = {
+ .name = "pwrseq_sd8787",
+ .of_match_table = mmc_pwrseq_sd8787_of_match,
+ },
+};
+
+module_platform_driver(mmc_pwrseq_sd8787_driver);
+MODULE_LICENSE("GPL v2");
--
2.7.4
[...]
>> +
>> +Example:
>> +
>> + wifi_pwrseq: wifi_pwrseq {
>> + compatible = "mmc-pwrseq-sd8787";
>> + pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
>> + reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
>> + }
>> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> index c421aba0a5bc..08fd65d35725 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> @@ -32,6 +32,9 @@ Optional properties:
>> so that the wifi chip can wakeup host platform under certain condition.
>> during system resume, the irq will be disabled to make sure
>> unnecessary interrupt is not received.
>> + - vmmc-supply: a phandle of a regulator, supplying VCC to the card
>
> This is why pwrseq is wrong. You have some properties in the card node
> and some in pwrseq node. Everything should be in the card node.
Put "all" in the card node, just doesn't work for MMC. Particular in
cases when we have removable cards, as then it would be wrong to have
a card node.
The mmc pwrseq DT bindings just follows the legacy approach for MMC
and that's why the pwrseq handle is at the controller node. Yes, would
could have done it differently, but this is the case now, so we will
have to accept that.
[...]
Kind regards
Uffe
Hello Matt,
On Thu, Nov 17, 2016 at 10:55 PM, Matt Ranostay
<[email protected]> wrote:
> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
> This can be abstracted to other chipsets if needed in the future.
>
> Cc: Tony Lindgren <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Matt Ranostay <[email protected]>
> ---
> .../devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt | 14 +++
> .../bindings/net/wireless/marvell-sd8xxx.txt | 4 +
> drivers/mmc/core/Kconfig | 10 ++
> drivers/mmc/core/Makefile | 1 +
> drivers/mmc/core/pwrseq_sd8787.c | 117 +++++++++++++++++++++
> 5 files changed, 146 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
> create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
According Documentation/devicetree/bindings/submitting-patches.txt,
the DT bindings patches should posted as a separate patch.
> new file mode 100644
> index 000000000000..1b658351629b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
> @@ -0,0 +1,14 @@
> +* Marvell SD8787 power sequence provider
> +
> +Required properties:
> +- compatible: must be "mmc-pwrseq-sd8787".
Since this is not a generic binding, the compatible string should have
a vendor prefix.
> +- pwndn-gpio: contains a power down GPIO specifier.
> +- reset-gpio: contains a reset GPIO specifier.
> +
I wonder if we really need a custom power sequence provider for just
this SDIO WiFI chip though. AFAICT the only missing piece in
mmc-pwrseq-simple is the power down GPIO property, so maybe
mmc-pwrseq-simple could be extended instead to have an optional
powerdown-gpios property and instead in the Marvell SD8787 DT binding
can be mentioned which mmc-pwrseq-simple properties are required for
the device.
> +Example:
> +
> + wifi_pwrseq: wifi_pwrseq {
> + compatible = "mmc-pwrseq-sd8787";
> + pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
> + reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
> + }
> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
Does this patch depend on a previous posted series? I don't see this
file in today's linux-next...
> index c421aba0a5bc..08fd65d35725 100644
> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> @@ -32,6 +32,9 @@ Optional properties:
> so that the wifi chip can wakeup host platform under certain condition.
> during system resume, the irq will be disabled to make sure
> unnecessary interrupt is not received.
> + - vmmc-supply: a phandle of a regulator, supplying VCC to the card
> + - mmc-pwrseq: phandle to the MMC power sequence node. See "mmc-pwrseq-*"
> + for documentation of MMC power sequence bindings.
>
> Example:
>
> @@ -44,6 +47,7 @@ so that firmware can wakeup host using this device side pin.
> &mmc3 {
> status = "okay";
> vmmc-supply = <&wlan_en_reg>;
> + mmc-pwrseq = <&wifi_pwrseq>;
> bus-width = <4>;
> cap-power-off-card;
> keep-power-in-suspend;
I think this change should be split in a separate patch as well.
Best regards,
Javier
On Tue, Nov 29, 2016 at 9:13 AM, Javier Martinez Canillas
<[email protected]> wrote:
> Hello Matt,
>
> On Thu, Nov 17, 2016 at 10:55 PM, Matt Ranostay
> <[email protected]> wrote:
>> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
>> This can be abstracted to other chipsets if needed in the future.
>>
>> Cc: Tony Lindgren <[email protected]>
>> Cc: Ulf Hansson <[email protected]>
>> Cc: Mark Rutland <[email protected]>
>> Cc: Srinivas Kandagatla <[email protected]>
>> Signed-off-by: Matt Ranostay <[email protected]>
>> ---
>> .../devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt | 14 +++
>> .../bindings/net/wireless/marvell-sd8xxx.txt | 4 +
>> drivers/mmc/core/Kconfig | 10 ++
>> drivers/mmc/core/Makefile | 1 +
>> drivers/mmc/core/pwrseq_sd8787.c | 117 +++++++++++++++++++++
>> 5 files changed, 146 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
>> create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
>
> According Documentation/devicetree/bindings/submitting-patches.txt,
> the DT bindings patches should posted as a separate patch.
Ok will do.
>
>> new file mode 100644
>> index 000000000000..1b658351629b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
>> @@ -0,0 +1,14 @@
>> +* Marvell SD8787 power sequence provider
>> +
>> +Required properties:
>> +- compatible: must be "mmc-pwrseq-sd8787".
>
> Since this is not a generic binding, the compatible string should have
> a vendor prefix.
>
Makes sense to me.
>> +- pwndn-gpio: contains a power down GPIO specifier.
>> +- reset-gpio: contains a reset GPIO specifier.
>> +
>
> I wonder if we really need a custom power sequence provider for just
> this SDIO WiFI chip though. AFAICT the only missing piece in
> mmc-pwrseq-simple is the power down GPIO property, so maybe
> mmc-pwrseq-simple could be extended instead to have an optional
> powerdown-gpios property and instead in the Marvell SD8787 DT binding
> can be mentioned which mmc-pwrseq-simple properties are required for
> the device.
>
The reason we didn't do that is we need delay between the two
assertions/desertions of GPIOs. It wouldn't seems good practice to
hack the pwrseq-simple for this...
>> +Example:
>> +
>> + wifi_pwrseq: wifi_pwrseq {
>> + compatible = "mmc-pwrseq-sd8787";
>> + pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
>> + reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
>> + }
>> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>
> Does this patch depend on a previous posted series? I don't see this
> file in today's linux-next...
Got renamed to ->
Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt it
seems very recently.
>
>> index c421aba0a5bc..08fd65d35725 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> @@ -32,6 +32,9 @@ Optional properties:
>> so that the wifi chip can wakeup host platform under certain condition.
>> during system resume, the irq will be disabled to make sure
>> unnecessary interrupt is not received.
>> + - vmmc-supply: a phandle of a regulator, supplying VCC to the card
>> + - mmc-pwrseq: phandle to the MMC power sequence node. See "mmc-pwrseq-*"
>> + for documentation of MMC power sequence bindings.
>>
>> Example:
>>
>> @@ -44,6 +47,7 @@ so that firmware can wakeup host using this device side pin.
>> &mmc3 {
>> status = "okay";
>> vmmc-supply = <&wlan_en_reg>;
>> + mmc-pwrseq = <&wifi_pwrseq>;
>> bus-width = <4>;
>> cap-power-off-card;
>> keep-power-in-suspend;
>
> I think this change should be split in a separate patch as well.
>
> Best regards,
> Javier
On Thu, Nov 17, 2016 at 05:55:09PM -0800, Matt Ranostay wrote:
> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
> This can be abstracted to other chipsets if needed in the future.
I don't like the MMC pwrseq bindings, so I won't be acking any. Look at
the USB pwrseq for why.
> Cc: Tony Lindgren <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Matt Ranostay <[email protected]>
> ---
> .../devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt | 14 +++
> .../bindings/net/wireless/marvell-sd8xxx.txt | 4 +
> drivers/mmc/core/Kconfig | 10 ++
> drivers/mmc/core/Makefile | 1 +
> drivers/mmc/core/pwrseq_sd8787.c | 117 +++++++++++++++++++++
> 5 files changed, 146 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
> create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
> new file mode 100644
> index 000000000000..1b658351629b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
> @@ -0,0 +1,14 @@
> +* Marvell SD8787 power sequence provider
> +
> +Required properties:
> +- compatible: must be "mmc-pwrseq-sd8787".
> +- pwndn-gpio: contains a power down GPIO specifier.
powerdown-gpios
> +- reset-gpio: contains a reset GPIO specifier.
reset-gpios
> +
> +Example:
> +
> + wifi_pwrseq: wifi_pwrseq {
> + compatible = "mmc-pwrseq-sd8787";
> + pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
> + reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
> + }
> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> index c421aba0a5bc..08fd65d35725 100644
> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> @@ -32,6 +32,9 @@ Optional properties:
> so that the wifi chip can wakeup host platform under certain condition.
> during system resume, the irq will be disabled to make sure
> unnecessary interrupt is not received.
> + - vmmc-supply: a phandle of a regulator, supplying VCC to the card
This is why pwrseq is wrong. You have some properties in the card node
and some in pwrseq node. Everything should be in the card node.
> + - mmc-pwrseq: phandle to the MMC power sequence node. See "mmc-pwrseq-*"
> + for documentation of MMC power sequence bindings.
>
> Example:
>
> @@ -44,6 +47,7 @@ so that firmware can wakeup host using this device side pin.
> &mmc3 {
> status = "okay";
> vmmc-supply = <&wlan_en_reg>;
> + mmc-pwrseq = <&wifi_pwrseq>;
> bus-width = <4>;
> cap-power-off-card;
> keep-power-in-suspend;
* Matt Ranostay <[email protected]> [161117 17:55]:
> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
> This can be abstracted to other chipsets if needed in the future.
Good to see this happening:
Acked-by: Tony Lindgren <[email protected]>
Hello Matt,
On Tue, Nov 29, 2016 at 10:20 PM, Matt Ranostay
<[email protected]> wrote:
> On Tue, Nov 29, 2016 at 9:13 AM, Javier Martinez Canillas
[snip]
>
>
>>> +- pwndn-gpio: contains a power down GPIO specifier.
>>> +- reset-gpio: contains a reset GPIO specifier.
>>> +
>>
>> I wonder if we really need a custom power sequence provider for just
>> this SDIO WiFI chip though. AFAICT the only missing piece in
>> mmc-pwrseq-simple is the power down GPIO property, so maybe
>> mmc-pwrseq-simple could be extended instead to have an optional
>> powerdown-gpios property and instead in the Marvell SD8787 DT binding
>> can be mentioned which mmc-pwrseq-simple properties are required for
>> the device.
>>
>
> The reason we didn't do that is we need delay between the two
> assertions/desertions of GPIOs. It wouldn't seems good practice to
> hack the pwrseq-simple for this...
>
Yes, I noticed that. I wouldn't say that it would be a hack for the
pwrseq-simple since it already has a "post-power-on-delay-ms" DT
property, so AFAICT it would just be adding a "pre-power-on-delay-ms"
property for your use case.
It would also be more consistent since it would support a delay for
pre and post power callbacks. It would also make you avoid hardcoding
the 300 msec wait, in case other device has a similar need but with a
different wait time.
In summary, I think that devices having a power (or power down) and
enable GPIO, and needing to wait between the GPIO toggling are common.
So I would prefer to make pwrseq-simple usable for these instead of
adding device specific power sequence providers. But it's just my
opinion and not my call :-)
>>> +Example:
>>> +
>>> + wifi_pwrseq: wifi_pwrseq {
>>> + compatible = "mmc-pwrseq-sd8787";
>>> + pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
>>> + reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
>>> + }
>>> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>
>> Does this patch depend on a previous posted series? I don't see this
>> file in today's linux-next...
>
> Got renamed to ->
> Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt it
> seems very recently.
>
I see, that's what I thought but I wasn't able to find the commit /
patch that did it.
>>
>>> index c421aba0a5bc..08fd65d35725 100644
>>> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>> @@ -32,6 +32,9 @@ Optional properties:
>>> so that the wifi chip can wakeup host platform under certain condition.
>>> during system resume, the irq will be disabled to make sure
>>> unnecessary interrupt is not received.
>>> + - vmmc-supply: a phandle of a regulator, supplying VCC to the card
>>> + - mmc-pwrseq: phandle to the MMC power sequence node. See "mmc-pwrseq-*"
>>> + for documentation of MMC power sequence bindings.
>>>
>>> Example:
>>>
>>> @@ -44,6 +47,7 @@ so that firmware can wakeup host using this device side pin.
>>> &mmc3 {
>>> status = "okay";
>>> vmmc-supply = <&wlan_en_reg>;
>>> + mmc-pwrseq = <&wifi_pwrseq>;
>>> bus-width = <4>;
>>> cap-power-off-card;
>>> keep-power-in-suspend;
>>
>> I think this change should be split in a separate patch as well.
>>
You didn't answer about this, but I guess you agree it should be in a
separate patch.
Best regards,
Javier
On Mon, Nov 28, 2016 at 9:54 AM, Ulf Hansson <[email protected]> wrote:
> [...]
>
>>> +
>>> +Example:
>>> +
>>> + wifi_pwrseq: wifi_pwrseq {
>>> + compatible = "mmc-pwrseq-sd8787";
>>> + pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
>>> + reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
>>> + }
>>> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>> index c421aba0a5bc..08fd65d35725 100644
>>> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>> @@ -32,6 +32,9 @@ Optional properties:
>>> so that the wifi chip can wakeup host platform under certain condition.
>>> during system resume, the irq will be disabled to make sure
>>> unnecessary interrupt is not received.
>>> + - vmmc-supply: a phandle of a regulator, supplying VCC to the card
>>
>> This is why pwrseq is wrong. You have some properties in the card node
>> and some in pwrseq node. Everything should be in the card node.
>
> Put "all" in the card node, just doesn't work for MMC. Particular in
> cases when we have removable cards, as then it would be wrong to have
> a card node.
When is there a problem with removable cards? The connector is
standard and everything needed (CD, VMMC, VDDIO, etc.) is defined in
the host controller node. If that isn't sufficient, then we should
start defining a connector node.
> The mmc pwrseq DT bindings just follows the legacy approach for MMC
> and that's why the pwrseq handle is at the controller node. Yes, would
> could have done it differently, but this is the case now, so we will
> have to accept that.
We're stuck with supporting the existing cases. That doesn't mean
we're stuck with the same thing for new cases.
Rob
On 30 November 2016 at 14:11, Javier Martinez Canillas
<[email protected]> wrote:
> Hello Matt,
>
> On Tue, Nov 29, 2016 at 10:20 PM, Matt Ranostay
> <[email protected]> wrote:
>> On Tue, Nov 29, 2016 at 9:13 AM, Javier Martinez Canillas
>
> [snip]
>
>>
>>
>>>> +- pwndn-gpio: contains a power down GPIO specifier.
>>>> +- reset-gpio: contains a reset GPIO specifier.
>>>> +
>>>
>>> I wonder if we really need a custom power sequence provider for just
>>> this SDIO WiFI chip though. AFAICT the only missing piece in
>>> mmc-pwrseq-simple is the power down GPIO property, so maybe
>>> mmc-pwrseq-simple could be extended instead to have an optional
>>> powerdown-gpios property and instead in the Marvell SD8787 DT binding
>>> can be mentioned which mmc-pwrseq-simple properties are required for
>>> the device.
>>>
>>
>> The reason we didn't do that is we need delay between the two
>> assertions/desertions of GPIOs. It wouldn't seems good practice to
>> hack the pwrseq-simple for this...
>>
>
> Yes, I noticed that. I wouldn't say that it would be a hack for the
> pwrseq-simple since it already has a "post-power-on-delay-ms" DT
> property, so AFAICT it would just be adding a "pre-power-on-delay-ms"
> property for your use case.
>
> It would also be more consistent since it would support a delay for
> pre and post power callbacks. It would also make you avoid hardcoding
> the 300 msec wait, in case other device has a similar need but with a
> different wait time.
>
> In summary, I think that devices having a power (or power down) and
> enable GPIO, and needing to wait between the GPIO toggling are common.
> So I would prefer to make pwrseq-simple usable for these instead of
> adding device specific power sequence providers. But it's just my
> opinion and not my call :-)
This is a good idea. Please try out this approach.
[...]
Kind regards
Uffe
On 29 November 2016 at 15:51, Rob Herring <[email protected]> wrote:
> On Mon, Nov 28, 2016 at 9:54 AM, Ulf Hansson <[email protected]> wrote:
>> [...]
>>
>>>> +
>>>> +Example:
>>>> +
>>>> + wifi_pwrseq: wifi_pwrseq {
>>>> + compatible = "mmc-pwrseq-sd8787";
>>>> + pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
>>>> + reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
>>>> + }
>>>> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>>> index c421aba0a5bc..08fd65d35725 100644
>>>> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>>> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>>> @@ -32,6 +32,9 @@ Optional properties:
>>>> so that the wifi chip can wakeup host platform under certain condition.
>>>> during system resume, the irq will be disabled to make sure
>>>> unnecessary interrupt is not received.
>>>> + - vmmc-supply: a phandle of a regulator, supplying VCC to the card
>>>
>>> This is why pwrseq is wrong. You have some properties in the card node
>>> and some in pwrseq node. Everything should be in the card node.
>>
>> Put "all" in the card node, just doesn't work for MMC. Particular in
>> cases when we have removable cards, as then it would be wrong to have
>> a card node.
>
> When is there a problem with removable cards? The connector is
> standard and everything needed (CD, VMMC, VDDIO, etc.) is defined in
> the host controller node. If that isn't sufficient, then we should
> start defining a connector node.
I probably didn't get your point. Anyway, let's try again.
I don't see any problems with removable cards and neither with
non-removable cards.
Normally, we don't need a connector node, but instead we put the
related properties/phandles in the host controller node. This works
fine!
For SDIO func devices, sometimes we need a child node of the host
controller node, as to be able to describe certain characteristics of
the SDIO func device. So this case is kind of a special type of card
node (because one can have several SDIO func devices attached, even if
this isn't used).
Now, to follow the current bindings, we must not put connector related
bindings in the card node, like the vmmc-supply, perhaps that is what
causes confusion here?
>
>> The mmc pwrseq DT bindings just follows the legacy approach for MMC
>> and that's why the pwrseq handle is at the controller node. Yes, would
>> could have done it differently, but this is the case now, so we will
>> have to accept that.
>
> We're stuck with supporting the existing cases. That doesn't mean
> we're stuck with the same thing for new cases.
Agree!
But I think there is nothing to improve in this case, or am I wrong?
Kind regards
Uffe