Re: [Qemu-devel] [PATCH target-arm v5 4/5] arm: Implement reset GPIO.

[Peter Crosthwaite]

On Tue, Jan 28, 2014 at 7:22 PM, Peter Maydell <address@hidden> wrote:
> On 28 January 2014 00:48, Peter Crosthwaite
> <address@hidden> wrote:
>> On Tue, Jan 28, 2014 at 3:52 AM, Peter Maydell <address@hidden> wrote:
>>> On 15 January 2014 09:14, Peter Crosthwaite
>>> <address@hidden> wrote:
>>>> Implement a reset GPIO for ARM CPUs. This allows individual reset of ARM
>>>> CPUs from device land without the need for the much unwanted reset API
>>>> calls.
>>>>
>>>> The CPU is halted as long as the pin is held in reset. Releasing the
>>>> reset starts the CPU running again.
>>>
>>>> +static void arm_cpu_reset_gpio(void *opaque, int irq, int level)
>>>> +{
>>>> +    CPUState *cpu = opaque;
>>>> +
>>>> +    if (level) {
>>>> +        cpu_reset(cpu);
>>>> +        cpu_interrupt(cpu, CPU_INTERRUPT_HALT);
>>>> +    } else {
>>>> +        cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);
>>>> +        cpu_interrupt(cpu, CPU_INTERRUPT_EXITTB);
>>>> +    }
>>>> +}
>>>
>>> I don't think this will work properly. For instance,
>>> cpu_exec() will bring the CPU out of halt if an inbound
>>> interrupt arrives, but we should stay in reset until
>>> the reset line is deasserted.
>>>
>>
>> I see. I guess I'm going to have to save the reset pin state as a
>> boolean in CPU state (and I guess that means is should be migratable).
>> Then inhibit the true return from cpu_has_work when the pin is
>> asserted.
>
> Maybe we can do it with a CPU_INTERRUPT_RESET ?
>
>>> Also ideally speaking we should probably do the reset
>>> actions on the falling edge of reset, not the rising edge.
>>>
>>
>> Any particular reason? I would have thought that any externally
>> visible state would best be reset ASAP. For level sensitive behavior,
>> the transitionals should happen going into the active level. Unless
>> ARM CPU resets are actually falling edge sensitive (in which case the
>> CPU would continue to run while the reset is held).
>
> I'm not an expert here, but what I thought happened was:
>  * on asserting edge of reset CPU stops running and starts
>    resetting its internal state
>  * on real hardware you have to hold the line asserted for N clocks
>    for this to finish
>  * on deasserting edge of reset, CPU samples config lines
>    and starts running again
>
> so you should be able to say 'assert reset; change state of
> configuration signals; deassert reset', for instance. (On QEMU
> that would probably be 'assert reset; change r/w QOM properties;
> deassert reset'). Slightly less theoretically, M-class CPUs do
> the initial read of the PC from memory in cpu_reset(), so if you
> do that on asserting edge then you prevent "hold CPU in reset
> and modify the vector table before releasing CPU".
>
> So at least some of what we do in cpu_reset() ought I think
> to be done only on deasserting; unless you really want to
> split reset into two phases, it seems easiest just to hold the
> CPU not-running while reset is asserted and call cpu_reset()
> when it is deasserted.
>
>>> Does this work properly when we're running under KVM
>>> rather than using the TCG CPU?
>>
>> I must confess no, I explicitly LOG_UNIMP for KVM, as I have no means
>> to develop or test ARM KVM.
>
> I definitely don't want to take this if it doesn't cope with KVM.
> I guess I'll have to take a look at how that would work.
>

Is it as simple as a call to kvm_reset_vcpu?

Regards,
Peter

>>> Is there anything really ARM-specific in this reset_gpio
>>> function, or could it be implemented at a common level for
>>> all target architectures?
>>>
>>
>> Not yet, but probably will be ARM specific once I add the cpu reset
>> pin state. Unless Andreas is happy for that pin state and all this
>> code to go up to the base TYPE_CPU class.
>
> It might be possible for most of the mechanics to be shared
> (CPU_INTERRUPT_RESET and what setting/clearing it does),
> just not the gpio pin.
>
>> I wonder however, whether different arch will have level/edge/high/low
>> variances in reset behavior that must be accommodated.
>
> Mmm.
>
> thanks
> -- PMM
>