From patchwork Thu Oct 29 18:16:25 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alistair Francis X-Patchwork-Id: 537971 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id B69C41401AD for ; Fri, 30 Oct 2015 05:17:27 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=Wmb7f5OQ; dkim-atps=neutral Received: from localhost ([::1]:45782 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZrrlN-00020V-Mt for incoming@patchwork.ozlabs.org; Thu, 29 Oct 2015 14:17:25 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33676) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zrrkv-0001OO-LT for qemu-devel@nongnu.org; Thu, 29 Oct 2015 14:16:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zrrkt-00043W-SC for qemu-devel@nongnu.org; Thu, 29 Oct 2015 14:16:57 -0400 Received: from mail-oi0-x22e.google.com ([2607:f8b0:4003:c06::22e]:33132) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zrrkt-00043P-Lc for qemu-devel@nongnu.org; Thu, 29 Oct 2015 14:16:55 -0400 Received: by oiad129 with SMTP id d129so45277568oia.0 for ; Thu, 29 Oct 2015 11:16:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; bh=rxyYuHp5cbOEAk2zCOWWChvIkpbTEJal+Y9dvX0Bi6I=; b=Wmb7f5OQ+6EOlBZ0g8ju8lpxjcH0PqEKCRQFyMlnBmkelGQaAD3fLr7ABs44eONMPD TSX4isBtFhYogVma2c+HYurXzEt030kC3mLoMe8iPWQKh97/OFLsl2qegW7d5+AFt4oM J3vmWA/hd/31nlVAQfibFJCvYN81f6ao4K1mm9f9VPugdBvDP21FxJ8P2CeGr1QHYqek pFPrxgxsq90IuMr1VFlYEBI/SKIfRqCfXSOl0Pngrf1wM77PQrnzfmMxkbKogYSaZ78B B+HyDFkfDSxEiEPMPu+atvPL+EQRliCdHLff7VjLibn+GVPjujT4ycax71QLgea8XKUi N03Q== X-Received: by 10.182.121.163 with SMTP id ll3mr2323169obb.27.1446142614886; Thu, 29 Oct 2015 11:16:54 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.49.228 with HTTP; Thu, 29 Oct 2015 11:16:25 -0700 (PDT) In-Reply-To: References: <5631D858.7060001@greensocs.com> From: Alistair Francis Date: Thu, 29 Oct 2015 11:16:25 -0700 X-Google-Sender-Auth: 1raopRqE88-GWB_RL5mMaRSMqds Message-ID: To: Alistair Francis X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2607:f8b0:4003:c06::22e Cc: Edgar Iglesias , Peter Maydell , "qemu-devel@nongnu.org Developers" , Peter Crosthwaite , "Edgar E. Iglesias" , Frederic Konrad Subject: Re: [Qemu-devel] [PATCH v3 4/5] xlnx-zynqmp: Connect the SPI devices X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Thu, Oct 29, 2015 at 10:45 AM, Alistair Francis wrote: > On Thu, Oct 29, 2015 at 1:27 AM, Frederic Konrad > wrote: >> On 29/10/2015 03:00, Peter Crosthwaite wrote: >>> On Wed, Oct 28, 2015 at 10:32 AM, Alistair Francis < >>> alistair.francis@xilinx.com> wrote: >>> >>>> Connect the Xilinx SPI device to the ZynqMP model. >>>> >>>> >>> "devices" >>> >>> >>>> Signed-off-by: Alistair Francis >>>> --- >>>> V3: >>>> - Expose the SPI Bus as part of the SoC device >>>> V2: >>>> - Don't connect the SPI flash to the SoC >>>> >>>> hw/arm/xlnx-zynqmp.c | 37 +++++++++++++++++++++++++++++++++++++ >>>> include/hw/arm/xlnx-zynqmp.h | 4 ++++ >>>> 2 files changed, 41 insertions(+) >>>> >>>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c >>>> index b36ca3d..5671d7a 100644 >>>> --- a/hw/arm/xlnx-zynqmp.c >>>> +++ b/hw/arm/xlnx-zynqmp.c >>>> @@ -48,6 +48,14 @@ static const int uart_intr[XLNX_ZYNQMP_NUM_UARTS] = { >>>> 21, 22, >>>> }; >>>> >>>> +static const uint64_t spi_addr[XLNX_ZYNQMP_NUM_SPIS] = { >>>> + 0xFF040000, 0xFF050000, >>>> +}; >>>> + >>>> +static const int spi_intr[XLNX_ZYNQMP_NUM_SPIS] = { >>>> + 19, 20, >>>> +}; >>>> + >>>> typedef struct XlnxZynqMPGICRegion { >>>> int region_index; >>>> uint32_t address; >>>> @@ -97,6 +105,12 @@ static void xlnx_zynqmp_init(Object *obj) >>>> >>>> object_initialize(&s->sata, sizeof(s->sata), TYPE_SYSBUS_AHCI); >>>> qdev_set_parent_bus(DEVICE(&s->sata), sysbus_get_default()); >>>> + >>>> + for (i = 0; i < XLNX_ZYNQMP_NUM_SPIS; i++) { >>>> + object_initialize(&s->spi[i], sizeof(s->spi[i]), >>>> + TYPE_XILINX_SPIPS); >>>> + qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default()); >>>> + } >>>> } >>>> >>>> static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) >>>> @@ -258,6 +272,29 @@ static void xlnx_zynqmp_realize(DeviceState *dev, >>>> Error **errp) >>>> >>>> sysbus_mmio_map(SYS_BUS_DEVICE(&s->sata), 0, SATA_ADDR); >>>> sysbus_connect_irq(SYS_BUS_DEVICE(&s->sata), 0, gic_spi[SATA_INTR]); >>>> + >>>> + for (i = 0; i < XLNX_ZYNQMP_NUM_SPIS; i++) { >>>> + BusState *spi_bus; >>>> + char bus_name[6]; >>>> + >>>> + object_property_set_int(OBJECT(&s->spi[i]), XLNX_ZYNQMP_NUM_SPIS, >>>> + "num-busses", &error_abort); >>>> >>> The number of busses-per-controller is unrelated to the number of >>> controllers. Setting num_busses != 1 is primarily a QSPI thing, so should >>> this just default to 1? I think you can drop this setter completely. > > True, but see below for a problem. > >>> >>> >>>> + object_property_set_bool(OBJECT(&s->spi[i]), true, "realized", >>>> &err); >>>> + if (err) { >>>> + error_propagate(errp, err); >>>> + return; >>>> + } >>>> + >>>> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 0, spi_addr[i]); >>>> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->spi[i]), 0, >>>> + gic_spi[spi_intr[i]]); >>>> + >>>> + snprintf(bus_name, 6, "spi%d", i); >>>> + spi_bus = qdev_get_child_bus(DEVICE(&s->spi), bus_name); >>>> + >>>> + /* Add the SPI buses to the SoC child bus */ >>>> + QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); >>>> >>> Nice! That is pretty simple in the end. One, question though, what happen >>> with info qtree? Do you get doubles because the bus is double parented? > > I don't see the double parent problem, but I do see another problem. > I was doing it a little wrong with the multiple buses. > > When I assign the SPI bus to the SoC, the more recent one replaces the > previous one. I didn't notice it before because I had two buses (which > meant they had different names) so it ended up working. > > Now with only one bus per I2C they both have the same name and conflict. > > I can't change the name of the bus either, so this is a bit of a problem. > > I can't see a way around this, while still assigning the buses to the > SoC. I guess the best option would be to not just take the first match > when calling qdev_get_child_bus(). Which would mean implementing that > function manually. How does that sound? Ok, it isn't actually too bad. This is the diff I have (it's still a little hacky): /*** Device API. ***/ ****************************** That results in this qtee for the spi devices. I'm not sure why one bus is there twice, but hopefully it is an easy fix. bus: main-system-bus type System dev: xlnx.ps7-spi, id "" gpio-out "sysbus-irq" 5 num-busses = 1 (0x1) num-ss-bits = 4 (0x4) num-txrx-bytes = 1 (0x1) mmio 00000000ff050000/0000000000000100 bus: spi0 type SSI dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 bus: spi0 type SSI dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 dev: xlnx.ps7-spi, id "" gpio-out "sysbus-irq" 5 num-busses = 1 (0x1) num-ss-bits = 4 (0x4) num-txrx-bytes = 1 (0x1) mmio 00000000ff040000/0000000000000100 bus: spi0 type SSI dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 dev: sst25wf080, id "" gpio-in "ssi-gpio-cs" 1 Thanks, Alistair > > Thanks, > > Alistair > >>> >>> I think this concept also might apply to the DP/DPDMA work, where the >>> display port (or AUX bus?) should be put on the SoC container. Then the >>> machine model (ep108) is responsible for detecting if the user wants a >>> display and connecting it. I.e. the DP controller shouldn't be doing the UI >>> init. >> >> You mean get the AUX and I2C bus here and connect the edid and the dpcd? >> I can take a look. >> >> Fred >>> >>>> + } >>>> } >>>> >>>> static Property xlnx_zynqmp_props[] = { >>>> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h >>>> index 4005a99..6d1d2a9 100644 >>>> --- a/include/hw/arm/xlnx-zynqmp.h >>>> +++ b/include/hw/arm/xlnx-zynqmp.h >>>> @@ -24,6 +24,7 @@ >>>> #include "hw/char/cadence_uart.h" >>>> #include "hw/ide/pci.h" >>>> #include "hw/ide/ahci.h" >>>> +#include "hw/ssi/xilinx_spips.h" >>>> >>>> #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp" >>>> #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \ >>>> @@ -33,6 +34,8 @@ >>>> #define XLNX_ZYNQMP_NUM_RPU_CPUS 2 >>>> #define XLNX_ZYNQMP_NUM_GEMS 4 >>>> #define XLNX_ZYNQMP_NUM_UARTS 2 >>>> +#define XLNX_ZYNQMP_NUM_SPIS 2 >>>> >>> >>>> +#define XLNX_ZYNQMP_NUM_SPI_FLASHES 4 >>>> >>> NUM_SPI_FLASHES is local to ep108 so it should just be in ep108.c >>> >>> Regards, >>> Peter >>> >>> >>>> #define XLNX_ZYNQMP_NUM_OCM_BANKS 4 >>>> #define XLNX_ZYNQMP_OCM_RAM_0_ADDRESS 0xFFFC0000 >>>> @@ -63,6 +66,7 @@ typedef struct XlnxZynqMPState { >>>> CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS]; >>>> CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS]; >>>> SysbusAHCIState sata; >>>> + XilinxSPIPS spi[XLNX_ZYNQMP_NUM_SPIS]; >>>> >>>> char *boot_cpu; >>>> ARMCPU *boot_cpu_ptr; >>>> -- >>>> 2.5.0 >>>> >>>> >> >> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c index 9ac6e6f..d59cec0 100644 --- a/hw/arm/xlnx-ep108.c +++ b/hw/arm/xlnx-ep108.c @@ -63,18 +63,17 @@ static void xlnx_ep108_init(MachineState *machine) for (i = 0; i < XLNX_ZYNQMP_NUM_SPIS; i++) { SSIBus *spi_bus; - char bus_name[6]; - snprintf(bus_name, 6, "spi%d", i); - spi_bus = (SSIBus *)qdev_get_child_bus(DEVICE(&s->soc), bus_name); + fprintf(stderr, "SPI device %d, bus 0\n", i); + + spi_bus = (SSIBus *)qdev_get_num_child_bus(DEVICE(&s->soc), "spi0", i); for (j = 0; j < XLNX_ZYNQMP_NUM_SPI_FLASHES; ++j) { DeviceState *flash_dev = ssi_create_slave(spi_bus, "sst25wf080"); qemu_irq cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0); - sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.spi[i]), - i * XLNX_ZYNQMP_NUM_SPI_FLASHES + j, + sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.spi[i]), j, cs_line); } } diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c index d2daf80..bc497fa 100644 --- a/hw/arm/xlnx-zynqmp.c +++ b/hw/arm/xlnx-zynqmp.c @@ -275,7 +275,6 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) for (i = 0; i < XLNX_ZYNQMP_NUM_SPIS; i++) { BusState *spi_bus; - char bus_name[6]; object_property_set_bool(OBJECT(&s->spi[i]), true, "realized", &err); if (err) { @@ -287,8 +286,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) sysbus_connect_irq(SYS_BUS_DEVICE(&s->spi[i]), 0, gic_spi[spi_intr[i]]); - snprintf(bus_name, 6, "spi%d", i); - spi_bus = qdev_get_child_bus(DEVICE(&s->spi), bus_name); + spi_bus = qdev_get_child_bus(DEVICE(&s->spi[i]), "spi0"); /* Add the SPI buses to the SoC child bus */ QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 4ab04aa..a831985 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -573,18 +573,27 @@ void qdev_pass_gpios(DeviceState *dev, DeviceState *container, QLIST_INSERT_HEAD(&container->gpios, ngl, node); } -BusState *qdev_get_child_bus(DeviceState *dev, const char *name) +BusState *qdev_get_num_child_bus(DeviceState *dev, const char *name, int num) { BusState *bus; QLIST_FOREACH(bus, &dev->child_bus, sibling) { if (strcmp(name, bus->name) == 0) { - return bus; + if (!num) { + return bus; + } else { + num--; + } } } return NULL; } +BusState *qdev_get_child_bus(DeviceState *dev, const char *name) +{ + return qdev_get_num_child_bus(dev, name, 0); +} + int qbus_walk_children(BusState *bus, qdev_walkerfn *pre_devfn, qbus_walkerfn *pre_busfn, qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn, diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 8057aed..2650e6f 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -295,6 +295,7 @@ qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char *name, int n); qemu_irq qdev_intercept_gpio_out(DeviceState *dev, qemu_irq icpt, const char *name, int n); +BusState *qdev_get_num_child_bus(DeviceState *dev, const char *name, int num); BusState *qdev_get_child_bus(DeviceState *dev, const char *name);