Re: [Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Peter Maydell]

On 19 December 2015 at 21:39, Peter Crosthwaite
 wrote:
> On Fri, Dec 11, 2015 at 04:37:06PM +, Peter Maydell wrote:
>> +carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD);
>> +qdev_prop_set_drive(carddev, "drive", blk, errp);
>> +if (*errp) {
>
> It is generally valid for an errp to be NULL. I think we should use a
> local even if we know the caller will not pass NULL.

Agreed.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Peter Crosthwaite]

On Fri, Dec 11, 2015 at 04:37:06PM +, Peter Maydell wrote:
> Update the SDHCI code to use the new SDBus APIs.
> 
> This commit introduces the new command line options required
> to connect a disk to sdhci-pci:
> 
>  -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive
> 
> Signed-off-by: Peter Maydell 
> ---
>  hw/sd/sdhci.c | 95 
> +++
>  include/hw/sd/sdhci.h |  3 +-
>  2 files changed, 67 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 991c9b5..17e08a2 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -55,6 +55,9 @@
>  } \
>  } while (0)
>  
> +#define TYPE_SDHCI_BUS "sdhci-bus"
> +#define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)
> +
>  /* Default SD/MMC host controller features information, which will be
>   * presented in CAPABILITIES register of generic SD host controller at reset.
>   * If not stated otherwise:
> @@ -145,9 +148,9 @@ static void sdhci_raise_insertion_irq(void *opaque)
>  }
>  }
>  
> -static void sdhci_insert_eject_cb(void *opaque, int irq, int level)
> +static void sdhci_set_inserted(DeviceState *dev, bool level)
>  {
> -SDHCIState *s = (SDHCIState *)opaque;
> +SDHCIState *s = (SDHCIState *)dev;
>  DPRINT_L1("Card state changed: %s!\n", level ? "insert" : "eject");
>  
>  if ((s->norintsts & SDHC_NIS_REMOVE) && level) {
> @@ -172,9 +175,9 @@ static void sdhci_insert_eject_cb(void *opaque, int irq, 
> int level)
>  }
>  }
>  
> -static void sdhci_card_readonly_cb(void *opaque, int irq, int level)
> +static void sdhci_set_readonly(DeviceState *dev, bool level)
>  {
> -SDHCIState *s = (SDHCIState *)opaque;
> +SDHCIState *s = (SDHCIState *)dev;
>  
>  if (level) {
>  s->prnsts &= ~SDHC_WRITE_PROTECT;
> @@ -186,6 +189,8 @@ static void sdhci_card_readonly_cb(void *opaque, int irq, 
> int level)
>  
>  static void sdhci_reset(SDHCIState *s)
>  {
> +DeviceState *dev = DEVICE(s);
> +
>  timer_del(s->insert_timer);
>  timer_del(s->transfer_timer);
>  /* Set all registers to 0. Capabilities registers are not cleared
> @@ -193,7 +198,10 @@ static void sdhci_reset(SDHCIState *s)
>   * initialization */
>  memset(>sdmasysad, 0, (uintptr_t)>capareg - 
> (uintptr_t)>sdmasysad);
>  
> -sd_set_cb(s->card, s->ro_cb, s->eject_cb);
> +/* Reset other state based on current card insertion/readonly status */
> +sdhci_set_inserted(dev, sdbus_get_inserted(>sdbus));
> +sdhci_set_readonly(dev, sdbus_get_readonly(>sdbus));
> +
>  s->data_count = 0;
>  s->stopped_state = sdhc_not_stopped;
>  }
> @@ -211,7 +219,7 @@ static void sdhci_send_command(SDHCIState *s)
>  request.cmd = s->cmdreg >> 8;
>  request.arg = s->argument;
>  DPRINT_L1("sending CMD%u ARG[0x%08x]\n", request.cmd, request.arg);
> -rlen = sd_do_command(s->card, , response);
> +rlen = sdbus_do_command(>sdbus, , response);
>  
>  if (s->cmdreg & SDHC_CMD_RESPONSE) {
>  if (rlen == 4) {
> @@ -270,7 +278,7 @@ static void sdhci_end_transfer(SDHCIState *s)
>  request.cmd = 0x0C;
>  request.arg = 0;
>  DPRINT_L1("Automatically issue CMD%d %08x\n", request.cmd, 
> request.arg);
> -sd_do_command(s->card, , response);
> +sdbus_do_command(>sdbus, , response);
>  /* Auto CMD12 response goes to the upper Response register */
>  s->rspreg[3] = (response[0] << 24) | (response[1] << 16) |
>  (response[2] << 8) | response[3];
> @@ -302,7 +310,7 @@ static void sdhci_read_block_from_card(SDHCIState *s)
>  }
>  
>  for (index = 0; index < (s->blksize & 0x0fff); index++) {
> -s->fifo_buffer[index] = sd_read_data(s->card);
> +s->fifo_buffer[index] = sdbus_read_data(>sdbus);
>  }
>  
>  /* New data now available for READ through Buffer Port Register */
> @@ -395,7 +403,7 @@ static void sdhci_write_block_to_card(SDHCIState *s)
>  }
>  
>  for (index = 0; index < (s->blksize & 0x0fff); index++) {
> -sd_write_data(s->card, s->fifo_buffer[index]);
> +sdbus_write_data(>sdbus, s->fifo_buffer[index]);
>  }
>  
>  /* Next data can be written through BUFFER DATORT register */
> @@ -477,7 +485,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] = sd_read_data(s->card);
> +s->fifo_buffer[n] = sdbus_read_data(>sdbus);
>  }
>  }
>  begin = s->data_count;
> @@ -518,7 +526,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++) {
> -   

[Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Peter Maydell]

Update the SDHCI code to use the new SDBus APIs.

This commit introduces the new command line options required
to connect a disk to sdhci-pci:

 -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive

Signed-off-by: Peter Maydell 
---
 hw/sd/sdhci.c | 95 +++
 include/hw/sd/sdhci.h |  3 +-
 2 files changed, 67 insertions(+), 31 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 991c9b5..17e08a2 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -55,6 +55,9 @@
 } \
 } while (0)
 
