From: John Keeping <hidden> Date: 2017-01-21 16:37:08
This is a re-roll of a series originally posted back in September [1].
It's mostly unchanged but in response to Chris and Mark's feedback I have
dropped the first two patches and replaces them with the new patches 3
and 4; it's a slightly different approach from Mark's suggestion because
it turns out that we only use the mode in the enable function and it is
safe to read from the connector state there.
[1] https://www.spinics.net/lists/arm-kernel/msg532931.html
John Keeping (26):
drm/rockchip: dw-mipi-dsi: don't configure hardware in mode_set for
MIPI
drm/rockchip: dw-mipi-dsi: rename commit hook to enable
drm/rockchip: dw-mipi-dsi: pass mode in where needed
drm/rockchip: dw-mipi-dsi: remove mode_set hook
drm/rockchip: dw-mipi-dsi: fix command header writes
drm/rockchip: dw-mipi-dsi: fix generic packet status check
drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf
drm/rockchip: dw-mipi-dsi: include bad value in error message
drm/rockchip: dw-mipi-dsi: respect message flags
drm/rockchip: dw-mipi-dsi: only request HS clock when required
drm/rockchip: dw-mipi-dsi: don't assume buffer is aligned
drm/rockchip: dw-mipi-dsi: prepare panel after phy init
drm/rockchip: dw-mipi-dsi: allow commands in panel_disable
drm/rockchip: dw-mipi-dsi: fix escape clock rate
drm/rockchip: dw-mipi-dsi: ensure PHY is reset
drm/rockchip: dw-mipi-dsi: configure bias and bandgap before enable
drm/rockchip: dw-mipi-dsi: don't enable PHY PLL until it's configured
drm/rockchip: dw-mipi-dsi: properly configure PHY timing
drm/rockchip: dw-mipi-dsi: improve PLL configuration
drm/rockchip: dw-mipi-dsi: use specific poll helper
drm/rockchip: dw-mipi-dsi: use positive check for N{H,V}SYNC
drm/rockchip: vop: test for P{H,V}SYNC
drm/rockchip: dw-mipi-dsi: defer probe if panel is not loaded
drm/rockchip: dw-mipi-dsi: support non-burst modes
drm/rockchip: dw-mipi-dsi: add reset control
drm/rockchip: dw-mipi-dsi: support read commands
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 341 +++++++++++++++++++---------
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 +-
2 files changed, 239 insertions(+), 106 deletions(-)
--
2.11.0.197.gb556de5.dirty
From: John Keeping <hidden> Date: 2017-01-21 16:32:12
We should configure these functions before enabling them.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:32:31
Panel drivers may want to sent commands during the disable function, for
example MIPI_DCS_SET_DISPLAY_OFF before the video signal ends. In order
to send commands we need to write to registers, so pclk must be enabled.
While changing this, remove the unnecessary code after the panel
unprepare call which seems to be a workaround for a specific panel and
thus belongs in the panel driver.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:32:39
This is not needed since we can access the mode via the CRTC from the
enable hook. Also remove the "mode" field that is no longer used.
Signed-off-by: John Keeping <redacted>
---
New in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:32:57
When connected to the MIPI DSI output, we need to use N{H,V}SYNC for the
internal connection but these flags are meaningless for DSI panels.
Switch the test so that we do not set the P{H,V}SYNC bits unless the
mode requires it.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:33:09
In order to fully reset the state of the MIPI controller we must assert
this reset.
This is slightly more complicated than it could be in order to maintain
compatibility with device trees that do not specify the reset property.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
@@ -1159,6 +1161,34 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master,returnret;}+/*+*Notethattheresetwasnotdefinedintheinitialdevicetree,so+*wehavetobepreparedforitnotbeingfound.+*/+apb_rst=devm_reset_control_get(dev,"apb");+if(IS_ERR(apb_rst)){+if(PTR_ERR(apb_rst)==-ENODEV){+apb_rst=NULL;+}else{+dev_err(dev,"Unable to get reset control: %d\n",ret);+returnPTR_ERR(apb_rst);+}+}++if(apb_rst){+ret=clk_prepare_enable(dsi->pclk);+if(ret){+dev_err(dev,"%s: Failed to enable pclk\n",__func__);+returnret;+}++reset_control_assert(apb_rst);+usleep_range(10,20);+reset_control_deassert(apb_rst);++clk_disable_unprepare(dsi->pclk);+}+ret=clk_prepare_enable(dsi->pllref_clk);if(ret){dev_err(dev,"%s: Failed to enable pllref_clk\n",__func__);
From: John Keeping <hidden> Date: 2017-01-21 16:33:18
By dereferencing the MIPI command buffer as a u32* we rely on it being
correctly aligned on ARM, but this may not be the case. Copy it into a
stack variable that will be correctly aligned.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:33:29
As the documentation for readx_poll_timeout says, we want to use the
specialized macro for readl rather than using the generic version
directly.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
@@ -471,14 +471,14 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)PHY_UNRSTZ|PHY_UNSHUTDOWNZ);-ret=readx_poll_timeout(readl,dsi->base+DSI_PHY_STATUS,+ret=readl_poll_timeout(dsi->base+DSI_PHY_STATUS,val,val&LOCK,1000,PHY_STATUS_TIMEOUT_US);if(ret<0){dev_err(dsi->dev,"failed to wait for phy lock state\n");returnret;}-ret=readx_poll_timeout(readl,dsi->base+DSI_PHY_STATUS,+ret=readl_poll_timeout(dsi->base+DSI_PHY_STATUS,val,val&STOP_STATE_CLK_LANE,1000,PHY_STATUS_TIMEOUT_US);if(ret<0){
From: John Keeping <hidden> Date: 2017-01-21 16:33:38
We want to check that both the GEN_CMD_EMPTY and GEN_PLD_W_EMPTY bits
are set so we can't just check "val & mask" because that will be true if
either bit is set.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:33:47
Requesting the HS clock from the PHY before we initialize it causes an
invalid signal to be sent out since the input clock is not yet
configured. The PHY databook suggests only asserting this signal when
performing HS transfers, so let's do that.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:33:57
As an aid to debugging.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: John Keeping <hidden> Date: 2017-01-21 16:34:52
This shows that we only use the mode from the enable function and
prepares us to remove the "mode" field and the mode_set hook in the next
commit.
Signed-off-by: John Keeping <redacted>
---
New in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 41 ++++++++++++++++++----------------
1 file changed, 22 insertions(+), 19 deletions(-)
@@ -330,11 +330,11 @@ static int max_mbps_to_testdin(unsigned int max_mbps)*Thecontrollershouldgenerate2framesbefore*preparingtheperipheral.*/-staticvoiddw_mipi_dsi_wait_for_two_frames(structdw_mipi_dsi*dsi)+staticvoiddw_mipi_dsi_wait_for_two_frames(structdrm_display_mode*mode){intrefresh,two_frames;-refresh=drm_mode_vrefresh(dsi->mode);+refresh=drm_mode_vrefresh(mode);two_frames=DIV_ROUND_UP(MSEC_PER_SEC,refresh)*2;msleep(two_frames);}
@@ -459,7 +459,8 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)returnret;}-staticintdw_mipi_dsi_get_lane_bps(structdw_mipi_dsi*dsi)+staticintdw_mipi_dsi_get_lane_bps(structdw_mipi_dsi*dsi,+structdrm_display_mode*mode){unsignedinti,pre;unsignedlongmpclk,pllref,tmp;
@@ -474,7 +475,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi)returnbpp;}-mpclk=DIV_ROUND_UP(dsi->mode->clock,MSEC_PER_SEC);+mpclk=DIV_ROUND_UP(mode->clock,MSEC_PER_SEC);if(mpclk){/* take 1 / 0.9, since mbps must big than bandwidth of RGB */tmp=mpclk*(bpp/dsi->lanes)*10/9;
@@ -742,43 +743,44 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)/* Get lane byte clock cycles. */staticu32dw_mipi_dsi_get_hcomponent_lbcc(structdw_mipi_dsi*dsi,+structdrm_display_mode*mode,u32hcomponent){u32frac,lbcc;lbcc=hcomponent*dsi->lane_mbps*MSEC_PER_SEC/8;-frac=lbcc%dsi->mode->clock;-lbcc=lbcc/dsi->mode->clock;+frac=lbcc%mode->clock;+lbcc=lbcc/mode->clock;if(frac)lbcc++;returnlbcc;}-staticvoiddw_mipi_dsi_line_timer_config(structdw_mipi_dsi*dsi)+staticvoiddw_mipi_dsi_line_timer_config(structdw_mipi_dsi*dsi,+structdrm_display_mode*mode){u32htotal,hsa,hbp,lbcc;-structdrm_display_mode*mode=dsi->mode;htotal=mode->htotal;hsa=mode->hsync_end-mode->hsync_start;hbp=mode->htotal-mode->hsync_end;-lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,htotal);+lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,mode,htotal);dsi_write(dsi,DSI_VID_HLINE_TIME,lbcc);-lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,hsa);+lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,mode,hsa);dsi_write(dsi,DSI_VID_HSA_TIME,lbcc);-lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,hbp);+lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,mode,hbp);dsi_write(dsi,DSI_VID_HBP_TIME,lbcc);}-staticvoiddw_mipi_dsi_vertical_timing_config(structdw_mipi_dsi*dsi)+staticvoiddw_mipi_dsi_vertical_timing_config(structdw_mipi_dsi*dsi,+structdrm_display_mode*mode){u32vactive,vsa,vfp,vbp;-structdrm_display_mode*mode=dsi->mode;vactive=mode->vdisplay;vsa=mode->vsync_end-mode->vsync_start;
From: John Keeping <hidden> Date: 2017-01-21 16:35:02
The multiplication ratio for the PLL is required to be even due to the
use of a "by 2 pre-scaler". Currently we are likely to end up with an
odd multiplier even though there is an equivalent set of parameters with
an even multiplier.
For example, using the 324MHz bit rate with a reference clock of 24MHz
we end up with M = 27, N = 2 whereas the example in the PHY databook
gives M = 54, N = 4 for this bit rate and reference clock.
By walking down through the available multiplier instead of up we are
more likely to hit an even multiplier. With the above example we do now
get M = 54, N = 4 as given by the databook.
While doing this, change the loop limits to encode the actual limits on
the divisor, which are:
40MHz >= (pllref / N) >= 5MHz
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: John Keeping <hidden> Date: 2017-01-21 16:35:12
Use the same calculation as the vendor kernel to derive the escape clock
speed.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: John Keeping <hidden> Date: 2017-01-21 16:35:22
This ensures that the output resolution is known before fbcon loads.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:35:32
These values are specified as constant time periods but the PHY
configuration is in terms of the current lane byte clock so using
constant values guarantees that the timings will be outside the
specification with some display configurations.
Derive the necessary configuration from the byte clock in order to
ensure that the PHY configuration is correct.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 36 ++++++++++++++++++++++++++++++----
1 file changed, 32 insertions(+), 4 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:35:42
Also don't power up the DSI host at this point since this is not
necessary in order to configure the PHY and we do so later when
selecting video or command mode.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
@@ -397,7 +397,10 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)returntestdin;}-dsi_write(dsi,DSI_PWR_UP,POWERUP);+/* Start by clearing PHY state */+dsi_write(dsi,DSI_PHY_TST_CTRL0,PHY_UNTESTCLR);+dsi_write(dsi,DSI_PHY_TST_CTRL0,PHY_TESTCLR);+dsi_write(dsi,DSI_PHY_TST_CTRL0,PHY_UNTESTCLR);dw_mipi_dsi_phy_write(dsi,0x10,BYPASS_VCO_RANGE|VCO_RANGE_CON_SEL(vco)|
From: John Keeping <hidden> Date: 2017-01-21 16:35:49
As a side-effect of this, encode the endianness explicitly rather than
casting a u16.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
@@ -572,8 +572,13 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)staticintdw_mipi_dsi_dcs_short_write(structdw_mipi_dsi*dsi,conststructmipi_dsi_msg*msg){-constu16*tx_buf=msg->tx_buf;-u32val=GEN_HDATA(*tx_buf)|GEN_HTYPE(msg->type);+constu8*tx_buf=msg->tx_buf;+u32val=GEN_HTYPE(msg->type);++if(msg->tx_len>0)+val|=GEN_HDATA(tx_buf[0]);+if(msg->tx_len>1)+val|=GEN_HDATA(tx_buf[1]<<8);if(msg->tx_len>2){dev_err(dsi->dev,"too long tx buf length %zu for short write\n",
From: John Keeping <hidden> Date: 2017-01-21 16:36:00
Some panels need to be configured with commands sent over the MIPI link,
which they will do in the prepare hook. Call this after the PHY has
been initialized so that we are able to send commands to the panel.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:36:07
Rockchip DRM is fully atomic and commit is deprecated for atomic
drivers. No changed are needed beyond renaming the function.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:36:48
With atomic modesetting the hardware will be powered off when the
mode_set function is called. We should configure the hardware in the
commit function (or even the enable function, but switching from commit
to enable is left for a future patch).
Signed-off-by: John Keeping <redacted>
---
v2:
- also move dw_mipi_dsi_get_lane_bps into the commit function
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 45 ++++++++++++++--------------------
1 file changed, 19 insertions(+), 26 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:36:58
Instead of always sending commands in LP mode, respect the
MIPI_DSI_MSG_USE_LPM flag to decide how to send each message. Also
request acks if MIPI_DSI_MSG_REQ_ACK is set.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
From: John Keeping <hidden> Date: 2017-01-21 16:37:15
In a couple of places here we use "val" for the value that is about to
be written to a register but then reuse the same variable for the value
of a status register before we get around to writing it. Rename the
value to be written to so that we write the value we intend to and not
what we have just read from the status register.
Signed-off-by: John Keeping <redacted>
Tested-by: Chris Zhong <redacted>
Reviewed-by: Chris Zhong <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
From: John Keeping <hidden> Date: 2017-01-21 16:37:27
I haven't found any method for getting the length of a response, so this
just uses the requested rx_len
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 54 ++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
From: Chris Zhong <hidden> Date: 2017-01-22 03:06:58
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
These values are specified as constant time periods but the PHY
configuration is in terms of the current lane byte clock so using
constant values guarantees that the timings will be outside the
specification with some display configurations.
Derive the necessary configuration from the byte clock in order to
ensure that the PHY configuration is correct.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 36 ++++++++++++++++++++++++++++++----
1 file changed, 32 insertions(+), 4 deletions(-)
From: Chris Zhong <hidden> Date: 2017-01-22 03:09:57
Hi John
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
I haven't found any method for getting the length of a response, so this
just uses the requested rx_len
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 54 ++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
From: Chris Zhong <hidden> Date: 2017-01-22 03:58:45
Hi John
On 01/22/2017 12:31 AM, John Keeping wrote:
With atomic modesetting the hardware will be powered off when the
mode_set function is called. We should configure the hardware in the
commit function (or even the enable function, but switching from commit
to enable is left for a future patch).
I tend to merge the 2 patches into one.
quoted hunk
Signed-off-by: John Keeping <redacted>
---
v2:
- also move dw_mipi_dsi_get_lane_bps into the commit function
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 45 ++++++++++++++--------------------
1 file changed, 19 insertions(+), 26 deletions(-)
From: Chris Zhong <hidden> Date: 2017-01-22 04:00:34
Hi John
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
This shows that we only use the mode from the enable function and
prepares us to remove the "mode" field and the mode_set hook in the next
commit.
Signed-off-by: John Keeping <redacted>
---
New in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 41 ++++++++++++++++++----------------
1 file changed, 22 insertions(+), 19 deletions(-)
@@ -330,11 +330,11 @@ static int max_mbps_to_testdin(unsigned int max_mbps)*Thecontrollershouldgenerate2framesbefore*preparingtheperipheral.*/-staticvoiddw_mipi_dsi_wait_for_two_frames(structdw_mipi_dsi*dsi)+staticvoiddw_mipi_dsi_wait_for_two_frames(structdrm_display_mode*mode){intrefresh,two_frames;-refresh=drm_mode_vrefresh(dsi->mode);+refresh=drm_mode_vrefresh(mode);two_frames=DIV_ROUND_UP(MSEC_PER_SEC,refresh)*2;msleep(two_frames);}
@@ -459,7 +459,8 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)returnret;}-staticintdw_mipi_dsi_get_lane_bps(structdw_mipi_dsi*dsi)+staticintdw_mipi_dsi_get_lane_bps(structdw_mipi_dsi*dsi,+structdrm_display_mode*mode){unsignedinti,pre;unsignedlongmpclk,pllref,tmp;
@@ -474,7 +475,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi)returnbpp;}-mpclk=DIV_ROUND_UP(dsi->mode->clock,MSEC_PER_SEC);+mpclk=DIV_ROUND_UP(mode->clock,MSEC_PER_SEC);if(mpclk){/* take 1 / 0.9, since mbps must big than bandwidth of RGB */tmp=mpclk*(bpp/dsi->lanes)*10/9;
@@ -742,43 +743,44 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)/* Get lane byte clock cycles. */staticu32dw_mipi_dsi_get_hcomponent_lbcc(structdw_mipi_dsi*dsi,+structdrm_display_mode*mode,u32hcomponent){u32frac,lbcc;lbcc=hcomponent*dsi->lane_mbps*MSEC_PER_SEC/8;-frac=lbcc%dsi->mode->clock;-lbcc=lbcc/dsi->mode->clock;+frac=lbcc%mode->clock;+lbcc=lbcc/mode->clock;if(frac)lbcc++;returnlbcc;}-staticvoiddw_mipi_dsi_line_timer_config(structdw_mipi_dsi*dsi)+staticvoiddw_mipi_dsi_line_timer_config(structdw_mipi_dsi*dsi,+structdrm_display_mode*mode){u32htotal,hsa,hbp,lbcc;-structdrm_display_mode*mode=dsi->mode;htotal=mode->htotal;hsa=mode->hsync_end-mode->hsync_start;hbp=mode->htotal-mode->hsync_end;-lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,htotal);+lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,mode,htotal);dsi_write(dsi,DSI_VID_HLINE_TIME,lbcc);-lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,hsa);+lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,mode,hsa);dsi_write(dsi,DSI_VID_HSA_TIME,lbcc);-lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,hbp);+lbcc=dw_mipi_dsi_get_hcomponent_lbcc(dsi,mode,hbp);dsi_write(dsi,DSI_VID_HBP_TIME,lbcc);}-staticvoiddw_mipi_dsi_vertical_timing_config(structdw_mipi_dsi*dsi)+staticvoiddw_mipi_dsi_vertical_timing_config(structdw_mipi_dsi*dsi,+structdrm_display_mode*mode){u32vactive,vsa,vfp,vbp;-structdrm_display_mode*mode=dsi->mode;vactive=mode->vdisplay;vsa=mode->vsync_end-mode->vsync_start;
From: Chris Zhong <hidden> Date: 2017-01-22 06:18:22
Hi John
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
This is not needed since we can access the mode via the CRTC from the
enable hook. Also remove the "mode" field that is no longer used.
Signed-off-by: John Keeping <redacted>
---
New in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
From: Chris Zhong <hidden> Date: 2017-01-22 06:24:29
Hi John
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
We want to check that both the GEN_CMD_EMPTY and GEN_PLD_W_EMPTY bits
are set so we can't just check "val & mask" because that will be true if
either bit is set.
According to DW mipi dsi controller databook, you are right. we should
check both the 2 BIT.
gen_pld_w_empty:
This bit indicates the empty status of the generic write payload FIFO.
Dependency : DSI_GENERIC = 1. Otherwise, this bit is reserved.
Value after reset : 0x1
quoted hunk
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
From: Chris Zhong <hidden> Date: 2017-01-22 06:43:38
Hi John
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
As a side-effect of this, encode the endianness explicitly rather than
casting a u16.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
@@ -572,8 +572,13 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)staticintdw_mipi_dsi_dcs_short_write(structdw_mipi_dsi*dsi,conststructmipi_dsi_msg*msg){-constu16*tx_buf=msg->tx_buf;-u32val=GEN_HDATA(*tx_buf)|GEN_HTYPE(msg->type);+constu8*tx_buf=msg->tx_buf;+u32val=GEN_HTYPE(msg->type);++if(msg->tx_len>0)+val|=GEN_HDATA(tx_buf[0]);+if(msg->tx_len>1)+val|=GEN_HDATA(tx_buf[1]<<8);if(msg->tx_len>2){dev_err(dsi->dev,"too long tx buf length %zu for short write\n",
From: Chris Zhong <hidden> Date: 2017-01-22 06:45:19
Hi John
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
As an aid to debugging.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Chris Zhong <hidden> Date: 2017-01-22 07:14:25
Hi John
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
Instead of always sending commands in LP mode, respect the
MIPI_DSI_MSG_USE_LPM flag to decide how to send each message. Also
request acks if MIPI_DSI_MSG_REQ_ACK is set.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
From: Chris Zhong <hidden> Date: 2017-01-22 08:11:36
Hi John
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
Requesting the HS clock from the PHY before we initialize it causes an
invalid signal to be sent out since the input clock is not yet
configured. The PHY databook suggests only asserting this signal when
performing HS transfers, so let's do that.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
From: Chris Zhong <hidden> Date: 2017-01-22 08:17:02
Hi John
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
By dereferencing the MIPI command buffer as a u32* we rely on it being
correctly aligned on ARM, but this may not be the case. Copy it into a
stack variable that will be correctly aligned.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
From: Chris Zhong <hidden> Date: 2017-01-22 08:22:24
Hi John
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
Some panels need to be configured with commands sent over the MIPI link,
which they will do in the prepare hook. Call this after the PHY has
been initialized so that we are able to send commands to the panel.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
From: Chris Zhong <hidden> Date: 2017-01-22 08:37:53
Hi John
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
Panel drivers may want to sent commands during the disable function, for
example MIPI_DCS_SET_DISPLAY_OFF before the video signal ends. In order
to send commands we need to write to registers, so pclk must be enabled.
While changing this, remove the unnecessary code after the panel
unprepare call which seems to be a workaround for a specific panel and
thus belongs in the panel driver.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
From: Chris Zhong <hidden> Date: 2017-01-22 09:37:48
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
Also don't power up the DSI host at this point since this is not
necessary in order to configure the PHY and we do so later when
selecting video or command mode.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
@@ -397,7 +397,10 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)returntestdin;}-dsi_write(dsi,DSI_PWR_UP,POWERUP);+/* Start by clearing PHY state */+dsi_write(dsi,DSI_PHY_TST_CTRL0,PHY_UNTESTCLR);+dsi_write(dsi,DSI_PHY_TST_CTRL0,PHY_TESTCLR);+dsi_write(dsi,DSI_PHY_TST_CTRL0,PHY_UNTESTCLR);dw_mipi_dsi_phy_write(dsi,0x10,BYPASS_VCO_RANGE|VCO_RANGE_CON_SEL(vco)|
From: Chris Zhong <hidden> Date: 2017-01-22 09:37:51
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
Use the same calculation as the vendor kernel to derive the escape clock
speed.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Chris Zhong <hidden> Date: 2017-01-22 10:10:08
Hi John
This patch do the similar thing with
https://patchwork.kernel.org/patch/9530405/
They are changing the phy configuration order, my suggestion is to merge
them.
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Chris Zhong <hidden> Date: 2017-01-23 00:50:00
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
As the documentation for readx_poll_timeout says, we want to use the
specialized macro for readl rather than using the generic version
directly.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
@@ -471,14 +471,14 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)PHY_UNRSTZ|PHY_UNSHUTDOWNZ);-ret=readx_poll_timeout(readl,dsi->base+DSI_PHY_STATUS,+ret=readl_poll_timeout(dsi->base+DSI_PHY_STATUS,val,val&LOCK,1000,PHY_STATUS_TIMEOUT_US);if(ret<0){dev_err(dsi->dev,"failed to wait for phy lock state\n");returnret;}-ret=readx_poll_timeout(readl,dsi->base+DSI_PHY_STATUS,+ret=readl_poll_timeout(dsi->base+DSI_PHY_STATUS,val,val&STOP_STATE_CLK_LANE,1000,PHY_STATUS_TIMEOUT_US);if(ret<0){
From: Chris Zhong <hidden> Date: 2017-01-23 01:39:44
Hi John
On 01/22/2017 12:31 AM, John Keeping wrote:
The multiplication ratio for the PLL is required to be even due to the
use of a "by 2 pre-scaler". Currently we are likely to end up with an
odd multiplier even though there is an equivalent set of parameters with
an even multiplier.
For example, using the 324MHz bit rate with a reference clock of 24MHz
we end up with M = 27, N = 2 whereas the example in the PHY databook
gives M = 54, N = 4 for this bit rate and reference clock.
By walking down through the available multiplier instead of up we are
more likely to hit an even multiplier. With the above example we do now
get M = 54, N = 4 as given by the databook.
While doing this, change the loop limits to encode the actual limits on
the divisor, which are:
40MHz >= (pllref / N) >= 5MHz
This formula is limit for N, but we still can not guarantee to get an
even M.
Do you think we should do a check for M.
such as:
if (m % 2)
continue;
...
for (i = pllref / 5; i > (pllref / 40); i--) {
pre = pllref / i;
if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
tmp = target_mbps % pre;
n = i;
m = target_mbps / pre;
if (m % 2)
continue;
}
if (tmp == 0)
break;
}
if (m % 2)
m++;
dsi->lane_mbps = pllref / n * m;
dsi->input_div = n;
dsi->feedback_div = m;
quoted hunk
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Chris Zhong <hidden> Date: 2017-01-23 06:13:06
Reviewed-by: Chris Zhong <redacted>
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted hunk
In order to fully reset the state of the MIPI controller we must assert
this reset.
This is slightly more complicated than it could be in order to maintain
compatibility with device trees that do not specify the reset property.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
@@ -1159,6 +1161,34 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master,returnret;}+/*+*Notethattheresetwasnotdefinedintheinitialdevicetree,so+*wehavetobepreparedforitnotbeingfound.+*/+apb_rst=devm_reset_control_get(dev,"apb");+if(IS_ERR(apb_rst)){+if(PTR_ERR(apb_rst)==-ENODEV){+apb_rst=NULL;+}else{+dev_err(dev,"Unable to get reset control: %d\n",ret);+returnPTR_ERR(apb_rst);+}+}++if(apb_rst){+ret=clk_prepare_enable(dsi->pclk);+if(ret){+dev_err(dev,"%s: Failed to enable pclk\n",__func__);+returnret;+}++reset_control_assert(apb_rst);+usleep_range(10,20);+reset_control_deassert(apb_rst);++clk_disable_unprepare(dsi->pclk);+}+ret=clk_prepare_enable(dsi->pllref_clk);if(ret){dev_err(dev,"%s: Failed to enable pllref_clk\n",__func__);
When connected to the MIPI DSI output, we need to use N{H,V}SYNC for the
internal connection but these flags are meaningless for DSI panels.
Switch the test so that we do not set the P{H,V}SYNC bits unless the
mode requires it.
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
I'm confuse that why SYNC flags have N and P on drm, they are same meaning.
If no one configure display mode's flags, I don't know which one is
correct, N or P? it's a problem.
Does anyone can answer it?
For this patch, it may effect non-sync flags mode on other connector's
behavior,
but seems mostly display mode has sync flags except mipi dsi connector,
I think feed mipi's requirement would be better.
So it's no problem on my side.
Reviewed-by: Mark Yao <redacted>
From: John Keeping <hidden> Date: 2017-01-23 13:07:35
Hi Chris,
On Mon, 23 Jan 2017 09:38:54 +0800, Chris Zhong wrote:
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted
The multiplication ratio for the PLL is required to be even due to the
use of a "by 2 pre-scaler". Currently we are likely to end up with an
odd multiplier even though there is an equivalent set of parameters with
an even multiplier.
For example, using the 324MHz bit rate with a reference clock of 24MHz
we end up with M = 27, N = 2 whereas the example in the PHY databook
gives M = 54, N = 4 for this bit rate and reference clock.
By walking down through the available multiplier instead of up we are
more likely to hit an even multiplier. With the above example we do now
get M = 54, N = 4 as given by the databook.
While doing this, change the loop limits to encode the actual limits on
the divisor, which are:
40MHz >= (pllref / N) >= 5MHz
This formula is limit for N, but we still can not guarantee to get an
even M.
Do you think we should do a check for M.
such as:
if (m % 2)
continue;
...
for (i = pllref / 5; i > (pllref / 40); i--) {
pre = pllref / i;
if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
tmp = target_mbps % pre;
n = i;
m = target_mbps / pre;
if (m % 2)
continue;
}
if (tmp == 0)
break;
}
if (m % 2)
m++;
dsi->lane_mbps = pllref / n * m;
dsi->input_div = n;
dsi->feedback_div = m;
Yes, I agree that there should be a check for M, but I'm not sure if
the version above is sufficient. The "m % 2" check inside the loop
means that we don't break immediately when tmp=0 but then we are
guaranteed to break next time without having modified n, m because now
tmp=0 so "tmp > (target_mbps % pre)" is always false and we just hit the
"if (tmp == 0) break" case next time.
Given that the descending loop already means that if we can hit "tmp"
exactly we are more likely to do so with a bigger N and even M, I think
it might be better to just fix M after the loop like:
if (m % 2) {
if (m < 256 && (n * 2) <= (pllref / 5)) {
n *= 2;
m *= 2;
} else {
m++;
}
}
but I haven't thought about this too carefully.
For this series, I'd rather either keep this patch as it is or drop it
in favour of a more comprehensive solution. I don't want to block the
other fixes waiting for a perfect fix here and we can always improve
this further with a follow-up patch.
quoted
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Chris Zhong <hidden> Date: 2017-01-24 02:43:50
On 01/23/2017 08:49 PM, John Keeping wrote:
Hi Chris,
On Mon, 23 Jan 2017 09:38:54 +0800, Chris Zhong wrote:
quoted
On 01/22/2017 12:31 AM, John Keeping wrote:
quoted
The multiplication ratio for the PLL is required to be even due to the
use of a "by 2 pre-scaler". Currently we are likely to end up with an
odd multiplier even though there is an equivalent set of parameters with
an even multiplier.
For example, using the 324MHz bit rate with a reference clock of 24MHz
we end up with M = 27, N = 2 whereas the example in the PHY databook
gives M = 54, N = 4 for this bit rate and reference clock.
By walking down through the available multiplier instead of up we are
more likely to hit an even multiplier. With the above example we do now
get M = 54, N = 4 as given by the databook.
While doing this, change the loop limits to encode the actual limits on
the divisor, which are:
40MHz >= (pllref / N) >= 5MHz
This formula is limit for N, but we still can not guarantee to get an
even M.
Do you think we should do a check for M.
such as:
if (m % 2)
continue;
...
for (i = pllref / 5; i > (pllref / 40); i--) {
pre = pllref / i;
if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
tmp = target_mbps % pre;
n = i;
m = target_mbps / pre;
if (m % 2)
continue;
}
if (tmp == 0)
break;
}
if (m % 2)
m++;
dsi->lane_mbps = pllref / n * m;
dsi->input_div = n;
dsi->feedback_div = m;
Yes, I agree that there should be a check for M, but I'm not sure if
the version above is sufficient. The "m % 2" check inside the loop
means that we don't break immediately when tmp=0 but then we are
guaranteed to break next time without having modified n, m because now
tmp=0 so "tmp > (target_mbps % pre)" is always false and we just hit the
"if (tmp == 0) break" case next time.
Given that the descending loop already means that if we can hit "tmp"
exactly we are more likely to do so with a bigger N and even M, I think
it might be better to just fix M after the loop like:
if (m % 2) {
if (m < 256 && (n * 2) <= (pllref / 5)) {
n *= 2;
m *= 2;
} else {
m++;
}
}
but I haven't thought about this too carefully.
For this series, I'd rather either keep this patch as it is or drop it
in favour of a more comprehensive solution. I don't want to block the
other fixes waiting for a perfect fix here and we can always improve
this further with a follow-up patch.
Agree, We can improve the whole formula in the future.
quoted
quoted
Signed-off-by: John Keeping <redacted>
---
Unchanged in v2
---
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)