Hi,
On 18/02/17 12:03, Dr. Philipp Tomsich wrote:
On 18 Feb 2017, at 02:15, André Przywara <andre.przywara@arm.com mailto:andre.przywara@arm.com> wrote:
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index bba9b7c..a47b113 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -485,90 +485,108 @@ int board_mmc_init(bd_t *bis) void i2c_init_board(void) { #ifdef CONFIG_I2C0_ENABLE #if defined(CONFIG_MACH_SUN4I) || defined(CONFIG_MACH_SUN5I) || defined(CONFIG_MACH_SUN7I) sunxi_gpio_set_cfgpin(SUNXI_GPB(0), SUN4I_GPB_TWI0); sunxi_gpio_set_cfgpin(SUNXI_GPB(1), SUN4I_GPB_TWI0); clock_twi_onoff(0, 1); #elif defined(CONFIG_MACH_SUN6I) sunxi_gpio_set_cfgpin(SUNXI_GPH(14), SUN6I_GPH_TWI0); sunxi_gpio_set_cfgpin(SUNXI_GPH(15), SUN6I_GPH_TWI0); clock_twi_onoff(0, 1); #elif defined(CONFIG_MACH_SUN8I) sunxi_gpio_set_cfgpin(SUNXI_GPH(2), SUN8I_GPH_TWI0); sunxi_gpio_set_cfgpin(SUNXI_GPH(3), SUN8I_GPH_TWI0); clock_twi_onoff(0, 1); +#elif defined(CONFIG_MACH_SUN50I) +sunxi_gpio_set_cfgpin(SUNXI_GPH(0), SUN50I_GPH_TWI0); +sunxi_gpio_set_cfgpin(SUNXI_GPH(1), SUN50I_GPH_TWI0); +/* The A64-uQ7 doesn't have external pull-ups for I2C[01]. */ +sunxi_gpio_set_pull(SUNXI_GPH(0), SUNXI_GPIO_PULL_UP); +sunxi_gpio_set_pull(SUNXI_GPH(1), SUNXI_GPIO_PULL_UP);
Is this specific to your board?
No. This is board-specific for any board. Every I2C bus will need pull-ups, but it’s not defined where on the bus these should/will be located. There’s even a few I2C peripherals that have the pull-ups integrated.
I think I got this, but it's just that no other SoC above is doing this, also your comment hints at this being specific to your board. So for the sake of a generic A64 I2C support patch, I'd rather remove those lines from this file for now. You could keep a separate patch that fixes this for your board build.
In other words: this was one of many things that triggered me going after support DM (and it was the one change that caused me to look at the size of the SPL and whether there’s still space available for DM drivers when using OF_PLATDATA … but we both know the conclusion).
Which isn't officially supported yet?
I can send patches with the board-specific DTS and defconfig, if that helps ;-)
You can of course always do, but to be honest this was more a rhetorical question. I'd rather avoid putting some board specific code into this function in this way. I clearly see where you are going to, but please keep this patch sane.
Just wanted until we get dual-IO SPI reads validated (and the DM changes merged), so it can be a single changeset instead of relying on multiple updates to the same files.
If you think it’s worthwhile to send out a defconfig that doesn’t depend on DM together with the DTS (without the gpiobank-entries), let me know.
I don't think it's necessary for now, we have enough stuff on our plate already ;-) All of those patches seem to work toward a saner board specific configuration, so you could post your defconfig & DT at the end to motivate these changes.
And having a DM conversion would make this easily fixable via DT, I guess?
Yes. But that will still leave the SPL/TPL discussion for another day, as the SPL still depends on these programmatic settings.
Our board’s early-stage normally shouldn't need to do anything on this I2C bus, but customer boards may come up with a use for it (image this going towards battery/power managing circuitry on a baseboard).
In this case we should definitely remove these three lines.
Cheers, Andre.