On 09/04/2009 11:47 AM, Gole, Anant wrote: > 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.
OK, it's fine if the manual mandates it. Maybe a warning message (dev_warn) would be a nice to have. > [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. Can you show the output of canecho_gen? > 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. The driver *must* ensure that messages go out and come in order. The problem arises because you use more than one TX/RX buffer, e.g. the SJA1000 does not have that problem. Lost packets are a different issue. > [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? OK, if the interrupt comes, one of the counters is greater than 127, at least, and for sure. Just if both have reached 128, your method is more correct, indeed. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
