[RFC v2 2/2] dt-bindings: mipi-dsi: Add dual-channel DSI related info
Archit Taneja
architt at codeaurora.org
Wed Jun 6 16:07:46 UTC 2018
On Wednesday 06 June 2018 04:16 PM, Heiko Stübner wrote:
> Hi Archit,
>
> Am Mittwoch, 6. Juni 2018, 12:21:16 CEST schrieb Archit Taneja:
>> On Wednesday 06 June 2018 02:00 PM, Heiko Stübner wrote:
>>> Am Mittwoch, 6. Juni 2018, 07:59:29 CEST schrieb Archit Taneja:
>>>> On Monday 04 June 2018 05:47 PM, Heiko Stuebner wrote:
>>>>> Am Donnerstag, 18. Januar 2018, 05:53:55 CEST schrieb Archit Taneja:
>>>>>> Add binding info for peripherals that support dual-channel DSI. Add
>>>>>> corresponding optional bindings for DSI host controllers that may
>>>>>> be configured in this mode. Add an example of an I2C controlled
>>>>>> device operating in dual-channel DSI mode.
>>>>>>
>>>>>> Signed-off-by: Archit Taneja <architt at codeaurora.org>
>>>>>
>>>>> Looks like a great solution for that problem, so
>>>>> Reviewed-by: Heiko Stuebner <heiko at sntech.de>
>>>>>
>>>>> As I'm looking into that for my rk3399-scarlet device right now and
>>>>> couldn't find this patchset in the kernel yet, is it planned to
>>>>> merge or refresh these binding changes or were problems encountered.
>>>>>
>>>>> At least an Ack/Review from Rob seems to be missing.
>>>>
>>>> I forgot about these patches. Rob had reviewed the first one in
>>>> the set the second one still needed an Ack. I'll post a v3
>>>> that adds the Reviewed-bys and fixes a small typo.
>>>
>>> very nice ... because it looks like yesterday I managed to make the
>>> Rockchip dsi work in dual mode following this.
>>>
>>> But one question came up, do you really want two input ports on the panel
>>> side? I.e. hardware-wise, I guess the panel will have one 8-lane or so
>>> input thatonly gets split up on the soc side onto 2 dsi controllers?
>>
>> I think all dual DSI panels actually have 2 DSI controllers/parsers
>> within them, one on each port. The MIPI DSI spec doesn't support 8
>> lanes. Also, the pixels coming out of the host are distributed among
>> the lanes differently than what would have been the case with a
>> 'theoretical' 8 lane receiver.
>>
>> Other than that, some dual DSI panels only accept DSI commands on the
>> 'master' port, where as others expect the same command to be sent across
>> both the ports.
>>
>> Therefore, I think it's better to represent dual DSI panels having 2
>> DSI input ports.
>>
>> Your DT looks good to me.
>
> Hmm, that doesn't match up then ;-) ... as my dt uses 2 endpoints
> in one port for the dsi-links.
>
Sorry, I didn't notice you'd created two endpoints within a single port.
I don't think I'm particular about 2 ports vs 1 port with 2 endpoints.
They both seem okay to me as long as we follow it consistently. I'm
myself not 100% sure of how to figure where one should prefer endpoints
over ports. Maybe someone more familiar with the of graph bindings
could comment here.
> The issue I see with using ports and not endpoints for dual-dsi links
> is with distinguishing between input and output ports.
>
> For a panel that's easy, as you every port will be an input port and if
> you have 2, it's supposed dual-dsi. But for example I guess there might
> exist some dual-dsi-to-something bridges, where you would end up
> with say 3 (or even more) ports ... two dual-dsi inputs and 1 or more
> outputs.
Okay, I get your point here. Although, even if the remote device had
exactly 2 ports. Is it safe to assume that port #0 is an input port and
port #1 is an output port? Is that the norm?
I've at least seen one device (toshiba,tc358767 bridge) that can
actually take either DPI as input or DSI based on how you configure it.
There are 2 input ports here, port #0 for DSI and port #1 for DPI. Would
it have made sense here to have a single port and 2 endpoints here too?
>
>
> While the following argument might not be 100% valid from a dt-purity
> standpoint implementing this might get hairy for _any_ operating system,
> as you will need each panel/bridge to tell what the ports are used for.
Yeah.
>
> I.e. in my endpoint based mapping, right now I have this nice and generic
> WIP function to parse the of_graph and get the master+slave nodes:
>
> https://github.com/mmind/linux-rockchip/blob/tmp/rk3399-gru-bob-scarlet/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#L697
> [0]
I'd tried out something locally before posting this patch, I don't have
the code for it, but I can describe the steps I took. This code expects
the panel/bridge to have 2 input ports.
1. DSI0 host driver looks up its output port, gets the remote endpoint,
and get this endpoint's parent (using
of_graph_get_remote_port_parent()). Keeps the parent device node in a
temp variable.
2. DSI1 host driver does the same thing.
3. DSI0 and DSI1 check whether their outputs are connected to the
same device. If so, they're in dual DSI mode. If not, they are
operating independently.
The positive of this approach is that we don't need to make any
assumptions about the panel/bridge's port numbers, which is great.
The negative is that our DSI controller instances now need to query
each other, which can be messy, but not too hard to implement.
I think the choice finally boils down to what makes more sense w.r.t
representing the HW correctly. We'd need Rob's comment on that.
Thanks,
Archit
>
> So I guess my proposal would be to have one port for inputs
> and one port for outputs for dsi peripherals, with possibly
> multiple endpoints in each.
>
>
> Heiko
>
>
> [0] github seems to have reliability problems, so for reference my
> parsing function:
>
> static int dw_mipi_dsi_is_dual(struct dw_mipi_dsi_rockchip *dsi,
> struct device_node **master, struct device_node **slave)
> {
> struct device_node *local_ep, *remote_port, *ep;
> struct device_node *ctrls[2] = { NULL, NULL };
> int num = 0, ret = 0, idx;
>
> /* get local panel endpoint of the dsi controller */
> local_ep = of_graph_get_endpoint_by_regs(dsi->dev->of_node, 1, 0);
> if (!local_ep) {
> DRM_DEV_ERROR(dsi->dev, "couldn't find local panel endpoint\n");
> return -ENXIO;
> }
>
> /* get panel port */
> remote_port = of_graph_get_remote_port_parent(local_ep);
> of_node_put(local_ep);
> if (!remote_port) {
> DRM_DEV_ERROR(dsi->dev, "couldn't find panel port\n");
> return -ENXIO;
> }
>
> /* check other endpoints */
> for_each_endpoint_of_node(remote_port, ep) {
> struct device_node *np = of_graph_get_remote_port_parent(ep);
>
> if (!np)
> continue;
>
> idx = of_property_read_bool(np, "clock-master");
>
> /*
> * Either master or slave already defined, drop refcnt
> * but catch errors only after the full loop.
> */
> if (ctrls[idx])
> of_node_put(np);
> else
> ctrls[idx] = np;
>
> num++;
> }
> of_node_put(remote_port);
>
> if (num > 2) {
> DRM_DEV_ERROR(dsi->dev, "too many dsi devices linked\n");
> ret = -EINVAL;
> goto cleanup;
> }
>
> /* nothing to do */
> if (num < 1) {
> ret = 0;
> goto cleanup;
> }
>
> if (!ctrls[1]) {
> DRM_DEV_ERROR(dsi->dev, "no master defined in dual-dsi\n");
> ret = -ENODEV;
> goto cleanup;
> }
>
> if (!ctrls[0]) {
> DRM_DEV_ERROR(dsi->dev, "no slave defined in dual-dsi\n");
> ret = -ENODEV;
> goto cleanup;
> }
>
> *master = ctrls[1];
> *slave = ctrls[0];
>
> return 1;
>
> cleanup:
> for (idx = 0; idx < 2; idx++)
> if (ctrls[idx])
> of_node_put(ctrls[idx]);
> return ret;
> }
>
>>> So right now I'm operating with a devicetree like
>>>
>>> &mipi_dsi {
>>>
>>> status = "okay";
>>> clock-master;
>>>
>>> ports {
>>>
>>> mipi_out: port at 1 {
>>>
>>> reg = <1>;
>>>
>>> mipi_out_panel: endpoint {
>>>
>>> remote-endpoint = <&mipi_in_panel>;
>>>
>>> };
>>>
>>> };
>>>
>>> };
>>>
>>> mipi_panel: panel at 0 {
>>>
>>> compatible = "innolux,p097pfg";
>>>
>>> reg = <0>;
>>> backlight = <&backlight>;
>>> enable-gpios = <&gpio4 25 GPIO_ACTIVE_HIGH>;
>>> pinctrl-names = "default";
>>> pinctrl-0 = <&display_rst_l>;
>>>
>>> port {
>>>
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>>
>>> mipi_in_panel: endpoint at 0 {
>>>
>>> reg = <0>;
>>> remote-endpoint = <&mipi_out_panel>;
>>>
>>> };
>>>
>>> mipi1_in_panel: endpoint at 1 {
>>>
>>> reg = <1>;
>>> remote-endpoint = <&mipi1_out_panel>;
>>>
>>> };
>>>
>>> };
>>>
>>> };
>>>
>>> };
>>>
>>> &mipi_dsi1 {
>>>
>>> status = "okay";
>>>
>>> ports {
>>>
>>> mipi1_out: port at 1 {
>>>
>>> reg = <1>;
>>>
>>> mipi1_out_panel: endpoint {
>>>
>>> remote-endpoint = <&mipi1_in_panel>;
>>>
>>> };
>>>
>>> };
>>>
>>> };
>>>
>>> };
>>>
>>>
>>> I guess it is a matter of preference on what reflects the hardware
>>> best, so maybe that's Robs call?
>>>
>>>
>>> Heiko
>>>
>>>>>> ---
>>>>>> v2:
>>>>>> - Specify that clock-master is a boolean property.
>>>>>> - Drop/add unit-address and #*-cells where applicable.
>>>>>>
>>>>>> .../devicetree/bindings/display/mipi-dsi-bus.txt | 80
>>>>>> ++++++++++++++++++++++ 1 file changed, 80 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
>>>>>> b/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt index
>>>>>> 94fb72cb916f..7a3abbedb3fa 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
>>>>>> +++ b/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
>>>>>>
>>>>>> @@ -29,6 +29,13 @@ Required properties:
>>>>>> - #size-cells: Should be 0. There are cases where it makes sense to
>>>>>> use
>>>>>> a
>>>>>>
>>>>>> different value here. See below.
>>>>>>
>>>>>> +Optional properties:
>>>>>> +- clock-master: boolean. Should be enabled if the host is being used
>>>>>> in
>>>>>> + conjunction with another DSI host to drive the same peripheral.
>>>>>> Hardware
>>>>>> + supporting such a configuration generally requires the data on both
>>>>>> the busses + to be driven by the same clock. Only the DSI host
>>>>>> instance
>>>>>> controlling this + clock should contain this property.
>>>>>> +
>>>>>>
>>>>>> DSI peripheral
>>>>>> ==============
>>>>>>
>>>>>> @@ -62,6 +69,16 @@ primary control bus, but are also connected to a DSI
>>>>>> bus (mostly for the data>>
>>>>>>
>>>>>> path). Connections between such peripherals and a DSI host can be
>>>>>> represented using the graph bindings [1], [2].
>>>>>>
>>>>>> +Peripherals that support dual channel DSI
>>>>>> +-----------------------------------------
>>>>>> +
>>>>>> +Peripherals with higher bandwidth requirements can be connected to 2
>>>>>> DSI
>>>>>> +busses. Each DSI bus/channel drives some portion of the pixel data
>>>>>> (generally +left/right half of each line of the display, or even/odd
>>>>>> lines of the display). +The graph bindings should be used to represent
>>>>>> the multiple DSI busses that are +connected to this peripheral. Each
>>>>>> DSI
>>>>>> host's output endpoint can be linked to +an input endpoint of the DSI
>>>>>> peripheral.
>>>>>> +
>>>>>>
>>>>>> [1] Documentation/devicetree/bindings/graph.txt
>>>>>> [2] Documentation/devicetree/bindings/media/video-interfaces.txt
>>>>>>
>>>>>> @@ -71,6 +88,8 @@ Examples
>>>>>>
>>>>>> with different virtual channel configurations.
>>>>>>
>>>>>> - (4) is an example of a peripheral on a I2C control bus connected
>>>>>> with
>>>>>> to
>>>>>>
>>>>>> a DSI host using of-graph bindings.
>>>>>>
>>>>>> +- (5) is an example of 2 DSI hosts driving a dual-channel DSI
>>>>>> peripheral,
>>>>>> + which uses I2C as its primary control bus.
>>>>>>
>>>>>> 1)
>>>>>>
>>>>>> dsi-host {
>>>>>>
>>>>>> @@ -153,3 +172,64 @@ Examples
>>>>>>
>>>>>> };
>>>>>>
>>>>>> };
>>>>>>
>>>>>> };
>>>>>>
>>>>>> +
>>>>>> +5)
>>>>>> + i2c-host {
>>>>>> + dsi-bridge at 35 {
>>>>>> + compatible = "...";
>>>>>> + reg = <0x35>;
>>>>>> +
>>>>>> + ports {
>>>>>> + #address-cells = <1>;
>>>>>> + #size-cells = <0>;
>>>>>> +
>>>>>> + port at 0 {
>>>>>> + reg = <0>;
>>>>>> + dsi0_in: endpoint {
>>>>>> + remote-endpoint = <&dsi0_out>;
>>>>>> + };
>>>>>> + };
>>>>>> +
>>>>>> + port at 1 {
>>>>>> + reg = <1>;
>>>>>> + dsi1_in: endpoint {
>>>>>> + remote-endpoint = <&dsi1_out>;
>>>>>> + };
>>>>>> + };
>>>>>> + };
>>>>>> + };
>>>>>> + };
>>>>>> +
>>>>>> + dsi0-host {
>>>>>> + ...
>>>>>> +
>>>>>> + /*
>>>>>> + * this DSI instance drives the clock for both the host
>>>>>> + * controllers
>>>>>> + */
>>>>>> + clock-master;
>>>>>> +
>>>>>> + ports {
>>>>>> + ...
>>>>>> +
>>>>>> + port {
>>>>>> + dsi0_out: endpoint {
>>>>>> + remote-endpoint = <&dsi0_in>;
>>>>>> + };
>>>>>> + };
>>>>>> + };
>>>>>> + };
>>>>>> +
>>>>>> + dsi1-host {
>>>>>> + ...
>>>>>> +
>>>>>> + ports {
>>>>>> + ...
>>>>>> +
>>>>>> + port {
>>>>>> + dsi1_out: endpoint {
>>>>>> + remote-endpoint = <&dsi1_in>;
>>>>>> + };
>>>>>> + };
>>>>>> + };
>>>>>> + };
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm"
>>>>> in
>>>>> the body of a message to majordomo at vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
More information about the dri-devel
mailing list