Re: [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code

[Andreas Färber]

Am 06.01.2012 19:58, schrieb Peter Maydell:
> Use the new memory mutator API to simplify the flash remap code;
> this allows us to drop the flash_mapped flag.
> 
> Signed-off-by: Peter Maydell <address@hidden>

Reviewed-by: Andreas Färber <address@hidden>

I noticed you're dropping a comment about tlb_flush(). Is that because
it was dead code, or does the Memory API take care of it for us?
(I assume the former because memory_region_set_enabled() does not take a
CPUState* to operate on.)

Andreas

> ---
>  hw/integratorcp.c |   26 +++++++-------------------
>  1 files changed, 7 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/integratorcp.c b/hw/integratorcp.c
> index d9d8da3..e5712fc 100644
> --- a/hw/integratorcp.c
> +++ b/hw/integratorcp.c
> @@ -21,7 +21,6 @@ typedef struct {
>      MemoryRegion iomem;
>      uint32_t memsz;
>      MemoryRegion flash;
> -    bool flash_mapped;
>      uint32_t cm_osc;
>      uint32_t cm_ctrl;
>      uint32_t cm_lock;
> @@ -110,20 +109,12 @@ static uint64_t integratorcm_read(void *opaque, 
> target_phys_addr_t offset,
>      }
>  }
>  
> -static void integratorcm_do_remap(integratorcm_state *s, int flash)
> +static void integratorcm_do_remap(integratorcm_state *s)
>  {
> -    if (!flash) {
> -        if (s->flash_mapped) {
> -            sysbus_del_memory(&s->busdev, &s->flash);
> -            s->flash_mapped = false;
> -        }
> -    } else {
> -        if (!s->flash_mapped) {
> -            sysbus_add_memory_overlap(&s->busdev, 0, &s->flash, 1);
> -            s->flash_mapped = true;
> -        }
> -    }
> -    //??? tlb_flush (cpu_single_env, 1);
> +    /* Sync memory region state with CM_CTRL REMAP bit:
> +     * bit 0 => flash at address 0; bit 1 => RAM
> +     */
> +    memory_region_set_enabled(&s->flash, !(s->cm_ctrl & 4));
>  }
>  
>  static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value)
> @@ -131,9 +122,6 @@ static void integratorcm_set_ctrl(integratorcm_state *s, 
> uint32_t value)
>      if (value & 8) {
>          qemu_system_reset_request();
>      }
> -    if ((s->cm_ctrl ^ value) & 4) {
> -        integratorcm_do_remap(s, (value & 4) == 0);
> -    }
>      if ((s->cm_ctrl ^ value) & 1) {
>          /* (value & 1) != 0 means the green "MISC LED" is lit.
>           * We don't have any nice place to display LEDs. printf is a bad
> @@ -143,6 +131,7 @@ static void integratorcm_set_ctrl(integratorcm_state *s, 
> uint32_t value)
>      }
>      /* Note that the RESET bit [3] always reads as zero */
>      s->cm_ctrl = (s->cm_ctrl & ~5) | (value & 5);
> +    integratorcm_do_remap(s);
>  }
>  
>  static void integratorcm_update(integratorcm_state *s)
> @@ -263,13 +252,12 @@ static int integratorcm_init(SysBusDevice *dev)
>      s->cm_init = 0x00000112;
>      memory_region_init_ram(&s->flash, "integrator.flash", 0x100000);
>      vmstate_register_ram_global(&s->flash);
> -    s->flash_mapped = false;
>  
>      memory_region_init_io(&s->iomem, &integratorcm_ops, s,
>                            "integratorcm", 0x00800000);
>      sysbus_init_mmio(dev, &s->iomem);
>  
> -    integratorcm_do_remap(s, 1);
> +    integratorcm_do_remap(s);
>      /* ??? Save/restore.  */
>      return 0;
>  }

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg