[RESEND PATCH v3 3/6] soc: xilinx: vcu: implement clock provider for output clocks
Michael Tretter
m.tretter at pengutronix.de
Fri Jul 24 03:28:27 EDT 2020
Hi Rajan,
On Fri, 24 Jul 2020 06:44:22 +0000, Rajan Vaja wrote:
> > -----Original Message-----
> > From: Michael Tretter <m.tretter at pengutronix.de>
> > Sent: 22 July 2020 08:22 PM
> > To: Rajan Vaja <RAJANV at xilinx.com>
> > Cc: linux-arm-kernel at lists.infradead.org; Rohit Visavalia <RVISAVAL at xilinx.com>;
> > Michal Simek <michals at xilinx.com>; Dhaval Rajeshbhai Shah
> > <dshah at xilinx.com>; Greg Kroah-Hartman <gregkh at linuxfoundation.org>;
> > kernel at 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 at pengutronix.de>
> > > > Sent: 19 June 2020 01:29 PM
> > > > To: linux-arm-kernel at lists.infradead.org
> > > > Cc: Rohit Visavalia <RVISAVAL at xilinx.com>; Michal Simek
> > <michals at xilinx.com>;
> > > > Dhaval Rajeshbhai Shah <dshah at xilinx.com>; Greg Kroah-Hartman
> > > > <gregkh at linuxfoundation.org>; kernel at pengutronix.de; Michael Tretter
> > > > <m.tretter at 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 at 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 at 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
More information about the linux-arm-kernel
mailing list