[i2c, sw, hal] Added functions to calculate timing params for all supported speeds and correct SCL high time#565
Conversation
KinzaQamar
force-pushed
the
scl_high_time_calc
branch
from
b7055d9 to
a5d3443
Compare
KinzaQamar
requested review from
elliotb-lowrisc,
engdoreis,
marnovandermaas and
martin-velay
KinzaQamar
force-pushed
the
scl_high_time_calc
branch
from
a5d3443 to
dce994d
Compare
KinzaQamar
force-pushed
the
scl_high_time_calc
branch
2 times, most recently
from
b625e70 to
c2f861e
Compare
engdoreis
left a comment
engdoreis
left a comment
There was a problem hiding this comment.
I think that we should bite the bullet and port the Opentitan dif_i2c to mocha. Starting by the timing functions.
You're right. I'll do this in a separate PR |
KinzaQamar
force-pushed
the
scl_high_time_calc
branch
2 times, most recently
from
5456433 to
d1fcf2d
Compare
I've done it as part of this PR |
KinzaQamar
changed the title
KinzaQamar
force-pushed
the
scl_high_time_calc
branch
2 times, most recently
from
d709543 to
a7e6856
Compare
ziuziakowska
left a comment
ziuziakowska
left a comment
There was a problem hiding this comment.
Just some style nits.
| typedef enum { standard_mode, fast_mode, fast_plus_mode } i2c_speed_t; | ||
|
|
||
| typedef struct { | ||
| uint32_t rise_cycles; | ||
| uint32_t fall_cycles; | ||
| uint32_t scl_low_cycles; | ||
| uint32_t scl_high_cycles; | ||
| uint32_t scl_period_cycles; | ||
| uint32_t setup_start_cycles; | ||
| uint32_t hold_start_cycles; | ||
| uint32_t setup_data_cycles; | ||
| uint32_t hold_data_cycles; | ||
| uint32_t setup_stop_cycles; | ||
| uint32_t bus_free_time_cycles; | ||
| } i2c_timing_params_cycles_t; |
There was a problem hiding this comment.
I would prefer that enums and structs are not typedef-ed out (with some exceptions, e.g volatile-qualified pointers for devices like i2c_t)
There was a problem hiding this comment.
These typedefs are going to be used as function arguments and return types. Isn't it nice to make them a type and use them wherever required?
| uint32_t hold_data_cycles; | ||
| uint32_t setup_stop_cycles; | ||
| uint32_t bus_free_time_cycles; | ||
| } i2c_timing_params_cycles_t; |
There was a problem hiding this comment.
I think cycles in the type name is redundant as the units are in the struct member names already. Perhaps this should be i2c_timing_parameters?
There was a problem hiding this comment.
Just naming it i2c_timing_paramaters will get misunderstood as timing parameters in ns
KinzaQamar
force-pushed
the
scl_high_time_calc
branch
4 times, most recently
from
b625e70 to
fb1bd95
Compare
It takes care that the resultant SCL high cycles must satisfy below
equations:
** scl_high_cycles >= 4 (to support correct function in clock
streching)
** scl_high_cycles = scl_period_cycles - (rise_cycles + fall_cycles
+ scl_low_cycles)
Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>
OT's I2C block supports Standard, Fast and Fast Plus modes. Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>
KinzaQamar
force-pushed
the
scl_high_time_calc
branch
from
fb1bd95 to
c00e4a9
Compare
|
Thanks for your review. I've addressed your comments and left some questions. |
3 participants
First commit takes care that the resultant SCL high cycles must satisfy below equations:
** scl_high_cycles >= 4 (to support correct function in clock streching)
** scl_high_cycles = scl_period_cycles - (rise_cycles + fall_cycles
+ scl_low_cycles)
Second commit added calculations for each supported timing parameters