| Message ID | 20240108135421.684263-5-tmaimon77@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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 > <krzysztof.kozlowski@linaro.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@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. >> >> 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 <tmaimon77@gmail.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
Hi Krzysztof, Thanks for your comment On Tue, 16 Jan 2024 at 22:37, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.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@linaro.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@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. > > >> > >> 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 <tmaimon77@gmail.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
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 + +maintainers: + - Tomer Maimon <tmaimon77@gmail.com> + +description: + The clock and reset registers are a registers block in Nuvoton SoCs that + handle both reset and clock functionality. + +properties: + compatible: + items: + - enum: + - nuvoton,npcm750-clk-rst + - nuvoton,npcm845-clk-rst + - const: syscon + - const: simple-mfd + + reg: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: + type: object + +examples: + - | + clk_rst: syscon@801000 { + compatible = "nuvoton,npcm750-clk-rst", "syscon", "simple-mfd"; + reg = <0x801000 0x6C>; + };
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. This patch adds a binding to describe this node. Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> --- .../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