[v3,1/7] arm/virt: place power button pin number on a define

Commit Message

Mauro Carvalho Chehab July 22, 2024, 6:45 a.m. UTC

Having magic numbers inside the code is not a good idea, as it
is error-prone. So, instead, create a macro with the number
definition.

Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
Reviewed-by: Jonathan Cameron <[e-mail]>
---
 hw/arm/virt-acpi-build.c | 6 +++---
 hw/arm/virt.c            | 7 ++++---
 include/hw/arm/virt.h    | 3 +++
 3 files changed, 10 insertions(+), 6 deletions(-)

Comments

Igor Mammedov July 30, 2024, 7:25 a.m. UTC | #1

On Mon, 22 Jul 2024 08:45:53 +0200
Mauro Carvalho Chehab <[e-mail]> wrote:

> Having magic numbers inside the code is not a good idea, as it
> is error-prone. So, instead, create a macro with the number
> definition.
> 
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> Reviewed-by: Jonathan Cameron <[e-mail]>
> ---
>  hw/arm/virt-acpi-build.c | 6 +++---
>  hw/arm/virt.c            | 7 ++++---
>  include/hw/arm/virt.h    | 3 +++
>  3 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index e10cad86dd73..f76fb117adff 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -154,10 +154,10 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
>      aml_append(dev, aml_name_decl("_CRS", crs));
>  
>      Aml *aei = aml_resource_template();
> -    /* Pin 3 for power button */
> -    const uint32_t pin_list[1] = {3};
> +
> +    const uint32_t pin = GPIO_PIN_POWER_BUTTON;
>      aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
> -                                 AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1,
> +                                 AML_EXCLUSIVE, AML_PULL_UP, 0, &pin, 1,
>                                   "GPO0", NULL, 0));
>      aml_append(dev, aml_name_decl("_AEI", aei));
>  
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b0c68d66a345..c99c8b1713c6 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1004,7 +1004,7 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
>      if (s->acpi_dev) {
>          acpi_send_event(s->acpi_dev, ACPI_POWER_DOWN_STATUS);
>      } else {
> -        /* use gpio Pin 3 for power button event */
> +        /* use gpio Pin for power button event */
>          qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);

/me confused, it was saying Pin 3 but is passing 0 as argument where as elsewhere
you are passing 3. Is this a bug?

BTW: dropping '3' from comment doesn't make it any better.

