[U-Boot] [PATCH v1 1/2] sunxi (sun50i): support i2c on A64 (pin-config, clocking)

[Andre Przywara]

Hi,

On 18/02/17 12:03, Dr. Philipp Tomsich wrote:
> 
>> On 18 Feb 2017, at 02:15, André Przywara <andre.przywara at arm.com
>> <mailto:andre.przywara at 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.