[PATCH v2 5/6] clk: samsung: add clock-driver for s3c2416, s3c2443 and s3c2450

Tomasz Figa tomasz.figa at gmail.com
Thu Jul 11 05:13:55 EDT 2013


Hi Heiko,

Looks mostly good, but please see my comments inline.

On Wednesday 10 of July 2013 01:00:00 Heiko Stübner wrote:
> The three SoCs share a common clock tree which only differs in the
> existence of some special clocks.
> 
> As with all parts common to these three SoCs the driver is named
> after the s3c2443, as it was the first SoC introducing this structure
> and there exists no other label to describe this s3c24xx epoch.
> 
> The clock structure is built according to the manuals of the included
> SoCs and might include changes in comparison to the previous clock
> structure. As an example the sclk_uart gate was never handled previously
> and the div_uart was made to be the clock used by the serial driver.
> 
> Signed-off-by: Heiko Stuebner <heiko at sntech.de>
> ---
>  .../bindings/clock/samsung,s3c2443-clock.txt       |   48 +++
>  drivers/clk/Kconfig                                |    1 +
>  drivers/clk/samsung/Kconfig                        |    2 +
>  drivers/clk/samsung/Makefile                       |    1 +
>  drivers/clk/samsung/clk-s3c2443.c                  |  422
> ++++++++++++++++++++ include/dt-bindings/clock/samsung,s3c2443-clock.h  |  
> 96 +++++
>  6 files changed, 570 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/clock/samsung,s3c2443-clock.txt create
> mode 100644 drivers/clk/samsung/Kconfig
>  create mode 100644 drivers/clk/samsung/clk-s3c2443.c
>  create mode 100644 include/dt-bindings/clock/samsung,s3c2443-clock.h
> 
> diff --git
> a/Documentation/devicetree/bindings/clock/samsung,s3c2443-clock.txt
> b/Documentation/devicetree/bindings/clock/samsung,s3c2443-clock.txt new
> file mode 100644
> index 0000000..a61d8d1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/samsung,s3c2443-clock.txt
> @@ -0,0 +1,48 @@
> +* Samsung S3C2443 Clock Controller
> +
> +The S3C2443 clock controller generates and supplies clock to various
> controllers +within the SoC. The clock binding described here is applicable
> to all SoCs in +the s3c24x family starting with the s3c2443.
> +
> +Required Properties:
> +
> +- comptible: should be one of the following.

nit: compatible

> +  - "samsung,s3c2416-clock" - controller compatible with S3C2416 SoC.
> +  - "samsung,s3c2443-clock" - controller compatible with S3C2443 SoC.
> +  - "samsung,s3c2450-clock" - controller compatible with S3C2450 SoC.
> +
> +- reg: physical base address of the controller and length of memory mapped
> +  region.
> +
> +- #clock-cells: should be 1.
> +
> +Each clock is assigned an identifier and client nodes can use this
> identifier +to specify the clock which they consume. Some of the clocks are
> available only +on a particular SoC.
> +
> +All available clocks are defined as preprocessor macros in
> +dt-bindings/clock/samsung,s3c2443-clock.h header and can be used in device
> +tree sources.
> +
> +Example: Clock controller node:
> +
> +	clocks: clock-controller at 4c000000 {
> +		compatible = "samsung,s3c2416-clock";
> +		reg = <0x4c000000 0x40>;
> +		#clock-cells = <1>;
> +	};
> +
> +Example: UART controller node that consumes the clock generated by the
> clock +  controller (refer to the standard clock bindings for information
> about +  "clocks" and "clock-names" properties):
> +
> +	serial at 50004000 {
> +		compatible = "samsung,s3c2440-uart";
> +		reg = <0x50004000 0x4000>;
> +		interrupts = <1 23 3 4>, <1 23 4 4>;
> +		clock-names = "uart", "clk_uart_baud2",
> +				"clk_uart_baud3";
> +		clocks = <&clocks PCLK_UART0>, <&clocks PCLK_UART0>,
> +				<&clocks SCLK_UART>;
> +		status = "disabled";
> +	};
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 0357ac4..b2fdd68 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -84,3 +84,4 @@ config COMMON_CLK_AXI_CLKGEN
>  endmenu
> 
>  source "drivers/clk/mvebu/Kconfig"
> +source "drivers/clk/samsung/Kconfig"
> diff --git a/drivers/clk/samsung/Kconfig b/drivers/clk/samsung/Kconfig
> new file mode 100644
> index 0000000..0cfbb29
> --- /dev/null
> +++ b/drivers/clk/samsung/Kconfig
> @@ -0,0 +1,2 @@
> +config COMMON_CLK_S3C2443
> +       bool

Do you need to introduce a new Kconfig file for this? I guess it's just a matter 
of preference, but since it's here just temporarily, I would just put this 
Kconfig entry into the top level clk Kconfig file or even in arch/arm/mach-
s3c24xx/Kconfig.

> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> index 5d4d432..1c7932c 100644
> --- a/drivers/clk/samsung/Makefile
> +++ b/drivers/clk/samsung/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_SOC_EXYNOS5250)	+= clk-exynos5250.o
>  obj-$(CONFIG_SOC_EXYNOS5420)	+= clk-exynos5420.o
>  obj-$(CONFIG_SOC_EXYNOS5440)	+= clk-exynos5440.o
>  obj-$(CONFIG_ARCH_EXYNOS)	+= clk-exynos-audss.o
> +obj-$(CONFIG_COMMON_CLK_S3C2443)+= clk-s3c2443.o
> diff --git a/drivers/clk/samsung/clk-s3c2443.c
> b/drivers/clk/samsung/clk-s3c2443.c new file mode 100644
> index 0000000..7d57b08
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-s3c2443.c
> @@ -0,0 +1,422 @@
> +/*
> + * Copyright (c) 2013 Heiko Stuebner <heiko at sntech.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Common Clock Framework support for S3C2443 and following SoCs.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +
> +#include <dt-bindings/clock/samsung,s3c2443-clock.h>
> +
> +#include "clk.h"
> +#include "clk-pll.h"
> +
> +/* S3C2416 clock controller register offsets */
> +#define MPLLCON		0x10
> +#define EPLLCON		0x18
> +#define EPLLCON_K	0x1C
> +#define CLKSRC		0x20
> +#define CLKDIV0		0x24
> +#define CLKDIV1		0x28
> +#define CLKDIV2		0x2C
> +#define HCLKCON		0x30
> +#define PCLKCON		0x34
> +#define SCLKCON		0x38
> +
> +/* the soc types */
> +enum supported_socs {
> +	S3C2416,
> +	S3C2443,
> +	S3C2450,
> +};
> +
> +/*
> + * list of controller registers to be saved and restored during a
> + * suspend/resume cycle.
> + */
> +static __initdata unsigned long s3c2443_clk_regs[] = {
> +	MPLLCON,
> +	EPLLCON,
> +	EPLLCON_K,
> +	CLKSRC,
> +	CLKDIV0,
> +	CLKDIV1,
> +	CLKDIV2,
> +	PCLKCON,
> +	HCLKCON,
> +	SCLKCON,
> +};
> +
> +PNAME(epllref_p) = { "mpllref", "mpllref", "xti", "ext" };
> +PNAME(esysclk_p) = { "epllref", "epll" };
> +PNAME(msysclk_p) = { "mpllref, mdivclk", "mpll", "mpll" };
> +PNAME(armclk_p) = { "armdiv" , "hclk" };
> +PNAME(i2s0_p) = { "div_i2s0", "ext_i2s", "epllref", "epllref" };
> +
> +/* fixed rate clocks generated outside the soc */
> +struct samsung_fixed_rate_clock s3c2443_common_frate_clks[] __initdata = {
> +	FRATE(XTI, "xti", NULL, CLK_IS_ROOT, 0),
> +	FRATE(EXT, "ext", NULL, CLK_IS_ROOT, 0),
> +	FRATE(EXT_I2S, "ext_i2s", NULL, CLK_IS_ROOT, 0),
> +	FRATE(EXT_UART, "ext_uart", NULL, CLK_IS_ROOT, 0),
> +};
> +
> +/* mpllref is a direct descendant of clk_xtal by default, but it is not

nit: The preferred style of multiline comments is:

/*
 * mpllref is a...

> + * elided as the EPLL can be either sourced by the XTAL or EXTCLK and as
> + * such directly equating the two source clocks is impossible.
> + */
> +struct samsung_fixed_factor_clock s3c2443_common_ffactor[] __initdata = {
> +	FFACTOR(0, "mpllref", "xti", 1, 1, 0),
> +};

Hmm, I don't understand the reason for having this fixed factor clock. Why 
can't xti be used directly instead?

> +struct samsung_mux_clock s3c2443_common_muxes[] __initdata = {
> +	MUX(0, "epllref", epllref_p, CLKSRC, 7, 2),
> +	MUX(ESYSCLK, "esysclk", esysclk_p, CLKSRC, 6, 1),
> +	MUX_A(MSYSCLK, "msysclk", msysclk_p, CLKSRC, 3, 2, "msysclk"),
> +	MUX_A(ARMCLK, "armclk", armclk_p, CLKDIV0, 13, 1, "armclk"),
> +	MUX(0, "mux_i2s0", i2s0_p, CLKSRC, 14, 2),
> +};
> +
> +static struct clk_div_table hclk_d[] = {
> +	{ .val = 0, .div = 1 },
> +	{ .val = 1, .div = 2 },
> +	{ .val = 3, .div = 4 },
> +	{ .div = 0 },
> +};

Hmm, this makes me rethink some of the code I made for S3C64xx. There are few 
dividers that have additional constraints, like only even divisors, or so, but 
they are just normal linear dividers, so can be registered as simple divider 
clocks.

Originally I registered them as simple dividers, but now I'm thinking if they 
shouldn't be constrained by a table.

