Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1222403 > unrolled thread

[PATCH v2 1/4] clk: bcm2835: Move under bcm/ with other Broadcom SoC clk drivers.

Started byEric Anholt <eric@anholt.net>
First post2015-09-11 00:00 +0200
Last post2015-09-28 23:20 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH v2 1/4] clk: bcm2835: Move under bcm/ with other Broadcom SoC clk drivers. Eric Anholt <eric@anholt.net> - 2015-09-11 00:00 +0200
    [PATCH v2 2/4] clk: bcm2835: Add binding docs for the new platform clock driver. Eric Anholt <eric@anholt.net> - 2015-09-11 00:00 +0200
      Re: [PATCH v2 2/4] clk: bcm2835: Add binding docs for the new  platform clock driver. Stephen Warren <swarren@wwwdotorg.org> - 2015-09-22 04:10 +0200
    [PATCH v2 4/4] ARM: bcm2835: Switch to using the new clock driver support. Eric Anholt <eric@anholt.net> - 2015-09-11 00:00 +0200
      Re: [PATCH v2 4/4] ARM: bcm2835: Switch to using the new clock driver  support. Stephen Warren <swarren@wwwdotorg.org> - 2015-09-22 04:10 +0200
    Re: [PATCH v2 3/4] clk: bcm2835: Add support for programming the audio domain clocks. Martin Sperl <martin@sperl.org> - 2015-09-11 12:10 +0200
    Re: [PATCH v2 3/4] clk: bcm2835: Add support for programming the  audio domain clocks. Stephen Warren <swarren@wwwdotorg.org> - 2015-09-22 04:10 +0200
      Re: [PATCH v2 3/4] clk: bcm2835: Add support for programming the audio domain clocks. Eric Anholt <eric@anholt.net> - 2015-09-28 23:20 +0200

#1222403 — [PATCH v2 1/4] clk: bcm2835: Move under bcm/ with other Broadcom SoC clk drivers.

FromEric Anholt <eric@anholt.net>
Date2015-09-11 00:00 +0200
Subject[PATCH v2 1/4] clk: bcm2835: Move under bcm/ with other Broadcom SoC clk drivers.
Message-ID<q7i2R-5S9-9@gated-at.bofh.it>
clk-bcm2835.c predates the drivers under bcm/, but all the new BCM
drivers are going in there so let's follow them.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/clk/Makefile                | 1 -
 drivers/clk/bcm/Makefile            | 1 +
 drivers/clk/{ => bcm}/clk-bcm2835.c | 0
 3 files changed, 1 insertion(+), 1 deletion(-)
 rename drivers/clk/{ => bcm}/clk-bcm2835.c (100%)

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index c4cf075..1bdbe9b 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -19,7 +19,6 @@ endif
 obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
-obj-$(CONFIG_ARCH_BCM2835)		+= clk-bcm2835.o
 obj-$(CONFIG_COMMON_CLK_CDCE706)	+= clk-cdce706.o
 obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
 obj-$(CONFIG_ARCH_EFM32)		+= clk-efm32gg.o
diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile
index 8a7a477..ee2349b 100644
--- a/drivers/clk/bcm/Makefile
+++ b/drivers/clk/bcm/Makefile
@@ -3,4 +3,5 @@ obj-$(CONFIG_CLK_BCM_KONA)	+= clk-kona-setup.o
 obj-$(CONFIG_CLK_BCM_KONA)	+= clk-bcm281xx.o
 obj-$(CONFIG_CLK_BCM_KONA)	+= clk-bcm21664.o
 obj-$(CONFIG_COMMON_CLK_IPROC)	+= clk-iproc-armpll.o clk-iproc-pll.o clk-iproc-asiu.o
+obj-$(CONFIG_ARCH_BCM2835)	+= clk-bcm2835.o
 obj-$(CONFIG_ARCH_BCM_CYGNUS)	+= clk-cygnus.o
diff --git a/drivers/clk/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
similarity index 100%
rename from drivers/clk/clk-bcm2835.c
rename to drivers/clk/bcm/clk-bcm2835.c
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]


#1222405 — [PATCH v2 2/4] clk: bcm2835: Add binding docs for the new platform clock driver.

FromEric Anholt <eric@anholt.net>
Date2015-09-11 00:00 +0200
Subject[PATCH v2 2/4] clk: bcm2835: Add binding docs for the new platform clock driver.
Message-ID<q7i2S-5S9-25@gated-at.bofh.it>
In reply to#1222403
Previously we've only supported a few fixed clocks based on
assumptions about how the firmware sets up the clocks, but this
binding will let us control the actual (audio power domain) clock
manager.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 .../bindings/clock/brcm,bcm2835-cprman.txt         | 45 +++++++++++++++++++++
 include/dt-bindings/clock/bcm2835.h                | 47 ++++++++++++++++++++++
 2 files changed, 92 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/brcm,bcm2835-cprman.txt
 create mode 100644 include/dt-bindings/clock/bcm2835.h

diff --git a/Documentation/devicetree/bindings/clock/brcm,bcm2835-cprman.txt b/Documentation/devicetree/bindings/clock/brcm,bcm2835-cprman.txt
new file mode 100644
index 0000000..e56a1df
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/brcm,bcm2835-cprman.txt
@@ -0,0 +1,45 @@
+Broadcom BCM2835 CPRMAN clocks
+
+This binding uses the common clock binding:
+    Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+The CPRMAN clock controller generates clocks in the audio power domain
+of the BCM2835.  There is a level of PLLs deriving from an external
+oscillator, a level of PLL dividers that produce channels off of the
+few PLLs, and a level of mostly-generic clock generators sourcing from
+the PLL channels.  Most other hardware components source from the
+clock generators, but a few (like the ARM or HDMI) will source from
+the PLL dividers directly.
+
+Required properties:
+- compatible:	Should be "brcm,bcm2835-cprman"
+- #clock-cells:	Should be <1>. The permitted clock-specifier values can be
+		  found in include/dt-bindings/clock/bcm2835.h
+- reg:		Specifies base physical address and size of the registers
+- clocks:	The external oscillator clock phandle
+
+Example:
+
+	clk_osc: clock@3 {
+		compatible = "fixed-clock";
+		reg = <3>;
+		#clock-cells = <0>;
+		clock-output-names = "osc";
+		clock-frequency = <19200000>;
+	};
+
+	clocks: cprman@7e101000 {
+		compatible = "brcm,bcm2835-cprman";
+		#clock-cells = <1>;
+		reg = <0x7e101000 0x2000>;
+		clocks = <&clk_osc>;
+	};
+
+	i2c0: i2c@7e205000 {
+		compatible = "brcm,bcm2835-i2c";
+		reg = <0x7e205000 0x1000>;
+		interrupts = <2 21>;
+		clocks = <&clocks BCM2835_CLOCK_VPU>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
diff --git a/include/dt-bindings/clock/bcm2835.h b/include/dt-bindings/clock/bcm2835.h
new file mode 100644
index 0000000..d323efa
--- /dev/null
+++ b/include/dt-bindings/clock/bcm2835.h
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#define BCM2835_PLLA			0
+#define BCM2835_PLLB			1
+#define BCM2835_PLLC			2
+#define BCM2835_PLLD			3
+#define BCM2835_PLLH			4
+
+#define BCM2835_PLLA_CORE		5
+#define BCM2835_PLLA_PER		6
+#define BCM2835_PLLB_ARM		7
+#define BCM2835_PLLC_CORE0		8
+#define BCM2835_PLLC_CORE1		9
+#define BCM2835_PLLC_CORE2		10
+#define BCM2835_PLLC_PER		11
+#define BCM2835_PLLD_CORE		12
+#define BCM2835_PLLD_PER		13
+#define BCM2835_PLLH_RCAL		14
+#define BCM2835_PLLH_AUX		15
+#define BCM2835_PLLH_PIX		16
+
+#define BCM2835_CLOCK_TIMER		17
+#define BCM2835_CLOCK_OTP		18
+#define BCM2835_CLOCK_UART		19
+#define BCM2835_CLOCK_VPU		20
+#define BCM2835_CLOCK_V3D		21
+#define BCM2835_CLOCK_ISP		22
+#define BCM2835_CLOCK_H264		23
+#define BCM2835_CLOCK_VEC		24
+#define BCM2835_CLOCK_HSM		25
+#define BCM2835_CLOCK_SDRAM		26
+#define BCM2835_CLOCK_TSENS		27
+#define BCM2835_CLOCK_EMMC		28
+#define BCM2835_CLOCK_PERI_IMAGE	29
+
+#define BCM2835_CLOCK_COUNT		30
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1229879 — Re: [PATCH v2 2/4] clk: bcm2835: Add binding docs for the new platform clock driver.

FromStephen Warren <swarren@wwwdotorg.org>
Date2015-09-22 04:10 +0200
SubjectRe: [PATCH v2 2/4] clk: bcm2835: Add binding docs for the new platform clock driver.
Message-ID<qblbQ-8bk-9@gated-at.bofh.it>
In reply to#1222405
On 09/10/2015 02:58 PM, Eric Anholt wrote:
> Previously we've only supported a few fixed clocks based on
> assumptions about how the firmware sets up the clocks, but this
> binding will let us control the actual (audio power domain) clock
> manager.

Patches 1 and 2,
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1222406 — [PATCH v2 4/4] ARM: bcm2835: Switch to using the new clock driver support.

FromEric Anholt <eric@anholt.net>
Date2015-09-11 00:00 +0200
Subject[PATCH v2 4/4] ARM: bcm2835: Switch to using the new clock driver support.
Message-ID<q7i2S-5S9-23@gated-at.bofh.it>
In reply to#1222403
This will give us the ability to set the pixel and HDMI state machine
clocks for the VC4 KMS driver, change the CPU frequency, and
potentially gate clocks in the future (once we also write a power
domain driver).  It also gives the uart an explicit clock reference,
so that we don't need to change the physical addresses of the old
fixed clk_bcm2835.c clocks for Raspberry Pi 2 port.

Two clocks get their frequencies updated as a result of this.  One is
uart's apb_pclk, which was previously accidentally grabbing the fixed
uart0_pclk due to the apb_pclk not having clk_register_clkdev()
called.  The uart doesn't seem to do anything with apb_pclk other than
make sure it's on, so that appears safe (also, as far as I can see,
the apb clock is actually the same as the VPU clock).  The other is
EMMC, which according to the docs was supposed to be in the 50-100Mhz
range, but it turns out the firmware needed to change to running it at
the 250Mhz core clock speed to avoid a bug in clock domain crossing.

Additionally, anything using BCM2835_CLOCK_VPU will now have a correct
clock rate if the user configures the boot-time core clock speed using
config.txt.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 arch/arm/boot/dts/bcm2835.dtsi | 52 +++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 301c73f..a6a55b7 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -1,4 +1,5 @@
 #include <dt-bindings/pinctrl/bcm2835.h>
+#include <dt-bindings/clock/bcm2835.h>
 #include "skeleton.dtsi"
 
 / {
@@ -21,6 +22,10 @@
 			compatible = "brcm,bcm2835-system-timer";
 			reg = <0x7e003000 0x1000>;
 			interrupts = <1 0>, <1 1>, <1 2>, <1 3>;
+			/* This could be a reference to BCM2835_CLOCK_TIMER,
+			 * but we don't have the driver using the common clock
+			 * support yet.
+			 */
 			clock-frequency = <1000000>;
 		};
 
@@ -57,6 +62,17 @@
 			reg = <0x7e100000 0x28>;
 		};
 
+		clocks: cprman@7e101000 {
+			compatible = "brcm,bcm2835-cprman";
+			#clock-cells = <1>;
+			reg = <0x7e101000 0x2000>;
+
+			/* CPRMAN derives everything from the platform's
+			 * oscillator.
+			 */
+			clocks = <&clk_osc>;
+		};
+
 		rng@7e104000 {
 			compatible = "brcm,bcm2835-rng";
 			reg = <0x7e104000 0x10>;
@@ -96,7 +112,9 @@
 			compatible = "brcm,bcm2835-pl011", "arm,pl011", "arm,primecell";
 			reg = <0x7e201000 0x1000>;
 			interrupts = <2 25>;
-			clock-frequency = <3000000>;
+			clocks = <&clocks BCM2835_CLOCK_UART>,
+				 <&clocks BCM2835_CLOCK_VPU>;
+			clock-names = "uartclk", "apb_pclk";
 			arm,primecell-periphid = <0x00241011>;
 		};
 
