Hi Krzysztof,


On Mon, 22 Jan 2024 at 19:14, Tomer Maimon [off-list ref] wrote:

Hi Krzysztof,

Thanks for your comment

On Tue, 16 Jan 2024 at 22:37, Krzysztof Kozlowski
[off-list ref] wrote:

quoted

On 16/01/2024 20:02, Tomer Maimon wrote:

quoted

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.

quoted

quoted

On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski
[off-list ref] wrote:

quoted

On 08/01/2024 14:54, Tomer Maimon wrote:

quoted

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

quoted

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

quoted

Signed-off-by: Tomer Maimon <tmaimon77@gmail.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.

quoted

quoted

quoted

NAK, it's something completely new without any explanation.

Limited review follows.

quoted

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

quoted

quoted

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.

quoted

quoted

quoted

+
+maintainers:
+  - Tomer Maimon [off-list ref]
+
+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

quoted

quoted

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

quoted

I will remove the simple-mfd.

quoted

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