* [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml @ 2020-04-15 15:48 Douglas Anderson 2020-04-15 15:48 ` [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings Douglas Anderson ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Douglas Anderson @ 2020-04-15 15:48 UTC (permalink / raw) To: airlied, daniel, robh+dt, narmstrong, a.hajda, Laurent.pinchart, spanda Cc: jonas, bjorn.andersson, devicetree, jeffrey.l.hugo, swboyd, jernej.skrabec, linux-arm-msm, robdclark, dri-devel, Douglas Anderson, Krzysztof Kozlowski, Paul Walmsley, Stephen Boyd, linux-kernel This moves the bindings over, based a lot on toshiba,tc358768.yaml. Unless there's someone known to be better, I've set the maintainer in the yaml as the first person to submit bindings. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- .../bindings/display/bridge/ti,sn65dsi86.txt | 87 -------- .../bindings/display/bridge/ti,sn65dsi86.yaml | 188 ++++++++++++++++++ 2 files changed, 188 insertions(+), 87 deletions(-) delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt deleted file mode 100644 index 8ec4a7f2623a..000000000000 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt +++ /dev/null @@ -1,87 +0,0 @@ -SN65DSI86 DSI to eDP bridge chip --------------------------------- - -This is the binding for Texas Instruments SN65DSI86 bridge. -http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf - -Required properties: -- compatible: Must be "ti,sn65dsi86" -- reg: i2c address of the chip, 0x2d as per datasheet -- enable-gpios: gpio specification for bridge_en pin (active high) - -- vccio-supply: A 1.8V supply that powers up the digital IOs. -- vpll-supply: A 1.8V supply that powers up the displayport PLL. -- vcca-supply: A 1.2V supply that powers up the analog circuits. -- vcc-supply: A 1.2V supply that powers up the digital core. - -Optional properties: -- interrupts-extended: Specifier for the SN65DSI86 interrupt line. - -- gpio-controller: Marks the device has a GPIO controller. -- #gpio-cells : Should be two. The first cell is the pin number and - the second cell is used to specify flags. - See ../../gpio/gpio.txt for more information. -- #pwm-cells : Should be one. See ../../pwm/pwm.yaml for description of - the cell formats. - -- clock-names: should be "refclk" -- clocks: Specification for input reference clock. The reference - clock rate must be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz. - -- data-lanes: See ../../media/video-interface.txt -- lane-polarities: See ../../media/video-interface.txt - -- suspend-gpios: specification for GPIO1 pin on bridge (active low) - -Required nodes: -This device has two video ports. Their connections are modelled using the -OF graph bindings specified in Documentation/devicetree/bindings/graph.txt. - -- Video port 0 for DSI input -- Video port 1 for eDP output - -Example -------- - -edp-bridge@2d { - compatible = "ti,sn65dsi86"; - #address-cells = <1>; - #size-cells = <0>; - reg = <0x2d>; - - enable-gpios = <&msmgpio 33 GPIO_ACTIVE_HIGH>; - suspend-gpios = <&msmgpio 34 GPIO_ACTIVE_LOW>; - - interrupts-extended = <&gpio3 4 IRQ_TYPE_EDGE_FALLING>; - - vccio-supply = <&pm8916_l17>; - vcca-supply = <&pm8916_l6>; - vpll-supply = <&pm8916_l17>; - vcc-supply = <&pm8916_l6>; - - clock-names = "refclk"; - clocks = <&input_refclk>; - - ports { - #address-cells = <1>; - #size-cells = <0>; - - port@0 { - reg = <0>; - - edp_bridge_in: endpoint { - remote-endpoint = <&dsi_out>; - }; - }; - - port@1 { - reg = <1>; - - edp_bridge_out: endpoint { - data-lanes = <2 1 3 0>; - lane-polarities = <0 1 0 1>; - remote-endpoint = <&edp_panel_in>; - }; - }; - }; -} diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml new file mode 100644 index 000000000000..8cacc6db33a9 --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml @@ -0,0 +1,188 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/bridge/ti,sn65dsi86.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: SN65DSI86 DSI to eDP bridge chip + +maintainers: + - Sandeep Panda <spanda@codeaurora.org> + +description: | + The Texas Instruments SN65DSI86 bridge takes MIPI DSI in and outputs eDP. + http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf + +properties: + compatible: + const: ti,sn65dsi86 + + reg: + const: 0x2d + + enable-gpios: + maxItems: 1 + description: GPIO specification for bridge_en pin (active high). + + vccio-supply: + description: A 1.8V supply that powers up the digital IOs. + + vpll-supply: + description: A 1.8V supply that powers up the DisplayPort PLL. + + vcca-supply: + description: A 1.2V supply that powers up the analog circuits. + + vcc-supply: + description: A 1.2V supply that powers up the digital core. + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + description: + Specification for input reference clock. The reference clock rate must + be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz. + + clock-names: + const: refclk + + gpio-controller: true + '#gpio-cells': + const: 2 + description: + First cell is pin number, second cell is flags. GPIO pin numbers are + 1-based to match the datasheet. See ../../gpio/gpio.txt for more + information. + + '#pwm-cells': + const: 1 + description: See ../../pwm/pwm.yaml for description of the cell formats. + + data-lanes: + description: See ../../media/video-interface.txt + + lane-polarities: + description: See ../../media/video-interface.txt + + ports: + type: object + + properties: + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + port@0: + type: object + additionalProperties: false + + description: + Video port for MIPI DSI input + + properties: + reg: + const: 0 + + patternProperties: + endpoint: + type: object + additionalProperties: false + + properties: + remote-endpoint: true + + required: + - reg + + port@1: + type: object + additionalProperties: false + + description: + Video port for eDP output (panel or connector). + + properties: + reg: + const: 1 + + patternProperties: + endpoint: + type: object + additionalProperties: false + + properties: + remote-endpoint: true + + required: + - reg + + required: + - "#address-cells" + - "#size-cells" + - port@0 + - port@1 + +required: + - compatible + - reg + - enable-gpios + - vccio-supply + - vpll-supply + - vcca-supply + - vcc-supply + - ports + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/qcom,rpmh.h> + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + + i2c1 { + #address-cells = <1>; + #size-cells = <0>; + + bridge@2d { + compatible = "ti,sn65dsi86"; + reg = <0x2d>; + + interrupt-parent = <&tlmm>; + interrupts = <10 IRQ_TYPE_LEVEL_HIGH>; + + enable-gpios = <&tlmm 102 GPIO_ACTIVE_HIGH>; + + vpll-supply = <&src_pp1800_s4a>; + vccio-supply = <&src_pp1800_s4a>; + vcca-supply = <&src_pp1200_l2a>; + vcc-supply = <&src_pp1200_l2a>; + + clocks = <&rpmhcc RPMH_LN_BB_CLK2>; + clock-names = "refclk"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + endpoint { + remote-endpoint = <&dsi0_out>; + }; + }; + + port@1 { + reg = <1>; + endpoint { + remote-endpoint = <&panel_in_edp>; + }; + }; + }; + }; + }; + -- 2.26.0.110.g2183baf09c-goog ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings 2020-04-15 15:48 [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml Douglas Anderson @ 2020-04-15 15:48 ` Douglas Anderson 2020-04-15 19:53 ` Stephen Boyd 2020-04-15 15:48 ` [PATCH 3/3] drm/bridge: ti-sn65dsi86: Allow one of the bridge GPIOs to be HPD Douglas Anderson ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Douglas Anderson @ 2020-04-15 15:48 UTC (permalink / raw) To: airlied, daniel, robh+dt, narmstrong, a.hajda, Laurent.pinchart, spanda Cc: jonas, bjorn.andersson, devicetree, jeffrey.l.hugo, swboyd, jernej.skrabec, linux-arm-msm, robdclark, dri-devel, Douglas Anderson, linux-kernel Allow people to specify to use a GPIO for hot-plug-detect. Add an example. NOTE: The current patch adding support for hpd-gpios to the Linux driver for hpd-gpios only adds enough support to the driver so that the bridge can use one of its own GPIOs. The bindings, however, are written generically. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- .../bindings/display/bridge/ti,sn65dsi86.yaml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml index 8cacc6db33a9..554bfd003000 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml @@ -60,6 +60,10 @@ properties: const: 1 description: See ../../pwm/pwm.yaml for description of the cell formats. + hpd-gpios: + maxItems: 1 + description: If present use the given GPIO for hot-plug-detect. + data-lanes: description: See ../../media/video-interface.txt @@ -148,7 +152,7 @@ examples: #address-cells = <1>; #size-cells = <0>; - bridge@2d { + sn65dsi86_bridge: bridge@2d { compatible = "ti,sn65dsi86"; reg = <0x2d>; @@ -165,6 +169,10 @@ examples: clocks = <&rpmhcc RPMH_LN_BB_CLK2>; clock-names = "refclk"; + gpio-controller; + #gpio-cells = <2>; + hpd-gpios = <&sn65dsi86_bridge 2 GPIO_ACTIVE_HIGH>; + ports { #address-cells = <1>; #size-cells = <0>; -- 2.26.0.110.g2183baf09c-goog ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings 2020-04-15 15:48 ` [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings Douglas Anderson @ 2020-04-15 19:53 ` Stephen Boyd 2020-04-15 20:32 ` Laurent Pinchart 0 siblings, 1 reply; 13+ messages in thread From: Stephen Boyd @ 2020-04-15 19:53 UTC (permalink / raw) To: Douglas Anderson, Laurent.pinchart, a.hajda, airlied, daniel, narmstrong, robh+dt, spanda Cc: jonas, bjorn.andersson, devicetree, jeffrey.l.hugo, jernej.skrabec, linux-arm-msm, robdclark, dri-devel, Douglas Anderson, linux-kernel Quoting Douglas Anderson (2020-04-15 08:48:40) > Allow people to specify to use a GPIO for hot-plug-detect. Add an > example. > > NOTE: The current patch adding support for hpd-gpios to the Linux > driver for hpd-gpios only adds enough support to the driver so that > the bridge can use one of its own GPIOs. The bindings, however, are > written generically. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > .../bindings/display/bridge/ti,sn65dsi86.yaml | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > index 8cacc6db33a9..554bfd003000 100644 > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > @@ -60,6 +60,10 @@ properties: > const: 1 > description: See ../../pwm/pwm.yaml for description of the cell formats. > > + hpd-gpios: > + maxItems: 1 > + description: If present use the given GPIO for hot-plug-detect. Shouldn't this go in the panel node? And the panel driver should get the gpio and poll it after powering up the panel? Presumably that's why we have the no-hpd property in the simple panel binding vs. putting it here in the bridge. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings 2020-04-15 19:53 ` Stephen Boyd @ 2020-04-15 20:32 ` Laurent Pinchart 2020-04-15 23:49 ` Doug Anderson 0 siblings, 1 reply; 13+ messages in thread From: Laurent Pinchart @ 2020-04-15 20:32 UTC (permalink / raw) To: Stephen Boyd Cc: Douglas Anderson, a.hajda, airlied, daniel, narmstrong, robh+dt, spanda, jonas, bjorn.andersson, devicetree, jeffrey.l.hugo, jernej.skrabec, linux-arm-msm, robdclark, dri-devel, linux-kernel On Wed, Apr 15, 2020 at 12:53:02PM -0700, Stephen Boyd wrote: > Quoting Douglas Anderson (2020-04-15 08:48:40) > > Allow people to specify to use a GPIO for hot-plug-detect. Add an > > example. > > > > NOTE: The current patch adding support for hpd-gpios to the Linux > > driver for hpd-gpios only adds enough support to the driver so that > > the bridge can use one of its own GPIOs. The bindings, however, are > > written generically. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > .../bindings/display/bridge/ti,sn65dsi86.yaml | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > index 8cacc6db33a9..554bfd003000 100644 > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > @@ -60,6 +60,10 @@ properties: > > const: 1 > > description: See ../../pwm/pwm.yaml for description of the cell formats. > > > > + hpd-gpios: > > + maxItems: 1 > > + description: If present use the given GPIO for hot-plug-detect. > > Shouldn't this go in the panel node? And the panel driver should get the > gpio and poll it after powering up the panel? Presumably that's why we > have the no-hpd property in the simple panel binding vs. putting it here > in the bridge. Same question really, I think this belongs to the panel (or connector) node indeed. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings 2020-04-15 20:32 ` Laurent Pinchart @ 2020-04-15 23:49 ` Doug Anderson 2020-04-16 0:54 ` Laurent Pinchart 0 siblings, 1 reply; 13+ messages in thread From: Doug Anderson @ 2020-04-15 23:49 UTC (permalink / raw) To: Laurent Pinchart Cc: Stephen Boyd, Andrzej Hajda, David Airlie, Daniel Vetter, Neil Armstrong, Rob Herring, Sandeep Panda, Jonas Karlman, Bjorn Andersson, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jeffrey Hugo, Jernej Skrabec, linux-arm-msm, Rob Clark, dri-devel, LKML Hi, On Wed, Apr 15, 2020 at 1:33 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Wed, Apr 15, 2020 at 12:53:02PM -0700, Stephen Boyd wrote: > > Quoting Douglas Anderson (2020-04-15 08:48:40) > > > Allow people to specify to use a GPIO for hot-plug-detect. Add an > > > example. > > > > > > NOTE: The current patch adding support for hpd-gpios to the Linux > > > driver for hpd-gpios only adds enough support to the driver so that > > > the bridge can use one of its own GPIOs. The bindings, however, are > > > written generically. > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > --- > > > > > > .../bindings/display/bridge/ti,sn65dsi86.yaml | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > index 8cacc6db33a9..554bfd003000 100644 > > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > @@ -60,6 +60,10 @@ properties: > > > const: 1 > > > description: See ../../pwm/pwm.yaml for description of the cell formats. > > > > > > + hpd-gpios: > > > + maxItems: 1 > > > + description: If present use the given GPIO for hot-plug-detect. > > > > Shouldn't this go in the panel node? And the panel driver should get the > > gpio and poll it after powering up the panel? Presumably that's why we > > have the no-hpd property in the simple panel binding vs. putting it here > > in the bridge. > > Same question really, I think this belongs to the panel (or connector) > node indeed. Hrm. To me "no-hpd" feels OK in the panel because the lack of a connection is somewhat symmetric. Thus it's OK to say either "HPD isn't hooked up to the panel in this system" or "HPD isn't hooked up to the bridge in this system" and both express the same thing (AKA that there is no HPD connection between the bridge and the panel). In the case of "no-hpd" it's more convenient to express it on the panel side because the panel driver is the one whose behavior has to change if HPD isn't hooked up. The panel datasheet is the one that says how long of a delay we need if HPD isn't hooked up. ...but when you're talking about where the bridge driver should look to find the HPD signal that it needs, that really feels like it should be described as part of the bridge. Specifically imagine we were using our bridge for DP, not for eDP. In that case simple-panel wouldn't be involved because we could get any type of display plugged in. Thus it couldn't go in the panel node. Here it feels clearer that hpd-gpio needs to be a property of the bridge driver. Looking at other usages of "hpd-gpio" in the kernel, it seems like the usage I'm proposing is also common. Grepping for "hpd-gpios" shows numerous examples of "hpd-gpios" being defined at the display controller level and (effectively) I believe the bridge is at the equivalent level. -Doug ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings 2020-04-15 23:49 ` Doug Anderson @ 2020-04-16 0:54 ` Laurent Pinchart 2020-04-16 21:53 ` Doug Anderson 0 siblings, 1 reply; 13+ messages in thread From: Laurent Pinchart @ 2020-04-16 0:54 UTC (permalink / raw) To: Doug Anderson Cc: Stephen Boyd, Andrzej Hajda, David Airlie, Daniel Vetter, Neil Armstrong, Rob Herring, Sandeep Panda, Jonas Karlman, Bjorn Andersson, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jeffrey Hugo, Jernej Skrabec, linux-arm-msm, Rob Clark, dri-devel, LKML Hi Doug, On Wed, Apr 15, 2020 at 04:49:00PM -0700, Doug Anderson wrote: > On Wed, Apr 15, 2020 at 1:33 PM Laurent Pinchart wrote: > > On Wed, Apr 15, 2020 at 12:53:02PM -0700, Stephen Boyd wrote: > > > Quoting Douglas Anderson (2020-04-15 08:48:40) > > > > Allow people to specify to use a GPIO for hot-plug-detect. Add an > > > > example. > > > > > > > > NOTE: The current patch adding support for hpd-gpios to the Linux > > > > driver for hpd-gpios only adds enough support to the driver so that > > > > the bridge can use one of its own GPIOs. The bindings, however, are > > > > written generically. > > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > --- > > > > > > > > .../bindings/display/bridge/ti,sn65dsi86.yaml | 10 +++++++++- > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > > index 8cacc6db33a9..554bfd003000 100644 > > > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > > @@ -60,6 +60,10 @@ properties: > > > > const: 1 > > > > description: See ../../pwm/pwm.yaml for description of the cell formats. > > > > > > > > + hpd-gpios: > > > > + maxItems: 1 > > > > + description: If present use the given GPIO for hot-plug-detect. > > > > > > Shouldn't this go in the panel node? And the panel driver should get the > > > gpio and poll it after powering up the panel? Presumably that's why we > > > have the no-hpd property in the simple panel binding vs. putting it here > > > in the bridge. > > > > Same question really, I think this belongs to the panel (or connector) > > node indeed. > > Hrm. > > To me "no-hpd" feels OK in the panel because the lack of a connection > is somewhat symmetric. Thus it's OK to say either "HPD isn't hooked > up to the panel in this system" or "HPD isn't hooked up to the bridge > in this system" and both express the same thing (AKA that there is no > HPD connection between the bridge and the panel). In the case of > "no-hpd" it's more convenient to express it on the panel side because > the panel driver is the one whose behavior has to change if HPD isn't > hooked up. The panel datasheet is the one that says how long of a > delay we need if HPD isn't hooked up. > > ...but when you're talking about where the bridge driver should look > to find the HPD signal that it needs, that really feels like it should > be described as part of the bridge. Specifically imagine we were > using our bridge for DP, not for eDP. In that case simple-panel > wouldn't be involved because we could get any type of display plugged > in. Thus it couldn't go in the panel node. Here it feels clearer > that hpd-gpio needs to be a property of the bridge driver. If you were using it for DP, you would need a DT node for the DP connector (with bindings to be added to Documentation/devicetree/bindings/display/connector/, similar to the ones we already have for other connectors). That DT node should reference the HPD pin GPIO. The bridge driver for the connector (drivers/gpu/drm/bridge/display-connector.c) would then handle HPD. The good news is that it already does :-) > Looking at other usages of "hpd-gpio" in the kernel, it seems like the > usage I'm proposing is also common. Grepping for "hpd-gpios" shows > numerous examples of "hpd-gpios" being defined at the display > controller level and (effectively) I believe the bridge is at the > equivalent level. Bridge drivers should only implement support for features available from the corresponding hardware. If an HPD signal is connected to a dedicated pin of the bridge, and the bridge can generate an interrupt and expose the HPD status through I2C, then it should implement HPD-related operations. If the HPD pin from the connector is hooked up to a GPIO of the SoC, it should be handled by the connector bridge driver. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings 2020-04-16 0:54 ` Laurent Pinchart @ 2020-04-16 21:53 ` Doug Anderson 2020-04-17 18:08 ` Laurent Pinchart 0 siblings, 1 reply; 13+ messages in thread From: Doug Anderson @ 2020-04-16 21:53 UTC (permalink / raw) To: Laurent Pinchart Cc: Stephen Boyd, Andrzej Hajda, David Airlie, Daniel Vetter, Neil Armstrong, Rob Herring, Sandeep Panda, Jonas Karlman, Bjorn Andersson, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jeffrey Hugo, Jernej Skrabec, linux-arm-msm, Rob Clark, dri-devel, LKML Hi, On Wed, Apr 15, 2020 at 5:54 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Doug, > > On Wed, Apr 15, 2020 at 04:49:00PM -0700, Doug Anderson wrote: > > On Wed, Apr 15, 2020 at 1:33 PM Laurent Pinchart wrote: > > > On Wed, Apr 15, 2020 at 12:53:02PM -0700, Stephen Boyd wrote: > > > > Quoting Douglas Anderson (2020-04-15 08:48:40) > > > > > Allow people to specify to use a GPIO for hot-plug-detect. Add an > > > > > example. > > > > > > > > > > NOTE: The current patch adding support for hpd-gpios to the Linux > > > > > driver for hpd-gpios only adds enough support to the driver so that > > > > > the bridge can use one of its own GPIOs. The bindings, however, are > > > > > written generically. > > > > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > --- > > > > > > > > > > .../bindings/display/bridge/ti,sn65dsi86.yaml | 10 +++++++++- > > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > > > index 8cacc6db33a9..554bfd003000 100644 > > > > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > > > @@ -60,6 +60,10 @@ properties: > > > > > const: 1 > > > > > description: See ../../pwm/pwm.yaml for description of the cell formats. > > > > > > > > > > + hpd-gpios: > > > > > + maxItems: 1 > > > > > + description: If present use the given GPIO for hot-plug-detect. > > > > > > > > Shouldn't this go in the panel node? And the panel driver should get the > > > > gpio and poll it after powering up the panel? Presumably that's why we > > > > have the no-hpd property in the simple panel binding vs. putting it here > > > > in the bridge. > > > > > > Same question really, I think this belongs to the panel (or connector) > > > node indeed. > > > > Hrm. > > > > To me "no-hpd" feels OK in the panel because the lack of a connection > > is somewhat symmetric. Thus it's OK to say either "HPD isn't hooked > > up to the panel in this system" or "HPD isn't hooked up to the bridge > > in this system" and both express the same thing (AKA that there is no > > HPD connection between the bridge and the panel). In the case of > > "no-hpd" it's more convenient to express it on the panel side because > > the panel driver is the one whose behavior has to change if HPD isn't > > hooked up. The panel datasheet is the one that says how long of a > > delay we need if HPD isn't hooked up. > > > > ...but when you're talking about where the bridge driver should look > > to find the HPD signal that it needs, that really feels like it should > > be described as part of the bridge. Specifically imagine we were > > using our bridge for DP, not for eDP. In that case simple-panel > > wouldn't be involved because we could get any type of display plugged > > in. Thus it couldn't go in the panel node. Here it feels clearer > > that hpd-gpio needs to be a property of the bridge driver. > > If you were using it for DP, you would need a DT node for the DP > connector (with bindings to be added to > Documentation/devicetree/bindings/display/connector/, similar to the > ones we already have for other connectors). That DT node should > reference the HPD pin GPIO. The bridge driver for the connector > (drivers/gpu/drm/bridge/display-connector.c) would then handle HPD. The > good news is that it already does :-) I'm having a really hard time following, but maybe it's because my knowledge of the DRM terminology is feeble at best? Looking at it from a DRM driver perspective and thus looking in 'drm/bridge/ti-sn65dsi86.c' I see that the driver for this bridge chip effectively is both the bridge and the connector. The struct encapsulating the driver data has both: struct drm_bridge bridge; struct drm_connector connector; ...in ti_sn_bridge_attach() the code calls drm_connector_init() for the connector. Looking at it from a device tree point of view, there is no separate node representing an eDP connector for one mainline user of 'ti,sn65dsi86' (sdm845-cheza). The device tree node has one input port (from "dsi0_out") and one output port (to "panel_in_edp"). There is no separate connector node as I can see with "hdmi-connector". ...and, as far as I can tell, sdm845-cheza is using the bindings as documented. The bindings say that the 'ti,sn65dsi86' node needs two ports: - Video port 0 for DSI input - Video port 1 for eDP output So, though I'm probably terribly confused, I would tentatively say that: - I'd guess that the 'ti,sn65dsi86' bindings were written / approved back before people were encouraged to model the connector as a separate node. - In the case of 'ti,sn65dsi86' the current dts node is both the node for the bridge and the connector. - If we want to try to deprecate the way that 'ti,sn65dsi86' works it feels like a big-ish effort. This would include adding a new "eDP" connector class and trying to figure out how to deal with backward compatibility for old dts files (assuming folks care). Did I get that right? If so, maybe my "hpd-gpios" is already part of the "connector" node? > > Looking at other usages of "hpd-gpio" in the kernel, it seems like the > > usage I'm proposing is also common. Grepping for "hpd-gpios" shows > > numerous examples of "hpd-gpios" being defined at the display > > controller level and (effectively) I believe the bridge is at the > > equivalent level. > > Bridge drivers should only implement support for features available from > the corresponding hardware. If an HPD signal is connected to a dedicated > pin of the bridge, and the bridge can generate an interrupt and expose > the HPD status through I2C, then it should implement HPD-related > operations. If the HPD pin from the connector is hooked up to a GPIO of > the SoC, it should be handled by the connector bridge driver. So the case I'm trying to deal with is a little odd. I tried to spell it all out in patch #3 [1] but to talk about it here too: 1. The 'ti,sn65dsi86' does have a hardware HPD pin. That pin can generate an interrupt. 2. For reasons described in patch #3 (and the other commit it references, c2bfc223882d), the hardware HPD pin on 'ti,sn65dsi86' is nearly useless for eDP. Specifically, eDP panels are usually (always?) not removable and thus HPD isn't a signal that needs debouncing. ...yet the signal is debounced in hardware on 'ti,sn65dsi86' and that means a delay of 100 - 200ms before you can see the true value of HPD. That's an extra 100 - 200ms before the panel can turn on. 3. Even if eDP panels aren't actually hot plugged, HPD is still a useful concept for eDP. It can be used to avoid hardcoded delays since panels use it to signal when they're ready. ...but if HPD is debounced that doesn't work so well. 4. 'ti,sn65dsi86' has some pins that can be used as GPIOs. These are ideal places to route HPD since they are not debounced and pretty much a perfect fit for this signal (don't waste SoC GPIOs, routing signals on your board is easier, pins are powered exactly when you need them). 5. The GPIOs on 'ti,sn65dsi86' cannot generate IRQs and can only be polled. ...but this is OK. I'm specifically trying to support the case of a panel that is always connected and I just want HPD to be the signal that the panel is ready for me to talk to it. Polling is fine. Specifically the bridge driver doesn't try to poll HPD to decide if we have something connected--it always returns 'connector_status_connected'. ...and this is the correct behavior for eDP because you know the hardware is always there and HPD won't even be asserted until you start to power up the panel. 6. My current implementation in patch #3 actually doesn't fully implement a Linux GPIO provider in the bridge driver. See that patch for justification. While I could do the work to do this and I'll do it if folks insist, I think the current simpler code is nice. If there was a separate "edp-connector" driver then presumably I'd have to add the complexity of implementing the GPIO provider API. I guess to summarize my understanding of all the above: - I think designing and adding a separate 'edp-connector' driver and device tree node and migrating existing users over would be a big chunk of work and is out of scope for me. - I'm hoping that the current approach is still OK. - If people really like the edp-connector concept and want to try to adapt old code later there's nothing here that prevents it--it's just a bunch of extra code. [1] https://lore.kernel.org/r/20200415084758.3.Ia50267a5549392af8b37e67092ca653a59c95886@changeid ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings 2020-04-16 21:53 ` Doug Anderson @ 2020-04-17 18:08 ` Laurent Pinchart 2020-04-21 5:10 ` Doug Anderson 0 siblings, 1 reply; 13+ messages in thread From: Laurent Pinchart @ 2020-04-17 18:08 UTC (permalink / raw) To: Doug Anderson Cc: Stephen Boyd, Andrzej Hajda, David Airlie, Daniel Vetter, Neil Armstrong, Rob Herring, Sandeep Panda, Jonas Karlman, Bjorn Andersson, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jeffrey Hugo, Jernej Skrabec, linux-arm-msm, Rob Clark, dri-devel, LKML Hi Doug, On Thu, Apr 16, 2020 at 02:53:18PM -0700, Doug Anderson wrote: > On Wed, Apr 15, 2020 at 5:54 PM Laurent Pinchart wrote: > > On Wed, Apr 15, 2020 at 04:49:00PM -0700, Doug Anderson wrote: > > > On Wed, Apr 15, 2020 at 1:33 PM Laurent Pinchart wrote: > > > > On Wed, Apr 15, 2020 at 12:53:02PM -0700, Stephen Boyd wrote: > > > > > Quoting Douglas Anderson (2020-04-15 08:48:40) > > > > > > Allow people to specify to use a GPIO for hot-plug-detect. Add an > > > > > > example. > > > > > > > > > > > > NOTE: The current patch adding support for hpd-gpios to the Linux > > > > > > driver for hpd-gpios only adds enough support to the driver so that > > > > > > the bridge can use one of its own GPIOs. The bindings, however, are > > > > > > written generically. > > > > > > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > --- > > > > > > > > > > > > .../bindings/display/bridge/ti,sn65dsi86.yaml | 10 +++++++++- > > > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > > > > index 8cacc6db33a9..554bfd003000 100644 > > > > > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > > > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > > > > @@ -60,6 +60,10 @@ properties: > > > > > > const: 1 > > > > > > description: See ../../pwm/pwm.yaml for description of the cell formats. > > > > > > > > > > > > + hpd-gpios: > > > > > > + maxItems: 1 > > > > > > + description: If present use the given GPIO for hot-plug-detect. > > > > > > > > > > Shouldn't this go in the panel node? And the panel driver should get the > > > > > gpio and poll it after powering up the panel? Presumably that's why we > > > > > have the no-hpd property in the simple panel binding vs. putting it here > > > > > in the bridge. > > > > > > > > Same question really, I think this belongs to the panel (or connector) > > > > node indeed. > > > > > > Hrm. > > > > > > To me "no-hpd" feels OK in the panel because the lack of a connection > > > is somewhat symmetric. Thus it's OK to say either "HPD isn't hooked > > > up to the panel in this system" or "HPD isn't hooked up to the bridge > > > in this system" and both express the same thing (AKA that there is no > > > HPD connection between the bridge and the panel). In the case of > > > "no-hpd" it's more convenient to express it on the panel side because > > > the panel driver is the one whose behavior has to change if HPD isn't > > > hooked up. The panel datasheet is the one that says how long of a > > > delay we need if HPD isn't hooked up. > > > > > > ...but when you're talking about where the bridge driver should look > > > to find the HPD signal that it needs, that really feels like it should > > > be described as part of the bridge. Specifically imagine we were > > > using our bridge for DP, not for eDP. In that case simple-panel > > > wouldn't be involved because we could get any type of display plugged > > > in. Thus it couldn't go in the panel node. Here it feels clearer > > > that hpd-gpio needs to be a property of the bridge driver. > > > > If you were using it for DP, you would need a DT node for the DP > > connector (with bindings to be added to > > Documentation/devicetree/bindings/display/connector/, similar to the > > ones we already have for other connectors). That DT node should > > reference the HPD pin GPIO. The bridge driver for the connector > > (drivers/gpu/drm/bridge/display-connector.c) would then handle HPD. The > > good news is that it already does :-) That's a long e-mail. Taking a deep breath :-) > I'm having a really hard time following, but maybe it's because my > knowledge of the DRM terminology is feeble at best? > > Looking at it from a DRM driver perspective and thus looking in > 'drm/bridge/ti-sn65dsi86.c' I see that the driver for this bridge chip > effectively is both the bridge and the connector. The struct > encapsulating the driver data has both: > > struct drm_bridge bridge; > struct drm_connector connector; > > ...in ti_sn_bridge_attach() the code calls drm_connector_init() for > the connector. That is correct, and that's the old mode. With the new model (based on the DRM_BRIDGE_ATTACH_NO_CONNECTOR flags and the drm_bridge_funcs .get_modes(), .get_edid(), .detect(), .hpd_enable() and .hpd_disabled() operations), the ti-sn65dsi86 will not create a drm_connector anymore. The two modes of operation will be temporarily supported to all display controller drivers to be migrated one by one to the new model. Once the conversion of all display controller drivers that use the ti-sn65dsi86 will be complete, creation of a drm_connector will be removed completely from the ti-sn65dsi86 driver. > Looking at it from a device tree point of view, there is no separate > node representing an eDP connector for one mainline user of > 'ti,sn65dsi86' (sdm845-cheza). The device tree node has one input > port (from "dsi0_out") and one output port (to "panel_in_edp"). There > is no separate connector node as I can see with "hdmi-connector". > ...and, as far as I can tell, sdm845-cheza is using the bindings as > documented. The bindings say that the 'ti,sn65dsi86' node needs two > ports: > - Video port 0 for DSI input > - Video port 1 for eDP output You are right here. The confusing part is the word "connector". Historically, a DRM connector models a physical connector (VGA, HDMI, ...). The concept was extended over time with more connector types, including connectors for display panels (LVDS, DSI, eDP, ...) that are integrated in the system and can't be hotplugged, unlike the user-facing VGA or HDMI connectors. A DRM connector thus represents the sink at the output of a display pipeline. When dealing with external monitors, they can't be represented in the device tree (or any other firmware) as they're hot-pluggable. Panels, however, are different, as they're part of the system, and are thus described in the firmware (DT in this case). Technically speaking, there's a physical connector for the panel, but it's irrelevant for the user, and the end of the display pipeline in the system isn't the connector, but the panel. For this reason, when dealing with panel, there's no DT node to represent the connector. The drm_connector object abstracts either a physical user-facing connector or a panel, and contains properties that are applicable to either the panel or the external monitor (such as the format of the data, the connection type, the physical size of the display, ...). It is part of the API exposed to userspace. Inside the kernel, when the need for panel-specific code arose, the drm_panel object was created. It represents a panel, has properties and methods specific to panels, and is not exposed to userspace. > So, though I'm probably terribly confused, I would tentatively say that: > > - I'd guess that the 'ti,sn65dsi86' bindings were written / approved > back before people were encouraged to model the connector as a > separate node. Not exactly. The physical connector should only be modelled as a separate node when the connector is the output of the display pipeline for the system. As explained above, when the output of the display pipeline is a panel, there's no need for a connector DT node. > - In the case of 'ti,sn65dsi86' the current dts node is both the node > for the bridge and the connector. This one is a bit tricky :-) The DT node for the ti,sn65dsi86 represents the bridge only (it is linked in DT to a separate node for the panel). The corresponding driver instantiates a drm_bridge and a drm_connector, and delegates the drm_connector operations to the drm_panel object that handles the connected panel. This is the model that we're replacing, as the ti-sn65dsi86 driver should focus on the SN65DSI86 chip only, and shouldn't handle the connected panel. For all we know, it could be connected to an eDP to HDMI bridge, itself connected to an HDMI connector. We now want bridge drivers to focus only on the device they correspond to. > - If we want to try to deprecate the way that 'ti,sn65dsi86' works it > feels like a big-ish effort. This would include adding a new "eDP" > connector class and trying to figure out how to deal with backward > compatibility for old dts files (assuming folks care). As I explained above (hopefully in a not too confusing way), no change is required in DT. On the kernel side, the ti-sn65dsi86 should stop creating a connector. The next component in the pipeline, in your case a panel, should be handled by a driver of its own, and in this case this is already possible with the help of drm_panel_bridge_add() (and related functions) that will wrap the drm_panel in a drm_bridge. This currently needs to be done by the ti-sn65dsi86 driver, but once all users of panels will use drm_panel_bridge_add(), I plan to make this handled by the DRM panel core, transparently for bridge drivers. For the case where a physical DP connector is present at the end of the pipeline, connected to an external DP monitor, there shall be a DT node for the DP connector (we have DT bindings for several connector types, creating bindings for DP connectors should be straightforward). The bridge/display-connector.c driver shall be extended to support the new compatible string, and I expect it won't take much more than diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c index 4d278573cdb9..27185f324b2d 100644 --- a/drivers/gpu/drm/bridge/display-connector.c +++ b/drivers/gpu/drm/bridge/display-connector.c @@ -63,10 +63,11 @@ display_connector_detect(struct drm_bridge *bridge) case DRM_MODE_CONNECTOR_Composite: case DRM_MODE_CONNECTOR_SVIDEO: + case DRM_MODE_CONNECTOR_DisplayPort: case DRM_MODE_CONNECTOR_VGA: default: /* - * Composite and S-Video connectors have no other detection + * Composite, S-Video and DP connectors have no other detection * mean than the HPD GPIO. For VGA connectors, even if we have * an I2C bus, we can't assume that the cable is disconnected * if drm_probe_ddc fails, as some cables don't wire the DDC @@ -172,11 +173,12 @@ static int display_connector_probe(struct platform_device *pdev) of_property_read_string(pdev->dev.of_node, "label", &label); /* - * Get the HPD GPIO for DVI and HDMI connectors. If the GPIO can provide - * edge interrupts, register an interrupt handler. + * Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can + * provide edge interrupts, register an interrupt handler. */ if (type == DRM_MODE_CONNECTOR_DVII || - type == DRM_MODE_CONNECTOR_HDMIA) { + type == DRM_MODE_CONNECTOR_HDMIA || + type == DRM_MODE_CONNECTOR_DisplayPort) { conn->hpd_gpio = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN); if (IS_ERR(conn->hpd_gpio)) { @@ -263,6 +265,9 @@ static const struct of_device_id display_connector_match[] = { { .compatible = "composite-video-connector", .data = (void *)DRM_MODE_CONNECTOR_Composite, + }, { + .compatible = "dp-connector", + .data = (void *)DRM_MODE_CONNECTOR_DisplayPort, }, { .compatible = "dvi-connector", .data = (void *)DRM_MODE_CONNECTOR_DVII, With this, the display-connector driver will create a drm_bridge instance for the DP connector. The ti-sn65dsi86 will thus always be followed by a bridge, either a bridge wrapping a drm_panel, or a bridge for the connector. Once all pipelines are fully made of bridges until the last component, the drm_connector shall be created by the display controller driver, which shall pass the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to the bridges to prevent them from creating a drm_connector themselves. The drm_connector created by the display controller driver will need to implement operations such as .get_modes(), which will be delegated to the appropriate bridge in the chain. The drm_bridge_connector_init() helper automates this, making it straightforward for the display controller driver to create such a drm_connector. As explained in the helper's documentation, * To use the helper, display controller drivers create a bridge connector with * a call to drm_bridge_connector_init(). This associates the newly created * connector with the chain of bridges passed to the function and registers it * with the DRM device. At that point the connector becomes fully usable, no * further operation is needed. > Did I get that right? If so, maybe my "hpd-gpios" is already part of > the "connector" node? You got it mostly right :-) As for the hpd-gpios, it should be specified in the DT node of the component that provides the HPD signal, and contain a GPIO specifier describing what the signal is connected to. When dealing with a physical DP connector and external monitor, the HPD signal is provided by the DP connector, the hpd-gpios property shall then be specified in the DP connector DT node. The display-connector driver already handles that property. When dealing with an eDP panel, the HPD signal is provided by the panel, the hpd-gpios property shall be specified in the panel DT node. > > > Looking at other usages of "hpd-gpio" in the kernel, it seems like the > > > usage I'm proposing is also common. Grepping for "hpd-gpios" shows > > > numerous examples of "hpd-gpios" being defined at the display > > > controller level and (effectively) I believe the bridge is at the > > > equivalent level. > > > > Bridge drivers should only implement support for features available from > > the corresponding hardware. If an HPD signal is connected to a dedicated > > pin of the bridge, and the bridge can generate an interrupt and expose > > the HPD status through I2C, then it should implement HPD-related > > operations. If the HPD pin from the connector is hooked up to a GPIO of > > the SoC, it should be handled by the connector bridge driver. > > So the case I'm trying to deal with is a little odd. I tried to spell > it all out in patch #3 [1] but to talk about it here too: > > 1. The 'ti,sn65dsi86' does have a hardware HPD pin. That pin can > generate an interrupt. As the SN65DSI86 has native HPD detect capability with a dedicated HPD input (note that this doesn't make the SN65DSI86 a providder of the HPD signal in the sense described above), the bridge driver, in the new model, shall implement the HPD-related operations and the .detect() operation. The drm_bridge_connector_init() helper will then delegate HPD and detection to the ti-sn65dsi86 driver. This assumes that the HPD signal is routed to the SN65DSI86. That's not always the case, see below for an explanation of how to handle that. > 2. For reasons described in patch #3 (and the other commit it > references, c2bfc223882d), the hardware HPD pin on 'ti,sn65dsi86' is > nearly useless for eDP. Specifically, eDP panels are usually > (always?) not removable and thus HPD isn't a signal that needs > debouncing. ...yet the signal is debounced in hardware on > 'ti,sn65dsi86' and that means a delay of 100 - 200ms before you can > see the true value of HPD. That's an extra 100 - 200ms before the > panel can turn on. > > 3. Even if eDP panels aren't actually hot plugged, HPD is still a > useful concept for eDP. It can be used to avoid hardcoded delays > since panels use it to signal when they're ready. ...but if HPD is > debounced that doesn't work so well. > > 4. 'ti,sn65dsi86' has some pins that can be used as GPIOs. These are > ideal places to route HPD since they are not debounced and pretty much > a perfect fit for this signal (don't waste SoC GPIOs, routing signals > on your board is easier, pins are powered exactly when you need them). The new drm_bridge model has support for this use case. It makes a difference between the intrinsic capability of a device to provide a feature (for instance the SN65DSI86 has the intrinsic capability to provide the HPD feature), and the fact that the feature is actually provided by that device on a particular system (in the case you describe here, the SN65DSI86 intrinsic HPD capability isn't used, as the HPD signal isn't connect to the SN65DSI86 HPD input). The former is reported by implementing the corresponding drm_bridge_funcs operations, the latter is reported by setting DRM_BRIGE_OP_* flags in drm_bridge.ops. This mechanism allows bridge drivers to unconditionally set function pointers in their drm_bridge_funcs structure (allowing the structure to make stored in read-only memory), while exposing, for each device instance, whether the feature is actually provided or not. The drm_bridge_connector_init() helper, to delegate drm_connector operations to bridges, will look for the first bridge in the chain, starting at the output of the pipeline (connector or panel), that supports the corresponding feature. If your DP connector DT node, or your eDP connector DT node, specifies that the HPD signal is routed to a GPIO (through the hpd-gpios property), then the corresponding bridge driver shall reprot the DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD capabilities. The display-connector driver already supports this, the panel bridge driver doesn't and needs to be extended. The drm_bridge_connector_init() helper will then detect that the drm_bridge for the DP connector or eDP panel supports HPD, and will delegate the related drm_connector operations to that bridge. If the HPD signal is routed to the HPD pin of the SN65DSI86, the DP connector or eDP panel DT node should not contain an hpd-gpios property, the corresponding drm_bridge will not set DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD, and the drm_bridge_connector_init() will look at the next component in the next bridge in the chain, which will be the ti-sn65dsi86. That bridge will report support for the HPD-related operations, and will be used. To be fully correct the ti-sn65dsi86 shouldn't set the DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD flags when the HPD signal isn't routed to its HPD input pin. As it should not peek into the DT node of the DP connector or eDP panel for its output, it should have an additional no-hpd DT property in this case. In practice that's may not always be required, as if an hpd-gpios property is specified in the DP connector or eDP panel DT node, the drm_bridge_connector_init() will not look further, but for the case where the HPD signal isn't routed anywhere, we need to make sure that the ti-sn65dsi86 driver will not incorrectly advertise HPD support. > 5. The GPIOs on 'ti,sn65dsi86' cannot generate IRQs and can only be > polled. ...but this is OK. I'm specifically trying to support the > case of a panel that is always connected and I just want HPD to be the > signal that the panel is ready for me to talk to it. Polling is fine. > Specifically the bridge driver doesn't try to poll HPD to decide if we > have something connected--it always returns > 'connector_status_connected'. ...and this is the correct behavior for > eDP because you know the hardware is always there and HPD won't even > be asserted until you start to power up the panel. If you look at bridge/display-connector.c, you will see that it reports DRM_BRIDGE_OP_DETECT if there's an hpd-gpios property, and additionally reports DRM_BRIDGE_OP_HPD if that GPIO has interrupt capability. If a bridge in the pipeline reports DRM_BRIDGE_OP_DETECT but no bridge reports DRM_BRIDGE_OP_HPD, drm_bridge_connector_init() creates a connector that uses polling. This is another reason why a no-hpd property is needed for the ti,sn65dsi86, as otherwise the helper would incorrectly consider that the SN65DSI86 will report HPD through an interrupt. > 6. My current implementation in patch #3 actually doesn't fully > implement a Linux GPIO provider in the bridge driver. See that patch > for justification. While I could do the work to do this and I'll do > it if folks insist, I think the current simpler code is nice. If > there was a separate "edp-connector" driver then presumably I'd have > to add the complexity of implementing the GPIO provider API. This is the only reason why I don't like asking you to change your implementation, due to the additional complexity required to expose a GPIO provider. However, I think that the new bridge usage model is much cleaner than the current one, and this justifies in my opinion additional complexity in a small number of places, even if it's unfortunate. That being said, if we can put the DT properties where they belong for the new model with isolated bridge drivers to only handle the features of the hardware they correspond to, I wouldn't be opposed to a localized hack (without any derogatory meaning implied) on the driver side to ease the implementation. I'm willing to look at you at how this could be done, once we complete this discussion about the new model, with the hard rule that DT bindings should be designed based on the new model. > I guess to summarize my understanding of all the above: > > - I think designing and adding a separate 'edp-connector' driver and > device tree node and migrating existing users over would be a big > chunk of work and is out of scope for me. > > - I'm hoping that the current approach is still OK. > > - If people really like the edp-connector concept and want to try to > adapt old code later there's nothing here that prevents it--it's just > a bunch of extra code. > > > [1] https://lore.kernel.org/r/20200415084758.3.Ia50267a5549392af8b37e67092ca653a59c95886@changeid -- Regards, Laurent Pinchart ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings 2020-04-17 18:08 ` Laurent Pinchart @ 2020-04-21 5:10 ` Doug Anderson 0 siblings, 0 replies; 13+ messages in thread From: Doug Anderson @ 2020-04-21 5:10 UTC (permalink / raw) To: Laurent Pinchart Cc: Stephen Boyd, Andrzej Hajda, David Airlie, Daniel Vetter, Neil Armstrong, Rob Herring, Sandeep Panda, Jonas Karlman, Bjorn Andersson, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jeffrey Hugo, Jernej Skrabec, linux-arm-msm, Rob Clark, dri-devel, LKML Hi, On Fri, Apr 17, 2020 at 11:08 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > As for the hpd-gpios, it should be specified in the DT node of the > component that provides the HPD signal, and contain a GPIO specifier > describing what the signal is connected to. When dealing with a physical > DP connector and external monitor, the HPD signal is provided by the DP > connector, the hpd-gpios property shall then be specified in the DP > connector DT node. The display-connector driver already handles that > property. When dealing with an eDP panel, the HPD signal is provided by > the panel, the hpd-gpios property shall be specified in the panel DT > node. OK, patch posted to add "hpd-gpios" to "panel-common.yaml" which is I think the summary of what you're saying above. I _think_ this also means that I need to add support to panel-simple.c for it so I've posted got a patch for that. If I followed your whole description of the future plans it might eventually move somewhere else but we're not there yet. If I screwed this up hopefully it's OK to continue the conversation in v2. It seemed nice to have code to talk about. > As the SN65DSI86 has native HPD detect capability with a dedicated HPD > input (note that this doesn't make the SN65DSI86 a providder of the HPD > signal in the sense described above), the bridge driver, in the new > model, shall implement the HPD-related operations and the .detect() > operation. The drm_bridge_connector_init() helper will then delegate HPD > and detection to the ti-sn65dsi86 driver. I guess this assumes that anyone ever uses it. Right now the driver hardcodes HPD to be off and it seems hard for me to imagine anyone would have a real use for the hardware line given the terrible debouncing. Maybe a panel whose hardcoded delay is super bad? > The new drm_bridge model has support for this use case. It makes a > difference between the intrinsic capability of a device to provide a > feature (for instance the SN65DSI86 has the intrinsic capability to > provide the HPD feature), and the fact that the feature is actually > provided by that device on a particular system (in the case you describe > here, the SN65DSI86 intrinsic HPD capability isn't used, as the HPD > signal isn't connect to the SN65DSI86 HPD input). The former is reported > by implementing the corresponding drm_bridge_funcs operations, the > latter is reported by setting DRM_BRIGE_OP_* flags in drm_bridge.ops. > This mechanism allows bridge drivers to unconditionally set function > pointers in their drm_bridge_funcs structure (allowing the structure to > make stored in read-only memory), while exposing, for each device > instance, whether the feature is actually provided or not. > > The drm_bridge_connector_init() helper, to delegate drm_connector > operations to bridges, will look for the first bridge in the chain, > starting at the output of the pipeline (connector or panel), that > supports the corresponding feature. If your DP connector DT node, or > your eDP connector DT node, specifies that the HPD signal is routed to a > GPIO (through the hpd-gpios property), then the corresponding bridge > driver shall reprot the DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD > capabilities. The display-connector driver already supports this, the > panel bridge driver doesn't and needs to be extended. The > drm_bridge_connector_init() helper will then detect that the drm_bridge > for the DP connector or eDP panel supports HPD, and will delegate the > related drm_connector operations to that bridge. If the HPD signal is > routed to the HPD pin of the SN65DSI86, the DP connector or eDP panel DT > node should not contain an hpd-gpios property, the corresponding > drm_bridge will not set DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD, and > the drm_bridge_connector_init() will look at the next component in the > next bridge in the chain, which will be the ti-sn65dsi86. That bridge > will report support for the HPD-related operations, and will be used. > > To be fully correct the ti-sn65dsi86 shouldn't set the > DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD flags when the HPD signal > isn't routed to its HPD input pin. As it should not peek into the DT > node of the DP connector or eDP panel for its output, it should have an > additional no-hpd DT property in this case. In practice that's may not > always be required, as if an hpd-gpios property is specified in the DP > connector or eDP panel DT node, the drm_bridge_connector_init() will not > look further, but for the case where the HPD signal isn't routed > anywhere, we need to make sure that the ti-sn65dsi86 driver will not > incorrectly advertise HPD support. Sounds like you've thought out a lot of the corner cases! Right now the 'ti-sn65dsi86' driver is hardcoded not to look at HPD but its bindings doesn't have the 'no-hpd' property. Sounds like that should be OK-ish as long as the panel either has "hpd-gpios" or "no-hpd" because then nobody will actually query the bridge. ...but it would be cleaner to add it. > > 5. The GPIOs on 'ti,sn65dsi86' cannot generate IRQs and can only be > > polled. ...but this is OK. I'm specifically trying to support the > > case of a panel that is always connected and I just want HPD to be the > > signal that the panel is ready for me to talk to it. Polling is fine. > > Specifically the bridge driver doesn't try to poll HPD to decide if we > > have something connected--it always returns > > 'connector_status_connected'. ...and this is the correct behavior for > > eDP because you know the hardware is always there and HPD won't even > > be asserted until you start to power up the panel. > > If you look at bridge/display-connector.c, you will see that it reports > DRM_BRIDGE_OP_DETECT if there's an hpd-gpios property, and additionally > reports DRM_BRIDGE_OP_HPD if that GPIO has interrupt capability. If a > bridge in the pipeline reports DRM_BRIDGE_OP_DETECT but no bridge > reports DRM_BRIDGE_OP_HPD, drm_bridge_connector_init() creates a > connector that uses polling. This is another reason why a no-hpd > property is needed for the ti,sn65dsi86, as otherwise the helper would > incorrectly consider that the SN65DSI86 will report HPD through an > interrupt. Hrm. I guess technically it breaks bindings compatibility that "no-hpd" wasn't there before but there's something that will break if we don't specify it. ...but it won't break anything until someone actually tries to add DRM_BRIDGE_OP_HPD to ti-sn65dsi86. Maybe we're OK as long as we fix it before then? I've put this in v2 so we can discuss. > > 6. My current implementation in patch #3 actually doesn't fully > > implement a Linux GPIO provider in the bridge driver. See that patch > > for justification. While I could do the work to do this and I'll do > > it if folks insist, I think the current simpler code is nice. If > > there was a separate "edp-connector" driver then presumably I'd have > > to add the complexity of implementing the GPIO provider API. > > This is the only reason why I don't like asking you to change your > implementation, due to the additional complexity required to expose a > GPIO provider. However, I think that the new bridge usage model is much > cleaner than the current one, and this justifies in my opinion > additional complexity in a small number of places, even if it's > unfortunate. That being said, if we can put the DT properties where they > belong for the new model with isolated bridge drivers to only handle the > features of the hardware they correspond to, I wouldn't be opposed to a > localized hack (without any derogatory meaning implied) on the driver > side to ease the implementation. I'm willing to look at you at how this > could be done, once we complete this discussion about the new model, > with the hard rule that DT bindings should be designed based on the new > model. OK, I managed to implement the GPIO controller. Let's see how it looks. I threw GPIO folks on the series too so hopefully they can tell me if I'm doing something stupid. -Doug ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] drm/bridge: ti-sn65dsi86: Allow one of the bridge GPIOs to be HPD 2020-04-15 15:48 [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml Douglas Anderson 2020-04-15 15:48 ` [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings Douglas Anderson @ 2020-04-15 15:48 ` Douglas Anderson 2020-04-15 19:50 ` [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml Stephen Boyd 2020-04-15 20:31 ` Laurent Pinchart 3 siblings, 0 replies; 13+ messages in thread From: Douglas Anderson @ 2020-04-15 15:48 UTC (permalink / raw) To: airlied, daniel, robh+dt, narmstrong, a.hajda, Laurent.pinchart, spanda Cc: jonas, bjorn.andersson, devicetree, jeffrey.l.hugo, swboyd, jernej.skrabec, linux-arm-msm, robdclark, dri-devel, Douglas Anderson, linux-kernel As talked about in commit c2bfc223882d ("drm/bridge: ti-sn65dsi86: Remove the mystery delay"), the normal HPD pin on ti-sn65dsi86 is kinda useless, at least for embedded DisplayPort (eDP). However, despite the fact that the actual HPD pin on the bridge is mostly useless for eDP, the concept of HPD for eDP still makes sense. It allows us to optimize out a hardcoded delay that many panels need if HPD isn't hooked up. Panel timing diagrams show HPD as one of the events to measure timing from and we have to assume the worst case if we can't actually read HPD. One way to use HPD for eDP without using the mostly useless HPD pin on ti-sn65dsi86 is to route the panel's HPD somewhere else in the system, like to a GPIO. This works great because eDP panels aren't physically hotplugged. That means the debouncing logic that caused us problems wasn't really needed and a raw GPIO works great. As per the above, a smart board designer would realize the value of HPD and choose to route it to a GPIO somewhere on the board to avoid the silly sn65dsi86 debouncing. While said "smart designer" could theoretically route HPD anywhere on the board, a really smart designer would realize that there are several GPIOs on the bridge itself that are nearly useless for anything but this purpose and route HPD to one of those. Specifically, to argue that the GPIOs on the bridge are mostly useless for non-bridge-related things: - You need to power on the bridge to use them. Ideally a system only powers on the bridge when the screen is on which means you could only use the GPIOs on the bridge when the screen is on (or you're willing to burn a bunch of extra power). - You need to control the GPIOs via i2c commands, which means that you can only use these GPIOs for drivers that can call gpio_set_value_cansleep(). With the above, let's assume that: - If someone was going to route the HPD to a GPIO they'd use one of the GPIOs on the bridge. - There's not lots of value exposing the GPIOs on the bridge through the normal Linux GPIO framework because nobody will likely use them for something non-bridge-related. As you can probably guess from the above arguments, this patch adds the ability to use one of the bridge's GPIOs as the HPD pin but it doesn't add it in the completely generic (and a bit heavier) way of going through the GPIO subsystem. This means: - With this patch you can't use bridge GPIOs for non-bridge purposes. - With this patch you can't use a different GPIO in the SoC for HPD. Despite the above limitations, which keep the code simpler, we still use the generic GPIO device tree bindings. If someone later has a need to relax some of the restrictions here, it would just need a code patch. We wouldn't need to change the device tree bindings. This patch has been tested on a board that is not yet mainline. On the hardware I have: - Panel spec says HPD could take up to 200 ms to come up, so without HPD hooked up we need to delay 200 ms. - On my board the panel is powered by the same rail as the touchscreen. By chance of probe order the touchscreen comes up first. This means by the time we check HPD in ti_sn_bridge_enable() it's already up. Thus we can use the panel on 200 ms earlier. - If I hack out the touscreen, I see that I wait ~31 ms for HPD to go high. This means I save ~169 ms of delay. - If I measure HPD on this pane it comes up ~56 ms after the panel is powered. The delta between 31 and 56 ms is because the panel regulator is turned on at the end of ti_sn_bridge_pre_enable() in drm_panel_prepare(). There is apparently some time between that and ti_sn_bridge_enable(). Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 107 ++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 6ad688b320ae..187b2bdd0cb4 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -54,6 +54,13 @@ #define BPP_18_RGB BIT(0) #define SN_HPD_DISABLE_REG 0x5C #define HPD_DISABLE BIT(0) +#define SN_GPIO_IO_REG 0x5E +#define SN_GPIO_INPUT_SHIFT 4 +#define SN_GPIO_OUTPUT_SHIFT 0 +#define SN_GPIO_CTRL_REG 0x5F +#define SN_GPIO_MUX_INPUT 0 +#define SN_GPIO_MUX_OUTPUT 1 +#define SN_GPIO_MUX_SPECIAL 2 #define SN_AUX_WDATA_REG(x) (0x64 + (x)) #define SN_AUX_ADDR_19_16_REG 0x74 #define SN_AUX_ADDR_15_8_REG 0x75 @@ -102,6 +109,7 @@ struct ti_sn_bridge { struct gpio_desc *enable_gpio; struct regulator_bulk_data supplies[SN_REGULATOR_SUPPLY_NUM]; int dp_lanes; + int hpd_gpio_pin; }; static const struct regmap_range ti_sn_bridge_volatile_ranges[] = { @@ -120,6 +128,45 @@ static const struct regmap_config ti_sn_bridge_regmap_config = { .cache_type = REGCACHE_NONE, }; +/** + * ti_sn_read_gpio() - Read a GPIO pin. + * @pdata: Platform data. + * @gpio_num: The GPIO to read. This is 1-based to match the pin names so + * valid values are 1-4. + * + * This function assumes the pin direction / muxing is already configured. + * + * Return: 0 if the pin is low; 1 if the pin is high; -error upon failure + */ +static int ti_sn_read_gpio(struct ti_sn_bridge *pdata, int gpio_num) +{ + unsigned int val; + int ret; + + ret = regmap_read(pdata->regmap, SN_GPIO_IO_REG, &val); + if (ret) + return ret; + + return (val >> (SN_GPIO_INPUT_SHIFT + gpio_num - 1)) & 1; +} + +/** + * ti_sn_setup_mux() - Setup a GPIO pinmux. + * @pdata: Platform data. + * @gpio_num: The GPIO to setup. This is 1-based to match the pin names so + * valid values are 1-4. + * @val: The mux value; One of the SN_GPIO_MUX_... constants. + * + * Return: 0 if success; either error. + */ +static int ti_sn_setup_mux(struct ti_sn_bridge *pdata, int gpio_num, int val) +{ + int shift = (gpio_num - 1) * 2; + + return regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG, + 0x3 << shift, val << shift); +} + static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata, unsigned int reg, u16 val) { @@ -658,6 +705,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, return ret; } +static bool ti_sn_read_hpd_gpio(struct ti_sn_bridge *pdata) +{ + return ti_sn_read_gpio(pdata, pdata->hpd_gpio_pin) == 1; +} + static void ti_sn_bridge_enable(struct drm_bridge *bridge) { struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); @@ -666,6 +718,25 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) int dp_rate_idx; unsigned int val; int ret = -EINVAL; + bool hpd_asserted; + + /* + * On some designs HPD could be routed to one of the GPIOs on the + * bridge. In that case, delay up to 2 seconds waiting for HPD to be + * asserted. + */ + if (pdata->hpd_gpio_pin) { + ret = ti_sn_setup_mux(pdata, pdata->hpd_gpio_pin, + SN_GPIO_MUX_INPUT); + if (ret) { + DRM_ERROR("failed to setup HPD mux\n"); + return; + } + + ret = readx_poll_timeout(ti_sn_read_hpd_gpio, pdata, + hpd_asserted, hpd_asserted, + 1000, 2000000); + } /* * Run with the maximum number of lanes that the DP sink supports. @@ -874,6 +945,40 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata) return 0; } +static void ti_sn_probe_hpd_gpio(struct ti_sn_bridge *pdata) +{ + struct device_node *np = pdata->dev->of_node; + int num_elems; + u32 hpd_gpio_pin; + + num_elems = of_property_count_u32_elems(np, "hpd-gpios"); + + /* It's optional, so no worries if it's not there */ + if (num_elems == -EINVAL) + return; + + if (num_elems != 3) { + dev_warn(pdata->dev, + "Unexpected hpd-gpios count (%d); ignoring\n", + num_elems); + return; + } + + /* + * Right now, we only support using one of our own GPIOs for + * this, but our device tree bindings are more generic. Confirm + * we're actually a client of ourselves. + */ + if (of_parse_phandle(np, "hpd-gpios", 0) != np) { + dev_warn(pdata->dev, + "Only bridge-local hpd-gpios supported; ignoring\n"); + return; + } + + if (!of_property_read_u32_index(np, "hpd-gpios", 1, &hpd_gpio_pin)) + pdata->hpd_gpio_pin = hpd_gpio_pin; +} + static int ti_sn_bridge_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -916,6 +1021,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, return ret; } + ti_sn_probe_hpd_gpio(pdata); + ret = ti_sn_bridge_parse_regulators(pdata); if (ret) { DRM_ERROR("failed to parse regulators\n"); -- 2.26.0.110.g2183baf09c-goog ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml 2020-04-15 15:48 [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml Douglas Anderson 2020-04-15 15:48 ` [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings Douglas Anderson 2020-04-15 15:48 ` [PATCH 3/3] drm/bridge: ti-sn65dsi86: Allow one of the bridge GPIOs to be HPD Douglas Anderson @ 2020-04-15 19:50 ` Stephen Boyd 2020-04-15 20:31 ` Laurent Pinchart 3 siblings, 0 replies; 13+ messages in thread From: Stephen Boyd @ 2020-04-15 19:50 UTC (permalink / raw) To: Douglas Anderson, Laurent.pinchart, a.hajda, airlied, daniel, narmstrong, robh+dt, spanda Cc: jonas, bjorn.andersson, devicetree, jeffrey.l.hugo, swboyd, jernej.skrabec, linux-arm-msm, robdclark, dri-devel, Douglas Anderson, Krzysztof Kozlowski, Paul Walmsley, linux-kernel Quoting Douglas Anderson (2020-04-15 08:48:39) > This moves the bindings over, based a lot on toshiba,tc358768.yaml. > Unless there's someone known to be better, I've set the maintainer in > the yaml as the first person to submit bindings. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- Awesome! > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > new file mode 100644 > index 000000000000..8cacc6db33a9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > @@ -0,0 +1,188 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/bridge/ti,sn65dsi86.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: SN65DSI86 DSI to eDP bridge chip > + > +maintainers: > + - Sandeep Panda <spanda@codeaurora.org> > + > +description: | > + The Texas Instruments SN65DSI86 bridge takes MIPI DSI in and outputs eDP. > + http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf > + > +properties: > + compatible: > + const: ti,sn65dsi86 > + > + reg: > + const: 0x2d > + > + enable-gpios: > + maxItems: 1 > + description: GPIO specification for bridge_en pin (active high). s/specification/specifier/ ? I know the previous binding said specification but I don't know what that is. It's a specifier. > + > + vccio-supply: > + description: A 1.8V supply that powers up the digital IOs. > + > + vpll-supply: > + description: A 1.8V supply that powers up the DisplayPort PLL. > + > + vcca-supply: > + description: A 1.2V supply that powers up the analog circuits. > + > + vcc-supply: > + description: A 1.2V supply that powers up the digital core. Nitpick: Can we remove 'up' from these descriptions? > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + description: > + Specification for input reference clock. The reference clock rate must Clock specifier for input reference clock? > + be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz. > + > + clock-names: > + const: refclk > + ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml 2020-04-15 15:48 [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml Douglas Anderson ` (2 preceding siblings ...) 2020-04-15 19:50 ` [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml Stephen Boyd @ 2020-04-15 20:31 ` Laurent Pinchart 2020-04-21 5:07 ` Doug Anderson 3 siblings, 1 reply; 13+ messages in thread From: Laurent Pinchart @ 2020-04-15 20:31 UTC (permalink / raw) To: Douglas Anderson Cc: airlied, daniel, robh+dt, narmstrong, a.hajda, spanda, jonas, bjorn.andersson, devicetree, jeffrey.l.hugo, swboyd, jernej.skrabec, linux-arm-msm, robdclark, dri-devel, Krzysztof Kozlowski, Paul Walmsley, Stephen Boyd, linux-kernel Hi Douglas, Thank you for the patch. On Wed, Apr 15, 2020 at 08:48:39AM -0700, Douglas Anderson wrote: > This moves the bindings over, based a lot on toshiba,tc358768.yaml. > Unless there's someone known to be better, I've set the maintainer in > the yaml as the first person to submit bindings. You can also add your name ;-) > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > .../bindings/display/bridge/ti,sn65dsi86.txt | 87 -------- > .../bindings/display/bridge/ti,sn65dsi86.yaml | 188 ++++++++++++++++++ > 2 files changed, 188 insertions(+), 87 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt > create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt > deleted file mode 100644 > index 8ec4a7f2623a..000000000000 > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt > +++ /dev/null > @@ -1,87 +0,0 @@ > -SN65DSI86 DSI to eDP bridge chip > --------------------------------- > - > -This is the binding for Texas Instruments SN65DSI86 bridge. > -http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf > - > -Required properties: > -- compatible: Must be "ti,sn65dsi86" > -- reg: i2c address of the chip, 0x2d as per datasheet > -- enable-gpios: gpio specification for bridge_en pin (active high) > - > -- vccio-supply: A 1.8V supply that powers up the digital IOs. > -- vpll-supply: A 1.8V supply that powers up the displayport PLL. > -- vcca-supply: A 1.2V supply that powers up the analog circuits. > -- vcc-supply: A 1.2V supply that powers up the digital core. > - > -Optional properties: > -- interrupts-extended: Specifier for the SN65DSI86 interrupt line. > - > -- gpio-controller: Marks the device has a GPIO controller. > -- #gpio-cells : Should be two. The first cell is the pin number and > - the second cell is used to specify flags. > - See ../../gpio/gpio.txt for more information. > -- #pwm-cells : Should be one. See ../../pwm/pwm.yaml for description of > - the cell formats. > - > -- clock-names: should be "refclk" > -- clocks: Specification for input reference clock. The reference > - clock rate must be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz. > - > -- data-lanes: See ../../media/video-interface.txt > -- lane-polarities: See ../../media/video-interface.txt > - > -- suspend-gpios: specification for GPIO1 pin on bridge (active low) Where has suspend-gpios gone ? :-) > - > -Required nodes: > -This device has two video ports. Their connections are modelled using the > -OF graph bindings specified in Documentation/devicetree/bindings/graph.txt. > - > -- Video port 0 for DSI input > -- Video port 1 for eDP output > - > -Example > -------- > - > -edp-bridge@2d { > - compatible = "ti,sn65dsi86"; > - #address-cells = <1>; > - #size-cells = <0>; > - reg = <0x2d>; > - > - enable-gpios = <&msmgpio 33 GPIO_ACTIVE_HIGH>; > - suspend-gpios = <&msmgpio 34 GPIO_ACTIVE_LOW>; > - > - interrupts-extended = <&gpio3 4 IRQ_TYPE_EDGE_FALLING>; > - > - vccio-supply = <&pm8916_l17>; > - vcca-supply = <&pm8916_l6>; > - vpll-supply = <&pm8916_l17>; > - vcc-supply = <&pm8916_l6>; > - > - clock-names = "refclk"; > - clocks = <&input_refclk>; > - > - ports { > - #address-cells = <1>; > - #size-cells = <0>; > - > - port@0 { > - reg = <0>; > - > - edp_bridge_in: endpoint { > - remote-endpoint = <&dsi_out>; > - }; > - }; > - > - port@1 { > - reg = <1>; > - > - edp_bridge_out: endpoint { > - data-lanes = <2 1 3 0>; > - lane-polarities = <0 1 0 1>; > - remote-endpoint = <&edp_panel_in>; > - }; > - }; > - }; > -} > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > new file mode 100644 > index 000000000000..8cacc6db33a9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > @@ -0,0 +1,188 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/bridge/ti,sn65dsi86.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: SN65DSI86 DSI to eDP bridge chip > + > +maintainers: > + - Sandeep Panda <spanda@codeaurora.org> > + > +description: | > + The Texas Instruments SN65DSI86 bridge takes MIPI DSI in and outputs eDP. > + http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf > + > +properties: > + compatible: > + const: ti,sn65dsi86 > + > + reg: > + const: 0x2d > + > + enable-gpios: > + maxItems: 1 > + description: GPIO specification for bridge_en pin (active high). > + > + vccio-supply: > + description: A 1.8V supply that powers up the digital IOs. > + > + vpll-supply: > + description: A 1.8V supply that powers up the DisplayPort PLL. > + > + vcca-supply: > + description: A 1.2V supply that powers up the analog circuits. > + > + vcc-supply: > + description: A 1.2V supply that powers up the digital core. > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + description: > + Specification for input reference clock. The reference clock rate must > + be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz. > + > + clock-names: > + const: refclk > + > + gpio-controller: true > + '#gpio-cells': > + const: 2 > + description: > + First cell is pin number, second cell is flags. GPIO pin numbers are > + 1-based to match the datasheet. See ../../gpio/gpio.txt for more > + information. > + > + '#pwm-cells': > + const: 1 > + description: See ../../pwm/pwm.yaml for description of the cell formats. > + > + data-lanes: Should this have minItems: 1 maxItems: 4 items: enum: - 0 - 1 - 2 - 3 > + description: See ../../media/video-interface.txt > + > + lane-polarities: > + description: See ../../media/video-interface.txt And something similar here, minItems: 1 maxItems: 4 items: enum: - 0 - 1 uniqueItems: false I'm not entirely sure where uniqueItems should be placed. I'm also not sure how to specify that both data-lanes and lane-polarities need to have the same number of items, maybe dependencies: data-lanes: [lane-polarities] > + > + ports: > + type: object > + > + properties: > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 > + > + port@0: > + type: object > + additionalProperties: false > + > + description: > + Video port for MIPI DSI input > + > + properties: > + reg: > + const: 0 > + > + patternProperties: > + endpoint: If there's a single endpoint, you don't need patternProperties, it can be specified in properties. > + type: object > + additionalProperties: false > + > + properties: > + remote-endpoint: true > + > + required: > + - reg > + > + port@1: > + type: object > + additionalProperties: false > + > + description: > + Video port for eDP output (panel or connector). > + > + properties: > + reg: > + const: 1 > + > + patternProperties: > + endpoint: Same here. > + type: object > + additionalProperties: false > + > + properties: > + remote-endpoint: true > + > + required: > + - reg > + > + required: > + - "#address-cells" > + - "#size-cells" > + - port@0 > + - port@1 > + > +required: > + - compatible > + - reg > + - enable-gpios > + - vccio-supply > + - vpll-supply > + - vcca-supply > + - vcc-supply > + - ports > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/qcom,rpmh.h> > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + i2c1 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + bridge@2d { > + compatible = "ti,sn65dsi86"; > + reg = <0x2d>; > + > + interrupt-parent = <&tlmm>; > + interrupts = <10 IRQ_TYPE_LEVEL_HIGH>; > + > + enable-gpios = <&tlmm 102 GPIO_ACTIVE_HIGH>; > + > + vpll-supply = <&src_pp1800_s4a>; > + vccio-supply = <&src_pp1800_s4a>; > + vcca-supply = <&src_pp1200_l2a>; > + vcc-supply = <&src_pp1200_l2a>; > + > + clocks = <&rpmhcc RPMH_LN_BB_CLK2>; > + clock-names = "refclk"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + endpoint { > + remote-endpoint = <&dsi0_out>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + endpoint { > + remote-endpoint = <&panel_in_edp>; > + }; > + }; > + }; > + }; > + }; > + -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml 2020-04-15 20:31 ` Laurent Pinchart @ 2020-04-21 5:07 ` Doug Anderson 0 siblings, 0 replies; 13+ messages in thread From: Doug Anderson @ 2020-04-21 5:07 UTC (permalink / raw) To: Laurent Pinchart Cc: David Airlie, Daniel Vetter, Rob Herring, Neil Armstrong, Andrzej Hajda, Sandeep Panda, Jonas Karlman, Bjorn Andersson, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Jeffrey Hugo, Stephen Boyd, Jernej Skrabec, linux-arm-msm, Rob Clark, dri-devel, Krzysztof Kozlowski, Paul Walmsley, Stephen Boyd, LKML Hi, On Wed, Apr 15, 2020 at 1:31 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Douglas, > > Thank you for the patch. > > On Wed, Apr 15, 2020 at 08:48:39AM -0700, Douglas Anderson wrote: > > This moves the bindings over, based a lot on toshiba,tc358768.yaml. > > Unless there's someone known to be better, I've set the maintainer in > > the yaml as the first person to submit bindings. > > You can also add your name ;-) Sure, though I spend most of my days flitting from subsystem to subsystem, always a noob everywhere I go. I'm not sure I'd really be qualified. ;-) If you want, I can add myself though I'd rather not be solely responsible for this file since I probably won't be the best at keeping track of it... I left this as-is for v2 but can change it if you want. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > .../bindings/display/bridge/ti,sn65dsi86.txt | 87 -------- > > .../bindings/display/bridge/ti,sn65dsi86.yaml | 188 ++++++++++++++++++ > > 2 files changed, 188 insertions(+), 87 deletions(-) > > delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt > > create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt > > deleted file mode 100644 > > index 8ec4a7f2623a..000000000000 > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt > > +++ /dev/null > > @@ -1,87 +0,0 @@ > > -SN65DSI86 DSI to eDP bridge chip > > --------------------------------- > > - > > -This is the binding for Texas Instruments SN65DSI86 bridge. > > -http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf > > - > > -Required properties: > > -- compatible: Must be "ti,sn65dsi86" > > -- reg: i2c address of the chip, 0x2d as per datasheet > > -- enable-gpios: gpio specification for bridge_en pin (active high) > > - > > -- vccio-supply: A 1.8V supply that powers up the digital IOs. > > -- vpll-supply: A 1.8V supply that powers up the displayport PLL. > > -- vcca-supply: A 1.2V supply that powers up the analog circuits. > > -- vcc-supply: A 1.2V supply that powers up the digital core. > > - > > -Optional properties: > > -- interrupts-extended: Specifier for the SN65DSI86 interrupt line. > > - > > -- gpio-controller: Marks the device has a GPIO controller. > > -- #gpio-cells : Should be two. The first cell is the pin number and > > - the second cell is used to specify flags. > > - See ../../gpio/gpio.txt for more information. > > -- #pwm-cells : Should be one. See ../../pwm/pwm.yaml for description of > > - the cell formats. > > - > > -- clock-names: should be "refclk" > > -- clocks: Specification for input reference clock. The reference > > - clock rate must be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz. > > - > > -- data-lanes: See ../../media/video-interface.txt > > -- lane-polarities: See ../../media/video-interface.txt > > - > > -- suspend-gpios: specification for GPIO1 pin on bridge (active low) > > Where has suspend-gpios gone ? :-) Oops. Added it back. > > - > > -Required nodes: > > -This device has two video ports. Their connections are modelled using the > > -OF graph bindings specified in Documentation/devicetree/bindings/graph.txt. > > - > > -- Video port 0 for DSI input > > -- Video port 1 for eDP output > > - > > -Example > > -------- > > - > > -edp-bridge@2d { > > - compatible = "ti,sn65dsi86"; > > - #address-cells = <1>; > > - #size-cells = <0>; > > - reg = <0x2d>; > > - > > - enable-gpios = <&msmgpio 33 GPIO_ACTIVE_HIGH>; > > - suspend-gpios = <&msmgpio 34 GPIO_ACTIVE_LOW>; > > - > > - interrupts-extended = <&gpio3 4 IRQ_TYPE_EDGE_FALLING>; > > - > > - vccio-supply = <&pm8916_l17>; > > - vcca-supply = <&pm8916_l6>; > > - vpll-supply = <&pm8916_l17>; > > - vcc-supply = <&pm8916_l6>; > > - > > - clock-names = "refclk"; > > - clocks = <&input_refclk>; > > - > > - ports { > > - #address-cells = <1>; > > - #size-cells = <0>; > > - > > - port@0 { > > - reg = <0>; > > - > > - edp_bridge_in: endpoint { > > - remote-endpoint = <&dsi_out>; > > - }; > > - }; > > - > > - port@1 { > > - reg = <1>; > > - > > - edp_bridge_out: endpoint { > > - data-lanes = <2 1 3 0>; > > - lane-polarities = <0 1 0 1>; > > - remote-endpoint = <&edp_panel_in>; > > - }; > > - }; > > - }; > > -} > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > new file mode 100644 > > index 000000000000..8cacc6db33a9 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > @@ -0,0 +1,188 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/display/bridge/ti,sn65dsi86.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: SN65DSI86 DSI to eDP bridge chip > > + > > +maintainers: > > + - Sandeep Panda <spanda@codeaurora.org> > > + > > +description: | > > + The Texas Instruments SN65DSI86 bridge takes MIPI DSI in and outputs eDP. > > + http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf > > + > > +properties: > > + compatible: > > + const: ti,sn65dsi86 > > + > > + reg: > > + const: 0x2d > > + > > + enable-gpios: > > + maxItems: 1 > > + description: GPIO specification for bridge_en pin (active high). > > + > > + vccio-supply: > > + description: A 1.8V supply that powers up the digital IOs. > > + > > + vpll-supply: > > + description: A 1.8V supply that powers up the DisplayPort PLL. > > + > > + vcca-supply: > > + description: A 1.2V supply that powers up the analog circuits. > > + > > + vcc-supply: > > + description: A 1.2V supply that powers up the digital core. > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + description: > > + Specification for input reference clock. The reference clock rate must > > + be 12 MHz, 19.2 MHz, 26 MHz, 27 MHz or 38.4 MHz. > > + > > + clock-names: > > + const: refclk > > + > > + gpio-controller: true > > + '#gpio-cells': > > + const: 2 > > + description: > > + First cell is pin number, second cell is flags. GPIO pin numbers are > > + 1-based to match the datasheet. See ../../gpio/gpio.txt for more > > + information. > > + > > + '#pwm-cells': > > + const: 1 > > + description: See ../../pwm/pwm.yaml for description of the cell formats. > > + > > + data-lanes: > > Should this have > > minItems: 1 > maxItems: 4 > items: > enum: > - 0 > - 1 > - 2 > - 3 Interestingly this seemed to be at totally the wrong location in the old ".txt" file and in my v1. I moved it to the right place now by making sure I put the old example back in, not just the example of what's currently in the tree. > > + description: See ../../media/video-interface.txt > > + > > + lane-polarities: > > + description: See ../../media/video-interface.txt > > And something similar here, > > minItems: 1 > maxItems: 4 > items: > enum: > - 0 > - 1 > uniqueItems: false > > I'm not entirely sure where uniqueItems should be placed. I left this out of v2. "uniqueItems: false" appears to be the default so it would only be needed if you were trying to override someone who had already made this "true". I tested this by setting it to true and seeing the error, then removing the set to true and seeing the error gone. > I'm also not > sure how to specify that both data-lanes and lane-polarities need to > have the same number of items, maybe > > dependencies: > data-lanes: [lane-polarities] The opposite of that is interesting, that is: dependencies: data-lanes: [lane-polarities] ...that seems to say that specifying "lane-polarities" without "data-lanes" is an error. ...but that doesn't specify that data-lanes and lane-polarities need to have the same number of items. If someone wants to provide the syntax I'm happy to add it, otherwise it feels like it could be something to improve in the future. In general I haven't seen people get to this level of detail in yaml. > > + > > + ports: > > + type: object > > + > > + properties: > > + "#address-cells": > > + const: 1 > > + > > + "#size-cells": > > + const: 0 > > + > > + port@0: > > + type: object > > + additionalProperties: false > > + > > + description: > > + Video port for MIPI DSI input > > + > > + properties: > > + reg: > > + const: 0 > > + > > + patternProperties: > > + endpoint: > > If there's a single endpoint, you don't need patternProperties, it can > be specified in properties. Done. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-04-21 5:10 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-15 15:48 [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml Douglas Anderson 2020-04-15 15:48 ` [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings Douglas Anderson 2020-04-15 19:53 ` Stephen Boyd 2020-04-15 20:32 ` Laurent Pinchart 2020-04-15 23:49 ` Doug Anderson 2020-04-16 0:54 ` Laurent Pinchart 2020-04-16 21:53 ` Doug Anderson 2020-04-17 18:08 ` Laurent Pinchart 2020-04-21 5:10 ` Doug Anderson 2020-04-15 15:48 ` [PATCH 3/3] drm/bridge: ti-sn65dsi86: Allow one of the bridge GPIOs to be HPD Douglas Anderson 2020-04-15 19:50 ` [PATCH 1/3] dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml Stephen Boyd 2020-04-15 20:31 ` Laurent Pinchart 2020-04-21 5:07 ` Doug Anderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).