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.toms...@theobroma-systems.com> wrote:
>
> Alexander,
>
>> On 26 Feb 2018, at 10:25, Alexander Kochetkov <al.koc...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
