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:

quoted

On 05/26/2014 12:38 PM, Mark Brown wrote:

quoted

On Sun, May 25, 2014 at 09:20:52PM +0200, Hans de Goede wrote:

quoted

quoted

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.

quoted

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.

quoted

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

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

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.

quoted

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