| Message ID | 20250327-wip-obbardc-qcom-t14s-oled-panel-v3-1-45d5f2747398@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for OLED panel used on Snapdragon Lenovo T14s Gen6 | expand |
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";