On 08/01/2024 14:54, Tomer Maimon wrote:
> A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and
> will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the
> NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to
> retrieve SoC model and version information.
>
A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> This patch adds a binding to describe this node.
Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
> Signed-off-by: Tomer Maimon <[email protected]>
> ---
How possibly could it be v22 if there is:
1. No changelog
2. No previous submissions
?
NAK, it's something completely new without any explanation.
Limited review follows.
> .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml | 40 +++++++++++++++++++
> 1 file changed, 40 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
>
> diff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> new file mode 100644
> index 000000000000..dfec64a8eb26
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Clock and reset registers block in Nuvoton SoCs
This is vague. Any block? All blocks? Your SoC has only one block? I
doubt, although possible.
Anyway, clocks go to clock directory, not to soc! We've been here and
you already received that feedback.
> +
> +maintainers:
> + - Tomer Maimon <[email protected]>
> +
> +description:
> + The clock and reset registers are a registers block in Nuvoton SoCs that
> + handle both reset and clock functionality.
That's still vague. Say something useful.
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - nuvoton,npcm750-clk-rst
> + - nuvoton,npcm845-clk-rst
> + - const: syscon
> + - const: simple-mfd
No, it's not a syscon and not a simple-mfd. You just said it is clock
provider and reset controller. Thus missing clock cells and reset cells.
> +
> + reg:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties:
> + type: object
No, instead:
additionalProperties: false
> +
> +examples:
> + - |
> + clk_rst: syscon@801000 {
Suddenly a syscon?
Drop unused label.
> + compatible = "nuvoton,npcm750-clk-rst", "syscon", "simple-mfd";
> + reg = <0x801000 0x6C>;
Only lowercase hex.
You just sent some v22 of something new, making all the mistakes from
the past submissions for which you received feedback.
> + };
Best regards,
Krzysztof
Hi Krzysztof,
Thanks for your comments.
Sorry for the long explanation but I think it is necessary.
In the NPCM8XX SoC, the reset and the clock register modules are
scrambled in the same memory register region.
The NPCM8XX Clock driver is still in the upstream process (for a long
time) but the NPCM8XX reset driver is already upstreamed.
One of the main comments in the NPCM8XX Clock driver upstream process
is that the clock register is mixed with the reset register and
therefore we can't map (ioremap) the clock register
region because is already mapped by the reset module, therefore we
decided to use an external syscon to handle the clock and the reset
registers driver.
I highly appreciate your guidance on this topic.
On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 08/01/2024 14:54, Tomer Maimon wrote:
> > A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and
> > will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the
> > NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to
> > retrieve SoC model and version information.
> >
>
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> > This patch adds a binding to describe this node.
>
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
> >
> > Signed-off-by: Tomer Maimon <[email protected]>
> > ---
>
> How possibly could it be v22 if there is:
> 1. No changelog
> 2. No previous submissions
> ?
Should the dt-binding and dts patches be a part of the clock patch set
(this is why it's V22) or should I open a new patch set?
>
> NAK, it's something completely new without any explanation.
>
> Limited review follows.
>
>
> > .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml | 40 +++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> > new file mode 100644
> > index 000000000000..dfec64a8eb26
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> > @@ -0,0 +1,40 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Clock and reset registers block in Nuvoton SoCs
>
> This is vague. Any block? All blocks? Your SoC has only one block? I
> doubt, although possible.
>
> Anyway, clocks go to clock directory, not to soc! We've been here and
> you already received that feedback.
Since one region handles the reset and the clock registers shouldn't I
add the dt-binding to the SoC like the GCR and not to the clock
directory?
https://elixir.bootlin.com/linux/v6.7/source/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-gcr.yaml
>
>
> > +
> > +maintainers:
> > + - Tomer Maimon <[email protected]>
> > +
> > +description:
> > + The clock and reset registers are a registers block in Nuvoton SoCs that
> > + handle both reset and clock functionality.
>
> That's still vague. Say something useful.
Will describe more
>
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - nuvoton,npcm750-clk-rst
> > + - nuvoton,npcm845-clk-rst
> > + - const: syscon
> > + - const: simple-mfd
>
> No, it's not a syscon and not a simple-mfd. You just said it is clock
Yes, I understand the syscon node represents a register region
containing a set of miscellaneous registers, but as explain above it
is quite the case here.
I will remove the simple-mfd.
> provider and reset controller. Thus missing clock cells and reset cells.
The reset cell and clock cell found at the clock and reset dt-binding,
is it enough?
>
> > +
> > + reg:
> > + maxItems: 1
> > +
> > +required:
> > + - compatible
> > + - reg
> > +
> > +additionalProperties:
> > + type: object
>
> No, instead:
> additionalProperties: false
O.K.
>
> > +
> > +examples:
> > + - |
> > + clk_rst: syscon@801000 {
I should use syscon no? if no what should I use?
>
> Suddenly a syscon?
>
> Drop unused label.
>
> > + compatible = "nuvoton,npcm750-clk-rst", "syscon", "simple-mfd";
> > + reg = <0x801000 0x6C>;
>
> Only lowercase hex.
>
> You just sent some v22 of something new, making all the mistakes from
> the past submissions for which you received feedback.
> > + };
>
> Best regards,
> Krzysztof
>
Thanks a lot!
Tomer
On 16/01/2024 20:02, Tomer Maimon wrote:
> Hi Krzysztof,
>
> Thanks for your comments.
>
> Sorry for the long explanation but I think it is necessary.
>
> In the NPCM8XX SoC, the reset and the clock register modules are
> scrambled in the same memory register region.
> The NPCM8XX Clock driver is still in the upstream process (for a long
> time) but the NPCM8XX reset driver is already upstreamed.
>
> One of the main comments in the NPCM8XX Clock driver upstream process
> is that the clock register is mixed with the reset register and
> therefore we can't map (ioremap) the clock register
> region because is already mapped by the reset module, therefore we
> decided to use an external syscon to handle the clock and the reset
> registers driver.
>
> I highly appreciate your guidance on this topic.
Linux deals with it easily, that's why we have regmaps. What's the
problem exactly?
>
> On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 08/01/2024 14:54, Tomer Maimon wrote:
>>> A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and
>>> will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the
>>> NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to
>>> retrieve SoC model and version information.
>>>
>>
>> A nit, subject: drop second/last, redundant "bindings". The
>> "dt-bindings" prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>
>>> This patch adds a binding to describe this node.
>>
>> Please do not use "This commit/patch/change", but imperative mood. See
>> longer explanation here:
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>
>>>
>>> Signed-off-by: Tomer Maimon <[email protected]>
>>> ---
>>
>> How possibly could it be v22 if there is:
>> 1. No changelog
>> 2. No previous submissions
>> ?
> Should the dt-binding and dts patches be a part of the clock patch set
> (this is why it's V22) or should I open a new patch set?
You should explain what is happening here. That's why you have changelog
for.
>>
>> NAK, it's something completely new without any explanation.
>>
>> Limited review follows.
>>
>>
>>> .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml | 40 +++++++++++++++++++
>>> 1 file changed, 40 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
>>> new file mode 100644
>>> index 000000000000..dfec64a8eb26
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
>>> @@ -0,0 +1,40 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Clock and reset registers block in Nuvoton SoCs
>>
>> This is vague. Any block? All blocks? Your SoC has only one block? I
>> doubt, although possible.
>>
>> Anyway, clocks go to clock directory, not to soc! We've been here and
>> you already received that feedback.
> Since one region handles the reset and the clock registers shouldn't I
> add the dt-binding to the SoC like the GCR and not to the clock
No, soc is not a dumping ground..
> directory?
> https://elixir.bootlin.com/linux/v6.7/source/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-gcr.yaml
Choose the main feature of the block - either clock controller or reset
controller - and put it there.
>>
>>
>>> +
>>> +maintainers:
>>> + - Tomer Maimon <[email protected]>
>>> +
>>> +description:
>>> + The clock and reset registers are a registers block in Nuvoton SoCs that
>>> + handle both reset and clock functionality.
>>
>> That's still vague. Say something useful.
> Will describe more
>>
>>> +
>>> +properties:
>>> + compatible:
>>> + items:
>>> + - enum:
>>> + - nuvoton,npcm750-clk-rst
>>> + - nuvoton,npcm845-clk-rst
>>> + - const: syscon
>>> + - const: simple-mfd
>>
>> No, it's not a syscon and not a simple-mfd. You just said it is clock
> Yes, I understand the syscon node represents a register region
> containing a set of miscellaneous registers, but as explain above it
> is quite the case here.
Nothing in this patch was telling this.
> I will remove the simple-mfd.
>> provider and reset controller. Thus missing clock cells and reset cells.
> The reset cell and clock cell found at the clock and reset dt-binding,
> is it enough?
This is the reset and clock binding, isn't it? You called it (your title):
"Clock and reset registers block in Nuvoton SoCs"
Best regards,
Krzysztof
Hi Krzysztof,
Thanks for your comment
On Tue, 16 Jan 2024 at 22:37, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 16/01/2024 20:02, Tomer Maimon wrote:
> > Hi Krzysztof,
> >
> > Thanks for your comments.
> >
> > Sorry for the long explanation but I think it is necessary.
> >
> > In the NPCM8XX SoC, the reset and the clock register modules are
> > scrambled in the same memory register region.
> > The NPCM8XX Clock driver is still in the upstream process (for a long
> > time) but the NPCM8XX reset driver is already upstreamed.
> >
> > One of the main comments in the NPCM8XX Clock driver upstream process
> > is that the clock register is mixed with the reset register and
> > therefore we can't map (ioremap) the clock register
> > region because is already mapped by the reset module, therefore we
> > decided to use an external syscon to handle the clock and the reset
> > registers driver.
> >
> > I highly appreciate your guidance on this topic.
>
> Linux deals with it easily, that's why we have regmaps. What's the
> problem exactly?
This is exactly what is done in the submitted clock driver.
>
> >
> > On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 08/01/2024 14:54, Tomer Maimon wrote:
> >>> A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and
> >>> will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the
> >>> NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to
> >>> retrieve SoC model and version information.
> >>>
> >>
> >> A nit, subject: drop second/last, redundant "bindings". The
> >> "dt-bindings" prefix is already stating that these are bindings.
> >> See also:
> >> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> >>
> >>> This patch adds a binding to describe this node.
> >>
> >> Please do not use "This commit/patch/change", but imperative mood. See
> >> longer explanation here:
> >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> >>
> >>>
> >>> Signed-off-by: Tomer Maimon <[email protected]>
> >>> ---
> >>
> >> How possibly could it be v22 if there is:
> >> 1. No changelog
> >> 2. No previous submissions
> >> ?
> > Should the dt-binding and dts patches be a part of the clock patch set
> > (this is why it's V22) or should I open a new patch set?
>
> You should explain what is happening here. That's why you have changelog
> for.
Will explain it better in the cover letter in the change log.
>
> >>
> >> NAK, it's something completely new without any explanation.
> >>
> >> Limited review follows.
> >>
> >>
> >>> .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml | 40 +++++++++++++++++++
> >>> 1 file changed, 40 insertions(+)
> >>> create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> >>> new file mode 100644
> >>> index 000000000000..dfec64a8eb26
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> >>> @@ -0,0 +1,40 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Clock and reset registers block in Nuvoton SoCs
> >>
> >> This is vague. Any block? All blocks? Your SoC has only one block? I
> >> doubt, although possible.
> >>
> >> Anyway, clocks go to clock directory, not to soc! We've been here and
> >> you already received that feedback.
> > Since one region handles the reset and the clock registers shouldn't I
> > add the dt-binding to the SoC like the GCR and not to the clock
>
> No, soc is not a dumping ground..
Maybe I do not need to add dt binding document for the clock and reset
syscon but handle the registers as follows in the dtsi.
sysctrl: system-controller@f0801000 {
compatible = "syscon", "simple-mfd";
reg = <0x0 0xf0801000 0x0 0x1000>;
rstc: reset-controller {
compatible = "nuvoton,npcm845-reset";
#reset-cells = <2>;
nuvoton,sysgcr = <&gcr>;
};
clk: clock-controller {
compatible = "nuvoton,npcm845-clk";
#clock-cells = <1>;
clocks = <&refclk>;
clock-names = "refclk";
};
};
is it acceptable?
>
> > directory?
> > https://elixir.bootlin.com/linux/v6.7/source/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-gcr.yaml
>
> Choose the main feature of the block - either clock controller or reset
> controller - and put it there.
>
> >>
> >>
> >>> +
> >>> +maintainers:
> >>> + - Tomer Maimon <[email protected]>
> >>> +
> >>> +description:
> >>> + The clock and reset registers are a registers block in Nuvoton SoCs that
> >>> + handle both reset and clock functionality.
> >>
> >> That's still vague. Say something useful.
> > Will describe more
> >>
> >>> +
> >>> +properties:
> >>> + compatible:
> >>> + items:
> >>> + - enum:
> >>> + - nuvoton,npcm750-clk-rst
> >>> + - nuvoton,npcm845-clk-rst
> >>> + - const: syscon
> >>> + - const: simple-mfd
> >>
> >> No, it's not a syscon and not a simple-mfd. You just said it is clock
> > Yes, I understand the syscon node represents a register region
> > containing a set of miscellaneous registers, but as explain above it
> > is quite the case here.
>
> Nothing in this patch was telling this.
>
> > I will remove the simple-mfd.
> >> provider and reset controller. Thus missing clock cells and reset cells.
> > The reset cell and clock cell found at the clock and reset dt-binding,
> > is it enough?
>
> This is the reset and clock binding, isn't it? You called it (your title):
> "Clock and reset registers block in Nuvoton SoCs"
>
>
>
>
> Best regards,
> Krzysztof
>
Best regards,
Tomer
Hi Krzysztof,
On Mon, 22 Jan 2024 at 19:14, Tomer Maimon <[email protected]> wrote:
>
> Hi Krzysztof,
>
> Thanks for your comment
>
> On Tue, 16 Jan 2024 at 22:37, Krzysztof Kozlowski
> <[email protected]> wrote:
> >
> > On 16/01/2024 20:02, Tomer Maimon wrote:
> > > Hi Krzysztof,
> > >
> > > Thanks for your comments.
> > >
> > > Sorry for the long explanation but I think it is necessary.
> > >
> > > In the NPCM8XX SoC, the reset and the clock register modules are
> > > scrambled in the same memory register region.
> > > The NPCM8XX Clock driver is still in the upstream process (for a long
> > > time) but the NPCM8XX reset driver is already upstreamed.
> > >
> > > One of the main comments in the NPCM8XX Clock driver upstream process
> > > is that the clock register is mixed with the reset register and
> > > therefore we can't map (ioremap) the clock register
> > > region because is already mapped by the reset module, therefore we
> > > decided to use an external syscon to handle the clock and the reset
> > > registers driver.
> > >
> > > I highly appreciate your guidance on this topic.
> >
> > Linux deals with it easily, that's why we have regmaps. What's the
> > problem exactly?
> This is exactly what is done in the submitted clock driver.
> >
> > >
> > > On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
> > > <[email protected]> wrote:
> > >>
> > >> On 08/01/2024 14:54, Tomer Maimon wrote:
> > >>> A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and
> > >>> will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the
> > >>> NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to
> > >>> retrieve SoC model and version information.
> > >>>
> > >>
> > >> A nit, subject: drop second/last, redundant "bindings". The
> > >> "dt-bindings" prefix is already stating that these are bindings.
> > >> See also:
> > >> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> > >>
> > >>> This patch adds a binding to describe this node.
> > >>
> > >> Please do not use "This commit/patch/change", but imperative mood. See
> > >> longer explanation here:
> > >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> > >>
> > >>>
> > >>> Signed-off-by: Tomer Maimon <[email protected]>
> > >>> ---
> > >>
> > >> How possibly could it be v22 if there is:
> > >> 1. No changelog
> > >> 2. No previous submissions
> > >> ?
> > > Should the dt-binding and dts patches be a part of the clock patch set
> > > (this is why it's V22) or should I open a new patch set?
> >
> > You should explain what is happening here. That's why you have changelog
> > for.
> Will explain it better in the cover letter in the change log.
> >
> > >>
> > >> NAK, it's something completely new without any explanation.
> > >>
> > >> Limited review follows.
> > >>
> > >>
> > >>> .../soc/nuvoton/nuvoton,npcm-clk-rst.yaml | 40 +++++++++++++++++++
> > >>> 1 file changed, 40 insertions(+)
> > >>> create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> > >>> new file mode 100644
> > >>> index 000000000000..dfec64a8eb26
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml
> > >>> @@ -0,0 +1,40 @@
> > >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > >>> +%YAML 1.2
> > >>> +---
> > >>> +$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml#
> > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>> +
> > >>> +title: Clock and reset registers block in Nuvoton SoCs
> > >>
> > >> This is vague. Any block? All blocks? Your SoC has only one block? I
> > >> doubt, although possible.
> > >>
> > >> Anyway, clocks go to clock directory, not to soc! We've been here and
> > >> you already received that feedback.
> > > Since one region handles the reset and the clock registers shouldn't I
> > > add the dt-binding to the SoC like the GCR and not to the clock
> >
> > No, soc is not a dumping ground..
> Maybe I do not need to add dt binding document for the clock and reset
> syscon but handle the registers as follows in the dtsi.
>
> sysctrl: system-controller@f0801000 {
> compatible = "syscon", "simple-mfd";
> reg = <0x0 0xf0801000 0x0 0x1000>;
>
> rstc: reset-controller {
> compatible = "nuvoton,npcm845-reset";
> #reset-cells = <2>;
> nuvoton,sysgcr = <&gcr>;
> };
>
> clk: clock-controller {
> compatible = "nuvoton,npcm845-clk";
> #clock-cells = <1>;
> clocks = <&refclk>;
> clock-names = "refclk";
> };
> };
>
> is it acceptable?
Appreciate for your advice on the question above.
> >
> > > directory?
> > > https://elixir.bootlin.com/linux/v6.7/source/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-gcr.yaml
> >
> > Choose the main feature of the block - either clock controller or reset
> > controller - and put it there.
> >
> > >>
> > >>
> > >>> +
> > >>> +maintainers:
> > >>> + - Tomer Maimon <[email protected]>
> > >>> +
> > >>> +description:
> > >>> + The clock and reset registers are a registers block in Nuvoton SoCs that
> > >>> + handle both reset and clock functionality.
> > >>
> > >> That's still vague. Say something useful.
> > > Will describe more
> > >>
> > >>> +
> > >>> +properties:
> > >>> + compatible:
> > >>> + items:
> > >>> + - enum:
> > >>> + - nuvoton,npcm750-clk-rst
> > >>> + - nuvoton,npcm845-clk-rst
> > >>> + - const: syscon
> > >>> + - const: simple-mfd
> > >>
> > >> No, it's not a syscon and not a simple-mfd. You just said it is clock
> > > Yes, I understand the syscon node represents a register region
> > > containing a set of miscellaneous registers, but as explain above it
> > > is quite the case here.
> >
> > Nothing in this patch was telling this.
> >
> > > I will remove the simple-mfd.
> > >> provider and reset controller. Thus missing clock cells and reset cells.
> > > The reset cell and clock cell found at the clock and reset dt-binding,
> > > is it enough?
> >
> > This is the reset and clock binding, isn't it? You called it (your title):
> > "Clock and reset registers block in Nuvoton SoCs"
> >
> >
> >
> >
> > Best regards,
> > Krzysztof
> >
>
> Best regards,
>
> Tomer
Best regards,
Tomer