Date:   Wed, 8 Feb 2023 11:49:19 +0100
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
To:     zhuyinbo <zhuyinbo@loongson.cn>
Cc:     "Rafael J . Wysocki" <rafael@kernel.org>,
        Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        zhanghongchen <zhanghongchen@loongson.cn>,
        Liu Peibao <liupeibao@loongson.cn>, lvjianmin@loongson.cn,
        wanghongliang@loongson.cn
Subject: Re: [PATCH v12 1/2] thermal: loongson-2: add thermal management
 support
Message-ID: <20230208104919.GA120053@linaro.org>
References: <20221114024709.7975-1-zhuyinbo@loongson.cn>
 <20230206135921.GA15176@linaro.org>
 <64d9782c-cafd-cdc3-3602-719c386d98cc@loongson.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <64d9782c-cafd-cdc3-3602-719c386d98cc@loongson.cn>
Precedence: bulk

On Wed, Feb 08, 2023 at 11:13:33AM +0800, zhuyinbo wrote:

[ ... ]

> > > +struct loongson2_thermal_data {
> > > +	struct thermal_zone_device *tzd;
> > 'tzd' won't be needed after taking into account the comments
> 
> The 'tzd' element is needed,  because the thermal_zone_device_update need
> pass a data->tzd element.
> 
> static irqreturn_t loongson2_thermal_irq_thread(int irq, void *dev)
> {
>         struct loongson2_thermal_data *data = dev;
> 
>         thermal_zone_device_update(data->tzd,
>                                    THERMAL_EVENT_UNSPECIFIED);
>         enable_irq(data->irq);
> 
>         return IRQ_HANDLED;
> }

After taking into account all the comments, enabled_irq() won't be
called, so 'data' won't be needed. 'tzd' will be passed to
devm_request_threaded_irq() instead of 'data'.

As loongson2_thermal_irq_thread() is the only place where 'tzd' is
needed and 'tzd' being local to the call site of
thermal_zone_device_register() and devm_request_threaded_irq(), there
is no need to store the pointer in the 'data' structure.

> > 
> > > +	int irq;
> > 'irq' won't be needed after taking into account the comments
> I will drop it.
> > > +	int id;
> > > +	void __iomem *regs;
> > > +	struct platform_device *pdev;
> > 'pdev' is not needed
> I will drop it.
> > 
> > > +	u16 ctrl_low_val;
> > > +	u16 ctrl_hi_val;
> > Those fields won't be needed after taking into account the comments
> I will drop it.
> > > +};
> > > +
> > > +static int loongson2_thermal_set(struct loongson2_thermal_data *data,
> > > +					int low, int high, bool enable)
> > > +{
> > > +	u64 reg_ctrl = 0;
> > > +	int reg_off = data->id * 2;
> > > +
> > > +	if (low > high)
> > > +		return -EINVAL;
> > > +
> > > +	low = max(low, -100);
> > > +	high = min(high, 155);

Documentation says -40, 125

> > > +	low += 100;
> > > +	high += 100;
> > Why are those values added to the low and high ? Did you mean low += 0x100 ?
> > 
> > Mind to describe a bit the register layout?
> 
> node(cpu) temp = Thens0_out -100,
> 
> low and high is record node temp, so low and high need add '100' as
> Thens0_out.

If I refer to the documentation it is a raw value converted from
centigrade. The function has degree.

So it should be:

temp_deci = temp_milli / 10

raw = temp_to_raw(temp_deci);

-> temp_to_raw to be determined from temp = (raw * 731) / 0x4000 - 273

[ ... ]

> > > +static int loongson2_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> > > +{
> > > +	u32 reg_val;
> > > +	struct loongson2_thermal_data *data = tz->devdata;
> > > +
> > > +	reg_val = readl(data->regs + LOONGSON2_TSENSOR_OUT);
> > > +	*temp = ((reg_val & 0xff) - 100) * 1000;
> > Why '-100' ?
> > 
> > Is the unit returned 'degrees' ?
> 
> node(cpu) temp = Thens0_out -100,
> 
> Here we need to get a node temp.

If I refer to the Loongson-3A5000 manual and assuming it is the right
one, the documentation says:

Temperature = Thens0_out * 731 / 0x4000 - 273

The unit is centigrade.

[ ... ]

> > > +	writeb(0xff, data->regs + LOONGSON2_TSENSOR_STATUS);
> > > +
> > > +	loongson2_thermal_set(data, 0, 0, false);
> > It would be nicer to use a reset line if it is available
> sorry, I don't get your meaning. Please describe more details about 'reset
> line'.

After a reset, the thermal controller should be in default state and the interrupt
flag cleared.

One example:

[1] https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/arch/arm64/boot/dts/nvidia/tegra210.dtsi#n1560
[2] https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/tegra/soctherm.c#n2169

Then search in the driver for:
     reset_control_assert(reset);
     reset_control_deassert(reset);


[ ... ]

Thanks

  -- Daniel

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog