[PATCH 2/6] drm/radeon: ATOM Endian fix for atombios_crtc_program_pll()

[PATCH 2/6] drm/radeon: ATOM Endian fix for atombios_crtc_program_pll()

[Benjamin Herrenschmidt]

v6 of the structure was programmed incorrectly:

  args.v6.ulCrtcPclkFreq.ulPixelClock = cpu_to_le32(clock / 10);

ulPixelClock is a 24-bit bitfield. This statement would thus
do a 32-bit swap of (clock / 10) and drop the top 8 bits which
are ... the LSB. Not what we want. Instead use masks & shifts.

Signed-off-by: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
---

(resent adding dri-devel to the CC list to hit patchwork)

 drivers/gpu/drm/radeon/atombios_crtc.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
index 9541995..c742944 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -764,7 +764,7 @@ static void atombios_crtc_set_dcpll(struct drm_crtc *crtc,
 }
 
 static void atombios_crtc_program_pll(struct drm_crtc *crtc,
-				      int crtc_id,
+				      u32 crtc_id,
 				      int pll_id,
 				      u32 encoder_mode,
 				      u32 encoder_id,
@@ -851,8 +851,7 @@ static void atombios_crtc_program_pll(struct drm_crtc *crtc,
 			args.v5.ucPpll = pll_id;
 			break;
 		case 6:
-			args.v6.ulCrtcPclkFreq.ucCRTC = crtc_id;
-			args.v6.ulCrtcPclkFreq.ulPixelClock = cpu_to_le32(clock / 10);
+			args.v6.ulDispEngClkFreq = cpu_to_le32(crtc_id << 24 | clock / 10);
 			args.v6.ucRefDiv = ref_div;
 			args.v6.usFbDiv = cpu_to_le16(fb_div);
 			args.v6.ulFbDivDecFrac = cpu_to_le32(frac_fb_div * 100000);

Re: [PATCH 2/6] drm/radeon: ATOM Endian fix for atombios_crtc_program_pll()

[Alex Deucher]

On Wed, Jul 13, 2011 at 2:28 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> v6 of the structure was programmed incorrectly:
>
>  args.v6.ulCrtcPclkFreq.ulPixelClock = cpu_to_le32(clock / 10);
>
> ulPixelClock is a 24-bit bitfield. This statement would thus
> do a 32-bit swap of (clock / 10) and drop the top 8 bits which
> are ... the LSB. Not what we want. Instead use masks & shifts.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>
> (resent adding dri-devel to the CC list to hit patchwork)
>
>  drivers/gpu/drm/radeon/atombios_crtc.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
> index 9541995..c742944 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -764,7 +764,7 @@ static void atombios_crtc_set_dcpll(struct drm_crtc *crtc,
>  }
>
>  static void atombios_crtc_program_pll(struct drm_crtc *crtc,
> -                                     int crtc_id,
> +                                     u32 crtc_id,
>                                      int pll_id,
>                                      u32 encoder_mode,
>                                      u32 encoder_id,
> @@ -851,8 +851,7 @@ static void atombios_crtc_program_pll(struct drm_crtc *crtc,
>                        args.v5.ucPpll = pll_id;
>                        break;
>                case 6:
> -                       args.v6.ulCrtcPclkFreq.ucCRTC = crtc_id;
> -                       args.v6.ulCrtcPclkFreq.ulPixelClock = cpu_to_le32(clock / 10);
> +                       args.v6.ulDispEngClkFreq = cpu_to_le32(crtc_id << 24 | clock / 10);

For clarity (i can never remember op precedence), you might put:

cpu_to_le32((crtc_id << 24) | (clock / 10));

Other than that,

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>


>                        args.v6.ucRefDiv = ref_div;
>                        args.v6.usFbDiv = cpu_to_le16(fb_div);
>                        args.v6.ulFbDivDecFrac = cpu_to_le32(frac_fb_div * 100000);
>
>
>
>

Re: [PATCH 2/6] drm/radeon: ATOM Endian fix for atombios_crtc_program_pll()

[Benjamin Herrenschmidt]

On Wed, 2011-07-13 at 10:38 -0400, Alex Deucher wrote:

> >                case 6:
> > -                       args.v6.ulCrtcPclkFreq.ucCRTC = crtc_id;
> > -                       args.v6.ulCrtcPclkFreq.ulPixelClock = cpu_to_le32(clock / 10);
> > +                       args.v6.ulDispEngClkFreq = cpu_to_le32(crtc_id << 24 | clock / 10);
> 
> For clarity (i can never remember op precedence), you might put:
> 
> cpu_to_le32((crtc_id << 24) | (clock / 10));

I can't either but I have a nice chart printed on my wall :-)

I can respin if you really want but the above is correct.

Cheers,
Ben.

> Other than that,
> 
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> 
> 
> >                        args.v6.ucRefDiv = ref_div;
> >                        args.v6.usFbDiv = cpu_to_le16(fb_div);
> >                        args.v6.ulFbDivDecFrac = cpu_to_le32(frac_fb_div * 100000);
> >
> >
> >
> >

Re: [PATCH 2/6] drm/radeon: ATOM Endian fix for atombios_crtc_program_pll()

[Alex Deucher]

On Wed, Jul 13, 2011 at 5:40 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2011-07-13 at 10:38 -0400, Alex Deucher wrote:
>
>> >                case 6:
>> > -                       args.v6.ulCrtcPclkFreq.ucCRTC = crtc_id;
>> > -                       args.v6.ulCrtcPclkFreq.ulPixelClock = cpu_to_le32(clock / 10);
>> > +                       args.v6.ulDispEngClkFreq = cpu_to_le32(crtc_id << 24 | clock / 10);
>>
>> For clarity (i can never remember op precedence), you might put:
>>
>> cpu_to_le32((crtc_id << 24) | (clock / 10));
>
> I can't either but I have a nice chart printed on my wall :-)
>
> I can respin if you really want but the above is correct.

That's fine as is then.

Alex

>
> Cheers,
> Ben.
>
>> Other than that,
>>
>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>>
>>
>> >                        args.v6.ucRefDiv = ref_div;
>> >                        args.v6.usFbDiv = cpu_to_le16(fb_div);
>> >                        args.v6.ulFbDivDecFrac = cpu_to_le32(frac_fb_div * 100000);
>> >
>> >
>> >
>> >
>
>
>