Re: [PATCH] Platform/ARM/VExpress: refine the check for DVI support

[Thomas Abraham <thomas.abraham@arm.com>]

Hi Leif,

> On Mon, Jan 29, 2018 at 8:21 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Jan 26, 2018 at 10:34:59PM +0530, Thomas Abraham wrote:
> > The base models could have different values for the revision ID field
> > in the System ID register. Base models do not have support for DVI and
> > so the revision ID field should also be masked out when checking for
> > the presence of DVI support.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Thomas Abraham <thomas.abraham@arm.com>
> > ---
> >  .../VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c    |
> 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git
> >
> a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmV
> > Express.c
> >
> b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmV
> > Express.c
> > index 3f3ceb3..89ba130 100644
> > ---
> >
> a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmV
> > Express.c
> > +++
> b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111Lcd
> > +++ ArmVExpress.c
> > @@ -264,7 +264,7 @@ LcdPlatformSetMode (
> >    SysId = MmioRead32 (ARM_VE_SYS_ID_REG);
> >    if (SysId != ARM_RTSM_SYS_ID) {
> >      // Take out the FVP GIC variant to reduce the permutations.
> > -    SysId &= ~ARM_FVP_SYS_ID_VARIANT_MASK;
> > +    SysId &= ~(ARM_FVP_SYS_ID_VARIANT_MASK |
> > + ARM_FVP_SYS_ID_REV_MASK);
> >      if (SysId != ARM_FVP_BASE_BOARD_SYS_ID) {
> >        // Set the DVI into the new mode
> >        Status = ArmPlatformSysConfigSet (SYS_CFG_DVIMODE,
> > mResolutions[ModeNumber].Mode);
>
> I think this change makes sense (and should arguably have been there from the
> start). But I also think it highlights that the surrounding comments are incorrect,
> and get increasingly confusing with this modification.
>
> I would suggest that
>   // On the FVP models the GIC variant in encoded in bits [15:12].
> is replaced by
>   // On the FVP models, the build variant is encoded in bits [15:12].
>
> and that
>   // Take out the FVP GIC variant to reduce the permutations.
> is replaced by
>   // Ignore build variant and revision.
>
> If you're happy with that, I can fold these changes to the surrounding comments
> in and commit.

Yes, happy to have these changes folded in. Thank you!.

Thomas.



>
> /
>     Leif
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.