[PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Tomi Valkeinen]

Hi Boris,


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 31/08/17 18:55, Boris Brezillon wrote:
> Add a driver for Cadence DPI -> DSI bridge.
> 
> This driver only support a subset of Cadence DSI bridge capabilities.
> 
> Here is a non-exhaustive list of missing features:
>  * burst mode
>  * dynamic configuration of the DPHY based on the
>  * support for additional input interfaces (SDI input)
> 
> Signed-off-by: Boris Brezillon <boris.brezillon at free-electrons.com>
> ---
> Changes in v3:
> - replace magic values by real timing calculation. The DPHY PLL clock
>   is still hardcoded since we don't have a working DPHY block yet, and
>   this is the piece of HW we need to dynamically configure the PLL
>   rate based on the display refresh rate and the resolution.
> - parse DSI devices represented with the OF-graph. This is needed to
>   support DSI devices controlled through an external bus like I2C or
>   SPI.
> - use the DRM panel-bridge infrastructure to simplify the DRM panel
>   logic
> 
> Changes in v2:
> - rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
> - return the correct error when devm_clk_get(sysclk) fails
> - add missing depends on OF and select DRM_PANEL in the Kconfig entry
> ---
>  drivers/gpu/drm/bridge/Kconfig    |    9 +
>  drivers/gpu/drm/bridge/Makefile   |    1 +
>  drivers/gpu/drm/bridge/cdns-dsi.c | 1090 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 1100 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c

We need some power management. At the moment the clocks are kept always
enabled. Those need to be turned off when the IP is not used.

> +static irqreturn_t cdns_dsi_interrupt(int irq, void *data)
> +{
> +	struct cdns_dsi *dsi = data;
> +	irqreturn_t ret = IRQ_NONE;
> +	u32 flag, ctl;
> +
> +	flag = readl(dsi->regs + DIRECT_CMD_STS_FLAG);
> +	if (flag) {
> +		ctl = readl(dsi->regs + DIRECT_CMD_STS_CTL);
> +		ctl &= ~flag;
> +		writel(ctl, dsi->regs + DIRECT_CMD_STS_CTL);

I presume it's the enable/disable bit in STS_CTL that prevents the
interrupt from triggering again, instead of the status flag? Just making
sure, because I think on some IPs the status flag has been the one that
triggers the interrupt.

> +		complete(&dsi->direct_cmd_comp);
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	return ret;
> +}
> +
> +static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
> +				 const struct mipi_dsi_msg *msg)
> +{
> +	struct cdns_dsi *dsi = to_cdns_dsi(host);
> +	u32 cmd, sts, val, wait = WRITE_COMPLETED, ctl = 0;
> +	struct mipi_dsi_packet packet;
> +	int ret, i, tx_len, rx_len;
> +
> +	ret = mipi_dsi_create_packet(&packet, msg);
> +	if (ret)
> +		return ret;
> +
> +	tx_len = msg->tx_buf ? msg->tx_len : 0;
> +	rx_len = msg->rx_buf ? msg->rx_len : 0;
> +
> +	/* For read operations, the maximum TX len is 2. */

Hmm, why is that?

> +	if (rx_len && tx_len > 2)
> +		return -ENOTSUPP;
> +
> +	/* TX len is limited by the CMD FIFO depth. */
> +	if (tx_len > dsi->direct_cmd_fifo_depth)
> +		return -ENOTSUPP;
> +
> +	/* RX len is limited by the RX FIFO depth. */
> +	if (rx_len > dsi->rx_fifo_depth)
> +		return -ENOTSUPP;
> +
> +	cmd = CMD_SIZE(tx_len) | CMD_VCHAN_ID(msg->channel) |
> +	      CMD_DATATYPE(msg->type);
> +
> +	if (msg->flags & MIPI_DSI_MSG_USE_LPM)
> +		cmd |= CMD_LP_EN;
> +
> +	if (mipi_dsi_packet_format_is_long(msg->type))
> +		cmd |= CMD_LONG;
> +
> +	if (rx_len) {
> +		cmd |= READ_CMD;
> +		wait = READ_COMPLETED_WITH_ERR | READ_COMPLETED;
> +		ctl = READ_EN | BTA_EN;
> +	} else if (msg->flags & MIPI_DSI_MSG_REQ_ACK) {
> +		cmd |= BTA_REQ;
> +		wait = ACK_WITH_ERR_RCVD | ACK_RCVD;
> +		ctl = BTA_EN;
> +	}

It's been a while since I worked with DSI, but... Shouldn't there be a
check somewhere that the packet(s) can fit into the blanking intervals?

 Tomi