RFC: representing sdio devices oob interrupt, clks, etc. in device tree

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi All,

Arend asked me to test these 2 patches for adding devicetree support to brcmfmac sdio devices:
"dt: bindings: add bindings for Broadcom bcm43xx sdio devices"
"brcmfmac: add device tree support for SDIO devices"
https://groups.google.com/forum/#!msg/linux-sunxi/Zph6zDTnAcw/_-wOO-gnIuQJ

Getting this to do anything at all meant also adding this patch:

"mmc: Add SDIO function devicetree subnode parsing"
https://lkml.org/lkml/2014/4/3/522

So that the card / sdio-func devices would actually get their of_node
pointer set to the child node under the mmc controller describing
the sdio wifi module.

But that does not solve the entire problem. Listing the oob-interrupt info
in the child node works fine. But listing the brcm,wlan-supply gpio does
not. Since these sdio wifi modules are typically soldered onto the
board, there mmc controller device tree node has non-removable; set,
so the mmc subsystem will try to initialize the sdio device as soon
as the mmc driver loads, and if that fails will never look at it again.

So we need to have the brcm,wlan-supply gpio driven high *before* the mmc
host driver initializes. In case of the brcm,wlan-supply property, this
is actually a problem which we've already solved in a number of dts files
for allwinner boards, simply create a fixed-regulator with an enable/disable
gpio, and set that as vmmc-supply for the mmc-host for the sdio wifi.

However the "brcmfmac: add device tree support for SDIO devices" patch
also adds support for 2 devicetree controlled clocks. Assuming these need
to be ungated before the sdio module will respond to mmc commands, we have
the same problem.

I've been thinking a bit about this, and it is a non trivial problem since
sdio devices are normally instantiated when probed, unlike ie spi devices
which are instantiated from devicetree.

Adding device tree instantiation of sdio devices seems like a bad idea, as
then we get 2 vastly different device instantiation paths. Still we need some
way to get power and clks setup before the mmc host initializes.

Therefor I would like to suggest to add a number of standard properties to
mmc-bus child nodes. Making the dts for an mmc host with an sdio device which
needs clks setup before it will work look like this:

                mmc3: mmc at 01c12000 {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        pinctrl-names = "default";
                        pinctrl-0 = <&mmc3_pins_a>;
                        vmmc-supply = <&reg_vmmc3>;
                        bus-width = <4>;
                        non-removable;
                        status = "okay";

                        brcmf: bcrmf at 0 {
                                reg = <0>;
                                compatible = "brcm,bcm43xx-fmac";
				clocks = <&clk_out_a>, <&clk_out_b>;
				clock-frequency = <32768>, <26000000>;
                                interrupt-parent = <&pio>;
                                interrupts = <10 4>; /* PH10 / EINT10 */
                                interrupt-names = "host-wake";
                                status = "okay";
                        };
                };

Where the "clocks" and "clock-frequency" attributes would be something
which we standardize in Documentation/devicetree/bindings/mmc/mmc-bus.txt

These standard properties would *not* be used by the driver for the
childnode device, so as to avoid the chicken and egg problem of that driver
needing to enable clks, and it only getting bound to the device after
the device has been discovered which requires those clocks.

Instead these properties would be parsed by mmc_of_parse, and we will
get enabled automatically by mmc_add_host before doing any probing.

If the optional clock-frequency property is also set, then mmc_add_host
will not only enable the clks but also set them to the specified
frequency.

Note I've no boards which actually need the clocks, all 3 boards with
sdio wifi which I've use a 26MHz crystal directly attached to the module,
so once I get the oob interrupt working (this needs some work on
the allwinner side) we can merge Arend's patches with the clock bits
dropped for now.

Still I want to get a discussion going on this, so that when the time
comes that we do need it we know how to deal with this, or ideally
already have code in place to deal with it.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Chen-Yu Tsai]

Hi,

On Thu, May 22, 2014 at 5:49 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi All,
>
> Arend asked me to test these 2 patches for adding devicetree support to brcmfmac sdio devices:
> "dt: bindings: add bindings for Broadcom bcm43xx sdio devices"
> "brcmfmac: add device tree support for SDIO devices"
> https://groups.google.com/forum/#!msg/linux-sunxi/Zph6zDTnAcw/_-wOO-gnIuQJ
>
> Getting this to do anything at all meant also adding this patch:
>
> "mmc: Add SDIO function devicetree subnode parsing"
> https://lkml.org/lkml/2014/4/3/522
>
> So that the card / sdio-func devices would actually get their of_node
> pointer set to the child node under the mmc controller describing
> the sdio wifi module.
>
> But that does not solve the entire problem. Listing the oob-interrupt info
> in the child node works fine. But listing the brcm,wlan-supply gpio does
> not. Since these sdio wifi modules are typically soldered onto the
> board, there mmc controller device tree node has non-removable; set,
> so the mmc subsystem will try to initialize the sdio device as soon
> as the mmc driver loads, and if that fails will never look at it again.
>
> So we need to have the brcm,wlan-supply gpio driven high *before* the mmc
> host driver initializes. In case of the brcm,wlan-supply property, this
> is actually a problem which we've already solved in a number of dts files
> for allwinner boards, simply create a fixed-regulator with an enable/disable
> gpio, and set that as vmmc-supply for the mmc-host for the sdio wifi.
>
> However the "brcmfmac: add device tree support for SDIO devices" patch
> also adds support for 2 devicetree controlled clocks. Assuming these need
> to be ungated before the sdio module will respond to mmc commands, we have
> the same problem.

Olof posted an RFC series on this very matter:

  http://www.spinics.net/lists/linux-mmc/msg24411.html

There was a lengthy discussion, however I don't think the matter was settled.

CC-ed everyone from that discussion.

> I've been thinking a bit about this, and it is a non trivial problem since
> sdio devices are normally instantiated when probed, unlike ie spi devices
> which are instantiated from devicetree.
>
> Adding device tree instantiation of sdio devices seems like a bad idea, as
> then we get 2 vastly different device instantiation paths. Still we need some
> way to get power and clks setup before the mmc host initializes.
>
> Therefor I would like to suggest to add a number of standard properties to
> mmc-bus child nodes. Making the dts for an mmc host with an sdio device which
> needs clks setup before it will work look like this:
>
>                 mmc3: mmc at 01c12000 {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                         pinctrl-names = "default";
>                         pinctrl-0 = <&mmc3_pins_a>;
>                         vmmc-supply = <&reg_vmmc3>;
>                         bus-width = <4>;
>                         non-removable;
>                         status = "okay";
>
>                         brcmf: bcrmf at 0 {
>                                 reg = <0>;
>                                 compatible = "brcm,bcm43xx-fmac";
>                                 clocks = <&clk_out_a>, <&clk_out_b>;
>                                 clock-frequency = <32768>, <26000000>;
>                                 interrupt-parent = <&pio>;
>                                 interrupts = <10 4>; /* PH10 / EINT10 */
>                                 interrupt-names = "host-wake";
>                                 status = "okay";
>                         };
>                 };
>
> Where the "clocks" and "clock-frequency" attributes would be something
> which we standardize in Documentation/devicetree/bindings/mmc/mmc-bus.txt
>
> These standard properties would *not* be used by the driver for the
> childnode device, so as to avoid the chicken and egg problem of that driver
> needing to enable clks, and it only getting bound to the device after
> the device has been discovered which requires those clocks.
>
> Instead these properties would be parsed by mmc_of_parse, and we will
> get enabled automatically by mmc_add_host before doing any probing.
>
> If the optional clock-frequency property is also set, then mmc_add_host
> will not only enable the clks but also set them to the specified
> frequency.
>
> Note I've no boards which actually need the clocks, all 3 boards with
> sdio wifi which I've use a 26MHz crystal directly attached to the module,
> so once I get the oob interrupt working (this needs some work on
> the allwinner side) we can merge Arend's patches with the clock bits
> dropped for now.
>
> Still I want to get a discussion going on this, so that when the time
> comes that we do need it we know how to deal with this, or ideally
> already have code in place to deal with it.


Cheers
ChenYu

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/22/2014 12:23 PM, Chen-Yu Tsai wrote:
> Hi,
> 
> On Thu, May 22, 2014 at 5:49 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi All,
>>
>> Arend asked me to test these 2 patches for adding devicetree support to brcmfmac sdio devices:
>> "dt: bindings: add bindings for Broadcom bcm43xx sdio devices"
>> "brcmfmac: add device tree support for SDIO devices"
>> https://groups.google.com/forum/#!msg/linux-sunxi/Zph6zDTnAcw/_-wOO-gnIuQJ
>>
>> Getting this to do anything at all meant also adding this patch:
>>
>> "mmc: Add SDIO function devicetree subnode parsing"
>> https://lkml.org/lkml/2014/4/3/522
>>
>> So that the card / sdio-func devices would actually get their of_node
>> pointer set to the child node under the mmc controller describing
>> the sdio wifi module.
>>
>> But that does not solve the entire problem. Listing the oob-interrupt info
>> in the child node works fine. But listing the brcm,wlan-supply gpio does
>> not. Since these sdio wifi modules are typically soldered onto the
>> board, there mmc controller device tree node has non-removable; set,
>> so the mmc subsystem will try to initialize the sdio device as soon
>> as the mmc driver loads, and if that fails will never look at it again.
>>
>> So we need to have the brcm,wlan-supply gpio driven high *before* the mmc
>> host driver initializes. In case of the brcm,wlan-supply property, this
>> is actually a problem which we've already solved in a number of dts files
>> for allwinner boards, simply create a fixed-regulator with an enable/disable
>> gpio, and set that as vmmc-supply for the mmc-host for the sdio wifi.
>>
>> However the "brcmfmac: add device tree support for SDIO devices" patch
>> also adds support for 2 devicetree controlled clocks. Assuming these need
>> to be ungated before the sdio module will respond to mmc commands, we have
>> the same problem.
> 
> Olof posted an RFC series on this very matter:
> 
>   http://www.spinics.net/lists/linux-mmc/msg24411.html
> 
> There was a lengthy discussion, however I don't think the matter was settled.

Phew that was a long read. Indeed it seems that the matter was not
entirely settled.  It seems that the discussion did gravitate towards this
information belonging in child nodes under the host controller node to represent
the device.

I think we need to do this step by step:

1) Right now, for my use case, I really only care about oob interrupts.

If we can agree that we for additional info for sdio devices we will
be using a child node under the mmc host with #address-cells = <1>;
#size-cells = <0>; and reg == function number, iow go with Sacha's
patch to attach an of node to sdio device functions, then that use case
is covered.

I think this will cover quite a few use-cases already. and it should not
get in the way of adding fancier stuff, up to doing the entire device
instantation statically based on the child node, later.

2) Note that my proposal for the clks is very much like Olof's proposal
in that it tries to keep things KISS, but instead of adding the info
to the mmc-host node it adds it to the sdio-func child nodes. We could
ask someone who has an actual use case where power-sequencing with clks
and/or resets is needed to first implement things along Olof's proposal,
but then with the clks and resets using standardized properties inside the
child nodes. This would keep things KISS, again without getting in the
way of getting fancier later.

3) If someone has a usecase where 2 does not work or become ugly, we can
then ask that someone to come up with a proposal for something better.

Regards,

Hans



> 
> CC-ed everyone from that discussion.
> 
>> I've been thinking a bit about this, and it is a non trivial problem since
>> sdio devices are normally instantiated when probed, unlike ie spi devices
>> which are instantiated from devicetree.
>>
>> Adding device tree instantiation of sdio devices seems like a bad idea, as
>> then we get 2 vastly different device instantiation paths. Still we need some
>> way to get power and clks setup before the mmc host initializes.
>>
>> Therefor I would like to suggest to add a number of standard properties to
>> mmc-bus child nodes. Making the dts for an mmc host with an sdio device which
>> needs clks setup before it will work look like this:
>>
>>                 mmc3: mmc at 01c12000 {
>>                         #address-cells = <1>;
>>                         #size-cells = <0>;
>>                         pinctrl-names = "default";
>>                         pinctrl-0 = <&mmc3_pins_a>;
>>                         vmmc-supply = <&reg_vmmc3>;
>>                         bus-width = <4>;
>>                         non-removable;
>>                         status = "okay";
>>
>>                         brcmf: bcrmf at 0 {
>>                                 reg = <0>;
>>                                 compatible = "brcm,bcm43xx-fmac";
>>                                 clocks = <&clk_out_a>, <&clk_out_b>;
>>                                 clock-frequency = <32768>, <26000000>;
>>                                 interrupt-parent = <&pio>;
>>                                 interrupts = <10 4>; /* PH10 / EINT10 */
>>                                 interrupt-names = "host-wake";
>>                                 status = "okay";
>>                         };
>>                 };
>>
>> Where the "clocks" and "clock-frequency" attributes would be something
>> which we standardize in Documentation/devicetree/bindings/mmc/mmc-bus.txt
>>
>> These standard properties would *not* be used by the driver for the
>> childnode device, so as to avoid the chicken and egg problem of that driver
>> needing to enable clks, and it only getting bound to the device after
>> the device has been discovered which requires those clocks.
>>
>> Instead these properties would be parsed by mmc_of_parse, and we will
>> get enabled automatically by mmc_add_host before doing any probing.
>>
>> If the optional clock-frequency property is also set, then mmc_add_host
>> will not only enable the clks but also set them to the specified
>> frequency.
>>
>> Note I've no boards which actually need the clocks, all 3 boards with
>> sdio wifi which I've use a 26MHz crystal directly attached to the module,
>> so once I get the oob interrupt working (this needs some work on
>> the allwinner side) we can merge Arend's patches with the clock bits
>> dropped for now.
>>
>> Still I want to get a discussion going on this, so that when the time
>> comes that we do need it we know how to deal with this, or ideally
>> already have code in place to deal with it.
> 
> 
> Cheers
> ChenYu
> 

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Tomasz Figa]

Hi,


On 22.05.2014 13:38, Hans de Goede wrote:
> On 05/22/2014 12:23 PM, Chen-Yu Tsai wrote:
>> On Thu, May 22, 2014 at 5:49 PM, Hans de Goede <hdegoede@redhat.com> wrote:

snip

>>> I've been thinking a bit about this, and it is a non trivial problem since
>>> sdio devices are normally instantiated when probed, unlike ie spi devices
>>> which are instantiated from devicetree.
>>>
>>> Adding device tree instantiation of sdio devices seems like a bad idea, as
>>> then we get 2 vastly different device instantiation paths. Still we need some
>>> way to get power and clks setup before the mmc host initializes.

What about introducing some extra callbacks to mmc drivers to build
driver data and control power?

struct mmc_legacy_device {
	/* ... */
	struct device_node *of_node;
	void *driver_data;
};

struct mmc_driver {
	/* ... */
	int (*prepare)(struct mmc_legacy_device *);
	void (*unprepare)(struct mmc_legacy_device *);
};

static int my_wlan_prepare(struct mmc_legacy_device *dev)
{
	struct my_wlan_data *data;

	data = kzalloc(...);
	/* ... */

	of_*();
	clk_*();
	regulator_*();
	/* ... */

	ret = my_wlan_power_on(data);
	/* ... */

	dev->driver_data = data;	
}

static void my_wlan_unprepare(struct mmc_legacy_device *dev)
{
	struct my_wlan_data *data;

	data = dev->driver_data;

	my_wlan_power_off(data);
	/* ... */

	dev->driver_data = NULL;
	kfree(data);
}

I don't really like this, especially the fact that this would be highly
MMC-specific, while there are other interfaces that also need this
problem solved, e.g. USB/HSIC.

I had proposed another solution that would require almost no changes in
MMC core and could work for any data interface. You can find it
somewhere in the thread mentioned by Chen-Yu. Unfortunately, for reasons
that don't appeal to me, it wasn't positively received by MMC people.

>>>
>>> Therefor I would like to suggest to add a number of standard properties to
>>> mmc-bus child nodes. Making the dts for an mmc host with an sdio device which
>>> needs clks setup before it will work look like this:
>>>
>>>                 mmc3: mmc at 01c12000 {
>>>                         #address-cells = <1>;
>>>                         #size-cells = <0>;
>>>                         pinctrl-names = "default";
>>>                         pinctrl-0 = <&mmc3_pins_a>;
>>>                         vmmc-supply = <&reg_vmmc3>;
>>>                         bus-width = <4>;
>>>                         non-removable;
>>>                         status = "okay";
>>>
>>>                         brcmf: bcrmf at 0 {
>>>                                 reg = <0>;
>>>                                 compatible = "brcm,bcm43xx-fmac";
>>>                                 clocks = <&clk_out_a>, <&clk_out_b>;
>>>                                 clock-frequency = <32768>, <26000000>;
>>>                                 interrupt-parent = <&pio>;
>>>                                 interrupts = <10 4>; /* PH10 / EINT10 */
>>>                                 interrupt-names = "host-wake";
>>>                                 status = "okay";
>>>                         };
>>>                 };
>>>
>>> Where the "clocks" and "clock-frequency" attributes would be something
>>> which we standardize in Documentation/devicetree/bindings/mmc/mmc-bus.txt

I like the binding. It has all properties defined where they belong.

>>>
>>> These standard properties would *not* be used by the driver for the
>>> childnode device, so as to avoid the chicken and egg problem of that driver
>>> needing to enable clks, and it only getting bound to the device after
>>> the device has been discovered which requires those clocks.
>>>
>>> Instead these properties would be parsed by mmc_of_parse, and we will
>>> get enabled automatically by mmc_add_host before doing any probing.
>>>
>>> If the optional clock-frequency property is also set, then mmc_add_host
>>> will not only enable the clks but also set them to the specified
>>> frequency.
>>>

I don't like power control and parsing in MMC core, because they are
highly chip-specific. Throwing this kind of knowledge to MMC subsystem
violates separation and just won't scale as more different WLAN chips
show up. However, this is at least not enforced by design of the
bindings and so can be changed at any time in future, while having
something already working.

See above for my take on other ways of handling this.

>>> Note I've no boards which actually need the clocks, all 3 boards with
>>> sdio wifi which I've use a 26MHz crystal directly attached to the module,
>>> so once I get the oob interrupt working (this needs some work on
>>> the allwinner side) we can merge Arend's patches with the clock bits
>>> dropped for now.

Exynos4210-based TRATS and Exynos4412-based TRATS2 boards already
supported by mainline kernel have WLAN 32k clock driven by PMIC and
needs to be ungated. For the 26M clock a normal crystal is used as well
as in your case.

Best regards,
Tomasz

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/22/2014 07:20 PM, Tomasz Figa wrote:
> Hi,
> 
> 
> On 22.05.2014 13:38, Hans de Goede wrote:
>> On 05/22/2014 12:23 PM, Chen-Yu Tsai wrote:
>>> On Thu, May 22, 2014 at 5:49 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
> snip
> 
>>>> I've been thinking a bit about this, and it is a non trivial problem since
>>>> sdio devices are normally instantiated when probed, unlike ie spi devices
>>>> which are instantiated from devicetree.
>>>>
>>>> Adding device tree instantiation of sdio devices seems like a bad idea, as
>>>> then we get 2 vastly different device instantiation paths. Still we need some
>>>> way to get power and clks setup before the mmc host initializes.
> 
> What about introducing some extra callbacks to mmc drivers to build
> driver data and control power?
> 
> struct mmc_legacy_device {
> 	/* ... */
> 	struct device_node *of_node;
> 	void *driver_data;
> };
> 
> struct mmc_driver {
> 	/* ... */
> 	int (*prepare)(struct mmc_legacy_device *);
> 	void (*unprepare)(struct mmc_legacy_device *);
> };
> 
> static int my_wlan_prepare(struct mmc_legacy_device *dev)
> {
> 	struct my_wlan_data *data;
> 
> 	data = kzalloc(...);
> 	/* ... */
> 
> 	of_*();
> 	clk_*();
> 	regulator_*();
> 	/* ... */
> 
> 	ret = my_wlan_power_on(data);
> 	/* ... */
> 
> 	dev->driver_data = data;	
> }
> 
> static void my_wlan_unprepare(struct mmc_legacy_device *dev)
> {
> 	struct my_wlan_data *data;
> 
> 	data = dev->driver_data;
> 
> 	my_wlan_power_off(data);
> 	/* ... */
> 
> 	dev->driver_data = NULL;
> 	kfree(data);
> }
> 
> I don't really like this, especially the fact that this would be highly
> MMC-specific, while there are other interfaces that also need this
> problem solved, e.g. USB/HSIC.

I don't really like this either :)  My preferred approach is to just go
with the most KISS approach for now, and slowly make things more complex
as needed.

> 
> I had proposed another solution that would require almost no changes in
> MMC core and could work for any data interface. You can find it
> somewhere in the thread mentioned by Chen-Yu. Unfortunately, for reasons
> that don't appeal to me, it wasn't positively received by MMC people.
> 
>>>>
>>>> Therefor I would like to suggest to add a number of standard properties to
>>>> mmc-bus child nodes. Making the dts for an mmc host with an sdio device which
>>>> needs clks setup before it will work look like this:
>>>>
>>>>                 mmc3: mmc at 01c12000 {
>>>>                         #address-cells = <1>;
>>>>                         #size-cells = <0>;
>>>>                         pinctrl-names = "default";
>>>>                         pinctrl-0 = <&mmc3_pins_a>;
>>>>                         vmmc-supply = <&reg_vmmc3>;
>>>>                         bus-width = <4>;
>>>>                         non-removable;
>>>>                         status = "okay";
>>>>
>>>>                         brcmf: bcrmf at 0 {
>>>>                                 reg = <0>;
>>>>                                 compatible = "brcm,bcm43xx-fmac";
>>>>                                 clocks = <&clk_out_a>, <&clk_out_b>;
>>>>                                 clock-frequency = <32768>, <26000000>;
>>>>                                 interrupt-parent = <&pio>;
>>>>                                 interrupts = <10 4>; /* PH10 / EINT10 */
>>>>                                 interrupt-names = "host-wake";
>>>>                                 status = "okay";
>>>>                         };
>>>>                 };
>>>>
>>>> Where the "clocks" and "clock-frequency" attributes would be something
>>>> which we standardize in Documentation/devicetree/bindings/mmc/mmc-bus.txt
> 
> I like the binding. It has all properties defined where they belong.

