can: c_can: TX handling

Hi all,

I did some more testing on the SocketCAN driver for the Bosch c_can controller and I observed some strange behaviour for the TX handling. First of all the TX bytes are not accounted correctly. The reason for that seems to be quite obvious if we look into c_can_do_tx():

[...]

c_can_inval_msg_object(dev, 0, msg_obj_no); val = c_can_read_reg32(priv, &priv->regs->txrqst1);

if (!(val & (1 << msg_obj_no))) {
        can_get_echo_skb(dev,
                        msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
        stats->tx_bytes += priv->read_reg(priv,
                        &priv->regs->ifregs[0].msg_cntrl)
                        & IF_MCONT_DLC_MASK;

So, we first invalidate the message object and afterwards we read the DLC value from the msg_cntrl (which is 0 after invalidating the message object) to account the TX bytes. So tx_bytes will always be 0. The fix should be easy, I think, we can just move c_can_inval_msg_object to the end of that loop.

Cheers,
Jan

Quoted 5 lines

So, we first invalidate the message object and afterwards we read the DLC value from the msg_cntrl (which is 0 after invalidating the message object) to account the TX bytes. So tx_bytes will always be
0. The fix should be easy, I think, we can just move c_can_inval_msg_object to the end of that loop.

IMO, it looks necessary to call c_can_inval_msg_object inside the if (), after it has been transmitted. Otherwise, if for some other (TX) reason you get in this loop, you may clear a pending transmission? Again, I haven't read this one's datasheet. I was familiar with its predecessor.

Hi,

Quoted 5 lines

Quoted 5 lines

So, we first invalidate the message object and afterwards we read the DLC value from the msg_cntrl (which is 0 after invalidating the message object) to account the TX bytes. So tx_bytes will always be
0. The fix should be easy, I think, we can just move c_can_inval_msg_object to the end of that loop.

Quoted 5 lines

IMO, it looks necessary to call c_can_inval_msg_object inside the if (), after it has been transmitted. Otherwise, if for some other (TX) reason you get in this loop, you may clear a pending transmission? Again, I haven't read this one's datasheet. I was familiar with its predecessor.

I tried to check, but the datasheet is a bit unclear regarding that point ;-) My interpretation is, that c_can_inval_msg_object() shouldn't affect the txrqst bit, but nevertheless calling it inside the if() statement would make the code more readable.

I can prepare a patch, but I'd like to wait for Wolfgang's / Bupesh's feedback.

This means that "ifconfig" will report *0* TX Bytes!?

I'm following the discussion and as I'm not familiar with that chip, I'm waiting for Bupesh's answer as well... before I start digging. You can also have a look to the pch_can driver, which is C_CAN based as well.

Hi all,

The second problem is related to tx_next, which should hold the number of the oldest CAN frame, which was not on the line:

for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
        msg_obj_no = get_tx_echo_msg_obj(priv);
        c_can_inval_msg_object(dev, 0, msg_obj_no);
        val = c_can_read_reg32(priv, &priv->regs->txrqst1);
        if (!(val & (1 << msg_obj_no))) {
                can_get_echo_skb(dev,
                                msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
                stats->tx_bytes += priv->read_reg(priv,
                                &priv->regs->ifregs[0].msg_cntrl)
                                & IF_MCONT_DLC_MASK;
                stats->tx_packets++;
        }
}

But tx_echo is incremented unconditionally and we don't actually track the number of the oldest unsent frame.
Let's assume the following scenario: We bring up can0 and send 3
frames: TX object: 0, 1, 2; 1 and 2 make it on the line, but 0 is still pending. If we go through the above loop in that situation, we will skip message object 0, because the txrqst bit is still set. We will account message object 1 and 2. That's correct, but afterwards tx_echo is set to 2, BUT the oldest message which is pending is 0. Am I right or did I get something wrong?

The operation of c_can_do_tx() is described as follows: "We iterate from priv->tx_echo to priv->tx_next and check if the packet has been transmitted, echo it back to the CAN framework. If we discover a not yet transmitted package, stop looking for more." The actual implementation doesn't seem to stop if we discover a not yet transmitted package. But I'm not sure if just stopping might be a good idea, because in that case, the echo skb for already transmitted messages might be delayed by not yet transmitted messages.

Cheers,
Jan

I split your 2 questions in 2 replies.

not sure if I made my point. Note that this will eliminate the need for explicit wrap-around. It's done implicitely.

Hi,

Quoted 1 line

I split your 2 questions in 2 replies.

Thanks :)

Quoted 2 lines

not sure if I made my point. Note that this will eliminate the need for explicit wrap-around. It's done implicitely.

Hmmm, I double-checked the datasheet, which gives the following statement:
"The receive/transmit priority for the Message Objects is attached to the message number. Message Object 1 has the highest priority, while Message Object 32 has the lowest priority. If more than one transmission request is pending, they are serviced due to the priority of the corresponding Message Object."

I'm quite new to Bosch's c_can, so maybe Bhupesh can give some feedback (or beat me for causing some confusion ;-)).

Sorry for the confusion!
Jan

Hi,

Quoted 1 line

I split your 2 questions in 2 replies.

Thanks :)

Quoted 2 lines

not sure if I made my point. Note that this will eliminate the need for explicit wrap-around. It's done implicitely.

Hmmm, I double-checked the datasheet, which gives the following statement:
"The receive/transmit priority for the Message Objects is attached to the message number. Message Object 1 has the highest priority, while Message Object 32 has the lowest priority. If more than one transmission request is pending, they are serviced due to the priority of the corresponding Message Object."

So, we shouldn't run into the scenario I described in my previous mail

and the existing implementation should be OK, right?! I'm quite sure I've seen a situation where msg_obj 17 "seemed" to be pending, while msg_obj 18 and 19 already have been transmitted. But in that case, I enabled ONESHOT for the can interface, which enables the DA mode (automatic
retransmission is disabled). The errata sheet for c_can covers that

mode. There's a problem with "Concurrent transmission requests" and I'm quite sure my test-case hit that problem.

I'm quite new to Bosch's c_can, so maybe Bhupesh can give some feedback (or beat me for causing some confusion ;-)).

Hi all,

I did some more testing on the SocketCAN driver for the Bosch c_can controller and I observed some strange behaviour for the TX handling. First of all the TX bytes are not accounted correctly. The reason for that
seems to be quite obvious if we look into c_can_do_tx():

[...]

c_can_inval_msg_object(dev, 0, msg_obj_no); val = c_can_read_reg32(priv, &priv->regs->txrqst1);

