1. Patchew
  2. EDK2
  3. View series

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

Thomas Abraham posted 1 patch 6 years, 11 months ago
Failed in applying to current master (apply log)
.../VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c    | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[edk2] [PATCH] Platform/ARM/VExpress: refine the check for DVI support
Posted by Thomas Abraham 6 years, 11 months ago 
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] Platform/ARM/VExpress: refine the check for DVI support
Posted by Thomas Abraham 6 years, 10 months ago 
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] Platform/ARM/VExpress: refine the check for DVI support
Posted by Leif Lindholm 6 years, 10 months ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] Platform/ARM/VExpress: refine the check for DVI support
Posted by Leif Lindholm 6 years, 10 months ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] Platform/ARM/VExpress: refine the check for DVI support
Posted by Thomas Abraham 6 years, 10 months ago 
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] Platform/ARM/VExpress: refine the check for DVI support
Posted by Ard Biesheuvel 6 years, 10 months ago 
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>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] Platform/ARM/VExpress: refine the check for DVI support
Posted by Leif Lindholm 6 years, 10 months ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.