I'm glad you like the binding, I really want us to work towards a binding
where we all agree on the binding bits, so that we can set those in stone,
then we can go with a simple implementation for now, and move to something
more complex as needed.

> 
>>>>
>>>> These standard properties would *not* be used by the driver for the
>>>> childnode device, so as to avoid the chicken and egg problem of that driver
>>>> needing to enable clks, and it only getting bound to the device after
>>>> the device has been discovered which requires those clocks.
>>>>
>>>> Instead these properties would be parsed by mmc_of_parse, and we will
>>>> get enabled automatically by mmc_add_host before doing any probing.
>>>>
>>>> If the optional clock-frequency property is also set, then mmc_add_host
>>>> will not only enable the clks but also set them to the specified
>>>> frequency.
>>>>
> 
> I don't like power control and parsing in MMC core, because they are
> highly chip-specific. Throwing this kind of knowledge to MMC subsystem
> violates separation and just won't scale as more different WLAN chips
> show up. However, this is at least not enforced by design of the
> bindings and so can be changed at any time in future, while having
> something already working.

Right, the whole idea is to add some simple power up logic to the mmc
core, as with Olof's original proposal, but make things so that later
on we can do something more complex as needed.

Also note that adding this to the mmc-core has the advantage that
we don't need to add power-up logic to each sdio driver separately.

Thinking more about this, I would like to make one change to my
proposal, the mmc-core should only do power up of child-nodes if
they have a compatible of: "simple-sdio-powerup". This way
when we add something more complex, we can keep the simple powerup
code in the mmc core, keeping what we've already using this working
and the mmc core won't respond to the child nodes for more complex
devices, so it won't conflict with more complex power-up handling
handled by some other driver.

FWIW if we ever get truely complex cases I think modeling the
power-up hardware as a pmic platform device is not a bad idea,
we would then need to have a generic mmc-host pmic property, which
would be used both to do the initial powerup before scanning, as
well as for the sdio device driver to get a handle to the pmic,
for run time power-management (if desired).

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Mark Brown]

On Fri, May 23, 2014 at 11:13:44AM +0200, Hans de Goede wrote:

> Thinking more about this, I would like to make one change to my
> proposal, the mmc-core should only do power up of child-nodes if
> they have a compatible of: "simple-sdio-powerup". This way
> when we add something more complex, we can keep the simple powerup
> code in the mmc core, keeping what we've already using this working
> and the mmc core won't respond to the child nodes for more complex
> devices, so it won't conflict with more complex power-up handling
> handled by some other driver.

Would it not be better to have this be something in the driver struct
rather than in the device tree?  Putting a compatible in there would be
encoding details of the Linux implementation in the DT which doesn't
seem right especially since these are details we're thinking of changing
later on.  Something like have the driver set flags saying if it wants
to do complicated things.

> FWIW if we ever get truely complex cases I think modeling the
> power-up hardware as a pmic platform device is not a bad idea,
> we would then need to have a generic mmc-host pmic property, which
> would be used both to do the initial powerup before scanning, as
> well as for the sdio device driver to get a handle to the pmic,
> for run time power-management (if desired).

I don't know if this will ever apply to SDIO but with other buses the
complicated bits come when the driver wants to take over some of the
power management do things like turn some of the supplies or clocks on
and off independently at runtime for low power modes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140523/7f8f7960/attachment.sig>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/23/2014 01:22 PM, Mark Brown wrote:
> On Fri, May 23, 2014 at 11:13:44AM +0200, Hans de Goede wrote:
> 
>> Thinking more about this, I would like to make one change to my
>> proposal, the mmc-core should only do power up of child-nodes if
>> they have a compatible of: "simple-sdio-powerup". This way
>> when we add something more complex, we can keep the simple powerup
>> code in the mmc core, keeping what we've already using this working
>> and the mmc core won't respond to the child nodes for more complex
>> devices, so it won't conflict with more complex power-up handling
>> handled by some other driver.
> 
> Would it not be better to have this be something in the driver struct
> rather than in the device tree?  Putting a compatible in there would be
> encoding details of the Linux implementation in the DT which doesn't
> seem right especially since these are details we're thinking of changing
> later on.

The compatible is not a Linux specific thing, it is a marking saying
that something needs to take care of enabling the clks (and whatever
else we will make part of the binding for this compatible), before
scanning the mmc bus.

> Something like have the driver set flags saying if it wants
> to do complicated things.

Chicken <-> egg, we won't know which driver to use before we've probed
the mmc bus, and we cannot probe the bus before enabling the clks, etc.

>> FWIW if we ever get truely complex cases I think modeling the
>> power-up hardware as a pmic platform device is not a bad idea,
>> we would then need to have a generic mmc-host pmic property, which
>> would be used both to do the initial powerup before scanning, as
>> well as for the sdio device driver to get a handle to the pmic,
>> for run time power-management (if desired).
> 
> I don't know if this will ever apply to SDIO but with other buses the
> complicated bits come when the driver wants to take over some of the
> power management do things like turn some of the supplies or clocks on
> and off independently at runtime for low power modes.

Hmm, good point in that case actually having these things in the
child node makes most sense, because then the driver can find them
their. Note that the mmc core enabling things does not mean that
the driver cannot later disable them if needed.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Arend van Spriel]

On 05/23/14 13:50, Hans de Goede wrote:
> Hi,
>
> On 05/23/2014 01:22 PM, Mark Brown wrote:
>> On Fri, May 23, 2014 at 11:13:44AM +0200, Hans de Goede wrote:
>>
>>> Thinking more about this, I would like to make one change to my
>>> proposal, the mmc-core should only do power up of child-nodes if
>>> they have a compatible of: "simple-sdio-powerup". This way
>>> when we add something more complex, we can keep the simple powerup
>>> code in the mmc core, keeping what we've already using this working
>>> and the mmc core won't respond to the child nodes for more complex
>>> devices, so it won't conflict with more complex power-up handling
>>> handled by some other driver.
>>
>> Would it not be better to have this be something in the driver struct
>> rather than in the device tree?  Putting a compatible in there would be
>> encoding details of the Linux implementation in the DT which doesn't
>> seem right especially since these are details we're thinking of changing
>> later on.
>
> The compatible is not a Linux specific thing, it is a marking saying
> that something needs to take care of enabling the clks (and whatever
> else we will make part of the binding for this compatible), before
> scanning the mmc bus.
>
>> Something like have the driver set flags saying if it wants
>> to do complicated things.
>
> Chicken<->  egg, we won't know which driver to use before we've probed
> the mmc bus, and we cannot probe the bus before enabling the clks, etc.

The approach I took with brcmfmac is that upon module init I search the 
DT for "brcm,bcm43xx-fmac" compatible and get the clock and/or gpio 
resource and enable them before registering the sdio driver. The 
difficulty is probably when using the driver built-in as the clocks and 
gpios may not be available yet and we can not rely on deferred probing 
in module init stage.

Regards,
Arend

>>> FWIW if we ever get truely complex cases I think modeling the
>>> power-up hardware as a pmic platform device is not a bad idea,
>>> we would then need to have a generic mmc-host pmic property, which
>>> would be used both to do the initial powerup before scanning, as
>>> well as for the sdio device driver to get a handle to the pmic,
>>> for run time power-management (if desired).
>>
>> I don't know if this will ever apply to SDIO but with other buses the
>> complicated bits come when the driver wants to take over some of the
>> power management do things like turn some of the supplies or clocks on
>> and off independently at runtime for low power modes.
>
> Hmm, good point in that case actually having these things in the
> child node makes most sense, because then the driver can find them
> their. Note that the mmc core enabling things does not mean that
> the driver cannot later disable them if needed.

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/23/2014 03:21 PM, Arend van Spriel wrote:
> On 05/23/14 13:50, Hans de Goede wrote:
>> Hi,
>>
>> On 05/23/2014 01:22 PM, Mark Brown wrote:
>>> On Fri, May 23, 2014 at 11:13:44AM +0200, Hans de Goede wrote:
>>>
>>>> Thinking more about this, I would like to make one change to my
>>>> proposal, the mmc-core should only do power up of child-nodes if
>>>> they have a compatible of: "simple-sdio-powerup". This way
>>>> when we add something more complex, we can keep the simple powerup
>>>> code in the mmc core, keeping what we've already using this working
>>>> and the mmc core won't respond to the child nodes for more complex
>>>> devices, so it won't conflict with more complex power-up handling
>>>> handled by some other driver.
>>>
>>> Would it not be better to have this be something in the driver struct
>>> rather than in the device tree?  Putting a compatible in there would be
>>> encoding details of the Linux implementation in the DT which doesn't
>>> seem right especially since these are details we're thinking of changing
>>> later on.
>>
>> The compatible is not a Linux specific thing, it is a marking saying
>> that something needs to take care of enabling the clks (and whatever
>> else we will make part of the binding for this compatible), before
>> scanning the mmc bus.
>>
>>> Something like have the driver set flags saying if it wants
>>> to do complicated things.
>>
>> Chicken<->  egg, we won't know which driver to use before we've probed
>> the mmc bus, and we cannot probe the bus before enabling the clks, etc.
> 
> The approach I took with brcmfmac is that upon module init I search the DT for "brcm,bcm43xx-fmac" compatible and get the clock and/or gpio resource and enable them before registering the sdio driver. The difficulty is probably when using the driver built-in as the clocks and gpios may not be available yet and we can not rely on deferred probing in module init stage.

I know, and that approach does not work, by the time the brcmfmac loads,
the mmc core has long probed the mmc bus and decided no one is home.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Ulf Hansson]

On 22 May 2014 19:20, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi,
>
>
> On 22.05.2014 13:38, Hans de Goede wrote:
>> On 05/22/2014 12:23 PM, Chen-Yu Tsai wrote:
>>> On Thu, May 22, 2014 at 5:49 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>
> snip
>
>>>> I've been thinking a bit about this, and it is a non trivial problem since
>>>> sdio devices are normally instantiated when probed, unlike ie spi devices
>>>> which are instantiated from devicetree.
>>>>
>>>> Adding device tree instantiation of sdio devices seems like a bad idea, as
>>>> then we get 2 vastly different device instantiation paths. Still we need some
>>>> way to get power and clks setup before the mmc host initializes.
>
> What about introducing some extra callbacks to mmc drivers to build
> driver data and control power?
>
> struct mmc_legacy_device {
>         /* ... */
>         struct device_node *of_node;
>         void *driver_data;
> };
>
> struct mmc_driver {
>         /* ... */
>         int (*prepare)(struct mmc_legacy_device *);
>         void (*unprepare)(struct mmc_legacy_device *);
> };
>
> static int my_wlan_prepare(struct mmc_legacy_device *dev)
> {
>         struct my_wlan_data *data;
>
>         data = kzalloc(...);
>         /* ... */
>
>         of_*();
>         clk_*();
>         regulator_*();
>         /* ... */
>
>         ret = my_wlan_power_on(data);
>         /* ... */
>
>         dev->driver_data = data;
> }
>
> static void my_wlan_unprepare(struct mmc_legacy_device *dev)
> {
>         struct my_wlan_data *data;
>
>         data = dev->driver_data;
>
>         my_wlan_power_off(data);
>         /* ... */
>
>         dev->driver_data = NULL;
>         kfree(data);
> }
>
> I don't really like this, especially the fact that this would be highly
> MMC-specific, while there are other interfaces that also need this
> problem solved, e.g. USB/HSIC.
>
> I had proposed another solution that would require almost no changes in
> MMC core and could work for any data interface. You can find it
> somewhere in the thread mentioned by Chen-Yu. Unfortunately, for reasons
> that don't appeal to me, it wasn't positively received by MMC people.

Hi Tomasz,

To be clear, I surely liked your proposal, sorry if you interpreted my
comments differently. I was just eager to see some patches before
continuing the discussion. :-)

If remember correctly, our discussion went on hold, while we tried to
figure out how to handle the probing of the "power controller driver".
I gave some suggestion, which had some pros and cons. Let's follow up
on this.

I start by giving a short re-cap of my view for how to solve this
problem, which is based upon your idea with the "power controller
driver".

1.
The power controller driver handles the needed _external_ PM resources
and maintains SOC specific power on sequence, to be able to initialize
the "non-removable" SDIO card.

To not mix things up, the mmc host will also manages some PM resources
to be able to serve communication with the SDIO card. These resources
are typically the SDIO VCC power, the SDIO bus interface clock,
pinctrl for the interface signals, etc.

2.
To handle reference counting, we rely on runtime PM, using
pm_runtime_get|put() of the power controller's struct device.

3.
By using runtime PM as in 2), we enable the power controller driver to
keep a centralized control of the PM resources. This will also be
useful when the resources are shared. For example, like we have on
ux500 with bluetooth and wlan.


If this above seems like a reasonable approach, we still have a few
things to iron out.

1.
The power controller driver needs to be probed prior the actual SDIO
card detection starts, which is managed through a scheduled work
triggered when the mmc host gets added to the mmc core.

2.
The mmc core or host, need a handle of the power controller's struct
device to  be able to invoke pm_runtime_get|put() on it.

Suggestions/comments? They are welcome! :-)

Kind regards
Ulf Hansson

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Arend van Spriel]

On 05/23/14 15:28, Hans de Goede wrote:
> Hi,
>
> On 05/23/2014 03:21 PM, Arend van Spriel wrote:
>> On 05/23/14 13:50, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 05/23/2014 01:22 PM, Mark Brown wrote:
>>>> On Fri, May 23, 2014 at 11:13:44AM +0200, Hans de Goede wrote:
>>>>
>>>>> Thinking more about this, I would like to make one change to my
>>>>> proposal, the mmc-core should only do power up of child-nodes if
>>>>> they have a compatible of: "simple-sdio-powerup". This way
>>>>> when we add something more complex, we can keep the simple powerup
>>>>> code in the mmc core, keeping what we've already using this working
>>>>> and the mmc core won't respond to the child nodes for more complex
>>>>> devices, so it won't conflict with more complex power-up handling
>>>>> handled by some other driver.
>>>>
>>>> Would it not be better to have this be something in the driver struct
>>>> rather than in the device tree?  Putting a compatible in there would be
>>>> encoding details of the Linux implementation in the DT which doesn't
>>>> seem right especially since these are details we're thinking of changing
>>>> later on.
>>>
>>> The compatible is not a Linux specific thing, it is a marking saying
>>> that something needs to take care of enabling the clks (and whatever
>>> else we will make part of the binding for this compatible), before
>>> scanning the mmc bus.
>>>
>>>> Something like have the driver set flags saying if it wants
>>>> to do complicated things.
>>>
>>> Chicken<->   egg, we won't know which driver to use before we've probed
>>> the mmc bus, and we cannot probe the bus before enabling the clks, etc.
>>
>> The approach I took with brcmfmac is that upon module init I search the DT for "brcm,bcm43xx-fmac" compatible and get the clock and/or gpio resource and enable them before registering the sdio driver. The difficulty is probably when using the driver built-in as the clocks and gpios may not be available yet and we can not rely on deferred probing in module init stage.
>
> I know, and that approach does not work, by the time the brcmfmac loads,
> the mmc core has long probed the mmc bus and decided no one is home.

Ok. That is due to the non-removable property, right? I assumed a mmc 
rescan, which is (supposedly) triggered upon sdio driver registration, 
would subsequently find the device.

Regards,
Arend

> Regards,
>
> Hans
>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Mark Brown]

On Fri, May 23, 2014 at 01:50:40PM +0200, Hans de Goede wrote:
> On 05/23/2014 01:22 PM, Mark Brown wrote:

> > Would it not be better to have this be something in the driver struct
> > rather than in the device tree?  Putting a compatible in there would be
> > encoding details of the Linux implementation in the DT which doesn't
> > seem right especially since these are details we're thinking of changing
> > later on.

> The compatible is not a Linux specific thing, it is a marking saying
> that something needs to take care of enabling the clks (and whatever
> else we will make part of the binding for this compatible), before
> scanning the mmc bus.

We could just say that the mere presence of a child node with the right
properties is sufficient to trigger the bus to do the startup?

> > Something like have the driver set flags saying if it wants
> > to do complicated things.

> Chicken <-> egg, we won't know which driver to use before we've probed
> the mmc bus, and we cannot probe the bus before enabling the clks, etc.

If the device is sufficiently complicated to need a special power up
sequence I'd expect we'd be able to have a compatible string which would
provide enough information for us to figure things out.

> >> FWIW if we ever get truely complex cases I think modeling the
> >> power-up hardware as a pmic platform device is not a bad idea,
> >> we would then need to have a generic mmc-host pmic property, which
> >> would be used both to do the initial powerup before scanning, as
> >> well as for the sdio device driver to get a handle to the pmic,
> >> for run time power-management (if desired).

> > I don't know if this will ever apply to SDIO but with other buses the
> > complicated bits come when the driver wants to take over some of the
> > power management do things like turn some of the supplies or clocks on
> > and off independently at runtime for low power modes.

> Hmm, good point in that case actually having these things in the
> child node makes most sense, because then the driver can find them
> their. Note that the mmc core enabling things does not mean that
> the driver cannot later disable them if needed.

Right, that's good idea for solving the problem - the child device can
either share the reference with the bus or have some way of getting at
the object the bus requested depending on what's sensible.  Only
potentia complication I can think of with that approach is a device with
multiple bus interfaces (I'm mainly thinking of SDIO vs SPI) but it
doesn't seem to hard to deal with that in the bus adaption layers of the
drivers.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140523/1c798047/attachment.sig>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Olof Johansson]

On Thu, May 22, 2014 at 07:20:55PM +0200, Tomasz Figa wrote:
> Hi,
> 
> 
> On 22.05.2014 13:38, Hans de Goede wrote:
> > On 05/22/2014 12:23 PM, Chen-Yu Tsai wrote:
> >> On Thu, May 22, 2014 at 5:49 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
> snip
> 
> >>> I've been thinking a bit about this, and it is a non trivial problem since
> >>> sdio devices are normally instantiated when probed, unlike ie spi devices
> >>> which are instantiated from devicetree.
> >>>
> >>> Adding device tree instantiation of sdio devices seems like a bad idea, as
> >>> then we get 2 vastly different device instantiation paths. Still we need some
> >>> way to get power and clks setup before the mmc host initializes.
> 
> What about introducing some extra callbacks to mmc drivers to build
> driver data and control power?

The MMC bus is probable, and there should be no need to put any information in
the device tree to pair up the right driver with the right device. The only
thing we should need is hardware description w.r.t. reset/power/clock lines.

It's pretty common during development to have a couple of different vendor
modules. Reset sequences and requirements might not be identical between them,
but in reality they all work well enough using the common settings.


Besides, where it ends up in the kernel implementation is mostly irrelevant, in
some ways. We can refactor and move things as needed at any time. The only
thing that needs to be reasonably stable (and/or only be expanded on, not
redefined) is the DT binding. So I'd rather see something KISS go in now,
implementation-wise (with a sane and simple binding), then getting stuck in
this infinite polishing of just how the kernel-side implementation should be.

This is one of the major missing pieces to make a lot of ARM systems usable
with a mainline kernel, since everybody use some out-of-tree solution today
(not necessarily hacks, but still not shared code). I'd really like to see it
resolved soon.

I'm OK in principle with Hans' proposed solution, as long as it provides the
properties I need on exynos5250-snow (and the new peach_pit/pi platforms).


-Olof

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/23/2014 06:27 PM, Mark Brown wrote:
> On Fri, May 23, 2014 at 01:50:40PM +0200, Hans de Goede wrote:
>> On 05/23/2014 01:22 PM, Mark Brown wrote:
> 
>>> Would it not be better to have this be something in the driver struct
>>> rather than in the device tree?  Putting a compatible in there would be
>>> encoding details of the Linux implementation in the DT which doesn't
>>> seem right especially since these are details we're thinking of changing
>>> later on.
> 
>> The compatible is not a Linux specific thing, it is a marking saying
>> that something needs to take care of enabling the clks (and whatever
>> else we will make part of the binding for this compatible), before
>> scanning the mmc bus.
> 
> We could just say that the mere presence of a child node with the right
> properties is sufficient to trigger the bus to do the startup?

Yes, except that most involved property names are standardized, ie clocks,
and we want to be able to opt out of the KISS mmc core code for
(future) complex power on sequences.

> 
>>> Something like have the driver set flags saying if it wants
>>> to do complicated things.
> 
>> Chicken <-> egg, we won't know which driver to use before we've probed
>> the mmc bus, and we cannot probe the bus before enabling the clks, etc.
> 
> If the device is sufficiently complicated to need a special power up
> sequence I'd expect we'd be able to have a compatible string which would
> provide enough information for us to figure things out.

Hmm, so what you're suggesting is indeed more of an opt out then my initial
opt-in to KISS powerup idea. So to be clear what you're suggesting is:

mmc core walks host mmc-child nodes. Loads drivers based on compatibles
there. The checks a flag field in the driver to see if the driver wants to
opt-out of the KISS powerup code. The problem with this is that it won't
work reliable with modules, think the mmc probe being done from the ramdisk,
and the driver in question only being available from the rootfs. I really
believe that using opt-in with a compatible such as simple-sdio-powerup
is by far the safest thing to do, and as an added advantage we don't need
to worry about how to deal with the future complex power on cases at all,
we leave all the room in the world for various future scenarios. since as
soon as the simple-sdio-powerup compatible is not there the mmc core will
behave as it does today.


> 
>>>> FWIW if we ever get truely complex cases I think modeling the
>>>> power-up hardware as a pmic platform device is not a bad idea,
>>>> we would then need to have a generic mmc-host pmic property, which
>>>> would be used both to do the initial powerup before scanning, as
>>>> well as for the sdio device driver to get a handle to the pmic,
>>>> for run time power-management (if desired).
> 
>>> I don't know if this will ever apply to SDIO but with other buses the
>>> complicated bits come when the driver wants to take over some of the
>>> power management do things like turn some of the supplies or clocks on
>>> and off independently at runtime for low power modes.
> 
>> Hmm, good point in that case actually having these things in the
>> child node makes most sense, because then the driver can find them
>> their. Note that the mmc core enabling things does not mean that
>> the driver cannot later disable them if needed.
> 
> Right, that's good idea for solving the problem - the child device can
> either share the reference with the bus or have some way of getting at
> the object the bus requested depending on what's sensible.  Only
> potentia complication I can think of with that approach is a device with
> multiple bus interfaces (I'm mainly thinking of SDIO vs SPI) but it
> doesn't seem to hard to deal with that in the bus adaption layers of the
> drivers.

I don't think there is a need to share references, ie for clks multiple drivers
can hold a reference and they can be enabled / disabled multiple times, only
when the last enable is countered by a disable the clock will really be disabled,
iow this should all just work. Anyways these are all implementation details
lets focus on the bindings bit first.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/23/2014 06:47 PM, Olof Johansson wrote:
> On Thu, May 22, 2014 at 07:20:55PM +0200, Tomasz Figa wrote:
>> Hi,
>>
>>
>> On 22.05.2014 13:38, Hans de Goede wrote:
>>> On 05/22/2014 12:23 PM, Chen-Yu Tsai wrote:
>>>> On Thu, May 22, 2014 at 5:49 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> snip
>>
>>>>> I've been thinking a bit about this, and it is a non trivial problem since
>>>>> sdio devices are normally instantiated when probed, unlike ie spi devices
>>>>> which are instantiated from devicetree.
>>>>>
>>>>> Adding device tree instantiation of sdio devices seems like a bad idea, as
>>>>> then we get 2 vastly different device instantiation paths. Still we need some
>>>>> way to get power and clks setup before the mmc host initializes.
>>
>> What about introducing some extra callbacks to mmc drivers to build
>> driver data and control power?
> 
> The MMC bus is probable, and there should be no need to put any information in
> the device tree to pair up the right driver with the right device. The only
> thing we should need is hardware description w.r.t. reset/power/clock lines.
> 
> It's pretty common during development to have a couple of different vendor
> modules. Reset sequences and requirements might not be identical between them,
> but in reality they all work well enough using the common settings.
> 
> 
> Besides, where it ends up in the kernel implementation is mostly irrelevant, in
> some ways. We can refactor and move things as needed at any time. The only
> thing that needs to be reasonably stable (and/or only be expanded on, not
> redefined) is the DT binding. So I'd rather see something KISS go in now,
> implementation-wise (with a sane and simple binding), then getting stuck in
> this infinite polishing of just how the kernel-side implementation should be.
> 
> This is one of the major missing pieces to make a lot of ARM systems usable
> with a mainline kernel, since everybody use some out-of-tree solution today
> (not necessarily hacks, but still not shared code). I'd really like to see it
> resolved soon.
> 
> I'm OK in principle with Hans' proposed solution, as long as it provides the
> properties I need on exynos5250-snow (and the new peach_pit/pi platforms).

It would help to know what properties exactly you need, then I can write a proposal
for the devicetree bindings documentation. I can also throw in an implementation
for bringing up clks before mmc probe, as that is something which I can test,
extending that with other properties (ie resets) should be easy (ish).

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/23/2014 04:54 PM, Arend van Spriel wrote:
> On 05/23/14 15:28, Hans de Goede wrote:
>> Hi,
>>
>> On 05/23/2014 03:21 PM, Arend van Spriel wrote:
>>> On 05/23/14 13:50, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 05/23/2014 01:22 PM, Mark Brown wrote:
>>>>> On Fri, May 23, 2014 at 11:13:44AM +0200, Hans de Goede wrote:
>>>>>
>>>>>> Thinking more about this, I would like to make one change to my
>>>>>> proposal, the mmc-core should only do power up of child-nodes if
>>>>>> they have a compatible of: "simple-sdio-powerup". This way
>>>>>> when we add something more complex, we can keep the simple powerup
>>>>>> code in the mmc core, keeping what we've already using this working
>>>>>> and the mmc core won't respond to the child nodes for more complex
>>>>>> devices, so it won't conflict with more complex power-up handling
>>>>>> handled by some other driver.
>>>>>
>>>>> Would it not be better to have this be something in the driver struct
>>>>> rather than in the device tree?  Putting a compatible in there would be
>>>>> encoding details of the Linux implementation in the DT which doesn't
>>>>> seem right especially since these are details we're thinking of changing
>>>>> later on.
>>>>
>>>> The compatible is not a Linux specific thing, it is a marking saying
>>>> that something needs to take care of enabling the clks (and whatever
>>>> else we will make part of the binding for this compatible), before
>>>> scanning the mmc bus.
>>>>
>>>>> Something like have the driver set flags saying if it wants
>>>>> to do complicated things.
>>>>
>>>> Chicken<->   egg, we won't know which driver to use before we've probed
>>>> the mmc bus, and we cannot probe the bus before enabling the clks, etc.
>>>
>>> The approach I took with brcmfmac is that upon module init I search the DT for "brcm,bcm43xx-fmac" compatible and get the clock and/or gpio resource and enable them before registering the sdio driver. The difficulty is probably when using the driver built-in as the clocks and gpios may not be available yet and we can not rely on deferred probing in module init stage.
>>
>> I know, and that approach does not work, by the time the brcmfmac loads,
>> the mmc core has long probed the mmc bus and decided no one is home.
> 
> Ok. That is due to the non-removable property, right? I assumed a mmc rescan, which is (supposedly) triggered upon sdio driver registration, would subsequently find the device.

That is what I thought, but I tried without the non-removable property and
the sdio wifi was still not recognized.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Mark Brown]

On Sat, May 24, 2014 at 12:06:30PM +0200, Hans de Goede wrote:
> On 05/23/2014 06:27 PM, Mark Brown wrote:
> > On Fri, May 23, 2014 at 01:50:40PM +0200, Hans de Goede wrote:
> >> On 05/23/2014 01:22 PM, Mark Brown wrote:

> >> The compatible is not a Linux specific thing, it is a marking saying
> >> that something needs to take care of enabling the clks (and whatever
> >> else we will make part of the binding for this compatible), before
> >> scanning the mmc bus.

> > We could just say that the mere presence of a child node with the right
> > properties is sufficient to trigger the bus to do the startup?

> Yes, except that most involved property names are standardized, ie clocks,
> and we want to be able to opt out of the KISS mmc core code for
> (future) complex power on sequences.

This is why I'm suggesting keying off the specific compatible strings
for drivers that want to opt out of the helpers rather than having a
compatible string to enable the helpers (which may depend on the
particular Linux version if the feature sets of drivers change for
example).

> > If the device is sufficiently complicated to need a special power up
> > sequence I'd expect we'd be able to have a compatible string which would
> > provide enough information for us to figure things out.

> Hmm, so what you're suggesting is indeed more of an opt out then my initial
> opt-in to KISS powerup idea. So to be clear what you're suggesting is:

> mmc core walks host mmc-child nodes. Loads drivers based on compatibles
> there. The checks a flag field in the driver to see if the driver wants to
> opt-out of the KISS powerup code. The problem with this is that it won't

Yes.

> work reliable with modules, think the mmc probe being done from the ramdisk,
> and the driver in question only being available from the rootfs. I really

Why is that a problem - if we have no driver for the device there is no
point in powering it up in the first place is there?

> believe that using opt-in with a compatible such as simple-sdio-powerup
> is by far the safest thing to do, and as an added advantage we don't need
> to worry about how to deal with the future complex power on cases at all,
> we leave all the room in the world for various future scenarios. since as
> soon as the simple-sdio-powerup compatible is not there the mmc core will
> behave as it does today.

Please remember that the DT is *supposed* to be an ABI - systems are
supposed to be able to ship with a fixed DT and have it work with random
operating systems they have no knowledge of (and may not even exist at
the time the DT is being created).  This means we can't rely on removing
a compatible string later on.

> >> Hmm, good point in that case actually having these things in the
> >> child node makes most sense, because then the driver can find them
> >> their. Note that the mmc core enabling things does not mean that
> >> the driver cannot later disable them if needed.

> > Right, that's good idea for solving the problem - the child device can
> > either share the reference with the bus or have some way of getting at
> > the object the bus requested depending on what's sensible.  Only

> I don't think there is a need to share references, ie for clks multiple drivers
> can hold a reference and they can be enabled / disabled multiple times, only
> when the last enable is countered by a disable the clock will really be disabled,
> iow this should all just work. Anyways these are all implementation details
> lets focus on the bindings bit first.

The reference I'm referring to here is the enable - a driver should not
normally disable something it didn't enable itself.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140525/7baf8db7/attachment.sig>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/25/2014 02:34 PM, Mark Brown wrote:
> On Sat, May 24, 2014 at 12:06:30PM +0200, Hans de Goede wrote:
>> On 05/23/2014 06:27 PM, Mark Brown wrote:
>>> On Fri, May 23, 2014 at 01:50:40PM +0200, Hans de Goede wrote:
>>>> On 05/23/2014 01:22 PM, Mark Brown wrote:
> 
>>>> The compatible is not a Linux specific thing, it is a marking saying
>>>> that something needs to take care of enabling the clks (and whatever
>>>> else we will make part of the binding for this compatible), before
>>>> scanning the mmc bus.
> 
>>> We could just say that the mere presence of a child node with the right
>>> properties is sufficient to trigger the bus to do the startup?
> 
>> Yes, except that most involved property names are standardized, ie clocks,
>> and we want to be able to opt out of the KISS mmc core code for
>> (future) complex power on sequences.
> 
> This is why I'm suggesting keying off the specific compatible strings
> for drivers that want to opt out of the helpers rather than having a
> compatible string to enable the helpers (which may depend on the
> particular Linux version if the feature sets of drivers change for
> example).
> 
>>> If the device is sufficiently complicated to need a special power up
>>> sequence I'd expect we'd be able to have a compatible string which would
>>> provide enough information for us to figure things out.
> 
>> Hmm, so what you're suggesting is indeed more of an opt out then my initial
>> opt-in to KISS powerup idea. So to be clear what you're suggesting is:
> 
>> mmc core walks host mmc-child nodes. Loads drivers based on compatibles
>> there. The checks a flag field in the driver to see if the driver wants to
>> opt-out of the KISS powerup code. The problem with this is that it won't
> 
> Yes.
> 
>> work reliable with modules, think the mmc probe being done from the ramdisk,
>> and the driver in question only being available from the rootfs. I really
> 
> Why is that a problem - if we have no driver for the device there is no
> point in powering it up in the first place is there?

Well the driver may show up later, so if we only do the power-up once we have
a driver, this means we need to re-check if we need to do the powerup later.

Also the mmc people are very much against specifying a driver, as that is
something which should be probed not specified. I agree with them I've
already seen boards were more or less standardized sdio modules from different
vendors are used, they have various standard sdio powerup related things, like
an enable signal in standard places, but different editions of the boards
have different sdio modules soldered on, using different drivers.

I expect that with some care we can make all variants of these boards work with
a single dts file, by using a simple sdio wifi powerup mechanism, and after
that letting the mmc core figure out which module is actually soldered on.

>> believe that using opt-in with a compatible such as simple-sdio-powerup
>> is by far the safest thing to do, and as an added advantage we don't need
>> to worry about how to deal with the future complex power on cases at all,
>> we leave all the room in the world for various future scenarios. since as
>> soon as the simple-sdio-powerup compatible is not there the mmc core will
>> behave as it does today.
> 
> Please remember that the DT is *supposed* to be an ABI - systems are
> supposed to be able to ship with a fixed DT and have it work with random
> operating systems they have no knowledge of (and may not even exist at
> the time the DT is being created).  This means we can't rely on removing
> a compatible string later on.

I know that the DT is an ABI, and I'm not arguing for removing support for
the simple-sdio-powerup compatible from the kernel when a more complex
case arrives, nor am I arguing to remove it from the DT for existing working
boards. The idea behind the simple-sdio-powerup compatible is that it makes
the simple powerup behavior opt-in. So if a new board comes along which
requires something more complex, the people working on this can do what ever
they want / need without the simple powerup code getting in the way, as
long as they don't *add* the simple-sdio-powerup compatible to their *new*
DT file.

Basically the idea behind the simple-sdio-powerup compatible is to make
the worst case scenario for more complex boards to be the scenario which
we have today, i.e. no support for sdio powerup at all, rather then having
something in place which actually may get in the way, making things worse
then they are today.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Arend van Spriel]

+ Russell

On 05/25/14 21:20, Hans de Goede wrote:
> Hi,
>
> On 05/25/2014 02:34 PM, Mark Brown wrote:
>> On Sat, May 24, 2014 at 12:06:30PM +0200, Hans de Goede wrote:
>>> On 05/23/2014 06:27 PM, Mark Brown wrote:
>>>> On Fri, May 23, 2014 at 01:50:40PM +0200, Hans de Goede wrote:
>>>>> On 05/23/2014 01:22 PM, Mark Brown wrote:
>>
>>>>> The compatible is not a Linux specific thing, it is a marking saying
>>>>> that something needs to take care of enabling the clks (and whatever
>>>>> else we will make part of the binding for this compatible), before
>>>>> scanning the mmc bus.
>>
>>>> We could just say that the mere presence of a child node with the right
>>>> properties is sufficient to trigger the bus to do the startup?
>>
>>> Yes, except that most involved property names are standardized, ie clocks,
>>> and we want to be able to opt out of the KISS mmc core code for
>>> (future) complex power on sequences.
>>
>> This is why I'm suggesting keying off the specific compatible strings
>> for drivers that want to opt out of the helpers rather than having a
>> compatible string to enable the helpers (which may depend on the
>> particular Linux version if the feature sets of drivers change for
>> example).
>>
>>>> If the device is sufficiently complicated to need a special power up
>>>> sequence I'd expect we'd be able to have a compatible string which would
>>>> provide enough information for us to figure things out.
>>
>>> Hmm, so what you're suggesting is indeed more of an opt out then my initial
>>> opt-in to KISS powerup idea. So to be clear what you're suggesting is:
>>
>>> mmc core walks host mmc-child nodes. Loads drivers based on compatibles
>>> there. The checks a flag field in the driver to see if the driver wants to
>>> opt-out of the KISS powerup code. The problem with this is that it won't
>>
>> Yes.
>>
>>> work reliable with modules, think the mmc probe being done from the ramdisk,
>>> and the driver in question only being available from the rootfs. I really
>>
>> Why is that a problem - if we have no driver for the device there is no
>> point in powering it up in the first place is there?
>
> Well the driver may show up later, so if we only do the power-up once we have
> a driver, this means we need to re-check if we need to do the powerup later.
>
> Also the mmc people are very much against specifying a driver, as that is
> something which should be probed not specified. I agree with them I've
> already seen boards were more or less standardized sdio modules from different
> vendors are used, they have various standard sdio powerup related things, like
> an enable signal in standard places, but different editions of the boards
> have different sdio modules soldered on, using different drivers.
>
> I expect that with some care we can make all variants of these boards work with
> a single dts file, by using a simple sdio wifi powerup mechanism, and after
> that letting the mmc core figure out which module is actually soldered on.
>
>>> believe that using opt-in with a compatible such as simple-sdio-powerup
>>> is by far the safest thing to do, and as an added advantage we don't need
>>> to worry about how to deal with the future complex power on cases at all,
>>> we leave all the room in the world for various future scenarios. since as
>>> soon as the simple-sdio-powerup compatible is not there the mmc core will
>>> behave as it does today.
>>
>> Please remember that the DT is *supposed* to be an ABI - systems are
>> supposed to be able to ship with a fixed DT and have it work with random
>> operating systems they have no knowledge of (and may not even exist at
>> the time the DT is being created).  This means we can't rely on removing
>> a compatible string later on.
>
> I know that the DT is an ABI, and I'm not arguing for removing support for
> the simple-sdio-powerup compatible from the kernel when a more complex
> case arrives, nor am I arguing to remove it from the DT for existing working
> boards. The idea behind the simple-sdio-powerup compatible is that it makes
> the simple powerup behavior opt-in. So if a new board comes along which
> requires something more complex, the people working on this can do what ever
> they want / need without the simple powerup code getting in the way, as
> long as they don't *add* the simple-sdio-powerup compatible to their *new*
> DT file.
>
> Basically the idea behind the simple-sdio-powerup compatible is to make
> the worst case scenario for more complex boards to be the scenario which
> we have today, i.e. no support for sdio powerup at all, rather then having
> something in place which actually may get in the way, making things worse
> then they are today.

Hi Hans,

I recalled a recent patchset from Russell King. He was working on i.MX6 
platform with brcmfmac device and ended reworking sdhci/mmc host 
controller code in a series of patches [1]. Patch 34 might be similar to 
what you are trying to accomplish.

Regards,
Arend

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Chen-Yu Tsai]

On Mon, May 26, 2014 at 3:51 PM, Arend van Spriel <arend@broadcom.com> wrote:
> + Russell
>
>
> On 05/25/14 21:20, Hans de Goede wrote:
>>
>> Hi,
>>
>> On 05/25/2014 02:34 PM, Mark Brown wrote:
>>>
>>> On Sat, May 24, 2014 at 12:06:30PM +0200, Hans de Goede wrote:
>>>>
>>>> On 05/23/2014 06:27 PM, Mark Brown wrote:
>>>>>
>>>>> On Fri, May 23, 2014 at 01:50:40PM +0200, Hans de Goede wrote:
>>>>>>
>>>>>> On 05/23/2014 01:22 PM, Mark Brown wrote:
>>>
>>>
>>>>>> The compatible is not a Linux specific thing, it is a marking saying
>>>>>> that something needs to take care of enabling the clks (and whatever
>>>>>> else we will make part of the binding for this compatible), before
>>>>>> scanning the mmc bus.
>>>
>>>
>>>>> We could just say that the mere presence of a child node with the right
>>>>> properties is sufficient to trigger the bus to do the startup?
>>>
>>>
>>>> Yes, except that most involved property names are standardized, ie
>>>> clocks,
>>>> and we want to be able to opt out of the KISS mmc core code for
>>>> (future) complex power on sequences.
>>>
>>>
>>> This is why I'm suggesting keying off the specific compatible strings
>>> for drivers that want to opt out of the helpers rather than having a
>>> compatible string to enable the helpers (which may depend on the
>>> particular Linux version if the feature sets of drivers change for
>>> example).
>>>
>>>>> If the device is sufficiently complicated to need a special power up
>>>>> sequence I'd expect we'd be able to have a compatible string which
>>>>> would
>>>>> provide enough information for us to figure things out.
>>>
>>>
>>>> Hmm, so what you're suggesting is indeed more of an opt out then my
>>>> initial
>>>> opt-in to KISS powerup idea. So to be clear what you're suggesting is:
>>>
>>>
>>>> mmc core walks host mmc-child nodes. Loads drivers based on compatibles
>>>> there. The checks a flag field in the driver to see if the driver wants
>>>> to
>>>> opt-out of the KISS powerup code. The problem with this is that it won't
>>>
>>>
>>> Yes.
>>>
>>>> work reliable with modules, think the mmc probe being done from the
>>>> ramdisk,
>>>> and the driver in question only being available from the rootfs. I
>>>> really
>>>
>>>
>>> Why is that a problem - if we have no driver for the device there is no
>>> point in powering it up in the first place is there?
>>
>>
>> Well the driver may show up later, so if we only do the power-up once we
>> have
>> a driver, this means we need to re-check if we need to do the powerup
>> later.
>>
>> Also the mmc people are very much against specifying a driver, as that is
>> something which should be probed not specified. I agree with them I've
>> already seen boards were more or less standardized sdio modules from
>> different
>> vendors are used, they have various standard sdio powerup related things,
>> like
>> an enable signal in standard places, but different editions of the boards
>> have different sdio modules soldered on, using different drivers.
>>
>> I expect that with some care we can make all variants of these boards work
>> with
>> a single dts file, by using a simple sdio wifi powerup mechanism, and
>> after
>> that letting the mmc core figure out which module is actually soldered on.
>>
>>>> believe that using opt-in with a compatible such as simple-sdio-powerup
>>>> is by far the safest thing to do, and as an added advantage we don't
>>>> need
>>>> to worry about how to deal with the future complex power on cases at
>>>> all,
>>>> we leave all the room in the world for various future scenarios. since
>>>> as
>>>> soon as the simple-sdio-powerup compatible is not there the mmc core
>>>> will
>>>> behave as it does today.
>>>
>>>
>>> Please remember that the DT is *supposed* to be an ABI - systems are
>>> supposed to be able to ship with a fixed DT and have it work with random
>>> operating systems they have no knowledge of (and may not even exist at
>>> the time the DT is being created).  This means we can't rely on removing
>>> a compatible string later on.
>>
>>
>> I know that the DT is an ABI, and I'm not arguing for removing support for
>> the simple-sdio-powerup compatible from the kernel when a more complex
>> case arrives, nor am I arguing to remove it from the DT for existing
>> working
>> boards. The idea behind the simple-sdio-powerup compatible is that it
>> makes
>> the simple powerup behavior opt-in. So if a new board comes along which
>> requires something more complex, the people working on this can do what
>> ever
>> they want / need without the simple powerup code getting in the way, as
>> long as they don't *add* the simple-sdio-powerup compatible to their *new*
>> DT file.
>>
>> Basically the idea behind the simple-sdio-powerup compatible is to make
>> the worst case scenario for more complex boards to be the scenario which
>> we have today, i.e. no support for sdio powerup at all, rather then having
>> something in place which actually may get in the way, making things worse
>> then they are today.
>
>
> Hi Hans,
>
> I recalled a recent patchset from Russell King. He was working on i.MX6
> platform with brcmfmac device and ended reworking sdhci/mmc host controller
> code in a series of patches [1]. Patch 34 might be similar to what you are
> trying to accomplish.

I believe that is a resend of Olof's patch I mentioned early in this
discussion. :)

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/26/2014 09:59 AM, Chen-Yu Tsai wrote:
> On Mon, May 26, 2014 at 3:51 PM, Arend van Spriel <arend@broadcom.com> wrote:
>> + Russell

