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

[Jan Dąbroś]

Hi Serge, Andy,

Thanks for your comments. Glad to hear that this work may (possibly)
be helpful for even broader audience. For most of the stuff I tried to
create generic code and actually these solutions are already meant to
be shared with platforms using baytrail semaphore.

Best Regards,
Jan


śr., 22 gru 2021 o 19:22 Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> napisał(a):
>
> On Wed, Dec 22, 2021 at 08:56:21PM +0300, Serge Semin wrote:
> > On Wed, Dec 22, 2021 at 01:46:07PM +0200, Andy Shevchenko wrote:
> > > 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.
>
> Anyway, thanks for your insights!
>
> --
> With Best Regards,
> Andy Shevchenko
>
>