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.
