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

[PATCH 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 | 94 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 10 ++++
 drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  | 76 ++++++++++++++++++++++++++
 3 files changed, 180 insertions(+)

-- 
2.7.4

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

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

[Andrey Grodzovsky]

Used to load PD FW to PSP.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@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

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

[Alex Deucher]

On Mon, Mar 2, 2020 at 2:24 PM Andrey Grodzovsky
<andrey.grodzovsky@amd.com> wrote:
>
> Used to load PD FW to PSP.

Might want to define PD = Power Delivery.  With that fixed:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Andrey Grodzovsky]

Add the programming sequence.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
index 8ab3bf3..621b99c 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
 
+/* USBC PD FW version retrieval command */
+#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x2000000
+
 static int psp_v11_0_init_microcode(struct psp_context *psp)
 {
 	struct amdgpu_device *adev = psp->adev;
@@ -1109,6 +1112,77 @@ 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;
+
+	/* 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 long time */
+	while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+					     0x80000000, 0x80000000, false))
+		msleep(1000);
+
+	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] = %02x...\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;
+
+	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
+
+	while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+				     0x80000000, 0x80000000, false))
+		msleep(1);
+
+	*fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
+
+	return 0;
+}
+
 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 +1207,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

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

[Luben Tuikov]

On 2020-03-02 2:24 p.m., Andrey Grodzovsky wrote:
> Add the programming sequence.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 76 ++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> index 8ab3bf3..621b99c 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
>  
> +/* USBC PD FW version retrieval command */
> +#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x2000000
> +
>  static int psp_v11_0_init_microcode(struct psp_context *psp)
>  {
>  	struct amdgpu_device *adev = psp->adev;
> @@ -1109,6 +1112,77 @@ 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;
> +
> +	/* 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 long time */
> +	while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +					     0x80000000, 0x80000000, false))

1. The LKCS wants a space after a keyword, "while" and before the opening parenthesis.
   https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces

2. I'd rather include a polling limit in this loop, so that if the PSP goes haywire,
   we don't spend too long (forever) here. Also, I'd convert this loop into
   a do-while loop, and let the "while" check for the polling limit, while in the body
   of the loop, we receive the status from psp_wait_for() and decide whether
   to continue or "break".

> +		msleep(1000);

That's a rather large timeout given that "psp_wait_for()" polls every micro-second
for 100 microseconds.

And also note that you poll immediately after writing to MP0_SMN_C2PMSG_35, i.e.
you don't give the PSP any time to process the timeout. I'd rather do
something like this:

	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x4000000);
	usleep(100);  <-- Or some optimal timeout which gives the PSP FW ample time to react and process the interrupt.
	do {
		res = psp_wait_for(psp, ...);
		if (res == 0)
			break;
	while (++polling_limit < POLLING_LIMIT);

The advantage here is that if you hit the polling limit and
a subsequent read of the address is not what you want,
you know for sure that the PSP is stuck.

> +
> +	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] = %02x...\n",
> +				reg_status & 0xFFFF);

Perhaps you want to say "%04X" so the field width is always 4 hexadecimal chars.

> +		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;
> +
> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
> +
> +	while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +				     0x80000000, 0x80000000, false))

Space after a keyword ("while") and before "(".

Same sentiment as above:
After writing the interrupt, wait some nominal time for the PSP to react,
then psp_wait_for() in a do-while loop also with a polling limit.

Regards,
Luben

> +		msleep(1);
> +
> +	*fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
> +
> +	return 0;
> +}
> +
>  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 +1207,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 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Andrey Grodzovsky]

On 3/2/20 4:19 PM, Luben Tuikov wrote:
> On 2020-03-02 2:24 p.m., Andrey Grodzovsky wrote:
>> Add the programming sequence.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 76 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 76 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
>> index 8ab3bf3..621b99c 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
>>   
>> +/* USBC PD FW version retrieval command */
>> +#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x2000000
>> +
>>   static int psp_v11_0_init_microcode(struct psp_context *psp)
>>   {
>>   	struct amdgpu_device *adev = psp->adev;
>> @@ -1109,6 +1112,77 @@ 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;
>> +
>> +	/* 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 long time */
>> +	while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>> +					     0x80000000, 0x80000000, false))
> 1. The LKCS wants a space after a keyword, "while" and before the opening parenthesis.
>     https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces
>
> 2. I'd rather include a polling limit in this loop, so that if the PSP goes haywire,
>     we don't spend too long (forever) here. Also, I'd convert this loop into
>     a do-while loop, and let the "while" check for the polling limit, while in the body
>     of the loop, we receive the status from psp_wait_for() and decide whether
>     to continue or "break".
>
>> +		msleep(1000);
> That's a rather large timeout given that "psp_wait_for()" polls every micro-second
> for 100 microseconds.


The download from PSP to USBC chip over I2C takes between 20s - 40s 
depending on the PD FW file size to download, only at the end of this 
time interval will the PSP respond by setting mmMP0_SMN_C2PMSG_35 to 
0x80000000, psp_wait_for is using udelay which is busy wait and so to 
avoid blocking the CPU for to much time during this long 20-40s wait i 
prefer the 1s interval between each busy wait of psp_wait_for


>
> And also note that you poll immediately after writing to MP0_SMN_C2PMSG_35, i.e.
> you don't give the PSP any time to process the timeout.


What do you mean here - why I need to give PSP any time here before 
starting the wait loop ?


> I'd rather do
> something like this:
>
> 	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x4000000);
> 	usleep(100);  <-- Or some optimal timeout which gives the PSP FW ample time to react and process the interrupt.


React to what ? The next time PSP will react will be when the I2C 
download finish or an error will happen and this will be caught during 
the next cycle of psp_wait_for

Andrey


> 	do {
> 		res = psp_wait_for(psp, ...);
> 		if (res == 0)
> 			break;
> 	while (++polling_limit < POLLING_LIMIT);
>
> The advantage here is that if you hit the polling limit and
> a subsequent read of the address is not what you want,
> you know for sure that the PSP is stuck.
>
>> +
>> +	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] = %02x...\n",
>> +				reg_status & 0xFFFF);
> Perhaps you want to say "%04X" so the field width is always 4 hexadecimal chars.
>
>> +		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;
>> +
>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
>> +
>> +	while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>> +				     0x80000000, 0x80000000, false))
> Space after a keyword ("while") and before "(".
>
> Same sentiment as above:
> After writing the interrupt, wait some nominal time for the PSP to react,
> then psp_wait_for() in a do-while loop also with a polling limit.
>
> Regards,
> Luben
>
>> +		msleep(1);
>> +
>> +	*fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
>> +
>> +	return 0;
>> +}
>> +
>>   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 +1207,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 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Luben Tuikov]

On 2020-03-02 4:51 p.m., Andrey Grodzovsky wrote:
> 
> On 3/2/20 4:19 PM, Luben Tuikov wrote:
>> On 2020-03-02 2:24 p.m., Andrey Grodzovsky wrote:
>>> Add the programming sequence.
>>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 76 ++++++++++++++++++++++++++++++++++
>>>   1 file changed, 76 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
>>> index 8ab3bf3..621b99c 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
>>>   
>>> +/* USBC PD FW version retrieval command */
>>> +#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x2000000
>>> +
>>>   static int psp_v11_0_init_microcode(struct psp_context *psp)
>>>   {
>>>   	struct amdgpu_device *adev = psp->adev;
>>> @@ -1109,6 +1112,77 @@ 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;
>>> +
>>> +	/* 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 long time */
>>> +	while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>>> +					     0x80000000, 0x80000000, false))
>> 1. The LKCS wants a space after a keyword, "while" and before the opening parenthesis.
>>     https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces
>>
>> 2. I'd rather include a polling limit in this loop, so that if the PSP goes haywire,
>>     we don't spend too long (forever) here. Also, I'd convert this loop into
>>     a do-while loop, and let the "while" check for the polling limit, while in the body
>>     of the loop, we receive the status from psp_wait_for() and decide whether
>>     to continue or "break".
>>
>>> +		msleep(1000);
>> That's a rather large timeout given that "psp_wait_for()" polls every micro-second
>> for 100 microseconds.
> 
> 
> The download from PSP to USBC chip over I2C takes between 20s - 40s 
> depending on the PD FW file size to download, only at the end of this 
> time interval will the PSP respond by setting mmMP0_SMN_C2PMSG_35 to 
> 0x80000000, psp_wait_for is using udelay which is busy wait and so to 
> avoid blocking the CPU for to much time during this long 20-40s wait i 
> prefer the 1s interval between each busy wait of psp_wait_for
> 
> 
>>
>> And also note that you poll immediately after writing to MP0_SMN_C2PMSG_35, i.e.
>> you don't give the PSP any time to process the timeout.
> 
> 
> What do you mean here - why I need to give PSP any time here before 
> starting the wait loop ?

