lkml.org 
[lkml]   [2024]   [Apr]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller
From


On 2024/4/19 05:03, neil.armstrong@linaro.org wrote:
> [ EXTERNAL EMAIL ]
>
> On 17/04/2024 21:08, Krzysztof Kozlowski wrote:
>> On 16/04/2024 01:30, neil.armstrong@linaro.org wrote:
>>> On 15/04/2024 12:31, Kelvin Zhang wrote:
>>>>
>>>> On 2024/4/13 02:03, Krzysztof Kozlowski wrote:
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> On 12/04/2024 19:57, Krzysztof Kozlowski wrote:
>>>>>> On 12/04/2024 15:12, Neil Armstrong wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 29/03/2024 20:39, Krzysztof Kozlowski wrote:
>>>>>>>> On 29/03/2024 10:17, Kelvin Zhang via B4 Relay wrote:
>>>>>>>>> From: Zelong Dong <zelong.dong@amlogic.com>
>>>>>>>>>
>>>>>>>>> Add a new compatible and the related header file
>>>>>>>>> for Amlogic T7 Reset Controller.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
>>>>>>>>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>>>>>>>>> ---
>>>>>>>>>     .../bindings/reset/amlogic,meson-reset.yaml        |   1 +
>>>>>>>>>     include/dt-bindings/reset/amlogic,t7-reset.h       | 197
>>>>>>>>> +++++++++++++++++++++
>>>>>>>>>     2 files changed, 198 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml b/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
>>>>>>>>> index f0c6c0df0ce3..fefe343e5afe 100644
>>>>>>>>> ---
>>>>>>>>> a/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
>>>>>>>>> +++
>>>>>>>>> b/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
>>>>>>>>> @@ -19,6 +19,7 @@ properties:
>>>>>>>>>           - amlogic,meson-a1-reset # Reset Controller on A1 and
>>>>>>>>> compatible SoCs
>>>>>>>>>           - amlogic,meson-s4-reset # Reset Controller on S4 and
>>>>>>>>> compatible SoCs
>>>>>>>>>           - amlogic,c3-reset # Reset Controller on C3 and
>>>>>>>>> compatible SoCs
>>>>>>>>> +      - amlogic,t7-reset # Reset Controller on T7 and
>>>>>>>>> compatible SoCs
>>>>>>>>>
>>>>>>>>
>>>>>>>> If there is going to be any resend, please drop the comment.
>>>>>>>> It's not
>>>>>>>> really helpful and makes it trickier to read.
>>>>>>>>
>>>>>>>>>       reg:
>>>>>>>>>         maxItems: 1
>>>>>>>>> diff --git a/include/dt-bindings/reset/amlogic,t7-reset.h
>>>>>>>>> b/include/dt-bindings/reset/amlogic,t7-reset.h
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000000..ca4a832eeeec
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/include/dt-bindings/reset/amlogic,t7-reset.h
>>>>>>>>> @@ -0,0 +1,197 @@
>>>>>>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
>>>>>>>>> +/*
>>>>>>>>> + * Copyright (c) 2024 Amlogic, Inc. All rights reserved.
>>>>>>>>> + */
>>>>>>>>> +
>>>>>>>>> +#ifndef _DT_BINDINGS_AMLOGIC_T7_RESET_H
>>>>>>>>> +#define _DT_BINDINGS_AMLOGIC_T7_RESET_H
>>>>>>>>> +
>>>>>>>>> +/* RESET0 */
>>>>>>>>> +/*                                        0-3     */
>>>>>>>>
>>>>>>>> I assume this matches existing drivers which do not use IDs but
>>>>>>>> map the
>>>>>>>> binding to hardware value? I remember we talked about changing
>>>>>>>> it, so if
>>>>>>>> something happened about this and it could be changed: please
>>>>>>>> change.
>>>>>>>
>>>>>>> I'm not aware of such discussion, and I don't really see the
>>>>>>> issue...
>>>>>>> thoses are IDs, and yes they match the Hardware offsets, and ?
>>>>>>
>>>>>> Bindings are not for hardware offsets/values/addresses. It's just
>>>>>> not a
>>>>>> binding.
>>>>>>
>>>>>> I quickly looked at your driver patch and it confirms: not a binding.
>>>>>> Binding constant is used by the driver and DTS consumer.
>>>>>>
>>>>>> I am really sure we had this talk in the past, but could be I think
>>>>>> about different platform. Since this is not a binding, I do not think
>>>>>> claiming there is any ABI here is reasonable. Feel free to store them
>>>>>> with other hardware values, like in DTS headers etc. We already
>>>>>> moved to
>>>>>> DTS headers several such "non-binding" constants.
>>>>>
>>>>> Un-acked.
>>>>>
>>>>> I looked at my archives and we did talk about it and you were CCed:
>>>>>
>>>>> https://lore.kernel.org/linux-devicetree/c088e01c-0714-82be-8347-6140daf56640@linaro.org/
>>>>> simple-reset is an exception.
>>>>>
>>>>> So to recap:
>>>>> That's not a binding. Don't add some real values to binding headers
>>>>> because it is not a binding then.
>>>
>>> So what's exactly a binding then?
>>
>> Binding headers is interface needed (necessary) between implementation
>> (like Linux drivers) and DTS.
>>
>>> random linear numbers that means nothing can be a binding
>>> but registers numbers can't be ? why ? I still don't understand, why
>>> this suddenly gets problematic ?
>>
>> There is no interface here. Drivers don't use them. It's not "suddenly"
>> problematic, I commented on this year or two years ago and we also
>> started moving such header-abusers out of bindings.
>>
>>>
>>>>>
>>>>> https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/
>>>>> https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/
>>>>> https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/
>>>>> https://lore.kernel.org/linux-devicetree/418c5f0c-5279-41f5-3705-345ec9a97ea2@linaro.org/
>>>>> https://lore.kernel.org/all/201401111415.29395.arnd@arndb.de/
>>>>>
>>>> Got it. Will delete amlogic,t7-reset.h and use the hardware numbers
>>>> directly in the DT. >
>>>> Hi Neil,
>>>> As you know, Amlogic reset controller is divided into several
>>>> groups: reset0, reset1, ..., resetN. I'd like to discuss the
>>>> rationality of splitting the one device node of reset controller
>>>> into device nodes according to the groups. Then we can use the bit
>>>> number within the 'resets' property.
>>>> reset0: reset-controller@2000 {
>>>> ...
>>>> };
>>>>
>>>> reset1: reset-controller@2004 {
>>>> ...
>>>> };
>>>> ...
>>>>
>>>> What do you think?
>>>
>>> No since you'll basically add a node per register, you need to add a
>>> node for the while reset HW function, another
>>> solution would be to split the phandle arguments in 2, the first
>>> first would be the reset bank, and the second one
>>> the reset line for the bank.
>>>
>>> But still it's a regression in readability to drop the macros, until
>>> gpios or pins the reset number doesn't mean anything per se.
>>
>>
>> What stops you from putting the header in the DTS? Just like others are
>> doing?
>
> Ok so now I understand, Kelvin just keep the header but move it in
> arch/arm64/boot/dts/amlogic along the DT patch and drop it from the
> bindings patch.
>
Will do.
Thanks!

> Thanks,
> Neil
>
>>
>> Best regards,
>> Krzysztof
>>
>

\
 
 \ /
  Last update: 2024-05-27 16:46    [W:0.073 / U:1.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site