diff mbox

[V4,0/5] Ehnahced SSI bus support + M25P80 SPI flash + Xilinx SPI controller

Message ID CAEgOgz6O4cwKuL+jqBLfPKvCiADWiELzCiyLPprbiWUvCs3+Hw@mail.gmail.com
State New
Headers show

Commit Message

Peter A. G. Crosthwaite June 5, 2012, 1:54 a.m. UTC 
Hi Paul,

Responses inline below.

On Tue, Jun 5, 2012 at 11:34 AM, Paul Brook <paul@codesourcery.com> 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:

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

Comments

Paul Brook June 6, 2012, 1:37 p.m. UTC | #1 
> > 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.

By my reading your xilinx device *should* behave like this.
 
> >  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:

I don't see why would would need a separate mux device.

One of my issues is that you've made this a device property.  A SPI device has 
no concept of address.  This really is a property of the controller.

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

IMHO the resulting tree device is better with multiple point-point links.  I'm 
hoping the hardcoded board descriptions (i.e. everything using 
ssi_create_slave) will go away sooner rather than later.  Having two m25p80 
devices that are indistinguishable apart from one minor property seems 
undesirable.

Paul
diff mbox

Patch

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