On 03/18/2016 01:04 PM, Stefan Roese wrote:
> On 18.03.2016 12:12, Marek Vasut wrote:
>> On 03/18/2016 08:54 AM, Stefan Roese wrote:
>>> dw_i2c_enable() is used to dis-/en-able the I2C controller. It makes
>>> sense to add such a function, as the controller is dis-/en-abled
>>> multiple times in the code. Additionally, this function now checks,
>>> if the controller is really dis-/en-abled. This code is copied
>>> from the Linux I2C driver version.
>>>
>>> Signed-off-by: Stefan Roese <s...@denx.de>
>>> Cc: Simon Glass <s...@chromium.org>
>>> Cc: Bin Meng <bmeng...@gmail.com>
>>> Cc: Marek Vasut <ma...@denx.de>
>>> Cc: Heiko Schocher <h...@denx.de>
>>> ---
>>> drivers/i2c/designware_i2c.c | 46
>>> +++++++++++++++++++++++++-------------------
>>> 1 file changed, 26 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c
>>> index e768cde..c8ea520 100644
>>> --- a/drivers/i2c/designware_i2c.c
>>> +++ b/drivers/i2c/designware_i2c.c
>>> @@ -34,6 +34,26 @@ static struct i2c_regs *i2c_get_base(struct
>>> i2c_adapter *adap)
>>> return NULL;
>>> }
>>>
>>> +static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable)
>>> +{
>>> + int timeout = 100;
>>> +
>>> + do {
>>> + writel(enable, &i2c_base->ic_enable);
>>
>> This should at least use IC_ENABLE_0B and not the boot enable.
>>
>>> + if ((readl(&i2c_base->ic_enable_status) & 1) == enable)
>>> + return;
>>> +
>>> + /*
>>> + * Wait 10 times the signaling period of the highest I2C
>>> + * transfer supported by the driver (for 400KHz this is
>>> + * 25us) as described in the DesignWare I2C databook.
>>> + */
>>> + udelay(25);
>>> + } while (timeout--);
>>> +
>>> + printf("timeout in %sabling I2C adapter\n", enable ? "en" : "dis");
>>> +}
>>> +
>>> /*
>>> * set_speed - Set the i2c speed mode (standard, high, fast)
>>> * @i2c_spd: required i2c speed mode
>>> @@ -45,12 +65,9 @@ static void set_speed(struct i2c_adapter *adap,
>>> int i2c_spd)
>>> struct i2c_regs *i2c_base = i2c_get_base(adap);
>>> unsigned int cntl;
>>> unsigned int hcnt, lcnt;
>>> - unsigned int enbl;
>>>
>>> /* to set speed cltr must be disabled */
>>> - enbl = readl(&i2c_base->ic_enable);
>>> - enbl &= ~IC_ENABLE_0B;
>>> - writel(enbl, &i2c_base->ic_enable);
>>> + dw_i2c_enable(i2c_base, 0);
>>
>> This and all the other places which you changed actually change the
>> logic of the code, right ? Is that a problem ?
>
> It is a functional change, yes. With a now added check, if the
> controller is actually getting enabled or disabled. The code is
> taken from the Linux kernel, as noted in the commit text. And
> I've tested this code on SoCFPGA without any issues so far.
>
> Additional testing would be very welcome though. ;)
I will have a board ready for mainlining that uses i2c, so if something
breaks, I will cry ;-)
--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
