Date:	Mon, 7 Oct 2013 10:52:21 -0700
From:	"Wendy Ng" <wendy.ng@...adcom.com>
To:	"Eduardo Valentin" <eduardo.valentin@...com>
cc:	"Mark Rutland" <Mark.Rutland@....com>,
	"Rob Herring" <rob.herring@...xeda.com>,
	"Stephen Warren" <swarren@...dotorg.org>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org, "Christian Daudt" <bcm@...thebug.org>,
	"Markus Mayer" <mmayer@...adcom.com>
Subject: Re: [PATCH 1/3] thermal: bcm281xx: Add thermal driver


On 10/7/2013 9:12 AM, Eduardo Valentin wrote:
> 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.
Hi Eduardo,

Thanks for your clarification.    Yes, there are thermal limits that I 
need to specify through the trip points.  The issue I am facing is that 
I don't have the cooling device that needs to be associated with the 
trip point in this patch series yet.  I was hoping to get the thermal 
sensor driver code into the mainline first.  The addition for the 
cooling device and the "cooling-maps" node will be added in another 
patch series.

Since this patch series is now gated by your work, I am wondering if you 
are targeting to get your patch series 
(http://lkml.org/lkml/2013/9/15/122) into 3.13?  I would like to get an 
idea of the timeline such that I can plan out my work as well.

Thanks!
>>> 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@...adcom.com>
>>>>>>> Reviewed-by: Markus Mayer <mmayer@...adcom.com>
>>>>>>> Reviewed-by: Christian Daudt <csd@...adcom.com>
>>>>>>> ---
>>>>>>>     .../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@...08000 {
>>>>>>> +        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");
>>>>>>>
>>
>

-- 
Best regards,
-Wendy


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists