From: Stephen Boyd <sboyd@codeaurora.org>
To: Lars Persson <lars.persson@axis.com>
Cc: devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
mturquette@baylibre.com, robh+dt@kernel.org, pawel.moll@arm.com,
mark.rutland@arm.com, ijc+devicetree@hellion.org.uk,
galak@codeaurora.org, linux-kernel@vger.kernel.org,
Lars Persson <larper@axis.com>
Subject: Re: [PATCH v5 2/2] clk: add artpec-6 clock controller
Date: Fri, 1 Apr 2016 18:39:28 -0700 [thread overview]
Message-ID: <20160402013928.GF18567@codeaurora.org> (raw)
In-Reply-To: <24806729fd9726fe79d315a3c88ecf1c9ea237ef.1459447089.git.larper@axis.com>
On 03/31, Lars Persson wrote:
> diff --git a/drivers/clk/axis/Makefile b/drivers/clk/axis/Makefile
> new file mode 100644
> index 0000000..628c9d3
> --- /dev/null
> +++ b/drivers/clk/axis/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_MACH_ARTPEC6) += clk-artpec6.o
> diff --git a/drivers/clk/axis/clk-artpec6.c b/drivers/clk/axis/clk-artpec6.c
> new file mode 100644
> index 0000000..cf716f1
> --- /dev/null
> +++ b/drivers/clk/axis/clk-artpec6.c
> @@ -0,0 +1,230 @@
> +/*
> + * ARTPEC-6 clock initialization
> + *
> + * Copyright 2015-2016 Axis Comunications AB.
> + *
> + * 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.
> + */
> +
> +#include <linux/clk.h>
Is this include used?
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
Is this include used?
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/axis,artpec6-clkctrl.h>
> +
> +#define NUM_I2S_CLOCKS 2
> +
> +struct artpec6_clkctrl_drvdata {
> + struct clk *clk_table[ARTPEC6_CLK_NUMCLOCKS];
> + void __iomem *syscon_base;
> + struct clk_onecell_data clk_data;
> + spinlock_t i2scfg_lock;
> +};
> +
> +
> +static struct artpec6_clkctrl_drvdata *clkdata;
> +
> +static const char *i2s_clk_names[NUM_I2S_CLOCKS] = {
const char * const?
> + "i2s0",
> + "i2s1",
> +};
> +
> +static const int i2s_clk_indexes[NUM_I2S_CLOCKS] = {
> + ARTPEC6_CLK_I2S0_CLK,
> + ARTPEC6_CLK_I2S1_CLK,
> +};
> +
> +static void of_artpec6_clkctrl_setup(struct device_node *np)
> +{
> + int i;
> + const char *sys_refclk_name;
> + u32 pll_mode, pll_m, pll_n;
> +
> + pr_info("of_artpec6_clkctrl_setup\n");
Debug noise?
> +
> + /* Mandatory parent clock. */
> + i = of_property_match_string(np, "clock-names", "sys_refclk");
> + if (i < 0)
> + return;
> +
> + sys_refclk_name = of_clk_get_parent_name(np, i);
> +
> + clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL);
> + if (!clkdata)
> + return;
> +
> + for (i = 0; i < ARTPEC6_CLK_NUMCLOCKS; ++i)
> + clkdata->clk_table[i] = ERR_PTR(-EPROBE_DEFER);
> +
> + clkdata->syscon_base = of_iomap(np, 0);
What if that fails?
> +
> + /* Read PLL1 factors configured by boot strap pins. */
> + pll_mode = (readl(clkdata->syscon_base) >> 6) & 3;
> + switch (pll_mode) {
> + case 0: /* DDR3-2133 mode */
> + pll_m = 4;
> + pll_n = 85;
> + break;
> + case 1: /* DDR3-1866 mode */
> + pll_m = 6;
> + pll_n = 112;
> + break;
> + case 2: /* DDR3-1600 mode */
> + pll_m = 4;
> + pll_n = 64;
> + break;
> + case 3: /* DDR3-1333 mode */
> + pll_m = 8;
> + pll_n = 106;
> + break;
> + }
> +
> + clkdata->clk_table[ARTPEC6_CLK_CPU] =
> + clk_register_fixed_factor(NULL, "cpu", sys_refclk_name, 0, pll_n,
> + pll_m);
> + clkdata->clk_table[ARTPEC6_CLK_CPU_PERIPH] =
> + clk_register_fixed_factor(NULL, "cpu_periph", "cpu", 0, 1, 2);
> +
> + /* EPROBE_DEFER on the apb_clock is not handled in amba devices. */
Shouldn't we fix that then?
> + clkdata->clk_table[ARTPEC6_CLK_UART_PCLK] =
> + clk_register_fixed_factor(NULL, "uart_pclk", "cpu", 0, 1, 8);
> + clkdata->clk_table[ARTPEC6_CLK_UART_REFCLK] =
> + clk_register_fixed_rate(NULL, "uart_ref", sys_refclk_name, 0,
> + 50000000);
> +
> + clkdata->clk_table[ARTPEC6_CLK_SPI_PCLK] =
> + clk_register_fixed_factor(NULL, "spi_pclk", "cpu", 0, 1, 8);
> + clkdata->clk_table[ARTPEC6_CLK_SPI_SSPCLK] =
> + clk_register_fixed_rate(NULL, "spi_sspclk", sys_refclk_name, 0,
> + 50000000);
> +
> + clkdata->clk_table[ARTPEC6_CLK_DBG_PCLK] =
> + clk_register_fixed_factor(NULL, "dbg_pclk", "cpu", 0, 1, 8);
> +
> + clkdata->clk_data.clks = clkdata->clk_table;
> + clkdata->clk_data.clk_num = ARTPEC6_CLK_NUMCLOCKS;
> +
> + of_clk_add_provider(np, of_clk_src_onecell_get, &clkdata->clk_data);
> +}
> +
Nitpick: Drop newline here.
> +CLK_OF_DECLARE(artpec6_clkctrl, "axis,artpec6-clkctrl",
> + of_artpec6_clkctrl_setup);
> +
> +static int artpec6_clkctrl_probe(struct platform_device *pdev)
> +{
> + int propidx;
> + struct device_node *np = pdev->dev.of_node;
> + const char *sys_refclk_name;
> + const char *i2s_refclk_name = NULL;
> + const char *frac_clk_name[2] = { NULL, NULL };
> + const char *i2s_mux_parents[2] = { NULL, NULL };
Do we need to initialize this to NULL?
> + u32 muxreg;
> + u32 i;
int i please.
> +
> + /* Mandatory parent clock. */
> + propidx = of_property_match_string(np, "clock-names", "sys_refclk");
> + if (propidx < 0)
> + return -EINVAL;
> +
> + sys_refclk_name = of_clk_get_parent_name(np, propidx);
> +
> + /* Find clock names of optional parent clocks. */
> + propidx = of_property_match_string(np, "clock-names", "i2s_refclk");
> + if (propidx >= 0)
> + i2s_refclk_name = of_clk_get_parent_name(np, propidx);
> +
> + propidx = of_property_match_string(np, "clock-names", "frac_clk0");
> + if (propidx >= 0)
> + frac_clk_name[0] = of_clk_get_parent_name(np, propidx);
> + propidx = of_property_match_string(np, "clock-names", "frac_clk1");
> + if (propidx >= 0)
> + frac_clk_name[1] = of_clk_get_parent_name(np, propidx);
> +
> + spin_lock_init(&clkdata->i2scfg_lock);
> +
> + clkdata->clk_table[ARTPEC6_CLK_NAND_CLKA] =
> + clk_register_fixed_factor(NULL, "nand_clka", "cpu", 0, 1, 8);
> + clkdata->clk_table[ARTPEC6_CLK_NAND_CLKB] =
> + clk_register_fixed_rate(NULL, "nand_clkb", sys_refclk_name, 0,
> + 100000000);
> + clkdata->clk_table[ARTPEC6_CLK_ETH_ACLK] =
> + clk_register_fixed_factor(NULL, "eth_aclk", "cpu", 0, 1, 4);
> + clkdata->clk_table[ARTPEC6_CLK_DMA_ACLK] =
> + clk_register_fixed_factor(NULL, "dma_aclk", "cpu", 0, 1, 4);
> + clkdata->clk_table[ARTPEC6_CLK_PTP_REF] =
> + clk_register_fixed_rate(NULL, "ptp_ref", sys_refclk_name, 0,
> + 100000000);
> + clkdata->clk_table[ARTPEC6_CLK_SD_PCLK] =
> + clk_register_fixed_rate(NULL, "sd_pclk", sys_refclk_name, 0,
> + 100000000);
> + clkdata->clk_table[ARTPEC6_CLK_SD_IMCLK] =
> + clk_register_fixed_rate(NULL, "sd_imclk", sys_refclk_name, 0,
> + 100000000);
> + clkdata->clk_table[ARTPEC6_CLK_I2S_HST] =
> + clk_register_fixed_factor(NULL, "i2s_hst", "cpu", 0, 1, 8);
Pass device pointer for all these.
> +
> + for (i = 0; i < NUM_I2S_CLOCKS; ++i) {
> + if (i2s_refclk_name && frac_clk_name[i]) {
> + i2s_mux_parents[0] = frac_clk_name[i];
> + i2s_mux_parents[1] = i2s_refclk_name;
> +
> + clkdata->clk_table[i2s_clk_indexes[i]] =
> + clk_register_mux(NULL, i2s_clk_names[i],
Please pass the device pointer here.
> + i2s_mux_parents, 2,
> + CLK_SET_RATE_NO_REPARENT |
> + CLK_SET_RATE_PARENT,
> + clkdata->syscon_base + 0x14, i, 1,
> + 0, &clkdata->i2scfg_lock);
> + } else if (frac_clk_name[i]) {
> + /* Lock the mux for internal clock reference. */
> + muxreg = readl(clkdata->syscon_base + 0x14);
> + muxreg &= ~BIT(i);
> + writel(muxreg, clkdata->syscon_base + 0x14);
> + clkdata->clk_table[i2s_clk_indexes[i]] =
> + clk_register_fixed_factor(NULL, i2s_clk_names[i],
> + frac_clk_name[i], 0, 1,
> + 1);
> + } else if (i2s_refclk_name) {
> + /* Lock the mux for external clock reference. */
> + muxreg = readl(clkdata->syscon_base + 0x14);
> + muxreg |= BIT(i);
> + writel(muxreg, clkdata->syscon_base + 0x14);
> + clkdata->clk_table[i2s_clk_indexes[i]] =
> + clk_register_fixed_factor(NULL, i2s_clk_names[i],
> + i2s_refclk_name, 0, 1, 1);
> + }
Please assign the register result to some local variable and then
assign that local variable to clkdata->clk_table[...] once.
> + }
> +
> + clkdata->clk_table[ARTPEC6_CLK_I2C] =
> + clk_register_fixed_rate(NULL, "i2c", sys_refclk_name, 0, 100000000);
> +
> + clkdata->clk_table[ARTPEC6_CLK_SYS_TIMER] =
> + clk_register_fixed_rate(NULL, "timer", sys_refclk_name, 0,
> + 100000000);
> + clkdata->clk_table[ARTPEC6_CLK_FRACDIV_IN] =
> + clk_register_fixed_rate(NULL, "fracdiv_in", sys_refclk_name, 0,
> + 600000000);
What if any of these fail?
> +
> + return 0;
> +}
> +
> +static const struct of_device_id artpec_clkctrl_of_match[] = {
> + { .compatible = "axis,artpec6-clkctrl" },
> + {},
Nitpick: Drop ',' here so that people don't get any wrong ideas.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
next prev parent reply other threads:[~2016-04-02 1:39 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-31 18:02 [PATCH v5 0/2] clk: Add Artpec-6 SoC support Lars Persson
2016-03-31 18:02 ` [PATCH v5 1/2] clk: add device tree binding for Artpec-6 clock controller Lars Persson
[not found] ` <cover.1459447089.git.larper-VrBV9hrLPhE@public.gmane.org>
2016-03-31 18:02 ` [PATCH v5 2/2] clk: add artpec-6 " Lars Persson
2016-04-02 1:39 ` Stephen Boyd [this message]
2016-04-04 8:24 ` Lars Persson
2016-04-15 22:58 ` Stephen Boyd
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160402013928.GF18567@codeaurora.org \
--to=sboyd@codeaurora.org \
--cc=devicetree@vger.kernel.org \
--cc=galak@codeaurora.org \
--cc=ijc+devicetree@hellion.org.uk \
--cc=larper@axis.com \
--cc=lars.persson@axis.com \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mturquette@baylibre.com \
--cc=pawel.moll@arm.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).