[PATCH V2] gpio: of: make example syntactically correct

[Wolfram Sang]

The ';' was missing. And cosmetic: there was a space too much.

Signed-off-by: Wolfram Sang 
---

Changes since V1:
* removed extra spaces (Thanks Sergei!)

 Documentation/devicetree/bindings/gpio/gpio.txt | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt 
b/Documentation/devicetree/bindings/gpio/gpio.txt
index 802402f6cc5d89..bc5fe95d33fff0 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -33,12 +33,12 @@ The following example could be used to describe GPIO pins 
used as device enable
 and bit-banged data signals:
 
gpio1: gpio1 {
-   gpio-controller
-#gpio-cells = <2>;
+   gpio-controller;
+   #gpio-cells = <2>;
};
gpio2: gpio2 {
-   gpio-controller
-#gpio-cells = <1>;
+   gpio-controller;
+   #gpio-cells = <1>;
};
[...]
 
-- 
2.11.0

Re: [PATCH] drm: rcar-du: lvds: fix LVDCR1 for R-Car gen3

[Sergei Shtylyov]

Hello!

On 12/23/2017 04:09 PM, Laurent Pinchart wrote:


On Thursday, 21 December 2017 22:23:30 EET Sergei Shtylyov wrote:

The LVDCR1 register for the R-Car gen3 SoCs was documented as having the
layout different from the gen2 SoCs in  the early R-Car gen3 manuals but
since v0.52 the LVDCR1 layout is described as being the same as on the gen2
SoCs; the old CHn control values are said to be prohibited now (and there
seems to be no valid output signal when they are used).


Could it be that the "Gen3" values are specific to the H3 ES1.x ? I've got the


   I don't know. All I know is that the LVDCR1 description was changed in 
v.52 of the gen3 manual (the 1st version that documented R-Car V3M).



LVDS output working on a Salvator-X H3 board with the existing code, although
I'm not sure at the moment whether that was on ES1.x or ES2.0. I can check
that when I'll get access to the hardware back in a week.

On which platform have you tested this patch ?


   R-Car V3M (R8A77970), V3M Starter Kit board.


Fixes: 6bc2e15cf21c ("drm: rcar-du: lvds: Add R-Car Gen3 support")
Signed-off-by: Sergei Shtylyov 

[...]

MBR, Sergei

Re: [PATCH 1/3] PM / core: Assign the wakeup_path status flag in __device_prepare()

[Ulf Hansson]

[...]

 How many drivers actually do call device_set_wakeup_enable() during 
 suspend?
>>>
>>> There are some ethernet/wifi drivers, although it hard to say how many
>>> without a more thorough investigation.
>>>
>>> Besides those I found these more obvious ones:
>>> drivers/ssb/pcihost_wrapper.c
>>> drivers/staging/rtlwifi/core.c
>>> drivers/tty/serial/atmel_serial.c
>>> drivers/usb/core/hcd-pci.c
>>
>> Ugh.  I need to look at the last one at least ...
>
> The drivers/usb/core/hcd-pci.c instance is actually valid, because it
> calls device_set_wakeup_enable() to remove the wakeup source object in
> case the device is dead.  Moreover, it does that in the "noirq" phase
> which is not covered by your patch anyway.

Right, but this puzzles me a bit.

Could you explain what you mean by valid here? Is it okay to call
device_set_wakeup_enable() in the suspend phase after all? No?

My very vague idea at this point is, if we find cases where
device_set_wakeup_enable() makes sense to call during system suspend,
probably those could/should be replaces by using the "wakeup_path"
flag instead, somehow.

Kind regards
Uffe

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

[...]

>
> So IMO the changes you are proposing make sense regardless of the
> genpd issue, because they generally simplify the phy code, but the
> additional use_runtime_pm field in struct phy represents redundant
> information (manipulating reference counters shouldn't matter if
> runtime PM is disabled), so it doesn't appear to be necessary.
>

Actually, the first version I posted treated the return codes from
pm_runtime_get_sync() according to your suggestion above.

However, Kishon pointed out that it didn't work. That's because, there
are phy provider drivers that enables runtime PM *after* calling
phy_create(). And in those cases, that is because they want to treat
runtime PM themselves.

I think that's probably something we should look into to change, but I
find it being a separate issue, that I didn't want to investigate as
part of this series.

See more about the thread here:
https://www.spinics.net/lists/linux-renesas-soc/msg21711.html

> [On a related note, I'm not sure why phy tries to intercept runtime PM
> errors and "fix up" the reference counters.  That doesn't look right
> to me at all.]
>
> That said, the current phy code is not strictly invalid.  While it
> looks more complicated than necessary, it doesn't do things documented
> as invalid in principle, so saying "The behaviour around the runtime
> PM deployment cause some issues during system suspend" in the
> changelog is describing the problem from a very specific angle.
> Simply put, pm_runtime_force_suspend() and the current phy code cannot
> work together and so using them together is a bug.  None of them
> individually is at fault, but combining them is incorrect.
>
> Fortunately enough, the phy code can be modified so that it can be
> used with pm_runtime_force_suspend() without problems, but picturing
> it as "problematic", because it cannot do that today is not entirely
> fair IMO.

Right, this makes sense. Let me clarify this in the changelog.

Kind regards
Uffe

Re: [PATCH] drm: rcar-du: lvds: fix LVDCR1 for R-Car gen3

[Laurent Pinchart]

Hi Sergei,

Thank you for the patch.

On Thursday, 21 December 2017 22:23:30 EET Sergei Shtylyov wrote:
> The LVDCR1 register for the R-Car gen3 SoCs was documented as having the
> layout different from the gen2 SoCs in  the early R-Car gen3 manuals but
> since v0.52 the LVDCR1 layout is described as being the same as on the gen2
> SoCs; the old CHn control values are said to be prohibited now (and there
> seems to be no valid output signal when they are used).

Could it be that the "Gen3" values are specific to the H3 ES1.x ? I've got the 
LVDS output working on a Salvator-X H3 board with the existing code, although 
I'm not sure at the moment whether that was on ES1.x or ES2.0. I can check 
that when I'll get access to the hardware back in a week.

On which platform have you tested this patch ?

> Fixes: 6bc2e15cf21c ("drm: rcar-du: lvds: Add R-Car Gen3 support")
> Signed-off-by: Sergei Shtylyov 
> 
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |   10 --
>  drivers/gpu/drm/rcar-du/rcar_lvds_regs.h  |6 ++
>  2 files changed, 6 insertions(+), 10 deletions(-)
> 
> Index: linux/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> ===
> --- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> +++ linux/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> @@ -70,9 +70,8 @@ static void rcar_du_lvdsenc_start_gen2(s
> 
>   /* Turn all the channels on. */
>   rcar_lvds_write(lvds, LVDCR1,
> - LVDCR1_CHSTBY_GEN2(3) | LVDCR1_CHSTBY_GEN2(2) |
> - LVDCR1_CHSTBY_GEN2(1) | LVDCR1_CHSTBY_GEN2(0) |
> - LVDCR1_CLKSTBY_GEN2);
> + LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) |
> + LVDCR1_CHSTBY(1) | LVDCR1_CHSTBY(0) | LVDCR1_CLKSTBY);
> 
>   /*
>* Turn the PLL on, wait for the startup delay, and turn the output
> @@ -109,9 +108,8 @@ static void rcar_du_lvdsenc_start_gen3(s
> 
>   /* Turn all the channels on. */
>   rcar_lvds_write(lvds, LVDCR1,
> - LVDCR1_CHSTBY_GEN3(3) | LVDCR1_CHSTBY_GEN3(2) |
> - LVDCR1_CHSTBY_GEN3(1) | LVDCR1_CHSTBY_GEN3(0) |
> - LVDCR1_CLKSTBY_GEN3);
> + LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) |
> + LVDCR1_CHSTBY(1) | LVDCR1_CHSTBY(0) | LVDCR1_CLKSTBY);
> 
>   /*
>* Turn the PLL on, set it to LVDS normal mode, wait for the startup
> Index: linux/drivers/gpu/drm/rcar-du/rcar_lvds_regs.h
> ===
> --- linux.orig/drivers/gpu/drm/rcar-du/rcar_lvds_regs.h
> +++ linux/drivers/gpu/drm/rcar-du/rcar_lvds_regs.h
> @@ -26,10 +26,8 @@
> 
>  #define LVDCR1   0x0004
>  #define LVDCR1_CKSEL (1 << 15)   /* Gen2 only */
> -#define LVDCR1_CHSTBY_GEN2(n)(3 << (2 + (n) * 2))/* Gen2 
> only */
> -#define LVDCR1_CHSTBY_GEN3(n)(1 << (2 + (n) * 2))/* Gen3 
> only */
> -#define LVDCR1_CLKSTBY_GEN2  (3 << 0)/* Gen2 only */
> -#define LVDCR1_CLKSTBY_GEN3  (1 << 0)/* Gen3 only */
> +#define LVDCR1_CHSTBY(n) (3 << (2 + (n) * 2))
> +#define LVDCR1_CLKSTBY   (3 << 0)
> 
>  #define LVDPLLCR 0x0008
>  #define LVDPLLCR_CEEN(1 << 14)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] PM / core: Assign the wakeup_path status flag in __device_prepare()

[Rafael J. Wysocki]

On Sat, Dec 23, 2017 at 1:50 PM, Rafael J. Wysocki  wrote:
> On Sat, Dec 23, 2017 at 1:03 PM, Ulf Hansson  wrote:
>> On 22 December 2017 at 20:12, Rafael J. Wysocki  wrote:
>>> On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote:
 On 21 December 2017 at 02:43, Rafael J. Wysocki  wrote:
 > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson  
 > wrote:
 >> The PM core in the device_prepare() phase, resets the wakeup_path status
 >> flag to the value of device_may_wakeup(). This means if a ->prepare() 
 >> or a
 >> ->suspend() callback for the device would update the device's wakeup
 >> setting, this doesn't become reflected in the wakeup_path status flag.
 >>
 >> In general this isn't a problem, because wakeup settings isn't supposed 
 >> to
 >> be changed during those system suspend phases. Nevertheless, there are a
 >> cases not conforming to that behaviour, as device_set_wakeup_enable() is
 >> indeed called from ->suspend() callbacks.
 >
 > And why is this regarded as correct?

 I am not saying that this behavior is correct. However, I am trying to
 improve the situation, which doesn't hurt or does it?
