On Sat, Nov 30, 2019 at 10:37 AM Rob Clark <robdclark@xxxxxxxxx> wrote:
>
> On Mon, Jul 1, 2019 at 7:03 AM Rob Herring <robh+dt@xxxxxxxxxx> wrote:
> >
> > On Sun, Jun 30, 2019 at 2:36 PM Rob Clark <robdclark@xxxxxxxxx> wrote:
> > >
> > > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> > >
> > > The panel-id property in chosen can be used to communicate which panel,
> > > of multiple possibilities, is installed.
> > >
> > > Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > ---
> > > 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
