| Message ID | 20230303165050.2918-1-mario.limonciello@amd.com |
|---|---|
| Headers | show |
| Series | Export platform features from ccp driver | expand |
On 3/3/23 10:50, Mario Limonciello wrote: > The i2c-designware-amdpsp driver communicates with a platform > features mailbox provided by the PSP. The address used for > communication is discovered via a non-architecturally > guaranteed mechanism. > > To better scale, export a feature for communication with platform > features directly from the ccp driver. > If there is agreement with Jan and Grzegorz for patches 7-9, I'm ok with the rest. Acked-by: Tom Lendacky <thomas.lendacky@amd.com> > v2->v3: > * Split new ACPI ID to own patch > * Squash doorbell offsets into doorbell patch > * Fix all feedback from v2 (see individual patches for details) > Mario Limonciello (9): > crypto: ccp: Drop TEE support for IRQ handler > crypto: ccp: Add a header for multiple drivers to use `__psp_pa` > crypto: ccp: Move some PSP mailbox bit definitions into common header > crypto: ccp: Add support for an interface for platform features > crypto: ccp: Enable platform access interface on client PSP parts > i2c: designware: Use PCI PSP driver for communication > crypto: ccp: Add support for ringing a platform doorbell > i2c: designware: Add doorbell support for Skyrim > i2c: designware: Add support for AMDI0020 ACPI ID > > arch/x86/kvm/svm/sev.c | 1 + > drivers/crypto/ccp/Makefile | 3 +- > drivers/crypto/ccp/platform-access.c | 218 ++++++++++++++++++++ > drivers/crypto/ccp/platform-access.h | 35 ++++ > drivers/crypto/ccp/psp-dev.c | 32 +-- > drivers/crypto/ccp/psp-dev.h | 11 +- > drivers/crypto/ccp/sev-dev.c | 16 +- > drivers/crypto/ccp/sev-dev.h | 2 +- > drivers/crypto/ccp/sp-dev.h | 10 + > drivers/crypto/ccp/sp-pci.c | 9 + > drivers/crypto/ccp/tee-dev.c | 17 +- > drivers/i2c/busses/Kconfig | 2 +- > drivers/i2c/busses/i2c-designware-amdpsp.c | 179 +++------------- > drivers/i2c/busses/i2c-designware-core.h | 1 - > drivers/i2c/busses/i2c-designware-platdrv.c | 2 +- > drivers/tee/amdtee/call.c | 2 +- > drivers/tee/amdtee/shm_pool.c | 2 +- > include/linux/psp-platform-access.h | 65 ++++++ > include/linux/psp-sev.h | 8 - > include/linux/psp.h | 29 +++ > 20 files changed, 438 insertions(+), 206 deletions(-) > create mode 100644 drivers/crypto/ccp/platform-access.c > create mode 100644 drivers/crypto/ccp/platform-access.h > create mode 100644 include/linux/psp-platform-access.h > create mode 100644 include/linux/psp.h >
On Fri, Mar 03, 2023 at 10:50:47AM -0600, Mario Limonciello wrote: > Cezanne and Skyrim have the same PSP hardware but use a different > protocol to negotiate I2C arbitration. To disambiguate this going > forward introduce a new ACPI ID to represent the protocol that utilizes > a doorbell. > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v2->v3: > * Split from earlier patch to standalone > --- > drivers/i2c/busses/i2c-designware-amdpsp.c | 5 +++-- > drivers/i2c/busses/i2c-designware-platdrv.c | 1 + > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c > index 2c671973010d..44b8432458b0 100644 > --- a/drivers/i2c/busses/i2c-designware-amdpsp.c > +++ b/drivers/i2c/busses/i2c-designware-amdpsp.c > @@ -101,11 +101,12 @@ static int psp_send_i2c_req_amdi0019(enum psp_i2c_req_type i2c_req_type) > > static int psp_send_i2c_req(enum psp_i2c_req_type i2c_req_type) > { > + const char *hid = acpi_device_hid(ACPI_COMPANION(psp_i2c_dev)); > unsigned long start = jiffies; > int ret; > > - /* Use doorbell for Skyrim and mailbox for Cezanne */ > - if (boot_cpu_data.x86 == 25 && boot_cpu_data.x86_model == 80) Ah, in this form it's getting better than I thought! Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > + /* Use doorbell for AMDI0020 and mailbox for AMDI0019 */ > + if (!strcmp(hid, "AMDI0019")) > ret = psp_send_i2c_req_amdi0019(i2c_req_type); > else > ret = psp_ring_platform_doorbell(i2c_req_type); > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index 89ad88c54754..5ca71bda9ac2 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -51,6 +51,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = { > { "AMD0010", ACCESS_INTR_MASK }, > { "AMDI0010", ACCESS_INTR_MASK }, > { "AMDI0019", ACCESS_INTR_MASK | ARBITRATION_SEMAPHORE }, > + { "AMDI0020", ACCESS_INTR_MASK | ARBITRATION_SEMAPHORE }, > { "AMDI0510", 0 }, > { "APMC0D0F", 0 }, > { "HISI02A1", 0 }, > -- > 2.34.1 >
On 3/6/23 14:04, Andy Shevchenko wrote: > On Fri, Mar 03, 2023 at 10:50:47AM -0600, Mario Limonciello wrote: >> Cezanne and Skyrim have the same PSP hardware but use a different >> protocol to negotiate I2C arbitration. To disambiguate this going >> forward introduce a new ACPI ID to represent the protocol that utilizes >> a doorbell. > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> v2->v3: >> * Split from earlier patch to standalone >> --- >> drivers/i2c/busses/i2c-designware-amdpsp.c | 5 +++-- >> drivers/i2c/busses/i2c-designware-platdrv.c | 1 + >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c >> index 2c671973010d..44b8432458b0 100644 >> --- a/drivers/i2c/busses/i2c-designware-amdpsp.c >> +++ b/drivers/i2c/busses/i2c-designware-amdpsp.c >> @@ -101,11 +101,12 @@ static int psp_send_i2c_req_amdi0019(enum psp_i2c_req_type i2c_req_type) >> >> static int psp_send_i2c_req(enum psp_i2c_req_type i2c_req_type) >> { >> + const char *hid = acpi_device_hid(ACPI_COMPANION(psp_i2c_dev)); >> unsigned long start = jiffies; >> int ret; >> >> - /* Use doorbell for Skyrim and mailbox for Cezanne */ >> - if (boot_cpu_data.x86 == 25 && boot_cpu_data.x86_model == 80) > > Ah, in this form it's getting better than I thought! > These removed lines were added by previous patch. I think a bit too short lived if the same patchset adds and then removes lines?
On Mon, Mar 06, 2023 at 02:28:05PM +0200, Jarkko Nikula wrote: > On 3/6/23 14:04, Andy Shevchenko wrote: > > On Fri, Mar 03, 2023 at 10:50:47AM -0600, Mario Limonciello wrote: > > > Cezanne and Skyrim have the same PSP hardware but use a different > > > protocol to negotiate I2C arbitration. To disambiguate this going > > > forward introduce a new ACPI ID to represent the protocol that utilizes > > > a doorbell. ... > > > - if (boot_cpu_data.x86 == 25 && boot_cpu_data.x86_model == 80) > > > > Ah, in this form it's getting better than I thought! > > > These removed lines were added by previous patch. I think a bit too short > lived if the same patchset adds and then removes lines? That what I have missed. Okay, coming to square 1, i.e. dropping CPU ID completely from the series. Note, for testing purposes you may always add a HACK patch at the end of the series, marking it respectively. So, people may test it all and maintainer apply w/o unneeded tail.
On 3/6/23 06:55, Andy Shevchenko wrote: > On Mon, Mar 06, 2023 at 02:28:05PM +0200, Jarkko Nikula wrote: >> On 3/6/23 14:04, Andy Shevchenko wrote: >>> On Fri, Mar 03, 2023 at 10:50:47AM -0600, Mario Limonciello wrote: >>>> Cezanne and Skyrim have the same PSP hardware but use a different >>>> protocol to negotiate I2C arbitration. To disambiguate this going >>>> forward introduce a new ACPI ID to represent the protocol that utilizes >>>> a doorbell. > > ... > >>>> - if (boot_cpu_data.x86 == 25 && boot_cpu_data.x86_model == 80) >>> >>> Ah, in this form it's getting better than I thought! >>> >> These removed lines were added by previous patch. I think a bit too short >> lived if the same patchset adds and then removes lines? > > That what I have missed. Okay, coming to square 1, i.e. dropping CPU ID > completely from the series. > > Note, for testing purposes you may always add a HACK patch at the end of the > series, marking it respectively. So, people may test it all and maintainer > apply w/o unneeded tail. > If it still works then new ID can be reserved and patches 8 and 9 could be squashed together either by subsystem maintainer when merging or for v4. My apologies if this wasn't obvious to reviewers. My goal was to separate the scalability and functionality for test purposes. The way I did it was the series could be tested with patches 1-8 on both Cezanne and Skyrim platforms and no BIOS changes. If it works, BIOS for Skyrim can be patched and patch 9 could be added to test kernel.
[Public] > -----Original Message----- > From: Limonciello, Mario > Sent: Monday, March 6, 2023 07:12 > To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Jarkko Nikula > <jarkko.nikula@linux.intel.com> > Cc: Jan Dąbroś <jsd@semihalf.com>; Grzegorz Bernacki > <gjb@semihalf.com>; Thomas, Rijo-john <Rijo-john.Thomas@amd.com>; > Lendacky, Thomas <Thomas.Lendacky@amd.com>; > herbert@gondor.apana.org.au; Mika Westerberg > <mika.westerberg@linux.intel.com>; linux-i2c@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH v3 9/9] i2c: designware: Add support for AMDI0020 ACPI > ID > > On 3/6/23 06:55, Andy Shevchenko wrote: > > On Mon, Mar 06, 2023 at 02:28:05PM +0200, Jarkko Nikula wrote: > >> On 3/6/23 14:04, Andy Shevchenko wrote: > >>> On Fri, Mar 03, 2023 at 10:50:47AM -0600, Mario Limonciello wrote: > >>>> Cezanne and Skyrim have the same PSP hardware but use a different > >>>> protocol to negotiate I2C arbitration. To disambiguate this going > >>>> forward introduce a new ACPI ID to represent the protocol that utilizes > >>>> a doorbell. > > > > ... > > > >>>> - if (boot_cpu_data.x86 == 25 && boot_cpu_data.x86_model == 80) > >>> > >>> Ah, in this form it's getting better than I thought! > >>> > >> These removed lines were added by previous patch. I think a bit too short > >> lived if the same patchset adds and then removes lines? > > > > That what I have missed. Okay, coming to square 1, i.e. dropping CPU ID > > completely from the series. > > > > Note, for testing purposes you may always add a HACK patch at the end of > the > > series, marking it respectively. So, people may test it all and maintainer > > apply w/o unneeded tail. > > > > If it still works then new ID can be reserved and patches 8 and 9 could > be squashed together either by subsystem maintainer when merging or for > v4. My apologies if this wasn't obvious to reviewers. My goal was to > separate the scalability and functionality for test purposes. > > The way I did it was the series could be tested with patches 1-8 on both > Cezanne and Skyrim platforms and no BIOS changes. If it works, BIOS for > Skyrim can be patched and patch 9 could be added to test kernel. I've found that AMDI0020 is already reserved and also in use for a while. f5eda99ee6c0c ("ACPI / APD: Add device HID for future AMD UART controller") Even if patches 1-9 all work with a patched BIOS to advertise AMDI0020 instead of AMDI0019, besides squashing patch 8 and 9 will need to discuss what ID to use. For this reason, I would suggest that if 1-8 work and there is agreement on them then merge 1-8 and patch 9 can be a later follow up if/after that discussion and alignment with stakeholders.