[PATCH v3 0/3] Add support for USBC PD FW download

[PATCH v3 0/3] Add support for USBC PD FW download

[Andrey Grodzovsky]

This patchset adds the kernel driver part of supporting USBC power delivery
firmware downloading to USBC chip on the ASIC. The FW is placed in DMA buffer
visible to PSP which then proceeds and copies the FW over I2C to the USBC chip.


Andrey Grodzovsky (3):
  drm/amdgpu: Add USBC PD FW load interface to PSP.
  drm/amdgpu: Add USBC PD FW load to PSP 11
  drm/amdgpu: Add support for USBC PD FW download

 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 110 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h |  10 +++
 drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h |   3 +
 drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  |  82 ++++++++++++++++++++++++
 4 files changed, 204 insertions(+), 1 deletion(-)

-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 1/3] drm/amdgpu: Add USBC PD FW load interface to PSP.

[Andrey Grodzovsky]

Used to load power Delivery FW to PSP.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
index 37fa184..297435c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
@@ -114,6 +114,8 @@ struct psp_funcs
 	int (*mem_training)(struct psp_context *psp, uint32_t ops);
 	uint32_t (*ring_get_wptr)(struct psp_context *psp);
 	void (*ring_set_wptr)(struct psp_context *psp, uint32_t value);
+	int (*load_usbc_pd_fw)(struct psp_context *psp, dma_addr_t dma_addr);
+	int (*read_usbc_pd_fw)(struct psp_context *psp, uint32_t *fw_ver);
 };
 
 #define AMDGPU_XGMI_MAX_CONNECTED_NODES		64
@@ -351,6 +353,14 @@ struct amdgpu_psp_funcs {
 #define psp_ring_get_wptr(psp) (psp)->funcs->ring_get_wptr((psp))
 #define psp_ring_set_wptr(psp, value) (psp)->funcs->ring_set_wptr((psp), (value))
 
+#define psp_load_usbc_pd_fw(psp, dma_addr) \
+	((psp)->funcs->load_usbc_pd_fw ? \
+	(psp)->funcs->load_usbc_pd_fw((psp), (dma_addr)) : -EINVAL)
+
+#define psp_read_usbc_pd_fw(psp, fw_ver) \
+	((psp)->funcs->read_usbc_pd_fw ? \
+	(psp)->funcs->read_usbc_pd_fw((psp), fw_ver) : -EINVAL)
+
 extern const struct amd_ip_funcs psp_ip_funcs;
 
 extern const struct amdgpu_ip_block_version psp_v3_1_ip_block;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Andrey Grodzovsky]

Add the programming sequence.

v2:
Change donwload wait loop to more efficient.
Move C2PMSG_CMD_GFX_USB_PD_FW_VER defintion

v3: Fix lack of loop counter increment typo

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h |  3 ++
 drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  | 82 +++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
index 36b6579..a44fd60 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
+++ b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
@@ -31,6 +31,9 @@
 #define GFX_CMD_RESERVED_MASK       0x7FF00000
 #define GFX_CMD_RESPONSE_MASK       0x80000000
 
+/* USBC PD FW version retrieval command */
+#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x2000000
+
 /* TEE Gfx Command IDs for the register interface.
 *  Command ID must be between 0x00010000 and 0x000F0000.
 */
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
index 8ab3bf3..3db1c0d 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
@@ -65,6 +65,9 @@ MODULE_FIRMWARE("amdgpu/arcturus_ta.bin");
 /* memory training timeout define */
 #define MEM_TRAIN_SEND_MSG_TIMEOUT_US	3000000
 
+/* For large FW files the time to complete can be very long */
+#define USBC_PD_POLLING_LIMIT_S 240
+
 static int psp_v11_0_init_microcode(struct psp_context *psp)
 {
 	struct amdgpu_device *adev = psp->adev;
@@ -1109,6 +1112,83 @@ static void psp_v11_0_ring_set_wptr(struct psp_context *psp, uint32_t value)
 		WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_67, value);
 }
 