>>>
>>> Adding a workaround for them kind of encourages new code to do the same
>>> thing, which actually may really hurt.  So I assume that these call sites 
>>> are
>>> all legacy and that's why you don't want to touch them for now, but in that
>>> case the commit message should make it very clear that this is about legacy
>>> only and new code should not call device_set_wakeup_enable() during suspend.
>>
>> Yeah, makes sense. Let me clarify that!
>>
>>>
>>> [Something should be printed to the log if wakeup source objects
>>> are created while system suspend is in progress I guess or similar.]
>>
>> Yes, good idea. Let me cook a patch for that as well.
>>
>>>
 More importantly, the next patch, which is about the wakeup path,
 depends on this.
>>>
>>> Honestly, this sounds like "We have this change we really really would like 
>>> to
>>> make, but there's incorrect code getting in the way, so let's paper over it
>>> and be done."  Not very nice. :-/
>>
>> Yeah it sounds a bit weird, I agree.
>>
>> However, maybe if I update the changelog and fold in a patch printing
>> an error in case the APIs is being abused, that would sort out the
>> confusion?
>
> That should be sufficient IMO.
>
>> Another option is simply to squash patch 1 and patch2, what do you prefer?
>>
>>>
>>> How many drivers actually do call device_set_wakeup_enable() during suspend?
>>
>> There are some ethernet/wifi drivers, although it hard to say how many
>> without a more thorough investigation.
>>
>> Besides those I found these more obvious ones:
>> drivers/ssb/pcihost_wrapper.c
>> drivers/staging/rtlwifi/core.c
>> drivers/tty/serial/atmel_serial.c
>> drivers/usb/core/hcd-pci.c
>
> Ugh.  I need to look at the last one at least ...

The drivers/usb/core/hcd-pci.c instance is actually valid, because it
calls device_set_wakeup_enable() to remove the wakeup source object in
case the device is dead.  Moreover, it does that in the "noirq" phase
which is not covered by your patch anyway.

Thanks,
Rafael

Re: [PATCH 1/3] PM / core: Assign the wakeup_path status flag in __device_prepare()

[Rafael J. Wysocki]

On Sat, Dec 23, 2017 at 1:03 PM, Ulf Hansson  wrote:
> On 22 December 2017 at 20:12, Rafael J. Wysocki  wrote:
>> On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote:
>>> On 21 December 2017 at 02:43, Rafael J. Wysocki  wrote:
>>> > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson  
>>> > wrote:
>>> >> The PM core in the device_prepare() phase, resets the wakeup_path status
>>> >> flag to the value of device_may_wakeup(). This means if a ->prepare() or 
>>> >> a
>>> >> ->suspend() callback for the device would update the device's wakeup
>>> >> setting, this doesn't become reflected in the wakeup_path status flag.
>>> >>
>>> >> In general this isn't a problem, because wakeup settings isn't supposed 
>>> >> to
>>> >> be changed during those system suspend phases. Nevertheless, there are a
>>> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is
>>> >> indeed called from ->suspend() callbacks.
>>> >
>>> > And why is this regarded as correct?
>>>
>>> I am not saying that this behavior is correct. However, I am trying to
>>> improve the situation, which doesn't hurt or does it?
>>
>> Adding a workaround for them kind of encourages new code to do the same
>> thing, which actually may really hurt.  So I assume that these call sites are
>> all legacy and that's why you don't want to touch them for now, but in that
>> case the commit message should make it very clear that this is about legacy
>> only and new code should not call device_set_wakeup_enable() during suspend.
>
> Yeah, makes sense. Let me clarify that!
>
>>
>> [Something should be printed to the log if wakeup source objects
>> are created while system suspend is in progress I guess or similar.]
>
> Yes, good idea. Let me cook a patch for that as well.
>
>>
>>> More importantly, the next patch, which is about the wakeup path,
>>> depends on this.
>>
>> Honestly, this sounds like "We have this change we really really would like 
>> to
>> make, but there's incorrect code getting in the way, so let's paper over it
>> and be done."  Not very nice. :-/
>
> Yeah it sounds a bit weird, I agree.
>
> However, maybe if I update the changelog and fold in a patch printing
> an error in case the APIs is being abused, that would sort out the
> confusion?

That should be sufficient IMO.

> Another option is simply to squash patch 1 and patch2, what do you prefer?
>
>>
>> How many drivers actually do call device_set_wakeup_enable() during suspend?
>
> There are some ethernet/wifi drivers, although it hard to say how many
> without a more thorough investigation.
>
> Besides those I found these more obvious ones:
> drivers/ssb/pcihost_wrapper.c
> drivers/staging/rtlwifi/core.c
> drivers/tty/serial/atmel_serial.c
> drivers/usb/core/hcd-pci.c

Ugh.  I need to look at the last one at least ...

Thanks,
Rafael

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Rafael J. Wysocki]

On Sat, Dec 23, 2017 at 1:37 PM, Ulf Hansson  wrote:
> On 23 December 2017 at 02:35, Rafael J. Wysocki  wrote:
>> On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson  wrote:
>>> On 21 December 2017 at 02:39, Rafael J. Wysocki  wrote:
 On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson  
 wrote:
