Hello Cedric,
On Fri, Dec 23, 2016 at 01:41:15PM +0100, M'boumba Cedric Madianga wrote:
> > I don't understand why DUTY is required to reach 400 kHz. Given a parent
> > freq of 30 MHz, with CCR = 25 and DUTY = 0 we have:
> >
> > t_high = 25 * 33.333 ns = 833.333 ns
> > t_low = 2 * 25 * 33.333 ns = 1666.667 ns
> >
> > then t_high and t_low satisfy the i2c bus specification
> > (t_low > 1300 ns, t_high > 600 ns) and we have t_low + t_high = 2500 ns
> > = 1 / 400 kHz.
> >
> > Where is the error?
> Hum ok you are right. I was a bad interpretation of the datasheet.
> So now it is clearer. Thanks for that.
> I will correct and improve my comments in the V8.
The benefit of DUTY=1 is (I think) that you can reach 400 kHz also with
lower parent frequencies. And so it't probably sensible to make use of
it unconditionally for fast mode.
> >> + * So, in order to cover both SCL high/low with Duty = 1,
> >> + * CCR = 16 * SCL period * I2C CLK frequency
> >
> > I don't get that. Actually you need to use low + high, so
> > CCR = parentrate / (25 * 400 kHz), right?
> With your new inputs above, I think I could use a simpler implementation:
> CCR = scl_high_period * parent_rate
> with scl_high_period = 5 µs in standard mode to reach 100khz
> and scl_high_period = 1 µs in fast mode to reach 400khz with 1/2 or
> 16/9 duty cycle.
> So, I am wondering if I have to let the customer setting the duty
> cycle in the DT for example with "st,duty=0" or "st,duty=1" property
> (0 for 1/2 and 1 for 16/9).
> Or perhaps the best option it to use a default value. What is your
> feeling regarding this point ?
No, don't add a property to the device tree. Just take pencil and paper
and think a while about the right algorithm to choose the right register
settings.
> >> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> >> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> >> +
> >> + if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
> >> + trise = freq + 1;
> >> + else
> >> + trise = freq * 300 / 1000 + 1;
> >
> > if freq is big such that freq * 300 overflows does this result in a
> > wrong result, or does the compiler optimize correctly?
> For sure the compiler will never exceeds u32 max value
If I compile
-------->8--------
#include <stdio.h>
void myfunc(unsigned freq)
{
unsigned trise = freq * 300 / 1000 + 1;
printf("trise = %u", trise);
}
-------->8--------
for arm with -O3 I get:
00000000 <myfunc>:
0: e3a01f4b mov r1, #300 ; 0x12c
4: e0010190 mul r1, r0, r1
8: e59f3010 ldr r3, [pc, #16] ; 20 <myfunc+0x20>
c: e59f0010 ldr r0, [pc, #16] ; 24 <myfunc+0x24>
10: e0812193 umull r2, r1, r3, r1
14: e1a01321 lsr r1, r1, #6
18: e2811001 add r1, r1, #1
1c: eafffffe b 0 <printf>
20: 10624dd3 .word 0x10624dd3
24: 00000000 .word 0x00000000
The mul instruction at offset 4 writes the least significant 32 bits of
300 * r0 to r1 and so doesn't handle overflow correctly.
> > This is still not really understandable.
> I have already added some comments from datasheet to explain the
> different cases.
> I don't see how I could be more understandable as it is clearly the
> hardware way of working...
You need to comment the check for the length, something like:
/*
* To end the transfer correctly the foo bit has to be cleared
* already on the last but one byte to be transferred.
*/
and bonus points if you can shrink the number of functions that check
for this stuff.
> > Just do:
> >
> > if (status & STM32F4_I2C_SR1_SB) {
> > ...
> > }
> >
> > if (status & ...) {
> >
> > }
> ok but I would prefer something like that:
> flag = status & possible_status
> if (flag & STM32F4_I2C_SR1_SB) {
> ...
> }
>
> if (flag & ...) {
> }
Ok, if you really need possible_status.
> > Then it's obvious by reading the code in which order they are handled
> > without the need to check the definitions. Do you really need to jugle
> > with possible_status?
> I think I have to use possible_status as some events could occur
> whereas the corresponding interrupt is disabled.
> For example, for a 2 byte-reception, we don't have to take into accout
> RXNE event so the corresponding interrupt is disabled.
Is it possible to make it more obvious by doing:
status = read_from_status_register() & read_from_interrupt_enable_register();
at a single place?
> >> + /* Use __fls() to check error bits first */
> >> + flag = __fls(status & possible_status);
> >> +
> >> + switch (1 << flag) {
> >> + case STM32F4_I2C_SR1_BERR:
> >> + reg = i2c_dev->base + STM32F4_I2C_SR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_BERR);
> >> + msg->result = -EIO;
> >> + break;
> >> +
> >> + case STM32F4_I2C_SR1_ARLO:
> >> + reg = i2c_dev->base + STM32F4_I2C_SR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_ARLO);
> >> + msg->result = -EAGAIN;
> >> + break;
> >> +
> >> + case STM32F4_I2C_SR1_AF:
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> >> + msg->result = -EIO;
> >> + break;
> >> +
> >> + default:
> >> + dev_err(i2c_dev->dev,
> >> + "err it unhandled: status=0x%08x)\n", status);
> >> + return IRQ_NONE;
> >> + }
> >
> > You only check a single irq flag here.
> Yes only the first error could be reported to the i2c clients via
> msg->result that's why I don't check all errors.
> Moreover, as soon as an error occurs, the I2C device is reset.
The the "first" in the comment for __fls is misleading. Also do:
if (status & MOST_IMPORTANT_ERROR) {
...
}
if (status & SECOND_MOST_IMPORTANT_ERROR) {
...
}
(if you need including possible_status stuff).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
