| Message ID | cover.1531135999.git.amit.kucheria@linaro.org |
|---|---|
| Headers | show |
| Series | thermal: tsens: Refactoring for TSENSv2 IP | expand |
On Mon 09 Jul 04:43 PDT 2018, Amit Kucheria wrote: > We also split up the regmap address space into two, one for the TM > registers, the other for the SROT registers. This was required to deal with > different address offsets for the TM and SROT registers across different > SoC families. > > Since tsens-common.c/init_common() currently only registers one address > space, the order is important (TM before SROT). This is OK since the code > doesn't really use the SROT functionality yet. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > index 8c7f9ca..6c8a857 100644 > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi > @@ -461,7 +461,17 @@ > > tsens0: thermal-sensor@4a8000 { > compatible = "qcom,msm8996-tsens"; > - reg = <0x4a8000 0x2000>; > + reg = <0x4a9000 0x1000>, /* TM */ > + <0x4a8000 0x1000>; /* SROT */ > + #qcom,sensors = <13>; > + #thermal-sensor-cells = <1>; > + }; > + > + tsens1: thermal-sensor@4ac000 { > + compatible = "qcom,msm8996-tsens"; > + reg = <0x4ad000 0x1000>, /* TM */ > + <0x4ac000 0x1000>; /* SROT */ > + #qcom,sensors = <8>; > #thermal-sensor-cells = <1>; > }; > > -- > 2.7.4 > -- 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
On Mon 09 Jul 04:43 PDT 2018, Amit Kucheria wrote: > SDM845 has two tsens blocks, one with 13 sensors and the other with 8 > sensors. It uses version 2 of the TSENS IP, so use the fallback property to > allow more common code. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > arch/arm64/boot/dts/qcom/sdm845.dtsi | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index cdaabeb..ba2899c 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -221,6 +221,22 @@ > #interrupt-cells = <2>; > }; > > + tsens0: tsens@c263000 { > + compatible = "qcom,sdm845-tsens", "qcom,tsens-v2"; > + reg = <0xc263000 0x1ff>, /* TM */ > + <0xc222000 0x1ff>; /* SROT */ > + #qcom,sensors = <13>; > + #thermal-sensor-cells = <1>; > + }; > + > + tsens1: tsens@c265000 { > + compatible = "qcom,sdm845-tsens", "qcom,tsens-v2"; > + reg = <0xc265000 0x1ff>, /* TM */ > + <0xc223000 0x1ff>; /* SROT */ > + #qcom,sensors = <8>; > + #thermal-sensor-cells = <1>; > + }; > + > spmi_bus: spmi@c440000 { > compatible = "qcom,spmi-pmic-arb"; > reg = <0xc440000 0x1100>, > -- > 2.7.4 > -- 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
Hi, On Mon, Jul 9, 2018 at 4:43 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote: > We also split up the regmap address space into two, one for the TM > registers, the other for the SROT registers. This was required to deal with > different address offsets for the TM and SROT registers across different > SoC families. The splitting into two regions is actually optional and that should probably be mentioned in the commit message. > Since tsens-common.c/init_common() currently only registers one address > space, the order is important (TM before SROT). This is OK since the code > doesn't really use the SROT functionality yet. Nowhere in the commit message does this say you're also adding a 2nd block of thermal sensors. It seems like you should say that somewhere. ...and it should also be obvious in ${SUBJECT}. > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > index 8c7f9ca..6c8a857 100644 > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi > @@ -461,7 +461,17 @@ > > tsens0: thermal-sensor@4a8000 { > compatible = "qcom,msm8996-tsens"; > - reg = <0x4a8000 0x2000>; > + reg = <0x4a9000 0x1000>, /* TM */ > + <0x4a8000 0x1000>; /* SROT */ Note that the unit address is supposed to match the first "reg" address, so either these should be reversed or you should update your node name. AKA your node name should be this now: tsens0: thermal-sensor@4a9000 > + #qcom,sensors = <13>; As per my responses to other patches, " #qcom,sensors" is undocumented and doesn't appear to be read by the driver. -- 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
Hi, On Mon, Jul 9, 2018 at 4:43 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote: > SDM845 has two tsens blocks, one with 13 sensors and the other with 8 > sensors. It uses version 2 of the TSENS IP, so use the fallback property to > allow more common code. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > arch/arm64/boot/dts/qcom/sdm845.dtsi | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index cdaabeb..ba2899c 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -221,6 +221,22 @@ > #interrupt-cells = <2>; > }; > > + tsens0: tsens@c263000 { As per my comments in the bindings, nit that this should probably be "thermal-sensor" not "tsens", AKA: tsens0: thermal-sensor@c263000 { > + compatible = "qcom,sdm845-tsens", "qcom,tsens-v2"; > + reg = <0xc263000 0x1ff>, /* TM */ > + <0xc222000 0x1ff>; /* SROT */ > + #qcom,sensors = <13>; As per my comment in the bindings and the code, I'm confused about the whole "#qcom,sensors" bit. It's not documented and doesn't seem hooked up in the code either. ...but if people have tested this, perhaps I'm confused. How can things work if num_sensors is 0??? -Doug -- 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
On Wed, Jul 11, 2018 at 11:44:13AM -0700, Doug Anderson wrote: > Hi, > > On Mon, Jul 9, 2018 at 4:43 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote: > > SDM845 has two tsens blocks, one with 13 sensors and the other with 8 > > sensors. It uses version 2 of the TSENS IP, so use the fallback property to > > allow more common code. > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/sdm845.dtsi | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > > index cdaabeb..ba2899c 100644 > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > > @@ -221,6 +221,22 @@ > > #interrupt-cells = <2>; > > }; > > > > + tsens0: tsens@c263000 { > > As per my comments in the bindings, nit that this should probably be > "thermal-sensor" not "tsens", AKA: > > tsens0: thermal-sensor@c263000 { > > > + compatible = "qcom,sdm845-tsens", "qcom,tsens-v2"; > > + reg = <0xc263000 0x1ff>, /* TM */ > > + <0xc222000 0x1ff>; /* SROT */ > > + #qcom,sensors = <13>; > > As per my comment in the bindings and the code, I'm confused about the > whole "#qcom,sensors" bit. It's not documented and doesn't seem > hooked up in the code either. > > ...but if people have tested this, perhaps I'm confused. How can > things work if num_sensors is 0??? The mystery is resolved by: commit 6d7c70d1cd6526dc79e3d3b3faae1c40c1681168 Author: Bjorn Andersson <bjorn.andersson@linaro.org> Date: Mon May 7 16:53:39 2018 -0700 thermal: qcom: tsens: Allow number of sensors to come from DT For platforms that has multiple copies of the TSENS hardware block it's necessary to be able to specify the number of sensors per block in DeviceTree. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org> Reviewed-by: Rob Herring <robh@kernel.org> Signed-off-by: Eduardo Valentin <edubezval@gmail.com> I bumped into this during testing ;-) -- 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
Hi Matthias, On Wed, Jul 11, 2018 at 2:51 PM, Matthias Kaehlcke <mka@chromium.org> wrote: > On Wed, Jul 11, 2018 at 11:44:13AM -0700, Doug Anderson wrote: >> Hi, >> >> On Mon, Jul 9, 2018 at 4:43 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote: >> > SDM845 has two tsens blocks, one with 13 sensors and the other with 8 >> > sensors. It uses version 2 of the TSENS IP, so use the fallback property to >> > allow more common code. >> > >> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> >> > --- >> > arch/arm64/boot/dts/qcom/sdm845.dtsi | 16 ++++++++++++++++ >> > 1 file changed, 16 insertions(+) >> > >> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi >> > index cdaabeb..ba2899c 100644 >> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi >> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi >> > @@ -221,6 +221,22 @@ >> > #interrupt-cells = <2>; >> > }; >> > >> > + tsens0: tsens@c263000 { >> >> As per my comments in the bindings, nit that this should probably be >> "thermal-sensor" not "tsens", AKA: >> >> tsens0: thermal-sensor@c263000 { >> >> > + compatible = "qcom,sdm845-tsens", "qcom,tsens-v2"; >> > + reg = <0xc263000 0x1ff>, /* TM */ >> > + <0xc222000 0x1ff>; /* SROT */ >> > + #qcom,sensors = <13>; >> >> As per my comment in the bindings and the code, I'm confused about the >> whole "#qcom,sensors" bit. It's not documented and doesn't seem >> hooked up in the code either. >> >> ...but if people have tested this, perhaps I'm confused. How can >> things work if num_sensors is 0??? > > The mystery is resolved by: > > commit 6d7c70d1cd6526dc79e3d3b3faae1c40c1681168 > Author: Bjorn Andersson <bjorn.andersson@linaro.org> > Date: Mon May 7 16:53:39 2018 -0700 > > thermal: qcom: tsens: Allow number of sensors to come from DT > > For platforms that has multiple copies of the TSENS hardware block it's > necessary to be able to specify the number of sensors per block in DeviceTree. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org> > Reviewed-by: Rob Herring <robh@kernel.org> > Signed-off-by: Eduardo Valentin <edubezval@gmail.com> > > > I bumped into this during testing ;-) Ah, now it makes sense to me! Serves me right for assuming it would be in the same series and not checking if it was something that had already landed. Thanks. Please ignore the parts of my comments related to the "#qcom,sensors" property. I guess Rob must have thought that the "#" in the name was fine and he's the one in charge not me. -Doug -- 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
On Thu, Jul 12, 2018 at 12:09 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Mon, Jul 9, 2018 at 4:43 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote: > > We also split up the regmap address space into two, one for the TM > > registers, the other for the SROT registers. This was required to deal with > > different address offsets for the TM and SROT registers across different > > SoC families. > > The splitting into two regions is actually optional and that should > probably be mentioned in the commit message. On the contrary, after this refactor, all new platforms with the v2.x.y TSENS IP should use two regions. The only reason for patch 2 is that we're stuck with supporting old 8996/8916 DTs. I'd prefer to phase out support for the old DTs if possible. I don't want to encourage any new bindings with a single address space. > > Since tsens-common.c/init_common() currently only registers one address > > space, the order is important (TM before SROT). This is OK since the code > > doesn't really use the SROT functionality yet. > > Nowhere in the commit message does this say you're also adding a 2nd > block of thermal sensors. It seems like you should say that > somewhere. > > ...and it should also be obvious in ${SUBJECT}. Fixed. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/msm8996.dtsi | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > > index 8c7f9ca..6c8a857 100644 > > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi > > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi > > @@ -461,7 +461,17 @@ > > > > tsens0: thermal-sensor@4a8000 { > > compatible = "qcom,msm8996-tsens"; > > - reg = <0x4a8000 0x2000>; > > + reg = <0x4a9000 0x1000>, /* TM */ > > + <0x4a8000 0x1000>; /* SROT */ > > Note that the unit address is supposed to match the first "reg" > address, so either these should be reversed or you should update your > node name. AKA your node name should be this now: > > tsens0: thermal-sensor@4a9000 Fixed. > > > + #qcom,sensors = <13>; > > As per my responses to other patches, " #qcom,sensors" is undocumented > and doesn't appear to be read by the driver. This feature was merged earlier. See commit 6d7c70d1cd6526 (thermal: qcom: tsens: Allow number of sensors to come from DT) Regards, Amit -- 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
On Thu, Jul 12, 2018 at 3:30 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi Matthias, > > On Wed, Jul 11, 2018 at 2:51 PM, Matthias Kaehlcke <mka@chromium.org> wrote: > > On Wed, Jul 11, 2018 at 11:44:13AM -0700, Doug Anderson wrote: > >> Hi, > >> > >> On Mon, Jul 9, 2018 at 4:43 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote: > >> > SDM845 has two tsens blocks, one with 13 sensors and the other with 8 > >> > sensors. It uses version 2 of the TSENS IP, so use the fallback property to > >> > allow more common code. > >> > > >> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > >> > --- > >> > arch/arm64/boot/dts/qcom/sdm845.dtsi | 16 ++++++++++++++++ > >> > 1 file changed, 16 insertions(+) > >> > > >> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > >> > index cdaabeb..ba2899c 100644 > >> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > >> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > >> > @@ -221,6 +221,22 @@ > >> > #interrupt-cells = <2>; > >> > }; > >> > > >> > + tsens0: tsens@c263000 { > >> > >> As per my comments in the bindings, nit that this should probably be > >> "thermal-sensor" not "tsens", AKA: > >> > >> tsens0: thermal-sensor@c263000 { Fixed. > >> > + compatible = "qcom,sdm845-tsens", "qcom,tsens-v2"; > >> > + reg = <0xc263000 0x1ff>, /* TM */ > >> > + <0xc222000 0x1ff>; /* SROT */ > >> > + #qcom,sensors = <13>; > >> > >> As per my comment in the bindings and the code, I'm confused about the > >> whole "#qcom,sensors" bit. It's not documented and doesn't seem > >> hooked up in the code either. > >> > >> ...but if people have tested this, perhaps I'm confused. How can > >> things work if num_sensors is 0??? > > > > The mystery is resolved by: > > > > commit 6d7c70d1cd6526dc79e3d3b3faae1c40c1681168 > > Author: Bjorn Andersson <bjorn.andersson@linaro.org> > > Date: Mon May 7 16:53:39 2018 -0700 > > > > thermal: qcom: tsens: Allow number of sensors to come from DT > > > > For platforms that has multiple copies of the TSENS hardware block it's > > necessary to be able to specify the number of sensors per block in DeviceTree. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org> > > Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Eduardo Valentin <edubezval@gmail.com> > > > > > > I bumped into this during testing ;-) I think this was merged in 4.17, so you didn't see it in your tree :-) > Ah, now it makes sense to me! Serves me right for assuming it would > be in the same series and not checking if it was something that had > already landed. Thanks. Please ignore the parts of my comments > related to the "#qcom,sensors" property. I guess Rob must have > thought that the "#" in the name was fine and he's the one in charge > not me. Thanks for your review, Doug. I'll test this and post a v7 today. I'd really like to get this accepted for 4.19 so I can post interrupt support and some more cleanups. Regards, Amit -- 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
This series is a mixed bag: - Some code moves to allow code sharing between different SoCs with v2 of the TSENS IP, - a generic qcom,tsens-v2 property as a fallback compatible for all v2.x.y platforms, - new platform support (sdm845) - a cleanup patch and - a DT change to have a common way to deal with the SROT and TM registers despite slightly different features across the IP family and different register offsets. Changes since v5: - Actually fix unit addressses for the two tsens blocks as per Stephen's comment. Changes since v4: - Revert back to a single fallback bindind qcom,tsens-v2 as per Rob's suggestion. - Rework how old (unsplit SROT and TM address space) DTs are handled by needing a 0x1000 offset but still sharing common code in tsens-v2.c - Remove the patch to added TRDY checks while we investigate Matthias' reports - Fix unit addressses for the two tsens blocks as per Stephen's comment. Changes since v3: - Introduce qcom,tsens-v2.4.0 property and make qcom,tsens-v2 a fallback, compatible property. - Rename ops_v2 to ops_generic_v2 Changes since v2: - Based on review, moved tsens-8996.c to tsens-v2.c and changed corresponding function names, struct names to allow for generic tsensv2 platforms - All v2 platforms will now only need to use the qcom,tsen-v2 property - Added a DT patch to initialize tsens driver on sdm845, now that 4.18-rc1 will contain an sdm845.dtsi Changes since v1: - Move get_temp() from tsens-8996 to tsens-common and rename - Change 8996 DT entry to allow init_common() to work across sdm845 and 8996 due to different offsets Amit Kucheria (7): thermal: tsens: Get rid of unused fields in structure thermal: tsens: Add support to split up register address space into two dt: qcom: 8996: thermal: Move to DT initialisation thermal: tsens: Rename tsens-8996 to tsens-v2 for reuse thermal: tsens: Add generic support for TSENS v2 IP dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP arm64: dts: sdm845: Add tsens nodes .../devicetree/bindings/thermal/qcom-tsens.txt | 25 +++++++++++++---- arch/arm64/boot/dts/qcom/msm8996.dtsi | 12 +++++++- arch/arm64/boot/dts/qcom/sdm845.dtsi | 16 +++++++++++ drivers/thermal/qcom/Makefile | 2 +- drivers/thermal/qcom/tsens-common.c | 11 ++++++++ drivers/thermal/qcom/{tsens-8996.c => tsens-v2.c} | 32 ++++++++++------------ drivers/thermal/qcom/tsens.c | 3 ++ drivers/thermal/qcom/tsens.h | 8 ++++-- 8 files changed, 81 insertions(+), 28 deletions(-) rename drivers/thermal/qcom/{tsens-8996.c => tsens-v2.c} (64%)