[PATCH] drm/dp: add drm_dp_link_power_down() helper

[Jani Nikula]

On Tue, 02 Dec 2014, Rob Clark <robdclark at gmail.com> wrote:
> We had _power_up(), but drivers also need to be able to power down.

Patch looks good, I'm just hijacking the thread to talk about the
_power_up() counterpart. Sorry. ;)

First, I'm not sure it's all right or sensible to read DP_SET_POWER
first when the sink is sleeping. Shouldn't it just write DP_SET_POWER_D0
to DP_SET_POWER?

Second, I think _power_up() should retry the wake up to ensure an AUX
reply. (If we decide it's all right to read DP_SET_POWER first, I think
it should have a retry too.)

DP v1.2 section 5.1.5 (section 5.2.5 is also relevant):

"""
The Source device must write the value of 1h to DPCD Address 600h of the
downstream device via AUX CH to switch the uPacket RX of the downstream
device out of power-save mode. A uPacket RX of a downstream device in a
power-save mode may not reply to this AUX request transaction. The
uPacket RX of a downstream device in a power-save mode for an open,
box-to-box connection is allowed to take up to 1ms till it is ready to
reply to the AUX request transaction. Therefore, the Source device must
retry until the 1ms wake-up timeout period of the Sink device expires.
"""

This also means that _probe() probably should not be used on sinks that
are in sleep. Therefore _power_up() after _probe() seems redundant too
(one example in tegra_output_sor_enable).

I seem to have reviewed the _power_up() and _probe() patch, so you can
blame me just as well...


BR,
Jani.

>
> Signed-off-by: Rob Clark <robdclark at gmail.com>
> ---
> This lets me drop edp_sink_power_state() from msm eDP code, and use
> the helpers instead.
>
>  drivers/gpu/drm/drm_dp_helper.c | 31 +++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     |  1 +
>  2 files changed, 32 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 959e207..82122ec 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -353,6 +353,37 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
>  EXPORT_SYMBOL(drm_dp_link_power_up);
>  
>  /**
> + * drm_dp_link_power_down() - power down a DisplayPort link
> + * @aux: DisplayPort AUX channel
> + * @link: pointer to a structure containing the link configuration
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link)
> +{
> +	u8 value;
> +	int err;
> +
> +	/* DP_SET_POWER register is only available on DPCD v1.1 and later */
> +	if (link->revision < 0x11)
> +		return 0;
> +
> +	err = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value);
> +	if (err < 0)
> +		return err;
> +
> +	value &= ~DP_SET_POWER_MASK;
> +	value |= DP_SET_POWER_D3;
> +
> +	err = drm_dp_dpcd_writeb(aux, DP_SET_POWER, value);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_link_power_down);
> +
> +/**
>   * drm_dp_link_configure() - configure a DisplayPort link
>   * @aux: DisplayPort AUX channel
>   * @link: pointer to a structure containing the link configuration
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 11f8c84..7e25030 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -586,6 +586,7 @@ struct drm_dp_link {
>  
>  int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
> +int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  
>  int drm_dp_aux_register(struct drm_dp_aux *aux);
> -- 
> 1.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center