Re: [PATCH 1/4] dt-bindings: thermal: mediatek: Add AP domain to LVTS thermal controllers for mt8195

[Daniel Lezcano]

On 09/03/2023 05:40, Chen-Yu Tsai wrote:
> On Wed, Mar 8, 2023 at 12:46 AM <bchihi@xxxxxxxxxxxx> wrote:
> > From: Balsam CHIHI <bchihi@xxxxxxxxxxxx>
> >
> > Add AP Domain to LVTS thermal controllers dt-binding definition for mt8195.
> >
> > Signed-off-by: Balsam CHIHI <bchihi@xxxxxxxxxxxx>
> > ---
> >   include/dt-bindings/thermal/mediatek,lvts-thermal.h | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> >
> > diff --git a/include/dt-bindings/thermal/mediatek,lvts-thermal.h b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
> > index c09398920468..8fa5a46675c4 100644
> > --- a/include/dt-bindings/thermal/mediatek,lvts-thermal.h
> > +++ b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
> > @@ -16,4 +16,14 @@
> >   #define MT8195_MCU_LITTLE_CPU2  6
> >   #define MT8195_MCU_LITTLE_CPU3  7
> >
> > +#define MT8195_AP_VPU0  8
>
> Can't this start from 0? This is a different hardware block. The index
> namespace is separate. Same question for MT8192.

The ID is used to differentiate the thermal zone identifier in the 
device tree from the driver.

+		vpu0-thermal {
+			polling-delay = <0>;
+			polling-delay-passive = <0>;
+			thermal-sensors = <&lvts_ap MT8195_AP_VPU0>;
+
+			trips {
+				vpu0_crit: trip-crit {
+					temperature = <100000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};

If MT8195_AP_VPU0 is 0, then the code won't be able to differentiate 
MT8195_AP_VPU0 and MT8195_MCU_BIG_CPU0

The LVTS driver will call devm_thermal_of_zone_register() with the 
sensor id. If MT8195_MCU_BIG_CPU0 and MT8195_AP_VPU0 have the same id, 
then at the moment of registering the MT8195_AP_VPU0, the underlying OF 
thermal framework code will use MT8195_MCU_BIG_CPU0 description instead 
because it will be the first to be find in the DT.

If MT8195_AP_VPU0 is described in DT before, then the same will happen 
when registering MT8195_MCU_BIG_CPU0, MT8195_AP_VPU0 will be registered 
instead.

IOW all ids must be different.

The namespace is already described by the macro name AFAICS, so whatever 
the values, we see only the macro names and those IDs are private the 
kernel implementation.

If the numbering is really important, may be something like:

#define MT8195_MCU_BIG_CPU0     00
#define MT8195_MCU_BIG_CPU1     01
#define MT8195_MCU_BIG_CPU2     02
#define MT8195_MCU_BIG_CPU3     03
#define MT8195_MCU_LITTLE_CPU0  04
#define MT8195_MCU_LITTLE_CPU1  05
#define MT8195_MCU_LITTLE_CPU2  06
#define MT8195_MCU_LITTLE_CPU3  07

#define MT8195_AP_VPU1  10
#define MT8195_AP_GPU0  11
#define MT8195_AP_GPU1  12
#define MT8195_AP_VDEC  13
#define MT8195_AP_IMG   14
#define MT8195_AP_INFRA 15
#define MT8195_AP_CAM0  16
#define MT8195_AP_CAM1  17

But I would suggest considering this change as a separate patch after 
the AP domain is added.


--
<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