Re: [PATCH v3 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Eduardo Valentin <edubezval@gmail.com>]

Wolfram,

Thanks for sending the driver. Questions on the driver locking strategy as follows.

On Mon, Nov 28, 2016 at 10:09:22PM +0100, Wolfram Sang wrote:

No commit message ? :-( generally speaking here, it is a good practice
to describe what you are doing, what you have considered, and what you
have tested, just for the sake of code history/documentation.

> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
> Signed-off-by: Thao Nguyen <thao.nguyen.yb@rvc.renesas.com>
> Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> Changes since last version:
> 
> * removed interrupt support, needs to be refactored seperately
> 
> * adapt code to the new bindings, especially one node for all sensors.
>   Similarly, we have one private structure containing n sensor structs.
> 
> * gave register bits proper namespaces, properly seperates H3 and M3-W
> 
> * refactored the way default values are determined when there are no fuses
>   (which is default case currently)
> 
> * refactored the rounding routine
> 
> * removed some superfluous macros
> 
> * use pass-by-reference instead of pass-by-value where apropriate
> 
> * removed BSPisms here and there
> 
>  drivers/thermal/Kconfig             |   9 +
>  drivers/thermal/Makefile            |   1 +
>  drivers/thermal/rcar_gen3_thermal.c | 347 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 357 insertions(+)
>  create mode 100644 drivers/thermal/rcar_gen3_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index c2c056cc7ea52e..3912d24a07b10f 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -245,6 +245,15 @@ config RCAR_THERMAL
>  	  Enable this to plug the R-Car thermal sensor driver into the Linux
>  	  thermal framework.
>  
> +config RCAR_GEN3_THERMAL
> +	tristate "Renesas R-Car Gen3 thermal driver"
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	depends on OF
> +	help
> +	  Enable this to plug the R-Car Gen3 thermal sensor driver into the Linux
> +	  thermal framework.
> +
>  config KIRKWOOD_THERMAL
>  	tristate "Temperature sensor on Marvell Kirkwood SoCs"
>  	depends on MACH_KIRKWOOD || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index c92eb22a41ff89..1216fb31ed4036 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -30,6 +30,7 @@ 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
>  obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
> +obj-$(CONFIG_RCAR_GEN3_THERMAL)	+= rcar_gen3_thermal.o
>  obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
>  obj-y				+= samsung/
>  obj-$(CONFIG_DOVE_THERMAL)  	+= dove_thermal.o
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> new file mode 100644
> index 00000000000000..6e162220672884
> --- /dev/null
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -0,0 +1,347 @@
> +/*
> + *  R-Car Gen3 THS thermal sensor driver
> + *  Based on rcar_thermal.c and work from Hien Dang and Khiem Nguyen.
> + *
> + * Copyright (C) 2016 Renesas Electronics Corporation.
> + * Copyright (C) 2016 Sang Engineering
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  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.
> + *
> + */
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/spinlock.h>
> +#include <linux/thermal.h>
> +
> +/* Register offsets */
> +#define REG_GEN3_IRQSTR		0x04
> +#define REG_GEN3_IRQMSK		0x08
> +#define REG_GEN3_IRQCTL		0x0C
> +#define REG_GEN3_IRQEN		0x10
> +#define REG_GEN3_IRQTEMP1	0x14
> +#define REG_GEN3_IRQTEMP2	0x18
> +#define REG_GEN3_IRQTEMP3	0x1C
> +#define REG_GEN3_CTSR		0x20
> +#define REG_GEN3_THCTR		0x20
> +#define REG_GEN3_TEMP		0x28
> +#define REG_GEN3_THCODE1	0x50
> +#define REG_GEN3_THCODE2	0x54
> +#define REG_GEN3_THCODE3	0x58
> +
> +/* CTSR bits */
> +#define CTSR_PONM	BIT(8)
> +#define CTSR_AOUT	BIT(7)
> +#define CTSR_THBGR	BIT(5)
> +#define CTSR_VMEN	BIT(4)
> +#define CTSR_VMST	BIT(1)
> +#define CTSR_THSST	BIT(0)
> +
> +/* THCTR bits */
> +#define THCTR_PONM	BIT(6)
> +#define THCTR_THSST	BIT(0)
> +
> +#define CTEMP_MASK	0xFFF
> +
> +#define MCELSIUS(temp)	((temp) * 1000)
> +#define GEN3_FUSE_MASK	0xFFF
> +
> +#define TSC_MAX_NUM	3
> +
> +/* Structure for thermal temperature calculation */
> +struct equation_coefs {
> +	long a1;
> +	long b1;
> +	long a2;
> +	long b2;
> +};
> +
> +struct rcar_gen3_thermal_tsc {
> +	void __iomem *base;
> +	struct thermal_zone_device *zone;
> +	struct equation_coefs coef;
> +	spinlock_t lock;
> +	u32 ctemp;
> +};
> +
> +struct rcar_gen3_thermal_priv {
> +	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
> +};
> +
> +struct rcar_gen3_thermal_data {
> +	void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
> +};
> +
> +/* Temperature calculation  */
> +#define RCAR3_THERMAL_GRAN 500
> +#define CODETSD(x) ((x) * 1000)
> +#define TJ_1 96000L
> +#define TJ_3 (-41000L)
> +
> +static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc, u32 reg)
> +{
> +	return ioread32(tsc->base + reg);
> +}
> +
> +static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
> +				     u32 reg, u32 data)
> +{
> +	iowrite32(data, tsc->base + reg);
> +}
> +
> +static int _round_temp(int temp)
> +{
> +	int result, round_offs;
> +
> +	round_offs = temp >= 0 ? RCAR3_THERMAL_GRAN / 2 : -RCAR3_THERMAL_GRAN / 2;
> +	result = (temp + round_offs) / RCAR3_THERMAL_GRAN;
> +	return result * RCAR3_THERMAL_GRAN;
> +}
> +
> +static void _linear_coefficient_calculation(struct rcar_gen3_thermal_tsc *tsc,
> +					    int *ptat, int *thcode)
> +{
> +	int tj_2;
> +	long a1, b1;
> +	long a2, b2;
> +	long a1_num, a1_den;
> +	long a2_num, a2_den;
> +
> +	tj_2 = (CODETSD((ptat[1] - ptat[2]) * 137)
> +		/ (ptat[0] - ptat[2])) - CODETSD(41);
> +
> +	/* calculate coefficients for linear equation */
> +	a1_num = CODETSD(thcode[1] - thcode[2]);
> +	a1_den = tj_2 - TJ_3;
> +	a1 = (10000 * a1_num) / a1_den;
> +	b1 = (10000 * thcode[2]) - ((a1 * TJ_3) / 1000);
> +
> +	a2_num = CODETSD(thcode[1] - thcode[0]);
> +	a2_den = tj_2 - TJ_1;
> +	a2 = (10000 * a2_num) / a2_den;
> +	b2 = (10000 * thcode[0]) - ((a2 * TJ_1) / 1000);
> +
> +	tsc->coef.a1 = DIV_ROUND_CLOSEST(a1, 10);
> +	tsc->coef.b1 = DIV_ROUND_CLOSEST(b1, 10);
> +	tsc->coef.a2 = DIV_ROUND_CLOSEST(a2, 10);
> +	tsc->coef.b2 = DIV_ROUND_CLOSEST(b2, 10);
> +}
> +
> +static int _linear_temp_converter(struct equation_coefs *coef,
> +					int temp_code)
> +{
> +	int temp, temp1, temp2;
> +
> +	temp1 = MCELSIUS((CODETSD(temp_code) - coef->b1)) / coef->a1;
> +	temp2 = MCELSIUS((CODETSD(temp_code) - coef->b2)) / coef->a2;
> +	temp = (temp1 + temp2) / 2;
> +
> +	return _round_temp(temp);
> +}
> +
> +static int rcar_gen3_thermal_update_temp(struct rcar_gen3_thermal_tsc *tsc)
> +{
> +	u32 ctemp;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tsc->lock, flags);

