| Message ID | 20171214103011.24713-6-miquel.raynal@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Miquel, On jeu., déc. 14 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote: > From: Baruch Siach <baruch@tkos.co.il> > > 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> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> Maybe you should mention that you have made some changes at least to be able to use the previous modification you have done. > --- > drivers/thermal/armada_thermal.c | 51 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 49 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c > index e5b184cee79b..279d01937bb8 100644 > --- a/drivers/thermal/armada_thermal.c > +++ b/drivers/thermal/armada_thermal.c > @@ -47,6 +47,11 @@ > #define CONTROL0_OFFSET 0x0 > #define CONTROL1_OFFSET 0x4 > > +/* TSEN refers to the temperature sensors within the AP */ > +#define CONTROL0_TSEN_START BIT(0) > +#define CONTROL0_TSEN_RESET BIT(1) > +#define CONTROL0_TSEN_ENABLE BIT(2) > + > struct armada_thermal_data; > > /* Marvell EBU Thermal Sensor Dev Structure */ > @@ -70,6 +75,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; > @@ -154,6 +160,24 @@ 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; > + > + if (!priv->control0) { > + dev_err(&pdev->dev, > + "Cannot access to control0 (control LSB) register\n"); > + return; > + } > + > + reg = readl_relaxed(priv->control0); > + reg &= ~CONTROL0_TSEN_RESET; > + reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE; > + writel(reg, priv->control0); > + msleep(10); > +} > + > static bool armada_is_valid(struct armada_thermal_priv *priv) > { > u32 reg = readl_relaxed(priv->status); > @@ -167,6 +191,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal, > struct armada_thermal_priv *priv = thermal->devdata; > u32 reg; > unsigned long m, b, div; > + int sample; > > /* Valid check */ > if (priv->data->is_valid && !priv->data->is_valid(priv)) { > @@ -177,6 +202,11 @@ static int armada_get_temp(struct thermal_zone_device *thermal, > > reg = readl_relaxed(priv->status); > 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; > @@ -184,9 +214,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; > } > > @@ -237,6 +267,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_bit = BIT(16), > + .temp_shift = 0, > + .temp_mask = 0x3ff, > + .coef_b = -150000, Don't you expect any side effect by storing a negative value in a unsigned variable? Thanks, Gregory > + .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", > @@ -255,6 +298,10 @@ static const struct of_device_id armada_thermal_id_table[] = { > .data = &armada380_data, > }, > { > + .compatible = "marvell,armada-ap806-thermal", > + .data = &armada_ap806_data, > + }, > + { > /* sentinel */ > }, > }; > -- > 2.11.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Miquèl, On Thu, Dec 14, 2017 at 11:30:05AM +0100, Miquel Raynal wrote: > From: Baruch Siach <baruch@tkos.co.il> > > 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> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > --- [...] > +static void armada_ap806_init_sensor(struct platform_device *pdev, > + struct armada_thermal_priv *priv) > +{ > + u32 reg; > + > + if (!priv->control0) { > + dev_err(&pdev->dev, > + "Cannot access to control0 (control LSB) register\n"); > + return; > + } With the probe time control area size validation this check would not be needed. baruch > + > + reg = readl_relaxed(priv->control0); > + reg &= ~CONTROL0_TSEN_RESET; > + reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE; > + writel(reg, priv->control0); > + msleep(10); > +}
Hello Gregory & Baruch, On Thu, 14 Dec 2017 12:05:43 +0100 Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > > @@ -184,9 +214,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; > > } > > > > @@ -237,6 +267,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_bit = BIT(16), > > + .temp_shift = 0, > > + .temp_mask = 0x3ff, > > + .coef_b = -150000, > > Don't you expect any side effect by storing a negative value in a > unsigned variable? That is a fair question, I did not spot that. As other values are really close to 2^32 I don't know what is the best option for us in this case. Should I: - don't care? - use signed values? (dangerous IMHO) - use a union with a signed and an unsigned value? (problem moved to ->get_temp()) Thanks for your input. Miquèl
Hi Miquèl, On Mon, Dec 18, 2017 at 10:41:27AM +0100, Miquel RAYNAL wrote: > Hello Gregory & Baruch, > > On Thu, 14 Dec 2017 12:05:43 +0100 > Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > > > > @@ -184,9 +214,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; > > > } > > > > > > @@ -237,6 +267,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_bit = BIT(16), > > > + .temp_shift = 0, > > > + .temp_mask = 0x3ff, > > > + .coef_b = -150000, > > > > Don't you expect any side effect by storing a negative value in a > > unsigned variable? > > That is a fair question, I did not spot that. > > As other values are really close to 2^32 I don't know what is the best > option for us in this case. Should I: > - don't care? > - use signed values? (dangerous IMHO) > - use a union with a signed and an unsigned value? (problem moved to > ->get_temp()) Another option is to use s64 type. baruch
Hi Baruch, On Mon, 18 Dec 2017 13:11:56 +0200 Baruch Siach <baruch@tkos.co.il> wrote: > Hi Miquèl, > > On Mon, Dec 18, 2017 at 10:41:27AM +0100, Miquel RAYNAL wrote: > > Hello Gregory & Baruch, > > > > On Thu, 14 Dec 2017 12:05:43 +0100 > > Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > > > > > > @@ -184,9 +214,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; > > > > } > > > > > > > > @@ -237,6 +267,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_bit = BIT(16), > > > > + .temp_shift = 0, > > > > + .temp_mask = 0x3ff, > > > > + .coef_b = -150000, > > > > > > Don't you expect any side effect by storing a negative value in a > > > unsigned variable? > > > > That is a fair question, I did not spot that. > > > > As other values are really close to 2^32 I don't know what is the > > best option for us in this case. Should I: > > - don't care? > > - use signed values? (dangerous IMHO) > > - use a union with a signed and an unsigned value? (problem moved > > to ->get_temp()) > > Another option is to use s64 type. I prefer this one! Thank you, Miquèl > > baruch >
diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c index e5b184cee79b..279d01937bb8 100644 --- a/drivers/thermal/armada_thermal.c +++ b/drivers/thermal/armada_thermal.c @@ -47,6 +47,11 @@ #define CONTROL0_OFFSET 0x0 #define CONTROL1_OFFSET 0x4 +/* TSEN refers to the temperature sensors within the AP */ +#define CONTROL0_TSEN_START BIT(0) +#define CONTROL0_TSEN_RESET BIT(1) +#define CONTROL0_TSEN_ENABLE BIT(2) + struct armada_thermal_data; /* Marvell EBU Thermal Sensor Dev Structure */ @@ -70,6 +75,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; @@ -154,6 +160,24 @@ 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; + + if (!priv->control0) { + dev_err(&pdev->dev, + "Cannot access to control0 (control LSB) register\n"); + return; + } + + reg = readl_relaxed(priv->control0); + reg &= ~CONTROL0_TSEN_RESET; + reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE; + writel(reg, priv->control0); + msleep(10); +} + static bool armada_is_valid(struct armada_thermal_priv *priv) { u32 reg = readl_relaxed(priv->status); @@ -167,6 +191,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal, struct armada_thermal_priv *priv = thermal->devdata; u32 reg; unsigned long m, b, div; + int sample; /* Valid check */ if (priv->data->is_valid && !priv->data->is_valid(priv)) { @@ -177,6 +202,11 @@ static int armada_get_temp(struct thermal_zone_device *thermal, reg = readl_relaxed(priv->status); 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; @@ -184,9 +214,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; } @@ -237,6 +267,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_bit = BIT(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", @@ -255,6 +298,10 @@ static const struct of_device_id armada_thermal_id_table[] = { .data = &armada380_data, }, { + .compatible = "marvell,armada-ap806-thermal", + .data = &armada_ap806_data, + }, + { /* sentinel */ }, };