[PATCH 0/8] reset: amlogic: move audio reset drivers out of CCF

[PATCH 0/8] reset: amlogic: move audio reset drivers out of CCF

[Jerome Brunet]

This patchset follows the discussion about having reset driver in the
clock tree [1]. Ideally those should reside in the reset part of tree.

Also the code of the amlogic reset driver is very similar between the 2
trees and could use the same driver code.

This patchset moves the reset driver of audio clock controller of the
g12 and sm1 SoC family to the reset tree, using the auxiliary bus.

The infrastructure put in place is meant to be generic enough so we may
eventually also move the reset drivers in the meson8b and aoclk clock
controllers.

Change since RFC [2]:
 * Move the aux registration helper out of clock too.

[1] https://lore.kernel.org/linux-clk/e3a85852b911fdf16dd9ae158f42b3ef.sboyd@kernel.org
[2] https://lore.kernel.org/linux-clk/20240516150842.705844-1-jbrunet@baylibre.com

Jerome Brunet (8):
  reset: amlogic: convert driver to regmap
  reset: amlogic: add driver parameters
  reset: amlogic: split the device and platform probe
  reset: amlogic: use reset number instead of register count
  reset: amlogic: add reset status support
  reset: amlogic: add toggle reset support
  reset: amlogic: add auxiliary reset driver support
  clk: amlogic: axg-audio: use the auxiliary reset driver

 drivers/clk/meson/Kconfig                   |   1 +
 drivers/clk/meson/axg-audio.c               | 109 +-------
 drivers/reset/Kconfig                       |   1 +
 drivers/reset/reset-meson.c                 | 285 ++++++++++++++++----
 include/soc/amlogic/meson-auxiliary-reset.h |  23 ++
 5 files changed, 271 insertions(+), 148 deletions(-)
 create mode 100644 include/soc/amlogic/meson-auxiliary-reset.h

-- 
2.43.0

[PATCH 1/8] reset: amlogic: convert driver to regmap

[Jerome Brunet]

To allow using the same driver for the main reset controller and the
auxiliary ones embedded in the clock controllers, convert the
the Amlogic reset driver to regmap.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/reset/reset-meson.c | 80 ++++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 36 deletions(-)

diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
index f78be97898bc..8f3d6e9df235 100644
--- a/drivers/reset/reset-meson.c
+++ b/drivers/reset/reset-meson.c
@@ -11,36 +11,44 @@
 #include <linux/of.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/reset-controller.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 
-#define BITS_PER_REG	32
-
 struct meson_reset_param {
 	int reg_count;
 	int level_offset;
 };
 
 struct meson_reset {
-	void __iomem *reg_base;
 	const struct meson_reset_param *param;
 	struct reset_controller_dev rcdev;
-	spinlock_t lock;
+	struct regmap *map;
 };
 
+static void meson_reset_offset_and_bit(struct meson_reset *data,
+				       unsigned long id,
+				       unsigned int *offset,
+				       unsigned int *bit)
+{
+	unsigned int stride = regmap_get_reg_stride(data->map);
+
+	*offset = (id / (stride * BITS_PER_BYTE)) * stride;
+	*bit = id % (stride * BITS_PER_BYTE);
+}
+
 static int meson_reset_reset(struct reset_controller_dev *rcdev,
-			      unsigned long id)
+			     unsigned long id)
 {
 	struct meson_reset *data =
 		container_of(rcdev, struct meson_reset, rcdev);
-	unsigned int bank = id / BITS_PER_REG;
-	unsigned int offset = id % BITS_PER_REG;
-	void __iomem *reg_addr = data->reg_base + (bank << 2);
+	unsigned int offset, bit;
 
-	writel(BIT(offset), reg_addr);
+	meson_reset_offset_and_bit(data, id, &offset, &bit);
 
-	return 0;
+	return regmap_update_bits(data->map, offset,
+				  BIT(bit), BIT(bit));
 }
 
 static int meson_reset_level(struct reset_controller_dev *rcdev,
@@ -48,25 +56,13 @@ static int meson_reset_level(struct reset_controller_dev *rcdev,
 {
 	struct meson_reset *data =
 		container_of(rcdev, struct meson_reset, rcdev);
-	unsigned int bank = id / BITS_PER_REG;
-	unsigned int offset = id % BITS_PER_REG;
-	void __iomem *reg_addr;
-	unsigned long flags;
-	u32 reg;
+	unsigned int offset, bit;
 
-	reg_addr = data->reg_base + data->param->level_offset + (bank << 2);
+	meson_reset_offset_and_bit(data, id, &offset, &bit);
+	offset += data->param->level_offset;
 
-	spin_lock_irqsave(&data->lock, flags);
-
-	reg = readl(reg_addr);
-	if (assert)
-		writel(reg & ~BIT(offset), reg_addr);
-	else
-		writel(reg | BIT(offset), reg_addr);
-
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return 0;
+	return regmap_update_bits(data->map, offset,
+				  BIT(bit), assert ? 0 : BIT(bit));
 }
 
 static int meson_reset_assert(struct reset_controller_dev *rcdev,
@@ -113,30 +109,42 @@ static const struct of_device_id meson_reset_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, meson_reset_dt_ids);
 
+static const struct regmap_config regmap_config = {
+	.reg_bits   = 32,
+	.val_bits   = 32,
+	.reg_stride = 4,
+};
+
 static int meson_reset_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct meson_reset *data;
+	void __iomem *base;
 
-	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	data->reg_base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(data->reg_base))
-		return PTR_ERR(data->reg_base);
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
 
-	data->param = of_device_get_match_data(&pdev->dev);
+	data->param = of_device_get_match_data(dev);
 	if (!data->param)
 		return -ENODEV;
 
-	spin_lock_init(&data->lock);
+	data->map = devm_regmap_init_mmio(dev, base, &regmap_config);
+	if (IS_ERR(data->map))
+		return dev_err_probe(dev, PTR_ERR(data->map),
+				     "can't init regmap mmio region\n");
 
 	data->rcdev.owner = THIS_MODULE;
-	data->rcdev.nr_resets = data->param->reg_count * BITS_PER_REG;
+	data->rcdev.nr_resets = data->param->reg_count * BITS_PER_BYTE
+		* regmap_config.reg_stride;
 	data->rcdev.ops = &meson_reset_ops;
-	data->rcdev.of_node = pdev->dev.of_node;
+	data->rcdev.of_node = dev->of_node;
 
-	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
+	return devm_reset_controller_register(dev, &data->rcdev);
 }
 
 static struct platform_driver meson_reset_driver = {
-- 
2.43.0

[PATCH 2/8] reset: amlogic: add driver parameters

[Jerome Brunet]

To allow using the same driver for the main reset controller and the
auxiliary ones embedded in the clock controllers, allow to customise
the reset offset, same as the level offset. Also add an option to make
the level reset active low or high.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/reset/reset-meson.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
index 8f3d6e9df235..59126c9f194a 100644
--- a/drivers/reset/reset-meson.c
+++ b/drivers/reset/reset-meson.c
@@ -18,7 +18,9 @@
 
 struct meson_reset_param {
 	int reg_count;
+	int reset_offset;
 	int level_offset;
+	bool level_low_reset;
 };
 
 struct meson_reset {
@@ -46,6 +48,7 @@ static int meson_reset_reset(struct reset_controller_dev *rcdev,
 	unsigned int offset, bit;
 
 	meson_reset_offset_and_bit(data, id, &offset, &bit);
+	offset += data->param->reset_offset;
 
 	return regmap_update_bits(data->map, offset,
 				  BIT(bit), BIT(bit));
@@ -60,9 +63,10 @@ static int meson_reset_level(struct reset_controller_dev *rcdev,
 
 	meson_reset_offset_and_bit(data, id, &offset, &bit);
 	offset += data->param->level_offset;
+	assert ^= data->param->level_low_reset;
 
 	return regmap_update_bits(data->map, offset,
-				  BIT(bit), assert ? 0 : BIT(bit));
+				  BIT(bit), assert ? BIT(bit) : 0);
 }
 
 static int meson_reset_assert(struct reset_controller_dev *rcdev,
@@ -85,17 +89,23 @@ static const struct reset_control_ops meson_reset_ops = {
 
 static const struct meson_reset_param meson8b_param = {
 	.reg_count	= 8,
+	.reset_offset	= 0x0,
 	.level_offset	= 0x7c,
+	.level_low_reset = true,
 };
 
 static const struct meson_reset_param meson_a1_param = {
 	.reg_count	= 3,
+	.reset_offset	= 0x0,
 	.level_offset	= 0x40,
+	.level_low_reset = true,
 };
 
 static const struct meson_reset_param meson_s4_param = {
 	.reg_count	= 6,
+	.reset_offset	= 0x0,
 	.level_offset	= 0x40,
+	.level_low_reset = true,
 };
 
 static const struct of_device_id meson_reset_dt_ids[] = {
-- 
2.43.0

[PATCH 3/8] reset: amlogic: split the device and platform probe

[Jerome Brunet]

To prepare the addition of the auxiliary device support, split
out the device probe from the probe of the platform device.

The device probe will be common to both the platform and auxiliary
driver.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/reset/reset-meson.c | 55 +++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
index 59126c9f194a..fec55321b52b 100644
--- a/drivers/reset/reset-meson.c
+++ b/drivers/reset/reset-meson.c
@@ -87,6 +87,27 @@ static const struct reset_control_ops meson_reset_ops = {
 	.deassert	= meson_reset_deassert,
 };
 
+static int meson_reset_probe(struct device *dev, struct regmap *map,
+			     const struct meson_reset_param *param)
+{
+	unsigned int stride = regmap_get_reg_stride(map);
+	struct meson_reset *data;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->param = param;
+	data->map = map;
+	data->rcdev.owner = dev->driver->owner;
+	data->rcdev.nr_resets = param->reg_count * BITS_PER_BYTE
+		* stride;
+	data->rcdev.ops = &meson_reset_ops;
+	data->rcdev.of_node = dev->of_node;
+
+	return devm_reset_controller_register(dev, &data->rcdev);
+}
+
 static const struct meson_reset_param meson8b_param = {
 	.reg_count	= 8,
 	.reset_offset	= 0x0,
@@ -125,46 +146,38 @@ static const struct regmap_config regmap_config = {
 	.reg_stride = 4,
 };
 
-static int meson_reset_probe(struct platform_device *pdev)
+static int meson_reset_pltf_probe(struct platform_device *pdev)
 {
+
+	const struct meson_reset_param *param;
 	struct device *dev = &pdev->dev;
-	struct meson_reset *data;
+	struct regmap *map;
 	void __iomem *base;
 
-	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
-	data->param = of_device_get_match_data(dev);
-	if (!data->param)
+	param = of_device_get_match_data(dev);
+	if (!param)
 		return -ENODEV;
 
-	data->map = devm_regmap_init_mmio(dev, base, &regmap_config);
-	if (IS_ERR(data->map))
-		return dev_err_probe(dev, PTR_ERR(data->map),
+	map = devm_regmap_init_mmio(dev, base, &regmap_config);
+	if (IS_ERR(map))
+		return dev_err_probe(dev, PTR_ERR(map),
 				     "can't init regmap mmio region\n");
 
-	data->rcdev.owner = THIS_MODULE;
-	data->rcdev.nr_resets = data->param->reg_count * BITS_PER_BYTE
-		* regmap_config.reg_stride;
-	data->rcdev.ops = &meson_reset_ops;
-	data->rcdev.of_node = dev->of_node;
-
-	return devm_reset_controller_register(dev, &data->rcdev);
+	return meson_reset_probe(dev, map, param);
 }
 
-static struct platform_driver meson_reset_driver = {
-	.probe	= meson_reset_probe,
+static struct platform_driver meson_reset_pltf_driver = {
+	.probe	= meson_reset_pltf_probe,
 	.driver = {
 		.name		= "meson_reset",
 		.of_match_table	= meson_reset_dt_ids,
 	},
 };
-module_platform_driver(meson_reset_driver);
+module_platform_driver(meson_reset_pltf_driver);
 
 MODULE_DESCRIPTION("Amlogic Meson Reset Controller driver");
 MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
-- 
2.43.0

[PATCH 4/8] reset: amlogic: use reset number instead of register count

[Jerome Brunet]

The reset driver from audio clock controller may register less
reset than a register can hold. To avoid making any change while
switching to auxiliary support, use the number of reset instead of the
register count to define the bounds of the reset controller.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/reset/reset-meson.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
index fec55321b52b..3e0447366ba6 100644
--- a/drivers/reset/reset-meson.c
+++ b/drivers/reset/reset-meson.c
@@ -17,7 +17,7 @@
 #include <linux/types.h>
 
 struct meson_reset_param {
-	int reg_count;
+	unsigned int reset_num;
 	int reset_offset;
 	int level_offset;
 	bool level_low_reset;
@@ -90,7 +90,6 @@ static const struct reset_control_ops meson_reset_ops = {
 static int meson_reset_probe(struct device *dev, struct regmap *map,
 			     const struct meson_reset_param *param)
 {
-	unsigned int stride = regmap_get_reg_stride(map);
 	struct meson_reset *data;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
@@ -100,8 +99,7 @@ static int meson_reset_probe(struct device *dev, struct regmap *map,
 	data->param = param;
 	data->map = map;
 	data->rcdev.owner = dev->driver->owner;
-	data->rcdev.nr_resets = param->reg_count * BITS_PER_BYTE
-		* stride;
+	data->rcdev.nr_resets = param->reset_num;
 	data->rcdev.ops = &meson_reset_ops;
 	data->rcdev.of_node = dev->of_node;
 
@@ -109,21 +107,21 @@ static int meson_reset_probe(struct device *dev, struct regmap *map,
 }
 
 static const struct meson_reset_param meson8b_param = {
-	.reg_count	= 8,
+	.reset_num	= 256,
 	.reset_offset	= 0x0,
 	.level_offset	= 0x7c,
 	.level_low_reset = true,
 };
 
 static const struct meson_reset_param meson_a1_param = {
-	.reg_count	= 3,
+	.reset_num	= 96,
 	.reset_offset	= 0x0,
 	.level_offset	= 0x40,
 	.level_low_reset = true,
 };
 
 static const struct meson_reset_param meson_s4_param = {
-	.reg_count	= 6,
+	.reset_num	= 192,
 	.reset_offset	= 0x0,
 	.level_offset	= 0x40,
 	.level_low_reset = true,
-- 
2.43.0

[PATCH 5/8] reset: amlogic: add reset status support

[Jerome Brunet]

Add a callback to check the status of the level reset, as done in
the reset driver of the audio clock controller.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/reset/reset-meson.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
index 3e0447366ba6..65ba9190cb53 100644
--- a/drivers/reset/reset-meson.c
+++ b/drivers/reset/reset-meson.c
@@ -69,6 +69,23 @@ static int meson_reset_level(struct reset_controller_dev *rcdev,
 				  BIT(bit), assert ? BIT(bit) : 0);
 }
 
+static int meson_reset_status(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	struct meson_reset *data =
+		container_of(rcdev, struct meson_reset, rcdev);
+	unsigned int val, offset, bit;
+
+	meson_reset_offset_and_bit(data, id, &offset, &bit);
+	offset += data->param->level_offset;
+
+	regmap_read(data->map, offset, &val);
+	val = !!(BIT(bit) & val);
+
+
+	return val ^ data->param->level_low_reset;
+}
+
 static int meson_reset_assert(struct reset_controller_dev *rcdev,
 			      unsigned long id)
 {
@@ -85,6 +102,7 @@ static const struct reset_control_ops meson_reset_ops = {
 	.reset		= meson_reset_reset,
 	.assert		= meson_reset_assert,
 	.deassert	= meson_reset_deassert,
+	.status		= meson_reset_status,
 };
 
 static int meson_reset_probe(struct device *dev, struct regmap *map,
-- 
2.43.0

[PATCH 6/8] reset: amlogic: add toggle reset support

[Jerome Brunet]

Add the emulation for the reset callback using level reset if reset is not
directly supported. This is done to keep the functionality of reset
driver of audio clock controller. This is expected to work by the related
reset consumers.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/reset/reset-meson.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
index 65ba9190cb53..e34a10b15593 100644
--- a/drivers/reset/reset-meson.c
+++ b/drivers/reset/reset-meson.c
@@ -17,6 +17,7 @@
 #include <linux/types.h>
 
 struct meson_reset_param {
+	const struct reset_control_ops *reset_ops;
 	unsigned int reset_num;
 	int reset_offset;
 	int level_offset;
@@ -98,6 +99,18 @@ static int meson_reset_deassert(struct reset_controller_dev *rcdev,
 	return meson_reset_level(rcdev, id, false);
 }
 
+static int meson_reset_level_toggle(struct reset_controller_dev *rcdev,
+				    unsigned long id)
+{
+	int ret;
+
+	ret = meson_reset_assert(rcdev, id);
+	if (ret)
+		return ret;
+
+	return meson_reset_deassert(rcdev, id);
+}
+
 static const struct reset_control_ops meson_reset_ops = {
 	.reset		= meson_reset_reset,
 	.assert		= meson_reset_assert,
@@ -105,6 +118,13 @@ static const struct reset_control_ops meson_reset_ops = {
 	.status		= meson_reset_status,
 };
 
+static const struct reset_control_ops meson_reset_toggle_ops = {
+	.reset		= meson_reset_level_toggle,
+	.assert		= meson_reset_assert,
+	.deassert	= meson_reset_deassert,
+	.status		= meson_reset_status,
+};
+
 static int meson_reset_probe(struct device *dev, struct regmap *map,
 			     const struct meson_reset_param *param)
 {
@@ -118,13 +138,14 @@ static int meson_reset_probe(struct device *dev, struct regmap *map,
 	data->map = map;
 	data->rcdev.owner = dev->driver->owner;
 	data->rcdev.nr_resets = param->reset_num;
-	data->rcdev.ops = &meson_reset_ops;
+	data->rcdev.ops = param->reset_ops;
 	data->rcdev.of_node = dev->of_node;
 
 	return devm_reset_controller_register(dev, &data->rcdev);
 }
 
 static const struct meson_reset_param meson8b_param = {
+	.reset_ops	= &meson_reset_ops,
 	.reset_num	= 256,
 	.reset_offset	= 0x0,
 	.level_offset	= 0x7c,
@@ -132,6 +153,7 @@ static const struct meson_reset_param meson8b_param = {
 };
 
 static const struct meson_reset_param meson_a1_param = {
+	.reset_ops	= &meson_reset_ops,
 	.reset_num	= 96,
 	.reset_offset	= 0x0,
 	.level_offset	= 0x40,
@@ -139,6 +161,7 @@ static const struct meson_reset_param meson_a1_param = {
 };
 
 static const struct meson_reset_param meson_s4_param = {
+	.reset_ops	= &meson_reset_ops,
 	.reset_num	= 192,
 	.reset_offset	= 0x0,
 	.level_offset	= 0x40,
-- 
2.43.0

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

[Jerome Brunet]

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

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 */
-- 
2.43.0

[PATCH 8/8] clk: amlogic: axg-audio: use the auxiliary reset driver

[Jerome Brunet]

Remove the implementation of the reset driver in axg audio
clock driver and migrate to the one provided by reset framework
on the auxiliary bus

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/meson/Kconfig     |   1 +
 drivers/clk/meson/axg-audio.c | 109 +++-------------------------------
 2 files changed, 10 insertions(+), 100 deletions(-)

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index 78f648c9c97d..b1c0c3ba96c4 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -106,6 +106,7 @@ config COMMON_CLK_AXG_AUDIO
 	select COMMON_CLK_MESON_SCLK_DIV
 	select COMMON_CLK_MESON_CLKC_UTILS
 	select REGMAP_MMIO
+	imply RESET_MESON
 	help
 	  Support for the audio clock controller on AmLogic A113D devices,
 	  aka axg, Say Y if you want audio subsystem to work.
diff --git a/drivers/clk/meson/axg-audio.c b/drivers/clk/meson/axg-audio.c
index e03a5bf899c0..6e199e751856 100644
--- a/drivers/clk/meson/axg-audio.c
+++ b/drivers/clk/meson/axg-audio.c
@@ -15,6 +15,8 @@
 #include <linux/reset-controller.h>
 #include <linux/slab.h>
 
+#include <soc/amlogic/meson-auxiliary-reset.h>
+
 #include "meson-clkc-utils.h"
 #include "axg-audio.h"
 #include "clk-regmap.h"
@@ -1648,84 +1650,6 @@ static struct clk_regmap *const sm1_clk_regmaps[] = {
 	&sm1_sysclk_b_en,
 };
 
-struct axg_audio_reset_data {
-	struct reset_controller_dev rstc;
-	struct regmap *map;
-	unsigned int offset;
-};
-
-static void axg_audio_reset_reg_and_bit(struct axg_audio_reset_data *rst,
-					unsigned long id,
-					unsigned int *reg,
-					unsigned int *bit)
-{
-	unsigned int stride = regmap_get_reg_stride(rst->map);
-
-	*reg = (id / (stride * BITS_PER_BYTE)) * stride;
-	*reg += rst->offset;
-	*bit = id % (stride * BITS_PER_BYTE);
-}
-
-static int axg_audio_reset_update(struct reset_controller_dev *rcdev,
-				unsigned long id, bool assert)
-{
-	struct axg_audio_reset_data *rst =
-		container_of(rcdev, struct axg_audio_reset_data, rstc);
-	unsigned int offset, bit;
-
-	axg_audio_reset_reg_and_bit(rst, id, &offset, &bit);
-
-	regmap_update_bits(rst->map, offset, BIT(bit),
-			assert ? BIT(bit) : 0);
-
-	return 0;
-}
-
-static int axg_audio_reset_status(struct reset_controller_dev *rcdev,
-				unsigned long id)
-{
-	struct axg_audio_reset_data *rst =
-		container_of(rcdev, struct axg_audio_reset_data, rstc);
-	unsigned int val, offset, bit;
-
-	axg_audio_reset_reg_and_bit(rst, id, &offset, &bit);
-
-	regmap_read(rst->map, offset, &val);
-
-	return !!(val & BIT(bit));
-}
-
-static int axg_audio_reset_assert(struct reset_controller_dev *rcdev,
-				unsigned long id)
-{
-	return axg_audio_reset_update(rcdev, id, true);
-}
-
-static int axg_audio_reset_deassert(struct reset_controller_dev *rcdev,
-				unsigned long id)
-{
-	return axg_audio_reset_update(rcdev, id, false);
-}
-
-static int axg_audio_reset_toggle(struct reset_controller_dev *rcdev,
-				unsigned long id)
-{
-	int ret;
-
-	ret = axg_audio_reset_assert(rcdev, id);
-	if (ret)
-		return ret;
-
-	return axg_audio_reset_deassert(rcdev, id);
-}
-
-static const struct reset_control_ops axg_audio_rstc_ops = {
-	.assert = axg_audio_reset_assert,
-	.deassert = axg_audio_reset_deassert,
-	.reset = axg_audio_reset_toggle,
-	.status = axg_audio_reset_status,
-};
-
 static const struct regmap_config axg_audio_regmap_cfg = {
 	.reg_bits	= 32,
 	.val_bits	= 32,
@@ -1737,15 +1661,13 @@ struct audioclk_data {
 	struct clk_regmap *const *regmap_clks;
 	unsigned int regmap_clk_num;
 	struct meson_clk_hw_data hw_clks;
-	unsigned int reset_offset;
-	unsigned int reset_num;
+	const char *rst_drvname;
 };
 
 static int axg_audio_clkc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	const struct audioclk_data *data;
-	struct axg_audio_reset_data *rst;
 	struct regmap *map;
 	void __iomem *regs;
 	struct clk_hw *hw;
@@ -1803,22 +1725,11 @@ static int axg_audio_clkc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/* Stop here if there is no reset */
-	if (!data->reset_num)
-		return 0;
-
-	rst = devm_kzalloc(dev, sizeof(*rst), GFP_KERNEL);
-	if (!rst)
-		return -ENOMEM;
-
-	rst->map = map;
-	rst->offset = data->reset_offset;
-	rst->rstc.nr_resets = data->reset_num;
-	rst->rstc.ops = &axg_audio_rstc_ops;
-	rst->rstc.of_node = dev->of_node;
-	rst->rstc.owner = THIS_MODULE;
+	/* Register auxiliary reset driver when applicable */
+	if (data->rst_drvname)
+		ret = devm_meson_rst_aux_register(dev, map, data->rst_drvname);
 
-	return devm_reset_controller_register(dev, &rst->rstc);
+	return ret;
 }
 
 static const struct audioclk_data axg_audioclk_data = {
@@ -1837,8 +1748,7 @@ static const struct audioclk_data g12a_audioclk_data = {
 		.hws = g12a_audio_hw_clks,
 		.num = ARRAY_SIZE(g12a_audio_hw_clks),
 	},
-	.reset_offset = AUDIO_SW_RESET,
-	.reset_num = 26,
+	.rst_drvname = "rst-g12a",
 };
 
 static const struct audioclk_data sm1_audioclk_data = {
@@ -1848,8 +1758,7 @@ static const struct audioclk_data sm1_audioclk_data = {
 		.hws = sm1_audio_hw_clks,
 		.num = ARRAY_SIZE(sm1_audio_hw_clks),
 	},
-	.reset_offset = AUDIO_SM1_SW_RESET0,
-	.reset_num = 39,
+	.rst_drvname = "rst-sm1",
 };
 
 static const struct of_device_id clkc_match_table[] = {
-- 
2.43.0

Re: [PATCH 2/8] reset: amlogic: add driver parameters

[Stephen Boyd]

Quoting Jerome Brunet (2024-07-10 09:25:11)
> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> index 8f3d6e9df235..59126c9f194a 100644
> --- a/drivers/reset/reset-meson.c
> +++ b/drivers/reset/reset-meson.c
> @@ -18,7 +18,9 @@
>  
>  struct meson_reset_param {
>         int reg_count;
> +       int reset_offset;

Make this unsigned?

>         int level_offset;

Probably this one too, but not in this patch.

> +       bool level_low_reset;
>  };
>  
>  struct meson_reset {

Re: [PATCH 3/8] reset: amlogic: split the device and platform probe

[Stephen Boyd]

Quoting Jerome Brunet (2024-07-10 09:25:12)
> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> index 59126c9f194a..fec55321b52b 100644
> --- a/drivers/reset/reset-meson.c
> +++ b/drivers/reset/reset-meson.c
> @@ -87,6 +87,27 @@ static const struct reset_control_ops meson_reset_ops = {
>         .deassert       = meson_reset_deassert,
>  };
>  
> +static int meson_reset_probe(struct device *dev, struct regmap *map,
> +                            const struct meson_reset_param *param)
> +{
> +       unsigned int stride = regmap_get_reg_stride(map);
> +       struct meson_reset *data;
> +
> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->param = param;
> +       data->map = map;
> +       data->rcdev.owner = dev->driver->owner;
> +       data->rcdev.nr_resets = param->reg_count * BITS_PER_BYTE
> +               * stride;

Nitpick: I'd just put this on the line above

> +       data->rcdev.ops = &meson_reset_ops;
> +       data->rcdev.of_node = dev->of_node;
> +
> +       return devm_reset_controller_register(dev, &data->rcdev);
> +}
> +
>  static const struct meson_reset_param meson8b_param = {
>         .reg_count      = 8,
>         .reset_offset   = 0x0,
> @@ -125,46 +146,38 @@ static const struct regmap_config regmap_config = {
>         .reg_stride = 4,
>  };
>  
> -static int meson_reset_probe(struct platform_device *pdev)
> +static int meson_reset_pltf_probe(struct platform_device *pdev)
>  {
> +
> +       const struct meson_reset_param *param;
>         struct device *dev = &pdev->dev;
> -       struct meson_reset *data;
> +       struct regmap *map;
>         void __iomem *base;
>  
> -       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> -       if (!data)
> -               return -ENOMEM;
> -
>         base = devm_platform_ioremap_resource(pdev, 0);
>         if (IS_ERR(base))
>                 return PTR_ERR(base);
>  
> -       data->param = of_device_get_match_data(dev);
> -       if (!data->param)
> +       param = of_device_get_match_data(dev);

Just use device_get_match_data()? I don't see any need to use DT
specific APIs.

> +       if (!param)
>                 return -ENODEV;
>  
> -       data->map = devm_regmap_init_mmio(dev, base, &regmap_config);
> -       if (IS_ERR(data->map))
> -               return dev_err_probe(dev, PTR_ERR(data->map),
> +       map = devm_regmap_init_mmio(dev, base, &regmap_config);
> +       if (IS_ERR(map))
> +               return dev_err_probe(dev, PTR_ERR(map),
>                                      "can't init regmap mmio region\n");
>

Re: [PATCH 5/8] reset: amlogic: add reset status support

[Stephen Boyd]

Quoting Jerome Brunet (2024-07-10 09:25:14)
> Add a callback to check the status of the level reset, as done in
> the reset driver of the audio clock controller.

Why? Presumably so that this driver has equivalent functionality to the
reset code in the audio clk controller?

> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/reset/reset-meson.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> index 3e0447366ba6..65ba9190cb53 100644
> --- a/drivers/reset/reset-meson.c
> +++ b/drivers/reset/reset-meson.c
> @@ -69,6 +69,23 @@ static int meson_reset_level(struct reset_controller_dev *rcdev,
>                                   BIT(bit), assert ? BIT(bit) : 0);
>  }
>  
> +static int meson_reset_status(struct reset_controller_dev *rcdev,
> +                             unsigned long id)
> +{
> +       struct meson_reset *data =
> +               container_of(rcdev, struct meson_reset, rcdev);

Nitpick: One line.

> +       unsigned int val, offset, bit;
> +
> +       meson_reset_offset_and_bit(data, id, &offset, &bit);
> +       offset += data->param->level_offset;
> +
> +       regmap_read(data->map, offset, &val);
> +       val = !!(BIT(bit) & val);
> +
> +

Nitpick: Drop the extra newline?

> +       return val ^ data->param->level_low_reset;
> +}
> +
>  static int meson_reset_assert(struct reset_controller_dev *rcdev,
>                               unsigned long id)

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

[Stephen Boyd]

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?

Re: [PATCH 5/8] reset: amlogic: add reset status support

[Jerome Brunet]

On Wed 10 Jul 2024 at 15:40, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Jerome Brunet (2024-07-10 09:25:14)
>> Add a callback to check the status of the level reset, as done in
>> the reset driver of the audio clock controller.
>
> Why? Presumably so that this driver has equivalent functionality to the
> reset code in the audio clk controller?

I thought the description was saying so. I'll be more explicit in v2

>
>> 
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/reset/reset-meson.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>> 
>> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
>> index 3e0447366ba6..65ba9190cb53 100644
>> --- a/drivers/reset/reset-meson.c
>> +++ b/drivers/reset/reset-meson.c
>> @@ -69,6 +69,23 @@ static int meson_reset_level(struct reset_controller_dev *rcdev,
>>                                   BIT(bit), assert ? BIT(bit) : 0);
>>  }
>>  
>> +static int meson_reset_status(struct reset_controller_dev *rcdev,
>> +                             unsigned long id)
>> +{
>> +       struct meson_reset *data =
>> +               container_of(rcdev, struct meson_reset, rcdev);
>
> Nitpick: One line.
>
>> +       unsigned int val, offset, bit;
>> +
>> +       meson_reset_offset_and_bit(data, id, &offset, &bit);
>> +       offset += data->param->level_offset;
>> +
>> +       regmap_read(data->map, offset, &val);
>> +       val = !!(BIT(bit) & val);
>> +
>> +
>
> Nitpick: Drop the extra newline?
>
>> +       return val ^ data->param->level_low_reset;
>> +}
>> +
>>  static int meson_reset_assert(struct reset_controller_dev *rcdev,
>>                               unsigned long id)

-- 
Jerome

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

[Jerome Brunet]

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.

-- 
Jerome

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Stephen Boyd]

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.

Re: [PATCH 0/8] reset: amlogic: move audio reset drivers out of CCF

[Jan Dakinevich]

Jerome, I have tested the series on SM1 SoC (Amlogic AC200 ref board).
As far as I can tell, it works as it should


On 7/10/24 19:25, Jerome Brunet wrote:
> This patchset follows the discussion about having reset driver in the
> clock tree [1]. Ideally those should reside in the reset part of tree.
> 
> Also the code of the amlogic reset driver is very similar between the 2
> trees and could use the same driver code.
> 
> This patchset moves the reset driver of audio clock controller of the
> g12 and sm1 SoC family to the reset tree, using the auxiliary bus.
> 
> The infrastructure put in place is meant to be generic enough so we may
> eventually also move the reset drivers in the meson8b and aoclk clock
> controllers.
> 
> Change since RFC [2]:
>  * Move the aux registration helper out of clock too.
> 
> [1] https://lore.kernel.org/linux-clk/e3a85852b911fdf16dd9ae158f42b3ef.sboyd@kernel.org
> [2] https://lore.kernel.org/linux-clk/20240516150842.705844-1-jbrunet@baylibre.com
> 
> Jerome Brunet (8):
>   reset: amlogic: convert driver to regmap
>   reset: amlogic: add driver parameters
>   reset: amlogic: split the device and platform probe
>   reset: amlogic: use reset number instead of register count
>   reset: amlogic: add reset status support
>   reset: amlogic: add toggle reset support
>   reset: amlogic: add auxiliary reset driver support
>   clk: amlogic: axg-audio: use the auxiliary reset driver
> 
>  drivers/clk/meson/Kconfig                   |   1 +
>  drivers/clk/meson/axg-audio.c               | 109 +-------
>  drivers/reset/Kconfig                       |   1 +
>  drivers/reset/reset-meson.c                 | 285 ++++++++++++++++----
>  include/soc/amlogic/meson-auxiliary-reset.h |  23 ++
>  5 files changed, 271 insertions(+), 148 deletions(-)
>  create mode 100644 include/soc/amlogic/meson-auxiliary-reset.h
> 

-- 
Best regards
Jan Dakinevich

Re: [PATCH 3/8] reset: amlogic: split the device and platform probe

[Jan Dakinevich]

On 7/10/24 19:25, Jerome Brunet wrote:
> To prepare the addition of the auxiliary device support, split
> out the device probe from the probe of the platform device.
> 
> The device probe will be common to both the platform and auxiliary
> driver.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/reset/reset-meson.c | 55 +++++++++++++++++++++++--------------
>  1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> index 59126c9f194a..fec55321b52b 100644
> --- a/drivers/reset/reset-meson.c
> +++ b/drivers/reset/reset-meson.c
> @@ -87,6 +87,27 @@ static const struct reset_control_ops meson_reset_ops = {
>  	.deassert	= meson_reset_deassert,
>  };
>  
> +static int meson_reset_probe(struct device *dev, struct regmap *map,
> +			     const struct meson_reset_param *param)
> +{
> +	unsigned int stride = regmap_get_reg_stride(map);
> +	struct meson_reset *data;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->param = param;
> +	data->map = map;
> +	data->rcdev.owner = dev->driver->owner;
> +	data->rcdev.nr_resets = param->reg_count * BITS_PER_BYTE
> +		* stride;
> +	data->rcdev.ops = &meson_reset_ops;
> +	data->rcdev.of_node = dev->of_node;

It will be good to add here something like this. Later it would help in
reset debugging.

data->rcdev.dev = dev;

> +
> +	return devm_reset_controller_register(dev, &data->rcdev);
> +}
> +
>  static const struct meson_reset_param meson8b_param = {
>  	.reg_count	= 8,
>  	.reset_offset	= 0x0,
> @@ -125,46 +146,38 @@ static const struct regmap_config regmap_config = {
>  	.reg_stride = 4,
>  };
>  
> -static int meson_reset_probe(struct platform_device *pdev)
> +static int meson_reset_pltf_probe(struct platform_device *pdev)
>  {
> +
> +	const struct meson_reset_param *param;
>  	struct device *dev = &pdev->dev;
> -	struct meson_reset *data;
> +	struct regmap *map;
>  	void __iomem *base;
>  
> -	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -
>  	base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
>  
> -	data->param = of_device_get_match_data(dev);
> -	if (!data->param)
> +	param = of_device_get_match_data(dev);
> +	if (!param)
>  		return -ENODEV;
>  
> -	data->map = devm_regmap_init_mmio(dev, base, &regmap_config);
> -	if (IS_ERR(data->map))
> -		return dev_err_probe(dev, PTR_ERR(data->map),
> +	map = devm_regmap_init_mmio(dev, base, &regmap_config);
> +	if (IS_ERR(map))
> +		return dev_err_probe(dev, PTR_ERR(map),
>  				     "can't init regmap mmio region\n");
>  
> -	data->rcdev.owner = THIS_MODULE;
> -	data->rcdev.nr_resets = data->param->reg_count * BITS_PER_BYTE
> -		* regmap_config.reg_stride;
> -	data->rcdev.ops = &meson_reset_ops;
> -	data->rcdev.of_node = dev->of_node;
> -
> -	return devm_reset_controller_register(dev, &data->rcdev);
> +	return meson_reset_probe(dev, map, param);
>  }
>  
> -static struct platform_driver meson_reset_driver = {
> -	.probe	= meson_reset_probe,
> +static struct platform_driver meson_reset_pltf_driver = {
> +	.probe	= meson_reset_pltf_probe,
>  	.driver = {
>  		.name		= "meson_reset",
>  		.of_match_table	= meson_reset_dt_ids,
>  	},
>  };
> -module_platform_driver(meson_reset_driver);
> +module_platform_driver(meson_reset_pltf_driver);
>  
>  MODULE_DESCRIPTION("Amlogic Meson Reset Controller driver");
>  MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");

-- 
Best regards
Jan Dakinevich

Re: [PATCH 3/8] reset: amlogic: split the device and platform probe

[Jerome Brunet]

On Tue 16 Jul 2024 at 01:48, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:

> On 7/10/24 19:25, Jerome Brunet wrote:
>> To prepare the addition of the auxiliary device support, split
>> out the device probe from the probe of the platform device.
>> 
>> The device probe will be common to both the platform and auxiliary
>> driver.
>> 
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/reset/reset-meson.c | 55 +++++++++++++++++++++++--------------
>>  1 file changed, 34 insertions(+), 21 deletions(-)
>> 
>> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
>> index 59126c9f194a..fec55321b52b 100644
>> --- a/drivers/reset/reset-meson.c
>> +++ b/drivers/reset/reset-meson.c
>> @@ -87,6 +87,27 @@ static const struct reset_control_ops meson_reset_ops = {
>>  	.deassert	= meson_reset_deassert,
>>  };
>>  
>> +static int meson_reset_probe(struct device *dev, struct regmap *map,
>> +			     const struct meson_reset_param *param)
>> +{
>> +	unsigned int stride = regmap_get_reg_stride(map);
>> +	struct meson_reset *data;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->param = param;
>> +	data->map = map;
>> +	data->rcdev.owner = dev->driver->owner;
>> +	data->rcdev.nr_resets = param->reg_count * BITS_PER_BYTE
>> +		* stride;
>> +	data->rcdev.ops = &meson_reset_ops;
>> +	data->rcdev.of_node = dev->of_node;
>
> It will be good to add here something like this. Later it would help in
> reset debugging.
>
> data->rcdev.dev = dev;

That is not the purpose of this change.
I'm merely re-organizing exiting code, not changing it.

Plus, if you refering to rcdev_name(), we already populate
rcdev->of_node, so a name is provided.

>
>> +
>> +	return devm_reset_controller_register(dev, &data->rcdev);
>> +}
>> +
>>  static const struct meson_reset_param meson8b_param = {
>>  	.reg_count	= 8,
>>  	.reset_offset	= 0x0,
>> @@ -125,46 +146,38 @@ static const struct regmap_config regmap_config = {
>>  	.reg_stride = 4,
>>  };
>>  
>> -static int meson_reset_probe(struct platform_device *pdev)
>> +static int meson_reset_pltf_probe(struct platform_device *pdev)
>>  {
>> +
>> +	const struct meson_reset_param *param;
>>  	struct device *dev = &pdev->dev;
>> -	struct meson_reset *data;
>> +	struct regmap *map;
>>  	void __iomem *base;
>>  
>> -	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> -	if (!data)
>> -		return -ENOMEM;
>> -
>>  	base = devm_platform_ioremap_resource(pdev, 0);
>>  	if (IS_ERR(base))
>>  		return PTR_ERR(base);
>>  
>> -	data->param = of_device_get_match_data(dev);
>> -	if (!data->param)
>> +	param = of_device_get_match_data(dev);
>> +	if (!param)
>>  		return -ENODEV;
>>  
>> -	data->map = devm_regmap_init_mmio(dev, base, &regmap_config);
>> -	if (IS_ERR(data->map))
>> -		return dev_err_probe(dev, PTR_ERR(data->map),
>> +	map = devm_regmap_init_mmio(dev, base, &regmap_config);
>> +	if (IS_ERR(map))
>> +		return dev_err_probe(dev, PTR_ERR(map),
>>  				     "can't init regmap mmio region\n");
>>  
>> -	data->rcdev.owner = THIS_MODULE;
>> -	data->rcdev.nr_resets = data->param->reg_count * BITS_PER_BYTE
>> -		* regmap_config.reg_stride;
>> -	data->rcdev.ops = &meson_reset_ops;
>> -	data->rcdev.of_node = dev->of_node;
>> -
>> -	return devm_reset_controller_register(dev, &data->rcdev);
>> +	return meson_reset_probe(dev, map, param);
>>  }
>>  
>> -static struct platform_driver meson_reset_driver = {
>> -	.probe	= meson_reset_probe,
>> +static struct platform_driver meson_reset_pltf_driver = {
>> +	.probe	= meson_reset_pltf_probe,
>>  	.driver = {
>>  		.name		= "meson_reset",
>>  		.of_match_table	= meson_reset_dt_ids,
>>  	},
>>  };
>> -module_platform_driver(meson_reset_driver);
>> +module_platform_driver(meson_reset_pltf_driver);
>>  
>>  MODULE_DESCRIPTION("Amlogic Meson Reset Controller driver");
>>  MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");

-- 
Jerome

Re: [PATCH 1/8] reset: amlogic: convert driver to regmap

[Jan Dakinevich]

On 7/10/24 19:25, Jerome Brunet wrote:
> To allow using the same driver for the main reset controller and the
> auxiliary ones embedded in the clock controllers, convert the
> the Amlogic reset driver to regmap.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/reset/reset-meson.c | 80 ++++++++++++++++++++-----------------
>  1 file changed, 44 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> index f78be97898bc..8f3d6e9df235 100644
> --- a/drivers/reset/reset-meson.c
> +++ b/drivers/reset/reset-meson.c
> @@ -11,36 +11,44 @@
>  #include <linux/of.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>  #include <linux/reset-controller.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  
> -#define BITS_PER_REG	32
> -
>  struct meson_reset_param {
>  	int reg_count;
>  	int level_offset;
>  };
>  
>  struct meson_reset {
> -	void __iomem *reg_base;
>  	const struct meson_reset_param *param;
>  	struct reset_controller_dev rcdev;
> -	spinlock_t lock;
> +	struct regmap *map;
>  };
>  
> +static void meson_reset_offset_and_bit(struct meson_reset *data,
> +				       unsigned long id,
> +				       unsigned int *offset,
> +				       unsigned int *bit)
> +{
> +	unsigned int stride = regmap_get_reg_stride(data->map);
> +
> +	*offset = (id / (stride * BITS_PER_BYTE)) * stride;
> +	*bit = id % (stride * BITS_PER_BYTE);
> +}
> +
>  static int meson_reset_reset(struct reset_controller_dev *rcdev,
> -			      unsigned long id)
> +			     unsigned long id)
>  {
>  	struct meson_reset *data =
>  		container_of(rcdev, struct meson_reset, rcdev);
> -	unsigned int bank = id / BITS_PER_REG;
> -	unsigned int offset = id % BITS_PER_REG;
> -	void __iomem *reg_addr = data->reg_base + (bank << 2);
> +	unsigned int offset, bit;
>  
> -	writel(BIT(offset), reg_addr);
> +	meson_reset_offset_and_bit(data, id, &offset, &bit);
>  
> -	return 0;
> +	return regmap_update_bits(data->map, offset,
> +				  BIT(bit), BIT(bit));

regmap_update_bits() is not equal to writel(), I suppose regmap_write()
should be here.

I'm still under testing, but I see unexpected behavior on A1 SoC:

[    1.482446] Call trace:
[    1.484860]  dump_backtrace+0x94/0xec
[    1.488482]  show_stack+0x18/0x24
[    1.491754]  dump_stack_lvl+0x78/0x90
[    1.495377]  dump_stack+0x18/0x24
[    1.498654]  __report_bad_irq+0x38/0xe4
[    1.502453]  note_interrupt+0x324/0x374
[    1.506244]  handle_irq_event+0x9c/0xac
[    1.510039]  handle_fasteoi_irq+0xa4/0x238
[    1.514093]  generic_handle_domain_irq+0x2c/0x44
[    1.518664]  gic_handle_irq+0x40/0xc4
[    1.522287]  call_on_irq_stack+0x24/0x4c
[    1.526168]  do_interrupt_handler+0x80/0x84
[    1.530308]  el1_interrupt+0x34/0x68
[    1.533844]  el1h_64_irq_handler+0x18/0x24
[    1.537898]  el1h_64_irq+0x64/0x68
[    1.541262]  default_idle_call+0x28/0x3c
[    1.545143]  do_idle+0x208/0x260
[    1.548334]  cpu_startup_entry+0x38/0x3c
[    1.552220]  kernel_init+0x0/0x1dc
[    1.555579]  start_kernel+0x5ac/0x6f0
[    1.559206]  __primary_switched+0x80/0x88


and using of regmap_write() (apparently) fixes it.

>  }
>  
>  static int meson_reset_level(struct reset_controller_dev *rcdev,
> @@ -48,25 +56,13 @@ static int meson_reset_level(struct reset_controller_dev *rcdev,
>  {
>  	struct meson_reset *data =
>  		container_of(rcdev, struct meson_reset, rcdev);
> -	unsigned int bank = id / BITS_PER_REG;
> -	unsigned int offset = id % BITS_PER_REG;
> -	void __iomem *reg_addr;
> -	unsigned long flags;
> -	u32 reg;
> +	unsigned int offset, bit;
>  
> -	reg_addr = data->reg_base + data->param->level_offset + (bank << 2);
> +	meson_reset_offset_and_bit(data, id, &offset, &bit);
> +	offset += data->param->level_offset;
>  
> -	spin_lock_irqsave(&data->lock, flags);
> -
> -	reg = readl(reg_addr);
> -	if (assert)
> -		writel(reg & ~BIT(offset), reg_addr);
> -	else
> -		writel(reg | BIT(offset), reg_addr);
> -
> -	spin_unlock_irqrestore(&data->lock, flags);
> -
> -	return 0;
> +	return regmap_update_bits(data->map, offset,
> +				  BIT(bit), assert ? 0 : BIT(bit));
>  }
>  
>  static int meson_reset_assert(struct reset_controller_dev *rcdev,
> @@ -113,30 +109,42 @@ static const struct of_device_id meson_reset_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, meson_reset_dt_ids);
>  
> +static const struct regmap_config regmap_config = {
> +	.reg_bits   = 32,
> +	.val_bits   = 32,
> +	.reg_stride = 4,
> +};
> +
>  static int meson_reset_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct meson_reset *data;
> +	void __iomem *base;
>  
> -	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> -	data->reg_base = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(data->reg_base))
> -		return PTR_ERR(data->reg_base);
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
>  
> -	data->param = of_device_get_match_data(&pdev->dev);
> +	data->param = of_device_get_match_data(dev);
>  	if (!data->param)
>  		return -ENODEV;
>  
> -	spin_lock_init(&data->lock);
> +	data->map = devm_regmap_init_mmio(dev, base, &regmap_config);
> +	if (IS_ERR(data->map))
> +		return dev_err_probe(dev, PTR_ERR(data->map),
> +				     "can't init regmap mmio region\n");
>  
>  	data->rcdev.owner = THIS_MODULE;
> -	data->rcdev.nr_resets = data->param->reg_count * BITS_PER_REG;
> +	data->rcdev.nr_resets = data->param->reg_count * BITS_PER_BYTE
> +		* regmap_config.reg_stride;
>  	data->rcdev.ops = &meson_reset_ops;
> -	data->rcdev.of_node = pdev->dev.of_node;
> +	data->rcdev.of_node = dev->of_node;
>  
> -	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
> +	return devm_reset_controller_register(dev, &data->rcdev);
>  }
>  
>  static struct platform_driver meson_reset_driver = {

-- 
Best regards
Jan Dakinevich

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

[Jerome Brunet]

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

-- 
Jerome

Re: [PATCH 1/8] reset: amlogic: convert driver to regmap

[Jerome Brunet]

On Thu 18 Jul 2024 at 05:39, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:

> On 7/10/24 19:25, Jerome Brunet wrote:
>> To allow using the same driver for the main reset controller and the
>> auxiliary ones embedded in the clock controllers, convert the
>> the Amlogic reset driver to regmap.
>> 
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/reset/reset-meson.c | 80 ++++++++++++++++++++-----------------
>>  1 file changed, 44 insertions(+), 36 deletions(-)
>> 
>> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
>> index f78be97898bc..8f3d6e9df235 100644
>> --- a/drivers/reset/reset-meson.c
>> +++ b/drivers/reset/reset-meson.c
>> @@ -11,36 +11,44 @@
>>  #include <linux/of.h>
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>>  #include <linux/reset-controller.h>
>>  #include <linux/slab.h>
>>  #include <linux/types.h>
>>  
>> -#define BITS_PER_REG	32
>> -
>>  struct meson_reset_param {
>>  	int reg_count;
>>  	int level_offset;
>>  };
>>  
>>  struct meson_reset {
>> -	void __iomem *reg_base;
>>  	const struct meson_reset_param *param;
>>  	struct reset_controller_dev rcdev;
>> -	spinlock_t lock;
>> +	struct regmap *map;
>>  };
>>  
>> +static void meson_reset_offset_and_bit(struct meson_reset *data,
>> +				       unsigned long id,
>> +				       unsigned int *offset,
>> +				       unsigned int *bit)
>> +{
>> +	unsigned int stride = regmap_get_reg_stride(data->map);
>> +
>> +	*offset = (id / (stride * BITS_PER_BYTE)) * stride;
>> +	*bit = id % (stride * BITS_PER_BYTE);
>> +}
>> +
>>  static int meson_reset_reset(struct reset_controller_dev *rcdev,
>> -			      unsigned long id)
>> +			     unsigned long id)
>>  {
>>  	struct meson_reset *data =
>>  		container_of(rcdev, struct meson_reset, rcdev);
>> -	unsigned int bank = id / BITS_PER_REG;
>> -	unsigned int offset = id % BITS_PER_REG;
>> -	void __iomem *reg_addr = data->reg_base + (bank << 2);
>> +	unsigned int offset, bit;
>>  
>> -	writel(BIT(offset), reg_addr);
>> +	meson_reset_offset_and_bit(data, id, &offset, &bit);
>>  
>> -	return 0;
>> +	return regmap_update_bits(data->map, offset,
>> +				  BIT(bit), BIT(bit));
>
> regmap_update_bits() is not equal to writel(), I suppose regmap_write()
> should be here.
>
> I'm still under testing, but I see unexpected behavior on A1 SoC:
>
> [    1.482446] Call trace:
> [    1.484860]  dump_backtrace+0x94/0xec
> [    1.488482]  show_stack+0x18/0x24
> [    1.491754]  dump_stack_lvl+0x78/0x90
> [    1.495377]  dump_stack+0x18/0x24
> [    1.498654]  __report_bad_irq+0x38/0xe4
> [    1.502453]  note_interrupt+0x324/0x374
> [    1.506244]  handle_irq_event+0x9c/0xac
> [    1.510039]  handle_fasteoi_irq+0xa4/0x238
> [    1.514093]  generic_handle_domain_irq+0x2c/0x44
> [    1.518664]  gic_handle_irq+0x40/0xc4
> [    1.522287]  call_on_irq_stack+0x24/0x4c
> [    1.526168]  do_interrupt_handler+0x80/0x84
> [    1.530308]  el1_interrupt+0x34/0x68
> [    1.533844]  el1h_64_irq_handler+0x18/0x24
> [    1.537898]  el1h_64_irq+0x64/0x68
> [    1.541262]  default_idle_call+0x28/0x3c
> [    1.545143]  do_idle+0x208/0x260
> [    1.548334]  cpu_startup_entry+0x38/0x3c
> [    1.552220]  kernel_init+0x0/0x1dc
> [    1.555579]  start_kernel+0x5ac/0x6f0
> [    1.559206]  __primary_switched+0x80/0x88

That stack trace is not helping

>
>
> and using of regmap_write() (apparently) fixes it.

Nor does that conclusion.

It is perfectly possible I have made mistake somewhere (I have already
fixed one in v2), but sending incomplete report like this, with
unargumented conclusion is just noise in the end.

No, there is no situation in which `regmap_write` would solve a problem
with `regmap_update_bits`.

Either send a full analysis of the problem you found, if you did one, or
at least send the full dump, explaining that you don't know what is
happening.

I'll also point that I did not send g12 clock along with v1 and that a1
support is not upstream or even reviewed. So this 'tests' obviously rely
on an integration of v1, rfc and your custom develpment. That is hardly a
reference at this point.

This patches address g12 and sm1. If you want to contribute with tests,
please test on these platforms. a1 will be addressed in due time.

Testing is good but making complete report (trace and environment
details) is equally important, especially for maintainers who might not
be following Amlogic development as much as you do.

>
>>  }
>>  
>>  static int meson_reset_level(struct reset_controller_dev *rcdev,
>> @@ -48,25 +56,13 @@ static int meson_reset_level(struct reset_controller_dev *rcdev,
>>  {
>>  	struct meson_reset *data =
>>  		container_of(rcdev, struct meson_reset, rcdev);
>> -	unsigned int bank = id / BITS_PER_REG;
>> -	unsigned int offset = id % BITS_PER_REG;
>> -	void __iomem *reg_addr;
>> -	unsigned long flags;
>> -	u32 reg;
>> +	unsigned int offset, bit;
>>  
>> -	reg_addr = data->reg_base + data->param->level_offset + (bank << 2);
>> +	meson_reset_offset_and_bit(data, id, &offset, &bit);
>> +	offset += data->param->level_offset;
>>  
>> -	spin_lock_irqsave(&data->lock, flags);
>> -
>> -	reg = readl(reg_addr);
>> -	if (assert)
>> -		writel(reg & ~BIT(offset), reg_addr);
>> -	else
>> -		writel(reg | BIT(offset), reg_addr);
>> -
>> -	spin_unlock_irqrestore(&data->lock, flags);
>> -
>> -	return 0;
>> +	return regmap_update_bits(data->map, offset,
>> +				  BIT(bit), assert ? 0 : BIT(bit));
>>  }
>>  
>>  static int meson_reset_assert(struct reset_controller_dev *rcdev,
>> @@ -113,30 +109,42 @@ static const struct of_device_id meson_reset_dt_ids[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, meson_reset_dt_ids);
>>  
>> +static const struct regmap_config regmap_config = {
>> +	.reg_bits   = 32,
>> +	.val_bits   = 32,
>> +	.reg_stride = 4,
>> +};
>> +
>>  static int meson_reset_probe(struct platform_device *pdev)
>>  {
>> +	struct device *dev = &pdev->dev;
>>  	struct meson_reset *data;
>> +	void __iomem *base;
>>  
>> -	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>  	if (!data)
>>  		return -ENOMEM;
>>  
>> -	data->reg_base = devm_platform_ioremap_resource(pdev, 0);
>> -	if (IS_ERR(data->reg_base))
>> -		return PTR_ERR(data->reg_base);
>> +	base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>>  
>> -	data->param = of_device_get_match_data(&pdev->dev);
>> +	data->param = of_device_get_match_data(dev);
>>  	if (!data->param)
>>  		return -ENODEV;
>>  
>> -	spin_lock_init(&data->lock);
>> +	data->map = devm_regmap_init_mmio(dev, base, &regmap_config);
>> +	if (IS_ERR(data->map))
>> +		return dev_err_probe(dev, PTR_ERR(data->map),
>> +				     "can't init regmap mmio region\n");
>>  
>>  	data->rcdev.owner = THIS_MODULE;
>> -	data->rcdev.nr_resets = data->param->reg_count * BITS_PER_REG;
>> +	data->rcdev.nr_resets = data->param->reg_count * BITS_PER_BYTE
>> +		* regmap_config.reg_stride;
>>  	data->rcdev.ops = &meson_reset_ops;
>> -	data->rcdev.of_node = pdev->dev.of_node;
>> +	data->rcdev.of_node = dev->of_node;
>>  
>> -	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
>> +	return devm_reset_controller_register(dev, &data->rcdev);
>>  }
>>  
>>  static struct platform_driver meson_reset_driver = {

-- 
Jerome

Re: [PATCH 1/8] reset: amlogic: convert driver to regmap

[Jan Dakinevich]

On 7/18/24 10:19, Jerome Brunet wrote:
> On Thu 18 Jul 2024 at 05:39, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:
> 
>> On 7/10/24 19:25, Jerome Brunet wrote:
>>> To allow using the same driver for the main reset controller and the
>>> auxiliary ones embedded in the clock controllers, convert the
>>> the Amlogic reset driver to regmap.
>>>
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>> ---
>>>  drivers/reset/reset-meson.c | 80 ++++++++++++++++++++-----------------
>>>  1 file changed, 44 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
>>> index f78be97898bc..8f3d6e9df235 100644
>>> --- a/drivers/reset/reset-meson.c
>>> +++ b/drivers/reset/reset-meson.c
>>> @@ -11,36 +11,44 @@
>>>  #include <linux/of.h>
>>>  #include <linux/module.h>
>>>  #include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>>  #include <linux/reset-controller.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/types.h>
>>>  
>>> -#define BITS_PER_REG	32
>>> -
>>>  struct meson_reset_param {
>>>  	int reg_count;
>>>  	int level_offset;
>>>  };
>>>  
>>>  struct meson_reset {
>>> -	void __iomem *reg_base;
>>>  	const struct meson_reset_param *param;
>>>  	struct reset_controller_dev rcdev;
>>> -	spinlock_t lock;
>>> +	struct regmap *map;
>>>  };
>>>  
>>> +static void meson_reset_offset_and_bit(struct meson_reset *data,
>>> +				       unsigned long id,
>>> +				       unsigned int *offset,
>>> +				       unsigned int *bit)
>>> +{
>>> +	unsigned int stride = regmap_get_reg_stride(data->map);
>>> +
>>> +	*offset = (id / (stride * BITS_PER_BYTE)) * stride;
>>> +	*bit = id % (stride * BITS_PER_BYTE);
>>> +}
>>> +
>>>  static int meson_reset_reset(struct reset_controller_dev *rcdev,
>>> -			      unsigned long id)
>>> +			     unsigned long id)
>>>  {
>>>  	struct meson_reset *data =
>>>  		container_of(rcdev, struct meson_reset, rcdev);
>>> -	unsigned int bank = id / BITS_PER_REG;
>>> -	unsigned int offset = id % BITS_PER_REG;
>>> -	void __iomem *reg_addr = data->reg_base + (bank << 2);
>>> +	unsigned int offset, bit;
>>>  
>>> -	writel(BIT(offset), reg_addr);
>>> +	meson_reset_offset_and_bit(data, id, &offset, &bit);
>>>  
>>> -	return 0;
>>> +	return regmap_update_bits(data->map, offset,
>>> +				  BIT(bit), BIT(bit));
>>
>> regmap_update_bits() is not equal to writel(), I suppose regmap_write()
>> should be here.
>>
>> I'm still under testing, but I see unexpected behavior on A1 SoC:
>>
>> [    1.482446] Call trace:
>> [    1.484860]  dump_backtrace+0x94/0xec
>> [    1.488482]  show_stack+0x18/0x24
>> [    1.491754]  dump_stack_lvl+0x78/0x90
>> [    1.495377]  dump_stack+0x18/0x24
>> [    1.498654]  __report_bad_irq+0x38/0xe4
>> [    1.502453]  note_interrupt+0x324/0x374
>> [    1.506244]  handle_irq_event+0x9c/0xac
>> [    1.510039]  handle_fasteoi_irq+0xa4/0x238
>> [    1.514093]  generic_handle_domain_irq+0x2c/0x44
>> [    1.518664]  gic_handle_irq+0x40/0xc4
>> [    1.522287]  call_on_irq_stack+0x24/0x4c
>> [    1.526168]  do_interrupt_handler+0x80/0x84
>> [    1.530308]  el1_interrupt+0x34/0x68
>> [    1.533844]  el1h_64_irq_handler+0x18/0x24
>> [    1.537898]  el1h_64_irq+0x64/0x68
>> [    1.541262]  default_idle_call+0x28/0x3c
>> [    1.545143]  do_idle+0x208/0x260
>> [    1.548334]  cpu_startup_entry+0x38/0x3c
>> [    1.552220]  kernel_init+0x0/0x1dc
>> [    1.555579]  start_kernel+0x5ac/0x6f0
>> [    1.559206]  __primary_switched+0x80/0x88
> 
> That stack trace is not helping
> >>
>>
>> and using of regmap_write() (apparently) fixes it.
> 
> Nor does that conclusion.
> > It is perfectly possible I have made mistake somewhere (I have already
> fixed one in v2), but sending incomplete report like this, with
> unargumented conclusion is just noise in the end.
> 
> No, there is no situation in which `regmap_write` would solve a problem
> with `regmap_update_bits`.
> 

What is the default regs' value of this reset controller? The doc that I
have doesn't clearly specifies it, but it tells that "the reset will
auto-cover to 0 by HW". However pr_info() say that there is 0xffffffff
before write (I am talking about A1).

Also we know, that reset is triggered by writing 1 to specific bit. So,
what will happen if 1 will be written where we did not intend to write it?

> Either send a full analysis of the problem you found, if you did one, or
> at least send the full dump, explaining that you don't know what is
> happening.
> 

Full analysis is following:
- using regmap_update_bits() instead of writel() is incorrect because
this changes the behavior of the driver
- regmap_update_bits() should not be used here because default value of
regs isn't taken into account and (_apparently_, the doc is terse) these
regs could be updated by hw itself.

> I'll also point that I did not send g12 clock along with v1 and that a1
> support is not upstream or even reviewed. So this 'tests' obviously rely
> on an integration of v1, rfc and your custom develpment. That is hardly a
> reference at this point.
> 
> This patches address g12 and sm1. If you want to contribute with tests,
> please test on these platforms. a1 will be addressed in due time.
> 

Where did you find in my words any reference to not upstreamed
functionality? Support of A1 SoC is here since commit bdb369e1e98a
("reset: add support for the Meson-A1 SoC Reset Controller").

> Testing is good but making complete report (trace and environment
> details) is equally important, especially for maintainers who might not
> be following Amlogic development as much as you do.
> 
>>
>>>  }
>>>  
>>>  static int meson_reset_level(struct reset_controller_dev *rcdev,
>>> @@ -48,25 +56,13 @@ static int meson_reset_level(struct reset_controller_dev *rcdev,
>>>  {
>>>  	struct meson_reset *data =
>>>  		container_of(rcdev, struct meson_reset, rcdev);
>>> -	unsigned int bank = id / BITS_PER_REG;
>>> -	unsigned int offset = id % BITS_PER_REG;
>>> -	void __iomem *reg_addr;
>>> -	unsigned long flags;
>>> -	u32 reg;
>>> +	unsigned int offset, bit;
>>>  
>>> -	reg_addr = data->reg_base + data->param->level_offset + (bank << 2);
>>> +	meson_reset_offset_and_bit(data, id, &offset, &bit);
>>> +	offset += data->param->level_offset;
>>>  
>>> -	spin_lock_irqsave(&data->lock, flags);
>>> -
>>> -	reg = readl(reg_addr);
>>> -	if (assert)
>>> -		writel(reg & ~BIT(offset), reg_addr);
>>> -	else
>>> -		writel(reg | BIT(offset), reg_addr);
>>> -
>>> -	spin_unlock_irqrestore(&data->lock, flags);
>>> -
>>> -	return 0;
>>> +	return regmap_update_bits(data->map, offset,
>>> +				  BIT(bit), assert ? 0 : BIT(bit));
>>>  }
>>>  
>>>  static int meson_reset_assert(struct reset_controller_dev *rcdev,
>>> @@ -113,30 +109,42 @@ static const struct of_device_id meson_reset_dt_ids[] = {
>>>  };
>>>  MODULE_DEVICE_TABLE(of, meson_reset_dt_ids);
>>>  
>>> +static const struct regmap_config regmap_config = {
>>> +	.reg_bits   = 32,
>>> +	.val_bits   = 32,
>>> +	.reg_stride = 4,
>>> +};
>>> +
>>>  static int meson_reset_probe(struct platform_device *pdev)
>>>  {
>>> +	struct device *dev = &pdev->dev;
>>>  	struct meson_reset *data;
>>> +	void __iomem *base;
>>>  
>>> -	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>>  	if (!data)
>>>  		return -ENOMEM;
>>>  
>>> -	data->reg_base = devm_platform_ioremap_resource(pdev, 0);
>>> -	if (IS_ERR(data->reg_base))
>>> -		return PTR_ERR(data->reg_base);
>>> +	base = devm_platform_ioremap_resource(pdev, 0);
>>> +	if (IS_ERR(base))
>>> +		return PTR_ERR(base);
>>>  
>>> -	data->param = of_device_get_match_data(&pdev->dev);
>>> +	data->param = of_device_get_match_data(dev);
>>>  	if (!data->param)
>>>  		return -ENODEV;
>>>  
>>> -	spin_lock_init(&data->lock);
>>> +	data->map = devm_regmap_init_mmio(dev, base, &regmap_config);
>>> +	if (IS_ERR(data->map))
>>> +		return dev_err_probe(dev, PTR_ERR(data->map),
>>> +				     "can't init regmap mmio region\n");
>>>  
>>>  	data->rcdev.owner = THIS_MODULE;
>>> -	data->rcdev.nr_resets = data->param->reg_count * BITS_PER_REG;
>>> +	data->rcdev.nr_resets = data->param->reg_count * BITS_PER_BYTE
>>> +		* regmap_config.reg_stride;
>>>  	data->rcdev.ops = &meson_reset_ops;
>>> -	data->rcdev.of_node = pdev->dev.of_node;
>>> +	data->rcdev.of_node = dev->of_node;
>>>  
>>> -	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
>>> +	return devm_reset_controller_register(dev, &data->rcdev);
>>>  }
>>>  
>>>  static struct platform_driver meson_reset_driver = {
> 

-- 
Best regards
Jan Dakinevich

Re: [PATCH 1/8] reset: amlogic: convert driver to regmap

[Stephen Boyd]

Quoting Jan Dakinevich (2024-07-18 04:01:28)
> 
> 
> On 7/18/24 10:19, Jerome Brunet wrote:
> > On Thu 18 Jul 2024 at 05:39, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:
> >> and using of regmap_write() (apparently) fixes it.
> > 
> > Nor does that conclusion.
> > > It is perfectly possible I have made mistake somewhere (I have already
> > fixed one in v2), but sending incomplete report like this, with
> > unargumented conclusion is just noise in the end.
> > 
> > No, there is no situation in which `regmap_write` would solve a problem
> > with `regmap_update_bits`.
> > 
> 
> What is the default regs' value of this reset controller? The doc that I
> have doesn't clearly specifies it, but it tells that "the reset will
> auto-cover to 0 by HW". However pr_info() say that there is 0xffffffff
> before write (I am talking about A1).
> 
> Also we know, that reset is triggered by writing 1 to specific bit. So,
> what will happen if 1 will be written where we did not intend to write it?
> 
> > Either send a full analysis of the problem you found, if you did one, or
> > at least send the full dump, explaining that you don't know what is
> > happening.
> > 
> 
> Full analysis is following:
> - using regmap_update_bits() instead of writel() is incorrect because
> this changes the behavior of the driver
> - regmap_update_bits() should not be used here because default value of
> regs isn't taken into account and (_apparently_, the doc is terse) these
> regs could be updated by hw itself.

Maybe use regmap_write_bits() instead.

Re: [PATCH 1/8] reset: amlogic: convert driver to regmap

[Jerome Brunet]

On Thu 18 Jul 2024 at 12:29, Stephen Boyd <sboyd@kernel.org> wrote:

>> 
>> Full analysis is following:
>> - using regmap_update_bits() instead of writel() is incorrect because
>> this changes the behavior of the driver
>> - regmap_update_bits() should not be used here because default value of
>> regs isn't taken into account and (_apparently_, the doc is terse) these
>> regs could be updated by hw itself.
>
> Maybe use regmap_write_bits() instead.

Actually regmap_write_bits() performs an update behind the scene.
You'd still get the undefined read value making a mess AFAICT.

I'll stick to the usual regmap_write().

-- 
Jerome