Why do you need a full blown spin lock irqsave? Can this locking be
solved by a simple mutex?

> +	ctemp = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> +	tsc->ctemp = ctemp;
> +	spin_unlock_irqrestore(&tsc->lock, flags);

I see it is a very short critical section, but still, do you really need
to disable irqs to read this sensor?

> +
> +	return 0;
> +}
> +
> +static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
> +{
> +	struct rcar_gen3_thermal_tsc *tsc = devdata;
> +	int ctemp;
> +	unsigned long flags;
> +
> +	rcar_gen3_thermal_update_temp(tsc);
> +
> +	spin_lock_irqsave(&tsc->lock, flags);
> +	ctemp = _linear_temp_converter(&tsc->coef, tsc->ctemp);
> +	spin_unlock_irqrestore(&tsc->lock, flags);
> +
> +	if ((ctemp < MCELSIUS(-40)) || (ctemp > MCELSIUS(125)))
> +		return -EIO;
> +
> +	*temp = ctemp;
> +
> +	return 0;
> +}
> +
> +static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
> +	.get_temp	= rcar_gen3_thermal_get_temp,
> +};
> +
> +static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tsc->lock, flags);
> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  0x0);
> +
> +	udelay(1000);

Why do you disable irqs, then busyloop for 1ms?

> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR, CTSR_PONM);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
> +			   CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN);
> +
> +	udelay(100);

1.1ms?

> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
> +			  CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN |
> +			  CTSR_VMST | CTSR_THSST);
> +
> +	spin_unlock_irqrestore(&tsc->lock, flags);

again, why not a simpler way, by using a mutex?

> +}
> +
> +static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> +{
> +	unsigned long flags;
> +	u32 reg_val;
> +
> +	spin_lock_irqsave(&tsc->lock, flags);
> +
> +	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
> +	reg_val &= ~THCTR_PONM;
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
> +
> +	udelay(1000);
> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
> +	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
> +	reg_val |= THCTR_THSST;
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
> +
> +	spin_unlock_irqrestore(&tsc->lock, flags);
>

 ditto

+}
> +
> +static const struct rcar_gen3_thermal_data r8a7795_data = {
> +	.thermal_init = r8a7795_thermal_init,
> +};
> +
> +static const struct rcar_gen3_thermal_data r8a7796_data = {
> +	.thermal_init = r8a7796_thermal_init,
> +};
> +
> +static const struct of_device_id rcar_gen3_thermal_dt_ids[] = {
> +	{ .compatible = "renesas,r8a7795-thermal", .data = &r8a7795_data},
> +	{ .compatible = "renesas,r8a7796-thermal", .data = &r8a7796_data},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rcar_gen3_thermal_dt_ids);
> +
> +static int rcar_gen3_thermal_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	pm_runtime_put(dev);
> +	pm_runtime_disable(dev);
> +
> +	return 0;
> +}
> +
> +static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> +{
> +	struct rcar_gen3_thermal_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct thermal_zone_device *zone;
> +	int ret, i;
> +	const struct rcar_gen3_thermal_data *match_data = of_device_get_match_data(dev);
> +
> +	/* default values if FUSEs are missing */
> +	int ptat[3] = { 2351, 1509, 435 };
> +	int thcode[TSC_MAX_NUM][3] = {
> +		{ 3248, 2800, 2221 },
> +		{ 3245, 2795, 2216 },
> +		{ 3250, 2805, 2237 },
> +	};

are these fuses valid for both? 7796 and 7797? or would you need a
different array for each version?

> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +
> +	for (i = 0; i < TSC_MAX_NUM; i++) {
> +		struct rcar_gen3_thermal_tsc *tsc;
> +
> +		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> +		if (!tsc)
> +			return -ENOMEM;
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		if (!res)
> +			break;
> +
> +		tsc->base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(tsc->base)) {
> +			ret = PTR_ERR(tsc->base);
> +			goto error_unregister;
> +		}
> +
> +		priv->tscs[i] = tsc;
> +		spin_lock_init(&tsc->lock);
> +
> +		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
> +					&rcar_gen3_tz_of_ops);
> +		if (IS_ERR(zone)) {
> +			dev_err(dev, "Can't register thermal zone\n");
> +			ret = PTR_ERR(zone);
> +			goto error_unregister;
> +		}
> +		tsc->zone = zone;
> +
> +		match_data->thermal_init(tsc);
> +
> +		_linear_coefficient_calculation(tsc, ptat, thcode[i]);
> +
> +		ret = rcar_gen3_thermal_update_temp(tsc);
> +
> +		if (ret < 0)
> +			goto error_unregister;
> +	}
> +
> +	return 0;
> +
> +error_unregister:
> +	rcar_gen3_thermal_remove(pdev);
> +
> +	return ret;
> +}
> +
> +static struct platform_driver rcar_gen3_thermal_driver = {
> +	.driver	= {
> +		.name	= "rcar_gen3_thermal",
> +		.of_match_table = rcar_gen3_thermal_dt_ids,
> +	},
> +	.probe		= rcar_gen3_thermal_probe,
> +	.remove		= rcar_gen3_thermal_remove,
> +};
> +module_platform_driver(rcar_gen3_thermal_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("R-Car Gen3 THS thermal sensor driver");
> +MODULE_AUTHOR("Wolfram Sang <wsa+renesas@sang-engineering.com>");
> -- 
> 2.10.2
>