[PATCH] arm64: dts: msm8916: Move smem below hwlock

[PATCH] arm64: dts: msm8916: Move smem below hwlock

[Georgi Djakov]

When the SMEM is probed it defers as it depends on the hardware lock, which
is not available yet. But the SMD bus and RPM regulators and clocks depend
on SMEM and they defer too. The problem with this is that the order of
registering the devices is not optimal and also we may end with messed
up serial console as the RPM clocks are not registered yet..

Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 7705207872a5..c497c7b1ae70 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -113,15 +113,6 @@
 		};
 	};
 
-	smem {
-		compatible = "qcom,smem";
-
-		memory-region = <&smem_mem>;
-		qcom,rpm-msg-ram = <&rpm_msg_ram>;
-
-		hwlocks = <&tcsr_mutex 3>;
-	};
-
 	soc: soc {
 		#address-cells = <1>;
 		#size-cells = <1>;
@@ -512,6 +503,16 @@
 		};
 	};
 
+	smem {
+		compatible = "qcom,smem";
+
+		memory-region = <&smem_mem>;
+		qcom,rpm-msg-ram = <&rpm_msg_ram>;
+
+		hwlocks = <&tcsr_mutex 3>;
+	};
+
+
 	smd {
 		compatible = "qcom,smd";
 

Re: [PATCH] arm64: dts: msm8916: Move smem below hwlock

[Srinivas Kandagatla]

On 23/02/16 17:21, Georgi Djakov wrote:
> When the SMEM is probed it defers as it depends on the hardware lock, which
> is not available yet. But the SMD bus and RPM regulators and clocks depend
> on SMEM and they defer too. The problem with this is that the order of
> registering the devices is not optimal and also we may end with messed
> up serial console as the RPM clocks are not registered yet..
I noticed the same issue but was wondering why would we end up with 
messed up serial console?

Could you add more details on why serial console is messed up?

I thought, serial driver has nothing to do with the rpm clocks directly!

--srini
>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/msm8916.dtsi |   19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 7705207872a5..c497c7b1ae70 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -113,15 +113,6 @@
>   		};
>   	};
>
> -	smem {
> -		compatible = "qcom,smem";
> -
> -		memory-region = <&smem_mem>;
> -		qcom,rpm-msg-ram = <&rpm_msg_ram>;
> -
> -		hwlocks = <&tcsr_mutex 3>;
> -	};
> -
>   	soc: soc {
>   		#address-cells = <1>;
>   		#size-cells = <1>;
> @@ -512,6 +503,16 @@
>   		};
>   	};
>
> +	smem {
> +		compatible = "qcom,smem";
> +
> +		memory-region = <&smem_mem>;
> +		qcom,rpm-msg-ram = <&rpm_msg_ram>;
> +
> +		hwlocks = <&tcsr_mutex 3>;
> +	};
> +
> +
>   	smd {
>   		compatible = "qcom,smd";
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] arm64: dts: msm8916: Move smem below hwlock

[Mark Rutland]

On Tue, Feb 23, 2016 at 07:21:11PM +0200, Georgi Djakov wrote:
> When the SMEM is probed it defers as it depends on the hardware lock, which
> is not available yet. But the SMD bus and RPM regulators and clocks depend
> on SMEM and they defer too. The problem with this is that the order of
> registering the devices is not optimal and also we may end with messed
> up serial console as the RPM clocks are not registered yet..

Re-ordering the DT is fragile at best, and not a real solution to this
class of problem.

Why can the drivers not defer probing in this case?

Mark.

> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8916.dtsi |   19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 7705207872a5..c497c7b1ae70 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -113,15 +113,6 @@
>  		};
>  	};
>  
> -	smem {
> -		compatible = "qcom,smem";
> -
> -		memory-region = <&smem_mem>;
> -		qcom,rpm-msg-ram = <&rpm_msg_ram>;
> -
> -		hwlocks = <&tcsr_mutex 3>;
> -	};
> -
>  	soc: soc {
>  		#address-cells = <1>;
>  		#size-cells = <1>;
> @@ -512,6 +503,16 @@
>  		};
>  	};
>  
> +	smem {
> +		compatible = "qcom,smem";
> +
> +		memory-region = <&smem_mem>;
> +		qcom,rpm-msg-ram = <&rpm_msg_ram>;
> +
> +		hwlocks = <&tcsr_mutex 3>;
> +	};
> +
> +
>  	smd {
>  		compatible = "qcom,smd";
>  
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] arm64: dts: msm8916: Move smem below hwlock

[Georgi Djakov]

On 23.02.16 г. 19:29, Srinivas Kandagatla wrote:
> 
> 
> On 23/02/16 17:21, Georgi Djakov wrote:
>> When the SMEM is probed it defers as it depends on the hardware lock, which
>> is not available yet. But the SMD bus and RPM regulators and clocks depend
>> on SMEM and they defer too. The problem with this is that the order of
>> registering the devices is not optimal and also we may end with messed
>> up serial console as the RPM clocks are not registered yet..
> I noticed the same issue but was wondering why would we end up with messed up serial console?
> 
> Could you add more details on why serial console is messed up?
> 
> I thought, serial driver has nothing to do with the rpm clocks directly!
> 

If we don't have the rpm clocks registered, the uart clock is an orphan
and when clk_get_rate() is called on orphan clocks it returns 0 as rate.
In our case the msm_serial driver calls clk_get_rate() and gets 0 rate
as the parent rpm clock has not registered yet. The result is that the
baudrate is set incorrectly.

BR,
Georgi

Re: [PATCH] arm64: dts: msm8916: Move smem below hwlock

[Andy Gross]

On Tue, Feb 23, 2016 at 08:47:56PM +0200, Georgi Djakov wrote:
> On 23.02.16 г. 19:29, Srinivas Kandagatla wrote:
> > 
> > 
> > On 23/02/16 17:21, Georgi Djakov wrote:
> >> When the SMEM is probed it defers as it depends on the hardware lock, which
> >> is not available yet. But the SMD bus and RPM regulators and clocks depend
> >> on SMEM and they defer too. The problem with this is that the order of
> >> registering the devices is not optimal and also we may end with messed
> >> up serial console as the RPM clocks are not registered yet..
> > I noticed the same issue but was wondering why would we end up with messed up serial console?
> > 
> > Could you add more details on why serial console is messed up?
> > 
> > I thought, serial driver has nothing to do with the rpm clocks directly!
> > 
> 
> If we don't have the rpm clocks registered, the uart clock is an orphan
> and when clk_get_rate() is called on orphan clocks it returns 0 as rate.
> In our case the msm_serial driver calls clk_get_rate() and gets 0 rate
> as the parent rpm clock has not registered yet. The result is that the
> baudrate is set incorrectly.

This isn't a probe defer issue w/ the SMEM and hwspinlock.  That works properly.
This is an issue with the msm_serial either not probe deferring to wait for the
rpm clocks or not handling the case of the clk framework giving us a 'bogus'
clock.  Can we queue off the clk_get_rate being 0 to probe defer for the rpm
clocks? (although that is hacky).

Regards,

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

Re: [PATCH] arm64: dts: msm8916: Move smem below hwlock

[Srinivas Kandagatla]

On 23/02/16 18:47, Georgi Djakov wrote:
> On 23.02.16 г. 19:29, Srinivas Kandagatla wrote:
>>
>>
>> On 23/02/16 17:21, Georgi Djakov wrote:
>>> When the SMEM is probed it defers as it depends on the hardware lock, which
>>> is not available yet. But the SMD bus and RPM regulators and clocks depend
>>> on SMEM and they defer too. The problem with this is that the order of
>>> registering the devices is not optimal and also we may end with messed
>>> up serial console as the RPM clocks are not registered yet..
>> I noticed the same issue but was wondering why would we end up with messed up serial console?
>>
>> Could you add more details on why serial console is messed up?
>>
>> I thought, serial driver has nothing to do with the rpm clocks directly!
>>
>
> If we don't have the rpm clocks registered, the uart clock is an orphan
> and when clk_get_rate() is called on orphan clocks it returns 0 as rate.
Shouldn't the actual uart clk provider registration fail/defer probe due 
to missing parent in this case?

