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@xxxxxxx>
> > > >
> > > > 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@xxxxxxx>
> > > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > > > ---
> > > > 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@xxxxxxx>");
> > > > +MODULE_DESCRIPTION("i.MX91 Thermal Monitor Unit driver");
> > > > +MODULE_LICENSE("GPL");
> > > >
> > > > --
> > > > 2.34.1
> > > >
> > > >
> >
