* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-27 14:36 ` Jonathan Liu
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-27 14:36 UTC (permalink / raw)
To: Maxime Ripard, David Airlie, Chen-Yu Tsai
Cc: linux-kernel, dri-devel, linux-arm-kernel, linux-sunxi, Jonathan Liu
The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
"As in the general case the DDC bus is accessible by the kernel at the I2C
level, drivers must make all reasonable efforts to expose it as an I2C
adapter and use drm_get_edid() instead of abusing this function."
Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
for purposes other than reading the EDID such as modifying the EDID or
using the HDMI DDC pins as an I2C bus through the I2C dev interface from
userspace (e.g. i2c-tools).
Implement this for A10s.
Signed-off-by: Jonathan Liu <net147@gmail.com>
---
Changes for v5:
- Use devm_kzalloc instead of devm_kmemdup and remove const struct i2c_adapter
- Rework to use readl_poll_timeout for checking FIFO status
Changes for v4:
- Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c
- Clean up indentation in sun4i_hdmi.h
- Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX
and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the
value to use the GENMASK macro to make it clear that it is derived from
the width of the field in the register
- Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be
SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW
- Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register
- Change struct i2c_adapter to be const by using devm_kmemdup on creation
- Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an
I2C message
- Instead of waiting for 1-2 bytes to transfer, wait for the time it would
take for remaining bytes to transfer (limited by FIFO size)
- Add additional comments
Changes for v3:
- Explain why drm_do_get_edid should be used and why it's better to expose it
as an I2C adapter in commit message
- Reorder bit defines in descending order for consistency
- Keep old unused macros instead of removing them
- The v2 algorithm split large transfers into 16 byte transfers but this may
cause a large single write to be treated as multiple writes causing data
corruption. The algorithm has been reworked to not split larger transfers
and make more use of the FIFO to avoid this.
- Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
sun4i_hdmi_i2c.c
- Reformatted code
- Instead of masking bits that we don't want to check for errors, explicitly
check the error bits
- Clear error bits at start of transfer in case of error from previous transfer
- Poll for completion of FIFO clear after setting FIFO clear bit
Changes for v2:
- Rebased against Maxime's sunxi-drm/for-next branch
- Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
any of the calls after the I2C adapter is created fails
- Remove unnecessary includes in sun4i_hdmi_i2c.c
drivers/gpu/drm/sun4i/Makefile | 1 +
drivers/gpu/drm/sun4i/sun4i_hdmi.h | 23 ++++
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++-------------
drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 226 +++++++++++++++++++++++++++++++++
4 files changed, 261 insertions(+), 90 deletions(-)
create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
sun4i-drm-y += sun4i_framebuffer.o
sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2f2f2ff1ea63..eaff92fe236c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -96,6 +96,7 @@
#define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31)
#define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30)
#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8)
+#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8)
#define SUN4I_HDMI_DDC_CTRL_RESET BIT(0)
@@ -105,14 +106,33 @@
#define SUN4I_HDMI_DDC_ADDR_OFFSET(off) (((off) & 0xff) << 8)
#define SUN4I_HDMI_DDC_ADDR_SLAVE(addr) ((addr) & 0xff)
+#define SUN4I_HDMI_DDC_INT_STATUS_REG 0x50c
+#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION BIT(7)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW BIT(6)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW BIT(5)
+#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4)
+#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR BIT(3)
+#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR BIT(2)
+#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR BIT(1)
+#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE BIT(0)
+
#define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510
#define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31)
+#define SUN4I_HDMI_DDC_FIFO_STATUS_REG 0x514
+#define SUN4I_HDMI_DDC_FIFO_STATUS_FULL BIT(6)
+#define SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY BIT(5)
+#define SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK GENMASK(4, 0)
+
#define SUN4I_HDMI_DDC_FIFO_DATA_REG 0x518
+
#define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c
+#define SUN4I_HDMI_DDC_BYTE_COUNT_MAX GENMASK(9, 0)
#define SUN4I_HDMI_DDC_CMD_REG 0x520
#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ 6
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE 3
#define SUN4I_HDMI_DDC_CLK_REG 0x528
#define SUN4I_HDMI_DDC_CLK_M(m) (((m) & 0x7) << 3)
@@ -146,6 +166,8 @@ struct sun4i_hdmi {
struct clk *ddc_clk;
struct clk *tmds_clk;
+ struct i2c_adapter *i2c;
+
struct sun4i_drv *drv;
bool hdmi_monitor;
@@ -153,5 +175,6 @@ struct sun4i_hdmi {
int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
+int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi);
#endif /* _SUN4I_HDMI_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index d3398f6250ef..b74607feb35c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -29,8 +29,6 @@
#include "sun4i_hdmi.h"
#include "sun4i_tcon.h"
-#define DDC_SEGMENT_ADDR 0x30
-
static inline struct sun4i_hdmi *
drm_encoder_to_sun4i_hdmi(struct drm_encoder *encoder)
{
@@ -184,93 +182,13 @@ static const struct drm_encoder_funcs sun4i_hdmi_funcs = {
.destroy = drm_encoder_cleanup,
};
-static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi,
- unsigned int blk, unsigned int offset,
- u8 *buf, unsigned int count)
-{
- unsigned long reg;
- int i;
-
- reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
- reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
- writel(reg | SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ,
- hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-
- writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) |
- SUN4I_HDMI_DDC_ADDR_EDDC(DDC_SEGMENT_ADDR << 1) |
- SUN4I_HDMI_DDC_ADDR_OFFSET(offset) |
- SUN4I_HDMI_DDC_ADDR_SLAVE(DDC_ADDR),
- hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
-
- reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
- writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
- hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
-
- writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
- writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ,
- hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
-
- reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
- writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
- hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-
- if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
- !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
- 100, 100000))
- return -EIO;
-
- for (i = 0; i < count; i++)
- buf[i] = readb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG);
-
- return 0;
-}
-
-static int sun4i_hdmi_read_edid_block(void *data, u8 *buf, unsigned int blk,
- size_t length)
-{
- struct sun4i_hdmi *hdmi = data;
- int retry = 2, i;
-
- do {
- for (i = 0; i < length; i += SUN4I_HDMI_DDC_FIFO_SIZE) {
- unsigned char offset = blk * EDID_LENGTH + i;
- unsigned int count = min((unsigned int)SUN4I_HDMI_DDC_FIFO_SIZE,
- length - i);
- int ret;
-
- ret = sun4i_hdmi_read_sub_block(hdmi, blk, offset,
- buf + i, count);
- if (ret)
- return ret;
- }
- } while (!drm_edid_block_valid(buf, blk, true, NULL) && (retry--));
-
- return 0;
-}
-
static int sun4i_hdmi_get_modes(struct drm_connector *connector)
{
struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
- unsigned long reg;
struct edid *edid;
int ret;
- /* Reset i2c controller */
- writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET,
- hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
- if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
- !(reg & SUN4I_HDMI_DDC_CTRL_RESET),
- 100, 2000))
- return -EIO;
-
- writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE |
- SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE,
- hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG);
-
- clk_prepare_enable(hdmi->ddc_clk);
- clk_set_rate(hdmi->ddc_clk, 100000);
-
- edid = drm_do_get_edid(connector, sun4i_hdmi_read_edid_block, hdmi);
+ edid = drm_get_edid(connector, hdmi->i2c);
if (!edid)
return 0;
@@ -282,8 +200,6 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
ret = drm_add_edid_modes(connector, edid);
kfree(edid);
- clk_disable_unprepare(hdmi->ddc_clk);
-
return ret;
}
@@ -407,9 +323,9 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
SUN4I_HDMI_PLL_CTRL_PLL_EN;
writel(reg, hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
- ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
+ ret = sun4i_hdmi_i2c_create(dev, hdmi);
if (ret) {
- dev_err(dev, "Couldn't create the DDC clock\n");
+ dev_err(dev, "Couldn't create the HDMI I2C adapter\n");
return ret;
}
@@ -422,13 +338,15 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
NULL);
if (ret) {
dev_err(dev, "Couldn't initialise the HDMI encoder\n");
- return ret;
+ goto err_del_i2c_adapter;
}
hdmi->encoder.possible_crtcs = drm_of_find_possible_crtcs(drm,
dev->of_node);
- if (!hdmi->encoder.possible_crtcs)
- return -EPROBE_DEFER;
+ if (!hdmi->encoder.possible_crtcs) {
+ ret = -EPROBE_DEFER;
+ goto err_del_i2c_adapter;
+ }
drm_connector_helper_add(&hdmi->connector,
&sun4i_hdmi_connector_helper_funcs);
@@ -451,6 +369,8 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
err_cleanup_connector:
drm_encoder_cleanup(&hdmi->encoder);
+err_del_i2c_adapter:
+ i2c_del_adapter(hdmi->i2c);
return ret;
}
@@ -461,6 +381,7 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
drm_connector_cleanup(&hdmi->connector);
drm_encoder_cleanup(&hdmi->encoder);
+ i2c_del_adapter(hdmi->i2c);
}
static const struct component_ops sun4i_hdmi_ops = {
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
new file mode 100644
index 000000000000..2afbce02532e
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
@@ -0,0 +1,226 @@
+/*
+ * Copyright (C) 2016 Maxime Ripard <maxime.ripard@free-electrons.com>
+ * Copyright (C) 2017 Jonathan Liu <net147@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/i2c.h>
+#include <linux/iopoll.h>
+#include <linux/ktime.h>
+
+#include "sun4i_hdmi.h"
+
+#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \
+ SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \
+ SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \
+ SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \
+ SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \
+ SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \
+ SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \
+)
+
+static bool is_err_status(u32 int_status)
+{
+ return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK);
+}
+
+static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status,
+ u32 flag)
+{
+ *fifo_status = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
+ return !(*fifo_status & flag);
+}
+
+static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool read)
+{
+ /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */
+ unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
+ clk_get_rate(hdmi->ddc_clk)) * 9;
+ u32 int_status;
+ u32 fifo_status;
+ /* Read needs empty flag unset, write needs full flag unset */
+ u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
+ SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
+ int ret;
+
+ /* Wait until error or FIFO ready */
+ ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
+ int_status,
+ is_err_status(int_status) ||
+ is_fifo_flag_unset(hdmi, &fifo_status, flag),
+ min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
+ 100000);
+
+ if (is_err_status(int_status))
+ return -EIO;
+ if (ret)
+ return -ETIMEDOUT;
+
+ /* Read FIFO level */
+ int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
+
+ /* Limit transfer length using FIFO level to avoid underflow/overflow */
+ len = min(len, read ? level : (SUN4I_HDMI_DDC_FIFO_SIZE - level));
+
+ if (read)
+ readsb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
+ else
+ writesb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
+
+ return len;
+}
+
+static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg)
+{
+ int i, len;
+ u32 reg;
+
+ /* Clear errors */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
+ reg &= ~SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK;
+ writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
+
+ /* Set FIFO direction */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+ reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
+ reg |= (msg->flags & I2C_M_RD) ?
+ SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ :
+ SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE;
+ writel(reg, hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+
+ /* Set I2C address */
+ writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->addr),
+ hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
+
+ /* Clear FIFO */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
+ writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
+ hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
+ if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG,
+ reg,
+ !(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR),
+ 100, 100000))
+ return -EIO;
+
+ /* Set transfer length */
+ writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
+
+ /* Set command */
+ writel(msg->flags & I2C_M_RD ?
+ SUN4I_HDMI_DDC_CMD_IMPLICIT_READ :
+ SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE,
+ hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
+
+ /* Start command */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+ writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
+ hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+
+ /* Transfer bytes */
+ for (i = 0; i < msg->len; i += len) {
+ len = fifo_transfer(hdmi, msg->buf + i, msg->len - i,
+ msg->flags & I2C_M_RD);
+ if (len <= 0)
+ return len;
+ }
+
+ /* Wait for command to finish */
+ if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG,
+ reg,
+ !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
+ 100, 100000))
+ return -EIO;
+
+ /* Check for errors */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
+ if ((reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) ||
+ !(reg & SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE)) {
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int sun4i_hdmi_i2c_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ struct sun4i_hdmi *hdmi = i2c_get_adapdata(adap);
+ u32 reg;
+ int err, i, ret = num;
+
+ for (i = 0; i < num; i++) {
+ if (!msgs[i].len)
+ return -EINVAL;
+ if (msgs[i].len > SUN4I_HDMI_DDC_BYTE_COUNT_MAX)
+ return -EINVAL;
+ }
+
+ /* Reset I2C controller */
+ writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET,
+ hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+ if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
+ !(reg & SUN4I_HDMI_DDC_CTRL_RESET),
+ 100, 2000))
+ return -EIO;
+
+ writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE |
+ SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE,
+ hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG);
+
+ clk_prepare_enable(hdmi->ddc_clk);
+ clk_set_rate(hdmi->ddc_clk, 100000);
+
+ for (i = 0; i < num; i++) {
+ err = xfer_msg(hdmi, &msgs[i]);
+ if (err) {
+ ret = err;
+ break;
+ }
+ }
+
+ clk_disable_unprepare(hdmi->ddc_clk);
+ return ret;
+}
+
+static u32 sun4i_hdmi_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm sun4i_hdmi_i2c_algorithm = {
+ .master_xfer = sun4i_hdmi_i2c_xfer,
+ .functionality = sun4i_hdmi_i2c_func,
+};
+
+int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi)
+{
+ struct i2c_adapter *adap;
+ int ret = 0;
+
+ ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
+ if (ret)
+ return ret;
+
+ adap = devm_kzalloc(dev, sizeof(*adap), GFP_KERNEL);
+ if (!adap)
+ return -ENOMEM;
+
+ adap->owner = THIS_MODULE;
+ adap->class = I2C_CLASS_DDC;
+ adap->algo = &sun4i_hdmi_i2c_algorithm;
+ strlcpy(adap->name, "sun4i_hdmi_i2c adapter", sizeof(adap->name));
+ i2c_set_adapdata(adap, hdmi);
+
+ ret = i2c_add_adapter(adap);
+ if (ret)
+ return ret;
+
+ hdmi->i2c = adap;
+
+ return ret;
+}
--
2.13.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-27 14:36 ` Jonathan Liu
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-27 14:36 UTC (permalink / raw)
To: linux-arm-kernel
The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
"As in the general case the DDC bus is accessible by the kernel at the I2C
level, drivers must make all reasonable efforts to expose it as an I2C
adapter and use drm_get_edid() instead of abusing this function."
Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
for purposes other than reading the EDID such as modifying the EDID or
using the HDMI DDC pins as an I2C bus through the I2C dev interface from
userspace (e.g. i2c-tools).
Implement this for A10s.
Signed-off-by: Jonathan Liu <net147@gmail.com>
---
Changes for v5:
- Use devm_kzalloc instead of devm_kmemdup and remove const struct i2c_adapter
- Rework to use readl_poll_timeout for checking FIFO status
Changes for v4:
- Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c
- Clean up indentation in sun4i_hdmi.h
- Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX
and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the
value to use the GENMASK macro to make it clear that it is derived from
the width of the field in the register
- Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be
SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW
- Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register
- Change struct i2c_adapter to be const by using devm_kmemdup on creation
- Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an
I2C message
- Instead of waiting for 1-2 bytes to transfer, wait for the time it would
take for remaining bytes to transfer (limited by FIFO size)
- Add additional comments
Changes for v3:
- Explain why drm_do_get_edid should be used and why it's better to expose it
as an I2C adapter in commit message
- Reorder bit defines in descending order for consistency
- Keep old unused macros instead of removing them
- The v2 algorithm split large transfers into 16 byte transfers but this may
cause a large single write to be treated as multiple writes causing data
corruption. The algorithm has been reworked to not split larger transfers
and make more use of the FIFO to avoid this.
- Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
sun4i_hdmi_i2c.c
- Reformatted code
- Instead of masking bits that we don't want to check for errors, explicitly
check the error bits
- Clear error bits at start of transfer in case of error from previous transfer
- Poll for completion of FIFO clear after setting FIFO clear bit
Changes for v2:
- Rebased against Maxime's sunxi-drm/for-next branch
- Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
any of the calls after the I2C adapter is created fails
- Remove unnecessary includes in sun4i_hdmi_i2c.c
drivers/gpu/drm/sun4i/Makefile | 1 +
drivers/gpu/drm/sun4i/sun4i_hdmi.h | 23 ++++
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++-------------
drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 226 +++++++++++++++++++++++++++++++++
4 files changed, 261 insertions(+), 90 deletions(-)
create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
sun4i-drm-y += sun4i_framebuffer.o
sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2f2f2ff1ea63..eaff92fe236c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -96,6 +96,7 @@
#define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31)
#define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30)
#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8)
+#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8)
#define SUN4I_HDMI_DDC_CTRL_RESET BIT(0)
@@ -105,14 +106,33 @@
#define SUN4I_HDMI_DDC_ADDR_OFFSET(off) (((off) & 0xff) << 8)
#define SUN4I_HDMI_DDC_ADDR_SLAVE(addr) ((addr) & 0xff)
+#define SUN4I_HDMI_DDC_INT_STATUS_REG 0x50c
+#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION BIT(7)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW BIT(6)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW BIT(5)
+#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4)
+#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR BIT(3)
+#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR BIT(2)
+#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR BIT(1)
+#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE BIT(0)
+
#define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510
#define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31)
+#define SUN4I_HDMI_DDC_FIFO_STATUS_REG 0x514
+#define SUN4I_HDMI_DDC_FIFO_STATUS_FULL BIT(6)
+#define SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY BIT(5)
+#define SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK GENMASK(4, 0)
+
#define SUN4I_HDMI_DDC_FIFO_DATA_REG 0x518
+
#define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c
+#define SUN4I_HDMI_DDC_BYTE_COUNT_MAX GENMASK(9, 0)
#define SUN4I_HDMI_DDC_CMD_REG 0x520
#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ 6
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE 3
#define SUN4I_HDMI_DDC_CLK_REG 0x528
#define SUN4I_HDMI_DDC_CLK_M(m) (((m) & 0x7) << 3)
@@ -146,6 +166,8 @@ struct sun4i_hdmi {
struct clk *ddc_clk;
struct clk *tmds_clk;
+ struct i2c_adapter *i2c;
+
struct sun4i_drv *drv;
bool hdmi_monitor;
@@ -153,5 +175,6 @@ struct sun4i_hdmi {
int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
+int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi);
#endif /* _SUN4I_HDMI_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index d3398f6250ef..b74607feb35c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -29,8 +29,6 @@
#include "sun4i_hdmi.h"
#include "sun4i_tcon.h"
-#define DDC_SEGMENT_ADDR 0x30
-
static inline struct sun4i_hdmi *
drm_encoder_to_sun4i_hdmi(struct drm_encoder *encoder)
{
@@ -184,93 +182,13 @@ static const struct drm_encoder_funcs sun4i_hdmi_funcs = {
.destroy = drm_encoder_cleanup,
};
-static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi,
- unsigned int blk, unsigned int offset,
- u8 *buf, unsigned int count)
-{
- unsigned long reg;
- int i;
-
- reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
- reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
- writel(reg | SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ,
- hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-
- writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) |
- SUN4I_HDMI_DDC_ADDR_EDDC(DDC_SEGMENT_ADDR << 1) |
- SUN4I_HDMI_DDC_ADDR_OFFSET(offset) |
- SUN4I_HDMI_DDC_ADDR_SLAVE(DDC_ADDR),
- hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
-
- reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
- writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
- hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
-
- writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
- writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ,
- hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
-
- reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
- writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
- hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-
- if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
- !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
- 100, 100000))
- return -EIO;
-
- for (i = 0; i < count; i++)
- buf[i] = readb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG);
-
- return 0;
-}
-
-static int sun4i_hdmi_read_edid_block(void *data, u8 *buf, unsigned int blk,
- size_t length)
-{
- struct sun4i_hdmi *hdmi = data;
- int retry = 2, i;
-
- do {
- for (i = 0; i < length; i += SUN4I_HDMI_DDC_FIFO_SIZE) {
- unsigned char offset = blk * EDID_LENGTH + i;
- unsigned int count = min((unsigned int)SUN4I_HDMI_DDC_FIFO_SIZE,
- length - i);
- int ret;
-
- ret = sun4i_hdmi_read_sub_block(hdmi, blk, offset,
- buf + i, count);
- if (ret)
- return ret;
- }
- } while (!drm_edid_block_valid(buf, blk, true, NULL) && (retry--));
-
- return 0;
-}
-
static int sun4i_hdmi_get_modes(struct drm_connector *connector)
{
struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
- unsigned long reg;
struct edid *edid;
int ret;
- /* Reset i2c controller */
- writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET,
- hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
- if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
- !(reg & SUN4I_HDMI_DDC_CTRL_RESET),
- 100, 2000))
- return -EIO;
-
- writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE |
- SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE,
- hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG);
-
- clk_prepare_enable(hdmi->ddc_clk);
- clk_set_rate(hdmi->ddc_clk, 100000);
-
- edid = drm_do_get_edid(connector, sun4i_hdmi_read_edid_block, hdmi);
+ edid = drm_get_edid(connector, hdmi->i2c);
if (!edid)
return 0;
@@ -282,8 +200,6 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
ret = drm_add_edid_modes(connector, edid);
kfree(edid);
- clk_disable_unprepare(hdmi->ddc_clk);
-
return ret;
}
@@ -407,9 +323,9 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
SUN4I_HDMI_PLL_CTRL_PLL_EN;
writel(reg, hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
- ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
+ ret = sun4i_hdmi_i2c_create(dev, hdmi);
if (ret) {
- dev_err(dev, "Couldn't create the DDC clock\n");
+ dev_err(dev, "Couldn't create the HDMI I2C adapter\n");
return ret;
}
@@ -422,13 +338,15 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
NULL);
if (ret) {
dev_err(dev, "Couldn't initialise the HDMI encoder\n");
- return ret;
+ goto err_del_i2c_adapter;
}
hdmi->encoder.possible_crtcs = drm_of_find_possible_crtcs(drm,
dev->of_node);
- if (!hdmi->encoder.possible_crtcs)
- return -EPROBE_DEFER;
+ if (!hdmi->encoder.possible_crtcs) {
+ ret = -EPROBE_DEFER;
+ goto err_del_i2c_adapter;
+ }
drm_connector_helper_add(&hdmi->connector,
&sun4i_hdmi_connector_helper_funcs);
@@ -451,6 +369,8 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
err_cleanup_connector:
drm_encoder_cleanup(&hdmi->encoder);
+err_del_i2c_adapter:
+ i2c_del_adapter(hdmi->i2c);
return ret;
}
@@ -461,6 +381,7 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
drm_connector_cleanup(&hdmi->connector);
drm_encoder_cleanup(&hdmi->encoder);
+ i2c_del_adapter(hdmi->i2c);
}
static const struct component_ops sun4i_hdmi_ops = {
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
new file mode 100644
index 000000000000..2afbce02532e
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
@@ -0,0 +1,226 @@
+/*
+ * Copyright (C) 2016 Maxime Ripard <maxime.ripard@free-electrons.com>
+ * Copyright (C) 2017 Jonathan Liu <net147@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/i2c.h>
+#include <linux/iopoll.h>
+#include <linux/ktime.h>
+
+#include "sun4i_hdmi.h"
+
+#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \
+ SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \
+ SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \
+ SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \
+ SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \
+ SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \
+ SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \
+)
+
+static bool is_err_status(u32 int_status)
+{
+ return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK);
+}
+
+static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status,
+ u32 flag)
+{
+ *fifo_status = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
+ return !(*fifo_status & flag);
+}
+
+static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool read)
+{
+ /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */
+ unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
+ clk_get_rate(hdmi->ddc_clk)) * 9;
+ u32 int_status;
+ u32 fifo_status;
+ /* Read needs empty flag unset, write needs full flag unset */
+ u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
+ SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
+ int ret;
+
+ /* Wait until error or FIFO ready */
+ ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
+ int_status,
+ is_err_status(int_status) ||
+ is_fifo_flag_unset(hdmi, &fifo_status, flag),
+ min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
+ 100000);
+
+ if (is_err_status(int_status))
+ return -EIO;
+ if (ret)
+ return -ETIMEDOUT;
+
+ /* Read FIFO level */
+ int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
+
+ /* Limit transfer length using FIFO level to avoid underflow/overflow */
+ len = min(len, read ? level : (SUN4I_HDMI_DDC_FIFO_SIZE - level));
+
+ if (read)
+ readsb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
+ else
+ writesb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
+
+ return len;
+}
+
+static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg)
+{
+ int i, len;
+ u32 reg;
+
+ /* Clear errors */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
+ reg &= ~SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK;
+ writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
+
+ /* Set FIFO direction */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+ reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
+ reg |= (msg->flags & I2C_M_RD) ?
+ SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ :
+ SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE;
+ writel(reg, hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+
+ /* Set I2C address */
+ writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->addr),
+ hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
+
+ /* Clear FIFO */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
+ writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
+ hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
+ if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG,
+ reg,
+ !(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR),
+ 100, 100000))
+ return -EIO;
+
+ /* Set transfer length */
+ writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
+
+ /* Set command */
+ writel(msg->flags & I2C_M_RD ?
+ SUN4I_HDMI_DDC_CMD_IMPLICIT_READ :
+ SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE,
+ hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
+
+ /* Start command */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+ writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
+ hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+
+ /* Transfer bytes */
+ for (i = 0; i < msg->len; i += len) {
+ len = fifo_transfer(hdmi, msg->buf + i, msg->len - i,
+ msg->flags & I2C_M_RD);
+ if (len <= 0)
+ return len;
+ }
+
+ /* Wait for command to finish */
+ if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG,
+ reg,
+ !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
+ 100, 100000))
+ return -EIO;
+
+ /* Check for errors */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
+ if ((reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) ||
+ !(reg & SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE)) {
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int sun4i_hdmi_i2c_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ struct sun4i_hdmi *hdmi = i2c_get_adapdata(adap);
+ u32 reg;
+ int err, i, ret = num;
+
+ for (i = 0; i < num; i++) {
+ if (!msgs[i].len)
+ return -EINVAL;
+ if (msgs[i].len > SUN4I_HDMI_DDC_BYTE_COUNT_MAX)
+ return -EINVAL;
+ }
+
+ /* Reset I2C controller */
+ writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET,
+ hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+ if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
+ !(reg & SUN4I_HDMI_DDC_CTRL_RESET),
+ 100, 2000))
+ return -EIO;
+
+ writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE |
+ SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE,
+ hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG);
+
+ clk_prepare_enable(hdmi->ddc_clk);
+ clk_set_rate(hdmi->ddc_clk, 100000);
+
+ for (i = 0; i < num; i++) {
+ err = xfer_msg(hdmi, &msgs[i]);
+ if (err) {
+ ret = err;
+ break;
+ }
+ }
+
+ clk_disable_unprepare(hdmi->ddc_clk);
+ return ret;
+}
+
+static u32 sun4i_hdmi_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm sun4i_hdmi_i2c_algorithm = {
+ .master_xfer = sun4i_hdmi_i2c_xfer,
+ .functionality = sun4i_hdmi_i2c_func,
+};
+
+int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi)
+{
+ struct i2c_adapter *adap;
+ int ret = 0;
+
+ ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
+ if (ret)
+ return ret;
+
+ adap = devm_kzalloc(dev, sizeof(*adap), GFP_KERNEL);
+ if (!adap)
+ return -ENOMEM;
+
+ adap->owner = THIS_MODULE;
+ adap->class = I2C_CLASS_DDC;
+ adap->algo = &sun4i_hdmi_i2c_algorithm;
+ strlcpy(adap->name, "sun4i_hdmi_i2c adapter", sizeof(adap->name));
+ i2c_set_adapdata(adap, hdmi);
+
+ ret = i2c_add_adapter(adap);
+ if (ret)
+ return ret;
+
+ hdmi->i2c = adap;
+
+ return ret;
+}
--
2.13.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-27 14:36 ` Jonathan Liu
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-27 14:36 UTC (permalink / raw)
To: Maxime Ripard, David Airlie, Chen-Yu Tsai
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Jonathan Liu
The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
"As in the general case the DDC bus is accessible by the kernel at the I2C
level, drivers must make all reasonable efforts to expose it as an I2C
adapter and use drm_get_edid() instead of abusing this function."
Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
for purposes other than reading the EDID such as modifying the EDID or
using the HDMI DDC pins as an I2C bus through the I2C dev interface from
userspace (e.g. i2c-tools).
Implement this for A10s.
Signed-off-by: Jonathan Liu <net147-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
Changes for v5:
- Use devm_kzalloc instead of devm_kmemdup and remove const struct i2c_adapter
- Rework to use readl_poll_timeout for checking FIFO status
Changes for v4:
- Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c
- Clean up indentation in sun4i_hdmi.h
- Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX
and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the
value to use the GENMASK macro to make it clear that it is derived from
the width of the field in the register
- Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be
SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW
- Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register
- Change struct i2c_adapter to be const by using devm_kmemdup on creation
- Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an
I2C message
- Instead of waiting for 1-2 bytes to transfer, wait for the time it would
take for remaining bytes to transfer (limited by FIFO size)
- Add additional comments
Changes for v3:
- Explain why drm_do_get_edid should be used and why it's better to expose it
as an I2C adapter in commit message
- Reorder bit defines in descending order for consistency
- Keep old unused macros instead of removing them
- The v2 algorithm split large transfers into 16 byte transfers but this may
cause a large single write to be treated as multiple writes causing data
corruption. The algorithm has been reworked to not split larger transfers
and make more use of the FIFO to avoid this.
- Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
sun4i_hdmi_i2c.c
- Reformatted code
- Instead of masking bits that we don't want to check for errors, explicitly
check the error bits
- Clear error bits at start of transfer in case of error from previous transfer
- Poll for completion of FIFO clear after setting FIFO clear bit
Changes for v2:
- Rebased against Maxime's sunxi-drm/for-next branch
- Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
any of the calls after the I2C adapter is created fails
- Remove unnecessary includes in sun4i_hdmi_i2c.c
drivers/gpu/drm/sun4i/Makefile | 1 +
drivers/gpu/drm/sun4i/sun4i_hdmi.h | 23 ++++
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++-------------
drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 226 +++++++++++++++++++++++++++++++++
4 files changed, 261 insertions(+), 90 deletions(-)
create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
sun4i-drm-y += sun4i_framebuffer.o
sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2f2f2ff1ea63..eaff92fe236c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -96,6 +96,7 @@
#define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31)
#define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30)
#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8)
+#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8)
#define SUN4I_HDMI_DDC_CTRL_RESET BIT(0)
@@ -105,14 +106,33 @@
#define SUN4I_HDMI_DDC_ADDR_OFFSET(off) (((off) & 0xff) << 8)
#define SUN4I_HDMI_DDC_ADDR_SLAVE(addr) ((addr) & 0xff)
+#define SUN4I_HDMI_DDC_INT_STATUS_REG 0x50c
+#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION BIT(7)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW BIT(6)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW BIT(5)
+#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4)
+#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR BIT(3)
+#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR BIT(2)
+#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR BIT(1)
+#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE BIT(0)
+
#define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510
#define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31)
+#define SUN4I_HDMI_DDC_FIFO_STATUS_REG 0x514
+#define SUN4I_HDMI_DDC_FIFO_STATUS_FULL BIT(6)
+#define SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY BIT(5)
+#define SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK GENMASK(4, 0)
+
#define SUN4I_HDMI_DDC_FIFO_DATA_REG 0x518
+
#define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c
+#define SUN4I_HDMI_DDC_BYTE_COUNT_MAX GENMASK(9, 0)
#define SUN4I_HDMI_DDC_CMD_REG 0x520
#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ 6
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE 3
#define SUN4I_HDMI_DDC_CLK_REG 0x528
#define SUN4I_HDMI_DDC_CLK_M(m) (((m) & 0x7) << 3)
@@ -146,6 +166,8 @@ struct sun4i_hdmi {
struct clk *ddc_clk;
struct clk *tmds_clk;
+ struct i2c_adapter *i2c;
+
struct sun4i_drv *drv;
bool hdmi_monitor;
@@ -153,5 +175,6 @@ struct sun4i_hdmi {
int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
+int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi);
#endif /* _SUN4I_HDMI_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index d3398f6250ef..b74607feb35c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -29,8 +29,6 @@
#include "sun4i_hdmi.h"
#include "sun4i_tcon.h"
-#define DDC_SEGMENT_ADDR 0x30
-
static inline struct sun4i_hdmi *
drm_encoder_to_sun4i_hdmi(struct drm_encoder *encoder)
{
@@ -184,93 +182,13 @@ static const struct drm_encoder_funcs sun4i_hdmi_funcs = {
.destroy = drm_encoder_cleanup,
};
-static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi,
- unsigned int blk, unsigned int offset,
- u8 *buf, unsigned int count)
-{
- unsigned long reg;
- int i;
-
- reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
- reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
- writel(reg | SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ,
- hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-
- writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) |
- SUN4I_HDMI_DDC_ADDR_EDDC(DDC_SEGMENT_ADDR << 1) |
- SUN4I_HDMI_DDC_ADDR_OFFSET(offset) |
- SUN4I_HDMI_DDC_ADDR_SLAVE(DDC_ADDR),
- hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
-
- reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
- writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
- hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
-
- writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
- writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ,
- hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
-
- reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
- writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
- hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-
- if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
- !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
- 100, 100000))
- return -EIO;
-
- for (i = 0; i < count; i++)
- buf[i] = readb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG);
-
- return 0;
-}
-
-static int sun4i_hdmi_read_edid_block(void *data, u8 *buf, unsigned int blk,
- size_t length)
-{
- struct sun4i_hdmi *hdmi = data;
- int retry = 2, i;
-
- do {
- for (i = 0; i < length; i += SUN4I_HDMI_DDC_FIFO_SIZE) {
- unsigned char offset = blk * EDID_LENGTH + i;
- unsigned int count = min((unsigned int)SUN4I_HDMI_DDC_FIFO_SIZE,
- length - i);
- int ret;
-
- ret = sun4i_hdmi_read_sub_block(hdmi, blk, offset,
- buf + i, count);
- if (ret)
- return ret;
- }
- } while (!drm_edid_block_valid(buf, blk, true, NULL) && (retry--));
-
- return 0;
-}
-
static int sun4i_hdmi_get_modes(struct drm_connector *connector)
{
struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
- unsigned long reg;
struct edid *edid;
int ret;
- /* Reset i2c controller */
- writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET,
- hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
- if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
- !(reg & SUN4I_HDMI_DDC_CTRL_RESET),
- 100, 2000))
- return -EIO;
-
- writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE |
- SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE,
- hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG);
-
- clk_prepare_enable(hdmi->ddc_clk);
- clk_set_rate(hdmi->ddc_clk, 100000);
-
- edid = drm_do_get_edid(connector, sun4i_hdmi_read_edid_block, hdmi);
+ edid = drm_get_edid(connector, hdmi->i2c);
if (!edid)
return 0;
@@ -282,8 +200,6 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
ret = drm_add_edid_modes(connector, edid);
kfree(edid);
- clk_disable_unprepare(hdmi->ddc_clk);
-
return ret;
}
@@ -407,9 +323,9 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
SUN4I_HDMI_PLL_CTRL_PLL_EN;
writel(reg, hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
- ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
+ ret = sun4i_hdmi_i2c_create(dev, hdmi);
if (ret) {
- dev_err(dev, "Couldn't create the DDC clock\n");
+ dev_err(dev, "Couldn't create the HDMI I2C adapter\n");
return ret;
}
@@ -422,13 +338,15 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
NULL);
if (ret) {
dev_err(dev, "Couldn't initialise the HDMI encoder\n");
- return ret;
+ goto err_del_i2c_adapter;
}
hdmi->encoder.possible_crtcs = drm_of_find_possible_crtcs(drm,
dev->of_node);
- if (!hdmi->encoder.possible_crtcs)
- return -EPROBE_DEFER;
+ if (!hdmi->encoder.possible_crtcs) {
+ ret = -EPROBE_DEFER;
+ goto err_del_i2c_adapter;
+ }
drm_connector_helper_add(&hdmi->connector,
&sun4i_hdmi_connector_helper_funcs);
@@ -451,6 +369,8 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
err_cleanup_connector:
drm_encoder_cleanup(&hdmi->encoder);
+err_del_i2c_adapter:
+ i2c_del_adapter(hdmi->i2c);
return ret;
}
@@ -461,6 +381,7 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
drm_connector_cleanup(&hdmi->connector);
drm_encoder_cleanup(&hdmi->encoder);
+ i2c_del_adapter(hdmi->i2c);
}
static const struct component_ops sun4i_hdmi_ops = {
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
new file mode 100644
index 000000000000..2afbce02532e
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
@@ -0,0 +1,226 @@
+/*
+ * Copyright (C) 2016 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ * Copyright (C) 2017 Jonathan Liu <net147-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/i2c.h>
+#include <linux/iopoll.h>
+#include <linux/ktime.h>
+
+#include "sun4i_hdmi.h"
+
+#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \
+ SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \
+ SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \
+ SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \
+ SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \
+ SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \
+ SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \
+)
+
+static bool is_err_status(u32 int_status)
+{
+ return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK);
+}
+
+static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status,
+ u32 flag)
+{
+ *fifo_status = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
+ return !(*fifo_status & flag);
+}
+
+static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool read)
+{
+ /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */
+ unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
+ clk_get_rate(hdmi->ddc_clk)) * 9;
+ u32 int_status;
+ u32 fifo_status;
+ /* Read needs empty flag unset, write needs full flag unset */
+ u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
+ SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
+ int ret;
+
+ /* Wait until error or FIFO ready */
+ ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
+ int_status,
+ is_err_status(int_status) ||
+ is_fifo_flag_unset(hdmi, &fifo_status, flag),
+ min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
+ 100000);
+
+ if (is_err_status(int_status))
+ return -EIO;
+ if (ret)
+ return -ETIMEDOUT;
+
+ /* Read FIFO level */
+ int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
+
+ /* Limit transfer length using FIFO level to avoid underflow/overflow */
+ len = min(len, read ? level : (SUN4I_HDMI_DDC_FIFO_SIZE - level));
+
+ if (read)
+ readsb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
+ else
+ writesb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
+
+ return len;
+}
+
+static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg)
+{
+ int i, len;
+ u32 reg;
+
+ /* Clear errors */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
+ reg &= ~SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK;
+ writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
+
+ /* Set FIFO direction */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+ reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
+ reg |= (msg->flags & I2C_M_RD) ?
+ SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ :
+ SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE;
+ writel(reg, hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+
+ /* Set I2C address */
+ writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->addr),
+ hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
+
+ /* Clear FIFO */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
+ writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
+ hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
+ if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG,
+ reg,
+ !(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR),
+ 100, 100000))
+ return -EIO;
+
+ /* Set transfer length */
+ writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
+
+ /* Set command */
+ writel(msg->flags & I2C_M_RD ?
+ SUN4I_HDMI_DDC_CMD_IMPLICIT_READ :
+ SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE,
+ hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
+
+ /* Start command */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+ writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
+ hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+
+ /* Transfer bytes */
+ for (i = 0; i < msg->len; i += len) {
+ len = fifo_transfer(hdmi, msg->buf + i, msg->len - i,
+ msg->flags & I2C_M_RD);
+ if (len <= 0)
+ return len;
+ }
+
+ /* Wait for command to finish */
+ if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG,
+ reg,
+ !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
+ 100, 100000))
+ return -EIO;
+
+ /* Check for errors */
+ reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
+ if ((reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) ||
+ !(reg & SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE)) {
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int sun4i_hdmi_i2c_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ struct sun4i_hdmi *hdmi = i2c_get_adapdata(adap);
+ u32 reg;
+ int err, i, ret = num;
+
+ for (i = 0; i < num; i++) {
+ if (!msgs[i].len)
+ return -EINVAL;
+ if (msgs[i].len > SUN4I_HDMI_DDC_BYTE_COUNT_MAX)
+ return -EINVAL;
+ }
+
+ /* Reset I2C controller */
+ writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET,
+ hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+ if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
+ !(reg & SUN4I_HDMI_DDC_CTRL_RESET),
+ 100, 2000))
+ return -EIO;
+
+ writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE |
+ SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE,
+ hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG);
+
+ clk_prepare_enable(hdmi->ddc_clk);
+ clk_set_rate(hdmi->ddc_clk, 100000);
+
+ for (i = 0; i < num; i++) {
+ err = xfer_msg(hdmi, &msgs[i]);
+ if (err) {
+ ret = err;
+ break;
+ }
+ }
+
+ clk_disable_unprepare(hdmi->ddc_clk);
+ return ret;
+}
+
+static u32 sun4i_hdmi_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm sun4i_hdmi_i2c_algorithm = {
+ .master_xfer = sun4i_hdmi_i2c_xfer,
+ .functionality = sun4i_hdmi_i2c_func,
+};
+
+int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi)
+{
+ struct i2c_adapter *adap;
+ int ret = 0;
+
+ ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
+ if (ret)
+ return ret;
+
+ adap = devm_kzalloc(dev, sizeof(*adap), GFP_KERNEL);
+ if (!adap)
+ return -ENOMEM;
+
+ adap->owner = THIS_MODULE;
+ adap->class = I2C_CLASS_DDC;
+ adap->algo = &sun4i_hdmi_i2c_algorithm;
+ strlcpy(adap->name, "sun4i_hdmi_i2c adapter", sizeof(adap->name));
+ i2c_set_adapdata(adap, hdmi);
+
+ ret = i2c_add_adapter(adap);
+ if (ret)
+ return ret;
+
+ hdmi->i2c = adap;
+
+ return ret;
+}
--
2.13.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
2017-06-27 14:36 ` Jonathan Liu
(?)
@ 2017-06-28 9:20 ` Maxime Ripard
-1 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-28 9:20 UTC (permalink / raw)
To: Jonathan Liu
Cc: David Airlie, Chen-Yu Tsai, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 2536 bytes --]
On Wed, Jun 28, 2017 at 12:36:52AM +1000, Jonathan Liu wrote:
> +#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \
> + SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \
> + SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \
> + SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \
> + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \
> + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \
> + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \
> +)
> +
> +static bool is_err_status(u32 int_status)
> +{
> + return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK);
> +}
> +
> +static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status,
> + u32 flag)
> +{
> + *fifo_status = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
> + return !(*fifo_status & flag);
> +}
> +
> +static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool read)
> +{
> + /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */
> + unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
> + clk_get_rate(hdmi->ddc_clk)) * 9;
There's no real need for it to be dynamic. The clock rate will not
change, and the order of magnitude is roughly 100us, so let's just use
that (and make a comment).
> + u32 int_status;
> + u32 fifo_status;
> + /* Read needs empty flag unset, write needs full flag unset */
> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> + int ret;
> +
> + /* Wait until error or FIFO ready */
> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> + int_status,
> + is_err_status(int_status) ||
> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> + 100000);
> +
> + if (is_err_status(int_status))
> + return -EIO;
> + if (ret)
> + return -ETIMEDOUT;
Why not just have
ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
!(reg & flag), 100, 100000);
if (ret < 0)
if (is_err_status())
return -EIO;
return ret;
> +
> + /* Read FIFO level */
> + int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
and explicitly read the fifo status here. That will make you remove
that function that does two things while claiming that it does only
one, and it will be more obvious.
You can also just use reg at this point, instead of reading it once
again.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-28 9:20 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-28 9:20 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 28, 2017 at 12:36:52AM +1000, Jonathan Liu wrote:
> +#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \
> + SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \
> + SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \
> + SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \
> + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \
> + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \
> + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \
> +)
> +
> +static bool is_err_status(u32 int_status)
> +{
> + return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK);
> +}
> +
> +static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status,
> + u32 flag)
> +{
> + *fifo_status = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
> + return !(*fifo_status & flag);
> +}
> +
> +static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool read)
> +{
> + /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */
> + unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
> + clk_get_rate(hdmi->ddc_clk)) * 9;
There's no real need for it to be dynamic. The clock rate will not
change, and the order of magnitude is roughly 100us, so let's just use
that (and make a comment).
> + u32 int_status;
> + u32 fifo_status;
> + /* Read needs empty flag unset, write needs full flag unset */
> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> + int ret;
> +
> + /* Wait until error or FIFO ready */
> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> + int_status,
> + is_err_status(int_status) ||
> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> + 100000);
> +
> + if (is_err_status(int_status))
> + return -EIO;
> + if (ret)
> + return -ETIMEDOUT;
Why not just have
ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
!(reg & flag), 100, 100000);
if (ret < 0)
if (is_err_status())
return -EIO;
return ret;
> +
> + /* Read FIFO level */
> + int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
and explicitly read the fifo status here. That will make you remove
that function that does two things while claiming that it does only
one, and it will be more obvious.
You can also just use reg at this point, instead of reading it once
again.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170628/52598f2f/attachment.sig>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-28 9:20 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-28 9:20 UTC (permalink / raw)
To: Jonathan Liu
Cc: David Airlie, Chen-Yu Tsai, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]
On Wed, Jun 28, 2017 at 12:36:52AM +1000, Jonathan Liu wrote:
> +#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \
> + SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \
> + SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \
> + SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \
> + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \
> + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \
> + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \
> +)
> +
> +static bool is_err_status(u32 int_status)
> +{
> + return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK);
> +}
> +
> +static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status,
> + u32 flag)
> +{
> + *fifo_status = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
> + return !(*fifo_status & flag);
> +}
> +
> +static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool read)
> +{
> + /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */
> + unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
> + clk_get_rate(hdmi->ddc_clk)) * 9;
There's no real need for it to be dynamic. The clock rate will not
change, and the order of magnitude is roughly 100us, so let's just use
that (and make a comment).
> + u32 int_status;
> + u32 fifo_status;
> + /* Read needs empty flag unset, write needs full flag unset */
> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> + int ret;
> +
> + /* Wait until error or FIFO ready */
> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> + int_status,
> + is_err_status(int_status) ||
> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> + 100000);
> +
> + if (is_err_status(int_status))
> + return -EIO;
> + if (ret)
> + return -ETIMEDOUT;
Why not just have
ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
!(reg & flag), 100, 100000);
if (ret < 0)
if (is_err_status())
return -EIO;
return ret;
> +
> + /* Read FIFO level */
> + int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
and explicitly read the fifo status here. That will make you remove
that function that does two things while claiming that it does only
one, and it will be more obvious.
You can also just use reg at this point, instead of reading it once
again.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
2017-06-28 9:20 ` Maxime Ripard
(?)
@ 2017-06-28 10:39 ` Jonathan Liu
-1 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-28 10:39 UTC (permalink / raw)
To: Maxime Ripard
Cc: David Airlie, Chen-Yu Tsai, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
Hi Maxime,
On 28 June 2017 at 19:20, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Jun 28, 2017 at 12:36:52AM +1000, Jonathan Liu wrote:
>> +#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \
>> + SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \
>> + SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \
>> + SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \
>> + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \
>> + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \
>> + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \
>> +)
>> +
>> +static bool is_err_status(u32 int_status)
>> +{
>> + return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK);
>> +}
>> +
>> +static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status,
>> + u32 flag)
>> +{
>> + *fifo_status = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
>> + return !(*fifo_status & flag);
>> +}
>> +
>> +static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool read)
>> +{
>> + /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */
>> + unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
>> + clk_get_rate(hdmi->ddc_clk)) * 9;
>
> There's no real need for it to be dynamic. The clock rate will not
> change, and the order of magnitude is roughly 100us, so let's just use
> that (and make a comment).
>
Ok.
>> + u32 int_status;
>> + u32 fifo_status;
>> + /* Read needs empty flag unset, write needs full flag unset */
>> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>> + int ret;
>> +
>> + /* Wait until error or FIFO ready */
>> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>> + int_status,
>> + is_err_status(int_status) ||
>> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>> + 100000);
>> +
>> + if (is_err_status(int_status))
>> + return -EIO;
>> + if (ret)
>> + return -ETIMEDOUT;
>
> Why not just have
> ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> !(reg & flag), 100, 100000);
>
> if (ret < 0)
> if (is_err_status())
> return -EIO;
> return ret;
>
>
If I check error status after readl_poll_timeout and there is an error
(e.g. the I2C address does not have a corresponding device connected
or nothing connected to HDMI port) it will keep checking the fifo
status even though error bit is set in the int status and then timeout
after 100 ms. If it checks the int status register at the same time,
it will error after 100 nanoseconds. I don't want to introduce
unnecessary delays considering part of the reason for adding this
driver to make it more usable for non-standard use cases.
>> +
>> + /* Read FIFO level */
>> + int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
>
> and explicitly read the fifo status here. That will make you remove
> that function that does two things while claiming that it does only
> one, and it will be more obvious.
>
I will fix the is_fifo_flag_unset function so it only does one thing.
> You can also just use reg at this point, instead of reading it once
> again.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-28 10:39 ` Jonathan Liu
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-28 10:39 UTC (permalink / raw)
To: linux-arm-kernel
Hi Maxime,
On 28 June 2017 at 19:20, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Jun 28, 2017 at 12:36:52AM +1000, Jonathan Liu wrote:
>> +#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \
>> + SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \
>> + SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \
>> + SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \
>> + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \
>> + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \
>> + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \
>> +)
>> +
>> +static bool is_err_status(u32 int_status)
>> +{
>> + return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK);
>> +}
>> +
>> +static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status,
>> + u32 flag)
>> +{
>> + *fifo_status = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
>> + return !(*fifo_status & flag);
>> +}
>> +
>> +static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool read)
>> +{
>> + /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */
>> + unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
>> + clk_get_rate(hdmi->ddc_clk)) * 9;
>
> There's no real need for it to be dynamic. The clock rate will not
> change, and the order of magnitude is roughly 100us, so let's just use
> that (and make a comment).
>
Ok.
>> + u32 int_status;
>> + u32 fifo_status;
>> + /* Read needs empty flag unset, write needs full flag unset */
>> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>> + int ret;
>> +
>> + /* Wait until error or FIFO ready */
>> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>> + int_status,
>> + is_err_status(int_status) ||
>> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>> + 100000);
>> +
>> + if (is_err_status(int_status))
>> + return -EIO;
>> + if (ret)
>> + return -ETIMEDOUT;
>
> Why not just have
> ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> !(reg & flag), 100, 100000);
>
> if (ret < 0)
> if (is_err_status())
> return -EIO;
> return ret;
>
>
If I check error status after readl_poll_timeout and there is an error
(e.g. the I2C address does not have a corresponding device connected
or nothing connected to HDMI port) it will keep checking the fifo
status even though error bit is set in the int status and then timeout
after 100 ms. If it checks the int status register at the same time,
it will error after 100 nanoseconds. I don't want to introduce
unnecessary delays considering part of the reason for adding this
driver to make it more usable for non-standard use cases.
>> +
>> + /* Read FIFO level */
>> + int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
>
> and explicitly read the fifo status here. That will make you remove
> that function that does two things while claiming that it does only
> one, and it will be more obvious.
>
I will fix the is_fifo_flag_unset function so it only does one thing.
> You can also just use reg at this point, instead of reading it once
> again.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-28 10:39 ` Jonathan Liu
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-28 10:39 UTC (permalink / raw)
To: Maxime Ripard
Cc: David Airlie, Chen-Yu Tsai, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
Hi Maxime,
On 28 June 2017 at 19:20, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Wed, Jun 28, 2017 at 12:36:52AM +1000, Jonathan Liu wrote:
>> +#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \
>> + SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \
>> + SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \
>> + SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \
>> + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \
>> + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \
>> + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \
>> +)
>> +
>> +static bool is_err_status(u32 int_status)
>> +{
>> + return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK);
>> +}
>> +
>> +static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status,
>> + u32 flag)
>> +{
>> + *fifo_status = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
>> + return !(*fifo_status & flag);
>> +}
>> +
>> +static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool read)
>> +{
>> + /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */
>> + unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
>> + clk_get_rate(hdmi->ddc_clk)) * 9;
>
> There's no real need for it to be dynamic. The clock rate will not
> change, and the order of magnitude is roughly 100us, so let's just use
> that (and make a comment).
>
Ok.
>> + u32 int_status;
>> + u32 fifo_status;
>> + /* Read needs empty flag unset, write needs full flag unset */
>> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>> + int ret;
>> +
>> + /* Wait until error or FIFO ready */
>> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>> + int_status,
>> + is_err_status(int_status) ||
>> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>> + 100000);
>> +
>> + if (is_err_status(int_status))
>> + return -EIO;
>> + if (ret)
>> + return -ETIMEDOUT;
>
> Why not just have
> ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> !(reg & flag), 100, 100000);
>
> if (ret < 0)
> if (is_err_status())
> return -EIO;
> return ret;
>
>
If I check error status after readl_poll_timeout and there is an error
(e.g. the I2C address does not have a corresponding device connected
or nothing connected to HDMI port) it will keep checking the fifo
status even though error bit is set in the int status and then timeout
after 100 ms. If it checks the int status register at the same time,
it will error after 100 nanoseconds. I don't want to introduce
unnecessary delays considering part of the reason for adding this
driver to make it more usable for non-standard use cases.
>> +
>> + /* Read FIFO level */
>> + int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
>
> and explicitly read the fifo status here. That will make you remove
> that function that does two things while claiming that it does only
> one, and it will be more obvious.
>
I will fix the is_fifo_flag_unset function so it only does one thing.
> You can also just use reg at this point, instead of reading it once
> again.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
2017-06-27 14:36 ` Jonathan Liu
(?)
@ 2017-06-28 22:06 ` kbuild test robot
-1 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2017-06-28 22:06 UTC (permalink / raw)
To: Jonathan Liu
Cc: kbuild-all, Maxime Ripard, David Airlie, Chen-Yu Tsai,
linux-kernel, dri-devel, linux-arm-kernel, linux-sunxi,
Jonathan Liu
[-- Attachment #1: Type: text/plain, Size: 2167 bytes --]
Hi Jonathan,
[auto build test WARNING on next-20170627]
[cannot apply to v4.12-rc7 v4.12-rc6 v4.12-rc5 v4.12-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jonathan-Liu/drm-sun4i-hdmi-Implement-I2C-adapter-for-A10s-DDC-bus/20170629-001335
config: arm-sunxi_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All warnings (new ones prefixed by >>):
drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c: In function 'fifo_transfer':
>> drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c:65:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
^~~
vim +65 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
49 int ret;
50
51 /* Wait until error or FIFO ready */
52 ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
53 int_status,
54 is_err_status(int_status) ||
55 is_fifo_flag_unset(hdmi, &fifo_status, flag),
56 min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
57 100000);
58
59 if (is_err_status(int_status))
60 return -EIO;
61 if (ret)
62 return -ETIMEDOUT;
63
64 /* Read FIFO level */
> 65 int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
66
67 /* Limit transfer length using FIFO level to avoid underflow/overflow */
68 len = min(len, read ? level : (SUN4I_HDMI_DDC_FIFO_SIZE - level));
69
70 if (read)
71 readsb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
72 else
73 writesb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21231 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-28 22:06 ` kbuild test robot
0 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2017-06-28 22:06 UTC (permalink / raw)
To: linux-arm-kernel
Hi Jonathan,
[auto build test WARNING on next-20170627]
[cannot apply to v4.12-rc7 v4.12-rc6 v4.12-rc5 v4.12-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jonathan-Liu/drm-sun4i-hdmi-Implement-I2C-adapter-for-A10s-DDC-bus/20170629-001335
config: arm-sunxi_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All warnings (new ones prefixed by >>):
drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c: In function 'fifo_transfer':
>> drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c:65:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
^~~
vim +65 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
49 int ret;
50
51 /* Wait until error or FIFO ready */
52 ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
53 int_status,
54 is_err_status(int_status) ||
55 is_fifo_flag_unset(hdmi, &fifo_status, flag),
56 min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
57 100000);
58
59 if (is_err_status(int_status))
60 return -EIO;
61 if (ret)
62 return -ETIMEDOUT;
63
64 /* Read FIFO level */
> 65 int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
66
67 /* Limit transfer length using FIFO level to avoid underflow/overflow */
68 len = min(len, read ? level : (SUN4I_HDMI_DDC_FIFO_SIZE - level));
69
70 if (read)
71 readsb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
72 else
73 writesb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 21231 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170629/d6760e51/attachment-0001.gz>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-28 22:06 ` kbuild test robot
0 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2017-06-28 22:06 UTC (permalink / raw)
Cc: kbuild-all-JC7UmRfGjtg, Maxime Ripard, David Airlie,
Chen-Yu Tsai, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Jonathan Liu
[-- Attachment #1: Type: text/plain, Size: 2484 bytes --]
Hi Jonathan,
[auto build test WARNING on next-20170627]
[cannot apply to v4.12-rc7 v4.12-rc6 v4.12-rc5 v4.12-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jonathan-Liu/drm-sun4i-hdmi-Implement-I2C-adapter-for-A10s-DDC-bus/20170629-001335
config: arm-sunxi_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All warnings (new ones prefixed by >>):
drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c: In function 'fifo_transfer':
>> drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c:65:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
^~~
vim +65 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
49 int ret;
50
51 /* Wait until error or FIFO ready */
52 ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
53 int_status,
54 is_err_status(int_status) ||
55 is_fifo_flag_unset(hdmi, &fifo_status, flag),
56 min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
57 100000);
58
59 if (is_err_status(int_status))
60 return -EIO;
61 if (ret)
62 return -ETIMEDOUT;
63
64 /* Read FIFO level */
> 65 int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
66
67 /* Limit transfer length using FIFO level to avoid underflow/overflow */
68 len = min(len, read ? level : (SUN4I_HDMI_DDC_FIFO_SIZE - level));
69
70 if (read)
71 readsb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
72 else
73 writesb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG, buf, len);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21231 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
2017-06-28 10:39 ` Jonathan Liu
(?)
@ 2017-06-29 15:56 ` Maxime Ripard
-1 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-29 15:56 UTC (permalink / raw)
To: Jonathan Liu
Cc: David Airlie, Chen-Yu Tsai, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]
On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
> >> + u32 int_status;
> >> + u32 fifo_status;
> >> + /* Read needs empty flag unset, write needs full flag unset */
> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> >> + int ret;
> >> +
> >> + /* Wait until error or FIFO ready */
> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> >> + int_status,
> >> + is_err_status(int_status) ||
> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> >> + 100000);
> >> +
> >> + if (is_err_status(int_status))
> >> + return -EIO;
> >> + if (ret)
> >> + return -ETIMEDOUT;
> >
> > Why not just have
> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> > !(reg & flag), 100, 100000);
> >
> > if (ret < 0)
> > if (is_err_status())
> > return -EIO;
> > return ret;
> >
> >
>
> If I check error status after readl_poll_timeout and there is an error
> (e.g. the I2C address does not have a corresponding device connected
> or nothing connected to HDMI port) it will keep checking the fifo
> status even though error bit is set in the int status and then timeout
> after 100 ms. If it checks the int status register at the same time,
> it will error after 100 nanoseconds. I don't want to introduce
> unnecessary delays considering part of the reason for adding this
> driver to make it more usable for non-standard use cases.
Well, polling for 100ms doesn't seem great either. What was the
rationale behind that timeout?
And we can also reverse the check and look at the INT_STATUS
register. The errors will be there, and we can program the threshold
we want in both directions and use the
DDC_FIFO_Request_Interrupt_Status bit.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-29 15:56 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-29 15:56 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
> >> + u32 int_status;
> >> + u32 fifo_status;
> >> + /* Read needs empty flag unset, write needs full flag unset */
> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> >> + int ret;
> >> +
> >> + /* Wait until error or FIFO ready */
> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> >> + int_status,
> >> + is_err_status(int_status) ||
> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> >> + 100000);
> >> +
> >> + if (is_err_status(int_status))
> >> + return -EIO;
> >> + if (ret)
> >> + return -ETIMEDOUT;
> >
> > Why not just have
> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> > !(reg & flag), 100, 100000);
> >
> > if (ret < 0)
> > if (is_err_status())
> > return -EIO;
> > return ret;
> >
> >
>
> If I check error status after readl_poll_timeout and there is an error
> (e.g. the I2C address does not have a corresponding device connected
> or nothing connected to HDMI port) it will keep checking the fifo
> status even though error bit is set in the int status and then timeout
> after 100 ms. If it checks the int status register at the same time,
> it will error after 100 nanoseconds. I don't want to introduce
> unnecessary delays considering part of the reason for adding this
> driver to make it more usable for non-standard use cases.
Well, polling for 100ms doesn't seem great either. What was the
rationale behind that timeout?
And we can also reverse the check and look at the INT_STATUS
register. The errors will be there, and we can program the threshold
we want in both directions and use the
DDC_FIFO_Request_Interrupt_Status bit.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170629/6b2dc932/attachment.sig>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-29 15:56 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-29 15:56 UTC (permalink / raw)
To: Jonathan Liu
Cc: David Airlie, Chen-Yu Tsai, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]
On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
> >> + u32 int_status;
> >> + u32 fifo_status;
> >> + /* Read needs empty flag unset, write needs full flag unset */
> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> >> + int ret;
> >> +
> >> + /* Wait until error or FIFO ready */
> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> >> + int_status,
> >> + is_err_status(int_status) ||
> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> >> + 100000);
> >> +
> >> + if (is_err_status(int_status))
> >> + return -EIO;
> >> + if (ret)
> >> + return -ETIMEDOUT;
> >
> > Why not just have
> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> > !(reg & flag), 100, 100000);
> >
> > if (ret < 0)
> > if (is_err_status())
> > return -EIO;
> > return ret;
> >
> >
>
> If I check error status after readl_poll_timeout and there is an error
> (e.g. the I2C address does not have a corresponding device connected
> or nothing connected to HDMI port) it will keep checking the fifo
> status even though error bit is set in the int status and then timeout
> after 100 ms. If it checks the int status register at the same time,
> it will error after 100 nanoseconds. I don't want to introduce
> unnecessary delays considering part of the reason for adding this
> driver to make it more usable for non-standard use cases.
Well, polling for 100ms doesn't seem great either. What was the
rationale behind that timeout?
And we can also reverse the check and look at the INT_STATUS
register. The errors will be there, and we can program the threshold
we want in both directions and use the
DDC_FIFO_Request_Interrupt_Status bit.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
2017-06-29 15:56 ` Maxime Ripard
@ 2017-06-29 22:22 ` Jonathan Liu
-1 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-29 22:22 UTC (permalink / raw)
To: Maxime Ripard
Cc: David Airlie, Chen-Yu Tsai, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
Hi Maxime,
On 30 June 2017 at 01:56, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>> >> + u32 int_status;
>> >> + u32 fifo_status;
>> >> + /* Read needs empty flag unset, write needs full flag unset */
>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>> >> + int ret;
>> >> +
>> >> + /* Wait until error or FIFO ready */
>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>> >> + int_status,
>> >> + is_err_status(int_status) ||
>> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>> >> + 100000);
>> >> +
>> >> + if (is_err_status(int_status))
>> >> + return -EIO;
>> >> + if (ret)
>> >> + return -ETIMEDOUT;
>> >
>> > Why not just have
>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>> > !(reg & flag), 100, 100000);
>> >
>> > if (ret < 0)
>> > if (is_err_status())
>> > return -EIO;
>> > return ret;
>> >
>> >
>>
>> If I check error status after readl_poll_timeout and there is an error
>> (e.g. the I2C address does not have a corresponding device connected
>> or nothing connected to HDMI port) it will keep checking the fifo
>> status even though error bit is set in the int status and then timeout
>> after 100 ms. If it checks the int status register at the same time,
>> it will error after 100 nanoseconds. I don't want to introduce
>> unnecessary delays considering part of the reason for adding this
>> driver to make it more usable for non-standard use cases.
>
> Well, polling for 100ms doesn't seem great either. What was the
> rationale behind that timeout?
>
When an error occurs one of the error bits will be set in the
INT_STATUS register so this is detected very quickly if I check the
INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
case the I2C slave does clock stretching in which case the transfer
may take longer than the predicted time.
> And we can also reverse the check and look at the INT_STATUS
> register. The errors will be there, and we can program the threshold
> we want in both directions and use the
> DDC_FIFO_Request_Interrupt_Status bit.
>
I did try that when I was doing the v3 patch but I couldn't get it to
work as mentioned previously in the v3 patch discussion. I programmed
the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
register at the same time as setting FIFO_Address_Clear but the
request interrupt status bit did not get updated to the appropriate
state that is consistent with the FIFO level and the thresholds. I did
try this several times for subsequent patch versions without success.
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-29 22:22 ` Jonathan Liu
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-29 22:22 UTC (permalink / raw)
To: linux-arm-kernel
Hi Maxime,
On 30 June 2017 at 01:56, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>> >> + u32 int_status;
>> >> + u32 fifo_status;
>> >> + /* Read needs empty flag unset, write needs full flag unset */
>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>> >> + int ret;
>> >> +
>> >> + /* Wait until error or FIFO ready */
>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>> >> + int_status,
>> >> + is_err_status(int_status) ||
>> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>> >> + 100000);
>> >> +
>> >> + if (is_err_status(int_status))
>> >> + return -EIO;
>> >> + if (ret)
>> >> + return -ETIMEDOUT;
>> >
>> > Why not just have
>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>> > !(reg & flag), 100, 100000);
>> >
>> > if (ret < 0)
>> > if (is_err_status())
>> > return -EIO;
>> > return ret;
>> >
>> >
>>
>> If I check error status after readl_poll_timeout and there is an error
>> (e.g. the I2C address does not have a corresponding device connected
>> or nothing connected to HDMI port) it will keep checking the fifo
>> status even though error bit is set in the int status and then timeout
>> after 100 ms. If it checks the int status register at the same time,
>> it will error after 100 nanoseconds. I don't want to introduce
>> unnecessary delays considering part of the reason for adding this
>> driver to make it more usable for non-standard use cases.
>
> Well, polling for 100ms doesn't seem great either. What was the
> rationale behind that timeout?
>
When an error occurs one of the error bits will be set in the
INT_STATUS register so this is detected very quickly if I check the
INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
case the I2C slave does clock stretching in which case the transfer
may take longer than the predicted time.
> And we can also reverse the check and look at the INT_STATUS
> register. The errors will be there, and we can program the threshold
> we want in both directions and use the
> DDC_FIFO_Request_Interrupt_Status bit.
>
I did try that when I was doing the v3 patch but I couldn't get it to
work as mentioned previously in the v3 patch discussion. I programmed
the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
register at the same time as setting FIFO_Address_Clear but the
request interrupt status bit did not get updated to the appropriate
state that is consistent with the FIFO level and the thresholds. I did
try this several times for subsequent patch versions without success.
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
2017-06-29 22:22 ` Jonathan Liu
(?)
@ 2017-06-30 3:16 ` Chen-Yu Tsai
-1 siblings, 0 replies; 34+ messages in thread
From: Chen-Yu Tsai @ 2017-06-30 3:16 UTC (permalink / raw)
To: Jonathan Liu
Cc: Maxime Ripard, David Airlie, Chen-Yu Tsai, linux-kernel,
dri-devel, linux-arm-kernel, linux-sunxi
On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu <net147@gmail.com> wrote:
> Hi Maxime,
>
> On 30 June 2017 at 01:56, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>>> >> + u32 int_status;
>>> >> + u32 fifo_status;
>>> >> + /* Read needs empty flag unset, write needs full flag unset */
>>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>>> >> + int ret;
>>> >> +
>>> >> + /* Wait until error or FIFO ready */
>>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>>> >> + int_status,
>>> >> + is_err_status(int_status) ||
>>> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>>> >> + 100000);
>>> >> +
>>> >> + if (is_err_status(int_status))
>>> >> + return -EIO;
>>> >> + if (ret)
>>> >> + return -ETIMEDOUT;
>>> >
>>> > Why not just have
>>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>>> > !(reg & flag), 100, 100000);
>>> >
>>> > if (ret < 0)
>>> > if (is_err_status())
>>> > return -EIO;
>>> > return ret;
>>> >
>>> >
>>>
>>> If I check error status after readl_poll_timeout and there is an error
>>> (e.g. the I2C address does not have a corresponding device connected
>>> or nothing connected to HDMI port) it will keep checking the fifo
>>> status even though error bit is set in the int status and then timeout
>>> after 100 ms. If it checks the int status register at the same time,
>>> it will error after 100 nanoseconds. I don't want to introduce
>>> unnecessary delays considering part of the reason for adding this
>>> driver to make it more usable for non-standard use cases.
>>
>> Well, polling for 100ms doesn't seem great either. What was the
>> rationale behind that timeout?
>>
>
> When an error occurs one of the error bits will be set in the
> INT_STATUS register so this is detected very quickly if I check the
> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
> case the I2C slave does clock stretching in which case the transfer
> may take longer than the predicted time.
>
>> And we can also reverse the check and look at the INT_STATUS
>> register. The errors will be there, and we can program the threshold
>> we want in both directions and use the
>> DDC_FIFO_Request_Interrupt_Status bit.
>>
>
> I did try that when I was doing the v3 patch but I couldn't get it to
> work as mentioned previously in the v3 patch discussion. I programmed
> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
> register at the same time as setting FIFO_Address_Clear but the
> request interrupt status bit did not get updated to the appropriate
> state that is consistent with the FIFO level and the thresholds. I did
> try this several times for subsequent patch versions without success.
The manual says "When FIFO level is above this value in read mode, DMA
request and FIFO request interrupt are asserted if relative enable is on."
Perhaps try enabling the interrupts? But if that were the case, wouldn't
using interrupts instead of polling be better?
ChenYu
>
>> Maxime
>>
>> --
>> Maxime Ripard, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
>
> Regards,
> Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-30 3:16 ` Chen-Yu Tsai
0 siblings, 0 replies; 34+ messages in thread
From: Chen-Yu Tsai @ 2017-06-30 3:16 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu <net147@gmail.com> wrote:
> Hi Maxime,
>
> On 30 June 2017 at 01:56, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>>> >> + u32 int_status;
>>> >> + u32 fifo_status;
>>> >> + /* Read needs empty flag unset, write needs full flag unset */
>>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>>> >> + int ret;
>>> >> +
>>> >> + /* Wait until error or FIFO ready */
>>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>>> >> + int_status,
>>> >> + is_err_status(int_status) ||
>>> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>>> >> + 100000);
>>> >> +
>>> >> + if (is_err_status(int_status))
>>> >> + return -EIO;
>>> >> + if (ret)
>>> >> + return -ETIMEDOUT;
>>> >
>>> > Why not just have
>>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>>> > !(reg & flag), 100, 100000);
>>> >
>>> > if (ret < 0)
>>> > if (is_err_status())
>>> > return -EIO;
>>> > return ret;
>>> >
>>> >
>>>
>>> If I check error status after readl_poll_timeout and there is an error
>>> (e.g. the I2C address does not have a corresponding device connected
>>> or nothing connected to HDMI port) it will keep checking the fifo
>>> status even though error bit is set in the int status and then timeout
>>> after 100 ms. If it checks the int status register at the same time,
>>> it will error after 100 nanoseconds. I don't want to introduce
>>> unnecessary delays considering part of the reason for adding this
>>> driver to make it more usable for non-standard use cases.
>>
>> Well, polling for 100ms doesn't seem great either. What was the
>> rationale behind that timeout?
>>
>
> When an error occurs one of the error bits will be set in the
> INT_STATUS register so this is detected very quickly if I check the
> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
> case the I2C slave does clock stretching in which case the transfer
> may take longer than the predicted time.
>
>> And we can also reverse the check and look at the INT_STATUS
>> register. The errors will be there, and we can program the threshold
>> we want in both directions and use the
>> DDC_FIFO_Request_Interrupt_Status bit.
>>
>
> I did try that when I was doing the v3 patch but I couldn't get it to
> work as mentioned previously in the v3 patch discussion. I programmed
> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
> register at the same time as setting FIFO_Address_Clear but the
> request interrupt status bit did not get updated to the appropriate
> state that is consistent with the FIFO level and the thresholds. I did
> try this several times for subsequent patch versions without success.
The manual says "When FIFO level is above this value in read mode, DMA
request and FIFO request interrupt are asserted if relative enable is on."
Perhaps try enabling the interrupts? But if that were the case, wouldn't
using interrupts instead of polling be better?
ChenYu
>
>> Maxime
>>
>> --
>> Maxime Ripard, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
>
> Regards,
> Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-30 3:16 ` Chen-Yu Tsai
0 siblings, 0 replies; 34+ messages in thread
From: Chen-Yu Tsai @ 2017-06-30 3:16 UTC (permalink / raw)
To: Jonathan Liu
Cc: Maxime Ripard, David Airlie, Chen-Yu Tsai, linux-kernel,
dri-devel, linux-arm-kernel, linux-sunxi
On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu <net147-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Maxime,
>
> On 30 June 2017 at 01:56, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>>> >> + u32 int_status;
>>> >> + u32 fifo_status;
>>> >> + /* Read needs empty flag unset, write needs full flag unset */
>>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>>> >> + int ret;
>>> >> +
>>> >> + /* Wait until error or FIFO ready */
>>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>>> >> + int_status,
>>> >> + is_err_status(int_status) ||
>>> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>>> >> + 100000);
>>> >> +
>>> >> + if (is_err_status(int_status))
>>> >> + return -EIO;
>>> >> + if (ret)
>>> >> + return -ETIMEDOUT;
>>> >
>>> > Why not just have
>>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>>> > !(reg & flag), 100, 100000);
>>> >
>>> > if (ret < 0)
>>> > if (is_err_status())
>>> > return -EIO;
>>> > return ret;
>>> >
>>> >
>>>
>>> If I check error status after readl_poll_timeout and there is an error
>>> (e.g. the I2C address does not have a corresponding device connected
>>> or nothing connected to HDMI port) it will keep checking the fifo
>>> status even though error bit is set in the int status and then timeout
>>> after 100 ms. If it checks the int status register at the same time,
>>> it will error after 100 nanoseconds. I don't want to introduce
>>> unnecessary delays considering part of the reason for adding this
>>> driver to make it more usable for non-standard use cases.
>>
>> Well, polling for 100ms doesn't seem great either. What was the
>> rationale behind that timeout?
>>
>
> When an error occurs one of the error bits will be set in the
> INT_STATUS register so this is detected very quickly if I check the
> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
> case the I2C slave does clock stretching in which case the transfer
> may take longer than the predicted time.
>
>> And we can also reverse the check and look at the INT_STATUS
>> register. The errors will be there, and we can program the threshold
>> we want in both directions and use the
>> DDC_FIFO_Request_Interrupt_Status bit.
>>
>
> I did try that when I was doing the v3 patch but I couldn't get it to
> work as mentioned previously in the v3 patch discussion. I programmed
> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
> register at the same time as setting FIFO_Address_Clear but the
> request interrupt status bit did not get updated to the appropriate
> state that is consistent with the FIFO level and the thresholds. I did
> try this several times for subsequent patch versions without success.
The manual says "When FIFO level is above this value in read mode, DMA
request and FIFO request interrupt are asserted if relative enable is on."
Perhaps try enabling the interrupts? But if that were the case, wouldn't
using interrupts instead of polling be better?
ChenYu
>
>> Maxime
>>
>> --
>> Maxime Ripard, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
>
> Regards,
> Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
2017-06-29 22:22 ` Jonathan Liu
(?)
@ 2017-06-30 9:34 ` Maxime Ripard
-1 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-30 9:34 UTC (permalink / raw)
To: Jonathan Liu
Cc: David Airlie, Chen-Yu Tsai, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 3495 bytes --]
Hi,
On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote:
> Hi Maxime,
>
> On 30 June 2017 at 01:56, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
> >> >> + u32 int_status;
> >> >> + u32 fifo_status;
> >> >> + /* Read needs empty flag unset, write needs full flag unset */
> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> >> >> + int ret;
> >> >> +
> >> >> + /* Wait until error or FIFO ready */
> >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> >> >> + int_status,
> >> >> + is_err_status(int_status) ||
> >> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> >> >> + 100000);
> >> >> +
> >> >> + if (is_err_status(int_status))
> >> >> + return -EIO;
> >> >> + if (ret)
> >> >> + return -ETIMEDOUT;
> >> >
> >> > Why not just have
> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> >> > !(reg & flag), 100, 100000);
> >> >
> >> > if (ret < 0)
> >> > if (is_err_status())
> >> > return -EIO;
> >> > return ret;
> >> >
> >> >
> >>
> >> If I check error status after readl_poll_timeout and there is an error
> >> (e.g. the I2C address does not have a corresponding device connected
> >> or nothing connected to HDMI port) it will keep checking the fifo
> >> status even though error bit is set in the int status and then timeout
> >> after 100 ms. If it checks the int status register at the same time,
> >> it will error after 100 nanoseconds. I don't want to introduce
> >> unnecessary delays considering part of the reason for adding this
> >> driver to make it more usable for non-standard use cases.
> >
> > Well, polling for 100ms doesn't seem great either. What was the
> > rationale behind that timeout?
> >
>
> When an error occurs one of the error bits will be set in the
> INT_STATUS register so this is detected very quickly if I check the
> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
> case the I2C slave does clock stretching in which case the transfer
> may take longer than the predicted time.
100ms isn't stretching anymore, it's worse than that.
> > And we can also reverse the check and look at the INT_STATUS
> > register. The errors will be there, and we can program the threshold
> > we want in both directions and use the
> > DDC_FIFO_Request_Interrupt_Status bit.
> >
>
> I did try that when I was doing the v3 patch but I couldn't get it to
> work as mentioned previously in the v3 patch discussion. I programmed
> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
> register at the same time as setting FIFO_Address_Clear but the
> request interrupt status bit did not get updated to the appropriate
> state that is consistent with the FIFO level and the thresholds. I did
> try this several times for subsequent patch versions without success.
What values did you set it to?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-30 9:34 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-30 9:34 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote:
> Hi Maxime,
>
> On 30 June 2017 at 01:56, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
> >> >> + u32 int_status;
> >> >> + u32 fifo_status;
> >> >> + /* Read needs empty flag unset, write needs full flag unset */
> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> >> >> + int ret;
> >> >> +
> >> >> + /* Wait until error or FIFO ready */
> >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> >> >> + int_status,
> >> >> + is_err_status(int_status) ||
> >> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> >> >> + 100000);
> >> >> +
> >> >> + if (is_err_status(int_status))
> >> >> + return -EIO;
> >> >> + if (ret)
> >> >> + return -ETIMEDOUT;
> >> >
> >> > Why not just have
> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> >> > !(reg & flag), 100, 100000);
> >> >
> >> > if (ret < 0)
> >> > if (is_err_status())
> >> > return -EIO;
> >> > return ret;
> >> >
> >> >
> >>
> >> If I check error status after readl_poll_timeout and there is an error
> >> (e.g. the I2C address does not have a corresponding device connected
> >> or nothing connected to HDMI port) it will keep checking the fifo
> >> status even though error bit is set in the int status and then timeout
> >> after 100 ms. If it checks the int status register at the same time,
> >> it will error after 100 nanoseconds. I don't want to introduce
> >> unnecessary delays considering part of the reason for adding this
> >> driver to make it more usable for non-standard use cases.
> >
> > Well, polling for 100ms doesn't seem great either. What was the
> > rationale behind that timeout?
> >
>
> When an error occurs one of the error bits will be set in the
> INT_STATUS register so this is detected very quickly if I check the
> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
> case the I2C slave does clock stretching in which case the transfer
> may take longer than the predicted time.
100ms isn't stretching anymore, it's worse than that.
> > And we can also reverse the check and look at the INT_STATUS
> > register. The errors will be there, and we can program the threshold
> > we want in both directions and use the
> > DDC_FIFO_Request_Interrupt_Status bit.
> >
>
> I did try that when I was doing the v3 patch but I couldn't get it to
> work as mentioned previously in the v3 patch discussion. I programmed
> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
> register at the same time as setting FIFO_Address_Clear but the
> request interrupt status bit did not get updated to the appropriate
> state that is consistent with the FIFO level and the thresholds. I did
> try this several times for subsequent patch versions without success.
What values did you set it to?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170630/0a1813b0/attachment.sig>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-30 9:34 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-30 9:34 UTC (permalink / raw)
To: Jonathan Liu
Cc: linux-sunxi, dri-devel, linux-kernel, Chen-Yu Tsai, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 3495 bytes --]
Hi,
On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote:
> Hi Maxime,
>
> On 30 June 2017 at 01:56, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
> >> >> + u32 int_status;
> >> >> + u32 fifo_status;
> >> >> + /* Read needs empty flag unset, write needs full flag unset */
> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> >> >> + int ret;
> >> >> +
> >> >> + /* Wait until error or FIFO ready */
> >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> >> >> + int_status,
> >> >> + is_err_status(int_status) ||
> >> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> >> >> + 100000);
> >> >> +
> >> >> + if (is_err_status(int_status))
> >> >> + return -EIO;
> >> >> + if (ret)
> >> >> + return -ETIMEDOUT;
> >> >
> >> > Why not just have
> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> >> > !(reg & flag), 100, 100000);
> >> >
> >> > if (ret < 0)
> >> > if (is_err_status())
> >> > return -EIO;
> >> > return ret;
> >> >
> >> >
> >>
> >> If I check error status after readl_poll_timeout and there is an error
> >> (e.g. the I2C address does not have a corresponding device connected
> >> or nothing connected to HDMI port) it will keep checking the fifo
> >> status even though error bit is set in the int status and then timeout
> >> after 100 ms. If it checks the int status register at the same time,
> >> it will error after 100 nanoseconds. I don't want to introduce
> >> unnecessary delays considering part of the reason for adding this
> >> driver to make it more usable for non-standard use cases.
> >
> > Well, polling for 100ms doesn't seem great either. What was the
> > rationale behind that timeout?
> >
>
> When an error occurs one of the error bits will be set in the
> INT_STATUS register so this is detected very quickly if I check the
> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
> case the I2C slave does clock stretching in which case the transfer
> may take longer than the predicted time.
100ms isn't stretching anymore, it's worse than that.
> > And we can also reverse the check and look at the INT_STATUS
> > register. The errors will be there, and we can program the threshold
> > we want in both directions and use the
> > DDC_FIFO_Request_Interrupt_Status bit.
> >
>
> I did try that when I was doing the v3 patch but I couldn't get it to
> work as mentioned previously in the v3 patch discussion. I programmed
> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
> register at the same time as setting FIFO_Address_Clear but the
> request interrupt status bit did not get updated to the appropriate
> state that is consistent with the FIFO level and the thresholds. I did
> try this several times for subsequent patch versions without success.
What values did you set it to?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
2017-06-30 9:34 ` Maxime Ripard
(?)
@ 2017-06-30 9:58 ` Jonathan Liu
-1 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-30 9:58 UTC (permalink / raw)
To: Maxime Ripard
Cc: David Airlie, Chen-Yu Tsai, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
Hi Maxime,
On 30 June 2017 at 19:34, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote:
>> Hi Maxime,
>>
>> On 30 June 2017 at 01:56, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>> >> >> + u32 int_status;
>> >> >> + u32 fifo_status;
>> >> >> + /* Read needs empty flag unset, write needs full flag unset */
>> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>> >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>> >> >> + int ret;
>> >> >> +
>> >> >> + /* Wait until error or FIFO ready */
>> >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>> >> >> + int_status,
>> >> >> + is_err_status(int_status) ||
>> >> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>> >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>> >> >> + 100000);
>> >> >> +
>> >> >> + if (is_err_status(int_status))
>> >> >> + return -EIO;
>> >> >> + if (ret)
>> >> >> + return -ETIMEDOUT;
>> >> >
>> >> > Why not just have
>> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>> >> > !(reg & flag), 100, 100000);
>> >> >
>> >> > if (ret < 0)
>> >> > if (is_err_status())
>> >> > return -EIO;
>> >> > return ret;
>> >> >
>> >> >
>> >>
>> >> If I check error status after readl_poll_timeout and there is an error
>> >> (e.g. the I2C address does not have a corresponding device connected
>> >> or nothing connected to HDMI port) it will keep checking the fifo
>> >> status even though error bit is set in the int status and then timeout
>> >> after 100 ms. If it checks the int status register at the same time,
>> >> it will error after 100 nanoseconds. I don't want to introduce
>> >> unnecessary delays considering part of the reason for adding this
>> >> driver to make it more usable for non-standard use cases.
>> >
>> > Well, polling for 100ms doesn't seem great either. What was the
>> > rationale behind that timeout?
>> >
>>
>> When an error occurs one of the error bits will be set in the
>> INT_STATUS register so this is detected very quickly if I check the
>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
>> case the I2C slave does clock stretching in which case the transfer
>> may take longer than the predicted time.
>
> 100ms isn't stretching anymore, it's worse than that.
>
What would you suggest?
>> > And we can also reverse the check and look at the INT_STATUS
>> > register. The errors will be there, and we can program the threshold
>> > we want in both directions and use the
>> > DDC_FIFO_Request_Interrupt_Status bit.
>> >
>>
>> I did try that when I was doing the v3 patch but I couldn't get it to
>> work as mentioned previously in the v3 patch discussion. I programmed
>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
>> register at the same time as setting FIFO_Address_Clear but the
>> request interrupt status bit did not get updated to the appropriate
>> state that is consistent with the FIFO level and the thresholds. I did
>> try this several times for subsequent patch versions without success.
>
> What values did you set it to?
>
FIFO_RX_TRIGGER_THRES: 0
FIFO_TX_TRIGGER_THRES: 15
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-30 9:58 ` Jonathan Liu
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-30 9:58 UTC (permalink / raw)
To: linux-arm-kernel
Hi Maxime,
On 30 June 2017 at 19:34, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote:
>> Hi Maxime,
>>
>> On 30 June 2017 at 01:56, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>> >> >> + u32 int_status;
>> >> >> + u32 fifo_status;
>> >> >> + /* Read needs empty flag unset, write needs full flag unset */
>> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>> >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>> >> >> + int ret;
>> >> >> +
>> >> >> + /* Wait until error or FIFO ready */
>> >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>> >> >> + int_status,
>> >> >> + is_err_status(int_status) ||
>> >> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>> >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>> >> >> + 100000);
>> >> >> +
>> >> >> + if (is_err_status(int_status))
>> >> >> + return -EIO;
>> >> >> + if (ret)
>> >> >> + return -ETIMEDOUT;
>> >> >
>> >> > Why not just have
>> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>> >> > !(reg & flag), 100, 100000);
>> >> >
>> >> > if (ret < 0)
>> >> > if (is_err_status())
>> >> > return -EIO;
>> >> > return ret;
>> >> >
>> >> >
>> >>
>> >> If I check error status after readl_poll_timeout and there is an error
>> >> (e.g. the I2C address does not have a corresponding device connected
>> >> or nothing connected to HDMI port) it will keep checking the fifo
>> >> status even though error bit is set in the int status and then timeout
>> >> after 100 ms. If it checks the int status register at the same time,
>> >> it will error after 100 nanoseconds. I don't want to introduce
>> >> unnecessary delays considering part of the reason for adding this
>> >> driver to make it more usable for non-standard use cases.
>> >
>> > Well, polling for 100ms doesn't seem great either. What was the
>> > rationale behind that timeout?
>> >
>>
>> When an error occurs one of the error bits will be set in the
>> INT_STATUS register so this is detected very quickly if I check the
>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
>> case the I2C slave does clock stretching in which case the transfer
>> may take longer than the predicted time.
>
> 100ms isn't stretching anymore, it's worse than that.
>
What would you suggest?
>> > And we can also reverse the check and look at the INT_STATUS
>> > register. The errors will be there, and we can program the threshold
>> > we want in both directions and use the
>> > DDC_FIFO_Request_Interrupt_Status bit.
>> >
>>
>> I did try that when I was doing the v3 patch but I couldn't get it to
>> work as mentioned previously in the v3 patch discussion. I programmed
>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
>> register at the same time as setting FIFO_Address_Clear but the
>> request interrupt status bit did not get updated to the appropriate
>> state that is consistent with the FIFO level and the thresholds. I did
>> try this several times for subsequent patch versions without success.
>
> What values did you set it to?
>
FIFO_RX_TRIGGER_THRES: 0
FIFO_TX_TRIGGER_THRES: 15
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-30 9:58 ` Jonathan Liu
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-30 9:58 UTC (permalink / raw)
To: Maxime Ripard
Cc: David Airlie, Chen-Yu Tsai, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
Hi Maxime,
On 30 June 2017 at 19:34, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote:
>> Hi Maxime,
>>
>> On 30 June 2017 at 01:56, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>> >> >> + u32 int_status;
>> >> >> + u32 fifo_status;
>> >> >> + /* Read needs empty flag unset, write needs full flag unset */
>> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>> >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>> >> >> + int ret;
>> >> >> +
>> >> >> + /* Wait until error or FIFO ready */
>> >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>> >> >> + int_status,
>> >> >> + is_err_status(int_status) ||
>> >> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>> >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>> >> >> + 100000);
>> >> >> +
>> >> >> + if (is_err_status(int_status))
>> >> >> + return -EIO;
>> >> >> + if (ret)
>> >> >> + return -ETIMEDOUT;
>> >> >
>> >> > Why not just have
>> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>> >> > !(reg & flag), 100, 100000);
>> >> >
>> >> > if (ret < 0)
>> >> > if (is_err_status())
>> >> > return -EIO;
>> >> > return ret;
>> >> >
>> >> >
>> >>
>> >> If I check error status after readl_poll_timeout and there is an error
>> >> (e.g. the I2C address does not have a corresponding device connected
>> >> or nothing connected to HDMI port) it will keep checking the fifo
>> >> status even though error bit is set in the int status and then timeout
>> >> after 100 ms. If it checks the int status register at the same time,
>> >> it will error after 100 nanoseconds. I don't want to introduce
>> >> unnecessary delays considering part of the reason for adding this
>> >> driver to make it more usable for non-standard use cases.
>> >
>> > Well, polling for 100ms doesn't seem great either. What was the
>> > rationale behind that timeout?
>> >
>>
>> When an error occurs one of the error bits will be set in the
>> INT_STATUS register so this is detected very quickly if I check the
>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
>> case the I2C slave does clock stretching in which case the transfer
>> may take longer than the predicted time.
>
> 100ms isn't stretching anymore, it's worse than that.
>
What would you suggest?
>> > And we can also reverse the check and look at the INT_STATUS
>> > register. The errors will be there, and we can program the threshold
>> > we want in both directions and use the
>> > DDC_FIFO_Request_Interrupt_Status bit.
>> >
>>
>> I did try that when I was doing the v3 patch but I couldn't get it to
>> work as mentioned previously in the v3 patch discussion. I programmed
>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
>> register at the same time as setting FIFO_Address_Clear but the
>> request interrupt status bit did not get updated to the appropriate
>> state that is consistent with the FIFO level and the thresholds. I did
>> try this several times for subsequent patch versions without success.
>
> What values did you set it to?
>
FIFO_RX_TRIGGER_THRES: 0
FIFO_TX_TRIGGER_THRES: 15
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
2017-06-30 3:16 ` Chen-Yu Tsai
@ 2017-06-30 14:14 ` Jonathan Liu
-1 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-30 14:14 UTC (permalink / raw)
To: Chen-Yu Tsai, Maxime Ripard
Cc: David Airlie, linux-kernel, dri-devel, linux-arm-kernel, linux-sunxi
Hi Chen-Yu and Maxime,
On 30 June 2017 at 13:16, Chen-Yu Tsai <wens@csie.org> wrote:
> On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu <net147@gmail.com> wrote:
>> Hi Maxime,
>>
>> On 30 June 2017 at 01:56, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>>>> >> + u32 int_status;
>>>> >> + u32 fifo_status;
>>>> >> + /* Read needs empty flag unset, write needs full flag unset */
>>>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>>>> >> + int ret;
>>>> >> +
>>>> >> + /* Wait until error or FIFO ready */
>>>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>>>> >> + int_status,
>>>> >> + is_err_status(int_status) ||
>>>> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>>>> >> + 100000);
>>>> >> +
>>>> >> + if (is_err_status(int_status))
>>>> >> + return -EIO;
>>>> >> + if (ret)
>>>> >> + return -ETIMEDOUT;
>>>> >
>>>> > Why not just have
>>>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>>>> > !(reg & flag), 100, 100000);
>>>> >
>>>> > if (ret < 0)
>>>> > if (is_err_status())
>>>> > return -EIO;
>>>> > return ret;
>>>> >
>>>> >
>>>>
>>>> If I check error status after readl_poll_timeout and there is an error
>>>> (e.g. the I2C address does not have a corresponding device connected
>>>> or nothing connected to HDMI port) it will keep checking the fifo
>>>> status even though error bit is set in the int status and then timeout
>>>> after 100 ms. If it checks the int status register at the same time,
>>>> it will error after 100 nanoseconds. I don't want to introduce
>>>> unnecessary delays considering part of the reason for adding this
>>>> driver to make it more usable for non-standard use cases.
>>>
>>> Well, polling for 100ms doesn't seem great either. What was the
>>> rationale behind that timeout?
>>>
>>
>> When an error occurs one of the error bits will be set in the
>> INT_STATUS register so this is detected very quickly if I check the
>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
>> case the I2C slave does clock stretching in which case the transfer
>> may take longer than the predicted time.
>>
>>> And we can also reverse the check and look at the INT_STATUS
>>> register. The errors will be there, and we can program the threshold
>>> we want in both directions and use the
>>> DDC_FIFO_Request_Interrupt_Status bit.
>>>
>>
>> I did try that when I was doing the v3 patch but I couldn't get it to
>> work as mentioned previously in the v3 patch discussion. I programmed
>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
>> register at the same time as setting FIFO_Address_Clear but the
>> request interrupt status bit did not get updated to the appropriate
>> state that is consistent with the FIFO level and the thresholds. I did
>> try this several times for subsequent patch versions without success.
>
> The manual says "When FIFO level is above this value in read mode, DMA
> request and FIFO request interrupt are asserted if relative enable is on."
>
> Perhaps try enabling the interrupts? But if that were the case, wouldn't
> using interrupts instead of polling be better?
>
> ChenYu
>
I managed to get the thresholds working so switching to using
interrupts instead of polling will be my next goal.
>>
>>> Maxime
>>>
>>> --
>>> Maxime Ripard, Free Electrons
>>> Embedded Linux and Kernel engineering
>>> http://free-electrons.com
>>
>> Regards,
>> Jonathan
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-30 14:14 ` Jonathan Liu
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-06-30 14:14 UTC (permalink / raw)
To: linux-arm-kernel
Hi Chen-Yu and Maxime,
On 30 June 2017 at 13:16, Chen-Yu Tsai <wens@csie.org> wrote:
> On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu <net147@gmail.com> wrote:
>> Hi Maxime,
>>
>> On 30 June 2017 at 01:56, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>>>> >> + u32 int_status;
>>>> >> + u32 fifo_status;
>>>> >> + /* Read needs empty flag unset, write needs full flag unset */
>>>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>>>> >> + int ret;
>>>> >> +
>>>> >> + /* Wait until error or FIFO ready */
>>>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>>>> >> + int_status,
>>>> >> + is_err_status(int_status) ||
>>>> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>>>> >> + 100000);
>>>> >> +
>>>> >> + if (is_err_status(int_status))
>>>> >> + return -EIO;
>>>> >> + if (ret)
>>>> >> + return -ETIMEDOUT;
>>>> >
>>>> > Why not just have
>>>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>>>> > !(reg & flag), 100, 100000);
>>>> >
>>>> > if (ret < 0)
>>>> > if (is_err_status())
>>>> > return -EIO;
>>>> > return ret;
>>>> >
>>>> >
>>>>
>>>> If I check error status after readl_poll_timeout and there is an error
>>>> (e.g. the I2C address does not have a corresponding device connected
>>>> or nothing connected to HDMI port) it will keep checking the fifo
>>>> status even though error bit is set in the int status and then timeout
>>>> after 100 ms. If it checks the int status register at the same time,
>>>> it will error after 100 nanoseconds. I don't want to introduce
>>>> unnecessary delays considering part of the reason for adding this
>>>> driver to make it more usable for non-standard use cases.
>>>
>>> Well, polling for 100ms doesn't seem great either. What was the
>>> rationale behind that timeout?
>>>
>>
>> When an error occurs one of the error bits will be set in the
>> INT_STATUS register so this is detected very quickly if I check the
>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
>> case the I2C slave does clock stretching in which case the transfer
>> may take longer than the predicted time.
>>
>>> And we can also reverse the check and look at the INT_STATUS
>>> register. The errors will be there, and we can program the threshold
>>> we want in both directions and use the
>>> DDC_FIFO_Request_Interrupt_Status bit.
>>>
>>
>> I did try that when I was doing the v3 patch but I couldn't get it to
>> work as mentioned previously in the v3 patch discussion. I programmed
>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
>> register at the same time as setting FIFO_Address_Clear but the
>> request interrupt status bit did not get updated to the appropriate
>> state that is consistent with the FIFO level and the thresholds. I did
>> try this several times for subsequent patch versions without success.
>
> The manual says "When FIFO level is above this value in read mode, DMA
> request and FIFO request interrupt are asserted if relative enable is on."
>
> Perhaps try enabling the interrupts? But if that were the case, wouldn't
> using interrupts instead of polling be better?
>
> ChenYu
>
I managed to get the thresholds working so switching to using
interrupts instead of polling will be my next goal.
>>
>>> Maxime
>>>
>>> --
>>> Maxime Ripard, Free Electrons
>>> Embedded Linux and Kernel engineering
>>> http://free-electrons.com
>>
>> Regards,
>> Jonathan
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
2017-06-30 14:14 ` Jonathan Liu
(?)
@ 2017-06-30 16:01 ` Maxime Ripard
-1 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-30 16:01 UTC (permalink / raw)
To: Jonathan Liu
Cc: Chen-Yu Tsai, David Airlie, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 4348 bytes --]
On Sat, Jul 01, 2017 at 12:14:44AM +1000, Jonathan Liu wrote:
> Hi Chen-Yu and Maxime,
>
> On 30 June 2017 at 13:16, Chen-Yu Tsai <wens@csie.org> wrote:
> > On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu <net147@gmail.com> wrote:
> >> Hi Maxime,
> >>
> >> On 30 June 2017 at 01:56, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
> >>>> >> + u32 int_status;
> >>>> >> + u32 fifo_status;
> >>>> >> + /* Read needs empty flag unset, write needs full flag unset */
> >>>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> >>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> >>>> >> + int ret;
> >>>> >> +
> >>>> >> + /* Wait until error or FIFO ready */
> >>>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> >>>> >> + int_status,
> >>>> >> + is_err_status(int_status) ||
> >>>> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> >>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> >>>> >> + 100000);
> >>>> >> +
> >>>> >> + if (is_err_status(int_status))
> >>>> >> + return -EIO;
> >>>> >> + if (ret)
> >>>> >> + return -ETIMEDOUT;
> >>>> >
> >>>> > Why not just have
> >>>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> >>>> > !(reg & flag), 100, 100000);
> >>>> >
> >>>> > if (ret < 0)
> >>>> > if (is_err_status())
> >>>> > return -EIO;
> >>>> > return ret;
> >>>> >
> >>>> >
> >>>>
> >>>> If I check error status after readl_poll_timeout and there is an error
> >>>> (e.g. the I2C address does not have a corresponding device connected
> >>>> or nothing connected to HDMI port) it will keep checking the fifo
> >>>> status even though error bit is set in the int status and then timeout
> >>>> after 100 ms. If it checks the int status register at the same time,
> >>>> it will error after 100 nanoseconds. I don't want to introduce
> >>>> unnecessary delays considering part of the reason for adding this
> >>>> driver to make it more usable for non-standard use cases.
> >>>
> >>> Well, polling for 100ms doesn't seem great either. What was the
> >>> rationale behind that timeout?
> >>>
> >>
> >> When an error occurs one of the error bits will be set in the
> >> INT_STATUS register so this is detected very quickly if I check the
> >> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
> >> case the I2C slave does clock stretching in which case the transfer
> >> may take longer than the predicted time.
> >>
> >>> And we can also reverse the check and look at the INT_STATUS
> >>> register. The errors will be there, and we can program the threshold
> >>> we want in both directions and use the
> >>> DDC_FIFO_Request_Interrupt_Status bit.
> >>>
> >>
> >> I did try that when I was doing the v3 patch but I couldn't get it to
> >> work as mentioned previously in the v3 patch discussion. I programmed
> >> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
> >> register at the same time as setting FIFO_Address_Clear but the
> >> request interrupt status bit did not get updated to the appropriate
> >> state that is consistent with the FIFO level and the thresholds. I did
> >> try this several times for subsequent patch versions without success.
> >
> > The manual says "When FIFO level is above this value in read mode, DMA
> > request and FIFO request interrupt are asserted if relative enable is on."
> >
> > Perhaps try enabling the interrupts? But if that were the case, wouldn't
> > using interrupts instead of polling be better?
> >
> > ChenYu
> >
>
> I managed to get the thresholds working so switching to using
> interrupts instead of polling will be my next goal.
I'd say that we can do it using polling first, and then move to
interrupts if needed. And that should definitely be part of a separate
patch. This one is already pretty invasive.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-30 16:01 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-30 16:01 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Jul 01, 2017 at 12:14:44AM +1000, Jonathan Liu wrote:
> Hi Chen-Yu and Maxime,
>
> On 30 June 2017 at 13:16, Chen-Yu Tsai <wens@csie.org> wrote:
> > On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu <net147@gmail.com> wrote:
> >> Hi Maxime,
> >>
> >> On 30 June 2017 at 01:56, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
> >>>> >> + u32 int_status;
> >>>> >> + u32 fifo_status;
> >>>> >> + /* Read needs empty flag unset, write needs full flag unset */
> >>>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> >>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> >>>> >> + int ret;
> >>>> >> +
> >>>> >> + /* Wait until error or FIFO ready */
> >>>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> >>>> >> + int_status,
> >>>> >> + is_err_status(int_status) ||
> >>>> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> >>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> >>>> >> + 100000);
> >>>> >> +
> >>>> >> + if (is_err_status(int_status))
> >>>> >> + return -EIO;
> >>>> >> + if (ret)
> >>>> >> + return -ETIMEDOUT;
> >>>> >
> >>>> > Why not just have
> >>>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> >>>> > !(reg & flag), 100, 100000);
> >>>> >
> >>>> > if (ret < 0)
> >>>> > if (is_err_status())
> >>>> > return -EIO;
> >>>> > return ret;
> >>>> >
> >>>> >
> >>>>
> >>>> If I check error status after readl_poll_timeout and there is an error
> >>>> (e.g. the I2C address does not have a corresponding device connected
> >>>> or nothing connected to HDMI port) it will keep checking the fifo
> >>>> status even though error bit is set in the int status and then timeout
> >>>> after 100 ms. If it checks the int status register at the same time,
> >>>> it will error after 100 nanoseconds. I don't want to introduce
> >>>> unnecessary delays considering part of the reason for adding this
> >>>> driver to make it more usable for non-standard use cases.
> >>>
> >>> Well, polling for 100ms doesn't seem great either. What was the
> >>> rationale behind that timeout?
> >>>
> >>
> >> When an error occurs one of the error bits will be set in the
> >> INT_STATUS register so this is detected very quickly if I check the
> >> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
> >> case the I2C slave does clock stretching in which case the transfer
> >> may take longer than the predicted time.
> >>
> >>> And we can also reverse the check and look at the INT_STATUS
> >>> register. The errors will be there, and we can program the threshold
> >>> we want in both directions and use the
> >>> DDC_FIFO_Request_Interrupt_Status bit.
> >>>
> >>
> >> I did try that when I was doing the v3 patch but I couldn't get it to
> >> work as mentioned previously in the v3 patch discussion. I programmed
> >> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
> >> register at the same time as setting FIFO_Address_Clear but the
> >> request interrupt status bit did not get updated to the appropriate
> >> state that is consistent with the FIFO level and the thresholds. I did
> >> try this several times for subsequent patch versions without success.
> >
> > The manual says "When FIFO level is above this value in read mode, DMA
> > request and FIFO request interrupt are asserted if relative enable is on."
> >
> > Perhaps try enabling the interrupts? But if that were the case, wouldn't
> > using interrupts instead of polling be better?
> >
> > ChenYu
> >
>
> I managed to get the thresholds working so switching to using
> interrupts instead of polling will be my next goal.
I'd say that we can do it using polling first, and then move to
interrupts if needed. And that should definitely be part of a separate
patch. This one is already pretty invasive.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170630/4ea810e2/attachment-0001.sig>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-06-30 16:01 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2017-06-30 16:01 UTC (permalink / raw)
To: Jonathan Liu
Cc: linux-sunxi, dri-devel, linux-kernel, Chen-Yu Tsai, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 4348 bytes --]
On Sat, Jul 01, 2017 at 12:14:44AM +1000, Jonathan Liu wrote:
> Hi Chen-Yu and Maxime,
>
> On 30 June 2017 at 13:16, Chen-Yu Tsai <wens@csie.org> wrote:
> > On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu <net147@gmail.com> wrote:
> >> Hi Maxime,
> >>
> >> On 30 June 2017 at 01:56, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
> >>>> >> + u32 int_status;
> >>>> >> + u32 fifo_status;
> >>>> >> + /* Read needs empty flag unset, write needs full flag unset */
> >>>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
> >>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
> >>>> >> + int ret;
> >>>> >> +
> >>>> >> + /* Wait until error or FIFO ready */
> >>>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
> >>>> >> + int_status,
> >>>> >> + is_err_status(int_status) ||
> >>>> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
> >>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
> >>>> >> + 100000);
> >>>> >> +
> >>>> >> + if (is_err_status(int_status))
> >>>> >> + return -EIO;
> >>>> >> + if (ret)
> >>>> >> + return -ETIMEDOUT;
> >>>> >
> >>>> > Why not just have
> >>>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
> >>>> > !(reg & flag), 100, 100000);
> >>>> >
> >>>> > if (ret < 0)
> >>>> > if (is_err_status())
> >>>> > return -EIO;
> >>>> > return ret;
> >>>> >
> >>>> >
> >>>>
> >>>> If I check error status after readl_poll_timeout and there is an error
> >>>> (e.g. the I2C address does not have a corresponding device connected
> >>>> or nothing connected to HDMI port) it will keep checking the fifo
> >>>> status even though error bit is set in the int status and then timeout
> >>>> after 100 ms. If it checks the int status register at the same time,
> >>>> it will error after 100 nanoseconds. I don't want to introduce
> >>>> unnecessary delays considering part of the reason for adding this
> >>>> driver to make it more usable for non-standard use cases.
> >>>
> >>> Well, polling for 100ms doesn't seem great either. What was the
> >>> rationale behind that timeout?
> >>>
> >>
> >> When an error occurs one of the error bits will be set in the
> >> INT_STATUS register so this is detected very quickly if I check the
> >> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
> >> case the I2C slave does clock stretching in which case the transfer
> >> may take longer than the predicted time.
> >>
> >>> And we can also reverse the check and look at the INT_STATUS
> >>> register. The errors will be there, and we can program the threshold
> >>> we want in both directions and use the
> >>> DDC_FIFO_Request_Interrupt_Status bit.
> >>>
> >>
> >> I did try that when I was doing the v3 patch but I couldn't get it to
> >> work as mentioned previously in the v3 patch discussion. I programmed
> >> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
> >> register at the same time as setting FIFO_Address_Clear but the
> >> request interrupt status bit did not get updated to the appropriate
> >> state that is consistent with the FIFO level and the thresholds. I did
> >> try this several times for subsequent patch versions without success.
> >
> > The manual says "When FIFO level is above this value in read mode, DMA
> > request and FIFO request interrupt are asserted if relative enable is on."
> >
> > Perhaps try enabling the interrupts? But if that were the case, wouldn't
> > using interrupts instead of polling be better?
> >
> > ChenYu
> >
>
> I managed to get the thresholds working so switching to using
> interrupts instead of polling will be my next goal.
I'd say that we can do it using polling first, and then move to
interrupts if needed. And that should definitely be part of a separate
patch. This one is already pretty invasive.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
2017-06-30 9:58 ` Jonathan Liu
(?)
@ 2017-07-01 6:29 ` Jonathan Liu
-1 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-07-01 6:29 UTC (permalink / raw)
To: Maxime Ripard
Cc: David Airlie, Chen-Yu Tsai, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
Hi Maxime,
On 30 June 2017 at 19:58, Jonathan Liu <net147@gmail.com> wrote:
> Hi Maxime,
>
> On 30 June 2017 at 19:34, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> Hi,
>>
>> On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote:
>>> Hi Maxime,
>>>
>>> On 30 June 2017 at 01:56, Maxime Ripard
>>> <maxime.ripard@free-electrons.com> wrote:
>>> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>>> >> >> + u32 int_status;
>>> >> >> + u32 fifo_status;
>>> >> >> + /* Read needs empty flag unset, write needs full flag unset */
>>> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>>> >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>>> >> >> + int ret;
>>> >> >> +
>>> >> >> + /* Wait until error or FIFO ready */
>>> >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>>> >> >> + int_status,
>>> >> >> + is_err_status(int_status) ||
>>> >> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>>> >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>>> >> >> + 100000);
>>> >> >> +
>>> >> >> + if (is_err_status(int_status))
>>> >> >> + return -EIO;
>>> >> >> + if (ret)
>>> >> >> + return -ETIMEDOUT;
>>> >> >
>>> >> > Why not just have
>>> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>>> >> > !(reg & flag), 100, 100000);
>>> >> >
>>> >> > if (ret < 0)
>>> >> > if (is_err_status())
>>> >> > return -EIO;
>>> >> > return ret;
>>> >> >
>>> >> >
>>> >>
>>> >> If I check error status after readl_poll_timeout and there is an error
>>> >> (e.g. the I2C address does not have a corresponding device connected
>>> >> or nothing connected to HDMI port) it will keep checking the fifo
>>> >> status even though error bit is set in the int status and then timeout
>>> >> after 100 ms. If it checks the int status register at the same time,
>>> >> it will error after 100 nanoseconds. I don't want to introduce
>>> >> unnecessary delays considering part of the reason for adding this
>>> >> driver to make it more usable for non-standard use cases.
>>> >
>>> > Well, polling for 100ms doesn't seem great either. What was the
>>> > rationale behind that timeout?
>>> >
>>>
>>> When an error occurs one of the error bits will be set in the
>>> INT_STATUS register so this is detected very quickly if I check the
>>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
>>> case the I2C slave does clock stretching in which case the transfer
>>> may take longer than the predicted time.
>>
>> 100ms isn't stretching anymore, it's worse than that.
>>
>
> What would you suggest?
>
You need to consider the fact that there are I2C devices such as
sensors that may do I2C clock stretching up to several tens of
milliseconds. For example the HTU21D humidity sensor takes up to 50 ms
max to perform measurement of temperature during which it is holding
down the clock to do I2C clock stretching.
>>> > And we can also reverse the check and look at the INT_STATUS
>>> > register. The errors will be there, and we can program the threshold
>>> > we want in both directions and use the
>>> > DDC_FIFO_Request_Interrupt_Status bit.
>>> >
>>>
>>> I did try that when I was doing the v3 patch but I couldn't get it to
>>> work as mentioned previously in the v3 patch discussion. I programmed
>>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
>>> register at the same time as setting FIFO_Address_Clear but the
>>> request interrupt status bit did not get updated to the appropriate
>>> state that is consistent with the FIFO level and the thresholds. I did
>>> try this several times for subsequent patch versions without success.
>>
>> What values did you set it to?
>>
>
> FIFO_RX_TRIGGER_THRES: 0
> FIFO_TX_TRIGGER_THRES: 15
>
>> Maxime
>>
>> --
>> Maxime Ripard, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
>
> Regards,
> Jonathan
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-07-01 6:29 ` Jonathan Liu
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-07-01 6:29 UTC (permalink / raw)
To: linux-arm-kernel
Hi Maxime,
On 30 June 2017 at 19:58, Jonathan Liu <net147@gmail.com> wrote:
> Hi Maxime,
>
> On 30 June 2017 at 19:34, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> Hi,
>>
>> On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote:
>>> Hi Maxime,
>>>
>>> On 30 June 2017 at 01:56, Maxime Ripard
>>> <maxime.ripard@free-electrons.com> wrote:
>>> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>>> >> >> + u32 int_status;
>>> >> >> + u32 fifo_status;
>>> >> >> + /* Read needs empty flag unset, write needs full flag unset */
>>> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>>> >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>>> >> >> + int ret;
>>> >> >> +
>>> >> >> + /* Wait until error or FIFO ready */
>>> >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>>> >> >> + int_status,
>>> >> >> + is_err_status(int_status) ||
>>> >> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>>> >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>>> >> >> + 100000);
>>> >> >> +
>>> >> >> + if (is_err_status(int_status))
>>> >> >> + return -EIO;
>>> >> >> + if (ret)
>>> >> >> + return -ETIMEDOUT;
>>> >> >
>>> >> > Why not just have
>>> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>>> >> > !(reg & flag), 100, 100000);
>>> >> >
>>> >> > if (ret < 0)
>>> >> > if (is_err_status())
>>> >> > return -EIO;
>>> >> > return ret;
>>> >> >
>>> >> >
>>> >>
>>> >> If I check error status after readl_poll_timeout and there is an error
>>> >> (e.g. the I2C address does not have a corresponding device connected
>>> >> or nothing connected to HDMI port) it will keep checking the fifo
>>> >> status even though error bit is set in the int status and then timeout
>>> >> after 100 ms. If it checks the int status register at the same time,
>>> >> it will error after 100 nanoseconds. I don't want to introduce
>>> >> unnecessary delays considering part of the reason for adding this
>>> >> driver to make it more usable for non-standard use cases.
>>> >
>>> > Well, polling for 100ms doesn't seem great either. What was the
>>> > rationale behind that timeout?
>>> >
>>>
>>> When an error occurs one of the error bits will be set in the
>>> INT_STATUS register so this is detected very quickly if I check the
>>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
>>> case the I2C slave does clock stretching in which case the transfer
>>> may take longer than the predicted time.
>>
>> 100ms isn't stretching anymore, it's worse than that.
>>
>
> What would you suggest?
>
You need to consider the fact that there are I2C devices such as
sensors that may do I2C clock stretching up to several tens of
milliseconds. For example the HTU21D humidity sensor takes up to 50 ms
max to perform measurement of temperature during which it is holding
down the clock to do I2C clock stretching.
>>> > And we can also reverse the check and look at the INT_STATUS
>>> > register. The errors will be there, and we can program the threshold
>>> > we want in both directions and use the
>>> > DDC_FIFO_Request_Interrupt_Status bit.
>>> >
>>>
>>> I did try that when I was doing the v3 patch but I couldn't get it to
>>> work as mentioned previously in the v3 patch discussion. I programmed
>>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
>>> register at the same time as setting FIFO_Address_Clear but the
>>> request interrupt status bit did not get updated to the appropriate
>>> state that is consistent with the FIFO level and the thresholds. I did
>>> try this several times for subsequent patch versions without success.
>>
>> What values did you set it to?
>>
>
> FIFO_RX_TRIGGER_THRES: 0
> FIFO_TX_TRIGGER_THRES: 15
>
>> Maxime
>>
>> --
>> Maxime Ripard, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
>
> Regards,
> Jonathan
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
@ 2017-07-01 6:29 ` Jonathan Liu
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Liu @ 2017-07-01 6:29 UTC (permalink / raw)
To: Maxime Ripard
Cc: David Airlie, Chen-Yu Tsai, linux-kernel, dri-devel,
linux-arm-kernel, linux-sunxi
Hi Maxime,
On 30 June 2017 at 19:58, Jonathan Liu <net147-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Maxime,
>
> On 30 June 2017 at 19:34, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> Hi,
>>
>> On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote:
>>> Hi Maxime,
>>>
>>> On 30 June 2017 at 01:56, Maxime Ripard
>>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>>> >> >> + u32 int_status;
>>> >> >> + u32 fifo_status;
>>> >> >> + /* Read needs empty flag unset, write needs full flag unset */
>>> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>>> >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>>> >> >> + int ret;
>>> >> >> +
>>> >> >> + /* Wait until error or FIFO ready */
>>> >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>>> >> >> + int_status,
>>> >> >> + is_err_status(int_status) ||
>>> >> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag),
>>> >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time,
>>> >> >> + 100000);
>>> >> >> +
>>> >> >> + if (is_err_status(int_status))
>>> >> >> + return -EIO;
>>> >> >> + if (ret)
>>> >> >> + return -ETIMEDOUT;
>>> >> >
>>> >> > Why not just have
>>> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>>> >> > !(reg & flag), 100, 100000);
>>> >> >
>>> >> > if (ret < 0)
>>> >> > if (is_err_status())
>>> >> > return -EIO;
>>> >> > return ret;
>>> >> >
>>> >> >
>>> >>
>>> >> If I check error status after readl_poll_timeout and there is an error
>>> >> (e.g. the I2C address does not have a corresponding device connected
>>> >> or nothing connected to HDMI port) it will keep checking the fifo
>>> >> status even though error bit is set in the int status and then timeout
>>> >> after 100 ms. If it checks the int status register at the same time,
>>> >> it will error after 100 nanoseconds. I don't want to introduce
>>> >> unnecessary delays considering part of the reason for adding this
>>> >> driver to make it more usable for non-standard use cases.
>>> >
>>> > Well, polling for 100ms doesn't seem great either. What was the
>>> > rationale behind that timeout?
>>> >
>>>
>>> When an error occurs one of the error bits will be set in the
>>> INT_STATUS register so this is detected very quickly if I check the
>>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
>>> case the I2C slave does clock stretching in which case the transfer
>>> may take longer than the predicted time.
>>
>> 100ms isn't stretching anymore, it's worse than that.
>>
>
> What would you suggest?
>
You need to consider the fact that there are I2C devices such as
sensors that may do I2C clock stretching up to several tens of
milliseconds. For example the HTU21D humidity sensor takes up to 50 ms
max to perform measurement of temperature during which it is holding
down the clock to do I2C clock stretching.
>>> > And we can also reverse the check and look at the INT_STATUS
>>> > register. The errors will be there, and we can program the threshold
>>> > we want in both directions and use the
>>> > DDC_FIFO_Request_Interrupt_Status bit.
>>> >
>>>
>>> I did try that when I was doing the v3 patch but I couldn't get it to
>>> work as mentioned previously in the v3 patch discussion. I programmed
>>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
>>> register at the same time as setting FIFO_Address_Clear but the
>>> request interrupt status bit did not get updated to the appropriate
>>> state that is consistent with the FIFO level and the thresholds. I did
>>> try this several times for subsequent patch versions without success.
>>
>> What values did you set it to?
>>
>
> FIFO_RX_TRIGGER_THRES: 0
> FIFO_TX_TRIGGER_THRES: 15
>
>> Maxime
>>
>> --
>> Maxime Ripard, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
>
> Regards,
> Jonathan
Regards,
Jonathan
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2017-07-01 6:29 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-27 14:36 [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus Jonathan Liu
2017-06-27 14:36 ` Jonathan Liu
2017-06-27 14:36 ` Jonathan Liu
2017-06-28 9:20 ` Maxime Ripard
2017-06-28 9:20 ` Maxime Ripard
2017-06-28 9:20 ` Maxime Ripard
2017-06-28 10:39 ` Jonathan Liu
2017-06-28 10:39 ` Jonathan Liu
2017-06-28 10:39 ` Jonathan Liu
2017-06-29 15:56 ` Maxime Ripard
2017-06-29 15:56 ` Maxime Ripard
2017-06-29 15:56 ` Maxime Ripard
2017-06-29 22:22 ` Jonathan Liu
2017-06-29 22:22 ` Jonathan Liu
2017-06-30 3:16 ` Chen-Yu Tsai
2017-06-30 3:16 ` Chen-Yu Tsai
2017-06-30 3:16 ` Chen-Yu Tsai
2017-06-30 14:14 ` Jonathan Liu
2017-06-30 14:14 ` Jonathan Liu
2017-06-30 16:01 ` Maxime Ripard
2017-06-30 16:01 ` Maxime Ripard
2017-06-30 16:01 ` Maxime Ripard
2017-06-30 9:34 ` Maxime Ripard
2017-06-30 9:34 ` Maxime Ripard
2017-06-30 9:34 ` Maxime Ripard
2017-06-30 9:58 ` Jonathan Liu
2017-06-30 9:58 ` Jonathan Liu
2017-06-30 9:58 ` Jonathan Liu
2017-07-01 6:29 ` Jonathan Liu
2017-07-01 6:29 ` Jonathan Liu
2017-07-01 6:29 ` Jonathan Liu
2017-06-28 22:06 ` kbuild test robot
2017-06-28 22:06 ` kbuild test robot
2017-06-28 22:06 ` kbuild test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.