Re: [PATCH v28 06/11] soc: mediatek: add mtk-mmsys config API for mt8195 vdosys1

Re: [PATCH v28 06/11] soc: mediatek: add mtk-mmsys config API for mt8195 vdosys1

[Matthias Brugger]

On 07/11/2022 08:22, Nancy.Lin wrote:
> Add four mmsys config APIs. The config APIs are used for config
> mmsys reg. Some mmsys regs need to be set according to the
> HW engine binding to the mmsys simultaneously.
> 
> 1. mtk_mmsys_merge_async_config: config merge async width/height.
>     async is used for cross-clock domain synchronization.
> 2. mtk_mmsys_hdr_confing: config hdr backend async width/height.
> 3. mtk_mmsys_mixer_in_config and mtk_mmsys_mixer_in_config:
>     config mixer related settings.
> 
> 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: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>

Not something we need to fix in this series, but it would make sense instead of 
adding all the EXPORTS to pass the functions as callbacks in the 
platform_device_register_data. But I realize you don't pass the VDOSYS number to 
the DRM driver to distinguish between the different MMSYS devices that created 
the platform device. I hadn't had a deep look on the DRM implementation but I 
suppose it will be challenge...

Regards,
Matthias

> ---
>   drivers/soc/mediatek/mt8195-mmsys.h    |  6 +++++
>   drivers/soc/mediatek/mtk-mmsys.c       | 35 ++++++++++++++++++++++++++
>   include/linux/soc/mediatek/mtk-mmsys.h |  9 +++++++
>   3 files changed, 50 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mt8195-mmsys.h b/drivers/soc/mediatek/mt8195-mmsys.h
> index fd7b455bd675..454944a9409c 100644
> --- a/drivers/soc/mediatek/mt8195-mmsys.h
> +++ b/drivers/soc/mediatek/mt8195-mmsys.h
> @@ -75,6 +75,12 @@
>   #define MT8195_SOUT_DSC_WRAP1_OUT_TO_SINA_VIRTUAL0		(2 << 16)
>   #define MT8195_SOUT_DSC_WRAP1_OUT_TO_VPP_MERGE			(3 << 16)
>   
> +#define MT8195_VDO1_MERGE0_ASYNC_CFG_WD				0xe30
> +#define MT8195_VDO1_HDRBE_ASYNC_CFG_WD				0xe70
> +#define MT8195_VDO1_HDR_TOP_CFG					0xd00
> +#define MT8195_VDO1_MIXER_IN1_ALPHA				0xd30
> +#define MT8195_VDO1_MIXER_IN1_PAD				0xd40
> +
>   #define MT8195_VDO1_VPP_MERGE0_P0_SEL_IN			0xf04
>   #define MT8195_VPP_MERGE0_P0_SEL_IN_FROM_MDP_RDMA0			1
>   
> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> index 73c8bd27e6ae..6040a3cff6f8 100644
> --- a/drivers/soc/mediatek/mtk-mmsys.c
> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> @@ -137,6 +137,41 @@ void mtk_mmsys_ddp_disconnect(struct device *dev,
>   }
>   EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_disconnect);
>   
> +void mtk_mmsys_merge_async_config(struct device *dev, int idx, int width, int height)
> +{
> +	mtk_mmsys_update_bits(dev_get_drvdata(dev), MT8195_VDO1_MERGE0_ASYNC_CFG_WD + 0x10 * idx,
> +			      ~0, height << 16 | width);
> +}
> +EXPORT_SYMBOL_GPL(mtk_mmsys_merge_async_config);
> +
> +void mtk_mmsys_hdr_config(struct device *dev, int be_width, int be_height)
> +{
> +	mtk_mmsys_update_bits(dev_get_drvdata(dev), MT8195_VDO1_HDRBE_ASYNC_CFG_WD, ~0,
> +			      be_height << 16 | be_width);
> +}
> +EXPORT_SYMBOL_GPL(mtk_mmsys_hdr_config);
> +
> +void mtk_mmsys_mixer_in_config(struct device *dev, int idx, bool alpha_sel, u16 alpha,
> +			       u8 mode, u32 biwidth)
> +{
> +	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> +
> +	mtk_mmsys_update_bits(mmsys, MT8195_VDO1_MIXER_IN1_ALPHA + (idx - 1) * 4, ~0,
> +			      alpha << 16 | alpha);
> +	mtk_mmsys_update_bits(mmsys, MT8195_VDO1_HDR_TOP_CFG, BIT(19 + idx),
> +			      alpha_sel << (19 + idx));
> +	mtk_mmsys_update_bits(mmsys, MT8195_VDO1_MIXER_IN1_PAD + (idx - 1) * 4,
> +			      GENMASK(31, 16) | GENMASK(1, 0), biwidth << 16 | mode);
> +}
> +EXPORT_SYMBOL_GPL(mtk_mmsys_mixer_in_config);
> +
> +void mtk_mmsys_mixer_in_channel_swap(struct device *dev, int idx, bool channel_swap)
> +{
> +	mtk_mmsys_update_bits(dev_get_drvdata(dev), MT8195_VDO1_MIXER_IN1_PAD + (idx - 1) * 4,
> +			      BIT(4), channel_swap << 4);
> +}
> +EXPORT_SYMBOL_GPL(mtk_mmsys_mixer_in_channel_swap);
> +
>   void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val)
>   {
>   	if (val)
> diff --git a/include/linux/soc/mediatek/mtk-mmsys.h b/include/linux/soc/mediatek/mtk-mmsys.h
> index 127f1b888ace..a4708859c188 100644
> --- a/include/linux/soc/mediatek/mtk-mmsys.h
> +++ b/include/linux/soc/mediatek/mtk-mmsys.h
> @@ -75,4 +75,13 @@ void mtk_mmsys_ddp_disconnect(struct device *dev,
>   
>   void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val);
>   
> +void mtk_mmsys_merge_async_config(struct device *dev, int idx, int width, int height);
> +
> +void mtk_mmsys_hdr_config(struct device *dev, int be_width, int be_height);
> +
> +void mtk_mmsys_mixer_in_config(struct device *dev, int idx, bool alpha_sel, u16 alpha,
> +			       u8 mode, u32 biwidth);
> +
> +void mtk_mmsys_mixer_in_channel_swap(struct device *dev, int idx, bool channel_swap);
> +
>   #endif /* __MTK_MMSYS_H */

Re: [PATCH v28 06/11] soc: mediatek: add mtk-mmsys config API for mt8195 vdosys1

[Nancy Lin (林欣螢)]

Dear Matthias,

Thanks for the review.


On Tue, 2022-11-08 at 18:46 +0100, Matthias Brugger wrote:
> 
> On 07/11/2022 08:22, Nancy.Lin wrote:
> > Add four mmsys config APIs. The config APIs are used for config
> > mmsys reg. Some mmsys regs need to be set according to the
> > HW engine binding to the mmsys simultaneously.
> > 
> > 1. mtk_mmsys_merge_async_config: config merge async width/height.
> >     async is used for cross-clock domain synchronization.
> > 2. mtk_mmsys_hdr_confing: config hdr backend async width/height.
> > 3. mtk_mmsys_mixer_in_config and mtk_mmsys_mixer_in_config:
> >     config mixer related settings.
> > 
> > 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: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> 
> Not something we need to fix in this series, but it would make sense
> instead of 
> adding all the EXPORTS to pass the functions as callbacks in the 
> platform_device_register_data. But I realize you don't pass the
> VDOSYS number to 
> the DRM driver to distinguish between the different MMSYS devices
> that created 
> the platform device. I hadn't had a deep look on the DRM
> implementation but I 
> suppose it will be challenge...
> 
> Regards,
> Matthias
> 

Do you mean to move all the mmsys config APIs as callback in mmsys
driver data?
If so, not only the four mmsys config APIs in this patch but also the
following original three APIs.
It would take some refactoring effort. I think it would be better to
change this after this series for vdosys1 if needed.

1. mtk_mmsys_ddp_connect
2. mtk_mmsys_ddp_disconnect
3. mtk_mmsys_ddp_dpi_fmt_config

Regards,
Nancy

> > ---
> >   drivers/soc/mediatek/mt8195-mmsys.h    |  6 +++++
> >   drivers/soc/mediatek/mtk-mmsys.c       | 35
> > ++++++++++++++++++++++++++
> >   include/linux/soc/mediatek/mtk-mmsys.h |  9 +++++++
> >   3 files changed, 50 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mt8195-mmsys.h
> > b/drivers/soc/mediatek/mt8195-mmsys.h
> > index fd7b455bd675..454944a9409c 100644
> > --- a/drivers/soc/mediatek/mt8195-mmsys.h
> > +++ b/drivers/soc/mediatek/mt8195-mmsys.h
> > @@ -75,6 +75,12 @@
> >   #define MT8195_SOUT_DSC_WRAP1_OUT_TO_SINA_VIRTUAL0		
> > (2 << 16)
> >   #define MT8195_SOUT_DSC_WRAP1_OUT_TO_VPP_MERGE			
> > (3 << 16)
> >   
> > +#define MT8195_VDO1_MERGE0_ASYNC_CFG_WD				
> > 0xe30
> > +#define MT8195_VDO1_HDRBE_ASYNC_CFG_WD				
> > 0xe70
> > +#define MT8195_VDO1_HDR_TOP_CFG					
> > 0xd00
> > +#define MT8195_VDO1_MIXER_IN1_ALPHA				
> > 0xd30
> > +#define MT8195_VDO1_MIXER_IN1_PAD				0xd40
> > +
> >   #define MT8195_VDO1_VPP_MERGE0_P0_SEL_IN			0xf04
> >   #define MT8195_VPP_MERGE0_P0_SEL_IN_FROM_MDP_RDMA0		
> > 	1
> >   
> > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > b/drivers/soc/mediatek/mtk-mmsys.c
> > index 73c8bd27e6ae..6040a3cff6f8 100644
> > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > @@ -137,6 +137,41 @@ void mtk_mmsys_ddp_disconnect(struct device
> > *dev,
> >   }
> >   EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_disconnect);
> >   
> > +void mtk_mmsys_merge_async_config(struct device *dev, int idx, int
> > width, int height)
> > +{
> > +	mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > MT8195_VDO1_MERGE0_ASYNC_CFG_WD + 0x10 * idx,
> > +			      ~0, height << 16 | width);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_merge_async_config);
> > +
> > +void mtk_mmsys_hdr_config(struct device *dev, int be_width, int
> > be_height)
> > +{
> > +	mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > MT8195_VDO1_HDRBE_ASYNC_CFG_WD, ~0,
> > +			      be_height << 16 | be_width);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_hdr_config);
> > +
> > +void mtk_mmsys_mixer_in_config(struct device *dev, int idx, bool
> > alpha_sel, u16 alpha,
> > +			       u8 mode, u32 biwidth)
> > +{
> > +	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +
> > +	mtk_mmsys_update_bits(mmsys, MT8195_VDO1_MIXER_IN1_ALPHA + (idx
> > - 1) * 4, ~0,
> > +			      alpha << 16 | alpha);
> > +	mtk_mmsys_update_bits(mmsys, MT8195_VDO1_HDR_TOP_CFG, BIT(19 +
> > idx),
> > +			      alpha_sel << (19 + idx));
> > +	mtk_mmsys_update_bits(mmsys, MT8195_VDO1_MIXER_IN1_PAD + (idx -
> > 1) * 4,
> > +			      GENMASK(31, 16) | GENMASK(1, 0), biwidth
> > << 16 | mode);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_mixer_in_config);
> > +
> > +void mtk_mmsys_mixer_in_channel_swap(struct device *dev, int idx,
> > bool channel_swap)
> > +{
> > +	mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > MT8195_VDO1_MIXER_IN1_PAD + (idx - 1) * 4,
> > +			      BIT(4), channel_swap << 4);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_mixer_in_channel_swap);
> > +
> >   void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val)
> >   {
> >   	if (val)
> > diff --git a/include/linux/soc/mediatek/mtk-mmsys.h
> > b/include/linux/soc/mediatek/mtk-mmsys.h
> > index 127f1b888ace..a4708859c188 100644
> > --- a/include/linux/soc/mediatek/mtk-mmsys.h
> > +++ b/include/linux/soc/mediatek/mtk-mmsys.h
> > @@ -75,4 +75,13 @@ void mtk_mmsys_ddp_disconnect(struct device
> > *dev,
> >   
> >   void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val);
> >   
> > +void mtk_mmsys_merge_async_config(struct device *dev, int idx, int
> > width, int height);
> > +
> > +void mtk_mmsys_hdr_config(struct device *dev, int be_width, int
> > be_height);
> > +
> > +void mtk_mmsys_mixer_in_config(struct device *dev, int idx, bool
> > alpha_sel, u16 alpha,
> > +			       u8 mode, u32 biwidth);
> > +
> > +void mtk_mmsys_mixer_in_channel_swap(struct device *dev, int idx,
> > bool channel_swap);
> > +
> >   #endif /* __MTK_MMSYS_H */

Re: [PATCH v28 04/11] soc: mediatek: add mtk-mmsys support for mt8195 vdosys1

[Matthias Brugger]

On 07/11/2022 08:22, Nancy.Lin wrote:
> Add mt8195 vdosys1 routing table to the driver data of mtk-mmsys.
> 
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> ---
>   drivers/soc/mediatek/mt8195-mmsys.h | 139 ++++++++++++++++++++++++++++
>   drivers/soc/mediatek/mtk-mmsys.c    |  10 ++
>   2 files changed, 149 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mt8195-mmsys.h b/drivers/soc/mediatek/mt8195-mmsys.h
> index abfe94a30248..fd7b455bd675 100644
> --- a/drivers/soc/mediatek/mt8195-mmsys.h
> +++ b/drivers/soc/mediatek/mt8195-mmsys.h
> @@ -75,6 +75,70 @@
>   #define MT8195_SOUT_DSC_WRAP1_OUT_TO_SINA_VIRTUAL0		(2 << 16)
>   #define MT8195_SOUT_DSC_WRAP1_OUT_TO_VPP_MERGE			(3 << 16)
>   
> +#define MT8195_VDO1_VPP_MERGE0_P0_SEL_IN			0xf04
> +#define MT8195_VPP_MERGE0_P0_SEL_IN_FROM_MDP_RDMA0			1
> +
> +#define MT8195_VDO1_VPP_MERGE0_P1_SEL_IN			0xf08
> +#define MT8195_VPP_MERGE0_P1_SEL_IN_FROM_MDP_RDMA1			1
> +
> +#define MT8195_VDO1_DISP_DPI1_SEL_IN				0xf10
> +#define MT8195_DISP_DPI1_SEL_IN_FROM_VPP_MERGE4_MOUT			0
> +
> +#define MT8195_VDO1_DISP_DP_INTF0_SEL_IN			0xf14
> +#define MT8195_DISP_DP_INTF0_SEL_IN_FROM_VPP_MERGE4_MOUT		0
> +
> +#define MT8195_VDO1_MERGE4_SOUT_SEL				0xf18
> +#define MT8195_MERGE4_SOUT_TO_DPI1_SEL					2
> +#define MT8195_MERGE4_SOUT_TO_DP_INTF0_SEL				3
> +
> +#define MT8195_VDO1_MIXER_IN1_SEL_IN				0xf24
> +#define MT8195_MIXER_IN1_SEL_IN_FROM_MERGE0_ASYNC_SOUT			1
> +
> +#define MT8195_VDO1_MIXER_IN2_SEL_IN				0xf28
> +#define MT8195_MIXER_IN2_SEL_IN_FROM_MERGE1_ASYNC_SOUT			1
> +
> +#define MT8195_VDO1_MIXER_IN3_SEL_IN				0xf2c
> +#define MT8195_MIXER_IN3_SEL_IN_FROM_MERGE2_ASYNC_SOUT			1
> +
> +#define MT8195_VDO1_MIXER_IN4_SEL_IN				0xf30
> +#define MT8195_MIXER_IN4_SEL_IN_FROM_MERGE3_ASYNC_SOUT			1
> +
> +#define MT8195_VDO1_MIXER_OUT_SOUT_SEL				0xf34
> +#define MT8195_MIXER_SOUT_TO_MERGE4_ASYNC_SEL				1
> +
> +#define MT8195_VDO1_VPP_MERGE1_P0_SEL_IN			0xf3c
> +#define MT8195_VPP_MERGE1_P0_SEL_IN_FROM_MDP_RDMA2			1
> +
> +#define MT8195_VDO1_MERGE0_ASYNC_SOUT_SEL			0xf40
> +#define MT8195_SOUT_TO_MIXER_IN1_SEL					1
> +
> +#define MT8195_VDO1_MERGE1_ASYNC_SOUT_SEL			0xf44
> +#define MT8195_SOUT_TO_MIXER_IN2_SEL					1
> +
> +#define MT8195_VDO1_MERGE2_ASYNC_SOUT_SEL			0xf48
> +#define MT8195_SOUT_TO_MIXER_IN3_SEL					1
> +
> +#define MT8195_VDO1_MERGE3_ASYNC_SOUT_SEL			0xf4c
> +#define MT8195_SOUT_TO_MIXER_IN4_SEL					1
> +
> +#define MT8195_VDO1_MERGE4_ASYNC_SEL_IN				0xf50
> +#define MT8195_MERGE4_ASYNC_SEL_IN_FROM_MIXER_OUT_SOUT			1
> +
> +#define MT8195_VDO1_MIXER_IN1_SOUT_SEL				0xf58
> +#define MT8195_MIXER_IN1_SOUT_TO_DISP_MIXER				0
> +
> +#define MT8195_VDO1_MIXER_IN2_SOUT_SEL				0xf5c
> +#define MT8195_MIXER_IN2_SOUT_TO_DISP_MIXER				0
> +
> +#define MT8195_VDO1_MIXER_IN3_SOUT_SEL				0xf60
> +#define MT8195_MIXER_IN3_SOUT_TO_DISP_MIXER				0
> +
> +#define MT8195_VDO1_MIXER_IN4_SOUT_SEL				0xf64
> +#define MT8195_MIXER_IN4_SOUT_TO_DISP_MIXER				0
> +
> +#define MT8195_VDO1_MIXER_SOUT_SEL_IN				0xf68
> +#define MT8195_MIXER_SOUT_SEL_IN_FROM_DISP_MIXER			0
> +
>   static const struct mtk_mmsys_routes mmsys_mt8195_routing_table[] = {
>   	{
>   		DDP_COMPONENT_OVL0, DDP_COMPONENT_RDMA0,
> @@ -367,4 +431,79 @@ static const struct mtk_mmsys_routes mmsys_mt8195_routing_table[] = {
>   	}
>   };
>   
> +static const struct mtk_mmsys_routes mmsys_mt8195_vdo1_routing_table[] = {
> +	{
> +		DDP_COMPONENT_MDP_RDMA0, DDP_COMPONENT_MERGE1,
> +		MT8195_VDO1_VPP_MERGE0_P0_SEL_IN, GENMASK(0, 0),
> +		MT8195_VPP_MERGE0_P0_SEL_IN_FROM_MDP_RDMA0
> +	}, {
> +		DDP_COMPONENT_MDP_RDMA1, DDP_COMPONENT_MERGE1,
> +		MT8195_VDO1_VPP_MERGE0_P1_SEL_IN, GENMASK(0, 0),
> +		MT8195_VPP_MERGE0_P1_SEL_IN_FROM_MDP_RDMA1
> +	}, {
> +		DDP_COMPONENT_MDP_RDMA2, DDP_COMPONENT_MERGE2,
> +		MT8195_VDO1_VPP_MERGE1_P0_SEL_IN, GENMASK(0, 0),
> +		MT8195_VPP_MERGE1_P0_SEL_IN_FROM_MDP_RDMA2
> +	}, {
> +		DDP_COMPONENT_MERGE1, DDP_COMPONENT_ETHDR_MIXER,
> +		MT8195_VDO1_MERGE0_ASYNC_SOUT_SEL, GENMASK(1, 0),
> +		MT8195_SOUT_TO_MIXER_IN1_SEL
> +	}, {
> +		DDP_COMPONENT_MERGE2, DDP_COMPONENT_ETHDR_MIXER,
> +		MT8195_VDO1_MERGE1_ASYNC_SOUT_SEL, GENMASK(1, 0),
> +		MT8195_SOUT_TO_MIXER_IN2_SEL
> +	}, {
> +		DDP_COMPONENT_MERGE3, DDP_COMPONENT_ETHDR_MIXER,
> +		MT8195_VDO1_MERGE2_ASYNC_SOUT_SEL, GENMASK(1, 0),
> +		MT8195_SOUT_TO_MIXER_IN3_SEL
> +	}, {
> +		DDP_COMPONENT_MERGE4, DDP_COMPONENT_ETHDR_MIXER,
> +		MT8195_VDO1_MERGE3_ASYNC_SOUT_SEL, GENMASK(1, 0),
> +		MT8195_SOUT_TO_MIXER_IN4_SEL
> +	}, {
> +		DDP_COMPONENT_ETHDR_MIXER, DDP_COMPONENT_MERGE5,
> +		MT8195_VDO1_MIXER_OUT_SOUT_SEL, GENMASK(0, 0),
> +		MT8195_MIXER_SOUT_TO_MERGE4_ASYNC_SEL
> +	}, {
> +		DDP_COMPONENT_MERGE1, DDP_COMPONENT_ETHDR_MIXER,
> +		MT8195_VDO1_MIXER_IN1_SEL_IN, GENMASK(0, 0),
> +		MT8195_MIXER_IN1_SEL_IN_FROM_MERGE0_ASYNC_SOUT
> +	}, {
> +		DDP_COMPONENT_MERGE2, DDP_COMPONENT_ETHDR_MIXER,
> +		MT8195_VDO1_MIXER_IN2_SEL_IN, GENMASK(0, 0),
> +		MT8195_MIXER_IN2_SEL_IN_FROM_MERGE1_ASYNC_SOUT
> +	}, {
> +		DDP_COMPONENT_MERGE3, DDP_COMPONENT_ETHDR_MIXER,
> +		MT8195_VDO1_MIXER_IN3_SEL_IN, GENMASK(0, 0),
> +		MT8195_MIXER_IN3_SEL_IN_FROM_MERGE2_ASYNC_SOUT
> +	}, {
> +		DDP_COMPONENT_MERGE4, DDP_COMPONENT_ETHDR_MIXER,
> +		MT8195_VDO1_MIXER_IN4_SEL_IN, GENMASK(0, 0),
> +		MT8195_MIXER_IN4_SEL_IN_FROM_MERGE3_ASYNC_SOUT
> +	}, {
> +		DDP_COMPONENT_ETHDR_MIXER, DDP_COMPONENT_MERGE5,
> +		MT8195_VDO1_MIXER_SOUT_SEL_IN, GENMASK(2, 0),
> +		MT8195_MIXER_SOUT_SEL_IN_FROM_DISP_MIXER
> +	}, {
> +		DDP_COMPONENT_ETHDR_MIXER, DDP_COMPONENT_MERGE5,
> +		MT8195_VDO1_MERGE4_ASYNC_SEL_IN, GENMASK(2, 0),
> +		MT8195_MERGE4_ASYNC_SEL_IN_FROM_MIXER_OUT_SOUT
> +	}, {
> +		DDP_COMPONENT_MERGE5, DDP_COMPONENT_DPI1,
> +		MT8195_VDO1_DISP_DPI1_SEL_IN, GENMASK(1, 0),
> +		MT8195_DISP_DPI1_SEL_IN_FROM_VPP_MERGE4_MOUT
> +	}, {
> +		DDP_COMPONENT_MERGE5, DDP_COMPONENT_DPI1,
> +		MT8195_VDO1_MERGE4_SOUT_SEL, GENMASK(1, 0),
> +		MT8195_MERGE4_SOUT_TO_DPI1_SEL
> +	}, {
> +		DDP_COMPONENT_MERGE5, DDP_COMPONENT_DP_INTF1,
> +		MT8195_VDO1_DISP_DP_INTF0_SEL_IN, GENMASK(1, 0),
> +		MT8195_DISP_DP_INTF0_SEL_IN_FROM_VPP_MERGE4_MOUT
> +	}, {
> +		DDP_COMPONENT_MERGE5, DDP_COMPONENT_DP_INTF1,
> +		MT8195_VDO1_MERGE4_SOUT_SEL, GENMASK(1, 0),
> +		MT8195_MERGE4_SOUT_TO_DP_INTF0_SEL
> +	}
> +};
>   #endif /* __SOC_MEDIATEK_MT8195_MMSYS_H */
> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> index 146a78ba06c1..9a327eb5d9d7 100644
> --- a/drivers/soc/mediatek/mtk-mmsys.c
> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> @@ -80,6 +80,12 @@ static const struct mtk_mmsys_driver_data mt8195_vdosys0_driver_data = {
>   	.num_routes = ARRAY_SIZE(mmsys_mt8195_routing_table),
>   };
>   
> +static const struct mtk_mmsys_driver_data mt8195_vdosys1_driver_data = {
> +	.clk_driver = "clk-mt8195-vdo1",
> +	.routes = mmsys_mt8195_vdo1_routing_table,
> +	.num_routes = ARRAY_SIZE(mmsys_mt8195_vdo1_routing_table),
> +};
> +
>   static const struct mtk_mmsys_driver_data mt8365_mmsys_driver_data = {
>   	.clk_driver = "clk-mt8365-mm",
>   	.routes = mt8365_mmsys_routing_table,
> @@ -292,6 +298,10 @@ static const struct of_device_id of_match_mtk_mmsys[] = {
>   		.compatible = "mediatek,mt8195-vdosys0",
>   		.data = &mt8195_vdosys0_driver_data,

It seems we are missing a patch in the series. vdosys0 also correct was never 
introduced in the driver...

>   	},
> +	{
> +		.compatible = "mediatek,mt8195-vdosys1",
> +		.data = &mt8195_vdosys1_driver_data,
> +	},
>   	{
>   		.compatible = "mediatek,mt8365-mmsys",
>   		.data = &mt8365_mmsys_driver_data,

Re: [PATCH v28 04/11] soc: mediatek: add mtk-mmsys support for mt8195 vdosys1

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

On Tue, Nov 08, 2022 at 06:46:54PM +0100, Matthias Brugger wrote:
> On 07/11/2022 08:22, Nancy.Lin wrote:
[..]
> > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > @@ -80,6 +80,12 @@ static const struct mtk_mmsys_driver_data mt8195_vdosys0_driver_data = {
> >   	.num_routes = ARRAY_SIZE(mmsys_mt8195_routing_table),
> >   };
> > +static const struct mtk_mmsys_driver_data mt8195_vdosys1_driver_data = {
> > +	.clk_driver = "clk-mt8195-vdo1",
> > +	.routes = mmsys_mt8195_vdo1_routing_table,
> > +	.num_routes = ARRAY_SIZE(mmsys_mt8195_vdo1_routing_table),
> > +};
> > +
> >   static const struct mtk_mmsys_driver_data mt8365_mmsys_driver_data = {
> >   	.clk_driver = "clk-mt8365-mm",
> >   	.routes = mt8365_mmsys_routing_table,
> > @@ -292,6 +298,10 @@ static const struct of_device_id of_match_mtk_mmsys[] = {
> >   		.compatible = "mediatek,mt8195-vdosys0",
> >   		.data = &mt8195_vdosys0_driver_data,
> 
> It seems we are missing a patch in the series. vdosys0 also correct was
> never introduced in the driver...

Hi Matthias,

as mentioned in the cover letter, this series is based on the series "Change
mmsys compatible for mt8195 mediatek-drm" [1], which introduces vdosys0. This
compatible entry specifically is added on patch 3 of that series [2].

[1] https://lore.kernel.org/all/20220927152704.12018-1-jason-jh.lin@mediatek.com/
[2] https://lore.kernel.org/all/20220927152704.12018-4-jason-jh.lin@mediatek.com/

Thanks,
Nícolas

> 
> >   	},
> > +	{
> > +		.compatible = "mediatek,mt8195-vdosys1",
> > +		.data = &mt8195_vdosys1_driver_data,
> > +	},
> >   	{
> >   		.compatible = "mediatek,mt8365-mmsys",
> >   		.data = &mt8365_mmsys_driver_data,

Re: [PATCH v28 04/11] soc: mediatek: add mtk-mmsys support for mt8195 vdosys1

[Matthias Brugger]

On 08/11/2022 20:10, Nícolas F. R. A. Prado wrote:
> On Tue, Nov 08, 2022 at 06:46:54PM +0100, Matthias Brugger wrote:
>> On 07/11/2022 08:22, Nancy.Lin wrote:
> [..]
>>> --- a/drivers/soc/mediatek/mtk-mmsys.c
>>> +++ b/drivers/soc/mediatek/mtk-mmsys.c
>>> @@ -80,6 +80,12 @@ static const struct mtk_mmsys_driver_data mt8195_vdosys0_driver_data = {
>>>    	.num_routes = ARRAY_SIZE(mmsys_mt8195_routing_table),
>>>    };
>>> +static const struct mtk_mmsys_driver_data mt8195_vdosys1_driver_data = {
>>> +	.clk_driver = "clk-mt8195-vdo1",
>>> +	.routes = mmsys_mt8195_vdo1_routing_table,
>>> +	.num_routes = ARRAY_SIZE(mmsys_mt8195_vdo1_routing_table),
>>> +};
>>> +
>>>    static const struct mtk_mmsys_driver_data mt8365_mmsys_driver_data = {
>>>    	.clk_driver = "clk-mt8365-mm",
>>>    	.routes = mt8365_mmsys_routing_table,
>>> @@ -292,6 +298,10 @@ static const struct of_device_id of_match_mtk_mmsys[] = {
>>>    		.compatible = "mediatek,mt8195-vdosys0",
>>>    		.data = &mt8195_vdosys0_driver_data,
>>
>> It seems we are missing a patch in the series. vdosys0 also correct was
>> never introduced in the driver...
> 
> Hi Matthias,
> 
> as mentioned in the cover letter, this series is based on the series "Change
> mmsys compatible for mt8195 mediatek-drm" [1], which introduces vdosys0. This
> compatible entry specifically is added on patch 3 of that series [2].
> 
> [1] https://lore.kernel.org/all/20220927152704.12018-1-jason-jh.lin@mediatek.com/

My bad. Thanks for the link. I realized that yesterday but had to leave 
urgently. I'll have a look on this series now.

Regards,
Matthias

> [2] https://lore.kernel.org/all/20220927152704.12018-4-jason-jh.lin@mediatek.com/
> 
> Thanks,
> Nícolas
> 
>>
>>>    	},
>>> +	{
>>> +		.compatible = "mediatek,mt8195-vdosys1",
>>> +		.data = &mt8195_vdosys1_driver_data,
>>> +	},
>>>    	{
>>>    		.compatible = "mediatek,mt8365-mmsys",
>>>    		.data = &mt8365_mmsys_driver_data,

Re: [PATCH v28 01/11] dt-bindings: arm: mediatek: mmsys: add vdosys1 compatible for MT8195

[Matthias Brugger]

On 07/11/2022 08:22, Nancy.Lin wrote:
> Add vdosys1 mmsys compatible for MT8195 platform.
> 
> For MT8195, VDOSYS0 and VDOSYS1 are 2 display HW pipelines binding to
> 2 different power domains, different clock drivers and different
> mediatek-drm drivers.
> 
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>   .../devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml      | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> index 0711f1834fbd..aaabe2196185 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> @@ -48,7 +48,9 @@ properties:
>             - const: syscon
>   
>         - items:
> -          - const: mediatek,mt8195-vdosys0
> +          - enum:
> +              - mediatek,mt8195-vdosys0
> +              - mediatek,mt8195-vdosys1
>             - const: mediatek,mt8195-mmsys
>             - const: syscon
>   

I think we had that several times already:
https://lore.kernel.org/all/6bbe9527-ae48-30e0-fb45-519223a744d7@linaro.org/

We will something like this, but please check that this does not give any 
errors/warnings:

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml 
b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
index eb451bec23d3d..8e9c4f4d7c389 100644
--- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
@@ -32,13 +32,22 @@ properties:
                - mediatek,mt8183-mmsys
                - mediatek,mt8186-mmsys
                - mediatek,mt8192-mmsys
-              - mediatek,mt8195-mmsys
                - mediatek,mt8365-mmsys
            - const: syscon
        - items:
            - const: mediatek,mt7623-mmsys
            - const: mediatek,mt2701-mmsys
            - const: syscon
+      - items:
+          - const: mediatek,mt8195-vdosys0
+          - const: syscon
+      - items:
+          - const: mediatek,mt8195-vdosys1
+          - const: syscon
+      - items:
+          - const: mediatek,mt8195-mmsys
+          - const: syscon
+      deprecated: true

    reg:
      maxItems: 1

Re: [PATCH v28 01/11] dt-bindings: arm: mediatek: mmsys: add vdosys1 compatible for MT8195

[Jason-JH Lin (林睿祥)]

On Tue, 2022-11-08 at 18:46 +0100, Matthias Brugger wrote:
> 
> On 07/11/2022 08:22, Nancy.Lin wrote:
> > Add vdosys1 mmsys compatible for MT8195 platform.
> > 
> > For MT8195, VDOSYS0 and VDOSYS1 are 2 display HW pipelines binding
> > to
> > 2 different power domains, different clock drivers and different
> > mediatek-drm drivers.
> > 
> > Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > ---
> >   .../devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml      | 4
> > +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yam
> > l
> > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yam
> > l
> > index 0711f1834fbd..aaabe2196185 100644
> > ---
> > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yam
> > l
> > +++
> > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yam
> > l
> > @@ -48,7 +48,9 @@ properties:
> >             - const: syscon
> >   
> >         - items:
> > -          - const: mediatek,mt8195-vdosys0
> > +          - enum:
> > +              - mediatek,mt8195-vdosys0
> > +              - mediatek,mt8195-vdosys1
> >             - const: mediatek,mt8195-mmsys
> >             - const: syscon
> >   
> 
> I think we had that several times already:
> 
https://lore.kernel.org/all/6bbe9527-ae48-30e0-fb45-519223a744d7@linaro.org/
> 
> We will something like this, but please check that this does not give
> any 
> errors/warnings:
> 
> diff --git
> a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml 
> b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> index eb451bec23d3d..8e9c4f4d7c389 100644
> ---
> a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> +++
> b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> @@ -32,13 +32,22 @@ properties:
>                 - mediatek,mt8183-mmsys
>                 - mediatek,mt8186-mmsys
>                 - mediatek,mt8192-mmsys
> -              - mediatek,mt8195-mmsys
>                 - mediatek,mt8365-mmsys
>             - const: syscon
>         - items:
>             - const: mediatek,mt7623-mmsys
>             - const: mediatek,mt2701-mmsys
>             - const: syscon
> +      - items:
> +          - const: mediatek,mt8195-vdosys0
> +          - const: syscon
> +      - items:
> +          - const: mediatek,mt8195-vdosys1
> +          - const: syscon
> +      - items:
> +          - const: mediatek,mt8195-mmsys
> +          - const: syscon
> +      deprecated: true
> 
>     reg:
>       maxItems: 1

Hi Matthias,

As the vdosys0 previous reviewed patch:

https://patchwork.kernel.org/project/linux-mediatek/patch/20220927152704.12018-2-jason-jh.lin@mediatek.com/
Should I modify the vdosys0 items format like your example?

Or should vdosys1 add items format like vdosys0's previous patch?
    - items:
        - const: mediatek,mt8195-vdosys1
        - const: mediatek,mt8195-mmsys
        - const: syscon

Regards,
Jason-JH.Lin

Re: [PATCH v28 01/11] dt-bindings: arm: mediatek: mmsys: add vdosys1 compatible for MT8195

[Matthias Brugger]

On 09/11/2022 06:10, Jason-JH Lin (林睿祥) wrote:
> On Tue, 2022-11-08 at 18:46 +0100, Matthias Brugger wrote:
>> 
>> On 07/11/2022 08:22, Nancy.Lin wrote:
>> > Add vdosys1 mmsys compatible for MT8195 platform.
>> > 
>> > For MT8195, VDOSYS0 and VDOSYS1 are 2 display HW pipelines binding
>> > to
>> > 2 different power domains, different clock drivers and different
>> > mediatek-drm drivers.
>> > 
>> > Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
>> > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>> > ---
>> >   .../devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml      | 4
>> > +++-
>> >   1 file changed, 3 insertions(+), 1 deletion(-)
>> > 
>> > diff --git
>> > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yam
>> > l
>> > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yam
>> > l
>> > index 0711f1834fbd..aaabe2196185 100644
>> > ---
>> > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yam
>> > l
>> > +++
>> > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yam
>> > l
>> > @@ -48,7 +48,9 @@ properties:
>> >             - const: syscon
>> >   
>> >         - items:
>> > -          - const: mediatek,mt8195-vdosys0
>> > +          - enum:
>> > +              - mediatek,mt8195-vdosys0
>> > +              - mediatek,mt8195-vdosys1
>> >             - const: mediatek,mt8195-mmsys
>> >             - const: syscon
>> >   
>> 
>> I think we had that several times already:
>> 
> https://lore.kernel.org/all/6bbe9527-ae48-30e0-fb45-519223a744d7@linaro.org/
>> 
>> We will something like this, but please check that this does not give
>> any 
>> errors/warnings:
>> 
>> diff --git
>> a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml 
>> b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
>> index eb451bec23d3d..8e9c4f4d7c389 100644
>> ---
>> a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
>> +++
>> b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
>> @@ -32,13 +32,22 @@ properties:
>>                 - mediatek,mt8183-mmsys
>>                 - mediatek,mt8186-mmsys
>>                 - mediatek,mt8192-mmsys
>> -              - mediatek,mt8195-mmsys
>>                 - mediatek,mt8365-mmsys
>>             - const: syscon
>>         - items:
>>             - const: mediatek,mt7623-mmsys
>>             - const: mediatek,mt2701-mmsys
>>             - const: syscon
>> +      - items:
>> +          - const: mediatek,mt8195-vdosys0
>> +          - const: syscon
>> +      - items:
>> +          - const: mediatek,mt8195-vdosys1
>> +          - const: syscon
>> +      - items:
>> +          - const: mediatek,mt8195-mmsys
>> +          - const: syscon
>> +      deprecated: true
>> 
>>     reg:
>>       maxItems: 1
> 
> Hi Matthias,
> 
> As the vdosys0 previous reviewed patch:
> 
> https://patchwork.kernel.org/project/linux-mediatek/patch/20220927152704.12018-2-jason-jh.lin@mediatek.com/
> Should I modify the vdosys0 items format like your example?
> 
> Or should vdosys1 add items format like vdosys0's previous patch?
>      - items:
>          - const: mediatek,mt8195-vdosys1
>          - const: mediatek,mt8195-mmsys
>          - const: syscon
> 

No, vdosys1 must not have mediatek,mt8195-mmsys fallback.

Regards,
Matthias

> Regards,
> Jason-JH.Lin
> 
> 
> 
> ************* MEDIATEK Confidentiality Notice
>   ********************
> The information contained in this e-mail message (including any
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be
> conveyed only to the designated recipient(s). Any use, dissemination,
> distribution, printing, retaining or copying of this e-mail (including its
> attachments) by unintended recipient(s) is strictly prohibited and may
> be unlawful. If you are not an intended recipient of this e-mail, or believe
>   
> that you have received this e-mail in error, please notify the sender
> immediately (by replying to this e-mail), delete any and all copies of
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!

Re: [PATCH v28 01/11] dt-bindings: arm: mediatek: mmsys: add vdosys1 compatible for MT8195

[Nancy Lin (林欣螢)]

Dear Matthias,

Thanks for the review.

On Thu, 2022-11-10 at 14:10 +0100, Matthias Brugger wrote:
> 
> On 09/11/2022 06:10, Jason-JH Lin (林睿祥) wrote:
> > On Tue, 2022-11-08 at 18:46 +0100, Matthias Brugger wrote:
> > > 
> > > On 07/11/2022 08:22, Nancy.Lin wrote:
> > > > Add vdosys1 mmsys compatible for MT8195 platform.
> > > > 
> > > > For MT8195, VDOSYS0 and VDOSYS1 are 2 display HW pipelines
> > > > binding
> > > > to
> > > > 2 different power domains, different clock drivers and
> > > > different
> > > > mediatek-drm drivers.
> > > > 
> > > > Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> > > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > > ---
> > > >  
> > > > .../devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml      |
> > > > 4
> > > > +++-
> > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys
> > > > .yam
> > > > l
> > > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys
> > > > .yam
> > > > l
> > > > index 0711f1834fbd..aaabe2196185 100644
> > > > ---
> > > > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys
> > > > .yam
> > > > l
> > > > +++
> > > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys
> > > > .yam
> > > > l
> > > > @@ -48,7 +48,9 @@ properties:
> > > >             - const: syscon
> > > >   
> > > >         - items:
> > > > -          - const: mediatek,mt8195-vdosys0
> > > > +          - enum:
> > > > +              - mediatek,mt8195-vdosys0
> > > > +              - mediatek,mt8195-vdosys1
> > > >             - const: mediatek,mt8195-mmsys
> > > >             - const: syscon
> > > >   
> > > 
> > > I think we had that several times already:
> > > 
> > 
> > 
https://lore.kernel.org/all/6bbe9527-ae48-30e0-fb45-519223a744d7@linaro.org/
> > > 
> > > We will something like this, but please check that this does not
> > > give
> > > any 
> > > errors/warnings:
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.y
> > > aml 
> > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.y
> > > aml
> > > index eb451bec23d3d..8e9c4f4d7c389 100644
> > > ---
> > > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.y
> > > aml
> > > +++
> > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.y
> > > aml
> > > @@ -32,13 +32,22 @@ properties:
> > >                 - mediatek,mt8183-mmsys
> > >                 - mediatek,mt8186-mmsys
> > >                 - mediatek,mt8192-mmsys
> > > -              - mediatek,mt8195-mmsys
> > >                 - mediatek,mt8365-mmsys
> > >             - const: syscon
> > >         - items:
> > >             - const: mediatek,mt7623-mmsys
> > >             - const: mediatek,mt2701-mmsys
> > >             - const: syscon
> > > +      - items:
> > > +          - const: mediatek,mt8195-vdosys0
> > > +          - const: syscon
> > > +      - items:
> > > +          - const: mediatek,mt8195-vdosys1
> > > +          - const: syscon
> > > +      - items:
> > > +          - const: mediatek,mt8195-mmsys
> > > +          - const: syscon
> > > +      deprecated: true
> > > 
> > >     reg:
> > >       maxItems: 1
> > 
> > Hi Matthias,
> > 
> > As the vdosys0 previous reviewed patch:
> > 
> > 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20220927152704.12018-2-jason-jh.lin@mediatek.com/__;!!CTRNKA9wMg0ARbw!zRdbIyyAsfqob2kapMAcKYATAguhEV0x0qE5cTOAcWUNhzeAbMHzZoos_2QzUCxS$
> >  
> > Should I modify the vdosys0 items format like your example?
> > 
> > Or should vdosys1 add items format like vdosys0's previous patch?
> >      - items:
> >          - const: mediatek,mt8195-vdosys1
> >          - const: mediatek,mt8195-mmsys
> >          - const: syscon
> > 
> 
> No, vdosys1 must not have mediatek,mt8195-mmsys fallback.
> 
> Regards,
> Matthias
> 

I will fix it and add the following vdosys1 binding base on [1].

      - description: vdosys0 and vdosys1 are 2 display HW pipelines,
                     so mt8195 binding should be deprecated.
        deprecated: true
        items:
          - const: mediatek,mt8195-mmsys
          - const: syscon
      - items:
          - const: mediatek,mt7623-mmsys
          - const: mediatek,mt2701-mmsys
          - const: syscon
      - items:
          - const: mediatek,mt8195-vdosys0
          - const: mediatek,mt8195-mmsys
          - const: syscon
 +    - items:
 +        - const: mediatek,mt8195-vdosys1
 +        - const: syscon

[1] 
https://kernel.googlesource.com/pub/scm/linux/kernel/git/matthias.bgg/linux/+/b237efd47df7d25b78c306e90b97c5aa0ff4c4fc/Documentation/devicetree/bindings/arm/mediatek/mediatek%2Cmmsys.yaml

Regards,
Nancy


> > Regards,
> > Jason-JH.Lin
> > 
> > 
> > 
> > ************* MEDIATEK Confidentiality Notice
> >   ********************
> > The information contained in this e-mail message (including any
> > attachments) may be confidential, proprietary, privileged, or
> > otherwise
> > exempt from disclosure under applicable laws. It is intended to be
> > conveyed only to the designated recipient(s). Any use,
> > dissemination,
> > distribution, printing, retaining or copying of this e-mail
> > (including its
> > attachments) by unintended recipient(s) is strictly prohibited and
> > may
> > be unlawful. If you are not an intended recipient of this e-mail,
> > or believe
> >   
> > that you have received this e-mail in error, please notify the
> > sender
> > immediately (by replying to this e-mail), delete any and all copies
> > of
> > this e-mail (including any attachments) from your system, and do
> > not
> > disclose the content of this e-mail to any other person. Thank you!
> 
> 

Re: [PATCH v28 01/11] dt-bindings: arm: mediatek: mmsys: add vdosys1 compatible for MT8195

[Matthias Brugger]

On 22/11/2022 11:51, Nancy Lin (林欣螢) wrote:
> Dear Matthias,
> 
> Thanks for the review.
> 
> On Thu, 2022-11-10 at 14:10 +0100, Matthias Brugger wrote:
>> 
>> On 09/11/2022 06:10, Jason-JH Lin (林睿祥) wrote:
>> > On Tue, 2022-11-08 at 18:46 +0100, Matthias Brugger wrote:
>> > > 
>> > > On 07/11/2022 08:22, Nancy.Lin wrote:
>> > > > Add vdosys1 mmsys compatible for MT8195 platform.
>> > > > 
>> > > > For MT8195, VDOSYS0 and VDOSYS1 are 2 display HW pipelines
>> > > > binding
>> > > > to
>> > > > 2 different power domains, different clock drivers and
>> > > > different
>> > > > mediatek-drm drivers.
>> > > > 
>> > > > Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
>> > > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>> > > > ---
>> > > >  
>> > > > .../devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml      |
>> > > > 4
>> > > > +++-
>> > > >   1 file changed, 3 insertions(+), 1 deletion(-)
>> > > > 
>> > > > diff --git
>> > > > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys
>> > > > .yam
>> > > > l
>> > > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys
>> > > > .yam
>> > > > l
>> > > > index 0711f1834fbd..aaabe2196185 100644
>> > > > ---
>> > > > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys
>> > > > .yam
>> > > > l
>> > > > +++
>> > > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys
>> > > > .yam
>> > > > l
>> > > > @@ -48,7 +48,9 @@ properties:
>> > > >             - const: syscon
>> > > >   
>> > > >         - items:
>> > > > -          - const: mediatek,mt8195-vdosys0
>> > > > +          - enum:
>> > > > +              - mediatek,mt8195-vdosys0
>> > > > +              - mediatek,mt8195-vdosys1
>> > > >             - const: mediatek,mt8195-mmsys
>> > > >             - const: syscon
>> > > >   
>> > > 
>> > > I think we had that several times already:
>> > > 
>> > 
>> > 
> https://lore.kernel.org/all/6bbe9527-ae48-30e0-fb45-519223a744d7@linaro.org/
>> > > 
>> > > We will something like this, but please check that this does not
>> > > give
>> > > any 
>> > > errors/warnings:
>> > > 
>> > > diff --git
>> > > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.y
>> > > aml 
>> > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.y
>> > > aml
>> > > index eb451bec23d3d..8e9c4f4d7c389 100644
>> > > ---
>> > > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.y
>> > > aml
>> > > +++
>> > > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.y
>> > > aml
>> > > @@ -32,13 +32,22 @@ properties:
>> > >                 - mediatek,mt8183-mmsys
>> > >                 - mediatek,mt8186-mmsys
>> > >                 - mediatek,mt8192-mmsys
>> > > -              - mediatek,mt8195-mmsys
>> > >                 - mediatek,mt8365-mmsys
>> > >             - const: syscon
>> > >         - items:
>> > >             - const: mediatek,mt7623-mmsys
>> > >             - const: mediatek,mt2701-mmsys
>> > >             - const: syscon
>> > > +      - items:
>> > > +          - const: mediatek,mt8195-vdosys0
>> > > +          - const: syscon
>> > > +      - items:
>> > > +          - const: mediatek,mt8195-vdosys1
>> > > +          - const: syscon
>> > > +      - items:
>> > > +          - const: mediatek,mt8195-mmsys
>> > > +          - const: syscon
>> > > +      deprecated: true
>> > > 
>> > >     reg:
>> > >       maxItems: 1
>> > 
>> > Hi Matthias,
>> > 
>> > As the vdosys0 previous reviewed patch:
>> > 
>> > 
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20220927152704.12018-2-jason-jh.lin@mediatek.com/__;!!CTRNKA9wMg0ARbw!zRdbIyyAsfqob2kapMAcKYATAguhEV0x0qE5cTOAcWUNhzeAbMHzZoos_2QzUCxS$
>> >  
>> > Should I modify the vdosys0 items format like your example?
>> > 
>> > Or should vdosys1 add items format like vdosys0's previous patch?
>> >      - items:
>> >          - const: mediatek,mt8195-vdosys1
>> >          - const: mediatek,mt8195-mmsys
>> >          - const: syscon
>> > 
>> 
>> No, vdosys1 must not have mediatek,mt8195-mmsys fallback.
>> 
>> Regards,
>> Matthias
>> 
> 
> I will fix it and add the following vdosys1 binding base on [1].
> 
>        - description: vdosys0 and vdosys1 are 2 display HW pipelines,
>                       so mt8195 binding should be deprecated.
>          deprecated: true
>          items:
>            - const: mediatek,mt8195-mmsys
>            - const: syscon
>        - items:
>            - const: mediatek,mt7623-mmsys
>            - const: mediatek,mt2701-mmsys
>            - const: syscon
>        - items:
>            - const: mediatek,mt8195-vdosys0
>            - const: mediatek,mt8195-mmsys
>            - const: syscon
>   +    - items:
>   +        - const: mediatek,mt8195-vdosys1
>   +        - const: syscon
> 

Looks good, thanks
Matthias

> [1]
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/matthias.bgg/linux/+/b237efd47df7d25b78c306e90b97c5aa0ff4c4fc/Documentation/devicetree/bindings/arm/mediatek/mediatek%2Cmmsys.yaml
> 
> Regards,
> Nancy
> 
> 
>> > Regards,
>> > Jason-JH.Lin
>> > 
>> > 
>> > 
>> > ************* MEDIATEK Confidentiality Notice
>> >   ********************
>> > The information contained in this e-mail message (including any
>> > attachments) may be confidential, proprietary, privileged, or
>> > otherwise
>> > exempt from disclosure under applicable laws. It is intended to be
>> > conveyed only to the designated recipient(s). Any use,
>> > dissemination,
>> > distribution, printing, retaining or copying of this e-mail
>> > (including its
>> > attachments) by unintended recipient(s) is strictly prohibited and
>> > may
>> > be unlawful. If you are not an intended recipient of this e-mail,
>> > or believe
>> >   
>> > that you have received this e-mail in error, please notify the
>> > sender
>> > immediately (by replying to this e-mail), delete any and all copies
>> > of
>> > this e-mail (including any attachments) from your system, and do
>> > not
>> > disclose the content of this e-mail to any other person. Thank you!
>> 
>> 
> 
> ************* MEDIATEK Confidentiality Notice ********************
> The information contained in this e-mail message (including any
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be
> conveyed only to the designated recipient(s). Any use, dissemination,
> distribution, printing, retaining or copying of this e-mail (including its
> attachments) by unintended recipient(s) is strictly prohibited and may
> be unlawful. If you are not an intended recipient of this e-mail, or believe
> that you have received this e-mail in error, please notify the sender
> immediately (by replying to this e-mail), delete any and all copies of
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!

Re: [PATCH v28 01/11] dt-bindings: arm: mediatek: mmsys: add vdosys1 compatible for MT8195

[Krzysztof Kozlowski]

On 07/11/2022 08:22, Nancy.Lin wrote:
> Add vdosys1 mmsys compatible for MT8195 platform.
> 
> For MT8195, VDOSYS0 and VDOSYS1 are 2 display HW pipelines binding to
> 2 different power domains, different clock drivers and different
> mediatek-drm drivers.
> 
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Any reason for not CC-ing maintainers pointed out by get_maintainers.pl?

Best regards,
Krzysztof

Re: [PATCH v28 01/11] dt-bindings: arm: mediatek: mmsys: add vdosys1 compatible for MT8195

[Nancy Lin (林欣螢)]

Dear Krzysztof,

Thanks for the review.

On Wed, 2022-11-23 at 17:06 +0100, Krzysztof Kozlowski wrote:
> On 07/11/2022 08:22, Nancy.Lin wrote:
> > Add vdosys1 mmsys compatible for MT8195 platform.
> > 
> > For MT8195, VDOSYS0 and VDOSYS1 are 2 display HW pipelines binding
> > to
> > 2 different power domains, different clock drivers and different
> > mediatek-drm drivers.
> > 
> > Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> Any reason for not CC-ing maintainers pointed out by
> get_maintainers.pl?
> 
> Best regards,
> Krzysztof
> 

> I used the recorded maintainer list, instead of executing the
> maintainer.pl every time. I will update maintainer in the next
> revision soon. Sorry for the inconvinence.

Regards,
Nancy

Re: [PATCH v28 05/11] soc: mediatek: refine code to use mtk_mmsys_update_bits API

[Matthias Brugger]

On 07/11/2022 08:22, Nancy.Lin wrote:
> Simplify code for update  mmsys reg.
> 
> 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: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>   drivers/soc/mediatek/mtk-mmsys.c | 45 ++++++++++++--------------------
>   1 file changed, 16 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> index 9a327eb5d9d7..73c8bd27e6ae 100644
> --- a/drivers/soc/mediatek/mtk-mmsys.c
> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> @@ -99,22 +99,27 @@ struct mtk_mmsys {
>   	struct reset_controller_dev rcdev;
>   };
>   
> +static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 val)
> +{
> +	u32 tmp;
> +
> +	tmp = readl_relaxed(mmsys->regs + offset);
> +	tmp = (tmp & ~mask) | (val & mask);

I'm not sure about the change in the implementation of mtk_mmsys_update_bits(). 
Nicolas tried to explain it to me on IRC but I wasn't totally convincing. As we 
have to go for at least another round of this patches, I'd like to get a clear 
understanding while it is needed that val bits are set to 1 in the mask.

Regards,
Matthias

> +	writel_relaxed(tmp, mmsys->regs + offset);
> +}
> +
>   void mtk_mmsys_ddp_connect(struct device *dev,
>   			   enum mtk_ddp_comp_id cur,
>   			   enum mtk_ddp_comp_id next)
>   {
>   	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
>   	const struct mtk_mmsys_routes *routes = mmsys->data->routes;
> -	u32 reg;
>   	int i;
>   
>   	for (i = 0; i < mmsys->data->num_routes; i++)
> -		if (cur == routes[i].from_comp && next == routes[i].to_comp) {
> -			reg = readl_relaxed(mmsys->regs + routes[i].addr);
> -			reg &= ~routes[i].mask;
> -			reg |= routes[i].val;
> -			writel_relaxed(reg, mmsys->regs + routes[i].addr);
> -		}
> +		if (cur == routes[i].from_comp && next == routes[i].to_comp)
> +			mtk_mmsys_update_bits(mmsys, routes[i].addr, routes[i].mask,
> +					      routes[i].val);
>   }
>   EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_connect);
>   
> @@ -124,27 +129,14 @@ void mtk_mmsys_ddp_disconnect(struct device *dev,
>   {
>   	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
>   	const struct mtk_mmsys_routes *routes = mmsys->data->routes;
> -	u32 reg;
>   	int i;
>   
>   	for (i = 0; i < mmsys->data->num_routes; i++)
> -		if (cur == routes[i].from_comp && next == routes[i].to_comp) {
> -			reg = readl_relaxed(mmsys->regs + routes[i].addr);
> -			reg &= ~routes[i].mask;
> -			writel_relaxed(reg, mmsys->regs + routes[i].addr);
> -		}
> +		if (cur == routes[i].from_comp && next == routes[i].to_comp)
> +			mtk_mmsys_update_bits(mmsys, routes[i].addr, routes[i].mask, 0);
>   }
>   EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_disconnect);
>   
> -static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 val)
> -{
> -	u32 tmp;
> -
> -	tmp = readl_relaxed(mmsys->regs + offset);
> -	tmp = (tmp & ~mask) | val;
> -	writel_relaxed(tmp, mmsys->regs + offset);
> -}
> -
>   void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val)
>   {
>   	if (val)
> @@ -161,18 +153,13 @@ 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 reg;
>   
>   	spin_lock_irqsave(&mmsys->lock, flags);
>   
> -	reg = readl_relaxed(mmsys->regs + mmsys->data->sw0_rst_offset);
> -
>   	if (assert)
> -		reg &= ~BIT(id);
> +		mtk_mmsys_update_bits(mmsys, mmsys->data->sw0_rst_offset, BIT(id), 0);
>   	else
> -		reg |= BIT(id);
> -
> -	writel_relaxed(reg, mmsys->regs + mmsys->data->sw0_rst_offset);
> +		mtk_mmsys_update_bits(mmsys, mmsys->data->sw0_rst_offset, BIT(id), BIT(id));
>   
>   	spin_unlock_irqrestore(&mmsys->lock, flags);
>   

Re: [PATCH v28 05/11] soc: mediatek: refine code to use mtk_mmsys_update_bits API

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

On Tue, Nov 08, 2022 at 06:37:19PM +0100, Matthias Brugger wrote:
> 
> 
> On 07/11/2022 08:22, Nancy.Lin wrote:
> > Simplify code for update  mmsys reg.
> > 
> > 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: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > ---
> >   drivers/soc/mediatek/mtk-mmsys.c | 45 ++++++++++++--------------------
> >   1 file changed, 16 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> > index 9a327eb5d9d7..73c8bd27e6ae 100644
> > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > @@ -99,22 +99,27 @@ struct mtk_mmsys {
> >   	struct reset_controller_dev rcdev;
> >   };
> > +static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 val)
> > +{
> > +	u32 tmp;
> > +
> > +	tmp = readl_relaxed(mmsys->regs + offset);
> > +	tmp = (tmp & ~mask) | (val & mask);
> 
> I'm not sure about the change in the implementation of
> mtk_mmsys_update_bits(). Nicolas tried to explain it to me on IRC but I
> wasn't totally convincing. As we have to go for at least another round of
> this patches, I'd like to get a clear understanding while it is needed that
> val bits are set to 1 in the mask.

The point here was to make sure that mtk_mmsys_update_bits() didn't allow
setting bits outside of the mask, since that's never what you want: the entire
point of having a mask is to specify the bits that should be updated (and the
ones that should be kept unchanged). So for example if you had

mask = 0x0ff0
val  = 0x00ff

the previous implementation would happily overwrite the 4 least significant bits
on the destination register, despite them not being present in the mask, which
is wrong.

This wrong behavior could easily lead to hard to trace bugs as soon as a badly
formatted/wrong val is passed and an unrelated bit updated due to the mask being
ignored.

For reference, _regmap_update_bits() does the same masking of the value [1].

That said, given that this function already existed and was just being moved,
it would've been cleaner to make this change in a separate commit.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap.c#L3122

Thanks,
Nícolas

> 
> Regards,
> Matthias
> 
> > +	writel_relaxed(tmp, mmsys->regs + offset);
> > +}
[..]
> > -static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 val)
> > -{
> > -	u32 tmp;
> > -
> > -	tmp = readl_relaxed(mmsys->regs + offset);
> > -	tmp = (tmp & ~mask) | val;
> > -	writel_relaxed(tmp, mmsys->regs + offset);
> > -}
> > -
[..]

Re: [PATCH v28 05/11] soc: mediatek: refine code to use mtk_mmsys_update_bits API

[Matthias Brugger]

On 08/11/2022 20:43, Nícolas F. R. A. Prado wrote:
> On Tue, Nov 08, 2022 at 06:37:19PM +0100, Matthias Brugger wrote:
>>
>>
>> On 07/11/2022 08:22, Nancy.Lin wrote:
>>> Simplify code for update  mmsys reg.
>>>
>>> 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: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
>>> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>>> ---
>>>    drivers/soc/mediatek/mtk-mmsys.c | 45 ++++++++++++--------------------
>>>    1 file changed, 16 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
>>> index 9a327eb5d9d7..73c8bd27e6ae 100644
>>> --- a/drivers/soc/mediatek/mtk-mmsys.c
>>> +++ b/drivers/soc/mediatek/mtk-mmsys.c
>>> @@ -99,22 +99,27 @@ struct mtk_mmsys {
>>>    	struct reset_controller_dev rcdev;
>>>    };
>>> +static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 val)
>>> +{
>>> +	u32 tmp;
>>> +
>>> +	tmp = readl_relaxed(mmsys->regs + offset);
>>> +	tmp = (tmp & ~mask) | (val & mask);
>>
>> I'm not sure about the change in the implementation of
>> mtk_mmsys_update_bits(). Nicolas tried to explain it to me on IRC but I
>> wasn't totally convincing. As we have to go for at least another round of
>> this patches, I'd like to get a clear understanding while it is needed that
>> val bits are set to 1 in the mask.
> 
> The point here was to make sure that mtk_mmsys_update_bits() didn't allow
> setting bits outside of the mask, since that's never what you want: the entire
> point of having a mask is to specify the bits that should be updated (and the
> ones that should be kept unchanged). So for example if you had
> 
> mask = 0x0ff0
> val  = 0x00ff
> 
> the previous implementation would happily overwrite the 4 least significant bits
> on the destination register, despite them not being present in the mask, which
> is wrong.
> 
> This wrong behavior could easily lead to hard to trace bugs as soon as a badly
> formatted/wrong val is passed and an unrelated bit updated due to the mask being
> ignored.
> 
> For reference, _regmap_update_bits() does the same masking of the value [1].
> 
> That said, given that this function already existed and was just being moved,
> it would've been cleaner to make this change in a separate commit.
> 

Would have been better, but we can leave it as it.

Regards,
Matthias

> [1] https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap.c#L3122
> 
> Thanks,
> Nícolas
> 
>>
>> Regards,
>> Matthias
>>
>>> +	writel_relaxed(tmp, mmsys->regs + offset);
>>> +}
> [..]
>>> -static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 val)
>>> -{
>>> -	u32 tmp;
>>> -
>>> -	tmp = readl_relaxed(mmsys->regs + offset);
>>> -	tmp = (tmp & ~mask) | val;
>>> -	writel_relaxed(tmp, mmsys->regs + offset);
>>> -}
>>> -
> [..]

Re: [PATCH v28 05/11] soc: mediatek: refine code to use mtk_mmsys_update_bits API

[Nancy Lin (林欣螢)]

Dear Matthias,

Thanks for the review.

On Thu, 2022-11-10 at 14:12 +0100, Matthias Brugger wrote:
> 
> On 08/11/2022 20:43, Nícolas F. R. A. Prado wrote:
> > On Tue, Nov 08, 2022 at 06:37:19PM +0100, Matthias Brugger wrote:
> > > 
> > > 
> > > On 07/11/2022 08:22, Nancy.Lin wrote:
> > > > Simplify code for update  mmsys reg.
> > > > 
> > > > 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: AngeloGioacchino Del Regno <
> > > > angelogioacchino.delregno@collabora.com>
> > > > Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> > > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > > ---
> > > >    drivers/soc/mediatek/mtk-mmsys.c | 45 ++++++++++++--------
> > > > ------------
> > > >    1 file changed, 16 insertions(+), 29 deletions(-)
> > > > 
> > > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > > > b/drivers/soc/mediatek/mtk-mmsys.c
> > > > index 9a327eb5d9d7..73c8bd27e6ae 100644
> > > > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > > > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > > > @@ -99,22 +99,27 @@ struct mtk_mmsys {
> > > >    	struct reset_controller_dev rcdev;
> > > >    };
> > > > +static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32
> > > > offset, u32 mask, u32 val)
> > > > +{
> > > > +	u32 tmp;
> > > > +
> > > > +	tmp = readl_relaxed(mmsys->regs + offset);
> > > > +	tmp = (tmp & ~mask) | (val & mask);
> > > 
> > > I'm not sure about the change in the implementation of
> > > mtk_mmsys_update_bits(). Nicolas tried to explain it to me on IRC
> > > but I
> > > wasn't totally convincing. As we have to go for at least another
> > > round of
> > > this patches, I'd like to get a clear understanding while it is
> > > needed that
> > > val bits are set to 1 in the mask.
> > 
> > The point here was to make sure that mtk_mmsys_update_bits() didn't
> > allow
> > setting bits outside of the mask, since that's never what you want:
> > the entire
> > point of having a mask is to specify the bits that should be
> > updated (and the
> > ones that should be kept unchanged). So for example if you had
> > 
> > mask = 0x0ff0
> > val  = 0x00ff
> > 
> > the previous implementation would happily overwrite the 4 least
> > significant bits
> > on the destination register, despite them not being present in the
> > mask, which
> > is wrong.
> > 
> > This wrong behavior could easily lead to hard to trace bugs as soon
> > as a badly
> > formatted/wrong val is passed and an unrelated bit updated due to
> > the mask being
> > ignored.
> > 
> > For reference, _regmap_update_bits() does the same masking of the
> > value [1].
> > 
> > That said, given that this function already existed and was just
> > being moved,
> > it would've been cleaner to make this change in a separate commit.
> > 
> 
> Would have been better, but we can leave it as it.
> 
> Regards,
> Matthias
> 

Do you mean to keep original one (1), or keep this (2) ?

  1. tmp = (tmp & ~mask) | val; 
  2. tmp = (tmp & ~mask) | (val & mask);


Regards,
Nancy

> > [1] 
> > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap.c*L3122__;Iw!!CTRNKA9wMg0ARbw!xSv5xbY6cv-Mg-1xDGOf3oVZ93uyrcv4tt87DKlx5emjmwufjf2DjION7GiNAaJB$
> >  
> > 
> > Thanks,
> > Nícolas
> > 
> > > 
> > > Regards,
> > > Matthias
> > > 
> > > > +	writel_relaxed(tmp, mmsys->regs + offset);
> > > > +}
> > 
> > [..]
> > > > -static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32
> > > > offset, u32 mask, u32 val)
> > > > -{
> > > > -	u32 tmp;
> > > > -
> > > > -	tmp = readl_relaxed(mmsys->regs + offset);
> > > > -	tmp = (tmp & ~mask) | val;
> > > > -	writel_relaxed(tmp, mmsys->regs + offset);
> > > > -}
> > > > -
> > 
> > [..]
> 
> 

Re: [PATCH v28 05/11] soc: mediatek: refine code to use mtk_mmsys_update_bits API

[Chen-Yu Tsai]

On Mon, Nov 7, 2022 at 3:23 PM Nancy.Lin <nancy.lin@mediatek.com> wrote:
>
> Simplify code for update  mmsys reg.
>
> 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: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>  drivers/soc/mediatek/mtk-mmsys.c | 45 ++++++++++++--------------------
>  1 file changed, 16 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> index 9a327eb5d9d7..73c8bd27e6ae 100644
> --- a/drivers/soc/mediatek/mtk-mmsys.c
> +++ b/drivers/soc/mediatek/mtk-mmsys.c

[...]

> @@ -124,27 +129,14 @@ void mtk_mmsys_ddp_disconnect(struct device *dev,
>  {
>         struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
>         const struct mtk_mmsys_routes *routes = mmsys->data->routes;
> -       u32 reg;
>         int i;
>
>         for (i = 0; i < mmsys->data->num_routes; i++)
> -               if (cur == routes[i].from_comp && next == routes[i].to_comp) {
> -                       reg = readl_relaxed(mmsys->regs + routes[i].addr);
> -                       reg &= ~routes[i].mask;
> -                       writel_relaxed(reg, mmsys->regs + routes[i].addr);
> -               }
> +               if (cur == routes[i].from_comp && next == routes[i].to_comp)
> +                       mtk_mmsys_update_bits(mmsys, routes[i].addr, routes[i].mask, 0);
>  }
>  EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_disconnect);
>
> -static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 val)
> -{
> -       u32 tmp;
> -
> -       tmp = readl_relaxed(mmsys->regs + offset);
> -       tmp = (tmp & ~mask) | val;
> -       writel_relaxed(tmp, mmsys->regs + offset);
> -}
> -
>  void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val)
>  {
>         if (val)

This hunk now doesn't apply due to

    soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config func

touching mtk_mmsys_ddp_dpi_fmt_config() as well. It's trivial to resolve
though.

ChenYu

Re: [PATCH v28 05/11] soc: mediatek: refine code to use mtk_mmsys_update_bits API

[Nancy Lin (林欣螢)]

Dear Chen-Yu,

Sorry for late response and thanks for the review.

On Thu, 2022-12-01 at 19:44 +0800, Chen-Yu Tsai wrote:
> On Mon, Nov 7, 2022 at 3:23 PM Nancy.Lin <nancy.lin@mediatek.com>
> wrote:
> > 
> > Simplify code for update  mmsys reg.
> > 
> > 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: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > ---
> >  drivers/soc/mediatek/mtk-mmsys.c | 45 ++++++++++++--------------
> > ------
> >  1 file changed, 16 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > b/drivers/soc/mediatek/mtk-mmsys.c
> > index 9a327eb5d9d7..73c8bd27e6ae 100644
> > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> 
> [...]
> 
> > @@ -124,27 +129,14 @@ void mtk_mmsys_ddp_disconnect(struct device
> > *dev,
> >  {
> >         struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> >         const struct mtk_mmsys_routes *routes = mmsys->data-
> > >routes;
> > -       u32 reg;
> >         int i;
> > 
> >         for (i = 0; i < mmsys->data->num_routes; i++)
> > -               if (cur == routes[i].from_comp && next ==
> > routes[i].to_comp) {
> > -                       reg = readl_relaxed(mmsys->regs +
> > routes[i].addr);
> > -                       reg &= ~routes[i].mask;
> > -                       writel_relaxed(reg, mmsys->regs +
> > routes[i].addr);
> > -               }
> > +               if (cur == routes[i].from_comp && next ==
> > routes[i].to_comp)
> > +                       mtk_mmsys_update_bits(mmsys,
> > routes[i].addr, routes[i].mask, 0);
> >  }
> >  EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_disconnect);
> > 
> > -static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32
> > offset, u32 mask, u32 val)
> > -{
> > -       u32 tmp;
> > -
> > -       tmp = readl_relaxed(mmsys->regs + offset);
> > -       tmp = (tmp & ~mask) | val;
> > -       writel_relaxed(tmp, mmsys->regs + offset);
> > -}
> > -
> >  void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val)
> >  {
> >         if (val)
> 
> This hunk now doesn't apply due to
> 
>     soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config
> func
> 
> touching mtk_mmsys_ddp_dpi_fmt_config() as well. It's trivial to
> resolve
> though.
> 
> ChenYu

I will update next revision base on next-20221226 which include the
patch you mentioned.

Regards,
Nancy