diff mbox

[1/3] mmc: add support for power-on sequencing through DT

Message ID 1390190215-22700-2-git-send-email-olof@lixom.net (mailing list archive)
State New, archived
Headers show

Commit Message

Olof Johansson Jan. 20, 2014, 3:56 a.m. UTC
This patch enables support for power-on sequencing of SDIO peripherals through DT.

In general, it's quite common that wifi modules and other similar
peripherals have several signals in addition to the SDIO interface that
needs wiggling before the module will power on. It's common to have a
reference clock, one or several power rails and one or several lines
for reset/enable type functions.

The binding as written today introduces a number of reset gpios,
a regulator and a clock specifier. The code will handle up to 2 gpio
reset lines, but it's trivial to increase to more than that if needed
at some point.

Implementation-wise, the MMC core has been changed to handle this during
host power up, before the host interface is powered on. I have not yet
implemented the power-down side, I wanted people to have a chance for
reporting back w.r.t. issues (or comments on the bindings) first.

I have not tested the regulator portion, since the system and module
I'm working on doesn't need one (Samsung Chromebook with Marvell
8797-based wifi). Testing of those portions (and reporting back) would
be appreciated.

Signed-off-by: Olof Johansson <olof@lixom.net>
---
 Documentation/devicetree/bindings/mmc/mmc.txt |   11 +++++++
 drivers/mmc/core/core.c                       |   42 +++++++++++++++++++++++++
 drivers/mmc/core/host.c                       |   30 +++++++++++++++++-
 include/linux/mmc/host.h                      |    5 +++
 4 files changed, 87 insertions(+), 1 deletion(-)

Comments

Ulf Hansson Jan. 20, 2014, 8:44 a.m. UTC | #1
On 20 January 2014 04:56, Olof Johansson <olof@lixom.net> wrote:
> This patch enables support for power-on sequencing of SDIO peripherals through DT.
>
> In general, it's quite common that wifi modules and other similar
> peripherals have several signals in addition to the SDIO interface that
> needs wiggling before the module will power on. It's common to have a
> reference clock, one or several power rails and one or several lines
> for reset/enable type functions.
>
> The binding as written today introduces a number of reset gpios,
> a regulator and a clock specifier. The code will handle up to 2 gpio
> reset lines, but it's trivial to increase to more than that if needed
> at some point.
>
> Implementation-wise, the MMC core has been changed to handle this during
> host power up, before the host interface is powered on. I have not yet
> implemented the power-down side, I wanted people to have a chance for
> reporting back w.r.t. issues (or comments on the bindings) first.
>
> I have not tested the regulator portion, since the system and module
> I'm working on doesn't need one (Samsung Chromebook with Marvell
> 8797-based wifi). Testing of those portions (and reporting back) would
> be appreciated.
>
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
>  Documentation/devicetree/bindings/mmc/mmc.txt |   11 +++++++
>  drivers/mmc/core/core.c                       |   42 +++++++++++++++++++++++++
>  drivers/mmc/core/host.c                       |   30 +++++++++++++++++-
>  include/linux/mmc/host.h                      |    5 +++
>  4 files changed, 87 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> index 458b57f..962e0ee 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> @@ -5,6 +5,8 @@ these definitions.
>  Interpreted by the OF core:
>  - reg: Registers location and length.
>  - interrupts: Interrupts used by the MMC controller.
> +- clocks: Clocks needed for the host controller, if any.
> +- clock-names: Goes with clocks above.
>
>  Card detection:
>  If no property below is supplied, host native card detect is used.
> @@ -30,6 +32,15 @@ Optional properties:
>  - cap-sdio-irq: enable SDIO IRQ signalling on this interface
>  - full-pwr-cycle: full power cycle of the card is supported
>
> +Card power and reset control:
> +The following properties can be specified for cases where the MMC
> +peripheral needs additional reset, regulator and clock lines. It is for
> +example common for WiFi/BT adapters to have these separate from the main
> +MMC bus:
> +  - card-reset-gpios: Specify GPIOs for card reset (reset active low)
> +  - card-external-vcc-supply: Regulator to drive (independent) card VCC
> +  - clock with name "card_ext_clock": External clock provided to the card
> +
>  *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
>  polarity properties, we have to fix the meaning of the "normal" and "inverted"
>  line levels. We choose to follow the SDHCI standard, which specifies both those
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 098374b..c43e6c8 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -13,11 +13,13 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> +#include <linux/clk.h>
>  #include <linux/completion.h>
>  #include <linux/device.h>
>  #include <linux/delay.h>
>  #include <linux/pagemap.h>
>  #include <linux/err.h>
> +#include <linux/gpio.h>
>  #include <linux/leds.h>
>  #include <linux/scatterlist.h>
>  #include <linux/log2.h>
> @@ -1519,6 +1521,43 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type)
>         mmc_host_clk_release(host);
>  }
>
> +static void mmc_card_power_up(struct mmc_host *host)
> +{
> +       int i;
> +       struct gpio_desc **gds = host->card_reset_gpios;
> +
> +       for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
> +               if (gds[i]) {
> +                       dev_dbg(host->parent, "Asserting reset line %d", i);
> +                       gpiod_set_value(gds[i], 1);
> +               }
> +       }
> +
> +       if (host->card_regulator) {
> +               dev_dbg(host->parent, "Enabling external regulator");
> +               if (regulator_enable(host->card_regulator))
> +                       dev_err(host->parent, "Failed to enable external regulator");
> +       }
> +
> +       if (host->card_clk) {
> +               dev_dbg(host->parent, "Enabling external clock");
> +               clk_prepare_enable(host->card_clk);
> +       }
> +
> +       /* 2ms delay to let clocks and power settle */
> +       mmc_delay(20);
> +
> +       for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
> +               if (gds[i]) {
> +                       dev_dbg(host->parent, "Deasserting reset line %d", i);
> +                       gpiod_set_value(gds[i], 0);
> +               }
> +       }
> +
> +       /* 2ms delay to after reset release */
> +       mmc_delay(20);
> +}
> +
>  /*
>   * Apply power to the MMC stack.  This is a two-stage process.
>   * First, we enable power to the card without the clock running.
> @@ -1535,6 +1574,9 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
>         if (host->ios.power_mode == MMC_POWER_ON)
>                 return;
>
> +       /* Power up the card/module first, if needed */
> +       mmc_card_power_up(host);
> +
>         mmc_host_clk_hold(host);
>
>         host->ios.vdd = fls(ocr) - 1;
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 49bc403..e6b850b 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -12,14 +12,18 @@
>   *  MMC host class device management
>   */
>
> +#include <linux/kernel.h>
> +#include <linux/clk.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/idr.h>
>  #include <linux/of.h>
>  #include <linux/of_gpio.h>
>  #include <linux/pagemap.h>
>  #include <linux/export.h>
>  #include <linux/leds.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
>
> @@ -312,7 +316,7 @@ int mmc_of_parse(struct mmc_host *host)
>         u32 bus_width;
>         bool explicit_inv_wp, gpio_inv_wp = false;
>         enum of_gpio_flags flags;
> -       int len, ret, gpio;
> +       int i, len, ret, gpio;
>
>         if (!host->parent || !host->parent->of_node)
>                 return 0;
> @@ -415,6 +419,30 @@ int mmc_of_parse(struct mmc_host *host)
>         if (explicit_inv_wp ^ gpio_inv_wp)
>                 host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
>
> +       /* Parse card power/reset/clock control */

I would like us to prevent to open up for confusion with the "eMMC hw
reset" when adding this. Unless we are able to combine them in some
way?

Could we maybe add some more comments about in what scenarios this DT
property would be useful?

> +       if (of_find_property(np, "card-reset-gpios", NULL)) {
> +               struct gpio_desc *gpd;
> +               for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
> +                       gpd = devm_gpiod_get_index(host->parent, "card-reset", i);
> +                       if (IS_ERR(gpd))
> +                               break;
> +                       gpiod_direction_output(gpd, 0);
> +                       host->card_reset_gpios[i] = gpd;
> +               }
> +
> +               gpd = devm_gpiod_get_index(host->parent, "card-reset", ARRAY_SIZE(host->card_reset_gpios));
> +               if (!IS_ERR(gpd)) {
> +                       dev_warn(host->parent, "More reset gpios than we can handle");
> +                       gpiod_put(gpd);
> +               }
> +       }
> +
> +       host->card_clk = of_clk_get_by_name(np, "card_ext_clock");

of_clk_get_by_name relies on COMMON_CLK, is that really what you want here?

> +       if (IS_ERR(host->card_clk))
> +               host->card_clk = NULL;
> +
> +       host->card_regulator = regulator_get(host->parent, "card-external-vcc");

Is the above regulator related to host->ocr_avail mask? Could the
above regulator be replaced by vmmc?

At the moment host drivers uses mmc_regulator_get_supply(), which
fetches regulators called "vmmc" and "vqmmc". It is also common to
have these defined in DT like "vmmc-supply". This has not been
properly documented for most host cases, and we should fix that. I
also think it would make sense to include these in the documentation
for the common mmc bindings, instead of host specific bindings.

Kind regards
Ulf Hansson

> +
>         if (of_find_property(np, "cap-sd-highspeed", &len))
>                 host->caps |= MMC_CAP_SD_HIGHSPEED;
>         if (of_find_property(np, "cap-mmc-highspeed", &len))
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 99f5709..6781887 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -297,6 +297,11 @@ struct mmc_host {
>         unsigned long           clkgate_delay;
>  #endif
>
> +       /* card specific properties to deal with power and reset */
> +       struct regulator        *card_regulator; /* External VCC needed by the card */
> +       struct gpio_desc        *card_reset_gpios[2]; /* External resets, active low */
> +       struct clk              *card_clk;      /* External clock needed by the card */
> +
>         /* host specific block data */
>         unsigned int            max_seg_size;   /* see blk_queue_max_segment_size */
>         unsigned short          max_segs;       /* see blk_queue_max_segments */
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Jan. 20, 2014, 4:36 p.m. UTC | #2
On Sun, Jan 19, 2014 at 07:56:53PM -0800, Olof Johansson wrote:

> +	if (host->card_regulator) {

NULL is a potentially valid regulator; use IS_ERR().  Also see below...

> +		dev_dbg(host->parent, "Enabling external regulator");
> +		if (regulator_enable(host->card_regulator))
> +			dev_err(host->parent, "Failed to enable external regulator");

You should really log the error code here.

> +	host->card_regulator = regulator_get(host->parent, "card-external-vcc");
> +

...this won't handle probe deferral or other errors.  Given what this is
for it should probably be using regulator_get_optional(), the driver is
happy to carry on if a supply isn't available.  On the other hand it
just enables and disables so it's probably easier to just ignore that
and let the stub regulator get used anyway.

I have to say I do worry what happens if the device has multiple
supplies that need to be managed (which is common enough, for example
analogue and digital supplies tend to be decoupled) or if the device can
do useful things with the supplies.  In the case of SDIO it's *probably*
less relevant though I have seen applications where it might be.
Russell King - ARM Linux Jan. 20, 2014, 4:48 p.m. UTC | #3
On Sun, Jan 19, 2014 at 07:56:53PM -0800, Olof Johansson wrote:
> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> index 458b57f..962e0ee 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> @@ -5,6 +5,8 @@ these definitions.
>  Interpreted by the OF core:
>  - reg: Registers location and length.
>  - interrupts: Interrupts used by the MMC controller.
> +- clocks: Clocks needed for the host controller, if any.
> +- clock-names: Goes with clocks above.
>  
>  Card detection:
>  If no property below is supplied, host native card detect is used.
> @@ -30,6 +32,15 @@ Optional properties:
>  - cap-sdio-irq: enable SDIO IRQ signalling on this interface
>  - full-pwr-cycle: full power cycle of the card is supported
>  
> +Card power and reset control:
> +The following properties can be specified for cases where the MMC
> +peripheral needs additional reset, regulator and clock lines. It is for
> +example common for WiFi/BT adapters to have these separate from the main
> +MMC bus:
> +  - card-reset-gpios: Specify GPIOs for card reset (reset active low)
> +  - card-external-vcc-supply: Regulator to drive (independent) card VCC
> +  - clock with name "card_ext_clock": External clock provided to the card
> +

This looks good.  I can connect the wifi/bt power control to a regulator,
and give that as the card-external-vcc-supply property.  I can specify
the WIFI/BT resets for card-reset-gpios.

So far so good.  Now, what about this external oscillator which has its
own separate power control.  My immediate thought is that this can be
specified via card_ext_clock - I would simply need to declare a fixed-rate
clock with either a regulator (power switch) controlled via a gpio (which
would probably be closer to the hardware) or a gpio as an enable... ah,
that requires me to write a common clock driver for that bit since this
is currently not modelled by CCF...
Fabio Estevam Jan. 20, 2014, 5:03 p.m. UTC | #4
On Mon, Jan 20, 2014 at 2:48 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> So far so good.  Now, what about this external oscillator which has its
> own separate power control.  My immediate thought is that this can be
> specified via card_ext_clock - I would simply need to declare a fixed-rate
> clock with either a regulator (power switch) controlled via a gpio (which
> would probably be closer to the hardware) or a gpio as an enable... ah,
> that requires me to write a common clock driver for that bit since this
> is currently not modelled by CCF...

Isn't this covered by the gpios property of fixed-clock?

We do the following to enable the 26MHz codec clock in
imx51-babbage.dts via GPIO4_26:

        clk_26M: codec_clock {
            compatible = "fixed-clock";
            reg=<0>;
            #clock-cells = <0>;
            clock-frequency = <26000000>;
            gpios = <&gpio4 26 GPIO_ACTIVE_LOW>;
        };

Regards,

Fabio Estevam
Russell King - ARM Linux Jan. 20, 2014, 5:16 p.m. UTC | #5
On Mon, Jan 20, 2014 at 03:03:50PM -0200, Fabio Estevam wrote:
> On Mon, Jan 20, 2014 at 2:48 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> 
> > So far so good.  Now, what about this external oscillator which has its
> > own separate power control.  My immediate thought is that this can be
> > specified via card_ext_clock - I would simply need to declare a fixed-rate
> > clock with either a regulator (power switch) controlled via a gpio (which
> > would probably be closer to the hardware) or a gpio as an enable... ah,
> > that requires me to write a common clock driver for that bit since this
> > is currently not modelled by CCF...
> 
> Isn't this covered by the gpios property of fixed-clock?
> 
> We do the following to enable the 26MHz codec clock in
> imx51-babbage.dts via GPIO4_26:
> 
>         clk_26M: codec_clock {
>             compatible = "fixed-clock";
>             reg=<0>;
>             #clock-cells = <0>;
>             clock-frequency = <26000000>;
>             gpios = <&gpio4 26 GPIO_ACTIVE_LOW>;
>         };

Not as far as I can see.  fixed-clock appears to have two properies:

	clock-frequency
	clock-output-names

and nothing else.  See of_fixed_clk_setup in drivers/clk/clk-fixed-rate.c.
You'll also find that the documentation in this file says this about it:

 * DOC: basic fixed-rate clock that cannot gate
 *
 * Traits of this clock:
 * prepare - clk_(un)prepare only ensures parents are prepared
 * enable - clk_enable only ensures parents are enabled
 * rate - rate is always a fixed value.  No clk_set_rate support
 * parent - fixed parent.  No clk_set_parent support

So, I think the bit which you quote from imx51-babbage.dts is wishful
thinking on the part of the author of the DT file, rather than actually
being implemented in any way by the kernel DT support.
Fabio Estevam Jan. 20, 2014, 6:47 p.m. UTC | #6
On Mon, Jan 20, 2014 at 3:16 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> Not as far as I can see.  fixed-clock appears to have two properies:
>
>         clock-frequency
>         clock-output-names
>
> and nothing else.  See of_fixed_clk_setup in drivers/clk/clk-fixed-rate.c.
> You'll also find that the documentation in this file says this about it:

Looks like Documentation/devicetree/bindings/clock/fixed-clock.txt is
misleading then:

"Optional properties:
- gpios : From common gpio binding; gpio connection to clock enable pin."

Regards,

Fabio Estevam
Arnd Bergmann Jan. 20, 2014, 6:58 p.m. UTC | #7
On Monday 20 January 2014, Olof Johansson wrote:
> 
> +Card power and reset control:
> +The following properties can be specified for cases where the MMC
> +peripheral needs additional reset, regulator and clock lines. It is for
> +example common for WiFi/BT adapters to have these separate from the main
> +MMC bus:
> +  - card-reset-gpios: Specify GPIOs for card reset (reset active low)
> +  - card-external-vcc-supply: Regulator to drive (independent) card VCC
> +  - clock with name "card_ext_clock": External clock provided to the card

I wonder whether the reset line should use the generic reset controller binding
rather than the gpio binding. There has been a recent discussion about a
gpio-reset driver, which should generalize this at the reset level, and
it would make it possible to use reset controllers that are not gpio driven.
In general, any gpio can be used as a reset, but not every reset line can
be a gpio. I don't know whether anyone would use an internal reset-controller
for an external SDIO chip though, so maybe your approach is sufficient.

	Arnd
Olof Johansson Jan. 20, 2014, 7:04 p.m. UTC | #8
On Mon, Jan 20, 2014 at 10:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 20 January 2014, Olof Johansson wrote:
>>
>> +Card power and reset control:
>> +The following properties can be specified for cases where the MMC
>> +peripheral needs additional reset, regulator and clock lines. It is for
>> +example common for WiFi/BT adapters to have these separate from the main
>> +MMC bus:
>> +  - card-reset-gpios: Specify GPIOs for card reset (reset active low)
>> +  - card-external-vcc-supply: Regulator to drive (independent) card VCC
>> +  - clock with name "card_ext_clock": External clock provided to the card
>
> I wonder whether the reset line should use the generic reset controller binding
> rather than the gpio binding. There has been a recent discussion about a
> gpio-reset driver, which should generalize this at the reset level, and
> it would make it possible to use reset controllers that are not gpio driven.
> In general, any gpio can be used as a reset, but not every reset line can
> be a gpio. I don't know whether anyone would use an internal reset-controller
> for an external SDIO chip though, so maybe your approach is sufficient.

I'd expect most usage of this to be through gpios, since we're talking
about external independent modules here. I would prefer not to bring
in the reset controller stuff here -- it just adds another layer of
abstraction that seems unnecessary.

I would prefer if the reset framework was contained to only be the
on-chip SoC IP block resets, etc, as originally intended.


-Olof
Arnd Bergmann Jan. 20, 2014, 7:12 p.m. UTC | #9
On Monday 20 January 2014 11:04:26 Olof Johansson wrote:
> 
> I'd expect most usage of this to be through gpios, since we're talking
> about external independent modules here. I would prefer not to bring
> in the reset controller stuff here -- it just adds another layer of
> abstraction that seems unnecessary.
> 
> I would prefer if the reset framework was contained to only be the
> on-chip SoC IP block resets, etc, as originally intended.

Ok, fair enough.

	Arnd
Olof Johansson Jan. 20, 2014, 7:13 p.m. UTC | #10
On Mon, Jan 20, 2014 at 09:44:23AM +0100, Ulf Hansson wrote:
> On 20 January 2014 04:56, Olof Johansson <olof@lixom.net> wrote:
> > This patch enables support for power-on sequencing of SDIO peripherals through DT.
> >
> > In general, it's quite common that wifi modules and other similar
> > peripherals have several signals in addition to the SDIO interface that
> > needs wiggling before the module will power on. It's common to have a
> > reference clock, one or several power rails and one or several lines
> > for reset/enable type functions.
> >
> > The binding as written today introduces a number of reset gpios,
> > a regulator and a clock specifier. The code will handle up to 2 gpio
> > reset lines, but it's trivial to increase to more than that if needed
> > at some point.
> >
> > Implementation-wise, the MMC core has been changed to handle this during
> > host power up, before the host interface is powered on. I have not yet
> > implemented the power-down side, I wanted people to have a chance for
> > reporting back w.r.t. issues (or comments on the bindings) first.
> >
> > I have not tested the regulator portion, since the system and module
> > I'm working on doesn't need one (Samsung Chromebook with Marvell
> > 8797-based wifi). Testing of those portions (and reporting back) would
> > be appreciated.
> >
> > Signed-off-by: Olof Johansson <olof@lixom.net>
> > ---
> >  Documentation/devicetree/bindings/mmc/mmc.txt |   11 +++++++
> >  drivers/mmc/core/core.c                       |   42 +++++++++++++++++++++++++
> >  drivers/mmc/core/host.c                       |   30 +++++++++++++++++-
> >  include/linux/mmc/host.h                      |    5 +++
> >  4 files changed, 87 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> > index 458b57f..962e0ee 100644
> > --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> > +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> > @@ -5,6 +5,8 @@ these definitions.
> >  Interpreted by the OF core:
> >  - reg: Registers location and length.
> >  - interrupts: Interrupts used by the MMC controller.
> > +- clocks: Clocks needed for the host controller, if any.
> > +- clock-names: Goes with clocks above.
> >
> >  Card detection:
> >  If no property below is supplied, host native card detect is used.
> > @@ -30,6 +32,15 @@ Optional properties:
> >  - cap-sdio-irq: enable SDIO IRQ signalling on this interface
> >  - full-pwr-cycle: full power cycle of the card is supported
> >
> > +Card power and reset control:
> > +The following properties can be specified for cases where the MMC
> > +peripheral needs additional reset, regulator and clock lines. It is for
> > +example common for WiFi/BT adapters to have these separate from the main
> > +MMC bus:
> > +  - card-reset-gpios: Specify GPIOs for card reset (reset active low)
> > +  - card-external-vcc-supply: Regulator to drive (independent) card VCC
> > +  - clock with name "card_ext_clock": External clock provided to the card
> > +
> >  *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
> >  polarity properties, we have to fix the meaning of the "normal" and "inverted"
> >  line levels. We choose to follow the SDHCI standard, which specifies both those
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 098374b..c43e6c8 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -13,11 +13,13 @@
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> >  #include <linux/interrupt.h>
> > +#include <linux/clk.h>
> >  #include <linux/completion.h>
> >  #include <linux/device.h>
> >  #include <linux/delay.h>
> >  #include <linux/pagemap.h>
> >  #include <linux/err.h>
> > +#include <linux/gpio.h>
> >  #include <linux/leds.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/log2.h>
> > @@ -1519,6 +1521,43 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type)
> >         mmc_host_clk_release(host);
> >  }
> >
> > +static void mmc_card_power_up(struct mmc_host *host)
> > +{
> > +       int i;
> > +       struct gpio_desc **gds = host->card_reset_gpios;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
> > +               if (gds[i]) {
> > +                       dev_dbg(host->parent, "Asserting reset line %d", i);
> > +                       gpiod_set_value(gds[i], 1);
> > +               }
> > +       }
> > +
> > +       if (host->card_regulator) {
> > +               dev_dbg(host->parent, "Enabling external regulator");
> > +               if (regulator_enable(host->card_regulator))
> > +                       dev_err(host->parent, "Failed to enable external regulator");
> > +       }
> > +
> > +       if (host->card_clk) {
> > +               dev_dbg(host->parent, "Enabling external clock");
> > +               clk_prepare_enable(host->card_clk);
> > +       }
> > +
> > +       /* 2ms delay to let clocks and power settle */
> > +       mmc_delay(20);
> > +
> > +       for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
> > +               if (gds[i]) {
> > +                       dev_dbg(host->parent, "Deasserting reset line %d", i);
> > +                       gpiod_set_value(gds[i], 0);
> > +               }
> > +       }
> > +
> > +       /* 2ms delay to after reset release */
> > +       mmc_delay(20);
> > +}
> > +
> >  /*
> >   * Apply power to the MMC stack.  This is a two-stage process.
> >   * First, we enable power to the card without the clock running.
> > @@ -1535,6 +1574,9 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
> >         if (host->ios.power_mode == MMC_POWER_ON)
> >                 return;
> >
> > +       /* Power up the card/module first, if needed */
> > +       mmc_card_power_up(host);
> > +
> >         mmc_host_clk_hold(host);
> >
> >         host->ios.vdd = fls(ocr) - 1;
> > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> > index 49bc403..e6b850b 100644
> > --- a/drivers/mmc/core/host.c
> > +++ b/drivers/mmc/core/host.c
> > @@ -12,14 +12,18 @@
> >   *  MMC host class device management
> >   */
> >
> > +#include <linux/kernel.h>
> > +#include <linux/clk.h>
> >  #include <linux/device.h>
> >  #include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/idr.h>
> >  #include <linux/of.h>
> >  #include <linux/of_gpio.h>
> >  #include <linux/pagemap.h>
> >  #include <linux/export.h>
> >  #include <linux/leds.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  #include <linux/suspend.h>
> >
> > @@ -312,7 +316,7 @@ int mmc_of_parse(struct mmc_host *host)
> >         u32 bus_width;
> >         bool explicit_inv_wp, gpio_inv_wp = false;
> >         enum of_gpio_flags flags;
> > -       int len, ret, gpio;
> > +       int i, len, ret, gpio;
> >
> >         if (!host->parent || !host->parent->of_node)
> >                 return 0;
> > @@ -415,6 +419,30 @@ int mmc_of_parse(struct mmc_host *host)
> >         if (explicit_inv_wp ^ gpio_inv_wp)
> >                 host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
> >
> > +       /* Parse card power/reset/clock control */
> 
> I would like us to prevent to open up for confusion with the "eMMC hw
> reset" when adding this. Unless we are able to combine them in some
> way?
> 
> Could we maybe add some more comments about in what scenarios this DT
> property would be useful?

Ok, can do. How about something like:

	/*
	 * Some cards need separate power/reset/clock control from the main
	 * MMC/SDIO bus. Parse the description of those controls so we can
	 * power on the card before the host controller.
	 */


> > +       if (of_find_property(np, "card-reset-gpios", NULL)) {
> > +               struct gpio_desc *gpd;
> > +               for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
> > +                       gpd = devm_gpiod_get_index(host->parent, "card-reset", i);
> > +                       if (IS_ERR(gpd))
> > +                               break;
> > +                       gpiod_direction_output(gpd, 0);
> > +                       host->card_reset_gpios[i] = gpd;
> > +               }
> > +
> > +               gpd = devm_gpiod_get_index(host->parent, "card-reset", ARRAY_SIZE(host->card_reset_gpios));
> > +               if (!IS_ERR(gpd)) {
> > +                       dev_warn(host->parent, "More reset gpios than we can handle");
> > +                       gpiod_put(gpd);
> > +               }
> > +       }
> > +
> > +       host->card_clk = of_clk_get_by_name(np, "card_ext_clock");
> 
> of_clk_get_by_name relies on COMMON_CLK, is that really what you want here?
> 
> > +       if (IS_ERR(host->card_clk))
> > +               host->card_clk = NULL;
> > +
> > +       host->card_regulator = regulator_get(host->parent, "card-external-vcc");
> 
> Is the above regulator related to host->ocr_avail mask? Could the
> above regulator be replaced by vmmc?

I have to admit that I don't know MMC as well as I could, but OCR seems to be
something that's between the driver/controller/device, not related to external
power control in this case?

> At the moment host drivers uses mmc_regulator_get_supply(), which
> fetches regulators called "vmmc" and "vqmmc". It is also common to
> have these defined in DT like "vmmc-supply". This has not been
> properly documented for most host cases, and we should fix that. I
> also think it would make sense to include these in the documentation
> for the common mmc bindings, instead of host specific bindings.

Hm, I had been of the impression that the vmmc stuff is to control
power/voltage on the signal lines, not for external card power. Still, even in
that case there's need for the reset line handling and clock control.

I'll take a look and see if there's a way to handle that in a properly
sequenced way and still use the same regulator.


-Olof
Fabio Estevam Jan. 20, 2014, 7:14 p.m. UTC | #11
On Mon, Jan 20, 2014 at 1:56 AM, Olof Johansson <olof@lixom.net> wrote:

> +Card power and reset control:
> +The following properties can be specified for cases where the MMC
> +peripheral needs additional reset, regulator and clock lines. It is for
> +example common for WiFi/BT adapters to have these separate from the main
> +MMC bus:
> +  - card-reset-gpios: Specify GPIOs for card reset (reset active low)

Wouldn't it be better not to assume that the card reset is always
active low and read the GPIO_ACTIVE_xxx flags instead?

Regards,

Fabio Estevam
Olof Johansson Jan. 20, 2014, 7:14 p.m. UTC | #12
On Mon, Jan 20, 2014 at 11:14 AM, Fabio Estevam <festevam@gmail.com> wrote:
> On Mon, Jan 20, 2014 at 1:56 AM, Olof Johansson <olof@lixom.net> wrote:
>
>> +Card power and reset control:
>> +The following properties can be specified for cases where the MMC
>> +peripheral needs additional reset, regulator and clock lines. It is for
>> +example common for WiFi/BT adapters to have these separate from the main
>> +MMC bus:
>> +  - card-reset-gpios: Specify GPIOs for card reset (reset active low)
>
> Wouldn't it be better not to assume that the card reset is always
> active low and read the GPIO_ACTIVE_xxx flags instead?

That's actually what the code does, I need to update the text. Thanks
for spotting it.


-Olof
Sascha Hauer Jan. 21, 2014, 7:24 a.m. UTC | #13
On Sun, Jan 19, 2014 at 07:56:53PM -0800, Olof Johansson wrote:
> This patch enables support for power-on sequencing of SDIO peripherals through DT.
> 
> In general, it's quite common that wifi modules and other similar
> peripherals have several signals in addition to the SDIO interface that
> needs wiggling before the module will power on. It's common to have a
> reference clock, one or several power rails and one or several lines
> for reset/enable type functions.
> 
> The binding as written today introduces a number of reset gpios,
> a regulator and a clock specifier. The code will handle up to 2 gpio
> reset lines, but it's trivial to increase to more than that if needed
> at some point.
> 
> Implementation-wise, the MMC core has been changed to handle this during
> host power up, before the host interface is powered on. I have not yet
> implemented the power-down side, I wanted people to have a chance for
> reporting back w.r.t. issues (or comments on the bindings) first.
> 
> I have not tested the regulator portion, since the system and module
> I'm working on doesn't need one (Samsung Chromebook with Marvell
> 8797-based wifi). Testing of those portions (and reporting back) would
> be appreciated.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
>  Documentation/devicetree/bindings/mmc/mmc.txt |   11 +++++++
>  drivers/mmc/core/core.c                       |   42 +++++++++++++++++++++++++
>  drivers/mmc/core/host.c                       |   30 +++++++++++++++++-
>  include/linux/mmc/host.h                      |    5 +++
>  4 files changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> index 458b57f..962e0ee 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> @@ -5,6 +5,8 @@ these definitions.
>  Interpreted by the OF core:
>  - reg: Registers location and length.
>  - interrupts: Interrupts used by the MMC controller.
> +- clocks: Clocks needed for the host controller, if any.
> +- clock-names: Goes with clocks above.
>  
>  Card detection:
>  If no property below is supplied, host native card detect is used.
> @@ -30,6 +32,15 @@ Optional properties:
>  - cap-sdio-irq: enable SDIO IRQ signalling on this interface
>  - full-pwr-cycle: full power cycle of the card is supported
>  
> +Card power and reset control:
> +The following properties can be specified for cases where the MMC
> +peripheral needs additional reset, regulator and clock lines. It is for
> +example common for WiFi/BT adapters to have these separate from the main
> +MMC bus:
> +  - card-reset-gpios: Specify GPIOs for card reset (reset active low)

We have GPIO_ACTIVE_LOW/HIGH. No need to hardcode this.

Sascha
Sascha Hauer Jan. 21, 2014, 7:25 a.m. UTC | #14
On Tue, Jan 21, 2014 at 08:24:47AM +0100, Sascha Hauer wrote:
> On Sun, Jan 19, 2014 at 07:56:53PM -0800, Olof Johansson wrote:
> > +Card power and reset control:
> > +The following properties can be specified for cases where the MMC
> > +peripheral needs additional reset, regulator and clock lines. It is for
> > +example common for WiFi/BT adapters to have these separate from the main
> > +MMC bus:
> > +  - card-reset-gpios: Specify GPIOs for card reset (reset active low)
> 
> We have GPIO_ACTIVE_LOW/HIGH. No need to hardcode this.

Fabio already spotted that. Sorry for the noise.

Sascha
Ulf Hansson Jan. 21, 2014, 8:55 a.m. UTC | #15
On 20 January 2014 20:13, Olof Johansson <olof@lixom.net> wrote:
> On Mon, Jan 20, 2014 at 09:44:23AM +0100, Ulf Hansson wrote:
>> On 20 January 2014 04:56, Olof Johansson <olof@lixom.net> wrote:
>> > This patch enables support for power-on sequencing of SDIO peripherals through DT.
>> >
>> > In general, it's quite common that wifi modules and other similar
>> > peripherals have several signals in addition to the SDIO interface that
>> > needs wiggling before the module will power on. It's common to have a
>> > reference clock, one or several power rails and one or several lines
>> > for reset/enable type functions.
>> >
>> > The binding as written today introduces a number of reset gpios,
>> > a regulator and a clock specifier. The code will handle up to 2 gpio
>> > reset lines, but it's trivial to increase to more than that if needed
>> > at some point.
>> >
>> > Implementation-wise, the MMC core has been changed to handle this during
>> > host power up, before the host interface is powered on. I have not yet
>> > implemented the power-down side, I wanted people to have a chance for
>> > reporting back w.r.t. issues (or comments on the bindings) first.
>> >
>> > I have not tested the regulator portion, since the system and module
>> > I'm working on doesn't need one (Samsung Chromebook with Marvell
>> > 8797-based wifi). Testing of those portions (and reporting back) would
>> > be appreciated.
>> >
>> > Signed-off-by: Olof Johansson <olof@lixom.net>
>> > ---
>> >  Documentation/devicetree/bindings/mmc/mmc.txt |   11 +++++++
>> >  drivers/mmc/core/core.c                       |   42 +++++++++++++++++++++++++
>> >  drivers/mmc/core/host.c                       |   30 +++++++++++++++++-
>> >  include/linux/mmc/host.h                      |    5 +++
>> >  4 files changed, 87 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
>> > index 458b57f..962e0ee 100644
>> > --- a/Documentation/devicetree/bindings/mmc/mmc.txt
>> > +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
>> > @@ -5,6 +5,8 @@ these definitions.
>> >  Interpreted by the OF core:
>> >  - reg: Registers location and length.
>> >  - interrupts: Interrupts used by the MMC controller.
>> > +- clocks: Clocks needed for the host controller, if any.
>> > +- clock-names: Goes with clocks above.
>> >
>> >  Card detection:
>> >  If no property below is supplied, host native card detect is used.
>> > @@ -30,6 +32,15 @@ Optional properties:
>> >  - cap-sdio-irq: enable SDIO IRQ signalling on this interface
>> >  - full-pwr-cycle: full power cycle of the card is supported
>> >
>> > +Card power and reset control:
>> > +The following properties can be specified for cases where the MMC
>> > +peripheral needs additional reset, regulator and clock lines. It is for
>> > +example common for WiFi/BT adapters to have these separate from the main
>> > +MMC bus:
>> > +  - card-reset-gpios: Specify GPIOs for card reset (reset active low)
>> > +  - card-external-vcc-supply: Regulator to drive (independent) card VCC
>> > +  - clock with name "card_ext_clock": External clock provided to the card
>> > +
>> >  *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
>> >  polarity properties, we have to fix the meaning of the "normal" and "inverted"
>> >  line levels. We choose to follow the SDHCI standard, which specifies both those
>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> > index 098374b..c43e6c8 100644
>> > --- a/drivers/mmc/core/core.c
>> > +++ b/drivers/mmc/core/core.c
>> > @@ -13,11 +13,13 @@
>> >  #include <linux/module.h>
>> >  #include <linux/init.h>
>> >  #include <linux/interrupt.h>
>> > +#include <linux/clk.h>
>> >  #include <linux/completion.h>
>> >  #include <linux/device.h>
>> >  #include <linux/delay.h>
>> >  #include <linux/pagemap.h>
>> >  #include <linux/err.h>
>> > +#include <linux/gpio.h>
>> >  #include <linux/leds.h>
>> >  #include <linux/scatterlist.h>
>> >  #include <linux/log2.h>
>> > @@ -1519,6 +1521,43 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type)
>> >         mmc_host_clk_release(host);
>> >  }
>> >
>> > +static void mmc_card_power_up(struct mmc_host *host)
>> > +{
>> > +       int i;
>> > +       struct gpio_desc **gds = host->card_reset_gpios;
>> > +
>> > +       for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
>> > +               if (gds[i]) {
>> > +                       dev_dbg(host->parent, "Asserting reset line %d", i);
>> > +                       gpiod_set_value(gds[i], 1);
>> > +               }
>> > +       }
>> > +
>> > +       if (host->card_regulator) {
>> > +               dev_dbg(host->parent, "Enabling external regulator");
>> > +               if (regulator_enable(host->card_regulator))
>> > +                       dev_err(host->parent, "Failed to enable external regulator");
>> > +       }
>> > +
>> > +       if (host->card_clk) {
>> > +               dev_dbg(host->parent, "Enabling external clock");
>> > +               clk_prepare_enable(host->card_clk);
>> > +       }
>> > +
>> > +       /* 2ms delay to let clocks and power settle */
>> > +       mmc_delay(20);
>> > +
>> > +       for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
>> > +               if (gds[i]) {
>> > +                       dev_dbg(host->parent, "Deasserting reset line %d", i);
>> > +                       gpiod_set_value(gds[i], 0);
>> > +               }
>> > +       }
>> > +
>> > +       /* 2ms delay to after reset release */
>> > +       mmc_delay(20);
>> > +}
>> > +
>> >  /*
>> >   * Apply power to the MMC stack.  This is a two-stage process.
>> >   * First, we enable power to the card without the clock running.
>> > @@ -1535,6 +1574,9 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
>> >         if (host->ios.power_mode == MMC_POWER_ON)
>> >                 return;
>> >
>> > +       /* Power up the card/module first, if needed */
>> > +       mmc_card_power_up(host);
>> > +
>> >         mmc_host_clk_hold(host);
>> >
>> >         host->ios.vdd = fls(ocr) - 1;
>> > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>> > index 49bc403..e6b850b 100644
>> > --- a/drivers/mmc/core/host.c
>> > +++ b/drivers/mmc/core/host.c
>> > @@ -12,14 +12,18 @@
>> >   *  MMC host class device management
>> >   */
>> >
>> > +#include <linux/kernel.h>
>> > +#include <linux/clk.h>
>> >  #include <linux/device.h>
>> >  #include <linux/err.h>
>> > +#include <linux/gpio/consumer.h>
>> >  #include <linux/idr.h>
>> >  #include <linux/of.h>
>> >  #include <linux/of_gpio.h>
>> >  #include <linux/pagemap.h>
>> >  #include <linux/export.h>
>> >  #include <linux/leds.h>
>> > +#include <linux/regulator/consumer.h>
>> >  #include <linux/slab.h>
>> >  #include <linux/suspend.h>
>> >
>> > @@ -312,7 +316,7 @@ int mmc_of_parse(struct mmc_host *host)
>> >         u32 bus_width;
>> >         bool explicit_inv_wp, gpio_inv_wp = false;
>> >         enum of_gpio_flags flags;
>> > -       int len, ret, gpio;
>> > +       int i, len, ret, gpio;
>> >
>> >         if (!host->parent || !host->parent->of_node)
>> >                 return 0;
>> > @@ -415,6 +419,30 @@ int mmc_of_parse(struct mmc_host *host)
>> >         if (explicit_inv_wp ^ gpio_inv_wp)
>> >                 host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
>> >
>> > +       /* Parse card power/reset/clock control */
>>
>> I would like us to prevent to open up for confusion with the "eMMC hw
>> reset" when adding this. Unless we are able to combine them in some
>> way?
>>
>> Could we maybe add some more comments about in what scenarios this DT
>> property would be useful?
>
> Ok, can do. How about something like:
>
>         /*
>          * Some cards need separate power/reset/clock control from the main
>          * MMC/SDIO bus. Parse the description of those controls so we can
>          * power on the card before the host controller.
>          */
>



