All of lore.kernel.org
 help / color / mirror / Atom feed
* re: drm/radeon: add radeon_atom_get_clock_dividers helper
@ 2013-04-18 18:47 Dan Carpenter
  2013-04-19 15:45 ` Christian König
  2013-04-22 14:03 ` [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers() alexdeucher
  0 siblings, 2 replies; 10+ messages in thread
From: Dan Carpenter @ 2013-04-18 18:47 UTC (permalink / raw)
  To: deathsimple; +Cc: dri-devel

Hello Christian König,

The patch 7062ab67d4c6: "drm/radeon: add
radeon_atom_get_clock_dividers helper" from Apr 8, 2013, has endian
bugs.

drivers/gpu/drm/radeon/radeon_atombios.c
  2712                          if (clock_type == COMPUTE_ENGINE_PLL_PARAM) {
  2713                                  args.v3.ulClock.ulComputeClockFlag = clock_type;
  2714                                  args.v3.ulClock.ulClockFreq = cpu_to_le32(clock);       /* 10 khz */
                                                        ^^^^^^^^^^^
This is 24 bit bitfield so it can't store a __le32.  On little endian
systems it will truncate high bits away so that's ok, but on big endian
it will break.

  2715  
  2716                                  atom_execute_table(rdev->mode_info.atom_context, index, (uint32_t *)&args);
  2717  
  2718                                  dividers->post_div = args.v3.ucPostDiv;
  2719                                  dividers->enable_post_div = (args.v3.ucCntlFlag &
  2720                                                               ATOM_PLL_CNTL_FLAG_PLL_POST_DIV_EN) ? true : false;

There are a lot of other Sparse endian warnings but I haven't looked
at them.  Here is how to check:  http://lwn.net/Articles/205624/

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: drm/radeon: add radeon_atom_get_clock_dividers helper
  2013-04-18 18:47 drm/radeon: add radeon_atom_get_clock_dividers helper Dan Carpenter
@ 2013-04-19 15:45 ` Christian König
  2013-04-22 14:03 ` [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers() alexdeucher
  1 sibling, 0 replies; 10+ messages in thread
From: Christian König @ 2013-04-19 15:45 UTC (permalink / raw)
  To: Dan Carpenter, Alex Deucher; +Cc: dri-devel

Am 18.04.2013 20:47, schrieb Dan Carpenter:
> Hello Christian König,
>
> The patch 7062ab67d4c6: "drm/radeon: add
> radeon_atom_get_clock_dividers helper" from Apr 8, 2013, has endian
> bugs.
>
> drivers/gpu/drm/radeon/radeon_atombios.c
>    2712                          if (clock_type == COMPUTE_ENGINE_PLL_PARAM) {
>    2713                                  args.v3.ulClock.ulComputeClockFlag = clock_type;
>    2714                                  args.v3.ulClock.ulClockFreq = cpu_to_le32(clock);       /* 10 khz */
>                                                          ^^^^^^^^^^^
> This is 24 bit bitfield so it can't store a __le32.  On little endian
> systems it will truncate high bits away so that's ok, but on big endian
> it will break.
>
>    2715
>    2716                                  atom_execute_table(rdev->mode_info.atom_context, index, (uint32_t *)&args);
>    2717
>    2718                                  dividers->post_div = args.v3.ucPostDiv;
>    2719                                  dividers->enable_post_div = (args.v3.ucCntlFlag &
>    2720                                                               ATOM_PLL_CNTL_FLAG_PLL_POST_DIV_EN) ? true : false;
>
> There are a lot of other Sparse endian warnings but I haven't looked
> at them.  Here is how to check:  http://lwn.net/Articles/205624/

Hi Dan,

in general I'm not sure that we support any big endian system with that 
code any more.

But Alex wrote most of the atombios code, so that is probably more a 
question for him.

Christian.

> regards,
> dan carpenter
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers()
  2013-04-18 18:47 drm/radeon: add radeon_atom_get_clock_dividers helper Dan Carpenter
  2013-04-19 15:45 ` Christian König
@ 2013-04-22 14:03 ` alexdeucher
  2013-04-22 14:08   ` Dan Carpenter
  1 sibling, 1 reply; 10+ messages in thread
From: alexdeucher @ 2013-04-22 14:03 UTC (permalink / raw)
  To: dri-devel, dan.carpenter; +Cc: Alex Deucher

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;
   };
   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..2fc444e 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.v3.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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers()
  2013-04-22 14:03 ` [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers() alexdeucher
@ 2013-04-22 14:08   ` Dan Carpenter
  2013-04-22 14:18     ` Alex Deucher
  0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2013-04-22 14:08 UTC (permalink / raw)
  To: alexdeucher; +Cc: Alex Deucher, dri-devel

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?


>    };
>    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..2fc444e 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.v3.ulClockFbDiv = cpu_to_le32((clock_type << 24) | clock);

We've changed from v5 to v3.  Was that intentional?

I'm confused by this patch as well.  I assumed the datatypes were
determined by the hardware spec.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers()
  2013-04-22 14:08   ` Dan Carpenter
@ 2013-04-22 14:18     ` Alex Deucher
  2013-04-22 14:31       ` Dan Carpenter
  2013-04-22 15:29       ` Michel Dänzer
  0 siblings, 2 replies; 10+ messages in thread
From: Alex Deucher @ 2013-04-22 14:18 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Alex Deucher, dri-devel

[-- Attachment #1: Type: text/plain, Size: 3417 bytes --]

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.

>
>
>>    };
>>    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..2fc444e 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.v3.ulClockFbDiv = cpu_to_le32((clock_type << 24) | clock);
>
> We've changed from v5 to v3.  Was that intentional?

Copy paste typo.  fixed in v2 which is attached.

>
> 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.

Alex

[-- Attachment #2: 0001-drm-radeon-fix-endian-bugs-in-radeon_atom_get_clock_.patch --]
[-- Type: text/x-patch, Size: 2724 bytes --]

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


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers()
  2013-04-22 14:18     ` Alex Deucher
@ 2013-04-22 14:31       ` Dan Carpenter
  2013-04-22 14:34         ` Alex Deucher
  2013-04-22 15:29       ` Michel Dänzer
  1 sibling, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2013-04-22 14:31 UTC (permalink / raw)
  To: Alex Deucher; +Cc: Alex Deucher, dri-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers()
  2013-04-22 14:31       ` Dan Carpenter
@ 2013-04-22 14:34         ` Alex Deucher
  0 siblings, 0 replies; 10+ messages in thread
From: Alex Deucher @ 2013-04-22 14:34 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Alex Deucher, dri-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers()
  2013-04-22 14:18     ` Alex Deucher
  2013-04-22 14:31       ` Dan Carpenter
@ 2013-04-22 15:29       ` Michel Dänzer
  2013-04-22 16:15         ` Alex Deucher
  1 sibling, 1 reply; 10+ messages in thread
From: Michel Dänzer @ 2013-04-22 15:29 UTC (permalink / raw)
  To: Alex Deucher; +Cc: dri-devel, Dan Carpenter

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.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers()
  2013-04-22 15:29       ` Michel Dänzer
@ 2013-04-22 16:15         ` Alex Deucher
  2013-04-22 16:25           ` Michel Dänzer
  0 siblings, 1 reply; 10+ messages in thread
From: Alex Deucher @ 2013-04-22 16:15 UTC (permalink / raw)
  To: Michel Dänzer; +Cc: dri-devel, Dan Carpenter

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

On Mon, Apr 22, 2013 at 11:29 AM, Michel Dänzer <michel@daenzer.net> wrote:
> 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.

Sounds good.  v3 attached.

Alex

[-- Attachment #2: 0001-drm-radeon-fix-endian-bugs-in-radeon_atom_get_clock_.patch --]
[-- Type: text/x-patch, Size: 2717 bytes --]

From d21f9d7ef1645fcb1573c80a8b0160cd3208a22a 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() (v3)

v2: fix copy paste typo.
v3: clarify new union member

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..0ee5737 100644
--- a/drivers/gpu/drm/radeon/atombios.h
+++ b/drivers/gpu/drm/radeon/atombios.h
@@ -458,6 +458,7 @@ typedef struct _COMPUTE_MEMORY_ENGINE_PLL_PARAMETERS_V3
   union
   {
     ATOM_COMPUTE_CLOCK_FREQ  ulClock;         //Input Parameter
+    ULONG ulClockParams;                      //ULONG access for BE
     ATOM_S_MPLL_FB_DIVIDER   ulFbDiv;         //Output Parameter
   };
   UCHAR   ucRefDiv;                           //Output Parameter      
@@ -490,6 +491,7 @@ typedef struct _COMPUTE_MEMORY_ENGINE_PLL_PARAMETERS_V5
   union
   {
     ATOM_COMPUTE_CLOCK_FREQ  ulClock;         //Input Parameter
+    ULONG ulClockParams;                      //ULONG access for BE
     ATOM_S_MPLL_FB_DIVIDER   ulFbDiv;         //Output Parameter
   };
   UCHAR   ucRefDiv;                           //Output Parameter      
diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c b/drivers/gpu/drm/radeon/radeon_atombios.c
index 8c1779c..0dd87c0 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.ulClockParams = 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.ulClockParams = 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


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers()
  2013-04-22 16:15         ` Alex Deucher
@ 2013-04-22 16:25           ` Michel Dänzer
  0 siblings, 0 replies; 10+ messages in thread
From: Michel Dänzer @ 2013-04-22 16:25 UTC (permalink / raw)
  To: Alex Deucher; +Cc: dri-devel, Dan Carpenter

On Mon, 2013-04-22 at 12:15 -0400, Alex Deucher wrote: 
> On Mon, Apr 22, 2013 at 11:29 AM, Michel Dänzer <michel@daenzer.net> wrote:
> > 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.
> 
> Sounds good.  v3 attached.

Thanks.

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com> 

-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-04-22 16:25 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-18 18:47 drm/radeon: add radeon_atom_get_clock_dividers helper Dan Carpenter
2013-04-19 15:45 ` Christian König
2013-04-22 14:03 ` [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers() alexdeucher
2013-04-22 14:08   ` Dan Carpenter
2013-04-22 14:18     ` Alex Deucher
2013-04-22 14:31       ` Dan Carpenter
2013-04-22 14:34         ` Alex Deucher
2013-04-22 15:29       ` Michel Dänzer
2013-04-22 16:15         ` Alex Deucher
2013-04-22 16:25           ` Michel Dänzer

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.