lkml.org 
[lkml]   [2017]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [linux-sunxi] [PATCH 1/5] pinctrl: sunxi: refactor pinctrl choice selecting for ARM64
On Wed, Mar 1, 2017 at 2:55 AM, Icenowy Zheng <icenowy@aosc.xyz> wrote:
>
>
> 01.03.2017, 02:15, "Andre Przywara" <andre.przywara@arm.com>:
>> Hi Icenowy,
>>
>> (first thing: could you create your series with --cover-letter and fill
>> this in? There you could explain what this series is about and also
>> state things like dependencies from other patches and the commit that
>> you based that on.)
>>
>> On 28/02/17 17:24, Icenowy Zheng wrote:
>>> ARM64 Allwinner SoCs used to have every pinctrl driver selected in
>>> ARCH_SUNXI. Change this to make their default value to (ARM64 &&
>>> ARCH_SUNXI).
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>>
>> Overall this is a nice solution. Thanks for posting this.
>>
>>> ---
>>> drivers/pinctrl/sunxi/Kconfig | 9 +++++----
>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/pinctrl/sunxi/Kconfig b/drivers/pinctrl/sunxi/Kconfig
>>> index 816015cf7053..92325736d953 100644
>>> --- a/drivers/pinctrl/sunxi/Kconfig
>>> +++ b/drivers/pinctrl/sunxi/Kconfig
>>> @@ -48,8 +48,9 @@ config PINCTRL_SUN8I_H3
>>> select PINCTRL_SUNXI
>>>
>>> config PINCTRL_SUN8I_H3_R
>>> - def_bool MACH_SUN8I
>>> - select PINCTRL_SUNXI_COMMON
>>> + def_bool MACH_SUN8I || (ARM64 && ARCH_SUNXI)
>>> + depends on RESET_CONTROLLER
>>
>> So this looks a bit odd. I take it this is for the extra reset register
>> in the PRCM block.
>> 1) I don't think this belongs into this patch. If this has been
>> forgotten in the past, please make an extra patch for this. It's cheap
>> to do so ;-)
>> 2) Is this really a "depends on"? Shouldn't this be a select? With
>> sunxi_ng clocks we don't need the reset controller for the normal
>> peripherals anymore, so this option might not be selected by default in
>> the future. And having this "depends on" leads to the PINCTRL_ option
>> being hidden if RESET_CONTROLLER isn't selected.
>> I think this bites us already in arm64, where ARCH_HAS_RESET_CONTROLLER
>> is not enabled for ARCH_SUNXI.
>
> PINCTRL_ options are always hidden now, Maxime, do you
> want to change that?
>
> We should enable ARCH_HAS_RESET_CONTROLLER for
> ARCH_SUNXI, as sunxi-ng ccu is a reset controller.

Agreed.

> Without RESET_CONTROLLER enabled, errors will occur
> when linking:
> ```
> drivers/built-in.o: In function `sunxi_ccu_probe':
> of_reserved_mem.c:(.text+0x16058): undefined reference to `reset_controller_register'
> ```
>
> If you don't care, I will make it an additional patch.
>
> And there's really a reset line for the R_PIO pinctrls.

Depends on is the right thing here, since this driver depends
on the reset controller subsystem to be present for any
devm_reset_control_get calls. Now if this pinctrl has a reset
line, but is missing that call, it needs to be fixed.

It seems only A23 and A31 R_PIO drivers have them.

Regards
ChenYu

>
> But you're right, this line shouldn't occur in this patch.
> I will make a more patch for it.
>
>>
>> But as the rest of the patch is fine, if you remove this line:
>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>
>> Cheers,
>> Andre.
>>
>>> + select PINCTRL_SUNXI
>>>
>>> config PINCTRL_SUN8I_V3S
>>> def_bool MACH_SUN8I
>>> @@ -65,11 +66,11 @@ config PINCTRL_SUN9I_A80_R
>>> select PINCTRL_SUNXI
>>>
>>> config PINCTRL_SUN50I_A64
>>> - bool
>>> + def_bool ARM64 && ARCH_SUNXI
>>> select PINCTRL_SUNXI
>>>
>>> config PINCTRL_SUN50I_H5
>>> - bool
>>> + def_bool ARM64 && ARCH_SUNXI
>>> select PINCTRL_SUNXI
>>>
>>> endif

\
 
 \ /
  Last update: 2017-03-01 03:57    [W:0.059 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site