Re: [PATCH 3/7] clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018

[Stephen Boyd <sboyd@xxxxxxxxxx>]

Quoting Varadarajan Narayanan (2020-09-27 22:15:36)
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 0583273..d1a2504 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -155,6 +155,14 @@ config IPQ_GCC_8074
>           i2c, USB, SD/eMMC, etc. Select this for the root clock
>           of ipq8074.
>  
> +config IPQ_GCC_5018
> +       tristate "IPQ5018 Global Clock Controller"
> +       help
> +        Support for global clock controller on ipq5018 devices.
> +        Say Y if you want to use peripheral devices such as UART, SPI,
> +        i2c, USB, SD/eMMC, etc. Select this for the root clock
> +        of ipq5018.

What is the root clock of ipq5018? Please drop that last sentence.

> +
>  config MSM_GCC_8660
>         tristate "MSM8660 Global Clock Controller"
>         help
> diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
> new file mode 100644
> index 00000000..9056386
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-ipq5018.c
> @@ -0,0 +1,3833 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/reset-controller.h>

Why is this attached to dt-bindings? Please remove that newline above
and move this away from dt-bindings below.

> +#include <dt-bindings/clock/qcom,gcc-ipq5018.h>
> +#include <dt-bindings/reset/qcom,gcc-ipq5018.h>
> +
> +#include "common.h"
> +#include "clk-regmap.h"
> +#include "clk-pll.h"
> +#include "clk-rcg.h"
> +#include "clk-branch.h"
> +#include "clk-alpha-pll.h"
> +#include "clk-regmap-divider.h"
> +#include "clk-regmap-mux.h"
> +#include "reset.h"
> +
> +#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }

This is in clk-rcg.h already.

> +
> +static const char * const gcc_usb3phy_0_cc_pipe_clk_xo[] = {
> +       "usb3phy_0_cc_pipe_clk",
> +       "xo",
> +};

All these names structures need to change, see next comment.

