diff mbox

[5/6] fw_cfg: use sysbus_init_mmio() in fw_cfg_io_realize()

Message ID 1497097821-32754-6-git-send-email-mark.cave-ayland@ilande.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Cave-Ayland June 10, 2017, 12:30 p.m. UTC 
This brings the function in line with fw_cfg_mem_realize(), deferring the
actual mapping until outside of the realize function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/nvram/fw_cfg.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Igor Mammedov June 12, 2017, 12:16 p.m. UTC | #1 
On Sat, 10 Jun 2017 13:30:20 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> This brings the function in line with fw_cfg_mem_realize(), deferring the
> actual mapping until outside of the realize function.
you are changing used API here, so add to commit message why is that

> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 87b4392..4159316 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -941,6 +941,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>                                  AddressSpace *dma_as)
>  {
>      DeviceState *dev;
> +    SysBusDevice *sbd;
>      FWCfgState *s;
>      bool dma_requested = dma_iobase && dma_as;
>  
> @@ -953,12 +954,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>  
>      qdev_init_nofail(dev);
>  
> +    sbd = SYS_BUS_DEVICE(dev);
> +    sysbus_add_io(ssysbus_mmio_map_overlapbd, iobase, sysbus_mmio_get_region(sbd, 0));
sysbus_mmio_map()/sysbus_mmio_map_overlap() is a proper conterpart for sysbus_init_mmio
so I'd use that if APIs are switched.

> +
>      s = FW_CFG(dev);
>  
>      if (s->dma_enabled) {
>          /* 64 bits for the address field */
>          s->dma_as = dma_as;
>          s->dma_addr = 0;
> +        sysbus_add_io(sbd, dma_iobase, sysbus_mmio_get_region(sbd, 1));
>      }
>  
>      return s;
> @@ -1090,13 +1095,13 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>       * of the i/o region used is FW_CFG_CTL_SIZE */
>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
>                            FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
> -    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
> +    sysbus_init_mmio(sbd, &s->comb_iomem);
>  
>      if (FW_CFG(s)->dma_enabled) {
>          memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
>                                &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
>                                sizeof(dma_addr_t));
> -        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
> +        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>      }
>  }
>
Mark Cave-Ayland June 12, 2017, 12:59 p.m. UTC | #2 
On 12/06/17 13:16, Igor Mammedov wrote:

> On Sat, 10 Jun 2017 13:30:20 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
>> This brings the function in line with fw_cfg_mem_realize(), deferring the
>> actual mapping until outside of the realize function.
> you are changing used API here, so add to commit message why is that

Okay I can add a comment mentioning that this is so we can wire up the
IO space ourselves?

>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/nvram/fw_cfg.c |    9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 87b4392..4159316 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -941,6 +941,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>>                                  AddressSpace *dma_as)
>>  {
>>      DeviceState *dev;
>> +    SysBusDevice *sbd;
>>      FWCfgState *s;
>>      bool dma_requested = dma_iobase && dma_as;
>>  
>> @@ -953,12 +954,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>>  
>>      qdev_init_nofail(dev);
>>  
>> +    sbd = SYS_BUS_DEVICE(dev);
>> +    sysbus_add_io(ssysbus_mmio_map_overlapbd, iobase, sysbus_mmio_get_region(sbd, 0));
> sysbus_mmio_map()/sysbus_mmio_map_overlap() is a proper conterpart for sysbus_init_mmio
> so I'd use that if APIs are switched.

Unfortunately we can't use sysbus_mmio_map() here because it maps the
resulting MemoryRegion into memory space instead of IO space.

The reason for using sysbus_init_mmio() here is that despite its name,
we can use sysbus_mmio_get_region() to obtain a reference to the
underlying s->comb_iomem MemoryRegion that the caller can use in order
to map the device into the desired IO space,

Otherwise if we use sysbus_add_io() at realize time as per the existing
code then the ioports are always mapped into system IO space which is
exactly the behaviour I'm trying to customise.

Note that this is how the m48t59 timer device is currently implemented
in hw/timer/m48t59.c.

> 
>> +
>>      s = FW_CFG(dev);
>>  
>>      if (s->dma_enabled) {
>>          /* 64 bits for the address field */
>>          s->dma_as = dma_as;
>>          s->dma_addr = 0;
>> +        sysbus_add_io(sbd, dma_iobase, sysbus_mmio_get_region(sbd, 1));
>>      }
>>  
>>      return s;
>> @@ -1090,13 +1095,13 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>>       * of the i/o region used is FW_CFG_CTL_SIZE */
>>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
>>                            FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
>> -    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
>> +    sysbus_init_mmio(sbd, &s->comb_iomem);
>>  
>>      if (FW_CFG(s)->dma_enabled) {
>>          memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
>>                                &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
>>                                sizeof(dma_addr_t));
>> -        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
>> +        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>>      }
>>  }
>>  


ATB,

Mark.
diff mbox

Patch

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 87b4392..4159316 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -941,6 +941,7 @@  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
                                 AddressSpace *dma_as)
 {
     DeviceState *dev;
+    SysBusDevice *sbd;
     FWCfgState *s;
     bool dma_requested = dma_iobase && dma_as;
 
@@ -953,12 +954,16 @@  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
 
     qdev_init_nofail(dev);
 
+    sbd = SYS_BUS_DEVICE(dev);
+    sysbus_add_io(sbd, iobase, sysbus_mmio_get_region(sbd, 0));
+
     s = FW_CFG(dev);
 
     if (s->dma_enabled) {
         /* 64 bits for the address field */
         s->dma_as = dma_as;
         s->dma_addr = 0;
+        sysbus_add_io(sbd, dma_iobase, sysbus_mmio_get_region(sbd, 1));
     }
 
     return s;
@@ -1090,13 +1095,13 @@  static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
      * of the i/o region used is FW_CFG_CTL_SIZE */
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
                           FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
-    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
+    sysbus_init_mmio(sbd, &s->comb_iomem);
 
     if (FW_CFG(s)->dma_enabled) {
         memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
                               &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
                               sizeof(dma_addr_t));
-        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
+        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
     }
 }