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

[Tomer Maimon]

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
<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?
>
> 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 <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.
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