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.
Changes since v3:
- Split the patch 3/6 into two: first one moves existing strings, second
one adds new compatible strings to the conditionals.
Changes since v2:
- Added the conditional clause to schema forbidding usage of clocks and
clock-names on platforms which do not pass additional clocks to the
APCS device node (Krzysztof)
- Added SDX55 compat string
- Moved MSM8976 compat to the list of platforms using syscon.
Changes since v1:
- Removed the clk.h header inclusion (Stephen)
- Changed the module license from GPL v2 to bare GPL.
Dmitry Baryshkov (7):
dt-bindings: mailbox: qcom: add SDX55 compatible
dt-bindings: mailbox: qcom: enable syscon compatible for msm8976
dt-bindings: mailbox: qcom: correct the list of platforms using clocks
dt-bindings: mailbox: qcom: add missing platforms to conditional
clauses
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 | 37 ++++++---
drivers/clk/qcom/Makefile | 2 +-
drivers/clk/qcom/apcs-msm8996.c | 76 +++++++++++++++++++
drivers/mailbox/qcom-apcs-ipc-mailbox.c | 2 +-
4 files changed, 106 insertions(+), 11 deletions(-)
create mode 100644 drivers/clk/qcom/apcs-msm8996.c
--
2.39.0
The commit 0d17014e9189 ("dt-bindings: mailbox: Add binding for SDX55
APCS") added SDX55 compatible string to one of clock-selection
conditions, but failed to add one to the main schema's compatible list.
Fix this omission.
Fixes: 0d17014e9189 ("dt-bindings: mailbox: Add binding for SDX55 APCS")
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Dmitry Baryshkov <[email protected]>
---
.../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 943f9472ae10..36deddca1524 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
@@ -39,6 +39,7 @@ properties:
- qcom,msm8953-apcs-kpss-global
- qcom,msm8994-apcs-kpss-global
- qcom,qcs404-apcs-apps-global
+ - qcom,sdx55-apcs-gcc
- const: syscon
reg:
maxItems: 1
--
2.39.0
The schema incorrectly lists some of the platforms in the statement
requiring clocks/clock-names. Correct this by moving platforms not
requiring additional clocks to the separate clause.
Fixes: 0d17014e9189 ("dt-bindings: mailbox: Add binding for SDX55 APCS")
Signed-off-by: Dmitry Baryshkov <[email protected]>
---
.../mailbox/qcom,apcs-kpss-global.yaml | 26 +++++++++++++------
1 file changed, 18 insertions(+), 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 ecc286ab49ef..5f7770036fed 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
@@ -72,15 +72,7 @@ allOf:
compatible:
enum:
- qcom,msm8916-apcs-kpss-global
- - qcom,msm8994-apcs-kpss-global
- - qcom,msm8996-apcs-hmss-global
- - qcom,msm8998-apcs-hmss-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:
@@ -124,6 +116,24 @@ allOf:
items:
- const: pll
- const: xo
+
+ - if:
+ properties:
+ compatible:
+ enum:
+ - qcom,msm8994-apcs-kpss-global
+ - qcom,msm8996-apcs-hmss-global
+ - qcom,msm8998-apcs-hmss-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: false
+ clock-names: false
+
- if:
properties:
compatible:
--
2.39.0
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 <[email protected]>
---
drivers/clk/qcom/Makefile | 2 +-
drivers/clk/qcom/apcs-msm8996.c | 76 +++++++++++++++++++++++++++++++++
2 files changed, 77 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 6c589f671003..98523c48c541 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..7e46ea8ed444
--- /dev/null
+++ b/drivers/clk/qcom/apcs-msm8996.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Qualcomm APCS clock controller driver
+ *
+ * Copyright (c) 2022, Linaro Limited
+ * Author: Dmitry Baryshkov <[email protected]>
+ */
+
+#include <linux/bits.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 <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Qualcomm MSM8996 APCS clock driver");
--
2.39.0
On 18/01/2023 14:22, Dmitry Baryshkov wrote:
> The schema incorrectly lists some of the platforms in the statement
> requiring clocks/clock-names. Correct this by moving platforms not
> requiring additional clocks to the separate clause.
>
> Fixes: 0d17014e9189 ("dt-bindings: mailbox: Add binding for SDX55 APCS")
> Signed-off-by: Dmitry Baryshkov <[email protected]>
> ---
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Quoting Dmitry Baryshkov (2023-01-18 05:22:54)
> diff --git a/drivers/clk/qcom/apcs-msm8996.c b/drivers/clk/qcom/apcs-msm8996.c
> new file mode 100644
> index 000000000000..7e46ea8ed444
> --- /dev/null
> +++ b/drivers/clk/qcom/apcs-msm8996.c
> @@ -0,0 +1,76 @@
[...]
> +
> +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 */
Delay for what? Setting the divider? What if the register value didn't
change at all? Can you skip the delay in that case?
> + 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.
The function doesn't block or return EPROBE_DEFER if the clk is orphaned
when registered. Why is this necessary? Are you getting defered by the
fw_devlink logic thinking it needs to defer probe of this driver until
gpll0 provider probes? We should fix fw_devlink to not do that. Maybe if
the node is a clk provider (#clock-cells exists) then we don't wait for
clocks property to be provided, because the clk core already handles
that itself.
> + */
> + hw = devm_clk_hw_register_fixed_rate(dev, "sys_apcs_aux", NULL, 0, 300000000);
On 25.01.2023 22:38, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2023-01-18 05:22:54)
>> diff --git a/drivers/clk/qcom/apcs-msm8996.c b/drivers/clk/qcom/apcs-msm8996.c
>> new file mode 100644
>> index 000000000000..7e46ea8ed444
>> --- /dev/null
>> +++ b/drivers/clk/qcom/apcs-msm8996.c
>> @@ -0,0 +1,76 @@
> [...]
>> +
>> +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 */
>
> Delay for what? Setting the divider? What if the register value didn't
> change at all? Can you skip the delay in that case?
Waiting 5 us unconditionally in exchange for ensured CPU clock
source stability sounds like a rather fair deal.. Checking if
the register value changed would not save us much time..
Konrad
>
>> + 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.
>
> The function doesn't block or return EPROBE_DEFER if the clk is orphaned
> when registered. Why is this necessary? Are you getting defered by the
> fw_devlink logic thinking it needs to defer probe of this driver until
> gpll0 provider probes? We should fix fw_devlink to not do that. Maybe if
> the node is a clk provider (#clock-cells exists) then we don't wait for
> clocks property to be provided, because the clk core already handles
> that itself.
>
>> + */
>> + hw = devm_clk_hw_register_fixed_rate(dev, "sys_apcs_aux", NULL, 0, 300000000);
Quoting Konrad Dybcio (2023-01-25 13:48:54)
>
>
> On 25.01.2023 22:38, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2023-01-18 05:22:54)
> >> diff --git a/drivers/clk/qcom/apcs-msm8996.c b/drivers/clk/qcom/apcs-msm8996.c
> >> new file mode 100644
> >> index 000000000000..7e46ea8ed444
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/apcs-msm8996.c
> >> @@ -0,0 +1,76 @@
> > [...]
> >> +
> >> +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 */
> >
> > Delay for what? Setting the divider? What if the register value didn't
> > change at all? Can you skip the delay in that case?
> Waiting 5 us unconditionally in exchange for ensured CPU clock
> source stability sounds like a rather fair deal.. Checking if
> the register value changed would not save us much time..
So it is waiting for the CPU clk to be stable? The comment is not clear.
On 25.01.2023 22:56, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2023-01-25 13:48:54)
>>
>>
>> On 25.01.2023 22:38, Stephen Boyd wrote:
>>> Quoting Dmitry Baryshkov (2023-01-18 05:22:54)
>>>> diff --git a/drivers/clk/qcom/apcs-msm8996.c b/drivers/clk/qcom/apcs-msm8996.c
>>>> new file mode 100644
>>>> index 000000000000..7e46ea8ed444
>>>> --- /dev/null
>>>> +++ b/drivers/clk/qcom/apcs-msm8996.c
>>>> @@ -0,0 +1,76 @@
>>> [...]
>>>> +
>>>> +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 */
>>>
>>> Delay for what? Setting the divider? What if the register value didn't
>>> change at all? Can you skip the delay in that case?
>> Waiting 5 us unconditionally in exchange for ensured CPU clock
>> source stability sounds like a rather fair deal.. Checking if
>> the register value changed would not save us much time..
>
> So it is waiting for the CPU clk to be stable? The comment is not clear.
Okay, so perhaps this is just a misunderstanding because of a lackluster
comment.. This SYS_APCS_AUX (provided by this driver) is one of the CPU
clock sources (and probably the "safest" of them all, as it's fed by
GPLL0 and not the CPU PLLs) the delay is there to ensure it can
stabilize after setting the divider to DIV2. In a theoretical case, the
big 8996 cpucc driver could select this clock as a target for one (or
both) of the per-cluster muxes and it could put the CPUs in a weird state.
As unlikely as that would be, especially considering 8996 (AFAIK) doesn't
use this clock source coming out of reset / bootloader, this lets us
ensure one less thing can break.
Konrad
Quoting Konrad Dybcio (2023-01-25 14:05:27)
>
> On 25.01.2023 22:56, Stephen Boyd wrote:
> >
> > So it is waiting for the CPU clk to be stable? The comment is not clear.
> Okay, so perhaps this is just a misunderstanding because of a lackluster
> comment.. This SYS_APCS_AUX (provided by this driver) is one of the CPU
> clock sources (and probably the "safest" of them all, as it's fed by
> GPLL0 and not the CPU PLLs) the delay is there to ensure it can
> stabilize after setting the divider to DIV2. In a theoretical case, the
> big 8996 cpucc driver could select this clock as a target for one (or
> both) of the per-cluster muxes and it could put the CPUs in a weird state.
>
> As unlikely as that would be, especially considering 8996 (AFAIK) doesn't
> use this clock source coming out of reset / bootloader, this lets us
> ensure one less thing can break.
Great! I look forward to a better comment.
On 25/01/2023 23:38, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2023-01-18 05:22:54)
>> diff --git a/drivers/clk/qcom/apcs-msm8996.c b/drivers/clk/qcom/apcs-msm8996.c
>> new file mode 100644
>> index 000000000000..7e46ea8ed444
>> --- /dev/null
>> +++ b/drivers/clk/qcom/apcs-msm8996.c
>> @@ -0,0 +1,76 @@
> [...]
>> +
>> +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 */
>
> Delay for what? Setting the divider? What if the register value didn't
> change at all? Can you skip the delay in that case?
Ack, I'll expand the comment.
>
>> + 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.
>
> The function doesn't block or return EPROBE_DEFER if the clk is orphaned
> when registered. Why is this necessary? Are you getting defered by the
> fw_devlink logic thinking it needs to defer probe of this driver until
> gpll0 provider probes? We should fix fw_devlink to not do that. Maybe if
> the node is a clk provider (#clock-cells exists) then we don't wait for
> clocks property to be provided, because the clk core already handles
> that itself.
Letting clock-controllers probe was my idea for the patch, but it was
delayed again by Saravana, see [1], [2]
[1]
https://lore.kernel.org/all/[email protected]/
[2]
https://lore.kernel.org/all/CAGETcx8Xy5OzsbW3123esxsbQJq-SqDkP1S5g2mmwzoCz4shtQ@mail.gmail.com/
>
>> + */
>> + hw = devm_clk_hw_register_fixed_rate(dev, "sys_apcs_aux", NULL, 0, 300000000);
--
With best wishes
Dmitry
Quoting Dmitry Baryshkov (2023-01-26 14:51:51)
> On 25/01/2023 23:38, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2023-01-18 05:22:54)
> >> diff --git a/drivers/clk/qcom/apcs-msm8996.c b/drivers/clk/qcom/apcs-msm8996.c
> >> new file mode 100644
> >> index 000000000000..7e46ea8ed444
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/apcs-msm8996.c
> >> @@ -0,0 +1,76 @@
> >> +
> >> + /*
> >> + * Register the clock as fixed rate instead of being a child of gpll0
> >> + * to let the driver register probe as early as possible.
> >
> > The function doesn't block or return EPROBE_DEFER if the clk is orphaned
> > when registered. Why is this necessary? Are you getting defered by the
> > fw_devlink logic thinking it needs to defer probe of this driver until
> > gpll0 provider probes? We should fix fw_devlink to not do that. Maybe if
> > the node is a clk provider (#clock-cells exists) then we don't wait for
> > clocks property to be provided, because the clk core already handles
> > that itself.
>
> Letting clock-controllers probe was my idea for the patch, but it was
> delayed again by Saravana, see [1], [2]
Ah, I didn't see it because linux-clk wasn't Cced and I look at that
mail pile less regularly.