Re: [PATCH v2 06/23] sunxi: Skip non-DM UART pin setup when PINCTRL=y

Re: [PATCH v2 06/23] sunxi: Skip non-DM UART pin setup when PINCTRL=y

[Karen]

awm16899@gmail.com

從我的iPhone傳送 VIP

Re: [PATCH v2 06/23] sunxi: Skip non-DM UART pin setup when PINCTRL=y

[Samuel Holland]

On 3/31/22 6:19 PM, Andre Przywara wrote:
> On Thu, 17 Mar 2022 22:54:03 -0500
> Samuel Holland <samuel@sholland.org> wrote:
> 
> Hi Samuel,
> 
>> When a pinctrl driver is available, it will take care of setting up
>> these pins. However, for now this code is still needed in SPL.
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> (no changes since v1)
>>
>>  arch/arm/mach-sunxi/board.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
>> index 0071de19ff..32e2477ae7 100644
>> --- a/arch/arm/mach-sunxi/board.c
>> +++ b/arch/arm/mach-sunxi/board.c
>> @@ -79,6 +79,7 @@ ulong board_get_usable_ram_top(ulong total_size)
>>  static int gpio_init(void)
>>  {
>>  	__maybe_unused uint val;
>> +#if !CONFIG_IS_ENABLED(PINCTRL)
> 
> So first this looks somewhat redundant, since the whole function (and
> its caller) is already wrapped in #ifdef CONFIG_SPL_BUILD. Unless you
> plan to have DM_PINCTRL for the RISC-V SPL?

Yes, I do. But that is not too relevant anyway, because this code is in
arch/arm. So I am okay with dropping the patch.

Regards,
Samuel

> But more importantly this function is already a nested #ifdef
> nightmare. I experimented with:
> 	if (CONFIG_IS_ENABLED(PINCTRL))
> 		return 0;
> and the toolchain garbage collection did the rest. This naive
> version breaks H6, AFAICS, but with a bit of refactoring this could be
> solved. But this is probably something for an independent cleanup patch.
> 
> So: if you agree to the SPL == !PINCTRL redundancy, please drop this
> patch. If you want to keep the extra check, please at least add a
> comment after the #endif, and I will look at cleaning this up
> separately.
> 
> Cheers,
> Andre
> 
> 
>>  #if CONFIG_CONS_INDEX == 1 && defined(CONFIG_UART0_PORT_F)
>>  #if defined(CONFIG_MACH_SUN4I) || \
>>      defined(CONFIG_MACH_SUN7I) || \
>> @@ -166,6 +167,7 @@ static int gpio_init(void)
>>  #else
>>  #error Unsupported console port number. Please fix pin mux settings in board.c
>>  #endif
>> +#endif
>>  
>>  #ifdef CONFIG_SUN50I_GEN_H6
>>  	/* Update PIO power bias configuration by copy hardware detected value */
> 

Re: [PATCH v2 06/23] sunxi: Skip non-DM UART pin setup when PINCTRL=y

[Andre Przywara]

On Thu, 17 Mar 2022 22:54:03 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi Samuel,

> When a pinctrl driver is available, it will take care of setting up
> these pins. However, for now this code is still needed in SPL.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> (no changes since v1)
> 
>  arch/arm/mach-sunxi/board.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> index 0071de19ff..32e2477ae7 100644
> --- a/arch/arm/mach-sunxi/board.c
> +++ b/arch/arm/mach-sunxi/board.c
> @@ -79,6 +79,7 @@ ulong board_get_usable_ram_top(ulong total_size)
>  static int gpio_init(void)
>  {
>  	__maybe_unused uint val;
> +#if !CONFIG_IS_ENABLED(PINCTRL)

So first this looks somewhat redundant, since the whole function (and
its caller) is already wrapped in #ifdef CONFIG_SPL_BUILD. Unless you
plan to have DM_PINCTRL for the RISC-V SPL?

But more importantly this function is already a nested #ifdef
nightmare. I experimented with:
	if (CONFIG_IS_ENABLED(PINCTRL))
		return 0;
and the toolchain garbage collection did the rest. This naive
version breaks H6, AFAICS, but with a bit of refactoring this could be
solved. But this is probably something for an independent cleanup patch.

So: if you agree to the SPL == !PINCTRL redundancy, please drop this
patch. If you want to keep the extra check, please at least add a
comment after the #endif, and I will look at cleaning this up
separately.

Cheers,
Andre


>  #if CONFIG_CONS_INDEX == 1 && defined(CONFIG_UART0_PORT_F)
>  #if defined(CONFIG_MACH_SUN4I) || \
>      defined(CONFIG_MACH_SUN7I) || \
> @@ -166,6 +167,7 @@ static int gpio_init(void)
>  #else
>  #error Unsupported console port number. Please fix pin mux settings in board.c
>  #endif
> +#endif
>  
>  #ifdef CONFIG_SUN50I_GEN_H6
>  	/* Update PIO power bias configuration by copy hardware detected value */

[PATCH v2 06/23] sunxi: Skip non-DM UART pin setup when PINCTRL=y

[Samuel Holland]

When a pinctrl driver is available, it will take care of setting up
these pins. However, for now this code is still needed in SPL.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

(no changes since v1)

 arch/arm/mach-sunxi/board.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
index 0071de19ff..32e2477ae7 100644
--- a/arch/arm/mach-sunxi/board.c
+++ b/arch/arm/mach-sunxi/board.c
@@ -79,6 +79,7 @@ ulong board_get_usable_ram_top(ulong total_size)
 static int gpio_init(void)
 {
 	__maybe_unused uint val;
+#if !CONFIG_IS_ENABLED(PINCTRL)
 #if CONFIG_CONS_INDEX == 1 && defined(CONFIG_UART0_PORT_F)
 #if defined(CONFIG_MACH_SUN4I) || \
     defined(CONFIG_MACH_SUN7I) || \
@@ -166,6 +167,7 @@ static int gpio_init(void)
 #else
 #error Unsupported console port number. Please fix pin mux settings in board.c
 #endif
+#endif
 
 #ifdef CONFIG_SUN50I_GEN_H6
 	/* Update PIO power bias configuration by copy hardware detected value */
-- 
2.33.1