<snip>

>> Hi Hans,
>>
>> I recalled a recent patchset from Russell King. He was working on i.MX6
>> platform with brcmfmac device and ended reworking sdhci/mmc host controller
>> code in a series of patches [1]. Patch 34 might be similar to what you are
>> trying to accomplish.
> 
> I believe that is a resend of Olof's patch I mentioned early in this
> discussion. :)

Ok, assuming that is the case, then it seems to me that we are all moving
somewhat in the same direction, which is good :)

What I would like to propose is to move forward with Olof's patch with
2 changes made to it:

1) Store the clocks / resets / whatever in childnodes of the mmc host node,
with the childnodes using the addressing scheme described in the patch
from Sascha Hauer titled: "mmc: Add SDIO function devicetree subnode parsing",
as this is where they really belong (and in some cases the sdio function driver
may need access to them too).

2) Make Olof's code only do the powerup if the child node has a compatible of
"simple-sdio-powerup", to avoid it getting in the way of more complex poweron
scenarios (which may require a separate pmic driver or some such) later.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Arend van Spriel]

On 05/26/14 10:07, Hans de Goede wrote:
> Hi,
>
> On 05/26/2014 09:59 AM, Chen-Yu Tsai wrote:
>> On Mon, May 26, 2014 at 3:51 PM, Arend van Spriel<arend@broadcom.com>  wrote:
>>> + Russell
>
> <snip>
>
>>> Hi Hans,
>>>
>>> I recalled a recent patchset from Russell King. He was working on i.MX6
>>> platform with brcmfmac device and ended reworking sdhci/mmc host controller
>>> code in a series of patches [1]. Patch 34 might be similar to what you are
>>> trying to accomplish.
>>
>> I believe that is a resend of Olof's patch I mentioned early in this
>> discussion. :)

Ok,

I meant to refer to this thread [1]. Indeed, the patch is from Olof.

Regards,
Arend

[1] http://thread.gmane.org/gmane.linux.documentation/22805

> Ok, assuming that is the case, then it seems to me that we are all moving
> somewhat in the same direction, which is good :)
>
> What I would like to propose is to move forward with Olof's patch with
> 2 changes made to it:
>
> 1) Store the clocks / resets / whatever in childnodes of the mmc host node,
> with the childnodes using the addressing scheme described in the patch
> from Sascha Hauer titled: "mmc: Add SDIO function devicetree subnode parsing",
> as this is where they really belong (and in some cases the sdio function driver
> may need access to them too).
>
> 2) Make Olof's code only do the powerup if the child node has a compatible of
> "simple-sdio-powerup", to avoid it getting in the way of more complex poweron
> scenarios (which may require a separate pmic driver or some such) later.
>
> Regards,
>
> Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Mark Brown]

On Sun, May 25, 2014 at 09:20:52PM +0200, Hans de Goede wrote:
> On 05/25/2014 02:34 PM, Mark Brown wrote:

> > Why is that a problem - if we have no driver for the device there is no
> > point in powering it up in the first place is there?

> Well the driver may show up later, so if we only do the power-up once we have
> a driver, this means we need to re-check if we need to do the powerup later.

Sure.  Does that seem like a problem?

> Also the mmc people are very much against specifying a driver, as that is
> something which should be probed not specified. I agree with them I've
> already seen boards were more or less standardized sdio modules from different
> vendors are used, they have various standard sdio powerup related things, like
> an enable signal in standard places, but different editions of the boards
> have different sdio modules soldered on, using different drivers.

If the device isn't specified then presumably it'll power up with the
default sequence so we shouldn't need to worry about overriding anything
until we've powered up and enumerated.  The only time that there's a
problem and would need to specify exactly what the device is in the DTS
is if we need the custom sequence prior to being able to do that at
which point I don't see much option.

> I know that the DT is an ABI, and I'm not arguing for removing support for
> the simple-sdio-powerup compatible from the kernel when a more complex
> case arrives, nor am I arguing to remove it from the DT for existing working
> boards. The idea behind the simple-sdio-powerup compatible is that it makes
> the simple powerup behavior opt-in. So if a new board comes along which
> requires something more complex, the people working on this can do what ever
> they want / need without the simple powerup code getting in the way, as
> long as they don't *add* the simple-sdio-powerup compatible to their *new*
> DT file.

I don't understand why not powering the device up would be a sensible
default or why other OSs would also choose to implement things that way.
It would seem more natural that we would have the custom override in
place for things that need special handling, not the other way around -
I think that's really the difference between what we're saying.  My
expectation would be that we do the standard case by default and then
have the complex cases override rather than the other way around, that
way the standard case just works with no effort.

> Basically the idea behind the simple-sdio-powerup compatible is to make
> the worst case scenario for more complex boards to be the scenario which
> we have today, i.e. no support for sdio powerup at all, rather then having
> something in place which actually may get in the way, making things worse
> then they are today.

Well, if things aren't going to work either way for these devices
without extra stuff it seems it doesn't much matter but it helps the
simple case to have things default to working.  If we're in the case
where we've got a DT that just describes a slot and says to probe it
then powering the slot up seems sensible; if we're in the case where we
have explicit information on what's in the slot then that gives us an
opportunity to key custom behaviour off that explict information.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140526/b3cea728/attachment.sig>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/26/2014 12:38 PM, Mark Brown wrote:
> On Sun, May 25, 2014 at 09:20:52PM +0200, Hans de Goede wrote:

<snip>

>> Also the mmc people are very much against specifying a driver, as that is
>> something which should be probed not specified. I agree with them I've
>> already seen boards were more or less standardized sdio modules from different
>> vendors are used, they have various standard sdio powerup related things, like
>> an enable signal in standard places, but different editions of the boards
>> have different sdio modules soldered on, using different drivers.
> 
> If the device isn't specified then presumably it'll power up with the
> default sequence so we shouldn't need to worry about overriding anything
> until we've powered up and enumerated.  The only time that there's a
> problem and would need to specify exactly what the device is in the DTS
> is if we need the custom sequence prior to being able to do that at
> which point I don't see much option.

Specified != driver available. Determining whether or not we should power
up based purely on there being a compatible string is not going to work, as
even when using simple powerup we still need compatible strings for non
powerup settings like sdio signal drive strength, oob irq, etc.

So the only way this will work is if we delay powerup until we've a driver
available, which may be never if the module is not build, or whatever,
and without powerup the user is never going to figure out he is missing
a module. Basically adding a driver flag for blacklisting from the simple
power up logic means inserting an unnecessary initialization ordering issue,
which we really don't need as each ordering dependency is usually one too many.

> 
>> I know that the DT is an ABI, and I'm not arguing for removing support for
>> the simple-sdio-powerup compatible from the kernel when a more complex
>> case arrives, nor am I arguing to remove it from the DT for existing working
>> boards. The idea behind the simple-sdio-powerup compatible is that it makes
>> the simple powerup behavior opt-in. So if a new board comes along which
>> requires something more complex, the people working on this can do what ever
>> they want / need without the simple powerup code getting in the way, as
>> long as they don't *add* the simple-sdio-powerup compatible to their *new*
>> DT file.
> 
> I don't understand why not powering the device up would be a sensible
> default or why other OSs would also choose to implement things that way.

Because if we are not 100% sure that our simple power up logic will do the
job properly (i.e. in the right order), then the SAFE thing to do is to
not power up.

> It would seem more natural that we would have the custom override in
> place for things that need special handling, not the other way around -
> I think that's really the difference between what we're saying.

Normally I would agree with you, but doing powerup the wrong way is not
necessarily safe, so we really should not go and enable stuff just
because it is there (most of it will used standardized properties even for
custom power up sequences).

What if a user takes an older distro kernel, where the driver does not
set the opt-out flag you're suggesting since at that time it did not
have its own power logic, then tries to boot that with a dtb file written
for a newer kernel (where the driver does have the opt out flag), and we
start powering up things in the wrong order and let the magic smoke out
of various components on the board ?

> My expectation would be that we do the standard case by default and then
> have the complex cases override rather than the other way around, that
> way the standard case just works with no effort.

Adding "compatible = "simple-sdio-powerup" to the child node which has
the properties specifying which clks, etc. to enable really is no effort,
esp. since this will be done at the same time as creating the child nodes
in the first place.

Also note that this is a perfectly standard use of compatibles, compatibles
are typically used to indicate which driver should be used, in this case
the compatible indicates that the simple powerup driver should be used,
where as if another powerup driver should be used another compatbile will
be there instead.

Where as what you're suggesting is to always pick driver foo, unless
driver bar is available and has a special flag saying to not use foo, this
is a whole new way to use compatibles really, and not one which I think
we want to introduce.



> 
>> Basically the idea behind the simple-sdio-powerup compatible is to make
>> the worst case scenario for more complex boards to be the scenario which
>> we have today, i.e. no support for sdio powerup at all, rather then having
>> something in place which actually may get in the way, making things worse
>> then they are today.
> 
> Well, if things aren't going to work either way for these devices
> without extra stuff it seems it doesn't much matter but it helps the
> simple case to have things default to working.

The simple case still needs a child node describing the needed resources,
adding a compatible = "simple-sdio-powerup" to that at the same as creating
the child node in the first place really is no extra effort at all.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Mark Brown]

On Mon, May 26, 2014 at 01:12:43PM +0200, Hans de Goede wrote:
> On 05/26/2014 12:38 PM, Mark Brown wrote:
> > On Sun, May 25, 2014 at 09:20:52PM +0200, Hans de Goede wrote:

> > until we've powered up and enumerated.  The only time that there's a
> > problem and would need to specify exactly what the device is in the DTS
> > is if we need the custom sequence prior to being able to do that at
> > which point I don't see much option.

> Specified != driver available. Determining whether or not we should power
> up based purely on there being a compatible string is not going to work, as
> even when using simple powerup we still need compatible strings for non
> powerup settings like sdio signal drive strength, oob irq, etc.

If there are compatible strings but not one we've heard of then we're
back in the situation where we may as well not bother powering up in the
first place since we've no idea what to do with the device once it's
powered.  If we fall down a list of compatibles and decide that the
only thing we know about the device is that we can power it up (this is
distinct from the case where we probe the device) I'd expect the device
to be turned off.

> So the only way this will work is if we delay powerup until we've a driver
> available, which may be never if the module is not build, or whatever,
> and without powerup the user is never going to figure out he is missing
> a module. Basically adding a driver flag for blacklisting from the simple
> power up logic means inserting an unnecessary initialization ordering issue,
> which we really don't need as each ordering dependency is usually one too many.

I'm afraid I'm really not seeing a substantial problem either way here,
powering up the device isn't going to help us find a driver for it and
the UI around reporting that the device is there without a driver should
hopefully be unaffected by its power state.

> > I don't understand why not powering the device up would be a sensible
> > default or why other OSs would also choose to implement things that way.

> Because if we are not 100% sure that our simple power up logic will do the
> job properly (i.e. in the right order), then the SAFE thing to do is to
> not power up.

So long as we've got a clear way of saying that we might need to do
something special I don't think we should have an issue either way;
it's mostly a case of how we specify.

> What if a user takes an older distro kernel, where the driver does not
> set the opt-out flag you're suggesting since at that time it did not
> have its own power logic, then tries to boot that with a dtb file written
> for a newer kernel (where the driver does have the opt out flag), and we
> start powering up things in the wrong order and let the magic smoke out
> of various components on the board ?

Conversely what if someone fixes a bug in the standard power up sequence
in a newer version of the kernel but then tries to run older software?
There's plenty of ways things can go wrong with this stuff.

> Also note that this is a perfectly standard use of compatibles, compatibles
> are typically used to indicate which driver should be used, in this case
> the compatible indicates that the simple powerup driver should be used,
> where as if another powerup driver should be used another compatbile will
> be there instead.

We don't typically actually bind multiple compatibles for a single
device.  We've got a bunch of options we can choose from but we
generally pick the one that matches best and ignore the others.

> Where as what you're suggesting is to always pick driver foo, unless
> driver bar is available and has a special flag saying to not use foo, this
> is a whole new way to use compatibles really, and not one which I think
> we want to introduce.

I'm not sure I'm buying the idea that we have a powerup driver that's
meaningfully not part of the main device driver.

> > Well, if things aren't going to work either way for these devices
> > without extra stuff it seems it doesn't much matter but it helps the
> > simple case to have things default to working.

> The simple case still needs a child node describing the needed resources,
> adding a compatible = "simple-sdio-powerup" to that at the same as creating
> the child node in the first place really is no extra effort at all.

>From where I'm sitting it's more effort since instead of just putting
the device in there (and possibly also some other devices that are
software compatible) we have to put in another compatible string which
is really just a boolean flag to be used in conjunction with the others.
That's harder to think about and we clearly don't want to go through the
compatible list, decide that we don't know how to handle the device
except power it up so go and do that.

If it were done as something like "set boolean property X or
powerup-method = Y in the card description" or whatever it'd seem a bit
annoying but not a big deal, I think it's the fact that it's getting put
into the compatible list that's really concerning me.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140526/bd24f785/attachment.sig>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/26/2014 04:22 PM, Mark Brown wrote:
> On Mon, May 26, 2014 at 01:12:43PM +0200, Hans de Goede wrote:
>> On 05/26/2014 12:38 PM, Mark Brown wrote:
>>> On Sun, May 25, 2014 at 09:20:52PM +0200, Hans de Goede wrote:
> 
>>> until we've powered up and enumerated.  The only time that there's a
>>> problem and would need to specify exactly what the device is in the DTS
>>> is if we need the custom sequence prior to being able to do that at
>>> which point I don't see much option.
> 
>> Specified != driver available. Determining whether or not we should power
>> up based purely on there being a compatible string is not going to work, as
>> even when using simple powerup we still need compatible strings for non
>> powerup settings like sdio signal drive strength, oob irq, etc.
> 
> If there are compatible strings but not one we've heard of then we're
> back in the situation where we may as well not bother powering up in the
> first place since we've no idea what to do with the device once it's
> powered.  If we fall down a list of compatibles and decide that the
> only thing we know about the device is that we can power it up (this is
> distinct from the case where we probe the device) I'd expect the device
> to be turned off.

We won't know if we have a valid driver until we power on the device,
and probe it, not all sdio drivers will have compatibles, actually most of
them won't since sdio is a discoverable bus.

> 
>> So the only way this will work is if we delay powerup until we've a driver
>> available, which may be never if the module is not build, or whatever,
>> and without powerup the user is never going to figure out he is missing
>> a module. Basically adding a driver flag for blacklisting from the simple
>> power up logic means inserting an unnecessary initialization ordering issue,
>> which we really don't need as each ordering dependency is usually one too many.
> 
> I'm afraid I'm really not seeing a substantial problem either way here,
> powering up the device isn't going to help us find a driver for it and
> the UI around reporting that the device is there without a driver should
> hopefully be unaffected by its power state.

How can the UI be unaffected if we cannot tell userspace that there is
a device there and what its vendor / product ids are ?  We need to power
up the device before it will respond to probes...

> 
>>> I don't understand why not powering the device up would be a sensible
>>> default or why other OSs would also choose to implement things that way.
> 
>> Because if we are not 100% sure that our simple power up logic will do the
>> job properly (i.e. in the right order), then the SAFE thing to do is to
>> not power up.
> 
> So long as we've got a clear way of saying that we might need to do
> something special I don't think we should have an issue either way;
> it's mostly a case of how we specify.
> 
>> What if a user takes an older distro kernel, where the driver does not
>> set the opt-out flag you're suggesting since at that time it did not
>> have its own power logic, then tries to boot that with a dtb file written
>> for a newer kernel (where the driver does have the opt out flag), and we
>> start powering up things in the wrong order and let the magic smoke out
>> of various components on the board ?
> 
> Conversely what if someone fixes a bug in the standard power up sequence
> in a newer version of the kernel but then tries to run older software?
> There's plenty of ways things can go wrong with this stuff.
> 
>> Also note that this is a perfectly standard use of compatibles, compatibles
>> are typically used to indicate which driver should be used, in this case
>> the compatible indicates that the simple powerup driver should be used,
>> where as if another powerup driver should be used another compatbile will
>> be there instead.
> 
> We don't typically actually bind multiple compatibles for a single
> device.  We've got a bunch of options we can choose from but we
> generally pick the one that matches best and ignore the others.
> 
>> Where as what you're suggesting is to always pick driver foo, unless
>> driver bar is available and has a special flag saying to not use foo, this
>> is a whole new way to use compatibles really, and not one which I think
>> we want to introduce.
> 
> I'm not sure I'm buying the idea that we have a powerup driver that's
> meaningfully not part of the main device driver.

Well, if we merge some variant of Olof's code, we will have a powerup driver
that is part of the mmc core, and thus not of the sdio function driver.

>>> Well, if things aren't going to work either way for these devices
>>> without extra stuff it seems it doesn't much matter but it helps the
>>> simple case to have things default to working.
> 
>> The simple case still needs a child node describing the needed resources,
>> adding a compatible = "simple-sdio-powerup" to that at the same as creating
>> the child node in the first place really is no extra effort at all.
> 
> From where I'm sitting it's more effort since instead of just putting
> the device in there (and possibly also some other devices that are
> software compatible) we have to put in another compatible string which
> is really just a boolean flag to be used in conjunction with the others.
> That's harder to think about and we clearly don't want to go through the
> compatible list, decide that we don't know how to handle the device
> except power it up so go and do that.
> 
> If it were done as something like "set boolean property X or
> powerup-method = Y in the card description" or whatever it'd seem a bit
> annoying but not a big deal, I think it's the fact that it's getting put
> into the compatible list that's really concerning me.

Ok, so lets switch it over to a boolean, options for the name I see are:

linux,mmc-host-powerup  (opt in to powerup being dealt with by the mmc core, implementation specific)
simple-powerup		(platform neutral opt in to say just enable all resources and be done with it)
custom-powerup		(platform neutral opt out version of simple-powerup)
linux,custom-powerup	(same, but consider this something linux specific)

I think that it may be best to go with one of the 2 linux prefixed options.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Ulf Hansson]

[snip]

>>
>> We don't typically actually bind multiple compatibles for a single
>> device.  We've got a bunch of options we can choose from but we
>> generally pick the one that matches best and ignore the others.
>>
>>> Where as what you're suggesting is to always pick driver foo, unless
>>> driver bar is available and has a special flag saying to not use foo, this
>>> is a whole new way to use compatibles really, and not one which I think
>>> we want to introduce.
>>
>> I'm not sure I'm buying the idea that we have a powerup driver that's
>> meaningfully not part of the main device driver.

I am having a bit hard to follow the terminology here. :-) What is a
"powerup driver" and what is a "main device driver" in this context?

I had a slide which I used at a mmc subsystem crash course recently,
please have a look - hopefully this will help us to sort out this.

https://drive.google.com/file/d/0B2ePGK-iqMupbDQ2S0o5b3Bhek0/edit?usp=sharing

>
> Well, if we merge some variant of Olof's code, we will have a powerup driver
> that is part of the mmc core, and thus not of the sdio function driver.
>
>>>> Well, if things aren't going to work either way for these devices
>>>> without extra stuff it seems it doesn't much matter but it helps the
>>>> simple case to have things default to working.
>>
>>> The simple case still needs a child node describing the needed resources,
>>> adding a compatible = "simple-sdio-powerup" to that at the same as creating
>>> the child node in the first place really is no extra effort at all.
>>
>> From where I'm sitting it's more effort since instead of just putting
>> the device in there (and possibly also some other devices that are
>> software compatible) we have to put in another compatible string which
>> is really just a boolean flag to be used in conjunction with the others.
>> That's harder to think about and we clearly don't want to go through the
>> compatible list, decide that we don't know how to handle the device
>> except power it up so go and do that.
>>
>> If it were done as something like "set boolean property X or
>> powerup-method = Y in the card description" or whatever it'd seem a bit
>> annoying but not a big deal, I think it's the fact that it's getting put
>> into the compatible list that's really concerning me.
>
> Ok, so lets switch it over to a boolean, options for the name I see are:
>
> linux,mmc-host-powerup  (opt in to powerup being dealt with by the mmc core, implementation specific)
> simple-powerup          (platform neutral opt in to say just enable all resources and be done with it)
> custom-powerup          (platform neutral opt out version of simple-powerup)
> linux,custom-powerup    (same, but consider this something linux specific)

This seems reasonable to me.

Well, I don't like the "simple-powerup", because I think a simple
powerup sequence is to me already supported by the mmc core, through
the regular host interface (->set_ios()).

If I understand correctly, this binding is supposed to be configured
per host device and thus also per host device slots?

Kind regards
Ulf Hansson

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Mark Brown]

On Mon, May 26, 2014 at 06:07:18PM +0200, Ulf Hansson wrote:

> >> I'm not sure I'm buying the idea that we have a powerup driver that's
> >> meaningfully not part of the main device driver.

> I am having a bit hard to follow the terminology here. :-) What is a
> "powerup driver" and what is a "main device driver" in this context?

Yes, that's mostly what I'm saying as well.  The main device driver is
the driver that will bind to the SDIO device and control it, the powerup
driver is the separate bit of code which will ensure that device is
powered up - calling it a device driver seems a bit of a misnomer to me.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140526/c44629f8/attachment.sig>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/26/2014 06:07 PM, Ulf Hansson wrote:
> [snip]
> 
>>>
>>> We don't typically actually bind multiple compatibles for a single
>>> device.  We've got a bunch of options we can choose from but we
>>> generally pick the one that matches best and ignore the others.
>>>
>>>> Where as what you're suggesting is to always pick driver foo, unless
>>>> driver bar is available and has a special flag saying to not use foo, this
>>>> is a whole new way to use compatibles really, and not one which I think
>>>> we want to introduce.
>>>
>>> I'm not sure I'm buying the idea that we have a powerup driver that's
>>> meaningfully not part of the main device driver.
> 
> I am having a bit hard to follow the terminology here. :-) What is a
> "powerup driver" and what is a "main device driver" in this context?
> 
> I had a slide which I used at a mmc subsystem crash course recently,
> please have a look - hopefully this will help us to sort out this.
> 
> https://drive.google.com/file/d/0B2ePGK-iqMupbDQ2S0o5b3Bhek0/edit?usp=sharing

