LinuxLists.cc - [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On 15/02/2023 12:32, William Qiu wrote:
> Add documentation to describe StarFive System Controller Registers.
>
> Signed-off-by: William Qiu <[email protected]>
> ---

Thank you for your patch. There is something to discuss/improve.

> +properties:
> + compatible:
> + items:
> + - enum:
> + - starfive,jh7110-stg-syscon
> + - starfive,jh7110-sys-syscon
> + - starfive,jh7110-aon-syscon

Maybe keep them ordered alphabetically?

> + - const: syscon
> +
> + reg:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + syscon@10240000 {
> + compatible = "starfive,jh7110-stg-syscon", "syscon";
> + reg = <0x10240000 0x1000>;
> + };

Keep only one example. All others are the same.

Best regards,
Krzysztof

2023-02-16 10:29:37

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On Thu, Feb 16, 2023 at 11:23:00AM +0100, Krzysztof Kozlowski wrote:
> On 15/02/2023 12:32, William Qiu wrote:
> > Add documentation to describe StarFive System Controller Registers.
> >
> > Signed-off-by: William Qiu <[email protected]>
> > ---
>
> Thank you for your patch. There is something to discuss/improve.
>
> > +properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - starfive,jh7110-stg-syscon
> > + - starfive,jh7110-sys-syscon
> > + - starfive,jh7110-aon-syscon
>
> Maybe keep them ordered alphabetically?
>
> > + - const: syscon
> > +
> > + reg:
> > + maxItems: 1
> > +
> > +required:
> > + - compatible
> > + - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + syscon@10240000 {
> > + compatible = "starfive,jh7110-stg-syscon", "syscon";
> > + reg = <0x10240000 0x1000>;
> > + };
>
> Keep only one example. All others are the same.

With these fixed:
Reviewed-by: Conor Dooley <[email protected]>

@Krzysztof, I assume the location of the binding is okay with you since
you didn't object to it & I suppose this one is up to me to apply if so.

Cheers,
Conor.

Attachments:

(No filename) (1.18 kB)
signature.asc (228.00 B)
Download all attachments

2023-02-16 10:31:11

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On 2023/2/16 18:23, Krzysztof Kozlowski wrote:
> On 15/02/2023 12:32, William Qiu wrote:
>> Add documentation to describe StarFive System Controller Registers.
>>
>> Signed-off-by: William Qiu <[email protected]>
>> ---
>
> Thank you for your patch. There is something to discuss/improve.
>
>> +properties:
>> + compatible:
>> + items:
>> + - enum:
>> + - starfive,jh7110-stg-syscon
>> + - starfive,jh7110-sys-syscon
>> + - starfive,jh7110-aon-syscon
>
> Maybe keep them ordered alphabetically?
>

I'm sorting by register address, or I can keep them ordered
alphabetically，which is better?
>> + - const: syscon
>> +
>> + reg:
>> + maxItems: 1
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + syscon@10240000 {
>> + compatible = "starfive,jh7110-stg-syscon", "syscon";
>> + reg = <0x10240000 0x1000>;
>> + };
>
> Keep only one example. All others are the same.
>
Will update in next version.
Thanks for taking times to review this patch series.

Best regards
William
>
> Best regards,
> Krzysztof
>

2023-02-16 10:31:55

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On 16/02/2023 11:29, Conor Dooley wrote:
> On Thu, Feb 16, 2023 at 11:23:00AM +0100, Krzysztof Kozlowski wrote:
>> On 15/02/2023 12:32, William Qiu wrote:
>>> Add documentation to describe StarFive System Controller Registers.
>>>
>>> Signed-off-by: William Qiu <[email protected]>
>>> ---
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> +properties:
>>> + compatible:
>>> + items:
>>> + - enum:
>>> + - starfive,jh7110-stg-syscon
>>> + - starfive,jh7110-sys-syscon
>>> + - starfive,jh7110-aon-syscon
>>
>> Maybe keep them ordered alphabetically?
>>
>>> + - const: syscon
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + syscon@10240000 {
>>> + compatible = "starfive,jh7110-stg-syscon", "syscon";
>>> + reg = <0x10240000 0x1000>;
>>> + };
>>
>> Keep only one example. All others are the same.
>
> With these fixed:
> Reviewed-by: Conor Dooley <[email protected]>
>
> @Krzysztof, I assume the location of the binding is okay with you since
> you didn't object to it & I suppose this one is up to me to apply if so.

Yeah, generic sysreg devices go to soc. If their primary functions were
different (e.g. clock controller which also is syscon), then they should
go to respective directories, but it's not the case here, I think.

Best regards,
Krzysztof

2023-02-16 10:32:30

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On 16/02/2023 11:30, William Qiu wrote:
>
>
> On 2023/2/16 18:23, Krzysztof Kozlowski wrote:
>> On 15/02/2023 12:32, William Qiu wrote:
>>> Add documentation to describe StarFive System Controller Registers.
>>>
>>> Signed-off-by: William Qiu <[email protected]>
>>> ---
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> +properties:
>>> + compatible:
>>> + items:
>>> + - enum:
>>> + - starfive,jh7110-stg-syscon
>>> + - starfive,jh7110-sys-syscon
>>> + - starfive,jh7110-aon-syscon
>>
>> Maybe keep them ordered alphabetically?
>>
>
> I'm sorting by register address, or I can keep them ordered
> alphabetically，which is better?

We don't know register address here, so I propose alphabetically.

Best regards,
Krzysztof

2023-02-20 23:43:42

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
> Add documentation to describe StarFive System Controller Registers.
>
> Signed-off-by: William Qiu <[email protected]>
> ---
> .../bindings/soc/starfive/jh7110-syscon.yaml | 51 +++++++++++++++++++
> MAINTAINERS | 5 ++
> 2 files changed, 56 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>
> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> new file mode 100644
> index 000000000000..fa4d8522a454
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 SoC system controller
> +
> +maintainers:
> + - William Qiu <[email protected]>
> +
> +description: |
> + The StarFive JH7110 SoC system controller provides register information such
> + as offset, mask and shift to configure related modules such as MMC and PCIe.
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - starfive,jh7110-stg-syscon
> + - starfive,jh7110-sys-syscon
> + - starfive,jh7110-aon-syscon

Is 'syscon' really part of what the blocks are called? Is just 'stg',
'sys' and 'aon' not unique enough?

Rob

2023-02-21 02:44:08

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On 2023/2/21 7:43, Rob Herring wrote:
> On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
>> Add documentation to describe StarFive System Controller Registers.
>>
>> Signed-off-by: William Qiu <[email protected]>
>> ---
>> .../bindings/soc/starfive/jh7110-syscon.yaml | 51 +++++++++++++++++++
>> MAINTAINERS | 5 ++
>> 2 files changed, 56 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>> new file mode 100644
>> index 000000000000..fa4d8522a454
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>> @@ -0,0 +1,51 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive JH7110 SoC system controller
>> +
>> +maintainers:
>> + - William Qiu <[email protected]>
>> +
>> +description: |
>> + The StarFive JH7110 SoC system controller provides register information such
>> + as offset, mask and shift to configure related modules such as MMC and PCIe.
>> +
>> +properties:
>> + compatible:
>> + items:
>> + - enum:
>> + - starfive,jh7110-stg-syscon
>> + - starfive,jh7110-sys-syscon
>> + - starfive,jh7110-aon-syscon
>
> Is 'syscon' really part of what the blocks are called? Is just 'stg',
> 'sys' and 'aon' not unique enough?
>
> Rob
Hi Rob,

In StarFive SoC, we do have syscrg/aoncrg/stgcrg, which is uesd to be the clock
controller, so 'syscon' is added to avoid confusion.

Best regards
William

2023-02-27 22:29:48

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On Tue, Feb 21, 2023 at 10:44:02AM +0800, William Qiu wrote:
>
>
> On 2023/2/21 7:43, Rob Herring wrote:
> > On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
> >> Add documentation to describe StarFive System Controller Registers.
> >>
> >> Signed-off-by: William Qiu <[email protected]>
> >> ---
> >> .../bindings/soc/starfive/jh7110-syscon.yaml | 51 +++++++++++++++++++
> >> MAINTAINERS | 5 ++
> >> 2 files changed, 56 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >> new file mode 100644
> >> index 000000000000..fa4d8522a454
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >> @@ -0,0 +1,51 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: StarFive JH7110 SoC system controller
> >> +
> >> +maintainers:
> >> + - William Qiu <[email protected]>
> >> +
> >> +description: |
> >> + The StarFive JH7110 SoC system controller provides register information such
> >> + as offset, mask and shift to configure related modules such as MMC and PCIe.
> >> +
> >> +properties:
> >> + compatible:
> >> + items:
> >> + - enum:
> >> + - starfive,jh7110-stg-syscon
> >> + - starfive,jh7110-sys-syscon
> >> + - starfive,jh7110-aon-syscon
> >
> > Is 'syscon' really part of what the blocks are called? Is just 'stg',
> > 'sys' and 'aon' not unique enough?
> >
> > Rob
> Hi Rob,
>
> In StarFive SoC, we do have syscrg/aoncrg/stgcrg, which is uesd to be the clock
> controller, so 'syscon' is added to avoid confusion.

You've only added to my confusion. 'syscrg' and 'sys-syscon' are 2
different h/w blocks and unrelated to each other? Or 'syscrg' is the
clock portion of 'sys-syscon'? In that case, 'syscrg' should be a child
of 'sys-syscon' or possibly just all one node. Please provide details on
the entire h/w block so we can provide better input on the bindings.

Rob

2023-02-28 09:06:57

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On 2023/2/28 6:29, Rob Herring wrote:
> On Tue, Feb 21, 2023 at 10:44:02AM +0800, William Qiu wrote:
>>
>>
>> On 2023/2/21 7:43, Rob Herring wrote:
>> > On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
>> >> Add documentation to describe StarFive System Controller Registers.
>> >>
>> >> Signed-off-by: William Qiu <[email protected]>
>> >> ---
>> >> .../bindings/soc/starfive/jh7110-syscon.yaml | 51 +++++++++++++++++++
>> >> MAINTAINERS | 5 ++
>> >> 2 files changed, 56 insertions(+)
>> >> create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>> >> new file mode 100644
>> >> index 000000000000..fa4d8522a454
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>> >> @@ -0,0 +1,51 @@
>> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> >> +%YAML 1.2
>> >> +---
>> >> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> +
>> >> +title: StarFive JH7110 SoC system controller
>> >> +
>> >> +maintainers:
>> >> + - William Qiu <[email protected]>
>> >> +
>> >> +description: |
>> >> + The StarFive JH7110 SoC system controller provides register information such
>> >> + as offset, mask and shift to configure related modules such as MMC and PCIe.
>> >> +
>> >> +properties:
>> >> + compatible:
>> >> + items:
>> >> + - enum:
>> >> + - starfive,jh7110-stg-syscon
>> >> + - starfive,jh7110-sys-syscon
>> >> + - starfive,jh7110-aon-syscon
>> >
>> > Is 'syscon' really part of what the blocks are called? Is just 'stg',
>> > 'sys' and 'aon' not unique enough?
>> >
>> > Rob
>> Hi Rob,
>>
>> In StarFive SoC, we do have syscrg/aoncrg/stgcrg, which is uesd to be the clock
>> controller, so 'syscon' is added to avoid confusion.
>
> You've only added to my confusion. 'syscrg' and 'sys-syscon' are 2
> different h/w blocks and unrelated to each other? Or 'syscrg' is the
> clock portion of 'sys-syscon'? In that case, 'syscrg' should be a child
> of 'sys-syscon' or possibly just all one node. Please provide details on
> the entire h/w block so we can provide better input on the bindings.
>
> Rob

Hi Rob,

It's my description that's problematic.'syscon' here refers to the hardware module
inside our JH7110, which is different from the syscon interface in linux. The syscon
I added now uses the syscon interface of linux to read and write the syscon register
in our JH7110. So we decided to name it that way.

Best regards
William

2023-02-28 10:37:32

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On 28/02/2023 10:05, William Qiu wrote:
>
>
> On 2023/2/28 6:29, Rob Herring wrote:
>> On Tue, Feb 21, 2023 at 10:44:02AM +0800, William Qiu wrote:
>>>
>>>
>>> On 2023/2/21 7:43, Rob Herring wrote:
>>>> On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
>>>>> Add documentation to describe StarFive System Controller Registers.
>>>>>
>>>>> Signed-off-by: William Qiu <[email protected]>
>>>>> ---
>>>>> .../bindings/soc/starfive/jh7110-syscon.yaml | 51 +++++++++++++++++++
>>>>> MAINTAINERS | 5 ++
>>>>> 2 files changed, 56 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..fa4d8522a454
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>> @@ -0,0 +1,51 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: StarFive JH7110 SoC system controller
>>>>> +
>>>>> +maintainers:
>>>>> + - William Qiu <[email protected]>
>>>>> +
>>>>> +description: |
>>>>> + The StarFive JH7110 SoC system controller provides register information such
>>>>> + as offset, mask and shift to configure related modules such as MMC and PCIe.
>>>>> +
>>>>> +properties:
>>>>> + compatible:
>>>>> + items:
>>>>> + - enum:
>>>>> + - starfive,jh7110-stg-syscon
>>>>> + - starfive,jh7110-sys-syscon
>>>>> + - starfive,jh7110-aon-syscon
>>>>
>>>> Is 'syscon' really part of what the blocks are called? Is just 'stg',
>>>> 'sys' and 'aon' not unique enough?
>>>>
>>>> Rob
>>> Hi Rob,
>>>
>>> In StarFive SoC, we do have syscrg/aoncrg/stgcrg, which is uesd to be the clock
>>> controller, so 'syscon' is added to avoid confusion.
>>
>> You've only added to my confusion. 'syscrg' and 'sys-syscon' are 2
>> different h/w blocks and unrelated to each other? Or 'syscrg' is the
>> clock portion of 'sys-syscon'? In that case, 'syscrg' should be a child
>> of 'sys-syscon' or possibly just all one node. Please provide details on
>> the entire h/w block so we can provide better input on the bindings.
>>
>> Rob
>
> Hi Rob,
>
> It's my description that's problematic.'syscon' here refers to the hardware module
> inside our JH7110, which is different from the syscon interface in linux. The syscon
> I added now uses the syscon interface of linux to read and write the syscon register
> in our JH7110. So we decided to name it that way.

You didn't really answer Rob's questions.

Also, syscon is Linux term, so are you sure hardware module is called
like this? Hardware engineers took pure Linux name and used it?

Best regards,
Krzysztof

2023-02-28 11:03:05

by Emil Renner Berthing

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On Tue, 28 Feb 2023 at 11:40, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 28/02/2023 10:05, William Qiu wrote:
> >
> >
> > On 2023/2/28 6:29, Rob Herring wrote:
> >> On Tue, Feb 21, 2023 at 10:44:02AM +0800, William Qiu wrote:
> >>>
> >>>
> >>> On 2023/2/21 7:43, Rob Herring wrote:
> >>>> On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
> >>>>> Add documentation to describe StarFive System Controller Registers.
> >>>>>
> >>>>> Signed-off-by: William Qiu <[email protected]>
> >>>>> ---
> >>>>> .../bindings/soc/starfive/jh7110-syscon.yaml | 51 +++++++++++++++++++
> >>>>> MAINTAINERS | 5 ++
> >>>>> 2 files changed, 56 insertions(+)
> >>>>> create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..fa4d8522a454
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>> @@ -0,0 +1,51 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: StarFive JH7110 SoC system controller
> >>>>> +
> >>>>> +maintainers:
> >>>>> + - William Qiu <[email protected]>
> >>>>> +
> >>>>> +description: |
> >>>>> + The StarFive JH7110 SoC system controller provides register information such
> >>>>> + as offset, mask and shift to configure related modules such as MMC and PCIe.
> >>>>> +
> >>>>> +properties:
> >>>>> + compatible:
> >>>>> + items:
> >>>>> + - enum:
> >>>>> + - starfive,jh7110-stg-syscon
> >>>>> + - starfive,jh7110-sys-syscon
> >>>>> + - starfive,jh7110-aon-syscon
> >>>>
> >>>> Is 'syscon' really part of what the blocks are called? Is just 'stg',
> >>>> 'sys' and 'aon' not unique enough?
> >>>>
> >>>> Rob
> >>> Hi Rob,
> >>>
> >>> In StarFive SoC, we do have syscrg/aoncrg/stgcrg, which is uesd to be the clock
> >>> controller, so 'syscon' is added to avoid confusion.
> >>
> >> You've only added to my confusion. 'syscrg' and 'sys-syscon' are 2
> >> different h/w blocks and unrelated to each other? Or 'syscrg' is the
> >> clock portion of 'sys-syscon'? In that case, 'syscrg' should be a child
> >> of 'sys-syscon' or possibly just all one node. Please provide details on
> >> the entire h/w block so we can provide better input on the bindings.
> >>
> >> Rob
> >
> > Hi Rob,
> >
> > It's my description that's problematic.'syscon' here refers to the hardware module
> > inside our JH7110, which is different from the syscon interface in linux. The syscon
> > I added now uses the syscon interface of linux to read and write the syscon register
> > in our JH7110. So we decided to name it that way.
>
> You didn't really answer Rob's questions.
>
> Also, syscon is Linux term, so are you sure hardware module is called
> like this? Hardware engineers took pure Linux name and used it?

Yes, from the documentation I could find[1] there are CRG blocks
(Clock and Reset Generator) and SYSCON blocks:
SYS CRG
STG CRG
AON CRG
SYS SYSCON
STG SYSCON
AON SYSCON

The CRG blocks contain registers to control clocks and resets that
follow a pattern used by the clock and reset drivers. The SYSCON
blocks just seem to contain registers to control whatever didn't fit
in any other blocks, but might be vaguely related to the peripherals
that run off clocks controlled by the corresponding CRG block.

[1]: https://doc-en.rvspace.org/JH7110/TRM/JH7110_TRM/system_control_registers.html

/Emil

> Best regards,
> Krzysztof
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

2023-02-28 11:09:31

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On Tue, Feb 28, 2023 at 11:37:20AM +0100, Krzysztof Kozlowski wrote:
> On 28/02/2023 10:05, William Qiu wrote:
> > On 2023/2/28 6:29, Rob Herring wrote:
> >> On Tue, Feb 21, 2023 at 10:44:02AM +0800, William Qiu wrote:
> >>> On 2023/2/21 7:43, Rob Herring wrote:
> >>>> On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
> >>>>> Add documentation to describe StarFive System Controller Registers.
> >>>>>
> >>>>> Signed-off-by: William Qiu <[email protected]>
> >>>>> ---
> >>>>> .../bindings/soc/starfive/jh7110-syscon.yaml | 51 +++++++++++++++++++
> >>>>> MAINTAINERS | 5 ++
> >>>>> 2 files changed, 56 insertions(+)
> >>>>> create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..fa4d8522a454
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>> @@ -0,0 +1,51 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: StarFive JH7110 SoC system controller
> >>>>> +
> >>>>> +maintainers:
> >>>>> + - William Qiu <[email protected]>
> >>>>> +
> >>>>> +description: |
> >>>>> + The StarFive JH7110 SoC system controller provides register information such
> >>>>> + as offset, mask and shift to configure related modules such as MMC and PCIe.
> >>>>> +
> >>>>> +properties:
> >>>>> + compatible:
> >>>>> + items:
> >>>>> + - enum:
> >>>>> + - starfive,jh7110-stg-syscon
> >>>>> + - starfive,jh7110-sys-syscon
> >>>>> + - starfive,jh7110-aon-syscon
> >>>>
> >>>> Is 'syscon' really part of what the blocks are called? Is just 'stg',
> >>>> 'sys' and 'aon' not unique enough?

> >>> In StarFive SoC, we do have syscrg/aoncrg/stgcrg, which is uesd to be the clock
> >>> controller, so 'syscon' is added to avoid confusion.
> >>
> >> You've only added to my confusion. 'syscrg' and 'sys-syscon' are 2
> >> different h/w blocks and unrelated to each other? Or 'syscrg' is the
> >> clock portion of 'sys-syscon'? In that case, 'syscrg' should be a child
> >> of 'sys-syscon' or possibly just all one node. Please provide details on
> >> the entire h/w block so we can provide better input on the bindings.

> > It's my description that's problematic.'syscon' here refers to the hardware module
> > inside our JH7110, which is different from the syscon interface in linux. The syscon
> > I added now uses the syscon interface of linux to read and write the syscon register
> > in our JH7110. So we decided to name it that way.
>
> You didn't really answer Rob's questions.
>
> Also, syscon is Linux term, so are you sure hardware module is called
> like this? Hardware engineers took pure Linux name and used it?

Their TRM uses the term SYSCON for these, yes.
Eg:
"The JH7110 system provides the following STG SYSCON control registers
which provides [sic] clock and reset signals to interfaces..."

In fact, the TRM I have describes the following system control register
blocks:
SYS CRG
STG CRG
AON CRG
SYS SYSCON
STG SYSCON
AON SYSCON
SYS IOMUX CFG
AON IOMUX CFG

My understanding is that the first 3 (the CRG ones) are concerned with
clocks and resets & the second 3 contain "random" configuration options,
such as their QSPI IP's configuration options, GPIO voltage settings
etc.

Each of these has a separate, 0x1000 aligned, block in the memory map.

Cheers,
Conor.

Attachments:

(No filename) (3.71 kB)
signature.asc (228.00 B)
Download all attachments

2023-02-28 11:29:41

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On 28/02/2023 12:02, Emil Renner Berthing wrote:
> On Tue, 28 Feb 2023 at 11:40, Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 28/02/2023 10:05, William Qiu wrote:
>>>
>>>
>>> On 2023/2/28 6:29, Rob Herring wrote:
>>>> On Tue, Feb 21, 2023 at 10:44:02AM +0800, William Qiu wrote:
>>>>>
>>>>>
>>>>> On 2023/2/21 7:43, Rob Herring wrote:
>>>>>> On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
>>>>>>> Add documentation to describe StarFive System Controller Registers.
>>>>>>>
>>>>>>> Signed-off-by: William Qiu <[email protected]>
>>>>>>> ---
>>>>>>> .../bindings/soc/starfive/jh7110-syscon.yaml | 51 +++++++++++++++++++
>>>>>>> MAINTAINERS | 5 ++
>>>>>>> 2 files changed, 56 insertions(+)
>>>>>>> create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..fa4d8522a454
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>>>> @@ -0,0 +1,51 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: StarFive JH7110 SoC system controller
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> + - William Qiu <[email protected]>
>>>>>>> +
>>>>>>> +description: |
>>>>>>> + The StarFive JH7110 SoC system controller provides register information such
>>>>>>> + as offset, mask and shift to configure related modules such as MMC and PCIe.
>>>>>>> +
>>>>>>> +properties:
>>>>>>> + compatible:
>>>>>>> + items:
>>>>>>> + - enum:
>>>>>>> + - starfive,jh7110-stg-syscon
>>>>>>> + - starfive,jh7110-sys-syscon
>>>>>>> + - starfive,jh7110-aon-syscon
>>>>>>
>>>>>> Is 'syscon' really part of what the blocks are called? Is just 'stg',
>>>>>> 'sys' and 'aon' not unique enough?
>>>>>>
>>>>>> Rob
>>>>> Hi Rob,
>>>>>
>>>>> In StarFive SoC, we do have syscrg/aoncrg/stgcrg, which is uesd to be the clock
>>>>> controller, so 'syscon' is added to avoid confusion.
>>>>
>>>> You've only added to my confusion. 'syscrg' and 'sys-syscon' are 2
>>>> different h/w blocks and unrelated to each other? Or 'syscrg' is the
>>>> clock portion of 'sys-syscon'? In that case, 'syscrg' should be a child
>>>> of 'sys-syscon' or possibly just all one node. Please provide details on
>>>> the entire h/w block so we can provide better input on the bindings.
>>>>
>>>> Rob
>>>
>>> Hi Rob,
>>>
>>> It's my description that's problematic.'syscon' here refers to the hardware module
>>> inside our JH7110, which is different from the syscon interface in linux. The syscon
>>> I added now uses the syscon interface of linux to read and write the syscon register
>>> in our JH7110. So we decided to name it that way.
>>
>> You didn't really answer Rob's questions.
>>
>> Also, syscon is Linux term, so are you sure hardware module is called
>> like this? Hardware engineers took pure Linux name and used it?
>
> Yes, from the documentation I could find[1] there are CRG blocks
> (Clock and Reset Generator) and SYSCON blocks:
> SYS CRG
> STG CRG
> AON CRG
> SYS SYSCON
> STG SYSCON
> AON SYSCON
>
> The CRG blocks contain registers to control clocks and resets that
> follow a pattern used by the clock and reset drivers. The SYSCON
> blocks just seem to contain registers to control whatever didn't fit
> in any other blocks, but might be vaguely related to the peripherals
> that run off clocks controlled by the corresponding CRG block.

The memory map [1] suggests these are indeed separate address spaces,
e.g. AON CRG, AON SYSCON and AON GPIO, but now I would argue that this
might be still one device - AON (or STG, SYS). Just like PCIE0 has four
address spaces, it does not mean you have four separate PCIE0 devices.
You have only one PCIE0, just like you have only one AON, one STG and
one SYS (System).

[1] https://doc-en.rvspace.org/JH7110/TRM/JH7110_TRM/system_memory_map.html

Best regards,
Krzysztof

2023-02-28 15:00:39

by Emil Renner Berthing

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On Tue, 28 Feb 2023 at 12:28, Krzysztof Kozlowski
<[email protected]> wrote:
> On 28/02/2023 12:02, Emil Renner Berthing wrote:
> > On Tue, 28 Feb 2023 at 11:40, Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 28/02/2023 10:05, William Qiu wrote:
> >>>
> >>>
> >>> On 2023/2/28 6:29, Rob Herring wrote:
> >>>> On Tue, Feb 21, 2023 at 10:44:02AM +0800, William Qiu wrote:
> >>>>>
> >>>>>
> >>>>> On 2023/2/21 7:43, Rob Herring wrote:
> >>>>>> On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
> >>>>>>> Add documentation to describe StarFive System Controller Registers.
> >>>>>>>
> >>>>>>> Signed-off-by: William Qiu <[email protected]>
> >>>>>>> ---
> >>>>>>> .../bindings/soc/starfive/jh7110-syscon.yaml | 51 +++++++++++++++++++
> >>>>>>> MAINTAINERS | 5 ++
> >>>>>>> 2 files changed, 56 insertions(+)
> >>>>>>> create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>>>> new file mode 100644
> >>>>>>> index 000000000000..fa4d8522a454
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>>>> @@ -0,0 +1,51 @@
> >>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>>>>>> +%YAML 1.2
> >>>>>>> +---
> >>>>>>> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
> >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>> +
> >>>>>>> +title: StarFive JH7110 SoC system controller
> >>>>>>> +
> >>>>>>> +maintainers:
> >>>>>>> + - William Qiu <[email protected]>
> >>>>>>> +
> >>>>>>> +description: |
> >>>>>>> + The StarFive JH7110 SoC system controller provides register information such
> >>>>>>> + as offset, mask and shift to configure related modules such as MMC and PCIe.
> >>>>>>> +
> >>>>>>> +properties:
> >>>>>>> + compatible:
> >>>>>>> + items:
> >>>>>>> + - enum:
> >>>>>>> + - starfive,jh7110-stg-syscon
> >>>>>>> + - starfive,jh7110-sys-syscon
> >>>>>>> + - starfive,jh7110-aon-syscon
> >>>>>>
> >>>>>> Is 'syscon' really part of what the blocks are called? Is just 'stg',
> >>>>>> 'sys' and 'aon' not unique enough?
> >>>>>>
> >>>>>> Rob
> >>>>> Hi Rob,
> >>>>>
> >>>>> In StarFive SoC, we do have syscrg/aoncrg/stgcrg, which is uesd to be the clock
> >>>>> controller, so 'syscon' is added to avoid confusion.
> >>>>
> >>>> You've only added to my confusion. 'syscrg' and 'sys-syscon' are 2
> >>>> different h/w blocks and unrelated to each other? Or 'syscrg' is the
> >>>> clock portion of 'sys-syscon'? In that case, 'syscrg' should be a child
> >>>> of 'sys-syscon' or possibly just all one node. Please provide details on
> >>>> the entire h/w block so we can provide better input on the bindings.
> >>>>
> >>>> Rob
> >>>
> >>> Hi Rob,
> >>>
> >>> It's my description that's problematic.'syscon' here refers to the hardware module
> >>> inside our JH7110, which is different from the syscon interface in linux. The syscon
> >>> I added now uses the syscon interface of linux to read and write the syscon register
> >>> in our JH7110. So we decided to name it that way.
> >>
> >> You didn't really answer Rob's questions.
> >>
> >> Also, syscon is Linux term, so are you sure hardware module is called
> >> like this? Hardware engineers took pure Linux name and used it?
> >
> > Yes, from the documentation I could find[1] there are CRG blocks
> > (Clock and Reset Generator) and SYSCON blocks:
> > SYS CRG
> > STG CRG
> > AON CRG
> > SYS SYSCON
> > STG SYSCON
> > AON SYSCON
> >
> > The CRG blocks contain registers to control clocks and resets that
> > follow a pattern used by the clock and reset drivers. The SYSCON
> > blocks just seem to contain registers to control whatever didn't fit
> > in any other blocks, but might be vaguely related to the peripherals
> > that run off clocks controlled by the corresponding CRG block.
>
> The memory map [1] suggests these are indeed separate address spaces,
> e.g. AON CRG, AON SYSCON and AON GPIO, but now I would argue that this
> might be still one device - AON (or STG, SYS). Just like PCIE0 has four
> address spaces, it does not mean you have four separate PCIE0 devices.
> You have only one PCIE0, just like you have only one AON, one STG and
> one SYS (System).

I see what you mean, but if you look into what the registers in the
SYSCON blocks actually do it's not clear to me that they should be
grouped with the clocks/resets any more than say the pinctrl/GPIO
node. Maybe it's my fault for not giving you the full picture. Eg. for
"system" and "always-on" there are blocks:

SYS CRG
SYS SYSCON
SYS IOMUX
AON CRG
AON SYSCON
AON IOMUX

..and it really don't see why eg. SYS CRG and SYS SYSCON should be
thought of as one device, but not include SYS IOMUX then.

As an examly the SYS SYSCON includes registers to control:
- remapping of different peripherals from SD controller to video encoders
- voltage select for certain GPIO pins
- phy interface selection for ethernet and CAN
- QuadSPI delay chain and SRAM configuration
- PLL configuration
- endian selection for the SD controller

To me this is pretty much exactly described by the syscon device tree binding:
"System controller node represents a register region containing a set
of miscellaneous registers. The registers are not cohesive enough to
represent as any specific type of device. [..]"
In any case it's clear that however the SYSCON blocks are represented
in the device tree, a driver for it would need to export registers in
the SYSCON block for other drivers to use.

/Emil

> [1] https://doc-en.rvspace.org/JH7110/TRM/JH7110_TRM/system_memory_map.html
>
>
> Best regards,
> Krzysztof
>

2023-02-28 17:00:01

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On 28/02/2023 15:59, Emil Renner Berthing wrote:
> On Tue, 28 Feb 2023 at 12:28, Krzysztof Kozlowski
> <[email protected]> wrote:
>> On 28/02/2023 12:02, Emil Renner Berthing wrote:
>>> On Tue, 28 Feb 2023 at 11:40, Krzysztof Kozlowski
>>> <[email protected]> wrote:
>>>>
>>>> On 28/02/2023 10:05, William Qiu wrote:
>>>>>
>>>>>
>>>>> On 2023/2/28 6:29, Rob Herring wrote:
>>>>>> On Tue, Feb 21, 2023 at 10:44:02AM +0800, William Qiu wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2023/2/21 7:43, Rob Herring wrote:
>>>>>>>> On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
>>>>>>>>> Add documentation to describe StarFive System Controller Registers.
>>>>>>>>>
>>>>>>>>> Signed-off-by: William Qiu <[email protected]>
>>>>>>>>> ---
>>>>>>>>> .../bindings/soc/starfive/jh7110-syscon.yaml | 51 +++++++++++++++++++
>>>>>>>>> MAINTAINERS | 5 ++
>>>>>>>>> 2 files changed, 56 insertions(+)
>>>>>>>>> create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000000..fa4d8522a454
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>>>>>> @@ -0,0 +1,51 @@
>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>>>>>> +%YAML 1.2
>>>>>>>>> +---
>>>>>>>>> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>>> +
>>>>>>>>> +title: StarFive JH7110 SoC system controller
>>>>>>>>> +
>>>>>>>>> +maintainers:
>>>>>>>>> + - William Qiu <[email protected]>
>>>>>>>>> +
>>>>>>>>> +description: |
>>>>>>>>> + The StarFive JH7110 SoC system controller provides register information such
>>>>>>>>> + as offset, mask and shift to configure related modules such as MMC and PCIe.
>>>>>>>>> +
>>>>>>>>> +properties:
>>>>>>>>> + compatible:
>>>>>>>>> + items:
>>>>>>>>> + - enum:
>>>>>>>>> + - starfive,jh7110-stg-syscon
>>>>>>>>> + - starfive,jh7110-sys-syscon
>>>>>>>>> + - starfive,jh7110-aon-syscon
>>>>>>>>
>>>>>>>> Is 'syscon' really part of what the blocks are called? Is just 'stg',
>>>>>>>> 'sys' and 'aon' not unique enough?
>>>>>>>>
>>>>>>>> Rob
>>>>>>> Hi Rob,
>>>>>>>
>>>>>>> In StarFive SoC, we do have syscrg/aoncrg/stgcrg, which is uesd to be the clock
>>>>>>> controller, so 'syscon' is added to avoid confusion.
>>>>>>
>>>>>> You've only added to my confusion. 'syscrg' and 'sys-syscon' are 2
>>>>>> different h/w blocks and unrelated to each other? Or 'syscrg' is the
>>>>>> clock portion of 'sys-syscon'? In that case, 'syscrg' should be a child
>>>>>> of 'sys-syscon' or possibly just all one node. Please provide details on
>>>>>> the entire h/w block so we can provide better input on the bindings.
>>>>>>
>>>>>> Rob
>>>>>
>>>>> Hi Rob,
>>>>>
>>>>> It's my description that's problematic.'syscon' here refers to the hardware module
>>>>> inside our JH7110, which is different from the syscon interface in linux. The syscon
>>>>> I added now uses the syscon interface of linux to read and write the syscon register
>>>>> in our JH7110. So we decided to name it that way.
>>>>
>>>> You didn't really answer Rob's questions.
>>>>
>>>> Also, syscon is Linux term, so are you sure hardware module is called
>>>> like this? Hardware engineers took pure Linux name and used it?
>>>
>>> Yes, from the documentation I could find[1] there are CRG blocks
>>> (Clock and Reset Generator) and SYSCON blocks:
>>> SYS CRG
>>> STG CRG
>>> AON CRG
>>> SYS SYSCON
>>> STG SYSCON
>>> AON SYSCON
>>>
>>> The CRG blocks contain registers to control clocks and resets that
>>> follow a pattern used by the clock and reset drivers. The SYSCON
>>> blocks just seem to contain registers to control whatever didn't fit
>>> in any other blocks, but might be vaguely related to the peripherals
>>> that run off clocks controlled by the corresponding CRG block.
>>
>> The memory map [1] suggests these are indeed separate address spaces,
>> e.g. AON CRG, AON SYSCON and AON GPIO, but now I would argue that this
>> might be still one device - AON (or STG, SYS). Just like PCIE0 has four
>> address spaces, it does not mean you have four separate PCIE0 devices.
>> You have only one PCIE0, just like you have only one AON, one STG and
>> one SYS (System).
>
> I see what you mean, but if you look into what the registers in the
> SYSCON blocks actually do it's not clear to me that they should be
> grouped with the clocks/resets any more than say the pinctrl/GPIO
> node. Maybe it's my fault for not giving you the full picture. Eg. for
> "system" and "always-on" there are blocks:
>
> SYS CRG
> SYS SYSCON
> SYS IOMUX
> AON CRG
> AON SYSCON
> AON IOMUX
>
> ..and it really don't see why eg. SYS CRG and SYS SYSCON should be
> thought of as one device, but not include SYS IOMUX then.

... include sys iomux as well, just like GPIO is included for AON.

>
> As an examly the SYS SYSCON includes registers to control:
> - remapping of different peripherals from SD controller to video encoders
> - voltage select for certain GPIO pins
> - phy interface selection for ethernet and CAN
> - QuadSPI delay chain and SRAM configuration
> - PLL configuration
> - endian selection for the SD controller
>
> To me this is pretty much exactly described by the syscon device tree binding:
> "System controller node represents a register region containing a set
> of miscellaneous registers. The registers are not cohesive enough to
> represent as any specific type of device. [..]"
> In any case it's clear that however the SYSCON blocks are represented
> in the device tree, a driver for it would need to export registers in
> the SYSCON block for other drivers to use.

You started entire sentence with "but" so you disagree but with what
exactly? The naming? But syscon is fine - hardware manual calls it like
that.

The point was that AON is one device (consisting of multiple blocks).

Best regards,
Krzysztof

2023-02-28 17:32:12

by Emil Renner Berthing

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On Tue, 28 Feb 2023 at 17:59, Krzysztof Kozlowski
<[email protected]> wrote:
> On 28/02/2023 15:59, Emil Renner Berthing wrote:
> > On Tue, 28 Feb 2023 at 12:28, Krzysztof Kozlowski
> > <[email protected]> wrote:
> >> On 28/02/2023 12:02, Emil Renner Berthing wrote:
> >>> On Tue, 28 Feb 2023 at 11:40, Krzysztof Kozlowski
> >>> <[email protected]> wrote:
> >>>>
> >>>> On 28/02/2023 10:05, William Qiu wrote:
> >>>>>
> >>>>>
> >>>>> On 2023/2/28 6:29, Rob Herring wrote:
> >>>>>> On Tue, Feb 21, 2023 at 10:44:02AM +0800, William Qiu wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 2023/2/21 7:43, Rob Herring wrote:
> >>>>>>>> On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
> >>>>>>>>> Add documentation to describe StarFive System Controller Registers.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: William Qiu <[email protected]>
> >>>>>>>>> ---
> >>>>>>>>> .../bindings/soc/starfive/jh7110-syscon.yaml | 51 +++++++++++++++++++
> >>>>>>>>> MAINTAINERS | 5 ++
> >>>>>>>>> 2 files changed, 56 insertions(+)
> >>>>>>>>> create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>>>>>>
> >>>>>>>>> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>>>>>> new file mode 100644
> >>>>>>>>> index 000000000000..fa4d8522a454
> >>>>>>>>> --- /dev/null
> >>>>>>>>> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
> >>>>>>>>> @@ -0,0 +1,51 @@
> >>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>>>>>>>> +%YAML 1.2
> >>>>>>>>> +---
> >>>>>>>>> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
> >>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>>>> +
> >>>>>>>>> +title: StarFive JH7110 SoC system controller
> >>>>>>>>> +
> >>>>>>>>> +maintainers:
> >>>>>>>>> + - William Qiu <[email protected]>
> >>>>>>>>> +
> >>>>>>>>> +description: |
> >>>>>>>>> + The StarFive JH7110 SoC system controller provides register information such
> >>>>>>>>> + as offset, mask and shift to configure related modules such as MMC and PCIe.
> >>>>>>>>> +
> >>>>>>>>> +properties:
> >>>>>>>>> + compatible:
> >>>>>>>>> + items:
> >>>>>>>>> + - enum:
> >>>>>>>>> + - starfive,jh7110-stg-syscon
> >>>>>>>>> + - starfive,jh7110-sys-syscon
> >>>>>>>>> + - starfive,jh7110-aon-syscon
> >>>>>>>>
> >>>>>>>> Is 'syscon' really part of what the blocks are called? Is just 'stg',
> >>>>>>>> 'sys' and 'aon' not unique enough?
> >>>>>>>>
> >>>>>>>> Rob
> >>>>>>> Hi Rob,
> >>>>>>>
> >>>>>>> In StarFive SoC, we do have syscrg/aoncrg/stgcrg, which is uesd to be the clock
> >>>>>>> controller, so 'syscon' is added to avoid confusion.
> >>>>>>
> >>>>>> You've only added to my confusion. 'syscrg' and 'sys-syscon' are 2
> >>>>>> different h/w blocks and unrelated to each other? Or 'syscrg' is the
> >>>>>> clock portion of 'sys-syscon'? In that case, 'syscrg' should be a child
> >>>>>> of 'sys-syscon' or possibly just all one node. Please provide details on
> >>>>>> the entire h/w block so we can provide better input on the bindings.
> >>>>>>
> >>>>>> Rob
> >>>>>
> >>>>> Hi Rob,
> >>>>>
> >>>>> It's my description that's problematic.'syscon' here refers to the hardware module
> >>>>> inside our JH7110, which is different from the syscon interface in linux. The syscon
> >>>>> I added now uses the syscon interface of linux to read and write the syscon register
> >>>>> in our JH7110. So we decided to name it that way.
> >>>>
> >>>> You didn't really answer Rob's questions.
> >>>>
> >>>> Also, syscon is Linux term, so are you sure hardware module is called
> >>>> like this? Hardware engineers took pure Linux name and used it?
> >>>
> >>> Yes, from the documentation I could find[1] there are CRG blocks
> >>> (Clock and Reset Generator) and SYSCON blocks:
> >>> SYS CRG
> >>> STG CRG
> >>> AON CRG
> >>> SYS SYSCON
> >>> STG SYSCON
> >>> AON SYSCON
> >>>
> >>> The CRG blocks contain registers to control clocks and resets that
> >>> follow a pattern used by the clock and reset drivers. The SYSCON
> >>> blocks just seem to contain registers to control whatever didn't fit
> >>> in any other blocks, but might be vaguely related to the peripherals
> >>> that run off clocks controlled by the corresponding CRG block.
> >>
> >> The memory map [1] suggests these are indeed separate address spaces,
> >> e.g. AON CRG, AON SYSCON and AON GPIO, but now I would argue that this
> >> might be still one device - AON (or STG, SYS). Just like PCIE0 has four
> >> address spaces, it does not mean you have four separate PCIE0 devices.
> >> You have only one PCIE0, just like you have only one AON, one STG and
> >> one SYS (System).
> >
> > I see what you mean, but if you look into what the registers in the
> > SYSCON blocks actually do it's not clear to me that they should be
> > grouped with the clocks/resets any more than say the pinctrl/GPIO
> > node. Maybe it's my fault for not giving you the full picture. Eg. for
> > "system" and "always-on" there are blocks:
> >
> > SYS CRG
> > SYS SYSCON
> > SYS IOMUX
> > AON CRG
> > AON SYSCON
> > AON IOMUX
> >
> > ..and it really don't see why eg. SYS CRG and SYS SYSCON should be
> > thought of as one device, but not include SYS IOMUX then.
>
> ... include sys iomux as well, just like GPIO is included for AON.

This would at least take the view that the blocks named alike should
be thought of as a single device to its logical conclusion.
Unfortunately we're a bit late for that. The pinctrl/GPiO bindings and
drivers are already merged:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d6e0a660097dcdb80e7c5c859eb12f776060b02e

> >
> > As an examly the SYS SYSCON includes registers to control:
> > - remapping of different peripherals from SD controller to video encoders
> > - voltage select for certain GPIO pins
> > - phy interface selection for ethernet and CAN
> > - QuadSPI delay chain and SRAM configuration
> > - PLL configuration
> > - endian selection for the SD controller
> >
> > To me this is pretty much exactly described by the syscon device tree binding:
> > "System controller node represents a register region containing a set
> > of miscellaneous registers. The registers are not cohesive enough to
> > represent as any specific type of device. [..]"
> > In any case it's clear that however the SYSCON blocks are represented
> > in the device tree, a driver for it would need to export registers in
> > the SYSCON block for other drivers to use.
>
> You started entire sentence with "but" so you disagree but with what
> exactly? The naming? But syscon is fine - hardware manual calls it like
> that.
>
> The point was that AON is one device (consisting of multiple blocks).

Yes, and what I'm trying to explain is that I'm not convinced that's
the right model. The CRG blocks and IOMUX blocks don't really have
anything in common other than the name StarFive gave them. You can
argue that the CRG and IOMUX blocks overlap with the corresponding
SYSCON block, but so do a lot of other peripherals as you can see from
the list above.

I think the IOMUX and SYSCON blocks are just named after the clock
domain they're under, but a lot of other peripherals are also under
the SYS and AON clock domains and we don't model them as one big
device.

/Emil

>
> Best regards,
> Krzysztof
>

2023-02-28 18:06:49

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

On Tue, Feb 28, 2023 at 06:31:46PM +0100, Emil Renner Berthing wrote:
> On Tue, 28 Feb 2023 at 17:59, Krzysztof Kozlowski
> <[email protected]> wrote:
> > On 28/02/2023 15:59, Emil Renner Berthing wrote:
> > > On Tue, 28 Feb 2023 at 12:28, Krzysztof Kozlowski
> > > <[email protected]> wrote:

> > > I see what you mean, but if you look into what the registers in the
> > > SYSCON blocks actually do it's not clear to me that they should be
> > > grouped with the clocks/resets any more than say the pinctrl/GPIO
> > > node. Maybe it's my fault for not giving you the full picture. Eg. for
> > > "system" and "always-on" there are blocks:
> > >
> > > SYS CRG
> > > SYS SYSCON
> > > SYS IOMUX
> > > AON CRG
> > > AON SYSCON
> > > AON IOMUX
> > >
> > > ..and it really don't see why eg. SYS CRG and SYS SYSCON should be
> > > thought of as one device, but not include SYS IOMUX then.
> >
> > ... include sys iomux as well, just like GPIO is included for AON.
>
> This would at least take the view that the blocks named alike should
> be thought of as a single device to its logical conclusion.
> Unfortunately we're a bit late for that. The pinctrl/GPiO bindings and
> drivers are already merged:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d6e0a660097dcdb80e7c5c859eb12f776060b02e
>
> > >
> > > As an examly the SYS SYSCON includes registers to control:
> > > - remapping of different peripherals from SD controller to video encoders
> > > - voltage select for certain GPIO pins
> > > - phy interface selection for ethernet and CAN
> > > - QuadSPI delay chain and SRAM configuration
> > > - PLL configuration
> > > - endian selection for the SD controller
> > >
> > > To me this is pretty much exactly described by the syscon device tree binding:
> > > "System controller node represents a register region containing a set
> > > of miscellaneous registers. The registers are not cohesive enough to
> > > represent as any specific type of device. [..]"
> > > In any case it's clear that however the SYSCON blocks are represented
> > > in the device tree, a driver for it would need to export registers in
> > > the SYSCON block for other drivers to use.
> >
> > You started entire sentence with "but" so you disagree but with what
> > exactly? The naming? But syscon is fine - hardware manual calls it like
> > that.
> >
> > The point was that AON is one device (consisting of multiple blocks).
>
> Yes, and what I'm trying to explain is that I'm not convinced that's
> the right model. The CRG blocks and IOMUX blocks don't really have
> anything in common other than the name StarFive gave them. You can
> argue that the CRG and IOMUX blocks overlap with the corresponding
> SYSCON block, but so do a lot of other peripherals as you can see from
> the list above.
>
> I think the IOMUX and SYSCON blocks are just named after the clock
> domain they're under, but a lot of other peripherals are also under
> the SYS and AON clock domains and we don't model them as one big
> device.

I went and bothered Rob/Krzysztof on IRC about this.
Not gonna speak for them, but I think they're now okay with keeping the
SYS_CRG (clock+reset block) separate from the SYS_SYSCON block ("random
collection of registers"). Possibly there was just confusion due to the
naming used here, thinking that "SYS", "STG" and "AON" were devices with
two register blocks, as opposed to being the name of a clock/power domain
on the SoC.

I'll leave it up to them to confirm that though!

Cheers,
Conor.

Attachments:

(No filename) (3.46 kB)
signature.asc (228.00 B)
Download all attachments

2023-03-06 14:05:55

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

Hey William,

On Thu, Feb 16, 2023 at 11:31:45AM +0100, Krzysztof Kozlowski wrote:
> On 16/02/2023 11:29, Conor Dooley wrote:
> > On Thu, Feb 16, 2023 at 11:23:00AM +0100, Krzysztof Kozlowski wrote:
> >> On 15/02/2023 12:32, William Qiu wrote:
> >>> Add documentation to describe StarFive System Controller Registers.
> >>>
> >>> Signed-off-by: William Qiu <[email protected]>
> >>> ---
> >>
> >> Thank you for your patch. There is something to discuss/improve.

Could you please submit a v5 of this, with the bits below fixed,
whenever Hal sends their next version of the base dts series?
There's no maintainers coverage for a soc/starfive subdirectory of
dt-bindings yet, so please CC [email protected] &
[email protected] on the patch.

Thanks,
Conor.

> >>
> >>> +properties:
> >>> + compatible:
> >>> + items:
> >>> + - enum:
> >>> + - starfive,jh7110-stg-syscon
> >>> + - starfive,jh7110-sys-syscon
> >>> + - starfive,jh7110-aon-syscon
> >>
> >> Maybe keep them ordered alphabetically?
> >>
> >>> + - const: syscon
> >>> +
> >>> + reg:
> >>> + maxItems: 1
> >>> +
> >>> +required:
> >>> + - compatible
> >>> + - reg
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> + - |
> >>> + syscon@10240000 {
> >>> + compatible = "starfive,jh7110-stg-syscon", "syscon";
> >>> + reg = <0x10240000 0x1000>;
> >>> + };
> >>
> >> Keep only one example. All others are the same.
> >
> > With these fixed:
> > Reviewed-by: Conor Dooley <[email protected]>
> >
> > @Krzysztof, I assume the location of the binding is okay with you since
> > you didn't object to it & I suppose this one is up to me to apply if so.
>
> Yeah, generic sysreg devices go to soc. If their primary functions were
> different (e.g. clock controller which also is syscon), then they should
> go to respective directories, but it's not the case here, I think.
>
> Best regards,
> Krzysztof
>
>

Attachments:

(No filename) (1.94 kB)
signature.asc (228.00 B)
Download all attachments

2023-03-07 01:43:52