diff mbox series

[v2,3/6] soc: xilinx: vcu: implement clock provider for output clocks

Message ID 20200414103202.4288-4-m.tretter@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series soc: xilinx: vcu: provide interfaces for other drivers | expand

Commit Message

Michael Tretter April 14, 2020, 10:31 a.m. UTC 
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>
---
Changelog:

v1 -> v2:
- unregister registered clocks when removing the driver
---
 drivers/soc/xilinx/Kconfig    |  2 +-
 drivers/soc/xilinx/xlnx_vcu.c | 76 ++++++++++++++++++++++++++++++++++-
 2 files changed, 75 insertions(+), 3 deletions(-)

Comments

kernel test robot April 14, 2020, 6:42 p.m. UTC | #1 
Hi Michael,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on ljones-mfd/for-mfd-next rockchip/for-next keystone/next arm64/for-next/core arm-soc/for-next shawnguo/for-next at91/at91-next v5.7-rc1 next-20200414]
[cannot apply to xlnx/master linux-rpi/for-rpi-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Michael-Tretter/soc-xilinx-vcu-provide-interfaces-for-other-drivers/20200414-235029
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: x86_64-randconfig-s1-20200414 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/soc/xilinx/xlnx_vcu.c: In function 'xvcu_reset':
>> drivers/soc/xilinx/xlnx_vcu.c:540:11: error: 'struct xvcu_device' has no member named 'reset_gpio'
     if (!xvcu->reset_gpio)
              ^~
>> drivers/soc/xilinx/xlnx_vcu.c:543:2: error: implicit declaration of function 'gpiod_set_value'; did you mean 'bitmap_set_value8'? [-Werror=implicit-function-declaration]
     gpiod_set_value(xvcu->reset_gpio, 0);
     ^~~~~~~~~~~~~~~
     bitmap_set_value8
   drivers/soc/xilinx/xlnx_vcu.c:543:22: error: 'struct xvcu_device' has no member named 'reset_gpio'
     gpiod_set_value(xvcu->reset_gpio, 0);
                         ^~
   drivers/soc/xilinx/xlnx_vcu.c:545:2: error: implicit declaration of function 'usleep_range'; did you mean 'sort_range'? [-Werror=implicit-function-declaration]
     usleep_range(60, 120);
     ^~~~~~~~~~~~
     sort_range
   drivers/soc/xilinx/xlnx_vcu.c:546:22: error: 'struct xvcu_device' has no member named 'reset_gpio'
     gpiod_set_value(xvcu->reset_gpio, 1);
                         ^~
   At top level:
   drivers/soc/xilinx/xlnx_vcu.c:538:13: warning: 'xvcu_reset' defined but not used [-Wunused-function]
    static void xvcu_reset(struct xvcu_device *xvcu)
                ^~~~~~~~~~
   cc1: some warnings being treated as errors

vim +540 drivers/soc/xilinx/xlnx_vcu.c

   537	
   538	static void xvcu_reset(struct xvcu_device *xvcu)
   539	{
 > 540		if (!xvcu->reset_gpio)
   541			return;
   542	
 > 543		gpiod_set_value(xvcu->reset_gpio, 0);
   544		/* min 2 clock cycle of vcu pll_ref, slowest freq is 33.33KHz */
   545		usleep_range(60, 120);
   546		gpiod_set_value(xvcu->reset_gpio, 1);
   547		usleep_range(60, 120);
   548	}
   549	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot April 14, 2020, 7:48 p.m. UTC | #2 
Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on ljones-mfd/for-mfd-next rockchip/for-next keystone/next arm64/for-next/core arm-soc/for-next shawnguo/for-next v5.7-rc1 next-20200414]
[cannot apply to xlnx/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Michael-Tretter/soc-xilinx-vcu-provide-interfaces-for-other-drivers/20200414-235029
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: x86_64-randconfig-a002-20200414 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/err.h:5:0,
                    from include/linux/clk.h:12,
                    from drivers/soc/xilinx/xlnx_vcu.c:9:
   drivers/soc/xilinx/xlnx_vcu.c: In function 'xvcu_reset':
   drivers/soc/xilinx/xlnx_vcu.c:540:11: error: 'struct xvcu_device' has no member named 'reset_gpio'
     if (!xvcu->reset_gpio)
              ^
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^~~~
>> drivers/soc/xilinx/xlnx_vcu.c:540:2: note: in expansion of macro 'if'
     if (!xvcu->reset_gpio)
     ^~
   drivers/soc/xilinx/xlnx_vcu.c:540:11: error: 'struct xvcu_device' has no member named 'reset_gpio'
     if (!xvcu->reset_gpio)
              ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^~~~
>> drivers/soc/xilinx/xlnx_vcu.c:540:2: note: in expansion of macro 'if'
     if (!xvcu->reset_gpio)
     ^~
   drivers/soc/xilinx/xlnx_vcu.c:540:11: error: 'struct xvcu_device' has no member named 'reset_gpio'
     if (!xvcu->reset_gpio)
              ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^~~~
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^~~~~~~~~~~~~~
>> drivers/soc/xilinx/xlnx_vcu.c:540:2: note: in expansion of macro 'if'
     if (!xvcu->reset_gpio)
     ^~
   drivers/soc/xilinx/xlnx_vcu.c:543:2: error: implicit declaration of function 'gpiod_set_value'; did you mean 'bitmap_set_value8'? [-Werror=implicit-function-declaration]
     gpiod_set_value(xvcu->reset_gpio, 0);
     ^~~~~~~~~~~~~~~
     bitmap_set_value8
   drivers/soc/xilinx/xlnx_vcu.c:543:22: error: 'struct xvcu_device' has no member named 'reset_gpio'
     gpiod_set_value(xvcu->reset_gpio, 0);
                         ^~
   drivers/soc/xilinx/xlnx_vcu.c:545:2: error: implicit declaration of function 'usleep_range'; did you mean 'sort_range'? [-Werror=implicit-function-declaration]
     usleep_range(60, 120);
     ^~~~~~~~~~~~
     sort_range
   drivers/soc/xilinx/xlnx_vcu.c:546:22: error: 'struct xvcu_device' has no member named 'reset_gpio'
     gpiod_set_value(xvcu->reset_gpio, 1);
                         ^~
   At top level:
   drivers/soc/xilinx/xlnx_vcu.c:538:13: warning: 'xvcu_reset' defined but not used [-Wunused-function]
    static void xvcu_reset(struct xvcu_device *xvcu)
                ^~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/if +540 drivers/soc/xilinx/xlnx_vcu.c

   537	
   538	static void xvcu_reset(struct xvcu_device *xvcu)
   539	{
 > 540		if (!xvcu->reset_gpio)
   541			return;
   542	
   543		gpiod_set_value(xvcu->reset_gpio, 0);
   544		/* min 2 clock cycle of vcu pll_ref, slowest freq is 33.33KHz */
   545		usleep_range(60, 120);
   546		gpiod_set_value(xvcu->reset_gpio, 1);
   547		usleep_range(60, 120);
   548	}
   549	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Michael Tretter April 15, 2020, 6:26 a.m. UTC | #3 
On Tue, Apr 14, 2020 at 12:31:59PM +0200, 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>
> ---
> Changelog:
> 
> v1 -> v2:
> - unregister registered clocks when removing the driver
> ---
>  drivers/soc/xilinx/Kconfig    |  2 +-
>  drivers/soc/xilinx/xlnx_vcu.c | 76 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 75 insertions(+), 3 deletions(-)
> 
[...]
> diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
> index dcd8e7824b06..f07a1361a2a8 100644
> --- a/drivers/soc/xilinx/xlnx_vcu.c
> +++ b/drivers/soc/xilinx/xlnx_vcu.c
[...]
> @@ -485,6 +490,63 @@ static int xvcu_set_pll(struct xvcu_device *xvcu)
>  	return -ETIMEDOUT;
>  }
>  
> +static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
> +{
> +	struct device *dev = xvcu->dev;
> +	const char *parent_name = __clk_get_name(xvcu->pll_ref);
> +	struct clk_onecell_data *data = &xvcu->clk_data;
> +	struct clk **clks;
> +	size_t num_clks = CLK_XVCU_MAX;
> +
> +	clks = devm_kcalloc(dev, num_clks, sizeof(*clks), GFP_KERNEL);
> +	if (!clks)
> +		return -ENOMEM;
> +
> +	data->clk_num = num_clks;
> +	data->clks = clks;
> +
> +	clks[CLK_XVCU_ENC_CORE] =
> +		clk_register_fixed_rate(dev, "venc_core_clk",
> +					parent_name, 0, xvcu->coreclk);
> +	clks[CLK_XVCU_ENC_MCU] =
> +		clk_register_fixed_rate(dev, "venc_mcu_clk",
> +					parent_name, 0, xvcu->mcuclk);
> +	clks[CLK_XVCU_DEC_CORE] =
> +		clk_register_fixed_rate(dev, "vdec_core_clk",
> +					parent_name, 0, xvcu->coreclk);
> +	clks[CLK_XVCU_DEC_MCU] =
> +		clk_register_fixed_rate(dev, "vdec_mcu_clk",
> +					parent_name, 0, xvcu->mcuclk);
> +
> +	return of_clk_add_provider(dev->of_node, of_clk_src_onecell_get, data);
> +}
> +
> +static void xvcu_unregister_clock_provider(struct xvcu_device *xvcu)
> +{
> +	struct device *dev = xvcu->dev;
> +	struct clk_onecell_data *data = &xvcu->clk_data;
> +	struct clk **clks = data->clks;
> +
> +	of_clk_del_provider(dev->of_node);
> +
> +	clk_unregister_fixed_rate(clks[CLK_XVCU_DEC_MCU]);
> +	clk_unregister_fixed_rate(clks[CLK_XVCU_DEC_CORE]);
> +	clk_unregister_fixed_rate(clks[CLK_XVCU_ENC_MCU]);
> +	clk_unregister_fixed_rate(clks[CLK_XVCU_ENC_CORE]);
> +}
> +
> +static void xvcu_reset(struct xvcu_device *xvcu)
> +{
> +	if (!xvcu->reset_gpio)
> +		return;
> +
> +	gpiod_set_value(xvcu->reset_gpio, 0);
> +	/* min 2 clock cycle of vcu pll_ref, slowest freq is 33.33KHz */
> +	usleep_range(60, 120);
> +	gpiod_set_value(xvcu->reset_gpio, 1);
> +	usleep_range(60, 120);
> +}
> +