Right, the problem is that in the case(s) we're talking about before the sdio
device in question can be probed the sdio device may need to have several clk
signals enables, reset signal deasserted, etc.

Some piece of code needs to do that, the proposal so far has been to let the
mmc-core deal with this, which will likely work fine for 90% of the cases
were we need any form of "powerup", but in some more complex case this may need
to happen in a specific order / with specific delays, etc. In this case some
separate piece of board and/or sdio device specific code would need to take
care of this, hence I was talking about a "powerup-driver", which I agree is
not the best name. So lets just forget about the power-driver nomenclature
completely.

> 
>>
>> Well, if we merge some variant of Olof's code, we will have a powerup driver
>> that is part of the mmc core, and thus not of the sdio function driver.
>>
>>>>> Well, if things aren't going to work either way for these devices
>>>>> without extra stuff it seems it doesn't much matter but it helps the
>>>>> simple case to have things default to working.
>>>
>>>> The simple case still needs a child node describing the needed resources,
>>>> adding a compatible = "simple-sdio-powerup" to that at the same as creating
>>>> the child node in the first place really is no extra effort at all.
>>>
>>> From where I'm sitting it's more effort since instead of just putting
>>> the device in there (and possibly also some other devices that are
>>> software compatible) we have to put in another compatible string which
>>> is really just a boolean flag to be used in conjunction with the others.
>>> That's harder to think about and we clearly don't want to go through the
>>> compatible list, decide that we don't know how to handle the device
>>> except power it up so go and do that.
>>>
>>> If it were done as something like "set boolean property X or
>>> powerup-method = Y in the card description" or whatever it'd seem a bit
>>> annoying but not a big deal, I think it's the fact that it's getting put
>>> into the compatible list that's really concerning me.
>>
>> Ok, so lets switch it over to a boolean, options for the name I see are:
>>
>> linux,mmc-host-powerup  (opt in to powerup being dealt with by the mmc core, implementation specific)
>> simple-powerup          (platform neutral opt in to say just enable all resources and be done with it)
>> custom-powerup          (platform neutral opt out version of simple-powerup)
>> linux,custom-powerup    (same, but consider this something linux specific)
> 
> This seems reasonable to me.

This being the last option ?

> Well, I don't like the "simple-powerup", because I think a simple
> powerup sequence is to me already supported by the mmc core, through
> the regular host interface (->set_ios()).
> 
> If I understand correctly, this binding is supposed to be configured
> per host device and thus also per host device slots?

Yes, although I must admit that have not thought about how to deal with
slots, I've no experience with the mmc slots concept at all, or is slot
just a different name for sdio function ?

Thinking more about this, maybe we can solve the problem of people
worrying about complex power-on scenarios coming around later, by
also encoding the sequence in dt, e.g. something like:

                mmc3: mmc at 01c12000 {
                        #address-cells = <1>;
                        #size-cells = <0>;

                        pinctrl-names = "default";
                        pinctrl-0 = <&mmc3_pins_a>;
                        vmmc-supply = <&reg_vmmc3>;
                        bus-width = <4>;
                        non-removable;
                        status = "okay";

                        brcmf: bcrmf at 1 {
                                reg = <1>;
                                compatible = "brcm,bcm43xx-fmac";
                                interrupt-parent = <&pio>;
                                interrupts = <10 8>; /* PH10 / EINT10 */
                                interrupt-names = "host-wake";
				clocks = <&clk_out_a>;
				clock-names = "clk_32khz";
				gpios = <&pio 7 24 0>;
				gpio-names = "gpio_reg_enable";
				power-on-seq = "gpio_reg_enable", "usleep 1000", "clk_32khz", "usleep 200";
                                status = "okay";				
                        };
                };

Where power-on-seq would tell the mmc-core exactly how to bring up the sdio
device, using standard prefixes so that the mmc-core knows that something is
a clock / gpio / reset / whatever.

This should pretty much work for everything, and if something comes around which
really needs some custom bit of code to bring it up, the mmc-core powerup stuff
can simply be opted-out by leaving out the power-on-seq property.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Ulf Hansson]

[snip]

>> I am having a bit hard to follow the terminology here. :-) What is a
>> "powerup driver" and what is a "main device driver" in this context?
>>
>> I had a slide which I used at a mmc subsystem crash course recently,
>> please have a look - hopefully this will help us to sort out this.
>>
>> https://drive.google.com/file/d/0B2ePGK-iqMupbDQ2S0o5b3Bhek0/edit?usp=sharing
>
> Right, the problem is that in the case(s) we're talking about before the sdio
> device in question can be probed the sdio device may need to have several clk
> signals enables, reset signal deasserted, etc.

Yes. That's an important point.

>
> Some piece of code needs to do that, the proposal so far has been to let the
> mmc-core deal with this, which will likely work fine for 90% of the cases
> were we need any form of "powerup", but in some more complex case this may need
> to happen in a specific order / with specific delays, etc. In this case some
> separate piece of board and/or sdio device specific code would need to take
> care of this, hence I was talking about a "powerup-driver", which I agree is
> not the best name. So lets just forget about the power-driver nomenclature
> completely.

I have now given this a serious thought, sorry for not doing this
earlier. Please consider the following ideas and statements. Feel free
to shoot them down. :-)

DT is supposed to described the hardware. Encoding power up sequences
within DT, to for example let the mmc core handle this means we are
touching the concept of open firmware. This is not what DT should be
used for.

To describe the HW in DT, the embedded SDIO card (actually it could be
any type of embedded card) shall be modelled as a child node to the
mmc host in DT. Similar to what you have proposed, but with the
difference that the child node _must_ contain a DT compatible string,
which means a "powerup-driver" can be probed.

Yes, I understand we might need one DT compatible string per board,
but that's because we need to model the hardware - and it differs.

To clarify my view, we do need a "powerup-driver" and the primary
reason is that we must not model "power up sequences" within DT.
Typically I see the "powerup-driver" as a simple platform driver
attached to the platform bus, but I that could of course differ.

[snip]

>>>
>>> Ok, so lets switch it over to a boolean, options for the name I see are:
>>>
>>> linux,mmc-host-powerup  (opt in to powerup being dealt with by the mmc core, implementation specific)
>>> simple-powerup          (platform neutral opt in to say just enable all resources and be done with it)
>>> custom-powerup          (platform neutral opt out version of simple-powerup)
>>> linux,custom-powerup    (same, but consider this something linux specific)
>>
>> This seems reasonable to me.
>
> This being the last option ?

Sorry for the confusion here. As stated above, we need a compatible string.

>
>> Well, I don't like the "simple-powerup", because I think a simple
>> powerup sequence is to me already supported by the mmc core, through
>> the regular host interface (->set_ios()).
>>
>> If I understand correctly, this binding is supposed to be configured
>> per host device and thus also per host device slots?
>
> Yes, although I must admit that have not thought about how to deal with
> slots, I've no experience with the mmc slots concept at all, or is slot
> just a different name for sdio function ?

Some mmc hosts may support more than one slot. Thus they can operate
on more than one card.

Currently, there are no support for this in the mmc core. There can
only be one card per host, but that's due to software limitation of
the mmc stack.  Following your suggestion; modelling the card as child
node under the mmc host, can easily be extended to support more than
one slot.

The slot will be the first level of child node under the mmc host,
then each slot may have a child node which models the embedded card.
But, let's leave that discussion for now. :-)

>
> Thinking more about this, maybe we can solve the problem of people
> worrying about complex power-on scenarios coming around later, by
> also encoding the sequence in dt, e.g. something like:
>
>                 mmc3: mmc at 01c12000 {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>
>                         pinctrl-names = "default";
>                         pinctrl-0 = <&mmc3_pins_a>;
>                         vmmc-supply = <&reg_vmmc3>;
>                         bus-width = <4>;
>                         non-removable;
>                         status = "okay";
>
>                         brcmf: bcrmf at 1 {
>                                 reg = <1>;
>                                 compatible = "brcm,bcm43xx-fmac";
>                                 interrupt-parent = <&pio>;
>                                 interrupts = <10 8>; /* PH10 / EINT10 */
>                                 interrupt-names = "host-wake";
>                                 clocks = <&clk_out_a>;
>                                 clock-names = "clk_32khz";
>                                 gpios = <&pio 7 24 0>;
>                                 gpio-names = "gpio_reg_enable";
>                                 power-on-seq = "gpio_reg_enable", "usleep 1000", "clk_32khz", "usleep 200";
>                                 status = "okay";
>                         };
>                 };
>
> Where power-on-seq would tell the mmc-core exactly how to bring up the sdio
> device, using standard prefixes so that the mmc-core knows that something is
> a clock / gpio / reset / whatever.

I suppose I already made my point here. I don't think this is not a good idea.

>
> This should pretty much work for everything, and if something comes around which
> really needs some custom bit of code to bring it up, the mmc-core powerup stuff
> can simply be opted-out by leaving out the power-on-seq property.
>
> Regards,
>
> Hans

Here are some more thoughts of how this should be implemented.

Synchronization point:
My idea is to let the mmc_of_parse() function act as the
synchronization point, to make sure we are able to perform a full
power up of the card - before we actually start initialization of it.

If the mmc host has a child node to model the card, mmc_of_parse()
needs to verify that a struct device is created for it and that it's
corresponding "powerup driver" has been probed successfully.

If the mmc_of_parse() returns -EPROBE_DEFER, the mmc host driver will
return the same error code from it's ->probe(). This provides us with
the ability of waiting for the "powerup driver" to be probed.

If the mmc_of_parse() returns another error code, due to that the
"powerup driver" failed to be probed, the mmc host driver's ->probe()
will return the same error code and consequentially no power up of the
card will be performed at all.


Powerup driver's ->probe():
Typically the "powerup driver" will need to register a few callback
functions towards the mmc core. Typically at mmc_of_parse(), those
callbacks will have to be connected to a particular mmc host.

I would like to see three different callbacks, mirroring each of the
mmc_ios power_mode states MMC_POWER_OFF|UP|ON.


The power up sequence, performed by the mmc core:
The mmc_power_up|off functions, will invoke the registered "powerup
driver's" callbacks if they exists for the particular host it operates
on.


Comments are of course welcome! :-)

Kind regards
Ulf Hansson

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Mark Brown]

On Mon, May 26, 2014 at 07:55:47PM +0200, Hans de Goede wrote:

> 				power-on-seq = "gpio_reg_enable", "usleep 1000", "clk_32khz", "usleep 200";

...

> Where power-on-seq would tell the mmc-core exactly how to bring up the sdio
> device, using standard prefixes so that the mmc-core knows that something is
> a clock / gpio / reset / whatever.

There was some work on such generic power on sequences in DT from 
Alexandre Courbot a while ago, search for "Runtime Interpreter Power
Sequences".  It ground to a halt IIRC mainly because it was never clear
that this should ever be encoded outside of the driver for a device but
the binding was pretty good and well reviewed, it's a good place to
start if there's a desire to do things like this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140527/b99e7b44/attachment.sig>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Mark Brown]

On Tue, May 27, 2014 at 03:50:33PM +0200, Ulf Hansson wrote:

> To describe the HW in DT, the embedded SDIO card (actually it could be
> any type of embedded card) shall be modelled as a child node to the
> mmc host in DT. Similar to what you have proposed, but with the
> difference that the child node _must_ contain a DT compatible string,
> which means a "powerup-driver" can be probed.

> Yes, I understand we might need one DT compatible string per board,
> but that's because we need to model the hardware - and it differs.

> To clarify my view, we do need a "powerup-driver" and the primary
> reason is that we must not model "power up sequences" within DT.
> Typically I see the "powerup-driver" as a simple platform driver
> attached to the platform bus, but I that could of course differ.

This then either conflicts with cases where we need to describe the
actual contents of the slot with a compatible string or means that the
SDIO driver needs to handle powerup sequencing since we should be
binding to the first compatible we find.  If the host controller driver
and/or subsystem is going to deal with the powering up it's not clear
that it specifically needs to be the compatible property that's used
to determine the powerup method, it could just be a boolean or a
'power-method = blah' property (where blah is one of a series of strings
defining methods).  Alternatively we could have separate nodes for the
slot and SDIO device but that feels meh.  What's the hard requirement
for it to specifically be a compatible property?

> The slot will be the first level of child node under the mmc host,
> then each slot may have a child node which models the embedded card.
> But, let's leave that discussion for now. :-)

OK, that's the separate node for the slot and device.

> Powerup driver's ->probe():
> Typically the "powerup driver" will need to register a few callback
> functions towards the mmc core. Typically at mmc_of_parse(), those
> callbacks will have to be connected to a particular mmc host.

> I would like to see three different callbacks, mirroring each of the
> mmc_ios power_mode states MMC_POWER_OFF|UP|ON.

> The power up sequence, performed by the mmc core:
> The mmc_power_up|off functions, will invoke the registered "powerup
> driver's" callbacks if they exists for the particular host it operates
> on.

There's also the need for the SDIO device to be able to get at the
resources provided and actively work with them at runtime if it wants to
manage things more actively (partial poweroff for low power states or
managing clock rates for example).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140527/4a1b1382/attachment-0001.sig>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Olof Johansson]

On Tue, May 27, 2014 at 06:53:26PM +0100, Mark Brown wrote:
> On Tue, May 27, 2014 at 03:50:33PM +0200, Ulf Hansson wrote:
> 
> > To describe the HW in DT, the embedded SDIO card (actually it could be
> > any type of embedded card) shall be modelled as a child node to the
> > mmc host in DT. Similar to what you have proposed, but with the
> > difference that the child node _must_ contain a DT compatible string,
> > which means a "powerup-driver" can be probed.
> 
> > Yes, I understand we might need one DT compatible string per board,
> > but that's because we need to model the hardware - and it differs.
> 
> > To clarify my view, we do need a "powerup-driver" and the primary
> > reason is that we must not model "power up sequences" within DT.
> > Typically I see the "powerup-driver" as a simple platform driver
> > attached to the platform bus, but I that could of course differ.
> 
> This then either conflicts with cases where we need to describe the
> actual contents of the slot with a compatible string or means that the
> SDIO driver needs to handle powerup sequencing since we should be
> binding to the first compatible we find.  If the host controller driver
> and/or subsystem is going to deal with the powering up it's not clear
> that it specifically needs to be the compatible property that's used
> to determine the powerup method, it could just be a boolean or a
> 'power-method = blah' property (where blah is one of a series of strings
> defining methods).  Alternatively we could have separate nodes for the
> slot and SDIO device but that feels meh.  What's the hard requirement
> for it to specifically be a compatible property?

+1. Just because we have a subnode in a device tree, we don't have to have
a driver bind against it. The MMC core code could go down into the subnodes,
find a "power-method = <foo>" property and go ahead and parse the rest of it.
There's no requirement that we do this through the Linux driver model of
probe(), etc.

> > The slot will be the first level of child node under the mmc host,
> > then each slot may have a child node which models the embedded card.
> > But, let's leave that discussion for now. :-)
> 
> OK, that's the separate node for the slot and device.
> 
> > Powerup driver's ->probe():
> > Typically the "powerup driver" will need to register a few callback
> > functions towards the mmc core. Typically at mmc_of_parse(), those
> > callbacks will have to be connected to a particular mmc host.
> 
> > I would like to see three different callbacks, mirroring each of the
> > mmc_ios power_mode states MMC_POWER_OFF|UP|ON.
> 
> > The power up sequence, performed by the mmc core:
> > The mmc_power_up|off functions, will invoke the registered "powerup
> > driver's" callbacks if they exists for the particular host it operates
> > on.
> 
> There's also the need for the SDIO device to be able to get at the
> resources provided and actively work with them at runtime if it wants to
> manage things more actively (partial poweroff for low power states or
> managing clock rates for example).

Again, I think it gets overly complicated by using a full driver for the
power management. Abstracted out into something separate and scalable
as number of devices grow? Sure, definitely. As a driver? Not convinced.


-Olof

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Arend van Spriel]

On 05/27/14 20:55, Olof Johansson wrote:
> On Tue, May 27, 2014 at 06:53:26PM +0100, Mark Brown wrote:
>> On Tue, May 27, 2014 at 03:50:33PM +0200, Ulf Hansson wrote:
>>
>>> To describe the HW in DT, the embedded SDIO card (actually it could be
>>> any type of embedded card) shall be modelled as a child node to the
>>> mmc host in DT. Similar to what you have proposed, but with the
>>> difference that the child node _must_ contain a DT compatible string,
>>> which means a "powerup-driver" can be probed.
>>
>>> Yes, I understand we might need one DT compatible string per board,
>>> but that's because we need to model the hardware - and it differs.
>>
>>> To clarify my view, we do need a "powerup-driver" and the primary
>>> reason is that we must not model "power up sequences" within DT.
>>> Typically I see the "powerup-driver" as a simple platform driver
>>> attached to the platform bus, but I that could of course differ.
>>
>> This then either conflicts with cases where we need to describe the
>> actual contents of the slot with a compatible string or means that the
>> SDIO driver needs to handle powerup sequencing since we should be
>> binding to the first compatible we find.  If the host controller driver
>> and/or subsystem is going to deal with the powering up it's not clear
>> that it specifically needs to be the compatible property that's used
>> to determine the powerup method, it could just be a boolean or a
>> 'power-method = blah' property (where blah is one of a series of strings
>> defining methods).  Alternatively we could have separate nodes for the
>> slot and SDIO device but that feels meh.  What's the hard requirement
>> for it to specifically be a compatible property?
>
> +1. Just because we have a subnode in a device tree, we don't have to have
> a driver bind against it. The MMC core code could go down into the subnodes,
> find a "power-method =<foo>" property and go ahead and parse the rest of it.
> There's no requirement that we do this through the Linux driver model of
> probe(), etc.

I prefer a power-method property over compatible matching. The fact that 
the subnode has a compatible string and properties for the device driver 
should not matter.

>>> The slot will be the first level of child node under the mmc host,
>>> then each slot may have a child node which models the embedded card.
>>> But, let's leave that discussion for now. :-)
>>
>> OK, that's the separate node for the slot and device.
>>
>>> Powerup driver's ->probe():
>>> Typically the "powerup driver" will need to register a few callback
>>> functions towards the mmc core. Typically at mmc_of_parse(), those
>>> callbacks will have to be connected to a particular mmc host.
>>
>>> I would like to see three different callbacks, mirroring each of the
>>> mmc_ios power_mode states MMC_POWER_OFF|UP|ON.
>>
>>> The power up sequence, performed by the mmc core:
>>> The mmc_power_up|off functions, will invoke the registered "powerup
>>> driver's" callbacks if they exists for the particular host it operates
>>> on.
>>
>> There's also the need for the SDIO device to be able to get at the
>> resources provided and actively work with them at runtime if it wants to
>> manage things more actively (partial poweroff for low power states or
>> managing clock rates for example).
>
> Again, I think it gets overly complicated by using a full driver for the
> power management. Abstracted out into something separate and scalable
> as number of devices grow? Sure, definitely. As a driver? Not convinced.

I think somewhere in the thread Hans already indicated the term 'driver' 
was a misnomer. While monitoring the discussion I was wondering whether 
this type of power-up sequence handling is specific to mmc/sdio or could 
it also apply to say spi, i2c, or whatever. In other words, could the 
power-up sequence code be placed in drivers/of code.

Regards,
Arend

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Ulf Hansson]

On 27 May 2014 19:53, Mark Brown <broonie@kernel.org> wrote:
> On Tue, May 27, 2014 at 03:50:33PM +0200, Ulf Hansson wrote:
>
>> To describe the HW in DT, the embedded SDIO card (actually it could be
>> any type of embedded card) shall be modelled as a child node to the
>> mmc host in DT. Similar to what you have proposed, but with the
>> difference that the child node _must_ contain a DT compatible string,
>> which means a "powerup-driver" can be probed.
>
>> Yes, I understand we might need one DT compatible string per board,
>> but that's because we need to model the hardware - and it differs.
>
>> To clarify my view, we do need a "powerup-driver" and the primary
>> reason is that we must not model "power up sequences" within DT.
>> Typically I see the "powerup-driver" as a simple platform driver
>> attached to the platform bus, but I that could of course differ.
>
> This then either conflicts with cases where we need to describe the
> actual contents of the slot with a compatible string or means that the
> SDIO driver needs to handle powerup sequencing since we should be
> binding to the first compatible we find.  If the host controller driver
> and/or subsystem is going to deal with the powering up it's not clear
> that it specifically needs to be the compatible property that's used
> to determine the powerup method, it could just be a boolean or a
> 'power-method = blah' property (where blah is one of a series of strings
> defining methods).  Alternatively we could have separate nodes for the
> slot and SDIO device but that feels meh.  What's the hard requirement
> for it to specifically be a compatible property?

Since the sdio bus is a discoverable bus, we must leave the "SDIO card
device id" outside the information of the DT child node. Instead the
"SDIO card device id" will be fetched once the card has been properly
powered and initialized. From sdio bus' ->match(), the "SDIO func
driver id" will be matched with the "SDIO card device id".

What we need to describe in the child node is only what "powerup
driver" to use (an obviously it's resources). I don't see why there
should be a conflict in using a compatible property, but I might be
missing your point.

>
>> The slot will be the first level of child node under the mmc host,
>> then each slot may have a child node which models the embedded card.
>> But, let's leave that discussion for now. :-)
>
> OK, that's the separate node for the slot and device.
>
>> Powerup driver's ->probe():
>> Typically the "powerup driver" will need to register a few callback
>> functions towards the mmc core. Typically at mmc_of_parse(), those
>> callbacks will have to be connected to a particular mmc host.
>
>> I would like to see three different callbacks, mirroring each of the
>> mmc_ios power_mode states MMC_POWER_OFF|UP|ON.
>
>> The power up sequence, performed by the mmc core:
>> The mmc_power_up|off functions, will invoke the registered "powerup
>> driver's" callbacks if they exists for the particular host it operates
>> on.
>
> There's also the need for the SDIO device to be able to get at the
> resources provided and actively work with them at runtime if it wants to
> manage things more actively (partial poweroff for low power states or
> managing clock rates for example).

