Message-ID: <CAP6Zq1jhy0GUtoO3=m-PHD3WCertSvAg+G7iygiRkWvjzfwSpQ@mail.gmail.com>
Date: Mon, 22 Jan 2024 19:14:35 +0200
From: Tomer Maimon <tmaimon77@...il.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: mturquette@...libre.com, sboyd@...nel.org, robh+dt@...nel.org, 
	krzysztof.kozlowski+dt@...aro.org, tali.perry1@...il.com, joel@....id.au, 
	venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com, 
	openbmc@...ts.ozlabs.org, linux-clk@...r.kernel.org, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock
 and reset registers

Hi Krzysztof,

Thanks for your comment

On Tue, 16 Jan 2024 at 22:37, Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> 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
> > <krzysztof.kozlowski@...aro.org> 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 <tmaimon77@...il.com>
> >>> ---
> >>
> >> 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@...01000 {
                        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 <tmaimon77@...il.com>
> >>> +
> >>> +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

Powered by blists - more mailing lists