Re: [PATCH V3] thermal: qoriq: Add thermal management support

[Scott Wood]

On Wed, 2015-11-04 at 11:24 -0800, Eduardo Valentin wrote:
> On Fri, Sep 25, 2015 at 03:09:42AM +, Hongtao Jia wrote:
> > 
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Friday, September 25, 2015 6:10 AM
> > > To: Jia Hongtao-B38951
> > > Cc: edubez...@gmail.com; linux...@vger.kernel.org; linuxppc-
> > > d...@lists.ozlabs.org
> > > Subject: Re: [PATCH V3] thermal: qoriq: Add thermal management support
> > > 
> > > On Wed, 2015-09-23 at 16:28 +0800, Jia Hongtao wrote:
> > > > This driver add thermal management support by enabling TMU (Thermal
> > > > Monitoring Unit) on QorIQ platform.
> > > > 
> > > > It's based on thermal of framework:
> > > > - Trip points defined in device tree.
> > > > - Cpufreq as cooling device registered in qoriq cpufreq driver.
> > > 
> > > I don't see any cooling device registered in the qoriq cpufreq driver.
> > > Is this dependent on some other patch?
> > 
> > It's not depend on any patch. But I saw your patch below:
> > [PATCH v3 5/5] cpufreq: qoriq: Don't look at clock implementation details
> > So I hold my patch waiting for your patch merged or there will be 
> > conflict.
> > 
> > I could send it out too if you are fine with it.
> 
> Would you guys benefit of cpufreq-cpu0?

If you mean cpufreq-dt, no, as I explained in 
https://lkml.org/lkml/2015/9/18/699

-Scott

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

RE: [PATCH V3] thermal: qoriq: Add thermal management support

[Hongtao Jia]

> -Original Message-
> From: Eduardo Valentin [mailto:edubez...@gmail.com]
> Sent: Thursday, November 05, 2015 3:25 AM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; linux...@vger.kernel.org; linuxppc-
> d...@lists.ozlabs.org
> Subject: Re: [PATCH V3] thermal: qoriq: Add thermal management support
> 
> On Fri, Sep 25, 2015 at 03:09:42AM +, Hongtao Jia wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Friday, September 25, 2015 6:10 AM
> > > To: Jia Hongtao-B38951
> > > Cc: edubez...@gmail.com; linux...@vger.kernel.org; linuxppc-
> > > d...@lists.ozlabs.org
> > > Subject: Re: [PATCH V3] thermal: qoriq: Add thermal management
> > > support
> > >
> > > On Wed, 2015-09-23 at 16:28 +0800, Jia Hongtao wrote:
> > > > This driver add thermal management support by enabling TMU
> > > > (Thermal Monitoring Unit) on QorIQ platform.
> > > >
> > > > It's based on thermal of framework:
> > > > - Trip points defined in device tree.
> > > > - Cpufreq as cooling device registered in qoriq cpufreq driver.
> > >
> > > I don't see any cooling device registered in the qoriq cpufreq driver.
> > > Is this dependent on some other patch?
> >
> > It's not depend on any patch. But I saw your patch below:
> > [PATCH v3 5/5] cpufreq: qoriq: Don't look at clock implementation
> > details So I hold my patch waiting for your patch merged or there will
> be conflict.
> >
> > I could send it out too if you are fine with it.
> 
> Would you guys benefit of cpufreq-cpu0?
> 
> 
> 
> 
> >
> > >
> > > >
> > > > Signed-off-by: Jia Hongtao <hongtao@freescale.com>
> > > > ---
> > > > V3: Using thermal of framework.
> > > >
> > > >  drivers/thermal/Kconfig |  11 ++
> > > >  drivers/thermal/Makefile|   1 +
> > > >  drivers/thermal/qoriq_thermal.c | 267
> 
> Please include a binding description in your next version. Also, remember
> to include an example of your binding.
> 
> 
> 
> > > > 
> > > >  3 files changed, 279 insertions(+)  create mode 100644
> > > > drivers/thermal/qoriq_thermal.c
> > > >
> > > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > > > index 0390044..c91041b 100644
> > > > --- a/drivers/thermal/Kconfig
> > > > +++ b/drivers/thermal/Kconfig
> > > > @@ -180,6 +180,17 @@ config IMX_THERMAL
> > > > cpufreq is used as the cooling device to throttle CPUs when
> the
> > > > passive trip is crossed.
> > > >
> > > > +config QORIQ_THERMAL
> 
> 
> It would be really great if you could consolidate this driver with
> IMX_THERMAL. Do you think it is doable?

The TMU IP on I.MX and QorIQ platform are completely different and they
use the different cpufreq driver.

I don't think they could share the same thermal driver elegantly.

-Hongtao. 

> 
> > > > + tristate "Freescale QorIQ Thermal Monitoring Unit"
> > > > + depends on CPU_THERMAL
> > > > + depends on THERMAL_OF
> > > > + default n
> > > > + help
> > > > +   Enable thermal management based on Freescale QorIQ Thermal
> > > Monitoring
> > > > +   Unit (TMU). It supports one critical trip point and one
> > > > + passive
> > > trip
> > > > +   point. The cpufreq is used as the cooling device to
> > > > + throttle
> > > CPUs when
> > > > +   the passive trip is crossed.
> > >
> > > "default n" is unnecessary -- n is already the default.
> >
> > Right.
> >
> > >
> > > Where is the interaction between this driver and cpufreq?
> >
> > It's all in cpufreq patch I mentioned above.
> >
> > >
> > > > config SPEAR_THERMAL
> > > >   bool "SPEAr thermal sensor driver"
> > > >   depends on PLAT_SPEAR
> > > > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > > > index
> > > > 26f1608..e55d703 100644
> > > > --- a/drivers/thermal/Makefile
> > > > +++ b/drivers/thermal/Makefile
> > > > @@ -33,6 +33,7 @@ obj-$(CONFIG_DOVE_THERMAL)  +=
> dove_thermal.o
> > > >  obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o
> > &

Re: [PATCH V3] thermal: qoriq: Add thermal management support

[Eduardo Valentin]

On Fri, Sep 25, 2015 at 03:09:42AM +, Hongtao Jia wrote:
> 
> > -Original Message-
> > From: Wood Scott-B07421
> > Sent: Friday, September 25, 2015 6:10 AM
> > To: Jia Hongtao-B38951
> > Cc: edubez...@gmail.com; linux...@vger.kernel.org; linuxppc-
> > d...@lists.ozlabs.org
> > Subject: Re: [PATCH V3] thermal: qoriq: Add thermal management support
> > 
> > On Wed, 2015-09-23 at 16:28 +0800, Jia Hongtao wrote:
> > > This driver add thermal management support by enabling TMU (Thermal
> > > Monitoring Unit) on QorIQ platform.
> > >
> > > It's based on thermal of framework:
> > > - Trip points defined in device tree.
> > > - Cpufreq as cooling device registered in qoriq cpufreq driver.
> > 
> > I don't see any cooling device registered in the qoriq cpufreq driver.
> > Is this dependent on some other patch?
> 
> It's not depend on any patch. But I saw your patch below:
> [PATCH v3 5/5] cpufreq: qoriq: Don't look at clock implementation details
> So I hold my patch waiting for your patch merged or there will be conflict.
> 
> I could send it out too if you are fine with it.

Would you guys benefit of cpufreq-cpu0?




> 
> > 
> > >
> > > Signed-off-by: Jia Hongtao <hongtao@freescale.com>
> > > ---
> > > V3: Using thermal of framework.
> > >
> > >  drivers/thermal/Kconfig |  11 ++
> > >  drivers/thermal/Makefile|   1 +
> > >  drivers/thermal/qoriq_thermal.c | 267

Please include a binding description in your next version. Also,
remember to include an example of your binding.



> > > 
> > >  3 files changed, 279 insertions(+)
> > >  create mode 100644 drivers/thermal/qoriq_thermal.c
> > >
> > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> > > 0390044..c91041b 100644
> > > --- a/drivers/thermal/Kconfig
> > > +++ b/drivers/thermal/Kconfig
> > > @@ -180,6 +180,17 @@ config IMX_THERMAL
> > > cpufreq is used as the cooling device to throttle CPUs when the
> > > passive trip is crossed.
> > >
> > > +config QORIQ_THERMAL


It would be really great if you could consolidate this driver with
IMX_THERMAL. Do you think it is doable?

> > > + tristate "Freescale QorIQ Thermal Monitoring Unit"
> > > + depends on CPU_THERMAL
> > > + depends on THERMAL_OF
> > > + default n
> > > + help
> > > +   Enable thermal management based on Freescale QorIQ Thermal
> > Monitoring
> > > +   Unit (TMU). It supports one critical trip point and one passive
> > trip
> > > +   point. The cpufreq is used as the cooling device to throttle
> > CPUs when
> > > +   the passive trip is crossed.
> > 
> > "default n" is unnecessary -- n is already the default.
> 
> Right.
> 
> > 
> > Where is the interaction between this driver and cpufreq?
> 
> It's all in cpufreq patch I mentioned above.
> 
> > 
> > > config SPEAR_THERMAL
> > >   bool "SPEAr thermal sensor driver"
> > >   depends on PLAT_SPEAR
> > > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index
> > > 26f1608..e55d703 100644
> > > --- a/drivers/thermal/Makefile
> > > +++ b/drivers/thermal/Makefile
> > > @@ -33,6 +33,7 @@ obj-$(CONFIG_DOVE_THERMAL)  += dove_thermal.o
> > >  obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o
> > >  obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o
> > >  obj-$(CONFIG_IMX_THERMAL)+= imx_thermal.o
> > > +obj-$(CONFIG_QORIQ_THERMAL)  += qoriq_thermal.o
> > >  obj-$(CONFIG_DB8500_CPUFREQ_COOLING) += db8500_cpufreq_cooling.o
> > >  obj-$(CONFIG_INTEL_POWERCLAMP)   += intel_powerclamp.o
> > >  obj-$(CONFIG_X86_PKG_TEMP_THERMAL)   += x86_pkg_temp_thermal.o
> > > diff --git a/drivers/thermal/qoriq_thermal.c
> > > b/drivers/thermal/qoriq_thermal.c new file mode 100644 index
> > > 000..7c2a3261
> > > --- /dev/null
> > > +++ b/drivers/thermal/qoriq_thermal.c
> > > @@ -0,0 +1,267 @@
> > > +/*
> > > + * Copyright 2015 Freescale Semiconductor, Inc.
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > +modify it
> > > + * under the terms and conditions of the GNU General Public License,
> > > + * version 2, as published by the Free Software Foundation.
> > > + *
> > > + * T

RE: [PATCH V3] thermal: qoriq: Add thermal management support

[Hongtao Jia]

> -Original Message-
> From: Wood Scott-B07421
> Sent: Friday, September 25, 2015 1:12 PM
> To: Jia Hongtao-B38951
> Cc: edubez...@gmail.com; linux...@vger.kernel.org; linuxppc-
> d...@lists.ozlabs.org
> Subject: Re: [PATCH V3] thermal: qoriq: Add thermal management support
> 
> On Thu, 2015-09-24 at 22:09 -0500, Jia Hongtao-B38951 wrote:
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Friday, September 25, 2015 6:10 AM
> > > To: Jia Hongtao-B38951
> > > Cc: edubez...@gmail.com; linux...@vger.kernel.org; linuxppc-
> > > d...@lists.ozlabs.org
> > > Subject: Re: [PATCH V3] thermal: qoriq: Add thermal management
> support
> > >
> > > On Wed, 2015-09-23 at 16:28 +0800, Jia Hongtao wrote:
> > > > This driver add thermal management support by enabling TMU (Thermal
> > > > Monitoring Unit) on QorIQ platform.
> > > >
> > > > It's based on thermal of framework:
> > > > - Trip points defined in device tree.
> > > > - Cpufreq as cooling device registered in qoriq cpufreq driver.
> > >
> > > I don't see any cooling device registered in the qoriq cpufreq driver.
> > > Is this dependent on some other patch?
> >
> > It's not depend on any patch. But I saw your patch below:
> > [PATCH v3 5/5] cpufreq: qoriq: Don't look at clock implementation
> details
> > So I hold my patch waiting for your patch merged or there will be
> conflict.
> >
> > I could send it out too if you are fine with it.
> 
> Yes, rebase it on that patch and send it.  Mention below the --- that
> you're
> depending on that patch, and provide a patchwork/archive link.

Ok. But I need rebase again if you have further changes on that patch.
Anyway, this is not a big problem for me.

> 
> > > > +static int tmu_get_temp(void *p, int *temp) {
> > > > + u8 val;
> > > > + struct qoriq_tmu_data *data = p;
> > > > +
> > > > + val = ioread32be(>regs->site[0].tritsr);
> > > > + *temp = (int)val * 1000;
> > >
> > > Why don't you declare val as int in the first place?
> >
> > It's a 32bit register.
> > Only the least significant 8 bits represent the temperature.
> > The most significant bit shows the validness of the value.
> > I use u8 type to remove the rest 24bits influence.
> 
> That's even worse.  Use an explicit & operation to extract the field
> you're
> interested in.

Will change for next version.

> 
> > > > + ret = qoriq_tmu_calibration(pdev);  /* TMU calibration */
> > > > + if (ret < 0) {
> > > > + dev_err(>dev, "TMU calibration failed.\n");
> > > > + ret = -ENODEV;
> > > > + goto err_iomap;
> > > > + }
> > >
> > > That function returns negative when device tree properties are
> missing,
> > > not when a calibration procedure fails.
> >
> > What's your suggestion here to return then?
> 
> Remove this message and add a message in qoriq_tmu_calibration for each
> error
> condition.  Also have qoriq_tmu_calibration pass back a proper error code,
> not -1.

