diff mbox series

[v6,04/12] dt-bindings: reset: nuvoton: add binding for ma35d1 IP reset control

Message ID 20230328021912.177301-5-ychuang570808@gmail.com
State New
Headers
Series Introduce Nuvoton ma35d1 SoC |

Commit Message

Jacky Huang March 28, 2023, 2:19 a.m. UTC 
  From: Jacky Huang <ychuang3@nuvoton.com>

Add the dt-bindings header for Nuvoton ma35d1, that gets shared
between the reset controller and reset references in the dts.
Add documentation to describe nuvoton ma35d1 reset driver.

Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
---
 .../bindings/reset/nuvoton,ma35d1-reset.yaml  |  44 +++++++
 .../dt-bindings/reset/nuvoton,ma35d1-reset.h  | 108 ++++++++++++++++++
 2 files changed, 152 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
 create mode 100644 include/dt-bindings/reset/nuvoton,ma35d1-reset.h

Comments

Stephen Boyd March 28, 2023, 5:48 p.m. UTC | #1 
Quoting Jacky Huang (2023-03-27 19:19:04)
> +description:
> +  The system reset controller can be used to reset various peripheral
> +  controllers in MA35D1 SoC.
> +
> +properties:
> +  compatible:
> +    const: nuvoton,ma35d1-reset
> +
> +  '#reset-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - '#reset-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  # system reset controller node:
> +  - |
> +
> +    system-management@40460000 {
> +        compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
> +        reg = <0x40460000 0x200>;
> +
> +        reset-controller {
> +            compatible = "nuvoton,ma35d1-reset";
> +            #reset-cells = <1>;
> +        };

This is prescribing driver details to the system-management node. The
reset-controller node should be removed, the #reset-cells moved up one
level, and the "nuvoton,ma35d1-sys" should match a driver that registers
an auxiliary device for reset functionality. Is anything besides child
nodes like 'reset-controller' using the syscon@40460000?
Krzysztof Kozlowski March 29, 2023, 8:17 a.m. UTC | #2 
On 28/03/2023 04:19, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>

Subject: drop second/last, redundant "bindings". The "dt-bindings"
prefix is already stating that these are bindings.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

> 
> Add the dt-bindings header for Nuvoton ma35d1, that gets shared
> between the reset controller and reset references in the dts.
> Add documentation to describe nuvoton ma35d1 reset driver.
> 
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> ---
>  .../bindings/reset/nuvoton,ma35d1-reset.yaml  |  44 +++++++
>  .../dt-bindings/reset/nuvoton,ma35d1-reset.h  | 108 ++++++++++++++++++
>  2 files changed, 152 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>  create mode 100644 include/dt-bindings/reset/nuvoton,ma35d1-reset.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
> new file mode 100644
> index 000000000000..342717257e5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/nuvoton,ma35d1-reset.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton MA35D1 Reset Controller
> +
> +maintainers:
> +  - Chi-Fang Li <cfli0@nuvoton.com>
> +  - Jacky Huang <ychuang3@nuvoton.com>
> +
> +description:
> +  The system reset controller can be used to reset various peripheral
> +  controllers in MA35D1 SoC.
> +
> +properties:
> +  compatible:
> +    const: nuvoton,ma35d1-reset
> +
> +  '#reset-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - '#reset-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  # system reset controller node:
> +  - |
> +
> +    system-management@40460000 {
> +        compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
> +        reg = <0x40460000 0x200>;
> +
> +        reset-controller {
> +            compatible = "nuvoton,ma35d1-reset";
> +            #reset-cells = <1>;

You do not take any resources here, thus this should be rather part of
the parent node.

> +        };
> +    };
> +...
> +
> diff --git a/include/dt-bindings/reset/nuvoton,ma35d1-reset.h b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
> new file mode 100644
> index 000000000000..f3088bc0afec
> --- /dev/null
> +++ b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause) */

Weird license, why 2.0+?

You already got here comment about license in previous version of this
patch.


> +/*
> + * Copyright (C) 2023 Nuvoton Technologies.
> + * Author: Chi-Fen Li <cfli0@nuvoton.com>
> + *
> + * Device Tree binding constants for MA35D1 reset controller.
> + */
> +
> +#ifndef __DT_BINDINGS_RESET_MA35D1_H
> +#define __DT_BINDINGS_RESET_MA35D1_H
> +
> +#define MA35D1_RESET_CHIP	0
> +#define MA35D1_RESET_CA35CR0	1
> +#define MA35D1_RESET_CA35CR1	2
> +#define MA35D1_RESET_CM4	3
> +#define MA35D1_RESET_PDMA0	4
> +#define MA35D1_RESET_PDMA1	5
> +#define MA35D1_RESET_PDMA2	6
> +#define MA35D1_RESET_PDMA3	7
> +#define MA35D1_RESET_DISP	9
> +#define MA35D1_RESET_VCAP0	10
> +#define MA35D1_RESET_VCAP1	11
> +#define MA35D1_RESET_GFX	12
> +#define MA35D1_RESET_VDEC	13
> +#define MA35D1_RESET_WHC0	14
> +#define MA35D1_RESET_WHC1	15
> +#define MA35D1_RESET_GMAC0	16
> +#define MA35D1_RESET_GMAC1	17
> +#define MA35D1_RESET_HWSEM	18
> +#define MA35D1_RESET_EBI	19
> +#define MA35D1_RESET_HSUSBH0	20
> +#define MA35D1_RESET_HSUSBH1	21
> +#define MA35D1_RESET_HSUSBD	22
> +#define MA35D1_RESET_USBHL	23
> +#define MA35D1_RESET_SDH0	24
> +#define MA35D1_RESET_SDH1	25
> +#define MA35D1_RESET_NAND	26
> +#define MA35D1_RESET_GPIO	27
> +#define MA35D1_RESET_MCTLP	28
> +#define MA35D1_RESET_MCTLC	29
> +#define MA35D1_RESET_DDRPUB	30
> +#define MA35D1_RESET_TMR0	34
> +#define MA35D1_RESET_TMR1	35
> +#define MA35D1_RESET_TMR2	36
> +#define MA35D1_RESET_TMR3	37
> +#define MA35D1_RESET_I2C0	40
> +#define MA35D1_RESET_I2C1	41
> +#define MA35D1_RESET_I2C2	42
> +#define MA35D1_RESET_I2C3	43
> +#define MA35D1_RESET_QSPI0	44
> +#define MA35D1_RESET_SPI0	45
> +#define MA35D1_RESET_SPI1	46
> +#define MA35D1_RESET_SPI2	47
> +#define MA35D1_RESET_UART0	48
> +#define MA35D1_RESET_UART1	49
> +#define MA35D1_RESET_UART2	50
> +#define MA35D1_RESET_UAER3	51
> +#define MA35D1_RESET_UART4	52
> +#define MA35D1_RESET_UART5	53
> +#define MA35D1_RESET_UART6	54
> +#define MA35D1_RESET_UART7	55
> +#define MA35D1_RESET_CANFD0	56
> +#define MA35D1_RESET_CANFD1	57
> +#define MA35D1_RESET_EADC0	60

Why do you have gaps here? These should be IDs, not register offsets.

Best regards,
Krzysztof
Jacky Huang March 29, 2023, 8:53 a.m. UTC | #3 
Dear Stephen,


On 2023/3/29 上午 01:48, Stephen Boyd wrote:
> Quoting Jacky Huang (2023-03-27 19:19:04)
>> +description:
>> +  The system reset controller can be used to reset various peripheral
>> +  controllers in MA35D1 SoC.
>> +
>> +properties:
>> +  compatible:
>> +    const: nuvoton,ma35d1-reset
>> +
>> +  '#reset-cells':
>> +    const: 1
>> +
>> +required:
>> +  - compatible
>> +  - '#reset-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  # system reset controller node:
>> +  - |
>> +
>> +    system-management@40460000 {
>> +        compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
>> +        reg = <0x40460000 0x200>;
>> +
>> +        reset-controller {
>> +            compatible = "nuvoton,ma35d1-reset";
>> +            #reset-cells = <1>;
>> +        };
> This is prescribing driver details to the system-management node. The
> reset-controller node should be removed, the #reset-cells moved up one
> level, and the "nuvoton,ma35d1-sys" should match a driver that registers
> an auxiliary device for reset functionality. Is anything besides child
> nodes like 'reset-controller' using the syscon@40460000?

OK, I will modify it as:

     sys: system-management@40460000 {
         compatible = "nuvoton,ma35d1-sys", "syscon",;
         reg = <0x0 0x40460000 0x0 0x200>;
         #reset-cells = <1>;
         };
     };

And my reset driver will use compatible "nuvoton,ma35d1-sys", instead of 
"nuvoton,ma35d1-reset".
There' no other node like reset-controller.


Best regards,
Jack Huang
Jacky Huang March 29, 2023, 9:12 a.m. UTC | #4 
Dear Krzysztof,


On 2023/3/29 下午 04:17, Krzysztof Kozlowski wrote:
> On 28/03/2023 04:19, Jacky Huang wrote:
>> From: Jacky Huang <ychuang3@nuvoton.com>
> Subject: drop second/last, redundant "bindings". The "dt-bindings"
> prefix is already stating that these are bindings.
>
> This is a friendly reminder during the review process.
>
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
>
> Thank you.
>
>> Add the dt-bindings header for Nuvoton ma35d1, that gets shared
>> between the reset controller and reset references in the dts.
>> Add documentation to describe nuvoton ma35d1 reset driver.
>>
>> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
>> ---
>>   .../bindings/reset/nuvoton,ma35d1-reset.yaml  |  44 +++++++
>>   .../dt-bindings/reset/nuvoton,ma35d1-reset.h  | 108 ++++++++++++++++++
>>   2 files changed, 152 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>>   create mode 100644 include/dt-bindings/reset/nuvoton,ma35d1-reset.h
>>
>> diff --git a/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>> new file mode 100644
>> index 000000000000..342717257e5c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>> @@ -0,0 +1,44 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/reset/nuvoton,ma35d1-reset.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Nuvoton MA35D1 Reset Controller
>> +
>> +maintainers:
>> +  - Chi-Fang Li <cfli0@nuvoton.com>
>> +  - Jacky Huang <ychuang3@nuvoton.com>
>> +
>> +description:
>> +  The system reset controller can be used to reset various peripheral
>> +  controllers in MA35D1 SoC.
>> +
>> +properties:
>> +  compatible:
>> +    const: nuvoton,ma35d1-reset
>> +
>> +  '#reset-cells':
>> +    const: 1
>> +
>> +required:
>> +  - compatible
>> +  - '#reset-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  # system reset controller node:
>> +  - |
>> +
>> +    system-management@40460000 {
>> +        compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
>> +        reg = <0x40460000 0x200>;
>> +
>> +        reset-controller {
>> +            compatible = "nuvoton,ma35d1-reset";
>> +            #reset-cells = <1>;
> You do not take any resources here, thus this should be rather part of
> the parent node.

I will modify it as:
     sys: system-management@40460000 {
         compatible = "nuvoton,ma35d1-sys", "syscon",;
         reg = <0x0 0x40460000 0x0 0x200>;
         #reset-cells = <1>;
         };
     };


>> +        };
>> +    };
>> +...
>> +
>> diff --git a/include/dt-bindings/reset/nuvoton,ma35d1-reset.h b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
>> new file mode 100644
>> index 000000000000..f3088bc0afec
>> --- /dev/null
>> +++ b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
>> @@ -0,0 +1,108 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause) */
> Weird license, why 2.0+?
>
> You already got here comment about license in previous version of this
> patch.
>

In fact, I always be confused with the license.
It should be right if I just copy one from the same directory 
/include/dt-bindings/reset/.
But, when I find there are several versions there, I was confused and 
just select one of them.

/* SPDX-License-Identifier: GPL-2.0-only */
/* SPDX-License-Identifier: GPL-2.0+ */
/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)*/
/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */

So, should I fix the license as

/* SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) */

just remove the '+' ?

