On Thu, 22 Sep 2022 00:08:46 -0700, Gupta, Anshuman wrote:
>
Hi Anshuman,
> On 9/21/2022 8:23 PM, Nilawar, Badal wrote:
> >
> > On 21-09-2022 17:15, Gupta, Anshuman wrote:
> >>
> >>> +static int
> >>> +hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
> >>> +{
> >>> + struct i915_hwmon *hwmon = ddat->hwmon;
> >>> +
> >>> + switch (attr) {
> >>> + case hwmon_power_max:
> >>> + *val = hwm_field_read_and_scale(ddat,
> >>> + hwmon->rg.pkg_rapl_limit,
> >>> + PKG_PWR_LIM_1,
> >>> + hwmon->scl_shift_power,
> >>> + SF_POWER);
> >>> + return 0;
> >>> + case hwmon_power_rated_max:
> >>> + *val = hwm_field_read_and_scale(ddat,
> >>> + hwmon->rg.pkg_power_sku,
> >>> + PKG_PKG_TDP,It seems a dead code,
> >>> pkg_power_sky register in initialized with
> >> INVALID_MMMIO_REG, why are we exposing this, unless i am missing
> >> something ?
> > Agree that for platforms considered in this series does not support
> > hwmon_power_rated_max. In fact hwm_power_is_visible will not allow to
> > create sysfs entry if pkg_power_sku is not supported. Considering future
> > dgfx platforms we didn't remove this entry. In future for supported
> > platforms we just need to assign valid register to pkg_power_sku.
>
> AFAIU PACKAGE_POWER_SKU reg is valid for both DG1 and DG2 from BSpec:51862
> So we need to define the register.
> See once more comment below,
Thanks for pointing out, I didn't know where to look for it. We will add
it. Thanks to Badal for locating the register too.
> >
> > Regards,
> > Badal
> >> Br,
> >> Anshuman.
> >>> + hwmon->scl_shift_power,
> >>> + SF_POWER);
> >>> + return 0;
> >>> + default:
> >>> + return -EOPNOTSUPP;
> >>> + }
> >>> +}
> >>> +
> >>> +static int
> /snip
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >>> b/drivers/gpu/drm/i915/i915_reg.h
> >>> index 1a9bd829fc7e..55c35903adca 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -1807,6 +1807,11 @@
> >>> #define POWER_LIMIT_1_MASK REG_BIT(10)
> >>> #define POWER_LIMIT_2_MASK REG_BIT(11)
> >>> +/*
> >>> + * *_PACKAGE_POWER_SKU - SKU power and timing parameters.
> >>> + */
> >>> +#define PKG_PKG_TDP GENMASK_ULL(14, 0)
> Define register above this definition, GENMASK should follow
> by a register.
Will do.
Thanks.
--
Ashutosh
