Add an helper module to register auxiliary reset drivers from
Amlogic clock controller.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/meson/Kconfig             |  5 ++
 drivers/clk/meson/Makefile            |  1 +
 drivers/clk/meson/meson-clk-rst-aux.c | 84 +++++++++++++++++++++++++++
 drivers/clk/meson/meson-clk-rst-aux.h | 14 +++++
 4 files changed, 104 insertions(+)
 create mode 100644 drivers/clk/meson/meson-clk-rst-aux.c
 create mode 100644 drivers/clk/meson/meson-clk-rst-aux.h

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index 59a40a49f8e1..6905aa2f080c 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -37,6 +37,11 @@ config COMMON_CLK_MESON_VCLK
 config COMMON_CLK_MESON_CLKC_UTILS
 	tristate
 
+config COMMON_CLK_MESON_CLK_RST_AUX
+	depends on REGMAP && AUXILIARY_BUS
+	imply RESET_MESON
+	tristate
+
 config COMMON_CLK_MESON_AO_CLKC
 	tristate
 	select COMMON_CLK_MESON_REGMAP
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 9ba43fe7a07a..2926a04b6c33 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -2,6 +2,7 @@
 # Amlogic clock drivers
 
 obj-$(CONFIG_COMMON_CLK_MESON_CLKC_UTILS) += meson-clkc-utils.o
+obj-$(CONFIG_COMMON_CLK_MESON_CLK_RST_AUX) += meson-clk-rst-aux.o
 obj-$(CONFIG_COMMON_CLK_MESON_AO_CLKC) += meson-aoclk.o
 obj-$(CONFIG_COMMON_CLK_MESON_CPU_DYNDIV) += clk-cpu-dyndiv.o
 obj-$(CONFIG_COMMON_CLK_MESON_DUALDIV) += clk-dualdiv.o
diff --git a/drivers/clk/meson/meson-clk-rst-aux.c b/drivers/clk/meson/meson-clk-rst-aux.c
new file mode 100644
index 000000000000..a7cf3c39828c
--- /dev/null
+++ b/drivers/clk/meson/meson-clk-rst-aux.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2024 BayLibre, SAS.
+// Author: Jerome Brunet <jbrunet@baylibre.com>
+
+#include <linux/auxiliary_bus.h>
+#include <linux/idr.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include <soc/amlogic/meson8b-auxiliary-reset.h>
+
+#include "meson-clk-rst-aux.h"
+
+static DEFINE_IDA(meson_clk_rst_aux_ida);
+
+static void meson_clk_rst_aux_release(struct device *dev)
+{
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
+	struct meson8b_reset_adev *raux =
+		to_meson8b_reset_adev(adev);
+
+	ida_free(&meson_clk_rst_aux_ida, adev->id);
+	kfree(raux);
+}
+
+static void meson_clk_rst_aux_unregister_adev(void *_adev)
+{
+	struct auxiliary_device *adev = _adev;
+
+	auxiliary_device_delete(adev);
+	auxiliary_device_uninit(adev);
+}
+
+int devm_meson_clk_rst_aux_register(struct device *dev,
+				    struct regmap *map,
+				    const char *adev_name)
+{
+	struct meson8b_reset_adev *raux;
+	struct auxiliary_device *adev;
+	int ret;
+
+	raux = kzalloc(sizeof(*raux), GFP_KERNEL);
+	if (!raux)
+		return -ENOMEM;
+
+	ret = ida_alloc(&meson_clk_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_clk_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_clk_rst_aux_unregister_adev,
+					adev);
+
+ida_free:
+	ida_free(&meson_clk_rst_aux_ida, adev->id);
+raux_free:
+	kfree(raux);
+	return ret;
+
+}
+EXPORT_SYMBOL_GPL(devm_meson_clk_rst_aux_register);
+
+MODULE_DESCRIPTION("Amlogic auxiliary reset helper for clock");
+MODULE_AUTHOR("Jerome Brunet <jbrunet@baylibre.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/clk/meson/meson-clk-rst-aux.h b/drivers/clk/meson/meson-clk-rst-aux.h
new file mode 100644
index 000000000000..386a55a36cd9
--- /dev/null
+++ b/drivers/clk/meson/meson-clk-rst-aux.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024 BayLibre, SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ */
+
+#ifndef __MESON_CLK_RST_AUX_H
+#define __MESON_CLK_RST_AUX_H
+
+int devm_meson_clk_rst_aux_register(struct device *dev,
+				    struct regmap *map,
+				    const char *adev_name);
+
+#endif /* __MESON_CLK_RST_AUX_H */
-- 
2.43.0

Quoting Jerome Brunet (2024-05-16 08:08:38)
> Add an helper module to register auxiliary reset drivers from
> Amlogic clock controller.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/clk/meson/Kconfig             |  5 ++
>  drivers/clk/meson/Makefile            |  1 +
>  drivers/clk/meson/meson-clk-rst-aux.c | 84 +++++++++++++++++++++++++++
>  drivers/clk/meson/meson-clk-rst-aux.h | 14 +++++
>  4 files changed, 104 insertions(+)
>  create mode 100644 drivers/clk/meson/meson-clk-rst-aux.c
>  create mode 100644 drivers/clk/meson/meson-clk-rst-aux.h
> 
> diff --git a/drivers/clk/meson/meson-clk-rst-aux.h b/drivers/clk/meson/meson-clk-rst-aux.h
> new file mode 100644
> index 000000000000..386a55a36cd9
> --- /dev/null
> +++ b/drivers/clk/meson/meson-clk-rst-aux.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2024 BayLibre, SAS.
> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> + */
> +
> +#ifndef __MESON_CLK_RST_AUX_H
> +#define __MESON_CLK_RST_AUX_H
> +
> +int devm_meson_clk_rst_aux_register(struct device *dev,
> +                                   struct regmap *map,
> +                                   const char *adev_name);

I'd prefer we move the device creation and registration logic to
drivers/reset as well. See commit 098c290a490d ("clock, reset:
microchip: move all mpfs reset code to the reset subsystem") for some
inspiration.

One thing I haven't really thought about too much is if they're two
different modules. One for clk and one for reset. If the device
registration API is a symbol the clk module depends on then maybe that
is better because it means both modules are loaded, avoiding a
round-trip through modprobe. It also makes sure that the drivers are
either both builtin or both modular.

On Wed 29 May 2024 at 18:01, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Jerome Brunet (2024-05-16 08:08:38)
>> Add an helper module to register auxiliary reset drivers from
>> Amlogic clock controller.
>> 
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/clk/meson/Kconfig             |  5 ++
>>  drivers/clk/meson/Makefile            |  1 +
>>  drivers/clk/meson/meson-clk-rst-aux.c | 84 +++++++++++++++++++++++++++
>>  drivers/clk/meson/meson-clk-rst-aux.h | 14 +++++
>>  4 files changed, 104 insertions(+)
>>  create mode 100644 drivers/clk/meson/meson-clk-rst-aux.c
>>  create mode 100644 drivers/clk/meson/meson-clk-rst-aux.h
>> 
>> diff --git a/drivers/clk/meson/meson-clk-rst-aux.h b/drivers/clk/meson/meson-clk-rst-aux.h
>> new file mode 100644
>> index 000000000000..386a55a36cd9
>> --- /dev/null
>> +++ b/drivers/clk/meson/meson-clk-rst-aux.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (c) 2024 BayLibre, SAS.
>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>> + */
>> +
>> +#ifndef __MESON_CLK_RST_AUX_H
>> +#define __MESON_CLK_RST_AUX_H
>> +
>> +int devm_meson_clk_rst_aux_register(struct device *dev,
>> +                                   struct regmap *map,
>> +                                   const char *adev_name);
>
> I'd prefer we move the device creation and registration logic to
> drivers/reset as well. See commit 098c290a490d ("clock, reset:
> microchip: move all mpfs reset code to the reset subsystem") for some
> inspiration.

Ok but if it lives in reset I don't really get the purpose served by the
auxiliary devices in that case. Why not export a function that directly
calls reset_controller_register() in that case ? 

I thought the point was to properly decouple both sides.

I don't have strong opinion about it, TBH. It is just how it made sense
to me. If you are sure about this, I don't mind changing

>
> One thing I haven't really thought about too much is if they're two
> different modules. One for clk and one for reset. If the device
> registration API is a symbol the clk module depends on then maybe that
> is better because it means both modules are loaded, avoiding a
> round-trip through modprobe. It also makes sure that the drivers are
> either both builtin or both modular.

I have checked with the current implementation, if the reset driver is
missing, the clock part does not fail. Registering the aux device
succeeds in clock but the device never comes up (duh). So it does
not crash, the consumers of the aux reset device will just defer.

Said differently, the '#if IS_ENABLED(CONFIG_RESET_CONTROLLER)' in
clk-mpfs.c was not necessary ... it was removed in the changed you
linked anyway.

(Sorry Stephen, you got it twice ... missed the Reply-all the first time
around)

-- 
Jerome

Quoting Jerome Brunet (2024-06-10 03:10:55)
> On Wed 29 May 2024 at 18:01, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> >
> > I'd prefer we move the device creation and registration logic to
> > drivers/reset as well. See commit 098c290a490d ("clock, reset:
> > microchip: move all mpfs reset code to the reset subsystem") for some
> > inspiration.
> 
> Ok but if it lives in reset I don't really get the purpose served by the
> auxiliary devices in that case. Why not export a function that directly
> calls reset_controller_register() in that case ? 
> 
> I thought the point was to properly decouple both sides.

Yes.

We use auxiliary devices so that the clk and reset drivers are
decoupled. Calling reset_controller_register() directly must return
success, whereas creating a device _should_ only fail if the device
couldn't be created/registered. It's less likely to fail with an
auxiliary device. This also allows the clk driver to be a module and the
reset driver builtin if, for example, the reset framework doesn't
support modules. Another benefit would be moving the probe context to
another thread if it can be async.

> 
> I don't have strong opinion about it, TBH. It is just how it made sense
> to me. If you are sure about this, I don't mind changing

It seems better to put the device creation in the same file as the
driver so that it's further decoupled from the clk driver and
consolidated in the reset directory. This way, a single API is the only
thing the clk driver uses to create the reset device, instead of putting
the matching string for the device and driver in the clk and reset
drivers and having to know that the auxiliary bus is used.

The downside I see is that the clk driver can't be builtin if the reset
driver is a module. I'm not sure that's really a problem though. If it
is then we can make the registration API into another C file that still
lives in drivers/reset that has another kconfig symbol.

> 
> >
> > One thing I haven't really thought about too much is if they're two
> > different modules. One for clk and one for reset. If the device
> > registration API is a symbol the clk module depends on then maybe that
> > is better because it means both modules are loaded, avoiding a
> > round-trip through modprobe. It also makes sure that the drivers are
> > either both builtin or both modular.
> 
> I have checked with the current implementation, if the reset driver is
> missing, the clock part does not fail. Registering the aux device
> succeeds in clock but the device never comes up (duh). So it does
> not crash, the consumers of the aux reset device will just defer.
> 
> Said differently, the '#if IS_ENABLED(CONFIG_RESET_CONTROLLER)' in
> clk-mpfs.c was not necessary ... it was removed in the changed you
> linked anyway.
> 

Cool.