i2c: designware: don't infer timings described by ACPI from clock rate
Subsystems (3)
PATCH 1/1 i2c: designware: don't infer timings described by ACPI from clock rate
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.
--- drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
drivers/i2c/busses/i2c-designware-platdrv.c +10 -8
@@ -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;struct acpi_device *adev;const char *uid;
@@ -107,23 +108,24 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)* Try to get SDA hold time and *CNT values from an ACPI method for * selected speed modes. */ +dw_i2c_acpi_params (pdev,"SSCN" , &dev->ss_hcnt, &dev->ss_lcnt, &ss_ht); +dw_i2c_acpi_params (pdev,"FPCN" , &dev->fp_hcnt, &dev->fp_lcnt, &fp_ht); +dw_i2c_acpi_params (pdev,"HSCN" , &dev->hs_hcnt, &dev->hs_lcnt, &hs_ht); +dw_i2c_acpi_params (pdev,"FMCN" , &dev->fs_hcnt, &dev->fs_lcnt, &fs_ht); +switch (dev->clk_freq) {case 100000 : -dw_i2c_acpi_params (pdev,"SSCN" , &dev->ss_hcnt, &dev->ss_lcnt, - &dev->sda_hold_time); + dev->sda_hold_time = ss_ht;break ;case 1000000 : -dw_i2c_acpi_params (pdev,"FPCN" , &dev->fp_hcnt, &dev->fp_lcnt, - &dev->sda_hold_time); + dev->sda_hold_time = fp_ht;break ;case 3400000 : -dw_i2c_acpi_params (pdev,"HSCN" , &dev->hs_hcnt, &dev->hs_lcnt, - &dev->sda_hold_time); + dev->sda_hold_time = hs_ht;break ;case 400000 :default : -dw_i2c_acpi_params (pdev,"FMCN" , &dev->fs_hcnt, &dev->fs_lcnt, - &dev->sda_hold_time); + dev->sda_hold_time = fs_ht;break ; }
Signature · 1 line
2.9.3
PATCH 1/1 i2c: designware: don't infer timings described by ACPI from clock rate
Quoted 16 lines · Ard Biesheuvel
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 aWARN_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.
Quoted 6 lines
Fixes bd698d24b1b57 ("i2c: designware: Get selected speed mode ...")Signed-off-by Ard Biesheuvel <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
Thanks, this is ok to me. Let's add also kudos to Lorenzo:
PATCH 1/1 i2c: designware: don't infer timings described by ACPI from clock rate
Quoted 24 lines · Jarkko Nikula
Quoted 17 lines · Ard Biesheuvel
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 aWARN_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.
Quoted 6 lines
Fixes bd698d24b1b57 ("i2c: designware: Get selected speed mode ...")Signed-off-by Ard Biesheuvel <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
Quoted 4 lines
Thanks, this is ok to me. Let's add also kudos to Lorenzo:
Reported-by Lorenzo Pieralisi <[email protected]>Acked-by Jarkko Nikula <[email protected]>
Thanks. I suppose this is going in as a fix? If not, please cc to -stable
PATCH 1/1 i2c: designware: don't infer timings described by ACPI from clock rate
Quoted 25 lines · Ard Biesheuvel
Quoted 24 lines · Jarkko Nikula
Quoted 23 lines · Ard Biesheuvel
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 aWARN_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 <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
Quoted 7 lines
Quoted 4 lines
Thanks, this is ok to me. Let's add also kudos to Lorenzo:
Reported-by Lorenzo Pieralisi <[email protected]>Acked-by Jarkko Nikula <[email protected]>Thanks. I suppose this is going in as a fix? If not, please cc to -stable
That's not needed as the bd698d24b1b57 came during pre 4.12-rc1 cycle. But good to get this into 4.12 due probable regression in high-speed transfers and to get rid of log spamming.
Signature · 1 line
Jarkko
PATCH 1/1 i2c: designware: don't infer timings described by ACPI from clock rate
Quoted 36 lines · Jarkko Nikula
Quoted 35 lines · Ard Biesheuvel
Quoted 30 lines · Jarkko Nikula
Quoted 18 lines · Ard Biesheuvel
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 aWARN_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.
Quoted 6 lines
Fixes bd698d24b1b57 ("i2c: designware: Get selected speed mode ...")Signed-off-by Ard Biesheuvel <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)Thanks, this is ok to me. Let's add also kudos to Lorenzo:
Reported-by Lorenzo Pieralisi <[email protected]>Acked-by Jarkko Nikula <[email protected]>
Quoted 6 lines
Quoted 2 lines
Thanks. I suppose this is going in as a fix? If not, please cc to -stable
That's not needed as the
bd698d24b1b57came during pre 4.12-rc1 cycle. But good to get this into 4.12 due probable regression in high-speed transfers and to get rid of log spamming.
Agreed. Thanks.
Wolfram Sang
AppliedPATCH 1/1 i2c: designware: don't infer timings described by ACPI from clock rate
In reply to Ard BiesheuvelQuoted 12 lines · Ard Biesheuvel
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 aWARN_ON().
Quoted 6 lines
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 <[email protected]>
Applied to for-current, thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170519/405553a6/attachment.sig>
PATCH 1/1 i2c: designware: don't infer timings described by ACPI from clock rate
In reply to Ard BiesheuvelQuoted 27 lines · Ard Biesheuvel
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 aWARN_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 <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)drivers/i2c/busses/i2c-designware-platdrv.c +0
-0
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_acpi_configure(struct platform_device *pdev) · line 99
Quoted 4 lines
drivers/i2c/busses/i2c-designware-platdrv.c +1
-0@@ -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.
Quoted 39 lines
drivers/i2c/busses/i2c-designware-platdrv.c +9
-9struct acpi_device *adev;const char *uid;@@ -107,23 +108,24 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)* Try to get SDA hold time and *CNT values from an ACPI method for * selected speed modes. */ +dw_i2c_acpi_params (pdev,"SSCN" , &dev->ss_hcnt, &dev->ss_lcnt, &ss_ht); +dw_i2c_acpi_params (pdev,"FPCN" , &dev->fp_hcnt, &dev->fp_lcnt, &fp_ht); +dw_i2c_acpi_params (pdev,"HSCN" , &dev->hs_hcnt, &dev->hs_lcnt, &hs_ht); +dw_i2c_acpi_params (pdev,"FMCN" , &dev->fs_hcnt, &dev->fs_lcnt, &fs_ht); +switch (dev->clk_freq) {case 100000 : -dw_i2c_acpi_params (pdev,"SSCN" , &dev->ss_hcnt, &dev->ss_lcnt, - &dev->sda_hold_time); + dev->sda_hold_time = ss_ht;break ;case 1000000 : -dw_i2c_acpi_params (pdev,"FPCN" , &dev->fp_hcnt, &dev->fp_lcnt, - &dev->sda_hold_time); + dev->sda_hold_time = fp_ht;break ;case 3400000 : -dw_i2c_acpi_params (pdev,"HSCN" , &dev->hs_hcnt, &dev->hs_lcnt, - &dev->sda_hold_time); + dev->sda_hold_time = hs_ht;break ;case 400000 :default : -dw_i2c_acpi_params (pdev,"FMCN" , &dev->fs_hcnt, &dev->fs_lcnt, - &dev->sda_hold_time); + dev->sda_hold_time = fs_ht;break ; } --2.9.3
PATCH 1/1 i2c: designware: don't infer timings described by ACPI from clock rate
Quoted 23 lines · Ard Biesheuvel
Quoted 22 lines · Ard Biesheuvel
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 aWARN_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 <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
Quoted 14 lines
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_acpi_configure(struct platform_device *pdev) · line 99
Quoted 9 lines
drivers/i2c/busses/i2c-designware-platdrv.c +1
-0@@ -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.
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.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux
PATCH 1/1 i2c: designware: don't infer timings described by ACPI from clock rate
Quoted 24 lines · Jan Kiszka
Quoted 23 lines · Ard Biesheuvel
Quoted 22 lines · Ard Biesheuvel
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 aWARN_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 <[email protected]>--- drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
drivers/i2c/busses/i2c-designware-platdrv.c +0
-0@@ -96,6 +96,7 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)struct dw_i2c_dev *dev =platform_get_drvdata (pdev);Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_acpi_configure(struct platform_device *pdev) · line 99
Quoted 3 lines
drivers/i2c/busses/i2c-designware-platdrv.c +1
-0acpi_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.
Quoted 5 lines
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.
Signature · 1 line
Jarkko
PATCH 1/1 i2c: designware: Fix bogus sda_hold_time due to uninitialized vars
In reply to Ard BiesheuvelWe need to initializes those variables to 0 for platforms that do not provide ACPI parameters. Otherwise, we set sda_hold_time to random values, breaking e.g. Galileo and IOT2000 boards.
---
Affects 4.12-rc2.
drivers/i2c/busses/i2c-designware-platdrv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-platdrv.c +1 -1
@@ -94,9 +94,9 @@ static void dw_i2c_acpi_params(struct platform_device *pdev, char method[],static int dw_i2c_acpi_configure (struct platform_device *pdev) {struct dw_i2c_dev *dev =platform_get_drvdata (pdev); +u32 ss_ht =0 , fp_ht =0 , hs_ht =0 , fs_ht =0 ;acpi_handle handle =ACPI_HANDLE (&pdev->dev);const struct acpi_device_id *id; -u32 ss_ht, fp_ht, hs_ht, fs_ht;struct acpi_device *adev;const char *uid;
Re: [PATCH] i2c: designware: Fix bogus sda_hold_time due to uninitialized vars
Quoted 10 lines · Jan Kiszka
We need to initializes those variables to 0 for platforms that do not provide ACPI parameters. Otherwise, we set
sda_hold_timeto random values, breaking e.g. Galileo and IOT2000 boards.Fixes 9d6408433019 ("i2c: designware: don't infer timings described by ACPI from clock rate")Signed-off-by Jan Kiszka <[email protected]>
Quoted 22 lines
---
Affects 4.12-rc2.
drivers/i2c/busses/i2c-designware-platdrv.c| 2 +-1 file changed, 1 insertion(+), 1 deletion(-)drivers/i2c/busses/i2c-designware-platdrv.c +1
-1@@ -94,9 +94,9 @@ static void dw_i2c_acpi_params(struct platform_device *pdev, char method[],static int dw_i2c_acpi_configure (struct platform_device *pdev) {struct dw_i2c_dev *dev =platform_get_drvdata (pdev); +u32 ss_ht =0 , fp_ht =0 , hs_ht =0 , fs_ht =0 ;acpi_handle handle =ACPI_HANDLE (&pdev->dev);const struct acpi_device_id *id; -u32 ss_ht, fp_ht, hs_ht, fs_ht;struct acpi_device *adev;const char *uid;
Re: [PATCH] i2c: designware: Fix bogus sda_hold_time due to uninitialized vars
In reply to Jan KiszkaQuoted 10 lines · Jan Kiszka
We need to initializes those variables to 0 for platforms that do not provide ACPI parameters. Otherwise, we set
sda_hold_timeto random values, breaking e.g. Galileo and IOT2000 boards.Fixes 9d6408433019 ("i2c: designware: don't infer timings described by ACPI from clock rate")Signed-off-by Jan Kiszka <[email protected]>---
Affects 4.12-rc2.
Wolfram Sang
AppliedRe: [PATCH] i2c: designware: Fix bogus sda_hold_time due to uninitialized vars
In reply to Jan KiszkaQuoted 6 lines · Jan Kiszka
We need to initializes those variables to 0 for platforms that do not provide ACPI parameters. Otherwise, we set
sda_hold_timeto random values, breaking e.g. Galileo and IOT2000 boards.Fixes 9d6408433019 ("i2c: designware: don't infer timings described by ACPI from clock rate")Signed-off-by Jan Kiszka <[email protected]>
Applied to for-current, thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170522/312b1807/attachment.sig>