Re: [linux-sunxi] [PATCH 1/5] pinctrl: sunxi: refactor pinctrl choice selecting for ARM64

[Chen-Yu Tsai]

On Wed, Mar 1, 2017 at 2:55 AM, Icenowy Zheng <icenowy@xxxxxxxx> wrote:
>
>
> 01.03.2017, 02:15, "Andre Przywara" <andre.przywara@xxxxxxx>:
>> 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@xxxxxxxx>
>>
>> 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@xxxxxxx>
>>
>> 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