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

["Friday Yang (杨阳)" <Friday.Yang@mediatek.com>]

On Wed, 2024-08-21 at 10:58 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  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?
> 

How about changing it like this,
patchset1:
(1)SMI reset control driver
(2)SMI reset bindings
patchset2
(1)SMI driver
(2)SMI bindings

> > 
> > 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

Thanks, I will fix it to '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 intlarb_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?
> 

This is the definition for imgsys1_dip_top and imgsys1_dip_top_rst.
SMI reset controller driver parse the 'mediatek,larb-rst-syscon'
to get the 'imgsys1_dip_top' device node and regmap.
Then SMI driver could read and write the register by the regmap
to apply reset operations here.

	imgsys1_dip_top: clock-controller@15110000 {
		compatible = "mediatek,mt8188-imgsys1-dip-top";
		reg = <0 0x15110000 0 0x1000>;
		#clock-cells = <1>;
	};

	imgsys1_dip_top_rst: reset-controller0 {
		compatible = "mediatek,smi-reset-mt8188";
		#reset-cells = <1>;
		mediatek,larb-rst-syscon = <&imgsys1_dip_top>;
	};
	
	larb10: smi@15120000{
		resets = <&imgsys1_dip_top_rst MT8188_SMI_RST_LARB10>;
		reset-names = "larb_rst";
	};

I am not so sure the meaning "MMIO-using" here. 
Cureently I use regmap_set_bits and regmap_clear_bits to update the
larb reset register. These registers locate in each subsys and are used
by respective drivers. So SMI add 'imgsys1_dip_top_rst' node to get the
regmap to avoid affecting subsys driver.

From my point of view, there are two methods:
(1)use of_iomap to get MMIO register region, and use writel/readl to
wirte/read register.
(2)use device_node_regmap to get the regmap, and use regmap_set_bits
and regmap_clear_bits to set register

And you prefer option (1), is this correct?

What is more, you prefer to put the imgsys1_dip_top_rst node into the
imgsys1_dip_top node as a child node, like this?

	imgsys1_dip_top: clock-controller@15110000 {
		compatible = "mediatek,mt8188-imgsys1-dip-top";
		reg = <0 0x15110000 0 0x1000>;
		#clock-cells = <1>;

		imgsys1_dip_top_rst: reset-controller0 {
			compatible = "mediatek,smi-reset-mt8188";
			#reset-cells = <1>;
		};
	};

> Best regards,
> Krzysztof
>