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

[Alex Deucher <alexdeucher@gmail.com>]

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