| Message ID | 1380010102-25817-4-git-send-email-l.majewski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Tue, Sep 24, 2013 at 1:38 PM, Lukasz Majewski <l.majewski@samsung.com> wrote: > The commit d0a0ce3e77c795258d47f9163e92d5031d0c5221 ("thermal: exynos: Add > missing definations and code cleanup") has removed setting of test MUX address > value at TMU configuration setting. > > This field is not present on Exynos4210 and Exynos5 SoCs. However on Exynos4412 > SoC it is required to set this field after reset because without it TMU shows > maximal available temperature, which causes immediate platform shutdown. Right In 5250 this field is not defined so didn't catch this. The changes looks fine but I have a minor comment that if this field is defined in 4412 in detail then you can add a field entry in exynos_tmu_registers with proper name and populate this field. The good thing is that in 5250 also this field is reserved and the default value is 0x6 so same TMU_DATA can be used for 5250 and 4412. The main idea of this suggestion is to reduce the soc checks in the driver. Thanks, Amit Daniel > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Reviewed-by: Tomasz Figa <t.figa@samsung.com> > --- > drivers/thermal/samsung/exynos_tmu.c | 3 +++ > drivers/thermal/samsung/exynos_tmu_data.h | 4 ++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c > index a858cc4..21b89e4 100644 > --- a/drivers/thermal/samsung/exynos_tmu.c > +++ b/drivers/thermal/samsung/exynos_tmu.c > @@ -317,6 +317,9 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on) > > con = readl(data->base + reg->tmu_ctrl); > > + if (pdata->type == SOC_ARCH_EXYNOS4412) > + con |= (EXYNOS4412_MUX_ADDR_VALUE << EXYNOS4412_MUX_ADDR_SHIFT); > + > if (pdata->reference_voltage) { > con &= ~(reg->buf_vref_sel_mask << reg->buf_vref_sel_shift); > con |= pdata->reference_voltage << reg->buf_vref_sel_shift; > diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h > index b130b1e..a1ea19d 100644 > --- a/drivers/thermal/samsung/exynos_tmu_data.h > +++ b/drivers/thermal/samsung/exynos_tmu_data.h > @@ -95,6 +95,10 @@ > > #define EXYNOS_MAX_TRIGGER_PER_REG 4 > > +/* Exynos4412 specific */ > +#define EXYNOS4412_MUX_ADDR_VALUE 6 > +#define EXYNOS4412_MUX_ADDR_SHIFT 20 > + > /*exynos5440 specific registers*/ > #define EXYNOS5440_TMU_S0_7_TRIM 0x000 > #define EXYNOS5440_TMU_S0_7_CTRL 0x020 > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html On Tue, Sep 24, 2013 at 1:38 PM, Lukasz Majewski <l.majewski@samsung.com> wrote: > The commit d0a0ce3e77c795258d47f9163e92d5031d0c5221 ("thermal: exynos: Add > missing definations and code cleanup") has removed setting of test MUX address > value at TMU configuration setting. > > This field is not present on Exynos4210 and Exynos5 SoCs. However on Exynos4412 > SoC it is required to set this field after reset because without it TMU shows > maximal available temperature, which causes immediate platform shutdown. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Reviewed-by: Tomasz Figa <t.figa@samsung.com> > --- > drivers/thermal/samsung/exynos_tmu.c | 3 +++ > drivers/thermal/samsung/exynos_tmu_data.h | 4 ++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c > index a858cc4..21b89e4 100644 > --- a/drivers/thermal/samsung/exynos_tmu.c > +++ b/drivers/thermal/samsung/exynos_tmu.c > @@ -317,6 +317,9 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on) > > con = readl(data->base + reg->tmu_ctrl); > > + if (pdata->type == SOC_ARCH_EXYNOS4412) > + con |= (EXYNOS4412_MUX_ADDR_VALUE << EXYNOS4412_MUX_ADDR_SHIFT); > + > if (pdata->reference_voltage) { > con &= ~(reg->buf_vref_sel_mask << reg->buf_vref_sel_shift); > con |= pdata->reference_voltage << reg->buf_vref_sel_shift; > diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h > index b130b1e..a1ea19d 100644 > --- a/drivers/thermal/samsung/exynos_tmu_data.h > +++ b/drivers/thermal/samsung/exynos_tmu_data.h > @@ -95,6 +95,10 @@ > > #define EXYNOS_MAX_TRIGGER_PER_REG 4 > > +/* Exynos4412 specific */ > +#define EXYNOS4412_MUX_ADDR_VALUE 6 > +#define EXYNOS4412_MUX_ADDR_SHIFT 20 > + > /*exynos5440 specific registers*/ > #define EXYNOS5440_TMU_S0_7_TRIM 0x000 > #define EXYNOS5440_TMU_S0_7_CTRL 0x020 > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Amit, > Hi, > > On Tue, Sep 24, 2013 at 1:38 PM, Lukasz Majewski > <l.majewski@samsung.com> wrote: > > The commit d0a0ce3e77c795258d47f9163e92d5031d0c5221 ("thermal: > > exynos: Add missing definations and code cleanup") has removed > > setting of test MUX address value at TMU configuration setting. > > > > This field is not present on Exynos4210 and Exynos5 SoCs. However > > on Exynos4412 SoC it is required to set this field after reset > > because without it TMU shows maximal available temperature, which > > causes immediate platform shutdown. > Right In 5250 this field is not defined so didn't catch this. The > changes looks fine but I have a minor comment that if this field is > defined in 4412 in detail then you can add a field entry in > exynos_tmu_registers with proper name and populate this field. It seems, that only at Exynos4412 (and Exynos4212) this field is valid. When I extent exynos_tmu_registers structure, then all other Samsung SoCs will be aware of it. Define with explicit EXYNOS4412 seems more readable. Also at exynos_tmu_control() function we use constructs like: data->base + EXYNOS_TMU_REG_CONTROL, not data->base + regs->tmu_ctrl. > The > good thing is that in 5250 also this field is reserved and the default > value is 0x6 so same TMU_DATA can be used for 5250 and 4412. I'm not keen to this kind of hacks. This field is only valid on Exynos4x12. And for Exynos5250 is reserved, which means that we shall not touch it. > The main > idea of this suggestion is to reduce the soc checks in the driver. Correct me if I'm wrong, but this MUX_ADDR initialization is performed at exynos_tmu_control() which is called at probe and thermal power management functions. Therefore, it seems that checking if SoC == Exynos4412 there is not an overkill. If you don't mind I would leave those patches as they are and kindly ask thermal maintainers for pulling them to v3.12. > > Thanks, > Amit Daniel > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > Reviewed-by: Tomasz Figa <t.figa@samsung.com> > > --- > > drivers/thermal/samsung/exynos_tmu.c | 3 +++ > > drivers/thermal/samsung/exynos_tmu_data.h | 4 ++++ > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/thermal/samsung/exynos_tmu.c > > b/drivers/thermal/samsung/exynos_tmu.c index a858cc4..21b89e4 100644 > > --- a/drivers/thermal/samsung/exynos_tmu.c > > +++ b/drivers/thermal/samsung/exynos_tmu.c > > @@ -317,6 +317,9 @@ static void exynos_tmu_control(struct > > platform_device *pdev, bool on) > > > > con = readl(data->base + reg->tmu_ctrl); > > > > + if (pdata->type == SOC_ARCH_EXYNOS4412) > > + con |= (EXYNOS4412_MUX_ADDR_VALUE << > > EXYNOS4412_MUX_ADDR_SHIFT); + > > if (pdata->reference_voltage) { > > con &= ~(reg->buf_vref_sel_mask << > > reg->buf_vref_sel_shift); con |= pdata->reference_voltage << > > reg->buf_vref_sel_shift; diff --git > > a/drivers/thermal/samsung/exynos_tmu_data.h > > b/drivers/thermal/samsung/exynos_tmu_data.h index b130b1e..a1ea19d > > 100644 --- a/drivers/thermal/samsung/exynos_tmu_data.h +++ > > b/drivers/thermal/samsung/exynos_tmu_data.h @@ -95,6 +95,10 @@ > > > > #define EXYNOS_MAX_TRIGGER_PER_REG 4 > > > > +/* Exynos4412 specific */ > > +#define EXYNOS4412_MUX_ADDR_VALUE 6 > > +#define EXYNOS4412_MUX_ADDR_SHIFT 20 > > + > > /*exynos5440 specific registers*/ > > #define EXYNOS5440_TMU_S0_7_TRIM 0x000 > > #define EXYNOS5440_TMU_S0_7_CTRL 0x020 > > -- > > 1.7.10.4 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pm" > > in the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > On Tue, Sep 24, 2013 at 1:38 PM, Lukasz Majewski > <l.majewski@samsung.com> wrote: > > The commit d0a0ce3e77c795258d47f9163e92d5031d0c5221 ("thermal: > > exynos: Add missing definations and code cleanup") has removed > > setting of test MUX address value at TMU configuration setting. > > > > This field is not present on Exynos4210 and Exynos5 SoCs. However > > on Exynos4412 SoC it is required to set this field after reset > > because without it TMU shows maximal available temperature, which > > causes immediate platform shutdown. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > Reviewed-by: Tomasz Figa <t.figa@samsung.com> > > --- > > drivers/thermal/samsung/exynos_tmu.c | 3 +++ > > drivers/thermal/samsung/exynos_tmu_data.h | 4 ++++ > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/thermal/samsung/exynos_tmu.c > > b/drivers/thermal/samsung/exynos_tmu.c index a858cc4..21b89e4 100644 > > --- a/drivers/thermal/samsung/exynos_tmu.c > > +++ b/drivers/thermal/samsung/exynos_tmu.c > > @@ -317,6 +317,9 @@ static void exynos_tmu_control(struct > > platform_device *pdev, bool on) > > > > con = readl(data->base + reg->tmu_ctrl); > > > > + if (pdata->type == SOC_ARCH_EXYNOS4412) > > + con |= (EXYNOS4412_MUX_ADDR_VALUE << > > EXYNOS4412_MUX_ADDR_SHIFT); + > > if (pdata->reference_voltage) { > > con &= ~(reg->buf_vref_sel_mask << > > reg->buf_vref_sel_shift); con |= pdata->reference_voltage << > > reg->buf_vref_sel_shift; diff --git > > a/drivers/thermal/samsung/exynos_tmu_data.h > > b/drivers/thermal/samsung/exynos_tmu_data.h index b130b1e..a1ea19d > > 100644 --- a/drivers/thermal/samsung/exynos_tmu_data.h +++ > > b/drivers/thermal/samsung/exynos_tmu_data.h @@ -95,6 +95,10 @@ > > > > #define EXYNOS_MAX_TRIGGER_PER_REG 4 > > > > +/* Exynos4412 specific */ > > +#define EXYNOS4412_MUX_ADDR_VALUE 6 > > +#define EXYNOS4412_MUX_ADDR_SHIFT 20 > > + > > /*exynos5440 specific registers*/ > > #define EXYNOS5440_TMU_S0_7_TRIM 0x000 > > #define EXYNOS5440_TMU_S0_7_CTRL 0x020 > > -- > > 1.7.10.4 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pm" > > in the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24-09-2013 04:08, Lukasz Majewski wrote: > The commit d0a0ce3e77c795258d47f9163e92d5031d0c5221 ("thermal: exynos: Add > missing definations and code cleanup") has removed setting of test MUX address > value at TMU configuration setting. > > This field is not present on Exynos4210 and Exynos5 SoCs. However on Exynos4412 > SoC it is required to set this field after reset because without it TMU shows > maximal available temperature, which causes immediate platform shutdown. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Reviewed-by: Tomasz Figa <t.figa@samsung.com> > --- > drivers/thermal/samsung/exynos_tmu.c | 3 +++ > drivers/thermal/samsung/exynos_tmu_data.h | 4 ++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c > index a858cc4..21b89e4 100644 > --- a/drivers/thermal/samsung/exynos_tmu.c > +++ b/drivers/thermal/samsung/exynos_tmu.c > @@ -317,6 +317,9 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on) > > con = readl(data->base + reg->tmu_ctrl); > > + if (pdata->type == SOC_ARCH_EXYNOS4412) > + con |= (EXYNOS4412_MUX_ADDR_VALUE << EXYNOS4412_MUX_ADDR_SHIFT); Amit has introduced a way to describe features instead of checking features per type. It would be interesting to have a reasoning why not to use it. Think what if new Exynos TMU versions come, are you guys going to steadily increase the above check for type? > + > if (pdata->reference_voltage) { > con &= ~(reg->buf_vref_sel_mask << reg->buf_vref_sel_shift); > con |= pdata->reference_voltage << reg->buf_vref_sel_shift; > diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h > index b130b1e..a1ea19d 100644 > --- a/drivers/thermal/samsung/exynos_tmu_data.h > +++ b/drivers/thermal/samsung/exynos_tmu_data.h > @@ -95,6 +95,10 @@ > > #define EXYNOS_MAX_TRIGGER_PER_REG 4 > > +/* Exynos4412 specific */ > +#define EXYNOS4412_MUX_ADDR_VALUE 6 > +#define EXYNOS4412_MUX_ADDR_SHIFT 20 > + > /*exynos5440 specific registers*/ > #define EXYNOS5440_TMU_S0_7_TRIM 0x000 > #define EXYNOS5440_TMU_S0_7_CTRL 0x020 >
Hi Eduardo, > On 24-09-2013 04:08, Lukasz Majewski wrote: > > The commit d0a0ce3e77c795258d47f9163e92d5031d0c5221 ("thermal: > > exynos: Add missing definations and code cleanup") has removed > > setting of test MUX address value at TMU configuration setting. > > > > This field is not present on Exynos4210 and Exynos5 SoCs. However > > on Exynos4412 SoC it is required to set this field after reset > > because without it TMU shows maximal available temperature, which > > causes immediate platform shutdown. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > Reviewed-by: Tomasz Figa <t.figa@samsung.com> > > --- > > drivers/thermal/samsung/exynos_tmu.c | 3 +++ > > drivers/thermal/samsung/exynos_tmu_data.h | 4 ++++ > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/thermal/samsung/exynos_tmu.c > > b/drivers/thermal/samsung/exynos_tmu.c index a858cc4..21b89e4 100644 > > --- a/drivers/thermal/samsung/exynos_tmu.c > > +++ b/drivers/thermal/samsung/exynos_tmu.c > > @@ -317,6 +317,9 @@ static void exynos_tmu_control(struct > > platform_device *pdev, bool on) > > con = readl(data->base + reg->tmu_ctrl); > > > > + if (pdata->type == SOC_ARCH_EXYNOS4412) > > + con |= (EXYNOS4412_MUX_ADDR_VALUE << > > EXYNOS4412_MUX_ADDR_SHIFT); > > Amit has introduced a way to describe features instead of checking > features per type. It would be interesting to have a reasoning why not > to use it. Problem with Exynos4412 and Exynos4212 is that _only_ those SoCs export this MUX_ADDR field at TMU_CONTROL register. Also I _must_ setup it correctly after reset (reset value causes board emergency shutdown). The Exynos5250 defines it as a "reserved". > Think what if new Exynos TMU versions come, are you guys > going to steadily increase the above check for type? As the alternative I can define the TMU_SUPPORT_MUX_ADDR at .features field for EXYNOS4412_TMU_DATA. Then I can test for this feature at exynos_tmu_control function. Proper shift and mask can be defined at struct exynos_tmu_registers. Eduardo, Amit, will we manage to review/pull those patches with new approach before -rcX ends? > > > + > > if (pdata->reference_voltage) { > > con &= ~(reg->buf_vref_sel_mask << > > reg->buf_vref_sel_shift); con |= pdata->reference_voltage << > > reg->buf_vref_sel_shift; diff --git > > a/drivers/thermal/samsung/exynos_tmu_data.h > > b/drivers/thermal/samsung/exynos_tmu_data.h index b130b1e..a1ea19d > > 100644 --- a/drivers/thermal/samsung/exynos_tmu_data.h +++ > > b/drivers/thermal/samsung/exynos_tmu_data.h @@ -95,6 +95,10 @@ > > > > #define EXYNOS_MAX_TRIGGER_PER_REG 4 > > > > +/* Exynos4412 specific */ > > +#define EXYNOS4412_MUX_ADDR_VALUE 6 > > +#define EXYNOS4412_MUX_ADDR_SHIFT 20 > > + > > /*exynos5440 specific registers*/ > > #define EXYNOS5440_TMU_S0_7_TRIM 0x000 > > #define EXYNOS5440_TMU_S0_7_CTRL 0x020 > > > >
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c index a858cc4..21b89e4 100644 --- a/drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/thermal/samsung/exynos_tmu.c @@ -317,6 +317,9 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on) con = readl(data->base + reg->tmu_ctrl); + if (pdata->type == SOC_ARCH_EXYNOS4412) + con |= (EXYNOS4412_MUX_ADDR_VALUE << EXYNOS4412_MUX_ADDR_SHIFT); + if (pdata->reference_voltage) { con &= ~(reg->buf_vref_sel_mask << reg->buf_vref_sel_shift); con |= pdata->reference_voltage << reg->buf_vref_sel_shift; diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h index b130b1e..a1ea19d 100644 --- a/drivers/thermal/samsung/exynos_tmu_data.h +++ b/drivers/thermal/samsung/exynos_tmu_data.h @@ -95,6 +95,10 @@ #define EXYNOS_MAX_TRIGGER_PER_REG 4 +/* Exynos4412 specific */ +#define EXYNOS4412_MUX_ADDR_VALUE 6 +#define EXYNOS4412_MUX_ADDR_SHIFT 20 + /*exynos5440 specific registers*/ #define EXYNOS5440_TMU_S0_7_TRIM 0x000 #define EXYNOS5440_TMU_S0_7_CTRL 0x020
The commit d0a0ce3e77c795258d47f9163e92d5031d0c5221 ("thermal: exynos: Add missing definations and code cleanup") has removed setting of test MUX address value at TMU configuration setting. This field is not present on Exynos4210 and Exynos5 SoCs. However on Exynos4412 SoC it is required to set this field after reset because without it TMU shows maximal available temperature, which causes immediate platform shutdown. Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Reviewed-by: Tomasz Figa <t.figa@samsung.com> --- drivers/thermal/samsung/exynos_tmu.c | 3 +++ drivers/thermal/samsung/exynos_tmu_data.h | 4 ++++ 2 files changed, 7 insertions(+)