[v2,02/14] sm501: Use defines instead of constants where available

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(-)

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.