| Message ID | 20210910184147.336618-2-paul.kocialkowski@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/22] clk: sunxi-ng: v3s: Make the ISP PLL clock public | expand |
On Fri, Sep 10, 2021 at 08:41:26PM +0200, Paul Kocialkowski wrote: > In order to reparent the CSI module clock to the ISP PLL via > device-tree, export the ISP PLL clock declaration in the public > device-tree header. You use clk_set_rate_exclusive in the ISP driver on the module clock so it should prevent what you're mentioning from happening. If it doesn't, then clk_set_rate_exclusive has a bug and should be fixed. Either way, using assigned-clock-parents is not a good solution here either, it only makes sure that this is the case when probe is run. > Details regarding why the CSI module clock is best parented to the ISP > PLL are provided in the related commit. This is relevant to this commit too and "the related commit" is far too blurry when you consider the entire Linux git history. Maxime
Hi, On Mon 13 Sep 21, 09:54, Maxime Ripard wrote: > On Fri, Sep 10, 2021 at 08:41:26PM +0200, Paul Kocialkowski wrote: > > In order to reparent the CSI module clock to the ISP PLL via > > device-tree, export the ISP PLL clock declaration in the public > > device-tree header. > > You use clk_set_rate_exclusive in the ISP driver on the module clock so > it should prevent what you're mentioning from happening. It does, but then it breaks display support entirely (because the DRM driver doesn't use clk_set_rate_exclusive). The bottomline is that using the same PLL for both display and camera easily results in conflicts. > If it doesn't, then clk_set_rate_exclusive has a bug and should be > fixed. > > Either way, using assigned-clock-parents is not a good solution here > either, it only makes sure that this is the case when probe is run. I'm not sure what could provide better guarantees. There is a clock parenting API (in the clock framework) which may, but this implies providing the parent clock to the driver which seems way out of line since this is a platform-specific matter that should certainly not be handled by the driver. I also tried hardcoding the reparenting bit in the CCU driver, but this felt less clean than doing it in device-tree. What do you think? > > Details regarding why the CSI module clock is best parented to the ISP > > PLL are provided in the related commit. > > This is relevant to this commit too and "the related commit" is far too > blurry when you consider the entire Linux git history. Fair enough! Cheers, Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com
Salut Paul, On Mon, Sep 13, 2021 at 10:53:51AM +0200, Paul Kocialkowski wrote: > On Mon 13 Sep 21, 09:54, Maxime Ripard wrote: > > On Fri, Sep 10, 2021 at 08:41:26PM +0200, Paul Kocialkowski wrote: > > > In order to reparent the CSI module clock to the ISP PLL via > > > device-tree, export the ISP PLL clock declaration in the public > > > device-tree header. > > > > You use clk_set_rate_exclusive in the ISP driver on the module clock so > > it should prevent what you're mentioning from happening. > > It does, but then it breaks display support entirely (because the DRM > driver doesn't use clk_set_rate_exclusive). > > The bottomline is that using the same PLL for both display and camera > easily results in conflicts. The commit log should reflect that then > > If it doesn't, then clk_set_rate_exclusive has a bug and should be > > fixed. > > > > Either way, using assigned-clock-parents is not a good solution here > > either, it only makes sure that this is the case when probe is run. > > I'm not sure what could provide better guarantees. There is a clock > parenting API (in the clock framework) which may, but this implies > providing the parent clock to the driver which seems way out of line > since this is a platform-specific matter that should certainly not > be handled by the driver. > > I also tried hardcoding the reparenting bit in the CCU driver, but > this felt less clean than doing it in device-tree. > > What do you think? This is essentially policy, and putting it in the DT fails for the reason we already discussed, but also if we ever want to change it for example to optimize it a bit. In this case, we would have to deal with the old and new DT, and the possible consequences. So yeah, hardcoding it in the clock driver seems like a more sensible choice. Maxime
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-v3s.h b/drivers/clk/sunxi-ng/ccu-sun8i-v3s.h index 108eeeedcbf7..48e7e2b9fcf8 100644 --- a/drivers/clk/sunxi-ng/ccu-sun8i-v3s.h +++ b/drivers/clk/sunxi-ng/ccu-sun8i-v3s.h @@ -23,7 +23,6 @@ #define CLK_PLL_DDR0 8 #define CLK_PLL_PERIPH0 9 #define CLK_PLL_PERIPH0_2X 10 -#define CLK_PLL_ISP 11 #define CLK_PLL_PERIPH1 12 /* Reserve one number for not implemented and not used PLL_DDR1 */ diff --git a/include/dt-bindings/clock/sun8i-v3s-ccu.h b/include/dt-bindings/clock/sun8i-v3s-ccu.h index 014ac6123d17..75f15e2d5404 100644 --- a/include/dt-bindings/clock/sun8i-v3s-ccu.h +++ b/include/dt-bindings/clock/sun8i-v3s-ccu.h @@ -46,6 +46,7 @@ #ifndef _DT_BINDINGS_CLK_SUN8I_V3S_H_ #define _DT_BINDINGS_CLK_SUN8I_V3S_H_ +#define CLK_PLL_ISP 11 #define CLK_CPU 14 #define CLK_BUS_CE 20
In order to reparent the CSI module clock to the ISP PLL via device-tree, export the ISP PLL clock declaration in the public device-tree header. Details regarding why the CSI module clock is best parented to the ISP PLL are provided in the related commit. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- drivers/clk/sunxi-ng/ccu-sun8i-v3s.h | 1 - include/dt-bindings/clock/sun8i-v3s-ccu.h | 1 + 2 files changed, 1 insertion(+), 1 deletion(-)