+static int psp_v11_0_load_usbc_pd_fw(struct psp_context *psp, dma_addr_t dma_addr)
+{
+	struct amdgpu_device *adev = psp->adev;
+	uint32_t reg_status;
+	int ret, i = 0;
+
+	/* Write lower 32-bit address of the PD Controller FW */
+	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, lower_32_bits(dma_addr));
+	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+			     0x80000000, 0x80000000, false);
+	if (ret)
+		return ret;
+
+	/* Fireup interrupt so PSP can pick up the lower address */
+	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x800000);
+	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+			     0x80000000, 0x80000000, false);
+	if (ret)
+		return ret;
+
+	reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
+
+	if ((reg_status & 0xFFFF) != 0) {
+		DRM_ERROR("Lower address load failed - MP0_SMN_C2PMSG_35.Bits [15:0] = %02x...\n",
+				reg_status & 0xFFFF);
+		return -EIO;
+	}
+
+	/* Write upper 32-bit address of the PD Controller FW */
+	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, upper_32_bits(dma_addr));
+
+	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+			     0x80000000, 0x80000000, false);
+	if (ret)
+		return ret;
+
+	/* Fireup interrupt so PSP can pick up the upper address */
+	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x4000000);
+
+	/* FW load takes very long time */
+	do {
+		msleep(1000);
+		reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
+
+		if (reg_status & 0x80000000)
+			goto done;
+
+	} while(i++ < USBC_PD_POLLING_LIMIT_S);
+
+	return -ETIME;
+done:
+	reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
+
+	if ((reg_status & 0xFFFF) != 0) {
+		DRM_ERROR("Upper address load failed - MP0_SMN_C2PMSG_35.Bits [15:0] = x%04x\n",
+				reg_status & 0xFFFF);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int psp_v11_0_read_usbc_pd_fw(struct psp_context *psp, uint32_t *fw_ver)
+{
+	struct amdgpu_device *adev = psp->adev;
+	int ret;
+
+	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
+
+	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+				     0x80000000, 0x80000000, false);
+	if (!ret)
+		*fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
+
+	return ret;
+}
+
 static const struct psp_funcs psp_v11_0_funcs = {
 	.init_microcode = psp_v11_0_init_microcode,
 	.bootloader_load_kdb = psp_v11_0_bootloader_load_kdb,
@@ -1133,6 +1213,8 @@ static const struct psp_funcs psp_v11_0_funcs = {
 	.mem_training = psp_v11_0_memory_training,
 	.ring_get_wptr = psp_v11_0_ring_get_wptr,
 	.ring_set_wptr = psp_v11_0_ring_set_wptr,
+	.load_usbc_pd_fw = psp_v11_0_load_usbc_pd_fw,
+	.read_usbc_pd_fw = psp_v11_0_read_usbc_pd_fw
 };
 
 void psp_v11_0_set_psp_funcs(struct psp_context *psp)
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 3/3] drm/amdgpu: Add support for USBC PD FW download

[Andrey Grodzovsky]

Starts USBC PD FW download and reads back the latest FW version.

v2:
Move sysfs file creation to late init
Add locking around PSP calls to avoid concurrent access to PSP's C2P registers

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 110 +++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index d33f741..cff0fd2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -24,6 +24,7 @@
  */
 
 #include <linux/firmware.h>
+#include <linux/dma-mapping.h>
 
 #include "amdgpu.h"
 #include "amdgpu_psp.h"
@@ -38,6 +39,9 @@
 
 static void psp_set_funcs(struct amdgpu_device *adev);
 