>      }
>  }
> @@ -1013,7 +1013,8 @@ static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
>                               uint32_t phandle)
>  {
>      gpio_key_dev = sysbus_create_simple("gpio-key", -1,
> -                                        qdev_get_gpio_in(pl061_dev, 3));
> +                                        qdev_get_gpio_in(pl061_dev,
> +                                                         GPIO_PIN_POWER_BUTTON));
>  
>      qemu_fdt_add_subnode(fdt, "/gpio-keys");
>      qemu_fdt_setprop_string(fdt, "/gpio-keys", "compatible", "gpio-keys");
> @@ -1024,7 +1025,7 @@ static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
>      qemu_fdt_setprop_cell(fdt, "/gpio-keys/poweroff", "linux,code",
>                            KEY_POWER);
>      qemu_fdt_setprop_cells(fdt, "/gpio-keys/poweroff",
> -                           "gpios", phandle, 3, 0);
> +                           "gpios", phandle, GPIO_PIN_POWER_BUTTON, 0);
>  }
>  
>  #define SECURE_GPIO_POWEROFF 0
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index ab961bb6a9b8..a4d937ed45ac 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -47,6 +47,9 @@
>  /* See Linux kernel arch/arm64/include/asm/pvclock-abi.h */
>  #define PVTIME_SIZE_PER_CPU 64
>  
> +/* GPIO pins */
> +#define GPIO_PIN_POWER_BUTTON  3
> +
>  enum {
>      VIRT_FLASH,
>      VIRT_MEM,

Peter Maydell July 30, 2024, 8:29 a.m. UTC | #2

On Tue, 30 Jul 2024 at 08:26, Igor Mammedov <[e-mail]> wrote:
>
> On Mon, 22 Jul 2024 08:45:53 +0200
> Mauro Carvalho Chehab <[e-mail]> wrote:
>
> > Having magic numbers inside the code is not a good idea, as it
> > is error-prone. So, instead, create a macro with the number
> > definition.
> >
> > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > Reviewed-by: Jonathan Cameron <[e-mail]>

> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index b0c68d66a345..c99c8b1713c6 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1004,7 +1004,7 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
> >      if (s->acpi_dev) {
> >          acpi_send_event(s->acpi_dev, ACPI_POWER_DOWN_STATUS);
> >      } else {
> > -        /* use gpio Pin 3 for power button event */
> > +        /* use gpio Pin for power button event */
> >          qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);
>
> /me confused, it was saying Pin 3 but is passing 0 as argument where as elsewhere
> you are passing 3. Is this a bug?

No. The gpio_key_dev is a gpio-key device which has one
input (which you assert to "press the key") and one output,
which goes high when the key is pressed and then falls
100ms later. The virt board wires up the output of the
gpio-key device to input 3 on the PL061 GPIO controller.
(This happens in create_gpio_keys().) So the code is correct
to assert input 0 on the gpio-key device and the comment
isn't wrong that this results in GPIO pin 3 being asserted:
the link is just indirect.

thanks
-- PMM

Igor Mammedov July 30, 2024, 11:26 a.m. UTC | #3

On Tue, 30 Jul 2024 09:29:37 +0100
Peter Maydell <[e-mail]> wrote:

> On Tue, 30 Jul 2024 at 08:26, Igor Mammedov <[e-mail]> wrote:
> >
> > On Mon, 22 Jul 2024 08:45:53 +0200
> > Mauro Carvalho Chehab <[e-mail]> wrote:
> >  
> > > Having magic numbers inside the code is not a good idea, as it
> > > is error-prone. So, instead, create a macro with the number
> > > definition.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > > Reviewed-by: Jonathan Cameron <[e-mail]>  
> 
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index b0c68d66a345..c99c8b1713c6 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -1004,7 +1004,7 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
> > >      if (s->acpi_dev) {
> > >          acpi_send_event(s->acpi_dev, ACPI_POWER_DOWN_STATUS);
> > >      } else {
> > > -        /* use gpio Pin 3 for power button event */
> > > +        /* use gpio Pin for power button event */
> > >          qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);  
> >
> > /me confused, it was saying Pin 3 but is passing 0 as argument where as elsewhere
> > you are passing 3. Is this a bug?  
> 
> No. The gpio_key_dev is a gpio-key device which has one
> input (which you assert to "press the key") and one output,
> which goes high when the key is pressed and then falls
> 100ms later. The virt board wires up the output of the
> gpio-key device to input 3 on the PL061 GPIO controller.
> (This happens in create_gpio_keys().) So the code is correct
> to assert input 0 on the gpio-key device and the comment
> isn't wrong that this results in GPIO pin 3 being asserted:
> the link is just indirect.

it's likely obvious to ARM folks, but maybe comment should
clarify above for unaware.
 
> 
> thanks
> -- PMM
>

Mauro Carvalho Chehab Aug. 1, 2024, 1:15 p.m. UTC | #4

Em Tue, 30 Jul 2024 13:26:20 +0200
Igor Mammedov <[e-mail]> escreveu:

> On Tue, 30 Jul 2024 09:29:37 +0100
> Peter Maydell <[e-mail]> wrote:
> 
> > On Tue, 30 Jul 2024 at 08:26, Igor Mammedov <[e-mail]> wrote:  
> > >
> > > On Mon, 22 Jul 2024 08:45:53 +0200
> > > Mauro Carvalho Chehab <[e-mail]> wrote:
> > >    
> > > > Having magic numbers inside the code is not a good idea, as it
> > > > is error-prone. So, instead, create a macro with the number
> > > > definition.
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > > > Reviewed-by: Jonathan Cameron <[e-mail]>    
> >   
> > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > index b0c68d66a345..c99c8b1713c6 100644
> > > > --- a/hw/arm/virt.c
> > > > +++ b/hw/arm/virt.c
> > > > @@ -1004,7 +1004,7 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
> > > >      if (s->acpi_dev) {
> > > >          acpi_send_event(s->acpi_dev, ACPI_POWER_DOWN_STATUS);
> > > >      } else {
> > > > -        /* use gpio Pin 3 for power button event */
> > > > +        /* use gpio Pin for power button event */
> > > >          qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);    
> > >
> > > /me confused, it was saying Pin 3 but is passing 0 as argument where as elsewhere
> > > you are passing 3. Is this a bug?    
> > 
> > No. The gpio_key_dev is a gpio-key device which has one
> > input (which you assert to "press the key") and one output,
> > which goes high when the key is pressed and then falls
> > 100ms later. The virt board wires up the output of the
> > gpio-key device to input 3 on the PL061 GPIO controller.
> > (This happens in create_gpio_keys().) So the code is correct
> > to assert input 0 on the gpio-key device and the comment
> > isn't wrong that this results in GPIO pin 3 being asserted:
> > the link is just indirect.  
> 
> it's likely obvious to ARM folks, but maybe comment should
> clarify above for unaware.

Not sure if a comment here with the pin number is a good idea.
After all, this patch was originated because we were using
Pin 6 for GPIO error, while the comment was outdated (stating
that it was pin 8 instead) :-)

After this series, there will be two GPIO pins used inside arm/virt,
both defined at arm/virt.h:

	/* GPIO pins */
	#define GPIO_PIN_POWER_BUTTON  3
	#define GPIO_PIN_GENERIC_ERROR 6

Those macros are used when GPIOs are created:

	static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
	                             uint32_t phandle)
	{
	    gpio_key_dev = sysbus_create_simple("gpio-key", -1,
	                                        qdev_get_gpio_in(pl061_dev,
                                                         GPIO_PIN_POWER_BUTTON));
	    gpio_error_dev = sysbus_create_simple("gpio-key", -1,
	                                          qdev_get_gpio_in(pl061_dev,
	                                                           GPIO_PIN_GENERIC_ERROR));
So, at least for me, it is clear that gpio_key_dev is using pin 3.

Thanks,
Mauro

Igor Mammedov Aug. 5, 2024, 2:04 p.m. UTC | #5

On Thu, 1 Aug 2024 15:15:44 +0200
Mauro Carvalho Chehab <[e-mail]> wrote:

> Em Tue, 30 Jul 2024 13:26:20 +0200
> Igor Mammedov <[e-mail]> escreveu:
> 
> > On Tue, 30 Jul 2024 09:29:37 +0100
> > Peter Maydell <[e-mail]> wrote:
> >   
> > > On Tue, 30 Jul 2024 at 08:26, Igor Mammedov <[e-mail]> wrote:    
> > > >
> > > > On Mon, 22 Jul 2024 08:45:53 +0200
> > > > Mauro Carvalho Chehab <[e-mail]> wrote:
> > > >      
> > > > > Having magic numbers inside the code is not a good idea, as it
> > > > > is error-prone. So, instead, create a macro with the number
> > > > > definition.
> > > > >
> > > > > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > > > > Reviewed-by: Jonathan Cameron <[e-mail]>      
> > >     
> > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > > index b0c68d66a345..c99c8b1713c6 100644
> > > > > --- a/hw/arm/virt.c
> > > > > +++ b/hw/arm/virt.c
> > > > > @@ -1004,7 +1004,7 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
> > > > >      if (s->acpi_dev) {
> > > > >          acpi_send_event(s->acpi_dev, ACPI_POWER_DOWN_STATUS);
> > > > >      } else {
> > > > > -        /* use gpio Pin 3 for power button event */
> > > > > +        /* use gpio Pin for power button event */
> > > > >          qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);      
> > > >
> > > > /me confused, it was saying Pin 3 but is passing 0 as argument where as elsewhere
> > > > you are passing 3. Is this a bug?      
> > > 
> > > No. The gpio_key_dev is a gpio-key device which has one
> > > input (which you assert to "press the key") and one output,
> > > which goes high when the key is pressed and then falls
> > > 100ms later. The virt board wires up the output of the
> > > gpio-key device to input 3 on the PL061 GPIO controller.
> > > (This happens in create_gpio_keys().) So the code is correct
> > > to assert input 0 on the gpio-key device and the comment
> > > isn't wrong that this results in GPIO pin 3 being asserted:
> > > the link is just indirect.    
> > 
> > it's likely obvious to ARM folks, but maybe comment should
> > clarify above for unaware.  
> 
> Not sure if a comment here with the pin number is a good idea.
> After all, this patch was originated because we were using
> Pin 6 for GPIO error, while the comment was outdated (stating
> that it was pin 8 instead) :-)
> 
> After this series, there will be two GPIO pins used inside arm/virt,
> both defined at arm/virt.h:
> 
> 	/* GPIO pins */
> 	#define GPIO_PIN_POWER_BUTTON  3
> 	#define GPIO_PIN_GENERIC_ERROR 6
> 
> Those macros are used when GPIOs are created:
> 
> 	static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
> 	                             uint32_t phandle)
> 	{
> 	    gpio_key_dev = sysbus_create_simple("gpio-key", -1,
> 	                                        qdev_get_gpio_in(pl061_dev,
>                                                          GPIO_PIN_POWER_BUTTON));
> 	    gpio_error_dev = sysbus_create_simple("gpio-key", -1,
> 	                                          qdev_get_gpio_in(pl061_dev,
> 	                                                           GPIO_PIN_GENERIC_ERROR));
> So, at least for me, it is clear that gpio_key_dev is using pin 3.

if you switch to using already existing GED device,
then this patch will go away since event will be delivered by GED
instead of GPIO + _AEI.

> 
> Thanks,
> Mauro
>

Mauro Carvalho Chehab Aug. 5, 2024, 3:22 p.m. UTC | #6

Em Mon, 5 Aug 2024 16:04:39 +0200
Igor Mammedov <[e-mail]> escreveu:

> On Thu, 1 Aug 2024 15:15:44 +0200
> Mauro Carvalho Chehab <[e-mail]> wrote:
> 
> > Em Tue, 30 Jul 2024 13:26:20 +0200
> > Igor Mammedov <[e-mail]> escreveu:
> >   
> > > On Tue, 30 Jul 2024 09:29:37 +0100
> > > Peter Maydell <[e-mail]> wrote:
> > >     
> > > > On Tue, 30 Jul 2024 at 08:26, Igor Mammedov <[e-mail]> wrote:      
> > > > >
> > > > > On Mon, 22 Jul 2024 08:45:53 +0200
> > > > > Mauro Carvalho Chehab <[e-mail]> wrote:
> > > > >        
> > > > > > Having magic numbers inside the code is not a good idea, as it
> > > > > > is error-prone. So, instead, create a macro with the number
> > > > > > definition.
> > > > > >
> > > > > > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > > > > > Reviewed-by: Jonathan Cameron <[e-mail]>        
> > > >       
> > > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > > > index b0c68d66a345..c99c8b1713c6 100644
> > > > > > --- a/hw/arm/virt.c
> > > > > > +++ b/hw/arm/virt.c
> > > > > > @@ -1004,7 +1004,7 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
> > > > > >      if (s->acpi_dev) {
> > > > > >          acpi_send_event(s->acpi_dev, ACPI_POWER_DOWN_STATUS);
> > > > > >      } else {
> > > > > > -        /* use gpio Pin 3 for power button event */
> > > > > > +        /* use gpio Pin for power button event */
> > > > > >          qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);        
> > > > >
> > > > > /me confused, it was saying Pin 3 but is passing 0 as argument where as elsewhere
> > > > > you are passing 3. Is this a bug?        
> > > > 
> > > > No. The gpio_key_dev is a gpio-key device which has one
> > > > input (which you assert to "press the key") and one output,
> > > > which goes high when the key is pressed and then falls
> > > > 100ms later. The virt board wires up the output of the
> > > > gpio-key device to input 3 on the PL061 GPIO controller.
> > > > (This happens in create_gpio_keys().) So the code is correct
> > > > to assert input 0 on the gpio-key device and the comment
> > > > isn't wrong that this results in GPIO pin 3 being asserted:
> > > > the link is just indirect.      
> > > 
> > > it's likely obvious to ARM folks, but maybe comment should
> > > clarify above for unaware.    
> > 
> > Not sure if a comment here with the pin number is a good idea.
> > After all, this patch was originated because we were using
> > Pin 6 for GPIO error, while the comment was outdated (stating
> > that it was pin 8 instead) :-)
> > 
> > After this series, there will be two GPIO pins used inside arm/virt,
> > both defined at arm/virt.h:
> > 
> > 	/* GPIO pins */
> > 	#define GPIO_PIN_POWER_BUTTON  3
> > 	#define GPIO_PIN_GENERIC_ERROR 6
> > 
> > Those macros are used when GPIOs are created:
> > 
> > 	static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
> > 	                             uint32_t phandle)
> > 	{
> > 	    gpio_key_dev = sysbus_create_simple("gpio-key", -1,
> > 	                                        qdev_get_gpio_in(pl061_dev,
> >                                                          GPIO_PIN_POWER_BUTTON));
> > 	    gpio_error_dev = sysbus_create_simple("gpio-key", -1,
> > 	                                          qdev_get_gpio_in(pl061_dev,
> > 	                                                           GPIO_PIN_GENERIC_ERROR));
> > So, at least for me, it is clear that gpio_key_dev is using pin 3.  
> 
> if you switch to using already existing GED device,
> then this patch will go away since event will be delivered by GED
> instead of GPIO + _AEI.

This patch is actually independent from the rest. It is related to a power
down event, and not related at all with error inject.

The rationale for keeping it on this series was due to the original
patch 2 (as otherwise merge conflicts would rise). It can now be merged
in separate.

Btw, this is doing a cleanup requested by Michael and Peter:

	https://lore.kernel.org/qemu-devel/CAFEAcA-PYnZ-32MRX+PgvzhnoAV80zBKMYg61j2f=[e-mail]/

Thanks,
Mauro

Patch

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index e10cad86dd73..f76fb117adff 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -154,10 +154,10 @@  static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
     aml_append(dev, aml_name_decl("_CRS", crs));
 
     Aml *aei = aml_resource_template();
-    /* Pin 3 for power button */
-    const uint32_t pin_list[1] = {3};
+
+    const uint32_t pin = GPIO_PIN_POWER_BUTTON;
     aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
-                                 AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1,
+                                 AML_EXCLUSIVE, AML_PULL_UP, 0, &pin, 1,
                                  "GPO0", NULL, 0));
     aml_append(dev, aml_name_decl("_AEI", aei));
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b0c68d66a345..c99c8b1713c6 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1004,7 +1004,7 @@  static void virt_powerdown_req(Notifier *n, void *opaque)
     if (s->acpi_dev) {
         acpi_send_event(s->acpi_dev, ACPI_POWER_DOWN_STATUS);
     } else {
-        /* use gpio Pin 3 for power button event */
+        /* use gpio Pin for power button event */
         qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);
     }
 }
@@ -1013,7 +1013,8 @@  static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
                              uint32_t phandle)
 {
     gpio_key_dev = sysbus_create_simple("gpio-key", -1,
-                                        qdev_get_gpio_in(pl061_dev, 3));
+                                        qdev_get_gpio_in(pl061_dev,
+                                                         GPIO_PIN_POWER_BUTTON));
 
     qemu_fdt_add_subnode(fdt, "/gpio-keys");
     qemu_fdt_setprop_string(fdt, "/gpio-keys", "compatible", "gpio-keys");
@@ -1024,7 +1025,7 @@  static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
     qemu_fdt_setprop_cell(fdt, "/gpio-keys/poweroff", "linux,code",
                           KEY_POWER);
     qemu_fdt_setprop_cells(fdt, "/gpio-keys/poweroff",
-                           "gpios", phandle, 3, 0);
+                           "gpios", phandle, GPIO_PIN_POWER_BUTTON, 0);
 }
 
 #define SECURE_GPIO_POWEROFF 0
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index ab961bb6a9b8..a4d937ed45ac 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -47,6 +47,9 @@ 
 /* See Linux kernel arch/arm64/include/asm/pvclock-abi.h */
 #define PVTIME_SIZE_PER_CPU 64
 
+/* GPIO pins */
+#define GPIO_PIN_POWER_BUTTON  3
+
 enum {
     VIRT_FLASH,
     VIRT_MEM,