Add a new reset_name field to the spacemit_ccu_data structure. If it is
non-null, the CCU implements a reset controller, and the name will be
used as the name for the auxiliary device that implements it.
Define a new type to hold an auxiliary device as well as the regmap
pointer that will be needed by CCU reset controllers. Set up code to
initialize and add an auxiliary device for any CCU that implements reset
functionality.
Make it optional for a CCU to implement a clock controller. This
doesn't apply to any of the existing CCUs but will for some new ones
that will be added soon.
Signed-off-by: Alex Elder <elder@riscstar.com>
---
drivers/clk/spacemit/ccu-k1.c | 85 +++++++++++++++++++++++++++++++----
include/soc/spacemit/ccu_k1.h | 12 +++++
2 files changed, 89 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
index 9545cfe60b92b..6b1845e899e5f 100644
--- a/drivers/clk/spacemit/ccu-k1.c
+++ b/drivers/clk/spacemit/ccu-k1.c
@@ -5,6 +5,7 @@
*/
#include <linux/array_size.h>
+#include <linux/auxiliary_bus.h>
#include <linux/clk-provider.h>
#include <linux/delay.h>
#include <linux/mfd/syscon.h>
@@ -21,6 +22,7 @@
#include <dt-bindings/clock/spacemit,k1-syscon.h>
struct spacemit_ccu_data {
+ const char *reset_name;
struct clk_hw **hws;
size_t num;
};
@@ -710,6 +712,7 @@ static struct clk_hw *k1_ccu_pll_hws[] = {
};
static const struct spacemit_ccu_data k1_ccu_pll_data = {
+ /* The PLL CCU implements no resets */
.hws = k1_ccu_pll_hws,
.num = ARRAY_SIZE(k1_ccu_pll_hws),
};
@@ -751,8 +754,9 @@ static struct clk_hw *k1_ccu_mpmu_hws[] = {
};
static const struct spacemit_ccu_data k1_ccu_mpmu_data = {
- .hws = k1_ccu_mpmu_hws,
- .num = ARRAY_SIZE(k1_ccu_mpmu_hws),
+ .reset_name = "mpmu-reset",
+ .hws = k1_ccu_mpmu_hws,
+ .num = ARRAY_SIZE(k1_ccu_mpmu_hws),
};
static struct clk_hw *k1_ccu_apbc_hws[] = {
@@ -859,8 +863,9 @@ static struct clk_hw *k1_ccu_apbc_hws[] = {
};
static const struct spacemit_ccu_data k1_ccu_apbc_data = {
- .hws = k1_ccu_apbc_hws,
- .num = ARRAY_SIZE(k1_ccu_apbc_hws),
+ .reset_name = "apbc-reset",
+ .hws = k1_ccu_apbc_hws,
+ .num = ARRAY_SIZE(k1_ccu_apbc_hws),
};
static struct clk_hw *k1_ccu_apmu_hws[] = {
@@ -929,8 +934,9 @@ static struct clk_hw *k1_ccu_apmu_hws[] = {
};
static const struct spacemit_ccu_data k1_ccu_apmu_data = {
- .hws = k1_ccu_apmu_hws,
- .num = ARRAY_SIZE(k1_ccu_apmu_hws),
+ .reset_name = "apmu-reset",
+ .hws = k1_ccu_apmu_hws,
+ .num = ARRAY_SIZE(k1_ccu_apmu_hws),
};
static int spacemit_ccu_register(struct device *dev,
@@ -941,6 +947,10 @@ static int spacemit_ccu_register(struct device *dev,
struct clk_hw_onecell_data *clk_data;
int i, ret;
+ /* Nothing to do if the CCU does not implement any clocks */
+ if (!data->hws)
+ return 0;
+
clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, data->num),
GFP_KERNEL);
if (!clk_data)
@@ -981,9 +991,63 @@ static int spacemit_ccu_register(struct device *dev,
return ret;
}
+static void spacemit_cadev_release(struct device *dev)
+{
+ struct auxiliary_device *adev = to_auxiliary_dev(dev);
+
+ kfree(to_spacemit_ccu_adev(adev));
+}
+
+static void spacemit_adev_unregister(void *data)
+{
+ struct auxiliary_device *adev = data;
+
+ auxiliary_device_delete(adev);
+ auxiliary_device_uninit(adev);
+}
+
+static int spacemit_ccu_reset_register(struct device *dev,
+ struct regmap *regmap,
+ const char *reset_name)
+{
+ struct spacemit_ccu_adev *cadev;
+ struct auxiliary_device *adev;
+ static u32 next_id;
+ int ret;
+
+ /* Nothing to do if the CCU does not implement a reset controller */
+ if (!reset_name)
+ return 0;
+
+ cadev = devm_kzalloc(dev, sizeof(*cadev), GFP_KERNEL);
+ if (!cadev)
+ return -ENOMEM;
+ cadev->regmap = regmap;
+
+ adev = &cadev->adev;
+ adev->name = reset_name;
+ adev->dev.parent = dev;
+ adev->dev.release = spacemit_cadev_release;
+ adev->dev.of_node = dev->of_node;
+ adev->id = next_id++;
+
+ ret = auxiliary_device_init(adev);
+ if (ret)
+ return ret;
+
+ ret = auxiliary_device_add(adev);
+ if (ret) {
+ auxiliary_device_uninit(adev);
+ return ret;
+ }
+
+ return devm_add_action_or_reset(dev, spacemit_adev_unregister, adev);
+}
+
static int k1_ccu_probe(struct platform_device *pdev)
{
struct regmap *base_regmap, *lock_regmap = NULL;
+ const struct spacemit_ccu_data *data;
struct device *dev = &pdev->dev;
int ret;
@@ -1012,11 +1076,16 @@ static int k1_ccu_probe(struct platform_device *pdev)
"failed to get lock regmap\n");
}
- ret = spacemit_ccu_register(dev, base_regmap, lock_regmap,
- of_device_get_match_data(dev));
+ data = of_device_get_match_data(dev);
+
+ ret = spacemit_ccu_register(dev, base_regmap, lock_regmap, data);
if (ret)
return dev_err_probe(dev, ret, "failed to register clocks\n");
+ ret = spacemit_ccu_reset_register(dev, base_regmap, data->reset_name);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to register resets\n");
+
return 0;
}
diff --git a/include/soc/spacemit/ccu_k1.h b/include/soc/spacemit/ccu_k1.h
index 7df75043e78af..8b2581fb3055d 100644
--- a/include/soc/spacemit/ccu_k1.h
+++ b/include/soc/spacemit/ccu_k1.h
@@ -2,6 +2,18 @@
/* SpacemiT clock and reset driver definitions for the K1 SoC */
+/* Auxiliary device used to represent a CCU reset controller */
+struct spacemit_ccu_adev {
+ struct auxiliary_device adev;
+ struct regmap *regmap;
+};
+
+static inline struct spacemit_ccu_adev *
+to_spacemit_ccu_adev(struct auxiliary_device *adev)
+{
+ return container_of(adev, struct spacemit_ccu_adev, adev);
+}
+
/* APBS register offset */
#define APBS_PLL1_SWCR1 0x100
#define APBS_PLL1_SWCR2 0x104
--
2.45.2On Tue, May 06, 2025 at 04:06:34PM -0500, Alex Elder wrote:
> Add a new reset_name field to the spacemit_ccu_data structure. If it is
> non-null, the CCU implements a reset controller, and the name will be
> used as the name for the auxiliary device that implements it.
>
> Define a new type to hold an auxiliary device as well as the regmap
> pointer that will be needed by CCU reset controllers. Set up code to
> initialize and add an auxiliary device for any CCU that implements reset
> functionality.
>
> Make it optional for a CCU to implement a clock controller. This
> doesn't apply to any of the existing CCUs but will for some new ones
> that will be added soon.
>
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> drivers/clk/spacemit/ccu-k1.c | 85 +++++++++++++++++++++++++++++++----
> include/soc/spacemit/ccu_k1.h | 12 +++++
> 2 files changed, 89 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> index 9545cfe60b92b..6b1845e899e5f 100644
> --- a/drivers/clk/spacemit/ccu-k1.c
> +++ b/drivers/clk/spacemit/ccu-k1.c
...
> +static void spacemit_cadev_release(struct device *dev)
> +{
> + struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +
> + kfree(to_spacemit_ccu_adev(adev));
> +}
spacemit_ccu_adev structures are allocated with devm_kzalloc() in
spacemit_ccu_reset_register(), which means its lifetime is bound to the
driver and it'll be automatically released after driver removal; won't
there be a possibility of double-free? I think the release callback
could be simply dropped.
...
> +static int spacemit_ccu_reset_register(struct device *dev,
> + struct regmap *regmap,
> + const char *reset_name)
> +{
> + struct spacemit_ccu_adev *cadev;
> + struct auxiliary_device *adev;
> + static u32 next_id;
> + int ret;
> +
> + /* Nothing to do if the CCU does not implement a reset controller */
> + if (!reset_name)
> + return 0;
> +
> + cadev = devm_kzalloc(dev, sizeof(*cadev), GFP_KERNEL);
Here spacemit_ccu_adev is allocated.
> + if (!cadev)
> + return -ENOMEM;
> + cadev->regmap = regmap;
> +
> + adev = &cadev->adev;
> + adev->name = reset_name;
> + adev->dev.parent = dev;
> + adev->dev.release = spacemit_cadev_release;
> + adev->dev.of_node = dev->of_node;
> + adev->id = next_id++;
> +
> + ret = auxiliary_device_init(adev);
> + if (ret)
> + return ret;
> +
> + ret = auxiliary_device_add(adev);
> + if (ret) {
> + auxiliary_device_uninit(adev);
> + return ret;
> + }
> +
> + return devm_add_action_or_reset(dev, spacemit_adev_unregister, adev);
> +}
> +
Best regards,
Haylen Chu
On 5/7/25 11:46 PM, Haylen Chu wrote:
> On Tue, May 06, 2025 at 04:06:34PM -0500, Alex Elder wrote:
>> Add a new reset_name field to the spacemit_ccu_data structure. If it is
>> non-null, the CCU implements a reset controller, and the name will be
>> used as the name for the auxiliary device that implements it.
>>
>> Define a new type to hold an auxiliary device as well as the regmap
>> pointer that will be needed by CCU reset controllers. Set up code to
>> initialize and add an auxiliary device for any CCU that implements reset
>> functionality.
>>
>> Make it optional for a CCU to implement a clock controller. This
>> doesn't apply to any of the existing CCUs but will for some new ones
>> that will be added soon.
>>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>> drivers/clk/spacemit/ccu-k1.c | 85 +++++++++++++++++++++++++++++++----
>> include/soc/spacemit/ccu_k1.h | 12 +++++
>> 2 files changed, 89 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
>> index 9545cfe60b92b..6b1845e899e5f 100644
>> --- a/drivers/clk/spacemit/ccu-k1.c
>> +++ b/drivers/clk/spacemit/ccu-k1.c
>
> ...
>
>> +static void spacemit_cadev_release(struct device *dev)
>> +{
>> + struct auxiliary_device *adev = to_auxiliary_dev(dev);
>> +
>> + kfree(to_spacemit_ccu_adev(adev));
>> +}
>
> spacemit_ccu_adev structures are allocated with devm_kzalloc() in
> spacemit_ccu_reset_register(), which means its lifetime is bound to the
> driver and it'll be automatically released after driver removal; won't
> there be a possibility of double-free? I think the release callback
> could be simply dropped.
You are correct. And unfortunately I didn't include the fix
for this in the patches I just posted, because somehow this
message was not included with the group in my mail program.
I'm going to send v8 after I fix this and verify it again.
-Alex
> ...
>
>> +static int spacemit_ccu_reset_register(struct device *dev,
>> + struct regmap *regmap,
>> + const char *reset_name)
>> +{
>> + struct spacemit_ccu_adev *cadev;
>> + struct auxiliary_device *adev;
>> + static u32 next_id;
>> + int ret;
>> +
>> + /* Nothing to do if the CCU does not implement a reset controller */
>> + if (!reset_name)
>> + return 0;
>> +
>> + cadev = devm_kzalloc(dev, sizeof(*cadev), GFP_KERNEL);
>
> Here spacemit_ccu_adev is allocated.
>
>> + if (!cadev)
>> + return -ENOMEM;
>> + cadev->regmap = regmap;
>> +
>> + adev = &cadev->adev;
>> + adev->name = reset_name;
>> + adev->dev.parent = dev;
>> + adev->dev.release = spacemit_cadev_release;
>> + adev->dev.of_node = dev->of_node;
>> + adev->id = next_id++;
>> +
>> + ret = auxiliary_device_init(adev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = auxiliary_device_add(adev);
>> + if (ret) {
>> + auxiliary_device_uninit(adev);
>> + return ret;
>> + }
>> +
>> + return devm_add_action_or_reset(dev, spacemit_adev_unregister, adev);
>> +}
>> +
>
> Best regards,
> Haylen Chu
On 5/8/25 3:04 PM, Alex Elder wrote:
> On 5/7/25 11:46 PM, Haylen Chu wrote:
>> On Tue, May 06, 2025 at 04:06:34PM -0500, Alex Elder wrote:
>>> Add a new reset_name field to the spacemit_ccu_data structure. If it is
>>> non-null, the CCU implements a reset controller, and the name will be
>>> used as the name for the auxiliary device that implements it.
>>>
>>> Define a new type to hold an auxiliary device as well as the regmap
>>> pointer that will be needed by CCU reset controllers. Set up code to
>>> initialize and add an auxiliary device for any CCU that implements reset
>>> functionality.
>>>
>>> Make it optional for a CCU to implement a clock controller. This
>>> doesn't apply to any of the existing CCUs but will for some new ones
>>> that will be added soon.
>>>
>>> Signed-off-by: Alex Elder <elder@riscstar.com>
>>> ---
>>> drivers/clk/spacemit/ccu-k1.c | 85 +++++++++++++++++++++++++++++++----
>>> include/soc/spacemit/ccu_k1.h | 12 +++++
>>> 2 files changed, 89 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/
>>> ccu-k1.c
>>> index 9545cfe60b92b..6b1845e899e5f 100644
>>> --- a/drivers/clk/spacemit/ccu-k1.c
>>> +++ b/drivers/clk/spacemit/ccu-k1.c
>>
>> ...
>>
>>> +static void spacemit_cadev_release(struct device *dev)
>>> +{
>>> + struct auxiliary_device *adev = to_auxiliary_dev(dev);
>>> +
>>> + kfree(to_spacemit_ccu_adev(adev));
>>> +}
>>
>> spacemit_ccu_adev structures are allocated with devm_kzalloc() in
>> spacemit_ccu_reset_register(), which means its lifetime is bound to the
>> driver and it'll be automatically released after driver removal; won't
>> there be a possibility of double-free? I think the release callback
>> could be simply dropped.
>
> You are correct. And unfortunately I didn't include the fix
> for this in the patches I just posted, because somehow this
> message was not included with the group in my mail program.
>
> I'm going to send v8 after I fix this and verify it again.
To be clear, the fix is to use kzalloc(), rather than calling
devm_kzalloc() with the parent device as first argument.
I'll also include <linux/slab.h> to avoid the warning
reported by the kernel test robot.
-Alex
>
> -Alex
>
>
>> ...
>>
>>> +static int spacemit_ccu_reset_register(struct device *dev,
>>> + struct regmap *regmap,
>>> + const char *reset_name)
>>> +{
>>> + struct spacemit_ccu_adev *cadev;
>>> + struct auxiliary_device *adev;
>>> + static u32 next_id;
>>> + int ret;
>>> +
>>> + /* Nothing to do if the CCU does not implement a reset
>>> controller */
>>> + if (!reset_name)
>>> + return 0;
>>> +
>>> + cadev = devm_kzalloc(dev, sizeof(*cadev), GFP_KERNEL);
>>
>> Here spacemit_ccu_adev is allocated.
>>
>>> + if (!cadev)
>>> + return -ENOMEM;
>>> + cadev->regmap = regmap;
>>> +
>>> + adev = &cadev->adev;
>>> + adev->name = reset_name;
>>> + adev->dev.parent = dev;
>>> + adev->dev.release = spacemit_cadev_release;
>>> + adev->dev.of_node = dev->of_node;
>>> + adev->id = next_id++;
>>> +
>>> + ret = auxiliary_device_init(adev);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = auxiliary_device_add(adev);
>>> + if (ret) {
>>> + auxiliary_device_uninit(adev);
>>> + return ret;
>>> + }
>>> +
>>> + return devm_add_action_or_reset(dev, spacemit_adev_unregister,
>>> adev);
>>> +}
>>> +
>>
>> Best regards,
>> Haylen Chu
>
Hi Alex,
kernel test robot noticed the following build errors:
[auto build test ERROR on cb9c3aeae509b36afbdf46942a7a0a0dfc856ce7]
url: https://github.com/intel-lab-lkp/linux/commits/Alex-Elder/dt-bindings-soc-spacemit-define-spacemit-k1-ccu-resets/20250507-051019
base: cb9c3aeae509b36afbdf46942a7a0a0dfc856ce7
patch link: https://lore.kernel.org/r/20250506210638.2800228-4-elder%40riscstar.com
patch subject: [PATCH v6 3/6] clk: spacemit: set up reset auxiliary devices
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20250508/202505081113.CwKtDYbs-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250508/202505081113.CwKtDYbs-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505081113.CwKtDYbs-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/clk/spacemit/ccu-k1.c:998:2: error: call to undeclared function 'kfree'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
998 | kfree(to_spacemit_ccu_adev(adev));
| ^
1 error generated.
vim +/kfree +998 drivers/clk/spacemit/ccu-k1.c
993
994 static void spacemit_cadev_release(struct device *dev)
995 {
996 struct auxiliary_device *adev = to_auxiliary_dev(dev);
997
> 998 kfree(to_spacemit_ccu_adev(adev));
999 }
1000
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2025 Red Hat, Inc.