Re: [PATCH 4/4] reset: mediatek: Add reset control driver for SMI

[Krzysztof Kozlowski <krzk@kernel.org>]

On 21/08/2024 10:26, friday.yang wrote:
> Add a reset-controller driver for performing reset management of
> SMI LARBs on MediaTek platform. This driver uses the regmap
> frameworks to actually implement the various reset functions
> needed when SMI LARBs apply clamp operations.

How does this depend on memory controller patches? Why is this grouped
in one patchset?

> 
> Signed-off-by: friday.yang <friday.yang@mediatek.com>
> ---
>  drivers/reset/Kconfig              |   9 ++
>  drivers/reset/Makefile             |   1 +
>  drivers/reset/reset-mediatek-smi.c | 152 +++++++++++++++++++++++++++++
>  3 files changed, 162 insertions(+)
>  create mode 100644 drivers/reset/reset-mediatek-smi.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 67bce340a87e..e984a5a332f1 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -154,6 +154,15 @@ config RESET_MESON_AUDIO_ARB
>  	  This enables the reset driver for Audio Memory Arbiter of
>  	  Amlogic's A113 based SoCs
>  
> +config RESET_MTK_SMI
> +	bool "MediaTek SMI Reset Driver"
> +	depends on MTK_SMI

compile test

> +	help
> +	  This option enables the reset controller driver for MediaTek SMI.
> +	  This reset driver is responsible for managing the reset signals
> +	  for SMI larbs. Say Y if you want to control reset signals for
> +	  MediaTek SMI larbs. Otherwise, say N.
> +
>  config RESET_NPCM
>  	bool "NPCM BMC Reset Driver" if COMPILE_TEST
>  	default ARCH_NPCM
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 27b0bbdfcc04..241777485b40 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -22,6 +22,7 @@ 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_ARB) += reset-meson-audio-arb.o
> +obj-$(CONFIG_RESET_MTK_SMI) += reset-mediatek-smi.o
>  obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
>  obj-$(CONFIG_RESET_NUVOTON_MA35D1) += reset-ma35d1.o
>  obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
> diff --git a/drivers/reset/reset-mediatek-smi.c b/drivers/reset/reset-mediatek-smi.c
> new file mode 100644
> index 000000000000..ead747e80ad5
> --- /dev/null
> +++ b/drivers/reset/reset-mediatek-smi.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Reset driver for MediaTek SMI module
> + *
> + * Copyright (C) 2024 MediaTek Inc.
> + */
> +
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +#include <dt-bindings/reset/mt8188-resets.h>
> +
> +#define to_mtk_smi_reset_data(_rcdev)	\
> +	container_of(_rcdev, struct mtk_smi_reset_data, rcdev)
> +
> +struct mtk_smi_larb_reset {
> +	unsigned int offset;
> +	unsigned int value;
> +};
> +
> +static const struct mtk_smi_larb_reset rst_signal_mt8188[] = {
> +	[MT8188_SMI_RST_LARB10]		= { 0xC, BIT(0) }, /* larb10 */
> +	[MT8188_SMI_RST_LARB11A]	= { 0xC, BIT(0) }, /* larb11a */
> +	[MT8188_SMI_RST_LARB11C]	= { 0xC, BIT(0) }, /* larb11c */
> +	[MT8188_SMI_RST_LARB12]		= { 0xC, BIT(8) }, /* larb12 */
> +	[MT8188_SMI_RST_LARB11B]	= { 0xC, BIT(0) }, /* larb11b */
> +	[MT8188_SMI_RST_LARB15]		= { 0xC, BIT(0) }, /* larb15 */
> +	[MT8188_SMI_RST_LARB16B]	= { 0xA0, BIT(4) }, /* larb16b */
> +	[MT8188_SMI_RST_LARB17B]	= { 0xA0, BIT(4) }, /* larb17b */
> +	[MT8188_SMI_RST_LARB16A]	= { 0xA0, BIT(4) }, /* larb16a */
> +	[MT8188_SMI_RST_LARB17A]	= { 0xA0, BIT(4) }, /* larb17a */
> +};
> +
> +struct mtk_smi_larb_plat {
> +	const struct mtk_smi_larb_reset		*reset_signal;
> +	const unsigned int			larb_reset_nr;
> +};
> +
> +struct mtk_smi_reset_data {
> +	const struct mtk_smi_larb_plat *larb_plat;
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +};
> +
> +static const struct mtk_smi_larb_plat mtk_smi_larb_mt8188 = {
> +	.reset_signal = rst_signal_mt8188,
> +	.larb_reset_nr = ARRAY_SIZE(rst_signal_mt8188),
> +};
> +
> +static int mtk_smi_larb_reset(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
> +	const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
> +	const struct mtk_smi_larb_reset *larb_rst = larb_plat->reset_signal + id;
> +	int ret;
> +
> +	ret = regmap_set_bits(data->regmap, larb_rst->offset, larb_rst->value);
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(data->regmap, larb_rst->offset, larb_rst->value);
> +
> +	return ret;
> +}
> +
> +static int mtk_smi_larb_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
> +	const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
> +	const struct mtk_smi_larb_reset *larb_rst = larb_plat->reset_signal + id;
> +	int ret;
> +
> +	ret = regmap_set_bits(data->regmap, larb_rst->offset, larb_rst->value);
> +	if (ret)
> +		dev_err(rcdev->dev, "[%s] Failed to shutdown larb %d\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static int mtk_smi_larb_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
> +	const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
> +	const struct mtk_smi_larb_reset *larb_rst = larb_plat->reset_signal + id;
> +	int ret;
> +
> +	ret = regmap_clear_bits(data->regmap, larb_rst->offset, larb_rst->value);
> +	if (ret)
> +		dev_err(rcdev->dev, "[%s] Failed to reopen larb %d\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static const struct reset_control_ops mtk_smi_reset_ops = {
> +	.reset		= mtk_smi_larb_reset,
> +	.assert		= mtk_smi_larb_reset_assert,
> +	.deassert	= mtk_smi_larb_reset_deassert,
> +};
> +
> +static int mtk_smi_reset_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct mtk_smi_larb_plat *larb_plat = of_device_get_match_data(dev);
> +	struct device_node *np = dev->of_node, *reset_node;
> +	struct mtk_smi_reset_data *data;
> +	struct regmap *regmap;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	reset_node = of_parse_phandle(np, "mediatek,larb-rst-syscon", 0);
> +	if (!reset_node)

This looks just wrong. This looks like a child of whatever phandle
points here.

Why do you create MMIO-using node as not MMIO?

Best regards,
Krzysztof