Message-ID: <86711b65-52a8-d96a-1039-3e5fc52a027f@codeaurora.org>
Date:   Fri, 17 Nov 2017 15:31:13 +0530
From:   Tirupathi Reddy T <tirupath@...eaurora.org>
To:     Stephen Boyd <sboyd@...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 11/15/2017 10:36 PM, Stephen Boyd wrote:
> 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.
Fixed in Patch version 6.
>
>> +		    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.
Fixed in Patch version 6.
>
>> +
>> +=======
>> +Example
>> +=======
>> +
>> +pm8998_clk_divs: qcom,clock@...0 {
> clock-controller@...0?
Fixed in Patch version 6.
>
>> +	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.
Fixed in Patch version 6.
>
>> +	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.
Verified. The changes look good.
>
> ----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)
>
>

Powered by blists - more mailing lists