Received-SPF: pass (google.com: domain of linux-kernel+bounces-135496-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 8 Apr 2024 07:23:43 -0700
From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: "Zhang, Rui" <rui.zhang@intel.com>
Cc: "linux@roeck-us.net" <linux@roeck-us.net>,
	"Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
	"jdelvare@suse.com" <jdelvare@suse.com>,
	"srinivas.pandruvada@linux.intel.com" <srinivas.pandruvada@linux.intel.com>,
	"lukasz.luba@arm.com" <lukasz.luba@arm.com>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
	"daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Neri, Ricardo" <ricardo.neri@intel.com>
Subject: Re: [PATCH 1/3] thermal: intel: intel_tcc: Add model checks for
 temperature registers
Message-ID: <20240408142343.GA16542@ranerica-svr.sc.intel.com>
References: <20240406010416.4821-1-ricardo.neri-calderon@linux.intel.com>
 <20240406010416.4821-2-ricardo.neri-calderon@linux.intel.com>
 <5e86524413ec2cfeb1096f49851bf18837c7e50b.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5e86524413ec2cfeb1096f49851bf18837c7e50b.camel@intel.com>
User-Agent: Mutt/1.9.4 (2018-02-28)

On Sun, Apr 07, 2024 at 08:13:28AM +0000, Zhang, Rui wrote:
> > +
> > +#define TCC_FAM6_MODEL_TEMP_MASKS

Thank your your review, Rui!

> 
> Future non FAM6 processors can still use this macro, right?
> So I'd prefer to remove FAM6_MODEL in the macro name.

Yes, it is true, FAM6_MODEL it is restrictive and also not needed here.
I will update accodingly.
 
> [...]
> > 
> > +
> > +/**
> > + * get_tcc_offset_mask() - Returns the model-specific bitmask for
> > TCC offset
> > + *
> > + * Get the model-specific bitmask to extract TCC_OFFSET from the
> > MSR_TEMPERATURE_
> > + * TARGET register. If the mask is 0, it means the processor does
> > not support TCC offset.
> > + *
> > + * Return: The model-specific bitmask for TCC offset.
> > + */
> > +u32 get_tcc_offset_mask(void)
> > +{
> > +???????return intel_tcc_temp_masks.tcc_offset;
> > +}
> > +EXPORT_SYMBOL_NS(get_tcc_offset_mask, INTEL_TCC);
> 
> the name is not consistent with the other intel_tcc APIs.
> 
> how about intel_tcc_get_offset_mask()?

Sure. I can make this change.

> 
> [...]
> 
> > diff --git a/include/linux/intel_tcc.h b/include/linux/intel_tcc.h
> > index 8ff8eabb4a98..e281cf06aeab 100644
> > --- a/include/linux/intel_tcc.h
> > +++ b/include/linux/intel_tcc.h
> > @@ -14,5 +14,13 @@ int intel_tcc_get_tjmax(int cpu);
> > ?int intel_tcc_get_offset(int cpu);
> > ?int intel_tcc_set_offset(int cpu, int offset);
> > ?int intel_tcc_get_temp(int cpu, int *temp, bool pkg);
> > +#ifdef CONFIG_INTEL_TCC
> > +u32 get_tcc_offset_mask(void);
> > +u32 intel_tcc_get_temp_mask(bool pkg);
> > +#else
> > +static inline u32 get_tcc_offset_mask(void) { return 0; }
> > +/* Use the architectural bitmask of the temperature readout. No
> > model checks. */
> > +static inline u32 intel_tcc_get_temp_mask(bool pkg) { return 0x7f; }
> > +#endif
> 
> for intel_tcc_get_temp_mask()
>    1. with CONFIG_INTEL_TCC
>       a) for a platform in the model list, return the hardcoded value
>       b) for a platform not in the model list, return 0xff
>    2. without CONFIG_INTEL_TCC, return 0x7f
> 
> This is a bit confusing. IMO, at least we should leave a comment about
> this difference.

If we don't do model checks, I think we should rely on what is architectural
as per the SDM. Hence the 0x7f value.

Perhaps I can expand the comment in this hunk to detail what we do when we
do model checks.