Because nothing would've been done in 0 or near-0 time. Effectively
you do WREG() immediately followed by a busy loop of RREG() (from psp_wait_for())
and there is no point in that busy loop.

> 
> 
>> I'd rather do
>> something like this:
>>
>> 	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x4000000);
>> 	usleep(100);  <-- Or some optimal timeout which gives the PSP FW ample time to react and process the interrupt.
> 
> 
> React to what ? The next time PSP will react will be when the I2C 
> download finish or an error will happen and this will be caught during 
> the next cycle of psp_wait_for

Since you're waiting 1 second (msleep(1000)), it'd be better
to wait 1 second immediately after telling the PSP to load the firmware,
and after that point in time, start polling for status.

I'd also suggest not using psp_wait_for() as it does a busy loop for
100,000 polls waiting 1 us in between--not very interactive-OS-friendly.

I'd suggest the following code:

	...

	/* Fire up the interrupt so that PSP can pick up
	 * the upper address.
	 */
	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x4000000);
A:	msleep(500);
	for (poll_count = 0; poll_count < USBC_PD_POLLING_LIMIT; poll_count++) {
B:		msleep(500);
		reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
		if ((reg_status & 0x80000000) == 0x80000000)
			break;
	}

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

	return 0;
}

This way you wait 1 second after telling the PSP to download
the firmware and before you poll the first time, but 500 ms
polling for status every polling loop, after that.

Note that you can eliminate timeout A and add it to
timeout B, if you don't want to have different timeout
times for work-being-done and for polling-status. Or,
you can increase A and decrease B.

Note also that the above avoids,
1. the busy loop in psp_wait_for(), and
2. an extraneous read after the loop to check for status.

Regards,
Luben

> 
> Andrey
> 
> 
>> 	do {
>> 		res = psp_wait_for(psp, ...);
>> 		if (res == 0)
>> 			break;
>> 	while (++polling_limit < POLLING_LIMIT);
>>
>> The advantage here is that if you hit the polling limit and
>> a subsequent read of the address is not what you want,
>> you know for sure that the PSP is stuck.
>>
>>> +
>>> +	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] = %02x...\n",
>>> +				reg_status & 0xFFFF);
>> Perhaps you want to say "%04X" so the field width is always 4 hexadecimal chars.
>>
>>> +		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;
>>> +
>>> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
>>> +
>>> +	while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>>> +				     0x80000000, 0x80000000, false))
>> Space after a keyword ("while") and before "(".
>>
>> Same sentiment as above:
>> After writing the interrupt, wait some nominal time for the PSP to react,
>> then psp_wait_for() in a do-while loop also with a polling limit.
>>
>> Regards,
>> Luben
>>
>>> +		msleep(1);
>>> +
>>> +	*fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   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 +1207,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 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Luben Tuikov]

On 2020-03-02 2:24 p.m., Andrey Grodzovsky wrote:
> Add the programming sequence.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 76 ++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> index 8ab3bf3..621b99c 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
>  
> +/* USBC PD FW version retrieval command */
> +#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x2000000
> +
>  static int psp_v11_0_init_microcode(struct psp_context *psp)
>  {
>  	struct amdgpu_device *adev = psp->adev;
> @@ -1109,6 +1112,77 @@ 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;
> +
> +	/* 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

C++ comment?

> +	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

C++ comment?

> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x4000000);
> +
> +	/* FW load takes long time */
> +	while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +					     0x80000000, 0x80000000, false))
> +		msleep(1000);
> +
> +	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] = %02x...\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;
> +
> +	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
> +
> +	while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +				     0x80000000, 0x80000000, false))
> +		msleep(1);
> +
> +	*fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
> +
> +	return 0;
> +}
> +
>  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 +1207,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 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Alex Deucher]

On Mon, Mar 2, 2020 at 2:24 PM Andrey Grodzovsky
<andrey.grodzovsky@amd.com> wrote:
>
> Add the programming sequence.
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 76 ++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> index 8ab3bf3..621b99c 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
>
> +/* USBC PD FW version retrieval command */
> +#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x2000000
> +

This probably belongs in psp_gfx_if.h.

>  static int psp_v11_0_init_microcode(struct psp_context *psp)
>  {
>         struct amdgpu_device *adev = psp->adev;
> @@ -1109,6 +1112,77 @@ 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;
> +
> +       /* 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

C style comments please.

> +       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

C style comments.

> +       WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x4000000);
> +
> +       /* FW load takes long time */
> +       while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +                                            0x80000000, 0x80000000, false))
> +               msleep(1000);
> +

Are we actually waiting for the full loading here or just the ack of
the command?

> +       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] = %02x...\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;
> +
> +       WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
> +
> +       while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +                                    0x80000000, 0x80000000, false))
> +               msleep(1);
> +
> +       *fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
> +
> +       return 0;
> +}

I think we need locking for mmMP0_SMN_C2PMSG_35/mmMP0_SMN_C2PMSG_36
since they are the mailbox registers for communicating with the PSP.
Maybe just a generic psp lock since I don't think we need fine grained
locking for psp.

Alex

> +
>  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 +1207,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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Andrey Grodzovsky]

On 3/2/20 4:30 PM, Alex Deucher wrote:
> On Mon, Mar 2, 2020 at 2:24 PM Andrey Grodzovsky
> <andrey.grodzovsky@amd.com> wrote:
>> Add the programming sequence.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 76 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 76 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
>> index 8ab3bf3..621b99c 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
>>
>> +/* USBC PD FW version retrieval command */
>> +#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x2000000
>> +
> This probably belongs in psp_gfx_if.h.
>
>>   static int psp_v11_0_init_microcode(struct psp_context *psp)
>>   {
>>          struct amdgpu_device *adev = psp->adev;
>> @@ -1109,6 +1112,77 @@ 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;
>> +
>> +       /* 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
> C style comments please.
>
>> +       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
> C style comments.
>
>> +       WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x4000000);
>> +
>> +       /* FW load takes long time */
>> +       while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>> +                                            0x80000000, 0x80000000, false))
>> +               msleep(1000);
>> +
> Are we actually waiting for the full loading here or just the ack of
> the command?
>
>> +       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] = %02x...\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;
>> +
>> +       WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
>> +
>> +       while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
>> +                                    0x80000000, 0x80000000, false))
>> +               msleep(1);
>> +
>> +       *fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
>> +
>> +       return 0;
>> +}
> I think we need locking for mmMP0_SMN_C2PMSG_35/mmMP0_SMN_C2PMSG_36
> since they are the mailbox registers for communicating with the PSP.
> Maybe just a generic psp lock since I don't think we need fine grained
> locking for psp.
>
> Alex


I wonder why we don't lock in any other place we access them (e.g. 
psp_v11_0_memory_training_send_msg or psp_v11_0_bootloader_load_sos) ?

Andrey

>
>> +
>>   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 +1207,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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cf3c9cee87052401e07f508d7bef10174%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637187814630848721&amp;sdata=Mo9%2BR4Nrh0rPxZ7xJrBzHl71M6rWL7KcrgEWOne6yQE%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 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.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index d33f741..76630b9 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:
@@ -94,6 +98,7 @@ static int psp_early_init(void *handle)
 		psp->autoload_supported = false;
 		break;
 	case CHIP_NAVI10:
+		psp_sysfs_init(adev);
 	case CHIP_NAVI14:
 	case CHIP_NAVI12:
 		psp_v11_0_set_psp_funcs(psp);
