| Message ID | 20220318035420.15058-18-samuel@sholland.org |
|---|---|
| State | Accepted |
| Commit | af2ec35c4179620e2655f195ec0fd41e7a1ff784 |
| Delegated to: | Andre Przywara |
| Headers | show |
| Series | sunxi: Add and use a pinctrl driver | expand |
On 3/18/22 04:54, Samuel Holland wrote: > When the DM_I2C driver is loaded, the pin setup is done automatically > from the device tree by the pinctrl driver. > > Clean up the code in the process: remove #ifdefs and recognize that the > pin configuration is the same for all sun8i/sun50i SoCs, not just those > which select CONFIG_MACH_SUN8I. > > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > > (no changes since v1) > > drivers/i2c/sun8i_rsb.c | 46 +++++++++++++++++------------------------ > 1 file changed, 19 insertions(+), 27 deletions(-) > > diff --git a/drivers/i2c/sun8i_rsb.c b/drivers/i2c/sun8i_rsb.c > index 716b245a00..0dea8f7a92 100644 > --- a/drivers/i2c/sun8i_rsb.c > +++ b/drivers/i2c/sun8i_rsb.c > @@ -95,27 +95,6 @@ static int sun8i_rsb_set_device_address(struct sunxi_rsb_reg *base, > return sun8i_rsb_do_trans(base); > } > > -static void sun8i_rsb_cfg_io(void) > -{ > -#ifdef CONFIG_MACH_SUN8I > - sunxi_gpio_set_cfgpin(SUNXI_GPL(0), SUN8I_GPL_R_RSB); > - sunxi_gpio_set_cfgpin(SUNXI_GPL(1), SUN8I_GPL_R_RSB); > - sunxi_gpio_set_pull(SUNXI_GPL(0), 1); > - sunxi_gpio_set_pull(SUNXI_GPL(1), 1); > - sunxi_gpio_set_drv(SUNXI_GPL(0), 2); > - sunxi_gpio_set_drv(SUNXI_GPL(1), 2); > -#elif defined CONFIG_MACH_SUN9I > - sunxi_gpio_set_cfgpin(SUNXI_GPN(0), SUN9I_GPN_R_RSB); > - sunxi_gpio_set_cfgpin(SUNXI_GPN(1), SUN9I_GPN_R_RSB); > - sunxi_gpio_set_pull(SUNXI_GPN(0), 1); > - sunxi_gpio_set_pull(SUNXI_GPN(1), 1); > - sunxi_gpio_set_drv(SUNXI_GPN(0), 2); > - sunxi_gpio_set_drv(SUNXI_GPN(1), 2); > -#else > -#error unsupported MACH_SUNXI > -#endif > -} > - > static void sun8i_rsb_set_clk(struct sunxi_rsb_reg *base) > { > u32 div = 0; > @@ -147,12 +126,6 @@ static int sun8i_rsb_set_device_mode(struct sunxi_rsb_reg *base) > > static int sun8i_rsb_init(struct sunxi_rsb_reg *base) > { > - /* Enable RSB and PIO clk, and de-assert their resets */ > - prcm_apb0_enable(PRCM_APB0_GATE_PIO | PRCM_APB0_GATE_RSB); > - > - /* Setup external pins */ > - sun8i_rsb_cfg_io(); > - > writel(RSB_CTRL_SOFT_RST, &base->ctrl); > sun8i_rsb_set_clk(base); > > @@ -185,6 +158,25 @@ int rsb_init(void) > { > struct sunxi_rsb_reg *base = (struct sunxi_rsb_reg *)SUNXI_RSB_BASE; > > + /* Enable RSB and PIO clk, and de-assert their resets */ > + prcm_apb0_enable(PRCM_APB0_GATE_PIO | PRCM_APB0_GATE_RSB); > + > + if (IS_ENABLED(CONFIG_MACH_SUN9I)) { > + sunxi_gpio_set_cfgpin(SUNXI_GPN(0), SUN9I_GPN_R_RSB); > + sunxi_gpio_set_cfgpin(SUNXI_GPN(1), SUN9I_GPN_R_RSB); > + sunxi_gpio_set_pull(SUNXI_GPN(0), 1); > + sunxi_gpio_set_pull(SUNXI_GPN(1), 1); > + sunxi_gpio_set_drv(SUNXI_GPN(0), 2); > + sunxi_gpio_set_drv(SUNXI_GPN(1), 2); > + } else { > + sunxi_gpio_set_cfgpin(SUNXI_GPL(0), SUN8I_GPL_R_RSB); > + sunxi_gpio_set_cfgpin(SUNXI_GPL(1), SUN8I_GPL_R_RSB); > + sunxi_gpio_set_pull(SUNXI_GPL(0), 1); > + sunxi_gpio_set_pull(SUNXI_GPL(1), 1); > + sunxi_gpio_set_drv(SUNXI_GPL(0), 2); > + sunxi_gpio_set_drv(SUNXI_GPL(1), 2); Don't repeat yourself. Please, pull these 4 lines out of the if-else construct. Best regards Heinrich > + } > + > return sun8i_rsb_init(base); > } > #endif
On 3/20/22 08:17, Heinrich Schuchardt wrote: > > > On 3/18/22 04:54, Samuel Holland wrote: >> When the DM_I2C driver is loaded, the pin setup is done automatically >> from the device tree by the pinctrl driver. >> >> Clean up the code in the process: remove #ifdefs and recognize that the >> pin configuration is the same for all sun8i/sun50i SoCs, not just those >> which select CONFIG_MACH_SUN8I. >> >> Signed-off-by: Samuel Holland <samuel@sholland.org> >> --- >> >> (no changes since v1) >> >> drivers/i2c/sun8i_rsb.c | 46 +++++++++++++++++------------------------ >> 1 file changed, 19 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/i2c/sun8i_rsb.c b/drivers/i2c/sun8i_rsb.c >> index 716b245a00..0dea8f7a92 100644 >> --- a/drivers/i2c/sun8i_rsb.c >> +++ b/drivers/i2c/sun8i_rsb.c >> @@ -95,27 +95,6 @@ static int sun8i_rsb_set_device_address(struct >> sunxi_rsb_reg *base, >> return sun8i_rsb_do_trans(base); >> } >> -static void sun8i_rsb_cfg_io(void) >> -{ >> -#ifdef CONFIG_MACH_SUN8I >> - sunxi_gpio_set_cfgpin(SUNXI_GPL(0), SUN8I_GPL_R_RSB); >> - sunxi_gpio_set_cfgpin(SUNXI_GPL(1), SUN8I_GPL_R_RSB); >> - sunxi_gpio_set_pull(SUNXI_GPL(0), 1); >> - sunxi_gpio_set_pull(SUNXI_GPL(1), 1); >> - sunxi_gpio_set_drv(SUNXI_GPL(0), 2); >> - sunxi_gpio_set_drv(SUNXI_GPL(1), 2); >> -#elif defined CONFIG_MACH_SUN9I >> - sunxi_gpio_set_cfgpin(SUNXI_GPN(0), SUN9I_GPN_R_RSB); >> - sunxi_gpio_set_cfgpin(SUNXI_GPN(1), SUN9I_GPN_R_RSB); >> - sunxi_gpio_set_pull(SUNXI_GPN(0), 1); >> - sunxi_gpio_set_pull(SUNXI_GPN(1), 1); >> - sunxi_gpio_set_drv(SUNXI_GPN(0), 2); >> - sunxi_gpio_set_drv(SUNXI_GPN(1), 2); >> -#else >> -#error unsupported MACH_SUNXI >> -#endif >> -} >> - >> static void sun8i_rsb_set_clk(struct sunxi_rsb_reg *base) >> { >> u32 div = 0; >> @@ -147,12 +126,6 @@ static int sun8i_rsb_set_device_mode(struct >> sunxi_rsb_reg *base) >> static int sun8i_rsb_init(struct sunxi_rsb_reg *base) >> { >> - /* Enable RSB and PIO clk, and de-assert their resets */ >> - prcm_apb0_enable(PRCM_APB0_GATE_PIO | PRCM_APB0_GATE_RSB); >> - >> - /* Setup external pins */ >> - sun8i_rsb_cfg_io(); >> - >> writel(RSB_CTRL_SOFT_RST, &base->ctrl); >> sun8i_rsb_set_clk(base); >> @@ -185,6 +158,25 @@ int rsb_init(void) >> { >> struct sunxi_rsb_reg *base = (struct sunxi_rsb_reg >> *)SUNXI_RSB_BASE; >> + /* Enable RSB and PIO clk, and de-assert their resets */ >> + prcm_apb0_enable(PRCM_APB0_GATE_PIO | PRCM_APB0_GATE_RSB); >> + >> + if (IS_ENABLED(CONFIG_MACH_SUN9I)) { >> + sunxi_gpio_set_cfgpin(SUNXI_GPN(0), SUN9I_GPN_R_RSB); >> + sunxi_gpio_set_cfgpin(SUNXI_GPN(1), SUN9I_GPN_R_RSB); >> + sunxi_gpio_set_pull(SUNXI_GPN(0), 1); >> + sunxi_gpio_set_pull(SUNXI_GPN(1), 1); >> + sunxi_gpio_set_drv(SUNXI_GPN(0), 2); >> + sunxi_gpio_set_drv(SUNXI_GPN(1), 2); >> + } else { >> + sunxi_gpio_set_cfgpin(SUNXI_GPL(0), SUN8I_GPL_R_RSB); >> + sunxi_gpio_set_cfgpin(SUNXI_GPL(1), SUN8I_GPL_R_RSB); >> + sunxi_gpio_set_pull(SUNXI_GPL(0), 1); >> + sunxi_gpio_set_pull(SUNXI_GPL(1), 1); >> + sunxi_gpio_set_drv(SUNXI_GPL(0), 2); >> + sunxi_gpio_set_drv(SUNXI_GPL(1), 2); > > Don't repeat yourself. Please, pull these 4 lines out of the if-else > construct. Sorry missied N vs L. > > Best regards > > Heinrich > >> + } >> + >> return sun8i_rsb_init(base); >> } >> #endif
On Thu, 17 Mar 2022 22:54:14 -0500 Samuel Holland <samuel@sholland.org> wrote: > When the DM_I2C driver is loaded, the pin setup is done automatically > from the device tree by the pinctrl driver. > > Clean up the code in the process: remove #ifdefs and recognize that the > pin configuration is the same for all sun8i/sun50i SoCs, not just those > which select CONFIG_MACH_SUN8I. Indeed, even though the F1C100s uses mux 4, but we don't use the RSB there, and can fix that when we need it. So this means we could enable RSB for the H616 SPL? Regardless this looks alright: Reviewed-by: Andre Przywara <andre.przywara@arm.com> Cheers, Andre > > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > > (no changes since v1) > > drivers/i2c/sun8i_rsb.c | 46 +++++++++++++++++------------------------ > 1 file changed, 19 insertions(+), 27 deletions(-) > > diff --git a/drivers/i2c/sun8i_rsb.c b/drivers/i2c/sun8i_rsb.c > index 716b245a00..0dea8f7a92 100644 > --- a/drivers/i2c/sun8i_rsb.c > +++ b/drivers/i2c/sun8i_rsb.c > @@ -95,27 +95,6 @@ static int sun8i_rsb_set_device_address(struct sunxi_rsb_reg *base, > return sun8i_rsb_do_trans(base); > } > > -static void sun8i_rsb_cfg_io(void) > -{ > -#ifdef CONFIG_MACH_SUN8I > - sunxi_gpio_set_cfgpin(SUNXI_GPL(0), SUN8I_GPL_R_RSB); > - sunxi_gpio_set_cfgpin(SUNXI_GPL(1), SUN8I_GPL_R_RSB); > - sunxi_gpio_set_pull(SUNXI_GPL(0), 1); > - sunxi_gpio_set_pull(SUNXI_GPL(1), 1); > - sunxi_gpio_set_drv(SUNXI_GPL(0), 2); > - sunxi_gpio_set_drv(SUNXI_GPL(1), 2); > -#elif defined CONFIG_MACH_SUN9I > - sunxi_gpio_set_cfgpin(SUNXI_GPN(0), SUN9I_GPN_R_RSB); > - sunxi_gpio_set_cfgpin(SUNXI_GPN(1), SUN9I_GPN_R_RSB); > - sunxi_gpio_set_pull(SUNXI_GPN(0), 1); > - sunxi_gpio_set_pull(SUNXI_GPN(1), 1); > - sunxi_gpio_set_drv(SUNXI_GPN(0), 2); > - sunxi_gpio_set_drv(SUNXI_GPN(1), 2); > -#else > -#error unsupported MACH_SUNXI > -#endif > -} > - > static void sun8i_rsb_set_clk(struct sunxi_rsb_reg *base) > { > u32 div = 0; > @@ -147,12 +126,6 @@ static int sun8i_rsb_set_device_mode(struct sunxi_rsb_reg *base) > > static int sun8i_rsb_init(struct sunxi_rsb_reg *base) > { > - /* Enable RSB and PIO clk, and de-assert their resets */ > - prcm_apb0_enable(PRCM_APB0_GATE_PIO | PRCM_APB0_GATE_RSB); > - > - /* Setup external pins */ > - sun8i_rsb_cfg_io(); > - > writel(RSB_CTRL_SOFT_RST, &base->ctrl); > sun8i_rsb_set_clk(base); > > @@ -185,6 +158,25 @@ int rsb_init(void) > { > struct sunxi_rsb_reg *base = (struct sunxi_rsb_reg *)SUNXI_RSB_BASE; > > + /* Enable RSB and PIO clk, and de-assert their resets */ > + prcm_apb0_enable(PRCM_APB0_GATE_PIO | PRCM_APB0_GATE_RSB); > + > + if (IS_ENABLED(CONFIG_MACH_SUN9I)) { > + sunxi_gpio_set_cfgpin(SUNXI_GPN(0), SUN9I_GPN_R_RSB); > + sunxi_gpio_set_cfgpin(SUNXI_GPN(1), SUN9I_GPN_R_RSB); > + sunxi_gpio_set_pull(SUNXI_GPN(0), 1); > + sunxi_gpio_set_pull(SUNXI_GPN(1), 1); > + sunxi_gpio_set_drv(SUNXI_GPN(0), 2); > + sunxi_gpio_set_drv(SUNXI_GPN(1), 2); > + } else { > + sunxi_gpio_set_cfgpin(SUNXI_GPL(0), SUN8I_GPL_R_RSB); > + sunxi_gpio_set_cfgpin(SUNXI_GPL(1), SUN8I_GPL_R_RSB); > + sunxi_gpio_set_pull(SUNXI_GPL(0), 1); > + sunxi_gpio_set_pull(SUNXI_GPL(1), 1); > + sunxi_gpio_set_drv(SUNXI_GPL(0), 2); > + sunxi_gpio_set_drv(SUNXI_GPL(1), 2); > + } > + > return sun8i_rsb_init(base); > } > #endif
On 3/31/22 6:20 PM, Andre Przywara wrote: > On Thu, 17 Mar 2022 22:54:14 -0500 > Samuel Holland <samuel@sholland.org> wrote: > >> When the DM_I2C driver is loaded, the pin setup is done automatically >> from the device tree by the pinctrl driver. >> >> Clean up the code in the process: remove #ifdefs and recognize that the >> pin configuration is the same for all sun8i/sun50i SoCs, not just those >> which select CONFIG_MACH_SUN8I. > > Indeed, even though the F1C100s uses mux 4, but we don't use the RSB > there, and can fix that when we need it. > > So this means we could enable RSB for the H616 SPL? Not in SPL, because the DM clock driver is not yet available there, and prcm_apb0_enable() does not support the H6 and newer CCU gate/reset bits layout. It should work for H616 in U-Boot proper. Regards, Samuel > Regardless this looks alright: > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > > Cheers, > Andre > >> >> Signed-off-by: Samuel Holland <samuel@sholland.org> >> --- >> >> (no changes since v1) >> >> drivers/i2c/sun8i_rsb.c | 46 +++++++++++++++++------------------------ >> 1 file changed, 19 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/i2c/sun8i_rsb.c b/drivers/i2c/sun8i_rsb.c >> index 716b245a00..0dea8f7a92 100644 >> --- a/drivers/i2c/sun8i_rsb.c >> +++ b/drivers/i2c/sun8i_rsb.c >> @@ -95,27 +95,6 @@ static int sun8i_rsb_set_device_address(struct sunxi_rsb_reg *base, >> return sun8i_rsb_do_trans(base); >> } >> >> -static void sun8i_rsb_cfg_io(void) >> -{ >> -#ifdef CONFIG_MACH_SUN8I >> - sunxi_gpio_set_cfgpin(SUNXI_GPL(0), SUN8I_GPL_R_RSB); >> - sunxi_gpio_set_cfgpin(SUNXI_GPL(1), SUN8I_GPL_R_RSB); >> - sunxi_gpio_set_pull(SUNXI_GPL(0), 1); >> - sunxi_gpio_set_pull(SUNXI_GPL(1), 1); >> - sunxi_gpio_set_drv(SUNXI_GPL(0), 2); >> - sunxi_gpio_set_drv(SUNXI_GPL(1), 2); >> -#elif defined CONFIG_MACH_SUN9I >> - sunxi_gpio_set_cfgpin(SUNXI_GPN(0), SUN9I_GPN_R_RSB); >> - sunxi_gpio_set_cfgpin(SUNXI_GPN(1), SUN9I_GPN_R_RSB); >> - sunxi_gpio_set_pull(SUNXI_GPN(0), 1); >> - sunxi_gpio_set_pull(SUNXI_GPN(1), 1); >> - sunxi_gpio_set_drv(SUNXI_GPN(0), 2); >> - sunxi_gpio_set_drv(SUNXI_GPN(1), 2); >> -#else >> -#error unsupported MACH_SUNXI >> -#endif >> -} >> - >> static void sun8i_rsb_set_clk(struct sunxi_rsb_reg *base) >> { >> u32 div = 0; >> @@ -147,12 +126,6 @@ static int sun8i_rsb_set_device_mode(struct sunxi_rsb_reg *base) >> >> static int sun8i_rsb_init(struct sunxi_rsb_reg *base) >> { >> - /* Enable RSB and PIO clk, and de-assert their resets */ >> - prcm_apb0_enable(PRCM_APB0_GATE_PIO | PRCM_APB0_GATE_RSB); >> - >> - /* Setup external pins */ >> - sun8i_rsb_cfg_io(); >> - >> writel(RSB_CTRL_SOFT_RST, &base->ctrl); >> sun8i_rsb_set_clk(base); >> >> @@ -185,6 +158,25 @@ int rsb_init(void) >> { >> struct sunxi_rsb_reg *base = (struct sunxi_rsb_reg *)SUNXI_RSB_BASE; >> >> + /* Enable RSB and PIO clk, and de-assert their resets */ >> + prcm_apb0_enable(PRCM_APB0_GATE_PIO | PRCM_APB0_GATE_RSB); >> + >> + if (IS_ENABLED(CONFIG_MACH_SUN9I)) { >> + sunxi_gpio_set_cfgpin(SUNXI_GPN(0), SUN9I_GPN_R_RSB); >> + sunxi_gpio_set_cfgpin(SUNXI_GPN(1), SUN9I_GPN_R_RSB); >> + sunxi_gpio_set_pull(SUNXI_GPN(0), 1); >> + sunxi_gpio_set_pull(SUNXI_GPN(1), 1); >> + sunxi_gpio_set_drv(SUNXI_GPN(0), 2); >> + sunxi_gpio_set_drv(SUNXI_GPN(1), 2); >> + } else { >> + sunxi_gpio_set_cfgpin(SUNXI_GPL(0), SUN8I_GPL_R_RSB); >> + sunxi_gpio_set_cfgpin(SUNXI_GPL(1), SUN8I_GPL_R_RSB); >> + sunxi_gpio_set_pull(SUNXI_GPL(0), 1); >> + sunxi_gpio_set_pull(SUNXI_GPL(1), 1); >> + sunxi_gpio_set_drv(SUNXI_GPL(0), 2); >> + sunxi_gpio_set_drv(SUNXI_GPL(1), 2); >> + } >> + >> return sun8i_rsb_init(base); >> } >> #endif >
diff --git a/drivers/i2c/sun8i_rsb.c b/drivers/i2c/sun8i_rsb.c index 716b245a00..0dea8f7a92 100644 --- a/drivers/i2c/sun8i_rsb.c +++ b/drivers/i2c/sun8i_rsb.c @@ -95,27 +95,6 @@ static int sun8i_rsb_set_device_address(struct sunxi_rsb_reg *base, return sun8i_rsb_do_trans(base); } -static void sun8i_rsb_cfg_io(void) -{ -#ifdef CONFIG_MACH_SUN8I - sunxi_gpio_set_cfgpin(SUNXI_GPL(0), SUN8I_GPL_R_RSB); - sunxi_gpio_set_cfgpin(SUNXI_GPL(1), SUN8I_GPL_R_RSB); - sunxi_gpio_set_pull(SUNXI_GPL(0), 1); - sunxi_gpio_set_pull(SUNXI_GPL(1), 1); - sunxi_gpio_set_drv(SUNXI_GPL(0), 2); - sunxi_gpio_set_drv(SUNXI_GPL(1), 2); -#elif defined CONFIG_MACH_SUN9I - sunxi_gpio_set_cfgpin(SUNXI_GPN(0), SUN9I_GPN_R_RSB); - sunxi_gpio_set_cfgpin(SUNXI_GPN(1), SUN9I_GPN_R_RSB); - sunxi_gpio_set_pull(SUNXI_GPN(0), 1); - sunxi_gpio_set_pull(SUNXI_GPN(1), 1); - sunxi_gpio_set_drv(SUNXI_GPN(0), 2); - sunxi_gpio_set_drv(SUNXI_GPN(1), 2); -#else -#error unsupported MACH_SUNXI -#endif -} - static void sun8i_rsb_set_clk(struct sunxi_rsb_reg *base) { u32 div = 0; @@ -147,12 +126,6 @@ static int sun8i_rsb_set_device_mode(struct sunxi_rsb_reg *base) static int sun8i_rsb_init(struct sunxi_rsb_reg *base) { - /* Enable RSB and PIO clk, and de-assert their resets */ - prcm_apb0_enable(PRCM_APB0_GATE_PIO | PRCM_APB0_GATE_RSB); - - /* Setup external pins */ - sun8i_rsb_cfg_io(); - writel(RSB_CTRL_SOFT_RST, &base->ctrl); sun8i_rsb_set_clk(base); @@ -185,6 +158,25 @@ int rsb_init(void) { struct sunxi_rsb_reg *base = (struct sunxi_rsb_reg *)SUNXI_RSB_BASE; + /* Enable RSB and PIO clk, and de-assert their resets */ + prcm_apb0_enable(PRCM_APB0_GATE_PIO | PRCM_APB0_GATE_RSB); + + if (IS_ENABLED(CONFIG_MACH_SUN9I)) { + sunxi_gpio_set_cfgpin(SUNXI_GPN(0), SUN9I_GPN_R_RSB); + sunxi_gpio_set_cfgpin(SUNXI_GPN(1), SUN9I_GPN_R_RSB); + sunxi_gpio_set_pull(SUNXI_GPN(0), 1); + sunxi_gpio_set_pull(SUNXI_GPN(1), 1); + sunxi_gpio_set_drv(SUNXI_GPN(0), 2); + sunxi_gpio_set_drv(SUNXI_GPN(1), 2); + } else { + sunxi_gpio_set_cfgpin(SUNXI_GPL(0), SUN8I_GPL_R_RSB); + sunxi_gpio_set_cfgpin(SUNXI_GPL(1), SUN8I_GPL_R_RSB); + sunxi_gpio_set_pull(SUNXI_GPL(0), 1); + sunxi_gpio_set_pull(SUNXI_GPL(1), 1); + sunxi_gpio_set_drv(SUNXI_GPL(0), 2); + sunxi_gpio_set_drv(SUNXI_GPL(1), 2); + } + return sun8i_rsb_init(base); } #endif
When the DM_I2C driver is loaded, the pin setup is done automatically from the device tree by the pinctrl driver. Clean up the code in the process: remove #ifdefs and recognize that the pin configuration is the same for all sun8i/sun50i SoCs, not just those which select CONFIG_MACH_SUN8I. Signed-off-by: Samuel Holland <samuel@sholland.org> --- (no changes since v1) drivers/i2c/sun8i_rsb.c | 46 +++++++++++++++++------------------------ 1 file changed, 19 insertions(+), 27 deletions(-)