diff mbox series

[v3,10/15] ARM: dts: qcom: msm8974: split TCSR halt regs out of mutex

Message ID 20220909092035.223915-11-krzysztof.kozlowski@linaro.org (mailing list archive)
State Not Applicable
Headers show
Series ARM/hwlock: qcom: switch TCSR mutex to MMIO | expand

Commit Message

Krzysztof Kozlowski Sept. 9, 2022, 9:20 a.m. UTC
The TCSR halt regs are next to TCSR mutex, so before converting the TCSR
mutex into device with address space, we need to split the halt regs to
its own syscon device.  This also describes more accurately the devices
and their IO address space.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 arch/arm/boot/dts/qcom-apq8074-dragonboard.dts        |  2 +-
 .../boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts   |  2 +-
 arch/arm/boot/dts/qcom-msm8974-sony-xperia-rhine.dtsi |  2 +-
 arch/arm/boot/dts/qcom-msm8974.dtsi                   | 11 ++++++++---
 arch/arm/boot/dts/qcom-msm8974pro-fairphone-fp2.dts   |  2 +-
 arch/arm/boot/dts/qcom-msm8974pro-samsung-klte.dts    |  2 +-
 .../qcom-msm8974pro-sony-xperia-shinano-castor.dts    |  2 +-
 7 files changed, 14 insertions(+), 9 deletions(-)

Comments

Bjorn Andersson Sept. 13, 2022, 10:44 p.m. UTC | #1
On Fri, Sep 09, 2022 at 11:20:30AM +0200, Krzysztof Kozlowski wrote:
[..]
> diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
> index 90a6d4b7605c..ada232bed2c8 100644
> --- a/arch/arm/boot/dts/qcom-msm8974.dtsi
> +++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
> @@ -1189,7 +1189,7 @@ remoteproc_mss: remoteproc@fc880000 {
>  			resets = <&gcc GCC_MSS_RESTART>;
>  			reset-names = "mss_restart";
>  
> -			qcom,halt-regs = <&tcsr_mutex_block 0x1180 0x1200 0x1280>;
> +			qcom,halt-regs = <&tcsr_1 0x180 0x200 0x280>;
>  
>  			qcom,smem-states = <&modem_smp2p_out 0>;
>  			qcom,smem-state-names = "stop";
> @@ -1230,10 +1230,15 @@ smd-edge {
>  
>  		tcsr_mutex_block: syscon@fd484000 {
>  			compatible = "syscon";
> -			reg = <0xfd484000 0x2000>;
> +			reg = <0xfd484000 0x1000>;
>  		};
>  
> -		tcsr: syscon@fd4a0000 {
> +		tcsr_1: syscon@fd485000 {

While the accessed registers look general purpose in nature, I would
prefer that we stick with naming it based on the register blocks - and
this is part of what's named "tcsr_mutex".

Is it not possible to claim that this region is a
"qcom,msm8974-tcsr-mutex" and a "syscon"?

> +			compatible = "qcom,tcsr-msm8974", "syscon";
> +			reg = <0xfd485000 0x1000>;
> +		};
> +
> +		tcsr_2: syscon@fd4a0000 {

And I would like to keep this as "tcsr".

Regards,
Bjorn
Krzysztof Kozlowski Sept. 15, 2022, 2:49 p.m. UTC | #2
On 13/09/2022 23:44, Bjorn Andersson wrote:
> On Fri, Sep 09, 2022 at 11:20:30AM +0200, Krzysztof Kozlowski wrote:
> [..]
>> diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
>> index 90a6d4b7605c..ada232bed2c8 100644
>> --- a/arch/arm/boot/dts/qcom-msm8974.dtsi
>> +++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
>> @@ -1189,7 +1189,7 @@ remoteproc_mss: remoteproc@fc880000 {
>>  			resets = <&gcc GCC_MSS_RESTART>;
>>  			reset-names = "mss_restart";
>>  
>> -			qcom,halt-regs = <&tcsr_mutex_block 0x1180 0x1200 0x1280>;
>> +			qcom,halt-regs = <&tcsr_1 0x180 0x200 0x280>;
>>  
>>  			qcom,smem-states = <&modem_smp2p_out 0>;
>>  			qcom,smem-state-names = "stop";
>> @@ -1230,10 +1230,15 @@ smd-edge {
>>  
>>  		tcsr_mutex_block: syscon@fd484000 {
>>  			compatible = "syscon";
>> -			reg = <0xfd484000 0x2000>;
>> +			reg = <0xfd484000 0x1000>;
>>  		};
>>  
>> -		tcsr: syscon@fd4a0000 {
>> +		tcsr_1: syscon@fd485000 {
> 
> While the accessed registers look general purpose in nature, I would
> prefer that we stick with naming it based on the register blocks - and
> this is part of what's named "tcsr_mutex".

Then everything would be like:

tcsr_mutex_1: syscon@fd484000
tcsr_mutex_2: syscon@fd485000
tcsr: syscon@fd4a0000
?

> 
> Is it not possible to claim that this region is a
> "qcom,msm8974-tcsr-mutex" and a "syscon"?

Hm, yes, that's another approach. We can go this way, but it has one
drawback - you could have two different devices (mutex and syscon user)
poking to the same registers. The regmap makes it safe from concurrency
point of view, but not safe from logic point of view.

Splitting these makes it sure, that no one touches hwlock registers,
except the hwlock driver.

Any preference?

> 
>> +			compatible = "qcom,tcsr-msm8974", "syscon";
>> +			reg = <0xfd485000 0x1000>;
>> +		};
>> +
>> +		tcsr_2: syscon@fd4a0000 {
> 
> And I would like to keep this as "tcsr".



Best regards,
Krzysztof
Bjorn Andersson Sept. 19, 2022, 9:01 p.m. UTC | #3
On Thu, Sep 15, 2022 at 03:49:37PM +0100, Krzysztof Kozlowski wrote:
> On 13/09/2022 23:44, Bjorn Andersson wrote:
> > On Fri, Sep 09, 2022 at 11:20:30AM +0200, Krzysztof Kozlowski wrote:
> > [..]
> >> diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
> >> index 90a6d4b7605c..ada232bed2c8 100644
> >> --- a/arch/arm/boot/dts/qcom-msm8974.dtsi
> >> +++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
> >> @@ -1189,7 +1189,7 @@ remoteproc_mss: remoteproc@fc880000 {
> >>  			resets = <&gcc GCC_MSS_RESTART>;
> >>  			reset-names = "mss_restart";
> >>  
> >> -			qcom,halt-regs = <&tcsr_mutex_block 0x1180 0x1200 0x1280>;
> >> +			qcom,halt-regs = <&tcsr_1 0x180 0x200 0x280>;
> >>  
> >>  			qcom,smem-states = <&modem_smp2p_out 0>;
> >>  			qcom,smem-state-names = "stop";
> >> @@ -1230,10 +1230,15 @@ smd-edge {
> >>  
> >>  		tcsr_mutex_block: syscon@fd484000 {
> >>  			compatible = "syscon";
> >> -			reg = <0xfd484000 0x2000>;
> >> +			reg = <0xfd484000 0x1000>;
> >>  		};
> >>  
> >> -		tcsr: syscon@fd4a0000 {
> >> +		tcsr_1: syscon@fd485000 {
> > 
> > While the accessed registers look general purpose in nature, I would
> > prefer that we stick with naming it based on the register blocks - and
> > this is part of what's named "tcsr_mutex".
> 
> Then everything would be like:
> 
> tcsr_mutex_1: syscon@fd484000
> tcsr_mutex_2: syscon@fd485000
> tcsr: syscon@fd4a0000
> ?
> 
> > 
> > Is it not possible to claim that this region is a
> > "qcom,msm8974-tcsr-mutex" and a "syscon"?
> 
> Hm, yes, that's another approach. We can go this way, but it has one
> drawback - you could have two different devices (mutex and syscon user)
> poking to the same registers. The regmap makes it safe from concurrency
> point of view, but not safe from logic point of view.
> 
> Splitting these makes it sure, that no one touches hwlock registers,
> except the hwlock driver.
> 
> Any preference?
> 

Certainly would be interesting if someone grabs the syscon and pokes at
the mutex registers, but I do prefer to have the DT match the register
regions when possible.

So if you're okay with making the whole tcsr mutex a hwlock and syscon
I prefer that.


PS. I picked all non-8974 patches from the series, just in case that
wasn't clear from the ty-letters.

Thanks,
Bjorn

> > 
> >> +			compatible = "qcom,tcsr-msm8974", "syscon";
> >> +			reg = <0xfd485000 0x1000>;
> >> +		};
> >> +
> >> +		tcsr_2: syscon@fd4a0000 {
> > 
> > And I would like to keep this as "tcsr".
> 
> 
> 
> Best regards,
> Krzysztof
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/qcom-apq8074-dragonboard.dts b/arch/arm/boot/dts/qcom-apq8074-dragonboard.dts
index 3051a861ff0c..2709a99e5c4c 100644
--- a/arch/arm/boot/dts/qcom-apq8074-dragonboard.dts
+++ b/arch/arm/boot/dts/qcom-apq8074-dragonboard.dts
@@ -38,7 +38,7 @@  &otg {
 	status = "okay";
 
 	phys = <&usb_hs2_phy>;
-	phy-select = <&tcsr 0xb000 1>;
+	phy-select = <&tcsr_2 0xb000 1>;
 	extcon = <&smbb>, <&usb_id>;
 	vbus-supply = <&chg_otg>;
 	hnp-disable;
diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
index ec5d340562b6..5fd94dd6a427 100644
--- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
+++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
@@ -251,7 +251,7 @@  &otg {
 	status = "okay";
 
 	phys = <&usb_hs1_phy>;
-	phy-select = <&tcsr 0xb000 0>;
+	phy-select = <&tcsr_2 0xb000 0>;
 
 	extcon = <&charger>, <&usb_id>;
 	vbus-supply = <&usb_otg_vbus>;
diff --git a/arch/arm/boot/dts/qcom-msm8974-sony-xperia-rhine.dtsi b/arch/arm/boot/dts/qcom-msm8974-sony-xperia-rhine.dtsi
index 5a70683d9103..118b231f3137 100644
--- a/arch/arm/boot/dts/qcom-msm8974-sony-xperia-rhine.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974-sony-xperia-rhine.dtsi
@@ -136,7 +136,7 @@  &otg {
 	status = "okay";
 
 	phys = <&usb_hs1_phy>;
-	phy-select = <&tcsr 0xb000 0>;
+	phy-select = <&tcsr_2 0xb000 0>;
 	extcon = <&smbb>, <&usb_id>;
 	vbus-supply = <&chg_otg>;
 
diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index 90a6d4b7605c..ada232bed2c8 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -1189,7 +1189,7 @@  remoteproc_mss: remoteproc@fc880000 {
 			resets = <&gcc GCC_MSS_RESTART>;
 			reset-names = "mss_restart";
 
-			qcom,halt-regs = <&tcsr_mutex_block 0x1180 0x1200 0x1280>;
+			qcom,halt-regs = <&tcsr_1 0x180 0x200 0x280>;
 
 			qcom,smem-states = <&modem_smp2p_out 0>;
 			qcom,smem-state-names = "stop";
@@ -1230,10 +1230,15 @@  smd-edge {
 
 		tcsr_mutex_block: syscon@fd484000 {
 			compatible = "syscon";
-			reg = <0xfd484000 0x2000>;
+			reg = <0xfd484000 0x1000>;
 		};
 
-		tcsr: syscon@fd4a0000 {
+		tcsr_1: syscon@fd485000 {
+			compatible = "qcom,tcsr-msm8974", "syscon";
+			reg = <0xfd485000 0x1000>;
+		};
+
+		tcsr_2: syscon@fd4a0000 {
 			compatible = "qcom,tcsr-msm8974", "syscon";
 			reg = <0xfd4a0000 0x10000>;
 		};
diff --git a/arch/arm/boot/dts/qcom-msm8974pro-fairphone-fp2.dts b/arch/arm/boot/dts/qcom-msm8974pro-fairphone-fp2.dts
index ff6e0066768b..c264d17e0953 100644
--- a/arch/arm/boot/dts/qcom-msm8974pro-fairphone-fp2.dts
+++ b/arch/arm/boot/dts/qcom-msm8974pro-fairphone-fp2.dts
@@ -89,7 +89,7 @@  &otg {
 	status = "okay";
 
 	phys = <&usb_hs1_phy>;
-	phy-select = <&tcsr 0xb000 0>;
+	phy-select = <&tcsr_2 0xb000 0>;
 	extcon = <&smbb>, <&usb_id>;
 	vbus-supply = <&chg_otg>;
 
diff --git a/arch/arm/boot/dts/qcom-msm8974pro-samsung-klte.dts b/arch/arm/boot/dts/qcom-msm8974pro-samsung-klte.dts
index 983e10c3d863..2691a6dbbb8b 100644
--- a/arch/arm/boot/dts/qcom-msm8974pro-samsung-klte.dts
+++ b/arch/arm/boot/dts/qcom-msm8974pro-samsung-klte.dts
@@ -379,7 +379,7 @@  &otg {
 	status = "okay";
 
 	phys = <&usb_hs1_phy>;
-	phy-select = <&tcsr 0xb000 0>;
+	phy-select = <&tcsr_2 0xb000 0>;
 
 	hnp-disable;
 	srp-disable;
diff --git a/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts b/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts
index 3f45f5c5d37b..d2bef3896c82 100644
--- a/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts
+++ b/arch/arm/boot/dts/qcom-msm8974pro-sony-xperia-shinano-castor.dts
@@ -216,7 +216,7 @@  &otg {
 	status = "okay";
 
 	phys = <&usb_hs1_phy>;
-	phy-select = <&tcsr 0xb000 0>;
+	phy-select = <&tcsr_2 0xb000 0>;
 	extcon = <&smbb>, <&usb_id>;
 	vbus-supply = <&chg_otg>;