Re: [PATCH 1/4] exynos4-is: Clear isp-i2c adapter power.ignore_children flag

Re: [PATCH 1/4] exynos4-is: Clear isp-i2c adapter power.ignore_children flag

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2861 bytes --]

On Thu, Sep 01, 2016 at 01:39:16PM +0200, Sylwester Nawrocki wrote:
> Since commit 04f59143b571161d25315dd52d7a2ecc022cb71a
> ("i2c: let I2C masters ignore their children for PM")
> the power.ignore_children flag is set when registering an I2C
> adapter. Since I2C transfers are not managed by the fimc-isp-i2c
> driver its clients use pm_runtime_* calls directly to communicate
> required power state of the bus controller.
> However when the power.ignore_children flag is set that doesn't
> work, so clear that flag back after registering the adapter.
> While at it drop pm_runtime_enable() call on the i2c_adapter
> as it is already done by the I2C subsystem when registering
> I2C adapter.
> 
> Cc: <stable@vger.kernel.org> # 4.7+
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

CCing the authors of the "offending" commit as well as linux-pm for more
PM expertise.

> ---
>  drivers/media/platform/exynos4-is/fimc-is-i2c.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos4-is/fimc-is-i2c.c b/drivers/media/platform/exynos4-is/fimc-is-i2c.c
> index 7521aa5..03b4246 100644
> --- a/drivers/media/platform/exynos4-is/fimc-is-i2c.c
> +++ b/drivers/media/platform/exynos4-is/fimc-is-i2c.c
> @@ -55,26 +55,37 @@ static int fimc_is_i2c_probe(struct platform_device *pdev)
>  	i2c_adap->algo = &fimc_is_i2c_algorithm;
>  	i2c_adap->class = I2C_CLASS_SPD;
>  
> +	platform_set_drvdata(pdev, isp_i2c);
> +	pm_runtime_enable(&pdev->dev);
> +
>  	ret = i2c_add_adapter(i2c_adap);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to add I2C bus %s\n",
>  						node->full_name);
> -		return ret;
> +		goto err_pm_dis;
>  	}
>  
> -	platform_set_drvdata(pdev, isp_i2c);
> -
> -	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_enable(&i2c_adap->dev);
> -
> +	/*
> +	 * Client drivers of this adapter don't do any I2C transfers as that
> +	 * is handled by the ISP firmware.  But we rely on the runtime PM
> +	 * state propagation from the clients up to the adapter driver so
> +	 * clear the ignore_children flags here.  PM rutnime calls are not
> +	 * used in probe() handler of clients of this adapter so there is
> +	 * no issues with clearing the flag right after registering the I2C
> +	 * adapter.
> +	 */
> +	pm_suspend_ignore_children(&i2c_adap->dev, false);
>  	return 0;
> +
> +err_pm_dis:
> +	pm_runtime_disable(&pdev->dev);
> +	return ret;
>  }
>  
>  static int fimc_is_i2c_remove(struct platform_device *pdev)
>  {
>  	struct fimc_is_i2c *isp_i2c = platform_get_drvdata(pdev);
>  
> -	pm_runtime_disable(&isp_i2c->adapter.dev);
>  	pm_runtime_disable(&pdev->dev);
>  	i2c_del_adapter(&isp_i2c->adapter);
>  
> -- 
> 1.9.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/4] exynos4-is: Clear isp-i2c adapter power.ignore_children flag

[Linus Walleij]

On Thu, Sep 1, 2016 at 1:47 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Sep 01, 2016 at 01:39:16PM +0200, Sylwester Nawrocki wrote:

>> Since commit 04f59143b571161d25315dd52d7a2ecc022cb71a
>> ("i2c: let I2C masters ignore their children for PM")
>> the power.ignore_children flag is set when registering an I2C
>> adapter. Since I2C transfers are not managed by the fimc-isp-i2c
>> driver its clients use pm_runtime_* calls directly to communicate
>> required power state of the bus controller.
>> However when the power.ignore_children flag is set that doesn't
>> work, so clear that flag back after registering the adapter.
>> While at it drop pm_runtime_enable() call on the i2c_adapter
>> as it is already done by the I2C subsystem when registering
>> I2C adapter.
>>
>> Cc: <stable@vger.kernel.org> # 4.7+
>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

I understand what the patch is doing but not this commit message.

What does it mean when you say "Since I2C transfers are not
managed by the fimc-isp-i2c driver its clients use pm_runtime_*
calls directly to communicate required power state of the bus
controller."?

I find it very hard to understand.

The intent of the commit is to decouple I2C slave devices'
runtime PM state from their parents, so say a gyroscope on an
I2C bus does not have to bring up it's host controller to be
active, for example it usually has an IRQ line to wake up
the driver and that will talk using I2C and the I2C traffic will
wake up the I2C master.

When I look at the driver it appears it is not even used for
I2C traffic, just to take the clocks up and down and make it
possible to manage a clock using runtime PM and interface
with the DT logic... so I guess since it's likely and odd one-off
and the driver is sufficiently weird anyways, it's fine to merge
this patch making it even weirder.

Maybe a sort of mock adapter type should actually be
created in the I2C core for these things so it can be handled
there but who am I to say.

Yours,
Linus Walleij

Re: [PATCH 1/4] exynos4-is: Clear isp-i2c adapter power.ignore_children flag

[Sylwester Nawrocki]

On 09/07/2016 10:38 PM, Linus Walleij wrote:
> On Thu, Sep 1, 2016 at 1:47 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> On Thu, Sep 01, 2016 at 01:39:16PM +0200, Sylwester Nawrocki wrote:
> 
>>> Since commit 04f59143b571161d25315dd52d7a2ecc022cb71a
>>> ("i2c: let I2C masters ignore their children for PM")
>>> the power.ignore_children flag is set when registering an I2C
>>> adapter. Since I2C transfers are not managed by the fimc-isp-i2c
>>> driver its clients use pm_runtime_* calls directly to communicate
>>> required power state of the bus controller.
>>> However when the power.ignore_children flag is set that doesn't
>>> work, so clear that flag back after registering the adapter.
>>> While at it drop pm_runtime_enable() call on the i2c_adapter
>>> as it is already done by the I2C subsystem when registering
>>> I2C adapter.
>>>
>>> Cc: <stable@vger.kernel.org> # 4.7+
>>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> I understand what the patch is doing but not this commit message.
> 
> What does it mean when you say "Since I2C transfers are not
> managed by the fimc-isp-i2c driver its clients use pm_runtime_*
> calls directly to communicate required power state of the bus
> controller."?
> 
> I find it very hard to understand.

When a video device node is opened the I2C client (v4l2 subdevice)
in its s_power() handler makes pm_runtime_get/pm_runtime_put calls
on its corresponding i2c_client.  It's true we use an imitation of
I2C bus adapter just to make things work with standard DT bindings
and except of device instantiation this adapter's task is just
to control the clock.
I admit that it's all an odd use case, it comes from the fact that
the image sensor devices are handled in the ISP's firmware, where
normally we have an I2C client driver for each sensor in V4L2.

Now, I don't like those pm_runtime_* calls in the sensor drivers
as that's a non standard thing.  The other option to fix the
regression which I have been considering is to remove the whole
PM runtime dependency and make the driver of the I2C controller's
parent device [1] handle the clocks, as it's done with clocks of
some other IP blocks in that whole camera subsystem.  Probably
it's the lesser of two evils.

> The intent of the commit is to decouple I2C slave devices'
> runtime PM state from their parents, so say a gyroscope on an
> I2C bus does not have to bring up it's host controller to be
> active, for example it usually has an IRQ line to wake up
> the driver and that will talk using I2C and the I2C traffic will
> wake up the I2C master.
> 
> When I look at the driver it appears it is not even used for
> I2C traffic, just to take the clocks up and down and make it
> possible to manage a clock using runtime PM and interface
> with the DT logic... so I guess since it's likely and odd one-off
> and the driver is sufficiently weird anyways, it's fine to merge
> this patch making it even weirder.

Yeah, it's an odd one-off, there is no other drivers in mainline
with such a "non standard" split of hardware control responsibility
between main CPU and the ISP's MCU.

I'd merge the $subject patch as a regression fix for 4.7 and 4.8
and then think about a little larger patch moving the clock handling
responsibility the the ISP_I2C controller's parent driver.

> Maybe a sort of mock adapter type should actually be
> created in the I2C core for these things so it can be handled
> there but who am I to say.

Not sure if we need it right now, probably if there is more such
use cases we could think about something like this.

[1] http://lxr.linux.no/#linux+v4.7.3/arch/arm/boot/dts/exynos4x12.dtsi#L148

-- 
Thanks,
Sylwester