Message-ID: <CANwerB2wvRp+6ej=9nACaWKowUFkfV037jTuxYpBLzxA7M1V3Q@mail.gmail.com>
Date:   Sat, 24 Jun 2017 15:59:37 +1000
From:   Jonathan Liu <net147@...il.com>
To:     Maxime Ripard <maxime.ripard@...e-electrons.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

Hi Maxime,

On 22 June 2017 at 07:26, Maxime Ripard
<maxime.ripard@...e-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

Powered by blists - more mailing lists