>> +/*
>> + * Copyright (C) 2023 Nuvoton Technologies.
>> + * Author: Chi-Fen Li <cfli0@nuvoton.com>
>> + *
>> + * Device Tree binding constants for MA35D1 reset controller.
>> + */
>> +
>> +#ifndef __DT_BINDINGS_RESET_MA35D1_H
>> +#define __DT_BINDINGS_RESET_MA35D1_H
>> +
>> +#define MA35D1_RESET_CHIP	0
>> +#define MA35D1_RESET_CA35CR0	1
>> +#define MA35D1_RESET_CA35CR1	2
>> +#define MA35D1_RESET_CM4	3
>> +#define MA35D1_RESET_PDMA0	4
>> +#define MA35D1_RESET_PDMA1	5
>> +#define MA35D1_RESET_PDMA2	6
>> +#define MA35D1_RESET_PDMA3	7
>> +#define MA35D1_RESET_DISP	9
>> +#define MA35D1_RESET_VCAP0	10
>> +#define MA35D1_RESET_VCAP1	11
>> +#define MA35D1_RESET_GFX	12
>> +#define MA35D1_RESET_VDEC	13
>> +#define MA35D1_RESET_WHC0	14
>> +#define MA35D1_RESET_WHC1	15
>> +#define MA35D1_RESET_GMAC0	16
>> +#define MA35D1_RESET_GMAC1	17
>> +#define MA35D1_RESET_HWSEM	18
>> +#define MA35D1_RESET_EBI	19
>> +#define MA35D1_RESET_HSUSBH0	20
>> +#define MA35D1_RESET_HSUSBH1	21
>> +#define MA35D1_RESET_HSUSBD	22
>> +#define MA35D1_RESET_USBHL	23
>> +#define MA35D1_RESET_SDH0	24
>> +#define MA35D1_RESET_SDH1	25
>> +#define MA35D1_RESET_NAND	26
>> +#define MA35D1_RESET_GPIO	27
>> +#define MA35D1_RESET_MCTLP	28
>> +#define MA35D1_RESET_MCTLC	29
>> +#define MA35D1_RESET_DDRPUB	30
>> +#define MA35D1_RESET_TMR0	34
>> +#define MA35D1_RESET_TMR1	35
>> +#define MA35D1_RESET_TMR2	36
>> +#define MA35D1_RESET_TMR3	37
>> +#define MA35D1_RESET_I2C0	40
>> +#define MA35D1_RESET_I2C1	41
>> +#define MA35D1_RESET_I2C2	42
>> +#define MA35D1_RESET_I2C3	43
>> +#define MA35D1_RESET_QSPI0	44
>> +#define MA35D1_RESET_SPI0	45
>> +#define MA35D1_RESET_SPI1	46
>> +#define MA35D1_RESET_SPI2	47
>> +#define MA35D1_RESET_UART0	48
>> +#define MA35D1_RESET_UART1	49
>> +#define MA35D1_RESET_UART2	50
>> +#define MA35D1_RESET_UAER3	51
>> +#define MA35D1_RESET_UART4	52
>> +#define MA35D1_RESET_UART5	53
>> +#define MA35D1_RESET_UART6	54
>> +#define MA35D1_RESET_UART7	55
>> +#define MA35D1_RESET_CANFD0	56
>> +#define MA35D1_RESET_CANFD1	57
>> +#define MA35D1_RESET_EADC0	60
> Why do you have gaps here? These should be IDs, not register offsets.
>
> Best regards,
> Krzysztof
>

The reset controller registers are composed of four 32-bits register.
And the ID will be translated into the corresponding bit of these register.
Each ID is mapped to a unique bit position.


Best regards,
Jacky Huang
Krzysztof Kozlowski March 29, 2023, 9:27 a.m. UTC | #5 
On 29/03/2023 11:12, Jacky Huang wrote:
>>> +        };
>>> +    };
>>> +...
>>> +
>>> diff --git a/include/dt-bindings/reset/nuvoton,ma35d1-reset.h b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
>>> new file mode 100644
>>> index 000000000000..f3088bc0afec
>>> --- /dev/null
>>> +++ b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
>>> @@ -0,0 +1,108 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause) */
>> Weird license, why 2.0+?
>>
>> You already got here comment about license in previous version of this
>> patch.
>>
> 
> In fact, I always be confused with the license.
> It should be right if I just copy one from the same directory 
> /include/dt-bindings/reset/.
> But, when I find there are several versions there, I was confused and 
> just select one of them.
> 
> /* SPDX-License-Identifier: GPL-2.0-only */
> /* SPDX-License-Identifier: GPL-2.0+ */
> /* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)*/
> /* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
> /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> 
> So, should I fix the license as
> 
> /* SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) */
> 
> just remove the '+' ?

The simplest is to use the same license as your binding, so
(GPL-2.0-only OR BSD-2-Clause) .

> 
>>> +/*
>>> + * Copyright (C) 2023 Nuvoton Technologies.
>>> + * Author: Chi-Fen Li <cfli0@nuvoton.com>
>>> + *
>>> + * Device Tree binding constants for MA35D1 reset controller.
>>> + */
>>> +
>>> +#ifndef __DT_BINDINGS_RESET_MA35D1_H
>>> +#define __DT_BINDINGS_RESET_MA35D1_H
>>> +
>>> +#define MA35D1_RESET_CHIP	0
>>> +#define MA35D1_RESET_CA35CR0	1
>>> +#define MA35D1_RESET_CA35CR1	2
>>> +#define MA35D1_RESET_CM4	3
>>> +#define MA35D1_RESET_PDMA0	4
>>> +#define MA35D1_RESET_PDMA1	5
>>> +#define MA35D1_RESET_PDMA2	6
>>> +#define MA35D1_RESET_PDMA3	7
>>> +#define MA35D1_RESET_DISP	9
>>> +#define MA35D1_RESET_VCAP0	10
>>> +#define MA35D1_RESET_VCAP1	11
>>> +#define MA35D1_RESET_GFX	12
>>> +#define MA35D1_RESET_VDEC	13
>>> +#define MA35D1_RESET_WHC0	14
>>> +#define MA35D1_RESET_WHC1	15
>>> +#define MA35D1_RESET_GMAC0	16
>>> +#define MA35D1_RESET_GMAC1	17
>>> +#define MA35D1_RESET_HWSEM	18
>>> +#define MA35D1_RESET_EBI	19
>>> +#define MA35D1_RESET_HSUSBH0	20
>>> +#define MA35D1_RESET_HSUSBH1	21
>>> +#define MA35D1_RESET_HSUSBD	22
>>> +#define MA35D1_RESET_USBHL	23
>>> +#define MA35D1_RESET_SDH0	24
>>> +#define MA35D1_RESET_SDH1	25
>>> +#define MA35D1_RESET_NAND	26
>>> +#define MA35D1_RESET_GPIO	27
>>> +#define MA35D1_RESET_MCTLP	28
>>> +#define MA35D1_RESET_MCTLC	29
>>> +#define MA35D1_RESET_DDRPUB	30
>>> +#define MA35D1_RESET_TMR0	34
>>> +#define MA35D1_RESET_TMR1	35
>>> +#define MA35D1_RESET_TMR2	36
>>> +#define MA35D1_RESET_TMR3	37
>>> +#define MA35D1_RESET_I2C0	40
>>> +#define MA35D1_RESET_I2C1	41
>>> +#define MA35D1_RESET_I2C2	42
>>> +#define MA35D1_RESET_I2C3	43
>>> +#define MA35D1_RESET_QSPI0	44
>>> +#define MA35D1_RESET_SPI0	45
>>> +#define MA35D1_RESET_SPI1	46
>>> +#define MA35D1_RESET_SPI2	47
>>> +#define MA35D1_RESET_UART0	48
>>> +#define MA35D1_RESET_UART1	49
>>> +#define MA35D1_RESET_UART2	50
>>> +#define MA35D1_RESET_UAER3	51
>>> +#define MA35D1_RESET_UART4	52
>>> +#define MA35D1_RESET_UART5	53
>>> +#define MA35D1_RESET_UART6	54
>>> +#define MA35D1_RESET_UART7	55
>>> +#define MA35D1_RESET_CANFD0	56
>>> +#define MA35D1_RESET_CANFD1	57
>>> +#define MA35D1_RESET_EADC0	60
>> Why do you have gaps here? These should be IDs, not register offsets.
>>
>> Best regards,
>> Krzysztof
>>
> 
> The reset controller registers are composed of four 32-bits register.
> And the ID will be translated into the corresponding bit of these register.
> Each ID is mapped to a unique bit position.

That's not the usual way. IDs start from 0 or 1 and gets incremented by
1, without gaps. Remember that IDs cannot change and your register bits
could have some changes (e.g. HW errata).


Best regards,
Krzysztof
Jacky Huang March 29, 2023, 9:33 a.m. UTC | #6 
Dear Krzysztof,



On 2023/3/29 下午 05:27, Krzysztof Kozlowski wrote:
> On 29/03/2023 11:12, Jacky Huang wrote:
>>>> +        };
>>>> +    };
>>>> +...
>>>> +
>>>> diff --git a/include/dt-bindings/reset/nuvoton,ma35d1-reset.h b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
>>>> new file mode 100644
>>>> index 000000000000..f3088bc0afec
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
>>>> @@ -0,0 +1,108 @@
>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause) */
>>> Weird license, why 2.0+?
>>>
>>> You already got here comment about license in previous version of this
>>> patch.
>>>
>> In fact, I always be confused with the license.
>> It should be right if I just copy one from the same directory
>> /include/dt-bindings/reset/.
>> But, when I find there are several versions there, I was confused and
>> just select one of them.
>>
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> /* SPDX-License-Identifier: GPL-2.0+ */
>> /* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)*/
>> /* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
>> /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>
>> So, should I fix the license as
>>
>> /* SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) */
>>
>> just remove the '+' ?
> The simplest is to use the same license as your binding, so
> (GPL-2.0-only OR BSD-2-Clause) .

OK, I got it. Thank you very much.

>>>> +/*
>>>> + * Copyright (C) 2023 Nuvoton Technologies.
>>>> + * Author: Chi-Fen Li <cfli0@nuvoton.com>
>>>> + *
>>>> + * Device Tree binding constants for MA35D1 reset controller.
>>>> + */
>>>> +
>>>> +#ifndef __DT_BINDINGS_RESET_MA35D1_H
>>>> +#define __DT_BINDINGS_RESET_MA35D1_H
>>>> +
>>>> +#define MA35D1_RESET_CHIP	0
>>>> +#define MA35D1_RESET_CA35CR0	1
>>>> +#define MA35D1_RESET_CA35CR1	2
>>>> +#define MA35D1_RESET_CM4	3
>>>> +#define MA35D1_RESET_PDMA0	4
>>>> +#define MA35D1_RESET_PDMA1	5
>>>> +#define MA35D1_RESET_PDMA2	6
>>>> +#define MA35D1_RESET_PDMA3	7
>>>> +#define MA35D1_RESET_DISP	9
>>>> +#define MA35D1_RESET_VCAP0	10
>>>> +#define MA35D1_RESET_VCAP1	11
>>>> +#define MA35D1_RESET_GFX	12
>>>> +#define MA35D1_RESET_VDEC	13
>>>> +#define MA35D1_RESET_WHC0	14
>>>> +#define MA35D1_RESET_WHC1	15
>>>> +#define MA35D1_RESET_GMAC0	16
>>>> +#define MA35D1_RESET_GMAC1	17
>>>> +#define MA35D1_RESET_HWSEM	18
>>>> +#define MA35D1_RESET_EBI	19
>>>> +#define MA35D1_RESET_HSUSBH0	20
>>>> +#define MA35D1_RESET_HSUSBH1	21
>>>> +#define MA35D1_RESET_HSUSBD	22
>>>> +#define MA35D1_RESET_USBHL	23
>>>> +#define MA35D1_RESET_SDH0	24
>>>> +#define MA35D1_RESET_SDH1	25
>>>> +#define MA35D1_RESET_NAND	26
>>>> +#define MA35D1_RESET_GPIO	27
>>>> +#define MA35D1_RESET_MCTLP	28
>>>> +#define MA35D1_RESET_MCTLC	29
>>>> +#define MA35D1_RESET_DDRPUB	30
>>>> +#define MA35D1_RESET_TMR0	34
>>>> +#define MA35D1_RESET_TMR1	35
>>>> +#define MA35D1_RESET_TMR2	36
>>>> +#define MA35D1_RESET_TMR3	37
>>>> +#define MA35D1_RESET_I2C0	40
>>>> +#define MA35D1_RESET_I2C1	41
>>>> +#define MA35D1_RESET_I2C2	42
>>>> +#define MA35D1_RESET_I2C3	43
>>>> +#define MA35D1_RESET_QSPI0	44
>>>> +#define MA35D1_RESET_SPI0	45
>>>> +#define MA35D1_RESET_SPI1	46
>>>> +#define MA35D1_RESET_SPI2	47
>>>> +#define MA35D1_RESET_UART0	48
>>>> +#define MA35D1_RESET_UART1	49
>>>> +#define MA35D1_RESET_UART2	50
>>>> +#define MA35D1_RESET_UAER3	51
>>>> +#define MA35D1_RESET_UART4	52
>>>> +#define MA35D1_RESET_UART5	53
>>>> +#define MA35D1_RESET_UART6	54
>>>> +#define MA35D1_RESET_UART7	55
>>>> +#define MA35D1_RESET_CANFD0	56
>>>> +#define MA35D1_RESET_CANFD1	57
>>>> +#define MA35D1_RESET_EADC0	60
>>> Why do you have gaps here? These should be IDs, not register offsets.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> The reset controller registers are composed of four 32-bits register.
>> And the ID will be translated into the corresponding bit of these register.
>> Each ID is mapped to a unique bit position.
> That's not the usual way. IDs start from 0 or 1 and gets incremented by
> 1, without gaps. Remember that IDs cannot change and your register bits
> could have some changes (e.g. HW errata).
>
>
> Best regards,
> Krzysztof
>

OK, I will modify the ID as contiguous number, and create a lookup table 
to translate
ID into bitmap.


Best Regards,
Jacky Huang
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
new file mode 100644
index 000000000000..342717257e5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
@@ -0,0 +1,44 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/nuvoton,ma35d1-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton MA35D1 Reset Controller
+
+maintainers:
+  - Chi-Fang Li <cfli0@nuvoton.com>
+  - Jacky Huang <ychuang3@nuvoton.com>
+
+description:
+  The system reset controller can be used to reset various peripheral
+  controllers in MA35D1 SoC.
+
+properties:
+  compatible:
+    const: nuvoton,ma35d1-reset
+
+  '#reset-cells':
+    const: 1
+
+required:
+  - compatible
+  - '#reset-cells'
+
+additionalProperties: false
+
+examples:
+  # system reset controller node:
+  - |
+
+    system-management@40460000 {
+        compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
+        reg = <0x40460000 0x200>;
+
+        reset-controller {
+            compatible = "nuvoton,ma35d1-reset";
+            #reset-cells = <1>;
+        };
+    };
+...
+
diff --git a/include/dt-bindings/reset/nuvoton,ma35d1-reset.h b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
new file mode 100644
index 000000000000..f3088bc0afec
--- /dev/null
+++ b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
@@ -0,0 +1,108 @@ 
+/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause) */
+/*
+ * Copyright (C) 2023 Nuvoton Technologies.
+ * Author: Chi-Fen Li <cfli0@nuvoton.com>
+ *
+ * Device Tree binding constants for MA35D1 reset controller.
+ */
+
+#ifndef __DT_BINDINGS_RESET_MA35D1_H
+#define __DT_BINDINGS_RESET_MA35D1_H
+
+#define MA35D1_RESET_CHIP	0
+#define MA35D1_RESET_CA35CR0	1
+#define MA35D1_RESET_CA35CR1	2
+#define MA35D1_RESET_CM4	3
+#define MA35D1_RESET_PDMA0	4
+#define MA35D1_RESET_PDMA1	5
+#define MA35D1_RESET_PDMA2	6
+#define MA35D1_RESET_PDMA3	7
+#define MA35D1_RESET_DISP	9
+#define MA35D1_RESET_VCAP0	10
+#define MA35D1_RESET_VCAP1	11
+#define MA35D1_RESET_GFX	12
+#define MA35D1_RESET_VDEC	13
+#define MA35D1_RESET_WHC0	14
+#define MA35D1_RESET_WHC1	15
+#define MA35D1_RESET_GMAC0	16
+#define MA35D1_RESET_GMAC1	17
+#define MA35D1_RESET_HWSEM	18
+#define MA35D1_RESET_EBI	19
+#define MA35D1_RESET_HSUSBH0	20
+#define MA35D1_RESET_HSUSBH1	21
+#define MA35D1_RESET_HSUSBD	22
+#define MA35D1_RESET_USBHL	23
+#define MA35D1_RESET_SDH0	24
+#define MA35D1_RESET_SDH1	25
+#define MA35D1_RESET_NAND	26
+#define MA35D1_RESET_GPIO	27
+#define MA35D1_RESET_MCTLP	28
+#define MA35D1_RESET_MCTLC	29
+#define MA35D1_RESET_DDRPUB	30
+#define MA35D1_RESET_TMR0	34
+#define MA35D1_RESET_TMR1	35
+#define MA35D1_RESET_TMR2	36
+#define MA35D1_RESET_TMR3	37
+#define MA35D1_RESET_I2C0	40
+#define MA35D1_RESET_I2C1	41
+#define MA35D1_RESET_I2C2	42
+#define MA35D1_RESET_I2C3	43
+#define MA35D1_RESET_QSPI0	44
+#define MA35D1_RESET_SPI0	45
+#define MA35D1_RESET_SPI1	46
+#define MA35D1_RESET_SPI2	47
+#define MA35D1_RESET_UART0	48
+#define MA35D1_RESET_UART1	49
+#define MA35D1_RESET_UART2	50
+#define MA35D1_RESET_UAER3	51
+#define MA35D1_RESET_UART4	52
+#define MA35D1_RESET_UART5	53
+#define MA35D1_RESET_UART6	54
+#define MA35D1_RESET_UART7	55
+#define MA35D1_RESET_CANFD0	56
+#define MA35D1_RESET_CANFD1	57
+#define MA35D1_RESET_EADC0	60
+#define MA35D1_RESET_I2S0	61
+#define MA35D1_RESET_SC0	64
+#define MA35D1_RESET_SC1	65
+#define MA35D1_RESET_QSPI1	68
+#define MA35D1_RESET_SPI3	70
+#define MA35D1_RESET_EPWM0	80
+#define MA35D1_RESET_EPWM1	81
+#define MA35D1_RESET_QEI0	86
+#define MA35D1_RESET_QEI1	87
+#define MA35D1_RESET_ECAP0	90
+#define MA35D1_RESET_ECAP1	91
+#define MA35D1_RESET_CANFD2	92
+#define MA35D1_RESET_ADC0	95
+#define MA35D1_RESET_TMR4	96
+#define MA35D1_RESET_TMR5	97
+#define MA35D1_RESET_TMR6	98
+#define MA35D1_RESET_TMR7	99
+#define MA35D1_RESET_TMR8	100
+#define MA35D1_RESET_TMR9	101
+#define MA35D1_RESET_TMR10	102
+#define MA35D1_RESET_TMR11	103
+#define MA35D1_RESET_UART8	104
+#define MA35D1_RESET_UART9	105
+#define MA35D1_RESET_UART10	106
+#define MA35D1_RESET_UART11	107
+#define MA35D1_RESET_UART12	108
+#define MA35D1_RESET_UART13	109
+#define MA35D1_RESET_UART14	110
+#define MA35D1_RESET_UART15	111
+#define MA35D1_RESET_UART16	112
+#define MA35D1_RESET_I2S1	113
+#define MA35D1_RESET_I2C4	114
+#define MA35D1_RESET_I2C5	115
+#define MA35D1_RESET_EPWM2	116
+#define MA35D1_RESET_ECAP2	117
+#define MA35D1_RESET_QEI2	118
+#define MA35D1_RESET_CANFD3	119
+#define MA35D1_RESET_KPI	120
+#define MA35D1_RESET_GIC	124
+#define MA35D1_RESET_SSMCC	126
+#define MA35D1_RESET_SSPCC	127
+#define MA35D1_RESET_COUNT	128
+
+#endif