diff mbox series

[7/8] reset: amlogic: add auxiliary reset driver support

Message ID 20240710162526.2341399-8-jbrunet@baylibre.com (mailing list archive)
State Changes Requested, archived
Headers show
Series reset: amlogic: move audio reset drivers out of CCF | expand

Commit Message

Jerome Brunet July 10, 2024, 4:25 p.m. UTC
Add support for the reset controller present in the audio clock
controller of the g12 and sm1 SoC families, using the auxiliary bus.

This is expected to replace the driver currently present directly
within the related clock driver.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/reset/Kconfig                       |   1 +
 drivers/reset/reset-meson.c                 | 121 +++++++++++++++++++-
 include/soc/amlogic/meson-auxiliary-reset.h |  23 ++++
 3 files changed, 144 insertions(+), 1 deletion(-)
 create mode 100644 include/soc/amlogic/meson-auxiliary-reset.h

Comments

Stephen Boyd July 10, 2024, 10:49 p.m. UTC | #1
Quoting Jerome Brunet (2024-07-10 09:25:16)
> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> index e34a10b15593..5cc767d50e8f 100644
> --- a/drivers/reset/reset-meson.c
> +++ b/drivers/reset/reset-meson.c
[...]
> +
> +int devm_meson_rst_aux_register(struct device *dev,
> +                               struct regmap *map,
> +                               const char *adev_name)
> +{
> +       struct meson_reset_adev *raux;
> +       struct auxiliary_device *adev;
> +       int ret;
> +
> +       raux = kzalloc(sizeof(*raux), GFP_KERNEL);
> +       if (!raux)
> +               return -ENOMEM;
> +
> +       ret = ida_alloc(&meson_rst_aux_ida, GFP_KERNEL);

Do we expect more than one device with the same name? I wonder if the
IDA can be skipped.

> +       if (ret < 0)
> +               goto raux_free;
> +
> +       raux->map = map;
> +
> +       adev = &raux->adev;
> +       adev->id = ret;
> +       adev->name = adev_name;
> +       adev->dev.parent = dev;
> +       adev->dev.release = meson_rst_aux_release;
> +       device_set_of_node_from_dev(&adev->dev, dev);
> +
> +       ret = auxiliary_device_init(adev);
> +       if (ret)
> +               goto ida_free;
> +
> +       ret = __auxiliary_device_add(adev, dev->driver->name);
> +       if (ret) {
> +               auxiliary_device_uninit(adev);
> +               return ret;
> +       }
> +
> +       return devm_add_action_or_reset(dev, meson_rst_aux_unregister_adev,
> +                                       adev);
> +
> +ida_free:
> +       ida_free(&meson_rst_aux_ida, adev->id);
> +raux_free:
> +       kfree(raux);
> +       return ret;
> +

Nitpick: Drop extra newline?

> +}
> +EXPORT_SYMBOL_GPL(devm_meson_rst_aux_register);
> +
> +MODULE_DESCRIPTION("Amlogic Meson Reset driver");
>  MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
> +MODULE_AUTHOR("Jerome Brunet <jbrunet@baylibre.com>");
>  MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/include/soc/amlogic/meson-auxiliary-reset.h b/include/soc/amlogic/meson-auxiliary-reset.h
> new file mode 100644
> index 000000000000..8fdb02b18d8c
> --- /dev/null
> +++ b/include/soc/amlogic/meson-auxiliary-reset.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SOC_AMLOGIC_MESON_AUX_RESET_H
> +#define __SOC_AMLOGIC_MESON_AUX_RESET_H
> +
> +#include <linux/err.h>
> +
> +struct device;
> +struct regmap;
> +
> +#ifdef CONFIG_RESET_MESON
> +int devm_meson_rst_aux_register(struct device *dev,
> +                               struct regmap *map,
> +                               const char *adev_name);
> +#else
> +static inline int devm_meson_rst_aux_register(struct device *dev,
> +                                             struct regmap *map,
> +                                             const char *adev_name)
> +{
> +       return -EOPNOTSUPP;

Shouldn't this be 'return 0' so that the clk driver doesn't have to care
about the config?
Jerome Brunet July 11, 2024, 9:01 a.m. UTC | #2
On Wed 10 Jul 2024 at 15:49, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Jerome Brunet (2024-07-10 09:25:16)
>> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
>> index e34a10b15593..5cc767d50e8f 100644
>> --- a/drivers/reset/reset-meson.c
>> +++ b/drivers/reset/reset-meson.c
> [...]
>> +
>> +int devm_meson_rst_aux_register(struct device *dev,
>> +                               struct regmap *map,
>> +                               const char *adev_name)
>> +{
>> +       struct meson_reset_adev *raux;
>> +       struct auxiliary_device *adev;
>> +       int ret;
>> +
>> +       raux = kzalloc(sizeof(*raux), GFP_KERNEL);
>> +       if (!raux)
>> +               return -ENOMEM;
>> +
>> +       ret = ida_alloc(&meson_rst_aux_ida, GFP_KERNEL);
>
> Do we expect more than one device with the same name? I wonder if the
> IDA can be skipped.

I've wondered about that too.

I don't think it is the case right now but I'm not 100% sure.
Since I spent time thinking about it, I thought it would just be safer (and
relatively cheap) to put in and enough annoying debugging the
expectation does not hold true.

I don't have a strong opinion on this. What do you prefer ?

>
>> +       if (ret < 0)
>> +               goto raux_free;
>> +
>> +       raux->map = map;
>> +
>> +       adev = &raux->adev;
>> +       adev->id = ret;
>> +       adev->name = adev_name;
>> +       adev->dev.parent = dev;
>> +       adev->dev.release = meson_rst_aux_release;
>> +       device_set_of_node_from_dev(&adev->dev, dev);
>> +
>> +       ret = auxiliary_device_init(adev);
>> +       if (ret)
>> +               goto ida_free;
>> +
>> +       ret = __auxiliary_device_add(adev, dev->driver->name);
>> +       if (ret) {
>> +               auxiliary_device_uninit(adev);
>> +               return ret;
>> +       }
>> +
>> +       return devm_add_action_or_reset(dev, meson_rst_aux_unregister_adev,
>> +                                       adev);
>> +
>> +ida_free:
>> +       ida_free(&meson_rst_aux_ida, adev->id);
>> +raux_free:
>> +       kfree(raux);
>> +       return ret;
>> +
>
> Nitpick: Drop extra newline?
>
>> +}
>> +EXPORT_SYMBOL_GPL(devm_meson_rst_aux_register);
>> +
>> +MODULE_DESCRIPTION("Amlogic Meson Reset driver");
>>  MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
>> +MODULE_AUTHOR("Jerome Brunet <jbrunet@baylibre.com>");
>>  MODULE_LICENSE("Dual BSD/GPL");
>> diff --git a/include/soc/amlogic/meson-auxiliary-reset.h b/include/soc/amlogic/meson-auxiliary-reset.h
>> new file mode 100644
>> index 000000000000..8fdb02b18d8c
>> --- /dev/null
>> +++ b/include/soc/amlogic/meson-auxiliary-reset.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __SOC_AMLOGIC_MESON_AUX_RESET_H
>> +#define __SOC_AMLOGIC_MESON_AUX_RESET_H
>> +
>> +#include <linux/err.h>
>> +
>> +struct device;
>> +struct regmap;
>> +
>> +#ifdef CONFIG_RESET_MESON
>> +int devm_meson_rst_aux_register(struct device *dev,
>> +                               struct regmap *map,
>> +                               const char *adev_name);
>> +#else
>> +static inline int devm_meson_rst_aux_register(struct device *dev,
>> +                                             struct regmap *map,
>> +                                             const char *adev_name)
>> +{
>> +       return -EOPNOTSUPP;
>
> Shouldn't this be 'return 0' so that the clk driver doesn't have to care
> about the config?

I don't think the system (in general) would be able function without the reset
driver, so the question is rather phylosophical.

Let's say it could, if this returns 0, consumers of the reset controller
will defer indefinitely ... which is always a bit more difficult to sort
out.

If it returns an error, the problem is pretty obvious, helping people
solve it quickly.

Also the actual device we trying to register provides clocks and reset.
It is not like the reset is an optional part we don't care about.

On this instance it starts from clock, but it could have been the other
way around. Both are equally important.

I'd prefer if it returns an error when the registration can't even start.
kernel test robot July 11, 2024, 1:02 p.m. UTC | #3
Hi Jerome,

kernel test robot noticed the following build errors:

[auto build test ERROR on pza/reset/next]
[also build test ERROR on clk/clk-next linus/master v6.10-rc7 next-20240711]
[cannot apply to pza/imx-drm/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jerome-Brunet/reset-amlogic-convert-driver-to-regmap/20240711-055833
base:   https://git.pengutronix.de/git/pza/linux reset/next
patch link:    https://lore.kernel.org/r/20240710162526.2341399-8-jbrunet%40baylibre.com
patch subject: [PATCH 7/8] reset: amlogic: add auxiliary reset driver support
config: i386-buildonly-randconfig-005-20240711 (https://download.01.org/0day-ci/archive/20240711/202407112023.ixKkILn7-lkp@intel.com/config)
compiler: gcc-10 (Ubuntu 10.5.0-1ubuntu1) 10.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240711/202407112023.ixKkILn7-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/202407112023.ixKkILn7-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/device/driver.h:21,
                    from include/linux/device.h:32,
                    from include/linux/auxiliary_bus.h:11,
                    from drivers/reset/reset-meson.c:8:
   include/linux/module.h:131:42: error: redefinition of '__inittest'
     131 |  static inline initcall_t __maybe_unused __inittest(void)  \
         |                                          ^~~~~~~~~~
   include/linux/device/driver.h:262:1: note: in expansion of macro 'module_init'
     262 | module_init(__driver##_init); \
         | ^~~~~~~~~~~
   include/linux/auxiliary_bus.h:245:2: note: in expansion of macro 'module_driver'
     245 |  module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |  ^~~~~~~~~~~~~
   drivers/reset/reset-meson.c:272:1: note: in expansion of macro 'module_auxiliary_driver'
     272 | module_auxiliary_driver(meson_reset_aux_driver);
         | ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/module.h:131:42: note: previous definition of '__inittest' was here
     131 |  static inline initcall_t __maybe_unused __inittest(void)  \
         |                                          ^~~~~~~~~~
   include/linux/device/driver.h:262:1: note: in expansion of macro 'module_init'
     262 | module_init(__driver##_init); \
         | ^~~~~~~~~~~
   include/linux/platform_device.h:303:2: note: in expansion of macro 'module_driver'
     303 |  module_driver(__platform_driver, platform_driver_register, \
         |  ^~~~~~~~~~~~~
   drivers/reset/reset-meson.c:232:1: note: in expansion of macro 'module_platform_driver'
     232 | module_platform_driver(meson_reset_pltf_driver);
         | ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/module.h:133:6: error: redefinition of 'init_module'
     133 |  int init_module(void) __copy(initfn)   \
         |      ^~~~~~~~~~~
   include/linux/device/driver.h:262:1: note: in expansion of macro 'module_init'
     262 | module_init(__driver##_init); \
         | ^~~~~~~~~~~
   include/linux/auxiliary_bus.h:245:2: note: in expansion of macro 'module_driver'
     245 |  module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |  ^~~~~~~~~~~~~
   drivers/reset/reset-meson.c:272:1: note: in expansion of macro 'module_auxiliary_driver'
     272 | module_auxiliary_driver(meson_reset_aux_driver);
         | ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/module.h:133:6: note: previous definition of 'init_module' was here
     133 |  int init_module(void) __copy(initfn)   \
         |      ^~~~~~~~~~~
   include/linux/device/driver.h:262:1: note: in expansion of macro 'module_init'
     262 | module_init(__driver##_init); \
         | ^~~~~~~~~~~
   include/linux/platform_device.h:303:2: note: in expansion of macro 'module_driver'
     303 |  module_driver(__platform_driver, platform_driver_register, \
         |  ^~~~~~~~~~~~~
   drivers/reset/reset-meson.c:232:1: note: in expansion of macro 'module_platform_driver'
     232 | module_platform_driver(meson_reset_pltf_driver);
         | ^~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:139:42: error: redefinition of '__exittest'
     139 |  static inline exitcall_t __maybe_unused __exittest(void)  \
         |                                          ^~~~~~~~~~
   include/linux/device/driver.h:267:1: note: in expansion of macro 'module_exit'
     267 | module_exit(__driver##_exit);
         | ^~~~~~~~~~~
   include/linux/auxiliary_bus.h:245:2: note: in expansion of macro 'module_driver'
     245 |  module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |  ^~~~~~~~~~~~~
   drivers/reset/reset-meson.c:272:1: note: in expansion of macro 'module_auxiliary_driver'
     272 | module_auxiliary_driver(meson_reset_aux_driver);
         | ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/module.h:139:42: note: previous definition of '__exittest' was here
     139 |  static inline exitcall_t __maybe_unused __exittest(void)  \
         |                                          ^~~~~~~~~~
   include/linux/device/driver.h:267:1: note: in expansion of macro 'module_exit'
     267 | module_exit(__driver##_exit);
         | ^~~~~~~~~~~
   include/linux/platform_device.h:303:2: note: in expansion of macro 'module_driver'
     303 |  module_driver(__platform_driver, platform_driver_register, \
         |  ^~~~~~~~~~~~~
   drivers/reset/reset-meson.c:232:1: note: in expansion of macro 'module_platform_driver'
     232 | module_platform_driver(meson_reset_pltf_driver);
         | ^~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:141:7: error: redefinition of 'cleanup_module'
     141 |  void cleanup_module(void) __copy(exitfn)  \
         |       ^~~~~~~~~~~~~~
   include/linux/device/driver.h:267:1: note: in expansion of macro 'module_exit'
     267 | module_exit(__driver##_exit);
         | ^~~~~~~~~~~
   include/linux/auxiliary_bus.h:245:2: note: in expansion of macro 'module_driver'
     245 |  module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |  ^~~~~~~~~~~~~
   drivers/reset/reset-meson.c:272:1: note: in expansion of macro 'module_auxiliary_driver'
     272 | module_auxiliary_driver(meson_reset_aux_driver);
         | ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/module.h:141:7: note: previous definition of 'cleanup_module' was here
     141 |  void cleanup_module(void) __copy(exitfn)  \
         |       ^~~~~~~~~~~~~~
   include/linux/device/driver.h:267:1: note: in expansion of macro 'module_exit'
     267 | module_exit(__driver##_exit);
         | ^~~~~~~~~~~
   include/linux/platform_device.h:303:2: note: in expansion of macro 'module_driver'
     303 |  module_driver(__platform_driver, platform_driver_register, \
         |  ^~~~~~~~~~~~~
   drivers/reset/reset-meson.c:232:1: note: in expansion of macro 'module_platform_driver'
     232 | module_platform_driver(meson_reset_pltf_driver);
         | ^~~~~~~~~~~~~~~~~~~~~~
>> drivers/reset/reset-meson.c:292:5: error: redefinition of 'devm_meson_rst_aux_register'
     292 | int devm_meson_rst_aux_register(struct device *dev,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/reset/reset-meson.c:20:
   include/soc/amlogic/meson-auxiliary-reset.h:15:19: note: previous definition of 'devm_meson_rst_aux_register' was here
      15 | static inline int devm_meson_rst_aux_register(struct device *dev,
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/devm_meson_rst_aux_register +292 drivers/reset/reset-meson.c

   224	
   225	static struct platform_driver meson_reset_pltf_driver = {
   226		.probe	= meson_reset_pltf_probe,
   227		.driver = {
   228			.name		= "meson_reset",
   229			.of_match_table	= meson_reset_dt_ids,
   230		},
   231	};
 > 232	module_platform_driver(meson_reset_pltf_driver);
   233	
   234	static const struct meson_reset_param meson_g12a_audio_param = {
   235		.reset_ops	= &meson_reset_toggle_ops,
   236		.reset_num	= 26,
   237		.level_offset	= 0x24,
   238	};
   239	
   240	static const struct meson_reset_param meson_sm1_audio_param = {
   241		.reset_ops	= &meson_reset_toggle_ops,
   242		.reset_num	= 39,
   243		.level_offset	= 0x28,
   244	};
   245	
   246	static const struct auxiliary_device_id meson_reset_aux_ids[] = {
   247		{
   248			.name = "axg-audio-clkc.rst-g12a",
   249			.driver_data = (kernel_ulong_t)&meson_g12a_audio_param,
   250		}, {
   251			.name = "axg-audio-clkc.rst-sm1",
   252			.driver_data = (kernel_ulong_t)&meson_sm1_audio_param,
   253		},
   254	};
   255	MODULE_DEVICE_TABLE(auxiliary, meson_reset_aux_ids);
   256	
   257	static int meson_reset_aux_probe(struct auxiliary_device *adev,
   258					 const struct auxiliary_device_id *id)
   259	{
   260		const struct meson_reset_param *param =
   261			(const struct meson_reset_param *)(id->driver_data);
   262		struct meson_reset_adev *raux =
   263			to_meson_reset_adev(adev);
   264	
   265		return meson_reset_probe(&adev->dev, raux->map, param);
   266	}
   267	
   268	static struct auxiliary_driver meson_reset_aux_driver = {
   269		.probe		= meson_reset_aux_probe,
   270		.id_table	= meson_reset_aux_ids,
   271	};
   272	module_auxiliary_driver(meson_reset_aux_driver);
   273	
   274	static void meson_rst_aux_release(struct device *dev)
   275	{
   276		struct auxiliary_device *adev = to_auxiliary_dev(dev);
   277		struct meson_reset_adev *raux =
   278			to_meson_reset_adev(adev);
   279	
   280		ida_free(&meson_rst_aux_ida, adev->id);
   281		kfree(raux);
   282	}
   283	
   284	static void meson_rst_aux_unregister_adev(void *_adev)
   285	{
   286		struct auxiliary_device *adev = _adev;
   287	
   288		auxiliary_device_delete(adev);
   289		auxiliary_device_uninit(adev);
   290	}
   291	
 > 292	int devm_meson_rst_aux_register(struct device *dev,
   293					struct regmap *map,
   294					const char *adev_name)
   295	{
   296		struct meson_reset_adev *raux;
   297		struct auxiliary_device *adev;
   298		int ret;
   299	
   300		raux = kzalloc(sizeof(*raux), GFP_KERNEL);
   301		if (!raux)
   302			return -ENOMEM;
   303	
   304		ret = ida_alloc(&meson_rst_aux_ida, GFP_KERNEL);
   305		if (ret < 0)
   306			goto raux_free;
   307	
   308		raux->map = map;
   309	
   310		adev = &raux->adev;
   311		adev->id = ret;
   312		adev->name = adev_name;
   313		adev->dev.parent = dev;
   314		adev->dev.release = meson_rst_aux_release;
   315		device_set_of_node_from_dev(&adev->dev, dev);
   316	
   317		ret = auxiliary_device_init(adev);
   318		if (ret)
   319			goto ida_free;
   320	
   321		ret = __auxiliary_device_add(adev, dev->driver->name);
   322		if (ret) {
   323			auxiliary_device_uninit(adev);
   324			return ret;
   325		}
   326	
   327		return devm_add_action_or_reset(dev, meson_rst_aux_unregister_adev,
   328						adev);
   329	
   330	ida_free:
   331		ida_free(&meson_rst_aux_ida, adev->id);
   332	raux_free:
   333		kfree(raux);
   334		return ret;
   335
kernel test robot July 11, 2024, 7:03 p.m. UTC | #4
Hi Jerome,

kernel test robot noticed the following build errors:

[auto build test ERROR on pza/reset/next]
[also build test ERROR on clk/clk-next linus/master v6.10-rc7 next-20240711]
[cannot apply to pza/imx-drm/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jerome-Brunet/reset-amlogic-convert-driver-to-regmap/20240711-055833
base:   https://git.pengutronix.de/git/pza/linux reset/next
patch link:    https://lore.kernel.org/r/20240710162526.2341399-8-jbrunet%40baylibre.com
patch subject: [PATCH 7/8] reset: amlogic: add auxiliary reset driver support
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240712/202407120208.ub5kh5K1-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project a0c6b8aef853eedaa0980f07c0a502a5a8a9740e)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240712/202407120208.ub5kh5K1-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/202407120208.ub5kh5K1-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/reset/reset-meson.c:8:
   In file included from include/linux/auxiliary_bus.h:11:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:173:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2253:
   include/linux/vmstat.h:500:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     500 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     501 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:507:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     507 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     508 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:519:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     519 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     520 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:528:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     528 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     529 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/reset/reset-meson.c:11:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
         |                                                           ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
     102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
         |                                                      ^
   In file included from drivers/reset/reset-meson.c:11:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
         |                                                           ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
     115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
         |                                                      ^
   In file included from drivers/reset/reset-meson.c:11:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     693 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     701 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     709 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     718 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     727 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     736 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
>> drivers/reset/reset-meson.c:272:1: error: redefinition of '__inittest'
     272 | module_auxiliary_driver(meson_reset_aux_driver);
         | ^
   include/linux/auxiliary_bus.h:245:2: note: expanded from macro 'module_auxiliary_driver'
     245 |         module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |         ^
   include/linux/device/driver.h:261:3: note: expanded from macro 'module_driver'
     261 | } \
         |   ^
   include/linux/module.h:131:42: note: expanded from macro '\
   module_init'
     131 |         static inline initcall_t __maybe_unused __inittest(void)                \
         |                                                 ^
   drivers/reset/reset-meson.c:232:1: note: previous definition is here
     232 | module_platform_driver(meson_reset_pltf_driver);
         | ^
   include/linux/platform_device.h:303:2: note: expanded from macro 'module_platform_driver'
     303 |         module_driver(__platform_driver, platform_driver_register, \
         |         ^
   include/linux/device/driver.h:261:3: note: expanded from macro 'module_driver'
     261 | } \
         |   ^
   include/linux/module.h:131:42: note: expanded from macro '\
   module_init'
     131 |         static inline initcall_t __maybe_unused __inittest(void)                \
         |                                                 ^
>> drivers/reset/reset-meson.c:272:1: error: redefinition of 'init_module'
     272 | module_auxiliary_driver(meson_reset_aux_driver);
         | ^
   include/linux/auxiliary_bus.h:245:2: note: expanded from macro 'module_auxiliary_driver'
     245 |         module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |         ^
   include/linux/device/driver.h:261:3: note: expanded from macro 'module_driver'
     261 | } \
         |   ^
   include/linux/module.h:133:6: note: expanded from macro '\
   module_init'
     133 |         int init_module(void) __copy(initfn)                    \
         |             ^
   drivers/reset/reset-meson.c:232:1: note: previous definition is here
     232 | module_platform_driver(meson_reset_pltf_driver);
         | ^
   include/linux/platform_device.h:303:2: note: expanded from macro 'module_platform_driver'
     303 |         module_driver(__platform_driver, platform_driver_register, \
         |         ^
   include/linux/device/driver.h:261:3: note: expanded from macro 'module_driver'
     261 | } \
         |   ^
   include/linux/module.h:133:6: note: expanded from macro '\
   module_init'
     133 |         int init_module(void) __copy(initfn)                    \
         |             ^
>> drivers/reset/reset-meson.c:272:1: error: redefinition of '__exittest'
     272 | module_auxiliary_driver(meson_reset_aux_driver);
         | ^
   include/linux/auxiliary_bus.h:245:2: note: expanded from macro 'module_auxiliary_driver'
     245 |         module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |         ^
   include/linux/device/driver.h:266:3: note: expanded from macro 'module_driver'
     266 | } \
         |   ^
   include/linux/module.h:139:42: note: expanded from macro '\
   module_exit'
     139 |         static inline exitcall_t __maybe_unused __exittest(void)                \
         |                                                 ^
   drivers/reset/reset-meson.c:232:1: note: previous definition is here
     232 | module_platform_driver(meson_reset_pltf_driver);
         | ^
   include/linux/platform_device.h:303:2: note: expanded from macro 'module_platform_driver'
     303 |         module_driver(__platform_driver, platform_driver_register, \
         |         ^
   include/linux/device/driver.h:266:3: note: expanded from macro 'module_driver'
     266 | } \
         |   ^
   include/linux/module.h:139:42: note: expanded from macro '\
   module_exit'
     139 |         static inline exitcall_t __maybe_unused __exittest(void)                \
         |                                                 ^
>> drivers/reset/reset-meson.c:272:1: error: redefinition of 'cleanup_module'
     272 | module_auxiliary_driver(meson_reset_aux_driver);
         | ^
   include/linux/auxiliary_bus.h:245:2: note: expanded from macro 'module_auxiliary_driver'
     245 |         module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |         ^
   include/linux/device/driver.h:266:3: note: expanded from macro 'module_driver'
     266 | } \
         |   ^
   include/linux/module.h:141:7: note: expanded from macro '\
   module_exit'
     141 |         void cleanup_module(void) __copy(exitfn)                \
         |              ^
   drivers/reset/reset-meson.c:232:1: note: previous definition is here
     232 | module_platform_driver(meson_reset_pltf_driver);
         | ^
   include/linux/platform_device.h:303:2: note: expanded from macro 'module_platform_driver'
     303 |         module_driver(__platform_driver, platform_driver_register, \
         |         ^
   include/linux/device/driver.h:266:3: note: expanded from macro 'module_driver'
     266 | } \
         |   ^
   include/linux/module.h:141:7: note: expanded from macro '\
   module_exit'
     141 |         void cleanup_module(void) __copy(exitfn)                \
         |              ^
   drivers/reset/reset-meson.c:292:5: error: redefinition of 'devm_meson_rst_aux_register'
     292 | int devm_meson_rst_aux_register(struct device *dev,
         |     ^
   include/soc/amlogic/meson-auxiliary-reset.h:15:19: note: previous definition is here
      15 | static inline int devm_meson_rst_aux_register(struct device *dev,
         |                   ^
   17 warnings and 5 errors generated.


vim +/__inittest +272 drivers/reset/reset-meson.c

   267	
   268	static struct auxiliary_driver meson_reset_aux_driver = {
   269		.probe		= meson_reset_aux_probe,
   270		.id_table	= meson_reset_aux_ids,
   271	};
 > 272	module_auxiliary_driver(meson_reset_aux_driver);
   273
Stephen Boyd July 15, 2024, 7:30 p.m. UTC | #5
Quoting Jerome Brunet (2024-07-11 02:01:16)
> On Wed 10 Jul 2024 at 15:49, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > Quoting Jerome Brunet (2024-07-10 09:25:16)
> >> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> >> index e34a10b15593..5cc767d50e8f 100644
> >> --- a/drivers/reset/reset-meson.c
> >> +++ b/drivers/reset/reset-meson.c
> > [...]
> >> +
> >> +int devm_meson_rst_aux_register(struct device *dev,
> >> +                               struct regmap *map,
> >> +                               const char *adev_name)
> >> +{
> >> +       struct meson_reset_adev *raux;
> >> +       struct auxiliary_device *adev;
> >> +       int ret;
> >> +
> >> +       raux = kzalloc(sizeof(*raux), GFP_KERNEL);
> >> +       if (!raux)
> >> +               return -ENOMEM;
> >> +
> >> +       ret = ida_alloc(&meson_rst_aux_ida, GFP_KERNEL);
> >
> > Do we expect more than one device with the same name? I wonder if the
> > IDA can be skipped.
> 
> I've wondered about that too.
> 
> I don't think it is the case right now but I'm not 100% sure.
> Since I spent time thinking about it, I thought it would just be safer (and
> relatively cheap) to put in and enough annoying debugging the
> expectation does not hold true.
> 
> I don't have a strong opinion on this. What do you prefer ?

I don't have a strong opinion either so it's fine to leave it there.

> >> diff --git a/include/soc/amlogic/meson-auxiliary-reset.h b/include/soc/amlogic/meson-auxiliary-reset.h
> >> new file mode 100644
> >> index 000000000000..8fdb02b18d8c
> >> --- /dev/null
> >> +++ b/include/soc/amlogic/meson-auxiliary-reset.h
> >> @@ -0,0 +1,23 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +#ifndef __SOC_AMLOGIC_MESON_AUX_RESET_H
> >> +#define __SOC_AMLOGIC_MESON_AUX_RESET_H
> >> +
> >> +#include <linux/err.h>
> >> +
> >> +struct device;
> >> +struct regmap;
> >> +
> >> +#ifdef CONFIG_RESET_MESON
> >> +int devm_meson_rst_aux_register(struct device *dev,
> >> +                               struct regmap *map,
> >> +                               const char *adev_name);
> >> +#else
> >> +static inline int devm_meson_rst_aux_register(struct device *dev,
> >> +                                             struct regmap *map,
> >> +                                             const char *adev_name)
> >> +{
> >> +       return -EOPNOTSUPP;
> >
> > Shouldn't this be 'return 0' so that the clk driver doesn't have to care
> > about the config?
> 
> I don't think the system (in general) would be able function without the reset
> driver, so the question is rather phylosophical.
> 
> Let's say it could, if this returns 0, consumers of the reset controller
> will defer indefinitely ... which is always a bit more difficult to sort
> out.
> 
> If it returns an error, the problem is pretty obvious, helping people
> solve it quickly.
> 
> Also the actual device we trying to register provides clocks and reset.
> It is not like the reset is an optional part we don't care about.
> 
> On this instance it starts from clock, but it could have been the other
> way around. Both are equally important.
> 
> I'd prefer if it returns an error when the registration can't even start.
> 

Ok. Fair enough.
Jerome Brunet July 18, 2024, 7:05 a.m. UTC | #6
On Mon 15 Jul 2024 at 12:30, Stephen Boyd <sboyd@kernel.org> wrote:

>> >> +int devm_meson_rst_aux_register(struct device *dev,
>> >> +                               struct regmap *map,
>> >> +                               const char *adev_name);
>> >> +#else
>> >> +static inline int devm_meson_rst_aux_register(struct device *dev,
>> >> +                                             struct regmap *map,
>> >> +                                             const char *adev_name)
>> >> +{
>> >> +       return -EOPNOTSUPP;
>> >
>> > Shouldn't this be 'return 0' so that the clk driver doesn't have to care
>> > about the config?
>> 
>> I don't think the system (in general) would be able function without the reset
>> driver, so the question is rather phylosophical.
>> 
>> Let's say it could, if this returns 0, consumers of the reset controller
>> will defer indefinitely ... which is always a bit more difficult to sort
>> out.
>> 
>> If it returns an error, the problem is pretty obvious, helping people
>> solve it quickly.
>> 
>> Also the actual device we trying to register provides clocks and reset.
>> It is not like the reset is an optional part we don't care about.
>> 
>> On this instance it starts from clock, but it could have been the other
>> way around. Both are equally important.
>> 
>> I'd prefer if it returns an error when the registration can't even start.
>> 
>
> Ok. Fair enough.

Actually, thinking about it more I changed my mind and I tend to agree
on 'return 0' which I'll use in the next version.

The initial request was to de-couple clk and reset. I was planning on
having clk 'imply' reset to have a weak dependency. That does not make
sense if an error is returned above. I would have to use 'depends on' and
don't like it in that case, sooo weak dependency it is.

It remains fairly easy to change later on if necessary
diff mbox series

Patch

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 7112f5932609..2a316c469bcc 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -134,6 +134,7 @@  config RESET_MCHP_SPARX5
 config RESET_MESON
 	tristate "Meson Reset Driver"
 	depends on ARCH_MESON || COMPILE_TEST
+	select AUXILIARY_BUS
 	default ARCH_MESON
 	help
 	  This enables the reset driver for Amlogic Meson SoCs.
diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
index e34a10b15593..5cc767d50e8f 100644
--- a/drivers/reset/reset-meson.c
+++ b/drivers/reset/reset-meson.c
@@ -5,6 +5,7 @@ 
  * Copyright (c) 2016 BayLibre, SAS.
  * Author: Neil Armstrong <narmstrong@baylibre.com>
  */
+#include <linux/auxiliary_bus.h>
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/io.h>
@@ -16,6 +17,10 @@ 
 #include <linux/slab.h>
 #include <linux/types.h>
 
+#include <soc/amlogic/meson-auxiliary-reset.h>
+
+static DEFINE_IDA(meson_rst_aux_ida);
+
 struct meson_reset_param {
 	const struct reset_control_ops *reset_ops;
 	unsigned int reset_num;
@@ -30,6 +35,14 @@  struct meson_reset {
 	struct regmap *map;
 };
 
+struct meson_reset_adev {
+	struct auxiliary_device adev;
+	struct regmap *map;
+};
+
+#define to_meson_reset_adev(_adev) \
+	container_of((_adev), struct meson_reset_adev, adev)
+
 static void meson_reset_offset_and_bit(struct meson_reset *data,
 				       unsigned long id,
 				       unsigned int *offset,
@@ -218,6 +231,112 @@  static struct platform_driver meson_reset_pltf_driver = {
 };
 module_platform_driver(meson_reset_pltf_driver);
 
-MODULE_DESCRIPTION("Amlogic Meson Reset Controller driver");
+static const struct meson_reset_param meson_g12a_audio_param = {
+	.reset_ops	= &meson_reset_toggle_ops,
+	.reset_num	= 26,
+	.level_offset	= 0x24,
+};
+
+static const struct meson_reset_param meson_sm1_audio_param = {
+	.reset_ops	= &meson_reset_toggle_ops,
+	.reset_num	= 39,
+	.level_offset	= 0x28,
+};
+
+static const struct auxiliary_device_id meson_reset_aux_ids[] = {
+	{
+		.name = "axg-audio-clkc.rst-g12a",
+		.driver_data = (kernel_ulong_t)&meson_g12a_audio_param,
+	}, {
+		.name = "axg-audio-clkc.rst-sm1",
+		.driver_data = (kernel_ulong_t)&meson_sm1_audio_param,
+	},
+};
+MODULE_DEVICE_TABLE(auxiliary, meson_reset_aux_ids);
+
+static int meson_reset_aux_probe(struct auxiliary_device *adev,
+				 const struct auxiliary_device_id *id)
+{
+	const struct meson_reset_param *param =
+		(const struct meson_reset_param *)(id->driver_data);
+	struct meson_reset_adev *raux =
+		to_meson_reset_adev(adev);
+
+	return meson_reset_probe(&adev->dev, raux->map, param);
+}
+
+static struct auxiliary_driver meson_reset_aux_driver = {
+	.probe		= meson_reset_aux_probe,
+	.id_table	= meson_reset_aux_ids,
+};
+module_auxiliary_driver(meson_reset_aux_driver);
+
+static void meson_rst_aux_release(struct device *dev)
+{
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
+	struct meson_reset_adev *raux =
+		to_meson_reset_adev(adev);
+
+	ida_free(&meson_rst_aux_ida, adev->id);
+	kfree(raux);
+}
+
+static void meson_rst_aux_unregister_adev(void *_adev)
+{
+	struct auxiliary_device *adev = _adev;
+
+	auxiliary_device_delete(adev);
+	auxiliary_device_uninit(adev);
+}
+
+int devm_meson_rst_aux_register(struct device *dev,
+				struct regmap *map,
+				const char *adev_name)
+{
+	struct meson_reset_adev *raux;
+	struct auxiliary_device *adev;
+	int ret;
+
+	raux = kzalloc(sizeof(*raux), GFP_KERNEL);
+	if (!raux)
+		return -ENOMEM;
+
+	ret = ida_alloc(&meson_rst_aux_ida, GFP_KERNEL);
+	if (ret < 0)
+		goto raux_free;
+
+	raux->map = map;
+
+	adev = &raux->adev;
+	adev->id = ret;
+	adev->name = adev_name;
+	adev->dev.parent = dev;
+	adev->dev.release = meson_rst_aux_release;
+	device_set_of_node_from_dev(&adev->dev, dev);
+
+	ret = auxiliary_device_init(adev);
+	if (ret)
+		goto ida_free;
+
+	ret = __auxiliary_device_add(adev, dev->driver->name);
+	if (ret) {
+		auxiliary_device_uninit(adev);
+		return ret;
+	}
+
+	return devm_add_action_or_reset(dev, meson_rst_aux_unregister_adev,
+					adev);
+
+ida_free:
+	ida_free(&meson_rst_aux_ida, adev->id);
+raux_free:
+	kfree(raux);
+	return ret;
+
+}
+EXPORT_SYMBOL_GPL(devm_meson_rst_aux_register);
+
+MODULE_DESCRIPTION("Amlogic Meson Reset driver");
 MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
+MODULE_AUTHOR("Jerome Brunet <jbrunet@baylibre.com>");
 MODULE_LICENSE("Dual BSD/GPL");
diff --git a/include/soc/amlogic/meson-auxiliary-reset.h b/include/soc/amlogic/meson-auxiliary-reset.h
new file mode 100644
index 000000000000..8fdb02b18d8c
--- /dev/null
+++ b/include/soc/amlogic/meson-auxiliary-reset.h
@@ -0,0 +1,23 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SOC_AMLOGIC_MESON_AUX_RESET_H
+#define __SOC_AMLOGIC_MESON_AUX_RESET_H
+
+#include <linux/err.h>
+
+struct device;
+struct regmap;
+
+#ifdef CONFIG_RESET_MESON
+int devm_meson_rst_aux_register(struct device *dev,
+				struct regmap *map,
+				const char *adev_name);
+#else
+static inline int devm_meson_rst_aux_register(struct device *dev,
+					      struct regmap *map,
+					      const char *adev_name)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
+#endif /* __SOC_AMLOGIC_MESON8B_AUX_RESET_H */