Re: [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks

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

On 16. 11. 20 8:55, Michael Tretter wrote:
> The VCU System-Level Control uses an internal PLL to drive the core and
> MCU clock for the allegro encoder and decoder based on an external PL
> clock.
> 
> In order be able to ensure that the clocks are enabled and to get their
> rate from other drivers, the module must implement a clock provider and
> register the clocks at the common clock framework. Other drivers are
> then able to access the clock via devicetree bindings.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/soc/xilinx/xlnx_vcu.c | 191 +++++++++++++++++++++++++++-------
>  1 file changed, 154 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
> index 725e646aa726..cedc8b7859f7 100644
> --- a/drivers/soc/xilinx/xlnx_vcu.c
> +++ b/drivers/soc/xilinx/xlnx_vcu.c
> @@ -18,6 +18,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  
> +#include <dt-bindings/clock/xlnx-vcu.h>
> +
>  /* vcu slcr registers, bitmask and shift */
>  #define VCU_PLL_CTRL			0x24
>  #define VCU_PLL_CTRL_RESET_MASK		0x01
> @@ -50,11 +52,6 @@
>  #define VCU_ENC_MCU_CTRL		0x34
>  #define VCU_DEC_CORE_CTRL		0x38
>  #define VCU_DEC_MCU_CTRL		0x3c
> -#define VCU_PLL_DIVISOR_MASK		0x3f
> -#define VCU_PLL_DIVISOR_SHIFT		4
> -#define VCU_SRCSEL_MASK			0x01
> -#define VCU_SRCSEL_SHIFT		0
> -#define VCU_SRCSEL_PLL			1
>  
>  #define VCU_PLL_STATUS			0x60
>  #define VCU_PLL_STATUS_LOCK_STATUS_MASK	0x01
> @@ -82,6 +79,7 @@ struct xvcu_device {
>  	struct regmap *logicore_reg_ba;
>  	void __iomem *vcu_slcr_ba;
>  	struct clk_hw *pll;
> +	struct clk_hw_onecell_data *clk_data;

The same here with clk_data - please update kernel-doc format.

>  };
>  
>  static struct regmap_config vcu_settings_regmap_config = {
> @@ -403,7 +401,7 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
>  	u32 refclk, coreclk, mcuclk, inte, deci;
>  	u32 divisor_mcu, divisor_core, fvco;
>  	u32 clkoutdiv, vcu_pll_ctrl, pll_clk;
> -	u32 mod, ctrl;
> +	u32 mod;
>  	int i;
>  	int ret;
>  	const struct xvcu_pll_cfg *found = NULL;
> @@ -478,37 +476,6 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
>  	dev_dbg(xvcu->dev, "Actual Core clock freq is %uHz\n", coreclk);
>  	dev_dbg(xvcu->dev, "Actual Mcu clock freq is %uHz\n", mcuclk);
>  
> -	/* Set divisor for the core and mcu clock */
> -	ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_ENC_CORE_CTRL);
> -	ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
> -	ctrl |= (divisor_core & VCU_PLL_DIVISOR_MASK) <<
> -		 VCU_PLL_DIVISOR_SHIFT;
> -	ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
> -	ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
> -	xvcu_write(xvcu->vcu_slcr_ba, VCU_ENC_CORE_CTRL, ctrl);
> -
> -	ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_DEC_CORE_CTRL);
> -	ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
> -	ctrl |= (divisor_core & VCU_PLL_DIVISOR_MASK) <<
> -		 VCU_PLL_DIVISOR_SHIFT;
> -	ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
> -	ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
> -	xvcu_write(xvcu->vcu_slcr_ba, VCU_DEC_CORE_CTRL, ctrl);
> -
> -	ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_ENC_MCU_CTRL);
> -	ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
> -	ctrl |= (divisor_mcu & VCU_PLL_DIVISOR_MASK) << VCU_PLL_DIVISOR_SHIFT;
> -	ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
> -	ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
> -	xvcu_write(xvcu->vcu_slcr_ba, VCU_ENC_MCU_CTRL, ctrl);
> -
> -	ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_DEC_MCU_CTRL);
> -	ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
> -	ctrl |= (divisor_mcu & VCU_PLL_DIVISOR_MASK) << VCU_PLL_DIVISOR_SHIFT;
> -	ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
> -	ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
> -	xvcu_write(xvcu->vcu_slcr_ba, VCU_DEC_MCU_CTRL, ctrl);
> -
>  	ret = xvcu_pll_set_rate(xvcu, fvco, refclk);
>  	if (ret)
>  		return ret;
> @@ -545,6 +512,146 @@ static int xvcu_set_pll(struct xvcu_device *xvcu)
>  	return xvcu_pll_enable(xvcu);
>  }
>  
> +static struct clk_hw *xvcu_clk_hw_register_leaf(struct device *dev,
> +						const char *name,
> +						const char * const *parent_names,
> +						u8 num_parents,
> +						struct clk_hw *parent_default,
> +						void __iomem *reg,
> +						spinlock_t *lock)
> +{
> +	unsigned long flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT;
> +	u8 divider_flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO |
> +			   CLK_DIVIDER_ROUND_CLOSEST;
> +	struct clk_hw *mux = NULL;
> +	struct clk_hw *divider = NULL;
> +	struct clk_hw *gate = NULL;
> +	char *name_mux;
> +	char *name_div;
> +	int err;
> +
> +	name_mux = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_mux");
> +	if (!name_mux) {
> +		err = -ENOMEM;
> +		goto err;
> +	}
> +	mux = clk_hw_register_mux(dev, name_mux, parent_names, num_parents,
> +				  flags, reg, 0, 1, 0, lock);
> +	if (IS_ERR(mux)) {
> +		err = PTR_ERR(divider);

mux here?
And smatch is reporting also this issue. Please take a look.

drivers/soc/xilinx/xlnx_vcu.c:541 xvcu_clk_hw_register_leaf() warn:
passing zero to 'PTR_ERR'
drivers/soc/xilinx/xlnx_vcu.c:577 xvcu_clk_hw_register_leaf() warn:
passing zero to 'ERR_PTR'



> +		goto err;
> +	}
> +	clk_hw_set_parent(mux, parent_default);
> +
> +	name_div = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_div");
> +	if (!name_div) {
> +		err = -ENOMEM;
> +		goto err;
> +	}
> +	divider = clk_hw_register_divider_parent_hw(dev, name_div, mux, flags,
> +						    reg, 4, 6, divider_flags,
> +						    lock);
> +	if (IS_ERR(divider)) {
> +		err = PTR_ERR(divider);
> +		goto err;
> +	}
> +
> +	gate = clk_hw_register_gate_parent_hw(dev, name, divider,
> +					      CLK_SET_RATE_PARENT, reg, 12, 0,
> +					      lock);
> +	if (IS_ERR(gate)) {
> +		err = PTR_ERR(gate);
> +		goto err;
> +	}
> +
> +	return gate;
> +
> +err:
> +	if (!IS_ERR_OR_NULL(gate))
> +		clk_hw_unregister_gate(gate);
> +	if (!IS_ERR_OR_NULL(divider))
> +		clk_hw_unregister_divider(divider);
> +	if (!IS_ERR_OR_NULL(mux))
> +		clk_hw_unregister_divider(mux);
> +
> +	return ERR_PTR(err);
> +}
> +
> +static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
> +{
> +	struct clk_hw *gate = hw;
> +	struct clk_hw *divider;
> +	struct clk_hw *mux;
> +
> +	if (!gate)
> +		return;
> +
> +	divider = clk_hw_get_parent(gate);
> +	clk_hw_unregister_gate(gate);
> +	if (!divider)
> +		return;
> +
> +	mux = clk_hw_get_parent(divider);
> +	clk_hw_unregister_mux(mux);
> +	if (!divider)
> +		return;
> +
> +	clk_hw_unregister_divider(divider);
> +}
> +
> +static DEFINE_SPINLOCK(venc_core_lock);
> +static DEFINE_SPINLOCK(venc_mcu_lock);
> +
> +static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
> +{
> +	struct device *dev = xvcu->dev;
> +	const char *parent_names[2];
> +	struct clk_hw *parent_default;
> +	struct clk_hw_onecell_data *data;
> +	struct clk_hw **hws;
> +	void __iomem *reg_base = xvcu->vcu_slcr_ba;
> +
> +	data = devm_kzalloc(dev, struct_size(data, hws, CLK_XVCU_NUM_CLOCKS), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +	data->num = CLK_XVCU_NUM_CLOCKS;
> +	hws = data->hws;
> +
> +	xvcu->clk_data = data;
> +
> +	parent_default = xvcu->pll;
> +	parent_names[0] = "dummy";
> +	parent_names[1] = clk_hw_get_name(parent_default);
> +
> +	hws[CLK_XVCU_ENC_CORE] =
> +		xvcu_clk_hw_register_leaf(dev, "venc_core_clk",
> +					  parent_names,
> +					  ARRAY_SIZE(parent_names),
> +					  parent_default,
> +					  reg_base + VCU_ENC_CORE_CTRL,
> +					  &venc_core_lock);
> +	hws[CLK_XVCU_ENC_MCU] =
> +		xvcu_clk_hw_register_leaf(dev, "venc_mcu_clk",
> +					  parent_names,
> +					  ARRAY_SIZE(parent_names),
> +					  parent_default,
> +					  reg_base + VCU_ENC_MCU_CTRL,
> +					  &venc_mcu_lock);
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, data);
> +}
> +
> +static void xvcu_unregister_clock_provider(struct xvcu_device *xvcu)
> +{
> +	struct clk_hw_onecell_data *data = xvcu->clk_data;
> +	struct clk_hw **hws = data->hws;
> +
> +	if (!IS_ERR_OR_NULL(hws[CLK_XVCU_ENC_MCU]))
> +		xvcu_clk_hw_unregister_leaf(hws[CLK_XVCU_ENC_MCU]);
> +	if (!IS_ERR_OR_NULL(hws[CLK_XVCU_ENC_CORE]))
> +		xvcu_clk_hw_unregister_leaf(hws[CLK_XVCU_ENC_CORE]);
> +}
> +
>  /**
>   * xvcu_probe - Probe existence of the logicoreIP
>   *			and initialize PLL
> @@ -639,10 +746,18 @@ static int xvcu_probe(struct platform_device *pdev)
>  		goto error_pll_ref;
>  	}
>  
> +	ret = xvcu_register_clock_provider(xvcu);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register clock provider\n");
> +		goto error_clk_provider;
> +	}
> +
>  	dev_set_drvdata(&pdev->dev, xvcu);
>  
>  	return 0;
>  
> +error_clk_provider:
> +	xvcu_unregister_clock_provider(xvcu);
>  error_pll_ref:
>  	clk_disable_unprepare(xvcu->aclk);
>  	return ret;
> @@ -664,6 +779,8 @@ static int xvcu_remove(struct platform_device *pdev)
>  	if (!xvcu)
>  		return -ENODEV;
>  
> +	xvcu_unregister_clock_provider(xvcu);
> +
>  	/* Add the the Gasket isolation and put the VCU in reset. */
>  	regmap_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, 0);
>  
> 

M