diff mbox

[V4,22/30] thermal: exynos: Add support for exynos5440 TMU sensor.

Message ID 1368525540-15034-23-git-send-email-amit.daniel@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Amit Kachhap May 14, 2013, 9:58 a.m. UTC
This patch modifies TMU controller to add changes needed to work with
exynos5440 platform. This sensor registers 3 instance of the tmu controller
with the thermal zone and hence reports 3 temperature output. This controller
supports upto five trip points. For critical threshold the driver uses the
core driver thermal framework for shutdown.

Acked-by: Kukjin Kim <kgene.kim@samsung.com>
Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
---
 .../devicetree/bindings/thermal/exynos-thermal.txt |   28 ++++++++++++-
 drivers/thermal/samsung/exynos_tmu.c               |   43 +++++++++++++++++--
 drivers/thermal/samsung/exynos_tmu.h               |    6 +++
 drivers/thermal/samsung/exynos_tmu_data.h          |    2 +
 4 files changed, 72 insertions(+), 7 deletions(-)

Comments

Jonghwa Lee May 18, 2013, 5:23 a.m. UTC | #1
On 2013? 05? 14? 18:58, Amit Daniel Kachhap wrote:

> This patch modifies TMU controller to add changes needed to work with
> exynos5440 platform. This sensor registers 3 instance of the tmu controller
> with the thermal zone and hence reports 3 temperature output. This controller
> supports upto five trip points. For critical threshold the driver uses the
> core driver thermal framework for shutdown.
> 
> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> ---
>  .../devicetree/bindings/thermal/exynos-thermal.txt |   28 ++++++++++++-
>  drivers/thermal/samsung/exynos_tmu.c               |   43 +++++++++++++++++--
>  drivers/thermal/samsung/exynos_tmu.h               |    6 +++
>  drivers/thermal/samsung/exynos_tmu_data.h          |    2 +
>  4 files changed, 72 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
> index 535fd0e..970eeba 100644
> --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
> @@ -6,13 +6,16 @@
>  	       "samsung,exynos4412-tmu"
>  	       "samsung,exynos4210-tmu"
>  	       "samsung,exynos5250-tmu"
> +	       "samsung,exynos5440-tmu"
>  - interrupt-parent : The phandle for the interrupt controller
> -- reg : Address range of the thermal registers
> +- reg : Address range of the thermal registers. For exynos5440-tmu which has 3
> +	instances of TMU, 2 set of register has to supplied. First set belongs
> +	to each instance of TMU and second set belongs to common TMU registers.
>  - interrupts : Should contain interrupt for thermal system
>  - clocks : The main clock for TMU device
>  - clock-names : Thermal system clock name
>  
> -Example:
> +Example 1):
>  
>  	tmu@100C0000 {
>  		compatible = "samsung,exynos4412-tmu";
> @@ -23,3 +26,24 @@ Example:
>  		clock-names = "tmu_apbif";
>  		status = "disabled";
>  	};
> +
> +Example 2):
> +
> +	tmuctrl_0: tmuctrl@160118 {
> +		compatible = "samsung,exynos5440-tmu";
> +		reg = <0x160118 0x230>, <0x160368 0x10>;
> +		interrupts = <0 58 0>;
> +		clocks = <&clock 21>;
> +		clock-names = "tmu_apbif";
> +	};
> +
> +Note: For multi-instance tmu each instance should have an alias correctly
> +numbered in "aliases" node.
> +
> +Example:
> +
> +aliases {
> +	tmuctrl0 = &tmuctrl_0;
> +	tmuctrl1 = &tmuctrl_1;
> +	tmuctrl2 = &tmuctrl_2;
> +};
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 7f7b1cf..7ca9c4d 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -185,9 +185,11 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>  			reg->threshold_th0 + i * sizeof(reg->threshold_th0));
>  
>  		writel(reg->inten_rise_mask, data->base + reg->tmu_intclear);
> -	} else if (data->soc == SOC_ARCH_EXYNOS) {
> +	} else if (data->soc == SOC_ARCH_EXYNOS ||
> +			data->soc == SOC_ARCH_EXYNOS5440) {
>  		/* Write temperature code for rising and falling threshold */
> -		for (i = 0; i < trigger_levs; i++) {
> +		for (i = 0;
> +		i < trigger_levs && i < EXYNOS_MAX_TRIGGER_PER_REG; i++) {
>  			threshold_code = temp_to_code(data,
>  						pdata->trigger_levels[i]);
>  			if (threshold_code < 0) {
> @@ -218,7 +220,30 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>  		writel((reg->inten_rise_mask << reg->inten_rise_shift) |
>  			(reg->inten_fall_mask << reg->inten_fall_shift),
>  				data->base + reg->tmu_intclear);
> +
> +		/* if 5th threshold limit is also present, use TH2 register */
> +		i = EXYNOS_MAX_TRIGGER_PER_REG;
> +		if (pdata->trigger_levels[i]) {
> +			threshold_code = temp_to_code(data,
> +						pdata->trigger_levels[i]);
> +			if (threshold_code < 0) {
> +				ret = threshold_code;
> +				goto out;
> +			}
> +			rising_threshold =
> +				threshold_code << reg->threshold_th3_l0_shift;
> +			writel(rising_threshold,
> +				data->base + reg->threshold_th2);
> +			if (pdata->trigger_type[i] == HW_TRIP) {
> +				con = readl(data->base + reg->tmu_ctrl);
> +				con |= (1 << reg->therm_trip_en_shift);
> +				writel(con, data->base + reg->tmu_ctrl);
> +			}
> +		}
>  	}
> +	/*Clear the PMIN in the common TMU register*/
> +	if (reg->tmu_pmin && !data->id)
> +		writel(0, data->base_common + reg->tmu_pmin);
>  out:
>  	clk_disable(data->clk);
>  	mutex_unlock(&data->lock);
> @@ -345,7 +370,14 @@ static void exynos_tmu_work(struct work_struct *work)
>  			struct exynos_tmu_data, irq_work);
>  	struct exynos_tmu_platform_data *pdata = data->pdata;
>  	const struct exynos_tmu_registers *reg = pdata->registers;
> -	unsigned int val_irq;
> +	unsigned int val_irq, val_type;
> +
> +	/* Find which sensor generated this interrupt */
> +	if (reg->tmu_irqstatus) {
> +		val_type = readl(data->base_common + reg->tmu_irqstatus);
> +		if (!((val_type >> data->id) & 0x1))
> +			goto out;


I have a question about your implementation for supporting EXYNOS5440.
I don't know exactly how EXYNO5440's tmu is working, but just guess it would be
similar with other EXYNOS series's without number of thermal sensors. (exclusive
register map and threshold level). Due to the multiple number of thermal sensor
in EXYNOS5440, it have multiple thermal zone devices and that's why it just
leave interrupt pin in pending if interrupt is not its, right?

So, my curious is, why we make all platform devices for each of thermal zone
devices? Why don't you just handle all thermal zone devices with one platform
device?

Yes, It's probably right to make multiple devices node to support them, because
it has different physical hardware(sensors). But we have one TMU , don't we?
(Maybe my assumption is wrong, I assume that it has one TMU because it looks
like it has only one irq line.). If I'm right, I think it is better to manage
all thermal zone devices with one platform device. Then, we don't need to leave
irq handler with leaving it pendded like above and also we may not need other
your patches like adding base_common iomem variable.

I'd like to listen your opinion about this.

Thanks,
Jonghwa

> +	}
>  
>  	exynos_report_trigger(data->reg_conf);
>  	mutex_lock(&data->lock);
> @@ -358,7 +390,7 @@ static void exynos_tmu_work(struct work_struct *work)
>  
>  	clk_disable(data->clk);
>  	mutex_unlock(&data->lock);
> -
> +out:
>  	enable_irq(data->irq);
>  }
>  
> @@ -520,7 +552,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	if (pdata->type == SOC_ARCH_EXYNOS ||
> -				pdata->type == SOC_ARCH_EXYNOS4210)
> +		pdata->type == SOC_ARCH_EXYNOS4210 ||
> +				pdata->type == SOC_ARCH_EXYNOS5440)
>  		data->soc = pdata->type;
>  	else {
>  		ret = -EINVAL;
> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
> index 65443d7..9151a30 100644
> --- a/drivers/thermal/samsung/exynos_tmu.h
> +++ b/drivers/thermal/samsung/exynos_tmu.h
> @@ -44,6 +44,7 @@ enum trigger_type {
>  enum soc_type {
>  	SOC_ARCH_EXYNOS4210 = 1,
>  	SOC_ARCH_EXYNOS,
> +	SOC_ARCH_EXYNOS5440,
>  };
>  
>  /**
> @@ -132,6 +133,8 @@ enum soc_type {
>   * @emul_temp_shift: shift bits of emulation temperature.
>   * @emul_time_shift: shift bits of emulation time.
>   * @emul_time_mask: mask bits of emulation time.
> + * @tmu_irqstatus: register to find which TMU generated interrupts.
> + * @tmu_pmin: register to get/set the Pmin value.
>   */
>  struct exynos_tmu_registers {
>  	u32	triminfo_data;
> @@ -199,6 +202,9 @@ struct exynos_tmu_registers {
>  	u32	emul_temp_shift;
>  	u32	emul_time_shift;
>  	u32	emul_time_mask;
> +
> +	u32	tmu_irqstatus;
> +	u32	tmu_pmin;
>  };
>  
>  /**
> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
> index 0e2244f..4acf070 100644
> --- a/drivers/thermal/samsung/exynos_tmu_data.h
> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
> @@ -91,6 +91,8 @@
>  #define EXYNOS_EMUL_DATA_MASK	0xFF
>  #define EXYNOS_EMUL_ENABLE	0x1
>  
> +#define EXYNOS_MAX_TRIGGER_PER_REG	4
> +
>  #if defined(CONFIG_CPU_EXYNOS4210)
>  extern struct exynos_tmu_platform_data const exynos4210_default_tmu_data;
>  #define EXYNOS4210_TMU_DRV_DATA (&exynos4210_default_tmu_data)


--
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
Eduardo Valentin May 31, 2013, 3:28 p.m. UTC | #2
Amit and Jonghwa,

On 18-05-2013 01:23, jonghwa3.lee@samsung.com wrote:
> On 2013? 05? 14? 18:58, Amit Daniel Kachhap wrote:
> 
>> This patch modifies TMU controller to add changes needed to work with
>> exynos5440 platform. This sensor registers 3 instance of the tmu controller
>> with the thermal zone and hence reports 3 temperature output. This controller
>> supports upto five trip points. For critical threshold the driver uses the
>> core driver thermal framework for shutdown.
>>
>> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>> ---
>>  .../devicetree/bindings/thermal/exynos-thermal.txt |   28 ++++++++++++-
>>  drivers/thermal/samsung/exynos_tmu.c               |   43 +++++++++++++++++--
>>  drivers/thermal/samsung/exynos_tmu.h               |    6 +++
>>  drivers/thermal/samsung/exynos_tmu_data.h          |    2 +
>>  4 files changed, 72 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>> index 535fd0e..970eeba 100644

<cut>

>> +			goto out;
> 
> 
> I have a question about your implementation for supporting EXYNOS5440.
> I don't know exactly how EXYNO5440's tmu is working, but just guess it would be
> similar with other EXYNOS series's without number of thermal sensors. (exclusive
> register map and threshold level). Due to the multiple number of thermal sensor
> in EXYNOS5440, it have multiple thermal zone devices and that's why it just
> leave interrupt pin in pending if interrupt is not its, right?
> 
> So, my curious is, why we make all platform devices for each of thermal zone
> devices? Why don't you just handle all thermal zone devices with one platform
> device?
> 
> Yes, It's probably right to make multiple devices node to support them, because
> it has different physical hardware(sensors). But we have one TMU , don't we?
> (Maybe my assumption is wrong, I assume that it has one TMU because it looks
> like it has only one irq line.). If I'm right, I think it is better to manage
> all thermal zone devices with one platform device. Then, we don't need to leave
> irq handler with leaving it pendded like above and also we may not need other
> your patches like adding base_common iomem variable.
> 
> I'd like to listen your opinion about this.
> 


I understand the concern risen by Jonghwa. In fact, this is a bit
confusing. The way I have decided to design the driver for TI
(drivers/thermal/ti-soc-thermal under thermal tree next branch) is to
have one platform device for the bandgap IP (that would be probably
equivalent of  your TMU).

Reasoning is to have a exact match between platform device and real HW
device interface. Thus its device resources are belonging to one single
device node. In TIs case, the resources, regarding IRQs, IO map area,
registers, etc, are belonging to the bandgap IP not to sensors. That
alone convinced me to use one single device node, instead of several,
per sensor. In fact, for OMAP devices it is a bit more complicated as
the bandgap is actually behind the control module, which holds the
interface. But that is another story.

So, in this case I decided to have 1 single platform device representing
the bandgap IP, which exposes and handles several thermal zones (one per
sensor). And of course, owns and manages all related resources (IRQ,
gpio and IO mem area).

To what I have understood of your case, I believe it is the very same
case, so I would recommend reusing the proposed design.

Keep in mind that this obviously does not stop you of having different
policies or trip setups per sensor. The framework is flexible in this sense.

I hope this helps.

> Thanks,
> Jonghwa
> 
>> +	}
>>  
>>  	exynos_report_trigger(data->reg_conf);
>>  	mutex_lock(&data->lock);
>> @@ -358,7 +390,7 @@ static void exynos_tmu_work(struct work_struct *work)
>>  
>>  	clk_disable(data->clk);
>>  	mutex_unlock(&data->lock);
>> -
>> +out:
>>  	enable_irq(data->irq);
>>  }
>>  
>> @@ -520,7 +552,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>>  		return ret;
>>  
>>  	if (pdata->type == SOC_ARCH_EXYNOS ||
>> -				pdata->type == SOC_ARCH_EXYNOS4210)
>> +		pdata->type == SOC_ARCH_EXYNOS4210 ||
>> +				pdata->type == SOC_ARCH_EXYNOS5440)
>>  		data->soc = pdata->type;
>>  	else {
>>  		ret = -EINVAL;
>> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
>> index 65443d7..9151a30 100644
>> --- a/drivers/thermal/samsung/exynos_tmu.h
>> +++ b/drivers/thermal/samsung/exynos_tmu.h
>> @@ -44,6 +44,7 @@ enum trigger_type {
>>  enum soc_type {
>>  	SOC_ARCH_EXYNOS4210 = 1,
>>  	SOC_ARCH_EXYNOS,
>> +	SOC_ARCH_EXYNOS5440,
>>  };
>>  
>>  /**
>> @@ -132,6 +133,8 @@ enum soc_type {
>>   * @emul_temp_shift: shift bits of emulation temperature.
>>   * @emul_time_shift: shift bits of emulation time.
>>   * @emul_time_mask: mask bits of emulation time.
>> + * @tmu_irqstatus: register to find which TMU generated interrupts.
>> + * @tmu_pmin: register to get/set the Pmin value.
>>   */
>>  struct exynos_tmu_registers {
>>  	u32	triminfo_data;
>> @@ -199,6 +202,9 @@ struct exynos_tmu_registers {
>>  	u32	emul_temp_shift;
>>  	u32	emul_time_shift;
>>  	u32	emul_time_mask;
>> +
>> +	u32	tmu_irqstatus;
>> +	u32	tmu_pmin;
>>  };
>>  
>>  /**
>> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
>> index 0e2244f..4acf070 100644
>> --- a/drivers/thermal/samsung/exynos_tmu_data.h
>> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
>> @@ -91,6 +91,8 @@
>>  #define EXYNOS_EMUL_DATA_MASK	0xFF
>>  #define EXYNOS_EMUL_ENABLE	0x1
>>  
>> +#define EXYNOS_MAX_TRIGGER_PER_REG	4
>> +
>>  #if defined(CONFIG_CPU_EXYNOS4210)
>>  extern struct exynos_tmu_platform_data const exynos4210_default_tmu_data;
>>  #define EXYNOS4210_TMU_DRV_DATA (&exynos4210_default_tmu_data)
> 
> 
> 
>
Amit Kachhap June 4, 2013, 4:44 a.m. UTC | #3
Hi Jonghwa,

Sorry for the late reply as I was on leave.

On Sat, May 18, 2013 at 10:53 AM,  <jonghwa3.lee@samsung.com> wrote:
> On 2013? 05? 14? 18:58, Amit Daniel Kachhap wrote:
>
>> This patch modifies TMU controller to add changes needed to work with
>> exynos5440 platform. This sensor registers 3 instance of the tmu controller
>> with the thermal zone and hence reports 3 temperature output. This controller
>> supports upto five trip points. For critical threshold the driver uses the
>> core driver thermal framework for shutdown.
>>
>> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>> ---
>>  .../devicetree/bindings/thermal/exynos-thermal.txt |   28 ++++++++++++-
>>  drivers/thermal/samsung/exynos_tmu.c               |   43 +++++++++++++++++--
>>  drivers/thermal/samsung/exynos_tmu.h               |    6 +++
>>  drivers/thermal/samsung/exynos_tmu_data.h          |    2 +
>>  4 files changed, 72 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>> index 535fd0e..970eeba 100644
>> --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>> +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>> @@ -6,13 +6,16 @@
>>              "samsung,exynos4412-tmu"
>>              "samsung,exynos4210-tmu"
>>              "samsung,exynos5250-tmu"
>> +            "samsung,exynos5440-tmu"
>>  - interrupt-parent : The phandle for the interrupt controller
>> -- reg : Address range of the thermal registers
>> +- reg : Address range of the thermal registers. For exynos5440-tmu which has 3
>> +     instances of TMU, 2 set of register has to supplied. First set belongs
>> +     to each instance of TMU and second set belongs to common TMU registers.
>>  - interrupts : Should contain interrupt for thermal system
>>  - clocks : The main clock for TMU device
>>  - clock-names : Thermal system clock name
>>
>> -Example:
>> +Example 1):
>>
>>       tmu@100C0000 {
>>               compatible = "samsung,exynos4412-tmu";
>> @@ -23,3 +26,24 @@ Example:
>>               clock-names = "tmu_apbif";
>>               status = "disabled";
>>       };
>> +
>> +Example 2):
>> +
>> +     tmuctrl_0: tmuctrl@160118 {
>> +             compatible = "samsung,exynos5440-tmu";
>> +             reg = <0x160118 0x230>, <0x160368 0x10>;
>> +             interrupts = <0 58 0>;
>> +             clocks = <&clock 21>;
>> +             clock-names = "tmu_apbif";
>> +     };
>> +
>> +Note: For multi-instance tmu each instance should have an alias correctly
>> +numbered in "aliases" node.
>> +
>> +Example:
>> +
>> +aliases {
>> +     tmuctrl0 = &tmuctrl_0;
>> +     tmuctrl1 = &tmuctrl_1;
>> +     tmuctrl2 = &tmuctrl_2;
>> +};
>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>> index 7f7b1cf..7ca9c4d 100644
>> --- a/drivers/thermal/samsung/exynos_tmu.c
>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>> @@ -185,9 +185,11 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>                       reg->threshold_th0 + i * sizeof(reg->threshold_th0));
>>
>>               writel(reg->inten_rise_mask, data->base + reg->tmu_intclear);
>> -     } else if (data->soc == SOC_ARCH_EXYNOS) {
>> +     } else if (data->soc == SOC_ARCH_EXYNOS ||
>> +                     data->soc == SOC_ARCH_EXYNOS5440) {
>>               /* Write temperature code for rising and falling threshold */
>> -             for (i = 0; i < trigger_levs; i++) {
>> +             for (i = 0;
>> +             i < trigger_levs && i < EXYNOS_MAX_TRIGGER_PER_REG; i++) {
>>                       threshold_code = temp_to_code(data,
>>                                               pdata->trigger_levels[i]);
>>                       if (threshold_code < 0) {
>> @@ -218,7 +220,30 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>               writel((reg->inten_rise_mask << reg->inten_rise_shift) |
>>                       (reg->inten_fall_mask << reg->inten_fall_shift),
>>                               data->base + reg->tmu_intclear);
>> +
>> +             /* if 5th threshold limit is also present, use TH2 register */
>> +             i = EXYNOS_MAX_TRIGGER_PER_REG;
>> +             if (pdata->trigger_levels[i]) {
>> +                     threshold_code = temp_to_code(data,
>> +                                             pdata->trigger_levels[i]);
>> +                     if (threshold_code < 0) {
>> +                             ret = threshold_code;
>> +                             goto out;
>> +                     }
>> +                     rising_threshold =
>> +                             threshold_code << reg->threshold_th3_l0_shift;
>> +                     writel(rising_threshold,
>> +                             data->base + reg->threshold_th2);
>> +                     if (pdata->trigger_type[i] == HW_TRIP) {
>> +                             con = readl(data->base + reg->tmu_ctrl);
>> +                             con |= (1 << reg->therm_trip_en_shift);
>> +                             writel(con, data->base + reg->tmu_ctrl);
>> +                     }
>> +             }
>>       }
>> +     /*Clear the PMIN in the common TMU register*/
>> +     if (reg->tmu_pmin && !data->id)
>> +             writel(0, data->base_common + reg->tmu_pmin);
>>  out:
>>       clk_disable(data->clk);
>>       mutex_unlock(&data->lock);
>> @@ -345,7 +370,14 @@ static void exynos_tmu_work(struct work_struct *work)
>>                       struct exynos_tmu_data, irq_work);
>>       struct exynos_tmu_platform_data *pdata = data->pdata;
>>       const struct exynos_tmu_registers *reg = pdata->registers;
>> -     unsigned int val_irq;
>> +     unsigned int val_irq, val_type;
>> +
>> +     /* Find which sensor generated this interrupt */
>> +     if (reg->tmu_irqstatus) {
>> +             val_type = readl(data->base_common + reg->tmu_irqstatus);
>> +             if (!((val_type >> data->id) & 0x1))
>> +                     goto out;
>
>
> I have a question about your implementation for supporting EXYNOS5440.
> I don't know exactly how EXYNO5440's tmu is working, but just guess it would be
> similar with other EXYNOS series's without number of thermal sensors. (exclusive
> register map and threshold level). Due to the multiple number of thermal sensor
> in EXYNOS5440, it have multiple thermal zone devices and that's why it just
> leave interrupt pin in pending if interrupt is not its, right?
Yes in 5440 the interrupt line is shared so pending bit is left uncleared.
>
> So, my curious is, why we make all platform devices for each of thermal zone
> devices? Why don't you just handle all thermal zone devices with one platform
> device?
Your doubt is genuine. Let me justify my design decision.
Initially I also thought of making a single platform device but since
there are 3 different TMU controllers and register maps for 4 more so
I followed this design as the driver looks clean and can be scalable
easily. Also I agree that some resources like IRQ line is shared but
it is due to h/w limitation.
Also it is easy to cleanly control each TMU instance with the device
tree data similar to I2C/SPI/MMC instance based device driver. Say I
do not want to use 2nd sensor then just pass device tree data for 1st
and 3rd sensor.
>
> Yes, It's probably right to make multiple devices node to support them, because
> it has different physical hardware(sensors). But we have one TMU , don't we?
> (Maybe my assumption is wrong, I assume that it has one TMU because it looks
> like it has only one irq line.). If I'm right, I think it is better to manage
> all thermal zone devices with one platform device. Then, we don't need to leave
> irq handler with leaving it pendded like above and also we may not need other
> your patches like adding base_common iomem variable.
Agreed that base_common variables is extra and present to handle the
common part. I will further analyse your suggestion.

Thanks,
Amit Daniel
>
> I'd like to listen your opinion about this.
>
> Thanks,
> Jonghwa
>
>> +     }
>>
>>       exynos_report_trigger(data->reg_conf);
>>       mutex_lock(&data->lock);
>> @@ -358,7 +390,7 @@ static void exynos_tmu_work(struct work_struct *work)
>>
>>       clk_disable(data->clk);
>>       mutex_unlock(&data->lock);
>> -
>> +out:
>>       enable_irq(data->irq);
>>  }
>>
>> @@ -520,7 +552,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>>               return ret;
>>
>>       if (pdata->type == SOC_ARCH_EXYNOS ||
>> -                             pdata->type == SOC_ARCH_EXYNOS4210)
>> +             pdata->type == SOC_ARCH_EXYNOS4210 ||
>> +                             pdata->type == SOC_ARCH_EXYNOS5440)
>>               data->soc = pdata->type;
>>       else {
>>               ret = -EINVAL;
>> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
>> index 65443d7..9151a30 100644
>> --- a/drivers/thermal/samsung/exynos_tmu.h
>> +++ b/drivers/thermal/samsung/exynos_tmu.h
>> @@ -44,6 +44,7 @@ enum trigger_type {
>>  enum soc_type {
>>       SOC_ARCH_EXYNOS4210 = 1,
>>       SOC_ARCH_EXYNOS,
>> +     SOC_ARCH_EXYNOS5440,
>>  };
>>
>>  /**
>> @@ -132,6 +133,8 @@ enum soc_type {
>>   * @emul_temp_shift: shift bits of emulation temperature.
>>   * @emul_time_shift: shift bits of emulation time.
>>   * @emul_time_mask: mask bits of emulation time.
>> + * @tmu_irqstatus: register to find which TMU generated interrupts.
>> + * @tmu_pmin: register to get/set the Pmin value.
>>   */
>>  struct exynos_tmu_registers {
>>       u32     triminfo_data;
>> @@ -199,6 +202,9 @@ struct exynos_tmu_registers {
>>       u32     emul_temp_shift;
>>       u32     emul_time_shift;
>>       u32     emul_time_mask;
>> +
>> +     u32     tmu_irqstatus;
>> +     u32     tmu_pmin;
>>  };
>>
>>  /**
>> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
>> index 0e2244f..4acf070 100644
>> --- a/drivers/thermal/samsung/exynos_tmu_data.h
>> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
>> @@ -91,6 +91,8 @@
>>  #define EXYNOS_EMUL_DATA_MASK        0xFF
>>  #define EXYNOS_EMUL_ENABLE   0x1
>>
>> +#define EXYNOS_MAX_TRIGGER_PER_REG   4
>> +
>>  #if defined(CONFIG_CPU_EXYNOS4210)
>>  extern struct exynos_tmu_platform_data const exynos4210_default_tmu_data;
>>  #define EXYNOS4210_TMU_DRV_DATA (&exynos4210_default_tmu_data)
>
>
--
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
Eduardo Valentin June 4, 2013, 12:55 p.m. UTC | #4
On 04-06-2013 00:44, amit daniel kachhap wrote:
> Hi Jonghwa,
> 
> Sorry for the late reply as I was on leave.
> 
> On Sat, May 18, 2013 at 10:53 AM,  <jonghwa3.lee@samsung.com> wrote:
>> On 2013? 05? 14? 18:58, Amit Daniel Kachhap wrote:
>>
>>> This patch modifies TMU controller to add changes needed to work with
>>> exynos5440 platform. This sensor registers 3 instance of the tmu controller
>>> with the thermal zone and hence reports 3 temperature output. This controller
>>> supports upto five trip points. For critical threshold the driver uses the
>>> core driver thermal framework for shutdown.
>>>
>>> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>> ---
>>>  .../devicetree/bindings/thermal/exynos-thermal.txt |   28 ++++++++++++-
>>>  drivers/thermal/samsung/exynos_tmu.c               |   43 +++++++++++++++++--
>>>  drivers/thermal/samsung/exynos_tmu.h               |    6 +++
>>>  drivers/thermal/samsung/exynos_tmu_data.h          |    2 +
>>>  4 files changed, 72 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>> index 535fd0e..970eeba 100644
>>> --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>> +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>> @@ -6,13 +6,16 @@
>>>              "samsung,exynos4412-tmu"
>>>              "samsung,exynos4210-tmu"
>>>              "samsung,exynos5250-tmu"
>>> +            "samsung,exynos5440-tmu"
>>>  - interrupt-parent : The phandle for the interrupt controller
>>> -- reg : Address range of the thermal registers
>>> +- reg : Address range of the thermal registers. For exynos5440-tmu which has 3
>>> +     instances of TMU, 2 set of register has to supplied. First set belongs
>>> +     to each instance of TMU and second set belongs to common TMU registers.
>>>  - interrupts : Should contain interrupt for thermal system
>>>  - clocks : The main clock for TMU device
>>>  - clock-names : Thermal system clock name
>>>
>>> -Example:
>>> +Example 1):
>>>
>>>       tmu@100C0000 {
>>>               compatible = "samsung,exynos4412-tmu";
>>> @@ -23,3 +26,24 @@ Example:
>>>               clock-names = "tmu_apbif";
>>>               status = "disabled";
>>>       };
>>> +
>>> +Example 2):
>>> +
>>> +     tmuctrl_0: tmuctrl@160118 {
>>> +             compatible = "samsung,exynos5440-tmu";
>>> +             reg = <0x160118 0x230>, <0x160368 0x10>;
>>> +             interrupts = <0 58 0>;
>>> +             clocks = <&clock 21>;
>>> +             clock-names = "tmu_apbif";
>>> +     };
>>> +
>>> +Note: For multi-instance tmu each instance should have an alias correctly
>>> +numbered in "aliases" node.
>>> +
>>> +Example:
>>> +
>>> +aliases {
>>> +     tmuctrl0 = &tmuctrl_0;
>>> +     tmuctrl1 = &tmuctrl_1;
>>> +     tmuctrl2 = &tmuctrl_2;
>>> +};
>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>> index 7f7b1cf..7ca9c4d 100644
>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>> @@ -185,9 +185,11 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>                       reg->threshold_th0 + i * sizeof(reg->threshold_th0));
>>>
>>>               writel(reg->inten_rise_mask, data->base + reg->tmu_intclear);
>>> -     } else if (data->soc == SOC_ARCH_EXYNOS) {
>>> +     } else if (data->soc == SOC_ARCH_EXYNOS ||
>>> +                     data->soc == SOC_ARCH_EXYNOS5440) {
>>>               /* Write temperature code for rising and falling threshold */
>>> -             for (i = 0; i < trigger_levs; i++) {
>>> +             for (i = 0;
>>> +             i < trigger_levs && i < EXYNOS_MAX_TRIGGER_PER_REG; i++) {
>>>                       threshold_code = temp_to_code(data,
>>>                                               pdata->trigger_levels[i]);
>>>                       if (threshold_code < 0) {
>>> @@ -218,7 +220,30 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>               writel((reg->inten_rise_mask << reg->inten_rise_shift) |
>>>                       (reg->inten_fall_mask << reg->inten_fall_shift),
>>>                               data->base + reg->tmu_intclear);
>>> +
>>> +             /* if 5th threshold limit is also present, use TH2 register */
>>> +             i = EXYNOS_MAX_TRIGGER_PER_REG;
>>> +             if (pdata->trigger_levels[i]) {
>>> +                     threshold_code = temp_to_code(data,
>>> +                                             pdata->trigger_levels[i]);
>>> +                     if (threshold_code < 0) {
>>> +                             ret = threshold_code;
>>> +                             goto out;
>>> +                     }
>>> +                     rising_threshold =
>>> +                             threshold_code << reg->threshold_th3_l0_shift;
>>> +                     writel(rising_threshold,
>>> +                             data->base + reg->threshold_th2);
>>> +                     if (pdata->trigger_type[i] == HW_TRIP) {
>>> +                             con = readl(data->base + reg->tmu_ctrl);
>>> +                             con |= (1 << reg->therm_trip_en_shift);
>>> +                             writel(con, data->base + reg->tmu_ctrl);
>>> +                     }
>>> +             }
>>>       }
>>> +     /*Clear the PMIN in the common TMU register*/
>>> +     if (reg->tmu_pmin && !data->id)
>>> +             writel(0, data->base_common + reg->tmu_pmin);
>>>  out:
>>>       clk_disable(data->clk);
>>>       mutex_unlock(&data->lock);
>>> @@ -345,7 +370,14 @@ static void exynos_tmu_work(struct work_struct *work)
>>>                       struct exynos_tmu_data, irq_work);
>>>       struct exynos_tmu_platform_data *pdata = data->pdata;
>>>       const struct exynos_tmu_registers *reg = pdata->registers;
>>> -     unsigned int val_irq;
>>> +     unsigned int val_irq, val_type;
>>> +
>>> +     /* Find which sensor generated this interrupt */
>>> +     if (reg->tmu_irqstatus) {
>>> +             val_type = readl(data->base_common + reg->tmu_irqstatus);
>>> +             if (!((val_type >> data->id) & 0x1))
>>> +                     goto out;
>>
>>
>> I have a question about your implementation for supporting EXYNOS5440.
>> I don't know exactly how EXYNO5440's tmu is working, but just guess it would be
>> similar with other EXYNOS series's without number of thermal sensors. (exclusive
>> register map and threshold level). Due to the multiple number of thermal sensor
>> in EXYNOS5440, it have multiple thermal zone devices and that's why it just
>> leave interrupt pin in pending if interrupt is not its, right?
> Yes in 5440 the interrupt line is shared so pending bit is left uncleared.
>>
>> So, my curious is, why we make all platform devices for each of thermal zone
>> devices? Why don't you just handle all thermal zone devices with one platform
>> device?
> Your doubt is genuine. Let me justify my design decision.
> Initially I also thought of making a single platform device but since
> there are 3 different TMU controllers and register maps for 4 more so
> I followed this design as the driver looks clean and can be scalable
> easily. Also I agree that some resources like IRQ line is shared but
> it is due to h/w limitation.
> Also it is easy to cleanly control each TMU instance with the device
> tree data similar to I2C/SPI/MMC instance based device driver. Say I
> do not want to use 2nd sensor then just pass device tree data for 1st
> and 3rd sensor.
>>
>> Yes, It's probably right to make multiple devices node to support them, because
>> it has different physical hardware(sensors). But we have one TMU , don't we?
>> (Maybe my assumption is wrong, I assume that it has one TMU because it looks
>> like it has only one irq line.). If I'm right, I think it is better to manage
>> all thermal zone devices with one platform device. Then, we don't need to leave
>> irq handler with leaving it pendded like above and also we may not need other
>> your patches like adding base_common iomem variable.
> Agreed that base_common variables is extra and present to handle the
> common part. I will further analyse your suggestion.
> 

What is the relation TMU <--> temperature sensor? Is it one to one or
one to many?

> Thanks,
> Amit Daniel
>>
>> I'd like to listen your opinion about this.
>>
>> Thanks,
>> Jonghwa
>>
>>> +     }
>>>
>>>       exynos_report_trigger(data->reg_conf);
>>>       mutex_lock(&data->lock);
>>> @@ -358,7 +390,7 @@ static void exynos_tmu_work(struct work_struct *work)
>>>
>>>       clk_disable(data->clk);
>>>       mutex_unlock(&data->lock);
>>> -
>>> +out:
>>>       enable_irq(data->irq);
>>>  }
>>>
>>> @@ -520,7 +552,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>>>               return ret;
>>>
>>>       if (pdata->type == SOC_ARCH_EXYNOS ||
>>> -                             pdata->type == SOC_ARCH_EXYNOS4210)
>>> +             pdata->type == SOC_ARCH_EXYNOS4210 ||
>>> +                             pdata->type == SOC_ARCH_EXYNOS5440)
>>>               data->soc = pdata->type;
>>>       else {
>>>               ret = -EINVAL;
>>> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
>>> index 65443d7..9151a30 100644
>>> --- a/drivers/thermal/samsung/exynos_tmu.h
>>> +++ b/drivers/thermal/samsung/exynos_tmu.h
>>> @@ -44,6 +44,7 @@ enum trigger_type {
>>>  enum soc_type {
>>>       SOC_ARCH_EXYNOS4210 = 1,
>>>       SOC_ARCH_EXYNOS,
>>> +     SOC_ARCH_EXYNOS5440,
>>>  };
>>>
>>>  /**
>>> @@ -132,6 +133,8 @@ enum soc_type {
>>>   * @emul_temp_shift: shift bits of emulation temperature.
>>>   * @emul_time_shift: shift bits of emulation time.
>>>   * @emul_time_mask: mask bits of emulation time.
>>> + * @tmu_irqstatus: register to find which TMU generated interrupts.
>>> + * @tmu_pmin: register to get/set the Pmin value.
>>>   */
>>>  struct exynos_tmu_registers {
>>>       u32     triminfo_data;
>>> @@ -199,6 +202,9 @@ struct exynos_tmu_registers {
>>>       u32     emul_temp_shift;
>>>       u32     emul_time_shift;
>>>       u32     emul_time_mask;
>>> +
>>> +     u32     tmu_irqstatus;
>>> +     u32     tmu_pmin;
>>>  };
>>>
>>>  /**
>>> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
>>> index 0e2244f..4acf070 100644
>>> --- a/drivers/thermal/samsung/exynos_tmu_data.h
>>> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
>>> @@ -91,6 +91,8 @@
>>>  #define EXYNOS_EMUL_DATA_MASK        0xFF
>>>  #define EXYNOS_EMUL_ENABLE   0x1
>>>
>>> +#define EXYNOS_MAX_TRIGGER_PER_REG   4
>>> +
>>>  #if defined(CONFIG_CPU_EXYNOS4210)
>>>  extern struct exynos_tmu_platform_data const exynos4210_default_tmu_data;
>>>  #define EXYNOS4210_TMU_DRV_DATA (&exynos4210_default_tmu_data)
>>
>>
> 
>
Amit Kachhap June 5, 2013, 3:20 a.m. UTC | #5
Hi Eduardo,

On Tue, Jun 4, 2013 at 6:25 PM, Eduardo Valentin
<eduardo.valentin@ti.com> wrote:
> On 04-06-2013 00:44, amit daniel kachhap wrote:
>> Hi Jonghwa,
>>
>> Sorry for the late reply as I was on leave.
>>
>> On Sat, May 18, 2013 at 10:53 AM,  <jonghwa3.lee@samsung.com> wrote:
>>> On 2013? 05? 14? 18:58, Amit Daniel Kachhap wrote:
>>>
>>>> This patch modifies TMU controller to add changes needed to work with
>>>> exynos5440 platform. This sensor registers 3 instance of the tmu controller
>>>> with the thermal zone and hence reports 3 temperature output. This controller
>>>> supports upto five trip points. For critical threshold the driver uses the
>>>> core driver thermal framework for shutdown.
>>>>
>>>> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
>>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>> ---
>>>>  .../devicetree/bindings/thermal/exynos-thermal.txt |   28 ++++++++++++-
>>>>  drivers/thermal/samsung/exynos_tmu.c               |   43 +++++++++++++++++--
>>>>  drivers/thermal/samsung/exynos_tmu.h               |    6 +++
>>>>  drivers/thermal/samsung/exynos_tmu_data.h          |    2 +
>>>>  4 files changed, 72 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>> index 535fd0e..970eeba 100644
>>>> --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>> +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>> @@ -6,13 +6,16 @@
>>>>              "samsung,exynos4412-tmu"
>>>>              "samsung,exynos4210-tmu"
>>>>              "samsung,exynos5250-tmu"
>>>> +            "samsung,exynos5440-tmu"
>>>>  - interrupt-parent : The phandle for the interrupt controller
>>>> -- reg : Address range of the thermal registers
>>>> +- reg : Address range of the thermal registers. For exynos5440-tmu which has 3
>>>> +     instances of TMU, 2 set of register has to supplied. First set belongs
>>>> +     to each instance of TMU and second set belongs to common TMU registers.
>>>>  - interrupts : Should contain interrupt for thermal system
>>>>  - clocks : The main clock for TMU device
>>>>  - clock-names : Thermal system clock name
>>>>
>>>> -Example:
>>>> +Example 1):
>>>>
>>>>       tmu@100C0000 {
>>>>               compatible = "samsung,exynos4412-tmu";
>>>> @@ -23,3 +26,24 @@ Example:
>>>>               clock-names = "tmu_apbif";
>>>>               status = "disabled";
>>>>       };
>>>> +
>>>> +Example 2):
>>>> +
>>>> +     tmuctrl_0: tmuctrl@160118 {
>>>> +             compatible = "samsung,exynos5440-tmu";
>>>> +             reg = <0x160118 0x230>, <0x160368 0x10>;
>>>> +             interrupts = <0 58 0>;
>>>> +             clocks = <&clock 21>;
>>>> +             clock-names = "tmu_apbif";
>>>> +     };
>>>> +
>>>> +Note: For multi-instance tmu each instance should have an alias correctly
>>>> +numbered in "aliases" node.
>>>> +
>>>> +Example:
>>>> +
>>>> +aliases {
>>>> +     tmuctrl0 = &tmuctrl_0;
>>>> +     tmuctrl1 = &tmuctrl_1;
>>>> +     tmuctrl2 = &tmuctrl_2;
>>>> +};
>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>>> index 7f7b1cf..7ca9c4d 100644
>>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>>> @@ -185,9 +185,11 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>                       reg->threshold_th0 + i * sizeof(reg->threshold_th0));
>>>>
>>>>               writel(reg->inten_rise_mask, data->base + reg->tmu_intclear);
>>>> -     } else if (data->soc == SOC_ARCH_EXYNOS) {
>>>> +     } else if (data->soc == SOC_ARCH_EXYNOS ||
>>>> +                     data->soc == SOC_ARCH_EXYNOS5440) {
>>>>               /* Write temperature code for rising and falling threshold */
>>>> -             for (i = 0; i < trigger_levs; i++) {
>>>> +             for (i = 0;
>>>> +             i < trigger_levs && i < EXYNOS_MAX_TRIGGER_PER_REG; i++) {
>>>>                       threshold_code = temp_to_code(data,
>>>>                                               pdata->trigger_levels[i]);
>>>>                       if (threshold_code < 0) {
>>>> @@ -218,7 +220,30 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>               writel((reg->inten_rise_mask << reg->inten_rise_shift) |
>>>>                       (reg->inten_fall_mask << reg->inten_fall_shift),
>>>>                               data->base + reg->tmu_intclear);
>>>> +
>>>> +             /* if 5th threshold limit is also present, use TH2 register */
>>>> +             i = EXYNOS_MAX_TRIGGER_PER_REG;
>>>> +             if (pdata->trigger_levels[i]) {
>>>> +                     threshold_code = temp_to_code(data,
>>>> +                                             pdata->trigger_levels[i]);
>>>> +                     if (threshold_code < 0) {
>>>> +                             ret = threshold_code;
>>>> +                             goto out;
>>>> +                     }
>>>> +                     rising_threshold =
>>>> +                             threshold_code << reg->threshold_th3_l0_shift;
>>>> +                     writel(rising_threshold,
>>>> +                             data->base + reg->threshold_th2);
>>>> +                     if (pdata->trigger_type[i] == HW_TRIP) {
>>>> +                             con = readl(data->base + reg->tmu_ctrl);
>>>> +                             con |= (1 << reg->therm_trip_en_shift);
>>>> +                             writel(con, data->base + reg->tmu_ctrl);
>>>> +                     }
>>>> +             }
>>>>       }
>>>> +     /*Clear the PMIN in the common TMU register*/
>>>> +     if (reg->tmu_pmin && !data->id)
>>>> +             writel(0, data->base_common + reg->tmu_pmin);
>>>>  out:
>>>>       clk_disable(data->clk);
>>>>       mutex_unlock(&data->lock);
>>>> @@ -345,7 +370,14 @@ static void exynos_tmu_work(struct work_struct *work)
>>>>                       struct exynos_tmu_data, irq_work);
>>>>       struct exynos_tmu_platform_data *pdata = data->pdata;
>>>>       const struct exynos_tmu_registers *reg = pdata->registers;
>>>> -     unsigned int val_irq;
>>>> +     unsigned int val_irq, val_type;
>>>> +
>>>> +     /* Find which sensor generated this interrupt */
>>>> +     if (reg->tmu_irqstatus) {
>>>> +             val_type = readl(data->base_common + reg->tmu_irqstatus);
>>>> +             if (!((val_type >> data->id) & 0x1))
>>>> +                     goto out;
>>>
>>>
>>> I have a question about your implementation for supporting EXYNOS5440.
>>> I don't know exactly how EXYNO5440's tmu is working, but just guess it would be
>>> similar with other EXYNOS series's without number of thermal sensors. (exclusive
>>> register map and threshold level). Due to the multiple number of thermal sensor
>>> in EXYNOS5440, it have multiple thermal zone devices and that's why it just
>>> leave interrupt pin in pending if interrupt is not its, right?
>> Yes in 5440 the interrupt line is shared so pending bit is left uncleared.
>>>
>>> So, my curious is, why we make all platform devices for each of thermal zone
>>> devices? Why don't you just handle all thermal zone devices with one platform
>>> device?
>> Your doubt is genuine. Let me justify my design decision.
>> Initially I also thought of making a single platform device but since
>> there are 3 different TMU controllers and register maps for 4 more so
>> I followed this design as the driver looks clean and can be scalable
>> easily. Also I agree that some resources like IRQ line is shared but
>> it is due to h/w limitation.
>> Also it is easy to cleanly control each TMU instance with the device
>> tree data similar to I2C/SPI/MMC instance based device driver. Say I
>> do not want to use 2nd sensor then just pass device tree data for 1st
>> and 3rd sensor.
>>>
>>> Yes, It's probably right to make multiple devices node to support them, because
>>> it has different physical hardware(sensors). But we have one TMU , don't we?
>>> (Maybe my assumption is wrong, I assume that it has one TMU because it looks
>>> like it has only one irq line.). If I'm right, I think it is better to manage
>>> all thermal zone devices with one platform device. Then, we don't need to leave
>>> irq handler with leaving it pendded like above and also we may not need other
>>> your patches like adding base_common iomem variable.
>> Agreed that base_common variables is extra and present to handle the
>> common part. I will further analyse your suggestion.
>>
>
> What is the relation TMU <--> temperature sensor? Is it one to one or
> one to many?
1 TMU --- > 1 temp sensor.(one to one)

Thanks,
Amit Daniel
>
>> Thanks,
>> Amit Daniel
>>>
>>> I'd like to listen your opinion about this.
>>>
>>> Thanks,
>>> Jonghwa
>>>
>>>> +     }
>>>>
>>>>       exynos_report_trigger(data->reg_conf);
>>>>       mutex_lock(&data->lock);
>>>> @@ -358,7 +390,7 @@ static void exynos_tmu_work(struct work_struct *work)
>>>>
>>>>       clk_disable(data->clk);
>>>>       mutex_unlock(&data->lock);
>>>> -
>>>> +out:
>>>>       enable_irq(data->irq);
>>>>  }
>>>>
>>>> @@ -520,7 +552,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>>>>               return ret;
>>>>
>>>>       if (pdata->type == SOC_ARCH_EXYNOS ||
>>>> -                             pdata->type == SOC_ARCH_EXYNOS4210)
>>>> +             pdata->type == SOC_ARCH_EXYNOS4210 ||
>>>> +                             pdata->type == SOC_ARCH_EXYNOS5440)
>>>>               data->soc = pdata->type;
>>>>       else {
>>>>               ret = -EINVAL;
>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
>>>> index 65443d7..9151a30 100644
>>>> --- a/drivers/thermal/samsung/exynos_tmu.h
>>>> +++ b/drivers/thermal/samsung/exynos_tmu.h
>>>> @@ -44,6 +44,7 @@ enum trigger_type {
>>>>  enum soc_type {
>>>>       SOC_ARCH_EXYNOS4210 = 1,
>>>>       SOC_ARCH_EXYNOS,
>>>> +     SOC_ARCH_EXYNOS5440,
>>>>  };
>>>>
>>>>  /**
>>>> @@ -132,6 +133,8 @@ enum soc_type {
>>>>   * @emul_temp_shift: shift bits of emulation temperature.
>>>>   * @emul_time_shift: shift bits of emulation time.
>>>>   * @emul_time_mask: mask bits of emulation time.
>>>> + * @tmu_irqstatus: register to find which TMU generated interrupts.
>>>> + * @tmu_pmin: register to get/set the Pmin value.
>>>>   */
>>>>  struct exynos_tmu_registers {
>>>>       u32     triminfo_data;
>>>> @@ -199,6 +202,9 @@ struct exynos_tmu_registers {
>>>>       u32     emul_temp_shift;
>>>>       u32     emul_time_shift;
>>>>       u32     emul_time_mask;
>>>> +
>>>> +     u32     tmu_irqstatus;
>>>> +     u32     tmu_pmin;
>>>>  };
>>>>
>>>>  /**
>>>> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
>>>> index 0e2244f..4acf070 100644
>>>> --- a/drivers/thermal/samsung/exynos_tmu_data.h
>>>> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
>>>> @@ -91,6 +91,8 @@
>>>>  #define EXYNOS_EMUL_DATA_MASK        0xFF
>>>>  #define EXYNOS_EMUL_ENABLE   0x1
>>>>
>>>> +#define EXYNOS_MAX_TRIGGER_PER_REG   4
>>>> +
>>>>  #if defined(CONFIG_CPU_EXYNOS4210)
>>>>  extern struct exynos_tmu_platform_data const exynos4210_default_tmu_data;
>>>>  #define EXYNOS4210_TMU_DRV_DATA (&exynos4210_default_tmu_data)
>>>
>>>
>>
>>
>
>
> --
> You have got to be excited about what you are doing. (L. Lamport)
>
> Eduardo Valentin
>
--
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
Eduardo Valentin June 5, 2013, 12:53 p.m. UTC | #6
On 04-06-2013 23:20, amit daniel kachhap wrote:
> Hi Eduardo,
> 
> On Tue, Jun 4, 2013 at 6:25 PM, Eduardo Valentin
> <eduardo.valentin@ti.com> wrote:
>> On 04-06-2013 00:44, amit daniel kachhap wrote:
>>> Hi Jonghwa,
>>>
>>> Sorry for the late reply as I was on leave.
>>>
>>> On Sat, May 18, 2013 at 10:53 AM,  <jonghwa3.lee@samsung.com> wrote:
>>>> On 2013? 05? 14? 18:58, Amit Daniel Kachhap wrote:
>>>>
>>>>> This patch modifies TMU controller to add changes needed to work with
>>>>> exynos5440 platform. This sensor registers 3 instance of the tmu controller
>>>>> with the thermal zone and hence reports 3 temperature output. This controller
>>>>> supports upto five trip points. For critical threshold the driver uses the
>>>>> core driver thermal framework for shutdown.
>>>>>
>>>>> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
>>>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>>> ---
>>>>>  .../devicetree/bindings/thermal/exynos-thermal.txt |   28 ++++++++++++-
>>>>>  drivers/thermal/samsung/exynos_tmu.c               |   43 +++++++++++++++++--
>>>>>  drivers/thermal/samsung/exynos_tmu.h               |    6 +++
>>>>>  drivers/thermal/samsung/exynos_tmu_data.h          |    2 +
>>>>>  4 files changed, 72 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>>> index 535fd0e..970eeba 100644
>>>>> --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>>> +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>>> @@ -6,13 +6,16 @@
>>>>>              "samsung,exynos4412-tmu"
>>>>>              "samsung,exynos4210-tmu"
>>>>>              "samsung,exynos5250-tmu"
>>>>> +            "samsung,exynos5440-tmu"
>>>>>  - interrupt-parent : The phandle for the interrupt controller
>>>>> -- reg : Address range of the thermal registers
>>>>> +- reg : Address range of the thermal registers. For exynos5440-tmu which has 3
>>>>> +     instances of TMU, 2 set of register has to supplied. First set belongs
>>>>> +     to each instance of TMU and second set belongs to common TMU registers.
>>>>>  - interrupts : Should contain interrupt for thermal system
>>>>>  - clocks : The main clock for TMU device
>>>>>  - clock-names : Thermal system clock name
>>>>>
>>>>> -Example:
>>>>> +Example 1):
>>>>>
>>>>>       tmu@100C0000 {
>>>>>               compatible = "samsung,exynos4412-tmu";
>>>>> @@ -23,3 +26,24 @@ Example:
>>>>>               clock-names = "tmu_apbif";
>>>>>               status = "disabled";
>>>>>       };
>>>>> +
>>>>> +Example 2):
>>>>> +
>>>>> +     tmuctrl_0: tmuctrl@160118 {
>>>>> +             compatible = "samsung,exynos5440-tmu";
>>>>> +             reg = <0x160118 0x230>, <0x160368 0x10>;
>>>>> +             interrupts = <0 58 0>;
>>>>> +             clocks = <&clock 21>;
>>>>> +             clock-names = "tmu_apbif";
>>>>> +     };
>>>>> +
>>>>> +Note: For multi-instance tmu each instance should have an alias correctly
>>>>> +numbered in "aliases" node.
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +aliases {
>>>>> +     tmuctrl0 = &tmuctrl_0;
>>>>> +     tmuctrl1 = &tmuctrl_1;
>>>>> +     tmuctrl2 = &tmuctrl_2;
>>>>> +};
>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>>>> index 7f7b1cf..7ca9c4d 100644
>>>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>>>> @@ -185,9 +185,11 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>>                       reg->threshold_th0 + i * sizeof(reg->threshold_th0));
>>>>>
>>>>>               writel(reg->inten_rise_mask, data->base + reg->tmu_intclear);
>>>>> -     } else if (data->soc == SOC_ARCH_EXYNOS) {
>>>>> +     } else if (data->soc == SOC_ARCH_EXYNOS ||
>>>>> +                     data->soc == SOC_ARCH_EXYNOS5440) {
>>>>>               /* Write temperature code for rising and falling threshold */
>>>>> -             for (i = 0; i < trigger_levs; i++) {
>>>>> +             for (i = 0;
>>>>> +             i < trigger_levs && i < EXYNOS_MAX_TRIGGER_PER_REG; i++) {
>>>>>                       threshold_code = temp_to_code(data,
>>>>>                                               pdata->trigger_levels[i]);
>>>>>                       if (threshold_code < 0) {
>>>>> @@ -218,7 +220,30 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>>               writel((reg->inten_rise_mask << reg->inten_rise_shift) |
>>>>>                       (reg->inten_fall_mask << reg->inten_fall_shift),
>>>>>                               data->base + reg->tmu_intclear);
>>>>> +
>>>>> +             /* if 5th threshold limit is also present, use TH2 register */
>>>>> +             i = EXYNOS_MAX_TRIGGER_PER_REG;
>>>>> +             if (pdata->trigger_levels[i]) {
>>>>> +                     threshold_code = temp_to_code(data,
>>>>> +                                             pdata->trigger_levels[i]);
>>>>> +                     if (threshold_code < 0) {
>>>>> +                             ret = threshold_code;
>>>>> +                             goto out;
>>>>> +                     }
>>>>> +                     rising_threshold =
>>>>> +                             threshold_code << reg->threshold_th3_l0_shift;
>>>>> +                     writel(rising_threshold,
>>>>> +                             data->base + reg->threshold_th2);
>>>>> +                     if (pdata->trigger_type[i] == HW_TRIP) {
>>>>> +                             con = readl(data->base + reg->tmu_ctrl);
>>>>> +                             con |= (1 << reg->therm_trip_en_shift);
>>>>> +                             writel(con, data->base + reg->tmu_ctrl);
>>>>> +                     }
>>>>> +             }
>>>>>       }
>>>>> +     /*Clear the PMIN in the common TMU register*/
>>>>> +     if (reg->tmu_pmin && !data->id)
>>>>> +             writel(0, data->base_common + reg->tmu_pmin);
>>>>>  out:
>>>>>       clk_disable(data->clk);
>>>>>       mutex_unlock(&data->lock);
>>>>> @@ -345,7 +370,14 @@ static void exynos_tmu_work(struct work_struct *work)
>>>>>                       struct exynos_tmu_data, irq_work);
>>>>>       struct exynos_tmu_platform_data *pdata = data->pdata;
>>>>>       const struct exynos_tmu_registers *reg = pdata->registers;
>>>>> -     unsigned int val_irq;
>>>>> +     unsigned int val_irq, val_type;
>>>>> +
>>>>> +     /* Find which sensor generated this interrupt */
>>>>> +     if (reg->tmu_irqstatus) {
>>>>> +             val_type = readl(data->base_common + reg->tmu_irqstatus);
>>>>> +             if (!((val_type >> data->id) & 0x1))
>>>>> +                     goto out;
>>>>
>>>>
>>>> I have a question about your implementation for supporting EXYNOS5440.
>>>> I don't know exactly how EXYNO5440's tmu is working, but just guess it would be
>>>> similar with other EXYNOS series's without number of thermal sensors. (exclusive
>>>> register map and threshold level). Due to the multiple number of thermal sensor
>>>> in EXYNOS5440, it have multiple thermal zone devices and that's why it just
>>>> leave interrupt pin in pending if interrupt is not its, right?
>>> Yes in 5440 the interrupt line is shared so pending bit is left uncleared.
>>>>
>>>> So, my curious is, why we make all platform devices for each of thermal zone
>>>> devices? Why don't you just handle all thermal zone devices with one platform
>>>> device?
>>> Your doubt is genuine. Let me justify my design decision.
>>> Initially I also thought of making a single platform device but since
>>> there are 3 different TMU controllers and register maps for 4 more so
>>> I followed this design as the driver looks clean and can be scalable
>>> easily. Also I agree that some resources like IRQ line is shared but
>>> it is due to h/w limitation.
>>> Also it is easy to cleanly control each TMU instance with the device
>>> tree data similar to I2C/SPI/MMC instance based device driver. Say I
>>> do not want to use 2nd sensor then just pass device tree data for 1st
>>> and 3rd sensor.
>>>>
>>>> Yes, It's probably right to make multiple devices node to support them, because
>>>> it has different physical hardware(sensors). But we have one TMU , don't we?
>>>> (Maybe my assumption is wrong, I assume that it has one TMU because it looks
>>>> like it has only one irq line.). If I'm right, I think it is better to manage
>>>> all thermal zone devices with one platform device. Then, we don't need to leave
>>>> irq handler with leaving it pendded like above and also we may not need other
>>>> your patches like adding base_common iomem variable.
>>> Agreed that base_common variables is extra and present to handle the
>>> common part. I will further analyse your suggestion.
>>>
>>
>> What is the relation TMU <--> temperature sensor? Is it one to one or
>> one to many?
> 1 TMU --- > 1 temp sensor.(one to one)
> 


OK. Then it is different to TI bandgap, which has one to many relation.
Does every TMU has its own resources, like a register map and IRQ?

> Thanks,
> Amit Daniel
>>
>>> Thanks,
>>> Amit Daniel
>>>>
>>>> I'd like to listen your opinion about this.
>>>>
>>>> Thanks,
>>>> Jonghwa
>>>>
>>>>> +     }
>>>>>
>>>>>       exynos_report_trigger(data->reg_conf);
>>>>>       mutex_lock(&data->lock);
>>>>> @@ -358,7 +390,7 @@ static void exynos_tmu_work(struct work_struct *work)
>>>>>
>>>>>       clk_disable(data->clk);
>>>>>       mutex_unlock(&data->lock);
>>>>> -
>>>>> +out:
>>>>>       enable_irq(data->irq);
>>>>>  }
>>>>>
>>>>> @@ -520,7 +552,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>>>>>               return ret;
>>>>>
>>>>>       if (pdata->type == SOC_ARCH_EXYNOS ||
>>>>> -                             pdata->type == SOC_ARCH_EXYNOS4210)
>>>>> +             pdata->type == SOC_ARCH_EXYNOS4210 ||
>>>>> +                             pdata->type == SOC_ARCH_EXYNOS5440)
>>>>>               data->soc = pdata->type;
>>>>>       else {
>>>>>               ret = -EINVAL;
>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
>>>>> index 65443d7..9151a30 100644
>>>>> --- a/drivers/thermal/samsung/exynos_tmu.h
>>>>> +++ b/drivers/thermal/samsung/exynos_tmu.h
>>>>> @@ -44,6 +44,7 @@ enum trigger_type {
>>>>>  enum soc_type {
>>>>>       SOC_ARCH_EXYNOS4210 = 1,
>>>>>       SOC_ARCH_EXYNOS,
>>>>> +     SOC_ARCH_EXYNOS5440,
>>>>>  };
>>>>>
>>>>>  /**
>>>>> @@ -132,6 +133,8 @@ enum soc_type {
>>>>>   * @emul_temp_shift: shift bits of emulation temperature.
>>>>>   * @emul_time_shift: shift bits of emulation time.
>>>>>   * @emul_time_mask: mask bits of emulation time.
>>>>> + * @tmu_irqstatus: register to find which TMU generated interrupts.
>>>>> + * @tmu_pmin: register to get/set the Pmin value.
>>>>>   */
>>>>>  struct exynos_tmu_registers {
>>>>>       u32     triminfo_data;
>>>>> @@ -199,6 +202,9 @@ struct exynos_tmu_registers {
>>>>>       u32     emul_temp_shift;
>>>>>       u32     emul_time_shift;
>>>>>       u32     emul_time_mask;
>>>>> +
>>>>> +     u32     tmu_irqstatus;
>>>>> +     u32     tmu_pmin;
>>>>>  };
>>>>>
>>>>>  /**
>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
>>>>> index 0e2244f..4acf070 100644
>>>>> --- a/drivers/thermal/samsung/exynos_tmu_data.h
>>>>> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
>>>>> @@ -91,6 +91,8 @@
>>>>>  #define EXYNOS_EMUL_DATA_MASK        0xFF
>>>>>  #define EXYNOS_EMUL_ENABLE   0x1
>>>>>
>>>>> +#define EXYNOS_MAX_TRIGGER_PER_REG   4
>>>>> +
>>>>>  #if defined(CONFIG_CPU_EXYNOS4210)
>>>>>  extern struct exynos_tmu_platform_data const exynos4210_default_tmu_data;
>>>>>  #define EXYNOS4210_TMU_DRV_DATA (&exynos4210_default_tmu_data)
>>>>
>>>>
>>>
>>>
>>
>>
>> --
>> You have got to be excited about what you are doing. (L. Lamport)
>>
>> Eduardo Valentin
>>
> 
>
Amit Kachhap June 6, 2013, 6:19 a.m. UTC | #7
Hi,
On Wed, Jun 5, 2013 at 6:23 PM, Eduardo Valentin
<eduardo.valentin@ti.com> wrote:
> On 04-06-2013 23:20, amit daniel kachhap wrote:
>> Hi Eduardo,
>>
>> On Tue, Jun 4, 2013 at 6:25 PM, Eduardo Valentin
>> <eduardo.valentin@ti.com> wrote:
>>> On 04-06-2013 00:44, amit daniel kachhap wrote:
>>>> Hi Jonghwa,
>>>>
>>>> Sorry for the late reply as I was on leave.
>>>>
>>>> On Sat, May 18, 2013 at 10:53 AM,  <jonghwa3.lee@samsung.com> wrote:
>>>>> On 2013? 05? 14? 18:58, Amit Daniel Kachhap wrote:
>>>>>
>>>>>> This patch modifies TMU controller to add changes needed to work with
>>>>>> exynos5440 platform. This sensor registers 3 instance of the tmu controller
>>>>>> with the thermal zone and hence reports 3 temperature output. This controller
>>>>>> supports upto five trip points. For critical threshold the driver uses the
>>>>>> core driver thermal framework for shutdown.
>>>>>>
>>>>>> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
>>>>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/thermal/exynos-thermal.txt |   28 ++++++++++++-
>>>>>>  drivers/thermal/samsung/exynos_tmu.c               |   43 +++++++++++++++++--
>>>>>>  drivers/thermal/samsung/exynos_tmu.h               |    6 +++
>>>>>>  drivers/thermal/samsung/exynos_tmu_data.h          |    2 +
>>>>>>  4 files changed, 72 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>>>> index 535fd0e..970eeba 100644
>>>>>> --- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>>>> +++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
>>>>>> @@ -6,13 +6,16 @@
>>>>>>              "samsung,exynos4412-tmu"
>>>>>>              "samsung,exynos4210-tmu"
>>>>>>              "samsung,exynos5250-tmu"
>>>>>> +            "samsung,exynos5440-tmu"
>>>>>>  - interrupt-parent : The phandle for the interrupt controller
>>>>>> -- reg : Address range of the thermal registers
>>>>>> +- reg : Address range of the thermal registers. For exynos5440-tmu which has 3
>>>>>> +     instances of TMU, 2 set of register has to supplied. First set belongs
>>>>>> +     to each instance of TMU and second set belongs to common TMU registers.
>>>>>>  - interrupts : Should contain interrupt for thermal system
>>>>>>  - clocks : The main clock for TMU device
>>>>>>  - clock-names : Thermal system clock name
>>>>>>
>>>>>> -Example:
>>>>>> +Example 1):
>>>>>>
>>>>>>       tmu@100C0000 {
>>>>>>               compatible = "samsung,exynos4412-tmu";
>>>>>> @@ -23,3 +26,24 @@ Example:
>>>>>>               clock-names = "tmu_apbif";
>>>>>>               status = "disabled";
>>>>>>       };
>>>>>> +
>>>>>> +Example 2):
>>>>>> +
>>>>>> +     tmuctrl_0: tmuctrl@160118 {
>>>>>> +             compatible = "samsung,exynos5440-tmu";
>>>>>> +             reg = <0x160118 0x230>, <0x160368 0x10>;
>>>>>> +             interrupts = <0 58 0>;
>>>>>> +             clocks = <&clock 21>;
>>>>>> +             clock-names = "tmu_apbif";
>>>>>> +     };
>>>>>> +
>>>>>> +Note: For multi-instance tmu each instance should have an alias correctly
>>>>>> +numbered in "aliases" node.
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> +aliases {
>>>>>> +     tmuctrl0 = &tmuctrl_0;
>>>>>> +     tmuctrl1 = &tmuctrl_1;
>>>>>> +     tmuctrl2 = &tmuctrl_2;
>>>>>> +};
>>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>>>>> index 7f7b1cf..7ca9c4d 100644
>>>>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>>>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>>>>> @@ -185,9 +185,11 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>>>                       reg->threshold_th0 + i * sizeof(reg->threshold_th0));
>>>>>>
>>>>>>               writel(reg->inten_rise_mask, data->base + reg->tmu_intclear);
>>>>>> -     } else if (data->soc == SOC_ARCH_EXYNOS) {
>>>>>> +     } else if (data->soc == SOC_ARCH_EXYNOS ||
>>>>>> +                     data->soc == SOC_ARCH_EXYNOS5440) {
>>>>>>               /* Write temperature code for rising and falling threshold */
>>>>>> -             for (i = 0; i < trigger_levs; i++) {
>>>>>> +             for (i = 0;
>>>>>> +             i < trigger_levs && i < EXYNOS_MAX_TRIGGER_PER_REG; i++) {
>>>>>>                       threshold_code = temp_to_code(data,
>>>>>>                                               pdata->trigger_levels[i]);
>>>>>>                       if (threshold_code < 0) {
>>>>>> @@ -218,7 +220,30 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>>>               writel((reg->inten_rise_mask << reg->inten_rise_shift) |
>>>>>>                       (reg->inten_fall_mask << reg->inten_fall_shift),
>>>>>>                               data->base + reg->tmu_intclear);
>>>>>> +
>>>>>> +             /* if 5th threshold limit is also present, use TH2 register */
>>>>>> +             i = EXYNOS_MAX_TRIGGER_PER_REG;
>>>>>> +             if (pdata->trigger_levels[i]) {
>>>>>> +                     threshold_code = temp_to_code(data,
>>>>>> +                                             pdata->trigger_levels[i]);
>>>>>> +                     if (threshold_code < 0) {
>>>>>> +                             ret = threshold_code;
>>>>>> +                             goto out;
>>>>>> +                     }
>>>>>> +                     rising_threshold =
>>>>>> +                             threshold_code << reg->threshold_th3_l0_shift;
>>>>>> +                     writel(rising_threshold,
>>>>>> +                             data->base + reg->threshold_th2);
>>>>>> +                     if (pdata->trigger_type[i] == HW_TRIP) {
>>>>>> +                             con = readl(data->base + reg->tmu_ctrl);
>>>>>> +                             con |= (1 << reg->therm_trip_en_shift);
>>>>>> +                             writel(con, data->base + reg->tmu_ctrl);
>>>>>> +                     }
>>>>>> +             }
>>>>>>       }
>>>>>> +     /*Clear the PMIN in the common TMU register*/
>>>>>> +     if (reg->tmu_pmin && !data->id)
>>>>>> +             writel(0, data->base_common + reg->tmu_pmin);
>>>>>>  out:
>>>>>>       clk_disable(data->clk);
>>>>>>       mutex_unlock(&data->lock);
>>>>>> @@ -345,7 +370,14 @@ static void exynos_tmu_work(struct work_struct *work)
>>>>>>                       struct exynos_tmu_data, irq_work);
>>>>>>       struct exynos_tmu_platform_data *pdata = data->pdata;
>>>>>>       const struct exynos_tmu_registers *reg = pdata->registers;
>>>>>> -     unsigned int val_irq;
>>>>>> +     unsigned int val_irq, val_type;
>>>>>> +
>>>>>> +     /* Find which sensor generated this interrupt */
>>>>>> +     if (reg->tmu_irqstatus) {
>>>>>> +             val_type = readl(data->base_common + reg->tmu_irqstatus);
>>>>>> +             if (!((val_type >> data->id) & 0x1))
>>>>>> +                     goto out;
>>>>>
>>>>>
>>>>> I have a question about your implementation for supporting EXYNOS5440.
>>>>> I don't know exactly how EXYNO5440's tmu is working, but just guess it would be
>>>>> similar with other EXYNOS series's without number of thermal sensors. (exclusive
>>>>> register map and threshold level). Due to the multiple number of thermal sensor
>>>>> in EXYNOS5440, it have multiple thermal zone devices and that's why it just
>>>>> leave interrupt pin in pending if interrupt is not its, right?
>>>> Yes in 5440 the interrupt line is shared so pending bit is left uncleared.
>>>>>
>>>>> So, my curious is, why we make all platform devices for each of thermal zone
>>>>> devices? Why don't you just handle all thermal zone devices with one platform
>>>>> device?
>>>> Your doubt is genuine. Let me justify my design decision.
>>>> Initially I also thought of making a single platform device but since
>>>> there are 3 different TMU controllers and register maps for 4 more so
>>>> I followed this design as the driver looks clean and can be scalable
>>>> easily. Also I agree that some resources like IRQ line is shared but
>>>> it is due to h/w limitation.
>>>> Also it is easy to cleanly control each TMU instance with the device
>>>> tree data similar to I2C/SPI/MMC instance based device driver. Say I
>>>> do not want to use 2nd sensor then just pass device tree data for 1st
>>>> and 3rd sensor.
>>>>>
>>>>> Yes, It's probably right to make multiple devices node to support them, because
>>>>> it has different physical hardware(sensors). But we have one TMU , don't we?
>>>>> (Maybe my assumption is wrong, I assume that it has one TMU because it looks
>>>>> like it has only one irq line.). If I'm right, I think it is better to manage
>>>>> all thermal zone devices with one platform device. Then, we don't need to leave
>>>>> irq handler with leaving it pendded like above and also we may not need other
>>>>> your patches like adding base_common iomem variable.
>>>> Agreed that base_common variables is extra and present to handle the
>>>> common part. I will further analyse your suggestion.
>>>>
>>>
>>> What is the relation TMU <--> temperature sensor? Is it one to one or
>>> one to many?
>> 1 TMU --- > 1 temp sensor.(one to one)
>>
>
>
> OK. Then it is different to TI bandgap, which has one to many relation.
> Does every TMU has its own resources, like a register map and IRQ?
Yes register maps are different but some registers are common like ISR
related registers. IRQ line is common. However all TMU's are totally
independent devices.
>
>> Thanks,
>> Amit Daniel
>>>
>>>> Thanks,
>>>> Amit Daniel
>>>>>
>>>>> I'd like to listen your opinion about this.
>>>>>
>>>>> Thanks,
>>>>> Jonghwa
>>>>>
>>>>>> +     }
>>>>>>
>>>>>>       exynos_report_trigger(data->reg_conf);
>>>>>>       mutex_lock(&data->lock);
>>>>>> @@ -358,7 +390,7 @@ static void exynos_tmu_work(struct work_struct *work)
>>>>>>
>>>>>>       clk_disable(data->clk);
>>>>>>       mutex_unlock(&data->lock);
>>>>>> -
>>>>>> +out:
>>>>>>       enable_irq(data->irq);
>>>>>>  }
>>>>>>
>>>>>> @@ -520,7 +552,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>>>>>>               return ret;
>>>>>>
>>>>>>       if (pdata->type == SOC_ARCH_EXYNOS ||
>>>>>> -                             pdata->type == SOC_ARCH_EXYNOS4210)
>>>>>> +             pdata->type == SOC_ARCH_EXYNOS4210 ||
>>>>>> +                             pdata->type == SOC_ARCH_EXYNOS5440)
>>>>>>               data->soc = pdata->type;
>>>>>>       else {
>>>>>>               ret = -EINVAL;
>>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
>>>>>> index 65443d7..9151a30 100644
>>>>>> --- a/drivers/thermal/samsung/exynos_tmu.h
>>>>>> +++ b/drivers/thermal/samsung/exynos_tmu.h
>>>>>> @@ -44,6 +44,7 @@ enum trigger_type {
>>>>>>  enum soc_type {
>>>>>>       SOC_ARCH_EXYNOS4210 = 1,
>>>>>>       SOC_ARCH_EXYNOS,
>>>>>> +     SOC_ARCH_EXYNOS5440,
>>>>>>  };
>>>>>>
>>>>>>  /**
>>>>>> @@ -132,6 +133,8 @@ enum soc_type {
>>>>>>   * @emul_temp_shift: shift bits of emulation temperature.
>>>>>>   * @emul_time_shift: shift bits of emulation time.
>>>>>>   * @emul_time_mask: mask bits of emulation time.
>>>>>> + * @tmu_irqstatus: register to find which TMU generated interrupts.
>>>>>> + * @tmu_pmin: register to get/set the Pmin value.
>>>>>>   */
>>>>>>  struct exynos_tmu_registers {
>>>>>>       u32     triminfo_data;
>>>>>> @@ -199,6 +202,9 @@ struct exynos_tmu_registers {
>>>>>>       u32     emul_temp_shift;
>>>>>>       u32     emul_time_shift;
>>>>>>       u32     emul_time_mask;
>>>>>> +
>>>>>> +     u32     tmu_irqstatus;
>>>>>> +     u32     tmu_pmin;
>>>>>>  };
>>>>>>
>>>>>>  /**
>>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
>>>>>> index 0e2244f..4acf070 100644
>>>>>> --- a/drivers/thermal/samsung/exynos_tmu_data.h
>>>>>> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
>>>>>> @@ -91,6 +91,8 @@
>>>>>>  #define EXYNOS_EMUL_DATA_MASK        0xFF
>>>>>>  #define EXYNOS_EMUL_ENABLE   0x1
>>>>>>
>>>>>> +#define EXYNOS_MAX_TRIGGER_PER_REG   4
>>>>>> +
>>>>>>  #if defined(CONFIG_CPU_EXYNOS4210)
>>>>>>  extern struct exynos_tmu_platform_data const exynos4210_default_tmu_data;
>>>>>>  #define EXYNOS4210_TMU_DRV_DATA (&exynos4210_default_tmu_data)
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> You have got to be excited about what you are doing. (L. Lamport)
>>>
>>> Eduardo Valentin
>>>
>>
>>
>
>
> --
> You have got to be excited about what you are doing. (L. Lamport)
>
> Eduardo Valentin
>
--
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
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
index 535fd0e..970eeba 100644
--- a/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/exynos-thermal.txt
@@ -6,13 +6,16 @@ 
 	       "samsung,exynos4412-tmu"
 	       "samsung,exynos4210-tmu"
 	       "samsung,exynos5250-tmu"
+	       "samsung,exynos5440-tmu"
 - interrupt-parent : The phandle for the interrupt controller
-- reg : Address range of the thermal registers
+- reg : Address range of the thermal registers. For exynos5440-tmu which has 3
+	instances of TMU, 2 set of register has to supplied. First set belongs
+	to each instance of TMU and second set belongs to common TMU registers.
 - interrupts : Should contain interrupt for thermal system
 - clocks : The main clock for TMU device
 - clock-names : Thermal system clock name
 
-Example:
+Example 1):
 
 	tmu@100C0000 {
 		compatible = "samsung,exynos4412-tmu";
@@ -23,3 +26,24 @@  Example:
 		clock-names = "tmu_apbif";
 		status = "disabled";
 	};
+
+Example 2):
+
+	tmuctrl_0: tmuctrl@160118 {
+		compatible = "samsung,exynos5440-tmu";
+		reg = <0x160118 0x230>, <0x160368 0x10>;
+		interrupts = <0 58 0>;
+		clocks = <&clock 21>;
+		clock-names = "tmu_apbif";
+	};
+
+Note: For multi-instance tmu each instance should have an alias correctly
+numbered in "aliases" node.
+
+Example:
+
+aliases {
+	tmuctrl0 = &tmuctrl_0;
+	tmuctrl1 = &tmuctrl_1;
+	tmuctrl2 = &tmuctrl_2;
+};
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 7f7b1cf..7ca9c4d 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -185,9 +185,11 @@  static int exynos_tmu_initialize(struct platform_device *pdev)
 			reg->threshold_th0 + i * sizeof(reg->threshold_th0));
 
 		writel(reg->inten_rise_mask, data->base + reg->tmu_intclear);
-	} else if (data->soc == SOC_ARCH_EXYNOS) {
+	} else if (data->soc == SOC_ARCH_EXYNOS ||
+			data->soc == SOC_ARCH_EXYNOS5440) {
 		/* Write temperature code for rising and falling threshold */
-		for (i = 0; i < trigger_levs; i++) {
+		for (i = 0;
+		i < trigger_levs && i < EXYNOS_MAX_TRIGGER_PER_REG; i++) {
 			threshold_code = temp_to_code(data,
 						pdata->trigger_levels[i]);
 			if (threshold_code < 0) {
@@ -218,7 +220,30 @@  static int exynos_tmu_initialize(struct platform_device *pdev)
 		writel((reg->inten_rise_mask << reg->inten_rise_shift) |
 			(reg->inten_fall_mask << reg->inten_fall_shift),
 				data->base + reg->tmu_intclear);
+
+		/* if 5th threshold limit is also present, use TH2 register */
+		i = EXYNOS_MAX_TRIGGER_PER_REG;
+		if (pdata->trigger_levels[i]) {
+			threshold_code = temp_to_code(data,
+						pdata->trigger_levels[i]);
+			if (threshold_code < 0) {
+				ret = threshold_code;
+				goto out;
+			}
+			rising_threshold =
+				threshold_code << reg->threshold_th3_l0_shift;
+			writel(rising_threshold,
+				data->base + reg->threshold_th2);
+			if (pdata->trigger_type[i] == HW_TRIP) {
+				con = readl(data->base + reg->tmu_ctrl);
+				con |= (1 << reg->therm_trip_en_shift);
+				writel(con, data->base + reg->tmu_ctrl);
+			}
+		}
 	}
+	/*Clear the PMIN in the common TMU register*/
+	if (reg->tmu_pmin && !data->id)
+		writel(0, data->base_common + reg->tmu_pmin);
 out:
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
@@ -345,7 +370,14 @@  static void exynos_tmu_work(struct work_struct *work)
 			struct exynos_tmu_data, irq_work);
 	struct exynos_tmu_platform_data *pdata = data->pdata;
 	const struct exynos_tmu_registers *reg = pdata->registers;
-	unsigned int val_irq;
+	unsigned int val_irq, val_type;
+
+	/* Find which sensor generated this interrupt */
+	if (reg->tmu_irqstatus) {
+		val_type = readl(data->base_common + reg->tmu_irqstatus);
+		if (!((val_type >> data->id) & 0x1))
+			goto out;
+	}
 
 	exynos_report_trigger(data->reg_conf);
 	mutex_lock(&data->lock);
@@ -358,7 +390,7 @@  static void exynos_tmu_work(struct work_struct *work)
 
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
-
+out:
 	enable_irq(data->irq);
 }
 
@@ -520,7 +552,8 @@  static int exynos_tmu_probe(struct platform_device *pdev)
 		return ret;
 
 	if (pdata->type == SOC_ARCH_EXYNOS ||
-				pdata->type == SOC_ARCH_EXYNOS4210)
+		pdata->type == SOC_ARCH_EXYNOS4210 ||
+				pdata->type == SOC_ARCH_EXYNOS5440)
 		data->soc = pdata->type;
 	else {
 		ret = -EINVAL;
diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
index 65443d7..9151a30 100644
--- a/drivers/thermal/samsung/exynos_tmu.h
+++ b/drivers/thermal/samsung/exynos_tmu.h
@@ -44,6 +44,7 @@  enum trigger_type {
 enum soc_type {
 	SOC_ARCH_EXYNOS4210 = 1,
 	SOC_ARCH_EXYNOS,
+	SOC_ARCH_EXYNOS5440,
 };
 
 /**
@@ -132,6 +133,8 @@  enum soc_type {
  * @emul_temp_shift: shift bits of emulation temperature.
  * @emul_time_shift: shift bits of emulation time.
  * @emul_time_mask: mask bits of emulation time.
+ * @tmu_irqstatus: register to find which TMU generated interrupts.
+ * @tmu_pmin: register to get/set the Pmin value.
  */
 struct exynos_tmu_registers {
 	u32	triminfo_data;
@@ -199,6 +202,9 @@  struct exynos_tmu_registers {
 	u32	emul_temp_shift;
 	u32	emul_time_shift;
 	u32	emul_time_mask;
+
+	u32	tmu_irqstatus;
+	u32	tmu_pmin;
 };
 
 /**
diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
index 0e2244f..4acf070 100644
--- a/drivers/thermal/samsung/exynos_tmu_data.h
+++ b/drivers/thermal/samsung/exynos_tmu_data.h
@@ -91,6 +91,8 @@ 
 #define EXYNOS_EMUL_DATA_MASK	0xFF
 #define EXYNOS_EMUL_ENABLE	0x1
 
+#define EXYNOS_MAX_TRIGGER_PER_REG	4
+
 #if defined(CONFIG_CPU_EXYNOS4210)
 extern struct exynos_tmu_platform_data const exynos4210_default_tmu_data;
 #define EXYNOS4210_TMU_DRV_DATA (&exynos4210_default_tmu_data)