[PATCH v3 5/5] clk/exynos5260: add clock file for exynos5260

[Tomasz Figa]

Hi Rahul,

On 18.02.2014 12:56, Rahul Sharma wrote:

[snip]

> diff --git a/drivers/clk/samsung/clk-exynos5260.c b/drivers/clk/samsung/clk-exynos5260.c
> new file mode 100644
> index 0000000..bcb633e
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-exynos5260.c
> @@ -0,0 +1,2235 @@
> +/*
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.

Most likely it should be 2013-2014 now.

> + *
> + * 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 Exynos5260 SoC.
> +*/

[snip]

> +#ifdef CONFIG_PM_SLEEP
> +
> +/*
> + * list of controller registers to be saved and restored during a
> + * suspend/resume cycle.
> +*/

nit: Unaligned star. + The same for a lot of comments in this file.

> +
> +static unsigned long exynos5260_aud_clk_regs[] __initdata = {

nit: There is no need to prefix every static variable in this file with 
exynos5260_. Shorter (but still completely readable) names will let the 
code using them have shorter lines.

> + *Registers for CMU_AUD
> +*/
> +	MUX_SEL_AUD,
> +	DIV_AUD0,
> +	DIV_AUD1,

[snip]

> +static void exynos5260_clk_sleep_init(void __iomem *reg_base,
> +			unsigned long *rdump,
> +			unsigned long nr_rdump)
> +{
> +	struct exynos5260_clock_reg_cache *reg_cache;
> +
> +	reg_cache = kzalloc(sizeof(struct exynos5260_clock_reg_cache),
> +			GFP_KERNEL);
> +	if (!reg_cache)
> +		panic("could not allocate register cache.\n");
> +
> +	reg_cache->rdump = samsung_clk_alloc_reg_dump(rdump, nr_rdump);
> +
> +	if (!reg_cache->rdump)
> +		panic("could not allocate register dump storage.\n");
> +
> +	reg_cache->rd_num = nr_rdump;
> +	reg_cache->reg_base = reg_base;
> +	list_add_tail(&reg_cache->node, &clock_reg_cache_list);
> +
> +	if (!syscore_ops_registered) {

To eliminate the need for having a separate variable, you can simply 
move this if clause before list_add_tail() and check for list_empty().

> +		register_syscore_ops(&exynos5260_clk_syscore_ops);
> +		syscore_ops_registered = true;
> +	}
> +
> +	exynos5260_clk_suspend();

What is the reason to call this here and save register values of all 
registered CMus every time a new CMU is instantiated?

> +}
> +
> +#else
> +static void exynos5260_clk_sleep_init(void) {}
> +#endif
> +
> +/*
> + * List of parent clocks for muses in CMU_AUD
> +*/
> +PNAME(mout_aud_pll_user_p) = {"fin_pll", "fout_aud_pll"};
> +PNAME(mout_sclk_aud_i2s_p) = {"mout_aud_pll_user", "ioclk_audcdclk0_user"};
> +PNAME(mout_sclk_aud_pcm_p) = {"mout_aud_pll_user", "ioclk_audcdclk0_user"};
> +
> +/*
> + * List of parent clocks for muses in CMU_DISP
> +*/
> +PNAME(mout_phyclk_dptx_phy_ch3_txd_clk_user_p) = {"fin_pll",
> +			"phyclk_dptx_phy_ch3_txd_clk"};
> +PNAME(mout_phyclk_dptx_phy_ch2_txd_clk_user_p) = {"fin_pll",
> +			"phyclk_dptx_phy_ch2_txd_clk"};
> +PNAME(mout_phyclk_dptx_phy_ch1_txd_clk_user_p) = {"fin_pll",
> +			"phyclk_dptx_phy_ch1_txd_clk"};
> +PNAME(mout_phyclk_dptx_phy_ch0_txd_clk_user_p) = {"fin_pll",
> +			"phyclk_dptx_phy_ch0_txd_clk"};

Whoa, these clock names are incredibly long. Are they real names from 
SoC User's Manual?

> +
> +PNAME(mout_aclk_disp_222_user_p) = {"fin_pll", "dout_aclk_disp_222"};
> +PNAME(mout_sclk_disp_pixel_user_p) = {"fin_pll", "dout_sclk_disp_pixel"};

[snip]

> +/* fixed rate clocks generated outside the soc */

Huh? If they are generated outside the SoC, they shouldn't be registered 
by this driver, but rather by respective fixed rate clock nodes in DT.

> +struct samsung_fixed_rate_clock exynos5260_fixed_rate_ext_clks[] __initdata = {
> +	FRATE(FIN_PLL, "fin_pll", NULL, CLK_IS_ROOT, 24000000),
> +	FRATE(ID_NONE, "xrtcxti", NULL, CLK_IS_ROOT, 32768),
> +
> +	FRATE(ID_NONE, "ioclk_audcdclk0_user", NULL, CLK_IS_ROOT, 0),
> +
> +	FRATE(ID_NONE, "ioclk_pcm_extclk", NULL, CLK_IS_ROOT, 2048000),
> +	FRATE(ID_NONE, "ioclk_aud_i2s_bclk", NULL, CLK_IS_ROOT, 2048000),
> +	FRATE(ID_NONE, "ioclk_spdif_extclk", NULL, CLK_IS_ROOT, 49152000),
> +	FRATE(ID_NONE, "ioclk_i2s_cdclk", NULL, CLK_IS_ROOT, 0),
> +	FRATE(ID_NONE, "ioclk_spdif_extlk", NULL, CLK_IS_ROOT, 0),
> +
> +	FRATE(ID_NONE, "ioclk_i2s_sclk", NULL, CLK_IS_ROOT, 0),
> +	FRATE(ID_NONE, "ioclk_spi0_clkin", NULL, CLK_IS_ROOT, 0),
> +	FRATE(ID_NONE, "ioclk_spi1_clkin", NULL, CLK_IS_ROOT, 0),
> +	FRATE(ID_NONE, "ioclk_spi2_clkin", NULL, CLK_IS_ROOT, 0),
> +
> +	FRATE(ID_NONE, "ioclk_mmc0_sdrdqs_in", NULL, CLK_IS_ROOT, 200000000),
> +
> +	FRATE(ID_NONE, "ioclk_spi0_isp_spi_clk_in", NULL,
> +			CLK_IS_ROOT, 50000000),
> +	FRATE(ID_NONE, "ioclk_spi1_isp_spi_clk_in", NULL,
> +			CLK_IS_ROOT, 50000000),
> +	FRATE(ID_NONE, "ioclk_spi0_isp_spi_clk_out", NULL,
> +			CLK_IS_ROOT, 50000000),
> +	FRATE(ID_NONE, "ioclk_spi1_isp_spi_clk_out", NULL,
> +			CLK_IS_ROOT, 50000000),
> +};
> +
> +/* fixed rate clocks generated inside the soc */
> +struct samsung_fixed_rate_clock exynos5260_fixed_rate_clks[] __initdata = {
> +	FRATE(ID_NONE, "phyclk_dptx_phy_ch3_txd_clk", NULL,
> +			CLK_IS_ROOT, 270000000),

Please define IDs for all clocks defined in this driver. This is 
necessary to allow rate and parent configuration using DT in future.

However, if there is a reason to not export some clock, please use 0 
directly, without defining a special macro like ID_NONE.

> +	FRATE(ID_NONE, "phyclk_dptx_phy_ch2_txd_clk", NULL,
> +			CLK_IS_ROOT, 270000000),
> +	FRATE(ID_NONE, "phyclk_dptx_phy_ch1_txd_clk", NULL,
> +			CLK_IS_ROOT, 270000000),
> +	FRATE(ID_NONE, "phyclk_dptx_phy_ch0_txd_clk", NULL,
> +			CLK_IS_ROOT, 270000000),
> +	FRATE(ID_NONE, "phyclk_hdmi_phy_tmds_clko", NULL,
> +			CLK_IS_ROOT, 250000000),
> +	FRATE(TOP_SCLK_HDMIPHY, "phyclk_hdmi_phy_pixel_clko", NULL,
> +			CLK_IS_ROOT, 1660000000),

Hmm, so is this clock top_sclk_hdmiphy or phyclk_hdmi_phy_pixel_clko? 
Shouldn't the macro and the name be using the same pattern?

> +	FRATE(ID_NONE, "phyclk_hdmi_link_o_tmds_clkhi", NULL,
> +			CLK_IS_ROOT, 125000000),
> +	FRATE(ID_NONE, "phyclk_mipi_dphy_4l_m_txbyteclkhs", NULL,
> +			CLK_IS_ROOT, 187500000),
> +	FRATE(ID_NONE, "phyclk_dptx_phy_o_ref_clk_24m", NULL,
> +			CLK_IS_ROOT, 24000000),
> +	FRATE(ID_NONE, "phyclk_dptx_phy_clk_div2", NULL,
> +			CLK_IS_ROOT, 135000000),
> +	FRATE(ID_NONE, "phyclk_mipi_dphy_4l_m_rxclkesc0", NULL,
> +			CLK_IS_ROOT, 20000000),
> +	FRATE(ID_NONE, "phyclk_usbhost20_phy_phyclock", NULL,
> +			CLK_IS_ROOT, 60000000),
> +	FRATE(ID_NONE, "phyclk_usbhost20_phy_freeclk", NULL,
> +			CLK_IS_ROOT, 60000000),
> +	FRATE(ID_NONE, "phyclk_usbhost20_phy_clk48mohci", NULL,
> +			CLK_IS_ROOT, 48000000),
> +	FRATE(ID_NONE, "phyclk_usbdrd30_udrd30_pipe_pclk", NULL,
> +			CLK_IS_ROOT, 125000000),
> +	FRATE(ID_NONE, "phyclk_usbdrd30_udrd30_phyclock", NULL,
> +			CLK_IS_ROOT, 60000000),

Are these really fixed rate clocks? It looks strange, because it's a bit 
unlike previous Samsung SoCs, which used to have up 5 fixed rate clocks 
in average.

> +};
> +
> +struct samsung_fixed_factor_clock exynos5260_fixed_factor_clks[] __initdata = {
> +};

If empty, why define this array?

> +
> +/* MULITPLEXER CLOCKS */

[snip]

> +
> +/* GATE CLOCKS */
> +
> +/*
> + * List of Gate clocks for CMU_AUD
> +*/
> +struct samsung_gate_clock exynos5260_aud_gate_clks[] __initdata = {
> +	GATE(AUD_CLK_AUD_UART, "clk_aud_uart", "dout_aclk_aud_131",
> +			EN_IP_AUD, 4, 0, 0),
> +	GATE(AUD_CLK_PCM, "clk_pcm", "dout_aclk_aud_131", EN_IP_AUD, 3, 0, 0),
> +	GATE(AUD_CLK_I2S, "clk_i2s", "dout_aclk_aud_131", EN_IP_AUD, 2, 0, 0),
> +	GATE(AUD_CLK_DMAC, "clk_dmac", "dout_aclk_aud_131",
> +			EN_IP_AUD, 1, 0, 0),
> +	GATE(ID_NONE, "clk_sramc", "dout_aclk_aud_131", EN_IP_AUD, 0, 0, 0),
> +
> +	GATE(AUD_SCLK_AUD_UART, "sclk_aud_uart", "dout_sclk_aud_uart",
> +			EN_SCLK_AUD, 2, 0, 0),
> +	GATE(AUD_SCLK_PCM, "sclk_aud_pcm", "dout_sclk_aud_pcm",
> +			EN_SCLK_AUD, 1, 0, 0),
> +	GATE(AUD_SCLK_I2S, "sclk_aud_i2s", "dout_sclk_aud_i2s",
> +			EN_SCLK_AUD, 0, 0, 0),

Shouldn't all the sclk's above have CLK_SET_RATE_PARENT flags to let 
consumer drivers change divisors of parent dividers? The same comment 
for remaining SCLK gates further in the file.

> +};
> +
> +/*
> + * List of Gate clocks for CMU_DISP
> +*/
> +struct samsung_gate_clock exynos5260_disp_gate_clks[] __initdata = {
> +	GATE(DISP_CLK_SMMU_TV, "clk_smmu3_tv", "mout_aclk_disp_222_user",
> +			EN_IP_DISP, 25, 0, 0),
> +	GATE(DISP_CLK_SMMU_FIMD1M1, "clk_smmu3_fimd1m1",
> +			"mout_aclk_disp_222_user",
> +			EN_IP_DISP, 23, 0, 0),
> +	GATE(DISP_CLK_SMMU_FIMD1M0, "clk_smmu3_fimd1m0",
> +			"mout_aclk_disp_222_user",
> +			EN_IP_DISP, 22, 0, 0),
> +	GATE(ID_NONE, "clk_pixel_mixer", "mout_aclk_disp_222_user",
> +			EN_IP_DISP, 13, CLK_IGNORE_UNUSED, 0),
> +	GATE(ID_NONE, "clk_pixel_disp", "mout_aclk_disp_222_user",
> +			EN_IP_DISP, 12, CLK_IGNORE_UNUSED, 0),

Why CLK_IGNORE_UNUSED?

> +	GATE(DISP_CLK_MIXER, "clk_mixer", "mout_aclk_disp_222_user",
> +			EN_IP_DISP, 11, 0, 0),
> +	GATE(DISP_CLK_MIPIPHY, "clk_mipi_dphy", "mout_aclk_disp_222_user",
> +			EN_IP_DISP, 10, 0, 0),
> +	GATE(DISP_CLK_HDMIPHY, "clk_hdmiphy", "mout_aclk_disp_222_user",
> +			EN_IP_DISP, 9, 0, 0),
> +	GATE(DISP_CLK_HDMI, "clk_hdmi", "mout_aclk_disp_222_user",
> +			EN_IP_DISP, 8, 0, 0),
> +	GATE(DISP_CLK_FIMD1, "clk_fimd1", "mout_aclk_disp_222_user",
> +			EN_IP_DISP, 7, 0, 0),
> +	GATE(DISP_CLK_DSIM1, "clk_dsim1", "mout_aclk_disp_222_user",
> +			EN_IP_DISP, 6, 0, 0),
> +	GATE(DISP_CLK_DPPHY, "clk_dptx_phy", "mout_aclk_disp_222_user",
> +			EN_IP_DISP, 5, 0, 0),
> +	GATE(DISP_CLK_DP, "clk_dptx_link", "mout_aclk_disp_222_user",
> +			EN_IP_DISP, 4, 0, 0),
> +
> +	GATE(DISP_SCLK_PIXEL, "sclk_hdmi_phy_pixel_clki",
> +			"dout_sclk_hdmi_phy_pixel_clki",
> +			EN_SCLK_DISP0, 29, 0, 0),
> +	GATE(DISP_SCLK_HDMI, "sclk_hdmi_link_i_pixel",
> +			"mout_phyclk_hdmi_phy_pixel_clko_user",
> +			EN_SCLK_DISP0, 26, 0, 0),
> +};
> +
> +/*
> + * List of Gate clocks for CMU_EGL
> +*/
> +struct samsung_gate_clock exynos5260_egl_gate_clks[] __initdata = {
> +};

Empty array.

> +
> +/*
> + * List of Gate clocks for CMU_FSYS

[snip]

> +
> +/*
> +* Applicable for all 2550 Type PLLS for Exynos5260, listed below
> +* DISP_PLL, EGL_PLL, KFC_PLL, MEM_PLL,
> +* BUS_PLL, MEDIA_PLL, G3D_PLL.
> +*/
> +static const struct samsung_pll_rate_table exynos5260_pll2550_24mhz_tbl[] = {
> +	PLL_35XX_RATE(1700000000, 425, 6, 0),
> +	PLL_35XX_RATE(1600000000, 200, 3, 0),
> +	PLL_35XX_RATE(1500000000, 250, 4, 0),
> +	PLL_35XX_RATE(1400000000, 175, 3, 0),
> +	PLL_35XX_RATE(1300000000, 325, 6, 0),
> +	PLL_35XX_RATE(1200000000, 400, 4, 1),
> +	PLL_35XX_RATE(1100000000, 275, 3, 1),
> +	PLL_35XX_RATE(1000000000, 250, 3, 1),
> +	PLL_35XX_RATE(933000000, 311, 4, 1),
> +	PLL_35XX_RATE(900000000, 300, 4, 1),
> +	PLL_35XX_RATE(800000000, 200, 3, 1),
> +	PLL_35XX_RATE(733000000, 733, 12, 1),
> +	PLL_35XX_RATE(700000000, 175, 3, 1),
> +	PLL_35XX_RATE(667000000, 667, 12, 1),
> +	PLL_35XX_RATE(633000000, 211, 4, 1),
> +	PLL_35XX_RATE(620000000, 310, 3, 2),
> +	PLL_35XX_RATE(600000000, 400, 4, 2),
> +	PLL_35XX_RATE(543000000, 362, 4, 2),
> +	PLL_35XX_RATE(533000000, 533, 6, 2),
> +	PLL_35XX_RATE(500000000, 250, 3, 2),
> +	PLL_35XX_RATE(450000000, 300, 4, 2),
> +	PLL_35XX_RATE(400000000, 200, 3, 2),
> +	PLL_35XX_RATE(350000000, 175, 3, 2),
> +	PLL_35XX_RATE(300000000, 400, 4, 3),
> +	PLL_35XX_RATE(266000000, 266, 3, 3),
> +	PLL_35XX_RATE(200000000, 200, 3, 3),
> +	PLL_35XX_RATE(160000000, 160, 3, 3),

Are all of those exact values, without any (incorrect) rounding? This is 
imporant for the rate setting code to work correctly.

> +};
> +
> +/*
> +* Applicable for 2650 Type PLL for AUD_PLL.
> +*/
> +static const struct samsung_pll_rate_table exynos5260_pll2650_24mhz_tbl[] = {
> +	PLL_36XX_RATE(1600000000, 200, 3, 0, 0),
> +	PLL_36XX_RATE(1200000000, 100, 2, 0, 0),
> +	PLL_36XX_RATE(1000000000, 250, 3, 1, 0),
> +	PLL_36XX_RATE(800000000, 200, 3, 1, 0),
> +	PLL_36XX_RATE(600000000, 100, 2, 1, 0),
> +	PLL_36XX_RATE(532000000, 266, 3, 2, 0),
> +	PLL_36XX_RATE(480000000, 160, 2, 2, 0),
> +	PLL_36XX_RATE(432000000, 144, 2, 2, 0),
> +	PLL_36XX_RATE(400000000, 200, 3, 2, 0),
> +	PLL_36XX_RATE(394216000, 459, 7, 2, 49282),
> +	PLL_36XX_RATE(333000000, 111, 2, 2, 0),
> +	PLL_36XX_RATE(300000000, 100, 2, 2, 0),
> +	PLL_36XX_RATE(266000000, 266, 3, 3, 0),
> +	PLL_36XX_RATE(200000000, 200, 3, 3, 0),
> +	PLL_36XX_RATE(166000000, 166, 3, 3, 0),
> +	PLL_36XX_RATE(133000000, 266, 3, 4, 0),
> +	PLL_36XX_RATE(100000000, 200, 3, 4, 0),
> +	PLL_36XX_RATE(66000000, 176, 2, 5, 0),

Ditto.

> +};
> +

[snip]

> +void __init exynos5260_clk_aud_init(struct device_node *np)

static void

> +{
> +	void __iomem *reg_base;
> +	static unsigned long *rdump;

Static?

> +	struct samsung_clk_provider *ctx;
> +	unsigned long nr_rdump;
> +
> +	if (!np)
> +		panic("%s: unable to determine cmu\n", __func__);

Can't happen. This function will be only called if a matching node is 
found in DT.

> +
> +	reg_base = of_iomap(np, 0);
> +	if (!reg_base)
> +		panic("%s: failed to map registers\n", __func__);
> +
> +	rdump = exynos5260_aud_clk_regs;
> +	nr_rdump = ARRAY_SIZE(exynos5260_aud_clk_regs);

You can use those two directly, without creating variables for them.

> +
> +	ctx = samsung_clk_init(np, reg_base, AUD_NR_CLK);
> +	if (!ctx)
> +		panic("%s: unable to alllocate ctx\n", __func__);
> +
> +	samsung_clk_register_mux(ctx, exynos5260_aud_mux_clks,
> +		ARRAY_SIZE(exynos5260_aud_mux_clks));
> +	samsung_clk_register_div(ctx, exynos5260_aud_div_clks,
> +		ARRAY_SIZE(exynos5260_aud_div_clks));
> +	samsung_clk_register_gate(ctx, exynos5260_aud_gate_clks,
> +		ARRAY_SIZE(exynos5260_aud_gate_clks));
> +
> +	exynos5260_clk_sleep_init(reg_base, rdump, nr_rdump);
> +}
> +
> +void __init exynos5260_clk_disp_init(struct device_node *np)
> +{
> +	void __iomem *reg_base;
> +	static unsigned long *rdump;
> +	struct samsung_clk_provider *ctx;
> +	unsigned long nr_rdump;
> +
> +	if (!np)
> +		panic("%s: unable to determine cmu\n", __func__);
> +
> +	reg_base = of_iomap(np, 0);
> +	if (!reg_base)
> +		panic("%s: failed to map registers\n", __func__);
> +
> +	rdump = exynos5260_disp_clk_regs;
> +	nr_rdump = ARRAY_SIZE(exynos5260_disp_clk_regs);
> +
> +	ctx = samsung_clk_init(np, reg_base, DISP_NR_CLK);
> +	if (!ctx)
> +		panic("%s: unable to alllocate ctx\n", __func__);
> +
> +	samsung_clk_register_mux(ctx, exynos5260_disp_mux_clks,
> +		ARRAY_SIZE(exynos5260_disp_mux_clks));
> +	samsung_clk_register_div(ctx, exynos5260_disp_div_clks,
> +		ARRAY_SIZE(exynos5260_disp_div_clks));
> +	samsung_clk_register_gate(ctx, exynos5260_disp_gate_clks,
> +		ARRAY_SIZE(exynos5260_disp_gate_clks));
> +
> +	exynos5260_clk_sleep_init(reg_base, rdump, nr_rdump);
> +}

Hmm, instead of repeating almost exactly the same function for each CMU, 
wouldn't it be better to simply write a single parametrized one?

What about creating a struct like this:

struct exynos5260_cmu_type {
	unsigned long *rdump;
	unsigned long nr_rdump;

	unsigned int nr_clks;

	struct samsung_mux_clock *mux_clks;
	unsigned int nr_mux_clks;
	/* Same for remaining clock types. */
};

and then defining instances for all the needed CMUs and passing a 
pointer to such struct to a generic registering function?

> +
> +void __init exynos5260_clk_egl_init(struct device_node *np)
> +{
> +	void __iomem *reg_base;

[snip]

> +struct of_device_id __clk_of_table_exynos5260[]
> +		__used __section(__clk_of_table) = {

I don't think this is a good idea. This construct should not be used 
directly. This is what CLK_OF_DECLARE() macro is for.

If you define a struct as mentioned above for every CMU, then you should 
end up with following code:

static const struct exynos5260_cmu_type cmu_xxx = {
	// ...
};

static void __init cmu_xxx_init(struct device_node *np)
{
         exynos5260_cmu_register_one(np, &cmu_xxx);
}
CLK_OF_DECLARE(cmu_xxx, "samsung,exynos5260-clock-xxx", cmu_xxx_init);

for every CMU type, which still needs some duplication, but should be 
much less than in current form.

Also it would be nice to group definitions for particular CMUs together, 
e.g.

/* CMU XXX */
// regs
// fixed rate
// muxes
// dividers
// gates
// plls
// struct exynos5260_cmu_type
// cmu_xxx_init()

/* CMU YYY */
...

This should improve readability of the driver, as definitions for 
different IP blocks would not be intermingled.

[snip]

> diff --git a/drivers/clk/samsung/clk-exynos5260.h b/drivers/clk/samsung/clk-exynos5260.h
> new file mode 100644
> index 0000000..392c152
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-exynos5260.h
> @@ -0,0 +1,480 @@
> +#ifndef __CLK_EXYNOS5260_H
> +#define __CLK_EXYNOS5260_H
> +
> +/*
> +*Base address for different CMUs
> +*TODO: All Bases should be removed at earliest.

True, very true. Please remove them in next version ;).

> +*/
> +#define CMU_AUD_BASE	0x128C0000
> +#define CMU_DISP_BASE	0x14550000
> +#define CMU_EGL_BASE	0x10600000
> +#define CMU_FSYS_BASE	0x122E0000
> +#define CMU_G2D_BASE	0x10A00000
> +#define CMU_G3D_BASE	0x11830000
> +#define CMU_GSCL_BASE	0x13F00000
> +#define CMU_ISP_BASE	0x133C0000
> +#define CMU_KFC_BASE	0x10700000
> +#define CMU_MFC_BASE	0x11090000
> +#define CMU_MIF_BASE	0x10CE0000
> +#define CMU_PERI_BASE	0x10200000
> +#define CMU_TOP_BASE	0x10010000
> +
> +#define AUD_REG(x)		(x)
> +#define DISP_REG(x)		(x)
> +#define EGL_REG(x)		(x)
> +#define FSYS_REG(x)		(x)
> +#define G2D_REG(x)		(x)
> +#define G3D_REG(x)		(x)
> +#define GSCL_REG(x)		(x)
> +#define ISP_REG(x)		(x)
> +#define KFC_REG(x)		(x)
> +#define MFC_REG(x)		(x)
> +#define MIF_REG(x)		(x)
> +#define PERI_REG(x)		(x)
> +#define TOP_REG(x)		(x)
> +
> +/*
> +*Registers for CMU_AUD
> +*/
> +#define MUX_SEL_AUD		AUD_REG(0x0200)
> +#define MUX_ENABLE_AUD		AUD_REG(0x0300)
> +#define MUX_STAT_AUD		AUD_REG(0x0400)
> +#define MUX_IGNORE_AUD		AUD_REG(0x0500)
> +#define DIV_AUD0		AUD_REG(0x0600)
> +#define DIV_AUD1		AUD_REG(0x0604)
> +#define DIV_STAT_AUD0		AUD_REG(0x0700)
> +#define DIV_STAT_AUD1		AUD_REG(0x0704)
> +#define EN_ACLK_AUD		AUD_REG(0x0800)
> +#define EN_PCLK_AUD		AUD_REG(0x0900)
> +#define EN_SCLK_AUD		AUD_REG(0x0a00)
> +#define EN_IP_AUD		AUD_REG(0x0b00)

No need to use XXX_REG() macros, simple offsets are enough. Also I don't 
think there is a need to have a separate header for these definitions 
used just by one source file. They can be safely moved there.

Best regards,
Tomasz