[linux-sunxi] [PATCH 2/7] drm/sun4i: backend: Create regmap after access is possible

[Chen-Yu Tsai]

The backend has various clocks and reset controls that need to be
enabled and deasserted before register access is possible.

Move the creation of the regmap to after the clocks and reset controls
have been configured where it makes more sense.

Signed-off-by: Chen-Yu Tsai <***@csie.org>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index ec5943627aa5..1cc1780f5091 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -369,13 +369,6 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
if (IS_ERR(regs))
return PTR_ERR(regs);

- backend->engine.regs = devm_regmap_init_mmio(dev, regs,
- &sun4i_backend_regmap_config);
- if (IS_ERR(backend->engine.regs)) {
- dev_err(dev, "Couldn't create the backend regmap\n");
- return PTR_ERR(backend->engine.regs);
- }
-
backend->reset = devm_reset_control_get(dev, NULL);
if (IS_ERR(backend->reset)) {
dev_err(dev, "Couldn't get our reset line\n");
@@ -421,6 +414,13 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
}
}

+ backend->engine.regs = devm_regmap_init_mmio(dev, regs,
+ &sun4i_backend_regmap_config);
+ if (IS_ERR(backend->engine.regs)) {
+ dev_err(dev, "Couldn't create the backend regmap\n");
+ return PTR_ERR(backend->engine.regs);
+ }
+
list_add_tail(&backend->engine.list, &drv->engine_list);

/* Reset the registers */

[Chen-Yu Tsai]

Commit 4636ce93d5b2 ("drm/fb-cma-helper: Add drm_fb_cma_get_gem_addr()")
adds a new helper, which covers fetching a drm_framebuffer's GEM object
and calculating the buffer address for a given plane.

This patch uses this helper to replace our own open coded version of the
same function.

Signed-off-by: Chen-Yu Tsai <***@csie.org>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 1cc1780f5091..243ddfdc9403 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -209,22 +209,11 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend,
{
struct drm_plane_state *state = plane->state;
struct drm_framebuffer *fb = state->fb;
- struct drm_gem_cma_object *gem;
u32 lo_paddr, hi_paddr;
dma_addr_t paddr;
- int bpp;
-
- /* Get the physical address of the buffer in memory */
- gem = drm_fb_cma_get_gem_obj(fb, 0);
-
- DRM_DEBUG_DRIVER("Using GEM @ %pad\n", &gem->paddr);
-
- /* Compute the start of the displayed memory */
- bpp = fb->format->cpp[0];
- paddr = gem->paddr + fb->offsets[0];
- paddr += (state->src_x >> 16) * bpp;
- paddr += (state->src_y >> 16) * fb->pitches[0];

+ /* Get the start of the displayed memory */
+ paddr = drm_fb_cma_get_gem_addr(fb, state, 0);
DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr);

/* Write the 32 lower bits of the address (in bits) */

[Chen-Yu Tsai]

The display backend, as well as other peripherals that have a DRAM
clock gate and access DRAM directly, bypassing the system bus,
address the DRAM starting from 0x0, while physical addresses the
system uses starts from 0x40000000 (or 0x20000000 in A80's case).

Correct the address configured into the backend layer registers
by PHYS_OFFSET to account for this.

Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
Signed-off-by: Chen-Yu Tsai <***@csie.org>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 4fefd8add714..d18d7e88d511 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -216,6 +216,13 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend,
paddr = drm_fb_cma_get_gem_addr(fb, state, 0);
DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr);

+ /*
+ * backend DMA accesses DRAM directly, bypassing the system
+ * bus. As such, the address range is different and the buffer
+ * address needs to be corrected.
+ */
+ paddr -= PHYS_OFFSET;
+
/* Write the 32 lower bits of the address (in bits) */
lo_paddr = paddr << 3;
DRM_DEBUG_DRIVER("Setting address lower bits to 0x%x\n", lo_paddr);

[Maxime Ripard]

Hi,

I've applied all the other patches.

