| Message ID | 20220909092035.223915-11-krzysztof.kozlowski@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | ARM/hwlock: qcom: switch TCSR mutex to MMIO | expand |
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
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
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 --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>;
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(-)