Message-ID: <CAOAejn2eOy2sn1VkE979ne23Sj9L6+kaQDNpL1EUKb2m=6sGXw@mail.gmail.com>
Date:   Wed, 11 Jan 2017 14:58:44 +0100
From:   "M'boumba Cedric Madianga" <cedric.madianga@...il.com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     Wolfram Sang <wsa@...-dreams.de>, Rob Herring <robh+dt@...nel.org>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Patrice Chotard <patrice.chotard@...com>,
        Russell King <linux@...linux.org.uk>,
        linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

Hi Uwe,

2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@...gutronix.de>:
> 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/  |

Powered by blists - more mailing lists