Hi Maxime,
On 22 June 2017 at 07:26, Maxime Ripard
<maxime.ripard@xxxxxxxxxxxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
