| Message ID | 20250327-wip-obbardc-qcom-t14s-oled-panel-v3-1-45d5f2747398@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for OLED panel used on Snapdragon Lenovo T14s Gen6 | expand |
On Thu, Mar 27, 2025 at 04:56:53PM +0000, Christopher Obbard wrote: > The eDP panel has an HPD GPIO. Describe it in the device tree > for the generic T14s model, as the HPD GPIO property is used in > both the OLED and LCD models which inherit this device tree. > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Christopher Obbard <christopher.obbard@linaro.org> > --- > arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
On Thu, Mar 27, 2025 at 04:56:53PM +0000, Christopher Obbard wrote: > The eDP panel has an HPD GPIO. Describe it in the device tree > for the generic T14s model, as the HPD GPIO property is used in > both the OLED and LCD models which inherit this device tree. AFAICT, this patch is not correct as the hotplug detect signal is connected directly to the display controller on (these) Qualcomm SoCs and is already handled by its driver. Describing it as you do here leads to less accurate delays, see commits: 2327b13d6c47 ("drm/panel-edp: Take advantage of wait_hpd_asserted() in struct drm_dp_aux"). 3b5765df375c ("drm/panel: atna33xc20: Take advantage of wait_hpd_asserted() in struct drm_dp_aux") Perhaps you lose some other functionality too. > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Christopher Obbard <christopher.obbard@linaro.org> > --- > arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > index 962fb050c55c4fd33f480a21a8c47a484d0c82b8..46c73f5c039ed982b553636cf8c4237a20ba7687 100644 > --- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > +++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > @@ -980,8 +980,12 @@ &mdss_dp3 { > aux-bus { > panel: panel { > compatible = "edp-panel"; > + hpd-gpios = <&tlmm 119 GPIO_ACTIVE_HIGH>; > power-supply = <&vreg_edp_3p3>; > > + pinctrl-0 = <&edp_hpd_n_default>; > + pinctrl-names = "default"; > + > port { > edp_panel_in: endpoint { > remote-endpoint = <&mdss_dp3_out>; > @@ -1286,6 +1290,13 @@ hall_int_n_default: hall-int-n-state { > bias-disable; > }; > > + edp_hpd_n_default: edp-hpd-n-state { > + pins = "gpio119"; > + function = "gpio"; > + drive-strength = <2>; > + bias-pull-up; > + }; I checked the firmware configuration for this pin on my T14s, which does not match what you have here. Instead the function is set to "edp0_hot" which forwards the signal to the display controller which already handles the signal on panel power on. (And there is also no internal pull up enabled). We may want to describe this pin configuration somewhere, but that's a separate issue. Johan
Hi Johan, On Mon, 31 Mar 2025 at 09:50, Johan Hovold <johan@kernel.org> wrote: > > On Thu, Mar 27, 2025 at 04:56:53PM +0000, Christopher Obbard wrote: > > The eDP panel has an HPD GPIO. Describe it in the device tree > > for the generic T14s model, as the HPD GPIO property is used in > > both the OLED and LCD models which inherit this device tree. > > AFAICT, this patch is not correct as the hotplug detect signal is > connected directly to the display controller on (these) Qualcomm SoCs > and is already handled by its driver. > > Describing it as you do here leads to less accurate delays, see commits: > > 2327b13d6c47 ("drm/panel-edp: Take advantage of wait_hpd_asserted() in struct drm_dp_aux"). > 3b5765df375c ("drm/panel: atna33xc20: Take advantage of wait_hpd_asserted() in struct drm_dp_aux") > > Perhaps you lose some other functionality too. > > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > Signed-off-by: Christopher Obbard <christopher.obbard@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > > index 962fb050c55c4fd33f480a21a8c47a484d0c82b8..46c73f5c039ed982b553636cf8c4237a20ba7687 100644 > > --- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > > +++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > > @@ -980,8 +980,12 @@ &mdss_dp3 { > > aux-bus { > > panel: panel { > > compatible = "edp-panel"; > > + hpd-gpios = <&tlmm 119 GPIO_ACTIVE_HIGH>; > > power-supply = <&vreg_edp_3p3>; > > > > + pinctrl-0 = <&edp_hpd_n_default>; > > + pinctrl-names = "default"; > > + > > port { > > edp_panel_in: endpoint { > > remote-endpoint = <&mdss_dp3_out>; > > @@ -1286,6 +1290,13 @@ hall_int_n_default: hall-int-n-state { > > bias-disable; > > }; > > > > + edp_hpd_n_default: edp-hpd-n-state { > > + pins = "gpio119"; > > + function = "gpio"; > > + drive-strength = <2>; > > + bias-pull-up; > > + }; > > I checked the firmware configuration for this pin on my T14s, which > does not match what you have here. Instead the function is set to > "edp0_hot" which forwards the signal to the display controller which > already handles the signal on panel power on. (And there is also no > internal pull up enabled). > > We may want to describe this pin configuration somewhere, but that's a > separate issue. Thanks for your review, I will send another version in coming days and drop this first patch (adding hpd to the T14s DTSI). As a consequence I will need to add no-hpd property to the panel node. I will add a short comment about how the hpd signal is handled by the driver already. Thanks! Chris
On 31/03/2025 18:39, Christopher Obbard wrote: > Hi Johan, > > On Mon, 31 Mar 2025 at 09:50, Johan Hovold <johan@kernel.org> wrote: >> >> On Thu, Mar 27, 2025 at 04:56:53PM +0000, Christopher Obbard wrote: >>> The eDP panel has an HPD GPIO. Describe it in the device tree >>> for the generic T14s model, as the HPD GPIO property is used in >>> both the OLED and LCD models which inherit this device tree. >> >> AFAICT, this patch is not correct as the hotplug detect signal is >> connected directly to the display controller on (these) Qualcomm SoCs >> and is already handled by its driver. >> >> Describing it as you do here leads to less accurate delays, see commits: >> >> 2327b13d6c47 ("drm/panel-edp: Take advantage of wait_hpd_asserted() in struct drm_dp_aux"). >> 3b5765df375c ("drm/panel: atna33xc20: Take advantage of wait_hpd_asserted() in struct drm_dp_aux") >> >> Perhaps you lose some other functionality too. >> >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>> Signed-off-by: Christopher Obbard <christopher.obbard@linaro.org> >>> --- >>> arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi >>> index 962fb050c55c4fd33f480a21a8c47a484d0c82b8..46c73f5c039ed982b553636cf8c4237a20ba7687 100644 >>> --- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi >>> @@ -980,8 +980,12 @@ &mdss_dp3 { >>> aux-bus { >>> panel: panel { >>> compatible = "edp-panel"; >>> + hpd-gpios = <&tlmm 119 GPIO_ACTIVE_HIGH>; >>> power-supply = <&vreg_edp_3p3>; >>> >>> + pinctrl-0 = <&edp_hpd_n_default>; >>> + pinctrl-names = "default"; >>> + >>> port { >>> edp_panel_in: endpoint { >>> remote-endpoint = <&mdss_dp3_out>; >>> @@ -1286,6 +1290,13 @@ hall_int_n_default: hall-int-n-state { >>> bias-disable; >>> }; >>> >>> + edp_hpd_n_default: edp-hpd-n-state { >>> + pins = "gpio119"; >>> + function = "gpio"; >>> + drive-strength = <2>; >>> + bias-pull-up; >>> + }; >> >> I checked the firmware configuration for this pin on my T14s, which >> does not match what you have here. Instead the function is set to >> "edp0_hot" which forwards the signal to the display controller which >> already handles the signal on panel power on. (And there is also no >> internal pull up enabled). >> >> We may want to describe this pin configuration somewhere, but that's a >> separate issue. > > Thanks for your review, I will send another version in coming days and > drop this first patch (adding hpd to the T14s DTSI). > > As a consequence I will need to add no-hpd property to the panel node. No, you won't. There is a HPD line and it is routed to the DP controller. > I will add a short comment about how the hpd signal is handled by the > driver already.
Hi Dmitry, On Mon, 31 Mar 2025 at 17:49, Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote: > > On 31/03/2025 18:39, Christopher Obbard wrote: > > Hi Johan, > > > > On Mon, 31 Mar 2025 at 09:50, Johan Hovold <johan@kernel.org> wrote: > >> > >> On Thu, Mar 27, 2025 at 04:56:53PM +0000, Christopher Obbard wrote: > >>> The eDP panel has an HPD GPIO. Describe it in the device tree > >>> for the generic T14s model, as the HPD GPIO property is used in > >>> both the OLED and LCD models which inherit this device tree. > >> > >> AFAICT, this patch is not correct as the hotplug detect signal is > >> connected directly to the display controller on (these) Qualcomm SoCs > >> and is already handled by its driver. > >> > >> Describing it as you do here leads to less accurate delays, see commits: > >> > >> 2327b13d6c47 ("drm/panel-edp: Take advantage of wait_hpd_asserted() in struct drm_dp_aux"). > >> 3b5765df375c ("drm/panel: atna33xc20: Take advantage of wait_hpd_asserted() in struct drm_dp_aux") > >> > >> Perhaps you lose some other functionality too. > >> > >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > >>> Signed-off-by: Christopher Obbard <christopher.obbard@linaro.org> > >>> --- > >>> arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi | 11 +++++++++++ > >>> 1 file changed, 11 insertions(+) > >>> > >>> diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > >>> index 962fb050c55c4fd33f480a21a8c47a484d0c82b8..46c73f5c039ed982b553636cf8c4237a20ba7687 100644 > >>> --- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > >>> +++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > >>> @@ -980,8 +980,12 @@ &mdss_dp3 { > >>> aux-bus { > >>> panel: panel { > >>> compatible = "edp-panel"; > >>> + hpd-gpios = <&tlmm 119 GPIO_ACTIVE_HIGH>; > >>> power-supply = <&vreg_edp_3p3>; > >>> > >>> + pinctrl-0 = <&edp_hpd_n_default>; > >>> + pinctrl-names = "default"; > >>> + > >>> port { > >>> edp_panel_in: endpoint { > >>> remote-endpoint = <&mdss_dp3_out>; > >>> @@ -1286,6 +1290,13 @@ hall_int_n_default: hall-int-n-state { > >>> bias-disable; > >>> }; > >>> > >>> + edp_hpd_n_default: edp-hpd-n-state { > >>> + pins = "gpio119"; > >>> + function = "gpio"; > >>> + drive-strength = <2>; > >>> + bias-pull-up; > >>> + }; > >> > >> I checked the firmware configuration for this pin on my T14s, which > >> does not match what you have here. Instead the function is set to > >> "edp0_hot" which forwards the signal to the display controller which > >> already handles the signal on panel power on. (And there is also no > >> internal pull up enabled). > >> > >> We may want to describe this pin configuration somewhere, but that's a > >> separate issue. > > > > Thanks for your review, I will send another version in coming days and > > drop this first patch (adding hpd to the T14s DTSI). > > > > As a consequence I will need to add no-hpd property to the panel node. > No, you won't. There is a HPD line and it is routed to the DP controller. OK, I think I misunderstand what Johan said. After taking some time to think about it in more detail: - The first commit will be changed so that the hpd GPIO will be added to the X1E DP controller instead of the panel. grepping the source for dp_hot_plug_det shows me how to do that. This part is clear. - The panel node in the generic T14s DTSI should not have the hpd-gpios property / pinctrl set. - The panel node should not have the hpd-gpios property / pinctrl set. I hope I understand that correctly. I will send a new series in the morning unless there is any objection. Thanks Chris
On Tue, 1 Apr 2025 at 00:38, Christopher Obbard <christopher.obbard@linaro.org> wrote: > > Hi Dmitry, > > On Mon, 31 Mar 2025 at 17:49, Dmitry Baryshkov > <dmitry.baryshkov@oss.qualcomm.com> wrote: > > > > On 31/03/2025 18:39, Christopher Obbard wrote: > > > Hi Johan, > > > > > > On Mon, 31 Mar 2025 at 09:50, Johan Hovold <johan@kernel.org> wrote: > > >> > > >> On Thu, Mar 27, 2025 at 04:56:53PM +0000, Christopher Obbard wrote: > > >>> The eDP panel has an HPD GPIO. Describe it in the device tree > > >>> for the generic T14s model, as the HPD GPIO property is used in > > >>> both the OLED and LCD models which inherit this device tree. > > >> > > >> AFAICT, this patch is not correct as the hotplug detect signal is > > >> connected directly to the display controller on (these) Qualcomm SoCs > > >> and is already handled by its driver. > > >> > > >> Describing it as you do here leads to less accurate delays, see commits: > > >> > > >> 2327b13d6c47 ("drm/panel-edp: Take advantage of wait_hpd_asserted() in struct drm_dp_aux"). > > >> 3b5765df375c ("drm/panel: atna33xc20: Take advantage of wait_hpd_asserted() in struct drm_dp_aux") > > >> > > >> Perhaps you lose some other functionality too. > > >> > > >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > >>> Signed-off-by: Christopher Obbard <christopher.obbard@linaro.org> > > >>> --- > > >>> arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi | 11 +++++++++++ > > >>> 1 file changed, 11 insertions(+) > > >>> > > >>> diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > > >>> index 962fb050c55c4fd33f480a21a8c47a484d0c82b8..46c73f5c039ed982b553636cf8c4237a20ba7687 100644 > > >>> --- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > > >>> +++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi > > >>> @@ -980,8 +980,12 @@ &mdss_dp3 { > > >>> aux-bus { > > >>> panel: panel { > > >>> compatible = "edp-panel"; > > >>> + hpd-gpios = <&tlmm 119 GPIO_ACTIVE_HIGH>; > > >>> power-supply = <&vreg_edp_3p3>; > > >>> > > >>> + pinctrl-0 = <&edp_hpd_n_default>; > > >>> + pinctrl-names = "default"; > > >>> + > > >>> port { > > >>> edp_panel_in: endpoint { > > >>> remote-endpoint = <&mdss_dp3_out>; > > >>> @@ -1286,6 +1290,13 @@ hall_int_n_default: hall-int-n-state { > > >>> bias-disable; > > >>> }; > > >>> > > >>> + edp_hpd_n_default: edp-hpd-n-state { > > >>> + pins = "gpio119"; > > >>> + function = "gpio"; > > >>> + drive-strength = <2>; > > >>> + bias-pull-up; > > >>> + }; > > >> > > >> I checked the firmware configuration for this pin on my T14s, which > > >> does not match what you have here. Instead the function is set to > > >> "edp0_hot" which forwards the signal to the display controller which > > >> already handles the signal on panel power on. (And there is also no > > >> internal pull up enabled). > > >> > > >> We may want to describe this pin configuration somewhere, but that's a > > >> separate issue. > > > > > > Thanks for your review, I will send another version in coming days and > > > drop this first patch (adding hpd to the T14s DTSI). > > > > > > As a consequence I will need to add no-hpd property to the panel node. > > No, you won't. There is a HPD line and it is routed to the DP controller. > > OK, I think I misunderstand what Johan said. After taking some time to > think about it in more detail: > - The first commit will be changed so that the hpd GPIO will be added > to the X1E DP controller instead of the panel. grepping the source for > dp_hot_plug_det shows me how to do that. This part is clear. > > - The panel node in the generic T14s DTSI should not have the > hpd-gpios property / pinctrl set. > > - The panel node should not have the hpd-gpios property / pinctrl set. LGTM. Use sc8180x-primus as an example > > I hope I understand that correctly. I will send a new series in the > morning unless there is any objection.
diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi index 962fb050c55c4fd33f480a21a8c47a484d0c82b8..46c73f5c039ed982b553636cf8c4237a20ba7687 100644 --- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi +++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi @@ -980,8 +980,12 @@ &mdss_dp3 { aux-bus { panel: panel { compatible = "edp-panel"; + hpd-gpios = <&tlmm 119 GPIO_ACTIVE_HIGH>; power-supply = <&vreg_edp_3p3>; + pinctrl-0 = <&edp_hpd_n_default>; + pinctrl-names = "default"; + port { edp_panel_in: endpoint { remote-endpoint = <&mdss_dp3_out>; @@ -1286,6 +1290,13 @@ hall_int_n_default: hall-int-n-state { bias-disable; }; + edp_hpd_n_default: edp-hpd-n-state { + pins = "gpio119"; + function = "gpio"; + drive-strength = <2>; + bias-pull-up; + }; + pcie4_default: pcie4-default-state { clkreq-n-pins { pins = "gpio147";