From: Brendan Higgins <brendanhiggins@google.com> To: Tali Perry <tali.perry1@gmail.com> Cc: " Tomer Maimon" <tmaimon77@gmail.com>, "Michael Turquette" <mturquette@baylibre.com>, sboyd@codeaurora.org, "Rob Herring" <robh+dt@kernel.org>, "Mark Rutland" <mark.rutland@arm.com>, "Avi Fishman" <avifishman70@gmail.com>, "Joel Stanley" <joel@jms.id.au>, linux-clk@vger.kernel.org, devicetree <devicetree@vger.kernel.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, "OpenBMC Maillist" <openbmc@lists.ozlabs.org> Subject: Re: [PATCH v1 2/2] clk: npcm: add NPCM7xx clock driver Date: Thu, 8 Feb 2018 16:07:44 -0800 [thread overview] Message-ID: <CAFd5g44vTyOciaT5+YcVOdxd0=7sxWb3r+gO6_DFv3bmS=EL-w@mail.gmail.com> (raw) In-Reply-To: <CAHb3i=uvj_KS4tZ_qZjQdMZ2CCg9_+11FbmvmF4D5DFGtaAW=g@mail.gmail.com> <snip> >> + >> + /* Define fixed clocks. >> + * Notice: the following clocks are fixed value on NPCM7XX and should >> + * not be changed. >> + * therefor they are not exposed to the dev tree . > > I am not convinced. The top level .dtsi is usually SoC specific. > > - CLKREF should be fixed on 25MHz. I didn't want to put it in DT since > it might appear as something that is changeable. > on npcm750 all clocks are set in ROM +BB (several possible settings) > , so this entire driver is only used for reading the current clock > settings. All clocks are read only and derived from CLKREF. CLKREF is > fixed so no point in putting it in DT and exposing it outside of this > driver. I understand your rationale, but the *.dtsi is typically used to describe the SoC, things that cannot change. For example, consider arch/arm/boot/dts/nuvoton-npcm750.dtsi, it describes the CPU cores, cache hierarchy, peripherals and what busses they reside on, memory layout, etc. None of these things are configurable; the *.dtsi just describes the SoC. Typically, a *.dts is made for a particular board and imports a *.dtsi and does some configuration, but it is not supposed to override things like what I listed above (amoung other things). There are several reasons that you put this information that cannot change in a *.dtsi. For one it is a convenient, all in one place description of the SoC that serves as a form of documentation. But more importantly, if you were to create a variation of the 750 with a different number of cores, a different memory layout, a different reference clock, etc, you could potentiallly use the same drivers for both SoCs, and you would only have to write a new *.dtsi. I know you may have no intention to do this, but many other CPU vendors do. And for this reason, this is how drivers are expected to be written. I hope this makes sense. > >> + */ >> + pr_debug("\tclk register fixed clocks\n"); >> + hw = clk_hw_register_fixed_rate(NULL, NPCM7XX_CLK_S_REFCLK, >> + NULL, 0, 25000000); >> + hw = clk_hw_register_fixed_rate(NULL, NPCM7XX_CLK_S_SYSBYPCK, >> + NULL, 0, 800000000); // rarely used. mostly testing. TBD: remove >> + hw = clk_hw_register_fixed_rate(NULL, NPCM7XX_CLK_S_MCBYPCK, >> + NULL, 0, 800000000); // rarely used. mostly testing. TBD:remove >> + >> + <snip>
WARNING: multiple messages have this Message-ID (diff)
From: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> To: Tali Perry <tali.perry1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Cc: " Tomer Maimon" <tmaimon77-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, "Michael Turquette" <mturquette-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>, sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, "Rob Herring" <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, "Mark Rutland" <mark.rutland-5wv7dgnIgG8@public.gmane.org>, "Avi Fishman" <avifishman70-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, "Joel Stanley" <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>, linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "Linux Kernel Mailing List" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "OpenBMC Maillist" <openbmc-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org> Subject: Re: [PATCH v1 2/2] clk: npcm: add NPCM7xx clock driver Date: Thu, 8 Feb 2018 16:07:44 -0800 [thread overview] Message-ID: <CAFd5g44vTyOciaT5+YcVOdxd0=7sxWb3r+gO6_DFv3bmS=EL-w@mail.gmail.com> (raw) In-Reply-To: <CAHb3i=uvj_KS4tZ_qZjQdMZ2CCg9_+11FbmvmF4D5DFGtaAW=g@mail.gmail.com> <snip> >> + >> + /* Define fixed clocks. >> + * Notice: the following clocks are fixed value on NPCM7XX and should >> + * not be changed. >> + * therefor they are not exposed to the dev tree . > > I am not convinced. The top level .dtsi is usually SoC specific. > > - CLKREF should be fixed on 25MHz. I didn't want to put it in DT since > it might appear as something that is changeable. > on npcm750 all clocks are set in ROM +BB (several possible settings) > , so this entire driver is only used for reading the current clock > settings. All clocks are read only and derived from CLKREF. CLKREF is > fixed so no point in putting it in DT and exposing it outside of this > driver. I understand your rationale, but the *.dtsi is typically used to describe the SoC, things that cannot change. For example, consider arch/arm/boot/dts/nuvoton-npcm750.dtsi, it describes the CPU cores, cache hierarchy, peripherals and what busses they reside on, memory layout, etc. None of these things are configurable; the *.dtsi just describes the SoC. Typically, a *.dts is made for a particular board and imports a *.dtsi and does some configuration, but it is not supposed to override things like what I listed above (amoung other things). There are several reasons that you put this information that cannot change in a *.dtsi. For one it is a convenient, all in one place description of the SoC that serves as a form of documentation. But more importantly, if you were to create a variation of the 750 with a different number of cores, a different memory layout, a different reference clock, etc, you could potentiallly use the same drivers for both SoCs, and you would only have to write a new *.dtsi. I know you may have no intention to do this, but many other CPU vendors do. And for this reason, this is how drivers are expected to be written. I hope this makes sense. > >> + */ >> + pr_debug("\tclk register fixed clocks\n"); >> + hw = clk_hw_register_fixed_rate(NULL, NPCM7XX_CLK_S_REFCLK, >> + NULL, 0, 25000000); >> + hw = clk_hw_register_fixed_rate(NULL, NPCM7XX_CLK_S_SYSBYPCK, >> + NULL, 0, 800000000); // rarely used. mostly testing. TBD: remove >> + hw = clk_hw_register_fixed_rate(NULL, NPCM7XX_CLK_S_MCBYPCK, >> + NULL, 0, 800000000); // rarely used. mostly testing. TBD:remove >> + >> + <snip> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-02-09 0:07 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-05 8:22 [PATCH v1 0/2] clk: npcm: add NPCM7xx clock driver Tomer Maimon 2018-02-05 8:22 ` [PATCH v1 1/2] dt-binding: clock: document NPCM7xx clock DT bindings Tomer Maimon 2018-02-05 8:22 ` Tomer Maimon 2018-02-09 2:22 ` Rob Herring 2018-02-09 2:22 ` Rob Herring 2018-02-05 8:22 ` [PATCH v1 2/2] clk: npcm: add NPCM7xx clock driver Tomer Maimon 2018-02-05 19:37 ` Brendan Higgins 2018-02-05 19:37 ` Brendan Higgins 2018-02-06 8:10 ` Tali Perry 2018-02-06 8:46 ` Fwd: " Tali Perry 2018-02-06 8:46 ` Tali Perry 2018-02-09 0:07 ` Brendan Higgins [this message] 2018-02-09 0:07 ` Brendan Higgins
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='CAFd5g44vTyOciaT5+YcVOdxd0=7sxWb3r+gO6_DFv3bmS=EL-w@mail.gmail.com' \ --to=brendanhiggins@google.com \ --cc=avifishman70@gmail.com \ --cc=devicetree@vger.kernel.org \ --cc=joel@jms.id.au \ --cc=linux-clk@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mturquette@baylibre.com \ --cc=openbmc@lists.ozlabs.org \ --cc=robh+dt@kernel.org \ --cc=sboyd@codeaurora.org \ --cc=tali.perry1@gmail.com \ --cc=tmaimon77@gmail.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.