Am Mittwoch, 6. Juni 2018, 18:07:46 CEST schrieb Archit Taneja: > > 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@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. > > > > 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 don't think that anything like that is specified anywhere, so you cannot assume anything about what a port contains. > 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? Nope, I guess having a port for DPI input, one port for DSI input makes quite a lot of sense. And then you can have the input-specifics living in the endpoints like dual links or so. > > 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. Which again would be very much panel-specific as you cannot assume to much about the 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. Taking your DPI+DSI example from above actually shows quite nicely how ports+endpoints could play together. panel-or-bridge@0 { compatible = "something,something"; reg = <0>; ports { /* DPI input */ port@0 { endpoint { remote-endpoint = <&dpi_out>; }; }; /* DSI input */ port@1 { /* from first dsi controller */ endpoint@0 { remote-endpoint = <&dsi0_out>; }; /* from second dsi controller */ endpoint@1 { remote-endpoint = <&dsi1_out>; }; }; /* another input, or an output for a bridge */ port@2 { endpoint { remote-endpoint = <&somewhere>; } }; }; }; But yeah, hopefully we can entice Rob for comments :-) . Heiko > 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@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 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 linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
