Re: [PATCH 1/3] thermal: bcm281xx: Add thermal driver

From: Eduardo Valentin
Date: Mon Oct 07 2013 - 12:12:48 EST

Next message: Steven Rostedt: "Re: [PATCHv5 11/11] x86, mm: enable split page table lock for PMDlevel"
Previous message: Stephane Eranian: "[PATCH v1 1/2] perf,x86: add Intel RAPL PMU support"
In reply to: Eduardo Valentin: "Re: [PATCH 1/3] thermal: bcm281xx: Add thermal driver"
Next in thread: Wendy Ng: "Re: [PATCH 1/3] thermal: bcm281xx: Add thermal driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 07-10-2013 10:52, Eduardo Valentin wrote:
> On 04-10-2013 18:17, Wendy Ng wrote:
>> Hello Eduardo,
>>
>
> Hello Wendy,
>
>> I have rebased my work on the top of yours and tried out the new way of
>> registering the thermal zone. The new method is certainly making
>> things easier for me to create a new thermal zone device driver. But I
>> have some questions and I hope to hear back from you before I upload my
>> new patch set that is based on your changes.
>
> Great that you gave it a shot. Thanks for that.
>
>>
>> 1. I found that the hwmon sysfs interface is no longer created when I
>> call thermal_zone_of_sensor_register() to register my thermal sensor. I
>> believe it is because you purposely hard-coded 'no_hwmon' to true.
>> Would it be possible to let the users to control this option as part of
>> the sensor register function?
>
> Hmmm.. I also thought of allowing the thermal to hwmon interface
> configurable. But the interface gets created in different path from the
> sensor registration, which makes things a bit harder. The zones are
> probed while parsing the DT file. While the sensors get linked,
> thermal_zone_of_sensor_register(), typically when sensors are probed.
>
> Do you have a real use case for using the thermal to hwmon interface?
> Durgadoss (original author) and Rui were considering deprecating and
> finally removing it. If yes, it is time to rise your voice here.
>
>>
>> 2. The of_thermal_get_temp() output parameter 'temp' is an unsigned
>> value in order to conforms with the thermal framework 'get_temp'
>> function prototype. But the new sensor interface 'get_temp' callback
>> function (defined in __thermal_zone struct) expects 'temp' to be
>> signed. So technically, a negative value can be returned from the
>> underlying sensor function. Should some checks be added to
>> of_thermal_get_temp() to ensure that a negative value (i.e. a very large
>> unsigned value) won't be passed back to the thermal framework? Or
>> should the 'get_temp' function prototype in __thermal_zone struct be
>> changed with 'unsigned' temperature value and it will be the
>> responsibility of the underlying function to do the check?
>>
>
> I could for sure, add a warning inside of_thermal_get_temp(). But in
> general, I believe the thermal framework has to cover for signed
> temperature values, even if typical usage is on positive temperature
> range. Mainly for correct temperature representation.
>
>
>> 3. Your documentation (thermal.txt) seems to suggest that the
>> 'cooling-attachments' are one of the required properties for
>> 'thermal-zones' node in the device tree binding files. However, I don't
>
> Hmm... That was not the intention.
>
>> have any cooling device ready in my current patch series yet and
>> therefore I tried to leave out the 'cooling-attachments' node from the
>> 'thermal-zones' node. It seems to be working just find and I can
>> register the sensor and read out the temperature once the target
>> platform has been booted up. Is this your expectation that people can
>> leave the 'cooling-attachments' node out? I want to make sure I can
>> still submit a new patch set for this series with just the functionality
>> of reading out the temperature from the thermal sensor.
>
> So, let me try to clarify a couple of things. First thing is that the
> cooling-attachments has been renamed to cooling-maps, just a new naming
> convention. Also, there are thermal zones that do not have
> cooling-maps, they just describe thermal shutdown by software, thermal
> zones with only CRITICAL trip points, for instance. For this reason,
> thermal zones must be probed properly without cooling-maps. I need to
> double check the documentation in this case, I believe.

Just to clarify a bit, this does not mean we should allow having thermal
zones only for monitoring. If you are generating a thermal zone, it
means you want to cover thermal limits. The example I gave was a thermal
zone that does not have cooling-maps, but it does have a thermal
constraint, a trip point which is CRITICAL, meaning, the silicon state
is not reliable anymore.

>
>>
>> Thanks,
>> -Wendy
>>
>> On 9/23/2013 4:46 PM, Eduardo Valentin wrote:
>>> On 23-09-2013 18:43, Wendy Ng wrote:
>>>> On 9/23/2013 11:19 AM, Eduardo Valentin wrote:
>>>>> On 23-09-2013 13:51, Wendy Ng wrote:
>>>>>> This adds the support for reading out temperature from Broadcom
>>>>>> bcm281xx
>>>>>> SoCs.
>>>>>>
>>>>>> Signed-off-by: Wendy Ng <wendy.ng@xxxxxxxxxxxx>
>>>>>> Reviewed-by: Markus Mayer <mmayer@xxxxxxxxxxxx>
>>>>>> Reviewed-by: Christian Daudt <csd@xxxxxxxxxxxx>
>>>>>> ---
>>>>>> .../bindings/thermal/bcm-kona-thermal.txt | 18 +++
>>>>>> drivers/thermal/Kconfig | 10 ++
>>>>>> drivers/thermal/Makefile | 1 +
>>>>>> drivers/thermal/bcm_thermal.c | 170
>>>>>> ++++++++++++++++++++
>>>>>> 4 files changed, 199 insertions(+)
>>>>>> create mode 100644
>>>>>> Documentation/devicetree/bindings/thermal/bcm-kona-thermal.txt
>>>>>> create mode 100644 drivers/thermal/bcm_thermal.c
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/thermal/bcm-kona-thermal.txt
>>>>>> b/Documentation/devicetree/bindings/thermal/bcm-kona-thermal.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..acca99e
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/thermal/bcm-kona-thermal.txt
>>>>>> @@ -0,0 +1,18 @@
>>>>>> +* Broadcom Kona Thermal Management Unit
>>>>>> +
>>>>>> +This version is for the BCM281xx family of SoCs.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible : "brcm,bcm11351-thermal", "brcm,kona-thermal"
>>>>>> +- reg : Address range of the thermal register
>>>>>> +- thermal-name: this entry must be specified and it will be passed
>>>>>> into
>>>>>> +thermal_zone_device_register(). This name will also be reported
>>>>>> under Hwmon
>>>>>> +sysfs 'name' attribute.
>>>>>> +
>>>>>> +Example:
>>>>>> + thermal@34008000 {
>>>>>> + compatible = "brcm,bcm11351-thermal", "brcm,kona-thermal";
>>>>>> + reg = <0x34008000 0x0024>;
>>>>>> + thermal-name = "bcm_kona_therm";
>>>>>> + status = "disabled";
>>>>>> + };
>>>>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>>>>>> index dbfc390..7f823f0 100644
>>>>>> --- a/drivers/thermal/Kconfig
>>>>>> +++ b/drivers/thermal/Kconfig
>>>>>> @@ -134,6 +134,16 @@ config KIRKWOOD_THERMAL
>>>>>> Support for the Kirkwood thermal sensor driver into the Linux
>>>>>> thermal
>>>>>> framework. Only kirkwood 88F6282 and 88F6283 have this
>>>>>> sensor.
>>>>>> +config BCM_THERMAL
>>>>>> + tristate "Temperature sensor on Broadcom BCM281xx family of SoCs"
>>>>>> + depends on ARCH_BCM
>>>>>> + default y
>>>>>> + help
>>>>>> + If you say yes here you get support for TMU (Thermal Management
>>>>>> + Unit) on Broadcom BCM281xx family of SoCs. This provides
>>>>>> thermal
>>>>>> + monitoring of CPU clusters, graphics, and SoC glue, but does
>>>>>> not
>>>>>> + include monitoring of charger temperature.
>>>>>> +
>>>>>> config DOVE_THERMAL
>>>>>> tristate "Temperature sensor on Marvell Dove SoCs"
>>>>>> depends on ARCH_DOVE
>>>>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>>>>>> index 584b363..3ea8c1c 100644
>>>>>> --- a/drivers/thermal/Makefile
>>>>>> +++ b/drivers/thermal/Makefile
>>>>>> @@ -21,6 +21,7 @@ obj-$(CONFIG_SPEAR_THERMAL) += spear_thermal.o
>>>>>> obj-$(CONFIG_RCAR_THERMAL) += rcar_thermal.o
>>>>>> obj-$(CONFIG_KIRKWOOD_THERMAL) += kirkwood_thermal.o
>>>>>> obj-y += samsung/
>>>>>> +obj-$(CONFIG_BCM_THERMAL) += bcm_thermal.o
>>>>>> obj-$(CONFIG_DOVE_THERMAL) += dove_thermal.o
>>>>>> obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o
>>>>>> obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o
>>>>>> diff --git a/drivers/thermal/bcm_thermal.c
>>>>>> b/drivers/thermal/bcm_thermal.c
>>>>>> new file mode 100644
>>>>>> index 0000000..131d3c4
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/thermal/bcm_thermal.c
>>>>>> @@ -0,0 +1,170 @@
>>>>>> +/*
>>>>>> + * Copyright 2013 Broadcom Corporation.
>>>>>> + *
>>>>>> + * This program is free software; you can redistribute it and/or
>>>>>> modify
>>>>>> + * it under the terms of the GNU General Public License, version 2,
>>>>>> + * as published by the Free Software Foundation (the "GPL").
>>>>>> + *
>>>>>> + * This program is distributed in the hope that it will be useful,
>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>>>> + * GNU General Public License for more details.
>>>>>> + *
>>>>>> + * A copy of the GPL is available at
>>>>>> http://www.broadcom.com/licenses/GPLv2.php,
>>>>>> + * or by writing to the Free Software Foundation, Inc.,
>>>>>> + * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
>>>>>> + */
>>>>>> +
>>>>>> +/**
>>>>>> +* Broadcom Thermal Management Unit - bcm_tmu
>>>>>> +*/
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/err.h>
>>>>>> +#include <linux/kernel.h>
>>>>>> +#include <linux/platform_device.h>
>>>>>> +#include <linux/io.h>
>>>>>> +#include <linux/thermal.h>
>>>>>> +#include <linux/of.h>
>>>>>> +
>>>>>> +/* From TMON Register Database */
>>>>>> +#define TMON_TEMP_VAL_OFFSET 0x0000001c
>>>>>> +#define TMON_TEMP_VAL_TEMP_VAL_SHIFT 0
>>>>>> +#define TMON_TEMP_VAL_TEMP_VAL_MASK 0x000003ff
>>>>>> +
>>>>>> +/* Broadcom Thermal Zone Device Structure */
>>>>>> +struct bcm_thermal_zone_priv {
>>>>>> + char name[THERMAL_NAME_LENGTH];
>>>>>> + void __iomem *base;
>>>>>> +};
>>>>>> +
>>>>>> +/* Temperature conversion function for TMON block */
>>>>>> +static long raw_to_mcelsius(u32 raw)
>>>>>> +{
>>>>>> + /*
>>>>>> + * According to Broadcom internal Analog Module Specification
>>>>>> + * the formula for converting TMON block output to temperature in
>>>>>> + * degree Celsius is:
>>>>>> + * T = 428 - (0.561 * raw)
>>>>>> + * Note: the valid operating range for the TMON block is -40C to
>>>>>> 125C
>>>>>> + */
>>>>>> + return 428000 - (561 * (long)raw);
>>>>>> +}
>>>>>> +
>>>>>> +/* Get temperature callback function for thermal zone */
>>>>>> +static int bcm_get_temp(struct thermal_zone_device *thermal,
>>>>>> + unsigned long *temp)
>>>>>> +{
>>>>>> + u32 raw;
>>>>>> + long mcelsius;
>>>>>> + struct bcm_thermal_zone_priv *priv = thermal->devdata;
>>>>>> +
>>>>>> + if (!priv) {
>>>>>> + pr_err("%s: thermal zone number %d devdata not
>>>>>> initialized.\n",
>>>>>> + __func__, thermal->id);
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + raw = (readl(priv->base + TMON_TEMP_VAL_OFFSET)
>>>>>> + & TMON_TEMP_VAL_TEMP_VAL_MASK) >>
>>>>>> TMON_TEMP_VAL_TEMP_VAL_SHIFT;
>>>>>> +
>>>>>> + pr_debug("%s: thermal zone number %d raw temp 0x%x\n", __func__,
>>>>>> + thermal->id, raw);
>>>>>> +
>>>>>> + mcelsius = raw_to_mcelsius(raw);
>>>>>> +
>>>>>> + /*
>>>>>> + * Since 'mcelsius' might be negative, we need to limit it to
>>>>>> smallest
>>>>>> + * unsigned value before returning it to thermal framework.
>>>>>> + */
>>>>>> + if (mcelsius < 0)
>>>>>> + *temp = 0;
>>>>>> + else
>>>>>> + *temp = mcelsius;
>>>>>> +
>>>>>> + pr_debug("%s: thermal zone number %d final temp %d\n", __func__,
>>>>>> + thermal->id, (int) *temp);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +/* Operation callback functions for thermal zone */
>>>>>> +static struct thermal_zone_device_ops bcm_dev_ops = {
>>>>>> + .get_temp = bcm_get_temp,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct of_device_id bcm_tmu_match_table[] = {
>>>>>> + { .compatible = "brcm,kona-thermal" },
>>>>>> + {},
>>>>>> +};
>>>>>> +MODULE_DEVICE_TABLE(of, bcm_tmu_match_table);
>>>>>> +
>>>>>> +static int bcm_tmu_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> + struct thermal_zone_device *thermal = NULL;
>>>>>> + struct bcm_thermal_zone_priv *priv;
>>>>>> + struct resource *res;
>>>>>> + const char *str;
>>>>>> +
>>>>>> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>>>>> + if (!priv) {
>>>>>> + dev_err(&pdev->dev, "Failed to malloc priv.\n");
>>>>>> + return -ENOMEM;
>>>>>> + }
>>>>>> +
>>>>>> + /* Obtain the tmu name from device tree file */
>>>>>> + if (of_property_read_string(pdev->dev.of_node, "thermal-name",
>>>>> Hello Wendy,
>>>>>
>>>>> I would prefer we wait until the work for thermal data [1] gets
>>>>> accepted
>>>>> before accepting this driver, specially because you are adding device
>>>>> specific DT entry to help in your registration with the thermal
>>>>> framework. With the mentioned work you wont need it at all.
>>>>>
>>>>> All best,
>>>>>
>>>>> [1] - http://lkml.org/lkml/2013/9/15/122
>>>> Hello Eduardo,
>>>>
>>>> Since your changes are quite extensive, would you recommend me to get
>>>> your 16 patches into my local workspace and try to integrate the code
>>>> from my patches on the top of your patches at this time? Or should I
>>>> wait until your patches get accepted.
>>> Hello again Wendy,
>>>
>>> The only part I am concerned is the device tree entry you are creating
>>> for your device/driver. That could be obsolete in near future with the
>>> work I am doing currently.
>>>
>>> The work is, as you mentioned, quite extensive, but should make things
>>> easier for device driver writer. If you want, of course, it would be
>>> great if you could rebase your work on top of mine and try it out on
>>> your side and let me know the outcome.
>>>
>>> Comments on the proposed binding and documentation is also welcome. Let
>>> me know if there is something that it is not clear.
>>>
>>>> Thanks,
>>>> -Wendy
>>>>>> + &str) == 0) {
>>>>>> + strlcpy(priv->name, str, sizeof(priv->name));
>>>>>> + } else {
>>>>>> + dev_err(&pdev->dev, "Failed to get thermal-name from DT.\n");
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>> + priv->base = devm_ioremap_resource(&pdev->dev, res);
>>>>>> + if (IS_ERR(priv->base))
>>>>>> + return PTR_ERR(priv->base);
>>>>>> +
>>>>>> + thermal = thermal_zone_device_register(priv->name, 0, 0, priv,
>>>>>> + &bcm_dev_ops, NULL, 0, 0);
>>>>>> + if (IS_ERR(thermal)) {
>>>>>> + dev_err(&pdev->dev,
>>>>>> + "Failed to register Broadcom thermal zone device.\n");
>>>>>> + return PTR_ERR(thermal);
>>>>>> + }
>>>>>> +
>>>>>> + platform_set_drvdata(pdev, thermal);
>>>>>> +
>>>>>> + dev_info(&pdev->dev, "Broadcom Thermal Monitor Initialized.\n");
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int bcm_tmu_remove(struct platform_device *pdev)
>>>>>> +{
>>>>>> + struct thermal_zone_device *broadcom_thermal =
>>>>>> + platform_get_drvdata(pdev);
>>>>>> +
>>>>>> + thermal_zone_device_unregister(broadcom_thermal);
>>>>>> +
>>>>>> + dev_info(&pdev->dev, "Broadcom Thermal Monitor
>>>>>> Uninitialized.\n");
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static struct platform_driver bcm_tmu_driver = {
>>>>>> + .driver = {
>>>>>> + .name = "bcm-thermal",
>>>>>> + .owner = THIS_MODULE,
>>>>>> + .of_match_table = bcm_tmu_match_table,
>>>>>> + },
>>>>>> + .probe = bcm_tmu_probe,
>>>>>> + .remove = bcm_tmu_remove,
>>>>>> +};
>>>>>> +
>>>>>> +module_platform_driver(bcm_tmu_driver);
>>>>>> +
>>>>>> +MODULE_DESCRIPTION("Broadcom Thermal Driver");
>>>>>> +MODULE_AUTHOR("Broadcom");
>>>>>> +MODULE_LICENSE("GPL v2");
>>>>>> +MODULE_ALIAS("platform:bcm-thermal");
>>>>>>
>>>
>>
>
>

--
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin

Attachment: signature.asc
Description: OpenPGP digital signature

Next message: Steven Rostedt: "Re: [PATCHv5 11/11] x86, mm: enable split page table lock for PMDlevel"
Previous message: Stephane Eranian: "[PATCH v1 1/2] perf,x86: add Intel RAPL PMU support"
In reply to: Eduardo Valentin: "Re: [PATCH 1/3] thermal: bcm281xx: Add thermal driver"
Next in thread: Wendy Ng: "Re: [PATCH 1/3] thermal: bcm281xx: Add thermal driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]