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

Groups > linux.kernel > #1532566 > unrolled thread

Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip

Started byJavier Martinez Canillas <javier@dowhile0.org>
First post2016-11-29 18:20 +0100
Last post2016-11-30 15:50 +0100
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip Javier Martinez Canillas <javier@dowhile0.org> - 2016-11-29 18:20 +0100
    Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip Matt Ranostay <matt@ranostay.consulting> - 2016-11-30 02:30 +0100
      Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip Javier Martinez Canillas <javier@dowhile0.org> - 2016-11-30 14:20 +0100
        Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip Ulf Hansson <ulf.hansson@linaro.org> - 2016-11-30 15:50 +0100

#1532566 — Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip

FromJavier Martinez Canillas <javier@dowhile0.org>
Date2016-11-29 18:20 +0100
SubjectRe: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip
Message-ID<sIUeu-2Xs-61@gated-at.bofh.it>
Hello Matt,

On Thu, Nov 17, 2016 at 10:55 PM, Matt Ranostay
<matt@ranostay.consulting> 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 <tony@atomide.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  .../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

[toc] | [next] | [standalone]

#1532891

FromMatt Ranostay <matt@ranostay.consulting>
Date2016-11-30 02:30 +0100
Message-ID<sJ1SF-7Ky-5@gated-at.bofh.it>
In reply to#1532566
On Tue, Nov 29, 2016 at 9:13 AM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> Hello Matt,
>
> On Thu, Nov 17, 2016 at 10:55 PM, Matt Ranostay
> <matt@ranostay.consulting> 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 <tony@atomide.com>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> ---
>>  .../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

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

#1533288

FromJavier Martinez Canillas <javier@dowhile0.org>
Date2016-11-30 14:20 +0100
Message-ID<sJcXL-6zu-15@gated-at.bofh.it>
In reply to#1532891
Hello Matt,

On Tue, Nov 29, 2016 at 10:20 PM, Matt Ranostay
<matt@ranostay.consulting> 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

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

#1533357

FromUlf Hansson <ulf.hansson@linaro.org>
Date2016-11-30 15:50 +0100
Message-ID<sJemS-7mo-11@gated-at.bofh.it>
In reply to#1533288
On 30 November 2016 at 14:11, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> Hello Matt,
>
> On Tue, Nov 29, 2016 at 10:20 PM, Matt Ranostay
> <matt@ranostay.consulting> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web