Re: [PATCH 6/6] thermal: Add Tegra SOCTHERM thermal management driver
On 04/07/14 11:43, Wei Ni wrote:
On 06/27/2014 04:11 PM, Mikko Perttunen wrote:
This adds support for the Tegra SOCTHERM thermal sensing and management
system found in the Tegra124 system-on-chip. This initial driver supports
the four thermal zones with hardware-tracked trip points.
diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c
+static int calculate_shared_calibration(struct tsensor_shared_calibration *r)
+{
+ u32 val;
+ u32 shifted_cp, shifted_ft;
+ int err;
+
+ err = tegra_fuse_readl(FUSE_TSENSOR8_CALIB, &val);
+ if (err)
+ return err;
+ r->base_cp = val & 0x3ff;
+ r->base_ft = (val & (0x7ff << 10)) >> 10;
+
+ err = tegra_fuse_readl(FUSE_SPARE_REALIGNMENT_REG_0, &val);
+ if (err)
+ return err;
+ /* Sign extend from 6 bits to 32 bits */
+ shifted_cp = (s32)((val & 0x1f) | ((val & 0x20) ? 0xffe0 : 0x0));
+ val = ((val & (0x1f << 21)) >> 21);
+ /* Sign extend from 5 bits to 32 bits */
+ shifted_ft = (s32)((val & 0xf) | ((val & 0x10) ? 0xfff0 : 0x0));
+
+ r->actual_temp_cp = 2 * 25 + shifted_cp;
Do we need to define the "25" as constant, just like below line.
I believe downstream just uses "25" which is the reason I used it, but
yes, a constant would be nicer.
+ r->actual_temp_ft = 2 * NOMINAL_CALIB_FT_T124 + shifted_ft;
+
+ return 0;
+}
+
+static irqreturn_t soctherm_isr(int irq, void *dev_id)
+{
+ struct tegra_thermctl_zone *zone = dev_id;
+ u32 val;
+ u32 intr_mask = 0x03 << t124_thermctl_shifts[zone->sensor];
+
+ val = readl(zone->tegra->regs + THERMCTL_INTR_STATUS);
+
+ if ((val & intr_mask) == 0)
+ return IRQ_NONE;
+
+ writel(val & intr_mask, zone->tegra->regs + THERMCTL_INTR_STATUS);
I think we need to disable the interrupt here, and enable it again after
updating the high/low limited values. If not, there will trigger mass of
interrupts.
Good point. It works now because of-thermal will set up the new trip
points during soctherm_thread_isr, and apparently the interrupt is kept
disabled until after that.
+
+static struct platform_driver tegra_soctherm_driver = {
+ .probe = tegra_soctherm_probe,
+ .remove = tegra_soctherm_remove,
+ .driver = {
+ .name = "tegra_soctherm",
+ .owner = THIS_MODULE,
+ .of_match_table = tegra_soctherm_of_match,
+ },
+};
Will you consider to add suspend/resume support?
I guess for this one it should be simple; same driver probe and teardown
as normally. Although currently suspend doesn't support LP0 so no-op is
enough.
Thanks.
Wei.
Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6] thermal: Add Tegra SOCTHERM thermal management driver
On 06/27/2014 04:11 PM, Mikko Perttunen wrote:
> This adds support for the Tegra SOCTHERM thermal sensing and management
> system found in the Tegra124 system-on-chip. This initial driver supports
> the four thermal zones with hardware-tracked trip points.
> diff --git a/drivers/thermal/tegra_soctherm.c
> b/drivers/thermal/tegra_soctherm.c
> +static int calculate_shared_calibration(struct tsensor_shared_calibration *r)
> +{
> + u32 val;
> + u32 shifted_cp, shifted_ft;
> + int err;
> +
> + err = tegra_fuse_readl(FUSE_TSENSOR8_CALIB, &val);
> + if (err)
> + return err;
> + r->base_cp = val & 0x3ff;
> + r->base_ft = (val & (0x7ff << 10)) >> 10;
> +
> + err = tegra_fuse_readl(FUSE_SPARE_REALIGNMENT_REG_0, &val);
> + if (err)
> + return err;
> + /* Sign extend from 6 bits to 32 bits */
> + shifted_cp = (s32)((val & 0x1f) | ((val & 0x20) ? 0xffe0 : 0x0));
> + val = ((val & (0x1f << 21)) >> 21);
> + /* Sign extend from 5 bits to 32 bits */
> + shifted_ft = (s32)((val & 0xf) | ((val & 0x10) ? 0xfff0 : 0x0));
> +
> + r->actual_temp_cp = 2 * 25 + shifted_cp;
Do we need to define the "25" as constant, just like below line.
> + r->actual_temp_ft = 2 * NOMINAL_CALIB_FT_T124 + shifted_ft;
> +
> + return 0;
> +}
> +
> +static irqreturn_t soctherm_isr(int irq, void *dev_id)
> +{
> + struct tegra_thermctl_zone *zone = dev_id;
> + u32 val;
> + u32 intr_mask = 0x03 << t124_thermctl_shifts[zone->sensor];
> +
> + val = readl(zone->tegra->regs + THERMCTL_INTR_STATUS);
> +
> + if ((val & intr_mask) == 0)
> + return IRQ_NONE;
> +
> + writel(val & intr_mask, zone->tegra->regs + THERMCTL_INTR_STATUS);
I think we need to disable the interrupt here, and enable it again after
updating the high/low limited values. If not, there will trigger mass of
interrupts.
> +
> +static struct platform_driver tegra_soctherm_driver = {
> + .probe = tegra_soctherm_probe,
> + .remove = tegra_soctherm_remove,
> + .driver = {
> + .name = "tegra_soctherm",
> + .owner = THIS_MODULE,
> + .of_match_table = tegra_soctherm_of_match,
> + },
> +};
Will you consider to add suspend/resume support?
Thanks.
Wei.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6] thermal: Add Tegra SOCTHERM thermal management driver
On 01/07/14 21:26, Stephen Warren wrote: Ah, so there's some manufacturing calibration process that sets some fuse value, and the HW uses a combination of that fuse value, and some parameters of the manufacturing process as represented by the SENSOR_CONFIG2 register, to apply the calibration? I wonder why SENSOR_CONFIG2 is a register not a fuse in that case, but anyway... Perhaps some comments or kerneldoc in the definition of struct tegra_tsensor would be useful? Yes, I'll add some comments. Why not read THERMCTL_INTR_STATUS inside the IRQ thread. IIRC, if the ISR wakes an IRQ thread, the interrupt remains disable until the thread has run its course, so there's no issue deferring the register read until the thread runs, at which point, the thread can simply loop over all the sensors. If that's the case, then that's definitely a better way to do it. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6] thermal: Add Tegra SOCTHERM thermal management driver
On 27/06/14 11:11, Mikko Perttunen wrote: + /* Sign extend from 6 bits to 32 bits */ + shifted_cp = (s32)((val & 0x1f) | ((val & 0x20) ? 0xffe0 : 0x0)); + val = ((val & (0x1f << 21)) >> 21); + /* Sign extend from 5 bits to 32 bits */ + shifted_ft = (s32)((val & 0xf) | ((val & 0x10) ? 0xfff0 : 0x0)); There's sign_extend32 in bitops.h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6] thermal: Add Tegra SOCTHERM thermal management driver
On 07/01/2014 02:06 AM, Mikko Perttunen wrote:
> Inline.
>
> On 01/07/14 00:23, Stephen Warren wrote:
>> On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
>>> This adds support for the Tegra SOCTHERM thermal sensing and management
>>> system found in the Tegra124 system-on-chip. This initial driver
>>> supports
>>> the four thermal zones with hardware-tracked trip points.
>>
>>> diff --git a/drivers/thermal/tegra_soctherm.c
>>> b/drivers/thermal/tegra_soctherm.c
>>
>>> +static struct tegra_tsensor t124_tsensors[] = {
>>> +{
>>> +.base = 0xc0,
>>> +.name = "cpu0",
>>> +.config = &t124_tsensor_config,
>>> +.calib_fuse_offset = 0x098,
>>> +.fuse_corr_alpha = 1135400,
>>> +.fuse_corr_beta = -6266900,
>>> +},
>>
>> I wonder why some of those fields are named "fuse_xxx" when the values
>> are hard-coded in these tables rather than read from fuses? These values
>> don't seem to be used to adjust values read from fuses.
>
> They are used to when calculating the thermal calibration in
> calculate_tsensor_calibration, which is based on the value read from the
> fuse. Downstream calls them fuse correction values, so I kept that. (I
> guess the meaning of corr might not be obvious..) On downstream there is
> another set of these correction values used depending on the fuse
> revision, but I believe the older revision is only found internally.
Ah, so there's some manufacturing calibration process that sets some
fuse value, and the HW uses a combination of that fuse value, and some
parameters of the manufacturing process as represented by the
SENSOR_CONFIG2 register, to apply the calibration? I wonder why
SENSOR_CONFIG2 is a register not a fuse in that case, but anyway...
Perhaps some comments or kerneldoc in the definition of struct
tegra_tsensor would be useful?
I did notice some inconsistency in bracketing at:
> + *calib = ((u16)(therma) << SENSOR_CONFIG2_THERMA_SHIFT) |
> + ((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);
>>> +err = devm_request_threaded_irq(&pdev->dev, irq, soctherm_isr,
>>> +soctherm_isr_thread,
>>> +IRQF_SHARED, "tegra_soctherm",
>>> +zone);
>>
>> Why request the same IRQ 4 times here. Rather, shouldn't the IRQ be
>> requested once, and the ISR simply loop over the status register (or
>> whatever there are 4 of)?
>
> I had that variant as well, but since we need to pass the list of
> tripped sensors to soctherm_isr_thread somehow, I guess some kind of
> locking or atomic is needed. This version doesn't need that, so I went
> with it.
Why not read THERMCTL_INTR_STATUS inside the IRQ thread. IIRC, if the
ISR wakes an IRQ thread, the interrupt remains disable until the thread
has run its course, so there's no issue deferring the register read
until the thread runs, at which point, the thread can simply loop over
all the sensors.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6] thermal: Add Tegra SOCTHERM thermal management driver
Inline.
On 01/07/14 00:23, Stephen Warren wrote:
On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
This adds support for the Tegra SOCTHERM thermal sensing and management
system found in the Tegra124 system-on-chip. This initial driver supports
the four thermal zones with hardware-tracked trip points.
diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c
+static struct tegra_tsensor t124_tsensors[] = {
+ {
+ .base = 0xc0,
+ .name = "cpu0",
+ .config = &t124_tsensor_config,
+ .calib_fuse_offset = 0x098,
+ .fuse_corr_alpha = 1135400,
+ .fuse_corr_beta = -6266900,
+ },
I wonder why some of those fields are named "fuse_xxx" when the values
are hard-coded in these tables rather than read from fuses? These values
don't seem to be used to adjust values read from fuses.
They are used to when calculating the thermal calibration in
calculate_tsensor_calibration, which is based on the value read from the
fuse. Downstream calls them fuse correction values, so I kept that. (I
guess the meaning of corr might not be obvious..) On downstream there is
another set of these correction values used depending on the fuse
revision, but I believe the older revision is only found internally.
+static int tegra_thermctl_get_temp(void *data, long *out_temp)
+ switch (zone->sensor) {
+ case 0:
+ val = readl(zone->tegra->regs + SENSOR_TEMP1)
+ >> SENSOR_TEMP1_CPU_TEMP_SHIFT;
Can't the register offset and shift be stored in *zone, so that this
whole switch can be replaced with something generic:
val = readl(zone->tegra->regs + zone->reg_offset) >> zone->value_shift;
Yes, certainly doable.
+static int tegra_soctherm_probe(struct platform_device *pdev)
+ irq = platform_get_irq(pdev, 0);
+ if (irq <= 0) {
+ dev_err(&pdev->dev, "can't get interrupt\n");
+ return -EINVAL;
+ }
irq is assigned once here ... (see later)
+ for (i = 0; i < 4; ++i) {
Why "4"? Should the loop count be the ARRAY_SIZE(some array)? At the
very least, a named constant that describes the value would be useful...
The thermctl sensors have been unchanged for a few chip generations, so
I was thinking that just hardcoding this wouldn't be so bad. But I guess
an array would look nicer here. Will fix.
+ err = devm_request_threaded_irq(&pdev->dev, irq, soctherm_isr,
+ soctherm_isr_thread,
+ IRQF_SHARED, "tegra_soctherm",
+ zone);
Why request the same IRQ 4 times here. Rather, shouldn't the IRQ be
requested once, and the ISR simply loop over the status register (or
whatever there are 4 of)?
I had that variant as well, but since we need to pass the list of
tripped sensors to soctherm_isr_thread somehow, I guess some kind of
locking or atomic is needed. This version doesn't need that, so I went
with it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6] thermal: Add Tegra SOCTHERM thermal management driver
On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
> This adds support for the Tegra SOCTHERM thermal sensing and management
> system found in the Tegra124 system-on-chip. This initial driver supports
> the four thermal zones with hardware-tracked trip points.
> diff --git a/drivers/thermal/tegra_soctherm.c
> b/drivers/thermal/tegra_soctherm.c
> +static struct tegra_tsensor t124_tsensors[] = {
> + {
> + .base = 0xc0,
> + .name = "cpu0",
> + .config = &t124_tsensor_config,
> + .calib_fuse_offset = 0x098,
> + .fuse_corr_alpha = 1135400,
> + .fuse_corr_beta = -6266900,
> + },
I wonder why some of those fields are named "fuse_xxx" when the values
are hard-coded in these tables rather than read from fuses? These values
don't seem to be used to adjust values read from fuses.
> +static int tegra_thermctl_get_temp(void *data, long *out_temp)
> + switch (zone->sensor) {
> + case 0:
> + val = readl(zone->tegra->regs + SENSOR_TEMP1)
> + >> SENSOR_TEMP1_CPU_TEMP_SHIFT;
Can't the register offset and shift be stored in *zone, so that this
whole switch can be replaced with something generic:
val = readl(zone->tegra->regs + zone->reg_offset) >> zone->value_shift;
> +static int tegra_soctherm_probe(struct platform_device *pdev)
> + irq = platform_get_irq(pdev, 0);
> + if (irq <= 0) {
> + dev_err(&pdev->dev, "can't get interrupt\n");
> + return -EINVAL;
> + }
irq is assigned once here ... (see later)
> + for (i = 0; i < 4; ++i) {
Why "4"? Should the loop count be the ARRAY_SIZE(some array)? At the
very least, a named constant that describes the value would be useful...
> + err = devm_request_threaded_irq(&pdev->dev, irq, soctherm_isr,
> + soctherm_isr_thread,
> + IRQF_SHARED, "tegra_soctherm",
> + zone);
Why request the same IRQ 4 times here. Rather, shouldn't the IRQ be
requested once, and the ISR simply loop over the status register (or
whatever there are 4 of)?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
