diff mbox series

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

Message ID 20220819061011.7672-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 (林欣螢) Aug. 19, 2022, 6:10 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 | 40 +++++++++++++++++++++-----------
 drivers/soc/mediatek/mtk-mmsys.h |  1 +
 2 files changed, 27 insertions(+), 14 deletions(-)

Comments

Matthias Brugger Aug. 23, 2022, 10:20 a.m. UTC | #1
On 19/08/2022 08:10, Nancy.Lin wrote:
> 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 | 40 +++++++++++++++++++++-----------
>   drivers/soc/mediatek/mtk-mmsys.h |  1 +
>   2 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> index 999be064103b..20ae751ad8a7 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 = {
> @@ -128,6 +133,7 @@ static const struct mtk_mmsys_driver_data mt8192_mmsys_driver_data = {
>   	.routes = mmsys_mt8192_routing_table,
>   	.num_routes = ARRAY_SIZE(mmsys_mt8192_routing_table),
>   	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
> +	.num_resets = 32,

You didn't reply to Nicolas regarding the reset numbers. I actually agree with 
him that we will need the num_resets declared for all devices. Why do you think 
this is not the case?

Regards,
Matthias


>   };
>   
>   static const struct mtk_mmsys_match_data mt8192_mmsys_match_data = {
> @@ -288,13 +294,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 +363,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 +384,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 {
Nancy Lin (林欣螢) Aug. 23, 2022, 11:30 a.m. UTC | #2
Hi Matthias,

Thanks for the review.

On Tue, 2022-08-23 at 12:20 +0200, Matthias Brugger wrote:
> 
> On 19/08/2022 08:10, Nancy.Lin wrote:
> > 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 | 40 +++++++++++++++++++++----
> > -------
> >   drivers/soc/mediatek/mtk-mmsys.h |  1 +
> >   2 files changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > b/drivers/soc/mediatek/mtk-mmsys.c
> > index 999be064103b..20ae751ad8a7 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
> > = {
> > @@ -128,6 +133,7 @@ static const struct mtk_mmsys_driver_data
> > mt8192_mmsys_driver_data = {
> >   	.routes = mmsys_mt8192_routing_table,
> >   	.num_routes = ARRAY_SIZE(mmsys_mt8192_routing_table),
> >   	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
> > +	.num_resets = 32,
> 
> You didn't reply to Nicolas regarding the reset numbers. I actually
> agree with 
> him that we will need the num_resets declared for all devices. Why do
> you think 
> this is not the case?
> 
> Regards,
> Matthias
> 

Sorry, I lost Nicolas's email. 

I checked with the mmsys git log with reset controller function.

1. Enric add mmsys reset controller function in [1]/[2].
   => in mtk_mmsys_reset_update(), all mmsys reset offset is
MMSYS_SW0_RST_B (0x140). 

2. After Enric's patch, Rex add sw0_rst_offset in mmsys driver data in
[3].

So, I think sw0_rst_offset is not zero. Instead of only add num_resets
but also need to add sw0_rst_offset for all mmsys. What do you think ?

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/soc/mediatek/mtk-mmsys.c?id=f27ef2856343e2ddc392975d7b15120442e4d7b7
[2]

https://patchwork.kernel.org/project/linux-mediatek/cover/20210825102632.601614-1-enric.balletbo@collabora.com/
[3]

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/soc/mediatek/mtk-mmsys.c?id=62dc30150c06774a8122c52aedd0eddaceaf5940

Regards,
Nancy
> 
> >   };
> >   
> >   static const struct mtk_mmsys_match_data mt8192_mmsys_match_data
> > = {
> > @@ -288,13 +294,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 +363,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 +384,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 {
> 
>
Matthias Brugger Aug. 23, 2022, 12:08 p.m. UTC | #3
On 23/08/2022 13:30, Nancy.Lin wrote:
> Hi Matthias,
> 
> Thanks for the review.
> 
> On Tue, 2022-08-23 at 12:20 +0200, Matthias Brugger wrote:
>>
>> On 19/08/2022 08:10, Nancy.Lin wrote:
>>> 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 | 40 +++++++++++++++++++++----
>>> -------
>>>    drivers/soc/mediatek/mtk-mmsys.h |  1 +
>>>    2 files changed, 27 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c
>>> b/drivers/soc/mediatek/mtk-mmsys.c
>>> index 999be064103b..20ae751ad8a7 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
>>> = {
>>> @@ -128,6 +133,7 @@ static const struct mtk_mmsys_driver_data
>>> mt8192_mmsys_driver_data = {
>>>    	.routes = mmsys_mt8192_routing_table,
>>>    	.num_routes = ARRAY_SIZE(mmsys_mt8192_routing_table),
>>>    	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
>>> +	.num_resets = 32,
>>
>> You didn't reply to Nicolas regarding the reset numbers. I actually
>> agree with
>> him that we will need the num_resets declared for all devices. Why do
>> you think
>> this is not the case?
>>
>> Regards,
>> Matthias
>>
> 
> Sorry, I lost Nicolas's email.
> 
> I checked with the mmsys git log with reset controller function.
> 
> 1. Enric add mmsys reset controller function in [1]/[2].
>     => in mtk_mmsys_reset_update(), all mmsys reset offset is
> MMSYS_SW0_RST_B (0x140).
> 
> 2. After Enric's patch, Rex add sw0_rst_offset in mmsys driver data in
> [3].
> 
> So, I think sw0_rst_offset is not zero. Instead of only add num_resets
> but also need to add sw0_rst_offset for all mmsys. What do you think ?
> 

Good point. It seems we have a bug in the driver. Either all SoCs have the 
reset, but it's broken since
62dc30150c06 ("soc: mediatek: mmsys: add sw0_rst_offset in mmsys driver data")
or we are adding a reset controller independently if the silicon has one, which 
would be an error in:
f27ef2856343 ("soc: mediatek: mmsys: Add reset controller support")

We have to find that out.

Regards,
Matthias

> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/soc/mediatek/mtk-mmsys.c?id=f27ef2856343e2ddc392975d7b15120442e4d7b7
> [2]
> 
> https://patchwork.kernel.org/project/linux-mediatek/cover/20210825102632.601614-1-enric.balletbo@collabora.com/
> [3]
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/soc/mediatek/mtk-mmsys.c?id=62dc30150c06774a8122c52aedd0eddaceaf5940
> 
> Regards,
> Nancy
>>
>>>    };
>>>    
>>>    static const struct mtk_mmsys_match_data mt8192_mmsys_match_data
>>> = {
>>> @@ -288,13 +294,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 +363,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 +384,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 {
>>
>>
>
Nancy Lin (林欣螢) Aug. 24, 2022, 2:44 a.m. UTC | #4
Hi Matthias,

Thanks for your comment.

On Tue, 2022-08-23 at 14:08 +0200, Matthias Brugger wrote:
> 
> On 23/08/2022 13:30, Nancy.Lin wrote:
> > Hi Matthias,
> > 
> > Thanks for the review.
> > 
> > On Tue, 2022-08-23 at 12:20 +0200, Matthias Brugger wrote:
> > > 
> > > On 19/08/2022 08:10, Nancy.Lin wrote:
> > > > 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 | 40 +++++++++++++++++++++-
> > > > ---
> > > > -------
> > > >    drivers/soc/mediatek/mtk-mmsys.h |  1 +
> > > >    2 files changed, 27 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > > > b/drivers/soc/mediatek/mtk-mmsys.c
> > > > index 999be064103b..20ae751ad8a7 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
> > > > = {
> > > > @@ -128,6 +133,7 @@ static const struct mtk_mmsys_driver_data
> > > > mt8192_mmsys_driver_data = {
> > > >    	.routes = mmsys_mt8192_routing_table,
> > > >    	.num_routes = ARRAY_SIZE(mmsys_mt8192_routing_table),
> > > >    	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
> > > > +	.num_resets = 32,
> > > 
> > > You didn't reply to Nicolas regarding the reset numbers. I
> > > actually
> > > agree with
> > > him that we will need the num_resets declared for all devices.
> > > Why do
> > > you think
> > > this is not the case?
> > > 
> > > Regards,
> > > Matthias
> > > 
> > 
> > Sorry, I lost Nicolas's email.
> > 
> > I checked with the mmsys git log with reset controller function.
> > 
> > 1. Enric add mmsys reset controller function in [1]/[2].
> >     => in mtk_mmsys_reset_update(), all mmsys reset offset is
> > MMSYS_SW0_RST_B (0x140).
> > 
> > 2. After Enric's patch, Rex add sw0_rst_offset in mmsys driver data
> > in
> > [3].
> > 
> > So, I think sw0_rst_offset is not zero. Instead of only add
> > num_resets
> > but also need to add sw0_rst_offset for all mmsys. What do you
> > think ?
> > 
> 
> Good point. It seems we have a bug in the driver. Either all SoCs
> have the 
> reset, but it's broken since
> 62dc30150c06 ("soc: mediatek: mmsys: add sw0_rst_offset in mmsys
> driver data")
> or we are adding a reset controller independently if the silicon has
> one, which 
> would be an error in:
> f27ef2856343 ("soc: mediatek: mmsys: Add reset controller support")
> 
> We have to find that out.
> 
> Regards,
> Matthias


In [2], I think the first revision of Enric's reset controller is added
for 8173 and 8183, not for all mmsys device.
    =>[v3,4/7] arm64: dts: mt8173: Add the mmsys reset bit to reset the
dsi0
    =>[v3,5/7] arm64: dts: mt8183: Add the mmsys reset bit to reset the
dsi0

In [3], Rex only add sw0_rst_offset in 8173 and 8183 mmsys driver data.
> 

For other SoCs, like mt2701,mt2712..., these SoCs even don't define
mmsys hw reset bit[4]. So I think only set the num_resets to 32 or 64
to those mmsys devices who really need the reset control, others set to
0(same as my v26 patch).


[4]mt2701-resets.h 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/mt2701-resets.h?id=62dc30150c06774a8122c52aedd0eddaceaf5940

Regards,
Nancy


> > [1]
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/soc/mediatek/mtk-mmsys.c?id=f27ef2856343e2ddc392975d7b15120442e4d7b7__;!!CTRNKA9wMg0ARbw!3cWAYlD1mrWRmNZy0zoJs8MNiD3s7K9PteJI9cGEvu_qp3VShfqxsBTb_fKynszs$
> >  
> > [2]
> > 
> > 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/cover/20210825102632.601614-1-enric.balletbo@collabora.com/__;!!CTRNKA9wMg0ARbw!3cWAYlD1mrWRmNZy0zoJs8MNiD3s7K9PteJI9cGEvu_qp3VShfqxsBTb_cH-3nM8$
> >  
> > [3]
> > 
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/soc/mediatek/mtk-mmsys.c?id=62dc30150c06774a8122c52aedd0eddaceaf5940__;!!CTRNKA9wMg0ARbw!3cWAYlD1mrWRmNZy0zoJs8MNiD3s7K9PteJI9cGEvu_qp3VShfqxsBTb_VXEsbNa$
> >  
> > 
> > Regards,
> > Nancy
> > > 
> > > >    };
> > > >    
> > > >    static const struct mtk_mmsys_match_data
> > > > mt8192_mmsys_match_data
> > > > = {
> > > > @@ -288,13 +294,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 +363,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 +384,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 {
> > > 
> > >
Matthias Brugger Aug. 24, 2022, 11:41 a.m. UTC | #5
On 24/08/2022 04:44, Nancy.Lin wrote:
> Hi Matthias,
> 
> Thanks for your comment.
> 
> On Tue, 2022-08-23 at 14:08 +0200, Matthias Brugger wrote:
>>
>> On 23/08/2022 13:30, Nancy.Lin wrote:
>>> Hi Matthias,
>>>
>>> Thanks for the review.
>>>
>>> On Tue, 2022-08-23 at 12:20 +0200, Matthias Brugger wrote:
>>>>
>>>> On 19/08/2022 08:10, Nancy.Lin wrote:
>>>>> 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 | 40 +++++++++++++++++++++-
>>>>> ---
>>>>> -------
>>>>>     drivers/soc/mediatek/mtk-mmsys.h |  1 +
>>>>>     2 files changed, 27 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c
>>>>> b/drivers/soc/mediatek/mtk-mmsys.c
>>>>> index 999be064103b..20ae751ad8a7 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
>>>>> = {
>>>>> @@ -128,6 +133,7 @@ static const struct mtk_mmsys_driver_data
>>>>> mt8192_mmsys_driver_data = {
>>>>>     	.routes = mmsys_mt8192_routing_table,
>>>>>     	.num_routes = ARRAY_SIZE(mmsys_mt8192_routing_table),
>>>>>     	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
>>>>> +	.num_resets = 32,
>>>>
>>>> You didn't reply to Nicolas regarding the reset numbers. I
>>>> actually
>>>> agree with
>>>> him that we will need the num_resets declared for all devices.
>>>> Why do
>>>> you think
>>>> this is not the case?
>>>>
>>>> Regards,
>>>> Matthias
>>>>
>>>
>>> Sorry, I lost Nicolas's email.
>>>
>>> I checked with the mmsys git log with reset controller function.
>>>
>>> 1. Enric add mmsys reset controller function in [1]/[2].
>>>      => in mtk_mmsys_reset_update(), all mmsys reset offset is
>>> MMSYS_SW0_RST_B (0x140).
>>>
>>> 2. After Enric's patch, Rex add sw0_rst_offset in mmsys driver data
>>> in
>>> [3].
>>>
>>> So, I think sw0_rst_offset is not zero. Instead of only add
>>> num_resets
>>> but also need to add sw0_rst_offset for all mmsys. What do you
>>> think ?
>>>
>>
>> Good point. It seems we have a bug in the driver. Either all SoCs
>> have the
>> reset, but it's broken since
>> 62dc30150c06 ("soc: mediatek: mmsys: add sw0_rst_offset in mmsys
>> driver data")
>> or we are adding a reset controller independently if the silicon has
>> one, which
>> would be an error in:
>> f27ef2856343 ("soc: mediatek: mmsys: Add reset controller support")
>>
>> We have to find that out.
>>
>> Regards,
>> Matthias
> 
> 
> In [2], I think the first revision of Enric's reset controller is added
> for 8173 and 8183, not for all mmsys device.
>      =>[v3,4/7] arm64: dts: mt8173: Add the mmsys reset bit to reset the
> dsi0
>      =>[v3,5/7] arm64: dts: mt8183: Add the mmsys reset bit to reset the
> dsi0
> 
> In [3], Rex only add sw0_rst_offset in 8173 and 8183 mmsys driver data.
>>
> 
> For other SoCs, like mt2701,mt2712..., these SoCs even don't define
> mmsys hw reset bit[4]. So I think only set the num_resets to 32 or 64
> to those mmsys devices who really need the reset control, others set to
> 0(same as my v26 patch).
> 

Thanks for looking into this, please see my comment further below.


> 
> [4]mt2701-resets.h
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/mt2701-resets.h?id=62dc30150c06774a8122c52aedd0eddaceaf5940
> 
> Regards,
> Nancy
> 
> 
>>> [1]
>>>
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/soc/mediatek/mtk-mmsys.c?id=f27ef2856343e2ddc392975d7b15120442e4d7b7__;!!CTRNKA9wMg0ARbw!3cWAYlD1mrWRmNZy0zoJs8MNiD3s7K9PteJI9cGEvu_qp3VShfqxsBTb_fKynszs$
>>>   
>>> [2]
>>>
>>>
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/cover/20210825102632.601614-1-enric.balletbo@collabora.com/__;!!CTRNKA9wMg0ARbw!3cWAYlD1mrWRmNZy0zoJs8MNiD3s7K9PteJI9cGEvu_qp3VShfqxsBTb_cH-3nM8$
>>>   
>>> [3]
>>>
>>>
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/soc/mediatek/mtk-mmsys.c?id=62dc30150c06774a8122c52aedd0eddaceaf5940__;!!CTRNKA9wMg0ARbw!3cWAYlD1mrWRmNZy0zoJs8MNiD3s7K9PteJI9cGEvu_qp3VShfqxsBTb_VXEsbNa$
>>>   
>>>
>>> Regards,
>>> Nancy
>>>>
>>>>>     };
>>>>>     
>>>>>     static const struct mtk_mmsys_match_data
>>>>> mt8192_mmsys_match_data
>>>>> = {
>>>>> @@ -288,13 +294,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 +363,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 +384,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;
>>>>> +	}
>>>>> +


This code is only relevant if mmsys->data->num_resets > 0. Let's check for that 
before setting up and registering an interrupt controller. What do you think?

Regards,
Matthias

>>>>>     #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 {
>>>>
>>>>
>
Nancy Lin (林欣螢) Aug. 25, 2022, 2:19 a.m. UTC | #6
Hi Matthias,

Thanks for your comment.

On Wed, 2022-08-24 at 13:41 +0200, Matthias Brugger wrote:
> 
> On 24/08/2022 04:44, Nancy.Lin wrote:
> > Hi Matthias,
> > 
> > Thanks for your comment.
> > 
> > On Tue, 2022-08-23 at 14:08 +0200, Matthias Brugger wrote:
> > > 
> > > On 23/08/2022 13:30, Nancy.Lin wrote:
> > > > Hi Matthias,
> > > > 
> > > > Thanks for the review.
> > > > 
> > > > On Tue, 2022-08-23 at 12:20 +0200, Matthias Brugger wrote:
> > > > > 
> > > > > On 19/08/2022 08:10, Nancy.Lin wrote:
> > > > > > 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 | 40
> > > > > > +++++++++++++++++++++-
> > > > > > ---
> > > > > > -------
> > > > > >     drivers/soc/mediatek/mtk-mmsys.h |  1 +
> > > > > >     2 files changed, 27 insertions(+), 14 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > > > > > b/drivers/soc/mediatek/mtk-mmsys.c
> > > > > > index 999be064103b..20ae751ad8a7 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
> > > > > > = {
> > > > > > @@ -128,6 +133,7 @@ static const struct
> > > > > > mtk_mmsys_driver_data
> > > > > > mt8192_mmsys_driver_data = {
> > > > > >     	.routes = mmsys_mt8192_routing_table,
> > > > > >     	.num_routes =
> > > > > > ARRAY_SIZE(mmsys_mt8192_routing_table),
> > > > > >     	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
> > > > > > +	.num_resets = 32,
> > > > > 
> > > > > You didn't reply to Nicolas regarding the reset numbers. I
> > > > > actually
> > > > > agree with
> > > > > him that we will need the num_resets declared for all
> > > > > devices.
> > > > > Why do
> > > > > you think
> > > > > this is not the case?
> > > > > 
> > > > > Regards,
> > > > > Matthias
> > > > > 
> > > > 
> > > > Sorry, I lost Nicolas's email.
> > > > 
> > > > I checked with the mmsys git log with reset controller
> > > > function.
> > > > 
> > > > 1. Enric add mmsys reset controller function in [1]/[2].
> > > >      => in mtk_mmsys_reset_update(), all mmsys reset offset is
> > > > MMSYS_SW0_RST_B (0x140).
> > > > 
> > > > 2. After Enric's patch, Rex add sw0_rst_offset in mmsys driver
> > > > data
> > > > in
> > > > [3].
> > > > 
> > > > So, I think sw0_rst_offset is not zero. Instead of only add
> > > > num_resets
> > > > but also need to add sw0_rst_offset for all mmsys. What do you
> > > > think ?
> > > > 
> > > 
> > > Good point. It seems we have a bug in the driver. Either all SoCs
> > > have the
> > > reset, but it's broken since
> > > 62dc30150c06 ("soc: mediatek: mmsys: add sw0_rst_offset in mmsys
> > > driver data")
> > > or we are adding a reset controller independently if the silicon
> > > has
> > > one, which
> > > would be an error in:
> > > f27ef2856343 ("soc: mediatek: mmsys: Add reset controller
> > > support")
> > > 
> > > We have to find that out.
> > > 
> > > Regards,
> > > Matthias
> > 
> > 
> > In [2], I think the first revision of Enric's reset controller is
> > added
> > for 8173 and 8183, not for all mmsys device.
> >      =>[v3,4/7] arm64: dts: mt8173: Add the mmsys reset bit to
> > reset the
> > dsi0
> >      =>[v3,5/7] arm64: dts: mt8183: Add the mmsys reset bit to
> > reset the
> > dsi0
> > 
> > In [3], Rex only add sw0_rst_offset in 8173 and 8183 mmsys driver
> > data.
> > > 
> > 
> > For other SoCs, like mt2701,mt2712..., these SoCs even don't define
> > mmsys hw reset bit[4]. So I think only set the num_resets to 32 or
> > 64
> > to those mmsys devices who really need the reset control, others
> > set to
> > 0(same as my v26 patch).
> > 
> 
> Thanks for looking into this, please see my comment further below.
> 
> 
> > 
> > [4]mt2701-resets.h
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/mt2701-resets.h?id=62dc30150c06774a8122c52aedd0eddaceaf5940__;!!CTRNKA9wMg0ARbw!3j1YotescyEb7vq_dITIlc-FdtaFSslPkcn3B-Sw95Zf613Z-TPG4FK0BdCyW0Fb$
> >  
> > 
> > Regards,
> > Nancy
> > 
> > 
> > > > [1]
> > > > 
> > 
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/soc/mediatek/mtk-mmsys.c?id=f27ef2856343e2ddc392975d7b15120442e4d7b7__;!!CTRNKA9wMg0ARbw!3cWAYlD1mrWRmNZy0zoJs8MNiD3s7K9PteJI9cGEvu_qp3VShfqxsBTb_fKynszs$
> > > >   
> > > > [2]
> > > > 
> > > > 
> > 
> > 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/cover/20210825102632.601614-1-enric.balletbo@collabora.com/__;!!CTRNKA9wMg0ARbw!3cWAYlD1mrWRmNZy0zoJs8MNiD3s7K9PteJI9cGEvu_qp3VShfqxsBTb_cH-3nM8$
> > > >   
> > > > [3]
> > > > 
> > > > 
> > 
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/soc/mediatek/mtk-mmsys.c?id=62dc30150c06774a8122c52aedd0eddaceaf5940__;!!CTRNKA9wMg0ARbw!3cWAYlD1mrWRmNZy0zoJs8MNiD3s7K9PteJI9cGEvu_qp3VShfqxsBTb_VXEsbNa$
> > > >   
> > > > 
> > > > Regards,
> > > > Nancy
> > > > > 
> > > > > >     };
> > > > > >     
> > > > > >     static const struct mtk_mmsys_match_data
> > > > > > mt8192_mmsys_match_data
> > > > > > = {
> > > > > > @@ -288,13 +294,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 +363,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 +384,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;
> > > > > > +	}
> > > > > > +
> 
> 
> This code is only relevant if mmsys->data->num_resets > 0. Let's
> check for that 
> before setting up and registering an interrupt controller. What do
> you think?
> 
> Regards,
> Matthias
> 
There is no doubt that we need to check num_resets > 0 before
registering reset controller.

I will add it in the next revision.

Regards,
Nancy


> > > > > >     #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 {
> > > > > 
> > > > > 
> 
>
diff mbox series

Patch

diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
index 999be064103b..20ae751ad8a7 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 = {
@@ -128,6 +133,7 @@  static const struct mtk_mmsys_driver_data mt8192_mmsys_driver_data = {
 	.routes = mmsys_mt8192_routing_table,
 	.num_routes = ARRAY_SIZE(mmsys_mt8192_routing_table),
 	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
+	.num_resets = 32,
 };
 
 static const struct mtk_mmsys_match_data mt8192_mmsys_match_data = {
@@ -288,13 +294,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 +363,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 +384,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 {