| Message ID | 1390190215-22700-2-git-send-email-olof@lixom.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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...
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
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.
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
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
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
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
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
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
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
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
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
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
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
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 */ >
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.
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
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
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. :/
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
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
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
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
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
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
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
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.
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
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
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).
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
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
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.
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
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.
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
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
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
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.
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
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
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
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.
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
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.
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
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 --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 */
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(-)