diff mbox series

sunxi: Properly check for SATAPWR and MACPWR

Message ID 20210119010520.13284-1-andre.przywara@arm.com
State Accepted
Commit e9ad1b8dc5c7bae5213309af7c49907bac5def73
Delegated to: Andre Przywara
Headers show
Series sunxi: Properly check for SATAPWR and MACPWR | expand

Commit Message

André Przywara Jan. 19, 2021, 1:05 a.m. UTC
The #ifdef CONFIG_xxxPWR conditionals were not working as expected, as
string Kconfig symbols are always "defined" from the preprocessor's
perspective. This lead to unnecessary calls to the GPIO routines, but
also always added a half a second delay to wait for a SATA disk to power
up. Many thanks to Peter for pointing this out!

Fix this by properly comparing the Kconfig symbols against the empty
string. strcmp() would be nicer for this, but GCC does not optimise this
away, probably due to our standalone compiler switches.

Reported-by: Peter Robinson <pbrobinson@gmail.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 board/sunxi/board.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

Comments

Samuel Holland Jan. 19, 2021, 1:46 a.m. UTC | #1
On 1/18/21 7:05 PM, Andre Przywara wrote:
> The #ifdef CONFIG_xxxPWR conditionals were not working as expected, as
> string Kconfig symbols are always "defined" from the preprocessor's
> perspective. This lead to unnecessary calls to the GPIO routines, but
> also always added a half a second delay to wait for a SATA disk to power
> up. Many thanks to Peter for pointing this out!
> 
> Fix this by properly comparing the Kconfig symbols against the empty
> string. strcmp() would be nicer for this, but GCC does not optimise this
> away, probably due to our standalone compiler switches.

Ethernet still works, and the speed-up is welcome.

Tested-by: Samuel Holland <samuel@sholland.org> # Orange Pi WinPlus

Cheers,
Samuel
Peter Robinson Jan. 19, 2021, 2:58 p.m. UTC | #2
On Tue, Jan 19, 2021 at 1:06 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> The #ifdef CONFIG_xxxPWR conditionals were not working as expected, as
> string Kconfig symbols are always "defined" from the preprocessor's
> perspective. This lead to unnecessary calls to the GPIO routines, but
> also always added a half a second delay to wait for a SATA disk to power
> up. Many thanks to Peter for pointing this out!
>
> Fix this by properly comparing the Kconfig symbols against the empty
> string. strcmp() would be nicer for this, but GCC does not optimise this
> away, probably due to our standalone compiler switches.
>
> Reported-by: Peter Robinson <pbrobinson@gmail.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Tested-by: Peter Robinson <pbrobinson@gmail.com>

Tested on Pine64, Cubietruck with root fs on SATA SSD, and an orangepi_pc
> ---
>  board/sunxi/board.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 4f058952b5b..a0b5778b3bc 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -265,18 +265,28 @@ int board_init(void)
>         if (ret)
>                 return ret;
>
> -#ifdef CONFIG_SATAPWR
> -       satapwr_pin = sunxi_name_to_gpio(CONFIG_SATAPWR);
> -       gpio_request(satapwr_pin, "satapwr");
> -       gpio_direction_output(satapwr_pin, 1);
> -       /* Give attached sata device time to power-up to avoid link timeouts */
> -       mdelay(500);
> -#endif
> -#ifdef CONFIG_MACPWR
> -       macpwr_pin = sunxi_name_to_gpio(CONFIG_MACPWR);
> -       gpio_request(macpwr_pin, "macpwr");
> -       gpio_direction_output(macpwr_pin, 1);
> -#endif
> +       /* strcmp() would look better, but doesn't get optimised away. */
> +       if (CONFIG_SATAPWR[0]) {
> +               satapwr_pin = sunxi_name_to_gpio(CONFIG_SATAPWR);
> +               if (satapwr_pin >= 0) {
> +                       gpio_request(satapwr_pin, "satapwr");
> +                       gpio_direction_output(satapwr_pin, 1);
> +
> +                       /*
> +                        * Give the attached SATA device time to power-up
> +                        * to avoid link timeouts
> +                        */
> +                       mdelay(500);
> +               }
> +       }
> +
> +       if (CONFIG_MACPWR[0]) {
> +               macpwr_pin = sunxi_name_to_gpio(CONFIG_MACPWR);
> +               if (macpwr_pin >= 0) {
> +                       gpio_request(macpwr_pin, "macpwr");
> +                       gpio_direction_output(macpwr_pin, 1);
> +               }
> +       }
>
>  #ifdef CONFIG_DM_I2C
>         /*
> --
> 2.17.5
>
Jernej Škrabec Jan. 19, 2021, 6:36 p.m. UTC | #3
Dne torek, 19. januar 2021 ob 02:05:20 CET je Andre Przywara napisal(a):
> The #ifdef CONFIG_xxxPWR conditionals were not working as expected, as
> string Kconfig symbols are always "defined" from the preprocessor's
> perspective. This lead to unnecessary calls to the GPIO routines, but
> also always added a half a second delay to wait for a SATA disk to power
> up. Many thanks to Peter for pointing this out!
> 
> Fix this by properly comparing the Kconfig symbols against the empty
> string. strcmp() would be nicer for this, but GCC does not optimise this
> away, probably due to our standalone compiler switches.
> 
> Reported-by: Peter Robinson <pbrobinson@gmail.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Nice find!

Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Best regards,
Jernej
diff mbox series

Patch

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 4f058952b5b..a0b5778b3bc 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -265,18 +265,28 @@  int board_init(void)
 	if (ret)
 		return ret;
 
-#ifdef CONFIG_SATAPWR
-	satapwr_pin = sunxi_name_to_gpio(CONFIG_SATAPWR);
-	gpio_request(satapwr_pin, "satapwr");
-	gpio_direction_output(satapwr_pin, 1);
-	/* Give attached sata device time to power-up to avoid link timeouts */
-	mdelay(500);
-#endif
-#ifdef CONFIG_MACPWR
-	macpwr_pin = sunxi_name_to_gpio(CONFIG_MACPWR);
-	gpio_request(macpwr_pin, "macpwr");
-	gpio_direction_output(macpwr_pin, 1);
-#endif
+	/* strcmp() would look better, but doesn't get optimised away. */
+	if (CONFIG_SATAPWR[0]) {
+		satapwr_pin = sunxi_name_to_gpio(CONFIG_SATAPWR);
+		if (satapwr_pin >= 0) {
+			gpio_request(satapwr_pin, "satapwr");
+			gpio_direction_output(satapwr_pin, 1);
+
+			/*
+			 * Give the attached SATA device time to power-up
+			 * to avoid link timeouts
+			 */
+			mdelay(500);
+		}
+	}
+
+	if (CONFIG_MACPWR[0]) {
+		macpwr_pin = sunxi_name_to_gpio(CONFIG_MACPWR);
+		if (macpwr_pin >= 0) {
+			gpio_request(macpwr_pin, "macpwr");
+			gpio_direction_output(macpwr_pin, 1);
+		}
+	}
 
 #ifdef CONFIG_DM_I2C
 	/*