diff mbox

[V2,2/2] thermal: broadcom: add Northstar thermal driver

Message ID 20170323223045.15786-2-zajec5@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Eduardo Valentin
Headers show

Commit Message

Rafał Miłecki March 23, 2017, 10:30 p.m. UTC
From: Rafał Miłecki <rafal@milecki.pl>

Northstar is a SoC family commonly used in home routers. This commit
adds a driver for checking CPU temperature. As Northstar Plus seems to
also have this IP block this new symbol gets ARCH_BCM_IPROC dependency.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Signed-off-by: Jon Mason <jon.mason@broadcom.com>
---
V2: Make it iProc specific as NSP can also use this driver
    Select proper symbols in config ARCH_BCM_IPROC
    Define PVTMON register bits
    Update code selecting temperature monitor mode
    Thank you Jon!
---
 arch/arm/mach-bcm/Kconfig             |  2 +
 drivers/thermal/Kconfig               |  5 ++
 drivers/thermal/Makefile              |  1 +
 drivers/thermal/broadcom/Kconfig      |  7 +++
 drivers/thermal/broadcom/Makefile     |  1 +
 drivers/thermal/broadcom/ns-thermal.c | 98 +++++++++++++++++++++++++++++++++++
 6 files changed, 114 insertions(+)
 create mode 100644 drivers/thermal/broadcom/Kconfig
 create mode 100644 drivers/thermal/broadcom/Makefile
 create mode 100644 drivers/thermal/broadcom/ns-thermal.c

Comments

