| Message ID | 0209faa3788f57deeb3ce4a197019b095bc2c05f.1450300479.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 16/12/2015 22:45, Alistair Francis wrote: > + > + /* Rename each SPI bus after the SPI device to allow the board > + * to access all of the busses from the SoC. > + */ > + spi_bus = qdev_get_child_bus(DEVICE(&s->spi[i]), "spi0"); > + snprintf(bus_name, 6, "spi%d", i); > + qdev_bus_rename(spi_bus, bus_name); > + > + /* Add the SPI buses to the SoC child bus */ > + /* FIXME: This causes the later buses to be duplicated in > + * the SPI devices printout when running qtre. > + */ > + QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); Isn't the SPI bus accessible with something similar to spi[0-5]/spi0, even without this hack? In any case, I would prefer qdev_bus_rename to stay in xlnx-zynqmp.c. Paolo
On Wed, Dec 16, 2015 at 3:24 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 16/12/2015 22:45, Alistair Francis wrote: >> + >> + /* Rename each SPI bus after the SPI device to allow the board >> + * to access all of the busses from the SoC. >> + */ >> + spi_bus = qdev_get_child_bus(DEVICE(&s->spi[i]), "spi0"); >> + snprintf(bus_name, 6, "spi%d", i); >> + qdev_bus_rename(spi_bus, bus_name); >> + >> + /* Add the SPI buses to the SoC child bus */ >> + /* FIXME: This causes the later buses to be duplicated in >> + * the SPI devices printout when running qtre. >> + */ >> + QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); > > Isn't the SPI bus accessible with something similar to spi[0-5]/spi0, > even without this hack? Not that I know of. That doesn't work for me. > > In any case, I would prefer qdev_bus_rename to stay in xlnx-zynqmp.c. That's fine with me. Thanks, Alistair > > Paolo >
On 16 December 2015 at 23:24, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 16/12/2015 22:45, Alistair Francis wrote: >> + >> + /* Rename each SPI bus after the SPI device to allow the board >> + * to access all of the busses from the SoC. >> + */ >> + spi_bus = qdev_get_child_bus(DEVICE(&s->spi[i]), "spi0"); >> + snprintf(bus_name, 6, "spi%d", i); >> + qdev_bus_rename(spi_bus, bus_name); >> + >> + /* Add the SPI buses to the SoC child bus */ >> + /* FIXME: This causes the later buses to be duplicated in >> + * the SPI devices printout when running qtre. >> + */ >> + QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); > > Isn't the SPI bus accessible with something similar to spi[0-5]/spi0, > even without this hack? > > In any case, I would prefer qdev_bus_rename to stay in xlnx-zynqmp.c. I disagree that it should be in xlnx-zynqmp.c. Either (a) qdev already provides some reasonable mechanism for SoC container like this to allow their users to get at buses provided by their child objects, in which case we should use it (b) qdev doesn't provide such a mechanism, in which case we need to provide one (either qdev_bus_rename or something else if you have a better idea) But we definitely shouldn't have the SoC container code messing around with the internals of the qdev objects. thanks -- PMM
On 17/12/2015 09:26, Peter Maydell wrote: >> > In any case, I would prefer qdev_bus_rename to stay in xlnx-zynqmp.c. > I disagree that it should be in xlnx-zynqmp.c. Either > (a) qdev already provides some reasonable mechanism for > SoC container like this to allow their users to get at > buses provided by their child objects, in which case we > should use it > (b) qdev doesn't provide such a mechanism, in which case > we need to provide one (either qdev_bus_rename or something > else if you have a better idea) > > But we definitely shouldn't have the SoC container code > messing around with the internals of the qdev objects. It's a hack and I don't want it to become a sanctioned way to do it. It's already messing around pretty heavily with qdev internals, see the line right after QLIST_INSERT_HEAD: QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); Paolo
On 17 December 2015 at 10:28, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 17/12/2015 09:26, Peter Maydell wrote: >>> > In any case, I would prefer qdev_bus_rename to stay in xlnx-zynqmp.c. >> I disagree that it should be in xlnx-zynqmp.c. Either >> (a) qdev already provides some reasonable mechanism for >> SoC container like this to allow their users to get at >> buses provided by their child objects, in which case we >> should use it >> (b) qdev doesn't provide such a mechanism, in which case >> we need to provide one (either qdev_bus_rename or something >> else if you have a better idea) >> >> But we definitely shouldn't have the SoC container code >> messing around with the internals of the qdev objects. > > It's a hack and I don't want it to become a sanctioned way to do it. > It's already messing around pretty heavily with qdev internals, see the > line right after QLIST_INSERT_HEAD: > > QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); Well, that doesn't look good either. I think my point still stands -- we should be providing proper infrastructure at the qdev level to allow SoC container devices to do the things they need to do, not just letting the containers mess with the qdev internals. thanks -- PMM
On 17/12/2015 12:11, Peter Maydell wrote: > > It's a hack and I don't want it to become a sanctioned way to do it. > > It's already messing around pretty heavily with qdev internals, see the > > line right after QLIST_INSERT_HEAD: > > > > QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); > > Well, that doesn't look good either. I think my point still > stands -- we should be providing proper infrastructure at > the qdev level to allow SoC container devices to do the > things they need to do, not just letting the containers > mess with the qdev internals. I agree completely. Paolo
On Thu, Dec 17, 2015 at 3:12 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 17/12/2015 12:11, Peter Maydell wrote: >> > It's a hack and I don't want it to become a sanctioned way to do it. >> > It's already messing around pretty heavily with qdev internals, see the >> > line right after QLIST_INSERT_HEAD: >> > >> > QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); >> >> Well, that doesn't look good either. I think my point still >> stands -- we should be providing proper infrastructure at >> the qdev level to allow SoC container devices to do the >> things they need to do, not just letting the containers >> mess with the qdev internals. > > I agree completely. Does anyone have any ideas on how we can do this? AFAIK there is no way to currently do this, so we need to add something. What is the preferred way to expose the buses? Thanks, Alistair > > Paolo >
On 18/12/2015 18:17, Alistair Francis wrote: > Does anyone have any ideas on how we can do this? > > AFAIK there is no way to currently do this, so we need to add > something. What is the preferred way to expose the buses? For now, what you're doing is okay for me, just moving the funky code in zynq-specific files. Thanks, Paolo > Thanks, > > Alistair
On Fri, Dec 18, 2015 at 9:55 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 18/12/2015 18:17, Alistair Francis wrote: >> Does anyone have any ideas on how we can do this? >> >> AFAIK there is no way to currently do this, so we need to add >> something. What is the preferred way to expose the buses? > > For now, what you're doing is okay for me, just moving the funky code in > zynq-specific files. > OK I think I have a real fix on this that doesn't require any funk. Backing up, qdev currently doubly connects buses and devices and devices to devs. The two connections are: 1: The child bus list 2: As QOM children Ultimately what we need here is an aliasing mechanism. I don't thing detaching and re-attaching works in the local sense, as it is realistic for a container device to make a few local connections to a bus (by ref to the controller bus) while also pinning it out on the container level. The bus should remain accessible on the two different entities. So QOM aliases make the most sense to me. The real problem is that qdev_get_child_bus only uses the child bus list and cannot resolve a QOM path. My solution is to add preferred attempt to use a QOM path to implement qdev_get_child_bus and fallback to the current child-bus-list behaviour on failure. I'll send a v5 within the next few hours after cleanup. Regards, Peter > Thanks, > > Paolo > >> Thanks, >> >> Alistair
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c index 87553bb..bce935d 100644 --- a/hw/arm/xlnx-zynqmp.c +++ b/hw/arm/xlnx-zynqmp.c @@ -56,6 +56,14 @@ static const int sdhci_intr[XLNX_ZYNQMP_NUM_SDHCI] = { 48, 49, }; +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; @@ -112,6 +120,12 @@ static void xlnx_zynqmp_init(Object *obj) qdev_set_parent_bus(DEVICE(&s->sdhci[i]), 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) @@ -286,6 +300,30 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci[i]), 0, gic_spi[sdhci_intr[i]]); } + + 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); + + 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]]); + + /* Rename each SPI bus after the SPI device to allow the board + * to access all of the busses from the SoC. + */ + spi_bus = qdev_get_child_bus(DEVICE(&s->spi[i]), "spi0"); + snprintf(bus_name, 6, "spi%d", i); + qdev_bus_rename(spi_bus, bus_name); + + /* Add the SPI buses to the SoC child bus */ + /* FIXME: This causes the later buses to be duplicated in + * the SPI devices printout when running qtre. + */ + QLIST_INSERT_HEAD(&dev->child_bus, spi_bus, sibling); + } } static Property xlnx_zynqmp_props[] = { diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h index d116092..f598a43 100644 --- a/include/hw/arm/xlnx-zynqmp.h +++ b/include/hw/arm/xlnx-zynqmp.h @@ -25,6 +25,7 @@ #include "hw/ide/pci.h" #include "hw/ide/ahci.h" #include "hw/sd/sdhci.h" +#include "hw/ssi/xilinx_spips.h" #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp" #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \ @@ -35,6 +36,7 @@ #define XLNX_ZYNQMP_NUM_GEMS 4 #define XLNX_ZYNQMP_NUM_UARTS 2 #define XLNX_ZYNQMP_NUM_SDHCI 2 +#define XLNX_ZYNQMP_NUM_SPIS 2 #define XLNX_ZYNQMP_NUM_OCM_BANKS 4 #define XLNX_ZYNQMP_OCM_RAM_0_ADDRESS 0xFFFC0000 @@ -66,6 +68,7 @@ typedef struct XlnxZynqMPState { CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS]; SysbusAHCIState sata; SDHCIState sdhci[XLNX_ZYNQMP_NUM_SDHCI]; + XilinxSPIPS spi[XLNX_ZYNQMP_NUM_SPIS]; char *boot_cpu; ARMCPU *boot_cpu_ptr;
Connect the Xilinx SPI devices to the ZynqMP model. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- V5: - Use the bus renaming function V4: - Rename the SPI busses so that they can all be accessed from the SoC - Don't set the num-busses property 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 | 38 ++++++++++++++++++++++++++++++++++++++ include/hw/arm/xlnx-zynqmp.h | 3 +++ 2 files changed, 41 insertions(+)