>
>> > +       if (of_find_property(np, "card-reset-gpios", NULL)) {
>> > +               struct gpio_desc *gpd;
>> > +               for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
>> > +                       gpd = devm_gpiod_get_index(host->parent, "card-reset", i);
>> > +                       if (IS_ERR(gpd))
>> > +                               break;
>> > +                       gpiod_direction_output(gpd, 0);
>> > +                       host->card_reset_gpios[i] = gpd;
>> > +               }
>> > +
>> > +               gpd = devm_gpiod_get_index(host->parent, "card-reset", ARRAY_SIZE(host->card_reset_gpios));
>> > +               if (!IS_ERR(gpd)) {
>> > +                       dev_warn(host->parent, "More reset gpios than we can handle");
>> > +                       gpiod_put(gpd);
>> > +               }
>> > +       }
>> > +
>> > +       host->card_clk = of_clk_get_by_name(np, "card_ext_clock");
>>
>> of_clk_get_by_name relies on COMMON_CLK, is that really what you want here?
>>
>> > +       if (IS_ERR(host->card_clk))
>> > +               host->card_clk = NULL;
>> > +
>> > +       host->card_regulator = regulator_get(host->parent, "card-external-vcc");
>>
>> Is the above regulator related to host->ocr_avail mask? Could the
>> above regulator be replaced by vmmc?
>
> I have to admit that I don't know MMC as well as I could, but OCR seems to be
> something that's between the driver/controller/device, not related to external
> power control in this case?

This is related to the power of the card, typically external
regulators controlled by the host driver.

Both the card and the host supports a voltage range. This range is
exactly what the OCR mask describes. At initialization of the card,
the host ocr is validated against the card ocr.

>
>> At the moment host drivers uses mmc_regulator_get_supply(), which
>> fetches regulators called "vmmc" and "vqmmc". It is also common to
>> have these defined in DT like "vmmc-supply". This has not been
>> properly documented for most host cases, and we should fix that. I
>> also think it would make sense to include these in the documentation
>> for the common mmc bindings, instead of host specific bindings.
>
> Hm, I had been of the impression that the vmmc stuff is to control
> power/voltage on the signal lines, not for external card power. Still, even in
> that case there's need for the reset line handling and clock control.

vmmc: the power to the card.
vqmmc: the I/O voltage levels (for the signal lines).

Regarding reset, I agree, those seems to be needed.

Regarding clock control. I suppose you are referring to separate
external clocks, not affecting the SDIO/SD/MMC interface speed!?

That could make sense, but still I wonder how those shall be handled
in a fine grained power management setup. In other words, when shall
those be gated/ungated? Is the mmc core able to take the correct
decision about these?

Kind regards
Uffe

>
> I'll take a look and see if there's a way to handle that in a properly
> sequenced way and still use the same regulator.
>

>
> -Olof
Olof Johansson Jan. 21, 2014, 6:14 p.m. UTC | #16
On Tue, Jan 21, 2014 at 12:55 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
[pruning quotes a bit]
>>> Is the above regulator related to host->ocr_avail mask? Could the
>>> above regulator be replaced by vmmc?
>>
>> I have to admit that I don't know MMC as well as I could, but OCR seems to be
>> something that's between the driver/controller/device, not related to external
>> power control in this case?
>
> This is related to the power of the card, typically external
> regulators controlled by the host driver.
>
> Both the card and the host supports a voltage range. This range is
> exactly what the OCR mask describes. At initialization of the card,
> the host ocr is validated against the card ocr.

Ok, so they specify the voltage needed, but there's no way to
determine what regulator is wired up to control. So that information
is still needed (possibly through vmmc though).


>>> At the moment host drivers uses mmc_regulator_get_supply(), which
>>> fetches regulators called "vmmc" and "vqmmc". It is also common to
>>> have these defined in DT like "vmmc-supply". This has not been
>>> properly documented for most host cases, and we should fix that. I
>>> also think it would make sense to include these in the documentation
>>> for the common mmc bindings, instead of host specific bindings.
>>
>> Hm, I had been of the impression that the vmmc stuff is to control
>> power/voltage on the signal lines, not for external card power. Still, even in
>> that case there's need for the reset line handling and clock control.
>
> vmmc: the power to the card.
> vqmmc: the I/O voltage levels (for the signal lines).

Ah, ok. So vmmc seems like it's the same then. I'll try to get some
cycles to look at it today.


> Regarding reset, I agree, those seems to be needed.
>
> Regarding clock control. I suppose you are referring to separate
> external clocks, not affecting the SDIO/SD/MMC interface speed!?
>
> That could make sense, but still I wonder how those shall be handled
> in a fine grained power management setup. In other words, when shall
> those be gated/ungated? Is the mmc core able to take the correct
> decision about these?

The reference clock is in most cases I've seen 32kHz, and not
something that's under fine-grained power management. So it's not used
to regulate interface speed, etc.


-Olof
Tomasz Figa Jan. 21, 2014, 6:34 p.m. UTC | #17
Hi,

On 20.01.2014 04:56, Olof Johansson wrote:
> This patch enables support for power-on sequencing of SDIO peripherals through DT.
>
> In general, it's quite common that wifi modules and other similar
> peripherals have several signals in addition to the SDIO interface that
> needs wiggling before the module will power on. It's common to have a
> reference clock, one or several power rails and one or several lines
> for reset/enable type functions.
>
> The binding as written today introduces a number of reset gpios,
> a regulator and a clock specifier. The code will handle up to 2 gpio
> reset lines, but it's trivial to increase to more than that if needed
> at some point.
>
> Implementation-wise, the MMC core has been changed to handle this during
> host power up, before the host interface is powered on. I have not yet
> implemented the power-down side, I wanted people to have a chance for
> reporting back w.r.t. issues (or comments on the bindings) first.
>
> I have not tested the regulator portion, since the system and module
> I'm working on doesn't need one (Samsung Chromebook with Marvell
> 8797-based wifi). Testing of those portions (and reporting back) would
> be appreciated.

While I fully agree that this is an important problem that needs to be 
solved, I really don't think this is the right way, because:

a) power-up sequence is really specific to the MMC device and often it's 
not simply a matter of switching on one regulator or one clock, e.g. 
specific time constraints need to be met.

b) you can have WLAN chips in which SDIO is just one of the options to 
use as host interface, which may be also HSIC, I2C or UART. Really. See [1].

c) this is leaking device specific details to generic host code, which 
isn't really elegant.

Now, to make this a bit more constructive, [2] is a solution that I came 
up with (not perfect either), which simply adds a separate platform 
device for the low level part of the chip. I believe this is a better 
solution because:

a) you can often see such WLAN/BT combo chip as a set of separate 
devices, e.g. SDIO WLAN, UART BT and a simple PMIC or management IC, 
which provides power/reset control, out of band signalling and etc. for 
the first two, so it isn't that bad to have a separate device node for 
the last one,

b) you have full freedom of defining your DT binding with whatever data 
you need, any number of clocks, regulators, GPIOs and even out of band 
interrupts (IMHO the most important one).

c) you can implement power-on, power-off sequences as needed for your 
particular device,

d) you have full separation of device-specific data from MMC core (or 
any other subsystem simply used as a way to perform I/O to the chip).

Now what's missing there is a way to signal the MMC core or any other 
transport that a device showed up and the controller should be woken up 
out of standby and scan of the bus initialized. This could be done by 
explicitly specifying the device as a subnode of the 
MMC/UART/USB(HSIC)/I2C or whatever with a link (phandle) to the power 
controller of the chip or the other way around - a link to the 
MMC/UART/... controller from the power controller node.

What do you think?

Best regards,
Tomasz

References:
[1] - http://www.marvell.com/wireless/assets/marvell_avastar_88w8797.pdf
[2] - 
https://review.tizen.org/git/?p=platform/kernel/linux-3.10.git;a=commitdiff;h=978bc9523622248271e4007330ae1a0eee6e0254

>
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
>   Documentation/devicetree/bindings/mmc/mmc.txt |   11 +++++++
>   drivers/mmc/core/core.c                       |   42 +++++++++++++++++++++++++
>   drivers/mmc/core/host.c                       |   30 +++++++++++++++++-
>   include/linux/mmc/host.h                      |    5 +++
>   4 files changed, 87 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> index 458b57f..962e0ee 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> @@ -5,6 +5,8 @@ these definitions.
>   Interpreted by the OF core:
>   - reg: Registers location and length.
>   - interrupts: Interrupts used by the MMC controller.
> +- clocks: Clocks needed for the host controller, if any.
> +- clock-names: Goes with clocks above.
>
>   Card detection:
>   If no property below is supplied, host native card detect is used.
> @@ -30,6 +32,15 @@ Optional properties:
>   - cap-sdio-irq: enable SDIO IRQ signalling on this interface
>   - full-pwr-cycle: full power cycle of the card is supported
>
> +Card power and reset control:
> +The following properties can be specified for cases where the MMC
> +peripheral needs additional reset, regulator and clock lines. It is for
> +example common for WiFi/BT adapters to have these separate from the main
> +MMC bus:
> +  - card-reset-gpios: Specify GPIOs for card reset (reset active low)
> +  - card-external-vcc-supply: Regulator to drive (independent) card VCC
> +  - clock with name "card_ext_clock": External clock provided to the card
> +
>   *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
>   polarity properties, we have to fix the meaning of the "normal" and "inverted"
>   line levels. We choose to follow the SDHCI standard, which specifies both those
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 098374b..c43e6c8 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -13,11 +13,13 @@
>   #include <linux/module.h>
>   #include <linux/init.h>
>   #include <linux/interrupt.h>
> +#include <linux/clk.h>
>   #include <linux/completion.h>
>   #include <linux/device.h>
>   #include <linux/delay.h>
>   #include <linux/pagemap.h>
>   #include <linux/err.h>
> +#include <linux/gpio.h>
>   #include <linux/leds.h>
>   #include <linux/scatterlist.h>
>   #include <linux/log2.h>
> @@ -1519,6 +1521,43 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type)
>   	mmc_host_clk_release(host);
>   }
>
> +static void mmc_card_power_up(struct mmc_host *host)
> +{
> +	int i;
> +	struct gpio_desc **gds = host->card_reset_gpios;
> +
> +	for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
> +		if (gds[i]) {
> +			dev_dbg(host->parent, "Asserting reset line %d", i);
> +			gpiod_set_value(gds[i], 1);
> +		}
> +	}
> +
> +	if (host->card_regulator) {
> +		dev_dbg(host->parent, "Enabling external regulator");
> +		if (regulator_enable(host->card_regulator))
> +			dev_err(host->parent, "Failed to enable external regulator");
> +	}
> +
> +	if (host->card_clk) {
> +		dev_dbg(host->parent, "Enabling external clock");
> +		clk_prepare_enable(host->card_clk);
> +	}
> +
> +	/* 2ms delay to let clocks and power settle */
> +	mmc_delay(20);
> +
> +	for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
> +		if (gds[i]) {
> +			dev_dbg(host->parent, "Deasserting reset line %d", i);
> +			gpiod_set_value(gds[i], 0);
> +		}
> +	}
> +
> +	/* 2ms delay to after reset release */
> +	mmc_delay(20);
> +}
> +
>   /*
>    * Apply power to the MMC stack.  This is a two-stage process.
>    * First, we enable power to the card without the clock running.
> @@ -1535,6 +1574,9 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
>   	if (host->ios.power_mode == MMC_POWER_ON)
>   		return;
>
> +	/* Power up the card/module first, if needed */
> +	mmc_card_power_up(host);
> +
>   	mmc_host_clk_hold(host);
>
>   	host->ios.vdd = fls(ocr) - 1;
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 49bc403..e6b850b 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -12,14 +12,18 @@
>    *  MMC host class device management
>    */
>
> +#include <linux/kernel.h>
> +#include <linux/clk.h>
>   #include <linux/device.h>
>   #include <linux/err.h>
> +#include <linux/gpio/consumer.h>
>   #include <linux/idr.h>
>   #include <linux/of.h>
>   #include <linux/of_gpio.h>
>   #include <linux/pagemap.h>
>   #include <linux/export.h>
>   #include <linux/leds.h>
> +#include <linux/regulator/consumer.h>
>   #include <linux/slab.h>
>   #include <linux/suspend.h>
>
> @@ -312,7 +316,7 @@ int mmc_of_parse(struct mmc_host *host)
>   	u32 bus_width;
>   	bool explicit_inv_wp, gpio_inv_wp = false;
>   	enum of_gpio_flags flags;
> -	int len, ret, gpio;
> +	int i, len, ret, gpio;
>
>   	if (!host->parent || !host->parent->of_node)
>   		return 0;
> @@ -415,6 +419,30 @@ int mmc_of_parse(struct mmc_host *host)
>   	if (explicit_inv_wp ^ gpio_inv_wp)
>   		host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
>
> +	/* Parse card power/reset/clock control */
> +	if (of_find_property(np, "card-reset-gpios", NULL)) {
> +		struct gpio_desc *gpd;
> +		for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
> +			gpd = devm_gpiod_get_index(host->parent, "card-reset", i);
> +			if (IS_ERR(gpd))
> +				break;
> +			gpiod_direction_output(gpd, 0);
> +			host->card_reset_gpios[i] = gpd;
> +		}
> +
> +		gpd = devm_gpiod_get_index(host->parent, "card-reset", ARRAY_SIZE(host->card_reset_gpios));
> +		if (!IS_ERR(gpd)) {
> +			dev_warn(host->parent, "More reset gpios than we can handle");
> +			gpiod_put(gpd);
> +		}
> +	}
> +
> +	host->card_clk = of_clk_get_by_name(np, "card_ext_clock");
> +	if (IS_ERR(host->card_clk))
> +		host->card_clk = NULL;
> +
> +	host->card_regulator = regulator_get(host->parent, "card-external-vcc");
> +
>   	if (of_find_property(np, "cap-sd-highspeed", &len))
>   		host->caps |= MMC_CAP_SD_HIGHSPEED;
>   	if (of_find_property(np, "cap-mmc-highspeed", &len))
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 99f5709..6781887 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -297,6 +297,11 @@ struct mmc_host {
>   	unsigned long           clkgate_delay;
>   #endif
>
> +	/* card specific properties to deal with power and reset */
> +	struct regulator	*card_regulator; /* External VCC needed by the card */
> +	struct gpio_desc	*card_reset_gpios[2]; /* External resets, active low */
> +	struct clk		*card_clk;	/* External clock needed by the card */
> +
>   	/* host specific block data */
>   	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
>   	unsigned short		max_segs;	/* see blk_queue_max_segments */
>
Russell King - ARM Linux Jan. 21, 2014, 7:19 p.m. UTC | #18
On Mon, Jan 20, 2014 at 04:47:34PM -0200, Fabio Estevam wrote:
> On Mon, Jan 20, 2014 at 3:16 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> 
> > Not as far as I can see.  fixed-clock appears to have two properies:
> >
> >         clock-frequency
> >         clock-output-names
> >
> > and nothing else.  See of_fixed_clk_setup in drivers/clk/clk-fixed-rate.c.
> > You'll also find that the documentation in this file says this about it:
> 
> Looks like Documentation/devicetree/bindings/clock/fixed-clock.txt is
> misleading then:
> 
> "Optional properties:
> - gpios : From common gpio binding; gpio connection to clock enable pin."

It seems so.  It appears that the binding was added to clk-fixed-rate.c
here:

commit 015ba40246497ae02a5f644d4c8adfec76d9b75c
Date:   Sat Apr 7 21:39:39 2012 -0500

and the binding documentation separately:

commit 766e6a4ec602d0c107553b91b3434fe9c03474f4
Date:   Mon Apr 9 14:50:06 2012 -0500

From what I can see, this "gpios" property has never been supported.
Olof Johansson Jan. 21, 2014, 9:30 p.m. UTC | #19
On Tue, Jan 21, 2014 at 10:34 AM, Tomasz Figa <t.figa@samsung.com> wrote:

> What do you think?

Sure, post the patches.


-Olof
Tomasz Figa Jan. 21, 2014, 9:39 p.m. UTC | #20
On 21.01.2014 22:30, Olof Johansson wrote:
> On Tue, Jan 21, 2014 at 10:34 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>
>> What do you think?
>
> Sure, post the patches.

OK. Will give it a try this weekend.

Best regards,
Tomasz
Mark Brown Jan. 22, 2014, 11:30 a.m. UTC | #21
On Tue, Jan 21, 2014 at 10:14:49AM -0800, Olof Johansson wrote:
> On Tue, Jan 21, 2014 at 12:55 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> > That could make sense, but still I wonder how those shall be handled
> > in a fine grained power management setup. In other words, when shall
> > those be gated/ungated? Is the mmc core able to take the correct
> > decision about these?

> The reference clock is in most cases I've seen 32kHz, and not
> something that's under fine-grained power management. So it's not used
> to regulate interface speed, etc.

I have seen devices connected using SDIO which did use fine grained
power management here with faster clocks.  They're definitely not the
common case but they're there.  :/
Fabio Estevam Jan. 24, 2014, 5:35 p.m. UTC | #22
On Mon, Jan 20, 2014 at 2:48 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> So far so good.  Now, what about this external oscillator which has its
> own separate power control.  My immediate thought is that this can be
> specified via card_ext_clock - I would simply need to declare a fixed-rate
> clock with either a regulator (power switch) controlled via a gpio (which
> would probably be closer to the hardware) or a gpio as an enable... ah,
> that requires me to write a common clock driver for that bit since this
> is currently not modelled by CCF...

Jiry Sarha posted a gpio controlled clock proposal:
http://www.spinics.net/lists/devicetree/msg16651.html

Regards,

Fabio Estevam
Tomasz Figa Jan. 26, 2014, 5:26 p.m. UTC | #23
On 21.01.2014 19:34, Tomasz Figa wrote:
> Hi,
>
> On 20.01.2014 04:56, Olof Johansson wrote:
>> This patch enables support for power-on sequencing of SDIO peripherals
>> through DT.
>>
>> In general, it's quite common that wifi modules and other similar
>> peripherals have several signals in addition to the SDIO interface that
>> needs wiggling before the module will power on. It's common to have a
>> reference clock, one or several power rails and one or several lines
>> for reset/enable type functions.
>>
>> The binding as written today introduces a number of reset gpios,
>> a regulator and a clock specifier. The code will handle up to 2 gpio
>> reset lines, but it's trivial to increase to more than that if needed
>> at some point.
>>
>> Implementation-wise, the MMC core has been changed to handle this during
>> host power up, before the host interface is powered on. I have not yet
>> implemented the power-down side, I wanted people to have a chance for
>> reporting back w.r.t. issues (or comments on the bindings) first.
>>
>> I have not tested the regulator portion, since the system and module
>> I'm working on doesn't need one (Samsung Chromebook with Marvell
>> 8797-based wifi). Testing of those portions (and reporting back) would
>> be appreciated.
>
> While I fully agree that this is an important problem that needs to be
> solved, I really don't think this is the right way, because:
>
> a) power-up sequence is really specific to the MMC device and often it's
> not simply a matter of switching on one regulator or one clock, e.g.
> specific time constraints need to be met.
>
> b) you can have WLAN chips in which SDIO is just one of the options to
> use as host interface, which may be also HSIC, I2C or UART. Really. See
> [1].
>
> c) this is leaking device specific details to generic host code, which
> isn't really elegant.
>
> Now, to make this a bit more constructive, [2] is a solution that I came
> up with (not perfect either), which simply adds a separate platform
> device for the low level part of the chip. I believe this is a better
> solution because:
>
> a) you can often see such WLAN/BT combo chip as a set of separate
> devices, e.g. SDIO WLAN, UART BT and a simple PMIC or management IC,
> which provides power/reset control, out of band signalling and etc. for
> the first two, so it isn't that bad to have a separate device node for
> the last one,
>
> b) you have full freedom of defining your DT binding with whatever data
> you need, any number of clocks, regulators, GPIOs and even out of band
> interrupts (IMHO the most important one).
>
> c) you can implement power-on, power-off sequences as needed for your
> particular device,
>
> d) you have full separation of device-specific data from MMC core (or
> any other subsystem simply used as a way to perform I/O to the chip).
>
> Now what's missing there is a way to signal the MMC core or any other
> transport that a device showed up and the controller should be woken up
> out of standby and scan of the bus initialized. This could be done by
> explicitly specifying the device as a subnode of the
> MMC/UART/USB(HSIC)/I2C or whatever with a link (phandle) to the power
> controller of the chip or the other way around - a link to the
> MMC/UART/... controller from the power controller node.

I've looked a bit around MMC core code and got some basic idea how 
things look. I will definitely need some guidance, or at least some 
opinions, from MMC guys, as some MMC core changes are unavoidable.

Now, the device-specific code is not really an issue, existing drivers 
usually already have their ways of powering the chips on and off, based 
on platform data. Everything needed here is to retrieve needed resources 
(GPIOs, clocks, regulators) using DT, which should be trivial.

The worse part is the interaction between MMC and power controller 
driver (the platform driver part of WLAN driver, if you look at brcmfmac 
as an example). I believe that we need following things:

a) A way to tell the MMC controller that there is no card detection 
mechanism available on given slot and it also should not be polling the 
slot to check card presence. Something like a "manual card detect" that 
would be triggered by another kernel entity that controls whether the 
MMC device is present (i.e. WLAN driver). We already have "broken-cd" 
property, but it only implies the former, wasting time on needless polling.

b) A mechanism to bind the power controller to used MMC slot. Something 
like "mmc-bus = <&mmc2>;" property in device node of the power 
controller and a function like of_find_mmc_controller_by_node(), which 
would be an MMC counterpart of I2C's of_find_i2c_adapter_by_node(). To 
avoid races, it should probably take a reference on MMC host that would 
have to be dropped explicitly whenever it is not needed anymore.

c) A method to notify the MMC subsystem that card presence has changed. 
We already have something like this in drivers/mmc/core/slot-gpio.c, but 
used for a simple GPIO-based card detection. If the main part of 
mmc_gpio_cd_irqt() could be turned into an exported helper, e.g. 
mmc_force_card_detect(host) then basically we would have everything needed.

Unfortunately, I don't have more time left for today to create patches 
and test them, so for now, I'd like to hear opinion of MMC maintainers 
about this approach. Do you find this acceptable?

By the way, it seems like slot-gpio.c could replace a lot of custom GPIO 
card detection code used in MMC host drivers, e.g. sdhci-s3c. Is there 
any reason why it couldn't?

Best regards,
Tomasz
Jyri Sarha Jan. 27, 2014, 8:43 a.m. UTC | #24
On 01/24/2014 07:35 PM, Fabio Estevam wrote:
> On Mon, Jan 20, 2014 at 2:48 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>
>> So far so good.  Now, what about this external oscillator which has its
>> own separate power control.  My immediate thought is that this can be
>> specified via card_ext_clock - I would simply need to declare a fixed-rate
>> clock with either a regulator (power switch) controlled via a gpio (which
>> would probably be closer to the hardware) or a gpio as an enable... ah,
>> that requires me to write a common clock driver for that bit since this
>> is currently not modelled by CCF...
>
> Jiry Sarha posted a gpio controlled clock proposal:
> http://www.spinics.net/lists/devicetree/msg16651.html
>

I have not received too much feedback to my patch yet. CCF is a bit new 
territory to me, but I think having a separate stackable clk-gpio would 
be more flexible than having the gpio property implemented in 
clk-fixed-rate.

Anyway, I am happy do it either way as long as I can get a gpio 
-controlled clock implementation into the main line.

Cheers,
Jyri
Chen-Yu Tsai Jan. 27, 2014, 8:54 a.m. UTC | #25
Hi,

On Mon, Jan 27, 2014 at 4:43 PM, Jyri Sarha <jsarha@ti.com> wrote:
> On 01/24/2014 07:35 PM, Fabio Estevam wrote:
>>
>> On Mon, Jan 20, 2014 at 2:48 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>
>>> So far so good.  Now, what about this external oscillator which has its
>>> own separate power control.  My immediate thought is that this can be
>>> specified via card_ext_clock - I would simply need to declare a
>>> fixed-rate
>>> clock with either a regulator (power switch) controlled via a gpio (which
>>> would probably be closer to the hardware) or a gpio as an enable... ah,
>>> that requires me to write a common clock driver for that bit since this
>>> is currently not modelled by CCF...
>>
>>
>> Jiry Sarha posted a gpio controlled clock proposal:
>> http://www.spinics.net/lists/devicetree/msg16651.html
>>
>
> I have not received too much feedback to my patch yet. CCF is a bit new
> territory to me, but I think having a separate stackable clk-gpio would be
> more flexible than having the gpio property implemented in clk-fixed-rate.

Not sure where stacking external clocks would be used, but sounds like a
fixed factor clock? Maybe you should add DT support to that if that's what
you need or have.

> Anyway, I am happy do it either way as long as I can get a gpio -controlled
> clock implementation into the main line.


Cheers,
ChenYu
Jyri Sarha Jan. 27, 2014, 9:48 a.m. UTC | #26
On 01/27/2014 10:54 AM, Chen-Yu Tsai wrote:
> Hi,
>
> On Mon, Jan 27, 2014 at 4:43 PM, Jyri Sarha <jsarha@ti.com> wrote:
>> On 01/24/2014 07:35 PM, Fabio Estevam wrote:
...
>> I have not received too much feedback to my patch yet. CCF is a bit new
>> territory to me, but I think having a separate stackable clk-gpio would be
>> more flexible than having the gpio property implemented in clk-fixed-rate.
>
> Not sure where stacking external clocks would be used, but sounds like a
> fixed factor clock? Maybe you should add DT support to that if that's what
> you need or have.
>

Ah, but then there is also the problem of having to change 
clk_register_fixed_rate() in the CCF api, which is called from all over 
the place.

Of course I could add a new clk_register_fixed_rate_gpio(), but still it 
does not look to me like the way to go.

Cheers,
Jyri
Ulf Hansson Jan. 27, 2014, 10:19 a.m. UTC | #27
On 26 January 2014 18:26, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 21.01.2014 19:34, Tomasz Figa wrote:
>>
>> Hi,
>>
>> On 20.01.2014 04:56, Olof Johansson wrote:
>>>
>>> This patch enables support for power-on sequencing of SDIO peripherals
>>> through DT.
>>>
>>> In general, it's quite common that wifi modules and other similar
>>> peripherals have several signals in addition to the SDIO interface that
>>> needs wiggling before the module will power on. It's common to have a
>>> reference clock, one or several power rails and one or several lines
>>> for reset/enable type functions.
>>>
>>> The binding as written today introduces a number of reset gpios,
>>> a regulator and a clock specifier. The code will handle up to 2 gpio
>>> reset lines, but it's trivial to increase to more than that if needed
>>> at some point.
>>>
>>> Implementation-wise, the MMC core has been changed to handle this during
>>> host power up, before the host interface is powered on. I have not yet
>>> implemented the power-down side, I wanted people to have a chance for
>>> reporting back w.r.t. issues (or comments on the bindings) first.
>>>
>>> I have not tested the regulator portion, since the system and module
>>> I'm working on doesn't need one (Samsung Chromebook with Marvell
>>> 8797-based wifi). Testing of those portions (and reporting back) would
>>> be appreciated.
>>
>>
>> While I fully agree that this is an important problem that needs to be
>> solved, I really don't think this is the right way, because:
>>
>> a) power-up sequence is really specific to the MMC device and often it's
>> not simply a matter of switching on one regulator or one clock, e.g.
>> specific time constraints need to be met.
>>
>> b) you can have WLAN chips in which SDIO is just one of the options to
>> use as host interface, which may be also HSIC, I2C or UART. Really. See
>> [1].
>>
>> c) this is leaking device specific details to generic host code, which
>> isn't really elegant.
>>
>> Now, to make this a bit more constructive, [2] is a solution that I came
>> up with (not perfect either), which simply adds a separate platform
>> device for the low level part of the chip. I believe this is a better
>> solution because:
>>
>> a) you can often see such WLAN/BT combo chip as a set of separate
>> devices, e.g. SDIO WLAN, UART BT and a simple PMIC or management IC,
>> which provides power/reset control, out of band signalling and etc. for
>> the first two, so it isn't that bad to have a separate device node for
>> the last one,
>>
>> b) you have full freedom of defining your DT binding with whatever data
>> you need, any number of clocks, regulators, GPIOs and even out of band
>> interrupts (IMHO the most important one).
>>
>> c) you can implement power-on, power-off sequences as needed for your
>> particular device,
>>
>> d) you have full separation of device-specific data from MMC core (or
>> any other subsystem simply used as a way to perform I/O to the chip).
>>
>> Now what's missing there is a way to signal the MMC core or any other
>> transport that a device showed up and the controller should be woken up
>> out of standby and scan of the bus initialized. This could be done by
>> explicitly specifying the device as a subnode of the
>> MMC/UART/USB(HSIC)/I2C or whatever with a link (phandle) to the power
>> controller of the chip or the other way around - a link to the
>> MMC/UART/... controller from the power controller node.
>
>
> I've looked a bit around MMC core code and got some basic idea how things
> look. I will definitely need some guidance, or at least some opinions, from
> MMC guys, as some MMC core changes are unavoidable.
>
> Now, the device-specific code is not really an issue, existing drivers
> usually already have their ways of powering the chips on and off, based on
> platform data. Everything needed here is to retrieve needed resources
> (GPIOs, clocks, regulators) using DT, which should be trivial.
>
> The worse part is the interaction between MMC and power controller driver
> (the platform driver part of WLAN driver, if you look at brcmfmac as an
> example). I believe that we need following things:
>
> a) A way to tell the MMC controller that there is no card detection
> mechanism available on given slot and it also should not be polling the slot
> to check card presence. Something like a "manual card detect" that would be
> triggered by another kernel entity that controls whether the MMC device is
> present (i.e. WLAN driver). We already have "broken-cd" property, but it
> only implies the former, wasting time on needless polling.

There is already a host capability that I think we could use to handle
this. MMC_CAP_NONREMOVABLE, the corresponding DT binding string is
"non-removable", and it may be set per host device.

Using this cap means the mmc_rescan process that runs to detect new
cards, will only be executed once and during boot. So, we need to make
sure all resources and powers are provided to the card at this point.
Otherwise the card will not be detected.

In the SDIO case, to save power, the SDIO func driver may use runtime
PM to tell the mmc core power about whether the card needs to be
powered. Typically from the WLAN driver's probe() and "interface
up/down" the runtime PM reference for the SDIO func device, should be
adjusted with pm_runtime_get|put.

>
> b) A mechanism to bind the power controller to used MMC slot. Something like
> "mmc-bus = <&mmc2>;" property in device node of the power controller and a
> function like of_find_mmc_controller_by_node(), which would be an MMC
> counterpart of I2C's of_find_i2c_adapter_by_node(). To avoid races, it
> should probably take a reference on MMC host that would have to be dropped
> explicitly whenever it is not needed anymore.

I suppose an "MMC slot" can be translated to "MMC host"?

What I am trying to understand is how the mmc core (or if we push it
to be handled from the mmc host's .set_ios callback) shall be able to
tell the power controller driver to enable/disable it's resources.
Somehow we need the struct device available to handle that. Then I
guess operating on it using runtime PM would be a solution that would
be quite nice!?

>
> c) A method to notify the MMC subsystem that card presence has changed. We
> already have something like this in drivers/mmc/core/slot-gpio.c, but used
> for a simple GPIO-based card detection. If the main part of
> mmc_gpio_cd_irqt() could be turned into an exported helper, e.g.
> mmc_force_card_detect(host) then basically we would have everything needed.

I am not sure I understand why this is needed. I think it would be
more convenient to use MMC_CAP_NONREMOVABLE instead as stated earlier.
But please elaborate, I might have missed something.

>
> Unfortunately, I don't have more time left for today to create patches and
> test them, so for now, I'd like to hear opinion of MMC maintainers about
> this approach. Do you find this acceptable?
>
> By the way, it seems like slot-gpio.c could replace a lot of custom GPIO
> card detection code used in MMC host drivers, e.g. sdhci-s3c. Is there any
> reason why it couldn't?

I suppose most host driver's should convert to the slot-gpio API, it's
is just a matter of someone to send the patches. :-)

Kind regards
Ulf Hansson

