| Message ID | 20170320114036.21475-7-heiko@sntech.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Heiko, On 20 March 2017 at 05:40, Heiko Stuebner <heiko@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@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? > + > ret = uclass_get_device(UCLASS_PINCTRL, 0, &pinctrl); > if (ret) { > debug("Pinctrl init failed: %d\n", ret); > -- > 2.11.0 > Regards, Simon
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@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@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. Heiko
Hi Heiko, On 24 March 2017 at 01:32, Heiko Stübner <heiko@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@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@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? Regards, Simon
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@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@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@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 :-) . Heiko
On 26 March 2017 at 07:01, Heiko Stuebner <heiko@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@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@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@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!
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); + } + ret = uclass_get_device(UCLASS_PINCTRL, 0, &pinctrl); if (ret) { debug("Pinctrl init failed: %d\n", ret);
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@sntech.de> --- arch/arm/mach-rockchip/rk3188-board-spl.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)