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

[Dr. Philipp Tomsich]

Simon,

please take a look and advise whether you want this as a reset-driver and have
us adding auto-processing of resets to the initial bind/probe/init cycle.

Thanks,
Philipp.

> On 26 Feb 2018, at 10:43, Dr. Philipp Tomsich <philipp.tomsich at theobroma-systems.com> wrote:
> 
> 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.