Jon Mason March 24, 2017, 2:35 p.m. UTC | #1
On Thu, Mar 23, 2017 at 11:30:45PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Northstar is a SoC family commonly used in home routers. This commit
> adds a driver for checking CPU temperature. As Northstar Plus seems to
> also have this IP block this new symbol gets ARCH_BCM_IPROC dependency.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> ---
> V2: Make it iProc specific as NSP can also use this driver
>     Select proper symbols in config ARCH_BCM_IPROC
>     Define PVTMON register bits
>     Update code selecting temperature monitor mode
>     Thank you Jon!
> ---
>  arch/arm/mach-bcm/Kconfig             |  2 +
>  drivers/thermal/Kconfig               |  5 ++
>  drivers/thermal/Makefile              |  1 +
>  drivers/thermal/broadcom/Kconfig      |  7 +++
>  drivers/thermal/broadcom/Makefile     |  1 +
>  drivers/thermal/broadcom/ns-thermal.c | 98 +++++++++++++++++++++++++++++++++++
>  6 files changed, 114 insertions(+)
>  create mode 100644 drivers/thermal/broadcom/Kconfig
>  create mode 100644 drivers/thermal/broadcom/Makefile
>  create mode 100644 drivers/thermal/broadcom/ns-thermal.c
> 
> diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
> index a0e66d8200c5..da2bfeb8e390 100644
> --- a/arch/arm/mach-bcm/Kconfig
> +++ b/arch/arm/mach-bcm/Kconfig
> @@ -19,6 +19,8 @@ config ARCH_BCM_IPROC
>  	select GPIOLIB
>  	select ARM_AMBA
>  	select PINCTRL
> +	select THERMAL
> +	select THERMAL_OF
>  	help
>  	  This enables support for systems based on Broadcom IPROC architected SoCs.
>  	  The IPROC complex contains one or more ARM CPUs along with common
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 776b34396144..008e173ec825 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -392,6 +392,11 @@ config MTK_THERMAL
>  	  Enable this option if you want to have support for thermal management
>  	  controller present in Mediatek SoCs
>  
> +menu "Broadcom thermal drivers"
> +depends on ARCH_BCM || COMPILE_TEST
> +source "drivers/thermal/broadcom/Kconfig"
> +endmenu
> +
>  menu "Texas Instruments thermal drivers"
>  depends on ARCH_HAS_BANDGAP || COMPILE_TEST
>  depends on HAS_IOMEM
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 7adae2029355..549d81b6363c 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -27,6 +27,7 @@ thermal_sys-$(CONFIG_CLOCK_THERMAL)	+= clock_cooling.o
>  thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
>  
>  # platform thermal drivers
> +obj-y				+= broadcom/
>  obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
>  obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
> diff --git a/drivers/thermal/broadcom/Kconfig b/drivers/thermal/broadcom/Kconfig
> new file mode 100644
> index 000000000000..39b92a1f3584
> --- /dev/null
> +++ b/drivers/thermal/broadcom/Kconfig
> @@ -0,0 +1,7 @@
> +config BCM_NS_THERMAL
> +	tristate "Northstar thermal driver"
> +	depends on ARCH_BCM_IPROC || COMPILE_TEST
> +	help
> +	  Northstar's DMU (Device Management Unit) block contains a thermal
> +	  sensor that allows checking CPU temperature. This driver provides
> +	  support for it.
> diff --git a/drivers/thermal/broadcom/Makefile b/drivers/thermal/broadcom/Makefile
> new file mode 100644
> index 000000000000..059df9a0ed69
> --- /dev/null
> +++ b/drivers/thermal/broadcom/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_BCM_NS_THERMAL)		+= ns-thermal.o
> diff --git a/drivers/thermal/broadcom/ns-thermal.c b/drivers/thermal/broadcom/ns-thermal.c
> new file mode 100644
> index 000000000000..acf3a4de62a4
> --- /dev/null
> +++ b/drivers/thermal/broadcom/ns-thermal.c
> @@ -0,0 +1,98 @@
> +/*
> + * Copyright (C) 2017 Rafał Miłecki <rafal@milecki.pl>
> + *
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +
> +#define PVTMON_CONTROL0					0x00
> +#define PVTMON_CONTROL0_SEL_MASK			0x0000000e
> +#define PVTMON_CONTROL0_SEL_TEMP_MONITOR		0x00000000
> +#define PVTMON_CONTROL0_SEL_TEST_MODE			0x0000000e
> +#define PVTMON_STATUS					0x08
> +
> +struct ns_thermal {
> +	void __iomem *pvtmon;
> +};
> +
> +static int ns_thermal_get_temp(void *data, int *temp)
> +{
> +	struct ns_thermal *ns_thermal = data;
> +	u32 val;
> +
> +	val = readl(ns_thermal->pvtmon + PVTMON_CONTROL0);
> +	if ((val & PVTMON_CONTROL0_SEL_MASK) != PVTMON_CONTROL0_SEL_TEMP_MONITOR) {
> +		val &= ~PVTMON_CONTROL0_SEL_MASK;
> +		val |= PVTMON_CONTROL0_SEL_TEMP_MONITOR;

I think this is slightly confusing here.  If this was off, ORing in 0
will not enable it.  I think we need to flip the #define to make it
PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE (or a less crappy name).
then use this line here to toggle it.  Something like

                val &= ~PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE;

Thanks,
Jon

> +		writel(val, ns_thermal->pvtmon + PVTMON_CONTROL0);
> +	}
> +
> +	val = readl(ns_thermal->pvtmon + PVTMON_STATUS);
> +	*temp = (418 - (val * 5556 / 10000)) * 1000;
> +
> +	return 0;
> +}
> +
> +const struct thermal_zone_of_device_ops ns_thermal_ops = {
> +	.get_temp = ns_thermal_get_temp,
> +};
> +
> +static int ns_thermal_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ns_thermal *ns_thermal;
> +	struct thermal_zone_device *tzd;
> +
> +	ns_thermal = devm_kzalloc(dev, sizeof(*ns_thermal), GFP_KERNEL);
> +	if (!ns_thermal)
> +		return -ENOMEM;
> +
> +	ns_thermal->pvtmon = of_iomap(dev_of_node(dev), 0);
> +	if (WARN_ON(!ns_thermal->pvtmon))
> +		return -ENOENT;
> +
> +	tzd = devm_thermal_zone_of_sensor_register(dev, 0, ns_thermal,
> +						   &ns_thermal_ops);
> +	if (IS_ERR(tzd)) {
> +		iounmap(ns_thermal->pvtmon);
> +		return PTR_ERR(tzd);
> +	}
> +
> +	platform_set_drvdata(pdev, ns_thermal);
> +
> +	return 0;
> +}
> +
> +static int ns_thermal_remove(struct platform_device *pdev)
> +{
> +	struct ns_thermal *ns_thermal = platform_get_drvdata(pdev);
> +
> +	iounmap(ns_thermal->pvtmon);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ns_thermal_of_match[] = {
> +	{ .compatible = "brcm,ns-thermal", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ns_thermal_of_match);
> +
> +static struct platform_driver ns_thermal_driver = {
> +	.probe		= ns_thermal_probe,
> +	.remove		= ns_thermal_remove,
> +	.driver = {
> +		.name = "ns-thermal",
> +		.of_match_table = ns_thermal_of_match,
> +	},
> +};
> +module_platform_driver(ns_thermal_driver);
> +
> +MODULE_DESCRIPTION("Northstar thermal driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.11.0
>
Eduardo Valentin March 31, 2017, 3:15 a.m. UTC | #2
Hello,

On Thu, Mar 23, 2017 at 11:30:45PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Northstar is a SoC family commonly used in home routers. This commit
> adds a driver for checking CPU temperature. As Northstar Plus seems to
> also have this IP block this new symbol gets ARCH_BCM_IPROC dependency.
> 

Can you please educate me on how different this driver is from 
https://patchwork.kernel.org/patch/9619579/

?

> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> ---
> V2: Make it iProc specific as NSP can also use this driver
>     Select proper symbols in config ARCH_BCM_IPROC
>     Define PVTMON register bits
>     Update code selecting temperature monitor mode
>     Thank you Jon!
> ---
>  arch/arm/mach-bcm/Kconfig             |  2 +
>  drivers/thermal/Kconfig               |  5 ++
>  drivers/thermal/Makefile              |  1 +
>  drivers/thermal/broadcom/Kconfig      |  7 +++
>  drivers/thermal/broadcom/Makefile     |  1 +
>  drivers/thermal/broadcom/ns-thermal.c | 98 +++++++++++++++++++++++++++++++++++

Ok. This driver attempts to create a common place for broadcom thermal
drivers, which is good, but..

>  6 files changed, 114 insertions(+)
>  create mode 100644 drivers/thermal/broadcom/Kconfig
>  create mode 100644 drivers/thermal/broadcom/Makefile
>  create mode 100644 drivers/thermal/broadcom/ns-thermal.c
> 
> diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
> index a0e66d8200c5..da2bfeb8e390 100644
> --- a/arch/arm/mach-bcm/Kconfig
> +++ b/arch/arm/mach-bcm/Kconfig
> @@ -19,6 +19,8 @@ config ARCH_BCM_IPROC
>  	select GPIOLIB
>  	select ARM_AMBA
>  	select PINCTRL
> +	select THERMAL
> +	select THERMAL_OF
>  	help
>  	  This enables support for systems based on Broadcom IPROC architected SoCs.
>  	  The IPROC complex contains one or more ARM CPUs along with common

To avoid conflicts I would suggest getting this patch split into driver
and arch code changes.

> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 776b34396144..008e173ec825 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -392,6 +392,11 @@ config MTK_THERMAL
>  	  Enable this option if you want to have support for thermal management
>  	  controller present in Mediatek SoCs
>  
> +menu "Broadcom thermal drivers"
> +depends on ARCH_BCM || COMPILE_TEST

Does the above dependency also work for other BCM chips?

> +source "drivers/thermal/broadcom/Kconfig"
> +endmenu
> +
>  menu "Texas Instruments thermal drivers"
>  depends on ARCH_HAS_BANDGAP || COMPILE_TEST
>  depends on HAS_IOMEM
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 7adae2029355..549d81b6363c 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -27,6 +27,7 @@ thermal_sys-$(CONFIG_CLOCK_THERMAL)	+= clock_cooling.o
>  thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
>  
>  # platform thermal drivers
> +obj-y				+= broadcom/
>  obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
>  obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
> diff --git a/drivers/thermal/broadcom/Kconfig b/drivers/thermal/broadcom/Kconfig
> new file mode 100644
> index 000000000000..39b92a1f3584
> --- /dev/null
> +++ b/drivers/thermal/broadcom/Kconfig
> @@ -0,0 +1,7 @@
> +config BCM_NS_THERMAL
> +	tristate "Northstar thermal driver"
> +	depends on ARCH_BCM_IPROC || COMPILE_TEST
> +	help
> +	  Northstar's DMU (Device Management Unit) block contains a thermal
> +	  sensor that allows checking CPU temperature. This driver provides
> +	  support for it.

It would be good if you could include a list of socs that have DMUs.

> diff --git a/drivers/thermal/broadcom/Makefile b/drivers/thermal/broadcom/Makefile
> new file mode 100644
> index 000000000000..059df9a0ed69
> --- /dev/null
> +++ b/drivers/thermal/broadcom/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_BCM_NS_THERMAL)		+= ns-thermal.o
> diff --git a/drivers/thermal/broadcom/ns-thermal.c b/drivers/thermal/broadcom/ns-thermal.c
> new file mode 100644
> index 000000000000..acf3a4de62a4
> --- /dev/null
> +++ b/drivers/thermal/broadcom/ns-thermal.c
> @@ -0,0 +1,98 @@
> +/*
> + * Copyright (C) 2017 Rafał Miłecki <rafal@milecki.pl>
> + *
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +
> +#define PVTMON_CONTROL0					0x00
> +#define PVTMON_CONTROL0_SEL_MASK			0x0000000e
> +#define PVTMON_CONTROL0_SEL_TEMP_MONITOR		0x00000000
> +#define PVTMON_CONTROL0_SEL_TEST_MODE			0x0000000e
> +#define PVTMON_STATUS					0x08
> +
> +struct ns_thermal {
> +	void __iomem *pvtmon;
> +};
> +
> +static int ns_thermal_get_temp(void *data, int *temp)
> +{
> +	struct ns_thermal *ns_thermal = data;
> +	u32 val;
> +
> +	val = readl(ns_thermal->pvtmon + PVTMON_CONTROL0);
> +	if ((val & PVTMON_CONTROL0_SEL_MASK) != PVTMON_CONTROL0_SEL_TEMP_MONITOR) {
> +		val &= ~PVTMON_CONTROL0_SEL_MASK;
> +		val |= PVTMON_CONTROL0_SEL_TEMP_MONITOR;
> +		writel(val, ns_thermal->pvtmon + PVTMON_CONTROL0);
> +	}
> +
> +	val = readl(ns_thermal->pvtmon + PVTMON_STATUS);
> +	*temp = (418 - (val * 5556 / 10000)) * 1000;
> +

Could you move the above equation to DT? The other BCM driver I posted
the link above just went through the same process.

> +	return 0;
> +}
> +
> +const struct thermal_zone_of_device_ops ns_thermal_ops = {
> +	.get_temp = ns_thermal_get_temp,
> +};
> +
> +static int ns_thermal_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ns_thermal *ns_thermal;
> +	struct thermal_zone_device *tzd;
> +
> +	ns_thermal = devm_kzalloc(dev, sizeof(*ns_thermal), GFP_KERNEL);
> +	if (!ns_thermal)
> +		return -ENOMEM;
> +
> +	ns_thermal->pvtmon = of_iomap(dev_of_node(dev), 0);
> +	if (WARN_ON(!ns_thermal->pvtmon))
> +		return -ENOENT;
> +
> +	tzd = devm_thermal_zone_of_sensor_register(dev, 0, ns_thermal,
> +						   &ns_thermal_ops);
> +	if (IS_ERR(tzd)) {
> +		iounmap(ns_thermal->pvtmon);
> +		return PTR_ERR(tzd);
> +	}
> +
> +	platform_set_drvdata(pdev, ns_thermal);
> +
> +	return 0;
> +}
> +
> +static int ns_thermal_remove(struct platform_device *pdev)
> +{
> +	struct ns_thermal *ns_thermal = platform_get_drvdata(pdev);
> +
> +	iounmap(ns_thermal->pvtmon);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ns_thermal_of_match[] = {
> +	{ .compatible = "brcm,ns-thermal", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ns_thermal_of_match);
> +
> +static struct platform_driver ns_thermal_driver = {
> +	.probe		= ns_thermal_probe,
> +	.remove		= ns_thermal_remove,
> +	.driver = {
> +		.name = "ns-thermal",
> +		.of_match_table = ns_thermal_of_match,
> +	},
> +};
> +module_platform_driver(ns_thermal_driver);
> +
> +MODULE_DESCRIPTION("Northstar thermal driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.11.0
>
Rafał Miłecki March 31, 2017, 7:03 a.m. UTC | #3
On 03/24/2017 03:35 PM, Jon Mason wrote:
> On Thu, Mar 23, 2017 at 11:30:45PM +0100, Rafał Miłecki wrote:
>> diff --git a/drivers/thermal/broadcom/ns-thermal.c b/drivers/thermal/broadcom/ns-thermal.c
>> new file mode 100644
>> index 000000000000..acf3a4de62a4
>> --- /dev/null
>> +++ b/drivers/thermal/broadcom/ns-thermal.c
>> @@ -0,0 +1,98 @@
>> +/*
>> + * Copyright (C) 2017 Rafał Miłecki <rafal@milecki.pl>
>> + *
>> + * 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.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/thermal.h>
>> +
>> +#define PVTMON_CONTROL0					0x00
>> +#define PVTMON_CONTROL0_SEL_MASK			0x0000000e
>> +#define PVTMON_CONTROL0_SEL_TEMP_MONITOR		0x00000000
>> +#define PVTMON_CONTROL0_SEL_TEST_MODE			0x0000000e
>> +#define PVTMON_STATUS					0x08
>> +
>> +struct ns_thermal {
>> +	void __iomem *pvtmon;
>> +};
>> +
>> +static int ns_thermal_get_temp(void *data, int *temp)
>> +{
>> +	struct ns_thermal *ns_thermal = data;
>> +	u32 val;
>> +
>> +	val = readl(ns_thermal->pvtmon + PVTMON_CONTROL0);
>> +	if ((val & PVTMON_CONTROL0_SEL_MASK) != PVTMON_CONTROL0_SEL_TEMP_MONITOR) {
>> +		val &= ~PVTMON_CONTROL0_SEL_MASK;
>> +		val |= PVTMON_CONTROL0_SEL_TEMP_MONITOR;
>
> I think this is slightly confusing here.  If this was off, ORing in 0
> will not enable it.  I think we need to flip the #define to make it
> PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE (or a less crappy name).
> then use this line here to toggle it.  Something like
>
>                 val &= ~PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE;

I don't understand this, can you help me further?

OR-ing with 0 is only a cosmetic thing obviously - just to make it clear which
value we expect to be set in bits 1:3. The important part is:
val &= ~PVTMON_CONTROL0_SEL_MASK;

AFAIU selecting any mode other than TEMP_MONITOR will disable monitor access.
AFAIU even switchint to TEST_MODE will prevent access to temp, so I don't see
how we could use PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE. It could be any
value other than 0.
Rafał Miłecki March 31, 2017, 7:08 a.m. UTC | #4
Thanks for your review!

On 03/31/2017 05:15 AM, Eduardo Valentin wrote:
> On Thu, Mar 23, 2017 at 11:30:45PM +0100, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Northstar is a SoC family commonly used in home routers. This commit
>> adds a driver for checking CPU temperature. As Northstar Plus seems to
>> also have this IP block this new symbol gets ARCH_BCM_IPROC dependency.
>>
>
> Can you please educate me on how different this driver is from
> https://patchwork.kernel.org/patch/9619579/
>
> ?

This is a different hardware block. Northstar and BCM283x are totally different
SoCs. Northstar is mostly used in home routers, BCM283x mostly in Rasperry Pi.


>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 776b34396144..008e173ec825 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -392,6 +392,11 @@ config MTK_THERMAL
>>  	  Enable this option if you want to have support for thermal management
>>  	  controller present in Mediatek SoCs
>>
>> +menu "Broadcom thermal drivers"
>> +depends on ARCH_BCM || COMPILE_TEST
>
> Does the above dependency also work for other BCM chips?

I believe so. If there ever appears a driver in drivers/thermal/broadcom/ that
can't be freely compiled with COMPILE_TEST, it can just have a proper
dependency.
Jon Mason March 31, 2017, 2:23 p.m. UTC | #5
On Fri, Mar 31, 2017 at 3:03 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 03/24/2017 03:35 PM, Jon Mason wrote:
>>
>> On Thu, Mar 23, 2017 at 11:30:45PM +0100, Rafał Miłecki wrote:
>>>
>>> diff --git a/drivers/thermal/broadcom/ns-thermal.c
>>> b/drivers/thermal/broadcom/ns-thermal.c
>>> new file mode 100644
>>> index 000000000000..acf3a4de62a4
>>> --- /dev/null
>>> +++ b/drivers/thermal/broadcom/ns-thermal.c
>>> @@ -0,0 +1,98 @@
>>> +/*
>>> + * Copyright (C) 2017 Rafał Miłecki <rafal@milecki.pl>
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/thermal.h>
>>> +
>>> +#define PVTMON_CONTROL0                                        0x00
>>> +#define PVTMON_CONTROL0_SEL_MASK                       0x0000000e
>>> +#define PVTMON_CONTROL0_SEL_TEMP_MONITOR               0x00000000
>>> +#define PVTMON_CONTROL0_SEL_TEST_MODE                  0x0000000e
>>> +#define PVTMON_STATUS                                  0x08
>>> +
>>> +struct ns_thermal {
>>> +       void __iomem *pvtmon;
>>> +};
>>> +
>>> +static int ns_thermal_get_temp(void *data, int *temp)
>>> +{
>>> +       struct ns_thermal *ns_thermal = data;
>>> +       u32 val;
>>> +
>>> +       val = readl(ns_thermal->pvtmon + PVTMON_CONTROL0);
>>> +       if ((val & PVTMON_CONTROL0_SEL_MASK) !=
>>> PVTMON_CONTROL0_SEL_TEMP_MONITOR) {
>>> +               val &= ~PVTMON_CONTROL0_SEL_MASK;
>>> +               val |= PVTMON_CONTROL0_SEL_TEMP_MONITOR;
>>
>>
>> I think this is slightly confusing here.  If this was off, ORing in 0
>> will not enable it.  I think we need to flip the #define to make it
>> PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE (or a less crappy name).
>> then use this line here to toggle it.  Something like
>>
>>                 val &= ~PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE;
>
>
> I don't understand this, can you help me further?
>
> OR-ing with 0 is only a cosmetic thing obviously - just to make it clear
> which
> value we expect to be set in bits 1:3. The important part is:
> val &= ~PVTMON_CONTROL0_SEL_MASK;

You are using a side effect of the masking to clear/enable the block.
I'm simply saying that we should be explicit about enabling it.  My
concern is that using the side effect hides what is being done and
could result in a bug somewhere down the line.  I think this is
improbable based on the code, but wanted to err on the side of
caution.

>
> AFAIU selecting any mode other than TEMP_MONITOR will disable monitor
> access.
> AFAIU even switchint to TEST_MODE will prevent access to temp, so I don't
> see
> how we could use PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE. It could be any
> value other than 0.
Rafał Miłecki March 31, 2017, 2:49 p.m. UTC | #6
On 03/31/2017 04:23 PM, Jon Mason wrote:
> On Fri, Mar 31, 2017 at 3:03 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On 03/24/2017 03:35 PM, Jon Mason wrote:
>>>
>>> On Thu, Mar 23, 2017 at 11:30:45PM +0100, Rafał Miłecki wrote:
>>>>
>>>> diff --git a/drivers/thermal/broadcom/ns-thermal.c
>>>> b/drivers/thermal/broadcom/ns-thermal.c
>>>> new file mode 100644
>>>> index 000000000000..acf3a4de62a4
>>>> --- /dev/null
>>>> +++ b/drivers/thermal/broadcom/ns-thermal.c
>>>> @@ -0,0 +1,98 @@
>>>> +/*
>>>> + * Copyright (C) 2017 Rafał Miłecki <rafal@milecki.pl>
>>>> + *
>>>> + * 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.
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/thermal.h>
>>>> +
>>>> +#define PVTMON_CONTROL0                                        0x00
>>>> +#define PVTMON_CONTROL0_SEL_MASK                       0x0000000e
>>>> +#define PVTMON_CONTROL0_SEL_TEMP_MONITOR               0x00000000
>>>> +#define PVTMON_CONTROL0_SEL_TEST_MODE                  0x0000000e
>>>> +#define PVTMON_STATUS                                  0x08
>>>> +
>>>> +struct ns_thermal {
>>>> +       void __iomem *pvtmon;
>>>> +};
>>>> +
>>>> +static int ns_thermal_get_temp(void *data, int *temp)
>>>> +{
>>>> +       struct ns_thermal *ns_thermal = data;
>>>> +       u32 val;
>>>> +
>>>> +       val = readl(ns_thermal->pvtmon + PVTMON_CONTROL0);
>>>> +       if ((val & PVTMON_CONTROL0_SEL_MASK) !=
>>>> PVTMON_CONTROL0_SEL_TEMP_MONITOR) {
>>>> +               val &= ~PVTMON_CONTROL0_SEL_MASK;
>>>> +               val |= PVTMON_CONTROL0_SEL_TEMP_MONITOR;
>>>
>>>
>>> I think this is slightly confusing here.  If this was off, ORing in 0
>>> will not enable it.  I think we need to flip the #define to make it
>>> PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE (or a less crappy name).
>>> then use this line here to toggle it.  Something like
>>>
>>>                 val &= ~PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE;
>>
>>
>> I don't understand this, can you help me further?
>>
>> OR-ing with 0 is only a cosmetic thing obviously - just to make it clear
>> which
>> value we expect to be set in bits 1:3. The important part is:
>> val &= ~PVTMON_CONTROL0_SEL_MASK;
>
> You are using a side effect of the masking to clear/enable the block.
> I'm simply saying that we should be explicit about enabling it.  My
> concern is that using the side effect hides what is being done and
> could result in a bug somewhere down the line.  I think this is
> improbable based on the code, but wanted to err on the side of
> caution.

Well, I'm clearing current mode selection and "selecting" the mode I want. By
OR-ing PVTMON_CONTROL0_SEL_TEMP_MONITOR I'm clearly indicating I want temp
monitor more.

How else I could make it more obvious? Should I add some comment maybe?
Jon Mason March 31, 2017, 5:29 p.m. UTC | #7
On Fri, Mar 31, 2017 at 10:49 AM, Rafał Miłecki <rafal@milecki.pl> wrote:
> On 03/31/2017 04:23 PM, Jon Mason wrote:
>>
>> On Fri, Mar 31, 2017 at 3:03 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>>>
>>> On 03/24/2017 03:35 PM, Jon Mason wrote:
>>>>
>>>>
>>>> On Thu, Mar 23, 2017 at 11:30:45PM +0100, Rafał Miłecki wrote:
>>>>>
>>>>>
>>>>> diff --git a/drivers/thermal/broadcom/ns-thermal.c
>>>>> b/drivers/thermal/broadcom/ns-thermal.c
>>>>> new file mode 100644
>>>>> index 000000000000..acf3a4de62a4
>>>>> --- /dev/null
>>>>> +++ b/drivers/thermal/broadcom/ns-thermal.c
>>>>> @@ -0,0 +1,98 @@
>>>>> +/*
>>>>> + * Copyright (C) 2017 Rafał Miłecki <rafal@milecki.pl>
>>>>> + *
>>>>> + * 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.
>>>>> + */
>>>>> +
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/of_address.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/thermal.h>
>>>>> +
>>>>> +#define PVTMON_CONTROL0                                        0x00
>>>>> +#define PVTMON_CONTROL0_SEL_MASK                       0x0000000e
>>>>> +#define PVTMON_CONTROL0_SEL_TEMP_MONITOR               0x00000000
>>>>> +#define PVTMON_CONTROL0_SEL_TEST_MODE                  0x0000000e
>>>>> +#define PVTMON_STATUS                                  0x08
>>>>> +
>>>>> +struct ns_thermal {
>>>>> +       void __iomem *pvtmon;
>>>>> +};
>>>>> +
>>>>> +static int ns_thermal_get_temp(void *data, int *temp)
>>>>> +{
>>>>> +       struct ns_thermal *ns_thermal = data;
>>>>> +       u32 val;
>>>>> +
>>>>> +       val = readl(ns_thermal->pvtmon + PVTMON_CONTROL0);
>>>>> +       if ((val & PVTMON_CONTROL0_SEL_MASK) !=
>>>>> PVTMON_CONTROL0_SEL_TEMP_MONITOR) {
>>>>> +               val &= ~PVTMON_CONTROL0_SEL_MASK;
>>>>> +               val |= PVTMON_CONTROL0_SEL_TEMP_MONITOR;
>>>>
>>>>
>>>>
>>>> I think this is slightly confusing here.  If this was off, ORing in 0
>>>> will not enable it.  I think we need to flip the #define to make it
>>>> PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE (or a less crappy name).
>>>> then use this line here to toggle it.  Something like
>>>>
>>>>                 val &= ~PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE;
>>>
>>>
>>>
>>> I don't understand this, can you help me further?
>>>
>>> OR-ing with 0 is only a cosmetic thing obviously - just to make it clear
>>> which
>>> value we expect to be set in bits 1:3. The important part is:
>>> val &= ~PVTMON_CONTROL0_SEL_MASK;
>>
>>
>> You are using a side effect of the masking to clear/enable the block.
>> I'm simply saying that we should be explicit about enabling it.  My
>> concern is that using the side effect hides what is being done and
>> could result in a bug somewhere down the line.  I think this is
>> improbable based on the code, but wanted to err on the side of
>> caution.
>
>
> Well, I'm clearing current mode selection and "selecting" the mode I want.
> By
> OR-ing PVTMON_CONTROL0_SEL_TEMP_MONITOR I'm clearly indicating I want temp
> monitor more.
>
> How else I could make it more obvious? Should I add some comment maybe?

I think it would be acceptable to remove the ORing of the 0, and
simply put a comment there that the masking of it is clearing the 0th
bit, thus enabling the IP block.
diff mbox

Patch

diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index a0e66d8200c5..da2bfeb8e390 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -19,6 +19,8 @@  config ARCH_BCM_IPROC
 	select GPIOLIB
 	select ARM_AMBA
 	select PINCTRL
+	select THERMAL
+	select THERMAL_OF
 	help
 	  This enables support for systems based on Broadcom IPROC architected SoCs.
 	  The IPROC complex contains one or more ARM CPUs along with common
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 776b34396144..008e173ec825 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -392,6 +392,11 @@  config MTK_THERMAL
 	  Enable this option if you want to have support for thermal management
 	  controller present in Mediatek SoCs
 
+menu "Broadcom thermal drivers"
+depends on ARCH_BCM || COMPILE_TEST
+source "drivers/thermal/broadcom/Kconfig"
+endmenu
+
 menu "Texas Instruments thermal drivers"
 depends on ARCH_HAS_BANDGAP || COMPILE_TEST
 depends on HAS_IOMEM
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 7adae2029355..549d81b6363c 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -27,6 +27,7 @@  thermal_sys-$(CONFIG_CLOCK_THERMAL)	+= clock_cooling.o
 thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
 
 # platform thermal drivers
+obj-y				+= broadcom/
 obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
 obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
 obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
diff --git a/drivers/thermal/broadcom/Kconfig b/drivers/thermal/broadcom/Kconfig
new file mode 100644
index 000000000000..39b92a1f3584
--- /dev/null
+++ b/drivers/thermal/broadcom/Kconfig
@@ -0,0 +1,7 @@ 
+config BCM_NS_THERMAL
+	tristate "Northstar thermal driver"
+	depends on ARCH_BCM_IPROC || COMPILE_TEST
+	help
+	  Northstar's DMU (Device Management Unit) block contains a thermal
+	  sensor that allows checking CPU temperature. This driver provides
+	  support for it.
diff --git a/drivers/thermal/broadcom/Makefile b/drivers/thermal/broadcom/Makefile
new file mode 100644
index 000000000000..059df9a0ed69
--- /dev/null
+++ b/drivers/thermal/broadcom/Makefile
@@ -0,0 +1 @@ 
+obj-$(CONFIG_BCM_NS_THERMAL)		+= ns-thermal.o
diff --git a/drivers/thermal/broadcom/ns-thermal.c b/drivers/thermal/broadcom/ns-thermal.c
new file mode 100644
index 000000000000..acf3a4de62a4
--- /dev/null
+++ b/drivers/thermal/broadcom/ns-thermal.c
@@ -0,0 +1,98 @@ 
+/*
+ * Copyright (C) 2017 Rafał Miłecki <rafal@milecki.pl>
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/thermal.h>
+
+#define PVTMON_CONTROL0					0x00
+#define PVTMON_CONTROL0_SEL_MASK			0x0000000e
+#define PVTMON_CONTROL0_SEL_TEMP_MONITOR		0x00000000
+#define PVTMON_CONTROL0_SEL_TEST_MODE			0x0000000e
+#define PVTMON_STATUS					0x08
+
+struct ns_thermal {
+	void __iomem *pvtmon;
+};
+
+static int ns_thermal_get_temp(void *data, int *temp)
+{
+	struct ns_thermal *ns_thermal = data;
+	u32 val;
+
+	val = readl(ns_thermal->pvtmon + PVTMON_CONTROL0);
+	if ((val & PVTMON_CONTROL0_SEL_MASK) != PVTMON_CONTROL0_SEL_TEMP_MONITOR) {
+		val &= ~PVTMON_CONTROL0_SEL_MASK;
+		val |= PVTMON_CONTROL0_SEL_TEMP_MONITOR;
+		writel(val, ns_thermal->pvtmon + PVTMON_CONTROL0);
+	}
+
+	val = readl(ns_thermal->pvtmon + PVTMON_STATUS);
+	*temp = (418 - (val * 5556 / 10000)) * 1000;
+
+	return 0;
+}
+
+const struct thermal_zone_of_device_ops ns_thermal_ops = {
+	.get_temp = ns_thermal_get_temp,
+};
+
+static int ns_thermal_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ns_thermal *ns_thermal;
+	struct thermal_zone_device *tzd;
+
+	ns_thermal = devm_kzalloc(dev, sizeof(*ns_thermal), GFP_KERNEL);
+	if (!ns_thermal)
+		return -ENOMEM;
+
+	ns_thermal->pvtmon = of_iomap(dev_of_node(dev), 0);
+	if (WARN_ON(!ns_thermal->pvtmon))
+		return -ENOENT;
+
+	tzd = devm_thermal_zone_of_sensor_register(dev, 0, ns_thermal,
+						   &ns_thermal_ops);
+	if (IS_ERR(tzd)) {
+		iounmap(ns_thermal->pvtmon);
+		return PTR_ERR(tzd);
+	}
+
+	platform_set_drvdata(pdev, ns_thermal);
+
+	return 0;
+}
+
+static int ns_thermal_remove(struct platform_device *pdev)
+{
+	struct ns_thermal *ns_thermal = platform_get_drvdata(pdev);
+
+	iounmap(ns_thermal->pvtmon);
+
+	return 0;
+}
+
+static const struct of_device_id ns_thermal_of_match[] = {
+	{ .compatible = "brcm,ns-thermal", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ns_thermal_of_match);
+
+static struct platform_driver ns_thermal_driver = {
+	.probe		= ns_thermal_probe,
+	.remove		= ns_thermal_remove,
+	.driver = {
+		.name = "ns-thermal",
+		.of_match_table = ns_thermal_of_match,
+	},
+};
+module_platform_driver(ns_thermal_driver);
+
+MODULE_DESCRIPTION("Northstar thermal driver");
+MODULE_LICENSE("GPL v2");