Hi Tomi,
Thank you for the patch.
On Tue, Nov 24, 2020 at 02:45:37PM +0200, Tomi Valkeinen wrote:
> ULPS doesn't work, and I have been unable to get it to work. As ULPS is
> a minor power-saving feature which requires substantial amount of
> non-trivial code, and we have trouble just getting and
> keeping DSI working at all, remove ULPS support.
>
> When the DSI driver works reliably for command and video mode displays,
> someone interested can add it back.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/omapdrm/dss/dsi.c | 297 +-----------------------------
> drivers/gpu/drm/omapdrm/dss/dsi.h | 4 -
> 2 files changed, 8 insertions(+), 293 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index 6e9c99402540..ffecacd7350a 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -53,8 +53,6 @@
> #define REG_FLD_MOD(dsi, idx, val, start, end) \
> dsi_write_reg(dsi, idx, FLD_MOD(dsi_read_reg(dsi, idx), val, start, end))
>
> -static void dsi_set_ulps_auto(struct dsi_data *dsi, bool enable);
> -
> static int dsi_init_dispc(struct dsi_data *dsi);
> static void dsi_uninit_dispc(struct dsi_data *dsi);
>
> @@ -688,44 +686,6 @@ static int dsi_unregister_isr_vc(struct dsi_data *dsi, int vc,
> return r;
> }
>
> -static int dsi_register_isr_cio(struct dsi_data *dsi, omap_dsi_isr_t isr,
> - void *arg, u32 mask)
> -{
> - unsigned long flags;
> - int r;
> -
> - spin_lock_irqsave(&dsi->irq_lock, flags);
> -
> - r = _dsi_register_isr(isr, arg, mask, dsi->isr_tables.isr_table_cio,
> - ARRAY_SIZE(dsi->isr_tables.isr_table_cio));
> -
> - if (r == 0)
> - _omap_dsi_set_irqs_cio(dsi);
> -
> - spin_unlock_irqrestore(&dsi->irq_lock, flags);
> -
> - return r;
> -}
> -
> -static int dsi_unregister_isr_cio(struct dsi_data *dsi, omap_dsi_isr_t isr,
> - void *arg, u32 mask)
> -{
> - unsigned long flags;
> - int r;
> -
> - spin_lock_irqsave(&dsi->irq_lock, flags);
> -
> - r = _dsi_unregister_isr(isr, arg, mask, dsi->isr_tables.isr_table_cio,
> - ARRAY_SIZE(dsi->isr_tables.isr_table_cio));
> -
> - if (r == 0)
> - _omap_dsi_set_irqs_cio(dsi);
> -
> - spin_unlock_irqrestore(&dsi->irq_lock, flags);
> -
> - return r;
> -}
> -
> static u32 dsi_get_errors(struct dsi_data *dsi)
> {
> unsigned long flags;
> @@ -1450,56 +1410,6 @@ static void dsi_cio_timings(struct dsi_data *dsi)
> dsi_write_reg(dsi, DSI_DSIPHY_CFG2, r);
> }
>
> -/* lane masks have lane 0 at lsb. mask_p for positive lines, n for negative */
> -static void dsi_cio_enable_lane_override(struct dsi_data *dsi,
> - unsigned int mask_p,
> - unsigned int mask_n)
> -{
> - int i;
> - u32 l;
> - u8 lptxscp_start = dsi->num_lanes_supported == 3 ? 22 : 26;
> -
> - l = 0;
> -
> - for (i = 0; i < dsi->num_lanes_supported; ++i) {
> - unsigned int p = dsi->lanes[i].polarity;
> -
> - if (mask_p & (1 << i))
> - l |= 1 << (i * 2 + (p ? 0 : 1));
> -
> - if (mask_n & (1 << i))
> - l |= 1 << (i * 2 + (p ? 1 : 0));
> - }
> -
> - /*
> - * Bits in REGLPTXSCPDAT4TO0DXDY:
> - * 17: DY0 18: DX0
> - * 19: DY1 20: DX1
> - * 21: DY2 22: DX2
> - * 23: DY3 24: DX3
> - * 25: DY4 26: DX4
> - */
> -
> - /* Set the lane override configuration */
> -
> - /* REGLPTXSCPDAT4TO0DXDY */
> - REG_FLD_MOD(dsi, DSI_DSIPHY_CFG10, l, lptxscp_start, 17);
> -
> - /* Enable lane override */
> -
> - /* ENLPTXSCPDAT */
> - REG_FLD_MOD(dsi, DSI_DSIPHY_CFG10, 1, 27, 27);
> -}
> -
> -static void dsi_cio_disable_lane_override(struct dsi_data *dsi)
> -{
> - /* Disable lane override */
> - REG_FLD_MOD(dsi, DSI_DSIPHY_CFG10, 0, 27, 27); /* ENLPTXSCPDAT */
> - /* Reset the lane override configuration */
> - /* REGLPTXSCPDAT4TO0DXDY */
> - REG_FLD_MOD(dsi, DSI_DSIPHY_CFG10, 0, 22, 17);
> -}
> -
> static int dsi_cio_wait_tx_clk_esc_reset(struct dsi_data *dsi)
> {
> int t, i;
> @@ -1674,32 +1584,6 @@ static int dsi_cio_init(struct dsi_data *dsi)
> l = FLD_MOD(l, 0x1fff, 12, 0); /* STOP_STATE_COUNTER_IO */
> dsi_write_reg(dsi, DSI_TIMING1, l);
>
> - if (dsi->ulps_enabled) {
> - unsigned int mask_p;
> - int i;
> -
> - DSSDBG("manual ulps exit\n");
> -
> - /* ULPS is exited by Mark-1 state for 1ms, followed by
> - * stop state. DSS HW cannot do this via the normal
> - * ULPS exit sequence, as after reset the DSS HW thinks
> - * that we are not in ULPS mode, and refuses to send the
> - * sequence. So we need to send the ULPS exit sequence
> - * manually by setting positive lines high and negative lines
> - * low for 1ms.
> - */
> -
> - mask_p = 0;
> -
> - for (i = 0; i < dsi->num_lanes_supported; ++i) {
> - if (dsi->lanes[i].function == DSI_LANE_UNUSED)
> - continue;
> - mask_p |= 1 << i;
> - }
> -
> - dsi_cio_enable_lane_override(dsi, mask_p, 0);
> - }
> -
> r = dsi_cio_power(dsi, DSI_COMPLEXIO_POWER_ON);
> if (r)
> goto err_cio_pwr;
> @@ -1718,17 +1602,6 @@ static int dsi_cio_init(struct dsi_data *dsi)
> if (r)
> goto err_tx_clk_esc_rst;
>
> - if (dsi->ulps_enabled) {
> - /* Keep Mark-1 state for 1ms (as per DSI spec) */
> - ktime_t wait = ns_to_ktime(1000 * 1000);
> - set_current_state(TASK_UNINTERRUPTIBLE);
> - schedule_hrtimeout(&wait, HRTIMER_MODE_REL);
> -
> - /* Disable the override. The lanes should be set to Mark-11
> - * state by the HW */
> - dsi_cio_disable_lane_override(dsi);
> - }
> -
> /* FORCE_TX_STOP_MODE_IO */
> REG_FLD_MOD(dsi, DSI_TIMING1, 0, 15, 15);
>
> @@ -1739,8 +1612,6 @@ static int dsi_cio_init(struct dsi_data *dsi)
> !(dsi->dsidev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS),
> 13, 13);
>
> - dsi->ulps_enabled = false;
> -
> DSSDBG("CIO init done\n");
>
> return 0;
> @@ -1750,8 +1621,6 @@ static int dsi_cio_init(struct dsi_data *dsi)
> err_cio_pwr_dom:
> dsi_cio_power(dsi, DSI_COMPLEXIO_POWER_OFF);
> err_cio_pwr:
> - if (dsi->ulps_enabled)
> - dsi_cio_disable_lane_override(dsi);
> err_scp_clk_dom:
> dsi_disable_scp_clk(dsi);
> dsi_disable_pads(dsi);
> @@ -2522,99 +2391,6 @@ static int dsi_vc_generic_read(struct omap_dss_device *dssdev, int vc,
> return r;
> }
>
> -static int dsi_enter_ulps(struct dsi_data *dsi)
> -{
> - DECLARE_COMPLETION_ONSTACK(completion);
> - int r, i;
> - unsigned int mask;
> -
> - DSSDBG("Entering ULPS");
> -
> - WARN_ON(!dsi_bus_is_locked(dsi));
> -
> - WARN_ON(dsi->ulps_enabled);
> -
> - if (dsi->ulps_enabled)
> - return 0;
> -
> - /* DDR_CLK_ALWAYS_ON */
> - if (REG_GET(dsi, DSI_CLK_CTRL, 13, 13)) {
> - dsi_if_enable(dsi, 0);
> - REG_FLD_MOD(dsi, DSI_CLK_CTRL, 0, 13, 13);
> - dsi_if_enable(dsi, 1);
> - }
> -
> - dsi_sync_vc(dsi, 0);
> - dsi_sync_vc(dsi, 1);
> - dsi_sync_vc(dsi, 2);
> - dsi_sync_vc(dsi, 3);
> -
> - dsi_force_tx_stop_mode_io(dsi);
> -
> - dsi_vc_enable(dsi, 0, false);
> - dsi_vc_enable(dsi, 1, false);
> - dsi_vc_enable(dsi, 2, false);
> - dsi_vc_enable(dsi, 3, false);
> -
> - if (REG_GET(dsi, DSI_COMPLEXIO_CFG2, 16, 16)) { /* HS_BUSY */
> - DSSERR("HS busy when enabling ULPS\n");
> - return -EIO;
> - }
> -
> - if (REG_GET(dsi, DSI_COMPLEXIO_CFG2, 17, 17)) { /* LP_BUSY */
> - DSSERR("LP busy when enabling ULPS\n");
> - return -EIO;
> - }
> -
> - r = dsi_register_isr_cio(dsi, dsi_completion_handler, &completion,
> - DSI_CIO_IRQ_ULPSACTIVENOT_ALL0);
> - if (r)
> - return r;
> -
> - mask = 0;
> -
> - for (i = 0; i < dsi->num_lanes_supported; ++i) {
> - if (dsi->lanes[i].function == DSI_LANE_UNUSED)
> - continue;
> - mask |= 1 << i;
> - }
> - /* Assert TxRequestEsc for data lanes and TxUlpsClk for clk lane */
> - /* LANEx_ULPS_SIG2 */
> - REG_FLD_MOD(dsi, DSI_COMPLEXIO_CFG2, mask, 9, 5);
> -
> - /* flush posted write and wait for SCP interface to finish the write */
> - dsi_read_reg(dsi, DSI_COMPLEXIO_CFG2);
> -
> - if (wait_for_completion_timeout(&completion,
> - msecs_to_jiffies(1000)) == 0) {
> - DSSERR("ULPS enable timeout\n");
> - r = -EIO;
> - goto err;
> - }
> -
> - dsi_unregister_isr_cio(dsi, dsi_completion_handler, &completion,
> - DSI_CIO_IRQ_ULPSACTIVENOT_ALL0);
> -
> - /* Reset LANEx_ULPS_SIG2 */
> - REG_FLD_MOD(dsi, DSI_COMPLEXIO_CFG2, 0, 9, 5);
> -
> - /* flush posted write and wait for SCP interface to finish the write */
> - dsi_read_reg(dsi, DSI_COMPLEXIO_CFG2);
> -
> - dsi_cio_power(dsi, DSI_COMPLEXIO_POWER_ULPS);
> -
> - dsi_if_enable(dsi, false);
> -
> - dsi->ulps_enabled = true;
> -
> - return 0;
> -
> -err:
> - dsi_unregister_isr_cio(dsi, dsi_completion_handler, &completion,
> - DSI_CIO_IRQ_ULPSACTIVENOT_ALL0);
> - return r;
> -}
> -
> static void dsi_set_lp_rx_timeout(struct dsi_data *dsi, unsigned int ticks,
> bool x4, bool x16)
> {
> @@ -3397,7 +3173,6 @@ static void dsi_handle_framedone(struct dsi_data *dsi, int error)
> REG_FLD_MOD(dsi, DSI_TIMING2, 1, 15, 15); /* LP_RX_TO */
> }
>
> - dsi_set_ulps_auto(dsi, true);
> dsi_bus_unlock(dsi);
>
> if (!error)
> @@ -3488,8 +3263,6 @@ static int dsi_update_channel(struct omap_dss_device *dssdev, int vc)
>
> DSSDBG("dsi_update_channel: %d", vc);
>
> - dsi_set_ulps_auto(dsi, false);
> -
> r = _dsi_send_nop(dsi, VC_CMD, dsi->dsidev->channel);
> if (r < 0) {
> DSSWARN("failed to send nop between frames: %d\n", r);
> @@ -3509,7 +3282,6 @@ static int dsi_update_channel(struct omap_dss_device *dssdev, int vc)
> return 0;
>
> err:
> - dsi_set_ulps_auto(dsi, true);
> dsi_bus_unlock(dsi);
> return r;
> }
> @@ -3702,12 +3474,8 @@ static int dsi_init_dsi(struct dsi_data *dsi)
> return r;
> }
>
> -static void dsi_uninit_dsi(struct dsi_data *dsi, bool disconnect_lanes,
> - bool enter_ulps)
> +static void dsi_uninit_dsi(struct dsi_data *dsi)
> {
> - if (enter_ulps && !dsi->ulps_enabled)
> - dsi_enter_ulps(dsi);
> -
> /* disable interface */
> dsi_if_enable(dsi, 0);
> dsi_vc_enable(dsi, 0, 0);
> @@ -3719,10 +3487,8 @@ static void dsi_uninit_dsi(struct dsi_data *dsi, bool disconnect_lanes,
> dsi_cio_uninit(dsi);
> dss_pll_disable(&dsi->pll);
>
> - if (disconnect_lanes) {
> - regulator_disable(dsi->vdds_dsi_reg);
> - dsi->vdds_dsi_enabled = false;
> - }
> + regulator_disable(dsi->vdds_dsi_reg);
> + dsi->vdds_dsi_enabled = false;
> }
>
> static void dsi_enable(struct dsi_data *dsi)
> @@ -3754,8 +3520,7 @@ static void dsi_enable(struct dsi_data *dsi)
> DSSDBG("dsi_enable FAILED\n");
> }
>
> -static void dsi_disable(struct dsi_data *dsi,
> - bool disconnect_lanes, bool enter_ulps)
> +static void dsi_disable(struct dsi_data *dsi)
> {
> WARN_ON(!dsi_bus_is_locked(dsi));
>
> @@ -3766,7 +3531,7 @@ static void dsi_disable(struct dsi_data *dsi,
> dsi_sync_vc(dsi, 2);
> dsi_sync_vc(dsi, 3);
>
> - dsi_uninit_dsi(dsi, disconnect_lanes, enter_ulps);
> + dsi_uninit_dsi(dsi);
>
> dsi_runtime_put(dsi);
>
> @@ -3787,42 +3552,6 @@ static int dsi_enable_te(struct dsi_data *dsi, bool enable)
> return 0;
> }
>
> -static void omap_dsi_ulps_work_callback(struct work_struct *work)
> -{
> - struct dsi_data *dsi = container_of(work, struct dsi_data,
> - ulps_work.work);
> -
> - dsi_bus_lock(dsi);
> -
> - dsi_enable_te(dsi, false);
> -
> - dsi_disable(dsi, false, true);
> -
> - dsi_bus_unlock(dsi);
> -}
> -
> -static void dsi_set_ulps_auto(struct dsi_data *dsi, bool enable)
> -{
> - WARN_ON(!dsi_bus_is_locked(dsi));
> -
> - if (!dsi->ulps_auto_idle)
> - return;
> -
> - if (enable) {
> - schedule_delayed_work(&dsi->ulps_work, msecs_to_jiffies(250));
> - } else {
> - cancel_delayed_work_sync(&dsi->ulps_work);
> -
> - if (!dsi->ulps_enabled)
> - return;
> -
> - dsi_bus_lock(dsi);
> - dsi_enable(dsi);
> - dsi_enable_te(dsi, true);
> - dsi_bus_unlock(dsi);
> - }
> -}
> -
> #ifdef PRINT_VERBOSE_VM_TIMINGS
> static void print_dsi_vm(const char *str,
> const struct omap_dss_dsi_videomode_timings *t)
> @@ -4494,13 +4223,10 @@ static ssize_t omap_dsi_host_transfer(struct mipi_dsi_host *host,
>
> dsi_bus_lock(dsi);
>
> - if (dsi->video_enabled) {
> - dsi_set_ulps_auto(dsi, false);
> + if (dsi->video_enabled)
> r = _omap_dsi_host_transfer(dsi, vc, msg);
> - dsi_set_ulps_auto(dsi, true);
> - } else {
> + else
> r = -EIO;
> - }
>
> dsi_bus_unlock(dsi);
>
> @@ -4642,9 +4368,6 @@ static int omap_dsi_host_attach(struct mipi_dsi_host *host,
> dsi->dsidev = client;
> dsi->pix_fmt = client->format;
>
> - INIT_DEFERRABLE_WORK(&dsi->ulps_work,
> - omap_dsi_ulps_work_callback);
> -
> dsi->config.hs_clk_min = 150000000; // TODO: get from client?
> dsi->config.hs_clk_max = client->hs_rate;
> dsi->config.lp_clk_min = 7000000; // TODO: get from client?
> @@ -4657,8 +4380,6 @@ static int omap_dsi_host_attach(struct mipi_dsi_host *host,
> else
> dsi->config.trans_mode = OMAP_DSS_DSI_EVENT_MODE;
>
> - dsi->ulps_auto_idle = false;
> -
> return 0;
> }
>
> @@ -4913,8 +4634,6 @@ static void dsi_bridge_enable(struct drm_bridge *bridge)
>
> dsi->video_enabled = true;
>
> - dsi_set_ulps_auto(dsi, true);
> -
> dsi_bus_unlock(dsi);
> }
>
> @@ -4929,7 +4648,7 @@ static void dsi_bridge_disable(struct drm_bridge *bridge)
>
> dsi_disable_video_output(dssdev, VC_VIDEO);
>
> - dsi_disable(dsi, true, false);
> + dsi_disable(dsi);
>
> dsi_bus_unlock(dsi);
> }
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.h b/drivers/gpu/drm/omapdrm/dss/dsi.h
> index 3543828e30eb..452cee3279db 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.h
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.h
> @@ -391,12 +391,8 @@ struct dsi_data {
> atomic_t do_ext_te_update;
>
> bool te_enabled;
> - bool ulps_enabled;
> - bool ulps_auto_idle;
> bool video_enabled;
>
> - struct delayed_work ulps_work;
> -
> struct delayed_work framedone_timeout_work;
>
> #ifdef DSI_CATCH_MISSING_TE
--
Regards,
Laurent Pinchart
