Hi Hans, czw., 20 sty 2022 o 12:15 Hans de Goede <hdegoede@xxxxxxxxxx> napisał(a): > > Hi Jan, > > On 1/20/22 01:16, Jan Dabros wrote: > > This patchset comprises support for new i2c-designware controller setup on some > > AMD Cezanne SoCs, where x86 is sharing i2c bus with PSP. PSP uses the same > > controller and acts as an i2c arbitrator there (x86 is leasing bus from it). > > > > First commit aims to improve generic i2c-designware code by adding extra locking > > on probe() and disable() paths. I would like to ask someone with access to > > boards which use Intel BayTrail(CONFIG_I2C_DESIGNWARE_BAYTRAIL) to verify > > behavior of my changes on such setup. > > > > Second commit adds support for new PSP semaphore arbitration mechanism. > > Implementation is similar to the one from i2c-designware-baytrail.c however > > there are two main differences: > > 1) Add new ACPI ID in order to protect against silent binding of the old driver > > to the setup with PSP semaphore. Extra flag ARBITRATION_SEMAPHORE added to this > > new _HID allows to recognize setup with PSP. > > 2) Beside acquire_lock() and release_lock() methods we are also applying quirks > > to the lock_bus() and unlock_bus() global adapter methods. With this in place > > all i2c clients drivers may lock i2c bus for a desired number of i2c > > transactions (e.g. write-wait-read) without being aware of that such bus is > > shared with another entity. > > > > This patchset is a follow-up to the RFC sent earlier on LKML [1], with review > > comments applied. > > > > Looking forward to some feedback. > > > > [1] https://lkml.org/lkml/2021/12/22/219 > > > Thank you for your patch series. > > As you may have seen I've done a lot of work on the Bay Trail semaphore > thing. I also own several Bay Trail and Cherry Trail based devices which > use this setup. > > I'll add your patches to my personal WIP tree which I regularly run > on these devices and I'll report back if I notice any issues. Thanks in advance, this will be really helpful! I don't have Bay Trail/Cherry Trail, so I've only tested that build of Bay Trail semaphore isn't broken. I would like to point to new locks in i2c_dw_disable() method as something to be the most fragile and error-prone, will be great if you can verify this thoroughly. This function is invoked on both dw_i2c_driver.remove() and dw_i2c_plat_suspend() paths. Considering that Bay Trail semaphore means that i2c bus is shared with PMIC, I'm not sure whether all corner cases are secured especially on platform suspend. > > One remark, I notice that there are no AMD people in the Cc, it > would be good if you can find someone from AMD to look at this, > also see my remarks to the 2nd patch in my reply to that patch. This was partially discussed with AMD folks and you are right that I should include someone from AMD to take a look at this. Thanks for all your comments! > Regards, > > Hans > > > > > > > > Jan Dabros (2): > > i2c: designware: Add missing locks > > i2c: designware: Add AMD PSP I2C bus support > > > > MAINTAINERS | 1 + > > drivers/acpi/acpi_apd.c | 1 + > > drivers/i2c/busses/Kconfig | 10 + > > drivers/i2c/busses/Makefile | 1 + > > drivers/i2c/busses/i2c-designware-amdpsp.c | 357 +++++++++++++++++++ > > drivers/i2c/busses/i2c-designware-baytrail.c | 10 +- > > drivers/i2c/busses/i2c-designware-common.c | 12 + > > drivers/i2c/busses/i2c-designware-core.h | 18 +- > > drivers/i2c/busses/i2c-designware-master.c | 6 + > > drivers/i2c/busses/i2c-designware-platdrv.c | 61 ++++ > > 10 files changed, 469 insertions(+), 8 deletions(-) > > create mode 100644 drivers/i2c/busses/i2c-designware-amdpsp.c > > >
