[Intel-gfx] [RFC-v19 02/13] drm/i915/pxp: set KCR reg init during the boot time

[Vivi, Rodrigo]

On Mon, 2021-01-11 at 21:38 +0000, Huang, Sean Z wrote:
> Hello,
> 
> I see, based on Joonas and Rodrigo's feedback.
> 
> I made the modification as below, I will still keep the macro in this
> .c instead of i915_reg.h, and this change will be reflected in rev20.
> 
> /* KCR register definitions */
>  #define KCR_INIT            _MMIO(0x320f0)
> -#define KCR_INIT_MASK_SHIFT (16)
> +
>  /* Setting KCR Init bit is required after system boot */
> -#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES (BIT(14) | (BIT(14) <<
> KCR_INIT_MASK_SHIFT))
> +#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES (BIT(14) | (BIT(14) << 16))

This is not what I asked actually.

I asked to get the BIT(14) to be defined separated, shift defined as
well... and the | and actuall shift operations to be performed in the
code and not in the defines

> 
> Best regards,
> Sean
> 
> -----Original Message-----
> From: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Sent: Friday, January 8, 2021 3:31 AM
> To: Huang, Sean Z <sean.z.huang at intel.com>; 
> Intel-gfx at lists.freedesktop.org; Vivi, Rodrigo <
> rodrigo.vivi at intel.com>
> Subject: Re: [Intel-gfx] [RFC-v19 02/13] drm/i915/pxp: set KCR reg
> init during the boot time
> 
> Quoting Vivi, Rodrigo (2021-01-07 17:31:36)
> > On Wed, 2021-01-06 at 15:12 -0800, Huang, Sean Z wrote:
> > > Set the KCR init during the boot time, which is required by
> > > hardware, to allow us doing further protection operation such as
> > > sending commands to GPU or TEE.
> > > 
> > > Signed-off-by: Huang, Sean Z <sean.z.huang at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/pxp/intel_pxp.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > index 9bc3c7e30654..f566a4fda044 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > @@ -6,6 +6,12 @@
> > >  #include "intel_pxp.h"
> > >  #include "intel_pxp_context.h"
> > > 
> > > +/* KCR register definitions */
> > 
> > please define this in i915_reg.h
> 
> Generally the trend on the GT side is to contain in a .c file if
> there are no shared users like these. So they should be at this spot,
> yet the rest of the review comments apply.
> 
> The spurious comments should be dropped and like Rodrigo pointed out,
> we should be using the appropriate macros for a masked writes, not
> baking in the #define.
> 
> Regards, Joonas