Re: [PATCH v2 5/5] clk: qcom: Add SDX55 APCS clock controller support

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Tue, Jan 12, 2021 at 11:37:04PM -0800, Stephen Boyd wrote:
> Quoting Manivannan Sadhasivam (2021-01-08 03:32:33)
> > Add a driver for the SDX55 APCS clock controller. It is part of the APCS
> > hardware block, which among other things implements also a combined mux
> > and half integer divider functionality. The APCS clock controller has 3
> > parent clocks:
> > 
> > 1. Board XO
> > 2. Fixed rate GPLL0
> > 3. A7 PLL
> > 
> > The source and the divider can be set both at the same time.
> 
> I don't understand what that means. Presumably it's a mux/divider
> combined?
> 

Yeah, will make it clear.

> > 
> > This is required for enabling CPU frequency scaling on SDX55-based
> > platforms.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/clk/qcom/Kconfig      |   9 ++
> >  drivers/clk/qcom/Makefile     |   1 +
> >  drivers/clk/qcom/apcs-sdx55.c | 149 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 159 insertions(+)
> >  create mode 100644 drivers/clk/qcom/apcs-sdx55.c
> > 
> > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > index d6f4aee4427a..2c67fdfae913 100644
> > --- a/drivers/clk/qcom/Kconfig
> > +++ b/drivers/clk/qcom/Kconfig
> > @@ -45,6 +45,15 @@ config QCOM_CLK_APCS_MSM8916
> >           Say Y if you want to support CPU frequency scaling on devices
> >           such as msm8916.
> >  
> > +config QCOM_CLK_APCS_SDX55
> 
> APCC comes before APCS
> 

Okay

> > +       tristate "SDX55 APCS Clock Controller"
> > +       depends on QCOM_APCS_IPC || COMPILE_TEST
> > +       help
> > +         Support for the APCS Clock Controller on SDX55 platform. The
> > +         APCS is managing the mux and divider which feeds the CPUs.
> > +         Say Y if you want to support CPU frequency scaling on devices
> > +         such as SDX55.
> > +
> >  config QCOM_CLK_APCC_MSM8996
> >         tristate "MSM8996 CPU Clock Controller"
> >         select QCOM_KRYO_L2_ACCESSORS
> > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > index e7e0ac382176..a9271f40916c 100644
> > --- a/drivers/clk/qcom/Makefile
> > +++ b/drivers/clk/qcom/Makefile
> > @@ -46,6 +46,7 @@ obj-$(CONFIG_MSM_MMCC_8998) += mmcc-msm8998.o
> >  obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
> >  obj-$(CONFIG_QCOM_A7PLL) += a7-pll.o
> >  obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
> > +obj-$(CONFIG_QCOM_CLK_APCS_SDX55) += apcs-sdx55.o
> >  obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += clk-cpu-8996.o
> >  obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
> >  obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
> > diff --git a/drivers/clk/qcom/apcs-sdx55.c b/drivers/clk/qcom/apcs-sdx55.c
> > new file mode 100644
> > index 000000000000..14413c957d83
> > --- /dev/null
> > +++ b/drivers/clk/qcom/apcs-sdx55.c
> > @@ -0,0 +1,149 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Qualcomm SDX55 APCS clock controller driver
> > + *
> > + * Copyright (c) 2020, Linaro Limited
> > + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/cpu.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#include "clk-regmap.h"
> > +#include "clk-regmap-mux-div.h"
> > +#include "common.h"
> 
> Curious what common is needed for?
> 

Not needed, will remove.

> > +
> > +static const u32 apcs_mux_clk_parent_map[] = { 0, 1, 5 };
> > +
> > +static const struct clk_parent_data pdata[] = {
> > +       { .fw_name = "ref", .name = "bi_tcxo", },
> > +       { .fw_name = "aux", .name = "gpll0", },
> > +       { .fw_name = "pll", .name = "a7pll", },
> 
> Please remove name from here. It shouldn't be necessary if the DT
> describes things properly. Or there isn't DT for this device?
> 

Will remove.

> > +};
> > +
> > +/*
> > + * We use the notifier function for switching to a temporary safe configuration
> > + * (mux and divider), while the A7 PLL is reconfigured.
> > + */
> > +static int a7cc_notifier_cb(struct notifier_block *nb, unsigned long event,
> > +                           void *data)
> > +{
> > +       int ret = 0;
> > +       struct clk_regmap_mux_div *md = container_of(nb,
> > +                                                    struct clk_regmap_mux_div,
> > +                                                    clk_nb);
> > +       if (event == PRE_RATE_CHANGE)
> > +               /* set the mux and divider to safe frequency (400mhz) */
> > +               ret = mux_div_set_src_div(md, 1, 2);
> > +
> > +       return notifier_from_errno(ret);
> > +}
> > +
> > +static int qcom_apcs_sdx55_clk_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct device *parent = dev->parent;
> > +       struct device *cpu_dev;
> > +       struct clk_regmap_mux_div *a7cc;
> > +       struct regmap *regmap;
> > +       struct clk_init_data init = { };
> > +       int ret = -ENODEV;
> 
> Drop assignement..
> 
> > +
> > +       regmap = dev_get_regmap(parent, NULL);
> > +       if (!regmap) {
> > +               dev_err(dev, "Failed to get parent regmap: %d\n", ret);
> > +               return ret;
> 
> .. and Just return -ENODEV?
> 
> > +       }
> > +
> > +       a7cc = devm_kzalloc(dev, sizeof(*a7cc), GFP_KERNEL);
> > +       if (!a7cc)
> > +               return -ENOMEM;
> > +
> > +       init.name = "a7mux";
> > +       init.parent_data = pdata;
> > +       init.num_parents = ARRAY_SIZE(pdata);
> > +       init.ops = &clk_regmap_mux_div_ops;
> > +
> > +       a7cc->clkr.hw.init = &init;
> > +       a7cc->clkr.regmap = regmap;
> > +       a7cc->reg_offset = 0x8;
> > +       a7cc->hid_width = 5;
> > +       a7cc->hid_shift = 0;
> > +       a7cc->src_width = 3;
> > +       a7cc->src_shift = 8;
> > +       a7cc->parent_map = apcs_mux_clk_parent_map;
> > +
> > +       a7cc->pclk = devm_clk_get(parent, "pll");
> > +       if (IS_ERR(a7cc->pclk)) {
> > +               ret = PTR_ERR(a7cc->pclk);
> > +               if (ret != -EPROBE_DEFER)
> > +                       dev_err(dev, "Failed to get PLL clk: %d\n", ret);
> 
> Use dev_err_probe() please.
> 
> > +               return ret;
> > +       }
> > +
> > +       a7cc->clk_nb.notifier_call = a7cc_notifier_cb;
> > +       ret = clk_notifier_register(a7cc->pclk, &a7cc->clk_nb);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to register clock notifier: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ret = devm_clk_register_regmap(dev, &a7cc->clkr);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to register regmap clock: %d\n", ret);
> > +               goto err;
> > +       }
> > +
> > +       ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> > +                                         &a7cc->clkr.hw);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to add clock provider: %d\n", ret);
> > +               goto err;
> > +       }
> > +
> > +       platform_set_drvdata(pdev, a7cc);
> > +
> > +       /*
> > +        * Attach the power domain to cpudev. There seems to be no better place
> > +        * to do this, so do it here.
> > +        */
> > +       cpu_dev = get_cpu_device(0);
> > +       dev_pm_domain_attach(cpu_dev, true);
> 
> I guess this works given that we don't have CPU drivers. The comment
> says what the code is doing but doesn't say why it's doing it. Adding
> why may help understand in the future and would be a better comment.
> Why can't cpufreq-dt attach a power domain from DT for a cpu device? Is
> that a bad idea?
> 

Yeah, I talked with Viresh about using cpufreq-dt for attaching the power
domain but he said it isn't the appropriate place. Hence, I decided to use
this driver.

Will make the comment more elaborate.

Thanks,
Mani

> > +
> > +       return 0;
> > +
> > +err:
> > +       clk_notifier_unregister(a7cc->pclk, &a7cc->clk_nb);
> > +       return ret;
> > +}