On 16. 11. 20 8:55, Michael Tretter wrote:
> Do not configure the PLL when probing the driver, but register the clock
> in the clock framework and do the configuration based on the respective
> callbacks.
>
> This is necessary to allow the consumers, i.e., encoder and decoder
> drivers, of the xlnx_vcu clock provider to set the clock rate and
> actually enable the clocks without relying on some pre-configuration.
>
> Signed-off-by: Michael Tretter <m.tretter@xxxxxxxxxxxxxx>
> ---
> drivers/soc/xilinx/xlnx_vcu.c | 137 ++++++++++++++++++++++++++--------
> 1 file changed, 106 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
> index cf8456b4ef78..84d7c46cd42f 100644
> --- a/drivers/soc/xilinx/xlnx_vcu.c
> +++ b/drivers/soc/xilinx/xlnx_vcu.c
> @@ -257,9 +257,18 @@ static void xvcu_write_field_reg(void __iomem *iomem, int offset,
> xvcu_write(iomem, offset, val);
> }
>
> -static int xvcu_pll_wait_for_lock(struct xvcu_device *xvcu)
> +#define to_vcu_pll(_hw) container_of(_hw, struct vcu_pll, hw)
> +
> +struct vcu_pll {
> + struct clk_hw hw;
> + void __iomem *reg_base;
> + unsigned long fvco_min;
> + unsigned long fvco_max;
> +};
> +
> +static int xvcu_pll_wait_for_lock(struct vcu_pll *pll)
> {
> - void __iomem *base = xvcu->vcu_slcr_ba;
> + void __iomem *base = pll->reg_base;
> unsigned long timeout;
> u32 lock_status;
>
> @@ -307,9 +316,9 @@ static const struct xvcu_pll_cfg *xvcu_find_cfg(int div)
> return cfg;
> }
>
> -static int xvcu_pll_set_div(struct xvcu_device *xvcu, int div)
> +static int xvcu_pll_set_div(struct vcu_pll *pll, int div)
> {
> - void __iomem *base = xvcu->vcu_slcr_ba;
> + void __iomem *base = pll->reg_base;
> const struct xvcu_pll_cfg *cfg = NULL;
> u32 vcu_pll_ctrl;
> u32 cfg_val;
> @@ -334,24 +343,49 @@ static int xvcu_pll_set_div(struct xvcu_device *xvcu, int div)
> return 0;
> }
>
> -static int xvcu_pll_set_rate(struct xvcu_device *xvcu,
> +static long xvcu_pll_round_rate(struct clk_hw *hw,
> + unsigned long rate, unsigned long *parent_rate)
> +{
> + struct vcu_pll *pll = to_vcu_pll(hw);
> + unsigned int feedback_div;
> +
> + rate = clamp_t(unsigned long, rate, pll->fvco_min, pll->fvco_max);
> +
> + feedback_div = DIV_ROUND_CLOSEST_ULL(rate, *parent_rate);
> + feedback_div = clamp_t(unsigned int, feedback_div, 25, 125);
> +
> + return *parent_rate * feedback_div;
> +}
> +
> +static unsigned long xvcu_pll_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct vcu_pll *pll = to_vcu_pll(hw);
> + void __iomem *base = pll->reg_base;
> + unsigned int div;
> + u32 vcu_pll_ctrl;
> +
> + vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
> + div = (vcu_pll_ctrl >> VCU_PLL_CTRL_FBDIV_SHIFT) & VCU_PLL_CTRL_FBDIV_MASK;
> +
> + return div * parent_rate;
> +}
> +
> +static int xvcu_pll_set_rate(struct clk_hw *hw,
> unsigned long rate, unsigned long parent_rate)
> {
> - return xvcu_pll_set_div(xvcu, rate / parent_rate);
> + struct vcu_pll *pll = to_vcu_pll(hw);
> +
> + return xvcu_pll_set_div(pll, rate / parent_rate);
> }
>
> -static int xvcu_pll_enable(struct xvcu_device *xvcu)
> +static int xvcu_pll_enable(struct clk_hw *hw)
> {
> - void __iomem *base = xvcu->vcu_slcr_ba;
> + struct vcu_pll *pll = to_vcu_pll(hw);
> + void __iomem *base = pll->reg_base;
> u32 vcu_pll_ctrl;
> int ret;
>
> - ret = clk_prepare_enable(xvcu->pll_ref);
> - if (ret) {
> - dev_err(xvcu->dev, "failed to enable pll_ref clock source\n");
> - return ret;
> - }
> -
> xvcu_write_field_reg(base, VCU_PLL_CTRL,
> 1, VCU_PLL_CTRL_BYPASS_MASK,
> VCU_PLL_CTRL_BYPASS_SHIFT);
> @@ -371,9 +405,9 @@ static int xvcu_pll_enable(struct xvcu_device *xvcu)
> vcu_pll_ctrl |= (0 & VCU_PLL_CTRL_RESET_MASK) << VCU_PLL_CTRL_RESET_SHIFT;
> xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
>
> - ret = xvcu_pll_wait_for_lock(xvcu);
> + ret = xvcu_pll_wait_for_lock(pll);
> if (ret) {
> - dev_err(xvcu->dev, "PLL is not locked\n");
> + pr_err("VCU PLL is not locked\n");
> goto err;
> }
>
> @@ -381,15 +415,14 @@ static int xvcu_pll_enable(struct xvcu_device *xvcu)
> 0, VCU_PLL_CTRL_BYPASS_MASK,
> VCU_PLL_CTRL_BYPASS_SHIFT);
>
> - return ret;
> err:
> - clk_disable_unprepare(xvcu->pll_ref);
> return ret;
> }
>
> -static void xvcu_pll_disable(struct xvcu_device *xvcu)
> +static void xvcu_pll_disable(struct clk_hw *hw)
> {
> - void __iomem *base = xvcu->vcu_slcr_ba;
> + struct vcu_pll *pll = to_vcu_pll(hw);
> + void __iomem *base = pll->reg_base;
> u32 vcu_pll_ctrl;
>
> vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
> @@ -400,8 +433,49 @@ static void xvcu_pll_disable(struct xvcu_device *xvcu)
> vcu_pll_ctrl &= ~(VCU_PLL_CTRL_RESET_MASK << VCU_PLL_CTRL_RESET_SHIFT);
> vcu_pll_ctrl |= (1 & VCU_PLL_CTRL_RESET_MASK) << VCU_PLL_CTRL_RESET_SHIFT;
> xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
> +}
> +
> +static const struct clk_ops vcu_pll_ops = {
> + .enable = xvcu_pll_enable,
> + .disable = xvcu_pll_disable,
> + .round_rate = xvcu_pll_round_rate,
> + .recalc_rate = xvcu_pll_recalc_rate,
> + .set_rate = xvcu_pll_set_rate,
> +};
> +
> +static struct clk_hw *xvcu_register_pll(struct device *dev,
> + void __iomem *reg_base,
> + const char *name, const char *parent,
> + unsigned long flags)
> +{
> + struct vcu_pll *pll;
> + struct clk_hw *hw;
> + struct clk_init_data init;
> + int ret;
> +
> + init.name = name;
> + init.parent_names = &parent;
> + init.ops = &vcu_pll_ops;
> + init.num_parents = 1;
> + init.flags = flags;
> +
> + pll = devm_kmalloc(dev, sizeof(*pll), GFP_KERNEL);
> + if (!pll)
> + return ERR_PTR(-ENOMEM);
> +
> + pll->hw.init = &init;
> + pll->reg_base = reg_base;
> + pll->fvco_min = FVCO_MIN;
> + pll->fvco_max = FVCO_MAX;
> +
> + hw = &pll->hw;
> + ret = devm_clk_hw_register(dev, hw);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + clk_hw_set_rate_range(hw, pll->fvco_min, pll->fvco_max);
>
> - clk_disable_unprepare(xvcu->pll_ref);
> + return hw;
> }
>
> /**
> @@ -426,7 +500,6 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
> u32 pll_clk;
> u32 mod;
> int i;
> - int ret;
> const struct xvcu_pll_cfg *found = NULL;
> struct clk_hw *hw;
>
> @@ -486,13 +559,9 @@ 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);
>
> - ret = xvcu_pll_set_rate(xvcu, fvco, refclk);
> - if (ret)
> - return ret;
> -
> - hw = clk_hw_register_fixed_rate(xvcu->dev, "vcu_pll",
> - __clk_get_name(xvcu->pll_ref),
> - 0, fvco);
> + hw = xvcu_register_pll(xvcu->dev, xvcu,
> + "vcu_pll", __clk_get_name(xvcu->pll_ref),
> + CLK_SET_RATE_NO_REPARENT);
You should be passing address not xvcu structure itself.
Reported by sparse.
drivers/soc/xilinx/xlnx_vcu.c:562:43: warning: incorrect type in
argument 2 (different address spaces)
drivers/soc/xilinx/xlnx_vcu.c:562:43: expected void [noderef] __iomem
*reg_base
drivers/soc/xilinx/xlnx_vcu.c:562:43: got struct xvcu_device *xvcu
Thanks,
Michal
