[U-Boot] [PATCH] rockchip: i2c: enable I2C inside GRF for rk3066 and rk3188

[Dr. Philipp Tomsich]

Alexander,

> On 26 Feb 2018, at 10:25, Alexander Kochetkov <al.kochet at gmail.com> wrote:
> 
> Hello, Philipp!
> 
>> 
>> What exactly are you trying to configure here?
>> 
>> Do you need to bring these out of reset, is this some IO config or
>> are these clock gates?  Note that if it’s any of these, then the 
>> respective drivers (i.e. reset, pinctrl, clock) should be modified
>> instead of putting this here. 
>> 
> 
> This is rki2c_sel bit used to switch I2C IP between different implementations.
> Here comment from TRM: "1: rki2c is used instead of old i2c». U-boot and
> kernel I2C drivers want work with legacy IP implementation. 

Given that I would have never guessed: this clearly needs comments and
symbolic constants then ;-)

> The patch is backport from upstream linux and the code was located inside
> I2C driver code.

My gut feeling is that this shouldn’t go into the i2c driver (after all, the device
is not a ‘rki2c’ until this bit is set).  However, if we want it in the i2c driver, we should
have an offset-of for the GRF offset and also make the bit-positions configurable
via the driver-data.

From your description this sounds like a (secondary) reset-bit for the i2c controller.
With a *-u-boot.dtsi, you could even model the GRF-offset and the bit-number via
the device tree.  The only drawback from this is that resets are (not yet) automatically
processed when probing the device in the driver model core implementation.

> 
>>> 
>>> +static const struct rk_i2c_soc_data rk3066_soc_data = {
>>> +	.grf_offset = 0x154,
>> 
>> Please don’t use open-coded constants for something like this.
> 
> This is GRF_SON_CON1 register. A lot of rockchip drivers inside linux kernel
> use following technic to make drivers universal between different chips.
> Logically it is not far away from simple constant definition using define syntax.
> I don’t use that constant inside functions directly.
> 
> I tried to replace constant with following code:
> 
> #include <asm/arch/grf_rk3036.h>
> #include <asm/arch/grf_rk3188.h>
> 
> ...
> 
> static const struct rk_i2c_soc_data rk3066_soc_data = {
> 	.grf_offset = offsetof(struct rk3036_grf, soc_con1),
> };
> 
> static const struct rk_i2c_soc_data rk3188_soc_data = {
> 	.grf_offset = offsetof(struct rk3188_grf, soc_con1),
> };
> 
> But the code wan’t compile, because grf_rk3036.h and grf_rk3188.h both
> define a lot of constants with same name and that produce a lot of build errors.

Yes, that you be preferable.

However, the GRF drivers first need to be cleaned up (i.e. the enums for the
IOMUX definitions need to move into the pinctrl drivers), so only the structure
definitions remain in the GRF drivers.

Some of this has happened lately for some of the more recent chips, but there
hasn’t been a larger effort to do it:
e.g. commit 301fff4e574d373a139dd47aceabc5b4259873da for the RK3328

It would be great if you’d pick this task up for the 3036 and 3188.

> 
> What about if I just add comment to grf_offset definintion something like that?
> That way it could be ease found using text search. 
> 
> static const struct rk_i2c_soc_data rk3066_soc_data = {
> 	.grf_offset = 0x154, // offsetof(struct rk3036_grf, soc_con1),
> };
> 
> static const struct rk_i2c_soc_data rk3188_soc_data = {
> 	.grf_offset = 0x0a4, // offsetof(struct rk3188_grf, soc_con1),
> };
> 
> 
> Another solution I can do is following. I declare RK3066_GRF_SOC_CON1 and
> RK3188_GFR_SOC_CON1 constants at top of i2c driver and use them.
> 
> static const struct rk_i2c_soc_data rk3066_soc_data = {
> 	.grf_offset = RK3066_GRF_SOC_CON1,
> };
> 
> static const struct rk_i2c_soc_data rk3188_soc_data = {
> 	.grf_offset = RK3188_GRF_SOC_CON1,
> };
> 
> What do you think? How to do better?
> 
> Regards,
> Alexander.
>