Re: [Qemu-arm] [Qemu-devel] [PATCH 04/11] sdbus: add sdbus_create_bus() to replace qbus_create_inplace()

[Alistair Francis]

On Wed, Dec 13, 2017 at 12:44 PM, Philippe Mathieu-Daudé
<address@hidden> wrote:
> A "bus-name" property can be use to change the default SDHCI "sd-bus" name.
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  include/hw/sd/sd.h           |  1 +
>  include/hw/sd/sdhci.h        |  3 ++-
>  hw/arm/bcm2835_peripherals.c |  2 +-
>  hw/sd/core.c                 |  5 +++++
>  hw/sd/sdhci.c                | 36 ++++++++++++++++++++----------------
>  5 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
> index f6994e61f2..dc9d697c12 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -51,6 +51,7 @@ struct SDBus {
>  };
>
>  /* Functions to be used by qdevified callers */
> +SDBus *sdbus_create_bus(DeviceState *parent, const char *name);
>  int sdbus_do_command(SDBus *sd, SDRequest *req, uint8_t *response);
>  void sdbus_write_data(SDBus *sd, uint8_t value);
>  uint8_t sdbus_read_data(SDBus *sd);
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index e6644e6e7d..579e0ea644 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -39,7 +39,8 @@ typedef struct SDHCIState {
>      };
>
>      /*< public >*/
> -    SDBus sdbus;
> +    char *bus_name;
> +    SDBus *sdbus;
>      MemoryRegion iomem;
>      MemoryRegion *dma_mr;
>      AddressSpace dma_as;
> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> index 7f30f19c4c..60a0eec4d1 100644
> --- a/hw/arm/bcm2835_peripherals.c
> +++ b/hw/arm/bcm2835_peripherals.c
> @@ -310,7 +310,7 @@ static void bcm2835_peripherals_realize(DeviceState *dev, 
> Error **errp)
>
>      /* GPIO */
>      object_property_add_const_link(OBJECT(&s->gpio), "sdbus-sdhci",
> -                                   OBJECT(&s->sdhci.sdbus), &error_abort);
> +                                   OBJECT(s->sdhci.sdbus), &error_abort);
>      object_property_add_const_link(OBJECT(&s->gpio), "sdbus-sdhost",
>                                     OBJECT(&s->sdhost.sdbus), &error_abort);
>      object_property_set_bool(OBJECT(&s->gpio), true, "realized", &err);
> diff --git a/hw/sd/core.c b/hw/sd/core.c
> index eda595973b..021a8d7258 100644
> --- a/hw/sd/core.c
> +++ b/hw/sd/core.c
> @@ -187,3 +187,8 @@ static void sd_bus_register_types(void)
>  }
>
>  type_init(sd_bus_register_types)
> +
> +SDBus *sdbus_create_bus(DeviceState *parent, const char *name)
> +{
> +    return SD_BUS(qbus_create(TYPE_SD_BUS, parent, name));
> +}
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 5f8064c59b..40596f6ebe 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -186,8 +186,8 @@ static void sdhci_reset(SDHCIState *s)
>      memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - 
> (uintptr_t)&s->sdmasysad);
>
>      /* Reset other state based on current card insertion/readonly status */
> -    sdhci_set_inserted(dev, sdbus_get_inserted(&s->sdbus));
> -    sdhci_set_readonly(dev, sdbus_get_readonly(&s->sdbus));
> +    sdhci_set_inserted(dev, sdbus_get_inserted(s->sdbus));
> +    sdhci_set_readonly(dev, sdbus_get_readonly(s->sdbus));
>
>      s->data_count = 0;
>      s->stopped_state = sdhc_not_stopped;
> @@ -223,7 +223,7 @@ static void sdhci_send_command(SDHCIState *s)
>      request.arg = s->argument;
>
>      trace_sdhci_send_command(request.cmd, request.arg);
> -    rlen = sdbus_do_command(&s->sdbus, &request, response);
> +    rlen = sdbus_do_command(s->sdbus, &request, response);
>
>      if (s->cmdreg & SDHC_CMD_RESPONSE) {
>          if (rlen == 4) {
> @@ -278,7 +278,7 @@ static void sdhci_end_transfer(SDHCIState *s)
>          request.cmd = 0x0C;
>          request.arg = 0;
>          trace_sdhci_end_transfer(request.cmd, request.arg);
> -        sdbus_do_command(&s->sdbus, &request, response);
> +        sdbus_do_command(s->sdbus, &request, response);
>          /* Auto CMD12 response goes to the upper Response register */
>          s->rspreg[3] = (response[0] << 24) | (response[1] << 16) |
>                  (response[2] << 8) | response[3];
> @@ -310,7 +310,7 @@ static void sdhci_read_block_from_card(SDHCIState *s)
>      }
>
>      for (index = 0; index < (s->blksize & 0x0fff); index++) {
> -        s->fifo_buffer[index] = sdbus_read_data(&s->sdbus);
> +        s->fifo_buffer[index] = sdbus_read_data(s->sdbus);
>      }
>
>      /* New data now available for READ through Buffer Port Register */
> @@ -402,7 +402,7 @@ static void sdhci_write_block_to_card(SDHCIState *s)
>      }
>
>      for (index = 0; index < (s->blksize & 0x0fff); index++) {
> -        sdbus_write_data(&s->sdbus, s->fifo_buffer[index]);
> +        sdbus_write_data(s->sdbus, s->fifo_buffer[index]);
>      }
>
>      /* Next data can be written through BUFFER DATORT register */
> @@ -488,7 +488,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState 
> *s)
>          while (s->blkcnt) {
>              if (s->data_count == 0) {
>                  for (n = 0; n < block_size; n++) {
> -                    s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
> +                    s->fifo_buffer[n] = sdbus_read_data(s->sdbus);
>                  }
>              }
>              begin = s->data_count;
> @@ -529,7 +529,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState 
> *s)
>              s->sdmasysad += s->data_count - begin;
>              if (s->data_count == block_size) {
>                  for (n = 0; n < block_size; n++) {
> -                    sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
> +                    sdbus_write_data(s->sdbus, s->fifo_buffer[n]);
>                  }
>                  s->data_count = 0;
>                  if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
> @@ -560,7 +560,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState 
> *s)
>
>      if (s->trnmod & SDHC_TRNS_READ) {
>          for (n = 0; n < datacnt; n++) {
> -            s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
> +            s->fifo_buffer[n] = sdbus_read_data(s->sdbus);
>          }
>          dma_memory_write(&address_space_memory, s->sdmasysad, s->fifo_buffer,
>                           datacnt);
> @@ -568,7 +568,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState 
> *s)
>          dma_memory_read(&address_space_memory, s->sdmasysad, s->fifo_buffer,
>                          datacnt);
>          for (n = 0; n < datacnt; n++) {
> -            sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
> +            sdbus_write_data(s->sdbus, s->fifo_buffer[n]);
>          }
>      }
>      s->blkcnt--;
> @@ -659,7 +659,7 @@ static void sdhci_do_adma(SDHCIState *s)
>                  while (length) {
>                      if (s->data_count == 0) {
>                          for (n = 0; n < block_size; n++) {
> -                            s->fifo_buffer[n] = sdbus_read_data(&s->sdbus);
> +                            s->fifo_buffer[n] = sdbus_read_data(s->sdbus);
>                          }
>                      }
>                      begin = s->data_count;
> @@ -700,7 +700,7 @@ static void sdhci_do_adma(SDHCIState *s)
>                      dscr.addr += s->data_count - begin;
>                      if (s->data_count == block_size) {
>                          for (n = 0; n < block_size; n++) {
> -                            sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
> +                            sdbus_write_data(s->sdbus, s->fifo_buffer[n]);
>                          }
>                          s->data_count = 0;
>                          if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
> @@ -807,7 +807,7 @@ static void sdhci_data_transfer(void *opaque)
>              break;
>          }
>      } else {
> -        if ((s->trnmod & SDHC_TRNS_READ) && sdbus_data_ready(&s->sdbus)) {
> +        if ((s->trnmod & SDHC_TRNS_READ) && sdbus_data_ready(s->sdbus)) {
>              s->prnsts |= SDHC_DOING_READ | SDHC_DATA_INHIBIT |
>                      SDHC_DAT_LINE_ACTIVE;
>              sdhci_read_block_from_card(s);
> @@ -1156,15 +1156,14 @@ static inline unsigned int 
> sdhci_get_fifolen(SDHCIState *s)
>
>  static void sdhci_initfn(SDHCIState *s)
>  {
> -    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
> -                        TYPE_SDHCI_BUS, DEVICE(s), "sd-bus");

Isn't "sd-bus" here the name? Can't you just move this line to realize
and pass the string in there?

Alistair

> -
>      s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, 
> sdhci_raise_insertion_irq, s);
>      s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, 
> sdhci_data_transfer, s);
>  }
>
>  static void sdhci_realizefn(SDHCIState *s, Error **errp)
>  {
> +    const char *name = s->bus_name ? s->bus_name : "sd-bus";
> +
>      sdhci_init_capareg(s, errp);
>
>      s->buf_maxsz = sdhci_get_fifolen(s);
> @@ -1177,10 +1176,14 @@ static void sdhci_realizefn(SDHCIState *s, Error 
> **errp)
>      address_space_init(&s->dma_as,
>                         s->dma_mr ? s->dma_mr : get_system_memory(),
>                         "sdhci-dma");
> +
> +    s->sdbus = sdbus_create_bus(DEVICE(s), name);
>  }
>
>  static void sdhci_unrealizefn(SDHCIState *s, Error **errp)
>  {
> +    g_free(s->sdbus);
> +
>      if (s->dma_mr) {
>          address_space_destroy(&s->dma_as);
>          object_unparent(OBJECT(&s->dma_mr));
> @@ -1269,6 +1272,7 @@ static Property sdhci_properties[] = {
>                       false),
>      DEFINE_PROP_LINK("dma-memory", SDHCIState, dma_mr,
>                       TYPE_MEMORY_REGION, MemoryRegion *),
> +    DEFINE_PROP_STRING("sd-bus-name", SDHCIState, bus_name),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> --
> 2.15.1
>
>