Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[Bibby Hsieh]

Hi, CK,

Thanks for your comments.

On Mon, 2016-07-25 at 14:49 +0800, CK Hu wrote:
> Hi, Bibby:
> 
> On Mon, 2016-07-25 at 14:24 +0800, Bibby Hsieh wrote:
> > Hi, CK,
> > 
> > Thanks for your comments.
> > 
> > On Wed, 2016-07-20 at 15:57 +0800, CK Hu wrote:
> > > Hi, Bibby:
> > > 
> > > Some comments inline.
> > > 
> > > On Wed, 2016-07-20 at 12:03 +0800, Bibby Hsieh wrote:
> > > > From: Junzhi Zhao 
> > > > 
> > > > Pixel clock should be 297MHz when resolution is 4K.
> > > > 
> > > > Signed-off-by: Junzhi Zhao 
> > > > Signed-off-by: Bibby Hsieh 
> > > > ---
> > > >  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> > > > +---
> > > >  1 file changed, 131 insertions(+), 53 deletions(-)
> > > > 
> 
> [snip...]
> 
> > > >  
> > > > +static int mt8173_parse_clk_from_dt(struct mtk_dpi *dpi, struct 
> > > > device_node *np)
> > > > +{
> > > > +   int i;
> > > > +
> > > > +   for (i = 0; i < ARRAY_SIZE(mtk_dpi_clk_names); i++) {
> > > > +   dpi->clk[i] = of_clk_get_by_name(np,
> > > > + mtk_dpi_clk_names[i]);
> > > > +   if (IS_ERR(dpi->clk[i]))
> > > > +   return PTR_ERR(dpi->clk[i]);
> > > > +   }
> > > > +   return 0;
> > > > +}
> > > 
> > > I think parsing device tree is a pure SW behavior. Would this vary for
> > > different MTK soc?
> > > 
> > Yes
> 
> I can not imaging that, so could you give me an example source code of
> other MTK soc for parse_clk_from_dt()?
> 
I will do some changes according to your comments. 

> > > > +
> > > > +
> > > > +static const struct mtk_dpi_conf mt8173_conf = {
> > > > +   .parse_clk_from_dt = mt8173_parse_clk_from_dt,
> > > > +   .clk_config = mt8173_clk_config,
> > > > +};
> > > > +
> > > > +static const struct of_device_id mtk_dpi_of_ids[] = {
> > > > +   { .compatible = "mediatek,mt8173-dpi",
> > > > +   .data = _conf,
> > > > +   },
> > > > +   {}
> > > > +};
> > > > +
> > > >  static int mtk_dpi_probe(struct platform_device *pdev)
> > > >  {
> > > > struct device *dev = >dev;
> > > > struct mtk_dpi *dpi;
> > > > struct resource *mem;
> > > > +   struct device_node *np = dev->of_node;
> > > > struct device_node *ep, *bridge_node = NULL;
> > > > int comp_id;
> > > > +   const struct of_device_id *match;
> > > > +   struct mtk_dpi_conf *conf;
> > > > int ret;
> > > >  
> > > > +   match = of_match_node(mtk_dpi_of_ids, dev->of_node);
> > > > +   if (!match)
> > > > +   return -ENODEV;
> > > > +
> > > > dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);
> > > > if (!dpi)
> > > > return -ENOMEM;
> > > >  
> > > > dpi->dev = dev;
> > > > +   dpi->data = (void *)match->data;
> > > > +   conf = (struct mtk_dpi_conf *)match->data;
> > > >  
> > > > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > dpi->regs = devm_ioremap_resource(dev, mem);
> > > > @@ -679,24 +777,9 @@ static int mtk_dpi_probe(struct platform_device 
> > > > *pdev)
> > > > return ret;
> > > > }
> > > >  
> > > > -   dpi->engine_clk = devm_clk_get(dev, "engine");
> > > > -   if (IS_ERR(dpi->engine_clk)) {
> > > > -   ret = PTR_ERR(dpi->engine_clk);
> > > > -   dev_err(dev, "Failed to get engine clock: %d\n", ret);
> > > > -   return ret;
> > > > -   }
> > > > -
> > > > -   dpi->pixel_clk = devm_clk_get(dev, "pixel");
> > > > -   if (IS_ERR(dpi->pixel_clk)) {
> > > > -   ret = PTR_ERR(dpi->pixel_clk);
> > > > -   dev_err(dev, "Failed to get pixel clock: %d\n", ret);
> > > > -   return ret;
> > > > -   }
> > > > -
> > > > -   dpi->tvd_clk = devm_clk_get(dev, "pll");
> > > > -   if (IS_ERR(dpi->tvd_clk)) {
> > > > -   ret = PTR_ERR(dpi->tvd_clk);
> > > > -   dev_err(dev, "Failed to get tvdpll clock: %d\n", ret);
> > > > +   ret = conf->parse_clk_from_dt(dpi, np);
> > > > +   if (ret) {
> > > > +   dev_err(dev, "parse tvd div clk failed!");
> > > > return ret;
> > > > }
> > > >  
> > > > @@ -754,11 +837,6 @@ static int mtk_dpi_remove(struct platform_device 
> > > > *pdev)
> > > > return 0;
> > > >  }
> > > >  
> > > > -static const struct of_device_id mtk_dpi_of_ids[] = {
> > > > -   { .compatible = "mediatek,mt8173-dpi", },
> > > > -   {}
> > > > -};
> > > > -
> > > >  struct platform_driver mtk_dpi_driver = {
> > > > .probe = mtk_dpi_probe,
> > > > .remove = mtk_dpi_remove,
> > > 
> > > Regards,
> > > CK
> > > 
> > 
> 
> 
> Regards,
> CK
> > 
> 
> 

Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[Bibby Hsieh]

Hi, CK,

Thanks for your comments.

On Mon, 2016-07-25 at 14:49 +0800, CK Hu wrote:
> Hi, Bibby:
> 
> On Mon, 2016-07-25 at 14:24 +0800, Bibby Hsieh wrote:
> > Hi, CK,
> > 
> > Thanks for your comments.
> > 
> > On Wed, 2016-07-20 at 15:57 +0800, CK Hu wrote:
> > > Hi, Bibby:
> > > 
> > > Some comments inline.
> > > 
> > > On Wed, 2016-07-20 at 12:03 +0800, Bibby Hsieh wrote:
> > > > From: Junzhi Zhao 
> > > > 
> > > > Pixel clock should be 297MHz when resolution is 4K.
> > > > 
> > > > Signed-off-by: Junzhi Zhao 
> > > > Signed-off-by: Bibby Hsieh 
> > > > ---
> > > >  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> > > > +---
> > > >  1 file changed, 131 insertions(+), 53 deletions(-)
> > > > 
> 
> [snip...]
> 
> > > >  
> > > > +static int mt8173_parse_clk_from_dt(struct mtk_dpi *dpi, struct 
> > > > device_node *np)
> > > > +{
> > > > +   int i;
> > > > +
> > > > +   for (i = 0; i < ARRAY_SIZE(mtk_dpi_clk_names); i++) {
> > > > +   dpi->clk[i] = of_clk_get_by_name(np,
> > > > + mtk_dpi_clk_names[i]);
> > > > +   if (IS_ERR(dpi->clk[i]))
> > > > +   return PTR_ERR(dpi->clk[i]);
> > > > +   }
> > > > +   return 0;
> > > > +}
> > > 
> > > I think parsing device tree is a pure SW behavior. Would this vary for
> > > different MTK soc?
> > > 
> > Yes
> 
> I can not imaging that, so could you give me an example source code of
> other MTK soc for parse_clk_from_dt()?
> 
I will do some changes according to your comments. 

> > > > +
> > > > +
> > > > +static const struct mtk_dpi_conf mt8173_conf = {
> > > > +   .parse_clk_from_dt = mt8173_parse_clk_from_dt,
> > > > +   .clk_config = mt8173_clk_config,
> > > > +};
> > > > +
> > > > +static const struct of_device_id mtk_dpi_of_ids[] = {
> > > > +   { .compatible = "mediatek,mt8173-dpi",
> > > > +   .data = _conf,
> > > > +   },
> > > > +   {}
> > > > +};
> > > > +
> > > >  static int mtk_dpi_probe(struct platform_device *pdev)
> > > >  {
> > > > struct device *dev = >dev;
> > > > struct mtk_dpi *dpi;
> > > > struct resource *mem;
> > > > +   struct device_node *np = dev->of_node;
> > > > struct device_node *ep, *bridge_node = NULL;
> > > > int comp_id;
> > > > +   const struct of_device_id *match;
> > > > +   struct mtk_dpi_conf *conf;
> > > > int ret;
> > > >  
> > > > +   match = of_match_node(mtk_dpi_of_ids, dev->of_node);
> > > > +   if (!match)
> > > > +   return -ENODEV;
> > > > +
> > > > dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);
> > > > if (!dpi)
> > > > return -ENOMEM;
> > > >  
> > > > dpi->dev = dev;
> > > > +   dpi->data = (void *)match->data;
> > > > +   conf = (struct mtk_dpi_conf *)match->data;
> > > >  
> > > > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > dpi->regs = devm_ioremap_resource(dev, mem);
> > > > @@ -679,24 +777,9 @@ static int mtk_dpi_probe(struct platform_device 
> > > > *pdev)
> > > > return ret;
> > > > }
> > > >  
> > > > -   dpi->engine_clk = devm_clk_get(dev, "engine");
> > > > -   if (IS_ERR(dpi->engine_clk)) {
> > > > -   ret = PTR_ERR(dpi->engine_clk);
> > > > -   dev_err(dev, "Failed to get engine clock: %d\n", ret);
> > > > -   return ret;
> > > > -   }
> > > > -
> > > > -   dpi->pixel_clk = devm_clk_get(dev, "pixel");
> > > > -   if (IS_ERR(dpi->pixel_clk)) {
> > > > -   ret = PTR_ERR(dpi->pixel_clk);
> > > > -   dev_err(dev, "Failed to get pixel clock: %d\n", ret);
> > > > -   return ret;
> > > > -   }
> > > > -
> > > > -   dpi->tvd_clk = devm_clk_get(dev, "pll");
> > > > -   if (IS_ERR(dpi->tvd_clk)) {
> > > > -   ret = PTR_ERR(dpi->tvd_clk);
> > > > -   dev_err(dev, "Failed to get tvdpll clock: %d\n", ret);
> > > > +   ret = conf->parse_clk_from_dt(dpi, np);
> > > > +   if (ret) {
> > > > +   dev_err(dev, "parse tvd div clk failed!");
> > > > return ret;
> > > > }
> > > >  
> > > > @@ -754,11 +837,6 @@ static int mtk_dpi_remove(struct platform_device 
> > > > *pdev)
> > > > return 0;
> > > >  }
> > > >  
> > > > -static const struct of_device_id mtk_dpi_of_ids[] = {
> > > > -   { .compatible = "mediatek,mt8173-dpi", },
> > > > -   {}
> > > > -};
> > > > -
> > > >  struct platform_driver mtk_dpi_driver = {
> > > > .probe = mtk_dpi_probe,
> > > > .remove = mtk_dpi_remove,
> > > 
> > > Regards,
> > > CK
> > > 
> > 
> 
> 
> Regards,
> CK
> > 
> 
> 

Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[CK Hu]

Hi, Bibby:

On Mon, 2016-07-25 at 14:24 +0800, Bibby Hsieh wrote:
> Hi, CK,
> 
> Thanks for your comments.
> 
> On Wed, 2016-07-20 at 15:57 +0800, CK Hu wrote:
> > Hi, Bibby:
> > 
> > Some comments inline.
> > 
> > On Wed, 2016-07-20 at 12:03 +0800, Bibby Hsieh wrote:
> > > From: Junzhi Zhao 
> > > 
> > > Pixel clock should be 297MHz when resolution is 4K.
> > > 
> > > Signed-off-by: Junzhi Zhao 
> > > Signed-off-by: Bibby Hsieh 
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> > > +---
> > >  1 file changed, 131 insertions(+), 53 deletions(-)
> > > 

[snip...]

> > >  
> > > +static int mt8173_parse_clk_from_dt(struct mtk_dpi *dpi, struct 
> > > device_node *np)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(mtk_dpi_clk_names); i++) {
> > > + dpi->clk[i] = of_clk_get_by_name(np,
> > > +   mtk_dpi_clk_names[i]);
> > > + if (IS_ERR(dpi->clk[i]))
> > > + return PTR_ERR(dpi->clk[i]);
> > > + }
> > > + return 0;
> > > +}
> > 
> > I think parsing device tree is a pure SW behavior. Would this vary for
> > different MTK soc?
> > 
> Yes

I can not imaging that, so could you give me an example source code of
other MTK soc for parse_clk_from_dt()?

> > > +
> > > +
> > > +static const struct mtk_dpi_conf mt8173_conf = {
> > > + .parse_clk_from_dt = mt8173_parse_clk_from_dt,
> > > + .clk_config = mt8173_clk_config,
> > > +};
> > > +
> > > +static const struct of_device_id mtk_dpi_of_ids[] = {
> > > + { .compatible = "mediatek,mt8173-dpi",
> > > + .data = _conf,
> > > + },
> > > + {}
> > > +};
> > > +
> > >  static int mtk_dpi_probe(struct platform_device *pdev)
> > >  {
> > >   struct device *dev = >dev;
> > >   struct mtk_dpi *dpi;
> > >   struct resource *mem;
> > > + struct device_node *np = dev->of_node;
> > >   struct device_node *ep, *bridge_node = NULL;
> > >   int comp_id;
> > > + const struct of_device_id *match;
> > > + struct mtk_dpi_conf *conf;
> > >   int ret;
> > >  
> > > + match = of_match_node(mtk_dpi_of_ids, dev->of_node);
> > > + if (!match)
> > > + return -ENODEV;
> > > +
> > >   dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);
> > >   if (!dpi)
> > >   return -ENOMEM;
> > >  
> > >   dpi->dev = dev;
> > > + dpi->data = (void *)match->data;
> > > + conf = (struct mtk_dpi_conf *)match->data;
> > >  
> > >   mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >   dpi->regs = devm_ioremap_resource(dev, mem);
> > > @@ -679,24 +777,9 @@ static int mtk_dpi_probe(struct platform_device 
> > > *pdev)
> > >   return ret;
> > >   }
> > >  
> > > - dpi->engine_clk = devm_clk_get(dev, "engine");
> > > - if (IS_ERR(dpi->engine_clk)) {
> > > - ret = PTR_ERR(dpi->engine_clk);
> > > - dev_err(dev, "Failed to get engine clock: %d\n", ret);
> > > - return ret;
> > > - }
> > > -
> > > - dpi->pixel_clk = devm_clk_get(dev, "pixel");
> > > - if (IS_ERR(dpi->pixel_clk)) {
> > > - ret = PTR_ERR(dpi->pixel_clk);
> > > - dev_err(dev, "Failed to get pixel clock: %d\n", ret);
> > > - return ret;
> > > - }
> > > -
> > > - dpi->tvd_clk = devm_clk_get(dev, "pll");
> > > - if (IS_ERR(dpi->tvd_clk)) {
> > > - ret = PTR_ERR(dpi->tvd_clk);
> > > - dev_err(dev, "Failed to get tvdpll clock: %d\n", ret);
> > > + ret = conf->parse_clk_from_dt(dpi, np);
> > > + if (ret) {
> > > + dev_err(dev, "parse tvd div clk failed!");
> > >   return ret;
> > >   }
> > >  
> > > @@ -754,11 +837,6 @@ static int mtk_dpi_remove(struct platform_device 
> > > *pdev)
> > >   return 0;
> > >  }
> > >  
> > > -static const struct of_device_id mtk_dpi_of_ids[] = {
> > > - { .compatible = "mediatek,mt8173-dpi", },
> > > - {}
> > > -};
> > > -
> > >  struct platform_driver mtk_dpi_driver = {
> > >   .probe = mtk_dpi_probe,
> > >   .remove = mtk_dpi_remove,
> > 
> > Regards,
> > CK
> > 
> 


Regards,
CK
> 

Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[CK Hu]

Hi, Bibby:

On Mon, 2016-07-25 at 14:24 +0800, Bibby Hsieh wrote:
> Hi, CK,
> 
> Thanks for your comments.
> 
> On Wed, 2016-07-20 at 15:57 +0800, CK Hu wrote:
> > Hi, Bibby:
> > 
> > Some comments inline.
> > 
> > On Wed, 2016-07-20 at 12:03 +0800, Bibby Hsieh wrote:
> > > From: Junzhi Zhao 
> > > 
> > > Pixel clock should be 297MHz when resolution is 4K.
> > > 
> > > Signed-off-by: Junzhi Zhao 
> > > Signed-off-by: Bibby Hsieh 
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> > > +---
> > >  1 file changed, 131 insertions(+), 53 deletions(-)
> > > 

[snip...]

> > >  
> > > +static int mt8173_parse_clk_from_dt(struct mtk_dpi *dpi, struct 
> > > device_node *np)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(mtk_dpi_clk_names); i++) {
> > > + dpi->clk[i] = of_clk_get_by_name(np,
> > > +   mtk_dpi_clk_names[i]);
> > > + if (IS_ERR(dpi->clk[i]))
> > > + return PTR_ERR(dpi->clk[i]);
> > > + }
> > > + return 0;
> > > +}
> > 
> > I think parsing device tree is a pure SW behavior. Would this vary for
> > different MTK soc?
> > 
> Yes

I can not imaging that, so could you give me an example source code of
other MTK soc for parse_clk_from_dt()?

