| Message ID | 20220208141218.2049591-1-jsd@semihalf.com |
|---|---|
| Headers | show |
| Series | i2c-designware: Add support for AMD PSP semaphore | expand |
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>
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
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, ¶m); > + 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 >
ś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