Received-SPF: pass (google.com: domain of linux-kernel+bounces-144913-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <48005005-54ad-473b-826d-23b0b3f3a52c@amlogic.com>
Date: Mon, 15 Apr 2024 18:31:46 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
 neil.armstrong@linaro.org, Philipp Zabel <p.zabel@pengutronix.de>,
 Rob Herring <robh@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, Kevin Hilman <khilman@baylibre.com>,
 Jerome Brunet <jbrunet@baylibre.com>,
 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org,
 Zelong Dong <zelong.dong@amlogic.com>
References: <20240329-t7-reset-v1-0-4c6e2e68359e@amlogic.com>
 <20240329-t7-reset-v1-1-4c6e2e68359e@amlogic.com>
 <927ad2cb-3b41-4eda-b930-856be64ae9ba@linaro.org>
 <10650c59-96f2-4234-b5fd-aa8efec90e5b@linaro.org>
 <579a1569-7bba-491f-ba5e-7cfcb34ccc1f@linaro.org>
 <c7e243e3-3f61-4d63-8727-3837838bdfcc@linaro.org>
Content-Language: en-US
From: Kelvin Zhang <kelvin.zhang@amlogic.com>
In-Reply-To: <c7e243e3-3f61-4d63-8727-3837838bdfcc@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?SHZOeWI1SU1rc01OaWJFZXAvZTV1UnFsQm1OYXVBZW96Z1ZSYWdMMEpsMFJ2?=
 =?utf-8?B?MTF5OTZTRWE4YWZtMC9CaUE2VVBVVXRybGxXa0ZTSk1CZGxRWFVLTy84UXlI?=
 =?utf-8?B?R0wycFNUeHNIUUFRTk5GUE5kK0lkT1V3ejdoQVRJN1JsSE0vWndkdVlvbDha?=
 =?utf-8?B?am9uWGhwOUhXRmxNM0QzOWJWc2JaQXRUUC9SWHA1N2pmNkZQSVlyQURKNEcr?=
 =?utf-8?B?enR4TU5PRUhLQ0RNYzJpWEpDR0RVeTRCQ0YvUE04eklpVXZ1WkNDOWRjUmtG?=
 =?utf-8?B?eEFOQlJrZ2ZrRlhjYzRMRCs0cWVWK3RJUll0MFdjZ3dxMGhvaERYZzNFL0JC?=
 =?utf-8?B?bU5KSVBPWW0wSlRHZHJzSzM5aWNQOGZVZ29rNE5pMVdnS1k4eisweWhJd0th?=
 =?utf-8?B?VFc5cVFmNTlmbk9EdGlCaTcvak5oQlhNTnJMdExHaWszL2VPMWlNZ242VUFl?=
 =?utf-8?B?SHlVUUZWWUVOdXZrNVJqbUhiaE8xL3BjSmxBQVV2RHNncFpaTW5jSWpJR3hj?=
 =?utf-8?B?QXpIWlBmcmlPczlKZE55d1B1Uk54VW9BM3FFUXRBczIzSEV5SGdMWHcwelph?=
 =?utf-8?B?NzhCRkNXZEJuaUo4clFwS2QwOStnQlBlelBsNGJNWStncFoxdmJZNUpJVDBZ?=
 =?utf-8?B?L2YzeExodDJmUVY2V1Y5ZVh0TmR5ektHb2pLTjJxOG1DMmQ0Si9MOG9meXh6?=
 =?utf-8?B?NUdEdzNKb2pGVktNWFg0Vm1EUktoOXdrdTFFdGZOQ3RubTBYME5DMm1Yb3dI?=
 =?utf-8?B?cFZDMjR4Tm1DMHdxei9GdDdUUHV6dXNoWmt5OTVnVlBhSmdtNFpTUDlqYjkv?=
 =?utf-8?B?SU9vTDdQZWw3aEU0M2NKQlNnUzBiamdnV1VIcS81RGIzWFRDQmZZU1ZzMk5T?=
 =?utf-8?B?TkNxa2hwcGpxU2t4RmpBdStaM0hscW9US1hoWUY4U0JuMi9TUXRQbGVGR2Mx?=
 =?utf-8?B?OVMzbGFJRVZibXJDOC92VG9adGsrdG42SjJUVUordTk5azF6RWVUSWlzSDNP?=
 =?utf-8?B?Zm43eGUvaHhHbno1QXFaTWdRNjNGb09Ub24zY2NtV3VIVkJScVB6d21tUUpF?=
 =?utf-8?B?RFFRNllXSldlR1hLYUN2MXYwNm1YSnpZa001TDY1M0ZvNWVEVEVFdU40Njlj?=
 =?utf-8?B?UUdBV1o4eEtmSUg2c3EvR2RLdStBRXZIUlZ5RE9GNGxVZXFlYU8xVXJvNFl6?=
 =?utf-8?B?TFdMb2t0VHVHa2lranViUVBtNllhb3FwSXZyZVR5dm1VTjlpNkpwTENXa0Q3?=
 =?utf-8?B?NUpYYk1TZmZQa2xLZTJObVNXTnNDK244amJsZHdjbThITGtzb3cyVnFBemlO?=
 =?utf-8?B?UlZKQTN1NUZOOWRsVkVKSDRHNEgzZWJnZGd1UHN0cVVTazhnL1B3VlZENENT?=
 =?utf-8?B?bjE5T3p6QmYyU0ZwWUpMaWVTbXozWDFqRUtBVVphMHpkd3N3Y3ZSaDROdWw2?=
 =?utf-8?B?NE1rTXZWSGVNTmZTM2c1YVJna0pkK0VmWkJITUM3anNmb2Zjdi9rbktSdUZ3?=
 =?utf-8?B?cE9uaml0ditjbVJpWG9MQ2t4eXpiTS9BbUJ5czZGbTNkMmdpM2tBUS9EQmRX?=
 =?utf-8?B?MHFmOENQTC80aTdhY2dNUiswUjczOHppdmRDMll4TjFrelpkenFJS2czR3Q4?=
 =?utf-8?B?ckp6eEthaXZHYk42MjBTQ1Ewc1ZHcHdycjRjcnlMdDVWTWVlWHF0ZlNZQVph?=
 =?utf-8?B?dlExZENZU0pyT3p6YUV2cUJMZWlUS3k0VndQbnBvS3BmWGI4eDBJNmlqRFFB?=
 =?utf-8?B?SVNoRGxBMnZaUTRpSHVUN3VlQmtxKzc2djVoeVJaSll2K1V6Z3VnN1hTTlc2?=
 =?utf-8?B?dnoyRVhNL3MyR3dGNzhoUGk1eDl4Z0VnOWVTQW4yWlp6eXdHL01QWWNQSklL?=
 =?utf-8?B?bHdPQllaQ0ZzbkhoU0xDTkRPSGpCWUdVdXNmWWRFUHRuOUtSamRSWEVnM04w?=
 =?utf-8?B?VDJzcitqNmlwSUVSM1FQT2kza0VSUEFTa2lJM0xoMjl3YmlUaXZ6LzFQeDk1?=
 =?utf-8?B?TXNvampiNzg4NFJYL2htaVpjSEVocW00ZXdCcTNNKy9wbzAwdVZnWVdsKzRJ?=
 =?utf-8?B?c1E1bVZtaTFGaXdnOC9iN3pYZjRCUGMwQnV1cnoyeWVlNFRlNGFhRllneUgy?=
 =?utf-8?B?RXZhdU5nWTFVdVlIV1labGpYbHg2VDVuMExXMGpnNzBjWHNOYld1Uk1mVUpB?=
 =?utf-8?B?TXc9PQ==?=
X-OriginatorOrg: amlogic.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 85b34fca-25ba-44c6-83d0-08dc5d3746f0
X-MS-Exchange-CrossTenant-AuthSource: JH0PR03MB7384.apcprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Apr 2024 10:31:57.6081
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0df2add9-25ca-4b3a-acb4-c99ddf0b1114
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8Xr+vNb7UcoR4liksWm7yxw9Et3odT1e6SwBsCaE/wKaIzPzUXkqOh7h87fgbZgQ+r1EE+ffa5T0T2A2CPwoKT5SO3ZEtoBS8cO7Euk3gCs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SEZPR03MB6958


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.
> 
> 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?
Thanks!
> 
> Best regards,
> Krzysztof
>