Re: [Qemu-devel] [PATCH v2 07/14] sm501: Fix device endianness

[BALATON Zoltan]

On Thu, 2 Mar 2017, Peter Maydell wrote:
> On 25 February 2017 at 18:46, BALATON Zoltan <address@hidden> wrote:
> > Signed-off-by: BALATON Zoltan <address@hidden>
> > ---
> >
> > v2: Split off small clean up to other patch
> >
> >  hw/display/sm501.c          |  6 +++---
> >  hw/display/sm501_template.h | 23 ++++++++++++++---------
> >  2 files changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> > index b977094..2694081 100644
> > --- a/hw/display/sm501.c
> > +++ b/hw/display/sm501.c
> > @@ -849,7 +849,7 @@ static const MemoryRegionOps sm501_system_config_ops = {
> >          .min_access_size = 4,
> >          .max_access_size = 4,
> >      },
> > -    .endianness = DEVICE_NATIVE_ENDIAN,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> >  };
> >
> >  static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
> > @@ -1085,7 +1085,7 @@ static const MemoryRegionOps sm501_disp_ctrl_ops = {
> >          .min_access_size = 4,
> >          .max_access_size = 4,
> >      },
> > -    .endianness = DEVICE_NATIVE_ENDIAN,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> >  };
> >
> >  static uint64_t sm501_2d_engine_read(void *opaque, hwaddr addr,
> > @@ -1173,7 +1173,7 @@ static const MemoryRegionOps sm501_2d_engine_ops = {
> >          .min_access_size = 4,
> >          .max_access_size = 4,
> >      },
> > -    .endianness = DEVICE_NATIVE_ENDIAN,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> >  };
> >
> >  /* draw line functions for all console modes */
> > diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
> > index 832ee61..6e56baf 100644
> > --- a/hw/display/sm501_template.h
> > +++ b/hw/display/sm501_template.h
> > @@ -64,10 +64,16 @@ static void glue(draw_line16_, PIXEL_NAME)(
> >      uint8_t r, g, b;
> >
> >      do {
> > -        rgb565 = lduw_p(s);
> > -        r = ((rgb565 >> 11) & 0x1f) << 3;
> > -        g = ((rgb565 >>  5) & 0x3f) << 2;
> > -        b = ((rgb565 >>  0) & 0x1f) << 3;
> > +        rgb565 = lduw_le_p(s);
> > +#if defined(TARGET_WORDS_BIGENDIAN)
> > +        r = (rgb565 >> 8) & 0xf8;
> > +        g = (rgb565 >> 3) & 0xfc;
> > +        b = (rgb565 << 3) & 0xf8;
> > +#else
> > +        b = (rgb565 >> 8) & 0xf8;
> > +        g = (rgb565 >> 3) & 0xfc;
> > +        r = (rgb565 << 3) & 0xf8;
> > +#endif
> >          *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
> >          s += 2;
> >          d += BPP;
> > @@ -80,15 +86,14 @@ static void glue(draw_line32_, PIXEL_NAME)(
> >      uint8_t r, g, b;
> >
> >      do {
> > -        ldub_p(s);
> >  #if defined(TARGET_WORDS_BIGENDIAN)
> > +        r = s[0];
> > +        g = s[1];
> > +        b = s[2];
> > +#else
> >          r = s[1];
> >          g = s[2];
> >          b = s[3];
> > -#else
> > -        b = s[0];
> > -        g = s[1];
> > -        r = s[2];
> >  #endif
> >          *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
> >          s += 4;
>
> I still suspect this is not correct. In particular, if you
> look at the Linux driver:
>
> http://lxr.free-electrons.com/source/drivers/video/fbdev/sm501fb.c#L341
>
> we have for 32 bit two layouts within a 32-bit word:
> BGRX (if "swap fb endian" is set)
> XRGB (the usual)
> (plus the cross-product with "32-bit load is BE or LE",
> since we've coded this as byte accesses rather than a ldl_*_p()).
>
> and for 16 bit we have either RGB or BGR ordering.
> I'm not completely sure this lines up with the code you have.

Maybe it's not correct but works for everything I could test better than 
the original code (which was broken even for the SH images one can find) 
so I think we could just go with this until someone complains and provides 
a test case. I've given up on trying to understand it because I don't 
really know this device and SH so I could only go by the images I could 
find and they seem to like it this way.

> Looking at how the SWAP_FB_ENDIAN flag gets set:
> * for the r2d board it is set always
> * for PCI devices, it is set only if big-endian
>
> I suspect that what we actually have here is something
> like:
> * for the PCI device it's always little endian (and the
>   code ought not to need to depend on TARGET_WORDS_BIGENDIAN)
> * sysbus device may be more complicated
>
> Also I note there's an SM501_ENDIAN_CONTROL register on the
> device, which presumably if written changes the behaviour.

I've seen this but it's not emulated so it can be ignored for now. The 
spec also says that default is little endian so for regs at least 
DEVICE_LITTLE_ENDIAN should be OK.

> Plus for the sysbus device if we change the settings to
> DEVICE_LITTLE_ENDIAN for the various control register
> regions that suggests we should be consistent about the
> serial_mm_init() endian order and also the USB controller.
>
> It's late evening here though so I can't investigate any
> further right now.
>
> thanks
> -- PMM