@@ -115,7 +133,7 @@
 			compatible = "brcm,bcm2835-spi";
 			reg = <0x7e204000 0x1000>;
 			interrupts = <2 22>;
-			clocks = <&clk_spi>;
+			clocks = <&clocks BCM2835_CLOCK_VPU>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 			status = "disabled";
@@ -125,7 +143,7 @@
 			compatible = "brcm,bcm2835-i2c";
 			reg = <0x7e205000 0x1000>;
 			interrupts = <2 21>;
-			clocks = <&clk_i2c>;
+			clocks = <&clocks BCM2835_CLOCK_VPU>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 			status = "disabled";
@@ -135,7 +153,7 @@
 			compatible = "brcm,bcm2835-sdhci";
 			reg = <0x7e300000 0x100>;
 			interrupts = <2 30>;
-			clocks = <&clk_mmc>;
+			clocks = <&clocks BCM2835_CLOCK_EMMC>;
 			status = "disabled";
 		};
 
@@ -143,7 +161,7 @@
 			compatible = "brcm,bcm2835-i2c";
 			reg = <0x7e804000 0x1000>;
 			interrupts = <2 21>;
-			clocks = <&clk_i2c>;
+			clocks = <&clocks BCM2835_CLOCK_VPU>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 			status = "disabled";
@@ -165,28 +183,14 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		clk_mmc: clock@0 {
-			compatible = "fixed-clock";
-			reg = <0>;
-			#clock-cells = <0>;
-			clock-output-names = "mmc";
-			clock-frequency = <100000000>;
-		};
-
-		clk_i2c: clock@1 {
+		/* The oscillator is the root of the clock tree. */
+		clk_osc: clock@3 {
 			compatible = "fixed-clock";
-			reg = <1>;
+			reg = <3>;
 			#clock-cells = <0>;
-			clock-output-names = "i2c";
-			clock-frequency = <250000000>;
+			clock-output-names = "osc";
+			clock-frequency = <19200000>;
 		};
 
-		clk_spi: clock@2 {
-			compatible = "fixed-clock";
-			reg = <2>;
-			#clock-cells = <0>;
-			clock-output-names = "spi";
-			clock-frequency = <250000000>;
-		};
 	};
 };
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1229877 — Re: [PATCH v2 4/4] ARM: bcm2835: Switch to using the new clock driver support.

FromStephen Warren <swarren@wwwdotorg.org>
Date2015-09-22 04:10 +0200
SubjectRe: [PATCH v2 4/4] ARM: bcm2835: Switch to using the new clock driver support.
Message-ID<qblbP-8bk-3@gated-at.bofh.it>
In reply to#1222406
On 09/10/2015 02:58 PM, Eric Anholt wrote:
> This will give us the ability to set the pixel and HDMI state machine
> clocks for the VC4 KMS driver, change the CPU frequency, and
> potentially gate clocks in the future (once we also write a power
> domain driver).  It also gives the uart an explicit clock reference,
> so that we don't need to change the physical addresses of the old
> fixed clk_bcm2835.c clocks for Raspberry Pi 2 port.
> 
> Two clocks get their frequencies updated as a result of this.  One is
> uart's apb_pclk, which was previously accidentally grabbing the fixed
> uart0_pclk due to the apb_pclk not having clk_register_clkdev()
> called.  The uart doesn't seem to do anything with apb_pclk other than
> make sure it's on, so that appears safe (also, as far as I can see,
> the apb clock is actually the same as the VPU clock).  The other is
> EMMC, which according to the docs was supposed to be in the 50-100Mhz
> range, but it turns out the firmware needed to change to running it at
> the 250Mhz core clock speed to avoid a bug in clock domain crossing.
> 
> Additionally, anything using BCM2835_CLOCK_VPU will now have a correct
> clock rate if the user configures the boot-time core clock speed using
> config.txt.

Acked-by: Stephen Warren <swarren@wwwdotorg.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1222677 — Re: [PATCH v2 3/4] clk: bcm2835: Add support for programming the audio domain clocks.

