| Message ID | 20211029152647.v3.2.If23c83a786fc4d318a1986f43803f22b4b1d82cd@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3,1/4] arm64: dts: sc7180: Include gpio.h in edp bridge dts | expand |
Quoting Philip Chen (2021-10-29 15:27:41) > MSM DSI host driver actually parses "data-lanes" in DT and compare > it with the number of DSI lanes the bridge driver sets for > mipi_dsi_device. So we need to always specify "data-lanes" for the > DSI host output. As of now, "data-lanes" is added to ti-sn65dsi86 dts > fragment, but missing in parade-ps8640 dts fragment, which requires > a fixup. I don't see data-lanes required in the schema, and dsi_host_parse_lane_data() seems happy to continue without it. I do see that num_data_lanes isn't set though. Does this patch fix it? ----8<---- diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index e269df285136..f6fba07220e5 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -1696,6 +1696,7 @@ static int dsi_host_parse_lane_data(struct msm_dsi_host *msm_host, if (!prop) { DRM_DEV_DEBUG(dev, "failed to find data lane mapping, using default\n"); + msm_host->num_data_lanes = 4; return 0; }
Hi Stephen, On Fri, Oct 29, 2021 at 4:16 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Philip Chen (2021-10-29 15:27:41) > > MSM DSI host driver actually parses "data-lanes" in DT and compare > > it with the number of DSI lanes the bridge driver sets for > > mipi_dsi_device. So we need to always specify "data-lanes" for the > > DSI host output. As of now, "data-lanes" is added to ti-sn65dsi86 dts > > fragment, but missing in parade-ps8640 dts fragment, which requires > > a fixup. > > I don't see data-lanes required in the schema, and > dsi_host_parse_lane_data() seems happy to continue without it. I do see > that num_data_lanes isn't set though. Does this patch fix it? The problem I see is from dsi_host_attach(). If there is no "data-lanes" in DT, num_data_lanes would be 0. Then dsi_host_attach() would return -EINVAL. > > ----8<---- > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c > b/drivers/gpu/drm/msm/dsi/dsi_host.c > index e269df285136..f6fba07220e5 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -1696,6 +1696,7 @@ static int dsi_host_parse_lane_data(struct > msm_dsi_host *msm_host, > if (!prop) { > DRM_DEV_DEBUG(dev, > "failed to find data lane mapping, using default\n"); > + msm_host->num_data_lanes = 4; > return 0; > } I haven't tried. But I think it can fix the problem I described above. Would you like to send it as a separate patch? Or I can do it.
Quoting Philip Chen (2021-10-29 16:24:58) > Hi Stephen, > > On Fri, Oct 29, 2021 at 4:16 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Philip Chen (2021-10-29 15:27:41) > > > MSM DSI host driver actually parses "data-lanes" in DT and compare > > > it with the number of DSI lanes the bridge driver sets for > > > mipi_dsi_device. So we need to always specify "data-lanes" for the > > > DSI host output. As of now, "data-lanes" is added to ti-sn65dsi86 dts > > > fragment, but missing in parade-ps8640 dts fragment, which requires > > > a fixup. > > > > I don't see data-lanes required in the schema, and > > dsi_host_parse_lane_data() seems happy to continue without it. I do see > > that num_data_lanes isn't set though. Does this patch fix it? > The problem I see is from dsi_host_attach(). > If there is no "data-lanes" in DT, num_data_lanes would be 0. > Then dsi_host_attach() would return -EINVAL. Ok, got it. > > > > > ----8<---- > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c > > b/drivers/gpu/drm/msm/dsi/dsi_host.c > > index e269df285136..f6fba07220e5 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > > @@ -1696,6 +1696,7 @@ static int dsi_host_parse_lane_data(struct > > msm_dsi_host *msm_host, > > if (!prop) { > > DRM_DEV_DEBUG(dev, > > "failed to find data lane mapping, using default\n"); > > + msm_host->num_data_lanes = 4; > > return 0; > > } > > I haven't tried. > But I think it can fix the problem I described above. > Would you like to send it as a separate patch? > Or I can do it. Sure feel free to send it as another patch. Or fix the schema to make data-lanes required. I think fixing the driver is probably better so that we don't have to set data-lanes when it's the default 4 lanes. At least I think 4 lanes is the default.
Hi, On Sun, Oct 31, 2021 at 7:52 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > On 10/31/2021 2:01 PM, Abhinav Kumar wrote: > > Hi Philip > > On 10/29/2021 4:24 PM, Philip Chen wrote: > > Hi Stephen, > > On Fri, Oct 29, 2021 at 4:16 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Philip Chen (2021-10-29 15:27:41) > > MSM DSI host driver actually parses "data-lanes" in DT and compare > it with the number of DSI lanes the bridge driver sets for > mipi_dsi_device. So we need to always specify "data-lanes" for the > DSI host output. As of now, "data-lanes" is added to ti-sn65dsi86 dts > fragment, but missing in parade-ps8640 dts fragment, which requires > a fixup. > > I don't see data-lanes required in the schema, and > dsi_host_parse_lane_data() seems happy to continue without it. I do see > that num_data_lanes isn't set though. Does this patch fix it? > > The problem I see is from dsi_host_attach(). > If there is no "data-lanes" in DT, num_data_lanes would be 0. > Then dsi_host_attach() would return -EINVAL. > > Let me know if i am missing something here. the dsi_host_attach returns an error if > > dsi->lanes is > than msm_host->num_data_lanes. That shouldnt happen even if > > num_data_lanes will be 0 > > static int dsi_host_attach(struct mipi_dsi_host *host, > struct mipi_dsi_device *dsi) > { > struct msm_dsi_host *msm_host = to_msm_dsi_host(host); > int ret; > > if (dsi->lanes > msm_host->num_data_lanes) > return -EINVAL; > > Thanks > > Abhinav > > Please ignore this comment, I understood now. The fix you had posted to default to 4 lanes seems fine. > > Will ack that one. Thanks! For the other folks: the fix I posted for dsi_host is here: https://patchwork.kernel.org/project/linux-arm-msm/patch/20211030100812.1.I6cd9af36b723fed277d34539d3b2ba4ca233ad2d@changeid/ > > > ----8<---- > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c > b/drivers/gpu/drm/msm/dsi/dsi_host.c > index e269df285136..f6fba07220e5 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -1696,6 +1696,7 @@ static int dsi_host_parse_lane_data(struct > msm_dsi_host *msm_host, > if (!prop) { > DRM_DEV_DEBUG(dev, > "failed to find data lane mapping, using default\n"); > + msm_host->num_data_lanes = 4; > return 0; > } > > I haven't tried. > But I think it can fix the problem I described above. > Would you like to send it as a separate patch? > Or I can do it.
Hi, On Fri, Oct 29, 2021 at 3:27 PM Philip Chen <philipchen@chromium.org> wrote: > > MSM DSI host driver actually parses "data-lanes" in DT and compare > it with the number of DSI lanes the bridge driver sets for > mipi_dsi_device. So we need to always specify "data-lanes" for the > DSI host output. As of now, "data-lanes" is added to ti-sn65dsi86 dts > fragment, but missing in parade-ps8640 dts fragment, which requires > a fixup. > > Since we'll do 4-lane DSI regardless of which bridge chip is used, > instead of adding "data-lanes" to parade-ps8640 dts fragment, let's > just move "data-lanes" from the bridge dts to sc7180-trogdor.dtsi. > > Signed-off-by: Philip Chen <philipchen@chromium.org> > --- > > (no changes since v1) > > arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi | 1 - > arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 4 ++++ > 2 files changed, 4 insertions(+), 1 deletion(-) I think it's fine to take this change even if we also decide to fix the driver as well. Reviewed-by: Douglas Anderson <dianders@chromium.org>
Quoting Philip Chen (2021-10-29 15:27:41) > MSM DSI host driver actually parses "data-lanes" in DT and compare > it with the number of DSI lanes the bridge driver sets for > mipi_dsi_device. So we need to always specify "data-lanes" for the > DSI host output. As of now, "data-lanes" is added to ti-sn65dsi86 dts > fragment, but missing in parade-ps8640 dts fragment, which requires > a fixup. > > Since we'll do 4-lane DSI regardless of which bridge chip is used, > instead of adding "data-lanes" to parade-ps8640 dts fragment, let's > just move "data-lanes" from the bridge dts to sc7180-trogdor.dtsi. > > Signed-off-by: Philip Chen <philipchen@chromium.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi index 6dbf413e4e5b..f869e6a343c1 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi @@ -9,7 +9,6 @@ &dsi0_out { remote-endpoint = <&sn65dsi86_in>; - data-lanes = <0 1 2 3>; }; edp_brij_i2c: &i2c2 { diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi index d4f4441179fc..bd5909ffb3dc 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi @@ -603,6 +603,10 @@ &dsi0 { vdda-supply = <&vdda_mipi_dsi0_1p2>; }; +&dsi0_out { + data-lanes = <0 1 2 3>; +}; + &dsi_phy { status = "okay"; vdds-supply = <&vdda_mipi_dsi0_pll>;
MSM DSI host driver actually parses "data-lanes" in DT and compare it with the number of DSI lanes the bridge driver sets for mipi_dsi_device. So we need to always specify "data-lanes" for the DSI host output. As of now, "data-lanes" is added to ti-sn65dsi86 dts fragment, but missing in parade-ps8640 dts fragment, which requires a fixup. Since we'll do 4-lane DSI regardless of which bridge chip is used, instead of adding "data-lanes" to parade-ps8640 dts fragment, let's just move "data-lanes" from the bridge dts to sc7180-trogdor.dtsi. Signed-off-by: Philip Chen <philipchen@chromium.org> --- (no changes since v1) arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi | 1 - arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-)