On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote:
> On 3/10/22 15:18, Maxime Ripard wrote:
> > On Thu, Mar 10, 2022 at 01:47:13PM +0100, Marek Vasut wrote:
> > > On 3/10/22 11:53, Maxime Ripard wrote:
> > > > On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
> > > > > On 3/8/22 17:21, Maxime Ripard wrote:
> > > > > > On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
> > > > > > > On 3/8/22 14:49, Maxime Ripard wrote:
> > > > > > > > On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
> > > > > > > > > On 3/8/22 13:51, 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).
> > > > > > > > >
> > > > > > > > > That's out of scope of this series anyway, so Jagan can implement patches
> > > > > > > > > for that mode if needed.
> > > > > > > >
> > > > > > > > Not really? You can't count on the port@0 being there generally
> > > > > > > > speaking, so you can't count on data-lanes being there either, which
> > > > > > > > exactly what you're doing in this patch.
> > > > > > >
> > > > > > > I can because the upstream DT bindings currently say port@0 must be present,
> > > > > > > see above. If that requirement should be relaxed, sure, but that's a
> > > > > > > separate series.
> > > > > >
> > > > > > And another upstream DT bindings say that you don't need them at all.
> > > > >
> > > > > Which "another upstream DT bindings" do you refer to ?
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> > > >
> > > > > > Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
> > > > > > binding is far more relevant than a single bridge driver.
> > > > >
> > > > > That seems to be the wrong way around, how can generic subsystem-wide
> > > > > binding take precedence over specific driver binding ?
> > > >
> > > > This is the binding of the bus. You're part of that bus. You're a child
> > > > node of that bus, but nothing ever mandates that your parent node uses
> > > > the same convention. And some don't. And since your bridge can be
> > > > connected to pretty much any DSI controller, you have to use the lowest
> > > > common denominator, not make up some new constraints that not all
> > > > controller will be able to comply with.
> > >
> > > It seems to me the ICN6211 DT bindings currently further constraint the
> > > generic DSI bus bindings, and that seems OK to me.
> > >
> > > Let me reiterate this again -- if someone wants to relax the requirements
> > > currently imposed by the ICN6211 DT bindings, fine, but that can be done in
> > > a separate patchset AND that needs DT bindings update. Furthermore, there
> > > are no users of this ICN6211 bridge in upstream DTs, so there is currently
> > > no bridge which would operate without OF graph using this driver.
> >
> > And let me reiterate this again: something that used to work for a user
> > doesn't anymore when your series is applied. This is a textbook
> > regression. I suggested a way forward, that you don't like for some
> > reason, fine. But pushing through a regression is just not acceptable.
>
> How can this be a regression if this mode of operation could not have ever
> been supported with valid upstream DT bindings in the first place ?
>
> Should we now require that kernel drivers somehow magically support all
> kinds of random broken DT bindings in addition to ones which pass YAML DT
> validation ?
The thing is, as I told you multiple times already, it was broken from
the bridge standpoint, but not from the controller's. If it had been
correct for the bridge, it wouldn't have been for the controller. So,
same story.
The only difference is that it wouldn't affect you, but I don't see how
it's relevant.
> > > > > > So figuring it out is very much a prerequisite to that series,
> > > > > > especially since those patches effectively make the OF-graph mandatory
> > > > > > in some situations, while it was purely aesthetics before.
> > > > >
> > > > > The OF-graph is mandatory per the DT bindings of this driver. If you
> > > > > implement invalid DT which does not contain port@0, it will fail DT
> > > > > validation.
> > > > >
> > > > > If this requirement should be relaxed, sure, it can and I don't think it
> > > > > would be hard to do, but I don't see why that should be part of this series,
> > > > > which follows the upstream DT binding document for this driver.
> > > > >
> > > > > If I cannot trust the driver DT bindings to indicate what is and is not
> > > > > mandatory, what other document can I trust then ...
> > > >
> > > > Oh, come on. Doing that, you also require OF-Graph support for the DSI
> > > > controller you attach to, and you can't require that. This is very
> > > > different from just requiring a property that doesn't have any impact on
> > > > any other device, and you know that very well.
> > >
> > > Currently the ICN6211 DT bindings DO require that kind of bridge.
> >
> > And while this wasn't enforced before, you make it a hard requirement
> > with this series. This is what changed, and what caused this whole
> > discussion.
>
> The current DT bindings already make it a hard requirement, so no, nothing
> changed here.
>
> Unless what you are trying to ask for is support for broken DT bindings
> which do not pass YAML DT validation by this driver, but that is very
> dangerous, because then the question is, how far should we support such
> broken bindings. What kind of broken is still OK and what kind of broken is
> no longer OK ?
If it ever worked in a mainline release, it must always work. See:
https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html
As far as I'm concerned, it's the sole criteria. So to answer your
question, if it was broken but worked at some point, yes, we need to
keep supporting it. If it never worked, no, we don't.
Honestly, I don't get the push-back. To fix this properly would require:
- to remove port@0 being mandatory
- to move the data-lanes property to the bridge node itself.
That's it. It takes 5 minutes, 30 minutes with the test and the commit
log. We've spent more time arguing about it already.
So if you want to continue debating on whether it's a regression or not,
or whether DT ABI stability is a good thing or not, go ahead. But as far
as I'm concerned, this isn't really up for debate.
Maxime
Attachment:
signature.asc
Description: PGP signature
