Wolfgang Grandegger wrote:
> Jan Kiszka wrote:
>> Wolfgang Grandegger wrote:
>>>>> @@ -894,6 +937,12 @@
>>>>>      /* We got access */
>>>>>  
>>>>>  
>>>>> +#ifdef CONFIG_XENO_DRIVERS_RTCAN_TX_LOOPBACK
>>>>> +    /* Push message onto stack for loopback when TX done */
>>>>> +    if (sock->tx_loopback)
>>>>> +    rtcan_tx_push(dev, sock, frame);
>>>>> +#endif /* CONFIG_XENO_DRIVERS_RTCAN_TX_LOOPBACK */
>>>> Hmm, I would prefer to define something like
>>>>
>>>> if (rtcan_tx_loopback_enabled(sock))
>>>>     rtcan_tx_push(dev, sock, frame);
>>>>
>>>> with rtcan_tx_loopback_enabled resolving to 0 in the configured-out
>>>> case. Thus some #ifdefs could be moved from the code to central places
>>>> in header files.
>>> OK, here I can avoid the #ifdef's ...
>>>
>>>>> Index: ksrc/drivers/can/mscan/rtcan_mscan.c
>>>>> ===================================================================
>>>>> --- ksrc/drivers/can/mscan/rtcan_mscan.c    (revision 1834)
>>>>> +++ ksrc/drivers/can/mscan/rtcan_mscan.c    (working copy)
>>>>> @@ -251,6 +251,21 @@
>>>>>      regs->cantier = 0;
>>>>>      /* Wake up a sender */
>>>>>      rtdm_sem_up(&dev->tx_sem);
>>>>> +
>>>>> +#ifdef CONFIG_XENO_DRIVERS_RTCAN_TX_LOOPBACK
>>>>> +    if (dev->tx_socket) {
>>>> Same here. A macro like rtcan_tx_loopback_pending(dev) would avoid the
>>>> #ifdef.
>>> ...but not here. Or does the optimizer remove the if block?
>>
>> AFAIK, all relevant gcc versions kill blocks like
>>
>> if (0) {
>>     ...
>> }
>>
>> from the binary. But one problem might appear: the block content must be
>> compilable in any case...
> 
> I'm going to checks this.
> 
>>>> I think the timestamp should rather be passed to rtcan_tc_loopback and
>>>> set there. Or, if passing that value increases the code size
>>>> needlessly,
>>>> rtcan_tx_loopback should be defined like
>>>>
>>>> static inline void
>>>> rtcan_tx_loopback(struct rtcan_device *dev, nanosecs_abs_t timestamp)
>>>> {
>>>>     <set timestamp>
>>>>     __rtcan_tx_loopback(dev);
>>>> }
>>>>
>>>> where __rtcan_tx_loopback is the uninlined code. That makes the driver
>>>> code leaner. Locking will hopefully change anyway, so nothing to do on
>>>> this part for now.
>>> Or do it directly in rtcan_tx_loopback() and rtcan_recv(). Would it be
>>> OK to read the time in these functions as well or should it be done, as
>>> is, at the beginning of the ISR? I have already planned some similar
>>> rewrite when Xenomai 2.3. is out, hopefully together with the new
>>> locking.
>>
>> A few cycles offset of the timestamp doesn't matter here. We should just
>> avoid reading it more than once as this can be a costly operation on
>> some systems.
> 
> Hm, what does "costly operation" mean? Does it make sense then to read
> the time only when really necessary e.g. not for pure TX done interrupts
> and if nobody request them?

I'm thinking about the TSC emulation on old or lowest-end x86 systems.
Also, converting the ticks into nanoseconds may take some cycles on slow
32 bit systems. So it might be a good idea to check - where possible -
if timestamps are needed.

Jan