thermal: Add Armada 375 SoC support

Now that a generic infrastructure is in place, it's possible to support the new Armada 375 SoC thermal sensor. This sensor is similar to the one available in the already supported SoCs, with its specific temperature formula and specific sensor initialization.

In addition, we also add support for the Z1 SoC stepping, which needs an initialization-quirk to work properly.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 .../devicetree/bindings/thermal/armada-thermal.txt |  7 ++
 drivers/thermal/armada_thermal.c                   | 83 ++++++++++++++++++++++
 2 files changed, 90 insertions(+)

Documentation/devicetree/bindings/thermal/armada-thermal.txt +7 -0

index fff93d5..745d241 100644
@@ -4,8 +4,15 @@ Required properties:
 
 - compatible:	Should be set to one of the following:
 		marvell,armada370-thermal
+		marvell,armada375-thermal
+		marvell,armada375-z1-thermal
 		marvell,armadaxp-thermal
 
+		Note: As the name suggests, "marvell,armada375-z1-thermal"
+		applies for the SoC Z1 stepping only. The operating system
+		may auto-detect the SoC stepping and update the compatible
+		at runtime.
+
 - reg:		Device's register space.
 		Two entries are expected, see the examples below.
 		The first one is required for the sensor register;

drivers/thermal/armada_thermal.c +28 -0

index 3e4d8ef..a37942b 100644
@@ -35,6 +35,15 @@
 #define PMU_TDC0_OTF_CAL_MASK		(0x1 << 30)
 #define PMU_TDC0_START_CAL_MASK		(0x1 << 25)
 
+#define A375_Z1_CAL_RESET_LSB		0x8011e214
+#define A375_Z1_CAL_RESET_MSB		0x30a88019
+#define A375_Z1_WORKAROUND_BIT		BIT(9)
+
+#define TSEN40_UNIT_CONTROL_OFFSET	27
+#define TSEN40_UNIT_CONTROL_MASK	0x7
+#define TSEN40_READOUT_INVERT		BIT(15)
+#define TSEN40_HW_RESETn		BIT(8)
+
 struct armada_thermal_data;
 
 /* Marvell EBU Thermal Sensor Dev Structure */

@@ -106,6 +115,50 @@ static void armada370_init_sensor(struct armada_thermal_priv *priv)
 	mdelay(10);
 }
 
+static void armada375_init_sensor(struct armada_thermal_priv *priv)
+{
+	unsigned long reg;
+
+	reg = readl(priv->control + 4);
+	reg &= ~(TSEN40_UNIT_CONTROL_MASK << TSEN40_UNIT_CONTROL_OFFSET);
+	reg &= ~TSEN40_READOUT_INVERT;
+	reg &= ~TSEN40_HW_RESETn;
+
+	writel(reg, priv->control + 4);
+	mdelay(20);
+
+	reg |= TSEN40_HW_RESETn;
+	writel(reg, priv->control + 4);
+	mdelay(50);
+}
+
+static void armada375_z1_init_sensor(struct armada_thermal_priv *priv)
+{

drivers/thermal/armada_thermal.c +20 -0

+
+	writel(reg, priv->control + 4);
+	mdelay(20);
+
+	reg |= TSEN40_HW_RESETn;
+	writel(reg, priv->control + 4);
+	mdelay(50);
+}
+
 static bool armada_is_valid(struct armada_thermal_priv *priv)
 {
 	unsigned long reg = readl_relaxed(priv->sensor);

@@ -163,6 +216,28 @@ static const struct armada_thermal_data armada370_data = {
 	.coef_div = 13825,
 };
 
 
 
 
+static const struct armada_thermal_data armada375_data = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada375_init_sensor,
+	.is_valid_offset = 10,
+	.temp_offset = 0,
+	.temp_mask = 0x1ff,
+	.coef_b = 3171900000UL,
+	.coef_m = 10000000UL,
+	.coef_div = 13616,
+};
+

drivers/thermal/armada_thermal.c +1 -0

+