Yes, that's is also a reason to why I thought it would make sense to
have a "powerup driver". Typically this driver should support runtime
PM to handle these kind of fine grained power management.

The SDIO func driver will have access (through the struct
mmc_card|host) to the "powerup driver's" struct device, and can use
pm_runtime_get|put() for it, to tell the "powerup driver" when it may
go into power save state.

This kind of fine grained power management is not related directly to
the SDIO specification, and should not be a part of the mmc core in my
opinion.

If we don't have a compatible string in the DT child node of the mmc
host, but just a DT property - is there a way to achieve the above
anyway?

Kind regards
Ulf Hansson

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Ulf Hansson]

On 27 May 2014 20:55, Olof Johansson <olof@lixom.net> wrote:
> On Tue, May 27, 2014 at 06:53:26PM +0100, Mark Brown wrote:
>> On Tue, May 27, 2014 at 03:50:33PM +0200, Ulf Hansson wrote:
>>
>> > To describe the HW in DT, the embedded SDIO card (actually it could be
>> > any type of embedded card) shall be modelled as a child node to the
>> > mmc host in DT. Similar to what you have proposed, but with the
>> > difference that the child node _must_ contain a DT compatible string,
>> > which means a "powerup-driver" can be probed.
>>
>> > Yes, I understand we might need one DT compatible string per board,
>> > but that's because we need to model the hardware - and it differs.
>>
>> > To clarify my view, we do need a "powerup-driver" and the primary
>> > reason is that we must not model "power up sequences" within DT.
>> > Typically I see the "powerup-driver" as a simple platform driver
>> > attached to the platform bus, but I that could of course differ.
>>
>> This then either conflicts with cases where we need to describe the
>> actual contents of the slot with a compatible string or means that the
>> SDIO driver needs to handle powerup sequencing since we should be
>> binding to the first compatible we find.  If the host controller driver
>> and/or subsystem is going to deal with the powering up it's not clear
>> that it specifically needs to be the compatible property that's used
>> to determine the powerup method, it could just be a boolean or a
>> 'power-method = blah' property (where blah is one of a series of strings
>> defining methods).  Alternatively we could have separate nodes for the
>> slot and SDIO device but that feels meh.  What's the hard requirement
>> for it to specifically be a compatible property?
>
> +1. Just because we have a subnode in a device tree, we don't have to have
> a driver bind against it. The MMC core code could go down into the subnodes,
> find a "power-method = <foo>" property and go ahead and parse the rest of it.
> There's no requirement that we do this through the Linux driver model of
> probe(), etc.

Using "power-method" might work.

It's would simplify things quite a bit regarding the synchronization
point, in the mmc_of_parse().

But... does it prevent us from handling fine grained power management
through runtime PM? See more comments below.

>
>> > The slot will be the first level of child node under the mmc host,
>> > then each slot may have a child node which models the embedded card.
>> > But, let's leave that discussion for now. :-)
>>
>> OK, that's the separate node for the slot and device.
>>
>> > Powerup driver's ->probe():
>> > Typically the "powerup driver" will need to register a few callback
>> > functions towards the mmc core. Typically at mmc_of_parse(), those
>> > callbacks will have to be connected to a particular mmc host.
>>
>> > I would like to see three different callbacks, mirroring each of the
>> > mmc_ios power_mode states MMC_POWER_OFF|UP|ON.
>>
>> > The power up sequence, performed by the mmc core:
>> > The mmc_power_up|off functions, will invoke the registered "powerup
>> > driver's" callbacks if they exists for the particular host it operates
>> > on.
>>
>> There's also the need for the SDIO device to be able to get at the
>> resources provided and actively work with them at runtime if it wants to
>> manage things more actively (partial poweroff for low power states or
>> managing clock rates for example).
>
> Again, I think it gets overly complicated by using a full driver for the
> power management. Abstracted out into something separate and scalable
> as number of devices grow? Sure, definitely. As a driver? Not convinced.

To handle the fine grained power management, runtime PM is for sure
what we should use here. So, then we need a struct device to be
created to represent the power-method.

Now, if we don't implement this through a "powerup driver" which may
have runtime PM callbacks set up - how do we handle this?

I suppose we could register a specific "mmc power bus", and create/add
the struct device representing the power-method to it. Would that
work?

Kind regards
Ulf Hansson

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/27/2014 03:50 PM, Ulf Hansson wrote:
> [snip]
> 
>>> I am having a bit hard to follow the terminology here. :-) What is a
>>> "powerup driver" and what is a "main device driver" in this context?
>>>
>>> I had a slide which I used at a mmc subsystem crash course recently,
>>> please have a look - hopefully this will help us to sort out this.
>>>
>>> https://drive.google.com/file/d/0B2ePGK-iqMupbDQ2S0o5b3Bhek0/edit?usp=sharing
>>
>> Right, the problem is that in the case(s) we're talking about before the sdio
>> device in question can be probed the sdio device may need to have several clk
>> signals enables, reset signal deasserted, etc.
> 
> Yes. That's an important point.
> 
>>
>> Some piece of code needs to do that, the proposal so far has been to let the
>> mmc-core deal with this, which will likely work fine for 90% of the cases
>> were we need any form of "powerup", but in some more complex case this may need
>> to happen in a specific order / with specific delays, etc. In this case some
>> separate piece of board and/or sdio device specific code would need to take
>> care of this, hence I was talking about a "powerup-driver", which I agree is
>> not the best name. So lets just forget about the power-driver nomenclature
>> completely.
> 
> I have now given this a serious thought, sorry for not doing this
> earlier. Please consider the following ideas and statements. Feel free
> to shoot them down. :-)
> 
> DT is supposed to described the hardware. Encoding power up sequences
> within DT, to for example let the mmc core handle this means we are
> touching the concept of open firmware. This is not what DT should be
> used for.

Ok, I already expected an answer like this, but I still wanted to voice
the idea of encoding the sequence into the dt.

> To describe the HW in DT, the embedded SDIO card (actually it could be
> any type of embedded card) shall be modelled as a child node to the
> mmc host in DT. Similar to what you have proposed, but with the
> difference that the child node _must_ contain a DT compatible string,
> which means a "powerup-driver" can be probed.
> 
> Yes, I understand we might need one DT compatible string per board,
> but that's because we need to model the hardware - and it differs.
> 
> To clarify my view, we do need a "powerup-driver" and the primary
> reason is that we must not model "power up sequences" within DT.
> Typically I see the "powerup-driver" as a simple platform driver
> attached to the platform bus, but I that could of course differ.

Ok, but if it is a platform device, then how can the mmc-host
depend on it ? Or will the mmc-core code instantiate this
platform device based on the child node, rather then it being
instantiated directly by the platform bus ?

<snip>

>>> Well, I don't like the "simple-powerup", because I think a simple
>>> powerup sequence is to me already supported by the mmc core, through
>>> the regular host interface (->set_ios()).
>>>
>>> If I understand correctly, this binding is supposed to be configured
>>> per host device and thus also per host device slots?
>>
>> Yes, although I must admit that have not thought about how to deal with
>> slots, I've no experience with the mmc slots concept at all, or is slot
>> just a different name for sdio function ?
> 
> Some mmc hosts may support more than one slot. Thus they can operate
> on more than one card.
> 
> Currently, there are no support for this in the mmc core. There can
> only be one card per host, but that's due to software limitation of
> the mmc stack.  Following your suggestion; modelling the card as child
> node under the mmc host, can easily be extended to support more than
> one slot.

Actually what I'm suggesting is based on Sascha Hauer's
"mmc: Add SDIO function devicetree subnode parsing"

Patch, which models the sdio functions as child nodes, (with the
one with reg = <0> being the card itself) this also makes sense since each
sdio-function gets its own device representing it, so having one child node
per sdio-functions leads to one child node per device which seems sensible.

This means how ever that if want to prepare for a future were we support
more then one slot we need an extra level for the slot, so we would
get something like this:

mmc3: mmc at 01c12000 {
    #address-cells = <1>;
    #size-cells = <0>;

    pinctrl-names = "default";
    pinctrl-0 = <&mmc3_pins_a>;
    vmmc-supply = <&reg_vcc3v3>;
    bus-width = <4>;
    non-removable;
    status = "okay";

    mmc3slot0: mmc3slot at 0 {
        #address-cells = <1>;
        #size-cells = <0>;

        reg = <0>;
        clocks = <&clk_out_a>, <&clk_out_b>;
        clock-frequency = <32768>, <26000000>;
	gpios = <&pio 4 5 0 &pio 1 18 0>;

        brcmf: bcrmf at 1 {
            reg = <1>;
            compatible = "brcm,bcm43xx-fmac";
            interrupt-parent = <&pio>;
            interrupts = <10 4>; /* PH10 / EINT10 */
            interrupt-names = "host-wake";
            status = "okay";
        };
    };
};

Note that I've put the powerup related resources at the slot
child-node level, I think having the slots as an explicit level
actually solves a lot of the powerup discussion we've been having.

The slot child node can have the properties for the nodes needed
to powerup the device attached to the slot.

The slot child node can have a compatible string, independent of the
sdio-func child node, and this slot child node compatible can then
be used to select a power-up driver for the slot (which then power
up the sdio device / sdio functions before probing).

>From a Linux implementation pov, this would mean that the mmc-core
would instantiate platform-devices for the slot child nodes and then
let the platform bus take care of loading a driver for this.
After instantiating the platform device, the mmc-core will check if
there is a driver bound, and if not return -EPROBEDEFER to deal with
the power-up driver (module) not being loaded yet, or the powerup
driver actually hitting -EPROBEDEFER itself. This means we will need
a small "null" driver to deal with cases where we don't need any
powerup, but for some reason still need child nodes. Or we could
skip the check if there is no compatible property in the slot child
node, but I think the latter option may come back to bite us later.

> The slot will be the first level of child node under the mmc host,
> then each slot may have a child node which models the embedded card.
> But, let's leave that discussion for now. :-)

Right, I agree, see above, but I believe we need to settle this now,
even without solving the powerup sequence thing we (at least I) need
to get the child node structure + addressing set in stone so that
we can add oob irq support, which requires storing info in the
sdio-func child node.


> 
>>
>> Thinking more about this, maybe we can solve the problem of people
>> worrying about complex power-on scenarios coming around later, by
>> also encoding the sequence in dt, e.g. something like:
>>
>>                 mmc3: mmc at 01c12000 {
>>                         #address-cells = <1>;
>>                         #size-cells = <0>;
>>
>>                         pinctrl-names = "default";
>>                         pinctrl-0 = <&mmc3_pins_a>;
>>                         vmmc-supply = <&reg_vmmc3>;
>>                         bus-width = <4>;
>>                         non-removable;
>>                         status = "okay";
>>
>>                         brcmf: bcrmf at 1 {
>>                                 reg = <1>;
>>                                 compatible = "brcm,bcm43xx-fmac";
>>                                 interrupt-parent = <&pio>;
>>                                 interrupts = <10 8>; /* PH10 / EINT10 */
>>                                 interrupt-names = "host-wake";
>>                                 clocks = <&clk_out_a>;
>>                                 clock-names = "clk_32khz";
>>                                 gpios = <&pio 7 24 0>;
>>                                 gpio-names = "gpio_reg_enable";
>>                                 power-on-seq = "gpio_reg_enable", "usleep 1000", "clk_32khz", "usleep 200";
>>                                 status = "okay";
>>                         };
>>                 };
>>
>> Where power-on-seq would tell the mmc-core exactly how to bring up the sdio
>> device, using standard prefixes so that the mmc-core knows that something is
>> a clock / gpio / reset / whatever.
> 
> I suppose I already made my point here. I don't think this is not a good idea.

I've heard you loud and clear. As said I already more or less expected this answer.

> 
>>
>> This should pretty much work for everything, and if something comes around which
>> really needs some custom bit of code to bring it up, the mmc-core powerup stuff
>> can simply be opted-out by leaving out the power-on-seq property.
>>
>> Regards,
>>
>> Hans
> 
> Here are some more thoughts of how this should be implemented.
> 
> Synchronization point:
> My idea is to let the mmc_of_parse() function act as the
> synchronization point, to make sure we are able to perform a full
> power up of the card - before we actually start initialization of it.

Ack.

> If the mmc host has a child node to model the card, mmc_of_parse()
> needs to verify that a struct device is created for it and that it's
> corresponding "powerup driver" has been probed successfully.

Right, this would be for the slot child node, which itself can have
card (reg = <0>); and sdio-func childnodes, listing compatible strings
for those (so that the driver loaded through sdio probing can check if
the info in the sdio-func childnode is formatted in a compatible way),
and things like oob irqs, clks not needed for powerup, etc.

> If the mmc_of_parse() returns -EPROBE_DEFER, the mmc host driver will
> return the same error code from it's ->probe(). This provides us with
> the ability of waiting for the "powerup driver" to be probed.

Ack. Note though that mmc_of_parse will likely not do the probe itself,
the way I see it it will do a platform_device_register() and let the
platform bus code do its thing. Downside of this is that
platform_device_register() will not propagate probe errors such as
-EPROBE_DEFER, so we need to check afterwards that a driver is actually
bound, see above.

> If the mmc_of_parse() returns another error code, due to that the
> "powerup driver" failed to be probed, the mmc host driver's ->probe()
> will return the same error code and consequentially no power up of the
> card will be performed at all.

Ack.

> Powerup driver's ->probe():
> Typically the "powerup driver" will need to register a few callback
> functions towards the mmc core. Typically at mmc_of_parse(), those
> callbacks will have to be connected to a particular mmc host.
> 
> I would like to see three different callbacks, mirroring each of the
> mmc_ios power_mode states MMC_POWER_OFF|UP|ON.

Hmm, can't we do something with runtime pm here instead? I would be
nice if we could use the platform bus for this instead of inventing
a new bus for this. Both from not needing to add extra count pov, but
also from the pov of having a solution which other subsystems can
later easily copy. I can even envision the parts of mmc_of_parse
dealing with this being a separate function taking a child node as
argument from day one, which can then hopefully later be moved out
of the mmc subsys and be used elsewhere too (*).

Regards,

Hans


*) I know I'm an optimist, a man can dream can he not ?

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Arend van Spriel]

On 05/28/14 11:42, Hans de Goede wrote:
> Hi,
>
> On 05/27/2014 03:50 PM, Ulf Hansson wrote:
>> [snip]
>>
>>>> I am having a bit hard to follow the terminology here. :-) What is a
>>>> "powerup driver" and what is a "main device driver" in this context?
>>>>
>>>> I had a slide which I used at a mmc subsystem crash course recently,
>>>> please have a look - hopefully this will help us to sort out this.
>>>>
>>>> https://drive.google.com/file/d/0B2ePGK-iqMupbDQ2S0o5b3Bhek0/edit?usp=sharing
>>>
>>> Right, the problem is that in the case(s) we're talking about before the sdio
>>> device in question can be probed the sdio device may need to have several clk
>>> signals enables, reset signal deasserted, etc.
>>
>> Yes. That's an important point.
>>
>>>
>>> Some piece of code needs to do that, the proposal so far has been to let the
>>> mmc-core deal with this, which will likely work fine for 90% of the cases
>>> were we need any form of "powerup", but in some more complex case this may need
>>> to happen in a specific order / with specific delays, etc. In this case some
>>> separate piece of board and/or sdio device specific code would need to take
>>> care of this, hence I was talking about a "powerup-driver", which I agree is
>>> not the best name. So lets just forget about the power-driver nomenclature
>>> completely.
>>
>> I have now given this a serious thought, sorry for not doing this
>> earlier. Please consider the following ideas and statements. Feel free
>> to shoot them down. :-)
>>
>> DT is supposed to described the hardware. Encoding power up sequences
>> within DT, to for example let the mmc core handle this means we are
>> touching the concept of open firmware. This is not what DT should be
>> used for.
>
> Ok, I already expected an answer like this, but I still wanted to voice
> the idea of encoding the sequence into the dt.
>
>> To describe the HW in DT, the embedded SDIO card (actually it could be
>> any type of embedded card) shall be modelled as a child node to the
>> mmc host in DT. Similar to what you have proposed, but with the
>> difference that the child node _must_ contain a DT compatible string,
>> which means a "powerup-driver" can be probed.
>>
>> Yes, I understand we might need one DT compatible string per board,
>> but that's because we need to model the hardware - and it differs.
>>
>> To clarify my view, we do need a "powerup-driver" and the primary
>> reason is that we must not model "power up sequences" within DT.
>> Typically I see the "powerup-driver" as a simple platform driver
>> attached to the platform bus, but I that could of course differ.
>
> Ok, but if it is a platform device, then how can the mmc-host
> depend on it ? Or will the mmc-core code instantiate this
> platform device based on the child node, rather then it being
> instantiated directly by the platform bus ?
>
> <snip>
>
>>>> Well, I don't like the "simple-powerup", because I think a simple
>>>> powerup sequence is to me already supported by the mmc core, through
>>>> the regular host interface (->set_ios()).
>>>>
>>>> If I understand correctly, this binding is supposed to be configured
>>>> per host device and thus also per host device slots?
>>>
>>> Yes, although I must admit that have not thought about how to deal with
>>> slots, I've no experience with the mmc slots concept at all, or is slot
>>> just a different name for sdio function ?
>>
>> Some mmc hosts may support more than one slot. Thus they can operate
>> on more than one card.
>>
>> Currently, there are no support for this in the mmc core. There can
>> only be one card per host, but that's due to software limitation of
>> the mmc stack.  Following your suggestion; modelling the card as child
>> node under the mmc host, can easily be extended to support more than
>> one slot.
>
> Actually what I'm suggesting is based on Sascha Hauer's
> "mmc: Add SDIO function devicetree subnode parsing"
>
> Patch, which models the sdio functions as child nodes, (with the
> one with reg =<0>  being the card itself) this also makes sense since each
> sdio-function gets its own device representing it, so having one child node
> per sdio-functions leads to one child node per device which seems sensible.

To complicate thing, for brcmfmac the sdio functions are not considered 
individual devices. This means that brcmfmac creates one driver instance 
which claims multiple sdio functions.

Regards,
Arend

> This means how ever that if want to prepare for a future were we support
> more then one slot we need an extra level for the slot, so we would
> get something like this:
>
> mmc3: mmc at 01c12000 {
>      #address-cells =<1>;
>      #size-cells =<0>;
>
>      pinctrl-names = "default";
>      pinctrl-0 =<&mmc3_pins_a>;
>      vmmc-supply =<&reg_vcc3v3>;
>      bus-width =<4>;
>      non-removable;
>      status = "okay";
>
>      mmc3slot0: mmc3slot at 0 {
>          #address-cells =<1>;
>          #size-cells =<0>;
>
>          reg =<0>;
>          clocks =<&clk_out_a>,<&clk_out_b>;
>          clock-frequency =<32768>,<26000000>;
> 	gpios =<&pio 4 5 0&pio 1 18 0>;
>
>          brcmf: bcrmf at 1 {
>              reg =<1>;
>              compatible = "brcm,bcm43xx-fmac";
>              interrupt-parent =<&pio>;
>              interrupts =<10 4>; /* PH10 / EINT10 */
>              interrupt-names = "host-wake";
>              status = "okay";
>          };
>      };
> };
>
> Note that I've put the powerup related resources at the slot
> child-node level, I think having the slots as an explicit level
> actually solves a lot of the powerup discussion we've been having.
>
> The slot child node can have the properties for the nodes needed
> to powerup the device attached to the slot.
>
> The slot child node can have a compatible string, independent of the
> sdio-func child node, and this slot child node compatible can then
> be used to select a power-up driver for the slot (which then power
> up the sdio device / sdio functions before probing).
>
>  From a Linux implementation pov, this would mean that the mmc-core
> would instantiate platform-devices for the slot child nodes and then
> let the platform bus take care of loading a driver for this.
> After instantiating the platform device, the mmc-core will check if
> there is a driver bound, and if not return -EPROBEDEFER to deal with
> the power-up driver (module) not being loaded yet, or the powerup
> driver actually hitting -EPROBEDEFER itself. This means we will need
> a small "null" driver to deal with cases where we don't need any
> powerup, but for some reason still need child nodes. Or we could
> skip the check if there is no compatible property in the slot child
> node, but I think the latter option may come back to bite us later.
>
>> The slot will be the first level of child node under the mmc host,
>> then each slot may have a child node which models the embedded card.
>> But, let's leave that discussion for now. :-)
>
> Right, I agree, see above, but I believe we need to settle this now,
> even without solving the powerup sequence thing we (at least I) need
> to get the child node structure + addressing set in stone so that
> we can add oob irq support, which requires storing info in the
> sdio-func child node.
>
>
>>
>>>
>>> Thinking more about this, maybe we can solve the problem of people
>>> worrying about complex power-on scenarios coming around later, by
>>> also encoding the sequence in dt, e.g. something like:
>>>
>>>                  mmc3: mmc at 01c12000 {
>>>                          #address-cells =<1>;
>>>                          #size-cells =<0>;
>>>
>>>                          pinctrl-names = "default";
>>>                          pinctrl-0 =<&mmc3_pins_a>;
>>>                          vmmc-supply =<&reg_vmmc3>;
>>>                          bus-width =<4>;
>>>                          non-removable;
>>>                          status = "okay";
>>>
>>>                          brcmf: bcrmf at 1 {
>>>                                  reg =<1>;
>>>                                  compatible = "brcm,bcm43xx-fmac";
>>>                                  interrupt-parent =<&pio>;
>>>                                  interrupts =<10 8>; /* PH10 / EINT10 */
>>>                                  interrupt-names = "host-wake";
>>>                                  clocks =<&clk_out_a>;
>>>                                  clock-names = "clk_32khz";
>>>                                  gpios =<&pio 7 24 0>;
>>>                                  gpio-names = "gpio_reg_enable";
>>>                                  power-on-seq = "gpio_reg_enable", "usleep 1000", "clk_32khz", "usleep 200";
>>>                                  status = "okay";
>>>                          };
>>>                  };
>>>
>>> Where power-on-seq would tell the mmc-core exactly how to bring up the sdio
>>> device, using standard prefixes so that the mmc-core knows that something is
>>> a clock / gpio / reset / whatever.
>>
>> I suppose I already made my point here. I don't think this is not a good idea.
>
> I've heard you loud and clear. As said I already more or less expected this answer.
>
>>
>>>
>>> This should pretty much work for everything, and if something comes around which
>>> really needs some custom bit of code to bring it up, the mmc-core powerup stuff
>>> can simply be opted-out by leaving out the power-on-seq property.
>>>
>>> Regards,
>>>
>>> Hans
>>
>> Here are some more thoughts of how this should be implemented.
>>
>> Synchronization point:
>> My idea is to let the mmc_of_parse() function act as the
>> synchronization point, to make sure we are able to perform a full
>> power up of the card - before we actually start initialization of it.
>
> Ack.
>
>> If the mmc host has a child node to model the card, mmc_of_parse()
>> needs to verify that a struct device is created for it and that it's
>> corresponding "powerup driver" has been probed successfully.
>
> Right, this would be for the slot child node, which itself can have
> card (reg =<0>); and sdio-func childnodes, listing compatible strings
> for those (so that the driver loaded through sdio probing can check if
> the info in the sdio-func childnode is formatted in a compatible way),
> and things like oob irqs, clks not needed for powerup, etc.
>
>> If the mmc_of_parse() returns -EPROBE_DEFER, the mmc host driver will
>> return the same error code from it's ->probe(). This provides us with
>> the ability of waiting for the "powerup driver" to be probed.
>
> Ack. Note though that mmc_of_parse will likely not do the probe itself,
> the way I see it it will do a platform_device_register() and let the
> platform bus code do its thing. Downside of this is that
> platform_device_register() will not propagate probe errors such as
> -EPROBE_DEFER, so we need to check afterwards that a driver is actually
> bound, see above.
>
>> If the mmc_of_parse() returns another error code, due to that the
>> "powerup driver" failed to be probed, the mmc host driver's ->probe()
>> will return the same error code and consequentially no power up of the
>> card will be performed at all.
>
> Ack.
>
>> Powerup driver's ->probe():
>> Typically the "powerup driver" will need to register a few callback
>> functions towards the mmc core. Typically at mmc_of_parse(), those
>> callbacks will have to be connected to a particular mmc host.
>>
>> I would like to see three different callbacks, mirroring each of the
>> mmc_ios power_mode states MMC_POWER_OFF|UP|ON.
>
> Hmm, can't we do something with runtime pm here instead? I would be
> nice if we could use the platform bus for this instead of inventing
> a new bus for this. Both from not needing to add extra count pov, but
> also from the pov of having a solution which other subsystems can
> later easily copy. I can even envision the parts of mmc_of_parse
> dealing with this being a separate function taking a child node as
> argument from day one, which can then hopefully later be moved out
> of the mmc subsys and be used elsewhere too (*).
>
> Regards,
>
> Hans
>
>
> *) I know I'm an optimist, a man can dream can he not ?

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/28/2014 12:12 PM, Arend van Spriel wrote:

<snip>

>>>> Yes, although I must admit that have not thought about how to deal with
>>>> slots, I've no experience with the mmc slots concept at all, or is slot
>>>> just a different name for sdio function ?
>>>
>>> Some mmc hosts may support more than one slot. Thus they can operate
>>> on more than one card.
>>>
>>> Currently, there are no support for this in the mmc core. There can
>>> only be one card per host, but that's due to software limitation of
>>> the mmc stack.  Following your suggestion; modelling the card as child
>>> node under the mmc host, can easily be extended to support more than
>>> one slot.
>>
>> Actually what I'm suggesting is based on Sascha Hauer's
>> "mmc: Add SDIO function devicetree subnode parsing"
>>
>> Patch, which models the sdio functions as child nodes, (with the
>> one with reg =<0>  being the card itself) this also makes sense since each
>> sdio-function gets its own device representing it, so having one child node
>> per sdio-functions leads to one child node per device which seems sensible.
> 
> To complicate thing, for brcmfmac the sdio functions are not considered individual devices. This means that brcmfmac creates one driver instance which claims multiple sdio functions.

Right, but that is not really important for the overall device tree model
we can just put all the info brcmfmac needs in the child-node for sdio-func 1,
and then the driver can get to it since it claims both functions anyways,
that is actually what the oob irq support patchset I posted a few days ago
does. But that patch-set is missing the extra slot level in the child-nodes,
so if we agree we want that extra level in the hierarchy I'll need to fix
that for v2 of the set.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Mark Brown]

On Wed, May 28, 2014 at 10:19:11AM +0200, Ulf Hansson wrote:
> On 27 May 2014 19:53, Mark Brown <broonie@kernel.org> wrote:

> > This then either conflicts with cases where we need to describe the
> > actual contents of the slot with a compatible string or means that the
> > SDIO driver needs to handle powerup sequencing since we should be
> > binding to the first compatible we find.  If the host controller driver
> > and/or subsystem is going to deal with the powering up it's not clear
> > that it specifically needs to be the compatible property that's used
> > to determine the powerup method, it could just be a boolean or a
> > 'power-method = blah' property (where blah is one of a series of strings
> > defining methods).  Alternatively we could have separate nodes for the
> > slot and SDIO device but that feels meh.  What's the hard requirement
> > for it to specifically be a compatible property?

> Since the sdio bus is a discoverable bus, we must leave the "SDIO card
> device id" outside the information of the DT child node. Instead the
> "SDIO card device id" will be fetched once the card has been properly
> powered and initialized. From sdio bus' ->match(), the "SDIO func
> driver id" will be matched with the "SDIO card device id".

> What we need to describe in the child node is only what "powerup
> driver" to use (an obviously it's resources). I don't see why there
> should be a conflict in using a compatible property, but I might be
> missing your point.

I wouldn't assume that the SDIO bus will always be accurately
discoverable - hardware engineers can always repurpose things for other
purposes.

> > There's also the need for the SDIO device to be able to get at the
> > resources provided and actively work with them at runtime if it wants to
> > manage things more actively (partial poweroff for low power states or
> > managing clock rates for example).

> Yes, that's is also a reason to why I thought it would make sense to
> have a "powerup driver". Typically this driver should support runtime
> PM to handle these kind of fine grained power management.

No, runtime PM isn't really fine grained - I'm talking about things
like starting and stopping individual resources or configuring them.

> The SDIO func driver will have access (through the struct
> mmc_card|host) to the "powerup driver's" struct device, and can use
> pm_runtime_get|put() for it, to tell the "powerup driver" when it may
> go into power save state.

> This kind of fine grained power management is not related directly to
> the SDIO specification, and should not be a part of the mmc core in my
> opinion.

> If we don't have a compatible string in the DT child node of the mmc
> host, but just a DT property - is there a way to achieve the above
> anyway?

I don't see any relationship between having compatible strings and the
ability to achieve things like that to be honest.  Having compatible
strings has no real impact on what we can implement, the power driver
you're talking about can easily be library code or a feature of part of
the code that already exists - so long as we know what hardware is there
it's up to us how we interpret that.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140528/580648d8/attachment.sig>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Tomasz Figa]

I'm following this discussion continuously, but (un)fortunately I'm on
vacation right now and don't have much time to work on this, so sorry
for a very selective reply.

On 28.05.2014 11:42, Hans de Goede wrote:
> Hi,
> 
> On 05/27/2014 03:50 PM, Ulf Hansson wrote:
>> [snip]
>>
>> Powerup driver's ->probe():
>> Typically the "powerup driver" will need to register a few callback
>> functions towards the mmc core. Typically at mmc_of_parse(), those
>> callbacks will have to be connected to a particular mmc host.
>>
>> I would like to see three different callbacks, mirroring each of the
>> mmc_ios power_mode states MMC_POWER_OFF|UP|ON.
> 
> Hmm, can't we do something with runtime pm here instead? I would be
> nice if we could use the platform bus for this instead of inventing
> a new bus for this. Both from not needing to add extra count pov, but
> also from the pov of having a solution which other subsystems can
> later easily copy. I can even envision the parts of mmc_of_parse
> dealing with this being a separate function taking a child node as
> argument from day one, which can then hopefully later be moved out
> of the mmc subsys and be used elsewhere too (*).

This is actually a real use case, because we already have HSIC (USB with
heavily stripped down physical layer) modems on certain Samsung boards
and similarly to the kind of SDIO devices being discussed here they
can't be discovered until a certain power up sequence is done.
Analogically, the modem chip uses OOB interrupts and certain GPIO lines
for various purposes and they need to be provided by DT.

Moreover, there are already WLAN chips available that can use HSIC as
their host interface and I'm not talking here about some exotic
products, but rather widely recognized products of Broadcom (BCM4335),
Marvell (88W8797) or Qualcomm Atheros (AR6004).

My conclusion is that I see the discussion here being too much focused
on MMC, especially considering the fact that the whole problem doesn't
have anything to do with MMC, which is just used as (one of possible)
host interface.

IMHO, all we need here is a way to tell the MMC (or HSIC) core when to
look for a new device and when not (e.g. power down the host controller
completely). Anything else, including proper power sequencing is up to
the platform driver of such non-discoverable device - it's only its host
interface that is discoverable when enabled. I believe this simplistic
approach would lead to much less new code added, better reusability of
code (power sequencing independent of host interface) and no need to
create overly generic code, which usually turns out to be not generic
enough.

Best regards,
Tomasz

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Mark Brown]

On Wed, May 28, 2014 at 01:47:50PM +0200, Tomasz Figa wrote:

> Moreover, there are already WLAN chips available that can use HSIC as
> their host interface and I'm not talking here about some exotic
> products, but rather widely recognized products of Broadcom (BCM4335),
> Marvell (88W8797) or Qualcomm Atheros (AR6004).

> My conclusion is that I see the discussion here being too much focused
> on MMC, especially considering the fact that the whole problem doesn't
> have anything to do with MMC, which is just used as (one of possible)
> host interface.

Right, the other example I keep mentioning for this is Slimbus where
some sort of external power control is required to trigger enumeration
in normal operation for common use cases.

> IMHO, all we need here is a way to tell the MMC (or HSIC) core when to
> look for a new device and when not (e.g. power down the host controller
> completely). Anything else, including proper power sequencing is up to
> the platform driver of such non-discoverable device - it's only its host
> interface that is discoverable when enabled. I believe this simplistic
> approach would lead to much less new code added, better reusability of
> code (power sequencing independent of host interface) and no need to
> create overly generic code, which usually turns out to be not generic
> enough.

We then have the problem of working out where that platform driver comes
from, for many of these buses the device can be completely described as
just a device on the parent bus with some resources connected.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140528/ec9297a2/attachment.sig>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 05/28/2014 06:43 PM, Mark Brown wrote:
> On Wed, May 28, 2014 at 01:47:50PM +0200, Tomasz Figa wrote:

<snip>

>> IMHO, all we need here is a way to tell the MMC (or HSIC) core when to
>> look for a new device and when not (e.g. power down the host controller
>> completely). Anything else, including proper power sequencing is up to
>> the platform driver of such non-discoverable device - it's only its host
>> interface that is discoverable when enabled. I believe this simplistic
>> approach would lead to much less new code added, better reusability of
>> code (power sequencing independent of host interface) and no need to
>> create overly generic code, which usually turns out to be not generic
>> enough.
> 
> We then have the problem of working out where that platform driver comes
> from, for many of these buses the device can be completely described as
> just a device on the parent bus with some resources connected.

Right, I think what we should do is focus on the DT description for this
first and then worry about the implementation later.

I believe that at the DT level this all should be represented as a child-node
under the host controller. Following that reasoning it makes perfect sense
to just focus on mmc for now, while keeping in mind that it would be nice if
what comes out of this will be re-usable.

I've done a detailed, updated proposal for the mmc case (with admittedly
some implementation comments in there) in my last mail, but unfortunately
I've seen little response to that. Can people please reply to my
proposal where we've 2 levels of child nodes, one per mmc slot, and then the
slot nodes has card / sdio-func child nodes, see my last mail.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Ulf Hansson]

On 28 May 2014 11:42, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 05/27/2014 03:50 PM, Ulf Hansson wrote:
>> [snip]
>>
>>>> I am having a bit hard to follow the terminology here. :-) What is a
>>>> "powerup driver" and what is a "main device driver" in this context?
>>>>
>>>> I had a slide which I used at a mmc subsystem crash course recently,
>>>> please have a look - hopefully this will help us to sort out this.
>>>>
>>>> https://drive.google.com/file/d/0B2ePGK-iqMupbDQ2S0o5b3Bhek0/edit?usp=sharing
>>>
>>> Right, the problem is that in the case(s) we're talking about before the sdio
>>> device in question can be probed the sdio device may need to have several clk
>>> signals enables, reset signal deasserted, etc.
>>
>> Yes. That's an important point.
>>
>>>
>>> Some piece of code needs to do that, the proposal so far has been to let the
>>> mmc-core deal with this, which will likely work fine for 90% of the cases
>>> were we need any form of "powerup", but in some more complex case this may need
>>> to happen in a specific order / with specific delays, etc. In this case some
>>> separate piece of board and/or sdio device specific code would need to take
>>> care of this, hence I was talking about a "powerup-driver", which I agree is
>>> not the best name. So lets just forget about the power-driver nomenclature
>>> completely.
>>
>> I have now given this a serious thought, sorry for not doing this
>> earlier. Please consider the following ideas and statements. Feel free
>> to shoot them down. :-)
>>
>> DT is supposed to described the hardware. Encoding power up sequences
>> within DT, to for example let the mmc core handle this means we are
>> touching the concept of open firmware. This is not what DT should be
>> used for.
>
> Ok, I already expected an answer like this, but I still wanted to voice
> the idea of encoding the sequence into the dt.
>
>> To describe the HW in DT, the embedded SDIO card (actually it could be
>> any type of embedded card) shall be modelled as a child node to the
>> mmc host in DT. Similar to what you have proposed, but with the
>> difference that the child node _must_ contain a DT compatible string,
>> which means a "powerup-driver" can be probed.
>>
>> Yes, I understand we might need one DT compatible string per board,
>> but that's because we need to model the hardware - and it differs.
>>
>> To clarify my view, we do need a "powerup-driver" and the primary
>> reason is that we must not model "power up sequences" within DT.
>> Typically I see the "powerup-driver" as a simple platform driver
>> attached to the platform bus, but I that could of course differ.
>
> Ok, but if it is a platform device, then how can the mmc-host
> depend on it ? Or will the mmc-core code instantiate this
> platform device based on the child node, rather then it being
> instantiated directly by the platform bus ?
>
> <snip>
>
>>>> Well, I don't like the "simple-powerup", because I think a simple
>>>> powerup sequence is to me already supported by the mmc core, through
>>>> the regular host interface (->set_ios()).
>>>>
>>>> If I understand correctly, this binding is supposed to be configured
>>>> per host device and thus also per host device slots?
>>>
>>> Yes, although I must admit that have not thought about how to deal with
>>> slots, I've no experience with the mmc slots concept at all, or is slot
>>> just a different name for sdio function ?
>>
>> Some mmc hosts may support more than one slot. Thus they can operate
>> on more than one card.
>>
>> Currently, there are no support for this in the mmc core. There can
>> only be one card per host, but that's due to software limitation of
>> the mmc stack.  Following your suggestion; modelling the card as child
>> node under the mmc host, can easily be extended to support more than
>> one slot.
>
> Actually what I'm suggesting is based on Sascha Hauer's
> "mmc: Add SDIO function devicetree subnode parsing"
>
> Patch, which models the sdio functions as child nodes, (with the
> one with reg = <0> being the card itself) this also makes sense since each
> sdio-function gets its own device representing it, so having one child node
> per sdio-functions leads to one child node per device which seems sensible.
>
> This means how ever that if want to prepare for a future were we support
> more then one slot we need an extra level for the slot, so we would
> get something like this:
>
> mmc3: mmc at 01c12000 {
>     #address-cells = <1>;
>     #size-cells = <0>;
>
>     pinctrl-names = "default";
>     pinctrl-0 = <&mmc3_pins_a>;
>     vmmc-supply = <&reg_vcc3v3>;
>     bus-width = <4>;
>     non-removable;
>     status = "okay";
>
>     mmc3slot0: mmc3slot at 0 {
>         #address-cells = <1>;
>         #size-cells = <0>;
>
>         reg = <0>;
>         clocks = <&clk_out_a>, <&clk_out_b>;
>         clock-frequency = <32768>, <26000000>;
>         gpios = <&pio 4 5 0 &pio 1 18 0>;
>
>         brcmf: bcrmf at 1 {
>             reg = <1>;
>             compatible = "brcm,bcm43xx-fmac";
>             interrupt-parent = <&pio>;
>             interrupts = <10 4>; /* PH10 / EINT10 */
>             interrupt-names = "host-wake";
>             status = "okay";
>         };
>     };
> };
>
> Note that I've put the powerup related resources at the slot
> child-node level, I think having the slots as an explicit level
> actually solves a lot of the powerup discussion we've been having.
>
> The slot child node can have the properties for the nodes needed
> to powerup the device attached to the slot.
>
> The slot child node can have a compatible string, independent of the
> sdio-func child node, and this slot child node compatible can then
> be used to select a power-up driver for the slot (which then power
> up the sdio device / sdio functions before probing).
>
> From a Linux implementation pov, this would mean that the mmc-core
> would instantiate platform-devices for the slot child nodes and then
> let the platform bus take care of loading a driver for this.
> After instantiating the platform device, the mmc-core will check if
> there is a driver bound, and if not return -EPROBEDEFER to deal with
> the power-up driver (module) not being loaded yet, or the powerup
> driver actually hitting -EPROBEDEFER itself. This means we will need
> a small "null" driver to deal with cases where we don't need any
> powerup, but for some reason still need child nodes. Or we could
> skip the check if there is no compatible property in the slot child
> node, but I think the latter option may come back to bite us later.
>
>> The slot will be the first level of child node under the mmc host,
>> then each slot may have a child node which models the embedded card.
>> But, let's leave that discussion for now. :-)
>
> Right, I agree, see above, but I believe we need to settle this now,
> even without solving the powerup sequence thing we (at least I) need
> to get the child node structure + addressing set in stone so that
> we can add oob irq support, which requires storing info in the
> sdio-func child node.
>
>
>>
>>>
>>> Thinking more about this, maybe we can solve the problem of people
>>> worrying about complex power-on scenarios coming around later, by
>>> also encoding the sequence in dt, e.g. something like:
>>>
>>>                 mmc3: mmc at 01c12000 {
>>>                         #address-cells = <1>;
>>>                         #size-cells = <0>;
>>>
>>>                         pinctrl-names = "default";
>>>                         pinctrl-0 = <&mmc3_pins_a>;
>>>                         vmmc-supply = <&reg_vmmc3>;
>>>                         bus-width = <4>;
>>>                         non-removable;
>>>                         status = "okay";
>>>
>>>                         brcmf: bcrmf at 1 {
>>>                                 reg = <1>;
>>>                                 compatible = "brcm,bcm43xx-fmac";
>>>                                 interrupt-parent = <&pio>;
>>>                                 interrupts = <10 8>; /* PH10 / EINT10 */
>>>                                 interrupt-names = "host-wake";
>>>                                 clocks = <&clk_out_a>;
>>>                                 clock-names = "clk_32khz";
>>>                                 gpios = <&pio 7 24 0>;
>>>                                 gpio-names = "gpio_reg_enable";
>>>                                 power-on-seq = "gpio_reg_enable", "usleep 1000", "clk_32khz", "usleep 200";
>>>                                 status = "okay";
>>>                         };
>>>                 };
>>>
>>> Where power-on-seq would tell the mmc-core exactly how to bring up the sdio
>>> device, using standard prefixes so that the mmc-core knows that something is
>>> a clock / gpio / reset / whatever.
>>
>> I suppose I already made my point here. I don't think this is not a good idea.
>
> I've heard you loud and clear. As said I already more or less expected this answer.
>
>>
>>>
>>> This should pretty much work for everything, and if something comes around which
>>> really needs some custom bit of code to bring it up, the mmc-core powerup stuff
>>> can simply be opted-out by leaving out the power-on-seq property.
>>>
>>> Regards,
>>>
>>> Hans
>>
>> Here are some more thoughts of how this should be implemented.
>>
>> Synchronization point:
>> My idea is to let the mmc_of_parse() function act as the
>> synchronization point, to make sure we are able to perform a full
>> power up of the card - before we actually start initialization of it.
>
> Ack.
>
>> If the mmc host has a child node to model the card, mmc_of_parse()
>> needs to verify that a struct device is created for it and that it's
>> corresponding "powerup driver" has been probed successfully.
>
> Right, this would be for the slot child node, which itself can have
> card (reg = <0>); and sdio-func childnodes, listing compatible strings
> for those (so that the driver loaded through sdio probing can check if
> the info in the sdio-func childnode is formatted in a compatible way),
> and things like oob irqs, clks not needed for powerup, etc.
>
>> If the mmc_of_parse() returns -EPROBE_DEFER, the mmc host driver will
>> return the same error code from it's ->probe(). This provides us with
>> the ability of waiting for the "powerup driver" to be probed.
>
> Ack. Note though that mmc_of_parse will likely not do the probe itself,
> the way I see it it will do a platform_device_register() and let the
> platform bus code do its thing. Downside of this is that
> platform_device_register() will not propagate probe errors such as
> -EPROBE_DEFER, so we need to check afterwards that a driver is actually
> bound, see above.

Just to confirm your ideas, this is how I see the instantiation of the
device and probe of the "powerup driver" as well.

