Re: [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two
On Thu, Jul 12, 2018 at 12:07 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jul 9, 2018 at 4:43 AM, 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.
>
> 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.
Re: [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two
Hi,
On Mon, Jul 9, 2018 at 4:43 AM, 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.
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
Re: [PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two
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
Reviewed-by: Bjorn Andersson
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
> #include
> #include
> +#include
> #include
> #include
> #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_contextctx;
> const struct tsens_ops *ops;
> struct tsens_sensor sensor[0];
> --
> 2.7.4
>
[PATCH v6 2/7] thermal: tsens: Add support to split up register address space into two
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
---
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
#include
#include
+#include
#include
#include
#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_contextctx;
const struct tsens_ops *ops;
struct tsens_sensor sensor[0];
--
2.7.4