> +
> +static struct clk_rcg2 apss_ahb_clk_src = {
> +       .cmd_rcgr = 0x46000,
> +       .mnd_width = 0,
> +       .hid_width = 5,
> +       .freq_tbl = ftbl_apss_ahb_clk_src,
> +       .parent_map = gcc_xo_gpll0_gpll0_out_main_div2_map,
> +       .clkr.hw.init = &(struct clk_init_data){
> +               .name = "apss_ahb_clk_src",
> +               .parent_names = gcc_xo_gpll0_gpll0_out_main_div2,
> +               .num_parents = 3,

Please migrate to the new way of specifying clks with clk_init_data::clk_parent_data

> +               .ops = &clk_rcg2_ops,
> +               .flags = CLK_IS_CRITICAL | CLK_IGNORE_UNUSED,

Why is it critical and ignore unused? Do you need this clk to be here at
all? Can we just enable it when this driver probes with a register write
and then ignore it from there on out?

> +       },
> +};
> +
> +static struct clk_regmap_div apss_ahb_postdiv_clk_src = {
> +       .reg = 0x46018,
> +       .shift = 4,
> +       .width = 4,
> +       .clkr = {
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "apss_ahb_postdiv_clk_src",
> +                       .parent_names = (const char *[]){
> +                               "apss_ahb_clk_src"
> +                       },
> +                       .num_parents = 1,
> +                       .ops = &clk_regmap_div_ops,
> +               },
> +       },
> +};
> +
[...]
> +
> +static struct clk_branch gcc_qdss_dap_clk = {
> +       .halt_reg = 0x29084,
> +       .clkr = {
> +               .enable_reg = 0x29084,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_qdss_dap_clk",
> +                       .parent_names = (const char *[]){
> +                               "qdss_tsctr_clk_src"
> +                       },
> +                       .num_parents = 1,
> +                       .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,

Whenever CLK_IS_CRITICAL is there please document why it is needed. And
if possible remove the clk structure and hit the clk on in driver probe
so we don't waste memory modeling something that never matters.
Typically that can only be done if nothing references this clk as a
parent or if we're willing to break the clk tree and ignore describing
parents. In this case it's a branch so probably nothing else is under it
so we can just turn it on during probe and stop caring.

> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch gcc_qdss_cfg_ahb_clk = {
> +       .halt_reg = 0x29008,
> +       .clkr = {
> +               .enable_reg = 0x29008,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_qdss_cfg_ahb_clk",
> +                       .parent_names = (const char *[]){
> +                               "pcnoc_clk_src"
> +                       },
> +                       .num_parents = 1,
> +                       .flags = CLK_SET_RATE_PARENT,
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
[...]
> +
> +static struct clk_branch gcc_qdss_stm_clk = {
> +       .halt_reg = 0x29044,
> +       .clkr = {
> +               .enable_reg = 0x29044,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_qdss_stm_clk",
> +                       .parent_names = (const char *[]){
> +                               "qdss_stm_clk_src"
> +                       },
> +                       .num_parents = 1,
> +                       .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,

Why ignore unused? Probably should just be turned on somewhere else?

> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch gcc_qdss_traceclkin_clk = {
> +       .halt_reg = 0x29060,
> +       .clkr = {
> +               .enable_reg = 0x29060,
[...]
> +
> +static int gcc_ipq5018_probe(struct platform_device *pdev)
> +{
> +       int i, ret;
> +       struct regmap *regmap;
> +       struct clk *clk;
> +       struct qcom_cc_desc ipq5018_desc = gcc_ipq5018_desc;
> +
> +       regmap = qcom_cc_map(pdev, &ipq5018_desc);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       for (i = 0; i < ARRAY_SIZE(gcc_ipq5018_hws); i++) {
> +               clk = devm_clk_register(&pdev->dev, gcc_ipq5018_hws[i]);
> +               if (IS_ERR(clk))
> +                       return PTR_ERR(clk);
> +       }

We really need to move this into the qcom_cc_desc so it is part of
qcom_cc_really_probe()

> +       /*Gen2 PHY*/
> +       clk_register_fixed_rate(&pdev->dev, "pcie20_phy0_pipe_clk", NULL,
> +                                       CLK_IS_ROOT, 125000000);
> +       clk_register_fixed_rate(&pdev->dev, "pcie20_phy1_pipe_clk", NULL,
> +                                       CLK_IS_ROOT, 125000000);

These should be coming from some pcie phy and part of the DT binding as
a 'clocks' element that this device consumes.

> +
> +       clk_alpha_pll_configure(&ubi32_pll_main, regmap, &ubi32_pll_config);
> +
> +       ret = qcom_cc_really_probe(pdev, &ipq5018_desc, regmap);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to register ipq5018 GCC clocks\n");
> +               return ret;
> +       }
> +
> +       dev_info(&pdev->dev, "Registered ipq5018 GCC clocks provider");

Please drop this noise.

> +
> +       return ret;
> +}
> +
> +static int gcc_ipq5018_remove(struct platform_device *pdev)
> +{
> +       return 0;
> +}
> +

If there isn't anything in the remove function it can be omitted.

> +static struct platform_driver gcc_ipq5018_driver = {
> +       .probe = gcc_ipq5018_probe,
> +       .remove = gcc_ipq5018_remove,
> +       .driver = {
> +               .name   = "qcom,gcc-ipq5018",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = gcc_ipq5018_match_table,
> +       },
> +};
> +
> +static int __init gcc_ipq5018_init(void)
> +{
> +       return platform_driver_register(&gcc_ipq5018_driver);
> +}
> +core_initcall(gcc_ipq5018_init);
> +
> +static void __exit gcc_ipq5018_exit(void)
> +{
> +       platform_driver_unregister(&gcc_ipq5018_driver);
> +}
> +module_exit(gcc_ipq5018_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. GCC IPQ5018 Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:gcc-ipq5018");

I think alias isn't needed anymore.

> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 03a5de5..31fde45 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -20,8 +20,8 @@
>  #define CLK_SET_PARENT_GATE    BIT(1) /* must be gated across re-parent */
>  #define CLK_SET_RATE_PARENT    BIT(2) /* propagate rate change up one level */
>  #define CLK_IGNORE_UNUSED      BIT(3) /* do not gate even if unused */
> -                               /* unused */
> -                               /* unused */
> +#define CLK_IS_ROOT            BIT(4) /* root clk, has no parent */
> +#define CLK_IS_BASIC           BIT(5) /* Basic clk, can't do a to_clk_foo() */

Please no. Drop this hunk.

>  #define CLK_GET_RATE_NOCACHE   BIT(6) /* do not use the cached clk rate */
>  #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
>  #define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */