Register by hand a platform device for the ACPM clocks.
The ACPM clocks are not modeled as a DT child of ACPM because:
1/ they don't have their own resources.
2/ they are not a block that can be reused. The clock identifying
data is reduced (clock ID, clock name and mailbox channel ID)
and may differ from a SoC to another.
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
drivers/firmware/samsung/exynos-acpm.c | 64 +++++++++++++++++++++++++++++++++-
1 file changed, 63 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
index 9fa0335ccf5db32892fdf09e8d4b0a885a8f8fb5..86a220a845d2934aa28e9bb8996cf914f65cdae6 100644
--- a/drivers/firmware/samsung/exynos-acpm.c
+++ b/drivers/firmware/samsung/exynos-acpm.c
@@ -24,10 +24,13 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_platform.h>
+#include <linux/platform_data/clk-acpm.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/types.h>
+#include <dt-bindings/clock/google,gs101.h>
+
#include "exynos-acpm.h"
#include "exynos-acpm-dvfs.h"
#include "exynos-acpm-pmic.h"
@@ -160,6 +163,7 @@ struct acpm_chan {
* struct acpm_info - driver's private data.
* @shmem: pointer to the SRAM configuration data.
* @sram_base: base address of SRAM.
+ * @clk_pdev: ACPM clocks platform device.
* @chans: pointer to the ACPM channel parameters retrieved from SRAM.
* @dev: pointer to the exynos-acpm device.
* @handle: instance of acpm_handle to send to clients.
@@ -168,6 +172,7 @@ struct acpm_chan {
struct acpm_info {
struct acpm_shmem __iomem *shmem;
void __iomem *sram_base;
+ struct platform_device *clk_pdev;
struct acpm_chan *chans;
struct device *dev;
struct acpm_handle handle;
@@ -177,14 +182,39 @@ struct acpm_info {
/**
* struct acpm_match_data - of_device_id data.
* @initdata_base: offset in SRAM where the channels configuration resides.
+ * @acpm_clk_pdata: ACPM clocks platform data.
*/
struct acpm_match_data {
loff_t initdata_base;
+ const struct acpm_clk_platform_data *acpm_clk_pdata;
};
#define client_to_acpm_chan(c) container_of(c, struct acpm_chan, cl)
#define handle_to_acpm_info(h) container_of(h, struct acpm_info, handle)
+#define ACPM_CLK(_id, cname) \
+ { \
+ .id = _id, \
+ .name = cname, \
+ }
+
+static const struct acpm_clk_variant gs101_acpm_clks[] = {
+ ACPM_CLK(CLK_ACPM_DVFS_MIF, "mif"),
+ ACPM_CLK(CLK_ACPM_DVFS_INT, "int"),
+ ACPM_CLK(CLK_ACPM_DVFS_CPUCL0, "cpucl0"),
+ ACPM_CLK(CLK_ACPM_DVFS_CPUCL1, "cpucl1"),
+ ACPM_CLK(CLK_ACPM_DVFS_CPUCL2, "cpucl2"),
+ ACPM_CLK(CLK_ACPM_DVFS_G3D, "g3d"),
+ ACPM_CLK(CLK_ACPM_DVFS_G3DL2, "g3dl2"),
+ ACPM_CLK(CLK_ACPM_DVFS_TPU, "tpu"),
+ ACPM_CLK(CLK_ACPM_DVFS_INTCAM, "intcam"),
+ ACPM_CLK(CLK_ACPM_DVFS_TNR, "tnr"),
+ ACPM_CLK(CLK_ACPM_DVFS_CAM, "cam"),
+ ACPM_CLK(CLK_ACPM_DVFS_MFC, "mfc"),
+ ACPM_CLK(CLK_ACPM_DVFS_DISP, "disp"),
+ ACPM_CLK(CLK_ACPM_DVFS_BO, "b0"),
+};
+
/**
* acpm_get_saved_rx() - get the response if it was already saved.
* @achan: ACPM channel info.
@@ -606,6 +636,7 @@ static void acpm_setup_ops(struct acpm_info *acpm)
static int acpm_probe(struct platform_device *pdev)
{
+ const struct acpm_clk_platform_data *acpm_clk_pdata;
const struct acpm_match_data *match_data;
struct device *dev = &pdev->dev;
struct device_node *shmem;
@@ -647,7 +678,30 @@ static int acpm_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, acpm);
- return devm_of_platform_populate(dev);
+ acpm_clk_pdata = match_data->acpm_clk_pdata;
+ acpm->clk_pdev = platform_device_register_data(dev, "acpm-clocks",
+ PLATFORM_DEVID_NONE,
+ acpm_clk_pdata,
+ sizeof(*acpm_clk_pdata));
+ if (IS_ERR(acpm->clk_pdev))
+ return dev_err_probe(dev, PTR_ERR(acpm->clk_pdev),
+ "Failed to register ACPM clocks device.\n");
+
+ ret = devm_of_platform_populate(dev);
+ if (ret) {
+ platform_device_unregister(acpm->clk_pdev);
+ return dev_err_probe(dev, ret,
+ "Failed to populate platform devices.\n");
+ }
+
+ return 0;
+}
+
+static void acpm_remove(struct platform_device *pdev)
+{
+ struct acpm_info *acpm = platform_get_drvdata(pdev);
+
+ platform_device_unregister(acpm->clk_pdev);
}
/**
@@ -744,8 +798,15 @@ const struct acpm_handle *devm_acpm_get_by_node(struct device *dev,
}
EXPORT_SYMBOL_GPL(devm_acpm_get_by_node);
+static const struct acpm_clk_platform_data acpm_clk_gs101 = {
+ .clks = gs101_acpm_clks,
+ .nr_clks = ARRAY_SIZE(gs101_acpm_clks),
+ .mbox_chan_id = 0,
+};
+
static const struct acpm_match_data acpm_gs101 = {
.initdata_base = ACPM_GS101_INITDATA_BASE,
+ .acpm_clk_pdata = &acpm_clk_gs101,
};
static const struct of_device_id acpm_match[] = {
@@ -759,6 +820,7 @@ MODULE_DEVICE_TABLE(of, acpm_match);
static struct platform_driver acpm_driver = {
.probe = acpm_probe,
+ .remove = acpm_remove,
.driver = {
.name = "exynos-acpm-protocol",
.of_match_table = acpm_match,
--
2.51.0.261.g7ce5a0a67e-googOn 27/08/2025 14:42, Tudor Ambarus wrote:
> +
> +static const struct acpm_clk_variant gs101_acpm_clks[] = {
> + ACPM_CLK(CLK_ACPM_DVFS_MIF, "mif"),
> + ACPM_CLK(CLK_ACPM_DVFS_INT, "int"),
> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL0, "cpucl0"),
> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL1, "cpucl1"),
> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL2, "cpucl2"),
> + ACPM_CLK(CLK_ACPM_DVFS_G3D, "g3d"),
> + ACPM_CLK(CLK_ACPM_DVFS_G3DL2, "g3dl2"),
> + ACPM_CLK(CLK_ACPM_DVFS_TPU, "tpu"),
> + ACPM_CLK(CLK_ACPM_DVFS_INTCAM, "intcam"),
> + ACPM_CLK(CLK_ACPM_DVFS_TNR, "tnr"),
> + ACPM_CLK(CLK_ACPM_DVFS_CAM, "cam"),
> + ACPM_CLK(CLK_ACPM_DVFS_MFC, "mfc"),
> + ACPM_CLK(CLK_ACPM_DVFS_DISP, "disp"),
> + ACPM_CLK(CLK_ACPM_DVFS_BO, "b0"),
> +};
I don't understand why clocks are defined in the firmware driver, not in
the clock driver.
This creates dependency of this patch on the clock patch, so basically
there is no way I will take it in one cycle.
> +
> /**
> * acpm_get_saved_rx() - get the response if it was already saved.
> * @achan: ACPM channel info.
> @@ -606,6 +636,7 @@ static void acpm_setup_ops(struct acpm_info *acpm)
>
> static int acpm_probe(struct platform_device *pdev)
> {
> + const struct acpm_clk_platform_data *acpm_clk_pdata;
> const struct acpm_match_data *match_data;
> struct device *dev = &pdev->dev;
> struct device_node *shmem;
> @@ -647,7 +678,30 @@ static int acpm_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, acpm);
>
> - return devm_of_platform_populate(dev);
> + acpm_clk_pdata = match_data->acpm_clk_pdata;
> + acpm->clk_pdev = platform_device_register_data(dev, "acpm-clocks",
> + PLATFORM_DEVID_NONE,
> + acpm_clk_pdata,
> + sizeof(*acpm_clk_pdata));
> + if (IS_ERR(acpm->clk_pdev))
> + return dev_err_probe(dev, PTR_ERR(acpm->clk_pdev),
> + "Failed to register ACPM clocks device.\n");
> +
> + ret = devm_of_platform_populate(dev);
> + if (ret) {
> + platform_device_unregister(acpm->clk_pdev);
I think this should stick to devm-interfaces everywhere, not mix them,
to have exactly expected cleanup sequence. Now your remove() first
unregisters and then de-populates, which is different order than it was
done in probe(). Use devm-action handler for device unregistering.
Best regards,
Krzysztof
On 8/31/25 11:50 AM, Krzysztof Kozlowski wrote:
> On 27/08/2025 14:42, Tudor Ambarus wrote:
>> +
>> +static const struct acpm_clk_variant gs101_acpm_clks[] = {
>> + ACPM_CLK(CLK_ACPM_DVFS_MIF, "mif"),
>> + ACPM_CLK(CLK_ACPM_DVFS_INT, "int"),
>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL0, "cpucl0"),
>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL1, "cpucl1"),
>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL2, "cpucl2"),
>> + ACPM_CLK(CLK_ACPM_DVFS_G3D, "g3d"),
>> + ACPM_CLK(CLK_ACPM_DVFS_G3DL2, "g3dl2"),
>> + ACPM_CLK(CLK_ACPM_DVFS_TPU, "tpu"),
>> + ACPM_CLK(CLK_ACPM_DVFS_INTCAM, "intcam"),
>> + ACPM_CLK(CLK_ACPM_DVFS_TNR, "tnr"),
>> + ACPM_CLK(CLK_ACPM_DVFS_CAM, "cam"),
>> + ACPM_CLK(CLK_ACPM_DVFS_MFC, "mfc"),
>> + ACPM_CLK(CLK_ACPM_DVFS_DISP, "disp"),
>> + ACPM_CLK(CLK_ACPM_DVFS_BO, "b0"),
>> +};
>
> I don't understand why clocks are defined in the firmware driver, not in
> the clock driver.
I chose to define the clocks in the firmware driver and pass them as
platform data to the clock platform device for extensibility. In case
other SoCs have different clock IDs, they'll be able to pass the
clock data without needing to modify the clock driver. GS201 defines
the same ACPM clocks as GS101, but I don't have access to other newer
SoCs to tell if the ACPM clocks differ or not.
The alternative is to define the clocks in the clock driver and
use platform_device_register_simple() to register the clock platform
device. The clock driver will be rigid in what clocks it supports.
I'm fine either way for now. What do you prefer?
>
> This creates dependency of this patch on the clock patch, so basically
> there is no way I will take it in one cycle.
Would it work to have an immutable tag for the clock and samsung-soc
subsytems to use?
>
>> +
>> /**
>> * acpm_get_saved_rx() - get the response if it was already saved.
>> * @achan: ACPM channel info.
>> @@ -606,6 +636,7 @@ static void acpm_setup_ops(struct acpm_info *acpm)
>>
>> static int acpm_probe(struct platform_device *pdev)
>> {
>> + const struct acpm_clk_platform_data *acpm_clk_pdata;
>> const struct acpm_match_data *match_data;
>> struct device *dev = &pdev->dev;
>> struct device_node *shmem;
>> @@ -647,7 +678,30 @@ static int acpm_probe(struct platform_device *pdev)
>>
>> platform_set_drvdata(pdev, acpm);
>>
>> - return devm_of_platform_populate(dev);
>> + acpm_clk_pdata = match_data->acpm_clk_pdata;
>> + acpm->clk_pdev = platform_device_register_data(dev, "acpm-clocks",
>> + PLATFORM_DEVID_NONE,
>> + acpm_clk_pdata,
>> + sizeof(*acpm_clk_pdata));
>> + if (IS_ERR(acpm->clk_pdev))
>> + return dev_err_probe(dev, PTR_ERR(acpm->clk_pdev),
>> + "Failed to register ACPM clocks device.\n");
>> +
>> + ret = devm_of_platform_populate(dev);
>> + if (ret) {
>> + platform_device_unregister(acpm->clk_pdev);
>
> I think this should stick to devm-interfaces everywhere, not mix them,
> to have exactly expected cleanup sequence. Now your remove() first
> unregisters and then de-populates, which is different order than it was
> done in probe(). Use devm-action handler for device unregistering.
>
Right, I will take a look. Thanks!
ta
On 01/09/2025 08:56, Tudor Ambarus wrote:
>
>
> On 8/31/25 11:50 AM, Krzysztof Kozlowski wrote:
>> On 27/08/2025 14:42, Tudor Ambarus wrote:
>>> +
>>> +static const struct acpm_clk_variant gs101_acpm_clks[] = {
>>> + ACPM_CLK(CLK_ACPM_DVFS_MIF, "mif"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_INT, "int"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL0, "cpucl0"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL1, "cpucl1"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL2, "cpucl2"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_G3D, "g3d"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_G3DL2, "g3dl2"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_TPU, "tpu"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_INTCAM, "intcam"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_TNR, "tnr"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_CAM, "cam"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_MFC, "mfc"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_DISP, "disp"),
>>> + ACPM_CLK(CLK_ACPM_DVFS_BO, "b0"),
>>> +};
>>
>> I don't understand why clocks are defined in the firmware driver, not in
>> the clock driver.
>
> I chose to define the clocks in the firmware driver and pass them as
> platform data to the clock platform device for extensibility. In case
> other SoCs have different clock IDs, they'll be able to pass the
You will have to modify firmware driver, so still at least one driver
has to be changed. Having clocks defined in non-clock driver is really
unusual.
This solution here creates also dependency on clock bindings and makes
merging everything unnecessary difficult.
> clock data without needing to modify the clock driver. GS201 defines
> the same ACPM clocks as GS101, but I don't have access to other newer
> SoCs to tell if the ACPM clocks differ or not.
>
> The alternative is to define the clocks in the clock driver and
> use platform_device_register_simple() to register the clock platform
> device. The clock driver will be rigid in what clocks it supports.
>
> I'm fine either way for now. What do you prefer?
Please move them to the driver.
>
>>
>> This creates dependency of this patch on the clock patch, so basically
>> there is no way I will take it in one cycle.
>
> Would it work to have an immutable tag for the clock and samsung-soc
> subsytems to use?
No, just try yourself. Patch #3 depends on patch #2, so that's the cross
tree merge. It's fine, but now patch #4 depends on patch #3, so you need
two merges.
Or how do you actually see it being merged with immutable tag? What goes
where?
Best regards,
Krzysztof
On 9/1/25 8:48 AM, Krzysztof Kozlowski wrote:
> On 01/09/2025 08:56, Tudor Ambarus wrote:
>>
>>
>> On 8/31/25 11:50 AM, Krzysztof Kozlowski wrote:
>>> On 27/08/2025 14:42, Tudor Ambarus wrote:
>>>> +
>>>> +static const struct acpm_clk_variant gs101_acpm_clks[] = {
>>>> + ACPM_CLK(CLK_ACPM_DVFS_MIF, "mif"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_INT, "int"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL0, "cpucl0"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL1, "cpucl1"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL2, "cpucl2"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_G3D, "g3d"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_G3DL2, "g3dl2"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_TPU, "tpu"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_INTCAM, "intcam"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_TNR, "tnr"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_CAM, "cam"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_MFC, "mfc"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_DISP, "disp"),
>>>> + ACPM_CLK(CLK_ACPM_DVFS_BO, "b0"),
>>>> +};
>>>
>>> I don't understand why clocks are defined in the firmware driver, not in
>>> the clock driver.
>>
>> I chose to define the clocks in the firmware driver and pass them as
>> platform data to the clock platform device for extensibility. In case
>> other SoCs have different clock IDs, they'll be able to pass the
>
> You will have to modify firmware driver, so still at least one driver
> has to be changed. Having clocks defined in non-clock driver is really
> unusual.
>
> This solution here creates also dependency on clock bindings and makes
> merging everything unnecessary difficult.
>
>> clock data without needing to modify the clock driver. GS201 defines
>> the same ACPM clocks as GS101, but I don't have access to other newer
>> SoCs to tell if the ACPM clocks differ or not.
>>
>> The alternative is to define the clocks in the clock driver and
>> use platform_device_register_simple() to register the clock platform
>> device. The clock driver will be rigid in what clocks it supports.
>>
>> I'm fine either way for now. What do you prefer?
>
> Please move them to the driver.
Okay, will move the clock definitions to the clock driver.
>
>>
>>>
>>> This creates dependency of this patch on the clock patch, so basically
>>> there is no way I will take it in one cycle.
>>
>> Would it work to have an immutable tag for the clock and samsung-soc
>> subsytems to use?
>
> No, just try yourself. Patch #3 depends on patch #2, so that's the cross
> tree merge. It's fine, but now patch #4 depends on patch #3, so you need
> two merges.
>
> Or how do you actually see it being merged with immutable tag? What goes
> where?
>
Unnecessary difficult indeed. Hypothetically, if we kept the current
structure, we could have have a single tag on #4. Since the dependency was
on a new clock driver, the clock subsystem could have lived without merging
the tag, as the chances of conflicts with the clk core are small. But not
ideal. Lesson learnt, always put yourself in the maintainer's shoes.
Thanks for the patience!
Cheers,
ta
On 01/09/2025 10:43, Tudor Ambarus wrote:
>
>
> On 9/1/25 8:48 AM, Krzysztof Kozlowski wrote:
>> On 01/09/2025 08:56, Tudor Ambarus wrote:
>>>
>>>
>>> On 8/31/25 11:50 AM, Krzysztof Kozlowski wrote:
>>>> On 27/08/2025 14:42, Tudor Ambarus wrote:
>>>>> +
>>>>> +static const struct acpm_clk_variant gs101_acpm_clks[] = {
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_MIF, "mif"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_INT, "int"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL0, "cpucl0"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL1, "cpucl1"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_CPUCL2, "cpucl2"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_G3D, "g3d"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_G3DL2, "g3dl2"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_TPU, "tpu"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_INTCAM, "intcam"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_TNR, "tnr"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_CAM, "cam"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_MFC, "mfc"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_DISP, "disp"),
>>>>> + ACPM_CLK(CLK_ACPM_DVFS_BO, "b0"),
>>>>> +};
>>>>
>>>> I don't understand why clocks are defined in the firmware driver, not in
>>>> the clock driver.
>>>
>>> I chose to define the clocks in the firmware driver and pass them as
>>> platform data to the clock platform device for extensibility. In case
>>> other SoCs have different clock IDs, they'll be able to pass the
>>
>> You will have to modify firmware driver, so still at least one driver
>> has to be changed. Having clocks defined in non-clock driver is really
>> unusual.
>>
>> This solution here creates also dependency on clock bindings and makes
>> merging everything unnecessary difficult.
>>
>>> clock data without needing to modify the clock driver. GS201 defines
>>> the same ACPM clocks as GS101, but I don't have access to other newer
>>> SoCs to tell if the ACPM clocks differ or not.
>>>
>>> The alternative is to define the clocks in the clock driver and
>>> use platform_device_register_simple() to register the clock platform
>>> device. The clock driver will be rigid in what clocks it supports.
>>>
>>> I'm fine either way for now. What do you prefer?
>>
>> Please move them to the driver.
>
> Okay, will move the clock definitions to the clock driver.
>
>>
>>>
>>>>
>>>> This creates dependency of this patch on the clock patch, so basically
>>>> there is no way I will take it in one cycle.
>>>
>>> Would it work to have an immutable tag for the clock and samsung-soc
>>> subsytems to use?
>>
>> No, just try yourself. Patch #3 depends on patch #2, so that's the cross
>> tree merge. It's fine, but now patch #4 depends on patch #3, so you need
>> two merges.
>>
>> Or how do you actually see it being merged with immutable tag? What goes
>> where?
>>
>
> Unnecessary difficult indeed. Hypothetically, if we kept the current
No, it is impossible.
> structure, we could have have a single tag on #4. Since the dependency was
What does it mean tag on #4? There are no further users, so tagging this
patch has zero effect.
> on a new clock driver, the clock subsystem could have lived without merging
> the tag, as the chances of conflicts with the clk core are small. But not
Quick look tells me nothing would compile. Really, try yourself. Neither
patch #3 nor patch #4 builds!
Best regards,
Krzysztof
© 2016 - 2026 Red Hat, Inc.