Messages in this thread | | | Date | Wed, 14 Feb 2024 11:28:48 +0530 | Subject | Re: [PATCH 4/5] clk: qcom: camcc-sm8650: Add camera clock controller driver for SM8650 | From | Jagadeesh Kona <> |
| |
On 2/12/2024 6:46 PM, Dmitry Baryshkov wrote: > On Mon, 12 Feb 2024 at 15:09, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> >> >> On 2/7/2024 6:41 PM, Bryan O'Donoghue wrote: >>> On 06/02/2024 11:31, Jagadeesh Kona wrote: >>>> Add support for the camera clock controller for camera clients to be >>>> able to request for camcc clocks on SM8650 platform. >>>> >>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>> >>>> +static struct clk_rcg2 cam_cc_mclk1_clk_src = { >>>> + .cmd_rcgr = 0x1501c, >>>> + .mnd_width = 8, >>>> + .hid_width = 5, >>>> + .parent_map = cam_cc_parent_map_1, >>>> + .freq_tbl = ftbl_cam_cc_mclk0_clk_src, >>>> + .clkr.hw.init = &(const struct clk_init_data) { >>>> + .name = "cam_cc_mclk1_clk_src", >>>> + .parent_data = cam_cc_parent_data_1, >>>> + .num_parents = ARRAY_SIZE(cam_cc_parent_data_1), >>>> + .flags = CLK_SET_RATE_PARENT, >>>> + .ops = &clk_rcg2_shared_ops, >>> >>> Nice. >>> >>> I compared this to WIP for x1e80100 which looks nearly register >>> compatible. Use of the shared_ops indicates to me you've thought about >>> which clocks should not be switched all the way off. >>> >> >> Thanks Bryan for your review, We want all RCG's to be parked at safe >> config(XO) when they are disabled, hence using shared ops for all the >> RCG's. > > What is the reason for parking it instead of fully disabling the clock? >
We don't do anything explicit in RCG disable, normally when all branch clocks are disabled, RCG gets disabled in HW. But as per the HW design recommendation, RCG needs to be parked at a safe clock source(XO) during disable path, hence we use shared_ops to achieve the same. After parking at XO, RCG gets disabled as all the branches are disabled.
Thanks, Jagadeesh
>> >> >>>> +static struct platform_driver cam_cc_sm8650_driver = { >>>> + .probe = cam_cc_sm8650_probe, >>>> + .driver = { >>>> + .name = "cam_cc-sm8650", >>> >>> That said .. please fix the name here "cam_cc-sm8650". The title of your >>> series is "camcc-sm8650" which IMO is a much more appropriate name. >>> >>> The admixture of hyphen "-" and underscore "_" is some kind of >>> tokenisation sin. >>> >> >> Sure, will fix this in next series. >> >> Thanks, >> Jagadeesh >> >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> > >
| |