Re: [PATCH v3 09/16] ArmPlatformPkg: Redefine LcdPlatformGetTimings function

[Evan Lloyd <Evan.Lloyd@arm.com>]

Reviewed-by: Evan Lloyd <evan.lloyd@arm.com>

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Girish
> Pathak
> Sent: 20 March 2018 16:12
> To: edk2-devel@lists.01.org
> Cc: nd <nd@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-
> Fitt@arm.com>; leif.lindholm@linaro.org; ard.biesheuvel@linaro.org
> Subject: [edk2] [PATCH v3 09/16] ArmPlatformPkg: Redefine
> LcdPlatformGetTimings function
> 
> From: Girish Pathak <girish.pathak at arm.com>
> 
> The LcdPlatformGetTimings interface function takes similar sets of multiple
> parameters for horizontal and vertical timings which can be aggregated in a
> common data type. This change defines a structure SCAN_TIMINGS for this
> which can be used to describe both horizontal and vertical scan timings,
> and accordingly redefines the LcdPlatformGetTiming interface, greatly
> reducing the amount of data passed about.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPlatformPkg/Include/Library/LcdPlatformLib.h                | 31 ++++++----
> --
>  ArmPlatformPkg/Library/HdLcd/HdLcd.c                           | 50 +++++++++----
> -------
>  ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.c | 10 +---
>  ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c                     | 43
> +++++++++--------
>  4 files changed, 64 insertions(+), 70 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> b/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> index
> e51e78640ae7b1acd51ac333ba3faa8c78aea5a5..8338b327fd2dd0d6b316
> 53e278e25da5ac850939 100644
> --- a/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> +++ b/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> @@ -153,6 +153,14 @@ typedef enum {
>    LCD_BITS_PER_PIXEL_12_444
>  } LCD_BPP;
> 
> +// Display timing settings.
> +typedef struct {
> +  UINT32                      Resolution;
> +  UINT32                      Sync;
> +  UINT32                      BackPorch;
> +  UINT32                      FrontPorch;
> +} SCAN_TIMINGS;
> +
>  /** Platform related initialization function.
> 
>    @param[in] Handle              Handle to the LCD device instance.
> @@ -228,14 +236,11 @@ LcdPlatformQueryMode (
> 
>    @param[in]  ModeNumber          Mode Number.
> 
> -  @param[out] HRes                Pointer to horizontal resolution.
> -  @param[out] HSync               Pointer to horizontal sync width.
> -  @param[out] HBackPorch          Pointer to horizontal back porch.
> -  @param[out] HFrontPorch         Pointer to horizontal front porch.
> -  @param[out] VRes                Pointer to vertical resolution.
> -  @param[out] VSync               Pointer to vertical sync width.
> -  @param[out] VBackPorch          Pointer to vertical back porch.
> -  @param[out] VFrontPorch         Pointer to vertical front porch.
> +  @param[out] Horizontal          Pointer to horizontal timing parameters.
> +                                  (Resolution, Sync, Back porch, Front porch)
> +  @param[out] Vertical            Pointer to vertical timing parameters.
> +                                  (Resolution, Sync, Back porch, Front
> + porch)
> +
> 
>    @retval EFI_SUCCESS             Display timing information for the requested
>                                    mode returned successfully.
> @@ -244,14 +249,8 @@ LcdPlatformQueryMode (  EFI_STATUS
> LcdPlatformGetTimings (
>    IN  UINT32                              ModeNumber,
> -  OUT UINT32*                             HRes,
> -  OUT UINT32*                             HSync,
> -  OUT UINT32*                             HBackPorch,
> -  OUT UINT32*                             HFrontPorch,
> -  OUT UINT32*                             VRes,
> -  OUT UINT32*                             VSync,
> -  OUT UINT32*                             VBackPorch,
> -  OUT UINT32*                             VFrontPorch
> +  OUT SCAN_TIMINGS                        **Horizontal,
> +  OUT SCAN_TIMINGS                        **Vertical
>    );
> 
>  /** Return bits per pixel information for a mode number.
> diff --git a/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> b/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> index
> 039048398c531ec944bc4b43a5551a554a368481..f5886848ce582b475b5
> 97ccca015c816707ade0e 100644
> --- a/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> +++ b/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> @@ -98,34 +98,25 @@ LcdSetMode (
>    )
>  {
>    EFI_STATUS        Status;
> -  UINT32            HRes;
> -  UINT32            HSync;
> -  UINT32            HBackPorch;
> -  UINT32            HFrontPorch;
> -  UINT32            VRes;
> -  UINT32            VSync;
> -  UINT32            VBackPorch;
> -  UINT32            VFrontPorch;
> +  SCAN_TIMINGS      *Horizontal;
> +  SCAN_TIMINGS      *Vertical;
>    UINT32            BytesPerPixel;
>    LCD_BPP           LcdBpp;
> 
>    // Set the video mode timings and other relevant information
>    Status = LcdPlatformGetTimings (
>               ModeNumber,
> -             &HRes,
> -             &HSync,
> -             &HBackPorch,
> -             &HFrontPorch,
> -             &VRes,
> -             &VSync,
> -             &VBackPorch,
> -             &VFrontPorch
> +             &Horizontal,
> +             &Vertical
>               );
>    if (EFI_ERROR (Status)) {
>      ASSERT_EFI_ERROR (Status);
>      return Status;
>    }
> 
> +  ASSERT (Horizontal != NULL);
> +  ASSERT (Vertical != NULL);
> +
>    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
>    if (EFI_ERROR (Status)) {
>      ASSERT_EFI_ERROR (Status);
> @@ -138,21 +129,26 @@ LcdSetMode (
>    MmioWrite32 (HDLCD_REG_COMMAND, HDLCD_DISABLE);
> 
>    // Update the frame buffer information with the new settings
> -  MmioWrite32 (HDLCD_REG_FB_LINE_LENGTH, HRes * BytesPerPixel);
> -  MmioWrite32 (HDLCD_REG_FB_LINE_PITCH,  HRes * BytesPerPixel);
> -  MmioWrite32 (HDLCD_REG_FB_LINE_COUNT,  VRes - 1);
> +  MmioWrite32 (
> +    HDLCD_REG_FB_LINE_LENGTH,
> +    Horizontal->Resolution * BytesPerPixel
> +    );
> +
> +  MmioWrite32 (HDLCD_REG_FB_LINE_PITCH, Horizontal->Resolution *
> + BytesPerPixel);
> +
> +  MmioWrite32 (HDLCD_REG_FB_LINE_COUNT, Vertical->Resolution - 1);
> 
>    // Set the vertical timing information
> -  MmioWrite32 (HDLCD_REG_V_SYNC,         VSync);
> -  MmioWrite32 (HDLCD_REG_V_BACK_PORCH,   VBackPorch);
> -  MmioWrite32 (HDLCD_REG_V_DATA,         VRes - 1);
> -  MmioWrite32 (HDLCD_REG_V_FRONT_PORCH,  VFrontPorch);
> +  MmioWrite32 (HDLCD_REG_V_SYNC,        Vertical->Sync);
> +  MmioWrite32 (HDLCD_REG_V_BACK_PORCH,  Vertical->BackPorch);
> +  MmioWrite32 (HDLCD_REG_V_DATA,        Vertical->Resolution - 1);
> +  MmioWrite32 (HDLCD_REG_V_FRONT_PORCH, Vertical->FrontPorch);
> 
>    // Set the horizontal timing information
> -  MmioWrite32 (HDLCD_REG_H_SYNC,         HSync);
> -  MmioWrite32 (HDLCD_REG_H_BACK_PORCH,   HBackPorch);
> -  MmioWrite32 (HDLCD_REG_H_DATA,         HRes - 1);
> -  MmioWrite32 (HDLCD_REG_H_FRONT_PORCH,  HFrontPorch);
> +  MmioWrite32 (HDLCD_REG_H_SYNC,        Horizontal->Sync);
> +  MmioWrite32 (HDLCD_REG_H_BACK_PORCH,  Horizontal->BackPorch);
> +  MmioWrite32 (HDLCD_REG_H_DATA,        Horizontal->Resolution - 1);
> +  MmioWrite32 (HDLCD_REG_H_FRONT_PORCH, Horizontal->FrontPorch);
> 
>    // Enable the controller
>    MmioWrite32 (HDLCD_REG_COMMAND, HDLCD_ENABLE); diff --git
> a/ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.c
> b/ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.c
> index
> b76894c534fb413d87c77c528f0bb7d49862c78f..492edd1ba2e57158c219
> 20d6df11eed5673649c1 100644
> --- a/ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.c
> +++ b/ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.c
> @@ -123,14 +123,8 @@ LcdPlatformQueryMode (  EFI_STATUS
> LcdPlatformGetTimings (
>    IN  UINT32                              ModeNumber,
> -  OUT UINT32*                             HRes,
> -  OUT UINT32*                             HSync,
> -  OUT UINT32*                             HBackPorch,
> -  OUT UINT32*                             HFrontPorch,
> -  OUT UINT32*                             VRes,
> -  OUT UINT32*                             VSync,
> -  OUT UINT32*                             VBackPorch,
> -  OUT UINT32*                             VFrontPorch
> +  OUT SCAN_TIMINGS                        **Horizontal,
> +  OUT SCAN_TIMINGS                        **Vertical
>    )
>  {
>    ASSERT (FALSE);
> diff --git a/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> b/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> index
> 465cb6845437f57d15f05a271d1b01f634e11b56..c9e2736911881fc36b51
> 562b9259b7bccf30747d 100644
> --- a/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> +++ b/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> @@ -84,34 +84,25 @@ LcdSetMode (
>    )
>  {
>    EFI_STATUS        Status;
> -  UINT32            HRes;
> -  UINT32            HSync;
> -  UINT32            HBackPorch;
> -  UINT32            HFrontPorch;
> -  UINT32            VRes;
> -  UINT32            VSync;
> -  UINT32            VBackPorch;
> -  UINT32            VFrontPorch;
> +  SCAN_TIMINGS      *Horizontal;
> +  SCAN_TIMINGS      *Vertical;
>    UINT32            LcdControl;
>    LCD_BPP           LcdBpp;
> 
>    // Set the video mode timings and other relevant information
>    Status = LcdPlatformGetTimings (
>               ModeNumber,
> -             &HRes,
> -             &HSync,
> -             &HBackPorch,
> -             &HFrontPorch,
> -             &VRes,
> -             &VSync,
> -             &VBackPorch,
> -             &VFrontPorch
> +             &Horizontal,
> +             &Vertical
>               );
>    if (EFI_ERROR (Status)) {
>      ASSERT_EFI_ERROR (Status);
>      return Status;
>    }
> 
> +  ASSERT (Horizontal != NULL);
> +  ASSERT (Vertical != NULL);
> +
>    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
>    if (EFI_ERROR (Status)) {
>      ASSERT_EFI_ERROR (Status);
> @@ -124,15 +115,29 @@ LcdSetMode (
>    // Set Timings
>    MmioWrite32 (
>      PL111_REG_LCD_TIMING_0,
> -    HOR_AXIS_PANEL (HBackPorch, HFrontPorch, HSync, HRes)
> +    HOR_AXIS_PANEL (
> +      Horizontal->BackPorch,
> +      Horizontal->FrontPorch,
> +      Horizontal->Sync,
> +      Horizontal->Resolution
> +      )
>      );
> 
>    MmioWrite32 (
>      PL111_REG_LCD_TIMING_1,
> -    VER_AXIS_PANEL (VBackPorch, VFrontPorch, VSync, VRes)
> +    VER_AXIS_PANEL (
> +      Vertical->BackPorch,
> +      Vertical->FrontPorch,
> +      Vertical->Sync,
> +      Vertical->Resolution
> +      )
> +    );
> +
> +  MmioWrite32 (
> +    PL111_REG_LCD_TIMING_2,
> +    CLK_SIG_POLARITY (Horizontal->Resolution)
>      );
> 
> -  MmioWrite32 (PL111_REG_LCD_TIMING_2, CLK_SIG_POLARITY (HRes));
>    MmioWrite32 (PL111_REG_LCD_TIMING_3, 0);
> 
>    // PL111_REG_LCD_CONTROL
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel