Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-12-02 Thread Ulf Hansson
On 2 December 2014 at 02:55, NeilBrown  wrote:
> On Fri, 28 Nov 2014 12:56:33 +0100 Ulf Hansson  wrote:
>
>> On 8 November 2014 at 01:14, NeilBrown  wrote:
>> > If the regulator supplying an SDIO device is shared
>> > with another device, the turning the regulator 'on' and 'off'
>> > will not actually cycle power and so will not reset
>> > the device.
>> >
>> > This is particularly a problem for some wi2si wireless modules which
>> > have a BT module on chip and can share power lines.
>> > Without the power-cycle, subsequent reset commands fail.
>> >
>> > So allow a 'reset' gpio to be specified.  If provided, the
>> > line is asserted during power-up.
>>
>> There have been several attempts to fix similar issues as this patch
>> does. The latest one I posted a few month ago, which wasn't accepted.
>> http://comments.gmane.org/gmane.linux.power-management.general/46635
>
> Thanks for that link.
>
>>
>> There has also been some off-list discussions on especially how we
>> should describe this in DT and there were actually some consensus made
>> around that. Still I haven't seen any patches on the mailing lists.
>
> Wish I could have a link for those off-list discussions :-)
>
> Based on what I read and my own thoughts about other devices that I'm having
> trouble managing I wonder if the right approach might be to admit that these
> buses are *not* 100% discoverable.
>
> i.e. you can discover what is there once it is turned on, but you cannot
> discover how to turn it on.
>
> So the *fix* is to allow attached devices to be explicitly listed.
> In my case I would create a child node of the mmc1 node, which is
> compatible="libertas,wifi" (or whatever the proper name is).
>
> Then when the mmc1 wants to power-up, it does:
>
>device_for_each_child(mmc_dev, NULL, power_up)
>
> where:
>
> static int power_up(struct device *dev, void *data)
> {
>pm_runtime_get_sync(dev);

No. We must not rely on runtime PM to be able to power up the device.

>return 0;
> }
>
> Then I can put my reset-line management in the libertas driver instead of
> trying to include some of it in the mmc driver.

Well, "somewhere" we need to handle the different power up scenarios.
These scenarios should be considered as SOC specific, and if we could
keep that as a separate piece of code, that would be the best.

So, I agree that we shouldn't pollute mmc host drivers with such code,
but I also think SDIO func drivers should be remained untouched.

Instead, my plan is to let the mmc core handle it.

>
> This has the advantage of the devicetree actually describing the hardware
> (there is a libertas wifi SDIO chip attached) rather than the behaviour
> (please turn on this regulator and toggle this GPIO to initialise the device).

I think both ways are viable, since they are both describing the
hardware and the characteristics of it.

>
> I want to do a very similar thing for UARTs (so my GPS and Bluetooth turn on
> when /dev/ttyO? is opened), and I've been thinking about something similar
> for USB - I have a USB attached GSM module, but it also has an Audio link and
> some reset/interrupt lines that need to be configured.
> If I could say to device tree "This USB port has this device attached", I
> think it would be a step in the right direction.
>
>
>
>>
>> So to summarize, I am really concerned that we keep having these power
>> sequence issues for SDIO devices and right now the discussion has been
>> on hold. I am considering to hack on it myself, since I am tired of
>> waiting. :-)
>
> Please Cc me if you do.  Meanwhile I'll try to hack together code supporting
> my latest idea and let you know if I get anywhere.

