mbox series

[00/10] clk: qcom: Introduce clocks drivers for SM8650

Message ID 20231025-topic-sm8650-upstream-clocks-v1-0-c89b59594caf@linaro.org
Headers show
Series clk: qcom: Introduce clocks drivers for SM8650 | expand

Message

Neil Armstrong Oct. 25, 2023, 7:32 a.m. UTC 
This patchset introduces the following SM8650 Clock drivers:
- GCC: Global Clock Controller
- DISPCC: Display Clock Controller
- TCSR Clock Controller
- GPUCC: GPU Clock Controller driver
- rpmh clocks

Dependencies: None

For convenience, a regularly refreshed linux-next based git tree containing
all the SM8650 related work is available at:
https://git.codelinaro.org/neil.armstrong/linux/-/tree/topic/sm85650/upstream/integ

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Neil Armstrong (10):
      dt-bindings: clock: qcom: document the SM8650 TCSR Clock Controller
      dt-bindings: clock: qcom: document the SM8650 General Clock Controller
      dt-bindings: clock: qcom: document the SM8650 Display Clock Controller
      dt-bindings: clock: qcom: document the SM8650 GPU Clock Controller
      dt-bindings: clock: qcom-rpmhcc: document the SM8650 RPMH Clock Controller
      clk: qcom: add the SM8650 Global Clock Controller driver
      clk: qcom: add the SM8650 TCSR Clock Controller driver
      clk: qcom: add the SM8650 Display Clock Controller driver
      clk: qcom: add the SM8650 GPU Clock Controller driver
      clk: qcom: rpmh: add clocks for SM8650

 .../devicetree/bindings/clock/qcom,rpmhcc.yaml     |    1 +
 .../bindings/clock/qcom,sm8450-gpucc.yaml          |    2 +
 .../bindings/clock/qcom,sm8650-dispcc.yaml         |  106 +
 .../devicetree/bindings/clock/qcom,sm8650-gcc.yaml |   65 +
 .../bindings/clock/qcom,sm8650-tcsr.yaml           |   55 +
 drivers/clk/qcom/Kconfig                           |   32 +
 drivers/clk/qcom/Makefile                          |    4 +
 drivers/clk/qcom/clk-rpmh.c                        |   29 +
 drivers/clk/qcom/dispcc-sm8650.c                   | 1806 +++++++++
 drivers/clk/qcom/gcc-sm8650.c                      | 3931 ++++++++++++++++++++
 drivers/clk/qcom/gpucc-sm8650.c                    |  660 ++++
 drivers/clk/qcom/tcsrcc-sm8650.c                   |  192 +
 include/dt-bindings/clock/qcom,sm8650-dispcc.h     |  101 +
 include/dt-bindings/clock/qcom,sm8650-gcc.h        |  257 ++
 include/dt-bindings/clock/qcom,sm8650-gpucc.h      |   43 +
 include/dt-bindings/clock/qcom,sm8650-tcsr.h       |   18 +
 include/dt-bindings/reset/qcom,sm8650-gpucc.h      |   20 +
 17 files changed, 7322 insertions(+)
---
base-commit: fe1998aa935b44ef873193c0772c43bce74f17dc
change-id: 20231016-topic-sm8650-upstream-clocks-3c09f464b7d4

Best regards,

Comments

Rob Herring (Arm) Oct. 25, 2023, 2:47 p.m. UTC | #1 
On Wed, 25 Oct 2023 09:32:40 +0200, Neil Armstrong wrote:
> Add bindings documentation for the SM8650 Display Clock Controller.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  .../bindings/clock/qcom,sm8650-dispcc.yaml         | 106 +++++++++++++++++++++
>  include/dt-bindings/clock/qcom,sm8650-dispcc.h     | 101 ++++++++++++++++++++
>  2 files changed, 207 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/clock/qcom,sm8650-dispcc.example.dts:18:18: fatal error: dt-bindings/clock/qcom,sm8650-gcc.h: No such file or directory
   18 |         #include <dt-bindings/clock/qcom,sm8650-gcc.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/clock/qcom,sm8650-dispcc.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1427: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231025-topic-sm8650-upstream-clocks-v1-3-c89b59594caf@linaro.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Rob Herring (Arm) Oct. 25, 2023, 7:40 p.m. UTC | #2 
On Wed, 25 Oct 2023 09:32:40 +0200, Neil Armstrong wrote:
> Add bindings documentation for the SM8650 Display Clock Controller.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  .../bindings/clock/qcom,sm8650-dispcc.yaml         | 106 +++++++++++++++++++++
>  include/dt-bindings/clock/qcom,sm8650-dispcc.h     | 101 ++++++++++++++++++++
>  2 files changed, 207 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Rob Herring (Arm) Oct. 25, 2023, 7:49 p.m. UTC | #3 
On Wed, Oct 25, 2023 at 09:47:33AM -0500, Rob Herring wrote:
> 
> On Wed, 25 Oct 2023 09:32:40 +0200, Neil Armstrong wrote:
> > Add bindings documentation for the SM8650 Display Clock Controller.
> > 
> > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > ---
> >  .../bindings/clock/qcom,sm8650-dispcc.yaml         | 106 +++++++++++++++++++++
> >  include/dt-bindings/clock/qcom,sm8650-dispcc.h     | 101 ++++++++++++++++++++
> >  2 files changed, 207 insertions(+)
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/clock/qcom,sm8650-dispcc.example.dts:18:18: fatal error: dt-bindings/clock/qcom,sm8650-gcc.h: No such file or directory
>    18 |         #include <dt-bindings/clock/qcom,sm8650-gcc.h>
>       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> compilation terminated.
> make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/clock/qcom,sm8650-dispcc.example.dtb] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1427: dt_binding_check] Error 2
> make: *** [Makefile:234: __sub-make] Error 2

Looks like the series got split up in the delivery causing this.

Rob
Neil Armstrong Oct. 26, 2023, 11:51 a.m. UTC | #4 
On 25/10/2023 10:41, Konrad Dybcio wrote:
> 
> 
> On 10/25/23 09:32, Neil Armstrong wrote:
>> Add Global Clock Controller (GCC) support for SM8650 platform.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
> Just a couple remarks
> 
> 1. looks like there's no usage of shared ops (corresponding
>     to enable_safe_parent or something along these lines
>     downstream)

Indeed, it was missing, I'll give a test before posting a v2.

> 
> 2. none of the GDSCs have interesting flags.. I have this
>     little cheat sheet that you may find handy:
> 
> qcom,retain-regs -> RETAIN_FF_ENABLE
> qcom,support-hw-trigger + set_mode in driver -> HW_CONTROL
> qcom,no-status-check-on-disable -> VOTABLE
> qcom,reset-aon-logic -> AON_RESET
> domain-addr  = clamp_io_ctrl

Thx, I updated the GDSCs.

> 
> 3. gcc_cpuss_ubwcp_clk_src uses the XO_A clock as parent, but
>     it's not there in the ftbl.. Could you confirm whether this
>     clock should even be accessed from HLOS?

Downstream this clock is only used by gem_noc, since we don't use such
clock upstream I think it's safer to remove it until we have the usage.

> 
> [...]
> 
>> +static int gcc_sm8650_probe(struct platform_device *pdev)
>> +{
>> +    struct regmap *regmap;
>> +    int ret;
>> +
>> +    regmap = qcom_cc_map(pdev, &gcc_sm8650_desc);
>> +    if (IS_ERR(regmap))
>> +        return PTR_ERR(regmap);
>> +
>> +    ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks,
>> +                       ARRAY_SIZE(gcc_dfs_clocks));
>> +    if (ret)
>> +        return ret;
>> +
>> +    /*
>> +     * Keep the critical clock always-On
>> +     * gcc_camera_ahb_clk, gcc_camera_xo_clk, gcc_disp_ahb_clk,
>> +     * gcc_disp_xo_clk, gcc_gpu_cfg_ahb_clk, gcc_video_ahb_clk,
>> +     * gcc_video_xo_clk
>> +     */
> Could you make these comments inline, i.e.
> 
> regmap_update_bits(regmap, 0x26004, BIT(0), BIT(0)); /* gcc_camera_ahb_clk */
> 
> ?

