Hi AngeloGioacchino,
Sorry for the late reply and thanks for the advice.
On Fri, 2022-01-07 at 15:34 +0100, AngeloGioacchino Del Regno wrote:
> Il 07/01/22 10:51, Roger Lu ha scritto:
> > SVS monitor mode is based on different thermal temperature
> > to provide suitable SVS bank voltages.
> >
> > Signed-off-by: Roger Lu <roger.lu@xxxxxxxxxxxx>
> > ---
> > drivers/soc/mediatek/mtk-svs.c | 253 ++++++++++++++++++++++++++++++++-
> > 1 file changed, 247 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> > index fc7e2ee44a92..042c6e8e9069 100644
> > --- a/drivers/soc/mediatek/mtk-svs.c
> > +++ b/drivers/soc/mediatek/mtk-svs.c
> > @@ -25,6 +25,7 @@
> > #include <linux/regulator/consumer.h>
> > #include <linux/slab.h>
> > #include <linux/spinlock.h>
> > +#include <linux/thermal.h>
> >
> > /* svs bank 1-line sw id */
> > #define SVSB_CPU_LITTLE BIT(0)
> > @@ -36,6 +37,7 @@
> > #define SVSB_MODE_ALL_DISABLE 0
> > #define SVSB_MODE_INIT01 BIT(1)
> > #define SVSB_MODE_INIT02 BIT(2)
> > +#define SVSB_MODE_MON BIT(3)
[snip]
> > /**
> > @@ -241,6 +254,7 @@ struct svs_platform {
> > * @get_volts: function pointer to get bank voltages
> > * @name: bank name
> > * @buck_name: regulator name
> > + * @tzone_name: thermal zone name
> > * @suspended: suspend flag of this bank
> > * @phase: bank current phase
> > * @volt_od: bank voltage overdrive
> > @@ -270,6 +284,13 @@ struct svs_platform {
> > * @sw_id: bank software identification
> > * @cpu_id: cpu core id for SVS CPU bank use only
> > * @ctl0: TS-x selection
> > + * @temp: bank temperature
> > + * @tzone_htemp: thermal zone high temperature threshold
> > + * @tzone_htemp_voffset: thermal zone high temperature voltage offset
> > + * @tzone_ltemp: thermal zone low temperature threshold
> > + * @tzone_ltemp_voffset: thermal zone low temperature voltage offset
> > + * @bts: svs efuse data
> > + * @mts: svs efuse data
> > * @bdes: svs efuse data
> > * @mdes: svs efuse data
> > * @mtdes: svs efuse data
> > @@ -292,6 +313,7 @@ struct svs_bank {
> > void (*get_volts)(struct svs_platform *svsp);
> > char *name;
> > char *buck_name;
> > + char *tzone_name;
> > bool suspended;
> > enum svsb_phase phase;
> > s32 volt_od;
> > @@ -321,6 +343,13 @@ struct svs_bank {
> > u32 sw_id;
> > u32 cpu_id;
> > u32 ctl0;
> > + u32 temp;
> > + u32 tzone_htemp;
> > + u32 tzone_htemp_voffset;
> > + u32 tzone_ltemp;
> > + u32 tzone_ltemp_voffset;
> > + u32 bts;
> > + u32 mts;
> > u32 bdes;
> > u32 mdes;
> > u32 mtdes;
> > @@ -361,10 +390,21 @@ static u32 svs_bank_volt_to_opp_volt(u32 svsb_volt,
> > u32 svsb_volt_step,
> > return (svsb_volt * svsb_volt_step) + svsb_volt_base;
> > }
> >
>
> I'm sorry for the double review, but this went unnoticed in the previous one.
>
> > +static int svs_get_zone_temperature(const char *tzone_name, int
> > *tzone_temp)
> > +{
> > + struct thermal_zone_device *tzd;
> > +
> > + tzd = thermal_zone_get_zone_by_name(tzone_name);
>
> This call is expensive, as it's iterating through the (possibly) entire
> thermal_tz_list (drivers/thermal/thermal_core.c) so, for performance purposes,
> noting that you're using this in svs_adjust_pm_opp_volts(), it's not a good
> idea
> to call it at every ISR.
>
> I would instead propose to get a pointer to the thermal_zone at driver probe
> time and cache that in struct svs_bank: this function would also be removed
> as the only thing that you'd need to do then would be just one call...
>
> [read forward...]
No problem. I'll cache thermal_zone at driver probe time and remove this API in
the next patch. Thanks.
>
> > + if (IS_ERR(tzd))
> > + return PTR_ERR(tzd);
> > +
> > + return thermal_zone_get_temp(tzd, tzone_temp);
> > +}
> > +
> > static int svs_adjust_pm_opp_volts(struct svs_bank *svsb, bool
> > force_update)
> > {
> > - int ret = -EPERM;
> > - u32 i, svsb_volt, opp_volt;
> > + int ret = -EPERM, tzone_temp = 0;
> > + u32 i, svsb_volt, opp_volt, temp_voffset = 0;
> >
> > mutex_lock(&svsb->lock);
> >
> > @@ -378,6 +418,22 @@ static int svs_adjust_pm_opp_volts(struct svs_bank
> > *svsb, bool force_update)
> > goto unlock_mutex;
> > }
> >
> > + /* Get thermal effect */
> > + if (svsb->phase == SVSB_PHASE_MON) {
> > + ret = svs_get_zone_temperature(svsb->tzone_name, &tzone_temp);
>
> ... so you can simply call ...
>
>
> ret = thermal_zone_get_temp(svsb->tzd, tzone_temp);
>
>
> ...without any need for any helper.
Sure, I'll call thermal_zone_get_temp() directly after applying this recommended
change in the next patch. Thanks.
>
> > + if (ret || (svsb->temp > SVSB_TEMP_UPPER_BOUND &&
> > + svsb->temp < SVSB_TEMP_LOWER_BOUND)) {
> > + dev_err(svsb->dev, "%s: %d (0x%x), run default volts\n",
> > + svsb->tzone_name, ret, svsb->temp);
> > + svsb->phase = SVSB_PHASE_ERROR;
> > + }
> > +
> > + if (tzone_temp >= svsb->tzone_htemp)
> > + temp_voffset += svsb->tzone_htemp_voffset;
> > + else if (tzone_temp <= svsb->tzone_ltemp)
> > + temp_voffset += svsb->tzone_ltemp_voffset;
> > + }
> > +
> > /* vmin <= svsb_volt (opp_volt) <= default opp voltage */
> > for (i = 0; i < svsb->opp_count; i++) {
> > switch (svsb->phase) {
>
> Apart from that, the commit looks good. Looking forward to review the new
> version!
>
> Regards,
> - Angelo