if (!(val & (1 << msg_obj_no))) {
        can_get_echo_skb(dev,
                        msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
        stats->tx_bytes += priv->read_reg(priv,
                        &priv->regs->ifregs[0].msg_cntrl)
                        & IF_MCONT_DLC_MASK;

So, we first invalidate the message object and afterwards we read the DLC
value from the msg_cntrl (which is 0 after invalidating the message object) to account the TX bytes. So tx_bytes will always be 0. The
fix should be easy, I think, we can just move c_can_inval_msg_object to the end of that loop.

The second problem is related to tx_next, which should hold the number of
the oldest CAN frame, which was not on the line:

for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
        msg_obj_no = get_tx_echo_msg_obj(priv);
        c_can_inval_msg_object(dev, 0, msg_obj_no);
        val = c_can_read_reg32(priv, &priv->regs->txrqst1);
        if (!(val & (1 << msg_obj_no))) {
                can_get_echo_skb(dev,
                                msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
                stats->tx_bytes += priv->read_reg(priv,
                                &priv->regs->ifregs[0].msg_cntrl)
                                & IF_MCONT_DLC_MASK;
                stats->tx_packets++;
        }
}

But tx_echo is incremented unconditionally and we don't actually track the
number of the oldest unsent frame.
Let's assume the following scenario: We bring up can0 and send 3
frames: TX object: 0, 1, 2; 1 and 2 make it on the line, but 0 is still pending. If we go through the above loop in that situation, we will
skip message object 0, because the txrqst bit is still set. We will account message object 1 and 2. That's correct, but afterwards tx_echo is
set to 2, BUT the oldest message which is pending is 0. Am I right or did
I get something wrong?
The operation of c_can_do_tx() is described as follows: "We iterate from

priv->tx_echo to priv->tx_next and check if the packet has been transmitted, echo it back to the CAN framework. If we discover a not yet
transmitted package, stop looking for more." The actual implementation doesn't seem to stop if we discover a not yet transmitted package. But I'm not sure if just stopping might be a good idea, because in that case, the echo skb for already transmitted messages might be delayed by not yet transmitted messages.

Thanks for your work on c_can.
This week has been very busy for me, so please excuse me for the late reply.

Just to better understand this can you send me your *candump* output when you see that the message object put earlier in the message RAM being pending on the line.

Your point seems valid but I want to make sure that this is not done by the C_CAN core implicitly..

Quoted 17 lines

I did some more testing on the SocketCAN driver for the Bosch c_can controller and I observed some strange behaviour for the TX handling. First of all the TX bytes are not accounted correctly. The reason for that
seems to be quite obvious if we look into c_can_do_tx():

[...]

c_can_inval_msg_object(dev, 0, msg_obj_no); val = c_can_read_reg32(priv, &priv->regs->txrqst1);

if (!(val & (1 << msg_obj_no))) {
        can_get_echo_skb(dev,
                        msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
        stats->tx_bytes += priv->read_reg(priv,
                        &priv->regs->ifregs[0].msg_cntrl)
                        & IF_MCONT_DLC_MASK;

Quoted 4 lines

So, we first invalidate the message object and afterwards we read the DLC
value from the msg_cntrl (which is 0 after invalidating the message object) to account the TX bytes. So tx_bytes will always be

0.

Quoted 3 lines

The
fix should be easy, I think, we can just move
c_can_inval_msg_object to the end of that loop.

Hmmm..
As far as I remember, *ifconfig* used to provide the correct tx_bytes stats
on my SPEAr board. Let me recheck this again today evening and get back to you.

Hi Bhupesh,

as discussed I moved c_can_inval_msg_object() to the end of the if() statement. That should fix the tx_bytes accounting. For me it's working fine now.

Signed-off-by Jan Altenberg <[email protected]>

---

The current SocketCAN implementation for the Bosch c_can cell doesn't account the TX bytes correctly, because it calls c_can_inval_msg_object() (which clears the msg ctrl register) before reading the DLC value:

The fix is quite easy: Just move c_can_inval_msg_object() to the end of the if() statement. So:
* We only call c_can_inval_msg_object() if the message was actually transmitted
* We read out the DLC value _before_ clearing the msg ctrl register

Signed-off-by Jan Altenberg <[email protected]>

Quoted 12 lines · Jan Altenberg

The current SocketCAN implementation for the Bosch c_can cell doesn't account the TX bytes correctly, because it calls c_can_inval_msg_object() (which clears the msg ctrl register) before reading the DLC value:

The fix is quite easy: Just move c_can_inval_msg_object() to the end of the if() statement. So:
* We only call c_can_inval_msg_object() if the message was actually transmitted
* We read out the DLC value _before_ clearing the msg ctrl register

Quoted 1 line

Signed-off-by Jan Altenberg <[email protected]>

Acked-by Kurt Van Dijck <kurt.van.dijck-/[email protected]>

Quoted 13 lines · Jan Altenberg

Quoted 12 lines

The current SocketCAN implementation for the Bosch c_can cell doesn't account the TX bytes correctly, because it calls c_can_inval_msg_object() (which clears the msg ctrl register) before reading the DLC value:

The fix is quite easy: Just move c_can_inval_msg_object() to the end of the if() statement. So:
* We only call c_can_inval_msg_object() if the message was actually transmitted
* We read out the DLC value _before_ clearing the msg ctrl register

Quoted 2 lines

Quoted 1 line

Signed-off-by Jan Altenberg <[email protected]>

Acked-by Kurt Van Dijck <kurt.van.dijck-/[email protected]>

Acked-by Wolfgang Grandegger <[email protected]>