[PATCH] i2c: designware: Round down ACPI provided clk to nearest supported clk

[PATCH] i2c: designware: Round down ACPI provided clk to nearest supported clk

[Hans de Goede]

The Lenovo Miix2 8 DSDT contains an i2c clk / bus speed of 1700000 Hz
for one if its devices, which is not supported.

This is the second DSDT to show up with an unsupported clk in a short
time, remove the hardcoded fix for DSDTs with a 1 MiHz clock and simply
always round down the clk to the nearest supported value.

Reported-by: russianneuromancer@ya.ru
Fixes: 682c6c2188 ("i2c: designware: Some broken DSTDs use 1MiHz ...")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 57248bccadbc..2b98a173136f 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -256,7 +256,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	struct dw_i2c_dev *dev;
 	u32 acpi_speed, ht = 0;
 	struct resource *mem;
-	int irq, ret;
+	int i, irq, ret;
+	const int supported_speeds[] = { 0, 100000, 400000, 1000000, 3400000 };
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
@@ -297,9 +298,16 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	}
 
 	acpi_speed = i2c_acpi_find_bus_speed(&pdev->dev);
-	/* Some broken DSTDs use 1MiHz instead of 1MHz */
-	if (acpi_speed == 1048576)
-		acpi_speed = 1000000;
+	/*
+	 * Some DSTDs use a non standard speed, round down to the lowest
+	 * standard speed.
+	 */
+	for (i = 1; i < ARRAY_SIZE(supported_speeds); i++) {
+		if (acpi_speed < supported_speeds[i])
+			break;
+	}
+	acpi_speed = supported_speeds[i - 1];
+
 	/*
 	 * Find bus speed from the "clock-frequency" device property, ACPI
 	 * or by using fast mode if neither is set.
-- 
2.13.4

Re: [PATCH] i2c: designware: Round down ACPI provided clk to nearest supported clk

[Andy Shevchenko]

On Tue, 2017-08-29 at 14:08 +0200, Hans de Goede wrote:
> The Lenovo Miix2 8 DSDT contains an i2c clk / bus speed of 1700000 Hz
> for one if its devices, which is not supported.
> 
> This is the second DSDT to show up with an unsupported clk in a short
> time, remove the hardcoded fix for DSDTs with a 1 MiHz clock and
> simply
> always round down the clk to the nearest supported value.
> 
> Reported-by: russianneuromancer@ya.ru
> Fixes: 682c6c2188 ("i2c: designware: Some broken DSTDs use 1MiHz ...")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 57248bccadbc..2b98a173136f 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -256,7 +256,8 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
>  	struct dw_i2c_dev *dev;
>  	u32 acpi_speed, ht = 0;
>  	struct resource *mem;
> -	int irq, ret;
> +	int i, irq, ret;
> +	const int supported_speeds[] = { 0, 100000, 400000, 1000000,
> 3400000 };
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
> @@ -297,9 +298,16 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
>  	}
>  
>  	acpi_speed = i2c_acpi_find_bus_speed(&pdev->dev);
> -	/* Some broken DSTDs use 1MiHz instead of 1MHz */
> -	if (acpi_speed == 1048576)
> -		acpi_speed = 1000000;
> +	/*
> +	 * Some DSTDs use a non standard speed, round down to the
> lowest
> +	 * standard speed.
> +	 */
> +	for (i = 1; i < ARRAY_SIZE(supported_speeds); i++) {
> +		if (acpi_speed < supported_speeds[i])
> +			break;
> +	}
> +	acpi_speed = supported_speeds[i - 1];

I dunno what standard says if we may or may not use 100 kHz as a last
resort even for speeds defined less than 100 kHz.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] i2c: designware: Round down ACPI provided clk to nearest supported clk

[Hans de Goede]

Hi,

