[PATCH 0/6] drm/msm: Misc patches

[PATCH 0/6] drm/msm: Misc patches

[Archit Taneja]

These are mainly small fixes in drm/msm. The last patch adds a way to
parse DSI lanes information via DT.

Archit Taneja (5):
  drm/msm/mdp: Use atomic helper to set crtc property
  drm/msm: Free fb helper resources in msm_unload
  drm/msm/dsi: Remove incorrect warning on host attach
  drm/msm/dsi: Update the "vdd" voltage range
  drm/msm/dsi: Parse DSI lanes via DT

Sricharan R (1):
  drm/msm/mdp: Detach iommu in mdp4_destroy

 .../devicetree/bindings/display/msm/dsi.txt        |  26 +++-
 drivers/gpu/drm/msm/dsi/dsi_cfg.c                  |   2 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c                 | 148 ++++++++++++++++++---
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c           |   9 +-
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c            |  17 ++-
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h            |   1 +
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c           |   9 +-
 drivers/gpu/drm/msm/msm_drv.c                      |   5 +
 drivers/gpu/drm/msm/msm_drv.h                      |   1 +
 9 files changed, 172 insertions(+), 46 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 1/6] drm/msm/mdp: Use atomic helper to set crtc property

[Archit Taneja]

Assign drm_atomic_helper_crtc_set_property helper to MDP4 and MDP5
crtcs' set_property ops. This replaces the custom funcs that
returned an error even for standard crtc properties.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 9 +--------
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 9 +--------
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
index 28df397..4b95d6c 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
@@ -361,13 +361,6 @@ static void mdp4_crtc_atomic_flush(struct drm_crtc *crtc,
 	request_pending(crtc, PENDING_FLIP);
 }
 
-static int mdp4_crtc_set_property(struct drm_crtc *crtc,
-		struct drm_property *property, uint64_t val)
-{
-	// XXX
-	return -EINVAL;
-}
-
 #define CURSOR_WIDTH 64
 #define CURSOR_HEIGHT 64
 
@@ -499,7 +492,7 @@ static const struct drm_crtc_funcs mdp4_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = mdp4_crtc_destroy,
 	.page_flip = drm_atomic_helper_page_flip,
-	.set_property = mdp4_crtc_set_property,
+	.set_property = drm_atomic_helper_crtc_set_property,
 	.cursor_set = mdp4_crtc_cursor_set,
 	.cursor_move = mdp4_crtc_cursor_move,
 	.reset = drm_atomic_helper_crtc_reset,
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
index 20cee5c..9a15205 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
@@ -468,13 +468,6 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc,
 	request_pending(crtc, PENDING_FLIP);
 }
 
-static int mdp5_crtc_set_property(struct drm_crtc *crtc,
-		struct drm_property *property, uint64_t val)
-{
-	// XXX
-	return -EINVAL;
-}
-
 static void get_roi(struct drm_crtc *crtc, uint32_t *roi_w, uint32_t *roi_h)
 {
 	struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc);
@@ -625,7 +618,7 @@ static const struct drm_crtc_funcs mdp5_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = mdp5_crtc_destroy,
 	.page_flip = drm_atomic_helper_page_flip,
-	.set_property = mdp5_crtc_set_property,
+	.set_property = drm_atomic_helper_crtc_set_property,
 	.reset = drm_atomic_helper_crtc_reset,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/6] drm/msm/mdp: Detach iommu in mdp4_destroy

[Archit Taneja]

From: Sricharan R <sricharan@codeaurora.org>

attach_dev gets called in mdp4_kms_init, but there is no corresponding
detach_dev called in the error path or in the kms driver unload path.

Detach and destroy mmu in mdp4_destroy.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 17 +++++++++++++----
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h |  1 +
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
index 5a8e3d6..760535b 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
@@ -189,9 +189,20 @@ static void mdp4_preclose(struct msm_kms *kms, struct drm_file *file)
 		mdp4_crtc_cancel_pending_flip(priv->crtcs[i], file);
 }
 
+static const char * const iommu_ports[] = {
+	"mdp_port0_cb0", "mdp_port1_cb0",
+};
+
 static void mdp4_destroy(struct msm_kms *kms)
 {
 	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
+	struct msm_mmu *mmu = mdp4_kms->mmu;
+
+	if (mmu) {
+		mmu->funcs->detach(mmu, iommu_ports, ARRAY_SIZE(iommu_ports));
+		mmu->funcs->destroy(mmu);
+	}
+
 	if (mdp4_kms->blank_cursor_iova)
 		msm_gem_put_iova(mdp4_kms->blank_cursor_bo, mdp4_kms->id);
 	if (mdp4_kms->blank_cursor_bo)
@@ -457,10 +468,6 @@ fail:
 	return ret;
 }
 
-static const char *iommu_ports[] = {
-		"mdp_port0_cb0", "mdp_port1_cb0",
-};
-
 struct msm_kms *mdp4_kms_init(struct drm_device *dev)
 {
 	struct platform_device *pdev = dev->platformdev;
@@ -565,6 +572,8 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
 				ARRAY_SIZE(iommu_ports));
 		if (ret)
 			goto fail;
+
+		mdp4_kms->mmu = mmu;
 	} else {
 		dev_info(dev->dev, "no iommu, fallback to phys "
 				"contig buffers for scanout\n");
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h
index d2c96ef..9dfc9d4 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h
@@ -45,6 +45,7 @@ struct mdp4_kms {
 	struct clk *pclk;
 	struct clk *lut_clk;
 	struct clk *axi_clk;
+	struct msm_mmu *mmu;
 
 	struct mdp_irq error_handler;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/6] drm/msm: Free fb helper resources in msm_unload

[Archit Taneja]

We have a msm_fbev_free function to uninit fb_helper stuff, but we aren't
using it. Call it in msm_unload.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 5 +++++
 drivers/gpu/drm/msm/msm_drv.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 9a30807..c1053d6 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -196,6 +196,11 @@ static int msm_unload(struct drm_device *dev)
 	}
 
 	drm_kms_helper_poll_fini(dev);
