| Message ID | 20220620121028.29234-12-rex-bc.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mediatek: Add MT8195 dp_intf driver | expand |
Hi, Bo-Chen: On Mon, 2022-06-20 at 20:10 +0800, Bo-Chen Chen wrote: > We should enable/disable tvd_clk when power_on/power_off, so add this > patch to do this. Without this patch, what would happen? It seems this patch is redundant for these SoCs: static const struct of_device_id mtk_dpi_of_ids[] = { { .compatible = "mediatek,mt2701-dpi", .data = &mt2701_conf, }, { .compatible = "mediatek,mt8173-dpi", .data = &mt8173_conf, }, { .compatible = "mediatek,mt8183-dpi", .data = &mt8183_conf, }, { .compatible = "mediatek,mt8192-dpi", .data = &mt8192_conf, }, { }, }; Regards, CK > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > b/drivers/gpu/drm/mediatek/mtk_dpi.c > index 2717b1741b7a..f83ecb154457 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -455,6 +455,7 @@ static void mtk_dpi_power_off(struct mtk_dpi > *dpi) > mtk_dpi_disable(dpi); > clk_disable_unprepare(dpi->pixel_clk); > clk_disable_unprepare(dpi->engine_clk); > + clk_disable_unprepare(dpi->tvd_clk); > } > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > @@ -464,10 +465,16 @@ static int mtk_dpi_power_on(struct mtk_dpi > *dpi) > if (++dpi->refcount != 1) > return 0; > > + ret = clk_prepare_enable(dpi->tvd_clk); > + if (ret) { > + dev_err(dpi->dev, "Failed to enable tvd pll: %d\n", > ret); > + goto err_refcount; > + } > + > ret = clk_prepare_enable(dpi->engine_clk); > if (ret) { > dev_err(dpi->dev, "Failed to enable engine clock: > %d\n", ret); > - goto err_refcount; > + goto err_engine; > } > > ret = clk_prepare_enable(dpi->pixel_clk); > @@ -484,6 +491,8 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > err_pixel: > clk_disable_unprepare(dpi->engine_clk); > +err_engine: > + clk_disable_unprepare(dpi->tvd_clk); > err_refcount: > dpi->refcount--; > return ret;
On Tue, 2022-06-21 at 10:55 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Mon, 2022-06-20 at 20:10 +0800, Bo-Chen Chen wrote: > > We should enable/disable tvd_clk when power_on/power_off, so add > > this > > patch to do this. > > Without this patch, what would happen? > It seems this patch is redundant for these SoCs: > > static const struct of_device_id mtk_dpi_of_ids[] = { > { .compatible = "mediatek,mt2701-dpi", > .data = &mt2701_conf, > }, > { .compatible = "mediatek,mt8173-dpi", > .data = &mt8173_conf, > }, > { .compatible = "mediatek,mt8183-dpi", > .data = &mt8183_conf, > }, > { .compatible = "mediatek,mt8192-dpi", > .data = &mt8192_conf, > }, > { }, > }; > > Regards, > CK > Hello CK, IMO, this is a bug fix patch. From the usage of clock, if we want to use it, we should enable it . Therefore, I think we should add this and I will add a fix tag for this patch. BRs, Bo-Chen > > > > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > --- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > index 2717b1741b7a..f83ecb154457 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > @@ -455,6 +455,7 @@ static void mtk_dpi_power_off(struct mtk_dpi > > *dpi) > > mtk_dpi_disable(dpi); > > clk_disable_unprepare(dpi->pixel_clk); > > clk_disable_unprepare(dpi->engine_clk); > > + clk_disable_unprepare(dpi->tvd_clk); > > } > > > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > @@ -464,10 +465,16 @@ static int mtk_dpi_power_on(struct mtk_dpi > > *dpi) > > if (++dpi->refcount != 1) > > return 0; > > > > + ret = clk_prepare_enable(dpi->tvd_clk); > > + if (ret) { > > + dev_err(dpi->dev, "Failed to enable tvd pll: %d\n", > > ret); > > + goto err_refcount; > > + } > > + > > ret = clk_prepare_enable(dpi->engine_clk); > > if (ret) { > > dev_err(dpi->dev, "Failed to enable engine clock: > > %d\n", ret); > > - goto err_refcount; > > + goto err_engine; > > } > > > > ret = clk_prepare_enable(dpi->pixel_clk); > > @@ -484,6 +491,8 @@ static int mtk_dpi_power_on(struct mtk_dpi > > *dpi) > > > > err_pixel: > > clk_disable_unprepare(dpi->engine_clk); > > +err_engine: > > + clk_disable_unprepare(dpi->tvd_clk); > > err_refcount: > > dpi->refcount--; > > return ret; > >
On Tue, 2022-06-21 at 11:11 +0800, Rex-BC Chen wrote: > On Tue, 2022-06-21 at 10:55 +0800, CK Hu wrote: > > Hi, Bo-Chen: > > > > On Mon, 2022-06-20 at 20:10 +0800, Bo-Chen Chen wrote: > > > We should enable/disable tvd_clk when power_on/power_off, so add > > > this > > > patch to do this. > > > > Without this patch, what would happen? > > It seems this patch is redundant for these SoCs: > > > > static const struct of_device_id mtk_dpi_of_ids[] = { > > { .compatible = "mediatek,mt2701-dpi", > > .data = &mt2701_conf, > > }, > > { .compatible = "mediatek,mt8173-dpi", > > .data = &mt8173_conf, > > }, > > { .compatible = "mediatek,mt8183-dpi", > > .data = &mt8183_conf, > > }, > > { .compatible = "mediatek,mt8192-dpi", > > .data = &mt8192_conf, > > }, > > { }, > > }; > > > > Regards, > > CK > > > > Hello CK, > > IMO, this is a bug fix patch. From the usage of clock, if we want to > use it, we should enable it . Therefore, I think we should add this > and > I will add a fix tag for this patch. I think mt8173 chromebook use this driver for HDMI output. So mt8173 chromebook HDMI could not work normally? Regards, CK > > BRs, > Bo-Chen > > > > > > > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > > --- > > > drivers/gpu/drm/mediatek/mtk_dpi.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > index 2717b1741b7a..f83ecb154457 100644 > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > @@ -455,6 +455,7 @@ static void mtk_dpi_power_off(struct mtk_dpi > > > *dpi) > > > mtk_dpi_disable(dpi); > > > clk_disable_unprepare(dpi->pixel_clk); > > > clk_disable_unprepare(dpi->engine_clk); > > > + clk_disable_unprepare(dpi->tvd_clk); > > > } > > > > > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > > @@ -464,10 +465,16 @@ static int mtk_dpi_power_on(struct mtk_dpi > > > *dpi) > > > if (++dpi->refcount != 1) > > > return 0; > > > > > > + ret = clk_prepare_enable(dpi->tvd_clk); > > > + if (ret) { > > > + dev_err(dpi->dev, "Failed to enable tvd pll: %d\n", > > > ret); > > > + goto err_refcount; > > > + } > > > + > > > ret = clk_prepare_enable(dpi->engine_clk); > > > if (ret) { > > > dev_err(dpi->dev, "Failed to enable engine clock: > > > %d\n", ret); > > > - goto err_refcount; > > > + goto err_engine; > > > } > > > > > > ret = clk_prepare_enable(dpi->pixel_clk); > > > @@ -484,6 +491,8 @@ static int mtk_dpi_power_on(struct mtk_dpi > > > *dpi) > > > > > > err_pixel: > > > clk_disable_unprepare(dpi->engine_clk); > > > +err_engine: > > > + clk_disable_unprepare(dpi->tvd_clk); > > > err_refcount: > > > dpi->refcount--; > > > return ret; > > > > > >
On Tue, 2022-06-21 at 11:45 +0800, CK Hu wrote: > On Tue, 2022-06-21 at 11:11 +0800, Rex-BC Chen wrote: > > On Tue, 2022-06-21 at 10:55 +0800, CK Hu wrote: > > > Hi, Bo-Chen: > > > > > > On Mon, 2022-06-20 at 20:10 +0800, Bo-Chen Chen wrote: > > > > We should enable/disable tvd_clk when power_on/power_off, so > > > > add > > > > this > > > > patch to do this. > > > > > > Without this patch, what would happen? > > > It seems this patch is redundant for these SoCs: > > > > > > static const struct of_device_id mtk_dpi_of_ids[] = { > > > { .compatible = "mediatek,mt2701-dpi", > > > .data = &mt2701_conf, > > > }, > > > { .compatible = "mediatek,mt8173-dpi", > > > .data = &mt8173_conf, > > > }, > > > { .compatible = "mediatek,mt8183-dpi", > > > .data = &mt8183_conf, > > > }, > > > { .compatible = "mediatek,mt8192-dpi", > > > .data = &mt8192_conf, > > > }, > > > { }, > > > }; > > > > > > Regards, > > > CK > > > > > > > Hello CK, > > > > IMO, this is a bug fix patch. From the usage of clock, if we want > > to > > use it, we should enable it . Therefore, I think we should add this > > and > > I will add a fix tag for this patch. > > I think mt8173 chromebook use this driver for HDMI output. So mt8173 > chromebook HDMI could not work normally? > > Regards, > CK > Hmm.. I am not sure about this. But without this patch, dpi is also working in mt8183/mt8192. It may be related to the ccf driver. But anyway, I think we should do this whether ccf driver helps us to enable this clock. BRs, Bo-Chen > > > > BRs, > > Bo-Chen > > > > > > > > > > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > > > --- > > > > drivers/gpu/drm/mediatek/mtk_dpi.c | 11 ++++++++++- > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > index 2717b1741b7a..f83ecb154457 100644 > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > @@ -455,6 +455,7 @@ static void mtk_dpi_power_off(struct > > > > mtk_dpi > > > > *dpi) > > > > mtk_dpi_disable(dpi); > > > > clk_disable_unprepare(dpi->pixel_clk); > > > > clk_disable_unprepare(dpi->engine_clk); > > > > + clk_disable_unprepare(dpi->tvd_clk); > > > > } > > > > > > > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > > > @@ -464,10 +465,16 @@ static int mtk_dpi_power_on(struct > > > > mtk_dpi > > > > *dpi) > > > > if (++dpi->refcount != 1) > > > > return 0; > > > > > > > > + ret = clk_prepare_enable(dpi->tvd_clk); > > > > + if (ret) { > > > > + dev_err(dpi->dev, "Failed to enable tvd pll: > > > > %d\n", > > > > ret); > > > > + goto err_refcount; > > > > + } > > > > + > > > > ret = clk_prepare_enable(dpi->engine_clk); > > > > if (ret) { > > > > dev_err(dpi->dev, "Failed to enable engine > > > > clock: > > > > %d\n", ret); > > > > - goto err_refcount; > > > > + goto err_engine; > > > > } > > > > > > > > ret = clk_prepare_enable(dpi->pixel_clk); > > > > @@ -484,6 +491,8 @@ static int mtk_dpi_power_on(struct mtk_dpi > > > > *dpi) > > > > > > > > err_pixel: > > > > clk_disable_unprepare(dpi->engine_clk); > > > > +err_engine: > > > > + clk_disable_unprepare(dpi->tvd_clk); > > > > err_refcount: > > > > dpi->refcount--; > > > > return ret; > > > > > > > > > > > >
Hi, Rex: On Tue, 2022-06-21 at 11:50 +0800, Rex-BC Chen wrote: > On Tue, 2022-06-21 at 11:45 +0800, CK Hu wrote: > > On Tue, 2022-06-21 at 11:11 +0800, Rex-BC Chen wrote: > > > On Tue, 2022-06-21 at 10:55 +0800, CK Hu wrote: > > > > Hi, Bo-Chen: > > > > > > > > On Mon, 2022-06-20 at 20:10 +0800, Bo-Chen Chen wrote: > > > > > We should enable/disable tvd_clk when power_on/power_off, so > > > > > add > > > > > this > > > > > patch to do this. > > > > > > > > Without this patch, what would happen? > > > > It seems this patch is redundant for these SoCs: > > > > > > > > static const struct of_device_id mtk_dpi_of_ids[] = { > > > > { .compatible = "mediatek,mt2701-dpi", > > > > .data = &mt2701_conf, > > > > }, > > > > { .compatible = "mediatek,mt8173-dpi", > > > > .data = &mt8173_conf, > > > > }, > > > > { .compatible = "mediatek,mt8183-dpi", > > > > .data = &mt8183_conf, > > > > }, > > > > { .compatible = "mediatek,mt8192-dpi", > > > > .data = &mt8192_conf, > > > > }, > > > > { }, > > > > }; > > > > > > > > Regards, > > > > CK > > > > > > > > > > Hello CK, > > > > > > IMO, this is a bug fix patch. From the usage of clock, if we want > > > to > > > use it, we should enable it . Therefore, I think we should add > > > this > > > and > > > I will add a fix tag for this patch. > > > > I think mt8173 chromebook use this driver for HDMI output. So > > mt8173 > > chromebook HDMI could not work normally? > > > > Regards, > > CK > > > > Hmm.. > I am not sure about this. But without this patch, dpi is also working > in mt8183/mt8192. It may be related to the ccf driver. But anyway, I > think we should do this whether ccf driver helps us to enable this > clock. OK. So, could you help to fix the bug in ccf? If HDMI is disabled but ccf still turn on this clock, the power would be wasted. Regards, CK > > BRs, > Bo-Chen > > > > > > > BRs, > > > Bo-Chen > > > > > > > > > > > > > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > > > > --- > > > > > drivers/gpu/drm/mediatek/mtk_dpi.c | 11 ++++++++++- > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > index 2717b1741b7a..f83ecb154457 100644 > > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > @@ -455,6 +455,7 @@ static void mtk_dpi_power_off(struct > > > > > mtk_dpi > > > > > *dpi) > > > > > mtk_dpi_disable(dpi); > > > > > clk_disable_unprepare(dpi->pixel_clk); > > > > > clk_disable_unprepare(dpi->engine_clk); > > > > > + clk_disable_unprepare(dpi->tvd_clk); > > > > > } > > > > > > > > > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > > > > @@ -464,10 +465,16 @@ static int mtk_dpi_power_on(struct > > > > > mtk_dpi > > > > > *dpi) > > > > > if (++dpi->refcount != 1) > > > > > return 0; > > > > > > > > > > + ret = clk_prepare_enable(dpi->tvd_clk); > > > > > + if (ret) { > > > > > + dev_err(dpi->dev, "Failed to enable tvd pll: > > > > > %d\n", > > > > > ret); > > > > > + goto err_refcount; > > > > > + } > > > > > + > > > > > ret = clk_prepare_enable(dpi->engine_clk); > > > > > if (ret) { > > > > > dev_err(dpi->dev, "Failed to enable engine > > > > > clock: > > > > > %d\n", ret); > > > > > - goto err_refcount; > > > > > + goto err_engine; > > > > > } > > > > > > > > > > ret = clk_prepare_enable(dpi->pixel_clk); > > > > > @@ -484,6 +491,8 @@ static int mtk_dpi_power_on(struct > > > > > mtk_dpi > > > > > *dpi) > > > > > > > > > > err_pixel: > > > > > clk_disable_unprepare(dpi->engine_clk); > > > > > +err_engine: > > > > > + clk_disable_unprepare(dpi->tvd_clk); > > > > > err_refcount: > > > > > dpi->refcount--; > > > > > return ret; > > > > > > > > > > > > > > > > > > > >
On Tue, 2022-06-21 at 12:08 +0800, CK Hu wrote: > Hi, Rex: > > On Tue, 2022-06-21 at 11:50 +0800, Rex-BC Chen wrote: > > On Tue, 2022-06-21 at 11:45 +0800, CK Hu wrote: > > > On Tue, 2022-06-21 at 11:11 +0800, Rex-BC Chen wrote: > > > > On Tue, 2022-06-21 at 10:55 +0800, CK Hu wrote: > > > > > Hi, Bo-Chen: > > > > > > > > > > On Mon, 2022-06-20 at 20:10 +0800, Bo-Chen Chen wrote: > > > > > > We should enable/disable tvd_clk when power_on/power_off, > > > > > > so > > > > > > add > > > > > > this > > > > > > patch to do this. > > > > > > > > > > Without this patch, what would happen? > > > > > It seems this patch is redundant for these SoCs: > > > > > > > > > > static const struct of_device_id mtk_dpi_of_ids[] = { > > > > > { .compatible = "mediatek,mt2701-dpi", > > > > > .data = &mt2701_conf, > > > > > }, > > > > > { .compatible = "mediatek,mt8173-dpi", > > > > > .data = &mt8173_conf, > > > > > }, > > > > > { .compatible = "mediatek,mt8183-dpi", > > > > > .data = &mt8183_conf, > > > > > }, > > > > > { .compatible = "mediatek,mt8192-dpi", > > > > > .data = &mt8192_conf, > > > > > }, > > > > > { }, > > > > > }; > > > > > > > > > > Regards, > > > > > CK > > > > > > > > > > > > > Hello CK, > > > > > > > > IMO, this is a bug fix patch. From the usage of clock, if we > > > > want > > > > to > > > > use it, we should enable it . Therefore, I think we should add > > > > this > > > > and > > > > I will add a fix tag for this patch. > > > > > > I think mt8173 chromebook use this driver for HDMI output. So > > > mt8173 > > > chromebook HDMI could not work normally? > > > > > > Regards, > > > CK > > > > > > > Hmm.. > > I am not sure about this. But without this patch, dpi is also > > working > > in mt8183/mt8192. It may be related to the ccf driver. But anyway, > > I > > think we should do this whether ccf driver helps us to enable this > > clock. > > OK. So, could you help to fix the bug in ccf? If HDMI is disabled but > ccf still turn on this clock, the power would be wasted. > > Regards, > CK > I am also testing if we don't have this patch and it also "works" (dpintf is working fine). do you think we need this patch or just drop this? For the ccf driver, I am not familiar to ccf and also not a expert of ccf. It just a guest for this. I am not sure whether it's a "bug" or just a. And I think it's not the purpose of this series. If there is any issue, I think we will fix it in the future. BRs, Bo-Chen > > > > BRs, > > Bo-Chen > > > > > > > > > > BRs, > > > > Bo-Chen > > > > > > > > > > > > > > > > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > > > > > --- > > > > > > drivers/gpu/drm/mediatek/mtk_dpi.c | 11 ++++++++++- > > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > index 2717b1741b7a..f83ecb154457 100644 > > > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > @@ -455,6 +455,7 @@ static void mtk_dpi_power_off(struct > > > > > > mtk_dpi > > > > > > *dpi) > > > > > > mtk_dpi_disable(dpi); > > > > > > clk_disable_unprepare(dpi->pixel_clk); > > > > > > clk_disable_unprepare(dpi->engine_clk); > > > > > > + clk_disable_unprepare(dpi->tvd_clk); > > > > > > } > > > > > > > > > > > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > > > > > @@ -464,10 +465,16 @@ static int mtk_dpi_power_on(struct > > > > > > mtk_dpi > > > > > > *dpi) > > > > > > if (++dpi->refcount != 1) > > > > > > return 0; > > > > > > > > > > > > + ret = clk_prepare_enable(dpi->tvd_clk); > > > > > > + if (ret) { > > > > > > + dev_err(dpi->dev, "Failed to enable tvd pll: > > > > > > %d\n", > > > > > > ret); > > > > > > + goto err_refcount; > > > > > > + } > > > > > > + > > > > > > ret = clk_prepare_enable(dpi->engine_clk); > > > > > > if (ret) { > > > > > > dev_err(dpi->dev, "Failed to enable engine > > > > > > clock: > > > > > > %d\n", ret); > > > > > > - goto err_refcount; > > > > > > + goto err_engine; > > > > > > } > > > > > > > > > > > > ret = clk_prepare_enable(dpi->pixel_clk); > > > > > > @@ -484,6 +491,8 @@ static int mtk_dpi_power_on(struct > > > > > > mtk_dpi > > > > > > *dpi) > > > > > > > > > > > > err_pixel: > > > > > > clk_disable_unprepare(dpi->engine_clk); > > > > > > +err_engine: > > > > > > + clk_disable_unprepare(dpi->tvd_clk); > > > > > > err_refcount: > > > > > > dpi->refcount--; > > > > > > return ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Hi, Rex: On Tue, 2022-06-21 at 13:47 +0800, Rex-BC Chen wrote: > On Tue, 2022-06-21 at 12:08 +0800, CK Hu wrote: > > Hi, Rex: > > > > On Tue, 2022-06-21 at 11:50 +0800, Rex-BC Chen wrote: > > > On Tue, 2022-06-21 at 11:45 +0800, CK Hu wrote: > > > > On Tue, 2022-06-21 at 11:11 +0800, Rex-BC Chen wrote: > > > > > On Tue, 2022-06-21 at 10:55 +0800, CK Hu wrote: > > > > > > Hi, Bo-Chen: > > > > > > > > > > > > On Mon, 2022-06-20 at 20:10 +0800, Bo-Chen Chen wrote: > > > > > > > We should enable/disable tvd_clk when power_on/power_off, > > > > > > > so > > > > > > > add > > > > > > > this > > > > > > > patch to do this. > > > > > > > > > > > > Without this patch, what would happen? > > > > > > It seems this patch is redundant for these SoCs: > > > > > > > > > > > > static const struct of_device_id mtk_dpi_of_ids[] = { > > > > > > { .compatible = "mediatek,mt2701-dpi", > > > > > > .data = &mt2701_conf, > > > > > > }, > > > > > > { .compatible = "mediatek,mt8173-dpi", > > > > > > .data = &mt8173_conf, > > > > > > }, > > > > > > { .compatible = "mediatek,mt8183-dpi", > > > > > > .data = &mt8183_conf, > > > > > > }, > > > > > > { .compatible = "mediatek,mt8192-dpi", > > > > > > .data = &mt8192_conf, > > > > > > }, > > > > > > { }, > > > > > > }; > > > > > > > > > > > > Regards, > > > > > > CK > > > > > > > > > > > > > > > > Hello CK, > > > > > > > > > > IMO, this is a bug fix patch. From the usage of clock, if we > > > > > want > > > > > to > > > > > use it, we should enable it . Therefore, I think we should > > > > > add > > > > > this > > > > > and > > > > > I will add a fix tag for this patch. > > > > > > > > I think mt8173 chromebook use this driver for HDMI output. So > > > > mt8173 > > > > chromebook HDMI could not work normally? > > > > > > > > Regards, > > > > CK > > > > > > > > > > Hmm.. > > > I am not sure about this. But without this patch, dpi is also > > > working > > > in mt8183/mt8192. It may be related to the ccf driver. But > > > anyway, > > > I > > > think we should do this whether ccf driver helps us to enable > > > this > > > clock. > > > > OK. So, could you help to fix the bug in ccf? If HDMI is disabled > > but > > ccf still turn on this clock, the power would be wasted. > > > > Regards, > > CK > > > > I am also testing if we don't have this patch and it also "works" > (dpintf is working fine). > do you think we need this patch or just drop this? > > For the ccf driver, I am not familiar to ccf and also not a expert of > ccf. > It just a guest for this. I am not sure whether it's a "bug" or just > a. > And I think it's not the purpose of this series. If there is any > issue, > I think we will fix it in the future. Because we have no idea how this works, I think it's better to drop this patch. Regards, CK > > BRs, > Bo-Chen > > > > > > > BRs, > > > Bo-Chen > > > > > > > > > > > > > BRs, > > > > > Bo-Chen > > > > > > > > > > > > > > > > > > > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/mediatek/mtk_dpi.c | 11 ++++++++++- > > > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > > index 2717b1741b7a..f83ecb154457 100644 > > > > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > > @@ -455,6 +455,7 @@ static void mtk_dpi_power_off(struct > > > > > > > mtk_dpi > > > > > > > *dpi) > > > > > > > mtk_dpi_disable(dpi); > > > > > > > clk_disable_unprepare(dpi->pixel_clk); > > > > > > > clk_disable_unprepare(dpi->engine_clk); > > > > > > > + clk_disable_unprepare(dpi->tvd_clk); > > > > > > > } > > > > > > > > > > > > > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > > > > > > @@ -464,10 +465,16 @@ static int mtk_dpi_power_on(struct > > > > > > > mtk_dpi > > > > > > > *dpi) > > > > > > > if (++dpi->refcount != 1) > > > > > > > return 0; > > > > > > > > > > > > > > + ret = clk_prepare_enable(dpi->tvd_clk); > > > > > > > + if (ret) { > > > > > > > + dev_err(dpi->dev, "Failed to enable tvd pll: > > > > > > > %d\n", > > > > > > > ret); > > > > > > > + goto err_refcount; > > > > > > > + } > > > > > > > + > > > > > > > ret = clk_prepare_enable(dpi->engine_clk); > > > > > > > if (ret) { > > > > > > > dev_err(dpi->dev, "Failed to enable engine > > > > > > > clock: > > > > > > > %d\n", ret); > > > > > > > - goto err_refcount; > > > > > > > + goto err_engine; > > > > > > > } > > > > > > > > > > > > > > ret = clk_prepare_enable(dpi->pixel_clk); > > > > > > > @@ -484,6 +491,8 @@ static int mtk_dpi_power_on(struct > > > > > > > mtk_dpi > > > > > > > *dpi) > > > > > > > > > > > > > > err_pixel: > > > > > > > clk_disable_unprepare(dpi->engine_clk); > > > > > > > +err_engine: > > > > > > > + clk_disable_unprepare(dpi->tvd_clk); > > > > > > > err_refcount: > > > > > > > dpi->refcount--; > > > > > > > return ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Tue, 2022-06-21 at 13:54 +0800, CK Hu wrote: > Hi, Rex: > > On Tue, 2022-06-21 at 13:47 +0800, Rex-BC Chen wrote: > > On Tue, 2022-06-21 at 12:08 +0800, CK Hu wrote: > > > Hi, Rex: > > > > > > On Tue, 2022-06-21 at 11:50 +0800, Rex-BC Chen wrote: > > > > On Tue, 2022-06-21 at 11:45 +0800, CK Hu wrote: > > > > > On Tue, 2022-06-21 at 11:11 +0800, Rex-BC Chen wrote: > > > > > > On Tue, 2022-06-21 at 10:55 +0800, CK Hu wrote: > > > > > > > Hi, Bo-Chen: > > > > > > > > > > > > > > On Mon, 2022-06-20 at 20:10 +0800, Bo-Chen Chen wrote: > > > > > > > > We should enable/disable tvd_clk when > > > > > > > > power_on/power_off, > > > > > > > > so > > > > > > > > add > > > > > > > > this > > > > > > > > patch to do this. > > > > > > > > > > > > > > Without this patch, what would happen? > > > > > > > It seems this patch is redundant for these SoCs: > > > > > > > > > > > > > > static const struct of_device_id mtk_dpi_of_ids[] = { > > > > > > > { .compatible = "mediatek,mt2701-dpi", > > > > > > > .data = &mt2701_conf, > > > > > > > }, > > > > > > > { .compatible = "mediatek,mt8173-dpi", > > > > > > > .data = &mt8173_conf, > > > > > > > }, > > > > > > > { .compatible = "mediatek,mt8183-dpi", > > > > > > > .data = &mt8183_conf, > > > > > > > }, > > > > > > > { .compatible = "mediatek,mt8192-dpi", > > > > > > > .data = &mt8192_conf, > > > > > > > }, > > > > > > > { }, > > > > > > > }; > > > > > > > > > > > > > > Regards, > > > > > > > CK > > > > > > > > > > > > > > > > > > > Hello CK, > > > > > > > > > > > > IMO, this is a bug fix patch. From the usage of clock, if > > > > > > we > > > > > > want > > > > > > to > > > > > > use it, we should enable it . Therefore, I think we should > > > > > > add > > > > > > this > > > > > > and > > > > > > I will add a fix tag for this patch. > > > > > > > > > > I think mt8173 chromebook use this driver for HDMI output. So > > > > > mt8173 > > > > > chromebook HDMI could not work normally? > > > > > > > > > > Regards, > > > > > CK > > > > > > > > > > > > > Hmm.. > > > > I am not sure about this. But without this patch, dpi is also > > > > working > > > > in mt8183/mt8192. It may be related to the ccf driver. But > > > > anyway, > > > > I > > > > think we should do this whether ccf driver helps us to enable > > > > this > > > > clock. > > > > > > OK. So, could you help to fix the bug in ccf? If HDMI is disabled > > > but > > > ccf still turn on this clock, the power would be wasted. > > > > > > Regards, > > > CK > > > > > > > I am also testing if we don't have this patch and it also "works" > > (dpintf is working fine). > > do you think we need this patch or just drop this? > > > > For the ccf driver, I am not familiar to ccf and also not a expert > > of > > ccf. > > It just a guest for this. I am not sure whether it's a "bug" or > > just > > a. > > And I think it's not the purpose of this series. If there is any > > issue, > > I think we will fix it in the future. > > Because we have no idea how this works, I think it's better to drop > this patch. > > Regards, > CK > ok, I will drop this patch in next version. BRs, Bo-Chen > > > > BRs, > > Bo-Chen > > > > > > > > > > BRs, > > > > Bo-Chen > > > > > > > > > > > > > > > > BRs, > > > > > > Bo-Chen > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/mediatek/mtk_dpi.c | 11 ++++++++++- > > > > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > > > index 2717b1741b7a..f83ecb154457 100644 > > > > > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > > > @@ -455,6 +455,7 @@ static void > > > > > > > > mtk_dpi_power_off(struct > > > > > > > > mtk_dpi > > > > > > > > *dpi) > > > > > > > > mtk_dpi_disable(dpi); > > > > > > > > clk_disable_unprepare(dpi->pixel_clk); > > > > > > > > clk_disable_unprepare(dpi->engine_clk); > > > > > > > > + clk_disable_unprepare(dpi->tvd_clk); > > > > > > > > } > > > > > > > > > > > > > > > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > > > > > > > @@ -464,10 +465,16 @@ static int > > > > > > > > mtk_dpi_power_on(struct > > > > > > > > mtk_dpi > > > > > > > > *dpi) > > > > > > > > if (++dpi->refcount != 1) > > > > > > > > return 0; > > > > > > > > > > > > > > > > + ret = clk_prepare_enable(dpi->tvd_clk); > > > > > > > > + if (ret) { > > > > > > > > + dev_err(dpi->dev, "Failed to enable tvd > > > > > > > > pll: > > > > > > > > %d\n", > > > > > > > > ret); > > > > > > > > + goto err_refcount; > > > > > > > > + } > > > > > > > > + > > > > > > > > ret = clk_prepare_enable(dpi->engine_clk); > > > > > > > > if (ret) { > > > > > > > > dev_err(dpi->dev, "Failed to enable > > > > > > > > engine > > > > > > > > clock: > > > > > > > > %d\n", ret); > > > > > > > > - goto err_refcount; > > > > > > > > + goto err_engine; > > > > > > > > } > > > > > > > > > > > > > > > > ret = clk_prepare_enable(dpi->pixel_clk); > > > > > > > > @@ -484,6 +491,8 @@ static int mtk_dpi_power_on(struct > > > > > > > > mtk_dpi > > > > > > > > *dpi) > > > > > > > > > > > > > > > > err_pixel: > > > > > > > > clk_disable_unprepare(dpi->engine_clk); > > > > > > > > +err_engine: > > > > > > > > + clk_disable_unprepare(dpi->tvd_clk); > > > > > > > > err_refcount: > > > > > > > > dpi->refcount--; > > > > > > > > return ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c index 2717b1741b7a..f83ecb154457 100644 --- a/drivers/gpu/drm/mediatek/mtk_dpi.c +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c @@ -455,6 +455,7 @@ static void mtk_dpi_power_off(struct mtk_dpi *dpi) mtk_dpi_disable(dpi); clk_disable_unprepare(dpi->pixel_clk); clk_disable_unprepare(dpi->engine_clk); + clk_disable_unprepare(dpi->tvd_clk); } static int mtk_dpi_power_on(struct mtk_dpi *dpi) @@ -464,10 +465,16 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) if (++dpi->refcount != 1) return 0; + ret = clk_prepare_enable(dpi->tvd_clk); + if (ret) { + dev_err(dpi->dev, "Failed to enable tvd pll: %d\n", ret); + goto err_refcount; + } + ret = clk_prepare_enable(dpi->engine_clk); if (ret) { dev_err(dpi->dev, "Failed to enable engine clock: %d\n", ret); - goto err_refcount; + goto err_engine; } ret = clk_prepare_enable(dpi->pixel_clk); @@ -484,6 +491,8 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) err_pixel: clk_disable_unprepare(dpi->engine_clk); +err_engine: + clk_disable_unprepare(dpi->tvd_clk); err_refcount: dpi->refcount--; return ret;
We should enable/disable tvd_clk when power_on/power_off, so add this patch to do this. Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> --- drivers/gpu/drm/mediatek/mtk_dpi.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)