[PATCH 0/4] clk: qcom: msm8996: add APCS clock driver

[PATCH 0/4] clk: qcom: msm8996: add APCS clock driver

[Dmitry Baryshkov]

The sys_apcs_aux clock can be used by CPU and CBF clock drivers to drive
those clocks from GPLL0 while doing initial setup. Add simple driver to
setup and export this clock.

Dmitry Baryshkov (4):
  dt-bindings: mailbox: qcom: correct the list of platforms using clocks
  dt-bindings: mailbox: qcom: add #clock-cells to msm8996 example
  mailbox: qcom-apcs-ipc: enable APCS clock device for MSM8996
  clk: qcom: add the driver for the MSM8996 APCS clocks

 .../mailbox/qcom,apcs-kpss-global.yaml        | 10 +--
 drivers/clk/qcom/Makefile                     |  2 +-
 drivers/clk/qcom/apcs-msm8996.c               | 77 +++++++++++++++++++
 drivers/mailbox/qcom-apcs-ipc-mailbox.c       |  2 +-
 4 files changed, 81 insertions(+), 10 deletions(-)
 create mode 100644 drivers/clk/qcom/apcs-msm8996.c

-- 
2.30.2

[PATCH 1/4] dt-bindings: mailbox: qcom: correct the list of platforms using clocks

[Dmitry Baryshkov]

Only three platforms require `pll' and `aux' clocks: msm8916, msm8939
and qcs404. Correct the list of platforms in the corresponding clause.

Fixes: 0d17014e9189 ("dt-bindings: mailbox: Add binding for SDX55 APCS")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../bindings/mailbox/qcom,apcs-kpss-global.yaml          | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
index 943f9472ae10..b8a44ef0540f 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
@@ -71,15 +71,8 @@ allOf:
         compatible:
           enum:
             - qcom,msm8916-apcs-kpss-global
-            - qcom,msm8994-apcs-kpss-global
-            - qcom,msm8996-apcs-hmss-global
-            - qcom,msm8998-apcs-hmss-global
+            - qcom,msm8939-apcs-kpss-global
             - qcom,qcs404-apcs-apps-global
-            - qcom,sc7180-apss-shared
-            - qcom,sdm660-apcs-hmss-global
-            - qcom,sdm845-apss-shared
-            - qcom,sm6125-apcs-hmss-global
-            - qcom,sm8150-apss-shared
     then:
       properties:
         clocks:
-- 
2.30.2

[PATCH 2/4] dt-bindings: mailbox: qcom: add #clock-cells to msm8996 example

[Dmitry Baryshkov]

Add the #clock-cells property to the MSM8996 example, as the APCS block
is going to provide the `sys_apcs_aux' clock to the consumers.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml       | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
index b8a44ef0540f..5e7c83345774 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
@@ -141,6 +141,7 @@ examples:
         reg = <0x9820000 0x1000>;
 
         #mbox-cells = <1>;
+        #clock-cells = <0>;
     };
 
     rpm-glink {
-- 
2.30.2

[PATCH 3/4] mailbox: qcom-apcs-ipc: enable APCS clock device for MSM8996

[Dmitry Baryshkov]

MSM8996 also has the clock-related part of the APCS mailbox device.
Follow the usual pattern and create a child device to handle these
clocks.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/mailbox/qcom-apcs-ipc-mailbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index 0e9f9cba8668..bd61dacb44ba 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -42,7 +42,7 @@ static const struct qcom_apcs_ipc_data msm8994_apcs_data = {
 };
 
 static const struct qcom_apcs_ipc_data msm8996_apcs_data = {
-	.offset = 16, .clk_name = NULL
+	.offset = 16, .clk_name = "qcom-apcs-msm8996-clk"
 };
 
 static const struct qcom_apcs_ipc_data apps_shared_apcs_data = {
-- 
2.30.2

[PATCH 4/4] clk: qcom: add the driver for the MSM8996 APCS clocks

[Dmitry Baryshkov]

Add a simple driver handling the APCS clocks on MSM8996. For now it
supports just a single aux clock, linking GPLL0 to CPU and CBF clocks.

Note, there is little sense in registering sys_apcs_aux as a child of
gpll0. The PLL is always-on. And listing the gpll0 as a property of the
apcs would delay its probing until the GCC has been probed (while we
would like for the apcs to be probed as early as possible).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/Makefile       |  2 +-
 drivers/clk/qcom/apcs-msm8996.c | 77 +++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/qcom/apcs-msm8996.c

diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index f18c446a97ea..ca2f586edb3e 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -52,7 +52,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_APCC_MSM8996) += clk-cpu-8996.o
+obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += apcs-msm8996.o clk-cpu-8996.o
 obj-$(CONFIG_QCOM_CLK_APCS_SDX55) += apcs-sdx55.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
 obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
diff --git a/drivers/clk/qcom/apcs-msm8996.c b/drivers/clk/qcom/apcs-msm8996.c
new file mode 100644
index 000000000000..2e9959974ed9
--- /dev/null
+++ b/drivers/clk/qcom/apcs-msm8996.c
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Qualcomm APCS clock controller driver
+ *
+ * Copyright (c) 2022, Linaro Limited
+ * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define APCS_AUX_OFFSET	0x50
+
+#define APCS_AUX_DIV_MASK GENMASK(17, 16)
+#define APCS_AUX_DIV_2 0x1
+
+static int qcom_apcs_msm8996_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device *parent = dev->parent;
+	struct regmap *regmap;
+	struct clk_hw *hw;
+	unsigned int val;
+	int ret = -ENODEV;
+
+	regmap = dev_get_regmap(parent, NULL);
+	if (!regmap) {
+		dev_err(dev, "failed to get regmap: %d\n", ret);
+		return ret;
+	}
+
+	regmap_read(regmap, APCS_AUX_OFFSET, &val);
+	regmap_update_bits(regmap, APCS_AUX_OFFSET, APCS_AUX_DIV_MASK,
+			   FIELD_PREP(APCS_AUX_DIV_MASK, APCS_AUX_DIV_2));
+
+	/* Hardware mandated delay */
+	udelay(5);
+
+	/*
+	 * Register the clock as fixed rate instead of being a child of gpll0
+	 * to let the driver register probe as early as possible.
+	 */
+	hw = devm_clk_hw_register_fixed_rate(dev, "sys_apcs_aux", NULL, 0, 300000000);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
+}
+
+static struct platform_driver qcom_apcs_msm8996_clk_driver = {
+	.probe = qcom_apcs_msm8996_clk_probe,
+	.driver = {
+		.name = "qcom-apcs-msm8996-clk",
+	},
+};
+
+/* Register early enough to fix the clock to be used for other cores */
+static int __init qcom_apcs_msm8996_clk_init(void)
+{
+	return platform_driver_register(&qcom_apcs_msm8996_clk_driver);
+}
+postcore_initcall(qcom_apcs_msm8996_clk_init);
+
+static void __exit qcom_apcs_msm8996_clk_exit(void)
+{
+	platform_driver_unregister(&qcom_apcs_msm8996_clk_driver);
+}
+module_exit(qcom_apcs_msm8996_clk_exit);
+
+MODULE_AUTHOR("Dmitry Baryshkov <dmitry.baryshkov@linaro.org>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Qualcomm MSM8996 APCS clock driver");
-- 
2.30.2

Re: [PATCH 4/4] clk: qcom: add the driver for the MSM8996 APCS clocks

[Konrad Dybcio]

On 11.01.2023 20:14, Dmitry Baryshkov wrote:
> Add a simple driver handling the APCS clocks on MSM8996. For now it
> supports just a single aux clock, linking GPLL0 to CPU and CBF clocks.
> 
> Note, there is little sense in registering sys_apcs_aux as a child of
> gpll0. The PLL is always-on. And listing the gpll0 as a property of the
> apcs would delay its probing until the GCC has been probed (while we
> would like for the apcs to be probed as early as possible).
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/Makefile       |  2 +-
>  drivers/clk/qcom/apcs-msm8996.c | 77 +++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/qcom/apcs-msm8996.c
> 
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index f18c446a97ea..ca2f586edb3e 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -52,7 +52,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_APCC_MSM8996) += clk-cpu-8996.o
> +obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += apcs-msm8996.o clk-cpu-8996.o
>  obj-$(CONFIG_QCOM_CLK_APCS_SDX55) += apcs-sdx55.o
>  obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
>  obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
> diff --git a/drivers/clk/qcom/apcs-msm8996.c b/drivers/clk/qcom/apcs-msm8996.c
> new file mode 100644
> index 000000000000..2e9959974ed9
> --- /dev/null
> +++ b/drivers/clk/qcom/apcs-msm8996.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm APCS clock controller driver
> + *
> + * Copyright (c) 2022, Linaro Limited
> + * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define APCS_AUX_OFFSET	0x50
> +
> +#define APCS_AUX_DIV_MASK GENMASK(17, 16)
> +#define APCS_AUX_DIV_2 0x1
> +
> +static int qcom_apcs_msm8996_clk_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device *parent = dev->parent;
> +	struct regmap *regmap;
> +	struct clk_hw *hw;
> +	unsigned int val;
> +	int ret = -ENODEV;
> +
> +	regmap = dev_get_regmap(parent, NULL);
> +	if (!regmap) {
> +		dev_err(dev, "failed to get regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	regmap_read(regmap, APCS_AUX_OFFSET, &val);
> +	regmap_update_bits(regmap, APCS_AUX_OFFSET, APCS_AUX_DIV_MASK,
> +			   FIELD_PREP(APCS_AUX_DIV_MASK, APCS_AUX_DIV_2));
> +
> +	/* Hardware mandated delay */
> +	udelay(5);
> +
> +	/*
> +	 * Register the clock as fixed rate instead of being a child of gpll0
> +	 * to let the driver register probe as early as possible.
> +	 */
Not sure.. you should keep a vote in GPLL0_ao supplied by XO_A
and perhaps it would be a better idea to move RPMCC (+deps) and
GCC to very early initcalls since there's a need for that..

Maybe it would even allow us to shave some miliseconds from
boot times, at less things would defer!

Konrad
> +	hw = devm_clk_hw_register_fixed_rate(dev, "sys_apcs_aux", NULL, 0, 300000000);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
> +}
> +
> +static struct platform_driver qcom_apcs_msm8996_clk_driver = {
> +	.probe = qcom_apcs_msm8996_clk_probe,
> +	.driver = {
> +		.name = "qcom-apcs-msm8996-clk",
> +	},
> +};
> +
> +/* Register early enough to fix the clock to be used for other cores */
> +static int __init qcom_apcs_msm8996_clk_init(void)
> +{
> +	return platform_driver_register(&qcom_apcs_msm8996_clk_driver);
> +}
> +postcore_initcall(qcom_apcs_msm8996_clk_init);
> +
> +static void __exit qcom_apcs_msm8996_clk_exit(void)
> +{
> +	platform_driver_unregister(&qcom_apcs_msm8996_clk_driver);
> +}
> +module_exit(qcom_apcs_msm8996_clk_exit);
> +
> +MODULE_AUTHOR("Dmitry Baryshkov <dmitry.baryshkov@linaro.org>");
> +MODULE_LICENSE("GPL v2");
"GPL"

> +MODULE_DESCRIPTION("Qualcomm MSM8996 APCS clock driver");

Re: [PATCH 4/4] clk: qcom: add the driver for the MSM8996 APCS clocks

[Dmitry Baryshkov]

On 12/01/2023 17:23, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 20:14, Dmitry Baryshkov wrote:
>> Add a simple driver handling the APCS clocks on MSM8996. For now it
>> supports just a single aux clock, linking GPLL0 to CPU and CBF clocks.
>>
>> Note, there is little sense in registering sys_apcs_aux as a child of
>> gpll0. The PLL is always-on. And listing the gpll0 as a property of the
>> apcs would delay its probing until the GCC has been probed (while we
>> would like for the apcs to be probed as early as possible).
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/clk/qcom/Makefile       |  2 +-
>>   drivers/clk/qcom/apcs-msm8996.c | 77 +++++++++++++++++++++++++++++++++
>>   2 files changed, 78 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/clk/qcom/apcs-msm8996.c
>>
>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>> index f18c446a97ea..ca2f586edb3e 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -52,7 +52,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_APCC_MSM8996) += clk-cpu-8996.o
>> +obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += apcs-msm8996.o clk-cpu-8996.o
>>   obj-$(CONFIG_QCOM_CLK_APCS_SDX55) += apcs-sdx55.o
>>   obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
>>   obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
>> diff --git a/drivers/clk/qcom/apcs-msm8996.c b/drivers/clk/qcom/apcs-msm8996.c
>> new file mode 100644
>> index 000000000000..2e9959974ed9
>> --- /dev/null
>> +++ b/drivers/clk/qcom/apcs-msm8996.c
>> @@ -0,0 +1,77 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Qualcomm APCS clock controller driver
>> + *
>> + * Copyright (c) 2022, Linaro Limited
>> + * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> + */
>> +
>> +#include <linux/bits.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#define APCS_AUX_OFFSET	0x50
>> +
>> +#define APCS_AUX_DIV_MASK GENMASK(17, 16)
>> +#define APCS_AUX_DIV_2 0x1
>> +
>> +static int qcom_apcs_msm8996_clk_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device *parent = dev->parent;
>> +	struct regmap *regmap;
>> +	struct clk_hw *hw;
>> +	unsigned int val;
>> +	int ret = -ENODEV;
>> +
>> +	regmap = dev_get_regmap(parent, NULL);
>> +	if (!regmap) {
>> +		dev_err(dev, "failed to get regmap: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	regmap_read(regmap, APCS_AUX_OFFSET, &val);
>> +	regmap_update_bits(regmap, APCS_AUX_OFFSET, APCS_AUX_DIV_MASK,
>> +			   FIELD_PREP(APCS_AUX_DIV_MASK, APCS_AUX_DIV_2));
>> +
>> +	/* Hardware mandated delay */
>> +	udelay(5);
>> +
>> +	/*
>> +	 * Register the clock as fixed rate instead of being a child of gpll0
>> +	 * to let the driver register probe as early as possible.
>> +	 */
> Not sure.. you should keep a vote in GPLL0_ao supplied by XO_A
> and perhaps it would be a better idea to move RPMCC (+deps) and
> GCC to very early initcalls since there's a need for that..
> 
> Maybe it would even allow us to shave some miliseconds from
> boot times, at less things would defer!

That's what I'm trying to stay away from. No deferrals. These pieces 
(APCS, CPU and CBF clocks) should be initialized as early as possible. 
E.g. Qualcomm did all the init manually, even before the devices had a 
chance to probe. Probe deferral means that the device is going to be put 
to the end of the list.

clk-smd-rpmcc and gcc-msm8996 are registered at core_initcall. This is 
the earliest usable init level.

> 
> Konrad
>> +	hw = devm_clk_hw_register_fixed_rate(dev, "sys_apcs_aux", NULL, 0, 300000000);
>> +	if (IS_ERR(hw))
>> +		return PTR_ERR(hw);
>> +
>> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
>> +}
>> +
>> +static struct platform_driver qcom_apcs_msm8996_clk_driver = {
>> +	.probe = qcom_apcs_msm8996_clk_probe,
>> +	.driver = {
>> +		.name = "qcom-apcs-msm8996-clk",
>> +	},
>> +};
>> +
>> +/* Register early enough to fix the clock to be used for other cores */
>> +static int __init qcom_apcs_msm8996_clk_init(void)
>> +{
>> +	return platform_driver_register(&qcom_apcs_msm8996_clk_driver);
>> +}
>> +postcore_initcall(qcom_apcs_msm8996_clk_init);
>> +
>> +static void __exit qcom_apcs_msm8996_clk_exit(void)
>> +{
>> +	platform_driver_unregister(&qcom_apcs_msm8996_clk_driver);
>> +}
>> +module_exit(qcom_apcs_msm8996_clk_exit);
>> +
>> +MODULE_AUTHOR("Dmitry Baryshkov <dmitry.baryshkov@linaro.org>");
>> +MODULE_LICENSE("GPL v2");
> "GPL"
> 
>> +MODULE_DESCRIPTION("Qualcomm MSM8996 APCS clock driver");

-- 
With best wishes
Dmitry

Re: [PATCH 4/4] clk: qcom: add the driver for the MSM8996 APCS clocks

[Stephen Boyd]

Quoting Dmitry Baryshkov (2023-01-11 11:14:53)
> diff --git a/drivers/clk/qcom/apcs-msm8996.c b/drivers/clk/qcom/apcs-msm8996.c
> new file mode 100644
> index 000000000000..2e9959974ed9
> --- /dev/null
> +++ b/drivers/clk/qcom/apcs-msm8996.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm APCS clock controller driver
> + *
> + * Copyright (c) 2022, Linaro Limited
> + * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/clk.h>

Remove this include please. It indicates that this is a clk consumer,
when this driver is only a clk provider.

> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>