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
+
 #endif  /* _DT_BINDINGS_RESET_CONTROLLER_MT8192 */
-- 
2.18.0

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.


> +
>  #endif  /* _DT_BINDINGS_RESET_CONTROLLER_MT8192 */


Best regards,
Krzysztof

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.

BRs,
Rex
> > +
> >  #endif  /* _DT_BINDINGS_RESET_CONTROLLER_MT8192 */
> 
> 
> Best regards,
> Krzysztof

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

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
----

infra_ao: syscon {
	compatible = "mediatek,mt8192-infracfg", "syscon";
	reg = <0 0x10001000 0 0x1000>;
	#clock-cells = <1>;
	#reset-cells = <1>;
};

thermal {
	...
	resets = <&infra_ao MT8192_INFRA_THERMAL_CTRL_MCU_RST>;
	...
};

BRs,
Rex

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

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.

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));
 }
-----

[1]: 
https://lore.kernel.org/all/20220427030950.23395-8-rex-bc.chen@mediatek.com/

BRs,
Rex

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

On Thu, 2022-04-28 at 15:23 +0800, Krzysztof Kozlowski wrote:
> 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

Hello Krzysztof,

The rest bits could be used in the future.
I am not sure who will use them.
I will list all 5*32 index in next version.

BRs,
Rex