Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6dc9f36a-f003-06eb-744c-0ebe645dfdf0@quicinc.com>
Date:   Wed, 14 Jun 2023 17:25:29 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH V4 2/4] clk: qcom: camcc-sm8550: Add camera clock
 controller driver for SM8550
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC:     Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-clk@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Taniya Das <quic_tdas@quicinc.com>,
        "Satya Priya Kakitapalli" <quic_skakitap@quicinc.com>,
        Imran Shaik <quic_imrashai@quicinc.com>,
        Ajit Pandey <quic_ajipan@quicinc.com>
References: <20230609115058.9059-1-quic_jkona@quicinc.com>
 <20230609115058.9059-3-quic_jkona@quicinc.com>
 <CAA8EJpr-iKMzYP7HVQV8pzXbxzLvBaq38aovJ5Ffny18yXvJZg@mail.gmail.com>
From:   Jagadeesh Kona <quic_jkona@quicinc.com>
In-Reply-To: <CAA8EJpr-iKMzYP7HVQV8pzXbxzLvBaq38aovJ5Ffny18yXvJZg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/9/2023 9:52 PM, Dmitry Baryshkov wrote:
> On Fri, 9 Jun 2023 at 14:52, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>>
>> Add support for the camera clock controller for camera clients to be
>> able to request for camcc clocks on SM8550 platform.
>>
>> Co-developed-by: Taniya Das <quic_tdas@quicinc.com>
>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> ---
>> Changes since V3:
>>   - No changes
>> Changes since V2:
>>   - No changes
>> Changes since V1:
>>   - Sorted the PLL names in proper order
>>   - Updated all PLL configurations to lower case hex
>>   - Reused evo ops instead of adding new ops for ole pll
>>   - Moved few clocks to separate patch to fix patch too long error
>>
>>   drivers/clk/qcom/Kconfig        |    7 +
>>   drivers/clk/qcom/Makefile       |    1 +
>>   drivers/clk/qcom/camcc-sm8550.c | 3405 +++++++++++++++++++++++++++++++
>>   3 files changed, 3413 insertions(+)
>>   create mode 100644 drivers/clk/qcom/camcc-sm8550.c
>>
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index 9cd1f05d436b..85efed78dc9a 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -756,6 +756,13 @@ config SM_CAMCC_8450
>>            Support for the camera clock controller on SM8450 devices.
>>            Say Y if you want to support camera devices and camera functionality.
>>
>> +config SM_CAMCC_8550
>> +       tristate "SM8550 Camera Clock Controller"
>> +       select SM_GCC_8550
>> +       help
>> +         Support for the camera clock controller on SM8550 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 75d035150118..97c8cefc2fd0 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -101,6 +101,7 @@ obj-$(CONFIG_SDX_GCC_75) += gcc-sdx75.o
>>   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_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-sm8550.c b/drivers/clk/qcom/camcc-sm8550.c
>> new file mode 100644
>> index 000000000000..85f0c1e09b2b
>> --- /dev/null
>> +++ b/drivers/clk/qcom/camcc-sm8550.c
>> @@ -0,0 +1,3405 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <dt-bindings/clock/qcom,sm8550-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,
>> +};
>> +
>> +enum {
>> +       P_BI_TCXO,
>> +       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_CAM_CC_PLL11_OUT_EVEN,
>> +       P_CAM_CC_PLL12_OUT_EVEN,
>> +};
>> +
>> +static const struct pll_vco lucid_ole_vco[] = {
>> +       { 249600000, 2300000000, 0 },
>> +};
>> +
>> +static const struct pll_vco rivian_ole_vco[] = {
>> +       { 777000000, 1285000000, 0 },
>> +};
>> +
>> +static const struct alpha_pll_config cam_cc_pll0_config = {
>> +       /* .l includes RINGOSC_CAL_L_VAL, CAL_L_VAL, L_VAL fields */
>> +       .l = 0x4444003e,
> 
> I'd still insist on not touching the config.l field semantics.
> 

We feel it is better to update config->l field and reuse existing code 
than adding separate function for lucid ole pll configure.

>> +       .alpha = 0x8000,
>> +       .config_ctl_val = 0x20485699,
>> +       .config_ctl_hi_val = 0x00182261,
>> +       .config_ctl_hi1_val = 0x82aa299c,
>> +       .test_ctl_val = 0x00000000,
>> +       .test_ctl_hi_val = 0x00000003,
>> +       .test_ctl_hi1_val = 0x00009000,
>> +       .test_ctl_hi2_val = 0x00000034,
>> +       .user_ctl_val = 0x00008400,
>> +       .user_ctl_hi_val = 0x00000005,
>> +};
>> +
> 
> [skipped the rest, LGTM]
> 
>> +
>> +static struct platform_driver cam_cc_sm8550_driver = {
>> +       .probe = cam_cc_sm8550_probe,
>> +       .driver = {
>> +               .name = "cam_cc-sm8550",
>> +               .of_match_table = cam_cc_sm8550_match_table,
>> +       },
>> +};
>> +
>> +static int __init cam_cc_sm8550_init(void)
>> +{
>> +       return platform_driver_register(&cam_cc_sm8550_driver);
>> +}
>> +subsys_initcall(cam_cc_sm8550_init);
> 
> As it was pointed out, this driver is built as a module by default.
> Please perform the tesing and cleanup before sending the driver and
> use module_platform_driver.
> 

We want clock drivers to be probed early in the bootup to avoid any 
probe deferrals of consumer drivers. If there is any scenario where 
clock drivers are built statically into kernel, then subsys_initcall() 
will ensure clock drivers are probed earlier. When built as module, 
subsys_initcall() will fallback to module_init() which is same as 
module_platform_driver().

Thanks,
Jagadeesh

>> +
>> +static void __exit cam_cc_sm8550_exit(void)
>> +{
>> +       platform_driver_unregister(&cam_cc_sm8550_driver);
>> +}
>> +module_exit(cam_cc_sm8550_exit);
>> +
>> +MODULE_DESCRIPTION("QTI CAMCC SM8550 Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.40.1
>>
> 
>