Done

> 
> Konrad

Thanks,
Neil
Neil Armstrong Oct. 26, 2023, 12:02 p.m. UTC | #5 
On 25/10/2023 10:45, Konrad Dybcio wrote:
> 
> 
> On 10/25/23 09:32, Neil Armstrong wrote:
>> Add Display Clock Controller (DISPCC) support for SM8650 platform.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
> [...]
> 
>> +static int disp_cc_sm8650_probe(struct platform_device *pdev)
>> +{
>> +    struct regmap *regmap;
>> +    int ret;
>> +
>> +    ret = devm_pm_runtime_enable(&pdev->dev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = pm_runtime_resume_and_get(&pdev->dev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    regmap = qcom_cc_map(pdev, &disp_cc_sm8650_desc);
>> +    if (IS_ERR(regmap))
>> +        return PTR_ERR(regmap);
> need to clean up RPM

Ack,

Thanks,
Neil

> 
> Konrad
Neil Armstrong Oct. 26, 2023, 12:27 p.m. UTC | #6 
On 25/10/2023 23:45, Stephen Boyd wrote:
> Quoting Neil Armstrong (2023-10-25 00:32:45)
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index c04b6526f4f3..5bf25e8d033c 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -842,6 +842,15 @@ config SM_DISPCC_8550
>>            Say Y if you want to support display devices and functionality such as
>>            splash screen.
>>   
>> +config SM_DISPCC_8650
>> +       tristate "SM8650 Display Clock Controller"
>> +       depends on SM_GCC_8650
> 
> selects?
> 
> We use selects instead of depends so that the driver can be built-in or
> modular regardless of parent clks that provide clks to this device.
> Orphan clk handling resolves issues with the driver registering clks
> before parents. And with fw_devlink the driver isn't even attempted to
> probe before the GCC driver is probed so there's no build dependency
> between these drivers.

All current DISPCC entries uses depends, but CAM_CC doesn't,
I'll switch to select and re-sync Kconfig for all 8650 entries.

> 
>> +       help
>> +         Support for the display clock controller on Qualcomm Technologies, Inc
>> +         SM8650 devices.
>> +         Say Y if you want to support display devices and functionality such as
>> +         splash screen.
>> +
>>   config SM_GCC_4450
>>          tristate "SM4450 Global Clock Controller"
>>          depends on ARM64 || COMPILE_TEST
>> diff --git a/drivers/clk/qcom/dispcc-sm8650.c b/drivers/clk/qcom/dispcc-sm8650.c
>> new file mode 100644
>> index 000000000000..7cb91306e895
>> --- /dev/null
>> +++ b/drivers/clk/qcom/dispcc-sm8650.c
>> @@ -0,0 +1,1806 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023, Linaro Ltd.
>> + */
>> +
>> +#include <linux/clk.h>
> 
> Is this include used?
> 
>> +#include <linux/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
> 
> Is this mod_devicetable.h?
> 
>> +#include <linux/of.h>
> 
> Is this include used?
> 
>> +#include <linux/regmap.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +#include <dt-bindings/clock/qcom,sm8650-dispcc.h>
>> +
>> +#include "common.h"
>> +#include "clk-alpha-pll.h"
>> +#include "clk-branch.h"
>> +#include "clk-pll.h"
>> +#include "clk-rcg.h"
>> +#include "clk-regmap.h"
>> +#include "clk-regmap-divider.h"
>> +#include "clk-regmap-mux.h"
> 
> Is this include used?
> 
>> +#include "reset.h"
>> +#include "gdsc.h"
>> +

Did a include cleanup aswell on all drivers.

Thanks,
Neil