diff mbox series

[v25,07/10] soc: mediatek: mmsys: add mmsys for support 64 reset bits

Message ID 20220711075245.10492-8-nancy.lin@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Add MediaTek SoC(vdosys1) support for mt8195 | expand

Commit Message

Nancy Lin (林欣螢) July 11, 2022, 7:52 a.m. UTC 
Add mmsys for support 64 reset bits. It is a preparation for MT8195
vdosys1 HW reset. MT8195 vdosys1 has more than 32 reset bits.

1. Add the number of reset bits in mmsys private data
2. move the whole "reset register code section" behind the
"get mmsys->data" code section for getting the num_resets in mmsys->data.

Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
---
 drivers/soc/mediatek/mtk-mmsys.c | 39 ++++++++++++++++++++------------
 drivers/soc/mediatek/mtk-mmsys.h |  1 +
 2 files changed, 26 insertions(+), 14 deletions(-)

Comments

Nícolas F. R. A. Prado Aug. 18, 2022, 9:47 p.m. UTC | #1 
Hi Nancy,

On Mon, Jul 11, 2022 at 03:52:42PM +0800, Nancy.Lin wrote:
[..]
>  static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = {
>  	.clk_driver = "clk-mt2701-mm",
>  	.routes = mmsys_default_routing_table,
> @@ -86,6 +88,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
>  	.routes = mmsys_default_routing_table,
>  	.num_routes = ARRAY_SIZE(mmsys_default_routing_table),
>  	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
> +	.num_resets = 32,
>  };
>  
>  static const struct mtk_mmsys_match_data mt8173_mmsys_match_data = {
> @@ -100,6 +103,7 @@ static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = {
>  	.routes = mmsys_mt8183_routing_table,
>  	.num_routes = ARRAY_SIZE(mmsys_mt8183_routing_table),
>  	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
> +	.num_resets = 32,
>  };
>  
>  static const struct mtk_mmsys_match_data mt8183_mmsys_match_data = {
> @@ -114,6 +118,7 @@ static const struct mtk_mmsys_driver_data mt8186_mmsys_driver_data = {
>  	.routes = mmsys_mt8186_routing_table,
>  	.num_routes = ARRAY_SIZE(mmsys_mt8186_routing_table),
>  	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
> +	.num_resets = 32,
>  };
[..]
> @@ -351,18 +362,6 @@ static int mtk_mmsys_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	spin_lock_init(&mmsys->lock);
> -
> -	mmsys->rcdev.owner = THIS_MODULE;
> -	mmsys->rcdev.nr_resets = 32;

The number of resets was previously always set to 32, and now you're instead
setting it based on num_resets from each machine. The issue is, you're
forgetting a bunch of them.

mt8192 didn't get a num_reset, so this commit breaks the display on mt8192 based
devices. But mt8192 isn't the only one, there are other platforms missing this
property. Please set num_resets to 32 in every single one of them, otherwise
there will be display regressions.

Thanks,
Nícolas
Nancy Lin (林欣螢) Aug. 19, 2022, 2:09 a.m. UTC | #2 
Hi Nicolas,

Thanks for the review.

On Thu, 2022-08-18 at 17:47 -0400, Nícolas F. R. A. Prado wrote:
> Hi Nancy,
> 
> On Mon, Jul 11, 2022 at 03:52:42PM +0800, Nancy.Lin wrote:
> [..]
> >  static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data
> > = {
> >  	.clk_driver = "clk-mt2701-mm",
> >  	.routes = mmsys_default_routing_table,
> > @@ -86,6 +88,7 @@ static const struct mtk_mmsys_driver_data
> > mt8173_mmsys_driver_data = {
> >  	.routes = mmsys_default_routing_table,
> >  	.num_routes = ARRAY_SIZE(mmsys_default_routing_table),
> >  	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
> > +	.num_resets = 32,
> >  };
> >  
> >  static const struct mtk_mmsys_match_data mt8173_mmsys_match_data =
> > {
> > @@ -100,6 +103,7 @@ static const struct mtk_mmsys_driver_data
> > mt8183_mmsys_driver_data = {
> >  	.routes = mmsys_mt8183_routing_table,
> >  	.num_routes = ARRAY_SIZE(mmsys_mt8183_routing_table),
> >  	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
> > +	.num_resets = 32,
> >  };
> >  
> >  static const struct mtk_mmsys_match_data mt8183_mmsys_match_data =
> > {
> > @@ -114,6 +118,7 @@ static const struct mtk_mmsys_driver_data
> > mt8186_mmsys_driver_data = {
> >  	.routes = mmsys_mt8186_routing_table,
> >  	.num_routes = ARRAY_SIZE(mmsys_mt8186_routing_table),
> >  	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
> > +	.num_resets = 32,
> >  };
> 
> [..]
> > @@ -351,18 +362,6 @@ static int mtk_mmsys_probe(struct
> > platform_device *pdev)
> >  		return ret;
> >  	}
> >  
> > -	spin_lock_init(&mmsys->lock);
> > -
> > -	mmsys->rcdev.owner = THIS_MODULE;
> > -	mmsys->rcdev.nr_resets = 32;
> 
> The number of resets was previously always set to 32, and now you're
> instead
> setting it based on num_resets from each machine. The issue is,
> you're
> forgetting a bunch of them.
> 
> mt8192 didn't get a num_reset, so this commit breaks the display on
> mt8192 based
> devices. But mt8192 isn't the only one, there are other platforms
> missing this
> property. Please set num_resets to 32 in every single one of them,
> otherwise
> there will be display regressions.
> 
> Thanks,
> Nícolas

It's my mistake. I will set num_resets to 32 for every mmsys driver.

Thanks,
Nancy
Nancy Lin (林欣螢) Aug. 19, 2022, 4:13 a.m. UTC | #3 
Hi Nicolas,


On Fri, 2022-08-19 at 10:09 +0800, Nancy.Lin wrote:
> Hi Nicolas,
> 
> Thanks for the review.
> 
> On Thu, 2022-08-18 at 17:47 -0400, Nícolas F. R. A. Prado wrote:
> > Hi Nancy,
> > 
> > On Mon, Jul 11, 2022 at 03:52:42PM +0800, Nancy.Lin wrote:
> > [..]
> > >  static const struct mtk_mmsys_driver_data
> > > mt2701_mmsys_driver_data
> > > = {
> > >  	.clk_driver = "clk-mt2701-mm",
> > >  	.routes = mmsys_default_routing_table,
> > > @@ -86,6 +88,7 @@ static const struct mtk_mmsys_driver_data
> > > mt8173_mmsys_driver_data = {
> > >  	.routes = mmsys_default_routing_table,
> > >  	.num_routes = ARRAY_SIZE(mmsys_default_routing_table),
> > >  	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
> > > +	.num_resets = 32,
> > >  };
> > >  
> > >  static const struct mtk_mmsys_match_data mt8173_mmsys_match_data
> > > =
> > > {
> > > @@ -100,6 +103,7 @@ static const struct mtk_mmsys_driver_data
> > > mt8183_mmsys_driver_data = {
> > >  	.routes = mmsys_mt8183_routing_table,
> > >  	.num_routes = ARRAY_SIZE(mmsys_mt8183_routing_table),
> > >  	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
> > > +	.num_resets = 32,
> > >  };
> > >  
> > >  static const struct mtk_mmsys_match_data mt8183_mmsys_match_data
> > > =
> > > {
> > > @@ -114,6 +118,7 @@ static const struct mtk_mmsys_driver_data
> > > mt8186_mmsys_driver_data = {
> > >  	.routes = mmsys_mt8186_routing_table,
> > >  	.num_routes = ARRAY_SIZE(mmsys_mt8186_routing_table),
> > >  	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
> > > +	.num_resets = 32,
> > >  };
> > 
> > [..]
> > > @@ -351,18 +362,6 @@ static int mtk_mmsys_probe(struct
> > > platform_device *pdev)
> > >  		return ret;
> > >  	}
> > >  
> > > -	spin_lock_init(&mmsys->lock);
> > > -
> > > -	mmsys->rcdev.owner = THIS_MODULE;
> > > -	mmsys->rcdev.nr_resets = 32;
> > 
> > The number of resets was previously always set to 32, and now
> > you're
> > instead
> > setting it based on num_resets from each machine. The issue is,
> > you're
> > forgetting a bunch of them.
> > 
> > mt8192 didn't get a num_reset, so this commit breaks the display on
> > mt8192 based
> > devices. But mt8192 isn't the only one, there are other platforms
> > missing this
> > property. Please set num_resets to 32 in every single one of them,
> > otherwise
> > there will be display regressions.
> > 
> > Thanks,
> > Nícolas
> 
> It's my mistake. I will set num_resets to 32 for every mmsys driver.
> 
> Thanks,
> Nancy

After review the code, I think only the mmsys driver with the
sw0_rst_offset setting should set num_resets to 32, otherwise it would
set wrong addr in the mtk_mmsys_reset_update() function. Those mmsys
without sw0_rst_offset set should not set num_resets to 32.

Thanks,
Nancy
Nícolas F. R. A. Prado Aug. 19, 2022, 2:25 p.m. UTC | #4 
On Fri, Aug 19, 2022 at 12:13:23PM +0800, Nancy.Lin wrote:
> Hi Nicolas,
> 
> 
> On Fri, 2022-08-19 at 10:09 +0800, Nancy.Lin wrote:
> > Hi Nicolas,
> > 
> > Thanks for the review.
> > 
> > On Thu, 2022-08-18 at 17:47 -0400, Nícolas F. R. A. Prado wrote:
> > > Hi Nancy,
> > > 
> > > On Mon, Jul 11, 2022 at 03:52:42PM +0800, Nancy.Lin wrote:
> > > [..]
> > > >  static const struct mtk_mmsys_driver_data
> > > > mt2701_mmsys_driver_data
> > > > = {
> > > >  	.clk_driver = "clk-mt2701-mm",
> > > >  	.routes = mmsys_default_routing_table,
> > > > @@ -86,6 +88,7 @@ static const struct mtk_mmsys_driver_data
> > > > mt8173_mmsys_driver_data = {
> > > >  	.routes = mmsys_default_routing_table,
> > > >  	.num_routes = ARRAY_SIZE(mmsys_default_routing_table),
> > > >  	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
> > > > +	.num_resets = 32,
> > > >  };
> > > >  
> > > >  static const struct mtk_mmsys_match_data mt8173_mmsys_match_data
> > > > =
> > > > {
> > > > @@ -100,6 +103,7 @@ static const struct mtk_mmsys_driver_data
> > > > mt8183_mmsys_driver_data = {
> > > >  	.routes = mmsys_mt8183_routing_table,
> > > >  	.num_routes = ARRAY_SIZE(mmsys_mt8183_routing_table),
> > > >  	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
> > > > +	.num_resets = 32,
> > > >  };
> > > >  
> > > >  static const struct mtk_mmsys_match_data mt8183_mmsys_match_data
> > > > =
> > > > {
> > > > @@ -114,6 +118,7 @@ static const struct mtk_mmsys_driver_data
> > > > mt8186_mmsys_driver_data = {
> > > >  	.routes = mmsys_mt8186_routing_table,
> > > >  	.num_routes = ARRAY_SIZE(mmsys_mt8186_routing_table),
> > > >  	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
> > > > +	.num_resets = 32,
> > > >  };
> > > 
> > > [..]
> > > > @@ -351,18 +362,6 @@ static int mtk_mmsys_probe(struct
> > > > platform_device *pdev)
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > -	spin_lock_init(&mmsys->lock);
> > > > -
> > > > -	mmsys->rcdev.owner = THIS_MODULE;
> > > > -	mmsys->rcdev.nr_resets = 32;
> > > 
> > > The number of resets was previously always set to 32, and now
> > > you're
> > > instead
> > > setting it based on num_resets from each machine. The issue is,
> > > you're
> > > forgetting a bunch of them.
> > > 
> > > mt8192 didn't get a num_reset, so this commit breaks the display on
> > > mt8192 based
> > > devices. But mt8192 isn't the only one, there are other platforms
> > > missing this
> > > property. Please set num_resets to 32 in every single one of them,
> > > otherwise
> > > there will be display regressions.
> > > 
> > > Thanks,
> > > Nícolas
> > 
> > It's my mistake. I will set num_resets to 32 for every mmsys driver.
> > 
> > Thanks,
> > Nancy
> 
> After review the code, I think only the mmsys driver with the
> sw0_rst_offset setting should set num_resets to 32, otherwise it would
> set wrong addr in the mtk_mmsys_reset_update() function. Those mmsys
> without sw0_rst_offset set should not set num_resets to 32.

I don't think that's the case. num_resets and sw0_rst_offset are completely
unrelated in the code. sw0_rst_offset just gives the offset from the base
mmsys memory to the register that manages resets; it can be 0 in which case the
reset register would be right at the start of the mmsys iospace.

nr_resets, which is set by num_resets, on the other hand, seems to be used in a
single place: of_reset_simple_xlate() in drivers/reset/core.c. And the logic
there is really simple: if you pass a reset id greater than nr_resets, you will
get an error. So when you leave num_resets unset (0) for any of the platforms,
you're making all reset registrations in the DT fail.

So I'm really confident that we need num_resets = 32 on *all* platforms (except
the ones that have 64 of course, we just can't leave them empty).

Thanks,
Nícolas
diff mbox series

Patch

diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
index 999be064103b..953fa5ab271e 100644
--- a/drivers/soc/mediatek/mtk-mmsys.c
+++ b/drivers/soc/mediatek/mtk-mmsys.c
@@ -20,6 +20,8 @@ 
 #include "mt8195-mmsys.h"
 #include "mt8365-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,
@@ -86,6 +88,7 @@  static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
 	.routes = mmsys_default_routing_table,
 	.num_routes = ARRAY_SIZE(mmsys_default_routing_table),
 	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
+	.num_resets = 32,
 };
 
 static const struct mtk_mmsys_match_data mt8173_mmsys_match_data = {
@@ -100,6 +103,7 @@  static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = {
 	.routes = mmsys_mt8183_routing_table,
 	.num_routes = ARRAY_SIZE(mmsys_mt8183_routing_table),
 	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
+	.num_resets = 32,
 };
 
 static const struct mtk_mmsys_match_data mt8183_mmsys_match_data = {
@@ -114,6 +118,7 @@  static const struct mtk_mmsys_driver_data mt8186_mmsys_driver_data = {
 	.routes = mmsys_mt8186_routing_table,
 	.num_routes = ARRAY_SIZE(mmsys_mt8186_routing_table),
 	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
+	.num_resets = 32,
 };
 
 static const struct mtk_mmsys_match_data mt8186_mmsys_match_data = {
@@ -288,13 +293,19 @@  static int mtk_mmsys_reset_update(struct reset_controller_dev *rcdev, unsigned l
 {
 	struct mtk_mmsys *mmsys = container_of(rcdev, struct mtk_mmsys, rcdev);
 	unsigned long flags;
+	u32 offset;
+	u32 reg;
+
+	offset = (id / MMSYS_SW_RESET_PER_REG) * sizeof(u32);
+	id = id % MMSYS_SW_RESET_PER_REG;
+	reg = mmsys->data->sw0_rst_offset + offset;
 
 	spin_lock_irqsave(&mmsys->lock, flags);
 
 	if (assert)
-		mtk_mmsys_update_bits(mmsys, mmsys->data->sw0_rst_offset, BIT(id), 0, NULL);
+		mtk_mmsys_update_bits(mmsys, reg, BIT(id), 0, NULL);
 	else
-		mtk_mmsys_update_bits(mmsys, mmsys->data->sw0_rst_offset, BIT(id), BIT(id), NULL);
+		mtk_mmsys_update_bits(mmsys, reg, BIT(id), BIT(id), NULL);
 
 	spin_unlock_irqrestore(&mmsys->lock, flags);
 
@@ -351,18 +362,6 @@  static int mtk_mmsys_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	spin_lock_init(&mmsys->lock);
-
-	mmsys->rcdev.owner = THIS_MODULE;
-	mmsys->rcdev.nr_resets = 32;
-	mmsys->rcdev.ops = &mtk_mmsys_reset_ops;
-	mmsys->rcdev.of_node = pdev->dev.of_node;
-	ret = devm_reset_controller_register(&pdev->dev, &mmsys->rcdev);
-	if (ret) {
-		dev_err(&pdev->dev, "Couldn't register mmsys reset controller: %d\n", ret);
-		return ret;
-	}
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(dev, "Couldn't get mmsys resource\n");
@@ -384,6 +383,18 @@  static int mtk_mmsys_probe(struct platform_device *pdev)
 		mmsys->data = match_data->drv_data[0];
 	}
 
+	spin_lock_init(&mmsys->lock);
+
+	mmsys->rcdev.owner = THIS_MODULE;
+	mmsys->rcdev.nr_resets = mmsys->data->num_resets;
+	mmsys->rcdev.ops = &mtk_mmsys_reset_ops;
+	mmsys->rcdev.of_node = pdev->dev.of_node;
+	ret = devm_reset_controller_register(&pdev->dev, &mmsys->rcdev);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't register mmsys reset controller: %d\n", ret);
+		return ret;
+	}
+
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	ret = cmdq_dev_get_client_reg(dev, &mmsys->cmdq_base, 0);
 	if (ret)
diff --git a/drivers/soc/mediatek/mtk-mmsys.h b/drivers/soc/mediatek/mtk-mmsys.h
index f01ba206481d..20a271b80b3b 100644
--- a/drivers/soc/mediatek/mtk-mmsys.h
+++ b/drivers/soc/mediatek/mtk-mmsys.h
@@ -92,6 +92,7 @@  struct mtk_mmsys_driver_data {
 	const struct mtk_mmsys_routes *routes;
 	const unsigned int num_routes;
 	const u16 sw0_rst_offset;
+	const u32 num_resets;
 };
 
 struct mtk_mmsys_match_data {