| Message ID | 1509034175-23767-1-git-send-email-philippe.cornu@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26.10.2017 18:09, Philippe Cornu wrote: > The pixel clock is optional. When available, it offers a better > preciseness for timing computations. > > Signed-off-by: Philippe Cornu <philippe.cornu@st.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index d9cca4f..8b3787d 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -225,6 +225,7 @@ struct dw_mipi_dsi { > void __iomem *base; > > struct clk *pclk; > + struct clk *px_clk; > > unsigned int lane_mbps; /* per lane */ > u32 channel; > @@ -753,24 +754,28 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, > struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge); > const struct dw_mipi_dsi_phy_ops *phy_ops = dsi->plat_data->phy_ops; > void *priv_data = dsi->plat_data->priv_data; > + struct drm_display_mode px_clk_mode = *mode; > int ret; > > clk_prepare_enable(dsi->pclk); > > - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, > + if (dsi->px_clk) > + px_clk_mode.clock = clk_get_rate(dsi->px_clk) / 1000; > + > + ret = phy_ops->get_lane_mbps(priv_data, &px_clk_mode, dsi->mode_flags, > dsi->lanes, dsi->format, &dsi->lane_mbps); Just small suggestion: if pixel clock rate matters, maybe better is to fix it in adjusted_mode in mode_fixup callback. > if (ret) > DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); > > pm_runtime_get_sync(dsi->dev); > dw_mipi_dsi_init(dsi); > - dw_mipi_dsi_dpi_config(dsi, mode); > + dw_mipi_dsi_dpi_config(dsi, &px_clk_mode); > dw_mipi_dsi_packet_handler_config(dsi); > dw_mipi_dsi_video_mode_config(dsi); > - dw_mipi_dsi_video_packet_config(dsi, mode); > + dw_mipi_dsi_video_packet_config(dsi, &px_clk_mode); > dw_mipi_dsi_command_mode_config(dsi); > - dw_mipi_dsi_line_timer_config(dsi, mode); > - dw_mipi_dsi_vertical_timing_config(dsi, mode); > + dw_mipi_dsi_line_timer_config(dsi, &px_clk_mode); > + dw_mipi_dsi_vertical_timing_config(dsi, &px_clk_mode); > > dw_mipi_dsi_dphy_init(dsi); > dw_mipi_dsi_dphy_timing_config(dsi); > @@ -784,7 +789,7 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, > > dw_mipi_dsi_dphy_enable(dsi); > > - dw_mipi_dsi_wait_for_two_frames(mode); > + dw_mipi_dsi_wait_for_two_frames(&px_clk_mode); > > /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ > dw_mipi_dsi_set_mode(dsi, 0); > @@ -878,6 +883,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge) > return ERR_PTR(ret); > } > > + dsi->px_clk = devm_clk_get(dev, "px_clk"); > + if (IS_ERR(dsi->px_clk)) { > + ret = PTR_ERR(dsi->px_clk); > + dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret); > + dsi->px_clk = NULL; > + } > + devm_clk_get is called from bridge::attach callback, do we have guarantee that in ::detach callback the clock will be removed? And what if the clock is defined in dts, but it cannot be get due to other reasons? I guess the code should fail then, ie you should have different paths for -ENOENT and for other errors. -- Regards Andrzej > /* > * Note that the reset was not defined in the initial device tree, so > * we have to be prepared for it not being found.
Hi Philippe, On Thu, 2017-10-26 at 18:09 +0200, Philippe Cornu wrote: > The pixel clock is optional. When available, it offers a better > preciseness for timing computations. > > Signed-off-by: Philippe Cornu <philippe.cornu@st.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index d9cca4f..8b3787d 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -225,6 +225,7 @@ struct dw_mipi_dsi { > void __iomem *base; > > struct clk *pclk; > + struct clk *px_clk; > > unsigned int lane_mbps; /* per lane */ > u32 channel; > @@ -753,24 +754,28 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, > struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge); > const struct dw_mipi_dsi_phy_ops *phy_ops = dsi->plat_data->phy_ops; > void *priv_data = dsi->plat_data->priv_data; > + struct drm_display_mode px_clk_mode = *mode; > int ret; > > clk_prepare_enable(dsi->pclk); > > - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, > + if (dsi->px_clk) > + px_clk_mode.clock = clk_get_rate(dsi->px_clk) / 1000; I don't understand why is the pixel clock rate divided by 1000 different from the mode clock in the first place? If px_clk is just the DPI input pixel clock signal from the display controller, I'd expect the mode to be adjusted correctly. > + > + ret = phy_ops->get_lane_mbps(priv_data, &px_clk_mode, dsi->mode_flags, > dsi->lanes, dsi->format, &dsi->lane_mbps); > if (ret) > DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); > > pm_runtime_get_sync(dsi->dev); > dw_mipi_dsi_init(dsi); > - dw_mipi_dsi_dpi_config(dsi, mode); > + dw_mipi_dsi_dpi_config(dsi, &px_clk_mode); > dw_mipi_dsi_packet_handler_config(dsi); > dw_mipi_dsi_video_mode_config(dsi); > - dw_mipi_dsi_video_packet_config(dsi, mode); > + dw_mipi_dsi_video_packet_config(dsi, &px_clk_mode); > dw_mipi_dsi_command_mode_config(dsi); > - dw_mipi_dsi_line_timer_config(dsi, mode); > - dw_mipi_dsi_vertical_timing_config(dsi, mode); > + dw_mipi_dsi_line_timer_config(dsi, &px_clk_mode); > + dw_mipi_dsi_vertical_timing_config(dsi, &px_clk_mode); > > dw_mipi_dsi_dphy_init(dsi); > dw_mipi_dsi_dphy_timing_config(dsi); > @@ -784,7 +789,7 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, > > dw_mipi_dsi_dphy_enable(dsi); > > - dw_mipi_dsi_wait_for_two_frames(mode); > + dw_mipi_dsi_wait_for_two_frames(&px_clk_mode); > > /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ > dw_mipi_dsi_set_mode(dsi, 0); > @@ -878,6 +883,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge) > return ERR_PTR(ret); > } > > + dsi->px_clk = devm_clk_get(dev, "px_clk"); Is "px_clk" what the pixel clock is called in the datasheet? If this is probed from DT, the new optional property must be documented in the binding docs. > + if (IS_ERR(dsi->px_clk)) { > + ret = PTR_ERR(dsi->px_clk); Better leave ret == 0 for -ENOENT, but return the error for all others. > + dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret); > + dsi->px_clk = NULL; > + } > + > /* > * Note that the reset was not defined in the initial device tree, so > * we have to be prepared for it not being found. regards Philipp
Hi Andrzej, On 10/27/2017 08:41 AM, Andrzej Hajda wrote: > On 26.10.2017 18:09, Philippe Cornu wrote: >> The pixel clock is optional. When available, it offers a better >> preciseness for timing computations. >> >> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------ >> 1 file changed, 18 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> index d9cca4f..8b3787d 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> @@ -225,6 +225,7 @@ struct dw_mipi_dsi { >> void __iomem *base; >> >> struct clk *pclk; >> + struct clk *px_clk; >> >> unsigned int lane_mbps; /* per lane */ >> u32 channel; >> @@ -753,24 +754,28 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, >> struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge); >> const struct dw_mipi_dsi_phy_ops *phy_ops = dsi->plat_data->phy_ops; >> void *priv_data = dsi->plat_data->priv_data; >> + struct drm_display_mode px_clk_mode = *mode; >> int ret; >> >> clk_prepare_enable(dsi->pclk); >> >> - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, >> + if (dsi->px_clk) >> + px_clk_mode.clock = clk_get_rate(dsi->px_clk) / 1000; >> + >> + ret = phy_ops->get_lane_mbps(priv_data, &px_clk_mode, dsi->mode_flags, >> dsi->lanes, dsi->format, &dsi->lane_mbps); > > Just small suggestion: if pixel clock rate matters, maybe better is to > fix it in adjusted_mode in mode_fixup callback. > Well, it was my first idea but unfortunately, "_adjusted_" stuffs are not used by many drivers, and it means that other drivers using this generic dw dsi driver will have to handle the "_adjusted_" stuffs too in their own crtc or dsi specific parts... So that is why I have chosen to use the "optional" pixel clock (as defined in the documentation): as it is optional, it is up to the "specific dt part" to use it or not, whatever the "crtc" driver (often handling the set rate, like in stm32 ltdc.c), avoiding then more patches in the specific parts and more importantly in the crtc parts... What is your opinion then? >> if (ret) >> DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); >> >> pm_runtime_get_sync(dsi->dev); >> dw_mipi_dsi_init(dsi); >> - dw_mipi_dsi_dpi_config(dsi, mode); >> + dw_mipi_dsi_dpi_config(dsi, &px_clk_mode); >> dw_mipi_dsi_packet_handler_config(dsi); >> dw_mipi_dsi_video_mode_config(dsi); >> - dw_mipi_dsi_video_packet_config(dsi, mode); >> + dw_mipi_dsi_video_packet_config(dsi, &px_clk_mode); >> dw_mipi_dsi_command_mode_config(dsi); >> - dw_mipi_dsi_line_timer_config(dsi, mode); >> - dw_mipi_dsi_vertical_timing_config(dsi, mode); >> + dw_mipi_dsi_line_timer_config(dsi, &px_clk_mode); >> + dw_mipi_dsi_vertical_timing_config(dsi, &px_clk_mode); >> >> dw_mipi_dsi_dphy_init(dsi); >> dw_mipi_dsi_dphy_timing_config(dsi); >> @@ -784,7 +789,7 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, >> >> dw_mipi_dsi_dphy_enable(dsi); >> >> - dw_mipi_dsi_wait_for_two_frames(mode); >> + dw_mipi_dsi_wait_for_two_frames(&px_clk_mode); >> >> /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ >> dw_mipi_dsi_set_mode(dsi, 0); >> @@ -878,6 +883,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge) >> return ERR_PTR(ret); >> } >> >> + dsi->px_clk = devm_clk_get(dev, "px_clk"); >> + if (IS_ERR(dsi->px_clk)) { >> + ret = PTR_ERR(dsi->px_clk); >> + dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret); >> + dsi->px_clk = NULL; >> + } >> + > > devm_clk_get is called from bridge::attach callback, do we have > guarantee that in ::detach callback the clock will be removed? > And what if the clock is defined in dts, but it cannot be get due to > other reasons? I guess the code should fail then, ie you should have > different paths for -ENOENT and for other errors. > I will do more tests related to your above comments, Many thanks for your comments and support, Philippe :-) > -- > Regards > Andrzej > >> /* >> * Note that the reset was not defined in the initial device tree, so >> * we have to be prepared for it not being found. > >
Hi Philipp, On 10/27/2017 10:06 AM, Philipp Zabel wrote: > Hi Philippe, > > On Thu, 2017-10-26 at 18:09 +0200, Philippe Cornu wrote: >> The pixel clock is optional. When available, it offers a better >> preciseness for timing computations. >> >> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------ >> 1 file changed, 18 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> index d9cca4f..8b3787d 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> @@ -225,6 +225,7 @@ struct dw_mipi_dsi { >> void __iomem *base; >> >> struct clk *pclk; >> + struct clk *px_clk; >> >> unsigned int lane_mbps; /* per lane */ >> u32 channel; >> @@ -753,24 +754,28 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, >> struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge); >> const struct dw_mipi_dsi_phy_ops *phy_ops = dsi->plat_data->phy_ops; >> void *priv_data = dsi->plat_data->priv_data; >> + struct drm_display_mode px_clk_mode = *mode; >> int ret; >> >> clk_prepare_enable(dsi->pclk); >> >> - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, >> + if (dsi->px_clk) >> + px_clk_mode.clock = clk_get_rate(dsi->px_clk) / 1000; > > I don't understand why is the pixel clock rate divided by 1000 different > from the mode clock in the first place? If px_clk is just the DPI input > pixel clock signal from the display controller, I'd expect the mode to > be adjusted correctly. > The display panel requests a given px clk frequency. Then, the set rate is performed by the "crtc" (ie. the display controller). Depending of the hw platform, the real frequency is different from the requested one as pll work with dividers (and sometimes pll are shared between several IPs adding more constraints...) so preciseness is not "perfect". This preciseness is "hw platform dependent" that is why this px clock is used here only if the platform populates the related dt. >> + >> + ret = phy_ops->get_lane_mbps(priv_data, &px_clk_mode, dsi->mode_flags, >> dsi->lanes, dsi->format, &dsi->lane_mbps); >> if (ret) >> DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); >> >> pm_runtime_get_sync(dsi->dev); >> dw_mipi_dsi_init(dsi); >> - dw_mipi_dsi_dpi_config(dsi, mode); >> + dw_mipi_dsi_dpi_config(dsi, &px_clk_mode); >> dw_mipi_dsi_packet_handler_config(dsi); >> dw_mipi_dsi_video_mode_config(dsi); >> - dw_mipi_dsi_video_packet_config(dsi, mode); >> + dw_mipi_dsi_video_packet_config(dsi, &px_clk_mode); >> dw_mipi_dsi_command_mode_config(dsi); >> - dw_mipi_dsi_line_timer_config(dsi, mode); >> - dw_mipi_dsi_vertical_timing_config(dsi, mode); >> + dw_mipi_dsi_line_timer_config(dsi, &px_clk_mode); >> + dw_mipi_dsi_vertical_timing_config(dsi, &px_clk_mode); >> >> dw_mipi_dsi_dphy_init(dsi); >> dw_mipi_dsi_dphy_timing_config(dsi); >> @@ -784,7 +789,7 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, >> >> dw_mipi_dsi_dphy_enable(dsi); >> >> - dw_mipi_dsi_wait_for_two_frames(mode); >> + dw_mipi_dsi_wait_for_two_frames(&px_clk_mode); >> >> /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ >> dw_mipi_dsi_set_mode(dsi, 0); >> @@ -878,6 +883,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge) >> return ERR_PTR(ret); >> } >> >> + dsi->px_clk = devm_clk_get(dev, "px_clk"); > > Is "px_clk" what the pixel clock is called in the datasheet? > > If this is probed from DT, the new optional property must be documented > in the binding docs. > The px clk (and "px_clk") are already documented in Documentation/devicetree/bindings/display/bridge/dw_mipi_dsi.txt as optional. - "px_clk" is the pixel clock for the DPI/RGB input. (optional) Do you think bindings is clear enough? >> + if (IS_ERR(dsi->px_clk)) { >> + ret = PTR_ERR(dsi->px_clk); > > Better leave ret == 0 for -ENOENT, but return the error for all others. > Ok, I will do more test here. Many thanks for your comments, Philippe :-) >> + dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret); >> + dsi->px_clk = NULL; >> + } >> + >> /* >> * Note that the reset was not defined in the initial device tree, so >> * we have to be prepared for it not being found. > > regards > Philipp >
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index d9cca4f..8b3787d 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -225,6 +225,7 @@ struct dw_mipi_dsi { void __iomem *base; struct clk *pclk; + struct clk *px_clk; unsigned int lane_mbps; /* per lane */ u32 channel; @@ -753,24 +754,28 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge); const struct dw_mipi_dsi_phy_ops *phy_ops = dsi->plat_data->phy_ops; void *priv_data = dsi->plat_data->priv_data; + struct drm_display_mode px_clk_mode = *mode; int ret; clk_prepare_enable(dsi->pclk); - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, + if (dsi->px_clk) + px_clk_mode.clock = clk_get_rate(dsi->px_clk) / 1000; + + ret = phy_ops->get_lane_mbps(priv_data, &px_clk_mode, dsi->mode_flags, dsi->lanes, dsi->format, &dsi->lane_mbps); if (ret) DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); pm_runtime_get_sync(dsi->dev); dw_mipi_dsi_init(dsi); - dw_mipi_dsi_dpi_config(dsi, mode); + dw_mipi_dsi_dpi_config(dsi, &px_clk_mode); dw_mipi_dsi_packet_handler_config(dsi); dw_mipi_dsi_video_mode_config(dsi); - dw_mipi_dsi_video_packet_config(dsi, mode); + dw_mipi_dsi_video_packet_config(dsi, &px_clk_mode); dw_mipi_dsi_command_mode_config(dsi); - dw_mipi_dsi_line_timer_config(dsi, mode); - dw_mipi_dsi_vertical_timing_config(dsi, mode); + dw_mipi_dsi_line_timer_config(dsi, &px_clk_mode); + dw_mipi_dsi_vertical_timing_config(dsi, &px_clk_mode); dw_mipi_dsi_dphy_init(dsi); dw_mipi_dsi_dphy_timing_config(dsi); @@ -784,7 +789,7 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, dw_mipi_dsi_dphy_enable(dsi); - dw_mipi_dsi_wait_for_two_frames(mode); + dw_mipi_dsi_wait_for_two_frames(&px_clk_mode); /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ dw_mipi_dsi_set_mode(dsi, 0); @@ -878,6 +883,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge) return ERR_PTR(ret); } + dsi->px_clk = devm_clk_get(dev, "px_clk"); + if (IS_ERR(dsi->px_clk)) { + ret = PTR_ERR(dsi->px_clk); + dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret); + dsi->px_clk = NULL; + } + /* * Note that the reset was not defined in the initial device tree, so * we have to be prepared for it not being found.
The pixel clock is optional. When available, it offers a better preciseness for timing computations. Signed-off-by: Philippe Cornu <philippe.cornu@st.com> --- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)