FromMartin Sperl <martin@sperl.org>
Date2015-09-11 12:10 +0200
SubjectRe: [PATCH v2 3/4] clk: bcm2835: Add support for programming the audio domain clocks.
Message-ID<q7trj-6Qv-11@gated-at.bofh.it>
In reply to#1222403
> On 10.09.2015, at 23:58, Eric Anholt <eric@anholt.net> wrote:
> 
> This adds support for enabling, disabling, and setting the rate of the
> audio domain clocks.  It will be necessary for setting the pixel clock
> for HDMI in the VC4 driver and let us write a cpufreq driver.  It will
> also improve compatibility with user changes to the firmware's
> config.txt, since our previous fixed clocks are unaware of it.
> 
> The firmware also has support for configuring the clocks through the
> mailbox channel, but the pixel clock setup by the firmware doesn't
> work, and it's Raspberry Pi specific anyway.  The only conflicts we
> should have with the firmware would be if we made firmware calls that
> result in clock management (like opening firmware V3D or ISP access,
> which we don't support in upstream), or on hardware over-thermal or
> under-voltage (when the firmware would rewrite PLLB to take the ARM
> out of overclock).  If that happens, our cached .recalc_rate() results
> would be incorrect, but that's no worse than our current state where
> we used fixed clocks.
> 
> The existing fixed clocks in the code are left in place to provide
> backwards compatibility with old device tree files.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>

This patch-sets sets the emmc clock to correct frequency - no longer
overclocking SDCards by a factor of 2.5.
This means booting from the eMMC on the Compute module works now
without issues.

Note that the DT bcm2835-rpi-b-plus.dtb can get used to boot
the compute module.

Thanks

Tested-by: Martin Sperl <kernel@martin.sperl.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1229876 — Re: [PATCH v2 3/4] clk: bcm2835: Add support for programming the audio domain clocks.

FromStephen Warren <swarren@wwwdotorg.org>
Date2015-09-22 04:10 +0200
SubjectRe: [PATCH v2 3/4] clk: bcm2835: Add support for programming the audio domain clocks.
Message-ID<qblbP-8bk-1@gated-at.bofh.it>
In reply to#1222403
On 09/10/2015 02:58 PM, Eric Anholt wrote:
> This adds support for enabling, disabling, and setting the rate of the
> audio domain clocks.  It will be necessary for setting the pixel clock
> for HDMI in the VC4 driver and let us write a cpufreq driver.  It will
> also improve compatibility with user changes to the firmware's
> config.txt, since our previous fixed clocks are unaware of it.
> 
> The firmware also has support for configuring the clocks through the
> mailbox channel, but the pixel clock setup by the firmware doesn't
> work, and it's Raspberry Pi specific anyway.  The only conflicts we
> should have with the firmware would be if we made firmware calls that
> result in clock management (like opening firmware V3D or ISP access,
> which we don't support in upstream), or on hardware over-thermal or
> under-voltage (when the firmware would rewrite PLLB to take the ARM
> out of overclock).  If that happens, our cached .recalc_rate() results
> would be incorrect, but that's no worse than our current state where
> we used fixed clocks.
> 
> The existing fixed clocks in the code are left in place to provide
> backwards compatibility with old device tree files.

> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c

> +/**
> + * DOC: BCM2835 CPRMAN (clock manager for the "audio" domain)
> + *
> + * The clock tree on the 2835 has several levels.  There's a root
> + * oscillator running at 19.2Mhz.  After the oscillator there are 4

Nit: s/4/5.

> +struct bcm2835_pll_data {
...
> +	/* Highest rate for the VCO before we have to use the
> +	 * pre-divide-by-2.
> +	 */

Nit: /* should be on a line on its own I think. Similar elsewhere.

> +static const struct bcm2835_pll_ana_bits bcm2835_ana_default = {
> +	0,
> +	0,
> +	~((7 << 19) | (15 << 15)),
> +	(2 << 19) | (8 << 15),
> +	~(7 << 7),
> +	(6 << 1),
> +
> +	14

Nit: Elide the blank line? In some other structs too.

Are there #defines or names you can use for those initializers?

> +static int bcm2835_clk_probe(struct platform_device *pdev)

> +	cprman = kzalloc(sizeof(*cprman), GFP_KERNEL);
> +	if (!cprman)
> +		return -ENOMEM;

This function allocates/initializes a lot of resources without using
devm_ versions of APIs.
> +
> +	spin_lock_init(&cprman->regs_lock);
> +	cprman->dev = &pdev->dev;
> +	cprman->regs = of_iomap(dev->of_node, 0);
> +	if (!cprman->regs)
> +		return -ENODEV;

.. consequently if probe() fails later on (such as here) then resources
allocated earlier are not released. probe() should either switch to APIs
such as devm_kzalloc() or add some cleanup handling.

> +	cprman->osc_name = of_clk_get_parent_name(dev->of_node, 0);
> +	if (!cprman->osc_name)
> +		return -ENODEV;
> +
> +	platform_set_drvdata(pdev, cprman);

I'd expect that to happen right after allocating cprman, since it feels
related to having allocated the cprman object.

> +	onecell = kmalloc(sizeof(*onecell), GFP_KERNEL);
> +	if (!onecell)
> +		return -ENOMEM;
> +	onecell->clk_num = BCM2835_CLOCK_COUNT;
> +	onecell->clks = kzalloc(sizeof(*onecell->clks) *
> +				BCM2835_CLOCK_COUNT, GFP_KERNEL);

Does this need to be dynamically allocated? I think you can just make
these fields in struct bcm2835_cprman; even the array of clks could be:

struct clks clks[BCM2835_CLOCK_COUNT];

> +	clks[BCM2835_PLLA] = bcm2835_register_pll(cprman, &bcm2835_plla_data);
> +	clks[BCM2835_PLLB] = bcm2835_register_pll(cprman, &bcm2835_pllb_data);

These can fail; shouldn't there be error-checking? Or, does
of_clk_add_provider() check for this?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1234470 — Re: [PATCH v2 3/4] clk: bcm2835: Add support for programming the audio domain clocks.

FromEric Anholt <eric@anholt.net>
Date2015-09-28 23:20 +0200
SubjectRe: [PATCH v2 3/4] clk: bcm2835: Add support for programming the audio domain clocks.
Message-ID<qdO03-2Tc-15@gated-at.bofh.it>
In reply to#1229876

[Multipart message — attachments visible in raw view] — view raw

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 09/10/2015 02:58 PM, Eric Anholt wrote:
>> This adds support for enabling, disabling, and setting the rate of the
>> audio domain clocks.  It will be necessary for setting the pixel clock
>> for HDMI in the VC4 driver and let us write a cpufreq driver.  It will
>> also improve compatibility with user changes to the firmware's
>> config.txt, since our previous fixed clocks are unaware of it.
>> 
>> The firmware also has support for configuring the clocks through the
>> mailbox channel, but the pixel clock setup by the firmware doesn't
>> work, and it's Raspberry Pi specific anyway.  The only conflicts we
>> should have with the firmware would be if we made firmware calls that
>> result in clock management (like opening firmware V3D or ISP access,
>> which we don't support in upstream), or on hardware over-thermal or
>> under-voltage (when the firmware would rewrite PLLB to take the ARM
>> out of overclock).  If that happens, our cached .recalc_rate() results
>> would be incorrect, but that's no worse than our current state where
>> we used fixed clocks.
>> 
>> The existing fixed clocks in the code are left in place to provide
>> backwards compatibility with old device tree files.
>
>> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c

>> +static const struct bcm2835_pll_ana_bits bcm2835_ana_default = {
>> +	0,
>> +	0,
>> +	~((7 << 19) | (15 << 15)),
>> +	(2 << 19) | (8 << 15),
>> +	~(7 << 7),
>> +	(6 << 1),
>> +
>> +	14
>
> Nit: Elide the blank line? In some other structs too.
>
> Are there #defines or names you can use for those initializers?

I've just used named initializers instad.  (These *are* the defines, so
another indirection wouldn't help anyone reading the driver).

>> +	clks[BCM2835_PLLA] = bcm2835_register_pll(cprman, &bcm2835_plla_data);
>> +	clks[BCM2835_PLLB] = bcm2835_register_pll(cprman, &bcm2835_pllb_data);
>
> These can fail; shouldn't there be error-checking? Or, does
> of_clk_add_provider() check for this?

When you get the clk from the of provider, the error gets passed
through, like it should.

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web