Page MenuHomeFreeBSD

i2c support for mediatek soc
Needs ReviewPublic

Authored by yamori813_yahoo.co.jp on Oct 20 2017, 6:01 AM.

Details

Summary

i2c support for rt soc.

Test Plan

user land i2c command and kernel rtc iic module on RT2880

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 12195
Build 12493: arc lint + arc unit

Event Timeline

mizhka requested changes to this revision.Oct 20 2017, 10:20 AM
mizhka added inline comments.
sys/mips/mediatek/mtk_iic.c
156–167

Could you please remove child "iicbus" if not NULL?

mtx_destroy?

The mtx_destroy() function is used to destroy mutex so the data

associated with it may be freed or otherwise overwritten.  Any mutex
which is destroyed must previously have been initialized with mtx_init().
It is permissible to have a single hold count on a mutex when it is
destroyed.  It is not permissible to hold the mutex recursively, or have
another thread blocked on the mutex when it is destroyed.
172–175

"style(9)" :)

397–399

To remove?

418

IICBUS_MINVER, IICBUS_PREFVER, IICBUS_MAXVER ?

sys/mips/mediatek/mtk_iic.h
44–65

Please use space between #define and macro name

70

As I understand, 333MHz is specific for RT2880, isn't it? For any other SoC, it can be different from 333.

This revision now requires changes to proceed.Oct 20 2017, 10:20 AM
ian added a subscriber: ian.Oct 21 2017, 5:31 PM

IMO, this entire driver is wrong. Because of the way the hardware works, it cannot support the start, repeat_start, stop, read, and write methods for iicbus_if, and it must not try to "fake" that support. It must implement only callback, reset, and transfer. The transfer method should be basically just

one time, maybe in reset(), set RA_I2C_CONFIG reg to 
      I2C_CONFIG_ADDRLEN(I2C_CONFIG_ADDRLEN_8) |
      I2C_CONFIG_DEVADLEN(I2C_CONFIG_DEVADLEN_7) |
      I2C_CONFIG_ADDRDIS;

in transfer method:
 for each msg
   set RA_I2C_DEVADDR reg to msg.slave >> 1
   set RA_I2C_BYTECNT reg to msg.len
   set RA_I2C_STARTXFER reg to either OP_READ or OP_WRITE
   loop to transfer all the bytes into/out-of msg.buf

There is an example of a transfer-only driver in sys/arm/broadcom/bcm2835/bcm2835_bsc.c, but it is a bit complex because it uses interrupts, and it uses a hardware quirk to make repeat-start work.

sys/mips/mediatek/mtk_iic.h
44–65

style(9) requires a tab after #define

ian added a reviewer: ian.Oct 21 2017, 5:31 PM
mizhka added inline comments.Oct 21 2017, 6:54 PM
sys/mips/mediatek/mtk_iic.h
44–65

Of course, a tab. Sorry, mistype.

mizhka accepted this revision.Dec 7 2017, 8:13 PM

@sgalabov , @adrian , could you please look at?

This revision is now accepted and ready to land.Dec 7 2017, 8:13 PM
sgalabov requested changes to this revision.Dec 8 2017, 8:48 AM
sgalabov added inline comments.
sys/boot/fdt/dts/mips/fbsd-rt2880.dtsi
2

The dtsi file changes do not belong with this change. They should be in a separate review IMO.
In any case, even then, we should not import the entire file. The rt2880.dtsi is already imported in sys/gnu/dts/mips.
What should be done (and it was done for the other Ralink/Mediatek SoCs) is the following:

  • add the following line at the end of sys/gnu/dts/mips/rt2880.dtsi:

#include "fbsd-rt2880.dtsi"

  • create the file sys/dts/fbsd-rt2880.dtsi and only include what's missing from the original sys/gnu/dts/mips/rt2880.dtsi file.
sys/mips/mediatek/mtk_iic.c
2

I agree with the comment by @ian - I think we should only implement callback, reset and transfer and we should not try to fake what the SoC cannot do.

This revision now requires changes to proceed.Dec 8 2017, 8:48 AM
  • delete dtsi file