Received-SPF: pass (google.com: domain of linux-kernel+bounces-152913-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
References: <20240419125812.983409-1-jan.dakinevich@salutedevices.com>
 <20240419125812.983409-2-jan.dakinevich@salutedevices.com>
User-agent: mu4e 1.10.8; emacs 29.2
From: Jerome Brunet <jbrunet@baylibre.com>
To: Jan Dakinevich <jan.dakinevich@salutedevices.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>, Jerome Brunet
 <jbrunet@baylibre.com>, Michael  Turquette <mturquette@baylibre.com>,
 Stephen Boyd <sboyd@kernel.org>, Rob  Herring <robh@kernel.org>, Krzysztof
 Kozlowski <krzk+dt@kernel.org>, Conor  Dooley <conor+dt@kernel.org>, Kevin
 Hilman <khilman@baylibre.com>, Martin  Blumenstingl
 <martin.blumenstingl@googlemail.com>, Philipp Zabel
 <p.zabel@pengutronix.de>, Jiucheng Xu <jiucheng.xu@amlogic.com>,
 linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH v3 1/6] reset: reset-meson-audio: introduce separate
 driver
Date: Mon, 22 Apr 2024 09:46:50 +0200
In-reply-to: <20240419125812.983409-2-jan.dakinevich@salutedevices.com>
Message-ID: <1j8r15bzqa.fsf@starbuckisacylon.baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain


On Fri 19 Apr 2024 at 15:58, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:

> Typically, Amlogic Meson SoCs have a couple a reset registers lost in
> middle of audio clock controller. Reset controller on top of them was
> implemented inside audio clock controller driver. This patch moves reset
> functionality of this controller to auxiliary driver. There are at least
> two reasons for this:
>
>   - architecturally it is more convenient;
>
>   - reusing the code of reset controller for new SoCs becomes easier.
>
> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
> ---
>  drivers/clk/meson/Kconfig               |   2 +
>  drivers/clk/meson/axg-audio.c           | 106 +------------
>  drivers/reset/Kconfig                   |   7 +
>  drivers/reset/Makefile                  |   1 +
>  drivers/reset/reset-meson-audio.c       | 197 ++++++++++++++++++++++++
>  include/soc/amlogic/meson-audio-reset.h |  10 ++

You must an effort to touch a single subsystem per series.
Making a series about a single subsystem makes like a lot easier for
maintainers. clk, reset and dt and different subsystems

That constraints relaxed for RFCs but mixing subsystems within a single
patch is a red flag, unless you really have a good reason ... and you
don't have one here.

Nothing blocks introducting support in reset, then use it in clocks.

You could also have allowed a bit more time before reposting since I
said I would look at the reset issue.

I doubt introducing a separate driver for this is required since since
the current amlogic reset driver is fairly close.

