Message-ID: <Z1n4h/AxzthB0V9l@lizhi-Precision-Tower-5810>
Date: Wed, 11 Dec 2024 15:39:35 -0500
From: Frank Li <Frank.li@....com>
To: Marco Felsch <m.felsch@...gutronix.de>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Zhang Rui <rui.zhang@...el.com>, Lukasz Luba <lukasz.luba@....com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Pengfei Li <pengfei.li_1@....com>, linux-pm@...r.kernel.org,
	devicetree@...r.kernel.org, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Peng Fan <peng.fan@....com>
Subject: Re: [PATCH v2 2/2] thermal: imx91: Add support for i.MX91 thermal
 monitoring unit

On Wed, Dec 11, 2024 at 07:57:05PM +0100, Marco Felsch wrote:
> On 24-12-11, Frank Li wrote:
> > On Wed, Dec 11, 2024 at 04:46:22PM +0100, Marco Felsch wrote:
> > > On 24-12-10, Frank Li wrote:
> > > > From: Pengfei Li <pengfei.li_1@....com>
> > > >
> > > > Introduce support for the i.MX91 thermal monitoring unit, which features a
> > > > single sensor for the CPU. The register layout differs from other chips,
> > > > necessitating the creation of a dedicated file for this.
> > > >
> > > > Signed-off-by: Pengfei Li <pengfei.li_1@....com>
> > > > Signed-off-by: Peng Fan <peng.fan@....com>
> > > > Signed-off-by: Frank Li <Frank.Li@....com>
> > > > ---
> > > > change from v1 to v2
> > > > - use low case for hexvalue
> > > > - combine struct imx91_tmu and tmu_sensor
> > > > - simplify imx91_tmu_start() and imx91_tmu_enable()
> > > > - use s16 for imx91_tmu_get_temp(), which may negative value
> > > > - use reverse christmas tree style
> > > > - use run time pm
> > > > - use oneshot to sample temp
> > > > - register thermal zone after hardware init
> > > > ---
> > > >  drivers/thermal/Kconfig         |  10 ++
> > > >  drivers/thermal/Makefile        |   1 +
> > > >  drivers/thermal/imx91_thermal.c | 265 ++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 276 insertions(+)
> > > >

[...]

> > > > +	platform_set_drvdata(pdev, tmu);
> > > > +
> > > > +	/* disable the monitor during initialization */
> > > > +	imx91_tmu_enable(tmu, false);
> > > > +	imx91_tmu_start(tmu, false);
> > >
> > > No need to disable it here since both bits (ENABLE and START) are 0
> > > after a reset.
> >
> > Maybe uboot enable it. We can't depend on reset value without really set
> > hardware reset bit.
>
> So the module can't be rested e.g. by disabling module power? Having a
> dedicated reset mechanism would be much simpler instead of writing each
> reg-field to the default value.

Not all module can be power off individual. Normally, it should have sw
reset bit in controller. but I have not find it in this module.

>
> > > > +	ret = imx91_init_from_nvmem_cells(tmu);
> > > > +	if (ret) {
> > > > +		writel_relaxed(DEFAULT_TRIM1_CONFIG, tmu->base + TRIM1);
> > > > +		writel_relaxed(DEFAULT_TRIM2_CONFIG, tmu->base + TRIM2);
> > > 			^
> > > Can you please anwer if _relaxed API is sufficient? I don't know why you
> > > making use of the _relaxed API here anyway. We have only a few MMIO
> > > accesses here, so why can't we use the writel() instead? This applies to
> > > the whole driver.
> >
> > There are not big difference writel_relaxed() or writel() for this driver.
> > Just original owner pick one.
>
> NACK, the difference is that _relaxed() APIs don't guarantee the order
> the register access is done.

It is totally wrong. *_relexed() API can guarantee the register access
order. Only can't guarantee order with other memory. such as

1 writel_related(A, 0)
2. x = 3
3 writel_related(B, 1)

Hardware/memory model, require B=1 must be after A=0, but not necessary
after x=3, that's means (2) x=3 may happen after 3.

if use writel(), guarantee 1, 2, 3 as order.  Here have not DMA involved.
So not any impact if (2) after (3).

Typically, only below case have to use writel().
2 (update DMA descritpor), 3 start DMA,

Additional, writel() also can't guarantee register access is done!!!!

writel() just means you send a write command to bus. Not guarantee to reach
target module. typical case is

writel(A, 0);
udelay(1);
writel(A, 1);

The interval between A=1 and A=0 may less than 1us if timer have not use
MMIO register. (arm system counter).

In this driver, the only valuable benefit by using writel() instead of
writel_relexed() is "short function name".

>
> > > > +	}
> > > > +
> > > > +	/* The typical conv clk is 4MHz, the output freq is 'rate / (div + 1)' */
> > > > +	rate = clk_get_rate(tmu->clk);
> > > > +	div = (rate / 4000000) - 1;
> > > > +	if (div > FIELD_GET(DIV_MASK, DIV_MASK))
> > > 			^
> > > This misuse the FIELD_GET() API. Instead please add a define e.g. DIV_MAX.
> >
> > I don't think so, It avoid define another macro DIV_MAX, which may miss
> > defined, the related marco should come from one source.
> >
> > For example:
> >
> > DIV_MASK is GENMASK(23, 16),  DIV_MAX is 256. But if hardware upgrade,
> > DIV_MASK to GENMASK(24, 16), DIV_MAX is quite easy to forget update it and
> > hard to find such mis-match when div value < 256.
>
> We not talking about "possible" other HW. For now it's just this one and
> using FIELD_GET() this way is seems odd, at least to me.

If you look implement of FIELD_GET()
(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));, it will be
reasonable.

Hardware IP may update in future, bitwidth of field may expand.

>
> > > > +		return -EINVAL;
> > > 			^
> > > 		dev_err_probe()
> > > > +
> > > > +	/* Set divider value and enable divider */
> > > > +	writel_relaxed(DIV_EN | FIELD_PREP(DIV_MASK, div), tmu->base + REF_DIV);
> > > > +
> > > > +	/* Set max power up delay: 'Tpud(ms) = 0xFF * 1000 / 4000000' */
> > > > +	writel_relaxed(FIELD_PREP(PUDL_MASK, 100U), tmu->base + PUD_ST_CTRL);
> > > 		^
> > > You dont need to repeat the default value, so this line can be dropped.
> > >
> > > > +
> > > > +	/*
> > > > +	 * Set resolution mode
> > > > +	 * 00b - Conversion time = 0.59325 ms
> > > > +	 * 01b - Conversion time = 1.10525 ms
> > > > +	 * 10b - Conversion time = 2.12925 ms
> > > > +	 * 11b - Conversion time = 4.17725 ms
> > > > +	 */
> > > > +	writel_relaxed(FIELD_PREP(CTRL1_RES_MASK, 0x3), tmu->base + CTRL1_CLR);
> > > > +	writel_relaxed(FIELD_PREP(CTRL1_RES_MASK, 0x1), tmu->base + CTRL1_SET);
> > >
> > > Same here, you repeat the module default after reset, so please drop it.
> > >
> > > > +	writel_relaxed(CTRL1_MEAS_MODE_MASK, tmu->base + CTRL1_CLR);
> > > > +	writel_relaxed(FIELD_PREP(CTRL1_MEAS_MODE_MASK, CTRL1_MEAS_MODE_SINGLE),
> > > > +		       tmu->base + CTRL1_SET);
> > > > +
> > > > +	clk_disable_unprepare(tmu->clk);
> > >
> > > Drop this, and
> > >
> > > > +	pm_runtime_set_suspended(dev);
> > >
> > > replace this with: pm_runtime_set_active();
> >
> > No big difference, if set_active, we need add Enable TMU here. I can
> > change to set_active.
>
> You don't need to manually disable the clock, it would be done by the
> runtime-pm.

Yes, runtime-pm can disable clock, but it need align harware and run time
state before enable runtime pm.

There are two options
1 disable all hardware resources and pm_runtime_set_suspended()
2. enable all hardware resources and set_active()

After algned, use runtime pm API to controller it.

I choose option1, you prefer option 2.

Frank
>
> Regards,
>   Marco
>
>
[...]
> > > > +
> > > > +MODULE_AUTHOR("Peng Fan <peng.fan@....com>");
> > > > +MODULE_DESCRIPTION("i.MX91 Thermal Monitor Unit driver");
> > > > +MODULE_LICENSE("GPL");
> > > >
> > > > --
> > > > 2.34.1
> > > >
> > > >
> >

Powered by blists - more mailing lists