linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/7] thermal: tsens: Refactoring for TSENSv2 IP
@ 2018-07-09 11:43 Amit Kucheria
  2018-07-09 11:43 ` [PATCH v6 1/7] thermal: tsens: Get rid of unused fields in structure Amit Kucheria
                   ` (6 more replies)
  0 siblings, 7 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-09 11:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: rnayak, linux-arm-msm, bjorn.andersson, edubezval, smohanad,
	vivek.gautam, andy.gross, Kees Cook, Rob Herring, devicetree,
	linux-arm-kernel, linux-pm, linux-soc

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%)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v6 1/7] thermal: tsens: Get rid of unused fields in structure
  2018-07-09 11:43 [PATCH v6 0/7] thermal: tsens: Refactoring for TSENSv2 IP Amit Kucheria
@ 2018-07-09 11:43 ` Amit Kucheria
  2018-07-09 11:43 ` [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two Amit Kucheria
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-09 11:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: rnayak, linux-arm-msm, bjorn.andersson, edubezval, smohanad,
	vivek.gautam, andy.gross, Zhang Rui, linux-pm

status_field and trdy are unused in any of the tsens drivers. Remove them.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Acked-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/thermal/qcom/tsens.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index 911c197..dc56e1e 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -77,9 +77,7 @@ struct tsens_device {
 	struct device			*dev;
 	u32				num_sensors;
 	struct regmap			*map;
-	struct regmap_field		*status_field;
 	struct tsens_context		ctx;
-	bool				trdy;
 	const struct tsens_ops		*ops;
 	struct tsens_sensor		sensor[0];
 };
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two
  2018-07-09 11:43 [PATCH v6 0/7] thermal: tsens: Refactoring for TSENSv2 IP Amit Kucheria
  2018-07-09 11:43 ` [PATCH v6 1/7] thermal: tsens: Get rid of unused fields in structure Amit Kucheria
@ 2018-07-09 11:43 ` Amit Kucheria
  2018-07-11 16:34   ` Bjorn Andersson
  2018-07-11 18:37   ` Doug Anderson
  2018-07-09 11:43 ` [PATCH v6 3/7] dt: qcom: 8996: thermal: Move to DT initialisation Amit Kucheria
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-09 11:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: rnayak, linux-arm-msm, bjorn.andersson, edubezval, smohanad,
	vivek.gautam, andy.gross, Zhang Rui, linux-pm

There are two banks of registers for v2 TSENS IPs: SROT and TM. On older
SoCs these were contiguous, leading to DTs mapping them as one register
address space of size 0x2000. In newer SoCs, these two banks are not
contiguous anymore.

Fixing old DTs to split the address space into allows us to have cleaner
common code e.g. get_temp() that is shared across new and old platforms.

But we need to add logic to init_common() to differentiate between old and
new DTs and adjust associated offsets for the TM register bank so that the
old DTs will continue to function correctly.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens-8996.c   |  2 +-
 drivers/thermal/qcom/tsens-common.c | 11 +++++++++++
 drivers/thermal/qcom/tsens.h        |  1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/qcom/tsens-8996.c b/drivers/thermal/qcom/tsens-8996.c
index e1f7781..60765b1 100644
--- a/drivers/thermal/qcom/tsens-8996.c
+++ b/drivers/thermal/qcom/tsens-8996.c
@@ -28,7 +28,7 @@ static int get_temp_8996(struct tsens_device *tmdev, int id, int *temp)
 	unsigned int sensor_addr;
 	int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
 
-	sensor_addr = STATUS_OFFSET + s->hw_id * 4;
+	sensor_addr = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
 	ret = regmap_read(tmdev->map, sensor_addr, &code);
 	if (ret)
 		return ret;
diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
index b1449ad..4a741b0 100644
--- a/drivers/thermal/qcom/tsens-common.c
+++ b/drivers/thermal/qcom/tsens-common.c
@@ -16,6 +16,7 @@
 #include <linux/io.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/of_address.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include "tsens.h"
@@ -126,11 +127,21 @@ static const struct regmap_config tsens_config = {
 int __init init_common(struct tsens_device *tmdev)
 {
 	void __iomem *base;
+	struct platform_device *op = of_find_device_by_node(tmdev->dev->of_node);
 
+	if (!op)
+		return -EINVAL;
 	base = of_iomap(tmdev->dev->of_node, 0);
 	if (!base)
 		return -EINVAL;
 
+	if (op->num_resources > 1) {
+		tmdev->tm_offset = 0;
+	} else {
+		/* old DTs where SROT and TM were in a contiguous 2K block */
+		tmdev->tm_offset = 0x1000;
+	}
+
 	tmdev->map = devm_regmap_init_mmio(tmdev->dev, base, &tsens_config);
 	if (IS_ERR(tmdev->map)) {
 		iounmap(base);
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index dc56e1e..d785b37 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -77,6 +77,7 @@ struct tsens_device {
 	struct device			*dev;
 	u32				num_sensors;
 	struct regmap			*map;
+	u32				tm_offset;
 	struct tsens_context		ctx;
 	const struct tsens_ops		*ops;
 	struct tsens_sensor		sensor[0];
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v6 3/7] dt: qcom: 8996: thermal: Move to DT initialisation
  2018-07-09 11:43 [PATCH v6 0/7] thermal: tsens: Refactoring for TSENSv2 IP Amit Kucheria
  2018-07-09 11:43 ` [PATCH v6 1/7] thermal: tsens: Get rid of unused fields in structure Amit Kucheria
  2018-07-09 11:43 ` [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two Amit Kucheria
@ 2018-07-09 11:43 ` Amit Kucheria
  2018-07-11 16:37   ` Bjorn Andersson
  2018-07-11 18:39   ` Doug Anderson
  2018-07-09 11:43 ` [PATCH v6 4/7] thermal: tsens: Rename tsens-8996 to tsens-v2 for reuse Amit Kucheria
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-09 11:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: rnayak, linux-arm-msm, bjorn.andersson, edubezval, smohanad,
	vivek.gautam, andy.gross, David Brown, Rob Herring, Mark Rutland,
	Catalin Marinas, Will Deacon, linux-soc, devicetree,
	linux-arm-kernel

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>
---
 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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v6 4/7] thermal: tsens: Rename tsens-8996 to tsens-v2 for reuse
  2018-07-09 11:43 [PATCH v6 0/7] thermal: tsens: Refactoring for TSENSv2 IP Amit Kucheria
                   ` (2 preceding siblings ...)
  2018-07-09 11:43 ` [PATCH v6 3/7] dt: qcom: 8996: thermal: Move to DT initialisation Amit Kucheria
@ 2018-07-09 11:43 ` Amit Kucheria
  2018-07-11 16:39   ` Bjorn Andersson
  2018-07-09 11:43 ` [PATCH v6 5/7] thermal: tsens: Add generic support for TSENS v2 IP Amit Kucheria
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Amit Kucheria @ 2018-07-09 11:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: rnayak, linux-arm-msm, bjorn.andersson, edubezval, smohanad,
	vivek.gautam, andy.gross, Zhang Rui, linux-pm

The TSENS block inside the 8996 is internally classified as version 2 of
the IP. Several other SoC families use this block and can share this code.

We rename get_temp() to reflect that it can be used across the v2 family.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/Makefile                     |  2 +-
 drivers/thermal/qcom/{tsens-8996.c => tsens-v2.c} | 26 ++++++++---------------
 2 files changed, 10 insertions(+), 18 deletions(-)
 rename drivers/thermal/qcom/{tsens-8996.c => tsens-v2.c} (66%)

diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
index 2cc2193..a821929 100644
--- a/drivers/thermal/qcom/Makefile
+++ b/drivers/thermal/qcom/Makefile
@@ -1,2 +1,2 @@
 obj-$(CONFIG_QCOM_TSENS)	+= qcom_tsens.o
-qcom_tsens-y			+= tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-8996.o
+qcom_tsens-y			+= tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-v2.o
diff --git a/drivers/thermal/qcom/tsens-8996.c b/drivers/thermal/qcom/tsens-v2.c
similarity index 66%
rename from drivers/thermal/qcom/tsens-8996.c
rename to drivers/thermal/qcom/tsens-v2.c
index 60765b1..34ba6c7 100644
--- a/drivers/thermal/qcom/tsens-8996.c
+++ b/drivers/thermal/qcom/tsens-v2.c
@@ -1,27 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2015, The Linux Foundation. All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 and
- * only version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
+ * Copyright (c) 2018, Linaro Limited
  */
 
-#include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include "tsens.h"
 
-#define STATUS_OFFSET	0x10a0
-#define LAST_TEMP_MASK	0xfff
+#define STATUS_OFFSET		0xa0
+#define LAST_TEMP_MASK		0xfff
 #define STATUS_VALID_BIT	BIT(21)
 #define CODE_SIGN_BIT		BIT(11)
 
-static int get_temp_8996(struct tsens_device *tmdev, int id, int *temp)
+static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
 {
 	struct tsens_sensor *s = &tmdev->sensor[id];
 	u32 code;
@@ -73,12 +64,13 @@ static int get_temp_8996(struct tsens_device *tmdev, int id, int *temp)
 	return 0;
 }
 
-static const struct tsens_ops ops_8996 = {
+static const struct tsens_ops ops_generic_v2 = {
 	.init		= init_common,
-	.get_temp	= get_temp_8996,
+	.get_temp	= get_temp_tsens_v2,
 };
 
 const struct tsens_data data_8996 = {
 	.num_sensors	= 13,
-	.ops		= &ops_8996,
+	.ops		= &ops_generic_v2,
 };
+
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v6 5/7] thermal: tsens: Add generic support for TSENS v2 IP
  2018-07-09 11:43 [PATCH v6 0/7] thermal: tsens: Refactoring for TSENSv2 IP Amit Kucheria
                   ` (3 preceding siblings ...)
  2018-07-09 11:43 ` [PATCH v6 4/7] thermal: tsens: Rename tsens-8996 to tsens-v2 for reuse Amit Kucheria
@ 2018-07-09 11:43 ` Amit Kucheria
  2018-07-11 16:40   ` Bjorn Andersson
  2018-07-11 18:40   ` Doug Anderson
  2018-07-09 11:43 ` [PATCH v6 6/7] dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP Amit Kucheria
  2018-07-09 11:43 ` [PATCH v6 7/7] arm64: dts: sdm845: Add tsens nodes Amit Kucheria
  6 siblings, 2 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-09 11:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: rnayak, linux-arm-msm, bjorn.andersson, edubezval, smohanad,
	vivek.gautam, andy.gross, Zhang Rui, Kees Cook, Rob Herring,
	linux-pm

SDM845 uses v2 of the TSENS IP block but the get_temp() function
appears to be identical across v2.x.y in code seen so far. We use the
generic get_temp() function defined as part of ops_generic_v2.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens-v2.c | 6 +++++-
 drivers/thermal/qcom/tsens.c    | 3 +++
 drivers/thermal/qcom/tsens.h    | 5 ++++-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
index 34ba6c7..f40150f 100644
--- a/drivers/thermal/qcom/tsens-v2.c
+++ b/drivers/thermal/qcom/tsens-v2.c
@@ -69,8 +69,12 @@ static const struct tsens_ops ops_generic_v2 = {
 	.get_temp	= get_temp_tsens_v2,
 };
 
+const struct tsens_data data_tsens_v2 = {
+	.ops            = &ops_generic_v2,
+};
+
+/* Kept around for backward compatibility with old msm8996.dtsi */
 const struct tsens_data data_8996 = {
 	.num_sensors	= 13,
 	.ops		= &ops_generic_v2,
 };
-
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 3440166c..a2c9bfa 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -72,6 +72,9 @@ static const struct of_device_id tsens_table[] = {
 	}, {
 		.compatible = "qcom,msm8996-tsens",
 		.data = &data_8996,
+	}, {
+		.compatible = "qcom,tsens-v2",
+		.data = &data_tsens_v2,
 	},
 	{}
 };
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index d785b37..14331eb 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -88,6 +88,9 @@ void compute_intercept_slope(struct tsens_device *, u32 *, u32 *, u32);
 int init_common(struct tsens_device *);
 int get_temp_common(struct tsens_device *, int, int *);
 
-extern const struct tsens_data data_8916, data_8974, data_8960, data_8996;
+/* TSENS v1 targets */
+extern const struct tsens_data data_8916, data_8974, data_8960;
+/* TSENS v2 targets */
+extern const struct tsens_data data_8996, data_tsens_v2;
 
 #endif /* __QCOM_TSENS_H__ */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v6 6/7] dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP
  2018-07-09 11:43 [PATCH v6 0/7] thermal: tsens: Refactoring for TSENSv2 IP Amit Kucheria
                   ` (4 preceding siblings ...)
  2018-07-09 11:43 ` [PATCH v6 5/7] thermal: tsens: Add generic support for TSENS v2 IP Amit Kucheria
@ 2018-07-09 11:43 ` Amit Kucheria
  2018-07-11 13:49   ` Rob Herring
                     ` (2 more replies)
  2018-07-09 11:43 ` [PATCH v6 7/7] arm64: dts: sdm845: Add tsens nodes Amit Kucheria
  6 siblings, 3 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-09 11:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: rnayak, linux-arm-msm, bjorn.andersson, edubezval, smohanad,
	vivek.gautam, andy.gross, Zhang Rui, Rob Herring, Mark Rutland,
	linux-pm, devicetree

We want to create common code for v2 of the TSENS IP block that is used in
a large number of Qualcomm SoCs. "qcom,tsens-v2" should be able to handle
most of the common functionality start with a common get_temp() function.

It is also necessary to split out the memory regions for the TM and SROT
register banks because their offsets are not constant across SoC families.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 .../devicetree/bindings/thermal/qcom-tsens.txt     | 25 +++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
index 06195e8..8f963b1 100644
--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
@@ -1,10 +1,16 @@
 * QCOM SoC Temperature Sensor (TSENS)
 
 Required properties:
-- compatible :
- - "qcom,msm8916-tsens" : For 8916 Family of SoCs
- - "qcom,msm8974-tsens" : For 8974 Family of SoCs
- - "qcom,msm8996-tsens" : For 8996 Family of SoCs
+- compatible:
+  Must be one of the following:
+    - "qcom,msm8916-tsens" (MSM8916)
+    - "qcom,msm8974-tsens" (MSM8974)
+    - "qcom,msm8996-tsens" (MSM8996)
+    - "qcom,msm8998-tsens", "qcom,tsens-v2" (MSM8998)
+    - "qcom,sdm845-tsens", "qcom,tsens-v2" (SDM845)
+  The generic "qcom,tsens-v2" property must be used as a fallback for any SoC with
+  version 2 of the TSENS IP. MSM8996 is the only exception beacause the generic
+  property did not exist when support was added.
 
 - reg: Address range of the thermal registers
 - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
@@ -12,7 +18,7 @@ Required properties:
 - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify
 nvmem cells
 
-Example:
+Example 1 (legacy support before a fallback tsens-v2 propoerty was introduced):
 tsens: thermal-sensor@900000 {
 		compatible = "qcom,msm8916-tsens";
 		reg = <0x4a8000 0x2000>;
@@ -20,3 +26,12 @@ tsens: thermal-sensor@900000 {
 		nvmem-cell-names = "caldata", "calsel";
 		#thermal-sensor-cells = <1>;
 	};
+
+Example 2 (for any platform containing v2 of the TSENS IP):
+tsens0: tsens@c222000 {
+		compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
+		reg = <0xc263000 0x1ff>, /* TM */
+			<0xc222000 0x1ff>; /* SROT */
+		#qcom,sensors = <13>;
+		#thermal-sensor-cells = <1>;
+	};
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v6 7/7] arm64: dts: sdm845: Add tsens nodes
  2018-07-09 11:43 [PATCH v6 0/7] thermal: tsens: Refactoring for TSENSv2 IP Amit Kucheria
                   ` (5 preceding siblings ...)
  2018-07-09 11:43 ` [PATCH v6 6/7] dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP Amit Kucheria
@ 2018-07-09 11:43 ` Amit Kucheria
  2018-07-11 16:41   ` Bjorn Andersson
  2018-07-11 18:44   ` Doug Anderson
  6 siblings, 2 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-09 11:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: rnayak, linux-arm-msm, bjorn.andersson, edubezval, smohanad,
	vivek.gautam, andy.gross, David Brown, Rob Herring, Mark Rutland,
	Catalin Marinas, Will Deacon, linux-soc, devicetree,
	linux-arm-kernel

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 {
+			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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 6/7] dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP
  2018-07-09 11:43 ` [PATCH v6 6/7] dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP Amit Kucheria
@ 2018-07-11 13:49   ` Rob Herring
  2018-07-11 16:42   ` Bjorn Andersson
  2018-07-11 18:42   ` Doug Anderson
  2 siblings, 0 replies; 27+ messages in thread
From: Rob Herring @ 2018-07-11 13:49 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: linux-kernel, rnayak, linux-arm-msm, bjorn.andersson, edubezval,
	smohanad, vivek.gautam, andy.gross, Zhang Rui, Mark Rutland,
	linux-pm, devicetree

On Mon, Jul 09, 2018 at 05:13:28PM +0530, Amit Kucheria wrote:
> We want to create common code for v2 of the TSENS IP block that is used in
> a large number of Qualcomm SoCs. "qcom,tsens-v2" should be able to handle
> most of the common functionality start with a common get_temp() function.
> 
> It is also necessary to split out the memory regions for the TM and SROT
> register banks because their offsets are not constant across SoC families.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  .../devicetree/bindings/thermal/qcom-tsens.txt     | 25 +++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two
  2018-07-09 11:43 ` [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two Amit Kucheria
@ 2018-07-11 16:34   ` Bjorn Andersson
  2018-07-11 18:37   ` Doug Anderson
  1 sibling, 0 replies; 27+ messages in thread
From: Bjorn Andersson @ 2018-07-11 16:34 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: linux-kernel, rnayak, linux-arm-msm, edubezval, smohanad,
	vivek.gautam, andy.gross, Zhang Rui, linux-pm

On Mon 09 Jul 04:43 PDT 2018, Amit Kucheria wrote:

> There are two banks of registers for v2 TSENS IPs: SROT and TM. On older
> SoCs these were contiguous, leading to DTs mapping them as one register
> address space of size 0x2000. In newer SoCs, these two banks are not
> contiguous anymore.
> 
> Fixing old DTs to split the address space into allows us to have cleaner
> common code e.g. get_temp() that is shared across new and old platforms.
> 
> But we need to add logic to init_common() to differentiate between old and
> new DTs and adjust associated offsets for the TM register bank so that the
> old DTs will continue to function correctly.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/thermal/qcom/tsens-8996.c   |  2 +-
>  drivers/thermal/qcom/tsens-common.c | 11 +++++++++++
>  drivers/thermal/qcom/tsens.h        |  1 +
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/qcom/tsens-8996.c b/drivers/thermal/qcom/tsens-8996.c
> index e1f7781..60765b1 100644
> --- a/drivers/thermal/qcom/tsens-8996.c
> +++ b/drivers/thermal/qcom/tsens-8996.c
> @@ -28,7 +28,7 @@ static int get_temp_8996(struct tsens_device *tmdev, int id, int *temp)
>  	unsigned int sensor_addr;
>  	int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
>  
> -	sensor_addr = STATUS_OFFSET + s->hw_id * 4;
> +	sensor_addr = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
>  	ret = regmap_read(tmdev->map, sensor_addr, &code);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index b1449ad..4a741b0 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -16,6 +16,7 @@
>  #include <linux/io.h>
>  #include <linux/nvmem-consumer.h>
>  #include <linux/of_address.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include "tsens.h"
> @@ -126,11 +127,21 @@ static const struct regmap_config tsens_config = {
>  int __init init_common(struct tsens_device *tmdev)
>  {
>  	void __iomem *base;
> +	struct platform_device *op = of_find_device_by_node(tmdev->dev->of_node);
>  
> +	if (!op)
> +		return -EINVAL;
>  	base = of_iomap(tmdev->dev->of_node, 0);
>  	if (!base)
>  		return -EINVAL;
>  
> +	if (op->num_resources > 1) {
> +		tmdev->tm_offset = 0;
> +	} else {
> +		/* old DTs where SROT and TM were in a contiguous 2K block */
> +		tmdev->tm_offset = 0x1000;
> +	}
> +
>  	tmdev->map = devm_regmap_init_mmio(tmdev->dev, base, &tsens_config);
>  	if (IS_ERR(tmdev->map)) {
>  		iounmap(base);
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index dc56e1e..d785b37 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -77,6 +77,7 @@ struct tsens_device {
>  	struct device			*dev;
>  	u32				num_sensors;
>  	struct regmap			*map;
> +	u32				tm_offset;
>  	struct tsens_context		ctx;
>  	const struct tsens_ops		*ops;
>  	struct tsens_sensor		sensor[0];
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 3/7] dt: qcom: 8996: thermal: Move to DT initialisation
  2018-07-09 11:43 ` [PATCH v6 3/7] dt: qcom: 8996: thermal: Move to DT initialisation Amit Kucheria
@ 2018-07-11 16:37   ` Bjorn Andersson
  2018-07-11 18:39   ` Doug Anderson
  1 sibling, 0 replies; 27+ messages in thread
From: Bjorn Andersson @ 2018-07-11 16:37 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: linux-kernel, rnayak, linux-arm-msm, edubezval, smohanad,
	vivek.gautam, andy.gross, David Brown, Rob Herring, Mark Rutland,
	Catalin Marinas, Will Deacon, linux-soc, devicetree,
	linux-arm-kernel

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
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 4/7] thermal: tsens: Rename tsens-8996 to tsens-v2 for reuse
  2018-07-09 11:43 ` [PATCH v6 4/7] thermal: tsens: Rename tsens-8996 to tsens-v2 for reuse Amit Kucheria
@ 2018-07-11 16:39   ` Bjorn Andersson
  0 siblings, 0 replies; 27+ messages in thread
From: Bjorn Andersson @ 2018-07-11 16:39 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: linux-kernel, rnayak, linux-arm-msm, edubezval, smohanad,
	vivek.gautam, andy.gross, Zhang Rui, linux-pm

On Mon 09 Jul 04:43 PDT 2018, Amit Kucheria wrote:

> The TSENS block inside the 8996 is internally classified as version 2 of
> the IP. Several other SoC families use this block and can share this code.
> 
> We rename get_temp() to reflect that it can be used across the v2 family.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/thermal/qcom/Makefile                     |  2 +-
>  drivers/thermal/qcom/{tsens-8996.c => tsens-v2.c} | 26 ++++++++---------------
>  2 files changed, 10 insertions(+), 18 deletions(-)
>  rename drivers/thermal/qcom/{tsens-8996.c => tsens-v2.c} (66%)
> 
> diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
> index 2cc2193..a821929 100644
> --- a/drivers/thermal/qcom/Makefile
> +++ b/drivers/thermal/qcom/Makefile
> @@ -1,2 +1,2 @@
>  obj-$(CONFIG_QCOM_TSENS)	+= qcom_tsens.o
> -qcom_tsens-y			+= tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-8996.o
> +qcom_tsens-y			+= tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-v2.o
> diff --git a/drivers/thermal/qcom/tsens-8996.c b/drivers/thermal/qcom/tsens-v2.c
> similarity index 66%
> rename from drivers/thermal/qcom/tsens-8996.c
> rename to drivers/thermal/qcom/tsens-v2.c
> index 60765b1..34ba6c7 100644
> --- a/drivers/thermal/qcom/tsens-8996.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -1,27 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 and
> - * only version 2 as published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> + * Copyright (c) 2018, Linaro Limited
>   */
>  
> -#include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include "tsens.h"
>  
> -#define STATUS_OFFSET	0x10a0
> -#define LAST_TEMP_MASK	0xfff
> +#define STATUS_OFFSET		0xa0
> +#define LAST_TEMP_MASK		0xfff
>  #define STATUS_VALID_BIT	BIT(21)
>  #define CODE_SIGN_BIT		BIT(11)
>  
> -static int get_temp_8996(struct tsens_device *tmdev, int id, int *temp)
> +static int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
>  {
>  	struct tsens_sensor *s = &tmdev->sensor[id];
>  	u32 code;
> @@ -73,12 +64,13 @@ static int get_temp_8996(struct tsens_device *tmdev, int id, int *temp)
>  	return 0;
>  }
>  
> -static const struct tsens_ops ops_8996 = {
> +static const struct tsens_ops ops_generic_v2 = {
>  	.init		= init_common,
> -	.get_temp	= get_temp_8996,
> +	.get_temp	= get_temp_tsens_v2,
>  };
>  
>  const struct tsens_data data_8996 = {
>  	.num_sensors	= 13,
> -	.ops		= &ops_8996,
> +	.ops		= &ops_generic_v2,
>  };
> +
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 5/7] thermal: tsens: Add generic support for TSENS v2 IP
  2018-07-09 11:43 ` [PATCH v6 5/7] thermal: tsens: Add generic support for TSENS v2 IP Amit Kucheria
@ 2018-07-11 16:40   ` Bjorn Andersson
  2018-07-11 18:40   ` Doug Anderson
  1 sibling, 0 replies; 27+ messages in thread
From: Bjorn Andersson @ 2018-07-11 16:40 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: linux-kernel, rnayak, linux-arm-msm, edubezval, smohanad,
	vivek.gautam, andy.gross, Zhang Rui, Kees Cook, Rob Herring,
	linux-pm

On Mon 09 Jul 04:43 PDT 2018, Amit Kucheria wrote:

> SDM845 uses v2 of the TSENS IP block but the get_temp() function
> appears to be identical across v2.x.y in code seen so far. We use the
> generic get_temp() function defined as part of ops_generic_v2.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/thermal/qcom/tsens-v2.c | 6 +++++-
>  drivers/thermal/qcom/tsens.c    | 3 +++
>  drivers/thermal/qcom/tsens.h    | 5 ++++-
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index 34ba6c7..f40150f 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -69,8 +69,12 @@ static const struct tsens_ops ops_generic_v2 = {
>  	.get_temp	= get_temp_tsens_v2,
>  };
>  
> +const struct tsens_data data_tsens_v2 = {
> +	.ops            = &ops_generic_v2,
> +};
> +
> +/* Kept around for backward compatibility with old msm8996.dtsi */
>  const struct tsens_data data_8996 = {
>  	.num_sensors	= 13,
>  	.ops		= &ops_generic_v2,
>  };
> -
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 3440166c..a2c9bfa 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -72,6 +72,9 @@ static const struct of_device_id tsens_table[] = {
>  	}, {
>  		.compatible = "qcom,msm8996-tsens",
>  		.data = &data_8996,
> +	}, {
> +		.compatible = "qcom,tsens-v2",
> +		.data = &data_tsens_v2,
>  	},
>  	{}
>  };
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index d785b37..14331eb 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -88,6 +88,9 @@ void compute_intercept_slope(struct tsens_device *, u32 *, u32 *, u32);
>  int init_common(struct tsens_device *);
>  int get_temp_common(struct tsens_device *, int, int *);
>  
> -extern const struct tsens_data data_8916, data_8974, data_8960, data_8996;
> +/* TSENS v1 targets */
> +extern const struct tsens_data data_8916, data_8974, data_8960;
> +/* TSENS v2 targets */
> +extern const struct tsens_data data_8996, data_tsens_v2;
>  
>  #endif /* __QCOM_TSENS_H__ */
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 7/7] arm64: dts: sdm845: Add tsens nodes
  2018-07-09 11:43 ` [PATCH v6 7/7] arm64: dts: sdm845: Add tsens nodes Amit Kucheria
@ 2018-07-11 16:41   ` Bjorn Andersson
  2018-07-11 18:44   ` Doug Anderson
  1 sibling, 0 replies; 27+ messages in thread
From: Bjorn Andersson @ 2018-07-11 16:41 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: linux-kernel, rnayak, linux-arm-msm, edubezval, smohanad,
	vivek.gautam, andy.gross, David Brown, Rob Herring, Mark Rutland,
	Catalin Marinas, Will Deacon, linux-soc, devicetree,
	linux-arm-kernel

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
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 6/7] dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP
  2018-07-09 11:43 ` [PATCH v6 6/7] dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP Amit Kucheria
  2018-07-11 13:49   ` Rob Herring
@ 2018-07-11 16:42   ` Bjorn Andersson
  2018-07-11 18:42   ` Doug Anderson
  2 siblings, 0 replies; 27+ messages in thread
From: Bjorn Andersson @ 2018-07-11 16:42 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: linux-kernel, rnayak, linux-arm-msm, edubezval, smohanad,
	vivek.gautam, andy.gross, Zhang Rui, Rob Herring, Mark Rutland,
	linux-pm, devicetree

On Mon 09 Jul 04:43 PDT 2018, Amit Kucheria wrote:

> We want to create common code for v2 of the TSENS IP block that is used in
> a large number of Qualcomm SoCs. "qcom,tsens-v2" should be able to handle
> most of the common functionality start with a common get_temp() function.
> 
> It is also necessary to split out the memory regions for the TM and SROT
> register banks because their offsets are not constant across SoC families.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  .../devicetree/bindings/thermal/qcom-tsens.txt     | 25 +++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> index 06195e8..8f963b1 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> @@ -1,10 +1,16 @@
>  * QCOM SoC Temperature Sensor (TSENS)
>  
>  Required properties:
> -- compatible :
> - - "qcom,msm8916-tsens" : For 8916 Family of SoCs
> - - "qcom,msm8974-tsens" : For 8974 Family of SoCs
> - - "qcom,msm8996-tsens" : For 8996 Family of SoCs
> +- compatible:
> +  Must be one of the following:
> +    - "qcom,msm8916-tsens" (MSM8916)
> +    - "qcom,msm8974-tsens" (MSM8974)
> +    - "qcom,msm8996-tsens" (MSM8996)
> +    - "qcom,msm8998-tsens", "qcom,tsens-v2" (MSM8998)
> +    - "qcom,sdm845-tsens", "qcom,tsens-v2" (SDM845)
> +  The generic "qcom,tsens-v2" property must be used as a fallback for any SoC with
> +  version 2 of the TSENS IP. MSM8996 is the only exception beacause the generic
> +  property did not exist when support was added.
>  
>  - reg: Address range of the thermal registers
>  - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
> @@ -12,7 +18,7 @@ Required properties:
>  - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify
>  nvmem cells
>  
> -Example:
> +Example 1 (legacy support before a fallback tsens-v2 propoerty was introduced):
>  tsens: thermal-sensor@900000 {
>  		compatible = "qcom,msm8916-tsens";
>  		reg = <0x4a8000 0x2000>;
> @@ -20,3 +26,12 @@ tsens: thermal-sensor@900000 {
>  		nvmem-cell-names = "caldata", "calsel";
>  		#thermal-sensor-cells = <1>;
>  	};
> +
> +Example 2 (for any platform containing v2 of the TSENS IP):
> +tsens0: tsens@c222000 {
> +		compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
> +		reg = <0xc263000 0x1ff>, /* TM */
> +			<0xc222000 0x1ff>; /* SROT */
> +		#qcom,sensors = <13>;
> +		#thermal-sensor-cells = <1>;
> +	};
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two
  2018-07-09 11:43 ` [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two Amit Kucheria
  2018-07-11 16:34   ` Bjorn Andersson
@ 2018-07-11 18:37   ` Doug Anderson
  2018-07-12  4:40     ` Amit Kucheria
  1 sibling, 1 reply; 27+ messages in thread
From: Doug Anderson @ 2018-07-11 18:37 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: LKML, Rajendra Nayak, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, smohanad, Vivek Gautam, Andy Gross, Zhang Rui,
	linux-pm

Hi,

On Mon, Jul 9, 2018 at 4:43 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
> There are two banks of registers for v2 TSENS IPs: SROT and TM. On older
> SoCs these were contiguous, leading to DTs mapping them as one register
> address space of size 0x2000. In newer SoCs, these two banks are not
> contiguous anymore.
>
> Fixing old DTs to split the address space into allows us to have cleaner
> common code e.g. get_temp() that is shared across new and old platforms.

This makes it sound like old DTs won't be supported anymore.  ...but
the code says otherwise.  I'd just remove the above paragraph.


> @@ -126,11 +127,21 @@ static const struct regmap_config tsens_config = {
>  int __init init_common(struct tsens_device *tmdev)
>  {
>         void __iomem *base;
> +       struct platform_device *op = of_find_device_by_node(tmdev->dev->of_node);
>
> +       if (!op)
> +               return -EINVAL;
>         base = of_iomap(tmdev->dev->of_node, 0);
>         if (!base)
>                 return -EINVAL;
>
> +       if (op->num_resources > 1) {

Maybe add a comment here that says that we don't actually map the SROT
yet because you don't read anything from there?  I kept getting
confused about how this patch could possibly work with no code to map
SROT...

> +               tmdev->tm_offset = 0;
> +       } else {
> +               /* old DTs where SROT and TM were in a contiguous 2K block */
> +               tmdev->tm_offset = 0x1000;

This patch without patch #4 will break compatibility.  You should
squash part of patch #4 into this one, specifically:

-#define STATUS_OFFSET 0x10a0
-#define LAST_TEMP_MASK 0xfff
+#define STATUS_OFFSET 0xa0
+#define LAST_TEMP_MASK 0xfff

Without that you break bisect-ability and also confuse anyone trying
to look at this patch.

-Doug

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 3/7] dt: qcom: 8996: thermal: Move to DT initialisation
  2018-07-09 11:43 ` [PATCH v6 3/7] dt: qcom: 8996: thermal: Move to DT initialisation Amit Kucheria
  2018-07-11 16:37   ` Bjorn Andersson
@ 2018-07-11 18:39   ` Doug Anderson
  2018-07-12  5:03     ` Amit Kucheria
  1 sibling, 1 reply; 27+ messages in thread
From: Doug Anderson @ 2018-07-11 18:39 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: LKML, Rajendra Nayak, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, smohanad, Vivek Gautam, Andy Gross,
	David Brown, Rob Herring, Mark Rutland, Catalin Marinas,
	Will Deacon, open list:ARM/QUALCOMM SUPPORT, devicetree,
	Linux ARM

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 5/7] thermal: tsens: Add generic support for TSENS v2 IP
  2018-07-09 11:43 ` [PATCH v6 5/7] thermal: tsens: Add generic support for TSENS v2 IP Amit Kucheria
  2018-07-11 16:40   ` Bjorn Andersson
@ 2018-07-11 18:40   ` Doug Anderson
  2018-07-12  5:12     ` Amit Kucheria
  1 sibling, 1 reply; 27+ messages in thread
From: Doug Anderson @ 2018-07-11 18:40 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: LKML, Rajendra Nayak, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, smohanad, Vivek Gautam, Andy Gross, Zhang Rui,
	Kees Cook, Rob Herring, linux-pm

Hi,

On Mon, Jul 9, 2018 at 4:43 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
> SDM845 uses v2 of the TSENS IP block but the get_temp() function
> appears to be identical across v2.x.y in code seen so far. We use the
> generic get_temp() function defined as part of ops_generic_v2.
>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/thermal/qcom/tsens-v2.c | 6 +++++-
>  drivers/thermal/qcom/tsens.c    | 3 +++
>  drivers/thermal/qcom/tsens.h    | 5 ++++-
>  3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index 34ba6c7..f40150f 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -69,8 +69,12 @@ static const struct tsens_ops ops_generic_v2 = {
>         .get_temp       = get_temp_tsens_v2,
>  };
>
> +const struct tsens_data data_tsens_v2 = {
> +       .ops            = &ops_generic_v2,
> +};
> +
> +/* Kept around for backward compatibility with old msm8996.dtsi */
>  const struct tsens_data data_8996 = {
>         .num_sensors    = 13,
>         .ops            = &ops_generic_v2,
>  };

Something seems fishy here.  You have a ".num_sensors" for sdm8996
hardcoded to 13 but you don't have a ".num_sensors" for your new v2.
Where does num_sensors get set for everyone else?  In patch #3 you
have a new "#qcom,sensors" but:

1. Nothing reads this as far as I can tell, so that means everyone
will end up with 0 sensors.

2. On your 2nd block of sensors in the sdm8996 device tree (see
earlier patch in the series) you try to set qcom,sensors to 8.  ...but
since you still have a compatible of "qcom,msm8996-tsens" you'll get
13.  That seems wrong.  ...or did I miss something?

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 6/7] dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP
  2018-07-09 11:43 ` [PATCH v6 6/7] dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP Amit Kucheria
  2018-07-11 13:49   ` Rob Herring
  2018-07-11 16:42   ` Bjorn Andersson
@ 2018-07-11 18:42   ` Doug Anderson
  2018-07-12  5:56     ` Amit Kucheria
  2 siblings, 1 reply; 27+ messages in thread
From: Doug Anderson @ 2018-07-11 18:42 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: LKML, Rajendra Nayak, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, smohanad, Vivek Gautam, Andy Gross, Zhang Rui,
	Rob Herring, Mark Rutland, linux-pm, devicetree

Hi,

On Mon, Jul 9, 2018 at 4:43 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
> We want to create common code for v2 of the TSENS IP block that is used in
> a large number of Qualcomm SoCs. "qcom,tsens-v2" should be able to handle
> most of the common functionality start with a common get_temp() function.
>
> It is also necessary to split out the memory regions for the TM and SROT
> register banks because their offsets are not constant across SoC families.

nit that bindings should be earlier in the patch series than the code
implementing the bindings.


> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  .../devicetree/bindings/thermal/qcom-tsens.txt     | 25 +++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> index 06195e8..8f963b1 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> @@ -1,10 +1,16 @@
>  * QCOM SoC Temperature Sensor (TSENS)
>
>  Required properties:
> -- compatible :
> - - "qcom,msm8916-tsens" : For 8916 Family of SoCs
> - - "qcom,msm8974-tsens" : For 8974 Family of SoCs
> - - "qcom,msm8996-tsens" : For 8996 Family of SoCs
> +- compatible:
> +  Must be one of the following:
> +    - "qcom,msm8916-tsens" (MSM8916)
> +    - "qcom,msm8974-tsens" (MSM8974)
> +    - "qcom,msm8996-tsens" (MSM8996)
> +    - "qcom,msm8998-tsens", "qcom,tsens-v2" (MSM8998)
> +    - "qcom,sdm845-tsens", "qcom,tsens-v2" (SDM845)
> +  The generic "qcom,tsens-v2" property must be used as a fallback for any SoC with
> +  version 2 of the TSENS IP. MSM8996 is the only exception beacause the generic
> +  property did not exist when support was added.
>
>  - reg: Address range of the thermal registers

You need to document that for old SoCs where TM / SROT were 0x1000
apart (SROT first) that one "reg" field was OK.  ...and that for new
SoCs you specify two reg ranges: the first for TM and the second for
SROT.


>  - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
> @@ -12,7 +18,7 @@ Required properties:
>  - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify
>  nvmem cells
>
> -Example:
> +Example 1 (legacy support before a fallback tsens-v2 propoerty was introduced):
>  tsens: thermal-sensor@900000 {
>                 compatible = "qcom,msm8916-tsens";
>                 reg = <0x4a8000 0x2000>;
> @@ -20,3 +26,12 @@ tsens: thermal-sensor@900000 {
>                 nvmem-cell-names = "caldata", "calsel";
>                 #thermal-sensor-cells = <1>;
>         };
> +
> +Example 2 (for any platform containing v2 of the TSENS IP):
> +tsens0: tsens@c222000 {

A) Use a generic name for the node, not a specific one.  Thus the node
should be "thermal-sensor", not "tsens".

B) This unit address needs to match the _first_ reg address listed.
Give your reg below, this should be @c263000

Thus your node name should be:

tsens0: thermal-sensor@c263000 {

> +               compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
> +               reg = <0xc263000 0x1ff>, /* TM */
> +                       <0xc222000 0x1ff>; /* SROT */
> +               #qcom,sensors = <13>;

The "#qcom,sensors" property seems wrong in a few ways:

A) I wouldn't have expected it to start with a "#".  I only expect to
see that on things specifying sizes / lengths.  Rob can feel free to
override me, though.

B) It's not documented above.  Just putting something in an example
doesn't document it--it needs to be listed in the "Optional
properties".

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 7/7] arm64: dts: sdm845: Add tsens nodes
  2018-07-09 11:43 ` [PATCH v6 7/7] arm64: dts: sdm845: Add tsens nodes Amit Kucheria
  2018-07-11 16:41   ` Bjorn Andersson
@ 2018-07-11 18:44   ` Doug Anderson
  2018-07-11 21:51     ` Matthias Kaehlcke
  1 sibling, 1 reply; 27+ messages in thread
From: Doug Anderson @ 2018-07-11 18:44 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: LKML, Rajendra Nayak, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, smohanad, Vivek Gautam, Andy Gross,
	David Brown, Rob Herring, Mark Rutland, Catalin Marinas,
	Will Deacon, open list:ARM/QUALCOMM SUPPORT, devicetree,
	Linux ARM

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 7/7] arm64: dts: sdm845: Add tsens nodes
  2018-07-11 18:44   ` Doug Anderson
@ 2018-07-11 21:51     ` Matthias Kaehlcke
  2018-07-11 22:00       ` Doug Anderson
  0 siblings, 1 reply; 27+ messages in thread
From: Matthias Kaehlcke @ 2018-07-11 21:51 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Amit Kucheria, LKML, Rajendra Nayak, linux-arm-msm,
	Bjorn Andersson, Eduardo Valentin, smohanad, Vivek Gautam,
	Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Catalin Marinas, Will Deacon, open list:ARM/QUALCOMM SUPPORT,
	devicetree, Linux ARM

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 ;-)

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 7/7] arm64: dts: sdm845: Add tsens nodes
  2018-07-11 21:51     ` Matthias Kaehlcke
@ 2018-07-11 22:00       ` Doug Anderson
  2018-07-12  5:36         ` Amit Kucheria
  0 siblings, 1 reply; 27+ messages in thread
From: Doug Anderson @ 2018-07-11 22:00 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Amit Kucheria, LKML, Rajendra Nayak, linux-arm-msm,
	Bjorn Andersson, Eduardo Valentin, smohanad, Vivek Gautam,
	Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Catalin Marinas, Will Deacon, open list:ARM/QUALCOMM SUPPORT,
	devicetree, Linux ARM

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two
  2018-07-11 18:37   ` Doug Anderson
@ 2018-07-12  4:40     ` Amit Kucheria
  0 siblings, 0 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-12  4:40 UTC (permalink / raw)
  To: Doug Anderson
  Cc: LKML, Rajendra Nayak, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, smohanad, Vivek Gautam, Andy Gross, Zhang Rui,
	Linux PM list

On Thu, Jul 12, 2018 at 12:07 AM, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Mon, Jul 9, 2018 at 4:43 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
>> There are two banks of registers for v2 TSENS IPs: SROT and TM. On older
>> SoCs these were contiguous, leading to DTs mapping them as one register
>> address space of size 0x2000. In newer SoCs, these two banks are not
>> contiguous anymore.
>>
>> Fixing old DTs to split the address space into allows us to have cleaner
>> common code e.g. get_temp() that is shared across new and old platforms.
>
> This makes it sound like old DTs won't be supported anymore.  ...but
> the code says otherwise.  I'd just remove the above paragraph.

OK.

>
>> @@ -126,11 +127,21 @@ static const struct regmap_config tsens_config = {
>>  int __init init_common(struct tsens_device *tmdev)
>>  {
>>         void __iomem *base;
>> +       struct platform_device *op = of_find_device_by_node(tmdev->dev->of_node);
>>
>> +       if (!op)
>> +               return -EINVAL;
>>         base = of_iomap(tmdev->dev->of_node, 0);
>>         if (!base)
>>                 return -EINVAL;
>>
>> +       if (op->num_resources > 1) {
>
> Maybe add a comment here that says that we don't actually map the SROT
> yet because you don't read anything from there?  I kept getting
> confused about how this patch could possibly work with no code to map
> SROT...

OK. The SROT comment got separated (patch 3) during patch refactoring.
Will add a comment.

>> +               tmdev->tm_offset = 0;
>> +       } else {
>> +               /* old DTs where SROT and TM were in a contiguous 2K block */
>> +               tmdev->tm_offset = 0x1000;
>
> This patch without patch #4 will break compatibility.  You should
> squash part of patch #4 into this one, specifically:
>
> -#define STATUS_OFFSET 0x10a0
> -#define LAST_TEMP_MASK 0xfff
> +#define STATUS_OFFSET 0xa0
> +#define LAST_TEMP_MASK 0xfff
>
> Without that you break bisect-ability and also confuse anyone trying
> to look at this patch.

Thanks. Will fix.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 3/7] dt: qcom: 8996: thermal: Move to DT initialisation
  2018-07-11 18:39   ` Doug Anderson
@ 2018-07-12  5:03     ` Amit Kucheria
  0 siblings, 0 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-12  5:03 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Linux Kernel Mailing List, Rajendra Nayak, linux-arm-msm,
	Bjorn Andersson, Eduardo Valentin, smohanad, Vivek Gautam,
	Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Catalin Marinas, Will Deacon, open list:ARM/QUALCOMM SUPPORT,
	DTML, Lists LAKML

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 5/7] thermal: tsens: Add generic support for TSENS v2 IP
  2018-07-11 18:40   ` Doug Anderson
@ 2018-07-12  5:12     ` Amit Kucheria
  0 siblings, 0 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-12  5:12 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Linux Kernel Mailing List, Rajendra Nayak, linux-arm-msm,
	Bjorn Andersson, Eduardo Valentin, smohanad, Vivek Gautam,
	Andy Gross, Zhang Rui, Kees Cook, Rob Herring, Linux PM list

On Thu, Jul 12, 2018 at 12:10 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Jul 9, 2018 at 4:43 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
> > SDM845 uses v2 of the TSENS IP block but the get_temp() function
> > appears to be identical across v2.x.y in code seen so far. We use the
> > generic get_temp() function defined as part of ops_generic_v2.
> >
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> >  drivers/thermal/qcom/tsens-v2.c | 6 +++++-
> >  drivers/thermal/qcom/tsens.c    | 3 +++
> >  drivers/thermal/qcom/tsens.h    | 5 ++++-
> >  3 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> > index 34ba6c7..f40150f 100644
> > --- a/drivers/thermal/qcom/tsens-v2.c
> > +++ b/drivers/thermal/qcom/tsens-v2.c
> > @@ -69,8 +69,12 @@ static const struct tsens_ops ops_generic_v2 = {
> >         .get_temp       = get_temp_tsens_v2,
> >  };
> >
> > +const struct tsens_data data_tsens_v2 = {
> > +       .ops            = &ops_generic_v2,
> > +};
> > +
> > +/* Kept around for backward compatibility with old msm8996.dtsi */
> >  const struct tsens_data data_8996 = {
> >         .num_sensors    = 13,
> >         .ops            = &ops_generic_v2,
> >  };
>
> Something seems fishy here.  You have a ".num_sensors" for sdm8996
> hardcoded to 13 but you don't have a ".num_sensors" for your new v2.
> Where does num_sensors get set for everyone else?  In patch #3 you
> have a new "#qcom,sensors" but:
>
> 1. Nothing reads this as far as I can tell, so that means everyone
> will end up with 0 sensors.
>
> 2. On your 2nd block of sensors in the sdm8996 device tree (see
> earlier patch in the series) you try to set qcom,sensors to 8.  ...but
> since you still have a compatible of "qcom,msm8996-tsens" you'll get
> 13.  That seems wrong.  ...or did I miss something?

Refer to commit 6d7c70d1cd6 (thermal: qcom: tsens: Allow number of
sensors to come from DT) merged earlier. #qcom,sensors will override
the platform data if defined.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 7/7] arm64: dts: sdm845: Add tsens nodes
  2018-07-11 22:00       ` Doug Anderson
@ 2018-07-12  5:36         ` Amit Kucheria
  0 siblings, 0 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-12  5:36 UTC (permalink / raw)
  To: Doug Anderson
  Cc: mka, Linux Kernel Mailing List, Rajendra Nayak, linux-arm-msm,
	Bjorn Andersson, Eduardo Valentin, smohanad, Vivek Gautam,
	Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Catalin Marinas, Will Deacon, open list:ARM/QUALCOMM SUPPORT,
	DTML, Lists LAKML

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6 6/7] dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP
  2018-07-11 18:42   ` Doug Anderson
@ 2018-07-12  5:56     ` Amit Kucheria
  0 siblings, 0 replies; 27+ messages in thread
From: Amit Kucheria @ 2018-07-12  5:56 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Linux Kernel Mailing List, Rajendra Nayak, linux-arm-msm,
	Bjorn Andersson, Eduardo Valentin, smohanad, Vivek Gautam,
	Andy Gross, Zhang Rui, Rob Herring, Mark Rutland, Linux PM list,
	DTML

On Thu, Jul 12, 2018 at 12:12 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 want to create common code for v2 of the TSENS IP block that is used in
> > a large number of Qualcomm SoCs. "qcom,tsens-v2" should be able to handle
> > most of the common functionality start with a common get_temp() function.
> >
> > It is also necessary to split out the memory regions for the TM and SROT
> > register banks because their offsets are not constant across SoC families.
>
> nit that bindings should be earlier in the patch series than the code
> implementing the bindings.

Will reorder.

>
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> >  .../devicetree/bindings/thermal/qcom-tsens.txt     | 25 +++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > index 06195e8..8f963b1 100644
> > --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > @@ -1,10 +1,16 @@
> >  * QCOM SoC Temperature Sensor (TSENS)
> >
> >  Required properties:
> > -- compatible :
> > - - "qcom,msm8916-tsens" : For 8916 Family of SoCs
> > - - "qcom,msm8974-tsens" : For 8974 Family of SoCs
> > - - "qcom,msm8996-tsens" : For 8996 Family of SoCs
> > +- compatible:
> > +  Must be one of the following:
> > +    - "qcom,msm8916-tsens" (MSM8916)
> > +    - "qcom,msm8974-tsens" (MSM8974)
> > +    - "qcom,msm8996-tsens" (MSM8996)
> > +    - "qcom,msm8998-tsens", "qcom,tsens-v2" (MSM8998)
> > +    - "qcom,sdm845-tsens", "qcom,tsens-v2" (SDM845)
> > +  The generic "qcom,tsens-v2" property must be used as a fallback for any SoC with
> > +  version 2 of the TSENS IP. MSM8996 is the only exception beacause the generic
> > +  property did not exist when support was added.
> >
> >  - reg: Address range of the thermal registers
>
> You need to document that for old SoCs where TM / SROT were 0x1000
> apart (SROT first) that one "reg" field was OK.  ...and that for new
> SoCs you specify two reg ranges: the first for TM and the second for
> SROT.

OK.

> >  - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
> > @@ -12,7 +18,7 @@ Required properties:
> >  - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify
> >  nvmem cells
> >
> > -Example:
> > +Example 1 (legacy support before a fallback tsens-v2 propoerty was introduced):
> >  tsens: thermal-sensor@900000 {
> >                 compatible = "qcom,msm8916-tsens";
> >                 reg = <0x4a8000 0x2000>;
> > @@ -20,3 +26,12 @@ tsens: thermal-sensor@900000 {
> >                 nvmem-cell-names = "caldata", "calsel";
> >                 #thermal-sensor-cells = <1>;
> >         };
> > +
> > +Example 2 (for any platform containing v2 of the TSENS IP):
> > +tsens0: tsens@c222000 {
>
> A) Use a generic name for the node, not a specific one.  Thus the node
> should be "thermal-sensor", not "tsens".
>
> B) This unit address needs to match the _first_ reg address listed.
> Give your reg below, this should be @c263000
>
> Thus your node name should be:
>
> tsens0: thermal-sensor@c263000 {

Fixed

> > +               compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
> > +               reg = <0xc263000 0x1ff>, /* TM */
> > +                       <0xc222000 0x1ff>; /* SROT */
> > +               #qcom,sensors = <13>;
>
> The "#qcom,sensors" property seems wrong in a few ways:
>
> A) I wouldn't have expected it to start with a "#".  I only expect to
> see that on things specifying sizes / lengths.  Rob can feel free to
> override me, though.
>
> B) It's not documented above.  Just putting something in an example
> doesn't document it--it needs to be listed in the "Optional
> properties".

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2018-07-12  5:56 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-09 11:43 [PATCH v6 0/7] thermal: tsens: Refactoring for TSENSv2 IP Amit Kucheria
2018-07-09 11:43 ` [PATCH v6 1/7] thermal: tsens: Get rid of unused fields in structure Amit Kucheria
2018-07-09 11:43 ` [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two Amit Kucheria
2018-07-11 16:34   ` Bjorn Andersson
2018-07-11 18:37   ` Doug Anderson
2018-07-12  4:40     ` Amit Kucheria
2018-07-09 11:43 ` [PATCH v6 3/7] dt: qcom: 8996: thermal: Move to DT initialisation Amit Kucheria
2018-07-11 16:37   ` Bjorn Andersson
2018-07-11 18:39   ` Doug Anderson
2018-07-12  5:03     ` Amit Kucheria
2018-07-09 11:43 ` [PATCH v6 4/7] thermal: tsens: Rename tsens-8996 to tsens-v2 for reuse Amit Kucheria
2018-07-11 16:39   ` Bjorn Andersson
2018-07-09 11:43 ` [PATCH v6 5/7] thermal: tsens: Add generic support for TSENS v2 IP Amit Kucheria
2018-07-11 16:40   ` Bjorn Andersson
2018-07-11 18:40   ` Doug Anderson
2018-07-12  5:12     ` Amit Kucheria
2018-07-09 11:43 ` [PATCH v6 6/7] dt: thermal: tsens: Document the fallback DT property for v2 of TSENS IP Amit Kucheria
2018-07-11 13:49   ` Rob Herring
2018-07-11 16:42   ` Bjorn Andersson
2018-07-11 18:42   ` Doug Anderson
2018-07-12  5:56     ` Amit Kucheria
2018-07-09 11:43 ` [PATCH v6 7/7] arm64: dts: sdm845: Add tsens nodes Amit Kucheria
2018-07-11 16:41   ` Bjorn Andersson
2018-07-11 18:44   ` Doug Anderson
2018-07-11 21:51     ` Matthias Kaehlcke
2018-07-11 22:00       ` Doug Anderson
2018-07-12  5:36         ` Amit Kucheria

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).