Post by Chen-Yu Tsai
The display backend, as well as other peripherals that have a DRAM
clock gate and access DRAM directly, bypassing the system bus,
address the DRAM starting from 0x0, while physical addresses the
system uses starts from 0x40000000 (or 0x20000000 in A80's case).
Correct the address configured into the backend layer registers
by PHYS_OFFSET to account for this.

However, I'm a bit confused at this.

The driver has been working so far, which it wouldn't have been able
to if the address was wrong. How was this problem noticed, and how can
that fix not be an issue in itself?

Thanks!
Maxime

[Chen-Yu Tsai]

Hi,

On Mon, Oct 16, 2017 at 4:00 PM, Maxime Ripard

Post by Maxime Ripard
Hi,
I've applied all the other patches.

Post by Chen-Yu Tsai
The display backend, as well as other peripherals that have a DRAM
clock gate and access DRAM directly, bypassing the system bus,
address the DRAM starting from 0x0, while physical addresses the
system uses starts from 0x40000000 (or 0x20000000 in A80's case).
Correct the address configured into the backend layer registers
by PHYS_OFFSET to account for this.

However, I'm a bit confused at this.
The driver has been working so far, which it wouldn't have been able
to if the address was wrong. How was this problem noticed, and how can
that fix not be an issue in itself?

This problem was witnessed on the Cubietruck, which has 2GB of RAM.

I believe devices with less RAM function normally due to the DRAM
address wrapping around. CMA seems to always allocate its buffer
at a very high address, close to the end of DRAM. On a 1GB RAM
device, the physical address would be something like 0x78000000.
The DRAM address 0x78000000 would access the same DRAM region as
0x38000000 on a system, as the DRAM address would only span 0x0 ~
0x3fffffff. The bit 0x40000000 is non-functional in this case.

However on the Cubietruck, the DRAM is 2GB. The physical address
is 0x40000000 ~ 0xbfffffff. The buffer would be something like
0xb8000000. But the DRAM address span 0x0 ~ 0x7fffffff, meaning
the buffer address wraps around to 0x38000000, which is wrong.
The correct DRAM address for it should be 0x78000000.

ChenYu

Post by Maxime Ripard
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
For more options, visit https://groups.google.com/d/optout.

[Maxime Ripard]

Post by Chen-Yu Tsai

Post by Maxime Ripard

Post by Chen-Yu Tsai
The display backend, as well as other peripherals that have a DRAM
clock gate and access DRAM directly, bypassing the system bus,
address the DRAM starting from 0x0, while physical addresses the
system uses starts from 0x40000000 (or 0x20000000 in A80's case).
Correct the address configured into the backend layer registers
by PHYS_OFFSET to account for this.

However, I'm a bit confused at this.
The driver has been working so far, which it wouldn't have been able
to if the address was wrong. How was this problem noticed, and how can
that fix not be an issue in itself?

This problem was witnessed on the Cubietruck, which has 2GB of RAM.
I believe devices with less RAM function normally due to the DRAM
address wrapping around. CMA seems to always allocate its buffer
at a very high address, close to the end of DRAM. On a 1GB RAM
device, the physical address would be something like 0x78000000.
The DRAM address 0x78000000 would access the same DRAM region as
0x38000000 on a system, as the DRAM address would only span 0x0 ~
0x3fffffff. The bit 0x40000000 is non-functional in this case.
However on the Cubietruck, the DRAM is 2GB. The physical address
is 0x40000000 ~ 0xbfffffff. The buffer would be something like
0xb8000000. But the DRAM address span 0x0 ~ 0x7fffffff, meaning
the buffer address wraps around to 0x38000000, which is wrong.
The correct DRAM address for it should be 0x78000000.

That looks great. Can you put that in the commit log?

Thanks!
Maxime

[Chen-Yu Tsai]

On Tue, Oct 17, 2017 at 4:15 AM, Maxime Ripard

Post by Maxime Ripard

Post by Chen-Yu Tsai

Post by Maxime Ripard

Post by Chen-Yu Tsai
The display backend, as well as other peripherals that have a DRAM
clock gate and access DRAM directly, bypassing the system bus,
address the DRAM starting from 0x0, while physical addresses the
system uses starts from 0x40000000 (or 0x20000000 in A80's case).
Correct the address configured into the backend layer registers
by PHYS_OFFSET to account for this.

However, I'm a bit confused at this.
The driver has been working so far, which it wouldn't have been able
to if the address was wrong. How was this problem noticed, and how can
that fix not be an issue in itself?

This problem was witnessed on the Cubietruck, which has 2GB of RAM.
I believe devices with less RAM function normally due to the DRAM
address wrapping around. CMA seems to always allocate its buffer
at a very high address, close to the end of DRAM. On a 1GB RAM
device, the physical address would be something like 0x78000000.
The DRAM address 0x78000000 would access the same DRAM region as
0x38000000 on a system, as the DRAM address would only span 0x0 ~
0x3fffffff. The bit 0x40000000 is non-functional in this case.
However on the Cubietruck, the DRAM is 2GB. The physical address
is 0x40000000 ~ 0xbfffffff. The buffer would be something like
0xb8000000. But the DRAM address span 0x0 ~ 0x7fffffff, meaning
the buffer address wraps around to 0x38000000, which is wrong.
The correct DRAM address for it should be 0x78000000.

That looks great. Can you put that in the commit log?

Will do.

ChenYu

[i***@aosc.io]

在 2017-10-16 16:00，Maxime Ripard 写道：

Post by Maxime Ripard
Hi,
I've applied all the other patches.

Post by Chen-Yu Tsai
The display backend, as well as other peripherals that have a DRAM
clock gate and access DRAM directly, bypassing the system bus,
address the DRAM starting from 0x0, while physical addresses the
system uses starts from 0x40000000 (or 0x20000000 in A80's case).
Correct the address configured into the backend layer registers
by PHYS_OFFSET to account for this.

However, I'm a bit confused at this.
The driver has been working so far, which it wouldn't have been able
to if the address was wrong. How was this problem noticed, and how can
that fix not be an issue in itself?

On devices with <=1GiB (0x40000000) DRAM, the DRAM access will just wrap
back and no problem would be seen.

Post by Maxime Ripard
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Chen-Yu Tsai]

Many of the backend's layer configuration registers have undefined
default values. This poses a risk as we use regmap_update_bits in
some places, and don't overwrite the whole register.

At probe/bind time we explicitly clear all the control registers
by writing 0 to them. This patch adds a more detailed explanation
on why we're doing this.

Signed-off-by: Chen-Yu Tsai <***@csie.org>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 243ddfdc9403..4fefd8add714 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -412,7 +412,14 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,

list_add_tail(&backend->engine.list, &drv->engine_list);

- /* Reset the registers */
+ /*
+ * Many of the backend's layer configuration registers have
+ * undefined default values. This poses a risk as we use
+ * regmap_update_bits in some places, and don't overwrite
+ * the whole register.
+ *
+ * Clear the registers here to have something predictable.
+ */
for (i = 0x800; i < 0x1000; i += 4)
regmap_write(backend->engine.regs, i, 0);

[Chen-Yu Tsai]

While debugging inverted color from the HDMI output on the A10, I
found that the lowest 3 bits were set. These were cleared on A20
boards that had normal display output. By manually toggling these
bits the mapping of the color components to these bits was found.

While these are not used anywhere, it would be nice to document
them somewhere.

Signed-off-by: Chen-Yu Tsai <***@csie.org>
---
drivers/gpu/drm/sun4i/sun4i_hdmi.h | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 9b97da39927e..b685ee11623d 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -72,6 +72,11 @@
#define SUN4I_HDMI_PAD_CTRL1_HALVE_CLK BIT(6)
#define SUN4I_HDMI_PAD_CTRL1_REG_AMP(n) (((n) & 7) << 3)

+/* These bits seem to invert the TMDS data channels */
+#define SUN4I_HDMI_PAD_CTRL1_INVERT_R BIT(2)
+#define SUN4I_HDMI_PAD_CTRL1_INVERT_G BIT(1)
+#define SUN4I_HDMI_PAD_CTRL1_INVERT_B BIT(0)
+
#define SUN4I_HDMI_PLL_CTRL_REG 0x208
#define SUN4I_HDMI_PLL_CTRL_PLL_EN BIT(31)
#define SUN4I_HDMI_PLL_CTRL_BWS BIT(30)

[Chen-Yu Tsai]

Initially we configured the PAD_CTRL1 register at probe/bind time.
However it seems the HDMI controller will modify some of the bits
in this register by itself. On the A10 it is particularly annoying
as it toggles the output invert bits, which inverts the colors on
the display output.

The U-boot driver this driver is based on sets this register twice,
though it seems it's only needed for actual display output. Hence
we move it to the mode_set function.

Signed-off-by: Chen-Yu Tsai <***@csie.org>
---
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index 027b5608dbe6..6ca6e6a74c4a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -144,6 +144,22 @@ static void sun4i_hdmi_mode_set(struct drm_encoder *encoder,
writel(SUN4I_HDMI_UNKNOWN_INPUT_SYNC,
hdmi->base + SUN4I_HDMI_UNKNOWN_REG);

+ /*
+ * Setup output pad (?) controls
+ *
+ * This is done here instead of at probe/bind time because
+ * the controller seems to toggle some of the bits on its own.
+ *
+ * We can't just initialize the register there, we need to
+ * protect the clock bits that have already been read out and
+ * cached by the clock framework.
+ */
+ val = readl(hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
+ val &= SUN4I_HDMI_PAD_CTRL1_HALVE_CLK;
+ val |= hdmi->variant->pad_ctrl1_init_val;
+ writel(val, hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
+ val = readl(hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
+
/* Setup timing registers */
writel(SUN4I_HDMI_VID_TIMING_X(mode->hdisplay) |
SUN4I_HDMI_VID_TIMING_Y(mode->vdisplay),
@@ -489,16 +505,6 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
writel(hdmi->variant->pad_ctrl0_init_val,
hdmi->base + SUN4I_HDMI_PAD_CTRL0_REG);

- /*
- * We can't just initialize the register there, we need to
- * protect the clock bits that have already been read out and
- * cached by the clock framework.
- */
- reg = readl(hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
- reg &= SUN4I_HDMI_PAD_CTRL1_HALVE_CLK;
- reg |= hdmi->variant->pad_ctrl1_init_val;
- writel(reg, hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
-
reg = readl(hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
reg &= SUN4I_HDMI_PLL_CTRL_DIV_MASK;
reg |= hdmi->variant->pll_ctrl_init_val;

[Chen-Yu Tsai]

Even though the components framework can handle duplicate entries,
the extra entries cause a lot more debug messages to be generated,
which would be confusing to developers not familiar with our driver
and the framework in general.

Instead, we can scan the relatively small queue and check if the
component to be added is already queued up. Since the display
pipelines are symmetrical (not considering the third display
pipeline on the A80), and we add components level by level, when
we get to the second instance at the same level, any shared downstream
components would already be in the queue.

Signed-off-by: Chen-Yu Tsai <***@csie.org>
---
drivers/gpu/drm/sun4i/sun4i_drv.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index a2012638d5f7..b5879d4620d8 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -226,6 +226,18 @@ struct endpoint_list {
struct list_head list;
};

+static bool node_is_in_list(struct list_head *endpoints,
+ struct device_node *node)
+{
+ struct endpoint_list *endpoint;
+
+ list_for_each_entry(endpoint, endpoints, list)
+ if (endpoint->node == node)
+ return true;
+
+ return false;
+}
+
static int sun4i_drv_add_endpoints(struct device *dev,
struct list_head *endpoints,
struct component_match **match,
@@ -292,6 +304,10 @@ static int sun4i_drv_add_endpoints(struct device *dev,
}
}

+ /* skip downstream node if it is already in the queue */
+ if (node_is_in_list(endpoints, remote))
+ continue;
+
/* Add downstream nodes to the queue */
endpoint = kzalloc(sizeof(*endpoint), GFP_KERNEL);
if (!endpoint) {