+static int psp_sysfs_init(struct amdgpu_device *adev);
+static void psp_sysfs_fini(struct amdgpu_device *adev);
+
 /*
  * Due to DF Cstate management centralized to PMFW, the firmware
  * loading sequence will be updated as below:
@@ -113,6 +117,16 @@ static int psp_early_init(void *handle)
 	return 0;
 }
 
+static int psp_late_init(void *handle)
+{
+	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
+
+	if (adev->asic_type == CHIP_NAVI10)
+		return psp_sysfs_init(adev);
+
+	return 0;
+}
+
 static int psp_sw_init(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
@@ -152,6 +166,10 @@ static int psp_sw_fini(void *handle)
 		release_firmware(adev->psp.ta_fw);
 		adev->psp.ta_fw = NULL;
 	}
+
+	if (adev->asic_type == CHIP_NAVI10)
+		psp_sysfs_fini(adev);
+
 	return 0;
 }
 
@@ -1816,10 +1834,85 @@ static int psp_set_powergating_state(void *handle,
 	return 0;
 }
 
+static ssize_t psp_usbc_pd_fw_sysfs_read(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct drm_device *ddev = dev_get_drvdata(dev);
+	struct amdgpu_device *adev = ddev->dev_private;
+	uint32_t fw_ver;
+	int ret;
+
+	mutex_lock(&adev->psp.mutex);
+	ret = psp_read_usbc_pd_fw(&adev->psp, &fw_ver);
+	mutex_unlock(&adev->psp.mutex);
+
+	if (ret) {
+		DRM_ERROR("Failed to read USBC PD FW, err = %d", ret);
+		return ret;
+	}
+
+	return snprintf(buf, PAGE_SIZE, "%x\n", fw_ver);
+}
+
+static ssize_t psp_usbc_pd_fw_sysfs_write(struct device *dev,
+						       struct device_attribute *attr,
+						       const char *buf,
+						       size_t count)
+{
+	struct drm_device *ddev = dev_get_drvdata(dev);
+	struct amdgpu_device *adev = ddev->dev_private;
+	void *cpu_addr;
+	dma_addr_t dma_addr;
+	int ret;
+	char fw_name[100];
+	const struct firmware *usbc_pd_fw;
+
+
+	snprintf(fw_name, sizeof(fw_name), "amdgpu/%s", buf);
+	ret = request_firmware(&usbc_pd_fw, fw_name, adev->dev);
+	if (ret)
+		goto fail;
+
+	/* We need contiguous physical mem to place the FW  for psp to access */
+	cpu_addr = dma_alloc_coherent(adev->dev, usbc_pd_fw->size, &dma_addr, GFP_KERNEL);
+
+	ret = dma_mapping_error(adev->dev, dma_addr);
+	if (ret)
+		goto rel_buf;
+
+	memcpy_toio(cpu_addr, usbc_pd_fw->data, usbc_pd_fw->size);
+
+	/*TODO Remove once PSP starts snooping CPU cache */
+	clflush_cache_range(cpu_addr, (usbc_pd_fw->size & ~(L1_CACHE_BYTES - 1)));
+
+	mutex_lock(&adev->psp.mutex);
+	ret = psp_load_usbc_pd_fw(&adev->psp, dma_addr);
+	mutex_unlock(&adev->psp.mutex);
+
+rel_buf:
+	dma_free_coherent(adev->dev, usbc_pd_fw->size, cpu_addr, dma_addr);
+	release_firmware(usbc_pd_fw);
+
+fail:
+	if (ret) {
+		DRM_ERROR("Failed to load USBC PD FW, err = %d", ret);
+		return ret;
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR(usbc_pd_fw, S_IRUGO | S_IWUSR,
+		   psp_usbc_pd_fw_sysfs_read,
+		   psp_usbc_pd_fw_sysfs_write);
+
+
+
 const struct amd_ip_funcs psp_ip_funcs = {
 	.name = "psp",
 	.early_init = psp_early_init,
-	.late_init = NULL,
+	.late_init = psp_late_init,
 	.sw_init = psp_sw_init,
 	.sw_fini = psp_sw_fini,
 	.hw_init = psp_hw_init,
@@ -1834,6 +1927,21 @@ const struct amd_ip_funcs psp_ip_funcs = {
 	.set_powergating_state = psp_set_powergating_state,
 };
 
+static int psp_sysfs_init(struct amdgpu_device *adev)
+{
+	int ret = device_create_file(adev->dev, &dev_attr_usbc_pd_fw);
+
+	if (ret)
+		DRM_ERROR("Failed to create USBC PD FW control file!");
+
+	return ret;
+}
+
+static void psp_sysfs_fini(struct amdgpu_device *adev)
+{
+	device_remove_file(adev->dev, &dev_attr_usbc_pd_fw);
+}
+
 static const struct amdgpu_psp_funcs psp_funcs = {
 	.check_fw_loading_status = psp_check_fw_loading_status,
 };
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Luben Tuikov]

On 2020-03-03 17:02, Andrey Grodzovsky wrote:
> Add the programming sequence.
> 
> v2:
> Change donwload wait loop to more efficient.
> Move C2PMSG_CMD_GFX_USB_PD_FW_VER defintion
> 
> v3: Fix lack of loop counter increment typo
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h |  3 ++
>  drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  | 82 +++++++++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
> index 36b6579..a44fd60 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
> @@ -31,6 +31,9 @@
>  #define GFX_CMD_RESERVED_MASK       0x7FF00000
>  #define GFX_CMD_RESPONSE_MASK       0x80000000
>  
> +/* USBC PD FW version retrieval command */
> +#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x2000000
> +
>  /* TEE Gfx Command IDs for the register interface.
>  *  Command ID must be between 0x00010000 and 0x000F0000.
>  */
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> index 8ab3bf3..3db1c0d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> @@ -65,6 +65,9 @@ MODULE_FIRMWARE("amdgpu/arcturus_ta.bin");
>  /* memory training timeout define */
>  #define MEM_TRAIN_SEND_MSG_TIMEOUT_US	3000000
>  
> +/* For large FW files the time to complete can be very long */
> +#define USBC_PD_POLLING_LIMIT_S 240
> +
>  static int psp_v11_0_init_microcode(struct psp_context *psp)
>  {
>  	struct amdgpu_device *adev = psp->adev;
> @@ -1109,6 +1112,83 @@ static void psp_v11_0_ring_set_wptr(struct psp_context *psp, uint32_t value)
>  		WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_67, value);
>  }
>  
> +static int psp_v11_0_load_usbc_pd_fw(struct psp_context *psp, dma_addr_t dma_addr)
> +{
> +	struct amdgpu_device *adev = psp->adev;
> +	uint32_t reg_status;
> +	int ret, i = 0;
> +
> +	/* Write lower 32-bit address of the PD Controller FW */
> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, lower_32_bits(dma_addr));
> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +			     0x80000000, 0x80000000, false);
> +	if (ret)
> +		return ret;
> +
> +	/* Fireup interrupt so PSP can pick up the lower address */
> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x800000);
> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +			     0x80000000, 0x80000000, false);
> +	if (ret)
> +		return ret;
> +
> +	reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
> +
> +	if ((reg_status & 0xFFFF) != 0) {
> +		DRM_ERROR("Lower address load failed - MP0_SMN_C2PMSG_35.Bits [15:0] = %02x...\n",
> +				reg_status & 0xFFFF);
> +		return -EIO;
> +	}
> +
> +	/* Write upper 32-bit address of the PD Controller FW */
> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, upper_32_bits(dma_addr));
> +
> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +			     0x80000000, 0x80000000, false);
> +	if (ret)
> +		return ret;
> +
> +	/* Fireup interrupt so PSP can pick up the upper address */
> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x4000000);
> +
> +	/* FW load takes very long time */
> +	do {
> +		msleep(1000);
> +		reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
> +
> +		if (reg_status & 0x80000000)
> +			goto done;
> +
> +	} while(i++ < USBC_PD_POLLING_LIMIT_S);

1. The LKCS wants a space after a keyword ("while") and before parenthesis "(".

2. In do-while loops, you want to pre-increment, in order to account for the
   already executed iteration of the loop, since the check is done _after_
   the loop executes, in contrast to "for" and "while" loops. So you'll need
   to do this:

   } while (++i < USBC_PD_POLLING_LIMIT_S);

> +
> +	return -ETIME;
> +done:
> +	reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);

3. You had _just_ read the register, right before the goto jump.
   You do not need to read it again. (Else a race would exist,
   and you'd need to poll, _again_.) You shouldn't have to
   read it again.

> +
> +	if ((reg_status & 0xFFFF) != 0) {
> +		DRM_ERROR("Upper address load failed - MP0_SMN_C2PMSG_35.Bits [15:0] = x%04x\n",
> +				reg_status & 0xFFFF);
> +		return -EIO;
> +	}

With those fixed, this patch is,
Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>

Regards,
Luben



> +
> +	return 0;
> +}
> +
> +static int psp_v11_0_read_usbc_pd_fw(struct psp_context *psp, uint32_t *fw_ver)
> +{
> +	struct amdgpu_device *adev = psp->adev;
> +	int ret;
> +
> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
> +
> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +				     0x80000000, 0x80000000, false);
> +	if (!ret)
> +		*fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
> +
> +	return ret;
> +}
> +
>  static const struct psp_funcs psp_v11_0_funcs = {
>  	.init_microcode = psp_v11_0_init_microcode,
>  	.bootloader_load_kdb = psp_v11_0_bootloader_load_kdb,
> @@ -1133,6 +1213,8 @@ static const struct psp_funcs psp_v11_0_funcs = {
>  	.mem_training = psp_v11_0_memory_training,
>  	.ring_get_wptr = psp_v11_0_ring_get_wptr,
>  	.ring_set_wptr = psp_v11_0_ring_set_wptr,
> +	.load_usbc_pd_fw = psp_v11_0_load_usbc_pd_fw,
> +	.read_usbc_pd_fw = psp_v11_0_read_usbc_pd_fw
>  };
>  
>  void psp_v11_0_set_psp_funcs(struct psp_context *psp)
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Andrey Grodzovsky]

On 3/3/20 6:14 PM, Luben Tuikov wrote:
> On 2020-03-03 17:02, Andrey Grodzovsky wrote:
>> Add the programming sequence.
>>
>> v2:
>> Change donwload wait loop to more efficient.
>> Move C2PMSG_CMD_GFX_USB_PD_FW_VER defintion
>>
>> v3: Fix lack of loop counter increment typo
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h |  3 ++
>>   drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  | 82 +++++++++++++++++++++++++++++++++
>>   2 files changed, 85 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
>> index 36b6579..a44fd60 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
>> @@ -31,6 +31,9 @@
>>   #define GFX_CMD_RESERVED_MASK       0x7FF00000
>>   #define GFX_CMD_RESPONSE_MASK       0x80000000
>>   
>> +/* USBC PD FW version retrieval command */
>> +#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x2000000
>> +
>>   /* TEE Gfx Command IDs for the register interface.
>>   *  Command ID must be between 0x00010000 and 0x000F0000.
>>   */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
>> index 8ab3bf3..3db1c0d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
>> @@ -65,6 +65,9 @@ MODULE_FIRMWARE("amdgpu/arcturus_ta.bin");
>>   /* memory training timeout define */
>>   #define MEM_TRAIN_SEND_MSG_TIMEOUT_US	3000000
>>   
>> +/* For large FW files the time to complete can be very long */
>> +#define USBC_PD_POLLING_LIMIT_S 240
>> +
>>   static int psp_v11_0_init_microcode(struct psp_context *psp)
>>   {
>>   	struct amdgpu_device *adev = psp->adev;
>> @@ -1109,6 +1112,83 @@ static void psp_v11_0_ring_set_wptr(struct psp_context *psp, uint32_t value)
>>   		WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_67, value);
>>   }
>>   
>> +static int psp_v11_0_load_usbc_pd_fw(struct psp_context *psp, dma_addr_t dma_addr)
>> +{
>> +	struct amdgpu_device *adev = psp->adev;
>> +	uint32_t reg_status;
>> +	int ret, i = 0;
>> +
>> +	/* Write lower 32-bit address of the PD Controller FW */
>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, lower_32_bits(dma_addr));
>> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>> +			     0x80000000, 0x80000000, false);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Fireup interrupt so PSP can pick up the lower address */
>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x800000);
>> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>> +			     0x80000000, 0x80000000, false);
>> +	if (ret)
>> +		return ret;
>> +
>> +	reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
>> +
>> +	if ((reg_status & 0xFFFF) != 0) {
>> +		DRM_ERROR("Lower address load failed - MP0_SMN_C2PMSG_35.Bits [15:0] = %02x...\n",
>> +				reg_status & 0xFFFF);
>> +		return -EIO;
>> +	}
>> +
>> +	/* Write upper 32-bit address of the PD Controller FW */
>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, upper_32_bits(dma_addr));
>> +
>> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>> +			     0x80000000, 0x80000000, false);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Fireup interrupt so PSP can pick up the upper address */
>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x4000000);
>> +
>> +	/* FW load takes very long time */
>> +	do {
>> +		msleep(1000);
>> +		reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
>> +
>> +		if (reg_status & 0x80000000)
>> +			goto done;
>> +
>> +	} while(i++ < USBC_PD_POLLING_LIMIT_S);
> 1. The LKCS wants a space after a keyword ("while") and before parenthesis "(".
>
> 2. In do-while loops, you want to pre-increment, in order to account for the
>     already executed iteration of the loop, since the check is done _after_
>     the loop executes, in contrast to "for" and "while" loops. So you'll need
>     to do this:
>
>     } while (++i < USBC_PD_POLLING_LIMIT_S);
>
>> +
>> +	return -ETIME;
>> +done:
>> +	reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
> 3. You had _just_ read the register, right before the goto jump.
>     You do not need to read it again. (Else a race would exist,
>     and you'd need to poll, _again_.) You shouldn't have to
>     read it again.


Just went with AlexJ over the relevant PSP code - seems you are right, 
both the completion bit (31) and result status bits (bits 0-15) are set 
within a single statement and hence the last read inside the loop should 
be enough. Will fix,

Still, I don't see a race in the original code - just a superfluous 
register read.

Andrey


>
>> +
>> +	if ((reg_status & 0xFFFF) != 0) {
>> +		DRM_ERROR("Upper address load failed - MP0_SMN_C2PMSG_35.Bits [15:0] = x%04x\n",
>> +				reg_status & 0xFFFF);
>> +		return -EIO;
>> +	}
> With those fixed, this patch is,
> Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>
>
> Regards,
> Luben
>
>
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int psp_v11_0_read_usbc_pd_fw(struct psp_context *psp, uint32_t *fw_ver)
>> +{
>> +	struct amdgpu_device *adev = psp->adev;
>> +	int ret;
>> +
>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
>> +
>> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>> +				     0x80000000, 0x80000000, false);
>> +	if (!ret)
>> +		*fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
>> +
>> +	return ret;
>> +}
>> +
>>   static const struct psp_funcs psp_v11_0_funcs = {
>>   	.init_microcode = psp_v11_0_init_microcode,
>>   	.bootloader_load_kdb = psp_v11_0_bootloader_load_kdb,
>> @@ -1133,6 +1213,8 @@ static const struct psp_funcs psp_v11_0_funcs = {
>>   	.mem_training = psp_v11_0_memory_training,
>>   	.ring_get_wptr = psp_v11_0_ring_get_wptr,
>>   	.ring_set_wptr = psp_v11_0_ring_set_wptr,
>> +	.load_usbc_pd_fw = psp_v11_0_load_usbc_pd_fw,
>> +	.read_usbc_pd_fw = psp_v11_0_read_usbc_pd_fw
>>   };
>>   
>>   void psp_v11_0_set_psp_funcs(struct psp_context *psp)
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Luben Tuikov]

On 2020-03-04 10:56, Andrey Grodzovsky wrote:
> 
> On 3/3/20 6:14 PM, Luben Tuikov wrote:
>> On 2020-03-03 17:02, Andrey Grodzovsky wrote:
>>> Add the programming sequence.
>>>
>>> v2:
>>> Change donwload wait loop to more efficient.
>>> Move C2PMSG_CMD_GFX_USB_PD_FW_VER defintion
>>>
>>> v3: Fix lack of loop counter increment typo
>>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h |  3 ++
>>>   drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  | 82 +++++++++++++++++++++++++++++++++
>>>   2 files changed, 85 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
>>> index 36b6579..a44fd60 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
>>> @@ -31,6 +31,9 @@
>>>   #define GFX_CMD_RESERVED_MASK       0x7FF00000
>>>   #define GFX_CMD_RESPONSE_MASK       0x80000000
>>>   
>>> +/* USBC PD FW version retrieval command */
>>> +#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x2000000
>>> +
>>>   /* TEE Gfx Command IDs for the register interface.
>>>   *  Command ID must be between 0x00010000 and 0x000F0000.
>>>   */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
>>> index 8ab3bf3..3db1c0d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
>>> @@ -65,6 +65,9 @@ MODULE_FIRMWARE("amdgpu/arcturus_ta.bin");
>>>   /* memory training timeout define */
>>>   #define MEM_TRAIN_SEND_MSG_TIMEOUT_US	3000000
>>>   
>>> +/* For large FW files the time to complete can be very long */
>>> +#define USBC_PD_POLLING_LIMIT_S 240
>>> +
>>>   static int psp_v11_0_init_microcode(struct psp_context *psp)
>>>   {
>>>   	struct amdgpu_device *adev = psp->adev;
>>> @@ -1109,6 +1112,83 @@ static void psp_v11_0_ring_set_wptr(struct psp_context *psp, uint32_t value)
>>>   		WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_67, value);
>>>   }
>>>   
>>> +static int psp_v11_0_load_usbc_pd_fw(struct psp_context *psp, dma_addr_t dma_addr)
>>> +{
>>> +	struct amdgpu_device *adev = psp->adev;
>>> +	uint32_t reg_status;
>>> +	int ret, i = 0;
>>> +
>>> +	/* Write lower 32-bit address of the PD Controller FW */
>>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, lower_32_bits(dma_addr));
>>> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>>> +			     0x80000000, 0x80000000, false);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Fireup interrupt so PSP can pick up the lower address */
>>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x800000);
>>> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>>> +			     0x80000000, 0x80000000, false);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
>>> +
>>> +	if ((reg_status & 0xFFFF) != 0) {
>>> +		DRM_ERROR("Lower address load failed - MP0_SMN_C2PMSG_35.Bits [15:0] = %02x...\n",
>>> +				reg_status & 0xFFFF);
>>> +		return -EIO;
>>> +	}
>>> +
>>> +	/* Write upper 32-bit address of the PD Controller FW */
>>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, upper_32_bits(dma_addr));
>>> +
>>> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>>> +			     0x80000000, 0x80000000, false);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Fireup interrupt so PSP can pick up the upper address */
>>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x4000000);
>>> +
>>> +	/* FW load takes very long time */
>>> +	do {
>>> +		msleep(1000);
>>> +		reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
>>> +
>>> +		if (reg_status & 0x80000000)
>>> +			goto done;
>>> +
>>> +	} while(i++ < USBC_PD_POLLING_LIMIT_S);
>> 1. The LKCS wants a space after a keyword ("while") and before parenthesis "(".
>>
>> 2. In do-while loops, you want to pre-increment, in order to account for the
>>     already executed iteration of the loop, since the check is done _after_
>>     the loop executes, in contrast to "for" and "while" loops. So you'll need
>>     to do this:
>>
>>     } while (++i < USBC_PD_POLLING_LIMIT_S);
>>
>>> +
>>> +	return -ETIME;
>>> +done:
>>> +	reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
>> 3. You had _just_ read the register, right before the goto jump.
>>     You do not need to read it again. (Else a race would exist,
>>     and you'd need to poll, _again_.) You shouldn't have to
>>     read it again.
> 
> 
> Just went with AlexJ over the relevant PSP code - seems you are right, 
> both the completion bit (31) and result status bits (bits 0-15) are set 
> within a single statement and hence the last read inside the loop should 
> be enough. Will fix,
> 
> Still, I don't see a race in the original code - just a superfluous 
> register read.

There is no race in the original code. I said "would exist",
as I knew it doesn't. I knew bit 31 and the address are updated
in one go, else there would've been a race and you'd have to poll
again. But since you don't, they are updated in one go and no polling
needs to be and no race exist: therefore you don't need to read
the register again.

Thanks in advance for fixing those three issues. Let's see the fixed
version of the patch.

Regards,
Luben

> 
> Andrey
> 
> 
>>
>>> +
>>> +	if ((reg_status & 0xFFFF) != 0) {
>>> +		DRM_ERROR("Upper address load failed - MP0_SMN_C2PMSG_35.Bits [15:0] = x%04x\n",
>>> +				reg_status & 0xFFFF);
>>> +		return -EIO;
>>> +	}
>> With those fixed, this patch is,
>> Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>
>>
>> Regards,
>> Luben
>>
>>
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int psp_v11_0_read_usbc_pd_fw(struct psp_context *psp, uint32_t *fw_ver)
>>> +{
>>> +	struct amdgpu_device *adev = psp->adev;
>>> +	int ret;
>>> +
>>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
>>> +
>>> +	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>>> +				     0x80000000, 0x80000000, false);
>>> +	if (!ret)
>>> +		*fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>   static const struct psp_funcs psp_v11_0_funcs = {
>>>   	.init_microcode = psp_v11_0_init_microcode,
>>>   	.bootloader_load_kdb = psp_v11_0_bootloader_load_kdb,
>>> @@ -1133,6 +1213,8 @@ static const struct psp_funcs psp_v11_0_funcs = {
>>>   	.mem_training = psp_v11_0_memory_training,
>>>   	.ring_get_wptr = psp_v11_0_ring_get_wptr,
>>>   	.ring_set_wptr = psp_v11_0_ring_set_wptr,
>>> +	.load_usbc_pd_fw = psp_v11_0_load_usbc_pd_fw,
>>> +	.read_usbc_pd_fw = psp_v11_0_read_usbc_pd_fw
>>>   };
>>>   
>>>   void psp_v11_0_set_psp_funcs(struct psp_context *psp)
>>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx