Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse

[Shannon Zhao <zhaoshenglong@huawei.com>]

Hi Wei,

On 2016/2/10 6:59, Wei Huang wrote:
> 
> On 02/04/2016 12:51 AM, Shannon Zhao wrote:
>>
>>
>> On 2016/2/4 14:10, Wei Huang wrote:
>>>
>>> On 02/03/2016 07:44 PM, Shannon Zhao wrote:
> 
> <snip>
> 
>>> I reversed the order of edge pulling. The state is 1 according to printk
>>> inside gpio_keys driver. However the reboot still failed with two
>>> reboots (1 very early, 1 later).
>>>
>> Because to make the input work, it should call input_event twice I think.
>>
>> input_event(input, type, button->code, 1) means the button pressed
>> input_event(input, type, button->code, 0) means the button released
>>
>> But We only see guest entering gpio_keys_gpio_report_event once.
>>
>> My original purpose is like below:
>>
>> call qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1) to make guest
>> execute input_event(input, type, button->code, 1)
>> call qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0) to make guest
>> execute input_event(input, type, button->code, 0).
>>
>> But even though it calls qemu_set_irq twice, it only calls pl061_update
>> once in qemu.
> 
> Hi Shannon,
> 
> Assuming that we are talking about the special case you found (i.e. send
> reboot very early and then send another one after guest VM fully
> booted). Dug into the code further, here are my findings:
> 
> === Why ACPI case failed? ===
> QEMU pl061.c checks the current request against the new request (see
> s->old_in_data ^ s->data in pl061.c file). If no change, nothing will
> happen.
> 
> So two consecutive reboots will cause the following state change;
> apparently the second request won't trigger VM reboot because
> pl01_update() decides _not_ to inject IRQ into guest VM.
> 
>   (PL061State fields)           data   old_in_data   istate
> VM boot                         0      0             0
> 1st ACPI reboot request         8      0             0
> After VM PL061 driver ACK       8      8             0
> 2nd ACPI reboot request         8     no-change, no IRQ <==
> 
> To solve this problem, we have to invert PL061State->data at least once
> to trigger IRQ inside VM. Two solutions:
> 
> S1: "Pulse"
> static void virt_powerdown_req(Notifier *n, void *opaque)
> {
>     qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);
>     qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
> }
> 
> S2: "Invert"
> static int old_gpio_level = 0;
> static void virt_powerdown_req(Notifier *n, void *opaque)
> {
>     /* use gpio Pin 3 for power button event */
>     qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), !old_gpio_level);
>     old_gpio_level = !old_gpio_level;
> }
> 
The S2 still doesn't work. After sending the early first reboot, whne
guest boots well, it doesn't react to the second reboot while it reacts
to the third one.

> Both S1 and S2 works for ACPI. But S1 has problem with DT, which is
> explained below.
> 
> === Why DT fails with S1 ===
> DT approach requires both PL061 and gpio-keys to work together. Looking
> into guest kernel gpio-keys/input code, you will find that it only
> reacts to both LEVEL-HI and input changes. Here is the code snip from
> drivers/input/input.c file:
> 
>     /* auto-repeat bypasses state updates */
>     if (value == 2) {
>             disposition = INPUT_PASS_TO_HANDLERS;
>             break;
>     }
> 
>     if (!!test_bit(code, dev->key) != !!value) {
>             __change_bit(code, dev->key);
>             disposition = INPUT_PASS_TO_HANDLERS;
>     }
> 
> Unless we adds gpio-keys DT property to "autorepeat", the
> "!!test_bit(code, dev->key) != !!value" is always FALSE with S1. Thus
> systemd won't receive any input event; and no power-switch will be
> triggered.
> 
> In comparison S2 will work because value is changed very time.
> 
> === Summary ===
> 1. Cleaning PL061 state is required. That means "[PATCH V2 1/2] ARM:
> PL061: Clear PL061 device state after reset" is necessary.
> 2. S2 is better. To support S2, we needs to change GPIO IRQ polarity to
> AML_ACTIVE_BOTH in ACPI description.
> 
Thanks. But we don't have the AML_ACTIVE_BOTH since there is only one
bit for "Interrupt Polarity" in ACPI table.
See ACPI SPEC 6.0 "Table 6-216 Extended Interrupt Descriptor Definition"
Bit [2] Interrupt Polarity, _LL
0 Active-High: This interrupt is sampled when the signal is high,
or true.
1 Active-Low: This interrupt is sampled when the signal is low, or
false.

> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -326,7 +326,7 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
>      Aml *aei = aml_resource_template();
>      /* Pin 3 for power button */
>      const uint32_t pin_list[1] = {3};
> -    aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
> +    aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, 3,
What's the meaning of 3 here?

>                                   AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1,
>                                   "GPO0", NULL, 0));
>      aml_append(dev, aml_name_decl("_AEI", aei));

Thanks,
-- 
Shannon