[v2,1/9] i2c: prepare runtime PM support for I2C client devices

From: Aaron Lu <aaron.lu@intel.com>

This patch adds runtime PM support for the I2C bus in a similar way that
has been done for PCI bus already. This means that the I2C bus core
prepares runtime PM for a client device just before a driver is about to be
bound to it. Devices that are not bound to any driver are not prepared for
runtime PM.

In order to take advantage of this runtime PM support, the client device
driver needs drop the device runtime PM reference count by calling
pm_runtime_put() in its ->probe() callback and possibly implement rest of
the runtime PM callbacks.

If the driver doesn't support runtime PM (like most of the existing I2C
client drivers), the device in question is regarded as being runtime PM
active and powered on.

The patch adds also runtime PM support for the adapter device because it is
needed to be able to runtime power manage the I2C controller device. The
adapter device is handled along with the I2C controller device (it uses
pm_runtime_no_callbacks()).

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/i2c-core.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

Mika Westerberg <mika.westerberg@linux.intel.com> writes:

> From: Aaron Lu <aaron.lu@intel.com>
>
> This patch adds runtime PM support for the I2C bus in a similar way that
> has been done for PCI bus already. This means that the I2C bus core
> prepares runtime PM for a client device just before a driver is about to be
> bound to it. Devices that are not bound to any driver are not prepared for
> runtime PM.
>
> In order to take advantage of this runtime PM support, the client device
> driver needs drop the device runtime PM reference count by calling
> pm_runtime_put() in its ->probe() callback and possibly implement rest of
> the runtime PM callbacks.
>
> If the driver doesn't support runtime PM (like most of the existing I2C
> client drivers), the device in question is regarded as being runtime PM
> active and powered on.

But for existing drivers which already support runtime PM (at least 7 by
a quick grep), they will be stuck runtime enabled and stop hitting
low-power states after this patch.

> The patch adds also runtime PM support for the adapter device because it is
> needed to be able to runtime power manage the I2C controller device. The
> adapter device is handled along with the I2C controller device (it uses
> pm_runtime_no_callbacks()).
>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/i2c/i2c-core.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index f32ca29..44374b4 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
>  					client->flags & I2C_CLIENT_WAKE);
>  	dev_dbg(dev, "probe\n");
>  
> +	/* Make sure the adapter is active */
> +	pm_runtime_get_sync(&client->adapter->dev);
> +
> +	/*
> +	 * Enable runtime PM for the client device. If the client wants to
> +	 * participate on runtime PM it should call pm_runtime_put() in its
> +	 * probe() callback.
> +	 */
> +	pm_runtime_get_noresume(&client->dev);
> +	pm_runtime_set_active(&client->dev);

Why the set_active here?

For hardware that is disabled/powered-off on startup, there will now be
a mismatch between the hardware state an the RPM core state.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Sep 12, 2013 at 2:34 PM, Kevin Hilman <khilman@linaro.org> wrote:
> Mika Westerberg <mika.westerberg@linux.intel.com> writes:
>
>> From: Aaron Lu <aaron.lu@intel.com>
>>
>> This patch adds runtime PM support for the I2C bus in a similar way that
>> has been done for PCI bus already. This means that the I2C bus core
>> prepares runtime PM for a client device just before a driver is about to be
>> bound to it. Devices that are not bound to any driver are not prepared for
>> runtime PM.
>>
>> In order to take advantage of this runtime PM support, the client device
>> driver needs drop the device runtime PM reference count by calling
>> pm_runtime_put() in its ->probe() callback and possibly implement rest of
>> the runtime PM callbacks.
>>
>> If the driver doesn't support runtime PM (like most of the existing I2C
>> client drivers), the device in question is regarded as being runtime PM
>> active and powered on.
>
> But for existing drivers which already support runtime PM (at least 7 by
> a quick grep), they will be stuck runtime enabled and stop hitting
> low-power states after this patch.

Oops, nevermind.  I was mixing this up with runtime PM on the i2c
adapter but this is for the i2c client.

Sorry for the noise.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 09/11/2013 05:32 PM, Mika Westerberg wrote:
> From: Aaron Lu<aaron.lu@intel.com>
>
> This patch adds runtime PM support for the I2C bus in a similar way that
> has been done for PCI bus already. This means that the I2C bus core
> prepares runtime PM for a client device just before a driver is about to be
> bound to it. Devices that are not bound to any driver are not prepared for
> runtime PM.
>
> In order to take advantage of this runtime PM support, the client device
> driver needs drop the device runtime PM reference count by calling
> pm_runtime_put() in its ->probe() callback and possibly implement rest of
> the runtime PM callbacks.
>
> If the driver doesn't support runtime PM (like most of the existing I2C
> client drivers), the device in question is regarded as being runtime PM
> active and powered on.
>
> The patch adds also runtime PM support for the adapter device because it is
> needed to be able to runtime power manage the I2C controller device. The
> adapter device is handled along with the I2C controller device (it uses
> pm_runtime_no_callbacks()).
>
> Signed-off-by: Aaron Lu<aaron.lu@intel.com>
> Signed-off-by: Mika Westerberg<mika.westerberg@linux.intel.com>
> ---
>   drivers/i2c/i2c-core.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index f32ca29..44374b4 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
>   					client->flags&  I2C_CLIENT_WAKE);
>   	dev_dbg(dev, "probe\n");
>
> +	/* Make sure the adapter is active */
> +	pm_runtime_get_sync(&client->adapter->dev);

So there is currently no way to avoid this behaviour, i.e. to have the 
adapter
not activated before any of its client devices is probed, but only later on,
after explicit call to pm_runtime_get*(&client->dev) in the client driver ?

--
Thanks,
Sylwester



--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 09/13/2013 06:06 AM, Sylwester Nawrocki wrote:
> On 09/11/2013 05:32 PM, Mika Westerberg wrote:
>> From: Aaron Lu<aaron.lu@intel.com>
>>
>> This patch adds runtime PM support for the I2C bus in a similar way that
>> has been done for PCI bus already. This means that the I2C bus core
>> prepares runtime PM for a client device just before a driver is about to be
>> bound to it. Devices that are not bound to any driver are not prepared for
>> runtime PM.
>>
>> In order to take advantage of this runtime PM support, the client device
>> driver needs drop the device runtime PM reference count by calling
>> pm_runtime_put() in its ->probe() callback and possibly implement rest of
>> the runtime PM callbacks.
>>
>> If the driver doesn't support runtime PM (like most of the existing I2C
>> client drivers), the device in question is regarded as being runtime PM
>> active and powered on.
>>
>> The patch adds also runtime PM support for the adapter device because it is
>> needed to be able to runtime power manage the I2C controller device. The
>> adapter device is handled along with the I2C controller device (it uses
>> pm_runtime_no_callbacks()).
>>
>> Signed-off-by: Aaron Lu<aaron.lu@intel.com>
>> Signed-off-by: Mika Westerberg<mika.westerberg@linux.intel.com>
>> ---
>>   drivers/i2c/i2c-core.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index f32ca29..44374b4 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
>>   					client->flags&  I2C_CLIENT_WAKE);
>>   	dev_dbg(dev, "probe\n");
>>
>> +	/* Make sure the adapter is active */
>> +	pm_runtime_get_sync(&client->adapter->dev);
> 
> So there is currently no way to avoid this behaviour, i.e. to have the 
> adapter
> not activated before any of its client devices is probed, but only later on,
> after explicit call to pm_runtime_get*(&client->dev) in the client driver ?

The above pm_runtime_get_sync is used to make sure when the client I2C
device is going to be probed, its host adapter device is turned on(or we
will fail the probe). It doesn't affect the adapter's status before the
probe of I2C client device.

Thanks,
Aaron
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index f32ca29..44374b4 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
> >  					client->flags & I2C_CLIENT_WAKE);
> >  	dev_dbg(dev, "probe\n");
> >  
> > +	/* Make sure the adapter is active */
> > +	pm_runtime_get_sync(&client->adapter->dev);
> > +
> > +	/*
> > +	 * Enable runtime PM for the client device. If the client wants to
> > +	 * participate on runtime PM it should call pm_runtime_put() in its
> > +	 * probe() callback.
> > +	 */
> > +	pm_runtime_get_noresume(&client->dev);
> > +	pm_runtime_set_active(&client->dev);
> 
> Why the set_active here?
> 
> For hardware that is disabled/powered-off on startup, there will now be
> a mismatch between the hardware state an the RPM core state.

The call to pm_runtime_get_noresume() should make sure that the device is
in active state (at least in state where it can access the bus) if I'm
understanding this right.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Sep 13, 2013 at 09:54:34AM +0300, Mika Westerberg wrote:
> On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:

> > For hardware that is disabled/powered-off on startup, there will now be
> > a mismatch between the hardware state an the RPM core state.

> The call to pm_runtime_get_noresume() should make sure that the device is
> in active state (at least in state where it can access the bus) if I'm
> understanding this right.

Accessing the bus isn't an issue for I2C outside of ACPI, the power
management of the device is totally disassociated from the bus and the
controller is responsible for ensuring it is available during transfers.

On Fri, Sep 13, 2013 at 09:14:20AM +0800, Aaron Lu wrote:
> On 09/13/2013 06:06 AM, Sylwester Nawrocki wrote:

> > So there is currently no way to avoid this behaviour, i.e. to have the 
> > adapter
> > not activated before any of its client devices is probed, but only later on,
> > after explicit call to pm_runtime_get*(&client->dev) in the client driver ?

> The above pm_runtime_get_sync is used to make sure when the client I2C
> device is going to be probed, its host adapter device is turned on(or we
> will fail the probe). It doesn't affect the adapter's status before the
> probe of I2C client device.

The expecation is that if the adaptor needs to do anything to transfer
it'll do that when asked to transfer - that way it can sit in a low
power state when the bus is idle.

On Fri, Sep 13, 2013 at 10:59:50AM +0100, Mark Brown wrote:
> On Fri, Sep 13, 2013 at 09:54:34AM +0300, Mika Westerberg wrote:
> > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
> 
> > > For hardware that is disabled/powered-off on startup, there will now be
> > > a mismatch between the hardware state an the RPM core state.
> 
> > The call to pm_runtime_get_noresume() should make sure that the device is
> > in active state (at least in state where it can access the bus) if I'm
> > understanding this right.
> 
> Accessing the bus isn't an issue for I2C outside of ACPI, the power
> management of the device is totally disassociated from the bus and the
> controller is responsible for ensuring it is available during transfers.

Yes, but since we want to support ACPI as well, we must make sure that the
adapter (and the associated controller) is available when client ->probe()
is called.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Sep 13, 2013 at 01:16:11PM +0300, Mika Westerberg wrote:
> On Fri, Sep 13, 2013 at 10:59:50AM +0100, Mark Brown wrote:

> > Accessing the bus isn't an issue for I2C outside of ACPI, the power
> > management of the device is totally disassociated from the bus and the
> > controller is responsible for ensuring it is available during transfers.

> Yes, but since we want to support ACPI as well, we must make sure that the
> adapter (and the associated controller) is available when client ->probe()
> is called.

Right, but this probably needs to be highlighted more since it's a very
surprising thing for I2C and is causing confusion.

On Fri, Sep 13, 2013 at 11:31:52AM +0100, Mark Brown wrote:
> On Fri, Sep 13, 2013 at 01:16:11PM +0300, Mika Westerberg wrote:
> > On Fri, Sep 13, 2013 at 10:59:50AM +0100, Mark Brown wrote:
> 
> > > Accessing the bus isn't an issue for I2C outside of ACPI, the power
> > > management of the device is totally disassociated from the bus and the
> > > controller is responsible for ensuring it is available during transfers.
> 
> > Yes, but since we want to support ACPI as well, we must make sure that the
> > adapter (and the associated controller) is available when client ->probe()
> > is called.
> 
> Right, but this probably needs to be highlighted more since it's a very
> surprising thing for I2C and is causing confusion.

By highlighted more, do you mean something like adding a comment in the
code about this or?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Sep 13, 2013 at 02:50:35PM +0300, Mika Westerberg wrote:
> On Fri, Sep 13, 2013 at 11:31:52AM +0100, Mark Brown wrote:

> > Right, but this probably needs to be highlighted more since it's a very
> > surprising thing for I2C and is causing confusion.

> By highlighted more, do you mean something like adding a comment in the
> code about this or?

Perhaps, yes.  Or possibly the commit log is going to be enough going
forwards.

Mika Westerberg <mika.westerberg@linux.intel.com> writes:

> On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
>> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> > index f32ca29..44374b4 100644
>> > --- a/drivers/i2c/i2c-core.c
>> > +++ b/drivers/i2c/i2c-core.c
>> > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
>> >  					client->flags & I2C_CLIENT_WAKE);
>> >  	dev_dbg(dev, "probe\n");
>> >  
>> > +	/* Make sure the adapter is active */
>> > +	pm_runtime_get_sync(&client->adapter->dev);
>> > +
>> > +	/*
>> > +	 * Enable runtime PM for the client device. If the client wants to
>> > +	 * participate on runtime PM it should call pm_runtime_put() in its
>> > +	 * probe() callback.
>> > +	 */
>> > +	pm_runtime_get_noresume(&client->dev);
>> > +	pm_runtime_set_active(&client->dev);
>> 
>> Why the set_active here?
>> 
>> For hardware that is disabled/powered-off on startup, there will now be
>> a mismatch between the hardware state an the RPM core state.
>
> The call to pm_runtime_get_noresume() should make sure that the device is
> in active state (at least in state where it can access the bus) if I'm
> understanding this right.

No, after _get_noresume(), nothing happens to the hardware.  It simply
increments the usecount.  From pm_runtime.h:

static inline void pm_runtime_get_noresume(struct device *dev)
{
	atomic_inc(&dev->power.usage_count);
}

So after the _get_noresume() and _set_active() you're very likely to
have a disconnect between the hardware state and what state RPM thinks
the hardware is in.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Sep 13, 2013 at 07:30:55AM -0700, Kevin Hilman wrote:
> Mika Westerberg <mika.westerberg@linux.intel.com> writes:
> 
> > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
> >> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >> > index f32ca29..44374b4 100644
> >> > --- a/drivers/i2c/i2c-core.c
> >> > +++ b/drivers/i2c/i2c-core.c
> >> > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
> >> >  					client->flags & I2C_CLIENT_WAKE);
> >> >  	dev_dbg(dev, "probe\n");
> >> >  
> >> > +	/* Make sure the adapter is active */
> >> > +	pm_runtime_get_sync(&client->adapter->dev);
> >> > +
> >> > +	/*
> >> > +	 * Enable runtime PM for the client device. If the client wants to
> >> > +	 * participate on runtime PM it should call pm_runtime_put() in its
> >> > +	 * probe() callback.
> >> > +	 */
> >> > +	pm_runtime_get_noresume(&client->dev);
> >> > +	pm_runtime_set_active(&client->dev);
> >> 
> >> Why the set_active here?
> >> 
> >> For hardware that is disabled/powered-off on startup, there will now be
> >> a mismatch between the hardware state an the RPM core state.
> >
> > The call to pm_runtime_get_noresume() should make sure that the device is
> > in active state (at least in state where it can access the bus) if I'm
> > understanding this right.
> 
> No, after _get_noresume(), nothing happens to the hardware.  It simply
> increments the usecount.  From pm_runtime.h:
> 
> static inline void pm_runtime_get_noresume(struct device *dev)
> {
> 	atomic_inc(&dev->power.usage_count);
> }
> 
> So after the _get_noresume() and _set_active() you're very likely to
> have a disconnect between the hardware state and what state RPM thinks
> the hardware is in.

Good point.

I suppose calling pm_runtime_get() here would work (and make the state
active in all case)? I used _noresume() here because at this point the
driver itself hasn't had change to install it's RPM hooks.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 09/13/2013 08:54 AM, Mika Westerberg wrote:
> On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
>>> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>>> > > index f32ca29..44374b4 100644
>>> > > --- a/drivers/i2c/i2c-core.c
>>> > > +++ b/drivers/i2c/i2c-core.c
>>> > > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
>>> > >  					client->flags & I2C_CLIENT_WAKE);
>>> > >  	dev_dbg(dev, "probe\n");
>>> > >  
>>> > > +	/* Make sure the adapter is active */
>>> > > +	pm_runtime_get_sync(&client->adapter->dev);
>>> > > +
>>> > > +	/*
>>> > > +	 * Enable runtime PM for the client device. If the client wants to
>>> > > +	 * participate on runtime PM it should call pm_runtime_put() in its
>>> > > +	 * probe() callback.
>>> > > +	 */
>>> > > +	pm_runtime_get_noresume(&client->dev);
>>> > > +	pm_runtime_set_active(&client->dev);
>> > 
>> > Why the set_active here?
>> > 
>> > For hardware that is disabled/powered-off on startup, there will now be
>> > a mismatch between the hardware state an the RPM core state.
>
> The call to pm_runtime_get_noresume() should make sure that the device is
> in active state (at least in state where it can access the bus) if I'm
> understanding this right.

I can't see how this would happen. How runtime_resume/runtime_suspend
callbacks would get invoked with this code, if, e.g. originally driver called 
pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ? 

pm_runtime_get_noresume() merely increments usage counter of a device. 
It seems that these changes will break the s5p-tv driver. I might be missing 
something though.

As Mark pointed out this is currently unwanted behaviour to runtime PM
activate a bus controller device manually in the core for when the client's
probe() is executed, since i2c core will activate the bus controller for when 
transfer is being carried out.  
But I can understand this is needed for ACPI and it shouldn't break existing
drivers, that do runtime PM activate the client device in probe().

Now I'm sure this will break power management of the drivers/media/exynos4-is
driver, due to incorrect power sequence (power domain and clocks handling).
I'll try to take care of it in separate patch, as I have some patches pending,
that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to 
drivers/media/i2c/s5k6a3.c.

--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Sep 13, 2013 at 05:14:56PM +0200, Sylwester Nawrocki wrote:
> On 09/13/2013 08:54 AM, Mika Westerberg wrote:
> > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
> >>> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >>> > > index f32ca29..44374b4 100644
> >>> > > --- a/drivers/i2c/i2c-core.c
> >>> > > +++ b/drivers/i2c/i2c-core.c
> >>> > > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
> >>> > >  					client->flags & I2C_CLIENT_WAKE);
> >>> > >  	dev_dbg(dev, "probe\n");
> >>> > >  
> >>> > > +	/* Make sure the adapter is active */
> >>> > > +	pm_runtime_get_sync(&client->adapter->dev);
> >>> > > +
> >>> > > +	/*
> >>> > > +	 * Enable runtime PM for the client device. If the client wants to
> >>> > > +	 * participate on runtime PM it should call pm_runtime_put() in its
> >>> > > +	 * probe() callback.
> >>> > > +	 */
> >>> > > +	pm_runtime_get_noresume(&client->dev);
> >>> > > +	pm_runtime_set_active(&client->dev);
> >> > 
> >> > Why the set_active here?
> >> > 
> >> > For hardware that is disabled/powered-off on startup, there will now be
> >> > a mismatch between the hardware state an the RPM core state.
> >
> > The call to pm_runtime_get_noresume() should make sure that the device is
> > in active state (at least in state where it can access the bus) if I'm
> > understanding this right.
> 
> I can't see how this would happen. How runtime_resume/runtime_suspend
> callbacks would get invoked with this code, if, e.g. originally driver called 
> pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ? 

The driver callbacks are not called but if the device has been attached to
a power domain (like we do with ACPI) the power domain callbacks get called
and it brings the "bus" to such state that we are able to access the
device. That also was the reason I used _noresume() but didn't look too
close the implementation.

> pm_runtime_get_noresume() merely increments usage counter of a device. 
> It seems that these changes will break the s5p-tv driver. I might be missing 
> something though.

You are right and Kevin also mentioned this. It should be pm_runtime_get(),
if I'm not mistaken.

> As Mark pointed out this is currently unwanted behaviour to runtime PM
> activate a bus controller device manually in the core for when the client's
> probe() is executed, since i2c core will activate the bus controller for when 
> transfer is being carried out.  
>
> But I can understand this is needed for ACPI and it shouldn't break existing
> drivers, that do runtime PM activate the client device in probe().

Indeed, we don't want to break anything (and we still need something like
this for ACPI).

> Now I'm sure this will break power management of the drivers/media/exynos4-is
> driver, due to incorrect power sequence (power domain and clocks handling).
> I'll try to take care of it in separate patch, as I have some patches pending,
> that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to 
> drivers/media/i2c/s5k6a3.c.

I missed that code when I converted existing users to this method. Sorry
about that (I can handle that in the next version).

I quickly looked at it and I don't see anything that could break (once
converted). What it does is this:

	pm_runtime_no_callbacks(dev);
	pm_runtime_enable(dev);

changing that to:

	pm_runtime_no_callbacks(dev);
	pm_runtime_put(dev);

shouldn't cause problems AFAICT.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Sep 13, 2013 at 05:50:22PM +0300, Mika Westerberg wrote:
> On Fri, Sep 13, 2013 at 07:30:55AM -0700, Kevin Hilman wrote:
> > Mika Westerberg <mika.westerberg@linux.intel.com> writes:
> > 
> > > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
> > >> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > >> > index f32ca29..44374b4 100644
> > >> > --- a/drivers/i2c/i2c-core.c
> > >> > +++ b/drivers/i2c/i2c-core.c
> > >> > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
> > >> >  					client->flags & I2C_CLIENT_WAKE);
> > >> >  	dev_dbg(dev, "probe\n");
> > >> >  
> > >> > +	/* Make sure the adapter is active */
> > >> > +	pm_runtime_get_sync(&client->adapter->dev);
> > >> > +
> > >> > +	/*
> > >> > +	 * Enable runtime PM for the client device. If the client wants to
> > >> > +	 * participate on runtime PM it should call pm_runtime_put() in its
> > >> > +	 * probe() callback.
> > >> > +	 */
> > >> > +	pm_runtime_get_noresume(&client->dev);
> > >> > +	pm_runtime_set_active(&client->dev);
> > >> 
> > >> Why the set_active here?
> > >> 
> > >> For hardware that is disabled/powered-off on startup, there will now be
> > >> a mismatch between the hardware state an the RPM core state.
> > >
> > > The call to pm_runtime_get_noresume() should make sure that the device is
> > > in active state (at least in state where it can access the bus) if I'm
> > > understanding this right.
> > 
> > No, after _get_noresume(), nothing happens to the hardware.  It simply
> > increments the usecount.  From pm_runtime.h:
> > 
> > static inline void pm_runtime_get_noresume(struct device *dev)
> > {
> > 	atomic_inc(&dev->power.usage_count);
> > }
> > 
> > So after the _get_noresume() and _set_active() you're very likely to
> > have a disconnect between the hardware state and what state RPM thinks
> > the hardware is in.
> 
> Good point.
> 
> I suppose calling pm_runtime_get() here would work (and make the state
> active in all case)? I used _noresume() here because at this point the
> driver itself hasn't had change to install it's RPM hooks.

I take that back.

[ It has been some time since this code was originally written so I can't
remember all the details :-/ ]

The pm_runtime_get_noresume() is there just to increment the refcount. It
has nothing to do to "activate" the device in question. Sorry about the
confusion from my part.

This is what happens in case the device is enumerated from ACPI:

	// This makes sure that the controller itself is powered on
	// (adapter device follows its parent which is the controller). The
	// controller is attached to the ACPI power domain so it is
	// brought to D0 now.
	pm_runtime_get_sync(&client->adapter->dev);

	// This binds the client device to the ACPI power domain, and in
	// addition to that brings the client device to D0.
	if (ACPI_HANDLE(&client->dev))
		acpi_dev_pm_attach(&client->dev, true);

	// Increase the refcount so that client can start runtime PM
	// transitions when it calls _put().
	pm_runtime_get_noresume(&client->dev);

	// Mark the device being active as
	//  1) In ACPI case we know that is true as we just powered the
	//     device on.
	//  2) We treat the device by default to be runtime PM active and
	//     powered on (that's in the changelog and should follow what
	//     the PCI bus did).
	pm_runtime_set_active(&client->dev);

	// Enable runtime PM but nothing happens yet as long as the client
	// driver doesn't call _put().
	pm_runtime_enable(&client->dev);

So, yes there might be a disconnect between the runtime PM state and the
device HW state now (same is with default to suspended).

When the driver ->probe() is called, it needs to power on the device (which
it probably needs to do anyway to be able to talk to the device and probe
its registers, etc.). Once the driver calls _put() the device is eventually
moved to suspended state (and its RPM hooks are called). I might be missing
something but is there a case where this is not beneficial?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mika Westerberg <mika.westerberg@linux.intel.com> writes:

> On Fri, Sep 13, 2013 at 05:50:22PM +0300, Mika Westerberg wrote:
>> On Fri, Sep 13, 2013 at 07:30:55AM -0700, Kevin Hilman wrote:
>> > Mika Westerberg <mika.westerberg@linux.intel.com> writes:
>> > 
>> > > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
>> > >> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> > >> > index f32ca29..44374b4 100644
>> > >> > --- a/drivers/i2c/i2c-core.c
>> > >> > +++ b/drivers/i2c/i2c-core.c
>> > >> > @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
>> > >> >  					client->flags & I2C_CLIENT_WAKE);
>> > >> >  	dev_dbg(dev, "probe\n");
>> > >> >  
>> > >> > +	/* Make sure the adapter is active */
>> > >> > +	pm_runtime_get_sync(&client->adapter->dev);
>> > >> > +
>> > >> > +	/*
>> > >> > +	 * Enable runtime PM for the client device. If the client wants to
>> > >> > +	 * participate on runtime PM it should call pm_runtime_put() in its
>> > >> > +	 * probe() callback.
>> > >> > +	 */
>> > >> > +	pm_runtime_get_noresume(&client->dev);
>> > >> > +	pm_runtime_set_active(&client->dev);
>> > >> 
>> > >> Why the set_active here?
>> > >> 
>> > >> For hardware that is disabled/powered-off on startup, there will now be
>> > >> a mismatch between the hardware state an the RPM core state.
>> > >
>> > > The call to pm_runtime_get_noresume() should make sure that the device is
>> > > in active state (at least in state where it can access the bus) if I'm
>> > > understanding this right.
>> > 
>> > No, after _get_noresume(), nothing happens to the hardware.  It simply
>> > increments the usecount.  From pm_runtime.h:
>> > 
>> > static inline void pm_runtime_get_noresume(struct device *dev)
>> > {
>> > 	atomic_inc(&dev->power.usage_count);
>> > }
>> > 
>> > So after the _get_noresume() and _set_active() you're very likely to
>> > have a disconnect between the hardware state and what state RPM thinks
>> > the hardware is in.
>> 
>> Good point.
>> 
>> I suppose calling pm_runtime_get() here would work (and make the state
>> active in all case)? I used _noresume() here because at this point the
>> driver itself hasn't had change to install it's RPM hooks.
>
> I take that back.
>
> [ It has been some time since this code was originally written so I can't
> remember all the details :-/ ]
>
> The pm_runtime_get_noresume() is there just to increment the refcount. It
> has nothing to do to "activate" the device in question. Sorry about the
> confusion from my part.
>
> This is what happens in case the device is enumerated from ACPI:
>
> 	// This makes sure that the controller itself is powered on
> 	// (adapter device follows its parent which is the controller). The
> 	// controller is attached to the ACPI power domain so it is
> 	// brought to D0 now.
> 	pm_runtime_get_sync(&client->adapter->dev);
>
> 	// This binds the client device to the ACPI power domain, and in
> 	// addition to that brings the client device to D0.

OK, then here is where the problem is, because you're building ACPI
assumptions into the core. For non-ACPI devices, this part is a nop, so
the client device is still powered off, which breaks the assumptions
below.

> 	if (ACPI_HANDLE(&client->dev))
> 		acpi_dev_pm_attach(&client->dev, true);
>
> 	// Increase the refcount so that client can start runtime PM
> 	// transitions when it calls _put().
> 	pm_runtime_get_noresume(&client->dev);

> 	// Mark the device being active as
> 	//  1) In ACPI case we know that is true as we just powered the
> 	//     device on.
> 	//  2) We treat the device by default to be runtime PM active and
> 	//     powered on (that's in the changelog and should follow what
> 	//     the PCI bus did).
> 	pm_runtime_set_active(&client->dev);
>
> 	// Enable runtime PM but nothing happens yet as long as the client
> 	// driver doesn't call _put().
> 	pm_runtime_enable(&client->dev);
>
> So, yes there might be a disconnect between the runtime PM state and the
> device HW state now (same is with default to suspended).

Yes, but until now, default to suspended has been assumed, so any
changes to that will likely require more thorough auditing of other drivers.

Kevin

> When the driver ->probe() is called, it needs to power on the device (which
> it probably needs to do anyway to be able to talk to the device and probe
> its registers, etc.). Once the driver calls _put() the device is eventually
> moved to suspended state (and its RPM hooks are called). I might be missing
> something but is there a case where this is not beneficial?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Sep 13, 2013 at 02:10:43PM -0700, Kevin Hilman wrote:
> >
> > 	// This makes sure that the controller itself is powered on
> > 	// (adapter device follows its parent which is the controller). The
> > 	// controller is attached to the ACPI power domain so it is
> > 	// brought to D0 now.
> > 	pm_runtime_get_sync(&client->adapter->dev);
> >
> > 	// This binds the client device to the ACPI power domain, and in
> > 	// addition to that brings the client device to D0.
> 
> OK, then here is where the problem is, because you're building ACPI
> assumptions into the core. For non-ACPI devices, this part is a nop, so
> the client device is still powered off, which breaks the assumptions
> below.

We expect that once the driver ->probe() is called, and it doesn't
participate the runtime PM prepared here, the device is regarded as powered
on, runtime PM active.

If the driver participates in runtime PM, it needs to power on the device
and then call pm_runtime_put() to suspend the device.

> > 	if (ACPI_HANDLE(&client->dev))
> > 		acpi_dev_pm_attach(&client->dev, true);
> >
> > 	// Increase the refcount so that client can start runtime PM
> > 	// transitions when it calls _put().
> > 	pm_runtime_get_noresume(&client->dev);
> 
> > 	// Mark the device being active as
> > 	//  1) In ACPI case we know that is true as we just powered the
> > 	//     device on.
> > 	//  2) We treat the device by default to be runtime PM active and
> > 	//     powered on (that's in the changelog and should follow what
> > 	//     the PCI bus did).
> > 	pm_runtime_set_active(&client->dev);
> >
> > 	// Enable runtime PM but nothing happens yet as long as the client
> > 	// driver doesn't call _put().
> > 	pm_runtime_enable(&client->dev);
> >
> > So, yes there might be a disconnect between the runtime PM state and the
> > device HW state now (same is with default to suspended).
> 
> Yes, but until now, default to suspended has been assumed, so any
> changes to that will likely require more thorough auditing of other drivers.

I agree. And it looks like I missed few existing drivers as well. I'm going
to update them in the next version of the series.

There's also a less intrusive way of fixing the problem we see with ACPI
enabled I2C devices:

	1. In I2C core i2c_device_probe() we power on the I2C controller
	and attach the client device to the ACPI power domain. Just like in
	this patch but we don't touch the I2C client device runtime PM.

	-> This should allow the existing drivers to keep using whatever
	runtime PM strategy they have chosen.

	2. For ACPI enumerated I2C client devices drivers we need to
	implement the runtime PM in order to save power (otherwise the
	devices will be left powered on).

and do the same for SPI devices as well.

Then only thing that changes for non-ACPI devices is that the controller
will be powered on during the client device probe (well, and during
remove).

Thoughts?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sun, Sep 15, 2013 at 09:41:39AM +0300, Mika Westerberg wrote:

> There's also a less intrusive way of fixing the problem we see with ACPI
> enabled I2C devices:

> 	1. In I2C core i2c_device_probe() we power on the I2C controller
> 	and attach the client device to the ACPI power domain. Just like in
> 	this patch but we don't touch the I2C client device runtime PM.

> 	-> This should allow the existing drivers to keep using whatever
> 	runtime PM strategy they have chosen.

There should be no explicit need to power on the I2C controller if it's
implemented the same way the existing ones are - just have it power
itself on when it is doing a transfer.

> 	2. For ACPI enumerated I2C client devices drivers we need to
> 	implement the runtime PM in order to save power (otherwise the
> 	devices will be left powered on).

> and do the same for SPI devices as well.

> Then only thing that changes for non-ACPI devices is that the controller
> will be powered on during the client device probe (well, and during
> remove).

> Thoughts?

This is definitely less intrusive than the current proposal, there's one
ACPI I2C device binding queued for -next after the merge window (rt5640)
which will need an update.

On Sun, Sep 15, 2013 at 01:47:44PM +0100, Mark Brown wrote:
> On Sun, Sep 15, 2013 at 09:41:39AM +0300, Mika Westerberg wrote:
> 
> > There's also a less intrusive way of fixing the problem we see with ACPI
> > enabled I2C devices:
> 
> > 	1. In I2C core i2c_device_probe() we power on the I2C controller
> > 	and attach the client device to the ACPI power domain. Just like in
> > 	this patch but we don't touch the I2C client device runtime PM.
> 
> > 	-> This should allow the existing drivers to keep using whatever
> > 	runtime PM strategy they have chosen.
> 
> There should be no explicit need to power on the I2C controller if it's
> implemented the same way the existing ones are - just have it power
> itself on when it is doing a transfer.

The problem is that the ACPI child device can't be in higher power state
than its parent (and this is also what the runtime PM expects). If we don't
power the I2C controller device before we power on the I2C client device
that rule is violated and we get an complaint to the console.

> > 	2. For ACPI enumerated I2C client devices drivers we need to
> > 	implement the runtime PM in order to save power (otherwise the
> > 	devices will be left powered on).
> 
> > and do the same for SPI devices as well.
> 
> > Then only thing that changes for non-ACPI devices is that the controller
> > will be powered on during the client device probe (well, and during
> > remove).
> 
> > Thoughts?
> 
> This is definitely less intrusive than the current proposal, there's one
> ACPI I2C device binding queued for -next after the merge window (rt5640)
> which will need an update.

OK, thanks for the heads up. I'll check it.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 09/13/2013 05:40 PM, Mika Westerberg wrote:
[...]
>>> The call to pm_runtime_get_noresume() should make sure that the device is
>>> in active state (at least in state where it can access the bus) if I'm
>>> understanding this right.
>>
>> I can't see how this would happen. How runtime_resume/runtime_suspend
>> callbacks would get invoked with this code, if, e.g. originally driver called
>> pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
>
> The driver callbacks are not called but if the device has been attached to
> a power domain (like we do with ACPI) the power domain callbacks get called
> and it brings the "bus" to such state that we are able to access the
> device. That also was the reason I used _noresume() but didn't look too
> close the implementation.

OK, but if a client driver assumes default inactive power state it will 
expect
its callbacks to get called. Otherwise exisiting code might break. So, e.g.
in case of s5p-tv it would rather need to be something like:

	pm_runtime_put()

	pm_runtime_get_sync()
  	sii9234_verify_version()
  	pm_runtime_put(dev)

>> pm_runtime_get_noresume() merely increments usage counter of a device.
>> It seems that these changes will break the s5p-tv driver. I might be missing
>> something though.
>
> You are right and Kevin also mentioned this. It should be pm_runtime_get(),
> if I'm not mistaken.

Note that client drivers usually call pm_runtime_enable() only when it 
is safe
to call their driver's runtime PM callbacks. By enabling runtime PM 
before the
client's driver has completely initialized we may risk that the 
callbacks are
executed with uninitialized data, if I understand things correctly.

>> As Mark pointed out this is currently unwanted behaviour to runtime PM
>> activate a bus controller device manually in the core for when the client's
>> probe() is executed, since i2c core will activate the bus controller for when
>> transfer is being carried out.
>>
>> But I can understand this is needed for ACPI and it shouldn't break existing
>> drivers, that do runtime PM activate the client device in probe().
>
> Indeed, we don't want to break anything (and we still need something like
> this for ACPI).
>
>> Now I'm sure this will break power management of the drivers/media/exynos4-is
>> driver, due to incorrect power sequence (power domain and clocks handling).
>> I'll try to take care of it in separate patch, as I have some patches pending,
>> that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
>> drivers/media/i2c/s5k6a3.c.
>
> I missed that code when I converted existing users to this method. Sorry
> about that (I can handle that in the next version).
>
> I quickly looked at it and I don't see anything that could break (once
> converted). What it does is this:
>
> 	pm_runtime_no_callbacks(dev);
> 	pm_runtime_enable(dev);
>
> changing that to:
>
> 	pm_runtime_no_callbacks(dev);
> 	pm_runtime_put(dev);
>
> shouldn't cause problems AFAICT.

Yes, considering this driver in isolation it should be fine.

However, I observed system suspend issues when the I2C bus controller was
being activated (which would happen in the I2C core after your changes)
before some other driver has initialized.

So to ensure things continue to work the "fimc-isp-i2c" driver would need
to be registered after the "exynos4-fimc-is" driver has initialized. Or the
"exynos4-fimc-is" would need to call of_platform_populate() to instantiate
its all children devices as specified in device tree (see arch/arm/boot/dts/
exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
the compatible property of that top level device. So to avoid regressions
some additional changes would be needed, outside of this particular I2C
client driver. I guess this could be avoided by better design of the
exynos4-is driver right from the beginning. But it's all some times tricky
when there is some many IP blocks involved and the hardware behaviour/device
interactions are not always well documented.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:
> On 09/13/2013 05:40 PM, Mika Westerberg wrote:
> [...]
> >>>The call to pm_runtime_get_noresume() should make sure that the device is
> >>>in active state (at least in state where it can access the bus) if I'm
> >>>understanding this right.
> >>
> >>I can't see how this would happen. How runtime_resume/runtime_suspend
> >>callbacks would get invoked with this code, if, e.g. originally driver called
> >>pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
> >
> >The driver callbacks are not called but if the device has been attached to
> >a power domain (like we do with ACPI) the power domain callbacks get called
> >and it brings the "bus" to such state that we are able to access the
> >device. That also was the reason I used _noresume() but didn't look too
> >close the implementation.
> 
> OK, but if a client driver assumes default inactive power state it
> will expect
> its callbacks to get called. Otherwise exisiting code might break. So, e.g.
> in case of s5p-tv it would rather need to be something like:
> 
> 	pm_runtime_put()
> 
> 	pm_runtime_get_sync()
>  	sii9234_verify_version()
>  	pm_runtime_put(dev)

Yes, or even call directly its runtime_resume callback to bring the device
to the active state.

> >>pm_runtime_get_noresume() merely increments usage counter of a device.
> >>It seems that these changes will break the s5p-tv driver. I might be missing
> >>something though.
> >
> >You are right and Kevin also mentioned this. It should be pm_runtime_get(),
> >if I'm not mistaken.
> 
> Note that client drivers usually call pm_runtime_enable() only when
> it is safe
> to call their driver's runtime PM callbacks. By enabling runtime PM
> before the
> client's driver has completely initialized we may risk that the
> callbacks are
> executed with uninitialized data, if I understand things correctly.

I think that calling pm_runtime_enable() on behalf of the client driver
shouldn't cause any problems. There is no PM events queued for that device
yet (and we have prevented that from happening when we called
_get_noresume() for the device).

Only when the client driver calls _put() things start to happen and at that
phase the runtime PM hooks should be usable.

> >>As Mark pointed out this is currently unwanted behaviour to runtime PM
> >>activate a bus controller device manually in the core for when the client's
> >>probe() is executed, since i2c core will activate the bus controller for when
> >>transfer is being carried out.
> >>
> >>But I can understand this is needed for ACPI and it shouldn't break existing
> >>drivers, that do runtime PM activate the client device in probe().
> >
> >Indeed, we don't want to break anything (and we still need something like
> >this for ACPI).
> >
> >>Now I'm sure this will break power management of the drivers/media/exynos4-is
> >>driver, due to incorrect power sequence (power domain and clocks handling).
> >>I'll try to take care of it in separate patch, as I have some patches pending,
> >>that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
> >>drivers/media/i2c/s5k6a3.c.
> >
> >I missed that code when I converted existing users to this method. Sorry
> >about that (I can handle that in the next version).
> >
> >I quickly looked at it and I don't see anything that could break (once
> >converted). What it does is this:
> >
> >	pm_runtime_no_callbacks(dev);
> >	pm_runtime_enable(dev);
> >
> >changing that to:
> >
> >	pm_runtime_no_callbacks(dev);
> >	pm_runtime_put(dev);
> >
> >shouldn't cause problems AFAICT.
> 
> Yes, considering this driver in isolation it should be fine.
> 
> However, I observed system suspend issues when the I2C bus controller was
> being activated (which would happen in the I2C core after your changes)
> before some other driver has initialized.
> 
> So to ensure things continue to work the "fimc-isp-i2c" driver would need
> to be registered after the "exynos4-fimc-is" driver has initialized. Or the
> "exynos4-fimc-is" would need to call of_platform_populate() to instantiate
> its all children devices as specified in device tree (see arch/arm/boot/dts/
> exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
> the compatible property of that top level device. So to avoid regressions
> some additional changes would be needed, outside of this particular I2C
> client driver. I guess this could be avoided by better design of the
> exynos4-is driver right from the beginning. But it's all some times tricky
> when there is some many IP blocks involved and the hardware behaviour/device
> interactions are not always well documented.

OK.

I'm actually thinking that it is probably better now if we don't touch the
client runtime PM at all in the I2C core.

I proposed a less intrusive solution in this same thread where we power the
I2C controller briefly at the client ->probe() (In order to have all the
ACPI power resources etc. and the controller on) and let the client driver
handle their own runtime PM as they do currently.

Do you think that would work better wrt. fimc-isp-i2c driver?

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sun, Sep 15, 2013 at 04:28:24PM +0300, Mika Westerberg wrote:
> On Sun, Sep 15, 2013 at 01:47:44PM +0100, Mark Brown wrote:
> > On Sun, Sep 15, 2013 at 09:41:39AM +0300, Mika Westerberg wrote:

> > > 	1. In I2C core i2c_device_probe() we power on the I2C controller
> > > 	and attach the client device to the ACPI power domain. Just like in
> > > 	this patch but we don't touch the I2C client device runtime PM.

> > > 	-> This should allow the existing drivers to keep using whatever
> > > 	runtime PM strategy they have chosen.

> > There should be no explicit need to power on the I2C controller if it's
> > implemented the same way the existing ones are - just have it power
> > itself on when it is doing a transfer.

> The problem is that the ACPI child device can't be in higher power state
> than its parent (and this is also what the runtime PM expects). If we don't
> power the I2C controller device before we power on the I2C client device
> that rule is violated and we get an complaint to the console.

That's definitely an ACPI specific (probably x86 specific ACPI?)
requirement not a generic one, on some systems it would increase power
consumption since the controller will need to sit on while the device is
functioning autonomously.  Even though the controller power consumption
is going to be minimal the power domain it is in may be relatively
large.  Can't the power domains for ACPI deal with this requirement, for
example by making the I2C slave power domains children of the controller
power domain?

On Mon, Sep 16, 2013 at 11:12:49AM +0100, Mark Brown wrote:
> That's definitely an ACPI specific (probably x86 specific ACPI?)
> requirement not a generic one, on some systems it would increase power
> consumption since the controller will need to sit on while the device is
> functioning autonomously.

Yes, the ACPI 5.0 spec says that the device cannot be in higher D-state
than its parent. This is not x86 specific, though I'm not sure if this is
implemented elsewhere.

> Even though the controller power consumption is going to be minimal the
> power domain it is in may be relatively large.  Can't the power domains
> for ACPI deal with this requirement, for example by making the I2C slave
> power domains children of the controller power domain?

We'll look into this. Thanks for the suggestion.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Sep 16, 2013 at 05:38:12PM +0300, Mika Westerberg wrote:
> On Mon, Sep 16, 2013 at 11:12:49AM +0100, Mark Brown wrote:
> > That's definitely an ACPI specific (probably x86 specific ACPI?)
> > requirement not a generic one, on some systems it would increase power
> > consumption since the controller will need to sit on while the device is
> > functioning autonomously.
> 
> Yes, the ACPI 5.0 spec says that the device cannot be in higher D-state
> than its parent. This is not x86 specific, though I'm not sure if this is
> implemented elsewhere.
> 
I do not think this stops the OS fine controlling the power of the device
though. It is only a mechanism to make sure the tree of D states is vaguely
sane from the ACPI point of view. What happens in each D state is never
actually defined in the ACPI spec.

Graeme

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Sep 16, 2013 at 03:46:16PM +0100, Graeme Gregory wrote:
> On Mon, Sep 16, 2013 at 05:38:12PM +0300, Mika Westerberg wrote:
> > On Mon, Sep 16, 2013 at 11:12:49AM +0100, Mark Brown wrote:
> > > That's definitely an ACPI specific (probably x86 specific ACPI?)
> > > requirement not a generic one, on some systems it would increase power
> > > consumption since the controller will need to sit on while the device is
> > > functioning autonomously.
> > 
> > Yes, the ACPI 5.0 spec says that the device cannot be in higher D-state
> > than its parent. This is not x86 specific, though I'm not sure if this is
> > implemented elsewhere.
> > 
> I do not think this stops the OS fine controlling the power of the device
> though. It is only a mechanism to make sure the tree of D states is vaguely
> sane from the ACPI point of view. What happens in each D state is never
> actually defined in the ACPI spec.

I think there's a pretty good definition of the D-states in chapter 2.3 of
the ACPI 5.0 spec.

In our case the problem is that the I2C controller is in D3Cold (off) and
we try to move the I2C client device to D0 (on) it violates the ACPI spec.

Anyway we are looking if we can somehow make this work in such way that it
doesn't prevent non-ACPI devices from functioning as they expect now.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Monday, September 16, 2013 11:47:08 AM Mika Westerberg wrote:
> On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:
> > On 09/13/2013 05:40 PM, Mika Westerberg wrote:
> > [...]
> > >>>The call to pm_runtime_get_noresume() should make sure that the device is
> > >>>in active state (at least in state where it can access the bus) if I'm
> > >>>understanding this right.
> > >>
> > >>I can't see how this would happen. How runtime_resume/runtime_suspend
> > >>callbacks would get invoked with this code, if, e.g. originally driver called
> > >>pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
> > >
> > >The driver callbacks are not called but if the device has been attached to
> > >a power domain (like we do with ACPI) the power domain callbacks get called
> > >and it brings the "bus" to such state that we are able to access the
> > >device. That also was the reason I used _noresume() but didn't look too
> > >close the implementation.
> > 
> > OK, but if a client driver assumes default inactive power state it
> > will expect
> > its callbacks to get called. Otherwise exisiting code might break. So, e.g.
> > in case of s5p-tv it would rather need to be something like:
> > 
> > 	pm_runtime_put()
> > 
> > 	pm_runtime_get_sync()
> >  	sii9234_verify_version()
> >  	pm_runtime_put(dev)
> 
> Yes, or even call directly its runtime_resume callback to bring the device
> to the active state.
> 
> > >>pm_runtime_get_noresume() merely increments usage counter of a device.
> > >>It seems that these changes will break the s5p-tv driver. I might be missing
> > >>something though.
> > >
> > >You are right and Kevin also mentioned this. It should be pm_runtime_get(),
> > >if I'm not mistaken.
> > 
> > Note that client drivers usually call pm_runtime_enable() only when
> > it is safe
> > to call their driver's runtime PM callbacks. By enabling runtime PM
> > before the
> > client's driver has completely initialized we may risk that the
> > callbacks are
> > executed with uninitialized data, if I understand things correctly.
> 
> I think that calling pm_runtime_enable() on behalf of the client driver
> shouldn't cause any problems. There is no PM events queued for that device
> yet (and we have prevented that from happening when we called
> _get_noresume() for the device).

That only is the case if the device in RPM_ACTIVE when we enable runtime PM for
it.  If it is RPM_SUSPENDED at that point, it still is possible that the resume
callback will be executed then.

> Only when the client driver calls _put() things start to happen and at that
> phase the runtime PM hooks should be usable.
> 
> > >>As Mark pointed out this is currently unwanted behaviour to runtime PM
> > >>activate a bus controller device manually in the core for when the client's
> > >>probe() is executed, since i2c core will activate the bus controller for when
> > >>transfer is being carried out.
> > >>
> > >>But I can understand this is needed for ACPI and it shouldn't break existing
> > >>drivers, that do runtime PM activate the client device in probe().
> > >
> > >Indeed, we don't want to break anything (and we still need something like
> > >this for ACPI).
> > >
> > >>Now I'm sure this will break power management of the drivers/media/exynos4-is
> > >>driver, due to incorrect power sequence (power domain and clocks handling).
> > >>I'll try to take care of it in separate patch, as I have some patches pending,
> > >>that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
> > >>drivers/media/i2c/s5k6a3.c.
> > >
> > >I missed that code when I converted existing users to this method. Sorry
> > >about that (I can handle that in the next version).
> > >
> > >I quickly looked at it and I don't see anything that could break (once
> > >converted). What it does is this:
> > >
> > >	pm_runtime_no_callbacks(dev);
> > >	pm_runtime_enable(dev);
> > >
> > >changing that to:
> > >
> > >	pm_runtime_no_callbacks(dev);
> > >	pm_runtime_put(dev);
> > >
> > >shouldn't cause problems AFAICT.
> > 
> > Yes, considering this driver in isolation it should be fine.
> > 
> > However, I observed system suspend issues when the I2C bus controller was
> > being activated (which would happen in the I2C core after your changes)
> > before some other driver has initialized.
> > 
> > So to ensure things continue to work the "fimc-isp-i2c" driver would need
> > to be registered after the "exynos4-fimc-is" driver has initialized. Or the
> > "exynos4-fimc-is" would need to call of_platform_populate() to instantiate
> > its all children devices as specified in device tree (see arch/arm/boot/dts/
> > exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
> > the compatible property of that top level device. So to avoid regressions
> > some additional changes would be needed, outside of this particular I2C
> > client driver. I guess this could be avoided by better design of the
> > exynos4-is driver right from the beginning. But it's all some times tricky
> > when there is some many IP blocks involved and the hardware behaviour/device
> > interactions are not always well documented.
> 
> OK.
> 
> I'm actually thinking that it is probably better now if we don't touch the
> client runtime PM at all in the I2C core.
> 
> I proposed a less intrusive solution in this same thread where we power the
> I2C controller briefly at the client ->probe() (In order to have all the
> ACPI power resources etc. and the controller on) and let the client driver
> handle their own runtime PM as they do currently.

I'm not sure if the I2C core needs to power up the controller at the probe time.
That may be left to the client driver altogether.  I mean, if the client wants
the controller to be powered up, it should just call
pm_runtime_get_sync(controller device) at a suitable place (and then do the
corresponding _put when the controller is not necessary anu more) from its
->probe() callback.

Or the core can just check if the device is in the ACPI PM domain and power up
the controller if that's the case.  However, that may not match the case when
the I2C client is not a direct descendant of the controller (it may just use
an I2C resource pointing to the controller via a namespace path).

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote:

> That may be left to the client driver altogether.  I mean, if the client wants
> the controller to be powered up, it should just call
> pm_runtime_get_sync(controller device) at a suitable place (and then do the
> corresponding _put when the controller is not necessary anu more) from its
> ->probe() callback.

It shouldn't even need to do that, it should just be able to rely on the
controller to power itself up when asked to do work.   This is how the
existing implementations are done - the controller power management is
totally transparent to the slave.

On Tuesday, September 17, 2013 12:31:11 AM Mark Brown wrote:
> On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote:
> 
> > That may be left to the client driver altogether.  I mean, if the client wants
> > the controller to be powered up, it should just call
> > pm_runtime_get_sync(controller device) at a suitable place (and then do the
> > corresponding _put when the controller is not necessary anu more) from its
> > ->probe() callback.
> 
> It shouldn't even need to do that, it should just be able to rely on the
> controller to power itself up when asked to do work.   This is how the
> existing implementations are done - the controller power management is
> totally transparent to the slave.

If both the I2C client and I2C controller have corresponding objects in the
ACPI namespace and the client's object is a child of the controller's object,
then in order to power up the client we need to power up the controller even
if no transactions are going to be carried out.  That's what the spec simply
requires us to do in that case.

Thanks,
Rafael

On Tue, Sep 17, 2013 at 03:25:25AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 17, 2013 12:31:11 AM Mark Brown wrote:

> > It shouldn't even need to do that, it should just be able to rely on the
> > controller to power itself up when asked to do work.   This is how the
> > existing implementations are done - the controller power management is
> > totally transparent to the slave.

> If both the I2C client and I2C controller have corresponding objects in the
> ACPI namespace and the client's object is a child of the controller's object,
> then in order to power up the client we need to power up the controller even
> if no transactions are going to be carried out.  That's what the spec simply
> requires us to do in that case.

Like I said I think this should be handled by the power domains (or
otherwise in the ACPI specific code) - we shouldn't be needing to modify
individual drivers to work around thoughtlessness in the ACPI spec.

On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote:
> On Monday, September 16, 2013 11:47:08 AM Mika Westerberg wrote:
> > On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:
> > > On 09/13/2013 05:40 PM, Mika Westerberg wrote:
> > > [...]
> > > >>>The call to pm_runtime_get_noresume() should make sure that the device is
> > > >>>in active state (at least in state where it can access the bus) if I'm
> > > >>>understanding this right.
> > > >>
> > > >>I can't see how this would happen. How runtime_resume/runtime_suspend
> > > >>callbacks would get invoked with this code, if, e.g. originally driver called
> > > >>pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
> > > >
> > > >The driver callbacks are not called but if the device has been attached to
> > > >a power domain (like we do with ACPI) the power domain callbacks get called
> > > >and it brings the "bus" to such state that we are able to access the
> > > >device. That also was the reason I used _noresume() but didn't look too
> > > >close the implementation.
> > > 
> > > OK, but if a client driver assumes default inactive power state it
> > > will expect
> > > its callbacks to get called. Otherwise exisiting code might break. So, e.g.
> > > in case of s5p-tv it would rather need to be something like:
> > > 
> > > 	pm_runtime_put()
> > > 
> > > 	pm_runtime_get_sync()
> > >  	sii9234_verify_version()
> > >  	pm_runtime_put(dev)
> > 
> > Yes, or even call directly its runtime_resume callback to bring the device
> > to the active state.
> > 
> > > >>pm_runtime_get_noresume() merely increments usage counter of a device.
> > > >>It seems that these changes will break the s5p-tv driver. I might be missing
> > > >>something though.
> > > >
> > > >You are right and Kevin also mentioned this. It should be pm_runtime_get(),
> > > >if I'm not mistaken.
> > > 
> > > Note that client drivers usually call pm_runtime_enable() only when
> > > it is safe
> > > to call their driver's runtime PM callbacks. By enabling runtime PM
> > > before the
> > > client's driver has completely initialized we may risk that the
> > > callbacks are
> > > executed with uninitialized data, if I understand things correctly.
> > 
> > I think that calling pm_runtime_enable() on behalf of the client driver
> > shouldn't cause any problems. There is no PM events queued for that device
> > yet (and we have prevented that from happening when we called
> > _get_noresume() for the device).
> 
> That only is the case if the device in RPM_ACTIVE when we enable runtime PM for
> it.  If it is RPM_SUSPENDED at that point, it still is possible that the resume
> callback will be executed then.

OK, thanks for the clarification.

> > Only when the client driver calls _put() things start to happen and at that
> > phase the runtime PM hooks should be usable.
> > 
> > > >>As Mark pointed out this is currently unwanted behaviour to runtime PM
> > > >>activate a bus controller device manually in the core for when the client's
> > > >>probe() is executed, since i2c core will activate the bus controller for when
> > > >>transfer is being carried out.
> > > >>
> > > >>But I can understand this is needed for ACPI and it shouldn't break existing
> > > >>drivers, that do runtime PM activate the client device in probe().
> > > >
> > > >Indeed, we don't want to break anything (and we still need something like
> > > >this for ACPI).
> > > >
> > > >>Now I'm sure this will break power management of the drivers/media/exynos4-is
> > > >>driver, due to incorrect power sequence (power domain and clocks handling).
> > > >>I'll try to take care of it in separate patch, as I have some patches pending,
> > > >>that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
> > > >>drivers/media/i2c/s5k6a3.c.
> > > >
> > > >I missed that code when I converted existing users to this method. Sorry
> > > >about that (I can handle that in the next version).
> > > >
> > > >I quickly looked at it and I don't see anything that could break (once
> > > >converted). What it does is this:
> > > >
> > > >	pm_runtime_no_callbacks(dev);
> > > >	pm_runtime_enable(dev);
> > > >
> > > >changing that to:
> > > >
> > > >	pm_runtime_no_callbacks(dev);
> > > >	pm_runtime_put(dev);
> > > >
> > > >shouldn't cause problems AFAICT.
> > > 
> > > Yes, considering this driver in isolation it should be fine.
> > > 
> > > However, I observed system suspend issues when the I2C bus controller was
> > > being activated (which would happen in the I2C core after your changes)
> > > before some other driver has initialized.
> > > 
> > > So to ensure things continue to work the "fimc-isp-i2c" driver would need
> > > to be registered after the "exynos4-fimc-is" driver has initialized. Or the
> > > "exynos4-fimc-is" would need to call of_platform_populate() to instantiate
> > > its all children devices as specified in device tree (see arch/arm/boot/dts/
> > > exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
> > > the compatible property of that top level device. So to avoid regressions
> > > some additional changes would be needed, outside of this particular I2C
> > > client driver. I guess this could be avoided by better design of the
> > > exynos4-is driver right from the beginning. But it's all some times tricky
> > > when there is some many IP blocks involved and the hardware behaviour/device
> > > interactions are not always well documented.
> > 
> > OK.
> > 
> > I'm actually thinking that it is probably better now if we don't touch the
> > client runtime PM at all in the I2C core.
> > 
> > I proposed a less intrusive solution in this same thread where we power the
> > I2C controller briefly at the client ->probe() (In order to have all the
> > ACPI power resources etc. and the controller on) and let the client driver
> > handle their own runtime PM as they do currently.
> 
> I'm not sure if the I2C core needs to power up the controller at the probe time.
> That may be left to the client driver altogether.  I mean, if the client wants
> the controller to be powered up, it should just call
> pm_runtime_get_sync(controller device) at a suitable place (and then do the
> corresponding _put when the controller is not necessary anu more) from its
> ->probe() callback.
> 
> Or the core can just check if the device is in the ACPI PM domain and power up
> the controller if that's the case.  However, that may not match the case when
> the I2C client is not a direct descendant of the controller (it may just use
> an I2C resource pointing to the controller via a namespace path).

I sure hope we don't have to deal with such devices.

What if we make runtime PM enabled for the I2C adapter device only in case
of ACPI enumerated devices? That way runtime PM itself keeps the adapter
powered on when it has any active kids so we don't violate the ACPI spec,
and still let non-ACPI systems to use I2C as they do today.

Then we could do something like:

static int i2c_device_probe(struct device *dev)
{
	...
	/*
	 * Make sure that the adapter is powered on when the client is
	 * probed.
	 *
	 * Note that this is no-op on non-ACPI systems as runtime PM for
	 * the adapter is not enabled.
	 */
	pm_runtime_get_sync(&client->adapter->dev);
        acpi_dev_pm_attach(&client->dev, true);

        status = driver->probe(client, i2c_match_id(driver->id_table, client));
	if (status) {
		...

and enable runtime PM only when we find that there are ACPI I2C devices
behind the controller and they are power manageable.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 09/16/2013 10:47 AM, Mika Westerberg wrote:
> I'm actually thinking that it is probably better now if we don't touch the
> client runtime PM at all in the I2C core.
> 
> I proposed a less intrusive solution in this same thread where we power the
> I2C controller briefly at the client ->probe() (In order to have all the
> ACPI power resources etc. and the controller on) and let the client driver
> handle their own runtime PM as they do currently.
> 
> Do you think that would work better wrt. fimc-isp-i2c driver?

That would be no different for this particular driver, as long as the 
I2C bus controller is activated right before the I2C client's probe().
 
In general I would expect such additional device activation not to be 
harmful. For that particular driver I'm going to prepare patches to 
ensure that the I2C bus controller device and its driver is registered 
only when a driver it depends on has initialized. This should have been 
ensured right from the beginning. So don't need to worry about this 
particular case. I'm just not sure what other devices could be similarly
affected.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday, September 17, 2013 02:00:22 PM Mika Westerberg wrote:
> On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote:
> > On Monday, September 16, 2013 11:47:08 AM Mika Westerberg wrote:
> > > On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:
> > > > On 09/13/2013 05:40 PM, Mika Westerberg wrote:
> > > > [...]
> > > > >>>The call to pm_runtime_get_noresume() should make sure that the device is
> > > > >>>in active state (at least in state where it can access the bus) if I'm
> > > > >>>understanding this right.
> > > > >>
> > > > >>I can't see how this would happen. How runtime_resume/runtime_suspend
> > > > >>callbacks would get invoked with this code, if, e.g. originally driver called
> > > > >>pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
> > > > >
> > > > >The driver callbacks are not called but if the device has been attached to
> > > > >a power domain (like we do with ACPI) the power domain callbacks get called
> > > > >and it brings the "bus" to such state that we are able to access the
> > > > >device. That also was the reason I used _noresume() but didn't look too
> > > > >close the implementation.
> > > > 
> > > > OK, but if a client driver assumes default inactive power state it
> > > > will expect
> > > > its callbacks to get called. Otherwise exisiting code might break. So, e.g.
> > > > in case of s5p-tv it would rather need to be something like:
> > > > 
> > > > 	pm_runtime_put()
> > > > 
> > > > 	pm_runtime_get_sync()
> > > >  	sii9234_verify_version()
> > > >  	pm_runtime_put(dev)
> > > 
> > > Yes, or even call directly its runtime_resume callback to bring the device
> > > to the active state.
> > > 
> > > > >>pm_runtime_get_noresume() merely increments usage counter of a device.
> > > > >>It seems that these changes will break the s5p-tv driver. I might be missing
> > > > >>something though.
> > > > >
> > > > >You are right and Kevin also mentioned this. It should be pm_runtime_get(),
> > > > >if I'm not mistaken.
> > > > 
> > > > Note that client drivers usually call pm_runtime_enable() only when
> > > > it is safe
> > > > to call their driver's runtime PM callbacks. By enabling runtime PM
> > > > before the
> > > > client's driver has completely initialized we may risk that the
> > > > callbacks are
> > > > executed with uninitialized data, if I understand things correctly.
> > > 
> > > I think that calling pm_runtime_enable() on behalf of the client driver
> > > shouldn't cause any problems. There is no PM events queued for that device
> > > yet (and we have prevented that from happening when we called
> > > _get_noresume() for the device).
> > 
> > That only is the case if the device in RPM_ACTIVE when we enable runtime PM for
> > it.  If it is RPM_SUSPENDED at that point, it still is possible that the resume
> > callback will be executed then.
> 
> OK, thanks for the clarification.
> 
> > > Only when the client driver calls _put() things start to happen and at that
> > > phase the runtime PM hooks should be usable.
> > > 
> > > > >>As Mark pointed out this is currently unwanted behaviour to runtime PM
> > > > >>activate a bus controller device manually in the core for when the client's
> > > > >>probe() is executed, since i2c core will activate the bus controller for when
> > > > >>transfer is being carried out.
> > > > >>
> > > > >>But I can understand this is needed for ACPI and it shouldn't break existing
> > > > >>drivers, that do runtime PM activate the client device in probe().
> > > > >
> > > > >Indeed, we don't want to break anything (and we still need something like
> > > > >this for ACPI).
> > > > >
> > > > >>Now I'm sure this will break power management of the drivers/media/exynos4-is
> > > > >>driver, due to incorrect power sequence (power domain and clocks handling).
> > > > >>I'll try to take care of it in separate patch, as I have some patches pending,
> > > > >>that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
> > > > >>drivers/media/i2c/s5k6a3.c.
> > > > >
> > > > >I missed that code when I converted existing users to this method. Sorry
> > > > >about that (I can handle that in the next version).
> > > > >
> > > > >I quickly looked at it and I don't see anything that could break (once
> > > > >converted). What it does is this:
> > > > >
> > > > >	pm_runtime_no_callbacks(dev);
> > > > >	pm_runtime_enable(dev);
> > > > >
> > > > >changing that to:
> > > > >
> > > > >	pm_runtime_no_callbacks(dev);
> > > > >	pm_runtime_put(dev);
> > > > >
> > > > >shouldn't cause problems AFAICT.
> > > > 
> > > > Yes, considering this driver in isolation it should be fine.
> > > > 
> > > > However, I observed system suspend issues when the I2C bus controller was
> > > > being activated (which would happen in the I2C core after your changes)
> > > > before some other driver has initialized.
> > > > 
> > > > So to ensure things continue to work the "fimc-isp-i2c" driver would need
> > > > to be registered after the "exynos4-fimc-is" driver has initialized. Or the
> > > > "exynos4-fimc-is" would need to call of_platform_populate() to instantiate
> > > > its all children devices as specified in device tree (see arch/arm/boot/dts/
> > > > exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
> > > > the compatible property of that top level device. So to avoid regressions
> > > > some additional changes would be needed, outside of this particular I2C
> > > > client driver. I guess this could be avoided by better design of the
> > > > exynos4-is driver right from the beginning. But it's all some times tricky
> > > > when there is some many IP blocks involved and the hardware behaviour/device
> > > > interactions are not always well documented.
> > > 
> > > OK.
> > > 
> > > I'm actually thinking that it is probably better now if we don't touch the
> > > client runtime PM at all in the I2C core.
> > > 
> > > I proposed a less intrusive solution in this same thread where we power the
> > > I2C controller briefly at the client ->probe() (In order to have all the
> > > ACPI power resources etc. and the controller on) and let the client driver
> > > handle their own runtime PM as they do currently.
> > 
> > I'm not sure if the I2C core needs to power up the controller at the probe time.
> > That may be left to the client driver altogether.  I mean, if the client wants
> > the controller to be powered up, it should just call
> > pm_runtime_get_sync(controller device) at a suitable place (and then do the
> > corresponding _put when the controller is not necessary anu more) from its
> > ->probe() callback.
> > 
> > Or the core can just check if the device is in the ACPI PM domain and power up
> > the controller if that's the case.  However, that may not match the case when
> > the I2C client is not a direct descendant of the controller (it may just use
> > an I2C resource pointing to the controller via a namespace path).
> 
> I sure hope we don't have to deal with such devices.
> 
> What if we make runtime PM enabled for the I2C adapter device only in case
> of ACPI enumerated devices? That way runtime PM itself keeps the adapter
> powered on when it has any active kids so we don't violate the ACPI spec,
> and still let non-ACPI systems to use I2C as they do today.
> 
> Then we could do something like:
> 
> static int i2c_device_probe(struct device *dev)
> {
> 	...
> 	/*
> 	 * Make sure that the adapter is powered on when the client is
> 	 * probed.
> 	 *
> 	 * Note that this is no-op on non-ACPI systems as runtime PM for
> 	 * the adapter is not enabled.
> 	 */
> 	pm_runtime_get_sync(&client->adapter->dev);
>         acpi_dev_pm_attach(&client->dev, true);

I would make the code indicate the ACPI special case, like:

	if (ACPI_HANDLE(&client->dev)) {
		/* Power up the controller, if necessary, to follow the ACPI spec. */
		pm_runtime_get_sync(&client->adapter->dev);
		acpi_dev_pm_attach(&client->dev, true);
	}

> 
>         status = driver->probe(client, i2c_match_id(driver->id_table, client));
> 	if (status) {
> 		...
> 

and of course you need to do the corresponding pm_runtime_put() for the
controller somewhere.

And because ACPI_HANDLE() is simply false for CONFIG_ACPI unset, that would
cover that case too.

> and enable runtime PM only when we find that there are ACPI I2C devices
> behind the controller and they are power manageable.

We need to power up the controller regardless of whether or not the child
devices are power manageable.  If a client device we want to access has an
ACPI handle, the controller has to be in D0 at that point.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Sep 17, 2013 at 01:07:37PM +0200, Sylwester Nawrocki wrote:
> On 09/16/2013 10:47 AM, Mika Westerberg wrote:
> > I'm actually thinking that it is probably better now if we don't touch the
> > client runtime PM at all in the I2C core.
> > 
> > I proposed a less intrusive solution in this same thread where we power the
> > I2C controller briefly at the client ->probe() (In order to have all the
> > ACPI power resources etc. and the controller on) and let the client driver
> > handle their own runtime PM as they do currently.
> > 
> > Do you think that would work better wrt. fimc-isp-i2c driver?
> 
> That would be no different for this particular driver, as long as the 
> I2C bus controller is activated right before the I2C client's probe().

Rafael pointed out that we can use ->ignore_children for the I2C adapter
device for everything else than ACPI devices. That should keep the existing
behavior.

For ACPI devices we don't set that flag and the runtime PM core will
activate the adapter whenever we try to power on I2C client device. This
will make ACPI happy as well.

If there are no objections, I'm going to send a new version based on the
above.

Thanks everyone for valuable input :)
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html