[U-Boot] [PATCH 6/8] rockchip: rk3188: Switch to new i2c IP blocks

[Simon Glass]

On 26 March 2017 at 07:01, Heiko Stuebner <heiko at sntech.de> wrote:
> Am Samstag, 25. März 2017, 19:17:33 CEST schrieb Simon Glass:
>> Hi Heiko,
>>
>> On 24 March 2017 at 01:32, Heiko Stübner <heiko at sntech.de> wrote:
>> > Am Donnerstag, 23. März 2017, 21:28:08 CET schrieb Simon Glass:
>> >> Hi Heiko,
>> >>
>> >> On 20 March 2017 at 05:40, Heiko Stuebner <heiko at sntech.de> wrote:
>> >> > The rk3066/rk3188 introduced new i2c IP blocks but kept the old ones
>> >> > around just in case. The default also points to these old controllers.
>> >> >
>> >> > The "new" blocks proved stable and nobody ever used the old ones anywhere,
>> >> > not in the kernel and not in U-Boot, so to be able to reuse the already
>> >> > existing driver make the rk3188 switch to the new ones in U-Boot as well.
>> >> >
>> >> > Signed-off-by: Heiko Stuebner <heiko at sntech.de>
>> >> > ---
>> >> >
>> >> >  arch/arm/mach-rockchip/rk3188-board-spl.c | 21 +++++++++++++++++++++
>> >> >  1 file changed, 21 insertions(+)
>> >> >
>> >> > diff --git a/arch/arm/mach-rockchip/rk3188-board-spl.c
>> >> > b/arch/arm/mach-rockchip/rk3188-board-spl.c index affd959f86..14847a7b1b
>> >> > 100644
>> >> > --- a/arch/arm/mach-rockchip/rk3188-board-spl.c
>> >> > +++ b/arch/arm/mach-rockchip/rk3188-board-spl.c
>> >> > @@ -17,6 +17,7 @@
>> >> >
>> >> >  #include <asm/io.h>
>> >> >  #include <asm/arch/bootrom.h>
>> >> >  #include <asm/arch/clock.h>
>> >> >
>> >> > +#include <asm/arch/grf_rk3188.h>
>> >> >
>> >> >  #include <asm/arch/hardware.h>
>> >> >  #include <asm/arch/periph.h>
>> >> >  #include <asm/arch/pmu_rk3188.h>
>> >> >
>> >> > @@ -102,6 +103,7 @@ void board_init_f(ulong dummy)
>> >> >
>> >> >  {
>> >> >
>> >> >         struct udevice *pinctrl, *dev;
>> >> >         struct rk3188_pmu *pmu;
>> >> >
>> >> > +       struct rk3188_grf *grf;
>> >> >
>> >> >         int ret;
>> >> >
>> >> >         /* Example code showing how to enable the debug UART on RK3188 */
>> >> >
>> >> > @@ -154,6 +156,25 @@ void board_init_f(ulong dummy)
>> >> >
>> >> >                 error("pmu syscon returned %ld\n", PTR_ERR(pmu));
>> >> >
>> >> >         SAVE_SP_ADDR = readl(&pmu->sys_reg[2]);
>> >> >
>> >> > +       /* init common grf settings */
>> >> > +       grf = syscon_get_first_range(ROCKCHIP_SYSCON_GRF);
>> >> > +       if (IS_ERR(grf)) {
>> >> > +               error("grf syscon returned %ld\n", PTR_ERR(grf));
>> >> > +       } else {
>> >> > +               /* make i2c controllers use the new IP */
>> >> > +               rk_clrsetreg(&grf->soc_con1,
>> >> > +                       RKI2C4_SEL_MASK << RKI2C4_SEL_SHIFT |
>> >> > +                       RKI2C3_SEL_MASK << RKI2C3_SEL_SHIFT |
>> >> > +                       RKI2C2_SEL_MASK << RKI2C2_SEL_SHIFT |
>> >> > +                       RKI2C1_SEL_MASK << RKI2C1_SEL_SHIFT |
>> >> > +                       RKI2C0_SEL_MASK << RKI2C0_SEL_SHIFT,
>> >> > +                       RKI2C4_SEL_MASK << RKI2C4_SEL_SHIFT |
>> >> > +                       RKI2C3_SEL_MASK << RKI2C3_SEL_SHIFT |
>> >> > +                       RKI2C2_SEL_MASK << RKI2C2_SEL_SHIFT |
>> >> > +                       RKI2C1_SEL_MASK << RKI2C1_SEL_SHIFT |
>> >> > +                       RKI2C0_SEL_MASK << RKI2C0_SEL_SHIFT);
>> >> > +       }
>> >>
>> >> Can you move this to the pinctrl driver?
>> >
>> > Are you sure that is the right approach?
>> >
>> > This setting switches the i2c controller IP block used, while the i2c-pins are
>> > not affected by this at all. You could also use the i2c pins with the other
>> > i2c controller with the same pinctrl settings, so it feels a tiny bit strange
>> > to burden the pinctrl driver with this.
>>
>> It still seems like pinctrl to me, in that you are selecting which IP
>> block uses those pins. If you don't want it in pinctrl, perhaps this
>> whole thing should go in the I2C driver?
>
> no pinctrl is fine, and funnily enough, when I tried moving that there,
> I realized that the pinctrl driver had the switch to the new controller
> right from the beginning and I just had forgotten about it :-S, see the
>
>         /* enable new i2c controller */
>         rk_clrsetreg(&grf->soc_con1, 1 << RKI2C0_SEL_SHIFT,
>                                      1 << RKI2C0_SEL_SHIFT);
>
> parts in pinctrl_rk3188.c .
>
> So it looks like we can just drop this patch altogether :-) .

OK, nice, thanks!