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

[Alistair Francis]

On Wed, Oct 14, 2015 at 12:25 PM, Peter Crosthwaite
<address@hidden> wrote:
> 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.

Ok, this aliasing is tricky. the QOM infrastructure for aliases is
built on object propertys, which doesn't really help too much with
qdev buses/devices.

I was looking at adding a qdev_get_child_bus() similar function which
can find the devices children's buses as well, but that seems
difficult as well. Though it might be the best option.

I'm just not sure how yet... I'll look into that tomorrow

Thanks,

Alistair

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