| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170621212616.uzmxspziaknxs2ju@flea.lan>
Date: Wed, 21 Jun 2017 23:26:16 +0200
From: Maxime Ripard <maxime.ripard@...e-electrons.com>
To: Jonathan Liu <net147@...il.com>
Cc: David Airlie <airlied@...ux.ie>, Chen-Yu Tsai <wens@...e.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
dri-devel <dri-devel@...ts.freedesktop.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
linux-sunxi <linux-sunxi@...glegroups.com>
Subject: Re: [PATCH v3] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC
bus
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
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)
Powered by blists - more mailing lists