LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines
@ 2014-12-12  8:06 Kishon Vijay Abraham I
  2014-12-15 20:50 ` Nishanth Menon
  0 siblings, 1 reply; 10+ messages in thread
From: Kishon Vijay Abraham I @ 2014-12-12  8:06 UTC (permalink / raw)
  To: linus.walleij, gnurou, linux-gpio, linux-kernel, nsekhar
  Cc: kishon, linux-omap, linux-mmc

The reset values for all the PCF lines are high and hence on shutdown
we should drive all the lines high in order to bring it to the reset state.

This is actually required since pcf doesn't have a reset line and even after
warm reset (by invoking "reboot" in prompt) the pcf lines maintains it's
previous programmed state. This becomes a problem if the boards are designed
to work with the default initial state.

DRA7XX_evm uses PCF8575 and one of the PCF output lines feeds to MMC/SD and
this line should be driven high in order for the MMC/SD to be detected.
This line is modelled as regulator and the hsmmc driver takes care of enabling
and disabling it. In the case of 'reboot', during shutdown path as part of it's
cleanup process the hsmmc driver disables this regulator. This makes MMC boot
not functional.

Fixed it by driving high all the pcf lines.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/gpio/gpio-pcf857x.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index 236708a..00b15b2 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -448,6 +448,14 @@ static int pcf857x_remove(struct i2c_client *client)
 	return status;
 }
 
+static void pcf857x_shutdown(struct i2c_client *client)
+{
+	struct pcf857x *gpio = i2c_get_clientdata(client);
+
+	/* Drive all the I/O lines high */
+	gpio->write(gpio->client, BIT(gpio->chip.ngpio) - 1);
+}
+
 static struct i2c_driver pcf857x_driver = {
 	.driver = {
 		.name	= "pcf857x",
@@ -456,6 +464,7 @@ static struct i2c_driver pcf857x_driver = {
 	},
 	.probe	= pcf857x_probe,
 	.remove	= pcf857x_remove,
+	.shutdown = pcf857x_shutdown,
 	.id_table = pcf857x_id,
 };
 
-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines
  2014-12-12  8:06 [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines Kishon Vijay Abraham I
@ 2014-12-15 20:50 ` Nishanth Menon
  2014-12-18  6:18   ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 10+ messages in thread
From: Nishanth Menon @ 2014-12-15 20:50 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, linus.walleij, gnurou, linux-gpio,
	linux-kernel, nsekhar
  Cc: linux-omap, linux-mmc, Benoit Parrot

