| Message ID | 20200619075913.18900-4-m.tretter@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: xilinx: vcu: provide interfaces for other drivers | expand |
Hi Michael, Thanks for the patch. -----Original Message----- > From: Michael Tretter <m.tretter@pengutronix.de> > Sent: 19 June 2020 01:29 PM > To: linux-arm-kernel@lists.infradead.org > Cc: Rohit Visavalia <RVISAVAL@xilinx.com>; Michal Simek <michals@xilinx.com>; > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; kernel@pengutronix.de; Michael Tretter > <m.tretter@pengutronix.de> > Subject: [RESEND PATCH v3 3/6] soc: xilinx: vcu: implement clock provider for > output clocks > > CAUTION: This message has originated from an External Source. Please use proper > judgment and caution when opening attachments, clicking links, or responding to > this email. > > > 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/Kconfig | 2 +- > drivers/soc/xilinx/xlnx_vcu.c | 64 +++++++++++++++++++++++++++++++++-- > 2 files changed, 63 insertions(+), 3 deletions(-) > > diff --git a/drivers/soc/xilinx/Kconfig b/drivers/soc/xilinx/Kconfig > index 646512d7276f..2a4e80a36f78 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..03bf252737aa 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,51 @@ 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); > + [Rajan] These clocks are not fixed rate clock. These clocks are mux-->div-->gate clock. Can we separate out clock provider as a different kernel modules instead of having it in VCU driver it self? We have already implemented clock provider https://github.com/Xilinx/linux-xlnx/blob/master/drivers/soc/xilinx/xlnx_vcu_clk.c and removed manual rate calculation as separate kernel module. You can refer it, and see if it works for you. Thanks, Rajan > + 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]); > +} > + > /** > * xvcu_probe - Probe existence of the logicoreIP > * and initialize PLL > @@ -569,10 +619,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 +654,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
Hi Rajan, On Tue, 21 Jul 2020 06:59:47 +0000, Rajan Vaja wrote: > Hi Michael, > > Thanks for the patch. > > -----Original Message----- > > From: Michael Tretter <m.tretter@pengutronix.de> > > Sent: 19 June 2020 01:29 PM > > To: linux-arm-kernel@lists.infradead.org > > Cc: Rohit Visavalia <RVISAVAL@xilinx.com>; Michal Simek <michals@xilinx.com>; > > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Greg Kroah-Hartman > > <gregkh@linuxfoundation.org>; kernel@pengutronix.de; Michael Tretter > > <m.tretter@pengutronix.de> > > Subject: [RESEND PATCH v3 3/6] soc: xilinx: vcu: implement clock provider for > > output clocks > > > > CAUTION: This message has originated from an External Source. Please use proper > > judgment and caution when opening attachments, clicking links, or responding to > > this email. > > > > > > 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/Kconfig | 2 +- > > drivers/soc/xilinx/xlnx_vcu.c | 64 +++++++++++++++++++++++++++++++++-- > > 2 files changed, 63 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/soc/xilinx/Kconfig b/drivers/soc/xilinx/Kconfig > > index 646512d7276f..2a4e80a36f78 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..03bf252737aa 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,51 @@ 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); > > + > [Rajan] These clocks are not fixed rate clock. These clocks are mux-->div-->gate clock. I agree that the mux->div->gate clock structure should be properly implemented, but for clock consumers it doesn't matter, if the clocks are fixed rate or mux-->div-->rate if this driver already sets the expected rate. The expected rate is already read from the logicore registers. If properly implemented, this boils down to an implementation detail of the VCU driver. > Can we separate out clock provider as a different kernel modules instead of having it in VCU driver it self? I am not sure, if separating the clock driver as a different kernel module is actually useful. With a separated clock driver, the only things left in this driver are the configuration of the gasket isolation and the reset GPIO. These just don't justify a separate driver. > > We have already implemented clock provider https://github.com/Xilinx/linux-xlnx/blob/master/drivers/soc/xilinx/xlnx_vcu_clk.c and removed manual rate calculation > as separate kernel module. You can refer it, and see if it works for you. What I am missing from the downstream driver is the ability to reference the clocks from other drivers via device tree binding (i.e. the CLK_XVCU_ENC_CORE, CLK_XVCU_ENC_MCU, CLK_XVCU_DEC_CORE, CLK_XVCU_DEC_MCU defines). The codec driver needs to enable the clocks and, therefore, somehow needs to find the clocks. As I fail to see, why this would require an MFD driver and a separate kernel module for the clocks, I would prefer to implement the full clock driver (including the mux-->div-->gate clock) in the VCU driver and export the clocks as suggested by this series. Michael > > Thanks, > Rajan > > + 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]); > > +} > > + > > /** > > * xvcu_probe - Probe existence of the logicoreIP > > * and initialize PLL > > @@ -569,10 +619,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 +654,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 > >
Hi Michael, > -----Original Message----- > From: Michael Tretter <m.tretter@pengutronix.de> > Sent: 22 July 2020 08:22 PM > To: Rajan Vaja <RAJANV@xilinx.com> > Cc: linux-arm-kernel@lists.infradead.org; Rohit Visavalia <RVISAVAL@xilinx.com>; > Michal Simek <michals@xilinx.com>; Dhaval Rajeshbhai Shah > <dshah@xilinx.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; > kernel@pengutronix.de > Subject: Re: [RESEND PATCH v3 3/6] soc: xilinx: vcu: implement clock provider for > output clocks > > CAUTION: This message has originated from an External Source. Please use proper > judgment and caution when opening attachments, clicking links, or responding to > this email. > > > Hi Rajan, > > On Tue, 21 Jul 2020 06:59:47 +0000, Rajan Vaja wrote: > > Hi Michael, > > > > Thanks for the patch. > > > > -----Original Message----- > > > From: Michael Tretter <m.tretter@pengutronix.de> > > > Sent: 19 June 2020 01:29 PM > > > To: linux-arm-kernel@lists.infradead.org > > > Cc: Rohit Visavalia <RVISAVAL@xilinx.com>; Michal Simek > <michals@xilinx.com>; > > > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org>; kernel@pengutronix.de; Michael Tretter > > > <m.tretter@pengutronix.de> > > > Subject: [RESEND PATCH v3 3/6] soc: xilinx: vcu: implement clock provider for > > > output clocks > > > > > > CAUTION: This message has originated from an External Source. Please use > proper > > > judgment and caution when opening attachments, clicking links, or responding > to > > > this email. > > > > > > > > > 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/Kconfig | 2 +- > > > drivers/soc/xilinx/xlnx_vcu.c | 64 +++++++++++++++++++++++++++++++++-- > > > 2 files changed, 63 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/soc/xilinx/Kconfig b/drivers/soc/xilinx/Kconfig > > > index 646512d7276f..2a4e80a36f78 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..03bf252737aa 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,51 @@ 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); > > > + > > [Rajan] These clocks are not fixed rate clock. These clocks are mux-->div-->gate > clock. > > I agree that the mux->div->gate clock structure should be properly > implemented, but for clock consumers it doesn't matter, if the clocks are > fixed rate or mux-->div-->rate if this driver already sets the expected rate. > The expected rate is already read from the logicore registers. If properly > implemented, this boils down to an implementation detail of the VCU driver. > > > Can we separate out clock provider as a different kernel modules instead of > having it in VCU driver it self? > > I am not sure, if separating the clock driver as a different kernel module is > actually useful. With a separated clock driver, the only things left in this > driver are the configuration of the gasket isolation and the reset GPIO. > These just don't justify a separate driver. > > > > > We have already implemented clock provider https://github.com/Xilinx/linux- > xlnx/blob/master/drivers/soc/xilinx/xlnx_vcu_clk.c and removed manual rate > calculation > > as separate kernel module. You can refer it, and see if it works for you. > > What I am missing from the downstream driver is the ability to reference the > clocks from other drivers via device tree binding (i.e. the CLK_XVCU_ENC_CORE, > CLK_XVCU_ENC_MCU, CLK_XVCU_DEC_CORE, CLK_XVCU_DEC_MCU defines). The > codec > driver needs to enable the clocks and, therefore, somehow needs to find the > clocks. > > As I fail to see, why this would require an MFD driver and a separate kernel > module for the clocks, I would prefer to implement the full clock driver > (including the mux-->div-->gate clock) in the VCU driver and export the clocks > as suggested by this series. [Rajan] Reason we choose to go with MFD driver, as existing driver VCU will be clock consumer, So, we thought to separate out provider and consume. Let us know if you think otherwise. Thanks, Rajan > > Michael > > > > > Thanks, > > Rajan > > > + 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]); > > > +} > > > + > > > /** > > > * xvcu_probe - Probe existence of the logicoreIP > > > * and initialize PLL > > > @@ -569,10 +619,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 +654,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 > > > >
Hi Rajan, On Fri, 24 Jul 2020 06:44:22 +0000, Rajan Vaja wrote: > > -----Original Message----- > > From: Michael Tretter <m.tretter@pengutronix.de> > > Sent: 22 July 2020 08:22 PM > > To: Rajan Vaja <RAJANV@xilinx.com> > > Cc: linux-arm-kernel@lists.infradead.org; Rohit Visavalia <RVISAVAL@xilinx.com>; > > Michal Simek <michals@xilinx.com>; Dhaval Rajeshbhai Shah > > <dshah@xilinx.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; > > kernel@pengutronix.de > > Subject: Re: [RESEND PATCH v3 3/6] soc: xilinx: vcu: implement clock provider for > > output clocks > > > > CAUTION: This message has originated from an External Source. Please use proper > > judgment and caution when opening attachments, clicking links, or responding to > > this email. > > > > > > Hi Rajan, > > > > On Tue, 21 Jul 2020 06:59:47 +0000, Rajan Vaja wrote: > > > Hi Michael, > > > > > > Thanks for the patch. > > > > > > -----Original Message----- > > > > From: Michael Tretter <m.tretter@pengutronix.de> > > > > Sent: 19 June 2020 01:29 PM > > > > To: linux-arm-kernel@lists.infradead.org > > > > Cc: Rohit Visavalia <RVISAVAL@xilinx.com>; Michal Simek > > <michals@xilinx.com>; > > > > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org>; kernel@pengutronix.de; Michael Tretter > > > > <m.tretter@pengutronix.de> > > > > Subject: [RESEND PATCH v3 3/6] soc: xilinx: vcu: implement clock provider for > > > > output clocks > > > > > > > > CAUTION: This message has originated from an External Source. Please use > > proper > > > > judgment and caution when opening attachments, clicking links, or responding > > to > > > > this email. > > > > > > > > > > > > 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/Kconfig | 2 +- > > > > drivers/soc/xilinx/xlnx_vcu.c | 64 +++++++++++++++++++++++++++++++++-- > > > > 2 files changed, 63 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/soc/xilinx/Kconfig b/drivers/soc/xilinx/Kconfig > > > > index 646512d7276f..2a4e80a36f78 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..03bf252737aa 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,51 @@ 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); > > > > + > > > [Rajan] These clocks are not fixed rate clock. These clocks are mux-->div-->gate > > clock. > > > > I agree that the mux->div->gate clock structure should be properly > > implemented, but for clock consumers it doesn't matter, if the clocks are > > fixed rate or mux-->div-->rate if this driver already sets the expected rate. > > The expected rate is already read from the logicore registers. If properly > > implemented, this boils down to an implementation detail of the VCU driver. > > > > > Can we separate out clock provider as a different kernel modules instead of > > having it in VCU driver it self? > > > > I am not sure, if separating the clock driver as a different kernel module is > > actually useful. With a separated clock driver, the only things left in this > > driver are the configuration of the gasket isolation and the reset GPIO. > > These just don't justify a separate driver. > > > > > > > > We have already implemented clock provider https://github.com/Xilinx/linux- > > xlnx/blob/master/drivers/soc/xilinx/xlnx_vcu_clk.c and removed manual rate > > calculation > > > as separate kernel module. You can refer it, and see if it works for you. > > > > What I am missing from the downstream driver is the ability to reference the > > clocks from other drivers via device tree binding (i.e. the CLK_XVCU_ENC_CORE, > > CLK_XVCU_ENC_MCU, CLK_XVCU_DEC_CORE, CLK_XVCU_DEC_MCU defines). The > > codec > > driver needs to enable the clocks and, therefore, somehow needs to find the > > clocks. > > > > As I fail to see, why this would require an MFD driver and a separate kernel > > module for the clocks, I would prefer to implement the full clock driver > > (including the mux-->div-->gate clock) in the VCU driver and export the clocks > > as suggested by this series. > > [Rajan] Reason we choose to go with MFD driver, as existing driver VCU will be clock consumer, > So, we thought to separate out provider and consume. Let us know if you think otherwise. The xlnx_vcu should only be the provider of the clocks. Please check out the device tree bindings in Documentation/devicetree/bindings/media/allegro.txt. That binding describes the actual encoder and decoder cores. It defines the five clocks that are documented as input clocks to the encoder and decoder blocks in PG252 H.264/H.265 Video Codec Unit v1.2, Figure 8-1. Therefore, the driver for the encoder/decoder blocks should be the consumer of the clocks that are provided by the xlnx_vcu driver. Currently, this would be the allegro driver (drivers/staging/media/allegro-dvt). Once the xlnx_vcu driver is a proper clock provider, the allegro driver will be able to get the venc_core_clk and venc_mcu_clk via the device tree binding and enable and set the rate of these clocks. As your out-of-tree VCU driver is also a driver for the encoder/decoder blocks, you would add the code to enable and set the clock rate there, too. BTW: The allegro driver is also the reason for the vcu-settings bindings that are part of the series. The vcu-settings are the replacement for the xvcu_get_color_depth, xvcu_get_memory_depth, and xvcu_get_num_cores functions defined downstream in the xlnx_vcu driver. Thanks, Michael
diff --git a/drivers/soc/xilinx/Kconfig b/drivers/soc/xilinx/Kconfig index 646512d7276f..2a4e80a36f78 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..03bf252737aa 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,51 @@ 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]); +} + /** * xvcu_probe - Probe existence of the logicoreIP * and initialize PLL @@ -569,10 +619,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 +654,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);
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/Kconfig | 2 +- drivers/soc/xilinx/xlnx_vcu.c | 64 +++++++++++++++++++++++++++++++++-- 2 files changed, 63 insertions(+), 3 deletions(-)