diff mbox series

[3/6] crypto: ccp: Move some PSP mailbox bit definitions into common header

Message ID 20230209223811.4993-4-mario.limonciello@amd.com
State New
Headers
Series Export platform features from ccp driver |

Commit Message

Mario Limonciello Feb. 9, 2023, 10:38 p.m. UTC 
  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(-)

Comments

Jan Dąbroś Feb. 14, 2023, 9:04 a.m. UTC | #1 
(...)
> @@ -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.)
Mario Limonciello Feb. 14, 2023, 10:05 p.m. UTC | #2 
[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.
Tom Lendacky Feb. 16, 2023, 2:23 p.m. UTC | #3 
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 mbox series

Patch

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, &reg);
 	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 */