| Message ID | 20201116075532.4019252-1-m.tretter@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | soc: xilinx: vcu: Convert driver to clock provider | expand |
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. 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 Thanks, Michal
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
On 03. 12. 20 10:00, Michael Tretter wrote: > 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. Definitely good idea. We are also waiting for his review for others stuff. Thanks, Michal
Quoting Michael Tretter (2020-11-15 23:55:20) > 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]. After this series is this anything besides a clk provider? If it's only providing clks it would make sense to move the driver into drivers/clk/
On Sat, 12 Dec 2020 21:50:00 -0800, Stephen Boyd wrote: > Quoting Michael Tretter (2020-11-15 23:55:20) > > 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]. > > After this series is this anything besides a clk provider? If it's only > providing clks it would make sense to move the driver into drivers/clk/ > 1. The driver is also responsible for resetting the entire VCU (the VCU_GASKET_INIT register). This isn't something that an individual encoder or decoder driver should be doing. However, other clock drivers also implement a reset controller. 2. There are several registers for AXI performance monitoring in the VCU System-Level Control register space. Right now, these are not used by the driver and I have no plans to actually use them, but this might be an argument against the move. I think it is OK to move the driver to drivers/clk/, but I don't have a strong opinion about it. Michael
Quoting Michael Tretter (2020-12-15 03:56:32) > On Sat, 12 Dec 2020 21:50:00 -0800, Stephen Boyd wrote: > > Quoting Michael Tretter (2020-11-15 23:55:20) > > > 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]. > > > > After this series is this anything besides a clk provider? If it's only > > providing clks it would make sense to move the driver into drivers/clk/ > > > > 1. The driver is also responsible for resetting the entire VCU (the > VCU_GASKET_INIT register). This isn't something that an individual encoder or > decoder driver should be doing. However, other clock drivers also implement a > reset controller. Right. > > 2. There are several registers for AXI performance monitoring in the VCU > System-Level Control register space. Right now, these are not used by the > driver and I have no plans to actually use them, but this might be an argument > against the move. I suppose if/when that happens we can have a small parent driver that probes the compatible string and makes two child platform devices, one for the clk part and one for the PMU? That would let us keep the code in drivers/clk/ for ease of find-ability. This assumes that the PMU registers don't overlap with the clk/reset registers. We usually put the clk and reset controllers together if they use the same registers and need to make sure the frameworks don't stomp on each other. > > I think it is OK to move the driver to drivers/clk/, but I don't have a strong > opinion about it. > Ok. I'm not too strong on it either, but drivers/soc/ is sort of a dumping ground for random soc things. I'm not looking at it closely but if the driver is in drivers/clk/ I'd be more inclined to look after the clk bits.
On Tue, 15 Dec 2020 18:37:20 -0800, Stephen Boyd wrote: > Quoting Michael Tretter (2020-12-15 03:56:32) > > On Sat, 12 Dec 2020 21:50:00 -0800, Stephen Boyd wrote: > > > Quoting Michael Tretter (2020-11-15 23:55:20) > > > > 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]. > > > > > > After this series is this anything besides a clk provider? If it's only > > > providing clks it would make sense to move the driver into drivers/clk/ > > > > > > > 1. The driver is also responsible for resetting the entire VCU (the > > VCU_GASKET_INIT register). This isn't something that an individual encoder or > > decoder driver should be doing. However, other clock drivers also implement a > > reset controller. > > Right. > > > > > 2. There are several registers for AXI performance monitoring in the VCU > > System-Level Control register space. Right now, these are not used by the > > driver and I have no plans to actually use them, but this might be an argument > > against the move. > > I suppose if/when that happens we can have a small parent driver that > probes the compatible string and makes two child platform devices, one > for the clk part and one for the PMU? That would let us keep the code in > drivers/clk/ for ease of find-ability. This assumes that the PMU > registers don't overlap with the clk/reset registers. We usually put the > clk and reset controllers together if they use the same registers and > need to make sure the frameworks don't stomp on each other. > > > > > I think it is OK to move the driver to drivers/clk/, but I don't have a strong > > opinion about it. > > > > Ok. I'm not too strong on it either, but drivers/soc/ is sort of a > dumping ground for random soc things. I'm not looking at it closely but > if the driver is in drivers/clk/ I'd be more inclined to look after the > clk bits. OK, I will move the driver to drivers/clk/xilinx/ Michael