[lkml]   [2011]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRE: [RFC PATCHv5 1/7] HSI: hsi: Introducing HSI framework
    Hi Sjur,

    Sorry for the long delay. My comments below:

    On Tue, 2011-06-28 at 15:05 +0200, ext Sjur BRENDELAND wrote:
    -- cut--
    > >
    > > > Will multiple read queued operations result in chained DMA
    > > operations, or single read operations?
    > > >
    > >
    > > Well, it is up to you, but my initial idea is that the complete
    > > callback is called right after the request has been fulfilled. This
    > > may be not possible if you chain several read requests.
    > I think our concern is to squeeze every bit of bandwidth out of the link.
    > Perhaps we can utilize bandwidth better by chaining the DMA jobs.
    > But due to latency we need the complete callback for each DMA job.
    > If the DMA cannot handle this, the HSI controller should handle the queuing
    > of IO requests.


    > > > ...
    > > > >+/**
    > > > >+ * hsi_flush - Flush all pending transactions on the client's port
    > > > >+ * @cl: Pointer to the HSI client
    > > > >+ *
    > > > >+ * This function will destroy all pending hsi_msg in the port and
    > > reset
    > > > >+ * the HW port so it is ready to receive and transmit from a clean
    > > state.
    > > > >+ *
    > > > >+ * Return -errno on failure, 0 on success */ static inline int
    > > > >+hsi_flush(struct hsi_client *cl) {
    > > > >+ if (!hsi_port_claimed(cl))
    > > > >+ return -EACCES;
    > > > >+ return hsi_get_port(cl)->flush(cl); }
    > > >
    > > > For CAIF we need to have independent RX and TX flush operations.
    > > >
    > >
    > > The current framework assumes that in the unlikely case of an error or
    > > whenever you need to to do some cleanup, you will end up cleaning up
    > > the two sides anyway. Moreover, you will reset the state of the HW
    > > also.
    > >
    > > Exactly, in which case CAIF will only need to cleanup the TX path but
    > > not the RX or vice versa ?
    > >
    > > > Flushing the TX FIFO can be long duration operation due to HSI flow
    > > control,
    > > > if counterpart is not receiving data. I would prefer to see a
    > > callback here.
    > > >
    > >
    > > No, flush should not wait for an ongoing TX transfer to finish. You
    > > should stop all ongoing transfers, call the destructor callback on all
    > > the requests (queued and ongoing) and clean up the HW state.
    > ...
    > > > *Missing function*: hsi_reset()
    > > > I would also like to see a hsi_reset function.
    > >
    > > This is currently done also in the hsi_flush. See my previous comment.
    > Sorry, I misunderstood your API description here. hsi_flush() seems to work
    > like the hsi_reset I was looking for. I would prefer if you rename this
    > function to hsi_reset for clarity (see flush_work() in workqueue.c, where
    > flush wait for the work to finish).
    > Anyway, I still see a need for ensuring fifos are empty or reading the
    > number of bytes in the fifos.
    > CAIF is using the wake lines for controlling when the Modem and Host can
    > power down the HSI blocks. In order to go to low power mode, the Host set
    > AC_WAKE low, and wait for modem to respond by setting CA_WAKE low. The host
    > cannot set AC_WAKE high again before modem has done CA_WAKE low (in order
    > to avoid races).
    > When going up from low power anyone could set the WAKE line high, and wait for
    > the other side to respond by setting WAKE line high.
    > So CAIF implements the following protocol for handshaking before going to
    > low-power mode:
    > 1. Inactivity timeout expires on Host, i.e the host has nothing to send and no
    > RX has happened the last couple of seconds.
    > 2. Host request low-power state by setting AC_WAKE low. In this state Host
    > side can still receive data, but is not allowed to send data.
    > 3. Modem responds with setting CA_WAKE low, and cannot send data either.
    > 4. When both AC_WAKE and CA_WAKE are low, the host must set AC_FLAG, AC_DATA
    > and AC_READY low.
    > 5. When Host side RX-fifo is empty and DMA jobs are completed,
    > ongoing RX requests are cancelled.
    > 6. HSI block can be powered down.
    > After AC_WAKE is set low the Host must guarantee that the modem does not receive
    > data until AC_WAKE is high again. This implies that the Host must know that the
    > TX FIFO is empty before setting the AC_WAKE low. So we need some way to know that
    > the TX fifo is empty.
    > I think the cleanest design here is that hsi_stop_tx() handles this.
    > So his_stop_tx() should wait for any pending TX jobs and wait for the TX
    > FIFO to be empty, and then set the AC_WAKE low.

    Hmmm, I don't like this. My initial idea is that you could call this
    functions on interrupt context. This will prevent this. However, nothing
    prevents you from schedule a delayed work, which will be in charge of
    checking that the last frame has gone through the wire and then bring
    down the AC_WAKE line.

    > As described above, when going down to low power mode the host has set AC_WAKE low.
    > The Host should then set AC_FLAG, AC_DATA and AC_READY low.
    > >...I think also the
    > >workaround of setting the DATA and FLAG lines low can be implemented
    > >just in the hsi_controller without hsi_client intervention.
    > Great :-) Perhaps a function hsi_stop_rx()?

    No need for a new function. The hsi_controller driver knows when it goes
    to "low power mode" so it can set the DATA and FLAG lines down just righ
    before that.

    > > > *Missing function*: hsi_rx_fifo_occupancy()
    > > > Before putting the link asleep we need to know if the fifo is empty
    > > > or not.
    > > > So we would like to have a way to read out the number of bytes in the
    > > > RX fifo.
    > >
    > > This should be handled only by hsi_controller. Clients should not care
    > > about this.
    > There is a corner case when going to low power mode and both side has put the WAKE line low,
    > but a RX DMA job is ongoing or the RX-fifo is not empty.
    > In this case the host side must wait for the DMA job to complete and RX-
    > fifo to be empty, before canceling any pending RX jobs.
    > One option would be to provide a function hsi_rx_sync() that guarantees that any ongoing
    > RX-job is completed and that the RX FIFO is empty. Another option could be to be
    > able to provide API for reading RX-job states and RX-fifo occupancy.

    I think we don't need another function to do this neither. The
    hsi_controller driver should implement a usecount scheme to know when
    the HW can be switch off. IMO it is not a good idea to relay just on the
    wakelines to power on/off the device, exactly because of this kind of

    Carlos Chinea <>

     \ /
      Last update: 2011-07-22 12:51    [W:0.031 / U:4.208 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site