>  6 files changed, 224 insertions(+), 99 deletions(-)
>  create mode 100644 drivers/reset/reset-meson-audio.c
>  create mode 100644 include/soc/amlogic/meson-audio-reset.h
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index 29ffd14d267b..33614f8b8cf7 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -101,6 +101,8 @@ config COMMON_CLK_AXG_AUDIO
>  	select COMMON_CLK_MESON_PHASE
>  	select COMMON_CLK_MESON_SCLK_DIV
>  	select COMMON_CLK_MESON_CLKC_UTILS
> +	select RESET_CONTROLLER
> +	select RESET_MESON_AUDIO
>  	select REGMAP_MMIO
>  	help
>  	  Support for the audio clock controller on AmLogic A113D devices,
> diff --git a/drivers/clk/meson/axg-audio.c b/drivers/clk/meson/axg-audio.c
> index ac3482960903..9cd6b5c3aa7e 100644
> --- a/drivers/clk/meson/axg-audio.c
> +++ b/drivers/clk/meson/axg-audio.c
> @@ -12,9 +12,10 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
> -#include <linux/reset-controller.h>
>  #include <linux/slab.h>
>  
> +#include <soc/amlogic/meson-audio-reset.h>
> +
>  #include "meson-clkc-utils.h"
>  #include "axg-audio.h"
>  #include "clk-regmap.h"
> @@ -1648,84 +1649,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 +1660,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 *reset_name;
>  };
>  
>  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;
> @@ -1804,21 +1725,10 @@ static int axg_audio_clkc_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	/* Stop here if there is no reset */
> -	if (!data->reset_num)
> +	if (!data->reset_name)
>  		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;
> -
> -	return devm_reset_controller_register(dev, &rst->rstc);
> +	return meson_audio_reset_register(dev, data->reset_name);
>  }
>  
>  static const struct audioclk_data axg_audioclk_data = {
> @@ -1837,8 +1747,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,
> +	.reset_name = "g12a",
>  };
>  
>  static const struct audioclk_data sm1_audioclk_data = {
> @@ -1848,8 +1757,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,
> +	.reset_name = "sm1",
>  };
>  
>  static const struct of_device_id clkc_match_table[] = {
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 7112f5932609..98106694566f 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -138,6 +138,13 @@ config RESET_MESON
>  	help
>  	  This enables the reset driver for Amlogic Meson SoCs.
>  
> +config RESET_MESON_AUDIO
> +	tristate "Meson Audio Reset Driver"
> +	depends on ARCH_MESON || COMPILE_TEST
> +	select AUXILIARY_BUS
> +	help
> +	  This enables the audio reset driver for Amlogic Meson SoCs.
> +
>  config RESET_MESON_AUDIO_ARB
>  	tristate "Meson Audio Memory Arbiter Reset Driver"
>  	depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index fd8b49fa46fc..8ee7a57ccf03 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
>  obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
>  obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
>  obj-$(CONFIG_RESET_MESON) += reset-meson.o
> +obj-$(CONFIG_RESET_MESON_AUDIO) += reset-meson-audio.o
>  obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
>  obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
>  obj-$(CONFIG_RESET_NUVOTON_MA35D1) += reset-ma35d1.o
> diff --git a/drivers/reset/reset-meson-audio.c b/drivers/reset/reset-meson-audio.c
> new file mode 100644
> index 000000000000..aaea9931cfe2
> --- /dev/null
> +++ b/drivers/reset/reset-meson-audio.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (c) 2018 BayLibre, SAS.
> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> + */
> +
> +#include <linux/regmap.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/reset-controller.h>
> +
> +#include <soc/amlogic/meson-audio-reset.h>
> +
> +struct meson_audio_reset_data {
> +	struct reset_controller_dev rstc;
> +	struct regmap *map;
> +	unsigned int offset;
> +};
> +
> +struct meson_audio_reset_info {
> +	unsigned int reset_offset;
> +	unsigned int reset_num;
> +};
> +
> +static void meson_audio_reset_reg_and_bit(struct meson_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 meson_audio_reset_update(struct reset_controller_dev *rcdev,
> +				    unsigned long id, bool assert)
> +{
> +	struct meson_audio_reset_data *rst =
> +		container_of(rcdev, struct meson_audio_reset_data, rstc);
> +	unsigned int offset, bit;
> +
> +	meson_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 meson_audio_reset_status(struct reset_controller_dev *rcdev,
> +				    unsigned long id)
> +{
> +	struct meson_audio_reset_data *rst =
> +		container_of(rcdev, struct meson_audio_reset_data, rstc);
> +	unsigned int val, offset, bit;
> +
> +	meson_audio_reset_reg_and_bit(rst, id, &offset, &bit);
> +
> +	regmap_read(rst->map, offset, &val);
> +
> +	return !!(val & BIT(bit));
> +}
> +
> +static int meson_audio_reset_assert(struct reset_controller_dev *rcdev,
> +				    unsigned long id)
> +{
> +	return meson_audio_reset_update(rcdev, id, true);
> +}
> +
> +static int meson_audio_reset_deassert(struct reset_controller_dev *rcdev,
> +				      unsigned long id)
> +{
> +	return meson_audio_reset_update(rcdev, id, false);
> +}
> +
> +static int meson_audio_reset_toggle(struct reset_controller_dev *rcdev,
> +				    unsigned long id)
> +{
> +	int ret;
> +
> +	ret = meson_audio_reset_assert(rcdev, id);
> +	if (ret)
> +		return ret;
> +
> +	return meson_audio_reset_deassert(rcdev, id);
> +}
> +
> +static const struct reset_control_ops meson_audio_reset_ops = {
> +	.assert = meson_audio_reset_assert,
> +	.deassert = meson_audio_reset_deassert,
> +	.reset = meson_audio_reset_toggle,
> +	.status = meson_audio_reset_status,
> +};
> +
> +static int meson_audio_reset_probe(struct auxiliary_device *adev,
> +				  const struct auxiliary_device_id *id)
> +{
> +	struct device *dev = &adev->dev;
> +	struct meson_audio_reset_info *info =
> +		(struct meson_audio_reset_info *)id->driver_data;
> +	struct meson_audio_reset_data *rst;
> +
> +	dev_info(dev, "%s, reset_offset = %#x, reset_num = %u", __func__,
> +		 info->reset_offset, info->reset_num);
> +
> +	rst = devm_kzalloc(dev, sizeof(*rst), GFP_KERNEL);
> +	if (!rst)
> +		return -ENOMEM;
> +
> +	rst->map = dev_get_regmap(dev->parent, NULL);
> +	if (!rst->map)
> +		return -ENODEV;
> +
> +	rst->offset = info->reset_offset;
> +	rst->rstc.ops = &meson_audio_reset_ops;
> +	rst->rstc.of_node = dev->parent->of_node;
> +	rst->rstc.nr_resets = info->reset_num;
> +	rst->rstc.owner = THIS_MODULE;
> +
> +	return devm_reset_controller_register(dev, &rst->rstc);
> +}
> +
> +static void meson_audio_reset_adev_release(struct device *dev)
> +{
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +
> +	kfree(adev);
> +}
> +
> +static void meson_audio_reset_adev_unregister(void *_adev)
> +{
> +	struct auxiliary_device *adev = _adev;
> +
> +	auxiliary_device_delete(adev);
> +	auxiliary_device_uninit(adev);
> +}
> +
> +static const struct meson_audio_reset_info meson_audio_reset_info_g12a = {
> +	.reset_offset = 0x024,
> +	.reset_num = 26,
> +};
> +
> +static const struct meson_audio_reset_info meson_audio_reset_info_sm1 = {
> +	.reset_offset = 0x028,
> +	.reset_num = 39,
> +};
> +static const struct auxiliary_device_id meson_audio_reset_id[] = {
> +	{
> +		.name = "reset_meson_audio.g12a",
> +		.driver_data = (kernel_ulong_t)&meson_audio_reset_info_g12a,
> +	},
> +	{
> +		.name = "reset_meson_audio.sm1",
> +		.driver_data = (kernel_ulong_t)&meson_audio_reset_info_sm1,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(auxiliary, meson_audio_reset_id);
> +
> +static struct auxiliary_driver meson_audio_reset_driver = {
> +	.probe = meson_audio_reset_probe,
> +	.id_table = meson_audio_reset_id,
> +};
> +
> +module_auxiliary_driver(meson_audio_reset_driver);
> +
> +int meson_audio_reset_register(struct device *dev, const char *name)
> +{
> +	struct auxiliary_device *adev;
> +	int ret;
> +
> +	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> +	if (!adev)
> +		return -ENOMEM;
> +
> +	adev->name = name;
> +	adev->dev.parent = dev;
> +	adev->dev.release = meson_audio_reset_adev_release;
> +	adev->id = 0;
> +
> +	ret = auxiliary_device_init(adev);
> +	if (ret)
> +		return ret;
> +
> +	ret = auxiliary_device_add(adev);
> +	if (ret) {
> +		auxiliary_device_uninit(adev);
> +		return ret;
> +	}
> +
> +	return devm_add_action_or_reset(dev, meson_audio_reset_adev_unregister,
> +					adev);
> +}
> +EXPORT_SYMBOL_GPL(meson_audio_reset_register);
> +
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/soc/amlogic/meson-audio-reset.h b/include/soc/amlogic/meson-audio-reset.h
> new file mode 100644
> index 000000000000..279c6a2197ec
> --- /dev/null
> +++ b/include/soc/amlogic/meson-audio-reset.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +
> +#ifndef __MESON_AUDIO_RESET_H
> +#define __MESON_AUDIO_RESET_H
> +
> +#include <linux/device.h>
> +
> +int meson_audio_reset_register(struct device *dev, const char *name);
> +
> +#endif /* __MESON_AUDIO_RESET_H */


-- 
Jerome