dt-bindings: thermal/armada: describe AP806 and CP110

The CP110 component is integrated in the Armada 8k and 7k lines of processors.

This patch also adds an option of offset to the MSB of the control register. The existing DT binding for Armada 38x refers to a single 32 bit control register. It turns out that this is actually only the MSB of the control area. Changing the binding to fix that would break existing DT files, so the Armada 38x binding is left as is.

The new CP110 binding increases the size of the control area to 64 bits, thus moving the MSB to offset 4.

Signed-off-by Baruch Siach <[email protected]>

---
v2: No change

---
 drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/thermal/armada_thermal.c index 0eb82097571f..59b75f63945d 100644 --- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c

drivers/thermal/armada_thermal.c +1 -0

@@ -73,6 +73,7 @@ struct armada_thermal_data {
 	unsigned int temp_shift;
 	unsigned int temp_mask;
 	unsigned int is_valid_shift;
+	unsigned int control_msb_offset;
 };
 
 static void armadaxp_init_sensor(struct platform_device *pdev,

@@ -142,12 +143,14 @@ static void armada375_init_sensor(struct

platform_device *pdev, static void armada380_init_sensor(struct platform_device *pdev, struct armada_thermal_priv *priv)
{

drivers/thermal/armada_thermal.c +4 -2

-	unsigned long reg = readl_relaxed(priv->control);
+	void __iomem *control_msb =
+		priv->control + priv->data->control_msb_offset;
+	unsigned long reg = readl_relaxed(control_msb);
 
 	/* Reset hardware once */
 	if (!(reg & A380_HW_RESET)) {
 		reg |= A380_HW_RESET;
-		writel(reg, priv->control);
+		writel(reg, control_msb);
 		mdelay(10);
 	}
 }

Quoted 49 lines · On Sun, 3 Dec 2017 13:11:23 +0200 Baruch Siach <[email protected]> wrote:

The CP110 component is integrated in the Armada 8k and 7k lines of processors.

This patch also adds an option of offset to the MSB of the control register. The existing DT binding for Armada 38x refers to a single 32 bit control register. It turns out that this is actually only the MSB of the control area. Changing the binding to fix that would break existing DT files, so the Armada 38x binding is left as is.

The new CP110 binding increases the size of the control area to 64 bits, thus moving the MSB to offset 4.

Signed-off-by Baruch Siach <[email protected]>

---
v2: No change

---
 drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/thermal/armada_thermal.c index 0eb82097571f..59b75f63945d 100644 --- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c

drivers/thermal/armada_thermal.c +1 -0

@@ -73,6 +73,7 @@ struct armada_thermal_data {
 	unsigned int temp_shift;
 	unsigned int temp_mask;
 	unsigned int is_valid_shift;
+	unsigned int control_msb_offset;
 };
 
 static void armadaxp_init_sensor(struct platform_device *pdev,

@@ -142,12 +143,14 @@ static void armada375_init_sensor(struct

platform_device *pdev, static void armada380_init_sensor(struct platform_device *pdev, struct armada_thermal_priv *priv)
{

drivers/thermal/armada_thermal.c +4 -2

-	unsigned long reg = readl_relaxed(priv->control);
+	void __iomem *control_msb =
+		priv->control + priv->data->control_msb_offset;
+	unsigned long reg = readl_relaxed(control_msb);
 
 	/* Reset hardware once */
 	if (!(reg & A380_HW_RESET)) {
 		reg |= A380_HW_RESET;
-		writel(reg, priv->control);
+		writel(reg, control_msb);
 		mdelay(10);
 	}
 }

drivers/thermal/armada_thermal.c static const struct armada_thermal_data · line 274

Quoted 6 lines

drivers/thermal/armada_thermal.c +3 -0

@@ -266,6 +269,19 @@ static const struct armada_thermal_data
 armada_ap806_data = { .signed_sample = true,
 };
 
+static const struct armada_thermal_data armada_cp110_data = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada380_init_sensor,

I see the initialization for CP110 thermal IP is close to Armada-380's, but, as you point it in the commit log it is still different.

I don't know what is the best way to handle this but until now each new compatible had his own ->init_sensor function, shouldn't we do the same here as changes are requested? This would naturally avoid the situation with Armada-380 bindings.

Hi Miquel,

Quoted 63 lines · Miquel RAYNAL

Quoted 49 lines · On Sun, 3 Dec 2017 13:11:23 +0200 Baruch Siach <[email protected]> wrote:

The CP110 component is integrated in the Armada 8k and 7k lines of processors.

This patch also adds an option of offset to the MSB of the control register. The existing DT binding for Armada 38x refers to a single 32 bit control register. It turns out that this is actually only the MSB of the control area. Changing the binding to fix that would break existing DT files, so the Armada 38x binding is left as is.

The new CP110 binding increases the size of the control area to 64 bits, thus moving the MSB to offset 4.

Signed-off-by Baruch Siach <[email protected]>

---
v2: No change

---
 drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/thermal/armada_thermal.c index 0eb82097571f..59b75f63945d 100644 --- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c

drivers/thermal/armada_thermal.c +1 -0

@@ -73,6 +73,7 @@ struct armada_thermal_data {
 	unsigned int temp_shift;
 	unsigned int temp_mask;
 	unsigned int is_valid_shift;
+	unsigned int control_msb_offset;
 };
 
 static void armadaxp_init_sensor(struct platform_device *pdev,

@@ -142,12 +143,14 @@ static void armada375_init_sensor(struct

platform_device *pdev, static void armada380_init_sensor(struct platform_device *pdev, struct armada_thermal_priv *priv)
{

drivers/thermal/armada_thermal.c +4 -2

-	unsigned long reg = readl_relaxed(priv->control);
+	void __iomem *control_msb =
+		priv->control + priv->data->control_msb_offset;
+	unsigned long reg = readl_relaxed(control_msb);
 
 	/* Reset hardware once */
 	if (!(reg & A380_HW_RESET)) {
 		reg |= A380_HW_RESET;
-		writel(reg, priv->control);
+		writel(reg, control_msb);
 		mdelay(10);
 	}
 }

drivers/thermal/armada_thermal.c static const struct armada_thermal_data · line 274

Quoted 6 lines

drivers/thermal/armada_thermal.c +3 -0

@@ -266,6 +269,19 @@ static const struct armada_thermal_data
 armada_ap806_data = { .signed_sample = true,
 };
 
+static const struct armada_thermal_data armada_cp110_data = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada380_init_sensor,

I see the initialization for CP110 thermal IP is close to Armada-380's, but, as you point it in the commit log it is still different.

Quoted 4 lines

I don't know what is the best way to handle this but until now each new compatible had his own ->init_sensor function, shouldn't we do the same here as changes are requested? This would naturally avoid the situation with Armada-380 bindings.

I'm not sure I understand your suggestion.

There is no difference between the CP110 and the Armada 38x, as far as I can see. The only quirk is that the existing Armada 38x DT binding is wrong I that the 'reg' property references the control MSB, while leaving the LSB out. We

can't change the Armada 38x binding without breaking existing DTs. The 'control_msb_offset' field that this patch adds allows correct binding for CP110, while keeping compatibility with the existing Armada 38x binding.

How would a separate init_sensor routine improve things?

baruch

Signature · 3 lines

     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Quoted 69 lines · Baruch Siach

Quoted 63 lines · Miquel RAYNAL

Quoted 49 lines · On Sun, 3 Dec 2017 13:11:23 +0200 Baruch Siach <[email protected]> wrote:

The CP110 component is integrated in the Armada 8k and 7k lines of processors.

This patch also adds an option of offset to the MSB of the control register. The existing DT binding for Armada 38x refers to a single 32 bit control register. It turns out that this is actually only the MSB of the control area. Changing the binding to fix that would break existing DT files, so the Armada 38x binding is left as is.

The new CP110 binding increases the size of the control area to 64 bits, thus moving the MSB to offset 4.

Signed-off-by Baruch Siach <[email protected]>

---
v2: No change

---
 drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/thermal/armada_thermal.c index 0eb82097571f..59b75f63945d 100644 --- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c

drivers/thermal/armada_thermal.c +1 -0

@@ -73,6 +73,7 @@ struct armada_thermal_data {
 	unsigned int temp_shift;
 	unsigned int temp_mask;
 	unsigned int is_valid_shift;
+	unsigned int control_msb_offset;
 };
 
 static void armadaxp_init_sensor(struct platform_device *pdev,

@@ -142,12 +143,14 @@ static void armada375_init_sensor(struct

platform_device *pdev, static void armada380_init_sensor(struct platform_device *pdev, struct armada_thermal_priv *priv)
{

drivers/thermal/armada_thermal.c +4 -2

-	unsigned long reg = readl_relaxed(priv->control);
+	void __iomem *control_msb =
+		priv->control + priv->data->control_msb_offset;
+	unsigned long reg = readl_relaxed(control_msb);
 
 	/* Reset hardware once */
 	if (!(reg & A380_HW_RESET)) {
 		reg |= A380_HW_RESET;
-		writel(reg, priv->control);
+		writel(reg, control_msb);
 		mdelay(10);
 	}
 }

drivers/thermal/armada_thermal.c static const struct armada_thermal_data · line 274

Quoted 6 lines

drivers/thermal/armada_thermal.c +3 -0

@@ -266,6 +269,19 @@ static const struct armada_thermal_data
 armada_ap806_data = { .signed_sample = true,
 };
 
+static const struct armada_thermal_data armada_cp110_data = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada380_init_sensor,

I see the initialization for CP110 thermal IP is close to Armada-380's, but, as you point it in the commit log it is still different.

Quoted 4 lines

I don't know what is the best way to handle this but until now each new compatible had his own ->init_sensor function, shouldn't we do the same here as changes are requested? This would naturally avoid the situation with Armada-380 bindings.

Quoted 6 lines

I'm not sure I understand your suggestion.

There is no difference between the CP110 and the Armada 38x, as far as I can see. The only quirk is that the existing Armada 38x DT binding is wrong I that the 'reg' property references the control MSB, while leaving the LSB out. We

Well I would not say it was wrong but more incomplete :)

Quoted 4 lines

can't change the Armada 38x binding without breaking existing DTs. The 'control_msb_offset' field that this patch adds allows correct binding for CP110, while keeping compatibility with the existing Armada 38x binding.

I am not against adding a new compatible string for CP110 but ot be honest the new binding for CP110 does not bring anything as you don't use at all the LSB register.

Actually, if on Armada 375 we initially mapped the LSB register it was to support an very early release of the SoC (stepping Z) and only for resetting its value. So I guess you started to write the AP860 part based on the Armada 375 and then found that we could map a more complete range of the registers.

Quoted 1 line

How would a separate init_sensor routine improve things?

So yes please do it, thanks to this you won't have to add the control_msb_offset member and can use a clean function. Moreover if in the future we see some usefulness for this LSB register then we could use the new compatible for the Armada 38x.

Quoted 6 lines

Quoted 1 line

How would a separate init_sensor routine improve things?

So yes please do it, thanks to this you won't have to add the control_msb_offset member and can use a clean function. Moreover if in the future we see some usefulness for this LSB register then we could use the new compatible for the Armada 38x.

There are two separate issues here:

1. DT binding

2. init_sensor callback implementation

We both agree on #1. The A38x and CP110 need separate compatible strings. In case we want to access the LSB control register on Armada 38x, we will need yet another compatible string (marvell,armada380-v2-thermal maybe?).

As for #2, I'm all for sharing as much code as possible. I find the vendor kernel approach of duplicating the init routines[1] unhelpful as it violates the DRY principle. The differences between armada380_init_sensor() and cp110_init_sensor() are minor. In my opinion, these differences should be expressed explicitly in the armada_thermal_data, in a similar way to my suggested control_msb_offset field. The vendor code hides these differences in slight variations of duplicated code.

What is the advantage of a separate init routine?

Quoted 27 lines

Quoted 6 lines

Quoted 1 line

How would a separate init_sensor routine improve things?

So yes please do it, thanks to this you won't have to add the control_msb_offset member and can use a clean function. Moreover if in the future we see some usefulness for this LSB register then we could use the new compatible for the Armada 38x.

There are two separate issues here:

1. DT binding

2. init_sensor callback implementation

We both agree on #1. The A38x and CP110 need separate compatible strings. In case we want to access the LSB control register on Armada 38x, we will need yet another compatible string (marvell,armada380-v2-thermal maybe?).

As for #2, I'm all for sharing as much code as possible. I find the vendor kernel approach of duplicating the init routines[1] unhelpful as it violates the DRY principle. The differences between armada380_init_sensor() and cp110_init_sensor() are minor. In my opinion, these differences should be expressed explicitly in the armada_thermal_data, in a similar way to my suggested control_msb_offset field. The vendor code hides these differences in slight variations of duplicated code.

Quoted 1 line

What is the advantage of a separate init routine?

The advantage is that is the very near future I plan to add the overheat interrupt only on CP110 (not on 38x) and this needs some initialization. So if we don't make different routines now, I will have to do it right after.

What would be fine is to have the shared code in a separate function, like it is done in Marvell kernel. What do you think about that?

Hi Miquel,

Quoted 27 lines · Miquel RAYNAL

Quoted 27 lines

Quoted 6 lines

Quoted 1 line

How would a separate init_sensor routine improve things?

So yes please do it, thanks to this you won't have to add the control_msb_offset member and can use a clean function. Moreover if in the future we see some usefulness for this LSB register then we could use the new compatible for the Armada 38x.

There are two separate issues here:

1. DT binding

2. init_sensor callback implementation

We both agree on #1. The A38x and CP110 need separate compatible strings. In case we want to access the LSB control register on Armada 38x, we will need yet another compatible string (marvell,armada380-v2-thermal maybe?).

As for #2, I'm all for sharing as much code as possible. I find the vendor kernel approach of duplicating the init routines[1] unhelpful as it violates the DRY principle. The differences between armada380_init_sensor() and cp110_init_sensor() are minor. In my opinion, these differences should be expressed explicitly in the armada_thermal_data, in a similar way to my suggested control_msb_offset field. The vendor code hides these differences in slight variations of duplicated code.

Quoted 6 lines

Quoted 1 line

What is the advantage of a separate init routine?

The advantage is that is the very near future I plan to add the overheat interrupt only on CP110 (not on 38x) and this needs some initialization. So if we don't make different routines now, I will have to do it right after.

I don't think so. The code of these functions in the vendor kernel overheat support implementation is the same, duplicated. The variations are only in registers/bits offsets. A single routine with one or two added armada_thermal_data fields would be much easier to comprehend and maintain.

Quoted 2 lines

What would be fine is to have the shared code in a separate function, like it is done in Marvell kernel. What do you think about that?

The Marvell code does not "share" the code. Separate functions means duplicated code that obscures the hardware details, making maintenance harder on the long run.

https://en.wikipedia.org/wiki/Don%27t_repeat_yourself

baruch

Signature · 8 lines

     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Gregory,

Quoted 79 lines · Gregory CLEMENT

Quoted 69 lines · Baruch Siach

Quoted 63 lines · Miquel RAYNAL

Quoted 49 lines · On Sun, 3 Dec 2017 13:11:23 +0200 Baruch Siach <[email protected]> wrote:

The CP110 component is integrated in the Armada 8k and 7k lines of processors.

This patch also adds an option of offset to the MSB of the control register. The existing DT binding for Armada 38x refers to a single 32 bit control register. It turns out that this is actually only the MSB of the control area. Changing the binding to fix that would break existing DT files, so the Armada 38x binding is left as is.

The new CP110 binding increases the size of the control area to 64 bits, thus moving the MSB to offset 4.

Signed-off-by Baruch Siach <[email protected]>

---
v2: No change

---
 drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/thermal/armada_thermal.c index 0eb82097571f..59b75f63945d 100644 --- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c

drivers/thermal/armada_thermal.c +1 -0

@@ -73,6 +73,7 @@ struct armada_thermal_data {
 	unsigned int temp_shift;
 	unsigned int temp_mask;
 	unsigned int is_valid_shift;
+	unsigned int control_msb_offset;
 };
 
 static void armadaxp_init_sensor(struct platform_device *pdev,

@@ -142,12 +143,14 @@ static void armada375_init_sensor(struct

platform_device *pdev, static void armada380_init_sensor(struct platform_device *pdev, struct armada_thermal_priv *priv)
{

drivers/thermal/armada_thermal.c +4 -2

-	unsigned long reg = readl_relaxed(priv->control);
+	void __iomem *control_msb =
+		priv->control + priv->data->control_msb_offset;
+	unsigned long reg = readl_relaxed(control_msb);
 
 	/* Reset hardware once */
 	if (!(reg & A380_HW_RESET)) {
 		reg |= A380_HW_RESET;
-		writel(reg, priv->control);
+		writel(reg, control_msb);
 		mdelay(10);
 	}
 }

drivers/thermal/armada_thermal.c static const struct armada_thermal_data · line 274

Quoted 6 lines

drivers/thermal/armada_thermal.c +3 -0

@@ -266,6 +269,19 @@ static const struct armada_thermal_data
 armada_ap806_data = { .signed_sample = true,
 };
 
+static const struct armada_thermal_data armada_cp110_data = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada380_init_sensor,

I see the initialization for CP110 thermal IP is close to Armada-380's, but, as you point it in the commit log it is still different.

Quoted 4 lines

I don't know what is the best way to handle this but until now each new compatible had his own ->init_sensor function, shouldn't we do the same here as changes are requested? This would naturally avoid the situation with Armada-380 bindings.

Quoted 6 lines

I'm not sure I understand your suggestion.

There is no difference between the CP110 and the Armada 38x, as far as I can see. The only quirk is that the existing Armada 38x DT binding is wrong I that the 'reg' property references the control MSB, while leaving the LSB out. We

Well I would not say it was wrong but more incomplete :)

Quoted 8 lines

Quoted 4 lines

can't change the Armada 38x binding without breaking existing DTs. The 'control_msb_offset' field that this patch adds allows correct binding for CP110, while keeping compatibility with the existing Armada 38x binding.

I am not against adding a new compatible string for CP110 but ot be honest the new binding for CP110 does not bring anything as you don't use at all the LSB register.

We don't use the LSB yet in mainline driver. But the vendor kernel uses it to "change temperature band gap circuit curve" (quoting vendor kernel commit 4ff2d8a7d3 log). Chances are that we want to do this as well. But said commit changed the DT binding in an incompatible way. We can't do that, and we both agree on that.

Quoted 6 lines

Actually, if on Armada 375 we initially mapped the LSB register it was to support an very early release of the SoC (stepping Z) and only for resetting its value. So I guess you started to write the AP860 part based on the Armada 375 and then found that we could map a more complete range of the registers.

Quoted 6 lines

Quoted 1 line

How would a separate init_sensor routine improve things?

So yes please do it, thanks to this you won't have to add the control_msb_offset member and can use a clean function. Moreover if in the future we see some usefulness for this LSB register then we could use the new compatible for the Armada 38x.

There are two separate issues here:

1. DT binding

2. init_sensor callback implementation

We both agree on #1. The A38x and CP110 need separate compatible strings. In case we want to access the LSB control register on Armada 38x, we will need yet another compatible string (marvell,armada380-v2-thermal maybe?).

As for #2, I'm all for sharing as much code as possible. I find the vendor kernel approach of duplicating the init routines[1] unhelpful as it violates the DRY principle. The differences between armada380_init_sensor() and cp110_init_sensor() are minor. In my opinion, these differences should be expressed explicitly in the armada_thermal_data, in a similar way to my suggested control_msb_offset field. The vendor code hides these differences in slight variations of duplicated code.

What is the advantage of a separate init routine?

baruch

[1] https://github.com/MarvellEmbeddedProcessors/linux-marvell/blob/linux-4.4.52-armada-17.10/drivers/thermal/armada_thermal.c

Signature · 3 lines

     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

On mer., d?c. 13 2017, Baruch Siach <[email protected]> wrote:

Quoted 4 lines

There are two separate issues here:

1. DT binding

Quoted 5 lines

2. init_sensor callback implementation

We both agree on #1. The A38x and CP110 need separate compatible strings. In case we want to access the LSB control register on Armada 38x, we will need yet another compatible string (marvell,armada380-v2-thermal maybe?).

Actually, if it is _compatible_ then we will use the same compatible, ie "marvell,armadacp110-thermal"

Quoted 8 lines

As for #2, I'm all for sharing as much code as possible. I find the vendor kernel approach of duplicating the init routines[1] unhelpful as it violates the DRY principle. The differences between armada380_init_sensor() and cp110_init_sensor() are minor. In my opinion, these differences should be expressed explicitly in the armada_thermal_data, in a similar way to my suggested control_msb_offset field. The vendor code hides these differences in slight variations of duplicated code.

Quoted 1 line

What is the advantage of a separate init routine?

The main advantage is to be able keep the armada380_init_sensor as the legacy init, and then being able to use the new armadacp110_init_sensor for the new binding.

Add compatible strings for AP806 and CP110 that are part of the Armada 8k/7k line of SoCs.

Add a note on the difference in the size of the control area in different bindings. This is an existing difference between the Armada 375 binding and the rest. The new AP806 and CP110 bindings are similar to the existing Armada 375 in this regard.

Signed-off-by Baruch Siach <[email protected]>

---
v2: No change

---
 Documentation/devicetree/bindings/thermal/armada-thermal.txt | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)