On Tue, 12 Jan 2021, "Vivi, Rodrigo" <rodrigo.vivi@xxxxxxxxx> wrote: > 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) Useless parenthesis. >> + >> /* 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)) BIT(14) << 16 is actually BIT(14+16), or BIT(30). The above is pointless. Also, you'll end up with problems by using BIT() instead of REG_BIT() defined in i915_reg.h due to BIT() using unsigned long. Also, read the big style comment near the top of i915_reg.h. BR, Jani. > > 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@xxxxxxxxxxxxxxx> >> Sent: Friday, January 8, 2021 3:31 AM >> To: Huang, Sean Z <sean.z.huang@xxxxxxxxx>; >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vivi, Rodrigo < >> rodrigo.vivi@xxxxxxxxx> >> Subject: Re: [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@xxxxxxxxx> >> > > --- >> > > 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 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
