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

[Jan Dąbroś]

czw., 20 sty 2022 o 16:57 Hans de Goede <hdegoede@xxxxxxxxxx> napisał(a):
>
> 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.

Thanks for pointing this! So actually the only path which is now (with
my patch) altered on the Bay Trails is unbinding driver from the
device which will call dw_i2c_driver.remove().

>
> So all in all I don't really expect any problems, still thank
> you for Cc-ing me.

Thanks a lot for your help with testing and reviewing my code.

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