Re: [PATCH v2 04/12] driver: clk: imx: Add clock driver for imx6sll

[Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 12/27, Bai Ping wrote:
> diff --git a/drivers/clk/imx/clk-imx6sll.c b/drivers/clk/imx/clk-imx6sll.c
> new file mode 100644
> index 0000000..73758fe1
> --- /dev/null
> +++ b/drivers/clk/imx/clk-imx6sll.c
> @@ -0,0 +1,369 @@
> +/*
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <dt-bindings/clock/imx6sll-clock.h>
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>

Is this used?

> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>

Is this used?

> +#include <linux/types.h>
> +
> +#include "clk.h"
> +
> +#define CCM_ANALOG_PLL_BYPASS		(0x1 << 16)
> +#define BM_CCM_CCDR_MMDC_CH0_MASK	(0x2 << 16)
> +#define CCDR	0x4
> +#define xPLL_CLR(offset)		 (offset + 0x8)
> +
> +static const char *pll_bypass_src_sels[] = { "osc", "dummy", };

All these should be const char * const unless something is wrong.

> +static const char *pll1_bypass_sels[] = { "pll1", "pll1_bypass_src", };
> +static const char *pll2_bypass_sels[] = { "pll2", "pll2_bypass_src", };
> +static const char *pll3_bypass_sels[] = { "pll3", "pll3_bypass_src", };
> +static const char *pll4_bypass_sels[] = { "pll4", "pll4_bypass_src", };
> +static const char *pll5_bypass_sels[] = { "pll5", "pll5_bypass_src", };
> +static const char *pll6_bypass_sels[] = { "pll6", "pll6_bypass_src", };
[...]
> +	clks[IMX6SLL_CLK_USDHC3]	= imx_clk_gate2("usdhc3",	"usdhc3_podf",	 base + 0x80,	6);
> +
> +	/* mask handshake of mmdc */
> +	writel_relaxed(BM_CCM_CCDR_MMDC_CH0_MASK, base + CCDR);
> +
> +	for (i = 0; i < ARRAY_SIZE(clks); i++)
> +		if (IS_ERR(clks[i]))
> +			pr_err("i.MX6SLL clk %d: register failed with %ld\n", i, PTR_ERR(clks[i]));
> +
> +	clk_data.clks = clks;
> +	clk_data.clk_num = ARRAY_SIZE(clks);
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> +
> +	/* set perclk to from OSC */
> +	clk_set_parent(clks[IMX6SLL_CLK_PERCLK_SEL], clks[IMX6SLL_CLK_OSC]);

Can this be done with assigned-clocks in DT?

> +
> +	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
> +		clk_prepare_enable(clks[clks_init_on[i]]);

Critical clocks?

> +
> +	if (IS_ENABLED(CONFIG_USB_MXS_PHY)) {
> +		clk_prepare_enable(clks[IMX6SLL_CLK_USBPHY1_GATE]);
> +		clk_prepare_enable(clks[IMX6SLL_CLK_USBPHY2_GATE]);

The phy driver can't enable these?

> +	}
> +
> +	/* Lower the AHB clock rate before changing the clock source. */
> +	clk_set_rate(clks[IMX6SLL_CLK_AHB], 99000000);
> +
> +	/* Change periph_pre clock to pll2_bus to adjust AXI rate to 264MHz */
> +	clk_set_parent(clks[IMX6SLL_CLK_PERIPH_CLK2_SEL], clks[IMX6SLL_CLK_PLL3_USB_OTG]);
> +	clk_set_parent(clks[IMX6SLL_CLK_PERIPH], clks[IMX6SLL_CLK_PERIPH_CLK2]);
> +	clk_set_parent(clks[IMX6SLL_CLK_PERIPH_PRE], clks[IMX6SLL_CLK_PLL2_BUS]);
> +	clk_set_parent(clks[IMX6SLL_CLK_PERIPH], clks[IMX6SLL_CLK_PERIPH_PRE]);
> +
> +	clk_set_rate(clks[IMX6SLL_CLK_AHB], 132000000);

assigned-clocks for rates now? Or perhaps we shouldn't be
exposing these as clks if they have some sort of complicated rate
sequence switch that we can't guarantee with the clk_ops we have
today.

> +}
> +
> +CLK_OF_DECLARE(imx6sll, "fsl,imx6sll-ccm", imx6sll_clocks_init);
> +

Please drop this extra newline.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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