| Message ID | 1583681240-14782-2-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] dt-bindings: thermal: imx8mm-thermal: Add support for i.MX8MP | expand |
On 08/03/2020 16:27, Anson Huang wrote: > i.MX8MP shares same TMU with i.MX8MM, the only difference is i.MX8MP > has two thermal sensors while i.MX8MM ONLY has one, add multiple sensors > support for i.MX8MM TMU driver. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > drivers/thermal/imx8mm_thermal.c | 108 +++++++++++++++++++++++++++++++++------ > 1 file changed, 93 insertions(+), 15 deletions(-) > > diff --git a/drivers/thermal/imx8mm_thermal.c b/drivers/thermal/imx8mm_thermal.c > index d597ceb..8a87ed0 100644 > --- a/drivers/thermal/imx8mm_thermal.c > +++ b/drivers/thermal/imx8mm_thermal.c > @@ -10,34 +10,75 @@ > #include <linux/io.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/thermal.h> > > #include "thermal_core.h" > > #define TER 0x0 /* TMU enable */ > +#define TPS 0x4 > #define TRITSR 0x20 /* TMU immediate temp */ > > #define TER_EN BIT(31) > #define TRITSR_VAL_MASK 0xff > > -#define TEMP_LOW_LIMIT 10 > +#define PROBE_SEL_ALL GENMASK(31, 30) > > -struct imx8mm_tmu { > +#define PROBE0_STATUS_OFFSET 30 > +#define PROBE0_VAL_OFFSET 16 > +#define SIGN_BIT BIT(7) > +#define TEMP_VAL_MASK GENMASK(6, 0) > + > +#define VER1_TEMP_LOW_LIMIT 10 > +#define VER2_TEMP_LOW_LIMIT -40 > +#define VER2_TEMP_HIGH_LIMIT 125 > + > +#define TMU_VER1 0x1 > +#define TMU_VER2 0x2 > + > +struct thermal_soc_data { > + u32 num_sensors; > + u32 version; > +}; > + > +struct tmu_sensor { > + struct imx8mm_tmu *priv; > + u32 hw_id; > struct thermal_zone_device *tzd; > +}; > + > +struct imx8mm_tmu { > void __iomem *base; > struct clk *clk; > + const struct thermal_soc_data *socdata; > + struct tmu_sensor sensors[0]; > }; > > static int tmu_get_temp(void *data, int *temp) > { > - struct imx8mm_tmu *tmu = data; > + struct tmu_sensor *sensor = data; > + struct imx8mm_tmu *tmu = sensor->priv; > + bool ready; > u32 val; > > - val = readl_relaxed(tmu->base + TRITSR) & TRITSR_VAL_MASK; > - if (val < TEMP_LOW_LIMIT) > - return -EAGAIN; > + if (tmu->socdata->version == TMU_VER1) { Don't do this here, implement a callback to read the temp, store it in the socdata and call it directly from here. So you end up with something simple like: *temp = tmu->socdata->get_temp(...); > + val = readl_relaxed(tmu->base + TRITSR) & TRITSR_VAL_MASK; > + if (val < VER1_TEMP_LOW_LIMIT) > + return -EAGAIN;> + } else { > + val = readl_relaxed(tmu->base + TRITSR); > + ready = val & (1 << (sensor->hw_id + PROBE0_STATUS_OFFSET)); test_bit()? > + val = (val >> (sensor->hw_id * PROBE0_VAL_OFFSET)) > + & TRITSR_VAL_MASK; > + if (val & SIGN_BIT) /* negative */ > + val = (~(val & TEMP_VAL_MASK) + 1); Please have a look at the different bitops available to simplify this decoding. > + *temp = val; > + if (!ready || *temp < VER2_TEMP_LOW_LIMIT || > + *temp > VER2_TEMP_HIGH_LIMIT) > + return -EAGAIN; > + } > > *temp = val * 1000; > > @@ -50,14 +91,21 @@ static struct thermal_zone_of_device_ops tmu_tz_ops = { > > static int imx8mm_tmu_probe(struct platform_device *pdev) > { > + const struct thermal_soc_data *data; > struct imx8mm_tmu *tmu; > u32 val; > int ret; > + int i; > + > + data = of_device_get_match_data(&pdev->dev); > > - tmu = devm_kzalloc(&pdev->dev, sizeof(struct imx8mm_tmu), GFP_KERNEL); > + tmu = devm_kzalloc(&pdev->dev, struct_size(tmu, sensors, > + data->num_sensors), GFP_KERNEL); > if (!tmu) > return -ENOMEM; > > + tmu->socdata = data; > + > tmu->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(tmu->base)) > return PTR_ERR(tmu->base); > @@ -77,16 +125,35 @@ static int imx8mm_tmu_probe(struct platform_device *pdev) > return ret; > } > > - tmu->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, > - tmu, &tmu_tz_ops); > - if (IS_ERR(tmu->tzd)) { > - dev_err(&pdev->dev, > - "failed to register thermal zone sensor: %d\n", ret); > - return PTR_ERR(tmu->tzd); > + /* disable the monitor during initialization */ > + val = readl_relaxed(tmu->base + TER); > + val &= ~TER_EN; > + writel_relaxed(val, tmu->base + TER); Could you wrap those calls inside a small helper function with a self described name? > + > + for (i = 0; i < data->num_sensors; i++) { > + tmu->sensors[i].priv = tmu; > + tmu->sensors[i].tzd = > + devm_thermal_zone_of_sensor_register(&pdev->dev, i, > + &tmu->sensors[i], > + &tmu_tz_ops); > + if (IS_ERR(tmu->sensors[i].tzd)) { > + dev_err(&pdev->dev, > + "failed to register thermal zone sensor[%d]: %d\n", > + i, ret); > + return PTR_ERR(tmu->sensors[i].tzd); > + } > + tmu->sensors[i].hw_id = i; May be you can store the offset directly, so it is not computed every time the temperature is read? > } > > platform_set_drvdata(pdev, tmu); > > + /* enable all the probes for V2 TMU */ > + if (tmu->socdata->version == TMU_VER2) { > + val = readl_relaxed(tmu->base + TPS); > + val |= PROBE_SEL_ALL; > + writel_relaxed(val, tmu->base + TPS); > + } Same comment as before about putting these in a helper > /* enable the monitor */ > val = readl_relaxed(tmu->base + TER); > val |= TER_EN; > @@ -111,8 +178,19 @@ static int imx8mm_tmu_remove(struct platform_device *pdev) > return 0; > } > > +static struct thermal_soc_data imx8mm_tmu_data = { > + .num_sensors = 1, > + .version = TMU_VER1, > +}; > + > +static struct thermal_soc_data imx8mp_tmu_data = { > + .num_sensors = 2, > + .version = TMU_VER2, > +}; > + > static const struct of_device_id imx8mm_tmu_table[] = { > - { .compatible = "fsl,imx8mm-tmu", }, > + { .compatible = "fsl,imx8mm-tmu", .data = &imx8mm_tmu_data, }, > + { .compatible = "fsl,imx8mp-tmu", .data = &imx8mp_tmu_data, }, > { }, > }; > >
Hi, Daniel > Subject: Re: [PATCH 2/3] thermal: imx8mm: Add i.MX8MP support > > On 08/03/2020 16:27, Anson Huang wrote: > > i.MX8MP shares same TMU with i.MX8MM, the only difference is i.MX8MP > > has two thermal sensors while i.MX8MM ONLY has one, add multiple > > sensors support for i.MX8MM TMU driver. > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > --- > > drivers/thermal/imx8mm_thermal.c | 108 > > +++++++++++++++++++++++++++++++++------ > > 1 file changed, 93 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/thermal/imx8mm_thermal.c > > b/drivers/thermal/imx8mm_thermal.c > > index d597ceb..8a87ed0 100644 > > --- a/drivers/thermal/imx8mm_thermal.c > > +++ b/drivers/thermal/imx8mm_thermal.c > > @@ -10,34 +10,75 @@ > > #include <linux/io.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/thermal.h> > > > > #include "thermal_core.h" > > > > #define TER 0x0 /* TMU enable */ > > +#define TPS 0x4 > > #define TRITSR 0x20 /* TMU immediate temp */ > > > > #define TER_EN BIT(31) > > #define TRITSR_VAL_MASK 0xff > > > > -#define TEMP_LOW_LIMIT 10 > > +#define PROBE_SEL_ALL GENMASK(31, 30) > > > > -struct imx8mm_tmu { > > +#define PROBE0_STATUS_OFFSET 30 > > +#define PROBE0_VAL_OFFSET 16 > > +#define SIGN_BIT BIT(7) > > +#define TEMP_VAL_MASK GENMASK(6, 0) > > + > > +#define VER1_TEMP_LOW_LIMIT 10 > > +#define VER2_TEMP_LOW_LIMIT -40 > > +#define VER2_TEMP_HIGH_LIMIT 125 > > + > > +#define TMU_VER1 0x1 > > +#define TMU_VER2 0x2 > > + > > +struct thermal_soc_data { > > + u32 num_sensors; > > + u32 version; > > +}; > > + > > +struct tmu_sensor { > > + struct imx8mm_tmu *priv; > > + u32 hw_id; > > struct thermal_zone_device *tzd; > > +}; > > + > > +struct imx8mm_tmu { > > void __iomem *base; > > struct clk *clk; > > + const struct thermal_soc_data *socdata; > > + struct tmu_sensor sensors[0]; > > }; > > > > static int tmu_get_temp(void *data, int *temp) { > > - struct imx8mm_tmu *tmu = data; > > + struct tmu_sensor *sensor = data; > > + struct imx8mm_tmu *tmu = sensor->priv; > > + bool ready; > > u32 val; > > > > - val = readl_relaxed(tmu->base + TRITSR) & TRITSR_VAL_MASK; > > - if (val < TEMP_LOW_LIMIT) > > - return -EAGAIN; > > + if (tmu->socdata->version == TMU_VER1) { > > Don't do this here, implement a callback to read the temp, store it in the > socdata and call it directly from here. > > So you end up with something simple like: > > *temp = tmu->socdata->get_temp(...); > Make sense, do it in V2. > > + val = readl_relaxed(tmu->base + TRITSR) & > TRITSR_VAL_MASK; > > + if (val < VER1_TEMP_LOW_LIMIT) > > + return -EAGAIN;> + } else { > > + val = readl_relaxed(tmu->base + TRITSR); > > + ready = val & (1 << (sensor->hw_id + > PROBE0_STATUS_OFFSET)); > > test_bit()? OK. > > > + val = (val >> (sensor->hw_id * PROBE0_VAL_OFFSET)) > > + & TRITSR_VAL_MASK; > > + if (val & SIGN_BIT) /* negative */ > > + val = (~(val & TEMP_VAL_MASK) + 1); > > Please have a look at the different bitops available to simplify this decoding. I can ONLY find the FIELD_GET for getting the temperature value field, for the positive and negative value check, I can't find any API for it. > > > + *temp = val; > > + if (!ready || *temp < VER2_TEMP_LOW_LIMIT || > > + *temp > VER2_TEMP_HIGH_LIMIT) > > + return -EAGAIN; > > + } > > > > *temp = val * 1000; > > > > @@ -50,14 +91,21 @@ static struct thermal_zone_of_device_ops > > tmu_tz_ops = { > > > > static int imx8mm_tmu_probe(struct platform_device *pdev) { > > + const struct thermal_soc_data *data; > > struct imx8mm_tmu *tmu; > > u32 val; > > int ret; > > + int i; > > + > > + data = of_device_get_match_data(&pdev->dev); > > > > - tmu = devm_kzalloc(&pdev->dev, sizeof(struct imx8mm_tmu), > GFP_KERNEL); > > + tmu = devm_kzalloc(&pdev->dev, struct_size(tmu, sensors, > > + data->num_sensors), GFP_KERNEL); > > if (!tmu) > > return -ENOMEM; > > > > + tmu->socdata = data; > > + > > tmu->base = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(tmu->base)) > > return PTR_ERR(tmu->base); > > @@ -77,16 +125,35 @@ static int imx8mm_tmu_probe(struct > platform_device *pdev) > > return ret; > > } > > > > - tmu->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, > > - tmu, &tmu_tz_ops); > > - if (IS_ERR(tmu->tzd)) { > > - dev_err(&pdev->dev, > > - "failed to register thermal zone sensor: %d\n", ret); > > - return PTR_ERR(tmu->tzd); > > + /* disable the monitor during initialization */ > > + val = readl_relaxed(tmu->base + TER); > > + val &= ~TER_EN; > > + writel_relaxed(val, tmu->base + TER); > > Could you wrap those calls inside a small helper function with a self > described name? OK. > > > + > > + for (i = 0; i < data->num_sensors; i++) { > > + tmu->sensors[i].priv = tmu; > > + tmu->sensors[i].tzd = > > + devm_thermal_zone_of_sensor_register(&pdev->dev, > i, > > + &tmu->sensors[i], > > + &tmu_tz_ops); > > + if (IS_ERR(tmu->sensors[i].tzd)) { > > + dev_err(&pdev->dev, > > + "failed to register thermal zone > sensor[%d]: %d\n", > > + i, ret); > > + return PTR_ERR(tmu->sensors[i].tzd); > > + } > > + tmu->sensors[i].hw_id = i; > > May be you can store the offset directly, so it is not computed every time the > temperature is read? There are 2 place need to identify the sensor ID, the ready bit and the temperature value field, so I think the hw_id is necessary, and it also make the logic easy to read. > > > } > > > > platform_set_drvdata(pdev, tmu); > > > > + /* enable all the probes for V2 TMU */ > > + if (tmu->socdata->version == TMU_VER2) { > > + val = readl_relaxed(tmu->base + TPS); > > + val |= PROBE_SEL_ALL; > > + writel_relaxed(val, tmu->base + TPS); > > + } > > Same comment as before about putting these in a helper OK Please help review V2 patch. Thanks, Anson
diff --git a/drivers/thermal/imx8mm_thermal.c b/drivers/thermal/imx8mm_thermal.c index d597ceb..8a87ed0 100644 --- a/drivers/thermal/imx8mm_thermal.c +++ b/drivers/thermal/imx8mm_thermal.c @@ -10,34 +10,75 @@ #include <linux/io.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/thermal.h> #include "thermal_core.h" #define TER 0x0 /* TMU enable */ +#define TPS 0x4 #define TRITSR 0x20 /* TMU immediate temp */ #define TER_EN BIT(31) #define TRITSR_VAL_MASK 0xff -#define TEMP_LOW_LIMIT 10 +#define PROBE_SEL_ALL GENMASK(31, 30) -struct imx8mm_tmu { +#define PROBE0_STATUS_OFFSET 30 +#define PROBE0_VAL_OFFSET 16 +#define SIGN_BIT BIT(7) +#define TEMP_VAL_MASK GENMASK(6, 0) + +#define VER1_TEMP_LOW_LIMIT 10 +#define VER2_TEMP_LOW_LIMIT -40 +#define VER2_TEMP_HIGH_LIMIT 125 + +#define TMU_VER1 0x1 +#define TMU_VER2 0x2 + +struct thermal_soc_data { + u32 num_sensors; + u32 version; +}; + +struct tmu_sensor { + struct imx8mm_tmu *priv; + u32 hw_id; struct thermal_zone_device *tzd; +}; + +struct imx8mm_tmu { void __iomem *base; struct clk *clk; + const struct thermal_soc_data *socdata; + struct tmu_sensor sensors[0]; }; static int tmu_get_temp(void *data, int *temp) { - struct imx8mm_tmu *tmu = data; + struct tmu_sensor *sensor = data; + struct imx8mm_tmu *tmu = sensor->priv; + bool ready; u32 val; - val = readl_relaxed(tmu->base + TRITSR) & TRITSR_VAL_MASK; - if (val < TEMP_LOW_LIMIT) - return -EAGAIN; + if (tmu->socdata->version == TMU_VER1) { + val = readl_relaxed(tmu->base + TRITSR) & TRITSR_VAL_MASK; + if (val < VER1_TEMP_LOW_LIMIT) + return -EAGAIN; + } else { + val = readl_relaxed(tmu->base + TRITSR); + ready = val & (1 << (sensor->hw_id + PROBE0_STATUS_OFFSET)); + val = (val >> (sensor->hw_id * PROBE0_VAL_OFFSET)) + & TRITSR_VAL_MASK; + if (val & SIGN_BIT) /* negative */ + val = (~(val & TEMP_VAL_MASK) + 1); + + *temp = val; + if (!ready || *temp < VER2_TEMP_LOW_LIMIT || + *temp > VER2_TEMP_HIGH_LIMIT) + return -EAGAIN; + } *temp = val * 1000; @@ -50,14 +91,21 @@ static struct thermal_zone_of_device_ops tmu_tz_ops = { static int imx8mm_tmu_probe(struct platform_device *pdev) { + const struct thermal_soc_data *data; struct imx8mm_tmu *tmu; u32 val; int ret; + int i; + + data = of_device_get_match_data(&pdev->dev); - tmu = devm_kzalloc(&pdev->dev, sizeof(struct imx8mm_tmu), GFP_KERNEL); + tmu = devm_kzalloc(&pdev->dev, struct_size(tmu, sensors, + data->num_sensors), GFP_KERNEL); if (!tmu) return -ENOMEM; + tmu->socdata = data; + tmu->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(tmu->base)) return PTR_ERR(tmu->base); @@ -77,16 +125,35 @@ static int imx8mm_tmu_probe(struct platform_device *pdev) return ret; } - tmu->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, - tmu, &tmu_tz_ops); - if (IS_ERR(tmu->tzd)) { - dev_err(&pdev->dev, - "failed to register thermal zone sensor: %d\n", ret); - return PTR_ERR(tmu->tzd); + /* disable the monitor during initialization */ + val = readl_relaxed(tmu->base + TER); + val &= ~TER_EN; + writel_relaxed(val, tmu->base + TER); + + for (i = 0; i < data->num_sensors; i++) { + tmu->sensors[i].priv = tmu; + tmu->sensors[i].tzd = + devm_thermal_zone_of_sensor_register(&pdev->dev, i, + &tmu->sensors[i], + &tmu_tz_ops); + if (IS_ERR(tmu->sensors[i].tzd)) { + dev_err(&pdev->dev, + "failed to register thermal zone sensor[%d]: %d\n", + i, ret); + return PTR_ERR(tmu->sensors[i].tzd); + } + tmu->sensors[i].hw_id = i; } platform_set_drvdata(pdev, tmu); + /* enable all the probes for V2 TMU */ + if (tmu->socdata->version == TMU_VER2) { + val = readl_relaxed(tmu->base + TPS); + val |= PROBE_SEL_ALL; + writel_relaxed(val, tmu->base + TPS); + } + /* enable the monitor */ val = readl_relaxed(tmu->base + TER); val |= TER_EN; @@ -111,8 +178,19 @@ static int imx8mm_tmu_remove(struct platform_device *pdev) return 0; } +static struct thermal_soc_data imx8mm_tmu_data = { + .num_sensors = 1, + .version = TMU_VER1, +}; + +static struct thermal_soc_data imx8mp_tmu_data = { + .num_sensors = 2, + .version = TMU_VER2, +}; + static const struct of_device_id imx8mm_tmu_table[] = { - { .compatible = "fsl,imx8mm-tmu", }, + { .compatible = "fsl,imx8mm-tmu", .data = &imx8mm_tmu_data, }, + { .compatible = "fsl,imx8mp-tmu", .data = &imx8mp_tmu_data, }, { }, };
i.MX8MP shares same TMU with i.MX8MM, the only difference is i.MX8MP has two thermal sensors while i.MX8MM ONLY has one, add multiple sensors support for i.MX8MM TMU driver. Signed-off-by: Anson Huang <Anson.Huang@nxp.com> --- drivers/thermal/imx8mm_thermal.c | 108 +++++++++++++++++++++++++++++++++------ 1 file changed, 93 insertions(+), 15 deletions(-)