wt., 9 sie 2022 o 14:05 Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx> napisał(a): > > Hi > > Sorry the delay, this slipped through my eyes during vacation. Couple > minor comments below. > > On 7/25/22 11:02, Jan Dabros wrote: > > In order to optimize performance, limit amount of back and forth > > transactions between x86 and PSP. This is done by introduction of > > cooldown period - that is window in which x86 isn't releasing the bus > > immediately after each I2C transaction. > > > > In order to protect PSP from being starved while waiting for > > arbitration, after a programmed time bus is automatically released by a > > deferred function. > > > > Signed-off-by: Jan Dabros <jsd@xxxxxxxxxxxx> > > --- > > drivers/i2c/busses/i2c-designware-amdpsp.c | 68 +++++++++++++++++----- > > 1 file changed, 53 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c > > index b624356c945f..2e1bb5ae72c3 100644 > > --- a/drivers/i2c/busses/i2c-designware-amdpsp.c > > +++ b/drivers/i2c/busses/i2c-designware-amdpsp.c > > @@ -6,6 +6,7 @@ > > #include <linux/io-64-nonatomic-lo-hi.h> > > #include <linux/psp-sev.h> > > #include <linux/types.h> > > +#include <linux/workqueue.h> > > > > #include <asm/msr.h> > > > > @@ -15,6 +16,8 @@ > > #define PSP_MBOX_OFFSET 0x10570 > > #define PSP_CMD_TIMEOUT_US (500 * USEC_PER_MSEC) > > > > +#define PSP_I2C_COOLDOWN_TIME_MS 100 > > + > > "cooldown" distract me thinking thermal management. Would semaphore > reservation time/timer fit better? Yes, it makes sense. I will change this here and in the commit message to "semaphore reservation timer". > > > +static void release_bus_now(void) > > +static void psp_release_i2c_bus_deferred(struct work_struct *work) > > +static DECLARE_DELAYED_WORK(release_queue, psp_release_i2c_bus_deferred); > > + > > I'd use the same namespace here. Perhaps _now can be dropped from the > name since the release_bus and release_bus_deferred are near to each > other and _deferred variant implies it's called after timeout. Right, release_bus_now -> release_bus. > > > + /* > > + * Send a release command to PSP if the cooldown timeout elapsed but x86 still > > + * owns the ctrlr. > > + */ > > Replace "ctrlr" -> "control" here since then it doesn't lead to think > is't some technical object like register etc. This is about "controller" not "control", but I think your comment is still applicable. Best Regards, Jan
