Openbmc archive mirror
 help / color / mirror / Atom feed
From: Tomer Maimon <tmaimon77@gmail.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: devicetree@vger.kernel.org, benjaminfair@google.com,
	sboyd@kernel.org, venture@google.com, mturquette@baylibre.com,
	linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
	tali.perry1@gmail.com, robh+dt@kernel.org, joel@jms.id.au,
	krzysztof.kozlowski+dt@linaro.org, openbmc@lists.ozlabs.org
Subject: Re: [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers
Date: Wed, 24 Jan 2024 16:21:02 +0200	[thread overview]
Message-ID: <CAP6Zq1j0-zZvpKRc4T_DPOo=7bewc9u-5NuM48e0Akvrd0KLLQ@mail.gmail.com> (raw)
In-Reply-To: <CAP6Zq1jhy0GUtoO3=m-PHD3WCertSvAg+G7iygiRkWvjzfwSpQ@mail.gmail.com>

Hi Krzysztof,


On Mon, 22 Jan 2024 at 19:14, Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> 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?
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@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

Best regards,

Tomer

  reply	other threads:[~2024-01-24 14:22 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-08 13:54 [PATCH v22 0/8] Introduce Nuvoton Arbel NPCM8XX BMC SoC Tomer Maimon
2024-01-08 13:54 ` [PATCH v22 1/8] dt-bindings: clock: npcm845: Add reference 25m clock property Tomer Maimon
2024-01-08 21:09   ` Rob Herring
2024-01-09  6:45     ` Tomer Maimon
2024-01-09 17:08   ` Rob Herring
2024-01-10 13:47     ` Tomer Maimon
     [not found]       ` <dc739435-d955-44f7-b5ee-9de4d5336725@linaro.org>
2024-01-10 21:45         ` Stephen Boyd
2024-01-16 15:21           ` Tomer Maimon
2024-01-08 13:54 ` [PATCH v22 2/8] arm64: dts: nuvoton: npcm8xx: add refernace clock Tomer Maimon
2024-01-08 13:54 ` [PATCH v22 3/8] arm: dts: nuvoton: npcm7xx: modify rst syscon node Tomer Maimon
     [not found]   ` <99eca166-7d78-4ada-ac0a-ad67f3d8eb33@linaro.org>
2024-01-16 19:51     ` Tomer Maimon
2024-01-08 13:54 ` [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers Tomer Maimon
     [not found]   ` <98eaac00-1e3d-4c27-89f5-0b6ec0fcb710@linaro.org>
2024-01-16 19:02     ` Tomer Maimon
     [not found]       ` <64a35c38-38e4-486f-8a07-4aee1d14e05a@linaro.org>
2024-01-22 17:14         ` Tomer Maimon
2024-01-24 14:21           ` Tomer Maimon [this message]
2024-01-08 13:54 ` [PATCH v22 5/8] arm64: dts: nuvoton: npcm8xx: add clock reset syscon node Tomer Maimon
     [not found]   ` <4e9cc473-dbab-4e7c-ac7f-871a4025ef5b@linaro.org>
2024-01-16 19:03     ` Tomer Maimon
2024-01-08 13:54 ` [PATCH v22 6/8] dt-bindings: clock: npcm845: replace reg with syscon property Tomer Maimon
     [not found]   ` <171300da-3d49-4e1f-8969-9a454ecdd698@linaro.org>
2024-01-16 19:37     ` Tomer Maimon
     [not found]       ` <018fc173-edf3-4490-8f29-4059bd17268e@linaro.org>
2024-01-22 17:26         ` Tomer Maimon
2024-01-08 13:54 ` [PATCH v22 7/8] arm64: dts: nuvoton: npcm8xx: " Tomer Maimon
     [not found]   ` <23fdd643-ae30-474b-93b0-fb98edeb0071@linaro.org>
2024-01-16 19:39     ` Tomer Maimon
2024-01-08 13:54 ` [PATCH v22 8/8] clk: npcm8xx: add clock controller Tomer Maimon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAP6Zq1j0-zZvpKRc4T_DPOo=7bewc9u-5NuM48e0Akvrd0KLLQ@mail.gmail.com' \
    --to=tmaimon77@gmail.com \
    --cc=benjaminfair@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=joel@jms.id.au \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=krzysztof.kozlowski@linaro.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=openbmc@lists.ozlabs.org \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=tali.perry1@gmail.com \
    --cc=venture@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).