On Thu, Mar 10, 2022 at 01:16:57PM +0200, Laurent Pinchart wrote:
> On Thu, Mar 10, 2022 at 11:57:38AM +0100, Maxime Ripard wrote:
> > On Thu, Mar 10, 2022 at 12:42:12PM +0200, Laurent Pinchart wrote:
> > > On Tue, Mar 08, 2022 at 01:51:40PM +0100, Maxime Ripard wrote:
> > > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > > > > On 3/8/22 11:07, Jagan Teki wrote:
> > > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@xxxxxxx> wrote:
> > > > > > >
> > > > > > > On 3/8/22 09:03, Jagan Teki wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > > > > > > static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > > {
> > > > > > > > > struct device *dev = icn->dev;
> > > > > > > > > + struct device_node *endpoint;
> > > > > > > > > struct drm_panel *panel;
> > > > > > > > > + int dsi_lanes;
> > > > > > > > > int ret;
> > > > > > > > >
> > > > > > > > > icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > > return PTR_ERR(icn->enable_gpio);
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > > > > > + dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > > > > > + icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > > > > > + of_node_put(endpoint);
> > > > > > > > > +
> > > > > > > > > + if (!icn->host_node)
> > > > > > > > > + return -ENODEV;
> > > > > > > >
> > > > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > > > > > Display pipeline in R16 [1].
> > > > > > > >
> > > > > > > > We need to have a helper to return host_node for non-ports as I have
> > > > > > > > done it for drm_of_find_bridge.
> > > > > > > >
> > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > > > > >
> > > > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > > > > > required, see:
> > > > > > >
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > > > > >
> > > > > > > What is "non-ports-based OF graph" ?
> > > > > > >
> > > > > > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > > > > >
> > > > > > port@0 is optional as some of the DSI host OF-graph represent the
> > > > > > bridge or panel as child nodes instead of ports. (i think dt-binding
> > > > > > has to fix it to make port@0 optional)
> > > > >
> > > > > The current upstream DT binding document says:
> > > > >
> > > > > required:
> > > > > - port@0
> > > > > - port@1
> > > > >
> > > > > So port@0 is mandatory.
> > > >
> > > > In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> > > > correct. If the bridge supports DCS, there's no reason to use the OF
> > > > graph in the first place: the bridge node will be a child node of the
> > > > MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> > > > MIPI-DSI controller).
> > > >
> > > > I believe port@0 should be made optional (or downright removed if
> > > > MIPI-DCS in the only control bus).
> > >
> > > I think we should make ports mandatory in all cases actually.
> > >
> > > The DT parent-child hierarchy is meant to model control relations
> > > between devices, so a DSI device controlled through DCS should be a
> > > child of the DSI controller. No disagreement there.
> > >
> > > The OF graph is meant to model data connections. While a DSI device
> > > controlled through DCS will use the same DSI link for data transfer, the
> > > two concepts are different. We have taken shortcuts and decided to not
> > > use OF graph for some DSI devices (not necessarily as a well thought
> > > decision, it was sometimes just not considered).
> >
> > I disagree. Unless the data path is explicitly stated using the OF-graph
> > or some other binding, it's inferred.
>
> It is today, and for video data, I think it's showing to be a problem
> :-)
>
> > We never asked ourselves where the
> > data from an i2c chip, an ethernet controller or an v4l2 output device
> > was coming from. It comes from the parent bus, because it's what makes
> > sense. Making a requirement on the OF-Graph to model this would create a
> > big inconsistency.
>
> I'm afraid I disagree, especially when it comes to data transfers from
> device to device. The device tree has never tried to model those until
> OF graph.
Except the data transfer isn't happening between "siblings" devices like
a CRTC and an HDMI controller that the OF-Graph describes perfectly
fine. Here the transfer happens between a parent device and its child.
So it's a bus to device transfer, just like any DMA transfer, or MDIO
transfer, or SPI transfer, etc.
> > > This has led to different issues that we're having to deal with today,
> > > making it more difficult to develop generic code. Going forward, I
> > > think new bindings should always use OF graph to model the data
> > > connection.
> >
> > Either way, that discussion is irrelevant. Not all DSI controllers use
> > OF-Graph, a bridge can be attached to any of them, so we can't require
> > OF-Graph support in any bridge.
>
> Not in any bridge, but we could in new ones, and we could also require
> it in new DT for existing bridge to support new features.
Above disagreement aside, since not all controllers are using it, it
would effectively prevent those new bridges from being used with
!OF-Graph controllers. I don't see how that can be an option.
Maxime
Attachment:
signature.asc
Description: PGP signature