>
> Best regards,
> Tomasz
Tomasz Figa Jan. 28, 2014, 12:59 a.m. UTC | #28
On 27.01.2014 11:19, Ulf Hansson wrote:
> On 26 January 2014 18:26, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> On 21.01.2014 19:34, Tomasz Figa wrote:
>>>
>>> Hi,
>>>
>>> On 20.01.2014 04:56, Olof Johansson wrote:
>>>>
>>>> This patch enables support for power-on sequencing of SDIO peripherals
>>>> through DT.
>>>>
>>>> In general, it's quite common that wifi modules and other similar
>>>> peripherals have several signals in addition to the SDIO interface that
>>>> needs wiggling before the module will power on. It's common to have a
>>>> reference clock, one or several power rails and one or several lines
>>>> for reset/enable type functions.
>>>>
>>>> The binding as written today introduces a number of reset gpios,
>>>> a regulator and a clock specifier. The code will handle up to 2 gpio
>>>> reset lines, but it's trivial to increase to more than that if needed
>>>> at some point.
>>>>
>>>> Implementation-wise, the MMC core has been changed to handle this during
>>>> host power up, before the host interface is powered on. I have not yet
>>>> implemented the power-down side, I wanted people to have a chance for
>>>> reporting back w.r.t. issues (or comments on the bindings) first.
>>>>
>>>> I have not tested the regulator portion, since the system and module
>>>> I'm working on doesn't need one (Samsung Chromebook with Marvell
>>>> 8797-based wifi). Testing of those portions (and reporting back) would
>>>> be appreciated.
>>>
>>>
>>> While I fully agree that this is an important problem that needs to be
>>> solved, I really don't think this is the right way, because:
>>>
>>> a) power-up sequence is really specific to the MMC device and often it's
>>> not simply a matter of switching on one regulator or one clock, e.g.
>>> specific time constraints need to be met.
>>>
>>> b) you can have WLAN chips in which SDIO is just one of the options to
>>> use as host interface, which may be also HSIC, I2C or UART. Really. See
>>> [1].
>>>
>>> c) this is leaking device specific details to generic host code, which
>>> isn't really elegant.
>>>
>>> Now, to make this a bit more constructive, [2] is a solution that I came
>>> up with (not perfect either), which simply adds a separate platform
>>> device for the low level part of the chip. I believe this is a better
>>> solution because:
>>>
>>> a) you can often see such WLAN/BT combo chip as a set of separate
>>> devices, e.g. SDIO WLAN, UART BT and a simple PMIC or management IC,
>>> which provides power/reset control, out of band signalling and etc. for
>>> the first two, so it isn't that bad to have a separate device node for
>>> the last one,
>>>
>>> b) you have full freedom of defining your DT binding with whatever data
>>> you need, any number of clocks, regulators, GPIOs and even out of band
>>> interrupts (IMHO the most important one).
>>>
>>> c) you can implement power-on, power-off sequences as needed for your
>>> particular device,
>>>
>>> d) you have full separation of device-specific data from MMC core (or
>>> any other subsystem simply used as a way to perform I/O to the chip).
>>>
>>> Now what's missing there is a way to signal the MMC core or any other
>>> transport that a device showed up and the controller should be woken up
>>> out of standby and scan of the bus initialized. This could be done by
>>> explicitly specifying the device as a subnode of the
>>> MMC/UART/USB(HSIC)/I2C or whatever with a link (phandle) to the power
>>> controller of the chip or the other way around - a link to the
>>> MMC/UART/... controller from the power controller node.
>>
>>
>> I've looked a bit around MMC core code and got some basic idea how things
>> look. I will definitely need some guidance, or at least some opinions, from
>> MMC guys, as some MMC core changes are unavoidable.
>>
>> Now, the device-specific code is not really an issue, existing drivers
>> usually already have their ways of powering the chips on and off, based on
>> platform data. Everything needed here is to retrieve needed resources
>> (GPIOs, clocks, regulators) using DT, which should be trivial.
>>
>> The worse part is the interaction between MMC and power controller driver
>> (the platform driver part of WLAN driver, if you look at brcmfmac as an
>> example). I believe that we need following things:
>>
>> a) A way to tell the MMC controller that there is no card detection
>> mechanism available on given slot and it also should not be polling the slot
>> to check card presence. Something like a "manual card detect" that would be
>> triggered by another kernel entity that controls whether the MMC device is
>> present (i.e. WLAN driver). We already have "broken-cd" property, but it
>> only implies the former, wasting time on needless polling.
>
> There is already a host capability that I think we could use to handle
> this. MMC_CAP_NONREMOVABLE, the corresponding DT binding string is
> "non-removable", and it may be set per host device.
>
> Using this cap means the mmc_rescan process that runs to detect new
> cards, will only be executed once and during boot. So, we need to make
> sure all resources and powers are provided to the card at this point.
> Otherwise the card will not be detected.

I don't quite like this requirement, especially if you consider 
multi-platform kernels where a lot of drivers is going to be provided as 
modules. WLAN drivers are especially good candidates. This means that 
even if the card is powered off at boot-up, if user (or init system) 
loads appropriate module, which powers the chip on, MMC core must be 
able to notice this.

> In the SDIO case, to save power, the SDIO func driver may use runtime
> PM to tell the mmc core power about whether the card needs to be
> powered. Typically from the WLAN driver's probe() and "interface
> up/down" the runtime PM reference for the SDIO func device, should be
> adjusted with pm_runtime_get|put.

I need to think a bit more about the power management control flow here. 
In case of such chips I'd tend to look at MMC merely as a host 
interface, which as I said, might be only one of available options. I'm 
not sure if it should be the host interface core that decides whether 
the whole device should be powered off. However there might be a 
solution that leverages SDIO func runtime PM, which wouldn't imply such 
control flow. Let me reconsider this.

>
>>
>> b) A mechanism to bind the power controller to used MMC slot. Something like
>> "mmc-bus = <&mmc2>;" property in device node of the power controller and a
>> function like of_find_mmc_controller_by_node(), which would be an MMC
>> counterpart of I2C's of_find_i2c_adapter_by_node(). To avoid races, it
>> should probably take a reference on MMC host that would have to be dropped
>> explicitly whenever it is not needed anymore.
>
> I suppose an "MMC slot" can be translated to "MMC host"?

Right.

> What I am trying to understand is how the mmc core (or if we push it
> to be handled from the mmc host's .set_ios callback) shall be able to
> tell the power controller driver to enable/disable it's resources.
> Somehow we need the struct device available to handle that. Then I
> guess operating on it using runtime PM would be a solution that would
> be quite nice!?

As I wrote above, I'm not quite sure about this yet.

>>
>> c) A method to notify the MMC subsystem that card presence has changed. We
>> already have something like this in drivers/mmc/core/slot-gpio.c, but used
>> for a simple GPIO-based card detection. If the main part of
>> mmc_gpio_cd_irqt() could be turned into an exported helper, e.g.
>> mmc_force_card_detect(host) then basically we would have everything needed.
>
> I am not sure I understand why this is needed. I think it would be
> more convenient to use MMC_CAP_NONREMOVABLE instead as stated earlier.
> But please elaborate, I might have missed something.

See above. I'm not quite convinced that state of MMC interface should 
determine power state of the chip. I can easily imagine a situation 
where the MMC link is powered down (link power management) but the WLAN 
chip keeps operation. Keep in mind that those are usually complete SoCs 
that can keep processing network traffic autonomously and wake-up the 
application processor whenever anything interesting happens using extra 
out of bounds signalling, which might trigger re-enabling the MMC link.

>>
>> Unfortunately, I don't have more time left for today to create patches and
>> test them, so for now, I'd like to hear opinion of MMC maintainers about
>> this approach. Do you find this acceptable?
>>
>> By the way, it seems like slot-gpio.c could replace a lot of custom GPIO
>> card detection code used in MMC host drivers, e.g. sdhci-s3c. Is there any
>> reason why it couldn't?
>
> I suppose most host driver's should convert to the slot-gpio API, it's
> is just a matter of someone to send the patches. :-)

OK, great. I'll add conversion of sdhci-s3c to my queue then.

Best regards,
Tomasz
Chris Ball Jan. 28, 2014, 1:08 a.m. UTC | #29
Hi,

On Tue, Jan 28 2014, Tomasz Figa wrote:
>> I am not sure I understand why this is needed. I think it would be
>> more convenient to use MMC_CAP_NONREMOVABLE instead as stated earlier.
>> But please elaborate, I might have missed something.
>
> See above. I'm not quite convinced that state of MMC interface should
> determine power state of the chip. I can easily imagine a situation
> where the MMC link is powered down (link power management) but the
> WLAN chip keeps operation. Keep in mind that those are usually
> complete SoCs that can keep processing network traffic autonomously
> and wake-up the application processor whenever anything interesting
> happens using extra out of bounds signalling, which might trigger
> re-enabling the MMC link.

For what it's worth, we did this using upstream code (libertas-sd8686
WLAN with sdhci-pxav3) at OLPC.  We set the SDIO device to 1-bit mode
on system suspend, using MMC_CAP_NONREMOVABLE, MMC_PM_KEEP_POWER and
MMC_PM_WAKE_SDIO_IRQ, and tell the PMU to wake on the 1-bit data line.
When it wakes the system, the system sees the SDIO interrupt and
processes the waiting network traffic.

So this use case is already supported using the current interfaces.
If this interface doesn't work for your use case, could you talk a
little more about what you're trying to achieve?

Thanks,

- Chris.
Ulf Hansson Jan. 28, 2014, 10:06 a.m. UTC | #30
On 28 January 2014 01:59, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 27.01.2014 11:19, Ulf Hansson wrote:
>>
>> On 26 January 2014 18:26, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>
>>> On 21.01.2014 19:34, Tomasz Figa wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>> On 20.01.2014 04:56, Olof Johansson wrote:
>>>>>
>>>>>
>>>>> This patch enables support for power-on sequencing of SDIO peripherals
>>>>> through DT.
>>>>>
>>>>> In general, it's quite common that wifi modules and other similar
>>>>> peripherals have several signals in addition to the SDIO interface that
>>>>> needs wiggling before the module will power on. It's common to have a
>>>>> reference clock, one or several power rails and one or several lines
>>>>> for reset/enable type functions.
>>>>>
>>>>> The binding as written today introduces a number of reset gpios,
>>>>> a regulator and a clock specifier. The code will handle up to 2 gpio
>>>>> reset lines, but it's trivial to increase to more than that if needed
>>>>> at some point.
>>>>>
>>>>> Implementation-wise, the MMC core has been changed to handle this
>>>>> during
>>>>> host power up, before the host interface is powered on. I have not yet
>>>>> implemented the power-down side, I wanted people to have a chance for
>>>>> reporting back w.r.t. issues (or comments on the bindings) first.
>>>>>
>>>>> I have not tested the regulator portion, since the system and module
>>>>> I'm working on doesn't need one (Samsung Chromebook with Marvell
>>>>> 8797-based wifi). Testing of those portions (and reporting back) would
>>>>> be appreciated.
>>>>
>>>>
>>>>
>>>> While I fully agree that this is an important problem that needs to be
>>>> solved, I really don't think this is the right way, because:
>>>>
>>>> a) power-up sequence is really specific to the MMC device and often it's
>>>> not simply a matter of switching on one regulator or one clock, e.g.
>>>> specific time constraints need to be met.
>>>>
>>>> b) you can have WLAN chips in which SDIO is just one of the options to
>>>> use as host interface, which may be also HSIC, I2C or UART. Really. See
>>>> [1].
>>>>
>>>> c) this is leaking device specific details to generic host code, which
>>>> isn't really elegant.
>>>>
>>>> Now, to make this a bit more constructive, [2] is a solution that I came
>>>> up with (not perfect either), which simply adds a separate platform
>>>> device for the low level part of the chip. I believe this is a better
>>>> solution because:
>>>>
>>>> a) you can often see such WLAN/BT combo chip as a set of separate
>>>> devices, e.g. SDIO WLAN, UART BT and a simple PMIC or management IC,
>>>> which provides power/reset control, out of band signalling and etc. for
>>>> the first two, so it isn't that bad to have a separate device node for
>>>> the last one,
>>>>
>>>> b) you have full freedom of defining your DT binding with whatever data
>>>> you need, any number of clocks, regulators, GPIOs and even out of band
>>>> interrupts (IMHO the most important one).
>>>>
>>>> c) you can implement power-on, power-off sequences as needed for your
>>>> particular device,
>>>>
>>>> d) you have full separation of device-specific data from MMC core (or
>>>> any other subsystem simply used as a way to perform I/O to the chip).
>>>>
>>>> Now what's missing there is a way to signal the MMC core or any other
>>>> transport that a device showed up and the controller should be woken up
>>>> out of standby and scan of the bus initialized. This could be done by
>>>> explicitly specifying the device as a subnode of the
>>>> MMC/UART/USB(HSIC)/I2C or whatever with a link (phandle) to the power
>>>> controller of the chip or the other way around - a link to the
>>>> MMC/UART/... controller from the power controller node.
>>>
>>>
>>>
>>> I've looked a bit around MMC core code and got some basic idea how things
>>> look. I will definitely need some guidance, or at least some opinions,
>>> from
>>> MMC guys, as some MMC core changes are unavoidable.
>>>
>>> Now, the device-specific code is not really an issue, existing drivers
>>> usually already have their ways of powering the chips on and off, based
>>> on
>>> platform data. Everything needed here is to retrieve needed resources
>>> (GPIOs, clocks, regulators) using DT, which should be trivial.
>>>
>>> The worse part is the interaction between MMC and power controller driver
>>> (the platform driver part of WLAN driver, if you look at brcmfmac as an
>>> example). I believe that we need following things:
>>>
>>> a) A way to tell the MMC controller that there is no card detection
>>> mechanism available on given slot and it also should not be polling the
>>> slot
>>> to check card presence. Something like a "manual card detect" that would
>>> be
>>> triggered by another kernel entity that controls whether the MMC device
>>> is
>>> present (i.e. WLAN driver). We already have "broken-cd" property, but it
>>> only implies the former, wasting time on needless polling.
>>
>>
>> There is already a host capability that I think we could use to handle
>> this. MMC_CAP_NONREMOVABLE, the corresponding DT binding string is
>> "non-removable", and it may be set per host device.
>>
>> Using this cap means the mmc_rescan process that runs to detect new
>> cards, will only be executed once and during boot. So, we need to make
>> sure all resources and powers are provided to the card at this point.
>> Otherwise the card will not be detected.
>
>
> I don't quite like this requirement, especially if you consider
> multi-platform kernels where a lot of drivers is going to be provided as
> modules. WLAN drivers are especially good candidates. This means that even
> if the card is powered off at boot-up, if user (or init system) loads
> appropriate module, which powers the chip on, MMC core must be able to
> notice this.

To be able to detect the card, the WLAN driver doesn't have to be
probed, only the "power controller" driver. I suppose this is were it
becomes a bit tricky.

Somehow the mmc core needs to be involved in the probe process of the
power controller driver. Could perhaps the power controller bus be
located in the mmc core and thus the power controller driver needs to
register itself by using a new API from the mmc core? Similar how SDIO
func driver's register themselves.

I have one concern here though. Unless the SDIO func driver gets
probed, the SDIO card will be kept powered, which is not optimal from
a power management perspective.

To solve this, we need to change the policy about how to handle SDIO
cards after the initialization sequence (mmc_rescan) has been
completed. This will affect SDIO func driver's as well, since at the
moment those expects the card to be fully powered once they are being
probed.

>
>
>> In the SDIO case, to save power, the SDIO func driver may use runtime
>> PM to tell the mmc core power about whether the card needs to be
>> powered. Typically from the WLAN driver's probe() and "interface
>> up/down" the runtime PM reference for the SDIO func device, should be
>> adjusted with pm_runtime_get|put.
>
>
> I need to think a bit more about the power management control flow here. In
> case of such chips I'd tend to look at MMC merely as a host interface, which
> as I said, might be only one of available options. I'm not sure if it should
> be the host interface core that decides whether the whole device should be
> powered off. However there might be a solution that leverages SDIO func
> runtime PM, which wouldn't imply such control flow. Let me reconsider this.
>

Just to clarify things; it is not the "host interface" that decides
whether the whole device should be powered off. This is decided from
the SDIO func driver, by using runtime PM.

The "host interface" still needs to be in control of the power on/off
sequence, since the knowledge about the SDIO spec is required to
handle this.


>
>>
>>>
>>> b) A mechanism to bind the power controller to used MMC slot. Something
>>> like
>>> "mmc-bus = <&mmc2>;" property in device node of the power controller and
>>> a
>>> function like of_find_mmc_controller_by_node(), which would be an MMC
>>> counterpart of I2C's of_find_i2c_adapter_by_node(). To avoid races, it
>>> should probably take a reference on MMC host that would have to be
>>> dropped
>>> explicitly whenever it is not needed anymore.
>>
>>
>> I suppose an "MMC slot" can be translated to "MMC host"?
>
>
> Right.
>
>
>> What I am trying to understand is how the mmc core (or if we push it
>> to be handled from the mmc host's .set_ios callback) shall be able to
>> tell the power controller driver to enable/disable it's resources.
>> Somehow we need the struct device available to handle that. Then I
>> guess operating on it using runtime PM would be a solution that would
>> be quite nice!?
>
>
> As I wrote above, I'm not quite sure about this yet.
>
>
>>>
>>> c) A method to notify the MMC subsystem that card presence has changed.
>>> We
>>> already have something like this in drivers/mmc/core/slot-gpio.c, but
>>> used
>>> for a simple GPIO-based card detection. If the main part of
>>> mmc_gpio_cd_irqt() could be turned into an exported helper, e.g.
>>> mmc_force_card_detect(host) then basically we would have everything
>>> needed.
>>
>>
>> I am not sure I understand why this is needed. I think it would be
>> more convenient to use MMC_CAP_NONREMOVABLE instead as stated earlier.
>> But please elaborate, I might have missed something.
>
>
> See above. I'm not quite convinced that state of MMC interface should
> determine power state of the chip. I can easily imagine a situation where
> the MMC link is powered down (link power management) but the WLAN chip keeps
> operation. Keep in mind that those are usually complete SoCs that can keep
> processing network traffic autonomously and wake-up the application
> processor whenever anything interesting happens using extra out of bounds
> signalling, which might trigger re-enabling the MMC link.

Am not sure I understand what you mean with MMC link here.

We have the VCC regulator that the mmc host driver handles and the
resources by "power controller" driver. Do you want these to be
remained enabled during system suspend or are you saying we might need
even more fine grained power management?

Additionally, as Chris also pointed out in his reply; SDIO func
drivers can prevent the mmc core from powering off the card during
system suspend. Check for the flag, MMC_PM_KEEP_POWER in the code.

Kind regards
Uffe

>
>
>>>
>>> Unfortunately, I don't have more time left for today to create patches
>>> and
>>> test them, so for now, I'd like to hear opinion of MMC maintainers about
>>> this approach. Do you find this acceptable?
>>>
>>> By the way, it seems like slot-gpio.c could replace a lot of custom GPIO
>>> card detection code used in MMC host drivers, e.g. sdhci-s3c. Is there
>>> any
>>> reason why it couldn't?
>>
>>
>> I suppose most host driver's should convert to the slot-gpio API, it's
>> is just a matter of someone to send the patches. :-)
>
>
> OK, great. I'll add conversion of sdhci-s3c to my queue then.
>
> Best regards,
> Tomasz
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Jan. 28, 2014, 10:48 a.m. UTC | #31
On Tuesday 28 January 2014, Ulf Hansson wrote:
> On 28 January 2014 01:59, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > On 27.01.2014 11:19, Ulf Hansson wrote:
> >> There is already a host capability that I think we could use to handle
> >> this. MMC_CAP_NONREMOVABLE, the corresponding DT binding string is
> >> "non-removable", and it may be set per host device.
> >>
> >> Using this cap means the mmc_rescan process that runs to detect new
> >> cards, will only be executed once and during boot. So, we need to make
> >> sure all resources and powers are provided to the card at this point.
> >> Otherwise the card will not be detected.
> >
> > I don't quite like this requirement, especially if you consider
> > multi-platform kernels where a lot of drivers is going to be provided as
> > modules. WLAN drivers are especially good candidates. This means that even
> > if the card is powered off at boot-up, if user (or init system) loads
> > appropriate module, which powers the chip on, MMC core must be able to
> > notice this.
> 
> To be able to detect the card, the WLAN driver doesn't have to be
> probed, only the "power controller" driver. I suppose this is were it
> becomes a bit tricky.
> 
> Somehow the mmc core needs to be involved in the probe process of the
> power controller driver. Could perhaps the power controller bus be
> located in the mmc core and thus the power controller driver needs to
> register itself by using a new API from the mmc core? Similar how SDIO
> func driver's register themselves.

I think there is another option, which does have its own pros and cons:
We could move all the power handling back into the sdio function driver
if we allow a secondary detection path using DT rather than the probing
of the SDIO bus. Essentially you'd have to list the class/vendor/device
ID for each function that cannot be autodetected in DT, and have the
SDIO core pretend that it found the device just by looking at the
device nodes, and register the struct sdio_func so it can be bound to
the driver. The driver then does all the power handling in a device
specific way. At some point the hardware gets registered at the 
mmc host, and the sdio core connects the bus state to the already present
sdio_func, possibly notifying the function driver that it has become
usable.

Obviously, this can only work for CAP_NONREMOVABLE devices, but those
are exactly the ones we are worried about here. The advantage is that
the power sequencing for a particular device can then be in device
specific code and can have an arbitrarily complex in the driver without
needing the mmc code to handle all possible corner cases.

	Arnd
Mark Brown Feb. 12, 2014, 6:33 p.m. UTC | #32
On Tue, Jan 28, 2014 at 11:48:10AM +0100, Arnd Bergmann wrote:

> I think there is another option, which does have its own pros and cons:
> We could move all the power handling back into the sdio function driver
> if we allow a secondary detection path using DT rather than the probing
> of the SDIO bus. Essentially you'd have to list the class/vendor/device
> ID for each function that cannot be autodetected in DT, and have the
> SDIO core pretend that it found the device just by looking at the
> device nodes, and register the struct sdio_func so it can be bound to
> the driver. The driver then does all the power handling in a device
> specific way. At some point the hardware gets registered at the 
> mmc host, and the sdio core connects the bus state to the already present
> sdio_func, possibly notifying the function driver that it has become
> usable.

> Obviously, this can only work for CAP_NONREMOVABLE devices, but those
> are exactly the ones we are worried about here. The advantage is that
> the power sequencing for a particular device can then be in device
> specific code and can have an arbitrarily complex in the driver without
> needing the mmc code to handle all possible corner cases.

I think this is going to be the most generic solution overall, it's
going to work with other buses and it's going to allow the device to
fully power itself off while running (I don't know if it's useful for SD
but that's definitely common with some other buses).
Ulf Hansson Feb. 13, 2014, 8:56 a.m. UTC | #33
On 28 January 2014 11:48, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 28 January 2014, Ulf Hansson wrote:
>> On 28 January 2014 01:59, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> > On 27.01.2014 11:19, Ulf Hansson wrote:
>> >> There is already a host capability that I think we could use to handle
>> >> this. MMC_CAP_NONREMOVABLE, the corresponding DT binding string is
>> >> "non-removable", and it may be set per host device.
>> >>
>> >> Using this cap means the mmc_rescan process that runs to detect new
>> >> cards, will only be executed once and during boot. So, we need to make
>> >> sure all resources and powers are provided to the card at this point.
>> >> Otherwise the card will not be detected.
>> >
>> > I don't quite like this requirement, especially if you consider
>> > multi-platform kernels where a lot of drivers is going to be provided as
>> > modules. WLAN drivers are especially good candidates. This means that even
>> > if the card is powered off at boot-up, if user (or init system) loads
>> > appropriate module, which powers the chip on, MMC core must be able to
>> > notice this.
>>
>> To be able to detect the card, the WLAN driver doesn't have to be
>> probed, only the "power controller" driver. I suppose this is were it
>> becomes a bit tricky.
>>
>> Somehow the mmc core needs to be involved in the probe process of the
>> power controller driver. Could perhaps the power controller bus be
>> located in the mmc core and thus the power controller driver needs to
>> register itself by using a new API from the mmc core? Similar how SDIO
>> func driver's register themselves.
>
> I think there is another option, which does have its own pros and cons:
> We could move all the power handling back into the sdio function driver
> if we allow a secondary detection path using DT rather than the probing
> of the SDIO bus. Essentially you'd have to list the class/vendor/device
> ID for each function that cannot be autodetected in DT, and have the
> SDIO core pretend that it found the device just by looking at the
> device nodes, and register the struct sdio_func so it can be bound to
> the driver. The driver then does all the power handling in a device
> specific way. At some point the hardware gets registered at the
> mmc host, and the sdio core connects the bus state to the already present
> sdio_func, possibly notifying the function driver that it has become
> usable.

It seems like an option we could try.

There are some tricky parts that needs to be taken care of by the
mmc/sdio core, regarding the probe/removal and the suspend/resume
sequence, but I suppose it should be possible.

A minor concern is how do we handle devices that are fully powered at
boot? Unless the sdio func driver will be loaded we can't power off
them, right? Do we need to cover this case, do you think?

>
> Obviously, this can only work for CAP_NONREMOVABLE devices, but those
> are exactly the ones we are worried about here. The advantage is that
> the power sequencing for a particular device can then be in device
> specific code and can have an arbitrarily complex in the driver without
> needing the mmc code to handle all possible corner cases.

Agree!

I think a removable SDIO card won't l need this additional power
controller mechanism.

Kind regards
Uffe

>
>         Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Feb. 13, 2014, 9:01 a.m. UTC | #34
On 13.02.2014 09:56, Ulf Hansson wrote:
> On 28 January 2014 11:48, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tuesday 28 January 2014, Ulf Hansson wrote:
>>> On 28 January 2014 01:59, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>> On 27.01.2014 11:19, Ulf Hansson wrote:
>>>>> There is already a host capability that I think we could use to handle
>>>>> this. MMC_CAP_NONREMOVABLE, the corresponding DT binding string is
>>>>> "non-removable", and it may be set per host device.
>>>>>
>>>>> Using this cap means the mmc_rescan process that runs to detect new
>>>>> cards, will only be executed once and during boot. So, we need to make
>>>>> sure all resources and powers are provided to the card at this point.
>>>>> Otherwise the card will not be detected.
>>>>
>>>> I don't quite like this requirement, especially if you consider
>>>> multi-platform kernels where a lot of drivers is going to be provided as
>>>> modules. WLAN drivers are especially good candidates. This means that even
>>>> if the card is powered off at boot-up, if user (or init system) loads
>>>> appropriate module, which powers the chip on, MMC core must be able to
>>>> notice this.
>>>
>>> To be able to detect the card, the WLAN driver doesn't have to be
>>> probed, only the "power controller" driver. I suppose this is were it
>>> becomes a bit tricky.
>>>
>>> Somehow the mmc core needs to be involved in the probe process of the
>>> power controller driver. Could perhaps the power controller bus be
>>> located in the mmc core and thus the power controller driver needs to
>>> register itself by using a new API from the mmc core? Similar how SDIO
>>> func driver's register themselves.
>>
>> I think there is another option, which does have its own pros and cons:
>> We could move all the power handling back into the sdio function driver
>> if we allow a secondary detection path using DT rather than the probing
>> of the SDIO bus. Essentially you'd have to list the class/vendor/device
>> ID for each function that cannot be autodetected in DT, and have the
>> SDIO core pretend that it found the device just by looking at the
>> device nodes, and register the struct sdio_func so it can be bound to
>> the driver. The driver then does all the power handling in a device
>> specific way. At some point the hardware gets registered at the
>> mmc host, and the sdio core connects the bus state to the already present
>> sdio_func, possibly notifying the function driver that it has become
>> usable.
>
> It seems like an option we could try.
>
> There are some tricky parts that needs to be taken care of by the
> mmc/sdio core, regarding the probe/removal and the suspend/resume
> sequence, but I suppose it should be possible.
>
> A minor concern is how do we handle devices that are fully powered at
> boot? Unless the sdio func driver will be loaded we can't power off
> them, right? Do we need to cover this case, do you think?
>
>>
>> Obviously, this can only work for CAP_NONREMOVABLE devices, but those
>> are exactly the ones we are worried about here. The advantage is that
>> the power sequencing for a particular device can then be in device
>> specific code and can have an arbitrarily complex in the driver without
>> needing the mmc code to handle all possible corner cases.
>
> Agree!
>
> I think a removable SDIO card won't l need this additional power
> controller mechanism.

Yes, sounds good to me too. It will be more tricky to implement than the 
solution I initially proposed, but should end up being much cleaner and 
possibly cover more cases.

Best regards,
Tomasz
Russell King - ARM Linux Feb. 13, 2014, 10:42 a.m. UTC | #35
On Tue, Jan 28, 2014 at 11:48:10AM +0100, Arnd Bergmann wrote:
> I think there is another option, which does have its own pros and cons:
> We could move all the power handling back into the sdio function driver
> if we allow a secondary detection path using DT rather than the probing
> of the SDIO bus.

No thanks.

What if we have a platform where things subtly change, like for instance,
the wiring on the SD slot to fix a problem with UHS-1 cards, which means
you don't have UHS-1 support for some platforms but do for others.

What if you have a platform which uses a brcm4329 chip for Wifi, but then
later in the production run switch to using a different Wifi chipset?

With this information encoded into DT, the number of DT files quickly
increases, and then this presents its own problem - how do users get to
know which DT file should be used for their platform when all they see
externally is "a product of type A"?

Let's say that the board folk were kind enough to set some kind of
identifing feature for the first but not the second (why would they,
it's probe-able, damn it).

The "we can do it in DT" approach just makes things unnecessarily more
difficult from the _user_ and _hardware_ point of view.
Arnd Bergmann Feb. 13, 2014, 12:48 p.m. UTC | #36
On Thursday 13 February 2014 10:42:48 Russell King - ARM Linux wrote:
> 
> What if we have a platform where things subtly change, like for instance,
> the wiring on the SD slot to fix a problem with UHS-1 cards, which means
> you don't have UHS-1 support for some platforms but do for others.
> 
> What if you have a platform which uses a brcm4329 chip for Wifi, but then
> later in the production run switch to using a different Wifi chipset?

As far as I can tell, the power sequencing is normally really
dependent on the device. If someone has an on-board brcm4329
that requires a specific set of clocks, resets, voltages etc
to be routed to the chip and enabled in the correct order to
allow probing, it seems unlikely that changing the wifi chipset
to something else would keep the exact same requirements.

> With this information encoded into DT, the number of DT files quickly
> increases, and then this presents its own problem - how do users get to
> know which DT file should be used for their platform when all they see
> externally is "a product of type A"?

I haven't seen a suggestion that would not encode this in DT. The
difference between Olof's original suggestion and mine is that he
proposed to put the data into the existing node of the host controller,
while my approach would be to add a node for each function with
these requirements and modify the sdio code so that we can deal
with it in the function driver. The data that would get passed
however is almost the same.

	Arnd
Russell King - ARM Linux Feb. 13, 2014, 2:41 p.m. UTC | #37
On Thu, Feb 13, 2014 at 01:48:55PM +0100, Arnd Bergmann wrote:
> On Thursday 13 February 2014 10:42:48 Russell King - ARM Linux wrote:
> > 
> > What if we have a platform where things subtly change, like for instance,
> > the wiring on the SD slot to fix a problem with UHS-1 cards, which means
> > you don't have UHS-1 support for some platforms but do for others.
> > 
> > What if you have a platform which uses a brcm4329 chip for Wifi, but then
> > later in the production run switch to using a different Wifi chipset?
> 
> As far as I can tell, the power sequencing is normally really
> dependent on the device. If someone has an on-board brcm4329
> that requires a specific set of clocks, resets, voltages etc
> to be routed to the chip and enabled in the correct order to
> allow probing, it seems unlikely that changing the wifi chipset
> to something else would keep the exact same requirements.

That's your assertion - however, do we /know/ whether there's a situation
where Olof's solution doesn't work because the sequencing is wrong?

I see nothing unreasonable about the sequence:

1. hold reset at low level
2. apply power
3. turn clock on
4. apply reset
5. release reset

The points being:
* never set a signal to a high level before power is applied, otherwise
  we can end up supplying power through that signal (which may cause
  damage.)  That goes for the reset and clock.
* devices normally want clocks running to complete their reset sequencing.

This is actually the sequence which MMC/SD cards do (except without the
reset) anyway - it's spec'd that on the MMC/SD bus, power will be applied
and will be stable before the clock signal is applied, and then the clock
signal runs for a certain number of cycles before you even start talking
to the card.

That all said, we do have the problem that once we decide, we need to
support it because it becomes part of DT - this is one of the things I
hate about DT, it requires over-design.  Your point is "Olof's solution
may break if we have a device which requires a different sequence" which
is a valid point which has to be considered from the DT perspective and
addressed whether or not we actually have a device which meets that
criteria.  I don't see an easy solution to this.
Arnd Bergmann Feb. 13, 2014, 4:13 p.m. UTC | #38
On Thursday 13 February 2014 14:41:06 Russell King - ARM Linux wrote:
> On Thu, Feb 13, 2014 at 01:48:55PM +0100, Arnd Bergmann wrote:
> > On Thursday 13 February 2014 10:42:48 Russell King - ARM Linux wrote:
> > > 
> > > What if we have a platform where things subtly change, like for instance,
> > > the wiring on the SD slot to fix a problem with UHS-1 cards, which means
> > > you don't have UHS-1 support for some platforms but do for others.
> > > 
> > > What if you have a platform which uses a brcm4329 chip for Wifi, but then
> > > later in the production run switch to using a different Wifi chipset?
> > 
> > As far as I can tell, the power sequencing is normally really
> > dependent on the device. If someone has an on-board brcm4329
> > that requires a specific set of clocks, resets, voltages etc
> > to be routed to the chip and enabled in the correct order to
> > allow probing, it seems unlikely that changing the wifi chipset
> > to something else would keep the exact same requirements.
> 
> That's your assertion - however, do we /know/ whether there's a situation
> where Olof's solution doesn't work because the sequencing is wrong?
> 
> I see nothing unreasonable about the sequence:
> 
> 1. hold reset at low level
> 2. apply power
> 3. turn clock on
> 4. apply reset
> 5. release reset

I was thinking of cases where you may need a more complex sequence:
- wait for a device specific time between some of the steps
  (the cw1200 driver seems to need that, but we could probably
   get away with waiting long enough for everyone)
- have more than one of each, and turn them on in the right order.
  cw1200 seems to need two voltages, two gpio resets ("reset"
  and "powerup").
  Again, we could specify a larger number of clocks that can be
  provided to the host, but if we make it a device specific
  property, we already know how many we need.

I can't think of anything that would require significant changes
to the procedure though, just refinements as we run into problems.

> The points being:
> * never set a signal to a high level before power is applied, otherwise
>   we can end up supplying power through that signal (which may cause
>   damage.)  That goes for the reset and clock.
> * devices normally want clocks running to complete their reset sequencing.
> 
> This is actually the sequence which MMC/SD cards do (except without the
> reset) anyway - it's spec'd that on the MMC/SD bus, power will be applied
> and will be stable before the clock signal is applied, and then the clock
> signal runs for a certain number of cycles before you even start talking
> to the card.

It may be dangerous to refer to the spec, since we are talking
specifically about devices that require something beyond what the
spec says ;-) For instance in SD/MMC cards I'd assume the device clock
to be derived from the bus clock. However we can expect that clock
to work already (any working mmc host driver would provide that),
but we may need to drive the device clock. It still sounds reasonable
to assume that the sequencing is the same as for the bus clock.

> That all said, we do have the problem that once we decide, we need to
> support it because it becomes part of DT - this is one of the things I
> hate about DT, it requires over-design.

Yes, I agree. It is a problem that we have to face all the time.
We have in the past defined bindings of both types, overdesigned
and not thought through enough.

>  Your point is "Olof's solution
> may break if we have a device which requires a different sequence" which
> is a valid point which has to be considered from the DT perspective and
> addressed whether or not we actually have a device which meets that
> criteria.  I don't see an easy solution to this.

I think either one will work. With Olof's suggestion that may mean we
have to keep adding support for increasingly complex cases when we
run into them, or it may all be easy. With my suggestion, we give
more room for function drivers to mess things up, but at least we
can keep the complexity in the places that need them and only need
to change the core once.

Aside from the power-on problem, my suggestion would at the same
time solve the second problem of having a place to stick arbitrary
DT properties for the sdio function. Again looking at the cw1200
example, they may require passing an IRQ descriptor, a MAC address,
the device clock rate, and two flags for things that are not
detectable by looking at the device ID (whether a 5GHz antenna is
connected and something about odd block size transfers).
This is probably the main difference between the two approaches.

	Arnd
Olof Johansson Feb. 13, 2014, 5:31 p.m. UTC | #39
On Thu, Feb 13, 2014 at 05:13:14PM +0100, Arnd Bergmann wrote:
> On Thursday 13 February 2014 14:41:06 Russell King - ARM Linux wrote:
> > On Thu, Feb 13, 2014 at 01:48:55PM +0100, Arnd Bergmann wrote:
> > > On Thursday 13 February 2014 10:42:48 Russell King - ARM Linux wrote:
> > > > 
> > > > What if we have a platform where things subtly change, like for instance,
> > > > the wiring on the SD slot to fix a problem with UHS-1 cards, which means
> > > > you don't have UHS-1 support for some platforms but do for others.
> > > > 
> > > > What if you have a platform which uses a brcm4329 chip for Wifi, but then
> > > > later in the production run switch to using a different Wifi chipset?
> > > 
> > > As far as I can tell, the power sequencing is normally really
> > > dependent on the device. If someone has an on-board brcm4329
> > > that requires a specific set of clocks, resets, voltages etc
> > > to be routed to the chip and enabled in the correct order to
> > > allow probing, it seems unlikely that changing the wifi chipset
> > > to something else would keep the exact same requirements.
> > 
> > That's your assertion - however, do we /know/ whether there's a situation
> > where Olof's solution doesn't work because the sequencing is wrong?
> > 
> > I see nothing unreasonable about the sequence:
> > 
> > 1. hold reset at low level
> > 2. apply power
> > 3. turn clock on
> > 4. apply reset
> > 5. release reset
> 
> I was thinking of cases where you may need a more complex sequence:
> - wait for a device specific time between some of the steps
>   (the cw1200 driver seems to need that, but we could probably
>    get away with waiting long enough for everyone)
> - have more than one of each, and turn them on in the right order.
>   cw1200 seems to need two voltages, two gpio resets ("reset"
>   and "powerup").

Those two gpio resets are extremely common, but on snow (the chromebook) the
powerup gpio is hard-wired. So it's not all that unusual. As I mention in the
patch, a positive-sense powerup and a negative-sense reset aren't all that
different in practice.

>   Again, we could specify a larger number of clocks that can be
>   provided to the host, but if we make it a device specific
>   property, we already know how many we need.
> 
> I can't think of anything that would require significant changes
> to the procedure though, just refinements as we run into problems.

The main pain will be if there's a requirement to do
gpio-requlator-gpio-regulator. We could mandate that regulators are turned on
in order. (Also, see below).

> 
> > The points being:
> > * never set a signal to a high level before power is applied, otherwise
> >   we can end up supplying power through that signal (which may cause
> >   damage.)  That goes for the reset and clock.
> > * devices normally want clocks running to complete their reset sequencing.
> > 
> > This is actually the sequence which MMC/SD cards do (except without the
> > reset) anyway - it's spec'd that on the MMC/SD bus, power will be applied
> > and will be stable before the clock signal is applied, and then the clock
> > signal runs for a certain number of cycles before you even start talking
> > to the card.
> 
> It may be dangerous to refer to the spec, since we are talking
> specifically about devices that require something beyond what the
> spec says ;-) For instance in SD/MMC cards I'd assume the device clock
> to be derived from the bus clock. However we can expect that clock
> to work already (any working mmc host driver would provide that),
> but we may need to drive the device clock. It still sounds reasonable
> to assume that the sequencing is the same as for the bus clock.
> 
> > That all said, we do have the problem that once we decide, we need to
> > support it because it becomes part of DT - this is one of the things I
> > hate about DT, it requires over-design.
> 
> Yes, I agree. It is a problem that we have to face all the time.
> We have in the past defined bindings of both types, overdesigned
> and not thought through enough.
> 
> >  Your point is "Olof's solution
> > may break if we have a device which requires a different sequence" which
> > is a valid point which has to be considered from the DT perspective and
> > addressed whether or not we actually have a device which meets that
> > criteria.  I don't see an easy solution to this.
> 
> I think either one will work. With Olof's suggestion that may mean we
> have to keep adding support for increasingly complex cases when we
> run into them, or it may all be easy. With my suggestion, we give
> more room for function drivers to mess things up, but at least we
> can keep the complexity in the places that need them and only need
> to change the core once.

