Re: [PATCH RESEND v4 2/2] thermal: hisilicon: add new hisilicon thermal sensor driver

[Xinwei Kong <kong.kongxinwei-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>]

On 2015/5/12 9:29, Eduardo Valentin wrote:
> Dear Kongxinwei,
> 
> 
> Apologize for late answer. I really missed this one. Let's make it for
> the next merge window!
> 
> Please find a couple of comments as follows.
> 
> 
> On Mon, May 04, 2015 at 10:52:20AM +0800, Xinwei Kong wrote:
>> From: kongxinwei <kong.kongxinwei-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
>>
>> This patch adds the support for hisilicon thermal sensor, within
>> hisilicon SoC. there will register sensors for thermal framework
>> and use device tree to bind cooling device.
>>
>> +
>> +static void hisi_thermal_enable_sensor(struct hisi_thermal_data *data)
>> +{
>> +	hisi_thermal_bind_irq(data);
>> +}
>> +
>> +static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
>> +{
>> +
>> +	mutex_lock(&data->thermal_lock);
>> +
>> +	/* disable sensor module */
>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>> +	writel(0x0, data->regs + TEMP0_EN);
>> +
>> +	mutex_unlock(&data->thermal_lock);
>> +
> 
> nit: remove empty line.
> 
>> +}
>> +
>> +static int hisi_thermal_get_temp(void *_sensor, long *temp)
>> +{
>> +	struct hisi_thermal_sensor *sensor = _sensor;
>> +	struct hisi_thermal_data *data = sensor->thermal;
>> +
>> +	int sensor_id = 0, i;
>> +	long max_temp = 0;
>> +
>> +	*temp = hisi_thermal_get_sensor_temp(data, sensor);
>> +
>> +	data->sensor_temp[sensor->id] = *temp;
>> +
>> +	for (i = 0; i < HISI_MAX_SENSORS; i++) {
>> +		if (data->sensor_temp[i] >= max_temp) {
>> +			max_temp = data->sensor_temp[i];
>> +			sensor_id = i;
>> +		}
>> +	}
>> +
>> +	data->irq_bind_sensor = sensor_id;
> 
> I believe this irq_bind_sensor needs to be protected by a lock.
> 
> You read it in t_irq context, write it here while getting temp, and read
> it in bind irq.

OK, add mutex lock to protected this variable.

> 
> I still don't follow why you need it though, can you please explain?

The thermal sensor module has four sensors (local、acpu0、acpu1、gpu),
Four sensor share or use the same register. let diff sensor ip enable
by setting diff bit for the same register. For the interrupt it has
only one interrupt signal; This interrupt signal can only be used by
one sensor;

For example, Four sensor can generate interrupt signal by selecting sensor
which use  "writel((sensor->id << 12), data->regs + TEMP0_CFG)" , when
using sensor->id value to enable diff sensor, the enabled sensor maybe
generate the interrupt signal while other three sensor don't generate the
interrupt signal.

Four sensor ip distribute diff situation in the Soc. We will use kernel to
decide the interrupt signal binding one of four sensor. However, the binding
sensor will be close to interrupt status. the variable "irq_bind_sensor" will
record this binding sensor id.

> 
> In any case, seams to be racy, given that it is shared by the four
> sensors, but you don't lock it to use it.
> 
>> +
>> +	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%ld, thres=%d\n",
>> +		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
>> +	/*
>> +	 * Bind irq to sensor for two cases:
>> +	 *   Reenable alarm IRQ if temperature below threshold;
>> +	 *   if irq has been enabled, always set it;
>> +	 */
>> +	if (data->irq_enabled) {
>> +		hisi_thermal_bind_irq(data);
>> +		return 0;
>> +	}
>> +
>> +	if (max_temp < sensor->thres_temp) {
>> +		data->irq_enabled = true;
>> +		hisi_thermal_bind_irq(data);
>> +		enable_irq(data->irq);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
>> +	.get_temp = hisi_thermal_get_temp,
>> +};
>> +
>> +static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev;
>> +
>> +	disable_irq_nosync(irq);
>> +	data->irq_enabled = false;
>> +
>> +	return IRQ_WAKE_THREAD;
>> +}
>> +
>> +static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev;
>> +	struct hisi_thermal_sensor *sensor;
>> +	int i;
>> +
>> +	sensor = &data->sensors[data->irq_bind_sensor];
>> +
>> +	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
>> +		 sensor->thres_temp / 1000);
> 
> Do you really want to be printing this every time you receive an IRQ?

This print information will display some sensor which generate the interrupt
signal. If deleting it,We don't know which sensor generate interrupt signal.

Generating interruption frequency is low. When the temperature of binding
sensor is higher than "thres_temp" value, this will print it. But in the thermal
framework it will get temperature by polling sensor, then cooling this device.
During polling time the sensor temperature is higher than "thres_temp" value,it
can generate interrupt signal. The interrupt dealing is helpful to the polling
machine.

> 
>> +
>> +	for (i = 0; i < data->sensors_num; i++)
>> +		thermal_zone_device_update(data->sensors[i].tzd);
> 
> 
> Can you please educate me on how this chip works?

This chip have four sensors, every time you can only use one sensor by selecting
sensor register. At the same time one sensor is regular work, you require to
set the same register for the diff bit.

> 
> You receive a thermal alarm from the chip, does it really  mean you have
> to update all thermal zones? Or do you have a way to check which sensor
> generated the alarm irq?
> 

If one sensor generate interrupt signal, it will update all thermal zones.

>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int hisi_thermal_register_sensor(struct platform_device *pdev,
>> +					struct hisi_thermal_data *data,
>> +					struct hisi_thermal_sensor *sensor,
>> +					int index)
>> +{
>> +	int ret, i;
>> +
>> +	sensor->id = index;
>> +	sensor->thermal = data;
>> +
>> +	sensor->tzd = thermal_zone_of_sensor_register(&pdev->dev, sensor->id,
>> +				sensor, &hisi_of_thermal_ops);
>> +	if (IS_ERR(sensor->tzd)) {
>> +		ret = PTR_ERR(sensor->tzd);
>> +		dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
>> +			sensor->id, ret);
>> +		return ret;
>> +	}
>> +
>> +	sensor->trip = of_thermal_get_trip_points(sensor->tzd);
>> +
>> +	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
>> +		if (sensor->trip[i].type == THERMAL_TRIP_PASSIVE) {
> 
> what if you have more than one passive trip? You just care about the
> first?
> 

Yes, this "passive" type have satisfied the demands for our Soc.

>> +			sensor->thres_temp = sensor->trip[i].temperature;
> 
> Looks like you use sensor->trip only for filling thres_temp here. Does
> it make sense to use a local variable in this function, given you won't
> use sensor->trip anywhere else?
> 
good idea. I understand it. I remove this "const struct thermal_trip *trip;"
from " struct hisi_thermal_sensor". Then use it in this function.

>> +			break;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id of_hisi_thermal_match[] = {
>> +	{ .compatible = "hisilicon,tsensor" },
>> +	{ /* end */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>> +
>> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>> +				       bool on)
>> +{
>> +	struct thermal_zone_device *tzd = sensor->tzd;
>> +
>> +	tzd->ops->set_mode(tzd,
>> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>> +}
>> +
>> +static int hisi_thermal_probe(struct platform_device *pdev)
>> +{
>> +	struct hisi_thermal_data *data;
>> +	struct resource *res;
>> +	int i;
>> +	int ret;
>> +
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&data->thermal_lock);
>> +	data->pdev = pdev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(data->regs)) {
>> +		dev_err(&pdev->dev, "failed to get io address\n");
>> +		return PTR_ERR(data->regs);
>> +	}
>> +
>> +	data->irq = platform_get_irq(pdev, 0);
>> +	if (data->irq < 0)
>> +		return data->irq;
>> +
>> +	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>> +			hisi_thermal_alarm_irq, hisi_thermal_alarm_irq_thread,
>> +			0, "hisi_thermal", data);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, data);
>> +
>> +	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
>> +	if (IS_ERR(data->clk)) {
>> +		ret = PTR_ERR(data->clk);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(&pdev->dev,
>> +				"failed to get thermal clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* enable clock for thermal */
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
>> +		ret = hisi_thermal_register_sensor(pdev, data,
>> +					&data->sensors[i], i);
>> +		if (ret) {
>> +			dev_err(&pdev->dev,
>> +			"failed to register thermal sensor: %d\n", ret);
>> +			goto err_get_sensor_data;
>> +		}
>> +	}
>> +
>> +	hisi_thermal_enable_sensor(data);
>> +	data->irq_enabled = true;
>> +
>> +	for (i = 0; i < data->sensors_num; i++)
>> +		hisi_thermal_toggle_sensor(&data->sensors[i], true);
>> +
>> +	return 0;
>> +
>> +err_get_sensor_data:
>> +	clk_disable_unprepare(data->clk);
> 
> nit: empty line here to follow your pattern.
> 
>> +	return ret;
>> +}
>> +
>> +static int hisi_thermal_remove(struct platform_device *pdev)
>> +{
>> +	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
>> +	int i;
>> +
>> +	for (i = 0; i < data->sensors_num; i++) {
>> +		struct hisi_thermal_sensor *sensor = &data->sensors[i];
>> +
>> +		hisi_thermal_toggle_sensor(sensor, false);
>> +		thermal_zone_of_sensor_unregister(&pdev->dev, sensor->tzd);
>> +	}
>> +
>> +	hisi_thermal_disable_sensor(data);
>> +	clk_disable_unprepare(data->clk);
> 
> nit: empty line here to follow your pattern.
> 
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int hisi_thermal_suspend(struct device *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>> +
>> +	hisi_thermal_disable_sensor(data);
>> +	data->irq_enabled = false;
>> +
>> +	clk_disable_unprepare(data->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hisi_thermal_resume(struct device *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>> +
>> +	clk_prepare_enable(data->clk);
>> +
>> +	data->irq_enabled = true;
>> +	hisi_thermal_enable_sensor(data);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops,
>> +			 hisi_thermal_suspend, hisi_thermal_resume);
>> +
>> +static struct platform_driver hisi_thermal_driver = {
>> +	.driver = {
>> +		.name	= "hisi_thermal",
>> +		.owner  = THIS_MODULE,
>> +		.pm	= &hisi_thermal_pm_ops,
>> +		.of_match_table = of_hisi_thermal_match,
> 
> 
> nit: align on the '=' operator.
> 
>> +	},
>> +	.probe		= hisi_thermal_probe,
>> +	.remove		= hisi_thermal_remove,
>> +};
>> +
>> +module_platform_driver(hisi_thermal_driver);
>> +
>> +MODULE_AUTHOR("Xinwei Kong <kong.kongxinwei-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>");
>> +MODULE_AUTHOR("Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>");
>> +MODULE_DESCRIPTION("Hisilicon thermal driver");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 1.9.1
>>
>>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html