Re: [Qemu-devel] [PATCH v3 1/4] SSI: Built in multiple device support
On Wed, Apr 25, 2012 at 2:41 AM, Peter Maydell wrote:
> On 20 April 2012 03:12, Peter A. G. Crosthwaite
> wrote:
>> Added support for multiple devices attached to a single SSI bus (Previously
>> SSI masters with multiple slaves were emulated as multiple point to point SSI
>> busses)
>
>> static struct BusInfo ssi_bus_info = {
>> .name = "SSI",
>> .size = sizeof(SSIBus),
>> + .props = (Property[]) {
>> + DEFINE_PROP_INT32("ss", struct SSISlave, ss, 0),
>
> "ss" is a terrible name for a property. Can we have something
> a little less abbreviated, please?
>
>> SSIBus *ssi_create_bus(DeviceState *parent, const char *name)
>> {
>> - BusState *bus;
>> - bus = qbus_create(&ssi_bus_info, parent, name);
>> - return FROM_QBUS(SSIBus, bus);
>> + SSIBus *bus;
>> +
>> + bus = FROM_QBUS(SSIBus, qbus_create(&ssi_bus_info, parent, name));
>> + vmstate_register(NULL, -1, &vmstate_ssi_bus, bus);
>> + return bus;
>
> Stray double space.
>
ack
>> +void ssi_select_slave(SSIBus *bus, int32_t ss)
>> +{
>> + SSISlave *slave;
>> + SSISlaveClass *ssc;
>> +
>> + if (bus->ss == ss) {
>> + return;
>> + }
>> +
>> + slave = get_current_slave(bus);
>> + if (slave) {
>> + ssc = SSI_SLAVE_GET_CLASS(slave);
>> + if (ssc->set_cs) {
>> + ssc->set_cs(slave, 0);
>> + }
>> + }
>> + bus->ss = ss;
>
> Something wrong here. If bus->ss is a property you can't modify
> it randomly at runtime. (It won't get put back to the right
> value at reset, for instance.)
>
Hi Peter,
Thanks for your review.
I think there is confusion here in that I have named both the bus and
slave properties ss. Each slave has a index which ive called ss and
the bus has a property ss. The property defintion above applies the
slave device ss property which is constant. I think this will go away
when I clear up the name confusion as you have suggested.
Regards,
Peter
> -- PMM
Re: [Qemu-devel] [PATCH v3 1/4] SSI: Built in multiple device support
> Im happy to spend the 10 mins updating stellaris.c accordingly, but is > someone sitting on a binary package and brief instructions or some > such to regression test it? Do you of this machine have some sort of > kernel image handy? I've attached a tarball with some test binaries. They're built from the example libraries shipped with this board. The first exercises the display, amongst other things. The second exercises the SD card. Simple SD card image also included (remember to ungzip it first). "ls" and "cat readme.txt" over the serial port to make it do something verifiable. Run them with: ./qemu-system-arm -M lm3s6965evb -kernel qs_ek-lm3s6965.bin ./qemu-system-arm -M lm3s6965evb -serial stdio -kernel sd_card.bin -sd sdcard.img I don't have any software handy that exercises both simultaneously. It's probably worth mentioning that we don't currently implement the all CS lines accurately for this board. Most pins on this device are dual-function. They can be configured either as regular GPIO, or driven from a periperal (aka "alternate function", e.g. the SSI controller). Config is cone via the GPIO controllers. There are 7 GPIO contollers (A-G) with 8 pins each. On reset all pins are configured as floating GPIO, and we let D0 float high. The frame start/chip select line from the SPI controller goes via GPIO A3. This is connected to the display controller (ssd0323) CS pin. The SD card CS pin is connected to GPIO D0. When comminucating with the display controller the SSI pins will be configured normally. When communicating with the SD card we configure A3 as a GPIO pin, set high (inactive), and pull D0 low to select the SD card. The current implementation ignores the SSI select pin (A3), and assumes the display controller is selected whenever the SD card (D0) is not. We do not implement the alternate function select in the GPIO controller. It's a bit of a strange setup, but I guess probably not that unusual. Paul
Re: [Qemu-devel] [PATCH v3 1/4] SSI: Built in multiple device support
On 20 April 2012 03:12, Peter A. G. Crosthwaite
wrote:
> Added support for multiple devices attached to a single SSI bus (Previously
> SSI masters with multiple slaves were emulated as multiple point to point SSI
> busses)
> static struct BusInfo ssi_bus_info = {
> .name = "SSI",
> .size = sizeof(SSIBus),
> + .props = (Property[]) {
> + DEFINE_PROP_INT32("ss", struct SSISlave, ss, 0),
"ss" is a terrible name for a property. Can we have something
a little less abbreviated, please?
> SSIBus *ssi_create_bus(DeviceState *parent, const char *name)
> {
> - BusState *bus;
> - bus = qbus_create(&ssi_bus_info, parent, name);
> - return FROM_QBUS(SSIBus, bus);
> + SSIBus *bus;
> +
> + bus = FROM_QBUS(SSIBus, qbus_create(&ssi_bus_info, parent, name));
> + vmstate_register(NULL, -1, &vmstate_ssi_bus, bus);
> + return bus;
Stray double space.
> +void ssi_select_slave(SSIBus *bus, int32_t ss)
> +{
> + SSISlave *slave;
> + SSISlaveClass *ssc;
> +
> + if (bus->ss == ss) {
> + return;
> + }
> +
> + slave = get_current_slave(bus);
> + if (slave) {
> + ssc = SSI_SLAVE_GET_CLASS(slave);
> + if (ssc->set_cs) {
> + ssc->set_cs(slave, 0);
> + }
> + }
> + bus->ss = ss;
Something wrong here. If bus->ss is a property you can't modify
it randomly at runtime. (It won't get put back to the right
value at reset, for instance.)
-- PMM
Re: [Qemu-devel] [PATCH v3 1/4] SSI: Built in multiple device support
Hi Paul, Im happy to spend the 10 mins updating stellaris.c accordingly, but is someone sitting on a binary package and brief instructions or some such to regression test it? Do you of this machine have some sort of kernel image handy? Peter 2012/4/21 Paul Brook : >> /* QEMU Synchronous Serial Interface support. */ >> >> -/* In principle SSI is a point-point interface. As such the qemu >> - implementation has a single slave device on a "bus". >> +/* In principle SSI is a point-point interface. >> However it is fairly common for boards to have multiple slaves >> connected to a single master, and select devices with an external >> - chip select. This is implemented in qemu by having an explicit mux dev. >> + chip select. SSI busses can therefore have any number of slaves, >> + of which a master can select using ssi_select_slave(). >> It is assumed that master and slave are both using the same transfer > > This would be much more convincing if you'd also implemented it. In particular > the lm3s6965evb has two devices (LCD controller and SD card) connected to a > single SSI controller, and selects between the two by twiddling chip selects. > > Paul
Re: [Qemu-devel] [PATCH v3 1/4] SSI: Built in multiple device support
> /* QEMU Synchronous Serial Interface support. */ > > -/* In principle SSI is a point-point interface. As such the qemu > - implementation has a single slave device on a "bus". > +/* In principle SSI is a point-point interface. > However it is fairly common for boards to have multiple slaves > connected to a single master, and select devices with an external > - chip select. This is implemented in qemu by having an explicit mux dev. > + chip select. SSI busses can therefore have any number of slaves, > + of which a master can select using ssi_select_slave(). > It is assumed that master and slave are both using the same transfer This would be much more convincing if you'd also implemented it. In particular the lm3s6965evb has two devices (LCD controller and SD card) connected to a single SSI controller, and selects between the two by twiddling chip selects. Paul
