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

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

[Thomas Abraham]

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/PL111LcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
index 3f3ceb3..89ba130 100644
--- a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
+++ b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.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);
--
2.7.4

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.

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

[Thomas Abraham]

Hi Leif,

> On Fri, Jan 26, 2018 at 10:34 PM, Thomas Abraham <thomas.abraham@arm.com> 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/PL111LcdAr
> +++ mVExpress.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);
> --

Sorry, I don't mean to be pushy -- just wanted to know if there are any comments on this patch. This patch fixes a check on system ID register value without which an assert is triggered on FVP platforms. This fix is also required for an upcoming release for fvp platforms.

Thanks,
Thomas.
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.

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

[Leif Lindholm]

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/PL111LcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> index 3f3ceb3..89ba130 100644
> --- a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> +++ b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.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.

/
    Leif

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

[Leif Lindholm]

On Mon, Jan 29, 2018 at 01:12:35PM +0000, Thomas Abraham wrote:
> Sorry, I don't mean to be pushy -- just wanted to know if there are
> any comments on this patch.

I don't mind the occasional prod, especially when I forget to respond
quickly to something I've promised to do so on in a side discussion :)

/
    Leif

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

[Thomas Abraham]

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.

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

[Ard Biesheuvel]

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>

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

[Leif Lindholm]

On Mon, Jan 29, 2018 at 04:13:40PM +0000, Ard Biesheuvel wrote:
> 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>

Thanks.

Pushed as cf8aef9c96 with
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
[Reworded adjacent code comments.]
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
and Ard's ACK.

/
    Leif