Hi, On 1/20/22 13:29, Jan Dąbroś wrote: > 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. You are right that the whole sharing of the bus to the PMIC between the SoC's internal power-management microcontroller (P-Unit) and the OS is a bit fragile (it really is a bit crazy design IMHO). You are also right that disabling the controller on suspend is a problem, because once everything is suspended and we hit deeper power-saving states then the P-Unit actually needs the controller to tell the PMIC to disable certain regulators; and the P-Unit is not prepared for us having turned the controller off, therefor dw_i2c_plat_suspend() looks like this: static int dw_i2c_plat_suspend(struct device *dev) { struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); i_dev->suspended = true; if (i_dev->shared_with_punit) return 0; ... Note the shared_with_punit flag, so on the Bay Trail case i2c_dw_disable() never gets called on suspend, so that should not be an issue. So all in all I don't really expect any problems, still thank you for Cc-ing me. Regards, Hans >> 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 >>> >> >
