Message-ID: <20171115170647.GR11955@codeaurora.org>
Date:   Wed, 15 Nov 2017 09:06:47 -0800
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Tirupathi Reddy <tirupath@...eaurora.org>
Cc:     mturquette@...libre.com, robh+dt@...nel.org, mark.rutland@....com,
        andy.gross@...aro.org, david.brown@...aro.org,
        linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-soc@...r.kernel.org
Subject: Re: [PATCH V5] clk: qcom: Add spmi_pmic clock divider support

On 09/19, Tirupathi Reddy wrote:
> diff --git a/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt b/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt
> new file mode 100644
> index 0000000..e37a136
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt
> @@ -0,0 +1,56 @@
> +Qualcomm Technologies, Inc. SPMI PMIC clock divider (clkdiv)
> +
> +clkdiv configures the clock frequency of a set of outputs on the PMIC.
> +These clocks are typically wired through alternate functions on
> +gpio pins.
> +
> +=======================
> +Properties
> +=======================
> +
> +- compatible
> +	Usage:      required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,spmi-clkdiv"
> +		    "qcom,pm8998-clkdiv"
> +
> +- reg
> +	Usage:      required
> +	Value type: <prop-encoded-array>
> +	Definition: Addresses and sizes for the memory of this CLKDIV

There's no size though.

> +		    peripheral.
> +
> +- clocks:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: reference to the xo clock.
> +
> +- clock-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "xo".
> +
> +- clock-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: shall contain 1.

Can we get a property to indicate the number of clks? Something
like "qcom,num-clkdivs" or something like that. That would make
things easier to handle in the driver because we could match the
generic compatible and look for the property and then populate
that many clks. Otherwise, we're going to need a "small" driver
update for each PMIC just because the number of clks changes.

Of course we're going to need to update the binding to add the
new PMIC compatibles as new things are created, but I'd like to
avoid driver updates for the compatible. If someone messes up the
number we can always fallback to matching the more specific
compatible and fix it up in the driver, but let's hope that
doesn't happen.

> +
> +=======
> +Example
> +=======
> +
> +pm8998_clk_divs: qcom,clock@...0 {

clock-controller@...0?

> +	compatible = "qcom,pm8998-clkdiv";
> +	reg = <0x5b00>;
> +	#clock-cells = <1>;
> +	clocks = <&xo_board>;
> +	clock-names = "xo";
> +
> +	assigned-clocks = <&pm8998_clk_divs 1>,
> +			  <&pm8998_clk_divs 2>,
> +			  <&pm8998_clk_divs 3>;
> +	assigned-clock-rates = <9600000>,
> +			       <9600000>,
> +			       <9600000>;
> +};
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 9f6c278..dd5b18e 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -196,3 +196,12 @@ config MSM_MMCC_8996
>  	  Support for the multimedia clock controller on msm8996 devices.
>  	  Say Y if you want to support multimedia devices such as display,
>  	  graphics, video encode/decode, camera, etc.
> +
> +config SPMI_PMIC_CLKDIV
> +	tristate "spmi pmic clkdiv driver"

Capitalize SPMI and PMIC and replace driver with something else.

> +	depends on (COMMON_CLK_QCOM && SPMI) || COMPILE_TEST
> +	help
> +	  This driver supports the clkdiv functionality on the Qualcomm
> +	  Technologies, Inc. SPMI PMIC. It configures the frequency of
> +	  clkdiv outputs on the PMIC. These clocks are typically wired
> +	  through alternate functions on gpio pins.

I rewrote a bunch of stuff in the driver. Let me know if anything
doesn't work.

----8<----
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index dd5b18ecd9d1..20b5d6fd501d 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -198,10 +198,10 @@ config MSM_MMCC_8996
 	  graphics, video encode/decode, camera, etc.
 
 config SPMI_PMIC_CLKDIV
-	tristate "spmi pmic clkdiv driver"
+	tristate "SPMI PMIC clkdiv Support"
 	depends on (COMMON_CLK_QCOM && SPMI) || COMPILE_TEST
 	help
 	  This driver supports the clkdiv functionality on the Qualcomm
 	  Technologies, Inc. SPMI PMIC. It configures the frequency of
-	  clkdiv outputs on the PMIC. These clocks are typically wired
-	  through alternate functions on gpio pins.
+	  clkdiv outputs of the PMIC. These clocks are typically wired
+	  through alternate functions on GPIO pins.
diff --git a/drivers/clk/qcom/clk-spmi-pmic-div.c b/drivers/clk/qcom/clk-spmi-pmic-div.c
index af343ad2ee0b..a7217ee9f741 100644
--- a/drivers/clk/qcom/clk-spmi-pmic-div.c
+++ b/drivers/clk/qcom/clk-spmi-pmic-div.c
@@ -18,6 +18,7 @@
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
@@ -29,29 +30,12 @@
 #define REG_EN_CTL			0x46
 #define REG_EN_MASK			BIT(7)
 
-#define ENABLE_DELAY_NS(cxo_ns, div)	((2 + 3 * div) * cxo_ns)
-#define DISABLE_DELAY_NS(cxo_ns, div)	((3 * div) * cxo_ns)
-
-#define CLK_SPMI_PMIC_DIV_OFFSET	1
-
-#define CLKDIV_XO_DIV_1_0		0
-#define CLKDIV_XO_DIV_1			1
-#define CLKDIV_XO_DIV_2			2
-#define CLKDIV_XO_DIV_4			3
-#define CLKDIV_XO_DIV_8			4
-#define CLKDIV_XO_DIV_16		5
-#define CLKDIV_XO_DIV_32		6
-#define CLKDIV_XO_DIV_64		7
-#define CLKDIV_MAX_ALLOWED		8
-
 struct clkdiv {
 	struct regmap		*regmap;
 	u16			base;
 	spinlock_t		lock;
 
-	/* clock properties */
 	struct clk_hw		hw;
-	unsigned int		div_factor;
 	unsigned int		cxo_period_ns;
 };
 
@@ -62,94 +46,68 @@ static inline struct clkdiv *to_clkdiv(struct clk_hw *hw)
 
 static inline unsigned int div_factor_to_div(unsigned int div_factor)
 {
-	if (div_factor == CLKDIV_XO_DIV_1_0)
-		return 1;
+	if (!div_factor)
+		div_factor = 1;
 
-	return 1 << (div_factor - CLK_SPMI_PMIC_DIV_OFFSET);
+	return 1 << (div_factor - 1);
 }
 
 static inline unsigned int div_to_div_factor(unsigned int div)
 {
-	return min(ilog2(div) + CLK_SPMI_PMIC_DIV_OFFSET,
-		   CLKDIV_MAX_ALLOWED - 1);
+	return min(ilog2(div) + 1, 7);
 }
 
 static bool is_spmi_pmic_clkdiv_enabled(struct clkdiv *clkdiv)
 {
 	unsigned int val = 0;
 
-	regmap_read(clkdiv->regmap, clkdiv->base + REG_EN_CTL,
-			 &val);
-	return (val & REG_EN_MASK) ? true : false;
+	regmap_read(clkdiv->regmap, clkdiv->base + REG_EN_CTL, &val);
+
+	return val & REG_EN_MASK;
 }
 
-static int spmi_pmic_clkdiv_set_enable_state(struct clkdiv *clkdiv,
-			bool enable_state)
+static int
+__spmi_pmic_clkdiv_set_enable_state(struct clkdiv *clkdiv, bool enable,
+				    unsigned int div_factor)
 {
-	int rc;
+	int ret;
+	unsigned int ns = clkdiv->cxo_period_ns;
+	unsigned int div = div_factor_to_div(div_factor);
 
-	rc = regmap_update_bits(clkdiv->regmap, clkdiv->base + REG_EN_CTL,
-				REG_EN_MASK,
-				(enable_state == true) ? REG_EN_MASK : 0);
-	if (rc)
-		return rc;
+	ret = regmap_update_bits(clkdiv->regmap, clkdiv->base + REG_EN_CTL,
+				 REG_EN_MASK, enable ? REG_EN_MASK : 0);
+	if (ret)
+		return ret;
 
-	if (enable_state == true)
-		ndelay(ENABLE_DELAY_NS(clkdiv->cxo_period_ns,
-				div_factor_to_div(clkdiv->div_factor)));
+	if (enable)
+		ndelay((2 + 3 * div) * ns);
 	else
-		ndelay(DISABLE_DELAY_NS(clkdiv->cxo_period_ns,
-				div_factor_to_div(clkdiv->div_factor)));
+		ndelay(3 * div * ns);
 
-	return rc;
+	return 0;
 }
 
-static int spmi_pmic_clkdiv_config_freq_div(struct clkdiv *clkdiv,
-			unsigned int div)
+static int spmi_pmic_clkdiv_set_enable_state(struct clkdiv *clkdiv, bool enable)
 {
 	unsigned int div_factor;
-	unsigned long flags;
-	bool enabled;
-	int rc;
-
-	div_factor = div_to_div_factor(div);
-
-	spin_lock_irqsave(&clkdiv->lock, flags);
-
-	enabled = is_spmi_pmic_clkdiv_enabled(clkdiv);
-	if (enabled) {
-		rc = spmi_pmic_clkdiv_set_enable_state(clkdiv, false);
-		if (rc)
-			goto fail;
-	}
 
-	rc = regmap_update_bits(clkdiv->regmap,
-				clkdiv->base + REG_DIV_CTL1,
-				DIV_CTL1_DIV_FACTOR_MASK, div_factor);
-	if (rc)
-		goto fail;
+	regmap_read(clkdiv->regmap, clkdiv->base + REG_DIV_CTL1, &div_factor);
+	div_factor &= DIV_CTL1_DIV_FACTOR_MASK;
 
-	clkdiv->div_factor = div_factor;
-
-	if (enabled)
-		rc = spmi_pmic_clkdiv_set_enable_state(clkdiv, true);
-
-fail:
-	spin_unlock_irqrestore(&clkdiv->lock, flags);
-	return rc;
+	return __spmi_pmic_clkdiv_set_enable_state(clkdiv, enable, div_factor);
 }
 
 static int clk_spmi_pmic_div_enable(struct clk_hw *hw)
 {
 	struct clkdiv *clkdiv = to_clkdiv(hw);
 	unsigned long flags;
-	int rc;
+	int ret;
 
 	spin_lock_irqsave(&clkdiv->lock, flags);
-	rc = spmi_pmic_clkdiv_set_enable_state(clkdiv, true);
+	ret = spmi_pmic_clkdiv_set_enable_state(clkdiv, true);
 	spin_unlock_irqrestore(&clkdiv->lock, flags);
 
-	return rc;
+	return ret;
 }
 
 static void clk_spmi_pmic_div_disable(struct clk_hw *hw)
@@ -163,35 +121,59 @@ static void clk_spmi_pmic_div_disable(struct clk_hw *hw)
 }
 
 static long clk_spmi_pmic_div_round_rate(struct clk_hw *hw, unsigned long rate,
-			unsigned long *parent_rate)
+					 unsigned long *parent_rate)
 {
-	unsigned long new_rate;
 	unsigned int div, div_factor;
 
 	div = DIV_ROUND_UP(*parent_rate, rate);
 	div_factor = div_to_div_factor(div);
-	new_rate = *parent_rate / div_factor_to_div(div_factor);
+	div = div_factor_to_div(div_factor);
 
-	return new_rate;
+	return *parent_rate / div;
 }
 
-static unsigned long clk_spmi_pmic_div_recalc_rate(struct clk_hw *hw,
-			unsigned long parent_rate)
+static unsigned long
+clk_spmi_pmic_div_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 {
 	struct clkdiv *clkdiv = to_clkdiv(hw);
-	unsigned long rate;
+	unsigned int div_factor;
 
-	rate = parent_rate / div_factor_to_div(clkdiv->div_factor);
+	regmap_read(clkdiv->regmap, clkdiv->base + REG_DIV_CTL1, &div_factor);
+	div_factor &= DIV_CTL1_DIV_FACTOR_MASK;
 
-	return rate;
+	return parent_rate / div_factor_to_div(div_factor);
 }
 
 static int clk_spmi_pmic_div_set_rate(struct clk_hw *hw, unsigned long rate,
-			unsigned long parent_rate)
+				      unsigned long parent_rate)
 {
 	struct clkdiv *clkdiv = to_clkdiv(hw);
+	unsigned int div_factor = div_to_div_factor(parent_rate / rate);
+	unsigned long flags;
+	bool enabled;
+	int ret;
+
+	spin_lock_irqsave(&clkdiv->lock, flags);
+	enabled = is_spmi_pmic_clkdiv_enabled(clkdiv);
+	if (enabled) {
+		ret = spmi_pmic_clkdiv_set_enable_state(clkdiv, false);
+		if (ret)
+			goto unlock;
+	}
+
+	ret = regmap_update_bits(clkdiv->regmap, clkdiv->base + REG_DIV_CTL1,
+				 DIV_CTL1_DIV_FACTOR_MASK, div_factor);
+	if (ret)
+		goto unlock;
+
+	if (enabled)
+		ret = __spmi_pmic_clkdiv_set_enable_state(clkdiv, true,
+							  div_factor);
 
-	return spmi_pmic_clkdiv_config_freq_div(clkdiv, parent_rate / rate);
+unlock:
+	spin_unlock_irqrestore(&clkdiv->lock, flags);
+
+	return ret;
 }
 
 static const struct clk_ops clk_spmi_pmic_div_ops = {
@@ -203,30 +185,25 @@ static const struct clk_ops clk_spmi_pmic_div_ops = {
 };
 
 struct spmi_pmic_div_clk_cc {
-	struct clk_hw	**div_clks;
 	int		nclks;
+	struct clkdiv	clks[];
 };
 
-#define SPMI_PMIC_CLKDIV_MIN_INDEX	1
-
-static struct clk_hw *spmi_pmic_div_clk_hw_get(struct of_phandle_args *clkspec,
-				      void *data)
+static struct clk_hw *
+spmi_pmic_div_clk_hw_get(struct of_phandle_args *clkspec, void *data)
 {
-	struct spmi_pmic_div_clk_cc *clk_cc = data;
-	unsigned int idx = (clkspec->args[0] - SPMI_PMIC_CLKDIV_MIN_INDEX);
+	struct spmi_pmic_div_clk_cc *cc = data;
+	int idx = clkspec->args[0] - 1; /* Start at 1 instead of 0 */
 
-	if (idx < 0 || idx >= clk_cc->nclks) {
-		pr_err("%s: index value %u is invalid; allowed range: [%d, %d]\n",
-		       __func__, clkspec->args[0], SPMI_PMIC_CLKDIV_MIN_INDEX,
-		       clk_cc->nclks);
+	if (idx < 0 || idx >= cc->nclks) {
+		pr_err("%s: index value %u is invalid; allowed range [1, %d]\n",
+		       __func__, clkspec->args[0], cc->nclks);
 		return ERR_PTR(-EINVAL);
 	}
 
-	return clk_cc->div_clks[idx];
+	return &cc->clks[idx].hw;
 }
 
-#define SPMI_PMIC_DIV_CLK_SIZE		0x100
-
 static const struct of_device_id spmi_pmic_clkdiv_match_table[] = {
 	{
 		.compatible = "qcom,spmi-clkdiv",
@@ -242,20 +219,21 @@ MODULE_DEVICE_TABLE(of, spmi_pmic_clkdiv_match_table);
 
 static int spmi_pmic_clkdiv_probe(struct platform_device *pdev)
 {
-	struct spmi_pmic_div_clk_cc *clk_cc;
+	struct spmi_pmic_div_clk_cc *cc;
 	struct clk_init_data init = {};
 	struct clkdiv *clkdiv;
 	struct clk *cxo;
 	struct regmap *regmap;
 	struct device *dev = &pdev->dev;
+	struct device_node *of_node = dev->of_node;
 	const char *parent_name;
-	int nclks, i, rc, cxo_hz;
+	int nclks, i, ret, cxo_hz;
 	u32 start;
 
-	rc = of_property_read_u32(dev->of_node, "reg", &start);
-	if (rc < 0) {
+	ret = of_property_read_u32(of_node, "reg", &start);
+	if (ret < 0) {
 		dev_err(dev, "reg property reading failed\n");
-		return rc;
+		return ret;
 	}
 
 	regmap = dev_get_regmap(dev->parent, NULL);
@@ -264,62 +242,51 @@ static int spmi_pmic_clkdiv_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	nclks = (uintptr_t)of_match_node(spmi_pmic_clkdiv_match_table,
-					 dev->of_node)->data;
-
-	clkdiv = devm_kcalloc(dev, nclks, sizeof(*clkdiv), GFP_KERNEL);
-	if (!clkdiv)
-		return -ENOMEM;
-
-	clk_cc = devm_kzalloc(&pdev->dev, sizeof(*clk_cc), GFP_KERNEL);
-	if (!clk_cc)
-		return -ENOMEM;
+	nclks = (uintptr_t)of_device_get_match_data(dev);
+	if (!nclks)
+		return -EINVAL;
 
-	clk_cc->div_clks = devm_kcalloc(&pdev->dev, nclks,
-					sizeof(*clk_cc->div_clks), GFP_KERNEL);
-	if (!clk_cc->div_clks)
+	cc = devm_kzalloc(dev, sizeof(*cc) + sizeof(*cc->clks) * nclks,
+			  GFP_KERNEL);
+	if (!cc)
 		return -ENOMEM;
+	cc->nclks = nclks;
 
 	cxo = clk_get(dev, "xo");
 	if (IS_ERR(cxo)) {
-		rc = PTR_ERR(cxo);
-		if (rc != -EPROBE_DEFER)
-			dev_err(dev, "failed to get xo clock");
-		return rc;
+		ret = PTR_ERR(cxo);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get xo clock\n");
+		return ret;
 	}
 	cxo_hz = clk_get_rate(cxo);
 	clk_put(cxo);
 
-	parent_name = of_clk_get_parent_name(dev->of_node, 0);
+	parent_name = of_clk_get_parent_name(of_node, 0);
 	if (!parent_name) {
 		dev_err(dev, "missing parent clock\n");
 		return -ENODEV;
 	}
 
 	init.parent_names = &parent_name;
-	init.num_parents = parent_name ? 1 : 0;
+	init.num_parents = 1;
 	init.ops = &clk_spmi_pmic_div_ops;
-	init.flags = 0;
 
-	for (i = 0; i < nclks; i++) {
+	for (i = 0, clkdiv = cc->clks; i < nclks; i++) {
 		spin_lock_init(&clkdiv[i].lock);
-		clkdiv[i].base = start + i * SPMI_PMIC_DIV_CLK_SIZE;
+		clkdiv[i].base = start + i * 0x100;
 		clkdiv[i].regmap = regmap;
 		clkdiv[i].cxo_period_ns = NSEC_PER_SEC / cxo_hz;
 		init.name = kasprintf(GFP_KERNEL, "div_clk%d", i + 1);
 		clkdiv[i].hw.init = &init;
-		rc = devm_clk_hw_register(dev, &clkdiv[i].hw);
-		kfree(init.name); /* clock framework made a copy of the name */
-		if (rc)
-			return rc;
 
-		clk_cc->div_clks[i] = &clkdiv[i].hw;
+		ret = devm_clk_hw_register(dev, &clkdiv[i].hw);
+		kfree(init.name); /* clk framework made a copy */
+		if (ret)
+			return ret;
 	}
 
-	clk_cc->nclks = nclks;
-	rc = of_clk_add_hw_provider(pdev->dev.of_node, spmi_pmic_div_clk_hw_get,
-				    clk_cc);
-	return rc;
+	return of_clk_add_hw_provider(of_node, spmi_pmic_div_clk_hw_get, cc);
 }
 
 static int spmi_pmic_clkdiv_remove(struct platform_device *pdev)


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists