| Message ID | 1432814996-13464-4-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 5 June 2015 at 23:57, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Thu, May 28, 2015 at 5:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> The pxa2xx_ssp device was missing a reset method; add one. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> hw/arm/pxa2xx.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c >> index fc77b44..770902f 100644 >> --- a/hw/arm/pxa2xx.c >> +++ b/hw/arm/pxa2xx.c >> @@ -756,6 +756,22 @@ static int pxa2xx_ssp_load(QEMUFile *f, void *opaque, int version_id) >> return 0; >> } >> >> +static void pxa2xx_ssp_reset(DeviceState *d) >> +{ >> + PXA2xxSSPState *s = PXA2XX_SSP(d); >> + >> + s->enable = 0; >> + s->sscr[0] = s->sscr[1] = 0; >> + s->sspsp = 0; >> + s->ssto = 0; >> + s->ssitr = 0; >> + s->sssr = 0; >> + s->sstsa = 0; >> + s->ssrsa = 0; >> + s->ssacd = 0; >> + s->rx_start = s->rx_level = 0; > > Does this need a ssp_int_update to deassert any set interrupts? No, because the thing on the other end of the irq line should also reset itself to an "interrupt deasserted" state. Calling qemu_set_irq or qemu_reset_irq in a reset function is dubious, because you don't know if the device on the other end has (a) already reset itself or (b) not yet reset itself but will do so after you; so the effect on the other device is indeterminate... -- PMM
On 6 June 2015 at 00:06, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > But I have some follow up design questions, mainly in that I have > started using individual device resets that back onto the ->reset hook > to implement reset controllers. This means an IRQ source should reset > its IRQ pin to notify its sinks of the state change as those sinks > will not get a reset callback. > > I know its not supported by the current reset semantic but can we > start doing this going forward? Device reset is a sink and a quagmire. Note incidentally that what we call "reset" in QEMU is actually "we hard powercycled the simulation", not an emulated reset. If you can propose some coherent and workable semantics I think it would be nice if we could clean things up. (You may find you need two-phase reset by analogy with two-phase init...) -- PMM
On 6 June 2015 at 02:37, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Fri, Jun 5, 2015 at 4:18 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> Device reset is a sink and a quagmire. Note incidentally that what >> we call "reset" in QEMU is actually "we hard powercycled the simulation", >> not an emulated reset. If you can propose some coherent and workable >> semantics I think it would be nice if we could clean things up. >> (You may find you need two-phase reset by analogy with two-phase init...) >> > > So I don't really like the predetermined finite number of init phases > as the number of phases seems to just grow over time. We already have > about 4 phases of init if you count all of init() post_init(), > realize() and machine_init_done-notifiers. Resets will probably go the > same way. Can we instead have a dependency management system where > reset handlers can register deps? If you rely on another device for > your reset (e.g. a boot-loader must wait out a CPU reset) then it > makes sense you have awareness of that dep and can use a QOM link > navigation to register a reset dep. Devs can register multiple resets > if deps demand but I would expect that to be super rare. I don't think this will work, because as a device you don't know what you're connected to. The idea I had was something like: * phase 1, all devices reset their internal state and reset outbound signal lines accordingly They also set a flag to say "being held in reset"; calls inbound for "your gpio line changed" are only handled to note the new state of the line, not to trigger device state change * phase 2, devices go out of reset: they do whatever the device does when it leaves reset and samples the state of its inbound signals (which might result in changing state, outbound signal levels, etc) These basically correspond to "leading edge of reset" and "trailing edge of reset". If you're modelling them with QEMU IRQs then they'd be "things done when level goes 0->1" and "things done when level goes 1->0". There is an obvious problem here, though, which is that this is (a) an enormous upheaval in every device and (b) breaking migration, because of all the new state. Anthony Liguori had a vaguely similar idea at one point IIRC, but done by making the QEMU IRQ lines maintain internal state (at the moment they don't at all) rather than having the device do it. It's probably worth trawling the list archives for earlier discussions and ideas... > So my semantics are: > > * The reset function must reset the device to POR state. > * Resets may depend on any number of other resets to occur first. No > loops in the deps. > * If a device is reset concurrently with one of its deps, the dep is > guaranteed to go first > * a dev with externally provided state that doesn't register reset dep > may need to preserve that state through reset (e.g. state of a GPIO > input). > * Systems can register reset domains, with reset operations respecting > deps within > * It is an error to define a reset domain that doesn't include all deps. > * By extension a trivial one-device domain can implement an individual > core reset (if the platform/dev supports that) How many cases really need this dependency-driven reset rather than just being "I have an external signal that tells me when to reset" ? There are oddities like the boot loader, but is there anything else? > FWIW, I already have reset GPIOs for ARM CPUs that back onto > device::reset. These in turn connect to the reset controller to > support run time CPU reset via hardware peripheral. Right, this is what I would think of as the "normal case". -- PMM
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c index fc77b44..770902f 100644 --- a/hw/arm/pxa2xx.c +++ b/hw/arm/pxa2xx.c @@ -756,6 +756,22 @@ static int pxa2xx_ssp_load(QEMUFile *f, void *opaque, int version_id) return 0; } +static void pxa2xx_ssp_reset(DeviceState *d) +{ + PXA2xxSSPState *s = PXA2XX_SSP(d); + + s->enable = 0; + s->sscr[0] = s->sscr[1] = 0; + s->sspsp = 0; + s->ssto = 0; + s->ssitr = 0; + s->sssr = 0; + s->sstsa = 0; + s->ssrsa = 0; + s->ssacd = 0; + s->rx_start = s->rx_level = 0; +} + static int pxa2xx_ssp_init(SysBusDevice *sbd) { DeviceState *dev = DEVICE(sbd); @@ -2333,8 +2349,10 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size) static void pxa2xx_ssp_class_init(ObjectClass *klass, void *data) { SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); sdc->init = pxa2xx_ssp_init; + dc->reset = pxa2xx_ssp_reset; } static const TypeInfo pxa2xx_ssp_info = {
The pxa2xx_ssp device was missing a reset method; add one. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/pxa2xx.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)