[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 at amd.com> wrote:
>> Add the programming sequence.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at 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 at lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cf3c9cee87052401e07f508d7bef10174%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637187814630848721&sdata=Mo9%2BR4Nrh0rPxZ7xJrBzHl71M6rWL7KcrgEWOne6yQE%3D&reserved=0