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

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

[Nícolas F. R. A. Prado]

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

Re: [PATCH v25 00/10] Add MediaTek SoC(vdosys1) support for mt8195

[Nícolas F. R. A. Prado]

On Fri, Aug 05, 2022 at 06:04:08PM +0800, Nancy.Lin wrote:
[..]
> Hello Matthias,
> 
> This series is about mmsys configuration of external display path.
> 
> It is in version *25*, and it is reviewed by many reviewers, like CK
> and Angelo.
> The reset.h is also reviewed by Krzysztof.
> 
> This series is also well tested by Rex-BC and Angelo.

There is actually a silly but breaking bug in this version [1] affecting a lot
of platforms, including mt8192.

I'll provide a Tested-by tag for mt8192 in the next version, just in case. (If
you can, please CC me, Nancy).

Thanks,
Nícolas

[1] https://lore.kernel.org/all/20220818214715.spbyic34szubx3gi@notapiano/

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

[Nícolas F. R. A. Prado]

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