Re: [PATCH 0/2] i2c-designware: Add support for AMD PSP semaphore

[Jan Dąbroś]

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
> >
>