+
+#ifdef CONFIG_DRM_FBDEV_EMULATION
+	if (fbdev && priv->fbdev)
+		msm_fbdev_free(dev);
+#endif
 	drm_mode_config_cleanup(dev);
 	drm_vblank_cleanup(dev);
 
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index c1e7bba..1aff485 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -240,6 +240,7 @@ struct drm_framebuffer *msm_framebuffer_create(struct drm_device *dev,
 		struct drm_file *file, const struct drm_mode_fb_cmd2 *mode_cmd);
 
 struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev);
+void msm_fbdev_free(struct drm_device *dev);
 
 struct hdmi;
 int hdmi_modeset_init(struct hdmi *hdmi, struct drm_device *dev,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm/msm/dsi: Remove incorrect warning on host attach

[Archit Taneja]

With the implementation of of_graph parsing, it isn't any longer
necessary for msm_host->device node to be same as dsi->dev.of_node. This
only holds true when the connected device is also a child of the dsi_host.

In the case of external bridge chips belonging to a different control
bus, these are guaranteed to be different.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 48f9967..69bac59 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1484,8 +1484,6 @@ static int dsi_host_attach(struct mipi_dsi_host *host,
 	msm_host->format = dsi->format;
 	msm_host->mode_flags = dsi->mode_flags;
 
-	WARN_ON(dsi->dev.of_node != msm_host->device_node);
-
 	/* Some gpios defined in panel DT need to be controlled by host */
 	ret = dsi_host_init_panel_gpios(msm_host, &dsi->dev);
 	if (ret)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 5/6] drm/msm/dsi: Update the "vdd" voltage range

[Archit Taneja]

The min and max voltage levels for the VDD input to DSI were initially set
to 2.85V (as suggested by the spec).

We have a platform (db410c) where the same regulator supply is also needed
by another consumer at a higher voltage. Bump up the max voltage level to
3.3V. No regressions are seen with this.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/dsi/dsi_cfg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index 2a827d8..53e58203 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -60,7 +60,7 @@ static const struct msm_dsi_config msm8916_dsi_cfg = {
 		.num = 4,
 		.regs = {
 			{"gdsc", -1, -1, -1, -1},
-			{"vdd", 2850000, 2850000, 100000, 100},
+			{"vdd", 2850000, 3300000, 100000, 100},
 			{"vdda", 1200000, 1200000, 100000, 100},
 			{"vddio", 1800000, 1800000, 100000, 100},
 		},
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 6/6] drm/msm/dsi: Parse DSI lanes via DT

[Archit Taneja]

The DSI driver is currently unaware of how the DSI clock and data pins
are mapped to the logical lanes provided by the DSI controller.

Use the generic 'lanes' DT binding provided for DSI lanes (used for DSI
in bindings/display/ti/ti,omap4-dss.txt) to get the desired mapping.

The MSM DSI controller is restricted in terms of what all mappings
it can support. The lane polarity is fixed for all the lanes, the clock
lanes are fixed, and the data lanes can be swapped among each other only
for a few combinations. Apply these restrictions when we parse the DT
data.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>

Signed-off-by: Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 .../devicetree/bindings/display/msm/dsi.txt        |  26 +++-
 drivers/gpu/drm/msm/dsi/dsi_host.c                 | 146 ++++++++++++++++++---
 2 files changed, 149 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
index e7423be..f0d8b6f 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi.txt
+++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
@@ -44,9 +44,28 @@ Optional properties:
 - pinctrl-names: the pin control state names; should contain "default"
 - pinctrl-0: the default pinctrl state (active)
 - pinctrl-n: the "sleep" pinctrl state
-- port: DSI controller output port. This contains one endpoint subnode, with its
-  remote-endpoint set to the phandle of the connected panel's endpoint.
-  See Documentation/devicetree/bindings/graph.txt for device graph info.
+- port: DSI controller output port, containing one endpoint subnode.
+
+  DSI Endpoint properties:
+  - remote-endpoint: set to phandle of the connected panel's endpoint.
+    See Documentation/devicetree/bindings/graph.txt for device graph info.
+  - lanes: list of pin numbers for the DSI lanes: CLKp, CLKn, DATA0p, DATA0n,
+    DATA1p, DATA1n, ...
+    This provides a physical to logical mapping of the DSI lanes. The CLKp and
+    CLKn pins have to be mapped to pins 0 and 1. For data lanes, there are only
+    a limited number of physical to logical mappings possible:
+
+     "0123": Logic 0->Phys 0; Logic 1->Phys 1; Logic 2->Phys 2; Logic 3->Phys 3;
+     "3012": Logic 3->Phys 0; Logic 0->Phys 1; Logic 1->Phys 2; Logic 2->Phys 3;
+     "2301": Logic 2->Phys 0; Logic 3->Phys 1; Logic 0->Phys 2; Logic 1->Phys 3;
+     "1230": Logic 1->Phys 0; Logic 2->Phys 1; Logic 3->Phys 2; Logic 0->Phys 3;
+     "0321": Logic 0->Phys 0; Logic 3->Phys 1; Logic 2->Phys 2; Logic 1->Phys 3;
+     "1032": Logic 1->Phys 0; Logic 0->Phys 1; Logic 3->Phys 2; Logic 2->Phys 3;
+     "2103": Logic 2->Phys 0; Logic 1->Phys 1; Logic 0->Phys 2; Logic 3->Phys 3;
+     "3210": Logic 3->Phys 0; Logic 2->Phys 1; Logic 1->Phys 2; Logic 0->Phys 3;
+
+     Here, a "3012" mapping will be represented by:
+     lanes = <0 1 8 9 2 3 4 5 6 7>;
 
 DSI PHY:
 Required properties:
@@ -131,6 +150,7 @@ Example:
 		port {
 			dsi0_out: endpoint {
 				remote-endpoint = <&panel_in>;
+				lanes = <0 1 2 3 4 5 6 7 8 9>;
 			};
 		};
 	};
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 69bac59..5dc58bb 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -163,6 +163,10 @@ struct msm_dsi_host {
 	enum mipi_dsi_pixel_format format;
 	unsigned long mode_flags;
 
+	/* lane data parsed via DT */
+	int dlane_swap;
+	int num_data_lanes;
+
 	u32 dma_cmd_ctrl_restore;
 
 	bool registered;
@@ -845,19 +849,10 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
 	data = DSI_CTRL_CLK_EN;
 
 	DBG("lane number=%d", msm_host->lanes);
-	if (msm_host->lanes == 2) {
-		data |= DSI_CTRL_LANE1 | DSI_CTRL_LANE2;
-		/* swap lanes for 2-lane panel for better performance */
-		dsi_write(msm_host, REG_DSI_LANE_SWAP_CTRL,
-			DSI_LANE_SWAP_CTRL_DLN_SWAP_SEL(LANE_SWAP_1230));
-	} else {
-		/* Take 4 lanes as default */
-		data |= DSI_CTRL_LANE0 | DSI_CTRL_LANE1 | DSI_CTRL_LANE2 |
-			DSI_CTRL_LANE3;
-		/* Do not swap lanes for 4-lane panel */
-		dsi_write(msm_host, REG_DSI_LANE_SWAP_CTRL,
-			DSI_LANE_SWAP_CTRL_DLN_SWAP_SEL(LANE_SWAP_0123));
-	}
+	data |= ((DSI_CTRL_LANE0 << msm_host->lanes) - DSI_CTRL_LANE0);
+
+	dsi_write(msm_host, REG_DSI_LANE_SWAP_CTRL,
+		  DSI_LANE_SWAP_CTRL_DLN_SWAP_SEL(msm_host->dlane_swap));
 
 	if (!(flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
 		dsi_write(msm_host, REG_DSI_LANE_CTRL,
@@ -1479,6 +1474,9 @@ static int dsi_host_attach(struct mipi_dsi_host *host,
 	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
 	int ret;
 
+	if (dsi->lanes > msm_host->num_data_lanes)
+		return -EINVAL;
+
 	msm_host->channel = dsi->channel;
 	msm_host->lanes = dsi->lanes;
 	msm_host->format = dsi->format;
@@ -1532,6 +1530,105 @@ static struct mipi_dsi_host_ops dsi_host_ops = {
 	.transfer = dsi_host_transfer,
 };
 
+/*
+ * List of supported physical to logical lane mappings.
+ * For example, the 2nd entry represents the following mapping:
+ *
+ * "3012": Logic 3->Phys 0; Logic 0->Phys 1; Logic 1->Phys 2; Logic 2->Phys 3;
+ */
+static const int supported_data_lane_swaps[][4] = {
+	{ 0, 1, 2, 3 },
+	{ 3, 0, 1, 2 },
+	{ 2, 3, 0, 1 },
+	{ 1, 2, 3, 0 },
+	{ 0, 3, 2, 1 },
+	{ 1, 0, 3, 2 },
+	{ 2, 1, 0, 3 },
+	{ 3, 2, 1, 0 },
+};
+
+static int dsi_host_parse_lane_data(struct msm_dsi_host *msm_host,
+				    struct device_node *ep)
+{
+	struct device *dev = &msm_host->pdev->dev;
+	struct property *prop;
+	u32 pins[10];
+	int logic_pos[4];
+	int ret, i, len, num_pins, num_data;
+
+	prop = of_find_property(ep, "lanes", &len);
+	if (!prop) {
+		dev_dbg(dev, "failed to find lane data, setting defaults\n");
+		msm_host->num_data_lanes = 4;
+		msm_host->dlane_swap = 0;
+		return -EINVAL;
+	}
+
+	num_pins = len / sizeof(u32);
+
+	if (num_pins < 4 || num_pins > 10 || num_pins % 2 != 0) {
+		dev_err(dev, "bad number of lanes\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32_array(ep, "lanes", pins, num_pins);
+	if (ret) {
+		dev_err(dev, "failed to read lane data\n");
+		return ret;
+	}
+
+	/* parse CLK pins */
+	if (pins[0] != 0 || pins[1] != 1) {
+		dev_err(dev, "clkp and clkn have to be at positions 0 and 1\n");
+		return -EINVAL;
+	}
+
+	/* parse DATA pins */
+	num_data = 0;
+
+	for (i = 2; i < num_pins; i += 2) {
+		int dp, dn;
+
+		dp = pins[i];
+		dn = pins[i + 1];
+
+		if ((dp < 0 || dp > 9) || (dn < 0 || dn > 9))
+			return -EINVAL;
+
+		if (dn & 1) {
+			if (dn != dp + 1)
+				return -EINVAL;
+		} else {
+			return -EINVAL;
+		}
+
+		logic_pos[num_data++] = dp / 2 - 1;
+	}
+
+	msm_host->num_data_lanes = num_data;
+
+	/*
+	 * compare DT specified physical-logical lane mappings with the ones
+	 * supported by hardware
+	 */
+	for (i = 0; i < ARRAY_SIZE(supported_data_lane_swaps); i++) {
+		const int *swap = supported_data_lane_swaps[i];
+		int j;
+
+		for (j = 0; j < num_data; j++) {
+			if (swap[j] != logic_pos[j])
+				break;
+		}
+
+		if (j == num_data) {
+			msm_host->dlane_swap = i;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
 static int dsi_host_parse_dt(struct msm_dsi_host *msm_host)
 {
 	struct device *dev = &msm_host->pdev->dev;
@@ -1558,17 +1655,21 @@ static int dsi_host_parse_dt(struct msm_dsi_host *msm_host)
 		return 0;
 	}
 
+	ret = dsi_host_parse_lane_data(msm_host, endpoint);
+	if (ret) {
+		dev_err(dev, "%s: invalid lane configuration %d\n",
+			__func__, ret);
+		goto err;
+	}
+
 	/* Get panel node from the output port's endpoint data */
 	device_node = of_graph_get_remote_port_parent(endpoint);
 	if (!device_node) {
 		dev_err(dev, "%s: no valid device\n", __func__);
-		of_node_put(endpoint);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto err;
 	}
 
-	of_node_put(endpoint);
-	of_node_put(device_node);
-
 	msm_host->device_node = device_node;
 
 	if (of_property_read_bool(np, "syscon-sfpb")) {
@@ -1577,11 +1678,16 @@ static int dsi_host_parse_dt(struct msm_dsi_host *msm_host)
 		if (IS_ERR(msm_host->sfpb)) {
 			dev_err(dev, "%s: failed to get sfpb regmap\n",
 				__func__);
-			return PTR_ERR(msm_host->sfpb);
+			ret = PTR_ERR(msm_host->sfpb);
 		}
 	}
 
-	return 0;
+	of_node_put(device_node);
+
+err:
+	of_node_put(endpoint);
+
+	return ret;
 }
 
 int msm_dsi_host_init(struct msm_dsi *msm_dsi)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] drm/msm/mdp: Use atomic helper to set crtc property

[Daniel Vetter]

On Mon, Feb 15, 2016 at 06:30:54PM +0530, Archit Taneja wrote:
> Assign drm_atomic_helper_crtc_set_property helper to MDP4 and MDP5
> crtcs' set_property ops. This replaces the custom funcs that
> returned an error even for standard crtc properties.
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 9 +--------
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 9 +--------
>  2 files changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> index 28df397..4b95d6c 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> @@ -361,13 +361,6 @@ static void mdp4_crtc_atomic_flush(struct drm_crtc *crtc,
>  	request_pending(crtc, PENDING_FLIP);
>  }
>  
> -static int mdp4_crtc_set_property(struct drm_crtc *crtc,
> -		struct drm_property *property, uint64_t val)
> -{
> -	// XXX
> -	return -EINVAL;

Oops. Otoh you can't set atomic props by default through legacy funcs (or
at least they're not listed), so not to harmful.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -}
> -
>  #define CURSOR_WIDTH 64
>  #define CURSOR_HEIGHT 64
>  
> @@ -499,7 +492,7 @@ static const struct drm_crtc_funcs mdp4_crtc_funcs = {
>  	.set_config = drm_atomic_helper_set_config,
>  	.destroy = mdp4_crtc_destroy,
>  	.page_flip = drm_atomic_helper_page_flip,
> -	.set_property = mdp4_crtc_set_property,
> +	.set_property = drm_atomic_helper_crtc_set_property,
>  	.cursor_set = mdp4_crtc_cursor_set,
>  	.cursor_move = mdp4_crtc_cursor_move,
>  	.reset = drm_atomic_helper_crtc_reset,
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> index 20cee5c..9a15205 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> @@ -468,13 +468,6 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc,
>  	request_pending(crtc, PENDING_FLIP);
>  }
>  
> -static int mdp5_crtc_set_property(struct drm_crtc *crtc,
> -		struct drm_property *property, uint64_t val)
> -{
> -	// XXX
> -	return -EINVAL;
> -}
> -
>  static void get_roi(struct drm_crtc *crtc, uint32_t *roi_w, uint32_t *roi_h)
>  {
>  	struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc);
> @@ -625,7 +618,7 @@ static const struct drm_crtc_funcs mdp5_crtc_funcs = {
>  	.set_config = drm_atomic_helper_set_config,
>  	.destroy = mdp5_crtc_destroy,
>  	.page_flip = drm_atomic_helper_page_flip,
> -	.set_property = mdp5_crtc_set_property,
> +	.set_property = drm_atomic_helper_crtc_set_property,
>  	.reset = drm_atomic_helper_crtc_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] drm/msm/dsi: Parse DSI lanes via DT

[Rob Herring]

On Mon, Feb 15, 2016 at 06:30:59PM +0530, Archit Taneja wrote:
> The DSI driver is currently unaware of how the DSI clock and data pins
> are mapped to the logical lanes provided by the DSI controller.
> 
> Use the generic 'lanes' DT binding provided for DSI lanes (used for DSI
> in bindings/display/ti/ti,omap4-dss.txt) to get the desired mapping.
> 
> The MSM DSI controller is restricted in terms of what all mappings
> it can support. The lane polarity is fixed for all the lanes, the clock
> lanes are fixed, and the data lanes can be swapped among each other only
> for a few combinations. Apply these restrictions when we parse the DT
> data.
> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh@kernel.org>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  .../devicetree/bindings/display/msm/dsi.txt        |  26 +++-
>  drivers/gpu/drm/msm/dsi/dsi_host.c                 | 146 ++++++++++++++++++---
>  2 files changed, 149 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> index e7423be..f0d8b6f 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> @@ -44,9 +44,28 @@ Optional properties:
>  - pinctrl-names: the pin control state names; should contain "default"
>  - pinctrl-0: the default pinctrl state (active)
>  - pinctrl-n: the "sleep" pinctrl state
> -- port: DSI controller output port. This contains one endpoint subnode, with its
> -  remote-endpoint set to the phandle of the connected panel's endpoint.
> -  See Documentation/devicetree/bindings/graph.txt for device graph info.
> +- port: DSI controller output port, containing one endpoint subnode.
> +
> +  DSI Endpoint properties:
> +  - remote-endpoint: set to phandle of the connected panel's endpoint.
> +    See Documentation/devicetree/bindings/graph.txt for device graph info.
> +  - lanes: list of pin numbers for the DSI lanes: CLKp, CLKn, DATA0p, DATA0n,
> +    DATA1p, DATA1n, ...
> +    This provides a physical to logical mapping of the DSI lanes. The CLKp and
> +    CLKn pins have to be mapped to pins 0 and 1. For data lanes, there are only

Then why describe the clk pins?

> +    a limited number of physical to logical mappings possible:
> +
> +     "0123": Logic 0->Phys 0; Logic 1->Phys 1; Logic 2->Phys 2; Logic 3->Phys 3;
> +     "3012": Logic 3->Phys 0; Logic 0->Phys 1; Logic 1->Phys 2; Logic 2->Phys 3;
> +     "2301": Logic 2->Phys 0; Logic 3->Phys 1; Logic 0->Phys 2; Logic 1->Phys 3;
> +     "1230": Logic 1->Phys 0; Logic 2->Phys 1; Logic 3->Phys 2; Logic 0->Phys 3;
> +     "0321": Logic 0->Phys 0; Logic 3->Phys 1; Logic 2->Phys 2; Logic 1->Phys 3;
> +     "1032": Logic 1->Phys 0; Logic 0->Phys 1; Logic 3->Phys 2; Logic 2->Phys 3;
> +     "2103": Logic 2->Phys 0; Logic 1->Phys 1; Logic 0->Phys 2; Logic 3->Phys 3;
> +     "3210": Logic 3->Phys 0; Logic 2->Phys 1; Logic 1->Phys 2; Logic 0->Phys 3;
> +
> +     Here, a "3012" mapping will be represented by:
> +     lanes = <0 1 8 9 2 3 4 5 6 7>;

I'm lost here. What does 8 mean for example. The index represents?

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] drm/msm/dsi: Parse DSI lanes via DT

[Archit Taneja]

On 02/22/2016 08:23 AM, Rob Herring wrote:
> On Mon, Feb 15, 2016 at 06:30:59PM +0530, Archit Taneja wrote:
>> The DSI driver is currently unaware of how the DSI clock and data pins
>> are mapped to the logical lanes provided by the DSI controller.
>>
>> Use the generic 'lanes' DT binding provided for DSI lanes (used for DSI
>> in bindings/display/ti/ti,omap4-dss.txt) to get the desired mapping.
>>
>> The MSM DSI controller is restricted in terms of what all mappings
>> it can support. The lane polarity is fixed for all the lanes, the clock
>> lanes are fixed, and the data lanes can be swapped among each other only
>> for a few combinations. Apply these restrictions when we parse the DT
>> data.
>>
>> Cc: devicetree@vger.kernel.org
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>>   .../devicetree/bindings/display/msm/dsi.txt        |  26 +++-
>>   drivers/gpu/drm/msm/dsi/dsi_host.c                 | 146 ++++++++++++++++++---
>>   2 files changed, 149 insertions(+), 23 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
>> index e7423be..f0d8b6f 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
>> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
>> @@ -44,9 +44,28 @@ Optional properties:
>>   - pinctrl-names: the pin control state names; should contain "default"
>>   - pinctrl-0: the default pinctrl state (active)
>>   - pinctrl-n: the "sleep" pinctrl state
>> -- port: DSI controller output port. This contains one endpoint subnode, with its
>> -  remote-endpoint set to the phandle of the connected panel's endpoint.
>> -  See Documentation/devicetree/bindings/graph.txt for device graph info.
>> +- port: DSI controller output port, containing one endpoint subnode.
>> +
>> +  DSI Endpoint properties:
>> +  - remote-endpoint: set to phandle of the connected panel's endpoint.
>> +    See Documentation/devicetree/bindings/graph.txt for device graph info.
>> +  - lanes: list of pin numbers for the DSI lanes: CLKp, CLKn, DATA0p, DATA0n,
>> +    DATA1p, DATA1n, ...
>> +    This provides a physical to logical mapping of the DSI lanes. The CLKp and
>> +    CLKn pins have to be mapped to pins 0 and 1. For data lanes, there are only
>
> Then why describe the clk pins?

I was trying to use a DT binding that is already used for DSI hosts in
TI SoCs.

SoCs give different flexibility over how we map the physical lines with
the actual data/clock lanes in the DSI controller. From what I know,
this flexibility varies. For example, TI SoCs lets us move clock lanes
too, and swap polarities.

If we want all DSI host controllers to use a common binding to describe
lanes, we'd need to go with the most flexible one, and the driver
restricts it to the subsets that we support.

>
>> +    a limited number of physical to logical mappings possible:
>> +
>> +     "0123": Logic 0->Phys 0; Logic 1->Phys 1; Logic 2->Phys 2; Logic 3->Phys 3;
>> +     "3012": Logic 3->Phys 0; Logic 0->Phys 1; Logic 1->Phys 2; Logic 2->Phys 3;
>> +     "2301": Logic 2->Phys 0; Logic 3->Phys 1; Logic 0->Phys 2; Logic 1->Phys 3;
>> +     "1230": Logic 1->Phys 0; Logic 2->Phys 1; Logic 3->Phys 2; Logic 0->Phys 3;
>> +     "0321": Logic 0->Phys 0; Logic 3->Phys 1; Logic 2->Phys 2; Logic 1->Phys 3;
>> +     "1032": Logic 1->Phys 0; Logic 0->Phys 1; Logic 3->Phys 2; Logic 2->Phys 3;
>> +     "2103": Logic 2->Phys 0; Logic 1->Phys 1; Logic 0->Phys 2; Logic 3->Phys 3;
>> +     "3210": Logic 3->Phys 0; Logic 2->Phys 1; Logic 1->Phys 2; Logic 0->Phys 3;
>> +
>> +     Here, a "3012" mapping will be represented by:
>> +     lanes = <0 1 8 9 2 3 4 5 6 7>;
>
> I'm lost here. What does 8 mean for example. The index represents?

The numbers here describe the logical lanes. I.e, the way in which the
DSI controller pushes out data to the PHY. The ordering is as follows:

0 - CLKp
1 - CLKn
2 - DATA0p
3 - DATA0n
4 - DATA1p
5 - DATA1n
6 - DATA2p
7 - DATA2n
8 - DATA3p
9 - DATA3n

The indices corresponding to these values represent the actual
physical pins. The index 0 will always represent the pin CLKp,
index 8 will always represent the physical pin DATA3p. This is
something we would want to keep fixed across all SoCs.

The configuration in the example would provide the following
mapping:

L: CLKp CLKn DATA3p DATA3n ATA0p DATA0n DATA1p DATA1n DATA2p DATA2n

P: CLKp CLKn DATA0p DATA0n DATA1p DATA1n DATA2p DATA2n DATA3p DATA3n

L represents the logical lanes, and P represents the physical lanes.
L is what we provide in the DT property, and P are what the indices
represent. Here, the 3rd logical lane is mapped on to the 0th
physical data lane. The 0th logical lane to the 1st physical data
lane and so on.

Archit

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation

Re: [PATCH 6/6] drm/msm/dsi: Parse DSI lanes via DT

[Rob Herring]

On Mon, Feb 22, 2016 at 1:19 AM, Archit Taneja <architt@codeaurora.org> wrote:
>
>
> On 02/22/2016 08:23 AM, Rob Herring wrote:
>>
>> On Mon, Feb 15, 2016 at 06:30:59PM +0530, Archit Taneja wrote:
>>>
>>> The DSI driver is currently unaware of how the DSI clock and data pins
>>> are mapped to the logical lanes provided by the DSI controller.
>>>
>>> Use the generic 'lanes' DT binding provided for DSI lanes (used for DSI
>>> in bindings/display/ti/ti,omap4-dss.txt) to get the desired mapping.
>>>
>>> The MSM DSI controller is restricted in terms of what all mappings
>>> it can support. The lane polarity is fixed for all the lanes, the clock
>>> lanes are fixed, and the data lanes can be swapped among each other only
>>> for a few combinations. Apply these restrictions when we parse the DT
>>> data.
>>>
>>> Cc: devicetree@vger.kernel.org
>>> Cc: Rob Herring <robh@kernel.org>
>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>
>>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>>> ---
>>>   .../devicetree/bindings/display/msm/dsi.txt        |  26 +++-
>>>   drivers/gpu/drm/msm/dsi/dsi_host.c                 | 146
>>> ++++++++++++++++++---
>>>   2 files changed, 149 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt
>>> b/Documentation/devicetree/bindings/display/msm/dsi.txt
>>> index e7423be..f0d8b6f 100644
>>> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
>>> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
>>> @@ -44,9 +44,28 @@ Optional properties:
>>>   - pinctrl-names: the pin control state names; should contain "default"
>>>   - pinctrl-0: the default pinctrl state (active)
>>>   - pinctrl-n: the "sleep" pinctrl state
>>> -- port: DSI controller output port. This contains one endpoint subnode,
>>> with its
>>> -  remote-endpoint set to the phandle of the connected panel's endpoint.
>>> -  See Documentation/devicetree/bindings/graph.txt for device graph info.
>>> +- port: DSI controller output port, containing one endpoint subnode.
>>> +
>>> +  DSI Endpoint properties:
>>> +  - remote-endpoint: set to phandle of the connected panel's endpoint.
>>> +    See Documentation/devicetree/bindings/graph.txt for device graph
>>> info.
>>> +  - lanes: list of pin numbers for the DSI lanes: CLKp, CLKn, DATA0p,
>>> DATA0n,
>>> +    DATA1p, DATA1n, ...
>>> +    This provides a physical to logical mapping of the DSI lanes. The
>>> CLKp and
>>> +    CLKn pins have to be mapped to pins 0 and 1. For data lanes, there
>>> are only
>>
>>
>> Then why describe the clk pins?
>
>
> I was trying to use a DT binding that is already used for DSI hosts in
> TI SoCs.
>
> SoCs give different flexibility over how we map the physical lines with
> the actual data/clock lanes in the DSI controller. From what I know,
> this flexibility varies. For example, TI SoCs lets us move clock lanes
> too, and swap polarities.
>
> If we want all DSI host controllers to use a common binding to describe
> lanes, we'd need to go with the most flexible one, and the driver
> restricts it to the subsets that we support.
>
>>
>>> +    a limited number of physical to logical mappings possible:
>>> +
>>> +     "0123": Logic 0->Phys 0; Logic 1->Phys 1; Logic 2->Phys 2; Logic
>>> 3->Phys 3;
>>> +     "3012": Logic 3->Phys 0; Logic 0->Phys 1; Logic 1->Phys 2; Logic
>>> 2->Phys 3;
>>> +     "2301": Logic 2->Phys 0; Logic 3->Phys 1; Logic 0->Phys 2; Logic
>>> 1->Phys 3;
>>> +     "1230": Logic 1->Phys 0; Logic 2->Phys 1; Logic 3->Phys 2; Logic
>>> 0->Phys 3;
>>> +     "0321": Logic 0->Phys 0; Logic 3->Phys 1; Logic 2->Phys 2; Logic
>>> 1->Phys 3;
>>> +     "1032": Logic 1->Phys 0; Logic 0->Phys 1; Logic 3->Phys 2; Logic
>>> 2->Phys 3;
>>> +     "2103": Logic 2->Phys 0; Logic 1->Phys 1; Logic 0->Phys 2; Logic
>>> 3->Phys 3;
>>> +     "3210": Logic 3->Phys 0; Logic 2->Phys 1; Logic 1->Phys 2; Logic
>>> 0->Phys 3;
>>> +
>>> +     Here, a "3012" mapping will be represented by:
>>> +     lanes = <0 1 8 9 2 3 4 5 6 7>;
>>
>>
>> I'm lost here. What does 8 mean for example. The index represents?
>
>
> The numbers here describe the logical lanes. I.e, the way in which the
> DSI controller pushes out data to the PHY. The ordering is as follows:
>
> 0 - CLKp
> 1 - CLKn
> 2 - DATA0p
> 3 - DATA0n
> 4 - DATA1p
> 5 - DATA1n
> 6 - DATA2p
> 7 - DATA2n
> 8 - DATA3p
> 9 - DATA3n
>
> The indices corresponding to these values represent the actual
> physical pins. The index 0 will always represent the pin CLKp,
> index 8 will always represent the physical pin DATA3p. This is
> something we would want to keep fixed across all SoCs.
>
> The configuration in the example would provide the following
> mapping:
>
> L: CLKp CLKn DATA3p DATA3n ATA0p DATA0n DATA1p DATA1n DATA2p DATA2n
>
> P: CLKp CLKn DATA0p DATA0n DATA1p DATA1n DATA2p DATA2n DATA3p DATA3n
>
> L represents the logical lanes, and P represents the physical lanes.
> L is what we provide in the DT property, and P are what the indices
> represent. Here, the 3rd logical lane is mapped on to the 0th
> physical data lane. The 0th logical lane to the 1st physical data
> lane and so on.

Okay, so the confusing part is the description is all about lanes
(0-3) but the binding (index and value) is all about pin/signal
numbers. Either simplify the binding to be lanes or describe the
binding in terms of pins.

Rob

Re: [PATCH 6/6] drm/msm/dsi: Parse DSI lanes via DT

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 2868 bytes --]


On 22/02/16 22:10, Rob Herring wrote:

>> If we want all DSI host controllers to use a common binding to describe
>> lanes, we'd need to go with the most flexible one, and the driver
>> restricts it to the subsets that we support.

True, but I wonder if that's necessary. The lane property for the SoC
should be read by the SoC specific driver, right? So the DT property can
be anything. I'm not sure if there's ever a reason for a generic code to
observe the DSI lane setup.

That said, if we do have a common binding, it's perhaps easier for
people to understand the setups for different SoCs.

>>>> +    a limited number of physical to logical mappings possible:
>>>> +
>>>> +     "0123": Logic 0->Phys 0; Logic 1->Phys 1; Logic 2->Phys 2; Logic
>>>> 3->Phys 3;
>>>> +     "3012": Logic 3->Phys 0; Logic 0->Phys 1; Logic 1->Phys 2; Logic
>>>> 2->Phys 3;
>>>> +     "2301": Logic 2->Phys 0; Logic 3->Phys 1; Logic 0->Phys 2; Logic
>>>> 1->Phys 3;
>>>> +     "1230": Logic 1->Phys 0; Logic 2->Phys 1; Logic 3->Phys 2; Logic
>>>> 0->Phys 3;
>>>> +     "0321": Logic 0->Phys 0; Logic 3->Phys 1; Logic 2->Phys 2; Logic
>>>> 1->Phys 3;
>>>> +     "1032": Logic 1->Phys 0; Logic 0->Phys 1; Logic 3->Phys 2; Logic
>>>> 2->Phys 3;
>>>> +     "2103": Logic 2->Phys 0; Logic 1->Phys 1; Logic 0->Phys 2; Logic
>>>> 3->Phys 3;
>>>> +     "3210": Logic 3->Phys 0; Logic 2->Phys 1; Logic 1->Phys 2; Logic
>>>> 0->Phys 3;
>>>> +
>>>> +     Here, a "3012" mapping will be represented by:
>>>> +     lanes = <0 1 8 9 2 3 4 5 6 7>;
>>>
>>>
>>> I'm lost here. What does 8 mean for example. The index represents?
>>
>>
>> The numbers here describe the logical lanes. I.e, the way in which the
>> DSI controller pushes out data to the PHY. The ordering is as follows:

If I read you right, I think that's vice versa. At least the OMAP
version. The OMAP doc says:

"- lanes: list of pin numbers for the DSI lanes: CLK+, CLK-, DATA0+,
DATA0-, DATA1+, DATA1-, ..."

This means that the first value in the array is the pin number for CLK+.
In other words, CLK+ wire going to the panel/encoder is connected to pin
number X.

What the pin number means is SoC specific. CLK+ could be connected to,
say, SoC pin number 123, and then you'd enter 123 as the first value. On
OMAP we use DSI block specific pin numbers, so they start at 0.

> Okay, so the confusing part is the description is all about lanes
> (0-3) but the binding (index and value) is all about pin/signal
> numbers. Either simplify the binding to be lanes or describe the
> binding in terms of pins.

Perhaps "lane-pins" or something would be more descriptive. If we want
to make the description common, I could change the OMAP implementation
to accept the new property name too.

But, I'm not sure if a common description helps much. Any thoughts?

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] drm/msm/dsi: Parse DSI lanes via DT

[Archit Taneja]

On 02/23/2016 02:48 PM, Tomi Valkeinen wrote:
>
> On 22/02/16 22:10, Rob Herring wrote:
>
>>> If we want all DSI host controllers to use a common binding to describe
>>> lanes, we'd need to go with the most flexible one, and the driver
>>> restricts it to the subsets that we support.
>
> True, but I wonder if that's necessary. The lane property for the SoC
> should be read by the SoC specific driver, right? So the DT property can
> be anything. I'm not sure if there's ever a reason for a generic code to
> observe the DSI lane setup.

Yeah, it is very SoC specific.

The only place where it might matter is if a panel/bridge ever needs to
know what pins implement what lanes on the platform. A common binding
there might help us keep the panel driver generic. Although, this need
itself is a bit hypothetical.

>
> That said, if we do have a common binding, it's perhaps easier for
> people to understand the setups for different SoCs.
>
>>>>> +    a limited number of physical to logical mappings possible:
>>>>> +
>>>>> +     "0123": Logic 0->Phys 0; Logic 1->Phys 1; Logic 2->Phys 2; Logic
>>>>> 3->Phys 3;
>>>>> +     "3012": Logic 3->Phys 0; Logic 0->Phys 1; Logic 1->Phys 2; Logic
>>>>> 2->Phys 3;
>>>>> +     "2301": Logic 2->Phys 0; Logic 3->Phys 1; Logic 0->Phys 2; Logic
>>>>> 1->Phys 3;
>>>>> +     "1230": Logic 1->Phys 0; Logic 2->Phys 1; Logic 3->Phys 2; Logic
>>>>> 0->Phys 3;
>>>>> +     "0321": Logic 0->Phys 0; Logic 3->Phys 1; Logic 2->Phys 2; Logic
>>>>> 1->Phys 3;
>>>>> +     "1032": Logic 1->Phys 0; Logic 0->Phys 1; Logic 3->Phys 2; Logic
>>>>> 2->Phys 3;
>>>>> +     "2103": Logic 2->Phys 0; Logic 1->Phys 1; Logic 0->Phys 2; Logic
>>>>> 3->Phys 3;
>>>>> +     "3210": Logic 3->Phys 0; Logic 2->Phys 1; Logic 1->Phys 2; Logic
>>>>> 0->Phys 3;
>>>>> +
>>>>> +     Here, a "3012" mapping will be represented by:
>>>>> +     lanes = <0 1 8 9 2 3 4 5 6 7>;
>>>>
>>>>
>>>> I'm lost here. What does 8 mean for example. The index represents?
>>>
>>>
>>> The numbers here describe the logical lanes. I.e, the way in which the
>>> DSI controller pushes out data to the PHY. The ordering is as follows:
>
> If I read you right, I think that's vice versa. At least the OMAP
> version. The OMAP doc says:
>
> "- lanes: list of pin numbers for the DSI lanes: CLK+, CLK-, DATA0+,
> DATA0-, DATA1+, DATA1-, ..."
>
> This means that the first value in the array is the pin number for CLK+.
> In other words, CLK+ wire going to the panel/encoder is connected to pin
> number X.
>
> What the pin number means is SoC specific. CLK+ could be connected to,
> say, SoC pin number 123, and then you'd enter 123 as the first value. On
> OMAP we use DSI block specific pin numbers, so they start at 0.

You're right. I have it the other way round. Yours describes the
hardware better. I'll change to yours if we decide to have a common
binding.

>
>> Okay, so the confusing part is the description is all about lanes
>> (0-3) but the binding (index and value) is all about pin/signal
>> numbers. Either simplify the binding to be lanes or describe the
>> binding in terms of pins.
>
> Perhaps "lane-pins" or something would be more descriptive. If we want
> to make the description common, I could change the OMAP implementation
> to accept the new property name too.
>
> But, I'm not sure if a common description helps much. Any thoughts?

If having a common binding doesn't bring any benefit, then a common
description doesn't help much. I could then move to a simpler binding
too.

Archit

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation

Re: [PATCH 6/6] drm/msm/dsi: Parse DSI lanes via DT

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]



On 23/02/16 12:43, Archit Taneja wrote:
> 
> 
> On 02/23/2016 02:48 PM, Tomi Valkeinen wrote:
>>
>> On 22/02/16 22:10, Rob Herring wrote:
>>
>>>> If we want all DSI host controllers to use a common binding to describe
>>>> lanes, we'd need to go with the most flexible one, and the driver
>>>> restricts it to the subsets that we support.
>>
>> True, but I wonder if that's necessary. The lane property for the SoC
>> should be read by the SoC specific driver, right? So the DT property can
>> be anything. I'm not sure if there's ever a reason for a generic code to
>> observe the DSI lane setup.
> 
> Yeah, it is very SoC specific.
> 
> The only place where it might matter is if a panel/bridge ever needs to
> know what pins implement what lanes on the platform. A common binding
> there might help us keep the panel driver generic. Although, this need
> itself is a bit hypothetical.

My opinion here is that if the panel/bridge needs to know something
about the DSI lanes/pins, we should have that data in the
panel's/bridge's endpoint data.

So if both SoC and the DSI peripheral need complex DSI pin/lane setup,
you might have very similar data on both sides. There's possibly some
duplication there, but I think it keeps things much simpler.

For example, if the SoC needs OMAP style DSI pin data, and the DSI
peripheral needs to know the amount of DSI lanes used (but nothing
else), you might think it's nice if the DSI peripheral would peek at the
SoC side data, finding out about the DSI lanes.

But I think in that case you should just add a "num-lanes" property to
the DSI peripheral. The DT data for a device should be private to the
driver handling the device, except for some special cases like following
the graph.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 6/6] drm/msm/dsi: Parse DSI lanes via DT

[Rob Herring]

On Tue, Feb 23, 2016 at 01:11:24PM +0200, Tomi Valkeinen wrote:
> 
> 
> On 23/02/16 12:43, Archit Taneja wrote:
> > 
> > 
> > On 02/23/2016 02:48 PM, Tomi Valkeinen wrote:
> >>
> >> On 22/02/16 22:10, Rob Herring wrote:
> >>
> >>>> If we want all DSI host controllers to use a common binding to describe
> >>>> lanes, we'd need to go with the most flexible one, and the driver
> >>>> restricts it to the subsets that we support.
> >>
> >> True, but I wonder if that's necessary. The lane property for the SoC
> >> should be read by the SoC specific driver, right? So the DT property can
> >> be anything. I'm not sure if there's ever a reason for a generic code to
> >> observe the DSI lane setup.
> > 
> > Yeah, it is very SoC specific.

Agreed.

> > The only place where it might matter is if a panel/bridge ever needs to
> > know what pins implement what lanes on the platform. A common binding
> > there might help us keep the panel driver generic. Although, this need
> > itself is a bit hypothetical.
> 
> My opinion here is that if the panel/bridge needs to know something
> about the DSI lanes/pins, we should have that data in the
> panel's/bridge's endpoint data.
> 
> So if both SoC and the DSI peripheral need complex DSI pin/lane setup,
> you might have very similar data on both sides. There's possibly some
> duplication there, but I think it keeps things much simpler.
> 
> For example, if the SoC needs OMAP style DSI pin data, and the DSI
> peripheral needs to know the amount of DSI lanes used (but nothing
> else), you might think it's nice if the DSI peripheral would peek at the
> SoC side data, finding out about the DSI lanes.
> 
> But I think in that case you should just add a "num-lanes" property to
> the DSI peripheral. The DT data for a device should be private to the
> driver handling the device, except for some special cases like following
> the graph.

num-lanes might not be enough. You could need to have a mask instead. 
Not sure if there is really any h/w like that though.

Also, I think it is fine for a parent to look at standard properties in 
a child.

Rob

Re: [PATCH 6/6] drm/msm/dsi: Parse DSI lanes via DT

[Archit Taneja]

On 02/24/2016 01:34 AM, Rob Herring wrote:
> On Tue, Feb 23, 2016 at 01:11:24PM +0200, Tomi Valkeinen wrote:
>>
>>
>> On 23/02/16 12:43, Archit Taneja wrote:
>>>
>>>
>>> On 02/23/2016 02:48 PM, Tomi Valkeinen wrote:
>>>>
>>>> On 22/02/16 22:10, Rob Herring wrote:
>>>>
>>>>>> If we want all DSI host controllers to use a common binding to describe
>>>>>> lanes, we'd need to go with the most flexible one, and the driver
>>>>>> restricts it to the subsets that we support.
>>>>
>>>> True, but I wonder if that's necessary. The lane property for the SoC
>>>> should be read by the SoC specific driver, right? So the DT property can
>>>> be anything. I'm not sure if there's ever a reason for a generic code to
>>>> observe the DSI lane setup.
>>>
>>> Yeah, it is very SoC specific.
>
> Agreed.

Okay. We probably don't need to go with a generic binding, then. I'll
simplify the msm/dsi lane bindings (i.e, drop the clock lanes, and
represent things in lanes instead of pins).

Thanks,
Archit

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation