qemu-devel
[Top][All Lists]
Advanced

[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 14:51:51 -0700

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.

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]