Re: [RFC PATCH v3 1/6] reset: reset-meson-audio: introduce separate driver

From: Jerome Brunet
Date: Mon Apr 22 2024 - 03:54:46 EST

Next message: Dongsheng Yang: "[PATCH 4/7] cbd: introduce cbd_host"
Previous message: Kemeng Shi: "[PATCH v4 2/4] writeback: support retrieving per group debug writeback stats of bdi"
In reply to: Jan Dakinevich: "[RFC PATCH v3 1/6] reset: reset-meson-audio: introduce separate driver"
Next in thread: Jan Dakinevich: "[RFC PATCH v3 2/6] clk: meson: axg: share the set of audio helper macro"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri 19 Apr 2024 at 15:58, Jan Dakinevich <jan.dakinevich@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
> + */
> +
> +#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

Next message: Dongsheng Yang: "[PATCH 4/7] cbd: introduce cbd_host"
Previous message: Kemeng Shi: "[PATCH v4 2/4] writeback: support retrieving per group debug writeback stats of bdi"
In reply to: Jan Dakinevich: "[RFC PATCH v3 1/6] reset: reset-meson-audio: introduce separate driver"
Next in thread: Jan Dakinevich: "[RFC PATCH v3 2/6] clk: meson: axg: share the set of audio helper macro"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]