Re: [Qemu-devel] [PATCH 09/10] sm501: Add some more missing registers

[Peter Maydell]

On 24 February 2017 at 21:49, BALATON Zoltan  wrote:
> On Fri, 24 Feb 2017, BALATON Zoltan wrote:
>>
>> On Fri, 24 Feb 2017, Peter Maydell wrote:
>>>
>>> On 19 February 2017 at 16:35, BALATON Zoltan  wrote:

 Write only to allow clients to initialise these without failing

 Signed-off-by: BALATON Zoltan 
>>>
>>>
>>> What's the point in write-only register values?

>>> If the registers are writes-ignored, there's no need to store
>>> the data written into the state struct; if the registers are
>>> reads-as-written then implement them that way.
>
>
> Still not sure what do you mean by read-as-written because I think that's
> exactly what is done here, value stored and read back as is, except for
> video_control where there are reserved bits that are always 0.

There are several ways we can make a register behave:

"Read only" -> the guest can only read, it cannot write
"Writes ignored" -> the guest can write but it has no effect
"Reads as written" -> the guest can write, and we store the
data so that when the guest reads it gets back the
value it wrote, but it doesn't have any effect on device behaviour
"Reads as zero" -> the guest can read but it always gets zero
"Write only" -> the guest can write values and we store them
but don't let the guest read them back. This is pointless.

For cases where we don't actually implement some bit of hardware
behaviour yet, it can be a reasonable choice to do either
"read-as-zero/writes-ignored", or "reads as written",
depending on what the guest is expecting. (writes-ignored
is the easiest behaviour to implement, but sometimes guests
won't work if you do that.)

If you're implementing "reads as written" then the commit
message shouldn't say "write only"... But the patch seems
to only add code to the register-write function, not to
the register-read function. That means we're storing
values the guest writes but never doing anything with them.
Either throw away the values immediately ("writes ignored")
or let the guest read them back ("reads as written").

(Optionally, we can also log the access via LOG_UNIMP
to let the user know the guest is trying to use some
bit of the device that doesn't work yet.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 09/10] sm501: Add some more missing registers

[BALATON Zoltan]

On Fri, 24 Feb 2017, BALATON Zoltan wrote:

On Fri, 24 Feb 2017, Peter Maydell wrote:

On 19 February 2017 at 16:35, BALATON Zoltan  wrote:

Write only to allow clients to initialise these without failing

Signed-off-by: BALATON Zoltan 


What's the point in write-only register values?


U-boot writes this register during setting up the device and without this it 
would abort QEMU.



What does the real hardware do here?


This register contains bits to set up FIFO parameters and memory priorities 
which we are not emulating so these can be ignored here but the hardware 
would change parameters according the value written.


Sorry, this is for the arbitration_control register. The other registers 
added in this patch are for the 2D engine which is only partially emulated 
but writing the registers is OK as long as no operation using them is 
called. (That case is handled in sm501_2d_operation.) We need to allow 
writes as these are initialised during boot but not used afterwards. Later 
when implementing more of the 2D engine we may use the written values.



If the registers are writes-ignored, there's no need to store
the data written into the state struct; if the registers are
reads-as-written then implement them that way.


Still not sure what do you mean by read-as-written because I think that's 
exactly what is done here, value stored and read back as is, except for 
video_control where there are reserved bits that are always 0.

Re: [Qemu-devel] [PATCH 09/10] sm501: Add some more missing registers

[BALATON Zoltan]

On Fri, 24 Feb 2017, Peter Maydell wrote:

On 19 February 2017 at 16:35, BALATON Zoltan  wrote:

Write only to allow clients to initialise these without failing

Signed-off-by: BALATON Zoltan 


What's the point in write-only register values?


U-boot writes this register during setting up the device and without this 
it would abort QEMU.



What does the real hardware do here?


This register contains bits to set up FIFO parameters and memory 
priorities which we are not emulating so these can be ignored here but 
the hardware would change parameters according the value written.



If the registers are writes-ignored, there's no need to store
the data written into the state struct; if the registers are
reads-as-written then implement them that way.


I'm not sure what you get on real hardware but it's documented to be R/W 
(except reserved bits that are masked which are always 0). Why is it not 
implemented as read-as-written or what do you mean by that?

Re: [Qemu-devel] [PATCH 09/10] sm501: Add some more missing registers

[Peter Maydell]

On 19 February 2017 at 16:35, BALATON Zoltan  wrote:
> Write only to allow clients to initialise these without failing
>
> Signed-off-by: BALATON Zoltan 

What's the point in write-only register values?

What does the real hardware do here?

If the registers are writes-ignored, there's no need to store
the data written into the state struct; if the registers are
reads-as-written then implement them that way.

(If they get state struct fields the vmstate needs to be
updated accordingly, as does reset.)

thanks
-- PMM

[Qemu-devel] [PATCH 09/10] sm501: Add some more missing registers

[BALATON Zoltan]

Write only to allow clients to initialise these without failing

Signed-off-by: BALATON Zoltan 
---
 hw/display/sm501.c | 42 ++
 1 file changed, 42 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 2e1c4b7..16a00cc 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -508,6 +508,8 @@ typedef struct SM501State {
 uint32_t dc_panel_hwc_color_1_2;
 uint32_t dc_panel_hwc_color_3;
 
+uint32_t dc_video_control;
+
 uint32_t dc_crt_control;
 uint32_t dc_crt_fb_addr;
 uint32_t dc_crt_fb_offset;
@@ -527,12 +529,21 @@ typedef struct SM501State {
 uint32_t twoD_control;
 uint32_t twoD_pitch;
 uint32_t twoD_foreground;
+uint32_t twoD_background;
 uint32_t twoD_stretch;
+uint32_t twoD_color_compare;
 uint32_t twoD_color_compare_mask;
 uint32_t twoD_mask;
+uint32_t twoD_clip_tl;
+uint32_t twoD_clip_br;
+uint32_t twoD_mono_pattern_low;
+uint32_t twoD_mono_pattern_high;
 uint32_t twoD_window_width;
 uint32_t twoD_source_base;
 uint32_t twoD_destination_base;
+uint32_t twoD_alpha;
+uint32_t twoD_wrap;
+uint32_t twoD_status;
 
 } SM501State;
 
@@ -1057,6 +1068,10 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr 
addr,
 s->dc_panel_hwc_color_3 = value & 0x;
 break;
 
+case SM501_DC_VIDEO_CONTROL:
+s->dc_video_control = value & 0x0003;
+break;
+
 case SM501_DC_CRT_CONTROL:
 s->dc_crt_control = value & 0x0003;
 break;
@@ -1174,15 +1189,33 @@ static void sm501_2d_engine_write(void *opaque, hwaddr 
addr,
 case SM501_2D_FOREGROUND:
 s->twoD_foreground = value;
 break;
+case SM501_2D_BACKGROUND:
+s->twoD_background = value;
+break;
 case SM501_2D_STRETCH:
 s->twoD_stretch = value;
 break;
+case SM501_2D_COLOR_COMPARE:
+s->twoD_color_compare = value;
+break;
 case SM501_2D_COLOR_COMPARE_MASK:
 s->twoD_color_compare_mask = value;
 break;
 case SM501_2D_MASK:
 s->twoD_mask = value;
 break;
+case SM501_2D_CLIP_TL:
+s->twoD_clip_tl = value;
+break;
+case SM501_2D_CLIP_BR:
+s->twoD_clip_br = value;
+break;
+case SM501_2D_MONO_PATTERN_LOW:
+s->twoD_mono_pattern_low = value;
+break;
+case SM501_2D_MONO_PATTERN_HIGH:
+s->twoD_mono_pattern_high = value;
+break;
 case SM501_2D_WINDOW_WIDTH:
 s->twoD_window_width = value;
 break;
@@ -1192,6 +1225,15 @@ static void sm501_2d_engine_write(void *opaque, hwaddr 
addr,
 case SM501_2D_DESTINATION_BASE:
 s->twoD_destination_base = value;
 break;
+case SM501_2D_ALPHA:
+s->twoD_alpha = value;
+break;
+case SM501_2D_WRAP:
+s->twoD_wrap = value;
+break;
+case SM501_2D_STATUS:
+s->twoD_status = value;
+break;
 default:
 printf("sm501 2d engine : not implemented register write."
" addr=%x, val=%x\n", (int)addr, (unsigned)value);
-- 
2.7.4