mbox series

[v4,0/2] i2c-designware: Add support for AMD PSP semaphore

Message ID 20220208141218.2049591-1-jsd@semihalf.com
Headers show
Series i2c-designware: Add support for AMD PSP semaphore | expand

Message

Jan Dąbroś Feb. 8, 2022, 2:12 p.m. UTC
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

v3->v4:
* Remove unnecessary alignment of psp_i2c_req
* Add missing bits.h header
* Make use of USEC_PER_MSEC
* Simplify `if` conditions with unsigned variables
* Add additional comments

v2 -> v3:
* Change X86_64 Kconfig dependency to X86_MSR
* Switch from phys_addr_t to u64 in mailbox struct definition
* Remove redundant guard in semaphores' probes
* Add comments about error propagation
* Move credits for kernel test robot into changelog

v1 -> v2:
* Remove usage of unions
* Get rid of unnecessary __packed attributes
* Switch to use iopoll.h and bitfields.h APIs were applicable
* Follow the convention to check for the error first
* Reorder entries (includes, table entries) alphabetically
* Add necessary includes
* Add Kconfig dependency on X86_64
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
* Modify probe() to use terminating entry for traversing through table
  instead of ARRAY_SIZE
* Fix typos in comments
* Rebase patchset

Jan Dabros (2):
  i2c: designware: Add missing locks
  i2c: designware: Add AMD PSP I2C bus support

 MAINTAINERS                                  |   1 +
 drivers/acpi/acpi_apd.c                      |   7 +-
 drivers/i2c/busses/Kconfig                   |  11 +
 drivers/i2c/busses/Makefile                  |   1 +
 drivers/i2c/busses/i2c-designware-amdpsp.c   | 394 +++++++++++++++++++
 drivers/i2c/busses/i2c-designware-baytrail.c |  12 +-
 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  |  60 +++
 10 files changed, 510 insertions(+), 12 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-designware-amdpsp.c

Comments

Jarkko Nikula Feb. 9, 2022, 3:01 p.m. UTC | #1
On 2/8/22 16:12, Jan Dabros wrote:
> All accesses to controller's registers should be protected on
> probe, disable and xfer paths. This is needed for i2c bus controllers
> that are shared with but not controller by kernel.
> 
> Signed-off-by: Jan Dabros <jsd@semihalf.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/i2c/busses/i2c-designware-common.c | 12 ++++++++++++
>   drivers/i2c/busses/i2c-designware-master.c |  6 ++++++
>   2 files changed, 18 insertions(+)
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Shortly tested by rmmod & modprobe loop.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Jarkko Nikula Feb. 9, 2022, 3:11 p.m. UTC | #2
On 2/8/22 16:12, 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.
> 
I'm going to run overnight with both patches a test case that used to 
cause some activity on a shared I2C bus on Baytrail based MRD 7 tablet. 
Test below used to trigger system hang after a few hours - days before 
some PUNIT - graphics related issue was fixed a few years ago:

#!/bin/sh
X &
export DISPLAY=:0
sleep 2
xclock -update 30 -digital -geometry 500x50+1+1027 &
xload -update 60 -bg black -hl red -fg green -geometry 1916x250+1+774 &
sleep 1
xsetroot -solid red
xset s noblank s off -dpms
glxgears >/dev/null &
while :; do acpi -b; sleep 1.2; done
Andy Shevchenko Feb. 9, 2022, 3:23 p.m. UTC | #3
On Tue, Feb 08, 2022 at 03:12:17PM +0100, Jan Dabros wrote:
> All accesses to controller's registers should be protected on
> probe, disable and xfer paths. This is needed for i2c bus controllers
> that are shared with but not controller by kernel.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Jan Dabros <jsd@semihalf.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/i2c/busses/i2c-designware-common.c | 12 ++++++++++++
>  drivers/i2c/busses/i2c-designware-master.c |  6 ++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index bf2a4920638a..9f8574320eb2 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -578,7 +578,12 @@ int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
>  	 * Try to detect the FIFO depth if not set by interface driver,
>  	 * the depth could be from 2 to 256 from HW spec.
>  	 */
> +	ret = i2c_dw_acquire_lock(dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = regmap_read(dev->map, DW_IC_COMP_PARAM_1, &param);
> +	i2c_dw_release_lock(dev);
>  	if (ret)
>  		return ret;
>  
> @@ -607,6 +612,11 @@ u32 i2c_dw_func(struct i2c_adapter *adap)
>  void i2c_dw_disable(struct dw_i2c_dev *dev)
>  {
>  	u32 dummy;
> +	int ret;
> +
> +	ret = i2c_dw_acquire_lock(dev);
> +	if (ret)
> +		return;
>  
>  	/* Disable controller */
>  	__i2c_dw_disable(dev);
> @@ -614,6 +624,8 @@ void i2c_dw_disable(struct dw_i2c_dev *dev)
>  	/* Disable all interrupts */
>  	regmap_write(dev->map, DW_IC_INTR_MASK, 0);
>  	regmap_read(dev->map, DW_IC_CLR_INTR, &dummy);
> +
> +	i2c_dw_release_lock(dev);
>  }
>  
>  void i2c_dw_disable_int(struct dw_i2c_dev *dev)
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index 9177463c2cbb..1a4b23556db3 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -905,7 +905,13 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
>  		irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND;
>  	}
>  
> +	ret = i2c_dw_acquire_lock(dev);
> +	if (ret)
> +		return ret;
> +
>  	i2c_dw_disable_int(dev);
> +	i2c_dw_release_lock(dev);
> +
>  	ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, irq_flags,
>  			       dev_name(dev->dev), dev);
>  	if (ret) {
> -- 
> 2.35.0.263.gb82422642f-goog
>
Jan Dąbroś Feb. 10, 2022, 8:13 a.m. UTC | #4
śr., 9 lut 2022 o 16:11 Jarkko Nikula <jarkko.nikula@linux.intel.com>
napisał(a):
>
> On 2/8/22 16:12, 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.
> >
> I'm going to run overnight with both patches a test case that used to
> cause some activity on a shared I2C bus on Baytrail based MRD 7 tablet.
> Test below used to trigger system hang after a few hours - days before
> some PUNIT - graphics related issue was fixed a few years ago:
>
> #!/bin/sh
> X &
> export DISPLAY=:0
> sleep 2
> xclock -update 30 -digital -geometry 500x50+1+1027 &
> xload -update 60 -bg black -hl red -fg green -geometry 1916x250+1+774 &
> sleep 1
> xsetroot -solid red
> xset s noblank s off -dpms
> glxgears >/dev/null &
> while :; do acpi -b; sleep 1.2; done

Thanks, looking forward to the results.

Best Regards,
Jan