>
>> If the mmc_of_parse() returns another error code, due to that the
>> "powerup driver" failed to be probed, the mmc host driver's ->probe()
>> will return the same error code and consequentially no power up of the
>> card will be performed at all.
>
> Ack.
>
>> Powerup driver's ->probe():
>> Typically the "powerup driver" will need to register a few callback
>> functions towards the mmc core. Typically at mmc_of_parse(), those
>> callbacks will have to be connected to a particular mmc host.
>>
>> I would like to see three different callbacks, mirroring each of the
>> mmc_ios power_mode states MMC_POWER_OFF|UP|ON.
>
> Hmm, can't we do something with runtime pm here instead? I would be
> nice if we could use the platform bus for this instead of inventing
> a new bus for this.

We don't need another bus. The driver only have to register some mmc
specific callbacks, that's all I am saying. Of course these parts
can't be re-used for other subsystems, unless we find it useful to
have similar callbacks for all subsystems.

Still, using runtime PM might work.

I see these important things that follow if we decide to use runtime
PM to trigger the power up/off sequence.
1) In cases of !CONFIG_PM_RUNTIME, it means the "powerup driver" once
probed, will keep it's resources enabled forever.
2) If we want to use runtime PM to control fine grained power
management of the "powerup driver", now this can't be done.
3) The "powerup driver" must be able to cope with two states (on/off),
instead the three MMC_POWER_OFF|UP|ON states.
4) The system suspend/resume sequence for the SDIO card, will be more
tricky to handle.

In principle we need to decide what runtime PM should be used for in
this context.

> Both from not needing to add extra count pov, but
> also from the pov of having a solution which other subsystems can
> later easily copy. I can even envision the parts of mmc_of_parse
> dealing with this being a separate function taking a child node as
> argument from day one, which can then hopefully later be moved out
> of the mmc subsys and be used elsewhere too (*).
>
> Regards,
>
> Hans
>
>
> *) I know I'm an optimist, a man can dream can he not ?

Kind regards
Uffe

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Ulf Hansson]

On 28 May 2014 13:03, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 28, 2014 at 10:19:11AM +0200, Ulf Hansson wrote:
>> On 27 May 2014 19:53, Mark Brown <broonie@kernel.org> wrote:
>
>> > This then either conflicts with cases where we need to describe the
>> > actual contents of the slot with a compatible string or means that the
>> > SDIO driver needs to handle powerup sequencing since we should be
>> > binding to the first compatible we find.  If the host controller driver
>> > and/or subsystem is going to deal with the powering up it's not clear
>> > that it specifically needs to be the compatible property that's used
>> > to determine the powerup method, it could just be a boolean or a
>> > 'power-method = blah' property (where blah is one of a series of strings
>> > defining methods).  Alternatively we could have separate nodes for the
>> > slot and SDIO device but that feels meh.  What's the hard requirement
>> > for it to specifically be a compatible property?
>
>> Since the sdio bus is a discoverable bus, we must leave the "SDIO card
>> device id" outside the information of the DT child node. Instead the
>> "SDIO card device id" will be fetched once the card has been properly
>> powered and initialized. From sdio bus' ->match(), the "SDIO func
>> driver id" will be matched with the "SDIO card device id".
>
>> What we need to describe in the child node is only what "powerup
>> driver" to use (an obviously it's resources). I don't see why there
>> should be a conflict in using a compatible property, but I might be
>> missing your point.
>
> I wouldn't assume that the SDIO bus will always be accurately
> discoverable - hardware engineers can always repurpose things for other
> purposes.

Right! Though I think we should be able to handle most if these issues
through the mmc quirks.

>
>> > There's also the need for the SDIO device to be able to get at the
>> > resources provided and actively work with them at runtime if it wants to
>> > manage things more actively (partial poweroff for low power states or
>> > managing clock rates for example).
>
>> Yes, that's is also a reason to why I thought it would make sense to
>> have a "powerup driver". Typically this driver should support runtime
>> PM to handle these kind of fine grained power management.
>
> No, runtime PM isn't really fine grained - I'm talking about things
> like starting and stopping individual resources or configuring them.

Are you saying that you have several levels of quiescent mode of your
external chip?

>
>> The SDIO func driver will have access (through the struct
>> mmc_card|host) to the "powerup driver's" struct device, and can use
>> pm_runtime_get|put() for it, to tell the "powerup driver" when it may
>> go into power save state.
>
>> This kind of fine grained power management is not related directly to
>> the SDIO specification, and should not be a part of the mmc core in my
>> opinion.
>
>> If we don't have a compatible string in the DT child node of the mmc
>> host, but just a DT property - is there a way to achieve the above
>> anyway?
>
> I don't see any relationship between having compatible strings and the
> ability to achieve things like that to be honest.  Having compatible
> strings has no real impact on what we can implement, the power driver
> you're talking about can easily be library code or a feature of part of
> the code that already exists - so long as we know what hardware is there
> it's up to us how we interpret that.

Agree.

I am a bit puzzled of how such library should look like, but I suppose
we need to be able to go to different power states through it.
Typically the library may be invoked both from the mmc core to do
power up/off and from the sdio func driver to handle power save.

Does it make sense to you as well?

Kind regards
Ulf Hansson

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 06/03/2014 12:14 PM, Ulf Hansson wrote:
> On 28 May 2014 11:42, Hans de Goede <hdegoede@redhat.com> wrote:

<mega snip>

>>> If the mmc_of_parse() returns -EPROBE_DEFER, the mmc host driver will
>>> return the same error code from it's ->probe(). This provides us with
>>> the ability of waiting for the "powerup driver" to be probed.
>>
>> Ack. Note though that mmc_of_parse will likely not do the probe itself,
>> the way I see it it will do a platform_device_register() and let the
>> platform bus code do its thing. Downside of this is that
>> platform_device_register() will not propagate probe errors such as
>> -EPROBE_DEFER, so we need to check afterwards that a driver is actually
>> bound, see above.
> 
> Just to confirm your ideas, this is how I see the instantiation of the
> device and probe of the "powerup driver" as well.

Ok, so given that in another mail thread we've just decided to not use
slot subnodes in the devicetree hierarchy, how are we going to represent
the powerup-bits in devicetree? I suggest that we represent this with
a separate subnode under the mmc host, with its own compatible string.

Since reg == 0 is for the card device, and reg 1-7 is for the sdio function
devices, I suggest that we use reg = <8> for the powerup subnode. Then
the mmc-core can check for such a child subnode, and if it is there
instantiate a platform device for it, and then handle the probe as
described above.

> 
>>
>>> If the mmc_of_parse() returns another error code, due to that the
>>> "powerup driver" failed to be probed, the mmc host driver's ->probe()
>>> will return the same error code and consequentially no power up of the
>>> card will be performed at all.
>>
>> Ack.
>>
>>> Powerup driver's ->probe():
>>> Typically the "powerup driver" will need to register a few callback
>>> functions towards the mmc core. Typically at mmc_of_parse(), those
>>> callbacks will have to be connected to a particular mmc host.
>>>
>>> I would like to see three different callbacks, mirroring each of the
>>> mmc_ios power_mode states MMC_POWER_OFF|UP|ON.
>>
>> Hmm, can't we do something with runtime pm here instead? I would be
>> nice if we could use the platform bus for this instead of inventing
>> a new bus for this.
> 
> We don't need another bus. The driver only have to register some mmc
> specific callbacks, that's all I am saying. Of course these parts
> can't be re-used for other subsystems, unless we find it useful to
> have similar callbacks for all subsystems.
> 
> Still, using runtime PM might work.
> 
> I see these important things that follow if we decide to use runtime
> PM to trigger the power up/off sequence.
> 1) In cases of !CONFIG_PM_RUNTIME, it means the "powerup driver" once
> probed, will keep it's resources enabled forever.

Ack.

> 2) If we want to use runtime PM to control fine grained power
> management of the "powerup driver", now this can't be done.

We can always add something more elaborate later if needed, the advantage
of sticking with a platform-dev represented by its own dt subnode +
runtime PM, is that powerup drivers can be used with other busses too,
all the other busses will need is to specify the subnode location + address
inside the tree, and add code to their subsys core to instantiate the
platform device.

> 3) The "powerup driver" must be able to cope with two states (on/off),
> instead the three MMC_POWER_OFF|UP|ON states.

Since we need to powerup before probing, I think this is fine,
we will want to do the power-up before we do the OFF -> UP -> ON
sequence in mmc_power_up(), and we will want to do the power-down
after transitioning to OFF.

> 4) The system suspend/resume sequence for the SDIO card, will be more
> tricky to handle.

See below.

> In principle we need to decide what runtime PM should be used for in
> this context.

I think we should add a powerup_dev pdev pointer to the mmc-card struct,
so that sdio drivers which want to shutdown the device to save power can
do so (by making the relevant runtime pm calls on the pdev).

The mmc core will never know if it is safe to actually power down the
device again as even if the sdio driver indicates it is ok to shutdown
the mmc-host, it may still need the sdio device to stay powered so as to
not loose state. Or maybe even for system-wakeup through an oob irq.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Ulf Hansson]

On 3 June 2014 13:07, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 06/03/2014 12:14 PM, Ulf Hansson wrote:
>> On 28 May 2014 11:42, Hans de Goede <hdegoede@redhat.com> wrote:
>
> <mega snip>
>
>>>> If the mmc_of_parse() returns -EPROBE_DEFER, the mmc host driver will
>>>> return the same error code from it's ->probe(). This provides us with
>>>> the ability of waiting for the "powerup driver" to be probed.
>>>
>>> Ack. Note though that mmc_of_parse will likely not do the probe itself,
>>> the way I see it it will do a platform_device_register() and let the
>>> platform bus code do its thing. Downside of this is that
>>> platform_device_register() will not propagate probe errors such as
>>> -EPROBE_DEFER, so we need to check afterwards that a driver is actually
>>> bound, see above.
>>
>> Just to confirm your ideas, this is how I see the instantiation of the
>> device and probe of the "powerup driver" as well.
>
> Ok, so given that in another mail thread we've just decided to not use
> slot subnodes in the devicetree hierarchy, how are we going to represent
> the powerup-bits in devicetree? I suggest that we represent this with
> a separate subnode under the mmc host, with its own compatible string.
>
> Since reg == 0 is for the card device, and reg 1-7 is for the sdio function
> devices, I suggest that we use reg = <8> for the powerup subnode. Then
> the mmc-core can check for such a child subnode, and if it is there
> instantiate a platform device for it, and then handle the probe as
> described above.

Why do we need to put the sdio functions devices in DT?

>
>>
>>>
>>>> If the mmc_of_parse() returns another error code, due to that the
>>>> "powerup driver" failed to be probed, the mmc host driver's ->probe()
>>>> will return the same error code and consequentially no power up of the
>>>> card will be performed at all.
>>>
>>> Ack.
>>>
>>>> Powerup driver's ->probe():
>>>> Typically the "powerup driver" will need to register a few callback
>>>> functions towards the mmc core. Typically at mmc_of_parse(), those
>>>> callbacks will have to be connected to a particular mmc host.
>>>>
>>>> I would like to see three different callbacks, mirroring each of the
>>>> mmc_ios power_mode states MMC_POWER_OFF|UP|ON.
>>>
>>> Hmm, can't we do something with runtime pm here instead? I would be
>>> nice if we could use the platform bus for this instead of inventing
>>> a new bus for this.
>>
>> We don't need another bus. The driver only have to register some mmc
>> specific callbacks, that's all I am saying. Of course these parts
>> can't be re-used for other subsystems, unless we find it useful to
>> have similar callbacks for all subsystems.
>>
>> Still, using runtime PM might work.
>>
>> I see these important things that follow if we decide to use runtime
>> PM to trigger the power up/off sequence.
>> 1) In cases of !CONFIG_PM_RUNTIME, it means the "powerup driver" once
>> probed, will keep it's resources enabled forever.
>
> Ack.

So, the consequence is that for CONFIG_PM_SLEEP systems not using
CONFIG_PM_RUNTIME - we don't have a good solution.

Is that acceptable?

>
>> 2) If we want to use runtime PM to control fine grained power
>> management of the "powerup driver", now this can't be done.
>
> We can always add something more elaborate later if needed, the advantage
> of sticking with a platform-dev represented by its own dt subnode +
> runtime PM, is that powerup drivers can be used with other busses too,
> all the other busses will need is to specify the subnode location + address
> inside the tree, and add code to their subsys core to instantiate the
> platform device.
>
>> 3) The "powerup driver" must be able to cope with two states (on/off),
>> instead the three MMC_POWER_OFF|UP|ON states.
>
> Since we need to powerup before probing, I think this is fine,
> we will want to do the power-up before we do the OFF -> UP -> ON
> sequence in mmc_power_up(), and we will want to do the power-down
> after transitioning to OFF.
>
>> 4) The system suspend/resume sequence for the SDIO card, will be more
>> tricky to handle.
>
> See below.
>
>> In principle we need to decide what runtime PM should be used for in
>> this context.
>
> I think we should add a powerup_dev pdev pointer to the mmc-card struct,
> so that sdio drivers which want to shutdown the device to save power can
> do so (by making the relevant runtime pm calls on the pdev).

Makes sense.

>
> The mmc core will never know if it is safe to actually power down the
> device again as even if the sdio driver indicates it is ok to shutdown
> the mmc-host, it may still need the sdio device to stay powered so as to
> not loose state. Or maybe even for system-wakeup through an oob irq.

That should already be handled through the flags MMC_PM_KEEP_POWER and
MMC_PM_WAKE_SDIO_IRQ.

>
> Regards,
>
> Hans

Kind regards
Ulf Hansson

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Hans de Goede]

Hi,

On 06/03/2014 02:58 PM, Ulf Hansson wrote:
> On 3 June 2014 13:07, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 06/03/2014 12:14 PM, Ulf Hansson wrote:
>>> On 28 May 2014 11:42, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> <mega snip>
>>
>>>>> If the mmc_of_parse() returns -EPROBE_DEFER, the mmc host driver will
>>>>> return the same error code from it's ->probe(). This provides us with
>>>>> the ability of waiting for the "powerup driver" to be probed.
>>>>
>>>> Ack. Note though that mmc_of_parse will likely not do the probe itself,
>>>> the way I see it it will do a platform_device_register() and let the
>>>> platform bus code do its thing. Downside of this is that
>>>> platform_device_register() will not propagate probe errors such as
>>>> -EPROBE_DEFER, so we need to check afterwards that a driver is actually
>>>> bound, see above.
>>>
>>> Just to confirm your ideas, this is how I see the instantiation of the
>>> device and probe of the "powerup driver" as well.
>>
>> Ok, so given that in another mail thread we've just decided to not use
>> slot subnodes in the devicetree hierarchy, how are we going to represent
>> the powerup-bits in devicetree? I suggest that we represent this with
>> a separate subnode under the mmc host, with its own compatible string.
>>
>> Since reg == 0 is for the card device, and reg 1-7 is for the sdio function
>> devices, I suggest that we use reg = <8> for the powerup subnode. Then
>> the mmc-core can check for such a child subnode, and if it is there
>> instantiate a platform device for it, and then handle the probe as
>> described above.
> 
> Why do we need to put the sdio functions devices in DT?

To define sdio function specific non probable info, such as oob irqs,
also see the "mmc: Add SDIO function devicetree subnode parsing" patch-set
of which I send v3 this morning.

> 
>>
>>>
>>>>
>>>>> If the mmc_of_parse() returns another error code, due to that the
>>>>> "powerup driver" failed to be probed, the mmc host driver's ->probe()
>>>>> will return the same error code and consequentially no power up of the
>>>>> card will be performed at all.
>>>>
>>>> Ack.
>>>>
>>>>> Powerup driver's ->probe():
>>>>> Typically the "powerup driver" will need to register a few callback
>>>>> functions towards the mmc core. Typically at mmc_of_parse(), those
>>>>> callbacks will have to be connected to a particular mmc host.
>>>>>
>>>>> I would like to see three different callbacks, mirroring each of the
>>>>> mmc_ios power_mode states MMC_POWER_OFF|UP|ON.
>>>>
>>>> Hmm, can't we do something with runtime pm here instead? I would be
>>>> nice if we could use the platform bus for this instead of inventing
>>>> a new bus for this.
>>>
>>> We don't need another bus. The driver only have to register some mmc
>>> specific callbacks, that's all I am saying. Of course these parts
>>> can't be re-used for other subsystems, unless we find it useful to
>>> have similar callbacks for all subsystems.
>>>
>>> Still, using runtime PM might work.
>>>
>>> I see these important things that follow if we decide to use runtime
>>> PM to trigger the power up/off sequence.
>>> 1) In cases of !CONFIG_PM_RUNTIME, it means the "powerup driver" once
>>> probed, will keep it's resources enabled forever.
>>
>> Ack.
> 
> So, the consequence is that for CONFIG_PM_SLEEP systems not using
> CONFIG_PM_RUNTIME - we don't have a good solution.
> 
> Is that acceptable?

IMHO yes, if people want maximum power savings they should use
CONFIG_PM_RUNTIME. And since this is all for yet to be added
systems / configs I would expect CONFIG_PM_RUNTIME to be supported
there just fine.

> 
>>
>>> 2) If we want to use runtime PM to control fine grained power
>>> management of the "powerup driver", now this can't be done.
>>
>> We can always add something more elaborate later if needed, the advantage
>> of sticking with a platform-dev represented by its own dt subnode +
>> runtime PM, is that powerup drivers can be used with other busses too,
>> all the other busses will need is to specify the subnode location + address
>> inside the tree, and add code to their subsys core to instantiate the
>> platform device.
>>
>>> 3) The "powerup driver" must be able to cope with two states (on/off),
>>> instead the three MMC_POWER_OFF|UP|ON states.
>>
>> Since we need to powerup before probing, I think this is fine,
>> we will want to do the power-up before we do the OFF -> UP -> ON
>> sequence in mmc_power_up(), and we will want to do the power-down
>> after transitioning to OFF.
>>
>>> 4) The system suspend/resume sequence for the SDIO card, will be more
>>> tricky to handle.
>>
>> See below.
>>
>>> In principle we need to decide what runtime PM should be used for in
>>> this context.
>>
>> I think we should add a powerup_dev pdev pointer to the mmc-card struct,
>> so that sdio drivers which want to shutdown the device to save power can
>> do so (by making the relevant runtime pm calls on the pdev).
> 
> Makes sense.
> 
>>
>> The mmc core will never know if it is safe to actually power down the
>> device again as even if the sdio driver indicates it is ok to shutdown
>> the mmc-host, it may still need the sdio device to stay powered so as to
>> not loose state. Or maybe even for system-wakeup through an oob irq.
> 
> That should already be handled through the flags MMC_PM_KEEP_POWER and
> MMC_PM_WAKE_SDIO_IRQ.

True. So we could have the sdio core do power down / up on the powerup_dev
on suspend / resume and on host mmc_power_on / off.

Regards,

Hans

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Ulf Hansson]

[snip]

>>
>> Why do we need to put the sdio functions devices in DT?
>
> To define sdio function specific non probable info, such as oob irqs,
> also see the "mmc: Add SDIO function devicetree subnode parsing" patch-set
> of which I send v3 this morning.

Yes, of course - makes sense.

[snip]

>>>> I see these important things that follow if we decide to use runtime
>>>> PM to trigger the power up/off sequence.
>>>> 1) In cases of !CONFIG_PM_RUNTIME, it means the "powerup driver" once
>>>> probed, will keep it's resources enabled forever.
>>>
>>> Ack.
>>
>> So, the consequence is that for CONFIG_PM_SLEEP systems not using
>> CONFIG_PM_RUNTIME - we don't have a good solution.
>>
>> Is that acceptable?
>
> IMHO yes, if people want maximum power savings they should use
> CONFIG_PM_RUNTIME. And since this is all for yet to be added
> systems / configs I would expect CONFIG_PM_RUNTIME to be supported
> there just fine.

That's a valid point, but on the other hand, "old" systems could
benefit from this "feature" as well, were runtime PM might not be that
interesting.

Kind regards
Uffe

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Mark Brown]

On Tue, Jun 03, 2014 at 12:57:52PM +0200, Ulf Hansson wrote:
> On 28 May 2014 13:03, Mark Brown <broonie@kernel.org> wrote:

> > No, runtime PM isn't really fine grained - I'm talking about things
> > like starting and stopping individual resources or configuring them.

> Are you saying that you have several levels of quiescent mode of your
> external chip?

Partly, but also there can be active modes that don't use all
functionality and hence can leave some of the inputs powered down (for
example if a reference clock is used for some feature that isn't used
all the time then that reference clock may be able to be powered down
when the function is not in use).  The device isn't suspended but it can
do without some of the resources it has.

> I am a bit puzzled of how such library should look like, but I suppose
> we need to be able to go to different power states through it.
> Typically the library may be invoked both from the mmc core to do
> power up/off and from the sdio func driver to handle power save.

> Does it make sense to you as well?

Yes, certainly for the power up/down and runtime PM type operations.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140604/e57d3b5b/attachment.sig>

RFC: representing sdio devices oob interrupt, clks, etc. in device tree

[Ulf Hansson]

On 4 June 2014 17:55, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jun 03, 2014 at 12:57:52PM +0200, Ulf Hansson wrote:
>> On 28 May 2014 13:03, Mark Brown <broonie@kernel.org> wrote:
>
>> > No, runtime PM isn't really fine grained - I'm talking about things
>> > like starting and stopping individual resources or configuring them.
>
>> Are you saying that you have several levels of quiescent mode of your
>> external chip?
>
> Partly, but also there can be active modes that don't use all
> functionality and hence can leave some of the inputs powered down (for
> example if a reference clock is used for some feature that isn't used
> all the time then that reference clock may be able to be powered down
> when the function is not in use).  The device isn't suspended but it can
> do without some of the resources it has.
>
>> I am a bit puzzled of how such library should look like, but I suppose
>> we need to be able to go to different power states through it.
>> Typically the library may be invoked both from the mmc core to do
>> power up/off and from the sdio func driver to handle power save.
>
>> Does it make sense to you as well?
>
> Yes, certainly for the power up/down and runtime PM type operations.

For your information; I have started implementing a library to handle
power sequences. I intend to have this separated from mmc, to let
other subsystems re-use the same code.

It will not require a compatible string, but instead use something
like 'power-method = blah' property.

I intend to post this as an RFC in couple of days.

Kind regards
Uffe