On 12/12/2014 02:06 AM, Kishon Vijay Abraham I wrote:
> The reset values for all the PCF lines are high and hence on shutdown
> we should drive all the lines high in order to bring it to the reset state.
> 
> This is actually required since pcf doesn't have a reset line and even after
> warm reset (by invoking "reboot" in prompt) the pcf lines maintains it's
> previous programmed state. This becomes a problem if the boards are designed
> to work with the default initial state.
> 
> DRA7XX_evm uses PCF8575 and one of the PCF output lines feeds to MMC/SD and
> this line should be driven high in order for the MMC/SD to be detected.
> This line is modelled as regulator and the hsmmc driver takes care of enabling
> and disabling it. In the case of 'reboot', during shutdown path as part of it's
> cleanup process the hsmmc driver disables this regulator. This makes MMC boot
> not functional.
> 
> Fixed it by driving high all the pcf lines.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/gpio/gpio-pcf857x.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
> index 236708a..00b15b2 100644
> --- a/drivers/gpio/gpio-pcf857x.c
> +++ b/drivers/gpio/gpio-pcf857x.c
> @@ -448,6 +448,14 @@ static int pcf857x_remove(struct i2c_client *client)
>  	return status;
>  }
>  
> +static void pcf857x_shutdown(struct i2c_client *client)
> +{
> +	struct pcf857x *gpio = i2c_get_clientdata(client);
> +
> +	/* Drive all the I/O lines high */
> +	gpio->write(gpio->client, BIT(gpio->chip.ngpio) - 1);

you might force a contention here - depending on System configuration.
example:
+-------+
|       |
|  U1   |         +------+      +-----------+
|       +--------->      |      |           |
+-------+         |      |      |           |
                  | Switch<-----+    SoC    |
+-------+         |      |      |           |
|       |         |      |      |           |
| U2    <---------+--^---+      +-----------+
|       |            |
|       |            |
+-------+            |
                  +--+--+
                  |     |
                  | PCF |
                  |     |
                  +-----+

At low, SoC pin is connected to U2 as drive. when reset to high, you
now have U1 driving to the same pin that SoC has, potentially
resulting in contention.


Unfortunately, at this level, you do not know what the state of the
system is, blindly forcing a pin level will potentially cause
contention risk depending on pin configuration.


> +}
> +
>  static struct i2c_driver pcf857x_driver = {
>  	.driver = {
>  		.name	= "pcf857x",
> @@ -456,6 +464,7 @@ static struct i2c_driver pcf857x_driver = {
>  	},
>  	.probe	= pcf857x_probe,
>  	.remove	= pcf857x_remove,
> +	.shutdown = pcf857x_shutdown,
>  	.id_table = pcf857x_id,
>  };
>  
> 


-- 
Regards,
Nishanth Menon

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines
  2014-12-15 20:50 ` Nishanth Menon
@ 2014-12-18  6:18   ` Kishon Vijay Abraham I
  2014-12-18 14:11     ` Nishanth Menon
  0 siblings, 1 reply; 10+ messages in thread
From: Kishon Vijay Abraham I @ 2014-12-18  6:18 UTC (permalink / raw)
  To: Nishanth Menon, linus.walleij, gnurou, linux-gpio, linux-kernel, nsekhar
  Cc: linux-omap, linux-mmc, Benoit Parrot



On Tuesday 16 December 2014 02:20 AM, Nishanth Menon wrote:
> On 12/12/2014 02:06 AM, Kishon Vijay Abraham I wrote:
>> The reset values for all the PCF lines are high and hence on shutdown
>> we should drive all the lines high in order to bring it to the reset state.
>>
>> This is actually required since pcf doesn't have a reset line and even after
>> warm reset (by invoking "reboot" in prompt) the pcf lines maintains it's
>> previous programmed state. This becomes a problem if the boards are designed
>> to work with the default initial state.
>>
>> DRA7XX_evm uses PCF8575 and one of the PCF output lines feeds to MMC/SD and
>> this line should be driven high in order for the MMC/SD to be detected.
>> This line is modelled as regulator and the hsmmc driver takes care of enabling
>> and disabling it. In the case of 'reboot', during shutdown path as part of it's
>> cleanup process the hsmmc driver disables this regulator. This makes MMC boot
>> not functional.
>>
>> Fixed it by driving high all the pcf lines.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/gpio/gpio-pcf857x.c |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
>> index 236708a..00b15b2 100644
>> --- a/drivers/gpio/gpio-pcf857x.c
>> +++ b/drivers/gpio/gpio-pcf857x.c
>> @@ -448,6 +448,14 @@ static int pcf857x_remove(struct i2c_client *client)
>>  	return status;
>>  }
>>  
>> +static void pcf857x_shutdown(struct i2c_client *client)
>> +{
>> +	struct pcf857x *gpio = i2c_get_clientdata(client);
>> +
>> +	/* Drive all the I/O lines high */
>> +	gpio->write(gpio->client, BIT(gpio->chip.ngpio) - 1);
> 
> you might force a contention here - depending on System configuration.
> example:
> +-------+
> |       |
> |  U1   |         +------+      +-----------+
> |       +--------->      |      |           |
> +-------+         |      |      |           |
>                   | Switch<-----+    SoC    |
> +-------+         |      |      |           |
> |       |         |      |      |           |
> | U2    <---------+--^---+      +-----------+
> |       |            |
> |       |            |
> +-------+            |
>                   +--+--+
>                   |     |
>                   | PCF |
>                   |     |
>                   +-----+
> 
> At low, SoC pin is connected to U2 as drive. when reset to high, you
> now have U1 driving to the same pin that SoC has, potentially
> resulting in contention.
> 
> 
> Unfortunately, at this level, you do not know what the state of the
> system is, blindly forcing a pin level will potentially cause
> contention risk depending on pin configuration.

Assume we are doing a reset when the system is powered on, irrespective of the
state of the system, we'll be forcing the pin level to the default state.

The same thing happens here so I don't think this would be a problem.

Thanks
Kishon

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines
  2014-12-18  6:18   ` Kishon Vijay Abraham I
@ 2014-12-18 14:11     ` Nishanth Menon
  2015-01-05  6:26       ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 10+ messages in thread
From: Nishanth Menon @ 2014-12-18 14:11 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, linus.walleij, gnurou, linux-gpio,
	linux-kernel, nsekhar
  Cc: linux-omap, linux-mmc, Benoit Parrot

On 12/18/2014 12:18 AM, Kishon Vijay Abraham I wrote:
> 
> 
> On Tuesday 16 December 2014 02:20 AM, Nishanth Menon wrote:
>> On 12/12/2014 02:06 AM, Kishon Vijay Abraham I wrote:
>>> The reset values for all the PCF lines are high and hence on shutdown
>>> we should drive all the lines high in order to bring it to the reset state.
>>>
>>> This is actually required since pcf doesn't have a reset line and even after
>>> warm reset (by invoking "reboot" in prompt) the pcf lines maintains it's
>>> previous programmed state. This becomes a problem if the boards are designed
>>> to work with the default initial state.
>>>
>>> DRA7XX_evm uses PCF8575 and one of the PCF output lines feeds to MMC/SD and
>>> this line should be driven high in order for the MMC/SD to be detected.
>>> This line is modelled as regulator and the hsmmc driver takes care of enabling
>>> and disabling it. In the case of 'reboot', during shutdown path as part of it's
>>> cleanup process the hsmmc driver disables this regulator. This makes MMC boot
>>> not functional.
>>>
>>> Fixed it by driving high all the pcf lines.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  drivers/gpio/gpio-pcf857x.c |    9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
>>> index 236708a..00b15b2 100644
>>> --- a/drivers/gpio/gpio-pcf857x.c
>>> +++ b/drivers/gpio/gpio-pcf857x.c
>>> @@ -448,6 +448,14 @@ static int pcf857x_remove(struct i2c_client *client)
>>>  	return status;
>>>  }
>>>  
>>> +static void pcf857x_shutdown(struct i2c_client *client)
>>> +{
>>> +	struct pcf857x *gpio = i2c_get_clientdata(client);
>>> +
>>> +	/* Drive all the I/O lines high */
>>> +	gpio->write(gpio->client, BIT(gpio->chip.ngpio) - 1);
>>
>> you might force a contention here - depending on System configuration.
>> example:
>> +-------+
>> |       |
>> |  U1   |         +------+      +-----------+
>> |       +--------->      |      |           |
>> +-------+         |      |      |           |
>>                   | Switch<-----+    SoC    |
>> +-------+         |      |      |           |
>> |       |         |      |      |           |
>> | U2    <---------+--^---+      +-----------+
>> |       |            |
>> |       |            |
>> +-------+            |
>>                   +--+--+
>>                   |     |
>>                   | PCF |
>>                   |     |
>>                   +-----+
>>
>> At low, SoC pin is connected to U2 as drive. when reset to high, you
>> now have U1 driving to the same pin that SoC has, potentially
>> resulting in contention.
>>
>>
>> Unfortunately, at this level, you do not know what the state of the
>> system is, blindly forcing a pin level will potentially cause
>> contention risk depending on pin configuration.
> 
> Assume we are doing a reset when the system is powered on, irrespective of the
> state of the system, we'll be forcing the pin level to the default state.

Yes, I dont deny that system will be fine *after* reset sequence is
started or completed. However there is a duration between the pcf
shutdown handler is called and the final reset handler is invoked -
that is the duration when  the contention might cause device behavior.
Essentially ignoring the state various drivers have asked PCF to setup
the pins and doing a hands down configuration may have side effects we
cant properly expect.

> 
> The same thing happens here so I don't think this would be a problem.
> 
> Thanks
> Kishon
> 


-- 
Regards,
Nishanth Menon

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines
  2014-12-18 14:11     ` Nishanth Menon
@ 2015-01-05  6:26       ` Kishon Vijay Abraham I
  2015-01-14 11:58         ` Linus Walleij
  0 siblings, 1 reply; 10+ messages in thread
From: Kishon Vijay Abraham I @ 2015-01-05  6:26 UTC (permalink / raw)
  To: Nishanth Menon, linus.walleij, gnurou, linux-gpio, linux-kernel, nsekhar
  Cc: linux-omap, linux-mmc, Benoit Parrot

Hi,

On Thursday 18 December 2014 07:41 PM, Nishanth Menon wrote:
> On 12/18/2014 12:18 AM, Kishon Vijay Abraham I wrote:
>>
>>
>> On Tuesday 16 December 2014 02:20 AM, Nishanth Menon wrote:
>>> On 12/12/2014 02:06 AM, Kishon Vijay Abraham I wrote:
>>>> The reset values for all the PCF lines are high and hence on shutdown
>>>> we should drive all the lines high in order to bring it to the reset state.
>>>>
>>>> This is actually required since pcf doesn't have a reset line and even after
>>>> warm reset (by invoking "reboot" in prompt) the pcf lines maintains it's
>>>> previous programmed state. This becomes a problem if the boards are designed
>>>> to work with the default initial state.
>>>>
>>>> DRA7XX_evm uses PCF8575 and one of the PCF output lines feeds to MMC/SD and
>>>> this line should be driven high in order for the MMC/SD to be detected.
>>>> This line is modelled as regulator and the hsmmc driver takes care of enabling
>>>> and disabling it. In the case of 'reboot', during shutdown path as part of it's
>>>> cleanup process the hsmmc driver disables this regulator. This makes MMC boot
>>>> not functional.
>>>>
>>>> Fixed it by driving high all the pcf lines.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>>  drivers/gpio/gpio-pcf857x.c |    9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
>>>> index 236708a..00b15b2 100644
>>>> --- a/drivers/gpio/gpio-pcf857x.c
>>>> +++ b/drivers/gpio/gpio-pcf857x.c
>>>> @@ -448,6 +448,14 @@ static int pcf857x_remove(struct i2c_client *client)
>>>>  	return status;
>>>>  }
>>>>  
>>>> +static void pcf857x_shutdown(struct i2c_client *client)
>>>> +{
>>>> +	struct pcf857x *gpio = i2c_get_clientdata(client);
>>>> +
>>>> +	/* Drive all the I/O lines high */
>>>> +	gpio->write(gpio->client, BIT(gpio->chip.ngpio) - 1);
>>>
>>> you might force a contention here - depending on System configuration.
>>> example:
>>> +-------+
>>> |       |
>>> |  U1   |         +------+      +-----------+
>>> |       +--------->      |      |           |
>>> +-------+         |      |      |           |
>>>                   | Switch<-----+    SoC    |
>>> +-------+         |      |      |           |
>>> |       |         |      |      |           |
>>> | U2    <---------+--^---+      +-----------+
>>> |       |            |
>>> |       |            |
>>> +-------+            |
>>>                   +--+--+
>>>                   |     |
>>>                   | PCF |
>>>                   |     |
>>>                   +-----+
>>>
>>> At low, SoC pin is connected to U2 as drive. when reset to high, you
>>> now have U1 driving to the same pin that SoC has, potentially
>>> resulting in contention.
>>>
>>>
>>> Unfortunately, at this level, you do not know what the state of the
>>> system is, blindly forcing a pin level will potentially cause
>>> contention risk depending on pin configuration.
>>
>> Assume we are doing a reset when the system is powered on, irrespective of the
>> state of the system, we'll be forcing the pin level to the default state.
> 
> Yes, I dont deny that system will be fine *after* reset sequence is
> started or completed. However there is a duration between the pcf
> shutdown handler is called and the final reset handler is invoked -
> that is the duration when  the contention might cause device behavior.
> Essentially ignoring the state various drivers have asked PCF to setup
> the pins and doing a hands down configuration may have side effects we
> cant properly expect.

The solution might be to invoke the shutdown handler of the various drivers
using the PCF before the shutdown handler of the PCF driver itself gets
invoked? But I'm not sure how can that be achieved in linux kernel :-(

Thanks
Kishon

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines
  2015-01-05  6:26       ` Kishon Vijay Abraham I
@ 2015-01-14 11:58         ` Linus Walleij
  2015-03-16  8:46           ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2015-01-14 11:58 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Nishanth Menon, Alexandre Courbot, linux-gpio, linux-kernel,
	Sekhar Nori, Linux-OMAP, linux-mmc, Benoit Parrot

On Mon, Jan 5, 2015 at 7:26 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> On Thursday 18 December 2014 07:41 PM, Nishanth Menon wrote:
>> On 12/18/2014 12:18 AM, Kishon Vijay Abraham I wrote:
>>>
>>>
>>> On Tuesday 16 December 2014 02:20 AM, Nishanth Menon wrote:
>>>> On 12/12/2014 02:06 AM, Kishon Vijay Abraham I wrote:
>>>>> The reset values for all the PCF lines are high and hence on shutdown
>>>>> we should drive all the lines high in order to bring it to the reset state.
>>>>>
>>>>> This is actually required since pcf doesn't have a reset line and even after
>>>>> warm reset (by invoking "reboot" in prompt) the pcf lines maintains it's
>>>>> previous programmed state. This becomes a problem if the boards are designed
>>>>> to work with the default initial state.
>>>>>
>>>>> DRA7XX_evm uses PCF8575 and one of the PCF output lines feeds to MMC/SD and
>>>>> this line should be driven high in order for the MMC/SD to be detected.
>>>>> This line is modelled as regulator and the hsmmc driver takes care of enabling
>>>>> and disabling it. In the case of 'reboot', during shutdown path as part of it's
>>>>> cleanup process the hsmmc driver disables this regulator. This makes MMC boot
>>>>> not functional.
>>>>>
>>>>> Fixed it by driving high all the pcf lines.
>>>>>
>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>> ---
>>>>>  drivers/gpio/gpio-pcf857x.c |    9 +++++++++
>>>>>  1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
>>>>> index 236708a..00b15b2 100644
>>>>> --- a/drivers/gpio/gpio-pcf857x.c
>>>>> +++ b/drivers/gpio/gpio-pcf857x.c
>>>>> @@ -448,6 +448,14 @@ static int pcf857x_remove(struct i2c_client *client)
>>>>>    return status;
>>>>>  }
>>>>>
>>>>> +static void pcf857x_shutdown(struct i2c_client *client)
>>>>> +{
>>>>> +  struct pcf857x *gpio = i2c_get_clientdata(client);
>>>>> +
>>>>> +  /* Drive all the I/O lines high */
>>>>> +  gpio->write(gpio->client, BIT(gpio->chip.ngpio) - 1);
>>>>
>>>> you might force a contention here - depending on System configuration.
>>>> example:
>>>> +-------+
>>>> |       |
>>>> |  U1   |         +------+      +-----------+
>>>> |       +--------->      |      |           |
>>>> +-------+         |      |      |           |
>>>>                   | Switch<-----+    SoC    |
>>>> +-------+         |      |      |           |
>>>> |       |         |      |      |           |
>>>> | U2    <---------+--^---+      +-----------+
>>>> |       |            |
>>>> |       |            |
>>>> +-------+            |
>>>>                   +--+--+
>>>>                   |     |
>>>>                   | PCF |
>>>>                   |     |
>>>>                   +-----+
>>>>
>>>> At low, SoC pin is connected to U2 as drive. when reset to high, you
>>>> now have U1 driving to the same pin that SoC has, potentially
>>>> resulting in contention.
>>>>
>>>>
>>>> Unfortunately, at this level, you do not know what the state of the
>>>> system is, blindly forcing a pin level will potentially cause
>>>> contention risk depending on pin configuration.
>>>
>>> Assume we are doing a reset when the system is powered on, irrespective of the
>>> state of the system, we'll be forcing the pin level to the default state.
>>
>> Yes, I dont deny that system will be fine *after* reset sequence is
>> started or completed. However there is a duration between the pcf
>> shutdown handler is called and the final reset handler is invoked -
>> that is the duration when  the contention might cause device behavior.
>> Essentially ignoring the state various drivers have asked PCF to setup
>> the pins and doing a hands down configuration may have side effects we
>> cant properly expect.
>
> The solution might be to invoke the shutdown handler of the various drivers
> using the PCF before the shutdown handler of the PCF driver itself gets
> invoked? But I'm not sure how can that be achieved in linux kernel :-(

#include <linux/reboot.h>

static int foo_reboot_handler(struct notifier_block *this,
                                unsigned long code,
                                void *unused)
{
        pr_crit("do some last minute stuff\n");
        return NOTIFY_OK;
}

static struct notifier_block foo_reboot_notifier = {
        .notifier_call = foo_reboot_handler,
};

register_reboot_notifier(&foo_reboot_notifier);

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines
  2015-01-14 11:58         ` Linus Walleij
@ 2015-03-16  8:46           ` Kishon Vijay Abraham I
  2015-03-18 12:21             ` Linus Walleij
  0 siblings, 1 reply; 10+ messages in thread
From: Kishon Vijay Abraham I @ 2015-03-16  8:46 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Nishanth Menon, Alexandre Courbot, linux-gpio, linux-kernel,
	Sekhar Nori, Linux-OMAP, linux-mmc, Benoit Parrot

Hi,

On Wednesday 14 January 2015 05:28 PM, Linus Walleij wrote:
> On Mon, Jan 5, 2015 at 7:26 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> On Thursday 18 December 2014 07:41 PM, Nishanth Menon wrote:
>>> On 12/18/2014 12:18 AM, Kishon Vijay Abraham I wrote:
>>>>
>>>>
>>>> On Tuesday 16 December 2014 02:20 AM, Nishanth Menon wrote:
>>>>> On 12/12/2014 02:06 AM, Kishon Vijay Abraham I wrote:
>>>>>> The reset values for all the PCF lines are high and hence on shutdown
>>>>>> we should drive all the lines high in order to bring it to the reset state.
>>>>>>
>>>>>> This is actually required since pcf doesn't have a reset line and even after
>>>>>> warm reset (by invoking "reboot" in prompt) the pcf lines maintains it's
>>>>>> previous programmed state. This becomes a problem if the boards are designed
>>>>>> to work with the default initial state.
>>>>>>
>>>>>> DRA7XX_evm uses PCF8575 and one of the PCF output lines feeds to MMC/SD and
>>>>>> this line should be driven high in order for the MMC/SD to be detected.
>>>>>> This line is modelled as regulator and the hsmmc driver takes care of enabling
>>>>>> and disabling it. In the case of 'reboot', during shutdown path as part of it's
>>>>>> cleanup process the hsmmc driver disables this regulator. This makes MMC boot
>>>>>> not functional.
>>>>>>
>>>>>> Fixed it by driving high all the pcf lines.
>>>>>>
>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>> ---
>>>>>>   drivers/gpio/gpio-pcf857x.c |    9 +++++++++
>>>>>>   1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
>>>>>> index 236708a..00b15b2 100644
>>>>>> --- a/drivers/gpio/gpio-pcf857x.c
>>>>>> +++ b/drivers/gpio/gpio-pcf857x.c
>>>>>> @@ -448,6 +448,14 @@ static int pcf857x_remove(struct i2c_client *client)
>>>>>>     return status;
>>>>>>   }
>>>>>>
>>>>>> +static void pcf857x_shutdown(struct i2c_client *client)
>>>>>> +{
>>>>>> +  struct pcf857x *gpio = i2c_get_clientdata(client);
>>>>>> +
>>>>>> +  /* Drive all the I/O lines high */
>>>>>> +  gpio->write(gpio->client, BIT(gpio->chip.ngpio) - 1);
>>>>>
>>>>> you might force a contention here - depending on System configuration.
>>>>> example:
>>>>> +-------+
>>>>> |       |
>>>>> |  U1   |         +------+      +-----------+
>>>>> |       +--------->      |      |           |
>>>>> +-------+         |      |      |           |
>>>>>                    | Switch<-----+    SoC    |
>>>>> +-------+         |      |      |           |
>>>>> |       |         |      |      |           |
>>>>> | U2    <---------+--^---+      +-----------+
>>>>> |       |            |
>>>>> |       |            |
>>>>> +-------+            |
>>>>>                    +--+--+
>>>>>                    |     |
>>>>>                    | PCF |
>>>>>                    |     |
>>>>>                    +-----+
>>>>>
>>>>> At low, SoC pin is connected to U2 as drive. when reset to high, you
>>>>> now have U1 driving to the same pin that SoC has, potentially
>>>>> resulting in contention.
>>>>>
>>>>>
>>>>> Unfortunately, at this level, you do not know what the state of the
>>>>> system is, blindly forcing a pin level will potentially cause
>>>>> contention risk depending on pin configuration.
>>>>
>>>> Assume we are doing a reset when the system is powered on, irrespective of the
>>>> state of the system, we'll be forcing the pin level to the default state.
>>>
>>> Yes, I dont deny that system will be fine *after* reset sequence is
>>> started or completed. However there is a duration between the pcf
>>> shutdown handler is called and the final reset handler is invoked -
>>> that is the duration when  the contention might cause device behavior.
>>> Essentially ignoring the state various drivers have asked PCF to setup
>>> the pins and doing a hands down configuration may have side effects we
>>> cant properly expect.
>>
>> The solution might be to invoke the shutdown handler of the various drivers
>> using the PCF before the shutdown handler of the PCF driver itself gets
>> invoked? But I'm not sure how can that be achieved in linux kernel :-(
>
> #include <linux/reboot.h>
>
> static int foo_reboot_handler(struct notifier_block *this,
>                                  unsigned long code,
>                                  void *unused)
> {
>          pr_crit("do some last minute stuff\n");
>          return NOTIFY_OK;
> }
>
> static struct notifier_block foo_reboot_notifier = {
>          .notifier_call = foo_reboot_handler,
> };
>
> register_reboot_notifier(&foo_reboot_notifier);

Added debug prints and found the reboot notifier gets invoked before the
shutdown handler which means some stuff can be done after this reboot
notifier:-(

Thanks
Kishon

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines
  2015-03-16  8:46           ` Kishon Vijay Abraham I
@ 2015-03-18 12:21             ` Linus Walleij
  2015-03-18 13:21               ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2015-03-18 12:21 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Nishanth Menon, Alexandre Courbot, linux-gpio, linux-kernel,
	Sekhar Nori, Linux-OMAP, linux-mmc, Benoit Parrot

On Mon, Mar 16, 2015 at 9:46 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> On Wednesday 14 January 2015 05:28 PM, Linus Walleij wrote:

>> #include <linux/reboot.h>
>>
>> static int foo_reboot_handler(struct notifier_block *this,
>>                                  unsigned long code,
>>                                  void *unused)
>> {
>>          pr_crit("do some last minute stuff\n");
>>          return NOTIFY_OK;
>> }
>>
>> static struct notifier_block foo_reboot_notifier = {
>>          .notifier_call = foo_reboot_handler,
>> };
>>
>> register_reboot_notifier(&foo_reboot_notifier);
>
>
> Added debug prints and found the reboot notifier gets invoked before the
> shutdown handler which means some stuff can be done after this reboot
> notifier:-(

Specify exactly what stuff may happen after the reboot notifier.

Of course stuff happens both before and after the reboot
notifier, the question is exactly what, that may conflict with this.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines
  2015-03-18 12:21             ` Linus Walleij
@ 2015-03-18 13:21               ` Kishon Vijay Abraham I
  2015-03-25 15:28                 ` Linus Walleij
  0 siblings, 1 reply; 10+ messages in thread
From: Kishon Vijay Abraham I @ 2015-03-18 13:21 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Nishanth Menon, Alexandre Courbot, linux-gpio, linux-kernel,
	Sekhar Nori, Linux-OMAP, linux-mmc, Benoit Parrot

Hi,

On Wednesday 18 March 2015 05:51 PM, Linus Walleij wrote:
> On Mon, Mar 16, 2015 at 9:46 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> On Wednesday 14 January 2015 05:28 PM, Linus Walleij wrote:
>
>>> #include <linux/reboot.h>
>>>
>>> static int foo_reboot_handler(struct notifier_block *this,
>>>                                   unsigned long code,
>>>                                   void *unused)
>>> {
>>>           pr_crit("do some last minute stuff\n");
>>>           return NOTIFY_OK;
>>> }
>>>
>>> static struct notifier_block foo_reboot_notifier = {
>>>           .notifier_call = foo_reboot_handler,
>>> };
>>>
>>> register_reboot_notifier(&foo_reboot_notifier);
>>
>>
>> Added debug prints and found the reboot notifier gets invoked before the
>> shutdown handler which means some stuff can be done after this reboot
>> notifier:-(
>
> Specify exactly what stuff may happen after the reboot notifier.

okay, it's assumed the device may be used (active) till the shutdown handler
of that particular device is called.

In this particular case we are restoring the initial line state of all pcf
lines even though we don't know if the devices connected to it are still being
active, which might cause a contention as explained by NM [1]

If the reboot notifier gets invoked after the shutdown handler then we could
be sure that restoring the initial line state of the pcf lines wouldn't affect
the devices connected to it.

Thanks
Kishon

[1] -> http://www.spinics.net/lists/linux-mmc/msg29855.html
>
> Of course stuff happens both before and after the reboot
> notifier, the question is exactly what, that may conflict with this.
>
> Yours,
> Linus Walleij
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines
  2015-03-18 13:21               ` Kishon Vijay Abraham I
@ 2015-03-25 15:28                 ` Linus Walleij
  0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2015-03-25 15:28 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Nishanth Menon, Alexandre Courbot, linux-gpio, linux-kernel,
	Sekhar Nori, Linux-OMAP, linux-mmc, Benoit Parrot

On Wed, Mar 18, 2015 at 2:21 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>[Me]
>> Specify exactly what stuff may happen after the reboot notifier.
>
> okay, it's assumed the device may be used (active) till the shutdown handler
> of that particular device is called.
>
> In this particular case we are restoring the initial line state of all pcf
> lines even though we don't know if the devices connected to it are still
> being active, which might cause a contention as explained by NM [1]

OK I understand this, just that I want to know if it is a practical problem,
i.e. do you run into it? Does the system lock up?

> If the reboot notifier gets invoked after the shutdown handler then we could
> be sure that restoring the initial line state of the pcf lines wouldn't
> affect the devices connected to it.

OK so does it?

ftrace is your friend.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-03-25 15:28 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-12  8:06 [PATCH] gpio: pcf857x: restore the initial line state of all pcf lines Kishon Vijay Abraham I
2014-12-15 20:50 ` Nishanth Menon
2014-12-18  6:18   ` Kishon Vijay Abraham I
2014-12-18 14:11     ` Nishanth Menon
2015-01-05  6:26       ` Kishon Vijay Abraham I
2015-01-14 11:58         ` Linus Walleij
2015-03-16  8:46           ` Kishon Vijay Abraham I
2015-03-18 12:21             ` Linus Walleij
2015-03-18 13:21               ` Kishon Vijay Abraham I
2015-03-25 15:28                 ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).