+#define TYPE_SDHCI_BUS "sdhci-bus"
+#define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)
+
 /* Default SD/MMC host controller features information, which will be
  * presented in CAPABILITIES register of generic SD host controller at reset.
  * If not stated otherwise:
@@ -145,9 +148,9 @@ static void sdhci_raise_insertion_irq(void *opaque)
 }
 }
 
-static void sdhci_insert_eject_cb(void *opaque, int irq, int level)
+static void sdhci_set_inserted(DeviceState *dev, bool level)
 {
-SDHCIState *s = (SDHCIState *)opaque;
+SDHCIState *s = (SDHCIState *)dev;
 DPRINT_L1("Card state changed: %s!\n", level ? "insert" : "eject");
 
 if ((s->norintsts & SDHC_NIS_REMOVE) && level) {
@@ -172,9 +175,9 @@ static void sdhci_insert_eject_cb(void *opaque, int irq, 
int level)
 }
 }
 
-static void sdhci_card_readonly_cb(void *opaque, int irq, int level)
+static void sdhci_set_readonly(DeviceState *dev, bool level)
 {
-SDHCIState *s = (SDHCIState *)opaque;
+SDHCIState *s = (SDHCIState *)dev;
 
 if (level) {
 s->prnsts &= ~SDHC_WRITE_PROTECT;
@@ -186,6 +189,8 @@ static void sdhci_card_readonly_cb(void *opaque, int irq, 
int level)
 
 static void sdhci_reset(SDHCIState *s)
 {
+DeviceState *dev = DEVICE(s);
+
 timer_del(s->insert_timer);
 timer_del(s->transfer_timer);
 /* Set all registers to 0. Capabilities registers are not cleared
@@ -193,7 +198,10 @@ static void sdhci_reset(SDHCIState *s)
  * initialization */
 memset(>sdmasysad, 0, (uintptr_t)>capareg - 
(uintptr_t)>sdmasysad);
 
-sd_set_cb(s->card, s->ro_cb, s->eject_cb);
+/* Reset other state based on current card insertion/readonly status */
+sdhci_set_inserted(dev, sdbus_get_inserted(>sdbus));
+sdhci_set_readonly(dev, sdbus_get_readonly(>sdbus));
+
 s->data_count = 0;
 s->stopped_state = sdhc_not_stopped;
 }
@@ -211,7 +219,7 @@ static void sdhci_send_command(SDHCIState *s)
 request.cmd = s->cmdreg >> 8;
 request.arg = s->argument;
 DPRINT_L1("sending CMD%u ARG[0x%08x]\n", request.cmd, request.arg);
-rlen = sd_do_command(s->card, , response);
+rlen = sdbus_do_command(>sdbus, , response);
 
 if (s->cmdreg & SDHC_CMD_RESPONSE) {
 if (rlen == 4) {
@@ -270,7 +278,7 @@ static void sdhci_end_transfer(SDHCIState *s)
 request.cmd = 0x0C;
 request.arg = 0;
 DPRINT_L1("Automatically issue CMD%d %08x\n", request.cmd, 
request.arg);
-sd_do_command(s->card, , response);
+sdbus_do_command(>sdbus, , response);
 /* Auto CMD12 response goes to the upper Response register */
 s->rspreg[3] = (response[0] << 24) | (response[1] << 16) |
 (response[2] << 8) | response[3];
@@ -302,7 +310,7 @@ static void sdhci_read_block_from_card(SDHCIState *s)
 }
 
 for (index = 0; index < (s->blksize & 0x0fff); index++) {
-s->fifo_buffer[index] = sd_read_data(s->card);
+s->fifo_buffer[index] = sdbus_read_data(>sdbus);
 }
 
 /* New data now available for READ through Buffer Port Register */
@@ -395,7 +403,7 @@ static void sdhci_write_block_to_card(SDHCIState *s)
 }
 
 for (index = 0; index < (s->blksize & 0x0fff); index++) {
-sd_write_data(s->card, s->fifo_buffer[index]);
+sdbus_write_data(>sdbus, s->fifo_buffer[index]);
 }
 
 /* Next data can be written through BUFFER DATORT register */
@@ -477,7 +485,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] = sd_read_data(s->card);
+s->fifo_buffer[n] = sdbus_read_data(>sdbus);
 }
 }
 begin = s->data_count;
@@ -518,7 +526,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++) {
-sd_write_data(s->card, s->fifo_buffer[n]);
+sdbus_write_data(>sdbus, s->fifo_buffer[n]);
 }
 s->data_count = 0;
 if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
@@ -550,7 +558,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
 
 if (s->trnmod 

Re: [Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Peter Maydell]

On 11 December 2015 at 19:01, Kevin O'Connor  wrote:
> On Fri, Dec 11, 2015 at 04:37:06PM +, Peter Maydell wrote:
>> Update the SDHCI code to use the new SDBus APIs.
>>
>> This commit introduces the new command line options required
>> to connect a disk to sdhci-pci:
>>
>>  -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive
>
> I can't review in depth right now, but I did notice
>
> [...]
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -55,6 +55,9 @@
>>  } \
>>  } while (0)
>>
>> +#define TYPE_SDHCI_BUS "sdhci-bus"
>> +#define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)
>
> the above PXA2XX typo

Oops. We never actually use this macro, so I didn't notice the
cut-n-paste error.

> [...]
>> @@ -1303,6 +1323,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, 
>> Error ** errp)
>>  memory_region_init_io(>iomem, OBJECT(s), _mmio_ops, s, "sdhci",
>>  SDHC_REGISTERS_MAP_SIZE);
>>  sysbus_init_mmio(sbd, >iomem);
>> +
>>  }
>
> and the above white space damage.

Will fix.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/10] hw/sd/sdhci.c: Update to use SDBus APIs

[Kevin O'Connor]

On Fri, Dec 11, 2015 at 04:37:06PM +, Peter Maydell wrote:
> Update the SDHCI code to use the new SDBus APIs.
> 
> This commit introduces the new command line options required
> to connect a disk to sdhci-pci:
> 
>  -device sdhci-pci -drive id=mydrive,[...] -device sd,drive=mydrive

I can't review in depth right now, but I did notice

[...]
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -55,6 +55,9 @@
>  } \
>  } while (0)
>  
> +#define TYPE_SDHCI_BUS "sdhci-bus"
> +#define SDHCI_BUS(obj) OBJECT_CHECK(SDBus, (obj), TYPE_PXA2XX_MMCI_BUS)

the above PXA2XX typo

[...]
> @@ -1303,6 +1323,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, 
> Error ** errp)
>  memory_region_init_io(>iomem, OBJECT(s), _mmio_ops, s, "sdhci",
>  SDHC_REGISTERS_MAP_SIZE);
>  sysbus_init_mmio(sbd, >iomem);
> +
>  }

and the above white space damage.

-Kevin