[PATCH v8 3/6] clk: spacemit: set up reset auxiliary devices
Philipp Zabel
p.zabel at pengutronix.de
Mon May 12 08:46:48 PDT 2025
On Mo, 2025-05-12 at 10:34 -0500, Alex Elder wrote:
> On 5/12/25 8:54 AM, Yixun Lan wrote:
> > On 06:20 Fri 09 May , 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 in 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 at riscstar.com>
> > > ---
> > > v8: Allocate the auxiliary device using kzalloc(), not devm_kzalloc()
> > >
> > > drivers/clk/spacemit/Kconfig | 1 +
> > > drivers/clk/spacemit/ccu-k1.c | 90 ++++++++++++++++++++++++++++----
> > > include/soc/spacemit/k1-syscon.h | 12 +++++
> > > 3 files changed, 93 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/clk/spacemit/Kconfig b/drivers/clk/spacemit/Kconfig
> > > index 4c4df845b3cb2..3854f6ae6d0ea 100644
> > > --- a/drivers/clk/spacemit/Kconfig
> > > +++ b/drivers/clk/spacemit/Kconfig
> > > @@ -3,6 +3,7 @@
> > > config SPACEMIT_CCU
> > > tristate "Clock support for SpacemiT SoCs"
> > > depends on ARCH_SPACEMIT || COMPILE_TEST
> > > + select AUXILIARY_BUS
> > > select MFD_SYSCON
> > > help
> > > Say Y to enable clock controller unit support for SpacemiT SoCs.
> > > diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> > > index 801150f4ff0f5..551df9d076859 100644
> > > --- a/drivers/clk/spacemit/ccu-k1.c
> > > +++ b/drivers/clk/spacemit/ccu-k1.c
> > > @@ -5,12 +5,14 @@
> > > */
> > >
> > > #include <linux/array_size.h>
> > > +#include <linux/auxiliary_bus.h>
> > > #include <linux/clk-provider.h>
> > > #include <linux/delay.h>
> > > #include <linux/mfd/syscon.h>
> > > #include <linux/minmax.h>
> > > #include <linux/module.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > #include <soc/spacemit/k1-syscon.h>
> > >
> > > #include "ccu_common.h"
> > > @@ -21,6 +23,7 @@
> > > #include <dt-bindings/clock/spacemit,k1-syscon.h>
> > >
> > > struct spacemit_ccu_data {
> > > + const char *reset_name;
> > see my comment below..
> >
> > > struct clk_hw **hws;
> > > size_t num;
> > > };
> > > @@ -710,8 +713,9 @@ static struct clk_hw *k1_ccu_pll_hws[] = {
> > > };
> > >
> > > static const struct spacemit_ccu_data k1_ccu_pll_data = {
> > > - .hws = k1_ccu_pll_hws,
> > > - .num = ARRAY_SIZE(k1_ccu_pll_hws),
> > > + /* The PLL CCU implements no resets */
> > > + .hws = k1_ccu_pll_hws,
> > > + .num = ARRAY_SIZE(k1_ccu_pll_hws),
> > > };
> > >
> > > static struct clk_hw *k1_ccu_mpmu_hws[] = {
> > > @@ -751,8 +755,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 +864,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 +935,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 +948,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 +992,63 @@ static int spacemit_ccu_register(struct device *dev,
> > > return ret;
> > > }
> > >
> > > +static void spacemit_cadev_release(struct device *dev)
> > why this function define as _cadev_ prefix, while below as _adev_
> > is it a typo? or c short for ccu, I just feel it isn't consistent..
>
> It is not a typo. Yes, it was intended to represent CCU
> Auxiliary device, while "adev" represents just Auxiliary
> Device. It is releasing (freeing) a spacemit_ccu_adev
> structure.
>
> > > +{
> > > + struct auxiliary_device *adev = to_auxiliary_dev(dev);
> > > +
> > > + kfree(to_spacemit_ccu_adev(adev));
> > > +}
> > > +
>
> This function is operating on an auxiliary_device structure,
> so "adev" is used in its name.
>
> > > +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 = kzalloc(sizeof(*cadev), GFP_KERNEL);
> > > + if (!cadev)
> > > + return -ENOMEM;
> > add one blank line here?
>
> If I do a new version that's easy but this was intentional.
>
> > > + 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++;
> > so I'd assume the underlying device doesn't really care the id?
> > but with different order of registration, it will result random id for the device
>
> These things are identified in DTS files by their index values
> defined in "spacemit,k1-syscon.h". If there is a need for the
> assigned device ID to be consistent, I'm not aware of it. Can
> you think of one? I think all that matters is that they're
> unique, and this ensures that (for up to 2^32 PMICs).
If there are multiple reset controllers and the driver can be unbound,
it's trivial to provoke a collision by keeping one device bound and
unbinding/binding the second one until next_id wraps.
This could be fixed by using ida_alloc/free to manage the id.
regards
Philipp
More information about the linux-riscv
mailing list