| Message ID | 20230209223811.4993-4-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Export platform features from ccp driver | expand |
(...) > @@ -99,7 +93,7 @@ static int psp_check_mbox_recovery(struct psp_mbox __iomem *mbox) > > tmp = readl(&mbox->cmd_fields); > > - return FIELD_GET(PSP_MBOX_FIELDS_RECOVERY, tmp); > + return FIELD_GET(PSP_CMDRESP_RECOVERY, tmp); > } > > static int psp_wait_cmd(struct psp_mbox __iomem *mbox) > @@ -107,7 +101,7 @@ static int psp_wait_cmd(struct psp_mbox __iomem *mbox) > u32 tmp, expected; > > /* Expect mbox_cmd to be cleared and ready bit to be set by PSP */ > - expected = FIELD_PREP(PSP_MBOX_FIELDS_READY, 1); > + expected = FIELD_PREP(PSP_CMDRESP_RESP, 1); What's the meaning of "PSP_CMDRESP_RESP"? I see that this new macro name is currently used by other drivers, but in my opinion "READY" is more descriptive. (It is also aligned to the comment above this line.)
[Public] > -----Original Message----- > From: Jan Dąbroś <jsd@semihalf.com> > Sent: Tuesday, February 14, 2023 03:04 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Grzegorz Bernacki <gjb@semihalf.com>; Thomas, Rijo-john <Rijo- > john.Thomas@amd.com>; Lendacky, Thomas > <Thomas.Lendacky@amd.com>; herbert@gondor.apana.org.au; Allen, John > <John.Allen@amd.com>; Singh, Brijesh <Brijesh.Singh@amd.com>; Jarkko > Nikula <jarkko.nikula@linux.intel.com>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; Mika Westerberg > <mika.westerberg@linux.intel.com>; linux-i2c@vger.kernel.org; linux- > crypto@vger.kernel.org; linux-kernel@vger.kernel.org; David S. Miller > <davem@davemloft.net> > Subject: Re: [PATCH 3/6] crypto: ccp: Move some PSP mailbox bit definitions > into common header > > (...) > > @@ -99,7 +93,7 @@ static int psp_check_mbox_recovery(struct psp_mbox > __iomem *mbox) > > > > tmp = readl(&mbox->cmd_fields); > > > > - return FIELD_GET(PSP_MBOX_FIELDS_RECOVERY, tmp); > > + return FIELD_GET(PSP_CMDRESP_RECOVERY, tmp); > > } > > > > static int psp_wait_cmd(struct psp_mbox __iomem *mbox) > > @@ -107,7 +101,7 @@ static int psp_wait_cmd(struct psp_mbox __iomem > *mbox) > > u32 tmp, expected; > > > > /* Expect mbox_cmd to be cleared and ready bit to be set by PSP */ > > - expected = FIELD_PREP(PSP_MBOX_FIELDS_READY, 1); > > + expected = FIELD_PREP(PSP_CMDRESP_RESP, 1); > > What's the meaning of "PSP_CMDRESP_RESP"? I see that this new macro > name is currently used by other drivers, but in my opinion "READY" is > more descriptive. (It is also aligned to the comment above this line.) It should indicate that the PSP has responded. I think both terms work to describe what's going on. Tom - What's your preference? I'll either adjust all the drivers to use READY or fix the comment for v2.
On 2/14/23 16:05, Limonciello, Mario wrote: > [Public] > > > >> -----Original Message----- >> From: Jan Dąbroś <jsd@semihalf.com> >> Sent: Tuesday, February 14, 2023 03:04 >> To: Limonciello, Mario <Mario.Limonciello@amd.com> >> Cc: Grzegorz Bernacki <gjb@semihalf.com>; Thomas, Rijo-john <Rijo- >> john.Thomas@amd.com>; Lendacky, Thomas >> <Thomas.Lendacky@amd.com>; herbert@gondor.apana.org.au; Allen, John >> <John.Allen@amd.com>; Singh, Brijesh <Brijesh.Singh@amd.com>; Jarkko >> Nikula <jarkko.nikula@linux.intel.com>; Andy Shevchenko >> <andriy.shevchenko@linux.intel.com>; Mika Westerberg >> <mika.westerberg@linux.intel.com>; linux-i2c@vger.kernel.org; linux- >> crypto@vger.kernel.org; linux-kernel@vger.kernel.org; David S. Miller >> <davem@davemloft.net> >> Subject: Re: [PATCH 3/6] crypto: ccp: Move some PSP mailbox bit definitions >> into common header >> >> (...) >>> @@ -99,7 +93,7 @@ static int psp_check_mbox_recovery(struct psp_mbox >> __iomem *mbox) >>> >>> tmp = readl(&mbox->cmd_fields); >>> >>> - return FIELD_GET(PSP_MBOX_FIELDS_RECOVERY, tmp); >>> + return FIELD_GET(PSP_CMDRESP_RECOVERY, tmp); >>> } >>> >>> static int psp_wait_cmd(struct psp_mbox __iomem *mbox) >>> @@ -107,7 +101,7 @@ static int psp_wait_cmd(struct psp_mbox __iomem >> *mbox) >>> u32 tmp, expected; >>> >>> /* Expect mbox_cmd to be cleared and ready bit to be set by PSP */ >>> - expected = FIELD_PREP(PSP_MBOX_FIELDS_READY, 1); >>> + expected = FIELD_PREP(PSP_CMDRESP_RESP, 1); >> >> What's the meaning of "PSP_CMDRESP_RESP"? I see that this new macro >> name is currently used by other drivers, but in my opinion "READY" is >> more descriptive. (It is also aligned to the comment above this line.) > > It should indicate that the PSP has responded. I think both terms work > to describe what's going on. > > Tom - What's your preference? > I'll either adjust all the drivers to use READY or fix the comment for v2. I think the comment should be changed. Thanks, Tom
diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h index 06e1f317216d2..55f54bb2b3fba 100644 --- a/drivers/crypto/ccp/psp-dev.h +++ b/drivers/crypto/ccp/psp-dev.h @@ -17,9 +17,6 @@ #include "sp-dev.h" -#define PSP_CMDRESP_RESP BIT(31) -#define PSP_CMDRESP_ERR_MASK 0xffff - #define MAX_PSP_NAME_LEN 16 extern struct psp_device *psp_master; diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 28945ca7c8563..6440d35dfa4ee 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -7,6 +7,7 @@ * Author: Brijesh Singh <brijesh.singh@amd.com> */ +#include <linux/bitfield.h> #include <linux/module.h> #include <linux/kernel.h> #include <linux/kthread.h> @@ -103,7 +104,7 @@ static void sev_irq_handler(int irq, void *data, unsigned int status) /* Check if it is SEV command completion: */ reg = ioread32(sev->io_regs + sev->vdata->cmdresp_reg); - if (reg & PSP_CMDRESP_RESP) { + if (FIELD_GET(PSP_CMDRESP_RESP, reg)) { sev->int_rcvd = 1; wake_up(&sev->int_queue); } @@ -347,9 +348,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) sev->int_rcvd = 0; - reg = cmd; - reg <<= SEV_CMDRESP_CMD_SHIFT; - reg |= SEV_CMDRESP_IOC; + reg = FIELD_PREP(SEV_CMDRESP_CMD, cmd) | SEV_CMDRESP_IOC; iowrite32(reg, sev->io_regs + sev->vdata->cmdresp_reg); /* wait for command completion */ @@ -367,11 +366,11 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) psp_timeout = psp_cmd_timeout; if (psp_ret) - *psp_ret = reg & PSP_CMDRESP_ERR_MASK; + *psp_ret = FIELD_GET(PSP_CMDRESP_STS, reg); - if (reg & PSP_CMDRESP_ERR_MASK) { - dev_dbg(sev->dev, "sev command %#x failed (%#010x)\n", - cmd, reg & PSP_CMDRESP_ERR_MASK); + if (FIELD_GET(PSP_CMDRESP_STS, reg)) { + dev_dbg(sev->dev, "sev command %#x failed (%#010lx)\n", + cmd, FIELD_GET(PSP_CMDRESP_STS, reg)); ret = -EIO; } else { ret = sev_write_init_ex_file_if_required(cmd); diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h index 666c21eb81ab3..778c95155e745 100644 --- a/drivers/crypto/ccp/sev-dev.h +++ b/drivers/crypto/ccp/sev-dev.h @@ -25,8 +25,8 @@ #include <linux/miscdevice.h> #include <linux/capability.h> +#define SEV_CMDRESP_CMD GENMASK(26, 16) #define SEV_CMD_COMPLETE BIT(1) -#define SEV_CMDRESP_CMD_SHIFT 16 #define SEV_CMDRESP_IOC BIT(0) struct sev_misc_dev { diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c index f24fc953718a0..5560bf8329a12 100644 --- a/drivers/crypto/ccp/tee-dev.c +++ b/drivers/crypto/ccp/tee-dev.c @@ -8,6 +8,7 @@ * Copyright (C) 2019,2021 Advanced Micro Devices, Inc. */ +#include <linux/bitfield.h> #include <linux/types.h> #include <linux/mutex.h> #include <linux/delay.h> @@ -69,7 +70,7 @@ static int tee_wait_cmd_poll(struct psp_tee_device *tee, unsigned int timeout, while (--nloop) { *reg = ioread32(tee->io_regs + tee->vdata->cmdresp_reg); - if (*reg & PSP_CMDRESP_RESP) + if (FIELD_GET(PSP_CMDRESP_RESP, *reg)) return 0; usleep_range(10000, 10100); @@ -149,9 +150,9 @@ static int tee_init_ring(struct psp_tee_device *tee) goto free_buf; } - if (reg & PSP_CMDRESP_ERR_MASK) { - dev_err(tee->dev, "tee: ring init command failed (%#010x)\n", - reg & PSP_CMDRESP_ERR_MASK); + if (FIELD_GET(PSP_CMDRESP_STS, reg)) { + dev_err(tee->dev, "tee: ring init command failed (%#010lx)\n", + FIELD_GET(PSP_CMDRESP_STS, reg)); tee_free_ring(tee); ret = -EIO; } @@ -179,9 +180,9 @@ static void tee_destroy_ring(struct psp_tee_device *tee) ret = tee_wait_cmd_poll(tee, TEE_DEFAULT_TIMEOUT, ®); if (ret) { dev_err(tee->dev, "tee: ring destroy command timed out\n"); - } else if (reg & PSP_CMDRESP_ERR_MASK) { - dev_err(tee->dev, "tee: ring destroy command failed (%#010x)\n", - reg & PSP_CMDRESP_ERR_MASK); + } else if (FIELD_GET(PSP_CMDRESP_STS, reg)) { + dev_err(tee->dev, "tee: ring destroy command failed (%#010lx)\n", + FIELD_GET(PSP_CMDRESP_STS, reg)); } free_ring: diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c index 80f28a1bbbef6..85d91cb6b9056 100644 --- a/drivers/i2c/busses/i2c-designware-amdpsp.c +++ b/drivers/i2c/busses/i2c-designware-amdpsp.c @@ -25,12 +25,6 @@ #define PSP_I2C_REQ_STS_BUS_BUSY 0x1 #define PSP_I2C_REQ_STS_INV_PARAM 0x3 -#define PSP_MBOX_FIELDS_STS GENMASK(15, 0) -#define PSP_MBOX_FIELDS_CMD GENMASK(23, 16) -#define PSP_MBOX_FIELDS_RESERVED GENMASK(29, 24) -#define PSP_MBOX_FIELDS_RECOVERY BIT(30) -#define PSP_MBOX_FIELDS_READY BIT(31) - struct psp_req_buffer_hdr { u32 total_size; u32 status; @@ -99,7 +93,7 @@ static int psp_check_mbox_recovery(struct psp_mbox __iomem *mbox) tmp = readl(&mbox->cmd_fields); - return FIELD_GET(PSP_MBOX_FIELDS_RECOVERY, tmp); + return FIELD_GET(PSP_CMDRESP_RECOVERY, tmp); } static int psp_wait_cmd(struct psp_mbox __iomem *mbox) @@ -107,7 +101,7 @@ static int psp_wait_cmd(struct psp_mbox __iomem *mbox) u32 tmp, expected; /* Expect mbox_cmd to be cleared and ready bit to be set by PSP */ - expected = FIELD_PREP(PSP_MBOX_FIELDS_READY, 1); + expected = FIELD_PREP(PSP_CMDRESP_RESP, 1); /* * Check for readiness of PSP mailbox in a tight loop in order to @@ -124,7 +118,7 @@ static u32 psp_check_mbox_sts(struct psp_mbox __iomem *mbox) cmd_reg = readl(&mbox->cmd_fields); - return FIELD_GET(PSP_MBOX_FIELDS_STS, cmd_reg); + return FIELD_GET(PSP_CMDRESP_STS, cmd_reg); } static int psp_send_cmd(struct psp_i2c_req *req) @@ -148,7 +142,7 @@ static int psp_send_cmd(struct psp_i2c_req *req) writeq(req_addr, &mbox->i2c_req_addr); /* Write command register to trigger processing */ - cmd_reg = FIELD_PREP(PSP_MBOX_FIELDS_CMD, PSP_I2C_REQ_BUS_CMD); + cmd_reg = FIELD_PREP(PSP_CMDRESP_CMD, PSP_I2C_REQ_BUS_CMD); writel(cmd_reg, &mbox->cmd_fields); if (psp_wait_cmd(mbox)) diff --git a/include/linux/psp.h b/include/linux/psp.h index 202162487ec3b..d3424790a70eb 100644 --- a/include/linux/psp.h +++ b/include/linux/psp.h @@ -11,4 +11,16 @@ #define __psp_pa(x) __pa(x) #endif +/* + * Fields and bits used by most PSP mailboxes + * + * Note: Some mailboxes (such as SEV) have extra bits or different meanings + * and should include an appropriate local definition in their source file. + */ +#define PSP_CMDRESP_STS GENMASK(15, 0) +#define PSP_CMDRESP_CMD GENMASK(23, 16) +#define PSP_CMDRESP_RESERVED GENMASK(29, 24) +#define PSP_CMDRESP_RECOVERY BIT(30) +#define PSP_CMDRESP_RESP BIT(31) + #endif /* __PSP_H */
Some of the bits and fields used for mailboxes communicating with the PSP are common across all mailbox implementations (SEV, TEE, etc). Move these bits into the common `linux/psp.h` so they don't need to be re-defined for each implementation. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/crypto/ccp/psp-dev.h | 3 --- drivers/crypto/ccp/sev-dev.c | 15 +++++++-------- drivers/crypto/ccp/sev-dev.h | 2 +- drivers/crypto/ccp/tee-dev.c | 15 ++++++++------- drivers/i2c/busses/i2c-designware-amdpsp.c | 14 ++++---------- include/linux/psp.h | 12 ++++++++++++ 6 files changed, 32 insertions(+), 29 deletions(-)