[RESEND PATCH 0/2] rockchip: kevin: Enable edp display

[RESEND PATCH 0/2] rockchip: kevin: Enable edp display

[Jeffy Chen]

Make edp display works on chromebook kevin.


Jeffy Chen (2):
  spi: rockchip: Convert to late and early system PM callbacks
  arm64: dts: rockchip: Enable edp disaplay on kevin

 arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 29 +++++++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi      | 16 +++++++++++++
 drivers/spi/spi-rockchip.c                        |  2 +-
 3 files changed, 46 insertions(+), 1 deletion(-)

-- 
2.11.0

[RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[Jeffy Chen]

Currently we are suspending the spi master in it's ->suspend callback,
which is racy as some other drivers may still want to transmit messages
on the bus(e.g. spi based pwm backlight).

Convert to late and early system PM callbacks to avoid the race.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index fdcf3076681b..ae539c735ea6 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -914,7 +914,7 @@ static int rockchip_spi_runtime_resume(struct device *dev)
 #endif /* CONFIG_PM */
 
 static const struct dev_pm_ops rockchip_spi_pm = {
-	SET_SYSTEM_SLEEP_PM_OPS(rockchip_spi_suspend, rockchip_spi_resume)
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(rockchip_spi_suspend, rockchip_spi_resume)
 	SET_RUNTIME_PM_OPS(rockchip_spi_runtime_suspend,
 			   rockchip_spi_runtime_resume, NULL)
 };
-- 
2.11.0

[RESEND PATCH 2/2] arm64: dts: rockchip: Enable edp disaplay on kevin

[Jeffy Chen]

Add edp panel and enable related nodes on kevin.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 29 +++++++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi      | 16 +++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
index a3d3cea7dc4f..bc67b19f0af5 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
@@ -93,6 +93,18 @@
 		pwm-delay-us = <10000>;
 	};
 
+	edp_panel: edp-panel {
+		compatible = "sharp,lq123p1jx31", "simple-panel";
+		backlight = <&backlight>;
+		power-supply = <&pp3300_disp>;
+
+		ports {
+			panel_in_edp: endpoint {
+				remote-endpoint = <&edp_out_panel>;
+			};
+		};
+	};
+
 	thermistor_ppvar_bigcpu: thermistor-ppvar-bigcpu {
 		compatible = "murata,ncp15wb473";
 		pullup-uv = <1800000>;
@@ -264,6 +276,23 @@ ap_i2c_dig: &i2c2 {
 	};
 };
 
+&edp {
+	status = "okay";
+
+	ports {
+		edp_out: port@1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			edp_out_panel: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&panel_in_edp>;
+			};
+		};
+	};
+};
+
 &ppvar_bigcpu_pwm {
 	regulator-min-microvolt = <798674>;
 	regulator-max-microvolt = <1302172>;
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index 5772c52fbfd3..470105d651c2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -927,6 +927,22 @@ ap_i2c_audio: &i2c8 {
 	dr_mode = "host";
 };
 
+&vopb {
+	status = "okay";
+};
+
+&vopb_mmu {
+	status = "okay";
+};
+
+&vopl {
+	status = "okay";
+};
+
+&vopl_mmu {
+	status = "okay";
+};
+
 #include <arm/cros-ec-keyboard.dtsi>
 #include <arm/cros-ec-sbs.dtsi>
 
-- 
2.11.0

Re: [RESEND PATCH 2/2] arm64: dts: rockchip: Enable edp disaplay on kevin

[Heiko Stuebner]

Hi Jeffy,

Am Freitag, 13. Oktober 2017, 18:41:38 CEST schrieb Jeffy Chen:
> Add edp panel and enable related nodes on kevin.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>

does this actually work with mainline kernel sources?

Sean Paul did look at making the edp work on Chromebooks recently,
but it seemed there were still parts missing to make it actually display
something.


Heiko

> ---
> 
>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 29 +++++++++++++++++++++++
>  arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi      | 16 +++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> index a3d3cea7dc4f..bc67b19f0af5 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> @@ -93,6 +93,18 @@
>  		pwm-delay-us = <10000>;
>  	};
>  
> +	edp_panel: edp-panel {
> +		compatible = "sharp,lq123p1jx31", "simple-panel";
> +		backlight = <&backlight>;
> +		power-supply = <&pp3300_disp>;
> +
> +		ports {
> +			panel_in_edp: endpoint {
> +				remote-endpoint = <&edp_out_panel>;
> +			};
> +		};
> +	};
> +
>  	thermistor_ppvar_bigcpu: thermistor-ppvar-bigcpu {
>  		compatible = "murata,ncp15wb473";
>  		pullup-uv = <1800000>;
> @@ -264,6 +276,23 @@ ap_i2c_dig: &i2c2 {
>  	};
>  };
>  
> +&edp {
> +	status = "okay";
> +
> +	ports {
> +		edp_out: port@1 {
> +			reg = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			edp_out_panel: endpoint@0 {
> +				reg = <0>;
> +				remote-endpoint = <&panel_in_edp>;
> +			};
> +		};
> +	};
> +};
> +
>  &ppvar_bigcpu_pwm {
>  	regulator-min-microvolt = <798674>;
>  	regulator-max-microvolt = <1302172>;
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> index 5772c52fbfd3..470105d651c2 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> @@ -927,6 +927,22 @@ ap_i2c_audio: &i2c8 {
>  	dr_mode = "host";
>  };
>  
> +&vopb {
> +	status = "okay";
> +};
> +
> +&vopb_mmu {
> +	status = "okay";
> +};
> +
> +&vopl {
> +	status = "okay";
> +};
> +
> +&vopl_mmu {
> +	status = "okay";
> +};
> +
>  #include <arm/cros-ec-keyboard.dtsi>
>  #include <arm/cros-ec-sbs.dtsi>
>  
> 

Re: [RESEND PATCH 2/2] arm64: dts: rockchip: Enable edp disaplay on kevin

[Emil Renner Berthing]

On 13 October 2017 at 15:25, Heiko Stuebner <heiko@sntech.de> wrote:
> Am Freitag, 13. Oktober 2017, 18:41:38 CEST schrieb Jeffy Chen:
>> Add edp panel and enable related nodes on kevin.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>
> does this actually work with mainline kernel sources?
>
> Sean Paul did look at making the edp work on Chromebooks recently,
> but it seemed there were still parts missing to make it actually display
> something.

That's funny. I've been using your own similar patch on my Chromebook
Plus for a while now and it works "fine":
https://github.com/esmil/linux/commits/kevin

..where fine means I can't get the armsoc X11 driver to work, but
modesetting works and screenblank seems to freeze the kernel.

I'll test this version of the dts changes as soon as I'm near my
chrombook again.

/Emil

>> ---
>>
>>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 29 +++++++++++++++++++++++
>>  arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi      | 16 +++++++++++++
>>  2 files changed, 45 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>> index a3d3cea7dc4f..bc67b19f0af5 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>> @@ -93,6 +93,18 @@
>>               pwm-delay-us = <10000>;
>>       };
>>
>> +     edp_panel: edp-panel {
>> +             compatible = "sharp,lq123p1jx31", "simple-panel";
>> +             backlight = <&backlight>;
>> +             power-supply = <&pp3300_disp>;
>> +
>> +             ports {
>> +                     panel_in_edp: endpoint {
>> +                             remote-endpoint = <&edp_out_panel>;
>> +                     };
>> +             };
>> +     };
>> +
>>       thermistor_ppvar_bigcpu: thermistor-ppvar-bigcpu {
>>               compatible = "murata,ncp15wb473";
>>               pullup-uv = <1800000>;
>> @@ -264,6 +276,23 @@ ap_i2c_dig: &i2c2 {
>>       };
>>  };
>>
>> +&edp {
>> +     status = "okay";
>> +
>> +     ports {
>> +             edp_out: port@1 {
>> +                     reg = <1>;
>> +                     #address-cells = <1>;
>> +                     #size-cells = <0>;
>> +
>> +                     edp_out_panel: endpoint@0 {
>> +                             reg = <0>;
>> +                             remote-endpoint = <&panel_in_edp>;
>> +                     };
>> +             };
>> +     };
>> +};
>> +
>>  &ppvar_bigcpu_pwm {
>>       regulator-min-microvolt = <798674>;
>>       regulator-max-microvolt = <1302172>;
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>> index 5772c52fbfd3..470105d651c2 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>> @@ -927,6 +927,22 @@ ap_i2c_audio: &i2c8 {
>>       dr_mode = "host";
>>  };
>>
>> +&vopb {
>> +     status = "okay";
>> +};
>> +
>> +&vopb_mmu {
>> +     status = "okay";
>> +};
>> +
>> +&vopl {
>> +     status = "okay";
>> +};
>> +
>> +&vopl_mmu {
>> +     status = "okay";
>> +};
>> +
>>  #include <arm/cros-ec-keyboard.dtsi>
>>  #include <arm/cros-ec-sbs.dtsi>
>>
>>
>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [RESEND PATCH 2/2] arm64: dts: rockchip: Enable edp disaplay on kevin

[Emil Renner Berthing]

On 13 October 2017 at 15:42, Emil Renner Berthing
<emil.renner.berthing@gmail.com> wrote:
> On 13 October 2017 at 15:25, Heiko Stuebner <heiko@sntech.de> wrote:
>> Am Freitag, 13. Oktober 2017, 18:41:38 CEST schrieb Jeffy Chen:
>>> Add edp panel and enable related nodes on kevin.
>>>
>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>>
>> does this actually work with mainline kernel sources?
>>
>> Sean Paul did look at making the edp work on Chromebooks recently,
>> but it seemed there were still parts missing to make it actually display
>> something.
>
> That's funny. I've been using your own similar patch on my Chromebook
> Plus for a while now and it works "fine":
> https://github.com/esmil/linux/commits/kevin
>
> ..where fine means I can't get the armsoc X11 driver to work, but
> modesetting works and screenblank seems to freeze the kernel.
>
> I'll test this version of the dts changes as soon as I'm near my
> chrombook again.

I've just tested this series and the internal screen works with this
too. Screenblank still seems to freeze the kernel though. Eg.
# setterm --powersave on
# setterm --blank 1
#  < wait 1 minute >

But getting output on the screen is progress ;)

