| Message ID | 20170829120835.17276-1-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: designware: Round down ACPI provided clk to nearest supported clk | expand |
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.
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
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>
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
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
> 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...
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.
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.
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!
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.
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(-)