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@xxxxxxxxxxxxxx> > >>> > >>> Looks like a great solution for that problem, so > >>> Reviewed-by: Heiko Stuebner <heiko@xxxxxxxxx> > >>> > >>> 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. 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. 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. 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] 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@1 { > > > > reg = <1>; > > > > mipi_out_panel: endpoint { > > > > remote-endpoint = <&mipi_in_panel>; > > > > }; > > > > }; > > > > }; > > > > mipi_panel: panel@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@0 { > > > > reg = <0>; > > remote-endpoint = <&mipi_out_panel>; > > > > }; > > > > mipi1_in_panel: endpoint@1 { > > > > reg = <1>; > > remote-endpoint = <&mipi1_out_panel>; > > > > }; > > > > }; > > > > }; > > > > }; > > > > &mipi_dsi1 { > > > > status = "okay"; > > > > ports { > > > > mipi1_out: port@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@35 { > >>>> + compatible = "..."; > >>>> + reg = <0x35>; > >>>> + > >>>> + ports { > >>>> + #address-cells = <1>; > >>>> + #size-cells = <0>; > >>>> + > >>>> + port@0 { > >>>> + reg = <0>; > >>>> + dsi0_in: endpoint { > >>>> + remote-endpoint = <&dsi0_out>; > >>>> + }; > >>>> + }; > >>>> + > >>>> + port@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@xxxxxxxxxxxxxxx > >>> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
