[RFC v2 2/2] dt-bindings: mipi-dsi: Add dual-channel DSI related info
Heiko Stuebner
heiko at sntech.de
Wed Jun 6 23:08:53 UTC 2018
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 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 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 at 0 {
compatible = "something,something";
reg = <0>;
ports {
/* DPI input */
port at 0 {
endpoint {
remote-endpoint = <&dpi_out>;
};
};
/* DSI input */
port at 1 {
/* from first dsi controller */
endpoint at 0 {
remote-endpoint = <&dsi0_out>;
};
/* from second dsi controller */
endpoint at 1 {
remote-endpoint = <&dsi1_out>;
};
};
/* another input, or an output for a bridge */
port at 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 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