| Message ID | CADnq5_PZCPtCk48L1BGqrAY-0vGVoG-jkZN-MgTw5Z6LhCd1Ag@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 22, 2013 at 10:18:09AM -0400, Alex Deucher wrote: > On Mon, Apr 22, 2013 at 10:08 AM, Dan Carpenter > <dan.carpenter@oracle.com> wrote: > > On Mon, Apr 22, 2013 at 10:03:13AM -0400, alexdeucher@gmail.com wrote: > >> From: Alex Deucher <alexander.deucher@amd.com> > >> > >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > >> --- > >> drivers/gpu/drm/radeon/atombios.h | 2 ++ > >> drivers/gpu/drm/radeon/radeon_atombios.c | 6 ++---- > >> 2 files changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/radeon/atombios.h b/drivers/gpu/drm/radeon/atombios.h > >> index 4b04ba3..de678dd 100644 > >> --- a/drivers/gpu/drm/radeon/atombios.h > >> +++ b/drivers/gpu/drm/radeon/atombios.h > >> @@ -459,6 +459,7 @@ typedef struct _COMPUTE_MEMORY_ENGINE_PLL_PARAMETERS_V3 > >> { > >> ATOM_COMPUTE_CLOCK_FREQ ulClock; //Input Parameter > >> ATOM_S_MPLL_FB_DIVIDER ulFbDiv; //Output Parameter > >> + ULONG ulClockFbDiv; > > > > Why is this a long instead of an __le32 or u32? > > atombios.h is shared across OSes and has it's own types. Can ULONG be 64 bit? It's ugly when ULONG, ulong and "unsigned long" are different types. > > > > I'm confused by this patch as well. I assumed the datatypes were > > determined by the hardware spec. > > I'm not sure I follow. The atombios interpretor requires data in > little endian format. I was expecting that the code would stay the same and the annotations would change is all... I haven't tested this so I'm sure it's right, but it just wasn't the change I was expecting. regards, dan carpenter
On Mon, Apr 22, 2013 at 10:31 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Mon, Apr 22, 2013 at 10:18:09AM -0400, Alex Deucher wrote: >> On Mon, Apr 22, 2013 at 10:08 AM, Dan Carpenter >> <dan.carpenter@oracle.com> wrote: >> > On Mon, Apr 22, 2013 at 10:03:13AM -0400, alexdeucher@gmail.com wrote: >> >> From: Alex Deucher <alexander.deucher@amd.com> >> >> >> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> >> >> --- >> >> drivers/gpu/drm/radeon/atombios.h | 2 ++ >> >> drivers/gpu/drm/radeon/radeon_atombios.c | 6 ++---- >> >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/radeon/atombios.h b/drivers/gpu/drm/radeon/atombios.h >> >> index 4b04ba3..de678dd 100644 >> >> --- a/drivers/gpu/drm/radeon/atombios.h >> >> +++ b/drivers/gpu/drm/radeon/atombios.h >> >> @@ -459,6 +459,7 @@ typedef struct _COMPUTE_MEMORY_ENGINE_PLL_PARAMETERS_V3 >> >> { >> >> ATOM_COMPUTE_CLOCK_FREQ ulClock; //Input Parameter >> >> ATOM_S_MPLL_FB_DIVIDER ulFbDiv; //Output Parameter >> >> + ULONG ulClockFbDiv; >> > >> > Why is this a long instead of an __le32 or u32? >> >> atombios.h is shared across OSes and has it's own types. > > Can ULONG be 64 bit? It's ugly when ULONG, ulong and > "unsigned long" are different types. The atombios ULONG type is always 32 bits. > >> > >> > I'm confused by this patch as well. I assumed the datatypes were >> > determined by the hardware spec. >> >> I'm not sure I follow. The atombios interpretor requires data in >> little endian format. > > I was expecting that the code would stay the same and the > annotations would change is all... I haven't tested this so I'm > sure it's right, but it just wasn't the change I was expecting. This seemed like the most obvious solution to me, but it may not be optimal. What would you have done? Alex
On Mon, 2013-04-22 at 10:18 -0400, Alex Deucher wrote: > On Mon, Apr 22, 2013 at 10:08 AM, Dan Carpenter > <dan.carpenter@oracle.com> wrote: > > On Mon, Apr 22, 2013 at 10:03:13AM -0400, alexdeucher@gmail.com wrote: > >> From: Alex Deucher <alexander.deucher@amd.com> > >> > >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > >> --- > >> drivers/gpu/drm/radeon/atombios.h | 2 ++ > >> drivers/gpu/drm/radeon/radeon_atombios.c | 6 ++---- > >> 2 files changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/radeon/atombios.h b/drivers/gpu/drm/radeon/atombios.h > >> index 4b04ba3..de678dd 100644 > >> --- a/drivers/gpu/drm/radeon/atombios.h > >> +++ b/drivers/gpu/drm/radeon/atombios.h [...] > >> @@ -491,6 +492,7 @@ typedef struct _COMPUTE_MEMORY_ENGINE_PLL_PARAMETERS_V5 > >> { > >> ATOM_COMPUTE_CLOCK_FREQ ulClock; //Input Parameter > >> ATOM_S_MPLL_FB_DIVIDER ulFbDiv; //Output Parameter > >> + ULONG ulClockFbDiv; > >> }; > >> UCHAR ucRefDiv; //Output Parameter > >> UCHAR ucPostDiv; //Output Parameter This may just be a nitpick, but the location and name of the new union member is slightly confusing: It suggests that it's some kind of combination of the ulClock and ulFbDiv members, when it's just an alternative representation of ulClock. I'd suggest moving up the new member, clarifying its name and/or adding a comment explaining what it is for. Looks good to me other than that.
From ce94ed1083df11895dbec520ef243d4fd805a4a9 Mon Sep 17 00:00:00 2001 From: Alex Deucher <alexander.deucher@amd.com> Date: Mon, 22 Apr 2013 09:59:01 -0400 Subject: [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers() (v2) v2: fix copy paste typo. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> --- drivers/gpu/drm/radeon/atombios.h | 2 ++ drivers/gpu/drm/radeon/radeon_atombios.c | 6 ++---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/radeon/atombios.h b/drivers/gpu/drm/radeon/atombios.h index 4b04ba3..de678dd 100644 --- a/drivers/gpu/drm/radeon/atombios.h +++ b/drivers/gpu/drm/radeon/atombios.h @@ -459,6 +459,7 @@ typedef struct _COMPUTE_MEMORY_ENGINE_PLL_PARAMETERS_V3 { ATOM_COMPUTE_CLOCK_FREQ ulClock; //Input Parameter ATOM_S_MPLL_FB_DIVIDER ulFbDiv; //Output Parameter + ULONG ulClockFbDiv; }; UCHAR ucRefDiv; //Output Parameter UCHAR ucPostDiv; //Output Parameter @@ -491,6 +492,7 @@ typedef struct _COMPUTE_MEMORY_ENGINE_PLL_PARAMETERS_V5 { ATOM_COMPUTE_CLOCK_FREQ ulClock; //Input Parameter ATOM_S_MPLL_FB_DIVIDER ulFbDiv; //Output Parameter + ULONG ulClockFbDiv; }; UCHAR ucRefDiv; //Output Parameter UCHAR ucPostDiv; //Output Parameter diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c b/drivers/gpu/drm/radeon/radeon_atombios.c index 8c1779c..4b853d8 100644 --- a/drivers/gpu/drm/radeon/radeon_atombios.c +++ b/drivers/gpu/drm/radeon/radeon_atombios.c @@ -2710,8 +2710,7 @@ int radeon_atom_get_clock_dividers(struct radeon_device *rdev, dividers->enable_post_div = (dividers->fb_div & 1) ? true : false; } else { if (clock_type == COMPUTE_ENGINE_PLL_PARAM) { - args.v3.ulClock.ulComputeClockFlag = clock_type; - args.v3.ulClock.ulClockFreq = cpu_to_le32(clock); /* 10 khz */ + args.v3.ulClockFbDiv = cpu_to_le32((clock_type << 24) | clock); atom_execute_table(rdev->mode_info.atom_context, index, (uint32_t *)&args); @@ -2726,8 +2725,7 @@ int radeon_atom_get_clock_dividers(struct radeon_device *rdev, dividers->vco_mode = (args.v3.ucCntlFlag & ATOM_PLL_CNTL_FLAG_MPLL_VCO_MODE) ? 1 : 0; } else { - args.v5.ulClock.ulComputeClockFlag = clock_type; - args.v5.ulClock.ulClockFreq = cpu_to_le32(clock); /* 10 khz */ + args.v5.ulClockFbDiv = cpu_to_le32((clock_type << 24) | clock); if (strobe_mode) args.v5.ucInputFlag = ATOM_PLL_INPUT_FLAG_PLL_STROBE_MODE_EN; -- 1.7.7.5