Re: [PATCH 1/4] dt-bindings: chosen: document panel-id binding

[Rob Clark <robdclark@gmail.com>]

On Sat, Nov 30, 2019 at 10:37 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Jul 1, 2019 at 7:03 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Sun, Jun 30, 2019 at 2:36 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > The panel-id property in chosen can be used to communicate which panel,
> > > of multiple possibilities, is installed.
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >  Documentation/devicetree/bindings/chosen.txt | 69 ++++++++++++++++++++
> > >  1 file changed, 69 insertions(+)
> >
> > I need to update this file to say it's moved to the schema repository...
> >
> > But I don't think that will matter...
> >
> > > diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> > > index 45e79172a646..d502e6489b8b 100644
> > > --- a/Documentation/devicetree/bindings/chosen.txt
> > > +++ b/Documentation/devicetree/bindings/chosen.txt
> > > @@ -68,6 +68,75 @@ on PowerPC "stdout" if "stdout-path" is not found.  However, the
> > >  "linux,stdout-path" and "stdout" properties are deprecated. New platforms
> > >  should only use the "stdout-path" property.
> > >
> > > +panel-id
> > > +--------
> > > +
> > > +For devices that have multiple possible display panels (multi-sourcing the
> > > +display panels is common on laptops, phones, tablets), this allows the
> > > +bootloader to communicate which panel is installed, e.g.
> >
> > How does the bootloader figure out which panel? Why can't the kernel
> > do the same thing?
> >
> > > +
> > > +/ {
> > > +       chosen {
> > > +               panel-id = <0xc4>;
> > > +       };
> > > +
> > > +       ivo_panel {
> > > +               compatible = "ivo,m133nwf4-r0";
> > > +               power-supply = <&vlcm_3v3>;
> > > +               no-hpd;
> > > +
> > > +               ports {
> > > +                       port {
> > > +                               ivo_panel_in_edp: endpoint {
> > > +                                       remote-endpoint = <&sn65dsi86_out_ivo>;
> > > +                               };
> > > +                       };
> > > +               };
> > > +       };
> > > +
> > > +       boe_panel {
> > > +               compatible = "boe,nv133fhm-n61";
> >
> > Both panels are going to probe. So the bootloader needs to disable the
> > not populated panel setting 'status' (or delete the node). If you do
> > that, do you even need 'panel-id'?
> >
>
> So, I'm finally having some time to revisit this proposal..  I have a
> few updates:
>
> + Updated DtbLoader.efi to read UEFIDisplayInfo and get the panel-id
>   so I can drop the efi/libstub patch[1]
> + I can drop drm_of_find_panel_id() and fold the logic to look at
>   /chosen/panel-id into drm_of_find_panel_or_bridge() so that each
>   driver or bridge doesn't need an update
>
> This doesn't realy solve the issue that both panels will probe.  On
> the other hand, I really don't want to make the DtbLoader know enough
> about the dt structure of each laptop to patch dt, since that is not
> going to be scalable at all.  (Likewise, there is some interest for
> devices that use u-boot to take the panel-id from a uboot env var and
> patch dt, but again knowing enough to intelligently patch the dt is
> not going to be feasible.)
>
> But, an alternate solution could be, instead of adding a 'panel-id'
> node in chosen, I could add it as an optional property in the panel
> node.  So taking my original example of the yoga c630 laptops, with
> the two possible panel id's 0xc4 and 0xc5:
>
>     ivo_panel {
>         compatible = "ivo,m133nwf4-r0";
>         panel-id = <0xc4>;

correction, the ivo panel should have panel-id = <0xc5>

>         status = "disabled";
>
>         ports {
>             port {
>                 ivo_panel_in_edp: endpoint {
>                     remote-endpoint = <&sn65dsi86_out_ivo>;
>                 };
>             };
>         };
>     };
>
>     boe_panel {
>         compatible = "boe,nv133fhm-n61";
>         panel-id = <0xc4>;
>         status = "disabled";
>
>         ports {
>             port {
>                 boe_panel_in_edp: endpoint {
>                     remote-endpoint = <&sn65dsi86_out_boe>;
>                 };
>             };
>         };
>     };
>
>     sn65dsi86: bridge@2c {
>         compatible = "ti,sn65dsi86";
>
>         ports {
>             #address-cells = <1>;
>             #size-cells = <0>;
>
>             port@0 {
>                 reg = <0>;
>                 sn65dsi86_in_a: endpoint {
>                     remote-endpoint = <&dsi0_out>;
>                 };
>             };
>
>             port@1 {
>                 reg = <1>;
>
>                 sn65dsi86_out_boe: endpoint@c4 {
>                     remote-endpoint = <&boe_panel_in_edp>;
>                 };
>
>                 sn65dsi86_out_ivo: endpoint@c5 {
>                     remote-endpoint = <&ivo_panel_in_edp>;
>                 };
>             };
>         };
>     };
>
> With this, the "firmware" (DtbLoader, u-boot, etc) could crawl the
> entire dt looking for a node with a panel-id that matches the one it's
> looking for, and change that node's status to enabled.
>
> The advantage would be that the other panel(s) that is not installed
> will not be probed.  The downsides are that (1) the drm drivers would
> have to loop over all the endpoints to find the active panel (some
> drivers do this already, most do not), and (2) the property name
> "panel-id" (or whatever we pick) will now be somehow special, you
> couldn't re-use that name elsewhere without potential to confuse the
> firmware.  And it is more complexity in the firmware (although at
> least it can be done generically) compared to just adding a property
> in chosen.
>
> Not sure if this is better, I thought my initial proposal was more
> elegant.  I am open to other suggestions, anything other than teaching
> DtbLoader/u-boot about the specific dt of each different device that
> would use this.
>
> BR,
> -R
>
> [1] https://github.com/robclark/edk2/commits/dtbloader