* [PATCH 0/4] drm+dt: multi panel selection and yoga c630 display
@ 2019-12-07 20:35 ` Rob Clark
0 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 20:35 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, aarch64-laptops
Cc: Laurent Pinchart, Vasily Khoruzhick, Bjorn Andersson,
Jeffrey Hugo, Rob Clark,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jernej Skrabec, Jonas Karlman, open list, Sam Ravnborg
From: Rob Clark <robdclark@chromium.org>
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(-)
--
2.23.0
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 0/4] drm+dt: multi panel selection and yoga c630 display
@ 2019-12-07 20:35 ` Rob Clark
0 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 20:35 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, aarch64-laptops
Cc: Rob Clark,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jernej Skrabec, Jonas Karlman, Jeffrey Hugo, open list,
Bjorn Andersson, Vasily Khoruzhick, Laurent Pinchart,
Sam Ravnborg
From: Rob Clark <robdclark@chromium.org>
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(-)
--
2.23.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/4] dt-bindings: display: panel: document panel-id
2019-12-07 20:35 ` Rob Clark
@ 2019-12-07 20:35 ` Rob Clark
-1 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 20:35 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, aarch64-laptops
Cc: Laurent Pinchart, Vasily Khoruzhick, Bjorn Andersson,
Jeffrey Hugo, Rob Clark, Thierry Reding, Sam Ravnborg,
David Airlie, Daniel Vetter, Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
From: Rob Clark <robdclark@chromium.org>
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 <robdclark@chromium.org>
---
.../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
index ef8d8cdfcede..6113319b91dd 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
+++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
@@ -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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 1/4] dt-bindings: display: panel: document panel-id
@ 2019-12-07 20:35 ` Rob Clark
0 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 20:35 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, aarch64-laptops
Cc: Rob Clark, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, open list, Rob Herring, Bjorn Andersson,
Vasily Khoruzhick, David Airlie, Thierry Reding,
Laurent Pinchart, Sam Ravnborg
From: Rob Clark <robdclark@chromium.org>
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 <robdclark@chromium.org>
---
.../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
index ef8d8cdfcede..6113319b91dd 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
+++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
@@ -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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/4] drm/of: add support to find any enabled endpoint
2019-12-07 20:35 ` Rob Clark
@ 2019-12-07 20:35 ` Rob Clark
-1 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 20:35 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, aarch64-laptops
Cc: Laurent Pinchart, Vasily Khoruzhick, Bjorn Andersson,
Jeffrey Hugo, Rob Clark, Maarten Lankhorst, Maxime Ripard,
Sean Paul, David Airlie, Daniel Vetter, open list
From: Rob Clark <robdclark@chromium.org>
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 <robdclark@chromium.org>
---
drivers/gpu/drm/drm_of.c | 41 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 40 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 0ca58803ba46..2baf44e401b8 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -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;
--
2.23.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/4] drm/of: add support to find any enabled endpoint
@ 2019-12-07 20:35 ` Rob Clark
0 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 20:35 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, aarch64-laptops
Cc: Rob Clark, Jeffrey Hugo, open list, Bjorn Andersson,
Vasily Khoruzhick, David Airlie, Laurent Pinchart, Sean Paul
From: Rob Clark <robdclark@chromium.org>
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 <robdclark@chromium.org>
---
drivers/gpu/drm/drm_of.c | 41 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 40 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 0ca58803ba46..2baf44e401b8 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -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;
--
2.23.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 3/4] drm/bridge: ti-sn65dsi86: find any enabled endpoint
2019-12-07 20:35 ` Rob Clark
@ 2019-12-07 20:35 ` Rob Clark
-1 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 20:35 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, aarch64-laptops
Cc: Laurent Pinchart, Vasily Khoruzhick, Bjorn Andersson,
Jeffrey Hugo, Rob Clark, Andrzej Hajda, Neil Armstrong,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
open list
From: Rob Clark <robdclark@chromium.org>
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.
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 43abf01ebd4c..62bc98d9d152 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -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");
--
2.23.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 3/4] drm/bridge: ti-sn65dsi86: find any enabled endpoint
@ 2019-12-07 20:35 ` Rob Clark
0 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 20:35 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, aarch64-laptops
Cc: Rob Clark, Jernej Skrabec, Neil Armstrong, Jeffrey Hugo,
David Airlie, Jonas Karlman, open list, Bjorn Andersson,
Vasily Khoruzhick, Laurent Pinchart
From: Rob Clark <robdclark@chromium.org>
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.
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 43abf01ebd4c..62bc98d9d152 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -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");
--
2.23.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 4/4] arm64: dts: qcom: c630: Enable display
2019-12-07 20:35 ` Rob Clark
@ 2019-12-07 20:35 ` Rob Clark
-1 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 20:35 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, aarch64-laptops
Cc: Laurent Pinchart, Vasily Khoruzhick, Bjorn Andersson,
Jeffrey Hugo, Rob Clark, Andy Gross, Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
From: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
[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.]
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
.../boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 165 ++++++++++++++++++
1 file changed, 165 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
index 13dc619687f3..459f65e3eb53 100644
--- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
+++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
@@ -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";
--
2.23.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 4/4] arm64: dts: qcom: c630: Enable display
@ 2019-12-07 20:35 ` Rob Clark
0 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 20:35 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, aarch64-laptops
Cc: Rob Clark, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, open list, Rob Herring, Bjorn Andersson,
Vasily Khoruzhick, Andy Gross, Laurent Pinchart
From: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
[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.]
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
.../boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 165 ++++++++++++++++++
1 file changed, 165 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
index 13dc619687f3..459f65e3eb53 100644
--- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
+++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
@@ -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";
--
2.23.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
2019-12-07 20:35 ` Rob Clark
@ 2019-12-07 21:13 ` Sam Ravnborg
-1 siblings, 0 replies; 32+ messages in thread
From: Sam Ravnborg @ 2019-12-07 21:13 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, linux-arm-msm, aarch64-laptops, Rob Clark,
Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, open list, Rob Herring, Bjorn Andersson,
Vasily Khoruzhick, David Airlie, Thierry Reding,
Laurent Pinchart
On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> 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
> 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 <robdclark@chromium.org>
> ---
> .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> index ef8d8cdfcede..6113319b91dd 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> @@ -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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
@ 2019-12-07 21:13 ` Sam Ravnborg
0 siblings, 0 replies; 32+ messages in thread
From: Sam Ravnborg @ 2019-12-07 21:13 UTC (permalink / raw)
To: Rob Clark
Cc: Rob Clark, Mark Rutland, aarch64-laptops,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, linux-arm-msm, open list, dri-devel,
Bjorn Andersson, Vasily Khoruzhick, David Airlie, Rob Herring,
Thierry Reding, Laurent Pinchart
On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> 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
> 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 <robdclark@chromium.org>
> ---
> .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> index ef8d8cdfcede..6113319b91dd 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> @@ -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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
2019-12-07 21:13 ` Sam Ravnborg
@ 2019-12-07 21:34 ` Rob Clark
-1 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 21:34 UTC (permalink / raw)
To: Sam Ravnborg
Cc: dri-devel, linux-arm-msm, aarch64-laptops, Rob Clark,
Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, open list, Rob Herring, Bjorn Andersson,
Vasily Khoruzhick, David Airlie, Thierry Reding,
Laurent Pinchart
On Sat, Dec 7, 2019 at 1:13 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > 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.
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.
>
> 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
> 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
>
> Sam
>
>
>
>
> > 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 <robdclark@chromium.org>
> > ---
> > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > index ef8d8cdfcede..6113319b91dd 100644
> > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > @@ -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
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
@ 2019-12-07 21:34 ` Rob Clark
0 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-07 21:34 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Rob Clark, Mark Rutland, aarch64-laptops,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, linux-arm-msm, open list, dri-devel,
Bjorn Andersson, Vasily Khoruzhick, David Airlie, Rob Herring,
Thierry Reding, Laurent Pinchart
On Sat, Dec 7, 2019 at 1:13 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > 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.
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.
>
> 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
> 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
>
> Sam
>
>
>
>
> > 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 <robdclark@chromium.org>
> > ---
> > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > index ef8d8cdfcede..6113319b91dd 100644
> > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > @@ -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
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] drm/of: add support to find any enabled endpoint
2019-12-07 20:35 ` Rob Clark
@ 2019-12-08 9:31 ` Sam Ravnborg
-1 siblings, 0 replies; 32+ messages in thread
From: Sam Ravnborg @ 2019-12-08 9:31 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, linux-arm-msm, aarch64-laptops, Rob Clark,
Jeffrey Hugo, open list, Bjorn Andersson, Vasily Khoruzhick,
David Airlie, Laurent Pinchart, Sean Paul
Hi Rob.
Patch looks good, one small improvement proposal.
On Sat, Dec 07, 2019 at 12:35:51PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> 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 <robdclark@chromium.org>
> ---
> drivers/gpu/drm/drm_of.c | 41 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 0ca58803ba46..2baf44e401b8 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -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;
> +}
This function seems pretty generic. Should this be part of
drivers/of/property.c - as others may have the same need?
> +
> /**
> * 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
> *
> @@ -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;
> --
> 2.23.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] drm/of: add support to find any enabled endpoint
@ 2019-12-08 9:31 ` Sam Ravnborg
0 siblings, 0 replies; 32+ messages in thread
From: Sam Ravnborg @ 2019-12-08 9:31 UTC (permalink / raw)
To: Rob Clark
Cc: Rob Clark, aarch64-laptops, Jeffrey Hugo, linux-arm-msm,
open list, dri-devel, Bjorn Andersson, Vasily Khoruzhick,
David Airlie, Laurent Pinchart, Sean Paul
Hi Rob.
Patch looks good, one small improvement proposal.
On Sat, Dec 07, 2019 at 12:35:51PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> 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 <robdclark@chromium.org>
> ---
> drivers/gpu/drm/drm_of.c | 41 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 0ca58803ba46..2baf44e401b8 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -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;
> +}
This function seems pretty generic. Should this be part of
drivers/of/property.c - as others may have the same need?
> +
> /**
> * 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
> *
> @@ -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;
> --
> 2.23.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
2019-12-07 20:35 ` Rob Clark
@ 2019-12-08 9:39 ` Sam Ravnborg
-1 siblings, 0 replies; 32+ messages in thread
From: Sam Ravnborg @ 2019-12-08 9:39 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, linux-arm-msm, aarch64-laptops, Rob Clark,
Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, open list, Rob Herring, Bjorn Andersson,
Vasily Khoruzhick, David Airlie, Thierry Reding,
Laurent Pinchart
Hi Rob.
The panel-id can be used to help in several usecase.
With a few nits pointed out below fixed:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Sam
On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> 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 <robdclark@chromium.org>
> ---
> .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> index ef8d8cdfcede..6113319b91dd 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> @@ -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.
> + used by the HLOS itself.
Spell out HLOS - it is not obvious for all what it is.
> +
> + 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"
> + 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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
@ 2019-12-08 9:39 ` Sam Ravnborg
0 siblings, 0 replies; 32+ messages in thread
From: Sam Ravnborg @ 2019-12-08 9:39 UTC (permalink / raw)
To: Rob Clark
Cc: Rob Clark, Mark Rutland, aarch64-laptops,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, linux-arm-msm, open list, dri-devel,
Bjorn Andersson, Vasily Khoruzhick, David Airlie, Rob Herring,
Thierry Reding, Laurent Pinchart
Hi Rob.
The panel-id can be used to help in several usecase.
With a few nits pointed out below fixed:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Sam
On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> 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 <robdclark@chromium.org>
> ---
> .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> index ef8d8cdfcede..6113319b91dd 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> @@ -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.
> + used by the HLOS itself.
Spell out HLOS - it is not obvious for all what it is.
> +
> + 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"
> + 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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
2019-12-07 20:35 ` Rob Clark
@ 2019-12-08 14:45 ` Laurent Pinchart
-1 siblings, 0 replies; 32+ messages in thread
From: Laurent Pinchart @ 2019-12-08 14:45 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, linux-arm-msm, aarch64-laptops, Vasily Khoruzhick,
Bjorn Andersson, Jeffrey Hugo, Rob Clark, Thierry Reding,
Sam Ravnborg, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
Hi Rob,
Thank you for the patch.
On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> 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 <robdclark@chromium.org>
> ---
> .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> index ef8d8cdfcede..6113319b91dd 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> @@ -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.
> +
> + 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:
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
@ 2019-12-08 14:45 ` Laurent Pinchart
0 siblings, 0 replies; 32+ messages in thread
From: Laurent Pinchart @ 2019-12-08 14:45 UTC (permalink / raw)
To: Rob Clark
Cc: Rob Clark, aarch64-laptops, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, linux-arm-msm, open list, dri-devel,
Bjorn Andersson, Vasily Khoruzhick, David Airlie, Thierry Reding,
Mark Rutland, Sam Ravnborg
Hi Rob,
Thank you for the patch.
On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> 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 <robdclark@chromium.org>
> ---
> .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> index ef8d8cdfcede..6113319b91dd 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> @@ -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.
> +
> + 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:
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
2019-12-08 14:45 ` Laurent Pinchart
@ 2019-12-08 16:50 ` Rob Clark
-1 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-08 16:50 UTC (permalink / raw)
To: Laurent Pinchart
Cc: dri-devel, linux-arm-msm, aarch64-laptops, Vasily Khoruzhick,
Bjorn Andersson, Jeffrey Hugo, Rob Clark, Thierry Reding,
Sam Ravnborg, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> Thank you for the patch.
>
> On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > 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 <robdclark@chromium.org>
> > ---
> > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > index ef8d8cdfcede..6113319b91dd 100644
> > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > @@ -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.
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
> 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.
>
> > +
> > + 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:
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
@ 2019-12-08 16:50 ` Rob Clark
0 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-08 16:50 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Rob Clark, aarch64-laptops, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, linux-arm-msm, open list, dri-devel,
Bjorn Andersson, Vasily Khoruzhick, David Airlie, Thierry Reding,
Mark Rutland, Sam Ravnborg
On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> Thank you for the patch.
>
> On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > 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 <robdclark@chromium.org>
> > ---
> > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > index ef8d8cdfcede..6113319b91dd 100644
> > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > @@ -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.
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
> 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.
>
> > +
> > + 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:
>
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
2019-12-08 16:50 ` Rob Clark
@ 2019-12-08 18:27 ` Laurent Pinchart
-1 siblings, 0 replies; 32+ messages in thread
From: Laurent Pinchart @ 2019-12-08 18:27 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, linux-arm-msm, aarch64-laptops, Vasily Khoruzhick,
Bjorn Andersson, Jeffrey Hugo, Rob Clark, Thierry Reding,
Sam Ravnborg, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
Hi Rob,
On Sun, Dec 08, 2019 at 08:50:32AM -0800, Rob Clark wrote:
> On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart wrote:
> > On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > 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 <robdclark@chromium.org>
> > > ---
> > > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > > 1 file changed, 26 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > index ef8d8cdfcede..6113319b91dd 100644
> > > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > @@ -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.
>
> 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.
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.
> > 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.
> >
> > > +
> > > + 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:
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
@ 2019-12-08 18:27 ` Laurent Pinchart
0 siblings, 0 replies; 32+ messages in thread
From: Laurent Pinchart @ 2019-12-08 18:27 UTC (permalink / raw)
To: Rob Clark
Cc: Rob Clark, aarch64-laptops, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, linux-arm-msm, open list, dri-devel,
Bjorn Andersson, Vasily Khoruzhick, David Airlie, Thierry Reding,
Mark Rutland, Sam Ravnborg
Hi Rob,
On Sun, Dec 08, 2019 at 08:50:32AM -0800, Rob Clark wrote:
> On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart wrote:
> > On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > 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 <robdclark@chromium.org>
> > > ---
> > > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > > 1 file changed, 26 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > index ef8d8cdfcede..6113319b91dd 100644
> > > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > @@ -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.
>
> 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.
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.
> > 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.
> >
> > > +
> > > + 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:
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
2019-12-08 18:27 ` Laurent Pinchart
@ 2019-12-08 21:23 ` Rob Clark
-1 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-08 21:23 UTC (permalink / raw)
To: Laurent Pinchart
Cc: dri-devel, linux-arm-msm, aarch64-laptops, Vasily Khoruzhick,
Bjorn Andersson, Jeffrey Hugo, Rob Clark, Thierry Reding,
Sam Ravnborg, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
On Sun, Dec 8, 2019 at 10:28 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> On Sun, Dec 08, 2019 at 08:50:32AM -0800, Rob Clark wrote:
> > On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart wrote:
> > > On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > > > From: Rob Clark <robdclark@chromium.org>
> > > >
> > > > 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 <robdclark@chromium.org>
> > > > ---
> > > > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > > > 1 file changed, 26 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > index ef8d8cdfcede..6113319b91dd 100644
> > > > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > @@ -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.
> >
> > 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.
>
> 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:
---------
diff --git a/arch/arm64/boot/dts/qcom/Makefile
b/arch/arm64/boot/dts/qcom/Makefile
index 6498a1ec893f..1a61e8da2521 100644
--- a/arch/arm64/boot/dts/qcom/Makefile
+++ b/arch/arm64/boot/dts/qcom/Makefile
@@ -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
diff --git a/arch/arm64/boot/dts/qcom/panels/Makefile
b/arch/arm64/boot/dts/qcom/panels/Makefile
new file mode 100644
index 000000000000..dbf55f423555
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/panels/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb
+dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtb
diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
new file mode 100644
index 000000000000..ebcf65419dad
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
@@ -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";
+ };
+ };
+};
diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
new file mode 100644
index 000000000000..0ad5bb6003e3
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
@@ -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";
+ };
+ };
+};
diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
index c35d8099d8eb..92c76afb721c 100644
--- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
+++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
@@ -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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
@ 2019-12-08 21:23 ` Rob Clark
0 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-08 21:23 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Rob Clark, aarch64-laptops, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, linux-arm-msm, open list, dri-devel,
Bjorn Andersson, Vasily Khoruzhick, David Airlie, Thierry Reding,
Mark Rutland, Sam Ravnborg
On Sun, Dec 8, 2019 at 10:28 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> On Sun, Dec 08, 2019 at 08:50:32AM -0800, Rob Clark wrote:
> > On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart wrote:
> > > On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > > > From: Rob Clark <robdclark@chromium.org>
> > > >
> > > > 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 <robdclark@chromium.org>
> > > > ---
> > > > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > > > 1 file changed, 26 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > index ef8d8cdfcede..6113319b91dd 100644
> > > > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > @@ -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.
> >
> > 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.
>
> 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:
---------
diff --git a/arch/arm64/boot/dts/qcom/Makefile
b/arch/arm64/boot/dts/qcom/Makefile
index 6498a1ec893f..1a61e8da2521 100644
--- a/arch/arm64/boot/dts/qcom/Makefile
+++ b/arch/arm64/boot/dts/qcom/Makefile
@@ -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
diff --git a/arch/arm64/boot/dts/qcom/panels/Makefile
b/arch/arm64/boot/dts/qcom/panels/Makefile
new file mode 100644
index 000000000000..dbf55f423555
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/panels/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb
+dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtb
diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
new file mode 100644
index 000000000000..ebcf65419dad
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
@@ -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";
+ };
+ };
+};
diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
new file mode 100644
index 000000000000..0ad5bb6003e3
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
@@ -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";
+ };
+ };
+};
diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
index c35d8099d8eb..92c76afb721c 100644
--- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
+++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
@@ -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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
2019-12-08 21:23 ` Rob Clark
@ 2019-12-09 11:05 ` Laurent Pinchart
-1 siblings, 0 replies; 32+ messages in thread
From: Laurent Pinchart @ 2019-12-09 11:05 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, linux-arm-msm, aarch64-laptops, Vasily Khoruzhick,
Bjorn Andersson, Jeffrey Hugo, Rob Clark, Thierry Reding,
Sam Ravnborg, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
Hi Rob,
On Sun, Dec 08, 2019 at 01:23:59PM -0800, Rob Clark wrote:
> On Sun, Dec 8, 2019 at 10:28 AM Laurent Pinchart wrote:
> > On Sun, Dec 08, 2019 at 08:50:32AM -0800, Rob Clark wrote:
> > > On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart wrote:
> > > > On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > > > > From: Rob Clark <robdclark@chromium.org>
> > > > >
> > > > > 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 <robdclark@chromium.org>
> > > > > ---
> > > > > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > > > > 1 file changed, 26 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > index ef8d8cdfcede..6113319b91dd 100644
> > > > > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > @@ -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.
> > >
> > > 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.
> >
> > 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 :-)
> ---------
> diff --git a/arch/arm64/boot/dts/qcom/Makefile
> b/arch/arm64/boot/dts/qcom/Makefile
> index 6498a1ec893f..1a61e8da2521 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -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
> diff --git a/arch/arm64/boot/dts/qcom/panels/Makefile
> b/arch/arm64/boot/dts/qcom/panels/Makefile
> new file mode 100644
> index 000000000000..dbf55f423555
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb
> +dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtb
> diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> new file mode 100644
> index 000000000000..ebcf65419dad
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> @@ -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";
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> new file mode 100644
> index 000000000000..0ad5bb6003e3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> @@ -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";
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> index c35d8099d8eb..92c76afb721c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> @@ -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.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
@ 2019-12-09 11:05 ` Laurent Pinchart
0 siblings, 0 replies; 32+ messages in thread
From: Laurent Pinchart @ 2019-12-09 11:05 UTC (permalink / raw)
To: Rob Clark
Cc: Rob Clark, aarch64-laptops, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, linux-arm-msm, open list, dri-devel,
Bjorn Andersson, Vasily Khoruzhick, David Airlie, Thierry Reding,
Mark Rutland, Sam Ravnborg
Hi Rob,
On Sun, Dec 08, 2019 at 01:23:59PM -0800, Rob Clark wrote:
> On Sun, Dec 8, 2019 at 10:28 AM Laurent Pinchart wrote:
> > On Sun, Dec 08, 2019 at 08:50:32AM -0800, Rob Clark wrote:
> > > On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart wrote:
> > > > On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > > > > From: Rob Clark <robdclark@chromium.org>
> > > > >
> > > > > 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 <robdclark@chromium.org>
> > > > > ---
> > > > > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > > > > 1 file changed, 26 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > index ef8d8cdfcede..6113319b91dd 100644
> > > > > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > @@ -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.
> > >
> > > 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.
> >
> > 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 :-)
> ---------
> diff --git a/arch/arm64/boot/dts/qcom/Makefile
> b/arch/arm64/boot/dts/qcom/Makefile
> index 6498a1ec893f..1a61e8da2521 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -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
> diff --git a/arch/arm64/boot/dts/qcom/panels/Makefile
> b/arch/arm64/boot/dts/qcom/panels/Makefile
> new file mode 100644
> index 000000000000..dbf55f423555
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb
> +dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtb
> diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> new file mode 100644
> index 000000000000..ebcf65419dad
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> @@ -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";
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> new file mode 100644
> index 000000000000..0ad5bb6003e3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> @@ -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";
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> index c35d8099d8eb..92c76afb721c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> @@ -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.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
2019-12-08 21:23 ` Rob Clark
@ 2019-12-09 15:31 ` Rob Herring
-1 siblings, 0 replies; 32+ messages in thread
From: Rob Herring @ 2019-12-09 15:31 UTC (permalink / raw)
To: Rob Clark
Cc: Laurent Pinchart, dri-devel, linux-arm-msm, aarch64-laptops,
Vasily Khoruzhick, Bjorn Andersson, Jeffrey Hugo, Rob Clark,
Thierry Reding, Sam Ravnborg, David Airlie, Daniel Vetter,
Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
On Sun, Dec 8, 2019 at 3:24 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Sun, Dec 8, 2019 at 10:28 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Rob,
> >
> > On Sun, Dec 08, 2019 at 08:50:32AM -0800, Rob Clark wrote:
> > > On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart wrote:
> > > > On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > > > > From: Rob Clark <robdclark@chromium.org>
> > > > >
> > > > > 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 <robdclark@chromium.org>
> > > > > ---
> > > > > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > > > > 1 file changed, 26 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > index ef8d8cdfcede..6113319b91dd 100644
> > > > > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > @@ -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.
> > >
> > > 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.
> >
> > 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:
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?
> ---------
> diff --git a/arch/arm64/boot/dts/qcom/Makefile
> b/arch/arm64/boot/dts/qcom/Makefile
> index 6498a1ec893f..1a61e8da2521 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -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
> diff --git a/arch/arm64/boot/dts/qcom/panels/Makefile
> b/arch/arm64/boot/dts/qcom/panels/Makefile
> new file mode 100644
> index 000000000000..dbf55f423555
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb
> +dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtb
> diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> new file mode 100644
> index 000000000000..ebcf65419dad
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> @@ -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";
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> new file mode 100644
> index 000000000000..0ad5bb6003e3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> @@ -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";
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> index c35d8099d8eb..92c76afb721c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> @@ -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.
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
@ 2019-12-09 15:31 ` Rob Herring
0 siblings, 0 replies; 32+ messages in thread
From: Rob Herring @ 2019-12-09 15:31 UTC (permalink / raw)
To: Rob Clark
Cc: Rob Clark, aarch64-laptops,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, linux-arm-msm, open list, dri-devel,
Bjorn Andersson, Vasily Khoruzhick, David Airlie, Thierry Reding,
Laurent Pinchart, Mark Rutland, Sam Ravnborg
On Sun, Dec 8, 2019 at 3:24 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Sun, Dec 8, 2019 at 10:28 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Rob,
> >
> > On Sun, Dec 08, 2019 at 08:50:32AM -0800, Rob Clark wrote:
> > > On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart wrote:
> > > > On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > > > > From: Rob Clark <robdclark@chromium.org>
> > > > >
> > > > > 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 <robdclark@chromium.org>
> > > > > ---
> > > > > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > > > > 1 file changed, 26 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > index ef8d8cdfcede..6113319b91dd 100644
> > > > > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > @@ -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.
> > >
> > > 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.
> >
> > 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:
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?
> ---------
> diff --git a/arch/arm64/boot/dts/qcom/Makefile
> b/arch/arm64/boot/dts/qcom/Makefile
> index 6498a1ec893f..1a61e8da2521 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -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
> diff --git a/arch/arm64/boot/dts/qcom/panels/Makefile
> b/arch/arm64/boot/dts/qcom/panels/Makefile
> new file mode 100644
> index 000000000000..dbf55f423555
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb
> +dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtb
> diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> new file mode 100644
> index 000000000000..ebcf65419dad
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> @@ -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";
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> new file mode 100644
> index 000000000000..0ad5bb6003e3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> @@ -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";
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> index c35d8099d8eb..92c76afb721c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> @@ -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.
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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
2019-12-09 15:31 ` Rob Herring
@ 2019-12-09 15:57 ` Rob Clark
-1 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-09 15:57 UTC (permalink / raw)
To: Rob Herring
Cc: Laurent Pinchart, dri-devel, linux-arm-msm, aarch64-laptops,
Vasily Khoruzhick, Bjorn Andersson, Jeffrey Hugo, Rob Clark,
Thierry Reding, Sam Ravnborg, David Airlie, Daniel Vetter,
Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
On Mon, Dec 9, 2019 at 7:31 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Sun, Dec 8, 2019 at 3:24 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Sun, Dec 8, 2019 at 10:28 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Rob,
> > >
> > > On Sun, Dec 08, 2019 at 08:50:32AM -0800, Rob Clark wrote:
> > > > On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart wrote:
> > > > > On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > >
> > > > > > 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 <robdclark@chromium.org>
> > > > > > ---
> > > > > > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > > > > > 1 file changed, 26 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > > index ef8d8cdfcede..6113319b91dd 100644
> > > > > > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > > @@ -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.
> > > >
> > > > 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.
> > >
> > > 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:
>
> 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.
> > ---------
> > diff --git a/arch/arm64/boot/dts/qcom/Makefile
> > b/arch/arm64/boot/dts/qcom/Makefile
> > index 6498a1ec893f..1a61e8da2521 100644
> > --- a/arch/arm64/boot/dts/qcom/Makefile
> > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > @@ -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
> > diff --git a/arch/arm64/boot/dts/qcom/panels/Makefile
> > b/arch/arm64/boot/dts/qcom/panels/Makefile
> > new file mode 100644
> > index 000000000000..dbf55f423555
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/panels/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb
> > +dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtb
> > diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> > b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> > new file mode 100644
> > index 000000000000..ebcf65419dad
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> > @@ -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";
> > + };
> > + };
> > +};
> > diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> > b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> > new file mode 100644
> > index 000000000000..0ad5bb6003e3
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> > @@ -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";
> > + };
> > + };
> > +};
> > diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> > b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> > index c35d8099d8eb..92c76afb721c 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> > +++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> > @@ -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.
>
> 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.
>
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id
@ 2019-12-09 15:57 ` Rob Clark
0 siblings, 0 replies; 32+ messages in thread
From: Rob Clark @ 2019-12-09 15:57 UTC (permalink / raw)
To: Rob Herring
Cc: Rob Clark, aarch64-laptops,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Jeffrey Hugo, linux-arm-msm, open list, dri-devel,
Bjorn Andersson, Vasily Khoruzhick, David Airlie, Thierry Reding,
Laurent Pinchart, Mark Rutland, Sam Ravnborg
On Mon, Dec 9, 2019 at 7:31 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Sun, Dec 8, 2019 at 3:24 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Sun, Dec 8, 2019 at 10:28 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Rob,
> > >
> > > On Sun, Dec 08, 2019 at 08:50:32AM -0800, Rob Clark wrote:
> > > > On Sun, Dec 8, 2019 at 6:45 AM Laurent Pinchart wrote:
> > > > > On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > >
> > > > > > 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 <robdclark@chromium.org>
> > > > > > ---
> > > > > > .../bindings/display/panel/panel-common.yaml | 26 +++++++++++++++++++
> > > > > > 1 file changed, 26 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > > index ef8d8cdfcede..6113319b91dd 100644
> > > > > > --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > > > > > @@ -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.
> > > >
> > > > 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.
> > >
> > > 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:
>
> 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.
> > ---------
> > diff --git a/arch/arm64/boot/dts/qcom/Makefile
> > b/arch/arm64/boot/dts/qcom/Makefile
> > index 6498a1ec893f..1a61e8da2521 100644
> > --- a/arch/arm64/boot/dts/qcom/Makefile
> > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > @@ -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
> > diff --git a/arch/arm64/boot/dts/qcom/panels/Makefile
> > b/arch/arm64/boot/dts/qcom/panels/Makefile
> > new file mode 100644
> > index 000000000000..dbf55f423555
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/panels/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +dtb-$(CONFIG_ARCH_QCOM) += panel-c4.dtb
> > +dtb-$(CONFIG_ARCH_QCOM) += panel-c5.dtb
> > diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> > b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> > new file mode 100644
> > index 000000000000..ebcf65419dad
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/panels/panel-c4.dts
> > @@ -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";
> > + };
> > + };
> > +};
> > diff --git a/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> > b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> > new file mode 100644
> > index 000000000000..0ad5bb6003e3
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/panels/panel-c5.dts
> > @@ -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";
> > + };
> > + };
> > +};
> > diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> > b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> > index c35d8099d8eb..92c76afb721c 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> > +++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> > @@ -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.
>
> 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.
>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2019-12-09 15:57 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-07 20:35 [PATCH 0/4] drm+dt: multi panel selection and yoga c630 display Rob Clark
2019-12-07 20:35 ` Rob Clark
2019-12-07 20:35 ` [PATCH 1/4] dt-bindings: display: panel: document panel-id Rob Clark
2019-12-07 20:35 ` Rob Clark
2019-12-07 21:13 ` Sam Ravnborg
2019-12-07 21:13 ` Sam Ravnborg
2019-12-07 21:34 ` Rob Clark
2019-12-07 21:34 ` Rob Clark
2019-12-08 9:39 ` Sam Ravnborg
2019-12-08 9:39 ` Sam Ravnborg
2019-12-08 14:45 ` Laurent Pinchart
2019-12-08 14:45 ` Laurent Pinchart
2019-12-08 16:50 ` Rob Clark
2019-12-08 16:50 ` Rob Clark
2019-12-08 18:27 ` Laurent Pinchart
2019-12-08 18:27 ` Laurent Pinchart
2019-12-08 21:23 ` Rob Clark
2019-12-08 21:23 ` Rob Clark
2019-12-09 11:05 ` Laurent Pinchart
2019-12-09 11:05 ` Laurent Pinchart
2019-12-09 15:31 ` Rob Herring
2019-12-09 15:31 ` Rob Herring
2019-12-09 15:57 ` Rob Clark
2019-12-09 15:57 ` Rob Clark
2019-12-07 20:35 ` [PATCH 2/4] drm/of: add support to find any enabled endpoint Rob Clark
2019-12-07 20:35 ` Rob Clark
2019-12-08 9:31 ` Sam Ravnborg
2019-12-08 9:31 ` Sam Ravnborg
2019-12-07 20:35 ` [PATCH 3/4] drm/bridge: ti-sn65dsi86: " Rob Clark
2019-12-07 20:35 ` Rob Clark
2019-12-07 20:35 ` [PATCH 4/4] arm64: dts: qcom: c630: Enable display Rob Clark
2019-12-07 20:35 ` Rob Clark
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.