Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 11 May 2018 10:35:18 -0400
From:   Sean Paul <seanpaul@chromium.org>
To:     Lin Huang <hl@rock-chips.com>
Cc:     seanpaul@chromium.org, airlied@linux.ie, zyw@rock-chips.com,
        dianders@chromium.org, briannorris@chromium.org,
        linux-rockchip@lists.infradead.org, heiko@sntech.de,
        daniel.vetter@intel.com, jani.nikula@linux.intel.com,
        dri-devel@lists.freedesktop.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        eballetbo@gmail.com
Subject: Re: [PATCH v2 1/4] drm/rockchip: add transfer function for cdn-dp
Message-ID: <20180511143518.GN33053@art_vandelay>
References: <1525861364-26323-1-git-send-email-hl@rock-chips.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1525861364-26323-1-git-send-email-hl@rock-chips.com>
User-Agent: Mutt/1.9.2 (2017-12-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, May 09, 2018 at 06:22:41PM +0800, Lin Huang wrote:
> From: Chris Zhong <zyw@rock-chips.com>
> 
> We may support training outside firmware, so we need support
> dpcd read/write to get the message or do some setting with
> display.
> 
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> Signed-off-by: Lin Huang <hl@rock-chips.com>

FTR, I've already done one pass at [1]. All of those nits look fixed, so 

Reviewed-by: Sean Paul <seanpaul@chromium.org>

[1]- https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/985572

> ---
> 
> Changes in v2: 
> - update patch following Enric suggest
> 
>  drivers/gpu/drm/rockchip/cdn-dp-core.c | 55 ++++++++++++++++++++++++----
>  drivers/gpu/drm/rockchip/cdn-dp-core.h |  1 +
>  drivers/gpu/drm/rockchip/cdn-dp-reg.c  | 67 ++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/rockchip/cdn-dp-reg.h  | 14 ++++++-
>  4 files changed, 120 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> index c6fbdcd..cce64c1 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> @@ -176,8 +176,8 @@ static int cdn_dp_get_sink_count(struct cdn_dp_device *dp, u8 *sink_count)
>  	u8 value;
>  
>  	*sink_count = 0;
> -	ret = cdn_dp_dpcd_read(dp, DP_SINK_COUNT, &value, 1);
> -	if (ret)
> +	ret = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, &value, 1);
> +	if (ret < 0)
>  		return ret;
>  
>  	*sink_count = DP_GET_SINK_COUNT(value);
> @@ -374,9 +374,9 @@ static int cdn_dp_get_sink_capability(struct cdn_dp_device *dp)
>  	if (!cdn_dp_check_sink_connection(dp))
>  		return -ENODEV;
>  
> -	ret = cdn_dp_dpcd_read(dp, DP_DPCD_REV, dp->dpcd,
> -			       DP_RECEIVER_CAP_SIZE);
> -	if (ret) {
> +	ret = drm_dp_dpcd_read(&dp->aux, DP_DPCD_REV, dp->dpcd,
> +			       sizeof(dp->dpcd));
> +	if (ret < 0) {
>  		DRM_DEV_ERROR(dp->dev, "Failed to get caps %d\n", ret);
>  		return ret;
>  	}
> @@ -582,8 +582,8 @@ static bool cdn_dp_check_link_status(struct cdn_dp_device *dp)
>  	if (!port || !dp->link.rate || !dp->link.num_lanes)
>  		return false;
>  
> -	if (cdn_dp_dpcd_read(dp, DP_LANE0_1_STATUS, link_status,
> -			     DP_LINK_STATUS_SIZE)) {
> +	if (drm_dp_dpcd_read_link_status(&dp->aux, link_status) !=
> +	    DP_LINK_STATUS_SIZE) {
>  		DRM_ERROR("Failed to get link status\n");
>  		return false;
>  	}
> @@ -1012,6 +1012,40 @@ static int cdn_dp_pd_event(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> +static ssize_t cdn_dp_aux_transfer(struct drm_dp_aux *aux,
> +				   struct drm_dp_aux_msg *msg)
> +{
> +	struct cdn_dp_device *dp = container_of(aux, struct cdn_dp_device, aux);
> +	int ret;
> +	u8 status;
> +
> +	switch (msg->request & ~DP_AUX_I2C_MOT) {
> +	case DP_AUX_NATIVE_WRITE:
> +	case DP_AUX_I2C_WRITE:
> +	case DP_AUX_I2C_WRITE_STATUS_UPDATE:
> +		ret = cdn_dp_dpcd_write(dp, msg->address, msg->buffer,
> +					msg->size);
> +		break;
> +	case DP_AUX_NATIVE_READ:
> +	case DP_AUX_I2C_READ:
> +		ret = cdn_dp_dpcd_read(dp, msg->address, msg->buffer,
> +				       msg->size);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	status = cdn_dp_get_aux_status(dp);
> +	if (status == AUX_STATUS_ACK)
> +		msg->reply = DP_AUX_NATIVE_REPLY_ACK;
> +	else if (status == AUX_STATUS_NACK)
> +		msg->reply = DP_AUX_NATIVE_REPLY_NACK;
> +	else if (status == AUX_STATUS_DEFER)
> +		msg->reply = DP_AUX_NATIVE_REPLY_DEFER;
> +
> +	return ret;
> +}
> +
>  static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>  {
>  	struct cdn_dp_device *dp = dev_get_drvdata(dev);
> @@ -1030,6 +1064,13 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>  	dp->active = false;
>  	dp->active_port = -1;
>  	dp->fw_loaded = false;
> +	dp->aux.name = "DP-AUX";
> +	dp->aux.transfer = cdn_dp_aux_transfer;
> +	dp->aux.dev = dev;
> +
> +	ret = drm_dp_aux_register(&dp->aux);
> +	if (ret)
> +		return ret;
>  
>  	INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);
>  
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.h b/drivers/gpu/drm/rockchip/cdn-dp-core.h
> index f57e296..46159b2 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.h
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.h
> @@ -78,6 +78,7 @@ struct cdn_dp_device {
>  	struct platform_device *audio_pdev;
>  	struct work_struct event_work;
>  	struct edid *edid;
> +	struct drm_dp_aux aux;
>  
>  	struct mutex lock;
>  	bool connected;
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.c b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> index eb3042c..afdfda0 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> @@ -221,7 +221,11 @@ static int cdn_dp_reg_write_bit(struct cdn_dp_device *dp, u16 addr,
>  				   sizeof(field), field);
>  }
>  
> -int cdn_dp_dpcd_read(struct cdn_dp_device *dp, u32 addr, u8 *data, u16 len)
> +/*
> + * Returns the number of bytes transferred on success, or a negative error
> + * code on failure. -ETIMEDOUT is returned if mailbox message not send success;
> + */
> +ssize_t cdn_dp_dpcd_read(struct cdn_dp_device *dp, u32 addr, u8 *data, u16 len)
>  {
>  	u8 msg[5], reg[5];
>  	int ret;
> @@ -247,24 +251,40 @@ int cdn_dp_dpcd_read(struct cdn_dp_device *dp, u32 addr, u8 *data, u16 len)
>  		goto err_dpcd_read;
>  
>  	ret = cdn_dp_mailbox_read_receive(dp, data, len);
> +	if (!ret)
> +		return len;
>  
>  err_dpcd_read:
> +	DRM_DEV_ERROR(dp->dev, "dpcd read failed: %d\n", ret);
>  	return ret;
>  }
>  
> -int cdn_dp_dpcd_write(struct cdn_dp_device *dp, u32 addr, u8 value)
> +#define CDN_AUX_HEADER_SIZE	5
> +#define CDN_AUX_MSG_SIZE	20
> +/*
> + * Returns the number of bytes transferred on success, or a negative error
> + * code on failure. -ETIMEDOUT is returned if mailbox message not send success;
> + * -EINVAL is return if get the wrong data size after message send.
> + */
> +ssize_t cdn_dp_dpcd_write(struct cdn_dp_device *dp, u32 addr, u8 *data, u16 len)
>  {
> -	u8 msg[6], reg[5];
> +	u8 msg[CDN_AUX_MSG_SIZE + CDN_AUX_HEADER_SIZE];
> +	u8 reg[CDN_AUX_HEADER_SIZE];
>  	int ret;
>  
> -	msg[0] = 0;
> -	msg[1] = 1;
> +	if (WARN_ON(len > CDN_AUX_MSG_SIZE) || WARN_ON(len <= 0))
> +		return -EINVAL;
> +
> +	msg[0] = (len >> 8) & 0xff;
> +	msg[1] = len & 0xff;
>  	msg[2] = (addr >> 16) & 0xff;
>  	msg[3] = (addr >> 8) & 0xff;
>  	msg[4] = addr & 0xff;
> -	msg[5] = value;
> +
> +	memcpy(msg + CDN_AUX_HEADER_SIZE, data, len);
> +
>  	ret = cdn_dp_mailbox_send(dp, MB_MODULE_ID_DP_TX, DPTX_WRITE_DPCD,
> -				  sizeof(msg), msg);
> +				  CDN_AUX_HEADER_SIZE + len, msg);
>  	if (ret)
>  		goto err_dpcd_write;
>  
> @@ -277,8 +297,12 @@ int cdn_dp_dpcd_write(struct cdn_dp_device *dp, u32 addr, u8 value)
>  	if (ret)
>  		goto err_dpcd_write;
>  
> -	if (addr != (reg[2] << 16 | reg[3] << 8 | reg[4]))
> +	if ((len != (reg[0] << 8 | reg[1])) ||
> +	    (addr != (reg[2] << 16 | reg[3] << 8 | reg[4]))) {
>  		ret = -EINVAL;
> +	} else {
> +		return len;
> +	}
>  
>  err_dpcd_write:
>  	if (ret)
> @@ -286,6 +310,33 @@ int cdn_dp_dpcd_write(struct cdn_dp_device *dp, u32 addr, u8 value)
>  	return ret;
>  }
>  
> +int cdn_dp_get_aux_status(struct cdn_dp_device *dp)
> +{
> +	u8 status;
> +	int ret;
> +
> +	ret = cdn_dp_mailbox_send(dp, MB_MODULE_ID_DP_TX,
> +				  DPTX_GET_LAST_AUX_STAUS, 0, NULL);
> +	if (ret)
> +		goto err_get_hpd;
> +
> +	ret = cdn_dp_mailbox_validate_receive(dp, MB_MODULE_ID_DP_TX,
> +					      DPTX_GET_LAST_AUX_STAUS,
> +					      sizeof(status));
> +	if (ret)
> +		goto err_get_hpd;
> +
> +	ret = cdn_dp_mailbox_read_receive(dp, &status, sizeof(status));
> +	if (ret)
> +		goto err_get_hpd;
> +
> +	return status;
> +
> +err_get_hpd:
> +	DRM_DEV_ERROR(dp->dev, "get aux status failed: %d\n", ret);
> +	return ret;
> +}
> +
>  int cdn_dp_load_firmware(struct cdn_dp_device *dp, const u32 *i_mem,
>  			 u32 i_size, const u32 *d_mem, u32 d_size)
>  {
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.h b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
> index c4bbb4a83..6580b11 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.h
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
> @@ -328,6 +328,13 @@
>  #define GENERAL_BUS_SETTINGS            0x03
>  #define GENERAL_TEST_ACCESS             0x04
>  
> +/* AUX status*/
> +#define AUX_STATUS_ACK			0
> +#define AUX_STATUS_NACK			1
> +#define AUX_STATUS_DEFER			2
> +#define AUX_STATUS_SINK_ERROR		3
> +#define AUX_STATUS_BUS_ERROR		4
> +
>  #define DPTX_SET_POWER_MNG			0x00
>  #define DPTX_SET_HOST_CAPABILITIES		0x01
>  #define DPTX_GET_EDID				0x02
> @@ -469,8 +476,11 @@ int cdn_dp_set_host_cap(struct cdn_dp_device *dp, u8 lanes, bool flip);
>  int cdn_dp_event_config(struct cdn_dp_device *dp);
>  u32 cdn_dp_get_event(struct cdn_dp_device *dp);
>  int cdn_dp_get_hpd_status(struct cdn_dp_device *dp);
> -int cdn_dp_dpcd_write(struct cdn_dp_device *dp, u32 addr, u8 value);
> -int cdn_dp_dpcd_read(struct cdn_dp_device *dp, u32 addr, u8 *data, u16 len);
> +ssize_t cdn_dp_dpcd_write(struct cdn_dp_device *dp, u32 addr,
> +			  u8 *data, u16 len);
> +ssize_t cdn_dp_dpcd_read(struct cdn_dp_device *dp, u32 addr,
> +			 u8 *data, u16 len);
> +int cdn_dp_get_aux_status(struct cdn_dp_device *dp);
>  int cdn_dp_get_edid_block(void *dp, u8 *edid,
>  			  unsigned int block, size_t length);
>  int cdn_dp_train_link(struct cdn_dp_device *dp);
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS