| Message ID | 20170323223045.15786-1-zajec5@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Rafal, On Thu, Mar 23, 2017 at 11:30:44PM +0100, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This commit documents binding for thermal used in Northstar family SoCs. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > .../devicetree/bindings/thermal/brcm,ns-thermal | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > create mode 100644 Documentation/devicetree/bindings/thermal/brcm,ns-thermal > > diff --git a/Documentation/devicetree/bindings/thermal/brcm,ns-thermal b/Documentation/devicetree/bindings/thermal/brcm,ns-thermal > new file mode 100644 > index 000000000000..9a22dd76ec01 > --- /dev/null > +++ b/Documentation/devicetree/bindings/thermal/brcm,ns-thermal > @@ -0,0 +1,17 @@ > +* Broadcom Northstar Thermal > + > +This binding describes thermal sensor that is part of Northstar's DMU (Device > +Management Unit). > + > +Required properties: > +- compatible : Must be "brcm,ns-thermal" > +- reg : iomem address range of PVTMON registers > +- #thermal-sensor-cells : Should be <0> > + > +Example: > + > +thermal { > + compatible = "brcm,ns-thermal"; > + reg = <0x1800c2c0 0x10>; > + #thermal-sensor-cells = <0>; > +}; Could you please also include an example of the thermal zone one should include in the board/cpu DTS while using this sensor? Besides, based on what I saw you in your driver, you could also describe your offset and slope in DT, and read them using thermal helpers. BR, > -- > 2.11.0 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/thermal/brcm,ns-thermal b/Documentation/devicetree/bindings/thermal/brcm,ns-thermal new file mode 100644 index 000000000000..9a22dd76ec01 --- /dev/null +++ b/Documentation/devicetree/bindings/thermal/brcm,ns-thermal @@ -0,0 +1,17 @@ +* Broadcom Northstar Thermal + +This binding describes thermal sensor that is part of Northstar's DMU (Device +Management Unit). + +Required properties: +- compatible : Must be "brcm,ns-thermal" +- reg : iomem address range of PVTMON registers +- #thermal-sensor-cells : Should be <0> + +Example: + +thermal { + compatible = "brcm,ns-thermal"; + reg = <0x1800c2c0 0x10>; + #thermal-sensor-cells = <0>; +};