[PATCH v2 1/4] dt-bindings: thermal/armada: describe AP806 and CP110

[PATCH v2 1/4] dt-bindings: thermal/armada: describe AP806 and CP110

[Baruch Siach]

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 <baruch@tkos.co.il>
---
v2: No change
---
 Documentation/devicetree/bindings/thermal/armada-thermal.txt | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
index 24aacf8948c5..eec57f509166 100644
--- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
@@ -7,12 +7,19 @@ Required properties:
 		marvell,armada375-thermal
 		marvell,armada380-thermal
 		marvell,armadaxp-thermal
+		marvell,armada-ap806-thermal
+		marvell,armada-cp110-thermal
 
 - reg:		Device's register space.
 		Two entries are expected, see the examples below.
 		The first one is required for the sensor register;
-		the second one is required for the control register
+		the second one is required for the control area
 		to be used for sensor initialization (a.k.a. calibration).
+		The size of the control area must be 4 for
+		marvell,armada370-thermal, marvell,armada380-thermal, and
+		marvell,armadaxp-thermal. The size must be 8 for
+		marvell,armada375-thermal, marvell,armada-ap806-thermal, and
+		marvell,armada-cp110-thermal.
 
 Example:
 
-- 
2.15.0

[PATCH v2 2/4] thermal: armada: add support for AP806

[Baruch Siach]

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

The thermal sensor sample field on the status register is a signed
value. Extend armada_get_temp() to handle signed values.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v2:
  Use msleep instead of mdelay (RMK).
  Fix temperature calculation formula according to recent documentation
  and vendor code. Update the commit log.
---
 drivers/thermal/armada_thermal.c | 44 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 706d74798cbe..0eb82097571f 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -41,6 +41,10 @@
 #define A375_HW_RESETn			BIT(8)
 #define A380_HW_RESET			BIT(8)
 
+#define AP806_START			BIT(0)
+#define AP806_RESET			BIT(1)
+#define AP806_ENABLE			BIT(2)
+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */
@@ -63,6 +67,7 @@ struct armada_thermal_data {
 	unsigned long coef_m;
 	unsigned long coef_div;
 	bool inverted;
+	bool signed_sample;
 
 	/* Register shift and mask to access the sensor temperature */
 	unsigned int temp_shift;
@@ -147,6 +152,18 @@ static void armada380_init_sensor(struct platform_device *pdev,
 	}
 }
 
+static void armada_ap806_init_sensor(struct platform_device *pdev,
+				     struct armada_thermal_priv *priv)
+{
+	u32 reg = readl_relaxed(priv->control);
+
+	reg &= ~AP806_RESET;
+	reg |= AP806_START;
+	reg |= AP806_ENABLE;
+	writel(reg, priv->control);
+	msleep(10);
+}
+
 static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
 	unsigned long reg = readl_relaxed(priv->sensor);
@@ -160,6 +177,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 	struct armada_thermal_priv *priv = thermal->devdata;
 	unsigned long reg;
 	unsigned long m, b, div;
+	int sample;
 
 	/* Valid check */
 	if (priv->data->is_valid && !priv->data->is_valid(priv)) {
@@ -170,6 +188,11 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 
 	reg = readl_relaxed(priv->sensor);
 	reg = (reg >> priv->data->temp_shift) & priv->data->temp_mask;
+	if (priv->data->signed_sample)
+		/* The most significant bit is the sign bit */
+		sample = sign_extend32(reg, fls(priv->data->temp_mask) - 1);
+	else
+		sample = reg;
 
 	/* Get formula coeficients */
 	b = priv->data->coef_b;
@@ -177,9 +200,9 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
 	div = priv->data->coef_div;
 
 	if (priv->data->inverted)
-		*temp = ((m * reg) - b) / div;
+		*temp = ((m * sample) - b) / div;
 	else
-		*temp = (b - (m * reg)) / div;
+		*temp = (b - (m * sample)) / div;
 	return 0;
 }
 
@@ -230,6 +253,19 @@ static const struct armada_thermal_data armada380_data = {
 	.inverted = true,
 };
 
+static const struct armada_thermal_data armada_ap806_data = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada_ap806_init_sensor,
+	.is_valid_shift = 16,
+	.temp_shift = 0,
+	.temp_mask = 0x3ff,
+	.coef_b = -150000,
+	.coef_m = 423UL,
+	.coef_div = 1,
+	.inverted = true,
+	.signed_sample = true,
+};
+
 static const struct of_device_id armada_thermal_id_table[] = {
 	{
 		.compatible = "marvell,armadaxp-thermal",
@@ -247,6 +283,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
 		.compatible = "marvell,armada380-thermal",
 		.data       = &armada380_data,
 	},
+	{
+		.compatible = "marvell,armada-ap806-thermal",
+		.data       = &armada_ap806_data,
+	},
 	{
 		/* sentinel */
 	},
-- 
2.15.0

[PATCH v2 3/4] thermal: armada: add support for CP110

[Baruch Siach]

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 <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
v2: No change
---
 drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 0eb82097571f..59b75f63945d 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -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)
 {
-	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);
 	}
 }
@@ -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,
+	.is_valid_shift = 10,
+	.temp_shift = 0,
+	.temp_mask = 0x3ff,
+	.control_msb_offset = 4,
+	.coef_b = 1172499100UL,
+	.coef_m = 2000096UL,
+	.coef_div = 4201,
+	.inverted = true,
+};
+
 static const struct of_device_id armada_thermal_id_table[] = {
 	{
 		.compatible = "marvell,armadaxp-thermal",
@@ -287,6 +303,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
 		.compatible = "marvell,armada-ap806-thermal",
 		.data       = &armada_ap806_data,
 	},
+	{
+		.compatible = "marvell,armada-cp110-thermal",
+		.data       = &armada_cp110_data,
+	},
 	{
 		/* sentinel */
 	},
-- 
2.15.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/4] thermal: armada: use msleep for long delays

[Baruch Siach]

Use msleep for long (> 10ms) delays, instead of the busy waiting mdelay.
All delays are called from the probe routine, where scheduling is
allowed.

Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
v2: New patch
---
 drivers/thermal/armada_thermal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
index 59b75f63945d..d86538d4f519 100644
--- a/drivers/thermal/armada_thermal.c
+++ b/drivers/thermal/armada_thermal.c
@@ -119,7 +119,7 @@ static void armada370_init_sensor(struct platform_device *pdev,
 	reg &= ~PMU_TDC0_START_CAL_MASK;
 	writel(reg, priv->control);
 
-	mdelay(10);
+	msleep(10);
 }
 
 static void armada375_init_sensor(struct platform_device *pdev,
@@ -133,11 +133,11 @@ static void armada375_init_sensor(struct platform_device *pdev,
 	reg &= ~A375_HW_RESETn;
 
 	writel(reg, priv->control + 4);
-	mdelay(20);
+	msleep(20);
 
 	reg |= A375_HW_RESETn;
 	writel(reg, priv->control + 4);
-	mdelay(50);
+	msleep(50);
 }
 
 static void armada380_init_sensor(struct platform_device *pdev,
@@ -151,7 +151,7 @@ static void armada380_init_sensor(struct platform_device *pdev,
 	if (!(reg & A380_HW_RESET)) {
 		reg |= A380_HW_RESET;
 		writel(reg, control_msb);
-		mdelay(10);
+		msleep(10);
 	}
 }
 
-- 
2.15.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] dt-bindings: thermal/armada: describe AP806 and CP110

[Rob Herring]

On Sun, Dec 03, 2017 at 01:11:21PM +0200, Baruch Siach wrote:
> 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 <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> ---
> v2: No change
> ---
>  Documentation/devicetree/bindings/thermal/armada-thermal.txt | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] thermal: armada: add support for CP110

[Miquel RAYNAL]

Hello Baruch,

On Sun,  3 Dec 2017 13:11:23 +0200
Baruch Siach <baruch@tkos.co.il> 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 <baruch@tkos.co.il>
> ---
> v2: No change
> ---
>  drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/armada_thermal.c
> b/drivers/thermal/armada_thermal.c index 0eb82097571f..59b75f63945d
> 100644 --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -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)
>  {
> -	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);
>  	}
>  }
> @@ -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.

Thanks,
Miquèl

Re: [PATCH v2 3/4] thermal: armada: add support for CP110

[Baruch Siach]

Hi Miquel,

On Mon, Dec 11, 2017 at 04:09:32PM +0100, Miquel RAYNAL wrote:
> On Sun,  3 Dec 2017 13:11:23 +0200
> Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> 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 <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> > ---
> > v2: No change
> > ---
> >  drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/thermal/armada_thermal.c
> > b/drivers/thermal/armada_thermal.c index 0eb82097571f..59b75f63945d
> > 100644 --- a/drivers/thermal/armada_thermal.c
> > +++ b/drivers/thermal/armada_thermal.c
> > @@ -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)
> >  {
> > -	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);
> >  	}
> >  }
> > @@ -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.

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] thermal: armada: add support for CP110

[Gregory CLEMENT]

Hi Baruch,
 
 On lun., déc. 11 2017, Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Miquel,
>
> On Mon, Dec 11, 2017 at 04:09:32PM +0100, Miquel RAYNAL wrote:
>> On Sun,  3 Dec 2017 13:11:23 +0200
>> Baruch Siach <baruch@tkos.co.il> 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 <baruch@tkos.co.il>
>> > ---
>> > v2: No change
>> > ---
>> >  drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
>> >  1 file changed, 22 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/thermal/armada_thermal.c
>> > b/drivers/thermal/armada_thermal.c index 0eb82097571f..59b75f63945d
>> > 100644 --- a/drivers/thermal/armada_thermal.c
>> > +++ b/drivers/thermal/armada_thermal.c
>> > @@ -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)
>> >  {
>> > -	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);
>> >  	}
>> >  }
>> > @@ -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.
>
> 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 :)


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

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

Thanks,

Gregory

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

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 3/4] thermal: armada: add support for CP110

[Baruch Siach]

Hi Gregory,

On Mon, Dec 11, 2017 at 06:02:49PM +0100, Gregory CLEMENT wrote:
>  On lun., déc. 11 2017, Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:
> > On Mon, Dec 11, 2017 at 04:09:32PM +0100, Miquel RAYNAL wrote:
> >> On Sun,  3 Dec 2017 13:11:23 +0200
> >> Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> 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 <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> >> > ---
> >> > v2: No change
> >> > ---
> >> >  drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
> >> >  1 file changed, 22 insertions(+), 2 deletions(-)
> >> > 
> >> > diff --git a/drivers/thermal/armada_thermal.c
> >> > b/drivers/thermal/armada_thermal.c index 0eb82097571f..59b75f63945d
> >> > 100644 --- a/drivers/thermal/armada_thermal.c
> >> > +++ b/drivers/thermal/armada_thermal.c
> >> > @@ -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)
> >> >  {
> >> > -	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);
> >> >  	}
> >> >  }
> >> > @@ -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.
> >
> > 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 :)
> 
> > 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.

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] thermal: armada: add support for CP110

[Miquel RAYNAL]

Hello Baruch,


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

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?

Thanks,
Miquèl

Re: [PATCH v2 3/4] thermal: armada: add support for CP110

[Baruch Siach]

Hi Miquel,

On Wed, Dec 13, 2017 at 09:55:01AM +0100, Miquel RAYNAL wrote:
> > > > 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?
> 
> 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.

> 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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] thermal: armada: add support for CP110

[Gregory CLEMENT]

Hi Baruch,
 
 On mer., déc. 13 2017, Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Gregory,
>
> On Mon, Dec 11, 2017 at 06:02:49PM +0100, Gregory CLEMENT wrote:
>>  On lun., déc. 11 2017, Baruch Siach <baruch@tkos.co.il> wrote:
>> > On Mon, Dec 11, 2017 at 04:09:32PM +0100, Miquel RAYNAL wrote:
>> >> On Sun,  3 Dec 2017 13:11:23 +0200
>> >> Baruch Siach <baruch@tkos.co.il> 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 <baruch@tkos.co.il>
>> >> > ---
>> >> > v2: No change
>> >> > ---
>> >> >  drivers/thermal/armada_thermal.c | 24 ++++++++++++++++++++++--
>> >> >  1 file changed, 22 insertions(+), 2 deletions(-)
>> >> > 
>> >> > diff --git a/drivers/thermal/armada_thermal.c
>> >> > b/drivers/thermal/armada_thermal.c index 0eb82097571f..59b75f63945d
>> >> > 100644 --- a/drivers/thermal/armada_thermal.c
>> >> > +++ b/drivers/thermal/armada_thermal.c
>> >> > @@ -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)
>> >> >  {
>> >> > -	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);
>> >> >  	}
>> >> >  }
>> >> > @@ -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.
>> >
>> > 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 :)
>> 
>> > 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.
>
>> 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.
>> 
>> > 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?).

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

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


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.

Gregory



>
> baruch
>
> [1] https://github.com/MarvellEmbeddedProcessors/linux-marvell/blob/linux-4.4.52-armada-17.10/drivers/thermal/armada_thermal.c
>
> -- 
>      http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 3/4] thermal: armada: add support for CP110

[Gregory CLEMENT]

Hi Baruch,
 
 On mer., déc. 13 2017, Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:

> Hi Miquel,
>
> On Wed, Dec 13, 2017 at 09:55:01AM +0100, Miquel RAYNAL wrote:
>> > > > 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?
>> 
>> 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.
>
>> 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.

Well, Miquel speak about writting new code, so I don't see why you refer
the Marvell LSP code. Also, I don't see how having common function will
duplicate the code.

Gregory

>
> https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
>
> baruch
>
> -- 
>      http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] thermal: armada: add support for CP110

[Baruch Siach]

Hi Gregory,

On Wed, Dec 13, 2017 at 10:13:02AM +0100, Gregory CLEMENT wrote:
>  On mer., déc. 13 2017, Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:

[...]

> > 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?).
> 
> Actually, if it is _compatible_ then we will use the same compatible, ie
> "marvell,armadacp110-thermal"

Reusing the same compatible string for the same hardware peripheral in 
different SoCs is not a good idea. You often find out later that they are not 
actually the same.

But this point is moot. The A38x and CP110 thermal sensors are not the same. 
The overheat interrupt registers are in different offsets.

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

I disagree, sorry. I don't think I can make my point any more clear than I 
did.

I am fine with you or Miquel making the code changes that you think are 
necessary. I'll comment on the code when I see it.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html