Re: [Qemu-devel] [PATCH v3 4/5] xlnx-zynqmp: Connect the SPI devices

[Alistair Francis]

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?

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
>>>
>>>
>
>