Sounds good. I have started hacking on it as well, let's see where we end up.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-12-02 Thread Ulf Hansson
On 2 December 2014 at 02:55, NeilBrown ne...@suse.de wrote:
 On Fri, 28 Nov 2014 12:56:33 +0100 Ulf Hansson ulf.hans...@linaro.org wrote:

 On 8 November 2014 at 01:14, NeilBrown ne...@suse.de wrote:
  If the regulator supplying an SDIO device is shared
  with another device, the turning the regulator 'on' and 'off'
  will not actually cycle power and so will not reset
  the device.
 
  This is particularly a problem for some wi2si wireless modules which
  have a BT module on chip and can share power lines.
  Without the power-cycle, subsequent reset commands fail.
 
  So allow a 'reset' gpio to be specified.  If provided, the
  line is asserted during power-up.

 There have been several attempts to fix similar issues as this patch
 does. The latest one I posted a few month ago, which wasn't accepted.
 http://comments.gmane.org/gmane.linux.power-management.general/46635

 Thanks for that link.


 There has also been some off-list discussions on especially how we
 should describe this in DT and there were actually some consensus made
 around that. Still I haven't seen any patches on the mailing lists.

 Wish I could have a link for those off-list discussions :-)

 Based on what I read and my own thoughts about other devices that I'm having
 trouble managing I wonder if the right approach might be to admit that these
 buses are *not* 100% discoverable.

 i.e. you can discover what is there once it is turned on, but you cannot
 discover how to turn it on.

 So the *fix* is to allow attached devices to be explicitly listed.
 In my case I would create a child node of the mmc1 node, which is
 compatible=libertas,wifi (or whatever the proper name is).

 Then when the mmc1 wants to power-up, it does:

device_for_each_child(mmc_dev, NULL, power_up)

 where:

 static int power_up(struct device *dev, void *data)
 {
pm_runtime_get_sync(dev);

No. We must not rely on runtime PM to be able to power up the device.

return 0;
 }

 Then I can put my reset-line management in the libertas driver instead of
 trying to include some of it in the mmc driver.

Well, somewhere we need to handle the different power up scenarios.
These scenarios should be considered as SOC specific, and if we could
keep that as a separate piece of code, that would be the best.

So, I agree that we shouldn't pollute mmc host drivers with such code,
but I also think SDIO func drivers should be remained untouched.

Instead, my plan is to let the mmc core handle it.


 This has the advantage of the devicetree actually describing the hardware
 (there is a libertas wifi SDIO chip attached) rather than the behaviour
 (please turn on this regulator and toggle this GPIO to initialise the device).

I think both ways are viable, since they are both describing the
hardware and the characteristics of it.


 I want to do a very similar thing for UARTs (so my GPS and Bluetooth turn on
 when /dev/ttyO? is opened), and I've been thinking about something similar
 for USB - I have a USB attached GSM module, but it also has an Audio link and
 some reset/interrupt lines that need to be configured.
 If I could say to device tree This USB port has this device attached, I
 think it would be a step in the right direction.




 So to summarize, I am really concerned that we keep having these power
 sequence issues for SDIO devices and right now the discussion has been
 on hold. I am considering to hack on it myself, since I am tired of
 waiting. :-)

 Please Cc me if you do.  Meanwhile I'll try to hack together code supporting
 my latest idea and let you know if I get anywhere.

Sounds good. I have started hacking on it as well, let's see where we end up.

Kind regards
Uffe
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [Gta04-owner] [PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-12-01 Thread Dr. H. Nikolaus Schaller
Hi Neil,

Am 02.12.2014 um 02:55 schrieb NeilBrown :

> On Fri, 28 Nov 2014 12:56:33 +0100 Ulf Hansson  wrote:
> 
>> On 8 November 2014 at 01:14, NeilBrown  wrote:
>>> If the regulator supplying an SDIO device is shared
>>> with another device, the turning the regulator 'on' and 'off'
>>> will not actually cycle power and so will not reset
>>> the device.
>>> 
>>> This is particularly a problem for some wi2si wireless modules which
>>> have a BT module on chip and can share power lines.
>>> Without the power-cycle, subsequent reset commands fail.
>>> 
>>> So allow a 'reset' gpio to be specified.  If provided, the
>>> line is asserted during power-up.
>> 
>> There have been several attempts to fix similar issues as this patch
>> does. The latest one I posted a few month ago, which wasn't accepted.
>> http://comments.gmane.org/gmane.linux.power-management.general/46635
> 
> Thanks for that link.
> 
>> 
>> There has also been some off-list discussions on especially how we
>> should describe this in DT and there were actually some consensus made
>> around that. Still I haven't seen any patches on the mailing lists.
> 
> Wish I could have a link for those off-list discussions :-)
> 
> Based on what I read and my own thoughts about other devices that I'm having
> trouble managing I wonder if the right approach might be to admit that these
> buses are *not* 100% discoverable.
> 
> i.e. you can discover what is there once it is turned on, but you cannot
> discover how to turn it on.

Indeed.

> 
> So the *fix* is to allow attached devices to be explicitly listed.
> In my case I would create a child node of the mmc1 node, which is
> compatible=“libertas,wifi" (or whatever the proper name is).

Sounds like a good idea to me.

> 
> Then when the mmc1 wants to power-up, it does:
> 
>   device_for_each_child(mmc_dev, NULL, power_up)
> 
> where:
> 
> static int power_up(struct device *dev, void *data)
> {
>   pm_runtime_get_sync(dev);
>   return 0;
> }
> 
> Then I can put my reset-line management in the libertas driver instead of
> trying to include some of it in the mmc driver.
> 
> This has the advantage of the devicetree actually describing the hardware
> (there is a libertas wifi SDIO chip attached) rather than the behaviour
> (please turn on this regulator and toggle this GPIO to initialise the device).
> 
> I want to do a very similar thing for UARTs (so my GPS and Bluetooth turn on
> when /dev/ttyO? is opened), and I've been thinking about something similar
> for USB - I have a USB attached GSM module, but it also has an Audio link and
> some reset/interrupt lines that need to be configured.
> If I could say to device tree "This USB port has this device attached", I
> think it would be a step in the right direction.

Thinking a little further, it could either be the core driver of the 
device/bus/protocol
or a special driver that only does power management. Or audio.

And we should consider using a list of strings in the compatible entry so that
several drivers can be loaded if the subsystem structure shows that this is 
simpler.

It could be one for power, one for audio. Or in the case of the libertas a 
separate
power driver for a specific chip that uses the libertas driver so that 
chip
specific reset management is not introduced into the libertas core but separate.

For the usb connected modem the subnode to be attached is likely the PHY
where the self-powered device is connected to.

> 
> 
> 
>> 
>> So to summarize, I am really concerned that we keep having these power
>> sequence issues for SDIO devices and right now the discussion has been
>> on hold. I am considering to hack on it myself, since I am tired of
>> waiting. :-)
> 
> Please Cc me if you do.  Meanwhile I'll try to hack together code supporting
> my latest idea and let you know if I get anywhere.
> 
>> 
>> Regarding this patch, I don't intent to apply it.
> 
> Fair enough, I’m starting to not like it so much anyway.
> 

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-12-01 Thread NeilBrown
On Fri, 28 Nov 2014 12:56:33 +0100 Ulf Hansson  wrote:

> On 8 November 2014 at 01:14, NeilBrown  wrote:
> > If the regulator supplying an SDIO device is shared
> > with another device, the turning the regulator 'on' and 'off'
> > will not actually cycle power and so will not reset
> > the device.
> >
> > This is particularly a problem for some wi2si wireless modules which
> > have a BT module on chip and can share power lines.
> > Without the power-cycle, subsequent reset commands fail.
> >
> > So allow a 'reset' gpio to be specified.  If provided, the
> > line is asserted during power-up.
> 
> There have been several attempts to fix similar issues as this patch
> does. The latest one I posted a few month ago, which wasn't accepted.
> http://comments.gmane.org/gmane.linux.power-management.general/46635

Thanks for that link.

> 
> There has also been some off-list discussions on especially how we
> should describe this in DT and there were actually some consensus made
> around that. Still I haven't seen any patches on the mailing lists.

Wish I could have a link for those off-list discussions :-)

Based on what I read and my own thoughts about other devices that I'm having
trouble managing I wonder if the right approach might be to admit that these
buses are *not* 100% discoverable.

i.e. you can discover what is there once it is turned on, but you cannot
discover how to turn it on.

So the *fix* is to allow attached devices to be explicitly listed.
In my case I would create a child node of the mmc1 node, which is
compatible="libertas,wifi" (or whatever the proper name is).

Then when the mmc1 wants to power-up, it does:

   device_for_each_child(mmc_dev, NULL, power_up)

where:

static int power_up(struct device *dev, void *data)
{
   pm_runtime_get_sync(dev);
   return 0;
}

Then I can put my reset-line management in the libertas driver instead of
trying to include some of it in the mmc driver.

This has the advantage of the devicetree actually describing the hardware
(there is a libertas wifi SDIO chip attached) rather than the behaviour
(please turn on this regulator and toggle this GPIO to initialise the device).

I want to do a very similar thing for UARTs (so my GPS and Bluetooth turn on
when /dev/ttyO? is opened), and I've been thinking about something similar
for USB - I have a USB attached GSM module, but it also has an Audio link and
some reset/interrupt lines that need to be configured.
If I could say to device tree "This USB port has this device attached", I
think it would be a step in the right direction.



> 
> So to summarize, I am really concerned that we keep having these power
> sequence issues for SDIO devices and right now the discussion has been
> on hold. I am considering to hack on it myself, since I am tired of
> waiting. :-)

Please Cc me if you do.  Meanwhile I'll try to hack together code supporting
my latest idea and let you know if I get anywhere.

> 
> Regarding this patch, I don't intent to apply it.

Fair enough, I'm starting to not like it so much anyway.

Thanks,
NeilBrown




pgpZitB8XaIcU.pgp
Description: OpenPGP digital signature


Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-12-01 Thread NeilBrown
On Fri, 28 Nov 2014 12:56:33 +0100 Ulf Hansson ulf.hans...@linaro.org wrote:

 On 8 November 2014 at 01:14, NeilBrown ne...@suse.de wrote:
  If the regulator supplying an SDIO device is shared
  with another device, the turning the regulator 'on' and 'off'
  will not actually cycle power and so will not reset
  the device.
 
  This is particularly a problem for some wi2si wireless modules which
  have a BT module on chip and can share power lines.
  Without the power-cycle, subsequent reset commands fail.
 
  So allow a 'reset' gpio to be specified.  If provided, the
  line is asserted during power-up.
 
 There have been several attempts to fix similar issues as this patch
 does. The latest one I posted a few month ago, which wasn't accepted.
 http://comments.gmane.org/gmane.linux.power-management.general/46635

Thanks for that link.

 
 There has also been some off-list discussions on especially how we
 should describe this in DT and there were actually some consensus made
 around that. Still I haven't seen any patches on the mailing lists.

Wish I could have a link for those off-list discussions :-)

Based on what I read and my own thoughts about other devices that I'm having
trouble managing I wonder if the right approach might be to admit that these
buses are *not* 100% discoverable.

i.e. you can discover what is there once it is turned on, but you cannot
discover how to turn it on.

So the *fix* is to allow attached devices to be explicitly listed.
In my case I would create a child node of the mmc1 node, which is
compatible=libertas,wifi (or whatever the proper name is).

Then when the mmc1 wants to power-up, it does:

   device_for_each_child(mmc_dev, NULL, power_up)

where:

static int power_up(struct device *dev, void *data)
{
   pm_runtime_get_sync(dev);
   return 0;
}

Then I can put my reset-line management in the libertas driver instead of
trying to include some of it in the mmc driver.

This has the advantage of the devicetree actually describing the hardware
(there is a libertas wifi SDIO chip attached) rather than the behaviour
(please turn on this regulator and toggle this GPIO to initialise the device).

I want to do a very similar thing for UARTs (so my GPS and Bluetooth turn on
when /dev/ttyO? is opened), and I've been thinking about something similar
for USB - I have a USB attached GSM module, but it also has an Audio link and
some reset/interrupt lines that need to be configured.
If I could say to device tree This USB port has this device attached, I
think it would be a step in the right direction.



 
 So to summarize, I am really concerned that we keep having these power
 sequence issues for SDIO devices and right now the discussion has been
 on hold. I am considering to hack on it myself, since I am tired of
 waiting. :-)

Please Cc me if you do.  Meanwhile I'll try to hack together code supporting
my latest idea and let you know if I get anywhere.

 
 Regarding this patch, I don't intent to apply it.

Fair enough, I'm starting to not like it so much anyway.

Thanks,
NeilBrown




pgpZitB8XaIcU.pgp
Description: OpenPGP digital signature


Re: [Gta04-owner] [PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-12-01 Thread Dr. H. Nikolaus Schaller
Hi Neil,

Am 02.12.2014 um 02:55 schrieb NeilBrown ne...@suse.de:

 On Fri, 28 Nov 2014 12:56:33 +0100 Ulf Hansson ulf.hans...@linaro.org wrote:
 
 On 8 November 2014 at 01:14, NeilBrown ne...@suse.de wrote:
 If the regulator supplying an SDIO device is shared
 with another device, the turning the regulator 'on' and 'off'
 will not actually cycle power and so will not reset
 the device.
 
 This is particularly a problem for some wi2si wireless modules which
 have a BT module on chip and can share power lines.
 Without the power-cycle, subsequent reset commands fail.
 
 So allow a 'reset' gpio to be specified.  If provided, the
 line is asserted during power-up.
 
 There have been several attempts to fix similar issues as this patch
 does. The latest one I posted a few month ago, which wasn't accepted.
 http://comments.gmane.org/gmane.linux.power-management.general/46635
 
 Thanks for that link.
 
 
 There has also been some off-list discussions on especially how we
 should describe this in DT and there were actually some consensus made
 around that. Still I haven't seen any patches on the mailing lists.
 
 Wish I could have a link for those off-list discussions :-)
 
 Based on what I read and my own thoughts about other devices that I'm having
 trouble managing I wonder if the right approach might be to admit that these
 buses are *not* 100% discoverable.
 
 i.e. you can discover what is there once it is turned on, but you cannot
 discover how to turn it on.

Indeed.

 
 So the *fix* is to allow attached devices to be explicitly listed.
 In my case I would create a child node of the mmc1 node, which is
 compatible=“libertas,wifi (or whatever the proper name is).

Sounds like a good idea to me.

 
 Then when the mmc1 wants to power-up, it does:
 
   device_for_each_child(mmc_dev, NULL, power_up)
 
 where:
 
 static int power_up(struct device *dev, void *data)
 {
   pm_runtime_get_sync(dev);
   return 0;
 }
 
 Then I can put my reset-line management in the libertas driver instead of
 trying to include some of it in the mmc driver.
 
 This has the advantage of the devicetree actually describing the hardware
 (there is a libertas wifi SDIO chip attached) rather than the behaviour
 (please turn on this regulator and toggle this GPIO to initialise the device).
 
 I want to do a very similar thing for UARTs (so my GPS and Bluetooth turn on
 when /dev/ttyO? is opened), and I've been thinking about something similar
 for USB - I have a USB attached GSM module, but it also has an Audio link and
 some reset/interrupt lines that need to be configured.
 If I could say to device tree This USB port has this device attached, I
 think it would be a step in the right direction.

Thinking a little further, it could either be the core driver of the 
device/bus/protocol
or a special driver that only does power management. Or audio.

And we should consider using a list of strings in the compatible entry so that
several drivers can be loaded if the subsystem structure shows that this is 
simpler.

It could be one for power, one for audio. Or in the case of the libertas a 
separate
powerreset driver for a specific chip that uses the libertas driver so that 
chip
specific reset management is not introduced into the libertas core but separate.

For the usb connected modem the subnode to be attached is likely the PHY
where the self-powered device is connected to.

 
 
 
 
 So to summarize, I am really concerned that we keep having these power
 sequence issues for SDIO devices and right now the discussion has been
 on hold. I am considering to hack on it myself, since I am tired of
 waiting. :-)
 
 Please Cc me if you do.  Meanwhile I'll try to hack together code supporting
 my latest idea and let you know if I get anywhere.
 
 
 Regarding this patch, I don't intent to apply it.
 
 Fair enough, I’m starting to not like it so much anyway.
 

BR,
Nikolaus

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-11-28 Thread Mark Brown
On Fri, Nov 28, 2014 at 12:56:33PM +0100, Ulf Hansson wrote:
> On 8 November 2014 at 01:14, NeilBrown  wrote:

> > If the regulator supplying an SDIO device is shared
> > with another device, the turning the regulator 'on' and 'off'
> > will not actually cycle power and so will not reset
> > the device.

> > This is particularly a problem for some wi2si wireless modules which
> > have a BT module on chip and can share power lines.
> > Without the power-cycle, subsequent reset commands fail.

> > So allow a 'reset' gpio to be specified.  If provided, the
> > line is asserted during power-up.

> There have been several attempts to fix similar issues as this patch
> does. The latest one I posted a few month ago, which wasn't accepted.
> http://comments.gmane.org/gmane.linux.power-management.general/46635

Note also that if the issue is about needing to take different actions
depending on if the power actually got pulled you can request
notification from the regulator API when things actually happen.


signature.asc
Description: Digital signature


Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-11-28 Thread Ulf Hansson
On 8 November 2014 at 01:14, NeilBrown  wrote:
> If the regulator supplying an SDIO device is shared
> with another device, the turning the regulator 'on' and 'off'
> will not actually cycle power and so will not reset
> the device.
>
> This is particularly a problem for some wi2si wireless modules which
> have a BT module on chip and can share power lines.
> Without the power-cycle, subsequent reset commands fail.
>
> So allow a 'reset' gpio to be specified.  If provided, the
> line is asserted during power-up.

There have been several attempts to fix similar issues as this patch
does. The latest one I posted a few month ago, which wasn't accepted.
http://comments.gmane.org/gmane.linux.power-management.general/46635

There has also been some off-list discussions on especially how we
should describe this in DT and there were actually some consensus made
around that. Still I haven't seen any patches on the mailing lists.

So to summarize, I am really concerned that we keep having these power
sequence issues for SDIO devices and right now the discussion has been
on hold. I am considering to hack on it myself, since I am tired of
waiting. :-)

Regarding this patch, I don't intent to apply it.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-11-28 Thread Ulf Hansson
On 8 November 2014 at 01:14, NeilBrown ne...@suse.de wrote:
 If the regulator supplying an SDIO device is shared
 with another device, the turning the regulator 'on' and 'off'
 will not actually cycle power and so will not reset
 the device.

 This is particularly a problem for some wi2si wireless modules which
 have a BT module on chip and can share power lines.
 Without the power-cycle, subsequent reset commands fail.

 So allow a 'reset' gpio to be specified.  If provided, the
 line is asserted during power-up.

There have been several attempts to fix similar issues as this patch
does. The latest one I posted a few month ago, which wasn't accepted.
http://comments.gmane.org/gmane.linux.power-management.general/46635

There has also been some off-list discussions on especially how we
should describe this in DT and there were actually some consensus made
around that. Still I haven't seen any patches on the mailing lists.

So to summarize, I am really concerned that we keep having these power
sequence issues for SDIO devices and right now the discussion has been
on hold. I am considering to hack on it myself, since I am tired of
waiting. :-)

Regarding this patch, I don't intent to apply it.

Kind regards
Uffe
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-11-28 Thread Mark Brown
On Fri, Nov 28, 2014 at 12:56:33PM +0100, Ulf Hansson wrote:
 On 8 November 2014 at 01:14, NeilBrown ne...@suse.de wrote:

  If the regulator supplying an SDIO device is shared
  with another device, the turning the regulator 'on' and 'off'
  will not actually cycle power and so will not reset
  the device.

  This is particularly a problem for some wi2si wireless modules which
  have a BT module on chip and can share power lines.
  Without the power-cycle, subsequent reset commands fail.

  So allow a 'reset' gpio to be specified.  If provided, the
  line is asserted during power-up.

 There have been several attempts to fix similar issues as this patch
 does. The latest one I posted a few month ago, which wasn't accepted.
 http://comments.gmane.org/gmane.linux.power-management.general/46635

Note also that if the issue is about needing to take different actions
depending on if the power actually got pulled you can request
notification from the regulator API when things actually happen.


signature.asc
Description: Digital signature


[PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-11-07 Thread NeilBrown
If the regulator supplying an SDIO device is shared
with another device, the turning the regulator 'on' and 'off'
will not actually cycle power and so will not reset
the device.

This is particularly a problem for some wi2si wireless modules which
have a BT module on chip and can share power lines.
Without the power-cycle, subsequent reset commands fail.

So allow a 'reset' gpio to be specified.  If provided, the
line is asserted during power-up.

Signed-off-by: NeilBrown 
---
 Documentation/devicetree/bindings/mmc/mmc.txt |3 +
 drivers/mmc/core/core.c   |3 +
 drivers/mmc/core/host.c   |   12 
 drivers/mmc/core/slot-gpio.c  |   70 +
 include/linux/mmc/slot-gpio.h |4 +
 5 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt 
b/Documentation/devicetree/bindings/mmc/mmc.txt
index 431716e37a39..06b84b3bb3ee 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -21,6 +21,9 @@ Optional properties:
   below for the case, when a GPIO is used for the CD line
 - wp-inverted: when present, polarity on the WP line is inverted. See the note
   below for the case, when a GPIO is used for the WP line
+- reset-gpios: Specify a GPIO to be asserted during power-up. This is
+  useful is power is not actually removed (e.g. due to shared
+  regulator) but a reset is needed before reconfiguration.
 - max-frequency: maximum operating clock frequency
 - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
   this system, even if the controller claims it is.
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index d03a080fb9cd..64572c44f9b5 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1533,6 +1533,8 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
 
mmc_host_clk_hold(host);
 
+   /* Reset during power-off */
+   mmc_gpio_set_rs(host, 1);
host->ios.vdd = fls(ocr) - 1;
if (mmc_host_is_spi(host))
host->ios.chip_select = MMC_CS_HIGH;
@@ -1568,6 +1570,7 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
 * time required to reach a stable voltage.
 */
mmc_delay(10);
+   mmc_gpio_set_rs(host, 0);
 
mmc_host_clk_release(host);
 }
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 95cceae96944..42dbf7a521d4 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -415,6 +415,18 @@ int mmc_of_parse(struct mmc_host *host)
if (explicit_inv_wp ^ gpio_inv_wp)
host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
 
+   gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, );
+   if (gpio == -EPROBE_DEFER) {
+   ret = -EPROBE_DEFER;
+   goto out;
+   }
+   if (gpio_is_valid(gpio)) {
+   ret = mmc_gpio_request_rs(host, gpio,
+ flags & OF_GPIO_ACTIVE_LOW);
+   if (ret < 0)
+   goto out;
+   }
+
if (of_find_property(np, "cap-sd-highspeed", ))
host->caps |= MMC_CAP_SD_HIGHSPEED;
if (of_find_property(np, "cap-mmc-highspeed", ))
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index f3bc51f9aba9..354034a8519f 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -21,10 +21,12 @@
 struct mmc_gpio {
struct gpio_desc *ro_gpio;
struct gpio_desc *cd_gpio;
+   struct gpio_desc *rs_gpio; /* reset line */
bool override_ro_active_level;
bool override_cd_active_level;
irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id);
char *ro_label;
+   char *rs_label;
char cd_label[0];
 };
 
@@ -53,12 +55,14 @@ static int mmc_gpio_alloc(struct mmc_host *host)
 * before device_add(), i.e., between mmc_alloc_host() and
 * mmc_add_host()
 */
-   ctx = devm_kzalloc(>class_dev, sizeof(*ctx) + 2 * len,
+   ctx = devm_kzalloc(>class_dev, sizeof(*ctx) + 3 * len,
   GFP_KERNEL);
if (ctx) {
ctx->ro_label = ctx->cd_label + len;
+   ctx->rs_label = ctx->ro_label + len;
snprintf(ctx->cd_label, len, "%s cd", 
dev_name(host->parent));
snprintf(ctx->ro_label, len, "%s ro", 
dev_name(host->parent));
+   snprintf(ctx->rs_label, len, "%s rs", 
dev_name(host->parent));
host->slot.handler_priv = ctx;
}
}
@@ -98,6 +102,18 @@ int mmc_gpio_get_cd(struct mmc_host *host)
 }
 EXPORT_SYMBOL(mmc_gpio_get_cd);
 
+void mmc_gpio_set_rs(struct mmc_host *host, int state)
+{
+   struct mmc_gpio *ctx = host->slot.handler_priv;
+
+   if (!ctx || 

[PATCH 1/2] mmc: core: allow a reset gpio to be configured.

2014-11-07 Thread NeilBrown
If the regulator supplying an SDIO device is shared
with another device, the turning the regulator 'on' and 'off'
will not actually cycle power and so will not reset
the device.

This is particularly a problem for some wi2si wireless modules which
have a BT module on chip and can share power lines.
Without the power-cycle, subsequent reset commands fail.

So allow a 'reset' gpio to be specified.  If provided, the
line is asserted during power-up.

Signed-off-by: NeilBrown ne...@suse.de
---
 Documentation/devicetree/bindings/mmc/mmc.txt |3 +
 drivers/mmc/core/core.c   |3 +
 drivers/mmc/core/host.c   |   12 
 drivers/mmc/core/slot-gpio.c  |   70 +
 include/linux/mmc/slot-gpio.h |4 +
 5 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt 
b/Documentation/devicetree/bindings/mmc/mmc.txt
index 431716e37a39..06b84b3bb3ee 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -21,6 +21,9 @@ Optional properties:
   below for the case, when a GPIO is used for the CD line
 - wp-inverted: when present, polarity on the WP line is inverted. See the note
   below for the case, when a GPIO is used for the WP line
+- reset-gpios: Specify a GPIO to be asserted during power-up. This is
+  useful is power is not actually removed (e.g. due to shared
+  regulator) but a reset is needed before reconfiguration.
 - max-frequency: maximum operating clock frequency
 - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
   this system, even if the controller claims it is.
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index d03a080fb9cd..64572c44f9b5 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1533,6 +1533,8 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
 
mmc_host_clk_hold(host);
 
+   /* Reset during power-off */
+   mmc_gpio_set_rs(host, 1);
host-ios.vdd = fls(ocr) - 1;
if (mmc_host_is_spi(host))
host-ios.chip_select = MMC_CS_HIGH;
@@ -1568,6 +1570,7 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
 * time required to reach a stable voltage.
 */
mmc_delay(10);
+   mmc_gpio_set_rs(host, 0);
 
mmc_host_clk_release(host);
 }
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 95cceae96944..42dbf7a521d4 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -415,6 +415,18 @@ int mmc_of_parse(struct mmc_host *host)
if (explicit_inv_wp ^ gpio_inv_wp)
host-caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
 
+   gpio = of_get_named_gpio_flags(np, reset-gpios, 0, flags);
+   if (gpio == -EPROBE_DEFER) {
+   ret = -EPROBE_DEFER;
+   goto out;
+   }
+   if (gpio_is_valid(gpio)) {
+   ret = mmc_gpio_request_rs(host, gpio,
+ flags  OF_GPIO_ACTIVE_LOW);
+   if (ret  0)
+   goto out;
+   }
+
if (of_find_property(np, cap-sd-highspeed, len))
host-caps |= MMC_CAP_SD_HIGHSPEED;
if (of_find_property(np, cap-mmc-highspeed, len))
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index f3bc51f9aba9..354034a8519f 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -21,10 +21,12 @@
 struct mmc_gpio {
struct gpio_desc *ro_gpio;
struct gpio_desc *cd_gpio;
+   struct gpio_desc *rs_gpio; /* reset line */
bool override_ro_active_level;
bool override_cd_active_level;
irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id);
char *ro_label;
+   char *rs_label;
char cd_label[0];
 };
 
@@ -53,12 +55,14 @@ static int mmc_gpio_alloc(struct mmc_host *host)
 * before device_add(), i.e., between mmc_alloc_host() and
 * mmc_add_host()
 */
-   ctx = devm_kzalloc(host-class_dev, sizeof(*ctx) + 2 * len,
+   ctx = devm_kzalloc(host-class_dev, sizeof(*ctx) + 3 * len,
   GFP_KERNEL);
if (ctx) {
ctx-ro_label = ctx-cd_label + len;
+   ctx-rs_label = ctx-ro_label + len;
snprintf(ctx-cd_label, len, %s cd, 
dev_name(host-parent));
snprintf(ctx-ro_label, len, %s ro, 
dev_name(host-parent));
+   snprintf(ctx-rs_label, len, %s rs, 
dev_name(host-parent));
host-slot.handler_priv = ctx;
}
}
@@ -98,6 +102,18 @@ int mmc_gpio_get_cd(struct mmc_host *host)
 }
 EXPORT_SYMBOL(mmc_gpio_get_cd);
 
+void mmc_gpio_set_rs(struct mmc_host *host, int state)
+{
+   struct mmc_gpio *ctx = host-slot.handler_priv;
+
+   if (!ctx ||