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

[hdegoede@redhat.com (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