I always anticipated the binding needing amendment over time -- for example if
a device needs longer delays between clock enable and reset release. But most
of those can be handled through bindings amendments as needed (with default
behavior for non-amended bindings the same as today).

Chances are that if we do a per-device binding, we'll likely end up having
shared helpers to parse the settings anyway, so in the end we end up with
similar code, and similar bind maybe subtly different bindings. I suspect
sharing one common binding and common code will be easier long-term but it's
not a black and white choice.

> Aside from the power-on problem, my suggestion would at the same
> time solve the second problem of having a place to stick arbitrary
> DT properties for the sdio function. Again looking at the cw1200
> example, they may require passing an IRQ descriptor, a MAC address,
> the device clock rate, and two flags for things that are not
> detectable by looking at the device ID (whether a 5GHz antenna is
> connected and something about odd block size transfers).
> This is probably the main difference between the two approaches.

So, we do have the option of making the mmc binding take a device subnode
that gets passed in as the of_node when binding the device, and we can
move the power/reset/clock data there even if we don't leave it up to
the card driver to handle and act upon it. It would give us a place to
locate per-device properties like these, but it wouldn't greatly affect
how the rest of the solution looks.


-Olof
Arnd Bergmann Feb. 15, 2014, 12:18 p.m. UTC | #40
On Thursday 13 February 2014 17:13:14 Arnd Bergmann wrote:
> 
> Aside from the power-on problem, my suggestion would at the same
> time solve the second problem of having a place to stick arbitrary
> DT properties for the sdio function. Again looking at the cw1200
> example, they may require passing an IRQ descriptor, a MAC address,
> the device clock rate, and two flags for things that are not
> detectable by looking at the device ID (whether a 5GHz antenna is
> connected and something about odd block size transfers).
> This is probably the main difference between the two approaches.

I just realized one more thing: Some devices have multiple
alternative bus interfaces and it would be nice to use just
one binding for the device, independent of the interface.

If a wlan adapter has both SPI and SDIO front-ends, the external
dependencies (reset, clock, voltage, ...) will be the same, and
from the kernel perspective the main difference is that SPI cannot
be probed at all, while SDIO can be probed as long as the device
is powered on already. If we do what I suggested and add a way to
the SDIO core to probe a device just from DT information, both
the binding and much of the device driver code can be shared
between the bus front-end glues, which would not be possible
if we rely on the SDIO bus probe and add the power-on sequencing
to the SDIO core.

	Arnd
Russell King - ARM Linux Feb. 15, 2014, 12:27 p.m. UTC | #41
On Sat, Feb 15, 2014 at 01:18:02PM +0100, Arnd Bergmann wrote:
> If a wlan adapter has both SPI and SDIO front-ends, the external
> dependencies (reset, clock, voltage, ...) will be the same, and
> from the kernel perspective the main difference is that SPI cannot
> be probed at all, while SDIO can be probed as long as the device
> is powered on already.

Remember that MMC/SD/SDIO cards can be driven by either a MMC host
interface, or a SPI interface.  Both are probe-able.
Arnd Bergmann Feb. 15, 2014, 1:09 p.m. UTC | #42
On Saturday 15 February 2014 12:27:33 Russell King - ARM Linux wrote:
> On Sat, Feb 15, 2014 at 01:18:02PM +0100, Arnd Bergmann wrote:
> > If a wlan adapter has both SPI and SDIO front-ends, the external
> > dependencies (reset, clock, voltage, ...) will be the same, and
> > from the kernel perspective the main difference is that SPI cannot
> > be probed at all, while SDIO can be probed as long as the device
> > is powered on already.
> 
> Remember that MMC/SD/SDIO cards can be driven by either a MMC host
> interface, or a SPI interface.  Both are probe-able.

I knew about MMC/SD cards being required to understand simple SPI,
I wasn't sure about SDIO. My understanding however is that you
have to use the mmc_spi host driver to actually use MMC/SD devices
as a block device, and that requires having either a DT description
for the host or an spi_board_info, which I would not consider
discoverable.

For spi-mode SDIO devices I'm assuming it's similar, except that
you'd describe the actual SDIO device in the board info rather than
create a fake SDIO controller. Still not discoverable unless I'm
missing your point.

	Arnd
Tomasz Figa Feb. 15, 2014, 1:22 p.m. UTC | #43
On 15.02.2014 14:09, Arnd Bergmann wrote:
> On Saturday 15 February 2014 12:27:33 Russell King - ARM Linux wrote:
>> On Sat, Feb 15, 2014 at 01:18:02PM +0100, Arnd Bergmann wrote:
>>> If a wlan adapter has both SPI and SDIO front-ends, the external
>>> dependencies (reset, clock, voltage, ...) will be the same, and
>>> from the kernel perspective the main difference is that SPI cannot
>>> be probed at all, while SDIO can be probed as long as the device
>>> is powered on already.
>>
>> Remember that MMC/SD/SDIO cards can be driven by either a MMC host
>> interface, or a SPI interface.  Both are probe-able.
>
> I knew about MMC/SD cards being required to understand simple SPI,
> I wasn't sure about SDIO. My understanding however is that you
> have to use the mmc_spi host driver to actually use MMC/SD devices
> as a block device, and that requires having either a DT description
> for the host or an spi_board_info, which I would not consider
> discoverable.
>
> For spi-mode SDIO devices I'm assuming it's similar, except that
> you'd describe the actual SDIO device in the board info rather than
> create a fake SDIO controller. Still not discoverable unless I'm
> missing your point.

I'm not sure if we should assume that SPI = MMC over SPI. I believe 
there might be a custom protocol involved as well.

Stepping aside from SPI, I already gave an example of a WLAN chip that 
supports multiple control busses [1]. In addition to the commonly used 
SDIO, it supports USB and HSIC as well:

[1] http://www.marvell.com/wireless/assets/marvell_avastar_88w8797.pdf

Moreover, some of Samsung boards use HSIC to communicate with modem 
chips, which have exactly the same problem as we're trying to solve here 
- they need to be powered on to be discovered.

So I really don't think we should be limiting this to MMC alone by any 
means.

Now I don't really know why we want that badly to represent low level 
control parts of such devices as children of control buses of their 
enumerable parts. Could you tell me what benefits it has to justify the 
added complexity of having to instantiate fake devices in respective 
devices, even though they can be fully detected later?

Best regards,
Tomasz
Arnd Bergmann Feb. 15, 2014, 4:21 p.m. UTC | #44
On Saturday 15 February 2014 14:22:30 Tomasz Figa wrote:
> On 15.02.2014 14:09, Arnd Bergmann wrote:
>
> > For spi-mode SDIO devices I'm assuming it's similar, except that
> > you'd describe the actual SDIO device in the board info rather than
> > create a fake SDIO controller. Still not discoverable unless I'm
> > missing your point.
> 
> I'm not sure if we should assume that SPI = MMC over SPI. I believe 
> there might be a custom protocol involved as well.

In case of SD/MMC, you essentially have three separate command sets:
SPI, MMC and SD, and each of them has multiple versions. MMC and SD
compatible devices generally also support the SPI command set (IIRC
it is required, but I'm not completely sure), and on the bus 
the main difference seems to be that you have only 1-bit serial
signalling, while MMC also supports 4-bit and 8-bit parallel
transmission.

If a device supports both SDIO and SPI, I think a straightforward
implementation would be to use the exact same command set, but
you are right that this isn't the only possibility, and the SD/MMC
shows how they can be slightly different already.

> Stepping aside from SPI, I already gave an example of a WLAN chip that 
> supports multiple control busses [1]. In addition to the commonly used 
> SDIO, it supports USB and HSIC as well:
> 
> [1] http://www.marvell.com/wireless/assets/marvell_avastar_88w8797.pdf
> 
> Moreover, some of Samsung boards use HSIC to communicate with modem 
> chips, which have exactly the same problem as we're trying to solve here 
> - they need to be powered on to be discovered.

Thanks, this definitely makes a good example. I see that it also
supports SPI mode for SDIO as mentioned in your link.

> So I really don't think we should be limiting this to MMC alone by any 
> means.

Agreed. Putting the same chip on USB or HSIC has the exact same
requirements, since we also have a discoverable bus, but actually
finding the device likely involves some power-on sequencing before
the bus controller can find it.

> Now I don't really know why we want that badly to represent low level 
> control parts of such devices as children of control buses of their 
> enumerable parts. Could you tell me what benefits it has to justify the 
> added complexity of having to instantiate fake devices in respective 
> devices, even though they can be fully detected later?

It's not that I "badly want" to do it one way or another, I'm just
trying to find arguments either way, since as Russell points out
whatever we decide to do in DT is what we're stuck with for the
long term future.

Let me try to summarize what we found so far:

* Common aspects:
  * we need a way to attach properties for run-time configuration
    to devices on discoverable buses when the configuration
    is not discoverable. In your 88w8797 example, this includes
    the use of the GPIO pins, runtime power management (clocks,
    regulators) and the attached codecs.
  * We may want to connect the same device to either a discoverable
    or a nondiscoverable bus, ideally using the same binding and
    sharing code whereever possible.

* Olof's proposal (add properties or a child node to the host
  controller node with just power-on sequencing information):
  + We only need one implementation for each bus, possibly shared
    across buses to some degree, and can handle lots of devices
    without having to touch their individual drivers.
  + A logical extension of things we already do on SD cards
    (CD/WP GPIOs, external clocks and voltages supplied to
    standard compliant devices as part of the normal probing)
  - The shared code may get rather complex to deal with all
    possible corner cases we run into over the years.
  - Somewhat harder to do if you have to attach the power
    information to a device node for a USB hub port, rather
    than an SDIO controller that only has one slave device.

* Arnd's proposal (change bus code to probe nonstandard devices
  from DT if we can't easily detect them):
  + Matches what we already do for PCI (at least on powerpc)
    and AMBA/Primecell devices: If a device can't be probed
    using the standard method, we treat it as nondiscoverable
    and describe it using DT.
  + Devices can have arbitrary complex requirements without
    impacting the core, since all code is contained in the
    driver for the nonstandard device.
  + Properties that are required for probing and runtime
    configuration only have to be set once (e.g. you may
    need clk_get() for probing and clk_set_rate() for
    runtime-pm).
  + Devices that have alternative bus interfaces like 88w8797
    can implement the power-on code in a central place per
    driver, and can reuse the code they have for nondiscoverable
    buses on the buses that are normally discoverable but
    broken here.
  - Still need to modify each subsystem to have alternate
    ways of probing, and match up devices later.
  - Has to be implemented in each driver that needs it, making
    it harder to share code for drivers with the same need
    (e.g. every device that just needs an external reset
    trigger).

	Arnd