> > > +
> > > +
> > > +static const struct mtk_dpi_conf mt8173_conf = {
> > > + .parse_clk_from_dt = mt8173_parse_clk_from_dt,
> > > + .clk_config = mt8173_clk_config,
> > > +};
> > > +
> > > +static const struct of_device_id mtk_dpi_of_ids[] = {
> > > + { .compatible = "mediatek,mt8173-dpi",
> > > + .data = _conf,
> > > + },
> > > + {}
> > > +};
> > > +
> > >  static int mtk_dpi_probe(struct platform_device *pdev)
> > >  {
> > >   struct device *dev = >dev;
> > >   struct mtk_dpi *dpi;
> > >   struct resource *mem;
> > > + struct device_node *np = dev->of_node;
> > >   struct device_node *ep, *bridge_node = NULL;
> > >   int comp_id;
> > > + const struct of_device_id *match;
> > > + struct mtk_dpi_conf *conf;
> > >   int ret;
> > >  
> > > + match = of_match_node(mtk_dpi_of_ids, dev->of_node);
> > > + if (!match)
> > > + return -ENODEV;
> > > +
> > >   dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);
> > >   if (!dpi)
> > >   return -ENOMEM;
> > >  
> > >   dpi->dev = dev;
> > > + dpi->data = (void *)match->data;
> > > + conf = (struct mtk_dpi_conf *)match->data;
> > >  
> > >   mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >   dpi->regs = devm_ioremap_resource(dev, mem);
> > > @@ -679,24 +777,9 @@ static int mtk_dpi_probe(struct platform_device 
> > > *pdev)
> > >   return ret;
> > >   }
> > >  
> > > - dpi->engine_clk = devm_clk_get(dev, "engine");
> > > - if (IS_ERR(dpi->engine_clk)) {
> > > - ret = PTR_ERR(dpi->engine_clk);
> > > - dev_err(dev, "Failed to get engine clock: %d\n", ret);
> > > - return ret;
> > > - }
> > > -
> > > - dpi->pixel_clk = devm_clk_get(dev, "pixel");
> > > - if (IS_ERR(dpi->pixel_clk)) {
> > > - ret = PTR_ERR(dpi->pixel_clk);
> > > - dev_err(dev, "Failed to get pixel clock: %d\n", ret);
> > > - return ret;
> > > - }
> > > -
> > > - dpi->tvd_clk = devm_clk_get(dev, "pll");
> > > - if (IS_ERR(dpi->tvd_clk)) {
> > > - ret = PTR_ERR(dpi->tvd_clk);
> > > - dev_err(dev, "Failed to get tvdpll clock: %d\n", ret);
> > > + ret = conf->parse_clk_from_dt(dpi, np);
> > > + if (ret) {
> > > + dev_err(dev, "parse tvd div clk failed!");
> > >   return ret;
> > >   }
> > >  
> > > @@ -754,11 +837,6 @@ static int mtk_dpi_remove(struct platform_device 
> > > *pdev)
> > >   return 0;
> > >  }
> > >  
> > > -static const struct of_device_id mtk_dpi_of_ids[] = {
> > > - { .compatible = "mediatek,mt8173-dpi", },
> > > - {}
> > > -};
> > > -
> > >  struct platform_driver mtk_dpi_driver = {
> > >   .probe = mtk_dpi_probe,
> > >   .remove = mtk_dpi_remove,
> > 
> > Regards,
> > CK
> > 
> 


Regards,
CK
> 

Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[Bibby Hsieh]

Hi, Philipp,

Thanks for your comment.

On Wed, 2016-07-20 at 11:41 +0200, Philipp Zabel wrote:
> Am Mittwoch, den 20.07.2016, 12:03 +0800 schrieb Bibby Hsieh:
> > From: Junzhi Zhao 
> > 
> > Pixel clock should be 297MHz when resolution is 4K.
> > 
> > Signed-off-by: Junzhi Zhao 
> > Signed-off-by: Bibby Hsieh 
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> > +---
> >  1 file changed, 131 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c 
> > b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > index d05ca79..c0f04d2 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > @@ -60,14 +60,35 @@ enum mtk_dpi_out_color_format {
> > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL
> >  };
> >  
> > +enum mtk_dpi_clk_id {
> > +   MTK_DPI_CLK_DPI_ENGINE,
> > +   MTK_DPI_CLK_DPI_PIXEL,
> > +   MTK_DPI_CLK_TVD_PLL,
> > +   MTK_DPI_CLK_TVDPLL_MUX,
> > +   MTK_DPI_CLK_TVDPLL_D2,
> > +   MTK_DPI_CLK_TVDPLL_D4,
> > +   MTK_DPI_CLK_TVDPLL_D8,
> > +   MTK_DPI_CLK_TVDPLL_D16,
> > +   MTK_DPI_CLK_COUNT,
> > +};
> 
> I think this is going in the wrong direction. If the pixel clock output
> isn't correct after a clk_set_rate(dpi->pixel_clk, rate), the clock
> drivers should be fixed, not worked around in the dpi driver.
> 
> The TVDPLL_* mux and dividers are not direct inputs to the DPI module:
> 
>tvdpll ("pll")
>  |   ..|\
>  v   ..| | mm_sel > mm_dpi_engine ("engine")
>tvdpll_594m(1/3)  ..|/
>  |
>  |`-> tvdpll_d2 -->|\
>  |`-> tvdpll_d4 -->| | dpi0_sel --> mm_dpi_pixel ("pixel")
>  |`-> tvdpll_d8 -->| |
>  `--> tvdpll_d16 ->|/
> 
> Currently the code first sets the "pll" to the desired multiple of the
> pixel clock manually (*3*4, *3*8) and than calls clk_set_rate on the
> "pixel" clock which gets propagated by the clock framework up to
> dpi0_sel. Since dpi0_sel doesn't have the CLK_SET_RATE_PARENT flag set,
> it should just choose the tvdpll_d* input divider. I'd like not to give
> the dpi driver direct access to all the intermediate clocks.
> 
Ok, I will make some modifications according to your comment.

> regards
> Philipp

Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[Bibby Hsieh]

Hi, Philipp,

Thanks for your comment.

On Wed, 2016-07-20 at 11:41 +0200, Philipp Zabel wrote:
> Am Mittwoch, den 20.07.2016, 12:03 +0800 schrieb Bibby Hsieh:
> > From: Junzhi Zhao 
> > 
> > Pixel clock should be 297MHz when resolution is 4K.
> > 
> > Signed-off-by: Junzhi Zhao 
> > Signed-off-by: Bibby Hsieh 
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> > +---
> >  1 file changed, 131 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c 
> > b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > index d05ca79..c0f04d2 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > @@ -60,14 +60,35 @@ enum mtk_dpi_out_color_format {
> > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL
> >  };
> >  
> > +enum mtk_dpi_clk_id {
> > +   MTK_DPI_CLK_DPI_ENGINE,
> > +   MTK_DPI_CLK_DPI_PIXEL,
> > +   MTK_DPI_CLK_TVD_PLL,
> > +   MTK_DPI_CLK_TVDPLL_MUX,
> > +   MTK_DPI_CLK_TVDPLL_D2,
> > +   MTK_DPI_CLK_TVDPLL_D4,
> > +   MTK_DPI_CLK_TVDPLL_D8,
> > +   MTK_DPI_CLK_TVDPLL_D16,
> > +   MTK_DPI_CLK_COUNT,
> > +};
> 
> I think this is going in the wrong direction. If the pixel clock output
> isn't correct after a clk_set_rate(dpi->pixel_clk, rate), the clock
> drivers should be fixed, not worked around in the dpi driver.
> 
> The TVDPLL_* mux and dividers are not direct inputs to the DPI module:
> 
>tvdpll ("pll")
>  |   ..|\
>  v   ..| | mm_sel > mm_dpi_engine ("engine")
>tvdpll_594m(1/3)  ..|/
>  |
>  |`-> tvdpll_d2 -->|\
>  |`-> tvdpll_d4 -->| | dpi0_sel --> mm_dpi_pixel ("pixel")
>  |`-> tvdpll_d8 -->| |
>  `--> tvdpll_d16 ->|/
> 
> Currently the code first sets the "pll" to the desired multiple of the
> pixel clock manually (*3*4, *3*8) and than calls clk_set_rate on the
> "pixel" clock which gets propagated by the clock framework up to
> dpi0_sel. Since dpi0_sel doesn't have the CLK_SET_RATE_PARENT flag set,
> it should just choose the tvdpll_d* input divider. I'd like not to give
> the dpi driver direct access to all the intermediate clocks.
> 
Ok, I will make some modifications according to your comment.

> regards
> Philipp

Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[Bibby Hsieh]

Hi, CK,

Thanks for your comments.

On Wed, 2016-07-20 at 15:57 +0800, CK Hu wrote:
> Hi, Bibby:
> 
> Some comments inline.
> 
> On Wed, 2016-07-20 at 12:03 +0800, Bibby Hsieh wrote:
> > From: Junzhi Zhao 
> > 
> > Pixel clock should be 297MHz when resolution is 4K.
> > 
> > Signed-off-by: Junzhi Zhao 
> > Signed-off-by: Bibby Hsieh 
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> > +---
> >  1 file changed, 131 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c 
> > b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > index d05ca79..c0f04d2 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > @@ -60,14 +60,35 @@ enum mtk_dpi_out_color_format {
> > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL
> >  };
> >  
> > +enum mtk_dpi_clk_id {
> > +   MTK_DPI_CLK_DPI_ENGINE,
> > +   MTK_DPI_CLK_DPI_PIXEL,
> > +   MTK_DPI_CLK_TVD_PLL,
> > +   MTK_DPI_CLK_TVDPLL_MUX,
> > +   MTK_DPI_CLK_TVDPLL_D2,
> > +   MTK_DPI_CLK_TVDPLL_D4,
> > +   MTK_DPI_CLK_TVDPLL_D8,
> > +   MTK_DPI_CLK_TVDPLL_D16,
> > +   MTK_DPI_CLK_COUNT,
> > +};
> > +
> > +static const char * const mtk_dpi_clk_names[MTK_DPI_CLK_COUNT] = {
> > +   [MTK_DPI_CLK_DPI_ENGINE] = "engine",
> > +   [MTK_DPI_CLK_DPI_PIXEL] = "pixel",
> > +   [MTK_DPI_CLK_TVD_PLL] = "pll",
> > +   [MTK_DPI_CLK_TVDPLL_MUX] = "tvdpll_mux",
> > +   [MTK_DPI_CLK_TVDPLL_D2] = "tvdpll_d2",
> > +   [MTK_DPI_CLK_TVDPLL_D4] = "tvdpll_d4",
> > +   [MTK_DPI_CLK_TVDPLL_D8] = "tvdpll_d8",
> > +   [MTK_DPI_CLK_TVDPLL_D16] = "tvdpll_d16",
> > +};
> > +
> >  struct mtk_dpi {
> > struct mtk_ddp_comp ddp_comp;
> > struct drm_encoder encoder;
> > void __iomem *regs;
> > struct device *dev;
> > -   struct clk *engine_clk;
> > -   struct clk *pixel_clk;
> > -   struct clk *tvd_clk;
> > +   struct clk *clk[MTK_DPI_CLK_COUNT];
> > int irq;
> > struct drm_display_mode mode;
> > enum mtk_dpi_out_color_format color_format;
> > @@ -76,6 +97,7 @@ struct mtk_dpi {
> > enum mtk_dpi_out_channel_swap channel_swap;
> > bool power_sta;
> > u8 power_ctl;
> > +   void *data;
> >  };
> >  
> >  static inline struct mtk_dpi *mtk_dpi_from_encoder(struct drm_encoder *e)
> > @@ -114,6 +136,11 @@ struct mtk_dpi_yc_limit {
> > u16 c_bottom;
> >  };
> >  
> > +struct mtk_dpi_conf {
> > +   int (*parse_clk_from_dt)(struct mtk_dpi *dpi, struct device_node *np);
> > +   int (*clk_config)(struct mtk_dpi *dpi, struct drm_display_mode *mode);
> > +};
> > +
> >  static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 
> > mask)
> >  {
> > u32 tmp = readl(dpi->regs + offset) & ~mask;
> > @@ -377,8 +404,8 @@ static void mtk_dpi_power_off(struct mtk_dpi *dpi, enum 
> > mtk_dpi_power_ctl pctl)
> > return;
> >  
> > mtk_dpi_disable(dpi);
> > -   clk_disable_unprepare(dpi->pixel_clk);
> > -   clk_disable_unprepare(dpi->engine_clk);
> > +   clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_PIXEL]);
> > +   clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
> > dpi->power_sta = false;
> >  }
> >  
> > @@ -395,13 +422,13 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi, enum 
> > mtk_dpi_power_ctl pctl)
> > if (dpi->power_sta)
> > return 0;
> >  
> > -   ret = clk_prepare_enable(dpi->engine_clk);
> > +   ret = clk_prepare_enable(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
> > if (ret) {
> > dev_err(dpi->dev, "Failed to enable engine clock: %d\n", ret);
> > goto err_eng;
> > }
> >  
> > -   ret = clk_prepare_enable(dpi->pixel_clk);
> > +   ret = clk_prepare_enable(dpi->clk[MTK_DPI_CLK_DPI_PIXEL]);
> > if (ret) {
> > dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", ret);
> > goto err_pixel;
> > @@ -412,7 +439,7 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi, enum 
> > mtk_dpi_power_ctl pctl)
> > return 0;
> >  
> >  err_pixel:
> > -   clk_disable_unprepare(dpi->engine_clk);
> > +   clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
> >  err_eng:
> > dpi->power_ctl &= ~pctl;
> > return ret;
> > @@ -428,34 +455,16 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi 
> > *dpi,
> > struct mtk_dpi_sync_param vsync_leven = { 0 };
> > struct mtk_dpi_sync_param vsync_rodd = { 0 };
> > struct mtk_dpi_sync_param vsync_reven = { 0 };
> > -   unsigned long pix_rate;
> > -   unsigned long pll_rate;
> > -   unsigned int factor;
> > +   struct mtk_dpi_conf *conf;
> > +   int ret;
> >  
> > if (!dpi) {
> > dev_err(dpi->dev, "invalid argument\n");
> > return -EINVAL;
> > }
> >  
> > -   pix_rate = 1000UL * mode->clock;
> > -   if (mode->clock <= 74000)
> > -   factor = 8 * 3;
> > -   else
> > -   factor = 4 * 3;
> > -   pll_rate = pix_rate * factor;
> > -
> > -   dev_dbg(dpi->dev, "Want PLL %lu Hz, pixel clock %lu Hz\n",
> > - 

Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[Bibby Hsieh]

Hi, CK,

Thanks for your comments.

On Wed, 2016-07-20 at 15:57 +0800, CK Hu wrote:
> Hi, Bibby:
> 
> Some comments inline.
> 
> On Wed, 2016-07-20 at 12:03 +0800, Bibby Hsieh wrote:
> > From: Junzhi Zhao 
> > 
> > Pixel clock should be 297MHz when resolution is 4K.
> > 
> > Signed-off-by: Junzhi Zhao 
> > Signed-off-by: Bibby Hsieh 
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> > +---
> >  1 file changed, 131 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c 
> > b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > index d05ca79..c0f04d2 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > @@ -60,14 +60,35 @@ enum mtk_dpi_out_color_format {
> > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL
> >  };
> >  
> > +enum mtk_dpi_clk_id {
> > +   MTK_DPI_CLK_DPI_ENGINE,
> > +   MTK_DPI_CLK_DPI_PIXEL,
> > +   MTK_DPI_CLK_TVD_PLL,
> > +   MTK_DPI_CLK_TVDPLL_MUX,
> > +   MTK_DPI_CLK_TVDPLL_D2,
> > +   MTK_DPI_CLK_TVDPLL_D4,
> > +   MTK_DPI_CLK_TVDPLL_D8,
> > +   MTK_DPI_CLK_TVDPLL_D16,
> > +   MTK_DPI_CLK_COUNT,
> > +};
> > +
> > +static const char * const mtk_dpi_clk_names[MTK_DPI_CLK_COUNT] = {
> > +   [MTK_DPI_CLK_DPI_ENGINE] = "engine",
> > +   [MTK_DPI_CLK_DPI_PIXEL] = "pixel",
> > +   [MTK_DPI_CLK_TVD_PLL] = "pll",
> > +   [MTK_DPI_CLK_TVDPLL_MUX] = "tvdpll_mux",
> > +   [MTK_DPI_CLK_TVDPLL_D2] = "tvdpll_d2",
> > +   [MTK_DPI_CLK_TVDPLL_D4] = "tvdpll_d4",
> > +   [MTK_DPI_CLK_TVDPLL_D8] = "tvdpll_d8",
> > +   [MTK_DPI_CLK_TVDPLL_D16] = "tvdpll_d16",
> > +};
> > +
> >  struct mtk_dpi {
> > struct mtk_ddp_comp ddp_comp;
> > struct drm_encoder encoder;
> > void __iomem *regs;
> > struct device *dev;
> > -   struct clk *engine_clk;
> > -   struct clk *pixel_clk;
> > -   struct clk *tvd_clk;
> > +   struct clk *clk[MTK_DPI_CLK_COUNT];
> > int irq;
> > struct drm_display_mode mode;
> > enum mtk_dpi_out_color_format color_format;
> > @@ -76,6 +97,7 @@ struct mtk_dpi {
> > enum mtk_dpi_out_channel_swap channel_swap;
> > bool power_sta;
> > u8 power_ctl;
> > +   void *data;
> >  };
> >  
> >  static inline struct mtk_dpi *mtk_dpi_from_encoder(struct drm_encoder *e)
> > @@ -114,6 +136,11 @@ struct mtk_dpi_yc_limit {
> > u16 c_bottom;
> >  };
> >  
> > +struct mtk_dpi_conf {
> > +   int (*parse_clk_from_dt)(struct mtk_dpi *dpi, struct device_node *np);
> > +   int (*clk_config)(struct mtk_dpi *dpi, struct drm_display_mode *mode);
> > +};
> > +
> >  static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 
> > mask)
> >  {
> > u32 tmp = readl(dpi->regs + offset) & ~mask;
> > @@ -377,8 +404,8 @@ static void mtk_dpi_power_off(struct mtk_dpi *dpi, enum 
> > mtk_dpi_power_ctl pctl)
> > return;
> >  
> > mtk_dpi_disable(dpi);
> > -   clk_disable_unprepare(dpi->pixel_clk);
> > -   clk_disable_unprepare(dpi->engine_clk);
> > +   clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_PIXEL]);
> > +   clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
> > dpi->power_sta = false;
> >  }
> >  
> > @@ -395,13 +422,13 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi, enum 
> > mtk_dpi_power_ctl pctl)
> > if (dpi->power_sta)
> > return 0;
> >  
> > -   ret = clk_prepare_enable(dpi->engine_clk);
> > +   ret = clk_prepare_enable(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
> > if (ret) {
> > dev_err(dpi->dev, "Failed to enable engine clock: %d\n", ret);
> > goto err_eng;
> > }
> >  
> > -   ret = clk_prepare_enable(dpi->pixel_clk);
> > +   ret = clk_prepare_enable(dpi->clk[MTK_DPI_CLK_DPI_PIXEL]);
> > if (ret) {
> > dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", ret);
> > goto err_pixel;
> > @@ -412,7 +439,7 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi, enum 
> > mtk_dpi_power_ctl pctl)
> > return 0;
> >  
> >  err_pixel:
> > -   clk_disable_unprepare(dpi->engine_clk);
> > +   clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
> >  err_eng:
> > dpi->power_ctl &= ~pctl;
> > return ret;
> > @@ -428,34 +455,16 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi 
> > *dpi,
> > struct mtk_dpi_sync_param vsync_leven = { 0 };
> > struct mtk_dpi_sync_param vsync_rodd = { 0 };
> > struct mtk_dpi_sync_param vsync_reven = { 0 };
> > -   unsigned long pix_rate;
> > -   unsigned long pll_rate;
> > -   unsigned int factor;
> > +   struct mtk_dpi_conf *conf;
> > +   int ret;
> >  
> > if (!dpi) {
> > dev_err(dpi->dev, "invalid argument\n");
> > return -EINVAL;
> > }
> >  
> > -   pix_rate = 1000UL * mode->clock;
> > -   if (mode->clock <= 74000)
> > -   factor = 8 * 3;
> > -   else
> > -   factor = 4 * 3;
> > -   pll_rate = pix_rate * factor;
> > -
> > -   dev_dbg(dpi->dev, "Want PLL %lu Hz, pixel clock %lu Hz\n",
> > -   pll_rate, pix_rate);
> > -
> > -   clk_set_rate(dpi->tvd_clk, pll_rate);
> 

Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[Philipp Zabel]

Am Mittwoch, den 20.07.2016, 12:03 +0800 schrieb Bibby Hsieh:
> From: Junzhi Zhao 
> 
> Pixel clock should be 297MHz when resolution is 4K.
> 
> Signed-off-by: Junzhi Zhao 
> Signed-off-by: Bibby Hsieh 
> ---
>  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> +---
>  1 file changed, 131 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c 
> b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index d05ca79..c0f04d2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -60,14 +60,35 @@ enum mtk_dpi_out_color_format {
>   MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL
>  };
>  
> +enum mtk_dpi_clk_id {
> + MTK_DPI_CLK_DPI_ENGINE,
> + MTK_DPI_CLK_DPI_PIXEL,
> + MTK_DPI_CLK_TVD_PLL,
> + MTK_DPI_CLK_TVDPLL_MUX,
> + MTK_DPI_CLK_TVDPLL_D2,
> + MTK_DPI_CLK_TVDPLL_D4,
> + MTK_DPI_CLK_TVDPLL_D8,
> + MTK_DPI_CLK_TVDPLL_D16,
> + MTK_DPI_CLK_COUNT,
> +};

I think this is going in the wrong direction. If the pixel clock output
isn't correct after a clk_set_rate(dpi->pixel_clk, rate), the clock
drivers should be fixed, not worked around in the dpi driver.

The TVDPLL_* mux and dividers are not direct inputs to the DPI module:

   tvdpll ("pll")
 |   ..|\
 v   ..| | mm_sel > mm_dpi_engine ("engine")
   tvdpll_594m(1/3)  ..|/
 |
 |`-> tvdpll_d2 -->|\
 |`-> tvdpll_d4 -->| | dpi0_sel --> mm_dpi_pixel ("pixel")
 |`-> tvdpll_d8 -->| |
 `--> tvdpll_d16 ->|/

Currently the code first sets the "pll" to the desired multiple of the
pixel clock manually (*3*4, *3*8) and than calls clk_set_rate on the
"pixel" clock which gets propagated by the clock framework up to
dpi0_sel. Since dpi0_sel doesn't have the CLK_SET_RATE_PARENT flag set,
it should just choose the tvdpll_d* input divider. I'd like not to give
the dpi driver direct access to all the intermediate clocks.

regards
Philipp

Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[Philipp Zabel]

Am Mittwoch, den 20.07.2016, 12:03 +0800 schrieb Bibby Hsieh:
> From: Junzhi Zhao 
> 
> Pixel clock should be 297MHz when resolution is 4K.
> 
> Signed-off-by: Junzhi Zhao 
> Signed-off-by: Bibby Hsieh 
> ---
>  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> +---
>  1 file changed, 131 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c 
> b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index d05ca79..c0f04d2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -60,14 +60,35 @@ enum mtk_dpi_out_color_format {
>   MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL
>  };
>  
> +enum mtk_dpi_clk_id {
> + MTK_DPI_CLK_DPI_ENGINE,
> + MTK_DPI_CLK_DPI_PIXEL,
> + MTK_DPI_CLK_TVD_PLL,
> + MTK_DPI_CLK_TVDPLL_MUX,
> + MTK_DPI_CLK_TVDPLL_D2,
> + MTK_DPI_CLK_TVDPLL_D4,
> + MTK_DPI_CLK_TVDPLL_D8,
> + MTK_DPI_CLK_TVDPLL_D16,
> + MTK_DPI_CLK_COUNT,
> +};

I think this is going in the wrong direction. If the pixel clock output
isn't correct after a clk_set_rate(dpi->pixel_clk, rate), the clock
drivers should be fixed, not worked around in the dpi driver.

The TVDPLL_* mux and dividers are not direct inputs to the DPI module:

   tvdpll ("pll")
 |   ..|\
 v   ..| | mm_sel > mm_dpi_engine ("engine")
   tvdpll_594m(1/3)  ..|/
 |
 |`-> tvdpll_d2 -->|\
 |`-> tvdpll_d4 -->| | dpi0_sel --> mm_dpi_pixel ("pixel")
 |`-> tvdpll_d8 -->| |
 `--> tvdpll_d16 ->|/

Currently the code first sets the "pll" to the desired multiple of the
pixel clock manually (*3*4, *3*8) and than calls clk_set_rate on the
"pixel" clock which gets propagated by the clock framework up to
dpi0_sel. Since dpi0_sel doesn't have the CLK_SET_RATE_PARENT flag set,
it should just choose the tvdpll_d* input divider. I'd like not to give
the dpi driver direct access to all the intermediate clocks.

regards
Philipp

Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[CK Hu]

Hi, Bibby:

Some comments inline.

On Wed, 2016-07-20 at 12:03 +0800, Bibby Hsieh wrote:
> From: Junzhi Zhao 
> 
> Pixel clock should be 297MHz when resolution is 4K.
> 
> Signed-off-by: Junzhi Zhao 
> Signed-off-by: Bibby Hsieh 
> ---
>  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> +---
>  1 file changed, 131 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c 
> b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index d05ca79..c0f04d2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -60,14 +60,35 @@ enum mtk_dpi_out_color_format {
>   MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL
>  };
>  
> +enum mtk_dpi_clk_id {
> + MTK_DPI_CLK_DPI_ENGINE,
> + MTK_DPI_CLK_DPI_PIXEL,
> + MTK_DPI_CLK_TVD_PLL,
> + MTK_DPI_CLK_TVDPLL_MUX,
> + MTK_DPI_CLK_TVDPLL_D2,
> + MTK_DPI_CLK_TVDPLL_D4,
> + MTK_DPI_CLK_TVDPLL_D8,
> + MTK_DPI_CLK_TVDPLL_D16,
> + MTK_DPI_CLK_COUNT,
> +};
> +
> +static const char * const mtk_dpi_clk_names[MTK_DPI_CLK_COUNT] = {
> + [MTK_DPI_CLK_DPI_ENGINE] = "engine",
> + [MTK_DPI_CLK_DPI_PIXEL] = "pixel",
> + [MTK_DPI_CLK_TVD_PLL] = "pll",
> + [MTK_DPI_CLK_TVDPLL_MUX] = "tvdpll_mux",
> + [MTK_DPI_CLK_TVDPLL_D2] = "tvdpll_d2",
> + [MTK_DPI_CLK_TVDPLL_D4] = "tvdpll_d4",
> + [MTK_DPI_CLK_TVDPLL_D8] = "tvdpll_d8",
> + [MTK_DPI_CLK_TVDPLL_D16] = "tvdpll_d16",
> +};
> +
>  struct mtk_dpi {
>   struct mtk_ddp_comp ddp_comp;
>   struct drm_encoder encoder;
>   void __iomem *regs;
>   struct device *dev;
> - struct clk *engine_clk;
> - struct clk *pixel_clk;
> - struct clk *tvd_clk;
> + struct clk *clk[MTK_DPI_CLK_COUNT];
>   int irq;
>   struct drm_display_mode mode;
>   enum mtk_dpi_out_color_format color_format;
> @@ -76,6 +97,7 @@ struct mtk_dpi {
>   enum mtk_dpi_out_channel_swap channel_swap;
>   bool power_sta;
>   u8 power_ctl;
> + void *data;
>  };
>  
>  static inline struct mtk_dpi *mtk_dpi_from_encoder(struct drm_encoder *e)
> @@ -114,6 +136,11 @@ struct mtk_dpi_yc_limit {
>   u16 c_bottom;
>  };
>  
> +struct mtk_dpi_conf {
> + int (*parse_clk_from_dt)(struct mtk_dpi *dpi, struct device_node *np);
> + int (*clk_config)(struct mtk_dpi *dpi, struct drm_display_mode *mode);
> +};
> +
>  static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 mask)
>  {
>   u32 tmp = readl(dpi->regs + offset) & ~mask;
> @@ -377,8 +404,8 @@ static void mtk_dpi_power_off(struct mtk_dpi *dpi, enum 
> mtk_dpi_power_ctl pctl)
>   return;
>  
>   mtk_dpi_disable(dpi);
> - clk_disable_unprepare(dpi->pixel_clk);
> - clk_disable_unprepare(dpi->engine_clk);
> + clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_PIXEL]);
> + clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
>   dpi->power_sta = false;
>  }
>  
> @@ -395,13 +422,13 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi, enum 
> mtk_dpi_power_ctl pctl)
>   if (dpi->power_sta)
>   return 0;
>  
> - ret = clk_prepare_enable(dpi->engine_clk);
> + ret = clk_prepare_enable(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
>   if (ret) {
>   dev_err(dpi->dev, "Failed to enable engine clock: %d\n", ret);
>   goto err_eng;
>   }
>  
> - ret = clk_prepare_enable(dpi->pixel_clk);
> + ret = clk_prepare_enable(dpi->clk[MTK_DPI_CLK_DPI_PIXEL]);
>   if (ret) {
>   dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", ret);
>   goto err_pixel;
> @@ -412,7 +439,7 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi, enum 
> mtk_dpi_power_ctl pctl)
>   return 0;
>  
>  err_pixel:
> - clk_disable_unprepare(dpi->engine_clk);
> + clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
>  err_eng:
>   dpi->power_ctl &= ~pctl;
>   return ret;
> @@ -428,34 +455,16 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
>   struct mtk_dpi_sync_param vsync_leven = { 0 };
>   struct mtk_dpi_sync_param vsync_rodd = { 0 };
>   struct mtk_dpi_sync_param vsync_reven = { 0 };
> - unsigned long pix_rate;
> - unsigned long pll_rate;
> - unsigned int factor;
> + struct mtk_dpi_conf *conf;
> + int ret;
>  
>   if (!dpi) {
>   dev_err(dpi->dev, "invalid argument\n");
>   return -EINVAL;
>   }
>  
> - pix_rate = 1000UL * mode->clock;
> - if (mode->clock <= 74000)
> - factor = 8 * 3;
> - else
> - factor = 4 * 3;
> - pll_rate = pix_rate * factor;
> -
> - dev_dbg(dpi->dev, "Want PLL %lu Hz, pixel clock %lu Hz\n",
> - pll_rate, pix_rate);
> -
> - clk_set_rate(dpi->tvd_clk, pll_rate);
> - pll_rate = clk_get_rate(dpi->tvd_clk);
> -
> - pix_rate = pll_rate / factor;
> - clk_set_rate(dpi->pixel_clk, pix_rate);
> -   

Re: [PATCH 3/4] drm/mediatek: fix the wrong pixel clock when resolution is 4K

[CK Hu]

Hi, Bibby:

Some comments inline.

On Wed, 2016-07-20 at 12:03 +0800, Bibby Hsieh wrote:
> From: Junzhi Zhao 
> 
> Pixel clock should be 297MHz when resolution is 4K.
> 
> Signed-off-by: Junzhi Zhao 
> Signed-off-by: Bibby Hsieh 
> ---
>  drivers/gpu/drm/mediatek/mtk_dpi.c |  184 
> +---
>  1 file changed, 131 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c 
> b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index d05ca79..c0f04d2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -60,14 +60,35 @@ enum mtk_dpi_out_color_format {
>   MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL
>  };
>  
> +enum mtk_dpi_clk_id {
> + MTK_DPI_CLK_DPI_ENGINE,
> + MTK_DPI_CLK_DPI_PIXEL,
> + MTK_DPI_CLK_TVD_PLL,
> + MTK_DPI_CLK_TVDPLL_MUX,
> + MTK_DPI_CLK_TVDPLL_D2,
> + MTK_DPI_CLK_TVDPLL_D4,
> + MTK_DPI_CLK_TVDPLL_D8,
> + MTK_DPI_CLK_TVDPLL_D16,
> + MTK_DPI_CLK_COUNT,
> +};
> +
> +static const char * const mtk_dpi_clk_names[MTK_DPI_CLK_COUNT] = {
> + [MTK_DPI_CLK_DPI_ENGINE] = "engine",
> + [MTK_DPI_CLK_DPI_PIXEL] = "pixel",
> + [MTK_DPI_CLK_TVD_PLL] = "pll",
> + [MTK_DPI_CLK_TVDPLL_MUX] = "tvdpll_mux",
> + [MTK_DPI_CLK_TVDPLL_D2] = "tvdpll_d2",
> + [MTK_DPI_CLK_TVDPLL_D4] = "tvdpll_d4",
> + [MTK_DPI_CLK_TVDPLL_D8] = "tvdpll_d8",
> + [MTK_DPI_CLK_TVDPLL_D16] = "tvdpll_d16",
> +};
> +
>  struct mtk_dpi {
>   struct mtk_ddp_comp ddp_comp;
>   struct drm_encoder encoder;
>   void __iomem *regs;
>   struct device *dev;
> - struct clk *engine_clk;
> - struct clk *pixel_clk;
> - struct clk *tvd_clk;
> + struct clk *clk[MTK_DPI_CLK_COUNT];
>   int irq;
>   struct drm_display_mode mode;
>   enum mtk_dpi_out_color_format color_format;
> @@ -76,6 +97,7 @@ struct mtk_dpi {
>   enum mtk_dpi_out_channel_swap channel_swap;
>   bool power_sta;
>   u8 power_ctl;
> + void *data;
>  };
>  
>  static inline struct mtk_dpi *mtk_dpi_from_encoder(struct drm_encoder *e)
> @@ -114,6 +136,11 @@ struct mtk_dpi_yc_limit {
>   u16 c_bottom;
>  };
>  
> +struct mtk_dpi_conf {
> + int (*parse_clk_from_dt)(struct mtk_dpi *dpi, struct device_node *np);
> + int (*clk_config)(struct mtk_dpi *dpi, struct drm_display_mode *mode);
> +};
> +
>  static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 mask)
>  {
>   u32 tmp = readl(dpi->regs + offset) & ~mask;
> @@ -377,8 +404,8 @@ static void mtk_dpi_power_off(struct mtk_dpi *dpi, enum 
> mtk_dpi_power_ctl pctl)
>   return;
>  
>   mtk_dpi_disable(dpi);
> - clk_disable_unprepare(dpi->pixel_clk);
> - clk_disable_unprepare(dpi->engine_clk);
> + clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_PIXEL]);
> + clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
>   dpi->power_sta = false;
>  }
>  
> @@ -395,13 +422,13 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi, enum 
> mtk_dpi_power_ctl pctl)
>   if (dpi->power_sta)
>   return 0;
>  
> - ret = clk_prepare_enable(dpi->engine_clk);
> + ret = clk_prepare_enable(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
>   if (ret) {
>   dev_err(dpi->dev, "Failed to enable engine clock: %d\n", ret);
>   goto err_eng;
>   }
>  
> - ret = clk_prepare_enable(dpi->pixel_clk);
> + ret = clk_prepare_enable(dpi->clk[MTK_DPI_CLK_DPI_PIXEL]);
>   if (ret) {
>   dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", ret);
>   goto err_pixel;
> @@ -412,7 +439,7 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi, enum 
> mtk_dpi_power_ctl pctl)
>   return 0;
>  
>  err_pixel:
> - clk_disable_unprepare(dpi->engine_clk);
> + clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);
>  err_eng:
>   dpi->power_ctl &= ~pctl;
>   return ret;
> @@ -428,34 +455,16 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
>   struct mtk_dpi_sync_param vsync_leven = { 0 };
>   struct mtk_dpi_sync_param vsync_rodd = { 0 };
>   struct mtk_dpi_sync_param vsync_reven = { 0 };
> - unsigned long pix_rate;
> - unsigned long pll_rate;
> - unsigned int factor;
> + struct mtk_dpi_conf *conf;
> + int ret;
>  
>   if (!dpi) {
>   dev_err(dpi->dev, "invalid argument\n");
>   return -EINVAL;
>   }
>  
> - pix_rate = 1000UL * mode->clock;
> - if (mode->clock <= 74000)
> - factor = 8 * 3;
> - else
> - factor = 4 * 3;
> - pll_rate = pix_rate * factor;
> -
> - dev_dbg(dpi->dev, "Want PLL %lu Hz, pixel clock %lu Hz\n",
> - pll_rate, pix_rate);
> -
> - clk_set_rate(dpi->tvd_clk, pll_rate);
> - pll_rate = clk_get_rate(dpi->tvd_clk);
> -
> - pix_rate = pll_rate / factor;
> - clk_set_rate(dpi->pixel_clk, pix_rate);
> - pix_rate = clk_get_rate(dpi->pixel_clk);
> -
> - dev_dbg(dpi->dev, "Got