[U-Boot] [PATCH v2 06/11] clk: Add SiFive FU540 PRCI clock driver

[Auer, Lukas]

On Mon, 2019-01-21 at 05:55 +0000, Anup Patel wrote:
> > -----Original Message-----
> > From: Auer, Lukas [mailto:lukas.auer at aisec.fraunhofer.de]
> > Sent: Monday, January 21, 2019 1:43 AM
> > To: sjg at chromium.org; bmeng.cn at gmail.com; rick at andestech.com; Anup
> > Patel <Anup.Patel at wdc.com>; joe.hershberger at ni.com;
> > yamada.masahiro at socionext.com
> > Cc: paul.walmsley at sifive.com; palmer at sifive.com; hch at infradead.org;
> > u-
> > boot at lists.denx.de; agraf at suse.de; Atish Patra <Atish.Patra at wdc.com
> > >
> > Subject: Re: [PATCH v2 06/11] clk: Add SiFive FU540 PRCI clock
> > driver
> > 
> > Hi Anup,
> > 
> > On Fri, 2019-01-18 at 11:19 +0000, Anup Patel wrote:
> > > Add driver code for the SiFive FU540 PRCI IP block.  This IP
> > > block
> > > handles reset and clock control for the SiFive FU540 device and
> > > implements SoC-level clock tree controls and dividers.
> > > 
> > > Based on code written by Wesley Terpstra <wesley at sifive.com>
> > > found in
> > > commit 999529edf517ed75b56659d456d221b2ee56bb60 of:
> > > https://github.com/riscv/riscv-linux
> > > 
> > > Boot and PLL rate change were tested on a SiFive HiFive Unleashed
> > > board.
> > > 
> > > Signed-off-by: Paul Walmsley <paul.walmsley at sifive.com>
> > > Signed-off-by: Atish Patra <atish.patra at wdc.com>
> > > Signed-off-by: Anup Patel <anup.patel at wdc.com>
> > > Reviewed-by: Alexander Graf <agraf at suse.de>
> > > ---
> > >  drivers/clk/Kconfig                           |   1 +
> > >  drivers/clk/Makefile                          |   1 +
> > >  drivers/clk/sifive/Kconfig                    |  19 +
> > >  drivers/clk/sifive/Makefile                   |   5 +
> > >  .../clk/sifive/analogbits-wrpll-cln28hpc.h    | 101 +++
> > >  drivers/clk/sifive/fu540-prci.c               | 604
> > > ++++++++++++++++++
> > >  drivers/clk/sifive/wrpll-cln28hpc.c           | 390 +++++++++++
> > >  include/dt-bindings/clk/sifive-fu540-prci.h   |  29 +
> > >  8 files changed, 1150 insertions(+)
> > >  create mode 100644 drivers/clk/sifive/Kconfig  create mode
> > > 100644
> > > drivers/clk/sifive/Makefile  create mode 100644
> > > drivers/clk/sifive/analogbits-wrpll-cln28hpc.h
> > >  create mode 100644 drivers/clk/sifive/fu540-prci.c  create mode
> > > 100644 drivers/clk/sifive/wrpll-cln28hpc.c
> > >  create mode 100644 include/dt-bindings/clk/sifive-fu540-prci.h
> > > 
> > 
> > The corresponding patch series for the Linux Kernel [1] includes
> > code
> > comments, which have not been addressed yet in this version of the
> > driver.
> > Are you planning on syncing it with the Linux driver once it is
> > upstream, or
> > incorporating the comments directly?
> > 
> > [1]:
> > https://patchwork.kernel.org/project/linux-
> > clk/list/?series=33383&state=%2A&archive=both
> 
> Yes, the Linux patches might take some time to merge so unblock
> this patch series we took the first version as reference. Later once
> it is merged we will send one sync-up patch to U-Boot.

That sounds good, thanks!

> 
> > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index
> > > eadf7f8250..ce462f5717 100644
> > > --- a/drivers/clk/Kconfig
> > > +++ b/drivers/clk/Kconfig
> > > @@ -104,6 +104,7 @@ source "drivers/clk/imx/Kconfig"
> > >  source "drivers/clk/mvebu/Kconfig"
> > >  source "drivers/clk/owl/Kconfig"
> > >  source "drivers/clk/renesas/Kconfig"
> > > +source "drivers/clk/sifive/Kconfig"
> > >  source "drivers/clk/tegra/Kconfig"
> > >  source "drivers/clk/uniphier/Kconfig"
> > > 
> > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index
> > > 9acbb1a650..2f4446568c 100644
> > > --- a/drivers/clk/Makefile
> > > +++ b/drivers/clk/Makefile
> > > @@ -22,6 +22,7 @@ obj-$(CONFIG_CLK_HSDK) += clk-hsdk-cgu.o
> > >  obj-$(CONFIG_CLK_MPC83XX) += mpc83xx_clk.o
> > >  obj-$(CONFIG_CLK_OWL) += owl/
> > >  obj-$(CONFIG_CLK_RENESAS) += renesas/
> > > +obj-$(CONFIG_CLK_SIFIVE) += sifive/
> > >  obj-$(CONFIG_CLK_STM32F) += clk_stm32f.o
> > >  obj-$(CONFIG_CLK_STM32MP1) += clk_stm32mp1.o
> > >  obj-$(CONFIG_CLK_UNIPHIER) += uniphier/ diff --git
> > > a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig new
> > > file
> > > mode 100644 index 0000000000..81fc9f8fda
> > > --- /dev/null
> > > +++ b/drivers/clk/sifive/Kconfig
> > > @@ -0,0 +1,19 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +
> > > +config CLK_ANALOGBITS_WRPLL_CLN28HPC
> > > +	bool
> > 
> > The analogbits library is not specific to SiFive. The Linux Kernel
> > patch series
> > adds it as a standalone entry to make it available to other
> > drivers. I think the
> > same thing would also make sense here.
> 
> Based on other CLK drivers, it seems analogbits library is not
> generic enough
> might require more work to become generic PLL library.
> 
> To unblock this patch series, we have made it SiFive specific. Later
> if it is
> accepted in Linux as standalone library then we will refactor here as
> well.
> 
> IMHO, analogbits library is not generic enough so might be difficult
> to
> take it as standalone library in Linux.
> 

That makes sense. I agree, the best option will be to include the
library the same way as in Linux.

Thanks,
Lukas