@@ -152,6 +157,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,6 +1825,76 @@ 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;
+
+	ret = psp_read_usbc_pd_fw(&adev->psp, &fw_ver);
+	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 = 0;
+	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)));
+
+	ret = psp_load_usbc_pd_fw(&adev->psp, dma_addr);
+
+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,
@@ -1834,6 +1913,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 3/3] drm/amdgpu: Add support for USBC PD FW download

[Alex Deucher]

On Mon, Mar 2, 2020 at 2:24 PM Andrey Grodzovsky
<andrey.grodzovsky@amd.com> wrote:
>
> Starts USBC PD FW download and reads back the latest FW version.
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 94 +++++++++++++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index d33f741..76630b9 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:
> @@ -94,6 +98,7 @@ static int psp_early_init(void *handle)
>                 psp->autoload_supported = false;
>                 break;
>         case CHIP_NAVI10:
> +               psp_sysfs_init(adev);

We should add the file as late as possible.  Maybe add a new
psp_late_init() function in amdgpu_psp.c?  Otherwise, you'll get
processes trying to use the sysfs interface before the driver is
ready.

Alex

>         case CHIP_NAVI14:
>         case CHIP_NAVI12:
>                 psp_v11_0_set_psp_funcs(psp);
> @@ -152,6 +157,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,6 +1825,76 @@ 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;
> +
> +       ret = psp_read_usbc_pd_fw(&adev->psp, &fw_ver);
> +       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 = 0;
> +       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)));
> +
> +       ret = psp_load_usbc_pd_fw(&adev->psp, dma_addr);
> +
> +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,
> @@ -1834,6 +1913,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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Luben Tuikov]

On 2020-03-02 2:24 p.m., Andrey Grodzovsky wrote:
> Starts USBC PD FW download and reads back the latest FW version.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 94 +++++++++++++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index d33f741..76630b9 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:
> @@ -94,6 +98,7 @@ static int psp_early_init(void *handle)
>  		psp->autoload_supported = false;
>  		break;
>  	case CHIP_NAVI10:
> +		psp_sysfs_init(adev);
>  	case CHIP_NAVI14:
>  	case CHIP_NAVI12:
>  		psp_v11_0_set_psp_funcs(psp);
> @@ -152,6 +157,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,6 +1825,76 @@ 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)

Alignment of the parameters is off here. After applying your patch, the alignment
looks like the above.

> +{
> +	struct drm_device *ddev = dev_get_drvdata(dev);
> +	struct amdgpu_device *adev = ddev->dev_private;
> +	uint32_t fw_ver;
> +	int ret;
> +
> +	ret = psp_read_usbc_pd_fw(&adev->psp, &fw_ver);
> +	if (ret) {
> +		DRM_ERROR("Failed to read USBC PD FW, err = %d", ret);
> +		return ret;
> +	}
> +
> +	return snprintf(buf, PAGE_SIZE, "%x\n", fw_ver);

I'd rather print the firmware version in some fixed format,
like say "%08X" (in upper hex digits), since it is not an integer, but a fixed
string (8) of hexadecimal numbers.

It'll make it easy to copy-and-paste and the visual cue is there
when displayed to a screen. Even more so when visually compared,
say in a report of what was being tested and so on.

> +}
> +
> +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 = 0;

No need to set "ret" to anything here. After all, you cannot predict
that "all will be well". Let the compiler prove that
it is never being used without being set.

> +	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;

"rel_buf" could also be the identifier of a string or a buffer.
But if you named it "Out_release_buf", then no one would ever confuse
it for a name of a variable (all lower case) or a macro (all upper-case).
Not a blocker, just style.

> +
> +	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)));

Not a blocker, but it is easier to read a commment like this:

	/* TODO: Remove once PSP starts snooping the cache.
	 */
	cflush_cache_range(cpu_addr, ...);


> +
> +	ret = psp_load_usbc_pd_fw(&adev->psp, dma_addr);
> +
> +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,
> @@ -1834,6 +1913,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!");

There's an extra space after the tab char in the above two lines--perhaps
from a cut-and-paste.

Regards,
Luben

> +
> +	 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,
>  };
> 

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