| Message ID | 20181222202623.4521-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | i2c: move handling of suspended adapters to the core | expand |
Hi, On 22-12-18 21:26, Wolfram Sang wrote: > Here is the new version without specific I2C helpers but using the > 'is_suspended' flag from the PM core. I didn't like messing with the > flag directly, so I did a helper in patch 1. So far, I like the > approach. The diffstat looks nice, and i2c-rcar.c and i2c-sh_mobile.c > rejected rightfully too later transfers without further modifications. > Tested on a Renesas Lager board (R-Car H2). > > I dropped a few Tested-by tags because I think this approach is too > different from V1 to keep them. I hope you guys can have a look again. > Thanks for all the testing, so far! > > A branch can be found here: > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/reject-when-suspended > > If this series is acceptable, I'd suggest to take it via my i2c tree > after rc1. And then I'll provide an immutable branch for the PM tree to > pick. Let me know if this works for you. > > And thanks to Renesas for funding this work! Series looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > > Thanks and kind regards, > > Wolfram > > Wolfram Sang (9): > PM / core: add helper to return suspend status of a device > i2c: reject new transfers when adapters are suspended > i2c: synquacer: remove unused is_suspended flag > i2c: brcmstb: don't open code to reject transfers when suspended > i2c: zx2967: don't open code to reject transfers when suspended > i2c: sprd: don't use pdev as variable name for struct device * > i2c: sprd: don't open code to reject transfers when suspended > i2c: exynos5: don't open code to reject transfers when suspended > i2c: s3c2410: don't open code to reject transfers when suspended > > Documentation/i2c/fault-codes | 4 ++++ > drivers/i2c/busses/i2c-brcmstb.c | 22 +------------------- > drivers/i2c/busses/i2c-exynos5.c | 10 ---------- > drivers/i2c/busses/i2c-s3c2410.c | 7 ------- > drivers/i2c/busses/i2c-sprd.c | 32 ++++++++++-------------------- > drivers/i2c/busses/i2c-synquacer.c | 5 ----- > drivers/i2c/busses/i2c-zx2967.c | 8 -------- > drivers/i2c/i2c-core-base.c | 3 +++ > drivers/i2c/i2c-core-smbus.c | 4 ++++ > include/linux/device.h | 5 +++++ > 10 files changed, 27 insertions(+), 73 deletions(-) >
Hi Wolfram, On Sat, Dec 22, 2018 at 9:26 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > Here is the new version without specific I2C helpers but using the > 'is_suspended' flag from the PM core. I didn't like messing with the > flag directly, so I did a helper in patch 1. So far, I like the > approach. The diffstat looks nice, and i2c-rcar.c and i2c-sh_mobile.c > rejected rightfully too later transfers without further modifications. > Tested on a Renesas Lager board (R-Car H2). > > I dropped a few Tested-by tags because I think this approach is too > different from V1 to keep them. I hope you guys can have a look again. > Thanks for all the testing, so far! > > A branch can be found here: > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/reject-when-suspended > > If this series is acceptable, I'd suggest to take it via my i2c tree > after rc1. And then I'll provide an immutable branch for the PM tree to > pick. Let me know if this works for you. This breaks resume of the CS2000 clock driver on Salvator-X(S) (and presumable ULCB, too): Accessing already suspended I2C/SMBus adapter WARNING: CPU: 1 PID: 1186 at drivers/i2c/i2c-core-smbus.c:551 __i2c_smbus_xfer+0x38/0x66c Modules linked in: CPU: 1 PID: 1186 Comm: s2idle Not tainted 4.20.0-salvator-x-08442-ge5992c41ac706409 #264 Hardware name: Renesas Salvator-X board based on r8a7795 ES1.x (DT) pstate: 60400005 (nZCv daif +PAN -UAO) pc : __i2c_smbus_xfer+0x38/0x66c lr : __i2c_smbus_xfer+0x38/0x66c sp : ffffff8013413880 x29: ffffff8013413880 x28: ffffffc6f78b4820 x27: 0000000000000010 x26: ffffff8010cf6178 x25: ffffff8013413976 x24: 0000000000000002 x23: 0000000000000016 x22: ffffffc6f7872088 x21: 0000000000000000 x20: 000000000000004f x19: ffffffc6f7872088 x18: 000000000000000a x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000029c80 x14: 0720072007200720 x13: 0720072007200720 x12: 0720072007200720 x11: 0720072007200720 x10: 0720072007200720 x9 : ffffff801100e6d0 x8 : 6461207375424d53 x7 : ffffff8011c825c8 x6 : ffffff8011c82000 x5 : 0000000000000000 x4 : ffffff8013414000 x3 : ffffff80134136e0 x2 : 00000046ee875000 x1 : 82c6d7c720d64000 x0 : 0000000000000000 Call trace: __i2c_smbus_xfer+0x38/0x66c i2c_smbus_xfer+0x64/0x98 i2c_smbus_read_byte_data+0x40/0x6c cs2000_bset.isra.1+0x2c/0x58 __cs2000_set_rate.constprop.7+0x8c/0x134 cs2000_resume+0x14/0x1c dpm_run_callback+0x15c/0x2d8 device_resume_early+0x98/0xec dpm_resume_early+0x3b0/0x454 suspend_devices_and_enter+0x7bc/0xbb0 The CS2000 driver uses SET_LATE_SYSTEM_SLEEP_PM_OPS(). Suspend/resume of the BD9571 driver works fine, as that one uses SET_SYSTEM_SLEEP_PM_OPS(). Gr{oetje,eeting}s, Geert
Hi, On 26-12-18 12:01, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Sat, Dec 22, 2018 at 9:26 PM Wolfram Sang > <wsa+renesas@sang-engineering.com> wrote: >> Here is the new version without specific I2C helpers but using the >> 'is_suspended' flag from the PM core. I didn't like messing with the >> flag directly, so I did a helper in patch 1. So far, I like the >> approach. The diffstat looks nice, and i2c-rcar.c and i2c-sh_mobile.c >> rejected rightfully too later transfers without further modifications. >> Tested on a Renesas Lager board (R-Car H2). >> >> I dropped a few Tested-by tags because I think this approach is too >> different from V1 to keep them. I hope you guys can have a look again. >> Thanks for all the testing, so far! >> >> A branch can be found here: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/reject-when-suspended >> >> If this series is acceptable, I'd suggest to take it via my i2c tree >> after rc1. And then I'll provide an immutable branch for the PM tree to >> pick. Let me know if this works for you. > > This breaks resume of the CS2000 clock driver on Salvator-X(S) (and > presumable ULCB, too): > > Accessing already suspended I2C/SMBus adapter > WARNING: CPU: 1 PID: 1186 at drivers/i2c/i2c-core-smbus.c:551 > __i2c_smbus_xfer+0x38/0x66c > Modules linked in: > CPU: 1 PID: 1186 Comm: s2idle Not tainted > 4.20.0-salvator-x-08442-ge5992c41ac706409 #264 > Hardware name: Renesas Salvator-X board based on r8a7795 ES1.x (DT) > pstate: 60400005 (nZCv daif +PAN -UAO) > pc : __i2c_smbus_xfer+0x38/0x66c > lr : __i2c_smbus_xfer+0x38/0x66c > sp : ffffff8013413880 > x29: ffffff8013413880 x28: ffffffc6f78b4820 > x27: 0000000000000010 x26: ffffff8010cf6178 > x25: ffffff8013413976 x24: 0000000000000002 > x23: 0000000000000016 x22: ffffffc6f7872088 > x21: 0000000000000000 x20: 000000000000004f > x19: ffffffc6f7872088 x18: 000000000000000a > x17: 0000000000000000 x16: 0000000000000000 > x15: 0000000000029c80 x14: 0720072007200720 > x13: 0720072007200720 x12: 0720072007200720 > x11: 0720072007200720 x10: 0720072007200720 > x9 : ffffff801100e6d0 x8 : 6461207375424d53 > x7 : ffffff8011c825c8 x6 : ffffff8011c82000 > x5 : 0000000000000000 x4 : ffffff8013414000 > x3 : ffffff80134136e0 x2 : 00000046ee875000 > x1 : 82c6d7c720d64000 x0 : 0000000000000000 > Call trace: > __i2c_smbus_xfer+0x38/0x66c > i2c_smbus_xfer+0x64/0x98 > i2c_smbus_read_byte_data+0x40/0x6c > cs2000_bset.isra.1+0x2c/0x58 > __cs2000_set_rate.constprop.7+0x8c/0x134 > cs2000_resume+0x14/0x1c > dpm_run_callback+0x15c/0x2d8 > device_resume_early+0x98/0xec > dpm_resume_early+0x3b0/0x454 > suspend_devices_and_enter+0x7bc/0xbb0 > > The CS2000 driver uses SET_LATE_SYSTEM_SLEEP_PM_OPS(). > > Suspend/resume of the BD9571 driver works fine, as that one uses > SET_SYSTEM_SLEEP_PM_OPS(). Ok, so this is the same thing I noticed, the approach using the pm-core is_suspend flag only works for adapters which suspend during the regular suspend phase and as such that approach cannot work everywhere. Regards, Hans