The xvcu_reset function shouldn't have been there at all. Looks like it
slipped through, when I rebased the patches. I'll send a v3.

Michael

>  /**
>   * xvcu_probe - Probe existence of the logicoreIP
>   *			and initialize PLL
> @@ -569,10 +631,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->pll_ref);
>  error_aclk:
> @@ -596,6 +666,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. */
>  	xvcu_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, 0);
>  
> -- 
> 2.20.1
> 
>
diff mbox series

Patch

diff --git a/drivers/soc/xilinx/Kconfig b/drivers/soc/xilinx/Kconfig
index 223f1f9d0922..331f124902e8 100644
--- a/drivers/soc/xilinx/Kconfig
+++ b/drivers/soc/xilinx/Kconfig
@@ -3,7 +3,7 @@  menu "Xilinx SoC drivers"
 
 config XILINX_VCU
 	tristate "Xilinx VCU logicoreIP Init"
-	depends on HAS_IOMEM
+	depends on HAS_IOMEM && COMMON_CLK
 	help
 	  Provides the driver to enable and disable the isolation between the
 	  processing system and programmable logic part by using the logicoreIP
diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
index dcd8e7824b06..f07a1361a2a8 100644
--- a/drivers/soc/xilinx/xlnx_vcu.c
+++ b/drivers/soc/xilinx/xlnx_vcu.c
@@ -7,6 +7,7 @@ 
  * Contacts   Dhaval Shah <dshah@xilinx.com>
  */
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/io.h>
@@ -14,6 +15,8 @@ 
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 
+#include <dt-bindings/clock/xlnx-vcu.h>
+
 /* Address map for different registers implemented in the VCU LogiCORE IP. */
 #define VCU_ECODER_ENABLE		0x00
 #define VCU_DECODER_ENABLE		0x04
@@ -108,7 +111,9 @@  struct xvcu_device {
 	struct clk *aclk;
 	void __iomem *logicore_reg_ba;
 	void __iomem *vcu_slcr_ba;
+	struct clk_onecell_data clk_data;
 	u32 coreclk;
+	u32 mcuclk;
 };
 
 /**
@@ -375,10 +380,10 @@  static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
 	}
 
 	xvcu->coreclk = pll_clk / divisor_core;
-	mcuclk = pll_clk / divisor_mcu;
+	xvcu->mcuclk = pll_clk / divisor_mcu;
 	dev_dbg(xvcu->dev, "Actual Ref clock freq is %uHz\n", refclk);
 	dev_dbg(xvcu->dev, "Actual Core clock freq is %uHz\n", xvcu->coreclk);
-	dev_dbg(xvcu->dev, "Actual Mcu clock freq is %uHz\n", mcuclk);
+	dev_dbg(xvcu->dev, "Actual Mcu clock freq is %uHz\n", xvcu->mcuclk);
 
 	vcu_pll_ctrl &= ~(VCU_PLL_CTRL_FBDIV_MASK << VCU_PLL_CTRL_FBDIV_SHIFT);
 	vcu_pll_ctrl |= (found->fbdiv & VCU_PLL_CTRL_FBDIV_MASK) <<
@@ -485,6 +490,63 @@  static int xvcu_set_pll(struct xvcu_device *xvcu)
 	return -ETIMEDOUT;
 }
 
+static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
+{
+	struct device *dev = xvcu->dev;
+	const char *parent_name = __clk_get_name(xvcu->pll_ref);
+	struct clk_onecell_data *data = &xvcu->clk_data;
+	struct clk **clks;
+	size_t num_clks = CLK_XVCU_MAX;
+
+	clks = devm_kcalloc(dev, num_clks, sizeof(*clks), GFP_KERNEL);
+	if (!clks)
+		return -ENOMEM;
+
+	data->clk_num = num_clks;
+	data->clks = clks;
+
+	clks[CLK_XVCU_ENC_CORE] =
+		clk_register_fixed_rate(dev, "venc_core_clk",
+					parent_name, 0, xvcu->coreclk);
+	clks[CLK_XVCU_ENC_MCU] =
+		clk_register_fixed_rate(dev, "venc_mcu_clk",
+					parent_name, 0, xvcu->mcuclk);
+	clks[CLK_XVCU_DEC_CORE] =
+		clk_register_fixed_rate(dev, "vdec_core_clk",
+					parent_name, 0, xvcu->coreclk);
+	clks[CLK_XVCU_DEC_MCU] =
+		clk_register_fixed_rate(dev, "vdec_mcu_clk",
+					parent_name, 0, xvcu->mcuclk);
+
+	return of_clk_add_provider(dev->of_node, of_clk_src_onecell_get, data);
+}
+
+static void xvcu_unregister_clock_provider(struct xvcu_device *xvcu)
+{
+	struct device *dev = xvcu->dev;
+	struct clk_onecell_data *data = &xvcu->clk_data;
+	struct clk **clks = data->clks;
+
+	of_clk_del_provider(dev->of_node);
+
+	clk_unregister_fixed_rate(clks[CLK_XVCU_DEC_MCU]);
+	clk_unregister_fixed_rate(clks[CLK_XVCU_DEC_CORE]);
+	clk_unregister_fixed_rate(clks[CLK_XVCU_ENC_MCU]);
+	clk_unregister_fixed_rate(clks[CLK_XVCU_ENC_CORE]);
+}
+
+static void xvcu_reset(struct xvcu_device *xvcu)
+{
+	if (!xvcu->reset_gpio)
+		return;
+
+	gpiod_set_value(xvcu->reset_gpio, 0);
+	/* min 2 clock cycle of vcu pll_ref, slowest freq is 33.33KHz */
+	usleep_range(60, 120);
+	gpiod_set_value(xvcu->reset_gpio, 1);
+	usleep_range(60, 120);
+}
+
 /**
  * xvcu_probe - Probe existence of the logicoreIP
  *			and initialize PLL
@@ -569,10 +631,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->pll_ref);
 error_aclk:
@@ -596,6 +666,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. */
 	xvcu_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, 0);