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@xxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
