Message-ID: <05ee7331-c582-49ff-9f4e-2eee13f3a429@kernel.org>
Date: Tue, 29 Oct 2024 07:35:50 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Friday Yang (杨阳) <Friday.Yang@...iatek.com>,
 "robh@...nel.org" <robh@...nel.org>,
 "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
 "conor+dt@...nel.org" <conor+dt@...nel.org>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 Yong Wu (吴勇) <Yong.Wu@...iatek.com>,
 "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
 Project_Global_Chrome_Upstream_Group
 <Project_Global_Chrome_Upstream_Group@...iatek.com>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 4/4] reset: mediatek: Add reset control driver for SMI

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@...iatek.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@...10000 {
> 		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@...20000{
> 		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

Powered by blists - more mailing lists