Date:   Wed, 22 Dec 2021 20:56:21 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Jan Dabros <jsd@semihalf.com>, linux-kernel@vger.kernel.org,
        linux-i2c@vger.kernel.org, jarkko.nikula@linux.intel.com,
        mika.westerberg@linux.intel.com, wsa@kernel.org,
        rrangel@chromium.org, mw@semihalf.com, jaz@semihalf.com,
        upstream@semihalf.com
Subject: Re: [RFC 0/2] i2c-designware: Add support for AMD PSP semaphore
Message-ID: <20211222175621.7gikyvqu7xvc2qxb@mobilestation>
References: <20211222094558.2098791-1-jsd@semihalf.com>
 <YcMP/9ATna3/yckC@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YcMP/9ATna3/yckC@smile.fi.intel.com>
Precedence: bulk

Hi Andy, Jan

On Wed, Dec 22, 2021 at 01:46:07PM +0200, Andy Shevchenko wrote:
> +Serge
> 
> On Wed, Dec 22, 2021 at 10:45:56AM +0100, 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.
> > 
> > Mark this patchset as RFC, since waiting for new ACPI ID value. As a temporary
> > measure use "AMDI9999". Once proper one will be ready, will re-send this CL for
> > review & merge.
> > 
> > Looking forward to some feedback.
> 
> If I am not mistaken something similar happened in Baikal T1.
> Perhaps Serge has something to share.

No, Baikal-T1 doesn't have such specific interface sharing since it
doesn't have any co-processor (though a scenario of booting different
kernels on each CPU core was at consideration by some our customers).
So the only peculiar things the SoC has got are two interfaces with
non-standard access:
1) DW SPI controller with memory mapped 16MB direct EEPROM access. DW
SPI CSR/EEPROM mapping are switched by a multiplexer (basically just a
flag) embedded into the system controller.
2) DW i2c controller with indirect registers access. Originally it was
supposed to be used by the system bootloader for some bootloading
stuff, but the actual usage scenario wasn't described by the SoC
engineers. The chip initially loads the code from the SPI-flash only,
which can be of up to 16MB size. It's more than enough to start pretty
complex systems, so an additional i2c interface turned to be not
really needed. Anyway other than having an indirectly accessible
CSRs it's pretty much normal DW I2C controller.

But you are right in a reference to another BE-chip - Baikal-M1. The
i2c/spi/gpio/uart interfaces sharing support might get to be needed
for it since aside with four 2-cored Cortex-A57 clusters it has got an
embedded SCP co-processor which can access the same SoC interfaces as
the CPU cores. Though Baikal-M1 isn't supported by the mainline kernel
at the moment. We are going to start working on it on the next year.
Then we'll most likely need to implement the interface sharing feature
like the one introduced in this RFC. But for now alas I can't be much
helpful.

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>