| Message ID | 65f979bc1683c46f74ddb5e931810f4e2b8ea27c.1488068248.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7 November 2016 at 09:03, BALATON Zoltan <balaton@eik.bme.hu> wrote: > This also fixes the initial value of misc_control register to match the > comment which is likely what was intended but the DAC_POWER bit was set > instead. This value is unused so it does not really matter but is > fixed here for consistency. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/display/sm501.c | 8 ++++---- > hw/display/sm501_template.h | 2 +- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/hw/display/sm501.c b/hw/display/sm501.c > index 4f40dee..4eb085c 100644 > --- a/hw/display/sm501.c > +++ b/hw/display/sm501.c > @@ -555,7 +555,7 @@ static uint32_t get_local_mem_size_index(uint32_t size) > static inline int is_hwc_enabled(SM501State *state, int crt) > { > uint32_t addr = crt ? state->dc_crt_hwc_addr : state->dc_panel_hwc_addr; > - return addr & 0x80000000; > + return addr & SM501_HWC_EN; > } > > /** > @@ -1411,9 +1411,9 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base, > s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes); > SM501_DPRINTF("local mem size=%x. index=%d\n", get_local_mem_size(s), > s->local_mem_size_index); > - s->system_control = 0x00100000; > - s->misc_control = 0x00001000; /* assumes SH, active=low */ > - s->dc_panel_control = 0x00010000; > + s->system_control = 0x00100000; /* 2D engine FIFO empty */ > + s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */ > + s->dc_panel_control = 0x00010000; /* FIFO level 3 */ > s->dc_crt_control = 0x00010000; I just found a datasheet which says that the power-on-default for the misc-control register is 0b0000.0000.0000.00x0.0001.0000.xxx0.0xxx which means that the 0x1000 decimal value is correct and setting the IRQ_INVERT bit is wrong. I think the "active=low" bit is referencing the fact that the DAC_POWER bit is 1 for "disable" and 0 for "enable". It makes sense hardware-wise for the DAC to be powered off by default at reset as well I think. thanks -- PMM
On 2 March 2017 at 18:53, Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 November 2016 at 09:03, BALATON Zoltan <balaton@eik.bme.hu> wrote: > I just found a datasheet which says that the power-on-default > for the misc-control register is 0b0000.0000.0000.00x0.0001.0000.xxx0.0xxx > which means that the 0x1000 decimal value is correct and setting > the IRQ_INVERT bit is wrong. > > I think the "active=low" bit is referencing the fact that the > DAC_POWER bit is 1 for "disable" and 0 for "enable". Actually it's referring to bit 17 (which is an "X" in the reset value above), which is documented as being set by a GPIO pin on reset, and having different meanings for SH and SA1110. In this case we're saying 'reset bit 17 to 0, meaning "SH series CPU, Active Low"'. Note also that the low bits indicate the bus type which is 000 for SH and 001 for PCI, etc. So a better comment here would be: /* Bits 17 (SH), 7 (CDR), 6:5 (Test), 2:0 (Bus) are all supposed * to be determined at reset by GPIO lines which set config bits. * We hardwire them: * SH = 0 : Hitachi Ready Polarity == Active Low * CDR = 0 : do not reset clock divider * TEST = 0 : Normal mode (not testing the silicon) * BUS = 0 : Hitachi SH3/SH4 */ s->misc_control = SM501_MISC_DAC_POWER; ...and you'll want to put something in to get the right Bus value for the PCI version. thanks -- PMM
On Thu, 2 Mar 2017, Peter Maydell wrote: > On 2 March 2017 at 18:53, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 7 November 2016 at 09:03, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> I just found a datasheet which says that the power-on-default >> for the misc-control register is 0b0000.0000.0000.00x0.0001.0000.xxx0.0xxx >> which means that the 0x1000 decimal value is correct and setting >> the IRQ_INVERT bit is wrong. >> >> I think the "active=low" bit is referencing the fact that the >> DAC_POWER bit is 1 for "disable" and 0 for "enable". > > Actually it's referring to bit 17 (which is an "X" in the reset value > above), which is documented as being set by a GPIO pin on reset, > and having different meanings for SH and SA1110. In this case we're > saying 'reset bit 17 to 0, meaning "SH series CPU, Active Low"'. > > Note also that the low bits indicate the bus type which is 000 for > SH and 001 for PCI, etc. > > So a better comment here would be: > /* Bits 17 (SH), 7 (CDR), 6:5 (Test), 2:0 (Bus) are all supposed > * to be determined at reset by GPIO lines which set config bits. > * We hardwire them: > * SH = 0 : Hitachi Ready Polarity == Active Low > * CDR = 0 : do not reset clock divider > * TEST = 0 : Normal mode (not testing the silicon) > * BUS = 0 : Hitachi SH3/SH4 > */ > s->misc_control = SM501_MISC_DAC_POWER; > > ...and you'll want to put something in to get the right Bus value > for the PCI version. Thanks for investigating this, I'll update it in v3.
diff --git a/hw/display/sm501.c b/hw/display/sm501.c index 4f40dee..4eb085c 100644 --- a/hw/display/sm501.c +++ b/hw/display/sm501.c @@ -555,7 +555,7 @@ static uint32_t get_local_mem_size_index(uint32_t size) static inline int is_hwc_enabled(SM501State *state, int crt) { uint32_t addr = crt ? state->dc_crt_hwc_addr : state->dc_panel_hwc_addr; - return addr & 0x80000000; + return addr & SM501_HWC_EN; } /** @@ -1411,9 +1411,9 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base, s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes); SM501_DPRINTF("local mem size=%x. index=%d\n", get_local_mem_size(s), s->local_mem_size_index); - s->system_control = 0x00100000; - s->misc_control = 0x00001000; /* assumes SH, active=low */ - s->dc_panel_control = 0x00010000; + s->system_control = 0x00100000; /* 2D engine FIFO empty */ + s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */ + s->dc_panel_control = 0x00010000; /* FIFO level 3 */ s->dc_crt_control = 0x00010000; /* allocate local memory */ diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h index aeeac5d..16e500b 100644 --- a/hw/display/sm501_template.h +++ b/hw/display/sm501_template.h @@ -108,7 +108,7 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt, /* get hardware cursor pattern */ uint32_t cursor_addr = get_hwc_address(s, crt); assert(0 <= c_y && c_y < SM501_HWC_HEIGHT); - cursor_addr += 64 * c_y / 4; /* 4 pixels per byte */ + cursor_addr += SM501_HWC_WIDTH * c_y / 4; /* 4 pixels per byte */ cursor_addr += s->base; /* get cursor position */
This also fixes the initial value of misc_control register to match the comment which is likely what was intended but the DAC_POWER bit was set instead. This value is unused so it does not really matter but is fixed here for consistency. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/display/sm501.c | 8 ++++---- hw/display/sm501_template.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)