Re: [PATCH 7/7] ARM: EXYNOS: Remove code for restart and poweroff for exynos SoCs

[Javier Martinez Canillas <javier@osg.samsung.com>]

Hello Alim,

On 10/19/2015 06:13 PM, Alim Akhtar wrote:
> Hi Javier,
> 
> On Mon, Oct 19, 2015 at 7:37 PM, Javier Martinez Canillas
> <javier@osg.samsung.com> wrote:
>> Hello Krzysztof,
>>
>> On 10/19/2015 03:28 PM, Krzysztof Kozlowski wrote:
>>> 2015-10-19 18:56 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>>> Hello,
>>>>
>>>> On 10/19/2015 09:00 AM, Krzysztof Kozlowski wrote:
>>>>> On 19.10.2015 15:03, Alim Akhtar wrote:
>>>>>> Now we can use the generic syscon-{reboot/poweroff} drivers,
>>>>>> so we don't need special handling for reboot/poweroff in
>>>>>> exynos pmu driver. This patch remove the same.
>>>>>>
>>>>>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
>>>>>> ---
>>>>>>  arch/arm/mach-exynos/pmu.c |   43 -------------------------------------------
>>>>>>  1 file changed, 43 deletions(-)
>>>>>
>>>>> I think that removal of this stuff will effectively remove the
>>>>> restart/poweroff handlers from:
>>>>> 1. Other defconfigs, like multi_v7
>>>>> 2. Custom configs.
>>>>>
>>>>
>>>> This will also break old DTBs that don't have a "syscon-poweroff" device
>>>> node that contains the necessary PMU regmap, offset and mask information.
>>>
>>> I am not sure whether this is ABI break issue. There was no compatible
>>> mentioning that "reset works" which now would be replaced. The
>>> existing PMU compatible (like samsung,exynos4412-pmu) does not mention
>>> "reset" as a feature coming with this compatible.
>>>
>>> So no ABI break.
>>>
>>>
>>
>> I deliberately didn't use the "DT ABI break" expression since as you said is
>> not part of the documented DT bindings. But what I said is that this change
>> will break old DTBs with newer kernels since reboot and power off will stop
>> working after $SUBJECT.
>>
>> I'm not a particular fan of the stable DT idea since in practice it seems to
>> do more harm than good but since that was decided, the expectation for users
>> is that booting a new kernel with an old DT should not cause any regression.
>>
>> So I think that at least a comment in the commit message is needed so if
>> there are people really using old DTs with newer kernels on Exynos boards,
>> they can know that the commit causes such an issue instead of having to
>> figure it out themselves.
>>
> Agree, will add a comment about this in commit message.
>

Thanks, with that comment in the commit message, feel free
to add my Reviewed-by tag to this patch as well.
 
>>>>
>>>>> Previously this code was always compiled in for ARCH_EXYNOS. Now it is
>>>>> not so I am thinking about selecting necessary drivers from main exynos
>>>>> Kconfig symbol. That could be tricky though, because "select" should be
>>>>> used only for non-visible symbols.
>>>>>
>>>>> Any ideas how to solve that?
>>>>>
>>>>
>>>> Is true that select should only be used for non-visible symbols but there
>>>> are others user visible symbols that are selected by ARCH_EXYNOS such as
>>>> EXYNOS_THERMAL. So I think selecting the regmap syscon reset stuff there
>>>> is a sensible option.
>>>
>>> Selecting from defconfig is not sufficient... since I do not have
>>> other idea than selecting then ovak, but Alim please check it whether
>>> it does not create circular dependencies on various configs.
>>>
>>
>> Agreed, Kconfig circular dependencies is the reason why select is avoided.
>> Fortunately now the 0-day bot analyzes even posted patches so it's possible
>> that such an issue could be found even before these patches are merged.
>>
> ok..will select it from mach-exynos/Kconfig.
> Thanks.
>

Ok, great.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America