[U-Boot] [PATCH v1 1/3] arm: socfpga: stratix10: Add Stratix10 FPGA Reconfiguration Driver

[Marek Vasut]

On 04/27/2018 07:51 AM, Ang, Chee Hong wrote:
[...]

>>>>> +		/* Check for any error */
>>>>> +		ret =
>>>>> reconfig_status_resp[RECONFIG_STATUS_STATE];
>>>>> +		if (ret && ret != MBOX_CFGSTAT_STATE_CONFIG)
>>>>> +			return ret;
>>>>> +
>>>>> +		/* Make sure nStatus is not 0 */
>>>>> +		ret =
>>>>> reconfig_status_resp[RECONFIG_STATUS_PIN_STATUS];
>>>>> +		if (!(ret & RCF_PIN_STATUS_NSTATUS))
>>>>> +			return
>>>>> MBOX_CFGSTAT_STATE_ERROR_HARDWARE;
>>>> wait_for_bit_le32() or somesuch ?
>>> No, we don't read the bit status directly from register. We only
>>> need
>>> to test the bit of the pin status stored in memory.
>> Who's populating the memory ?
> ret = mbox_send_cmd(MBOX_ID_UBOOT, MBOX_RECONFIG_STATUS,
> MBOX_CMD_DIRECT, 0, NULL, 0, &reconfig_status_resp_len,
> reconfig_status_resp);
> 
> We send mailbox command to the device and the device will respond by
> filling the 'reconfig_status_resp' with the device status.

Ah, I see. Would it make sense to implement a generic "poll for bit" for
this mailbox communication ? Also, we have drivers/mailbox/ , would it
make sense to move the mailbox stuff there ?

[...]

>>>>> +	if (*resp_count < buf_size) {
>>>>> +		u32 rcv_len_max = buf_size - *resp_count;
>>>>> +
>>>>> +		if (rcv_len_max > MBOX_RESP_BUFFER_SIZE)
>>>>> +			rcv_len_max = MBOX_RESP_BUFFER_SIZE;
>>>>> +		resp_len = mbox_rcv_resp(buf, rcv_len_max);
>>>>> +
>>>>> +		for (i = 0; i < resp_len; i++) {
>>>>> +			resp_buf[(*w_index)++] = buf[i];
>>>> Is this like a memcpy() ?
>>> No. This is a circular buffer, index to the memory location may
>>> wrap
>>> around.
>> Two memcpys then ?
> Will refactor this part in v2.

Does it even have to be a circular buffer in the first place ?

[...]

>>>>> +	/* Make sure we don't send MBOX_RECONFIG_STATUS too
>>>>> fast
>>>>> */
>>>>> +	udelay(RECONFIG_STATUS_INTERVAL_DELAY_US);
>>>> Hum, this is iffy, is that a hardware bug ?
>>> Yes. The firmware running in that device is not able to response
>>> quickly.
>> And you cannot poll it in some way ?
> I know this is ugly and looks buggy. But there are no mechanism to poll
> whether the device is ready to accept any mailbox command or not. So
> for now, slowing down the mailbox exchange is the only way to go.

If it works reliably, so be it.

-- 
Best regards,
Marek Vasut