On 29-08-17 14:22, Andy Shevchenko wrote:
> On Tue, 2017-08-29 at 14:08 +0200, Hans de Goede wrote:
>> The Lenovo Miix2 8 DSDT contains an i2c clk / bus speed of 1700000 Hz
>> for one if its devices, which is not supported.
>>
>> This is the second DSDT to show up with an unsupported clk in a short
>> time, remove the hardcoded fix for DSDTs with a 1 MiHz clock and
>> simply
>> always round down the clk to the nearest supported value.
>>
>> Reported-by: russianneuromancer@ya.ru
>> Fixes: 682c6c2188 ("i2c: designware: Some broken DSTDs use 1MiHz ...")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/i2c/busses/i2c-designware-platdrv.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 57248bccadbc..2b98a173136f 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -256,7 +256,8 @@ static int dw_i2c_plat_probe(struct
>> platform_device *pdev)
>>   	struct dw_i2c_dev *dev;
>>   	u32 acpi_speed, ht = 0;
>>   	struct resource *mem;
>> -	int irq, ret;
>> +	int i, irq, ret;
>> +	const int supported_speeds[] = { 0, 100000, 400000, 1000000,
>> 3400000 };
>>   
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq < 0)
>> @@ -297,9 +298,16 @@ static int dw_i2c_plat_probe(struct
>> platform_device *pdev)
>>   	}
>>   
>>   	acpi_speed = i2c_acpi_find_bus_speed(&pdev->dev);
>> -	/* Some broken DSTDs use 1MiHz instead of 1MHz */
>> -	if (acpi_speed == 1048576)
>> -		acpi_speed = 1000000;
>> +	/*
>> +	 * Some DSTDs use a non standard speed, round down to the
>> lowest
>> +	 * standard speed.
>> +	 */
>> +	for (i = 1; i < ARRAY_SIZE(supported_speeds); i++) {
>> +		if (acpi_speed < supported_speeds[i])
>> +			break;
>> +	}
>> +	acpi_speed = supported_speeds[i - 1];
> 
> I dunno what standard says if we may or may not use 100 kHz as a last
> resort even for speeds defined less than 100 kHz.

The < 100000 case is for when i2c_acpi_find_bus_speed() returns 0, so
that we then keep it 0, in which case the code a bit lower will pick
a default. Since speeds < 100000 are clearly not valid treating them
as ACPI not providing any bus-speed info seems sensible to me.

Regards,

Hans

Re: [PATCH] i2c: designware: Round down ACPI provided clk to nearest supported clk

[Jarkko Nikula]

On 08/29/2017 03:52 PM, Hans de Goede wrote:
> Hi,
> 
> On 29-08-17 14:22, Andy Shevchenko wrote:
>> On Tue, 2017-08-29 at 14:08 +0200, Hans de Goede wrote:
>>> The Lenovo Miix2 8 DSDT contains an i2c clk / bus speed of 1700000 Hz
>>> for one if its devices, which is not supported.
>>>
>>> This is the second DSDT to show up with an unsupported clk in a short
>>> time, remove the hardcoded fix for DSDTs with a 1 MiHz clock and
>>> simply
>>> always round down the clk to the nearest supported value.
>>>
>>> Reported-by: russianneuromancer@ya.ru
>>> Fixes: 682c6c2188 ("i2c: designware: Some broken DSTDs use 1MiHz ...")
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   drivers/i2c/busses/i2c-designware-platdrv.c | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index 57248bccadbc..2b98a173136f 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -256,7 +256,8 @@ static int dw_i2c_plat_probe(struct
>>> platform_device *pdev)
>>>       struct dw_i2c_dev *dev;
>>>       u32 acpi_speed, ht = 0;
>>>       struct resource *mem;
>>> -    int irq, ret;
>>> +    int i, irq, ret;
>>> +    const int supported_speeds[] = { 0, 100000, 400000, 1000000,
>>> 3400000 };
>>>       irq = platform_get_irq(pdev, 0);
>>>       if (irq < 0)
>>> @@ -297,9 +298,16 @@ static int dw_i2c_plat_probe(struct
>>> platform_device *pdev)
>>>       }
>>>       acpi_speed = i2c_acpi_find_bus_speed(&pdev->dev);
>>> -    /* Some broken DSTDs use 1MiHz instead of 1MHz */
>>> -    if (acpi_speed == 1048576)
>>> -        acpi_speed = 1000000;
>>> +    /*
>>> +     * Some DSTDs use a non standard speed, round down to the
>>> lowest
>>> +     * standard speed.
>>> +     */
>>> +    for (i = 1; i < ARRAY_SIZE(supported_speeds); i++) {
>>> +        if (acpi_speed < supported_speeds[i])
>>> +            break;
>>> +    }
>>> +    acpi_speed = supported_speeds[i - 1];
>>
>> I dunno what standard says if we may or may not use 100 kHz as a last
>> resort even for speeds defined less than 100 kHz.
> 
> The < 100000 case is for when i2c_acpi_find_bus_speed() returns 0, so
> that we then keep it 0, in which case the code a bit lower will pick
> a default. Since speeds < 100000 are clearly not valid treating them
> as ACPI not providing any bus-speed info seems sensible to me.
> 
I don't know how sensible values timing parameter calculation routines 
would produce for "bogus" < 100 kHz ACPI speeds so picking the default 
400 kHz sounds more sensible to me as well as it used to be the default 
speed earlier.

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH] i2c: designware: Round down ACPI provided clk to nearest supported clk

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

Hi,

> > I dunno what standard says if we may or may not use 100 kHz as a last
> > resort even for speeds defined less than 100 kHz.
> 
> The < 100000 case is for when i2c_acpi_find_bus_speed() returns 0, so
> that we then keep it 0, in which case the code a bit lower will pick
> a default.

This is fine by me.

> Since speeds < 100000 are clearly not valid treating them