> +static struct clk_div_table mdivclk_d[] = {
> +	{ .val = 0, .div = 1 },
> +	{ .val = 1, .div = 3 },
> +	{ .val = 2, .div = 5 },
> +	{ .val = 3, .div = 7 },
> +	{ .val = 4, .div = 9 },
> +	{ .val = 5, .div = 11 },
> +	{ .val = 6, .div = 13 },
> +	{ .val = 7, .div = 15 },
> +	{ .div = 0 },
> +};
> +
> +struct samsung_div_clock s3c2443_common_dividers[] __initdata = {
> +	DIV_T(0, "mdivclk", "mpllref", CLKDIV0, 6, 3, mdivclk_d),
> +	DIV(0, "prediv", "msysclk", CLKDIV0, 4, 2),
> +	DIV_T(HCLK, "hclk", "prediv", CLKDIV0, 0, 2, hclk_d),
> +	DIV(PCLK, "pclk", "hclk", CLKDIV0, 2, 1),
> +	DIV(0, "div_hsspi0_epll", "esysclk", CLKDIV1, 24, 2),
> +	DIV(0, "div_fimd", "esysclk", CLKDIV1, 16, 8),
> +	DIV(0, "div_i2s0", "esysclk", CLKDIV1, 12, 4),
> +	DIV(0, "div_uart", "esysclk", CLKDIV1, 8, 4),
> +	DIV(0, "div_hsmmc1", "esysclk", CLKDIV1, 6, 2),
> +	DIV(0, "div_usbhost", "esysclk", CLKDIV1, 4, 2),
> +};
> +
> +struct samsung_gate_clock s3c2443_common_gates[] __initdata = {
> +	GATE(SCLK_HSMMC_EXT, "sclk_hsmmcext", "ext", SCLKCON, 13, 0, 0),
> +	GATE(SCLK_HSMMC1, "sclk_hsmmc1", "div_hsmmc1", SCLKCON, 12, 0, 0),
> +	GATE(SCLK_FIMD, "sclk_fimd", "div_fimd", SCLKCON, 10, 0, 0),
> +	GATE(SCLK_I2S0, "sclk_i2s0", "mux_i2s0", SCLKCON, 9, 0, 0),
> +	GATE(SCLK_UART, "sclk_uart", "div_uart", SCLKCON, 8, 0, 0),
> +	GATE(SCLK_USBH, "sclk_usbhost", "div_usbhost", SCLKCON, 1, 0, 0),
> +	GATE(HCLK_DRAM, "dram", "hclk", HCLKCON, 19, CLK_IGNORE_UNUSED, 0),
> +	GATE(HCLK_SSMC, "ssmc", "hclk", HCLKCON, 18, CLK_IGNORE_UNUSED, 0),
> +	GATE(HCLK_HSMMC1, "hsmmc1", "hclk", HCLKCON, 16, 0, 0),
> +	GATE(HCLK_USBD, "usb-device", "hclk", HCLKCON, 12, 0, 0),
> +	GATE(HCLK_USBH, "usb-host", "hclk", HCLKCON, 11, 0, 0),
> +	GATE(HCLK_LCD, "lcd", "hclk", HCLKCON, 9, 0, 0),
> +	GATE(HCLK_DMA5, "dma5", "hclk", HCLKCON, 5, CLK_IGNORE_UNUSED, 0),
> +	GATE(HCLK_DMA4, "dma4", "hclk", HCLKCON, 4, CLK_IGNORE_UNUSED, 0),
> +	GATE(HCLK_DMA3, "dma3", "hclk", HCLKCON, 3, CLK_IGNORE_UNUSED, 0),
> +	GATE(HCLK_DMA2, "dma2", "hclk", HCLKCON, 2, CLK_IGNORE_UNUSED, 0),
> +	GATE(HCLK_DMA1, "dma1", "hclk", HCLKCON, 1, CLK_IGNORE_UNUSED, 0),
> +	GATE(HCLK_DMA0, "dma0", "hclk", HCLKCON, 0, CLK_IGNORE_UNUSED, 0),

Hmm, I guess these CLK_IGNORE_UNUSED flags are needed because the old DMA 
driver doesn't handle clocks, right?

By the way, do you have some plans to continue working on the DMA engine 
driver for s3c24xx you posted some time ago? This would be really helpful, as 
we could then switch all the drivers using currently the Samsung-specific DMA 
API to the generic DMA engine API. (Actually I already have some patches for 
Samsung sound drivers. Will probably post an RFC soon.)

> +	GATE(PCLK_GPIO, "gpio", "pclk", PCLKCON, 13, CLK_IGNORE_UNUSED, 0),
> +	GATE(PCLK_RTC, "rtc", "pclk", PCLKCON, 12, 0, 0),
> +	GATE(PCLK_WDT, "wdt", "pclk", PCLKCON, 11, 0, 0),
> +	GATE(PCLK_PWM, "pwm", "pclk", PCLKCON, 10, 0, 0),
> +	GATE(PCLK_I2S0, "i2s0", "pclk", PCLKCON, 9, 0, 0),
> +	GATE(PCLK_AC97, "ac97", "pclk", PCLKCON, 8, 0, 0),
> +	GATE(PCLK_ADC, "adc", "pclk", PCLKCON, 7, 0, 0),
> +	GATE(PCLK_SPI0, "spi0", "pclk", PCLKCON, 6, 0, 0),
> +	GATE(PCLK_I2C0, "i2c0", "pclk", PCLKCON, 4, 0, 0),
> +	GATE(PCLK_UART3, "uart3", "pclk", PCLKCON, 3, 0, 0),
> +	GATE(PCLK_UART2, "uart2", "pclk", PCLKCON, 2, 0, 0),
> +	GATE(PCLK_UART1, "uart1", "pclk", PCLKCON, 1, 0, 0),
> +	GATE(PCLK_UART0, "uart0", "pclk", PCLKCON, 0, 0, 0),
> +};
> +
> +struct samsung_clock_alias s3c2443_common_aliases[] __initdata = {
> +	ALIAS(HCLK, NULL, "hclk"),
> +	ALIAS(HCLK_SSMC, NULL, "nand"),
> +	ALIAS(PCLK_UART0, "s3c2440-uart.0", "uart"),
> +	ALIAS(PCLK_UART1, "s3c2440-uart.1", "uart"),
> +	ALIAS(PCLK_UART2, "s3c2440-uart.2", "uart"),
> +	ALIAS(PCLK_UART3, "s3c2440-uart.3", "uart"),
> +	ALIAS(EXT_UART, NULL, "clk_uart_baud1"),
> +	ALIAS(PCLK_UART0, "s3c2440-uart.0", "clk_uart_baud2"),
> +	ALIAS(PCLK_UART1, "s3c2440-uart.1", "clk_uart_baud2"),
> +	ALIAS(PCLK_UART2, "s3c2440-uart.2", "clk_uart_baud2"),
> +	ALIAS(PCLK_UART3, "s3c2440-uart.3", "clk_uart_baud2"),
> +	ALIAS(SCLK_UART, NULL, "clk_uart_baud3"),
> +	ALIAS(PCLK_PWM, NULL, "timers"),
> +	ALIAS(PCLK_RTC, NULL, "rtc"),
> +	ALIAS(PCLK_WDT, NULL, "watchdog"),
> +	ALIAS(PCLK_ADC, NULL, "adc"),
> +	ALIAS(PCLK_I2C0, "s3c2410-i2c.0", "i2c"),
> +	ALIAS(HCLK_USBD, NULL, "usb-device"),
> +	ALIAS(HCLK_USBH, NULL, "usb-host"),
> +	ALIAS(SCLK_USBH, NULL, "usb-bus-host"),
> +	ALIAS(PCLK_SPI0, "s3c2443-spi.0", "spi"),
> +	ALIAS(PCLK_SPI0, "s3c2443-spi.0", "spi_busclk0"),
> +	ALIAS(HCLK_HSMMC1, "s3c-sdhci.1", "hsmmc"),
> +	ALIAS(HCLK_HSMMC1, "s3c-sdhci.1", "mmc_busclk.0"),
> +	ALIAS(PCLK_I2S0, "samsung-i2s.0", "iis"),
> +	ALIAS(SCLK_I2S0, NULL, "i2s-if"),
> +	ALIAS(HCLK_LCD, NULL, "lcd"),
> +	ALIAS(SCLK_FIMD, NULL, "sclk_fimd"),
> +};
> +
> +/* S3C2416 specific clocks */
> +
> +PNAME(s3c2416_hsmmc0_p) = { "sclk_hsmmc0", "sclk_hsmmcext" };
> +PNAME(s3c2416_hsmmc1_p) = { "sclk_hsmmc1", "sclk_hsmmcext" };
> +PNAME(s3c2416_hsspi0_p) = { "hsspi0_epll", "hsspi0_mpll" };
> +
> +static struct clk_div_table armdiv_s3c2416_d[] = {
> +	{ .val = 0, .div = 1 },
> +	{ .val = 1, .div = 2 },
> +	{ .val = 2, .div = 3 },
> +	{ .val = 3, .div = 4 },
> +	{ .val = 5, .div = 6 },
> +	{ .val = 7, .div = 8 },
> +	{ .div = 0 },
> +};
> +
> +struct samsung_div_clock s3c2416_dividers[] __initdata = {
> +	DIV_T(ARMDIV, "armdiv", "msysclk", CLKDIV0, 9, 3, armdiv_s3c2416_d),
> +	DIV(0, "div_hsspi0_mpll", "msysclk", CLKDIV2, 0, 4),
> +	DIV(0, "div_hsmmc0", "esysclk", CLKDIV2, 6, 2),
> +};
> +
> +struct samsung_mux_clock s3c2416_muxes[] __initdata = {
> +	MUX(MUX_HSMMC0, "mux_hsmmc0", s3c2416_hsmmc0_p, CLKSRC, 16, 1),
> +	MUX(MUX_HSMMC1, "mux_hsmmc1", s3c2416_hsmmc1_p, CLKSRC, 17, 1),
> +	MUX(MUX_HSSPI0, "mux_hsspi0", s3c2416_hsspi0_p, CLKSRC, 18, 1),
> +};
> +
> +struct samsung_gate_clock s3c2416_gates[] __initdata = {
> +	GATE(0, "hsspi0_mpll", "div_hsspi0_mpll", SCLKCON, 19, 0, 0),
> +	GATE(0, "hsspi0_epll", "div_hsspi0_epll", SCLKCON, 14, 0, 0),
> +	GATE(0, "sclk_hsmmc0", "div_hsmmc0", SCLKCON, 6, 0, 0),
> +	GATE(HCLK_2D, "2d", "hclk", HCLKCON, 20, 0, 0),
> +	GATE(HCLK_HSMMC0, "hsmmc0", "hclk", HCLKCON, 15, 0, 0),
> +	GATE(HCLK_IROM, "irom", "hclk", HCLKCON, 13, CLK_IGNORE_UNUSED, 0),
> +	GATE(PCLK_PCM, "pcm", "pclk", PCLKCON, 19, 0, 0),
> +};
> +
> +struct samsung_clock_alias s3c2416_aliases[] __initdata = {
> +	ALIAS(HCLK_HSMMC0, "s3c-sdhci.0", "hsmmc"),
> +	ALIAS(HCLK_HSMMC0, "s3c-sdhci.0", "mmc_busclk.0"),
> +	ALIAS(MUX_HSMMC0, "s3c-sdhci.0", "mmc_busclk.2"),
> +	ALIAS(MUX_HSMMC1, "s3c-sdhci.1", "mmc_busclk.2"),
> +	ALIAS(MUX_HSSPI0, "s3c2443-spi.0", "spi_busclk2"),
> +	ALIAS(ARMDIV, NULL, "armdiv"),
> +};
> +
> +/* S3C2443 specific clocks */
> +
> +static struct clk_div_table armdiv_s3c2443_d[] = {
> +	{ .val = 0, .div = 1 },
> +	{ .val = 8, .div = 2 },
> +	{ .val = 2, .div = 3 },
> +	{ .val = 9, .div = 4 },
> +	{ .val = 10, .div = 6 },
> +	{ .val = 11, .div = 8 },
> +	{ .val = 13, .div = 12 },
> +	{ .val = 15, .div = 16 },
> +	{ .div = 0 },
> +};
> +
> +struct samsung_div_clock s3c2443_dividers[] __initdata = {
> +	DIV_T(ARMDIV, "armdiv", "msysclk", CLKDIV0, 9, 4, armdiv_s3c2443_d),
> +	DIV(0, "div_cam", "esysclk", CLKDIV1, 26, 4),
> +};
> +
> +struct samsung_gate_clock s3c2443_gates[] __initdata = {
> +	GATE(SCLK_HSSPI0, "sclk_hsspi0", "div_hsspi0_epll", SCLKCON, 14, 0, 0),
> +	GATE(SCLK_CAM, "sclk_cam", "div_cam", SCLKCON, 11, 0, 0),
> +	GATE(HCLK_CFC, "cfc", "hclk", HCLKCON, 17, CLK_IGNORE_UNUSED, 0),
> +	GATE(HCLK_CAM, "cam", "hclk", HCLKCON, 8, 0, 0),
> +	GATE(PCLK_SPI1, "spi1", "pclk", PCLKCON, 15, 0, 0),
> +	GATE(PCLK_SDI, "sdi", "pclk", PCLKCON, 5, 0, 0),
> +};
> +
> +struct samsung_clock_alias s3c2443_aliases[] __initdata = {
> +	ALIAS(SCLK_HSSPI0, "s3c2443-spi.0", "spi_busclk2"),
> +	ALIAS(SCLK_HSMMC1, "s3c-sdhci.1", "mmc_busclk.2"),
> +	ALIAS(SCLK_CAM, NULL, "camif-upll"),
> +	ALIAS(PCLK_SPI1, "s3c2410-spi.0", "spi"),
> +	ALIAS(PCLK_SDI, NULL, "sdi"),
> +	ALIAS(HCLK_CFC, NULL, "cfc"),
> +	ALIAS(ARMDIV, NULL, "armdiv"),
> +};
> +
> +/* S3C2450 specific clocks */
> +
> +PNAME(s3c2450_cam_p) = { "div_cam", "hclk" };
> +PNAME(s3c2450_hsspi1_p) = { "hsspi1_epll", "hsspi1_mpll" };
> +PNAME(i2s1_p) = { "div_i2s1", "ext_i2s", "epllref", "epllref" };
> +
> +struct samsung_div_clock s3c2450_dividers[] __initdata = {
> +	DIV(0, "div_cam", "esysclk", CLKDIV1, 26, 4),
> +	DIV(0, "div_hsspi1_epll", "esysclk", CLKDIV2, 24, 2),
> +	DIV(0, "div_hsspi1_mpll", "msysclk", CLKDIV2, 16, 4),
> +	DIV(0, "div_i2s1", "esysclk", CLKDIV2, 12, 4),
> +};
> +
> +struct samsung_mux_clock s3c2450_muxes[] __initdata = {
> +	MUX(0, "mux_cam", s3c2450_cam_p, CLKSRC, 20, 1),
> +	MUX(MUX_HSSPI1, "mux_hsspi1", s3c2450_hsspi1_p, CLKSRC, 19, 1),
> +	MUX(0, "mux_i2s1", i2s1_p, CLKSRC, 12, 2),
> +};
> +
> +struct samsung_gate_clock s3c2450_gates[] __initdata = {
> +	GATE(SCLK_I2S1, "sclk_i2s1", "div_i2s1", SCLKCON, 5, 0, 0),
> +	GATE(HCLK_CFC, "cfc", "hclk", HCLKCON, 17, 0, 0),
> +	GATE(HCLK_CAM, "cam", "hclk", HCLKCON, 8, 0, 0),
> +	GATE(HCLK_DMA7, "dma7", "hclk", HCLKCON, 7, CLK_IGNORE_UNUSED, 0),
> +	GATE(HCLK_DMA6, "dma6", "hclk", HCLKCON, 6, CLK_IGNORE_UNUSED, 0),
> +	GATE(PCLK_I2S1, "i2s1", "pclk", PCLKCON, 17, 0, 0),
> +	GATE(PCLK_I2C1, "i2c1", "pclk", PCLKCON, 16, 0, 0),
> +	GATE(PCLK_SPI1, "spi1", "pclk", PCLKCON, 14, 0, 0),
> +};
> +
> +struct samsung_clock_alias s3c2450_aliases[] __initdata = {
> +	ALIAS(PCLK_SPI1, "s3c2443-spi.1", "spi"),
> +	ALIAS(PCLK_SPI1, "s3c2443-spi.1", "spi_busclk0"),
> +	ALIAS(MUX_HSSPI1, "s3c2443-spi.1", "spi_busclk2"),
> +	ALIAS(PCLK_I2C1, "s3c2410-i2c.1", "i2c"),
> +};
> +
> +/*
> + * This function allows non-dt platforms to specify the clock speed of the
> + * xti and ext clocks.
> + */
> +void __init s3c2443_clk_register_fixed_ext(unsigned long xti_f,
> +				unsigned long ext_f, unsigned long i2s_f,
> +				unsigned long uart_f)
> +{
> +	s3c2443_common_frate_clks[0].fixed_rate = xti_f;
> +	s3c2443_common_frate_clks[1].fixed_rate = ext_f;
> +	s3c2443_common_frate_clks[2].fixed_rate = i2s_f;
> +	s3c2443_common_frate_clks[3].fixed_rate = uart_f;
> +
> +	samsung_clk_register_fixed_rate(s3c2443_common_frate_clks,
> +			ARRAY_SIZE(s3c2443_common_frate_clks));
> +}
> +
> +static __initdata struct of_device_id ext_clk_match[] = {
> +	{ .compatible = "samsung,clock-xti", .data = (void *)0, },
> +	{ .compatible = "samsung,clock-ext", .data = (void *)1, },
> +	{ .compatible = "samsung,clock-ext-i2s", .data = (void *)2, },
> +	{ .compatible = "samsung,clock-ext-uart", .data = (void *)3, },
> +	{},
> +};

I wonder if we shouldn't use the generic clock bindings to define these instead 
of introducing platform specific binding. I know this has been already done for 
Exynos (and even in my patches for S3C64xx - I need to fix this), but I 
think the preferred way now (or maybe even the only allowed) is to use the 
generic bindings.

> +
> +void __init s3c2443_common_clk_init(struct device_node *np, int
> current_soc, +			     void __iomem *reg_base)
> +{
> +	struct clk *mpll, *epll;
> +
> +	if (np) {
> +		reg_base = of_iomap(np, 0);
> +		if (!reg_base)
> +			panic("%s: failed to map registers\n", __func__);
> +	}
> +
> +	samsung_clk_init(np, reg_base, NR_CLKS,
> +		s3c2443_clk_regs, ARRAY_SIZE(s3c2443_clk_regs), NULL, 0);
> +
> +	if (np)
> +		samsung_clk_of_register_fixed_ext(s3c2443_common_frate_clks,
> +			ARRAY_SIZE(s3c2443_common_frate_clks),
> +			ext_clk_match);
> +
> +	samsung_clk_register_fixed_factor(s3c2443_common_ffactor,
> +			ARRAY_SIZE(s3c2443_common_ffactor));
> +
> +	if (current_soc == S3C2416 || current_soc == S3C2450) {
> +		mpll = samsung_clk_register_pll6552x("mpll", "mpllref",
> +					reg_base + MPLLCON);
> +		epll = samsung_clk_register_pll6553x("epll", "epllref",
> +					reg_base + EPLLCON);
> +	} else {
> +		mpll = samsung_clk_register_pll3000x("mpll", "mpllref",
> +					reg_base + MPLLCON);
> +		epll = samsung_clk_register_pll2126x("epll", "epllref",
> +					reg_base + EPLLCON);
> +	}
> +
> +	samsung_clk_register_mux(s3c2443_common_muxes,
> +			ARRAY_SIZE(s3c2443_common_muxes));
> +	samsung_clk_register_div(s3c2443_common_dividers,
> +			ARRAY_SIZE(s3c2443_common_dividers));
> +	samsung_clk_register_gate(s3c2443_common_gates,
> +		ARRAY_SIZE(s3c2443_common_gates));
> +	samsung_clk_register_alias(s3c2443_common_aliases,
> +		ARRAY_SIZE(s3c2443_common_aliases));
> +
> +	if (current_soc == S3C2416 || current_soc == S3C2450) {
> +		samsung_clk_register_div(s3c2416_dividers,
> +				ARRAY_SIZE(s3c2416_dividers));
> +		samsung_clk_register_mux(s3c2416_muxes,
> +				ARRAY_SIZE(s3c2416_muxes));
> +		samsung_clk_register_gate(s3c2416_gates,
> +				ARRAY_SIZE(s3c2416_gates));
> +		samsung_clk_register_alias(s3c2416_aliases,
> +				ARRAY_SIZE(s3c2416_aliases));
> +	} else {
> +		samsung_clk_register_div(s3c2443_dividers,
> +				ARRAY_SIZE(s3c2443_dividers));
> +		samsung_clk_register_gate(s3c2443_gates,
> +				ARRAY_SIZE(s3c2443_gates));
> +		samsung_clk_register_alias(s3c2443_aliases,
> +				ARRAY_SIZE(s3c2443_aliases));
> +	}
> +
> +	/* s3c2450 extends the s3c2416 clocks */
> +	if (current_soc == S3C2450) {
> +		samsung_clk_register_div(s3c2450_dividers,
> +				ARRAY_SIZE(s3c2450_dividers));
> +		samsung_clk_register_mux(s3c2450_muxes,
> +				ARRAY_SIZE(s3c2450_muxes));
> +		samsung_clk_register_gate(s3c2450_gates,
> +				ARRAY_SIZE(s3c2450_gates));
> +		samsung_clk_register_alias(s3c2450_aliases,
> +				ARRAY_SIZE(s3c2450_aliases));
> +	}

nit: What about using a switch statement, like:

	switch (current_soc) {
	case S3C2450:
		/* register 2450 specific clocks */
		/* fall through */
	case S3C2416:
		/* register clocks common to 2416 and 2450 */
		break;
	case S3C2443:
		/* register 2443 specific clocks */
		break;
	}

Still, I guess it's just a matter of preference, so just feel free to leave it 
as it is.

Best regards,
Tomasz




More information about the linux-arm-kernel mailing list