Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

>
>> +              */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>
> You could get rid of this, when caching the value of CR1. Would save two
> register reads here. This doesn't work for all registers, but it should
> be possible to apply for most of them, maybe enough to get rid of the
> clr_bits and set_bits function.

I agree at many places I could save registers read by not using
clr_bits and set_bits function when the registers in question has been
already read.
But it is not enough to get rid of the clr_bits and set_bits function.
For example when calling stm32f4_i2c_terminate_xfer(), the CR1
register is never read before so set_bits function is useful.
Another example, when stm32f4_i2c_handle_rx_done(), the CR1 register
is also never read before so clr_bits finction is again useful.

2017-01-11 14:58 GMT+01:00 M'boumba Cedric Madianga <cedric.madianga@xxxxxxxxx>:
> Hi Uwe,
>
> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-KÃnig <u.kleine-koenig@xxxxxxxxxxxxxx>:
>> Hello Cedric,
>>
>> On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>>> +/*
>>> + * In standard mode:
>>> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
>>> + *
>>> + * In fast mode:
>>> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
>>> + *           SCL low period  = 2  * CCR * I2C parent clk period
>>> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
>>> + *           SCL low period  = 16 * CCR * I2C parent clk period
>> s/  \*/ */ several times
>
> Sorry but I don't see where is the issue as the style for multi-line
> comments seems ok.
> Could you please clarify that point if possible ? Thanks in advance
>
>>
>>> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
>>> + * Duty = 1
>>> + *
>>> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>>> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
>> s/mode/Mode/
>
> ok thanks
>
>>
>>> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
>>> + * constraints defined by i2c bus specification
>>
>> I don't understand scl_period = 1 Âs for Fast Mode. For a bus freqency
>> of 400 kHz we need low + high = 2.5 Âs. Is there a factor 10 missing
>> somewhere?
>
> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
> 2Mhz and SCL_period = 1 we have:
> CCR = 1 * 2Mhz = 2.
> But to compute, scl_low and scl_high in Fast mode, we have to do the
> following thing as Duty=1:
> scl_high = 9 * CCR * I2C parent clk period
> scl_low = 16 * CCR * I2C parent clk period
> In our example:
> scl_high = 9 * 2 * 0,0000005 = 0,000009 sec = 9 Âs
> scl_low = 16 * 2 * 0.0000005 = 0,000016 sec = 16 Âs
> So low + high = 27 Âs > 2,5 Âs
>
>>
>>> + */
>>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>>> [...]
>>> +
>>> +/**
>>> + * stm32f4_i2c_hw_config() - Prepare I2C block
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +     int ret = 0;
>>> +
>>> +     /* Disable I2C */
>>> +     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>>> +
>>> +     ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     stm32f4_i2c_set_rise_time(i2c_dev);
>>> +
>>> +     stm32f4_i2c_set_speed_mode(i2c_dev);
>>> +
>>> +     stm32f4_i2c_set_filter(i2c_dev);
>>> +
>>> +     /* Enable I2C */
>>> +     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
>>
>> This function is called after a hw reset, so there should be no need to
>> use clr_bits and set_bits because the value read from hw should be
>> known.
>
> ok thanks
>
>>
>>> +     return ret;
>>
>> return 0;
>
> ok thanks
>
>>
>>> +}
>>> +
>>> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     u32 status;
>>> +     int ret;
>>> +
>>> +     ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>>> +                                      status,
>>> +                                      !(status & STM32F4_I2C_SR2_BUSY),
>>> +                                      10, 1000);
>>> +     if (ret) {
>>> +             dev_dbg(i2c_dev->dev, "bus not free\n");
>>> +             ret = -EBUSY;
>>> +     }
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
>>> + * @i2c_dev: Controller's private data
>>> + * @byte: Data to write in the register
>>> + */
>>> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
>>> +{
>>> +     writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
>>> + * @i2c_dev: Controller's private data
>>> + *
>>> + * This function fills the data register with I2C transfer buffer
>>> + */
>>> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +
>>> +     stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
>>> +     msg->count--;
>>> +}
>>> +
>>> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     u32 rbuf;
>>> +
>>> +     rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>>> +     *msg->buf++ = rbuf & 0xff;
>>
>> This is unnecessary. buf has an 8 bit wide type so
>>
>>         *msg->buf++ = rbuf;
>>
>> has the same effect. (ISTR this is something I already pointed out
>> earlier?)
>
> Yes you are right.
>
>>
>>> +     msg->count--;
>>> +}
>>> +
>>> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> +
>>> +     stm32f4_i2c_disable_irq(i2c_dev);
>>> +
>>> +     reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +     if (msg->stop)
>>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>>> +     else
>>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>>> +
>>> +     complete(&i2c_dev->complete);
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> +
>>> +     if (msg->count) {
>>> +             stm32f4_i2c_write_msg(i2c_dev);
>>> +             if (!msg->count) {
>>> +                     /* Disable buffer interrupts for RXNE/TXE events */
>>> +                     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>>> +             }
>>> +     } else {
>>> +             stm32f4_i2c_terminate_xfer(i2c_dev);
>>
>> Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
>> yes, is it then right to set STM32F4_I2C_CR1_STOP or
>> STM32F4_I2C_CR1_START?
>
> If arbitration is lost, stm32f4_i2c_terminate_xfer() is not called.
> In that case, we return -EAGAIN and i2c-core will retry by calling
> stm32f4_i2c_xfer()
>
>>
>>> +     }
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> +
>>> +     switch (msg->count) {
>>> +     case 1:
>>> +             stm32f4_i2c_disable_irq(i2c_dev);
>>> +             stm32f4_i2c_read_msg(i2c_dev);
>>> +             complete(&i2c_dev->complete);
>>> +             break;
>>> +     /*
>>> +      * For 2 or 3-byte reception, we do not have to read the data register
>>> +      * when RXNE occurs as we have to wait for byte transferred finished
>>
>> it's hard to understand because if you don't know the hardware the
>> meaning of RXNE is unknown.
>
> Ok I will replace RXNE by RX not empty in that comment
>
>>
>>> +      * event before reading data. So, here we just disable buffer
>>> +      * interrupt in order to avoid another system preemption due to RXNE
>>> +      * event
>>> +      */
>>> +     case 2:
>>> +     case 3:
>>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>>> +             break;
>>> +     /* For N byte reception with N > 3 we directly read data register */
>>> +     default:
>>> +             stm32f4_i2c_read_msg(i2c_dev);
>>> +     }
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
>>> + * in case of read
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>
>> btf is a hw-related name. Maybe better use _done which is easier to
>> understand?
>
> OK
>
>>
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     void __iomem *reg;
>>> +     u32 mask;
>>> +     int i;
>>> +
>>> +     switch (msg->count) {
>>> +     case 2:
>>> +             /*
>>> +              * In order to correctly send the Stop or Repeated Start
>>> +              * condition on the I2C bus, the STOP/START bit has to be set
>>> +              * before reading the last two bytes.
>>> +              * After that, we could read the last two bytes, disable
>>> +              * remaining interrupts and notify the end of xfer to the
>>> +              * client
>>
>> This is surprising. I didn't recheck the manual, but that looks very
>> uncomfortable.
>
> I agree but this exactly the hardware way of working described in the
> reference manual.
>
>>How does this work, when I only want to read a single
>> byte? Same problem for ACK below.
>
> For a single reception, we enable NACK and STOP or Repeatead START
> bits during address match.
> The NACK and STOP/START pulses are sent as soon as the data is
> received in the shift register.
> Please note that in that case, we don't have to wait BTF event to read the data.
> Data is read as soon as RXNE event occurs.
>
>>
>>> +              */
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +             if (msg->stop)
>>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>>> +             else
>>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>>> +
>>> +             for (i = 2; i > 0; i--)
>>> +                     stm32f4_i2c_read_msg(i2c_dev);
>>> +
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> +             mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>>> +             stm32f4_i2c_clr_bits(reg, mask);
>>> +
>>> +             complete(&i2c_dev->complete);
>>> +             break;
>>> +     case 3:
>>> +             /*
>>> +              * In order to correctly send the ACK on the I2C bus for the
>>> +              * last two bytes, we have to set ACK bit before reading the
>>> +              * third last data byte
>>> +              */
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>>> +             stm32f4_i2c_read_msg(i2c_dev);
>>> +             break;
>>> +     default:
>>> +             stm32f4_i2c_read_msg(i2c_dev);
>>> +     }
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
>>> + * master receiver
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     void __iomem *reg;
>>> +
>>> +     switch (msg->count) {
>>> +     case 0:
>>> +             stm32f4_i2c_terminate_xfer(i2c_dev);
>>> +             /* Clear ADDR flag */
>>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>>> +             break;
>>> +     case 1:
>>> +             /*
>>> +              * Single byte reception:
>>
>> This also happens for the last byte of a 5 byte transfer, right?
>
> For a 5 byte transfer the behavior is different:
> We have to read data from DR (data register)  as soon as the RXNE (RX
> not empty) event occurs for data1, data2 and data3 (until N-2 data for
> a more generic case)
> The ACK is automatically sent as soon as the data is received in the
> shift register as the I2C controller was configured to do that during
> adress match phase.
>
> For data3 (N-2 data), we wait for BTF (Byte Transfer finished) event
> in order to set NACK before reading DR.
> This event occurs when a new data has been received in shift register
> (in our case data4 or N-1 data) but the prevoius data in DR (in our
> case data3 or N-2 data) has not been read yet.
> In that way, the NACK pulse will be correctly generated after the last
> received data byte.
>
> For data4 and data5, we wait for BTF event (data4 or N-1 data in DR
> and data5 or N data in shift register), set STOP or repeated Start in
> order to correctly sent the right pulse after the last received data
> byte and run 2 consecutives read of DR.
>
>>
>>> +              * Enable NACK, clear ADDR flag and generate STOP or RepSTART
>>> +              */
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>>> +             if (msg->stop)
>>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>>> +             else
>>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>>> +             break;
>>> +     case 2:
>>> +             /*
>>> +              * 2-byte reception:
>>> +              * Enable NACK and set POS
>>
>> What is POS?
> POS is used to define the position of the (N)ACK pulse
> 0: ACK is generated when the current is being received in the shift register
> 1: ACK is generated when the next byte which will be received in the
> shift register (used for 2-byte reception)
>
>>
>>> +              */
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>>
>> You could get rid of this, when caching the value of CR1. Would save two
>> register reads here. This doesn't work for all registers, but it should
>> be possible to apply for most of them, maybe enough to get rid of the
>> clr_bits and set_bits function.
>>
>>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>>> +             break;
>>> +
>>> +     default:
>>> +             /* N-byte reception: Enable ACK */
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
>>
>> Do you need to set ACK for each byte transferred?
> I need to do that in order to be SMBus compatible and the ACK/NACK
> seems to be used by default in Documentation/i2c/i2c-protocol file.
>
>>
>> I stopp reviewing here because of -ENOTIME on my side but don't want to
>> delay discussion, so sent my comments up to here already now.
>>
>> Best regards
>> Uwe
>>
>> --
>> Pengutronix e.K.                           | Uwe Kleine-KÃnig            |
>> Industrial Linux Solutions                 | http://www.pengutronix.de/  |