Here I wonder: Not valid because of ACPI specs? Because I2C specs surely
allow speeds < 100kHz.

> as ACPI not providing any bus-speed info seems sensible to me.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: designware: Round down ACPI provided clk to nearest supported clk

[Hans de Goede]

Hi,

On 08/29/2017 10:18 PM, Wolfram Sang wrote:
> Hi,
> 
>>> I dunno what standard says if we may or may not use 100 kHz as a last
>>> resort even for speeds defined less than 100 kHz.
>>
>> The < 100000 case is for when i2c_acpi_find_bus_speed() returns 0, so
>> that we then keep it 0, in which case the code a bit lower will pick
>> a default.
> 
> This is fine by me.
> 
>> Since speeds < 100000 are clearly not valid treating them
> 
> Here I wonder: Not valid because of ACPI specs? Because I2C specs surely
> allow speeds < 100kHz.

The speed comes from an ACPI entry describing an i2c client,
any compliant i2c client must at least support 100KHz, right ?

Alternatively I could wrap the entire round-down for loop in
an if (acpi_speed) {} block.

Regards,

Hans

Re: [PATCH] i2c: designware: Round down ACPI provided clk to nearest supported clk

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]


> The speed comes from an ACPI entry describing an i2c client,
> any compliant i2c client must at least support 100KHz, right ?

Well, due to flaky board design, you may not be able to utilize the max
speed because of interferences etc even if the devices would support it.
Such knowledge of flaky boards could be encoded in the ACPI tables, or?
But probably not in the client device, hmmm...

> Alternatively I could wrap the entire round-down for loop in
> an if (acpi_speed) {} block.

I don't know enough about real-world ACPI tables to suggest a best
practice here. I just wanted to add that busses < 100 kHz are legal from
how I read the specs.

Oh well, Jarkko liked the patch, so let's all sleep over this patch and
if nothing else comes up, I'll apply it tomorrow or so...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: designware: Round down ACPI provided clk to nearest supported clk

[Phil Reid]

On 30/08/2017 05:00, Wolfram Sang wrote:
> 
>> The speed comes from an ACPI entry describing an i2c client,
>> any compliant i2c client must at least support 100KHz, right ?
> 
> Well, due to flaky board design, you may not be able to utilize the max
> speed because of interferences etc even if the devices would support it.
> Such knowledge of flaky boards could be encoded in the ACPI tables, or?
> But probably not in the client device, hmmm...
> 
>> Alternatively I could wrap the entire round-down for loop in
>> an if (acpi_speed) {} block.
> 
> I don't know enough about real-world ACPI tables to suggest a best
> practice here. I just wanted to add that busses < 100 kHz are legal from
> how I read the specs.
> 
> Oh well, Jarkko liked the patch, so let's all sleep over this patch and
> if nothing else comes up, I'll apply it tomorrow or so...
> 

My understanding is 100k is what the client must support.
But sometimes buses need to be run slower.
Particularly when using range extenders.

eg: I have an i2c bus running over a 10m cable that needs to run at about 40k
to be reliable.


-- 
Regards
Phil Reid

Re: [PATCH] i2c: designware: Round down ACPI provided clk to nearest supported clk

[Jarkko Nikula]

On 08/30/2017 04:23 AM, Phil Reid wrote:
> On 30/08/2017 05:00, Wolfram Sang wrote:
>> I don't know enough about real-world ACPI tables to suggest a best
>> practice here. I just wanted to add that busses < 100 kHz are legal from
>> how I read the specs.
>>
>> Oh well, Jarkko liked the patch, so let's all sleep over this patch and
>> if nothing else comes up, I'll apply it tomorrow or so...
>>
> 
> My understanding is 100k is what the client must support.
> But sometimes buses need to be run slower.
> Particularly when using range extenders.
> 
> eg: I have an i2c bus running over a 10m cable that needs to run at 
> about 40k
> to be reliable.
> 
I acked the patch because I see it as a possibility for a regression if 
we blindly accept slower than 100 kHz speed from ACPI without validating 
does that result working setup and timing parameters.

It is better to have an another patch explicitly adding support for
< 100 kHz speeds when needed.

-- 
Jarkko

Re: [PATCH] i2c: designware: Round down ACPI provided clk to nearest supported clk

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Tue, Aug 29, 2017 at 02:08:35PM +0200, Hans de Goede wrote:
> The Lenovo Miix2 8 DSDT contains an i2c clk / bus speed of 1700000 Hz
> for one if its devices, which is not supported.
> 
> This is the second DSDT to show up with an unsupported clk in a short
> time, remove the hardcoded fix for DSDTs with a 1 MiHz clock and simply
> always round down the clk to the nearest supported value.
> 
> Reported-by: russianneuromancer@ya.ru
> Fixes: 682c6c2188 ("i2c: designware: Some broken DSTDs use 1MiHz ...")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]