Re: [Qemu-devel] [PATCH V4 0/5] Ehnahced SSI bus support + M25P80 SPI flash + Xilinx SPI controller

[Peter Crosthwaite]

Hi Paul,

Responses inline below.

On Tue, Jun 5, 2012 at 11:34 AM, Paul Brook <address@hidden> wrote:
>> Patch 1 Enhances SSI bus support to properly support multiple attached
>> devices. An api is provided for SSI/SPI masters to select a particular
>> device attached to the bus.
>>
>> Patch 2 is a device model for the m25p80 style SPI flash chip.
>>
>> Patch 3 is  the Xilinx XPS SPI contoller. Its a sysbus device that
>> instantiates a ssi bus, and interfaces the two (as per the controllers
>> functionality)
>>
>> Patch 4 instantiates the XPS SPI controller in the petalogix ML605
>> reference platform and connects two m25p80s to it.
>>
>> Patch 5 updates the stellaris machine model to use the multi slave SSI
>> support
>
> I'm still not convinced modelling this as a multipoint bus is a good idea.  If
> nothing else you've failed to model the case where multiple slaves are
> selected simultanously.

The bus can easily be changed such that multiple devices are
selectable at once to get your desired multi device behaviour. AFAICT
though nothing in QEMU behaves like this ATM.

  Given the chip selects are actual wires, not part of
> the bus itself, I think multiple point-point busses are a better fit.
>
> For the stellaris device we still have the synthetic mux device and
> intermediate bus.
>

Yes, because in your stellaris architecture, the SSI controller
(pl022) is point to point so that exactly matches the hardware.

In the microblaze controller in this series, the controller has
inbuilt muxing with one-hot CS behavior. To implement with point to
point, I would have to dynamically create a number of sub-busses
(driven by a qdev property). I would also have to have a device within
a device to model the internal mux which increases my code volume
significantly. Also you end up with this little piece of ugliness in
your machine model and device model:

diff --git a/hw/petalogix_ml605_mmu.c b/hw/petalogix_ml605_mmu.c
index 01af0da..394a80e 100644
--- a/hw/petalogix_ml605_mmu.c
+++ b/hw/petalogix_ml605_mmu.c
@@ -145,10 +145,11 @@ petalogix_ml605_init(ram_addr_t ram_size,
         sysbus_mmio_map(busdev, 0, 0x40a00000);
         sysbus_connect_irq(busdev, 0, irq[4]);

-        spi = qdev_get_child_bus(dev, "spi");

         for (i = 0; i < NUM_SPI_FLASHES; i++) {
-            dev = ssi_create_slave_no_init(spi, "m25p80", i);
+            sprintf(sub_bus_name, "spi%d", i);
+            spi = qdev_get_child_bus(dev, sub_bus_name);
+            dev = ssi_create_slave_no_init(spi, "m25p80");
             qdev_prop_set_string(dev, "partname", (char *)"s25fl064k");
             qdev_init_nofail(dev);
         }
diff --git a/hw/xilinx_spi.c b/hw/xilinx_spi.c
index cae88ad..6c81f70 100644
--- a/hw/xilinx_spi.c
+++ b/hw/xilinx_spi.c
@@ -420,8 +420,11 @@ static int xilinx_spi_init(SysBusDevice *dev)
     s->irqline = -1;
     ptimer_set_freq(s->ptimer, 10 * 1000 * 1000);

-    s->spi = ssi_create_bus(&dev->qdev, "spi");
-
+    char sub_bus_name;
+    for (i = 0; i < num_cs; i++) {
+        sprintf(sub_bus_name, "spi%d", i);
+        s->spi[i] = ssi_create_bus(&dev->qdev, sub_bus_name);
+    }
     return 0;
 }

The multi-slave bus is a direct superset on point-to-point. There is
nothing stopping anyone from using it as p2p. Its just things are very
ugly for SPI controllers with integrated muxes to treat everything as
point to point.

Regards,
Peter
>
> Paul