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

[Ang, Chee Hong <chee.hong.ang@intel.com>]

On Fri, 2018-04-27 at 09:08 +0200, Marek Vasut wrote:
> 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 ?
There is a separate patch for mailbox communication stuffs in the
process of upstreaming by my colleague Tan, Ley Foon. But currently, I
don't think the mailbox code is placed under drivers/mailbox. I can
refactor this code into a generic function like 'get fpga device
status' via mailbox and place it under drivers/mailbox. Will work with
her to address this.
> 
> [...]
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > +	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 ?
Yes, these mailbox responses are stored in buffer and will be retrieved
in sequence at later stage.
> 
> [...]
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > +	/* 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.
Yes. It works reliably.
>