Re: [PATCH] i2c: designware: don't infer timings described by ACPI from clock rate

[Jarkko Nikula <jarkko.nikula@linux.intel.com>]

On 05/22/2017 08:44 AM, Jan Kiszka wrote:
> On 2017-05-21 10:09, Ard Biesheuvel wrote:
>> On 19 May 2017 at 09:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> Commit bd698d24b1b57 ("i2c: designware: Get selected speed mode
>>> sda-hold-time via ACPI") updated the logic that reads the timing
>>> parameters for various I2C bus rates from the DSDT, to only read
>>> the timing parameters for the currently selected mode.
>>>
>>> This causes a WARN_ON() splat on platforms that legally omit the clock
>>> frequency from the ACPI description, because in the new situation, the
>>> core I2C designware driver still accesses the fields in the driver
>>> struct that we no longer populate, and proceeds to calculate them from
>>> the clock frequency. Since the clock frequency is unspecified, the
>>> driver complains loudly using a WARN_ON().
>>>
>>> So revert back to the old situation, where the struct fields for all
>>> timings are populated, but retain the new logic which chooses the SDA
>>> hold time from the timing mode that is currently in use.
>>>
>>> Fixes: bd698d24b1b57 ("i2c: designware: Get selected speed mode ...")
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++++++++++--------
>>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index f2acd4b6bf01..6283b99d2b17 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -96,6 +96,7 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
>>>         struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
>>>         acpi_handle handle = ACPI_HANDLE(&pdev->dev);
>>>         const struct acpi_device_id *id;
>>> +       u32 ss_ht, fp_ht, hs_ht, fs_ht;
>>
>> Should these be initialized to zero? I realized that
>> dw_i2c_acpi_params() could fail, resulting in bogus hold time values
>> being returnted, but it is unclear to me how that affects the logic in
>> the core driver.
>
Oh, indeed, great you found this quickly and we got also report and fix 
quickly from Jan. In bad case this could have been difficult to bisect.

> I can tell you what happens: this breaks e.g. the Galileo and the
> IOT2000 boards. Will send a patch to initialize the vars to 0, restoring
> the previous behavior in the absence to the ACPI parameters. The core is
> prepared for sda_hold_time == 0.
>
Yes, i2c-designware-core.c will read the hold time from HW set by 
bootloader or the reset default when dev->sda_hold_time is zero.

-- 
Jarkko