Re: [PATCH 1/3] thermal: intel: intel_tcc: Add model checks for temperature registers

[Ricardo Neri]

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.