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

Groups > linux.kernel > #1531343 > unrolled thread

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

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 Rob Herring <robh@kernel.org> - 2016-11-28 15:20 +0100
    Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip Ulf Hansson <ulf.hansson@linaro.org> - 2016-11-28 17:00 +0100
      Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip Rob Herring <robh@kernel.org> - 2016-11-29 16:00 +0100
        Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip Ulf Hansson <ulf.hansson@linaro.org> - 2016-11-29 18:00 +0100

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

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 <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
> 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;

[toc] | [next] | [standalone]

#1531410

[...]

>> +
>> +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

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

#1532374

On Mon, Nov 28, 2016 at 9:54 AM, Ulf Hansson <ulf.hansson@linaro.org> 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

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

#1532528

On 29 November 2016 at 15:51, Rob Herring <robh@kernel.org> wrote:
> On Mon, Nov 28, 2016 at 9:54 AM, Ulf Hansson <ulf.hansson@linaro.org> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web