Re: [U-Boot] [PATCH v3 3/4] dm: video: use constants to refer to colors

[Heinrich Schuchardt]

On 02/07/2018 05:38 AM, Simon Glass wrote:

Hi Heinrich,

On 4 February 2018 at 15:55, Heinrich Schuchardt > wrote:

 > On 02/04/2018 02:40 PM, Simon Glass wrote:
 >> Hi Heinrich,
 >>
 >> On 29 January 2018 at 00:19, Heinrich Schuchardt > wrote:

 >>> Use constants to refer to colors.
 >>> Adjust initialization of foreground and background color to avoid
 >>> setting reserved bits.
 >>> Consistently u32 instead of unsigned for color bit mask.
 >>>
 >>> Signed-off-by: Heinrich Schuchardt >

 >>> ---
 >>> v3
 >>>         Use color constants for initalizing the console.
 >>> v2
 >>>         no change
 >>> ---
 >>>  drivers/video/vidconsole-uclass.c | 55 
+++

 >>>  drivers/video/video-uclass.c      | 19 +-
 >>>  include/video.h                   | 11 ++--
 >>>  include/video_console.h           | 31 ++
 >>>  4 files changed, 85 insertions(+), 31 deletions(-)
 >>
 >> Reviewed-by: Simon Glass >
 >>
 >> Regarding my point about using u32 in function return values and args,
 >> I don't think I explained it very well.
 >>
 >> IMO is makes no sense to structure all the intermediate code which
 >> generates pixel values to use u32, when an unsigned int is enough on
 >> all machines that U-Boot supports. The packing / unpacking into a
 >> 32-bit word in memory is something that is done once when the pixel is
 >> accessed. Thereafter I don't see a need to push things around in a
 >> particular format.
 >
 > The definition I found was
 > typedef unsigned int u32;
 >
 > What makes you believe there is any packing and unpacking or masking
 > overhead avoided by using unsigned int or any other 32bit integer type?

A machine with a 32-bit register has to mask its argument on entry to 
the function to ensure that the caller does not pass an invalid value.


If this were true it would also hold true for unsigned int. See above 
typedef.


C does not check if a value is negative when copying int to unsigned int.



 >
 > For struct vid_rgb other integer types could be used. I not sure if
 > accessing a char is any faster in this context than using 32bit integers.

In general the natural word size is best for arguments and return values 
I think (int or unsigned int).


So I leave it as it is.

By the way: natural word size is size_t and not int.



 >
 > Regards
 >
 > Heinrich
 >
 >>
 >> I have an aversion to code which forces the compiler to mask every
 >> variable access just to pass the data around.
 >>
 >> So I would prefer to use u32 only when accessing the hardware, or for
 >> pointers which do that.
 >>
 >> Reards,
 >> Simon
 >>
 >

Regards,
Simon


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/4] dm: video: use constants to refer to colors

[Simon Glass]

Hi Heinrich,

On 4 February 2018 at 15:55, Heinrich Schuchardt  wrote:
> On 02/04/2018 02:40 PM, Simon Glass wrote:
>> Hi Heinrich,
>>
>> On 29 January 2018 at 00:19, Heinrich Schuchardt 
wrote:
>>> Use constants to refer to colors.
>>> Adjust initialization of foreground and background color to avoid
>>> setting reserved bits.
>>> Consistently u32 instead of unsigned for color bit mask.
>>>
>>> Signed-off-by: Heinrich Schuchardt 
>>> ---
>>> v3
>>> Use color constants for initalizing the console.
>>> v2
>>> no change
>>> ---
>>>  drivers/video/vidconsole-uclass.c | 55
+++
>>>  drivers/video/video-uclass.c  | 19 +-
>>>  include/video.h   | 11 ++--
>>>  include/video_console.h   | 31 ++
>>>  4 files changed, 85 insertions(+), 31 deletions(-)
>>
>> Reviewed-by: Simon Glass 
>>
>> Regarding my point about using u32 in function return values and args,
>> I don't think I explained it very well.
>>
>> IMO is makes no sense to structure all the intermediate code which
>> generates pixel values to use u32, when an unsigned int is enough on
>> all machines that U-Boot supports. The packing / unpacking into a
>> 32-bit word in memory is something that is done once when the pixel is
>> accessed. Thereafter I don't see a need to push things around in a
>> particular format.
>
> The definition I found was
> typedef unsigned int u32;
>
> What makes you believe there is any packing and unpacking or masking
> overhead avoided by using unsigned int or any other 32bit integer type?

