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

[Alistair Francis]

On Fri, Oct 30, 2015 at 3:21 AM, Alistair Francis
<address@hidden> wrote:
> On Thu, Oct 29, 2015 at 12:04 PM, Peter Crosthwaite
> <address@hidden> wrote:
>> 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.
>>>
>>
>> I dont thinks this would have functioned though, as it would be 1st
>> bus of 1st controller and 2nd bus of 2nd controller.
>>
>>> 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.
>>>
>>
>> Can we add this renaming capability? I think it is the right solution.
>
> Renaming the bus is pretty easy. There is still a qtree problem though.
>
> For some reason SPI0 gets attached to the second SPI controller and I
> can't figure it out.

Ok, I think I have it figured out. Sending a new patch now.

Thanks,

Alistair

>
> 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: spi1
>       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
>
>>
>> Regards,
>> Peter
>>
>>> 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
>>>>>>
>>>>>>
>>>>
>>>>
>>