Re: [RFC PATCHv5 1/7] HSI: hsi: Introducing HSI framework
Hi,
On Sun, 2011-07-24 at 23:56 +0200, ext Sjur Brændeland wrote:
Hi Carlos,
Sorry for the long delay. My comments below:
No worries, I will probably be very slow when responding to you as
well for the next
couple of weeks...
+ * @flow: RX flow type (SYNCHRONIZED or PIPELINE)
+ * @arb_mode: Arbitration mode for TX frame (Round robin, priority)
+ */
+struct hsi_config {
+unsigned intmode;
+unsigned intchannels;
+unsigned intspeed;
I have to pick up on one issue I missed earlier. The CAIF-HSI protocol
is going to use
separate RX and TX speeds, where modem and host side looks at the
throughput and
TX-queues and request their TX speeds accordingly. So I would prefer
to be able to set
the RX and TX speed in each direction individually.
You can already do that ;) RX and TX configuration are different.
/**
* struct hsi_client - HSI client attached to an HSI port
* @device: Driver model representation of the device
* @tx_cfg: HSI TX configuration
* @rx_cfg: HSI RX configuration
* @hsi_start_rx: Called after incoming wake line goes high
* @hsi_stop_rx: Called after incoming wake line goes low
*/
struct hsi_client {
struct device device;
struct hsi_config tx_cfg;
struct hsi_config rx_cfg;
...
...
... Why don't you
just remove the sync API altogether and use only async, then the OMAP
HSI controller driver is supposed to know when it can go to sleep. If
you receive some data before a client queues a request, you just defer
processing of that data until a new request is queued, or something...
Hmmm, Do you mean I remove the hsi_start_tx() and hsi_stop_tx()
completely ? Or Do I just create an async version of them ?
I would say remove completely and add async-only version.
Yes, this is probably the best way, but I'm not too concerned how this is
done,
as long as the API provides some way to assure that the TX FIFO is empty
before putting the WAKE line low.
Ok let's see. We can rephrase this problem as that you want to be
certain that the last TX frame has gone through the wires before doing
something, like bringing the wake line down.
This can be done and should be done in the hsi_controller. It is just a
matter of calling the complete() callback on the right time. Meaning,
that the hsi_controller does not call the complete() callback when the
DMA transfer for TX has completed, but when the last TX frame is already
in the wires. As optimization, you may also do this only when there is
no more TX requests in the hsi_controller driver queue.
*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
issues.
For the RX FIFO maybe you are right that the controller can handle the
power down issues
on it's own. However I'm uneasy about not having the possibility to
read out the RX
FIFO-occupancy from the HSI-controller. In the CAIF HSI implementation
queued for the 3.1
kernel
(git.kernel.org/?p=linux/kernel/git/davem/net.git;a=blob;f=drivers/net/caif/caif_hsi.c
)
we use this both in probe() and when wakeline go low.
There may also be other corner-cases related to wakeline handling or
speed change,
where we need this in the future. So from my point of view think we
still need to be able read
out the RX FIFO occupancy.
In the case of wakeline handling:
- The HSI HW block should be kept power on as long as there is some
activity, regardless of the wake line state.
In the case of speed change:
- Could you explain a little bit more which is the problem ?
How can the data already stored in the RX FIFO be affected by changes
in the RX and/or TX HSI functional clock ?
Br,
--
Carlos Chinea carlos.chi...@nokia.com
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo
Re: [RFC PATCHv5 1/7] HSI: hsi: Introducing HSI framework
Hi Carlos,
Sorry for the long delay. My comments below:
No worries, I will probably be very slow when responding to you as
well for the next
couple of weeks...
+ * @flow: RX flow type (SYNCHRONIZED or PIPELINE)
+ * @arb_mode: Arbitration mode for TX frame (Round robin, priority)
+ */
+struct hsi_config {
+ unsigned intmode;
+ unsigned intchannels;
+ unsigned intspeed;
I have to pick up on one issue I missed earlier. The CAIF-HSI protocol
is going to use
separate RX and TX speeds, where modem and host side looks at the
throughput and
TX-queues and request their TX speeds accordingly. So I would prefer
to be able to set
the RX and TX speed in each direction individually.
...
... Why don't you
just remove the sync API altogether and use only async, then the OMAP
HSI controller driver is supposed to know when it can go to sleep. If
you receive some data before a client queues a request, you just defer
processing of that data until a new request is queued, or something...
Hmmm, Do you mean I remove the hsi_start_tx() and hsi_stop_tx()
completely ? Or Do I just create an async version of them ?
I would say remove completely and add async-only version.
Yes, this is probably the best way, but I'm not too concerned how this is done,
as long as the API provides some way to assure that the TX FIFO is empty
before putting the WAKE line low.
*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
issues.
For the RX FIFO maybe you are right that the controller can handle the
power down issues
on it's own. However I'm uneasy about not having the possibility to
read out the RX
FIFO-occupancy from the HSI-controller. In the CAIF HSI implementation
queued for the 3.1
kernel
(git.kernel.org/?p=linux/kernel/git/davem/net.git;a=blob;f=drivers/net/caif/caif_hsi.c
)
we use this both in probe() and when wakeline go low.
There may also be other corner-cases related to wakeline handling or
speed change,
where we need this in the future. So from my point of view think we
still need to be able read
out the RX FIFO occupancy.
Regards,
Sjur
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [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.
Exactly.
...
+/**
+ * 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
Re: [RFC PATCHv5 1/7] HSI: hsi: Introducing HSI framework
Hi,
On Fri, Jul 22, 2011 at 01:43:36PM +0300, Carlos Chinea wrote:
+/**
+ * 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.
why don't you use threaded IRQ ? It's higher priority than a delayed
work and you would be able to use something like
wait_for_completion_timeout() to wait for TX FIFOs to become empty (??)
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.
but, if I read correctly, only when TX FIFO is known to be empty, right?
...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.
are you already using pm_runtime ? You could put that logic grouped in
runtime_suspend()/runtime_resume() calls and from the driver, just call
pm_runtime_put()/pm_runtime_get_sync() whenever you want to
suspend/resume.
*Missing function*: hsi_rx_fifo_occupancy()
Before putting the link
Re: [RFC PATCHv5 1/7] HSI: hsi: Introducing HSI framework
Hi Felipe,
:)
On Fri, 2011-07-22 at 14:01 +0300, ext Felipe Balbi wrote:
Hi,
On Fri, Jul 22, 2011 at 01:43:36PM +0300, Carlos Chinea wrote:
+/**
+ * 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.
why don't you use threaded IRQ ? It's higher priority than a delayed
work and you would be able to use something like
wait_for_completion_timeout() to wait for TX FIFOs to become empty (??)
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.
but, if I read correctly, only when TX FIFO is known to be empty, right?
...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.
are you already using pm_runtime ? You could put that logic grouped in
Re: [RFC PATCHv5 1/7] HSI: hsi: Introducing HSI framework
Hi, On Fri, Jul 22, 2011 at 02:51:12PM +0300, Carlos Chinea wrote: Hi Felipe, hello there :-) are you already using pm_runtime ? You could put that logic grouped in runtime_suspend()/runtime_resume() calls and from the driver, just call pm_runtime_put()/pm_runtime_get_sync() whenever you want to suspend/resume. In the case of the omap_ssi driver not yet, but hopefully the implementation will come soon ;) Anyway, the DATA/FLAG workaround is not something I'm going to add to the omap_ssi, at least not now. This seems to be needed by ST-Ericcson in their HSI controller. aha, I see. Thanks for the clarification. true, but I'm not sure a usecount is a good way to go. Why don't you just remove the sync API altogether and use only async, then the OMAP HSI controller driver is supposed to know when it can go to sleep. If you receive some data before a client queues a request, you just defer processing of that data until a new request is queued, or something... Hmmm, Do you mean I remove the hsi_start_tx() and hsi_stop_tx() completely ? Or Do I just create an async version of them ? I would say remove completely and add async-only version. The truth is that they should not even exist in the first place, and the wakelines should have been handled silently by the hsi controllers. But they are need because the hsi_clients/protocols, like CAIF or the ssi_protocol in N900, need access to the wakelines to implement some workarounds due to some races in the HSI/SSI HW spec. that's quite nasty situation :-) I still think, though, that if the clients don't really start a transfer straight away (meaning it's all ASYNC), you might be able to handle the workarounds in the HSI framework itself by having some extra flags on your hsi request structure (?). See for example the use of short_not_ok flags on the gadget framework. The gadget drivers which can't handle short transfers (as of today only the Mass Storage gadget) will set that flag to tell the controller that we're not expecting a short packet and if we do, treat it as error. In case of such error, gadget driver is required to dequeue any queued transfer, flush the FIFO and restart ;-) Maybe you could have something similar ? Depending on the workaround you're talking about, this might be feasible with few lines of code on the async API. -- balbi signature.asc Description: Digital signature
Re: [RFC PATCHv5 1/7] HSI: hsi: Introducing HSI framework
On Fri, 2011-07-22 at 15:05 +0300, ext Felipe Balbi wrote: Hi, On Fri, Jul 22, 2011 at 02:51:12PM +0300, Carlos Chinea wrote: Hi Felipe, hello there :-) are you already using pm_runtime ? You could put that logic grouped in runtime_suspend()/runtime_resume() calls and from the driver, just call pm_runtime_put()/pm_runtime_get_sync() whenever you want to suspend/resume. In the case of the omap_ssi driver not yet, but hopefully the implementation will come soon ;) Anyway, the DATA/FLAG workaround is not something I'm going to add to the omap_ssi, at least not now. This seems to be needed by ST-Ericcson in their HSI controller. aha, I see. Thanks for the clarification. true, but I'm not sure a usecount is a good way to go. Why don't you just remove the sync API altogether and use only async, then the OMAP HSI controller driver is supposed to know when it can go to sleep. If you receive some data before a client queues a request, you just defer processing of that data until a new request is queued, or something... Hmmm, Do you mean I remove the hsi_start_tx() and hsi_stop_tx() completely ? Or Do I just create an async version of them ? I would say remove completely and add async-only version. The truth is that they should not even exist in the first place, and the wakelines should have been handled silently by the hsi controllers. But they are need because the hsi_clients/protocols, like CAIF or the ssi_protocol in N900, need access to the wakelines to implement some workarounds due to some races in the HSI/SSI HW spec. that's quite nasty situation :-) I still think, though, that if the clients don't really start a transfer straight away (meaning it's all ASYNC), you might be able to handle the workarounds in the HSI framework itself by having some extra flags on your hsi request structure (?). See for example the use of short_not_ok flags on the gadget framework. The gadget drivers which can't handle short transfers (as of today only the Mass Storage gadget) will set that flag to tell the controller that we're not expecting a short packet and if we do, treat it as error. In case of such error, gadget driver is required to dequeue any queued transfer, flush the FIFO and restart ;-) Maybe you could have something similar ? Depending on the workaround you're talking about, this might be feasible with few lines of code on the async API. The problem is that there is no standard way to handle this situation :( For example ssi_protocol tries to deal with the wakelines race problem by raising the wakeline and then waiting for the other end to send a READY to receive data frame, before start sending. But as you can see in the case of CAIF, they went for a solution where they raise their wakeline and wait for the other end to raise its wakeline to signal that they are READY to received. Both solutions have their pros and cons. Br, -- Carlos Chinea carlos.chi...@nokia.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCHv5 1/7] HSI: hsi: Introducing HSI framework
Hi Carlos,
What about information about the supported transmission speeds?
Is it any way to obtain this information, so we can know the legal
values for speed in hsi_config?
For TX speed sets the max tx speed the HSI should go. The controller
driver will try to get as close as it possible to that value without
going over.
For the RX path it sets min RX speed the HSI should use. The
controller should ensure that it does not drop under that value to
avoid breaking the communication.
All this values need to be and can be known beforehand and are
platform specific.
Ok fine, so the controller adjusts the speed according the HW supported
frequencies.
Q: Can multiple Read operations be queued?
Yes
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.
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 :-)
Re: [RFC PATCHv5 1/7] HSI: hsi: Introducing HSI framework
Hi Sjur,
On Wed, 2011-06-22 at 21:11 +0200, ext Sjur Brændeland wrote:
Hi Carlos,
Some weeks ago I submitted a CAIF-HSI protocol driver for
Linux 3.0.1, located in drivers/net/caif in David Miller's net-next-2.6.
This driver depends on a platform specific glue-layer.
It would be nice to adapt to a generic HSI API, so I'm looking forward
to see a this patch going upstream.
I have tried to investigate if this API proposal fulfills the
needs for the CAIF HSI protocol used by the ST-Ericsson modems.
Please find my review comments below.
+/**
+ * struct hsi_config - Configuration for RX/TX HSI modules
+ * @mode: Bit transmission mode (STREAM or FRAME)
+ * @channels: Number of channels to use [1..16]
+ * @speed: Max bit transmission speed (Kbit/s)
Just for clarity maybe you should say if you meen 1000 bit/s or 1024 bit/s?
I meant kbit/s - 10^3 bit/s. I will change the capital K.
+ * @flow: RX flow type (SYNCHRONIZED or PIPELINE)
+ * @arb_mode: Arbitration mode for TX frame (Round robin, priority)
+ */
+struct hsi_config {
+unsigned intmode;
+unsigned intchannels;
+unsigned intspeed;
frame_size: Can we assume 32 bits the only supported frame size,
or should this be configurable?
Correct me if I am wrong but the HSI spec sets 32 bits frame size into
stone. I know some HW (like OMAP SSI) allows changing the frame size,
but I can not for see why an hsi_client will need a lower frame size. If
a weird client needs this then this information should be pass to the
controller through other means.
+union {
+unsigned intflow; /* RX only */
+unsigned intarb_mode; /* TX only */
+};
It would be usefull with the following RX counters:
unsigned intframe_timeout_counter:16;
unsigned inttailing_bit_counter:8;
unsigned intframe_burst_counter:8;
+};
IMHO clients do not need to care about this HW specific values. I will
pass this through the controller in some other way (e.g: platform_data)
and use the same values for all the clients.
...
+
+/**
+ * struct hsi_board_info - HSI client board info
+ * @name: Name for the HSI device
+ * @hsi_id: HSI controller id where the client sits
+ * @port: Port number in the controller where the client sits
+ * @tx_cfg: HSI TX configuration
+ * @rx_cfg: HSI RX configuration
+ * @platform_data: Platform related data
+ * @archdata: Architecture-dependent device data
+ */
+struct hsi_board_info {
+const char *name;
+unsigned inthsi_id;
+unsigned intport;
+struct hsi_config tx_cfg;
+struct hsi_config rx_cfg;
+void*platform_data;
+struct dev_archdata *archdata;
+};
What about information about the supported transmission speeds?
Is it any way to obtain this information, so we can know the legal
values for speed in hsi_config?
For TX speed sets the max tx speed the HSI should go. The controller
driver will try to get as close as it possible to that value without
going over.
For the RX path it sets min RX speed the HSI should use. The controller
should ensure that it does not drop under that value to avoid breaking
the communication.
All this values need to be and can be known beforehand and are platform
specific.
Can we really assume that all HW supports linked DMA jobs (scatter list),
or do we need some information about DMA support in board_info as well?
No we don't assume anything and we do not need DMA support info in the
board_info. The hsi_controller will know. I think Linus Walleij already
make a comment on this ;)
...
+/**
+ * struct hsi_msg - HSI message descriptor
+ * @link: Free to use by the current descriptor owner
+ * @cl: HSI device client that issues the transfer
+ * @sgt: Head of the scatterlist array
+ * @context: Client context data associated to the transfer
+ * @complete: Transfer completion callback
+ * @destructor: Destructor to free resources when flushing
+ * @status: Status of the transfer when completed
I guess you refer to the enum HSI message status codes.
I think this would be more readable if you don't use anynomus enums,
but use enum-name to reference the enum here in the documentation.
Hmm HSI_STATUS_X I think it is quite self explanatory.
+ * @actual_len: Actual length of data transfered on completion
+ * @channel: Channel were to TX/RX the message
+ * @ttype: Transfer type (TX if set, RX otherwise)
+ * @break_frame: if true HSI will send/receive a break frame. Data buffers
are
+ * ignored in the request.
+ */
+struct hsi_msg {
+struct list_headlink;
+struct hsi_client *cl;
+struct sg_table sgt;
+void*context;
+
+void(*complete)(struct hsi_msg *msg);
+void
Re: [RFC PATCHv5 1/7] HSI: hsi: Introducing HSI framework
On Wed, Jun 22, 2011 at 9:11 PM, Sjur Brændeland sjur.brandel...@stericsson.com wrote: +/* + * API for HSI clients + */ +int hsi_async(struct hsi_client *cl, struct hsi_msg *msg); + +/** I'm pleased to see scatter list support. But is this supported by all HW? What is the behavior if HW doesn't support this, will hsi_async fail, or is the scatter-list handled 'under the hood'? I think it's pretty straight-forward even if you have to use PIO (IRQs and CPU-filling of FIFO). You simply use a memory iterator (sg_miter), so for example the MMC layer can only handle SGlists and in mmci.c we use sg_miter_start/next/stop to do this in the PIO case. If that kind of hardware would ever be fast enough for anyone using a HSI link is a good question though :-) Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCHv5 1/7] HSI: hsi: Introducing HSI framework
Hi Carlos,
Some weeks ago I submitted a CAIF-HSI protocol driver for
Linux 3.0.1, located in drivers/net/caif in David Miller's net-next-2.6.
This driver depends on a platform specific glue-layer.
It would be nice to adapt to a generic HSI API, so I'm looking forward
to see a this patch going upstream.
I have tried to investigate if this API proposal fulfills the
needs for the CAIF HSI protocol used by the ST-Ericsson modems.
Please find my review comments below.
+/**
+ * struct hsi_config - Configuration for RX/TX HSI modules
+ * @mode: Bit transmission mode (STREAM or FRAME)
+ * @channels: Number of channels to use [1..16]
+ * @speed: Max bit transmission speed (Kbit/s)
Just for clarity maybe you should say if you meen 1000 bit/s or 1024 bit/s?
+ * @flow: RX flow type (SYNCHRONIZED or PIPELINE)
+ * @arb_mode: Arbitration mode for TX frame (Round robin, priority)
+ */
+struct hsi_config {
+ unsigned intmode;
+ unsigned intchannels;
+ unsigned intspeed;
frame_size: Can we assume 32 bits the only supported frame size,
or should this be configurable?
+ union {
+ unsigned intflow; /* RX only */
+ unsigned intarb_mode; /* TX only */
+ };
It would be usefull with the following RX counters:
unsigned intframe_timeout_counter:16;
unsigned inttailing_bit_counter:8;
unsigned intframe_burst_counter:8;
+};
...
+
+/**
+ * struct hsi_board_info - HSI client board info
+ * @name: Name for the HSI device
+ * @hsi_id: HSI controller id where the client sits
+ * @port: Port number in the controller where the client sits
+ * @tx_cfg: HSI TX configuration
+ * @rx_cfg: HSI RX configuration
+ * @platform_data: Platform related data
+ * @archdata: Architecture-dependent device data
+ */
+struct hsi_board_info {
+ const char *name;
+ unsigned inthsi_id;
+ unsigned intport;
+ struct hsi_config tx_cfg;
+ struct hsi_config rx_cfg;
+ void*platform_data;
+ struct dev_archdata *archdata;
+};
What about information about the supported transmission speeds?
Is it any way to obtain this information, so we can know the legal
values for speed in hsi_config?
Can we really assume that all HW supports linked DMA jobs (scatter list),
or do we need some information about DMA support in board_info as well?
...
+/**
+ * struct hsi_msg - HSI message descriptor
+ * @link: Free to use by the current descriptor owner
+ * @cl: HSI device client that issues the transfer
+ * @sgt: Head of the scatterlist array
+ * @context: Client context data associated to the transfer
+ * @complete: Transfer completion callback
+ * @destructor: Destructor to free resources when flushing
+ * @status: Status of the transfer when completed
I guess you refer to the enum HSI message status codes.
I think this would be more readable if you don't use anynomus enums,
but use enum-name to reference the enum here in the documentation.
+ * @actual_len: Actual length of data transfered on completion
+ * @channel: Channel were to TX/RX the message
+ * @ttype: Transfer type (TX if set, RX otherwise)
+ * @break_frame: if true HSI will send/receive a break frame. Data buffers are
+ *ignored in the request.
+ */
+struct hsi_msg {
+ struct list_headlink;
+ struct hsi_client *cl;
+ struct sg_table sgt;
+ void*context;
+
+ void(*complete)(struct hsi_msg *msg);
+ void(*destructor)(struct hsi_msg *msg);
+
+ int status;
+ unsigned intactual_len;
size_t ?
+ unsigned intchannel;
+ unsigned intttype:1;
+ unsigned intbreak_frame:1;
+};
...
+/*
+ * API for HSI clients
+ */
+int hsi_async(struct hsi_client *cl, struct hsi_msg *msg);
+
+/**
I'm pleased to see scatter list support. But is this supported by all HW?
What is the behavior if HW doesn't support this, will hsi_async fail, or
is the scatter-list handled 'under the hood'?
Q: Can multiple Read operations be queued?
Will multiple read queued operations result in chained DMA operations, or
single read operations?
...
+/**
+ * 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.
Flushing the TX FIFO can be long duration operation due to HSI flow control,
if counterpart