--srini
> In our case the msm_serial driver calls clk_get_rate() and gets 0 rate
> as the parent rpm clock has not registered yet. The result is that the
> baudrate is set incorrectly.
>
> BR,
> Georgi
>

Re: [PATCH] arm64: dts: msm8916: Move smem below hwlock

[Georgi Djakov]

On 23.02.16 г. 21:28, Srinivas Kandagatla wrote:
> 
> 
> On 23/02/16 18:47, Georgi Djakov wrote:
>> On 23.02.16 г. 19:29, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 23/02/16 17:21, Georgi Djakov wrote:
>>>> When the SMEM is probed it defers as it depends on the hardware lock, which
>>>> is not available yet. But the SMD bus and RPM regulators and clocks depend
>>>> on SMEM and they defer too. The problem with this is that the order of
>>>> registering the devices is not optimal and also we may end with messed
>>>> up serial console as the RPM clocks are not registered yet..
>>> I noticed the same issue but was wondering why would we end up with messed up serial console?
>>>
>>> Could you add more details on why serial console is messed up?
>>>
>>> I thought, serial driver has nothing to do with the rpm clocks directly!
>>>
>>
>> If we don't have the rpm clocks registered, the uart clock is an orphan
>> and when clk_get_rate() is called on orphan clocks it returns 0 as rate.
> Shouldn't the actual uart clk provider registration fail/defer probe due to missing parent in this case?
> 

Yes, this is a known issue and people are currently working on it.
http://www.spinics.net/lists/linux-clk/msg00065.html
http://www.spinics.net/lists/arm-kernel/msg475910.html

Re: [PATCH] arm64: dts: msm8916: Move smem below hwlock

[Bjorn Andersson]

On Tue 23 Feb 09:21 PST 2016, Georgi Djakov wrote:

> When the SMEM is probed it defers as it depends on the hardware lock, which
> is not available yet. But the SMD bus and RPM regulators and clocks depend
> on SMEM and they defer too. The problem with this is that the order of
> registering the devices is not optimal and also we may end with messed
> up serial console as the RPM clocks are not registered yet..
> 
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>

Not only do you need the probe order to be "correct", you also need some
luck to have the smd code execute its two workers to initialise the
rpmcc driver before your serial driver is probed. So I don't think we
should merge this upstream.


I tried this on my linux-next tree on db410c and it doesn't hide the
problem for me.

Regards,
Bjorn

Re: [PATCH] arm64: dts: msm8916: Move smem below hwlock

[Georgi Djakov]

On 02/23/2016 09:03 PM, Andy Gross wrote:
> On Tue, Feb 23, 2016 at 08:47:56PM +0200, Georgi Djakov wrote:
>> On 23.02.16 г. 19:29, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 23/02/16 17:21, Georgi Djakov wrote:
>>>> When the SMEM is probed it defers as it depends on the hardware lock, which
>>>> is not available yet. But the SMD bus and RPM regulators and clocks depend
>>>> on SMEM and they defer too. The problem with this is that the order of
>>>> registering the devices is not optimal and also we may end with messed
>>>> up serial console as the RPM clocks are not registered yet..
>>> I noticed the same issue but was wondering why would we end up with messed up serial console?
>>>
>>> Could you add more details on why serial console is messed up?
>>>
>>> I thought, serial driver has nothing to do with the rpm clocks directly!
>>>
>>
>> If we don't have the rpm clocks registered, the uart clock is an orphan
>> and when clk_get_rate() is called on orphan clocks it returns 0 as rate.
>> In our case the msm_serial driver calls clk_get_rate() and gets 0 rate
>> as the parent rpm clock has not registered yet. The result is that the
>> baudrate is set incorrectly.
> 
> This isn't a probe defer issue w/ the SMEM and hwspinlock.  That works properly.

Ok, agree.

> This is an issue with the msm_serial either not probe deferring to wait for the
> rpm clocks or not handling the case of the clk framework giving us a 'bogus'
> clock.  Can we queue off the clk_get_rate being 0 to probe defer for the rpm
> clocks? (although that is hacky).

The proper solution would be to handle this in the clock framework.