[PATCH v2 1/2] drm/bridge: Add Cadence DSI driver
Archit Taneja
architt at codeaurora.org
Wed Jun 14 04:02:24 UTC 2017
On 06/06/2017 03:50 PM, Boris Brezillon wrote:
> Hi Archit,
>
> On Sun, 4 Jun 2017 00:20:06 +0530
> Archit Taneja <architt at codeaurora.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
More information about the dri-devel
mailing list