| Message ID | 20230307154524.118541-2-bchihi@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | Add LVTS's AP thermal domain support for mt8195 | expand |
Il 07/03/23 16:45, bchihi@baylibre.com ha scritto: > From: Balsam CHIHI <bchihi@baylibre.com> > > Add AP Domain to LVTS thermal controllers dt-binding definition for mt8195. > > Signed-off-by: Balsam CHIHI <bchihi@baylibre.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On Wed, Mar 8, 2023 at 12:46 AM <bchihi@baylibre.com> wrote: > > From: Balsam CHIHI <bchihi@baylibre.com> > > Add AP Domain to LVTS thermal controllers dt-binding definition for mt8195. > > Signed-off-by: Balsam CHIHI <bchihi@baylibre.com> > --- > 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. ChenYu > +#define MT8195_AP_VPU1 9 > +#define MT8195_AP_GPU0 10 > +#define MT8195_AP_GPU1 11 > +#define MT8195_AP_VDEC 12 > +#define MT8195_AP_IMG 13 > +#define MT8195_AP_INFRA 14 > +#define MT8195_AP_CAM0 15 > +#define MT8195_AP_CAM1 16 > + > #endif /* __MEDIATEK_LVTS_DT_H */ > -- > 2.34.1 > >
On 09/03/2023 05:40, Chen-Yu Tsai wrote: > On Wed, Mar 8, 2023 at 12:46 AM <bchihi@baylibre.com> wrote: >> >> From: Balsam CHIHI <bchihi@baylibre.com> >> >> Add AP Domain to LVTS thermal controllers dt-binding definition for mt8195. >> >> Signed-off-by: Balsam CHIHI <bchihi@baylibre.com> >> --- >> 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.
On Thu, Mar 9, 2023 at 6:39 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 09/03/2023 05:40, Chen-Yu Tsai wrote: > > On Wed, Mar 8, 2023 at 12:46 AM <bchihi@baylibre.com> wrote: > >> > >> From: Balsam CHIHI <bchihi@baylibre.com> > >> > >> Add AP Domain to LVTS thermal controllers dt-binding definition for mt8195. > >> > >> Signed-off-by: Balsam CHIHI <bchihi@baylibre.com> > >> --- > >> 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. I see. I didn't realize the lookup namespace covered the whole platform. In that case, please ignore my request. ChenYu > 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 >
On Thu, Mar 09, 2023 at 11:39:13AM +0100, Daniel Lezcano wrote: > On 09/03/2023 05:40, Chen-Yu Tsai wrote: > > On Wed, Mar 8, 2023 at 12:46 AM <bchihi@baylibre.com> wrote: > > > > > > From: Balsam CHIHI <bchihi@baylibre.com> > > > > > > Add AP Domain to LVTS thermal controllers dt-binding definition for mt8195. > > > > > > Signed-off-by: Balsam CHIHI <bchihi@baylibre.com> > > > --- > > > 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. That's broken for how producer/consumer phandle+args bindings work. Rob
On Tue, 07 Mar 2023 16:45:21 +0100, bchihi@baylibre.com wrote: > From: Balsam CHIHI <bchihi@baylibre.com> > > Add AP Domain to LVTS thermal controllers dt-binding definition for mt8195. > > Signed-off-by: Balsam CHIHI <bchihi@baylibre.com> > --- > include/dt-bindings/thermal/mediatek,lvts-thermal.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > Acked-by: Rob Herring <robh@kernel.org>
Hi Rob, On 16/03/2023 23:35, Rob Herring wrote: > On Thu, Mar 09, 2023 at 11:39:13AM +0100, Daniel Lezcano wrote: >> On 09/03/2023 05:40, Chen-Yu Tsai wrote: >>> On Wed, Mar 8, 2023 at 12:46 AM <bchihi@baylibre.com> wrote: >>>> >>>> From: Balsam CHIHI <bchihi@baylibre.com> >>>> >>>> Add AP Domain to LVTS thermal controllers dt-binding definition for mt8195. >>>> >>>> Signed-off-by: Balsam CHIHI <bchihi@baylibre.com> >>>> --- >>>> 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. > > That's broken for how producer/consumer phandle+args bindings work. Do you mean this is broken for thermal zone description in the DT in general ? What would be the correct approach ?
On 07/03/2023 16:45, bchihi@baylibre.com wrote: > From: Balsam CHIHI <bchihi@baylibre.com> > > Add AP Domain to LVTS thermal controllers dt-binding definition for mt8195. > > Signed-off-by: Balsam CHIHI <bchihi@baylibre.com> > --- Applied, thanks
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 +#define MT8195_AP_VPU1 9 +#define MT8195_AP_GPU0 10 +#define MT8195_AP_GPU1 11 +#define MT8195_AP_VDEC 12 +#define MT8195_AP_IMG 13 +#define MT8195_AP_INFRA 14 +#define MT8195_AP_CAM0 15 +#define MT8195_AP_CAM1 16 + #endif /* __MEDIATEK_LVTS_DT_H */