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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 29 January 2018 at 16:01, Thomas Abraham <thomas.abraham@arm.com> wrote:
> 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!.
>

Likewise

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>