diff mbox series

[v8,2/5] dt-bindings: soc: sophgo: Add Sophgo system control module

Message ID 598b1026fdf9989bc48e5e10d1034b37947d3b80.1705388518.git.unicorn_wang@outlook.com
State New
Headers
Series None |

Commit Message

Chen Wang Jan. 16, 2024, 7:21 a.m. UTC 
  From: Chen Wang <unicorn_wang@outlook.com>

Add documentation to describe Sophgo System Control for SG2042.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 .../soc/sophgo/sophgo,sg2042-sysctrl.yaml     | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml

Comments

Krzysztof Kozlowski Jan. 16, 2024, 10:06 a.m. UTC | #1 
On 16/01/2024 08:21, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Add documentation to describe Sophgo System Control for SG2042.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  .../soc/sophgo/sophgo,sg2042-sysctrl.yaml     | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
> new file mode 100644
> index 000000000000..7b50bb56b4cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/sophgo/sophgo,sg2042-sysctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo SG2042 SoC system control
> +
> +maintainers:
> +  - Chen Wang <unicorn_wang@outlook.com>
> +
> +description:
> +  The Sophgo system control is a registers block (SYS_CTRL), providing multiple
> +  low level platform functions like chip configuration, clock control, etc.
> +
> +properties:
> +  compatible:
> +    const: sophgo,sg2042-sysctrl
> +
> +  reg:
> +    maxItems: 1
> +
> +  clock-controller:
> +    # Child node

Drop the comment, it is obvious. It cannot be anything else.

> +    $ref: /schemas/clock/sophgo,sg2042-sysclk.yaml#
> +    type: object

Why isn't this merged here? You do not need the child node really...
unless the clock inputs are specific to that clock controller and you
will have here more devices? But where are they in such case?

> +
> +required:
> +  - compatible
> +  - reg
> +  - clock-controller
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    system-control@30010000 {

Why did you change the name? Please provide detailed changelog with
explanation of such changes.


> +        compatible = "sophgo,sg2042-sysctrl";
> +        reg = <0x30010000 0x1000>;
> +

Best regards,
Krzysztof
Chen Wang Jan. 16, 2024, 11:37 a.m. UTC | #2 
On 2024/1/16 18:06, Krzysztof Kozlowski wrote:
> On 16/01/2024 08:21, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> Add documentation to describe Sophgo System Control for SG2042.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>> ---
>>   .../soc/sophgo/sophgo,sg2042-sysctrl.yaml     | 46 +++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>> new file mode 100644
>> index 000000000000..7b50bb56b4cf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>> @@ -0,0 +1,46 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/sophgo/sophgo,sg2042-sysctrl.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sophgo SG2042 SoC system control
>> +
>> +maintainers:
>> +  - Chen Wang <unicorn_wang@outlook.com>
>> +
>> +description:
>> +  The Sophgo system control is a registers block (SYS_CTRL), providing multiple
>> +  low level platform functions like chip configuration, clock control, etc.
>> +
>> +properties:
>> +  compatible:
>> +    const: sophgo,sg2042-sysctrl
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clock-controller:
>> +    # Child node
> Drop the comment, it is obvious. It cannot be anything else.
>
>> +    $ref: /schemas/clock/sophgo,sg2042-sysclk.yaml#
>> +    type: object
> Why isn't this merged here? You do not need the child node really...
> unless the clock inputs are specific to that clock controller and you
> will have here more devices? But where are they in such case?
I don't see more devices will be included later. It should be ok to 
merge them into one.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clock-controller
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    system-control@30010000 {
> Why did you change the name? Please provide detailed changelog with
> explanation of such changes.

I changed the name due to I find the 
TRM(https://github.com/sophgo/sophgo-doc/blob/main/SG2042/TRM/source/system-control.rst) 
call it "system control",  so I changed it in v8.

Which one do you prefer? I'm not sure if there are any requirements for 
this?

>> +        compatible = "sophgo,sg2042-sysctrl";
>> +        reg = <0x30010000 0x1000>;
>> +
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Jan. 16, 2024, 12:15 p.m. UTC | #3 
On 16/01/2024 12:37, Chen Wang wrote:
> 
> On 2024/1/16 18:06, Krzysztof Kozlowski wrote:
>> On 16/01/2024 08:21, Chen Wang wrote:
>>> From: Chen Wang <unicorn_wang@outlook.com>
>>>
>>> Add documentation to describe Sophgo System Control for SG2042.
>>>
>>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>>> ---
>>>   .../soc/sophgo/sophgo,sg2042-sysctrl.yaml     | 46 +++++++++++++++++++
>>>   1 file changed, 46 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>> new file mode 100644
>>> index 000000000000..7b50bb56b4cf
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>> @@ -0,0 +1,46 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/soc/sophgo/sophgo,sg2042-sysctrl.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Sophgo SG2042 SoC system control
>>> +
>>> +maintainers:
>>> +  - Chen Wang <unicorn_wang@outlook.com>
>>> +
>>> +description:
>>> +  The Sophgo system control is a registers block (SYS_CTRL), providing multiple
>>> +  low level platform functions like chip configuration, clock control, etc.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: sophgo,sg2042-sysctrl
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clock-controller:
>>> +    # Child node
>> Drop the comment, it is obvious. It cannot be anything else.
>>
>>> +    $ref: /schemas/clock/sophgo,sg2042-sysclk.yaml#
>>> +    type: object
>> Why isn't this merged here? You do not need the child node really...
>> unless the clock inputs are specific to that clock controller and you
>> will have here more devices? But where are they in such case?
> I don't see more devices will be included later. It should be ok to 
> merge them into one.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - clock-controller
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    system-control@30010000 {
>> Why did you change the name? Please provide detailed changelog with
>> explanation of such changes.
> 
> I changed the name due to I find the 
> TRM(https://github.com/sophgo/sophgo-doc/blob/main/SG2042/TRM/source/system-control.rst) 
> call it "system control",  so I changed it in v8.
> 
> Which one do you prefer? I'm not sure if there are any requirements for 
> this?

Node names should be generic and follow common guidelines, not match
your TRM. Please use the same name all other devices use for the same class.

Best regards,
Krzysztof
Chen Wang Jan. 18, 2024, 5:29 a.m. UTC | #4 
On 2024/1/16 19:37, Chen Wang wrote:
>
> On 2024/1/16 18:06, Krzysztof Kozlowski wrote:
>> On 16/01/2024 08:21, Chen Wang wrote:
>>> From: Chen Wang <unicorn_wang@outlook.com>
>>>
>>> Add documentation to describe Sophgo System Control for SG2042.
>>>
>>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>>> ---
>>>   .../soc/sophgo/sophgo,sg2042-sysctrl.yaml     | 46 
>>> +++++++++++++++++++
>>>   1 file changed, 46 insertions(+)
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml 
>>> b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml 
>>>
>>> new file mode 100644
>>> index 000000000000..7b50bb56b4cf
>>> --- /dev/null
>>> +++ 
>>> b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>> @@ -0,0 +1,46 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: 
>>> http://devicetree.org/schemas/soc/sophgo/sophgo,sg2042-sysctrl.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Sophgo SG2042 SoC system control
>>> +
>>> +maintainers:
>>> +  - Chen Wang <unicorn_wang@outlook.com>
>>> +
>>> +description:
>>> +  The Sophgo system control is a registers block (SYS_CTRL), 
>>> providing multiple
>>> +  low level platform functions like chip configuration, clock 
>>> control, etc.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: sophgo,sg2042-sysctrl
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clock-controller:
>>> +    # Child node
>> Drop the comment, it is obvious. It cannot be anything else.
>>
>>> +    $ref: /schemas/clock/sophgo,sg2042-sysclk.yaml#
>>> +    type: object
>> Why isn't this merged here? You do not need the child node really...
>> unless the clock inputs are specific to that clock controller and you
>> will have here more devices? But where are they in such case?
> I don't see more devices will be included later. It should be ok to 
> merge them into one.

hi, Krzysztof,

After some double check, I find we will have more devices in 
system-control. For example, in the SYS_CTRL area, there is also a 
section of registers used to control the "General Purpose Interrupt". 
The pcie controller of sg2042 will use this interrupt controller which 
is defined in SYS_CTRL, we will add it in later work.

Specifically, the distribution (offset) of registers in SYS_CTRL is as 
follows:

- 0x0C0 ~ 0x0FC: for some PLL clocks :

- ......

- 0x2E0 ~ 0x30C: for General Purpose Interrupt:

- ......

- 0x368 ~ 0x3FC: For some gate clocks

So it seems that it is still necessary to keep the current child node 
method, and it will also facilitate future expansion.

What do you think, please feel free let me know.

Thanks,

Chen
Chen Wang Jan. 22, 2024, 3:41 a.m. UTC | #5 
On 2024/1/18 13:29, Chen Wang wrote:
>
> On 2024/1/16 19:37, Chen Wang wrote:
>>
>> On 2024/1/16 18:06, Krzysztof Kozlowski wrote:
>>> On 16/01/2024 08:21, Chen Wang wrote:
>>>> From: Chen Wang <unicorn_wang@outlook.com>
>>>>
>>>> Add documentation to describe Sophgo System Control for SG2042.
>>>>
>>>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>>>> ---
>>>>   .../soc/sophgo/sophgo,sg2042-sysctrl.yaml     | 46 
>>>> +++++++++++++++++++
>>>>   1 file changed, 46 insertions(+)
>>>>   create mode 100644 
>>>> Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml 
>>>>
>>>>
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml 
>>>> b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml 
>>>>
>>>> new file mode 100644
>>>> index 000000000000..7b50bb56b4cf
>>>> --- /dev/null
>>>> +++ 
>>>> b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>>> @@ -0,0 +1,46 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: 
>>>> http://devicetree.org/schemas/soc/sophgo/sophgo,sg2042-sysctrl.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Sophgo SG2042 SoC system control
>>>> +
>>>> +maintainers:
>>>> +  - Chen Wang <unicorn_wang@outlook.com>
>>>> +
>>>> +description:
>>>> +  The Sophgo system control is a registers block (SYS_CTRL), 
>>>> providing multiple
>>>> +  low level platform functions like chip configuration, clock 
>>>> control, etc.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: sophgo,sg2042-sysctrl
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  clock-controller:
>>>> +    # Child node
>>> Drop the comment, it is obvious. It cannot be anything else.
>>>
>>>> +    $ref: /schemas/clock/sophgo,sg2042-sysclk.yaml#
>>>> +    type: object
>>> Why isn't this merged here? You do not need the child node really...
>>> unless the clock inputs are specific to that clock controller and you
>>> will have here more devices? But where are they in such case?
>> I don't see more devices will be included later. It should be ok to 
>> merge them into one.
>
> hi, Krzysztof,
>
> After some double check, I find we will have more devices in 
> system-control. For example, in the SYS_CTRL area, there is also a 
> section of registers used to control the "General Purpose Interrupt". 
> The pcie controller of sg2042 will use this interrupt controller which 
> is defined in SYS_CTRL, we will add it in later work.
>
> Specifically, the distribution (offset) of registers in SYS_CTRL is as 
> follows:
>
> - 0x0C0 ~ 0x0FC: for some PLL clocks :
>
> - ......
>
> - 0x2E0 ~ 0x30C: for General Purpose Interrupt:
>
> - ......
>
> - 0x368 ~ 0x3FC: For some gate clocks
>
> So it seems that it is still necessary to keep the current child node 
> method, and it will also facilitate future expansion.
>
> What do you think, please feel free let me know.
>
> Thanks,
>
> Chen

hi, Krzysztof,

Can you please take a look if you have time, looking forward to your reply.

Thanks,

Chen


>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Krzysztof Kozlowski Jan. 22, 2024, 8:10 a.m. UTC | #6 
On 18/01/2024 06:29, Chen Wang wrote:
> 
> On 2024/1/16 19:37, Chen Wang wrote:
>>
>> On 2024/1/16 18:06, Krzysztof Kozlowski wrote:
>>> On 16/01/2024 08:21, Chen Wang wrote:
>>>> From: Chen Wang <unicorn_wang@outlook.com>
>>>>
>>>> Add documentation to describe Sophgo System Control for SG2042.
>>>>
>>>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>>>> ---
>>>>   .../soc/sophgo/sophgo,sg2042-sysctrl.yaml     | 46 
>>>> +++++++++++++++++++
>>>>   1 file changed, 46 insertions(+)
>>>>   create mode 100644 
>>>> Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>>>
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml 
>>>> b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml 
>>>>
>>>> new file mode 100644
>>>> index 000000000000..7b50bb56b4cf
>>>> --- /dev/null
>>>> +++ 
>>>> b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>>> @@ -0,0 +1,46 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: 
>>>> http://devicetree.org/schemas/soc/sophgo/sophgo,sg2042-sysctrl.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Sophgo SG2042 SoC system control
>>>> +
>>>> +maintainers:
>>>> +  - Chen Wang <unicorn_wang@outlook.com>
>>>> +
>>>> +description:
>>>> +  The Sophgo system control is a registers block (SYS_CTRL), 
>>>> providing multiple
>>>> +  low level platform functions like chip configuration, clock 
>>>> control, etc.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: sophgo,sg2042-sysctrl
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  clock-controller:
>>>> +    # Child node
>>> Drop the comment, it is obvious. It cannot be anything else.
>>>
>>>> +    $ref: /schemas/clock/sophgo,sg2042-sysclk.yaml#
>>>> +    type: object
>>> Why isn't this merged here? You do not need the child node really...
>>> unless the clock inputs are specific to that clock controller and you
>>> will have here more devices? But where are they in such case?
>> I don't see more devices will be included later. It should be ok to 
>> merge them into one.
> 
> hi, Krzysztof,
> 
> After some double check, I find we will have more devices in 
> system-control. For example, in the SYS_CTRL area, there is also a 
> section of registers used to control the "General Purpose Interrupt". 
> The pcie controller of sg2042 will use this interrupt controller which 
> is defined in SYS_CTRL, we will add it in later work.
> 

I expect then all devices to be documented.

Best regards,
Krzysztof
Chen Wang Jan. 22, 2024, 10:11 a.m. UTC | #7 
On 2024/1/22 16:10, Krzysztof Kozlowski wrote:
> On 18/01/2024 06:29, Chen Wang wrote:
>> On 2024/1/16 19:37, Chen Wang wrote:
>>> On 2024/1/16 18:06, Krzysztof Kozlowski wrote:
>>>> On 16/01/2024 08:21, Chen Wang wrote:
>>>>> From: Chen Wang <unicorn_wang@outlook.com>
>>>>>
>>>>> Add documentation to describe Sophgo System Control for SG2042.
>>>>>
>>>>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>>>>> ---
>>>>>    .../soc/sophgo/sophgo,sg2042-sysctrl.yaml     | 46
>>>>> +++++++++++++++++++
>>>>>    1 file changed, 46 insertions(+)
>>>>>    create mode 100644
>>>>> Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>>>> b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>>>>
>>>>> new file mode 100644
>>>>> index 000000000000..7b50bb56b4cf
>>>>> --- /dev/null
>>>>> +++
>>>>> b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
>>>>> @@ -0,0 +1,46 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id:
>>>>> http://devicetree.org/schemas/soc/sophgo/sophgo,sg2042-sysctrl.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Sophgo SG2042 SoC system control
>>>>> +
>>>>> +maintainers:
>>>>> +  - Chen Wang <unicorn_wang@outlook.com>
>>>>> +
>>>>> +description:
>>>>> +  The Sophgo system control is a registers block (SYS_CTRL),
>>>>> providing multiple
>>>>> +  low level platform functions like chip configuration, clock
>>>>> control, etc.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: sophgo,sg2042-sysctrl
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clock-controller:
>>>>> +    # Child node
>>>> Drop the comment, it is obvious. It cannot be anything else.
>>>>
>>>>> +    $ref: /schemas/clock/sophgo,sg2042-sysclk.yaml#
>>>>> +    type: object
>>>> Why isn't this merged here? You do not need the child node really...
>>>> unless the clock inputs are specific to that clock controller and you
>>>> will have here more devices? But where are they in such case?
>>> I don't see more devices will be included later. It should be ok to
>>> merge them into one.
>> hi, Krzysztof,
>>
>> After some double check, I find we will have more devices in
>> system-control. For example, in the SYS_CTRL area, there is also a
>> section of registers used to control the "General Purpose Interrupt".
>> The pcie controller of sg2042 will use this interrupt controller which
>> is defined in SYS_CTRL, we will add it in later work.
>>
> I expect then all devices to be documented.

hi, Krzysztof.

First, I'm very sorry for having double-checked with you for this system 
controller and child node issue, but this time I'm sure there should be 
no more child nodes except the clock and interrupt controllers, though 
there are some other registers in SYS_CTRL section, but we will not use 
them till now.

One question, when you say "to be documented", do you mean I need write 
binding/yaml files for other child node? But they exceed the scope of 
this patchset (this patchset is for clock support only). That's why I 
suggest just add clock-controller in this patchset and to add the 
interrupt controller in another patchset for pcie support. This 
mechanism should be suitable for our expansion.

Thanks,

Chen

>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Jan. 22, 2024, 12:56 p.m. UTC | #8 
On 22/01/2024 11:11, Chen Wang wrote:
>>>>>> +    $ref: /schemas/clock/sophgo,sg2042-sysclk.yaml#
>>>>>> +    type: object
>>>>> Why isn't this merged here? You do not need the child node really...
>>>>> unless the clock inputs are specific to that clock controller and you
>>>>> will have here more devices? But where are they in such case?
>>>> I don't see more devices will be included later. It should be ok to
>>>> merge them into one.
>>> hi, Krzysztof,
>>>
>>> After some double check, I find we will have more devices in
>>> system-control. For example, in the SYS_CTRL area, there is also a
>>> section of registers used to control the "General Purpose Interrupt".
>>> The pcie controller of sg2042 will use this interrupt controller which
>>> is defined in SYS_CTRL, we will add it in later work.
>>>
>> I expect then all devices to be documented.
> 
> hi, Krzysztof.
> 
> First, I'm very sorry for having double-checked with you for this system 
> controller and child node issue, but this time I'm sure there should be 
> no more child nodes except the clock and interrupt controllers, though 
> there are some other registers in SYS_CTRL section, but we will not use 
> them till now.
> 
> One question, when you say "to be documented", do you mean I need write 
> binding/yaml files for other child node? But they exceed the scope of 
> this patchset (this patchset is for clock support only). That's why I 

That's not true. The scope of this patch is to add DT binding
description for your device. If you choose any other scope, I don't
agree and I am not going to provide positive review.

> suggest just add clock-controller in this patchset and to add the 
> interrupt controller in another patchset for pcie support. This 
> mechanism should be suitable for our expansion.

How then are you going to solve the requirement: "DO attempt to make
bindings complete even"?

https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L17

Best regards,
Krzysztof
Chen Wang Jan. 23, 2024, 9:28 a.m. UTC | #9 
On 2024/1/22 20:56, Krzysztof Kozlowski wrote:
> On 22/01/2024 11:11, Chen Wang wrote:
>>>>>>> +    $ref: /schemas/clock/sophgo,sg2042-sysclk.yaml#
>>>>>>> +    type: object
>>>>>> Why isn't this merged here? You do not need the child node really...
>>>>>> unless the clock inputs are specific to that clock controller and you
>>>>>> will have here more devices? But where are they in such case?
>>>>> I don't see more devices will be included later. It should be ok to
>>>>> merge them into one.
>>>> hi, Krzysztof,
>>>>
>>>> After some double check, I find we will have more devices in
>>>> system-control. For example, in the SYS_CTRL area, there is also a
>>>> section of registers used to control the "General Purpose Interrupt".
>>>> The pcie controller of sg2042 will use this interrupt controller which
>>>> is defined in SYS_CTRL, we will add it in later work.
>>>>
>>> I expect then all devices to be documented.
>> hi, Krzysztof.
>>
>> First, I'm very sorry for having double-checked with you for this system
>> controller and child node issue, but this time I'm sure there should be
>> no more child nodes except the clock and interrupt controllers, though
>> there are some other registers in SYS_CTRL section, but we will not use
>> them till now.
>>
>> One question, when you say "to be documented", do you mean I need write
>> binding/yaml files for other child node? But they exceed the scope of
>> this patchset (this patchset is for clock support only). That's why I
> That's not true. The scope of this patch is to add DT binding
> description for your device. If you choose any other scope, I don't
> agree and I am not going to provide positive review.
>
>> suggest just add clock-controller in this patchset and to add the
>> interrupt controller in another patchset for pcie support. This
>> mechanism should be suitable for our expansion.
> How then are you going to solve the requirement: "DO attempt to make
> bindings complete even"?
>
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L17
Learned and I will try to make bindings for system-controller device 
complete, thanks.
>
> Best regards,
> Krzysztof
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
new file mode 100644
index 000000000000..7b50bb56b4cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
@@ -0,0 +1,46 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/sophgo/sophgo,sg2042-sysctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo SG2042 SoC system control
+
+maintainers:
+  - Chen Wang <unicorn_wang@outlook.com>
+
+description:
+  The Sophgo system control is a registers block (SYS_CTRL), providing multiple
+  low level platform functions like chip configuration, clock control, etc.
+
+properties:
+  compatible:
+    const: sophgo,sg2042-sysctrl
+
+  reg:
+    maxItems: 1
+
+  clock-controller:
+    # Child node
+    $ref: /schemas/clock/sophgo,sg2042-sysclk.yaml#
+    type: object
+
+required:
+  - compatible
+  - reg
+  - clock-controller
+
+additionalProperties: false
+
+examples:
+  - |
+    system-control@30010000 {
+        compatible = "sophgo,sg2042-sysctrl";
+        reg = <0x30010000 0x1000>;
+
+        clock-controller {
+          compatible = "sophgo,sg2042-sysclk";
+          clocks = <&cgi_main>, <&cgi_dpll0>, <&cgi_dpll1>;
+          #clock-cells = <1>;
+        };
+    };