RE: [PATCH v7 5/6] i2c: designware: Use PCI PSP driver for communication

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Jan Dąbroś <jsd@xxxxxxxxxxxx>
> Sent: Friday, March 31, 2023 07:04
> To: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
> Cc: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Grzegorz Bernacki
> <gjb@xxxxxxxxxxxx>; Mark Hasemeyer <markhas@xxxxxxxxxxxx>; Andy
> Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Mika Westerberg
> <mika.westerberg@xxxxxxxxxxxxxxx>; Held, Felix <Felix.Held@xxxxxxx>;
> linux-i2c@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 5/6] i2c: designware: Use PCI PSP driver for
> communication
> 
> pt., 31 mar 2023 o 13:53 Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
> napisał(a):
> >
> > On 3/30/23 01:07, Mario Limonciello wrote:
> > > Currently the PSP semaphore communication base address is discovered
> > > by using an MSR that is not architecturally guaranteed for future
> > > platforms.  Also the mailbox that is utilized for communication with
> > > the PSP may have other consumers in the kernel, so it's better to
> > > make all communication go through a single driver.
> > >
> > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > > ---
> > > v6->v7:
> > >   * Also imply CRYPTO_DEV_CCP_DD to fix build errors
> > >   * Fix error message acquire/release inversion
> > > v5->v6:
> > >   * Drop now unnecessary asm/msr.h header
> > >   * Fix IS_REACHABLE
> > >   * Drop tags
> > >   * Fix status code handling for Cezanne
> > > v4->v5:
> > >   * Pick up tags
> > > v3->v4:
> > >   * Pick up tags
> > > v1->v2:
> > >   * Fix Kconfig to use imply
> > >   * Use IS_REACHABLE
> > > ---
> > >   drivers/i2c/busses/Kconfig                  |   3 +-
> > >   drivers/i2c/busses/i2c-designware-amdpsp.c  | 177 +++-----------------
> > >   drivers/i2c/busses/i2c-designware-core.h    |   1 -
> > >   drivers/i2c/busses/i2c-designware-platdrv.c |   1 -
> > >   include/linux/psp-platform-access.h         |   1 +
> > >   5 files changed, 29 insertions(+), 154 deletions(-)
> > >
> > One note below in case there is a need to have another version of if you
> > want. Not a show stopper for this.
> >
> > Acked-by: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
> >
> > > @@ -214,7 +95,7 @@ static int psp_send_i2c_req(enum
> psp_i2c_req_type i2c_req_type)
> > >       if (ret) {
> > >               dev_err(psp_i2c_dev, "Timed out waiting for PSP to %s I2C
> bus\n",
> > >                       (i2c_req_type == PSP_I2C_REQ_ACQUIRE) ?
> > > -                     "release" : "acquire");
> > > +                     "acquire" : "release");
> > >               goto cleanup;
> > >       }
> > >
> > This looks like worth of being an own patch. Maybe even for the
> > linux-stable. I think it's very useful to have an error message to show
> > correct information what operation actually failed.
> 
> Msg here is "TImed out waiting for _PSP to_ %s I2C bus" - when x86
> wants to ACQUIRE the bus (i2c_req_type == PSP_I2C_REQ_ACQUIRE) then
> PSP needs to RELEASE it and vice versa. If you think logic here is not
> straightforward and should be adjusted, then we need to change the
> whole sentence.

Thanks for clarifying it.  We should just drop this hunk then, it was correct
previously.

I'll wait for Mark or Grzegorz testing results to spin again with the tags
And dropping this hunk.