| Message ID | 20170614152933.19482-1-net147@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 15, 2017 at 01:29:33AM +1000, Jonathan Liu wrote: > 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 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 | 21 ++++ > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++------------- > drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 +++++++++++++++++++++++++++++++++ > 4 files changed, 253 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..018af9cbb593 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,30 @@ > #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_UNDERFLOW 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) > + The indentation of those bits and the ones above are weird, please check them. > #define SUN4I_HDMI_DDC_FIFO_DATA_REG 0x518 > #define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c > > #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) > @@ -123,6 +140,7 @@ > #define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE BIT(8) > > #define SUN4I_HDMI_DDC_FIFO_SIZE 16 > +#define SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE 1023 Where is that limit mentionned ? > > enum sun4i_hdmi_pkt_type { > SUN4I_HDMI_PKT_AVI = 2, > @@ -146,6 +164,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 +173,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 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..57a9a7a13e02 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(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..e82fa8a55165 > --- /dev/null > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c > @@ -0,0 +1,220 @@ > +/* > + * Copyright (C) 2017 Jonathan Liu > + * > + * 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_UNDERFLOW | \ > + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \ > + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \ > + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \ > +) > + > +static int wait_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 flag) > +{ > + /* 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; > + ktime_t wait_timeout = ktime_add_us(ktime_get(), 100000); > + u32 reg; > + > + for (;;) { > + /* Check for errors */ > + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > + if (reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) { > + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > + return -EIO; > + } > + > + /* Check if requested FIFO flag is unset */ > + reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG); > + if (!(reg & flag)) > + return 0; > + > + /* Timeout */ > + if (ktime_compare(ktime_get(), wait_timeout) > 0) > + return -EIO; > + > + /* Wait for 1-2 bytes to transfer */ > + usleep_range(byte_time, 2 * byte_time); > + } > + > + return -EIO; > +} > + > +static int wait_fifo_read_ready(struct sun4i_hdmi *hdmi) > +{ > + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY); > +} > + > +static int wait_fifo_write_ready(struct sun4i_hdmi *hdmi) > +{ > + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_FULL); > +} Both of these can use readl_poll_timeout, and I'm not sure you need to be that aggressive in your timings. > + > +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg) > +{ > + u32 reg; > + int i; > + > + 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); > + > + 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); > + > + writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->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); > + > + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG, > + reg, > + !(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR), > + 100, 100000)) > + return -EIO; > + > + writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG); > + 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); > + > + 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 (msg->flags & I2C_M_RD) { > + for (i = 0; i < msg->len; i++) { > + if (wait_fifo_read_ready(hdmi)) > + return -EIO; it seems weird that a function called read_ready would return 1 if there's an error. > + > + *msg->buf++ = readb(hdmi->base + > + SUN4I_HDMI_DDC_FIFO_DATA_REG); Don't you have an hardware FIFO you can rely on intead of reading everything byte per byte? > + } > + } else { > + for (i = 0; i < msg->len; i++) { > + if (wait_fifo_write_ready(hdmi)) > + return -EIO; > + > + writeb(*msg->buf++, hdmi->base + > + SUN4I_HDMI_DDC_FIFO_DATA_REG); > + } > + } > + > + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, > + reg, > + !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD), > + 100, 100000)) > + return -EIO; > + > + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > + > + /* Check for errors */ > + if ((reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) || > + !(reg & SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE)) { > + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > + 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_MAX_TRANSFER_SIZE) > + 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, > +}; > + > +static struct i2c_adapter sun4i_hdmi_i2c_adapter = { > + .owner = THIS_MODULE, > + .class = I2C_CLASS_DDC, > + .algo = &sun4i_hdmi_i2c_algorithm, > + .name = "sun4i_hdmi_i2c adapter", > +}; const? (plus the changes mentionned in the v2 I just replied to, if they still apply). Maxime
Hi Maxime, On 21 June 2017 at 18:51, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Thu, Jun 15, 2017 at 01:29:33AM +1000, Jonathan Liu wrote: >> 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 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 | 21 ++++ >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++------------- >> drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 +++++++++++++++++++++++++++++++++ >> 4 files changed, 253 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..018af9cbb593 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,30 @@ >> #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_UNDERFLOW 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) >> + > > The indentation of those bits and the ones above are weird, please > check them. > I will fix the inconsistent indentation. I only just realised the register field bits are indented more than the registry addresses. >> #define SUN4I_HDMI_DDC_FIFO_DATA_REG 0x518 >> #define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c >> >> #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) >> @@ -123,6 +140,7 @@ >> #define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE BIT(8) >> >> #define SUN4I_HDMI_DDC_FIFO_SIZE 16 >> +#define SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE 1023 > > Where is that limit mentionned ? It is the width of the DDC_Access_Data_Byte_Number field in DDC_Byte_Counter register. How about moving it and renaming it like this? #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) Grouping it and having it similar name would be clearer. > >> >> enum sun4i_hdmi_pkt_type { >> SUN4I_HDMI_PKT_AVI = 2, >> @@ -146,6 +164,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 +173,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 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..57a9a7a13e02 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(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..e82fa8a55165 >> --- /dev/null >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c >> @@ -0,0 +1,220 @@ >> +/* >> + * Copyright (C) 2017 Jonathan Liu >> + * >> + * 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_UNDERFLOW | \ >> + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \ >> + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \ >> + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \ >> +) >> + >> +static int wait_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 flag) >> +{ >> + /* 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; >> + ktime_t wait_timeout = ktime_add_us(ktime_get(), 100000); >> + u32 reg; >> + >> + for (;;) { >> + /* Check for errors */ >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); >> + if (reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) { >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); >> + return -EIO; >> + } >> + >> + /* Check if requested FIFO flag is unset */ >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG); >> + if (!(reg & flag)) >> + return 0; >> + >> + /* Timeout */ >> + if (ktime_compare(ktime_get(), wait_timeout) > 0) >> + return -EIO; >> + >> + /* Wait for 1-2 bytes to transfer */ >> + usleep_range(byte_time, 2 * byte_time); >> + } >> + >> + return -EIO; >> +} >> + >> +static int wait_fifo_read_ready(struct sun4i_hdmi *hdmi) >> +{ >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY); >> +} >> + >> +static int wait_fifo_write_ready(struct sun4i_hdmi *hdmi) >> +{ >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_FULL); >> +} > > Both of these can use readl_poll_timeout, and I'm not sure you need to > be that aggressive in your timings. > I have set my timings to minimize communication delays - e.g. when userspace is reading from I2C one byte at a time (like i2cdump from i2c-tools). readl_poll_timeout can't be used to check 2 registers at once and I couldn't get DDC_FIFO_Request_Interrupt_Status_Bit in DDC_Int_Status register to behave properly. I also wanted to minimize the chance of FIFO underrun/overrun. >> + >> +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg) >> +{ >> + u32 reg; >> + int i; >> + >> + 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); >> + >> + 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); >> + >> + writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->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); >> + >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG, >> + reg, >> + !(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR), >> + 100, 100000)) >> + return -EIO; >> + >> + writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG); >> + 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); >> + >> + 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 (msg->flags & I2C_M_RD) { >> + for (i = 0; i < msg->len; i++) { >> + if (wait_fifo_read_ready(hdmi)) >> + return -EIO; > > it seems weird that a function called read_ready would return 1 if > there's an error. I will change this so wait_fifo_flag_unset returns false on error and true on success. Then the wait_fifo_read_ready and wait_fifo_write_ready conditions will be inverted. > >> + >> + *msg->buf++ = readb(hdmi->base + >> + SUN4I_HDMI_DDC_FIFO_DATA_REG); > > Don't you have an hardware FIFO you can rely on intead of reading > everything byte per byte? > Hardware FIFO by nature is transferring one byte at a time. This is my first kernel driver so I still have a lot to learn. In the future it can be improved by using DMA and interrupts. >> + } >> + } else { >> + for (i = 0; i < msg->len; i++) { >> + if (wait_fifo_write_ready(hdmi)) >> + return -EIO; >> + >> + writeb(*msg->buf++, hdmi->base + >> + SUN4I_HDMI_DDC_FIFO_DATA_REG); >> + } >> + } >> + >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, >> + reg, >> + !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD), >> + 100, 100000)) >> + return -EIO; >> + >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); >> + >> + /* Check for errors */ >> + if ((reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) || >> + !(reg & SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE)) { >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); >> + 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_MAX_TRANSFER_SIZE) >> + 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, >> +}; >> + >> +static struct i2c_adapter sun4i_hdmi_i2c_adapter = { >> + .owner = THIS_MODULE, >> + .class = I2C_CLASS_DDC, >> + .algo = &sun4i_hdmi_i2c_algorithm, >> + .name = "sun4i_hdmi_i2c adapter", >> +}; > > const? i2c_add_adapter and i2c_del_adapter take "struct i2c_adapter *adapter" argument so I can't make it const. > > (plus the changes mentionned in the v2 I just replied to, if they > still apply). Ok. > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Regards, Jonathan
On Wed, Jun 21, 2017 at 07:42:47PM +1000, Jonathan Liu wrote: > >> +static int wait_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 flag) > >> +{ > >> + /* 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; > >> + ktime_t wait_timeout = ktime_add_us(ktime_get(), 100000); > >> + u32 reg; > >> + > >> + for (;;) { > >> + /* Check for errors */ > >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > >> + if (reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) { > >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > >> + return -EIO; > >> + } > >> + > >> + /* Check if requested FIFO flag is unset */ > >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG); > >> + if (!(reg & flag)) > >> + return 0; > >> + > >> + /* Timeout */ > >> + if (ktime_compare(ktime_get(), wait_timeout) > 0) > >> + return -EIO; > >> + > >> + /* Wait for 1-2 bytes to transfer */ > >> + usleep_range(byte_time, 2 * byte_time); > >> + } > >> + > >> + return -EIO; > >> +} > >> + > >> +static int wait_fifo_read_ready(struct sun4i_hdmi *hdmi) > >> +{ > >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY); > >> +} > >> + > >> +static int wait_fifo_write_ready(struct sun4i_hdmi *hdmi) > >> +{ > >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_FULL); > >> +} > > > > Both of these can use readl_poll_timeout, and I'm not sure you need to > > be that aggressive in your timings. > > > I have set my timings to minimize communication delays - e.g. when > userspace is reading from I2C one byte at a time (like i2cdump from > i2c-tools). I wouldn't try to optimise that too much. There's already a lot of overhead, it's way inefficient, and it's not really a significant use case anyway. > readl_poll_timeout can't be used to check 2 registers at > once and I couldn't get DDC_FIFO_Request_Interrupt_Status_Bit in > DDC_Int_Status register to behave properly. Can't you just use readl_poll_timeout, and then if it timed out check for errors? > I also wanted to minimize the chance of FIFO underrun/overrun. > > >> + > >> +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg) > >> +{ > >> + u32 reg; > >> + int i; > >> + > >> + 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); > >> + > >> + 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); > >> + > >> + writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->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); > >> + > >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG, > >> + reg, > >> + !(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR), > >> + 100, 100000)) > >> + return -EIO; > >> + > >> + writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG); > >> + 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); > >> + > >> + 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 (msg->flags & I2C_M_RD) { > >> + for (i = 0; i < msg->len; i++) { > >> + if (wait_fifo_read_ready(hdmi)) > >> + return -EIO; > > > > it seems weird that a function called read_ready would return 1 if > > there's an error. > > I will change this so wait_fifo_flag_unset returns false on error > and true on success. Then the wait_fifo_read_ready and > wait_fifo_write_ready conditions will be inverted. Or just invert the return value, whatever makes sense > > > >> + > >> + *msg->buf++ = readb(hdmi->base + > >> + SUN4I_HDMI_DDC_FIFO_DATA_REG); > > > > Don't you have an hardware FIFO you can rely on intead of reading > > everything byte per byte? > > > Hardware FIFO by nature is transferring one byte at a time. This is my > first kernel driver so I still have a lot to learn. In the future it > can be improved by using DMA and interrupts. Yes, you can read one byte at a time, but you don't need to check whether you're ready each byte you're reading. Can't you use FIFO_LEVEL or the byte counter to read the number of bytes you need to read in a single shot ? > >> + } > >> + } else { > >> + for (i = 0; i < msg->len; i++) { > >> + if (wait_fifo_write_ready(hdmi)) > >> + return -EIO; > >> + > >> + writeb(*msg->buf++, hdmi->base + > >> + SUN4I_HDMI_DDC_FIFO_DATA_REG); > >> + } > >> + } > >> + > >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, > >> + reg, > >> + !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD), > >> + 100, 100000)) > >> + return -EIO; > >> + > >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > >> + > >> + /* Check for errors */ > >> + if ((reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) || > >> + !(reg & SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE)) { > >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); > >> + 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_MAX_TRANSFER_SIZE) > >> + 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, > >> +}; > >> + > >> +static struct i2c_adapter sun4i_hdmi_i2c_adapter = { > >> + .owner = THIS_MODULE, > >> + .class = I2C_CLASS_DDC, > >> + .algo = &sun4i_hdmi_i2c_algorithm, > >> + .name = "sun4i_hdmi_i2c adapter", > >> +}; > > > > const? > > i2c_add_adapter and i2c_del_adapter take "struct i2c_adapter *adapter" > argument so I can't make it const. Ah, right, they modify it. You can't allocate it statically then, otherwise if you get two instances, they'll both step on each other's toe. Maxime
Hi Maxime, On 22 June 2017 at 07:26, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Wed, Jun 21, 2017 at 07:42:47PM +1000, Jonathan Liu wrote: >> >> +static int wait_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 flag) >> >> +{ >> >> + /* 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; >> >> + ktime_t wait_timeout = ktime_add_us(ktime_get(), 100000); >> >> + u32 reg; >> >> + >> >> + for (;;) { >> >> + /* Check for errors */ >> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); >> >> + if (reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) { >> >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); >> >> + return -EIO; >> >> + } >> >> + >> >> + /* Check if requested FIFO flag is unset */ >> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG); >> >> + if (!(reg & flag)) >> >> + return 0; >> >> + >> >> + /* Timeout */ >> >> + if (ktime_compare(ktime_get(), wait_timeout) > 0) >> >> + return -EIO; >> >> + >> >> + /* Wait for 1-2 bytes to transfer */ >> >> + usleep_range(byte_time, 2 * byte_time); >> >> + } >> >> + >> >> + return -EIO; >> >> +} >> >> + >> >> +static int wait_fifo_read_ready(struct sun4i_hdmi *hdmi) >> >> +{ >> >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY); >> >> +} >> >> + >> >> +static int wait_fifo_write_ready(struct sun4i_hdmi *hdmi) >> >> +{ >> >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_FULL); >> >> +} >> > >> > Both of these can use readl_poll_timeout, and I'm not sure you need to >> > be that aggressive in your timings. >> > >> I have set my timings to minimize communication delays - e.g. when >> userspace is reading from I2C one byte at a time (like i2cdump from >> i2c-tools). > > I wouldn't try to optimise that too much. There's already a lot of > overhead, it's way inefficient, and it's not really a significant use > case anyway. > Ok. >> readl_poll_timeout can't be used to check 2 registers at >> once and I couldn't get DDC_FIFO_Request_Interrupt_Status_Bit in >> DDC_Int_Status register to behave properly. > > Can't you just use readl_poll_timeout, and then if it timed out check > for errors? > I think it is more flexible this way as I can adjust the usleep_range min. >> I also wanted to minimize the chance of FIFO underrun/overrun. >> >> >> + >> >> +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg) >> >> +{ >> >> + u32 reg; >> >> + int i; >> >> + >> >> + 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); >> >> + >> >> + 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); >> >> + >> >> + writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->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); >> >> + >> >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG, >> >> + reg, >> >> + !(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR), >> >> + 100, 100000)) >> >> + return -EIO; >> >> + >> >> + writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG); >> >> + 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); >> >> + >> >> + 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 (msg->flags & I2C_M_RD) { >> >> + for (i = 0; i < msg->len; i++) { >> >> + if (wait_fifo_read_ready(hdmi)) >> >> + return -EIO; >> > >> > it seems weird that a function called read_ready would return 1 if >> > there's an error. >> >> I will change this so wait_fifo_flag_unset returns false on error >> and true on success. Then the wait_fifo_read_ready and >> wait_fifo_write_ready conditions will be inverted. > > Or just invert the return value, whatever makes sense > This will actually be different as I will rework it to use FIFO_LEVEL. >> > >> >> + >> >> + *msg->buf++ = readb(hdmi->base + >> >> + SUN4I_HDMI_DDC_FIFO_DATA_REG); >> > >> > Don't you have an hardware FIFO you can rely on intead of reading >> > everything byte per byte? >> > >> Hardware FIFO by nature is transferring one byte at a time. This is my >> first kernel driver so I still have a lot to learn. In the future it >> can be improved by using DMA and interrupts. > > Yes, you can read one byte at a time, but you don't need to check > whether you're ready each byte you're reading. Can't you use > FIFO_LEVEL or the byte counter to read the number of bytes you need to > read in a single shot ? > Ok. I will check FIFO_LEVEL and change readb/writeb to readsb/writesb. >> >> + } >> >> + } else { >> >> + for (i = 0; i < msg->len; i++) { >> >> + if (wait_fifo_write_ready(hdmi)) >> >> + return -EIO; >> >> + >> >> + writeb(*msg->buf++, hdmi->base + >> >> + SUN4I_HDMI_DDC_FIFO_DATA_REG); >> >> + } >> >> + } >> >> + >> >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, >> >> + reg, >> >> + !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD), >> >> + 100, 100000)) >> >> + return -EIO; >> >> + >> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); >> >> + >> >> + /* Check for errors */ >> >> + if ((reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) || >> >> + !(reg & SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE)) { >> >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); >> >> + 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_MAX_TRANSFER_SIZE) >> >> + 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, >> >> +}; >> >> + >> >> +static struct i2c_adapter sun4i_hdmi_i2c_adapter = { >> >> + .owner = THIS_MODULE, >> >> + .class = I2C_CLASS_DDC, >> >> + .algo = &sun4i_hdmi_i2c_algorithm, >> >> + .name = "sun4i_hdmi_i2c adapter", >> >> +}; >> > >> > const? >> >> i2c_add_adapter and i2c_del_adapter take "struct i2c_adapter *adapter" >> argument so I can't make it const. > > Ah, right, they modify it. You can't allocate it statically then, > otherwise if you get two instances, they'll both step on each other's > toe. > I will make it const and use devm_kmemdup. > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Regards, Jonathan
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..018af9cbb593 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,30 @@ #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_UNDERFLOW 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_DATA_REG 0x518 #define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c #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) @@ -123,6 +140,7 @@ #define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE BIT(8) #define SUN4I_HDMI_DDC_FIFO_SIZE 16 +#define SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE 1023 enum sun4i_hdmi_pkt_type { SUN4I_HDMI_PKT_AVI = 2, @@ -146,6 +164,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 +173,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 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..57a9a7a13e02 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(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..e82fa8a55165 --- /dev/null +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c @@ -0,0 +1,220 @@ +/* + * Copyright (C) 2017 Jonathan Liu + * + * 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_UNDERFLOW | \ + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \ + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \ + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \ +) + +static int wait_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 flag) +{ + /* 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; + ktime_t wait_timeout = ktime_add_us(ktime_get(), 100000); + u32 reg; + + for (;;) { + /* Check for errors */ + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); + if (reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) { + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); + return -EIO; + } + + /* Check if requested FIFO flag is unset */ + reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG); + if (!(reg & flag)) + return 0; + + /* Timeout */ + if (ktime_compare(ktime_get(), wait_timeout) > 0) + return -EIO; + + /* Wait for 1-2 bytes to transfer */ + usleep_range(byte_time, 2 * byte_time); + } + + return -EIO; +} + +static int wait_fifo_read_ready(struct sun4i_hdmi *hdmi) +{ + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY); +} + +static int wait_fifo_write_ready(struct sun4i_hdmi *hdmi) +{ + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_FULL); +} + +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg) +{ + u32 reg; + int i; + + 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); + + 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); + + writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->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); + + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG, + reg, + !(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR), + 100, 100000)) + return -EIO; + + writel(msg->len, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG); + 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); + + 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 (msg->flags & I2C_M_RD) { + for (i = 0; i < msg->len; i++) { + if (wait_fifo_read_ready(hdmi)) + return -EIO; + + *msg->buf++ = readb(hdmi->base + + SUN4I_HDMI_DDC_FIFO_DATA_REG); + } + } else { + for (i = 0; i < msg->len; i++) { + if (wait_fifo_write_ready(hdmi)) + return -EIO; + + writeb(*msg->buf++, hdmi->base + + SUN4I_HDMI_DDC_FIFO_DATA_REG); + } + } + + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, + reg, + !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD), + 100, 100000)) + return -EIO; + + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); + + /* Check for errors */ + if ((reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) || + !(reg & SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE)) { + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); + 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_MAX_TRANSFER_SIZE) + 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, +}; + +static struct i2c_adapter sun4i_hdmi_i2c_adapter = { + .owner = THIS_MODULE, + .class = I2C_CLASS_DDC, + .algo = &sun4i_hdmi_i2c_algorithm, + .name = "sun4i_hdmi_i2c adapter", +}; + +int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi) +{ + int ret = 0; + + ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk); + if (ret) + return ret; + + i2c_set_adapdata(&sun4i_hdmi_i2c_adapter, hdmi); + + ret = i2c_add_adapter(&sun4i_hdmi_i2c_adapter); + if (ret) + return ret; + + hdmi->i2c = &sun4i_hdmi_i2c_adapter; + + return ret; +}
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 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 | 21 ++++ drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++------------- drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 +++++++++++++++++++++++++++++++++ 4 files changed, 253 insertions(+), 90 deletions(-) create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c