> The runtime PM deployment in the phy core is deployed using the phy core
> device, which is created by the phy core and assigned as a child device of
> the phy provider device.
>
> The behaviour around the runtime PM deployment cause some issues during
> system suspend, in cases when the phy provider device is put into a low
> power state via a call to the pm_runtime_force_suspend() helper, as is the
> case for a Renesas SoC, which has its phy provider device attached to the
> generic PM domain.
>
> In more detail, the problem in this case is that 
> pm_runtime_force_suspend()
> expects the child device of the provider device, which is the phy core
> device, to be runtime suspended, else a WARN splat will be printed
> (correctly) when runtime PM gets re-enabled at system resume.

 So we are now trying to work around issues with
 pm_runtime_force_suspend().  Lovely. :-/
>>>
>>> Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or
>>> are you saying we should just ignore all issues related to it?
>>
>> Or get rid of it?
>>
>> Seriously, is the resulting pain worth it?
>
> I am not sure what pain you refer to? :-) So far I haven't got much
> errors being reported because of its use, have you?
>
> Moreover, to me, this small series fixes the problems in a rather trivial way.

Well, I agree here (please see the message regarding that I've just
sent in this thread).

>>
>>> Of course, if we had something that could replace
>>> pm_runtime_force_suspend(), that would be great, but there isn't.
>>
>> I beg to differ.
>>
>> At least some of it could be replaced with the driver flags.
>
> Yes, true.
>
> On the other hand that is exactly the problem, only *some*. And more
> importantly, genpd can't use them, because it can't cope with have
> system wide PM callbacks to be skipped.

Its own system-wide PM callbacks cannot be skipped, but it already
skips driver callbacks in some cases. :-)

> In other words, so far, the driver PM flags can't solve this issue.

It is unclear which issue you are talking about, but anyway, yes, they
aren't equivalent to pm_runtime_force_suspend/resume() which is quite
intentional, honestly ...

> In the current scenario, even if a call to phy_power_off() triggers it to
> invoke pm_runtime_put() during system suspend, the phy core device doesn't
> get runtime suspended, because this is prevented in the system suspend
> phases by the PM core.
>
> To solve this problem, let's move the runtime PM deployment from the phy
> core device to the phy provider device, as this provides the similar
> behaviour. Changing this makes it redundant to enable runtime PM for the
> phy core device, so let's avoid doing that.

 I'm not really convinced that this approach is the best one to be honest.

 I'll have a deeper look at this in the next few days, stay tuned.
>>>
>>> There is different ways to solve this, for sure. I picked this one,
>>> because I think it's the most trivial thing to do, and it shouldn't
>>> cause any other problems.
>>>
>>> I think any other option would involve assigning ->suspend|resume()
>>> callbacks to the phy core device, but that's fine too, if you prefer
>>> that.
>>>
>>> Also, I have considered how to deal with wakeup paths for phys,
>>> although I didn't want to post changes as a part of this series, but
>>> maybe I should to give a more complete picture?
>>
>> Yes, you should.
>
> Okay!
>
>>
>> The point is that without genpd using pm_runtime_force_suspend() the
>> phy code could very well stay the way it is.  And it is logical,
>> because having a parent with enabled runtime PM without enabling
>> runtime PM for its children is at least conceptually questionable.
>
> I agree that the phy core is today logical okay. But I think that's
> also the case, if we pick up this small series.
>
> There are many reasons and cases where a children is runtime PM
> disabled, while the parent is runtime PM enabled. Moreover the runtime
> PM core copes fine with that.

Fair enough.

>>
>> So the conclusion may be that the phy code is OK, but calling
>> pm_runtime_force_suspend() from genpd isn't.
>
> So I am worried that changing genpd in this regards, may introduce
> other regressions. @subject series seems far less risky - and again,
> to me, the changes are trivial.
>
> Anyway, I will keep your suggestion in mind and think about, how/if
> genpd can be changed.

Thanks a lot!

Rafael

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Rafael J. Wysocki]

On Thu, Dec 21, 2017 at 2:39 AM, Rafael J. Wysocki  wrote:
> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson  wrote:
>> The runtime PM deployment in the phy core is deployed using the phy core
>> device, which is created by the phy core and assigned as a child device of
>> the phy provider device.
>>
>> The behaviour around the runtime PM deployment cause some issues during
>> system suspend, in cases when the phy provider device is put into a low
>> power state via a call to the pm_runtime_force_suspend() helper, as is the
>> case for a Renesas SoC, which has its phy provider device attached to the
>> generic PM domain.
>>
>> In more detail, the problem in this case is that pm_runtime_force_suspend()
>> expects the child device of the provider device, which is the phy core
>> device, to be runtime suspended, else a WARN splat will be printed
>> (correctly) when runtime PM gets re-enabled at system resume.
>
> So we are now trying to work around issues with
> pm_runtime_force_suspend().  Lovely. :-/
>
>> In the current scenario, even if a call to phy_power_off() triggers it to
>> invoke pm_runtime_put() during system suspend, the phy core device doesn't
>> get runtime suspended, because this is prevented in the system suspend
>> phases by the PM core.
>>
>> To solve this problem, let's move the runtime PM deployment from the phy
>> core device to the phy provider device, as this provides the similar
>> behaviour. Changing this makes it redundant to enable runtime PM for the
>> phy core device, so let's avoid doing that.
>
> I'm not really convinced that this approach is the best one to be honest.
>
> I'll have a deeper look at this in the next few days, stay tuned.

So IMO the changes you are proposing make sense regardless of the
genpd issue, because they generally simplify the phy code, but the
additional use_runtime_pm field in struct phy represents redundant
information (manipulating reference counters shouldn't matter if
runtime PM is disabled), so it doesn't appear to be necessary.

[On a related note, I'm not sure why phy tries to intercept runtime PM
errors and "fix up" the reference counters.  That doesn't look right
to me at all.]

That said, the current phy code is not strictly invalid.  While it
looks more complicated than necessary, it doesn't do things documented
as invalid in principle, so saying "The behaviour around the runtime
PM deployment cause some issues during system suspend" in the
changelog is describing the problem from a very specific angle.
Simply put, pm_runtime_force_suspend() and the current phy code cannot
work together and so using them together is a bug.  None of them
individually is at fault, but combining them is incorrect.

Fortunately enough, the phy code can be modified so that it can be
used with pm_runtime_force_suspend() without problems, but picturing
it as "problematic", because it cannot do that today is not entirely
fair IMO.

Thanks,
Rafael

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

On 23 December 2017 at 02:35, Rafael J. Wysocki  wrote:
> On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson  wrote:
>> On 21 December 2017 at 02:39, Rafael J. Wysocki  wrote:
>>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson  wrote:
 The runtime PM deployment in the phy core is deployed using the phy core
 device, which is created by the phy core and assigned as a child device of
 the phy provider device.

 The behaviour around the runtime PM deployment cause some issues during
 system suspend, in cases when the phy provider device is put into a low
 power state via a call to the pm_runtime_force_suspend() helper, as is the
 case for a Renesas SoC, which has its phy provider device attached to the
 generic PM domain.

 In more detail, the problem in this case is that pm_runtime_force_suspend()
 expects the child device of the provider device, which is the phy core
 device, to be runtime suspended, else a WARN splat will be printed
 (correctly) when runtime PM gets re-enabled at system resume.
>>>
>>> So we are now trying to work around issues with
>>> pm_runtime_force_suspend().  Lovely. :-/
>>
>> Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or
>> are you saying we should just ignore all issues related to it?
>
> Or get rid of it?
>
> Seriously, is the resulting pain worth it?

I am not sure what pain you refer to? :-) So far I haven't got much
errors being reported because of its use, have you?

Moreover, to me, this small series fixes the problems in a rather trivial way.

>
>> Of course, if we had something that could replace
>> pm_runtime_force_suspend(), that would be great, but there isn't.
>
> I beg to differ.
>
> At least some of it could be replaced with the driver flags.

Yes, true.

On the other hand that is exactly the problem, only *some*. And more
importantly, genpd can't use them, because it can't cope with have
system wide PM callbacks to be skipped.

In other words, so far, the driver PM flags can't solve this issue.

>
 In the current scenario, even if a call to phy_power_off() triggers it to
 invoke pm_runtime_put() during system suspend, the phy core device doesn't
 get runtime suspended, because this is prevented in the system suspend
 phases by the PM core.

 To solve this problem, let's move the runtime PM deployment from the phy
 core device to the phy provider device, as this provides the similar
 behaviour. Changing this makes it redundant to enable runtime PM for the
 phy core device, so let's avoid doing that.
>>>
>>> I'm not really convinced that this approach is the best one to be honest.
>>>
>>> I'll have a deeper look at this in the next few days, stay tuned.
>>
>> There is different ways to solve this, for sure. I picked this one,
>> because I think it's the most trivial thing to do, and it shouldn't
>> cause any other problems.
>>
>> I think any other option would involve assigning ->suspend|resume()
>> callbacks to the phy core device, but that's fine too, if you prefer
>> that.
>>
>> Also, I have considered how to deal with wakeup paths for phys,
>> although I didn't want to post changes as a part of this series, but
>> maybe I should to give a more complete picture?
>
> Yes, you should.

Okay!

>
> The point is that without genpd using pm_runtime_force_suspend() the
> phy code could very well stay the way it is.  And it is logical,
> because having a parent with enabled runtime PM without enabling
> runtime PM for its children is at least conceptually questionable.

I agree that the phy core is today logical okay. But I think that's
also the case, if we pick up this small series.

There are many reasons and cases where a children is runtime PM
disabled, while the parent is runtime PM enabled. Moreover the runtime
PM core copes fine with that.

>
> So the conclusion may be that the phy code is OK, but calling
> pm_runtime_force_suspend() from genpd isn't.

So I am worried that changing genpd in this regards, may introduce
other regressions. @subject series seems far less risky - and again,
to me, the changes are trivial.

Anyway, I will keep your suggestion in mind and think about, how/if
genpd can be changed.

Kind regards
Uffe

Re: [PATCH 1/3] PM / core: Assign the wakeup_path status flag in __device_prepare()

[Ulf Hansson]

On 22 December 2017 at 20:12, Rafael J. Wysocki  wrote:
> On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote:
>> On 21 December 2017 at 02:43, Rafael J. Wysocki  wrote:
>> > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson  
>> > wrote:
>> >> The PM core in the device_prepare() phase, resets the wakeup_path status
>> >> flag to the value of device_may_wakeup(). This means if a ->prepare() or a
>> >> ->suspend() callback for the device would update the device's wakeup
>> >> setting, this doesn't become reflected in the wakeup_path status flag.
>> >>
>> >> In general this isn't a problem, because wakeup settings isn't supposed to
>> >> be changed during those system suspend phases. Nevertheless, there are a
>> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is
>> >> indeed called from ->suspend() callbacks.
>> >
>> > And why is this regarded as correct?
>>
>> I am not saying that this behavior is correct. However, I am trying to
>> improve the situation, which doesn't hurt or does it?
>
> Adding a workaround for them kind of encourages new code to do the same
> thing, which actually may really hurt.  So I assume that these call sites are
> all legacy and that's why you don't want to touch them for now, but in that
> case the commit message should make it very clear that this is about legacy
> only and new code should not call device_set_wakeup_enable() during suspend.

Yeah, makes sense. Let me clarify that!

>
> [Something should be printed to the log if wakeup source objects
> are created while system suspend is in progress I guess or similar.]

Yes, good idea. Let me cook a patch for that as well.

>
>> More importantly, the next patch, which is about the wakeup path,
>> depends on this.
>
> Honestly, this sounds like "We have this change we really really would like to
> make, but there's incorrect code getting in the way, so let's paper over it
> and be done."  Not very nice. :-/

Yeah it sounds a bit weird, I agree.

However, maybe if I update the changelog and fold in a patch printing
an error in case the APIs is being abused, that would sort out the
confusion?

Another option is simply to squash patch 1 and patch2, what do you prefer?

>
> How many drivers actually do call device_set_wakeup_enable() during suspend?

There are some ethernet/wifi drivers, although it hard to say how many
without a more thorough investigation.

Besides those I found these more obvious ones:
drivers/ssb/pcihost_wrapper.c
drivers/staging/rtlwifi/core.c
drivers/tty/serial/atmel_serial.c
drivers/usb/core/hcd-pci.c

Kind regards
Uffe

Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver

[Laurent Pinchart]

Hi Jacopo,

