From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ang, Chee Hong Date: Fri, 27 Apr 2018 09:10:20 +0000 Subject: [U-Boot] [PATCH v1 1/3] arm: socfpga: stratix10: Add Stratix10 FPGA Reconfiguration Driver In-Reply-To: References: <1524194806-4821-1-git-send-email-chee.hong.ang@intel.com> <1524194806-4821-2-git-send-email-chee.hong.ang@intel.com> <1524723173.29684.22.camel@intel.com> <1524808304.32372.17.camel@intel.com> Message-ID: <1524820219.33799.24.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. >