Hello K, On Tue, Apr 07, 2015 at 08:50:04AM +0530, Keerthy wrote: > Hi Eduardo, > > On Friday 27 March 2015 02:15 PM, Keerthy wrote: > >Bandgap Temperature read Dtemp can be corrupted > > > >DESCRIPTION > > Read accesses to registers listed below can be corrupted due to > > incorrect resynchronization between > > clock domains. > > > > Read access to registers below can be corrupted : > > • CTRL_CORE_DTEMP_MPU/GPU/CORE/DSPEVE/IVA_n (n = 0 to 4) > > • CTRL_CORE_TEMP_SENSOR_MPU/GPU/CORE/DSPEVE/IVA_n > > > >WORKAROUND > > Multiple reads to > > CTRL_CORE_TEMP_SENSOR_MPU/GPU/CORE/DSPEVE/IVA[9:0]: > > BGAP_DTEMPMPU/GPU/CORE/DSPEVE/IVA is needed to discard false value > > and read right value: > > 1. Perform two successive reads to BGAP_DTEMP bit field. > > (a) If read1 returns Val1 and read2 returns Val1, > > then right value is Val1. > > (b) If read1 returns Val1, read 2 returns Val2, a > > third read is needed. > > 2. Perform third read > > (a) If read3 returns Val2 then right value is Val2. > > (b) If read3 returns Val3, then right value is Val3.
The third read description (b) is misleading/confusing. Does it mean
third read value is always correct or do we need to compare against val1
and val2? if val3 != val2 && val3 != val1, which one is correct? none?
> >
>
Thanks for the detailed errata description.
> A gentle ping on this.
>
Sorry for the late answer. (minor) Comment below:
> >Signed-off-by: Keerthy <j-keer...@ti.com>
> >---
> >
> >Read all the temperatures from the 5 sensors and saw that
> >they were sane with time.
> >
> >Ran cpuloadgen and saw that temperatures rising on all the sensors
> >and cooled down as soon as the load was reduced.
> >
> > .../thermal/ti-soc-thermal/dra752-thermal-data.c | 3 +-
> > drivers/thermal/ti-soc-thermal/ti-bandgap.c | 37
> > +++++++++++++++++++++-
> > drivers/thermal/ti-soc-thermal/ti-bandgap.h | 4 +++
> > 3 files changed, 42 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/thermal/ti-soc-thermal/dra752-thermal-data.c
> >b/drivers/thermal/ti-soc-thermal/dra752-thermal-data.c
> >index a492927..58b5c66 100644
> >--- a/drivers/thermal/ti-soc-thermal/dra752-thermal-data.c
> >+++ b/drivers/thermal/ti-soc-thermal/dra752-thermal-data.c
> >@@ -420,7 +420,8 @@ const struct ti_bandgap_data dra752_data = {
> > TI_BANDGAP_FEATURE_FREEZE_BIT |
> > TI_BANDGAP_FEATURE_TALERT |
> > TI_BANDGAP_FEATURE_COUNTER_DELAY |
> >- TI_BANDGAP_FEATURE_HISTORY_BUFFER,
> >+ TI_BANDGAP_FEATURE_HISTORY_BUFFER |
> >+ TI_BANDGAP_FEATURE_ERRATA_814,
> > .fclock_name = "l3instr_ts_gclk_div",
> > .div_ck_name = "l3instr_ts_gclk_div",
> > .conv_table = dra752_adc_to_temp,
> >diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> >b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> >index 74c0e34..baf822e 100644
> >--- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> >+++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> >@@ -119,6 +119,37 @@ exit:
> > }
> >
> > /**
> >+ * ti_dra7_bandgap_read_temp() - helper function to read dra7 sensor
> >temperature
> >+ * @bgp: pointer to ti_bandgap structure
> >+ * @reg: desired register (offset) to be read
> >+ *
> >+ * Function to read dra7 bandgap sensor temperature. This is done separately
> >+ * so as to workaround the errata "Bandgap Temperature read Dtemp can be
> >+ * corrupted" - Errata ID: i814".
> >+ * Read accesses to registers listed below can be corrupted due to incorrect
> >+ * resynchronization between clock domains.
> >+ * Read access to registers below can be corrupted :
> >+ * CTRL_CORE_DTEMP_MPU/GPU/CORE/DSPEVE/IVA_n (n = 0 to 4)
> >+ * CTRL_CORE_TEMP_SENSOR_MPU/GPU/CORE/DSPEVE/IVA_n
> >+ *
> >+ * Return: the register value.
> >+ */
> >+static u32 ti_dra7_bandgap_read_temp(struct ti_bandgap *bgp, u32 reg)
Are you sure this bug affects only DRA7? I would prefer you name this
not specific to soc version. Maybe bind it to the errata:
+static u32 ti_i814_bandgap_read_temp(struct ti_bandgap *bgp, u32 reg)
> >+{
> >+ u32 val1, val2;
> >+
> >+ val1 = ti_bandgap_readl(bgp, reg);
> >+ val2 = ti_bandgap_readl(bgp, reg);
> >+
> >+/* If both times we read the same value then that is right */
Please, indent the comments too.
> >+ if (val1 == val2)
> >+ return val1;
> >+
> >+/* if val1 and val2 are different read it third time */
ditto.
> >+ return ti_bandgap_readl(bgp, reg);
Actually, if I understood the errata description correctly, you need to:
1. read a third time (val3)
2. compare val3 against val2, and return val2 if they are same
If they are different, the errata is not clear. Can you please clarify?
> >+}
> >+
> >+/**
> > * ti_bandgap_read_temp() - helper function to read sensor temperature
> > * @bgp: pointer to ti_bandgap structure
> > * @id: bandgap sensor id
> >@@ -148,7 +179,11 @@ static u32 ti_bandgap_read_temp(struct ti_bandgap *bgp,
> >int id)
> > }
> >
> > /* read temperature */
> >- temp = ti_bandgap_readl(bgp, reg);
> >+ if (TI_BANDGAP_HAS(bgp, ERRATA_814))
> >+ temp = ti_dra7_bandgap_read_temp(bgp, reg);
> >+ else
> >+ temp = ti_bandgap_readl(bgp, reg);
> >+
> > temp &= tsr->bgap_dtemp_mask;
> >
> > if (TI_BANDGAP_HAS(bgp, FREEZE_BIT))
> >diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.h
> >b/drivers/thermal/ti-soc-thermal/ti-bandgap.h
> >index b3adf72..b2da3fc 100644
> >--- a/drivers/thermal/ti-soc-thermal/ti-bandgap.h
> >+++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.h
> >@@ -318,6 +318,9 @@ struct ti_temp_sensor {
> > * TI_BANDGAP_FEATURE_HISTORY_BUFFER - used when the bandgap device
> > features
> > * a history buffer of temperatures.
> > *
> >+ * TI_BANDGAP_FEATURE_ERRATA_814 - used to workaorund when the bandgap
> >device
> >+ * has Errata 814
> >+ *
> > * TI_BANDGAP_HAS(b, f) - macro to check if a bandgap device is capable of
> > a
> > * specific feature (above) or not. Return non-zero, if yes.
> > */
> >@@ -331,6 +334,7 @@ struct ti_temp_sensor {
> > #define TI_BANDGAP_FEATURE_FREEZE_BIT BIT(7)
> > #define TI_BANDGAP_FEATURE_COUNTER_DELAY BIT(8)
> > #define TI_BANDGAP_FEATURE_HISTORY_BUFFER BIT(9)
> >+#define TI_BANDGAP_FEATURE_ERRATA_814 BIT(10)
> > #define TI_BANDGAP_HAS(b, f) \
> > ((b)->conf->features & TI_BANDGAP_FEATURE_ ## f)
> >
> >
signature.asc
Description: Digital signature
