Message-ID: <7f165170-ed25-7804-b756-4944a4067b8a@redhat.com>
Date:   Thu, 20 Jan 2022 16:57:10 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Jan Dąbroś <jsd@...ihalf.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Wolfram Sang <wsa@...nel.org>,
        Raul E Rangel <rrangel@...omium.org>,
        Marcin Wojtas <mw@...ihalf.com>,
        Grzegorz Jaszczyk <jaz@...ihalf.com>, upstream@...ihalf.com
Subject: Re: [PATCH 0/2] i2c-designware: Add support for AMD PSP semaphore

Hi,

On 1/20/22 13:29, Jan Dąbroś wrote:
> Hi Hans,
> 
> 
> czw., 20 sty 2022 o 12:15 Hans de Goede <hdegoede@...hat.com> 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
>>>
>>
> 

Powered by blists - more mailing lists