As for the spi change suspend now works too, which it didn't before,
but the screen just didn't come up properly after the suspend. I could
still ssh into it after suspend though.

/Emil

>>>
>>>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 29 +++++++++++++++++++++++
>>>  arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi      | 16 +++++++++++++
>>>  2 files changed, 45 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>> index a3d3cea7dc4f..bc67b19f0af5 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>> @@ -93,6 +93,18 @@
>>>               pwm-delay-us = <10000>;
>>>       };
>>>
>>> +     edp_panel: edp-panel {
>>> +             compatible = "sharp,lq123p1jx31", "simple-panel";
>>> +             backlight = <&backlight>;
>>> +             power-supply = <&pp3300_disp>;
>>> +
>>> +             ports {
>>> +                     panel_in_edp: endpoint {
>>> +                             remote-endpoint = <&edp_out_panel>;
>>> +                     };
>>> +             };
>>> +     };
>>> +
>>>       thermistor_ppvar_bigcpu: thermistor-ppvar-bigcpu {
>>>               compatible = "murata,ncp15wb473";
>>>               pullup-uv = <1800000>;
>>> @@ -264,6 +276,23 @@ ap_i2c_dig: &i2c2 {
>>>       };
>>>  };
>>>
>>> +&edp {
>>> +     status = "okay";
>>> +
>>> +     ports {
>>> +             edp_out: port@1 {
>>> +                     reg = <1>;
>>> +                     #address-cells = <1>;
>>> +                     #size-cells = <0>;
>>> +
>>> +                     edp_out_panel: endpoint@0 {
>>> +                             reg = <0>;
>>> +                             remote-endpoint = <&panel_in_edp>;
>>> +                     };
>>> +             };
>>> +     };
>>> +};
>>> +
>>>  &ppvar_bigcpu_pwm {
>>>       regulator-min-microvolt = <798674>;
>>>       regulator-max-microvolt = <1302172>;
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>>> index 5772c52fbfd3..470105d651c2 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>>> @@ -927,6 +927,22 @@ ap_i2c_audio: &i2c8 {
>>>       dr_mode = "host";
>>>  };
>>>
>>> +&vopb {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&vopb_mmu {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&vopl {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&vopl_mmu {
>>> +     status = "okay";
>>> +};
>>> +
>>>  #include <arm/cros-ec-keyboard.dtsi>
>>>  #include <arm/cros-ec-sbs.dtsi>
>>>
>>>
>>
>>
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[Doug Anderson]

Hi,

On Fri, Oct 13, 2017 at 3:41 AM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> Currently we are suspending the spi master in it's ->suspend callback,
> which is racy as some other drivers may still want to transmit messages
> on the bus(e.g. spi based pwm backlight).
>
> Convert to late and early system PM callbacks to avoid the race.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
>  drivers/spi/spi-rockchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

It shouldn't hurt to do this, but I'm curious if you did any digging
about why this happens?  As I understood it suspend order is supposed
to be opposite of probe order.  Thus anything that was able to get a
reference to the cros-ec PWM at its probe time should get suspended
before cros-ec suspends.


-Doug

Re: [RESEND PATCH 0/2] rockchip: kevin: Enable edp display

[Doug Anderson]

Hi,

On Fri, Oct 13, 2017 at 3:41 AM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>
> Make edp display works on chromebook kevin.
>
>
> Jeffy Chen (2):
>   spi: rockchip: Convert to late and early system PM callbacks
>   arm64: dts: rockchip: Enable edp disaplay on kevin
>
>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 29 +++++++++++++++++++++++
>  arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi      | 16 +++++++++++++
>  drivers/spi/spi-rockchip.c                        |  2 +-
>  3 files changed, 46 insertions(+), 1 deletion(-)

Presumably including Sean Paul would be a wise idea on future patches
trying to make eDP work well upstream since he's worked on supporting
rk3399 eDP in the past.  I think Alexandru has also expressed interest
in this area, too.

-Doug

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[Brian Norris]

Hi,

On Fri, Oct 13, 2017 at 08:32:12AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Oct 13, 2017 at 3:41 AM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> > Currently we are suspending the spi master in it's ->suspend callback,
> > which is racy as some other drivers may still want to transmit messages
> > on the bus(e.g. spi based pwm backlight).
> >
> > Convert to late and early system PM callbacks to avoid the race.
> >
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> >
> >  drivers/spi/spi-rockchip.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> It shouldn't hurt to do this, but I'm curious if you did any digging
> about why this happens?  As I understood it suspend order is supposed
> to be opposite of probe order.  Thus anything that was able to get a
> reference to the cros-ec PWM at its probe time should get suspended
> before cros-ec suspends.

Yes, this does seem odd to me too. This looks like an arms race hack
that should be avoided unless we know a legit root cause. Also,
"probe order implies suspend order" doesn't quite work for async suspend
anyway, so we'd probably want to express the dependency properly
anyway.

Any chance this is related? Seems like that might break the parent/child
relationship for master/slave:

commit d7e2ee257038baeb03baef602500368a51ee9eef
Author: Linus Walleij <linus.walleij@linaro.org>
Date:   Mon Apr 11 13:51:03 2016 +0200

    spi: let SPI masters ignore their children for PM

Brian

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[Mark Brown]

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

On Fri, Oct 13, 2017 at 08:51:21AM -0700, Brian Norris wrote:

> Yes, this does seem odd to me too. This looks like an arms race hack
> that should be avoided unless we know a legit root cause. Also,
> "probe order implies suspend order" doesn't quite work for async suspend
> anyway, so we'd probably want to express the dependency properly
> anyway.

Yeah, it's the same stuff as we get with initcall ordering.  This sort
of thing does happen with things like PMICs which tend to have hardware
that the system wants to manipulate in the IRQs off part of suspend.
Ideally the dependency annotation stuff would figure things out though
I'm not sure what the status of that is.

> Any chance this is related? Seems like that might break the parent/child
> relationship for master/slave:

> commit d7e2ee257038baeb03baef602500368a51ee9eef
> Author: Linus Walleij <linus.walleij@linaro.org>
> Date:   Mon Apr 11 13:51:03 2016 +0200

>     spi: let SPI masters ignore their children for PM

That's for runtime PM, I'd not expect it to affect system suspend.

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

Re: [RESEND PATCH 2/2] arm64: dts: rockchip: Enable edp disaplay on kevin

[jeffy]

Hi heiko,

On 10/13/2017 09:25 PM, Heiko Stuebner wrote:
> does this actually work with mainline kernel sources?
>
> Sean Paul did look at making the edp work on Chromebooks recently,
> but it seemed there were still parts missing to make it actually display
> something.

yes, it works, but only for boot-splash(boot animation). we still need 
mali gru stuff to bring chrome browser up...i think caesar tried that 
before(but i don't think he would submit that)
>
>
> Heiko

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[jeffy]

Hi guys,

it looks like the suspend sequence depends on the dt node sequence, and 
we are putting display-subsystem dt node above spi dt node, so it would 
be earlier in the device list, then got suspended later than spi device.

the pwm backlight and cros_ec_spi pwm are very interesting, not only 
about suspend dependency... if we unbind cros_ec_spi pwm, the pwm 
backlight would still hold a reference to it, and crash the kernel later.

On 10/14/2017 12:42 AM, Mark Brown wrote:
> On Fri, Oct 13, 2017 at 08:51:21AM -0700, Brian Norris wrote:
>
>> Yes, this does seem odd to me too. This looks like an arms race hack
>> that should be avoided unless we know a legit root cause. Also,
>> "probe order implies suspend order" doesn't quite work for async suspend
>> anyway, so we'd probably want to express the dependency properly
>> anyway.
>
> Yeah, it's the same stuff as we get with initcall ordering.  This sort
> of thing does happen with things like PMICs which tend to have hardware
> that the system wants to manipulate in the IRQs off part of suspend.
> Ideally the dependency annotation stuff would figure things out though
> I'm not sure what the status of that is.
>
>> Any chance this is related? Seems like that might break the parent/child
>> relationship for master/slave:
>
>> commit d7e2ee257038baeb03baef602500368a51ee9eef
>> Author: Linus Walleij <linus.walleij@linaro.org>
>> Date:   Mon Apr 11 13:51:03 2016 +0200
>
>>      spi: let SPI masters ignore their children for PM
>
> That's for runtime PM, I'd not expect it to affect system suspend.
>

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[jeffy]

On 10/14/2017 02:19 AM, jeffy wrote:
>
>
> it looks like the suspend sequence depends on the dt node sequence, and
> we are putting display-subsystem dt node above spi dt node, so it would
> be earlier in the device list, then got suspended later than spi device.
>
> the pwm backlight and cros_ec_spi pwm are very interesting, not only
> about suspend dependency... if we unbind cros_ec_spi pwm, the pwm
> backlight would still hold a reference to it, and crash the kernel later.

or maybe we should move device_pm_add() from device_add() to driver_bound()?

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[Dmitry Torokhov]

On Sat, Oct 14, 2017 at 02:19:28AM +0800, jeffy wrote:
> Hi guys,
> 
> it looks like the suspend sequence depends on the dt node sequence, and we
> are putting display-subsystem dt node above spi dt node, so it would be
> earlier in the device list, then got suspended later than spi device.

Would it not get a deferral when trying to get resource reference, which
would cause it bumped down to the end of dpm list?

> 
> the pwm backlight and cros_ec_spi pwm are very interesting, not only about
> suspend dependency... if we unbind cros_ec_spi pwm, the pwm backlight would
> still hold a reference to it, and crash the kernel later.

That would be a bug in PWM/cors_ec and it should keep the PWM object
until last reference drops and simply error out on all requests.

> 
> On 10/14/2017 12:42 AM, Mark Brown wrote:
> > On Fri, Oct 13, 2017 at 08:51:21AM -0700, Brian Norris wrote:
> > 
> > > Yes, this does seem odd to me too. This looks like an arms race hack
> > > that should be avoided unless we know a legit root cause. Also,
> > > "probe order implies suspend order" doesn't quite work for async suspend
> > > anyway, so we'd probably want to express the dependency properly
> > > anyway.
> > 
> > Yeah, it's the same stuff as we get with initcall ordering.  This sort
> > of thing does happen with things like PMICs which tend to have hardware
> > that the system wants to manipulate in the IRQs off part of suspend.
> > Ideally the dependency annotation stuff would figure things out though
> > I'm not sure what the status of that is.

I'd say non-existent for resources such as regulators, pwms, clocks,
etc. I do not think many places call device_link_add()... I think adding
this to devm_* APIs might be easiest to get the ball going as they
naturally have consumer device and can easily figure out the supplier
side.

> > 
> > > Any chance this is related? Seems like that might break the parent/child
> > > relationship for master/slave:
> > 
> > > commit d7e2ee257038baeb03baef602500368a51ee9eef
> > > Author: Linus Walleij <linus.walleij@linaro.org>
> > > Date:   Mon Apr 11 13:51:03 2016 +0200
> > 
> > >      spi: let SPI masters ignore their children for PM
> > 
> > That's for runtime PM, I'd not expect it to affect system suspend.
> > 
> 
> 

Thanks.

-- 
Dmitry

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[Dmitry Torokhov]

On Sat, Oct 14, 2017 at 02:25:48AM +0800, jeffy wrote:
> On 10/14/2017 02:19 AM, jeffy wrote:
> > 
> > 
> > it looks like the suspend sequence depends on the dt node sequence, and
> > we are putting display-subsystem dt node above spi dt node, so it would
> > be earlier in the device list, then got suspended later than spi device.
> > 
> > the pwm backlight and cros_ec_spi pwm are very interesting, not only
> > about suspend dependency... if we unbind cros_ec_spi pwm, the pwm
> > backlight would still hold a reference to it, and crash the kernel later.
> 
> or maybe we should move device_pm_add() from device_add() to driver_bound()?

You do not necessarily need to have a driver to power the dveice on and
off.

-- 
Dmitry

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[Mark Brown]

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

On Fri, Oct 13, 2017 at 11:30:06AM -0700, Dmitry Torokhov wrote:
> On Sat, Oct 14, 2017 at 02:19:28AM +0800, jeffy wrote:

> > > Yeah, it's the same stuff as we get with initcall ordering.  This sort
> > > of thing does happen with things like PMICs which tend to have hardware
> > > that the system wants to manipulate in the IRQs off part of suspend.
> > > Ideally the dependency annotation stuff would figure things out though
> > > I'm not sure what the status of that is.

> I'd say non-existent for resources such as regulators, pwms, clocks,
> etc. I do not think many places call device_link_add()... I think adding
> this to devm_* APIs might be easiest to get the ball going as they
> naturally have consumer device and can easily figure out the supplier
> side.

Hrm, are things in a state where we're supposed to be doing that?  I've
not seen anyone even trying which is a bit odd, I'd thought there was
some core work still ongoing (and it wasn't clear to me if we were going
to try to do things like use DT/ACPI cross references to figure this
stuff out).

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

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[jeffy]

Hi Dmitry,

On 10/14/2017 02:30 AM, Dmitry Torokhov wrote:
> On Sat, Oct 14, 2017 at 02:19:28AM +0800, jeffy wrote:
>> Hi guys,
>>
>> it looks like the suspend sequence depends on the dt node sequence, and we
>> are putting display-subsystem dt node above spi dt node, so it would be
>> earlier in the device list, then got suspended later than spi device.
>
> Would it not get a deferral when trying to get resource reference, which
> would cause it bumped down to the end of dpm list?
hmm, right, check again, the rockchip drm would not depend on spi, but 
the edp driver does.

so the drm driver(display-subsystem) would probed before spi, but try to 
control the backlight in the suspend/resume...

so i was wrong in the commit message, will fix it in next version.
>
>>
>> the pwm backlight and cros_ec_spi pwm are very interesting, not only about
>> suspend dependency... if we unbind cros_ec_spi pwm, the pwm backlight would
>> still hold a reference to it, and crash the kernel later.
>
> That would be a bug in PWM/cors_ec and it should keep the PWM object
> until last reference drops and simply error out on all requests.
right, and maybe try to refresh the pwm reference when we bind it again
>
>>
>> On 10/14/2017 12:42 AM, Mark Brown wrote:
>>> On Fri, Oct 13, 2017 at 08:51:21AM -0700, Brian Norris wrote:
>>>
>>>> Yes, this does seem odd to me too. This looks like an arms race hack
>>>> that should be avoided unless we know a legit root cause. Also,
>>>> "probe order implies suspend order" doesn't quite work for async suspend
>>>> anyway, so we'd probably want to express the dependency properly
>>>> anyway.
>>>
>>> Yeah, it's the same stuff as we get with initcall ordering.  This sort
>>> of thing does happen with things like PMICs which tend to have hardware
>>> that the system wants to manipulate in the IRQs off part of suspend.
>>> Ideally the dependency annotation stuff would figure things out though
>>> I'm not sure what the status of that is.
>
> I'd say non-existent for resources such as regulators, pwms, clocks,
> etc. I do not think many places call device_link_add()... I think adding
> this to devm_* APIs might be easiest to get the ball going as they
> naturally have consumer device and can easily figure out the supplier
> side.
>
>>>
>>>> Any chance this is related? Seems like that might break the parent/child
>>>> relationship for master/slave:
>>>
>>>> commit d7e2ee257038baeb03baef602500368a51ee9eef
>>>> Author: Linus Walleij <linus.walleij@linaro.org>
>>>> Date:   Mon Apr 11 13:51:03 2016 +0200
>>>
>>>>       spi: let SPI masters ignore their children for PM
>>>
>>> That's for runtime PM, I'd not expect it to affect system suspend.
>>>
>>
>>
>
> Thanks.
>

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[jeffy]

Hi guys,

so what happens here is:
1/ we put display-subsystem dt node before spi node, which cause 
rockchip drm driver probed before spi(also before edp driver/vop driver...)

2/ rockchip drm driver bound after spi/edp/vop... drivers probed

3/ in rockchip drm driver's resume callback, it would try to enable edp 
panel backlight(through spi), but spi master is still suspended, then we 
got these errors:

1970-01-01T08:02:59.607315+08:00 ERR kernel: [  178.754005] cros-ec-spi 
spi2.0: spi
  transfer failed: -108
1970-01-01T08:02:59.607320+08:00 ERR kernel: [  178.760102] cros-ec-spi 
spi2.0: cs-
deassert spi transfer failed: -108
1970-01-01T08:02:59.607325+08:00 ERR kernel: [  178.767380] cros-ec-spi 
spi2.0: Com
mand xfer error (err:-108)
1970-01-01T08:02:59.607331+08:00 ERR kernel: [  178.773963] cros-ec-spi 
spi2.0: spi
  transfer failed: -108
1970-01-01T08:02:59.607336+08:00 ERR kernel: [  178.780066] cros-ec-spi 
spi2.0: cs-
deassert spi transfer failed: -108
1970-01-01T08:02:59.607341+08:00 ERR kernel: [  178.787359] cros-ec-spi 
spi2.0: Com
mand xfer error (err:-108)

so other than move spi master suspend to late suspend, maybe we could 
defer rockchip drm driver probe after it's component drivers somehow?



On 10/14/2017 02:44 AM, jeffy wrote:
> Hi Dmitry,
>
> On 10/14/2017 02:30 AM, Dmitry Torokhov wrote:
>> On Sat, Oct 14, 2017 at 02:19:28AM +0800, jeffy wrote:
>>> Hi guys,
>>>
>>> it looks like the suspend sequence depends on the dt node sequence,
>>> and we
>>> are putting display-subsystem dt node above spi dt node, so it would be
>>> earlier in the device list, then got suspended later than spi device.
>>
>> Would it not get a deferral when trying to get resource reference, which
>> would cause it bumped down to the end of dpm list?
> hmm, right, check again, the rockchip drm would not depend on spi, but
> the edp driver does.
>
> so the drm driver(display-subsystem) would probed before spi, but try to
> control the backlight in the suspend/resume...
>
> so i was wrong in the commit message, will fix it in next version.
>>
>>>
>>> the pwm backlight and cros_ec_spi pwm are very interesting, not only
>>> about
>>> suspend dependency... if we unbind cros_ec_spi pwm, the pwm backlight
>>> would
>>> still hold a reference to it, and crash the kernel later.
>>
>> That would be a bug in PWM/cors_ec and it should keep the PWM object
>> until last reference drops and simply error out on all requests.
> right, and maybe try to refresh the pwm reference when we bind it again
>>
>>>
>>> On 10/14/2017 12:42 AM, Mark Brown wrote:
>>>> On Fri, Oct 13, 2017 at 08:51:21AM -0700, Brian Norris wrote:
>>>>
>>>>> Yes, this does seem odd to me too. This looks like an arms race hack
>>>>> that should be avoided unless we know a legit root cause. Also,
>>>>> "probe order implies suspend order" doesn't quite work for async
>>>>> suspend
>>>>> anyway, so we'd probably want to express the dependency properly
>>>>> anyway.
>>>>
>>>> Yeah, it's the same stuff as we get with initcall ordering.  This sort
>>>> of thing does happen with things like PMICs which tend to have hardware
>>>> that the system wants to manipulate in the IRQs off part of suspend.
>>>> Ideally the dependency annotation stuff would figure things out though
>>>> I'm not sure what the status of that is.
>>
>> I'd say non-existent for resources such as regulators, pwms, clocks,
>> etc. I do not think many places call device_link_add()... I think adding
>> this to devm_* APIs might be easiest to get the ball going as they
>> naturally have consumer device and can easily figure out the supplier
>> side.
>>
>>>>
>>>>> Any chance this is related? Seems like that might break the
>>>>> parent/child
>>>>> relationship for master/slave:
>>>>
>>>>> commit d7e2ee257038baeb03baef602500368a51ee9eef
>>>>> Author: Linus Walleij <linus.walleij@linaro.org>
>>>>> Date:   Mon Apr 11 13:51:03 2016 +0200
>>>>
>>>>>       spi: let SPI masters ignore their children for PM
>>>>
>>>> That's for runtime PM, I'd not expect it to affect system suspend.
>>>>
>>>
>>>
>>
>> Thanks.
>>
>

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[Dmitry Torokhov]

On Fri, Oct 13, 2017 at 07:37:50PM +0100, Mark Brown wrote:
> On Fri, Oct 13, 2017 at 11:30:06AM -0700, Dmitry Torokhov wrote:
> > On Sat, Oct 14, 2017 at 02:19:28AM +0800, jeffy wrote:
> 
> > > > Yeah, it's the same stuff as we get with initcall ordering.  This sort
> > > > of thing does happen with things like PMICs which tend to have hardware
> > > > that the system wants to manipulate in the IRQs off part of suspend.
> > > > Ideally the dependency annotation stuff would figure things out though
> > > > I'm not sure what the status of that is.
> 
> > I'd say non-existent for resources such as regulators, pwms, clocks,
> > etc. I do not think many places call device_link_add()... I think adding
> > this to devm_* APIs might be easiest to get the ball going as they
> > naturally have consumer device and can easily figure out the supplier
> > side.
> 
> Hrm, are things in a state where we're supposed to be doing that?  I've
> not seen anyone even trying which is a bit odd, I'd thought there was
> some core work still ongoing (and it wasn't clear to me if we were going
> to try to do things like use DT/ACPI cross references to figure this
> stuff out).

Not sure, adding Rafael...

-- 
Dmitry

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[Rafael J. Wysocki]

On Saturday, October 14, 2017 1:45:11 AM CEST Dmitry Torokhov wrote:
> On Fri, Oct 13, 2017 at 07:37:50PM +0100, Mark Brown wrote:
> > On Fri, Oct 13, 2017 at 11:30:06AM -0700, Dmitry Torokhov wrote:
> > > On Sat, Oct 14, 2017 at 02:19:28AM +0800, jeffy wrote:
> > 
> > > > > Yeah, it's the same stuff as we get with initcall ordering.  This sort
> > > > > of thing does happen with things like PMICs which tend to have hardware
> > > > > that the system wants to manipulate in the IRQs off part of suspend.
> > > > > Ideally the dependency annotation stuff would figure things out though
> > > > > I'm not sure what the status of that is.
> > 
> > > I'd say non-existent for resources such as regulators, pwms, clocks,
> > > etc. I do not think many places call device_link_add()... I think adding
> > > this to devm_* APIs might be easiest to get the ball going as they
> > > naturally have consumer device and can easily figure out the supplier
> > > side.
> > 
> > Hrm, are things in a state where we're supposed to be doing that?  I've
> > not seen anyone even trying which is a bit odd, I'd thought there was
> > some core work still ongoing (and it wasn't clear to me if we were going
> > to try to do things like use DT/ACPI cross references to figure this
> > stuff out).
> 
> Not sure, adding Rafael...

The infrastructure in the core is there, the part related to getting that
information from firmware is (mostly) missing.

Still, moving system suspend/resume callbacks to the late/early phases may
be a good idea anyway.

Thanks,
Rafael

Re: [RESEND PATCH 1/2] spi: rockchip: Convert to late and early system PM callbacks

[Mark Brown]

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

On Sat, Oct 14, 2017 at 02:15:34AM +0200, Rafael J. Wysocki wrote:

> The infrastructure in the core is there, the part related to getting that
> information from firmware is (mostly) missing.

Ah, yes - now I remember.  We want this for probe ordering so we don't
really want to be doing it in the subsystems when they request resources
as that's too late (though I guess duplication there won't hurt).  Fun.

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