| 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 |
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?
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.
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
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
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.
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 --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 */
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