Yes, this is more reasonable.

> 
> > > > +static const struct of_device_id qoriq_tmu_match[] = {
> > > > + { .compatible = "fsl,qoriq-tmu", },
> > > > + {},
> > > > +};
> > >
> > > Binding?
> >
> > Not send out yet.
> 
> The binding needs to come before the driver that uses it.

Right. I will send out the whole patchset.

> 
> -Scott
> 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V3] thermal: qoriq: Add thermal management support

[Scott Wood]

On Wed, 2015-09-23 at 16:28 +0800, Jia Hongtao wrote:
> This driver add thermal management support by enabling TMU (Thermal
> Monitoring Unit) on QorIQ platform.
> 
> It's based on thermal of framework:
> - Trip points defined in device tree.
> - Cpufreq as cooling device registered in qoriq cpufreq driver.

I don't see any cooling device registered in the qoriq cpufreq driver.  Is 
this dependent on some other patch?

> 
> Signed-off-by: Jia Hongtao 
> ---
> V3: Using thermal of framework.
> 
>  drivers/thermal/Kconfig |  11 ++
>  drivers/thermal/Makefile|   1 +
>  drivers/thermal/qoriq_thermal.c | 267 
> 
>  3 files changed, 279 insertions(+)
>  create mode 100644 drivers/thermal/qoriq_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 0390044..c91041b 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -180,6 +180,17 @@ config IMX_THERMAL
> cpufreq is used as the cooling device to throttle CPUs when the
> passive trip is crossed.
>  
> +config QORIQ_THERMAL
> + tristate "Freescale QorIQ Thermal Monitoring Unit"
> + depends on CPU_THERMAL
> + depends on THERMAL_OF
> + default n
> + help
> +   Enable thermal management based on Freescale QorIQ Thermal Monitoring
> +   Unit (TMU). It supports one critical trip point and one passive trip
> +   point. The cpufreq is used as the cooling device to throttle CPUs when
> +   the passive trip is crossed.

"default n" is unnecessary -- n is already the default.

Where is the interaction between this driver and cpufreq?

> config SPEAR_THERMAL
>   bool "SPEAr thermal sensor driver"
>   depends on PLAT_SPEAR
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 26f1608..e55d703 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -33,6 +33,7 @@ obj-$(CONFIG_DOVE_THERMAL)  += dove_thermal.o
>  obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o
>  obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o
>  obj-$(CONFIG_IMX_THERMAL)+= imx_thermal.o
> +obj-$(CONFIG_QORIQ_THERMAL)  += qoriq_thermal.o
>  obj-$(CONFIG_DB8500_CPUFREQ_COOLING) += db8500_cpufreq_cooling.o
>  obj-$(CONFIG_INTEL_POWERCLAMP)   += intel_powerclamp.o
>  obj-$(CONFIG_X86_PKG_TEMP_THERMAL)   += x86_pkg_temp_thermal.o
> diff --git a/drivers/thermal/qoriq_thermal.c 
> b/drivers/thermal/qoriq_thermal.c
> new file mode 100644
> index 000..7c2a3261
> --- /dev/null
> +++ b/drivers/thermal/qoriq_thermal.c
> @@ -0,0 +1,267 @@
> +/*
> + * Copyright 2015 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + */
> +
> +/*
> + * Based on Freescale QorIQ Thermal Monitoring Unit (TMU)
> + */

What does this comment mean?  This *is* the "Freescale QorIQ Thermal 
Monitoring Unit" driver.

> +#include 

What do you use from this header?

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "thermal_core.h"
> +
> +#define SITES_MAX16
> +
> +/*
> + * QorIQ TMU Registers
> + */
> +struct qoriq_tmu_site_regs {
> + __be32 tritsr;  /* Immediate Temperature Site Register */
> + __be32 tratsr;  /* Average Temperature Site Register */
> + u8 res0[0x8];
> +} __packed;
> +
> +struct qoriq_tmu_regs {
> + __be32 tmr; /* Mode Register */
> +#define TMR_DISABLE  0x0
> +#define TMR_ME   0x8000
> +#define TMR_ALPF 0x0c00
> +#define TMR_MSITE0x8000  /* Core temperature site */
> +#define TMR_ALL  (TMR_ME | TMR_ALPF | TMR_MSITE)
> + __be32 tsr; /* Status Register */
> + __be32 tmtmir;  /* Temperature measurement interval Register */
> +#define TMTMIR_DEFAULT   0x0007
> + u8 res0[0x14];
> + __be32 tier;/* Interrupt Enable Register */
> +#define TIER_DISABLE 0x0
> + __be32 tidr;/* Interrupt Detect Register */
> + __be32 tiscr;   /* Interrupt Site Capture Register */
> + __be32 ticscr;  /* Interrupt Critical Site Capture Register */
> + u8 res1[0x10];
> + __be32 tmhtcrh; /* High Temperature Capture Register */
> + __be32 tmhtcrl; /* Low Temperature Capture Register */
> + u8 res2[0x8];
> + __be32 tmhtitr; /* High Temperature Immediate Threshold */
> + __be32 tmhtatr; /* High Temperature Average Threshold */
> + __be32 tmhtactr; 

RE: [PATCH V3] thermal: qoriq: Add thermal management support

[Hongtao Jia]

> -Original Message-
> From: Wood Scott-B07421
> Sent: Friday, September 25, 2015 6:10 AM
> To: Jia Hongtao-B38951
> Cc: edubez...@gmail.com; linux...@vger.kernel.org; linuxppc-
> d...@lists.ozlabs.org
> Subject: Re: [PATCH V3] thermal: qoriq: Add thermal management support
> 
> On Wed, 2015-09-23 at 16:28 +0800, Jia Hongtao wrote:
> > This driver add thermal management support by enabling TMU (Thermal
> > Monitoring Unit) on QorIQ platform.
> >
> > It's based on thermal of framework:
> > - Trip points defined in device tree.
> > - Cpufreq as cooling device registered in qoriq cpufreq driver.
> 
> I don't see any cooling device registered in the qoriq cpufreq driver.
> Is this dependent on some other patch?

It's not depend on any patch. But I saw your patch below:
[PATCH v3 5/5] cpufreq: qoriq: Don't look at clock implementation details
So I hold my patch waiting for your patch merged or there will be conflict.

I could send it out too if you are fine with it.

> 
> >
> > Signed-off-by: Jia Hongtao <hongtao@freescale.com>
> > ---
> > V3: Using thermal of framework.
> >
> >  drivers/thermal/Kconfig |  11 ++
> >  drivers/thermal/Makefile|   1 +
> >  drivers/thermal/qoriq_thermal.c | 267
> > 
> >  3 files changed, 279 insertions(+)
> >  create mode 100644 drivers/thermal/qoriq_thermal.c
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> > 0390044..c91041b 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -180,6 +180,17 @@ config IMX_THERMAL
> > cpufreq is used as the cooling device to throttle CPUs when the
> > passive trip is crossed.
> >
> > +config QORIQ_THERMAL
> > + tristate "Freescale QorIQ Thermal Monitoring Unit"
> > + depends on CPU_THERMAL
> > + depends on THERMAL_OF
> > + default n
> > + help
> > +   Enable thermal management based on Freescale QorIQ Thermal
> Monitoring
> > +   Unit (TMU). It supports one critical trip point and one passive
> trip
> > +   point. The cpufreq is used as the cooling device to throttle
> CPUs when
> > +   the passive trip is crossed.
> 
> "default n" is unnecessary -- n is already the default.

Right.

> 
> Where is the interaction between this driver and cpufreq?

It's all in cpufreq patch I mentioned above.

> 
> > config SPEAR_THERMAL
> >   bool "SPEAr thermal sensor driver"
> >   depends on PLAT_SPEAR
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index
> > 26f1608..e55d703 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -33,6 +33,7 @@ obj-$(CONFIG_DOVE_THERMAL)  += dove_thermal.o
> >  obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o
> >  obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o
> >  obj-$(CONFIG_IMX_THERMAL)+= imx_thermal.o
> > +obj-$(CONFIG_QORIQ_THERMAL)  += qoriq_thermal.o
> >  obj-$(CONFIG_DB8500_CPUFREQ_COOLING) += db8500_cpufreq_cooling.o
> >  obj-$(CONFIG_INTEL_POWERCLAMP)   += intel_powerclamp.o
> >  obj-$(CONFIG_X86_PKG_TEMP_THERMAL)   += x86_pkg_temp_thermal.o
> > diff --git a/drivers/thermal/qoriq_thermal.c
> > b/drivers/thermal/qoriq_thermal.c new file mode 100644 index
> > 000..7c2a3261
> > --- /dev/null
> > +++ b/drivers/thermal/qoriq_thermal.c
> > @@ -0,0 +1,267 @@
> > +/*
> > + * Copyright 2015 Freescale Semiconductor, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope 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.
> > + *
> > + */
> > +
> > +/*
> > + * Based on Freescale QorIQ Thermal Monitoring Unit (TMU)  */
> 
> What does this comment mean?  This *is* the "Freescale QorIQ Thermal
> Monitoring Unit" driver.

I mean thermal management based on the monitor TMU.
I could remove this comment though.

> 
> > +#include 
> 
> What do you use from this header?

Sorry, forget to remove it from last version in which cooling devices are
registered in thermal driver i

Re: [PATCH V3] thermal: qoriq: Add thermal management support

[Scott Wood]

On Thu, 2015-09-24 at 22:09 -0500, Jia Hongtao-B38951 wrote:
> > -Original Message-
> > From: Wood Scott-B07421
> > Sent: Friday, September 25, 2015 6:10 AM
> > To: Jia Hongtao-B38951
> > Cc: edubez...@gmail.com; linux...@vger.kernel.org; linuxppc-
> > d...@lists.ozlabs.org
> > Subject: Re: [PATCH V3] thermal: qoriq: Add thermal management support
> > 
> > On Wed, 2015-09-23 at 16:28 +0800, Jia Hongtao wrote:
> > > This driver add thermal management support by enabling TMU (Thermal
> > > Monitoring Unit) on QorIQ platform.
> > > 
> > > It's based on thermal of framework:
> > > - Trip points defined in device tree.
> > > - Cpufreq as cooling device registered in qoriq cpufreq driver.
> > 
> > I don't see any cooling device registered in the qoriq cpufreq driver.
> > Is this dependent on some other patch?
> 
> It's not depend on any patch. But I saw your patch below:
> [PATCH v3 5/5] cpufreq: qoriq: Don't look at clock implementation details
> So I hold my patch waiting for your patch merged or there will be conflict.
> 
> I could send it out too if you are fine with it.

Yes, rebase it on that patch and send it.  Mention below the --- that you're 
depending on that patch, and provide a patchwork/archive link.

> > > +static int tmu_get_temp(void *p, int *temp) {
> > > + u8 val;
> > > + struct qoriq_tmu_data *data = p;
> > > +
> > > + val = ioread32be(>regs->site[0].tritsr);
> > > + *temp = (int)val * 1000;
> > 
> > Why don't you declare val as int in the first place?
> 
> It's a 32bit register.
> Only the least significant 8 bits represent the temperature.
> The most significant bit shows the validness of the value.
> I use u8 type to remove the rest 24bits influence.

That's even worse.  Use an explicit & operation to extract the field you're 
interested in.

> > > + ret = qoriq_tmu_calibration(pdev);  /* TMU calibration */
> > > + if (ret < 0) {
> > > + dev_err(>dev, "TMU calibration failed.\n");
> > > + ret = -ENODEV;
> > > + goto err_iomap;
> > > + }
> > 
> > That function returns negative when device tree properties are missing,
> > not when a calibration procedure fails.
> 
> What's your suggestion here to return then?

Remove this message and add a message in qoriq_tmu_calibration for each error 
condition.  Also have qoriq_tmu_calibration pass back a proper error code, 
not -1.

> > > +static const struct of_device_id qoriq_tmu_match[] = {
> > > + { .compatible = "fsl,qoriq-tmu", },
> > > + {},
> > > +};
> > 
> > Binding?
> 
> Not send out yet.

The binding needs to come before the driver that uses it.

-Scott


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev