[PATCH 0/3] *** Add PSP ECC support for Vega10 ***

[PATCH 0/3] *** Add PSP ECC support for Vega10 ***

[Junwei Zhang]

Junwei Zhang (3):
  drm/amdgpu: add psp bootloader command list
  drm/amdgpu: add psp ecc support
  drm/amdgpu: add psp ecc support for vega10

 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 20 +++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 32 +++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/psp_v3_1.c   | 64 +++++++++++++++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/psp_v3_1.h   |  2 ++
 4 files changed, 113 insertions(+), 5 deletions(-)

-- 
1.9.1

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

[PATCH 1/3] drm/amdgpu: add psp bootloader command list

[Junwei Zhang]

Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 10 ++++++++++
 drivers/gpu/drm/amd/amdgpu/psp_v3_1.c   |  4 ++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
index 538fa9d..3776186 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
@@ -53,6 +53,16 @@ struct psp_ring
 	uint32_t			ring_size;
 };
 
+enum psp_bootloader_command_list
+{
+	PSP_BL__LOAD_SYSDRV = 0x10000,
+	PSP_BL__LOAD_SOSDRV = 0x20000,
+	PSP_BL__NO_ECC      = 0x30000,
+	PSP_BL__PARTIAL_ECC = 0x30001,
+	PSP_BL__FULL_ECC    = 0x30002,
+	PSP_BL__DEFAULT_ECC = 0x30003,
+};
+
 struct psp_context
 {
 	struct amdgpu_device            *adev;
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
index 2718e86..f93a66e 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
@@ -190,7 +190,7 @@ int psp_v3_1_bootloader_load_sysdrv(struct psp_context *psp)
 	/* Provide the sys driver to bootrom */
 	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36,
 	       (uint32_t)(psp->fw_pri_mc_addr >> 20));
-	psp_gfxdrv_command_reg = 1 << 16;
+	psp_gfxdrv_command_reg = PSP_BL__LOAD_SYSDRV;
 	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35,
 	       psp_gfxdrv_command_reg);
 
@@ -231,7 +231,7 @@ int psp_v3_1_bootloader_load_sos(struct psp_context *psp)
 	/* Provide the PSP secure OS to bootrom */
 	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36,
 	       (uint32_t)(psp->fw_pri_mc_addr >> 20));
-	psp_gfxdrv_command_reg = 2 << 16;
+	psp_gfxdrv_command_reg = PSP_BL__LOAD_SOSDRV;
 	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35,
 	       psp_gfxdrv_command_reg);
 
-- 
1.9.1

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

Re: [PATCH 1/3] drm/amdgpu: add psp bootloader command list

[Huang Rui]

On Fri, Jul 28, 2017 at 05:11:17PM +0800, Junwei Zhang wrote:
> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
> ---

Reviewed-by: Huang Rui <ray.huang@amd.com>

>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 10 ++++++++++
>  drivers/gpu/drm/amd/amdgpu/psp_v3_1.c   |  4 ++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> index 538fa9d..3776186 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> @@ -53,6 +53,16 @@ struct psp_ring
>  	uint32_t			ring_size;
>  };
>  
> +enum psp_bootloader_command_list
> +{
> +	PSP_BL__LOAD_SYSDRV = 0x10000,
> +	PSP_BL__LOAD_SOSDRV = 0x20000,
> +	PSP_BL__NO_ECC      = 0x30000,
> +	PSP_BL__PARTIAL_ECC = 0x30001,
> +	PSP_BL__FULL_ECC    = 0x30002,
> +	PSP_BL__DEFAULT_ECC = 0x30003,
> +};
> +
>  struct psp_context
>  {
>  	struct amdgpu_device            *adev;
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> index 2718e86..f93a66e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> @@ -190,7 +190,7 @@ int psp_v3_1_bootloader_load_sysdrv(struct psp_context *psp)
>  	/* Provide the sys driver to bootrom */
>  	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36,
>  	       (uint32_t)(psp->fw_pri_mc_addr >> 20));
> -	psp_gfxdrv_command_reg = 1 << 16;
> +	psp_gfxdrv_command_reg = PSP_BL__LOAD_SYSDRV;
>  	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35,
>  	       psp_gfxdrv_command_reg);
>  
> @@ -231,7 +231,7 @@ int psp_v3_1_bootloader_load_sos(struct psp_context *psp)
>  	/* Provide the PSP secure OS to bootrom */
>  	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36,
>  	       (uint32_t)(psp->fw_pri_mc_addr >> 20));
> -	psp_gfxdrv_command_reg = 2 << 16;
> +	psp_gfxdrv_command_reg = PSP_BL__LOAD_SOSDRV;
>  	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35,
>  	       psp_gfxdrv_command_reg);
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> 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 psp ecc support

[Junwei Zhang]

Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 17 ++++++++++++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 22 ++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 1aa41af..b04cc80 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -256,6 +256,10 @@ static int psp_hw_start(struct psp_context *psp)
 {
 	int ret;
 
+	ret = psp_bootloader_set_ecc_mode(psp);
+	if (ret)
+		return ret;
+
 	ret = psp_bootloader_load_sysdrv(psp);
 	if (ret)
 		return ret;
@@ -365,9 +369,16 @@ static int psp_load_fw(struct amdgpu_device *adev)
 	if (ret)
 		goto failed_mem;
 
-	ret = psp_hw_start(psp);
-	if (ret)
-		goto failed_mem;
+	if (psp_bootloader_is_sos_running(psp) &&
+			psp->config.ecc_mode != PSP_ECC_MODE__NONE) {
+		if (psp_ring_create(psp, PSP_RING_TYPE__KM))
+			goto failed_mem;
+		if (psp_tmr_load(psp))
+			goto failed_mem;
+	} else {
+		if (psp_hw_start(psp))
+			goto failed_mem;
+	}
 
 	ret = psp_np_fw_load(psp);
 	if (ret)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
index 3776186..8ec9194 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
@@ -63,6 +63,19 @@ enum psp_bootloader_command_list
 	PSP_BL__DEFAULT_ECC = 0x30003,
 };
 
+enum psp_ecc_mode
+{
+	PSP_ECC_MODE__NONE = 0,
+	PSP_ECC_MODE__OFF = 1,
+	PSP_ECC_MODE__ON = 2,
+	PSP_ECC_MODE__PARTIALON = 3,
+};
+
+struct psp_config
+{
+	enum psp_ecc_mode		ecc_mode;
+};
+
 struct psp_context
 {
 	struct amdgpu_device            *adev;
@@ -70,6 +83,8 @@ struct psp_context
 	struct psp_gfx_cmd_resp		*cmd;
 
 	int (*init_microcode)(struct psp_context *psp);
+	int (*bootloader_set_ecc_mode)(struct psp_context *psp);
+	bool (*bootloader_is_sos_running)(struct psp_context *psp);
 	int (*bootloader_load_sysdrv)(struct psp_context *psp);
 	int (*bootloader_load_sos)(struct psp_context *psp);
 	int (*prep_cmd_buf)(struct amdgpu_firmware_info *ucode,
@@ -123,6 +138,9 @@ struct psp_context
 	struct amdgpu_bo		*cmd_buf_bo;
 	uint64_t			cmd_buf_mc_addr;
 	struct psp_gfx_cmd_resp		*cmd_buf_mem;
+
+	/* psp config */
+	struct psp_config		config;
 };
 
 struct amdgpu_psp_funcs {
@@ -140,6 +158,10 @@ struct amdgpu_psp_funcs {
 		(psp)->compare_sram_data((psp), (ucode), (type))
 #define psp_init_microcode(psp) \
 		((psp)->init_microcode ? (psp)->init_microcode((psp)) : 0)
+#define psp_bootloader_set_ecc_mode(psp) \
+		((psp)->bootloader_set_ecc_mode ? (psp)->bootloader_set_ecc_mode((psp)) : 0)
+#define psp_bootloader_is_sos_running(psp) \
+		((psp)->bootloader_is_sos_running ? (psp)->bootloader_is_sos_running((psp)) : 0)
 #define psp_bootloader_load_sysdrv(psp) \
 		((psp)->bootloader_load_sysdrv ? (psp)->bootloader_load_sysdrv((psp)) : 0)
 #define psp_bootloader_load_sos(psp) \
-- 
1.9.1

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

Re: [PATCH 2/3] drm/amdgpu: add psp ecc support

[Huang Rui]

On Fri, Jul 28, 2017 at 05:11:18PM +0800, Junwei Zhang wrote:
> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 17 ++++++++++++++---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 22 ++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index 1aa41af..b04cc80 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -256,6 +256,10 @@ static int psp_hw_start(struct psp_context *psp)
>  {
>  	int ret;
>  
> +	ret = psp_bootloader_set_ecc_mode(psp);
> +	if (ret)
> +		return ret;
> +
>  	ret = psp_bootloader_load_sysdrv(psp);
>  	if (ret)
>  		return ret;
> @@ -365,9 +369,16 @@ static int psp_load_fw(struct amdgpu_device *adev)
>  	if (ret)
>  		goto failed_mem;
>  
> -	ret = psp_hw_start(psp);
> -	if (ret)
> -		goto failed_mem;
> +	if (psp_bootloader_is_sos_running(psp) &&
> +			psp->config.ecc_mode != PSP_ECC_MODE__NONE) {

It need set a default value to config psp->ecc_mode, otherwise, it is
always "0" in this implementation.

> +		if (psp_ring_create(psp, PSP_RING_TYPE__KM))
> +			goto failed_mem;
> +		if (psp_tmr_load(psp))
> +			goto failed_mem;
> +	} else {
> +		if (psp_hw_start(psp))
> +			goto failed_mem;
> +	}
>  
>  	ret = psp_np_fw_load(psp);
>  	if (ret)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> index 3776186..8ec9194 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> @@ -63,6 +63,19 @@ enum psp_bootloader_command_list
>  	PSP_BL__DEFAULT_ECC = 0x30003,
>  };
>  
> +enum psp_ecc_mode
> +{
> +	PSP_ECC_MODE__NONE = 0,
> +	PSP_ECC_MODE__OFF = 1,
> +	PSP_ECC_MODE__ON = 2,
> +	PSP_ECC_MODE__PARTIALON = 3,
> +};
> +
> +struct psp_config
> +{
> +	enum psp_ecc_mode		ecc_mode;
> +};
> +
>  struct psp_context
>  {
>  	struct amdgpu_device            *adev;
> @@ -70,6 +83,8 @@ struct psp_context
>  	struct psp_gfx_cmd_resp		*cmd;
>  
>  	int (*init_microcode)(struct psp_context *psp);
> +	int (*bootloader_set_ecc_mode)(struct psp_context *psp);
> +	bool (*bootloader_is_sos_running)(struct psp_context *psp);
>  	int (*bootloader_load_sysdrv)(struct psp_context *psp);
>  	int (*bootloader_load_sos)(struct psp_context *psp);
>  	int (*prep_cmd_buf)(struct amdgpu_firmware_info *ucode,
> @@ -123,6 +138,9 @@ struct psp_context
>  	struct amdgpu_bo		*cmd_buf_bo;
>  	uint64_t			cmd_buf_mc_addr;
>  	struct psp_gfx_cmd_resp		*cmd_buf_mem;
> +
> +	/* psp config */
> +	struct psp_config		config;

At current, we don't need a psp_config wrapper here. Use "enum ecc_mode"
directly to make code more simple.

>  };
>  
>  struct amdgpu_psp_funcs {
> @@ -140,6 +158,10 @@ struct amdgpu_psp_funcs {
>  		(psp)->compare_sram_data((psp), (ucode), (type))
>  #define psp_init_microcode(psp) \
>  		((psp)->init_microcode ? (psp)->init_microcode((psp)) : 0)
> +#define psp_bootloader_set_ecc_mode(psp) \
> +		((psp)->bootloader_set_ecc_mode ? (psp)->bootloader_set_ecc_mode((psp)) : 0)
> +#define psp_bootloader_is_sos_running(psp) \
> +		((psp)->bootloader_is_sos_running ? (psp)->bootloader_is_sos_running((psp)) : 0)
>  #define psp_bootloader_load_sysdrv(psp) \
>  		((psp)->bootloader_load_sysdrv ? (psp)->bootloader_load_sysdrv((psp)) : 0)
>  #define psp_bootloader_load_sos(psp) \
> -- 
> 1.9.1
> 
> _______________________________________________
> 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 psp ecc support

[Zhang, Jerry (Junwei)]

On 07/31/2017 01:57 PM, Huang Rui wrote:
> On Fri, Jul 28, 2017 at 05:11:18PM +0800, Junwei Zhang wrote:
>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 17 ++++++++++++++---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 22 ++++++++++++++++++++++
>>   2 files changed, 36 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>> index 1aa41af..b04cc80 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>> @@ -256,6 +256,10 @@ static int psp_hw_start(struct psp_context *psp)
>>   {
>>   	int ret;
>>
>> +	ret = psp_bootloader_set_ecc_mode(psp);
>> +	if (ret)
>> +		return ret;
>> +
>>   	ret = psp_bootloader_load_sysdrv(psp);
>>   	if (ret)
>>   		return ret;
>> @@ -365,9 +369,16 @@ static int psp_load_fw(struct amdgpu_device *adev)
>>   	if (ret)
>>   		goto failed_mem;
>>
>> -	ret = psp_hw_start(psp);
>> -	if (ret)
>> -		goto failed_mem;
>> +	if (psp_bootloader_is_sos_running(psp) &&
>> +			psp->config.ecc_mode != PSP_ECC_MODE__NONE) {
>
> It need set a default value to config psp->ecc_mode, otherwise, it is
> always "0" in this implementation.

For ASIC support ECC, it's set in psp_sw_init(), like vega10 case.
For ASIC not support ECC yet, it always "0", indicating PSP_ECC_MODE__NONE,
aka not enabled by default.

It's expected result, I think.

>
>> +		if (psp_ring_create(psp, PSP_RING_TYPE__KM))
>> +			goto failed_mem;
>> +		if (psp_tmr_load(psp))
>> +			goto failed_mem;
>> +	} else {
>> +		if (psp_hw_start(psp))
>> +			goto failed_mem;
>> +	}
>>
>>   	ret = psp_np_fw_load(psp);
>>   	if (ret)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
>> index 3776186..8ec9194 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
>> @@ -63,6 +63,19 @@ enum psp_bootloader_command_list
>>   	PSP_BL__DEFAULT_ECC = 0x30003,
>>   };
>>
>> +enum psp_ecc_mode
>> +{
>> +	PSP_ECC_MODE__NONE = 0,
>> +	PSP_ECC_MODE__OFF = 1,
>> +	PSP_ECC_MODE__ON = 2,
>> +	PSP_ECC_MODE__PARTIALON = 3,
>> +};
>> +
>> +struct psp_config
>> +{
>> +	enum psp_ecc_mode		ecc_mode;
>> +};
>> +
>>   struct psp_context
>>   {
>>   	struct amdgpu_device            *adev;
>> @@ -70,6 +83,8 @@ struct psp_context
>>   	struct psp_gfx_cmd_resp		*cmd;
>>
>>   	int (*init_microcode)(struct psp_context *psp);
>> +	int (*bootloader_set_ecc_mode)(struct psp_context *psp);
>> +	bool (*bootloader_is_sos_running)(struct psp_context *psp);
>>   	int (*bootloader_load_sysdrv)(struct psp_context *psp);
>>   	int (*bootloader_load_sos)(struct psp_context *psp);
>>   	int (*prep_cmd_buf)(struct amdgpu_firmware_info *ucode,
>> @@ -123,6 +138,9 @@ struct psp_context
>>   	struct amdgpu_bo		*cmd_buf_bo;
>>   	uint64_t			cmd_buf_mc_addr;
>>   	struct psp_gfx_cmd_resp		*cmd_buf_mem;
>> +
>> +	/* psp config */
>> +	struct psp_config		config;
>
> At current, we don't need a psp_config wrapper here. Use "enum ecc_mode"
> directly to make code more simple.

I considered it twice when implemented the code.
IMO, it's good way to collect all config info in a structure like gfx config. 
It's not only used for ECC, but an initial step for psp_config.

How do you think about it?

Jerry

>
>>   };
>>
>>   struct amdgpu_psp_funcs {
>> @@ -140,6 +158,10 @@ struct amdgpu_psp_funcs {
>>   		(psp)->compare_sram_data((psp), (ucode), (type))
>>   #define psp_init_microcode(psp) \
>>   		((psp)->init_microcode ? (psp)->init_microcode((psp)) : 0)
>> +#define psp_bootloader_set_ecc_mode(psp) \
>> +		((psp)->bootloader_set_ecc_mode ? (psp)->bootloader_set_ecc_mode((psp)) : 0)
>> +#define psp_bootloader_is_sos_running(psp) \
>> +		((psp)->bootloader_is_sos_running ? (psp)->bootloader_is_sos_running((psp)) : 0)
>>   #define psp_bootloader_load_sysdrv(psp) \
>>   		((psp)->bootloader_load_sysdrv ? (psp)->bootloader_load_sysdrv((psp)) : 0)
>>   #define psp_bootloader_load_sos(psp) \
>> --
>> 1.9.1
>>
>> _______________________________________________
>> 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
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amdgpu: add psp ecc support

[Huang Rui]

On Mon, Jul 31, 2017 at 04:10:10PM +0800, Zhang, Jerry wrote:
> On 07/31/2017 01:57 PM, Huang Rui wrote:
> > On Fri, Jul 28, 2017 at 05:11:18PM +0800, Junwei Zhang wrote:
> >> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
> >> ---
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 17 ++++++++++++++---
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 22 ++++++++++++++++++++++
> >>   2 files changed, 36 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> >> index 1aa41af..b04cc80 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> >> @@ -256,6 +256,10 @@ static int psp_hw_start(struct psp_context *psp)
> >>   {
> >>   	int ret;
> >>
> >> +	ret = psp_bootloader_set_ecc_mode(psp);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >>   	ret = psp_bootloader_load_sysdrv(psp);
> >>   	if (ret)
> >>   		return ret;
> >> @@ -365,9 +369,16 @@ static int psp_load_fw(struct amdgpu_device *adev)
> >>   	if (ret)
> >>   		goto failed_mem;
> >>
> >> -	ret = psp_hw_start(psp);
> >> -	if (ret)
> >> -		goto failed_mem;
> >> +	if (psp_bootloader_is_sos_running(psp) &&
> >> +			psp->config.ecc_mode != PSP_ECC_MODE__NONE) {
> >
> > It need set a default value to config psp->ecc_mode, otherwise, it is
> > always "0" in this implementation.
> 
> For ASIC support ECC, it's set in psp_sw_init(), like vega10 case.

Not only asic support, but also VBIOS enablement. Yes, we would better
initialize it in sw init and use a boot param to configure it at least
until we confirmed the method to get ecc_mode from VBIOS. At same time, it
should add a comment to like below in sw_init:

/* FIXME: will find a way to get ecc mode via VBIOS */

> For ASIC not support ECC yet, it always "0", indicating PSP_ECC_MODE__NONE,
> aka not enabled by default.
> 
> It's expected result, I think.
> 
> >
> >> +		if (psp_ring_create(psp, PSP_RING_TYPE__KM))
> >> +			goto failed_mem;
> >> +		if (psp_tmr_load(psp))
> >> +			goto failed_mem;
> >> +	} else {
> >> +		if (psp_hw_start(psp))
> >> +			goto failed_mem;
> >> +	}
> >>
> >>   	ret = psp_np_fw_load(psp);
> >>   	if (ret)
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> >> index 3776186..8ec9194 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> >> @@ -63,6 +63,19 @@ enum psp_bootloader_command_list
> >>   	PSP_BL__DEFAULT_ECC = 0x30003,
> >>   };
> >>
> >> +enum psp_ecc_mode
> >> +{
> >> +	PSP_ECC_MODE__NONE = 0,
> >> +	PSP_ECC_MODE__OFF = 1,
> >> +	PSP_ECC_MODE__ON = 2,
> >> +	PSP_ECC_MODE__PARTIALON = 3,
> >> +};
> >> +
> >> +struct psp_config
> >> +{
> >> +	enum psp_ecc_mode		ecc_mode;
> >> +};
> >> +
> >>   struct psp_context
> >>   {
> >>   	struct amdgpu_device            *adev;
> >> @@ -70,6 +83,8 @@ struct psp_context
> >>   	struct psp_gfx_cmd_resp		*cmd;
> >>
> >>   	int (*init_microcode)(struct psp_context *psp);
> >> +	int (*bootloader_set_ecc_mode)(struct psp_context *psp);
> >> +	bool (*bootloader_is_sos_running)(struct psp_context *psp);
> >>   	int (*bootloader_load_sysdrv)(struct psp_context *psp);
> >>   	int (*bootloader_load_sos)(struct psp_context *psp);
> >>   	int (*prep_cmd_buf)(struct amdgpu_firmware_info *ucode,
> >> @@ -123,6 +138,9 @@ struct psp_context
> >>   	struct amdgpu_bo		*cmd_buf_bo;
> >>   	uint64_t			cmd_buf_mc_addr;
> >>   	struct psp_gfx_cmd_resp		*cmd_buf_mem;
> >> +
> >> +	/* psp config */
> >> +	struct psp_config		config;
> >
> > At current, we don't need a psp_config wrapper here. Use "enum ecc_mode"
> > directly to make code more simple.
> 
> I considered it twice when implemented the code.
> IMO, it's good way to collect all config info in a structure like gfx config. 
> It's not only used for ECC, but an initial step for psp_config.
> 
> How do you think about it?
> 

Yes, make sense. But we could introduce the config wrapper when we need add
new configration in future. Currently, it looks a little superfluous.
Anyway, I am also fine if you want to introduce it now, no matter.

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

Re: [PATCH 2/3] drm/amdgpu: add psp ecc support

[Zhang, Jerry (Junwei)]

On 07/31/2017 04:41 PM, Huang Rui wrote:
> On Mon, Jul 31, 2017 at 04:10:10PM +0800, Zhang, Jerry wrote:
>> On 07/31/2017 01:57 PM, Huang Rui wrote:
>>> On Fri, Jul 28, 2017 at 05:11:18PM +0800, Junwei Zhang wrote:
>>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 17 ++++++++++++++---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 22 ++++++++++++++++++++++
>>>>    2 files changed, 36 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>>>> index 1aa41af..b04cc80 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>>>> @@ -256,6 +256,10 @@ static int psp_hw_start(struct psp_context *psp)
>>>>    {
>>>>    	int ret;
>>>>
>>>> +	ret = psp_bootloader_set_ecc_mode(psp);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>>    	ret = psp_bootloader_load_sysdrv(psp);
>>>>    	if (ret)
>>>>    		return ret;
>>>> @@ -365,9 +369,16 @@ static int psp_load_fw(struct amdgpu_device *adev)
>>>>    	if (ret)
>>>>    		goto failed_mem;
>>>>
>>>> -	ret = psp_hw_start(psp);
>>>> -	if (ret)
>>>> -		goto failed_mem;
>>>> +	if (psp_bootloader_is_sos_running(psp) &&
>>>> +			psp->config.ecc_mode != PSP_ECC_MODE__NONE) {
>>>
>>> It need set a default value to config psp->ecc_mode, otherwise, it is
>>> always "0" in this implementation.
>>
>> For ASIC support ECC, it's set in psp_sw_init(), like vega10 case.
>
> Not only asic support, but also VBIOS enablement. Yes, we would better
> initialize it in sw init and use a boot param to configure it at least
> until we confirmed the method to get ecc_mode from VBIOS. At same time, it
> should add a comment to like below in sw_init:
>
> /* FIXME: will find a way to get ecc mode via VBIOS */

Thanks for your advice.
I could add a comment for that.

Actually there is a patch 3 you may miss in mailbox, where already initialize 
the ecc mode for vega10.
   * [PATCH 3/3] drm/amdgpu: add psp ecc support for vega10

Currently I'm not sure the requirements for ECC support, especially ECC mode 
initialization. So just hard code ECC mode for a specific ASIC(Vega10 for now).

>
>> For ASIC not support ECC yet, it always "0", indicating PSP_ECC_MODE__NONE,
>> aka not enabled by default.
>>
>> It's expected result, I think.
>>
>>>
>>>> +		if (psp_ring_create(psp, PSP_RING_TYPE__KM))
>>>> +			goto failed_mem;
>>>> +		if (psp_tmr_load(psp))
>>>> +			goto failed_mem;
>>>> +	} else {
>>>> +		if (psp_hw_start(psp))
>>>> +			goto failed_mem;
>>>> +	}
>>>>
>>>>    	ret = psp_np_fw_load(psp);
>>>>    	if (ret)
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
>>>> index 3776186..8ec9194 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
>>>> @@ -63,6 +63,19 @@ enum psp_bootloader_command_list
>>>>    	PSP_BL__DEFAULT_ECC = 0x30003,
>>>>    };
>>>>
>>>> +enum psp_ecc_mode
>>>> +{
>>>> +	PSP_ECC_MODE__NONE = 0,
>>>> +	PSP_ECC_MODE__OFF = 1,
>>>> +	PSP_ECC_MODE__ON = 2,
>>>> +	PSP_ECC_MODE__PARTIALON = 3,
>>>> +};
>>>> +
>>>> +struct psp_config
>>>> +{
>>>> +	enum psp_ecc_mode		ecc_mode;
>>>> +};
>>>> +
>>>>    struct psp_context
>>>>    {
>>>>    	struct amdgpu_device            *adev;
>>>> @@ -70,6 +83,8 @@ struct psp_context
>>>>    	struct psp_gfx_cmd_resp		*cmd;
>>>>
>>>>    	int (*init_microcode)(struct psp_context *psp);
>>>> +	int (*bootloader_set_ecc_mode)(struct psp_context *psp);
>>>> +	bool (*bootloader_is_sos_running)(struct psp_context *psp);
>>>>    	int (*bootloader_load_sysdrv)(struct psp_context *psp);
>>>>    	int (*bootloader_load_sos)(struct psp_context *psp);
>>>>    	int (*prep_cmd_buf)(struct amdgpu_firmware_info *ucode,
>>>> @@ -123,6 +138,9 @@ struct psp_context
>>>>    	struct amdgpu_bo		*cmd_buf_bo;
>>>>    	uint64_t			cmd_buf_mc_addr;
>>>>    	struct psp_gfx_cmd_resp		*cmd_buf_mem;
>>>> +
>>>> +	/* psp config */
>>>> +	struct psp_config		config;
>>>
>>> At current, we don't need a psp_config wrapper here. Use "enum ecc_mode"
>>> directly to make code more simple.
>>
>> I considered it twice when implemented the code.
>> IMO, it's good way to collect all config info in a structure like gfx config.
>> It's not only used for ECC, but an initial step for psp_config.
>>
>> How do you think about it?
>>
>
> Yes, make sense. But we could introduce the config wrapper when we need add
> new configration in future. Currently, it looks a little superfluous.
> Anyway, I am also fine if you want to introduce it now, no matter.

Got your concern.

Cause actually we always forget to add config structure until some day when 
somebody gets time to have a code clean-up.

Jerry

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

[PATCH 3/3] drm/amdgpu: add psp ecc support for vega10

[Junwei Zhang]

Disable ecc by default

Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c |  3 ++
 drivers/gpu/drm/amd/amdgpu/psp_v3_1.c   | 60 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/psp_v3_1.h   |  2 ++
 3 files changed, 65 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index b04cc80..ec433b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -52,6 +52,8 @@ static int psp_sw_init(void *handle)
 	switch (adev->asic_type) {
 	case CHIP_VEGA10:
 		psp->init_microcode = psp_v3_1_init_microcode;
+		psp->bootloader_is_sos_running = psp_v3_1_bootloader_is_sos_running;
+		psp->bootloader_set_ecc_mode = psp_v3_1_bootloader_set_ecc_mode;
 		psp->bootloader_load_sysdrv = psp_v3_1_bootloader_load_sysdrv;
 		psp->bootloader_load_sos = psp_v3_1_bootloader_load_sos;
 		psp->prep_cmd_buf = psp_v3_1_prep_cmd_buf;
@@ -61,6 +63,7 @@ static int psp_sw_init(void *handle)
 		psp->cmd_submit = psp_v3_1_cmd_submit;
 		psp->compare_sram_data = psp_v3_1_compare_sram_data;
 		psp->smu_reload_quirk = psp_v3_1_smu_reload_quirk;
+		psp->config.ecc_mode = PSP_ECC_MODE__NONE;
 		break;
 	case CHIP_RAVEN:
 #if 0
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
index f93a66e..0a51dde 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
@@ -162,6 +162,66 @@ int psp_v3_1_init_microcode(struct psp_context *psp)
 	return err;
 }
 
+bool psp_v3_1_bootloader_is_sos_running(struct psp_context *psp)
+{
+	struct amdgpu_device *adev = psp->adev;
+
+	if (RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_81) != 0)
+		return true;
+	else
+		return false;
+}
+
+int psp_v3_1_bootloader_set_ecc_mode(struct psp_context *psp)
+{
+	int ret = 0;
+	uint32_t sol_reg;
+	struct amdgpu_device *adev = psp->adev;
+	uint32_t psp_gfxdrv_command_reg = 0;
+
+	/* Workaround: check bootloader version and skip old one */
+	sol_reg = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_81);
+	if (sol_reg < 0xB0C00)
+		return ret;
+
+	switch (psp->config.ecc_mode)
+	{
+		case PSP_ECC_MODE__NONE:
+			break;
+		case PSP_ECC_MODE__OFF:
+			psp_gfxdrv_command_reg = PSP_BL__NO_ECC;
+			break;
+		case PSP_ECC_MODE__ON:
+			psp_gfxdrv_command_reg = PSP_BL__FULL_ECC;
+			break;
+		case PSP_ECC_MODE__PARTIALON:
+			psp_gfxdrv_command_reg = PSP_BL__PARTIAL_ECC;
+			break;
+		default:
+			break;
+	}
+
+	if (psp_gfxdrv_command_reg == 0)
+		return ret;
+
+	/* Wait for bootloader to signify that is ready having bit 31 of C2PMSG_35 set to 1 */
+	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+			   0x80000000, 0x80000000, false);
+	if (ret)
+		return ret;
+
+	WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, psp_gfxdrv_command_reg);
+
+	/* There might be handshake issue with hardware which needs delay */
+	mdelay(20);
+
+	/* Wait for bootloader to signify that is ready having bit 31 of C2PMSG_35 set to 1 */
+	ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+			   0x80000000, 0x80000000, false);
+
+	return ret;
+}
+
 int psp_v3_1_bootloader_load_sysdrv(struct psp_context *psp)
 {
 	int ret;
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.h b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.h
index 9dcd0b2..3e52b5d 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.h
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.h
@@ -34,6 +34,8 @@
 
 extern int psp_v3_1_init_microcode(struct psp_context *psp);
 extern int psp_v3_1_bootloader_load_sysdrv(struct psp_context *psp);
+extern bool psp_v3_1_bootloader_is_sos_running(struct psp_context *psp);
+extern int psp_v3_1_bootloader_set_ecc_mode(struct psp_context *psp);
 extern int psp_v3_1_bootloader_load_sos(struct psp_context *psp);
 extern int psp_v3_1_prep_cmd_buf(struct amdgpu_firmware_info *ucode,
 				 struct psp_gfx_cmd_resp *cmd);
-- 
1.9.1

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