Re: [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers

[Tomer Maimon]

Hi Krzysztof,


On Mon, 22 Jan 2024 at 19:14, Tomer Maimon <tmaimon77@xxxxxxxxx> wrote:
>
> Hi Krzysztof,
>
> Thanks for your comment
>
> On Tue, 16 Jan 2024 at 22:37, Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx>
> > >>> ---
> > >>
> > >> 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 <tmaimon77@xxxxxxxxx>
> > >>> +
> > >>> +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