Re: [PATCH v4 06/19] soc: mediatek: Add runtime PM and top clocks and async controls for MMSYS

["Paul-pl Chen (陳柏霖)" <Paul-pl.Chen@mediatek.com>]

On Tue, 2025-09-16 at 03:03 +0000, CK Hu (胡俊光) wrote:
> On Thu, 2025-08-28 at 16:07 +0800, Paul Chen wrote:
> > From: Nancy Lin <nancy.lin@mediatek.com>
> > 
> > - Add initialization of top clocks and async clocks for each MMSYS.
> > - Add PM runtime control and new functions to manage these clocks.
> > - Add functions to set these clocks according to the default
> >   configuration of the corresponding MMSYS.
> 
> This patch include 3 things:
> 1. Add runtime PM
> 2. top clock control
> 3. async control
> 
> I would like you to separate these to different patch.
> 
> > 
> > Sure I will seperate it to three different patch.
> > Signed-off-by: Nancy Lin <nancy.lin@mediatek.com>
> > Signed-off-by: Paul-pl Chen <paul-pl.chen@mediatek.com>
> > ---
> 
> [snip]
> 
> > +
> > +int mtk_mmsys_ddp_clk_enable(struct device *dev, enum
> > mtk_ddp_comp_id comp_id)
> 
> This is to control async clock, so change the function name to
> 
> mtk_mmsys_async_clk_enable()
> 
> 
> OK Got it.
> > +{
> > +	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +	const struct mtk_mmsys_async_info *async = mmsys->data-
> > >async_info;
> > +
> > +	int i;
> > +
> > +	if (!mmsys->data->num_async_info)
> 
> for-loop would check this, so drop this.
> 
Sure, I will drop this condition check.
> > +		return 0;
> > +
> > +	for (i = 0; i < mmsys->data->num_async_info; i++)
> > +		if (comp_id == async[i].comp_id)
> > +			return clk_prepare_enable(mmsys-
> > >async_clk[async[i].index]);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_clk_enable);
> > +
> > +void mtk_mmsys_ddp_clk_disable(struct device *dev, enum
> > mtk_ddp_comp_id comp_id)
> > +{
> > +	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +	const struct mtk_mmsys_async_info *async = mmsys->data-
> > >async_info;
> > +	int i;
> > +
> > +	if (!mmsys->data->num_async_info)
> 
> for-loop would check this, so drop this.
> 
> Sure, I will drop this condition check.
> 
> > +		return;
> > +
> > +	for (i = 0; i < mmsys->data->num_async_info; i++)
> > +		if (comp_id == async[i].comp_id)
> > +			clk_disable_unprepare(mmsys-
> > >async_clk[async[i].index]);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_clk_disable);
> > +
> > +void mtk_mmsys_ddp_config(struct device *dev, enum mtk_ddp_comp_id
> > comp_id,
> > +			  int width, int height, struct cmdq_pkt
> > *cmdq_pkt)
> 
> This is to control async. So change the function name to
> 
> mtk_mmsys_async_config()
> 
> > +{
> > +	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +	const struct mtk_mmsys_async_info *async = mmsys->data-
> > >async_info;
> > +	int i;
> > +	u32 val;
> > +
> > +	if (!mmsys->data->num_async_info)
> 
> If mmsys->data->num_async_info = 0, then i = 0, and it would return
> below.
> So this checking is redundant.
> 
> > +		return;
> > +
> > +	for (i = 0; i < mmsys->data->num_async_info; i++)
> > +		if (comp_id == async[i].comp_id)
> > +			break;
> > +
> > +	if (i == mmsys->data->num_async_info)
> > +		return;
> > +
> > +	val = FIELD_PREP(GENMASK(31, 16), height);
> > +	val |= FIELD_PREP(GENMASK(15, 0), width);
> > +	mtk_mmsys_update_bits(mmsys, async[i].offset,
> > async[i].mask, val, cmdq_pkt);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_config);
> > +
> > +void mtk_mmsys_default_config(struct device *dev)
> 
> Why not do this when mmsys probe?

> 
> 
> The mtk_mmsys_default_config is also using at DP Suspnd /Resume.
Hence, we need to keep the default config setting at here.

Best, Paul-pl Chen

> 
>  
> 
> Regards,
> CK
> 
> > +{
> > +	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +	const struct mtk_mmsys_default *def_config = mmsys->data-
> > >def_config;
> > +	int i;
> > +
> > +	if (!mmsys->data->num_def_config)
> > +		return;
> > +
> > +	for (i = 0; i < mmsys->data->num_def_config; i++)
> > +		mtk_mmsys_update_bits(mmsys, def_config[i].offset,
> > def_config[i].mask,
> > +				      def_config[i].val, NULL);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_default_config);
> > +
> 
>