Russell King - ARM Linux Feb. 15, 2014, 8:52 p.m. UTC | #45
On Sat, Feb 15, 2014 at 05:21:11PM +0100, Arnd Bergmann wrote:
> On Saturday 15 February 2014 14:22:30 Tomasz Figa wrote:
> > On 15.02.2014 14:09, Arnd Bergmann wrote:
> >
> > > For spi-mode SDIO devices I'm assuming it's similar, except that
> > > you'd describe the actual SDIO device in the board info rather than
> > > create a fake SDIO controller. Still not discoverable unless I'm
> > > missing your point.
> > 
> > I'm not sure if we should assume that SPI = MMC over SPI. I believe 
> > there might be a custom protocol involved as well.
> 
> In case of SD/MMC, you essentially have three separate command sets:
> SPI, MMC and SD, and each of them has multiple versions. MMC and SD
> compatible devices generally also support the SPI command set (IIRC
> it is required,

SPI support is mandatory for SDIO as well.

SDIO has CIS (remember card information structures... like PCMCIA)
which identifies the various different logical functions of the device,
giving class information, vendor information etc.

So... certainly the type of the attached device is discoverable even
on SPI.

> If a device supports both SDIO and SPI, I think a straightforward
> implementation would be to use the exact same command set, but
> you are right that this isn't the only possibility, and the SD/MMC
> shows how they can be slightly different already.

Given that the SPI mode is mandatory for SDIO cards, why would you
also implement another SPI mode with different commands?

> > Stepping aside from SPI, I already gave an example of a WLAN chip that 
> > supports multiple control busses [1]. In addition to the commonly used 
> > SDIO, it supports USB and HSIC as well:
> > 
> > [1] http://www.marvell.com/wireless/assets/marvell_avastar_88w8797.pdf
> > 
> > Moreover, some of Samsung boards use HSIC to communicate with modem 
> > chips, which have exactly the same problem as we're trying to solve here 
> > - they need to be powered on to be discovered.
> 
> Thanks, this definitely makes a good example. I see that it also
> supports SPI mode for SDIO as mentioned in your link.

Well, USB is another discoverable bus.  As HSIC is a derivative of USB,
I'd be surprised if it weren't discoverable there too.

So, out of everything identified so far, we have no undiscoverable buses.

> Agreed. Putting the same chip on USB or HSIC has the exact same
> requirements, since we also have a discoverable bus, but actually
> finding the device likely involves some power-on sequencing before
> the bus controller can find it.

That's partly the nature of integrating something onto a board where you
want maximal power savings.  It's basically that dreaded word which
software people seem to hate: "embedded".

> * Arnd's proposal (change bus code to probe nonstandard devices
>   from DT if we can't easily detect them):
>   + Matches what we already do for PCI (at least on powerpc)
>     and AMBA/Primecell devices: If a device can't be probed
>     using the standard method, we treat it as nondiscoverable
>     and describe it using DT.
>   + Devices can have arbitrary complex requirements without
>     impacting the core, since all code is contained in the
>     driver for the nonstandard device.
>   + Properties that are required for probing and runtime
>     configuration only have to be set once (e.g. you may
>     need clk_get() for probing and clk_set_rate() for
>     runtime-pm).
>   + Devices that have alternative bus interfaces like 88w8797
>     can implement the power-on code in a central place per
>     driver, and can reuse the code they have for nondiscoverable
>     buses on the buses that are normally discoverable but
>     broken here.
>   - Still need to modify each subsystem to have alternate
>     ways of probing, and match up devices later.
>   - Has to be implemented in each driver that needs it, making
>     it harder to share code for drivers with the same need
>     (e.g. every device that just needs an external reset
>     trigger).

 - requires different DT if the chip is changed, which causes problems
   for users to identify which out of zillions of DT files they should
   use for their exact platform.
 - have to work out how to match up the fake device with a probed device
   when it becomes available: existing SDIO drivers all assume that the
   card has been through a fairly complex initialisation sequence already.
 - multi-function SDIO is much harder to deal with since you have mutliple
   drivers involved, and the SDIO device as a whole needs initialisation
   before anyone can drive it.
 - adds complexity to the SDIO drivers; they would have to know whether
   they're embedded or on a plug-in card.
Tomasz Figa Feb. 15, 2014, 9:35 p.m. UTC | #46
On 15.02.2014 21:52, Russell King - ARM Linux wrote:
> On Sat, Feb 15, 2014 at 05:21:11PM +0100, Arnd Bergmann wrote:
>> On Saturday 15 February 2014 14:22:30 Tomasz Figa wrote:
>>> On 15.02.2014 14:09, Arnd Bergmann wrote:
>>>
>>>> For spi-mode SDIO devices I'm assuming it's similar, except that
>>>> you'd describe the actual SDIO device in the board info rather than
>>>> create a fake SDIO controller. Still not discoverable unless I'm
>>>> missing your point.
>>>
>>> I'm not sure if we should assume that SPI = MMC over SPI. I believe
>>> there might be a custom protocol involved as well.
>>
>> In case of SD/MMC, you essentially have three separate command sets:
>> SPI, MMC and SD, and each of them has multiple versions. MMC and SD
>> compatible devices generally also support the SPI command set (IIRC
>> it is required,
>
> SPI support is mandatory for SDIO as well.
>
> SDIO has CIS (remember card information structures... like PCMCIA)
> which identifies the various different logical functions of the device,
> giving class information, vendor information etc.
>
> So... certainly the type of the attached device is discoverable even
> on SPI.

It certainly is, assuming that you properly configure the 
non-discoverable part.

>> If a device supports both SDIO and SPI, I think a straightforward
>> implementation would be to use the exact same command set, but
>> you are right that this isn't the only possibility, and the SD/MMC
>> shows how they can be slightly different already.
>
> Given that the SPI mode is mandatory for SDIO cards, why would you
> also implement another SPI mode with different commands?

Well, isn't this embedded world we're living in a constant race between 
software and hardware engineers, where the former invent more and more 
generic solutions to cover wider ranges of hardware with nice 
abstractions, while the latter invent more quirky hardware designs to 
destroy the effort of the former? Cute embedded nonsense hacks and so...

>>> Stepping aside from SPI, I already gave an example of a WLAN chip that
>>> supports multiple control busses [1]. In addition to the commonly used
>>> SDIO, it supports USB and HSIC as well:
>>>
>>> [1] http://www.marvell.com/wireless/assets/marvell_avastar_88w8797.pdf
>>>
>>> Moreover, some of Samsung boards use HSIC to communicate with modem
>>> chips, which have exactly the same problem as we're trying to solve here
>>> - they need to be powered on to be discovered.
>>
>> Thanks, this definitely makes a good example. I see that it also
>> supports SPI mode for SDIO as mentioned in your link.
>
> Well, USB is another discoverable bus.  As HSIC is a derivative of USB,
> I'd be surprised if it weren't discoverable there too.
>
> So, out of everything identified so far, we have no undiscoverable buses.
>
>> Agreed. Putting the same chip on USB or HSIC has the exact same
>> requirements, since we also have a discoverable bus, but actually
>> finding the device likely involves some power-on sequencing before
>> the bus controller can find it.
>
> That's partly the nature of integrating something onto a board where you
> want maximal power savings.  It's basically that dreaded word which
> software people seem to hate: "embedded".
>

This isn't something we can ignore, though.

>> * Arnd's proposal (change bus code to probe nonstandard devices
>>    from DT if we can't easily detect them):
>>    + Matches what we already do for PCI (at least on powerpc)
>>      and AMBA/Primecell devices: If a device can't be probed
>>      using the standard method, we treat it as nondiscoverable
>>      and describe it using DT.
>>    + Devices can have arbitrary complex requirements without
>>      impacting the core, since all code is contained in the
>>      driver for the nonstandard device.
>>    + Properties that are required for probing and runtime
>>      configuration only have to be set once (e.g. you may
>>      need clk_get() for probing and clk_set_rate() for
>>      runtime-pm).
>>    + Devices that have alternative bus interfaces like 88w8797
>>      can implement the power-on code in a central place per
>>      driver, and can reuse the code they have for nondiscoverable
>>      buses on the buses that are normally discoverable but
>>      broken here.
>>    - Still need to modify each subsystem to have alternate
>>      ways of probing, and match up devices later.
>>    - Has to be implemented in each driver that needs it, making
>>      it harder to share code for drivers with the same need
>>      (e.g. every device that just needs an external reset
>>      trigger).
>
>   - requires different DT if the chip is changed, which causes problems
>     for users to identify which out of zillions of DT files they should
>     use for their exact platform.

That's how things work in embedded world, unfortunately. As Arnd stated 
in one of his previous replies, different chips use to have different 
power-up sequences and set of data the driver needs to know.

If you change your hardware in an incompatible way and such change can't 
be detected automatically (i.e. the chip is non-discoverable) then I 
don't know how you could let the OS know about this change in any other 
way than providing it with the information it needs.

>   - have to work out how to match up the fake device with a probed device
>     when it becomes available: existing SDIO drivers all assume that the
>     card has been through a fairly complex initialisation sequence already.
>   - multi-function SDIO is much harder to deal with since you have mutliple
>     drivers involved, and the SDIO device as a whole needs initialisation
>     before anyone can drive it.
>   - adds complexity to the SDIO drivers; they would have to know whether
>     they're embedded or on a plug-in card.
>

The three points above could be eliminated by adopting the solution I 
proposed [1]. Moreover it could be analogically implemented for other 
bus types and use almost identical DT bindings.

[1] http://thread.gmane.org/gmane.linux.kernel.mmc/24728/focus=24864

Best regards,
Tomasz
Russell King - ARM Linux Feb. 15, 2014, 10:03 p.m. UTC | #47
On Sat, Feb 15, 2014 at 10:35:34PM +0100, Tomasz Figa wrote:
> If you change your hardware in an incompatible way and such change can't  
> be detected automatically (i.e. the chip is non-discoverable) then I  
> don't know how you could let the OS know about this change in any other  
> way than providing it with the information it needs.

I'm not talking about such major changes.  I'm talking about minor
changes where the SDIO device may change, but everything else stays
compatible.

Why should such a situation be forced down the path of having a different
DT file to specify the different SDIO device, when the SDIO device itself
is discoverable?  For example, BRCM4329 to a BRCM4330.

Look, let's think seriously about this.  Let's say we have a platform
where there's already two different DT files - one for models with a
single CPU, and a different DT file for a quad CPU.  This is fine,
users generally know which model they have, and can choose between
the two without much difficulty - because this information is part of
their decision making process when buying the product.

How, let's say that the SDIO chip becomes obsolete, and has to be
replaced later in the production run.  The external form, fit and
function remain the same, it's just the internals have now changed.

Now, if we go down Arnd's route, then we need to have not two DT files,
but now four.  One for single CPU with BRCM4329, quad CPU with the
same, and then those two with a BRCM4330.

Now, how does the user choose which DT file is right for their device?
Do they:

(a) ask the manufacturer, leading to hundreds of support requests.
(b) open the device up, invalidating the warranty to find out what chips
    are inside.
(c) something else (please specify)

What I'm trying to get here is the perspective from the other side - the
/user/ perspective, and the problems that making the wrong decision here
brings.  It has huge implications, and can make the difference between
"shall we use a mainline kernel, or shall we keep our own kernel/use an
ancient vendor's kernel with our own private hacks so we can reduce our
support costs".

I'm already seeing exactly this kind of problem on the horizon, because
I know of a platform where there was a bug in part of the hardware -
and enabling support for some interface features screws up on those
with the hardware bug.  So, there's already _four_ DT files to think
about for this hardware platform.  If Arnd's suggestion goes forward,
then we end up with _six_.  It doesn't take much to see where this
leads.

Meanwhile, these kinds of differences could be dealt with using
/board specific code/ in the old days to read on-SoC configuration and
adjust things appropriately - and transparently to the user.

DT is _our_ convenience as kernel hackers.  It is a big _inconvenience_
to the user when a particular product goes through a revision.

So, never ever forget that "oh, we can just deal with it with a different
DT file" makes our lives easier at the expense of our users, and without
users, we've lost.
Arnd Bergmann Feb. 17, 2014, 1 p.m. UTC | #48
On Saturday 15 February 2014, Russell King - ARM Linux wrote:
> On Sat, Feb 15, 2014 at 05:21:11PM +0100, Arnd Bergmann wrote:
> > On Saturday 15 February 2014 14:22:30 Tomasz Figa wrote:
> > > On 15.02.2014 14:09, Arnd Bergmann wrote:
> > >
> > > > For spi-mode SDIO devices I'm assuming it's similar, except that
> > > > you'd describe the actual SDIO device in the board info rather than
> > > > create a fake SDIO controller. Still not discoverable unless I'm
> > > > missing your point.
> > > 
> > > I'm not sure if we should assume that SPI = MMC over SPI. I believe 
> > > there might be a custom protocol involved as well.
> > 
> > In case of SD/MMC, you essentially have three separate command sets:
> > SPI, MMC and SD, and each of them has multiple versions. MMC and SD
> > compatible devices generally also support the SPI command set (IIRC
> > it is required,
> 
> SPI support is mandatory for SDIO as well.
> 
> SDIO has CIS (remember card information structures... like PCMCIA)
> which identifies the various different logical functions of the device,
> giving class information, vendor information etc.
> 
> So... certainly the type of the attached device is discoverable even
> on SPI

But how are SPI SDIO devices used in Linux and described in DT?

If we always use the MMC_SPI host controller driver and only
describe the controller in DT, then you are right, and there is
no difference to the normal SDIO case.

If we just describe it as an SPI device OTOH, the fact that we can
identify it is useless, because we first have to know what it
is before we can ask for its identity.

> > If a device supports both SDIO and SPI, I think a straightforward
> > implementation would be to use the exact same command set, but
> > you are right that this isn't the only possibility, and the SD/MMC
> > shows how they can be slightly different already.
> 
> Given that the SPI mode is mandatory for SDIO cards, why would you
> also implement another SPI mode with different commands?

I don't know why, I was just saying that it's what storage SD cards do.

> > > Stepping aside from SPI, I already gave an example of a WLAN chip that 
> > > supports multiple control busses [1]. In addition to the commonly used 
> > > SDIO, it supports USB and HSIC as well:
> > > 
> > > [1] http://www.marvell.com/wireless/assets/marvell_avastar_88w8797.pdf
> > > 
> > > Moreover, some of Samsung boards use HSIC to communicate with modem 
> > > chips, which have exactly the same problem as we're trying to solve here 
> > > - they need to be powered on to be discovered.
> > 
> > Thanks, this definitely makes a good example. I see that it also
> > supports SPI mode for SDIO as mentioned in your link.
> 
> Well, USB is another discoverable bus.  As HSIC is a derivative of USB,
> I'd be surprised if it weren't discoverable there too.
> 
> So, out of everything identified so far, we have no undiscoverable buses.

The device also lists a high-speed uart interface. While in Linux a
tty line discipline is not exactly the same as a driver, I would
still count it as a nondiscoverable interface.

>  - requires different DT if the chip is changed, which causes problems
>    for users to identify which out of zillions of DT files they should
>    use for their exact platform.

I'm still not buying this argument. Making any changes to the board
design that are visible to software can have this effect.

>  - have to work out how to match up the fake device with a probed device
>    when it becomes available: existing SDIO drivers all assume that the
>    card has been through a fairly complex initialisation sequence already.

This one is an implementation detail. Yes, we have to ensure it works,
but it's not inherently harder that getting the other approach to work.

>  - multi-function SDIO is much harder to deal with since you have mutliple
>    drivers involved, and the SDIO device as a whole needs initialisation
>    before anyone can drive it.

Agreed, this point is a big one that I hadn't considered before.
I agree that my approach isn't ideal for multifunction cards, especially
when you want to be able to load function drivers individually or have
only one of them in use. It can probably work fine for things like calling
clk_get() where you can simply describe the clock in each function device
node separately, but I don't see how I'd easily use a 'reset' line
that has to be triggered for the first function driver that gets loaded
but must not be triggered again for the second one :(

>  - adds complexity to the SDIO drivers; they would have to know whether
>    they're embedded or on a plug-in card.

How so? If a device can be a plug-in card, I would assume that it is
actually discoverable without a special power-on sequence, while a device
that needs this can't be used standalone.

	Arnd
Mark Brown Feb. 17, 2014, 11:25 p.m. UTC | #49
On Sat, Feb 15, 2014 at 05:21:11PM +0100, Arnd Bergmann wrote:
> On Saturday 15 February 2014 14:22:30 Tomasz Figa wrote:

> > I'm not sure if we should assume that SPI = MMC over SPI. I believe 
> > there might be a custom protocol involved as well.

> In case of SD/MMC, you essentially have three separate command sets:
> SPI, MMC and SD, and each of them has multiple versions. MMC and SD
> compatible devices generally also support the SPI command set (IIRC

...

> If a device supports both SDIO and SPI, I think a straightforward
> implementation would be to use the exact same command set, but
> you are right that this isn't the only possibility, and the SD/MMC
> shows how they can be slightly different already.

I'm aware of existing devices that do in fact break this assumption.

> > Stepping aside from SPI, I already gave an example of a WLAN chip that 
> > supports multiple control busses [1]. In addition to the commonly used 
> > SDIO, it supports USB and HSIC as well:

> > [1] http://www.marvell.com/wireless/assets/marvell_avastar_88w8797.pdf

> > Moreover, some of Samsung boards use HSIC to communicate with modem 
> > chips, which have exactly the same problem as we're trying to solve here 
> > - they need to be powered on to be discovered.

> Thanks, this definitely makes a good example. I see that it also
> supports SPI mode for SDIO as mentioned in your link.

Slimbus has all these issues too with the added fun that for normal
operation some devices want to be in a low power mode where they're
disconnected from the Slimbus a lot of the time.

> * Olof's proposal (add properties or a child node to the host
>   controller node with just power-on sequencing information):
>   + We only need one implementation for each bus, possibly shared
>     across buses to some degree, and can handle lots of devices
>     without having to touch their individual drivers.
>   + A logical extension of things we already do on SD cards
>     (CD/WP GPIOs, external clocks and voltages supplied to
>     standard compliant devices as part of the normal probing)
>   - The shared code may get rather complex to deal with all
>     possible corner cases we run into over the years.
>   - Somewhat harder to do if you have to attach the power
>     information to a device node for a USB hub port, rather
>     than an SDIO controller that only has one slave device.

We would also need mechanisms to allow devices to take over the running
of their own resources for cases like the Slimbus ones I mentioned, and
some mechanism to cope with devices that hotplug themselves in normal
operation.

> * Arnd's proposal (change bus code to probe nonstandard devices
>   from DT if we can't easily detect them):

I've also proposed this in the past FWIW but never got far enough
through my list of things I want to do with my subsystems to actually
start coding.

>   - Has to be implemented in each driver that needs it, making
>     it harder to share code for drivers with the same need
>     (e.g. every device that just needs an external reset
>     trigger).

I think we can probably come up with some standard helpers that work
well for the common case here (see also some of the discussions about
power domains ensuring that core IP clocks are provided for IPs, it's
kind of circling back to the same issues).
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index 458b57f..962e0ee 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -5,6 +5,8 @@  these definitions.
 Interpreted by the OF core:
 - reg: Registers location and length.
 - interrupts: Interrupts used by the MMC controller.
+- clocks: Clocks needed for the host controller, if any.
+- clock-names: Goes with clocks above.
 
 Card detection:
 If no property below is supplied, host native card detect is used.
@@ -30,6 +32,15 @@  Optional properties:
 - cap-sdio-irq: enable SDIO IRQ signalling on this interface
 - full-pwr-cycle: full power cycle of the card is supported
 
+Card power and reset control:
+The following properties can be specified for cases where the MMC
+peripheral needs additional reset, regulator and clock lines. It is for
+example common for WiFi/BT adapters to have these separate from the main
+MMC bus:
+  - card-reset-gpios: Specify GPIOs for card reset (reset active low)
+  - card-external-vcc-supply: Regulator to drive (independent) card VCC
+  - clock with name "card_ext_clock": External clock provided to the card
+
 *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
 polarity properties, we have to fix the meaning of the "normal" and "inverted"
 line levels. We choose to follow the SDHCI standard, which specifies both those
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 098374b..c43e6c8 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -13,11 +13,13 @@ 
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/delay.h>
 #include <linux/pagemap.h>
 #include <linux/err.h>
+#include <linux/gpio.h>
 #include <linux/leds.h>
 #include <linux/scatterlist.h>
 #include <linux/log2.h>
@@ -1519,6 +1521,43 @@  void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type)
 	mmc_host_clk_release(host);
 }
 
+static void mmc_card_power_up(struct mmc_host *host)
+{
+	int i;
+	struct gpio_desc **gds = host->card_reset_gpios;
+
+	for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
+		if (gds[i]) {
+			dev_dbg(host->parent, "Asserting reset line %d", i);
+			gpiod_set_value(gds[i], 1);
+		}
+	}
+
+	if (host->card_regulator) {
+		dev_dbg(host->parent, "Enabling external regulator");
+		if (regulator_enable(host->card_regulator))
+			dev_err(host->parent, "Failed to enable external regulator");
+	}
+
+	if (host->card_clk) {
+		dev_dbg(host->parent, "Enabling external clock");
+		clk_prepare_enable(host->card_clk);
+	}
+
+	/* 2ms delay to let clocks and power settle */
+	mmc_delay(20);
+
+	for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
+		if (gds[i]) {
+			dev_dbg(host->parent, "Deasserting reset line %d", i);
+			gpiod_set_value(gds[i], 0);
+		}
+	}
+
+	/* 2ms delay to after reset release */
+	mmc_delay(20);
+}
+
 /*
  * Apply power to the MMC stack.  This is a two-stage process.
  * First, we enable power to the card without the clock running.
@@ -1535,6 +1574,9 @@  void mmc_power_up(struct mmc_host *host, u32 ocr)
 	if (host->ios.power_mode == MMC_POWER_ON)
 		return;
 
+	/* Power up the card/module first, if needed */
+	mmc_card_power_up(host);
+
 	mmc_host_clk_hold(host);
 
 	host->ios.vdd = fls(ocr) - 1;
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 49bc403..e6b850b 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -12,14 +12,18 @@ 
  *  MMC host class device management
  */
 
+#include <linux/kernel.h>
+#include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/idr.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/pagemap.h>
 #include <linux/export.h>
 #include <linux/leds.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
 
@@ -312,7 +316,7 @@  int mmc_of_parse(struct mmc_host *host)
 	u32 bus_width;
 	bool explicit_inv_wp, gpio_inv_wp = false;
 	enum of_gpio_flags flags;
-	int len, ret, gpio;
+	int i, len, ret, gpio;
 
 	if (!host->parent || !host->parent->of_node)
 		return 0;
@@ -415,6 +419,30 @@  int mmc_of_parse(struct mmc_host *host)
 	if (explicit_inv_wp ^ gpio_inv_wp)
 		host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
 
+	/* Parse card power/reset/clock control */
+	if (of_find_property(np, "card-reset-gpios", NULL)) {
+		struct gpio_desc *gpd;
+		for (i = 0; i < ARRAY_SIZE(host->card_reset_gpios); i++) {
+			gpd = devm_gpiod_get_index(host->parent, "card-reset", i);
+			if (IS_ERR(gpd))
+				break;
+			gpiod_direction_output(gpd, 0);
+			host->card_reset_gpios[i] = gpd;
+		}
+
+		gpd = devm_gpiod_get_index(host->parent, "card-reset", ARRAY_SIZE(host->card_reset_gpios));
+		if (!IS_ERR(gpd)) {
+			dev_warn(host->parent, "More reset gpios than we can handle");
+			gpiod_put(gpd);
+		}
+	}
+
+	host->card_clk = of_clk_get_by_name(np, "card_ext_clock");
+	if (IS_ERR(host->card_clk))
+		host->card_clk = NULL;
+
+	host->card_regulator = regulator_get(host->parent, "card-external-vcc");
+
 	if (of_find_property(np, "cap-sd-highspeed", &len))
 		host->caps |= MMC_CAP_SD_HIGHSPEED;
 	if (of_find_property(np, "cap-mmc-highspeed", &len))
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 99f5709..6781887 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -297,6 +297,11 @@  struct mmc_host {
 	unsigned long           clkgate_delay;
 #endif
 
+	/* card specific properties to deal with power and reset */
+	struct regulator	*card_regulator; /* External VCC needed by the card */
+	struct gpio_desc	*card_reset_gpios[2]; /* External resets, active low */
+	struct clk		*card_clk;	/* External clock needed by the card */
+
 	/* host specific block data */
 	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
 	unsigned short		max_segs;	/* see blk_queue_max_segments */