[PATCH v7,1/3] soc: mediatek: Add mmsys func to adapt to dpi output for MT8186

[xinlei.lee]

On Thu, 2022-09-22 at 10:16 +0200, AngeloGioacchino Del Regno wrote:
> Il 22/09/22 09:29, xinlei.lee at mediatek.com ha scritto:
> > From: Xinlei Lee <xinlei.lee at mediatek.com>
> > 
> > The difference between MT8186 and other ICs is that when modifying
> > the
> > output format, we need to modify the mmsys_base+0x400 register to
> > take
> > effect.
> > So when setting the dpi output format, we need to call mmsys_func
> > to set
> > it to MT8186 synchronously.
> > 
> > Co-developed-by: Jitao Shi <jitao.shi at mediatek.com>
> > Signed-off-by: Jitao Shi <jitao.shi at mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee at mediatek.com>
> > ---
> >   drivers/soc/mediatek/mt8186-mmsys.h    |  8 +++++++
> >   drivers/soc/mediatek/mtk-mmsys.c       | 32
> > ++++++++++++++++++++++++++
> >   include/linux/soc/mediatek/mtk-mmsys.h |  9 ++++++++
> >   3 files changed, 49 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mt8186-mmsys.h
> > b/drivers/soc/mediatek/mt8186-mmsys.h
> > index eb1ad9c37a9c..536005d1cc55 100644
> > --- a/drivers/soc/mediatek/mt8186-mmsys.h
> > +++ b/drivers/soc/mediatek/mt8186-mmsys.h
> > @@ -3,6 +3,14 @@
> >   #ifndef __SOC_MEDIATEK_MT8186_MMSYS_H
> >   #define __SOC_MEDIATEK_MT8186_MMSYS_H
> >   
> > +/* Values for DPI configuration in MMSYS address space */
> > +#define MT8186_MMSYS_DPI_OUTPUT_FORMAT		0x400
> > +#define DPI_FORMAT_MASK					0x3
> 
> This is GENMASK(1, 0)
> 
> > +#define DPI_RGB888_SDR_CON				0
> > +#define DPI_RGB888_DDR_CON				1
> > +#define DPI_RGB565_SDR_CON				2
> > +#define DPI_RGB565_DDR_CON				3
> > +
> >   #define MT8186_MMSYS_OVL_CON			0xF04
> >   #define MT8186_MMSYS_OVL0_CON_MASK			0x3
> >   #define MT8186_MMSYS_OVL0_2L_CON_MASK			0xC
> > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > b/drivers/soc/mediatek/mtk-mmsys.c
> > index 06d8e83a2cb5..0857806206dc 100644
> > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > @@ -227,6 +227,38 @@ void mtk_mmsys_ddp_disconnect(struct device
> > *dev,
> >   }
> >   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)
> > +{
> > +	switch (val) {
> 
> You're not handling the MTK_DPI_RGB888_SDR_CON case.
> 
> > +	case MTK_DPI_RGB888_DDR_CON: > +		mtk_mmsys_update_bi
> > ts(dev_get_drvdata(dev), MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> 
> Are there any other (future, past, present) MTK SoCs having a MMSYS
> DPI_OUTPUT_FORMAT register?
> 
> I don't like seeing this kind of model-agnostic function in a not
> model-agnostic
> driver, especially when this is only because of a register address.
> That would change if no other future (or present) MediaTek SoCs have
> this register.
> 
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB888_DDR_CON);
> > +		break;
> > +	case MTK_DPI_RGB565_SDR_CON:
> > +		mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB565_SDR_CON);
> > +		break;
> > +	case MTK_DPI_RGB565_DDR_CON:
> > +		mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB565_DDR_CON);
> > +		break;
> 
> This goes here...
> 
> 	case MTK_DPI_RGB888_DDR_CON:
> > +	default:
> > +		mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB888_DDR_CON);
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_dpi_fmt_config);
> > +
> >   static int mtk_mmsys_reset_update(struct reset_controller_dev
> > *rcdev, unsigned long id,
> >   				  bool assert)
> >   {
> > diff --git a/include/linux/soc/mediatek/mtk-mmsys.h
> > b/include/linux/soc/mediatek/mtk-mmsys.h
> > index 59117d970daf..b85f66db33e1 100644
> > --- a/include/linux/soc/mediatek/mtk-mmsys.h
> > +++ b/include/linux/soc/mediatek/mtk-mmsys.h
> > @@ -9,6 +9,13 @@
> >   enum mtk_ddp_comp_id;
> >   struct device;
> >   
> > +enum mtk_dpi_out_format_con {
> > +	MTK_DPI_RGB888_SDR_CON,
> > +	MTK_DPI_RGB888_DDR_CON,
> > +	MTK_DPI_RGB565_SDR_CON,
> > +	MTK_DPI_RGB565_DDR_CON
> > +};
> > +
> >   enum mtk_ddp_comp_id {
> >   	DDP_COMPONENT_AAL0,
> >   	DDP_COMPONENT_AAL1,
> > @@ -65,4 +72,6 @@ void mtk_mmsys_ddp_disconnect(struct device *dev,
> >   			      enum mtk_ddp_comp_id cur,
> >   			      enum mtk_ddp_comp_id next);
> >   
> > +void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val);
> > +
> >   #endif /* __MTK_MMSYS_H */
> 
> 

Hi Angelo:

Thanks for your review!

1. I will modify the macro definition to use GENMASK(1, 0).
2. At present, only mt8186 needs to modify the register of mmsys
synchronously when changing the output format. 
   Other ICs do not need it, because the MT8186 DPI has been modified
on the hardware.
   And the software needs to cooperate with the modification of the
hardware before it is used. Reserved register for mmsys_base+0x400.
3. will add all of the settings to mtk_mmsys_ddp_dpi_fmt_config.

Best Regards!
xinlei