drm+dt: multi panel selection and yoga c630 display
Subsystems (8)
cover drm+dt: multi panel selection and yoga c630 display
From: Rob Clark <[email protected]>
It is not uncommon for devices to use one of several possible panels. The Lenovo Yoga C630 laptop is one such device. This patchset introduces an optional "panel-id" property which can be used by the firmware to find the correct panel node to enable. The second patch adds support in drm/of to automatically pick the enabled endpoint, to avoid adding the same logic in multiple bridges/drivers. The last patch uses this mechanism to enable display support for the Yoga C630.
An example usage:
boe_panel {
compatible = "boe,nv133fhm-n61";
panel-id = <0xc4>;
/* fw will change status to "Ok" if this panel is installed */
status = "disabled"; ports {
port {
boe_panel_in_edp: endpoint {
remote-endpoint = <&sn65dsi86_out_boe>;
};
};
};};
ivo_panel {
compatible = "ivo,m133nwf4-r0";
panel-id = <0xc5>;
/* fw will change status to "Ok" if this panel is installed */
status = "disabled"; ports {
port {
ivo_panel_in_edp: endpoint {
remote-endpoint = <&sn65dsi86_out_ivo>;
};
};
};};
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>;
};
};
};};
This replaces an earlier proposal[1] to use chosen/panel-id to select the installed panel, in favor of adding support[2] to an EFI driver module (DtbLoader.efi) to find the installed panel, locate it in dtb via the 'panel-id' property, and update it's status to "Ok".
[1] https://patchwork.kernel.org/cover/11024613/
[2] https://github.com/robclark/edk2/commits/dtbloader
In this case, DtbLoader, which is somewhat generic (ie. this mechanism applies to all snapdragon based devices which orignally ship with windows), determines the panel-id of the installed panel from the UEFIDisplayInfo variable.
As I understand, a similar situation exists with the pine64 laptops. A similar scheme could be used to support this, by loading the panel-id from a u-boot variable.
In other cases (phones), a more device specific shim would be needed to determine the panel-id by reading some GPIOs, or some other more device- specific mechanism.
Bjorn Andersson (1):
arm64: dts: qcom: c630: Enable display
Rob Clark (3):
dt-bindings: display: panel: document panel-id drm/of: add support to find any enabled endpoint drm/bridge: ti-sn65dsi86: find any enabled endpoint
.../bindings/display/panel/panel-common.yaml | 26 +++ .../boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 165 ++++++++++++++++++ drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +- drivers/gpu/drm/drm_of.c | 41 ++++- 4 files changed, 232 insertions(+), 2 deletions(-)
Signature · 1 line
2.23.0
PATCH 1/4 dt-bindings: display: panel: document panel-id
From: Rob Clark <[email protected]>
For devices that have one of several possible panels installed, the panel-id property gives firmware a generic way to locate and enable the panel node corresponding to the installed panel. Example of how to use this property:
ivo_panel {
compatible = "ivo,m133nwf4-r0";
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>;
};
};
};
};--- .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)
Documentation/devicetree/bindings/display/panel/panel-common.yaml +26 -0
@@ -75,6 +75,32 @@ properties: in the device graph bindings defined in Documentation/devicetree/bindings/graph.txt. + panel-id: + description: + To support the case where one of several different panels can be installed + on a device, the panel-id property can be used by the firmware to identify + which panel should have it's status changed to "ok". This property is not + used by the HLOS itself. + + For a device with multiple potential panels, a node for each potential + should be defined with status = "disabled", and an appropriate panel-id + property. The video data producer should be setup with endpoints going to + each possible panel. The firmware will find the dt node with a panel-id + matching the actual panel installed, and change it's status to "ok". + + The exact method the firmware uses to determine the panel-id of the installed + panel is outside the scope of this binding, but a few examples are + + 1) u-boot module reading a value from a u-boot env var + 2) EFI driver module reading a value from an EFI variable + 3) device specific firmware reading some device specific GPIOs or + e-fuse + + The panel-id values are an opaque integer. They can be sparse. The only + important thing is that each possible panel in the system has a unique + panel-id, and that the values configured in the device's DTB match the + values that the firmware is looking for. + ddc-i2c-bus: $ref: /schemas/types.yaml#/definitions/phandle description:
Signature · 6 lines
2.23.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
Quoted 5 lines · Rob Clark
From: Rob Clark <[email protected]>
For devices that have one of several possible panels installed, the panel-id property gives firmware a generic way to locate and enable the panel node corresponding to the installed panel.
For display timings there is something similar.
Here the property is named native-mode and is a phandle to the preferred timing.
And it is documented that if no native-mode is specified the first timing in the tree is chosen.
So a different concept than this.
I could not from your otherwise well-documented changelog see why you wanted to go for an opauge integer and status rather than a phandle to the active display.
The panel-id, if I get it right, is optional and the important part is that the first panel with staus = "okay" is selected.
This would cover my usecase fine.
I have a target with four different displays and the bootloader knows what display is used (based on gpio etc).
The bootloader (barebox in my case) uses a simple variant of the DT, but reads in the DT used by the kernel and can modify the DT before it is passed to the kernel.
Sam
Quoted 107 lines
Example of how to use this property:
ivo_panel { compatible = "ivo,m133nwf4-r0"; 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>; }; }; }; };Signed-off-by Rob Clark <[email protected]>--- .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)Documentation/devicetree/bindings/display/panel/panel-common.yaml +26
-0@@ -75,6 +75,32 @@ properties: in the device graph bindings defined in Documentation/devicetree/bindings/graph.txt. + panel-id: + description: + To support the case where one of several different panels can be installed + on a device, the panel-id property can be used by the firmware to identify + which panel should have it's status changed to "ok". This property is not + used by the HLOS itself. + + For a device with multiple potential panels, a node for each potential + should be defined with status = "disabled", and an appropriate panel-id + property. The video data producer should be setup with endpoints going to + each possible panel. The firmware will find the dt node with a panel-id + matching the actual panel installed, and change it's status to "ok". + + The exact method the firmware uses to determine the panel-id of the installed + panel is outside the scope of this binding, but a few examples are + + 1) u-boot module reading a value from a u-boot env var + 2) EFI driver module reading a value from an EFI variable + 3) device specific firmware reading some device specific GPIOs or + e-fuse + + The panel-id values are an opaque integer. They can be sparse. The only + important thing is that each possible panel in the system has a unique + panel-id, and that the values configured in the device's DTB match the + values that the firmware is looking for. + ddc-i2c-bus: $ref: /schemas/types.yaml#/definitions/phandle description:Signature · 6 lines
2.23.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
Quoted 15 lines · Sam Ravnborg
Quoted 5 lines · Rob Clark
From: Rob Clark <[email protected]>
For devices that have one of several possible panels installed, the panel-id property gives firmware a generic way to locate and enable the panel node corresponding to the installed panel.
For display timings there is something similar.
Here the property is named native-mode and is a phandle to the preferred timing.
And it is documented that if no native-mode is specified the first timing in the tree is chosen.
So a different concept than this.
Quoted 3 lines
I could not from your otherwise well-documented changelog see why you wanted to go for an opauge integer and status rather than a phandle to the active display.
I think a lot of cases, panel-id could simply be an integer 0..N, but for the snapdragon windows devices, they seem to assign each panel a unique id. For example, the two possible panels that we've seen on the c630 are 0xc4 and 0xc5. I think all the values we've seen so far on other aarch64 laptops fit in an u8, but the actual value is defined as u32. The meaning behind those values is not really terribly important (and might well be arbitrary.. I'm not sure why they didn't go with a GUID). All that matters is they match what DtbLoader pulls out of the u32 PanelId field in the UEFIDisplayInfo variable. The intention behind describing the value as "opaque" was simply "don't assume it has to be 0..N".
As far as using phandles, I had toyed around with the idea.. the ideal thing would be if I could compile the dtb with an unresolved phandle link, and then fixup that link in DtbLoader based on panel-id. But this seems not to be possible, afaict I'd have to create a dummy node for the phandle to point to. Maybe I'm missing something, if there were a way to do this then I could make this work without any drm patches. Ofc I'm open to suggestions.
Quoted 3 lines
The panel-id, if I get it right, is optional and the important part is that the first panel with staus = "okay" is selected.
yup, this was to ensure that the other panels don't probe, which was a problem pointed out by robher with my previous approach
Quoted 6 lines
This would cover my usecase fine.
I have a target with four different displays and the bootloader knows what display is used (based on gpio etc).
The bootloader (barebox in my case) uses a simple variant of the DT, but reads in the DT used by the kernel and can modify the DT before it is passed to the kernel.
I'd be pretty happy if this (or whatever the eventual solution is) covers all the possible multi-sourced panel cases.. this comes up in nearly all consumer devices (laptops, phones, etc) and we pretty badly need an upstream solution for this.
BR,
-R
Quoted 109 lines
Sam
Quoted 103 lines
Example of how to use this property:
ivo_panel { compatible = "ivo,m133nwf4-r0"; 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>; }; }; }; };Signed-off-by Rob Clark <[email protected]>--- .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)Documentation/devicetree/bindings/display/panel/panel-common.yaml +26
-1@@ -75,6 +75,32 @@ properties: in the device graph bindings defined in Documentation/devicetree/bindings/graph.txt. + panel-id: + description: + To support the case where one of several different panels can be installed + on a device, the panel-id property can be used by the firmware to identify + which panel should have it's status changed to "ok". This property is not + used by the HLOS itself. + + For a device with multiple potential panels, a node for each potential + should be defined with status = "disabled", and an appropriate panel-id + property. The video data producer should be setup with endpoints going to + each possible panel. The firmware will find the dt node with a panel-id + matching the actual panel installed, and change it's status to "ok". + + The exact method the firmware uses to determine the panel-id of the installed + panel is outside the scope of this binding, but a few examples are + + 1) u-boot module reading a value from a u-boot env var + 2) EFI driver module reading a value from an EFI variable + 3) device specific firmware reading some device specific GPIOs or + e-fuse + + The panel-id values are an opaque integer. They can be sparse. The only + important thing is that each possible panel in the system has a unique + panel-id, and that the values configured in the device's DTB match the + values that the firmware is looking for. + ddc-i2c-bus: $ref: /schemas/types.yaml#/definitions/phandle description: --2.23.0
Quoted 4 lines
Quoted 4 lines
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
In reply to Rob ClarkHi Rob.
The panel-id can be used to help in several usecase. With a few nits pointed out below fixed:
Sam
Quoted 73 lines · Rob Clark
From: Rob Clark <[email protected]>
For devices that have one of several possible panels installed, the panel-id property gives firmware a generic way to locate and enable the panel node corresponding to the installed panel. Example of how to use this property:
ivo_panel { compatible = "ivo,m133nwf4-r0"; 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>; }; }; }; };Signed-off-by Rob Clark <[email protected]>--- .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)Documentation/devicetree/bindings/display/panel/panel-common.yaml +0
-0
Documentation/devicetree/bindings/display/panel/panel-common.yaml properties: · line 82
Quoted 8 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +5
-0@@ -75,6 +75,32 @@ properties: in the device graph bindings defined in Documentation/devicetree/bindings/graph.txt. + panel-id: + description: + To support the case where one of several different panels can be installed + on a device, the panel-id property can be used by the firmware to identify + which panel should have it's status changed to "ok". This property is not
Use "okay" as this is waht is specified in the CT files.
Documentation/devicetree/bindings/display/panel/panel-common.yaml properties: · line 83
Quoted 1 line
Documentation/devicetree/bindings/display/panel/panel-common.yaml +1
-0+ used by the HLOS itself.
Spell out HLOS - it is not obvious for all what it is.
Documentation/devicetree/bindings/display/panel/panel-common.yaml properties: · line 86
Quoted 3 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +3
-0+ + For a device with multiple potential panels, a node for each potential + should be defined with status = "disabled", and an appropriate panel-id
"potential panel should"
Quoted 27 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +17
-0+ property. The video data producer should be setup with endpoints going to + each possible panel. The firmware will find the dt node with a panel-id + matching the actual panel installed, and change it's status to "ok". + + The exact method the firmware uses to determine the panel-id of the installed + panel is outside the scope of this binding, but a few examples are + + 1) u-boot module reading a value from a u-boot env var + 2) EFI driver module reading a value from an EFI variable + 3) device specific firmware reading some device specific GPIOs or + e-fuse + + The panel-id values are an opaque integer. They can be sparse. The only + important thing is that each possible panel in the system has a unique + panel-id, and that the values configured in the device's DTB match the + values that the firmware is looking for. + ddc-i2c-bus: $ref: /schemas/types.yaml#/definitions/phandle description:Signature · 6 lines
2.23.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Laurent Pinchart
reviewerRe: [PATCH 1/4] dt-bindings: display: panel: document panel-id
In reply to Rob ClarkHi Rob,
Thank you for the patch.
Quoted 73 lines · Rob Clark
From: Rob Clark <[email protected]>
For devices that have one of several possible panels installed, the panel-id property gives firmware a generic way to locate and enable the panel node corresponding to the installed panel. Example of how to use this property:
ivo_panel { compatible = "ivo,m133nwf4-r0"; 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>; }; }; }; };Signed-off-by Rob Clark <[email protected]>--- .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)Documentation/devicetree/bindings/display/panel/panel-common.yaml +0
-0
Documentation/devicetree/bindings/display/panel/panel-common.yaml properties: · line 83
Quoted 9 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +6
-0@@ -75,6 +75,32 @@ properties: in the device graph bindings defined in Documentation/devicetree/bindings/graph.txt. + panel-id: + description: + To support the case where one of several different panels can be installed + on a device, the panel-id property can be used by the firmware to identify + which panel should have it's status changed to "ok". This property is not + used by the HLOS itself.
If your firmware can modify the status property of a panel, it can also add DT nodes. As discussed before, I don't think this belongs to DT. Even if panel-id isn't used by the operating system, you have Linux kernel patches in this series that show that this isn't transparent.
This needs to be handled in the firmware (or, if not possible, in a kernel board driver). The above DT fragment, visible to the kernel, doesn't describe the actual hardware. Furthermore, you would require all bridge drivers to be patched to support this method, which shows again that the issue isn't handled in the right place.
Finally, unless I'm mistaken, this series is meant to support display for an ACPI-based ARM machine. Using DT as a stop-gap measure because ACPI support isn't there yet is fine out-of-tree, and fine by me in-tree provided that the DT bindings are clean, but not when DT is abused like this.
I'm sorry, but this is a NACK from me. Please handle this transparently in the firmware if you want DT-based boot, or with ACPI.
Quoted 23 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +20
-0+ + For a device with multiple potential panels, a node for each potential + should be defined with status = "disabled", and an appropriate panel-id + property. The video data producer should be setup with endpoints going to + each possible panel. The firmware will find the dt node with a panel-id + matching the actual panel installed, and change it's status to "ok". + + The exact method the firmware uses to determine the panel-id of the installed + panel is outside the scope of this binding, but a few examples are + + 1) u-boot module reading a value from a u-boot env var + 2) EFI driver module reading a value from an EFI variable + 3) device specific firmware reading some device specific GPIOs or + e-fuse + + The panel-id values are an opaque integer. They can be sparse. The only + important thing is that each possible panel in the system has a unique + panel-id, and that the values configured in the device's DTB match the + values that the firmware is looking for. + ddc-i2c-bus: $ref: /schemas/types.yaml#/definitions/phandle description:
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
Quoted 82 lines · Laurent Pinchart
Hi Rob,
Thank you for the patch.
Quoted 76 lines · Rob Clark
From: Rob Clark <[email protected]>
For devices that have one of several possible panels installed, the panel-id property gives firmware a generic way to locate and enable the panel node corresponding to the installed panel. Example of how to use this property:
ivo_panel { compatible = "ivo,m133nwf4-r0"; 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>; }; }; }; };Signed-off-by Rob Clark <[email protected]>--- .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)Documentation/devicetree/bindings/display/panel/panel-common.yaml +0
-0@@ -75,6 +75,32 @@ properties: in the device graph bindings defined in Documentation/devicetree/bindings/graph.txt.
Quoted 11 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml properties: · line 83
Quoted 6 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +6
-0+ panel-id: + description: + To support the case where one of several different panels can be installed + on a device, the panel-id property can be used by the firmware to identify + which panel should have it's status changed to "ok". This property is not + used by the HLOS itself.If your firmware can modify the status property of a panel, it can also add DT nodes. As discussed before, I don't think this belongs to DT. Even if panel-id isn't used by the operating system, you have Linux kernel patches in this series that show that this isn't transparent.
I've already explained several times why this is not feasible. It would require DtbLoader to be familiar with each individual device, and be rev'd every time a new device appears. That is not practical at all.
(And fwiw, the ACPI tables describe each panel.. with an ACPI method that is passed the the panel-id and returns the appropriate table.. since DT doesn't have methods, this is the solution.)
I stand by this patch, we can't keep running away from this problem and wave the magic firmware wand.
BR,
-R
Quoted 42 lines
This needs to be handled in the firmware (or, if not possible, in a kernel board driver). The above DT fragment, visible to the kernel, doesn't describe the actual hardware. Furthermore, you would require all bridge drivers to be patched to support this method, which shows again that the issue isn't handled in the right place.
Finally, unless I'm mistaken, this series is meant to support display for an ACPI-based ARM machine. Using DT as a stop-gap measure because ACPI support isn't there yet is fine out-of-tree, and fine by me in-tree provided that the DT bindings are clean, but not when DT is abused like this.
I'm sorry, but this is a NACK from me. Please handle this transparently in the firmware if you want DT-based boot, or with ACPI.
Quoted 13 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +13
-0+ + For a device with multiple potential panels, a node for each potential + should be defined with status = "disabled", and an appropriate panel-id + property. The video data producer should be setup with endpoints going to + each possible panel. The firmware will find the dt node with a panel-id + matching the actual panel installed, and change it's status to "ok". + + The exact method the firmware uses to determine the panel-id of the installed + panel is outside the scope of this binding, but a few examples are + + 1) u-boot module reading a value from a u-boot env var + 2) EFI driver module reading a value from an EFI variable + 3) device specific firmware reading some device specific GPIOs orDocumentation/devicetree/bindings/display/panel/panel-common.yaml properties: · line 106
Quoted 10 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +7
-0+ e-fuse + + The panel-id values are an opaque integer. They can be sparse. The only + important thing is that each possible panel in the system has a unique + panel-id, and that the values configured in the device's DTB match the + values that the firmware is looking for. + ddc-i2c-bus: $ref: /schemas/types.yaml#/definitions/phandle description:--
Regards,
Quoted 1 line
Laurent Pinchart
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Laurent Pinchart
reviewerRe: [PATCH 1/4] dt-bindings: display: panel: document panel-id
Hi Rob,
Quoted 95 lines · Rob Clark
Quoted 77 lines · Laurent Pinchart
Quoted 76 lines · Rob Clark
From: Rob Clark <[email protected]>
For devices that have one of several possible panels installed, the panel-id property gives firmware a generic way to locate and enable the panel node corresponding to the installed panel. Example of how to use this property:
ivo_panel { compatible = "ivo,m133nwf4-r0"; 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>; }; }; }; };Signed-off-by Rob Clark <[email protected]>--- .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)Documentation/devicetree/bindings/display/panel/panel-common.yaml +0
-0@@ -75,6 +75,32 @@ properties: in the device graph bindings defined in Documentation/devicetree/bindings/graph.txt.Quoted 11 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml properties: · line 83
Quoted 6 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +6
-0+ panel-id: + description: + To support the case where one of several different panels can be installed + on a device, the panel-id property can be used by the firmware to identify + which panel should have it's status changed to "ok". This property is not + used by the HLOS itself.If your firmware can modify the status property of a panel, it can also add DT nodes. As discussed before, I don't think this belongs to DT. Even if panel-id isn't used by the operating system, you have Linux kernel patches in this series that show that this isn't transparent.
I've already explained several times why this is not feasible. It would require DtbLoader to be familiar with each individual device, and be rev'd every time a new device appears. That is not practical at all.
Quoted 6 lines
(And fwiw, the ACPI tables describe each panel.. with an ACPI method that is passed the the panel-id and returns the appropriate table.. since DT doesn't have methods, this is the solution.)
I stand by this patch, we can't keep running away from this problem and wave the magic firmware wand.
I believe in firmware solutions more than firmware magic wands :-)
While device-specific knowledge in DtbLoader is indeed not practical, you still need device-specific knowledge at the firmware level in the sense that you pass a device-specific DT binary to DtbLoader to be patched based on the panel ID. The device-specific information required at the firmware level can thus be expressed as data instead of code.
I understand it wouldn't be practical for DtbLoader to receive two independent pieces of device-specific data (the DT binary and another custom data blob). Why couldn't however DtbLoader get a DT binary as described in the commit message of this patch, and strip off all the panel nodes that are not applicable to the platform, as well as the panel-id property ? This would be completely transparent on the OS side, and would not require patching DtbLoader for every device, as all the information required would be present in a single DT binary, encoded using DT syntax.
Ths would create a dichotomy in the DT bindings, in the sense that we would have bindings applicable to the boot loader only, and bindings for the OS, but this is already the case in what you're proposing here as the panel-id property is documented as not used by the OS itself.
Quoted 38 lines
Quoted 38 lines
This needs to be handled in the firmware (or, if not possible, in a kernel board driver). The above DT fragment, visible to the kernel, doesn't describe the actual hardware. Furthermore, you would require all bridge drivers to be patched to support this method, which shows again that the issue isn't handled in the right place.
Finally, unless I'm mistaken, this series is meant to support display for an ACPI-based ARM machine. Using DT as a stop-gap measure because ACPI support isn't there yet is fine out-of-tree, and fine by me in-tree provided that the DT bindings are clean, but not when DT is abused like this.
I'm sorry, but this is a NACK from me. Please handle this transparently in the firmware if you want DT-based boot, or with ACPI.
Quoted 23 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +20
-0+ + For a device with multiple potential panels, a node for each potential + should be defined with status = "disabled", and an appropriate panel-id + property. The video data producer should be setup with endpoints going to + each possible panel. The firmware will find the dt node with a panel-id + matching the actual panel installed, and change it's status to "ok". + + The exact method the firmware uses to determine the panel-id of the installed + panel is outside the scope of this binding, but a few examples are + + 1) u-boot module reading a value from a u-boot env var + 2) EFI driver module reading a value from an EFI variable + 3) device specific firmware reading some device specific GPIOs or + e-fuse + + The panel-id values are an opaque integer. They can be sparse. The only + important thing is that each possible panel in the system has a unique + panel-id, and that the values configured in the device's DTB match the + values that the firmware is looking for. + ddc-i2c-bus: $ref: /schemas/types.yaml#/definitions/phandle description:
Signature · 7 lines
Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
Quoted 99 lines · Laurent Pinchart
Hi Rob,
Quoted 95 lines · Rob Clark
Quoted 77 lines · Laurent Pinchart
Quoted 76 lines · Rob Clark
From: Rob Clark <[email protected]>
For devices that have one of several possible panels installed, the panel-id property gives firmware a generic way to locate and enable the panel node corresponding to the installed panel. Example of how to use this property:
ivo_panel { compatible = "ivo,m133nwf4-r0"; 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>; }; }; }; };Signed-off-by Rob Clark <[email protected]>--- .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)Documentation/devicetree/bindings/display/panel/panel-common.yaml +0
-0@@ -75,6 +75,32 @@ properties: in the device graph bindings defined in Documentation/devicetree/bindings/graph.txt.Quoted 11 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml properties: · line 83
Quoted 6 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +6
-0+ panel-id: + description: + To support the case where one of several different panels can be installed + on a device, the panel-id property can be used by the firmware to identify + which panel should have it's status changed to "ok". This property is not + used by the HLOS itself.If your firmware can modify the status property of a panel, it can also add DT nodes. As discussed before, I don't think this belongs to DT. Even if panel-id isn't used by the operating system, you have Linux kernel patches in this series that show that this isn't transparent.
I've already explained several times why this is not feasible. It would require DtbLoader to be familiar with each individual device, and be rev'd every time a new device appears. That is not practical at all.
Quoted 9 lines
Quoted 6 lines
(And fwiw, the ACPI tables describe each panel.. with an ACPI method that is passed the the panel-id and returns the appropriate table.. since DT doesn't have methods, this is the solution.)
I stand by this patch, we can't keep running away from this problem and wave the magic firmware wand.
I believe in firmware solutions more than firmware magic wands :-)
and with that in mind, I think I've come up with a firmware solution, in the form of dtb overlays :-)
I've managed to get DtbLoader to find and load a panel overlay based on the panel-id it reads, which drops all patches in the patchset except the last one, which now has this delta:
---------
(quoted diff) +0 -0
b/arch/arm64/boot/dts/qcom/Makefile
index 6498a1ec893f..1a61e8da2521 100644
arch/arm64/boot/dts/qcom/Makefile +1 -0
@@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +subdir-y += panels dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb dtb-$(CONFIG_ARCH_QCOM) += ipq8074-hk01.dtb
(quoted diff) +0 -0
b/arch/arm64/boot/dts/qcom/panels/Makefile
new file mode 100644
index 000000000000..dbf55f423555
arch/arm64/boot/dts/qcom/panels/Makefile new file +3 -0
@@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0 +dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb +dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtb
(quoted diff) +0 -0
b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
new file mode 100644
index 000000000000..ebcf65419dad
arch/arm64/boot/dts/qcom/panels/panel-c4.dts new file +17 -0
@@ -0,0 +1,17 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Panel overlay for panel-id 0xc4 + * + * Copyright (c) 2019, Linaro Ltd. + */ + +/dts-v1/; +/plugin/; +/ { + fragment@0 { + target-path = "/panel"; + __overlay__ { + compatible = "boe,nv133fhm-n61"; + }; + }; +};
(quoted diff) +0 -0
b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
new file mode 100644
index 000000000000..0ad5bb6003e3
arch/arm64/boot/dts/qcom/panels/panel-c5.dts new file +17 -0
@@ -0,0 +1,17 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Panel overlay for panel-id 0xc5 + * + * Copyright (c) 2019, Linaro Ltd. + */ + +/dts-v1/; +/plugin/; +/ { + fragment@0 { + target-path = "/panel"; + __overlay__ { + compatible = "ivo,m133nwf4-r0"; + }; + }; +};
(quoted diff) +0 -0
b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts index c35d8099d8eb..92c76afb721c 100644
arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts +4 -2
@@ -22,11 +22,13 @@ hsuart0 = &uart6; }; + /* + * stub node which defines how panel is connected to bridge, which + * will be updated by panel specific overlay + */ panel { - compatible = "ivo,m133nwf4-r0"; power-supply = <&vlcm_3v3>; no-hpd; - ports { port { panel_in_edp: endpoint {
---------
Side note, try as I might, I couldn't get the 'target = <&phandle>' approach to work in the overlays, so I ended up going with target-path instead. From digging thru the fdt_overlay code, I *think* it is because I end up w/ an overlay dtb without symbols. In the end, I guess target-path works just as well.
BR,
-R
Laurent Pinchart
reviewerRe: [PATCH 1/4] dt-bindings: display: panel: document panel-id
Hi Rob,
Quoted 104 lines · Rob Clark
Quoted 103 lines · Laurent Pinchart
Quoted 101 lines · Rob Clark
Quoted 77 lines · Laurent Pinchart
Quoted 76 lines · Rob Clark
From: Rob Clark <[email protected]>
For devices that have one of several possible panels installed, the panel-id property gives firmware a generic way to locate and enable the panel node corresponding to the installed panel. Example of how to use this property:
ivo_panel { compatible = "ivo,m133nwf4-r0"; 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>; }; }; }; };Signed-off-by Rob Clark <[email protected]>--- .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)Documentation/devicetree/bindings/display/panel/panel-common.yaml +0
-0@@ -75,6 +75,32 @@ properties: in the device graph bindings defined in Documentation/devicetree/bindings/graph.txt.Quoted 11 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml properties: · line 83
Quoted 6 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +6
-0+ panel-id: + description: + To support the case where one of several different panels can be installed + on a device, the panel-id property can be used by the firmware to identify + which panel should have it's status changed to "ok". This property is not + used by the HLOS itself.If your firmware can modify the status property of a panel, it can also add DT nodes. As discussed before, I don't think this belongs to DT. Even if panel-id isn't used by the operating system, you have Linux kernel patches in this series that show that this isn't transparent.
I've already explained several times why this is not feasible. It would require DtbLoader to be familiar with each individual device, and be rev'd every time a new device appears. That is not practical at all.
(And fwiw, the ACPI tables describe each panel.. with an ACPI method that is passed the the panel-id and returns the appropriate table.. since DT doesn't have methods, this is the solution.)
I stand by this patch, we can't keep running away from this problem and wave the magic firmware wand.
Quoted 8 lines
Quoted 1 line
I believe in firmware solutions more than firmware magic wands :-)
and with that in mind, I think I've come up with a firmware solution, in the form of dtb overlays :-)
I've managed to get DtbLoader to find and load a panel overlay based on the panel-id it reads, which drops all patches in the patchset except the last one, which now has this delta:
Thank you for looking into this, I really like the outcome :-)
Quoted 98 lines
---------
arch/arm64/boot/dts/qcom/Makefile +1
-0@@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +subdir-y += panels dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb dtb-$(CONFIG_ARCH_QCOM) += ipq8074-hk01.dtbarch/arm64/boot/dts/qcom/panels/Makefile new file +3
-0@@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0 +dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb +dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtbarch/arm64/boot/dts/qcom/panels/panel-c4.dts new file +17
-0@@ -0,0 +1,17 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Panel overlay for panel-id 0xc4 + * + * Copyright (c) 2019, Linaro Ltd. + */ + +/dts-v1/; +/plugin/; +/ { + fragment@0 { + target-path = "/panel"; + __overlay__ { + compatible = "boe,nv133fhm-n61"; + }; + }; +};arch/arm64/boot/dts/qcom/panels/panel-c5.dts new file +17
-0@@ -0,0 +1,17 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Panel overlay for panel-id 0xc5 + * + * Copyright (c) 2019, Linaro Ltd. + */ + +/dts-v1/; +/plugin/; +/ { + fragment@0 { + target-path = "/panel"; + __overlay__ { + compatible = "ivo,m133nwf4-r0"; + }; + }; +};arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts +4
-3@@ -22,11 +22,13 @@ hsuart0 = &uart6; }; + /* + * stub node which defines how panel is connected to bridge, which + * will be updated by panel specific overlay + */ panel { - compatible = "ivo,m133nwf4-r0"; power-supply = <&vlcm_3v3>; no-hpd; - ports { port { panel_in_edp: endpoint { ---------Side note, try as I might, I couldn't get the 'target = <&phandle>' approach to work in the overlays, so I ended up going with target-path instead. From digging thru the
fdt_overlaycode, I *think* it is because I end up w/ an overlay dtb without symbols. In the end, I guess target-path works just as well.
Signature · 7 lines
Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
In reply to Rob ClarkQuoted 112 lines · Rob Clark
Quoted 99 lines · Laurent Pinchart
Hi Rob,
Quoted 95 lines · Rob Clark
Quoted 77 lines · Laurent Pinchart
Quoted 76 lines · Rob Clark
From: Rob Clark <[email protected]>
For devices that have one of several possible panels installed, the panel-id property gives firmware a generic way to locate and enable the panel node corresponding to the installed panel. Example of how to use this property:
ivo_panel { compatible = "ivo,m133nwf4-r0"; 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>; }; }; }; };Signed-off-by Rob Clark <[email protected]>--- .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)Documentation/devicetree/bindings/display/panel/panel-common.yaml +0
-0@@ -75,6 +75,32 @@ properties: in the device graph bindings defined in Documentation/devicetree/bindings/graph.txt.Quoted 11 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml properties: · line 83
Quoted 6 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +6
-0+ panel-id: + description: + To support the case where one of several different panels can be installed + on a device, the panel-id property can be used by the firmware to identify + which panel should have it's status changed to "ok". This property is not + used by the HLOS itself.If your firmware can modify the status property of a panel, it can also add DT nodes. As discussed before, I don't think this belongs to DT. Even if panel-id isn't used by the operating system, you have Linux kernel patches in this series that show that this isn't transparent.
I've already explained several times why this is not feasible. It would require DtbLoader to be familiar with each individual device, and be rev'd every time a new device appears. That is not practical at all.
Quoted 9 lines
Quoted 6 lines
(And fwiw, the ACPI tables describe each panel.. with an ACPI method that is passed the the panel-id and returns the appropriate table.. since DT doesn't have methods, this is the solution.)
I stand by this patch, we can't keep running away from this problem and wave the magic firmware wand.
I believe in firmware solutions more than firmware magic wands :-)
Quoted 6 lines
and with that in mind, I think I've come up with a firmware solution, in the form of dtb overlays :-)
I've managed to get DtbLoader to find and load a panel overlay based on the panel-id it reads, which drops all patches in the patchset except the last one, which now has this delta:
This looks good to me. The only slight concern I have with it is making the overlay filename an ABI. I don't have a better suggestion though. How would this work for other vendors or the same panel ID (for different panels) used on different platforms? For different vendors at least, I guess dtbloader gets the base dtb path somehow and the overlay's are relative to that?
Quoted 93 lines
---------
arch/arm64/boot/dts/qcom/Makefile +1
-0@@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +subdir-y += panels dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb dtb-$(CONFIG_ARCH_QCOM) += ipq8074-hk01.dtbarch/arm64/boot/dts/qcom/panels/Makefile new file +3
-0@@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0 +dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb +dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtbarch/arm64/boot/dts/qcom/panels/panel-c4.dts new file +17
-0@@ -0,0 +1,17 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Panel overlay for panel-id 0xc4 + * + * Copyright (c) 2019, Linaro Ltd. + */ + +/dts-v1/; +/plugin/; +/ { + fragment@0 { + target-path = "/panel"; + __overlay__ { + compatible = "boe,nv133fhm-n61"; + }; + }; +};arch/arm64/boot/dts/qcom/panels/panel-c5.dts new file +17
-0@@ -0,0 +1,17 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Panel overlay for panel-id 0xc5 + * + * Copyright (c) 2019, Linaro Ltd. + */ + +/dts-v1/; +/plugin/; +/ { + fragment@0 { + target-path = "/panel"; + __overlay__ { + compatible = "ivo,m133nwf4-r0"; + }; + }; +};arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts +4
-3@@ -22,11 +22,13 @@ hsuart0 = &uart6; }; + /* + * stub node which defines how panel is connected to bridge, which + * will be updated by panel specific overlay + */ panel { - compatible = "ivo,m133nwf4-r0"; power-supply = <&vlcm_3v3>; no-hpd; - ports { port { panel_in_edp: endpoint { ---------
Quoted 5 lines
Side note, try as I might, I couldn't get the 'target = <&phandle>' approach to work in the overlays, so I ended up going with target-path instead. From digging thru the
fdt_overlaycode, I *think* it is because I end up w/ an overlay dtb without symbols. In the end, I guess target-path works just as well.
It's the base dtb that needs the symbols I think.
BTW, to answer the question on #dri-devel, if you wanted to put the full panel into an overlay, the way to solve the problem of having bridge specific knowledge is defining a connector node. That should provide enough abstraction. Presumably the connector is actually the same across panels in this situation, so that should match up with the actual h/w. It could be possible to have a different physical connector populated for each possible panel, but hopefully that's not the common case.
Rob
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
Quoted 121 lines · Rob Herring
Quoted 112 lines · Rob Clark
Quoted 99 lines · Laurent Pinchart
Hi Rob,
Quoted 95 lines · Rob Clark
Quoted 77 lines · Laurent Pinchart
Quoted 76 lines · Rob Clark
From: Rob Clark <[email protected]>
For devices that have one of several possible panels installed, the panel-id property gives firmware a generic way to locate and enable the panel node corresponding to the installed panel. Example of how to use this property:
ivo_panel { compatible = "ivo,m133nwf4-r0"; 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>; }; }; }; };Signed-off-by Rob Clark <[email protected]>--- .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)Documentation/devicetree/bindings/display/panel/panel-common.yaml +0
-0@@ -75,6 +75,32 @@ properties: in the device graph bindings defined in Documentation/devicetree/bindings/graph.txt.Quoted 11 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml properties: · line 83
Quoted 6 lines
Documentation/devicetree/bindings/display/panel/panel-common.yaml +6
-0+ panel-id: + description: + To support the case where one of several different panels can be installed + on a device, the panel-id property can be used by the firmware to identify + which panel should have it's status changed to "ok". This property is not + used by the HLOS itself.If your firmware can modify the status property of a panel, it can also add DT nodes. As discussed before, I don't think this belongs to DT. Even if panel-id isn't used by the operating system, you have Linux kernel patches in this series that show that this isn't transparent.
I've already explained several times why this is not feasible. It would require DtbLoader to be familiar with each individual device, and be rev'd every time a new device appears. That is not practical at all.
Quoted 9 lines
Quoted 6 lines
(And fwiw, the ACPI tables describe each panel.. with an ACPI method that is passed the the panel-id and returns the appropriate table.. since DT doesn't have methods, this is the solution.)
I stand by this patch, we can't keep running away from this problem and wave the magic firmware wand.
I believe in firmware solutions more than firmware magic wands :-)
Quoted 6 lines
and with that in mind, I think I've come up with a firmware solution, in the form of dtb overlays :-)
I've managed to get DtbLoader to find and load a panel overlay based on the panel-id it reads, which drops all patches in the patchset except the last one, which now has this delta:
Quoted 6 lines
This looks good to me. The only slight concern I have with it is making the overlay filename an ABI. I don't have a better suggestion though. How would this work for other vendors or the same panel ID (for different panels) used on different platforms? For different vendors at least, I guess dtbloader gets the base dtb path somehow and the overlay's are relative to that?
Not sure if "different vendors" in this context means different OEMs/ODMs, or different SoC's? This solution is snapdragon specific.. and in this case the panel id seems to be a flat namespace (I don't see re-use for different panels). But in DtbLoader I attempt loading a device specific path first, just in case. In particular the paths DtbLoader uses (for dtb and panel overlay) are (where $SysVendor/$ProductName/$BoardName come from SMBIOS tables and $PanelId comes from a qcom specific UEFIDisplayInfo variable) described below:
It tries to load dtb from (in order, paths relative to EFI system partition where DtbLoader is installed):
1) \dtb\$SysVendor\$ProductName-$BoardName.dtb
2) \dtb\$SysVendor\$ProductName.dtb
and panel overlay dtb from:
1) \dtb\$SysVendor\$ProductName-$BoardName-panel-$PanelId.dtb
2) \dtb\$SysVendor\$ProductName-panel-$PanelId.dtb
3) \dtb\panels\panel-$PanelId.dtb
We are already using different dtb names for the main dtb compared to what the kernel uses. Which isn't great. At some point we might want to add SysVendor/ProductName/BoardName fields in the dtb so we could automate renaming and stuffing the dtb's into the correct layout.
Quoted 101 lines
Quoted 93 lines
---------
arch/arm64/boot/dts/qcom/Makefile +1
-0@@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +subdir-y += panels dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb dtb-$(CONFIG_ARCH_QCOM) += ipq8074-hk01.dtbarch/arm64/boot/dts/qcom/panels/Makefile new file +3
-0@@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0 +dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb +dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtbarch/arm64/boot/dts/qcom/panels/panel-c4.dts new file +17
-0@@ -0,0 +1,17 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Panel overlay for panel-id 0xc4 + * + * Copyright (c) 2019, Linaro Ltd. + */ + +/dts-v1/; +/plugin/; +/ { + fragment@0 { + target-path = "/panel"; + __overlay__ { + compatible = "boe,nv133fhm-n61"; + }; + }; +};arch/arm64/boot/dts/qcom/panels/panel-c5.dts new file +17
-0@@ -0,0 +1,17 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Panel overlay for panel-id 0xc5 + * + * Copyright (c) 2019, Linaro Ltd. + */ + +/dts-v1/; +/plugin/; +/ { + fragment@0 { + target-path = "/panel"; + __overlay__ { + compatible = "ivo,m133nwf4-r0"; + }; + }; +};arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts +4
-3@@ -22,11 +22,13 @@ hsuart0 = &uart6; }; + /* + * stub node which defines how panel is connected to bridge, which + * will be updated by panel specific overlay + */ panel { - compatible = "ivo,m133nwf4-r0"; power-supply = <&vlcm_3v3>; no-hpd; - ports { port { panel_in_edp: endpoint { ---------Quoted 5 lines
Side note, try as I might, I couldn't get the 'target = <&phandle>' approach to work in the overlays, so I ended up going with target-path instead. From digging thru the
fdt_overlaycode, I *think* it is because I end up w/ an overlay dtb without symbols. In the end, I guess target-path works just as well.It's the base dtb that needs the symbols I think.
Quoted 9 lines
BTW, to answer the question on #dri-devel, if you wanted to put the full panel into an overlay, the way to solve the problem of having bridge specific knowledge is defining a connector node. That should provide enough abstraction. Presumably the connector is actually the same across panels in this situation, so that should match up with the actual h/w. It could be possible to have a different physical connector populated for each possible panel, but hopefully that's not the common case.
ok, I'm not too familiar with this connector node thing. I think in the end, it is really just the compatible string that differs (ie. power-supply, etc would all be the same for each possible panel). But it might be worth trying this connector node thing
BR,
-R
PATCH 2/4 drm/of: add support to find any enabled endpoint
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
To handle the case where there are multiple panel endpoints, only one of which is enabled/installed, add support for a wildcard endpoint value to request finding whichever endpoint is enabled.
--- drivers/gpu/drm/drm_of.c | 41 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)
drivers/gpu/drm/drm_of.c +40 -1
@@ -219,11 +219,44 @@ int drm_of_encoder_active_endpoint(struct device_node *node, }EXPORT_SYMBOL_GPL (drm_of_encoder_active_endpoint); +static int find_enabled_endpoint (const struct device_node *node,u32 port) +{ +struct device_node *endpoint_node, *remote; +u32 endpoint =0 ; + +for (endpoint =0 ; ; endpoint++) { + endpoint_node =of_graph_get_endpoint_by_regs (node, port, endpoint); +if (!endpoint_node) { +pr_debug ("No more endpoints!\n" ); +return -ENODEV ; + } + + remote =of_graph_get_remote_port_parent (endpoint_node); +of_node_put (endpoint_node); +if (!remote) { +pr_debug ("no valid remote node\n" ); +continue ; + } + +if (!of_device_is_available (remote)) { +pr_debug ("not available for remote node\n" ); +of_node_put (remote); +continue ; + } + +pr_debug ("found enabled endpoint %d for %s\n" , endpoint, remote->name); +of_node_put (remote); +return endpoint; + } + +return -ENODEV ; +} +/** * drm_of_find_panel_or_bridge - return connected panel or bridge device * @np: device tree node containing encoder output ports * @port: port in the device tree node -* @endpoint: endpoint in the device tree node +* @endpoint: endpoint in the device tree node, or -1 to find an enabled endpoint * @panel: pointer to hold returned drm_panel * @bridge: pointer to hold returned drm_bridge *
@@ -246,6 +279,12 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,if (panel) *panel =NULL ; +if (endpoint ==-1 ) { + endpoint =find_enabled_endpoint (np, port); +if (endpoint <0 ) +return endpoint; + } + remote =of_graph_get_remote_node (np, port, endpoint);if (!remote)return -ENODEV ;
Signature · 6 lines
2.23.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/4] drm/of: add support to find any enabled endpoint
Hi Rob.
Patch looks good, one small improvement proposal.
Quoted 41 lines · Rob Clark
From: Rob Clark <[email protected]>
To handle the case where there are multiple panel endpoints, only one of which is enabled/installed, add support for a wildcard endpoint value to request finding whichever endpoint is enabled.
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/drm_of.c | 41 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)drivers/gpu/drm/drm_of.c +22
-0@@ -219,11 +219,44 @@ int drm_of_encoder_active_endpoint(struct device_node *node, }EXPORT_SYMBOL_GPL (drm_of_encoder_active_endpoint); +static int find_enabled_endpoint (const struct device_node *node,u32 port) +{ +struct device_node *endpoint_node, *remote; +u32 endpoint =0 ; + +for (endpoint =0 ; ; endpoint++) { + endpoint_node =of_graph_get_endpoint_by_regs (node, port, endpoint); +if (!endpoint_node) { +pr_debug ("No more endpoints!\n" ); +return -ENODEV ; + } + + remote =of_graph_get_remote_port_parent (endpoint_node); +of_node_put (endpoint_node); +if (!remote) { +pr_debug ("no valid remote node\n" ); +continue ; + } + +if (!of_device_is_available (remote)) { +pr_debug ("not available for remote node\n" ); +of_node_put (remote);
drivers/gpu/drm/drm_of.c int drm_of_encoder_active_endpoint(struct device_node *node, · line 253
Quoted 10 lines
drivers/gpu/drm/drm_of.c +10
-0+continue ; + } + +pr_debug ("found enabled endpoint %d for %s\n" , endpoint, remote->name); +of_node_put (remote); +return endpoint; + } + +return -ENODEV ; +}
This function seems pretty generic. Should this be part of drivers/of/property.c - as others may have the same need?
drivers/gpu/drm/drm_of.c int drm_of_encoder_active_endpoint(struct device_node *node, · line 261
Quoted 9 lines
drivers/gpu/drm/drm_of.c +2
-1+/** * drm_of_find_panel_or_bridge - return connected panel or bridge device * @np: device tree node containing encoder output ports * @port: port in the device tree node -* @endpoint: endpoint in the device tree node +* @endpoint: endpoint in the device tree node, or -1 to find an enabled endpoint * @panel: pointer to hold returned drm_panel * @bridge: pointer to hold returned drm_bridge
Introducing a define would make it easier to use this function in the right way.
For example:
#define OF_ENDPOINT_FIRST -1
Then we would see this code in the user side:
+ ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1,
OF_ENDPOINT_FIRST, &pdata->panel, NULL);
Or something like this.
Sam
Quoted 21 lines
drivers/gpu/drm/drm_of.c +6
-0* @@ -246,6 +279,12 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,if (panel) *panel =NULL ; +if (endpoint ==-1 ) { + endpoint =find_enabled_endpoint (np, port); +if (endpoint <0 ) +return endpoint; + } + remote =of_graph_get_remote_node (np, port, endpoint);if (!remote)return -ENODEV ;Signature · 6 lines
2.23.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 3/4 drm/bridge: ti-sn65dsi86: find any enabled endpoint
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
This bridge is used on a number of devices that can have one of multiple different panels installed. The firmware will enable the panel driver node for the panel that is actually installed. So the bridge should ask drm_of_find_panel_or_bridge() to find the endpoint for the enabled panel.
--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/bridge/ti-sn65dsi86.c +1 -1
@@ -720,7 +720,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client, pdata->dev = &client->dev; - ret =drm_of_find_panel_or_bridge (pdata->dev->of_node,1 ,0 , + ret =drm_of_find_panel_or_bridge (pdata->dev->of_node,1 ,-1 , &pdata->panel,NULL );if (ret) {DRM_ERROR ("could not find any panel node\n" );
Signature · 6 lines
2.23.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 4/4 arm64: dts: qcom: c630: Enable display
In reply to Rob ClarkFrom: Bjorn Andersson <[email protected]>
[Initial patch from Bjorn, I've added some regulator-boot-on's to
account for display related regulators enabled by the firmware, and updated to handle the two possible panels that can be installed.]
--- .../boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 165 ++++++++++++++++++ 1 file changed, 165 insertions(+)
arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts +165 -0
@@ -7,6 +7,7 @@ /dts-v1/; +#include <dt-bindings/gpio/gpio.h> #include <dt-bindings/regulator/qcom,rpmh-regulator.h> #include "sdm845.dtsi" #include "pm8998.dtsi"
@@ -18,6 +19,70 @@ aliases { hsuart0 = &uart6; }; + + ivo_panel { + compatible = "ivo,m133nwf4-r0"; + panel-id = <0xc5>; + status = "disabled"; + power-supply = <&vlcm_3v3>; + no-hpd; + + ports { + port { + ivo_panel_in_edp: endpoint { + remote-endpoint = <&sn65dsi86_out_ivo>; + }; + }; + }; + }; + + boe_panel { + compatible = "boe,nv133fhm-n61"; + panel-id = <0xc4>; + status = "disabled"; + power-supply = <&vlcm_3v3>; + no-hpd; + + ports { + port { + boe_panel_in_edp: endpoint { + remote-endpoint = <&sn65dsi86_out_boe>; + }; + }; + }; + }; + + vlcm_3v3: vlcm-3v3-power { + compatible = "regulator-fixed"; + regulator-name = "VLCM_3V3"; + + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + + gpio = <&tlmm 88 GPIO_ACTIVE_HIGH>; + enable-active-high; + regulator-boot-on; + }; + + sw_edp_1p2: sw-edp-1p2-regulator { + compatible = "regulator-fixed"; + regulator-name = "SW_EDP_1P2"; + + vin-supply = <&vreg_l2a_1p2>; + regulator-min-microvolt = <1200000>; + regulator-max-microvolt = <1200000>; + + gpio = <&pm8998_gpio 9 GPIO_ACTIVE_HIGH>; + enable-active-high; + regulator-boot-on; + }; + + sn65dsi86_refclk: sn65dsi86-refclk { + compatible = "fixed-clock"; + #clock-cells = <0>; + + clock-frequency = <19200000>; + }; }; &adsp_pas {
@@ -79,6 +144,7 @@ regulator-min-microvolt = <880000>; regulator-max-microvolt = <880000>; regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; + regulator-boot-on; }; vddpx_10:
@@ -216,6 +282,7 @@ regulator-min-microvolt = <1200000>; regulator-max-microvolt = <1208000>; regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; + regulator-boot-on; }; vreg_l28a_3p0: ldo28 {
@@ -239,6 +306,25 @@ status = "okay"; }; +&dsi0 { + status = "okay"; + vdda-supply = <&vreg_l26a_1p2>; + + ports { + port@1 { + endpoint { + remote-endpoint = <&sn65dsi86_in_a>; + data-lanes = <0 1 2 3>; + }; + }; + }; +}; + +&dsi0_phy { + status = "okay"; + vdds-supply = <&vreg_l1a_0p875>; +}; + &gcc { protected-clocks = <GCC_QSPI_CORE_CLK>, <GCC_QSPI_CORE_CLK_SRC>,
@@ -290,6 +376,58 @@ }; }; +&i2c10 { + status = "okay"; + clock-frequency = <400000>; + + sn65dsi86: bridge@2c { + compatible = "ti,sn65dsi86"; + reg = <0x2c>; + pinctrl-names = "default"; + pinctrl-0 = <&edp_bridge_en>, <&edp_bridge_irq>; + + interrupts-extended = <&tlmm 10 IRQ_TYPE_LEVEL_HIGH>; + + enable-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>; + + vpll-supply = <&vreg_l14a_1p88>; + vccio-supply = <&vreg_l14a_1p88>; + vcca-supply = <&sw_edp_1p2>; + vcc-supply = <&sw_edp_1p2>; + + clocks = <&sn65dsi86_refclk>; + clock-names = "refclk"; + + max-brightness = <255>; + + 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_ivo: endpoint@c5 { + reg = <0>; + remote-endpoint = <&ivo_panel_in_edp>; + }; + + sn65dsi86_out_boe: endpoint@c4 { + reg = <1>; + remote-endpoint = <&boe_panel_in_edp>; + }; + }; + }; + }; +}; + &i2c11 { status = "okay"; clock-frequency = <400000>;
@@ -306,6 +444,14 @@ }; }; +&mdss { + status = "okay"; +}; + +&mdss_mdp { + status = "okay"; +}; + &mss_pil { firmware-name = "qcom/LENOVO/81JL/qcdsp1v2850.mbn", "qcom/LENOVO/81JL/qcdsp2850.mbn"; };
@@ -338,6 +484,14 @@ }; }; +&qup_i2c10_default { + pinconf { + pins = "gpio55", "gpio56"; + drive-strength = <2>; + bias-disable; + }; +}; + &qupv3_id_0 { status = "okay"; };
@@ -349,6 +503,17 @@ &tlmm { gpio-reserved-ranges = <0 4>, <81 4>; + edp_bridge_en: edp-bridge-enable { + pins = "gpio96"; + drive-strength = <2>; + bias-disable; + }; + + edp_bridge_irq: edp-bridge-irq { + pins = "gpio10"; + bias-pull-down; + }; + i2c2_hid_active: i2c2-hid-active { pins = <37>; function = "gpio";
Signature · 6 lines
2.23.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel