Re: [PATCH 06/14] staging: clocking-wizard: Swap CCF clock providers

[Michal Simek <michal.simek@xilinx.com>]

On 7.5.2018 03:20, James Kelly wrote:
> Replace existing CCF clock providers with new clock provider that can
> be enhanced to meet our needs.
> 
> AXI clock prepare/enable/disable/unprepare is now managed by regmap APIs.
> 
> Unregistering of clk instances now handled by devm APIs.
> 
> Drop warning that fractional ratios are not supported because it used
> undocumented register fields and it won't be needed anymore.
> 
> Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
> ---
>  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 211 +++++++--------------
>  1 file changed, 71 insertions(+), 140 deletions(-)
> 
> diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> index ba9b1dc93d50..1dbeda92fb9a 100644
> --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> @@ -76,24 +76,6 @@
>  #define WZRD_CLKNAME_IN1	"clk_in1"
>  #define WZRD_FLAG_MULTIPLY	BIT(0)
>  
> -#define WZRD_CLK_CFG_REG(n)	(0x200 + 4 * (n))
> -
> -#define WZRD_CLKOUT0_FRAC_EN	BIT(18)
> -#define WZRD_CLKFBOUT_FRAC_EN	BIT(26)
> -
> -#define WZRD_CLKFBOUT_MULT_SHIFT	8
> -#define WZRD_CLKFBOUT_MULT_MASK		(0xff << WZRD_CLKFBOUT_MULT_SHIFT)
> -#define WZRD_DIVCLK_DIVIDE_SHIFT	0
> -#define WZRD_DIVCLK_DIVIDE_MASK		(0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
> -#define WZRD_CLKOUT_DIVIDE_SHIFT	0
> -#define WZRD_CLKOUT_DIVIDE_MASK		(0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
> -
> -enum clk_wzrd_int_clks {
> -	wzrd_clk_mul_div,
> -	wzrd_clk_mul,
> -	wzrd_clk_int_max
> -};
> -
>  enum clk_wzrd_clk {
>  	WZRD_CLK_DIV,
>  	WZRD_CLK_PLL,
> @@ -149,14 +131,13 @@ struct clk_wzrd_clk_data {
>  		container_of(_hw, struct clk_wzrd_clk_data, hw)
>  
>  /**
> - * struct clk_wzrd:
> - * @clk_data:		Clock data
> + * struct clk_wzrd - Platform driver data for clocking wizard device
> + *
> + * @clk_data:		Clock data accessible to other devices via device tree
>   * @nb:			Notifier block
> - * @base:		Memory base
>   * @regmap:		Register map for hardware
>   * @clk_in1:		Handle to input clock 'clk_in1'
>   * @axi_clk:		Handle to input clock 's_axi_aclk'
> - * @clks_internal:	Internal clocks
>   * @clkout:		Output clocks
>   * @speed_grade:	Speed grade of the device
>   * @suspended:		Flag indicating power state of the device
> @@ -167,11 +148,9 @@ struct clk_wzrd_clk_data {
>  struct clk_wzrd {
>  	struct clk_onecell_data		clk_data;
>  	struct notifier_block		nb;
> -	void __iomem			*base;
>  	struct regmap			*regmap;
>  	struct clk			*clk_in1;
>  	struct clk			*axi_clk;
> -	struct clk			*clks_internal[wzrd_clk_int_max];
>  	struct clk			*clkout[WZRD_NUM_OUTPUTS];
>  	unsigned int			speed_grade;
>  	bool				suspended;
> @@ -179,7 +158,6 @@ struct clk_wzrd {
>  	struct clk_wzrd_clk_data	pll_data;
>  	struct clk_wzrd_clk_data	clkout_data[0];
>  };
> -
>  #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
>  
>  /* maximum frequencies for input/output clocks per speed grade */
> @@ -321,7 +299,6 @@ static int __maybe_unused clk_wzrd_suspend(struct device *dev)
>  {
>  	struct clk_wzrd *cw = dev_get_drvdata(dev);
>  
> -	clk_disable_unprepare(cw->axi_clk);
>  	cw->suspended = true;
>  
>  	return 0;
> @@ -329,15 +306,8 @@ static int __maybe_unused clk_wzrd_suspend(struct device *dev)
>  
>  static int __maybe_unused clk_wzrd_resume(struct device *dev)
>  {
> -	int ret;
>  	struct clk_wzrd *cw = dev_get_drvdata(dev);
>  
> -	ret = clk_prepare_enable(cw->axi_clk);
> -	if (ret) {
> -		dev_err(dev, "unable to enable s_axi_aclk\n");
> -		return ret;
> -	}
> -
>  	cw->suspended = false;
>  
>  	return 0;
> @@ -348,17 +318,32 @@ static SIMPLE_DEV_PM_OPS(clk_wzrd_dev_pm_ops, clk_wzrd_suspend,
>  
>  static int clk_wzrd_get_device_tree_data(struct device *dev)
>  {
> -	int ret;
> -	unsigned long rate;
> +	int num_outputs, ret;
>  	struct clk_wzrd *cw;
> +	struct device_node *node = dev->of_node;
> +
> +	num_outputs = of_property_count_strings(node,
> +						"clock-output-names");
> +	if (num_outputs < 1) {
> +		dev_err(dev, "No clock output names in device tree\n");
> +		if (num_outputs < 0)
> +			return num_outputs;
> +		else
> +			return -EINVAL;
> +	}
> +	if (num_outputs > WZRD_NUM_OUTPUTS) {
> +		dev_warn(dev, "Too many clock output names in device tree\n");
> +		num_outputs = WZRD_NUM_OUTPUTS;
> +	}
>  
> -	cw = devm_kzalloc(dev, sizeof(*cw), GFP_KERNEL);
> +	cw = devm_kzalloc(dev, sizeof(*cw) + num_outputs *
> +			  sizeof(struct clk_wzrd_clk_data), GFP_KERNEL);
>  	if (!cw)
>  		return -ENOMEM;
>  	dev_set_drvdata(dev, cw);
> +	cw->clk_data.clk_num = num_outputs;
>  
> -	ret = of_property_read_u32(dev->of_node, "speed-grade",
> -				   &cw->speed_grade);
> +	ret = of_property_read_u32(node, "speed-grade", &cw->speed_grade);
>  	if (!ret) {
>  		if (cw->speed_grade < 1 || cw->speed_grade > 3) {
>  			dev_warn(dev, "invalid speed grade '%d'\n",
> @@ -380,18 +365,12 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
>  			dev_err(dev, "Clock %s not found\n", WZRD_CLKNAME_AXI);
>  		return PTR_ERR(cw->axi_clk);
>  	}
> -	ret = clk_prepare_enable(cw->axi_clk);
> +	ret = clk_set_max_rate(cw->axi_clk, WZRD_ACLK_MAX_FREQ);
>  	if (ret) {
> -		dev_err(dev, "enabling %s failed\n", WZRD_CLKNAME_AXI);
> +		dev_err(dev, "Unable to set clock %s to valid range\n",
> +			WZRD_CLKNAME_AXI);
>  		return ret;
>  	}
> -	rate = clk_get_rate(cw->axi_clk);
> -	if (rate > WZRD_ACLK_MAX_FREQ) {
> -		dev_err(dev, "%s frequency (%lu) too high\n", WZRD_CLKNAME_AXI,
> -			rate);
> -		clk_disable_unprepare(cw->axi_clk);
> -		return -EINVAL;
> -	}
>  
>  	return 0;
>  }
> @@ -399,12 +378,18 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
>  static int clk_wzrd_regmap_alloc(struct device *dev)
>  {
>  	struct resource *mem;
> +	void __iomem *base;
>  	struct clk_wzrd *cw = dev_get_drvdata(dev);
>  
>  	mem = platform_get_resource(to_platform_device(dev), IORESOURCE_MEM, 0);
> -	cw->base = devm_ioremap_resource(dev, mem);
> -	if (IS_ERR(cw->base))
> -		return PTR_ERR(cw->base);
> +	base = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	cw->regmap = devm_regmap_init_mmio_clk(dev, WZRD_CLKNAME_AXI,
> +					       base, &clk_wzrd_regmap_config);
> +	if (IS_ERR(cw->regmap))
> +		return PTR_ERR(cw->regmap);
>  
>  	return 0;
>  }
> @@ -412,115 +397,68 @@ static int clk_wzrd_regmap_alloc(struct device *dev)
>  static int clk_wzrd_register_ccf(struct device *dev)
>  {
>  	int i, ret;
> -	u32 reg;
> -	const char *clk_name;
> +	const char *clk_div_name;
> +	const char *clk_pll_name;
> +
>  	struct clk_wzrd *cw = dev_get_drvdata(dev);
>  
> -	/* we don't support fractional div/mul yet */
> -	reg = readl(cw->base + WZRD_CLK_CFG_REG(0)) &
> -		    WZRD_CLKFBOUT_FRAC_EN;
> -	reg |= readl(cw->base + WZRD_CLK_CFG_REG(2)) &
> -		     WZRD_CLKOUT0_FRAC_EN;
> -	if (reg)
> -		dev_warn(dev, "fractional div/mul not supported\n");
> -
> -	/* register div */
> -	reg = (readl(cw->base + WZRD_CLK_CFG_REG(0)) &
> -	       WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
> -	clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(dev));
> -	if (!clk_name) {
> -		ret = -ENOMEM;
> -		goto err_disable_clk;
> -	}
> -	cw->clks_internal[wzrd_clk_mul_div] = clk_register_fixed_factor(
> -			dev, clk_name,
> -			__clk_get_name(cw->clk_in1),
> -			0, 1, reg);
> -	kfree(clk_name);
> -	if (IS_ERR(cw->clks_internal[wzrd_clk_mul_div])) {
> -		dev_err(dev, "unable to register divider clock\n");
> -		ret = PTR_ERR(cw->clks_internal[wzrd_clk_mul_div]);
> -		goto err_disable_clk;
> -	}
> +	clk_div_name = kasprintf(GFP_KERNEL, "%s_div", dev_name(dev));
> +	if (!clk_div_name)
> +		return -ENOMEM;
> +	ret = clk_wzrd_register_clk(dev, clk_div_name, WZRD_CLK_DIV, 0, 0);
> +	if (ret)
> +		goto err_free_div_name;
>  
> -	/* register multiplier */
> -	reg = (readl(cw->base + WZRD_CLK_CFG_REG(0)) &
> -	       WZRD_CLKFBOUT_MULT_MASK) >> WZRD_CLKFBOUT_MULT_SHIFT;
> -	clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(dev));
> -	if (!clk_name) {
> +	clk_pll_name = kasprintf(GFP_KERNEL, "%s_pll", dev_name(dev));
> +	if (!clk_pll_name) {
>  		ret = -ENOMEM;
> -		goto err_rm_int_clk;
> -	}
> -	cw->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor(
> -			dev, clk_name,
> -			__clk_get_name(cw->clks_internal[wzrd_clk_mul_div]),
> -			0, reg, 1);
> -	if (IS_ERR(cw->clks_internal[wzrd_clk_mul])) {
> -		dev_err(dev, "unable to register fixed-factor clock\n");
> -		ret = PTR_ERR(cw->clks_internal[wzrd_clk_mul]);
> -		goto err_rm_int_clk;
> +		goto err_free_div_name;
>  	}
> +	ret = clk_wzrd_register_clk(dev, clk_pll_name, WZRD_CLK_PLL, 0, 0);
> +	if (ret)
> +		goto err_free_pll_name;
>  
> -	/* register div per output */
> -	for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) {
> +	for (i = cw->clk_data.clk_num - 1; i >= 0 ; i--) {
>  		const char *clkout_name;
>  
>  		if (of_property_read_string_index(dev->of_node,
>  						  "clock-output-names", i,
>  						  &clkout_name)) {
> -			dev_err(dev,
> -				"clock output name not specified\n");
> +			dev_err(dev, "clock output %d name not specified\n", i);
>  			ret = -EINVAL;
> -			goto err_rm_int_clks;
> +			goto err_free_pll_name;
>  		}
> -		reg = readl(cw->base + WZRD_CLK_CFG_REG(2) + i * 12);
> -		reg &= WZRD_CLKOUT_DIVIDE_MASK;
> -		reg >>= WZRD_CLKOUT_DIVIDE_SHIFT;
> -		cw->clkout[i] = clk_register_fixed_factor(dev, clkout_name,
> -							  clk_name, 0, 1, reg);
> -		if (IS_ERR(cw->clkout[i])) {
> -			int j;
> -
> -			for (j = i + 1; j < WZRD_NUM_OUTPUTS; j++)
> -				clk_unregister(cw->clkout[j]);
> -			dev_err(dev,
> -				"unable to register divider clock\n");
> -			ret = PTR_ERR(cw->clkout[i]);
> -			goto err_rm_int_clks;
> -		}
> -	}
>  
> -	kfree(clk_name);
> +		ret = clk_wzrd_register_clk(dev, clkout_name, WZRD_CLK_OUT,
> +					    i, 0);
> +		if (ret)
> +			goto err_free_pll_name;
> +
> +		cw->clkout[i] = cw->clkout_data[i].hw.clk;
> +	}
>  
>  	cw->clk_data.clks = cw->clkout;
> -	cw->clk_data.clk_num = ARRAY_SIZE(cw->clkout);
>  	of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
>  			    &cw->clk_data);
>  
>  	if (cw->speed_grade) {
>  		cw->nb.notifier_call = clk_wzrd_clk_notifier;
>  
> -		ret = clk_notifier_register(cw->clk_in1,
> -					    &cw->nb);
> -		if (ret)
> -			dev_warn(dev,
> -				 "unable to register clock notifier\n");
> +		ret = clk_notifier_register(cw->clk_in1, &cw->nb);
> +			if (ret)
> +				dev_warn(dev, "Unable to register input clock notifier\n");
>  
>  		ret = clk_notifier_register(cw->axi_clk, &cw->nb);
> -		if (ret)
> -			dev_warn(dev,
> -				 "unable to register clock notifier\n");
> +			if (ret)
> +				dev_warn(dev, "Unable to register AXI clock notifier\n");
>  	}
>  
> -	return 0;
> +	ret = 0;
>  
> -err_rm_int_clks:
> -	clk_unregister(cw->clks_internal[1]);
> -err_rm_int_clk:
> -	kfree(clk_name);
> -	clk_unregister(cw->clks_internal[0]);
> -err_disable_clk:
> -	clk_disable_unprepare(cw->axi_clk);
> +err_free_pll_name:
> +	kfree(clk_pll_name);
> +err_free_div_name:
> +	kfree(clk_div_name);
>  
>  	return ret;
>  }
> @@ -547,23 +485,15 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>  
>  static int clk_wzrd_remove(struct platform_device *pdev)
>  {
> -	int i;
>  	struct clk_wzrd *cw = platform_get_drvdata(pdev);
>  
>  	of_clk_del_provider(pdev->dev.of_node);
>  
> -	for (i = 0; i < WZRD_NUM_OUTPUTS; i++)
> -		clk_unregister(cw->clkout[i]);
> -	for (i = 0; i < wzrd_clk_int_max; i++)
> -		clk_unregister(cw->clks_internal[i]);
> -
>  	if (cw->speed_grade) {
>  		clk_notifier_unregister(cw->axi_clk, &cw->nb);
>  		clk_notifier_unregister(cw->clk_in1, &cw->nb);
>  	}
>  
> -	clk_disable_unprepare(cw->axi_clk);
> -
>  	return 0;
>  }
>  
> @@ -577,6 +507,7 @@ static struct platform_driver clk_wzrd_driver = {
>  	.driver = {
>  		.name = "clk-wizard",
>  		.of_match_table = clk_wzrd_ids,
> +		.owner = THIS_MODULE,

This is not needed and coccinelle check this.
drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:510:3-8: No need
to set .owner here. The core will do it.


Also there is one more warning.
drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:391:1-3:
WARNING: PTR_ERR_OR_ZERO can be used

Thanks,
Michal