diff mbox series

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

Message ID 20240821082845.11792-5-friday.yang@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Add SMI clamp and reset | expand

Commit Message

Friday Yang (杨阳) Aug. 21, 2024, 8:26 a.m. UTC 
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.

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

Comments

Krzysztof Kozlowski Aug. 21, 2024, 8:58 a.m. UTC | #1 
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
Friday Yang (杨阳) Oct. 24, 2024, 1:29 a.m. UTC | #2 
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
>
Krzysztof Kozlowski Oct. 29, 2024, 6:35 a.m. UTC | #3 
On 24/10/2024 03:29, Friday Yang (杨阳) wrote:
> 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>;

This is obviously incorrect DTS code. Run standard checks on your DTS
code first.


> 	};
> 	
> 	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. 

You pretend that something is MMIO but you do not use it as MMIO.

Anyway, this was 2 months ago, I lost all the context, all the emails
and I am not going back.

Respond to feedback in reasonable time, if you want to keep discussion
going.

All this is so far: NAK

Best regards,
Krzysztof
diff mbox series

Patch

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
+	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)
+		return -EINVAL;
+
+	regmap = device_node_to_regmap(reset_node);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	data->larb_plat = larb_plat;
+	data->regmap = regmap;
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.ops = &mtk_smi_reset_ops;
+	data->rcdev.of_node = np;
+	data->rcdev.nr_resets = larb_plat->larb_reset_nr;
+	data->rcdev.dev = dev;
+	platform_set_drvdata(pdev, data);
+
+	return devm_reset_controller_register(dev, &data->rcdev);
+}
+
+static const struct of_device_id mtk_smi_larb_reset_of_match[] = {
+	{ .compatible = "mediatek,smi-reset-mt8188", .data = &mtk_smi_larb_mt8188 },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mtk_smi_larb_reset_of_match);
+
+static struct platform_driver mtk_smi_reset_driver = {
+	.probe = mtk_smi_reset_probe,
+	.driver = {
+		.name = "mediatek-smi-reset",
+		.of_match_table = mtk_smi_larb_reset_of_match,
+	},
+};
+module_platform_driver(mtk_smi_reset_driver);
+
+MODULE_AUTHOR("Friday.Yang@mediatek.com");
+MODULE_DESCRIPTION("MediaTek SMI Reset Driver");
+MODULE_LICENSE("GPL");