Signed-off-by: Guillaume La Roque <[email protected]>
---
drivers/thermal/Kconfig | 11 +
drivers/thermal/Makefile | 1 +
drivers/thermal/amlogic_thermal.c | 332 ++++++++++++++++++++++++++++++
3 files changed, 344 insertions(+)
create mode 100644 drivers/thermal/amlogic_thermal.c
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 9966364a6deb..0f31bb4bc372 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -348,6 +348,17 @@ config MTK_THERMAL
Enable this option if you want to have support for thermal management
controller present in Mediatek SoCs
+config AMLOGIC_THERMAL
+ tristate "Amlogic Thermal Support"
+ default ARCH_MESON
+ depends on OF && ARCH_MESON
+ help
+ If you say yes here you get support for Amlogic Thermal
+ for G12 SoC Family.
+
+ This driver can also be built as a module. If so, the module will
+ be called amlogic_thermal.
+
menu "Intel thermal drivers"
depends on X86 || X86_INTEL_QUARK || COMPILE_TEST
source "drivers/thermal/intel/Kconfig"
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 74a37c7f847a..baeb70bf0568 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -54,3 +54,4 @@ obj-$(CONFIG_MTK_THERMAL) += mtk_thermal.o
obj-$(CONFIG_GENERIC_ADC_THERMAL) += thermal-generic-adc.o
obj-$(CONFIG_ZX2967_THERMAL) += zx2967_thermal.o
obj-$(CONFIG_UNIPHIER_THERMAL) += uniphier_thermal.o
+obj-$(CONFIG_AMLOGIC_THERMAL) += amlogic_thermal.o
diff --git a/drivers/thermal/amlogic_thermal.c b/drivers/thermal/amlogic_thermal.c
new file mode 100644
index 000000000000..13cd4c42721a
--- /dev/null
+++ b/drivers/thermal/amlogic_thermal.c
@@ -0,0 +1,332 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Amlogic Meson Thermal Sensor Driver
+ *
+ * Copyright (C) 2017 Huan Biao <[email protected]>
+ * Copyright (C) 2019 Guillaume La Roque <[email protected]>
+ *
+ * Register value to celsius temperature formulas:
+ * Read_Val m * U
+ * U = ---------, Uptat = ---------
+ * 2^16 1 + n * U
+ *
+ * Temperature = A * ( Uptat + u_efuse / 2^16 )- B
+ *
+ * A B m n : calibration parameters
+ * u_efuse : fused calibration value, it's a signed 16 bits value
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/thermal.h>
+
+#include "thermal_core.h"
+
+#define TSENSOR_CFG_REG1 0x4
+ #define TSENSOR_CFG_REG1_RSET_VBG BIT(12)
+ #define TSENSOR_CFG_REG1_RSET_ADC BIT(11)
+ #define TSENSOR_CFG_REG1_VCM_EN BIT(10)
+ #define TSENSOR_CFG_REG1_VBG_EN BIT(9)
+ #define TSENSOR_CFG_REG1_OUT_CTL BIT(6)
+ #define TSENSOR_CFG_REG1_FILTER_EN BIT(5)
+ #define TSENSOR_CFG_REG1_DEM_EN BIT(3)
+ #define TSENSOR_CFG_REG1_CH_SEL GENMASK(1, 0)
+ #define TSENSOR_CFG_REG1_ENABLE \
+ (TSENSOR_CFG_REG1_FILTER_EN | \
+ TSENSOR_CFG_REG1_VCM_EN | \
+ TSENSOR_CFG_REG1_VBG_EN | \
+ TSENSOR_CFG_REG1_DEM_EN | \
+ TSENSOR_CFG_REG1_CH_SEL)
+
+#define TSENSOR_STAT0 0x40
+
+#define TSENSOR_STAT9 0x64
+
+#define TSENSOR_READ_TEMP_MASK GENMASK(15, 0)
+#define TSENSOR_TEMP_MASK GENMASK(11, 0)
+
+#define TSENSOR_TRIM_SIGN_MASK BIT(15)
+#define TSENSOR_TRIM_TEMP_MASK GENMASK(14, 0)
+#define TSENSOR_TRIM_VERSION_MASK GENMASK(31, 24)
+
+#define TSENSOR_TRIM_VERSION(_version) \
+ FIELD_GET(TSENSOR_TRIM_VERSION_MASK, _version)
+
+#define TSENSOR_TRIM_CALIB_VALID_MASK (GENMASK(3, 2) | BIT(7))
+
+#define TSENSOR_CALIB_OFFSET 1
+#define TSENSOR_CALIB_SHIFT 4
+
+/**
+ * struct amlogic_thermal_soc_data
+ * @A, B, m, n: calibration parameters
+ * This structure is required for configuration of amlogic thermal driver.
+ */
+struct amlogic_thermal_soc_data {
+ int A;
+ int B;
+ int m;
+ int n;
+};
+
+/**
+ * struct amlogic_thermal_data
+ * @u_efuse_off: register offset to read fused calibration value
+ * @soc: calibration parameters structure pointer
+ * @regmap_config: regmap config for the device
+ * This structure is required for configuration of amlogic thermal driver.
+ */
+struct amlogic_thermal_data {
+ int u_efuse_off;
+ const struct amlogic_thermal_soc_data *soc;
+ const struct regmap_config *regmap_config;
+};
+
+struct amlogic_thermal {
+ struct platform_device *pdev;
+ const struct amlogic_thermal_data *data;
+ struct regmap *regmap;
+ struct regmap *sec_ao_map;
+ struct clk *clk;
+ struct thermal_zone_device *tzd;
+ u32 trim_info;
+ void __iomem *base;
+};
+
+/*
+ * Calculate a temperature value from a temperature code.
+ * The unit of the temperature is degree Celsius.
+ */
+static int code_to_temp(struct amlogic_thermal *pdata, int temp_code)
+{
+ const struct amlogic_thermal_soc_data *param = pdata->data->soc;
+ int temp;
+ s64 factor, Uptat, uefuse;
+
+ uefuse = pdata->trim_info & TSENSOR_TRIM_SIGN_MASK ?
+ ~(pdata->trim_info & TSENSOR_TRIM_TEMP_MASK) + 1 :
+ (pdata->trim_info & TSENSOR_TRIM_TEMP_MASK);
+
+ factor = param->n * temp_code;
+ factor = div_s64(factor, 100);
+
+ Uptat = temp_code * param->m;
+ Uptat = div_s64(Uptat, 100);
+ Uptat = Uptat * BIT(16);
+ Uptat = div_s64(Uptat, BIT(16) + factor);
+
+ temp = (Uptat + uefuse) * param->A;
+ temp = div_s64(temp, BIT(16));
+ temp = (temp - param->B) * 100;
+
+ return temp;
+}
+
+static int amlogic_thermal_initialize(struct amlogic_thermal *pdata)
+{
+ int ret = 0;
+ int ver;
+
+ regmap_read(pdata->sec_ao_map, pdata->data->u_efuse_off,
+ &pdata->trim_info);
+
+ ver = TSENSOR_TRIM_VERSION(pdata->trim_info);
+
+ if ((ver & TSENSOR_TRIM_CALIB_VALID_MASK) == 0) {
+ ret = -EINVAL;
+ dev_err(&pdata->pdev->dev,
+ "tsensor thermal calibration not supported: 0x%x!\n",
+ ver);
+ }
+
+ return ret;
+}
+
+static int amlogic_thermal_enable(struct amlogic_thermal *data)
+{
+ clk_prepare_enable(data->clk);
+ regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
+ TSENSOR_CFG_REG1_ENABLE, TSENSOR_CFG_REG1_ENABLE);
+
+ return 0;
+}
+
+static int amlogic_thermal_disable(struct amlogic_thermal *data)
+{
+ regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
+ TSENSOR_CFG_REG1_ENABLE, 0);
+ clk_disable(data->clk);
+
+ return 0;
+}
+
+static int amlogic_thermal_get_temp(void *data, int *temp)
+{
+ unsigned int tvalue;
+ struct amlogic_thermal *pdata = data;
+
+ if (!data)
+ return -EINVAL;
+
+ regmap_read(pdata->regmap, TSENSOR_STAT0, &tvalue);
+ *temp = code_to_temp(pdata,
+ tvalue & TSENSOR_READ_TEMP_MASK);
+
+ return 0;
+}
+
+static const struct thermal_zone_of_device_ops amlogic_thermal_ops = {
+ .get_temp = amlogic_thermal_get_temp,
+};
+
+static const struct regmap_config amlogic_thermal_regmap_config_g12 = {
+ .reg_bits = 8,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .max_register = TSENSOR_STAT9,
+};
+
+static const struct amlogic_thermal_soc_data amlogic_thermal_g12 = {
+ .A = 9411,
+ .B = 3159,
+ .m = 424,
+ .n = 324,
+};
+
+static const struct amlogic_thermal_data amlogic_thermal_g12_cpu_param = {
+ .u_efuse_off = 0x128,
+ .soc = &amlogic_thermal_g12,
+ .regmap_config = &amlogic_thermal_regmap_config_g12,
+};
+
+static const struct amlogic_thermal_data amlogic_thermal_g12_ddr_param = {
+ .u_efuse_off = 0xF0,
+ .soc = &amlogic_thermal_g12,
+ .regmap_config = &amlogic_thermal_regmap_config_g12,
+};
+
+static const struct of_device_id of_amlogic_thermal_match[] = {
+ {
+ .compatible = "amlogic,g12-ddr-thermal",
+ .data = &amlogic_thermal_g12_ddr_param,
+ },
+ {
+ .compatible = "amlogic,g12-cpu-thermal",
+ .data = &amlogic_thermal_g12_cpu_param,
+ },
+ { /* end */ }
+};
+MODULE_DEVICE_TABLE(of, of_amlogic_thermal_match);
+
+static int amlogic_thermal_probe(struct platform_device *pdev)
+{
+ struct amlogic_thermal *pdata;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ int ret;
+
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ pdata->data = of_device_get_match_data(dev);
+ pdata->pdev = pdev;
+ platform_set_drvdata(pdev, pdata);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pdata->base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(pdata->base)) {
+ dev_err(dev, "failed to get io address\n");
+ return PTR_ERR(pdata->base);
+ }
+
+ pdata->regmap = devm_regmap_init_mmio(dev, pdata->base,
+ pdata->data->regmap_config);
+ if (IS_ERR(pdata->regmap))
+ return PTR_ERR(pdata->regmap);
+
+ pdata->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(pdata->clk)) {
+ if (PTR_ERR(pdata->clk) != -EPROBE_DEFER)
+ dev_err(dev, "failed to get clock\n");
+ return PTR_ERR(pdata->clk);
+ }
+
+ pdata->sec_ao_map = syscon_regmap_lookup_by_phandle
+ (pdev->dev.of_node, "amlogic,ao-secure");
+ if (IS_ERR(pdata->sec_ao_map)) {
+ dev_err(dev, "syscon regmap lookup failed.\n");
+ return PTR_ERR(pdata->sec_ao_map);
+ }
+
+ pdata->tzd = devm_thermal_zone_of_sensor_register
+ (&pdev->dev,
+ 0,
+ pdata,
+ &amlogic_thermal_ops);
+ if (IS_ERR(pdata->tzd)) {
+ ret = PTR_ERR(pdata->tzd);
+ dev_err(dev, "Failed to register tsensor: %d\n", ret);
+ return PTR_ERR(pdata->tzd);
+ }
+
+ ret = amlogic_thermal_initialize(pdata);
+ if (ret)
+ return ret;
+
+ ret = amlogic_thermal_enable(pdata);
+ if (ret)
+ clk_unprepare(pdata->clk);
+
+ return ret;
+}
+
+static int amlogic_thermal_remove(struct platform_device *pdev)
+{
+ struct amlogic_thermal *data = platform_get_drvdata(pdev);
+
+ return amlogic_thermal_disable(data);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int amlogic_thermal_suspend(struct device *dev)
+{
+ struct amlogic_thermal *data = dev_get_drvdata(dev);
+
+ return amlogic_thermal_disable(data);
+}
+
+static int amlogic_thermal_resume(struct device *dev)
+{
+ struct amlogic_thermal *data = dev_get_drvdata(dev);
+
+ return amlogic_thermal_enable(data);
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(amlogic_thermal_pm_ops,
+ amlogic_thermal_suspend, amlogic_thermal_resume);
+
+static struct platform_driver amlogic_thermal_driver = {
+ .driver = {
+ .name = "amlogic_thermal",
+ .pm = &amlogic_thermal_pm_ops,
+ .of_match_table = of_amlogic_thermal_match,
+ },
+ .probe = amlogic_thermal_probe,
+ .remove = amlogic_thermal_remove,
+};
+
+module_platform_driver(amlogic_thermal_driver);
+
+MODULE_AUTHOR("Guillaume La Roque <[email protected]>");
+MODULE_DESCRIPTION("Amlogic thermal driver");
+MODULE_LICENSE("GPL v2");
--
2.17.1
Hi Guillaume,
(I still don't have any experience with the thermal framework, so
below are some general comments)
On Wed, Jul 31, 2019 at 5:36 PM Guillaume La Roque
<[email protected]> wrote:
I would add a patch description here:
"
Amlogic G12A and G12B SoCs integrate two thermal sensors with the same design.
One is located close to the DDR (controller?) and the other one is
located close to the PLLs (between the CPU and GPU).
The calibration data for each of the thermal sensors instance is
stored in a different location within the AO region.
Implement reading the temperature from each thermal sensor.
The IP block has more functionality, which may be added to this driver
in the future:
- reading up to 16 stored temperature samples
- chip reset when the temperature exceeds a configurable threshold
- up to four interrupts when the temperature has risen above a
configurable threshold
- up to four interrupts when the temperature has fallen below a
configurable threshold
"
> Signed-off-by: Guillaume La Roque <[email protected]>
> ---
> drivers/thermal/Kconfig | 11 +
> drivers/thermal/Makefile | 1 +
> drivers/thermal/amlogic_thermal.c | 332 ++++++++++++++++++++++++++++++
> 3 files changed, 344 insertions(+)
> create mode 100644 drivers/thermal/amlogic_thermal.c
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 9966364a6deb..0f31bb4bc372 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -348,6 +348,17 @@ config MTK_THERMAL
> Enable this option if you want to have support for thermal management
> controller present in Mediatek SoCs
>
> +config AMLOGIC_THERMAL
we typically use "MESON" in the Kconfig symbols:
$ grep -c AMLOGIC .config
1
$ grep -c MESON .config
33
I also wonder if we should add G12 or G12A so we don't conflict with
upcoming thermal sensors with a different design (assuming that this
will be a thing).
for example we already have three different USB2 PHY drivers
[...]
> +/*
> + * Calculate a temperature value from a temperature code.
> + * The unit of the temperature is degree Celsius.
is it really degree Celsius or millicelsius?
> + */
> +static int code_to_temp(struct amlogic_thermal *pdata, int temp_code)
PREFIX_thermal_code_to_millicelsius?
[...]
> +static int amlogic_thermal_enable(struct amlogic_thermal *data)
> +{
> + clk_prepare_enable(data->clk);
no clock error handling?
> + regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
> + TSENSOR_CFG_REG1_ENABLE, TSENSOR_CFG_REG1_ENABLE);
> +
> + return 0;
> +}
> +
> +static int amlogic_thermal_disable(struct amlogic_thermal *data)
> +{
> + regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
> + TSENSOR_CFG_REG1_ENABLE, 0);
> + clk_disable(data->clk);
either clk_disable_unprepare here or use only clk_enable in
amlogic_thermal_enable and move prepare/unprepare somewhere else
> +
> + return 0;
> +}
> +
> +static int amlogic_thermal_get_temp(void *data, int *temp)
> +{
> + unsigned int tvalue;
> + struct amlogic_thermal *pdata = data;
> +
> + if (!data)
> + return -EINVAL;
> +
> + regmap_read(pdata->regmap, TSENSOR_STAT0, &tvalue);
> + *temp = code_to_temp(pdata,
> + tvalue & TSENSOR_READ_TEMP_MASK);
maybe simply move the implementation from code_to_temp here?
[...]
> +static const struct amlogic_thermal_data amlogic_thermal_g12_cpu_param = {
> + .u_efuse_off = 0x128,
> + .soc = &amlogic_thermal_g12,
based on the variable name I expected this to be an enum of some sort.
I would have expected it to be called calibration_parameters or
similar to match the explanation in the driver description
(no need to change it if you prefer it like this, I just want to make
you aware of this)
> + .regmap_config = &amlogic_thermal_regmap_config_g12,
if regmap_config is always the same you may also pass it directly to
devm_regmap_init_mmio
> +};
> +
> +static const struct amlogic_thermal_data amlogic_thermal_g12_ddr_param = {
> + .u_efuse_off = 0xF0,
I believe we use lower-case hex everywhere else
[...]
> +static const struct of_device_id of_amlogic_thermal_match[] = {
> + {
> + .compatible = "amlogic,g12-ddr-thermal",
> + .data = &amlogic_thermal_g12_ddr_param,
> + },
> + {
> + .compatible = "amlogic,g12-cpu-thermal",
> + .data = &amlogic_thermal_g12_cpu_param,
> + },
> + { /* end */ }
other drivers use "sentinel", but personally I have no preference here
[...]
> + pdata->tzd = devm_thermal_zone_of_sensor_register
> + (&pdev->dev,
> + 0,
> + pdata,
> + &amlogic_thermal_ops);
I believe the opening brace has to go onto the same line as the function name
[...]
> + ret = amlogic_thermal_enable(pdata);
> + if (ret)
> + clk_unprepare(pdata->clk);
clk_disable_unprepare, else you'll leave the clock prepared
> +#ifdef CONFIG_PM_SLEEP
> +static int amlogic_thermal_suspend(struct device *dev)
> +{
> + struct amlogic_thermal *data = dev_get_drvdata(dev);
> +
> + return amlogic_thermal_disable(data);
> +}
> +
> +static int amlogic_thermal_resume(struct device *dev)
> +{
> + struct amlogic_thermal *data = dev_get_drvdata(dev);
> +
> + return amlogic_thermal_enable(data);
> +}
> +#endif
instead of using an #ifdef here annotate the suspend/resume functions
with __maybe_unused, see [0]
Martin
[0] https://lore.kernel.org/patchwork/patch/732981/
Hi Martin,
again thanks for your review.
On 8/3/19 8:24 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> (I still don't have any experience with the thermal framework, so
> below are some general comments)
>
> On Wed, Jul 31, 2019 at 5:36 PM Guillaume La Roque
> <[email protected]> wrote:
> I would add a patch description here:
> "
> Amlogic G12A and G12B SoCs integrate two thermal sensors with the same design.
> One is located close to the DDR (controller?) and the other one is
> located close to the PLLs (between the CPU and GPU).
>
> The calibration data for each of the thermal sensors instance is
> stored in a different location within the AO region.
>
> Implement reading the temperature from each thermal sensor.
>
> The IP block has more functionality, which may be added to this driver
> in the future:
> - reading up to 16 stored temperature samples
it's not working, you can verify it if you check the regmap define in the driver. in fact temp is only write in one register, it's confirmed by amlogic.
> - chip reset when the temperature exceeds a configurable threshold
> - up to four interrupts when the temperature has risen above a
> configurable threshold
> - up to four interrupts when the temperature has fallen below a
> configurable threshold
> "
agreed with you to add description and optional without 16 register part.
>
>> Signed-off-by: Guillaume La Roque <[email protected]>
>> ---
>> drivers/thermal/Kconfig | 11 +
>> drivers/thermal/Makefile | 1 +
>> drivers/thermal/amlogic_thermal.c | 332 ++++++++++++++++++++++++++++++
>> 3 files changed, 344 insertions(+)
>> create mode 100644 drivers/thermal/amlogic_thermal.c
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 9966364a6deb..0f31bb4bc372 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -348,6 +348,17 @@ config MTK_THERMAL
>> Enable this option if you want to have support for thermal management
>> controller present in Mediatek SoCs
>>
>> +config AMLOGIC_THERMAL
> we typically use "MESON" in the Kconfig symbols:
> $ grep -c AMLOGIC .config
> 1
> $ grep -c MESON .config
> 33
>
> I also wonder if we should add G12 or G12A so we don't conflict with
> upcoming thermal sensors with a different design (assuming that this
> will be a thing).
> for example we already have three different USB2 PHY drivers
>
> [...]
i check with Neil and for new family it's better to use Amlogic instead of meson.
i don't add G12 because we already know it's same sensors for SM1 SoC family [0].
>> +/*
>> + * Calculate a temperature value from a temperature code.
>> + * The unit of the temperature is degree Celsius.
> is it really degree Celsius or millicelsius?
good point it's in millicelsius, i will fix all
>
>> + */
>> +static int code_to_temp(struct amlogic_thermal *pdata, int temp_code)
> PREFIX_thermal_code_to_millicelsius?
ok
> [...]
>> +static int amlogic_thermal_enable(struct amlogic_thermal *data)
>> +{
>> + clk_prepare_enable(data->clk);
> no clock error handling?
i will fix
>
>> + regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
>> + TSENSOR_CFG_REG1_ENABLE, TSENSOR_CFG_REG1_ENABLE);
>> +
>> + return 0;
>> +}
>> +
>> +static int amlogic_thermal_disable(struct amlogic_thermal *data)
>> +{
>> + regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
>> + TSENSOR_CFG_REG1_ENABLE, 0);
>> + clk_disable(data->clk);
> either clk_disable_unprepare here or use only clk_enable in
> amlogic_thermal_enable and move prepare/unprepare somewhere else
i will align with enable and use clk_disable_unprepare
>
>> +
>> + return 0;
>> +}
>> +
>> +static int amlogic_thermal_get_temp(void *data, int *temp)
>> +{
>> + unsigned int tvalue;
>> + struct amlogic_thermal *pdata = data;
>> +
>> + if (!data)
>> + return -EINVAL;
>> +
>> + regmap_read(pdata->regmap, TSENSOR_STAT0, &tvalue);
>> + *temp = code_to_temp(pdata,
>> + tvalue & TSENSOR_READ_TEMP_MASK);
> maybe simply move the implementation from code_to_temp here?
for the optional function it could be a problem if i move all in code_to_temp.
i prefer to have a function which are just do the conversion.
>
> [...]
>> +static const struct amlogic_thermal_data amlogic_thermal_g12_cpu_param = {
>> + .u_efuse_off = 0x128,
>> + .soc = &amlogic_thermal_g12,
> based on the variable name I expected this to be an enum of some sort.
> I would have expected it to be called calibration_parameters or
> similar to match the explanation in the driver description
> (no need to change it if you prefer it like this, I just want to make
> you aware of this)
ok to rename structure name
>
>> + .regmap_config = &amlogic_thermal_regmap_config_g12,
> if regmap_config is always the same you may also pass it directly to
> devm_regmap_init_mmio
it's the same for now but we don't know if in the future SoCs use same regmap.
>
>> +};
>> +
>> +static const struct amlogic_thermal_data amlogic_thermal_g12_ddr_param = {
>> + .u_efuse_off = 0xF0,
> I believe we use lower-case hex everywhere else
>
> [...]
will fix
>> +static const struct of_device_id of_amlogic_thermal_match[] = {
>> + {
>> + .compatible = "amlogic,g12-ddr-thermal",
>> + .data = &amlogic_thermal_g12_ddr_param,
>> + },
>> + {
>> + .compatible = "amlogic,g12-cpu-thermal",
>> + .data = &amlogic_thermal_g12_cpu_param,
>> + },
>> + { /* end */ }
> other drivers use "sentinel", but personally I have no preference here
>
> [...]
same for me i will do same as other drivers
>> + pdata->tzd = devm_thermal_zone_of_sensor_register
>> + (&pdev->dev,
>> + 0,
>> + pdata,
>> + &amlogic_thermal_ops);
> I believe the opening brace has to go onto the same line as the function name
>
> [...]
will fix
>> + ret = amlogic_thermal_enable(pdata);
>> + if (ret)
>> + clk_unprepare(pdata->clk);
> clk_disable_unprepare, else you'll leave the clock prepared
will fix
>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int amlogic_thermal_suspend(struct device *dev)
>> +{
>> + struct amlogic_thermal *data = dev_get_drvdata(dev);
>> +
>> + return amlogic_thermal_disable(data);
>> +}
>> +
>> +static int amlogic_thermal_resume(struct device *dev)
>> +{
>> + struct amlogic_thermal *data = dev_get_drvdata(dev);
>> +
>> + return amlogic_thermal_enable(data);
>> +}
>> +#endif
> instead of using an #ifdef here annotate the suspend/resume functions
> with __maybe_unused, see [0]
will fix
>
>
> Martin
>
>
> [0] https://lore.kernel.org/patchwork/patch/732981/
[0] https://lore.kernel.org/linux-arm-kernel/[email protected]/
thanks
Guillaume
Hi Guillaume,
On Mon, Aug 5, 2019 at 2:48 PM guillaume La Roque <[email protected]> wrote:
>
> Hi Martin,
>
> again thanks for your review.
you're welcome - thank you for working on the driver :-)
[...]
> > The IP block has more functionality, which may be added to this driver
> > in the future:
> > - reading up to 16 stored temperature samples
>
> it's not working, you can verify it if you check the regmap define in the driver. in fact temp is only write in one register, it's confirmed by amlogic.
I missed that - so please skip this part
[...]
> >> +config AMLOGIC_THERMAL
> > we typically use "MESON" in the Kconfig symbols:
> > $ grep -c AMLOGIC .config
> > 1
> > $ grep -c MESON .config
> > 33
> >
> > I also wonder if we should add G12 or G12A so we don't conflict with
> > upcoming thermal sensors with a different design (assuming that this
> > will be a thing).
> > for example we already have three different USB2 PHY drivers
> >
> > [...]
>
> i check with Neil and for new family it's better to use Amlogic instead of meson.
can you please share the considerations behind this decision?
if new drivers should use AMLOGIC_* Kconfig symbols instead of MESON_*
then we all should know about it
> i don't add G12 because we already know it's same sensors for SM1 SoC family [0].
my idea behind this was to avoid conflicts in the future
in case of the thermal driver we may be fine with using a generic name
assuming that Amlogic will not switch to a new IP block in the next
years
I'm not saying you have to change the name - I'm bringing this up so
you can decide for yourself based on examples from the past
here are a few examples:
- when Kevin upstreamed the MMC driver for GX he decided to use
MMC_MESON_GX for the Kconfig symbol name. it turns out that this is
smart because there are at least two other MMC controller IPs on the
32-bit SoCs. due to him including GX in the name the drivers are easy
to differentiate (MMC_MESON_MX_SDIO and MMC_MESON_MX_SDHC being the
other ones, while the latter is not upstream yet)
- when Carlo upstreamed the eFuse driver he decided to use MESON_EFUSE
for the Kconfig symbol name. I found out much later that the 32-bit
SoCs use a different IP (or at least direct register access instead of
going through Secure Monitor). the driver for the 32-bit SoCs now uses
MESON_MX_EFUSE. if you don't know which driver applies where then it's
easy to mix up MESON_EFUSE and MESON_MX_EFUSE
- when Jerome upstreamed the ALSA driver for AXG (which is also used
on G12A and G12B) he decided to use the SND_MESON_AXG_* prefix for the
Kconfig symbol names. in my opinion this was a good choice because GXM
and everything earlier (including the 32-bit SoCs) use a different
audio IP block. we won't have a Kconfig symbol name clash when a
driver for the "older" SoCs is upstreamed
- (there are more examples, Meson8b USB PHY driver, Meson8b DWMAC
glue, ... - just like there's many examples where the IP block is
mostly compatible with older generations: SAR ADC, RNG, SPI, ...)
I'm not sure what driver naming rules other mainline SoC teams use
to me it seems that the rule for Allwinner driver names is to use the
"code-name of the first SoC the IP block appeared in"
[...]
> >> +static int amlogic_thermal_get_temp(void *data, int *temp)
> >> +{
> >> + unsigned int tvalue;
> >> + struct amlogic_thermal *pdata = data;
> >> +
> >> + if (!data)
> >> + return -EINVAL;
> >> +
> >> + regmap_read(pdata->regmap, TSENSOR_STAT0, &tvalue);
> >> + *temp = code_to_temp(pdata,
> >> + tvalue & TSENSOR_READ_TEMP_MASK);
> > maybe simply move the implementation from code_to_temp here?
>
> for the optional function it could be a problem if i move all in code_to_temp.
>
> i prefer to have a function which are just do the conversion.
I didn't consider this before but you are right
if the other temperature registers (like IRQ thresholds) also use a
"temperature code" then it should be a dedicated function (so it'll be
easier to add more functionality to the driver)
Martin
Martin Blumenstingl <[email protected]> writes:
> Hi Guillaume,
>
> On Mon, Aug 5, 2019 at 2:48 PM guillaume La Roque <[email protected]> wrote:
>>
>> Hi Martin,
>>
>> again thanks for your review.
> you're welcome - thank you for working on the driver :-)
>
> [...]
>> > The IP block has more functionality, which may be added to this driver
>> > in the future:
>> > - reading up to 16 stored temperature samples
>>
>> it's not working, you can verify it if you check the regmap define in the driver. in fact temp is only write in one register, it's confirmed by amlogic.
> I missed that - so please skip this part
>
> [...]
>> >> +config AMLOGIC_THERMAL
>> > we typically use "MESON" in the Kconfig symbols:
>> > $ grep -c AMLOGIC .config
>> > 1
>> > $ grep -c MESON .config
>> > 33
>> >
>> > I also wonder if we should add G12 or G12A so we don't conflict with
>> > upcoming thermal sensors with a different design (assuming that this
>> > will be a thing).
>> > for example we already have three different USB2 PHY drivers
>> >
>> > [...]
>>
>> i check with Neil and for new family it's better to use Amlogic instead of meson.
> can you please share the considerations behind this decision?
> if new drivers should use AMLOGIC_* Kconfig symbols instead of MESON_*
> then we all should know about it
>
>> i don't add G12 because we already know it's same sensors for SM1 SoC family [0].
> my idea behind this was to avoid conflicts in the future
> in case of the thermal driver we may be fine with using a generic name
> assuming that Amlogic will not switch to a new IP block in the next
> years
> I'm not saying you have to change the name - I'm bringing this up so
> you can decide for yourself based on examples from the past
>
> here are a few examples:
> - when Kevin upstreamed the MMC driver for GX he decided to use
> MMC_MESON_GX for the Kconfig symbol name. it turns out that this is
> smart because there are at least two other MMC controller IPs on the
> 32-bit SoCs. due to him including GX in the name the drivers are easy
> to differentiate (MMC_MESON_MX_SDIO and MMC_MESON_MX_SDHC being the
> other ones, while the latter is not upstream yet)
> - when Carlo upstreamed the eFuse driver he decided to use MESON_EFUSE
> for the Kconfig symbol name. I found out much later that the 32-bit
> SoCs use a different IP (or at least direct register access instead of
> going through Secure Monitor). the driver for the 32-bit SoCs now uses
> MESON_MX_EFUSE. if you don't know which driver applies where then it's
> easy to mix up MESON_EFUSE and MESON_MX_EFUSE
> - when Jerome upstreamed the ALSA driver for AXG (which is also used
> on G12A and G12B) he decided to use the SND_MESON_AXG_* prefix for the
> Kconfig symbol names. in my opinion this was a good choice because GXM
> and everything earlier (including the 32-bit SoCs) use a different
> audio IP block. we won't have a Kconfig symbol name clash when a
> driver for the "older" SoCs is upstreamed
> - (there are more examples, Meson8b USB PHY driver, Meson8b DWMAC
> glue, ... - just like there's many examples where the IP block is
> mostly compatible with older generations: SAR ADC, RNG, SPI, ...)
While these are all good examples, you can see it can go both ways, so
there's really no way know up front what is the "right" way. We only
know after the fact. Unfortunately, we simply have no visibility into
future chips and where IP blocks may be shared or not (there are other
examples where vendors add a new version of an IP *and* keep the old
version. ;)
Even having worked inside a (different) SoC vendor and having some
knowledge about what IPs are shared, it's difficult to get this right.
> I'm not sure what driver naming rules other mainline SoC teams use
> to me it seems that the rule for Allwinner driver names is to use the
> "code-name of the first SoC the IP block appeared in"
That's a good rule of thumb (and one we generally follow) but I don't
feel it's important enough to enforce strictly either.
Kevin
Hi Kevin,
On Thu, Aug 8, 2019 at 4:59 AM Kevin Hilman <[email protected]> wrote:
>
> Martin Blumenstingl <[email protected]> writes:
>
> > Hi Guillaume,
> >
> > On Mon, Aug 5, 2019 at 2:48 PM guillaume La Roque <[email protected]> wrote:
> >>
> >> Hi Martin,
> >>
> >> again thanks for your review.
> > you're welcome - thank you for working on the driver :-)
> >
> > [...]
> >> > The IP block has more functionality, which may be added to this driver
> >> > in the future:
> >> > - reading up to 16 stored temperature samples
> >>
> >> it's not working, you can verify it if you check the regmap define in the driver. in fact temp is only write in one register, it's confirmed by amlogic.
> > I missed that - so please skip this part
> >
> > [...]
> >> >> +config AMLOGIC_THERMAL
> >> > we typically use "MESON" in the Kconfig symbols:
> >> > $ grep -c AMLOGIC .config
> >> > 1
> >> > $ grep -c MESON .config
> >> > 33
> >> >
> >> > I also wonder if we should add G12 or G12A so we don't conflict with
> >> > upcoming thermal sensors with a different design (assuming that this
> >> > will be a thing).
> >> > for example we already have three different USB2 PHY drivers
> >> >
> >> > [...]
> >>
> >> i check with Neil and for new family it's better to use Amlogic instead of meson.
> > can you please share the considerations behind this decision?
> > if new drivers should use AMLOGIC_* Kconfig symbols instead of MESON_*
> > then we all should know about it
> >
> >> i don't add G12 because we already know it's same sensors for SM1 SoC family [0].
> > my idea behind this was to avoid conflicts in the future
> > in case of the thermal driver we may be fine with using a generic name
> > assuming that Amlogic will not switch to a new IP block in the next
> > years
> > I'm not saying you have to change the name - I'm bringing this up so
> > you can decide for yourself based on examples from the past
> >
> > here are a few examples:
> > - when Kevin upstreamed the MMC driver for GX he decided to use
> > MMC_MESON_GX for the Kconfig symbol name. it turns out that this is
> > smart because there are at least two other MMC controller IPs on the
> > 32-bit SoCs. due to him including GX in the name the drivers are easy
> > to differentiate (MMC_MESON_MX_SDIO and MMC_MESON_MX_SDHC being the
> > other ones, while the latter is not upstream yet)
> > - when Carlo upstreamed the eFuse driver he decided to use MESON_EFUSE
> > for the Kconfig symbol name. I found out much later that the 32-bit
> > SoCs use a different IP (or at least direct register access instead of
> > going through Secure Monitor). the driver for the 32-bit SoCs now uses
> > MESON_MX_EFUSE. if you don't know which driver applies where then it's
> > easy to mix up MESON_EFUSE and MESON_MX_EFUSE
> > - when Jerome upstreamed the ALSA driver for AXG (which is also used
> > on G12A and G12B) he decided to use the SND_MESON_AXG_* prefix for the
> > Kconfig symbol names. in my opinion this was a good choice because GXM
> > and everything earlier (including the 32-bit SoCs) use a different
> > audio IP block. we won't have a Kconfig symbol name clash when a
> > driver for the "older" SoCs is upstreamed
> > - (there are more examples, Meson8b USB PHY driver, Meson8b DWMAC
> > glue, ... - just like there's many examples where the IP block is
> > mostly compatible with older generations: SAR ADC, RNG, SPI, ...)
>
> While these are all good examples, you can see it can go both ways, so
> there's really no way know up front what is the "right" way. We only
> know after the fact. Unfortunately, we simply have no visibility into
> future chips and where IP blocks may be shared or not (there are other
> examples where vendors add a new version of an IP *and* keep the old
> version. ;)
>
> Even having worked inside a (different) SoC vendor and having some
> knowledge about what IPs are shared, it's difficult to get this right.
right. The fact that it'll be the IP block in SM1 will be backwards
compatible (or even the same) means that it has a longer life-span
than some of the USB PHY IP.
so I'm fine either way
Martin