On Friday, 22 December 2017 16:40:16 EET jacopo mondi wrote:
> On Fri, Dec 22, 2017 at 02:03:41PM +0200, Laurent Pinchart wrote:
> > On Thursday, 21 December 2017 18:27:02 EET jacopo mondi wrote:
> >> On Mon, Dec 18, 2017 at 05:28:43PM +0200, Laurent Pinchart wrote:
> >>> On Monday, 18 December 2017 14:25:12 EET jacopo mondi wrote:
>  On Mon, Dec 11, 2017 at 06:15:23PM +0200, Laurent Pinchart wrote:
> >> 
> >> [snip]
> >> 
> >> +/**
> >> + * ceu_soft_reset() - Software reset the CEU interface
> >> + */
> >> +static int ceu_soft_reset(struct ceu_device *ceudev)
> >> +{
> >> +  unsigned int reset_done;
> >> +  unsigned int i;
> >> +
> >> +  ceu_write(ceudev, CEU_CAPSR, CEU_CAPSR_CPKIL);
> >> +
> >> +  reset_done = 0;
> >> +  for (i = 0; i < 1000 && !reset_done; i++) {
> >> +  udelay(1);
> >> +  if (!(ceu_read(ceudev, CEU_CSTSR) & CEU_CSTRST_CPTON))
> >> +  reset_done++;
> >> +  }
> > 
> > How many iterations does this typically require ? Wouldn't a sleep
> > be better than a delay ? As far as I can tell the ceu_soft_reset()
> > function is only called with interrupts disabled (in interrupt
> > context) from ceu_capture() in an error path, and that code should be
> > reworked to make it possible to sleep if a reset takes too long.
>  
>  The HW manual does not provide any indication about absolute timings.
>  I can empirically try and see, but that would just be a hint.
> >>> 
> >>> That's why I asked how many iterations it typically takes :-) A hint
> >>> is enough to start with, preferably on both SH and ARM SoCs.
> >> 
> >> I've seen only 0s when printing out how many cycles it takes to clear
> >> both registers. This means 1usec is enough, therefore I would keep using
> >> udelay here. Also, I would reduce the attempts to 100 here (or even
> >> 10), as if a single one is typically enough, 1000 is definitely an
> >> overkill.
> > 
> > I'd go for 10. This being said, please make sure you run tests where the
> > reset is performed during capture in the middle of a frame, to see if it
> > changes the number of iterations.
> 
> The only way I can think to do this is to stream_on then immediately
> stream_off before we get the frame and thus casue the interface reset.
> Any other idea?

I think we should test reset of the state machine both during vertical 
blanking when it's waiting for a frame, and in the middle of the frame. The 
former should be easy to test by stopping the stream immediately before the 
sensor starts outputting a frame (if you can disable the HSYNC and/or VSYNC 
outputs of the sensor it would ensure that the CEU doesn't start receiving 
data, that could be useful). The latter shouldn't be difficult to test with an 
appropriate delay from the frame end interrupt to the stream stop.

> [snip]
> 
> >> +
> >> +/**
> >> + * ceu_capture() - Trigger start of a capture sequence
> >> + *
> >> + * Return value doesn't reflect the success/failure to queue the
> >> new buffer,
> >> + * but rather the status of the previous capture.
> >> + */
> >> +static int ceu_capture(struct ceu_device *ceudev)
> >> +{
> >> +  struct v4l2_pix_format_mplane *pix = >v4l2_pix;
> >> +  dma_addr_t phys_addr_top;
> >> +  u32 status;
> >> +
> >> +  /* Clean interrupt status and re-enable interrupts */
> >> +  status = ceu_read(ceudev, CEU_CETCR);
> >> +  ceu_write(ceudev, CEU_CEIER,
> >> +ceu_read(ceudev, CEU_CEIER) & ~CEU_CEIER_MASK);
> >> +  ceu_write(ceudev, CEU_CETCR, ~status & CEU_CETCR_MAGIC);
> >> +  ceu_write(ceudev, CEU_CEIER, CEU_CEIER_MASK);
> > 
> > I wonder why there's a need to disable and reenable interrupts here.
>  
>  The original driver clearly said "The hardware is -very- picky about
>  this sequence" and I got scared and nerver touched this.
> >>> 
> >>> How about experimenting to see how the hardware reacts ?
> >> 
> >> Turns out this was not needed at all, both on RZ and SH4. I captured
> >> several images without any issues in both platforms just clearing the
> >> interrupt state without disabling interrutps.
> > 
> > I wonder whether it could cause an issue when interrupts are raised by the
> > hardware at the same time they are cleared by the driver. That's hard to
> > test though.
> > 
> > What happens when an interrupt source is masked by the CEIER register, is
> > it still reported by the status CETCR register (obviously without raising
> > an interrupt to the CPU), or does it not get flagged at all ?
> 
> They get flagged, yes, and right now I'm clearing all of them at the
> beginning of IRQ handler writing ~CEU_CETR_ALL_INT to CETCR.

OK. If they didn't get flagged it would mean that disabling interrupts while 
clearing the flags could have an influence on what 

Re: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[kbuild test robot]

Hi Ulf,

I love your patch! Yet something to improve:

[auto build test ERROR on phy/next]
[also build test ERROR on v4.15-rc4 next-20171222]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Ulf-Hansson/phy-core-Re-work-runtime-PM-deployment-and-fix-an-issue/20171223-170432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git 
next
config: x86_64-acpi-redef (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers//ata/libahci.c: In function 'ahci_rpm_get_port':
>> drivers//ata/libahci.c:239:9: error: implicit declaration of function 
>> 'pm_runtime_get_sync'; did you mean 'ktime_get_ns'? 
>> [-Werror=implicit-function-declaration]
 return pm_runtime_get_sync(ap->dev);
^~~
ktime_get_ns
   drivers//ata/libahci.c: In function 'ahci_rpm_put_port':
>> drivers//ata/libahci.c:251:2: error: implicit declaration of function 
>> 'pm_runtime_put'; did you mean 'of_node_put'? 
>> [-Werror=implicit-function-declaration]
 pm_runtime_put(ap->dev);
 ^~
 of_node_put
   cc1: some warnings being treated as errors

vim +251 drivers//ata/libahci.c

365cfa1e Anton Vorontsov 2010-03-28  228  
bb03c640 Mika Westerberg 2016-02-18  229  /**
bb03c640 Mika Westerberg 2016-02-18  230   *ahci_rpm_get_port - Make sure 
the port is powered on
bb03c640 Mika Westerberg 2016-02-18  231   *@ap: Port to power on
bb03c640 Mika Westerberg 2016-02-18  232   *
bb03c640 Mika Westerberg 2016-02-18  233   *Whenever there is need to 
access the AHCI host registers outside of
bb03c640 Mika Westerberg 2016-02-18  234   *normal execution paths, call 
this function to make sure the host is
bb03c640 Mika Westerberg 2016-02-18  235   *actually powered on.
bb03c640 Mika Westerberg 2016-02-18  236   */
bb03c640 Mika Westerberg 2016-02-18  237  static int ahci_rpm_get_port(struct 
ata_port *ap)
bb03c640 Mika Westerberg 2016-02-18  238  {
bb03c640 Mika Westerberg 2016-02-18 @239return 
pm_runtime_get_sync(ap->dev);
bb03c640 Mika Westerberg 2016-02-18  240  }
bb03c640 Mika Westerberg 2016-02-18  241  
bb03c640 Mika Westerberg 2016-02-18  242  /**
bb03c640 Mika Westerberg 2016-02-18  243   *ahci_rpm_put_port - Undoes 
ahci_rpm_get_port()
bb03c640 Mika Westerberg 2016-02-18  244   *@ap: Port to power down
bb03c640 Mika Westerberg 2016-02-18  245   *
bb03c640 Mika Westerberg 2016-02-18  246   *Undoes ahci_rpm_get_port() and 
possibly powers down the AHCI host
bb03c640 Mika Westerberg 2016-02-18  247   *if it has no more active users.
bb03c640 Mika Westerberg 2016-02-18  248   */
bb03c640 Mika Westerberg 2016-02-18  249  static void ahci_rpm_put_port(struct 
ata_port *ap)
bb03c640 Mika Westerberg 2016-02-18  250  {
bb03c640 Mika Westerberg 2016-02-18 @251pm_runtime_put(ap->dev);
bb03c640 Mika Westerberg 2016-02-18  252  }
bb03c640 Mika Westerberg 2016-02-18  253  

:: The code at line 251 was first introduced by commit
:: bb03c640697155639b2e15e2aaa4c10f60bf0d5e ahci: Add functions to manage 
runtime PM of AHCI ports

:: TO: Mika Westerberg <mika.westerb...@linux.intel.com>
:: CC: Tejun Heo <t...@kernel.org>

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[kbuild test robot]

Hi Ulf,

I love your patch! Yet something to improve:

[auto build test ERROR on phy/next]
[also build test ERROR on v4.15-rc4 next-20171222]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Ulf-Hansson/phy-core-Re-work-runtime-PM-deployment-and-fix-an-issue/20171223-170432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git 
next
config: i386-randconfig-a0-201751 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers//ata/ahci.c: In function 'ahci_init_one':
>> drivers//ata/ahci.c:1761:2: error: implicit declaration of function 
>> 'pm_runtime_put_noidle' [-Werror=implicit-function-declaration]
 pm_runtime_put_noidle(>dev);
 ^
   drivers//ata/ahci.c: In function 'ahci_remove_one':
>> drivers//ata/ahci.c:1767:2: error: implicit declaration of function 
>> 'pm_runtime_get_noresume' [-Werror=implicit-function-declaration]
 pm_runtime_get_noresume(>dev);
 ^
   cc1: some warnings being treated as errors
--
   drivers//ata/libahci.c: In function 'ahci_rpm_get_port':
>> drivers//ata/libahci.c:239:2: error: implicit declaration of function 
>> 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
 return pm_runtime_get_sync(ap->dev);
 ^
   drivers//ata/libahci.c: In function 'ahci_rpm_put_port':
>> drivers//ata/libahci.c:251:2: error: implicit declaration of function 
>> 'pm_runtime_put' [-Werror=implicit-function-declaration]
 pm_runtime_put(ap->dev);
 ^
   cc1: some warnings being treated as errors
--
   drivers//ata/ahci_ceva.c: In function 'ceva_ahci_resume':
>> drivers//ata/ahci_ceva.c:326:2: error: implicit declaration of function 
>> 'pm_runtime_disable' [-Werror=implicit-function-declaration]
 pm_runtime_disable(dev);
 ^
>> drivers//ata/ahci_ceva.c:327:2: error: implicit declaration of function 
>> 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
 pm_runtime_set_active(dev);
 ^
>> drivers//ata/ahci_ceva.c:328:2: error: implicit declaration of function 
>> 'pm_runtime_enable' [-Werror=implicit-function-declaration]
 pm_runtime_enable(dev);
 ^
   cc1: some warnings being treated as errors
--
   drivers//ata/ahci_qoriq.c: In function 'ahci_qoriq_resume':
>> drivers//ata/ahci_qoriq.c:306:2: error: implicit declaration of function 
>> 'pm_runtime_disable' [-Werror=implicit-function-declaration]
 pm_runtime_disable(dev);
 ^
>> drivers//ata/ahci_qoriq.c:307:2: error: implicit declaration of function 
>> 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
 pm_runtime_set_active(dev);
 ^
>> drivers//ata/ahci_qoriq.c:308:2: error: implicit declaration of function 
>> 'pm_runtime_enable' [-Werror=implicit-function-declaration]
 pm_runtime_enable(dev);
 ^
   cc1: some warnings being treated as errors

vim +/pm_runtime_put_noidle +1761 drivers//ata/ahci.c

d243bed32f drivers/ata/ahci.c  Tirumalesh Chalamarla   2016-02-16  1642  
4447d35156 drivers/ata/ahci.c  Tejun Heo   2007-04-17  1643 
/* save initial config */
394d6e535f drivers/ata/ahci.c  Anton Vorontsov 2010-03-03  1644 
ahci_pci_save_initial_config(pdev, hpriv);
^1da177e4c drivers/scsi/ahci.c Linus Torvalds  2005-04-16  1645  
4447d35156 drivers/ata/ahci.c  Tejun Heo   2007-04-17  1646 
/* prepare host */
453d3131ec drivers/ata/ahci.c  Robert Hancock  2010-01-26  1647 
if (hpriv->cap & HOST_CAP_NCQ) {
453d3131ec drivers/ata/ahci.c  Robert Hancock  2010-01-26  1648 
pi.flags |= ATA_FLAG_NCQ;
83f2b9630c drivers/ata/ahci.c  Tejun Heo   2010-03-30  1649 
/*
83f2b9630c drivers/ata/ahci.c  Tejun Heo   2010-03-30  1650 
 * Auto-activate optimization is supposed to be
83f2b9630c drivers/ata/ahci.c  Tejun Heo   2010-03-30  1651 
 * supported on all AHCI controllers indicating NCQ
83f2b9630c drivers/ata/ahci.c  Tejun Heo   2010-03-30  1652 
 * capability, but it seems to be broken on some
83f2b9630c drivers/ata/ahci.c  Tejun Heo   2010-03-30  1653 
 * chipsets including NVIDIAs.
83f2b9630c drivers/ata/ahci.c  Tejun Heo   2010-03-30  1654 
 */
83f2b9630c drivers/ata/ahci.c  Tejun Heo   2010-03-30  1655 
if (!(hpriv->flags & AHCI_HFLAG_NO_FPDMA_AA))
453d3131ec drivers/ata/ahci.c  Robert Hancock  2010-01-26  1656 
pi.flags |= ATA_FLAG_FPDMA_AA;
40fb59e75a drivers/ata/ahci.c  Marc Carino 2