Re: [Qemu-devel] [PATCH v2 5/5] xlnx-ep108: Connect the SPI Flash

[Peter Crosthwaite]

On Thu, Oct 8, 2015 at 4:30 PM, Alistair Francis
<address@hidden> wrote:
> On Wed, Oct 7, 2015 at 5:01 PM, Peter Crosthwaite
> <address@hidden> wrote:
>> On Wed, Oct 7, 2015 at 2:34 PM, Alistair Francis
>> <address@hidden> wrote:
>>> Connect the sst25wf080 SPI flash to the EP108 board.
>>>
>>> Signed-off-by: Alistair Francis <address@hidden>
>>> ---
>>> V2:
>>>  - Use sst25wf080 instead of m25p80
>>>
>>>  hw/arm/xlnx-ep108.c | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
>>> index 2899698..9755c30 100644
>>> --- a/hw/arm/xlnx-ep108.c
>>> +++ b/hw/arm/xlnx-ep108.c
>>> @@ -33,6 +33,7 @@ static struct arm_boot_info xlnx_ep108_binfo;
>>>  static void xlnx_ep108_init(MachineState *machine)
>>>  {
>>>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
>>> +    int i, j;
>>>      Error *err = NULL;
>>>
>>>      object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
>>> @@ -60,6 +61,25 @@ static void xlnx_ep108_init(MachineState *machine)
>>>                                           machine->ram_size);
>>>      memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
>>>
>>> +    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.spi[i]),
>>> +                                               bus_name);
>>
>> So machine level code shouldn't reach into the SoC like this. Instead
>> the bus should be passed from the SPI controller to SoC itself, then
>> use qdev_get_child_bus on the SoC itself.
>
> I'm not sure what you mean. The board needs multiple buses and I can't
> see how it can do that without reaching into the SoC.
>

The bus child object "spi0" should be a bus of the SoC, so the
instantiator of the SoC (ep108) does not need to reach into the SoC
implementation. You should end up with

        spi_bus = (SSIBus *)qdev_get_child_bus(DEVICE(&s->soc), "spi0");

The bus should be exported by the SoC as a bus of its own to avoid
exposing internals. Not sure if we need some new QOM infrastructure to
support this yet. Wondering if you just QOM detach the child bus from
the SPI controller and reparent to the SoC. That however is non-ideal
as we ultimately want a mechanism whereby both a SoC and a board can
attach to a bus (mix of internal and external connections).

So I think the SPI bus as a child of the SPI controller needs to be
aliased to the SoC (might be similar to that prop aliasing stuff for
processors to MPCore). qdev_get_child_bus might need a patch to scan
for such aliases.

Alternatively, you could do the alias and then ditch
qdev_get_child_bus and use QOM APIs instead.

Regards,
Peter

> Thanks,
>
> Alistair
>
>>
>> Regards,
>> Peter
>>
>>> +
>>> +        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,
>>> +                               cs_line);
>>> +        }
>>> +    }
>>> +
>>>      xlnx_ep108_binfo.ram_size = machine->ram_size;
>>>      xlnx_ep108_binfo.kernel_filename = machine->kernel_filename;
>>>      xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
>>> --
>>> 2.1.4
>>>
>>