Re: [PATCH v2 5/5] clk: qcom: Add SDX55 APCS clock controller support
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
> > ---
> > 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 ..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
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#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 = >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) {
> > +
Re: [PATCH v2 5/5] clk: qcom: Add SDX55 APCS clock controller support
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?
>
> This is required for enabling CPU frequency scaling on SDX55-based
> platforms.
>
> Signed-off-by: Manivannan Sadhasivam
> ---
> 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
> + 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 ..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
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "clk-regmap.h"
> +#include "clk-regmap-mux-div.h"
> +#include "common.h"
Curious what common is needed for?
> +
> +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?
> +};
> +
> +/*
> + * 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 = >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 = _regmap_mux_div_ops;
> +
> +
[PATCH v2 5/5] clk: qcom: Add SDX55 APCS clock controller support
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.
This is required for enabling CPU frequency scaling on SDX55-based
platforms.
Signed-off-by: Manivannan Sadhasivam
---
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
+ 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 ..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
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "clk-regmap.h"
+#include "clk-regmap-mux-div.h"
+#include "common.h"
+
+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", },
+};
+
+/*
+ * 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 = >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;
+
+ regmap = dev_get_regmap(parent, NULL);
+ if (!regmap) {
+ dev_err(dev, "Failed to get parent regmap: %d\n", ret);
+ return ret;
+ }
+
+ 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 = _regmap_mux_div_ops;
+
+ a7cc->clkr.hw.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);
+ return ret;
+ }
+
+ a7cc->clk_nb.notifier_call = a7cc_notifier_cb;
+ ret =