static const struct of_device_id armada_thermal_id_table[] = {

drivers/thermal/armada_thermal.c +8 -0

 	{
 		.compatible = "marvell,armadaxp-thermal",

@@ -173,6 +248,14 @@ static const struct of_device_id armada_thermal_id_table[] = {
 		.data       = &armada370_data,
 	},
 	{
+		.compatible = "marvell,armada375-thermal",
+		.data       = &armada375_data,
+	},
+	{
+		.compatible = "marvell,armada375-z1-thermal",
+		.data       = &armada375_z1_data,
+	},
+	{
 		/* sentinel */
 	},
 };

Signature · 1 line

1.9.1

drivers/thermal/armada_thermal.c static void armada370_init_sensor(struct armada_thermal_priv *priv) · line 152

Quoted 2 lines · Ezequiel Garcia

drivers/thermal/armada_thermal.c +2 -0

+	/* This is only needed on A375 Z1 SoC silicon revision */
+	reg |= A375_Z1_WORKAROUND_BIT;

and this seem to be the only differences between the two init functions.

It also appears to be the only reason for having two data structs below. Is it worth checking for the compatible string in the init function so you only have one init and one data struct?

Quoted 10 lines · Jason Cooper

drivers/thermal/armada_thermal.c static void armada370_init_sensor(struct armada_thermal_priv *priv) · line 152

Quoted 2 lines · Ezequiel Garcia

drivers/thermal/armada_thermal.c +2 -0

+	/* This is only needed on A375 Z1 SoC silicon revision */
+	reg |= A375_Z1_WORKAROUND_BIT;

and this seem to be the only differences between the two init functions.

It also appears to be the only reason for having two data structs below. Is it worth checking for the compatible string in the init function so you only have one init and one data struct?

Yes, thought about it at one point but I guess it seemed to me cleaner this way.

I'll squash it if you think keeping two structs is stupid bloat.

In addition, we also add support for the Z1 SoC stepping, which needs an initialization-quirk to work properly.

Documentation/devicetree/bindings/thermal/armada-thermal.txt +0 -0

index fff93d5..745d241 100644
@@ -4,8 +4,15 @@ Required properties:
 

Quoted 2 lines · Ezequiel Garcia

In addition, we also add support for the Z1 SoC stepping, which needs an initialization-quirk to work properly.

...

Quoted 12 lines

Documentation/devicetree/bindings/thermal/armada-thermal.txt +2 -0

index fff93d5..745d241 100644
@@ -4,8 +4,15 @@ Required properties:
 
 - compatible:	Should be set to one of the following:
 		marvell,armada370-thermal
+		marvell,armada375-thermal
+		marvell,armada375-z1-thermal
 		marvell,armadaxp-thermal
 

Documentation/devicetree/bindings/thermal/armada-thermal.txt Required properties: · line 14

Quoted 4 lines

Documentation/devicetree/bindings/thermal/armada-thermal.txt +4 -0

+		Note: As the name suggests, "marvell,armada375-z1-thermal"
+		applies for the SoC Z1 stepping only. The operating system
+		may auto-detect the SoC stepping and update the compatible
+		at runtime.

Please also include a statement in here regarding the register quirk for the z1 stepping.

The initial release of the Armada 375 DB board has an Armada 375 Z1 stepping silicon. This commit introduces a quirk that allows to workaround a series of issues with the thermal sensor in this stepping, but updating the devicetree:

* Updates the compatible string for the thermal, so the driver can perform a specific initialization of the sensor.

* Moves the offset of the thermal control register. This quirk allows to specifiy the correct (A0 stepping) offset in the devicetree.

Signed-off-by Ezequiel Garcia <[email protected]>

---
  arch/arm/mach-mvebu/board-v7.c     | 57 ++++++++++++++++++++++++++++++++++++++
  arch/arm/mach-mvebu/mvebu-soc-id.h |  3 ++
  2 files changed, 60 insertions(+)

arch/arm/mach-mvebu/board-v7.c +0 -0

index 333fca8..93f50f2 100644

arch/arm/mach-mvebu/board-v7.c +54 -0

+{
+	struct device_node *np;
+	u32 dev, rev;
+
+	if (mvebu_get_soc_id(&dev, &rev) && rev > ARMADA_375_Z1_REV)
+		return;
+
+	for_each_compatible_node(np, NULL, "marvell,armada375-thermal") {
+		struct property *prop;
+		__be32 newval, *newprop, *oldprop;
+		int len;
+
+		/*
+		 * The register offset is at a wrong location. This quirk
+		 * creates a new reg property as a clone of the previous
+		 * one and corrects the offset.
+		 */
+		oldprop = (__be32 *)of_get_property(np, "reg", &len);
+		if (!oldprop)
+			continue;
+
+		/* Create a duplicate of the 'reg' property */
+		prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+		prop->length = len;
+		prop->name = kstrdup("reg", GFP_KERNEL);
+		prop->value = kzalloc(len, GFP_KERNEL);
+		memcpy(prop->value, oldprop, len);
+
+		/* Fixup the register offset of the second entry */
+		oldprop += 2;
+		newprop = (__be32 *)prop->value + 2;
+		newval = cpu_to_be32(be32_to_cpu(*oldprop) -
+				     A375_Z1_THERMAL_FIXUP_OFFSET);
+		*newprop = newval;
+		of_update_property(np, prop);
+
+		/*
+		 * The thermal controller needs some quirk too, so let's change
+		 * the compatible string to reflect this.
+		 */
+		prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+		prop->name = kstrdup("compatible", GFP_KERNEL);
+		prop->length = sizeof("marvell,armada375-z1-thermal");
+		prop->value = kstrdup("marvell,armada375-z1-thermal",
+						GFP_KERNEL);
+		of_update_property(np, prop);
+	}
+	return;
+}
+
  static void __init mvebu_dt_init(void)
  {
  	if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
  		i2c_quirk();
+	if (of_machine_is_compatible("marvell,a375-db"))
+		thermal_quirk();
+
  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
  }
 

@@ -123,6 +179,7 @@ static const char * const armada_375_dt_compat[] = {
 
  DT_MACHINE_START(ARMADA_375_DT, "Marvell Armada 375 (Device Tree)")
  	.init_time	= mvebu_timer_and_clk_init,
+	.init_machine	= mvebu_dt_init,
  	.restart	= mvebu_restart,
  	.dt_compat	= armada_375_dt_compat,
  MACHINE_END

arch/arm/mach-mvebu/mvebu-soc-id.h +3 -0

index 3165425..294a443 100644
@@ -20,6 +20,9 @@
  #define MV78XX0_A0_REV	    0x1
  #define MV78XX0_B0_REV	    0x2
 
+/* Armada 375 */
+#define ARMADA_375_Z1_REV   0x0
+
  #ifdef CONFIG_ARCH_MVEBU
  int mvebu_get_soc_id(u32 *dev, u32 *rev);
  #else

Are we sure, we want to fixup quirks like this the way below?

Alternatively, we can also keep some armada-375-z1.dtsi and one for the board including it.

For minor differences such as SoC stepping, I personally prefer to not have separate Device Trees. We already have many of them, for each variant of the various SOCs. If we add the different steppings, it's going to be even more complicated. Also, there will be a new iteration of the Armada 375 DB with an A0 chip, which does not have the Z1 bug.

Quoted 5 lines

For minor differences such as SoC stepping, I personally prefer to not have separate Device Trees. We already have many of them, for each variant of the various SOCs. If we add the different steppings, it's going to be even more complicated. Also, there will be a new iteration of the Armada 375 DB with an A0 chip, which does not have the Z1 bug.

How many Z1 are there out and about? Would it be simpler to just disable thermal on Z1? If only development boards have Z1, this could be reasonable.

So, I would like to see the Z1 stepping supported for now, and have the freedom to remove its support later once we are all ready to switch to A0.

Quoted 3 lines

So, I would like to see the Z1 stepping supported for now, and have the freedom to remove its support later once we are all ready to switch to A0.

Hi Thomas

Sounds like a good plan. Try to keep all the mess to support Z1 in one place so that it can be easily taken out once people have A0.

Quoted 2 lines

Sounds like a good plan. Try to keep all the mess to support Z1 in one place so that it can be easily taken out once people have A0.

Agreed. Thomas, do you think you'll be able to get a definitive answer from Marvell re the number of Z1 boards in the wild? Once they've moved to the A0, of course.

Unless we can get a hard answer on that, I doubt we'll ever withdraw support for the Z1. Not that that's a bad thing, just trying to be realistic. If we add code, expect to support it.

And keeping it all in one place is kind of an impossibility. Just for thermal Z1, there are changes to the binding docs, the thermal driver, and the soc code.

I'd say it's more important to keep it clean, with dts files targeting the A0+ SoCs, and the Z1 being the exception case(s).