A machine with a 32-bit register has to mask its argument on entry to the
function to ensure that the caller does not pass an invalid value.

>
> For struct vid_rgb other integer types could be used. I not sure if
> accessing a char is any faster in this context than using 32bit integers.

In general the natural word size is best for arguments and return values I
think (int or unsigned int).

>
> Regards
>
> Heinrich
>
>>
>> I have an aversion to code which forces the compiler to mask every
>> variable access just to pass the data around.
>>
>> So I would prefer to use u32 only when accessing the hardware, or for
>> pointers which do that.
>>
>> Reards,
>> Simon
>>
>

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/4] dm: video: use constants to refer to colors

[Heinrich Schuchardt]

On 02/04/2018 02:40 PM, Simon Glass wrote:
> Hi Heinrich,
> 
> On 29 January 2018 at 00:19, Heinrich Schuchardt  wrote:
>> Use constants to refer to colors.
>> Adjust initialization of foreground and background color to avoid
>> setting reserved bits.
>> Consistently u32 instead of unsigned for color bit mask.
>>
>> Signed-off-by: Heinrich Schuchardt 
>> ---
>> v3
>> Use color constants for initalizing the console.
>> v2
>> no change
>> ---
>>  drivers/video/vidconsole-uclass.c | 55 
>> +++
>>  drivers/video/video-uclass.c  | 19 +-
>>  include/video.h   | 11 ++--
>>  include/video_console.h   | 31 ++
>>  4 files changed, 85 insertions(+), 31 deletions(-)
> 
> Reviewed-by: Simon Glass 
> 
> Regarding my point about using u32 in function return values and args,
> I don't think I explained it very well.
> 
> IMO is makes no sense to structure all the intermediate code which
> generates pixel values to use u32, when an unsigned int is enough on
> all machines that U-Boot supports. The packing / unpacking into a
> 32-bit word in memory is something that is done once when the pixel is
> accessed. Thereafter I don't see a need to push things around in a
> particular format.

The definition I found was
typedef unsigned int u32;

What makes you believe there is any packing and unpacking or masking
overhead avoided by using unsigned int or any other 32bit integer type?

For struct vid_rgb other integer types could be used. I not sure if
accessing a char is any faster in this context than using 32bit integers.

Regards

Heinrich

> 
> I have an aversion to code which forces the compiler to mask every
> variable access just to pass the data around.
> 
> So I would prefer to use u32 only when accessing the hardware, or for
> pointers which do that.
> 
> Reards,
> Simon
> 

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/4] dm: video: use constants to refer to colors

[Simon Glass]

Hi Heinrich,

On 29 January 2018 at 00:19, Heinrich Schuchardt  wrote:
> Use constants to refer to colors.
> Adjust initialization of foreground and background color to avoid
> setting reserved bits.
> Consistently u32 instead of unsigned for color bit mask.
>
> Signed-off-by: Heinrich Schuchardt 
> ---
> v3
> Use color constants for initalizing the console.
> v2
> no change
> ---
>  drivers/video/vidconsole-uclass.c | 55 
> +++
>  drivers/video/video-uclass.c  | 19 +-
>  include/video.h   | 11 ++--
>  include/video_console.h   | 31 ++
>  4 files changed, 85 insertions(+), 31 deletions(-)

Reviewed-by: Simon Glass 

Regarding my point about using u32 in function return values and args,
I don't think I explained it very well.

IMO is makes no sense to structure all the intermediate code which
generates pixel values to use u32, when an unsigned int is enough on
all machines that U-Boot supports. The packing / unpacking into a
32-bit word in memory is something that is done once when the pixel is
accessed. Thereafter I don't see a need to push things around in a
particular format.

I have an aversion to code which forces the compiler to mask every
variable access just to pass the data around.

So I would prefer to use u32 only when accessing the hardware, or for
pointers which do that.

Reards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot