Re: [PATCH v21 4/8] soc: mediatek: SVS: add monitor mode

[Roger Lu]

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