Hi Enric,

Thanks for your review.

On Fri, 2021-07-23 at 12:57 +0200, Enric Balletbo Serra wrote:

Hi Nancy,

Thank you for your patch.

Missatge de Nancy.Lin [off-list ref] del dia dj., 22 de
jul.
2021 a les 11:46:

quoted

Among other features the mmsys driver should implement a reset
controller to be able to reset different bits from their space.

I'm working on a series that does the same, it should be nice if we
can coordinate [1]

[1] 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=515355__;!!CTRNKA9wMg0ARbw!xP6Ko9hF-3KasGgr7-8Aby_tCwiU2M6gFBAngDLVcJjzooj-MEeTcNG8cf2e9wGb$
 

OK, I will add this series to my reference base in the next patch
revision.

quoted

Signed-off-by: Nancy.Lin <redacted>
---
 drivers/soc/mediatek/mt8195-mmsys.h |  1 +
 drivers/soc/mediatek/mtk-mmsys.c    | 77
+++++++++++++++++++++++++++++
 drivers/soc/mediatek/mtk-mmsys.h    |  1 +
 3 files changed, 79 insertions(+)

diff --git a/drivers/soc/mediatek/mt8195-mmsys.h

b/drivers/soc/mediatek/mt8195-mmsys.h
index 4bdb2087250c..a7f6e275bfe5 100644

--- a/drivers/soc/mediatek/mt8195-mmsys.h
+++ b/drivers/soc/mediatek/mt8195-mmsys.h

@@ -154,6 +154,7 @@
 #define DISP_DP_INTF0_SEL_IN_FROM_VDO0_MERGE_DL_ASYNC_MOUT     (1

<< 0)
 #define DISP_DP_INTF0_SEL_IN_FROM_VDO0_DSC_DL_ASYNC_MOUT       (2
<< 0)

+#define MT8195_VDO1_SW0_RST_B           0x1d0
 #define MT8195_VDO1_MERGE0_ASYNC_CFG_WD        0xe30
 #define MT8195_VDO1_MERGE1_ASYNC_CFG_WD        0xe40
 #define MT8195_VDO1_MERGE2_ASYNC_CFG_WD        0xe50

diff --git a/drivers/soc/mediatek/mtk-mmsys.c

b/drivers/soc/mediatek/mtk-mmsys.c
index d0f4a407f8f8..1ae04efeadab 100644

--- a/drivers/soc/mediatek/mtk-mmsys.c
+++ b/drivers/soc/mediatek/mtk-mmsys.c

@@ -4,10 +4,12 @@
  * Author: James Liao <jamesjj.liao@mediatek.com>
  */

+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/reset-controller.h>
 #include <linux/soc/mediatek/mtk-mmsys.h>

 #include "mtk-mmsys.h"

@@ -15,6 +17,8 @@
 #include "mt8183-mmsys.h"
 #include "mt8195-mmsys.h"

+#define MMSYS_SW_RESET_PER_REG 32
+
 static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data

= {
        .clk_driver = "clk-mt2701-mm",
        .routes = mmsys_default_routing_table,

@@ -65,12 +69,15 @@ static const struct mtk_mmsys_driver_data

mt8195_vdosys1_driver_data = {
        .num_routes = ARRAY_SIZE(mmsys_mt8195_routing_table),
        .config = mmsys_mt8195_config_table,
        .num_configs = ARRAY_SIZE(mmsys_mt8195_config_table),
+       .sw_reset_start = MT8195_VDO1_SW0_RST_B,

That change is interesting and I think I should also take it into
consideration with my series.

quoted

 };

 struct mtk_mmsys {
        void __iomem *regs;
        struct cmdq_client_reg cmdq_base;
        const struct mtk_mmsys_driver_data *data;
+       spinlock_t lock; /* protects mmsys_sw_rst_b reg */

Seems that mmsys_sw_rst_b reg has different names for different SoCs?
I mean I know that for MT8173 and MT8183 the register is called
mmsys_sw0_rst_b but looks like for MT8195 the name is vdo1_sw0_rst_b?
So maybe we should update this comment to be more generic.

Yes, the name of MT8195 vdosys1 sw reset is called VDOSYS1_SW0_RST_B 
and the name of vdosys0 sw reset is called GLOBAL0_SW0_RST_B. They have
a different name. Maybe we can change the comment to "protects mmsys sw
reset reg".

quoted

quoted

+       struct reset_controller_dev rcdev;
 };

 void mtk_mmsys_ddp_connect(struct device *dev,

@@ -148,6 +155,63 @@ void mtk_mmsys_ddp_config(struct device *dev,

enum mtk_mmsys_config_type config,
 }
 EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_config);

+static int mtk_mmsys_reset_update(struct reset_controller_dev
*rcdev, unsigned long id,
+                                 bool assert)
+{
+       struct mtk_mmsys *mmsys = container_of(rcdev, struct
mtk_mmsys, rcdev);
+       unsigned long flags;
+       u32 reg;
+       int i;
+       u32 offset;
+
+       offset = (id / MMSYS_SW_RESET_PER_REG) * sizeof(u32);
+       id = 1 << (id % MMSYS_SW_RESET_PER_REG);
+
+       spin_lock_irqsave(&mmsys->lock, flags);
+
+       reg = readl_relaxed(mmsys->regs + mmsys->data-

quoted

sw_reset_start + offset);

+
+       if (assert)
+               reg &= ~BIT(id);
+       else
+               reg |= BIT(id);
+
+       writel_relaxed(reg, mmsys->regs + mmsys->data-

quoted

sw_reset_start + offset);

+
+       spin_unlock_irqrestore(&mmsys->lock, flags);
+
+       return 0;
+}
+
+static int mtk_mmsys_reset_assert(struct reset_controller_dev
*rcdev, unsigned long id)
+{
+       return mtk_mmsys_reset_update(rcdev, id, true);
+}
+
+static int mtk_mmsys_reset_deassert(struct reset_controller_dev
*rcdev, unsigned long id)
+{
+       return mtk_mmsys_reset_update(rcdev, id, false);
+}
+
+static int mtk_mmsys_reset(struct reset_controller_dev *rcdev,
unsigned long id)
+{
+       int ret;
+
+       ret = mtk_mmsys_reset_assert(rcdev, id);
+       if (ret)
+               return ret;
+
+       usleep_range(1000, 1100);
+

One question that I received in my series, and I couldn't answer
because I don't have the datasheet, is if
is this known to be enough for all IP cores that can be reset by this
controller? Is this time specified in the datasheet?

It only takes few cycles for the reset. The 1000us is enough for the
reset to take effect.

quoted

+       return mtk_mmsys_reset_deassert(rcdev, id);
+}
+
+static const struct reset_control_ops mtk_mmsys_reset_ops = {
+       .assert = mtk_mmsys_reset_assert,
+       .deassert = mtk_mmsys_reset_deassert,
+       .reset = mtk_mmsys_reset,
+};
+
 static int mtk_mmsys_probe(struct platform_device *pdev)
 {
        struct device *dev = &pdev->dev;

@@ -174,6 +238,19 @@ static int mtk_mmsys_probe(struct

platform_device *pdev)
        if (ret)
                dev_dbg(dev, "No mediatek,gce-client-reg!\n");
 #endif
+
+       spin_lock_init(&mmsys->lock);
+
+       mmsys->rcdev.owner = THIS_MODULE;
+       mmsys->rcdev.nr_resets = 64;

Is the number of resets 64 for MT8195? I think is 32 for MT8173 and
MT8183. Can you confirm?

Thanks,
  Enric

The number of resets in MT8195 vdosys1 is 64 (43 resets are used, 21
are not used).

quoted

+       mmsys->rcdev.ops = &mtk_mmsys_reset_ops;
+       mmsys->rcdev.of_node = pdev->dev.of_node;
+       ret = devm_reset_controller_register(&pdev->dev, &mmsys-

quoted

rcdev);

+       if (ret) {
+               dev_err(&pdev->dev, "Couldn't register mmsys reset
controller: %d\n", ret);
+               return ret;
+       }
+
        platform_set_drvdata(pdev, mmsys);

        clks = platform_device_register_data(&pdev->dev, mmsys-

quoted

data->clk_driver,

diff --git a/drivers/soc/mediatek/mtk-mmsys.h

b/drivers/soc/mediatek/mtk-mmsys.h
index 084b1f5f3c88..cc57c3895c51 100644

--- a/drivers/soc/mediatek/mtk-mmsys.h
+++ b/drivers/soc/mediatek/mtk-mmsys.h

@@ -87,6 +87,7 @@ struct mtk_mmsys_driver_data {
        const unsigned int num_routes;
        const struct mtk_mmsys_config *config;
        const unsigned int num_configs;
+       u32 sw_reset_start;
 };

 /*
--

2.18.0

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek