| Message ID | 20241010173929.3910466-1-tavip@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] cli: add connect-gpios option | expand |
On Thu, 10 Oct 2024 at 18:39, Octavian Purdila <tavip@google.com> wrote: > > From: Valentin Ghita <valentinghita@google.com> > > Add option to allow for connecting device GPIOs. This is useful when > adding a peripheral device from the command line which uses an > interrupt. > > It takes the following options: > > * in-dev-path, out-dev-path - required, the device canonical object > path (e.g. /machine/peripheral-anon/device[0], > /machine/iotkit/cluster0/armv7m0) for the devices that should have > their in <-> out GPIOs connected > > * in-gpio-name, out-gpio-name - optional, the name of the GPIO list; > if not specified, the unnamed GPIO list is used > > * in-gpio-index, out-gpio-index - optional, the index in the GPIO list > that identifies the GPIO to be used; if not specified 0 (the first > GPIO in the list) is used > > Usage example: > > # add the tmp105 sensor and connects its irq line to the CPU > qemu-system-arm \ > --machine mps2-an505 \ > --device tmp105,bus=/versatile_i2c/i2c,address=0x50 \ > --connect-gpios out-dev-path=/machine/peripheral-anon/device[0],\ > in-dev-path=/machine/iotkit/cluster0/armv7m0,in-gpio-index=100 This seems to be moving down the path towards "create and wire up machines on the command line". We shouldn't be doing that ad-hoc with one small commandline option at a time, we should be doing it with a coherent plan. I think Philippe will know the current intentions in this area. thanks -- PMM
On Fri, Oct 18, 2024 at 04:38:24PM +0100, Peter Maydell wrote: > On Thu, 10 Oct 2024 at 18:39, Octavian Purdila <tavip@google.com> wrote: > > > > From: Valentin Ghita <valentinghita@google.com> > > > > Add option to allow for connecting device GPIOs. This is useful when > > adding a peripheral device from the command line which uses an > > interrupt. > > > > It takes the following options: > > > > * in-dev-path, out-dev-path - required, the device canonical object > > path (e.g. /machine/peripheral-anon/device[0], > > /machine/iotkit/cluster0/armv7m0) for the devices that should have > > their in <-> out GPIOs connected > > > > * in-gpio-name, out-gpio-name - optional, the name of the GPIO list; > > if not specified, the unnamed GPIO list is used > > > > * in-gpio-index, out-gpio-index - optional, the index in the GPIO list > > that identifies the GPIO to be used; if not specified 0 (the first > > GPIO in the list) is used > > > > Usage example: > > > > # add the tmp105 sensor and connects its irq line to the CPU > > qemu-system-arm \ > > --machine mps2-an505 \ > > --device tmp105,bus=/versatile_i2c/i2c,address=0x50 \ > > --connect-gpios out-dev-path=/machine/peripheral-anon/device[0],\ > > in-dev-path=/machine/iotkit/cluster0/armv7m0,in-gpio-index=100 > > > This seems to be moving down the path towards "create and > wire up machines on the command line". We shouldn't > be doing that ad-hoc with one small commandline option > at a time, we should be doing it with a coherent plan. Yeah, as a general rule, adding new CLI args is pretty undesirable. To avoid that we could utilize QOM for representing data: eg a "gpio-connection" object: --object gpio-connection,out-dev-path=/machine/peripheral-anon/device[0],\ in-dev-path=/machine/iotkit/cluster0/armv7m0,in-gpio-index=100 this would be OK if we're fine with some code somewhere that iterates over the 'gpio-connection' object instances performing the wiring. If we want this wiring done in the machine realization path though, we would want to properties directly assocaited with the devices, rather than via a side-loaded object. With regards, Daniel
On Fri, 18 Oct 2024 at 17:05, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Fri, Oct 18, 2024 at 04:38:24PM +0100, Peter Maydell wrote: > > On Thu, 10 Oct 2024 at 18:39, Octavian Purdila <tavip@google.com> wrote: > > > > > > From: Valentin Ghita <valentinghita@google.com> > > > > > > Add option to allow for connecting device GPIOs. This is useful when > > > adding a peripheral device from the command line which uses an > > > interrupt. > > > > > > It takes the following options: > > > > > > * in-dev-path, out-dev-path - required, the device canonical object > > > path (e.g. /machine/peripheral-anon/device[0], > > > /machine/iotkit/cluster0/armv7m0) for the devices that should have > > > their in <-> out GPIOs connected > > > > > > * in-gpio-name, out-gpio-name - optional, the name of the GPIO list; > > > if not specified, the unnamed GPIO list is used > > > > > > * in-gpio-index, out-gpio-index - optional, the index in the GPIO list > > > that identifies the GPIO to be used; if not specified 0 (the first > > > GPIO in the list) is used > > > > > > Usage example: > > > > > > # add the tmp105 sensor and connects its irq line to the CPU > > > qemu-system-arm \ > > > --machine mps2-an505 \ > > > --device tmp105,bus=/versatile_i2c/i2c,address=0x50 \ > > > --connect-gpios out-dev-path=/machine/peripheral-anon/device[0],\ > > > in-dev-path=/machine/iotkit/cluster0/armv7m0,in-gpio-index=100 > > > > > > This seems to be moving down the path towards "create and > > wire up machines on the command line". We shouldn't > > be doing that ad-hoc with one small commandline option > > at a time, we should be doing it with a coherent plan. > > Yeah, as a general rule, adding new CLI args is pretty undesirable. > To avoid that we could utilize QOM for representing data: > eg a "gpio-connection" object: > > --object gpio-connection,out-dev-path=/machine/peripheral-anon/device[0],\ > in-dev-path=/machine/iotkit/cluster0/armv7m0,in-gpio-index=100 > > this would be OK if we're fine with some code somewhere that iterates > over the 'gpio-connection' object instances performing the wiring. Maybe, but as I say we would want an overall design for this, not a single "well this happens to let this one user do this one specific thing" approach. At the moment our design is "QEMU command line options are for doing the equivalent of plugging in PCI cards into slots, not for the equivalent of soldering chips onto boards". And in that view of the world "link this gpio line from the tmp105 up to the armv7m interrupt controller" is not something to do on the command line, because it's definitely soldering wires. So for current QEMU the answer is "if the AN505 has a tmp105 sensor, we should be creating it in the C code for the machine, and if it doesn't then we don't support creating it on the command line". thanks -- PMM
On Fri, Oct 18, 2024 at 9:09 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 18 Oct 2024 at 17:05, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Fri, Oct 18, 2024 at 04:38:24PM +0100, Peter Maydell wrote: > > > On Thu, 10 Oct 2024 at 18:39, Octavian Purdila <tavip@google.com> wrote: > > > > > > > > From: Valentin Ghita <valentinghita@google.com> > > > > > > > > Add option to allow for connecting device GPIOs. This is useful when > > > > adding a peripheral device from the command line which uses an > > > > interrupt. > > > > > > > > It takes the following options: > > > > > > > > * in-dev-path, out-dev-path - required, the device canonical object > > > > path (e.g. /machine/peripheral-anon/device[0], > > > > /machine/iotkit/cluster0/armv7m0) for the devices that should have > > > > their in <-> out GPIOs connected > > > > > > > > * in-gpio-name, out-gpio-name - optional, the name of the GPIO list; > > > > if not specified, the unnamed GPIO list is used > > > > > > > > * in-gpio-index, out-gpio-index - optional, the index in the GPIO list > > > > that identifies the GPIO to be used; if not specified 0 (the first > > > > GPIO in the list) is used > > > > > > > > Usage example: > > > > > > > > # add the tmp105 sensor and connects its irq line to the CPU > > > > qemu-system-arm \ > > > > --machine mps2-an505 \ > > > > --device tmp105,bus=/versatile_i2c/i2c,address=0x50 \ > > > > --connect-gpios out-dev-path=/machine/peripheral-anon/device[0],\ > > > > in-dev-path=/machine/iotkit/cluster0/armv7m0,in-gpio-index=100 > > > > > > > > > This seems to be moving down the path towards "create and > > > wire up machines on the command line". We shouldn't > > > be doing that ad-hoc with one small commandline option > > > at a time, we should be doing it with a coherent plan. > > > > Yeah, as a general rule, adding new CLI args is pretty undesirable. > > To avoid that we could utilize QOM for representing data: > > eg a "gpio-connection" object: > > > > --object gpio-connection,out-dev-path=/machine/peripheral-anon/device[0],\ > > in-dev-path=/machine/iotkit/cluster0/armv7m0,in-gpio-index=100 > > > > this would be OK if we're fine with some code somewhere that iterates > > over the 'gpio-connection' object instances performing the wiring. > > Maybe, but as I say we would want an overall design for this, > not a single "well this happens to let this one user do this > one specific thing" approach. > > At the moment our design is "QEMU command line options are for > doing the equivalent of plugging in PCI cards into slots, not > for the equivalent of soldering chips onto boards". And in > that view of the world "link this gpio line from the tmp105 > up to the armv7m interrupt controller" is not something to do on > the command line, because it's definitely soldering wires. > So for current QEMU the answer is "if the AN505 has a tmp105 > sensor, we should be creating it in the C code for the machine, > and if it doesn't then we don't support creating it on the > command line". > I should have used a better example, like: qemu-system-arm \ --machine rt595-evk \ --device tmp105,bus=/flexcomm6-i2c,address=0x50 \ --connect-gpios in-dev-path=/machine/soc/gpio,in-gpio-index=22,\ out-dev-path=/machine/peripheral-anon/device[0] This enables use-cases like plugging in daughter cards on a board that has I2C/SPI/GPIO pins exposed on a header. IMO this is very similar to plugging in PCI cards into slots.
On Fri, 18 Oct 2024 at 19:50, Octavian Purdila <tavip@google.com> wrote: > > On Fri, Oct 18, 2024 at 9:09 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > At the moment our design is "QEMU command line options are for > > doing the equivalent of plugging in PCI cards into slots, not > > for the equivalent of soldering chips onto boards". And in > > that view of the world "link this gpio line from the tmp105 > > up to the armv7m interrupt controller" is not something to do on > > the command line, because it's definitely soldering wires. > > So for current QEMU the answer is "if the AN505 has a tmp105 > > sensor, we should be creating it in the C code for the machine, > > and if it doesn't then we don't support creating it on the > > command line". > > > > I should have used a better example, like: > > qemu-system-arm \ > --machine rt595-evk \ > --device tmp105,bus=/flexcomm6-i2c,address=0x50 \ > --connect-gpios in-dev-path=/machine/soc/gpio,in-gpio-index=22,\ > out-dev-path=/machine/peripheral-anon/device[0] > > This enables use-cases like plugging in daughter cards on a board that > has I2C/SPI/GPIO pins exposed on a header. IMO this is very similar to > plugging in PCI cards into slots. So in the current design framework, the way you would model that kind of daughtercard is that you define a bus type corresponding to the daughtercard slot. For instance, for the Arduino Shield daughtercard spec supported by the mps3 boards, you would define the machine models to implement that bus (which wraps up i2c, gpio etc), and define some models of Shield daughtercards. Then users can use "-device tmp105-shield" on the command line and they don't need to know the internal details of which IRQ lines on the board are supposed to be used by the daughterboard, etc. This matches the real world hardware where the user plugs in a daughterboard rather than soldering wires directly between components. Directly exposing to users a "plug GPIO lines together" interface has some awkward sharp edges: * it is effectively making the QOM paths into user-facing APIs, which restricts our ability to refactor the implementation of the board without breaking existing user command lines. (In particular, anything using /machine/peripheral-anon/ is extremely vulnerable to changes in the board code.) * the user needs to know exactly how the board is wired up, including details like what GPIO inputs are free and which are used, and how each device orders its GPIO input lines. * at the moment there is no guard against trying to plug two GPIOs into a single output -- it just won't do the right thing. Currently we can say "that's a bug in QEMU C code if we do that", but a user-facing interface ought to be more robust. * how does this interact with hotpluggable devices? If you gpio-connect to a hotpluggable device and then hot-unplug it do we refuse the unplug, auto-disconnect the gpios, or just crash? As I say, the "I want to be able to plug devices together on the command line" usecase is real -- but I think that satisfying it is something we should do with a coherent approach to the whole problem, not piecemeal. thanks -- PMM
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h index 1d57bf6577..dc05b70146 100644 --- a/include/monitor/qdev.h +++ b/include/monitor/qdev.h @@ -6,6 +6,7 @@ void hmp_info_qtree(Monitor *mon, const QDict *qdict); void hmp_info_qdm(Monitor *mon, const QDict *qdict); void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp); +void qmp_connect_gpios(QDict *qdict, QObject **ret_data, Error **errp); int qdev_device_help(QemuOpts *opts); DeviceState *qdev_device_add(QemuOpts *opts, Error **errp); @@ -36,4 +37,6 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, */ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp); +int qdev_connect_gpios(QemuOpts *opts, Error **errp); + #endif diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 5b4397eeb8..66c5f5129e 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -106,6 +106,7 @@ extern QemuOptsList qemu_drive_opts; extern QemuOptsList bdrv_runtime_opts; extern QemuOptsList qemu_chardev_opts; extern QemuOptsList qemu_device_opts; +extern QemuOptsList qemu_connect_gpios_opts; extern QemuOptsList qemu_netdev_opts; extern QemuOptsList qemu_nic_opts; extern QemuOptsList qemu_net_opts; diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index f84a0dc523..3333598a5b 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -203,6 +203,9 @@ static void __attribute__((__constructor__)) monitor_init_qmp_commands(void) qmp_register_command(&qmp_commands, "device_add", qmp_device_add, 0, 0); + qmp_register_command(&qmp_commands, "connect_gpios", + qmp_connect_gpios, 0, 0); + QTAILQ_INIT(&qmp_cap_negotiation_commands); qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities", qmp_marshal_qmp_capabilities, diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index 44994ea0e1..16d417610a 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -735,6 +735,68 @@ err_del_dev: return NULL; } +static DeviceState *qdev_from_opt(QemuOpts *opts, const char *name, + Error **errp) +{ + Object *obj; + const char *path = qemu_opt_get(opts, name); + + if (!path) { + error_setg(errp, QERR_MISSING_PARAMETER, name); + return NULL; + } + + obj = object_resolve_path(path, NULL); + if (!obj) { + error_setg(errp, "Could not resolve path: %s", path); + return NULL; + } + + if (!object_dynamic_cast(obj, TYPE_DEVICE)) { + error_setg(errp, "%s is not a device", path); + return NULL; + } + + return DEVICE(obj); +} + +int qdev_connect_gpios(QemuOpts *opts, Error **errp) +{ + qemu_irq in_irq; + const char *in_gpio_name, *out_gpio_name; + int in_gpio_idx = 0, out_gpio_idx = 0; + DeviceState *dev; + + dev = qdev_from_opt(opts, "in-dev-path", errp); + if (!dev) { + return -1; + } + + in_gpio_name = qemu_opt_get(opts, "in-gpio-name"); + out_gpio_name = qemu_opt_get(opts, "out-gpio-name"); + in_gpio_idx = qemu_opt_get_number(opts, "out-gpio-index", 0); + out_gpio_idx = qemu_opt_get_number(opts, "out-gpio-index", 0); + + if (in_gpio_name) { + in_irq = qdev_get_gpio_in_named(dev, in_gpio_name, in_gpio_idx); + } else { + in_irq = qdev_get_gpio_in(dev, in_gpio_idx); + } + + dev = qdev_from_opt(opts, "out-dev-path", errp); + if (!dev) { + return -1; + } + + if (out_gpio_name) { + qdev_connect_gpio_out_named(dev, out_gpio_name, out_gpio_idx, in_irq); + } else { + qdev_connect_gpio_out(dev, out_gpio_idx, in_irq); + } + + return 0; +} + /* Takes ownership of @opts on success */ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) { @@ -885,6 +947,20 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) object_unref(OBJECT(dev)); } +void qmp_connect_gpios(QDict *qdict, QObject **ret_data, Error **errp) +{ + QemuOpts *opts; + + opts = qemu_opts_from_qdict(qemu_find_opts("connect-gpios"), qdict, errp); + if (!opts) { + return; + } + + qdev_connect_gpios(opts, errp); + + qemu_opts_del(opts); +} + static DeviceState *find_device_state(const char *id, Error **errp) { Object *obj = object_resolve_path_at(qdev_get_peripheral(), id); @@ -1102,6 +1178,33 @@ QemuOptsList qemu_device_opts = { }, }; +QemuOptsList qemu_connect_gpios_opts = { + .name = "connect-gpios", + .head = QTAILQ_HEAD_INITIALIZER(qemu_connect_gpios_opts.head), + .desc = { + { + .name = "in-dev-path", + .type = QEMU_OPT_STRING, + },{ + .name = "in-gpio-name", + .type = QEMU_OPT_STRING, + },{ + .name = "in-gpio-index", + .type = QEMU_OPT_NUMBER, + },{ + .name = "out-dev-path", + .type = QEMU_OPT_STRING, + },{ + .name = "out-gpio-name", + .type = QEMU_OPT_STRING, + },{ + .name = "out-gpio-index", + .type = QEMU_OPT_NUMBER, + }, + { /* end of list */ } + }, +}; + QemuOptsList qemu_global_opts = { .name = "global", .head = QTAILQ_HEAD_INITIALIZER(qemu_global_opts.head), diff --git a/system/vl.c b/system/vl.c index fe547ca47c..c36562fbeb 100644 --- a/system/vl.c +++ b/system/vl.c @@ -1222,6 +1222,19 @@ static int device_init_func(void *opaque, QemuOpts *opts, Error **errp) return 0; } +static int connect_gpios_func(void *opaque, QemuOpts *opts, Error **errp) +{ + int err; + + err = qdev_connect_gpios(opts, errp); + if (err != 0 && *errp) { + error_report_err(*errp); + return err; + } + + return 0; +} + static int chardev_init_func(void *opaque, QemuOpts *opts, Error **errp) { Error *local_err = NULL; @@ -2665,6 +2678,10 @@ static void qemu_create_cli_devices(void) object_unref(OBJECT(dev)); loc_pop(&opt->loc); } + + qemu_opts_foreach(qemu_find_opts("connect-gpios"), + connect_gpios_func, NULL, &error_fatal); + rom_reset_order_override(); } @@ -2765,6 +2782,7 @@ void qemu_init(int argc, char **argv) qemu_add_drive_opts(&bdrv_runtime_opts); qemu_add_opts(&qemu_chardev_opts); qemu_add_opts(&qemu_device_opts); + qemu_add_opts(&qemu_connect_gpios_opts); qemu_add_opts(&qemu_netdev_opts); qemu_add_opts(&qemu_nic_opts); qemu_add_opts(&qemu_net_opts); @@ -3373,6 +3391,12 @@ void qemu_init(int argc, char **argv) } } break; + case QEMU_OPTION_connect_gpios: + if (!qemu_opts_parse_noisily(qemu_find_opts("connect-gpios"), + optarg, false)) { + exit(1); + } + break; case QEMU_OPTION_smp: machine_parse_property_opt(qemu_find_opts("smp-opts"), "smp", optarg); diff --git a/qemu-options.hx b/qemu-options.hx index 20a1ce0d43..011aa66569 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1187,6 +1187,37 @@ SRST ERST +DEF("connect-gpios", HAS_ARG, QEMU_OPTION_connect_gpios, + "-connect-gpios in-dev-path=path,out-dev-path=path[,in-gpio-name=name][,out-gpio-name=name][,in-gpio-index=index][,out-gpio-index=index]\n" + " connect an input and an output gpio.\n", + QEMU_ARCH_ALL) +SRST +``-connect-gpios in-dev-path=path,out-dev-path=path[,in-gpio-name=name][,out-gpio-name=name][,in-gpio-index=index][,out-gpio-index=index]`` + Connect an input and an output gpio. + + ``in-dev-path``, ``out-dev-path`` required, the device canonical + object path (e.g. /machine/peripheral-anon/device[0], + /machine/iotkit/cluster0/armv7m0) for the devices that should + have their in <-> out GPIOs connected + + ``in-gpio-name``, ``out-gpio-name`` optional, the name of the GPIO list; + if not specified, the unnamed GPIO list is used + + ``in-gpio-index``, ``out-gpio-index`` optional, the index in the GPIO list + that identifies the GPIO to be used; if not specified 0 (the first + GPIO in the list) is used + + Examples: + + .. parsed-literal:: + + # add the tmp105 sensor and connects its irq line to the CPU + qemu-system-arm \\ + --machine mps2-an505 \\ + --device tmp105,bus=/versatile_i2c/i2c,address=0x50 \\ + --connect-gpios out-dev-path=/machine/peripheral-anon/device[0],in-dev-path=/machine/iotkit/cluster0/armv7m0,in-gpio-index=100 +ERST + DEF("name", HAS_ARG, QEMU_OPTION_name, "-name string1[,process=string2][,debug-threads=on|off]\n" " set the name of the guest\n"