Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

2016-01-08 Thread Peter Crosthwaite 
On Thu, Jan 7, 2016 at 10:09 AM, Peter Maydell  wrote:
> On 20 December 2015 at 20:51, Peter Crosthwaite
>  wrote:
>> On Sun, Dec 20, 2015 at 9:10 AM, Peter Maydell  
>> wrote:
>>> For user-level back compat I think we need to retain "might have
>>> an sdcard object with no block backend, and that means
>>> 'no-card-present'". This is both for the user facing
>>> monitor commands to manipulate the sd card, and also
>>
>> What are the user-facing monitor commands? I tried using "change" and
>> "eject", but they don't seem to work for SD, due to the tray being
>> closed. Has this ever worked in a way that is user manipulatable for
>> SD or is it just to handle the case of unconditional SD card creation
>> (with the card never hotplugging over the system lifetime)?
>
> I investigated this, and it looks like we accidentally broke
> 'change' for SD cards in 2.5 (specifically in commit de2c6c05).
> I think we should fix that regression, which in turn implies that
> we still want to support the "sd card object with no block backend" case.
>

Yes, saw the patches on list. I guess we are stuck with it. It would
be good to do this in a way that supports use of the hotplug API
alongside though, so SDIO device could all be ejected and inserted
with the same way.

Regards,
Peter

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

2015-12-20 Thread Peter Crosthwaite 
On Sun, Dec 20, 2015 at 3:18 PM, Peter Maydell  wrote:
> On 20 December 2015 at 20:51, Peter Crosthwaite
>  wrote:
>> On Sun, Dec 20, 2015 at 9:10 AM, Peter Maydell  
>> wrote:
>>> On 19 December 2015 at 21:38, Peter Crosthwaite
>>>  wrote:
 On Fri, Dec 11, 2015 at 04:37:05PM +, Peter Maydell wrote:
> +bool sdbus_get_inserted(SDBus *sdbus)
> +{
> +SDState *card = get_card(sdbus);
> +
> +if (card) {
> +SDClass *sc = SD_GET_CLASS(card);
> +
> +return sc->get_inserted(card);
> +}
> +
> +return false;
> +}

 This I am not sure about. Realistically, the card has no self
 awareness of its ejection state so it can't be polled for "are
 you there". The card insertion switch is implemented as a
 physical switch on the slot itself and a property of the bus.

 The absence on presence of a device should determine this, making me
 think this should return !!card.

 Unfortunately, we have the case of the block UI being able to trigger a
 card ejection from underneath the bus level. But since the SD card is 
 already
 using qdev_get_parent_bus() the removal from the bus can be managed at the
 card level.
>>>
>>> For user-level back compat I think we need to retain "might have
>>> an sdcard object with no block backend, and that means
>>> 'no-card-present'". This is both for the user facing
>>> monitor commands to manipulate the sd card, and also
>>
>> What are the user-facing monitor commands? I tried using "change" and
>> "eject", but they don't seem to work for SD, due to the tray being
>> closed. Has this ever worked in a way that is user manipulatable for
>> SD or is it just to handle the case of unconditional SD card creation
>> (with the card never hotplugging over the system lifetime)?
>
> I admit to just assuming that this stuff worked rather than
> testing it :-)
>

Can we capitilise on the opportunity here to break the backwards
compat for the non-functional feature and defeature eject and change
for SD? File substitution for SD cards doesn't match the real world
nicely, as we use the file size to modify the SD card implementation
(SDSC vs SDHC as well as the # of blocks). It really should be a
destroy+recreate rather than a substitution.We can then deal with SD
card ejection and insertion as proper hotplug.

Regards,
Peter

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

2015-12-20 Thread Peter Maydell 
On 20 December 2015 at 20:51, Peter Crosthwaite
 wrote:
> On Sun, Dec 20, 2015 at 9:10 AM, Peter Maydell  
> wrote:
>> On 19 December 2015 at 21:38, Peter Crosthwaite
>>  wrote:
>>> On Fri, Dec 11, 2015 at 04:37:05PM +, Peter Maydell wrote:
 +bool sdbus_get_inserted(SDBus *sdbus)
 +{
 +SDState *card = get_card(sdbus);
 +
 +if (card) {
 +SDClass *sc = SD_GET_CLASS(card);
 +
 +return sc->get_inserted(card);
 +}
 +
 +return false;
 +}
>>>
>>> This I am not sure about. Realistically, the card has no self
>>> awareness of its ejection state so it can't be polled for "are
>>> you there". The card insertion switch is implemented as a
>>> physical switch on the slot itself and a property of the bus.
>>>
>>> The absence on presence of a device should determine this, making me
>>> think this should return !!card.
>>>
>>> Unfortunately, we have the case of the block UI being able to trigger a
>>> card ejection from underneath the bus level. But since the SD card is 
>>> already
>>> using qdev_get_parent_bus() the removal from the bus can be managed at the
>>> card level.
>>
>> For user-level back compat I think we need to retain "might have
>> an sdcard object with no block backend, and that means
>> 'no-card-present'". This is both for the user facing
>> monitor commands to manipulate the sd card, and also
>
> What are the user-facing monitor commands? I tried using "change" and
> "eject", but they don't seem to work for SD, due to the tray being
> closed. Has this ever worked in a way that is user manipulatable for
> SD or is it just to handle the case of unconditional SD card creation
> (with the card never hotplugging over the system lifetime)?

I admit to just assuming that this stuff worked rather than
testing it :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

2015-12-20 Thread Peter Crosthwaite 
On Sun, Dec 20, 2015 at 9:10 AM, Peter Maydell  wrote:
> On 19 December 2015 at 21:38, Peter Crosthwaite
>  wrote:
>> On Fri, Dec 11, 2015 at 04:37:05PM +, Peter Maydell wrote:
>>> +bool sdbus_get_inserted(SDBus *sdbus)
>>> +{
>>> +SDState *card = get_card(sdbus);
>>> +
>>> +if (card) {
>>> +SDClass *sc = SD_GET_CLASS(card);
>>> +
>>> +return sc->get_inserted(card);
>>> +}
>>> +
>>> +return false;
>>> +}
>>
>> This I am not sure about. Realistically, the card has no self
>> awareness of its ejection state so it can't be polled for "are
>> you there". The card insertion switch is implemented as a
>> physical switch on the slot itself and a property of the bus.
>>
>> The absence on presence of a device should determine this, making me
>> think this should return !!card.
>>
>> Unfortunately, we have the case of the block UI being able to trigger a
>> card ejection from underneath the bus level. But since the SD card is already
>> using qdev_get_parent_bus() the removal from the bus can be managed at the
>> card level.
>
> For user-level back compat I think we need to retain "might have
> an sdcard object with no block backend, and that means
> 'no-card-present'". This is both for the user facing
> monitor commands to manipulate the sd card, and also

What are the user-facing monitor commands? I tried using "change" and
"eject", but they don't seem to work for SD, due to the tray being
closed. Has this ever worked in a way that is user manipulatable for
SD or is it just to handle the case of unconditional SD card creation
(with the card never hotplugging over the system lifetime)?

Test case below. Using ep108 with a backendless drive (-drive if=sd).

$ dd if=/dev/zero of=./sd0file bs=16M count=1
1+0 records in
1+0 records out
16777216 bytes (17 MB) copied, 0.170586 s, 98.4 MB/s
$ dd if=/dev/zero of=./sd1file bs=16M count=1
1+0 records in
1+0 records out
16777216 bytes (17 MB) copied, 0.183836 s, 91.3 MB/s
$ ./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -S -kernel
/dev/null -sd sd0file -drive if=sd -nographic
WARNING: Image format was not specified for 'sd0file' and probing guessed raw.
 Automatically detecting the format is dangerous for raw
images, write operations on block 0 will be restricted.
 Specify the 'raw' format explicitly to remove the restrictions.
QEMU 2.5.50 monitor - type 'help' for more information
(qemu) info block
sd0 (#block197): sd0file (raw)
Removable device: not locked, tray closed
Cache mode:   writeback

sd1: [not inserted]
Removable device: not locked, tray closed

ide1-cd0: [not inserted]
Removable device: not locked, tray closed

floppy0: [not inserted]
Removable device: not locked, tray closed
(qemu) change sd1 sd1file raw
Tray of device 'sd1' is not open
(qemu) eject -f sd1
Tray of device 'sd1' is not open
(qemu) info block
sd0 (#block197): sd0file (raw)
Removable device: not locked, tray closed
Cache mode:   writeback

sd1: [not inserted]
Removable device: not locked, tray closed

ide1-cd0: [not inserted]
Removable device: not locked, tray closed

floppy0: [not inserted]
Removable device: not locked, tray closed
(qemu)

Regards,
Peter

> for the not-yet-QOMified controllers, which will always
> create an sdcard object even if there's no block backend
> (which should continue to mean "no card").
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

2015-12-20 Thread Peter Maydell 
On 19 December 2015 at 21:38, Peter Crosthwaite
 wrote:
> On Fri, Dec 11, 2015 at 04:37:05PM +, Peter Maydell wrote:
>> +bool sdbus_get_inserted(SDBus *sdbus)
>> +{
>> +SDState *card = get_card(sdbus);
>> +
>> +if (card) {
>> +SDClass *sc = SD_GET_CLASS(card);
>> +
>> +return sc->get_inserted(card);
>> +}
>> +
>> +return false;
>> +}
>
> This I am not sure about. Realistically, the card has no self
> awareness of its ejection state so it can't be polled for "are
> you there". The card insertion switch is implemented as a
> physical switch on the slot itself and a property of the bus.
>
> The absence on presence of a device should determine this, making me
> think this should return !!card.
>
> Unfortunately, we have the case of the block UI being able to trigger a
> card ejection from underneath the bus level. But since the SD card is already
> using qdev_get_parent_bus() the removal from the bus can be managed at the
> card level.

For user-level back compat I think we need to retain "might have
an sdcard object with no block backend, and that means
'no-card-present'". This is both for the user facing
monitor commands to manipulate the sd card, and also
for the not-yet-QOMified controllers, which will always
create an sdcard object even if there's no block backend
(which should continue to mean "no card").

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

2015-12-19 Thread Peter Crosthwaite 
On Fri, Dec 11, 2015 at 04:37:05PM +, Peter Maydell wrote:
> Add a QOM bus for SD cards to plug in to.
> 
> Note that since sd_enable() is used only by one board and there
> only as part of a broken implementation, we do not provide it in
> the SDBus API (but instead add a warning comment about the old
> function). Whoever converts OMAP and the nseries boards to QOM
> will need to either implement the card switch properly or move
> the enable hack into the OMAP MMC controller model.
> 
> In the SDBus API, the old-style use of sd_set_cb to register some
> qemu_irqs for notification of card insertion and write-protect
> toggling is replaced with methods in the SDBusClass which the
> card calls on status changes and methods in the SDClass which
> the controller can call to find out the current status. The
> query methods will allow us to remove the abuse of the 'register
> irqs' API by controllers in their reset methods to trigger
> the card to tell them about the current status again.
> 
> Signed-off-by: Peter Maydell 
> ---
>  hw/sd/Makefile.objs |   2 +-
>  hw/sd/core.c| 146 
> 
>  hw/sd/sd.c  |  46 +++--
>  include/hw/sd/sd.h  |  60 +
>  4 files changed, 249 insertions(+), 5 deletions(-)
>  create mode 100644 hw/sd/core.c
> 
> diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
> index f1aed83..31c8330 100644
> --- a/hw/sd/Makefile.objs
> +++ b/hw/sd/Makefile.objs
> @@ -1,6 +1,6 @@
>  common-obj-$(CONFIG_PL181) += pl181.o
>  common-obj-$(CONFIG_SSI_SD) += ssi-sd.o
> -common-obj-$(CONFIG_SD) += sd.o
> +common-obj-$(CONFIG_SD) += sd.o core.o
>  common-obj-$(CONFIG_SDHCI) += sdhci.o
>  
>  obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
> diff --git a/hw/sd/core.c b/hw/sd/core.c
> new file mode 100644
> index 000..584eeb5
> --- /dev/null
> +++ b/hw/sd/core.c
> @@ -0,0 +1,146 @@
> +/*
> + * SD card bus interface code.
> + *
> + * Copyright (c) 2015 Linaro Limited
> + *
> + * Author:
> + *  Peter Maydell 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program.  If not, see .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/qdev-core.h"
> +#include "sysemu/block-backend.h"
> +#include "hw/sd/sd.h"
> +
> +static SDState *get_card(SDBus *sdbus)
> +{
> +/* We only ever have one child on the bus so just return it */
> +BusChild *kid = QTAILQ_FIRST(&sdbus->qbus.children);
> +
> +if (!kid) {
> +return NULL;
> +}
> +return SD(kid->child);
> +}
> +
> +int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
> +{
> +SDState *card = get_card(sdbus);
> +
> +if (card) {
> +SDClass *sc = SD_GET_CLASS(card);
> +
> +return sc->do_command(card, req, response);
> +}
> +
> +return 0;
> +}
> +
> +void sdbus_write_data(SDBus *sdbus, uint8_t value)
> +{
> +SDState *card = get_card(sdbus);
> +
> +if (card) {
> +SDClass *sc = SD_GET_CLASS(card);
> +
> +sc->write_data(card, value);
> +}
> +}
> +
> +uint8_t sdbus_read_data(SDBus *sdbus)
> +{
> +SDState *card = get_card(sdbus);
> +
> +if (card) {
> +SDClass *sc = SD_GET_CLASS(card);
> +
> +return sc->read_data(card);
> +}
> +
> +return 0;
> +}
> +
> +bool sdbus_data_ready(SDBus *sdbus)
> +{
> +SDState *card = get_card(sdbus);
> +
> +if (card) {
> +SDClass *sc = SD_GET_CLASS(card);
> +
> +return sc->data_ready(card);
> +}
> +
> +return false;
> +}
> +
> +bool sdbus_get_inserted(SDBus *sdbus)
> +{
> +SDState *card = get_card(sdbus);
> +
> +if (card) {
> +SDClass *sc = SD_GET_CLASS(card);
> +
> +return sc->get_inserted(card);
> +}
> +
> +return false;
> +}

This I am not sure about. Realistically, the card has no self
awareness of its ejection state so it can't be polled for "are
you there". The card insertion switch is implemented as a
physical switch on the slot itself and a property of the bus.

The absence on presence of a device should determine this, making me
think this should return !!card.

Unfortunately, we have the case of the block UI being able to trigger a
card ejection from underneath the bus level. But since the SD card is already
using qdev_get_parent_bus() the removal from the bus can be managed at the
card level.

> +
> +bool sdbus_get_readonly(SDBus *sdbus)
> +{
> +SDState *card = get_card(sdbus);

[Qemu-devel] [PATCH 04/10] hw/sd: Add QOM bus which SD cards plug in to

2015-12-11 Thread Peter Maydell 
Add a QOM bus for SD cards to plug in to.

Note that since sd_enable() is used only by one board and there
only as part of a broken implementation, we do not provide it in
the SDBus API (but instead add a warning comment about the old
function). Whoever converts OMAP and the nseries boards to QOM
will need to either implement the card switch properly or move
the enable hack into the OMAP MMC controller model.

In the SDBus API, the old-style use of sd_set_cb to register some
qemu_irqs for notification of card insertion and write-protect
toggling is replaced with methods in the SDBusClass which the
card calls on status changes and methods in the SDClass which
the controller can call to find out the current status. The
query methods will allow us to remove the abuse of the 'register
irqs' API by controllers in their reset methods to trigger
the card to tell them about the current status again.

Signed-off-by: Peter Maydell 
---
 hw/sd/Makefile.objs |   2 +-
 hw/sd/core.c| 146 
 hw/sd/sd.c  |  46 +++--
 include/hw/sd/sd.h  |  60 +
 4 files changed, 249 insertions(+), 5 deletions(-)
 create mode 100644 hw/sd/core.c

diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
index f1aed83..31c8330 100644
--- a/hw/sd/Makefile.objs
+++ b/hw/sd/Makefile.objs
@@ -1,6 +1,6 @@
 common-obj-$(CONFIG_PL181) += pl181.o
 common-obj-$(CONFIG_SSI_SD) += ssi-sd.o
-common-obj-$(CONFIG_SD) += sd.o
+common-obj-$(CONFIG_SD) += sd.o core.o
 common-obj-$(CONFIG_SDHCI) += sdhci.o
 
 obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
diff --git a/hw/sd/core.c b/hw/sd/core.c
new file mode 100644
index 000..584eeb5
--- /dev/null
+++ b/hw/sd/core.c
@@ -0,0 +1,146 @@
+/*
+ * SD card bus interface code.
+ *
+ * Copyright (c) 2015 Linaro Limited
+ *
+ * Author:
+ *  Peter Maydell 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-core.h"
+#include "sysemu/block-backend.h"
+#include "hw/sd/sd.h"
+
+static SDState *get_card(SDBus *sdbus)
+{
+/* We only ever have one child on the bus so just return it */
+BusChild *kid = QTAILQ_FIRST(&sdbus->qbus.children);
+
+if (!kid) {
+return NULL;
+}
+return SD(kid->child);
+}
+
+int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+return sc->do_command(card, req, response);
+}
+
+return 0;
+}
+
+void sdbus_write_data(SDBus *sdbus, uint8_t value)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+sc->write_data(card, value);
+}
+}
+
+uint8_t sdbus_read_data(SDBus *sdbus)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+return sc->read_data(card);
+}
+
+return 0;
+}
+
+bool sdbus_data_ready(SDBus *sdbus)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+return sc->data_ready(card);
+}
+
+return false;
+}
+
+bool sdbus_get_inserted(SDBus *sdbus)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+return sc->get_inserted(card);
+}
+
+return false;
+}
+
+bool sdbus_get_readonly(SDBus *sdbus)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDClass *sc = SD_GET_CLASS(card);
+
+return sc->get_readonly(card);
+}
+
+return false;
+}
+
+void sdbus_set_inserted(SDBus *sdbus, bool inserted)
+{
+SDBusClass *sbc = SD_BUS_GET_CLASS(sdbus);
+BusState *qbus = BUS(sdbus);
+
+if (sbc->set_inserted) {
+sbc->set_inserted(qbus->parent, inserted);
+}
+}
+
+void sdbus_set_readonly(SDBus *sdbus, bool readonly)
+{
+SDBusClass *sbc = SD_BUS_GET_CLASS(sdbus);
+BusState *qbus = BUS(sdbus);
+
+if (sbc->set_readonly) {
+sbc->set_readonly(qbus->parent, readonly);
+}
+}
+
+static const TypeInfo sd_bus_info = {
+.name = TYPE_SD_BUS,
+.parent = TYPE_BUS,
+.instance_size = sizeof(SDBus),
+.class_size = sizeof(SDBusClass),
+};
+
+static void sd_bus_register_types(void)
+{
+type_register_static(&sd_bus_info);
+}
+
+type_init(sd_bus_register_types)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b4a5a62