Re: [PATCH v8 3/6] clk: spacemit: set up reset auxiliary devices

[Philipp Zabel <p.zabel@pengutronix.de>]

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@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