[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 4/5] xlnx-zynqmp: Connect the SPI devices
From: |
Alistair Francis |
Subject: |
Re: [Qemu-devel] [PATCH v3 4/5] xlnx-zynqmp: Connect the SPI devices |
Date: |
Thu, 29 Oct 2015 11:16:25 -0700 |
On Thu, Oct 29, 2015 at 10:45 AM, Alistair Francis
<address@hidden> wrote:
> On Thu, Oct 29, 2015 at 1:27 AM, Frederic Konrad
> <address@hidden> wrote:
>> On 29/10/2015 03:00, Peter Crosthwaite wrote:
>>> On Wed, Oct 28, 2015 at 10:32 AM, Alistair Francis <
>>> address@hidden> wrote:
>>>
>>>> Connect the Xilinx SPI device to the ZynqMP model.
>>>>
>>>>
>>> "devices"
>>>
>>>
>>>> Signed-off-by: Alistair Francis <address@hidden>
>>>> ---
>>>> 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):
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);
/*** 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
>>>>
>>>>
>>
>>
- [Qemu-devel] [PATCH v3 2/5] ssi: Move ssi.h into a separate directory, (continued)
[Qemu-devel] [PATCH v3 5/5] xlnx-ep108: Connect the SPI Flash, Alistair Francis, 2015/10/28