Re: [PATCH V2 RESEND 5/6] clk: qcom: camcc-sm8650: Add SM8650 camera clock controller driver

[Jagadeesh Kona <quic_jkona@quicinc.com>]

On 4/19/2024 3:00 AM, Vladimir Zapolskiy wrote:
> Hello Jagadeesh,
> 
> thank you for submitting the clock driver.
> 
> On 3/21/24 11:25, 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>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   drivers/clk/qcom/Kconfig        |    8 +
>>   drivers/clk/qcom/Makefile       |    1 +
>>   drivers/clk/qcom/camcc-sm8650.c | 3591 +++++++++++++++++++++++++++++++
>>   3 files changed, 3600 insertions(+)
>>   create mode 100644 drivers/clk/qcom/camcc-sm8650.c
>>
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index 8ab08e7b5b6c..6257f4a02ec4 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -826,6 +826,14 @@ config SM_CAMCC_8550
>>         Support for the camera clock controller on SM8550 devices.
>>         Say Y if you want to support camera devices and camera 
>> functionality.
>> +config SM_CAMCC_8650
>> +    tristate "SM8650 Camera Clock Controller"
>> +    depends on ARM64 || COMPILE_TEST
>> +    select SM_GCC_8650
>> +    help
>> +      Support for the camera clock controller on SM8650 devices.
>> +      Say Y if you want to support camera devices and camera 
>> functionality.
>> +
>>   config SM_DISPCC_6115
>>       tristate "SM6115 Display Clock Controller"
>>       depends on ARM64 || COMPILE_TEST
>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>> index dec5b6db6860..28bffa1eb8dd 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -109,6 +109,7 @@ obj-$(CONFIG_SM_CAMCC_6350) += camcc-sm6350.o
>>   obj-$(CONFIG_SM_CAMCC_8250) += camcc-sm8250.o
>>   obj-$(CONFIG_SM_CAMCC_8450) += camcc-sm8450.o
>>   obj-$(CONFIG_SM_CAMCC_8550) += camcc-sm8550.o
>> +obj-$(CONFIG_SM_CAMCC_8650) += camcc-sm8650.o
>>   obj-$(CONFIG_SM_DISPCC_6115) += dispcc-sm6115.o
>>   obj-$(CONFIG_SM_DISPCC_6125) += dispcc-sm6125.o
>>   obj-$(CONFIG_SM_DISPCC_6350) += dispcc-sm6350.o
>> diff --git a/drivers/clk/qcom/camcc-sm8650.c 
>> b/drivers/clk/qcom/camcc-sm8650.c
>> new file mode 100644
>> index 000000000000..1b28e086e519
>> --- /dev/null
>> +++ b/drivers/clk/qcom/camcc-sm8650.c
>> @@ -0,0 +1,3591 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h>
>> +
>> +#include "clk-alpha-pll.h"
>> +#include "clk-branch.h"
>> +#include "clk-rcg.h"
>> +#include "clk-regmap.h"
>> +#include "common.h"
>> +#include "gdsc.h"
>> +#include "reset.h"
>> +
>> +enum {
>> +    DT_IFACE,
>> +    DT_BI_TCXO,
>> +    DT_BI_TCXO_AO,
>> +    DT_SLEEP_CLK,
>> +};
>> +
>> +enum {
>> +    P_BI_TCXO,
>> +    P_BI_TCXO_AO,
>> +    P_CAM_CC_PLL0_OUT_EVEN,
>> +    P_CAM_CC_PLL0_OUT_MAIN,
>> +    P_CAM_CC_PLL0_OUT_ODD,
>> +    P_CAM_CC_PLL1_OUT_EVEN,
>> +    P_CAM_CC_PLL2_OUT_EVEN,
>> +    P_CAM_CC_PLL2_OUT_MAIN,
>> +    P_CAM_CC_PLL3_OUT_EVEN,
>> +    P_CAM_CC_PLL4_OUT_EVEN,
>> +    P_CAM_CC_PLL5_OUT_EVEN,
>> +    P_CAM_CC_PLL6_OUT_EVEN,
>> +    P_CAM_CC_PLL7_OUT_EVEN,
>> +    P_CAM_CC_PLL8_OUT_EVEN,
>> +    P_CAM_CC_PLL9_OUT_EVEN,
>> +    P_CAM_CC_PLL9_OUT_ODD,
>> +    P_CAM_CC_PLL10_OUT_EVEN,
>> +    P_SLEEP_CLK,
>> +};
>> +
>> +static const struct pll_vco lucid_ole_vco[] = {
>> +    { 249600000, 2300000000, 0 },
>> +};
> 
> I've noticed that a downstream Android kernel v6.1.25 defines this clock as
> 
>      static const struct pll_vco lucid_ole_vco[] = {
>          { 249600000, 2100000000, 0 },
>      };
> 
> Do you know any particular reason why here the clock frequencies are 
> different?
> 

Thanks Vladimir for your review!

The min and max supported frequencies of PLL mentioned above are taken 
from the HW specification, and as per the latest HW spec, the maximum 
supported frequency for lucid OLE PLL is 2300MHz, hence used 2300MHz above.

>> +
>> +static const struct pll_vco rivian_ole_vco[] = {
>> +    { 777000000, 1285000000, 0 },
>> +};
>> +
> 
> <snip>
> 
>> +static struct clk_rcg2 cam_cc_bps_clk_src = {
>> +    .cmd_rcgr = 0x10050,
>> +    .mnd_width = 0,
>> +    .hid_width = 5,
>> +    .parent_map = cam_cc_parent_map_2,
>> +    .freq_tbl = ftbl_cam_cc_bps_clk_src,
>> +    .clkr.hw.init = &(const struct clk_init_data) {
>> +        .name = "cam_cc_bps_clk_src",
>> +        .parent_data = cam_cc_parent_data_2,
>> +        .num_parents = ARRAY_SIZE(cam_cc_parent_data_2),
>> +        .flags = CLK_SET_RATE_PARENT,
>> +        .ops = &clk_rcg2_shared_ops,
>> +    },
>> +};
> 
> Please let me ask after Dmitry about your rationale to select
> &clk_rcg2_shared_ops here and below for all *_src clocks introduced in
> the driver, I do remember you've did it in v1, could you please
> elaborate it a bit more?
> 
> I have a concern that it's not possible to get an .is_enabled status
> of the shared clocks, however at least in this particular case of
> camcc clocks it seems to be technically possible.
> 
> It might indicate that there is an incompleteness in clk-rcg2.c driver
> also, if it's really possible to get is_enabled runtime status at least
> for some of the shared clocks.
> 

The recommendation from HW team is to park the RCG's at XO clock source 
when RCG is in disabled state and clk_rcg2_shared_ops is the closest 
implementation for achieving the same, hence used clk_rcg2_shared_ops 
for all the RCG's.

I will check if .is_enabled callback can be added to shared_ops and post 
a separate series for it based on the requirement.

>> +
>> +static const struct freq_tbl ftbl_cam_cc_camnoc_axi_rt_clk_src[] = {
>> +    F(300000000, P_CAM_CC_PLL0_OUT_EVEN, 2, 0, 0),
>> +    F(400000000, P_CAM_CC_PLL0_OUT_ODD, 1, 0, 0),
>> +    { }
>> +};
>> +
> 
> <snip>
> 
> Other than two my open questions above I don't see any issues with the
> driver, if you be kind to provide the answers, please feel free to add
> my
> 
> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> Tested-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> 

Thanks, sure will add these tags in next series.

Thanks,
Jagadeesh

> -- 
> Best wishes,
> Vladimir
>