On Mon, Sep 28, 2020 at 6:26 PM Luben Tuikov <luben.tuikov@xxxxxxx> wrote:
>
> On 2020-09-25 4:10 p.m., Alex Deucher wrote:
> > From: Huang Rui <ray.huang@xxxxxxx>
> >
> > APU needs load toc firmware for gfx10 series on psp front door loading.
> >
> > v2: rebase against latest code
> >
> > Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> > Acked-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 11 ++++++++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 36 +++++++++++++++++++++++++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 7 +++++
> > drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 33 ++++++++++++++++-------
> > 4 files changed, 77 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > index bd0d14419841..26caa8d43483 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > @@ -325,6 +325,10 @@ static int amdgpu_firmware_info(struct drm_amdgpu_info_firmware *fw_info,
> > fw_info->ver = adev->dm.dmcub_fw_version;
> > fw_info->feature = 0;
> > break;
> > + case AMDGPU_INFO_FW_TOC:
> > + fw_info->ver = adev->psp.toc_fw_version;
> > + fw_info->feature = adev->psp.toc_feature_version;
> > + break;
> > default:
> > return -EINVAL;
> > }
> > @@ -1464,6 +1468,13 @@ static int amdgpu_debugfs_firmware_info(struct seq_file *m, void *data)
> > seq_printf(m, "DMCUB feature version: %u, firmware version: 0x%08x\n",
> > fw_info.feature, fw_info.ver);
> >
> > + /* TOC */
> > + query_fw.fw_type = AMDGPU_INFO_FW_TOC;
> > + ret = amdgpu_firmware_info(&fw_info, &query_fw, adev);
> > + if (ret)
> > + return ret;
> > + seq_printf(m, "TOC feature version: %u, firmware version: 0x%08x\n",
> > + fw_info.feature, fw_info.ver);
> >
> > seq_printf(m, "VBIOS version: %s\n", ctx->vbios_version);
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > index 18be544d8c1e..c8cec7ab499d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > @@ -2415,6 +2415,42 @@ int psp_init_asd_microcode(struct psp_context *psp,
> > return err;
> > }
> >
> > +int psp_init_toc_microcode(struct psp_context *psp,
> > + const char *chip_name)
> > +{
> > + struct amdgpu_device *adev = psp->adev;
> > + char fw_name[30];
> > + const struct psp_firmware_header_v1_0 *toc_hdr;
> > + int err = 0;
> > +
> > + if (!chip_name) {
> > + dev_err(adev->dev, "invalid chip name for toc microcode\n");
> > + return -EINVAL;
> > + }
> > +
> > + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_toc.bin", chip_name);
> > + err = request_firmware(&adev->psp.toc_fw, fw_name, adev->dev);
> > + if (err)
> > + goto out;
> > +
> > + err = amdgpu_ucode_validate(adev->psp.toc_fw);
> > + if (err)
> > + goto out;
> > +
> > + toc_hdr = (const struct psp_firmware_header_v1_0 *)adev->psp.toc_fw->data;
> > + adev->psp.toc_fw_version = le32_to_cpu(toc_hdr->header.ucode_version);
> > + adev->psp.toc_feature_version = le32_to_cpu(toc_hdr->ucode_feature_version);
> > + adev->psp.toc_bin_size = le32_to_cpu(toc_hdr->header.ucode_size_bytes);
> > + adev->psp.toc_start_addr = (uint8_t *)toc_hdr +
> > + le32_to_cpu(toc_hdr->header.ucode_array_offset_bytes);
> > + return 0;
> > +out:
>
> I'd rather this label be "Err:".
>
> Regardless of whether there already is a variable "err",
> (there is!), capitalizing goto labels is good practice, since
> it distinguishes them from variables (which are all lowercase),
> and macros (which are all caps). Plus, you also avoid conflict
> with the eponymous variable.
>
I see your point, but I find random caps in code hard to read.
> > + dev_err(adev->dev, "fail to initialize toc microcode\n");
>
> That's a very misleading message. Please print this instead:
>
> dev_err(adev->dev,
> "Failed to load/validate firmware for %s\n",
> fw_name);
>
> To make it clear what was being loaded and validated and failed.
>
Updated.
Thanks,
Alex
> Regards,
> Luben
>
> > + release_firmware(adev->psp.toc_fw);
> > + adev->psp.toc_fw = NULL;
> > + return err;
> > +}
> > +
> > int psp_init_sos_microcode(struct psp_context *psp,
> > const char *chip_name)
> > {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> > index 919d2fb7427b..13f56618660a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> > @@ -253,6 +253,11 @@ struct psp_context
> > uint32_t asd_ucode_size;
> > uint8_t *asd_start_addr;
> >
> > + /* toc firmware */
> > + const struct firmware *toc_fw;
> > + uint32_t toc_fw_version;
> > + uint32_t toc_feature_version;
> > +
> > /* fence buffer */
> > struct amdgpu_bo *fence_buf_bo;
> > uint64_t fence_buf_mc_addr;
> > @@ -386,6 +391,8 @@ int psp_ring_cmd_submit(struct psp_context *psp,
> > int index);
> > int psp_init_asd_microcode(struct psp_context *psp,
> > const char *chip_name);
> > +int psp_init_toc_microcode(struct psp_context *psp,
> > + const char *chip_name);
> > int psp_init_sos_microcode(struct psp_context *psp,
> > const char *chip_name);
> > int psp_init_ta_microcode(struct psp_context *psp,
> > diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> > index 6c5d9612abcb..f2d6b2518eee 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> > @@ -109,20 +109,16 @@ static int psp_v11_0_init_microcode(struct psp_context *psp)
> > BUG();
> > }
> >
> > - err = psp_init_sos_microcode(psp, chip_name);
> > - if (err)
> > - return err;
> > -
> > - if (adev->asic_type != CHIP_SIENNA_CICHLID &&
> > - adev->asic_type != CHIP_NAVY_FLOUNDER) {
> > - err = psp_init_asd_microcode(psp, chip_name);
> > - if (err)
> > - return err;
> > - }
> >
> > switch (adev->asic_type) {
> > case CHIP_VEGA20:
> > case CHIP_ARCTURUS:
> > + err = psp_init_sos_microcode(psp, chip_name);
> > + if (err)
> > + return err;
> > + err = psp_init_asd_microcode(psp, chip_name);
> > + if (err)
> > + return err;
> > snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_ta.bin", chip_name);
> > err = request_firmware(&adev->psp.ta_fw, fw_name, adev->dev);
> > if (err) {
> > @@ -150,6 +146,12 @@ static int psp_v11_0_init_microcode(struct psp_context *psp)
> > case CHIP_NAVI10:
> > case CHIP_NAVI14:
> > case CHIP_NAVI12:
> > + err = psp_init_sos_microcode(psp, chip_name);
> > + if (err)
> > + return err;
> > + err = psp_init_asd_microcode(psp, chip_name);
> > + if (err)
> > + return err;
> > if (amdgpu_sriov_vf(adev))
> > break;
> > snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_ta.bin", chip_name);
> > @@ -180,10 +182,21 @@ static int psp_v11_0_init_microcode(struct psp_context *psp)
> > break;
> > case CHIP_SIENNA_CICHLID:
> > case CHIP_NAVY_FLOUNDER:
> > + err = psp_init_sos_microcode(psp, chip_name);
> > + if (err)
> > + return err;
> > err = psp_init_ta_microcode(&adev->psp, chip_name);
> > if (err)
> > return err;
> > break;
> > + case CHIP_VANGOGH:
> > + err = psp_init_asd_microcode(psp, chip_name);
> > + if (err)
> > + return err;
> > + err = psp_init_toc_microcode(psp, chip_name);
> > + if (err)
> > + return err;
> > + break;
> > default:
> > BUG();
> > }
> >
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
