clk: Sigma Designs Tango4 cpuclk driver
PATCH 1/1 clk: Sigma Designs Tango4 cpuclk driver
Date: Tue, 6 Oct 2015 16:07:45 +0200
Subject: [PATCH] clk: Sigma Designs Tango4 cpuclk driver
--- drivers/clk/Makefile | 1 + drivers/clk/clk-tango4.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 drivers/clk/clk-tango4.c
drivers/clk/Makefile +1 -0
@@ -40,6 +40,7 @@ obj-$(CONFIG_COMMON_CLK_SI5351) += clk-si5351.o obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o obj-$(CONFIG_COMMON_CLK_CDCE925) += clk-cdce925.o obj-$(CONFIG_ARCH_STM32) += clk-stm32f4.o +obj-$(CONFIG_ARCH_TANGOX) += clk-tango4.o obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o obj-$(CONFIG_ARCH_U300) += clk-u300.o obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
drivers/clk/clk-tango4.c new file +59 -0
59 lines not on this page · raw message
Signature · 1 line
2.4.5
Quoted 2 lines · Marc Gonzalez
Date: Tue, 6 Oct 2015 16:07:45 +0200
Subject: [PATCH] clk: Sigma Designs Tango4 cpuclk driver
This part doesn't go here. Please fix your mailer. Also, please add some commit text.
Quoted 5 lines
Signed-off-by Marc Gonzalez <[email protected]>--- drivers/clk/Makefile | 1 + drivers/clk/clk-tango4.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
Is there a DT binding document somewhere?
drivers/clk/clk-tango4.c line 2
Quoted 11 lines
2 files changed, 60 insertions(+) create mode 100644 drivers/clk/clk-tango4.cdrivers/clk/clk-tango4.c new file +2
-0@@ -0,0 +1,59 @@ +#include <linux/clk-provider.h> +#include <linux/of_address.h>
We need a few more includes here for iomap, __init, readl().
drivers/clk/clk-tango4.c line 6
Quoted 4 lines
drivers/clk/clk-tango4.c new file +4
-0+ +#define REG (name, ...) union name { struct { u32 __VA_ARGS__; }; u32 val; } + +REG(SYS_clkgen_pll, N:7, :6, K:3, M:3, :5, Isel:3, :3, T:1, B:1);
This is new to me. Using bitfields like this is not really a good idea though. Please just use masks, shifts, etc. instead.
Quoted 23 lines
drivers/clk/clk-tango4.c new file +23
-0+/* +* CG0, CG1, CG2, CG3 PLL Control: +* ------------------------------- +* +* | Byte 3 | Byte 2 | Byte 1 | Byte 0 | +* |3 3 2 2 2 2 2 2|2 2 2 2 1 1 1 1|1 1 1 1 1 1 | | +* |1 0 9 8 7 6 5 4|3 2 1 0 9 8 7 6|5 4 3 2 1 0 9 8|7 6 5 4 3 2 1 0| +* |-|-|-----|-----|---------|-----|-----|---------|-|-------------| +* |B|T|xxxxx|Isel |xxxxxxxxx| M | K |xxxxxxxxx|x| N | +* |-|-|-----|-----|---------|-----|-----|---------|-|-------------| +* +* These registers are used to configure the PLL parameters: +* +* Bits 6 to 0: N[6:0]. Default = 29 +* Bits 15 to 13: K[2:0]. Default = 1 +* Bit 18 to 16: M[2:0]. Default = 0 +* Bits 26 to 24: Isel[2:0] (PLL Input Select). Default = 1 +* Bits 30 : T (PLL Test). Default = 0 +* Bits 31 : B (PLL Bypass). Default = 0 +* +* PLL0 : Out = In * (N+1) / (M+1) / 2^K +* PLL1 : Same as PLL0 +* PLL2 : Same as PLL0
drivers/clk/clk-tango4.c line 39
Quoted 10 lines
drivers/clk/clk-tango4.c new file +10
-0+* Default values : All PLLs configured to output 405MHz. +*/ +static void __init tango4_pll_setup (struct device_node *np) +{ +unsigned int mul, div; +union SYS_clkgen_pll pll; +const char *name = np->name; +const char *parent =of_clk_get_parent_name (np,0 ); + +void __iomem *clkgen_pll =of_iomap (np,0 );
What if of_iomap() fails?
drivers/clk/clk-tango4.c line 43
Quoted 4 lines
drivers/clk/clk-tango4.c new file +4
-0+ pll.val =readl_relaxed (clkgen_pll); +iounmap (clkgen_pll); + + mul = (pll.N +1 );
Parenthesis are useless, please remove.
Quoted 6 lines
drivers/clk/clk-tango4.c new file +6
-0+ div = (pll.M +1 ) << pll.K; +clk_register_fixed_factor (NULL , name, parent,0 , mul, div); +} + +static void __init tango4_div_setup (struct device_node *np) +{
drivers/clk/clk-tango4.c line 59
Quoted 10 lines
drivers/clk/clk-tango4.c new file +10
-0+const char *name = np->name; +const char *parent =of_clk_get_parent_name (np,0 ); +void __iomem *div_ctrl =of_iomap (np,0 ); + +clk_register_divider (NULL , name, parent,0 , + div_ctrl,8 ,8 ,CLK_DIVIDER_ONE_BASED ,NULL ); +} + +CLK_OF_DECLARE (tango4_pll,"sigma,tango4-pll" , tango4_pll_setup); +CLK_OF_DECLARE (tango4_cpuclk,"sigma,tango4-cpuclk" , tango4_div_setup);
More discussion will come with the binding, but we're pushing people towards making real platform drivers for their clock controllers, instead of parsing everything out of DT and having one node per clock. So if these are picking things out of some larger clock controller block, please rewrite the binding to be a real clock provider.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 7 lines · Stephen Boyd
Quoted 2 lines · Marc Gonzalez
Date: Tue, 6 Oct 2015 16:07:45 +0200
Subject: [PATCH] clk: Sigma Designs Tango4 cpuclk driverThis part doesn't go here. Please fix your mailer. Also, please add some commit text.
Sorry, I misread the instructions. Next submission will be properly formatted.
Could you point out the most recent driver additions, so that I may copy their log style?
Quoted 4 lines
Quoted 2 lines
drivers/clk/Makefile | 1 + drivers/clk/clk-tango4.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++Is there a DT binding document somewhere?
I assume this is a roundabout request for said document? :-)
Here's the actual clock tree DT I'm using:
+ clocks {
+ ranges;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ xtal: xtal {
+ compatible = "fixed-clock";
+ clock-frequency = <27000000>;
+ #clock-cells = <0>;
+ };
+
+ pll0: pll0 {
+ compatible = "sigma,tango4-pll";
+ reg = <0x10000 4>;
+ clocks = <&xtal>;
+ #clock-cells = <0>;
+ };
+
+ pll1: pll1 {
+ compatible = "sigma,tango4-pll";
+ reg = <0x10008 4>;
+ clocks = <&xtal>;
+ #clock-cells = <0>;
+ };
+
+ cpuclk: cpuclk {
+ compatible = "sigma,tango4-cpuclk";
+ reg = <0x10024 4>;
+ clocks = <&pll0>;
+ #clock-cells = <0>;
+ };
+
+ periphclk: periphclk {
+ compatible = "fixed-factor-clock";
+ clocks = <&cpuclk>;
+ clock-mult = <1>;
+ clock-div = <2>;
+ #clock-cells = <0>;
+ };
+
+ sysclk: sysclk {
+ compatible = "fixed-factor-clock";
+ clocks = <&pll1>;
+ clock-mult = <1>;
+ clock-div = <3>; /* HW bug precludes other dividers */
+ #clock-cells = <0>;
+ };
+ };
"sigma,tango4-pll" requires two properties:
- reg: the address of the relevant clkgen register (size 4)
- clocks: the input clock (must be the crystal oscillator) (I don't know if "#clock-cells = <0>;" is required?)
"sigma,tango4-cpuclk" requires two properties:
- reg: the address of the clkdiv register (size 4)
- clocks: the input clock (always pll0)
(I don't know if "#clock-cells = <0>;" is required?)
IIUC, I should provide this documentation in my patch? (Will probably require an iteration or two to work out the proper format.)
Quoted 13 lines
drivers/clk/clk-tango4.c line 2
Quoted 11 lines
2 files changed, 60 insertions(+) create mode 100644 drivers/clk/clk-tango4.cdrivers/clk/clk-tango4.c new file +2
-0@@ -0,0 +1,59 @@ +#include <linux/clk-provider.h> +#include <linux/of_address.h> We need a few more includes here for iomap,
__init,readl().
OK.
<linux/clk-provider.h>
<linux/of_address.h>
<linux/init.h>
<linux/io.h>
Quoted 6 lines
drivers/clk/clk-tango4.c line 6
Quoted 3 lines
drivers/clk/clk-tango4.c new file +3
-0+#define REG (name, ...) union name { struct { u32 __VA_ARGS__; }; u32 val; } + +REG(SYS_clkgen_pll, N:7, :6, K:3, M:3, :5, Isel:3, :3, T:1, B:1);This is new to me. Using bitfields like this is not really a good idea though. Please just use masks, shifts, etc. instead.
You don't say /why/ it's not a good idea ;-)
The typical objection to bit-fields is that they are not portable. As far as the compiler is concerned, the kernel community seems to have "standardized" on gcc, for its convenient extensions.
<parenthesis>
Some people want to compile the kernel with icc or clang, but there are several pit-falls.
https://llvm.linuxfoundation.org/index.php/Main_Page
https://software.intel.com/sites/default/files/article/146679/linuxkernelbuildwhitepaper.pdf
</parenthesis>
"bit-fields in gcc" are more easy to deal with than bit-fields in general. Their behavior is specified here:
https://gcc.gnu.org/onlinedocs/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html
Since this driver is only intended to be compiled for arm (if ARCH_TANGOX), non-portability is not an issue. In gcc, the order of allocation of bit-fields within a unit is "Determined by [the] ABI". (I'm using EABI)
I've also relied on this gcc extension:
https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html
to be able to write e.g. pll.N
(It's used elsewhere in the kernel)
What I like about the implementation using bit-fields is that one only needs specify the layout, and computing the offsets is left to the compiler (which is less error-prone). Also the definition is short, and I find that the intent of
pll.K
is clearer than
(pll >> 13) & 7
[...] That being said, if I must forgo bit-fields to get this driver accepted, I can write:
#define PLL_N(val) ((val) >> 0 & 0x7f)
#define PLL_K(val) ((val) >> 13 & 0x07)
#define PLL_M(val) ((val) >> 16 & 0x07)
Is that the preferred way?
Quoted 10 lines
drivers/clk/clk-tango4.c line 39
Quoted 8 lines
drivers/clk/clk-tango4.c new file +8
-0+static void __init tango4_pll_setup (struct device_node *np) +{ +unsigned int mul, div; +union SYS_clkgen_pll pll; +const char *name = np->name; +const char *parent =of_clk_get_parent_name (np,0 ); + +void __iomem *clkgen_pll =of_iomap (np,0 );What if
of_iomap()fails?
If of_iomap() fails, the system will panic when it tries to read from address 0.
I can make it explicit, if you prefer:
if (clkgen_pll == NULL)
panic("%s: invalid reg property\n", np->full_name);Quoted 6 lines
drivers/clk/clk-tango4.c line 43
Quoted 4 lines
drivers/clk/clk-tango4.c new file +4
-0+ pll.val =readl_relaxed (clkgen_pll); +iounmap (clkgen_pll); + + mul = (pll.N +1 );Parenthesis are useless, please remove.
They are aesthetic, to align the mul and div expressions.
Quoted 15 lines
Quoted 15 lines
drivers/clk/clk-tango4.c new file +15
-0+ div = (pll.M +1 ) << pll.K; +clk_register_fixed_factor (NULL , name, parent,0 , mul, div); +} + +static void __init tango4_div_setup (struct device_node *np) +{ +const char *name = np->name; +const char *parent =of_clk_get_parent_name (np,0 ); +void __iomem *div_ctrl =of_iomap (np,0 ); + +clk_register_divider (NULL , name, parent,0 , + div_ctrl,8 ,8 ,CLK_DIVIDER_ONE_BASED ,NULL ); +} + +CLK_OF_DECLARE (tango4_pll,"sigma,tango4-pll" , tango4_pll_setup);
Quoted 8 lines
drivers/clk/clk-tango4.c line 59
Quoted 1 line
drivers/clk/clk-tango4.c new file +1
-0+CLK_OF_DECLARE (tango4_cpuclk,"sigma,tango4-cpuclk" , tango4_div_setup);More discussion will come with the binding, but we're pushing people towards making real platform drivers for their clock controllers, instead of parsing everything out of DT and having one node per clock. So if these are picking things out of some larger clock controller block, please rewrite the binding to be a real clock provider.
I don't understand what you wrote.
Could you explain what you meant?
Regards.
Quoted 9 lines · Mason
Quoted 6 lines · Stephen Boyd
More discussion will come with the binding, but we're pushing people towards making real platform drivers for their clock controllers, instead of parsing everything out of DT and having one node per clock. So if these are picking things out of some larger clock controller block, please rewrite the binding to be a real clock provider.
Quoted 3 lines
I don't understand what you wrote.
Could you explain what you meant?
While the other issues are minor, I will need guidance to understand this request.
Regards.
PATCH v2 1/1 clk: tango4: clkgen driver for Tango4 ARM platforms
In reply to Stephen BoydProvide support for Sigma Designs Tango4 (ARM-based) clock generator.
NOTE: This driver is INCOMPATIBLE with Tango3 clkgen.
---
Changes in v2:
Provide missing includes
Use masks instead of bit-fields :-(
Error handling
Style nits
Model the clkgen block as a single node
--- drivers/clk/Makefile | 1 + drivers/clk/clk-tango4.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 drivers/clk/clk-tango4.c
drivers/clk/Makefile +1 -0
@@ -40,6 +40,7 @@ obj-$(CONFIG_COMMON_CLK_SI5351) += clk-si5351.o obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o obj-$(CONFIG_COMMON_CLK_CDCE925) += clk-cdce925.o obj-$(CONFIG_ARCH_STM32) += clk-stm32f4.o +obj-$(CONFIG_ARCH_TANGOX) += clk-tango4.o obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o obj-$(CONFIG_ARCH_U300) += clk-u300.o obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
drivers/clk/clk-tango4.c new file +49 -0
49 lines not on this page · raw message
Signature · 1 line
2.4.5
Re: [PATCH v2] clk: tango4: clkgen driver for Tango4 ARM platforms
Quoted 11 lines · Marc Gonzalez
Provide support for Sigma Designs Tango4 (ARM-based) clock generator.
NOTE: This driver is INCOMPATIBLE with Tango3 clkgen.Signed-off-by Marc Gonzalez <[email protected]>---
Changes in v2:
Provide missing includes
Use masks instead of bit-fields :-(
Error handling
Style nits
Model the clkgen block as a single node
And the corresponding DT will be along these lines:
clocks {
ranges;
#address-cells = <1>;
#size-cells = <1>;
xtal: xtal {
compatible = "fixed-clock";
clock-frequency = <27000000>;
#clock-cells = <0>;
};
clkgen: clkgen@10000 {
compatible = "sigma,tango4-clkgen";
reg = <0x10000 0x30>;
clocks = <&xtal>;
clock-output-names = "cpuclk", "sysclk";
#clock-cells = <1>;
};
periphclk: periphclk {
compatible = "fixed-factor-clock";
clocks = <&clkgen 0>;
clock-mult = <1>;
clock-div = <2>;
#clock-cells = <0>;
};
};Regards.