Removed Netdev list as there are not many comments from network folks. Answering to both Wolfgang and Marc's review comments
>-----Original Message----- >From: Wolfgang Grandegger [mailto:[email protected]] >Sent: Thursday, September 03, 2009 2:30 PM >To: Gole, Anant >Cc: [email protected]; [email protected] >Subject: Re: [PATCH V2] net-next-2.6:can: add TI CAN (HECC) driver > >Anant Gole wrote: >> TI HECC (High End CAN Ctonroller) module is found on many TI devices. >> It has 32 hardware mailboxes with full implementation of CAN protocol >> version 2.0B with bus speeds up to 1Mbps. Specifications of the >> module are available at TI web <http://www.ti.com> >> >> This driver is tested on a newer OMAP device EVM. >> >> Signed-off-by: Anant Gole <[email protected]> > >Much better now :-). Still a few issues, though. > Ack your comments. I will work through those. [snip] >> +static int ti_hecc_set_btc(struct ti_hecc_priv *priv) >> +{ >> + struct can_bittiming *bit_timing = &priv->can.bittiming; >> + u32 can_btc = 0; >> + >> + can_btc = ((bit_timing->phase_seg2 - 1) & 0x7); >> + can_btc |= (((bit_timing->phase_seg1 + bit_timing->prop_seg - 1) >> + & 0xF) << 3); >> + if ((bit_timing->brp > 4) && >> + (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)) >> + can_btc |= HECC_CANBTC_SAM; > >Why must brp be greater than 4 to allow triple sampling? HECC specs mandates it. Honestly I do not know the hardware reason for this. [snip] >Ensuring proper ordering of out-going messages is tricky. I suggest >using Vladislav's test programs posted recently for validation: > >https://lists.berlios.de/pipermail/socketcan-core/2009-August/002871.html I tried canecho utility and am indeed seeing issues - I am wondering if I am losing RX packets due to deferred handling. Will debug further. I am wondering what do higher level protocols (like CANOpen) do to take care of out of order and lost packets? Since the CAN packet is only 8 bytes there is not much room for sync's. [snip] >> + if (int_status & HECC_CANGIF_EPIF) { /* error passive int */ >> + if (0 == (int_status & HECC_CANGIF_BOIF)) { >> + priv->can.state = CAN_STATE_ERROR_PASSIVE; >> + ++priv->can.can_stats.error_passive; >> + cf->can_id |= CAN_ERR_CRTL; >> + if (hecc_read(priv, HECC_CANTEC) > 127) >> + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE; >> + if (hecc_read(priv, HECC_CANREC) > 127) >> + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE; > >Currently we set the flag as show: > > cf->data[1] = (txerr > rxerr) ? > CAN_ERR_CRTL_TX_PASSIVE : > CAN_ERR_CRTL_RX_PASSIVE; > >Making clear that the state change was triggered by TX or RX. If this is >the best choice for the user is debatable, though. > Since we do not know what triggered the warning/passive state isn't it a good idea to let the user know about both TX and RX? Marc's comments: >-----Original Message----- >From: Marc Kleine-Budde [mailto:[email protected]] >Sent: Thursday, September 03, 2009 4:48 PM >To: Gole, Anant >Cc: [email protected]; [email protected] >Subject: Re: [PATCH V2] net-next-2.6:can: add TI CAN (HECC) driver > >Hey Anant, > >some comments inlne: Ack on your comments. Will work on those. [snip] >If I see correct, you number of mailboxes and and numer of prios are >power of two. You can hold prio and mailbox numer in one variable: the >lower bits hold the mailbox number the middle ones the prio, the >remaining are unused. Then you can remove the if() block here. You can >use mask and shift operations to access mailbox and prio. See at91_can Yes I looked at at91 but I wanted to keep the logic simple instead of using masks and counters ( yes this can be debatable :) ). But I will go over it again and see if the code size/number of lines in my tx/rx reduces with your suggestions of counters. Regards, Anant _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
