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

[Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 06/06/2017 03:50 PM, Boris Brezillon wrote:
> Hi Archit,
> 
> On Sun, 4 Jun 2017 00:20:06 +0530
> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> 
> 
>>> +
>>> +#define DPHY_CFG0			0x1b0
>>> +#define DPHY_C_RSTB			BIT(20)
>>> +#define DPHY_D_RSTB(x)			((x) << 16)
>>> +#define DPHY_TIF_FORCE_WRITE		BIT(12)
>>> +#define DPHY_PLL_PDN			BIT(10)
>>> +#define DPHY_CMN_PDN			BIT(9)
>>> +#define DPHY_C_PDN			BIT(8)
>>> +#define DPHY_D_PDN(x)			((x) << 4)
>>> +#define DPHY_PLL_PSO			BIT(1)
>>> +#define DPHY_CMN_PSO			BIT(0)
>>> +
>>> +#define DPHY_CFG1			0x1b4
>>> +#define PDHY_PLL_OPDIV(x)		((x) << 20)
>>> +#define PDHY_PLL_IPDIV(x)		((x) << 12)
>>> +#define PDHY_PLL_FBDIV(x)		(x)
>>> +
>>> +#define DPHY_PLL_TM_LO			0x1b8
>>> +#define DPHY_PLL_TM_MID			0x1bc
>>> +#define DPHY_PLL_TM_HI			0x1c0
>>> +
>>> +#define DPHY_STATUS			0x1c4
>>> +#define PPI_D_RX_ULPS_ESC(x)		((x) >> 12)
>>> +#define PPI_C_TX_READY_HS		BIT(8)
>>> +#define PPI_PLL_LOCK			BIT(7)
>>> +#define PPI_PLL_COARSE			BIT(6)
>>> +#define PPI_PLL_COARSE_CODE(x)		((x) & GENMASK(5, 0))
>>> +
>>> +#define DPHY_BIST			0x1c8
>>> +#define PSO_BYPASS_CTX_EN		BIT(12)
>>> +#define PSO_BYPASS_TX_EN(l)		BIT(8 + (l))
>>> +#define BIST_CTX_EN			BIT(4)
>>> +#define BIST_TX_EN(l)			BIT(l)
>>> +
>>
>> Do the above DPHY registers actually configure the PHY used with the
>> controller, or do we need to configure any additional register outside
>> of this block to get things working?
> 
> The DPHY has a separate I/O mem range with its own interface. I didn't
> provide support for this part yet because the interface is not stable
> yet.
> 
>>
>> I ask because they aren't used in the code below, and the DT binding
>> for this device specifies the PHY as a separate device. What's the
>> plan in the future for PHY?
> 
> The short-term plan is to add support for this DPHY in the cdns-dsi
> driver. The driver will just retrieve the I/O mem range and interrupt
> attached to the cdns DPHY block by following the phandle and using
> of helpers to do the conversion, and then use these resources
> internally.
> 
> The mid/long-term plan is to add a dphy framework (on top of the PHY
> framework) to let dphy providers expose their features in a generic
> manner.
> There are 2 pros to this generic DPHY framework/layer:
> - you could possibly use DPHY and DSI blocks provided by 2 different
>    vendors (not sure how feasible this is in practice)
> - CSI can also use DPHY as its PHY layer, so DPHY drivers could be
>    shared between V4L and DRM drivers
> 
> Note that I can't promise the mid/long-term goals are achievable,
> because my knowledge on DPHY is quite limited, but designing the DT
> bindings to handle this use case is really important, because of the DT
> stable ABI thing.

Okay. There are patches to support the synopsis DW DSI bridge driver
in the list, and the consensus there was that it isn't necessary to
have a separate DPHY node for blocks which are tightly integrated
with a SoC:

https://patchwork.kernel.org/patch/9764395/

I was wondering if we should adopt to a standard way of dealing with
the phys, i.e, let the SoC driver provide phy ops as platform data
versus having a separate PHY driver altogether.

In any case, I don't think there isn't harm having a PHY node in
the bindings even if we go the other way.

> 
> 
>>> +
>>> +static int cdns_dsi_bridge_attach(struct drm_bridge *bridge)
>>> +{
>>> +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>> +	struct cdns_dsi_output *output = input->dsi->output;
>>> +	int ret;
>>> +
>>> +	if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
>>> +		dev_err(input->dsi->base.dev,
>>> +			"cdns-dsi driver is only compatible with DRM devices supporting atomic updates");
>>> +		return -ENOTSUPP;
>>> +	}
>>> +
>>> +	switch (output->type) {
>>> +	case CDNS_DSI_PANEL:
>>> +		ret = cdns_dsi_output_attach_panel(output);
>>> +		break;
>>> +
>>> +	case CDNS_DSI_BRIDGE:
>>> +		ret = drm_bridge_attach(bridge->encoder, output->bridge,
>>> +					bridge);
>>> +		break;
>>
>> Could you have a look at Eric's dsi-panel-bridge patch-set? I think it
>> should simplify things for this driver too.
> 
> Yep, that was planned. Was just waiting for this feature to reach
> drm-misc-next (which seems to be the case ;-)).
> 
>>
>>> +
>>> +	default:
>>> +		ret = -ENOTSUPP;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static bool cdns_dsi_bridge_mode_fixup(struct drm_bridge *bridge,
>>> +				       const struct drm_display_mode *mode,
>>> +				       struct drm_display_mode *adj)
>>> +{
>>> +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>> +	int bpp;
>>> +
>>> +	/*
>>> +	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
>>> +	 * least 1.
>>> +	 */
>>> +	if (adj->crtc_vtotal - adj->crtc_vsync_end < 2)
>>> +		return false;
>>> +
>>> +	/* VSA_DSI = VSA_DPI and must be at least 2. */
>>> +	if (adj->crtc_vsync_end - adj->crtc_vsync_start < 2)
>>> +		return false;
>>> +
>>> +	/* HACT must be a 32-bits aligned. */
>>> +	bpp = mipi_dsi_pixel_format_to_bpp(input->dsi->output->dev->format);
>>> +	if ((adj->hdisplay * bpp) % 32)
>>> +		return false;
>>> +
>>> +	return true;
>>
>> We could consider using the new mode_valid helpers that Jose recently
>> added. This might be better as bridge mode_valid() op.
> 
> Ditto. It was on my TODO list. I'll address that in my v3.
> 
>>
>>
>>> +}
> 
> [...]
> 
>>> +
>>> +static int cdns_dsi_attach(struct mipi_dsi_host *host,
>>> +			   struct mipi_dsi_device *dev)
>>> +{
>>> +	struct cdns_dsi *dsi = to_cdns_dsi(host);
>>> +	struct cdns_dsi_output *output;
>>> +	int ret;
>>> +
>>> +	/* TODO: support multi-devices setup? */
>>> +	if (dsi->output)
>>> +		return -EBUSY;
>>> +
>>> +	output = devm_kzalloc(host->dev, sizeof(*output), GFP_KERNEL);
>>> +	if (!output)
>>> +		return -ENOMEM;
>>> +
>>> +	output->dev = dev;
>>> +
>>> +	output->panel = of_drm_find_panel(dev->dev.of_node);
>>> +	if (output->panel) {
>>> +		output->type = CDNS_DSI_PANEL;
>>> +	} else {
>>> +		output->bridge = of_drm_find_bridge(dev->dev.of_node);
>>> +		if (!output->bridge) {
>>> +			dev_err(host->dev,
>>> +				"%s is neither a panel nor a bridge",
>>> +				dev->name);
>>> +			return -ENOTSUPP;
>>> +		}
>>> +
>>> +		output->type = CDNS_DSI_BRIDGE;
>>> +		dsi->input->bridge.next = output->bridge;
>>> +	}
>>> +
>>> +	dsi->output = output;
>>> +
>>> +	ret = cdns_dsi_init_link(dsi);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* FIXME: should be moved somewhere else. */
>>> +	return drm_bridge_add(&dsi->input->bridge);
>>
>> In the driver probe?
> 
> I assumed that not everything was in place at probe time, but maybe I
> was wrong. This should be good, as long as all DSI devices have been
> instantiated and attached after calling mipi_dsi_host_register(). I
> guess this is something we can ensure by using the proposed bindings
> (with all DSI output ports described in the DT) and returning
> EPROBE_DEFER if at least one of the DSI device is missing at probe time.
> 
>>
>>> +}
>>> +
> 
> [...]
> 
>>> +
>>> +static int cdns_dsi_drm_probe(struct platform_device *pdev)
>>
>> I wanted to bring up the point I made in the DT patch too: Is it
>> more suitable to implement this is a library with bind/unbind, and
>> probe/remove helpers like it's done for dw-hdmi. Could there be SoCs
>> that integrate this IP but require some additional wrapper
>> configurations to make things work?
> 
> As answered in my previous reply, yes it can be done this way, but is it
> worth introducing this infrastructure right now? Note that we can still
> overload the compatible to support SoC specific integration of this IP
> directly in this driver.
> For the rest, it should be pretty transparent to connect a DPI encoder
> to this DSI bridge.

Okay. Sounds good to me.

Thanks,
Archit

> 
> Right now, we have an easy way to connect a DPI encoder to this DSI
> bridge. The only thing that could be missing is the pixel format
> negotiation (the format transiting on the DPI bus), and this is a
> runtime thing, so maybe we can extend the drm_bridge interface to allow
> such negotiation.
> 
> Note that this is a need I had on atmel platforms as well, because the
> DPI bus can be connected to several devices (panels or external
> bridges), and, depending on the device we are enabling in the pipeline,
> we have to reconfigure the DPI encoder to send the pixel in the
> appropriate format.
> 
>>
>> Looks good otherwise.
> 
> Thanks for your review.
> 
> Boris
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html