Re: [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider

[Michael Tretter <m.tretter@pengutronix.de>]

On Thu, 03 Dec 2020 08:46:12 +0100, Michal Simek wrote:
> 
> 
> On 16. 11. 20 8:55, Michael Tretter wrote:
> > Hello,
> > 
> > the xlnx_vcu soc driver is actually a clock provider of a PLL and four output
> > clocks created from the PLL via dividers.
> > 
> > This series reworks the xlnx_vcu driver to use the common clock framework to
> > enable other drivers to use the clocks. I originally posted a series to expose
> > the output clocks as fixed clocks [0]. This series now implements the full
> > tree from the PLL to the output clocks. Therefore, I am sending a separate
> > series that focuses on the clocks, but it depends on v4 of the previous series
> > [1].
> > 
> > Possible consumers for the clocks are the allegro-dvt video encoder driver or
> > the Xilinx Video Codec Unit [2] out of tree driver.
> > 
> > Patch 1 defines the identifiers that shall be used by clock consumers in the
> > device tree.
> > 
> > Patch 2 fixes the generic clk-divider to correctly use parents that are passed
> > via struct clk_hw instead of the clock name.
> > 
> > Patches 3-6 refactor the existing driver and split the function to configure
> > the PLL into smaller helper functions.
> > 
> > Patch 7 registers a fixed rate clock for the PLL. The driver calculated and
> > set the PLL configuration during probe, and exposing a fixed rate clock for
> > that rate allows to use the existing configuration with output clocks from the
> > common clock framework.
> > 
> > Patches 8-10 switch the driver to the common clock framework and register the
> > clock provider.
> > 
> > Patches 11-12 are cleanup patches.
> > 
> > Michael
> > 
> > [0] https://lore.kernel.org/linux-arm-kernel/20200619075913.18900-1-m.tretter@pengutronix.de/
> > [1] https://lore.kernel.org/linux-arm-kernel/20201109134818.4159342-3-m.tretter@pengutronix.de/
> > [2] https://github.com/Xilinx/vcu-modules
> > 
> > Michael Tretter (12):
> >   ARM: dts: define indexes for output clocks
> >   clk: divider: fix initialization with parent_hw
> >   soc: xilinx: vcu: drop coreclk from struct xlnx_vcu
> >   soc: xilinx: vcu: add helper to wait for PLL locked
> >   soc: xilinx: vcu: add helpers for configuring PLL
> >   soc: xilinx: vcu: implement PLL disable
> >   soc: xilinx: vcu: register PLL as fixed rate clock
> >   soc: xilinx: vcu: implement clock provider for output clocks
> >   soc: xilinx: vcu: make pll post divider explicit
> >   soc: xilinx: vcu: make the PLL configurable
> >   soc: xilinx: vcu: remove calculation of PLL configuration
> >   soc: xilinx: vcu: use bitfields for register definition
> > 
> >  drivers/clk/clk-divider.c            |   9 +-
> >  drivers/soc/xilinx/Kconfig           |   2 +-
> >  drivers/soc/xilinx/xlnx_vcu.c        | 613 ++++++++++++++++-----------
> >  include/dt-bindings/clock/xlnx-vcu.h |  15 +
> >  4 files changed, 383 insertions(+), 256 deletions(-)
> >  create mode 100644 include/dt-bindings/clock/xlnx-vcu.h
> > 
> 
> I can't see any other problem with this series.

Thanks for the review! I will wait a bit longer if there is some review
feedback by Stephen regarding patch 2, and then send a v2.

> When we are on this. Can you also please fix these issues reported by
> checkpatch to have new issues more visible?
> 
> ./scripts/checkpatch.pl --strict -f drivers/soc/xilinx/xlnx_vcu.c
> CHECK: Alignment should match open parenthesis
> #614: FILE: drivers/soc/xilinx/xlnx_vcu.c:614:
> +	xvcu->vcu_slcr_ba = devm_ioremap(&pdev->dev, res->start,
> +						 resource_size(res));
> 
> WARNING: Possible repeated word: 'the'
> #707: FILE: drivers/soc/xilinx/xlnx_vcu.c:707:
> +	/* Add the the Gasket isolation and put the VCU in reset. */
> 
> total: 0 errors, 1 warnings, 1 checks, 735 lines checked

Sure. I will add a patch to fix the warnings in v2.

Michael