Re: [PATCH V3 12/17] dt-binding: mt8192: Add infra_ao reset bit

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 28/04/2022 08:48, Rex-BC Chen wrote:
> On Thu, 2022-04-28 at 14:40 +0800, Krzysztof Kozlowski wrote:
>> On 26/04/2022 10:23, Rex-BC Chen wrote:
>>> On Mon, 2022-04-25 at 15:52 +0800, Krzysztof Kozlowski wrote:
>>>> On 25/04/2022 07:01, Rex-BC Chen wrote:
>>>>> On Sat, 2022-04-23 at 18:28 +0800, Krzysztof Kozlowski wrote:
>>>>>> On 22/04/2022 08:01, Rex-BC Chen wrote:
>>>>>>> To support reset of infra_ao, add the bit definition for
>>>>>>> thermal/PCIe/SVS.
>>>>>>>
>>>>>>> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
>>>>>>> ---
>>>>>>>  include/dt-bindings/reset/mt8192-resets.h | 10 ++++++++++
>>>>>>>  1 file changed, 10 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/dt-bindings/reset/mt8192-resets.h
>>>>>>> b/include/dt-bindings/reset/mt8192-resets.h
>>>>>>> index be9a7ca245b9..d5f3433175c1 100644
>>>>>>> --- a/include/dt-bindings/reset/mt8192-resets.h
>>>>>>> +++ b/include/dt-bindings/reset/mt8192-resets.h
>>>>>>> @@ -27,4 +27,14 @@
>>>>>>>  
>>>>>>>  #define MT8192_TOPRGU_SW_RST_NUM				
>>>>>>> 23
>>>>>>>  
>>>>>>> +/* INFRA RST0 */
>>>>>>> +#define MT8192_INFRA_RST0_LVTS_AP_RST			
>>>>>>> 	
>>>>>>> 0
>>>>>>> +/* INFRA RST2 */
>>>>>>> +#define MT8192_INFRA_RST2_PCIE_PHY_RST			
>>>>>>> 	
>>>>>>> 15
>>>>>>> +/* INFRA RST3 */
>>>>>>> +#define MT8192_INFRA_RST3_PTP_RST				
>>>>>>> 5
>>>>>>> +/* INFRA RST4 */
>>>>>>> +#define MT8192_INFRA_RST4_LVTS_MCU				
>>>>>>> 12
>>>>>>> +#define MT8192_INFRA_RST4_PCIE_TOP				
>>>>>>> 1
>>>>>>
>>>>>> These should be the IDs of reset, not some register
>>>>>> values/offsets.
>>>>>> Therefore it is expected to have them incremented by 1.
>>>>>>
>>>>>>
>>>>>
>>>>> Hello Krzysztof,
>>>>>
>>>>> This is define bit.
>>>>>
>>>>> There is serveral reset set for infra_ao while it's not serial.
>>>>> For MT8192, it's 0x120/0x130/0x140/0x150/0x730.
>>>>> We are implement #reset-cells = <2>, and we can use this reset
>>>>> drive
>>>>> more easier.
>>>>>
>>>>> For example, in dts, we can define
>>>>> infra_ao: syscon {
>>>>> 	compatible = "mediatek,mt8192-infracfg", "syscon";
>>>>>  	reg = <0 0x10001000 0 0x1000>;
>>>>>  	#clock-cells = <1>;
>>>>> 	#reset-cells = <2>;
>>>>> };
>>>>>
>>>>> thermal {
>>>>> 	...
>>>>> 	resets = <&infra_ao 0x730 MT8192_INFRA_RST4_LVTS_MCU>;
>>>>> 	...
>>>>> };
>>>>>
>>>>> If it's acceptabel, I can update all bit difinition from 0 to
>>>>> 15
>>>>> for
>>>>> all reset set.
>>>>
>>>> Bits are not acceptable, because you embed specific device
>>>> programming
>>>> model (register bits) into the binding.
>>>>
>>>> These should be IDs, so decimal numbers incremented from 0, so:
>>>> #define MT8192_INFRA_RST0_LVTS_AP_RST				
>>>> 0
>>>> #define MT8192_INFRA_RST4_LVTS_MCU				
>>>> 1
>>>> #define MT8192_INFRA_RST4_PCIE_TOP				
>>>> 2
>>>>
>>>> And what is 0x730 in your example? It does not look like ID of a
>>>> reset...
>>>>
>>>> Entire changeset look wrong from DT point of view.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> Hello Krzysztof,
>>>
>>> Got it. I will modify them to reset index.
>>> And the dts in my next version would somthing like this:
>>>
>>> ----
>>> #define MT8192_INFRA_THERMAL_CTRL_RST			0
>>> #define MT8192_INFRA_PEXTP_PHY_RST			79
>>> #define MT8192_INFRA_PTP_RST				101
>>> #define MT8192_INFRA_RST4_PCIE_TOP			129
>>> #define MT8192_INFRA_THERMAL_CTRL_MCU_RST		140
>>
>> These are still not IDs, incremented by one.
>>
>> So again from beginning:
>> 0
>> 1
>> 2
>> ...
>>
>> Do not encode hardware register bits into the binding.
>>
>> Best regards,
>> Krzysztof
> 
> Hello Krzysztof,
> 
> It's not bit definiton, and it's index for our reset.
> We have 32*5 reset bits for infra.
> But we only use these 5 index currently, I do not list all of them.

You do not have to list all of them. You can list three, e.g.:

#define MT8192_INFRA_THERMAL_CTRL_RST			0
#define MT8192_INFRA_PEXTP_PHY_RST			1
#define MT8192_INFRA_PTP_RST				2

and you will add all further later. This is how all dt-binding headers
are created.

> 
> The implementation is in [1].
> -----
> static int mtk_reset_update_set_clr(struct reset_controller_dev *rcdev,
>  	unsigned int deassert_ofs = deassert ? 0x4 : 0;
>  
>  	return regmap_write(data->regmap,
> 			    data->desc->rst_bank_ofs[id /          
>  					RST_NR_PER_BANK] +
> 			    deassert_ofs,
> 			    BIT(id % RST_NR_PER_BANK));
>  }

Exactly, you hard-code the hardware programming model - register
values/bits/whatever - in the ID, which is not correct. Additionally,
bindings are (mostly) independent of Linux implementation.


Best regards,
Krzysztof