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